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

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

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