Re: [PATCH v3 2/3] leds: Add control of the voltage/current regulator to the LED core

[Jean-Jacques Hiblot <jjhiblot@xxxxxx>]

On 18/07/2019 14:24, Jacek Anaszewski wrote:
> Hi Jean,
>
> Thank you for the updated patch set.
>
> I have some more comments below.
>
> On 7/17/19 3:59 PM, Jean-Jacques Hiblot wrote:
> >   
> > +static bool __led_need_regulator_update(struct led_classdev *led_cdev,
> > +					int brightness)
> > +{
> > +	bool new_state = (brightness != LED_OFF);
> How about:
>
> bool new_state = !!brightness;

Throughout the code LED_OFF is used when the LED is turned off. I think 
it would be more consistent to use it there too.

> > +
> > +	return led_cdev->regulator && led_cdev->regulator_state != new_state;
> > +}
> > +static int __led_handle_regulator(struct led_classdev *led_cdev,
> > +				int brightness)
> > +{
> > +	int rc;
> > +
> > +	if (__led_need_regulator_update(led_cdev, brightness)) {
> > +
> > +		if (brightness != LED_OFF)
> > +			rc = regulator_enable(led_cdev->regulator);
> > +		else
> > +			rc = regulator_disable(led_cdev->regulator);
> > +		if (rc)
> > +			return rc;
> > +
> > +		led_cdev->regulator_state = (brightness != LED_OFF);
> > +	}
> > +	return 0;
> > +}
> Let's have these function names without leading underscores.
OK.
> >   static int __led_set_brightness(struct led_classdev *led_cdev,
> >   				enum led_brightness value)
> >   {
> > @@ -115,6 +142,8 @@ static void set_brightness_delayed(struct work_struct *ws)
> >   	if (ret == -ENOTSUPP)
> >   		ret = __led_set_brightness_blocking(led_cdev,
> >   					led_cdev->delayed_set_value);
> > +	__led_handle_regulator(led_cdev, led_cdev->delayed_set_value)
> If you called it from __led_set_brightness() and

We cannot call it from __led_set_brightness() because it is supposed not 
to block.

JJ