serial: Add LED trigger support

serial: Add LED trigger support

[Sascha Hauer]

This series brings LED trigger support to the serial_core layer
and adopts some driver to use it.
This is an updated version of https://patchwork.kernel.org/patch/9212885/
which added LED trigger support to the i.MX serial driver only.

Sascha

----------------------------------------------------------------
Sascha Hauer (4):
      serial: core: Add LED trigger support
      serial: 8250: Add LED trigger support
      serial: cpm_uart: Add LED trigger support
      serial: imx: Add LED trigger support

 drivers/tty/serial/8250/8250_core.c         | 12 ++++-
 drivers/tty/serial/8250/8250_port.c         |  4 ++
 drivers/tty/serial/cpm_uart/cpm_uart_core.c | 22 ++++++++-
 drivers/tty/serial/imx.c                    | 24 +++++++++-
 drivers/tty/serial/serial_core.c            | 73 +++++++++++++++++++++++++++++
 include/linux/serial_core.h                 | 10 ++++
 6 files changed, 139 insertions(+), 6 deletions(-)

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

[Sascha Hauer]

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

[PATCH 2/4] serial: 8250: Add LED trigger support

[Sascha Hauer]

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/tty/serial/8250/8250_core.c | 12 ++++++++++--
 drivers/tty/serial/8250/8250_port.c |  4 ++++
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index 240a361..bbcd2539 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -581,6 +581,7 @@ serial8250_register_ports(struct uart_driver *drv, struct device *dev)
 			up->port.flags |= UPF_NO_TXEN_TEST;
 
 		uart_add_one_port(drv, &up->port);
+		uart_add_led_triggers(drv, &up->port);
 	}
 }
 
