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