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

[Sascha Hauer]

On Thu, Nov 24, 2016 at 08:45:43AM -0700, Mathieu Poirier wrote:
> On Thu, Nov 24, 2016 at 07:41:37AM +0100, Sascha Hauer wrote:
> > 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@xxxxxxxxxxxxxx>
> > > > ---
> > > >  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.
> 
> So you did find instances where the blink time wasn't 50ms.  To me that's
> a valid reason to not hard code the blink time and proceed differently.

But they are hardcoded to these values. My gut feeling is that the
authors had to pick a value and some used 30ms while others used 50ms.

Making this configurable to me only means that it will be harder to
cleanup later. I'd rather wait until someone comes along who really
can present good reasons to make that configurable.

> 
> > 
> > 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?
> 
>         if (!uport->led_trigger_rx || !uport->led_triggertx)
>                 return -EINVAL;

uport->led_trigger_rx|tx are structs allocated in this function. It's
the normal case that they are NULL.

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 |