Re: [PATCH 3/3] V4L2: Add a v4l2-subdev (soc-camera) driver forOmniVision OV2640 sensor

From: Guennadi Liakhovetski
Date: Wed Dec 01 2010 - 18:33:42 EST

Well, I have no secrets, but I'm not sure everyone on the CC list is
really interested in this thread(s)... So, please consider dropping some
of them when replying, they might be grateful;)

In general looks good, just a couple of easy to fix remarks below, and,
please, fix line wrapping with the next version.

On Sun, 28 Nov 2010, Alberto Panizzo wrote:

> Signed-off-by: Alberto Panizzo <maramaopercheseimorto@xxxxxxxxx>
> ---
> drivers/media/video/Kconfig | 6 +
> drivers/media/video/Makefile | 1 +
> drivers/media/video/ov2640.c | 1153
> +++++++++++++++++++++++++++++++++++++++
> include/media/v4l2-chip-ident.h | 1 +
> 4 files changed, 1161 insertions(+), 0 deletions(-)
> create mode 100644 drivers/media/video/ov2640.c
>
> diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
> index 0efbb29..898f76f 100644
> --- a/drivers/media/video/Kconfig
> +++ b/drivers/media/video/Kconfig
> @@ -807,6 +807,12 @@ config SOC_CAMERA_OV9640
> help
> This is a ov9640 camera driver
>
> +config SOC_CAMERA_OV2640
> + tristate "ov2640 camera support"
> + depends on SOC_CAMERA && I2C
> + help
> + This is a ov2640 camera driver
> +

might as well keep them alphabetically and numerically ordered

> config MX1_VIDEO
> bool
>
> diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
> index af79d47..fac185d 100644
> --- a/drivers/media/video/Makefile
> +++ b/drivers/media/video/Makefile
> @@ -82,6 +82,7 @@ obj-$(CONFIG_SOC_CAMERA_MT9V022) += mt9v022.o
> obj-$(CONFIG_SOC_CAMERA_OV6650) += ov6650.o
> obj-$(CONFIG_SOC_CAMERA_OV772X) += ov772x.o
> obj-$(CONFIG_SOC_CAMERA_OV9640) += ov9640.o
> +obj-$(CONFIG_SOC_CAMERA_OV2640) += ov2640.o

ditto

> obj-$(CONFIG_SOC_CAMERA_RJ54N1) += rj54n1cb0c.o
> obj-$(CONFIG_SOC_CAMERA_TW9910) += tw9910.o
>
> diff --git a/drivers/media/video/ov2640.c b/drivers/media/video/ov2640.c
> new file mode 100644
> index 0000000..5edf23e
> --- /dev/null
> +++ b/drivers/media/video/ov2640.c
> @@ -0,0 +1,1153 @@
> +/*
> + * ov2640 Camera Driver
> + *
> + * Copyright (C) 2010 Alberto Panizzo <maramaopercheseimorto@xxxxxxxxx>
> + *
> + * Based on ov772x, ov9640 drivers and previous non merged
> implementations.

Wrapped lines throughout the patch

> + *
> + * Copyright 2005-2009 Freescale Semiconductor, Inc. All Rights
> Reserved.
> + * Copyright (C) 2006, OmniVision
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/slab.h>
> +#include <linux/delay.h>
> +#include <linux/videodev2.h>
> +#include <media/v4l2-chip-ident.h>
> +#include <media/v4l2-subdev.h>
> +#include <media/soc_camera.h>
> +#include <media/soc_mediabus.h>
> +
> +#define VAL_SET(x, mask, rshift, lshift) \
> + ((((x) >> rshift) & mask) << lshift)
> +/*
> + * DSP registers
> + * register offset for BANK_SEL == BANK_SEL_DSP
> + */
> +#define R_BYPASS 0x05 /* Bypass DSP */
> +#define R_BYPASS_DSP_BYPAS 0x01 /* Bypass DSP. sensor out directly
> */
> +#define R_BYPASS_USE_DSP 0x00 /* Bypass DSP. sensor out directly
> */

Second comment wrong?

