[patch 0/8] I2C fixes and -next from Ben Dooks

[patch 0/8] I2C fixes and -next from Ben Dooks

[Ben Dooks]

This is a set of my own fixes and development
work which includes fixes for the architecture
I maintain, and items that will be merged at
the next merge window.

-- 
Ben

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

[patch 1/8] I2C: S3C2410: Check ACK on byte transmission

[Ben Dooks]

[-- Attachment #1: simtec/simtec-drivers-i2c-s3c2410-fixack.patch --]
[-- Type: text/plain, Size: 1918 bytes --]

We should check for the reception of an ACK after transmissint each
data byte. The address send has been correctly checking this, but the
data write byte state should have also been checking for these failures.

As part of the same fix, we remove the ACK checking from the receive
path where it should not have been checking for an ACK which our hardware
was sending.

Signed-off-by: Ben Dooks <ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org>

Index: linux-2.6.26-rc4-quilt1/drivers/i2c/busses/i2c-s3c2410.c
===================================================================
--- linux-2.6.26-rc4-quilt1.orig/drivers/i2c/busses/i2c-s3c2410.c	2008-05-28 11:49:18.000000000 +0100
+++ linux-2.6.26-rc4-quilt1/drivers/i2c/busses/i2c-s3c2410.c	2008-05-28 11:56:14.000000000 +0100
@@ -324,6 +324,15 @@ static int i2s_s3c_irq_nextbyte(struct s
 		 */
 
 	retry_write:
+		if (!(i2c->msg->flags & I2C_M_IGNORE_NAK)) {
+			if (iicstat & S3C2410_IICSTAT_LASTBIT) {
+				dev_dbg(i2c->dev, "WRITE: No Ack\n");
+
+				s3c24xx_i2c_stop(i2c, -ECONNREFUSED);
+				goto out_ack;
+			}
+		}
+
 		if (!is_msgend(i2c)) {
 			byte = i2c->msg->buf[i2c->msg_ptr++];
 			writeb(byte, i2c->regs + S3C2410_IICDS);
@@ -377,17 +386,6 @@ static int i2s_s3c_irq_nextbyte(struct s
 		 * going to do any more read/write
 		 */
 
-		if (!(i2c->msg->flags & I2C_M_IGNORE_NAK) &&
-		    !(is_msglast(i2c) && is_lastmsg(i2c))) {
-
-			if (iicstat & S3C2410_IICSTAT_LASTBIT) {
-				dev_dbg(i2c->dev, "READ: No Ack\n");
-
-				s3c24xx_i2c_stop(i2c, -ECONNREFUSED);
-				goto out_ack;
-			}
-		}
-
 		byte = readb(i2c->regs + S3C2410_IICDS);
 		i2c->msg->buf[i2c->msg_ptr++] = byte;
 

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

  'a smiley only costs 4 bytes'

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

[patch 2/8] I2C: S3C2410: Add MODULE_ALIAS() for s3c2440 device.

[Ben Dooks]

[-- Attachment #1: simtec/simtec-drivers-i2c-s3c2410-fixmodalias.patch --]
[-- Type: text/plain, Size: 1057 bytes --]

Add a MODULE_ALIAS() statement for the i2c-s3c2410 controller
to ensure that it can be autoloaded on the S3C2440 systems that
we support.

Signed-off-by: Ben Dooks <ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org>

Index: linux-2.6.26-rc4-quilt1/drivers/i2c/busses/i2c-s3c2410.c
===================================================================
--- linux-2.6.26-rc4-quilt1.orig/drivers/i2c/busses/i2c-s3c2410.c	2008-05-28 11:56:54.000000000 +0100
+++ linux-2.6.26-rc4-quilt1/drivers/i2c/busses/i2c-s3c2410.c	2008-05-28 11:57:12.000000000 +0100
@@ -947,3 +947,4 @@ MODULE_DESCRIPTION("S3C24XX I2C Bus driv
 MODULE_AUTHOR("Ben Dooks, <ben-Y5A6D6n0/KfQXOPxS62xeg@public.gmane.org>");
 MODULE_LICENSE("GPL");
 MODULE_ALIAS("platform:s3c2410-i2c");
+MODULE_ALIAS("platform:s3c2440-i2c");

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

  'a smiley only costs 4 bytes'

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

[patch 3/8] I2C: S3C2410: Pass the I2C bus number via drivers platform data

[Ben Dooks]

[-- Attachment #1: simtec/simtec-drivers-i2c-s3c2410-busnum.patch --]
[-- Type: text/plain, Size: 2862 bytes --]

Allow the platform data to specify the bus bumber that the
new I2C bus will be given. This is to allow the use of the
board registration mechanism to specify the new style of
I2C device registration which allows boards to provide a
list of attached devices.

Note, as discussed on the mailing list, we have dropped
backwards compatibility of adding an dynamic bus number
as it should not affect most boards to have the bus pinned
to 0 if they have either not specified platform data for
driver. Any board supplying platform data will automatically
have the bus_num field set to 0, and anyone who needs the
driver on a different bus number can supply platform data
to set bus_num.

Signed-off-by: Ben Dooks <ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org>

Index: linux-2.6.26-rc4-quilt2/drivers/i2c/busses/i2c-s3c2410.c
===================================================================
--- linux-2.6.26-rc4-quilt2.orig/drivers/i2c/busses/i2c-s3c2410.c	2008-05-28 12:25:20.000000000 +0100
+++ linux-2.6.26-rc4-quilt2/drivers/i2c/busses/i2c-s3c2410.c	2008-05-28 12:27:38.000000000 +0100
@@ -751,9 +751,12 @@ static int s3c24xx_i2c_init(struct s3c24
 static int s3c24xx_i2c_probe(struct platform_device *pdev)
 {
 	struct s3c24xx_i2c *i2c = &s3c24xx_i2c;
+	struct s3c2410_platform_i2c *pdata;
 	struct resource *res;
 	int ret;
 
+	pdata = s3c24xx_i2c_get_platformdata(&pdev->dev);
+
 	/* find the clock and enable it */
 
 	i2c->dev = &pdev->dev;
@@ -831,7 +834,15 @@ static int s3c24xx_i2c_probe(struct plat
 	dev_dbg(&pdev->dev, "irq resource %p (%lu)\n", res,
 		(unsigned long)res->start);
 
-	ret = i2c_add_adapter(&i2c->adap);
+	/* Note, previous versions of the driver used i2c_add_adapter()
+	 * to add an bus at any number. We now pass the bus number via
+	 * the platform data, so if unset it will now default to always
+	 * being bus 0.
+	 */
+
+	i2c->adap.nr = pdata->bus_num;
+	ret = i2c_add_numbered_adapter(&i2c->adap);
+
 	if (ret < 0) {
 		dev_err(&pdev->dev, "failed to add bus to i2c core\n");
 		goto err_irq;
Index: linux-2.6.26-rc4-quilt2/include/asm-arm/plat-s3c/iic.h
===================================================================
--- linux-2.6.26-rc4-quilt2.orig/include/asm-arm/plat-s3c/iic.h	2008-05-28 11:49:18.000000000 +0100
+++ linux-2.6.26-rc4-quilt2/include/asm-arm/plat-s3c/iic.h	2008-05-28 12:28:00.000000000 +0100
@@ -21,6 +21,7 @@
 */
 
 struct s3c2410_platform_i2c {
+	int		bus_num;	/* bus number to use */
 	unsigned int	flags;
 	unsigned int	slave_addr;	/* slave address for controller */
 	unsigned long	bus_freq;	/* standard bus frequency */

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

  'a smiley only costs 4 bytes'

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

[patch 4/8] I2C: S3C2410: Add a option of reducing the bus busy waiting time.

[Ben Dooks]

[-- Attachment #1: simtec/simtec-drivers-i2c-s3c2410-singlemaster.patch --]
[-- Type: text/plain, Size: 2011 bytes --]

Add a single-master option to drasitcally reduce the time spent
waiting to check the bus is free and ready for another transfer.

Signed-off-by: Ben Dooks <ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org>

Index: linux-2.6.26-rc4-quilt2/drivers/i2c/busses/i2c-s3c2410.c
===================================================================
--- linux-2.6.26-rc4-quilt2.orig/drivers/i2c/busses/i2c-s3c2410.c	2008-05-28 12:27:38.000000000 +0100
+++ linux-2.6.26-rc4-quilt2/drivers/i2c/busses/i2c-s3c2410.c	2008-05-28 12:32:10.000000000 +0100
@@ -472,8 +472,15 @@ static irqreturn_t s3c24xx_i2c_irq(int i
 
 static int s3c24xx_i2c_set_master(struct s3c24xx_i2c *i2c)
 {
+	struct s3c2410_platform_i2c *pdata;
 	unsigned long iicstat;
-	int timeout = 400;
+	int timeout;
+
+	/* if we are a single master on this bus, reduce the delay awaiting
+	 * for bus traffic to a much lower value. */
+
+	pdata = s3c24xx_i2c_get_platformdata(i2c->dev);
+	timeout = (pdata->flags & S3C_IICFLG_SINGLE_MASTER) ? 10 : 400;
 
 	while (timeout-- > 0) {
 		iicstat = readl(i2c->regs + S3C2410_IICSTAT);
Index: linux-2.6.26-rc4-quilt2/include/asm-arm/plat-s3c/iic.h
===================================================================
--- linux-2.6.26-rc4-quilt2.orig/include/asm-arm/plat-s3c/iic.h	2008-05-28 12:28:00.000000000 +0100
+++ linux-2.6.26-rc4-quilt2/include/asm-arm/plat-s3c/iic.h	2008-05-28 12:32:10.000000000 +0100
@@ -13,7 +13,8 @@
 #ifndef __ASM_ARCH_IIC_H
 #define __ASM_ARCH_IIC_H __FILE__
 
-#define S3C_IICFLG_FILTER	(1<<0)	/* enable s3c2440 filter */
+#define S3C_IICFLG_FILTER	 (1<<0)	/* enable s3c2440 filter */
+#define S3C_IICFLG_SINGLE_MASTER (1<<1) /* s3c24xx is only bus master. */
 
 /* Notes:
  *	1) All frequencies are expressed in Hz

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

  'a smiley only costs 4 bytes'

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

[patch 5/8] OSIRIS: Add i2c device list to Simtec Osiris

[Ben Dooks]

[-- Attachment #1: simtec/simtec-mach-osiris-i2c-boardinfo.patch --]
[-- Type: text/plain, Size: 1790 bytes --]

Add an i2c board information initialisers to the board
to define which devices are present.

Signed-off-by: Ben Dooks <ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org>

--- linux-2.6.26-rc4-quilt1.orig/arch/arm/mach-s3c2440/mach-osiris.c	2008-05-27 15:14:42.000000000 +0100
+++ linux-2.6.26-rc4-quilt1/arch/arm/mach-s3c2440/mach-osiris.c	2008-05-27 15:22:33.000000000 +0100
@@ -1,6 +1,6 @@
 /* linux/arch/arm/mach-s3c2440/mach-osiris.c
  *
- * Copyright (c) 2005 Simtec Electronics
+ * Copyright (c) 2005,2008 Simtec Electronics
  *	http://armlinux.simtec.co.uk/
  *	Ben Dooks <ben-Y5A6D6n0/KfQXOPxS62xeg@public.gmane.org>
  *
@@ -19,6 +19,7 @@
 #include <linux/sysdev.h>
 #include <linux/serial_core.h>
 #include <linux/clk.h>
+#include <linux/i2c.h>
 
 #include <asm/mach/arch.h>
 #include <asm/mach/map.h>
@@ -322,6 +323,19 @@ static struct sys_device osiris_pm_sysde
 	.cls		= &osiris_pm_sysclass,
 };
 
+/* I2C devices fitted. */
+
+static struct i2c_board_info osiris_i2c_info[] __initdata = {
+	{
+		.type	= "tps65011",
+		.addr	= 0x48,
+		.irq	= IRQ_EINT20,
+	}, {
+		.type	= "eeprom",
+		.addr	= 0x50,
+	}
+};
+
 /* Standard Osiris devices */
 
 static struct platform_device *osiris_devices[] __initdata = {
@@ -388,6 +402,9 @@ static void __init osiris_init(void)
 	sysdev_class_register(&osiris_pm_sysclass);
 	sysdev_register(&osiris_pm_sysdev);
 
+	i2c_register_board_info(0, osiris_i2c_info,
+				ARRAY_SIZE(osiris_i2c_info));
+
 	platform_add_devices(osiris_devices, ARRAY_SIZE(osiris_devices));
 };
 

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

  'a smiley only costs 4 bytes'

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

[patch 6/8] BAST: Add i2c device list on Simtec Bast

[Ben Dooks]

[-- Attachment #1: simtec/simtec-mach-bast-add-i2c-boardinfo.patch --]
[-- Type: text/plain, Size: 1874 bytes --]

Add i2c boardinfo for the connected i2c devices on the
Simtec Bast.

Signed-off-by: Ben Dooks <ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org>

Index: linux-2.6.26-rc4-quilt2/arch/arm/mach-s3c2410/mach-bast.c
===================================================================
--- linux-2.6.26-rc4-quilt2.orig/arch/arm/mach-s3c2410/mach-bast.c	2008-05-28 22:37:09.000000000 +0100
+++ linux-2.6.26-rc4-quilt2/arch/arm/mach-s3c2410/mach-bast.c	2008-05-29 10:20:54.000000000 +0100
@@ -1,6 +1,6 @@
 /* linux/arch/arm/mach-s3c2410/mach-bast.c
  *
- * Copyright (c) 2003-2005 Simtec Electronics
+ * Copyright (c) 2003-2005,2008 Simtec Electronics
  *   Ben Dooks <ben-Y5A6D6n0/KfQXOPxS62xeg@public.gmane.org>
  *
  * http://www.simtec.co.uk/products/EB2410ITX/
@@ -21,6 +21,7 @@
 #include <linux/platform_device.h>
 #include <linux/dm9000.h>
 #include <linux/ata_platform.h>
+#include <linux/i2c.h>
 
 #include <net/ax88796.h>
 
@@ -534,6 +535,23 @@ static struct s3c2410fb_mach_info __init
 	.default_display = 1,
 };
 
+/* I2C devices fitted. */
+
+static struct i2c_board_info bast_i2c_devs[] __initdata = {
+	{
+		.type	= "tlv320aic23",
+		.addr	= 0x1a,
+	}, {
+		.type	= "eeprom",
+		.addr	= 0x50,
+	}, {
+		.type	= "simtec-pmu",
+		.addr	= 0x6b,
+	}, {
+		.type	= "ch7013",
+		.addr	= 0x75,
+	},
+};
 
 /* Standard BAST devices */
 
@@ -593,6 +611,9 @@ static void __init bast_init(void)
 	s3c24xx_fb_set_platdata(&bast_fb_info);
 	platform_add_devices(bast_devices, ARRAY_SIZE(bast_devices));
 
+	i2c_register_board_info(0, bast_i2c_devs,
+				ARRAY_SIZE(bast_i2c_devs));
+
 	nor_simtec_init();
 }
 

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

  'a smiley only costs 4 bytes'

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

[patch 7/8] VR1000: Add i2c device list to Thorcom VR1000

[Ben Dooks]

[-- Attachment #1: simtec/simtec-mach-vr1000-add-i2c-boardinfo.patch --]
[-- Type: text/plain, Size: 1935 bytes --]

Add i2c board intialisers to specify the I2C devices
attached on the Thorcom VR1000.

Signed-off-by: Ben Dooks <ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org>

Index: linux-2.6.26-rc4-quilt1/arch/arm/mach-s3c2410/mach-vr1000.c
===================================================================
--- linux-2.6.26-rc4-quilt1.orig/arch/arm/mach-s3c2410/mach-vr1000.c	2008-05-27 23:31:41.000000000 +0100
+++ linux-2.6.26-rc4-quilt1/arch/arm/mach-s3c2410/mach-vr1000.c	2008-05-27 23:59:05.000000000 +0100
@@ -1,6 +1,6 @@
 /* linux/arch/arm/mach-s3c2410/mach-vr1000.c
  *
- * Copyright (c) 2003-2005 Simtec Electronics
+ * Copyright (c) 2003-2005,2008 Simtec Electronics
  *   Ben Dooks <ben-Y5A6D6n0/KfQXOPxS62xeg@public.gmane.org>
  *
  * Machine support for Thorcom VR1000 board. Designed for Thorcom by
@@ -19,6 +19,7 @@
 #include <linux/timer.h>
 #include <linux/init.h>
 #include <linux/dm9000.h>
+#include <linux/i2c.h>
 
 #include <linux/serial.h>
 #include <linux/tty.h>
@@ -315,6 +316,24 @@ static struct platform_device vr1000_led
 	},
 };
 
+/* I2C devices. */
+
+static struct i2c_board_info vr1000_i2c_info[] __initdata = {
+	{
+		.type	= "tlv320aic23",
+		.addr	= 0x1a,
+	}, {
+		.type	= "tmp101",
+		.addr	= 0x48,
+	}, {
+		.type	= "eeprom",
+		.addr	= 0x50,
+	}, {
+		.type	= "m41st87",
+		.addr	= 0x68,
+	},
+};
+
 /* devices for this board */
 
 static struct platform_device *vr1000_devices[] __initdata = {
@@ -373,6 +392,9 @@ static void __init vr1000_init(void)
 {
 	platform_add_devices(vr1000_devices, ARRAY_SIZE(vr1000_devices));
 
+	i2c_register_board_info(0, vr1000_i2c_info,
+				ARRAY_SIZE(vr1000_i2c_info));
+
 	nor_simtec_init();
 }
 

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

  'a smiley only costs 4 bytes'

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

[patch 8/8] ANUBIS: Add i2c device list to Simtec Anubis

[Ben Dooks]

[-- Attachment #1: simtec/simtec-mach-anubis-add-i2c-boardinfo.patch --]
[-- Type: text/plain, Size: 1867 bytes --]

Add i2c board info initialiser to setup the list of
I2C devices present on an Simtec Anubis.

Signed-off-by: Ben Dooks <ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org>

Index: linux-2.6.26-rc4-quilt1/arch/arm/mach-s3c2440/mach-anubis.c
===================================================================
--- linux-2.6.26-rc4-quilt1.orig/arch/arm/mach-s3c2440/mach-anubis.c	2008-05-28 00:04:12.000000000 +0100
+++ linux-2.6.26-rc4-quilt1/arch/arm/mach-s3c2440/mach-anubis.c	2008-05-28 00:05:42.000000000 +0100
@@ -1,6 +1,6 @@
 /* linux/arch/arm/mach-s3c2440/mach-anubis.c
  *
- * Copyright (c) 2003-2005 Simtec Electronics
+ * Copyright (c) 2003-2005,2008 Simtec Electronics
  *	http://armlinux.simtec.co.uk/
  *	Ben Dooks <ben-Y5A6D6n0/KfQXOPxS62xeg@public.gmane.org>
  *
@@ -18,6 +18,7 @@
 #include <linux/serial_core.h>
 #include <linux/platform_device.h>
 #include <linux/ata_platform.h>
+#include <linux/i2c.h>
 
 #include <linux/sm501.h>
 #include <linux/sm501-regs.h>
@@ -421,6 +422,19 @@ static struct clk *anubis_clocks[] = {
 	&s3c24xx_uclk,
 };
 
+/* I2C devices. */
+
+static struct i2c_board_info anubis_i2c_info[] __initdata = {
+	{
+		.type	= "tps65011",
+		.addr	= 0x48,
+		.irq	= IRQ_EINT20,
+	}, {
+		.type	= "eeprom",
+		.addr	= 0x50,
+	}
+};
+
 static void __init anubis_map_io(void)
 {
 	/* initialise the clocks */
@@ -460,6 +474,9 @@ static void __init anubis_map_io(void)
 static void __init anubis_init(void)
 {
 	platform_add_devices(anubis_devices, ARRAY_SIZE(anubis_devices));
+
+	i2c_register_board_info(0, anubis_i2c_info,
+				ARRAY_SIZE(anubis_i2c_info));
 }
 
 

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

  'a smiley only costs 4 bytes'

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [patch 1/8] I2C: S3C2410: Check ACK on byte transmission

[Jean Delvare]

Hi Ben,

On Thu, 29 May 2008 14:22:45 +0100, Ben Dooks wrote:
> We should check for the reception of an ACK after transmissint each

Spelling: transmitting.

> data byte. The address send has been correctly checking this, but the
> data write byte state should have also been checking for these failures.
> 
> As part of the same fix, we remove the ACK checking from the receive
> path where it should not have been checking for an ACK which our hardware
> was sending.
> 
> Signed-off-by: Ben Dooks <ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org>
> 
> Index: linux-2.6.26-rc4-quilt1/drivers/i2c/busses/i2c-s3c2410.c
> ===================================================================
> --- linux-2.6.26-rc4-quilt1.orig/drivers/i2c/busses/i2c-s3c2410.c	2008-05-28 11:49:18.000000000 +0100
> +++ linux-2.6.26-rc4-quilt1/drivers/i2c/busses/i2c-s3c2410.c	2008-05-28 11:56:14.000000000 +0100
> @@ -324,6 +324,15 @@ static int i2s_s3c_irq_nextbyte(struct s
>  		 */
>  
>  	retry_write:
> +		if (!(i2c->msg->flags & I2C_M_IGNORE_NAK)) {
> +			if (iicstat & S3C2410_IICSTAT_LASTBIT) {
> +				dev_dbg(i2c->dev, "WRITE: No Ack\n");
> +
> +				s3c24xx_i2c_stop(i2c, -ECONNREFUSED);
> +				goto out_ack;
> +			}
> +		}
> +

Do you really want to check this again when you jump to retry_write? I
doubt it. My guess is that you want to add the code before the
retry_write label, rather than after it.

>  		if (!is_msgend(i2c)) {
>  			byte = i2c->msg->buf[i2c->msg_ptr++];
>  			writeb(byte, i2c->regs + S3C2410_IICDS);
> @@ -377,17 +386,6 @@ static int i2s_s3c_irq_nextbyte(struct s
>  		 * going to do any more read/write
>  		 */
>  
> -		if (!(i2c->msg->flags & I2C_M_IGNORE_NAK) &&
> -		    !(is_msglast(i2c) && is_lastmsg(i2c))) {
> -
> -			if (iicstat & S3C2410_IICSTAT_LASTBIT) {
> -				dev_dbg(i2c->dev, "READ: No Ack\n");
> -
> -				s3c24xx_i2c_stop(i2c, -ECONNREFUSED);
> -				goto out_ack;
> -			}
> -		}
> -
>  		byte = readb(i2c->regs + S3C2410_IICDS);
>  		i2c->msg->buf[i2c->msg_ptr++] = byte;
>  
> 


-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [patch 2/8] I2C: S3C2410: Add MODULE_ALIAS() for s3c2440 device.

[Jean Delvare]

Hi Ben,

On Thu, 29 May 2008 14:22:46 +0100, Ben Dooks wrote:
> Add a MODULE_ALIAS() statement for the i2c-s3c2410 controller
> to ensure that it can be autoloaded on the S3C2440 systems that
> we support.
> 
> Signed-off-by: Ben Dooks <ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org>
> 
> Index: linux-2.6.26-rc4-quilt1/drivers/i2c/busses/i2c-s3c2410.c
> ===================================================================
> --- linux-2.6.26-rc4-quilt1.orig/drivers/i2c/busses/i2c-s3c2410.c	2008-05-28 11:56:54.000000000 +0100
> +++ linux-2.6.26-rc4-quilt1/drivers/i2c/busses/i2c-s3c2410.c	2008-05-28 11:57:12.000000000 +0100
> @@ -947,3 +947,4 @@ MODULE_DESCRIPTION("S3C24XX I2C Bus driv
>  MODULE_AUTHOR("Ben Dooks, <ben-Y5A6D6n0/KfQXOPxS62xeg@public.gmane.org>");
>  MODULE_LICENSE("GPL");
>  MODULE_ALIAS("platform:s3c2410-i2c");
> +MODULE_ALIAS("platform:s3c2440-i2c");

With the current driver code, that's correct.

Acked-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>

That being said, I think the approach is wrong. You shouldn't register
two different platform drivers just to be able to differentiate between
device types. You have platform_data for that, it's cleaner and cheaper.

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [patch 3/8] I2C: S3C2410: Pass the I2C bus number via drivers platform data

[Jean Delvare]

Hi Ben,

On Thu, 29 May 2008 14:22:47 +0100, Ben Dooks wrote:
> Allow the platform data to specify the bus bumber that the
> new I2C bus will be given. This is to allow the use of the
> board registration mechanism to specify the new style of
> I2C device registration which allows boards to provide a
> list of attached devices.
> 
> Note, as discussed on the mailing list, we have dropped
> backwards compatibility of adding an dynamic bus number
> as it should not affect most boards to have the bus pinned
> to 0 if they have either not specified platform data for
> driver. Any board supplying platform data will automatically
> have the bus_num field set to 0, and anyone who needs the
> driver on a different bus number can supply platform data
> to set bus_num.
> 
> Signed-off-by: Ben Dooks <ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org>
> 
> Index: linux-2.6.26-rc4-quilt2/drivers/i2c/busses/i2c-s3c2410.c
> ===================================================================
> --- linux-2.6.26-rc4-quilt2.orig/drivers/i2c/busses/i2c-s3c2410.c	2008-05-28 12:25:20.000000000 +0100
> +++ linux-2.6.26-rc4-quilt2/drivers/i2c/busses/i2c-s3c2410.c	2008-05-28 12:27:38.000000000 +0100
> @@ -751,9 +751,12 @@ static int s3c24xx_i2c_init(struct s3c24
>  static int s3c24xx_i2c_probe(struct platform_device *pdev)
>  {
>  	struct s3c24xx_i2c *i2c = &s3c24xx_i2c;
> +	struct s3c2410_platform_i2c *pdata;
>  	struct resource *res;
>  	int ret;
>  
> +	pdata = s3c24xx_i2c_get_platformdata(&pdev->dev);
> +
>  	/* find the clock and enable it */
>  
>  	i2c->dev = &pdev->dev;
> @@ -831,7 +834,15 @@ static int s3c24xx_i2c_probe(struct plat
>  	dev_dbg(&pdev->dev, "irq resource %p (%lu)\n", res,
>  		(unsigned long)res->start);
>  
> -	ret = i2c_add_adapter(&i2c->adap);
> +	/* Note, previous versions of the driver used i2c_add_adapter()
> +	 * to add an bus at any number. We now pass the bus number via
> +	 * the platform data, so if unset it will now default to always
> +	 * being bus 0.
> +	 */
> +
> +	i2c->adap.nr = pdata->bus_num;
> +	ret = i2c_add_numbered_adapter(&i2c->adap);
> +
>  	if (ret < 0) {

Minor stylistic comment: in general you shouldn't have blank lines
between a function call and the code which checks the values returned
by the function in question.

>  		dev_err(&pdev->dev, "failed to add bus to i2c core\n");
>  		goto err_irq;
> Index: linux-2.6.26-rc4-quilt2/include/asm-arm/plat-s3c/iic.h
> ===================================================================
> --- linux-2.6.26-rc4-quilt2.orig/include/asm-arm/plat-s3c/iic.h	2008-05-28 11:49:18.000000000 +0100
> +++ linux-2.6.26-rc4-quilt2/include/asm-arm/plat-s3c/iic.h	2008-05-28 12:28:00.000000000 +0100
> @@ -21,6 +21,7 @@
>  */
>  
>  struct s3c2410_platform_i2c {
> +	int		bus_num;	/* bus number to use */
>  	unsigned int	flags;
>  	unsigned int	slave_addr;	/* slave address for controller */
>  	unsigned long	bus_freq;	/* standard bus frequency */
> 

Other than that, looks good to me.

Acked-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>

Do you plan to send this patch (and the next ones in the series) to
Linus yourself, or should I take them in my tree?

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [patch 4/8] I2C: S3C2410: Add a option of reducing the bus busy waiting time.

[Jean Delvare]

Hi Ben,

On Thu, 29 May 2008 14:22:48 +0100, Ben Dooks wrote:
> Add a single-master option to drasitcally reduce the time spent
> waiting to check the bus is free and ready for another transfer.

I don't understand. If you have a single-master bus, then the bus can
simply never be busy when you attempt to start a transaction. So, the
timeout setting shouldn't change anything, as you will get the bus
immediately anyway. Isn't it the case?

As a side note, this timeout loop is pretty broken anyway, as it
depends on the value of HZ. It should be adjusted to be time-based
rather than count-based.

> 
> Signed-off-by: Ben Dooks <ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org>
> 
> Index: linux-2.6.26-rc4-quilt2/drivers/i2c/busses/i2c-s3c2410.c
> ===================================================================
> --- linux-2.6.26-rc4-quilt2.orig/drivers/i2c/busses/i2c-s3c2410.c	2008-05-28 12:27:38.000000000 +0100
> +++ linux-2.6.26-rc4-quilt2/drivers/i2c/busses/i2c-s3c2410.c	2008-05-28 12:32:10.000000000 +0100
> @@ -472,8 +472,15 @@ static irqreturn_t s3c24xx_i2c_irq(int i
>  
>  static int s3c24xx_i2c_set_master(struct s3c24xx_i2c *i2c)
>  {
> +	struct s3c2410_platform_i2c *pdata;
>  	unsigned long iicstat;
> -	int timeout = 400;
> +	int timeout;
> +
> +	/* if we are a single master on this bus, reduce the delay awaiting
> +	 * for bus traffic to a much lower value. */
> +
> +	pdata = s3c24xx_i2c_get_platformdata(i2c->dev);
> +	timeout = (pdata->flags & S3C_IICFLG_SINGLE_MASTER) ? 10 : 400;
>  
>  	while (timeout-- > 0) {
>  		iicstat = readl(i2c->regs + S3C2410_IICSTAT);
> Index: linux-2.6.26-rc4-quilt2/include/asm-arm/plat-s3c/iic.h
> ===================================================================
> --- linux-2.6.26-rc4-quilt2.orig/include/asm-arm/plat-s3c/iic.h	2008-05-28 12:28:00.000000000 +0100
> +++ linux-2.6.26-rc4-quilt2/include/asm-arm/plat-s3c/iic.h	2008-05-28 12:32:10.000000000 +0100
> @@ -13,7 +13,8 @@
>  #ifndef __ASM_ARCH_IIC_H
>  #define __ASM_ARCH_IIC_H __FILE__
>  
> -#define S3C_IICFLG_FILTER	(1<<0)	/* enable s3c2440 filter */
> +#define S3C_IICFLG_FILTER	 (1<<0)	/* enable s3c2440 filter */
> +#define S3C_IICFLG_SINGLE_MASTER (1<<1) /* s3c24xx is only bus master. */
>  
>  /* Notes:
>   *	1) All frequencies are expressed in Hz
> 


-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [patch 5/8] OSIRIS: Add i2c device list to Simtec Osiris

[Jean Delvare]

Hi Ben,

On Thu, 29 May 2008 14:22:49 +0100, Ben Dooks wrote:
> Add an i2c board information initialisers to the board
> to define which devices are present.
> 
> Signed-off-by: Ben Dooks <ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org>
> 
> --- linux-2.6.26-rc4-quilt1.orig/arch/arm/mach-s3c2440/mach-osiris.c	2008-05-27 15:14:42.000000000 +0100
> +++ linux-2.6.26-rc4-quilt1/arch/arm/mach-s3c2440/mach-osiris.c	2008-05-27 15:22:33.000000000 +0100
> @@ -1,6 +1,6 @@
>  /* linux/arch/arm/mach-s3c2440/mach-osiris.c
>   *
> - * Copyright (c) 2005 Simtec Electronics
> + * Copyright (c) 2005,2008 Simtec Electronics
>   *	http://armlinux.simtec.co.uk/
>   *	Ben Dooks <ben-Y5A6D6n0/KfQXOPxS62xeg@public.gmane.org>
>   *
> @@ -19,6 +19,7 @@
>  #include <linux/sysdev.h>
>  #include <linux/serial_core.h>
>  #include <linux/clk.h>
> +#include <linux/i2c.h>
>  
>  #include <asm/mach/arch.h>
>  #include <asm/mach/map.h>
> @@ -322,6 +323,19 @@ static struct sys_device osiris_pm_sysde
>  	.cls		= &osiris_pm_sysclass,
>  };
>  
> +/* I2C devices fitted. */
> +
> +static struct i2c_board_info osiris_i2c_info[] __initdata = {
> +	{
> +		.type	= "tps65011",
> +		.addr	= 0x48,

Please use the I2C_BOARD_INFO helper in all your device declarations.

> +		.irq	= IRQ_EINT20,
> +	}, {
> +		.type	= "eeprom",
> +		.addr	= 0x50,
> +	}

The "eeprom" driver is not a new-style i2c driver, so it won't be able
to bind to device. Having it declared here even prevents the legacy
binding to happen, because the address will be busy. So you should
either not list the eeprom here and let the legacy eeprom driver pick
it later, or add a new-style mode to the eeprom driver - but then you
probably want to use David Brownell and Wolfram Sang's at24 driver
instead, it's much better.

> +};
> +
>  /* Standard Osiris devices */
>  
>  static struct platform_device *osiris_devices[] __initdata = {
> @@ -388,6 +402,9 @@ static void __init osiris_init(void)
>  	sysdev_class_register(&osiris_pm_sysclass);
>  	sysdev_register(&osiris_pm_sysdev);
>  
> +	i2c_register_board_info(0, osiris_i2c_info,
> +				ARRAY_SIZE(osiris_i2c_info));
> +
>  	platform_add_devices(osiris_devices, ARRAY_SIZE(osiris_devices));
>  };
>  
> 

Same comments apply to the next 3 patches of the series.

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [patch 7/8] VR1000: Add i2c device list to Thorcom VR1000

[Jean Delvare]

On Thu, 29 May 2008 14:22:51 +0100, Ben Dooks wrote:
> Add i2c board intialisers to specify the I2C devices
> attached on the Thorcom VR1000.
> 
> Signed-off-by: Ben Dooks <ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org>
> 
> Index: linux-2.6.26-rc4-quilt1/arch/arm/mach-s3c2410/mach-vr1000.c
> ===================================================================
> --- linux-2.6.26-rc4-quilt1.orig/arch/arm/mach-s3c2410/mach-vr1000.c	2008-05-27 23:31:41.000000000 +0100
> +++ linux-2.6.26-rc4-quilt1/arch/arm/mach-s3c2410/mach-vr1000.c	2008-05-27 23:59:05.000000000 +0100
> @@ -1,6 +1,6 @@
>  /* linux/arch/arm/mach-s3c2410/mach-vr1000.c
>   *
> - * Copyright (c) 2003-2005 Simtec Electronics
> + * Copyright (c) 2003-2005,2008 Simtec Electronics
>   *   Ben Dooks <ben-Y5A6D6n0/KfQXOPxS62xeg@public.gmane.org>
>   *
>   * Machine support for Thorcom VR1000 board. Designed for Thorcom by
> @@ -19,6 +19,7 @@
>  #include <linux/timer.h>
>  #include <linux/init.h>
>  #include <linux/dm9000.h>
> +#include <linux/i2c.h>
>  
>  #include <linux/serial.h>
>  #include <linux/tty.h>
> @@ -315,6 +316,24 @@ static struct platform_device vr1000_led
>  	},
>  };
>  
> +/* I2C devices. */
> +
> +static struct i2c_board_info vr1000_i2c_info[] __initdata = {
> +	{
> +		.type	= "tlv320aic23",
> +		.addr	= 0x1a,
> +	}, {
> +		.type	= "tmp101",
> +		.addr	= 0x48,

Same problem as "eeprom"... The lm75 driver doesn't yet support the
"tmp101" as a new-style device in mainline, so this won't work and will
prevent the lm75 driver from attaching as a legacy driver too. So this
device declaration should be delayed or commented out until the lm75
driver is ready.

> +	}, {
> +		.type	= "eeprom",
> +		.addr	= 0x50,
> +	}, {
> +		.type	= "m41st87",
> +		.addr	= 0x68,
> +	},
> +};
> +
>  /* devices for this board */
>  
>  static struct platform_device *vr1000_devices[] __initdata = {
> @@ -373,6 +392,9 @@ static void __init vr1000_init(void)
>  {
>  	platform_add_devices(vr1000_devices, ARRAY_SIZE(vr1000_devices));
>  
> +	i2c_register_board_info(0, vr1000_i2c_info,
> +				ARRAY_SIZE(vr1000_i2c_info));
> +
>  	nor_simtec_init();
>  }
>  
> 


-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [patch 5/8] OSIRIS: Add i2c device list to Simtec Osiris

[Ben Dooks]

On Sun, Jun 01, 2008 at 09:21:16AM +0200, Jean Delvare wrote:
> Hi Ben,
> 
> On Thu, 29 May 2008 14:22:49 +0100, Ben Dooks wrote:
> > Add an i2c board information initialisers to the board
> > to define which devices are present.
> > 
> > Signed-off-by: Ben Dooks <ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org>
> > 
> > --- linux-2.6.26-rc4-quilt1.orig/arch/arm/mach-s3c2440/mach-osiris.c	2008-05-27 15:14:42.000000000 +0100
> > +++ linux-2.6.26-rc4-quilt1/arch/arm/mach-s3c2440/mach-osiris.c	2008-05-27 15:22:33.000000000 +0100
> > @@ -1,6 +1,6 @@
> >  /* linux/arch/arm/mach-s3c2440/mach-osiris.c
> >   *
> > - * Copyright (c) 2005 Simtec Electronics
> > + * Copyright (c) 2005,2008 Simtec Electronics
> >   *	http://armlinux.simtec.co.uk/
> >   *	Ben Dooks <ben-Y5A6D6n0/KfQXOPxS62xeg@public.gmane.org>
> >   *
> > @@ -19,6 +19,7 @@
> >  #include <linux/sysdev.h>
> >  #include <linux/serial_core.h>
> >  #include <linux/clk.h>
> > +#include <linux/i2c.h>
> >  
> >  #include <asm/mach/arch.h>
> >  #include <asm/mach/map.h>
> > @@ -322,6 +323,19 @@ static struct sys_device osiris_pm_sysde
> >  	.cls		= &osiris_pm_sysclass,
> >  };
> >  
> > +/* I2C devices fitted. */
> > +
> > +static struct i2c_board_info osiris_i2c_info[] __initdata = {
> > +	{
> > +		.type	= "tps65011",
> > +		.addr	= 0x48,
> 
> Please use the I2C_BOARD_INFO helper in all your device declarations.
> 
> > +		.irq	= IRQ_EINT20,
> > +	}, {
> > +		.type	= "eeprom",
> > +		.addr	= 0x50,
> > +	}
> 
> The "eeprom" driver is not a new-style i2c driver, so it won't be able
> to bind to device. Having it declared here even prevents the legacy
> binding to happen, because the address will be busy. So you should
> either not list the eeprom here and let the legacy eeprom driver pick
> it later, or add a new-style mode to the eeprom driver - but then you
> probably want to use David Brownell and Wolfram Sang's at24 driver
> instead, it's much better.

Thanks, I'll change this to at94. Is this driver going to be included
on the next kernel window?

-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.


_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [patch 7/8] VR1000: Add i2c device list to Thorcom VR1000

[Ben Dooks]

On Sun, Jun 01, 2008 at 09:32:15AM +0200, Jean Delvare wrote:
> On Thu, 29 May 2008 14:22:51 +0100, Ben Dooks wrote:
> > Add i2c board intialisers to specify the I2C devices
> > attached on the Thorcom VR1000.
> > 
> > Signed-off-by: Ben Dooks <ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org>
> > 
> > Index: linux-2.6.26-rc4-quilt1/arch/arm/mach-s3c2410/mach-vr1000.c
> > ===================================================================
> > --- linux-2.6.26-rc4-quilt1.orig/arch/arm/mach-s3c2410/mach-vr1000.c	2008-05-27 23:31:41.000000000 +0100
> > +++ linux-2.6.26-rc4-quilt1/arch/arm/mach-s3c2410/mach-vr1000.c	2008-05-27 23:59:05.000000000 +0100
> > @@ -1,6 +1,6 @@
> >  /* linux/arch/arm/mach-s3c2410/mach-vr1000.c
> >   *
> > - * Copyright (c) 2003-2005 Simtec Electronics
> > + * Copyright (c) 2003-2005,2008 Simtec Electronics
> >   *   Ben Dooks <ben-Y5A6D6n0/KfQXOPxS62xeg@public.gmane.org>
> >   *
> >   * Machine support for Thorcom VR1000 board. Designed for Thorcom by
> > @@ -19,6 +19,7 @@
> >  #include <linux/timer.h>
> >  #include <linux/init.h>
> >  #include <linux/dm9000.h>
> > +#include <linux/i2c.h>
> >  
> >  #include <linux/serial.h>
> >  #include <linux/tty.h>
> > @@ -315,6 +316,24 @@ static struct platform_device vr1000_led
> >  	},
> >  };
> >  
> > +/* I2C devices. */
> > +
> > +static struct i2c_board_info vr1000_i2c_info[] __initdata = {
> > +	{
> > +		.type	= "tlv320aic23",
> > +		.addr	= 0x1a,
> > +	}, {
> > +		.type	= "tmp101",
> > +		.addr	= 0x48,
> 
> Same problem as "eeprom"... The lm75 driver doesn't yet support the
> "tmp101" as a new-style device in mainline, so this won't work and will
> prevent the lm75 driver from attaching as a legacy driver too. So this
> device declaration should be delayed or commented out until the lm75
> driver is ready.

I'll think about that one, although it does take a force to load the lm75
on current kernels due to it not being 'truly' lm75 compatible. Is there
any chance of the new driver being merged during the next change window?

PS, I've done a quick test of the new lm75 driver and it seems to work
fine for me and would like to see a solution where we don't need to force
probe the lm75 module.

-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.


_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [patch 2/8] I2C: S3C2410: Add MODULE_ALIAS() for s3c2440 device.

[Ben Dooks]

On Fri, May 30, 2008 at 09:54:52PM +0200, Jean Delvare wrote:
> Hi Ben,
> 
> On Thu, 29 May 2008 14:22:46 +0100, Ben Dooks wrote:
> > Add a MODULE_ALIAS() statement for the i2c-s3c2410 controller
> > to ensure that it can be autoloaded on the S3C2440 systems that
> > we support.
> > 
> > Signed-off-by: Ben Dooks <ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org>
> > 
> > Index: linux-2.6.26-rc4-quilt1/drivers/i2c/busses/i2c-s3c2410.c
> > ===================================================================
> > --- linux-2.6.26-rc4-quilt1.orig/drivers/i2c/busses/i2c-s3c2410.c	2008-05-28 11:56:54.000000000 +0100
> > +++ linux-2.6.26-rc4-quilt1/drivers/i2c/busses/i2c-s3c2410.c	2008-05-28 11:57:12.000000000 +0100
> > @@ -947,3 +947,4 @@ MODULE_DESCRIPTION("S3C24XX I2C Bus driv
> >  MODULE_AUTHOR("Ben Dooks, <ben-Y5A6D6n0/KfQXOPxS62xeg@public.gmane.org>");
> >  MODULE_LICENSE("GPL");
> >  MODULE_ALIAS("platform:s3c2410-i2c");
> > +MODULE_ALIAS("platform:s3c2440-i2c");
> 
> With the current driver code, that's correct.
> 
> Acked-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> 
> That being said, I think the approach is wrong. You shouldn't register
> two different platform drivers just to be able to differentiate between
> device types. You have platform_data for that, it's cleaner and cheaper.

Actually, there are down sides to doing it via platform data, and a few
upsides to using the driver model. I do agree that >1 driver does mean
that you end up allocating more space to the drivers, but I hope that
I can convince you that this isn't without merit. 

The following are downsides:

1) using platform data means that not only do all the boards that have
   i2c busses on (or other peripherals using this method) need to carry[1]
   and register platform data even if they do not need to.

2) We currently change the name of the platform device in the cpu specific
   architecture initialisation, which would mean either moving this to each
   machine or have some form of sharing between the arch code and the board
   init. 

3) Some boards can have more than one cpu type on them, which makes life
   even worse for the above.

The advantage of changing the name of the platform device means that the
type of device is shown in the /sys/devices/platform heirachy without any
need for code to add a new attribute to show it within the device itself.


[1] Even if this is __initdata, it means the kernel has to carry it to
    load it even if it gets dumped at startup, and we have a number of
    boards that are available.

-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.


_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [patch 3/8] I2C: S3C2410: Pass the I2C bus number via drivers platform data

[Ben Dooks]

On Sun, Jun 01, 2008 at 09:07:06AM +0200, Jean Delvare wrote:
> Hi Ben,
> 
> On Thu, 29 May 2008 14:22:47 +0100, Ben Dooks wrote:
> > Allow the platform data to specify the bus bumber that the
> > new I2C bus will be given. This is to allow the use of the
> > board registration mechanism to specify the new style of
> > I2C device registration which allows boards to provide a
> > list of attached devices.
> > 
> > Note, as discussed on the mailing list, we have dropped
> > backwards compatibility of adding an dynamic bus number
> > as it should not affect most boards to have the bus pinned
> > to 0 if they have either not specified platform data for
> > driver. Any board supplying platform data will automatically
> > have the bus_num field set to 0, and anyone who needs the
> > driver on a different bus number can supply platform data
> > to set bus_num.
> > 
> > Signed-off-by: Ben Dooks <ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org>
> > 
> > Index: linux-2.6.26-rc4-quilt2/drivers/i2c/busses/i2c-s3c2410.c
> > ===================================================================
> > --- linux-2.6.26-rc4-quilt2.orig/drivers/i2c/busses/i2c-s3c2410.c	2008-05-28 12:25:20.000000000 +0100
> > +++ linux-2.6.26-rc4-quilt2/drivers/i2c/busses/i2c-s3c2410.c	2008-05-28 12:27:38.000000000 +0100
> > @@ -751,9 +751,12 @@ static int s3c24xx_i2c_init(struct s3c24
> >  static int s3c24xx_i2c_probe(struct platform_device *pdev)
> >  {
> >  	struct s3c24xx_i2c *i2c = &s3c24xx_i2c;
> > +	struct s3c2410_platform_i2c *pdata;
> >  	struct resource *res;
> >  	int ret;
> >  
> > +	pdata = s3c24xx_i2c_get_platformdata(&pdev->dev);
> > +
> >  	/* find the clock and enable it */
> >  
> >  	i2c->dev = &pdev->dev;
> > @@ -831,7 +834,15 @@ static int s3c24xx_i2c_probe(struct plat
> >  	dev_dbg(&pdev->dev, "irq resource %p (%lu)\n", res,
> >  		(unsigned long)res->start);
> >  
> > -	ret = i2c_add_adapter(&i2c->adap);
> > +	/* Note, previous versions of the driver used i2c_add_adapter()
> > +	 * to add an bus at any number. We now pass the bus number via
> > +	 * the platform data, so if unset it will now default to always
> > +	 * being bus 0.
> > +	 */
> > +
> > +	i2c->adap.nr = pdata->bus_num;
> > +	ret = i2c_add_numbered_adapter(&i2c->adap);
> > +
> >  	if (ret < 0) {
> 
> Minor stylistic comment: in general you shouldn't have blank lines
> between a function call and the code which checks the values returned
> by the function in question.

Ok, will change this.
 
> >  		dev_err(&pdev->dev, "failed to add bus to i2c core\n");
> >  		goto err_irq;
> > Index: linux-2.6.26-rc4-quilt2/include/asm-arm/plat-s3c/iic.h
> > ===================================================================
> > --- linux-2.6.26-rc4-quilt2.orig/include/asm-arm/plat-s3c/iic.h	2008-05-28 11:49:18.000000000 +0100
> > +++ linux-2.6.26-rc4-quilt2/include/asm-arm/plat-s3c/iic.h	2008-05-28 12:28:00.000000000 +0100
> > @@ -21,6 +21,7 @@
> >  */
> >  
> >  struct s3c2410_platform_i2c {
> > +	int		bus_num;	/* bus number to use */
> >  	unsigned int	flags;
> >  	unsigned int	slave_addr;	/* slave address for controller */
> >  	unsigned long	bus_freq;	/* standard bus frequency */
> > 
> 
> Other than that, looks good to me.
> 
> Acked-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> 
> Do you plan to send this patch (and the next ones in the series) to
> Linus yourself, or should I take them in my tree?

No, firstly these are enhancements for the next kernel, and aren't ready
for merging yet. Secondly they form a part of a larger series that needs to
go to Russell King to avoid any horrible merge problems as some of these
machines require other updates.

I was considering sending the first two patches as fixes as soon as
possible myself as a first test to see how it all works.

-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.


_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [patch 5/8] OSIRIS: Add i2c device list to Simtec Osiris

[Jean Delvare]

On Sun, 1 Jun 2008 22:35:09 +0100, Ben Dooks wrote:
> On Sun, Jun 01, 2008 at 09:21:16AM +0200, Jean Delvare wrote:
> > The "eeprom" driver is not a new-style i2c driver, so it won't be able
> > to bind to device. Having it declared here even prevents the legacy
> > binding to happen, because the address will be busy. So you should
> > either not list the eeprom here and let the legacy eeprom driver pick
> > it later, or add a new-style mode to the eeprom driver - but then you
> > probably want to use David Brownell and Wolfram Sang's at24 driver
> > instead, it's much better.
> 
> Thanks, I'll change this to at94. Is this driver going to be included
> on the next kernel window?

at24, not at94 ;) I hope so. I've reviewed the driver and am now
waiting for Wolfram to send an updated version. The code looked good so
if I receive something soon enough, there's really no reason why it
wouldn't make it in kernel 2.6.27.

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [patch 7/8] VR1000: Add i2c device list to Thorcom VR1000

[Jean Delvare]

On Sun, 1 Jun 2008 22:38:55 +0100, Ben Dooks wrote:
> On Sun, Jun 01, 2008 at 09:32:15AM +0200, Jean Delvare wrote:
> > On Thu, 29 May 2008 14:22:51 +0100, Ben Dooks wrote:
> > > +/* I2C devices. */
> > > +
> > > +static struct i2c_board_info vr1000_i2c_info[] __initdata = {
> > > +	{
> > > +		.type	= "tlv320aic23",
> > > +		.addr	= 0x1a,
> > > +	}, {
> > > +		.type	= "tmp101",
> > > +		.addr	= 0x48,
> > 
> > Same problem as "eeprom"... The lm75 driver doesn't yet support the
> > "tmp101" as a new-style device in mainline, so this won't work and will
> > prevent the lm75 driver from attaching as a legacy driver too. So this
> > device declaration should be delayed or commented out until the lm75
> > driver is ready.
> 
> I'll think about that one, although it does take a force to load the lm75
> on current kernels due to it not being 'truly' lm75 compatible.

That's still better than not being able to load the driver at all,
which will happen with your device declaration above without the
updated lm75 driver.

> Is there
> any chance of the new driver being merged during the next change window?

Depends on Mark M. Hoffman (Cc'd), not me. I've reviewed and acked the
lm75 patches, but it's up to Mark to merge them.

> PS, I've done a quick test of the new lm75 driver and it seems to work
> fine for me and would like to see a solution where we don't need to force
> probe the lm75 module.

Of course, the updated driver is the way to go in your case.

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [patch 3/8] I2C: S3C2410: Pass the I2C bus number via drivers platform data

[Jean Delvare]

Hi Ben,

On Sun, 1 Jun 2008 22:53:35 +0100, Ben Dooks wrote:
> On Sun, Jun 01, 2008 at 09:07:06AM +0200, Jean Delvare wrote:
> > Do you plan to send this patch (and the next ones in the series) to
> > Linus yourself, or should I take them in my tree?
> 
> No, firstly these are enhancements for the next kernel, and aren't ready
> for merging yet. Secondly they form a part of a larger series that needs to
> go to Russell King to avoid any horrible merge problems as some of these
> machines require other updates.

OK, I'll leave these patches to you then.

> I was considering sending the first two patches as fixes as soon as
> possible myself as a first test to see how it all works.

Fine with me as well.

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [patch 2/8] I2C: S3C2410: Add MODULE_ALIAS() for s3c2440 device.

[Jean Delvare]

On Sun, 1 Jun 2008 22:51:02 +0100, Ben Dooks wrote:
> On Fri, May 30, 2008 at 09:54:52PM +0200, Jean Delvare wrote:
> > That being said, I think the approach is wrong. You shouldn't register
> > two different platform drivers just to be able to differentiate between
> > device types. You have platform_data for that, it's cleaner and cheaper.
> 
> Actually, there are down sides to doing it via platform data, and a few
> upsides to using the driver model. I do agree that >1 driver does mean
> that you end up allocating more space to the drivers, but I hope that
> I can convince you that this isn't without merit. 
> 
> The following are downsides:
> 
> 1) using platform data means that not only do all the boards that have
>    i2c busses on (or other peripherals using this method) need to carry[1]
>    and register platform data even if they do not need to.
> 
> 2) We currently change the name of the platform device in the cpu specific
>    architecture initialisation, which would mean either moving this to each
>    machine or have some form of sharing between the arch code and the board
>    init. 
> 
> 3) Some boards can have more than one cpu type on them, which makes life
>    even worse for the above.
> 
> The advantage of changing the name of the platform device means that the
> type of device is shown in the /sys/devices/platform heirachy without any
> need for code to add a new attribute to show it within the device itself.
> 
> 
> [1] Even if this is __initdata, it means the kernel has to carry it to
>     load it even if it gets dumped at startup, and we have a number of
>     boards that are available.
> 

Out of 6 platform driver in the kernel tree that do this, 5 were
written by you. So apparently not many developers share your opinion
that it's the best way to do it.

Anyway, I'm not going to argue any longer, it's your driver and your
platform and you do pretty much what you want with it.

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [patch 1/8] I2C: S3C2410: Check ACK on byte transmission

[Ben Dooks]

On Fri, May 30, 2008 at 09:45:40PM +0200, Jean Delvare wrote:
> Hi Ben,
> 
> On Thu, 29 May 2008 14:22:45 +0100, Ben Dooks wrote:
> > We should check for the reception of an ACK after transmissint each
> 
> Spelling: transmitting.

fixed.
 

> > +		if (!(i2c->msg->flags & I2C_M_IGNORE_NAK)) {
> > +			if (iicstat & S3C2410_IICSTAT_LASTBIT) {
> > +				dev_dbg(i2c->dev, "WRITE: No Ack\n");
> > +
> > +				s3c24xx_i2c_stop(i2c, -ECONNREFUSED);
> > +				goto out_ack;
> > +			}
> > +		}
> > +
> 
> Do you really want to check this again when you jump to retry_write? I
> doubt it. My guess is that you want to add the code before the
> retry_write label, rather than after it.

fixed, not really a bug, but more efficient to not try and run this
through again after we've already checked the i2c master status in
the interrupt.

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

  'a smiley only costs 4 bytes'

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c