Improving ov7670 sensor driver.

Improving ov7670 sensor driver.

[javier Martin]

Hi,
our new i.MX27 based platform (Visstrim-SM20) uses an ov7675 sensor
attached to the CSI interface. Apparently, this sensor is fully
compatible with the old ov7670. For this reason, it seems rather
sensible that they should share the same driver: ov7670.c
One of the challenges we have to face is that capture video support
for our platform is mx2_camera.c, which is a soc-camera host driver;
while ov7670.c was developed for being used as part of a more complex
video card.

Here is the list of current users of ov7670:

http://lxr.linux.no/#linux+v3.5.3/drivers/media/video/gspca/ov519.c
http://lxr.linux.no/#linux+v3.5.3/drivers/media/video/gspca/sn9c20x.c
http://lxr.linux.no/#linux+v3.5.3/drivers/media/video/gspca/vc032x.c
http://lxr.linux.no/#linux+v3.5.3/drivers/media/video/via-camera.c
http://lxr.linux.no/#linux+v3.5.3/drivers/media/video/marvell-ccic/mcam-core.c

These are basically the improvements we need to make to this driver in
order to satisfy our needs:

1.- Adapt v4l2 controls to the subvevice control framework, with a
proper ctrl handler, etc...
2.- Add the possibility to bypass PLL and clkrc preescaler.
3.- Adjust vstart/vstop in order to remove an horizontal green line.
4.- Disable pixclk during horizontal blanking.
5.- min_height, min_width should be respected in try_fmt().
6.- Pass platform data when used with a soc-camera host driver.
7.- Add V4L2_CID_POWER_LINE_FREQUENCY ctrl.

I will try to summarize below why we need to accomplish each of the
previous tasks and what solution we propose for them:

1.- Adapt v4l2 controls to the subvevice control framework, with a
proper ctrl handler, etc...

Why? Because soc-camera needs to inherit v4l2 subdevice controls in
order to expose them to user space.
How? Something like the following, incomplete, patch:

---
@@ -190,6 +196,7 @@ MODULE_PARM_DESC(debug, "Debug level (0-1)");
 struct ov7670_format_struct;  /* coming later */
 struct ov7670_info {
        struct v4l2_subdev sd;
+       struct v4l2_ctrl_handler hdl;
        struct ov7670_format_struct *fmt;  /* Current format */
        unsigned char sat;              /* Saturation value */
        int hue;                        /* Hue value */


@@ -1480,10 +1518,14 @@ static int ov7670_s_register(struct
v4l2_subdev *sd, struct v4l2_dbg_register *r

 /* ----------------------------------------------------------------------- */

+static const struct v4l2_ctrl_ops ov7670_ctrl_ops = {
+       .s_ctrl = ov7670_s_ctrl,
+};
+
 static const struct v4l2_subdev_core_ops ov7670_core_ops = {
        .g_chip_ident = ov7670_g_chip_ident,
-       .g_ctrl = ov7670_g_ctrl,
-       .s_ctrl = ov7670_s_ctrl,
+       .g_ctrl = v4l2_subdev_g_ctrl,
+       .s_ctrl = v4l2_subdev_s_ctrl,
        .queryctrl = ov7670_queryctrl,
        .reset = ov7670_reset,
        .init = ov7670_init,

@@ -1551,6 +1600,16 @@ static int ov7670_probe(struct i2c_client *client,
        v4l_info(client, "chip found @ 0x%02x (%s)\n",
                        client->addr << 1, client->adapter->name);

+       v4l2_ctrl_handler_init(&info->hdl, 1);
+       v4l2_ctrl_new_std(&info->hdl, &ov7670_ctrl_ops,
V4L2_CID_VFLIP, 0, 1, 1, 0);
...
...
+       sd->ctrl_handler = &info->hdl;
+       if (info->hdl.error) {
+               v4l2_ctrl_handler_free(&info->hdl);
+               kfree(info);
+               return info->hdl.error;
+       }
+       v4l2_ctrl_handler_setup(&info->hdl);
+
---

2.- Add the possibility to bypass PLL and clkrc preescaler.

Why? The formula to get the desired frame rate in this chip in YUV is
the following: fps = fpclk / (2 * 510 * 784) This means that for a
desired fps = 30 we need fpclk = 24MHz. For that reason we have a
clean 24MHz xvclk input that comes from an oscillator. If we enable
the PLL it internally transforms the 24MHz in 22MHz and thus fps is
not 30 but 27. In order to get 30fps we need to bypass the PLL.
How? Defining a platform flag 'direct_clk' or similar that allows
xvclk being used directly as the pixel clock.

3.- Adjust vstart/vstop in order to remove an horizontal green line.

Why? Currently, in the driver, for VGA, vstart =  10 and vstop = 490.
>From our tests we found out that vstart = 14, vstop = 494 in order to
remove a disgusting horizontal green line in ov7675.
How? It seems these sensor aren't provided with a version register or
anything similar so I can't think of a clean solution for this yet.
Suggestions will be much appreciated.

4.- Disable pixclk during horizontal blanking.

Why? Otherwise i.MX27 will capture wrong pixels during blanking periods.
How? Through a private V4L2 control.

5.- min_height, min_width should be respected in try_fmt().
Why? Otherwise you are telling the user you are going to use a
different size than the one you are going to use.
How? With a patch similar to this:

---
@@ -759,8 +772,10 @@ 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;
+       int win_sizes_limit = N_WIN_SIZES;
        struct ov7670_win_size *wsize;
+       struct ov7670_info *info = to_state(sd);

        for (index = 0; index < N_OV7670_FMTS; index++)
                if (ov7670_formats[index].mbus_code == fmt->code)
@@ -776,15 +791,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 + 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; wsize < ov7670_win_sizes + N_WIN_SIZES;
+       for (wsize = ov7670_win_sizes; wsize < ov7670_win_sizes +
win_sizes_limit;
             wsize++)
                if (fmt->width >= wsize->width && fmt->height >= wsize->height)
                        break;
-       if (wsize >= ov7670_win_sizes + N_WIN_SIZES)
+       if (wsize >= ov7670_win_sizes + win_sizes_limit)
                wsize--;   /* Take the smallest one */
        if (ret_wsize != NULL)
                *ret_wsize = wsize;
---

6.- Pass platform data when used with a soc-camera host driver.
Why? We need to set several platform data like 'min_height',
'min_width' and others.
How? This is an old subject we discussed in January. We agreed that
some soc-camera core changes were needed, but I couldn't find the time
and I think nobody else has addressed it either. Please, correct me if
I am wrong:http://patchwork.linuxtv.org/patch/8860/

7.- Add V4L2_CID_POWER_LINE_FREQUENCY ctrl.
Why? Because the platform will be used in several countries.
How? As long as point 1 is solved this is quite trivial.


The reason of this e-mail is to discuss whether you find these
solution suitable or not and, more important, whether you think the
suggested changes could break existing drivers.

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: Improving ov7670 sensor driver.

[Hans Verkuil]

On Thu 13 September 2012 11:48:17 javier Martin wrote:
> Hi,
> our new i.MX27 based platform (Visstrim-SM20) uses an ov7675 sensor
> attached to the CSI interface. Apparently, this sensor is fully
> compatible with the old ov7670. For this reason, it seems rather
> sensible that they should share the same driver: ov7670.c
> One of the challenges we have to face is that capture video support
> for our platform is mx2_camera.c, which is a soc-camera host driver;
> while ov7670.c was developed for being used as part of a more complex
> video card.
> 
> Here is the list of current users of ov7670:
> 
> http://lxr.linux.no/#linux+v3.5.3/drivers/media/video/gspca/ov519.c
> http://lxr.linux.no/#linux+v3.5.3/drivers/media/video/gspca/sn9c20x.c
> http://lxr.linux.no/#linux+v3.5.3/drivers/media/video/gspca/vc032x.c

These do not actually use the ov7670 driver. They program it themselves.
It would be nice if the gspca driver would get support for subdevs, but
that's a separate topic.

> http://lxr.linux.no/#linux+v3.5.3/drivers/media/video/via-camera.c
> http://lxr.linux.no/#linux+v3.5.3/drivers/media/video/marvell-ccic/mcam-core.c
> 
> These are basically the improvements we need to make to this driver in
> order to satisfy our needs:
> 
> 1.- Adapt v4l2 controls to the subvevice control framework, with a
> proper ctrl handler, etc...
> 2.- Add the possibility to bypass PLL and clkrc preescaler.
> 3.- Adjust vstart/vstop in order to remove an horizontal green line.
> 4.- Disable pixclk during horizontal blanking.
> 5.- min_height, min_width should be respected in try_fmt().
> 6.- Pass platform data when used with a soc-camera host driver.
> 7.- Add V4L2_CID_POWER_LINE_FREQUENCY ctrl.
> 
> I will try to summarize below why we need to accomplish each of the
> previous tasks and what solution we propose for them:
> 
> 1.- Adapt v4l2 controls to the subvevice control framework, with a
> proper ctrl handler, etc...
> 
> Why? Because soc-camera needs to inherit v4l2 subdevice controls in
> order to expose them to user space.
> How? Something like the following, incomplete, patch:

Luckily you didn't do too much work on this. I have old patches for this in
this tree:

http://git.linuxtv.org/hverkuil/media_tree.git/shortlog/refs/heads/cafe-ctrl

The main reason why I never continued with this was that at the time I wrote
this I realized that the control framework needed proper support for what's
now called auto-clusters (i.e. how to handle autofoo/foo controls like autogain
and gain correctly).

I intended to pick this up at some time, but never got around to it.

I think these patches will still apply with some work, but it needs to be
converted to use the autocluster support that's now in the control framework.

> 
> ---
> @@ -190,6 +196,7 @@ MODULE_PARM_DESC(debug, "Debug level (0-1)");
>  struct ov7670_format_struct;  /* coming later */
>  struct ov7670_info {
>         struct v4l2_subdev sd;
> +       struct v4l2_ctrl_handler hdl;
>         struct ov7670_format_struct *fmt;  /* Current format */
>         unsigned char sat;              /* Saturation value */
>         int hue;                        /* Hue value */
> 
> 
> @@ -1480,10 +1518,14 @@ static int ov7670_s_register(struct
> v4l2_subdev *sd, struct v4l2_dbg_register *r
> 
>  /* ----------------------------------------------------------------------- */
> 
> +static const struct v4l2_ctrl_ops ov7670_ctrl_ops = {
> +       .s_ctrl = ov7670_s_ctrl,
> +};
> +
>  static const struct v4l2_subdev_core_ops ov7670_core_ops = {
>         .g_chip_ident = ov7670_g_chip_ident,
> -       .g_ctrl = ov7670_g_ctrl,
> -       .s_ctrl = ov7670_s_ctrl,
> +       .g_ctrl = v4l2_subdev_g_ctrl,
> +       .s_ctrl = v4l2_subdev_s_ctrl,
>         .queryctrl = ov7670_queryctrl,
>         .reset = ov7670_reset,
>         .init = ov7670_init,
> 
> @@ -1551,6 +1600,16 @@ static int ov7670_probe(struct i2c_client *client,
>         v4l_info(client, "chip found @ 0x%02x (%s)\n",
>                         client->addr << 1, client->adapter->name);
> 
> +       v4l2_ctrl_handler_init(&info->hdl, 1);
> +       v4l2_ctrl_new_std(&info->hdl, &ov7670_ctrl_ops,
> V4L2_CID_VFLIP, 0, 1, 1, 0);
> ...
> ...
> +       sd->ctrl_handler = &info->hdl;
> +       if (info->hdl.error) {
> +               v4l2_ctrl_handler_free(&info->hdl);
> +               kfree(info);
> +               return info->hdl.error;
> +       }
> +       v4l2_ctrl_handler_setup(&info->hdl);
> +
> ---
> 
> 2.- Add the possibility to bypass PLL and clkrc preescaler.
> 
> Why? The formula to get the desired frame rate in this chip in YUV is
> the following: fps = fpclk / (2 * 510 * 784) This means that for a
> desired fps = 30 we need fpclk = 24MHz. For that reason we have a
> clean 24MHz xvclk input that comes from an oscillator. If we enable
> the PLL it internally transforms the 24MHz in 22MHz and thus fps is
> not 30 but 27. In order to get 30fps we need to bypass the PLL.
> How? Defining a platform flag 'direct_clk' or similar that allows
> xvclk being used directly as the pixel clock.
> 
> 3.- Adjust vstart/vstop in order to remove an horizontal green line.
> 
> Why? Currently, in the driver, for VGA, vstart =  10 and vstop = 490.
> From our tests we found out that vstart = 14, vstop = 494 in order to
> remove a disgusting horizontal green line in ov7675.
> How? It seems these sensor aren't provided with a version register or
> anything similar so I can't think of a clean solution for this yet.
> Suggestions will be much appreciated.

Using platform_data for this is what springs to mind.

> 4.- Disable pixclk during horizontal blanking.
> 
> Why? Otherwise i.MX27 will capture wrong pixels during blanking periods.
> How? Through a private V4L2 control.

Or platform_data as well?

> 5.- min_height, min_width should be respected in try_fmt().
> Why? Otherwise you are telling the user you are going to use a
> different size than the one you are going to use.
> How? With a patch similar to this:
> 
> ---
> @@ -759,8 +772,10 @@ 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;
> +       int win_sizes_limit = N_WIN_SIZES;
>         struct ov7670_win_size *wsize;
> +       struct ov7670_info *info = to_state(sd);
> 
>         for (index = 0; index < N_OV7670_FMTS; index++)
>                 if (ov7670_formats[index].mbus_code == fmt->code)
> @@ -776,15 +791,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 + 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; wsize < ov7670_win_sizes + N_WIN_SIZES;
> +       for (wsize = ov7670_win_sizes; wsize < ov7670_win_sizes +
> win_sizes_limit;
>              wsize++)
>                 if (fmt->width >= wsize->width && fmt->height >= wsize->height)
>                         break;
> -       if (wsize >= ov7670_win_sizes + N_WIN_SIZES)
> +       if (wsize >= ov7670_win_sizes + win_sizes_limit)
>                 wsize--;   /* Take the smallest one */
>         if (ret_wsize != NULL)
>                 *ret_wsize = wsize;
> ---
> 
> 6.- Pass platform data when used with a soc-camera host driver.
> Why? We need to set several platform data like 'min_height',
> 'min_width' and others.
> How? This is an old subject we discussed in January. We agreed that
> some soc-camera core changes were needed, but I couldn't find the time
> and I think nobody else has addressed it either. Please, correct me if
> I am wrong:http://patchwork.linuxtv.org/patch/8860/
> 
> 7.- Add V4L2_CID_POWER_LINE_FREQUENCY ctrl.
> Why? Because the platform will be used in several countries.
> How? As long as point 1 is solved this is quite trivial.
> 
> 
> The reason of this e-mail is to discuss whether you find these
> solution suitable or not and, more important, whether you think the
> suggested changes could break existing drivers.

Well, those bridge drivers that use the ov7670 subdev should also be converted
to the control framework. My tree does at least some of that work, although I
think some drivers got moved around or were renamed. The changes to those drivers
should be fairly minimal.

In theory it's possible to skip that conversion, but my goal is to get (almost)
all drivers converted to the control framework so this is a good opportunity
to convert these bridge drivers at the same time.

Regards,

	Hans

Re: Improving ov7670 sensor driver.

[javier Martin]

Hi Hans,
thank you for your response.

On 13 September 2012 12:07, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> On Thu 13 September 2012 11:48:17 javier Martin wrote:
>> Hi,
>> our new i.MX27 based platform (Visstrim-SM20) uses an ov7675 sensor
>> attached to the CSI interface. Apparently, this sensor is fully
>> compatible with the old ov7670. For this reason, it seems rather
>> sensible that they should share the same driver: ov7670.c
>> One of the challenges we have to face is that capture video support
>> for our platform is mx2_camera.c, which is a soc-camera host driver;
>> while ov7670.c was developed for being used as part of a more complex
>> video card.
>>
>> Here is the list of current users of ov7670:
>>
>> http://lxr.linux.no/#linux+v3.5.3/drivers/media/video/gspca/ov519.c
>> http://lxr.linux.no/#linux+v3.5.3/drivers/media/video/gspca/sn9c20x.c
>> http://lxr.linux.no/#linux+v3.5.3/drivers/media/video/gspca/vc032x.c
>
> These do not actually use the ov7670 driver. They program it themselves.
> It would be nice if the gspca driver would get support for subdevs, but
> that's a separate topic.
>
>> http://lxr.linux.no/#linux+v3.5.3/drivers/media/video/via-camera.c
>> http://lxr.linux.no/#linux+v3.5.3/drivers/media/video/marvell-ccic/mcam-core.c
>>
>> These are basically the improvements we need to make to this driver in
>> order to satisfy our needs:
>>
>> 1.- Adapt v4l2 controls to the subvevice control framework, with a
>> proper ctrl handler, etc...
>> 2.- Add the possibility to bypass PLL and clkrc preescaler.
>> 3.- Adjust vstart/vstop in order to remove an horizontal green line.
>> 4.- Disable pixclk during horizontal blanking.
>> 5.- min_height, min_width should be respected in try_fmt().
>> 6.- Pass platform data when used with a soc-camera host driver.
>> 7.- Add V4L2_CID_POWER_LINE_FREQUENCY ctrl.
>>
>> I will try to summarize below why we need to accomplish each of the
>> previous tasks and what solution we propose for them:
>>
>> 1.- Adapt v4l2 controls to the subvevice control framework, with a
>> proper ctrl handler, etc...
>>
>> Why? Because soc-camera needs to inherit v4l2 subdevice controls in
>> order to expose them to user space.
>> How? Something like the following, incomplete, patch:
>
> Luckily you didn't do too much work on this. I have old patches for this in
> this tree:
>
> http://git.linuxtv.org/hverkuil/media_tree.git/shortlog/refs/heads/cafe-ctrl
>

Great. This is the reason why I like to always ask first.

> The main reason why I never continued with this was that at the time I wrote
> this I realized that the control framework needed proper support for what's
> now called auto-clusters (i.e. how to handle autofoo/foo controls like autogain
> and gain correctly).
>
> I intended to pick this up at some time, but never got around to it.
>
> I think these patches will still apply with some work, but it needs to be
> converted to use the autocluster support that's now in the control framework.
>
>>
>> ---
>> @@ -190,6 +196,7 @@ MODULE_PARM_DESC(debug, "Debug level (0-1)");
>>  struct ov7670_format_struct;  /* coming later */
>>  struct ov7670_info {
>>         struct v4l2_subdev sd;
>> +       struct v4l2_ctrl_handler hdl;
>>         struct ov7670_format_struct *fmt;  /* Current format */
>>         unsigned char sat;              /* Saturation value */
>>         int hue;                        /* Hue value */
>>
>>
>> @@ -1480,10 +1518,14 @@ static int ov7670_s_register(struct
>> v4l2_subdev *sd, struct v4l2_dbg_register *r
>>
>>  /* ----------------------------------------------------------------------- */
>>
>> +static const struct v4l2_ctrl_ops ov7670_ctrl_ops = {
>> +       .s_ctrl = ov7670_s_ctrl,
>> +};
>> +
>>  static const struct v4l2_subdev_core_ops ov7670_core_ops = {
>>         .g_chip_ident = ov7670_g_chip_ident,
>> -       .g_ctrl = ov7670_g_ctrl,
>> -       .s_ctrl = ov7670_s_ctrl,
>> +       .g_ctrl = v4l2_subdev_g_ctrl,
>> +       .s_ctrl = v4l2_subdev_s_ctrl,
>>         .queryctrl = ov7670_queryctrl,
>>         .reset = ov7670_reset,
>>         .init = ov7670_init,
>>
>> @@ -1551,6 +1600,16 @@ static int ov7670_probe(struct i2c_client *client,
>>         v4l_info(client, "chip found @ 0x%02x (%s)\n",
>>                         client->addr << 1, client->adapter->name);
>>
>> +       v4l2_ctrl_handler_init(&info->hdl, 1);
>> +       v4l2_ctrl_new_std(&info->hdl, &ov7670_ctrl_ops,
>> V4L2_CID_VFLIP, 0, 1, 1, 0);
>> ...
>> ...
>> +       sd->ctrl_handler = &info->hdl;
>> +       if (info->hdl.error) {
>> +               v4l2_ctrl_handler_free(&info->hdl);
>> +               kfree(info);
>> +               return info->hdl.error;
>> +       }
>> +       v4l2_ctrl_handler_setup(&info->hdl);
>> +
>> ---
>>
>> 2.- Add the possibility to bypass PLL and clkrc preescaler.
>>
>> Why? The formula to get the desired frame rate in this chip in YUV is
>> the following: fps = fpclk / (2 * 510 * 784) This means that for a
>> desired fps = 30 we need fpclk = 24MHz. For that reason we have a
>> clean 24MHz xvclk input that comes from an oscillator. If we enable
>> the PLL it internally transforms the 24MHz in 22MHz and thus fps is
>> not 30 but 27. In order to get 30fps we need to bypass the PLL.
>> How? Defining a platform flag 'direct_clk' or similar that allows
>> xvclk being used directly as the pixel clock.
>>
>> 3.- Adjust vstart/vstop in order to remove an horizontal green line.
>>
>> Why? Currently, in the driver, for VGA, vstart =  10 and vstop = 490.
>> From our tests we found out that vstart = 14, vstop = 494 in order to
>> remove a disgusting horizontal green line in ov7675.
>> How? It seems these sensor aren't provided with a version register or
>> anything similar so I can't think of a clean solution for this yet.
>> Suggestions will be much appreciated.
>
> Using platform_data for this is what springs to mind.

I had thought about it too but, there
>> 4.- Disable pixclk during horizontal blanking.
>>
>> Why? Otherwise i.MX27 will capture wrong pixels during blanking periods.
>> How? Through a private V4L2 control.
>
> Or platform_data as well?

Yes, that could be a valid option too.

>
>> 5.- min_height, min_width should be respected in try_fmt().
>> Why? Otherwise you are telling the user you are going to use a
>> different size than the one you are going to use.
>> How? With a patch similar to this:
>>
>> ---
>> @@ -759,8 +772,10 @@ 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;
>> +       int win_sizes_limit = N_WIN_SIZES;
>>         struct ov7670_win_size *wsize;
>> +       struct ov7670_info *info = to_state(sd);
>>
>>         for (index = 0; index < N_OV7670_FMTS; index++)
>>                 if (ov7670_formats[index].mbus_code == fmt->code)
>> @@ -776,15 +791,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 + 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; wsize < ov7670_win_sizes + N_WIN_SIZES;
>> +       for (wsize = ov7670_win_sizes; wsize < ov7670_win_sizes +
>> win_sizes_limit;
>>              wsize++)
>>                 if (fmt->width >= wsize->width && fmt->height >= wsize->height)
>>                         break;
>> -       if (wsize >= ov7670_win_sizes + N_WIN_SIZES)
>> +       if (wsize >= ov7670_win_sizes + win_sizes_limit)
>>                 wsize--;   /* Take the smallest one */
>>         if (ret_wsize != NULL)
>>                 *ret_wsize = wsize;
>> ---
>>
>> 6.- Pass platform data when used with a soc-camera host driver.
>> Why? We need to set several platform data like 'min_height',
>> 'min_width' and others.
>> How? This is an old subject we discussed in January. We agreed that
>> some soc-camera core changes were needed, but I couldn't find the time
>> and I think nobody else has addressed it either. Please, correct me if
>> I am wrong:http://patchwork.linuxtv.org/patch/8860/
>>
>> 7.- Add V4L2_CID_POWER_LINE_FREQUENCY ctrl.
>> Why? Because the platform will be used in several countries.
>> How? As long as point 1 is solved this is quite trivial.
>>
>>
>> The reason of this e-mail is to discuss whether you find these
>> solution suitable or not and, more important, whether you think the
>> suggested changes could break existing drivers.
>
> Well, those bridge drivers that use the ov7670 subdev should also be converted
> to the control framework. My tree does at least some of that work, although I
> think some drivers got moved around or were renamed. The changes to those drivers
> should be fairly minimal.
>
> In theory it's possible to skip that conversion, but my goal is to get (almost)
> all drivers converted to the control framework so this is a good opportunity
> to convert these bridge drivers at the same time.

So, how can we proceed to collaborate on this task? Should I pick up
the ov7670 code from your tree and add some fixes so that it applies
to current mainline?
We'd like to do the ov7670 control conversion but waiting for all
bridge drivers to be converted at the same time seems like delaying
progress in ov7670, specially points 1 and 7.

What do you think?

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: Improving ov7670 sensor driver.

[Hans Verkuil]

On Thu 13 September 2012 12:47:53 javier Martin wrote:
> Hi Hans,
> thank you for your response.
> 
> On 13 September 2012 12:07, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> > On Thu 13 September 2012 11:48:17 javier Martin wrote:
> >> Hi,
> >> our new i.MX27 based platform (Visstrim-SM20) uses an ov7675 sensor
> >> attached to the CSI interface. Apparently, this sensor is fully
> >> compatible with the old ov7670. For this reason, it seems rather
> >> sensible that they should share the same driver: ov7670.c
> >> One of the challenges we have to face is that capture video support
> >> for our platform is mx2_camera.c, which is a soc-camera host driver;
> >> while ov7670.c was developed for being used as part of a more complex
> >> video card.
> >>
> >> Here is the list of current users of ov7670:
> >>
> >> http://lxr.linux.no/#linux+v3.5.3/drivers/media/video/gspca/ov519.c
> >> http://lxr.linux.no/#linux+v3.5.3/drivers/media/video/gspca/sn9c20x.c
> >> http://lxr.linux.no/#linux+v3.5.3/drivers/media/video/gspca/vc032x.c
> >
> > These do not actually use the ov7670 driver. They program it themselves.
> > It would be nice if the gspca driver would get support for subdevs, but
> > that's a separate topic.
> >
> >> http://lxr.linux.no/#linux+v3.5.3/drivers/media/video/via-camera.c
> >> http://lxr.linux.no/#linux+v3.5.3/drivers/media/video/marvell-ccic/mcam-core.c
> >>
> >> These are basically the improvements we need to make to this driver in
> >> order to satisfy our needs:
> >>
> >> 1.- Adapt v4l2 controls to the subvevice control framework, with a
> >> proper ctrl handler, etc...
> >> 2.- Add the possibility to bypass PLL and clkrc preescaler.
> >> 3.- Adjust vstart/vstop in order to remove an horizontal green line.
> >> 4.- Disable pixclk during horizontal blanking.
> >> 5.- min_height, min_width should be respected in try_fmt().
> >> 6.- Pass platform data when used with a soc-camera host driver.
> >> 7.- Add V4L2_CID_POWER_LINE_FREQUENCY ctrl.
> >>
> >> I will try to summarize below why we need to accomplish each of the
> >> previous tasks and what solution we propose for them:
> >>
> >> 1.- Adapt v4l2 controls to the subvevice control framework, with a
> >> proper ctrl handler, etc...
> >>
> >> Why? Because soc-camera needs to inherit v4l2 subdevice controls in
> >> order to expose them to user space.
> >> How? Something like the following, incomplete, patch:
> >
> > Luckily you didn't do too much work on this. I have old patches for this in
> > this tree:
> >
> > http://git.linuxtv.org/hverkuil/media_tree.git/shortlog/refs/heads/cafe-ctrl
> >
> 
> Great. This is the reason why I like to always ask first.
> 
> > The main reason why I never continued with this was that at the time I wrote
> > this I realized that the control framework needed proper support for what's
> > now called auto-clusters (i.e. how to handle autofoo/foo controls like autogain
> > and gain correctly).
> >
> > I intended to pick this up at some time, but never got around to it.
> >
> > I think these patches will still apply with some work, but it needs to be
> > converted to use the autocluster support that's now in the control framework.
> >
> >>
> >> ---
> >> @@ -190,6 +196,7 @@ MODULE_PARM_DESC(debug, "Debug level (0-1)");
> >>  struct ov7670_format_struct;  /* coming later */
> >>  struct ov7670_info {
> >>         struct v4l2_subdev sd;
> >> +       struct v4l2_ctrl_handler hdl;
> >>         struct ov7670_format_struct *fmt;  /* Current format */
> >>         unsigned char sat;              /* Saturation value */
> >>         int hue;                        /* Hue value */
> >>
> >>
> >> @@ -1480,10 +1518,14 @@ static int ov7670_s_register(struct
> >> v4l2_subdev *sd, struct v4l2_dbg_register *r
> >>
> >>  /* ----------------------------------------------------------------------- */
> >>
> >> +static const struct v4l2_ctrl_ops ov7670_ctrl_ops = {
> >> +       .s_ctrl = ov7670_s_ctrl,
> >> +};
> >> +
> >>  static const struct v4l2_subdev_core_ops ov7670_core_ops = {
> >>         .g_chip_ident = ov7670_g_chip_ident,
> >> -       .g_ctrl = ov7670_g_ctrl,
> >> -       .s_ctrl = ov7670_s_ctrl,
> >> +       .g_ctrl = v4l2_subdev_g_ctrl,
> >> +       .s_ctrl = v4l2_subdev_s_ctrl,
> >>         .queryctrl = ov7670_queryctrl,
> >>         .reset = ov7670_reset,
> >>         .init = ov7670_init,
> >>
> >> @@ -1551,6 +1600,16 @@ static int ov7670_probe(struct i2c_client *client,
> >>         v4l_info(client, "chip found @ 0x%02x (%s)\n",
> >>                         client->addr << 1, client->adapter->name);
> >>
> >> +       v4l2_ctrl_handler_init(&info->hdl, 1);
> >> +       v4l2_ctrl_new_std(&info->hdl, &ov7670_ctrl_ops,
> >> V4L2_CID_VFLIP, 0, 1, 1, 0);
> >> ...
> >> ...
> >> +       sd->ctrl_handler = &info->hdl;
> >> +       if (info->hdl.error) {
> >> +               v4l2_ctrl_handler_free(&info->hdl);
> >> +               kfree(info);
> >> +               return info->hdl.error;
> >> +       }
> >> +       v4l2_ctrl_handler_setup(&info->hdl);
> >> +
> >> ---
> >>
> >> 2.- Add the possibility to bypass PLL and clkrc preescaler.
> >>
> >> Why? The formula to get the desired frame rate in this chip in YUV is
> >> the following: fps = fpclk / (2 * 510 * 784) This means that for a
> >> desired fps = 30 we need fpclk = 24MHz. For that reason we have a
> >> clean 24MHz xvclk input that comes from an oscillator. If we enable
> >> the PLL it internally transforms the 24MHz in 22MHz and thus fps is
> >> not 30 but 27. In order to get 30fps we need to bypass the PLL.
> >> How? Defining a platform flag 'direct_clk' or similar that allows
> >> xvclk being used directly as the pixel clock.
> >>
> >> 3.- Adjust vstart/vstop in order to remove an horizontal green line.
> >>
> >> Why? Currently, in the driver, for VGA, vstart =  10 and vstop = 490.
> >> From our tests we found out that vstart = 14, vstop = 494 in order to
> >> remove a disgusting horizontal green line in ov7675.
> >> How? It seems these sensor aren't provided with a version register or
> >> anything similar so I can't think of a clean solution for this yet.
> >> Suggestions will be much appreciated.
> >
> > Using platform_data for this is what springs to mind.
> 
> I had thought about it too but, there

Unfinished sentence?

> >> 4.- Disable pixclk during horizontal blanking.
> >>
> >> Why? Otherwise i.MX27 will capture wrong pixels during blanking periods.
> >> How? Through a private V4L2 control.
> >
> > Or platform_data as well?
> 
> Yes, that could be a valid option too.
> 
> >
> >> 5.- min_height, min_width should be respected in try_fmt().
> >> Why? Otherwise you are telling the user you are going to use a
> >> different size than the one you are going to use.
> >> How? With a patch similar to this:
> >>
> >> ---
> >> @@ -759,8 +772,10 @@ 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;
> >> +       int win_sizes_limit = N_WIN_SIZES;
> >>         struct ov7670_win_size *wsize;
> >> +       struct ov7670_info *info = to_state(sd);
> >>
> >>         for (index = 0; index < N_OV7670_FMTS; index++)
> >>                 if (ov7670_formats[index].mbus_code == fmt->code)
> >> @@ -776,15 +791,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 + 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; wsize < ov7670_win_sizes + N_WIN_SIZES;
> >> +       for (wsize = ov7670_win_sizes; wsize < ov7670_win_sizes +
> >> win_sizes_limit;
> >>              wsize++)
> >>                 if (fmt->width >= wsize->width && fmt->height >= wsize->height)
> >>                         break;
> >> -       if (wsize >= ov7670_win_sizes + N_WIN_SIZES)
> >> +       if (wsize >= ov7670_win_sizes + win_sizes_limit)
> >>                 wsize--;   /* Take the smallest one */
> >>         if (ret_wsize != NULL)
> >>                 *ret_wsize = wsize;
> >> ---
> >>
> >> 6.- Pass platform data when used with a soc-camera host driver.
> >> Why? We need to set several platform data like 'min_height',
> >> 'min_width' and others.
> >> How? This is an old subject we discussed in January. We agreed that
> >> some soc-camera core changes were needed, but I couldn't find the time
> >> and I think nobody else has addressed it either. Please, correct me if
> >> I am wrong:http://patchwork.linuxtv.org/patch/8860/
> >>
> >> 7.- Add V4L2_CID_POWER_LINE_FREQUENCY ctrl.
> >> Why? Because the platform will be used in several countries.
> >> How? As long as point 1 is solved this is quite trivial.
> >>
> >>
> >> The reason of this e-mail is to discuss whether you find these
> >> solution suitable or not and, more important, whether you think the
> >> suggested changes could break existing drivers.
> >
> > Well, those bridge drivers that use the ov7670 subdev should also be converted
> > to the control framework. My tree does at least some of that work, although I
> > think some drivers got moved around or were renamed. The changes to those drivers
> > should be fairly minimal.
> >
> > In theory it's possible to skip that conversion, but my goal is to get (almost)
> > all drivers converted to the control framework so this is a good opportunity
> > to convert these bridge drivers at the same time.
> 
> So, how can we proceed to collaborate on this task? Should I pick up
> the ov7670 code from your tree and add some fixes so that it applies
> to current mainline?

Please do. I won't have time to work on this any time soon.

> We'd like to do the ov7670 control conversion but waiting for all
> bridge drivers to be converted at the same time seems like delaying
> progress in ov7670, specially points 1 and 7.
> 
> What do you think?

If you look at the two patches in my tree that convert the bridge drivers
you'll see that those patches are quite small. And there are only two bridge
drivers as well. Unless things have changed in those drivers since the last
time I looked I expect that this is a minimal amount of work.

Regards,

	Hans

Re: Improving ov7670 sensor driver.

[javier Martin]

On 13 September 2012 13:00, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> On Thu 13 September 2012 12:47:53 javier Martin wrote:
>> Hi Hans,
>> thank you for your response.
>>
>> On 13 September 2012 12:07, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> > On Thu 13 September 2012 11:48:17 javier Martin wrote:
>> >> Hi,
>> >> our new i.MX27 based platform (Visstrim-SM20) uses an ov7675 sensor
>> >> attached to the CSI interface. Apparently, this sensor is fully
>> >> compatible with the old ov7670. For this reason, it seems rather
>> >> sensible that they should share the same driver: ov7670.c
>> >> One of the challenges we have to face is that capture video support
>> >> for our platform is mx2_camera.c, which is a soc-camera host driver;
>> >> while ov7670.c was developed for being used as part of a more complex
>> >> video card.
>> >>
>> >> Here is the list of current users of ov7670:
>> >>
>> >> http://lxr.linux.no/#linux+v3.5.3/drivers/media/video/gspca/ov519.c
>> >> http://lxr.linux.no/#linux+v3.5.3/drivers/media/video/gspca/sn9c20x.c
>> >> http://lxr.linux.no/#linux+v3.5.3/drivers/media/video/gspca/vc032x.c
>> >
>> > These do not actually use the ov7670 driver. They program it themselves.
>> > It would be nice if the gspca driver would get support for subdevs, but
>> > that's a separate topic.
>> >
>> >> http://lxr.linux.no/#linux+v3.5.3/drivers/media/video/via-camera.c
>> >> http://lxr.linux.no/#linux+v3.5.3/drivers/media/video/marvell-ccic/mcam-core.c
>> >>
>> >> These are basically the improvements we need to make to this driver in
>> >> order to satisfy our needs:
>> >>
>> >> 1.- Adapt v4l2 controls to the subvevice control framework, with a
>> >> proper ctrl handler, etc...
>> >> 2.- Add the possibility to bypass PLL and clkrc preescaler.
>> >> 3.- Adjust vstart/vstop in order to remove an horizontal green line.
>> >> 4.- Disable pixclk during horizontal blanking.
>> >> 5.- min_height, min_width should be respected in try_fmt().
>> >> 6.- Pass platform data when used with a soc-camera host driver.
>> >> 7.- Add V4L2_CID_POWER_LINE_FREQUENCY ctrl.
>> >>
>> >> I will try to summarize below why we need to accomplish each of the
>> >> previous tasks and what solution we propose for them:
>> >>
>> >> 1.- Adapt v4l2 controls to the subvevice control framework, with a
>> >> proper ctrl handler, etc...
>> >>
>> >> Why? Because soc-camera needs to inherit v4l2 subdevice controls in
>> >> order to expose them to user space.
>> >> How? Something like the following, incomplete, patch:
>> >
>> > Luckily you didn't do too much work on this. I have old patches for this in
>> > this tree:
>> >
>> > http://git.linuxtv.org/hverkuil/media_tree.git/shortlog/refs/heads/cafe-ctrl
>> >
>>
>> Great. This is the reason why I like to always ask first.
>>
>> > The main reason why I never continued with this was that at the time I wrote
>> > this I realized that the control framework needed proper support for what's
>> > now called auto-clusters (i.e. how to handle autofoo/foo controls like autogain
>> > and gain correctly).
>> >
>> > I intended to pick this up at some time, but never got around to it.
>> >
>> > I think these patches will still apply with some work, but it needs to be
>> > converted to use the autocluster support that's now in the control framework.
>> >
>> >>
>> >> ---
>> >> @@ -190,6 +196,7 @@ MODULE_PARM_DESC(debug, "Debug level (0-1)");
>> >>  struct ov7670_format_struct;  /* coming later */
>> >>  struct ov7670_info {
>> >>         struct v4l2_subdev sd;
>> >> +       struct v4l2_ctrl_handler hdl;
>> >>         struct ov7670_format_struct *fmt;  /* Current format */
>> >>         unsigned char sat;              /* Saturation value */
>> >>         int hue;                        /* Hue value */
>> >>
>> >>
>> >> @@ -1480,10 +1518,14 @@ static int ov7670_s_register(struct
>> >> v4l2_subdev *sd, struct v4l2_dbg_register *r
>> >>
>> >>  /* ----------------------------------------------------------------------- */
>> >>
>> >> +static const struct v4l2_ctrl_ops ov7670_ctrl_ops = {
>> >> +       .s_ctrl = ov7670_s_ctrl,
>> >> +};
>> >> +
>> >>  static const struct v4l2_subdev_core_ops ov7670_core_ops = {
>> >>         .g_chip_ident = ov7670_g_chip_ident,
>> >> -       .g_ctrl = ov7670_g_ctrl,
>> >> -       .s_ctrl = ov7670_s_ctrl,
>> >> +       .g_ctrl = v4l2_subdev_g_ctrl,
>> >> +       .s_ctrl = v4l2_subdev_s_ctrl,
>> >>         .queryctrl = ov7670_queryctrl,
>> >>         .reset = ov7670_reset,
>> >>         .init = ov7670_init,
>> >>
>> >> @@ -1551,6 +1600,16 @@ static int ov7670_probe(struct i2c_client *client,
>> >>         v4l_info(client, "chip found @ 0x%02x (%s)\n",
>> >>                         client->addr << 1, client->adapter->name);
>> >>
>> >> +       v4l2_ctrl_handler_init(&info->hdl, 1);
>> >> +       v4l2_ctrl_new_std(&info->hdl, &ov7670_ctrl_ops,
>> >> V4L2_CID_VFLIP, 0, 1, 1, 0);
>> >> ...
>> >> ...
>> >> +       sd->ctrl_handler = &info->hdl;
>> >> +       if (info->hdl.error) {
>> >> +               v4l2_ctrl_handler_free(&info->hdl);
>> >> +               kfree(info);
>> >> +               return info->hdl.error;
>> >> +       }
>> >> +       v4l2_ctrl_handler_setup(&info->hdl);
>> >> +
>> >> ---
>> >>
>> >> 2.- Add the possibility to bypass PLL and clkrc preescaler.
>> >>
>> >> Why? The formula to get the desired frame rate in this chip in YUV is
>> >> the following: fps = fpclk / (2 * 510 * 784) This means that for a
>> >> desired fps = 30 we need fpclk = 24MHz. For that reason we have a
>> >> clean 24MHz xvclk input that comes from an oscillator. If we enable
>> >> the PLL it internally transforms the 24MHz in 22MHz and thus fps is
>> >> not 30 but 27. In order to get 30fps we need to bypass the PLL.
>> >> How? Defining a platform flag 'direct_clk' or similar that allows
>> >> xvclk being used directly as the pixel clock.
>> >>
>> >> 3.- Adjust vstart/vstop in order to remove an horizontal green line.
>> >>
>> >> Why? Currently, in the driver, for VGA, vstart =  10 and vstop = 490.
>> >> From our tests we found out that vstart = 14, vstop = 494 in order to
>> >> remove a disgusting horizontal green line in ov7675.
>> >> How? It seems these sensor aren't provided with a version register or
>> >> anything similar so I can't think of a clean solution for this yet.
>> >> Suggestions will be much appreciated.
>> >
>> > Using platform_data for this is what springs to mind.
>>
>> I had thought about it too but, there
>
> Unfinished sentence?
>

Yes. Sorry :)

I meant that I had thought about it too but there are one pair of
vstart,vstop values for each supported resolution: VGA, QVGA, CIF,
QCIF.
I could add 'vstart_vga', 'vstop_vga' as platform_data but in the
future someone could want to add the same values for the other ones
and I don't know if that would be acceptable.

Another solution I just came up with would be adding a flag 'version'
where we could indicate if we are dealing with an ov7670 or an ov7675
and change those 'vstart', 'vstop' values internally based on this.
This could be useful for some other issues in the future.

>> >> 4.- Disable pixclk during horizontal blanking.
>> >>
>> >> Why? Otherwise i.MX27 will capture wrong pixels during blanking periods.
>> >> How? Through a private V4L2 control.
>> >
>> > Or platform_data as well?
>>
>> Yes, that could be a valid option too.
>>
>> >
>> >> 5.- min_height, min_width should be respected in try_fmt().
>> >> Why? Otherwise you are telling the user you are going to use a
>> >> different size than the one you are going to use.
>> >> How? With a patch similar to this:
>> >>
>> >> ---
>> >> @@ -759,8 +772,10 @@ 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;
>> >> +       int win_sizes_limit = N_WIN_SIZES;
>> >>         struct ov7670_win_size *wsize;
>> >> +       struct ov7670_info *info = to_state(sd);
>> >>
>> >>         for (index = 0; index < N_OV7670_FMTS; index++)
>> >>                 if (ov7670_formats[index].mbus_code == fmt->code)
>> >> @@ -776,15 +791,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 + 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; wsize < ov7670_win_sizes + N_WIN_SIZES;
>> >> +       for (wsize = ov7670_win_sizes; wsize < ov7670_win_sizes +
>> >> win_sizes_limit;
>> >>              wsize++)
>> >>                 if (fmt->width >= wsize->width && fmt->height >= wsize->height)
>> >>                         break;
>> >> -       if (wsize >= ov7670_win_sizes + N_WIN_SIZES)
>> >> +       if (wsize >= ov7670_win_sizes + win_sizes_limit)
>> >>                 wsize--;   /* Take the smallest one */
>> >>         if (ret_wsize != NULL)
>> >>                 *ret_wsize = wsize;
>> >> ---
>> >>
>> >> 6.- Pass platform data when used with a soc-camera host driver.
>> >> Why? We need to set several platform data like 'min_height',
>> >> 'min_width' and others.
>> >> How? This is an old subject we discussed in January. We agreed that
>> >> some soc-camera core changes were needed, but I couldn't find the time
>> >> and I think nobody else has addressed it either. Please, correct me if
>> >> I am wrong:http://patchwork.linuxtv.org/patch/8860/
>> >>
>> >> 7.- Add V4L2_CID_POWER_LINE_FREQUENCY ctrl.
>> >> Why? Because the platform will be used in several countries.
>> >> How? As long as point 1 is solved this is quite trivial.
>> >>
>> >>
>> >> The reason of this e-mail is to discuss whether you find these
>> >> solution suitable or not and, more important, whether you think the
>> >> suggested changes could break existing drivers.
>> >
>> > Well, those bridge drivers that use the ov7670 subdev should also be converted
>> > to the control framework. My tree does at least some of that work, although I
>> > think some drivers got moved around or were renamed. The changes to those drivers
>> > should be fairly minimal.
>> >
>> > In theory it's possible to skip that conversion, but my goal is to get (almost)
>> > all drivers converted to the control framework so this is a good opportunity
>> > to convert these bridge drivers at the same time.
>>
>> So, how can we proceed to collaborate on this task? Should I pick up
>> the ov7670 code from your tree and add some fixes so that it applies
>> to current mainline?
>
> Please do. I won't have time to work on this any time soon.
>
>> We'd like to do the ov7670 control conversion but waiting for all
>> bridge drivers to be converted at the same time seems like delaying
>> progress in ov7670, specially points 1 and 7.
>>
>> What do you think?
>
> If you look at the two patches in my tree that convert the bridge drivers
> you'll see that those patches are quite small. And there are only two bridge
> drivers as well. Unless things have changed in those drivers since the last
> time I looked I expect that this is a minimal amount of work.

OK, I will take a look.

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: Improving ov7670 sensor driver.

[Hans Verkuil]

On Thu 13 September 2012 13:19:14 javier Martin wrote:
> On 13 September 2012 13:00, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> > On Thu 13 September 2012 12:47:53 javier Martin wrote:
> >> >> 3.- Adjust vstart/vstop in order to remove an horizontal green line.
> >> >>
> >> >> Why? Currently, in the driver, for VGA, vstart =  10 and vstop = 490.
> >> >> From our tests we found out that vstart = 14, vstop = 494 in order to
> >> >> remove a disgusting horizontal green line in ov7675.
> >> >> How? It seems these sensor aren't provided with a version register or
> >> >> anything similar so I can't think of a clean solution for this yet.
> >> >> Suggestions will be much appreciated.
> >> >
> >> > Using platform_data for this is what springs to mind.
> >>
> >> I had thought about it too but, there
> >
> > Unfinished sentence?
> >
> 
> Yes. Sorry :)
> 
> I meant that I had thought about it too but there are one pair of
> vstart,vstop values for each supported resolution: VGA, QVGA, CIF,
> QCIF.
> I could add 'vstart_vga', 'vstop_vga' as platform_data but in the
> future someone could want to add the same values for the other ones
> and I don't know if that would be acceptable.
> 
> Another solution I just came up with would be adding a flag 'version'
> where we could indicate if we are dealing with an ov7670 or an ov7675
> and change those 'vstart', 'vstop' values internally based on this.
> This could be useful for some other issues in the future.

You can actually add support for a ov7675 to the ov7670 driver itself
by adding a ov7675 entry to the ov7670_id table. See for example the
i2c/saa7127.c driver on how to do that.

Regards,

	Hans

Re: Improving ov7670 sensor driver.

[Mauro Carvalho Chehab]

Hi Javier,

I'm not too familiar with soc_camera and ov7670 drivers, so my comments
reflects my understanding of the question, without taking into account
drivers specifics.

Em 13-09-2012 06:48, javier Martin escreveu:
> Hi,
> our new i.MX27 based platform (Visstrim-SM20) uses an ov7675 sensor
> attached to the CSI interface. Apparently, this sensor is fully
> compatible with the old ov7670. For this reason, it seems rather
> sensible that they should share the same driver: ov7670.c
> One of the challenges we have to face is that capture video support
> for our platform is mx2_camera.c, which is a soc-camera host driver;
> while ov7670.c was developed for being used as part of a more complex
> video card.
> 
> Here is the list of current users of ov7670:
> 
> http://lxr.linux.no/#linux+v3.5.3/drivers/media/video/gspca/ov519.c
> http://lxr.linux.no/#linux+v3.5.3/drivers/media/video/gspca/sn9c20x.c
> http://lxr.linux.no/#linux+v3.5.3/drivers/media/video/gspca/vc032x.c
> http://lxr.linux.no/#linux+v3.5.3/drivers/media/video/via-camera.c
> http://lxr.linux.no/#linux+v3.5.3/drivers/media/video/marvell-ccic/mcam-core.c

In order to avoid breakages on those drivers, we need to be sure that
none of the changes will alter the register settings used there.

(C/C Hans de Goede, as he is the gspca maintainer)

> 
> These are basically the improvements we need to make to this driver in
> order to satisfy our needs:
> 
> 1.- Adapt v4l2 controls to the subvevice control framework, with a
> proper ctrl handler, etc...
> 2.- Add the possibility to bypass PLL and clkrc preescaler.
> 3.- Adjust vstart/vstop in order to remove an horizontal green line.
> 4.- Disable pixclk during horizontal blanking.
> 5.- min_height, min_width should be respected in try_fmt().
> 6.- Pass platform data when used with a soc-camera host driver.
> 7.- Add V4L2_CID_POWER_LINE_FREQUENCY ctrl.

Doing one patch per change helps to review the changes individually.
I suspect that it will needed to be tested with the above drivers,
anyway.

> I will try to summarize below why we need to accomplish each of the
> previous tasks and what solution we propose for them:
> 
> 1.- Adapt v4l2 controls to the subvevice control framework, with a
> proper ctrl handler, etc...
> 
> Why? Because soc-camera needs to inherit v4l2 subdevice controls in
> order to expose them to user space.
> How? Something like the following, incomplete, patch:
> 
> ---
> @@ -190,6 +196,7 @@ MODULE_PARM_DESC(debug, "Debug level (0-1)");
>  struct ov7670_format_struct;  /* coming later */
>  struct ov7670_info {
>         struct v4l2_subdev sd;
> +       struct v4l2_ctrl_handler hdl;
>         struct ov7670_format_struct *fmt;  /* Current format */
>         unsigned char sat;              /* Saturation value */
>         int hue;                        /* Hue value */
> 
> 
> @@ -1480,10 +1518,14 @@ static int ov7670_s_register(struct
> v4l2_subdev *sd, struct v4l2_dbg_register *r
> 
>  /* ----------------------------------------------------------------------- */
> 
> +static const struct v4l2_ctrl_ops ov7670_ctrl_ops = {
> +       .s_ctrl = ov7670_s_ctrl,
> +};
> +
>  static const struct v4l2_subdev_core_ops ov7670_core_ops = {
>         .g_chip_ident = ov7670_g_chip_ident,
> -       .g_ctrl = ov7670_g_ctrl,
> -       .s_ctrl = ov7670_s_ctrl,
> +       .g_ctrl = v4l2_subdev_g_ctrl,
> +       .s_ctrl = v4l2_subdev_s_ctrl,
>         .queryctrl = ov7670_queryctrl,
>         .reset = ov7670_reset,
>         .init = ov7670_init,
> 
> @@ -1551,6 +1600,16 @@ static int ov7670_probe(struct i2c_client *client,
>         v4l_info(client, "chip found @ 0x%02x (%s)\n",
>                         client->addr << 1, client->adapter->name);
> 
> +       v4l2_ctrl_handler_init(&info->hdl, 1);
> +       v4l2_ctrl_new_std(&info->hdl, &ov7670_ctrl_ops,
> V4L2_CID_VFLIP, 0, 1, 1, 0);
> ...
> ...
> +       sd->ctrl_handler = &info->hdl;
> +       if (info->hdl.error) {
> +               v4l2_ctrl_handler_free(&info->hdl);
> +               kfree(info);
> +               return info->hdl.error;
> +       }
> +       v4l2_ctrl_handler_setup(&info->hdl);
> +
> ---

Tests are required here, but I don't think this would break anything.
> 
> 2.- Add the possibility to bypass PLL and clkrc preescaler.
> 
> Why? The formula to get the desired frame rate in this chip in YUV is
> the following: fps = fpclk / (2 * 510 * 784) This means that for a
> desired fps = 30 we need fpclk = 24MHz. For that reason we have a
> clean 24MHz xvclk input that comes from an oscillator. If we enable
> the PLL it internally transforms the 24MHz in 22MHz and thus fps is
> not 30 but 27. In order to get 30fps we need to bypass the PLL.
> How? Defining a platform flag 'direct_clk' or similar that allows
> xvclk being used directly as the pixel clock.

As this should be a new platform data, provided that the old behavior
is to use the old formula, this shouldn't break anything.

> 
> 3.- Adjust vstart/vstop in order to remove an horizontal green line.
> 
> Why? Currently, in the driver, for VGA, vstart =  10 and vstop = 490.
>>From our tests we found out that vstart = 14, vstop = 494 in order to
> remove a disgusting horizontal green line in ov7675.
> How? It seems these sensor aren't provided with a version register or
> anything similar so I can't think of a clean solution for this yet.
> Suggestions will be much appreciated.

If it is not possible to differentiate between ov7670 and ov7675, just
add a platform data flag, in order to identify the ov7675

> 4.- Disable pixclk during horizontal blanking.
> 
> Why? Otherwise i.MX27 will capture wrong pixels during blanking periods.
> How? Through a private V4L2 control.

Hmm... I'm assuming that there is a register that controls pixclk disable
during horizontal blanking. In this case, the better is to add support for
it also via platform data.

> 
> 5.- min_height, min_width should be respected in try_fmt().
> Why? Otherwise you are telling the user you are going to use a
> different size than the one you are going to use.
> How? With a patch similar to this:
> 
> ---
> @@ -759,8 +772,10 @@ 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;
> +       int win_sizes_limit = N_WIN_SIZES;
>         struct ov7670_win_size *wsize;
> +       struct ov7670_info *info = to_state(sd);
> 
>         for (index = 0; index < N_OV7670_FMTS; index++)
>                 if (ov7670_formats[index].mbus_code == fmt->code)
> @@ -776,15 +791,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 + 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; wsize < ov7670_win_sizes + N_WIN_SIZES;
> +       for (wsize = ov7670_win_sizes; wsize < ov7670_win_sizes +
> win_sizes_limit;
>              wsize++)
>                 if (fmt->width >= wsize->width && fmt->height >= wsize->height)
>                         break;
> -       if (wsize >= ov7670_win_sizes + N_WIN_SIZES)
> +       if (wsize >= ov7670_win_sizes + win_sizes_limit)
>                 wsize--;   /* Take the smallest one */
>         if (ret_wsize != NULL)
>                 *ret_wsize = wsize;

Of course, patch need to be tested, but that change looks fine on my eyes.

> ---
> 
> 6.- Pass platform data when used with a soc-camera host driver.
> Why? We need to set several platform data like 'min_height',
> 'min_width' and others.
> How? This is an old subject we discussed in January. We agreed that
> some soc-camera core changes were needed, but I couldn't find the time
> and I think nobody else has addressed it either. Please, correct me if
> I am wrong:http://patchwork.linuxtv.org/patch/8860/

Hmm... patch 8860 is related to s_input logic, and not to pass platform data.

For sure passing platform data is needed, as different boards may require
different sensor configurations. If soc_camera doesn't support it yet,
this needs to be added there.

> 
> 7.- Add V4L2_CID_POWER_LINE_FREQUENCY ctrl.
> Why? Because the platform will be used in several countries.
> How? As long as point 1 is solved this is quite trivial.
> 
> 
> The reason of this e-mail is to discuss whether you find these
> solution suitable or not and, more important, whether you think the
> suggested changes could break existing drivers.
> 
> Regards.
> 

Regards,
Mauro

Re: Improving ov7670 sensor driver.

[javier Martin]

Hi Mauro,
thank you for your interest.

On 13 September 2012 15:00, Mauro Carvalho Chehab <mchehab@redhat.com> wrote:
> Hi Javier,
>
> I'm not too familiar with soc_camera and ov7670 drivers, so my comments
> reflects my understanding of the question, without taking into account
> drivers specifics.
>
> Em 13-09-2012 06:48, javier Martin escreveu:
>> Hi,
>> our new i.MX27 based platform (Visstrim-SM20) uses an ov7675 sensor
>> attached to the CSI interface. Apparently, this sensor is fully
>> compatible with the old ov7670. For this reason, it seems rather
>> sensible that they should share the same driver: ov7670.c
>> One of the challenges we have to face is that capture video support
>> for our platform is mx2_camera.c, which is a soc-camera host driver;
>> while ov7670.c was developed for being used as part of a more complex
>> video card.
>>
>> Here is the list of current users of ov7670:
>>
>> http://lxr.linux.no/#linux+v3.5.3/drivers/media/video/gspca/ov519.c
>> http://lxr.linux.no/#linux+v3.5.3/drivers/media/video/gspca/sn9c20x.c
>> http://lxr.linux.no/#linux+v3.5.3/drivers/media/video/gspca/vc032x.c
>> http://lxr.linux.no/#linux+v3.5.3/drivers/media/video/via-camera.c
>> http://lxr.linux.no/#linux+v3.5.3/drivers/media/video/marvell-ccic/mcam-core.c
>
> In order to avoid breakages on those drivers, we need to be sure that
> none of the changes will alter the register settings used there.
>
> (C/C Hans de Goede, as he is the gspca maintainer)
>
>>
>> These are basically the improvements we need to make to this driver in
>> order to satisfy our needs:
>>
>> 1.- Adapt v4l2 controls to the subvevice control framework, with a
>> proper ctrl handler, etc...
>> 2.- Add the possibility to bypass PLL and clkrc preescaler.
>> 3.- Adjust vstart/vstop in order to remove an horizontal green line.
>> 4.- Disable pixclk during horizontal blanking.
>> 5.- min_height, min_width should be respected in try_fmt().
>> 6.- Pass platform data when used with a soc-camera host driver.
>> 7.- Add V4L2_CID_POWER_LINE_FREQUENCY ctrl.
>
> Doing one patch per change helps to review the changes individually.
> I suspect that it will needed to be tested with the above drivers,
> anyway.
>
>> I will try to summarize below why we need to accomplish each of the
>> previous tasks and what solution we propose for them:
>>
>> 1.- Adapt v4l2 controls to the subvevice control framework, with a
>> proper ctrl handler, etc...
>>
>> Why? Because soc-camera needs to inherit v4l2 subdevice controls in
>> order to expose them to user space.
>> How? Something like the following, incomplete, patch:
>>
>> ---
>> @@ -190,6 +196,7 @@ MODULE_PARM_DESC(debug, "Debug level (0-1)");
>>  struct ov7670_format_struct;  /* coming later */
>>  struct ov7670_info {
>>         struct v4l2_subdev sd;
>> +       struct v4l2_ctrl_handler hdl;
>>         struct ov7670_format_struct *fmt;  /* Current format */
>>         unsigned char sat;              /* Saturation value */
>>         int hue;                        /* Hue value */
>>
>>
>> @@ -1480,10 +1518,14 @@ static int ov7670_s_register(struct
>> v4l2_subdev *sd, struct v4l2_dbg_register *r
>>
>>  /* ----------------------------------------------------------------------- */
>>
>> +static const struct v4l2_ctrl_ops ov7670_ctrl_ops = {
>> +       .s_ctrl = ov7670_s_ctrl,
>> +};
>> +
>>  static const struct v4l2_subdev_core_ops ov7670_core_ops = {
>>         .g_chip_ident = ov7670_g_chip_ident,
>> -       .g_ctrl = ov7670_g_ctrl,
>> -       .s_ctrl = ov7670_s_ctrl,
>> +       .g_ctrl = v4l2_subdev_g_ctrl,
>> +       .s_ctrl = v4l2_subdev_s_ctrl,
>>         .queryctrl = ov7670_queryctrl,
>>         .reset = ov7670_reset,
>>         .init = ov7670_init,
>>
>> @@ -1551,6 +1600,16 @@ static int ov7670_probe(struct i2c_client *client,
>>         v4l_info(client, "chip found @ 0x%02x (%s)\n",
>>                         client->addr << 1, client->adapter->name);
>>
>> +       v4l2_ctrl_handler_init(&info->hdl, 1);
>> +       v4l2_ctrl_new_std(&info->hdl, &ov7670_ctrl_ops,
>> V4L2_CID_VFLIP, 0, 1, 1, 0);
>> ...
>> ...
>> +       sd->ctrl_handler = &info->hdl;
>> +       if (info->hdl.error) {
>> +               v4l2_ctrl_handler_free(&info->hdl);
>> +               kfree(info);
>> +               return info->hdl.error;
>> +       }
>> +       v4l2_ctrl_handler_setup(&info->hdl);
>> +
>> ---
>
> Tests are required here, but I don't think this would break anything.
>>
>> 2.- Add the possibility to bypass PLL and clkrc preescaler.
>>
>> Why? The formula to get the desired frame rate in this chip in YUV is
>> the following: fps = fpclk / (2 * 510 * 784) This means that for a
>> desired fps = 30 we need fpclk = 24MHz. For that reason we have a
>> clean 24MHz xvclk input that comes from an oscillator. If we enable
>> the PLL it internally transforms the 24MHz in 22MHz and thus fps is
>> not 30 but 27. In order to get 30fps we need to bypass the PLL.
>> How? Defining a platform flag 'direct_clk' or similar that allows
>> xvclk being used directly as the pixel clock.
>
> As this should be a new platform data, provided that the old behavior
> is to use the old formula, this shouldn't break anything.
>
>>
>> 3.- Adjust vstart/vstop in order to remove an horizontal green line.
>>
>> Why? Currently, in the driver, for VGA, vstart =  10 and vstop = 490.
>>>From our tests we found out that vstart = 14, vstop = 494 in order to
>> remove a disgusting horizontal green line in ov7675.
>> How? It seems these sensor aren't provided with a version register or
>> anything similar so I can't think of a clean solution for this yet.
>> Suggestions will be much appreciated.
>
> If it is not possible to differentiate between ov7670 and ov7675, just
> add a platform data flag, in order to identify the ov7675
>
>> 4.- Disable pixclk during horizontal blanking.
>>
>> Why? Otherwise i.MX27 will capture wrong pixels during blanking periods.
>> How? Through a private V4L2 control.
>
> Hmm... I'm assuming that there is a register that controls pixclk disable
> during horizontal blanking. In this case, the better is to add support for
> it also via platform data.
>
>>
>> 5.- min_height, min_width should be respected in try_fmt().
>> Why? Otherwise you are telling the user you are going to use a
>> different size than the one you are going to use.
>> How? With a patch similar to this:
>>
>> ---
>> @@ -759,8 +772,10 @@ 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;
>> +       int win_sizes_limit = N_WIN_SIZES;
>>         struct ov7670_win_size *wsize;
>> +       struct ov7670_info *info = to_state(sd);
>>
>>         for (index = 0; index < N_OV7670_FMTS; index++)
>>                 if (ov7670_formats[index].mbus_code == fmt->code)
>> @@ -776,15 +791,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 + 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; wsize < ov7670_win_sizes + N_WIN_SIZES;
>> +       for (wsize = ov7670_win_sizes; wsize < ov7670_win_sizes +
>> win_sizes_limit;
>>              wsize++)
>>                 if (fmt->width >= wsize->width && fmt->height >= wsize->height)
>>                         break;
>> -       if (wsize >= ov7670_win_sizes + N_WIN_SIZES)
>> +       if (wsize >= ov7670_win_sizes + win_sizes_limit)
>>                 wsize--;   /* Take the smallest one */
>>         if (ret_wsize != NULL)
>>                 *ret_wsize = wsize;
>
> Of course, patch need to be tested, but that change looks fine on my eyes.
>
>> ---
>>
>> 6.- Pass platform data when used with a soc-camera host driver.
>> Why? We need to set several platform data like 'min_height',
>> 'min_width' and others.
>> How? This is an old subject we discussed in January. We agreed that
>> some soc-camera core changes were needed, but I couldn't find the time
>> and I think nobody else has addressed it either. Please, correct me if
>> I am wrong:http://patchwork.linuxtv.org/patch/8860/
>
> Hmm... patch 8860 is related to s_input logic, and not to pass platform data.

I know but in the end we discussed passing platform data to tvp5150
sensor using soc-camera. This is a similar case for the ov7670. Here
is the specific e-mail in the thread:
http://www.mail-archive.com/linux-media@vger.kernel.org/msg41437.html


> For sure passing platform data is needed, as different boards may require
> different sensor configurations. If soc_camera doesn't support it yet,
> this needs to be added there.
>
>>
>> 7.- Add V4L2_CID_POWER_LINE_FREQUENCY ctrl.
>> Why? Because the platform will be used in several countries.
>> How? As long as point 1 is solved this is quite trivial.
>>
>>
>> The reason of this e-mail is to discuss whether you find these
>> solution suitable or not and, more important, whether you think the
>> suggested changes could break existing drivers.
>>
>> Regards.
>>
>
> Regards,
> Mauro
>



-- 
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: Improving ov7670 sensor driver.

[Laurent Pinchart]

Hi Javier,

On Thursday 13 September 2012 11:48:17 javier Martin wrote:
> Hi,
> our new i.MX27 based platform (Visstrim-SM20) uses an ov7675 sensor
> attached to the CSI interface. Apparently, this sensor is fully
> compatible with the old ov7670. For this reason, it seems rather
> sensible that they should share the same driver: ov7670.c
> One of the challenges we have to face is that capture video support
> for our platform is mx2_camera.c, which is a soc-camera host driver;
> while ov7670.c was developed for being used as part of a more complex
> video card.
> 
> Here is the list of current users of ov7670:
> 
> http://lxr.linux.no/#linux+v3.5.3/drivers/media/video/gspca/ov519.c
> http://lxr.linux.no/#linux+v3.5.3/drivers/media/video/gspca/sn9c20x.c
> http://lxr.linux.no/#linux+v3.5.3/drivers/media/video/gspca/vc032x.c
> http://lxr.linux.no/#linux+v3.5.3/drivers/media/video/via-camera.c
> http://lxr.linux.no/#linux+v3.5.3/drivers/media/video/marvell-ccic/mcam-core
> .c
> 
> These are basically the improvements we need to make to this driver in
> order to satisfy our needs:
> 
> 1.- Adapt v4l2 controls to the subvevice control framework, with a
> proper ctrl handler, etc...
> 2.- Add the possibility to bypass PLL and clkrc preescaler.
> 3.- Adjust vstart/vstop in order to remove an horizontal green line.
> 4.- Disable pixclk during horizontal blanking.
> 5.- min_height, min_width should be respected in try_fmt().
> 6.- Pass platform data when used with a soc-camera host driver.
> 7.- Add V4L2_CID_POWER_LINE_FREQUENCY ctrl.

8. Make sure it still works when used with a non soc-camera bridge.

That's the tricky part. I've spent time working on this for the ov772x driver 
(albeit in the other direction, making it usable with a non soc-camera 
bridge). You can find work-in-progress hacks at

http://git.linuxtv.org/pinchartl/media.git/shortlog/refs/heads/omap3isp-
sensors-board

The basic idea is that the soc-camera platform data structure needs to be 
split into a generic device part (currently called soc_camera_pdtata, which 
should be renamed to something not related to soc-camera), and a soc-camera 
specific structure. The device should only see the device part, and the soc-
camera framework will get the host part. That's currently not implemented.

-- 
Regards,

Laurent Pinchart