> +#define QS 0x44 /* Quantization Scale Factor */
> +#define CTRLI 0x50
> +#define CTRLI_LP_DP 0x80
> +#define CTRLI_ROUND 0x40
> +#define CTRLI_V_DIV_SET(x) VAL_SET(x, 0x3, 0, 3)
> +#define CTRLI_H_DIV_SET(x) VAL_SET(x, 0x3, 0, 0)
> +#define HSIZE 0x51 /* H_SIZE[7:0] (real/4) */
> +#define HSIZE_SET(x) VAL_SET(x, 0xFF, 2, 0)
> +#define VSIZE 0x52 /* V_SIZE[7:0] (real/4) */
> +#define VSIZE_SET(x) VAL_SET(x, 0xFF, 2, 0)
> +#define XOFFL 0x53 /* OFFSET_X[7:0] */
> +#define XOFFL_SET(x) VAL_SET(x, 0xFF, 0, 0)
> +#define YOFFL 0x54 /* OFFSET_Y[7:0] */
> +#define YOFFL_SET(x) VAL_SET(x, 0xFF, 0, 0)
> +#define VHYX 0x55 /* Offset and size completion */
> +#define VHYX_VSIZE_SET(x) VAL_SET(x, 0x1, (8+2), 7)
> +#define VHYX_HSIZE_SET(x) VAL_SET(x, 0x1, (8+2), 3)
> +#define VHYX_YOFF_SET(x) VAL_SET(x, 0x3, 8, 4)
> +#define VHYX_XOFF_SET(x) VAL_SET(x, 0x3, 8, 0)
> +#define DPRP 0x56
> +#define TEST 0x57 /* Horizontal size completion */
> +#define TEST_HSIZE_SET(x) VAL_SET(x, 0x1, (9+2), 7)
> +#define ZMOW 0x5A /* Zoom: Out Width OUTW[7:0] (real/4) */
> +#define ZMOW_OUTW_SET(x) VAL_SET(x, 0xFF, 2, 0)
> +#define ZMOH 0x5B /* Zoom: Out Height OUTH[7:0] (real/4) */
> +#define ZMOH_OUTH_SET(x) VAL_SET(x, 0xFF, 2, 0)
> +#define ZMHH 0x5C /* Zoom: Speed and H&W completion */
> +#define ZMHH_ZSPEED_SET(x) VAL_SET(x, 0x0F, 0, 4)
> +#define ZMHH_OUTH_SET(x) VAL_SET(x, 0x1, (8+2), 2)
> +#define ZMHH_OUTW_SET(x) VAL_SET(x, 0x3, (8+2), 0)
> +#define BPADDR 0x7C /* SDE Indirect Register Access: Address */
> +#define BPDATA 0x7D /* SDE Indirect Register Access: Data */
> +#define CTRL2 0x86 /* DSP Module enable 2 */
> +#define CTRL2_DCW_EN 0x20
> +#define CTRL2_SDE_EN 0x10
> +#define CTRL2_UV_ADJ_EN 0x08
> +#define CTRL2_UV_AVG_EN 0x04
> +#define CTRL2_CMX_EN 0x01
> +#define CTRL3 0x87 /* DSP Module enable 3 */
> +#define CTRL3_BPC_EN 0x80
> +#define CTRL3_WPC_EN 0x40
> +#define SIZEL 0x8C /* Image Size Completion */
> +#define SIZEL_HSIZE8_11_SET(x) VAL_SET(x, 0x1, 11, 6)
> +#define SIZEL_HSIZE8_SET(x) VAL_SET(x, 0x7, 0, 3)
> +#define SIZEL_VSIZE8_SET(x) VAL_SET(x, 0x7, 0, 0)
> +#define HSIZE8 0xC0 /* Image Horizontal Size HSIZE[10:3] */
> +#define HSIZE8_SET(x) VAL_SET(x, 0xFF, 3, 0)
> +#define VSIZE8 0xC1 /* Image Vertical Size VSIZE[10:3] */
> +#define VSIZE8_SET(x) VAL_SET(x, 0xFF, 3, 0)
> +#define CTRL0 0xC2 /* DSP Module enable 0 */
> +#define CTRL0_AEC_EN 0x80
> +#define CTRL0_AEC_SEL 0x40
> +#define CTRL0_STAT_SEL 0x20
> +#define CTRL0_VFIRST 0x10
> +#define CTRL0_YUV422 0x08
> +#define CTRL0_YUV_EN 0x04
> +#define CTRL0_RGB_EN 0x02
> +#define CTRL0_RAW_EN 0x01
> +#define CTRL1 0xC3 /* DSP Module enable 1 */
> +#define CTRL1_CIP 0x80
> +#define CTRL1_DMY 0x40
> +#define CTRL1_RAW_GMA 0x20
> +#define CTRL1_DG 0x10
> +#define CTRL1_AWB 0x08
> +#define CTRL1_AWB_GAIN 0x04
> +#define CTRL1_LENC 0x02
> +#define CTRL1_PRE 0x01
> +#define R_DVP_SP 0xD3 /* DVP output speed control */
> +#define R_DVP_SP_AUTO_MODE 0x80
> +#define R_DVP_SP_DVP_MASK 0x3F /* DVP PCLK = sysclk (48)/[6:0]
> (YUV0);
> + * = sysclk (48)/(2*[6:0]) (RAW);*/
> +#define IMAGE_MODE 0xDA /* Image Output Format Select */
> +#define IMAGE_MODE_Y8_DVP_EN 0x40
> +#define IMAGE_MODE_JPEG_EN 0x10
> +#define IMAGE_MODE_YUV422 0x00
> +#define IMAGE_MODE_RAW10 0x04 /* (DVP) */
> +#define IMAGE_MODE_RGB565 0x08
> +#define IMAGE_MODE_HREF_VSYNC 0x02 /* HREF timing select in DVP JPEG
> output
> + * mode (0 for HREF is same as sensor) */
> +#define IMAGE_MODE_LBYTE_FIRST 0x01 /* Byte swap enable for DVP
> + * 1: Low byte first UYVY (C2[4] =0)
> + * VYUY (C2[4] =1)
> + * 0: High byte first YUYV (C2[4]=0)
> + * YVYU (C2[4] = 1) */
> +#define RESET 0xE0 /* Reset */
> +#define RESET_MICROC 0x40
> +#define RESET_SCCB 0x20
> +#define RESET_JPEG 0x10
> +#define RESET_DVP 0x04
> +#define RESET_IPU 0x02
> +#define RESET_CIF 0x01
> +#define REGED 0xED /* Register ED */
> +#define REGED_CLK_OUT_DIS 0x10
> +#define MS_SP 0xF0 /* SCCB Master Speed */
> +#define SS_ID 0xF7 /* SCCB Slave ID */
> +#define SS_CTRL 0xF8 /* SCCB Slave Control */
> +#define SS_CTRL_ADD_AUTO_INC 0x20
> +#define SS_CTRL_EN 0x08
> +#define SS_CTRL_DELAY_CLK 0x04
> +#define SS_CTRL_ACC_EN 0x02
> +#define SS_CTRL_SEN_PASS_THR 0x01
> +#define MC_BIST 0xF9 /* Microcontroller misc register */
> +#define MC_BIST_RESET 0x80 /* Microcontroller Reset */
> +#define MC_BIST_BOOT_ROM_SEL 0x40
> +#define MC_BIST_12KB_SEL 0x20
> +#define MC_BIST_12KB_MASK 0x30
> +#define MC_BIST_512KB_SEL 0x08
> +#define MC_BIST_512KB_MASK 0x0C
> +#define MC_BIST_BUSY_BIT_R 0x02
> +#define MC_BIST_MC_RES_ONE_SH_W 0x02
> +#define MC_BIST_LAUNCH 0x01
> +#define BANK_SEL 0xFF /* Register Bank Select */
> +#define BANK_SEL_DSP 0x00
> +#define BANK_SEL_SENS 0x01
> +
> +/*
> + * Sensor registers
> + * register offset for BANK_SEL == BANK_SEL_SENS
> + */
> +#define GAIN 0x00 /* AGC - Gain control gain setting */
> +#define COM1 0x03 /* Common control 1 */
> +#define COM1_1_DUMMY_FR 0x40
> +#define COM1_3_DUMMY_FR 0x80
> +#define COM1_7_DUMMY_FR 0xC0
> +#define COM1_VWIN_LSB_UXGA 0x0F
> +#define COM1_VWIN_LSB_SVGA 0x0A
> +#define COM1_VWIN_LSB_CIF 0x06
> +#define REG04 0x04 /* Register 04 */
> +#define REG04_DEF 0x20 /* Always set */
> +#define REG04_HFLIP_IMG 0x80 /* Horizontal mirror image ON/OFF
> */
> +#define REG04_VFLIP_IMG 0x40 /* Vertical flip image ON/OFF */
> +#define REG04_VREF_EN 0x10
> +#define REG04_HREF_EN 0x08
> +#define REG04_AEC_SET(x) VAL_SET(x, 0x3, 0, 0)
> +#define REG08 0x08 /* Frame Exposure One-pin Control Pre-charge
> Row Num */
> +#define COM2 0x09 /* Common control 2 */
> +#define COM2_SOFT_SLEEP_MODE 0x10 /* Soft sleep mode */
> + /* Output drive capability */
> +#define COM2_OCAP_Nx_SET(N) (((N) - 1) & 0x03) /* N = [1x .. 4x] */
> +#define PID 0x0A /* Product ID Number MSB */
> +#define VER 0x0B /* Product ID Number LSB */
> +#define COM3 0x0C /* Common control 3 */
> +#define COM3_BAND_50H 0x04 /* 0 For Banding at 60H */
> +#define COM3_BAND_AUTO 0x02 /* Auto Banding */
> +#define COM3_SING_FR_SNAPSH 0x01 /* 0 For enable live video output
> after the
> + * snapshot sequence*/
> +#define AEC 0x10 /* AEC[9:2] Exposure Value */
> +#define CLKRC 0x11 /* Internal clock */
> +#define CLKRC_EN 0x80
> +#define CLKRC_DIV_SET(x) (((x) - 1) & 0x1F) /* CLK = XVCLK/(x) */
> +#define COM7 0x12 /* Common control 7 */
> +#define COM7_SRST 0x80 /* Initiates system reset. All
> registers are
> + * set to factory default values after which
> + * the chip resumes normal operation */
> +#define COM7_RES_UXGA 0x00 /* Resolution selectors for UXGA */
> +#define COM7_RES_SVGA 0x40 /* SVGA */
> +#define COM7_RES_CIF 0x20 /* CIF */
> +#define COM7_ZOOM_EN 0x04 /* Enable Zoom mode */
> +#define COM7_COLOR_BAR_TEST 0x02 /* Enable Color Bar Test Pattern */
> +#define COM8 0x13 /* Common control 8 */
> +#define COM8_DEF 0xC0 /* Banding filter ON/OFF */
> +#define COM8_BNDF_EN 0x20 /* Banding filter ON/OFF */
> +#define COM8_AGC_EN 0x04 /* AGC Auto/Manual control
> selection */
> +#define COM8_AEC_EN 0x01 /* Auto/Manual Exposure control */
> +#define COM9 0x14 /* Common control 9
> + * Automatic gain ceiling - maximum AGC value [7:5]*/
> +#define COM9_AGC_GAIN_2x 0x00 /* 000 : 2x */
> +#define COM9_AGC_GAIN_4x 0x20 /* 001 : 4x */
> +#define COM9_AGC_GAIN_8x 0x40 /* 010 : 8x */
> +#define COM9_AGC_GAIN_16x 0x60 /* 011 : 16x */
> +#define COM9_AGC_GAIN_32x 0x80 /* 100 : 32x */
> +#define COM9_AGC_GAIN_64x 0xA0 /* 101 : 64x */
> +#define COM9_AGC_GAIN_128x 0xC0 /* 110 : 128x */
> +#define COM10 0x15 /* Common control 10 */
> +#define COM10_PCLK_HREF 0x20 /* PCLK output qualified by HREF */
> +#define COM10_PCLK_RISE 0x10 /* Data is updated at the rising
> edge of
> + * PCLK (user can latch data at the next
> + * falling edge of PCLK).
> + * 0 otherwise. */
> +#define COM10_HREF_INV 0x08 /* Invert HREF polarity:
> + * HREF negative for valid data*/
> +#define COM10_VSINC_INV 0x02 /* Invert VSYNC polarity */
> +#define HSTART 0x17 /* Horizontal Window start MSB 8 bit */
> +#define HEND 0x18 /* Horizontal Window end MSB 8 bit */
> +#define VSTART 0x19 /* Vertical Window start MSB 8 bit */
> +#define VEND 0x1A /* Vertical Window end MSB 8 bit */
> +#define MIDH 0x1C /* Manufacturer ID byte - high */
> +#define MIDL 0x1D /* Manufacturer ID byte - low */
> +#define AEW 0x24 /* AGC/AEC - Stable operating region (upper
> limit) */
> +#define AEB 0x25 /* AGC/AEC - Stable operating region (lower
> limit) */
> +#define VV 0x26 /* AGC/AEC Fast mode operating region */
> +#define VV_HIGH_TH_SET(x) VAL_SET(x, 0xF, 0, 4)
> +#define VV_LOW_TH_SET(x) VAL_SET(x, 0xF, 0, 0)
> +#define REG2A 0x2A /* Dummy pixel insert MSB */
> +#define FRARL 0x2B /* Dummy pixel insert LSB */
> +#define ADDVFL 0x2D /* LSB of insert dummy lines in Vertical
> direction */
> +#define ADDVFH 0x2E /* MSB of insert dummy lines in Vertical
> direction */
> +#define YAVG 0x2F /* Y/G Channel Average value */
> +#define REG32 0x32 /* Common Control 32 */
> +#define REG32_PCLK_DIV_2 0x80 /* PCLK freq divided by 2 */
> +#define REG32_PCLK_DIV_4 0xC0 /* PCLK freq divided by 4 */
> +#define ARCOM2 0x34 /* Zoom: Horizontal start point */
> +#define REG45 0x45 /* Register 45 */
> +#define FLL 0x46 /* Frame Length Adjustment LSBs */
> +#define FLH 0x47 /* Frame Length Adjustment MSBs */
> +#define COM19 0x48 /* Zoom: Vertical start point */
> +#define ZOOMS 0x49 /* Zoom: Vertical start point */
> +#define COM22 0x4B /* Flash light control */
> +#define COM25 0x4E /* For Banding operations */
> +#define BD50 0x4F /* 50Hz Banding AEC 8 LSBs */
> +#define BD60 0x50 /* 60Hz Banding AEC 8 LSBs */
> +#define REG5D 0x5D /* AVGsel[7:0], 16-zone average weight
> option */
> +#define REG5E 0x5E /* AVGsel[15:8], 16-zone average weight
> option */
> +#define REG5F 0x5F /* AVGsel[23:16], 16-zone average weight
> option */
> +#define REG60 0x60 /* AVGsel[31:24], 16-zone average weight
> option */
> +#define HISTO_LOW 0x61 /* Histogram Algorithm Low Level */
> +#define HISTO_HIGH 0x62 /* Histogram Algorithm High Level */
> +
> +/*
> + * ID
> + */
> +#define MANUFACTURER_ID 0x7FA2
> +#define PID_OV2640 0x2642
> +#define VERSION(pid, ver) ((pid<<8)|(ver&0xFF))

