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

[David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>]

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.