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 <[EMAIL PROTECTED]>
>
> 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 <[EMAIL PROTECTED]>
> ---
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
