Re: [PATCH v5 tty-next 3/4] 8250: microchip: pci1xxxx: Add RS485 support to quad-uart driver

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Thu, 17 Nov 2022, Kumaravel Thiagarajan wrote:

> pci1xxxx uart supports RS485 mode of operation in the hardware with
> auto-direction control with configurable delay for releasing RTS after
> the transmission. This patch adds support for the RS485 mode.
> 
> Co-developed-by: Tharun Kumar P <tharunkumar.pasumarthi@microchip.com>
> Signed-off-by: Tharun Kumar P <tharunkumar.pasumarthi@microchip.com>
> Signed-off-by: Kumaravel Thiagarajan <kumaravel.thiagarajan@microchip.com>
> ---
> Changes in v5:
> - Removed unnecessary assignments
> - Corrected styling issues in comments
> 
> Changes in v4:
> - No Change
> 
> Changes in v3:
> - Remove flags sanitization in driver which is taken care in core
> 
> Changes in v2:
> - move pci1xxxx_rs485_config to a separate patch with
>   pci1xxxx_rs485_supported.
> ---
>  drivers/tty/serial/8250/8250_pci1xxxx.c | 50 +++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
> 
> diff --git a/drivers/tty/serial/8250/8250_pci1xxxx.c b/drivers/tty/serial/8250/8250_pci1xxxx.c
> index 02b9c6959dcc..bead9fd4019e 100644
> --- a/drivers/tty/serial/8250/8250_pci1xxxx.c
> +++ b/drivers/tty/serial/8250/8250_pci1xxxx.c
> @@ -139,6 +139,54 @@ static void pci1xxxx_set_divisor(struct uart_port *port, unsigned int baud,
>  	       port->membase + UART_BAUD_CLK_DIVISOR_REG);
>  }
>  
> +static int pci1xxxx_rs485_config(struct uart_port *port,
> +				 struct ktermios *termios,
> +				 struct serial_rs485 *rs485)
> +{
> +	u32 clock_div = readl(port->membase + UART_BAUD_CLK_DIVISOR_REG);
> +	u8 delay_in_baud_periods;
> +	u32 baud_period_in_ns;
> +	u32 data = 0;
> +
> +	/*
> +	 * pci1xxxx's uart hardware supports only RTS delay after
> +	 * Tx and in units of bit times to a maximum of 15
> +	 */
> +	if (rs485->flags & SER_RS485_ENABLED) {
> +		data = ADCL_CFG_EN | ADCL_CFG_PIN_SEL;
> +
> +		if (!(rs485->flags & SER_RS485_RTS_ON_SEND))
> +			data |= ADCL_CFG_POL_SEL;
> +
> +		if (rs485->delay_rts_after_send) {
> +			baud_period_in_ns =
> +				FIELD_GET(BAUD_CLOCK_DIV_INT_MSK, clock_div) *
> +				UART_BIT_SAMPLE_CNT;
> +			delay_in_baud_periods =
> +				(rs485->delay_rts_after_send * NSEC_PER_MSEC) /
> +				baud_period_in_ns;
> +			delay_in_baud_periods =
> +				min_t(u8, delay_in_baud_periods,
> +				      FIELD_MAX(ADCL_CFG_RTS_DELAY_MASK));

I'm a bit worried about overflows here. If the division result does not 
fit into u8, you might not get the max value value but some pseudo 
garbage from LSB of the division result. Also note that min_t casts first,
not after the compare (but that trap is currently hidden by the earlier 
loss of bits).

> +			data |= FIELD_PREP(ADCL_CFG_RTS_DELAY_MASK,
> +					   delay_in_baud_periods);
> +			rs485->delay_rts_after_send =
> +				(baud_period_in_ns * delay_in_baud_periods) /
> +				NSEC_PER_MSEC;

> +			rs485->delay_rts_before_send = 0;

This should be handled by the core (IIRC) when it is not set in 
pci1xxxx_rs485_supported.

-- 
 i.


> +		}
> +	}
> +	writel(data, port->membase + ADCL_CFG_REG);
> +	return 0;
> +}
> +
> +static const struct serial_rs485 pci1xxxx_rs485_supported = {
> +	.flags = SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND |
> +		 SER_RS485_RTS_AFTER_SEND,
> +	.delay_rts_after_send = 1,
> +	/* Delay RTS before send is not supported */
> +};
> +
>  static int pci1xxxx_setup(struct pci1xxxx_8250 *priv,
>  			  struct uart_8250_port *port, int idx)
>  {
> @@ -193,6 +241,8 @@ static int pci1xxxx_setup(struct pci1xxxx_8250 *priv,
>  	port->port.set_termios = serial8250_do_set_termios;
>  	port->port.get_divisor = pci1xxxx_get_divisor;
>  	port->port.set_divisor = pci1xxxx_set_divisor;
> +	port->port.rs485_config = pci1xxxx_rs485_config;
> +	port->port.rs485_supported = pci1xxxx_rs485_supported;
>  	ret = serial8250_pci_setup_port(priv->dev, port, 0, offset, 0);
>  	if (ret < 0)
>  		return ret;
>