Hi Hans, On Mon, Nov 24, 2008 at 3:30 AM, Hans Verkuil <hverkuil@xxxxxxxxx> wrote: > Hi Vaibhav, > > Here is my review as promised (although a day late). It's a mix of > smaller and larger issues: > > 1) CONFIG_VIDEO_ADV_DEBUG is meant to enable the ability to set/get > registers through the VIDIOC_DBG_G/S_REGISTER ioctls. For general > debugging you should use a debug module option (see e.g. saa7115.c). Local dprintk with log levels is fine, as far as it not misused. > > 2) Please use the media/v4l2-i2c-drv.h or media/v4l2-i2c-drv-legacy.h > header to hide some of the i2c complexity (again, see e.g. saa7115.c). > The i2c API tends to change a lot (and some changes are upcoming) so > using this header will mean that i2c driver changes will be minimal in > the future. In addition it will ensure that this driver can be compiled > with older kernels as well once it is part of the v4l-dvb repository. I don't agree with having support to compile with older kernels. Even though I2C APIs change as lot it is for good, and creating abstractions doesn't help as saa7xxx is family of chips where I don't see the case here. Once this driver is mainlined if someone does i2c subsystem change which breaks this driver from building then he/she has to make changes to all the code affecting it. I am not in favour of adding support to compile with older kernels. > > 3) Remember that the use of v4l2-int-device.h must be temporary only. It > will make it impossible to use this driver with any other platform but > omap. I had hoped to release my generic v4l2 subdevice support today > which should replace v4l2-int-device.h in time, but I hit a bug that > needs to be resolved first. I hope to fix it during the next week so > that I can finally make it available for use asap. Better step would be to have single camera-sensor framework or merge with soc-camera framework. Two frameworks doesn't help anyone, but creates more confusion for new developers. -- ---Trilok Soni http://triloksoni.wordpress.com http://www.linkedin.com/in/triloksoni -- 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
