[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <2672a3b6-6457-4460-9e86-895923b80cdc@bootlin.com>
Date: Thu, 11 Jul 2024 10:40:48 +0200
From: Bastien Curutchet <bastien.curutchet@...tlin.com>
To: Lee Jones <lee@...nel.org>
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
Hi Lee,
On 7/11/24 10:30, Lee Jones wrote:
> 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?
>
OK I'll do this, thank you.
>>> + return -EINVAL;
>>> + }
>>> + }
>>> + }
>>> +
Best regards,
Bastien
Powered by blists - more mailing lists