please, add spaces

> +
> +/*
> + * struct
> + */
> +struct regval_list {
> + u8 reg_num;
> + u16 value;
> +};
> +
> +/* supported resolutions */
> +enum ov2640_width_sizes {

nitpicking really, but wouldn't "enum ov2640_width" be descriptive enough?

> + W_QCIF = 176,
> + W_QVGA = 320,
> + W_CIF = 352,
> + W_VGA = 640,
> + W_SVGA = 800,
> + W_XGA = 1024,
> + W_SXGA = 1280,
> + W_UXGA = 1600,
> +};
> +
> +enum ov2640_height_sizes {

ditto

> + H_QCIF = 144,
> + H_QVGA = 240,
> + H_CIF = 288,
> + H_VGA = 480,
> + H_SVGA = 600,
> + H_XGA = 768,
> + H_SXGA = 1024,
> + H_UXGA = 1200,
> +};
> +
> +struct ov2640_win_size {
> + char *name;
> + enum ov2640_width_sizes width;
> + enum ov2640_height_sizes height;
> + const struct regval_list *regs;
> +};
> +
> +
> +struct ov2640_priv {
> + struct v4l2_subdev subdev;
> + struct ov2640_camera_info *info;
> + enum v4l2_mbus_pixelcode cfmt_code;
> + const struct ov2640_win_size *win;
> + int model;
> + u16 flag_vflip:1;
> + u16 flag_hflip:1;
> +};
> +
> +/*
> + * Registers settings
> + */
> +
> +#define ENDMARKER { 0xff, 0xff }
> +
> +static const struct regval_list ov2640_init_regs[] = {
> + { BANK_SEL, BANK_SEL_DSP },
> + { 0x2c, 0xff },
> + { 0x2e, 0xdf },
> + { BANK_SEL, BANK_SEL_SENS },
> + { 0x3c, 0x32 },
> + { CLKRC, CLKRC_DIV_SET(1) },
> + { COM2, COM2_OCAP_Nx_SET(3) },
> + { REG04, REG04_DEF | REG04_HREF_EN },
> + { COM8, COM8_DEF | COM8_BNDF_EN | COM8_AGC_EN | COM8_AEC_EN },
> + { COM9, COM9_AGC_GAIN_8x | 0x08},
> + { 0x2c, 0x0c },
> + { 0x33, 0x78 },
> + { 0x3a, 0x33 },
> + { 0x3b, 0xfb },
> + { 0x3e, 0x00 },
> + { 0x43, 0x11 },
> + { 0x16, 0x10 },
> + { 0x39, 0x02 },
> + { 0x35, 0x88 },
> + { 0x22, 0x0a },
> + { 0x37, 0x40 },
> + { 0x23, 0x00 },
> + { ARCOM2, 0xa0 },
> + { 0x06, 0x02 },
> + { 0x06, 0x88 },
> + { 0x07, 0xc0 },
> + { 0x0d, 0xb7 },
> + { 0x0e, 0x01 },
> + { 0x4c, 0x00 },
> + { 0x4a, 0x81 },
> + { 0x21, 0x99 },
> + { AEW, 0x40 },
> + { AEB, 0x38 },
> + { VV, VV_HIGH_TH_SET(0x08) | VV_LOW_TH_SET(0x02) },
> + { 0x5c, 0x00 },
> + { 0x63, 0x00 },
> + { FLL, 0x22 },
> + { COM3, 0x38 | COM3_BAND_AUTO },
> + { REG5D, 0x55 },
> + { REG5E, 0x7d },
> + { REG5F, 0x7d },
> + { REG60, 0x55 },
> + { HISTO_LOW, 0x70 },
> + { HISTO_HIGH, 0x80 },
> + { 0x7c, 0x05 },
> + { 0x20, 0x80 },
> + { 0x28, 0x30 },
> + { 0x6c, 0x00 },
> + { 0x6d, 0x80 },
> + { 0x6e, 0x00 },
> + { 0x70, 0x02 },
> + { 0x71, 0x94 },
> + { 0x73, 0xc1 },
> + { 0x3d, 0x34 },
> + { COM7, COM7_RES_UXGA | COM7_ZOOM_EN },
> + { 0x5a, 0x57 },
> + { BD50, 0xbb },
> + { BD60, 0x9c },
> + { BANK_SEL, BANK_SEL_DSP },
> + { 0xe5, 0x7f },
> + { MC_BIST, MC_BIST_RESET | MC_BIST_BOOT_ROM_SEL },
> + { 0x41, 0x24 },
> + { RESET, RESET_JPEG | RESET_DVP },
> + { 0x76, 0xff },
> + { 0x33, 0xa0 },
> + { 0x42, 0x20 },
> + { 0x43, 0x18 },
> + { 0x4c, 0x00 },
> + { CTRL3, CTRL3_BPC_EN | CTRL3_WPC_EN | 0x10 },
> + { 0x88, 0x3f },
> + { 0xd7, 0x03 },
> + { 0xd9, 0x10 },
> + { R_DVP_SP , R_DVP_SP_AUTO_MODE | 0x2 },
> + { 0xc8, 0x08 },
> + { 0xc9, 0x80 },
> + { BPADDR, 0x00 },
> + { BPDATA, 0x00 },
> + { BPADDR, 0x03 },
> + { BPDATA, 0x48 },
> + { BPDATA, 0x48 },
> + { BPADDR, 0x08 },
> + { BPDATA, 0x20 },
> + { BPDATA, 0x10 },
> + { BPDATA, 0x0e },
> + { 0x90, 0x00 },
> + { 0x91, 0x0e },
> + { 0x91, 0x1a },
> + { 0x91, 0x31 },
> + { 0x91, 0x5a },
> + { 0x91, 0x69 },
> + { 0x91, 0x75 },
> + { 0x91, 0x7e },
> + { 0x91, 0x88 },
> + { 0x91, 0x8f },
> + { 0x91, 0x96 },
> + { 0x91, 0xa3 },
> + { 0x91, 0xaf },
> + { 0x91, 0xc4 },
> + { 0x91, 0xd7 },
> + { 0x91, 0xe8 },
> + { 0x91, 0x20 },
> + { 0x92, 0x00 },
> + { 0x93, 0x06 },
> + { 0x93, 0xe3 },
> + { 0x93, 0x03 },
> + { 0x93, 0x03 },
> + { 0x93, 0x00 },
> + { 0x93, 0x02 },
> + { 0x93, 0x00 },
> + { 0x93, 0x00 },
> + { 0x93, 0x00 },
> + { 0x93, 0x00 },
> + { 0x93, 0x00 },
> + { 0x93, 0x00 },
> + { 0x93, 0x00 },
> + { 0x96, 0x00 },
> + { 0x97, 0x08 },
> + { 0x97, 0x19 },
> + { 0x97, 0x02 },
> + { 0x97, 0x0c },
> + { 0x97, 0x24 },
> + { 0x97, 0x30 },
> + { 0x97, 0x28 },
> + { 0x97, 0x26 },
> + { 0x97, 0x02 },
> + { 0x97, 0x98 },
> + { 0x97, 0x80 },
> + { 0x97, 0x00 },
> + { 0x97, 0x00 },
> + { 0xa4, 0x00 },
> + { 0xa8, 0x00 },
> + { 0xc5, 0x11 },
> + { 0xc6, 0x51 },
> + { 0xbf, 0x80 },
> + { 0xc7, 0x10 },
> + { 0xb6, 0x66 },
> + { 0xb8, 0xA5 },
> + { 0xb7, 0x64 },
> + { 0xb9, 0x7C },
> + { 0xb3, 0xaf },
> + { 0xb4, 0x97 },
> + { 0xb5, 0xFF },
> + { 0xb0, 0xC5 },
> + { 0xb1, 0x94 },
> + { 0xb2, 0x0f },
> + { 0xc4, 0x5c },
> + { 0xa6, 0x00 },
> + { 0xa7, 0x20 },
> + { 0xa7, 0xd8 },
> + { 0xa7, 0x1b },
> + { 0xa7, 0x31 },
> + { 0xa7, 0x00 },
> + { 0xa7, 0x18 },
> + { 0xa7, 0x20 },
> + { 0xa7, 0xd8 },
> + { 0xa7, 0x19 },
> + { 0xa7, 0x31 },
> + { 0xa7, 0x00 },
> + { 0xa7, 0x18 },
> + { 0xa7, 0x20 },
> + { 0xa7, 0xd8 },
> + { 0xa7, 0x19 },
> + { 0xa7, 0x31 },
> + { 0xa7, 0x00 },
> + { 0xa7, 0x18 },
> + { 0x7f, 0x00 },
> + { 0xe5, 0x1f },
> + { 0xe1, 0x77 },
> + { 0xdd, 0x7f },
> + { CTRL0, CTRL0_YUV422 | CTRL0_YUV_EN | CTRL0_RGB_EN },
> + ENDMARKER,
> +};

