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

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

Hi Uwe,

On 4/18/23 00:27, Uwe Kleine-König wrote:
> hello Jacek,
> 
> On Mon, Apr 17, 2023 at 09:51:06PM +0200, Jacek Anaszewski wrote:
>> On 4/17/23 21:17, Uwe Kleine-König wrote:
>>> On Mon, Apr 17, 2023 at 08:33:31PM +0200, Jacek Anaszewski wrote:
>>>> On 4/17/23 14:44, Uwe Kleine-König wrote:
>>>>> 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;
> 
> Did you just reinvent led_set_brightness_sync() and the only thing we
> need is:

Actually you're right, but led_set_brightness_sync() needs to be
supplemented with the call to __led_set_brightness_blocking().

> 
> diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
> index ce4e79939731..4f718609032b 100644
> --- a/drivers/leds/leds-gpio.c
> +++ b/drivers/leds/leds-gpio.c
> @@ -83,8 +83,7 @@ static int create_gpio_led(const struct gpio_led *template,
>   	led_dat->can_sleep = gpiod_cansleep(led_dat->gpiod);
>   	if (!led_dat->can_sleep)
>   		led_dat->cdev.brightness_set = gpio_led_set;
> -	else
> -		led_dat->cdev.brightness_set_blocking = gpio_led_set_blocking;
> +	led_dat->cdev.brightness_set_blocking = gpio_led_set_blocking;
>   	led_dat->blinking = 0;
>   	if (blink_set) {
>   		led_dat->platform_gpio_blink_set = blink_set;
> 
> ?
> 
>> As a quick fix I would apply led_set_brightness_nosleep() nonetheless.
>> Does it have any observed downsides?
> 
> Best regards
> Uwe
> 

-- 
Best regards,
Jacek Anaszewski