On Friday 28 November 2008 11:16:20 Trilok Soni wrote: > -- > ---Trilok Soni > http://triloksoni.wordpress.com > http://www.linkedin.com/in/triloksoni Hi Trilok, I reviewed this sensor driver and it's fine except for one thing: setting the default registers from outside the driver. This is a really bad idea. I2C drivers should be self-contained. I've made the same comment in the tvp514x driver review which I'm copying below (with some small edits): This driver relies on an outside source to do the initialization of the registers. This is very much the wrong approach. It would require all platforms/master drivers that are going to use this chip to program the chip's registers. That's not the way to do it. The ov9640 driver should do this and the bridge driver should only pass high-level config data (such as with input/output pins are being used, signalling setup, whatever). This is how for example the Micron sensor mt* drivers do it. This approach means that all the relevant programming is inside the chip's driver and that bridge drivers can easily replace one chip for another since except for a little bit of initialization (usually routing, signalling setup) there is otherwise no difference between using one chip or another. Remember that this is not a driver for use with omap, this is a generic i2c driver for this chip. omap just happens to be the only one using it right now, but that can easily change. Yes, it makes it harder to write an i2c driver, but experience has shown us that the advantages regarding reuse far outweigh this initial cost. I noticed that the tcm825x.c driver takes exactly the same wrong approach, BTW. Regards, Hans -- Hans Verkuil - video4linux developer - sponsored by TANDBERG -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
