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: <b88d211f-3c5b-4428-bd94-3d089c56ed22@bootlin.com>
Date: Wed, 10 Jul 2024 11:17:18 +0200
From: Bastien Curutchet <bastien.curutchet@...tlin.com>
To: Riku Voipio <riku.voipio@....fi>, Pavel Machek <pavel@....cz>,
 Lee Jones <lee@...nel.org>
Cc: 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

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 ?

> +				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