@@ -974,8 +975,10 @@ int serial8250_register_8250_port(struct uart_8250_port *up)
 
 	uart = serial8250_find_match_or_unused(&up->port);
 	if (uart && uart->port.type != PORT_8250_CIR) {
-		if (uart->port.dev)
+		if (uart->port.dev) {
 			uart_remove_one_port(&serial8250_reg, &uart->port);
+			uart_remove_led_triggers(&uart->port);
+		}
 
 		uart->port.iobase       = up->port.iobase;
 		uart->port.membase      = up->port.membase;
@@ -1047,8 +1050,11 @@ int serial8250_register_8250_port(struct uart_8250_port *up)
 
 			ret = uart_add_one_port(&serial8250_reg,
 						&uart->port);
-			if (ret == 0)
+			if (ret == 0) {
+				uart_add_led_triggers(&serial8250_reg,
+						&uart->port);
 				ret = uart->port.line;
+			}
 		} else {
 			dev_info(uart->port.dev,
 				"skipping CIR port at 0x%lx / 0x%llx, IRQ %d\n",
@@ -1087,6 +1093,7 @@ void serial8250_unregister_port(int line)
 	}
 
 	uart_remove_one_port(&serial8250_reg, &uart->port);
+	uart_remove_led_triggers(&uart->port);
 	if (serial8250_isa_devs) {
 		uart->port.flags &= ~UPF_BOOT_AUTOCONF;
 		if (skip_txen_test)
@@ -1095,6 +1102,7 @@ void serial8250_unregister_port(int line)
 		uart->port.dev = &serial8250_isa_devs->dev;
 		uart->capabilities = 0;
 		uart_add_one_port(&serial8250_reg, &uart->port);
+		uart_add_led_triggers(&serial8250_reg, &uart->port);
 	} else {
 		uart->port.dev = NULL;
 	}
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 1731b98..c301ec1 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1703,6 +1703,8 @@ unsigned char serial8250_rx_chars(struct uart_8250_port *up, unsigned char lsr)
 	struct uart_port *port = &up->port;
 	int max_count = 256;
 
+	uart_led_trigger_rx(port);
+
 	do {
 		serial8250_read_char(up, lsr);
 		if (--max_count == 0)
@@ -1736,6 +1738,8 @@ void serial8250_tx_chars(struct uart_8250_port *up)
 		return;
 	}
 
+	uart_led_trigger_tx(port);
+
 	count = up->tx_loadsz;
 	do {
 		serial_out(up, UART_TX, xmit->buf[xmit->tail]);
-- 
2.10.2

[PATCH 3/4] serial: cpm_uart: Add LED trigger support

[Sascha Hauer]

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/tty/serial/cpm_uart/cpm_uart_core.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/cpm_uart/cpm_uart_core.c b/drivers/tty/serial/cpm_uart/cpm_uart_core.c
index d3e3d42..5d5633d 100644
--- a/drivers/tty/serial/cpm_uart/cpm_uart_core.c
+++ b/drivers/tty/serial/cpm_uart/cpm_uart_core.c
@@ -255,6 +255,8 @@ static void cpm_uart_int_rx(struct uart_port *port)
 
 	pr_debug("CPM uart[%d]:RX INT\n", port->line);
 
+	uart_led_trigger_rx(port);
+
 	/* Just loop through the closed BDs and copy the characters into
 	 * the buffer.
 	 */
@@ -721,6 +723,8 @@ static int cpm_uart_tx_pump(struct uart_port *port)
 	/* Pick next descriptor and fill from buffer */
 	bdp = pinfo->tx_cur;
 
+	uart_led_trigger_tx(port);
+
 	while (!(in_be16(&bdp->cbd_sc) & BD_SC_READY) &&
 	       xmit->tail != xmit->head) {
 		count = 0;
@@ -1426,13 +1430,27 @@ static int cpm_uart_probe(struct platform_device *ofdev)
 	if (ret)
 		return ret;
 
-	return uart_add_one_port(&cpm_reg, &pinfo->port);
+	ret = uart_add_one_port(&cpm_reg, &pinfo->port);
+	if (ret)
+		return ret;
+
+	uart_add_led_triggers(&cpm_reg, &pinfo->port);
+
+	return 0;
 }
 
 static int cpm_uart_remove(struct platform_device *ofdev)
 {
 	struct uart_cpm_port *pinfo = platform_get_drvdata(ofdev);
-	return uart_remove_one_port(&cpm_reg, &pinfo->port);
+	int ret;
+
+	ret = uart_remove_one_port(&cpm_reg, &pinfo->port);
+	if (ret)
+		return ret;
+
+	uart_remove_led_triggers(&pinfo->port);
+
+	return 0;
 }
 
 static const struct of_device_id cpm_uart_match[] = {
-- 
2.10.2

[PATCH 4/4] serial: imx: Add LED trigger support

[Sascha Hauer]

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/tty/serial/imx.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index a70356d..5eaf576 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -444,6 +444,8 @@ static inline void imx_transmit_buffer(struct imx_port *sport)
 		return;
 	}
 
+	uart_led_trigger_tx(&sport->port);
+
 	if (sport->dma_is_enabled) {
 		/*
 		 * We've just sent a X-char Ensure the TX DMA is enabled
@@ -569,6 +571,7 @@ static void imx_dma_tx(struct imx_port *sport)
 	/* fire it */
 	sport->dma_is_txing = 1;
 	dmaengine_submit(desc);
+	uart_led_trigger_tx(&sport->port);
 	dma_async_issue_pending(chan);
 	return;
 }
@@ -655,6 +658,8 @@ static irqreturn_t imx_rxint(int irq, void *dev_id)
 	struct tty_port *port = &sport->port.state->port;
 	unsigned long flags, temp;
 
+	uart_led_trigger_rx(&sport->port);
+
 	spin_lock_irqsave(&sport->port.lock, flags);
 
 	while (readl(sport->port.membase + USR2) & USR2_RDR) {
@@ -729,6 +734,8 @@ static void imx_dma_rxint(struct imx_port *sport)
 	unsigned long temp;
 	unsigned long flags;
 
+	uart_led_trigger_rx(&sport->port);
+
 	spin_lock_irqsave(&sport->port.lock, flags);
 
 	temp = readl(sport->port.membase + USR2);
@@ -2184,14 +2191,27 @@ static int serial_imx_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, sport);
 
-	return uart_add_one_port(&imx_reg, &sport->port);
+	ret = uart_add_one_port(&imx_reg, &sport->port);
+	if (ret)
+		return ret;
+
+	uart_add_led_triggers(&imx_reg, &sport->port);
+
+	return 0;
 }
 
 static int serial_imx_remove(struct platform_device *pdev)
 {
 	struct imx_port *sport = platform_get_drvdata(pdev);
+	int ret;
 
-	return uart_remove_one_port(&imx_reg, &sport->port);
+	ret = uart_remove_one_port(&imx_reg, &sport->port);
+	if (ret)
+		return ret;
+
+	uart_remove_led_triggers(&sport->port);
+
+	return 0;
 }
 
 static void serial_imx_restore_context(struct imx_port *sport)
-- 
2.10.2

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

[Greg Kroah-Hartman]

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

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

[Sascha Hauer]

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 |

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

[Mathieu Poirier]

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
> 

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

[Florian Fainelli]

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

Re: [PATCH 2/4] serial: 8250: Add LED trigger support

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1596 bytes --]

Hi Sascha,

