[PATCH 0/5] i2c: Add SMBus alert support

[PATCH 0/5] i2c: Add SMBus alert support

[Jean Delvare]

Hi all,

This patchset adds SMBus alert support.

[PATCH 1/5] i2c: Add SMBus alert support
[PATCH 2/5] i2c: Separate Kconfig option for i2c-smbus
[PATCH 3/5] i2c-parport: Add SMBus alert support
[PATCH 4/5] i2c-parport-light: Add SMBus alert support
[PATCH 5/5] hwmon: (lm90) Add SMBus alert support

Reviews and testing welcome.

-- 
Jean Delvare

[PATCH 1/5] i2c: Add SMBus alert support

[Jean Delvare]

SMBus alert support. The SMBus alert protocol allows several SMBus
slave devices to share a single interrupt pin on the SMBus master,
while still allowing the master to know which slave triggered the
interrupt.

This is based on preliminary work by David Brownell. The key
difference between David's implementation and mine is that his was
part of i2c-core, while mine is split into a separate, standalone
module named i2c-smbus. The i2c-smbus module is meant to include
support for all SMBus extensions to the I2C protocol in the future.

The benefit of this approach is a zero cost for I2C bus segments which
do not need SMBus alert support. Where David's implementation
increased the size of struct i2c_adapter by 7% (40 bytes on i386),
mine doesn't touch it. Where David's implementation added over 150
lines of code to i2c-core (+10%), mine doesn't touch it. The only
change that touches all the users of the i2c subsystem is a new
callback in struct i2c_driver (common to both implementations.) I seem
to remember Trent was worried about the footprint of David'd
implementation, hopefully mine addresses the issue.

Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
Cc: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
Cc: Trent Piepho <tpiepho-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
---
 Documentation/i2c/smbus-protocol |   16 ++
 drivers/i2c/Makefile             |    2 
 drivers/i2c/i2c-smbus.c          |  264 ++++++++++++++++++++++++++++++++++++++
 include/linux/i2c-smbus.h        |   50 +++++++
 include/linux/i2c.h              |    7 +
 5 files changed, 338 insertions(+), 1 deletion(-)

--- linux-2.6.33-rc7.orig/Documentation/i2c/smbus-protocol	2010-02-12 14:19:47.000000000 +0100
+++ linux-2.6.33-rc7/Documentation/i2c/smbus-protocol	2010-02-12 14:19:51.000000000 +0100
@@ -185,6 +185,22 @@ the protocol. All ARP communications use
 require PEC checksums.
 
 
+SMBus Alert
+===========
+
+SMBus Alert was introduced in Revision 1.0 of the specification.
+
+The SMBus alert protocol allows several SMBus slave devices to share a
+single interrupt pin on the SMBus master, while still allowing the master
+to know which slave triggered the interrupt.
+
+This is implemented the following way in the Linux kernel:
+* I2C bus drivers which support SMBus alert should call
+  i2c_setup_smbus_alert() to setup SMBus alert support.
+* I2C drivers for devices which can trigger SMBus alerts should implement
+  the optional alert() callback.
+
+
 I2C Block Transactions
 ======================
 
--- linux-2.6.33-rc7.orig/drivers/i2c/Makefile	2010-02-12 14:19:47.000000000 +0100
+++ linux-2.6.33-rc7/drivers/i2c/Makefile	2010-02-12 16:08:34.000000000 +0100
@@ -3,7 +3,7 @@
 #
 
 obj-$(CONFIG_I2C_BOARDINFO)	+= i2c-boardinfo.o
