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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
On Jan 21, 2008 7:15 PM, Jean Delvare <[email protected]> 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 <[email protected]>
> > ---
> > 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
[email protected]
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 <[email protected]>
> ---
> 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
Hi,
On Jan 22, 2008 2:01 PM, Jean Delvare <[email protected]> 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 <[email protected]>
> > ---
> > 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
[email protected]
On Tue, 22 Jan 2008 14:13:58 +0200, Felipe Balbi wrote:
> On Jan 22, 2008 2:01 PM, Jean Delvare <[email protected]> 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