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@xxxxxxxxxxxxxx>
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