Nice, have I mentioned, how I don't find such register lists particularly
developer-friendly?:) But this is one of the cases where I don't think
requesting you to open code this one would be a wise way to spend your
time, even if it is done in emacs with something around 50 key-strokes;)
Hm, these repeated writes to unnamed registers 0x91, 0x93, 0x97, 0xa7 look
like some writing to internal memory, perhaps?

> +
> +/*
> + * register setting for window size
> + * The preamble setup the internal DSP to input a UXGA (1600x1200)
> image,
> + * then the different zooming configurations will set up the output
> image size.
> + */
> +static const struct regval_list ov2640_size_change_preamble_regs[] = {
> + { BANK_SEL, BANK_SEL_DSP },
> + { RESET, RESET_DVP },
> + { HSIZE8, HSIZE8_SET(W_UXGA) }, { VSIZE8, VSIZE8_SET(H_UXGA) },
> + { CTRL2, CTRL2_DCW_EN | CTRL2_SDE_EN |
> + CTRL2_UV_AVG_EN | CTRL2_CMX_EN | CTRL2_UV_ADJ_EN },
> + { HSIZE, HSIZE_SET(W_UXGA) }, { VSIZE, VSIZE_SET(H_UXGA) },
> + { XOFFL, XOFFL_SET(0) }, { YOFFL, YOFFL_SET(0) },

IMHO this would look prettier one-per-line, similarly below

> + { VHYX, VHYX_HSIZE_SET(W_UXGA) | VHYX_VSIZE_SET(H_UXGA) |
> + VHYX_XOFF_SET(0) | VHYX_YOFF_SET(0)},
> + { TEST, TEST_HSIZE_SET(W_UXGA) },
> + ENDMARKER,
> +};
> +
> +#define PER_SIZE_REG_SEQ(x, y, v_div, h_div, pclk_div) \
> + { CTRLI, CTRLI_LP_DP | CTRLI_V_DIV_SET(v_div) | \
> + CTRLI_H_DIV_SET(h_div)}, \
> + { ZMOW, ZMOW_OUTW_SET(x) }, { ZMOH, ZMOH_OUTH_SET(y) }, \
> + { ZMHH, ZMHH_OUTW_SET(x) | ZMHH_OUTH_SET(y) }, \
> + { R_DVP_SP, pclk_div }, \
> + { RESET, 0x00}
> +
> +static const struct regval_list ov2640_qcif_regs[] = {
> + PER_SIZE_REG_SEQ(W_QCIF, H_QCIF, 3, 3, 4),
> + ENDMARKER,
> +};
> +
> +static const struct regval_list ov2640_qvga_regs[] = {
> + PER_SIZE_REG_SEQ(W_QVGA, H_QVGA, 2, 2, 4),
> + ENDMARKER,
> +};
> +
> +static const struct regval_list ov2640_cif_regs[] = {
> + PER_SIZE_REG_SEQ(W_CIF, H_CIF, 2, 2, 8),
> + ENDMARKER,
> +};
> +
> +static const struct regval_list ov2640_vga_regs[] = {
> + PER_SIZE_REG_SEQ(W_VGA, H_VGA, 0, 0, 2),
> + ENDMARKER,
> +};
> +
> +static const struct regval_list ov2640_svga_regs[] = {
> + PER_SIZE_REG_SEQ(W_SVGA, H_SVGA, 1, 1, 2),
> + ENDMARKER,
> +};
> +
> +static const struct regval_list ov2640_xga_regs[] = {
> + PER_SIZE_REG_SEQ(W_XGA, H_XGA, 0, 0, 2),
> + { CTRLI, 0x00},
> + ENDMARKER,
> +};
> +
> +static const struct regval_list ov2640_sxga_regs[] = {
> + PER_SIZE_REG_SEQ(W_SXGA, H_SXGA, 0, 0, 2),
> + { CTRLI, 0x00},
> + { R_DVP_SP, 2 | R_DVP_SP_AUTO_MODE },
> + ENDMARKER,
> +};
> +
> +static const struct regval_list ov2640_uxga_regs[] = {
> + PER_SIZE_REG_SEQ(W_UXGA, H_UXGA, 0, 0, 0),
> + { CTRLI, 0x00},
> + { R_DVP_SP, 0 | R_DVP_SP_AUTO_MODE },
> + ENDMARKER,
> +};
> +
> +#define OV2640_SIZE(n, w, h, r) \
> + {.name = n, .width = w , .height = h, .regs = r }
> +
> +static const struct ov2640_win_size ov2640_supported_win_sizes[] = {
> + OV2640_SIZE("QCIF", W_QCIF, H_QCIF, ov2640_qcif_regs),
> + OV2640_SIZE("QVGA", W_QVGA, H_QVGA, ov2640_qvga_regs),
> + OV2640_SIZE("CIF", W_CIF, H_CIF, ov2640_cif_regs),
> + OV2640_SIZE("VGA", W_VGA, H_VGA, ov2640_vga_regs),
> + OV2640_SIZE("SVGA", W_SVGA, H_SVGA, ov2640_svga_regs),
> + OV2640_SIZE("XGA", W_XGA, H_XGA, ov2640_xga_regs),
> + OV2640_SIZE("SXGA", W_SXGA, H_SXGA, ov2640_sxga_regs),
> + OV2640_SIZE("UXGA", W_UXGA, H_UXGA, ov2640_uxga_regs),
> +};
> +
> +/*
> + * Register settings for pixel formats
> + */
> +static const struct regval_list ov2640_format_change_preamble_regs[] =
> {
> + { BANK_SEL, BANK_SEL_DSP },
> + { R_BYPASS, R_BYPASS_USE_DSP },
> + ENDMARKER,
> +};
> +
> +static const struct regval_list ov2640_yuv422_regs[] = {
> + { IMAGE_MODE, IMAGE_MODE_LBYTE_FIRST | IMAGE_MODE_YUV422 },
> + { 0xD7, 0x01 },
> + { 0x33, 0xa0 },
> + { 0xe1, 0x67 },
> + { RESET, 0x00 },
> + { R_BYPASS, R_BYPASS_USE_DSP },
> + ENDMARKER,
> +};
> +
> +static const struct regval_list ov2640_rgb565_regs[] = {
> + { IMAGE_MODE, IMAGE_MODE_LBYTE_FIRST | IMAGE_MODE_RGB565 },
> + { 0xd7, 0x03 },
> + { RESET, 0x00 },
> + { R_BYPASS, R_BYPASS_USE_DSP },
> + ENDMARKER,
> +};
> +
> +static enum v4l2_mbus_pixelcode ov2640_codes[] = {
> + V4L2_MBUS_FMT_UYVY8_2X8,
> + V4L2_MBUS_FMT_RGB565_2X8_LE,

Have you also tested rgb565?

> +};
> +
> +/*
> + * Supported controls
> + */
> +static const struct v4l2_queryctrl ov2640_controls[] = {
> + {
> + .id = V4L2_CID_VFLIP,
> + .type = V4L2_CTRL_TYPE_BOOLEAN,
> + .name = "Flip Vertically",
> + .minimum = 0,
> + .maximum = 1,
> + .step = 1,
> + .default_value = 0,
> + }, {
> + .id = V4L2_CID_HFLIP,
> + .type = V4L2_CTRL_TYPE_BOOLEAN,
> + .name = "Flip Horizontally",
> + .minimum = 0,
> + .maximum = 1,
> + .step = 1,
> + .default_value = 0,
> + },
> +};
> +
> +/*
> + * general functions
> + */
> +static struct ov2640_priv *to_ov2640(const struct i2c_client *client)
> +{
> + return container_of(i2c_get_clientdata(client), struct ov2640_priv,
> + subdev);
> +}
> +
> +static int ov2640_write_array(struct i2c_client *client,
> + const struct regval_list *vals)
> +{
> + int ret;
> +
> + while ((vals->reg_num != 0xff) || (vals->value != 0xff)) {
> + ret = i2c_smbus_write_byte_data(client,
> + vals->reg_num,
> + vals->value & 0xFF);

Well, you trust and don't check, that register numbers are within range,
so, you might just as well trust the value.

> + dev_vdbg(&client->dev, "array: 0x%02x, 0x%02x",
> + vals->reg_num, vals->value & 0xFF);

ditto

> + if (ret < 0)
> + return ret;
> + vals++;
> + }
> + return 0;
> +}
> +
> +static int ov2640_mask_set(struct i2c_client *client,
> + u8 reg, u8 mask, u8 set)
> +{
> + s32 val = i2c_smbus_read_byte_data(client, reg);
> + if (val < 0)
> + return val;
> +
> + val &= ~mask;
> + val |= set & mask;
> +
> + dev_vdbg(&client->dev, "masks: 0x%02x, 0x%02x",
> + reg, val);
> + return i2c_smbus_write_byte_data(client, reg, val);
> +}
> +
> +static int ov2640_reset(struct i2c_client *client)
> +{
> + int ret;
> + const struct regval_list reset_seq[] = {
> + {BANK_SEL, BANK_SEL_SENS},
> + {COM7, COM7_SRST},
> + ENDMARKER,
> + };
> +
> + ret = ov2640_write_array(client, reset_seq);
> + if (ret)
> + goto err;
> +
> + msleep(5);
> +err:
> + dev_dbg(&client->dev, "%s: (ret %d)", __func__, ret);
> + return ret;
> +}
> +
> +/*
> + * soc_camera_ops functions
> + */
> +static int ov2640_s_stream(struct v4l2_subdev *sd, int enable)
> +{
> + return 0;
> +}
> +
> +static int ov2640_set_bus_param(struct soc_camera_device *icd,
> + unsigned long flags)
> +{
> + return 0;
> +}
> +
> +static unsigned long ov2640_query_bus_param(struct soc_camera_device
> *icd)
> +{
> + struct soc_camera_link *icl = to_soc_camera_link(icd);
> + unsigned long flags = SOCAM_PCLK_SAMPLE_RISING | SOCAM_MASTER |
> + SOCAM_VSYNC_ACTIVE_HIGH | SOCAM_HSYNC_ACTIVE_HIGH |
> + SOCAM_DATA_ACTIVE_HIGH | SOCAM_DATAWIDTH_8 | SOCAM_DATAWIDTH_10;

If I understand this correctly, your sensor has just 10 data lines, and
has no configuration to switch to any other bus width. Then I wouldn#t
advertise 8 and 10 bits here. Please, have a look how this is done, e.g.,
in mt9m001.c. There we're trying to reflect the configuration more
correctly: the sensor can do 10 bits only. But the platform can override
this, if only some data lines are actually connected on the board.

> +
> + return soc_camera_apply_sensor_flags(icl, flags);
> +}
> +
> +static int ov2640_g_ctrl(struct v4l2_subdev *sd, struct v4l2_control
> *ctrl)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(sd);
> + struct ov2640_priv *priv = to_ov2640(client);
> +
> + switch (ctrl->id) {
> + case V4L2_CID_VFLIP:
> + ctrl->value = priv->flag_vflip;
> + break;
> + case V4L2_CID_HFLIP:
> + ctrl->value = priv->flag_hflip;
> + break;
> + }
> + return 0;
> +}
> +
> +static int ov2640_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control
> *ctrl)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(sd);
> + struct ov2640_priv *priv = to_ov2640(client);
> + int ret = 0;
> + u8 val;
> +
> + switch (ctrl->id) {
> + case V4L2_CID_VFLIP:
> + val = ctrl->value ? REG04_VFLIP_IMG : 0x00;
> + priv->flag_vflip = ctrl->value ? 1 : 0;
> + ret = ov2640_mask_set(client, REG04, REG04_VFLIP_IMG, val);
> + break;
> + case V4L2_CID_HFLIP:
> + val = ctrl->value ? REG04_HFLIP_IMG : 0x00;
> + priv->flag_hflip = ctrl->value ? 1 : 0;
> + ret = ov2640_mask_set(client, REG04, REG04_HFLIP_IMG, val);
> + break;
> + }
> +
> + return ret;
> +}
> +
> +static int ov2640_g_chip_ident(struct v4l2_subdev *sd,
> + struct v4l2_dbg_chip_ident *id)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(sd);
> + struct ov2640_priv *priv = to_ov2640(client);
> +
> + id->ident = priv->model;
> + id->revision = 0;
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_VIDEO_ADV_DEBUG
> +static int ov2640_g_register(struct v4l2_subdev *sd,
> + struct v4l2_dbg_register *reg)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(sd);
> + int ret;
> +
> + reg->size = 1;
> + if (reg->reg > 0xff)
> + return -EINVAL;
> +
> + ret = i2c_smbus_read_byte_data(client, reg->reg);
> + if (ret < 0)
> + return ret;
> +
> + reg->val = (__u64)ret;

Is this type-cast really needed?

> +
> + return 0;
> +}
> +
> +static int ov2640_s_register(struct v4l2_subdev *sd,
> + struct v4l2_dbg_register *reg)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(sd);
> +
> + if (reg->reg > 0xff ||
> + reg->val > 0xff)
> + return -EINVAL;
> +
> + return i2c_smbus_write_byte_data(client, reg->reg, reg->val);
> +}
> +#endif
> +
> +/* select nearest higher resolution for capture */
> +static const struct ov2640_win_size *ov2640_select_win(u32 *width, u32
> *height)
> +{
> + int i, def = ARRAY_SIZE(ov2640_supported_win_sizes) - 1;
> +
> + for (i = 0; i < ARRAY_SIZE(ov2640_supported_win_sizes); i++) {

+ for (i = 0; i <= def; i++) {

would do too

> + if (ov2640_supported_win_sizes[i].width >= *width &&
> + ov2640_supported_win_sizes[i].height >= *height) {
> + *width = ov2640_supported_win_sizes[i].width;
> + *height = ov2640_supported_win_sizes[i].height;
> + return &ov2640_supported_win_sizes[i];
> + }
> + }
> +
> + *width = ov2640_supported_win_sizes[def].width;
> + *height = ov2640_supported_win_sizes[def].height;
> + return &ov2640_supported_win_sizes[def];
> +}
> +
> +static int ov2640_set_params(struct i2c_client *client, u32 *width, u32
> *height,
> + enum v4l2_mbus_pixelcode code)
> +{
> + struct ov2640_priv *priv = to_ov2640(client);
> + const struct regval_list *selected_cfmt_regs;
> + int ret = -EINVAL;
> +
> + /* select win */
> + priv->win = ov2640_select_win(width, height);
> +
> + /* select format */
> + priv->cfmt_code = 0;
> + switch (code) {
> + case V4L2_MBUS_FMT_RGB565_2X8_LE:
> + dev_dbg(&client->dev, "%s: Selected cfmt RGB565", __func__);
> + selected_cfmt_regs = ov2640_rgb565_regs;
> + break;
> + default:
> + case V4L2_MBUS_FMT_UYVY8_2X8:
> + dev_dbg(&client->dev, "%s: Selected cfmt YUV422", __func__);
> + selected_cfmt_regs = ov2640_yuv422_regs;
> + }
> +
> + /* reset hardware */
> + ov2640_reset(client);

hmm, what exactly does this reset do? It probably doesn't reset register
values, right? it only resets frame capture or what?

> +
> + dev_dbg(&client->dev, "%s: Init default", __func__);
> + /* Initialize the sensor with default data */
> + ret = ov2640_write_array(client, ov2640_init_regs);
> + if (ret < 0)
> + goto err;
> +
> + dev_dbg(&client->dev, "%s: Set size to %s", __func__,
> priv->win->name);
> + /* Select preamble */
> + ret = ov2640_write_array(client, ov2640_size_change_preamble_regs);
> + if (ret < 0)
> + goto err;
> +
> + /* set size win */
> + ret = ov2640_write_array(client, priv->win->regs);
> + if (ret < 0)
> + goto err;
> +
> + dev_dbg(&client->dev, "%s: Set cfmt", __func__);
> + /* Cfmt preamble */
> + ret = ov2640_write_array(client, ov2640_format_change_preamble_regs);
> + if (ret < 0)
> + goto err;
> +
> + /* set cfmt */
> + ret = ov2640_write_array(client, selected_cfmt_regs);
> + if (ret < 0)
> + goto err;
> +
> + priv->cfmt_code = code;
> + *width = priv->win->width;
> + *height = priv->win->height;
> +
> + return ret;
> +
> +err:
> + dev_err(&client->dev, "%s: Error %d", __func__, ret);
> + ov2640_reset(client);
> + priv->win = NULL;
> +
> + return ret;
> +}
> +
> +static int ov2640_g_fmt(struct v4l2_subdev *sd,
> + struct v4l2_mbus_framefmt *mf)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(sd);
> + struct ov2640_priv *priv = to_ov2640(client);
> +
> + if (!priv->win) {
> + u32 width = W_SVGA, height = H_SVGA;
> + int ret = ov2640_set_params(client, &width, &height,
> + V4L2_MBUS_FMT_UYVY8_2X8);
> + if (ret < 0)
> + return ret;
> + }
> +
> + mf->width = priv->win->width;
> + mf->height = priv->win->height;
> + mf->code = priv->cfmt_code;
> +
> + switch (mf->code) {
> + case V4L2_MBUS_FMT_RGB565_2X8_LE:
> + mf->colorspace = V4L2_COLORSPACE_SRGB;
> + break;
> + default:
> + case V4L2_MBUS_FMT_UYVY8_2X8:
> + mf->colorspace = V4L2_COLORSPACE_JPEG;
> + }
> + mf->field = V4L2_FIELD_NONE;
> +
> + return 0;
> +}
> +
> +static int ov2640_s_fmt(struct v4l2_subdev *sd,
> + struct v4l2_mbus_framefmt *mf)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(sd);
> + int ret;
> +
> +
> + switch (mf->code) {
> + case V4L2_MBUS_FMT_RGB565_2X8_LE:
> + mf->colorspace = V4L2_COLORSPACE_SRGB;
> + break;
> + default:
> + mf->code = V4L2_MBUS_FMT_UYVY8_2X8;
> + case V4L2_MBUS_FMT_UYVY8_2X8:
> + mf->colorspace = V4L2_COLORSPACE_JPEG;
> + }
> +
> + ret = ov2640_set_params(client, &mf->width, &mf->height,
> + mf->code);
> +
> + return ret;
> +}
> +
> +static int ov2640_try_fmt(struct v4l2_subdev *sd,
> + struct v4l2_mbus_framefmt *mf)
> +{
> + const struct ov2640_win_size *win;
> +
> + /*
> + * select suitable win
> + */
> + win = ov2640_select_win(&mf->width, &mf->height);
> +
> + mf->field = V4L2_FIELD_NONE;
> +
> + switch (mf->code) {
> + case V4L2_MBUS_FMT_RGB565_2X8_LE:
> + mf->colorspace = V4L2_COLORSPACE_SRGB;
> + break;
> + default:
> + mf->code = V4L2_MBUS_FMT_UYVY8_2X8;
> + case V4L2_MBUS_FMT_UYVY8_2X8:
> + mf->colorspace = V4L2_COLORSPACE_JPEG;
> + }
> +
> + return 0;
> +}
> +
> +static int ov2640_enum_fmt(struct v4l2_subdev *sd, unsigned int index,
> + enum v4l2_mbus_pixelcode *code)
> +{
> + if (index >= ARRAY_SIZE(ov2640_codes))
> + return -EINVAL;
> +
> + *code = ov2640_codes[index];
> + return 0;
> +}
> +
> +static int ov2640_video_probe(struct soc_camera_device *icd,
> + struct i2c_client *client)
> +{
> + struct ov2640_priv *priv = to_ov2640(client);
> + u8 pid, ver, midh, midl;
> + const char *devname;
> + int ret;
> +
> + /*
> + * We must have a parent by now. And it cannot be a wrong one.
> + * So this entire test is completely redundant.
> + */
> + if (!icd->dev.parent ||
> + to_soc_camera_host(icd->dev.parent)->nr != icd->iface) {
> + dev_err(&client->dev, "Parent missing or invalid!\n");
> + ret = -ENODEV;
> + goto err;
> + }
> +
> + /*
> + * check and show product ID and manufacturer ID
> + */
> + i2c_smbus_write_byte_data(client, BANK_SEL, BANK_SEL_SENS);
> + pid = i2c_smbus_read_byte_data(client, PID);
> + ver = i2c_smbus_read_byte_data(client, VER);
> + midh = i2c_smbus_read_byte_data(client, MIDH);
> + midl = i2c_smbus_read_byte_data(client, MIDL);
> +
> + switch (VERSION(pid, ver)) {
> + case PID_OV2640:
> + devname = "ov2640";
> + priv->model = V4L2_IDENT_OV2640;
> + break;
> + default:
> + dev_err(&client->dev,
> + "Product ID error %x:%x\n", pid, ver);
> + ret = -ENODEV;
> + goto err;
> + }
> +
> + dev_info(&client->dev,
> + "%s Product ID %0x:%0x Manufacturer ID %x:%x\n",
> + devname, pid, ver, midh, midl);
> +
> + return 0;
> +
> +err:
> + return ret;
> +}
> +
> +static struct soc_camera_ops ov2640_ops = {
> + .set_bus_param = ov2640_set_bus_param,
> + .query_bus_param = ov2640_query_bus_param,
> + .controls = ov2640_controls,
> + .num_controls = ARRAY_SIZE(ov2640_controls),
> +};
> +
> +static struct v4l2_subdev_core_ops ov2640_subdev_core_ops = {
> + .g_ctrl = ov2640_g_ctrl,
> + .s_ctrl = ov2640_s_ctrl,
> + .g_chip_ident = ov2640_g_chip_ident,
> +#ifdef CONFIG_VIDEO_ADV_DEBUG
> + .g_register = ov2640_g_register,
> + .s_register = ov2640_s_register,
> +#endif
> +};
> +
> +static struct v4l2_subdev_video_ops ov2640_subdev_video_ops = {
> + .s_stream = ov2640_s_stream,
> + .g_mbus_fmt = ov2640_g_fmt,
> + .s_mbus_fmt = ov2640_s_fmt,
> + .try_mbus_fmt = ov2640_try_fmt,
> + .enum_mbus_fmt = ov2640_enum_fmt,

Please, also implement at least a .cropcap, maybe also .g_crop - both
trivial for your case of a constant sensor window.

> +};
> +
> +static struct v4l2_subdev_ops ov2640_subdev_ops = {
> + .core = &ov2640_subdev_core_ops,
> + .video = &ov2640_subdev_video_ops,
> +};
> +
> +/*
> + * i2c_driver functions
> + */
> +static int ov2640_probe(struct i2c_client *client,
> + const struct i2c_device_id *did)
> +{
> + struct ov2640_priv *priv;
> + struct soc_camera_device *icd = client->dev.platform_data;
> + struct i2c_adapter *adapter =
> to_i2c_adapter(client->dev.parent);
> + struct soc_camera_link *icl;
> + int ret;
> +
> + if (!icd) {
> + dev_err(&adapter->dev, "OV2640: missing soc-camera data!\n");
> + return -EINVAL;
> + }
> +
> + icl = to_soc_camera_link(icd);
> + if (!icl) {
> + dev_err(&adapter->dev,
> + "OV2640: Missing platform_data for driver\n");
> + return -EINVAL;
> + }
> +
> + if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
> + dev_err(&adapter->dev,
> + "OV2640: I2C-Adapter doesn't support SMBUS\n");
> + return -EIO;
> + }
> +
> + priv = kzalloc(sizeof(struct ov2640_priv), GFP_KERNEL);
> + if (!priv) {
> + dev_err(&adapter->dev,
> + "Failed to allocate memory for private data!\n");
> + return -ENOMEM;
> + }
> +
> + priv->info = icl->priv;
> +
> + v4l2_i2c_subdev_init(&priv->subdev, client, &ov2640_subdev_ops);
> +
> + icd->ops = &ov2640_ops;
> +
> + ret = ov2640_video_probe(icd, client);
> + if (ret) {
> + icd->ops = NULL;
> + kfree(priv);
> + } else {
> + dev_info(&adapter->dev, "OV2640 Probed\n");
> + }
> +
> + return ret;
> +}
> +
> +static int ov2640_remove(struct i2c_client *client)
> +{
> + struct ov2640_priv *priv = to_ov2640(client);
> + struct soc_camera_device *icd = client->dev.platform_data;
> +
> + icd->ops = NULL;
> + kfree(priv);
> + return 0;
> +}
> +
> +static const struct i2c_device_id ov2640_id[] = {
> + { "ov2640", 0 },
> + { }
> +};
> +MODULE_DEVICE_TABLE(i2c, ov2640_id);
> +
> +static struct i2c_driver ov2640_i2c_driver = {
> + .driver = {
> + .name = "ov2640",
> + },
> + .probe = ov2640_probe,
> + .remove = ov2640_remove,
> + .id_table = ov2640_id,
> +};
> +
> +/*
> + * module functions
> + */
> +static int __init ov2640_module_init(void)
> +{
> + return i2c_add_driver(&ov2640_i2c_driver);
> +}
> +
> +static void __exit ov2640_module_exit(void)
> +{
> + i2c_del_driver(&ov2640_i2c_driver);
> +}
> +
> +module_init(ov2640_module_init);
> +module_exit(ov2640_module_exit);
> +
> +MODULE_DESCRIPTION("SoC Camera driver for Omni Vision 2640 sensor");
> +MODULE_AUTHOR("Alberto Panizzo");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/media/v4l2-chip-ident.h
> b/include/media/v4l2-chip-ident.h
> index 51e89f2..44fe44e 100644
> --- a/include/media/v4l2-chip-ident.h
> +++ b/include/media/v4l2-chip-ident.h
> @@ -74,6 +74,7 @@ enum {
> V4L2_IDENT_SOI968 = 256,
> V4L2_IDENT_OV9640 = 257,
> V4L2_IDENT_OV6650 = 258,
> + V4L2_IDENT_OV2640 = 259,
>
> /* module saa7146: reserved range 300-309 */
> V4L2_IDENT_SAA7146 = 300,
> --
> 1.6.3.3

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/