Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1528266 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 1/4] serial: core: Add LED trigger support Sascha Hauer <s.hauer@pengutronix.de> - 2016-11-23 11:10 +0100
    Re: [PATCH 1/4] serial: core: Add LED trigger support Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-11-23 11:10 +0100
      Re: [PATCH 1/4] serial: core: Add LED trigger support Sascha Hauer <s.hauer@pengutronix.de> - 2016-11-23 11:30 +0100
      Re: [PATCH 1/4] serial: core: Add LED trigger support Sascha Hauer <s.hauer@pengutronix.de> - 2016-11-24 09:30 +0100
        Re: [PATCH 1/4] serial: core: Add LED trigger support Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-11-24 11:00 +0100
    Re: [PATCH 1/4] serial: core: Add LED trigger support Mathieu Poirier <mathieu.poirier@linaro.org> - 2016-11-23 18:20 +0100
      Re: [PATCH 1/4] serial: core: Add LED trigger support Sascha Hauer <s.hauer@pengutronix.de> - 2016-11-24 07:50 +0100
        Re: [PATCH 1/4] serial: core: Add LED trigger support Mathieu Poirier <mathieu.poirier@linaro.org> - 2016-11-24 17:00 +0100
    Re: [PATCH 1/4] serial: core: Add LED trigger support Florian Fainelli <f.fainelli@gmail.com> - 2016-11-23 20:00 +0100
      Re: [PATCH 1/4] serial: core: Add LED trigger support Sascha Hauer <s.hauer@pengutronix.de> - 2016-11-24 09:20 +0100
    Re: [PATCH 1/4] serial: core: Add LED trigger support kbuild test robot <lkp@intel.com> - 2016-11-23 21:10 +0100

#1528266 — [PATCH 1/4] serial: core: Add LED trigger support

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

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);
+}
+
+/**
+ *	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;
+
+	uport->led_trigger_tx_name = kasprintf(GFP_KERNEL, "%s%d-tx",
+					       drv->dev_name, uport->line);
+	uport->led_trigger_rx_name = kasprintf(GFP_KERNEL, "%s%d-rx",
+					       drv->dev_name, uport->line);
+	if (!uport->led_trigger_tx_name || !uport->led_trigger_rx_name) {
+		ret = -ENOMEM;
+		goto err_alloc;
+	}
+
+	led_trigger_register_simple(uport->led_trigger_tx_name,
+				    &uport->led_trigger_tx);
+	led_trigger_register_simple(uport->led_trigger_rx_name,
+				    &uport->led_trigger_rx);
+
+	return 0;
+
+err_alloc:
+	kfree(uport->led_trigger_tx_name);
+	kfree(uport->led_trigger_rx_name);
+
+	return ret;
+}
+
+/**
+ *	uart_remove_led_triggers - remove LED triggers
+ *	@drv: pointer to the uart low level driver structure for this port
+ *	@uport: uart port structure to use for this port.
+ *
+ *	Remove LED triggers previously registered with uart_add_led_triggers
+ */
+void uart_remove_led_triggers(struct uart_port *uport)
+{
+	if (uport->led_trigger_rx)
+		led_trigger_unregister_simple(uport->led_trigger_rx);
+	kfree(uport->led_trigger_rx_name);
+
+	if (uport->led_trigger_tx)
+		led_trigger_unregister_simple(uport->led_trigger_tx);
+	kfree(uport->led_trigger_tx_name);
+}
+
 /**
  *	uart_add_one_port - attach a driver-defined port structure
  *	@drv: pointer to the uart low level driver structure for this port
@@ -2872,6 +2944,7 @@ int uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
 	WARN_ON(atomic_dec_return(&state->refcount) < 0);
 	wait_event(state->remove_wait, !atomic_read(&state->refcount));
 	state->uart_port = NULL;
+
 	mutex_unlock(&port->mutex);
 out:
 	mutex_unlock(&port_mutex);
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 3442014..1b0a169 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -29,6 +29,7 @@
 #include <linux/tty.h>
 #include <linux/mutex.h>
 #include <linux/sysrq.h>
+#include <linux/leds.h>
 #include <uapi/linux/serial_core.h>
 
 #ifdef CONFIG_SERIAL_CORE_CONSOLE
@@ -249,6 +250,10 @@ struct uart_port {
 	const struct attribute_group **tty_groups;	/* all attributes (serial core use only) */
 	struct serial_rs485     rs485;
 	void			*private_data;		/* generic platform data pointer */
+	struct led_trigger	*led_trigger_rx;
+	char			*led_trigger_rx_name;
+	struct led_trigger	*led_trigger_tx;
+	char			*led_trigger_tx_name;
 };
 
 static inline int serial_port_in(struct uart_port *up, int offset)
@@ -392,6 +397,11 @@ void uart_console_write(struct uart_port *port, const char *s,
 			unsigned int count,
 			void (*putchar)(struct uart_port *, int));
 
+int uart_add_led_triggers(struct uart_driver *drv, struct uart_port *uport);
+void uart_remove_led_triggers(struct uart_port *uport);
+void uart_led_trigger_tx(struct uart_port *port);
+void uart_led_trigger_rx(struct uart_port *port);
+
 /*
  * Port/driver registration/removal
  */
-- 
2.10.2

[toc] | [next] | [standalone]

#1528273

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
> activity.
> 
> 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);
> +}

Don't these functions need an EXPORT_SYMBOL_GPL() to work properly with
uart drivers being built as a module?

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1528288

On Wed, Nov 23, 2016 at 11:08:19AM +0100, Greg Kroah-Hartman 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
> > activity.
> > 
> > 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);
> > +}
> 
> Don't these functions need an EXPORT_SYMBOL_GPL() to work properly with
> uart drivers being built as a module?

Yes, for sure. Will fix in next version.

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 |

[toc] | [prev] | [next] | [standalone]

#1529059

On Wed, Nov 23, 2016 at 11:08:19AM +0100, Greg Kroah-Hartman 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
> > activity.

