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

[Bastien Curutchet]

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