Re: [PATCH v2 7/7] serial: imx: get rid of imx_uart_rts_auto()

From: Sergey Organov <sorganov@gmail•com>
To: "Uwe Kleine-König" <u.kleine-koenig@pengutronix•de>
Cc: Pengutronix Kernel Team <kernel@pengutronix•de>,
	Sascha Hauer <s.hauer@pengutronix•de>,
	linux-arm-kernel@lists•infradead.org,
	linux-serial@vger•kernel.org, NXP Linux Team <linux-imx@nxp•com>
Subject: Re: [PATCH v2 7/7] serial: imx: get rid of imx_uart_rts_auto()
Date: Thu, 27 Jun 2019 10:58:13 +0300	[thread overview]
Message-ID: <87v9wrh33e.fsf@osv.gnss.ru> (raw)
In-Reply-To: <20190627060838.ls5ghzngkoiczqex@pengutronix.de> ("Uwe \=\?utf-8\?Q\?Kleine-K\=C3\=B6nig\=22's\?\= message of "Thu, 27 Jun 2019 08:08:38 +0200")

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

> On Wed, Jun 26, 2019 at 05:11:33PM +0300, Sergey Organov wrote:
>> Called in only one place, for RS232, it only obscures things, as it
>> doesn't go well with 2 similar named functions,
>> imx_uart_rts_inactive() and imx_uart_rts_active(), that both are
>> RS485-specific.
>> 
>> 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, 4 insertions(+), 9 deletions(-)
>> 
>> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
>> index 171347d..a5e80a0 100644
>> --- a/drivers/tty/serial/imx.c
>> +++ b/drivers/tty/serial/imx.c
>> @@ -402,13 +402,6 @@ static void imx_uart_rts_inactive(struct imx_port *sport, u32 *ucr2)
>>  	mctrl_gpio_set(sport->gpios, sport->port.mctrl);
>>  }
>>  
>> -/* called with port.lock taken and irqs caller dependent */
>> -static void imx_uart_rts_auto(struct imx_port *sport, u32 *ucr2)
>> -{
>> -	if (*ucr2 & UCR2_CTS)
>> -		*ucr2 |= UCR2_CTSC;
>> -}
>> -
>>  /* called with port.lock taken and irqs off */
>>  static void imx_uart_start_rx(struct uart_port *port)
>>  {
>> @@ -1598,8 +1591,10 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
>>  		else
>>  			imx_uart_rts_inactive(sport, &ucr2);
>>  
>> -	} else if (termios->c_cflag & CRTSCTS)
>> -		imx_uart_rts_auto(sport, &ucr2);
>> +	} else if (termios->c_cflag & CRTSCTS) {
>> +		if (ucr2 & UCR2_CTS)
>> +			ucr2 |= UCR2_CTSC;
>> +	}
>
> At least before it was (somewhat) clear that this is about RTS and it
> is about something automatic. So I don't like the patch.

Maybe I just need to put a comment here to clarify?

Let me try to convince you removal is a good thing.

Let's try to mentally revert the patch. If we already have

	} else if (termios->c_cflag & CRTSCTS) {
		if (ucr2 & UCR2_CTS)
			ucr2 |= UCR2_CTSC;
	}

I see no reason to make 2 lines inside if() a function.

First, it's already obvious it's about something automatic, due to if()
condition itself.

Second, the fact that it's about RTS is as [non-]obvious as in any other
place in the driver, taking into account that iMX calls "RTS" "CTS" and
vice versa.

Finally, should we still argue adding a function would be useful, we'd
need to also add, for consistency,

  static void imx_uart_rts_manual(struct imx_port *sport, u32 *ucr2);

(as existing rts_on() and rts_off() do not serve the purpose),

as well as CTS counterparts:

  static void imx_uart_cts_auto(struct imx_port *sport, u32 *ucr2);
  static void imx_uart_cts_manual(struct imx_port *sport, u32 *ucr2);

and patch the code rather heavily, for no obvious gain.

Overall, I believe adding the function would only obscure things.

OTOH, existence of that function forced me to examine the whole source
just to figure that unlike other 2 similar named, it serves entirely
different logical purpose (i.e., it's _not_ 3-d alternative for those
2), and is not used anywhere else.

Look: when we have rts_auto(), rts_off(), and rts_on(), it's logical to
expect it's one of them that will be called when top-level asks for
automatic RTS/CTS, manual RTS off, and manual RTS on, respectively,
isn't it? But it is not the case at all! Still rts_auto() doesn't fit to
the overall picture.

Thanks!

-- Sergey

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