* [PATCH 0/2] ISP1301 updates @ 2008-01-03 16:59 Felipe Balbi 2008-01-03 16:59 ` [PATCH 1/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 1 Felipe Balbi 0 siblings, 1 reply; 39+ messages in thread From: Felipe Balbi @ 2008-01-03 16:59 UTC (permalink / raw) To: linux-kernel; +Cc: david-b, tony 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 ^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 1/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 1 2008-01-03 16:59 [PATCH 0/2] ISP1301 updates Felipe Balbi @ 2008-01-03 16:59 ` Felipe Balbi 2008-01-03 16:59 ` [PATCH 2/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 2 Felipe Balbi 2008-01-21 17:15 ` [PATCH 1/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 1 Jean Delvare 0 siblings, 2 replies; 39+ messages in thread From: Felipe Balbi @ 2008-01-03 16:59 UTC (permalink / raw) To: linux-kernel; +Cc: david-b, tony, 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 ^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH 2/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 2 2008-01-03 16:59 ` [PATCH 1/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 1 Felipe Balbi @ 2008-01-03 16:59 ` Felipe Balbi 2008-01-22 12:01 ` Jean Delvare 2008-01-21 17:15 ` [PATCH 1/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 1 Jean Delvare 1 sibling, 1 reply; 39+ messages in thread From: Felipe Balbi @ 2008-01-03 16:59 UTC (permalink / raw) To: linux-kernel; +Cc: david-b, tony, 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 ^ permalink raw reply related [flat|nested] 39+ messages in thread
* Re: [PATCH 2/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 2 2008-01-03 16:59 ` [PATCH 2/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 2 Felipe Balbi @ 2008-01-22 12:01 ` Jean Delvare [not found] ` <20080122130158.4d4d44dc-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org> 0 siblings, 1 reply; 39+ messages in thread From: Jean Delvare @ 2008-01-22 12:01 UTC (permalink / raw) To: Felipe Balbi; +Cc: linux-kernel, david-b, tony, Linux I2C 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 ^ permalink raw reply [flat|nested] 39+ messages in thread
[parent not found: <20080122130158.4d4d44dc-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>]
* Re: [PATCH 2/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 2 2008-01-22 12:01 ` Jean Delvare @ 2008-01-22 12:13 ` Felipe Balbi 0 siblings, 0 replies; 39+ messages in thread From: Felipe Balbi @ 2008-01-22 12:13 UTC (permalink / raw) To: Jean Delvare Cc: david-b-yBeKhBN/0LDR7s880joybQ, tony-4v6yS6AI5VpBDgjK7y7TUQ, Felipe Balbi, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Linux I2C Hi, On Jan 22, 2008 2:01 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.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-uiRdBs8odbtmTBlB0Cgj/Q@public.gmane.org> > > --- > > 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-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 2/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 2 @ 2008-01-22 12:13 ` Felipe Balbi 0 siblings, 0 replies; 39+ messages in thread From: Felipe Balbi @ 2008-01-22 12:13 UTC (permalink / raw) To: Jean Delvare; +Cc: Felipe Balbi, linux-kernel, david-b, tony, Linux I2C 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 ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 2/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 2 2008-01-22 12:13 ` Felipe Balbi (?) @ 2008-01-22 17:56 ` Jean Delvare [not found] ` <20080122185615.4a8ebbea-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org> -1 siblings, 1 reply; 39+ messages in thread From: Jean Delvare @ 2008-01-22 17:56 UTC (permalink / raw) To: Felipe Balbi; +Cc: linux-kernel, david-b, tony, Linux I2C 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 ^ permalink raw reply [flat|nested] 39+ messages in thread
[parent not found: <20080122185615.4a8ebbea-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>]
* Re: [PATCH 2/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 2 [not found] ` <20080122185615.4a8ebbea-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org> @ 2008-02-23 16:52 ` Jean Delvare [not found] ` <20080223175233.7d2f358e-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org> 0 siblings, 1 reply; 39+ messages in thread From: Jean Delvare @ 2008-02-23 16:52 UTC (permalink / raw) To: Felipe Balbi Cc: david-b-yBeKhBN/0LDR7s880joybQ, tony-4v6yS6AI5VpBDgjK7y7TUQ, Linux I2C Hi Felipe, Any update on this patch? -- Jean Delvare _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply [flat|nested] 39+ messages in thread
[parent not found: <20080223175233.7d2f358e-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>]
* Re: [PATCH 2/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 2 [not found] ` <20080223175233.7d2f358e-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org> @ 2008-02-23 17:01 ` Felipe Balbi [not found] ` <31e679430802230901j66e17535w6ac8ec57e60e5c6b-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 0 siblings, 1 reply; 39+ messages in thread From: Felipe Balbi @ 2008-02-23 17:01 UTC (permalink / raw) To: Jean Delvare Cc: david-b-yBeKhBN/0LDR7s880joybQ, tony-4v6yS6AI5VpBDgjK7y7TUQ, Linux I2C On Sat, Feb 23, 2008 at 6:52 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote: > Hi Felipe, > > Any update on this patch? Hi Jean, sorry the delay... I'm a bit crowded on omap3 usb support but I'll try to make it work during the next week. BTW, I don't have access to omap 1610 nor 1710. Do you anyone who could help me testing the patch ? > > -- > Jean Delvare > -- Best Regards, Felipe Balbi felipebalbi-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply [flat|nested] 39+ messages in thread
[parent not found: <31e679430802230901j66e17535w6ac8ec57e60e5c6b-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH 2/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 2 [not found] ` <31e679430802230901j66e17535w6ac8ec57e60e5c6b-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2008-02-23 17:11 ` Jean Delvare [not found] ` <20080223181120.30e9f63a-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org> 0 siblings, 1 reply; 39+ messages in thread From: Jean Delvare @ 2008-02-23 17:11 UTC (permalink / raw) To: Felipe Balbi Cc: david-b-yBeKhBN/0LDR7s880joybQ, tony-4v6yS6AI5VpBDgjK7y7TUQ, Deepak Saxena, Linux I2C On Sat, 23 Feb 2008 19:01:22 +0200, Felipe Balbi wrote: > On Sat, Feb 23, 2008 at 6:52 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote: > > Hi Felipe, > > > > Any update on this patch? > > Hi Jean, > > sorry the delay... > I'm a bit crowded on omap3 usb support but I'll try to make it work > during the next week. BTW, I don't have access to omap 1610 nor 1710. > Do you anyone who could help me testing the patch ? No, I'm not into embedded at all, sorry. Maybe Deepak (Cc'd) can help. Otherwise try the linux-omap mailing list. -- Jean Delvare _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply [flat|nested] 39+ messages in thread
[parent not found: <20080223181120.30e9f63a-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>]
* Re: [PATCH 2/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 2 [not found] ` <20080223181120.30e9f63a-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org> @ 2008-02-23 17:13 ` Felipe Balbi [not found] ` <31e679430802230913m33d8457x1760399183c56ced-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 0 siblings, 1 reply; 39+ messages in thread From: Felipe Balbi @ 2008-02-23 17:13 UTC (permalink / raw) To: Jean Delvare Cc: david-b-yBeKhBN/0LDR7s880joybQ, tony-4v6yS6AI5VpBDgjK7y7TUQ, Deepak Saxena, Linux I2C On Sat, Feb 23, 2008 at 7:11 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote: > On Sat, 23 Feb 2008 19:01:22 +0200, Felipe Balbi wrote: > > On Sat, Feb 23, 2008 at 6:52 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote: > > > Hi Felipe, > > > > > > Any update on this patch? > > > > Hi Jean, > > > > sorry the delay... > > I'm a bit crowded on omap3 usb support but I'll try to make it work > > during the next week. BTW, I don't have access to omap 1610 nor 1710. > > Do you anyone who could help me testing the patch ? > > No, I'm not into embedded at all, sorry. Maybe Deepak (Cc'd) can help. > Otherwise try the linux-omap mailing list. Maybe, Dave do you have an H2 to try my isp patch?? :-p > > -- > Jean Delvare > -- Best Regards, Felipe Balbi felipebalbi-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply [flat|nested] 39+ messages in thread
[parent not found: <31e679430802230913m33d8457x1760399183c56ced-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH 2/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 2 [not found] ` <31e679430802230913m33d8457x1760399183c56ced-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2008-02-23 17:53 ` David Brownell [not found] ` <20080223175356.95681BA69D-ZcXrCSuhvln6VZ3dlLfH/g4gEjPzgfUyLrfjE7I9kuVHxeISYlDBzl6hYfS7NtTn@public.gmane.org> 0 siblings, 1 reply; 39+ messages in thread From: David Brownell @ 2008-02-23 17:53 UTC (permalink / raw) To: khali-PUYAD+kWke1g9hUCZPvPmw, felipebalbi-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f Cc: tony-4v6yS6AI5VpBDgjK7y7TUQ, dsaxena-k7pgMgclrJvR7s880joybQ, i2c-GZX6beZjE8VD60Wz+7aTrA > Maybe, Dave do you have an H2 to try my isp patch?? :-p I have one boxed up, but I'm a bit short on time and table space to do anything with it. Unboxing it for some stuff is on the agenda for some point, but there's a bunch of other stuff ahead of it in the queue ... Why don't you forward me the patch in question, and if I get time to do that H2 stuff before you do, you'll see the results. :) - Dave _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply [flat|nested] 39+ messages in thread
[parent not found: <20080223175356.95681BA69D-ZcXrCSuhvln6VZ3dlLfH/g4gEjPzgfUyLrfjE7I9kuVHxeISYlDBzl6hYfS7NtTn@public.gmane.org>]
* Re: [PATCH 2/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 2 [not found] ` <20080223175356.95681BA69D-ZcXrCSuhvln6VZ3dlLfH/g4gEjPzgfUyLrfjE7I9kuVHxeISYlDBzl6hYfS7NtTn@public.gmane.org> @ 2008-02-23 18:00 ` Felipe Balbi [not found] ` <31e679430802231000i4a360269v5d0bc61fc21fd7c3-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 0 siblings, 1 reply; 39+ messages in thread From: Felipe Balbi @ 2008-02-23 18:00 UTC (permalink / raw) To: David Brownell Cc: tony-4v6yS6AI5VpBDgjK7y7TUQ, dsaxena-k7pgMgclrJvR7s880joybQ, i2c-GZX6beZjE8VD60Wz+7aTrA [-- Attachment #1: Type: text/plain, Size: 806 bytes --] On Sat, Feb 23, 2008 at 7:53 PM, David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote: > > Maybe, Dave do you have an H2 to try my isp patch?? :-p > > I have one boxed up, but I'm a bit short on time and table space to > do anything with it. Unboxing it for some stuff is on the agenda > for some point, but there's a bunch of other stuff ahead of it in > the queue ... > > Why don't you forward me the patch in question, and if I get time > to do that H2 stuff before you do, you'll see the results. :) Thanks Dave, the patch is attached. patch 1 is acked by Jean, right Jean? and patch2 needs some rework, to add some platform_data to get irq and other stuff ;-) > > - Dave > > -- Best Regards, Felipe Balbi felipebalbi-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: 0001-I2C-ISP1301_OMAP-New-style-i2c-driver-updates-par.patch --] [-- Type: text/x-patch; name=0001-I2C-ISP1301_OMAP-New-style-i2c-driver-updates-par.patch, Size: 7495 bytes --] From 3ff6ace182c71c9c0cd8c6a45959bdecb08eb4b5 Mon Sep 17 00:00:00 2001 From: Felipe Balbi <me@felipebalbi.com> Date: Tue, 1 Jan 2008 22:11:35 -0500 Subject: [PATCH 1/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 1 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 [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #3: 0002-I2C-ISP1301_OMAP-New-style-i2c-driver-updates-par.patch --] [-- Type: text/x-patch; name=0002-I2C-ISP1301_OMAP-New-style-i2c-driver-updates-par.patch, Size: 11022 bytes --] From d816adfb6053267485bc9c3cb70c032a3ebb9638 Mon Sep 17 00:00:00 2001 From: Felipe Balbi <me@felipebalbi.com> Date: Tue, 1 Jan 2008 23:00:18 -0500 Subject: [PATCH 2/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 2 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 [-- Attachment #4: Type: text/plain, Size: 157 bytes --] _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply related [flat|nested] 39+ messages in thread
[parent not found: <31e679430802231000i4a360269v5d0bc61fc21fd7c3-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH 2/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 2 [not found] ` <31e679430802231000i4a360269v5d0bc61fc21fd7c3-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2008-02-23 18:10 ` David Brownell [not found] ` <200802231010.40108.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> 0 siblings, 1 reply; 39+ messages in thread From: David Brownell @ 2008-02-23 18:10 UTC (permalink / raw) To: Felipe Balbi Cc: tony-4v6yS6AI5VpBDgjK7y7TUQ, dsaxena-k7pgMgclrJvR7s880joybQ, i2c-GZX6beZjE8VD60Wz+7aTrA On Saturday 23 February 2008, Felipe Balbi wrote: > Thanks Dave, the patch is attached. patch 1 is acked by Jean, right Jean? But not yet in the I2C merge queue?? > and patch2 needs some rework, to add some platform_data to get irq and > other stuff ;-) Is patch 2 known to work on omap2 and such, or is the only question whether the omap1 support has broken? - Dave _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply [flat|nested] 39+ messages in thread
[parent not found: <200802231010.40108.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>]
* Re: [PATCH 2/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 2 [not found] ` <200802231010.40108.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> @ 2008-02-23 19:00 ` Felipe Balbi [not found] ` <31e679430802231100r19df9157x9bbd910a592f4392-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2008-02-23 19:28 ` Jean Delvare 1 sibling, 1 reply; 39+ messages in thread From: Felipe Balbi @ 2008-02-23 19:00 UTC (permalink / raw) To: David Brownell Cc: tony-4v6yS6AI5VpBDgjK7y7TUQ, dsaxena-k7pgMgclrJvR7s880joybQ, i2c-GZX6beZjE8VD60Wz+7aTrA [-- Attachment #1: Type: text/plain, Size: 985 bytes --] On Sat, Feb 23, 2008 at 8:10 PM, David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote: > On Saturday 23 February 2008, Felipe Balbi wrote: > > Thanks Dave, the patch is attached. patch 1 is acked by Jean, right Jean? > > But not yet in the I2C merge queue?? not merged, yeah > > > > > and patch2 needs some rework, to add some platform_data to get irq and > > other stuff ;-) > > Is patch 2 known to work on omap2 and such, or is the only question whether > the omap1 support has broken? omap1 support and some comments from Jean which you can get on this thread. BTW, the patch is not applying anymore after tony's omap merge. I have an updated version attached here (of patch2), I'll try to do some more work and keep you Cc:ed PS: i just generated this patch, but it is part 2 of isp and should be on top of patch1 from previous email ;-) > > - Dave > > -- Best Regards, Felipe Balbi felipebalbi-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: 0001-I2C-ISP1301_OMAP-New-style-i2c-driver-updates-part.diff --] [-- Type: text/x-patch; name=0001-I2C-ISP1301_OMAP-New-style-i2c-driver-updates-part.diff, Size: 10313 bytes --] From 0fe387b0e1328942a5aa8590e053aa58a261dad8 Mon Sep 17 00:00:00 2001 From: Felipe Balbi <me@felipebalbi.com> Date: Tue, 1 Jan 2008 23:00:18 -0500 Subject: [PATCH] I2C: ISP1301_OMAP: New-style i2c driver updates, part 2 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-h3.c | 8 ++ arch/arm/mach-omap2/board-h4.c | 11 +++ drivers/i2c/chips/isp1301_omap.c | 147 ++++++++++++------------------------- 3 files changed, 67 insertions(+), 99 deletions(-) diff --git a/arch/arm/mach-omap1/board-h3.c b/arch/arm/mach-omap1/board-h3.c index 6fc5168..7c1b966 100644 --- a/arch/arm/mach-omap1/board-h3.c +++ b/arch/arm/mach-omap1/board-h3.c @@ -469,6 +469,14 @@ static struct omap_gpio_switch h3_gpio_switches[] __initdata = { }, }; +static struct i2c_board_info __initdata h3_i2c_board_info[] = { + { + I2C_BOARD_INFO("isp1301_omap", 0x2d), + .type = "isp1301_omap", + .irq = OMAP_GPIO_IRQ(14), + }, +}; + #define H3_NAND_RB_GPIO_PIN 10 static int nand_dev_ready(struct omap_nand_platform_data *data) diff --git a/arch/arm/mach-omap2/board-h4.c b/arch/arm/mach-omap2/board-h4.c index f125f43..d469c63 100644 --- a/arch/arm/mach-omap2/board-h4.c +++ b/arch/arm/mach-omap2/board-h4.c @@ -301,6 +301,14 @@ 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 +329,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 92a9c01..0981620 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 */ @@ -1190,9 +1175,7 @@ static void isp1301_timer(unsigned long _isp) static void isp1301_release(struct device *dev) { - struct isp1301 *isp; - - isp = container_of(dev, struct isp1301, c.dev); + struct isp1301 *isp = dev_get_drvdata(dev); /* ugly -- i2c hijacks our memory hook to wait_for_completion() */ if (isp->i2c_release) @@ -1202,30 +1185,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; } /*-------------------------------------------------------------------------*/ @@ -1295,9 +1275,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); @@ -1475,11 +1452,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; @@ -1492,46 +1468,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 */ @@ -1552,40 +1517,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, @@ -1602,43 +1552,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.34.g053d9 [-- Attachment #3: Type: text/plain, Size: 157 bytes --] _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply related [flat|nested] 39+ messages in thread
[parent not found: <31e679430802231100r19df9157x9bbd910a592f4392-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH 2/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 2 [not found] ` <31e679430802231100r19df9157x9bbd910a592f4392-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2008-02-23 19:08 ` Felipe Balbi [not found] ` <31e679430802231108k613bdd9ds2782aed429b65b50-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 0 siblings, 1 reply; 39+ messages in thread From: Felipe Balbi @ 2008-02-23 19:08 UTC (permalink / raw) To: David Brownell Cc: tony-4v6yS6AI5VpBDgjK7y7TUQ, dsaxena-k7pgMgclrJvR7s880joybQ, i2c-GZX6beZjE8VD60Wz+7aTrA [-- Attachment #1: Type: text/plain, Size: 1289 bytes --] Hi Dave, On Sat, Feb 23, 2008 at 9:00 PM, Felipe Balbi <felipebalbi-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org> wrote: > On Sat, Feb 23, 2008 at 8:10 PM, David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote: > > On Saturday 23 February 2008, Felipe Balbi wrote: > > > Thanks Dave, the patch is attached. patch 1 is acked by Jean, right Jean? > > > > But not yet in the I2C merge queue?? > > not merged, yeah > > > > > > > > > > > and patch2 needs some rework, to add some platform_data to get irq and > > > other stuff ;-) > > > > Is patch 2 known to work on omap2 and such, or is the only question whether > > the omap1 support has broken? > > omap1 support and some comments from Jean which you can get on this thread. > > BTW, the patch is not applying anymore after tony's omap merge. > I have an updated version attached here (of patch2), I'll try to do > some more work and keep you Cc:ed > > PS: i just generated this patch, but it is part 2 of isp and should be > on top of patch1 from previous email ;-) Sorry replying to myself, but I have a better with most of comments from Jean applied. Missing irq flags, mostly. It's attached -- Best Regards, Felipe Balbi felipebalbi-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: 0001-I2C-ISP1301_OMAP-New-style-i2c-driver-updates-part.diff --] [-- Type: text/x-patch; name=0001-I2C-ISP1301_OMAP-New-style-i2c-driver-updates-part.diff, Size: 9727 bytes --] From 9024a8844c1fdfd65a23e9e66eb6fbca497a14a2 Mon Sep 17 00:00:00 2001 From: Felipe Balbi <me@felipebalbi.com> Date: Tue, 1 Jan 2008 23:00:18 -0500 Subject: [PATCH] I2C: ISP1301_OMAP: New-style i2c driver updates, part 2 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-h3.c | 8 ++ arch/arm/mach-omap2/board-h4.c | 11 +++ drivers/i2c/chips/isp1301_omap.c | 134 ++++++++++--------------------------- 3 files changed, 55 insertions(+), 98 deletions(-) diff --git a/arch/arm/mach-omap1/board-h3.c b/arch/arm/mach-omap1/board-h3.c index 6fc5168..7c1b966 100644 --- a/arch/arm/mach-omap1/board-h3.c +++ b/arch/arm/mach-omap1/board-h3.c @@ -469,6 +469,14 @@ static struct omap_gpio_switch h3_gpio_switches[] __initdata = { }, }; +static struct i2c_board_info __initdata h3_i2c_board_info[] = { + { + I2C_BOARD_INFO("isp1301_omap", 0x2d), + .type = "isp1301_omap", + .irq = OMAP_GPIO_IRQ(14), + }, +}; + #define H3_NAND_RB_GPIO_PIN 10 static int nand_dev_ready(struct omap_nand_platform_data *data) diff --git a/arch/arm/mach-omap2/board-h4.c b/arch/arm/mach-omap2/board-h4.c index f125f43..d469c63 100644 --- a/arch/arm/mach-omap2/board-h4.c +++ b/arch/arm/mach-omap2/board-h4.c @@ -301,6 +301,14 @@ 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 +329,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 92a9c01..6e84995 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 */ @@ -1190,9 +1175,7 @@ static void isp1301_timer(unsigned long _isp) static void isp1301_release(struct device *dev) { - struct isp1301 *isp; - - isp = container_of(dev, struct isp1301, c.dev); + struct isp1301 *isp = dev_get_drvdata(dev); /* ugly -- i2c hijacks our memory hook to wait_for_completion() */ if (isp->i2c_release) @@ -1202,30 +1185,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; } /*-------------------------------------------------------------------------*/ @@ -1295,9 +1275,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); @@ -1475,11 +1452,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; @@ -1492,46 +1468,30 @@ 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); 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 */ @@ -1552,40 +1512,26 @@ 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); + if (client->irq > 0) + 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, @@ -1607,31 +1553,23 @@ fail2: #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), }; /*-------------------------------------------------------------------------*/ -- 1.5.4.34.g053d9 [-- Attachment #3: Type: text/plain, Size: 157 bytes --] _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply related [flat|nested] 39+ messages in thread
[parent not found: <31e679430802231108k613bdd9ds2782aed429b65b50-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH 2/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 2 [not found] ` <31e679430802231108k613bdd9ds2782aed429b65b50-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2008-02-23 19:48 ` Jean Delvare 0 siblings, 0 replies; 39+ messages in thread From: Jean Delvare @ 2008-02-23 19:48 UTC (permalink / raw) To: Felipe Balbi Cc: David Brownell, tony-4v6yS6AI5VpBDgjK7y7TUQ, dsaxena-k7pgMgclrJvR7s880joybQ, i2c-GZX6beZjE8VD60Wz+7aTrA On Sat, 23 Feb 2008 21:08:04 +0200, Felipe Balbi wrote: > Sorry replying to myself, but I have a better with most of comments > from Jean applied. > Missing irq flags, mostly. > > It's attached This update adds this bug: > + if (client->irq > 0) > + status = request_irq(client->irq, isp1301_irq, > + IRQF_SAMPLE_RANDOM | IRQF_TRIGGER_FALLING, > + DRIVER_NAME, isp); > if (status < 0) { This test should be moved inside the "if (client->irq > 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); Also note (I missed it in my previous review): don't call i2c_detach_client() for a new-syle driver! > goto fail1; > } You addressed some of the issues I raised (thanks) but not all, so I still can't include this patch into my i2c tree for now. -- Jean Delvare _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 2/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 2 [not found] ` <200802231010.40108.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> 2008-02-23 19:00 ` Felipe Balbi @ 2008-02-23 19:28 ` Jean Delvare [not found] ` <20080223202832.4a953910-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org> 1 sibling, 1 reply; 39+ messages in thread From: Jean Delvare @ 2008-02-23 19:28 UTC (permalink / raw) To: David Brownell Cc: tony-4v6yS6AI5VpBDgjK7y7TUQ, dsaxena-k7pgMgclrJvR7s880joybQ, i2c-GZX6beZjE8VD60Wz+7aTrA On Sat, 23 Feb 2008 10:10:39 -0800, David Brownell wrote: > On Saturday 23 February 2008, Felipe Balbi wrote: > > Thanks Dave, the patch is attached. patch 1 is acked by Jean, right Jean? Yes, except that my copy has "isp->irq_type = 0;" removed. > But not yet in the I2C merge queue?? I was waiting for the second part. The first part doesn't make much sense alone. I could add the first part to my i2c tree, but I can't merge it upstream without the second part. -- Jean Delvare _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply [flat|nested] 39+ messages in thread
[parent not found: <20080223202832.4a953910-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>]
* Re: [PATCH 2/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 2 [not found] ` <20080223202832.4a953910-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org> @ 2008-02-23 19:53 ` David Brownell [not found] ` <200802231153.04011.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> 0 siblings, 1 reply; 39+ messages in thread From: David Brownell @ 2008-02-23 19:53 UTC (permalink / raw) To: Jean Delvare Cc: tony-4v6yS6AI5VpBDgjK7y7TUQ, dsaxena-k7pgMgclrJvR7s880joybQ, i2c-GZX6beZjE8VD60Wz+7aTrA On Saturday 23 February 2008, Jean Delvare wrote: > On Sat, 23 Feb 2008 10:10:39 -0800, David Brownell wrote: > > On Saturday 23 February 2008, Felipe Balbi wrote: > > > Thanks Dave, the patch is attached. patch 1 is acked by Jean, right Jean? > > Yes, except that my copy has "isp->irq_type = 0;" removed. >From a bzeroed structure? Then I won't worry about that difference... > > But not yet in the I2C merge queue?? > > I was waiting for the second part. The first part doesn't make much > sense alone. I could add the first part to my i2c tree, but I can't > merge it upstream without the second part. That makes no sense to me. If the first part is OK, it's OK to merge without the second part. Even if you don't _plan_ to merge it soon, it'd be less confusing to have that in your queue ... say, towards the end, below a line that indicates a scheduled merge point. (FWIW that's not unlike Andrew does with MM.) - Dave _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply [flat|nested] 39+ messages in thread
[parent not found: <200802231153.04011.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>]
* Re: [PATCH 2/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 2 [not found] ` <200802231153.04011.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> @ 2008-02-24 9:30 ` Jean Delvare [not found] ` <20080224103052.66da6803-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org> 0 siblings, 1 reply; 39+ messages in thread From: Jean Delvare @ 2008-02-24 9:30 UTC (permalink / raw) To: David Brownell Cc: tony-4v6yS6AI5VpBDgjK7y7TUQ, dsaxena-k7pgMgclrJvR7s880joybQ, i2c-GZX6beZjE8VD60Wz+7aTrA On Sat, 23 Feb 2008 11:53:03 -0800, David Brownell wrote: > On Saturday 23 February 2008, Jean Delvare wrote: > > On Sat, 23 Feb 2008 10:10:39 -0800, David Brownell wrote: > > > On Saturday 23 February 2008, Felipe Balbi wrote: > > > > Thanks Dave, the patch is attached. patch 1 is acked by Jean, right Jean? > > > > Yes, except that my copy has "isp->irq_type = 0;" removed. > > From a bzeroed structure? Then I won't worry about that difference... I removed this statement exactly because the structure is kzalloc'd. You don't have to worry as a tester. I do worry as the i2c subsystem maintainer, because I'd like Felipe to base his second patch on the "right" first patch, so that it applies properly to my tree. Felipe, my version of the first patch is available here: http://khali.linux-fr.org/devel/linux-2.6/jdelvare-i2c/i2c-isp1301_omap-convert-to-new-style-1.patch Please make sure that the next update to the second part applies properly on top of it. > > > But not yet in the I2C merge queue?? > > > > I was waiting for the second part. The first part doesn't make much > > sense alone. I could add the first part to my i2c tree, but I can't > > merge it upstream without the second part. > > That makes no sense to me. If the first part is OK, it's OK to > merge without the second part. And if the second part never makes it, we end up with garbage in the upstream kernel. Thanks but no thanks. > Even if you don't _plan_ to merge it soon, it'd be less confusing > to have that in your queue ... say, towards the end, below a line > that indicates a scheduled merge point. (FWIW that's not unlike > Andrew does with MM.) Indeed, the first patch in question has even been in -mm for quite some time now, and Andrew keeps sending it to me so that I take it in my i2c tree. So I've now included the first part in my public i2c patch queue to make everyone happy. But then again, there is no "scheduled merge point" for a patchset that is not yet complete. -- Jean Delvare _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply [flat|nested] 39+ messages in thread
[parent not found: <20080224103052.66da6803-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>]
* Re: [PATCH 2/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 2 [not found] ` <20080224103052.66da6803-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org> @ 2008-02-24 11:05 ` felipebalbi-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f [not found] ` <31e679430802240305o62ec88a9jfcc54f0a3be658ff-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 0 siblings, 1 reply; 39+ messages in thread From: felipebalbi-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f @ 2008-02-24 11:05 UTC (permalink / raw) To: Jean Delvare Cc: David Brownell, tony-4v6yS6AI5VpBDgjK7y7TUQ, dsaxena-k7pgMgclrJvR7s880joybQ, i2c-GZX6beZjE8VD60Wz+7aTrA sorry im topostig, but im using gmail through gprs and it sucks. Jean, thanks for explaining everything and applying the first part to your i2c tree. I'll try to address the issues today or tomorrow and send you a better patch. Thanks again for your time ;) On 2/24/08, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote: > On Sat, 23 Feb 2008 11:53:03 -0800, David Brownell wrote: > > On Saturday 23 February 2008, Jean Delvare wrote: > > > On Sat, 23 Feb 2008 10:10:39 -0800, David Brownell wrote: > > > > On Saturday 23 February 2008, Felipe Balbi wrote: > > > > > Thanks Dave, the patch is attached. patch 1 is acked by Jean, right > Jean? > > > > > > Yes, except that my copy has "isp->irq_type = 0;" removed. > > > > From a bzeroed structure? Then I won't worry about that difference... > > I removed this statement exactly because the structure is kzalloc'd. > You don't have to worry as a tester. I do worry as the i2c subsystem > maintainer, because I'd like Felipe to base his second patch on the > "right" first patch, so that it applies properly to my tree. > > Felipe, my version of the first patch is available here: > http://khali.linux-fr.org/devel/linux-2.6/jdelvare-i2c/i2c-isp1301_omap-convert-to-new-style-1.patch > > Please make sure that the next update to the second part applies > properly on top of it. > > > > > But not yet in the I2C merge queue?? > > > > > > I was waiting for the second part. The first part doesn't make much > > > sense alone. I could add the first part to my i2c tree, but I can't > > > merge it upstream without the second part. > > > > That makes no sense to me. If the first part is OK, it's OK to > > merge without the second part. > > And if the second part never makes it, we end up with garbage in the > upstream kernel. Thanks but no thanks. > > > Even if you don't _plan_ to merge it soon, it'd be less confusing > > to have that in your queue ... say, towards the end, below a line > > that indicates a scheduled merge point. (FWIW that's not unlike > > Andrew does with MM.) > > Indeed, the first patch in question has even been in -mm for quite some > time now, and Andrew keeps sending it to me so that I take it in my i2c > tree. So I've now included the first part in my public i2c patch queue > to make everyone happy. But then again, there is no "scheduled merge > point" for a patchset that is not yet complete. > > -- > Jean Delvare > -- Best Regards, Felipe Balbi felipebalbi-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply [flat|nested] 39+ messages in thread
[parent not found: <31e679430802240305o62ec88a9jfcc54f0a3be658ff-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH 2/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 2 [not found] ` <31e679430802240305o62ec88a9jfcc54f0a3be658ff-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2008-02-25 8:46 ` Felipe Balbi [not found] ` <31e679430802250046h4df04aa0y6a32005e6d791e65-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 0 siblings, 1 reply; 39+ messages in thread From: Felipe Balbi @ 2008-02-25 8:46 UTC (permalink / raw) To: Jean Delvare Cc: David Brownell, tony-4v6yS6AI5VpBDgjK7y7TUQ, dsaxena-k7pgMgclrJvR7s880joybQ, i2c-GZX6beZjE8VD60Wz+7aTrA [-- Attachment #1: Type: text/plain, Size: 3115 bytes --] On Sun, Feb 24, 2008 at 1:05 PM, <felipebalbi-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org> wrote: > sorry im topostig, but im using gmail through gprs and it sucks. Jean, > thanks for explaining everything and applying the first part to your > i2c tree. I'll try to address the issues today or tomorrow and send > you a better patch. > > Thanks again for your time ;) > > > > On 2/24/08, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote: > > On Sat, 23 Feb 2008 11:53:03 -0800, David Brownell wrote: > > > On Saturday 23 February 2008, Jean Delvare wrote: > > > > On Sat, 23 Feb 2008 10:10:39 -0800, David Brownell wrote: > > > > > On Saturday 23 February 2008, Felipe Balbi wrote: > > > > > > Thanks Dave, the patch is attached. patch 1 is acked by Jean, right > > Jean? > > > > > > > > Yes, except that my copy has "isp->irq_type = 0;" removed. > > > > > > From a bzeroed structure? Then I won't worry about that difference... > > > > I removed this statement exactly because the structure is kzalloc'd. > > You don't have to worry as a tester. I do worry as the i2c subsystem > > maintainer, because I'd like Felipe to base his second patch on the > > "right" first patch, so that it applies properly to my tree. > > > > Felipe, my version of the first patch is available here: > > http://khali.linux-fr.org/devel/linux-2.6/jdelvare-i2c/i2c-isp1301_omap-convert-to-new-style-1.patch > > > > Please make sure that the next update to the second part applies > > properly on top of it. > > > > > > > But not yet in the I2C merge queue?? > > > > > > > > I was waiting for the second part. The first part doesn't make much > > > > sense alone. I could add the first part to my i2c tree, but I can't > > > > merge it upstream without the second part. > > > > > > That makes no sense to me. If the first part is OK, it's OK to > > > merge without the second part. > > > > And if the second part never makes it, we end up with garbage in the > > upstream kernel. Thanks but no thanks. > > > > > Even if you don't _plan_ to merge it soon, it'd be less confusing > > > to have that in your queue ... say, towards the end, below a line > > > that indicates a scheduled merge point. (FWIW that's not unlike > > > Andrew does with MM.) > > > > Indeed, the first patch in question has even been in -mm for quite some > > time now, and Andrew keeps sending it to me so that I take it in my i2c > > tree. So I've now included the first part in my public i2c patch queue > > to make everyone happy. But then again, there is no "scheduled merge > > point" for a patchset that is not yet complete. > > > > -- > > Jean Delvare > > > > > > > -- > Best Regards, > > Felipe Balbi > felipebalbi-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org > Another version here. Jean, I'm thinking about introducing include/asm/arch/isp1301.h so I can introduce struct isp1301_platform_data to pass irq_flags to the driver. What do you think ? Dave, any comments ? -- Best Regards, Felipe Balbi felipebalbi-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: 0001-I2C-ISP1301_OMAP-New-style-i2c-driver-updates-part.diff --] [-- Type: text/x-patch; name=0001-I2C-ISP1301_OMAP-New-style-i2c-driver-updates-part.diff, Size: 9582 bytes --] From 44a83059a776e607836139892ed2fd3d7fb307b2 Mon Sep 17 00:00:00 2001 From: Felipe Balbi <me@felipebalbi.com> Date: Tue, 1 Jan 2008 23:00:18 -0500 Subject: [PATCH] I2C: ISP1301_OMAP: New-style i2c driver updates, part 2 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-h3.c | 8 ++ arch/arm/mach-omap2/board-h4.c | 11 +++ drivers/i2c/chips/isp1301_omap.c | 135 ++++++++++--------------------------- 3 files changed, 56 insertions(+), 98 deletions(-) diff --git a/arch/arm/mach-omap1/board-h3.c b/arch/arm/mach-omap1/board-h3.c index 6fc5168..7c1b966 100644 --- a/arch/arm/mach-omap1/board-h3.c +++ b/arch/arm/mach-omap1/board-h3.c @@ -469,6 +469,14 @@ static struct omap_gpio_switch h3_gpio_switches[] __initdata = { }, }; +static struct i2c_board_info __initdata h3_i2c_board_info[] = { + { + I2C_BOARD_INFO("isp1301_omap", 0x2d), + .type = "isp1301_omap", + .irq = OMAP_GPIO_IRQ(14), + }, +}; + #define H3_NAND_RB_GPIO_PIN 10 static int nand_dev_ready(struct omap_nand_platform_data *data) diff --git a/arch/arm/mach-omap2/board-h4.c b/arch/arm/mach-omap2/board-h4.c index f125f43..d469c63 100644 --- a/arch/arm/mach-omap2/board-h4.c +++ b/arch/arm/mach-omap2/board-h4.c @@ -301,6 +301,14 @@ 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 +329,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 92a9c01..3b20366 100644 --- a/drivers/i2c/chips/isp1301_omap.c +++ b/drivers/i2c/chips/isp1301_omap.c @@ -31,8 +31,6 @@ #include <linux/usb/otg.h> #include <linux/i2c.h> #include <linux/workqueue.h> - -#include <asm/irq.h> #include <asm/arch/usb.h> @@ -50,12 +48,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 +134,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 */ @@ -1190,9 +1177,7 @@ static void isp1301_timer(unsigned long _isp) static void isp1301_release(struct device *dev) { - struct isp1301 *isp; - - isp = container_of(dev, struct isp1301, c.dev); + struct isp1301 *isp = dev_get_drvdata(dev); /* ugly -- i2c hijacks our memory hook to wait_for_completion() */ if (isp->i2c_release) @@ -1202,30 +1187,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; } /*-------------------------------------------------------------------------*/ @@ -1295,9 +1277,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); @@ -1475,11 +1454,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; @@ -1492,46 +1470,29 @@ 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); fail1: kfree(isp); return 0; } - isp->i2c_release = i2c->dev.release; - i2c->dev.release = isp1301_release; + isp->i2c_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 */ @@ -1552,40 +1513,26 @@ 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); - if (status < 0) { - dev_dbg(&i2c->dev, "can't get IRQ %d, err %d\n", - isp->irq, status); + if (client->irq > 0) { + status = request_irq(client->irq, isp1301_irq, + IRQF_SAMPLE_RANDOM | IRQF_TRIGGER_FALLING, + DRIVER_NAME, isp); + if (status < 0) { + 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); - goto fail1; + 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, @@ -1607,31 +1554,23 @@ fail2: #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), }; /*-------------------------------------------------------------------------*/ -- 1.5.4.34.g053d9 [-- Attachment #3: Type: text/plain, Size: 157 bytes --] _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply related [flat|nested] 39+ messages in thread
[parent not found: <31e679430802250046h4df04aa0y6a32005e6d791e65-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH 2/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 2 [not found] ` <31e679430802250046h4df04aa0y6a32005e6d791e65-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2008-02-25 12:54 ` Felipe Balbi [not found] ` <31e679430802250454k234e071alc9bbcb2e3b7e7605-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2008-02-26 5:38 ` David Brownell 2008-02-28 21:50 ` Jean Delvare 2 siblings, 1 reply; 39+ messages in thread From: Felipe Balbi @ 2008-02-25 12:54 UTC (permalink / raw) To: Jean Delvare Cc: David Brownell, tony-4v6yS6AI5VpBDgjK7y7TUQ, dsaxena-k7pgMgclrJvR7s880joybQ, i2c-GZX6beZjE8VD60Wz+7aTrA [-- Attachment #1: Type: text/plain, Size: 596 bytes --] Hi all, Dave, here I have two more patches (again). The first one is actually the second part of i2c new style driver for isp1301 and the second one, syncs some changes from linux-omap. Now that Tony merged omap changes to mainline we can update this driver. I'll also send a patch to Tony later when these patches are applied to mainline. Thanks Dave and Jean. ps: I'm still needing some tips about how to pass irq flags from board-files to the driver. A struct platform_data* would be enough ? -- Best Regards, Felipe Balbi felipebalbi-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: 0001-I2C-ISP1301_OMAP-New-style-i2c-driver-updates-part.diff --] [-- Type: text/x-patch; name=0001-I2C-ISP1301_OMAP-New-style-i2c-driver-updates-part.diff, Size: 9585 bytes --] From 0bc638319135186fbc5aa40d9e8b1ae5c3aa4409 Mon Sep 17 00:00:00 2001 From: Felipe Balbi <me@felipebalbi.com> Date: Tue, 1 Jan 2008 23:00:18 -0500 Subject: [PATCH] I2C: ISP1301_OMAP: New-style i2c driver updates, part 2 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-h3.c | 8 ++ arch/arm/mach-omap2/board-h4.c | 11 +++ drivers/i2c/chips/isp1301_omap.c | 137 +++++++++++--------------------------- 3 files changed, 57 insertions(+), 99 deletions(-) diff --git a/arch/arm/mach-omap1/board-h3.c b/arch/arm/mach-omap1/board-h3.c index 6fc5168..7c1b966 100644 --- a/arch/arm/mach-omap1/board-h3.c +++ b/arch/arm/mach-omap1/board-h3.c @@ -469,6 +469,14 @@ static struct omap_gpio_switch h3_gpio_switches[] __initdata = { }, }; +static struct i2c_board_info __initdata h3_i2c_board_info[] = { + { + I2C_BOARD_INFO("isp1301_omap", 0x2d), + .type = "isp1301_omap", + .irq = OMAP_GPIO_IRQ(14), + }, +}; + #define H3_NAND_RB_GPIO_PIN 10 static int nand_dev_ready(struct omap_nand_platform_data *data) diff --git a/arch/arm/mach-omap2/board-h4.c b/arch/arm/mach-omap2/board-h4.c index f125f43..d469c63 100644 --- a/arch/arm/mach-omap2/board-h4.c +++ b/arch/arm/mach-omap2/board-h4.c @@ -301,6 +301,14 @@ 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 +329,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 92a9c01..12ac0b3 100644 --- a/drivers/i2c/chips/isp1301_omap.c +++ b/drivers/i2c/chips/isp1301_omap.c @@ -31,8 +31,6 @@ #include <linux/usb/otg.h> #include <linux/i2c.h> #include <linux/workqueue.h> - -#include <asm/irq.h> #include <asm/arch/usb.h> @@ -50,12 +48,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 +134,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 */ @@ -1190,9 +1177,7 @@ static void isp1301_timer(unsigned long _isp) static void isp1301_release(struct device *dev) { - struct isp1301 *isp; - - isp = container_of(dev, struct isp1301, c.dev); + struct isp1301 *isp = dev_get_drvdata(dev); /* ugly -- i2c hijacks our memory hook to wait_for_completion() */ if (isp->i2c_release) @@ -1202,30 +1187,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; } /*-------------------------------------------------------------------------*/ @@ -1295,9 +1277,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); @@ -1475,11 +1454,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; @@ -1492,46 +1470,29 @@ 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); fail1: kfree(isp); return 0; - } - isp->i2c_release = i2c->dev.release; - i2c->dev.release = isp1301_release; + + isp->i2c_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 */ @@ -1552,40 +1513,26 @@ 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); - if (status < 0) { - dev_dbg(&i2c->dev, "can't get IRQ %d, err %d\n", - isp->irq, status); + if (client->irq > 0) { + status = request_irq(client->irq, isp1301_irq, + IRQF_SAMPLE_RANDOM | IRQF_TRIGGER_FALLING, + DRIVER_NAME, isp); + if (status < 0) { + 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); - goto fail1; + 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, @@ -1607,31 +1554,23 @@ fail2: #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), }; /*-------------------------------------------------------------------------*/ -- 1.5.4.34.g053d9 [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #3: 0002-I2C-ISP1301-Put-linux-mainline-and-linux-omap-in-sy.diff --] [-- Type: text/x-patch; name=0002-I2C-ISP1301-Put-linux-mainline-and-linux-omap-in-sy.diff, Size: 4209 bytes --] From 8a558ae11b68763a3c51cc1c20197f39820e1370 Mon Sep 17 00:00:00 2001 From: Felipe Balbi <felipe.balbi@nokia.com> Date: Mon, 25 Feb 2008 14:30:49 +0200 Subject: [PATCH] I2C: ISP1301: Put linux-mainline and linux-omap in sync This patch merges linux-omap's and mainline's isp1301 driver to put them in sync. Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com> --- drivers/i2c/chips/isp1301_omap.c | 42 ++++++++++++++++++++++++++++--------- 1 files changed, 32 insertions(+), 10 deletions(-) diff --git a/drivers/i2c/chips/isp1301_omap.c b/drivers/i2c/chips/isp1301_omap.c index 12ac0b3..b5ae803 100644 --- a/drivers/i2c/chips/isp1301_omap.c +++ b/drivers/i2c/chips/isp1301_omap.c @@ -18,6 +18,8 @@ * along with this program; if not, write to the Free Software * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA. */ +#undef DEBUG +#undef VERBOSE #include <linux/kernel.h> #include <linux/module.h> @@ -38,7 +40,6 @@ #undef VERBOSE #endif - #define DRIVER_VERSION "24 August 2004" #define DRIVER_NAME (isp1301_driver.driver.name) @@ -84,7 +85,8 @@ struct isp1301 { /*-------------------------------------------------------------------------*/ -#ifdef CONFIG_MACH_OMAP_H2 +#if defined(CONFIG_MACH_OMAP_H2) || \ + defined(CONFIG_MACH_OMAP_H3) /* board-specific PM hooks */ @@ -121,15 +123,26 @@ static void enable_vbus_source(struct isp1301 *isp) */ } +#else -/* products will deliver OTG messages with LEDs, GUI, etc */ -static inline void notresponding(struct isp1301 *isp) +static void enable_vbus_draw(struct isp1301 *isp, unsigned mA) { - printk(KERN_NOTICE "OTG device not responding.\n"); + pr_debug("%s UNIMPL\n", __FUNCTION__); } +static void enable_vbus_source(struct isp1301 *isp) +{ + pr_debug("%s UNIMPL\n", __FUNCTION__); +} #endif +/*-------------------------------------------------------------------------*/ + +/* products will deliver OTG messages with LEDs, GUI, etc */ +static inline void notresponding(struct isp1301 *isp) +{ + printk(KERN_NOTICE "OTG device not responding.\n"); +} /*-------------------------------------------------------------------------*/ @@ -495,6 +508,7 @@ static inline void check_state(struct isp1301 *isp, const char *tag) { } static void update_otg1(struct isp1301 *isp, u8 int_src) { u32 otg_ctrl; + u8 int_id; otg_ctrl = OTG_CTRL_REG & OTG_CTRL_MASK @@ -508,7 +522,10 @@ static void update_otg1(struct isp1301 *isp, u8 int_src) } if (int_src & INTR_VBUS_VLD) otg_ctrl |= OTG_VBUSVLD; - if (int_src & INTR_ID_GND) { /* default-A */ + + int_id = isp1301_get_u8(isp, ISP1301_INTERRUPT_SOURCE); + + if (int_id & INTR_ID_GND) { /* default-A */ if (isp->otg.state == OTG_STATE_B_IDLE || isp->otg.state == OTG_STATE_UNDEFINED) { a_idle(isp, "init"); @@ -1063,7 +1080,7 @@ static void isp_update_otg(struct isp1301 *isp, u8 stat) /* update the OTG controller state to match the isp1301; may * trigger OPRT_CHG irqs for changes going to the isp1301. */ - update_otg1(isp, isp_stat); + update_otg1(isp, isp_stat); // pass the actual interrupt latch status update_otg2(isp, isp_bstat); check_state(isp, __FUNCTION__); #endif @@ -1337,13 +1354,14 @@ isp1301_set_peripheral(struct otg_transceiver *otg, struct usb_gadget *gadget) power_up(isp); isp->otg.state = OTG_STATE_B_IDLE; - if (machine_is_omap_h2()) +/* REVISIT H4 too? */ + if (machine_is_omap_h2() || machine_is_omap_h3()) isp1301_set_bits(isp, ISP1301_MODE_CONTROL_1, MC1_DAT_SE0); isp1301_set_bits(isp, ISP1301_INTERRUPT_RISING, - INTR_SESS_VLD); + INTR_SESS_VLD | INTR_VBUS_VLD); isp1301_set_bits(isp, ISP1301_INTERRUPT_FALLING, - INTR_VBUS_VLD); + INTR_VBUS_VLD | INTR_SESS_VLD); dev_info(&isp->client->dev, "B-Peripheral sessions ok\n"); dump_regs(isp, __FUNCTION__); @@ -1420,6 +1438,10 @@ isp1301_start_hnp(struct otg_transceiver *dev) * So do this part as early as possible... */ switch (isp->otg.state) { + case OTG_STATE_B_PERIPHERAL: + isp->otg.state = OTG_STATE_B_WAIT_ACON; + isp1301_defer_work(isp, WORK_UPDATE_ISP); + break; case OTG_STATE_B_HOST: isp->otg.state = OTG_STATE_B_PERIPHERAL; /* caller will suspend next */ -- 1.5.4.34.g053d9 [-- Attachment #4: Type: text/plain, Size: 157 bytes --] _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply related [flat|nested] 39+ messages in thread
[parent not found: <31e679430802250454k234e071alc9bbcb2e3b7e7605-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH 2/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 2 [not found] ` <31e679430802250454k234e071alc9bbcb2e3b7e7605-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2008-03-06 6:28 ` David Brownell [not found] ` <200803052228.47162.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> 0 siblings, 1 reply; 39+ messages in thread From: David Brownell @ 2008-03-06 6:28 UTC (permalink / raw) To: Felipe Balbi Cc: tony-4v6yS6AI5VpBDgjK7y7TUQ, dsaxena-k7pgMgclrJvR7s880joybQ, i2c-GZX6beZjE8VD60Wz+7aTrA On Monday 25 February 2008, Felipe Balbi wrote: > > Hi all, > > Dave, here I have two more patches (again). > > The first one is actually the second part of i2c new style driver for > isp1301 I tried to apply this over the (Jean's version of) the first one, and got rejects ... _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply [flat|nested] 39+ messages in thread
[parent not found: <200803052228.47162.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>]
* Re: [PATCH 2/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 2 [not found] ` <200803052228.47162.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> @ 2008-03-06 9:15 ` Felipe Balbi 0 siblings, 0 replies; 39+ messages in thread From: Felipe Balbi @ 2008-03-06 9:15 UTC (permalink / raw) To: David Brownell Cc: tony-4v6yS6AI5VpBDgjK7y7TUQ, dsaxena-k7pgMgclrJvR7s880joybQ, i2c-GZX6beZjE8VD60Wz+7aTrA Hi, On Thu, Mar 6, 2008 at 8:28 AM, David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote: > On Monday 25 February 2008, Felipe Balbi wrote: > > > > Hi all, > > > > Dave, here I have two more patches (again). > > > > The first one is actually the second part of i2c new style driver for > > isp1301 > > I tried to apply this over the (Jean's version of) the first one, > and got rejects ... Crap, I'll refresh and resend ;-) -- Best Regards, Felipe Balbi felipebalbi-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 2/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 2 [not found] ` <31e679430802250046h4df04aa0y6a32005e6d791e65-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2008-02-25 12:54 ` Felipe Balbi @ 2008-02-26 5:38 ` David Brownell [not found] ` <200802252138.30301.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> 2008-02-28 21:50 ` Jean Delvare 2 siblings, 1 reply; 39+ messages in thread From: David Brownell @ 2008-02-26 5:38 UTC (permalink / raw) To: Felipe Balbi Cc: tony-4v6yS6AI5VpBDgjK7y7TUQ, dsaxena-k7pgMgclrJvR7s880joybQ, i2c-GZX6beZjE8VD60Wz+7aTrA On Monday 25 February 2008, Felipe Balbi wrote: > Jean, I'm thinking about introducing include/asm/arch/isp1301.h Or maybe isp1301_omap.h to match the driver name. > so I > can introduce struct isp1301_platform_data to pass irq_flags to the > driver. Passing irq_flags??? That sounds like nonsense. Please explain ... By the way, I see why there's no board-h2.c update. One of the patches in December stripped out most of the tps65010 i2c_board info from mainline, replacing some of it with isp1301 stuff ... that can't possibly work in mainline. So there are going to be some H2 and H3 regression-fix patches upcoming... - Dave _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply [flat|nested] 39+ messages in thread
[parent not found: <200802252138.30301.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>]
* Re: [PATCH 2/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 2 [not found] ` <200802252138.30301.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> @ 2008-02-26 9:22 ` Felipe Balbi [not found] ` <31e679430802260122xc30356ayd98fc3c079e613b7-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 0 siblings, 1 reply; 39+ messages in thread From: Felipe Balbi @ 2008-02-26 9:22 UTC (permalink / raw) To: David Brownell Cc: tony-4v6yS6AI5VpBDgjK7y7TUQ, dsaxena-k7pgMgclrJvR7s880joybQ, i2c-GZX6beZjE8VD60Wz+7aTrA On Tue, Feb 26, 2008 at 7:38 AM, David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote: > On Monday 25 February 2008, Felipe Balbi wrote: > > Jean, I'm thinking about introducing include/asm/arch/isp1301.h > > Or maybe isp1301_omap.h to match the driver name. > > > > > so I > > can introduce struct isp1301_platform_data to pass irq_flags to the > > driver. > > Passing irq_flags??? That sounds like nonsense. Please explain ... request_irq was using different irq flags depending on the board, so instead of keeping if (machine_is_*) in the driver just because of irq flags we could bring it from a platform_data. > > > By the way, I see why there's no board-h2.c update. One of > the patches in December stripped out most of the tps65010 > i2c_board info from mainline, replacing some of it with > isp1301 stuff ... that can't possibly work in mainline. > > So there are going to be some H2 and H3 regression-fix > patches upcoming... that's true... -- Best Regards, Felipe Balbi felipebalbi-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply [flat|nested] 39+ messages in thread
[parent not found: <31e679430802260122xc30356ayd98fc3c079e613b7-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH 2/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 2 [not found] ` <31e679430802260122xc30356ayd98fc3c079e613b7-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2008-03-07 20:20 ` David Brownell [not found] ` <200803071220.10744.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> 0 siblings, 1 reply; 39+ messages in thread From: David Brownell @ 2008-03-07 20:20 UTC (permalink / raw) To: Felipe Balbi Cc: tony-4v6yS6AI5VpBDgjK7y7TUQ, dsaxena-k7pgMgclrJvR7s880joybQ, i2c-GZX6beZjE8VD60Wz+7aTrA On Tuesday 26 February 2008, Felipe Balbi wrote: > > > > Passing irq_flags??? That sounds like nonsense. Please explain ... > > request_irq was using different irq flags depending on the board, so > instead of keeping if (machine_is_*) in the driver just because of irq > flags we could bring it from a platform_data. Just use the resource type flags ... IORESOURCE_IRQ can be combined with #define IORESOURCE_IRQ_HIGHEDGE (1<<0) #define IORESOURCE_IRQ_LOWEDGE (1<<1) #define IORESOURCE_IRQ_HIGHLEVEL (1<<2) #define IORESOURCE_IRQ_LOWLEVEL (1<<3) #define IORESOURCE_IRQ_SHAREABLE (1<<4) I'd presume "highedge" means "rising", etc. - Dave _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply [flat|nested] 39+ messages in thread
[parent not found: <200803071220.10744.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>]
* Re: [PATCH 2/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 2 [not found] ` <200803071220.10744.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> @ 2008-03-07 21:00 ` Felipe Balbi [not found] ` <31e679430803071300w2f2e80ddqd4516a976df4474-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 0 siblings, 1 reply; 39+ messages in thread From: Felipe Balbi @ 2008-03-07 21:00 UTC (permalink / raw) To: David Brownell Cc: tony-4v6yS6AI5VpBDgjK7y7TUQ, dsaxena-k7pgMgclrJvR7s880joybQ, i2c-GZX6beZjE8VD60Wz+7aTrA Hi, On Fri, Mar 7, 2008 at 10:20 PM, David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote: > On Tuesday 26 February 2008, Felipe Balbi wrote: > > > > > > Passing irq_flags??? That sounds like nonsense. Please explain ... > > > > request_irq was using different irq flags depending on the board, so > > instead of keeping if (machine_is_*) in the driver just because of irq > > flags we could bring it from a platform_data. > > Just use the resource type flags ... IORESOURCE_IRQ can > be combined with but then isp1301 should become a platform_driver, shouldn't it? I'd need a struct platform_device on the board file and get the device on probe just to get an irq flag? Maybe I'm missing something but I'm understanding something like: --- a/arch/arm/mach-omap1/board-h2.c +++ b/arch/arm/mach-omap1/board-h2.c @@ -140,6 +140,17 @@ static struct platform_device h2_nor_device = { .resource = &h2_nor_resource, }; +static struct resource h2_isp1301_resource = { + .flags = IORESOURCE_IRQ | IORESOURCE_IRQ_HIGHEDGE, +}; + +static struct platform_device h2_nor_device = { + .name = "isp1301_omap", + .id = 0, + .num_resources = 1, + .resource = &h2_isp1301_resource, +}; + static struct mtd_partition h2_nand_partitions[] = { #if 0 /* REVISIT: enable these partitions if you make NAND BOOT diff --git a/drivers/i2c/chips/isp1301_omap.c b/drivers/i2c/chips/isp1301_omap.c index 4bb931c..d0a5825 100644 --- a/drivers/i2c/chips/isp1301_omap.c +++ b/drivers/i2c/chips/isp1301_omap.c @@ -1453,6 +1453,27 @@ isp1301_start_hnp(struct otg_transceiver *dev) /*-------------------------------------------------------------------------*/ +struct platform_driver isp1301_plat_driver = { + .driver = { + .name = DRIVER_NAME, + .bus = &platform_bus_type, + .owner = THIS_MODULE, + }, +}; + +static int __init isp1301_plat_probe(struct platform_device *pdev) +{ + struct resource *resource; + + resource = platform_get_resource(pdev, IORESOURCE_IRQ, 0); + if (!resource) + return -ENODEV; + + the_transceiver->irq_type = resource->flags; +} + +/*-------------------------------------------------------------------------*/ + /* no error returns, they'd just make bus scanning stop */ static int __init isp1301_probe(struct i2c_client *client) { (of course the code above doesn't work, it's just to clear my mind :-p) Anyways, wouldn't be easier to put a platform_data that can ship withing struct i2c_board_info? Sorry if I'm being pointless :-s -- Best Regards, Felipe Balbi felipebalbi-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply related [flat|nested] 39+ messages in thread
[parent not found: <31e679430803071300w2f2e80ddqd4516a976df4474-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH 2/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 2 [not found] ` <31e679430803071300w2f2e80ddqd4516a976df4474-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2008-03-15 11:59 ` Jean Delvare [not found] ` <20080315125940.2fe84f24-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org> 0 siblings, 1 reply; 39+ messages in thread From: Jean Delvare @ 2008-03-15 11:59 UTC (permalink / raw) To: Felipe Balbi Cc: David Brownell, dsaxena-k7pgMgclrJvR7s880joybQ, i2c-GZX6beZjE8VD60Wz+7aTrA Hi Felipe, On Fri, 7 Mar 2008 23:00:40 +0200, Felipe Balbi wrote: > Hi, > > On Fri, Mar 7, 2008 at 10:20 PM, David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote: > > On Tuesday 26 February 2008, Felipe Balbi wrote: > > > > > > > > Passing irq_flags??? That sounds like nonsense. Please explain ... > > > > > > request_irq was using different irq flags depending on the board, so > > > instead of keeping if (machine_is_*) in the driver just because of irq > > > flags we could bring it from a platform_data. > > > > Just use the resource type flags ... IORESOURCE_IRQ can > > be combined with > > but then isp1301 should become a platform_driver, shouldn't it? > I'd need a struct platform_device on the board file and get the device > on probe just to get an irq flag? > > Maybe I'm missing something but I'm understanding something like: > > --- a/arch/arm/mach-omap1/board-h2.c > +++ b/arch/arm/mach-omap1/board-h2.c > @@ -140,6 +140,17 @@ static struct platform_device h2_nor_device = { > .resource = &h2_nor_resource, > }; > > +static struct resource h2_isp1301_resource = { > + .flags = IORESOURCE_IRQ | IORESOURCE_IRQ_HIGHEDGE, > +}; > + > +static struct platform_device h2_nor_device = { > + .name = "isp1301_omap", > + .id = 0, > + .num_resources = 1, > + .resource = &h2_isp1301_resource, > +}; > + > static struct mtd_partition h2_nand_partitions[] = { > #if 0 > /* REVISIT: enable these partitions if you make NAND BOOT > diff --git a/drivers/i2c/chips/isp1301_omap.c b/drivers/i2c/chips/isp1301_omap.c > index 4bb931c..d0a5825 100644 > --- a/drivers/i2c/chips/isp1301_omap.c > +++ b/drivers/i2c/chips/isp1301_omap.c > @@ -1453,6 +1453,27 @@ isp1301_start_hnp(struct otg_transceiver *dev) > > /*-------------------------------------------------------------------------*/ > > +struct platform_driver isp1301_plat_driver = { > + .driver = { > + .name = DRIVER_NAME, > + .bus = &platform_bus_type, > + .owner = THIS_MODULE, > + }, > +}; > + > +static int __init isp1301_plat_probe(struct platform_device *pdev) > +{ > + struct resource *resource; > + > + resource = platform_get_resource(pdev, IORESOURCE_IRQ, 0); > + if (!resource) > + return -ENODEV; > + > + the_transceiver->irq_type = resource->flags; > +} > + > +/*-------------------------------------------------------------------------*/ > + > /* no error returns, they'd just make bus scanning stop */ > static int __init isp1301_probe(struct i2c_client *client) > { > > (of course the code above doesn't work, it's just to clear my mind :-p) > > Anyways, wouldn't be easier to put a platform_data that can ship > withing struct i2c_board_info? The above looks all wrong to me. The ISP1301 is an I²C device, it gets an i2c driver, not a platform driver. If you need to pass any extra parameter from the platform code to the isp1301 driver, you must define a custom structure and pass it as i2c_board_info->platform_data. In your case, it would look like: struct isp1301_platform_data { int irq_type; } David, please correct me if I'm wrong. That being said, if many drivers need an irq_type field in addition to the irq number, we might consider adding an irq_type field to struct i2c_board_info and struct i2c_client directly. It probably doesn't make much sense to have irq and irq_type take different routes if they are most often needed together. -- Jean Delvare _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply [flat|nested] 39+ messages in thread
[parent not found: <20080315125940.2fe84f24-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>]
* Re: [PATCH 2/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 2 [not found] ` <20080315125940.2fe84f24-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org> @ 2008-03-15 12:04 ` Felipe Balbi 2008-03-16 3:57 ` David Brownell 1 sibling, 0 replies; 39+ messages in thread From: Felipe Balbi @ 2008-03-15 12:04 UTC (permalink / raw) To: Jean Delvare Cc: David Brownell, dsaxena-k7pgMgclrJvR7s880joybQ, i2c-GZX6beZjE8VD60Wz+7aTrA Hi, On Sat, Mar 15, 2008 at 1:59 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote: > Hi Felipe, > > > > On Fri, 7 Mar 2008 23:00:40 +0200, Felipe Balbi wrote: > > Hi, > > > > On Fri, Mar 7, 2008 at 10:20 PM, David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote: > > > On Tuesday 26 February 2008, Felipe Balbi wrote: > > > > > > > > > > Passing irq_flags??? That sounds like nonsense. Please explain ... > > > > > > > > request_irq was using different irq flags depending on the board, so > > > > instead of keeping if (machine_is_*) in the driver just because of irq > > > > flags we could bring it from a platform_data. > > > > > > Just use the resource type flags ... IORESOURCE_IRQ can > > > be combined with > > > > but then isp1301 should become a platform_driver, shouldn't it? > > I'd need a struct platform_device on the board file and get the device > > on probe just to get an irq flag? > > > > Maybe I'm missing something but I'm understanding something like: > > > > --- a/arch/arm/mach-omap1/board-h2.c > > +++ b/arch/arm/mach-omap1/board-h2.c > > @@ -140,6 +140,17 @@ static struct platform_device h2_nor_device = { > > .resource = &h2_nor_resource, > > }; > > > > +static struct resource h2_isp1301_resource = { > > + .flags = IORESOURCE_IRQ | IORESOURCE_IRQ_HIGHEDGE, > > +}; > > + > > +static struct platform_device h2_nor_device = { > > + .name = "isp1301_omap", > > + .id = 0, > > + .num_resources = 1, > > + .resource = &h2_isp1301_resource, > > +}; > > + > > static struct mtd_partition h2_nand_partitions[] = { > > #if 0 > > /* REVISIT: enable these partitions if you make NAND BOOT > > diff --git a/drivers/i2c/chips/isp1301_omap.c b/drivers/i2c/chips/isp1301_omap.c > > index 4bb931c..d0a5825 100644 > > --- a/drivers/i2c/chips/isp1301_omap.c > > +++ b/drivers/i2c/chips/isp1301_omap.c > > @@ -1453,6 +1453,27 @@ isp1301_start_hnp(struct otg_transceiver *dev) > > > > /*-------------------------------------------------------------------------*/ > > > > +struct platform_driver isp1301_plat_driver = { > > + .driver = { > > + .name = DRIVER_NAME, > > + .bus = &platform_bus_type, > > + .owner = THIS_MODULE, > > + }, > > +}; > > + > > +static int __init isp1301_plat_probe(struct platform_device *pdev) > > +{ > > + struct resource *resource; > > + > > + resource = platform_get_resource(pdev, IORESOURCE_IRQ, 0); > > + if (!resource) > > + return -ENODEV; > > + > > + the_transceiver->irq_type = resource->flags; > > +} > > + > > +/*-------------------------------------------------------------------------*/ > > + > > /* no error returns, they'd just make bus scanning stop */ > > static int __init isp1301_probe(struct i2c_client *client) > > { > > > > (of course the code above doesn't work, it's just to clear my mind :-p) > > > > Anyways, wouldn't be easier to put a platform_data that can ship > > withing struct i2c_board_info? > > The above looks all wrong to me. The ISP1301 is an I²C device, it gets > an i2c driver, not a platform driver. That's what I was trying to say. > If you need to pass any extra parameter from the platform code to the > isp1301 driver, you must define a custom structure and pass it as > i2c_board_info->platform_data. In your case, it would look like: > > struct isp1301_platform_data { > int irq_type; > } Yes, I was doing exactly this. > David, please correct me if I'm wrong. > > That being said, if many drivers need an irq_type field in addition to > the irq number, we might consider adding an irq_type field to struct > i2c_board_info and struct i2c_client directly. It probably doesn't make > much sense to have irq and irq_type take different routes if they are > most often needed together. Agreed here, the code will also look cleaner if we: request_irq(client->irq, irq_handler, client->irq_flags, CLIENT_DRIVER_NAME, client); -- Best Regards, Felipe Balbi felipebalbi-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 2/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 2 [not found] ` <20080315125940.2fe84f24-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org> 2008-03-15 12:04 ` Felipe Balbi @ 2008-03-16 3:57 ` David Brownell [not found] ` <200803152057.19375.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> 1 sibling, 1 reply; 39+ messages in thread From: David Brownell @ 2008-03-16 3:57 UTC (permalink / raw) To: Jean Delvare; +Cc: dsaxena-k7pgMgclrJvR7s880joybQ, i2c-GZX6beZjE8VD60Wz+7aTrA On Saturday 15 March 2008, Jean Delvare wrote: > The above looks all wrong to me. The ISP1301 is an I²C device, it gets > an i2c driver, not a platform driver. Right; blame that on me, for a moment I forgot and so I mentioned the use of the IORESOURCE_IRQ_* flags, which work on busses using resources -- like "platform" and "pnp"). Not helpful here. > That being said, if many drivers need an irq_type field in addition to > the irq number, we might consider adding an irq_type field to struct > i2c_board_info and struct i2c_client directly. It probably doesn't make > much sense to have irq and irq_type take different routes if they are > most often needed together. Actually ... why not just require the board-specific setup code to set the right trigger mode? Use set_irq_type(). That's what x86 does, and it works fine. Drivers don't need to worry about trigger modes ("irq_type") at all, since the board setup code (on PCs: ACPI or BIOS) just stuffs the same registers that set_irq_type() would stuff. Drivers can just: status = request_irq(irq, handler, 0, "name", data); Platforms already do the pinmux setup, making sure that the relevant pin is set up as an external interrupt source or GPIO input as appropriate. They can just as well do the rest. - Dave _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply [flat|nested] 39+ messages in thread
[parent not found: <200803152057.19375.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>]
* Re: [PATCH 2/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 2 [not found] ` <200803152057.19375.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> @ 2008-03-16 10:22 ` Felipe Balbi [not found] ` <31e679430803160322v7b8d6ecepf03bd8b87d975553-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 0 siblings, 1 reply; 39+ messages in thread From: Felipe Balbi @ 2008-03-16 10:22 UTC (permalink / raw) To: David Brownell; +Cc: dsaxena-k7pgMgclrJvR7s880joybQ, i2c-GZX6beZjE8VD60Wz+7aTrA On Sun, Mar 16, 2008 at 5:57 AM, David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote: > On Saturday 15 March 2008, Jean Delvare wrote: > > The above looks all wrong to me. The ISP1301 is an I²C device, it gets > > an i2c driver, not a platform driver. > > Right; blame that on me, for a moment I forgot and so I > mentioned the use of the IORESOURCE_IRQ_* flags, which work > on busses using resources -- like "platform" and "pnp"). > Not helpful here. > > > > > That being said, if many drivers need an irq_type field in addition to > > the irq number, we might consider adding an irq_type field to struct > > i2c_board_info and struct i2c_client directly. It probably doesn't make > > much sense to have irq and irq_type take different routes if they are > > most often needed together. > > Actually ... why not just require the board-specific setup > code to set the right trigger mode? Use set_irq_type(). > > That's what x86 does, and it works fine. Drivers don't need > to worry about trigger modes ("irq_type") at all, since the > board setup code (on PCs: ACPI or BIOS) just stuffs the same > registers that set_irq_type() would stuff. Drivers can just: > > status = request_irq(irq, handler, 0, "name", data); > > Platforms already do the pinmux setup, making sure that the > relevant pin is set up as an external interrupt source or > GPIO input as appropriate. They can just as well do the rest. Good catch dave, actually osk is doing that already :-) so I'll add set_irq_type($ISP1301_IRQ, $ISP1301_FLAGS); for all 3 boards and on the driver i'll request_irq like you said above. Thanks > > - Dave > > > > > -- Best Regards, Felipe Balbi felipebalbi-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply [flat|nested] 39+ messages in thread
[parent not found: <31e679430803160322v7b8d6ecepf03bd8b87d975553-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH 2/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 2 [not found] ` <31e679430803160322v7b8d6ecepf03bd8b87d975553-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2008-03-16 10:40 ` Jean Delvare [not found] ` <20080316114043.57e38dd7-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org> 0 siblings, 1 reply; 39+ messages in thread From: Jean Delvare @ 2008-03-16 10:40 UTC (permalink / raw) To: Felipe Balbi Cc: David Brownell, dsaxena-k7pgMgclrJvR7s880joybQ, i2c-GZX6beZjE8VD60Wz+7aTrA On Sun, 16 Mar 2008 12:22:10 +0200, Felipe Balbi wrote: > On Sun, Mar 16, 2008 at 5:57 AM, David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote: > > On Saturday 15 March 2008, Jean Delvare wrote: > > > The above looks all wrong to me. The ISP1301 is an I²C device, it gets > > > an i2c driver, not a platform driver. > > > > Right; blame that on me, for a moment I forgot and so I > > mentioned the use of the IORESOURCE_IRQ_* flags, which work > > on busses using resources -- like "platform" and "pnp"). > > Not helpful here. > > > > > > > > > That being said, if many drivers need an irq_type field in addition to > > > the irq number, we might consider adding an irq_type field to struct > > > i2c_board_info and struct i2c_client directly. It probably doesn't make > > > much sense to have irq and irq_type take different routes if they are > > > most often needed together. > > > > Actually ... why not just require the board-specific setup > > code to set the right trigger mode? Use set_irq_type(). > > > > That's what x86 does, and it works fine. Drivers don't need > > to worry about trigger modes ("irq_type") at all, since the > > board setup code (on PCs: ACPI or BIOS) just stuffs the same > > registers that set_irq_type() would stuff. Drivers can just: > > > > status = request_irq(irq, handler, 0, "name", data); > > > > Platforms already do the pinmux setup, making sure that the > > relevant pin is set up as an external interrupt source or > > GPIO input as appropriate. They can just as well do the rest. > > Good catch dave, actually osk is doing that already :-) > so I'll add set_irq_type($ISP1301_IRQ, $ISP1301_FLAGS); for all 3 > boards and on the driver i'll request_irq like you said above. Very nice. I wanted to suggest this originally, but stepped back as I noticed that request_irq() had a parameter for the IRQ flags. As request_irq() must be called by the device driver, I concluded that we really needed to carry the IRQ flags from the platform code to the driver... I didn't know that there was a way to configure the IRQ before requesting it. That's obviously much nicer. Would be great if we could get rid of that flags parameter of request_irq(), if you ask me. Thanks David for your help :) -- Jean Delvare _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply [flat|nested] 39+ messages in thread
[parent not found: <20080316114043.57e38dd7-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>]
* Re: [PATCH 2/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 2 [not found] ` <20080316114043.57e38dd7-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org> @ 2008-03-16 11:07 ` Felipe Balbi 0 siblings, 0 replies; 39+ messages in thread From: Felipe Balbi @ 2008-03-16 11:07 UTC (permalink / raw) To: Jean Delvare Cc: David Brownell, dsaxena-k7pgMgclrJvR7s880joybQ, i2c-GZX6beZjE8VD60Wz+7aTrA On Sun, Mar 16, 2008 at 12:40 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote: > > On Sun, 16 Mar 2008 12:22:10 +0200, Felipe Balbi wrote: > > On Sun, Mar 16, 2008 at 5:57 AM, David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote: > > > On Saturday 15 March 2008, Jean Delvare wrote: > > > > The above looks all wrong to me. The ISP1301 is an I²C device, it gets > > > > an i2c driver, not a platform driver. > > > > > > Right; blame that on me, for a moment I forgot and so I > > > mentioned the use of the IORESOURCE_IRQ_* flags, which work > > > on busses using resources -- like "platform" and "pnp"). > > > Not helpful here. > > > > > > > > > > > > > That being said, if many drivers need an irq_type field in addition to > > > > the irq number, we might consider adding an irq_type field to struct > > > > i2c_board_info and struct i2c_client directly. It probably doesn't make > > > > much sense to have irq and irq_type take different routes if they are > > > > most often needed together. > > > > > > Actually ... why not just require the board-specific setup > > > code to set the right trigger mode? Use set_irq_type(). > > > > > > That's what x86 does, and it works fine. Drivers don't need > > > to worry about trigger modes ("irq_type") at all, since the > > > board setup code (on PCs: ACPI or BIOS) just stuffs the same > > > registers that set_irq_type() would stuff. Drivers can just: > > > > > > status = request_irq(irq, handler, 0, "name", data); > > > > > > Platforms already do the pinmux setup, making sure that the > > > relevant pin is set up as an external interrupt source or > > > GPIO input as appropriate. They can just as well do the rest. > > > > Good catch dave, actually osk is doing that already :-) > > so I'll add set_irq_type($ISP1301_IRQ, $ISP1301_FLAGS); for all 3 > > boards and on the driver i'll request_irq like you said above. > > Very nice. I wanted to suggest this originally, but stepped back as I > noticed that request_irq() had a parameter for the IRQ flags. As > request_irq() must be called by the device driver, I concluded that we > really needed to carry the IRQ flags from the platform code to the > driver... I didn't know that there was a way to configure the IRQ > before requesting it. That's obviously much nicer. Would be great if we > could get rid of that flags parameter of request_irq(), if you ask me. I was just thinking about the same :-) If all the irq configuration were moved to board-files, we could get rid of that parameter and it'd be just great ;-) Thanks Dave and Jean for all your time during this patch. Hopefully it's now good enough. I also sent the patch putting linux-omap and linux-mainline in sync. If tell me that's ok, i'll send a similar patch to linux-omap asap Cc:ing both you and Dave. -- Best Regards, Felipe Balbi felipebalbi-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 2/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 2 [not found] ` <31e679430802250046h4df04aa0y6a32005e6d791e65-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2008-02-25 12:54 ` Felipe Balbi 2008-02-26 5:38 ` David Brownell @ 2008-02-28 21:50 ` Jean Delvare [not found] ` <20080228225032.61f07a18-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org> 2 siblings, 1 reply; 39+ messages in thread From: Jean Delvare @ 2008-02-28 21:50 UTC (permalink / raw) To: Felipe Balbi Cc: David Brownell, tony-4v6yS6AI5VpBDgjK7y7TUQ, dsaxena-k7pgMgclrJvR7s880joybQ, i2c-GZX6beZjE8VD60Wz+7aTrA Hi Felipe, On Mon, 25 Feb 2008 10:46:21 +0200, Felipe Balbi wrote: > Jean, I'm thinking about introducing include/asm/arch/isp1301.h so I > can introduce struct isp1301_platform_data to pass irq_flags to the > driver. > What do you think ? This would rather be include/linux/i2c/isp1301_omap.h, now that we have created this directory. -- Jean Delvare _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply [flat|nested] 39+ messages in thread
[parent not found: <20080228225032.61f07a18-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>]
* Re: [PATCH 2/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 2 [not found] ` <20080228225032.61f07a18-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org> @ 2008-02-28 21:53 ` Felipe Balbi 0 siblings, 0 replies; 39+ messages in thread From: Felipe Balbi @ 2008-02-28 21:53 UTC (permalink / raw) To: Jean Delvare Cc: David Brownell, tony-4v6yS6AI5VpBDgjK7y7TUQ, dsaxena-k7pgMgclrJvR7s880joybQ, i2c-GZX6beZjE8VD60Wz+7aTrA Hi, On Thu, Feb 28, 2008 at 11:50 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote: > Hi Felipe, > > > On Mon, 25 Feb 2008 10:46:21 +0200, Felipe Balbi wrote: > > Jean, I'm thinking about introducing include/asm/arch/isp1301.h so I > > can introduce struct isp1301_platform_data to pass irq_flags to the > > driver. > > What do you think ? > > This would rather be include/linux/i2c/isp1301_omap.h, now that we have > created this directory. Thanks Jean, I'll try to address this issue during this week and ask someone in linux-omap ML to try the patches agains linux-mainline :-) Have a nice day > > -- > Jean Delvare > -- Best Regards, Felipe Balbi felipebalbi-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 1/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 1 2008-01-03 16:59 ` [PATCH 1/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 1 Felipe Balbi 2008-01-03 16:59 ` [PATCH 2/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 2 Felipe Balbi @ 2008-01-21 17:15 ` Jean Delvare 2008-01-21 18:37 ` Felipe Balbi 1 sibling, 1 reply; 39+ messages in thread From: Jean Delvare @ 2008-01-21 17:15 UTC (permalink / raw) To: Felipe Balbi; +Cc: linux-kernel, david-b, tony, Linux I2C 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 ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 1/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 1 2008-01-21 17:15 ` [PATCH 1/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 1 Jean Delvare @ 2008-01-21 18:37 ` Felipe Balbi 0 siblings, 0 replies; 39+ messages in thread From: Felipe Balbi @ 2008-01-21 18:37 UTC (permalink / raw) To: Jean Delvare; +Cc: Felipe Balbi, linux-kernel, david-b, tony, Linux I2C 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 ^ permalink raw reply [flat|nested] 39+ messages in thread
end of thread, other threads:[~2008-03-16 11:07 UTC | newest]
Thread overview: 39+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-01-03 16:59 [PATCH 0/2] ISP1301 updates Felipe Balbi
2008-01-03 16:59 ` [PATCH 1/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 1 Felipe Balbi
2008-01-03 16:59 ` [PATCH 2/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 2 Felipe Balbi
2008-01-22 12:01 ` Jean Delvare
[not found] ` <20080122130158.4d4d44dc-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>
2008-01-22 12:13 ` Felipe Balbi
2008-01-22 12:13 ` Felipe Balbi
2008-01-22 17:56 ` Jean Delvare
[not found] ` <20080122185615.4a8ebbea-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>
2008-02-23 16:52 ` Jean Delvare
[not found] ` <20080223175233.7d2f358e-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>
2008-02-23 17:01 ` Felipe Balbi
[not found] ` <31e679430802230901j66e17535w6ac8ec57e60e5c6b-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2008-02-23 17:11 ` Jean Delvare
[not found] ` <20080223181120.30e9f63a-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>
2008-02-23 17:13 ` Felipe Balbi
[not found] ` <31e679430802230913m33d8457x1760399183c56ced-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2008-02-23 17:53 ` David Brownell
[not found] ` <20080223175356.95681BA69D-ZcXrCSuhvln6VZ3dlLfH/g4gEjPzgfUyLrfjE7I9kuVHxeISYlDBzl6hYfS7NtTn@public.gmane.org>
2008-02-23 18:00 ` Felipe Balbi
[not found] ` <31e679430802231000i4a360269v5d0bc61fc21fd7c3-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2008-02-23 18:10 ` David Brownell
[not found] ` <200802231010.40108.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>
2008-02-23 19:00 ` Felipe Balbi
[not found] ` <31e679430802231100r19df9157x9bbd910a592f4392-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2008-02-23 19:08 ` Felipe Balbi
[not found] ` <31e679430802231108k613bdd9ds2782aed429b65b50-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2008-02-23 19:48 ` Jean Delvare
2008-02-23 19:28 ` Jean Delvare
[not found] ` <20080223202832.4a953910-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>
2008-02-23 19:53 ` David Brownell
[not found] ` <200802231153.04011.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>
2008-02-24 9:30 ` Jean Delvare
[not found] ` <20080224103052.66da6803-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>
2008-02-24 11:05 ` felipebalbi-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f
[not found] ` <31e679430802240305o62ec88a9jfcc54f0a3be658ff-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2008-02-25 8:46 ` Felipe Balbi
[not found] ` <31e679430802250046h4df04aa0y6a32005e6d791e65-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2008-02-25 12:54 ` Felipe Balbi
[not found] ` <31e679430802250454k234e071alc9bbcb2e3b7e7605-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2008-03-06 6:28 ` David Brownell
[not found] ` <200803052228.47162.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>
2008-03-06 9:15 ` Felipe Balbi
2008-02-26 5:38 ` David Brownell
[not found] ` <200802252138.30301.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>
2008-02-26 9:22 ` Felipe Balbi
[not found] ` <31e679430802260122xc30356ayd98fc3c079e613b7-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2008-03-07 20:20 ` David Brownell
[not found] ` <200803071220.10744.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>
2008-03-07 21:00 ` Felipe Balbi
[not found] ` <31e679430803071300w2f2e80ddqd4516a976df4474-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2008-03-15 11:59 ` Jean Delvare
[not found] ` <20080315125940.2fe84f24-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>
2008-03-15 12:04 ` Felipe Balbi
2008-03-16 3:57 ` David Brownell
[not found] ` <200803152057.19375.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>
2008-03-16 10:22 ` Felipe Balbi
[not found] ` <31e679430803160322v7b8d6ecepf03bd8b87d975553-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2008-03-16 10:40 ` Jean Delvare
[not found] ` <20080316114043.57e38dd7-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>
2008-03-16 11:07 ` Felipe Balbi
2008-02-28 21:50 ` Jean Delvare
[not found] ` <20080228225032.61f07a18-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>
2008-02-28 21:53 ` Felipe Balbi
2008-01-21 17:15 ` [PATCH 1/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 1 Jean Delvare
2008-01-21 18:37 ` Felipe Balbi
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.