i2c-pnx driver issues

i2c-pnx driver issues

[Kevin Wells]

Sorry - I've resent with a more meaningful subject (than 'welcome')!

Hi, 

We use the i2c-pnx.c driver on our device (NXP LPC3xxx devices), but have had some
issues with compilation and general use. I hope these changes can be of use and can
get back into the mainline. I'm happy to test any changes for the driver here on
our boards.

The i2c-pnx.c file doesn't seem to compile (even with the pnx4008 platform selected).
It looks like several important header files are not included in the driver
(mach/i2c.h and asm/io.h). We have another board that uses this same IP, but with a
slight reordering of the registers, so the i2c.h file in the mach area (which defines
register offsets) is important where it is now.

For systems with a tick rate of 100Hz, the I2C_PNX_TIMEOUT value of 10mS give only
1 jiffie before the transfer times out. We've noticed on some transfers that the
jiffie may tick immediately after the expire count is set, causing the transfer in
progress to timeout before 10mS is up. A small test to make sure jiffies was at
least 2 fixed this.

We have multiple I2C busses on our system. The bus id value in the platform definition
area (in the arm/mach- area) wasn't being correctly matched to the specific I2C
bus. This would cause some devices to not match to the correct I2C bus.

The 'buf' field in the I2C transfer descriptor was typed as a char. In the I2C
driver, data values in the buffer pointer to by buf were being sign extended into
the stop and start bit positions (8 and 9). For I2C transfers where a data byte had
bit 7 set, both the start and stop bits were also getting set in the hardware.

The complete patch is listed below:

diff -Naur -X linux-2.6.32-rc6-ref/Documentation/dontdiff linux-2.6.32-rc6-ref/drivers/i2c/busses/i2c-pnx.c linux-2.6.32-rc6/drivers/i2c/busses/i2c-pnx.c
--- linux-2.6.32-rc6-ref/drivers/i2c/busses/i2c-pnx.c	2009-11-03 11:37:49.000000000 -0800
+++ linux-2.6.32-rc6/drivers/i2c/busses/i2c-pnx.c	2009-11-10 14:09:11.000000000 -0800
@@ -20,8 +20,10 @@
 #include <linux/platform_device.h>
 #include <linux/i2c-pnx.h>
 #include <mach/hardware.h>
+#include <mach/i2c.h>
 #include <asm/irq.h>
 #include <asm/uaccess.h>
+#include <asm/io.h>
 
 #define I2C_PNX_TIMEOUT		10 /* msec */
 #define I2C_PNX_SPEED_KHZ	100
@@ -54,6 +56,9 @@
 	struct timer_list *timer = &data->mif.timer;
 	int expires = I2C_PNX_TIMEOUT / (1000 / HZ);
 
+	if (expires <= 1)
+		expires = 2;
+
 	del_timer_sync(timer);
 
 	dev_dbg(&adap->dev, "Timer armed at %lu plus %u jiffies.\n",
@@ -633,7 +638,8 @@
 
 	/* Register this adapter with the I2C subsystem */
 	i2c_pnx->adapter->dev.parent = &pdev->dev;
-	ret = i2c_add_adapter(i2c_pnx->adapter);
+	i2c_pnx->adapter->nr = pdev->id;
+	ret = i2c_add_numbered_adapter(i2c_pnx->adapter);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "I2C: Failed to add bus\n");
 		goto out_irq;
diff -Naur -X linux-2.6.32-rc6-ref/Documentation/dontdiff linux-2.6.32-rc6-ref/include/linux/i2c-pnx.h linux-2.6.32-rc6/include/linux/i2c-pnx.h
--- linux-2.6.32-rc6-ref/include/linux/i2c-pnx.h	2009-11-03 11:37:49.000000000 -0800
+++ linux-2.6.32-rc6/include/linux/i2c-pnx.h	2009-11-10 14:16:55.000000000 -0800
@@ -21,7 +21,7 @@
 	int			mode;		/* Interface mode */
 	struct completion	complete;	/* I/O completion */
 	struct timer_list	timer;		/* Timeout */
