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@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;
Did you just reinvent led_set_brightness_sync() and the only thing we
need is:
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
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
