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