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

[Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>]

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