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