Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
>> Tested-by: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
>> Signed-off-by: Sergey Organov <sorganov@xxxxxxxxx>
>> ---
>> 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
