[PATCH] i2c-pnx: fix setting start/stop condition

[PATCH] i2c-pnx: fix setting start/stop condition

[Luotao Fu]

The start/stop condtions are set in different places repetedly in the i2c-pnx
driver.  Beside in i2c_pnx_start and i2c_pnx_stop the start/stop bit are also
set during the transfer of a i2c message in the master_xmit/rcv calls. This is
wrong since we can't set the start/stop condition during the transaction of a
single message any way. As a matter of fact, the driver will sometimes set both
the start and the stop bits at one time. This can be easily reproduced by
sending a simple read request like e.g
struct i2c_msg msgs[] = {
{ addr, 0, 1, buf },
{ addr, I2C_M_RD, offset, buf }
};
While processing the first message the i2c_pnx_master_xmit will set both the
start_bit and the stop_bit, which will eventually confuse the slave.

Fixed by remove setting start/stop condition from the transmit routines.

Signed-off-by: Luotao Fu <l.fu-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
---
 drivers/i2c/busses/i2c-pnx.c |   11 -----------
 1 files changed, 0 insertions(+), 11 deletions(-)

diff --git a/drivers/i2c/busses/i2c-pnx.c b/drivers/i2c/busses/i2c-pnx.c
index 5d1c260..92c0e46 100644
--- a/drivers/i2c/busses/i2c-pnx.c
+++ b/drivers/i2c/busses/i2c-pnx.c
@@ -175,12 +175,6 @@ static int i2c_pnx_master_xmit(struct i2c_adapter *adap)
 		/* We still have something to talk about... */
 		val = *alg_data->mif.buf++;
 
-		if (alg_data->mif.len == 1) {
-			val |= stop_bit;
-			if (!alg_data->last)
-				val |= start_bit;
-		}
-
 		alg_data->mif.len--;
 		iowrite32(val, I2C_REG_TX(alg_data));
 
