Hi Wen, Thanks for the patch. Comments below also apply to the dis71430m patch.
On Thursday 16 December 2010 14:21:55 Wang, Wen W wrote: > From e9207483efc02fee92dd4fd564ce95d1cd0722a0 Mon Sep 17 00:00:00 2001 > From: Wen Wang <[email protected]> > Date: Fri, 17 Dec 2010 01:54:31 +0800 > Subject: [PATCH] iCDK secondary camera (ov2720) sensor driver based on v4l2 > subdev framework The secondary camera (ov2720) sensor driver which is used > on MFLD iCDK platform. > This is a V4L2 subdev based driver. Why hasn't the driver been submitted to the linux-media list first ? > Signed-off-by: Wen Wang <[email protected]> > --- > drivers/media/video/ov2720/Kconfig | 9 + > drivers/media/video/ov2720/Makefile | 5 + > drivers/media/video/ov2720/cam_gpio.h | 32 + > drivers/media/video/ov2720/ov2720.c | 1009 ++++++++++++++++++++++++++ > drivers/media/video/ov2720/ov2720.h | 130 ++++ > drivers/media/video/ov2720/ov2720_reg.h | 1205 > +++++++++++++++++++++++++++++++ drivers/media/video/ov2720/sensor_com.h | > 153 ++++ > 7 files changed, 2543 insertions(+), 0 deletions(-) > create mode 100644 drivers/media/video/ov2720/Kconfig > create mode 100644 drivers/media/video/ov2720/Makefile > create mode 100644 drivers/media/video/ov2720/cam_gpio.h > create mode 100644 drivers/media/video/ov2720/ov2720.c > create mode 100644 drivers/media/video/ov2720/ov2720.h > create mode 100644 drivers/media/video/ov2720/ov2720_reg.h > create mode 100644 drivers/media/video/ov2720/sensor_com.h > > diff --git a/drivers/media/video/ov2720/Kconfig > b/drivers/media/video/ov2720/Kconfig new file mode 100644 > index 0000000..ba16844 > --- /dev/null > +++ b/drivers/media/video/ov2720/Kconfig > @@ -0,0 +1,9 @@ > +config VIDEO_OV2720 > + tristate "Medifiled OV2720 RAW Sensor" > + depends on I2C && VIDEO_MFLDCI Please don't. The sensor is an independent chip and can be used with other platforms. This driver must not depend on Medfield. > + > + ---help--- > + Say Y here if your platform support DIS RAW Sensor. > + > + To compile this driver as a module, choose M here: the > + module will be called ov2720.ko. [snip] > diff --git a/drivers/media/video/ov2720/cam_gpio.h > b/drivers/media/video/ov2720/cam_gpio.h new file mode 100644 > index 0000000..3c14995 > --- /dev/null > +++ b/drivers/media/video/ov2720/cam_gpio.h > @@ -0,0 +1,32 @@ > +/* > + * Support for Medfield Penwell Camera Imaging ISP subsystem. Support for OmniVision OV2720 1080p HD sensor. [snip] > diff --git a/drivers/media/video/ov2720/ov2720.c > b/drivers/media/video/ov2720/ov2720.c new file mode 100644 > index 0000000..a9ef94a > --- /dev/null > +++ b/drivers/media/video/ov2720/ov2720.c [snip] > +static int ov2720_write(struct i2c_client *c, u16 reg, u8 value); > +static int ov2720_read(struct i2c_client *c, u32 reg, u32 * value); > +static int ov2720_hw_config(void); > +static int ov2720_write_regs(struct i2c_client *c, > + struct __s_register_setting *regs); Please reorder the functions to avoid forward declarations when possible. [snip] > + > +/* > + * Init ov2720 gpio > + */ > +static int ov2720_init_gpio1(void) > +{ > + void __iomem *gpio1_base = NULL; > + > + gpio1_base = ioremap_nocache(0xff12c800, 0x80); Don't use magic values like that. #define register addresses and values instead. It seems this is used to control a GPIO. Please use the GPIO framework instead. > + if (gpio1_base == NULL) { > + printk(KERN_ERR "gpio1_base mapped failed\n"); > + return -1; > + } > + writel(0xffe80703, gpio1_base + 0x10); > + > + return 0; > +} [snip] > +struct ov2720_format ov2720_formats[] = { > + { > + .desc = "Raw RGB Bayer", Which one ? > + .regs = NULL, > + }, > +}; [snip] > +static int ov2720_try_fmt(struct v4l2_subdev *sd, struct v4l2_format *fmt) The subdev video operations operating on v4l2_format are deprecated and have been removed from mainline. Don't use them. [snip] > +static int ov2720_hw_config() > +{ > + int ret; > + > + /* > + * request PWGN/RESETB gpio pin > + */ > + ret = gpio_request(GP_CAMERA_1_POWER_DOWN, "Camera 1 PWGN"); Different platforms will use different GPIOs. That information needs to come from outside of the driver. [snip] > diff --git a/drivers/media/video/ov2720/ov2720_reg.h > b/drivers/media/video/ov2720/ov2720_reg.h new file mode 100644 > index 0000000..7f34fd8 > --- /dev/null > +++ b/drivers/media/video/ov2720/ov2720_reg.h [snip] > +static const struct __s_register_setting __ov2720_VGA_30fps_ex[] = { > + {0x0100, 0x01}, > + {0x4800, 0x24}, > + {0x3718, 0x10}, > + {0x3702, 0x24}, > + {0x373a, 0x60}, > + {0x3715, 0x01}, > + {0x3703, 0x2e}, > + {0x3705, 0x10}, > + {0x3730, 0x30}, > + {0x3704, 0x62}, > + {0x3f06, 0x3a}, > + {0x371c, 0x00}, > + {0x371d, 0xc4}, > + {0x371e, 0x01}, > + {0x371f, 0x0d}, > + {0x3708, 0x61}, > + {0x3709, 0x12}, Please don't hardcode a bunch of register values. You should compute them (or at least most of them) from parameters requested by userspace applications (such as format, size, frame rate, ...). [snip] -- Regards, Laurent Pinchart _______________________________________________ MeeGo-kernel mailing list [email protected] http://lists.meego.com/listinfo/meego-kernel
