Move RS485 implementation from drivers to serial core

Move RS485 implementation from drivers to serial core

[Lino Sanfilippo]

This patch series is an attempt to simplify rs485 implementation in drivers
by moving the following tasks out of the drivers into the serial core:

- ensure sane RTS settings: in case of an invalid configuration (both RTS
  after send and RTS on send set or both unset) enable RTS on send and
  disable RTS after send

- nullify the padding field of the serial_rs485 struct before it is
  returned to userspace

- copy the configuration stored in the serial_rs485 struct to the port
  configuration if setting the configuration in the driver was successfull

- limit the RTS delay to 100ms


Redundant code has been removed from the following drivers for now:

- atmel
- fsl_lpuart
- amba
- imx
- max310x
- omap-serial
- sc16is7xx
- stm32-usart

The code has been tested with the amba pl011 driver. This series applies
against tty-testing.

[PATCH 1/9] serial: core: move RS485 configuration tasks from drivers into core

[Lino Sanfilippo]

Several drivers that support setting the RS485 configuration via userspace
implement on or more of the following tasks:

- in case of an invalid RTS configuration (both RTS after send and RTS on
  send set or both unset) fall back to enable RTS on send and disable RTS
  after send

- nullify the padding field of the returned serial_rs485 struct

- copy the configuration into the uart port struct

- limit RTS delays to 100 ms

