[PATCH 1/1] leds: ledtrig-tty.c: call led_set_brightness() when the blocking callback is not available

[PATCH 1/1] leds: ledtrig-tty.c: call led_set_brightness() when the blocking callback is not available

[Qingtao Cao]

The Marvell GPIO controller's driver will setup its struct gpio_chip's can_sleep
false, making create_gpio_led() setting up the brightness_set function pointer
instead of the brightness_set_blocking function pointer. In this case the
nonblocking version led_set_brightness() should be fallen back on by ledtrig_tty_work()

Signed-off-by: Qingtao Cao <qingtao.cao@digi.com>
---
 drivers/leds/trigger/ledtrig-tty.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/leds/trigger/ledtrig-tty.c b/drivers/leds/trigger/ledtrig-tty.c
index f62db7e520b5..e43d285b5d06 100644
--- a/drivers/leds/trigger/ledtrig-tty.c
+++ b/drivers/leds/trigger/ledtrig-tty.c
@@ -122,12 +122,18 @@ 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);
+		if (trigger_data->led_cdev->brightness_set_blocking)
+			led_set_brightness_sync(trigger_data->led_cdev, LED_ON);
+		else if (trigger_data->led_cdev->brightness_set)
+			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);
+		if (trigger_data->led_cdev->brightness_set_blocking)
+			led_set_brightness_sync(trigger_data->led_cdev, LED_OFF);
+		else if (trigger_data->led_cdev->brightness_set)
+			led_set_brightness(trigger_data->led_cdev, LED_OFF);
 	}
 
 out:
-- 
2.34.1

Re: [PATCH 1/1] leds: ledtrig-tty.c: call led_set_brightness() when the blocking callback is not available

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1803 bytes --]

On Fri, Dec 09, 2022 at 10:10:38AM +1000, Qingtao Cao wrote:
> The Marvell GPIO controller's driver will setup its struct gpio_chip's can_sleep
> false, making create_gpio_led() setting up the brightness_set function pointer
> instead of the brightness_set_blocking function pointer. In this case the
> nonblocking version led_set_brightness() should be fallen back on by ledtrig_tty_work()
> 
> Signed-off-by: Qingtao Cao <qingtao.cao@digi.com>
> ---
>  drivers/leds/trigger/ledtrig-tty.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/leds/trigger/ledtrig-tty.c b/drivers/leds/trigger/ledtrig-tty.c
> index f62db7e520b5..e43d285b5d06 100644
> --- a/drivers/leds/trigger/ledtrig-tty.c
> +++ b/drivers/leds/trigger/ledtrig-tty.c
> @@ -122,12 +122,18 @@ 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);
> +		if (trigger_data->led_cdev->brightness_set_blocking)
> +			led_set_brightness_sync(trigger_data->led_cdev, LED_ON);
> +		else if (trigger_data->led_cdev->brightness_set)
> +			led_set_brightness(trigger_data->led_cdev, LED_ON);

I had similar issues in the past where a function call worked for
sleeping LEDs but not atomic ones (or the other way around? Don't
remember the details.)

I wonder if there isn't a function that does the right thing no matter
what type the LED is. The other triggers should have the same issue, and
doing the above check in all of them just looks wrong.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/1] leds: ledtrig-tty.c: call led_set_brightness() when the blocking callback is not available

[Lee Jones]

On Fri, 09 Dec 2022, Uwe Kleine-König wrote:

