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

[Sergey Organov <sorganov@gmail.com>]

Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:

> 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.

This one is simply modeled after similar comments in other drivers,
then adding the specifics.

> 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

Yeah, exactly. Fine, I'll change this, thanks!

>
> 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;
> 	}

Right, this is functionally correct as well, and thus it's a matter of
taste, but I still believe that what I suggested is better:

	ucr2 = imx_uart_readl(sport, UCR2);
	ucr2 &= ~(UCR2_CTS | UCR2_CTSC);
	if (mctrl & TIOCM_RTS) {
		ucr2 |= UCR2_CTS;
		if (!(ucr2 & UCR2_IRTS))
			ucr2 |= UCR2_CTSC;
	}

First, it always sets hardware RTS according to TIOCM_RTS, that IMHO is
less surprising than clearing hardware RTS bit when port is configured
CRTSCTS.

Second, (unfortunate) inter-dependency between TIOCM_RTS and CRTSCTS is
better isolated, so to get rid of it (even if only mentally), only
removals are required, that reduces the code to quite obvious:

	ucr2 = imx_uart_readl(sport, UCR2);
	ucr2 &= ~(UCR2_CTS);
	if (mctrl & TIOCM_RTS)
		ucr2 |= UCR2_CTS;

Thanks!

-- Sergey

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel