| Message ID | 20210503092542.14497-1-jbe@pengutronix.de |
|---|---|
| State | New |
| Headers | show |
| Series | leds: trigger/tty: Use led_set_brightness() to support all use cases | expand |
Hello Jürgen, On Mon, May 03, 2021 at 11:25:42AM +0200, Juergen Borleis wrote: > Using led_set_brightness_sync() only works for LEDs which are connected > via some kind of external bus like I²C or SPI. But it doesn't work for > the simple use case of directly connected LEDs via GPIOs. > Because this function only honors the led_classdev::brightness_set_blocking > callback. But the LED-GPIO driver registers the > led_classdev::brightness_set member if the GPIO can be modified directly > and thus, TTY triggers fail silently with -ENOTSUPP. > > With the previously used led_set_brightness() it works for both use cases. > This function first checks for the simple case where the GPIO can be changed > without additional overhead, and if it fails, does the modification via a > workqueue. > > Signed-off-by: Juergen Borleis <jbe@pengutronix.de> > --- > drivers/leds/trigger/ledtrig-tty.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/leds/trigger/ledtrig-tty.c b/drivers/leds/trigger/ledtrig-tty.c > index f62db7e..af61281 100644 > --- a/drivers/leds/trigger/ledtrig-tty.c > +++ b/drivers/leds/trigger/ledtrig-tty.c > @@ -122,12 +122,12 @@ static void ledtrig_tty_work(struct work_struct *work) > > if (icount.rx != trigger_data->rx || > icount.tx != trigger_data->tx) { > - led_set_brightness_sync(trigger_data->led_cdev, LED_ON); > + led_set_brightness(trigger_data->led_cdev, LED_ON); > > trigger_data->rx = icount.rx; > trigger_data->tx = icount.tx; > } else { > - led_set_brightness_sync(trigger_data->led_cdev, LED_OFF); > + led_set_brightness(trigger_data->led_cdev, LED_OFF); > } This problem still exists, right? I think the right thing here is to call led_set_brightness_nosleep() however. Having said that, I think there are too many variants of led_set_brightness which makes it difficult to pick the right one. (I'm aware of - led_set_brightness_nosleep - led_set_brightness_nopm - led_set_brightness_sync - led_set_brightness and there are a few more static variants in led-core.c (__led_set_brightness, __led_set_brightness_blocking, set_brightness_delayed).) Best regards Uwe
On Mon, 09 Jan 2023, Uwe Kleine-König wrote: > Hello Jürgen, > > On Mon, May 03, 2021 at 11:25:42AM +0200, Juergen Borleis wrote: > > Using led_set_brightness_sync() only works for LEDs which are connected > > via some kind of external bus like I²C or SPI. But it doesn't work for > > the simple use case of directly connected LEDs via GPIOs. > > Because this function only honors the led_classdev::brightness_set_blocking > > callback. But the LED-GPIO driver registers the > > led_classdev::brightness_set member if the GPIO can be modified directly > > and thus, TTY triggers fail silently with -ENOTSUPP. > > > > With the previously used led_set_brightness() it works for both use cases. > > This function first checks for the simple case where the GPIO can be changed > > without additional overhead, and if it fails, does the modification via a > > workqueue. > > > > Signed-off-by: Juergen Borleis <jbe@pengutronix.de> > > --- > > drivers/leds/trigger/ledtrig-tty.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/leds/trigger/ledtrig-tty.c b/drivers/leds/trigger/ledtrig-tty.c > > index f62db7e..af61281 100644 > > --- a/drivers/leds/trigger/ledtrig-tty.c > > +++ b/drivers/leds/trigger/ledtrig-tty.c > > @@ -122,12 +122,12 @@ static void ledtrig_tty_work(struct work_struct *work) > > > > if (icount.rx != trigger_data->rx || > > icount.tx != trigger_data->tx) { > > - led_set_brightness_sync(trigger_data->led_cdev, LED_ON); > > + led_set_brightness(trigger_data->led_cdev, LED_ON); > > > > trigger_data->rx = icount.rx; > > trigger_data->tx = icount.tx; > > } else { > > - led_set_brightness_sync(trigger_data->led_cdev, LED_OFF); > > + led_set_brightness(trigger_data->led_cdev, LED_OFF); > > } > > This problem still exists, right? > > I think the right thing here is to call led_set_brightness_nosleep() > however. > > Having said that, I think there are too many variants of > led_set_brightness which makes it difficult to pick the right one. > > (I'm aware of > - led_set_brightness_nosleep > - led_set_brightness_nopm > - led_set_brightness_sync > - led_set_brightness > > and there are a few more static variants in led-core.c > (__led_set_brightness, __led_set_brightness_blocking, > set_brightness_delayed).) This patch (and this response) was never sent to me. Not a great deal I can do if I'm not aware of it.
On Fri, 23 Feb 2024, Lee Jones wrote: > On Mon, 09 Jan 2023, Uwe Kleine-König wrote: > > > Hello Jürgen, > > > > On Mon, May 03, 2021 at 11:25:42AM +0200, Juergen Borleis wrote: > > > Using led_set_brightness_sync() only works for LEDs which are connected > > > via some kind of external bus like I²C or SPI. But it doesn't work for > > > the simple use case of directly connected LEDs via GPIOs. > > > Because this function only honors the led_classdev::brightness_set_blocking > > > callback. But the LED-GPIO driver registers the > > > led_classdev::brightness_set member if the GPIO can be modified directly > > > and thus, TTY triggers fail silently with -ENOTSUPP. > > > > > > With the previously used led_set_brightness() it works for both use cases. > > > This function first checks for the simple case where the GPIO can be changed > > > without additional overhead, and if it fails, does the modification via a > > > workqueue. > > > > > > Signed-off-by: Juergen Borleis <jbe@pengutronix.de> > > > --- > > > drivers/leds/trigger/ledtrig-tty.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/leds/trigger/ledtrig-tty.c b/drivers/leds/trigger/ledtrig-tty.c > > > index f62db7e..af61281 100644 > > > --- a/drivers/leds/trigger/ledtrig-tty.c > > > +++ b/drivers/leds/trigger/ledtrig-tty.c > > > @@ -122,12 +122,12 @@ static void ledtrig_tty_work(struct work_struct *work) > > > > > > if (icount.rx != trigger_data->rx || > > > icount.tx != trigger_data->tx) { > > > - led_set_brightness_sync(trigger_data->led_cdev, LED_ON); > > > + led_set_brightness(trigger_data->led_cdev, LED_ON); > > > > > > trigger_data->rx = icount.rx; > > > trigger_data->tx = icount.tx; > > > } else { > > > - led_set_brightness_sync(trigger_data->led_cdev, LED_OFF); > > > + led_set_brightness(trigger_data->led_cdev, LED_OFF); > > > } > > > > This problem still exists, right? > > > > I think the right thing here is to call led_set_brightness_nosleep() > > however. > > > > Having said that, I think there are too many variants of > > led_set_brightness which makes it difficult to pick the right one. > > > > (I'm aware of > > - led_set_brightness_nosleep > > - led_set_brightness_nopm > > - led_set_brightness_sync > > - led_set_brightness > > > > and there are a few more static variants in led-core.c > > (__led_set_brightness, __led_set_brightness_blocking, > > set_brightness_delayed).) > > This patch (and this response) was never sent to me. > > Not a great deal I can do if I'm not aware of it. Starting to think I've gone too far back in the LEDs ML's history? =;-)
diff --git a/drivers/leds/trigger/ledtrig-tty.c b/drivers/leds/trigger/ledtrig-tty.c index f62db7e..af61281 100644 --- a/drivers/leds/trigger/ledtrig-tty.c +++ b/drivers/leds/trigger/ledtrig-tty.c @@ -122,12 +122,12 @@ static void ledtrig_tty_work(struct work_struct *work) if (icount.rx != trigger_data->rx || icount.tx != trigger_data->tx) { - led_set_brightness_sync(trigger_data->led_cdev, LED_ON); + led_set_brightness(trigger_data->led_cdev, LED_ON); trigger_data->rx = icount.rx; trigger_data->tx = icount.tx; } else { - led_set_brightness_sync(trigger_data->led_cdev, LED_OFF); + led_set_brightness(trigger_data->led_cdev, LED_OFF); } out:
Using led_set_brightness_sync() only works for LEDs which are connected via some kind of external bus like I²C or SPI. But it doesn't work for the simple use case of directly connected LEDs via GPIOs. Because this function only honors the led_classdev::brightness_set_blocking callback. But the LED-GPIO driver registers the led_classdev::brightness_set member if the GPIO can be modified directly and thus, TTY triggers fail silently with -ENOTSUPP. With the previously used led_set_brightness() it works for both use cases. This function first checks for the simple case where the GPIO can be changed without additional overhead, and if it fails, does the modification via a workqueue. Signed-off-by: Juergen Borleis <jbe@pengutronix.de> --- drivers/leds/trigger/ledtrig-tty.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)