* [patch 2.6.25-git] i2c: smbalert# support [not found] ` <200804161027.49943.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> @ 2008-05-02 11:10 ` David Brownell [not found] ` <200805020410.44723.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> 0 siblings, 1 reply; 29+ messages in thread From: David Brownell @ 2008-05-02 11:10 UTC (permalink / raw) To: i2c-GZX6beZjE8VD60Wz+7aTrA Infrastructure supporting SMBALERT# interrupts and the related SMBus protocols. These are defined as "optional" by the spec. - The i2c_adapter now includes a work_struct doing the work of talking to the Alert Response Address until nobody responds any more (and hence the IRQ is no longer asserted). Follows SMBus 2.0 not 1.1; there seems to be no point in trying to handle ten-bit addresses. - Some of the ways that work_struct could be driven: * Adapter driver provides an IRQ, which is bound to a handler which schedules that work_struct (using keventd for now). NOTE: it's nicest if this is edge triggered, but the code should handle level triggered IRQs too. * Adapter driver schedules that work struct itself, maybe even on a workqueue of its own. It asks the core to set it up by setting i2c_adapter.do_setup_alert ... SMBALERT# could be a subcase of the adapter's normal interrupt handler. (Or, some boards may want to use polling.) - The "i2c-gpio" driver now handles an optional named resource for that SMBus alert signal. Named since, when this is substituted for a misbehaving "native" driver, positional ids should be left alone. (It might be better to put this logic into i2c core, to apply whenever the i2c_adapter.dev.parent is a platform device.) - There's a new driver method used to report that a given device has issued an alert. Its parameter includes the one bit of information provided by the device in its alert response message. The IRQ driven code path is always enabled, if it's available. Signed-off-by: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org> --- CHANGES since last post: cope with updated "new style binding"; work better with (preferred) edge triggered IRQs. drivers/i2c/busses/i2c-gpio.c | 10 ++ drivers/i2c/i2c-core.c | 146 ++++++++++++++++++++++++++++++++++++++++++ include/linux/i2c.h | 14 ++++ 3 files changed, 170 insertions(+) --- a/drivers/i2c/busses/i2c-gpio.c 2008-05-02 00:15:29.000000000 -0700 +++ b/drivers/i2c/busses/i2c-gpio.c 2008-05-02 01:53:33.000000000 -0700 @@ -82,6 +82,7 @@ static int __init i2c_gpio_probe(struct struct i2c_gpio_platform_data *pdata; struct i2c_algo_bit_data *bit_data; struct i2c_adapter *adap; + struct resource *smbalert; int ret; pdata = pdev->dev.platform_data; @@ -143,6 +144,15 @@ static int __init i2c_gpio_probe(struct adap->class = I2C_CLASS_HWMON; adap->dev.parent = &pdev->dev; + smbalert = platform_get_resource_byname(pdev, IORESOURCE_IRQ, + "smbalert#"); + if (smbalert) { + adap->irq = smbalert->start; + if ((IORESOURCE_IRQ_LOWEDGE | IORESOURCE_IRQ_HIGHEDGE) + & smbalert->flags) + adap->alert_edge_triggered = 1; + } + /* * If "dev->id" is negative we consider it as zero. * The reason to do so is to avoid sysfs names that only make --- a/drivers/i2c/i2c-core.c 2008-05-02 00:15:29.000000000 -0700 +++ b/drivers/i2c/i2c-core.c 2008-05-02 01:52:36.000000000 -0700 @@ -33,6 +33,8 @@ #include <linux/platform_device.h> #include <linux/mutex.h> #include <linux/completion.h> +#include <linux/interrupt.h> + #include <linux/hardirq.h> #include <linux/irqflags.h> #include <linux/semaphore.h> @@ -433,6 +435,108 @@ static int i2c_do_add_adapter(struct dev return 0; } +/* + * The IRQ handler needs to hand work off to a task which can issue SMBus + * calls, because those sleeping calls can't be made in IRQ context. + */ +static void smbus_alert(struct work_struct *work) +{ + struct i2c_adapter *bus; + + bus = container_of(work, struct i2c_adapter, alert); + for (;;) { + s32 status; + unsigned short addr; + struct i2c_client *client; + bool flag; + + /* Devices with pending alerts reply in address order, low + * to high, because of arbitration. After responding, an + * SMBus device stops asserting SMBALERT# ... so we can + * re-enable the IRQ as soon as read_byte() gets no reply. + * + * NOTE that SMBus 2.0 reserves 10-bit addresess for future + * use. We neither handle them, nor try to use PEC here. + */ + status = i2c_smbus_read_byte(bus->ara); + if (status < 0) + break; + flag = status & 1; + addr = status >> 1; + + dev_dbg(&bus->dev, "SMBALERT# %d from dev 0x%02x\n", flag, addr); + + /* Notify any driver for the device which issued the alert. + * The locking ensures it won't disappear while we do that. + */ + mutex_lock(&core_lock); + list_for_each_entry(client, &bus->clients, list) { + if (client->addr != addr) + continue; + if (client->flags & I2C_CLIENT_TEN) + continue; + if (!client->driver) + break; + + /* Drivers should either disable alerts or provide + * at least a minimal handler. + * + * REVISIT: drop lock while we call alert(). + */ + if (client->driver->alert) + client->driver->alert(client, flag); + else + dev_warn(&client->dev, "no driver alert()!\n"); + break; + } + mutex_unlock(&core_lock); + } + + /* reenable level-triggered IRQs */ + if (!bus->alert_edge_triggered) + enable_irq(bus->irq); +} + +static irqreturn_t smbus_irq(int irq, void *adap) +{ + struct i2c_adapter *bus = adap; + + /* disable level-triggered IRQs until we handle them */ + if (!bus->alert_edge_triggered) + disable_irq_nosync(irq); + + schedule_work(&bus->alert); + return IRQ_HANDLED; +} + +static int smbalert_probe(struct i2c_client *c, const struct i2c_device_id *id) +{ + return 0; +} + +static int smbalert_remove(struct i2c_client *c) +{ + return 0; +} + +static const struct i2c_device_id smbalert_ids[] = { + { "smbus_alert", 0, }, + { /* LIST END */ }, +}; + +static struct i2c_driver smbalert_driver = { + .driver = { + .name = "smbus_alert", + }, + .probe = smbalert_probe, + .remove = smbalert_remove, + .id_table = smbalert_ids, +}; + +static const struct i2c_board_info ara_board_info = { + I2C_BOARD_INFO("smbus_alert", 0x0c), +}; + static int i2c_register_adapter(struct i2c_adapter *adap) { int res = 0, dummy; @@ -441,6 +545,9 @@ static int i2c_register_adapter(struct i mutex_init(&adap->clist_lock); INIT_LIST_HEAD(&adap->clients); + /* Setup SMBALERT# infrastructure. */ + INIT_WORK(&adap->alert, smbus_alert); + mutex_lock(&core_lock); /* Add the adapter to the driver core. @@ -459,6 +566,33 @@ static int i2c_register_adapter(struct i if (res) goto out_list; + /* If we'll be handling SMBus alerts, register the alert responder + * address so that nobody else can accidentally claim it. + * Handling can be done either through our IRQ handler, or by the + * adapter (from its handler, periodic polling, or whatever). + * + * NOTE that if we manage the IRQ, we *MUST* know if it's level or + * edge triggered in order to hand it to the workqueue correctly. + * If triggering the alert seems to wedge the system, you probably + * should have said it's level triggered. + */ + if (adap->irq > 0 || adap->do_setup_alert) + adap->ara = i2c_new_device(adap, &ara_board_info); + + if (adap->irq > 0 && adap->ara) { + res = devm_request_irq(&adap->dev, adap->irq, smbus_irq, + 0, "smbus_alert", adap); + if (res == 0) { + dev_info(&adap->dev, + "supports SMBALERT#, %s trigger\n", + adap->alert_edge_triggered + ? "edge" : "level"); + adap->has_alert_irq = 1; + } else + res = 0; + + } + dev_dbg(&adap->dev, "adapter [%s] registered\n", adap->name); /* create pre-declared device nodes for new-style drivers */ @@ -635,6 +769,12 @@ int i2c_del_adapter(struct i2c_adapter * } } + if (adap->has_alert_irq) { + devm_free_irq(&adap->dev, adap->irq, adap); + adap->has_alert_irq = 0; + } + cancel_work_sync(&adap->alert); + /* clean up the sysfs representation */ init_completion(&adap->dev_released); device_unregister(&adap->dev); @@ -922,6 +1062,9 @@ static int __init i2c_init(void) retval = bus_register(&i2c_bus_type); if (retval) return retval; + retval = i2c_add_driver(&smbalert_driver); + if (retval) + goto alert_err; retval = class_register(&i2c_adapter_class); if (retval) goto bus_err; @@ -933,6 +1076,8 @@ static int __init i2c_init(void) class_err: class_unregister(&i2c_adapter_class); bus_err: + i2c_del_driver(&smbalert_driver); +alert_err: bus_unregister(&i2c_bus_type); return retval; } @@ -941,6 +1086,7 @@ static void __exit i2c_exit(void) { i2c_del_driver(&dummy_driver); class_unregister(&i2c_adapter_class); + i2c_del_driver(&smbalert_driver); bus_unregister(&i2c_bus_type); } --- a/include/linux/i2c.h 2008-05-02 00:15:29.000000000 -0700 +++ b/include/linux/i2c.h 2008-05-02 01:53:43.000000000 -0700 @@ -34,6 +34,7 @@ #include <linux/device.h> /* for struct device */ #include <linux/sched.h> /* for completion */ #include <linux/mutex.h> +#include <linux/workqueue.h> /* --- General options ------------------------------------------------ */ @@ -134,6 +135,11 @@ struct i2c_driver { int (*suspend)(struct i2c_client *, pm_message_t mesg); int (*resume)(struct i2c_client *); + /* SMBus alert protocol support; the low bit of the code sometimes + * passes event data (e.g. exceeding upper vs lower limit). + */ + void (*alert)(struct i2c_client *, bool flag); + /* a ioctl like command that can be used to perform specific functions * with the device. */ @@ -332,6 +338,14 @@ struct i2c_adapter { struct list_head clients; /* DEPRECATED */ char name[48]; struct completion dev_released; + + /* SMBALERT# support */ + unsigned do_setup_alert:1; + unsigned has_alert_irq:1; + unsigned alert_edge_triggered:1; + int irq; + struct i2c_client *ara; + struct work_struct alert; }; #define to_i2c_adapter(d) container_of(d, struct i2c_adapter, dev) _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply [flat|nested] 29+ messages in thread
[parent not found: <200805020410.44723.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>]
* [patch 2.6.265-rc1] i2c: smbalert# support [not found] ` <200805020410.44723.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> @ 2008-05-05 5:56 ` David Brownell [not found] ` <200805042256.49252.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> 0 siblings, 1 reply; 29+ messages in thread From: David Brownell @ 2008-05-05 5:56 UTC (permalink / raw) To: i2c-GZX6beZjE8VD60Wz+7aTrA Infrastructure supporting SMBALERT# interrupts and the related SMBus protocols. These are defined as "optional" by the spec. - The i2c_adapter now includes a work_struct doing the work of talking to the Alert Response Address until nobody responds any more (and hence the IRQ is no longer asserted). Follows SMBus 2.0 not 1.1; there seems to be no point in trying to handle ten-bit addresses. - Some of the ways that work_struct could be driven: * Adapter driver provides an IRQ, which is bound to a handler which schedules that work_struct (using keventd for now). NOTE: it's nicest if this is edge triggered, but the code should handle level triggered IRQs too. * Adapter driver schedules that work struct itself, maybe even on a workqueue of its own. It asks the core to set it up by setting i2c_adapter.do_setup_alert ... SMBALERT# could be a subcase of the adapter's normal interrupt handler. (Or, some boards may want to use polling.) - The "i2c-gpio" driver now handles an optional named resource for that SMBus alert signal. Named since, when this is substituted for a misbehaving "native" driver, positional ids should be left alone. (It might be better to put this logic into i2c core, to apply whenever the i2c_adapter.dev.parent is a platform device.) - There's a new driver method used to report that a given device has issued an alert. Its parameter includes the one bit of information provided by the device in its alert response message. The IRQ driven code path is always enabled, if it's available. Signed-off-by: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org> --- Updated to stop using the i2c_adapter.clients list. Resolves an commented-on locking issue. drivers/i2c/busses/i2c-gpio.c | 10 ++ drivers/i2c/i2c-core.c | 155 ++++++++++++++++++++++++++++++++++++++++++ include/linux/i2c.h | 14 +++ 3 files changed, 179 insertions(+) --- a/drivers/i2c/busses/i2c-gpio.c 2008-05-03 21:17:10.000000000 -0700 +++ b/drivers/i2c/busses/i2c-gpio.c 2008-05-03 21:21:21.000000000 -0700 @@ -82,6 +82,7 @@ static int __init i2c_gpio_probe(struct struct i2c_gpio_platform_data *pdata; struct i2c_algo_bit_data *bit_data; struct i2c_adapter *adap; + struct resource *smbalert; int ret; pdata = pdev->dev.platform_data; @@ -143,6 +144,15 @@ static int __init i2c_gpio_probe(struct adap->class = I2C_CLASS_HWMON; adap->dev.parent = &pdev->dev; + smbalert = platform_get_resource_byname(pdev, IORESOURCE_IRQ, + "smbalert#"); + if (smbalert) { + adap->irq = smbalert->start; + if ((IORESOURCE_IRQ_LOWEDGE | IORESOURCE_IRQ_HIGHEDGE) + & smbalert->flags) + adap->alert_edge_triggered = 1; + } + /* * If "dev->id" is negative we consider it as zero. * The reason to do so is to avoid sysfs names that only make --- a/drivers/i2c/i2c-core.c 2008-05-03 21:21:17.000000000 -0700 +++ b/drivers/i2c/i2c-core.c 2008-05-04 18:17:10.000000000 -0700 @@ -33,6 +33,8 @@ #include <linux/platform_device.h> #include <linux/mutex.h> #include <linux/completion.h> +#include <linux/interrupt.h> + #include <linux/hardirq.h> #include <linux/irqflags.h> #include <linux/semaphore.h> @@ -405,6 +407,117 @@ static struct class i2c_adapter_class = .dev_attrs = i2c_adapter_attrs, }; +struct alert_data { + unsigned short addr; + bool flag; +}; + +/* If this is the alerting device, notify its driver. */ +static int i2c_do_alert(struct device *dev, void *addrp) +{ + struct i2c_client *client = i2c_verify_client(dev); + struct alert_data *data = addrp; + + if (!client || client->addr != data->addr) + return 0; + if (client->flags & I2C_CLIENT_TEN) + return 0; + + /* Drivers should either disable alerts, or provide at least + * a minimal handler. Lock so client->driver won't change. + */ + down(&dev->sem); + if (client->driver) { + if (client->driver->alert) + client->driver->alert(client, data->flag); + else + dev_warn(&client->dev, "no driver alert()!\n"); + } else + dev_dbg(&client->dev, "alert with no driver\n"); + up(&dev->sem); + + /* Stop iterating after we find the device. */ + return -EBUSY; +} + +/* + * The alert IRQ handler needs to hand work off to a task which can issue + * SMBus calls, because those sleeping calls can't be made in IRQ context. + */ +static void smbus_alert(struct work_struct *work) +{ + struct i2c_adapter *bus; + + bus = container_of(work, struct i2c_adapter, alert); + for (;;) { + s32 status; + struct alert_data data; + + /* Devices with pending alerts reply in address order, low + * to high, because of slave transmit arbitration. After + * responding, an SMBus device stops asserting SMBALERT#. + * + * NOTE that SMBus 2.0 reserves 10-bit addresess for future + * use. We neither handle them, nor try to use PEC here. + */ + status = i2c_smbus_read_byte(bus->ara); + if (status < 0) + break; + + data.flag = (status & 1) != 0; + data.addr = status >> 1; + dev_dbg(&bus->dev, "SMBALERT# %s from dev 0x%02x\n", + data.flag ? "true" : "false", data.addr); + + /* Notify driver for the device which issued the alert. */ + device_for_each_child(&bus->dev, &data, i2c_do_alert); + } + + /* We handled all alerts; reenable level-triggered IRQs. */ + if (!bus->alert_edge_triggered) + enable_irq(bus->irq); +} + +static irqreturn_t smbus_irq(int irq, void *adap) +{ + struct i2c_adapter *bus = adap; + + /* Disable level-triggered IRQs until we handle them. */ + if (!bus->alert_edge_triggered) + disable_irq_nosync(irq); + + schedule_work(&bus->alert); + return IRQ_HANDLED; +} + +static int smbalert_probe(struct i2c_client *c, const struct i2c_device_id *id) +{ + return 0; +} + +static int smbalert_remove(struct i2c_client *c) +{ + return 0; +} + +static const struct i2c_device_id smbalert_ids[] = { + { "smbus_alert", 0, }, + { /* LIST END */ } +}; + +static struct i2c_driver smbalert_driver = { + .driver = { + .name = "smbus_alert", + }, + .probe = smbalert_probe, + .remove = smbalert_remove, + .id_table = smbalert_ids, +}; + +static const struct i2c_board_info ara_board_info = { + I2C_BOARD_INFO("smbus_alert", 0x0c), +}; + static void i2c_scan_static_board_info(struct i2c_adapter *adapter) { struct i2c_devinfo *devinfo; @@ -441,6 +554,9 @@ static int i2c_register_adapter(struct i mutex_init(&adap->clist_lock); INIT_LIST_HEAD(&adap->clients); + /* Setup SMBALERT# infrastructure. */ + INIT_WORK(&adap->alert, smbus_alert); + mutex_lock(&core_lock); /* Add the adapter to the driver core. @@ -459,6 +575,33 @@ static int i2c_register_adapter(struct i if (res) goto out_list; + /* If we'll be handling SMBus alerts, register the alert responder + * address so that nobody else can accidentally claim it. + * Handling can be done either through our IRQ handler, or by the + * adapter (from its handler, periodic polling, or whatever). + * + * NOTE that if we manage the IRQ, we *MUST* know if it's level or + * edge triggered in order to hand it to the workqueue correctly. + * If triggering the alert seems to wedge the system, you probably + * should have said it's level triggered. + */ + if (adap->irq > 0 || adap->do_setup_alert) + adap->ara = i2c_new_device(adap, &ara_board_info); + + if (adap->irq > 0 && adap->ara) { + res = devm_request_irq(&adap->dev, adap->irq, smbus_irq, + 0, "smbus_alert", adap); + if (res == 0) { + dev_info(&adap->dev, + "supports SMBALERT#, %s trigger\n", + adap->alert_edge_triggered + ? "edge" : "level"); + adap->has_alert_irq = 1; + } else + res = 0; + + } + dev_dbg(&adap->dev, "adapter [%s] registered\n", adap->name); /* create pre-declared device nodes for new-style drivers */ @@ -635,6 +778,12 @@ int i2c_del_adapter(struct i2c_adapter * } } + if (adap->has_alert_irq) { + devm_free_irq(&adap->dev, adap->irq, adap); + adap->has_alert_irq = 0; + } + cancel_work_sync(&adap->alert); + /* clean up the sysfs representation */ init_completion(&adap->dev_released); device_unregister(&adap->dev); @@ -922,6 +1071,9 @@ static int __init i2c_init(void) retval = bus_register(&i2c_bus_type); if (retval) return retval; + retval = i2c_add_driver(&smbalert_driver); + if (retval) + goto alert_err; retval = class_register(&i2c_adapter_class); if (retval) goto bus_err; @@ -933,6 +1085,8 @@ static int __init i2c_init(void) class_err: class_unregister(&i2c_adapter_class); bus_err: + i2c_del_driver(&smbalert_driver); +alert_err: bus_unregister(&i2c_bus_type); return retval; } @@ -941,6 +1095,7 @@ static void __exit i2c_exit(void) { i2c_del_driver(&dummy_driver); class_unregister(&i2c_adapter_class); + i2c_del_driver(&smbalert_driver); bus_unregister(&i2c_bus_type); } --- a/include/linux/i2c.h 2008-05-03 21:17:10.000000000 -0700 +++ b/include/linux/i2c.h 2008-05-03 21:21:21.000000000 -0700 @@ -34,6 +34,7 @@ #include <linux/device.h> /* for struct device */ #include <linux/sched.h> /* for completion */ #include <linux/mutex.h> +#include <linux/workqueue.h> /* --- General options ------------------------------------------------ */ @@ -134,6 +135,11 @@ struct i2c_driver { int (*suspend)(struct i2c_client *, pm_message_t mesg); int (*resume)(struct i2c_client *); + /* SMBus alert protocol support; the low bit of the code sometimes + * passes event data (e.g. exceeding upper vs lower limit). + */ + void (*alert)(struct i2c_client *, bool flag); + /* a ioctl like command that can be used to perform specific functions * with the device. */ @@ -332,6 +338,14 @@ struct i2c_adapter { struct list_head clients; /* DEPRECATED */ char name[48]; struct completion dev_released; + + /* SMBALERT# support */ + unsigned do_setup_alert:1; + unsigned has_alert_irq:1; + unsigned alert_edge_triggered:1; + int irq; + struct i2c_client *ara; + struct work_struct alert; }; #define to_i2c_adapter(d) container_of(d, struct i2c_adapter, dev) _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply [flat|nested] 29+ messages in thread
[parent not found: <200805042256.49252.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>]
* [patch 2.6.27-rc7] i2c: smbalert# support [not found] ` <200805042256.49252.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> @ 2008-09-23 22:32 ` David Brownell [not found] ` <200809231532.40083.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> 0 siblings, 1 reply; 29+ messages in thread From: David Brownell @ 2008-09-23 22:32 UTC (permalink / raw) To: i2c-GZX6beZjE8VD60Wz+7aTrA; +Cc: lm-sensors-GZX6beZjE8VD60Wz+7aTrA From: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org> Infrastructure supporting SMBALERT# interrupts and the related SMBus protocols. These are defined as "optional" by the SMBus spec. - The i2c_adapter now includes a work_struct doing the work of talking to the Alert Response Address until nobody responds any more (and hence the IRQ is no longer asserted). Follows SMBus 2.0 not 1.1; there seems to be no point in trying to handle ten-bit addresses. - Some of the ways that work_struct could be driven: * Adapter driver provides an IRQ, which is bound to a handler which schedules that work_struct (using keventd for now). NOTE: it's nicest if this is edge triggered, but the code should handle level triggered IRQs too. * Adapter driver schedules that work struct itself, maybe even on a workqueue of its own. It asks the core to set it up by setting i2c_adapter.do_setup_alert ... SMBALERT# could be a subcase of the adapter's normal interrupt handler. (Or, some boards may want to use polling.) - The "i2c-gpio" driver now handles an optional named resource for that SMBus alert signal. Named since, when this is substituted for a misbehaving "native" driver, positional ids should be left alone. (It might be better to put this logic into i2c core, to apply whenever the i2c_adapter.dev.parent is a platform device.) - There's a new driver method used to report that a given device has issued an alert. Its parameter includes the one bit of information provided by the device in its alert response message. The IRQ driven code path is always enabled, if it's available. Signed-off-by: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org> --- Just another semi-regular resend of this. I noticed this code should work OK with at least the ICH8 SMBus adapter, which has one interrupt status bit reserved for SMBALERT# ... an IRQ-driven version of that driver would ack then schedule_work(&bus->alert). For now I'd expect alerts to be most useful on non-PC boards. drivers/i2c/busses/i2c-gpio.c | 10 ++ drivers/i2c/i2c-core.c | 155 ++++++++++++++++++++++++++++++++++++++++ include/linux/i2c.h | 14 +++ 3 files changed, 179 insertions(+) --- a/drivers/i2c/busses/i2c-gpio.c +++ b/drivers/i2c/busses/i2c-gpio.c @@ -82,6 +82,7 @@ static int __devinit i2c_gpio_probe(stru struct i2c_gpio_platform_data *pdata; struct i2c_algo_bit_data *bit_data; struct i2c_adapter *adap; + struct resource *smbalert; int ret; pdata = pdev->dev.platform_data; @@ -143,6 +144,15 @@ static int __devinit i2c_gpio_probe(stru adap->class = I2C_CLASS_HWMON | I2C_CLASS_SPD; adap->dev.parent = &pdev->dev; + smbalert = platform_get_resource_byname(pdev, IORESOURCE_IRQ, + "smbalert#"); + if (smbalert) { + adap->irq = smbalert->start; + if ((IORESOURCE_IRQ_LOWEDGE | IORESOURCE_IRQ_HIGHEDGE) + & smbalert->flags) + adap->alert_edge_triggered = 1; + } + /* * If "dev->id" is negative we consider it as zero. * The reason to do so is to avoid sysfs names that only make --- a/drivers/i2c/i2c-core.c +++ b/drivers/i2c/i2c-core.c @@ -32,6 +32,8 @@ #include <linux/platform_device.h> #include <linux/mutex.h> #include <linux/completion.h> +#include <linux/interrupt.h> + #include <linux/hardirq.h> #include <linux/irqflags.h> #include <asm/uaccess.h> @@ -401,6 +403,117 @@ static struct class i2c_adapter_class = .dev_attrs = i2c_adapter_attrs, }; +struct alert_data { + unsigned short addr; + bool flag; +}; + +/* If this is the alerting device, notify its driver. */ +static int i2c_do_alert(struct device *dev, void *addrp) +{ + struct i2c_client *client = i2c_verify_client(dev); + struct alert_data *data = addrp; + + if (!client || client->addr != data->addr) + return 0; + if (client->flags & I2C_CLIENT_TEN) + return 0; + + /* Drivers should either disable alerts, or provide at least + * a minimal handler. Lock so client->driver won't change. + */ + down(&dev->sem); + if (client->driver) { + if (client->driver->alert) + client->driver->alert(client, data->flag); + else + dev_warn(&client->dev, "no driver alert()!\n"); + } else + dev_dbg(&client->dev, "alert with no driver\n"); + up(&dev->sem); + + /* Stop iterating after we find the device. */ + return -EBUSY; +} + +/* + * The alert IRQ handler needs to hand work off to a task which can issue + * SMBus calls, because those sleeping calls can't be made in IRQ context. + */ +static void smbus_alert(struct work_struct *work) +{ + struct i2c_adapter *bus; + + bus = container_of(work, struct i2c_adapter, alert); + for (;;) { + s32 status; + struct alert_data data; + + /* Devices with pending alerts reply in address order, low + * to high, because of slave transmit arbitration. After + * responding, an SMBus device stops asserting SMBALERT#. + * + * NOTE that SMBus 2.0 reserves 10-bit addresess for future + * use. We neither handle them, nor try to use PEC here. + */ + status = i2c_smbus_read_byte(bus->ara); + if (status < 0) + break; + + data.flag = (status & 1) != 0; + data.addr = status >> 1; + dev_dbg(&bus->dev, "SMBALERT# %s from dev 0x%02x\n", + data.flag ? "true" : "false", data.addr); + + /* Notify driver for the device which issued the alert. */ + device_for_each_child(&bus->dev, &data, i2c_do_alert); + } + + /* We handled all alerts; reenable level-triggered IRQs. */ + if (!bus->alert_edge_triggered) + enable_irq(bus->irq); +} + +static irqreturn_t smbus_irq(int irq, void *adap) +{ + struct i2c_adapter *bus = adap; + + /* Disable level-triggered IRQs until we handle them. */ + if (!bus->alert_edge_triggered) + disable_irq_nosync(irq); + + schedule_work(&bus->alert); + return IRQ_HANDLED; +} + +static int smbalert_probe(struct i2c_client *c, const struct i2c_device_id *id) +{ + return 0; +} + +static int smbalert_remove(struct i2c_client *c) +{ + return 0; +} + +static const struct i2c_device_id smbalert_ids[] = { + { "smbus_alert", 0, }, + { /* LIST END */ } +}; + +static struct i2c_driver smbalert_driver = { + .driver = { + .name = "smbus_alert", + }, + .probe = smbalert_probe, + .remove = smbalert_remove, + .id_table = smbalert_ids, +}; + +static const struct i2c_board_info ara_board_info = { + I2C_BOARD_INFO("smbus_alert", 0x0c), +}; + static void i2c_scan_static_board_info(struct i2c_adapter *adapter) { struct i2c_devinfo *devinfo; @@ -441,6 +554,9 @@ static int i2c_register_adapter(struct i mutex_init(&adap->clist_lock); INIT_LIST_HEAD(&adap->clients); + /* Setup SMBALERT# infrastructure. */ + INIT_WORK(&adap->alert, smbus_alert); + mutex_lock(&core_lock); /* Add the adapter to the driver core. @@ -459,6 +575,33 @@ static int i2c_register_adapter(struct i if (res) goto out_list; + /* If we'll be handling SMBus alerts, register the alert responder + * address so that nobody else can accidentally claim it. + * Handling can be done either through our IRQ handler, or by the + * adapter (from its handler, periodic polling, or whatever). + * + * NOTE that if we manage the IRQ, we *MUST* know if it's level or + * edge triggered in order to hand it to the workqueue correctly. + * If triggering the alert seems to wedge the system, you probably + * should have said it's level triggered. + */ + if (adap->irq > 0 || adap->do_setup_alert) + adap->ara = i2c_new_device(adap, &ara_board_info); + + if (adap->irq > 0 && adap->ara) { + res = devm_request_irq(&adap->dev, adap->irq, smbus_irq, + 0, "smbus_alert", adap); + if (res == 0) { + dev_info(&adap->dev, + "supports SMBALERT#, %s trigger\n", + adap->alert_edge_triggered + ? "edge" : "level"); + adap->has_alert_irq = 1; + } else + res = 0; + + } + dev_dbg(&adap->dev, "adapter [%s] registered\n", adap->name); /* create pre-declared device nodes for new-style drivers */ @@ -644,6 +787,12 @@ int i2c_del_adapter(struct i2c_adapter * } } + if (adap->has_alert_irq) { + devm_free_irq(&adap->dev, adap->irq, adap); + adap->has_alert_irq = 0; + } + cancel_work_sync(&adap->alert); + /* clean up the sysfs representation */ init_completion(&adap->dev_released); device_unregister(&adap->dev); @@ -959,12 +1108,17 @@ static int __init i2c_init(void) retval = class_register(&i2c_adapter_class); if (retval) goto bus_err; + retval = i2c_add_driver(&smbalert_driver); + if (retval) + goto alert_err; retval = i2c_add_driver(&dummy_driver); if (retval) goto class_err; return 0; class_err: + i2c_del_driver(&smbalert_driver); +alert_err: class_unregister(&i2c_adapter_class); bus_err: bus_unregister(&i2c_bus_type); @@ -975,6 +1129,7 @@ static void __exit i2c_exit(void) { i2c_del_driver(&dummy_driver); class_unregister(&i2c_adapter_class); + i2c_del_driver(&smbalert_driver); bus_unregister(&i2c_bus_type); } --- a/include/linux/i2c.h +++ b/include/linux/i2c.h @@ -34,6 +34,7 @@ #include <linux/device.h> /* for struct device */ #include <linux/sched.h> /* for completion */ #include <linux/mutex.h> +#include <linux/workqueue.h> extern struct bus_type i2c_bus_type; @@ -166,6 +167,11 @@ struct i2c_driver { int (*suspend)(struct i2c_client *, pm_message_t mesg); int (*resume)(struct i2c_client *); + /* SMBus alert protocol support. The alert response's low bit + * is the event flag (e.g. exceeding upper vs lower limit). + */ + void (*alert)(struct i2c_client *, bool flag); + /* a ioctl like command that can be used to perform specific functions * with the device. */ @@ -366,6 +372,14 @@ struct i2c_adapter { struct list_head clients; /* DEPRECATED */ char name[48]; struct completion dev_released; + + /* SMBALERT# support */ + unsigned do_setup_alert:1; + unsigned has_alert_irq:1; + unsigned alert_edge_triggered:1; + int irq; + struct i2c_client *ara; + struct work_struct alert; }; #define to_i2c_adapter(d) container_of(d, struct i2c_adapter, dev) _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply [flat|nested] 29+ messages in thread
[parent not found: <200809231532.40083.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>]
* Re: [lm-sensors] [patch 2.6.27-rc7] i2c: smbalert# support [not found] ` <200809231532.40083.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> @ 2008-09-26 1:07 ` Trent Piepho [not found] ` <Pine.LNX.4.64.0809251728130.7680-3bmvVOk6DZ+DGx/iekXGtrjh7zpefjiS@public.gmane.org> 2008-11-18 8:15 ` Jean Delvare 1 sibling, 1 reply; 29+ messages in thread From: Trent Piepho @ 2008-09-26 1:07 UTC (permalink / raw) To: David Brownell Cc: i2c-GZX6beZjE8VD60Wz+7aTrA, lm-sensors-GZX6beZjE8VD60Wz+7aTrA On Tue, 23 Sep 2008, David Brownell wrote: > From: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org> > > NOTE: it's nicest if this is edge triggered, but the code > should handle level triggered IRQs too. It seems like one could miss an IRQ with edge triggered interrupts. If a second interrupt from another device occurs while the first is asserted, then there won't be another edge and the second interrupt will be missed. If all the devices on the irq are smbalerts, then maybe you are ok. The smb poll should let you find all alerting devices even if they didn't generate an interrupt. But any devices that don't support smbalert won't be found. Level triggered interrupts don't have that problem, but it's necessary to disable the interrupt in the irq handler until it can be acked from the work queue, which isn't very nice if the irq is shared. I didn't try to use smbalert. I'm not sure if my i2c adapter supports it. I associated the irq with the lm63 instead of the i2c adapter and put the handler in the lm63 driver. The code in drivers/of/of_i2c.c will map the irq for me and add it to board_info for the lm63. A bigger problem is what happens while the alert continues. The LM63 will disable the alert irq until some code re-enables it. At that point it will generate a new alert irq at the next internal temperature conversion (default 16 Hz). So when does the irq get re-enabled? After acking it? Then the interrupt will be generated 16 times per second while the alert persists, is that desirable? _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply [flat|nested] 29+ messages in thread
[parent not found: <Pine.LNX.4.64.0809251728130.7680-3bmvVOk6DZ+DGx/iekXGtrjh7zpefjiS@public.gmane.org>]
* Re: [lm-sensors] [patch 2.6.27-rc7] i2c: smbalert# support [not found] ` <Pine.LNX.4.64.0809251728130.7680-3bmvVOk6DZ+DGx/iekXGtrjh7zpefjiS@public.gmane.org> @ 2008-09-26 2:01 ` David Brownell [not found] ` <200809251902.00201.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> 0 siblings, 1 reply; 29+ messages in thread From: David Brownell @ 2008-09-26 2:01 UTC (permalink / raw) To: Trent Piepho Cc: i2c-GZX6beZjE8VD60Wz+7aTrA, lm-sensors-GZX6beZjE8VD60Wz+7aTrA On Thursday 25 September 2008, Trent Piepho wrote: > On Tue, 23 Sep 2008, David Brownell wrote: > > From: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org> > > > > NOTE: it's nicest if this is edge triggered, but the code > > should handle level triggered IRQs too. > > It seems like one could miss an IRQ with edge triggered interrupts. If a > second interrupt from another device You mean, a second device pulling SMBALERT# low? No problem. SMB hosts must poll until no more devices respond to the Alert Response Address -- and thus nobody will be pulling it low. > occurs while the first is asserted, then > there won't be another edge and the second interrupt will be missed. SMBALERT# is defined to be level triggered, active low. But it's common to have only edge-triggered IRQ lines available. And there's an obvious equivalence between triggering once on a falling edge, and once when the edge has completely fallen. (And an obvious difference too, which is why "it's nicest if this is edge triggered.") So long that line isn't shared with device(s) not following the SMBALERT# protcool you're fine. By design and definition. > If all the devices on the irq are smbalerts, then maybe you are ok. If the line is not obeying SMBALERT# signaling rules ... your hardware design is deeply buggy, go and replace it. Or at least get them to stop claiming SMBALERT# support; they've just got some kind of hard-to-manage shared IRQ line. > I didn't try to use smbalert. I'm not sure if my i2c adapter supports it. It's not a question of the I2C adapter supporting it. It's a question of whether there's an SMBALERT# signal on your board, which can generate an interrupt. If it can, this patch has an IRQ handler which implements the SMBus alert protocol. Think of it as sideband signaling ... unknown to that adapter. > I associated the irq with the lm63 instead of the i2c adapter and put the > handler in the lm63 driver. The code in drivers/of/of_i2c.c will map the irq > for me and add it to board_info for the lm63. Which may be fine for your specific application. But it's not the SMBALERT# protocol, and that solution won't work when the system requires SMBALERT#. (I read the lm63 spec. It's very explicit about supporting that protocol. There are other ways to use its IRQ generation too.) > A bigger problem is what happens while the alert continues. The LM63 will > disable the alert irq until some code re-enables it. That is, it disables its own generation of the alert signal. IRQs are on the host side; the lm63 hardware can't disable them. > At that point it will > generate a new alert irq at the next internal temperature conversion (default > 16 Hz). So when does the irq get re-enabled? After acking it? Then the > interrupt will be generated 16 times per second while the alert persists, is > that desirable? That's your job, if you're writing an alert() handler. It'd probably be more intelligent not to re-enable the lm63 alert mode until the temperature is back in spec. The TMP75 sensor I used was much more intelligent about that. - Dave _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply [flat|nested] 29+ messages in thread
[parent not found: <200809251902.00201.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>]
* Re: [lm-sensors] [patch 2.6.27-rc7] i2c: smbalert# support [not found] ` <200809251902.00201.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> @ 2008-09-27 0:29 ` Trent Piepho [not found] ` <Pine.LNX.4.64.0809261652040.7680-3bmvVOk6DZ+DGx/iekXGtrjh7zpefjiS@public.gmane.org> 0 siblings, 1 reply; 29+ messages in thread From: Trent Piepho @ 2008-09-27 0:29 UTC (permalink / raw) To: David Brownell Cc: i2c-GZX6beZjE8VD60Wz+7aTrA, lm-sensors-GZX6beZjE8VD60Wz+7aTrA On Thu, 25 Sep 2008, David Brownell wrote: >> If all the devices on the irq are smbalerts, then maybe you are ok. > > If the line is not obeying SMBALERT# signaling rules ... your > hardware design is deeply buggy, go and replace it. Or at least > get them to stop claiming SMBALERT# support; they've just got > some kind of hard-to-manage shared IRQ line. CPUs only have so many IRQ lines. It might be necessary to share the hwmon's irq line with some other device. It seems like it should be possible to do safely if the irq is level triggered. The irq needs to be disabled in the lm63/smbalert irq handler and re-enabled when the hwmon's signal (if it was the interrupting device) has been turned off. Which means the other device's irq will be disabled during this time as well, which isn't very nice but if it's something like an ethernet phy is probably not a problem. >> I didn't try to use smbalert. I'm not sure if my i2c adapter supports it. > > It's not a question of the I2C adapter supporting it. It's a > question of whether there's an SMBALERT# signal on your board, > which can generate an interrupt. If it can, this patch has an > IRQ handler which implements the SMBus alert protocol. Think > of it as sideband signaling ... unknown to that adapter. If nothing is alerting, then the read from the ARA address won't get an ack. Some i2c adapters don't handle this so well. I've got two hwmon chips sharing an IRQ line, but they are on different adapters (the lm63 only has one address). It could be a problem if one adapter's handler re-enabled a level triggered irq before the other handler has run. It should be ok if the other handle has started by not finished since the disabled irqs should stack. If the irq is edge triggered the one could get lost: chip1 (or random other thing on irq) generates irq adapter2's handler runs, gets no ARA respons chip2 alerts, line still low so no edge and no irq adapter1's handler runs, gets chip1's ARA response chip1 releases irq, but chip2 is still holding it low Now the irq is wedged. >> At that point it will >> generate a new alert irq at the next internal temperature conversion (default >> 16 Hz). So when does the irq get re-enabled? After acking it? Then the >> interrupt will be generated 16 times per second while the alert persists, is >> that desirable? > > That's your job, if you're writing an alert() handler. It'd > probably be more intelligent not to re-enable the lm63 alert > mode until the temperature is back in spec. The TMP75 sensor > I used was much more intelligent about that. I guess that was more of an RFC for how userspace notification of alert irqs should work. Is it ok if sysfs_notify() keeps getting called while the alert persists, or should it open happen when the alert changes? The latter probably makes the most sense. Harder to do, as the lm63 doesn't produce an "alarm stopped" signal. _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply [flat|nested] 29+ messages in thread
[parent not found: <Pine.LNX.4.64.0809261652040.7680-3bmvVOk6DZ+DGx/iekXGtrjh7zpefjiS@public.gmane.org>]
* Re: [lm-sensors] [patch 2.6.27-rc7] i2c: smbalert# support [not found] ` <Pine.LNX.4.64.0809261652040.7680-3bmvVOk6DZ+DGx/iekXGtrjh7zpefjiS@public.gmane.org> @ 2008-10-17 19:04 ` David Brownell [not found] ` <200810171204.54448.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> 0 siblings, 1 reply; 29+ messages in thread From: David Brownell @ 2008-10-17 19:04 UTC (permalink / raw) To: Trent Piepho Cc: i2c-GZX6beZjE8VD60Wz+7aTrA, lm-sensors-GZX6beZjE8VD60Wz+7aTrA ... just to recap this before I scrub old mail from my mbox: On Friday 26 September 2008, Trent Piepho wrote: > > It's not a question of the I2C adapter supporting it. It's a > > question of whether there's an SMBALERT# signal on your board, > > which can generate an interrupt. If it can, this patch has an > > IRQ handler which implements the SMBus alert protocol. Think > > of it as sideband signaling ... unknown to that adapter. > > If nothing is alerting, then the read from the ARA address won't get an ack. This is indeed how the SMBALERT# protocol handling code must detect that all pending alerts have been handled: each read of ARA clears one pending alert, until none responds any more. (And the active-low open drain SMBALERT# signal went high.) > Some i2c adapters don't handle this so well. If your system can't implement the SMBALERT# protocol, then there's no problem at all. Just handle the IRQ using whatever non-standard hackery is necessary to cope with that hardware, and DO NOT tell Linux that it's really SMBALERT#. Simple. The SMBALERT# protocol is very specific about how it acts and what it does. It's also very specific to SMBus. The $SUBJECT patch never claimed to be an attempt to work with arbitrary collections of I2C device interrupts that weren't designed to cooperate at all, much less to follow the SMBALERT# rules. Absolutely nothing you've said makes a real argument against providing support for SMBALERT#. At best, you've made an argument that some drivers may need to support less-efficient IRQ dispatching schemes as well as SMBALERT#, in order to handle board-specific design quirks. - Dave _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c ^ permalink raw reply [flat|nested] 29+ messages in thread
[parent not found: <200810171204.54448.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>]
* Re: [lm-sensors] [patch 2.6.27-rc7] i2c: smbalert# support [not found] ` <200810171204.54448.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> @ 2008-10-17 20:43 ` Trent Piepho 0 siblings, 0 replies; 29+ messages in thread From: Trent Piepho @ 2008-10-17 20:43 UTC (permalink / raw) To: David Brownell Cc: lm-sensors-GZX6beZjE8VD60Wz+7aTrA, Trent Piepho, i2c-GZX6beZjE8VD60Wz+7aTrA [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: TEXT/PLAIN; charset=X-UNKNOWN, Size: 887 bytes --] On Fri, 17 Oct 2008, David Brownell wrote: > ... just to recap this before I scrub old mail from my mbox: > > On Friday 26 September 2008, Trent Piepho wrote: > > > It's not a question of the I2C adapter supporting it. It's a > > > question of whether there's an SMBALERT# signal on your board, > > > which can generate an interrupt. If it can, this patch has an > > > IRQ handler which implements the SMBus alert protocol. Think > > > of it as sideband signaling ... unknown to that adapter. > > > > If nothing is alerting, then the read from the ARA address won't get an ack. > > Absolutely nothing you've said makes a real argument against > providing support for SMBALERT#. Which isn't surprising, since I wasn't trying to make an argument against providing support for SMBALERT#. I was making an argument for why it might be necessary to do something else. [-- Attachment #2: 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 [flat|nested] 29+ messages in thread
* Re: [patch 2.6.27-rc7] i2c: smbalert# support [not found] ` <200809231532.40083.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> 2008-09-26 1:07 ` [lm-sensors] " Trent Piepho @ 2008-11-18 8:15 ` Jean Delvare [not found] ` <20081118091546.421d6b78-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org> 1 sibling, 1 reply; 29+ messages in thread From: Jean Delvare @ 2008-11-18 8:15 UTC (permalink / raw) To: David Brownell; +Cc: Linux I2C, lm-sensors-GZX6beZjE8VD60Wz+7aTrA Hi Dave, On Tue, 23 Sep 2008 15:32:39 -0700, David Brownell wrote: > From: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org> > > Infrastructure supporting SMBALERT# interrupts and the related SMBus > protocols. These are defined as "optional" by the SMBus spec. > > - The i2c_adapter now includes a work_struct doing the work of talking > to the Alert Response Address until nobody responds any more (and > hence the IRQ is no longer asserted). Follows SMBus 2.0 not 1.1; > there seems to be no point in trying to handle ten-bit addresses. > > - Some of the ways that work_struct could be driven: > > * Adapter driver provides an IRQ, which is bound to a handler > which schedules that work_struct (using keventd for now). > NOTE: it's nicest if this is edge triggered, but the code > should handle level triggered IRQs too. > > * Adapter driver schedules that work struct itself, maybe even > on a workqueue of its own. It asks the core to set it up by > setting i2c_adapter.do_setup_alert ... SMBALERT# could be a > subcase of the adapter's normal interrupt handler. (Or, some > boards may want to use polling.) > > - The "i2c-gpio" driver now handles an optional named resource for > that SMBus alert signal. Named since, when this is substituted > for a misbehaving "native" driver, positional ids should be left > alone. (It might be better to put this logic into i2c core, to > apply whenever the i2c_adapter.dev.parent is a platform device.) > > - There's a new driver method used to report that a given device has > issued an alert. Its parameter includes the one bit of information > provided by the device in its alert response message. > > The IRQ driven code path is always enabled, if it's available. > > Signed-off-by: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org> I guess it's about time that I review this patch and merge it. Is this still the latest version of your code, or do you have anything more recent? Thanks, -- Jean Delvare ^ permalink raw reply [flat|nested] 29+ messages in thread
[parent not found: <20081118091546.421d6b78-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>]
* Re: [patch 2.6.27-rc7] i2c: smbalert# support [not found] ` <20081118091546.421d6b78-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org> @ 2008-11-18 22:01 ` David Brownell [not found] ` <200811181401.34809.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> 0 siblings, 1 reply; 29+ messages in thread From: David Brownell @ 2008-11-18 22:01 UTC (permalink / raw) To: Jean Delvare; +Cc: Linux I2C, lm-sensors-GZX6beZjE8VD60Wz+7aTrA On Tuesday 18 November 2008, Jean Delvare wrote: > I guess it's about time that I review this patch and merge it. Is this > still the latest version of your code, or do you have anything more > recent? I don't recall changing any functionality in a long time. But I've appended the current patch version, just in case. But I did looking at Intel's adapter hardware (i2c-i801) and noticing that at least ICH8 -- and probably many older versions -- can support SMBus alert IRQs in a way that's very compatible with the approach I took. - Dave ======== SNIP SNIP SNIP! From: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org> Infrastructure supporting SMBALERT# interrupts and the related SMBus protocols. These are defined as "optional" by the SMBus spec. - The i2c_adapter now includes a work_struct doing the work of talking to the Alert Response Address until nobody responds any more (and hence the IRQ is no longer asserted). Follows SMBus 2.0 not 1.1; there seems to be no point in trying to handle ten-bit addresses. - Some of the ways that work_struct could be driven: * Adapter driver provides an IRQ, which is bound to a handler which schedules that work_struct (using keventd for now). NOTE: it's nicest if this is edge triggered, but the code should handle level triggered IRQs too. * Adapter driver schedules that work struct itself, maybe even on a workqueue of its own. It asks the core to set it up by setting i2c_adapter.do_setup_alert ... SMBALERT# could be a subcase of the adapter's normal interrupt handler. (Or, some boards may want to use polling.) - The "i2c-gpio" driver now handles an optional named resource for that SMBus alert signal. Named since, when this is substituted for a misbehaving "native" driver, positional ids should be left alone. (It might be better to put this logic into i2c core, to apply whenever the i2c_adapter.dev.parent is a platform device.) - There's a new driver method used to report that a given device has issued an alert. Its parameter includes the one bit of information provided by the device in its alert response message. The IRQ driven code path is always enabled, if it's available. Signed-off-by: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org> --- drivers/i2c/busses/i2c-gpio.c | 10 ++ drivers/i2c/i2c-core.c | 155 ++++++++++++++++++++++++++++++++++++++++ include/linux/i2c.h | 14 +++ 3 files changed, 179 insertions(+) --- a/drivers/i2c/busses/i2c-gpio.c +++ b/drivers/i2c/busses/i2c-gpio.c @@ -82,6 +82,7 @@ static int __devinit i2c_gpio_probe(stru struct i2c_gpio_platform_data *pdata; struct i2c_algo_bit_data *bit_data; struct i2c_adapter *adap; + struct resource *smbalert; int ret; pdata = pdev->dev.platform_data; @@ -143,6 +144,15 @@ static int __devinit i2c_gpio_probe(stru adap->class = I2C_CLASS_HWMON | I2C_CLASS_SPD; adap->dev.parent = &pdev->dev; + smbalert = platform_get_resource_byname(pdev, IORESOURCE_IRQ, + "smbalert#"); + if (smbalert) { + adap->irq = smbalert->start; + if ((IORESOURCE_IRQ_LOWEDGE | IORESOURCE_IRQ_HIGHEDGE) + & smbalert->flags) + adap->alert_edge_triggered = 1; + } + /* * If "dev->id" is negative we consider it as zero. * The reason to do so is to avoid sysfs names that only make --- a/drivers/i2c/i2c-core.c +++ b/drivers/i2c/i2c-core.c @@ -32,6 +32,8 @@ #include <linux/platform_device.h> #include <linux/mutex.h> #include <linux/completion.h> +#include <linux/interrupt.h> + #include <linux/hardirq.h> #include <linux/irqflags.h> #include <asm/uaccess.h> @@ -404,6 +406,117 @@ static struct class i2c_adapter_class = .dev_attrs = i2c_adapter_attrs, }; +struct alert_data { + unsigned short addr; + bool flag; +}; + +/* If this is the alerting device, notify its driver. */ +static int i2c_do_alert(struct device *dev, void *addrp) +{ + struct i2c_client *client = i2c_verify_client(dev); + struct alert_data *data = addrp; + + if (!client || client->addr != data->addr) + return 0; + if (client->flags & I2C_CLIENT_TEN) + return 0; + + /* Drivers should either disable alerts, or provide at least + * a minimal handler. Lock so client->driver won't change. + */ + down(&dev->sem); + if (client->driver) { + if (client->driver->alert) + client->driver->alert(client, data->flag); + else + dev_warn(&client->dev, "no driver alert()!\n"); + } else + dev_dbg(&client->dev, "alert with no driver\n"); + up(&dev->sem); + + /* Stop iterating after we find the device. */ + return -EBUSY; +} + +/* + * The alert IRQ handler needs to hand work off to a task which can issue + * SMBus calls, because those sleeping calls can't be made in IRQ context. + */ +static void smbus_alert(struct work_struct *work) +{ + struct i2c_adapter *bus; + + bus = container_of(work, struct i2c_adapter, alert); + for (;;) { + s32 status; + struct alert_data data; + + /* Devices with pending alerts reply in address order, low + * to high, because of slave transmit arbitration. After + * responding, an SMBus device stops asserting SMBALERT#. + * + * NOTE that SMBus 2.0 reserves 10-bit addresess for future + * use. We neither handle them, nor try to use PEC here. + */ + status = i2c_smbus_read_byte(bus->ara); + if (status < 0) + break; + + data.flag = (status & 1) != 0; + data.addr = status >> 1; + dev_dbg(&bus->dev, "SMBALERT# %s from dev 0x%02x\n", + data.flag ? "true" : "false", data.addr); + + /* Notify driver for the device which issued the alert. */ + device_for_each_child(&bus->dev, &data, i2c_do_alert); + } + + /* We handled all alerts; reenable level-triggered IRQs. */ + if (!bus->alert_edge_triggered) + enable_irq(bus->irq); +} + +static irqreturn_t smbus_irq(int irq, void *adap) +{ + struct i2c_adapter *bus = adap; + + /* Disable level-triggered IRQs until we handle them. */ + if (!bus->alert_edge_triggered) + disable_irq_nosync(irq); + + schedule_work(&bus->alert); + return IRQ_HANDLED; +} + +static int smbalert_probe(struct i2c_client *c, const struct i2c_device_id *id) +{ + return 0; +} + +static int smbalert_remove(struct i2c_client *c) +{ + return 0; +} + +static const struct i2c_device_id smbalert_ids[] = { + { "smbus_alert", 0, }, + { /* LIST END */ } +}; + +static struct i2c_driver smbalert_driver = { + .driver = { + .name = "smbus_alert", + }, + .probe = smbalert_probe, + .remove = smbalert_remove, + .id_table = smbalert_ids, +}; + +static const struct i2c_board_info ara_board_info = { + I2C_BOARD_INFO("smbus_alert", 0x0c), +}; + static void i2c_scan_static_board_info(struct i2c_adapter *adapter) { struct i2c_devinfo *devinfo; @@ -448,6 +561,9 @@ static int i2c_register_adapter(struct i mutex_init(&adap->clist_lock); INIT_LIST_HEAD(&adap->clients); + /* Setup SMBALERT# infrastructure. */ + INIT_WORK(&adap->alert, smbus_alert); + mutex_lock(&core_lock); /* Add the adapter to the driver core. @@ -466,6 +582,33 @@ static int i2c_register_adapter(struct i if (res) goto out_list; + /* If we'll be handling SMBus alerts, register the alert responder + * address so that nobody else can accidentally claim it. + * Handling can be done either through our IRQ handler, or by the + * adapter (from its handler, periodic polling, or whatever). + * + * NOTE that if we manage the IRQ, we *MUST* know if it's level or + * edge triggered in order to hand it to the workqueue correctly. + * If triggering the alert seems to wedge the system, you probably + * should have said it's level triggered. + */ + if (adap->irq > 0 || adap->do_setup_alert) + adap->ara = i2c_new_device(adap, &ara_board_info); + + if (adap->irq > 0 && adap->ara) { + res = devm_request_irq(&adap->dev, adap->irq, smbus_irq, + 0, "smbus_alert", adap); + if (res == 0) { + dev_info(&adap->dev, + "supports SMBALERT#, %s trigger\n", + adap->alert_edge_triggered + ? "edge" : "level"); + adap->has_alert_irq = 1; + } else + res = 0; + + } + dev_dbg(&adap->dev, "adapter [%s] registered\n", adap->name); /* create pre-declared device nodes for new-style drivers */ @@ -651,6 +794,12 @@ int i2c_del_adapter(struct i2c_adapter * } } + if (adap->has_alert_irq) { + devm_free_irq(&adap->dev, adap->irq, adap); + adap->has_alert_irq = 0; + } + cancel_work_sync(&adap->alert); + /* clean up the sysfs representation */ init_completion(&adap->dev_released); device_unregister(&adap->dev); @@ -970,12 +1119,17 @@ static int __init i2c_init(void) retval = class_register(&i2c_adapter_class); if (retval) goto bus_err; + retval = i2c_add_driver(&smbalert_driver); + if (retval) + goto alert_err; retval = i2c_add_driver(&dummy_driver); if (retval) goto class_err; return 0; class_err: + i2c_del_driver(&smbalert_driver); +alert_err: class_unregister(&i2c_adapter_class); bus_err: bus_unregister(&i2c_bus_type); @@ -986,6 +1140,7 @@ static void __exit i2c_exit(void) { i2c_del_driver(&dummy_driver); class_unregister(&i2c_adapter_class); + i2c_del_driver(&smbalert_driver); bus_unregister(&i2c_bus_type); } --- a/include/linux/i2c.h +++ b/include/linux/i2c.h @@ -34,6 +34,7 @@ #include <linux/device.h> /* for struct device */ #include <linux/sched.h> /* for completion */ #include <linux/mutex.h> +#include <linux/workqueue.h> extern struct bus_type i2c_bus_type; @@ -165,6 +166,11 @@ struct i2c_driver { int (*suspend)(struct i2c_client *, pm_message_t mesg); int (*resume)(struct i2c_client *); + /* SMBus alert protocol support. The alert response's low bit + * is the event flag (e.g. exceeding upper vs lower limit). + */ + void (*alert)(struct i2c_client *, bool flag); + /* a ioctl like command that can be used to perform specific functions * with the device. */ @@ -369,6 +375,14 @@ struct i2c_adapter { struct list_head clients; /* DEPRECATED */ char name[48]; struct completion dev_released; + + /* SMBALERT# support */ + unsigned do_setup_alert:1; + unsigned has_alert_irq:1; + unsigned alert_edge_triggered:1; + int irq; + struct i2c_client *ara; + struct work_struct alert; }; #define to_i2c_adapter(d) container_of(d, struct i2c_adapter, dev) ^ permalink raw reply [flat|nested] 29+ messages in thread
[parent not found: <200811181401.34809.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>]
* Re: [lm-sensors] [patch 2.6.27-rc7] i2c: smbalert# support [not found] ` <200811181401.34809.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> @ 2008-11-19 9:51 ` Trent Piepho [not found] ` <Pine.LNX.4.64.0811190140510.11673-3bmvVOk6DZ+DGx/iekXGtrjh7zpefjiS@public.gmane.org> 2008-11-19 13:57 ` Jean Delvare 2008-11-21 14:18 ` Jean Delvare 2 siblings, 1 reply; 29+ messages in thread From: Trent Piepho @ 2008-11-19 9:51 UTC (permalink / raw) To: David Brownell; +Cc: Jean Delvare, Linux I2C, lm-sensors-GZX6beZjE8VD60Wz+7aTrA On Tue, 18 Nov 2008, David Brownell wrote: > --- > drivers/i2c/busses/i2c-gpio.c | 10 ++ > drivers/i2c/i2c-core.c | 155 ++++++++++++++++++++++++++++++++++++++++ > include/linux/i2c.h | 14 +++ > 3 files changed, 179 insertions(+) Can this be made optional? Seeing as nothing uses it yet and it increases a brunch of core structs' sizes. > + smbalert = platform_get_resource_byname(pdev, IORESOURCE_IRQ, > + "smbalert#"); The handler for this irq uses the name "smbus_alert", maybe the resource and the handler should use the same name? ^ permalink raw reply [flat|nested] 29+ messages in thread
[parent not found: <Pine.LNX.4.64.0811190140510.11673-3bmvVOk6DZ+DGx/iekXGtrjh7zpefjiS@public.gmane.org>]
* Re: [lm-sensors] [patch 2.6.27-rc7] i2c: smbalert# support [not found] ` <Pine.LNX.4.64.0811190140510.11673-3bmvVOk6DZ+DGx/iekXGtrjh7zpefjiS@public.gmane.org> @ 2008-11-19 15:16 ` Jean Delvare [not found] ` <20081119161632.2d0bde9e-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org> 0 siblings, 1 reply; 29+ messages in thread From: Jean Delvare @ 2008-11-19 15:16 UTC (permalink / raw) To: Trent Piepho; +Cc: David Brownell, Linux I2C, lm-sensors-GZX6beZjE8VD60Wz+7aTrA Hi Trent, On Wed, 19 Nov 2008 01:51:08 -0800 (PST), Trent Piepho wrote: > On Tue, 18 Nov 2008, David Brownell wrote: > > --- > > drivers/i2c/busses/i2c-gpio.c | 10 ++ > > drivers/i2c/i2c-core.c | 155 ++++++++++++++++++++++++++++++++++++++++ > > include/linux/i2c.h | 14 +++ > > 3 files changed, 179 insertions(+) > > Can this be made optional? Seeing as nothing uses it yet and it increases a > brunch of core structs' sizes. Did you actually check the size increase? struct i2c_driver is added one pointer, so 4 or 8 bytes, hardly worth mentioning. struct i2c_adapter is added 72 bytes on x86-64, to 1360 bytes initially so an increase of less than 6%. Is this really such a big issue? I doubt it. You don't have that many i2c_adapters registered on any given system for it to really matter, methinks. Making it optional would have its problems as well. Either the drivers which implement or make use of SMBus alert would have to depend on said option being enabled, or you'd have to put #ifdefs in all these drivers to work both with and without the option. Neither option is really appealing IMHO. -- Jean Delvare ^ permalink raw reply [flat|nested] 29+ messages in thread
[parent not found: <20081119161632.2d0bde9e-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>]
* Re: [lm-sensors] [patch 2.6.27-rc7] i2c: smbalert# support [not found] ` <20081119161632.2d0bde9e-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org> @ 2008-11-20 22:00 ` Trent Piepho [not found] ` <Pine.LNX.4.64.0811201354310.11673-3bmvVOk6DZ+DGx/iekXGtrjh7zpefjiS@public.gmane.org> 0 siblings, 1 reply; 29+ messages in thread From: Trent Piepho @ 2008-11-20 22:00 UTC (permalink / raw) To: Jean Delvare; +Cc: David Brownell, Linux I2C, lm-sensors-GZX6beZjE8VD60Wz+7aTrA On Wed, 19 Nov 2008, Jean Delvare wrote: > On Wed, 19 Nov 2008 01:51:08 -0800 (PST), Trent Piepho wrote: >> On Tue, 18 Nov 2008, David Brownell wrote: >>> --- >>> drivers/i2c/busses/i2c-gpio.c | 10 ++ >>> drivers/i2c/i2c-core.c | 155 ++++++++++++++++++++++++++++++++++++++++ >>> include/linux/i2c.h | 14 +++ >>> 3 files changed, 179 insertions(+) >> >> Can this be made optional? Seeing as nothing uses it yet and it increases a >> brunch of core structs' sizes. > > Did you actually check the size increase? struct i2c_driver is added > one pointer, so 4 or 8 bytes, hardly worth mentioning. struct > i2c_adapter is added 72 bytes on x86-64, to 1360 bytes initially so an > increase of less than 6%. Is this really such a big issue? I doubt it. > You don't have that many i2c_adapters registered on any given system > for it to really matter, methinks. Except every other subsystem in the kernel does the same thing. Each release of 2.6 is significantly more bloated than the last and this is how it happens. One "it's not that much wasted" change at a time. > Making it optional would have its problems as well. Either the drivers > which implement or make use of SMBus alert would have to depend on said > option being enabled, or you'd have to put #ifdefs in all these drivers What's so hard about having them depend on smbus alert support? They already depend on I2C and so on. If any drivers ever get code for this, how hard is it to add one dependency or select to their Kconfig section? ^ permalink raw reply [flat|nested] 29+ messages in thread
[parent not found: <Pine.LNX.4.64.0811201354310.11673-3bmvVOk6DZ+DGx/iekXGtrjh7zpefjiS@public.gmane.org>]
* Re: [lm-sensors] [patch 2.6.27-rc7] i2c: smbalert# support [not found] ` <Pine.LNX.4.64.0811201354310.11673-3bmvVOk6DZ+DGx/iekXGtrjh7zpefjiS@public.gmane.org> @ 2008-11-20 22:56 ` David Brownell [not found] ` <200811201456.36551.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> 2008-11-21 8:42 ` Jean Delvare 1 sibling, 1 reply; 29+ messages in thread From: David Brownell @ 2008-11-20 22:56 UTC (permalink / raw) To: Trent Piepho; +Cc: Jean Delvare, Linux I2C, lm-sensors-GZX6beZjE8VD60Wz+7aTrA On Thursday 20 November 2008, Trent Piepho wrote: > > > > Did you actually check the size increase? struct i2c_driver is added > > one pointer, so 4 or 8 bytes, hardly worth mentioning. struct > > i2c_adapter is added 72 bytes on x86-64, to 1360 bytes initially so an > > increase of less than 6%. Is this really such a big issue? I doubt it. > > You don't have that many i2c_adapters registered on any given system > > for it to really matter, methinks. > > Except every other subsystem in the kernel does the same thing. Each release > of 2.6 is significantly more bloated than the last and this is how it happens. > One "it's not that much wasted" change at a time. I do recall the "4 MByte performance problem" morphing into the "8 MByte" problem, and then into the "16 MByte" version then "32 MByte" -- for UNIX workstations, including GUI. Today's handhelds are more powerful than those! And the embedded boards I get lately have 128 MB of RAM. The real counter-argument is that memory isn't *that* tight, especially since the hardware capacity is growing. That said, maybe you have a patch to propose which would address the problem of how to *cleanly* make this infrastructure optional? - Dave ^ permalink raw reply [flat|nested] 29+ messages in thread
[parent not found: <200811201456.36551.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>]
* Re: [lm-sensors] [patch 2.6.27-rc7] i2c: smbalert# support [not found] ` <200811201456.36551.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> @ 2008-11-22 0:55 ` Trent Piepho [not found] ` <Pine.LNX.4.64.0811211621340.18022-3bmvVOk6DZ+DGx/iekXGtrjh7zpefjiS@public.gmane.org> 0 siblings, 1 reply; 29+ messages in thread From: Trent Piepho @ 2008-11-22 0:55 UTC (permalink / raw) To: dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f Cc: Jean Delvare, Linux I2C, lm-sensors-GZX6beZjE8VD60Wz+7aTrA [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: TEXT/PLAIN; charset=X-UNKNOWN, Size: 2645 bytes --] On Thu, 20 Nov 2008, David Brownell wrote: > On Thursday 20 November 2008, Trent Piepho wrote: >>> >>> Did you actually check the size increase? struct i2c_driver is added >>> one pointer, so 4 or 8 bytes, hardly worth mentioning. struct >>> i2c_adapter is added 72 bytes on x86-64, to 1360 bytes initially so an >>> increase of less than 6%. Is this really such a big issue? I doubt it. >>> You don't have that many i2c_adapters registered on any given system >>> for it to really matter, methinks. >> >> Except every other subsystem in the kernel does the same thing. Each release >> of 2.6 is significantly more bloated than the last and this is how it happens. >> One "it's not that much wasted" change at a time. > > I do recall the "4 MByte performance problem" morphing into the "8 MByte" > problem, and then into the "16 MByte" version then "32 MByte" -- for UNIX > workstations, including GUI. Today's handhelds are more powerful than > those! And the embedded boards I get lately have 128 MB of RAM. > > The real counter-argument is that memory isn't *that* tight, especially > since the hardware capacity is growing. Those UNIX workstations cost thousands of dollars. If the increase in software inefficiency exactly matched in increase in hardware capacity, it wouldn't be possible to run UNIX on cell phones and $200 laptops powered by a foot pedal, we'd still need expensive workstations. If X MB memory isn't tight for a some task, then someone is going to want to do that task with X/2 MB of memory, so that they can have a cheaper, lower power, smaller product. The embedded board I have here has 256 MB of RAM. But it also has a dual core CPU that can run two copies of Linux at once. So cut that RAM in half. And then we have virtual machines... So yes, hardware keeps getting better. But you can't say, "I have twice the RAM so I can make my software twice as bloated as come out even." Because we also except to be able to do more with cheaper, lower power hardware. Though saying hardware is getting better isn't quite true. Compared to CPU speed, memory latency has steadily gotten _worse_ over the last decade. Making kernel data structures larger and increasing cache pressure can still be quite costly, and that cost isn't going down, it's going up. > That said, maybe you have a patch to propose which would address the > problem of how to *cleanly* make this infrastructure optional? Well, that depends on what one defines as "cleanly". Sometimes is feels as if the definition is intentionally made so narrow that that it is quite impossible to meet. ^ permalink raw reply [flat|nested] 29+ messages in thread
[parent not found: <Pine.LNX.4.64.0811211621340.18022-3bmvVOk6DZ+DGx/iekXGtrjh7zpefjiS@public.gmane.org>]
* Re: [lm-sensors] [patch 2.6.27-rc7] i2c: smbalert# support [not found] ` <Pine.LNX.4.64.0811211621340.18022-3bmvVOk6DZ+DGx/iekXGtrjh7zpefjiS@public.gmane.org> @ 2008-11-22 2:58 ` David Brownell 0 siblings, 0 replies; 29+ messages in thread From: David Brownell @ 2008-11-22 2:58 UTC (permalink / raw) To: Trent Piepho; +Cc: Jean Delvare, Linux I2C, lm-sensors-GZX6beZjE8VD60Wz+7aTrA On Friday 21 November 2008, Trent Piepho wrote: > So yes, hardware keeps getting better. But you can't say, "I have twice the > RAM so I can make my software twice as bloated as come out even." Because we > also except to be able to do more with cheaper, lower power hardware. Which wasn't my point at all; neither was even closely related: > > The real counter-argument is that memory isn't *that* tight, especially > > since the hardware capacity is growing. If you're keen on saving a hundred bytes, you could probably get it by removing some needless strings or moving code to init/exit sections ... and in fact, get a lot more than that. Without any loss of functionality. ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [patch 2.6.27-rc7] i2c: smbalert# support [not found] ` <Pine.LNX.4.64.0811201354310.11673-3bmvVOk6DZ+DGx/iekXGtrjh7zpefjiS@public.gmane.org> 2008-11-20 22:56 ` David Brownell @ 2008-11-21 8:42 ` Jean Delvare [not found] ` <20081121094218.34ecd82a-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org> 1 sibling, 1 reply; 29+ messages in thread From: Jean Delvare @ 2008-11-21 8:42 UTC (permalink / raw) To: Trent Piepho; +Cc: David Brownell, Linux I2C, lm-sensors-GZX6beZjE8VD60Wz+7aTrA Hi Trent. On Thu, 20 Nov 2008 14:00:21 -0800 (PST), Trent Piepho wrote: > On Wed, 19 Nov 2008, Jean Delvare wrote: > > On Wed, 19 Nov 2008 01:51:08 -0800 (PST), Trent Piepho wrote: > >> On Tue, 18 Nov 2008, David Brownell wrote: > >>> --- > >>> drivers/i2c/busses/i2c-gpio.c | 10 ++ > >>> drivers/i2c/i2c-core.c | 155 ++++++++++++++++++++++++++++++++++++++++ > >>> include/linux/i2c.h | 14 +++ > >>> 3 files changed, 179 insertions(+) > >> > >> Can this be made optional? Seeing as nothing uses it yet and it increases a > >> brunch of core structs' sizes. > > > > Did you actually check the size increase? struct i2c_driver is added > > one pointer, so 4 or 8 bytes, hardly worth mentioning. struct > > i2c_adapter is added 72 bytes on x86-64, to 1360 bytes initially so an > > increase of less than 6%. Is this really such a big issue? I doubt it. > > You don't have that many i2c_adapters registered on any given system > > for it to really matter, methinks. > > Except every other subsystem in the kernel does the same thing. Each release > of 2.6 is significantly more bloated than the last and this is how it happens. > One "it's not that much wasted" change at a time. You are unfair. We have been continuously cleaning up the i2c subsystem over the past few years, shrinking the main structures regularly, killing unused code, etc. And this clean-up process is going on. > > Making it optional would have its problems as well. Either the drivers > > which implement or make use of SMBus alert would have to depend on said > > option being enabled, or you'd have to put #ifdefs in all these drivers > > What's so hard about having them depend on smbus alert support? They already > depend on I2C and so on. If any drivers ever get code for this, how hard is > it to add one dependency or select to their Kconfig section? While you are complaining about ever-growing memory footprints, other users of the Linux kernel complain about ever-growing kernel config option list. There are so many kernel configuration options these days that it has become very hard for newcomers to configure their kernel on their own. Even distribution kernel maintainers start complaining that they don't know which options to select. So, adding new kernel options for something else than hardware drivers should be considered with great care. In practice, this means that I am opposed to an option that would appear in the configuration menu. If anything, that would be an hidden option that would have to be selected by drivers which need it (much as the "i2c algorithm" helper modules.) However, this approach has its shares of problems as well. For one, driver authors might forget to select the option in question, and this may lead to build failures later on. Even if they do not forget, that's still one more thing for them to think of (and for us to check when a driver is submitted for review.) For another, if anyone needs the option for an out-of-tree driver, they will ask for a manual way to select it anyway. On top of that, your proposal isn't even optimal: if you have just one driver that needs SMBus alert support, all adapters have their size increased. Taking a concrete example, on my system, there are normally 3 I2C adapters (the mainboard's SMBus and 2 multimedia adapters) and they don't make use of SMBus alert (not yet.) But I also have an evaluation board for a thermal sensor which connects to the parallel port. That one needs SMBus alert support. With your proposal, in order to be able to make use of my evaluation board once in a while, every other I2C adapter in my system would need to have the extra structure fields. I fear that, in practice, this means that the SMBus alert option will end up being selected in almost every kernel out there. Once the support for this functionality is added to the kernel, you will see drivers implementing support for it (David has not been the only person asking for it.) On the other hand, trying to optimize the memory usage isn't trivial. Moving the extra structure fields to a separate, dynamically allocated structure might increase the memory fragmentation and in practice use _more_ memory. This would also make the code more complex, so what you win on data structures, you lose on code. The alternative, as mentioned in my initial answer, would be to let each driver be built with or without support for SMBus alert, depending on a central configuration option (which unfortunately would no longer be possibly hidden.) After all, just because the hardware can do it and the driver has support for it doesn't mean that your system makes use of it! But then, not only you bloat the code with ifdefs, but you also introduce one more thing for every driver author to test: he/she will have to test his/her code with and without the option in question. If we later add a couple similar options, this can quickly become impossible to test all combinations in practice. And this will make bug reproduction more difficult too. All in all I am simply not convinced that it it worth optimizing. There appears to be more drawbacks than benefits. There are many different areas where cleanups would save a lot more memory than what you can hope for here. For example, completing the removal of the legacy i2c binding model. Want to help? -- Jean Delvare ^ permalink raw reply [flat|nested] 29+ messages in thread
[parent not found: <20081121094218.34ecd82a-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>]
* Re: [patch 2.6.27-rc7] i2c: smbalert# support [not found] ` <20081121094218.34ecd82a-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org> @ 2008-11-22 6:04 ` Trent Piepho [not found] ` <Pine.LNX.4.64.0811212122370.18022-3bmvVOk6DZ+DGx/iekXGtrjh7zpefjiS@public.gmane.org> 0 siblings, 1 reply; 29+ messages in thread From: Trent Piepho @ 2008-11-22 6:04 UTC (permalink / raw) To: Jean Delvare; +Cc: David Brownell, Linux I2C, lm-sensors-GZX6beZjE8VD60Wz+7aTrA On Fri, 21 Nov 2008, Jean Delvare wrote: > On Thu, 20 Nov 2008 14:00:21 -0800 (PST), Trent Piepho wrote: >>> Did you actually check the size increase? struct i2c_driver is added >>> one pointer, so 4 or 8 bytes, hardly worth mentioning. struct >>> i2c_adapter is added 72 bytes on x86-64, to 1360 bytes initially so an >>> increase of less than 6%. Is this really such a big issue? I doubt it. >>> You don't have that many i2c_adapters registered on any given system >>> for it to really matter, methinks. >> >> Except every other subsystem in the kernel does the same thing. Each release >> of 2.6 is significantly more bloated than the last and this is how it happens. >> One "it's not that much wasted" change at a time. > > You are unfair. We have been continuously cleaning up the i2c subsystem > over the past few years, shrinking the main structures regularly, > killing unused code, etc. And this clean-up process is going on. The size of i2c-core, struct i2c_client and struct i2c_adapter: v2.6.28 9718 392 500 w/ smbalert patch v2.6.28 8878 392 472 v2.6.27 8732 368 448 v2.6.26 7877 352 440 v2.6.25 7854 376 444 v2.6.24 9060 372 444 v2.6.23 9359 404 476 v2.6.22 9330 448 520 v2.6.21 7814 464 692 v2.6.20 7896 444 672 v2.6.19 7871 444 668 v2.6.18 6857 436 660 Other than some temporary bloat in 2.6.22-2.6.24, size the size of i2c core has been steadily increasing. The data structures got a shrink around 2.6.23-2.6.24, mostly from removing unneeded name fields, but most other changes have been an increase in size. > On top of that, your proposal isn't even optimal: if you have just one > driver that needs SMBus alert support, all adapters have their size > increased. Taking a concrete example, on my system, there are normally Seeing as there are currently no clients with smbalert support, this shouldn't happen to most people for quite some time. Forcing it on for people who don't need smbalert (and no one has it now, so there can't be that much demand) is clearly worse. Maybe most distro kernels will have it on, but people who have a reason to care about kernel size and speed can turn it off. > The alternative, as mentioned in my initial answer, would be to let > each driver be built with or without support for SMBus alert, depending > on a central configuration option (which unfortunately would no longer > be possibly hidden.) After all, just because the hardware can do it and Some drivers could select smbalert support and only build with it. Other drivers could have a driver option to enable alert support, core smbalert support would only be selected if that option was one. It would depend on how much bloat it adds and how complex it is to make optional. The alarm irq code I wrote for the lm63 driver ended up being quite complicated. > All in all I am simply not convinced that it it worth optimizing. There > appears to be more drawbacks than benefits. There are many different > areas where cleanups would save a lot more memory than what you can > hope for here. For example, completing the removal of the legacy i2c > binding model. Want to help? Increased efficiency in one place is not exclusive of increase efficiency elsewhere. ^ permalink raw reply [flat|nested] 29+ messages in thread
[parent not found: <Pine.LNX.4.64.0811212122370.18022-3bmvVOk6DZ+DGx/iekXGtrjh7zpefjiS@public.gmane.org>]
* Re: [patch 2.6.27-rc7] i2c: smbalert# support [not found] ` <Pine.LNX.4.64.0811212122370.18022-3bmvVOk6DZ+DGx/iekXGtrjh7zpefjiS@public.gmane.org> @ 2008-11-22 10:13 ` Jean Delvare [not found] ` <20081122111340.0bfc2c05-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org> 0 siblings, 1 reply; 29+ messages in thread From: Jean Delvare @ 2008-11-22 10:13 UTC (permalink / raw) To: Trent Piepho; +Cc: David Brownell, Linux I2C, lm-sensors-GZX6beZjE8VD60Wz+7aTrA Hi Trent, On Fri, 21 Nov 2008 22:04:26 -0800 (PST), Trent Piepho wrote: > On Fri, 21 Nov 2008, Jean Delvare wrote: > > You are unfair. We have been continuously cleaning up the i2c subsystem > > over the past few years, shrinking the main structures regularly, > > killing unused code, etc. And this clean-up process is going on. > > The size of i2c-core, struct i2c_client and struct i2c_adapter: > v2.6.28 9718 392 500 w/ smbalert patch > v2.6.28 8878 392 472 > v2.6.27 8732 368 448 > v2.6.26 7877 352 440 > v2.6.25 7854 376 444 > v2.6.24 9060 372 444 > v2.6.23 9359 404 476 > v2.6.22 9330 448 520 > v2.6.21 7814 464 692 > v2.6.20 7896 444 672 > v2.6.19 7871 444 668 > v2.6.18 6857 436 660 Thanks for providing these numbers, these are really useful! I'm curious if you would be able to compute the same numbers down to, say, kernel 2.6.5 or even 2.6.0? I'm curious where we started from. I am also interested in the script / method you used to come up with these numbers, so that maybe I can get the numbers myself if you do not have the time. > Other than some temporary bloat in 2.6.22-2.6.24, size the size of i2c core > has been steadily increasing. In 2.6.22 we merged the new-style i2c driver binding. The size increase was pretty much expected, as all the legacy binding code was (and is) still around. In 2.6.24 we killed i2c_control() (actually, it essentially moved to i2c-dev.) In 2.6.25 we did many cleanups. So it is worth noting that what you call "temporary bloat" isn't really that. The code duplication that was introduced in 2.6.22 wasn't removed in 2.6.25. It is still present in the kernel today and we should be able to get rid of it later, meaning that the size of i2c-core will decrease. In 2.6.27 we added detection capability to the new-style i2c binding model, temporarily duplicating a lot of code again. The old code is expected to go away in kernel 2.6.29 (I hope) or 2.6.30 together with the legacy binding model. It will be interesting to see where it gets us. > The data structures got a shrink around > 2.6.23-2.6.24, mostly from removing unneeded name fields, but most other > changes have been an increase in size. We should see a shrink again in kernel 2.6.29 or 2.6.30, as I wrote above. Anyway, your numbers pretty much prove my point: overall, the size of the i2c data structures has decreased over the last few years. -10% for i2c_client, -24% for i2c_adapter between 2.6.18 and ~2.6.28. So you can't claim that they are growing beyond control. We are clearly paying attention to the size of these structures. > > On top of that, your proposal isn't even optimal: if you have just one > > driver that needs SMBus alert support, all adapters have their size > > increased. Taking a concrete example, on my system, there are normally > > Seeing as there are currently no clients with smbalert support, this shouldn't > happen to most people for quite some time. Forcing it on for people who don't > need smbalert (and no one has it now, so there can't be that much demand) How could there be support for a feature which we are just adding now? And there _has_ been demand (Hendrik Sattler in August 2006 [1], David Brownell in September 2007 [2], Srinivas Ramana in October 2008 [3]), which is exactly why we are implementing support now. At this point I simply don't know how popular the feature will be. It may take years to find out, as people tend to not touch drivers that work, even when new features could let them rewrite the code in a better way. [1] http://lists.lm-sensors.org/pipermail/i2c/2006-August/000204.html [2] http://lists.lm-sensors.org/pipermail/i2c/2007-September/001845.html [3] http://lists.lm-sensors.org/pipermail/i2c/2008-October/004949.html > is > clearly worse. Maybe most distro kernels will have it on, but people who have > a reason to care about kernel size and speed can turn it off. As has been said before, you are welcome to submit a patch making the feature optional, and we'll evaluate it. But I still admit I am skeptical that you can come up with something with more advantages than drawbacks. We are talking about less than 2 kB of run-time memory here, aren't we? > > The alternative, as mentioned in my initial answer, would be to let > > each driver be built with or without support for SMBus alert, depending > > on a central configuration option (which unfortunately would no longer > > be possibly hidden.) After all, just because the hardware can do it and > > Some drivers could select smbalert support and only build with it. Other > drivers could have a driver option to enable alert support, core smbalert > support would only be selected if that option was one. It would depend on how > much bloat it adds and how complex it is to make optional. The alarm irq code > I wrote for the lm63 driver ended up being quite complicated. Good point, not all drivers have to do the same. My previous point remains though: this is adding complexity to the kernel configuration, bringing its share of potential issues when it comes to test coverage and bug reproducibility. > > All in all I am simply not convinced that it it worth optimizing. There > > appears to be more drawbacks than benefits. There are many different > > areas where cleanups would save a lot more memory than what you can > > hope for here. For example, completing the removal of the legacy i2c > > binding model. Want to help? > > Increased efficiency in one place is not exclusive of increase efficiency > elsewhere. In a fantasy world where spare time grows on trees, you may be correct. In the real world though, you are clearly wrong. If we spend time making SMBus alert support optional, this is time we aren't spending on improving something else. I am trying to spend my time where it is well spent, or where there is fun. Making SMBus alert optional doesn't fit in either category as far as I can see. -- Jean Delvare ^ permalink raw reply [flat|nested] 29+ messages in thread
[parent not found: <20081122111340.0bfc2c05-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>]
* Re: [patch 2.6.27-rc7] i2c: smbalert# support [not found] ` <20081122111340.0bfc2c05-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org> @ 2008-11-22 20:28 ` Trent Piepho [not found] ` <Pine.LNX.4.64.0811221055470.18022-3bmvVOk6DZ+DGx/iekXGtrjh7zpefjiS@public.gmane.org> 0 siblings, 1 reply; 29+ messages in thread From: Trent Piepho @ 2008-11-22 20:28 UTC (permalink / raw) To: Jean Delvare; +Cc: David Brownell, Linux I2C, lm-sensors-GZX6beZjE8VD60Wz+7aTrA On Sat, 22 Nov 2008, Jean Delvare wrote: > On Fri, 21 Nov 2008 22:04:26 -0800 (PST), Trent Piepho wrote: >> On Fri, 21 Nov 2008, Jean Delvare wrote: >>> You are unfair. We have been continuously cleaning up the i2c subsystem >>> over the past few years, shrinking the main structures regularly, >>> killing unused code, etc. And this clean-up process is going on. >> >> The size of i2c-core, struct i2c_client and struct i2c_adapter: >> v2.6.28 9718 392 500 w/ smbalert patch >> v2.6.28 8878 392 472 >> v2.6.27 8732 368 448 >> v2.6.26 7877 352 440 >> v2.6.25 7854 376 444 >> v2.6.24 9060 372 444 >> v2.6.23 9359 404 476 >> v2.6.22 9330 448 520 >> v2.6.21 7814 464 692 >> v2.6.20 7896 444 672 >> v2.6.19 7871 444 668 >> v2.6.18 6857 436 660 > > Thanks for providing these numbers, these are really useful! I'm > curious if you would be able to compute the same numbers down to, say, > kernel 2.6.5 or even 2.6.0? I'm curious where we started from. > > I am also interested in the script / method you used to come up with > these numbers, so that maybe I can get the numbers myself if you do not > have the time. It's not entirely automatic since you have to deal with configuring the kernel. It helps to start at the newest kernel and work backwards, since kconfig options are added more often than removed. I put this in my .bashrc to make getting object sizes easier: function objsize { ${CROSS_COMPILE}objdump -h $1 | awk '/text|data|bss/{x=strtonum("0x"$3);printf "%-16s%d\t%d\n",$2,x,s+=x;}' ;} Then to get structure sizes: cat > lib/size.c #include <linux/i2c.h> #define S(t) cpu_to_be32(sizeof(struct t)) uint32_t a=S(i2c_adapter), b=S(i2c_client); // reverse order in obj On older kernels you have to replace cpu_to_be32() with ___constant_swab32() on little-endian systems and remove it on big-endian. Then do this for each version: ver=v2.6.17 git checkout -f $ver echo 'obj-y += size.o' >> lib/Makefile mkdir $ver cp last.config $ver/.config make O=$ver # answer make questions, try not to turn on anything that adds debug/trace # code or extra fields to data structures. Or turns off extra fields that # used to be mandatory. Or changes compiler flags, e.g. inlining or regparm. # Wait... ssize=`objdump -s -j .data $ver/lib/size.o | awk '/^ 0/{print strtonum("0x"$2),strtonum("0x"$3);}'` csize=`objsize $ver/drivers/i2c/i2c-core.o | awk 'END{print $3}'` echo `git describe` $csize $ssize >> i2csize cp $ver/.config last.config v2.6.17 6934 416 640 v2.6.16 7320 404 620 ^ permalink raw reply [flat|nested] 29+ messages in thread
[parent not found: <Pine.LNX.4.64.0811221055470.18022-3bmvVOk6DZ+DGx/iekXGtrjh7zpefjiS@public.gmane.org>]
* Re: [patch 2.6.27-rc7] i2c: smbalert# support [not found] ` <Pine.LNX.4.64.0811221055470.18022-3bmvVOk6DZ+DGx/iekXGtrjh7zpefjiS@public.gmane.org> @ 2008-11-23 21:45 ` Jean Delvare 0 siblings, 0 replies; 29+ messages in thread From: Jean Delvare @ 2008-11-23 21:45 UTC (permalink / raw) To: Trent Piepho; +Cc: David Brownell, Linux I2C, lm-sensors-GZX6beZjE8VD60Wz+7aTrA Hi Trent, On Sat, 22 Nov 2008 12:28:56 -0800 (PST), Trent Piepho wrote: > On Sat, 22 Nov 2008, Jean Delvare wrote: > > On Fri, 21 Nov 2008 22:04:26 -0800 (PST), Trent Piepho wrote: > >> The size of i2c-core, struct i2c_client and struct i2c_adapter: > >> v2.6.28 9718 392 500 w/ smbalert patch > >> v2.6.28 8878 392 472 > >> v2.6.27 8732 368 448 > >> v2.6.26 7877 352 440 > >> v2.6.25 7854 376 444 > >> v2.6.24 9060 372 444 > >> v2.6.23 9359 404 476 > >> v2.6.22 9330 448 520 > >> v2.6.21 7814 464 692 > >> v2.6.20 7896 444 672 > >> v2.6.19 7871 444 668 > >> v2.6.18 6857 436 660 > > > > Thanks for providing these numbers, these are really useful! I'm > > curious if you would be able to compute the same numbers down to, say, > > kernel 2.6.5 or even 2.6.0? I'm curious where we started from. > > > > I am also interested in the script / method you used to come up with > > these numbers, so that maybe I can get the numbers myself if you do not > > have the time. > > It's not entirely automatic since you have to deal with configuring the > kernel. It helps to start at the newest kernel and work backwards, since > kconfig options are added more often than removed. > > I put this in my .bashrc to make getting object sizes easier: > function objsize { ${CROSS_COMPILE}objdump -h $1 | awk '/text|data|bss/{x=strtonum("0x"$3);printf "%-16s%d\t%d\n",$2,x,s+=x;}' ;} Not sure why you don't just use "size" for this? > Then to get structure sizes: > cat > lib/size.c > #include <linux/i2c.h> > #define S(t) cpu_to_be32(sizeof(struct t)) > uint32_t a=S(i2c_adapter), b=S(i2c_client); // reverse order in obj > > On older kernels you have to replace cpu_to_be32() with ___constant_swab32() > on little-endian systems and remove it on big-endian. > > Then do this for each version: > > ver=v2.6.17 > git checkout -f $ver > echo 'obj-y += size.o' >> lib/Makefile > mkdir $ver > cp last.config $ver/.config > make O=$ver > # answer make questions, try not to turn on anything that adds debug/trace > # code or extra fields to data structures. Or turns off extra fields that > # used to be mandatory. Or changes compiler flags, e.g. inlining or regparm. > # Wait... > ssize=`objdump -s -j .data $ver/lib/size.o | > awk '/^ 0/{print strtonum("0x"$2),strtonum("0x"$3);}'` > csize=`objsize $ver/drivers/i2c/i2c-core.o | awk 'END{print $3}'` > echo `git describe` $csize $ssize >> i2csize > cp $ver/.config last.config > > v2.6.17 6934 416 640 > v2.6.16 7320 404 620 Hmm, OK, that's not exactly trivial... I don't think I'll have the time to setup this. Given that you already have everything in place, would you please provide the additional numbers I am interested in? No need to do all kernel versions down to 2.6.0, you certainly have more interesting things to do with your time. All I am really interested in are 2.6.5 and 2.6.0. This should give us a good idea of where we started from. Thanks, -- Jean Delvare ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [patch 2.6.27-rc7] i2c: smbalert# support [not found] ` <200811181401.34809.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> 2008-11-19 9:51 ` [lm-sensors] " Trent Piepho @ 2008-11-19 13:57 ` Jean Delvare [not found] ` <20081119145712.1abaa63f-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org> 2008-11-21 14:18 ` Jean Delvare 2 siblings, 1 reply; 29+ messages in thread From: Jean Delvare @ 2008-11-19 13:57 UTC (permalink / raw) To: David Brownell; +Cc: Linux I2C Hi David, (Dropping the lm-sensors list.) On Tue, 18 Nov 2008 14:01:34 -0800, David Brownell wrote: > On Tuesday 18 November 2008, Jean Delvare wrote: > > I guess it's about time that I review this patch and merge it. Is this > > still the latest version of your code, or do you have anything more > > recent? > > I don't recall changing any functionality in a long time. > But I've appended the current patch version, just in case. > > But I did looking at Intel's adapter hardware (i2c-i801) > and noticing that at least ICH8 -- and probably many older > versions -- can support SMBus alert IRQs in a way that's > very compatible with the approach I took. > > - Dave > > > ======== SNIP SNIP SNIP! > From: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org> > > Infrastructure supporting SMBALERT# interrupts and the related SMBus > protocols. These are defined as "optional" by the SMBus spec. > > - The i2c_adapter now includes a work_struct doing the work of talking > to the Alert Response Address until nobody responds any more (and > hence the IRQ is no longer asserted). Follows SMBus 2.0 not 1.1; > there seems to be no point in trying to handle ten-bit addresses. > > - Some of the ways that work_struct could be driven: > > * Adapter driver provides an IRQ, which is bound to a handler > which schedules that work_struct (using keventd for now). > NOTE: it's nicest if this is edge triggered, but the code > should handle level triggered IRQs too. > > * Adapter driver schedules that work struct itself, maybe even > on a workqueue of its own. It asks the core to set it up by > setting i2c_adapter.do_setup_alert ... SMBALERT# could be a > subcase of the adapter's normal interrupt handler. (Or, some > boards may want to use polling.) > > - The "i2c-gpio" driver now handles an optional named resource for > that SMBus alert signal. Named since, when this is substituted > for a misbehaving "native" driver, positional ids should be left > alone. (It might be better to put this logic into i2c core, to > apply whenever the i2c_adapter.dev.parent is a platform device.) > > - There's a new driver method used to report that a given device has > issued an alert. Its parameter includes the one bit of information > provided by the device in its alert response message. > > The IRQ driven code path is always enabled, if it's available. Overall it looks good to me. Review: > Signed-off-by: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org> > --- > drivers/i2c/busses/i2c-gpio.c | 10 ++ > drivers/i2c/i2c-core.c | 155 ++++++++++++++++++++++++++++++++++++++++ > include/linux/i2c.h | 14 +++ > 3 files changed, 179 insertions(+) > > --- a/drivers/i2c/busses/i2c-gpio.c > +++ b/drivers/i2c/busses/i2c-gpio.c > @@ -82,6 +82,7 @@ static int __devinit i2c_gpio_probe(stru > struct i2c_gpio_platform_data *pdata; > struct i2c_algo_bit_data *bit_data; > struct i2c_adapter *adap; > + struct resource *smbalert; > int ret; > > pdata = pdev->dev.platform_data; > @@ -143,6 +144,15 @@ static int __devinit i2c_gpio_probe(stru > adap->class = I2C_CLASS_HWMON | I2C_CLASS_SPD; > adap->dev.parent = &pdev->dev; > > + smbalert = platform_get_resource_byname(pdev, IORESOURCE_IRQ, > + "smbalert#"); > + if (smbalert) { > + adap->irq = smbalert->start; > + if ((IORESOURCE_IRQ_LOWEDGE | IORESOURCE_IRQ_HIGHEDGE) > + & smbalert->flags) > + adap->alert_edge_triggered = 1; > + } > + > /* > * If "dev->id" is negative we consider it as zero. > * The reason to do so is to avoid sysfs names that only make > --- a/drivers/i2c/i2c-core.c > +++ b/drivers/i2c/i2c-core.c > @@ -32,6 +32,8 @@ > #include <linux/platform_device.h> > #include <linux/mutex.h> > #include <linux/completion.h> > +#include <linux/interrupt.h> > + > #include <linux/hardirq.h> > #include <linux/irqflags.h> > #include <asm/uaccess.h> > @@ -404,6 +406,117 @@ static struct class i2c_adapter_class = > .dev_attrs = i2c_adapter_attrs, > }; > > +struct alert_data { > + unsigned short addr; > + bool flag; > +}; > + > +/* If this is the alerting device, notify its driver. */ > +static int i2c_do_alert(struct device *dev, void *addrp) > +{ > + struct i2c_client *client = i2c_verify_client(dev); > + struct alert_data *data = addrp; > + > + if (!client || client->addr != data->addr) > + return 0; > + if (client->flags & I2C_CLIENT_TEN) > + return 0; > + > + /* Drivers should either disable alerts, or provide at least > + * a minimal handler. Lock so client->driver won't change. > + */ > + down(&dev->sem); > + if (client->driver) { > + if (client->driver->alert) > + client->driver->alert(client, data->flag); > + else > + dev_warn(&client->dev, "no driver alert()!\n"); > + } else > + dev_dbg(&client->dev, "alert with no driver\n"); > + up(&dev->sem); > + > + /* Stop iterating after we find the device. */ > + return -EBUSY; > +} > + > +/* > + * The alert IRQ handler needs to hand work off to a task which can issue > + * SMBus calls, because those sleeping calls can't be made in IRQ context. > + */ > +static void smbus_alert(struct work_struct *work) > +{ > + struct i2c_adapter *bus; > + > + bus = container_of(work, struct i2c_adapter, alert); > + for (;;) { > + s32 status; > + struct alert_data data; > + > + /* Devices with pending alerts reply in address order, low > + * to high, because of slave transmit arbitration. After > + * responding, an SMBus device stops asserting SMBALERT#. > + * > + * NOTE that SMBus 2.0 reserves 10-bit addresess for future > + * use. We neither handle them, nor try to use PEC here. > + */ > + status = i2c_smbus_read_byte(bus->ara); > + if (status < 0) > + break; > + > + data.flag = (status & 1) != 0; This is equivalent to just "status & 1". > + data.addr = status >> 1; > + dev_dbg(&bus->dev, "SMBALERT# %s from dev 0x%02x\n", > + data.flag ? "true" : "false", data.addr); The flag is really only a data bit, it has no true/false meaning, so presenting it that way is confusing. Please display it as 0/1 instead. > + > + /* Notify driver for the device which issued the alert. */ > + device_for_each_child(&bus->dev, &data, i2c_do_alert); > + } > + > + /* We handled all alerts; reenable level-triggered IRQs. */ > + if (!bus->alert_edge_triggered) > + enable_irq(bus->irq); > +} > + > +static irqreturn_t smbus_irq(int irq, void *adap) > +{ > + struct i2c_adapter *bus = adap; > + > + /* Disable level-triggered IRQs until we handle them. */ > + if (!bus->alert_edge_triggered) > + disable_irq_nosync(irq); > + > + schedule_work(&bus->alert); > + return IRQ_HANDLED; > +} > + > +static int smbalert_probe(struct i2c_client *c, const struct i2c_device_id *id) > +{ > + return 0; > +} > + > +static int smbalert_remove(struct i2c_client *c) > +{ > + return 0; > +} Please reuse dummy_probe() and dummy_remove(), there's no point in duplicating these empty functions. > + > +static const struct i2c_device_id smbalert_ids[] = { > + { "smbus_alert", 0, }, > + { /* LIST END */ } > +}; > + > +static struct i2c_driver smbalert_driver = { > + .driver = { > + .name = "smbus_alert", > + }, > + .probe = smbalert_probe, > + .remove = smbalert_remove, > + .id_table = smbalert_ids, > +}; Or even better... get rid of smbalert_driver altogether and add { "smbus_alert", 0 } to dummy_driver instead. This will simplify the bookkeeping a lot. > + > +static const struct i2c_board_info ara_board_info = { > + I2C_BOARD_INFO("smbus_alert", 0x0c), > +}; > + > static void i2c_scan_static_board_info(struct i2c_adapter *adapter) > { > struct i2c_devinfo *devinfo; > @@ -448,6 +561,9 @@ static int i2c_register_adapter(struct i > mutex_init(&adap->clist_lock); > INIT_LIST_HEAD(&adap->clients); > > + /* Setup SMBALERT# infrastructure. */ > + INIT_WORK(&adap->alert, smbus_alert); > + > mutex_lock(&core_lock); > > /* Add the adapter to the driver core. > @@ -466,6 +582,33 @@ static int i2c_register_adapter(struct i > if (res) > goto out_list; > > + /* If we'll be handling SMBus alerts, register the alert responder > + * address so that nobody else can accidentally claim it. > + * Handling can be done either through our IRQ handler, or by the > + * adapter (from its handler, periodic polling, or whatever). > + * > + * NOTE that if we manage the IRQ, we *MUST* know if it's level or > + * edge triggered in order to hand it to the workqueue correctly. > + * If triggering the alert seems to wedge the system, you probably > + * should have said it's level triggered. > + */ > + if (adap->irq > 0 || adap->do_setup_alert) > + adap->ara = i2c_new_device(adap, &ara_board_info); > + > + if (adap->irq > 0 && adap->ara) { > + res = devm_request_irq(&adap->dev, adap->irq, smbus_irq, > + 0, "smbus_alert", adap); > + if (res == 0) { > + dev_info(&adap->dev, > + "supports SMBALERT#, %s trigger\n", > + adap->alert_edge_triggered > + ? "edge" : "level"); > + adap->has_alert_irq = 1; > + } else > + res = 0; > + Extra blank line. > + } > + > dev_dbg(&adap->dev, "adapter [%s] registered\n", adap->name); > > /* create pre-declared device nodes for new-style drivers */ > @@ -651,6 +794,12 @@ int i2c_del_adapter(struct i2c_adapter * > } > } > > + if (adap->has_alert_irq) { > + devm_free_irq(&adap->dev, adap->irq, adap); > + adap->has_alert_irq = 0; > + } > + cancel_work_sync(&adap->alert); > + > /* clean up the sysfs representation */ > init_completion(&adap->dev_released); > device_unregister(&adap->dev); > @@ -970,12 +1119,17 @@ static int __init i2c_init(void) > retval = class_register(&i2c_adapter_class); > if (retval) > goto bus_err; > + retval = i2c_add_driver(&smbalert_driver); > + if (retval) > + goto alert_err; > retval = i2c_add_driver(&dummy_driver); > if (retval) > goto class_err; > return 0; > > class_err: > + i2c_del_driver(&smbalert_driver); > +alert_err: > class_unregister(&i2c_adapter_class); > bus_err: > bus_unregister(&i2c_bus_type); > @@ -986,6 +1140,7 @@ static void __exit i2c_exit(void) > { > i2c_del_driver(&dummy_driver); > class_unregister(&i2c_adapter_class); > + i2c_del_driver(&smbalert_driver); > bus_unregister(&i2c_bus_type); > } > > --- a/include/linux/i2c.h > +++ b/include/linux/i2c.h > @@ -34,6 +34,7 @@ > #include <linux/device.h> /* for struct device */ > #include <linux/sched.h> /* for completion */ > #include <linux/mutex.h> > +#include <linux/workqueue.h> > > extern struct bus_type i2c_bus_type; > > @@ -165,6 +166,11 @@ struct i2c_driver { > int (*suspend)(struct i2c_client *, pm_message_t mesg); > int (*resume)(struct i2c_client *); > > + /* SMBus alert protocol support. The alert response's low bit > + * is the event flag (e.g. exceeding upper vs lower limit). > + */ > + void (*alert)(struct i2c_client *, bool flag); > + > /* a ioctl like command that can be used to perform specific functions > * with the device. > */ > @@ -369,6 +375,14 @@ struct i2c_adapter { > struct list_head clients; /* DEPRECATED */ > char name[48]; > struct completion dev_released; > + > + /* SMBALERT# support */ > + unsigned do_setup_alert:1; > + unsigned has_alert_irq:1; > + unsigned alert_edge_triggered:1; > + int irq; > + struct i2c_client *ara; > + struct work_struct alert; > }; > #define to_i2c_adapter(d) container_of(d, struct i2c_adapter, dev) It would be nice to have kernel doc for all these new struct members. I also would like you to document the new API, either in Documentation/i2c or Documentation/DocBook/kernel-api.tmpl, as you prefer. This new API is not trivial and there's only one example in the kernel tree at the moment (i2c-gpio) so driver authors will need detailed information on how to add support for SMBus alert in their bus and chip drivers. As a side note, I tried to add support for SMBus alert to the i2c-parport driver, but I can't get it to work. I'll post my patch later today in case someone has an idea what I am doing wrong. Thanks, -- Jean Delvare ^ permalink raw reply [flat|nested] 29+ messages in thread
[parent not found: <20081119145712.1abaa63f-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>]
* Re: [patch 2.6.27-rc7] i2c: smbalert# support [not found] ` <20081119145712.1abaa63f-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org> @ 2008-11-19 18:08 ` David Brownell 0 siblings, 0 replies; 29+ messages in thread From: David Brownell @ 2008-11-19 18:08 UTC (permalink / raw) To: Jean Delvare; +Cc: Linux I2C On Wednesday 19 November 2008, Jean Delvare wrote: > > +/* > > + * The alert IRQ handler needs to hand work off to a task which can issue > > + * SMBus calls, because those sleeping calls can't be made in IRQ context. > > + */ > > +static void smbus_alert(struct work_struct *work) > > +{ > > + struct i2c_adapter *bus; > > + > > + bus = container_of(work, struct i2c_adapter, alert); > > + for (;;) { > > + s32 status; > > + struct alert_data data; > > + > > + /* Devices with pending alerts reply in address order, low > > + * to high, because of slave transmit arbitration. After > > + * responding, an SMBus device stops asserting SMBALERT#. > > + * > > + * NOTE that SMBus 2.0 reserves 10-bit addresess for future > > + * use. We neither handle them, nor try to use PEC here. > > + */ > > + status = i2c_smbus_read_byte(bus->ara); > > + if (status < 0) > > + break; > > + > > + data.flag = (status & 1) != 0; > > This is equivalent to just "status & 1". If you rely on "true == 1", which I always like to avoid. I'll change that for you though, and make "flag" a u8. > > + data.addr = status >> 1; > > + dev_dbg(&bus->dev, "SMBALERT# %s from dev 0x%02x\n", > > + data.flag ? "true" : "false", data.addr); > > The flag is really only a data bit, it has no true/false meaning, so > presenting it that way is confusing. Please display it as 0/1 instead. For bits, "1" == "high" == "true" is the normal convention. But I changed it to "u8" and treat it as a number. > > + > > + /* Notify driver for the device which issued the alert. */ > > + device_for_each_child(&bus->dev, &data, i2c_do_alert); > > + } > > + > > + /* We handled all alerts; reenable level-triggered IRQs. */ > > + if (!bus->alert_edge_triggered) > > + enable_irq(bus->irq); > > +} > Or even better... get rid of smbalert_driver altogether and add > { "smbus_alert", 0 } to dummy_driver instead. This will simplify the > bookkeeping a lot. Done. :) > > @@ -369,6 +375,14 @@ struct i2c_adapter { > > struct list_head clients; /* DEPRECATED */ > > char name[48]; > > struct completion dev_released; > > + > > + /* SMBALERT# support */ > > + unsigned do_setup_alert:1; > > + unsigned has_alert_irq:1; > > + unsigned alert_edge_triggered:1; > > + int irq; > > + struct i2c_client *ara; > > + struct work_struct alert; > > }; > > #define to_i2c_adapter(d) container_of(d, struct i2c_adapter, dev) > > It would be nice to have kernel doc for all these new struct members. Easiest for i2c_client, which already has kerneldoc... > I also would like you to document the new API, either in > Documentation/i2c or Documentation/DocBook/kernel-api.tmpl, as you > prefer. This new API is not trivial and there's only one example in the > kernel tree at the moment (i2c-gpio) so driver authors will need > detailed information on how to add support for SMBus alert in their bus > and chip drivers. OK, I'll work on that. It won't be done this week though. Key point: three models for smbalert# configuration. (a) none, don't set those new values (b) adapter handles it, just set do_setup_alert flag and then schedule_work(&adapter->alert) as needed (c) i2c-core handles it, set irq (and maybe alert_edge_triggered) So (b) would be a missing example. > As a side note, I tried to add support for SMBus alert to the > i2c-parport driver, but I can't get it to work. I'll post my patch > later today in case someone has an idea what I am doing wrong. Parport IRQs ... I almost started to muck with those at one point. Right now only one of my systems even has a parport. ;) - Dave ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [patch 2.6.27-rc7] i2c: smbalert# support [not found] ` <200811181401.34809.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> 2008-11-19 9:51 ` [lm-sensors] " Trent Piepho 2008-11-19 13:57 ` Jean Delvare @ 2008-11-21 14:18 ` Jean Delvare [not found] ` <20081121151808.324ca78c-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org> 2 siblings, 1 reply; 29+ messages in thread From: Jean Delvare @ 2008-11-21 14:18 UTC (permalink / raw) To: David Brownell; +Cc: Linux I2C Hi David, On Tue, 18 Nov 2008 14:01:34 -0800, David Brownell wrote: > +struct alert_data { > + unsigned short addr; > + bool flag; > +}; > + > +/* If this is the alerting device, notify its driver. */ > +static int i2c_do_alert(struct device *dev, void *addrp) > +{ > + struct i2c_client *client = i2c_verify_client(dev); > + struct alert_data *data = addrp; > + > + if (!client || client->addr != data->addr) > + return 0; > + if (client->flags & I2C_CLIENT_TEN) > + return 0; > + > + /* Drivers should either disable alerts, or provide at least > + * a minimal handler. Lock so client->driver won't change. > + */ > + down(&dev->sem); > + if (client->driver) { > + if (client->driver->alert) > + client->driver->alert(client, data->flag); > + else > + dev_warn(&client->dev, "no driver alert()!\n"); > + } else > + dev_dbg(&client->dev, "alert with no driver\n"); > + up(&dev->sem); > + > + /* Stop iterating after we find the device. */ > + return -EBUSY; > +} > + > +/* > + * The alert IRQ handler needs to hand work off to a task which can issue > + * SMBus calls, because those sleeping calls can't be made in IRQ context. > + */ > +static void smbus_alert(struct work_struct *work) > +{ > + struct i2c_adapter *bus; > + > + bus = container_of(work, struct i2c_adapter, alert); > + for (;;) { > + s32 status; > + struct alert_data data; > + > + /* Devices with pending alerts reply in address order, low > + * to high, because of slave transmit arbitration. After > + * responding, an SMBus device stops asserting SMBALERT#. > + * > + * NOTE that SMBus 2.0 reserves 10-bit addresess for future > + * use. We neither handle them, nor try to use PEC here. > + */ > + status = i2c_smbus_read_byte(bus->ara); > + if (status < 0) > + break; > + > + data.flag = (status & 1) != 0; > + data.addr = status >> 1; > + dev_dbg(&bus->dev, "SMBALERT# %s from dev 0x%02x\n", > + data.flag ? "true" : "false", data.addr); > + > + /* Notify driver for the device which issued the alert. */ > + device_for_each_child(&bus->dev, &data, i2c_do_alert); > + } > + > + /* We handled all alerts; reenable level-triggered IRQs. */ > + if (!bus->alert_edge_triggered) > + enable_irq(bus->irq); > +} I got this function stuck in an endless loop while I was experimenting with my parallel port evaluation board. You might argue that this is because my code was bogus or incomplete and that would be correct, but nevertheless, I don't think we should give driver authors the possibility to bring their system down that way. Not to mention the possibility of broken hardware answering to the ARA when it shouldn't. So, I propose the following addition to make sure that we can't get stuck in such an endless loop: From: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> Subject: i2c: Dismiss unhandled SMBus alerts If a given I2C driver doesn't handle SMBus alerts properly, then there is a risk that smbus_alert() will loop forever. We really do not want this to happen, so add a detection mechanism which will interrupt the loop in such a case. Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> Cc: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org> --- drivers/i2c/i2c-core.c | 8 ++++++++ 1 file changed, 8 insertions(+) --- linux-2.6.28-rc6.orig/drivers/i2c/i2c-core.c 2008-11-21 11:01:53.000000000 +0100 +++ linux-2.6.28-rc6/drivers/i2c/i2c-core.c 2008-11-21 14:55:29.000000000 +0100 @@ -446,6 +446,7 @@ static int i2c_do_alert(struct device *d static void smbus_alert(struct work_struct *work) { struct i2c_adapter *bus; + unsigned short prev_addr = 0; bus = container_of(work, struct i2c_adapter, alert); for (;;) { @@ -465,11 +466,18 @@ static void smbus_alert(struct work_stru data.flag = (status & 1) != 0; data.addr = status >> 1; + + if (data.addr == prev_addr) { + dev_warn(&bus->dev, "Duplicate SMBALERT# from dev " + "0x%02x, skipping\n", data.addr); + break; + } dev_dbg(&bus->dev, "SMBALERT# %s from dev 0x%02x\n", data.flag ? "true" : "false", data.addr); /* Notify driver for the device which issued the alert. */ device_for_each_child(&bus->dev, &data, i2c_do_alert); + prev_addr = data.addr; } /* We handled all alerts; reenable level-triggered IRQs. */ What do you think? Thanks, -- Jean Delvare ^ permalink raw reply [flat|nested] 29+ messages in thread
[parent not found: <20081121151808.324ca78c-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>]
* Re: [patch 2.6.27-rc7] i2c: smbalert# support [not found] ` <20081121151808.324ca78c-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org> @ 2008-11-21 16:24 ` David Brownell [not found] ` <200811210824.55601.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> 0 siblings, 1 reply; 29+ messages in thread From: David Brownell @ 2008-11-21 16:24 UTC (permalink / raw) To: Jean Delvare; +Cc: Linux I2C On Friday 21 November 2008, Jean Delvare wrote: > --- linux-2.6.28-rc6.orig/drivers/i2c/i2c-core.c 2008-11-21 11:01:53.000000000 +0100 > +++ linux-2.6.28-rc6/drivers/i2c/i2c-core.c 2008-11-21 14:55:29.000000000 +0100 > @@ -446,6 +446,7 @@ static int i2c_do_alert(struct device *d > static void smbus_alert(struct work_struct *work) > { > struct i2c_adapter *bus; > + unsigned short prev_addr = 0; > > bus = container_of(work, struct i2c_adapter, alert); > for (;;) { > @@ -465,11 +466,18 @@ static void smbus_alert(struct work_stru > > data.flag = (status & 1) != 0; > data.addr = status >> 1; > + > + if (data.addr == prev_addr) { > + dev_warn(&bus->dev, "Duplicate SMBALERT# from dev " > + "0x%02x, skipping\n", data.addr); > + break; > + } > dev_dbg(&bus->dev, "SMBALERT# %s from dev 0x%02x\n", > data.flag ? "true" : "false", data.addr); > > /* Notify driver for the device which issued the alert. */ > device_for_each_child(&bus->dev, &data, i2c_do_alert); > + prev_addr = data.addr; > } > > /* We handled all alerts; reenable level-triggered IRQs. */ > > What do you think? It's a start. If this is a level-triggered IRQ and the device is still raising the IRQ ... we'd need something more drastic, since the IRQ itself would be stuck on; wouldn't want to re-enable it. Edge triggered IRQs would be easier to cope with. Do you have a handle on why the device was malfunctioning? - Dave ^ permalink raw reply [flat|nested] 29+ messages in thread
[parent not found: <200811210824.55601.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>]
* Re: [patch 2.6.27-rc7] i2c: smbalert# support [not found] ` <200811210824.55601.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> @ 2008-11-21 19:22 ` Jean Delvare [not found] ` <20081121202223.0261fb9c-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org> 0 siblings, 1 reply; 29+ messages in thread From: Jean Delvare @ 2008-11-21 19:22 UTC (permalink / raw) To: David Brownell; +Cc: Linux I2C Hi David, On Fri, 21 Nov 2008 08:24:55 -0800, David Brownell wrote: > On Friday 21 November 2008, Jean Delvare wrote: > > --- linux-2.6.28-rc6.orig/drivers/i2c/i2c-core.c 2008-11-21 11:01:53.000000000 +0100 > > +++ linux-2.6.28-rc6/drivers/i2c/i2c-core.c 2008-11-21 14:55:29.000000000 +0100 > > @@ -446,6 +446,7 @@ static int i2c_do_alert(struct device *d > > static void smbus_alert(struct work_struct *work) > > { > > struct i2c_adapter *bus; > > + unsigned short prev_addr = 0; > > > > bus = container_of(work, struct i2c_adapter, alert); > > for (;;) { > > @@ -465,11 +466,18 @@ static void smbus_alert(struct work_stru > > > > data.flag = (status & 1) != 0; > > data.addr = status >> 1; > > + > > + if (data.addr == prev_addr) { > > + dev_warn(&bus->dev, "Duplicate SMBALERT# from dev " > > + "0x%02x, skipping\n", data.addr); > > + break; > > + } > > dev_dbg(&bus->dev, "SMBALERT# %s from dev 0x%02x\n", > > data.flag ? "true" : "false", data.addr); > > > > /* Notify driver for the device which issued the alert. */ > > device_for_each_child(&bus->dev, &data, i2c_do_alert); > > + prev_addr = data.addr; > > } > > > > /* We handled all alerts; reenable level-triggered IRQs. */ > > > > What do you think? > > It's a start. If this is a level-triggered IRQ and the device is > still raising the IRQ ... we'd need something more drastic, since > the IRQ itself would be stuck on; wouldn't want to re-enable it. > Edge triggered IRQs would be easier to cope with. In my case it was an edge-triggered interrupt: 7: 8 IO-APIC-edge parport0 For a level-triggering interrupt, I'd say it is up to the bus driver to disable it before calling smbus_alert(), as you did in smbus_irq()? > Do you have a handle on why the device was malfunctioning? The device was behaving as intended, the problem was that I had not yet implemented the alert() callback in the device driver. The device (ADM1032) keeps the ALERT# line low as long as the alarm condition exists, so smbus_alert() would receive the same address over and over again. I have now updated the device driver to mask out the ALERT# signal once it has triggered, until the alarm has worn off, at which point I unmask the ALERT# signal again. So it works OK now (I'll post the patch in a minute), but this is a condition that could easily happen for other devices / developers out there, so I think it's better to make the i2c-core code robust enough to handle it. -- Jean Delvare ^ permalink raw reply [flat|nested] 29+ messages in thread
[parent not found: <20081121202223.0261fb9c-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>]
* Re: [patch 2.6.27-rc7] i2c: smbalert# support [not found] ` <20081121202223.0261fb9c-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org> @ 2008-11-21 21:54 ` David Brownell [not found] ` <200811211354.51501.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> 0 siblings, 1 reply; 29+ messages in thread From: David Brownell @ 2008-11-21 21:54 UTC (permalink / raw) To: Jean Delvare; +Cc: Linux I2C On Friday 21 November 2008, Jean Delvare wrote: > > > > It's a start. If this is a level-triggered IRQ and the device is > > still raising the IRQ ... we'd need something more drastic, since > > the IRQ itself would be stuck on; wouldn't want to re-enable it. > > Edge triggered IRQs would be easier to cope with. > > In my case it was an edge-triggered interrupt: > > 7: 8 IO-APIC-edge parport0 > > For a level-triggering interrupt, I'd say it is up to the bus driver to > disable it before calling smbus_alert(), as you did in smbus_irq()? Yes, but then ... what would re-enable it? A mechanism seems to be missing; maybe a callback, for bus drivers that just schedule_work(&adapter->alert), or having them call the relevant routine in a task context instead of letting i2c-core provide that context. > > Do you have a handle on why the device was malfunctioning? > > The device was behaving as intended, the problem was that I had not yet > implemented the alert() callback in the device driver. The device > (ADM1032) keeps the ALERT# line low as long as the alarm condition > exists, so smbus_alert() would receive the same address over and over > again. Hmm, that "keep ALERT# low" behavior is contrary to what I took away from the SMBus spec: "After acknowledging the slave address the device must disengage its SMBALERT# pulldown." This ADM1032 isn't doing that; it only "disengages" when the alarm condition eventually goes away. That would be particularly bad if it was raising an alert but there was no driver for it at all! Maybe the fault cases (no alert method, or now driver) in i2c_do_alert() should have special return codes so that the code scanning the bus for alerting devices can be a bit smarter. > I have now updated the device driver to mask out the ALERT# signal once > it has triggered, until the alarm has worn off, at which point I unmask > the ALERT# signal again. So it works OK now (I'll post the patch in a > minute), but this is a condition that could easily happen for other > devices / developers out there, so I think it's better to make the > i2c-core code robust enough to handle it. Agreed, defend against this misbehavior. But I can't think of a more robust defence than leaving the IRQ disabled when it misbehaves. - Dave ^ permalink raw reply [flat|nested] 29+ messages in thread
[parent not found: <200811211354.51501.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>]
* Re: [patch 2.6.27-rc7] i2c: smbalert# support [not found] ` <200811211354.51501.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> @ 2008-11-22 9:03 ` Jean Delvare [not found] ` <20081122100344.61cf42b7-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org> 0 siblings, 1 reply; 29+ messages in thread From: Jean Delvare @ 2008-11-22 9:03 UTC (permalink / raw) To: David Brownell; +Cc: Linux I2C Hi David, On Fri, 21 Nov 2008 13:54:50 -0800, David Brownell wrote: > On Friday 21 November 2008, Jean Delvare wrote: > > In my case it was an edge-triggered interrupt: > > > > 7: 8 IO-APIC-edge parport0 > > > > For a level-triggering interrupt, I'd say it is up to the bus driver to > > disable it before calling smbus_alert(), as you did in smbus_irq()? > > Yes, but then ... what would re-enable it? A mechanism > seems to be missing; maybe a callback, for bus drivers > that just schedule_work(&adapter->alert), or having them > call the relevant routine in a task context instead of > letting i2c-core provide that context. Well, if bus->alert_edge_triggered is set to 0 and bus->irq is properly set, smbus_alert() is taking care of re-enabling the IRQ, isn't it? OTOH I admit that I was a little surprised that I had to set bus->alert_edge_triggered while while my driver also sets bus->do_setup_alert. But as long as it is properly documented, I guess this is OK. > > The device was behaving as intended, the problem was that I had not yet > > implemented the alert() callback in the device driver. The device > > (ADM1032) keeps the ALERT# line low as long as the alarm condition > > exists, so smbus_alert() would receive the same address over and over > > again. > > Hmm, that "keep ALERT# low" behavior is contrary to what I took away > from the SMBus spec: "After acknowledging the slave address the device > must disengage its SMBALERT# pulldown." This ADM1032 isn't doing that; > it only "disengages" when the alarm condition eventually goes away. Indeed, this doesn't match the specification. I'm not necessarily surprised, as the same ADM1032 also doesn't implement SMBus PEC properly. Apparently Analog Devices didn't put too much engineering into sticking to the SMBus specification :( At least there is a way to mask out the ALERT# output, so this can be addressed in the driver. Otherwise the ADM1032 wouldn't be usable at all... > That would be particularly bad if it was raising an alert but there > was no driver for it at all! True. And obviously this can happen. > Maybe the fault cases (no alert method, or now driver) in i2c_do_alert() > should have special return codes so that the code scanning the bus for > alerting devices can be a bit smarter. I have been thinking about this too. But OTOH, you also want to properly handle the case where there is a driver and it has a callback but this callback is misbehaving. So in the end I think it's safer to focus on the consequence (same slave answering to ARA several times) than the multiple possible causes. > > I have now updated the device driver to mask out the ALERT# signal once > > it has triggered, until the alarm has worn off, at which point I unmask > > the ALERT# signal again. So it works OK now (I'll post the patch in a > > minute), but this is a condition that could easily happen for other > > devices / developers out there, so I think it's better to make the > > i2c-core code robust enough to handle it. > > Agreed, defend against this misbehavior. But I can't think of a more > robust defence than leaving the IRQ disabled when it misbehaves. Except that you don't necessarily have control over the IRQ in question. At least in my case, the i2c-parport driver sets bus->alert_edge_triggered to 1 and bus->do_setup_alert to 1, which basically means that it is handling the IRQ on its own and i2c-core won't touch it. So if you want i2c-core to be more intrusive, you must make setting bus->irq mandatory. I have no objection, again as long as it is properly documented. -- Jean Delvare ^ permalink raw reply [flat|nested] 29+ messages in thread
[parent not found: <20081122100344.61cf42b7-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>]
* Re: [patch 2.6.27-rc7] i2c: smbalert# support [not found] ` <20081122100344.61cf42b7-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org> @ 2008-11-22 9:48 ` David Brownell 0 siblings, 0 replies; 29+ messages in thread From: David Brownell @ 2008-11-22 9:48 UTC (permalink / raw) To: Jean Delvare; +Cc: Linux I2C On Saturday 22 November 2008, Jean Delvare wrote: > Hi David, > > On Fri, 21 Nov 2008 13:54:50 -0800, David Brownell wrote: > > On Friday 21 November 2008, Jean Delvare wrote: > > > In my case it was an edge-triggered interrupt: > > > > > > 7: 8 IO-APIC-edge parport0 > > > > > > For a level-triggering interrupt, I'd say it is up to the bus driver to > > > disable it before calling smbus_alert(), as you did in smbus_irq()? > > > > Yes, but then ... what would re-enable it? A mechanism > > seems to be missing; maybe a callback, for bus drivers > > that just schedule_work(&adapter->alert), or having them > > call the relevant routine in a task context instead of > > letting i2c-core provide that context. > > Well, if bus->alert_edge_triggered is set to 0 and bus->irq is properly > set, smbus_alert() is taking care of re-enabling the IRQ, isn't it? I was mixing up a few issues in my mind. If it's a level-triggered interrupt, and the device is flaking out and still asserting it even after it responded to the ARA ... then fault isolation calls for leaving it disabled. Then how could it ever recover? > OTOH I admit that I was a little surprised that I had to set > bus->alert_edge_triggered while while my driver also sets > bus->do_setup_alert. But as long as it is properly documented, I guess > this is OK. I don't see a way around that, until we get to threaded IRQ handlers. In which case lots of this could just be part of a threaded handler... which would mask and unmask directly. It might be worth waiting till Thomas sends the threaded IRQ stuff for merge. It will make the interface tradeoffs a bit different, and one notion was to merge that stuff for 2.6.29 ... > > > The device was behaving as intended, the problem was that I had not yet > > > implemented the alert() callback in the device driver. The device > > > (ADM1032) keeps the ALERT# line low as long as the alarm condition > > > exists, so smbus_alert() would receive the same address over and over > > > again. > > > > Hmm, that "keep ALERT# low" behavior is contrary to what I took away > > from the SMBus spec: "After acknowledging the slave address the device > > must disengage its SMBALERT# pulldown." This ADM1032 isn't doing that; > > it only "disengages" when the alarm condition eventually goes away. > > Indeed, this doesn't match the specification. I'm not necessarily > surprised, as the same ADM1032 also doesn't implement SMBus PEC > properly. Apparently Analog Devices didn't put too much engineering > into sticking to the SMBus specification :( At least there is a way to > mask out the ALERT# output, so this can be addressed in the driver. > Otherwise the ADM1032 wouldn't be usable at all... My own SMBALERT# testing involved a TI sensor, which worked like a charm, and some AVR (ATtiny) firmware, ditto. > > That would be particularly bad if it was raising an alert but there > > was no driver for it at all! > > True. And obviously this can happen. Hence the "leave it disabled" thought ... > > Maybe the fault cases (no alert method, or now driver) in i2c_do_alert() > > should have special return codes so that the code scanning the bus for > > alerting devices can be a bit smarter. > > I have been thinking about this too. But OTOH, you also want to > properly handle the case where there is a driver and it has a callback > but this callback is misbehaving. So in the end I think it's safer to > focus on the consequence (same slave answering to ARA several times) > than the multiple possible causes. That's what I concluded too. > > > I have now updated the device driver to mask out the ALERT# signal once > > > it has triggered, until the alarm has worn off, at which point I unmask > > > the ALERT# signal again. So it works OK now (I'll post the patch in a > > > minute), but this is a condition that could easily happen for other > > > devices / developers out there, so I think it's better to make the > > > i2c-core code robust enough to handle it. > > > > Agreed, defend against this misbehavior. But I can't think of a more > > robust defence than leaving the IRQ disabled when it misbehaves. > > Except that you don't necessarily have control over the IRQ in > question. At least in my case, the i2c-parport driver sets > bus->alert_edge_triggered to 1 and bus->do_setup_alert to 1, which > basically means that it is handling the IRQ on its own and i2c-core > won't touch it. So if you want i2c-core to be more intrusive, you must > make setting bus->irq mandatory. I have no objection, again as long as > it is properly documented. Setting bus->irq won't work on common cases like ICH8, where the SMBALERT# interrupt is just one more subcase. - Dave > > -- > Jean Delvare > > ^ permalink raw reply [flat|nested] 29+ messages in thread
end of thread, other threads:[~2008-11-23 21:45 UTC | newest]
Thread overview: 29+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <200804161434.54335.laurentp@cse-semaphore.com>
[not found] ` <20080416153043.02f89554@hyperion.delvare>
[not found] ` <200804161027.49943.david-b@pacbell.net>
[not found] ` <200804161027.49943.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>
2008-05-02 11:10 ` [patch 2.6.25-git] i2c: smbalert# support David Brownell
[not found] ` <200805020410.44723.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>
2008-05-05 5:56 ` [patch 2.6.265-rc1] " David Brownell
[not found] ` <200805042256.49252.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>
2008-09-23 22:32 ` [patch 2.6.27-rc7] " David Brownell
[not found] ` <200809231532.40083.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>
2008-09-26 1:07 ` [lm-sensors] " Trent Piepho
[not found] ` <Pine.LNX.4.64.0809251728130.7680-3bmvVOk6DZ+DGx/iekXGtrjh7zpefjiS@public.gmane.org>
2008-09-26 2:01 ` David Brownell
[not found] ` <200809251902.00201.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>
2008-09-27 0:29 ` Trent Piepho
[not found] ` <Pine.LNX.4.64.0809261652040.7680-3bmvVOk6DZ+DGx/iekXGtrjh7zpefjiS@public.gmane.org>
2008-10-17 19:04 ` David Brownell
[not found] ` <200810171204.54448.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>
2008-10-17 20:43 ` Trent Piepho
2008-11-18 8:15 ` Jean Delvare
[not found] ` <20081118091546.421d6b78-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>
2008-11-18 22:01 ` David Brownell
[not found] ` <200811181401.34809.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>
2008-11-19 9:51 ` [lm-sensors] " Trent Piepho
[not found] ` <Pine.LNX.4.64.0811190140510.11673-3bmvVOk6DZ+DGx/iekXGtrjh7zpefjiS@public.gmane.org>
2008-11-19 15:16 ` Jean Delvare
[not found] ` <20081119161632.2d0bde9e-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>
2008-11-20 22:00 ` Trent Piepho
[not found] ` <Pine.LNX.4.64.0811201354310.11673-3bmvVOk6DZ+DGx/iekXGtrjh7zpefjiS@public.gmane.org>
2008-11-20 22:56 ` David Brownell
[not found] ` <200811201456.36551.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>
2008-11-22 0:55 ` Trent Piepho
[not found] ` <Pine.LNX.4.64.0811211621340.18022-3bmvVOk6DZ+DGx/iekXGtrjh7zpefjiS@public.gmane.org>
2008-11-22 2:58 ` David Brownell
2008-11-21 8:42 ` Jean Delvare
[not found] ` <20081121094218.34ecd82a-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>
2008-11-22 6:04 ` Trent Piepho
[not found] ` <Pine.LNX.4.64.0811212122370.18022-3bmvVOk6DZ+DGx/iekXGtrjh7zpefjiS@public.gmane.org>
2008-11-22 10:13 ` Jean Delvare
[not found] ` <20081122111340.0bfc2c05-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>
2008-11-22 20:28 ` Trent Piepho
[not found] ` <Pine.LNX.4.64.0811221055470.18022-3bmvVOk6DZ+DGx/iekXGtrjh7zpefjiS@public.gmane.org>
2008-11-23 21:45 ` Jean Delvare
2008-11-19 13:57 ` Jean Delvare
[not found] ` <20081119145712.1abaa63f-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>
2008-11-19 18:08 ` David Brownell
2008-11-21 14:18 ` Jean Delvare
[not found] ` <20081121151808.324ca78c-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>
2008-11-21 16:24 ` David Brownell
[not found] ` <200811210824.55601.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>
2008-11-21 19:22 ` Jean Delvare
[not found] ` <20081121202223.0261fb9c-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>
2008-11-21 21:54 ` David Brownell
[not found] ` <200811211354.51501.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>
2008-11-22 9:03 ` Jean Delvare
[not found] ` <20081122100344.61cf42b7-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>
2008-11-22 9:48 ` David Brownell
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox