Skip to contentSkip to conversation

serial: Two branches the same in timbuart_set_mctrl()

PATCH In review Roel Kluin 3 participants 7 messages Jan 17, 2010 – Mar 15, 2010 TTY layer and serial drivers

lore
Share & tools
lore

Roel Kluin

author
#0 lore raw

PATCH 1/1 serial: Two branches the same in timbuart_set_mctrl()

Regardless of the condition, the branches execute the same code

Signed-off-by Roel Kluin <[email protected]>

---
Was this maybe intended? please review.

drivers/serial/timbuart.c +1 -1
index 34b31da..c433d40 100644
@@ -219,7 +219,7 @@ static void timbuart_set_mctrl(struct uart_port *port, unsigned int mctrl)
 	if (mctrl & TIOCM_RTS)
 		iowrite8(TIMBUART_CTRL_RTS, port->membase + TIMBUART_CTRL);
 	else
-		iowrite8(TIMBUART_CTRL_RTS, port->membase + TIMBUART_CTRL);
+		iowrite8(TIMBUART_CTRL_CTS, port->membase + TIMBUART_CTRL);
 }
 
 static void timbuart_mctrl_check(struct uart_port *port, u32 isr, u32 *ier)
#1 lore raw
Quoted 5 lines · On Sun, 17 Jan 2010 16:29:57 +0100 Roel Kluin <[email protected]> wrote:

Regardless of the condition, the branches execute the same code

Signed-off-by Roel Kluin <[email protected]>

---
Was this maybe intended? please review.

I don't have data sheets and I'm very unwilling to play guess the intended behaviour in the absence of an actual bug report.

I'd suggest you contanct the original author because as you note - somthing looks odd indeed.

Alan

author
#2 lore raw

Dear Richard Röjfors,

It is possible that I found a bug in the Timberdale UART driver that was added to the linux kernel. The patch below tries to fix this, note the RTS to CTS change, could you confirm whether the patch below changes your driver correctly?

Thank you,

Roel Kluin ----------------->8----------------------------8<------------------------------- Regardless of whether the TIOCM_RTS bit was set in mctrl, the same write occurred.

Signed-off-by Roel Kluin <[email protected]>
---
 drivers/serial/timbuart.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/serial/timbuart.c +1 -1
index 34b31da..c433d40 100644
@@ -219,7 +219,7 @@ static void timbuart_set_mctrl(struct uart_port *port, unsigned int mctrl)
 	if (mctrl & TIOCM_RTS)
 		iowrite8(TIMBUART_CTRL_RTS, port->membase + TIMBUART_CTRL);
 	else
-		iowrite8(TIMBUART_CTRL_RTS, port->membase + TIMBUART_CTRL);
+		iowrite8(TIMBUART_CTRL_CTS, port->membase + TIMBUART_CTRL);
 }
 
 static void timbuart_mctrl_check(struct uart_port *port, u32 isr, u32 *ier)
#3 lore raw

Hi Roel,

Quoted 6 lines · Roel Kluin

Dear Richard Röjfors,

It is possible that I found a bug in the Timberdale UART driver that was added to the linux kernel. The patch below tries to fix this, note the RTS to CTS change, could you confirm whether the patch below changes your driver correctly?

Thanks for finding this, I have a comment on the patch below.

Quoted 18 lines

Thank you,

Roel Kluin ----------------->8----------------------------8<------------------------------- Regardless of whether the TIOCM_RTS bit was set in mctrl, the same write occurred.

Signed-off-by Roel Kluin<[email protected]>
---
  drivers/serial/timbuart.c |    2 +-
  1 files changed, 1 insertions(+), 1 deletions(-)
drivers/serial/timbuart.c +0 -0
index 34b31da..c433d40 100644

drivers/serial/timbuart.c static void timbuart_set_mctrl(struct uart_port *port, unsigned int mctrl) · line 222

Quoted 5 lines
drivers/serial/timbuart.c +1 -1
@@ -219,7 +219,7 @@ static void timbuart_set_mctrl(struct uart_port *port, unsigned int mctrl)
  	if (mctrl&  TIOCM_RTS)
  		iowrite8(TIMBUART_CTRL_RTS, port->membase + TIMBUART_CTRL);
  	else