BTW last time LED triggers were discussed
(https://patchwork.kernel.org/patch/9212885/) You and Arnd mandated the
triggers should be implemented in the tty layer. By tty layer did you
really mean the tty layer or did you mean serial_core?

We could implement it in the tty layer, but tty doesn't know when the
characters are actually sent. There could be arbitrary time passing
between a tty_operations->put_char and the character being on the wire.

Also I am not sure if we want to have LED triggers for each and every
tty in the system

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 |

[toc] | [prev] | [next] | [standalone]

#1529119

On Thu, Nov 24, 2016 at 09:26:26AM +0100, Sascha Hauer wrote:
> On Wed, Nov 23, 2016 at 11:08:19AM +0100, Greg Kroah-Hartman 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
> > > activity.
> 
> BTW last time LED triggers were discussed
> (https://patchwork.kernel.org/patch/9212885/) You and Arnd mandated the
> triggers should be implemented in the tty layer. By tty layer did you
> really mean the tty layer or did you mean serial_core?
> 
> We could implement it in the tty layer, but tty doesn't know when the
> characters are actually sent. There could be arbitrary time passing
> between a tty_operations->put_char and the character being on the wire.

With USB serial devices and even basic UARTs, you never really know when
"the character is on the wire", you can only guess.  And really, just
guessing is good enough given that no one is using this type of
interface to actually count when exactly the bits hit the wire.  This is
just for those that like blinky-lights :)

> Also I am not sure if we want to have LED triggers for each and every
> tty in the system

Why not?  It's opt-in by the user, so might as well let them do it for
whatever tty they want to.

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1528613

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.
 
> +}
> +
> +/**
> + *	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.

Thanks,
Mathieu

> +
> +	uport->led_trigger_tx_name = kasprintf(GFP_KERNEL, "%s%d-tx",
> +					       drv->dev_name, uport->line);
> +	uport->led_trigger_rx_name = kasprintf(GFP_KERNEL, "%s%d-rx",
> +					       drv->dev_name, uport->line);
> +	if (!uport->led_trigger_tx_name || !uport->led_trigger_rx_name) {
> +		ret = -ENOMEM;
> +		goto err_alloc;
> +	}
> +
> +	led_trigger_register_simple(uport->led_trigger_tx_name,
> +				    &uport->led_trigger_tx);
> +	led_trigger_register_simple(uport->led_trigger_rx_name,
> +				    &uport->led_trigger_rx);
> +
> +	return 0;
> +
> +err_alloc:
> +	kfree(uport->led_trigger_tx_name);
> +	kfree(uport->led_trigger_rx_name);
> +
> +	return ret;
> +}
> +
> +/**
> + *	uart_remove_led_triggers - remove LED triggers
> + *	@drv: pointer to the uart low level driver structure for this port
> + *	@uport: uart port structure to use for this port.
> + *
> + *	Remove LED triggers previously registered with uart_add_led_triggers
> + */
> +void uart_remove_led_triggers(struct uart_port *uport)
> +{
> +	if (uport->led_trigger_rx)
> +		led_trigger_unregister_simple(uport->led_trigger_rx);
> +	kfree(uport->led_trigger_rx_name);
> +
> +	if (uport->led_trigger_tx)
> +		led_trigger_unregister_simple(uport->led_trigger_tx);
> +	kfree(uport->led_trigger_tx_name);
> +}
> +
>  /**
>   *	uart_add_one_port - attach a driver-defined port structure
>   *	@drv: pointer to the uart low level driver structure for this port
> @@ -2872,6 +2944,7 @@ int uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
>  	WARN_ON(atomic_dec_return(&state->refcount) < 0);
>  	wait_event(state->remove_wait, !atomic_read(&state->refcount));
>  	state->uart_port = NULL;
> +
>  	mutex_unlock(&port->mutex);
>  out:
>  	mutex_unlock(&port_mutex);
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> index 3442014..1b0a169 100644
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -29,6 +29,7 @@
>  #include <linux/tty.h>
>  #include <linux/mutex.h>
>  #include <linux/sysrq.h>
> +#include <linux/leds.h>
>  #include <uapi/linux/serial_core.h>
>  
>  #ifdef CONFIG_SERIAL_CORE_CONSOLE
> @@ -249,6 +250,10 @@ struct uart_port {
>  	const struct attribute_group **tty_groups;	/* all attributes (serial core use only) */
>  	struct serial_rs485     rs485;
>  	void			*private_data;		/* generic platform data pointer */
> +	struct led_trigger	*led_trigger_rx;
> +	char			*led_trigger_rx_name;
> +	struct led_trigger	*led_trigger_tx;
> +	char			*led_trigger_tx_name;
>  };
>  
>  static inline int serial_port_in(struct uart_port *up, int offset)
> @@ -392,6 +397,11 @@ void uart_console_write(struct uart_port *port, const char *s,
>  			unsigned int count,
>  			void (*putchar)(struct uart_port *, int));
>  
> +int uart_add_led_triggers(struct uart_driver *drv, struct uart_port *uport);
> +void uart_remove_led_triggers(struct uart_port *uport);
> +void uart_led_trigger_tx(struct uart_port *port);
> +void uart_led_trigger_rx(struct uart_port *port);
> +
>  /*
>   * Port/driver registration/removal
>   */
> -- 
> 2.10.2
> 

[toc] | [prev] | [next] | [standalone]

#1529027

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 |

[toc] | [prev] | [next] | [standalone]

#1529518

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

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 |

[toc] | [prev] | [next] | [standalone]

#1528696

On 11/23/2016 02:01 AM, 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.

Looking at 8250, we call serial8250_tx_chars from __start_tx which is
called form the uart_ops::start_tx, for RX I sort of agree, since this
happens in interrupt handler. I suppose some drivers could actually
queue work but not do it right away though?

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

Could we somehow remedy the lack of knowledge from the core as whether
the HW sends/receives characters first before adding support for LED
triggers? It would be more generic and future proof to require UART
drivers to report to the core when they actually TX/RX, and then at the
core level, utilize that knowledge to perform the LED trigger.

Side note: are you positive using drv->dev_name is robust enough on
systems with many different UART drivers, yet all of them being ttyS*?

Thanks!
-- 
Florian

[toc] | [prev] | [next] | [standalone]

#1529053

On Wed, Nov 23, 2016 at 10:57:13AM -0800, Florian Fainelli wrote:
> On 11/23/2016 02:01 AM, 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.
> 
> Looking at 8250, we call serial8250_tx_chars from __start_tx which is
> called form the uart_ops::start_tx, for RX I sort of agree, since this
> happens in interrupt handler. I suppose some drivers could actually
> queue work but not do it right away though?

RX is not the problem. When characters are received all drivers call
tty_flip_buffer_push() which could be used for LED triggering (either
directly or we replace the calls to tty_flip_buffer_push() with some
wrapper function).
The problem comes with TX. The serial core has a FIFO which gets
characters from the tty layer. There is no function which the serial
drivers could call to read from this FIFO, instead they have to open code
this.
Continuing with the 8250 driver serial8250_tx_chars() is not only called
from __start_tx(), but also from the interrupt handler. Transmission
works without the serial_core ever noticing.

> 
> > 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
> > activity.
> 
> Could we somehow remedy the lack of knowledge from the core as whether
> the HW sends/receives characters first before adding support for LED
> triggers? It would be more generic and future proof to require UART
> drivers to report to the core when they actually TX/RX, and then at the
> core level, utilize that knowledge to perform the LED trigger.

Maybe we could introduce a function to read from the TX FIFO. Having
open coded this in each and every driver is not nice anyway.

> 
> Side note: are you positive using drv->dev_name is robust enough on
> systems with many different UART drivers, yet all of them being ttyS*?

If we have different UART drivers, only one of them provides ttyS*, no?
Other drivers will have to use another namespace.

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 |

[toc] | [prev] | [next] | [standalone]

#1528736

[Multipart message — attachments visible in raw view] — view raw

Hi Sascha,

[auto build test WARNING on tty/tty-testing]
[also build test WARNING on v4.9-rc6 next-20161123]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Sascha-Hauer/serial-core-Add-LED-trigger-support/20161124-010846
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
reproduce: make htmldocs

All warnings (new ones prefixed by >>):

   make[3]: warning: jobserver unavailable: using -j1.  Add '+' to parent make rule.
   include/linux/init.h:1: warning: no structured comments found
   include/linux/workqueue.h:392: warning: No description found for parameter '...'
   include/linux/workqueue.h:392: warning: Excess function parameter 'args' description in 'alloc_workqueue'
   include/linux/workqueue.h:413: warning: No description found for parameter '...'
   include/linux/workqueue.h:413: warning: Excess function parameter 'args' description in 'alloc_ordered_workqueue'
   include/linux/kthread.h:26: warning: No description found for parameter '...'
   kernel/sys.c:1: warning: no structured comments found
   drivers/dma-buf/seqno-fence.c:1: warning: no structured comments found
   include/linux/fence-array.h:61: warning: No description found for parameter 'fence'
