[PATCH 0/2] ISP1301 updates

[PATCH 0/2] ISP1301 updates

[Felipe Balbi]

Hello all,

The following two patches updates isp1301_omap.c to new-style i2c driver.
The patches are against curent linux mailine head.

Tony Lindgren (Cc'ed here) already has a working version of this driver
on his linux-omap git tree (see [1] and [2]), these are just a rework for
them to apply cleanly on mainline.

The patches were compile tested with omap_h2_1610_defconfig with isp1301
enabled.


[1] http://git.kernel.org/?p=linux/kernel/git/tmlind/linux-omap-2.6.git;a=commit;h=d8e0e848e562deaa405a2014240d6deedf3bfd37
[2] http://git.kernel.org/?p=linux/kernel/git/tmlind/linux-omap-2.6.git;a=commit;h=d24c8950a363044016679cdf76435abdace2f56d

BR,

Felipe Balbi

[PATCH 1/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 1

[Felipe Balbi]

Based on David Brownell's patch for tps65010, this patch
starts converting isp1301_omap.c to new-style i2c driver.

Signed-off-by: Felipe Balbi <me@felipebalbi.com>
---
 drivers/i2c/chips/isp1301_omap.c |   60 +++++++++++++++++++------------------
 1 files changed, 31 insertions(+), 29 deletions(-)

diff --git a/drivers/i2c/chips/isp1301_omap.c b/drivers/i2c/chips/isp1301_omap.c
index b767603..37e1403 100644
--- a/drivers/i2c/chips/isp1301_omap.c
+++ b/drivers/i2c/chips/isp1301_omap.c
@@ -49,7 +49,8 @@ MODULE_LICENSE("GPL");
 
 struct isp1301 {
 	struct otg_transceiver	otg;
-	struct i2c_client	client;
+	struct i2c_client	*client;
+	struct i2c_client	c;
 	void			(*i2c_release)(struct device *dev);
 
 	int			irq;
@@ -153,25 +154,25 @@ static struct i2c_driver isp1301_driver;
 static inline u8
 isp1301_get_u8(struct isp1301 *isp, u8 reg)
 {
-	return i2c_smbus_read_byte_data(&isp->client, reg + 0);
+	return i2c_smbus_read_byte_data(isp->client, reg + 0);
 }
 
 static inline int
 isp1301_get_u16(struct isp1301 *isp, u8 reg)
 {
-	return i2c_smbus_read_word_data(&isp->client, reg);
+	return i2c_smbus_read_word_data(isp->client, reg);
 }
 
 static inline int
 isp1301_set_bits(struct isp1301 *isp, u8 reg, u8 bits)
 {
-	return i2c_smbus_write_byte_data(&isp->client, reg + 0, bits);
+	return i2c_smbus_write_byte_data(isp->client, reg + 0, bits);
 }
 
 static inline int
 isp1301_clear_bits(struct isp1301 *isp, u8 reg, u8 bits)
 {
-	return i2c_smbus_write_byte_data(&isp->client, reg + 1, bits);
+	return i2c_smbus_write_byte_data(isp->client, reg + 1, bits);
 }
 
 /*-------------------------------------------------------------------------*/
@@ -355,10 +356,10 @@ isp1301_defer_work(struct isp1301 *isp, int work)
 	int status;
 
 	if (isp && !test_and_set_bit(work, &isp->todo)) {
-		(void) get_device(&isp->client.dev);
+		(void) get_device(&isp->client->dev);
 		status = schedule_work(&isp->work);
 		if (!status && !isp->working)
-			dev_vdbg(&isp->client.dev,
+			dev_vdbg(&isp->client->dev,
 				"work item %d may be lost\n", work);
 	}
 }
@@ -1113,7 +1114,7 @@ isp1301_work(struct work_struct *work)
 		/* transfer state from otg engine to isp1301 */
 		if (test_and_clear_bit(WORK_UPDATE_ISP, &isp->todo)) {
 			otg_update_isp(isp);
-			put_device(&isp->client.dev);
+			put_device(&isp->client->dev);
 		}
 #endif
 		/* transfer state from isp1301 to otg engine */
@@ -1121,7 +1122,7 @@ isp1301_work(struct work_struct *work)
 			u8		stat = isp1301_clear_latch(isp);
 
 			isp_update_otg(isp, stat);
-			put_device(&isp->client.dev);
+			put_device(&isp->client->dev);
 		}
 
 		if (test_and_clear_bit(WORK_HOST_RESUME, &isp->todo)) {
@@ -1156,7 +1157,7 @@ isp1301_work(struct work_struct *work)
 			}
 			host_resume(isp);
 			// mdelay(10);
-			put_device(&isp->client.dev);
+			put_device(&isp->client->dev);
 		}
 
 		if (test_and_clear_bit(WORK_TIMER, &isp->todo)) {
@@ -1165,15 +1166,15 @@ isp1301_work(struct work_struct *work)
 			if (!stop)
 				mod_timer(&isp->timer, jiffies + TIMER_JIFFIES);
 #endif
-			put_device(&isp->client.dev);
+			put_device(&isp->client->dev);
 		}
 
 		if (isp->todo)
-			dev_vdbg(&isp->client.dev,
+			dev_vdbg(&isp->client->dev,
 				"work done, todo = 0x%lx\n",
 				isp->todo);
 		if (stop) {
-			dev_dbg(&isp->client.dev, "stop\n");
+			dev_dbg(&isp->client->dev, "stop\n");
 			break;
 		}
 	} while (isp->todo);
@@ -1197,7 +1198,7 @@ static void isp1301_release(struct device *dev)
 {
 	struct isp1301	*isp;
 
-	isp = container_of(dev, struct isp1301, client.dev);
+	isp = container_of(dev, struct isp1301, c.dev);
 
 	/* ugly -- i2c hijacks our memory hook to wait_for_completion() */
 	if (isp->i2c_release)
@@ -1211,7 +1212,7 @@ static int isp1301_detach_client(struct i2c_client *i2c)
 {
 	struct isp1301	*isp;
 
-	isp = container_of(i2c, struct isp1301, client);
+	isp = container_of(i2c, struct isp1301, c);
 
 	isp1301_clear_bits(isp, ISP1301_INTERRUPT_FALLING, ~0);
 	isp1301_clear_bits(isp, ISP1301_INTERRUPT_RISING, ~0);
@@ -1263,7 +1264,7 @@ static int isp1301_otg_enable(struct isp1301 *isp)
 	isp1301_set_bits(isp, ISP1301_INTERRUPT_FALLING,
 		INTR_VBUS_VLD | INTR_SESS_VLD | INTR_ID_GND);
 
-	dev_info(&isp->client.dev, "ready for dual-role USB ...\n");
+	dev_info(&isp->client->dev, "ready for dual-role USB ...\n");
 
 	return 0;
 }
@@ -1288,7 +1289,7 @@ isp1301_set_host(struct otg_transceiver *otg, struct usb_bus *host)
 
 #ifdef	CONFIG_USB_OTG
 	isp->otg.host = host;
-	dev_dbg(&isp->client.dev, "registered host\n");
+	dev_dbg(&isp->client->dev, "registered host\n");
 	host_suspend(isp);
 	if (isp->otg.gadget)
 		return isp1301_otg_enable(isp);
@@ -1303,7 +1304,7 @@ isp1301_set_host(struct otg_transceiver *otg, struct usb_bus *host)
 	if (machine_is_omap_h2())
 		isp1301_set_bits(isp, ISP1301_MODE_CONTROL_1, MC1_DAT_SE0);
 
-	dev_info(&isp->client.dev, "A-Host sessions ok\n");
+	dev_info(&isp->client->dev, "A-Host sessions ok\n");
 	isp1301_set_bits(isp, ISP1301_INTERRUPT_RISING,
 		INTR_ID_GND);
 	isp1301_set_bits(isp, ISP1301_INTERRUPT_FALLING,
@@ -1321,7 +1322,7 @@ isp1301_set_host(struct otg_transceiver *otg, struct usb_bus *host)
 	return 0;
 
 #else
-	dev_dbg(&isp->client.dev, "host sessions not allowed\n");
+	dev_dbg(&isp->client->dev, "host sessions not allowed\n");
 	return -EINVAL;
 #endif
 
@@ -1347,7 +1348,7 @@ isp1301_set_peripheral(struct otg_transceiver *otg, struct usb_gadget *gadget)
 
 #ifdef	CONFIG_USB_OTG
 	isp->otg.gadget = gadget;
-	dev_dbg(&isp->client.dev, "registered gadget\n");
+	dev_dbg(&isp->client->dev, "registered gadget\n");
 	/* gadget driver may be suspended until vbus_connect () */
 	if (isp->otg.host)
 		return isp1301_otg_enable(isp);
@@ -1370,7 +1371,7 @@ isp1301_set_peripheral(struct otg_transceiver *otg, struct usb_gadget *gadget)
 		INTR_SESS_VLD);
 	isp1301_set_bits(isp, ISP1301_INTERRUPT_FALLING,
 		INTR_VBUS_VLD);
-	dev_info(&isp->client.dev, "B-Peripheral sessions ok\n");
+	dev_info(&isp->client->dev, "B-Peripheral sessions ok\n");
 	dump_regs(isp, __FUNCTION__);
 
 	/* If this has a Mini-AB connector, this mode is highly
@@ -1383,7 +1384,7 @@ isp1301_set_peripheral(struct otg_transceiver *otg, struct usb_gadget *gadget)
 	return 0;
 
 #else
-	dev_dbg(&isp->client.dev, "peripheral sessions not allowed\n");
+	dev_dbg(&isp->client->dev, "peripheral sessions not allowed\n");
 	return -EINVAL;
 #endif
 }
@@ -1499,12 +1500,13 @@ static int isp1301_probe(struct i2c_adapter *bus, int address, int kind)
 	isp->timer.data = (unsigned long) isp;
 
 	isp->irq = -1;
-	isp->client.addr = address;
-	i2c_set_clientdata(&isp->client, isp);
-	isp->client.adapter = bus;
-	isp->client.driver = &isp1301_driver;
-	strlcpy(isp->client.name, DRIVER_NAME, I2C_NAME_SIZE);
-	i2c = &isp->client;
+	isp->irq_type = 0;
+	isp->c.addr = address;
+	i2c_set_clientdata(&isp->c, isp);
+	isp->c.adapter = bus;
+	isp->c.driver = &isp1301_driver;
+	strlcpy(isp->c.name, DRIVER_NAME, I2C_NAME_SIZE);
+	isp->client = i2c = &isp->c;
 
 	/* if this is a true probe, verify the chip ... */
 	if (kind < 0) {
@@ -1589,7 +1591,7 @@ fail2:
 		goto fail1;
 	}
 
-	isp->otg.dev = &isp->client.dev;
+	isp->otg.dev = &isp->client->dev;
 	isp->otg.label = DRIVER_NAME;
 
 	isp->otg.set_host = isp1301_set_host,
-- 
1.5.4.rc1.21.g0e545-dirty

[PATCH 2/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 2

[Felipe Balbi]

Based on David Brownell's patch for tps65010, this patch
finish conversting isp1301_omap.c to new-style i2c driver.

Signed-off-by: Felipe Balbi <me@felipebalbi.com>
---
 arch/arm/mach-omap1/board-h2.c   |    6 ++-
 arch/arm/mach-omap1/board-h3.c   |    6 ++-
 arch/arm/mach-omap2/board-h4.c   |   12 +++
 drivers/i2c/chips/isp1301_omap.c |  149 +++++++++++++-------------------------
 4 files changed, 73 insertions(+), 100 deletions(-)

diff --git a/arch/arm/mach-omap1/board-h2.c b/arch/arm/mach-omap1/board-h2.c
index 1306812..0307f50 100644
--- a/arch/arm/mach-omap1/board-h2.c
+++ b/arch/arm/mach-omap1/board-h2.c
@@ -350,8 +350,12 @@ static struct i2c_board_info __initdata h2_i2c_board_info[] = {
 		.type		= "tps65010",
 		.irq		= OMAP_GPIO_IRQ(58),
 	},
+	{
+		I2C_BOARD_INFO("isp1301_omap", 0x2d),
+		.type		= "isp1301_omap",
+		.irq		= OMAP_GPIO_IRQ(2),
+	},
 	/* TODO when driver support is ready:
-	 *  - isp1301 OTG transceiver
 	 *  - optional ov9640 camera sensor at 0x30
 	 *  - pcf9754 for aGPS control
 	 *  - ... etc
diff --git a/arch/arm/mach-omap1/board-h3.c b/arch/arm/mach-omap1/board-h3.c
index 4f84ae2..71e285a 100644
--- a/arch/arm/mach-omap1/board-h3.c
+++ b/arch/arm/mach-omap1/board-h3.c
@@ -463,8 +463,12 @@ static struct i2c_board_info __initdata h3_i2c_board_info[] = {
 		.type		= "tps65013",
 		/* .irq		= OMAP_GPIO_IRQ(??), */
 	},
+	{
+		I2C_BOARD_INFO("isp1301_omap", 0x2d),
+		.type		= "isp1301_omap",
+		.irq		= OMAP_GPIO_IRQ(14),
+	},
 	/* TODO when driver support is ready:
-	 *  - isp1301 OTG transceiver
 	 *  - optional ov9640 camera sensor at 0x30
 	 *  - ...
 	 */
diff --git a/arch/arm/mach-omap2/board-h4.c b/arch/arm/mach-omap2/board-h4.c
index f125f43..33ff80b 100644
--- a/arch/arm/mach-omap2/board-h4.c
+++ b/arch/arm/mach-omap2/board-h4.c
@@ -301,6 +301,15 @@ static struct omap_board_config_kernel h4_config[] = {
 	{ OMAP_TAG_LCD,		&h4_lcd_config },
 };
 
+static struct i2c_board_info __initdata h4_i2c_board_info[] = {
+	{
+		I2C_BOARD_INFO("isp1301_omap", 0x2d),
+		.type		= "isp1301_omap",
+		.irq		= OMAP_GPIO_IRQ(125),
+	},
+};
+
+
 static void __init omap_h4_init(void)
 {
 	/*
@@ -321,6 +330,9 @@ static void __init omap_h4_init(void)
 	}
 #endif
 
+	i2c_register_board_info(1, h4_i2c_board_info,
+			ARRAY_SIZE(h4_i2c_board_info));
+
 	platform_add_devices(h4_devices, ARRAY_SIZE(h4_devices));
 	omap_board_config = h4_config;
 	omap_board_config_size = ARRAY_SIZE(h4_config);
diff --git a/drivers/i2c/chips/isp1301_omap.c b/drivers/i2c/chips/isp1301_omap.c
index 37e1403..c7a7c48 100644
--- a/drivers/i2c/chips/isp1301_omap.c
+++ b/drivers/i2c/chips/isp1301_omap.c
@@ -31,16 +31,12 @@
 #include <linux/usb/otg.h>
 #include <linux/i2c.h>
 #include <linux/workqueue.h>
-
-#include <asm/irq.h>
 #include <asm/arch/usb.h>
 
-
 #ifndef	DEBUG
 #undef	VERBOSE
 #endif
 
-
 #define	DRIVER_VERSION	"24 August 2004"
 #define	DRIVER_NAME	(isp1301_driver.driver.name)
 
@@ -50,12 +46,8 @@ MODULE_LICENSE("GPL");
 struct isp1301 {
 	struct otg_transceiver	otg;
 	struct i2c_client	*client;
-	struct i2c_client	c;
 	void			(*i2c_release)(struct device *dev);
 
-	int			irq;
-	int			irq_type;
-
 	u32			last_otg_ctrl;
 	unsigned		working:1;
 
@@ -140,13 +132,6 @@ static inline void notresponding(struct isp1301 *isp)
 /*-------------------------------------------------------------------------*/
 
 /* only two addresses possible */
-#define	ISP_BASE		0x2c
-static unsigned short normal_i2c[] = {
-	ISP_BASE, ISP_BASE + 1,
-	I2C_CLIENT_END };
-
-I2C_CLIENT_INSMOD;
-
 static struct i2c_driver isp1301_driver;
 
 /* smbus apis are used for portability */
@@ -1196,9 +1181,11 @@ static void isp1301_timer(unsigned long _isp)
 
 static void isp1301_release(struct device *dev)
 {
-	struct isp1301	*isp;
+	struct i2c_client	*client;
+	struct isp1301		*isp;
 
-	isp = container_of(dev, struct isp1301, c.dev);
+	client = container_of(dev, struct i2c_client, dev);
+	isp = i2c_get_clientdata(client);
 
 	/* ugly -- i2c hijacks our memory hook to wait_for_completion() */
 	if (isp->i2c_release)
@@ -1208,30 +1195,27 @@ static void isp1301_release(struct device *dev)
 
 static struct isp1301 *the_transceiver;
 
-static int isp1301_detach_client(struct i2c_client *i2c)
+static int __exit isp1301_remove(struct i2c_client *client)
 {
-	struct isp1301	*isp;
-
-	isp = container_of(i2c, struct isp1301, c);
+	struct isp1301	*isp = i2c_get_clientdata(client);
 
 	isp1301_clear_bits(isp, ISP1301_INTERRUPT_FALLING, ~0);
 	isp1301_clear_bits(isp, ISP1301_INTERRUPT_RISING, ~0);
-	free_irq(isp->irq, isp);
+
+	if (client->irq > 0)
+		free_irq(client->irq, isp);
 #ifdef	CONFIG_USB_OTG
 	otg_unbind(isp);
 #endif
-	if (machine_is_omap_h2())
-		omap_free_gpio(2);
-
 	isp->timer.data = 0;
 	set_bit(WORK_STOP, &isp->todo);
 	del_timer_sync(&isp->timer);
 	flush_scheduled_work();
 
-	put_device(&i2c->dev);
+	put_device(&client->dev);
 	the_transceiver = 0;
 
-	return i2c_detach_client(i2c);
+	return 0;
 }
 
 /*-------------------------------------------------------------------------*/
@@ -1301,9 +1285,6 @@ isp1301_set_host(struct otg_transceiver *otg, struct usb_bus *host)
 
 	power_up(isp);
 
-	if (machine_is_omap_h2())
-		isp1301_set_bits(isp, ISP1301_MODE_CONTROL_1, MC1_DAT_SE0);
-
 	dev_info(&isp->client->dev, "A-Host sessions ok\n");
 	isp1301_set_bits(isp, ISP1301_INTERRUPT_RISING,
 		INTR_ID_GND);
@@ -1481,11 +1462,10 @@ isp1301_start_hnp(struct otg_transceiver *dev)
 /*-------------------------------------------------------------------------*/
 
 /* no error returns, they'd just make bus scanning stop */
-static int isp1301_probe(struct i2c_adapter *bus, int address, int kind)
+static int __init isp1301_probe(struct i2c_client *client)
 {
 	int			status;
 	struct isp1301		*isp;
-	struct i2c_client	*i2c;
 
 	if (the_transceiver)
 		return 0;
@@ -1498,46 +1478,35 @@ static int isp1301_probe(struct i2c_adapter *bus, int address, int kind)
 	init_timer(&isp->timer);
 	isp->timer.function = isp1301_timer;
 	isp->timer.data = (unsigned long) isp;
-
-	isp->irq = -1;
-	isp->irq_type = 0;
-	isp->c.addr = address;
-	i2c_set_clientdata(&isp->c, isp);
-	isp->c.adapter = bus;
-	isp->c.driver = &isp1301_driver;
-	strlcpy(isp->c.name, DRIVER_NAME, I2C_NAME_SIZE);
-	isp->client = i2c = &isp->c;
+	isp->client = client;
 
 	/* if this is a true probe, verify the chip ... */
-	if (kind < 0) {
-		status = isp1301_get_u16(isp, ISP1301_VENDOR_ID);
-		if (status != I2C_VENDOR_ID_PHILIPS) {
-			dev_dbg(&bus->dev, "addr %d not philips id: %d\n",
-				address, status);
-			goto fail1;
-		}
-		status = isp1301_get_u16(isp, ISP1301_PRODUCT_ID);
-		if (status != I2C_PRODUCT_ID_PHILIPS_1301) {
-			dev_dbg(&bus->dev, "%d not isp1301, %d\n",
-				address, status);
-			goto fail1;
-		}
+	status = isp1301_get_u16(isp, ISP1301_VENDOR_ID);
+	if (status != I2C_VENDOR_ID_PHILIPS) {
+		dev_dbg(&client->dev, "not philips id: %d\n",
+				status);
+		goto fail1;
+	}
+	status = isp1301_get_u16(isp, ISP1301_PRODUCT_ID);
+	if (status != I2C_PRODUCT_ID_PHILIPS_1301) {
+		dev_dbg(&client->dev, "not isp1301, %d\n",
+				status);
+		goto fail1;
 	}
 
-	status = i2c_attach_client(i2c);
 	if (status < 0) {
-		dev_dbg(&bus->dev, "can't attach %s to device %d, err %d\n",
-				DRIVER_NAME, address, status);
+		dev_dbg(&client->dev, "can't attach %s to device, err %d\n",
+				DRIVER_NAME, status);
 fail1:
 		kfree(isp);
 		return 0;
 	}
-	isp->i2c_release = i2c->dev.release;
-	i2c->dev.release = isp1301_release;
+	isp->i2c_release = client->dev.release;
+	client->dev.release = isp1301_release;
 
 	/* initial development used chiprev 2.00 */
-	status = i2c_smbus_read_word_data(i2c, ISP1301_BCD_DEVICE);
-	dev_info(&i2c->dev, "chiprev %x.%02x, driver " DRIVER_VERSION "\n",
+	status = i2c_smbus_read_word_data(client, ISP1301_BCD_DEVICE);
+	dev_info(&client->dev, "chiprev %x.%02x, driver " DRIVER_VERSION "\n",
 		status >> 8, status & 0xff);
 
 	/* make like power-on reset */
@@ -1558,40 +1527,25 @@ fail1:
 #ifdef	CONFIG_USB_OTG
 	status = otg_bind(isp);
 	if (status < 0) {
-		dev_dbg(&i2c->dev, "can't bind OTG\n");
+		dev_dbg(&client->dev, "can't bind OTG\n");
 		goto fail2;
 	}
 #endif
 
-	if (machine_is_omap_h2()) {
-		/* full speed signaling by default */
-		isp1301_set_bits(isp, ISP1301_MODE_CONTROL_1,
-			MC1_SPEED_REG);
-		isp1301_set_bits(isp, ISP1301_MODE_CONTROL_2,
-			MC2_SPD_SUSP_CTRL);
-
-		/* IRQ wired at M14 */
-		omap_cfg_reg(M14_1510_GPIO2);
-		isp->irq = OMAP_GPIO_IRQ(2);
-		if (gpio_request(2, "isp1301") == 0)
-			gpio_direction_input(2);
-		isp->irq_type = IRQF_TRIGGER_FALLING;
-	}
-
-	isp->irq_type |= IRQF_SAMPLE_RANDOM;
-	status = request_irq(isp->irq, isp1301_irq,
-			isp->irq_type, DRIVER_NAME, isp);
+	status = request_irq(client->irq, isp1301_irq,
+			IRQF_SAMPLE_RANDOM | IRQF_TRIGGER_FALLING,
+			DRIVER_NAME, isp);
 	if (status < 0) {
-		dev_dbg(&i2c->dev, "can't get IRQ %d, err %d\n",
-				isp->irq, status);
+		dev_dbg(&client->dev, "can't get IRQ %d, err %d\n",
+				client->irq, status);
 #ifdef	CONFIG_USB_OTG
 fail2:
 #endif
-		i2c_detach_client(i2c);
+		i2c_detach_client(client);
 		goto fail1;
 	}
 
-	isp->otg.dev = &isp->client->dev;
+	isp->otg.dev = &client->dev;
 	isp->otg.label = DRIVER_NAME;
 
 	isp->otg.set_host = isp1301_set_host,
@@ -1608,43 +1562,42 @@ fail2:
 	update_otg1(isp, isp1301_get_u8(isp, ISP1301_INTERRUPT_SOURCE));
 	update_otg2(isp, isp1301_get_u8(isp, ISP1301_OTG_STATUS));
 #endif
-
 	dump_regs(isp, __FUNCTION__);
 
 #ifdef	VERBOSE
 	mod_timer(&isp->timer, jiffies + TIMER_JIFFIES);
-	dev_dbg(&i2c->dev, "scheduled timer, %d min\n", TIMER_MINUTES);
+	dev_dbg(&client->dev, "scheduled timer, %d min\n", TIMER_MINUTES);
 #endif
 
 	status = otg_set_transceiver(&isp->otg);
 	if (status < 0)
-		dev_err(&i2c->dev, "can't register transceiver, %d\n",
+		dev_err(&client->dev, "can't register transceiver, %d\n",
 			status);
 
 	return 0;
 }
 
-static int isp1301_scan_bus(struct i2c_adapter *bus)
-{
-	if (!i2c_check_functionality(bus, I2C_FUNC_SMBUS_BYTE_DATA
-			| I2C_FUNC_SMBUS_READ_WORD_DATA))
-		return -EINVAL;
-	return i2c_probe(bus, &addr_data, isp1301_probe);
-}
-
 static struct i2c_driver isp1301_driver = {
 	.driver = {
 		.name	= "isp1301_omap",
 	},
-	.attach_adapter	= isp1301_scan_bus,
-	.detach_client	= isp1301_detach_client,
+	.probe	= isp1301_probe,
+	.remove	= __exit_p(isp1301_remove),
 };
 
 /*-------------------------------------------------------------------------*/
 
 static int __init isp_init(void)
 {
-	return i2c_add_driver(&isp1301_driver);
+	int	status = -ENODEV;
+
+	printk(KERN_INFO "%s: version %s\n", DRIVER_NAME, DRIVER_VERSION);
+
+	status = i2c_add_driver(&isp1301_driver);
+	if (status)
+		printk(KERN_ERR "%s failed to probe\n", DRIVER_NAME);
+
+	return status;
 }
 module_init(isp_init);
 
-- 
1.5.4.rc1.21.g0e545-dirty

Re: [PATCH 2/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 2

[Jean Delvare]

Hi Felipe,

On Thu,  3 Jan 2008 11:59:57 -0500, Felipe Balbi wrote:
> Based on David Brownell's patch for tps65010, this patch
> finish conversting isp1301_omap.c to new-style i2c driver.
> 
> Signed-off-by: Felipe Balbi <me@felipebalbi.com>
> ---
>  arch/arm/mach-omap1/board-h2.c   |    6 ++-
>  arch/arm/mach-omap1/board-h3.c   |    6 ++-
>  arch/arm/mach-omap2/board-h4.c   |   12 +++
>  drivers/i2c/chips/isp1301_omap.c |  149 +++++++++++++-------------------------
>  4 files changed, 73 insertions(+), 100 deletions(-)
> 
> diff --git a/arch/arm/mach-omap1/board-h2.c b/arch/arm/mach-omap1/board-h2.c
> index 1306812..0307f50 100644
> --- a/arch/arm/mach-omap1/board-h2.c
> +++ b/arch/arm/mach-omap1/board-h2.c
> @@ -350,8 +350,12 @@ static struct i2c_board_info __initdata h2_i2c_board_info[] = {
>  		.type		= "tps65010",
>  		.irq		= OMAP_GPIO_IRQ(58),
>  	},
> +	{
> +		I2C_BOARD_INFO("isp1301_omap", 0x2d),
> +		.type		= "isp1301_omap",
> +		.irq		= OMAP_GPIO_IRQ(2),
> +	},
>  	/* TODO when driver support is ready:
> -	 *  - isp1301 OTG transceiver
>  	 *  - optional ov9640 camera sensor at 0x30
>  	 *  - pcf9754 for aGPS control
>  	 *  - ... etc
> diff --git a/arch/arm/mach-omap1/board-h3.c b/arch/arm/mach-omap1/board-h3.c
> index 4f84ae2..71e285a 100644
> --- a/arch/arm/mach-omap1/board-h3.c
> +++ b/arch/arm/mach-omap1/board-h3.c
> @@ -463,8 +463,12 @@ static struct i2c_board_info __initdata h3_i2c_board_info[] = {
>  		.type		= "tps65013",
>  		/* .irq		= OMAP_GPIO_IRQ(??), */
>  	},
> +	{
> +		I2C_BOARD_INFO("isp1301_omap", 0x2d),
> +		.type		= "isp1301_omap",
> +		.irq		= OMAP_GPIO_IRQ(14),
> +	},
>  	/* TODO when driver support is ready:
> -	 *  - isp1301 OTG transceiver
>  	 *  - optional ov9640 camera sensor at 0x30
>  	 *  - ...
>  	 */
> diff --git a/arch/arm/mach-omap2/board-h4.c b/arch/arm/mach-omap2/board-h4.c
> index f125f43..33ff80b 100644
> --- a/arch/arm/mach-omap2/board-h4.c
> +++ b/arch/arm/mach-omap2/board-h4.c
> @@ -301,6 +301,15 @@ static struct omap_board_config_kernel h4_config[] = {
>  	{ OMAP_TAG_LCD,		&h4_lcd_config },
>  };
>  
> +static struct i2c_board_info __initdata h4_i2c_board_info[] = {
> +	{
> +		I2C_BOARD_INFO("isp1301_omap", 0x2d),
> +		.type		= "isp1301_omap",
> +		.irq		= OMAP_GPIO_IRQ(125),
> +	},
> +};
> +
> +
>  static void __init omap_h4_init(void)
>  {
>  	/*
> @@ -321,6 +330,9 @@ static void __init omap_h4_init(void)
>  	}
>  #endif
>  
> +	i2c_register_board_info(1, h4_i2c_board_info,
> +			ARRAY_SIZE(h4_i2c_board_info));
> +
>  	platform_add_devices(h4_devices, ARRAY_SIZE(h4_devices));
>  	omap_board_config = h4_config;
>  	omap_board_config_size = ARRAY_SIZE(h4_config);
> diff --git a/drivers/i2c/chips/isp1301_omap.c b/drivers/i2c/chips/isp1301_omap.c
> index 37e1403..c7a7c48 100644
> --- a/drivers/i2c/chips/isp1301_omap.c
> +++ b/drivers/i2c/chips/isp1301_omap.c
> @@ -31,16 +31,12 @@
>  #include <linux/usb/otg.h>
>  #include <linux/i2c.h>
>  #include <linux/workqueue.h>
> -
> -#include <asm/irq.h>
>  #include <asm/arch/usb.h>
>  
> -
>  #ifndef	DEBUG
>  #undef	VERBOSE
>  #endif
>  
> -
>  #define	DRIVER_VERSION	"24 August 2004"
>  #define	DRIVER_NAME	(isp1301_driver.driver.name)
>  
> @@ -50,12 +46,8 @@ MODULE_LICENSE("GPL");
>  struct isp1301 {
>  	struct otg_transceiver	otg;
>  	struct i2c_client	*client;
> -	struct i2c_client	c;
>  	void			(*i2c_release)(struct device *dev);
>  
> -	int			irq;
> -	int			irq_type;
> -
>  	u32			last_otg_ctrl;
>  	unsigned		working:1;
>  
> @@ -140,13 +132,6 @@ static inline void notresponding(struct isp1301 *isp)
>  /*-------------------------------------------------------------------------*/
>  
>  /* only two addresses possible */
> -#define	ISP_BASE		0x2c
> -static unsigned short normal_i2c[] = {
> -	ISP_BASE, ISP_BASE + 1,
> -	I2C_CLIENT_END };
> -
> -I2C_CLIENT_INSMOD;
> -
>  static struct i2c_driver isp1301_driver;
>  
>  /* smbus apis are used for portability */
> @@ -1196,9 +1181,11 @@ static void isp1301_timer(unsigned long _isp)
>  
>  static void isp1301_release(struct device *dev)
>  {
> -	struct isp1301	*isp;
> +	struct i2c_client	*client;
> +	struct isp1301		*isp;
>  
> -	isp = container_of(dev, struct isp1301, c.dev);
> +	client = container_of(dev, struct i2c_client, dev);
> +	isp = i2c_get_clientdata(client);

This seems to be a quite complex way to do:

	isp = i2c_get_drvdata(dev);

Or am I missing something?

>  
>  	/* ugly -- i2c hijacks our memory hook to wait_for_completion() */
>  	if (isp->i2c_release)
> @@ -1208,30 +1195,27 @@ static void isp1301_release(struct device *dev)
>  
>  static struct isp1301 *the_transceiver;
>  
> -static int isp1301_detach_client(struct i2c_client *i2c)
> +static int __exit isp1301_remove(struct i2c_client *client)
>  {
> -	struct isp1301	*isp;
> -
> -	isp = container_of(i2c, struct isp1301, c);
> +	struct isp1301	*isp = i2c_get_clientdata(client);
>  
>  	isp1301_clear_bits(isp, ISP1301_INTERRUPT_FALLING, ~0);
>  	isp1301_clear_bits(isp, ISP1301_INTERRUPT_RISING, ~0);
> -	free_irq(isp->irq, isp);
> +
> +	if (client->irq > 0)
> +		free_irq(client->irq, isp);
>  #ifdef	CONFIG_USB_OTG
>  	otg_unbind(isp);
>  #endif
> -	if (machine_is_omap_h2())
> -		omap_free_gpio(2);
> -

Why?

>  	isp->timer.data = 0;
>  	set_bit(WORK_STOP, &isp->todo);
>  	del_timer_sync(&isp->timer);
>  	flush_scheduled_work();
>  
> -	put_device(&i2c->dev);
> +	put_device(&client->dev);
>  	the_transceiver = 0;
>  
> -	return i2c_detach_client(i2c);
> +	return 0;
>  }
>  
>  /*-------------------------------------------------------------------------*/
> @@ -1301,9 +1285,6 @@ isp1301_set_host(struct otg_transceiver *otg, struct usb_bus *host)
>  
>  	power_up(isp);
>  
> -	if (machine_is_omap_h2())
> -		isp1301_set_bits(isp, ISP1301_MODE_CONTROL_1, MC1_DAT_SE0);
> -

Where has this code gone? Why is it no longer needed?

(Did you test you patch on OMAP H2?)

>  	dev_info(&isp->client->dev, "A-Host sessions ok\n");
>  	isp1301_set_bits(isp, ISP1301_INTERRUPT_RISING,
>  		INTR_ID_GND);
> @@ -1481,11 +1462,10 @@ isp1301_start_hnp(struct otg_transceiver *dev)
>  /*-------------------------------------------------------------------------*/
>  
>  /* no error returns, they'd just make bus scanning stop */
> -static int isp1301_probe(struct i2c_adapter *bus, int address, int kind)
> +static int __init isp1301_probe(struct i2c_client *client)
>  {
>  	int			status;
>  	struct isp1301		*isp;
> -	struct i2c_client	*i2c;
>  
>  	if (the_transceiver)
>  		return 0;
> @@ -1498,46 +1478,35 @@ static int isp1301_probe(struct i2c_adapter *bus, int address, int kind)
>  	init_timer(&isp->timer);
>  	isp->timer.function = isp1301_timer;
>  	isp->timer.data = (unsigned long) isp;
> -
> -	isp->irq = -1;
> -	isp->irq_type = 0;
> -	isp->c.addr = address;
> -	i2c_set_clientdata(&isp->c, isp);
> -	isp->c.adapter = bus;
> -	isp->c.driver = &isp1301_driver;
> -	strlcpy(isp->c.name, DRIVER_NAME, I2C_NAME_SIZE);
> -	isp->client = i2c = &isp->c;
> +	isp->client = client;
>  
>  	/* if this is a true probe, verify the chip ... */
> -	if (kind < 0) {
> -		status = isp1301_get_u16(isp, ISP1301_VENDOR_ID);
> -		if (status != I2C_VENDOR_ID_PHILIPS) {
> -			dev_dbg(&bus->dev, "addr %d not philips id: %d\n",
> -				address, status);
> -			goto fail1;
> -		}
> -		status = isp1301_get_u16(isp, ISP1301_PRODUCT_ID);
> -		if (status != I2C_PRODUCT_ID_PHILIPS_1301) {
> -			dev_dbg(&bus->dev, "%d not isp1301, %d\n",
> -				address, status);
> -			goto fail1;
> -		}
> +	status = isp1301_get_u16(isp, ISP1301_VENDOR_ID);
> +	if (status != I2C_VENDOR_ID_PHILIPS) {
> +		dev_dbg(&client->dev, "not philips id: %d\n",
> +				status);

Fits on a single line.

> +		goto fail1;
> +	}
> +	status = isp1301_get_u16(isp, ISP1301_PRODUCT_ID);
> +	if (status != I2C_PRODUCT_ID_PHILIPS_1301) {
> +		dev_dbg(&client->dev, "not isp1301, %d\n",
> +				status);

Same here.

> +		goto fail1;
>  	}
>  
> -	status = i2c_attach_client(i2c);
>  	if (status < 0) {
> -		dev_dbg(&bus->dev, "can't attach %s to device %d, err %d\n",
> -				DRIVER_NAME, address, status);
> +		dev_dbg(&client->dev, "can't attach %s to device, err %d\n",
> +				DRIVER_NAME, status);

It doesn't make sense to remove the call to i2c_attach_client() but
preserve the status check and debug message!

>  fail1:
>  		kfree(isp);
>  		return 0;
>  	}
> -	isp->i2c_release = i2c->dev.release;
> -	i2c->dev.release = isp1301_release;
> +	isp->i2c_release = client->dev.release;
> +	client->dev.release = isp1301_release;
>  
>  	/* initial development used chiprev 2.00 */
> -	status = i2c_smbus_read_word_data(i2c, ISP1301_BCD_DEVICE);
> -	dev_info(&i2c->dev, "chiprev %x.%02x, driver " DRIVER_VERSION "\n",
> +	status = i2c_smbus_read_word_data(client, ISP1301_BCD_DEVICE);
> +	dev_info(&client->dev, "chiprev %x.%02x, driver " DRIVER_VERSION "\n",
>  		status >> 8, status & 0xff);
>  
>  	/* make like power-on reset */
> @@ -1558,40 +1527,25 @@ fail1:
>  #ifdef	CONFIG_USB_OTG
>  	status = otg_bind(isp);
>  	if (status < 0) {
> -		dev_dbg(&i2c->dev, "can't bind OTG\n");
> +		dev_dbg(&client->dev, "can't bind OTG\n");
>  		goto fail2;
>  	}
>  #endif
>  
> -	if (machine_is_omap_h2()) {
> -		/* full speed signaling by default */
> -		isp1301_set_bits(isp, ISP1301_MODE_CONTROL_1,
> -			MC1_SPEED_REG);
> -		isp1301_set_bits(isp, ISP1301_MODE_CONTROL_2,
> -			MC2_SPD_SUSP_CTRL);
> -
> -		/* IRQ wired at M14 */
> -		omap_cfg_reg(M14_1510_GPIO2);
> -		isp->irq = OMAP_GPIO_IRQ(2);
> -		if (gpio_request(2, "isp1301") == 0)
> -			gpio_direction_input(2);
> -		isp->irq_type = IRQF_TRIGGER_FALLING;
> -	}

Again, why would you remove this code?

> -
> -	isp->irq_type |= IRQF_SAMPLE_RANDOM;
> -	status = request_irq(isp->irq, isp1301_irq,
> -			isp->irq_type, DRIVER_NAME, isp);
> +	status = request_irq(client->irq, isp1301_irq,
> +			IRQF_SAMPLE_RANDOM | IRQF_TRIGGER_FALLING,
> +			DRIVER_NAME, isp);

When freeing the irq you first test that client->irq > 0, but when
requesting it you do not? It's inconsistent.

Also, the original code was passing different IRQF flags depending on
the platform, your changes force the same mode for everyone. This
doesn't look correct.

>  	if (status < 0) {
> -		dev_dbg(&i2c->dev, "can't get IRQ %d, err %d\n",
> -				isp->irq, status);
> +		dev_dbg(&client->dev, "can't get IRQ %d, err %d\n",
> +				client->irq, status);
>  #ifdef	CONFIG_USB_OTG
>  fail2:
>  #endif
> -		i2c_detach_client(i2c);
> +		i2c_detach_client(client);
>  		goto fail1;
>  	}
>  
> -	isp->otg.dev = &isp->client->dev;
> +	isp->otg.dev = &client->dev;
>  	isp->otg.label = DRIVER_NAME;
>  
>  	isp->otg.set_host = isp1301_set_host,
> @@ -1608,43 +1562,42 @@ fail2:
>  	update_otg1(isp, isp1301_get_u8(isp, ISP1301_INTERRUPT_SOURCE));
>  	update_otg2(isp, isp1301_get_u8(isp, ISP1301_OTG_STATUS));
>  #endif
> -

Noise, please revert.

>  	dump_regs(isp, __FUNCTION__);
>  
>  #ifdef	VERBOSE
>  	mod_timer(&isp->timer, jiffies + TIMER_JIFFIES);
> -	dev_dbg(&i2c->dev, "scheduled timer, %d min\n", TIMER_MINUTES);
> +	dev_dbg(&client->dev, "scheduled timer, %d min\n", TIMER_MINUTES);
>  #endif
>  
>  	status = otg_set_transceiver(&isp->otg);
>  	if (status < 0)
> -		dev_err(&i2c->dev, "can't register transceiver, %d\n",
> +		dev_err(&client->dev, "can't register transceiver, %d\n",
>  			status);
>  
>  	return 0;
>  }
>  
> -static int isp1301_scan_bus(struct i2c_adapter *bus)
> -{
> -	if (!i2c_check_functionality(bus, I2C_FUNC_SMBUS_BYTE_DATA
> -			| I2C_FUNC_SMBUS_READ_WORD_DATA))
> -		return -EINVAL;
> -	return i2c_probe(bus, &addr_data, isp1301_probe);
> -}
> -
>  static struct i2c_driver isp1301_driver = {
>  	.driver = {
>  		.name	= "isp1301_omap",
>  	},
> -	.attach_adapter	= isp1301_scan_bus,
> -	.detach_client	= isp1301_detach_client,
> +	.probe	= isp1301_probe,
> +	.remove	= __exit_p(isp1301_remove),
>  };
>  
>  /*-------------------------------------------------------------------------*/
>  
>  static int __init isp_init(void)
>  {
> -	return i2c_add_driver(&isp1301_driver);
> +	int	status = -ENODEV;
> +
> +	printk(KERN_INFO "%s: version %s\n", DRIVER_NAME, DRIVER_VERSION);

What's the point of printing a driver version that nobody bothers
updating?

Most i2c chip drivers keep quiet when loaded, they print a message when
a device is actually found, as this driver is doing.

> +
> +	status = i2c_add_driver(&isp1301_driver);
> +	if (status)
> +		printk(KERN_ERR "%s failed to probe\n", DRIVER_NAME);

This is extremely unlikely to happen, and if it did, i2c-core would
already log a more informative error message, and insmod/modprobe as
well. So you can just call i2c_add_driver() and be done with it (as
the driver was originally doing.)

> +
> +	return status;
>  }
>  module_init(isp_init);
>  


-- 
Jean Delvare

Re: [PATCH 2/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 2

[Felipe Balbi]

Hi,

On Jan 22, 2008 2:01 PM, Jean Delvare <khali@linux-fr.org> wrote:
> Hi Felipe,
>
>
> On Thu,  3 Jan 2008 11:59:57 -0500, Felipe Balbi wrote:
> > Based on David Brownell's patch for tps65010, this patch
> > finish conversting isp1301_omap.c to new-style i2c driver.
> >
> > Signed-off-by: Felipe Balbi <me@felipebalbi.com>
> > ---
> >  arch/arm/mach-omap1/board-h2.c   |    6 ++-
> >  arch/arm/mach-omap1/board-h3.c   |    6 ++-
> >  arch/arm/mach-omap2/board-h4.c   |   12 +++
> >  drivers/i2c/chips/isp1301_omap.c |  149 +++++++++++++-------------------------
> >  4 files changed, 73 insertions(+), 100 deletions(-)
> >
> > diff --git a/arch/arm/mach-omap1/board-h2.c b/arch/arm/mach-omap1/board-h2.c
> > index 1306812..0307f50 100644
> > --- a/arch/arm/mach-omap1/board-h2.c
> > +++ b/arch/arm/mach-omap1/board-h2.c
> > @@ -350,8 +350,12 @@ static struct i2c_board_info __initdata h2_i2c_board_info[] = {
> >               .type           = "tps65010",
> >               .irq            = OMAP_GPIO_IRQ(58),
> >       },
> > +     {
> > +             I2C_BOARD_INFO("isp1301_omap", 0x2d),
> > +             .type           = "isp1301_omap",
> > +             .irq            = OMAP_GPIO_IRQ(2),
> > +     },
> >       /* TODO when driver support is ready:
> > -      *  - isp1301 OTG transceiver
> >        *  - optional ov9640 camera sensor at 0x30
> >        *  - pcf9754 for aGPS control
> >        *  - ... etc
> > diff --git a/arch/arm/mach-omap1/board-h3.c b/arch/arm/mach-omap1/board-h3.c
> > index 4f84ae2..71e285a 100644
> > --- a/arch/arm/mach-omap1/board-h3.c
> > +++ b/arch/arm/mach-omap1/board-h3.c
> > @@ -463,8 +463,12 @@ static struct i2c_board_info __initdata h3_i2c_board_info[] = {
> >               .type           = "tps65013",
> >               /* .irq         = OMAP_GPIO_IRQ(??), */
> >       },
> > +     {
> > +             I2C_BOARD_INFO("isp1301_omap", 0x2d),
> > +             .type           = "isp1301_omap",
> > +             .irq            = OMAP_GPIO_IRQ(14),
> > +     },
> >       /* TODO when driver support is ready:
> > -      *  - isp1301 OTG transceiver
> >        *  - optional ov9640 camera sensor at 0x30
> >        *  - ...
> >        */
> > diff --git a/arch/arm/mach-omap2/board-h4.c b/arch/arm/mach-omap2/board-h4.c
> > index f125f43..33ff80b 100644
> > --- a/arch/arm/mach-omap2/board-h4.c
> > +++ b/arch/arm/mach-omap2/board-h4.c
> > @@ -301,6 +301,15 @@ static struct omap_board_config_kernel h4_config[] = {
> >       { OMAP_TAG_LCD,         &h4_lcd_config },
> >  };
> >
> > +static struct i2c_board_info __initdata h4_i2c_board_info[] = {
> > +     {
> > +             I2C_BOARD_INFO("isp1301_omap", 0x2d),
> > +             .type           = "isp1301_omap",
> > +             .irq            = OMAP_GPIO_IRQ(125),
> > +     },
> > +};
> > +
> > +
> >  static void __init omap_h4_init(void)
> >  {
> >       /*
> > @@ -321,6 +330,9 @@ static void __init omap_h4_init(void)
> >       }
> >  #endif
> >
> > +     i2c_register_board_info(1, h4_i2c_board_info,
> > +                     ARRAY_SIZE(h4_i2c_board_info));
> > +
> >       platform_add_devices(h4_devices, ARRAY_SIZE(h4_devices));
> >       omap_board_config = h4_config;
> >       omap_board_config_size = ARRAY_SIZE(h4_config);
> > diff --git a/drivers/i2c/chips/isp1301_omap.c b/drivers/i2c/chips/isp1301_omap.c
> > index 37e1403..c7a7c48 100644
> > --- a/drivers/i2c/chips/isp1301_omap.c
> > +++ b/drivers/i2c/chips/isp1301_omap.c
> > @@ -31,16 +31,12 @@
> >  #include <linux/usb/otg.h>
> >  #include <linux/i2c.h>
> >  #include <linux/workqueue.h>
> > -
> > -#include <asm/irq.h>
> >  #include <asm/arch/usb.h>
> >
> > -
> >  #ifndef      DEBUG
> >  #undef       VERBOSE
> >  #endif
> >
> > -
> >  #define      DRIVER_VERSION  "24 August 2004"
> >  #define      DRIVER_NAME     (isp1301_driver.driver.name)
> >
> > @@ -50,12 +46,8 @@ MODULE_LICENSE("GPL");
> >  struct isp1301 {
> >       struct otg_transceiver  otg;
> >       struct i2c_client       *client;
> > -     struct i2c_client       c;
> >       void                    (*i2c_release)(struct device *dev);
> >
> > -     int                     irq;
> > -     int                     irq_type;
> > -
> >       u32                     last_otg_ctrl;
> >       unsigned                working:1;
> >
> > @@ -140,13 +132,6 @@ static inline void notresponding(struct isp1301 *isp)
> >  /*-------------------------------------------------------------------------*/
> >
> >  /* only two addresses possible */
> > -#define      ISP_BASE                0x2c
> > -static unsigned short normal_i2c[] = {
> > -     ISP_BASE, ISP_BASE + 1,
> > -     I2C_CLIENT_END };
> > -
> > -I2C_CLIENT_INSMOD;
> > -
> >  static struct i2c_driver isp1301_driver;
> >
> >  /* smbus apis are used for portability */
> > @@ -1196,9 +1181,11 @@ static void isp1301_timer(unsigned long _isp)
> >
> >  static void isp1301_release(struct device *dev)
> >  {
> > -     struct isp1301  *isp;
> > +     struct i2c_client       *client;
> > +     struct isp1301          *isp;
> >
> > -     isp = container_of(dev, struct isp1301, c.dev);
> > +     client = container_of(dev, struct i2c_client, dev);
> > +     isp = i2c_get_clientdata(client);
>
> This seems to be a quite complex way to do:
>
>         isp = i2c_get_drvdata(dev);
>
> Or am I missing something?

My bad, thanks for getting this.

>
> >
> >       /* ugly -- i2c hijacks our memory hook to wait_for_completion() */
> >       if (isp->i2c_release)
> > @@ -1208,30 +1195,27 @@ static void isp1301_release(struct device *dev)
> >
> >  static struct isp1301 *the_transceiver;
> >
> > -static int isp1301_detach_client(struct i2c_client *i2c)
> > +static int __exit isp1301_remove(struct i2c_client *client)
> >  {
> > -     struct isp1301  *isp;
> > -
> > -     isp = container_of(i2c, struct isp1301, c);
> > +     struct isp1301  *isp = i2c_get_clientdata(client);
> >
> >       isp1301_clear_bits(isp, ISP1301_INTERRUPT_FALLING, ~0);
> >       isp1301_clear_bits(isp, ISP1301_INTERRUPT_RISING, ~0);
> > -     free_irq(isp->irq, isp);
> > +
> > +     if (client->irq > 0)
> > +             free_irq(client->irq, isp);
> >  #ifdef       CONFIG_USB_OTG
> >       otg_unbind(isp);
> >  #endif
> > -     if (machine_is_omap_h2())
> > -             omap_free_gpio(2);
> > -
>
> Why?

Board specific code should go to board files. I'll try to find a
better way of doing this. Maybe using a private_data.
Ditto to all cases below.

>
> >       isp->timer.data = 0;
> >       set_bit(WORK_STOP, &isp->todo);
> >       del_timer_sync(&isp->timer);
> >       flush_scheduled_work();
> >
> > -     put_device(&i2c->dev);
> > +     put_device(&client->dev);
> >       the_transceiver = 0;
> >
> > -     return i2c_detach_client(i2c);
> > +     return 0;
> >  }
> >
> >  /*-------------------------------------------------------------------------*/
> > @@ -1301,9 +1285,6 @@ isp1301_set_host(struct otg_transceiver *otg, struct usb_bus *host)
> >
> >       power_up(isp);
> >
> > -     if (machine_is_omap_h2())
> > -             isp1301_set_bits(isp, ISP1301_MODE_CONTROL_1, MC1_DAT_SE0);
> > -
>
> Where has this code gone? Why is it no longer needed?
>
> (Did you test you patch on OMAP H2?)

Can't remember anymore, but before applying these patches isp1301 was
crashing the kernel on H2 and H3.

>
>
> >       dev_info(&isp->client->dev, "A-Host sessions ok\n");
> >       isp1301_set_bits(isp, ISP1301_INTERRUPT_RISING,
> >               INTR_ID_GND);
> > @@ -1481,11 +1462,10 @@ isp1301_start_hnp(struct otg_transceiver *dev)
> >  /*-------------------------------------------------------------------------*/
> >
> >  /* no error returns, they'd just make bus scanning stop */
> > -static int isp1301_probe(struct i2c_adapter *bus, int address, int kind)
> > +static int __init isp1301_probe(struct i2c_client *client)
> >  {
> >       int                     status;
> >       struct isp1301          *isp;
> > -     struct i2c_client       *i2c;
> >
> >       if (the_transceiver)
> >               return 0;
> > @@ -1498,46 +1478,35 @@ static int isp1301_probe(struct i2c_adapter *bus, int address, int kind)
> >       init_timer(&isp->timer);
> >       isp->timer.function = isp1301_timer;
> >       isp->timer.data = (unsigned long) isp;
> > -
> > -     isp->irq = -1;
> > -     isp->irq_type = 0;
> > -     isp->c.addr = address;
> > -     i2c_set_clientdata(&isp->c, isp);
> > -     isp->c.adapter = bus;
> > -     isp->c.driver = &isp1301_driver;
> > -     strlcpy(isp->c.name, DRIVER_NAME, I2C_NAME_SIZE);
> > -     isp->client = i2c = &isp->c;
> > +     isp->client = client;
> >
> >       /* if this is a true probe, verify the chip ... */
> > -     if (kind < 0) {
> > -             status = isp1301_get_u16(isp, ISP1301_VENDOR_ID);
> > -             if (status != I2C_VENDOR_ID_PHILIPS) {
> > -                     dev_dbg(&bus->dev, "addr %d not philips id: %d\n",
> > -                             address, status);
> > -                     goto fail1;
> > -             }
> > -             status = isp1301_get_u16(isp, ISP1301_PRODUCT_ID);
> > -             if (status != I2C_PRODUCT_ID_PHILIPS_1301) {
> > -                     dev_dbg(&bus->dev, "%d not isp1301, %d\n",
> > -                             address, status);
> > -                     goto fail1;
> > -             }
> > +     status = isp1301_get_u16(isp, ISP1301_VENDOR_ID);
> > +     if (status != I2C_VENDOR_ID_PHILIPS) {
> > +             dev_dbg(&client->dev, "not philips id: %d\n",
> > +                             status);
>
> Fits on a single line.

Good catch. Changing today.

>
> > +             goto fail1;
> > +     }
> > +     status = isp1301_get_u16(isp, ISP1301_PRODUCT_ID);
> > +     if (status != I2C_PRODUCT_ID_PHILIPS_1301) {
> > +             dev_dbg(&client->dev, "not isp1301, %d\n",
> > +                             status);
>
> Same here.
>
> > +             goto fail1;
> >       }
> >
> > -     status = i2c_attach_client(i2c);
> >       if (status < 0) {
> > -             dev_dbg(&bus->dev, "can't attach %s to device %d, err %d\n",
> > -                             DRIVER_NAME, address, status);
> > +             dev_dbg(&client->dev, "can't attach %s to device, err %d\n",
> > +                             DRIVER_NAME, status);
>
> It doesn't make sense to remove the call to i2c_attach_client() but
> preserve the status check and debug message!

Hmm... sorry for that.
Changing.

>
>
> >  fail1:
> >               kfree(isp);
> >               return 0;
> >       }
> > -     isp->i2c_release = i2c->dev.release;
> > -     i2c->dev.release = isp1301_release;
> > +     isp->i2c_release = client->dev.release;
> > +     client->dev.release = isp1301_release;
> >
> >       /* initial development used chiprev 2.00 */
> > -     status = i2c_smbus_read_word_data(i2c, ISP1301_BCD_DEVICE);
> > -     dev_info(&i2c->dev, "chiprev %x.%02x, driver " DRIVER_VERSION "\n",
> > +     status = i2c_smbus_read_word_data(client, ISP1301_BCD_DEVICE);
> > +     dev_info(&client->dev, "chiprev %x.%02x, driver " DRIVER_VERSION "\n",
> >               status >> 8, status & 0xff);
> >
> >       /* make like power-on reset */
> > @@ -1558,40 +1527,25 @@ fail1:
> >  #ifdef       CONFIG_USB_OTG
> >       status = otg_bind(isp);
> >       if (status < 0) {
> > -             dev_dbg(&i2c->dev, "can't bind OTG\n");
> > +             dev_dbg(&client->dev, "can't bind OTG\n");
> >               goto fail2;
> >       }
> >  #endif
> >
> > -     if (machine_is_omap_h2()) {
> > -             /* full speed signaling by default */
> > -             isp1301_set_bits(isp, ISP1301_MODE_CONTROL_1,
> > -                     MC1_SPEED_REG);
> > -             isp1301_set_bits(isp, ISP1301_MODE_CONTROL_2,
> > -                     MC2_SPD_SUSP_CTRL);
> > -
> > -             /* IRQ wired at M14 */
> > -             omap_cfg_reg(M14_1510_GPIO2);
> > -             isp->irq = OMAP_GPIO_IRQ(2);
> > -             if (gpio_request(2, "isp1301") == 0)
> > -                     gpio_direction_input(2);
> > -             isp->irq_type = IRQF_TRIGGER_FALLING;
> > -     }
>
> Again, why would you remove this code?
>
> > -
> > -     isp->irq_type |= IRQF_SAMPLE_RANDOM;
> > -     status = request_irq(isp->irq, isp1301_irq,
> > -                     isp->irq_type, DRIVER_NAME, isp);
> > +     status = request_irq(client->irq, isp1301_irq,
> > +                     IRQF_SAMPLE_RANDOM | IRQF_TRIGGER_FALLING,
> > +                     DRIVER_NAME, isp);
>
> When freeing the irq you first test that client->irq > 0, but when
> requesting it you do not? It's inconsistent.

I'll fix it.

>
> Also, the original code was passing different IRQF flags depending on
> the platform, your changes force the same mode for everyone. This
> doesn't look correct.

Maybe one other thing to come from a private_data.

>
> >       if (status < 0) {
> > -             dev_dbg(&i2c->dev, "can't get IRQ %d, err %d\n",
> > -                             isp->irq, status);
> > +             dev_dbg(&client->dev, "can't get IRQ %d, err %d\n",
> > +                             client->irq, status);
> >  #ifdef       CONFIG_USB_OTG
> >  fail2:
> >  #endif
> > -             i2c_detach_client(i2c);
> > +             i2c_detach_client(client);
> >               goto fail1;
> >       }
> >
> > -     isp->otg.dev = &isp->client->dev;
> > +     isp->otg.dev = &client->dev;
> >       isp->otg.label = DRIVER_NAME;
> >
> >       isp->otg.set_host = isp1301_set_host,
> > @@ -1608,43 +1562,42 @@ fail2:
> >       update_otg1(isp, isp1301_get_u8(isp, ISP1301_INTERRUPT_SOURCE));
> >       update_otg2(isp, isp1301_get_u8(isp, ISP1301_OTG_STATUS));
> >  #endif
> > -
>
> Noise, please revert.

Reverting

>
>
> >       dump_regs(isp, __FUNCTION__);
> >
> >  #ifdef       VERBOSE
> >       mod_timer(&isp->timer, jiffies + TIMER_JIFFIES);
> > -     dev_dbg(&i2c->dev, "scheduled timer, %d min\n", TIMER_MINUTES);
> > +     dev_dbg(&client->dev, "scheduled timer, %d min\n", TIMER_MINUTES);
> >  #endif
> >
> >       status = otg_set_transceiver(&isp->otg);
> >       if (status < 0)
> > -             dev_err(&i2c->dev, "can't register transceiver, %d\n",
> > +             dev_err(&client->dev, "can't register transceiver, %d\n",
> >                       status);
> >
> >       return 0;
> >  }
> >
> > -static int isp1301_scan_bus(struct i2c_adapter *bus)
> > -{
> > -     if (!i2c_check_functionality(bus, I2C_FUNC_SMBUS_BYTE_DATA
> > -                     | I2C_FUNC_SMBUS_READ_WORD_DATA))
> > -             return -EINVAL;
> > -     return i2c_probe(bus, &addr_data, isp1301_probe);
> > -}
> > -
> >  static struct i2c_driver isp1301_driver = {
> >       .driver = {
> >               .name   = "isp1301_omap",
> >       },
> > -     .attach_adapter = isp1301_scan_bus,
> > -     .detach_client  = isp1301_detach_client,
> > +     .probe  = isp1301_probe,
> > +     .remove = __exit_p(isp1301_remove),
> >  };
> >
> >  /*-------------------------------------------------------------------------*/
> >
> >  static int __init isp_init(void)
> >  {
> > -     return i2c_add_driver(&isp1301_driver);
> > +     int     status = -ENODEV;
> > +
> > +     printk(KERN_INFO "%s: version %s\n", DRIVER_NAME, DRIVER_VERSION);
>
> What's the point of printing a driver version that nobody bothers
> updating?
>
> Most i2c chip drivers keep quiet when loaded, they print a message when
> a device is actually found, as this driver is doing.

Ok. Changing

>
> > +
> > +     status = i2c_add_driver(&isp1301_driver);
> > +     if (status)
> > +             printk(KERN_ERR "%s failed to probe\n", DRIVER_NAME);
>
> This is extremely unlikely to happen, and if it did, i2c-core would
> already log a more informative error message, and insmod/modprobe as
> well. So you can just call i2c_add_driver() and be done with it (as
> the driver was originally doing.)

Ok. Changing


-- 
Best Regards,

Felipe Balbi
felipebalbi@users.sourceforge.net

Re: [PATCH 2/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 2

[Jean Delvare]

On Tue, 22 Jan 2008 14:13:58 +0200, Felipe Balbi wrote:
> On Jan 22, 2008 2:01 PM, Jean Delvare <khali@linux-fr.org> wrote:
> > On Thu,  3 Jan 2008 11:59:57 -0500, Felipe Balbi wrote:
> > > -     if (machine_is_omap_h2())
> > > -             omap_free_gpio(2);
> > > -
> >
> > Why?
> 
> Board specific code should go to board files. I'll try to find a
> better way of doing this. Maybe using a private_data.
> Ditto to all cases below.

I agree that board code should ideally not be there, however you can't
just drop it and hope nobody will notice. It needs to be done in such a
way that everything still works afterwards. IMHO it is better move such
changes to a later patch so as to not make this one needlessly complex.

> > (Did you test you patch on OMAP H2?)
> 
> Can't remember anymore, but before applying these patches isp1301 was
> crashing the kernel on H2 and H3.

That's bad, this should be fixed, if possible even before applying your
patches.

Thanks,
-- 
Jean Delvare

Re: [PATCH 1/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 1

[Jean Delvare]

Hi Felipe,

On Thu,  3 Jan 2008 11:59:56 -0500, Felipe Balbi wrote:
> Based on David Brownell's patch for tps65010, this patch
> starts converting isp1301_omap.c to new-style i2c driver.
> 
> Signed-off-by: Felipe Balbi <me@felipebalbi.com>
> ---
>  drivers/i2c/chips/isp1301_omap.c |   60 +++++++++++++++++++------------------
>  1 files changed, 31 insertions(+), 29 deletions(-)
> 

Next time, please send this patch to the i2c mailing list instead of
LKML if you want to get some attention.

I'm fine with this patch except for:

> diff --git a/drivers/i2c/chips/isp1301_omap.c b/drivers/i2c/chips/isp1301_omap.c
> index b767603..37e1403 100644
> --- a/drivers/i2c/chips/isp1301_omap.c
> +++ b/drivers/i2c/chips/isp1301_omap.c
> (...)
> @@ -1499,12 +1500,13 @@ static int isp1301_probe(struct i2c_adapter *bus, int address, int kind)
>  	isp->timer.data = (unsigned long) isp;
>  
>  	isp->irq = -1;
> -	isp->client.addr = address;
> -	i2c_set_clientdata(&isp->client, isp);
> -	isp->client.adapter = bus;
> -	isp->client.driver = &isp1301_driver;
> -	strlcpy(isp->client.name, DRIVER_NAME, I2C_NAME_SIZE);
> -	i2c = &isp->client;
> +	isp->irq_type = 0;

The structure is initialized by kzalloc() so no need to explicitly set
this field to 0.

> +	isp->c.addr = address;
> +	i2c_set_clientdata(&isp->c, isp);
> +	isp->c.adapter = bus;
> +	isp->c.driver = &isp1301_driver;
> +	strlcpy(isp->c.name, DRIVER_NAME, I2C_NAME_SIZE);
> +	isp->client = i2c = &isp->c;
>  
>  	/* if this is a true probe, verify the chip ... */
>  	if (kind < 0) {

I'll change it myself, no need to resend.

-- 
Jean Delvare

Re: [PATCH 1/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 1

[Felipe Balbi]

On Jan 21, 2008 7:15 PM, Jean Delvare <khali@linux-fr.org> wrote:
> Hi Felipe,
>
> On Thu,  3 Jan 2008 11:59:56 -0500, Felipe Balbi wrote:
> > Based on David Brownell's patch for tps65010, this patch
> > starts converting isp1301_omap.c to new-style i2c driver.
> >
> > Signed-off-by: Felipe Balbi <me@felipebalbi.com>
> > ---
> >  drivers/i2c/chips/isp1301_omap.c |   60 +++++++++++++++++++------------------
> >  1 files changed, 31 insertions(+), 29 deletions(-)
> >
>
> Next time, please send this patch to the i2c mailing list instead of
> LKML if you want to get some attention.

Ok, i'll do it.
Sorry for missing i2c mailing list.

-- 
Best Regards,

Felipe Balbi
felipebalbi@users.sourceforge.net