> On Fri, Dec 09, 2022 at 10:10:38AM +1000, Qingtao Cao wrote:
> > The Marvell GPIO controller's driver will setup its struct gpio_chip's can_sleep
> > false, making create_gpio_led() setting up the brightness_set function pointer
> > instead of the brightness_set_blocking function pointer. In this case the
> > nonblocking version led_set_brightness() should be fallen back on by ledtrig_tty_work()
> > 
> > Signed-off-by: Qingtao Cao <qingtao.cao@digi.com>
> > ---
> >  drivers/leds/trigger/ledtrig-tty.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/leds/trigger/ledtrig-tty.c b/drivers/leds/trigger/ledtrig-tty.c
> > index f62db7e520b5..e43d285b5d06 100644
> > --- a/drivers/leds/trigger/ledtrig-tty.c
> > +++ b/drivers/leds/trigger/ledtrig-tty.c
> > @@ -122,12 +122,18 @@ 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);
> > +		if (trigger_data->led_cdev->brightness_set_blocking)
> > +			led_set_brightness_sync(trigger_data->led_cdev, LED_ON);
> > +		else if (trigger_data->led_cdev->brightness_set)
> > +			led_set_brightness(trigger_data->led_cdev, LED_ON);
> 
> I had similar issues in the past where a function call worked for
> sleeping LEDs but not atomic ones (or the other way around? Don't
> remember the details.)
> 
> I wonder if there isn't a function that does the right thing no matter
> what type the LED is. The other triggers should have the same issue, and
> doing the above check in all of them just looks wrong.

Anyone fancy taking a deeper dive into this?

I'd usually have a go myself, but I'm presently swamped.

Pavel already has an idea?

-- 
Lee Jones [李琼斯]

Re: [PATCH 1/1] leds: ledtrig-tty.c: call led_set_brightness() when the blocking callback is not available

[Jacek Anaszewski]

Hi all,

On 1/4/23 17:20, Lee Jones wrote:
> On Fri, 09 Dec 2022, Uwe Kleine-König wrote:
> 
>> On Fri, Dec 09, 2022 at 10:10:38AM +1000, Qingtao Cao wrote:
>>> The Marvell GPIO controller's driver will setup its struct gpio_chip's can_sleep
>>> false, making create_gpio_led() setting up the brightness_set function pointer
>>> instead of the brightness_set_blocking function pointer. In this case the
>>> nonblocking version led_set_brightness() should be fallen back on by ledtrig_tty_work()
>>>
>>> Signed-off-by: Qingtao Cao <qingtao.cao@digi.com>
>>> ---
>>>   drivers/leds/trigger/ledtrig-tty.c | 10 ++++++++--
>>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/leds/trigger/ledtrig-tty.c b/drivers/leds/trigger/ledtrig-tty.c
>>> index f62db7e520b5..e43d285b5d06 100644
>>> --- a/drivers/leds/trigger/ledtrig-tty.c
>>> +++ b/drivers/leds/trigger/ledtrig-tty.c
>>> @@ -122,12 +122,18 @@ 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);
>>> +		if (trigger_data->led_cdev->brightness_set_blocking)
>>> +			led_set_brightness_sync(trigger_data->led_cdev, LED_ON);
>>> +		else if (trigger_data->led_cdev->brightness_set)
>>> +			led_set_brightness(trigger_data->led_cdev, LED_ON);
>>
>> I had similar issues in the past where a function call worked for
>> sleeping LEDs but not atomic ones (or the other way around? Don't
>> remember the details.)
>>
>> I wonder if there isn't a function that does the right thing no matter
>> what type the LED is. The other triggers should have the same issue, and
>> doing the above check in all of them just looks wrong.
> 
> Anyone fancy taking a deeper dive into this?
> 
> I'd usually have a go myself, but I'm presently swamped.
> 
> Pavel already has an idea?
> 

There is led_set_brightness_nosleep() and it should be used here
from the beginning. Generally it should be used always in triggers.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH 1/1] leds: ledtrig-tty.c: call led_set_brightness() when the blocking callback is not available

[Lee Jones]

On Sat, 07 Jan 2023, Jacek Anaszewski wrote:

> Hi all,
> 
> On 1/4/23 17:20, Lee Jones wrote:
> > On Fri, 09 Dec 2022, Uwe Kleine-König wrote:
> > 
> > > On Fri, Dec 09, 2022 at 10:10:38AM +1000, Qingtao Cao wrote:
> > > > The Marvell GPIO controller's driver will setup its struct gpio_chip's can_sleep
> > > > false, making create_gpio_led() setting up the brightness_set function pointer
> > > > instead of the brightness_set_blocking function pointer. In this case the
> > > > nonblocking version led_set_brightness() should be fallen back on by ledtrig_tty_work()
> > > > 
> > > > Signed-off-by: Qingtao Cao <qingtao.cao@digi.com>
> > > > ---
> > > >   drivers/leds/trigger/ledtrig-tty.c | 10 ++++++++--
> > > >   1 file changed, 8 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/leds/trigger/ledtrig-tty.c b/drivers/leds/trigger/ledtrig-tty.c
> > > > index f62db7e520b5..e43d285b5d06 100644
> > > > --- a/drivers/leds/trigger/ledtrig-tty.c
> > > > +++ b/drivers/leds/trigger/ledtrig-tty.c
> > > > @@ -122,12 +122,18 @@ 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);
> > > > +		if (trigger_data->led_cdev->brightness_set_blocking)
> > > > +			led_set_brightness_sync(trigger_data->led_cdev, LED_ON);
> > > > +		else if (trigger_data->led_cdev->brightness_set)
> > > > +			led_set_brightness(trigger_data->led_cdev, LED_ON);
> > > 
> > > I had similar issues in the past where a function call worked for
> > > sleeping LEDs but not atomic ones (or the other way around? Don't
> > > remember the details.)
> > > 
> > > I wonder if there isn't a function that does the right thing no matter
> > > what type the LED is. The other triggers should have the same issue, and
> > > doing the above check in all of them just looks wrong.
> > 
> > Anyone fancy taking a deeper dive into this?
> > 
> > I'd usually have a go myself, but I'm presently swamped.
> > 
> > Pavel already has an idea?
> > 
> 
> There is led_set_brightness_nosleep() and it should be used here
> from the beginning. Generally it should be used always in triggers.

Thanks for helping to keep an eye on things Jacek.

Much appreciated.

-- 
Lee Jones [李琼斯]

Re: [PATCH 1/1] leds: ledtrig-tty.c: call led_set_brightness() when the blocking callback is not available

[Jacek Anaszewski]

On 1/20/23 14:27, Lee Jones wrote:
> On Sat, 07 Jan 2023, Jacek Anaszewski wrote:
> 
>> Hi all,
>>
>> On 1/4/23 17:20, Lee Jones wrote:
>>> On Fri, 09 Dec 2022, Uwe Kleine-König wrote:
>>>
>>>> On Fri, Dec 09, 2022 at 10:10:38AM +1000, Qingtao Cao wrote:
>>>>> The Marvell GPIO controller's driver will setup its struct gpio_chip's can_sleep
>>>>> false, making create_gpio_led() setting up the brightness_set function pointer
>>>>> instead of the brightness_set_blocking function pointer. In this case the
>>>>> nonblocking version led_set_brightness() should be fallen back on by ledtrig_tty_work()
>>>>>
>>>>> Signed-off-by: Qingtao Cao <qingtao.cao@digi.com>
>>>>> ---
>>>>>    drivers/leds/trigger/ledtrig-tty.c | 10 ++++++++--
>>>>>    1 file changed, 8 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/leds/trigger/ledtrig-tty.c b/drivers/leds/trigger/ledtrig-tty.c
>>>>> index f62db7e520b5..e43d285b5d06 100644
>>>>> --- a/drivers/leds/trigger/ledtrig-tty.c
>>>>> +++ b/drivers/leds/trigger/ledtrig-tty.c
>>>>> @@ -122,12 +122,18 @@ 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);
>>>>> +		if (trigger_data->led_cdev->brightness_set_blocking)
>>>>> +			led_set_brightness_sync(trigger_data->led_cdev, LED_ON);
>>>>> +		else if (trigger_data->led_cdev->brightness_set)
>>>>> +			led_set_brightness(trigger_data->led_cdev, LED_ON);
>>>>
>>>> I had similar issues in the past where a function call worked for
>>>> sleeping LEDs but not atomic ones (or the other way around? Don't
>>>> remember the details.)
>>>>
>>>> I wonder if there isn't a function that does the right thing no matter
>>>> what type the LED is. The other triggers should have the same issue, and
>>>> doing the above check in all of them just looks wrong.
>>>
>>> Anyone fancy taking a deeper dive into this?
>>>
>>> I'd usually have a go myself, but I'm presently swamped.
>>>
>>> Pavel already has an idea?
>>>
>>
>> There is led_set_brightness_nosleep() and it should be used here
>> from the beginning. Generally it should be used always in triggers.
> 
> Thanks for helping to keep an eye on things Jacek.
> 
> Much appreciated.
> 