-	char *			buf;		/* Data buffer */
+	u8 *			buf;		/* Data buffer */
 	int			len;		/* Length of data buffer */
 };

thanks,
Kevin Wells
NXP Semiconductors

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: i2c-pnx driver issues

[Ben Dooks]

On Wed, Nov 11, 2009 at 01:21:45AM +0100, Kevin Wells wrote:
> Sorry - I've resent with a more meaningful subject (than 'welcome')!
> 
> Hi, 

gah, please word-wrap your lines to less than 77 characters per line,
it makes it awfully difficult to read them on mailers in terminals.
 
> We use the i2c-pnx.c driver on our device (NXP LPC3xxx devices), but have had some
> issues with compilation and general use. I hope these changes can be of use and can
> get back into the mainline. I'm happy to test any changes for the driver here on
> our boards.
> 
> The i2c-pnx.c file doesn't seem to compile (even with the pnx4008 platform selected).
> It looks like several important header files are not included in the driver
> (mach/i2c.h and asm/io.h). We have another board that uses this same IP, but with a
> slight reordering of the registers, so the i2c.h file in the mach area (which defines
> register offsets) is important where it is now.

asm/io.h should not be included directly, <linux/io.h> is the proper
include file.
 
> For systems with a tick rate of 100Hz, the I2C_PNX_TIMEOUT value of 10mS give only
> 1 jiffie before the transfer times out. We've noticed on some transfers that the
> jiffie may tick immediately after the expire count is set, causing the transfer in
> progress to timeout before 10mS is up. A small test to make sure jiffies was at
> least 2 fixed this.
> 
> We have multiple I2C busses on our system. The bus id value in the platform definition
> area (in the arm/mach- area) wasn't being correctly matched to the specific I2C
> bus. This would cause some devices to not match to the correct I2C bus.

That should be a ok to do as long as you are the primary i2c block
on the systtem.
 
> The 'buf' field in the I2C transfer descriptor was typed as a char. In the I2C
> driver, data values in the buffer pointer to by buf were being sign extended into
> the stop and start bit positions (8 and 9). For I2C transfers where a data byte had
> bit 7 set, both the start and stop bits were also getting set in the hardware.
> 
> The complete patch is listed below:

any chance of having this patch in a form that could be easily
merged? If you need more information on this, read the kernel
documentation on the subject in Documentation/SubmittingPatches on how
to format patches.

even better, split the changes into individual bugfixes.
 
> diff -Naur -X linux-2.6.32-rc6-ref/Documentation/dontdiff linux-2.6.32-rc6-ref/drivers/i2c/busses/i2c-pnx.c linux-2.6.32-rc6/drivers/i2c/busses/i2c-pnx.c
> --- linux-2.6.32-rc6-ref/drivers/i2c/busses/i2c-pnx.c	2009-11-03 11:37:49.000000000 -0800
> +++ linux-2.6.32-rc6/drivers/i2c/busses/i2c-pnx.c	2009-11-10 14:09:11.000000000 -0800
> @@ -20,8 +20,10 @@
>  #include <linux/platform_device.h>
>  #include <linux/i2c-pnx.h>
>  #include <mach/hardware.h>
> +#include <mach/i2c.h>
>  #include <asm/irq.h>
>  #include <asm/uaccess.h>
> +#include <asm/io.h>
>  
>  #define I2C_PNX_TIMEOUT		10 /* msec */
>  #define I2C_PNX_SPEED_KHZ	100
> @@ -54,6 +56,9 @@
>  	struct timer_list *timer = &data->mif.timer;
>  	int expires = I2C_PNX_TIMEOUT / (1000 / HZ);
>  
> +	if (expires <= 1)
> +		expires = 2;
> +
>  	del_timer_sync(timer);
>  
>  	dev_dbg(&adap->dev, "Timer armed at %lu plus %u jiffies.\n",

adding -p to the diff options makes life easier to work out what
is being changed.

> @@ -633,7 +638,8 @@
>  
>  	/* Register this adapter with the I2C subsystem */
>  	i2c_pnx->adapter->dev.parent = &pdev->dev;
> -	ret = i2c_add_adapter(i2c_pnx->adapter);
> +	i2c_pnx->adapter->nr = pdev->id;
> +	ret = i2c_add_numbered_adapter(i2c_pnx->adapter);
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "I2C: Failed to add bus\n");
>  		goto out_irq;
> diff -Naur -X linux-2.6.32-rc6-ref/Documentation/dontdiff linux-2.6.32-rc6-ref/include/linux/i2c-pnx.h linux-2.6.32-rc6/include/linux/i2c-pnx.h
> --- linux-2.6.32-rc6-ref/include/linux/i2c-pnx.h	2009-11-03 11:37:49.000000000 -0800
> +++ linux-2.6.32-rc6/include/linux/i2c-pnx.h	2009-11-10 14:16:55.000000000 -0800
> @@ -21,7 +21,7 @@
>  	int			mode;		/* Interface mode */
>  	struct completion	complete;	/* I/O completion */
>  	struct timer_list	timer;		/* Timeout */
> -	char *			buf;		/* Data buffer */
> +	u8 *			buf;		/* Data buffer */
>  	int			len;		/* Length of data buffer */
>  };
> 
> thanks,
> Kevin Wells
> NXP Semiconductors

