On Wednesday 19 November 2008, Jean Delvare wrote:
> > +/*
> > + * 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".
If you rely on "true == 1", which I always like to avoid.
I'll change that for you though, and make "flag" a u8.
> > + 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.
For bits, "1" == "high" == "true" is the normal convention.
But I changed it to "u8" and treat it as a number.
> > +
> > + /* 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);
> > +}
> 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.
Done. :)
> > @@ -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.
Easiest for i2c_client, which already has kerneldoc...
> 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.
OK, I'll work on that. It won't be done this week though.
Key point: three models for smbalert# configuration.
(a) none, don't set those new values
(b) adapter handles it, just set do_setup_alert flag and then
schedule_work(&adapter->alert) as needed
(c) i2c-core handles it, set irq (and maybe alert_edge_triggered)
So (b) would be a missing example.
> 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.
Parport IRQs ... I almost started to muck with those at one point.
Right now only one of my systems even has a parport. ;)
- Dave
--
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
