Re: [PATCH 1/4] serial: core: Add LED trigger support

[Sascha Hauer <s.hauer@pengutronix.de>]

On Wed, Nov 23, 2016 at 10:13:02AM -0700, Mathieu Poirier wrote:
> On Wed, Nov 23, 2016 at 11:01:03AM +0100, Sascha Hauer wrote:
> > With this patch the serial core provides LED triggers for RX and TX.
> > 
> > As the serial core layer does not know when the hardware actually sends
> > or receives characters, this needs help from the UART drivers. The
> > LED triggers are registered in uart_add_led_triggers() called from
> > the UART drivers which want to support LED triggers. All the driver
> > has to do then is to call uart_led_trigger_[tx|rx] to indicate
> > activite.
> 
> Hello Sascha,
> 
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  drivers/tty/serial/serial_core.c | 73 ++++++++++++++++++++++++++++++++++++++++
> >  include/linux/serial_core.h      | 10 ++++++
> >  2 files changed, 83 insertions(+)
> > 
> > diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> > index f2303f3..3e8afb7 100644
> > --- a/drivers/tty/serial/serial_core.c
> > +++ b/drivers/tty/serial/serial_core.c
> > @@ -34,6 +34,7 @@
> >  #include <linux/serial_core.h>
> >  #include <linux/delay.h>
> >  #include <linux/mutex.h>
> > +#include <linux/leds.h>
> >  
> >  #include <asm/irq.h>
> >  #include <asm/uaccess.h>
> > @@ -2703,6 +2704,77 @@ static const struct attribute_group tty_dev_attr_group = {
> >  	.attrs = tty_dev_attrs,
> >  	};
> >  
> > +void uart_led_trigger_tx(struct uart_port *uport)
> > +{
> > +	unsigned long delay = 50;
> > +
> > +	led_trigger_blink_oneshot(uport->led_trigger_tx, &delay, &delay, 0);
> > +}
> > +
> > +void uart_led_trigger_rx(struct uart_port *uport)
> > +{
> > +	unsigned long delay = 50;
> > +
> > +	led_trigger_blink_oneshot(uport->led_trigger_rx, &delay, &delay, 0);
> 
> For both rx/tx the core constrains the delay_on/off along with the inversion.
> Instead of adding the led_trigger_rx/tx and led_trigger_rx/tx_name to the 
> struct uart_port, wouldn't it be better to add a new struct led_trigger that
> would encapsulate those along wit the on/off delay and the inversion?
> 
> That way those values could be communicated to the core at registration time
> instead of hard-coding things.

Not sure this goes into the right direction. Looking at the other
callers of led_trigger_blink_oneshot() most of them use an arbitrary
blink time of 30 or 50ms and the users seem to be happy with it. There
doesn't seem to be the desire to make that value configurable. If we
want to make it configurable, it's probably better to move the option
to the led device rather than putting the burden on every user of the
led triggers.

I don't think the inversion flag is meant for being configurable. It's
rather used for the default state of the LED. The CAN layer for example
turns the LED on when the interface is up and then blinks (turns off and
on again) on activity. For this it uses the inversion flag.

>  
> > +}
> > +
> > +/**
> > + *	uart_add_led_triggers - register LED triggers for a UART
> > + *	@drv: pointer to the uart low level driver structure for this port
> > + *	@uport: uart port structure to use for this port.
> > + *
> > + *	Called by the driver to register LED triggers for a UART. It's the
> > + *	drivers responsibility to call uart_led_trigger_rx/tx on received
> > + *	and transmitted chars then.
> > + */
> > +int uart_add_led_triggers(struct uart_driver *drv, struct uart_port *uport)
> > +{
> > +	int ret;
> > +
> > +	if (!IS_ENABLED(CONFIG_LEDS_TRIGGERS))
> > +		return 0;
> 
> Since this is a public interface, checking for valid led_trigger_tx/rx before
> moving on with the rest of the initialisation is probably a good idea.

What do you mean? Should we return an error when CONFIG_LEDS_TRIGGERS is
disabled?

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |