diff mbox series

[1/1] drivers: tty: imx: fix flags of rs485 not work properly

Message ID 20240906021905.197891-1-zaq14760@gmail.com (mailing list archive)
State New
Headers show
Series [1/1] drivers: tty: imx: fix flags of rs485 not work properly | expand

Commit Message

LiangCheng Wang Sept. 6, 2024, 2:19 a.m. UTC 
The rs485.flags are lost in functions such as imx_uart_stop_tx(),
causing the function of RS485 to be invalid when using the
serial port as the RS485 port. Use a variable to store the state to
avoid this issue.

Signed-off-by: LiangCheng Wang <zaq14760@gmail.com>
---
 drivers/tty/serial/imx.c | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

Comments

Jiri Slaby Sept. 6, 2024, 6:03 a.m. UTC | #1 
It seems gmail refuses to send this to zaq14760@gmail.com (the author).

On 06. 09. 24, 4:19, LiangCheng Wang wrote:
> The rs485.flags are lost in functions such as imx_uart_stop_tx(),
> causing the function of RS485 to be invalid when using the
> serial port as the RS485 port. Use a variable to store the state to
> avoid this issue.

AFAICT, this feels rather wrong. Any rs485 experts around?

At minimum, how are the flags "lost" and why this does not matter to 
other drivers?

> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -209,7 +209,7 @@ struct imx_port {
>   	const struct imx_uart_data *devdata;
>   
>   	struct mctrl_gpios *gpios;
> -
> +	int flags;

Definitely not int for flags.

thanks,
Ilpo Järvinen Sept. 20, 2024, 3:58 p.m. UTC | #2 
On Fri, 6 Sep 2024, Jiri Slaby wrote:

> It seems gmail refuses to send this to zaq14760@gmail.com (the author).
> 
> On 06. 09. 24, 4:19, LiangCheng Wang wrote:
> > The rs485.flags are lost in functions such as imx_uart_stop_tx(),
> > causing the function of RS485 to be invalid when using the
> > serial port as the RS485 port. Use a variable to store the state to
> > avoid this issue.
> 
> AFAICT, this feels rather wrong. Any rs485 experts around?

It is wrong. The patch makes no sense at all and prevents 
reconfiguring/setting rs485 from userspace.

> At minimum, how are the flags "lost" and why this does not matter to other
> drivers?

Perhaps some userspace program is altering rs485 settings, definitely 
nothing in imx_uart_stop_tx() writes to it. I'm skeptical it would be a 
problem in the kernel, especially given the patch that is supposed to 
"avoid the issue" (whatever the issue is).

> > --- a/drivers/tty/serial/imx.c
> > +++ b/drivers/tty/serial/imx.c
> > @@ -209,7 +209,7 @@ struct imx_port {
> >   	const struct imx_uart_data *devdata;
> >     	struct mctrl_gpios *gpios;
> > -
> > +	int flags;
> 
> Definitely not int for flags.

Driver is not supposed to duplicate the rs485 flags at all.
diff mbox series

Patch

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 67d4a72eda77..346bbd21536b 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -209,7 +209,7 @@  struct imx_port {
 	const struct imx_uart_data *devdata;
 
 	struct mctrl_gpios *gpios;
-
+	int flags;
 	/* counter to stop 0xff flood */
 	int idle_counter;
 
@@ -434,7 +434,7 @@  static void imx_uart_stop_tx(struct uart_port *port)
 	imx_uart_writel(sport, ucr4, UCR4);
 
 	/* in rs485 mode disable transmitter */
-	if (port->rs485.flags & SER_RS485_ENABLED) {
+	if (sport->flags & SER_RS485_ENABLED) {
 		if (sport->tx_state == SEND) {
 			sport->tx_state = WAIT_AFTER_SEND;
 
@@ -454,7 +454,7 @@  static void imx_uart_stop_tx(struct uart_port *port)
 			hrtimer_try_to_cancel(&sport->trigger_start_tx);
 
 			ucr2 = imx_uart_readl(sport, UCR2);
-			if (port->rs485.flags & SER_RS485_RTS_AFTER_SEND)
+			if (sport->flags & SER_RS485_RTS_AFTER_SEND)
 				imx_uart_rts_active(sport, &ucr2);
 			else
 				imx_uart_rts_inactive(sport, &ucr2);
@@ -490,8 +490,8 @@  static void imx_uart_stop_rx_with_loopback_ctrl(struct uart_port *port, bool loo
 	imx_uart_writel(sport, ucr4, UCR4);
 
 	/* See SER_RS485_ENABLED/UTS_LOOP comment in imx_uart_probe() */
-	if (port->rs485.flags & SER_RS485_ENABLED &&
-	    port->rs485.flags & SER_RS485_RTS_ON_SEND &&
+	if (sport->flags & SER_RS485_ENABLED &&
+	    sport->flags & SER_RS485_RTS_ON_SEND &&
 	    sport->have_rtscts && !sport->have_rtsgpio && loopback) {
 		uts = imx_uart_readl(sport, imx_uart_uts_reg(sport));
 		uts |= UTS_LOOP;
@@ -604,7 +604,7 @@  static void imx_uart_dma_tx_callback(void *data)
 	if (!kfifo_is_empty(&tport->xmit_fifo) &&
 			!uart_tx_stopped(&sport->port))
 		imx_uart_dma_tx(sport);
-	else if (sport->port.rs485.flags & SER_RS485_ENABLED) {
+	else if (sport->flags & SER_RS485_ENABLED) {
 		u32 ucr4 = imx_uart_readl(sport, UCR4);
 		ucr4 |= UCR4_TCEN;
 		imx_uart_writel(sport, ucr4, UCR4);
@@ -681,10 +681,10 @@  static void imx_uart_start_tx(struct uart_port *port)
 	 * imx_uart_stop_tx(), but tx_state is still SEND.
 	 */
 
-	if (port->rs485.flags & SER_RS485_ENABLED) {
+	if (sport->flags & SER_RS485_ENABLED) {
 		if (sport->tx_state == OFF) {
 			u32 ucr2 = imx_uart_readl(sport, UCR2);
-			if (port->rs485.flags & SER_RS485_RTS_ON_SEND)
+			if (sport->flags & SER_RS485_RTS_ON_SEND)
 				imx_uart_rts_active(sport, &ucr2);
 			else
 				imx_uart_rts_inactive(sport, &ucr2);
@@ -695,7 +695,7 @@  static void imx_uart_start_tx(struct uart_port *port)
 			 * with loopback enabled because that will make our
 			 * transmitted data being just looped to RX.
 			 */
-			if (!(port->rs485.flags & SER_RS485_RX_DURING_TX) &&
+			if (!(sport->flags & SER_RS485_RX_DURING_TX) &&
 			    !port->rs485_rx_during_tx_gpio)
 				imx_uart_stop_rx_with_loopback_ctrl(port, false);
 
@@ -1078,7 +1078,7 @@  static void imx_uart_set_mctrl(struct uart_port *port, unsigned int mctrl)
 	struct imx_port *sport = to_imx_port(port);
 	u32 ucr3, uts;
 
-	if (!(port->rs485.flags & SER_RS485_ENABLED)) {
+	if (!(sport->flags & SER_RS485_ENABLED)) {
 		u32 ucr2;
 
 		/*
@@ -1604,8 +1604,8 @@  static void imx_uart_shutdown(struct uart_port *port)
 	ucr1 &= ~(UCR1_TRDYEN | UCR1_RRDYEN | UCR1_RTSDEN | UCR1_RXDMAEN |
 		  UCR1_ATDMAEN | UCR1_SNDBRK);
 	/* See SER_RS485_ENABLED/UTS_LOOP comment in imx_uart_probe() */
-	if (port->rs485.flags & SER_RS485_ENABLED &&
-	    port->rs485.flags & SER_RS485_RTS_ON_SEND &&
+	if (sport->flags & SER_RS485_ENABLED &&
+	    sport->flags & SER_RS485_RTS_ON_SEND &&
 	    sport->have_rtscts && !sport->have_rtsgpio) {
 		uts = imx_uart_readl(sport, imx_uart_uts_reg(sport));
 		uts |= UTS_LOOP;
@@ -1643,7 +1643,7 @@  static void imx_uart_shutdown(struct uart_port *port)
 	 * those cases, we have to wait for the hrtimer to fire and
 	 * complete the transition to OFF.
 	 */
-	loops = port->rs485.flags & SER_RS485_ENABLED ?
+	loops = sport->flags & SER_RS485_ENABLED ?
 		port->rs485.delay_rts_after_send : 0;
 	while (sport->tx_state != OFF && loops--) {
 		uart_port_unlock_irqrestore(&sport->port, flags);
@@ -1659,9 +1659,9 @@  static void imx_uart_shutdown(struct uart_port *port)
 		 * signal is inactive in order not to block other
 		 * devices.
 		 */
-		if (port->rs485.flags & SER_RS485_ENABLED) {
+		if (sport->flags & SER_RS485_ENABLED) {
 			ucr2 = imx_uart_readl(sport, UCR2);
-			if (port->rs485.flags & SER_RS485_RTS_AFTER_SEND)
+			if (sport->flags & SER_RS485_RTS_AFTER_SEND)
 				imx_uart_rts_active(sport, &ucr2);
 			else
 				imx_uart_rts_inactive(sport, &ucr2);
@@ -1749,13 +1749,13 @@  imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
 	if (!sport->have_rtscts)
 		termios->c_cflag &= ~CRTSCTS;
 
-	if (port->rs485.flags & SER_RS485_ENABLED) {
+	if (sport->flags & SER_RS485_ENABLED) {
 		/*
 		 * RTS is mandatory for rs485 operation, so keep
 		 * it under manual control and keep transmitter
 		 * disabled.
 		 */
-		if (port->rs485.flags & SER_RS485_RTS_AFTER_SEND)
+		if (sport->flags & SER_RS485_RTS_AFTER_SEND)
 			imx_uart_rts_active(sport, &ucr2);
 		else
 			imx_uart_rts_inactive(sport, &ucr2);
@@ -2394,6 +2394,7 @@  static int imx_uart_probe(struct platform_device *pdev)
 	}
 
 	ret = uart_get_rs485_mode(&sport->port);
+	sport->flags = sport->port.rs485.flags;
 	if (ret)
 		goto err_clk;