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 = <&pck0>;
> > assigned-clock-rates = <2500>;
> >
> > + vsync-active = <0>;
> > + pclk-sample = <1>;
> > +
> > port {
> > ov7670_0: endpoint {
> > remote-endpoint = <&isi_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->