* [PATCH] serial: Two branches the same in timbuart_set_mctrl()
@ 2010-01-17 15:29 Roel Kluin
2010-01-17 18:17 ` Alan Cox
0 siblings, 1 reply; 7+ messages in thread
From: Roel Kluin @ 2010-01-17 15:29 UTC (permalink / raw)
To: alan, Andrew Morton, LKML
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)
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] serial: Two branches the same in timbuart_set_mctrl()
2010-01-17 15:29 [PATCH] serial: Two branches the same in timbuart_set_mctrl() Roel Kluin
@ 2010-01-17 18:17 ` Alan Cox
2010-01-17 19:02 ` Roel Kluin
0 siblings, 1 reply; 7+ messages in thread
From: Alan Cox @ 2010-01-17 18:17 UTC (permalink / raw)
To: Roel Kluin; +Cc: alan, Andrew Morton, LKML
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] serial: Two branches the same in timbuart_set_mctrl()
2010-01-17 18:17 ` Alan Cox
@ 2010-01-17 19:02 ` Roel Kluin
2010-03-15 9:44 ` Richard Röjfors
0 siblings, 1 reply; 7+ messages in thread
From: Roel Kluin @ 2010-01-17 19:02 UTC (permalink / raw)
To: richard.rojfors.ext; +Cc: Andrew Morton, LKML, Alan Cox
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)
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] serial: Two branches the same in timbuart_set_mctrl()
2010-01-17 19:02 ` Roel Kluin
@ 2010-03-15 9:44 ` Richard Röjfors
2010-03-15 13:33 ` Roel Kluin
0 siblings, 1 reply; 7+ messages in thread
From: Richard Röjfors @ 2010-03-15 9:44 UTC (permalink / raw)
To: Roel Kluin; +Cc: linux-serial, Andrew Morton, LKML, Alan Cox
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] serial: Two branches the same in timbuart_set_mctrl()
2010-03-15 9:44 ` Richard Röjfors
@ 2010-03-15 13:33 ` Roel Kluin
2010-03-15 13:41 ` Richard Röjfors
0 siblings, 1 reply; 7+ messages in thread
From: Roel Kluin @ 2010-03-15 13:33 UTC (permalink / raw)
To: Richard Röjfors; +Cc: linux-serial, Andrew Morton, LKML, Alan Cox
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] serial: Two branches the same in timbuart_set_mctrl()
2010-03-15 13:33 ` Roel Kluin
@ 2010-03-15 13:41 ` Richard Röjfors
2010-03-15 13:57 ` Roel Kluin
0 siblings, 1 reply; 7+ messages in thread
From: Richard Röjfors @ 2010-03-15 13:41 UTC (permalink / raw)
To: Roel Kluin; +Cc: linux-serial, Andrew Morton, LKML, Alan Cox
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] serial: Two branches the same in timbuart_set_mctrl()
2010-03-15 13:41 ` Richard Röjfors
@ 2010-03-15 13:57 ` Roel Kluin
0 siblings, 0 replies; 7+ messages in thread
From: Roel Kluin @ 2010-03-15 13:57 UTC (permalink / raw)
To: Richard Röjfors; +Cc: linux-serial, Andrew Morton, LKML, Alan Cox
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)
^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2010-03-15 13:55 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-01-17 15:29 [PATCH] serial: Two branches the same in timbuart_set_mctrl() Roel Kluin
2010-01-17 18:17 ` Alan Cox
2010-01-17 19:02 ` Roel Kluin
2010-03-15 9:44 ` Richard Röjfors
2010-03-15 13:33 ` Roel Kluin
2010-03-15 13:41 ` Richard Röjfors
2010-03-15 13:57 ` Roel Kluin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox