Subject: Re: [PATCH v4 1/3] serial: imx: set_termios(): do not enable autoRTS if RTS is unset

From: Sergey Organov <[email-masked]>
Date: Mon, 22 Jul 2019 10:42:57 +0300

Uwe Kleine-König <[email-masked]> writes:

> On Fri, Jul 19, 2019 at 06:13:52PM +0300, Sergey Organov wrote:
>> Hello Uwe,
>> 
>> Uwe Kleine-König <[email-masked]> writes:
>> > Hello Sergey,
>> >
>> > On Fri, Jul 19, 2019 at 03:18:13PM +0300, Sergey Organov wrote:
>> >> Uwe Kleine-König <[email-masked]> writes:
>> >> > On Fri, Jul 19, 2019 at 11:47:52AM +0300, Sergey Organov wrote:
>> >> >> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
>> >> >> index 57d6e6b..95d7984 100644
>> >> >> --- a/drivers/tty/serial/imx.c
>> >> >> +++ b/drivers/tty/serial/imx.c
>> >> >> @@ -405,7 +405,8 @@ static void imx_uart_rts_inactive(struct imx_port *sport, u32 *ucr2)
>> >> >>  /* called with port.lock taken and irqs caller dependent */
>> >> >>  static void imx_uart_rts_auto(struct imx_port *sport, u32 *ucr2)
>> >> >>  {
>> >> >> -	*ucr2 |= UCR2_CTSC;
>> >> >> +	if (*ucr2 & UCR2_CTS)
>> >> >> +		*ucr2 |= UCR2_CTSC;
>> >> >
>> >> > I think this patch is wrong or the commit log is insufficient.
>> >> > imx_uart_rts_auto() has only a single caller and there ucr2 & UCR2_CTS is
>> >> > never true. And CTSC isn't restored anywhere, is it?
>> >> 
>> >> This is rebase to 'tty-next' branch, and you need to look at it in that
>> >> context. There, ucr2 & UCR2_CTS does already make sense, due to previous
>> >> fix that is already there.
>> >
>> > I looked at 57d6e6b which is the file you patched. And there
>> > imx_uart_rts_auto is only ever called with *ucr2 not having UCR2_CTS.
>> >
>> > If you still think I'm wrong, please improve the commit log
>> > accordingly.
>> 
>> I still think you are wrong, but I don't know how to improve commit log.
>> 
>> To check it once again, I just did:
>> 
>> $ git show 57d6e6b > imx.c
>> 
>> There, in imx_uart_set_termios(), I see:
>> 
>> 1569:	old_ucr2 = imx_uart_readl(sport, UCR2);
>> 1570:	ucr2 = old_ucr2 & (UCR2_TXEN | UCR2_RXEN | UCR2_ATEN | UCR2_CTS);
>> 
>> Here, current UCR2 value is read into 'old_ucr2' and then its /current/
>> UCR2_CTS bit is copied into 'ucr2' (along with 3 other bits).
>> 
>> Then, later in the same function:
>> 
>> 1591:		imx_uart_rts_auto(sport, &ucr2);
>> 
>> is called that can check /current/ state of UCR2_CTS bit in '*ucr2'.
>> 
>> That's what the patch does, checks this bit.
>> 
>> Sorry, I fail to see how you came to conclusion that "*ucr2 not having
>> UCR2_CTS". Do we maybe still read different versions of the file?
>
> No, it's just that I failed to see that UCR2_CTS is in the set of bits
> that are retained even when looking twice :-|

Ah, that one... How familiar :-)

> So you convinced me that you are right and if you update the commit log
> as agreed already before and even add a comment in imx_uart_rts_auto
> along the lines of
>
> 	/*
> 	 * Only let the receiver control RTS output if we were not
> 	 * requested to have RTS inactive (which then should take
> 	 * precedence).
> 	 */
>
> you can have my Ack.

OK, will do

Thanks!

-- Sergey

_______________________________________________
linux-arm-kernel mailing list
[email-masked]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel