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

[Jean-Jacques Hiblot]

Hi Jacek,

On 18/07/2019 19:49, Jacek Anaszewski wrote:
> On 7/18/19 3:31 PM, Jean-Jacques Hiblot wrote:
> > 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.
> Basically brightness is a scalar and 0 always means off.
> We treat enum led_brightness as a legacy type - it is no
> longer valid on the whole its span since LED_FULL = 255
> was depreciated with addition of max_brightness property.
>
> IMHO use of reverse logic here only hinders code analysis.
>
> > > > +
> > > > +ÂÂÂ 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.
> You're right. The problematic part is that with regulator handling
> we cannot treat the whole brightness setting operation uniformly
> for brightness_set op case, i.e. without mediation of a workqueue.
>
> Now you have to fire workqueue in led_set_brightness_nopm()
> even for brightness_set() op path, if regulator state needs update.
> This is ugly and can be misleading. Can be also error prone and
> have non-obvious implications for software blink state transitions.

Taking your queue I reworked the series to take better care of the 
concurrency issues.

I believe it's in better shape right now.

> I think we would first need to improve locking between the workqueue
> and led_timer_function(). I proposed a patch [0] over a year
> ago.

I tried the patch and get a lot of warning because of triggers on 
storage devices.

Making led_set_brightness() not callable from a IRQ context, is probably 
not the right approach anymore.


JJ

> Only then we could think of adding another asynchronous dependency
> to the brightness setting chain.
>
> [0] https://lkml.org/lkml/2018/1/17/1144