linux-kernel - Re: [PATCH v2 2/4] leds: pca9532: Use PWM1 for hardware blinking

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240711083050.GK501857@google.com>
Date: Thu, 11 Jul 2024 09:30:50 +0100
From: Lee Jones <lee@...nel.org>
To: Bastien Curutchet <bastien.curutchet@...tlin.com>
Cc: Riku Voipio <riku.voipio@....fi>, Pavel Machek <pavel@....cz>,
	linux-leds@...r.kernel.org, linux-kernel@...r.kernel.org,
	Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
	Herve Codina <herve.codina@...tlin.com>,
	Christopher Cordahi <christophercordahi@...ometrics.ca>
Subject: Re: [PATCH v2 2/4] leds: pca9532: Use PWM1 for hardware blinking

On Wed, 10 Jul 2024, Bastien Curutchet wrote:

> Hi Lee,
> 
> On 6/17/24 16:39, Bastien Curutchet wrote:
> 
> > +static int pca9532_update_hw_blink(struct pca9532_led *led,
> > +				   unsigned long delay_on, unsigned long delay_off)
> > +{
> > +	struct pca9532_data *data = i2c_get_clientdata(led->client);
> > +	unsigned int psc;
> > +	int i;
> > +
> > +	/* Look for others LEDs that already use PWM1 */
> > +	for (i = 0; i < data->chip_info->num_leds; i++) {
> > +		struct pca9532_led *other = &data->leds[i];
> > +
> > +		if (other == led)
> > +			continue;
> > +
> > +		if (other->state == PCA9532_PWM1) {
> > +			if (other->ldev.blink_delay_on != delay_on ||
> > +			    other->ldev.blink_delay_off != delay_off) {
> > +				dev_err(&led->client->dev,
> > +					"HW can handle only one blink configuration at a time\n");
> 
> I'm having some second thoughts about this dev_err().
> 
> It was dev_dbg() in V1, but based on your suggestion, I changed it to
> dev_err() because an error was returned after.
> 
> I've worked more with this patch since it got applied, these error messages
> appear frequently, though they don’t seem to be 'real' errors to me as the
> software callback is used afterwards and the LED blinks at the expected
> period.
> 
> Don't you think a dev_dbg() would be more appropriate in this case ? Or
> perhaps a comment instead of a message ?

If it's not an error, then don't return an error message.

Maybe drop the message for a comment and return -EBUSY instead?

> > +				return -EINVAL;
> > +			}
> > +		}
> > +	}
> > +
> > +	psc = ((delay_on + delay_off) * PCA9532_PWM_PERIOD_DIV - 1) / 1000;
> > +	if (psc > U8_MAX) {
> > +		dev_err(&led->client->dev, "Blink period too long to be handled by hardware\n");
> 
> Same comments for this dev_err()
> 
> > +		return -EINVAL;
> > +	}
> 
> 
> Best regards,
> Bastien

-- 
Lee Jones [李琼斯]