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

[Lee Jones <lee@kernel.org>]

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 [李琼斯]