-obj-$(CONFIG_I2C)		+= i2c-core.o
+obj-$(CONFIG_I2C)		+= i2c-core.o i2c-smbus.o
 obj-$(CONFIG_I2C_CHARDEV)	+= i2c-dev.o
 obj-y				+= busses/ chips/ algos/
 
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux-2.6.33-rc7/drivers/i2c/i2c-smbus.c	2010-02-12 16:11:34.000000000 +0100
@@ -0,0 +1,264 @@
+/*
+ * i2c-smbus.c - SMBus extensions to the I2C protocol
+ *
+ * Copyright (C) 2010 Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
+ *
+ * SMBus alert support based on earlier work by David Brownell (2008).
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/semaphore.h>
+#include <linux/interrupt.h>
+#include <linux/workqueue.h>
+#include <linux/i2c.h>
+#include <linux/i2c-smbus.h>
+
+struct i2c_smbus_alert {
+	unsigned int		alert_edge_triggered:1;
+	int			irq;
+	struct work_struct	alert;
+	struct i2c_client	*ara;
+};
+
+struct alert_data {
+	unsigned short		addr;
+	u8			flag:1;
+};
+
+/* If this is the alerting device, notify its driver */
+static int smbus_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_smbus_alert *data;
+	struct i2c_client *ara;
+	unsigned short prev_addr = 0;	/* Not a valid address */
+
+	data = container_of(work, struct i2c_smbus_alert, alert);
+	ara = data->ara;
+
+	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(ara);
+		if (status < 0)
+			break;
+
+		data.flag = status & 1;
+		data.addr = status >> 1;
+
+		if (data.addr == prev_addr) {
+			dev_warn(&ara->dev, "Duplicate SMBALERT# from dev "
+				"0x%02x, skipping\n", data.addr);
+			break;
+		}
+		dev_dbg(&ara->dev, "SMBALERT# from dev 0x%02x, flag %d\n",
+			data.addr, data.flag);
+
+		/* Notify driver for the device which issued the alert */
+		device_for_each_child(&ara->adapter->dev, &data,
+				      smbus_do_alert);
+		prev_addr = data.addr;
+	}
+
+	/* We handled all alerts; re-enable level-triggered IRQs */
+	if (!data->alert_edge_triggered)
+		enable_irq(data->irq);
+}
+
+static irqreturn_t smbalert_irq(int irq, void *d)
+{
+	struct i2c_smbus_alert *data = d;
+
+	/* Disable level-triggered IRQs until we handle them */
+	if (!data->alert_edge_triggered)
+		disable_irq_nosync(irq);
+
+	schedule_work(&data->alert);
+	return IRQ_HANDLED;
+}
+
+/* Setup SMBALERT# infrastructure */
+static int smbalert_probe(struct i2c_client *ara,
+			  const struct i2c_device_id *id)
+{
+	struct i2c_smbus_alert_setup *setup = ara->dev.platform_data;
+	struct i2c_smbus_alert *data;
+	struct i2c_adapter *adapter = ara->adapter;
+	int res;
+
+	data = kzalloc(sizeof(struct i2c_smbus_alert), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->alert_edge_triggered = setup->alert_edge_triggered;
+	data->irq = setup->irq;
+	INIT_WORK(&data->alert, smbus_alert);
+	data->ara = ara;
+
+	if (setup->irq > 0) {
+		res = devm_request_irq(&ara->dev, setup->irq, smbalert_irq,
+				       0, "smbus_alert", data);
+		if (res) {
+			kfree(data);
+			return res;
+		}
+	}
+
+	i2c_set_clientdata(ara, data);
+	dev_info(&adapter->dev, "supports SMBALERT#, %s trigger\n",
+		 setup->alert_edge_triggered ? "edge" : "level");
+
+	return 0;
+}
+
+/* IRQ resource is managed so it is freed automatically */
+static int smbalert_remove(struct i2c_client *ara)
+{
+	struct i2c_smbus_alert *data = i2c_get_clientdata(ara);
+
+	cancel_work_sync(&data->alert);
+
+	i2c_set_clientdata(ara, NULL);
+	kfree(data);
+	return 0;
+}
+
+static const struct i2c_device_id smbalert_ids[] = {
+	{ "smbus_alert", 0 },
+	{ /* LIST END */ }
+};
+MODULE_DEVICE_TABLE(i2c, smbalert_ids);
+
+static struct i2c_driver smbalert_driver = {
+	.driver = {
+		.name	= "smbus_alert",
+	},
+	.probe		= smbalert_probe,
+	.remove		= smbalert_remove,
+	.id_table	= smbalert_ids,
+};
+
+/**
+ * i2c_setup_smbus_alert - Setup SMBus alert support
+ * @adapter: the target adapter
+ * @setup: setup data for the SMBus alert handler
+ * Context: can sleep
+ *
+ * Setup handling of the SMBus alert protocol on a given I2C bus segment.
+ *
+ * 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.
+ *
+ * This returns the ara client, which should be saved for later use with
+ * i2c_handle_smbus_alert() and ultimately i2c_unregister_device(); or NULL
+ * to indicate an error.
+ */
+struct i2c_client *i2c_setup_smbus_alert(struct i2c_adapter *adapter,
+					 struct i2c_smbus_alert_setup *setup)
+{
+	struct i2c_board_info ara_board_info = {
+		I2C_BOARD_INFO("smbus_alert", 0x0c),
+		.platform_data = setup,
+	};
+
+	return i2c_new_device(adapter, &ara_board_info);
+}
+EXPORT_SYMBOL_GPL(i2c_setup_smbus_alert);
+
+/**
+ * i2c_handle_smbus_alert - Handle an SMBus alert
+ * @ara: the ARA client on the relevant adapter
+ * Context: can't sleep
+ *
+ * Helper function to be called from an I2C bus driver's interrupt
+ * handler. It will schedule the alert work, in turn calling the
+ * corresponding I2C device driver's alert function.
+ *
+ * It is assumed that ara is a valid i2c client previously returned by
+ * i2c_setup_smbus_alert().
+ */
+int i2c_handle_smbus_alert(struct i2c_client *ara)
+{
+	struct i2c_smbus_alert *data = i2c_get_clientdata(ara);
+
+	return schedule_work(&data->alert);
+}
+EXPORT_SYMBOL_GPL(i2c_handle_smbus_alert);
+
+static int __init i2c_smbus_init(void)
+{
+	return i2c_add_driver(&smbalert_driver);
+}
+
+static void __exit i2c_smbus_exit(void)
+{
+	i2c_del_driver(&smbalert_driver);
+}
+
+module_init(i2c_smbus_init);
+module_exit(i2c_smbus_exit);
+
+MODULE_AUTHOR("Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>");
+MODULE_DESCRIPTION("SMBus protocol extensions support");
+MODULE_LICENSE("GPL");
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux-2.6.33-rc7/include/linux/i2c-smbus.h	2010-02-12 16:08:43.000000000 +0100
@@ -0,0 +1,50 @@
+/*
+ * i2c-smbus.h - SMBus extensions to the I2C protocol
+ *
+ * Copyright (C) 2010 Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#ifndef _LINUX_I2C_SMBUS_H
+#define _LINUX_I2C_SMBUS_H
+
+#include <linux/i2c.h>
+
+
+/**
+ * i2c_smbus_alert_setup - platform data for the smbus_alert i2c client
+ * @alert_edge_triggered: whether the alert interrupt is edge (1) or level (0)
+ *		triggered
+ * @irq: IRQ number, if the smbus_alert driver should take care of interrupt
+ *		handling
+ *
+ * If irq is not specified, the smbus_alert driver doesn't take care of
+ * interrupt handling. In that case it is up to the I2C bus driver to either
+ * handle the interrupts or to poll for alerts.
+ *
+ * If irq is specified then it it crucial that alert_edge_triggered is
+ * properly set.
+ */
+struct i2c_smbus_alert_setup {
+	unsigned int		alert_edge_triggered:1;
+	int			irq;
+};
+
+struct i2c_client *i2c_setup_smbus_alert(struct i2c_adapter *adapter,
+					 struct i2c_smbus_alert_setup *setup);
+int i2c_handle_smbus_alert(struct i2c_client *ara);
+
+#endif /* _LINUX_I2C_SMBUS_H */
--- linux-2.6.33-rc7.orig/include/linux/i2c.h	2010-02-12 14:19:47.000000000 +0100
+++ linux-2.6.33-rc7/include/linux/i2c.h	2010-02-12 14:19:51.000000000 +0100
@@ -152,6 +152,13 @@ struct i2c_driver {
 	int (*suspend)(struct i2c_client *, pm_message_t mesg);
 	int (*resume)(struct i2c_client *);
 
+	/* Alert callback, for example for the SMBus alert protocol.
+	 * The format and meaning of the data value depends on the protocol.
+	 * For the SMBus alert protocol, there is a single bit of data passed
+	 * as the alert response's low bit ("event flag").
+	 */
+	void (*alert)(struct i2c_client *, unsigned int data);
+
 	/* a ioctl like command that can be used to perform specific functions
 	 * with the device.
 	 */

-- 
Jean Delvare

Re: [PATCH 1/5] i2c: Add SMBus alert support

[David Brownell]

On Saturday 13 February 2010, Jean Delvare wrote:

> The benefit of this approach is a zero cost for I2C bus segments which
> do not need SMBus alert support. Where David's implementation
> increased the size of struct i2c_adapter by 7% (40 bytes on i386),
> mine doesn't touch it. Where David's implementation added over 150
> lines of code to i2c-core (+10%), mine doesn't touch it. The only
> change that touches all the users of the i2c subsystem is a new
> callback in struct i2c_driver (common to both implementations.) I seem
> to remember Trent was worried about the footprint of David'd
> implementation, hopefully mine addresses the issue.

I'm not sure I could justify making I2C and SMBus differ in *ONLY* this
particular way, since otherwise they're treated identically.  I certainly
wouldn't worry about 40 bytes in a Linux kernel ... that's noise.  (If
this were inside a microcontroller that only had a few KB of SRAM, then
I'd certainly worry!)

But that doesn't much matter.  If SMBALERT# support is now going to exist,
that's enough for me.


> + *
> + * Copyright (C) 2010 Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> + *
> + * SMBus alert support based on earlier work by David Brownell (2008).
> + *

That should be "Copyright (C) 2008 David Brownell" ... not
just "based on", since significant chunks are the same code.


> +       struct i2c_client       *ara;

Worth spelling out what "ARA" means; it shouldn't be
just another mysterious TLA.


> +struct alert_data {
> +       unsigned short          addr;
> +       u8                      flag:1;
> +};
> +

Better to make "flag" be "bool" ... there's no space saving
in the struct to have it be a one bit field, and in terms of
code generation it *costs more* than using a "bool".


> +/* If this is the alerting device, notify its driver */
> +static int smbus_do_alert(struct device *dev, void *addrp)
> +{
> +       struct i2c_client *client = i2c_verify_client(dev);
> +       struct alert_data *data = addrp;

Surely the callback should take a "struct alert_data *" then??


> +static irqreturn_t smbalert_irq(int irq, void *d)
> +{
> +       struct i2c_smbus_alert *data = d;
> +
> +       /* Disable level-triggered IRQs until we handle them */
> +       if (!data->alert_edge_triggered)
> +               disable_irq_nosync(irq);
> +
> +       schedule_work(&data->alert);
> +       return IRQ_HANDLED;
> +}
> +

Now that genirq handles threaded IRQs, should this code be updated
to use that infrastructure instead of a work struct?  There are
several mechanisms to kick in here.  It'd be fair to have a way for
IRQ threads to issue i2c_handle_smbus_alert() -- or maybe some
sibling method -- especially if i2c_setup_smbus_alert() causes
this code to provide the hardirq handler.

By the way ... you probably know that the I2C/SMBus controller
in most of Intel's Southbridge chips (like ICH8) supports
the SMBALERT# mechanism.  Testing may be awkward, but it'd be
good to verify this incarnation of SMBALERT# support can work
with those controllers.  (Where "alert" is just another cause
for an IRQ from i2c-i801.c or whatever, not a dedicated IRQ
from a GPIO or from the parport.)

- Dave

p.s. for the record ... I'd try testing this, but the board I
     have which needs that support doesn't have current-enough
     Linux to do so.

Re: [PATCH 1/5] i2c: Add SMBus alert support

[Jean Delvare]

Hi David,

On Sat, 13 Feb 2010 18:06:07 -0800, David Brownell wrote:
> On Saturday 13 February 2010, Jean Delvare wrote:
> 
> > The benefit of this approach is a zero cost for I2C bus segments which
> > do not need SMBus alert support. Where David's implementation
> > increased the size of struct i2c_adapter by 7% (40 bytes on i386),
> > mine doesn't touch it. Where David's implementation added over 150
> > lines of code to i2c-core (+10%), mine doesn't touch it. The only
> > change that touches all the users of the i2c subsystem is a new
> > callback in struct i2c_driver (common to both implementations.) I seem
> > to remember Trent was worried about the footprint of David'd
> > implementation, hopefully mine addresses the issue.
> 
> I'm not sure I could justify making I2C and SMBus differ in *ONLY* this
> particular way, since otherwise they're treated identically.

I'm not sure what you call "making I2C and SMBus differ". My
implementation simply makes it possible to exclude the extra code if
you don't need it. It doesn't prevent I2C controller drivers from
relying on that code if they want to. I named it "i2c-smbus" because I
don't think we want one separate module for every extension provided by
SMBus, and based on the assumption that people who don't want one,
won't want any of them. But if the future makes me wrong, I will be
happy to rename the module to i2c-smbus-alert.

> I certainly
> wouldn't worry about 40 bytes in a Linux kernel ... that's noise.  (If
> this were inside a microcontroller that only had a few KB of SRAM, then
> I'd certainly worry!)

That's 40 bytes * number of I2C segments on the system. There tends to
be more and more I2C segments on systems, and when we add support for
multiplexing (the next big thing on my list) it will only get worse.
BTW, my impression is that multiplexing and SMBus alert are almost
mutually exclusive, so not having all the data in every segment makes a
lot of sense to me.

> 
> But that doesn't much matter.  If SMBALERT# support is now going to exist,
> that's enough for me.

Honestly, I don't worry much either way myself. But I can imagine some
people would, and I didn't want to give them a reason to reject SMBus
alert support.

> > + *
> > + * Copyright (C) 2010 Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> > + *
> > + * SMBus alert support based on earlier work by David Brownell (2008).
> > + *
> 
> That should be "Copyright (C) 2008 David Brownell" ... not
> just "based on", since significant chunks are the same code.

I'm sorry about this. For some reason, I decided that, just because
your code never made it upstream, you did not get to hold a copyright
on it. Silly me. It's fixed now, please accept my humble excuses.

> > +       struct i2c_client       *ara;
> 
> Worth spelling out what "ARA" means; it shouldn't be
> just another mysterious TLA.

Good idea, added.

> > +struct alert_data {
> > +       unsigned short          addr;
> > +       u8                      flag:1;
> > +};
> > +
> 
> Better to make "flag" be "bool" ... there's no space saving
> in the struct to have it be a one bit field, and in terms of
> code generation it *costs more* than using a "bool".

No, thanks. Your code used a bool and I voluntarily changed it to a
regular number. My reason for doing this is that bool has semantics
which are absent from the SMBus specifications. This bit of data is
really 0 or 1, it doesn't carry the meaning of "false" or "true".

> > +/* If this is the alerting device, notify its driver */
> > +static int smbus_do_alert(struct device *dev, void *addrp)
> > +{
> > +       struct i2c_client *client = i2c_verify_client(dev);
> > +       struct alert_data *data = addrp;
> 
> Surely the callback should take a "struct alert_data *" then??

smbus_do_alert() is called by device_for_each_child(), we don't get to
change its prototype.

> > +static irqreturn_t smbalert_irq(int irq, void *d)
> > +{
> > +       struct i2c_smbus_alert *data = d;
> > +
> > +       /* Disable level-triggered IRQs until we handle them */
> > +       if (!data->alert_edge_triggered)
> > +               disable_irq_nosync(irq);
> > +
> > +       schedule_work(&data->alert);
> > +       return IRQ_HANDLED;
> > +}
> > +
> 
> Now that genirq handles threaded IRQs, should this code be updated
> to use that infrastructure instead of a work struct?  There are
> several mechanisms to kick in here.  It'd be fair to have a way for
> IRQ threads to issue i2c_handle_smbus_alert() -- or maybe some
> sibling method -- especially if i2c_setup_smbus_alert() causes
> this code to provide the hardirq handler.

Honestly, this is all beyond me. I suggest that anyone needing this
work on it on his/her own and submits an incremental patch when done. I
have already spent more time than I wanted on this, I can't afford
spending more, especially for a feature I don't need myself.

> By the way ... you probably know that the I2C/SMBus controller
> in most of Intel's Southbridge chips (like ICH8) supports
> the SMBALERT# mechanism.  Testing may be awkward, but it'd be
> good to verify this incarnation of SMBALERT# support can work
> with those controllers.  (Where "alert" is just another cause
> for an IRQ from i2c-i801.c or whatever, not a dedicated IRQ
> from a GPIO or from the parport.)

I have only one system with an ICH and an SMBus device with support for
alerts. It is not currently available for testing... when it becomes
available, I may take a look. That being said, the main obstacle I see
is that the i2c-i801 driver doesn't make use of interrupts at all at
the moment. I don't know if it is possible to enable them to get the
alerts but to not make use of them during regular transactions. The
various attempts to make the i2c-i801 driver use interrupts never made
it to mainline, there was always an least one issue remaining
preventing it. I would love if we finally managed to switch to full
interrupt support.

That being said, my code is based on yours, and I see to remember you
said yours was compatible with the ICH expectations, so I'd expect mine
to work too.

> p.s. for the record ... I'd try testing this, but the board I
>      have which needs that support doesn't have current-enough
>      Linux to do so.

Thank you! And thanks for the review as well, very much appreciated.

-- 
Jean Delvare

Re: [PATCH 1/5] i2c: Add SMBus alert support

[David Brownell]

On Sunday 14 February 2010, Jean Delvare wrote:
> > to use that infrastructure instead of a work struct?  There are
> > several mechanisms to kick in here.  It'd be fair to have a way for
> > IRQ threads to issue i2c_handle_smbus_alert() -- or maybe some
> > sibling method -- especially if i2c_setup_smbus_alert() causes
> > this code to provide the hardirq handler.
> 
> Honestly, this is all beyond me. I suggest that anyone needing this
> work on it on his/her own and submits an incremental patch when done. I
> have already spent more time than I wanted on this, I can't afford
> spending more, especially for a feature I don't need myself.

Then I'd suggest sticking in a REVISIT comment to that effect,
to help armor this patch against repeats of that feedback which
don't accompany such an incremental patch.  :)


> > By the way ... you probably know that the I2C/SMBus controller
> > in most of Intel's Southbridge chips (like ICH8) supports
> > the SMBALERT# mechanism.  Testing may be awkward, but it'd be
> > good to verify this incarnation of SMBALERT# support can work
> > with those controllers.  (Where "alert" is just another cause
> > for an IRQ from i2c-i801.c or whatever, not a dedicated IRQ
> > from a GPIO or from the parport.)
> 
> I have only one system with an ICH and an SMBus device with support for
> alerts. It is not currently available for testing... when it becomes
> available, I may take a look. That being said, the main obstacle I see
> is that the i2c-i801 driver doesn't make use of interrupts at all at
> the moment. I don't know if it is possible to enable them to get the
> alerts but to not make use of them during regular transactions. The
> various attempts to make the i2c-i801 driver use interrupts never made
> it to mainline, there was always an least one issue remaining
> preventing it. I would love if we finally managed to switch to full
> interrupt support.

Ah, I recall some of that mess, now that you mention it.  Ouch!

(By the way ... glad to see you did the nice thing with parport I2C
and SMALERT#.  I've not seen much Linux code using the parport IRQ,
and if nothing else it's nice to have some verification that it has
not bit-rotted to death!)


> That being said, my code is based on yours, and I see to remember you
> said yours was compatible with the ICH expectations, so I'd expect mine
> to work too.

The compatibility was because the driver could say "I got an SMBALERT IRQ"
to the infrastructure from its hardirq handler.  I think the comments in
your version touched on that point.

- Dave

Re: [PATCH 1/5] i2c: Add SMBus alert support

[Jean Delvare]

On Sun, 14 Feb 2010 10:05:53 -0800, David Brownell wrote:
> On Sunday 14 February 2010, Jean Delvare wrote:
> > > to use that infrastructure instead of a work struct?  There are
> > > several mechanisms to kick in here.  It'd be fair to have a way for
> > > IRQ threads to issue i2c_handle_smbus_alert() -- or maybe some
> > > sibling method -- especially if i2c_setup_smbus_alert() causes
> > > this code to provide the hardirq handler.
> > 
> > Honestly, this is all beyond me. I suggest that anyone needing this
> > work on it on his/her own and submits an incremental patch when done. I
> > have already spent more time than I wanted on this, I can't afford
> > spending more, especially for a feature I don't need myself.
> 
> Then I'd suggest sticking in a REVISIT comment to that effect,
> to help armor this patch against repeats of that feedback which
> don't accompany such an incremental patch.  :)

I would wholeheartedly accept a patch adding such a comment ;)
Seriously, I wouldn't know what to write.

> > > By the way ... you probably know that the I2C/SMBus controller
> > > in most of Intel's Southbridge chips (like ICH8) supports
> > > the SMBALERT# mechanism.  Testing may be awkward, but it'd be
> > > good to verify this incarnation of SMBALERT# support can work
> > > with those controllers.  (Where "alert" is just another cause
> > > for an IRQ from i2c-i801.c or whatever, not a dedicated IRQ
> > > from a GPIO or from the parport.)
> > 
> > I have only one system with an ICH and an SMBus device with support for
> > alerts. It is not currently available for testing... when it becomes
> > available, I may take a look. That being said, the main obstacle I see
> > is that the i2c-i801 driver doesn't make use of interrupts at all at
> > the moment. I don't know if it is possible to enable them to get the
> > alerts but to not make use of them during regular transactions. The
> > various attempts to make the i2c-i801 driver use interrupts never made
> > it to mainline, there was always an least one issue remaining
> > preventing it. I would love if we finally managed to switch to full
> > interrupt support.
> 
> Ah, I recall some of that mess, now that you mention it.  Ouch!
> 
> (By the way ... glad to see you did the nice thing with parport I2C
> and SMALERT#.  I've not seen much Linux code using the parport IRQ,
> and if nothing else it's nice to have some verification that it has
> not bit-rotted to death!)

I'm not so sure. You'll notice I had to call parport_disable_irq()
before registering my device. Without it, my IRQ handler would be
called before being ready. This is odd given that the comments in the
parport subsystem code say that you have to call parport_enable_irq()
if you want to receive interrupts (which seems logical). So there's
certainly something slightly broken down there. I don't really have the
time to investigate though :(

> > That being said, my code is based on yours, and I see to remember you
> > said yours was compatible with the ICH expectations, so I'd expect mine
> > to work too.
> 
> The compatibility was because the driver could say "I got an SMBALERT IRQ"
> to the infrastructure from its hardirq handler.  I think the comments in
> your version touched on that point.

-- 
Jean Delvare

Re: [PATCH 1/5] i2c: Add SMBus alert support

[Jonathan Cameron]

Hi Jean,

I have a couple of parts I can test this on (connected to a pxa271)
but it may be a little while before I get to it (so don't let me
hold the patch up!)

On tiny point below.  Worth changing if you are doing another roll of the patch
as at least in my dozy Monday evening state it confused me for a few moments!

Thanks,

Jonathan

Acked-by: Jonathan Cameron <jic23-KWPb1pKIrIJaa/9Udqfwiw@public.gmane.org>
> SMBus alert support. The SMBus alert protocol allows several SMBus
> slave devices to share a single interrupt pin on the SMBus master,
> while still allowing the master to know which slave triggered the
> interrupt.
> 
> This is based on preliminary work by David Brownell. The key
> difference between David's implementation and mine is that his was
> part of i2c-core, while mine is split into a separate, standalone
> module named i2c-smbus. The i2c-smbus module is meant to include
> support for all SMBus extensions to the I2C protocol in the future.
> 
> The benefit of this approach is a zero cost for I2C bus segments which
> do not need SMBus alert support. Where David's implementation
> increased the size of struct i2c_adapter by 7% (40 bytes on i386),
> mine doesn't touch it. Where David's implementation added over 150
> lines of code to i2c-core (+10%), mine doesn't touch it. The only
> change that touches all the users of the i2c subsystem is a new
> callback in struct i2c_driver (common to both implementations.) I seem
> to remember Trent was worried about the footprint of David'd
> implementation, hopefully mine addresses the issue.
> 
> Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> Cc: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
> Cc: Trent Piepho <tpiepho-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> ---
>  Documentation/i2c/smbus-protocol |   16 ++
>  drivers/i2c/Makefile             |    2 
>  drivers/i2c/i2c-smbus.c          |  264 ++++++++++++++++++++++++++++++++++++++
>  include/linux/i2c-smbus.h        |   50 +++++++
>  include/linux/i2c.h              |    7 +
>  5 files changed, 338 insertions(+), 1 deletion(-)
> 
> --- linux-2.6.33-rc7.orig/Documentation/i2c/smbus-protocol	2010-02-12 14:19:47.000000000 +0100
> +++ linux-2.6.33-rc7/Documentation/i2c/smbus-protocol	2010-02-12 14:19:51.000000000 +0100
> @@ -185,6 +185,22 @@ the protocol. All ARP communications use
>  require PEC checksums.
>  
>  
> +SMBus Alert
> +===========
> +
> +SMBus Alert was introduced in Revision 1.0 of the specification.
> +
> +The SMBus alert protocol allows several SMBus slave devices to share a
> +single interrupt pin on the SMBus master, while still allowing the master
> +to know which slave triggered the interrupt.
> +
> +This is implemented the following way in the Linux kernel:
> +* I2C bus drivers which support SMBus alert should call
> +  i2c_setup_smbus_alert() to setup SMBus alert support.
> +* I2C drivers for devices which can trigger SMBus alerts should implement
> +  the optional alert() callback.
> +
> +
>  I2C Block Transactions
>  ======================
>  
> --- linux-2.6.33-rc7.orig/drivers/i2c/Makefile	2010-02-12 14:19:47.000000000 +0100
> +++ linux-2.6.33-rc7/drivers/i2c/Makefile	2010-02-12 16:08:34.000000000 +0100
> @@ -3,7 +3,7 @@
>  #
>  
>  obj-$(CONFIG_I2C_BOARDINFO)	+= i2c-boardinfo.o
> -obj-$(CONFIG_I2C)		+= i2c-core.o
> +obj-$(CONFIG_I2C)		+= i2c-core.o i2c-smbus.o
>  obj-$(CONFIG_I2C_CHARDEV)	+= i2c-dev.o
>  obj-y				+= busses/ chips/ algos/
>  
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6.33-rc7/drivers/i2c/i2c-smbus.c	2010-02-12 16:11:34.000000000 +0100
...
> +/*
> + * 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_smbus_alert *data;
> +	struct i2c_client *ara;
> +	unsigned short prev_addr = 0;	/* Not a valid address */
> +
> +	data = container_of(work, struct i2c_smbus_alert, alert);
> +	ara = data->ara;
> +
> +	for (;;) {
> +		s32 status;
> +		struct alert_data data;
Can we change the name of data here.  From readability point of view it
would be better not to have this reliance on scope (as data used for 
struct i2c_smbus_alert *data above. (obviously changing it above would 
work as well!) 
> +
> +		/*
> +		 * 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(ara);
> +		if (status < 0)
> +			break;
> +
> +		data.flag = status & 1;
> +		data.addr = status >> 1;
> +
> +		if (data.addr == prev_addr) {
> +			dev_warn(&ara->dev, "Duplicate SMBALERT# from dev "
> +				"0x%02x, skipping\n", data.addr);
> +			break;
> +		}
> +		dev_dbg(&ara->dev, "SMBALERT# from dev 0x%02x, flag %d\n",
> +			data.addr, data.flag);
> +
> +		/* Notify driver for the device which issued the alert */
> +		device_for_each_child(&ara->adapter->dev, &data,
> +				      smbus_do_alert);
> +		prev_addr = data.addr;
> +	}
> +
> +	/* We handled all alerts; re-enable level-triggered IRQs */
> +	if (!data->alert_edge_triggered)
> +		enable_irq(data->irq);
> +}
> 

Re: [PATCH 1/5] i2c: Add SMBus alert support

[Jean Delvare]

Hi Jonathan,

On Mon, 15 Feb 2010 18:26:20 +0000, Jonathan Cameron wrote:
> I have a couple of parts I can test this on (connected to a pxa271)
> but it may be a little while before I get to it (so don't let me
> hold the patch up!)

That would be great. You can always get the latest version of the patch
either in my i2c tree or in linux-next.

> On tiny point below.  Worth changing if you are doing another roll of the patch
> as at least in my dozy Monday evening state it confused me for a few moments!
> ...
> > --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> > +++ linux-2.6.33-rc7/drivers/i2c/i2c-smbus.c	2010-02-12 16:11:34.000000000 +0100
> ...
> > +/*
> > + * 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_smbus_alert *data;
> > +	struct i2c_client *ara;
> > +	unsigned short prev_addr = 0;	/* Not a valid address */
> > +
> > +	data = container_of(work, struct i2c_smbus_alert, alert);
> > +	ara = data->ara;
> > +
> > +	for (;;) {
> > +		s32 status;
> > +		struct alert_data data;
> Can we change the name of data here.  From readability point of view it
> would be better not to have this reliance on scope (as data used for 
> struct i2c_smbus_alert *data above. (obviously changing it above would 
> work as well!) 

Oh, yeah, of course. Thanks for pointing this out. I wish we built the
kernel with -Wshadow so that gcc could tell us automatically...

I have changed all instances of struct i2c_smbus_alert * to name
"alert", this solves the problem and should make the code more
consistent and readable.

Thanks!
-- 
Jean Delvare

Re: [PATCH 1/5] i2c: Add SMBus alert support

[Mark Brown]

On Tue, Feb 16, 2010 at 10:56:06AM +0100, Jean Delvare wrote:
> On Mon, 15 Feb 2010 18:26:20 +0000, Jonathan Cameron wrote:

> > Can we change the name of data here.  From readability point of view it
> > would be better not to have this reliance on scope (as data used for 
> > struct i2c_smbus_alert *data above. (obviously changing it above would 
> > work as well!) 

> Oh, yeah, of course. Thanks for pointing this out. I wish we built the
> kernel with -Wshadow so that gcc could tell us automatically...

If you build with sparse (install it then add C=1 to the command line)
it'll pick up shadowing along with all the other stuff.

[PATCH 2/5] i2c: Separate Kconfig option for i2c-smbus

[Jean Delvare]

Having a separate Kconfig option for i2c-smbus makes it possible to
build that support as a module even when i2c-core itself is built-in.
Bus drivers which implement SMBus alert should select this option, so
in most cases this option is hidden and the user doesn't have to care
about it.

Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
Cc: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
Cc: Trent Piepho <tpiepho-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
---
 drivers/i2c/Kconfig  |   10 ++++++++++
 drivers/i2c/Makefile |    3 ++-
 2 files changed, 12 insertions(+), 1 deletion(-)

--- linux-2.6.33-rc7.orig/drivers/i2c/Kconfig	2010-02-12 14:20:30.000000000 +0100
+++ linux-2.6.33-rc7/drivers/i2c/Kconfig	2010-02-12 14:20:35.000000000 +0100
@@ -61,6 +61,16 @@ config I2C_HELPER_AUTO
 
 	  In doubt, say Y.
 
+config I2C_SMBUS
+	tristate "SMBus-specific protocols" if !I2C_HELPER_AUTO
+	help
+	  Say Y here if you want support for SMBus extensions to the I2C
+	  specification. At the moment, the only supported extension is
+	  the SMBus alert protocol.
+
+	  This support is also available as a module.  If so, the module
+	  will be called i2c-smbus.
+
 source drivers/i2c/algos/Kconfig
 source drivers/i2c/busses/Kconfig
 source drivers/i2c/chips/Kconfig
--- linux-2.6.33-rc7.orig/drivers/i2c/Makefile	2010-02-12 14:20:30.000000000 +0100
+++ linux-2.6.33-rc7/drivers/i2c/Makefile	2010-02-12 14:20:35.000000000 +0100
@@ -3,7 +3,8 @@
 #
 
 obj-$(CONFIG_I2C_BOARDINFO)	+= i2c-boardinfo.o
-obj-$(CONFIG_I2C)		+= i2c-core.o i2c-smbus.o
+obj-$(CONFIG_I2C)		+= i2c-core.o
+obj-$(CONFIG_I2C_SMBUS)		+= i2c-smbus.o
 obj-$(CONFIG_I2C_CHARDEV)	+= i2c-dev.o
 obj-y				+= busses/ chips/ algos/
 

-- 
Jean Delvare

[PATCH 3/5] i2c-parport: Add SMBus alert support

[Jean Delvare]

Add support for the SMBus alert mechanism to the i2c-parport driver.
The ADM1032 evaluation board at least is properly wired for this.

Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
Cc: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
Cc: Trent Piepho <tpiepho-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
---
 Documentation/i2c/busses/i2c-parport |    3 ++
 drivers/i2c/busses/Kconfig           |    1 
 drivers/i2c/busses/i2c-parport.c     |   37 ++++++++++++++++++++++++++++++++--
 drivers/i2c/busses/i2c-parport.h     |    4 ++-
 4 files changed, 42 insertions(+), 3 deletions(-)

--- linux-2.6.33-rc7.orig/Documentation/i2c/busses/i2c-parport	2010-02-12 14:20:30.000000000 +0100
+++ linux-2.6.33-rc7/Documentation/i2c/busses/i2c-parport	2010-02-12 14:20:39.000000000 +0100
@@ -29,6 +29,9 @@ can be easily added when needed.
 Earlier kernels defaulted to type=0 (Philips).  But now, if the type
 parameter is missing, the driver will simply fail to initialize.
 
+SMBus alert support is available on adapters which have this line properly
+connected to the parallel port's interrupt pin.
+
 
 Building your own adapter
 -------------------------
--- linux-2.6.33-rc7.orig/drivers/i2c/busses/Kconfig	2010-02-12 14:20:30.000000000 +0100
+++ linux-2.6.33-rc7/drivers/i2c/busses/Kconfig	2010-02-12 14:20:39.000000000 +0100
@@ -571,6 +571,7 @@ config I2C_PARPORT
 	tristate "Parallel port adapter"
 	depends on PARPORT
 	select I2C_ALGOBIT
+	select I2C_SMBUS
 	help
 	  This supports parallel port I2C adapters such as the ones made by
 	  Philips or Velleman, Analog Devices evaluation boards, and more.
--- linux-2.6.33-rc7.orig/drivers/i2c/busses/i2c-parport.c	2010-02-12 14:20:30.000000000 +0100
+++ linux-2.6.33-rc7/drivers/i2c/busses/i2c-parport.c	2010-02-12 14:20:39.000000000 +0100
@@ -1,7 +1,7 @@
 /* ------------------------------------------------------------------------ *
  * i2c-parport.c I2C bus over parallel port                                 *
  * ------------------------------------------------------------------------ *
-   Copyright (C) 2003-2007 Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
+   Copyright (C) 2003-2010 Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
    
    Based on older i2c-philips-par.c driver
    Copyright (C) 1995-2000 Simon G. Vogl
@@ -31,6 +31,7 @@
 #include <linux/parport.h>
 #include <linux/i2c.h>
 #include <linux/i2c-algo-bit.h>
+#include <linux/i2c-smbus.h>
 #include "i2c-parport.h"
 
 /* ----- Device list ------------------------------------------------------ */
@@ -39,6 +40,8 @@ struct i2c_par {
 	struct pardevice *pdev;
 	struct i2c_adapter adapter;
 	struct i2c_algo_bit_data algo_data;
+	struct i2c_smbus_alert_setup alert_data;
+	struct i2c_client *ara;
 	struct i2c_par *next;
 };
 
@@ -144,6 +147,19 @@ static struct i2c_algo_bit_data parport_
 
 /* ----- I2c and parallel port call-back functions and structures --------- */
 
+void i2c_parport_irq(void *data)
+{
+	struct i2c_par *adapter = data;
+	struct i2c_client *ara = adapter->ara;
+
+	if (ara) {
+		dev_dbg(&ara->dev, "SMBus alert received\n");
+		i2c_handle_smbus_alert(ara);
+	} else
+		dev_dbg(&adapter->adapter.dev,
+			"SMBus alert received but no ARA client!\n");
+}
+
 static void i2c_parport_attach (struct parport *port)
 {
 	struct i2c_par *adapter;
@@ -155,8 +171,9 @@ static void i2c_parport_attach (struct p
 	}
 
 	pr_debug("i2c-parport: attaching to %s\n", port->name);
+	parport_disable_irq(port);
 	adapter->pdev = parport_register_device(port, "i2c-parport",
-		NULL, NULL, NULL, PARPORT_FLAG_EXCL, NULL);
+		NULL, NULL, i2c_parport_irq, PARPORT_FLAG_EXCL, adapter);
 	if (!adapter->pdev) {
 		printk(KERN_ERR "i2c-parport: Unable to register with parport\n");
 		goto ERROR0;
@@ -197,6 +214,18 @@ static void i2c_parport_attach (struct p
 		goto ERROR1;
 	}
 
+	/* Setup SMBus alert if supported */
+	if (adapter_parm[type].smbus_alert) {
+		adapter->alert_data.alert_edge_triggered = 1;
+		adapter->ara = i2c_setup_smbus_alert(&adapter->adapter,
+						     &adapter->alert_data);
+		if (adapter->ara)
+			parport_enable_irq(port);
+		else
+			printk(KERN_WARNING "i2c-parport: Failed to register "
+			       "ARA client\n");
+	}
+
 	/* Add the new adapter to the list */
 	adapter->next = adapter_list;
 	adapter_list = adapter;
@@ -217,6 +246,10 @@ static void i2c_parport_detach (struct p
 	for (prev = NULL, adapter = adapter_list; adapter;
 	     prev = adapter, adapter = adapter->next) {
 		if (adapter->pdev->port == port) {
+			if (adapter->ara) {
+				parport_disable_irq(port);
+				i2c_unregister_device(adapter->ara);
+			}
 			i2c_del_adapter(&adapter->adapter);
 
 			/* Un-init if needed (power off...) */
--- linux-2.6.33-rc7.orig/drivers/i2c/busses/i2c-parport.h	2010-02-12 14:20:30.000000000 +0100
+++ linux-2.6.33-rc7/drivers/i2c/busses/i2c-parport.h	2010-02-12 14:20:39.000000000 +0100
@@ -1,7 +1,7 @@
 /* ------------------------------------------------------------------------ *
  * i2c-parport.h I2C bus over parallel port                                 *
  * ------------------------------------------------------------------------ *
-   Copyright (C) 2003-2004 Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
+   Copyright (C) 2003-2010 Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
    
    This program is free software; you can redistribute it and/or modify
    it under the terms of the GNU General Public License as published by
@@ -38,6 +38,7 @@ struct adapter_parm {
 	struct lineop getsda;
 	struct lineop getscl;
 	struct lineop init;
+	unsigned int smbus_alert:1;
 };
 
 static struct adapter_parm adapter_parm[] = {
@@ -73,6 +74,7 @@ static struct adapter_parm adapter_parm[
 		.setscl	= { 0x01, DATA, 1 },
 		.getsda	= { 0x10, STAT, 1 },
 		.init	= { 0xf0, DATA, 0 },
+		.smbus_alert = 1,
 	},
 	/* type 5: ADM1025, ADM1030 and ADM1031 evaluation boards */
 	{

-- 
Jean Delvare

[PATCH 4/5] i2c-parport-light: Add SMBus alert support

[Jean Delvare]

Add support for the SMBus alert mechanism to the i2c-parport-light
driver. The ADM1032 evaluation board at least is properly wired for
this.

Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
Cc: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
Cc: Trent Piepho <tpiepho-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
---
 Documentation/i2c/busses/i2c-parport-light |   11 +++++++
 drivers/i2c/busses/Kconfig                 |    1 
 drivers/i2c/busses/i2c-parport-light.c     |   42 ++++++++++++++++++++++++++--
 3 files changed, 51 insertions(+), 3 deletions(-)

--- linux-2.6.33-rc8.orig/Documentation/i2c/busses/i2c-parport-light	2010-02-13 22:31:35.000000000 +0100
+++ linux-2.6.33-rc8/Documentation/i2c/busses/i2c-parport-light	2010-02-13 22:34:52.000000000 +0100
@@ -9,3 +9,14 @@ parport handling is not an option. The d
 and the impossibility to daisy-chain other parallel port devices.                 
   
 Please see i2c-parport for documentation.
+
+Module parameters:
+
+* type: type of adapter (see i2c-parport or modinfo)
+
+* base: base I/O address
+  Default is 0x378 which is fairly common for parallel ports, at least on PC.
+
+* irq: optional IRQ
+  This must be passed if you want SMBus alert support, assuming your adapter
+  actually supports this.
--- linux-2.6.33-rc8.orig/drivers/i2c/busses/Kconfig	2010-02-13 22:34:09.000000000 +0100
+++ linux-2.6.33-rc8/drivers/i2c/busses/Kconfig	2010-02-13 22:34:52.000000000 +0100
@@ -595,6 +595,7 @@ config I2C_PARPORT
 config I2C_PARPORT_LIGHT
 	tristate "Parallel port adapter (light)"
 	select I2C_ALGOBIT
+	select I2C_SMBUS
 	help
 	  This supports parallel port I2C adapters such as the ones made by
 	  Philips or Velleman, Analog Devices evaluation boards, and more.
--- linux-2.6.33-rc8.orig/drivers/i2c/busses/i2c-parport-light.c	2010-02-13 22:31:36.000000000 +0100
+++ linux-2.6.33-rc8/drivers/i2c/busses/i2c-parport-light.c	2010-02-13 22:34:52.000000000 +0100
@@ -1,7 +1,7 @@
 /* ------------------------------------------------------------------------ *
  * i2c-parport-light.c I2C bus over parallel port                           *
  * ------------------------------------------------------------------------ *
-   Copyright (C) 2003-2007 Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
+   Copyright (C) 2003-2010 Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
    
    Based on older i2c-velleman.c driver
    Copyright (C) 1995-2000 Simon G. Vogl
@@ -32,6 +32,7 @@
 #include <linux/ioport.h>
 #include <linux/i2c.h>
 #include <linux/i2c-algo-bit.h>
+#include <linux/i2c-smbus.h>
 #include <asm/io.h>
 #include "i2c-parport.h"
 
@@ -44,6 +45,10 @@ static u16 base;
 module_param(base, ushort, 0);
 MODULE_PARM_DESC(base, "Base I/O address");
 
+static int irq;
+module_param(irq, int, 0);
+MODULE_PARM_DESC(irq, "IRQ (optional)");
+
 /* ----- Low-level parallel port access ----------------------------------- */
 
 static inline void port_write(unsigned char p, unsigned char d)
@@ -120,6 +125,16 @@ static struct i2c_adapter parport_adapte
 	.name		= "Parallel port adapter (light)",
 };
 
+/* SMBus alert support */
+static struct i2c_smbus_alert_setup alert_data = {
+	.alert_edge_triggered	= 1,
+};
+static struct i2c_client *ara;
+static struct lineop parport_ctrl_irq = {
+	.val		= (1 << 4),
+	.port		= CTRL,
+};
+
 static int __devinit i2c_parport_probe(struct platform_device *pdev)
 {
 	int err;
@@ -136,13 +151,31 @@ static int __devinit i2c_parport_probe(s
 
 	parport_adapter.dev.parent = &pdev->dev;
 	err = i2c_bit_add_bus(&parport_adapter);
-	if (err)
+	if (err) {
 		dev_err(&pdev->dev, "Unable to register with I2C\n");
-	return err;
+		return err;
+	}
+
+	/* Setup SMBus alert if supported */
+	if (adapter_parm[type].smbus_alert && irq) {
+		alert_data.irq = irq;
+		ara = i2c_setup_smbus_alert(&parport_adapter, &alert_data);
+		if (ara)
+			line_set(1, &parport_ctrl_irq);
+		else
+			dev_warn(&pdev->dev, "Failed to register ARA client\n");
+	}
+
+	return 0;
 }
 
 static int __devexit i2c_parport_remove(struct platform_device *pdev)
 {
+	if (ara) {
+		line_set(0, &parport_ctrl_irq);
+		i2c_unregister_device(ara);
+		ara = NULL;
+	}
 	i2c_del_adapter(&parport_adapter);
 
 	/* Un-init if needed (power off...) */
@@ -209,6 +242,9 @@ static int __init i2c_parport_init(void)
 	if (!request_region(base, 3, DRVNAME))
 		return -EBUSY;
 
+	if (irq != 0)
+		pr_info(DRVNAME ": using irq %d\n", irq);
+
         if (!adapter_parm[type].getscl.val)
 		parport_algo_data.getscl = NULL;
 

-- 
Jean Delvare

[PATCH 5/5] hwmon: (lm90) Add SMBus alert support

[Jean Delvare]

Tested successfully with an ADM1032 chip on its evaluation board. It
should work fine with all other chips as well.

At this point this is more of a proof-of-concept, we don't do anything
terribly useful on SMBus alert: we simply log the event. But this could
later evolve into libsensors signaling so that user-space applications
can take an appropriate action.

Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
Cc: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
Cc: Trent Piepho <tpiepho-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
---
 Documentation/hwmon/lm90 |   12 ++++++++
 drivers/hwmon/lm90.c     |   63 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 75 insertions(+)

--- linux-2.6.33-rc8.orig/Documentation/hwmon/lm90	2010-02-13 16:24:06.000000000 +0100
+++ linux-2.6.33-rc8/Documentation/hwmon/lm90	2010-02-13 16:24:41.000000000 +0100
@@ -173,6 +173,18 @@ The lm90 driver will not update its valu
 other second; reading them more often will do no harm, but will return
 'old' values.
 
+SMBus Alert Support
+-------------------
+
+This driver has basic support for SMBus alert. When an alert is received,
+the status register is read and the faulty temperature channel is logged.
+
+The Analog Devices chips (ADM1032 and ADT7461) do not implement the SMBus
+alert protocol properly so additional care is needed: the ALERT output is
+disabled when an alert is received, and is re-enabled only when the alarm
+is gone. Otherwise the chip would block alerts from other chips in the bus
+as long as the alarm is active.
+
 PEC Support
 -----------
 
--- linux-2.6.33-rc8.orig/drivers/hwmon/lm90.c	2010-02-13 16:24:17.000000000 +0100
+++ linux-2.6.33-rc8/drivers/hwmon/lm90.c	2010-02-13 15:50:53.000000000 +0100
@@ -152,6 +152,7 @@ static int lm90_detect(struct i2c_client
 static int lm90_probe(struct i2c_client *client,
 		      const struct i2c_device_id *id);
 static void lm90_init_client(struct i2c_client *client);
+static void lm90_alert(struct i2c_client *client, unsigned int flag);
 static int lm90_remove(struct i2c_client *client);
 static struct lm90_data *lm90_update_device(struct device *dev);
 
@@ -186,6 +187,7 @@ static struct i2c_driver lm90_driver = {
 	},
 	.probe		= lm90_probe,
 	.remove		= lm90_remove,
+	.alert		= lm90_alert,
 	.id_table	= lm90_id,
 	.detect		= lm90_detect,
 	.address_list	= normal_i2c,
@@ -204,6 +206,7 @@ struct lm90_data {
 	int flags;
 
 	u8 config_orig;		/* Original configuration register value */
+	u8 alert_alarms;	/* Which alarm bits trigger ALERT# */
 
 	/* registers values */
 	s8 temp8[4];	/* 0: local low limit
@@ -806,6 +809,19 @@ static int lm90_probe(struct i2c_client
 			new_client->flags &= ~I2C_CLIENT_PEC;
 	}
 
+	/* Different devices have different alarm bits triggering the
+	 * ALERT# output */
+	switch (data->kind) {
+	case lm90:
+	case lm99:
+	case lm86:
+		data->alert_alarms = 0x7b;
+		break;
+	default:
+		data->alert_alarms = 0x7c;
+		break;
+	}
+
 	/* Initialize the LM90 chip */
 	lm90_init_client(new_client);
 
@@ -895,6 +911,38 @@ static int lm90_remove(struct i2c_client
 	return 0;
 }
 
+static void lm90_alert(struct i2c_client *client, unsigned int flag)
+{
+	struct lm90_data *data = i2c_get_clientdata(client);
+	u8 config, alarms;
+
+	lm90_read_reg(client, LM90_REG_R_STATUS, &alarms);
+	if ((alarms & 0x7f) == 0) {
+		dev_info(&client->dev, "Everything OK\n");
+	} else {
+		if (alarms & 0x61)
+			dev_warn(&client->dev,
+				 "temp%d out of range, please check!\n", 1);
+		if (alarms & 0x1a)
+			dev_warn(&client->dev,
+				 "temp%d out of range, please check!\n", 2);
+		if (alarms & 0x04)
+			dev_warn(&client->dev,
+				 "temp%d diode open, please check!\n", 2);
+
+		/* Disable ALERT# output, because these chips don't implement
+		  SMBus alert correctly; they should only hold the alert line
+		  low briefly. */
+		if ((data->kind == adm1032 || data->kind == adt7461)
+		 && (alarms & data->alert_alarms)) {
+			dev_dbg(&client->dev, "Disabling ALERT#\n");
+			lm90_read_reg(client, LM90_REG_R_CONFIG1, &config);
+			i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1,
+						  config | 0x80);
+		}
+	}
+}
+
 static int lm90_read16(struct i2c_client *client, u8 regh, u8 regl, u16 *value)
 {
 	int err;
@@ -982,6 +1030,21 @@ static struct lm90_data *lm90_update_dev
 		}
 		lm90_read_reg(client, LM90_REG_R_STATUS, &data->alarms);
 
+		/* Re-enable ALERT# output if it was originally enabled and
+		 * relevant alarms are all clear */
+		if ((data->config_orig & 0x80) == 0
+		 && (data->alarms & data->alert_alarms) == 0) {
+			u8 config;
+
+			lm90_read_reg(client, LM90_REG_R_CONFIG1, &config);
+			if (config & 0x80) {
+				dev_dbg(&client->dev, "Re-enabling ALERT#\n");
+				i2c_smbus_write_byte_data(client,
+							  LM90_REG_W_CONFIG1,
+							  config & ~0x80);
+			}
+		}
+
 		data->last_updated = jiffies;
 		data->valid = 1;
 	}

-- 
Jean Delvare

Re: [PATCH 5/5] hwmon: (lm90) Add SMBus alert support

[David Brownell]

On Saturday 13 February 2010, Jean Delvare wrote:
> At this point this is more of a proof-of-concept, we don't do anything
> terribly useful on SMBus alert: we simply log the event. But this could
> later evolve into libsensors signaling so that user-space applications
> can take an appropriate action.

Yeah, this is a case where it's a *GOOD THING* to let that
particular camel stick its nose into the tent ... ;)

Polling for alerts is kind of sub-optimal, and that's the
best that's been possible until now.

- Dave

Re: [PATCH 5/5] hwmon: (lm90) Add SMBus alert support

[Trent Piepho]

On Sat, 13 Feb 2010, Jean Delvare wrote:
> Tested successfully with an ADM1032 chip on its evaluation board. It
> should work fine with all other chips as well.
>
> At this point this is more of a proof-of-concept, we don't do anything
> terribly useful on SMBus alert: we simply log the event. But this could
> later evolve into libsensors signaling so that user-space applications
> can take an appropriate action.

When I added alert support to the lm63 driver, I had it wake up any process
that's select()ing on the the alarm* files.  I also wrote a hw monitor
daemon that supports this system.  It worked pretty well.  Much more
efficient than polling once per second, and also much faster to resond to
an alert event.

I didn't bother to use ARA, since I know what device is generating the
interrupt.  Does your system support devices with an alert line that don't
use SMBus ARA?

> +static void lm90_alert(struct i2c_client *client, unsigned int flag)
> +{
> +	struct lm90_data *data = i2c_get_clientdata(client);
> +	u8 config, alarms;
> +
> +	lm90_read_reg(client, LM90_REG_R_STATUS, &alarms);

Does lm90_read_reg() update the driver's cached copy of the alarm status
register?  Because if it doesn't, then won't anything reading the alarm
status via sysfs not see the alarms?

What happens if a process reads one of the sysfs attributes after the
interrupt is generated but before the driver runs this code?

In the lm63, the alarm register is clear on read.  So you get the alert,
check alarm status, and think there are no alarms.

What I did for the lm63 driver was assume that if there was an alert
generated, there must have been an alarm bit set.  If there isn't one set
now, then the only way it may have been cleared is if the driver read the
alarm status.  In which case the driver's cache copy of the alarm status
should be used to see what generated the alert.

Another thing I found is that after getting notified of the alert, the most
natural thing for userspace to do is check the temperature/fan/etc
attribute that just alarmed.  "Fan Alert, fan1 speed of 2000 RPM is below
minimum of 300 RPM" Huh?  2000 < 300?  What's going on?  The 2000 RPM is
one second old cached data!  The current speed should cause an alarm, but
the 1 HZ max update rate the driver makes userspace wait for it, somewhat
defeating the purpose of the alert irq.  I have the lm63 driver invalidate
the cached data on alert, so that if userspace reads an attribute it gets
fresh data.

> +		/* Re-enable ALERT# output if it was originally enabled and
> +		 * relevant alarms are all clear */
> +		if ((data->config_orig & 0x80) == 0
> +		 && (data->alarms & data->alert_alarms) == 0) {
> +			u8 config;
> +
> +			lm90_read_reg(client, LM90_REG_R_CONFIG1, &config);
> +			if (config & 0x80) {
> +				dev_dbg(&client->dev, "Re-enabling ALERT#\n");
> +				i2c_smbus_write_byte_data(client,
> +							  LM90_REG_W_CONFIG1,
> +							  config & ~0x80);
> +			}
> +		}

I didn't like the idea of having to have userspace poll attributes to get
alarms re-enabled.  I have the driver start a kernel timer going that
checks the alarm status.  It also notified processes sleeping in the alarm
attributes when the check back to 0.  The time period the kernel polling
can be based on the driver's knowledge of the chip's update rate.  It can
be set to poll faster when there is an alarm.  Of course you rejected my
patch to let the lm63 driver set and know the update rate because that
information could never be useful to anyone...

Re: [PATCH 5/5] hwmon: (lm90) Add SMBus alert support

[Jean Delvare]

Hi Trent,

On Sun, 14 Feb 2010 00:33:53 -0800 (PST), Trent Piepho wrote:
> On Sat, 13 Feb 2010, Jean Delvare wrote:
> > Tested successfully with an ADM1032 chip on its evaluation board. It
> > should work fine with all other chips as well.
> >
> > At this point this is more of a proof-of-concept, we don't do anything
> > terribly useful on SMBus alert: we simply log the event. But this could
> > later evolve into libsensors signaling so that user-space applications
> > can take an appropriate action.
> 
> When I added alert support to the lm63 driver, I had it wake up any process
> that's select()ing on the the alarm* files.  I also wrote a hw monitor
> daemon that supports this system.  It worked pretty well.  Much more
> efficient than polling once per second, and also much faster to resond to
> an alert event.

Why didn't you add support to libsensors instead? This would seem the
right way if we want popular monitoring applications to take benefit of
this new feature.

> I didn't bother to use ARA, since I know what device is generating the
> interrupt.  Does your system support devices with an alert line that don't
> use SMBus ARA?

Not sure what you mean by "my system". My hardware setup is an
evaluation board connecting to the parallel port, it has an ADM1032
chip and that's all. Also, if you don't want to use the SMBus alert
mechanism, there's nothing preventing you from using raw interrupts
right now, you don't need my patches. Every driver can request an IRQ
and make use of it.

> > +static void lm90_alert(struct i2c_client *client, unsigned int flag)
> > +{
> > +	struct lm90_data *data = i2c_get_clientdata(client);
> > +	u8 config, alarms;
> > +
> > +	lm90_read_reg(client, LM90_REG_R_STATUS, &alarms);
> 
> Does lm90_read_reg() update the driver's cached copy of the alarm status
> register?

No, it doesn't.

> Because if it doesn't, then won't anything reading the alarm
> status via sysfs not see the alarms?

If the alarms have worn off meanwhile, yes indeed. But updating the
cache wouldn't really help, because it only lives for one second, so
any application polling less frequently than this would miss the
transient alarm anyway. And systems with more than one polling
application (for example sensord + a desktop applet) can easily miss
transient alarms even without my patch.

I tend to see transient alarms as an optional bonus without any
guarantee. Guaranteeing that transient alarms are always caught would
require a major redesign of all our drivers. And in practice, the
latching of alarm flags tend to confuse users more than it helps them.

> What happens if a process reads one of the sysfs attributes after the
> interrupt is generated but before the driver runs this code?

Good question. I admit that I have not necessarily thought of all the
possible race conditions. If the alarm is still present, it shouldn't
make a difference because the next conversion cycle will set the alarm
flag again. It would only make a difference if the alarm has gone away
meanwhile, and the extra read cleared the alarm bit. In that case, the
interrupt handler has no way to know what cause the problem. It will
log that everything is OK (to the user's surprise I guess) and do
nothing else. I don't see any problem here.

> In the lm63, the alarm register is clear on read.  So you get the alert,
> check alarm status, and think there are no alarms.

Yes, that's the same here. Almost all hwmon chips have alarm registers
which clear on read.

> What I did for the lm63 driver was assume that if there was an alert
> generated, there must have been an alarm bit set.  If there isn't one set
> now, then the only way it may have been cleared is if the driver read the
> alarm status.  In which case the driver's cache copy of the alarm status
> should be used to see what generated the alert.

Hmm, that's an interesting strategy, I admit. Now the problem I have at
least with the ADM1032 is that I also get an interrupt when I re-enable
the ALERT output. When this happens, the lack of alarm bit being set is
totally expected, so I certainly don't want to read the cached value to
display an alert message to the user.

I don't think this is a problem anyway. Either the alarm condition is
gone, meaning it was transient, and the user probably doesn't care. Or
the alarm condition is still there and the next cycle will trigger an
interrupt again, so the user will finally know what's up.

> 
> Another thing I found is that after getting notified of the alert, the most
> natural thing for userspace to do is check the temperature/fan/etc
> attribute that just alarmed.  "Fan Alert, fan1 speed of 2000 RPM is below
> minimum of 300 RPM" Huh?  2000 < 300?  What's going on?  The 2000 RPM is
> one second old cached data!  The current speed should cause an alarm, but
> the 1 HZ max update rate the driver makes userspace wait for it, somewhat
> defeating the purpose of the alert irq.  I have the lm63 driver invalidate
> the cached data on alert, so that if userspace reads an attribute it gets
> fresh data.

This is a good idea, I will probably do the same (even though I am a
little worried about locking... invalidating the cache requires holding
the lock, which may take long if user-space is reading the values at
that time). That being said, it is in no way bullet-proof: by the time
the user-space application reads the attributes, the faulty condition
may have gone away. So it is better to rely on the alarm flags to print
the warning, and only provide the sensor and limit values as hints.

> 
> > +		/* Re-enable ALERT# output if it was originally enabled and
> > +		 * relevant alarms are all clear */
> > +		if ((data->config_orig & 0x80) == 0
> > +		 && (data->alarms & data->alert_alarms) == 0) {
> > +			u8 config;
> > +
> > +			lm90_read_reg(client, LM90_REG_R_CONFIG1, &config);
> > +			if (config & 0x80) {
> > +				dev_dbg(&client->dev, "Re-enabling ALERT#\n");
> > +				i2c_smbus_write_byte_data(client,
> > +							  LM90_REG_W_CONFIG1,
> > +							  config & ~0x80);
> > +			}
> > +		}
> 
> I didn't like the idea of having to have userspace poll attributes to get
> alarms re-enabled.  I have the driver start a kernel timer going that
> checks the alarm status.  It also notified processes sleeping in the alarm
> attributes when the check back to 0.  The time period the kernel polling
> can be based on the driver's knowledge of the chip's update rate.  It can
> be set to poll faster when there is an alarm.

I tried to make my changes integrate into the current usage scheme of
the hwmon drivers (user-space repeatedly polling for values.) And it
was really only a way to test the rest of the SMBus alert code. As long
as it doesn't do anything useful, I do not think it is a problem to
rely on user-space. It can certainly evolve in the future, especially
if we make it possible for the kernel to take thermal management
measures by itself (e.g. processor throttling). But for this, we would
have to somehow unify the thermal zones subsystem with the hwmon one.
This is way beyond the scope of my patch.

> Of course you rejected my
> patch to let the lm63 driver set and know the update rate because that
> information could never be useful to anyone...

Trent, you rejected your patch yourself. We (I wasn't alone) asked you
to split your patch into a part which everybody agreed on and a part
where more discussion was needed. You never followed up, so your
changes never went anywhere. You did it to yourself, really.

-- 
Jean Delvare