Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 4 Jan 2023 16:20:00 +0000
From:   Lee Jones <lee@kernel.org>
To:     Uwe =?iso-8859-1?Q?Kleine-K=F6nig?= 
        <u.kleine-koenig@pengutronix.de>
Cc:     Qingtao Cao <qingtao.cao.au@gmail.com>,
        Qingtao Cao <qingtao.cao@digi.com>,
        Pavel Machek <pavel@ucw.cz>, linux-leds@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/1] leds: ledtrig-tty.c: call led_set_brightness() when
 the blocking callback is not available
Message-ID: <Y7WnMISK20Z4hsg1@google.com>
References: <20221209001039.7287-1-qingtao.cao@digi.com>
 <20221209124058.dwez7wohs7dfeiik@pengutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20221209124058.dwez7wohs7dfeiik@pengutronix.de>
Precedence: bulk

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