| Message ID | 1564167161-3972-4-git-send-email-sorganov@gmail.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | b777b5de6aaa98df37c0fe1f7a33fa1c63d0e326 |
| Headers | show |
| Series | serial: imx: fix RTS and RTS/CTS handling | expand |
On Fri, Jul 26, 2019 at 09:52:41PM +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. I don't share the critic. IMHO the name is fine. imx_uart_rts_inactive sets rts to its inactive level, imx_uart_rts_active() to its active level and imx_uart_rts_auto() lets the output drive automatically by the receiver. The name started to be a bit wrong in patch 1 of the series however. And I still object removing this function because with the semantic this function got in patch 1 it is suiteable to be used in imx_uart_set_mctrl(). Best regards Uwe
Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes: > On Fri, Jul 26, 2019 at 09:52:41PM +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. > > I don't share the critic. IMHO the name is fine. imx_uart_rts_inactive > sets rts to its inactive level, > imx_uart_rts_active() to its active level Not exactly, in fact both do more than that, in a similar manner. > imx_uart_rts_auto() lets the output drive automatically by the > receiver. And this one was different and it was rather confusing when I've tried to grok the logic of the driver. > The name started to be a bit wrong in patch 1 of the series however. The function was different from first two even before the patch, as it does not do any of those additional things the first two do. > And I still object removing this function because with the semantic > this function got in patch 1 it is suiteable to be used in > imx_uart_set_mctrl(). It is not, as it does require change to be used there, as we've already seen, and then it becomes very different function from what it was at the beginning. Even then, the end result I've shown you when attempting to somehow preserve some re-incarnation of this function still seems more cumbersome to me than the end result of these patches. That said, this a matter of taste and style, not correctness, and could be changed as a follow-up, not to risk breaking already tested patch series. Thanks, -- Sergey
On Mon, Jul 29, 2019 at 12:03:07PM +0300, Sergey Organov wrote: > Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes: > > > On Fri, Jul 26, 2019 at 09:52:41PM +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. > > > > I don't share the critic. IMHO the name is fine. imx_uart_rts_inactive > > sets rts to its inactive level, > > imx_uart_rts_active() to its active level > > Not exactly, in fact both do more than that, in a similar manner. They both handle mctrl-gpio, the autorts stuff isn't available for that, so we could fix that by letting rts-auto set the RTS gpio to active. > > imx_uart_rts_auto() lets the output drive automatically by the > > receiver. > > And this one was different and it was rather confusing when I've tried > to grok the logic of the driver. > > > The name started to be a bit wrong in patch 1 of the series however. > > The function was different from first two even before the patch, as it > does not do any of those additional things the first two do. > > > And I still object removing this function because with the semantic > > this function got in patch 1 it is suiteable to be used in > > imx_uart_set_mctrl(). > > It is not, as it does require change to be used there, as we've already > seen, and then it becomes very different function from what it was at > the beginning. > > Even then, the end result I've shown you when attempting to somehow preserve > some re-incarnation of this function still seems more cumbersome to me > than the end result of these patches. > > That said, this a matter of taste and style, not correctness, and could > be changed as a follow-up, not to risk breaking already tested patch > series. *shrug* I stop caring here. Best regards Uwe
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c index 059ba35..d9a73c7 100644 --- a/drivers/tty/serial/imx.c +++ b/drivers/tty/serial/imx.c @@ -402,17 +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) -{ - /* - * Only let receiver control RTS output if we were not requested to have - * RTS inactive (which then should take precedence). - */ - 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) { @@ -1604,8 +1593,14 @@ 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) { + /* + * Only let receiver control RTS output if we were not requested + * to have RTS inactive (which then should take precedence). + */ + if (ucr2 & UCR2_CTS) + ucr2 |= UCR2_CTSC; + } if (termios->c_cflag & CRTSCTS) ucr2 &= ~UCR2_IRTS;