First THANKS for the quick review Ben! I will address the issues swiftly. Below only the points I need to discuss further (the rest will be fixed). 2009/5/5 Ben Dooks <ben-linux@xxxxxxxxx>: On Tue, 5 May 2009 14:40:02 +0100, Ben Dooks wrote: > On Tue, May 05, 2009 at 01:52:22PM +0200, Linus Walleij wrote: > > --- /dev/null > > +++ b/drivers/i2c/busses/i2c-stddci2c.c > > @@ -0,0 +1,973 @@ > > +/* > > + * > > + * drivers/i2c/busses/i2c-stddci2c.c > > Is it really just for DDC? is there another i2c block in the chip? > if not, can we just call this sti2c or similar? Further Jean writes: > Both names are equally bad. The driver name starts with "i2c-" so there > is no point in adding "i2c" again. Please use i2c-stddc or i2c-st-u300 > or similar. The driver is named like this because the actual name of the HW block *is* "ddci2c" (like my name is "Linus"), I added "st" to make it more unique. It's going to be used in U300 but it is actually used in a Nomadik platform for which a linux port exists, so adding "u300" to the name would be misleading. (Well could be renamed later, but...) Then DDC: this block does *both* I2C and DDC1 and DDC2B. The switch can be made by hardware and software alike. By setting bit 7 resp bit 4 of I2C_CR it will be switched from plain I2C to DDC1 or DDC2 respectively. I didn't add code for it since it cannot be tested on this reference hardware, but if you like I can try to add that in anyway. Would be a module parameter to select mode plain I2C/DDC1/DDC2. Would that be nice? >> +static int __init >> +stddci2c_probe(struct platform_device *pdev) > > __devinit, iirc. > (...) >> +static int __exit > > __devexit, iirc. > (...) >> + .remove = __exit_p(stddci2c_remove), > > thought __devexit_p()? So did I until I tried to merge the RTC driver. >From http://groups.google.com/group/rtc-linux/web/checklist * use __devinit/__devexit/__devexit_p for hotpluggable devices * use __init/__exit/__exit_p otherwise This is also stated in a comment in include/linux/init.h: /* Used for HOTPLUG */ #define __devinit __section(.devinit.text) __cold #define __devinitdata __section(.devinit.data) (etc) Thinking about it, no platform_device should ever have __dev{init|exit} in it regarding the definition of a platform device as something being built in. I think there are a lot of sinners in the kernel regarding this though, including a driver fiddled with myself recently :-/ >> + /* DDC class but actually often used for more generic I2C */ >> + adap->class = I2C_CLASS_DDC; > > I thoguht jdelvare was trying to get rid of this? Hm, would be good since especially a device like this doesn't shoehorn very well into that schema doing both plain I2C and DDC. I would have to assign different values depending on module parameter if I add explicit DDC support. Shall I just leave this field unassigned? Linus Walleij -- 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
