Re: [PATCH v2 6/7] serial: imx: set_mctrl(): correctly restore autoRTS state

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

On Wed, Jun 26, 2019 at 05:11:32PM +0300, Sergey Organov wrote:
> imx_uart_set_mctrl() happened to set UCR2_CTSC bit whenever TIOCM_RTS
> was set, no matter if RTS/CTS handshake is enabled or not. Now fixed by
> turning handshake on only when CRTSCTS bit for the port is set.
> 
> Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de>
> Tested-by: Sascha Hauer <s.hauer@pengutronix.de>
> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> ---
>  drivers/tty/serial/imx.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 4867f80..171347d 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -970,10 +970,19 @@ static void imx_uart_set_mctrl(struct uart_port *port, unsigned int mctrl)
>  	if (!(port->rs485.flags & SER_RS485_ENABLED)) {
>  		u32 ucr2;
>  
> +		/*
> +		 * Turn off autoRTS (UCR2_CTSC) if RTS is lowered and restore
> +		 * autoRTS setting if RTS is raised. Inverted UCR2_IRTS is set
> +		 * if and only if CRTSCTS bit is set for the port, so we use it
> +		 * to get the state to restore to.
> +		 */

The comment is quite complicated. I like the comments of Sascha's patch
that addressed the same issue better.

Are you using UCR2_IRTS as an indicator if CRTSCTS is set? If it's that
what you intend to express in the second sentence that is hard to grasp.
Something like:

	UCR2_IRTS is unset iff the port is configured for CRTSCTS

Also as the value of the CTS bit doesn't matter if CTSC is set, the
order of the checks could be swapped to result in easier code (IMHO at
least) that doesn't need a nested if.

Something like:

	ucr2 = imx_uart_readl(sport, UCR2);
	ucr2 &= ~(UCR2_CTS | UCR2_CTSC);

	/* UCR2_IRTS is unset iff the port is configured for CRTSCTS */
	crtscts = !(ucr2 & UCR2_IRTS);

	if (!(mctrl & TIOCM_RTS)) {
		/* Force RTS inactive, i.e. UCR2_CTS=0 and UCR2_CTSC=0 */
	} else if (crtscts) {
		/* let the receiver control RTS */
		ucr2 |= UCR2_CTSC;
	} else {
		/* Force RTS active */
		ucr2 |= UCR2_CTS;
	}

Best regards
Uwe

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