-- 
Ben (ben-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'

[PATCH 1/4] i2c : Made buf type unsigned to prevent sign extension

[Kevin Wells]

Made buf type unsigned to prevent sign extension

Signed-off-by: Kevin Wells <kevin.wells-3arQi8VN3Tc@public.gmane.org>
---
i2c-pnx.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- include/linux/i2c-pnx.h.orig	2009-11-11 14:47:28.000000000 -0800
+++ include/linux/i2c-pnx.h	2009-11-10 14:16:55.000000000 -0800
@@ -21,7 +21,7 @@ struct i2c_pnx_mif {
 	int			mode;		/* Interface mode */
 	struct completion	complete;	/* I/O completion */
 	struct timer_list	timer;		/* Timeout */
-	char *			buf;		/* Data buffer */
+	u8 *			buf;		/* Data buffer */
 	int			len;		/* Length of data buffer */
 };

[PATCH 2/4] i2c : Added missing mach/i2c.h and linux/io.h header file includes

[Kevin Wells]

Added missing mach/i2c.h and linux/io.h header file includes

Signed-off-by: Kevin Wells <kevin.wells-3arQi8VN3Tc@public.gmane.org>
---
i2c-pnx.c |    2 ++
 1 file changed, 2 insertions(+)

--- drivers/i2c/busses/i2c-pnx.c.orig	2009-11-11 14:54:30.000000000 -0800
+++ drivers/i2c/busses/i2c-pnx.c	2009-11-11 14:55:14.000000000 -0800
@@ -19,7 +19,9 @@
 #include <linux/completion.h>
 #include <linux/platform_device.h>
 #include <linux/i2c-pnx.h>
+#include <linux/io.h>
 #include <mach/hardware.h>
+#include <mach/i2c.h>
 #include <asm/irq.h>
 #include <asm/uaccess.h>

[PATCH 3/4] i2c : Limit minimum jiffie timeout to 2

[Kevin Wells]

Limit minimum jiffie timeout to 2 to prevent early timeout on systems
with low tick rates

Signed-off-by: Kevin Wells <kevin.wells-3arQi8VN3Tc@public.gmane.org>
---
i2c-pnx.c |    3 +++
 1 file changed, 3 insertions(+)

--- drivers/i2c/busses/i2c-pnx.c.orig	2009-11-11 14:54:30.000000000 -0800
+++ drivers/i2c/busses/i2c-pnx.c	2009-11-11 15:00:17.000000000 -0800
@@ -54,6 +54,9 @@ static inline void i2c_pnx_arm_timer(str
 	struct timer_list *timer = &data->mif.timer;
 	int expires = I2C_PNX_TIMEOUT / (1000 / HZ);
 
+	if (expires <= 1)
+		expires = 2;
+
 	del_timer_sync(timer);
 
 	dev_dbg(&adap->dev, "Timer armed at %lu plus %u jiffies.\n",

[PATCH 4/4] i2c : Map I2C adapter number to platform ID number

[Kevin Wells]

Map I2C adapter number to platform ID number

Signed-off-by: Kevin Wells <kevin.wells-3arQi8VN3Tc@public.gmane.org>
---
i2c-pnx.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

--- drivers/i2c/busses/i2c-pnx.c.orig	2009-11-11 14:54:30.000000000 -0800
+++ drivers/i2c/busses/i2c-pnx.c	2009-11-11 15:02:30.000000000 -0800
@@ -633,7 +633,8 @@ static int __devinit i2c_pnx_probe(struc
 
 	/* Register this adapter with the I2C subsystem */
 	i2c_pnx->adapter->dev.parent = &pdev->dev;
-	ret = i2c_add_adapter(i2c_pnx->adapter);
+	i2c_pnx->adapter->nr = pdev->id;
+	ret = i2c_add_numbered_adapter(i2c_pnx->adapter);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "I2C: Failed to add bus\n");
 		goto out_irq;

Re: [PATCH 4/4] i2c : Map I2C adapter number to platform ID number

[Ben Dooks]

On Thu, Nov 12, 2009 at 12:34:17AM +0100, Kevin Wells wrote:
> Map I2C adapter number to platform ID number
> 
> Signed-off-by: Kevin Wells <kevin.wells-3arQi8VN3Tc@public.gmane.org>

I've queued this onto i2c-next-misc ready for the next kernel merge
window, as it isn't really a bugfix. The rest are on i2c-fixes-pnx
and will pushed in a few days time.

-- 
Ben (ben-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'

RE: i2c-pnx driver issues

[Kevin Wells]

Thanks Ben,

I've submitted the changes as 4 individual patches. We also have some
other enhancements for this driver coming up soon, but these 4 patches
fix critical issues in the current pnx driver.

thanks,
Kevin Wells
NXP Semiconductors

> -----Original Message-----
> From: Ben Dooks [mailto:ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org]
> Sent: Wednesday, November 11, 2009 1:22 PM
> To: Kevin Wells
> Cc: vitalywool-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org; linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: i2c-pnx driver issues
> 
> On Wed, Nov 11, 2009 at 01:21:45AM +0100, Kevin Wells wrote:
> > Sorry - I've resent with a more meaningful subject (than 'welcome')!
> >
> > Hi,
> 
> gah, please word-wrap your lines to less than 77 characters per line,
> it makes it awfully difficult to read them on mailers in terminals.
> 
> > We use the i2c-pnx.c driver on our device (NXP LPC3xxx devices), but have
> had some
> > issues with compilation and general use. I hope these changes can be of
> use and can
> > get back into the mainline. I'm happy to test any changes for the driver
> here on
> > our boards.
> >
> > The i2c-pnx.c file doesn't seem to compile (even with the pnx4008
> platform selected).
> > It looks like several important header files are not included in the
> driver
> > (mach/i2c.h and asm/io.h). We have another board that uses this same IP,
> but with a
> > slight reordering of the registers, so the i2c.h file in the mach area
> (which defines
> > register offsets) is important where it is now.
> 
> asm/io.h should not be included directly, <linux/io.h> is the proper
> include file.
> 
> > For systems with a tick rate of 100Hz, the I2C_PNX_TIMEOUT value of 10mS
> give only
> > 1 jiffie before the transfer times out. We've noticed on some transfers
> that the
> > jiffie may tick immediately after the expire count is set, causing the
> transfer in
> > progress to timeout before 10mS is up. A small test to make sure jiffies
> was at
> > least 2 fixed this.
> >
> > We have multiple I2C busses on our system. The bus id value in the
> platform definition
> > area (in the arm/mach- area) wasn't being correctly matched to the
> specific I2C
> > bus. This would cause some devices to not match to the correct I2C bus.
> 
> That should be a ok to do as long as you are the primary i2c block
> on the systtem.
> 
> > The 'buf' field in the I2C transfer descriptor was typed as a char. In
> the I2C
> > driver, data values in the buffer pointer to by buf were being sign
> extended into
> > the stop and start bit positions (8 and 9). For I2C transfers where a
> data byte had
> > bit 7 set, both the start and stop bits were also getting set in the
> hardware.
> >
> > The complete patch is listed below:
> 
> any chance of having this patch in a form that could be easily
> merged? If you need more information on this, read the kernel
> documentation on the subject in Documentation/SubmittingPatches on how
> to format patches.
> 
> even better, split the changes into individual bugfixes.
> 
> > diff -Naur -X linux-2.6.32-rc6-ref/Documentation/dontdiff linux-2.6.32-
> rc6-ref/drivers/i2c/busses/i2c-pnx.c linux-2.6.32-
> rc6/drivers/i2c/busses/i2c-pnx.c
> > --- linux-2.6.32-rc6-ref/drivers/i2c/busses/i2c-pnx.c	2009-11-03
> 11:37:49.000000000 -0800
> > +++ linux-2.6.32-rc6/drivers/i2c/busses/i2c-pnx.c	2009-11-10
> 14:09:11.000000000 -0800
> > @@ -20,8 +20,10 @@
> >  #include <linux/platform_device.h>
> >  #include <linux/i2c-pnx.h>
> >  #include <mach/hardware.h>
> > +#include <mach/i2c.h>
> >  #include <asm/irq.h>
> >  #include <asm/uaccess.h>
> > +#include <asm/io.h>
> >
> >  #define I2C_PNX_TIMEOUT		10 /* msec */
> >  #define I2C_PNX_SPEED_KHZ	100
> > @@ -54,6 +56,9 @@
> >  	struct timer_list *timer = &data->mif.timer;
> >  	int expires = I2C_PNX_TIMEOUT / (1000 / HZ);
> >
> > +	if (expires <= 1)
> > +		expires = 2;
> > +
> >  	del_timer_sync(timer);
> >
> >  	dev_dbg(&adap->dev, "Timer armed at %lu plus %u jiffies.\n",
> 
> adding -p to the diff options makes life easier to work out what
> is being changed.
> 
> > @@ -633,7 +638,8 @@
> >
> >  	/* Register this adapter with the I2C subsystem */
> >  	i2c_pnx->adapter->dev.parent = &pdev->dev;
> > -	ret = i2c_add_adapter(i2c_pnx->adapter);
> > +	i2c_pnx->adapter->nr = pdev->id;
> > +	ret = i2c_add_numbered_adapter(i2c_pnx->adapter);
> >  	if (ret < 0) {
> >  		dev_err(&pdev->dev, "I2C: Failed to add bus\n");
> >  		goto out_irq;
> > diff -Naur -X linux-2.6.32-rc6-ref/Documentation/dontdiff linux-2.6.32-
> rc6-ref/include/linux/i2c-pnx.h linux-2.6.32-rc6/include/linux/i2c-pnx.h
> > --- linux-2.6.32-rc6-ref/include/linux/i2c-pnx.h	2009-11-03
> 11:37:49.000000000 -0800
> > +++ linux-2.6.32-rc6/include/linux/i2c-pnx.h	2009-11-10
> 14:16:55.000000000 -0800
> > @@ -21,7 +21,7 @@
> >  	int			mode;		/* Interface mode */
> >  	struct completion	complete;	/* I/O completion */
> >  	struct timer_list	timer;		/* Timeout */
> > -	char *			buf;		/* Data buffer */
> > +	u8 *			buf;		/* Data buffer */
> >  	int			len;		/* Length of data buffer */
> >  };
> >
> > thanks,
> > Kevin Wells
> > NXP Semiconductors
> 
> --
> Ben (ben-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org, http://www.fluff.org/)
> 
>   'a smiley only costs 4 bytes'

Re: i2c-pnx driver issues

[Ben Dooks]

On Thu, Nov 12, 2009 at 12:37:36AM +0100, Kevin Wells wrote:
> 
> Thanks Ben,
> 
> I've submitted the changes as 4 individual patches. We also have some
> other enhancements for this driver coming up soon, but these 4 patches
> fix critical issues in the current pnx driver.

ok, will look at applying them and pushing before the weekend

-- 
Ben (ben-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'