[auto build test ERROR on tty/tty-testing]
[also build test ERROR 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
config: m32r-m32104ut_defconfig (attached as .config)
compiler: m32r-linux-gcc (GCC) 6.2.0
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=m32r 

All errors (new ones prefixed by >>):

>> ERROR: "uart_led_trigger_rx" [drivers/tty/serial/8250/8250_base.ko] undefined!
>> ERROR: "uart_led_trigger_tx" [drivers/tty/serial/8250/8250_base.ko] undefined!
>> ERROR: "uart_remove_led_triggers" [drivers/tty/serial/8250/8250.ko] undefined!
>> ERROR: "uart_add_led_triggers" [drivers/tty/serial/8250/8250.ko] undefined!
--
>> ERROR: "uart_led_trigger_rx" [drivers/tty/serial/8250/8250_base.ko] undefined!
>> ERROR: "uart_led_trigger_tx" [drivers/tty/serial/8250/8250_base.ko] undefined!
>> ERROR: "uart_remove_led_triggers" [drivers/tty/serial/8250/8250.ko] undefined!
>> ERROR: "uart_add_led_triggers" [drivers/tty/serial/8250/8250.ko] undefined!

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 10628 bytes --]

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 5472 bytes --]

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 6425 bytes --]

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1578 bytes --]

Hi Sascha,

[auto build test ERROR on tty/tty-testing]
[also build test ERROR 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
config: s390-allmodconfig (attached as .config)
compiler: s390x-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=s390 

All errors (new ones prefixed by >>):

>> ERROR: "uart_remove_led_triggers" [drivers/tty/serial/imx.ko] undefined!
>> ERROR: "uart_led_trigger_rx" [drivers/tty/serial/imx.ko] undefined!
>> ERROR: "uart_led_trigger_tx" [drivers/tty/serial/imx.ko] undefined!
>> ERROR: "uart_add_led_triggers" [drivers/tty/serial/imx.ko] undefined!
   ERROR: "uart_led_trigger_rx" [drivers/tty/serial/8250/8250_base.ko] undefined!
   ERROR: "uart_led_trigger_tx" [drivers/tty/serial/8250/8250_base.ko] undefined!
   ERROR: "uart_remove_led_triggers" [drivers/tty/serial/8250/8250.ko] undefined!
   ERROR: "uart_add_led_triggers" [drivers/tty/serial/8250/8250.ko] undefined!

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 43170 bytes --]

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

[Sascha Hauer]

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 |

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

[Sascha Hauer]

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 |

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

[Sascha Hauer]

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 |

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

[Greg Kroah-Hartman]

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

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

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

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 |

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

[Florian Fainelli]

On 11/24/2016 12:17 AM, Sascha Hauer wrote:
> 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.

Something like that could be nice to have yes.

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

I remember this was a problem a couple of years ago last I tried, with
the 8250 driver being actually preventing other drivers from using
ttyS*, but if you don't have 8250 taking over the low ttyS numbers, you
may run into a case where several drivers successfully register their
character devices under a ttyS* numbering scheme.

Whether this is hypothetical or not nowadays, it may be nicer to provide
a more uniquely distinct name like what dev_name() returns, or
ttyS*-driver-tx for instance?
-- 
Florian

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

[One Thousand Gnomes]

> > If we have different UART drivers, only one of them provides ttyS*, no?
> > Other drivers will have to use another namespace.  
> 
> I remember this was a problem a couple of years ago last I tried, with
> the 8250 driver being actually preventing other drivers from using
> ttyS*, but if you don't have 8250 taking over the low ttyS numbers, you
> may run into a case where several drivers successfully register their
> character devices under a ttyS* numbering scheme.
> 
> Whether this is hypothetical or not nowadays, it may be nicer to provide
> a more uniquely distinct name like what dev_name() returns, or
> ttyS*-driver-tx for instance?

It needs to be at the tty layer anyway or you will break low end
machines. On a low end system with a 16450A UART you've got something
like 160 microseconds at 57600 baud before you must have exited the
IRQ handler, re-entered it and be reading the data register again. The
current proposed patches touching the 8250 IRQ path are almost certainly
going to cause regressions on old machines.

So you need to hook receive at a point outside of the IRQ layer (eg when
the workqueue starts feeding it into the ldisc), but on the tx side you
need to hook the write method of the tty really, because tty_write sends
data to the ldisc and what happens then is up to the ldisc.

Even there though you have cases that are terribly latency dependant like
some of the dumb programming tools that don't window. If you halve
someones programming rate for microcontrollers you are going to get hate
mail 8) For RX that can mostly be avoided by queuing the data then doing
the LEDs, for TX I'm not clear you can easily hide the latency.

Alan

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1495 bytes --]

Hi!

> 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(+)
> 

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

Is it neccessary to have separate triggers for rx and tx?

Won't most common application be "light a led for rx _or_ tx", which
is something this can not do?

If I have system with 200 serials, this creates 400 triggers, each
trigger name will be about 10 characters AFAICT... and we'll overflow
some buffer when doing "cat triggers", no?

Would it be enough to have 3 triggers? (Any activity, any rx, any tx)?

Thanks,
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 686 bytes --]

Hi!

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

Yes, please.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]