Re: [PATCH] leds: trigger/tty: Use led_set_brightness_nosleep() to set brightness

[Jacek Anaszewski <jacek.anaszewski@gmail.com>]

On 4/17/23 21:17, Uwe Kleine-König wrote:
> On Mon, Apr 17, 2023 at 08:33:31PM +0200, Jacek Anaszewski wrote:
>> Hi Uwe,
>>
>> On 4/17/23 14:44, Uwe Kleine-König wrote:
>>> Hello,
>>>
>>> On Mon, Apr 17, 2023 at 02:28:52PM +0200, Pavel Machek wrote:
>>>>> After commit ba8a86e4dadb ("leds: trigger/tty: Use
>>>>> led_set_brightness_sync() from workqueue") this is the second try to
>>>>> pick the right function to set the LED brightness from a trigger.
>>>>>
>>>>> led_set_brightness_sync() has the problem that it doesn't work for LEDs
>>>>> without a .brightness_set_blocking() callback. This is (among others)
>>>>> the case for LEDs connected to non-sleeping GPIOs.
>>>>>
>>>>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>>>>
>>>> I don't think this is right.
>>>>
>>>> _nosleep calls _nopm, which assmues it can't sleep, and schedules
>>>> another workqueue to set the LED.
>>>
>>> Then which is the right variant?
>>> led_set_brightness() and led_set_brightness_nosleep() set via a workqueue
>>> (which is bad) and led_set_brightness_sync() doesn't work for some LEDs
>>> (notably LEDs on non-sleeping GPIOs).
>>
>> Can you remind me the context of this patch, why using workqueue is
>> bad here?
> 
> The workqueue is only needed if you have a slow LED and want to set the
> brightness in atomic context. However if you are allowed to sleep that
> is just needless overhead.

OK, now I get it. So new functions will be needed, something like
below (skipped args, need more thinking about corner cases, e.g.
interactions with led_classdev_suspend()):

int led_set_brightness_cansleep()
     led_cdev->brightness = min(value, led_cdev->max_brightness);

     if (led_cdev->flags & LED_SUSPENDED)
         return 0;

     return led_set_brightness_nopm_cansleep();


int led_set_brightness_nopm_cansleep()
     ret =__led_set_brightness();
     if (ret == -ENOTSUPP)
         ret = __led_set_brightness_blocking();

     return ret;

As a quick fix I would apply led_set_brightness_nosleep() nonetheless.
Does it have any observed downsides?

-- 
Best regards,
Jacek Anaszewski