[PATCH 0/5] media: ov7670: driver cleanup and support for ov7674.

[PATCH 0/5] media: ov7670: driver cleanup and support for ov7674.

[Javier Martin]

The following series includes all the changes discussed in [1] that
don't affect either bridge drivers that use ov7670 or soc-camera framework
For this reason they are considered non controversial and sent separately.
At least 1 more series will follow in order to implement all features
described in [1].



[1] http://www.mail-archive.com/linux-media@vger.kernel.org/msg51778.html

[PATCH 1/5] media: ov7670: add support for ov7675.

[Javier Martin]

ov7675 and ov7670 share the same registers but there is no way
to distinguish them at runtime. However, they require different
tweaks to achieve the desired resolution. For this reason this
patch adds a new ov7675 entry to the ov7670_id table.

Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
---
 drivers/media/i2c/ov7670.c |  164 ++++++++++++++++++++++++++++++--------------
 1 file changed, 111 insertions(+), 53 deletions(-)

diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c
index e7c82b2..0478a7b 100644
--- a/drivers/media/i2c/ov7670.c
+++ b/drivers/media/i2c/ov7670.c
@@ -183,6 +183,10 @@ MODULE_PARM_DESC(debug, "Debug level (0-1)");
 #define REG_HAECC7	0xaa	/* Hist AEC/AGC control 7 */
 #define REG_BD60MAX	0xab	/* 60hz banding step limit */
 
+enum ov7670_model {
+	MODEL_OV7670 = 0,
+	MODEL_OV7675,
+};
 
 /*
  * Information we maintain about a known sensor.
@@ -198,6 +202,7 @@ struct ov7670_info {
 	int clock_speed;		/* External clock speed (MHz) */
 	u8 clkrc;			/* Clock divider value */
 	bool use_smbus;			/* Use smbus I/O instead of I2C */
+	enum ov7670_model model;
 };
 
 static inline struct ov7670_info *to_state(struct v4l2_subdev *sd)
@@ -652,7 +657,7 @@ static struct regval_list ov7670_qcif_regs[] = {
 	{ 0xff, 0xff },
 };
 
