Re: [PATCH v4 1/3] led: make led_set_brightness_sync() use led_set_brightness_nosleep()

[Jean-Jacques Hiblot]

On 23/09/2019 23:03, Jacek Anaszewski wrote:
> Hi Jean,
>
> On 9/23/19 11:14 AM, Jean-Jacques Hiblot wrote:
> > Hi Jacek,
> >
> > On 20/09/2019 23:10, Jacek Anaszewski wrote:
> > > Hi Jean,
> > >
> > > On 9/20/19 2:25 PM, Jean-Jacques Hiblot wrote:
> > > > Making led_set_brightness_sync() use led_set_brightness_nosleep() has 2
> > > > advantages:
> > > > - works for LED controllers that do not provide
> > > > brightness_set_blocking()
> > > > - When the blocking callback is used, it uses the workqueue to update
> > > > the
> > > >  ÂÂ LED state, removing the need for mutual exclusion between
> > > >  ÂÂ led_set_brightness_sync() and set_brightness_delayed().
> > > And third:
> > >
> > > - it compromises the "sync" part of the function name :-)
> > Making it sync is the role of the flush_work() function. It waits until
> > the deferred work has been done.
> The thing is not in the blocking character of the function, but rather
> in the fastest possible way of setting torch brightness.
> led_set_brightness_nosleep() will defer brightness_set_blocking op
> to the workqueue so this condition will not be met then.

OK. I see the point there.

> This function was added specifically for LED class flash v4l2 wrapper:
> drivers/media/v4l2-core/v4l2-flash-led-class.c.
>
> It may need an addition of support for brightness_set only drivers,
> but we haven't had a use case so far, since all client flash LED
> controllers are driven via blocking buses (there are not many of them).
>
> Also, when LED flash class (and thus LED class also as a parent)
> is hijacked by v4l2-flash-led wrapper, its sysfs is disabled,
> so it is not possible to set e.g. timer trigger which could
> interfere with the led_set_brightness_sync() (and it also returns
> -EBUSY when blinking is enabled).

Then this is a really special use case and we don't really have to 
worry about synchronization with the other ways to set the LED 
brightness. I'll drop any change to this function then.

Thanks for the detailed explanation.

JJ


> > > This function has been introduced specifically to be blocking
> > > and have the immediate effect. Its sole client is
> > > drivers/media/v4l2-core/v4l2-flash-led-class.c.
> > >
> > > > Signed-off-by: Jean-Jacques Hiblot <jjhiblot@xxxxxx>
> > > > ---
> > > >  Â drivers/leds/led-core.c | 12 +++++++-----
> > > >  Â 1 file changed, 7 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
> > > > index f1f718dbe0f8..50e28a8f9357 100644
> > > > --- a/drivers/leds/led-core.c
> > > > +++ b/drivers/leds/led-core.c
> > > > @@ -294,15 +294,17 @@ EXPORT_SYMBOL_GPL(led_set_brightness_nosleep);
> > > >  Â int led_set_brightness_sync(struct led_classdev *led_cdev,
> > > >  ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ enum led_brightness value)
> > > >  Â {
> > > > +ÂÂÂ int ret;
> > > > +
> > > >  ÂÂÂÂÂ if (led_cdev->blink_delay_on || led_cdev->blink_delay_off)
> > > >  ÂÂÂÂÂÂÂÂÂ return -EBUSY;
> > > >  Â -ÂÂÂ led_cdev->brightness = min(value, led_cdev->max_brightness);
> > > > -
> > > > -ÂÂÂ if (led_cdev->flags & LED_SUSPENDED)
> > > > -ÂÂÂÂÂÂÂ return 0;
> > > > +ÂÂÂ ret = led_set_brightness_nosleep(led_cdev, value);
> > > > +ÂÂÂ if (!ret)
> > > > +ÂÂÂÂÂÂÂ return ret;
> > > >  Â -ÂÂÂ return __led_set_brightness_blocking(led_cdev,
> > > > led_cdev->brightness);
> > > > +ÂÂÂ flush_work(&led_cdev->set_brightness_work);
> > > > +ÂÂÂ return 0;
> > > >  Â }
> > > >  Â EXPORT_SYMBOL_GPL(led_set_brightness_sync);
> > > >