Re: [PATCH 3/5] media: ov7670: calculate framerate properly for ov7675.
On 26 September 2012 18:50, Jonathan Corbet cor...@lwn.net wrote: On Wed, 26 Sep 2012 11:47:55 +0200 Javier Martin javier.mar...@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 -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/5] media: ov7670: calculate framerate properly for ov7675.
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.mar...@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_BYPASS0x00/* Bypass PLL */
+#define DBLV_X40x01/* clock x4 */
+#define DBLV_X60x10/* clock x6 */
+#define DBLV_X80x11/* clock x8 */
+
#define REG_REG76 0x76/* OV's name */
#define R76_BLKPCOR0x80/* Black pixel correction enable */
#define R76_WHTPCOR0x40/* 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;
-
Re: [PATCH 3/5] media: ov7670: calculate framerate properly for ov7675.
On Wed, 26 Sep 2012 11:47:55 +0200 Javier Martin javier.mar...@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 -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
