On Fri, May 31, 2019 at 07:52:51AM +0300, Sergey Organov wrote: > Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx> writes: > > > On Thu, May 30, 2019 at 06:29:43PM +0300, Sergey Organov wrote: > >> imx_set_mctrl() had TIOCM_DTR meaning inverted > >> > >> Signed-off-by: Sergey Organov <sorganov@xxxxxxxxx> > >> --- > >> drivers/tty/serial/imx.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c > >> index dff75dc..e9e812a 100644 > >> --- a/drivers/tty/serial/imx.c > >> +++ b/drivers/tty/serial/imx.c > >> @@ -974,7 +974,7 @@ static void imx_uart_set_mctrl(struct uart_port *port, unsigned int mctrl) > >> } > >> > >> ucr3 = imx_uart_readl(sport, UCR3) & ~UCR3_DSR; > >> - if (!(mctrl & TIOCM_DTR)) > >> + if (mctrl & TIOCM_DTR) > >> ucr3 |= UCR3_DSR; > >> imx_uart_writel(sport, ucr3, UCR3); > > > > I'm not sure this is right and your commit log is too short to convince > > me otherwise. > > > > In the past I had several customers that used handshaking on an imx UART > > so I'd be surprised if such a bug would have stayed unnoticed until now. > > > > The i.MX25 Reference manual states: > > > > This bit [UCR3_DSR] used by software to control the DSR/DTR output pin > > for the modem interface. In DCE mode it applies to DSR and in DTE mode > > it applies to DTR. > > > > 0 DSR/ DTR pin is logic zero > > 1 DSR/ DTR pin is logic one > > > > > > Semantically if TIOCM_DTR is set in .set_mctrl, the DTR output should > > become active (i.e. low). Without testing I'm not sure if "active" > > corresponds to "logic one" which would make your patch correct. > > Yeah, I was not sure myself, and this pin is not wired on my board, so > in fact I can't check either. > > Unfortunately, the language in the manual is inconsistent. For CTS/ RTS > is clearly says: > > 0 The CTS_B pin is high (inactive) > 1 The CTS_B pin is low (active) Ack. @NXP: Maybe you can pick up this and improve the documentation here. > and the code in the driver is: > > if (mctrl & TIOCM_RTS) > ucr2 |= UCR2_CTS; > > that works correct. > > My best reasoning was that DSR/ DTR is likely implemented the same as > CTS/ RTS in the metal, and I found other drivers where both RTS and DSR > are inverted, so I guessed it could be a remnant of old copy-paste. This is not a good enough reason to "fix" that. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ |
