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

[jacopo mondi]

Hi Sakari,
thanks for review..

On Wed, Jan 03, 2018 at 12:11:32PM +0200, Sakari Ailus wrote:
> Hi Jacopo,
>
> Please see my comments below.
>
> On Tue, Jan 02, 2018 at 04:03:53PM +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 two custom properties already supported by platform
> > data.
> >
> > Signed-off-by: Jacopo Mondi 
> > ---
> >
> > I have made sure signal polarities gets properly changed using a scope and
> > capturing images with negative polarities using CEU capture interface.
> > Also verified with a scope that pixel clock gets suppressed on horizontal
> > blankings as well when "ov7670,pclk-hb-disable" property is specified.
> >
> > Thanks
> >j
> > ---
> >
> >  .../devicetree/bindings/media/i2c/ov7670.txt   |  12 +++
> >  drivers/media/i2c/ov7670.c | 101 
> > +++--
> >  2 files changed, 106 insertions(+), 7 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/media/i2c/ov7670.txt 
> > b/Documentation/devicetree/bindings/media/i2c/ov7670.txt
> > index 826b656..c005453 100644
> > --- a/Documentation/devicetree/bindings/media/i2c/ov7670.txt
> > +++ b/Documentation/devicetree/bindings/media/i2c/ov7670.txt
> > @@ -9,11 +9,20 @@ Required Properties:
> >  - clocks: reference to the xclk input clock.
> >  - clock-names: should be "xclk".
> >
> > +The following properties, as defined by "video-interfaces.txt", are 
> > supported:
> > +- hsync-active: active state of the HSYNC signal, 0/1 for LOW/HIGH 
> > respectively.
> > +- vsync-active: active state of the VSYNC signal, 0/1 for LOW/HIGH 
> > respectively.
> > +
> > +Default is high active state for both vsync and hsync signals.
> > +
> >  Optional Properties:
> >  - reset-gpios: reference to the GPIO connected to the resetb pin, if any.
> >Active is low.
> >  - powerdown-gpios: reference to the GPIO connected to the pwdn pin, if any.
> >Active is high.
> > +- ov7670,pll-bypass: set to 1 to bypass PLL for pixel clock generation.
>
> Is this something that should be specified using a device specific
> property?
>
> It looks like a rather crude way to configure the sensor's internal clock
> tree. Is this something that could be deduced from the external clock
> frequency? The check for the clock frequency seems very coarse and more or
> less satisfies the device's input clock frequency limits (between 10 and 48
> MHz).

Well, it was a platform data option, I barely replicated it in OF
bindings.

It has been added by this commit:
https://patchwork.kernel.org/patch/1515031/
and it applies to ov7675 only.

Do you think it is not the case to make a property of it?

>
> > +- ov7670,pclk-hb-disable: set to 1 to suppress pixel clock output signal 
> > during
> > +  horizontal blankings.
>
> Please separate the DT bindings from driver changes, and cc the devicetree
> list.
>
> >
> >  The device node must contain one 'port' child node for its digital output
> >  video port, in accordance with the video interface bindings defined in
> > @@ -34,6 +43,9 @@ Example:
> > assigned-clocks = <>;
> > assigned-clock-rates = <2500>;
> >
> > +   vsync-active = <0>;
> > +   pclk-sample = <1>;
> > +
> > port {
> > ov7670_0: endpoint {
> > remote-endpoint = <_0>;
> > diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c
> > index 950a0ac..a42bee7 100644
> > --- a/drivers/media/i2c/ov7670.c
> > +++ b/drivers/media/i2c/ov7670.c
> > @@ -11,6 +11,7 @@
> >   * Public License, version 2.
> >   */
> >  #include 
> > +#include 
>
> media/v4l2-fwnode.h includes linux/fwnode.h.
>
> >  #include 
> >  #include 
> >  #include 
> > @@ -21,6 +22,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> > @@ -237,6 +239,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) */
> > @@ -995,7 +998,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)
> > @@ -1027,6 +1030,18 @@ static int ov7670_set_fmt(struct v4l2_subdev *sd,
> > com7 = ovfmt->regs[0].value;
> >  

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

[jacopo mondi]

Hi Sakari,
thanks for review..

On Wed, Jan 03, 2018 at 12:11:32PM +0200, Sakari Ailus wrote:
> Hi Jacopo,
>
> Please see my comments below.
>
> On Tue, Jan 02, 2018 at 04:03:53PM +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 two custom properties already supported by platform
> > data.
> >
> > Signed-off-by: Jacopo Mondi 
> > ---
> >
> > I have made sure signal polarities gets properly changed using a scope and
> > capturing images with negative polarities using CEU capture interface.
> > Also verified with a scope that pixel clock gets suppressed on horizontal
> > blankings as well when "ov7670,pclk-hb-disable" property is specified.
> >
> > Thanks
> >j
> > ---
> >
> >  .../devicetree/bindings/media/i2c/ov7670.txt   |  12 +++
> >  drivers/media/i2c/ov7670.c | 101 
> > +++--
> >  2 files changed, 106 insertions(+), 7 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/media/i2c/ov7670.txt 
> > b/Documentation/devicetree/bindings/media/i2c/ov7670.txt
> > index 826b656..c005453 100644
> > --- a/Documentation/devicetree/bindings/media/i2c/ov7670.txt
> > +++ b/Documentation/devicetree/bindings/media/i2c/ov7670.txt
> > @@ -9,11 +9,20 @@ Required Properties:
> >  - clocks: reference to the xclk input clock.
> >  - clock-names: should be "xclk".
> >
> > +The following properties, as defined by "video-interfaces.txt", are 
> > supported:
> > +- hsync-active: active state of the HSYNC signal, 0/1 for LOW/HIGH 
> > respectively.
> > +- vsync-active: active state of the VSYNC signal, 0/1 for LOW/HIGH 
> > respectively.
> > +
> > +Default is high active state for both vsync and hsync signals.
> > +
> >  Optional Properties:
> >  - reset-gpios: reference to the GPIO connected to the resetb pin, if any.
> >Active is low.
> >  - powerdown-gpios: reference to the GPIO connected to the pwdn pin, if any.
> >Active is high.
> > +- ov7670,pll-bypass: set to 1 to bypass PLL for pixel clock generation.
>
> Is this something that should be specified using a device specific
> property?
>
> It looks like a rather crude way to configure the sensor's internal clock
> tree. Is this something that could be deduced from the external clock
> frequency? The check for the clock frequency seems very coarse and more or
> less satisfies the device's input clock frequency limits (between 10 and 48
> MHz).

Well, it was a platform data option, I barely replicated it in OF
bindings.

It has been added by this commit:
https://patchwork.kernel.org/patch/1515031/
and it applies to ov7675 only.

Do you think it is not the case to make a property of it?

>
> > +- ov7670,pclk-hb-disable: set to 1 to suppress pixel clock output signal 
> > during
> > +  horizontal blankings.
>
> Please separate the DT bindings from driver changes, and cc the devicetree
> list.
>
> >
> >  The device node must contain one 'port' child node for its digital output
> >  video port, in accordance with the video interface bindings defined in
> > @@ -34,6 +43,9 @@ Example:
> > assigned-clocks = <>;
> > assigned-clock-rates = <2500>;
> >
> > +   vsync-active = <0>;
> > +   pclk-sample = <1>;
> > +
> > port {
> > ov7670_0: endpoint {
> > remote-endpoint = <_0>;
> > diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c
> > index 950a0ac..a42bee7 100644
> > --- a/drivers/media/i2c/ov7670.c
> > +++ b/drivers/media/i2c/ov7670.c
> > @@ -11,6 +11,7 @@
> >   * Public License, version 2.
> >   */
> >  #include 
> > +#include 
>
> media/v4l2-fwnode.h includes linux/fwnode.h.
>
> >  #include 
> >  #include 
> >  #include 
> > @@ -21,6 +22,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> > @@ -237,6 +239,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) */
> > @@ -995,7 +998,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)
> > @@ -1027,6 +1030,18 @@ static int ov7670_set_fmt(struct v4l2_subdev *sd,
> > com7 = ovfmt->regs[0].value;
> > com7 |= 

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

[Sakari Ailus]

Hi Jacopo,

Please see my comments below.

On Tue, Jan 02, 2018 at 04:03:53PM +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 two custom properties already supported by platform
> data.
> 
> Signed-off-by: Jacopo Mondi 
> ---
> 
> I have made sure signal polarities gets properly changed using a scope and
> capturing images with negative polarities using CEU capture interface.
> Also verified with a scope that pixel clock gets suppressed on horizontal
> blankings as well when "ov7670,pclk-hb-disable" property is specified.
> 
> Thanks
>j
> ---
> 
>  .../devicetree/bindings/media/i2c/ov7670.txt   |  12 +++
>  drivers/media/i2c/ov7670.c | 101 
> +++--
>  2 files changed, 106 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/ov7670.txt 
> b/Documentation/devicetree/bindings/media/i2c/ov7670.txt
> index 826b656..c005453 100644
> --- a/Documentation/devicetree/bindings/media/i2c/ov7670.txt
> +++ b/Documentation/devicetree/bindings/media/i2c/ov7670.txt
> @@ -9,11 +9,20 @@ Required Properties:
>  - clocks: reference to the xclk input clock.
>  - clock-names: should be "xclk".
> 
> +The following properties, as defined by "video-interfaces.txt", are 
> supported:
> +- hsync-active: active state of the HSYNC signal, 0/1 for LOW/HIGH 
> respectively.
> +- vsync-active: active state of the VSYNC signal, 0/1 for LOW/HIGH 
> respectively.
> +
> +Default is high active state for both vsync and hsync signals.
> +
>  Optional Properties:
>  - reset-gpios: reference to the GPIO connected to the resetb pin, if any.
>Active is low.
>  - powerdown-gpios: reference to the GPIO connected to the pwdn pin, if any.
>Active is high.
> +- ov7670,pll-bypass: set to 1 to bypass PLL for pixel clock generation.

Is this something that should be specified using a device specific
property?

It looks like a rather crude way to configure the sensor's internal clock
tree. Is this something that could be deduced from the external clock
frequency? The check for the clock frequency seems very coarse and more or
less satisfies the device's input clock frequency limits (between 10 and 48
MHz).

> +- ov7670,pclk-hb-disable: set to 1 to suppress pixel clock output signal 
> during
> +  horizontal blankings.

Please separate the DT bindings from driver changes, and cc the devicetree
list.

> 
>  The device node must contain one 'port' child node for its digital output
>  video port, in accordance with the video interface bindings defined in
> @@ -34,6 +43,9 @@ Example:
>   assigned-clocks = <>;
>   assigned-clock-rates = <2500>;
> 
> + vsync-active = <0>;
> + pclk-sample = <1>;
> +
>   port {
>   ov7670_0: endpoint {
>   remote-endpoint = <_0>;
> diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c
> index 950a0ac..a42bee7 100644
> --- a/drivers/media/i2c/ov7670.c
> +++ b/drivers/media/i2c/ov7670.c
> @@ -11,6 +11,7 @@
>   * Public License, version 2.
>   */
>  #include 
> +#include 

media/v4l2-fwnode.h includes linux/fwnode.h.

>  #include 
>  #include 
>  #include 
> @@ -21,6 +22,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -237,6 +239,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) */
> @@ -995,7 +998,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)
> @@ -1027,6 +1030,18 @@ static int ov7670_set_fmt(struct v4l2_subdev *sd,
>   com7 = ovfmt->regs[0].value;
>   com7 |= wsize->com7_bit;
>   ov7670_write(sd, REG_COM7, com7);
> +
> + /*
> +  * 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;
> + ov7670_write(sd, REG_COM10, com10);

How about checking the return value here?

> +
>   /*
>

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

[Sakari Ailus]

Hi Jacopo,

Please see my comments below.

On Tue, Jan 02, 2018 at 04:03:53PM +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 two custom properties already supported by platform
> data.
> 
> Signed-off-by: Jacopo Mondi 
> ---
> 
> I have made sure signal polarities gets properly changed using a scope and
> capturing images with negative polarities using CEU capture interface.
> Also verified with a scope that pixel clock gets suppressed on horizontal
> blankings as well when "ov7670,pclk-hb-disable" property is specified.
> 
> Thanks
>j
> ---
> 
>  .../devicetree/bindings/media/i2c/ov7670.txt   |  12 +++
>  drivers/media/i2c/ov7670.c | 101 
> +++--
>  2 files changed, 106 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/ov7670.txt 
> b/Documentation/devicetree/bindings/media/i2c/ov7670.txt
> index 826b656..c005453 100644
> --- a/Documentation/devicetree/bindings/media/i2c/ov7670.txt
> +++ b/Documentation/devicetree/bindings/media/i2c/ov7670.txt
> @@ -9,11 +9,20 @@ Required Properties:
>  - clocks: reference to the xclk input clock.
>  - clock-names: should be "xclk".
> 
> +The following properties, as defined by "video-interfaces.txt", are 
> supported:
> +- hsync-active: active state of the HSYNC signal, 0/1 for LOW/HIGH 
> respectively.
> +- vsync-active: active state of the VSYNC signal, 0/1 for LOW/HIGH 
> respectively.
> +
> +Default is high active state for both vsync and hsync signals.
> +
>  Optional Properties:
>  - reset-gpios: reference to the GPIO connected to the resetb pin, if any.
>Active is low.
>  - powerdown-gpios: reference to the GPIO connected to the pwdn pin, if any.
>Active is high.
> +- ov7670,pll-bypass: set to 1 to bypass PLL for pixel clock generation.

Is this something that should be specified using a device specific
property?

It looks like a rather crude way to configure the sensor's internal clock
tree. Is this something that could be deduced from the external clock
frequency? The check for the clock frequency seems very coarse and more or
less satisfies the device's input clock frequency limits (between 10 and 48
MHz).

> +- ov7670,pclk-hb-disable: set to 1 to suppress pixel clock output signal 
> during
> +  horizontal blankings.

Please separate the DT bindings from driver changes, and cc the devicetree
list.

> 
>  The device node must contain one 'port' child node for its digital output
>  video port, in accordance with the video interface bindings defined in
> @@ -34,6 +43,9 @@ Example:
>   assigned-clocks = <>;
>   assigned-clock-rates = <2500>;
> 
> + vsync-active = <0>;
> + pclk-sample = <1>;
> +
>   port {
>   ov7670_0: endpoint {
>   remote-endpoint = <_0>;
> diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c
> index 950a0ac..a42bee7 100644
> --- a/drivers/media/i2c/ov7670.c
> +++ b/drivers/media/i2c/ov7670.c
> @@ -11,6 +11,7 @@
>   * Public License, version 2.
>   */
>  #include 
> +#include 

media/v4l2-fwnode.h includes linux/fwnode.h.

>  #include 
>  #include 
>  #include 
> @@ -21,6 +22,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -237,6 +239,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) */
> @@ -995,7 +998,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)
> @@ -1027,6 +1030,18 @@ static int ov7670_set_fmt(struct v4l2_subdev *sd,
>   com7 = ovfmt->regs[0].value;
>   com7 |= wsize->com7_bit;
>   ov7670_write(sd, REG_COM7, com7);
> +
> + /*
> +  * 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;
> + ov7670_write(sd, REG_COM10, com10);

How about checking the return value here?

> +
>   /*
>* Now write the rest