Move these tasks into the serial core to make them generic and to provide
a consistent beheviour among all drivers.

Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
---
 drivers/tty/serial/serial_core.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 846192a7b4bf..3fab4070359c 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1282,8 +1282,21 @@ static int uart_set_rs485_config(struct uart_port *port,
 	if (copy_from_user(&rs485, rs485_user, sizeof(*rs485_user)))
 		return -EFAULT;
 
+	/* pick sane settings if the user hasn't */
+	if (!(rs485.flags & SER_RS485_RTS_ON_SEND) ==
+	    !(rs485.flags & SER_RS485_RTS_AFTER_SEND)) {
+		rs485.flags |= SER_RS485_RTS_ON_SEND;
+		rs485.flags &= ~SER_RS485_RTS_AFTER_SEND;
+	}
+	/* clamp the delays to [0, 100ms] */
+	rs485.delay_rts_before_send = min(rs485.delay_rts_before_send, 100U);
+	rs485.delay_rts_after_send = min(rs485.delay_rts_after_send, 100U);
+	memset(rs485.padding, 0, sizeof(rs485.padding));
+
 	spin_lock_irqsave(&port->lock, flags);
 	ret = port->rs485_config(port, &rs485);
+	if (!ret)
+		port->rs485 = rs485;
 	spin_unlock_irqrestore(&port->lock, flags);
 	if (ret)
 		return ret;

base-commit: ad30d108a5135af584ff47f5ff81be971b6c26f1
-- 
2.34.1

[PATCH 2/9] serial: amba-pl011: remove redundant code in rs485_config

[Lino Sanfilippo]

When RS485 is configured by userspace the serial core already

- checks for valid RTS settings
- nullifies the padding field of the passed serial_rs485 struct
- clamps the RTS delays
- assigns the passed configuration to the uart port struct

So remove these tasks from the code of the rs485_config function to avoid
redundancy.

Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
---
 drivers/tty/serial/amba-pl011.c | 16 +---------------
 1 file changed, 1 insertion(+), 15 deletions(-)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index ba053a68529f..35c633739975 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -2170,25 +2170,11 @@ static int pl011_rs485_config(struct uart_port *port,
 	struct uart_amba_port *uap =
 		container_of(port, struct uart_amba_port, port);
 
-	/* pick sane settings if the user hasn't */
-	if (!(rs485->flags & SER_RS485_RTS_ON_SEND) ==
-	    !(rs485->flags & SER_RS485_RTS_AFTER_SEND)) {
-		rs485->flags |= SER_RS485_RTS_ON_SEND;
-		rs485->flags &= ~SER_RS485_RTS_AFTER_SEND;
-	}
-	/* clamp the delays to [0, 100ms] */
-	rs485->delay_rts_before_send = min(rs485->delay_rts_before_send, 100U);
-	rs485->delay_rts_after_send = min(rs485->delay_rts_after_send, 100U);
-	memset(rs485->padding, 0, sizeof(rs485->padding));
-
 	if (port->rs485.flags & SER_RS485_ENABLED)
 		pl011_rs485_tx_stop(uap);
 
-	/* Set new configuration */
-	port->rs485 = *rs485;
-
 	/* Make sure auto RTS is disabled */
-	if (port->rs485.flags & SER_RS485_ENABLED) {
+	if (rs485->flags & SER_RS485_ENABLED) {
 		u32 cr = pl011_read(uap, REG_CR);
 
 		cr &= ~UART011_CR_RTSEN;
-- 
2.34.1

[PATCH 3/9] serial: stm32: remove redundant code in rs485_config

[Lino Sanfilippo]

When RS485 is configured by userspace the serial core already ensures valid
RTS settings and assigns the configuration to the uart port. So remove
these tasks from the code of the drivers config_rs485 function to avoid
redundancy.

Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
---
 drivers/tty/serial/stm32-usart.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/serial/stm32-usart.c b/drivers/tty/serial/stm32-usart.c
index 1b3a611ac39e..6a014168102c 100644
--- a/drivers/tty/serial/stm32-usart.c
+++ b/drivers/tty/serial/stm32-usart.c
@@ -107,8 +107,6 @@ static int stm32_usart_config_rs485(struct uart_port *port,
 
 	stm32_usart_clr_bits(port, ofs->cr1, BIT(cfg->uart_enable_bit));
 
-	port->rs485 = *rs485conf;
-
 	rs485conf->flags |= SER_RS485_RX_DURING_TX;
 
 	if (rs485conf->flags & SER_RS485_ENABLED) {
@@ -128,13 +126,10 @@ static int stm32_usart_config_rs485(struct uart_port *port,
 					     rs485conf->delay_rts_after_send,
 					     baud);
 
-		if (rs485conf->flags & SER_RS485_RTS_ON_SEND) {
+		if (rs485conf->flags & SER_RS485_RTS_ON_SEND)
 			cr3 &= ~USART_CR3_DEP;
-			rs485conf->flags &= ~SER_RS485_RTS_AFTER_SEND;
-		} else {
+		else
 			cr3 |= USART_CR3_DEP;
-			rs485conf->flags |= SER_RS485_RTS_AFTER_SEND;
-		}
 
 		writel_relaxed(cr3, port->membase + ofs->cr3);
 		writel_relaxed(cr1, port->membase + ofs->cr1);
-- 
2.34.1

[PATCH 4/9] serial: sc16is7xx: remove redundant code in rs485_config

[Lino Sanfilippo]

When RS485 is configured by userspace the serial core already checks for
valid RTS settings. So remove the check from the implementation of the
sc16is7xx drivers rs485_config function to avoid redundancy.

Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
---
 drivers/tty/serial/sc16is7xx.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 64e7e6c8145f..730f697bb517 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -959,16 +959,6 @@ static int sc16is7xx_config_rs485(struct uart_port *port,
 	struct sc16is7xx_one *one = to_sc16is7xx_one(port, port);
 
 	if (rs485->flags & SER_RS485_ENABLED) {
-		bool rts_during_rx, rts_during_tx;
-
-		rts_during_rx = rs485->flags & SER_RS485_RTS_AFTER_SEND;
-		rts_during_tx = rs485->flags & SER_RS485_RTS_ON_SEND;
-
-		if (rts_during_rx == rts_during_tx)
-			dev_err(port->dev,
-				"unsupported RTS signalling on_send:%d after_send:%d - exactly one of RS485 RTS flags should be set\n",
-				rts_during_tx, rts_during_rx);
-
 		/*
 		 * RTS signal is handled by HW, it's timing can't be influenced.
 		 * However, it's sometimes useful to delay TX even without RTS
-- 
2.34.1

[PATCH 5/9] serial: omap: remove redundant code in rs485_config

[Lino Sanfilippo]

When RS485 is configured by userspace the serial core already clamps the
RTS delays and assigns the configuration to the uart port. So remove these
tasks from the code of the drivers rs485_config function to avoid
redundancy.

Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
---
 drivers/tty/serial/omap-serial.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index 0862941862c8..a3afcccfbd96 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -1350,18 +1350,11 @@ serial_omap_config_rs485(struct uart_port *port, struct serial_rs485 *rs485)
 	up->ier = 0;
 	serial_out(up, UART_IER, 0);
 
-	/* Clamp the delays to [0, 100ms] */
-	rs485->delay_rts_before_send = min(rs485->delay_rts_before_send, 100U);
-	rs485->delay_rts_after_send  = min(rs485->delay_rts_after_send, 100U);
-
-	/* store new config */
-	port->rs485 = *rs485;
-
 	if (up->rts_gpiod) {
 		/* enable / disable rts */
-		val = (port->rs485.flags & SER_RS485_ENABLED) ?
+		val = (rs485->flags & SER_RS485_ENABLED) ?
 			SER_RS485_RTS_AFTER_SEND : SER_RS485_RTS_ON_SEND;
-		val = (port->rs485.flags & val) ? 1 : 0;
+		val = (rs485->flags & val) ? 1 : 0;
 		gpiod_set_value(up->rts_gpiod, val);
 	}
 
@@ -1372,7 +1365,7 @@ serial_omap_config_rs485(struct uart_port *port, struct serial_rs485 *rs485)
 	/* If RS-485 is disabled, make sure the THR interrupt is fired when
 	 * TX FIFO is below the trigger level.
 	 */
-	if (!(port->rs485.flags & SER_RS485_ENABLED) &&
+	if (!(rs485->flags & SER_RS485_ENABLED) &&
 	    (up->scr & OMAP_UART_SCR_TX_EMPTY)) {
 		up->scr &= ~OMAP_UART_SCR_TX_EMPTY;
 		serial_out(up, UART_OMAP_SCR, up->scr);
-- 
2.34.1

[PATCH 6/9] serial: max310: remove redundant code in rs485_config

[Lino Sanfilippo]

When RS485 is configured by userspace the serial core already nullifies the
padding field of the returned configuration struct. Doing the same in the
drivers rs485_config function is redundant, so remove the concerning
code in this function.

Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
---
 drivers/tty/serial/max310x.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index dde0824b2fa5..2ecc5f66deaf 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -1037,7 +1037,6 @@ static int max310x_rs485_config(struct uart_port *port,
 
 	rs485->flags &= SER_RS485_RTS_ON_SEND | SER_RS485_RX_DURING_TX |
 			SER_RS485_ENABLED;
-	memset(rs485->padding, 0, sizeof(rs485->padding));
 	port->rs485 = *rs485;
 
 	schedule_work(&one->rs_work);
-- 
2.34.1

[PATCH 7/9] serial: imx: remove redundant assignment in rs485_config

[Lino Sanfilippo]

When RS485 is configured by userspace the serial core already assigns the
passed configuration to the uart port. Doing the same in the drivers
rs485_config function is redundant. So remove the concerning code in the
function.

Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
---
 drivers/tty/serial/imx.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 0b467ce8d737..ab56ff23e8a9 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1937,8 +1937,6 @@ static int imx_uart_rs485_config(struct uart_port *port,
 	    rs485conf->flags & SER_RS485_RX_DURING_TX)
 		imx_uart_start_rx(port);
 
-	port->rs485 = *rs485conf;
-
 	return 0;
 }
 
-- 
2.34.1

[PATCH 8/9] serial: fsl_lpuart: remove redundant code in rs485_config functions

[Lino Sanfilippo]

When RS485 is configured by userspace the serial core already checks for
valid RTS settings and assigns the configuration to the uart port. So
remove both tasks from the code of the lpuart_config_rs485 and the
lpuart32_config_rs485 function to avoid redundancy.

Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
---
 drivers/tty/serial/fsl_lpuart.c | 32 --------------------------------
 1 file changed, 32 deletions(-)

diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index 7d90c5a530ee..a201be44d68a 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -1377,19 +1377,6 @@ static int lpuart_config_rs485(struct uart_port *port,
 		/* Enable auto RS-485 RTS mode */
 		modem |= UARTMODEM_TXRTSE;
 
-		/*
-		 * RTS needs to be logic HIGH either during transfer _or_ after
-		 * transfer, other variants are not supported by the hardware.
-		 */
-
-		if (!(rs485->flags & (SER_RS485_RTS_ON_SEND |
-				SER_RS485_RTS_AFTER_SEND)))
-			rs485->flags |= SER_RS485_RTS_ON_SEND;
-
-		if (rs485->flags & SER_RS485_RTS_ON_SEND &&
-				rs485->flags & SER_RS485_RTS_AFTER_SEND)
-			rs485->flags &= ~SER_RS485_RTS_AFTER_SEND;
-
 		/*
 		 * The hardware defaults to RTS logic HIGH while transfer.
 		 * Switch polarity in case RTS shall be logic HIGH
@@ -1402,9 +1389,6 @@ static int lpuart_config_rs485(struct uart_port *port,
 			modem |= UARTMODEM_TXRTSPOL;
 	}
 
-	/* Store the new configuration */
-	sport->port.rs485 = *rs485;
-
 	writeb(modem, sport->port.membase + UARTMODEM);
 	return 0;
 }
@@ -1428,19 +1412,6 @@ static int lpuart32_config_rs485(struct uart_port *port,
 		/* Enable auto RS-485 RTS mode */
 		modem |= UARTMODEM_TXRTSE;
 
-		/*
-		 * RTS needs to be logic HIGH either during transfer _or_ after
-		 * transfer, other variants are not supported by the hardware.
-		 */
-
-		if (!(rs485->flags & (SER_RS485_RTS_ON_SEND |
-				SER_RS485_RTS_AFTER_SEND)))
-			rs485->flags |= SER_RS485_RTS_ON_SEND;
-
-		if (rs485->flags & SER_RS485_RTS_ON_SEND &&
-				rs485->flags & SER_RS485_RTS_AFTER_SEND)
-			rs485->flags &= ~SER_RS485_RTS_AFTER_SEND;
-
 		/*
 		 * The hardware defaults to RTS logic HIGH while transfer.
 		 * Switch polarity in case RTS shall be logic HIGH
@@ -1453,9 +1424,6 @@ static int lpuart32_config_rs485(struct uart_port *port,
 			modem |= UARTMODEM_TXRTSPOL;
 	}
 
-	/* Store the new configuration */
-	sport->port.rs485 = *rs485;
-
 	lpuart32_write(&sport->port, modem, UARTMODIR);
 	return 0;
 }
-- 
2.34.1

[PATCH 9/9] serial: atmel: remove redundant assignment in rs485_config

[Lino Sanfilippo]

When RS485 is configured by userspace the serial core already assigns the
passed configuration to the uart port. So remove the assignment from the
drivers rs485_config function to avoid redundancy.

Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
---
 drivers/tty/serial/atmel_serial.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index 2d09a89974a2..2ab589a3d86c 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -299,11 +299,9 @@ static int atmel_config_rs485(struct uart_port *port,
 	/* Resetting serial mode to RS232 (0x0) */
 	mode &= ~ATMEL_US_USMODE;
 
-	port->rs485 = *rs485conf;
-
 	if (rs485conf->flags & SER_RS485_ENABLED) {
 		dev_dbg(port->dev, "Setting UART to RS485\n");
-		if (port->rs485.flags & SER_RS485_RX_DURING_TX)
+		if (rs485conf->flags & SER_RS485_RX_DURING_TX)
 			atmel_port->tx_done_mask = ATMEL_US_TXRDY;
 		else
 			atmel_port->tx_done_mask = ATMEL_US_TXEMPTY;
-- 
2.34.1

Re: [PATCH 1/9] serial: core: move RS485 configuration tasks from drivers into core

[Jiri Slaby]

On 13. 02. 22, 23:27, Lino Sanfilippo wrote:
> Several drivers that support setting the RS485 configuration via userspace
> implement on or more of the following tasks:
> 
> - in case of an invalid RTS configuration (both RTS after send and RTS on
>    send set or both unset) fall back to enable RTS on send and disable RTS
>    after send
> 
> - nullify the padding field of the returned serial_rs485 struct
> 
> - copy the configuration into the uart port struct
> 
> - limit RTS delays to 100 ms
> 
> Move these tasks into the serial core to make them generic and to provide
> a consistent beheviour among all drivers.
> 
> Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
> ---
>   drivers/tty/serial/serial_core.c | 13 +++++++++++++
>   1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 846192a7b4bf..3fab4070359c 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -1282,8 +1282,21 @@ static int uart_set_rs485_config(struct uart_port *port,
>   	if (copy_from_user(&rs485, rs485_user, sizeof(*rs485_user)))
>   		return -EFAULT;
>   
> +	/* pick sane settings if the user hasn't */
> +	if (!(rs485.flags & SER_RS485_RTS_ON_SEND) ==
> +	    !(rs485.flags & SER_RS485_RTS_AFTER_SEND)) {
> +		rs485.flags |= SER_RS485_RTS_ON_SEND;
> +		rs485.flags &= ~SER_RS485_RTS_AFTER_SEND;
> +	}
> +	/* clamp the delays to [0, 100ms] */
> +	rs485.delay_rts_before_send = min(rs485.delay_rts_before_send, 100U);
> +	rs485.delay_rts_after_send = min(rs485.delay_rts_after_send, 100U);

Why is this magic 100? Can we have that number somehow documented? You 
should define a macro for that anyway.

> +	memset(rs485.padding, 0, sizeof(rs485.padding));

What is this memset good for?

>   	spin_lock_irqsave(&port->lock, flags);
>   	ret = port->rs485_config(port, &rs485);
> +	if (!ret)
> +		port->rs485 = rs485;
>   	spin_unlock_irqrestore(&port->lock, flags);
>   	if (ret)
>   		return ret;

thanks,
-- 
js
suse labs

Re: [PATCH 1/9] serial: core: move RS485 configuration tasks from drivers into core

[Uwe Kleine-König]

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

On Sun, Feb 13, 2022 at 11:27:29PM +0100, Lino Sanfilippo wrote:
> Several drivers that support setting the RS485 configuration via userspace
> implement on or more of the following tasks:

s/on/one/

> 
> - in case of an invalid RTS configuration (both RTS after send and RTS on
>   send set or both unset) fall back to enable RTS on send and disable RTS
>   after send
> 
> - nullify the padding field of the returned serial_rs485 struct
> 
> - copy the configuration into the uart port struct
> 
> - limit RTS delays to 100 ms
> 
> Move these tasks into the serial core to make them generic and to provide
> a consistent beheviour among all drivers.

s/beheviour/behaviour/

> Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
> ---
>  drivers/tty/serial/serial_core.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 846192a7b4bf..3fab4070359c 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -1282,8 +1282,21 @@ static int uart_set_rs485_config(struct uart_port *port,
>  	if (copy_from_user(&rs485, rs485_user, sizeof(*rs485_user)))
>  		return -EFAULT;
>  
> +	/* pick sane settings if the user hasn't */
> +	if (!(rs485.flags & SER_RS485_RTS_ON_SEND) ==
> +	    !(rs485.flags & SER_RS485_RTS_AFTER_SEND)) {
> +		rs485.flags |= SER_RS485_RTS_ON_SEND;
> +		rs485.flags &= ~SER_RS485_RTS_AFTER_SEND;
> +	}
> +	/* clamp the delays to [0, 100ms] */
> +	rs485.delay_rts_before_send = min(rs485.delay_rts_before_send, 100U);
> +	rs485.delay_rts_after_send = min(rs485.delay_rts_after_send, 100U);
> +	memset(rs485.padding, 0, sizeof(rs485.padding));
> +
>  	spin_lock_irqsave(&port->lock, flags);
>  	ret = port->rs485_config(port, &rs485);
> +	if (!ret)
> +		port->rs485 = rs485;

I was only Cc:d for the imx patch (patch #7) and tried to verify the
claim there that "the serial core already assigns the passed
configuration to the uart port". That failed when I looked at my kernel
tree.

So it would be great, if you sent dependencies (or at least a cover
letter) to all recipients of a given patch to ease review. Also I want
to suggest to mention uart_set_rs485_config() in the commit log of the
imx patch (and probably the others) to simplify verifying the claim
there.

Thanks
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/9] serial: core: move RS485 configuration tasks from drivers into core

[Lino Sanfilippo]

Hi,

On 14.02.22 at 08:06, Uwe Kleine-König wrote:
> On Sun, Feb 13, 2022 at 11:27:29PM +0100, Lino Sanfilippo wrote:
>> Several drivers that support setting the RS485 configuration via userspace
>> implement on or more of the following tasks:
>
> s/on/one/

Ok

>
>>
>> - in case of an invalid RTS configuration (both RTS after send and RTS on
>>   send set or both unset) fall back to enable RTS on send and disable RTS
>>   after send
>>
>> - nullify the padding field of the returned serial_rs485 struct
>>
>> - copy the configuration into the uart port struct
>>
>> - limit RTS delays to 100 ms
>>
>> Move these tasks into the serial core to make them generic and to provide
>> a consistent beheviour among all drivers.
>
> s/beheviour/behaviour/
>

Ok

>> Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
>> ---
>>  drivers/tty/serial/serial_core.c | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>
>> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
>> index 846192a7b4bf..3fab4070359c 100644
>> --- a/drivers/tty/serial/serial_core.c
>> +++ b/drivers/tty/serial/serial_core.c
>> @@ -1282,8 +1282,21 @@ static int uart_set_rs485_config(struct uart_port *port,
>>  	if (copy_from_user(&rs485, rs485_user, sizeof(*rs485_user)))
>>  		return -EFAULT;
>>
>> +	/* pick sane settings if the user hasn't */
>> +	if (!(rs485.flags & SER_RS485_RTS_ON_SEND) ==
>> +	    !(rs485.flags & SER_RS485_RTS_AFTER_SEND)) {
>> +		rs485.flags |= SER_RS485_RTS_ON_SEND;
>> +		rs485.flags &= ~SER_RS485_RTS_AFTER_SEND;
>> +	}
>> +	/* clamp the delays to [0, 100ms] */
>> +	rs485.delay_rts_before_send = min(rs485.delay_rts_before_send, 100U);
>> +	rs485.delay_rts_after_send = min(rs485.delay_rts_after_send, 100U);
>> +	memset(rs485.padding, 0, sizeof(rs485.padding));
>> +
>>  	spin_lock_irqsave(&port->lock, flags);
>>  	ret = port->rs485_config(port, &rs485);
>> +	if (!ret)
>> +		port->rs485 = rs485;
>
> I was only Cc:d for the imx patch (patch #7) and tried to verify the
> claim there that "the serial core already assigns the passed
> configuration to the uart port". That failed when I looked at my kernel
> tree.
>
> So it would be great, if you sent dependencies (or at least a cover
> letter) to all recipients of a given patch to ease review. Also I want
> to suggest to mention uart_set_rs485_config() in the commit log of the
> imx patch (and probably the others) to simplify verifying the claim
> there.
>

Thanks for the review, I will correct the typos in the next version.
I will also cc you directly for the next version if you dont mind.
get_maintainers only spit out "Pengutronix Kernel Team" so I used that
address for the whole series (including the cover letter).


Regards,
Lino

Re: [PATCH 1/9] serial: core: move RS485 configuration tasks from drivers into core

[Lino Sanfilippo]

Hi,

On 14.02.22 at 06:41, Jiri Slaby wrote:
> On 13. 02. 22, 23:27, Lino Sanfilippo wrote:
>> Several drivers that support setting the RS485 configuration via userspace
>> implement on or more of the following tasks:
>>
>> - in case of an invalid RTS configuration (both RTS after send and RTS on
>>    send set or both unset) fall back to enable RTS on send and disable RTS
>>    after send
>>
>> - nullify the padding field of the returned serial_rs485 struct
>>
>> - copy the configuration into the uart port struct
>>
>> - limit RTS delays to 100 ms
>>
>> Move these tasks into the serial core to make them generic and to provide
>> a consistent beheviour among all drivers.
>>
>> Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
>> ---
>>   drivers/tty/serial/serial_core.c | 13 +++++++++++++
>>   1 file changed, 13 insertions(+)
>>
>> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
>> index 846192a7b4bf..3fab4070359c 100644
>> --- a/drivers/tty/serial/serial_core.c
>> +++ b/drivers/tty/serial/serial_core.c
>> @@ -1282,8 +1282,21 @@ static int uart_set_rs485_config(struct uart_port *port,
>>       if (copy_from_user(&rs485, rs485_user, sizeof(*rs485_user)))
>>           return -EFAULT;
>>   +    /* pick sane settings if the user hasn't */
>> +    if (!(rs485.flags & SER_RS485_RTS_ON_SEND) ==
>> +        !(rs485.flags & SER_RS485_RTS_AFTER_SEND)) {
>> +        rs485.flags |= SER_RS485_RTS_ON_SEND;
>> +        rs485.flags &= ~SER_RS485_RTS_AFTER_SEND;
>> +    }
>> +    /* clamp the delays to [0, 100ms] */
>> +    rs485.delay_rts_before_send = min(rs485.delay_rts_before_send, 100U);
>> +    rs485.delay_rts_after_send = min(rs485.delay_rts_after_send, 100U);
>
> Why is this magic 100?

The only drivers that seem to care about a max value for the RTS delays use 100 ms
(omap-serial, amba pl011, 8250) so I chose this to stay compatible with the current
driver implementations. 100 ms also seems large enough to be used as a general max value.

> Can we have that number somehow documented? You should define a macro for that anyway.

Ok, I will do so.

>
>> +    memset(rs485.padding, 0, sizeof(rs485.padding));
>
> What is this memset good for?

Drivers like max310x, amba-pl011, 8250_pci, 8250_fintek, 8250_lpc18xx seem to care about
returning a serial_rs485 struct with cleared padding field to userspace. So they all clear
that field on their own. Although not really necessary, to me this seems to be a good
default behavior, so I added it to the serial core.

>
>>       spin_lock_irqsave(&port->lock, flags);
>>       ret = port->rs485_config(port, &rs485);
>> +    if (!ret)
>> +        port->rs485 = rs485;
>>       spin_unlock_irqrestore(&port->lock, flags);
>>       if (ret)
>>           return ret;
>
> thanks,

Thanks for the review!

Regards,
Lino

Re: [PATCH 1/9] serial: core: move RS485 configuration tasks from drivers into core

[Uwe Kleine-König]

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

Hello Lino,

On Mon, Feb 14, 2022 at 04:09:53PM +0100, Lino Sanfilippo wrote:
> On 14.02.22 at 08:06, Uwe Kleine-König wrote:
> > I was only Cc:d for the imx patch (patch #7) and tried to verify the
> > claim there that "the serial core already assigns the passed
> > configuration to the uart port". That failed when I looked at my kernel
> > tree.
> >
> > So it would be great, if you sent dependencies (or at least a cover
> > letter) to all recipients of a given patch to ease review. Also I want
> > to suggest to mention uart_set_rs485_config() in the commit log of the
> > imx patch (and probably the others) to simplify verifying the claim
> > there.
> 
> Thanks for the review, I will correct the typos in the next version.
> I will also cc you directly for the next version if you dont mind.

I don't mind. I get so many patches by mail, I'm good at ignoring them
;-)

> get_maintainers only spit out "Pengutronix Kernel Team" so I used that
> address for the whole series (including the cover letter).

That's why I eventually found the whole series and could reply to patch
#1.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]