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

[Mathieu Poirier]

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.

> 
> 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;


> 
> 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 |