>> drivers/tty/serial/serial_core.c:2773: warning: Excess function parameter 'drv' description in 'uart_remove_led_triggers'
   include/sound/core.h:324: warning: No description found for parameter '...'
   include/sound/core.h:335: warning: No description found for parameter '...'
   include/sound/core.h:388: warning: No description found for parameter '...'
   include/media/media-entity.h:1054: warning: No description found for parameter '...'
   include/net/mac80211.h:2148: WARNING: Inline literal start-string without end-string.
   include/net/mac80211.h:2153: WARNING: Inline literal start-string without end-string.
   include/net/mac80211.h:3202: ERROR: Unexpected indentation.
   include/net/mac80211.h:3205: WARNING: Block quote ends without a blank line; unexpected unindent.
   include/net/mac80211.h:3207: ERROR: Unexpected indentation.
   include/net/mac80211.h:3208: WARNING: Block quote ends without a blank line; unexpected unindent.
   include/net/mac80211.h:1435: WARNING: Inline emphasis start-string without end-string.
   include/net/mac80211.h:1172: WARNING: Inline literal start-string without end-string.
   include/net/mac80211.h:1173: WARNING: Inline literal start-string without end-string.
   include/net/mac80211.h:814: ERROR: Unexpected indentation.
   include/net/mac80211.h:815: WARNING: Block quote ends without a blank line; unexpected unindent.
   include/net/mac80211.h:820: ERROR: Unexpected indentation.
   include/net/mac80211.h:821: WARNING: Block quote ends without a blank line; unexpected unindent.
   include/net/mac80211.h:2489: ERROR: Unexpected indentation.
   include/net/mac80211.h:1768: ERROR: Unexpected indentation.
   include/net/mac80211.h:1772: WARNING: Block quote ends without a blank line; unexpected unindent.
   include/net/mac80211.h:1746: WARNING: Inline emphasis start-string without end-string.
   kernel/sched/fair.c:7259: WARNING: Inline emphasis start-string without end-string.
   kernel/time/timer.c:1240: ERROR: Unexpected indentation.
   kernel/time/timer.c:1242: ERROR: Unexpected indentation.
   kernel/time/timer.c:1243: WARNING: Block quote ends without a blank line; unexpected unindent.
   include/linux/wait.h:121: WARNING: Block quote ends without a blank line; unexpected unindent.
   include/linux/wait.h:124: ERROR: Unexpected indentation.
   include/linux/wait.h:126: WARNING: Block quote ends without a blank line; unexpected unindent.
   kernel/time/hrtimer.c:1021: WARNING: Block quote ends without a blank line; unexpected unindent.
   kernel/signal.c:317: WARNING: Inline literal start-string without end-string.
   drivers/base/firmware_class.c:1348: WARNING: Bullet list ends without a blank line; unexpected unindent.
   drivers/message/fusion/mptbase.c:5054: WARNING: Definition list ends without a blank line; unexpected unindent.
   drivers/tty/serial/serial_core.c:1897: WARNING: Definition list ends without a blank line; unexpected unindent.
   include/linux/spi/spi.h:369: ERROR: Unexpected indentation.
   WARNING: dvipng command 'dvipng' cannot be run (needed for math display), check the imgmath_dvipng setting

vim +2773 drivers/tty/serial/serial_core.c

  2757	err_alloc:
  2758		kfree(uport->led_trigger_tx_name);
  2759		kfree(uport->led_trigger_rx_name);
  2760	
  2761		return ret;
  2762	}
  2763	
  2764	/**
  2765	 *	uart_remove_led_triggers - remove LED triggers
  2766	 *	@drv: pointer to the uart low level driver structure for this port
  2767	 *	@uport: uart port structure to use for this port.
  2768	 *
  2769	 *	Remove LED triggers previously registered with uart_add_led_triggers
  2770	 */
  2771	void uart_remove_led_triggers(struct uart_port *uport)
  2772	{
> 2773		if (uport->led_trigger_rx)
  2774			led_trigger_unregister_simple(uport->led_trigger_rx);
  2775		kfree(uport->led_trigger_rx_name);
  2776	
  2777		if (uport->led_trigger_tx)
  2778			led_trigger_unregister_simple(uport->led_trigger_tx);
  2779		kfree(uport->led_trigger_tx_name);
  2780	}
  2781	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web