Re: Improving ov7670 sensor driver.

2012-09-14 Thread javier Martin
Hi Mauro,
thank you for your interest.

On 13 September 2012 15:00, Mauro Carvalho Chehab mche...@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 

Re: Improving ov7670 sensor driver.

2012-09-14 Thread 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

--
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


Re: Improving ov7670 sensor driver.

2012-09-13 Thread 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 

Re: Improving ov7670 sensor driver.

2012-09-13 Thread javier Martin
Hi Hans,
thank you for your response.

On 13 September 2012 12:07, Hans Verkuil hverk...@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? 

Re: Improving ov7670 sensor driver.

2012-09-13 Thread 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 hverk...@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 

Re: Improving ov7670 sensor driver.

2012-09-13 Thread javier Martin
On 13 September 2012 13:00, Hans Verkuil hverk...@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 hverk...@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 

Re: Improving ov7670 sensor driver.

2012-09-13 Thread Hans Verkuil
On Thu 13 September 2012 13:19:14 javier Martin wrote:
 On 13 September 2012 13:00, Hans Verkuil hverk...@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
--
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


Re: Improving ov7670 sensor driver.

2012-09-13 Thread 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