Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 3 Oct 2022 12:04:22 +0300 (EEST)
From:   =?ISO-8859-15?Q?Ilpo_J=E4rvinen?= <ilpo.jarvinen@linux.intel.com>
To:     Kumaravel Thiagarajan <kumaravel.thiagarajan@microchip.com>
cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jiri Slaby <jirislaby@kernel.org>, andy.shevchenko@gmail.com,
        u.kleine-koenig@pengutronix.de, johan@kernel.org,
        wander@redhat.com, etremblay@distech-controls.com,
        macro@orcam.me.uk, geert+renesas@glider.be, jk@ozlabs.org,
        phil.edworthy@renesas.com, Lukas Wunner <lukas@wunner.de>,
        LKML <linux-kernel@vger.kernel.org>,
        linux-serial <linux-serial@vger.kernel.org>,
        UNGLinuxDriver@microchip.com
Subject: Re: [PATCH v2 tty-next 2/3] 8250: microchip: pci1xxxx: Add rs485
 support to quad-uart driver.
In-Reply-To: <20221001061507.3508603-3-kumaravel.thiagarajan@microchip.com>
Message-ID: <e433da81-46d5-5aad-4ce9-6d48b2e674e@linux.intel.com>
References: <20221001061507.3508603-1-kumaravel.thiagarajan@microchip.com> <20221001061507.3508603-3-kumaravel.thiagarajan@microchip.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Sat, 1 Oct 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.
> 
> Signed-off-by: Kumaravel Thiagarajan <kumaravel.thiagarajan@microchip.com>
> ---
> Changes in v2:
> - move pci1xxxx_rs485_config to a separate patch with
>   pci1xxxx_rs485_supported.
> ---
>  drivers/tty/serial/8250/8250_pci1xxxx.c | 57 +++++++++++++++++++++++++
>  1 file changed, 57 insertions(+)
> 
> diff --git a/drivers/tty/serial/8250/8250_pci1xxxx.c b/drivers/tty/serial/8250/8250_pci1xxxx.c
> index 41a4b94f52b4..999e5a284266 100644
> --- a/drivers/tty/serial/8250/8250_pci1xxxx.c
> +++ b/drivers/tty/serial/8250/8250_pci1xxxx.c
> @@ -137,6 +137,61 @@ static void pci1xxxx_set_divisor(struct uart_port *port, unsigned int baud,
>  	writel((quot << 8) | frac, (port->membase + CLK_DIVISOR_REG));
>  }
>  
> +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_rs485_config(struct uart_port *port,
> +				 struct ktermios *termios,
> +				 struct serial_rs485 *rs485)
> +{
> +	u32 clock_div = readl(port->membase + CLK_DIVISOR_REG);
> +	u8 delay_in_baud_periods = 0;
> +	u32 baud_period_in_ns = 0;
> +	u8 data = 0;
> +
> +	/* pci1xxxx's uart hardware supports only RTS delay after
> +	 * Tx and in units of bit times to a maximum of 15
> +	 */
> +
> +	rs485->flags &= SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND |
> +			SER_RS485_RTS_AFTER_SEND;

Drop this, core handles it for you.

I think I already mentioned to you that there is a lot of stuff that core 
handles for you.

> +	if (rs485->flags & SER_RS485_ENABLED) {
> +		memset(rs485->padding, 0, sizeof(rs485->padding));

Core handles this for you.

> +		data = ADCL_CFG_EN | ADCL_CFG_PIN_SEL;
> +
> +		if (!(rs485->flags & SER_RS485_RTS_ON_SEND)) {
> +			data |= ADCL_CFG_POL_SEL;
> +			rs485->flags |=  SER_RS485_RTS_AFTER_SEND;
> +		} else {
> +			rs485->flags &= ~SER_RS485_RTS_AFTER_SEND;
> +		}

Core handles that flags sanitization for you.

> +		if (rs485->delay_rts_after_send) {
> +			baud_period_in_ns = ((clock_div >> 8) * 16);

If that >> 8 is there to take part of the CLK_DIVISOR_REG register's bits, 
you want to define a mask and use FIELD_GET().

> +			delay_in_baud_periods = (rs485->delay_rts_after_send * 1000000) /

NSEC_PER_MSEC?

> +						baud_period_in_ns;
> +			if (delay_in_baud_periods > 0xF)
> +				delay_in_baud_periods = 0xF;
> +			data |= delay_in_baud_periods << 8;

You want to add something along these lines:
#define ADCL_CFG_RTS_DELAY_MASK		GENMASK(11, 8)

Then do:
			delay_in_baud_periods = min(delay_in_baud_periods, 
						    FIELD_MAX(ADCL_CFG_RTS_DELAY_MASK));
			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) /
> +						      1000000;

NSEC_PER_MSEC?

> +			rs485->delay_rts_before_send = 0;
> +		}
> +	} else {
> +		memset(rs485, 0, sizeof(*rs485));

Core handles this.

> +	}
> +
> +	writeb(data, (port->membase + ADCL_CFG_REG));
> +	port->rs485 = *rs485;

Core handles this.

> +	return 0;
> +}
> +
>  static int pci1xxxx_get_num_ports(struct pci_dev *dev)
>  {
>  	int nr_ports = 1;
> @@ -217,6 +272,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 = pci1xxxx_setup_port(priv, port, offset);
>  	if (ret < 0)
>  		return ret;
> 

-- 
 i.