Re: [PATCH v2] tty: serial: Add RS422 flag to struct serial_rs485

[Lino Sanfilippo <LinoSanfilippo@gmx.de>]

Hi,

On 01.11.23 07:44, Crescent CY Hsieh wrote:
> Add "SER_RS422_ENABLED" flag within struct serial_rs485, so that serial
> port can switching interface into RS422 if supported by using ioctl
> command "TIOCSRS485".
>
> In case of interfaces confliction, add checks within

s/interfaces confliction/interface conflicts ?

> uart_sanitize_serial_rs485() such that only one of RS422/RS485 is set.
>
> Signed-off-by: Crescent CY Hsieh <crescentcy.hsieh@moxa.com>
>
> ---
> Changes in v2:
> - Revise the logic that checks whether RS422/RS485 are enabled
>   simultaneously.
>
> v1: https://lore.kernel.org/all/20231030053632.5109-1-crescentcy.hsieh@moxa.com/
>
> ---
>  drivers/tty/serial/serial_core.c | 19 +++++++++++++++++--
>  include/uapi/linux/serial.h      |  4 ++++
>  2 files changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 831d03361..54a104c52 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -1305,7 +1305,7 @@ static int uart_get_icount(struct tty_struct *tty,
>
>  #define SER_RS485_LEGACY_FLAGS	(SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND | \
>  				 SER_RS485_RTS_AFTER_SEND | SER_RS485_RX_DURING_TX | \
> -				 SER_RS485_TERMINATE_BUS)
> +				 SER_RS485_TERMINATE_BUS | SER_RS422_ENABLED)
>

How can this be a legacy flag when you just introduced it?

>  static int uart_check_rs485_flags(struct uart_port *port, struct serial_rs485 *rs485)
>  {
> @@ -1371,11 +1371,26 @@ static void uart_sanitize_serial_rs485(struct uart_port *port, struct serial_rs4
>  {
>  	u32 supported_flags = port->rs485_supported.flags;
>
> -	if (!(rs485->flags & SER_RS485_ENABLED)) {
> +	if (!(rs485->flags & (SER_RS485_ENABLED | SER_RS422_ENABLED))) {
>  		memset(rs485, 0, sizeof(*rs485));
>  		return;
>  	}
>
> +	/* Pick sane setting if the user enables both interfaces */> +	if (rs485->flags & SER_RS485_ENABLED && rs485->flags & SER_RS422_ENABLED) {
> +		dev_warn_ratelimited(port->dev,
> +			"%s (%d): Invalid serial interface setting, using RS485 instead\n",
> +			port->name, port->line);
> +		rs485->flags &= ~SER_RS422_ENABLED;
> +	}
> +
> +	/* Clear other bits and return if RS422 is enabled */
> +	if (rs485->flags & SER_RS422_ENABLED) {> +		memset(rs485, 0, sizeof(*rs485));

Why are all flags cleared but SER_RS422_ENABLED?

> +		rs485->flags |= SER_RS422_ENABLED;
> +		return;
> +	}

What about all the other code places that check for SER_RS485_ENABLED?
For example uart_update_mctrl(), uart_suspend_port() and uart_resume_port() check this flag
to decide whether to set the modem control lines or not. Should this not also apply to
SER_RS422_ENABLED?

Maybe it would be better to change the meaning of the flag: Instead of being a substitution for
SER_RS485_ENABLED, it could be used to mark a special mode.
So if both SER_RS485_ENABLED and SER_RS485_MODE_RS422 are set it would mean that we have RS422.


Regards,
Lino

>  	/* Pick sane settings if the user hasn't */
>  	if ((supported_flags & (SER_RS485_RTS_ON_SEND|SER_RS485_RTS_AFTER_SEND)) &&
>  	    !(rs485->flags & SER_RS485_RTS_ON_SEND) ==
> diff --git a/include/uapi/linux/serial.h b/include/uapi/linux/serial.h
> index 53bc1af67..427609fd5 100644
> --- a/include/uapi/linux/serial.h
> +++ b/include/uapi/linux/serial.h
> @@ -137,6 +137,8 @@ struct serial_icounter_struct {
>   * * %SER_RS485_ADDRB		- Enable RS485 addressing mode.
>   * * %SER_RS485_ADDR_RECV - Receive address filter (enables @addr_recv). Requires %SER_RS485_ADDRB.
>   * * %SER_RS485_ADDR_DEST - Destination address (enables @addr_dest). Requires %SER_RS485_ADDRB.
> + *
> + * * %SER_RS422_ENABLED		- RS422 enabled.
>   */
>  struct serial_rs485 {
>  	__u32	flags;
> @@ -149,6 +151,8 @@ struct serial_rs485 {
>  #define SER_RS485_ADDR_RECV		(1 << 7)
>  #define SER_RS485_ADDR_DEST		(1 << 8)
>
> +#define SER_RS422_ENABLED		(1 << 9)
> +
>  	__u32	delay_rts_before_send;
>  	__u32	delay_rts_after_send;
>