@@ -252,11 +246,6 @@ static int i2c_pnx_master_rcv(struct i2c_adapter *adap)
 			"Rx-fifo...\n", __func__);
 
 		if (alg_data->mif.len == 1) {
-			/* Last byte, do not acknowledge next rcv. */
-			val |= stop_bit;
-			if (!alg_data->last)
-				val |= start_bit;
-
 			/*
 			 * Enable interrupt RFDAIE (data in Rx fifo),
 			 * and disable DRMIE (need data for Tx)
-- 
1.7.0

Re: [PATCH] i2c-pnx: fix setting start/stop condition

[Vitaly Wool]

On Mon, Mar 1, 2010 at 1:24 PM, Luotao Fu <l.fu-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> The start/stop condtions are set in different places repetedly in the i2c-pnx
> driver.  Beside in i2c_pnx_start and i2c_pnx_stop the start/stop bit are also
> set during the transfer of a i2c message in the master_xmit/rcv calls. This is
> wrong since we can't set the start/stop condition during the transaction of a
> single message any way. As a matter of fact, the driver will sometimes set both
> the start and the stop bits at one time. This can be easily reproduced by
> sending a simple read request like e.g
> struct i2c_msg msgs[] = {
> { addr, 0, 1, buf },
> { addr, I2C_M_RD, offset, buf }
> };
> While processing the first message the i2c_pnx_master_xmit will set both the
> start_bit and the stop_bit, which will eventually confuse the slave.
>
> Fixed by remove setting start/stop condition from the transmit routines.
>
> Signed-off-by: Luotao Fu <l.fu-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>

Acked-by: Vitaly Wool <vitalywool-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

RE: [PATCH] i2c-pnx: fix setting start/stop condition

[Kevin Wells]

Hi Luotao,

> 
> The start/stop condtions are set in different places repetedly in the i2c-
> pnx
> driver.  Beside in i2c_pnx_start and i2c_pnx_stop the start/stop bit are
> also
> set during the transfer of a i2c message in the master_xmit/rcv calls.
> This is
> wrong since we can't set the start/stop condition during the transaction
> of a
> single message any way. As a matter of fact, the driver will sometimes set
> both
> the start and the stop bits at one time. This can be easily reproduced by
> sending a simple read request like e.g
> struct i2c_msg msgs[] = {
> { addr, 0, 1, buf },
> { addr, I2C_M_RD, offset, buf }
> };
> While processing the first message the i2c_pnx_master_xmit will set both
> the
> start_bit and the stop_bit, which will eventually confuse the slave.
> 
> Fixed by remove setting start/stop condition from the transmit routines.
> 
> Signed-off-by: Luotao Fu <l.fu-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-pnx.c |   11 -----------
>  1 files changed, 0 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-pnx.c b/drivers/i2c/busses/i2c-pnx.c
> index 5d1c260..92c0e46 100644
> --- a/drivers/i2c/busses/i2c-pnx.c
> +++ b/drivers/i2c/busses/i2c-pnx.c
> @@ -175,12 +175,6 @@ static int i2c_pnx_master_xmit(struct i2c_adapter
> *adap)
>  		/* We still have something to talk about... */
>  		val = *alg_data->mif.buf++;
> 
> -		if (alg_data->mif.len == 1) {
> -			val |= stop_bit;
> -			if (!alg_data->last)
> -				val |= start_bit;

Removing the start bit assertion here should be ok. The initial start
condition is setup as part of the start of the transfer when the addr
is sent out. The stop bit needs to stay or the I2C transfer will not
correctly terminate on the last byte out and you will end up with a
bus busy failure (clock low, data high) on the next transfer.

The stop_bit is used to indicate the end of the transfer for the I2C
controller. If it isn't used, the clock will stay low at the end
of the transfer after ACK (thinking more data still needs to be
transferred). If stop is enabled, an extra 1/2 clock is used to
reset the clock state back to high and the cycle is terminated
(dat and clk both high).

> -		}
> -
>  		alg_data->mif.len--;
>  		iowrite32(val, I2C_REG_TX(alg_data));
> 
> @@ -252,11 +246,6 @@ static int i2c_pnx_master_rcv(struct i2c_adapter
> *adap)
>  			"Rx-fifo...\n", __func__);
> 
>  		if (alg_data->mif.len == 1) {
> -			/* Last byte, do not acknowledge next rcv. */
> -			val |= stop_bit;

Likewise to TX, the stop bit needs to stay or the I2C transfer will
not correctly terminate on the last byte in.

 -			if (!alg_data->last)
> -				val |= start_bit;
> -
>  			/*
>  			 * Enable interrupt RFDAIE (data in Rx fifo),
>  			 * and disable DRMIE (need data for Tx)
> --
> 1.7.0

thanks,
Kevin

Re: [PATCH] i2c-pnx: fix setting start/stop condition

[Luotao Fu]

[-- Attachment #1.1: Type: text/plain, Size: 2678 bytes --]

On Mon, Mar 01, 2010 at 07:43:24PM +0100, Kevin Wells wrote:
> Hi Luotao,
> 
> > 
> > The start/stop condtions are set in different places repetedly in the i2c-
> > pnx
> > driver.  Beside in i2c_pnx_start and i2c_pnx_stop the start/stop bit are
> > also
> > set during the transfer of a i2c message in the master_xmit/rcv calls.
> > This is
> > wrong since we can't set the start/stop condition during the transaction
> > of a
> > single message any way. As a matter of fact, the driver will sometimes set
> > both
> > the start and the stop bits at one time. This can be easily reproduced by
> > sending a simple read request like e.g
> > struct i2c_msg msgs[] = {
> > { addr, 0, 1, buf },
> > { addr, I2C_M_RD, offset, buf }
> > };
> > While processing the first message the i2c_pnx_master_xmit will set both
> > the
> > start_bit and the stop_bit, which will eventually confuse the slave.
> > 
> > Fixed by remove setting start/stop condition from the transmit routines.
> > 
> > Signed-off-by: Luotao Fu <l.fu-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> > ---
> >  drivers/i2c/busses/i2c-pnx.c |   11 -----------
> >  1 files changed, 0 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-pnx.c b/drivers/i2c/busses/i2c-pnx.c
> > index 5d1c260..92c0e46 100644
> > --- a/drivers/i2c/busses/i2c-pnx.c
> > +++ b/drivers/i2c/busses/i2c-pnx.c
> > @@ -175,12 +175,6 @@ static int i2c_pnx_master_xmit(struct i2c_adapter
> > *adap)
> >  		/* We still have something to talk about... */
> >  		val = *alg_data->mif.buf++;
> > 
> > -		if (alg_data->mif.len == 1) {
> > -			val |= stop_bit;
> > -			if (!alg_data->last)
> > -				val |= start_bit;
> 
> Removing the start bit assertion here should be ok. The initial start
> condition is setup as part of the start of the transfer when the addr
> is sent out. The stop bit needs to stay or the I2C transfer will not
> correctly terminate on the last byte out and you will end up with a
> bus busy failure (clock low, data high) on the next transfer.

I had a closer look into this and think that you are right. I was
irritated by the unregular usage of start_bit and thought the stop bit
is asserted with the i2c_pnx_stop call, which is apparently only used
for zero sized transfers.

Fixed patch is attached with this mail.

cheers
Luotao Fu
-- 
Pengutronix e.K.                           | Dipl.-Ing. Luotao Fu        |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #1.2: 0001-i2c-pnx-fix-setting-start-condition.patch --]
[-- Type: text/x-diff, Size: 2215 bytes --]

From 4a142700fec80489e3aa38b64d0b519d3968b0d9 Mon Sep 17 00:00:00 2001
From: Luotao Fu <l.fu-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Date: Mon, 1 Mar 2010 13:00:20 +0100
Subject: [PATCH] i2c-pnx: fix setting start condition

The start condtions is set in different places repeatedly in the i2c-pnx driver.
Beside in i2c_pnx_start the start bit is also set during the transfer of a i2c
message in the master_xmit/rcv calls. This is wrong since we can't set the start
condition during the transaction of a single message any way. As a matter of
fact, the driver will sometimes set both the start and the stop bits at one
time. This can be easily reproduced by sending a simple read request like e.g
struct i2c_msg msgs[] = {
{ addr, 0, 1, buf },
{ addr, I2C_M_RD, offset, buf }
};
While processing the first message the i2c_pnx_master_xmit will set both the
start_bit and the stop_bit, which will eventually confuse the slave.

Fixed by remove setting start condition from the transmit routines.

Signed-off-by: Luotao Fu <l.fu-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Acked-by: Vitaly Wool <vitalywool-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/i2c/busses/i2c-pnx.c |    9 ++-------
 1 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/busses/i2c-pnx.c b/drivers/i2c/busses/i2c-pnx.c
index 5d1c260..9edbe98 100644
--- a/drivers/i2c/busses/i2c-pnx.c
+++ b/drivers/i2c/busses/i2c-pnx.c
@@ -175,11 +175,9 @@ static int i2c_pnx_master_xmit(struct i2c_adapter *adap)
 		/* We still have something to talk about... */
 		val = *alg_data->mif.buf++;
 
-		if (alg_data->mif.len == 1) {
+		/* last byte of a message */
+		if (alg_data->mif.len == 1)
 			val |= stop_bit;
-			if (!alg_data->last)
-				val |= start_bit;
-		}
 
 		alg_data->mif.len--;
 		iowrite32(val, I2C_REG_TX(alg_data));
@@ -254,9 +252,6 @@ static int i2c_pnx_master_rcv(struct i2c_adapter *adap)
 		if (alg_data->mif.len == 1) {
 			/* Last byte, do not acknowledge next rcv. */
 			val |= stop_bit;
-			if (!alg_data->last)
-				val |= start_bit;
-
 			/*
 			 * Enable interrupt RFDAIE (data in Rx fifo),
 			 * and disable DRMIE (need data for Tx)
-- 
1.7.0


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

RE: [PATCH] i2c-pnx: fix setting start/stop condition

[Kevin Wells]

Hi Luotao,

> 
> I had a closer look into this and think that you are right. I was
> irritated by the unregular usage of start_bit and thought the stop bit is
> asserted with the i2c_pnx_stop call, which is apparently only used for
> zero sized transfers.
> 
> Fixed patch is attached with this mail.
> 

This latest patch seems to work fine and tests well with all our
I2C devices - including the pcf8563, which didn't work right before
this patch. I think it's good to go.

thanks!
Kevin

> cheers
> Luotao Fu
> --
> Pengutronix e.K.                           | Dipl.-Ing. Luotao Fu        |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |