subject:"Re\: \[PATCH v4 2\/2\] v4l2\: i2c\: ov7670\: Implement OF mbus configuration"

Re: [PATCH v4 2/2] v4l2: i2c: ov7670: Implement OF mbus configuration

2018-02-19 Thread Sakari Ailus

On Wed, Jan 24, 2018 at 10:30:50AM +0100, Jacopo Mondi wrote:
> ov7670 driver supports two optional properties supplied through platform
> data, but currently does not support any standard video interface
> property.
> 
> Add support through OF parsing for 2 generic properties (vsync and hsync
> polarities) and for one custom property already supported through
> platform data to suppress pixel clock output during horizontal
> blanking.
> 
> While at there, check return value of register writes in set_fmt
> function and rationalize spacings.
> 
> Signal polarities and pixel clock blanking verified through scope and
> image capture.
> 
> Signed-off-by: Jacopo Mondi 

Hi Jacopo,

I'll add the following diff to the patch in order to fix the build error
without V4L2_FWNODE:

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 535f85b73075..245387f7b65e 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -683,6 +683,7 @@ config VIDEO_OV7670
tristate "OmniVision OV7670 sensor support"
depends on I2C && VIDEO_V4L2
depends on MEDIA_CAMERA_SUPPORT
+   select V4L2_FWNODE
---help---
  This is a Video4Linux2 sensor-level driver for the OmniVision
  OV7670 VGA camera.  It currently only works with the M88ALP01

> ---
>  drivers/media/i2c/ov7670.c | 98 
> +++---
>  1 file changed, 84 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c
> index 61c472e..80c822c 100644
> --- a/drivers/media/i2c/ov7670.c
> +++ b/drivers/media/i2c/ov7670.c
> @@ -21,6 +21,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -242,6 +243,7 @@ struct ov7670_info {
>   struct clk *clk;
>   struct gpio_desc *resetb_gpio;
>   struct gpio_desc *pwdn_gpio;
> + unsigned int mbus_config;   /* Media bus configuration flags */
>   int min_width;  /* Filter out smaller sizes */
>   int min_height; /* Filter out smaller sizes */
>   int clock_speed;/* External clock speed (MHz) */
> @@ -1018,7 +1020,7 @@ static int ov7670_set_fmt(struct v4l2_subdev *sd,
>  #ifdef CONFIG_VIDEO_V4L2_SUBDEV_API
>   struct v4l2_mbus_framefmt *mbus_fmt;
>  #endif
> - unsigned char com7;
> + unsigned char com7, com10 = 0;
>   int ret;
>  
>   if (format->pad)
> @@ -1038,7 +1040,6 @@ static int ov7670_set_fmt(struct v4l2_subdev *sd,
>   }
>  
>   ret = ov7670_try_fmt_internal(sd, >format, , );
> -
>   if (ret)
>   return ret;
>   /*
> @@ -1049,16 +1050,41 @@ static int ov7670_set_fmt(struct v4l2_subdev *sd,
>*/
>   com7 = ovfmt->regs[0].value;
>   com7 |= wsize->com7_bit;
> - ov7670_write(sd, REG_COM7, com7);
> + ret = ov7670_write(sd, REG_COM7, com7);
> + if (ret)
> + return ret;
> +
> + /*
> +  * Configure the media bus through COM10 register
> +  */
> + if (info->mbus_config & V4L2_MBUS_VSYNC_ACTIVE_LOW)
> + com10 |= COM10_VS_NEG;
> + if (info->mbus_config & V4L2_MBUS_HSYNC_ACTIVE_LOW)
> + com10 |= COM10_HREF_REV;
> + if (info->pclk_hb_disable)
> + com10 |= COM10_PCLK_HB;
> + ret = ov7670_write(sd, REG_COM10, com10);
> + if (ret)
> + return ret;
> +
>   /*
>* Now write the rest of the array.  Also store start/stops
>*/
> - ov7670_write_array(sd, ovfmt->regs + 1);
> - ov7670_set_hw(sd, wsize->hstart, wsize->hstop, wsize->vstart,
> - wsize->vstop);
> - ret = 0;
> - if (wsize->regs)
> + ret = ov7670_write_array(sd, ovfmt->regs + 1);
> + if (ret)
> + return ret;
> +
> + ret = ov7670_set_hw(sd, wsize->hstart, wsize->hstop, wsize->vstart,
> + wsize->vstop);
> + if (ret)
> + return ret;
> +
> + if (wsize->regs) {
>   ret = ov7670_write_array(sd, wsize->regs);
> + if (ret)
> + return ret;
> + }
> +
>   info->fmt = ovfmt;
>  
>   /*
> @@ -1071,8 +1097,10 @@ static int ov7670_set_fmt(struct v4l2_subdev *sd,
>* to write it unconditionally, and that will make the frame
>* rate persistent too.
>*/
> - if (ret == 0)
> - ret = ov7670_write(sd, REG_CLKRC, info->clkrc);
> + ret = ov7670_write(sd, REG_CLKRC, info->clkrc);
> + if (ret)
> + return ret;
> +
>   return 0;
>  }
>  
> @@ -1698,6 +1726,45 @@ static int ov7670_init_gpio(struct i2c_client *client, 
> struct ov7670_info *info)
>   return 0;
>  }
>  
> +/*
> + * ov7670_parse_dt() - Parse device tree to collect mbus configuration
> + *   properties
> + */
> +static int ov7670_parse_dt(struct device *dev,
> +struct ov7670_info *info)
> +{
> + struct

Re: [PATCH v4 2/2] v4l2: i2c: ov7670: Implement OF mbus configuration

2018-02-19 Thread Sakari Ailus

On Wed, Jan 24, 2018 at 10:30:50AM +0100, Jacopo Mondi wrote:
> ov7670 driver supports two optional properties supplied through platform
> data, but currently does not support any standard video interface
> property.
> 
> Add support through OF parsing for 2 generic properties (vsync and hsync
> polarities) and for one custom property already supported through
> platform data to suppress pixel clock output during horizontal
> blanking.
> 
> While at there, check return value of register writes in set_fmt
> function and rationalize spacings.
> 
> Signal polarities and pixel clock blanking verified through scope and
> image capture.
> 
> Signed-off-by: Jacopo Mondi 

Hi Jacopo,

I'll add the following diff to the patch in order to fix the build error
without V4L2_FWNODE:

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 535f85b73075..245387f7b65e 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -683,6 +683,7 @@ config VIDEO_OV7670
tristate "OmniVision OV7670 sensor support"
depends on I2C && VIDEO_V4L2
depends on MEDIA_CAMERA_SUPPORT
+   select V4L2_FWNODE
---help---
  This is a Video4Linux2 sensor-level driver for the OmniVision
  OV7670 VGA camera.  It currently only works with the M88ALP01

> ---
>  drivers/media/i2c/ov7670.c | 98 
> +++---
>  1 file changed, 84 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c
> index 61c472e..80c822c 100644
> --- a/drivers/media/i2c/ov7670.c
> +++ b/drivers/media/i2c/ov7670.c
> @@ -21,6 +21,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -242,6 +243,7 @@ struct ov7670_info {
>   struct clk *clk;
>   struct gpio_desc *resetb_gpio;
>   struct gpio_desc *pwdn_gpio;
> + unsigned int mbus_config;   /* Media bus configuration flags */
>   int min_width;  /* Filter out smaller sizes */
>   int min_height; /* Filter out smaller sizes */
>   int clock_speed;/* External clock speed (MHz) */
> @@ -1018,7 +1020,7 @@ static int ov7670_set_fmt(struct v4l2_subdev *sd,
>  #ifdef CONFIG_VIDEO_V4L2_SUBDEV_API
>   struct v4l2_mbus_framefmt *mbus_fmt;
>  #endif
> - unsigned char com7;
> + unsigned char com7, com10 = 0;
>   int ret;
>  
>   if (format->pad)
> @@ -1038,7 +1040,6 @@ static int ov7670_set_fmt(struct v4l2_subdev *sd,
>   }
>  
>   ret = ov7670_try_fmt_internal(sd, >format, , );
> -
>   if (ret)
>   return ret;
>   /*
> @@ -1049,16 +1050,41 @@ static int ov7670_set_fmt(struct v4l2_subdev *sd,
>*/
>   com7 = ovfmt->regs[0].value;
>   com7 |= wsize->com7_bit;
> - ov7670_write(sd, REG_COM7, com7);
> + ret = ov7670_write(sd, REG_COM7, com7);
> + if (ret)
> + return ret;
> +
> + /*
> +  * Configure the media bus through COM10 register
> +  */
> + if (info->mbus_config & V4L2_MBUS_VSYNC_ACTIVE_LOW)
> + com10 |= COM10_VS_NEG;
> + if (info->mbus_config & V4L2_MBUS_HSYNC_ACTIVE_LOW)
> + com10 |= COM10_HREF_REV;
> + if (info->pclk_hb_disable)
> + com10 |= COM10_PCLK_HB;
> + ret = ov7670_write(sd, REG_COM10, com10);
> + if (ret)
> + return ret;
> +
>   /*
>* Now write the rest of the array.  Also store start/stops
>*/
> - ov7670_write_array(sd, ovfmt->regs + 1);
> - ov7670_set_hw(sd, wsize->hstart, wsize->hstop, wsize->vstart,
> - wsize->vstop);
> - ret = 0;
> - if (wsize->regs)
> + ret = ov7670_write_array(sd, ovfmt->regs + 1);
> + if (ret)
> + return ret;
> +
> + ret = ov7670_set_hw(sd, wsize->hstart, wsize->hstop, wsize->vstart,
> + wsize->vstop);
> + if (ret)
> + return ret;
> +
> + if (wsize->regs) {
>   ret = ov7670_write_array(sd, wsize->regs);
> + if (ret)
> + return ret;
> + }
> +
>   info->fmt = ovfmt;
>  
>   /*
> @@ -1071,8 +1097,10 @@ static int ov7670_set_fmt(struct v4l2_subdev *sd,
>* to write it unconditionally, and that will make the frame
>* rate persistent too.
>*/
> - if (ret == 0)
> - ret = ov7670_write(sd, REG_CLKRC, info->clkrc);
> + ret = ov7670_write(sd, REG_CLKRC, info->clkrc);
> + if (ret)
> + return ret;
> +
>   return 0;
>  }
>  
> @@ -1698,6 +1726,45 @@ static int ov7670_init_gpio(struct i2c_client *client, 
> struct ov7670_info *info)
>   return 0;
>  }
>  
> +/*
> + * ov7670_parse_dt() - Parse device tree to collect mbus configuration
> + *   properties
> + */
> +static int ov7670_parse_dt(struct device *dev,
> +struct ov7670_info *info)
> +{
> + struct fwnode_handle *fwnode =

Re: [PATCH v4 2/2] v4l2: i2c: ov7670: Implement OF mbus configuration

Re: [PATCH v4 2/2] v4l2: i2c: ov7670: Implement OF mbus configuration

2 matches

Site Navigation

Mail list logo

Footer information