-		iowrite8(TIMBUART_CTRL_RTS, port->membase + TIMBUART_CTRL);
+		iowrite8(TIMBUART_CTRL_CTS, port->membase + TIMBUART_CTRL);

This is not completely correct. CTS is a read only bit and we are to stop signal RTS. So the line should look like:
iowrite8(0, port->membase + TIMBUART_CTRL);

drivers/serial/timbuart.c static void timbuart_set_mctrl(struct uart_port *port, unsigned int mctrl) · line 225

Quoted 3 lines
drivers/serial/timbuart.c +0 -0
  }
 
  static void timbuart_mctrl_check(struct uart_port *port, u32 isr, u32 *ier)

--Richard
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#4 lore raw

Hi Richard,

Quoted 9 lines

drivers/serial/timbuart.c static void timbuart_set_mctrl(struct uart_port *port, unsigned int mctrl) · line 222

Quoted 5 lines

if (mctrl& TIOCM_RTS)
iowrite8(TIMBUART_CTRL_RTS, port->membase + TIMBUART_CTRL);
else

drivers/serial/timbuart.c +1 -1
-        iowrite8(TIMBUART_CTRL_RTS, port->membase + TIMBUART_CTRL);
+        iowrite8(TIMBUART_CTRL_CTS, port->membase + TIMBUART_CTRL);

This is not completely correct. CTS is a read only bit and we are to stop signal RTS. So the line should look like:
iowrite8(0, port->membase + TIMBUART_CTRL);

It appears as if this will also unset TIMBUART_CTRL_CTS (which is probably ok since it's read-only) but TIMBUART_CTRL_FLSHTX and TIMBUART_CTRL_FLSHRX as well. If undesired we need something like this:

iowrite8(ioread8(port->membase + TIMBUART_CTRL) & ~TIMBUART_CTRL_RTS,

	port->membase + TIMBUART_CTRL);

If not required I'll send the patch as you suggested.

Regards, Roel

#5 lore raw
Quoted 8 lines · Roel Kluin

Hi Richard,

Quoted 6 lines
Quoted 5 lines

if (mctrl& TIOCM_RTS)
iowrite8(TIMBUART_CTRL_RTS, port->membase + TIMBUART_CTRL);
else

drivers/serial/timbuart.c +1 -1
-        iowrite8(TIMBUART_CTRL_RTS, port->membase + TIMBUART_CTRL);
+        iowrite8(TIMBUART_CTRL_CTS, port->membase + TIMBUART_CTRL);
Quoted 7 lines
Quoted 3 lines

This is not completely correct. CTS is a read only bit and we are to stop signal RTS. So the line should look like:
iowrite8(0, port->membase + TIMBUART_CTRL);

It appears as if this will also unset TIMBUART_CTRL_CTS (which is probably ok since it's read-only) but TIMBUART_CTRL_FLSHTX and TIMBUART_CTRL_FLSHRX as well.

TIMBUART_CTRL_FLSH(RX/TX) are write only bits, writing a 1 to these will flush the corresponding FIFO. In this case we should not flush the FIFO:s, so writing 0 as I suggested is correct.

Quoted 2 lines

If not required I'll send the patch as you suggested.

Great! Thanks!

--Richard

author Acked-by Richard Röjfors
#6 lore raw

CTS is a read only bit and we are to stop signal RTS if modem line TIOCM_RTS is not set.

Thanks for suggestions by Richard Röjfors.

Acked-by Richard Röjfors <[email protected]>
Signed-off-by Roel Kluin <[email protected]>

---

drivers/serial/timbuart.c +1 -1
index 7bf1026..823b9c5 100644
@@ -219,7 +219,7 @@ static void timbuart_set_mctrl(struct uart_port *port, unsigned int mctrl)
 	if (mctrl & TIOCM_RTS)
 		iowrite8(TIMBUART_CTRL_RTS, port->membase + TIMBUART_CTRL);
 	else
-		iowrite8(TIMBUART_CTRL_RTS, port->membase + TIMBUART_CTRL);
+		iowrite8(0, port->membase + TIMBUART_CTRL);
 }
 
 static void timbuart_mctrl_check(struct uart_port *port, u32 isr, u32 *ier)

--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html