-static struct ov7670_win_size {
+struct ov7670_win_size {
 	int	width;
 	int	height;
 	unsigned char com7_bit;
@@ -661,57 +666,105 @@ static struct ov7670_win_size {
 	int	vstart;		/* sense to humans, but evidently the sensor */
 	int	vstop;		/* will do the right thing... */
 	struct regval_list *regs; /* Regs to tweak */
-/* h/vref stuff */
-} ov7670_win_sizes[] = {
-	/* VGA */
-	{
-		.width		= VGA_WIDTH,
-		.height		= VGA_HEIGHT,
-		.com7_bit	= COM7_FMT_VGA,
-		.hstart		= 158,		/* These values from */
-		.hstop		=  14,		/* Omnivision */
-		.vstart		=  10,
-		.vstop		= 490,
-		.regs 		= NULL,
-	},
-	/* CIF */
-	{
-		.width		= CIF_WIDTH,
-		.height		= CIF_HEIGHT,
-		.com7_bit	= COM7_FMT_CIF,
-		.hstart		= 170,		/* Empirically determined */
-		.hstop		=  90,
-		.vstart		=  14,
-		.vstop		= 494,
-		.regs 		= NULL,
-	},
-	/* QVGA */
+};
+
+static struct ov7670_win_size ov7670_win_sizes[2][4] = {
+	/* ov7670 */
 	{
-		.width		= QVGA_WIDTH,
-		.height		= QVGA_HEIGHT,
-		.com7_bit	= COM7_FMT_QVGA,
-		.hstart		= 168,		/* Empirically determined */
-		.hstop		=  24,
-		.vstart		=  12,
-		.vstop		= 492,
-		.regs 		= NULL,
+		/* VGA */
+		{
+			.width		= VGA_WIDTH,
+			.height		= VGA_HEIGHT,
+			.com7_bit	= COM7_FMT_VGA,
+			.hstart		= 158,	/* These values from */
+			.hstop		=  14,	/* Omnivision */
+			.vstart		=  10,
+			.vstop		= 490,
+			.regs		= NULL,
+		},
+		/* CIF */
+		{
+			.width		= CIF_WIDTH,
+			.height		= CIF_HEIGHT,
+			.com7_bit	= COM7_FMT_CIF,
+			.hstart		= 170,	/* Empirically determined */
+			.hstop		=  90,
+			.vstart		=  14,
+			.vstop		= 494,
+			.regs		= NULL,
+		},
+		/* QVGA */
+		{
+			.width		= QVGA_WIDTH,
+			.height		= QVGA_HEIGHT,
+			.com7_bit	= COM7_FMT_QVGA,
+			.hstart		= 168,	/* Empirically determined */
+			.hstop		=  24,
+			.vstart		=  12,
+			.vstop		= 492,
+			.regs		= NULL,
+		},
+		/* QCIF */
+		{
+			.width		= QCIF_WIDTH,
+			.height		= QCIF_HEIGHT,
+			.com7_bit	= COM7_FMT_VGA, /* see comment above */
+			.hstart		= 456,	/* Empirically determined */
+			.hstop		=  24,
+			.vstart		=  14,
+			.vstop		= 494,
+			.regs 		= ov7670_qcif_regs,
+		}
 	},
-	/* QCIF */
+	/* ov7675 */
 	{
-		.width		= QCIF_WIDTH,
-		.height		= QCIF_HEIGHT,
-		.com7_bit	= COM7_FMT_VGA, /* see comment above */
-		.hstart		= 456,		/* Empirically determined */
-		.hstop		=  24,
-		.vstart		=  14,
-		.vstop		= 494,
-		.regs 		= ov7670_qcif_regs,
-	},
+		/* VGA */
+		{
+			.width		= VGA_WIDTH,
+			.height		= VGA_HEIGHT,
+			.com7_bit	= COM7_FMT_VGA,
+			.hstart		= 158,	/* These values from */
+			.hstop		=  14,	/* Omnivision */
+			.vstart		=  14,
+			.vstop		= 494,
+			.regs		= NULL,
+		},
+		/* CIF - WARNING: not tested for ov7675 */
+		{
+			.width		= CIF_WIDTH,
+			.height		= CIF_HEIGHT,
+			.com7_bit	= COM7_FMT_CIF,
+			.hstart		= 170,	/* Empirically determined */
+			.hstop		=  90,
+			.vstart		=  14,
+			.vstop		= 494,
+			.regs		= NULL,
+		},
+		/* QVGA - WARNING: not tested for ov7675 */
+		{
+			.width		= QVGA_WIDTH,
+			.height		= QVGA_HEIGHT,
+			.com7_bit	= COM7_FMT_QVGA,
+			.hstart		= 168,	/* Empirically determined */
+			.hstop		=  24,
+			.vstart		=  12,
+			.vstop		= 492,
+			.regs		= NULL,
+		},
+		/* QCIF - WARNING: not tested for ov7675 */
+		{
+			.width		= QCIF_WIDTH,
+			.height		= QCIF_HEIGHT,
+			.com7_bit	= COM7_FMT_VGA, /* see comment above */
+			.hstart		= 456,	/* Empirically determined */
+			.hstop		=  24,
+			.vstart		=  14,
+			.vstop		= 494,
+			.regs		= ov7670_qcif_regs,
+		}
+	}
 };
 
-#define N_WIN_SIZES (ARRAY_SIZE(ov7670_win_sizes))
-
-
 /*
  * Store a set of start/stop values into the camera.
  */
@@ -761,6 +814,8 @@ static int ov7670_try_fmt_internal(struct v4l2_subdev *sd,
 {
 	int index;
 	struct ov7670_win_size *wsize;
+	struct ov7670_info *info = to_state(sd);
+	int n_win_sizes = ARRAY_SIZE(ov7670_win_sizes[info->model]);
 
 	for (index = 0; index < N_OV7670_FMTS; index++)
 		if (ov7670_formats[index].mbus_code == fmt->code)
@@ -780,11 +835,11 @@ static int ov7670_try_fmt_internal(struct v4l2_subdev *sd,
 	 * Round requested image size down to the nearest
 	 * we support, but not below the smallest.
 	 */
-	for (wsize = ov7670_win_sizes; wsize < ov7670_win_sizes + N_WIN_SIZES;
-	     wsize++)
+	for (wsize = ov7670_win_sizes[info->model];
+	     wsize < ov7670_win_sizes[info->model] + n_win_sizes; wsize++)
 		if (fmt->width >= wsize->width && fmt->height >= wsize->height)
 			break;
-	if (wsize >= ov7670_win_sizes + N_WIN_SIZES)
+	if (wsize >= ov7670_win_sizes[info->model] + n_win_sizes)
 		wsize--;   /* Take the smallest one */
 	if (ret_wsize != NULL)
 		*ret_wsize = wsize;
@@ -931,13 +986,14 @@ static int ov7670_enum_framesizes(struct v4l2_subdev *sd,
 	int i;
 	int num_valid = -1;
 	__u32 index = fsize->index;
+	int n_win_sizes = ARRAY_SIZE(ov7670_win_sizes[info->model]);
 
 	/*
 	 * If a minimum width/height was requested, filter out the capture
 	 * windows that fall outside that.
 	 */
-	for (i = 0; i < N_WIN_SIZES; i++) {
-		struct ov7670_win_size *win = &ov7670_win_sizes[index];
+	for (i = 0; i < n_win_sizes; i++) {
+		struct ov7670_win_size *win = &ov7670_win_sizes[info->model][index];
 		if (info->min_width && win->width < info->min_width)
 			continue;
 		if (info->min_height && win->height < info->min_height)
@@ -1551,6 +1607,7 @@ static int ov7670_probe(struct i2c_client *client,
 	v4l_info(client, "chip found @ 0x%02x (%s)\n",
 			client->addr << 1, client->adapter->name);
 
+	info->model = id->driver_data;
 	info->fmt = &ov7670_formats[0];
 	info->sat = 128;	/* Review this */
 	info->clkrc = info->clock_speed / 30;
@@ -1568,7 +1625,8 @@ static int ov7670_remove(struct i2c_client *client)
 }
 
 static const struct i2c_device_id ov7670_id[] = {
-	{ "ov7670", 0 },
+	{ "ov7670", MODEL_OV7670 },
+	{ "ov7675", MODEL_OV7675 },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, ov7670_id);
-- 
1.7.9.5

[PATCH 2/5] media: ov7670: make try_fmt() consistent with 'min_height' and 'min_width'.

[Javier Martin]

'min_height' and 'min_width' are variables that allow to specify the minimum
resolution that the sensor will achieve. This patch make v4l2 fmt callbacks
consider this parameters in order to return valid data to user space.

Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
---
 drivers/media/i2c/ov7670.c |   22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c
index 0478a7b..627fe5f 100644
--- a/drivers/media/i2c/ov7670.c
+++ b/drivers/media/i2c/ov7670.c
@@ -812,10 +812,11 @@ static int ov7670_try_fmt_internal(struct v4l2_subdev *sd,
 		struct ov7670_format_struct **ret_fmt,
 		struct ov7670_win_size **ret_wsize)
 {
-	int index;
+	int index, i;
 	struct ov7670_win_size *wsize;
 	struct ov7670_info *info = to_state(sd);
 	int n_win_sizes = ARRAY_SIZE(ov7670_win_sizes[info->model]);
+	int win_sizes_limit = n_win_sizes;
 
 	for (index = 0; index < N_OV7670_FMTS; index++)
 		if (ov7670_formats[index].mbus_code == fmt->code)
@@ -831,15 +832,30 @@ static int ov7670_try_fmt_internal(struct v4l2_subdev *sd,
 	 * Fields: the OV devices claim to be progressive.
 	 */
 	fmt->field = V4L2_FIELD_NONE;
+
+	/*
+	 * Don't consider values that don't match min_height and min_width
+	 * constraints.
+	 */
+	if (info->min_width || info->min_height)
+		for (i = 0; i < n_win_sizes; i++) {
+			wsize = ov7670_win_sizes[info->model] + i;
+
+			if (wsize->width < info->min_width ||
+				wsize->height < info->min_height) {
+				win_sizes_limit = i;
+				break;
+			}
+		}
 	/*
 	 * Round requested image size down to the nearest
 	 * we support, but not below the smallest.
 	 */
 	for (wsize = ov7670_win_sizes[info->model];
-	     wsize < ov7670_win_sizes[info->model] + n_win_sizes; wsize++)
+	     wsize < ov7670_win_sizes[info->model] + win_sizes_limit; wsize++)
 		if (fmt->width >= wsize->width && fmt->height >= wsize->height)
 			break;
-	if (wsize >= ov7670_win_sizes[info->model] + n_win_sizes)
+	if (wsize >= ov7670_win_sizes[info->model] + win_sizes_limit)
 		wsize--;   /* Take the smallest one */
 	if (ret_wsize != NULL)
 		*ret_wsize = wsize;
-- 
1.7.9.5

[PATCH 3/5] media: ov7670: calculate framerate properly for ov7675.

[Javier Martin]

According to the datasheet ov7675 uses a formula to achieve
the desired framerate that is different from the operations
done in the current code.

In fact, this formula should apply to ov7670 too. This would
mean that current code is wrong but, in order to preserve
compatibility, the new formula will be used for ov7675 only.

Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
---
 drivers/media/i2c/ov7670.c |  122 ++++++++++++++++++++++++++++++++++++++------
 1 file changed, 105 insertions(+), 17 deletions(-)

diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c
index 627fe5f..175fbfc 100644
--- a/drivers/media/i2c/ov7670.c
+++ b/drivers/media/i2c/ov7670.c
@@ -47,6 +47,8 @@ MODULE_PARM_DESC(debug, "Debug level (0-1)");
  */
 #define OV7670_I2C_ADDR 0x42
 
+#define PLL_FACTOR	4
+
 /* Registers */
 #define REG_GAIN	0x00	/* Gain lower 8 bits (rest in vref) */
 #define REG_BLUE	0x01	/* blue gain */
@@ -164,6 +166,12 @@ MODULE_PARM_DESC(debug, "Debug level (0-1)");
 
 #define REG_GFIX	0x69	/* Fix gain control */
 
+#define REG_DBLV	0x6b	/* PLL control an debugging */
+#define   DBLV_BYPASS	  0x00	  /* Bypass PLL */
+#define   DBLV_X4	  0x01	  /* clock x4 */
+#define   DBLV_X6	  0x10	  /* clock x6 */
+#define   DBLV_X8	  0x11	  /* clock x8 */
+
 #define REG_REG76	0x76	/* OV's name */
 #define   R76_BLKPCOR	  0x80	  /* Black pixel correction enable */
 #define   R76_WHTPCOR	  0x40	  /* White pixel correction enable */
@@ -765,6 +773,67 @@ static struct ov7670_win_size ov7670_win_sizes[2][4] = {
 	}
 };
 
+static void ov7670_get_framerate(struct v4l2_subdev *sd,
+				 struct v4l2_fract *tpf)
+{
+	struct ov7670_info *info = to_state(sd);
+	u32 clkrc = info->clkrc;
+	u32 pll_factor = PLL_FACTOR;
+
+	clkrc++;
+	if (info->fmt->mbus_code == V4L2_MBUS_FMT_SBGGR8_1X8)
+		clkrc = (clkrc >> 1);
+
+	tpf->numerator = 1;
+	tpf->denominator = (5 * pll_factor * info->clock_speed) /
+			(4 * clkrc);
+}
+
+static int ov7670_set_framerate(struct v4l2_subdev *sd,
+				 struct v4l2_fract *tpf)
+{
+	struct ov7670_info *info = to_state(sd);
+	u32 clkrc;
+	u32 pll_factor = PLL_FACTOR;
+	int ret;
+
+	/*
+	 * The formula is fps = 5/4*pixclk for YUV/RGB and
+	 * fps = 5/2*pixclk for RAW.
+	 *
+	 * pixclk = clock_speed / (clkrc + 1) * PLLfactor
+	 *
+	 */
+	if (tpf->numerator == 0 || tpf->denominator == 0) {
+		clkrc = 0;
+	} else {
+		clkrc = (5 * pll_factor * info->clock_speed * tpf->numerator) /
+			(4 * tpf->denominator);
+		if (info->fmt->mbus_code == V4L2_MBUS_FMT_SBGGR8_1X8)
+			clkrc = (clkrc << 1);
+		clkrc--;
+	}
+
+	/*
+	 * The datasheet claims that clkrc = 0 will divide the input clock by 1
+	 * but we've checked with an oscilloscope that it divides by 2 instead.
+	 * So, if clkrc = 0 just bypass the divider.
+	 */
+	if (clkrc <= 0)
+		clkrc = CLK_EXT;
+	else if (clkrc > CLK_SCALE)
+		clkrc = CLK_SCALE;
+	info->clkrc = clkrc;
+
+	/* Recalculate frame rate */
+	ov7670_get_framerate(sd, tpf);
+
+	ret = ov7670_write(sd, REG_CLKRC, info->clkrc);
+	if (ret < 0)
+		return ret;
+	return ov7670_write(sd, REG_DBLV, DBLV_X4);
+}
+
 /*
  * Store a set of start/stop values into the camera.
  */
@@ -939,10 +1008,15 @@ static int ov7670_g_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *parms)
 
 	memset(cp, 0, sizeof(struct v4l2_captureparm));
 	cp->capability = V4L2_CAP_TIMEPERFRAME;
-	cp->timeperframe.numerator = 1;
-	cp->timeperframe.denominator = info->clock_speed;
-	if ((info->clkrc & CLK_EXT) == 0 && (info->clkrc & CLK_SCALE) > 1)
-		cp->timeperframe.denominator /= (info->clkrc & CLK_SCALE);
+	if (info->model == MODEL_OV7670) {
+		/* legacy */
+		cp->timeperframe.numerator = 1;
+		cp->timeperframe.denominator = info->clock_speed;
+		if ((info->clkrc & CLK_EXT) == 0 && (info->clkrc & CLK_SCALE) > 1)
+			cp->timeperframe.denominator /= (info->clkrc & CLK_SCALE);
+	} else {
+		ov7670_get_framerate(sd, &cp->timeperframe);
+	}
 	return 0;
 }
 
@@ -958,18 +1032,23 @@ static int ov7670_s_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *parms)
 	if (cp->extendedmode != 0)
 		return -EINVAL;
 
-	if (tpf->numerator == 0 || tpf->denominator == 0)
-		div = 1;  /* Reset to full rate */
-	else
-		div = (tpf->numerator * info->clock_speed) / tpf->denominator;
-	if (div == 0)
-		div = 1;
-	else if (div > CLK_SCALE)
-		div = CLK_SCALE;
-	info->clkrc = (info->clkrc & 0x80) | div;
-	tpf->numerator = 1;
-	tpf->denominator = info->clock_speed / div;
-	return ov7670_write(sd, REG_CLKRC, info->clkrc);
+	if (info->model == MODEL_OV7670) {
+		/* legacy */
+		if (tpf->numerator == 0 || tpf->denominator == 0)
+			div = 1;  /* Reset to full rate */
+		else
+			div = (tpf->numerator * info->clock_speed) / tpf->denominator;
+		if (div == 0)
+			div = 1;
+		else if (div > CLK_SCALE)
+			div = CLK_SCALE;
+		info->clkrc = (info->clkrc & 0x80) | div;
+		tpf->numerator = 1;
+		tpf->denominator = info->clock_speed / div;
+		return ov7670_write(sd, REG_CLKRC, info->clkrc);
+	} else {
+		return ov7670_set_framerate(sd, tpf);
+	}
 }
 
 
@@ -1585,6 +1664,7 @@ static const struct v4l2_subdev_ops ov7670_ops = {
 static int ov7670_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
+	struct v4l2_fract tpf;
 	struct v4l2_subdev *sd;
 	struct ov7670_info *info;
 	int ret;
@@ -1626,7 +1706,15 @@ static int ov7670_probe(struct i2c_client *client,
 	info->model = id->driver_data;
 	info->fmt = &ov7670_formats[0];
 	info->sat = 128;	/* Review this */
-	info->clkrc = info->clock_speed / 30;
+	/* Set default frame rate to 30 fps */
+	if (info->model == MODEL_OV7670) {
+		/* legacy */
+		info->clkrc = info->clock_speed / 30;
+	} else {
+		tpf.numerator = 1;
+		tpf.denominator = 30;
+		ov7670_set_framerate(sd, &tpf);
+	}
 	return 0;
 }
 
-- 
1.7.9.5

[PATCH 4/5] media: ov7670: add possibility to bypass pll for ov7675.

[Javier Martin]

Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
---
 drivers/media/i2c/ov7670.c |   24 ++++++++++++++++++++++--
 include/media/ov7670.h     |    1 +
 2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c
index 175fbfc..54fb535 100644
--- a/drivers/media/i2c/ov7670.c
+++ b/drivers/media/i2c/ov7670.c
@@ -210,6 +210,7 @@ struct ov7670_info {
 	int clock_speed;		/* External clock speed (MHz) */
 	u8 clkrc;			/* Clock divider value */
 	bool use_smbus;			/* Use smbus I/O instead of I2C */
+	bool pll_bypass;
 	enum ov7670_model model;
 };
 
@@ -778,7 +779,12 @@ static void ov7670_get_framerate(struct v4l2_subdev *sd,
 {
 	struct ov7670_info *info = to_state(sd);
 	u32 clkrc = info->clkrc;
-	u32 pll_factor = PLL_FACTOR;
+	int pll_factor;
+
+	if (info->pll_bypass)
+		pll_factor = 1;
+	else
+		pll_factor = PLL_FACTOR;
 
 	clkrc++;
 	if (info->fmt->mbus_code == V4L2_MBUS_FMT_SBGGR8_1X8)
@@ -794,7 +800,7 @@ static int ov7670_set_framerate(struct v4l2_subdev *sd,
 {
 	struct ov7670_info *info = to_state(sd);
 	u32 clkrc;
-	u32 pll_factor = PLL_FACTOR;
+	int pll_factor;
 	int ret;
 
 	/*
@@ -804,6 +810,16 @@ static int ov7670_set_framerate(struct v4l2_subdev *sd,
 	 * pixclk = clock_speed / (clkrc + 1) * PLLfactor
 	 *
 	 */
+	if (info->pll_bypass) {
+		pll_factor = 1;
+		ret = ov7670_write(sd, REG_DBLV, DBLV_BYPASS);
+	} else {
+		pll_factor = PLL_FACTOR;
+		ret = ov7670_write(sd, REG_DBLV, DBLV_X4);
+	}
+	if (ret < 0)
+		return ret;
+
 	if (tpf->numerator == 0 || tpf->denominator == 0) {
 		clkrc = 0;
 	} else {
@@ -831,6 +847,7 @@ static int ov7670_set_framerate(struct v4l2_subdev *sd,
 	ret = ov7670_write(sd, REG_CLKRC, info->clkrc);
 	if (ret < 0)
 		return ret;
+
 	return ov7670_write(sd, REG_DBLV, DBLV_X4);
 }
 
@@ -1689,6 +1706,9 @@ static int ov7670_probe(struct i2c_client *client,
 
 		if (config->clock_speed)
 			info->clock_speed = config->clock_speed;
+
+		if (config->pll_bypass && id->driver_data != MODEL_OV7670)
+			info->pll_bypass = true;
 	}
 
 	/* Make sure it's an ov7670 */
diff --git a/include/media/ov7670.h b/include/media/ov7670.h
index b133bc1..a68c8bb 100644
--- a/include/media/ov7670.h
+++ b/include/media/ov7670.h
@@ -15,6 +15,7 @@ struct ov7670_config {
 	int min_height;			/* Filter out smaller sizes */
 	int clock_speed;		/* External clock speed (MHz) */
 	bool use_smbus;			/* Use smbus I/O instead of I2C */
+	bool pll_bypass;		/* Choose whether to bypass the PLL */
 };
 
 #endif
-- 
1.7.9.5

[PATCH 5/5] media: ov7670: Add possibility to disable pixclk during hblank.

[Javier Martin]

Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
---
 drivers/media/i2c/ov7670.c |    8 ++++++++
 include/media/ov7670.h     |    1 +
 2 files changed, 9 insertions(+)

diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c
index 54fb535..f7e4341 100644
--- a/drivers/media/i2c/ov7670.c
+++ b/drivers/media/i2c/ov7670.c
@@ -211,6 +211,7 @@ struct ov7670_info {
 	u8 clkrc;			/* Clock divider value */
 	bool use_smbus;			/* Use smbus I/O instead of I2C */
 	bool pll_bypass;
+	bool pclk_hb_disable;
 	enum ov7670_model model;
 };
 
@@ -1709,6 +1710,9 @@ static int ov7670_probe(struct i2c_client *client,
 
 		if (config->pll_bypass && id->driver_data != MODEL_OV7670)
 			info->pll_bypass = true;
+
+		if (config->pclk_hb_disable)
+			info->pclk_hb_disable = true;
 	}
 
 	/* Make sure it's an ov7670 */
@@ -1735,6 +1739,10 @@ static int ov7670_probe(struct i2c_client *client,
 		tpf.denominator = 30;
 		ov7670_set_framerate(sd, &tpf);
 	}
+
+	if (info->pclk_hb_disable)
+		ov7670_write(sd, REG_COM10, COM10_PCLK_HB);
+
 	return 0;
 }
 
diff --git a/include/media/ov7670.h b/include/media/ov7670.h
index a68c8bb..1913d51 100644
--- a/include/media/ov7670.h
+++ b/include/media/ov7670.h
@@ -16,6 +16,7 @@ struct ov7670_config {
 	int clock_speed;		/* External clock speed (MHz) */
 	bool use_smbus;			/* Use smbus I/O instead of I2C */
 	bool pll_bypass;		/* Choose whether to bypass the PLL */
+	bool pclk_hb_disable;		/* Disable toggling pixclk during horizontal blanking */
 };
 
 #endif
-- 
1.7.9.5

Re: [PATCH 0/5] media: ov7670: driver cleanup and support for ov7674.

[javier Martin]

On 26 September 2012 11:47, Javier Martin
<javier.martin@vista-silicon.com> wrote:
> The following series includes all the changes discussed in [1] that
> don't affect either bridge drivers that use ov7670 or soc-camera framework
> For this reason they are considered non controversial and sent separately.
> At least 1 more series will follow in order to implement all features
> described in [1].
>
>
>
> [1] http://www.mail-archive.com/linux-media@vger.kernel.org/msg51778.html

Support is for ov7675, not ov7674, sorry for the typo.

-- 
Javier Martin
Vista Silicon S.L.
CDTUC - FASE C - Oficina S-345
Avda de los Castros s/n
39005- Santander. Cantabria. Spain
+34 942 25 32 60
www.vista-silicon.com

Re: [PATCH 1/5] media: ov7670: add support for ov7675.

[Jonathan Corbet]

This is going to have to be quick, sorry...

On Wed, 26 Sep 2012 11:47:53 +0200
Javier Martin <javier.martin@vista-silicon.com> wrote:

> +static struct ov7670_win_size ov7670_win_sizes[2][4] = {
> +	/* ov7670 */

I must confess I don't like this; now we've got constants in an array that
was automatically sized before and ov7670_win_sizes[info->model]
everywhere.  I'd suggest a separate array for each device and an
ov7670_get_wsizes(model) function.

> +		/* CIF - WARNING: not tested for ov7675 */
> +		{

...and this is part of why I don't like it.  My experience with this
particular sensor says that, if it's not tested, it hasn't yet seen the
magic-number tweaking required to actually make it work.  Please don't
claim to support formats that you don't know actually work, or I'll get
stuck with the bug reports :)

> +			.width		= CIF_WIDTH,
> +			.height		= CIF_HEIGHT,
> +			.com7_bit	= COM7_FMT_CIF,
> +			.hstart		= 170,	/* Empirically determined */
> +			.hstop		=  90,
> +			.vstart		=  14,
> +			.vstop		= 494,
> +			.regs		= NULL,
> +		},

jon

Re: [PATCH 2/5] media: ov7670: make try_fmt() consistent with 'min_height' and 'min_width'.

[Jonathan Corbet]

On Wed, 26 Sep 2012 11:47:54 +0200
Javier Martin <javier.martin@vista-silicon.com> wrote:

> 'min_height' and 'min_width' are variables that allow to specify the minimum
> resolution that the sensor will achieve. This patch make v4l2 fmt callbacks
> consider this parameters in order to return valid data to user space.
> 
I'd have preferred to do this differently, perhaps backing toward larger
sizes if the minimum turns out to be violated.  But so be it...

Acked-by: Jonathan Corbet <corbet@lwn.net>

jon

Re: [PATCH 3/5] media: ov7670: calculate framerate properly for ov7675.

[Jonathan Corbet]

On Wed, 26 Sep 2012 11:47:55 +0200
Javier Martin <javier.martin@vista-silicon.com> wrote:

> According to the datasheet ov7675 uses a formula to achieve
> the desired framerate that is different from the operations
> done in the current code.
> 
> In fact, this formula should apply to ov7670 too. This would
> mean that current code is wrong but, in order to preserve
> compatibility, the new formula will be used for ov7675 only.

At this point I couldn't tell you what the real situation is; it's been a
while and there's always a fair amount of black magic involved with
ov7670 configuration.  I do appreciate attention to not breaking existing
users.

> +static void ov7670_get_framerate(struct v4l2_subdev *sd,
> +				 struct v4l2_fract *tpf)

This bugs me, though.  It's called ov7670_get_framerate() but it's getting
the rate for the ov7675 - confusing.  Meanwhile the real ov7670 code
remains inline while ov7675 has its own function.  

Please make two functions, one of which is ov7675_get_framerate(), and call
the right one for the model.  Same for the "set" functions, obviously.
Maybe what's really needed is a structure full of sensor-specific
operations?  The get_wsizes() function could go there too.  That would take
a lot of if statements out of the code.

> +	/*
> +	 * The datasheet claims that clkrc = 0 will divide the input clock by 1
> +	 * but we've checked with an oscilloscope that it divides by 2 instead.
> +	 * So, if clkrc = 0 just bypass the divider.
> +	 */

Thanks for documenting this kind of thing.

jon

Re: [PATCH 4/5] media: ov7670: add possibility to bypass pll for ov7675.

[Jonathan Corbet]

On Wed, 26 Sep 2012 11:47:56 +0200
Javier Martin <javier.martin@vista-silicon.com> wrote:

> 
> Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>

This one needs a changelog - what does bypassing the PLL do and why might
you want to do it?  Otherwise:

Acked-by: Jonathan Corbet <corbet@lwn.net>

jon

Re: [PATCH 5/5] media: ov7670: Add possibility to disable pixclk during hblank.

[Jonathan Corbet]

On Wed, 26 Sep 2012 11:47:57 +0200
Javier Martin <javier.martin@vista-silicon.com> wrote:

> Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
> ---
>  drivers/media/i2c/ov7670.c |    8 ++++++++
>  include/media/ov7670.h     |    1 +
>  2 files changed, 9 insertions(+)

Again, needs a changelog.  Otherwise

Acked-by: Jonathan Corbet <corbet@lwn.net>

jon

Re: [PATCH 1/5] media: ov7670: add support for ov7675.

[javier Martin]

Hi Jonathan,
thank you for your time.

On 26 September 2012 18:40, Jonathan Corbet <corbet@lwn.net> wrote:
> This is going to have to be quick, sorry...
>
> On Wed, 26 Sep 2012 11:47:53 +0200
> Javier Martin <javier.martin@vista-silicon.com> wrote:
>
>> +static struct ov7670_win_size ov7670_win_sizes[2][4] = {
>> +     /* ov7670 */
>
> I must confess I don't like this; now we've got constants in an array that
> was automatically sized before and ov7670_win_sizes[info->model]
> everywhere.  I'd suggest a separate array for each device and an
> ov7670_get_wsizes(model) function.
>
>> +             /* CIF - WARNING: not tested for ov7675 */
>> +             {
>
> ...and this is part of why I don't like it.  My experience with this
> particular sensor says that, if it's not tested, it hasn't yet seen the
> magic-number tweaking required to actually make it work.  Please don't
> claim to support formats that you don't know actually work, or I'll get
> stuck with the bug reports :)

Your concern makes a lot of sense. In fact, that was one of my doubts
whether to 'support' not tested formats or not.

Let me fix that in a new version.

-- 
Javier Martin
Vista Silicon S.L.
CDTUC - FASE C - Oficina S-345
Avda de los Castros s/n
39005- Santander. Cantabria. Spain
+34 942 25 32 60
www.vista-silicon.com

Re: [PATCH 2/5] media: ov7670: make try_fmt() consistent with 'min_height' and 'min_width'.

[javier Martin]

On 26 September 2012 18:42, Jonathan Corbet <corbet@lwn.net> wrote:
> On Wed, 26 Sep 2012 11:47:54 +0200
> Javier Martin <javier.martin@vista-silicon.com> wrote:
>
>> 'min_height' and 'min_width' are variables that allow to specify the minimum
>> resolution that the sensor will achieve. This patch make v4l2 fmt callbacks
>> consider this parameters in order to return valid data to user space.
>>
> I'd have preferred to do this differently, perhaps backing toward larger
> sizes if the minimum turns out to be violated.  But so be it...
>
> Acked-by: Jonathan Corbet <corbet@lwn.net>
>
> jon

Thank you. I will have to modify this patch slightly when I fix the
previous one though.

-- 
Javier Martin
Vista Silicon S.L.
CDTUC - FASE C - Oficina S-345
Avda de los Castros s/n
39005- Santander. Cantabria. Spain
+34 942 25 32 60
www.vista-silicon.com

Re: [PATCH 3/5] media: ov7670: calculate framerate properly for ov7675.

[javier Martin]

On 26 September 2012 18:50, Jonathan Corbet <corbet@lwn.net> wrote:
> On Wed, 26 Sep 2012 11:47:55 +0200
> Javier Martin <javier.martin@vista-silicon.com> wrote:
>
>> According to the datasheet ov7675 uses a formula to achieve
>> the desired framerate that is different from the operations
>> done in the current code.
>>
>> In fact, this formula should apply to ov7670 too. This would
>> mean that current code is wrong but, in order to preserve
>> compatibility, the new formula will be used for ov7675 only.
>
> At this point I couldn't tell you what the real situation is; it's been a
> while and there's always a fair amount of black magic involved with
> ov7670 configuration.  I do appreciate attention to not breaking existing
> users.

Indeed, this sensor is the quirkier I've dealt with, with those magic
values in non documented registers...

>> +static void ov7670_get_framerate(struct v4l2_subdev *sd,
>> +                              struct v4l2_fract *tpf)
>
> This bugs me, though.  It's called ov7670_get_framerate() but it's getting
> the rate for the ov7675 - confusing.  Meanwhile the real ov7670 code
> remains inline while ov7675 has its own function.

Actually, I did this on purpose because I wanted to remark that this
function should be valid for both models and because I expected that
the old behaviour was removed sometime in the future.

> Please make two functions, one of which is ov7675_get_framerate(), and call
> the right one for the model.  Same for the "set" functions, obviously.
> Maybe what's really needed is a structure full of sensor-specific
> operations?  The get_wsizes() function could go there too.  That would take
> a lot of if statements out of the code.

The idea of a structure of sensor-specific operations seems
reasonable. Furthermore, I think we should encourage users to use the
right formula in the future. For this reason we could define 4
functions

ov7670_set_framerate_legacy()
ov7670_get_framerate_legacy()
ov7675_set_framerate()
ov7675_get_framerate()

>> +     /*
>> +      * The datasheet claims that clkrc = 0 will divide the input clock by 1
>> +      * but we've checked with an oscilloscope that it divides by 2 instead.
>> +      * So, if clkrc = 0 just bypass the divider.
>> +      */
>
> Thanks for documenting this kind of thing.

You are welcome.


-- 
Javier Martin
Vista Silicon S.L.
CDTUC - FASE C - Oficina S-345
Avda de los Castros s/n
39005- Santander. Cantabria. Spain
+34 942 25 32 60
www.vista-silicon.com

Re: [PATCH 4/5] media: ov7670: add possibility to bypass pll for ov7675.

[javier Martin]

On 26 September 2012 18:52, Jonathan Corbet <corbet@lwn.net> wrote:
> On Wed, 26 Sep 2012 11:47:56 +0200
> Javier Martin <javier.martin@vista-silicon.com> wrote:
>
>>
>> Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
>
> This one needs a changelog - what does bypassing the PLL do and why might
> you want to do it?  Otherwise:

As I stated in a previous patch, frame rate depends on the pixclk. Moreover:

pixclk = xvclk / clkrc * PLLfactor

Bypassing the PLL means that the PLL gets out of the way so, in
practice, PLLfactor = 1

pixclk = xvclk / clkrc

For a frame rate of 30 fps a pixclk of 24MHz is needed. Since we have
a clean clock signal we want pixclk = xvclk.

If one applies the formula in ov7670_set_framerate() with PLLfactor =
1 and clock_speed = 24 MHz the resulting clkrc = 1 which means that:
pixclk = xvclk   which is what we want


> Acked-by: Jonathan Corbet <corbet@lwn.net>

Thank you.

I will add a changelog when I send v2 of the series.

Regards.
-- 
Javier Martin
Vista Silicon S.L.
CDTUC - FASE C - Oficina S-345
Avda de los Castros s/n
39005- Santander. Cantabria. Spain
+34 942 25 32 60
www.vista-silicon.com

Re: [PATCH 5/5] media: ov7670: Add possibility to disable pixclk during hblank.

[javier Martin]

On 26 September 2012 18:52, Jonathan Corbet <corbet@lwn.net> wrote:
> On Wed, 26 Sep 2012 11:47:57 +0200
> Javier Martin <javier.martin@vista-silicon.com> wrote:
>
>> Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
>> ---
>>  drivers/media/i2c/ov7670.c |    8 ++++++++
>>  include/media/ov7670.h     |    1 +
>>  2 files changed, 9 insertions(+)
>
> Again, needs a changelog.  Otherwise

Our soc-camera host driver captures pixels during blanking periods if
pixclk is enabled. In order to avoid capturing bogus data we need to
disable pixclk during those blanking periods

I'll add it to v2.

> Acked-by: Jonathan Corbet <corbet@lwn.net>
>

Thank you.



-- 
Javier Martin
Vista Silicon S.L.
CDTUC - FASE C - Oficina S-345
Avda de los Castros s/n
39005- Santander. Cantabria. Spain
+34 942 25 32 60
www.vista-silicon.com

Re: [PATCH 1/5] media: ov7670: add support for ov7675.

[Mauro Carvalho Chehab]

Em Thu, 27 Sep 2012 08:58:33 +0200
javier Martin <javier.martin@vista-silicon.com> escreveu:

> Hi Jonathan,
> thank you for your time.
> 
> On 26 September 2012 18:40, Jonathan Corbet <corbet@lwn.net> wrote:
> > This is going to have to be quick, sorry...
> >
> > On Wed, 26 Sep 2012 11:47:53 +0200
> > Javier Martin <javier.martin@vista-silicon.com> wrote:
> >
> >> +static struct ov7670_win_size ov7670_win_sizes[2][4] = {
> >> +     /* ov7670 */
> >
> > I must confess I don't like this; now we've got constants in an array that
> > was automatically sized before and ov7670_win_sizes[info->model]
> > everywhere.  I'd suggest a separate array for each device and an
> > ov7670_get_wsizes(model) function.
> >
> >> +             /* CIF - WARNING: not tested for ov7675 */
> >> +             {
> >
> > ...and this is part of why I don't like it.  My experience with this
> > particular sensor says that, if it's not tested, it hasn't yet seen the
> > magic-number tweaking required to actually make it work.  Please don't
> > claim to support formats that you don't know actually work, or I'll get
> > stuck with the bug reports :)
> 
> Your concern makes a lot of sense. In fact, that was one of my doubts
> whether to 'support' not tested formats or not.
> 
> Let me fix that in a new version.

Hi Javier,

I'm assuming that you'll be sending a new version of this entire changeset.
So, I'll just mark this entire series as changes_requested.

Cheers,
Mauro

Re: [PATCH 1/5] media: ov7670: add support for ov7675.

[javier Martin]

On 6 October 2012 17:19, Mauro Carvalho Chehab <mchehab@infradead.org> wrote:
> Em Thu, 27 Sep 2012 08:58:33 +0200
> javier Martin <javier.martin@vista-silicon.com> escreveu:
>
>> Hi Jonathan,
>> thank you for your time.
>>
>> On 26 September 2012 18:40, Jonathan Corbet <corbet@lwn.net> wrote:
>> > This is going to have to be quick, sorry...
>> >
>> > On Wed, 26 Sep 2012 11:47:53 +0200
>> > Javier Martin <javier.martin@vista-silicon.com> wrote:
>> >
>> >> +static struct ov7670_win_size ov7670_win_sizes[2][4] = {
>> >> +     /* ov7670 */
>> >
>> > I must confess I don't like this; now we've got constants in an array that
>> > was automatically sized before and ov7670_win_sizes[info->model]
>> > everywhere.  I'd suggest a separate array for each device and an
>> > ov7670_get_wsizes(model) function.
>> >
>> >> +             /* CIF - WARNING: not tested for ov7675 */
>> >> +             {
>> >
>> > ...and this is part of why I don't like it.  My experience with this
>> > particular sensor says that, if it's not tested, it hasn't yet seen the
>> > magic-number tweaking required to actually make it work.  Please don't
>> > claim to support formats that you don't know actually work, or I'll get
>> > stuck with the bug reports :)
>>
>> Your concern makes a lot of sense. In fact, that was one of my doubts
>> whether to 'support' not tested formats or not.
>>
>> Let me fix that in a new version.
>
> Hi Javier,
>
> I'm assuming that you'll be sending a new version of this entire changeset.
> So, I'll just mark this entire series as changes_requested.

Hi Mauro,

v2 of this changeset has already been sent with Jon Corbet's ack:

http://www.mail-archive.com/linux-media@vger.kernel.org/msg52767.html

https://patchwork.kernel.org/patch/1515001/
https://patchwork.kernel.org/patch/1515021/
https://patchwork.kernel.org/patch/1515011/
https://patchwork.kernel.org/patch/1515031/
https://patchwork.kernel.org/patch/1515041/

Regards.

-- 
Javier Martin
Vista Silicon S.L.
CDTUC - FASE C - Oficina S-345
Avda de los Castros s/n
39005- Santander. Cantabria. Spain
+34 942 25 32 60
www.vista-silicon.com