| Message ID | 1561558293-7683-8-git-send-email-sorganov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | serial: imx: fix RTS and RTS/CTS handling | expand |
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. Best regards Uwe
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
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; + } if (termios->c_cflag & CRTSCTS) ucr2 &= ~UCR2_IRTS;