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

From: Sergey Organov <sorganov@gmail•com>
To: "Uwe Kleine-König" <u.kleine-koenig@pengutronix•de>
Cc: linux-serial@vger•kernel.org,
	Greg Kroah-Hartman <gregkh@linuxfoundation•org>,
	Sascha Hauer <s.hauer@pengutronix•de>,
	NXP Linux Team <linux-imx@nxp•com>,
	Pengutronix Kernel Team <kernel@pengutronix•de>,
	linux-arm-kernel@lists•infradead.org
Subject: Re: [PATCH v4 1/3] serial: imx: set_termios(): do not enable autoRTS if RTS is unset
Date: Fri, 19 Jul 2019 18:13:52 +0300	[thread overview]
Message-ID: <87woge9hvz.fsf@osv.gnss.ru> (raw)
In-Reply-To: <20190719143151.gx43ndn2oy35h247@pengutronix.de> ("Uwe \=\?utf-8\?Q\?Kleine-K\=C3\=B6nig\=22's\?\= message of "Fri, 19 Jul 2019 16:31:51 +0200")

Hello Uwe,

Uwe Kleine-König <u.kleine-koenig@pengutronix•de> writes:
> Hello Sergey,
>
> On Fri, Jul 19, 2019 at 03:18:13PM +0300, Sergey Organov wrote:
>> Uwe Kleine-König <u.kleine-koenig@pengutronix•de> 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?

-- Sergey

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