[PATCH] serial: Two branches the same in timbuart_set_mctrl()

[PATCH] serial: Two branches the same in timbuart_set_mctrl()

[Roel Kluin]

Regardless of the condition, the branches execute the same code

Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---
Was this maybe intended? please review.

diff --git a/drivers/serial/timbuart.c b/drivers/serial/timbuart.c
index 34b31da..c433d40 100644
--- a/drivers/serial/timbuart.c
+++ b/drivers/serial/timbuart.c
@@ -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)

Re: [PATCH] serial: Two branches the same in timbuart_set_mctrl()

[Alan Cox]

On Sun, 17 Jan 2010 16:29:57 +0100
Roel Kluin <roel.kluin@gmail.com> wrote:

> Regardless of the condition, the branches execute the same code
> 
> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> ---
> 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

Re: [PATCH] serial: Two branches the same in timbuart_set_mctrl()

[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?

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 <roel.kluin@gmail.com>
---
 drivers/serial/timbuart.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/serial/timbuart.c b/drivers/serial/timbuart.c
index 34b31da..c433d40 100644
--- a/drivers/serial/timbuart.c
+++ b/drivers/serial/timbuart.c
@@ -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)

Re: [PATCH] serial: Two branches the same in timbuart_set_mctrl()

[Richard Röjfors]

Hi Roel,

On 01/17/2010 08:02 PM, Roel Kluin wrote:
> 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.

>
> 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<roel.kluin@gmail.com>
> ---
>   drivers/serial/timbuart.c |    2 +-
>   1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/serial/timbuart.c b/drivers/serial/timbuart.c
> index 34b31da..c433d40 100644
> --- a/drivers/serial/timbuart.c
> +++ b/drivers/serial/timbuart.c
> @@ -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);


>   }
>
>   static void timbuart_mctrl_check(struct uart_port *port, u32 isr, u32 *ier)

--Richard

Re: [PATCH] serial: Two branches the same in timbuart_set_mctrl()

[Roel Kluin]

Hi Richard, 

>>       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);

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

Re: [PATCH] serial: Two branches the same in timbuart_set_mctrl()

[Richard Röjfors]

On 03/15/2010 02:33 PM, Roel Kluin wrote:
> Hi Richard,
>
>>>        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);
>
> 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.

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

Great! Thanks!

--Richard

Re: [PATCH] serial: Two branches the same in timbuart_set_mctrl()

[Roel Kluin]

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 <richard.rojfors@pelagicore.com>
Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---
diff --git a/drivers/serial/timbuart.c b/drivers/serial/timbuart.c
index 7bf1026..823b9c5 100644
--- a/drivers/serial/timbuart.c
+++ b/drivers/serial/timbuart.c
@@ -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)