No problem, I still read the list and can elaborate
on various details of LED subsystem, as needed.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH 1/1] leds: ledtrig-tty.c: call led_set_brightness() when the blocking callback is not available

[Lee Jones]

On Sun, 22 Jan 2023, Jacek Anaszewski wrote:

> On 1/20/23 14:27, Lee Jones wrote:
> > On Sat, 07 Jan 2023, Jacek Anaszewski wrote:
> > 
> > > Hi all,
> > > 
> > > On 1/4/23 17:20, Lee Jones wrote:
> > > > On Fri, 09 Dec 2022, Uwe Kleine-König wrote:
> > > > 
> > > > > On Fri, Dec 09, 2022 at 10:10:38AM +1000, Qingtao Cao wrote:
> > > > > > The Marvell GPIO controller's driver will setup its struct gpio_chip's can_sleep
> > > > > > false, making create_gpio_led() setting up the brightness_set function pointer
> > > > > > instead of the brightness_set_blocking function pointer. In this case the
> > > > > > nonblocking version led_set_brightness() should be fallen back on by ledtrig_tty_work()
> > > > > > 
> > > > > > Signed-off-by: Qingtao Cao <qingtao.cao@digi.com>
> > > > > > ---
> > > > > >    drivers/leds/trigger/ledtrig-tty.c | 10 ++++++++--
> > > > > >    1 file changed, 8 insertions(+), 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/leds/trigger/ledtrig-tty.c b/drivers/leds/trigger/ledtrig-tty.c
> > > > > > index f62db7e520b5..e43d285b5d06 100644
> > > > > > --- a/drivers/leds/trigger/ledtrig-tty.c
> > > > > > +++ b/drivers/leds/trigger/ledtrig-tty.c
> > > > > > @@ -122,12 +122,18 @@ 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);
> > > > > > +		if (trigger_data->led_cdev->brightness_set_blocking)
> > > > > > +			led_set_brightness_sync(trigger_data->led_cdev, LED_ON);
> > > > > > +		else if (trigger_data->led_cdev->brightness_set)
> > > > > > +			led_set_brightness(trigger_data->led_cdev, LED_ON);
> > > > > 
> > > > > I had similar issues in the past where a function call worked for
> > > > > sleeping LEDs but not atomic ones (or the other way around? Don't
> > > > > remember the details.)
> > > > > 
> > > > > I wonder if there isn't a function that does the right thing no matter
> > > > > what type the LED is. The other triggers should have the same issue, and
> > > > > doing the above check in all of them just looks wrong.
> > > > 
> > > > Anyone fancy taking a deeper dive into this?
> > > > 
> > > > I'd usually have a go myself, but I'm presently swamped.
> > > > 
> > > > Pavel already has an idea?
> > > > 
> > > 
> > > There is led_set_brightness_nosleep() and it should be used here
> > > from the beginning. Generally it should be used always in triggers.
> > 
> > Thanks for helping to keep an eye on things Jacek.
> > 
> > Much appreciated.
> > 
> 
> No problem, I still read the list and can elaborate
> on various details of LED subsystem, as needed.

That would be very helpful.  Thanks again.

-- 
Lee Jones [李琼斯]