[patch 2.6.25-git] i2c: smbalert# support

[patch 2.6.25-git] i2c: smbalert# support

[David Brownell]

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

[patch 2.6.265-rc1] i2c: smbalert# support

[David Brownell]

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

[patch 2.6.27-rc7] i2c: smbalert# support

[David Brownell]

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

Re: [lm-sensors] [patch 2.6.27-rc7] i2c: smbalert# support

[Trent Piepho]

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

Re: [lm-sensors] [patch 2.6.27-rc7] i2c: smbalert# support

[David Brownell]

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

Re: [lm-sensors] [patch 2.6.27-rc7] i2c: smbalert# support

[Trent Piepho]

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

Re: [lm-sensors] [patch 2.6.27-rc7] i2c: smbalert# support

[David Brownell]

... 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

Re: [lm-sensors] [patch 2.6.27-rc7] i2c: smbalert# support

[Trent Piepho]

[-- 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

Re: [patch 2.6.27-rc7] i2c: smbalert# support

[Jean Delvare]

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

Re: [patch 2.6.27-rc7] i2c: smbalert# support

[David Brownell]

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)
 

Re: [lm-sensors] [patch 2.6.27-rc7] i2c: smbalert# support

[Trent Piepho]

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?

Re: [lm-sensors] [patch 2.6.27-rc7] i2c: smbalert# support

[Jean Delvare]

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

Re: [lm-sensors] [patch 2.6.27-rc7] i2c: smbalert# support

[Trent Piepho]

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?

Re: [lm-sensors] [patch 2.6.27-rc7] i2c: smbalert# support

[David Brownell]

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

Re: [lm-sensors] [patch 2.6.27-rc7] i2c: smbalert# support

[Trent Piepho]

[-- 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.

Re: [lm-sensors] [patch 2.6.27-rc7] i2c: smbalert# support

[David Brownell]

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.

Re: [patch 2.6.27-rc7] i2c: smbalert# support

[Jean Delvare]

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

Re: [patch 2.6.27-rc7] i2c: smbalert# support

[Trent Piepho]

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.

Re: [patch 2.6.27-rc7] i2c: smbalert# support

[Jean Delvare]

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

Re: [patch 2.6.27-rc7] i2c: smbalert# support

[Trent Piepho]

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

Re: [patch 2.6.27-rc7] i2c: smbalert# support

[Jean Delvare]

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

Re: [patch 2.6.27-rc7] i2c: smbalert# support

[Jean Delvare]

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

Re: [patch 2.6.27-rc7] i2c: smbalert# support

[David Brownell]

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

Re: [patch 2.6.27-rc7] i2c: smbalert# support

[Jean Delvare]

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

Re: [patch 2.6.27-rc7] i2c: smbalert# support

[David Brownell]

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

Re: [patch 2.6.27-rc7] i2c: smbalert# support

[Jean Delvare]

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

Re: [patch 2.6.27-rc7] i2c: smbalert# support

[David Brownell]

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

Re: [patch 2.6.27-rc7] i2c: smbalert# support

[Jean Delvare]

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

Re: [patch 2.6.27-rc7] i2c: smbalert# support

[David Brownell]

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
> 
>