Re: [RFC] v4l: i2c: ov7670: Implement mbus configuration

[jacopo mondi]

Hi Sakari,
   thanks for the reply

On Wed, Nov 29, 2017 at 01:06:49PM +0200, Sakari Ailus wrote:
> On Wed, Nov 29, 2017 at 01:04:30PM +0200, Sakari Ailus wrote:
> > Hi Jacopo,
> >
> > On Mon, Nov 27, 2017 at 11:26:53AM +0100, Jacopo Mondi wrote:
> > > ov7670 currently supports configuration of a few parameters only through
> > > platform data. Implement media bus configuration by parsing DT properties
> > > at probe() time and opportunely configure REG_COM10 during s_format().
> > >
> > > Signed-off-by: Jacopo Mondi 
> > >
> > > ---
> > >
> > > Hi linux-media,
> > >I'm using this sensor to test the CEU driver I have submitted some 
> > > time ago
> > > and I would like to change synchronization signal polarities to test them 
> > > in
> > > combination with that driver.
> > >
> > > So I added support for retrieving some properties listed in the device 
> > > tree
> > > bindings documentation from sensor's DT node and made a patch, BUT I'm
> > > slightly confused about this (and that's why this is an RFC).
> > >
> > > I did a grep for "sync-active" in drivers/media/i2c/ and no sensor driver
> > > implements any property parsing, so I guess I'm doing something wrong 
> > > here.
> >
> > :-)
> >
> > The standard properties are parsed in the V4L2 fwnode framework, and
> > gathered to v4l2_fwnode_endpoint struct for drivers to use. Please see e.g.
> > the smiapp driver how to do this.
> >
> > You'll still need to parse device specific properties in the driver.

I totally misinterpreted this!

My understanding was that v4l2_fwnode_endpoint_parse() was supposed to
be used to parse the -remote- endpoint configuration, so that a
platform driver would know how the sensor is configured and adjust its
settings accordingly (and eventually configure the remote endpoint with
s_mbus_confi()).

Instead, if I got this right, -both- sensor and platform parse
their own endpoints, and they don't care how the remote is configured
because of, as you said, wirings..

I'm going to re-submit this using the v4l2_fwnode framework utilities
in the sensor driver!

Thanks for the clarification
   j

> >
> > >
> > > I thought that maybe sensor media bus configuration should come from the
> > > platform driver, through the s_mbus_config() operation in 
> > > v4l2_subdev_video_ops,
>
> It's specified in the sensor's local endpoint. The corresponding
> configuration needs to be present in the remote endpoint. These are not
> necessarily always the same due to e.g. wiring.
>
> > > but that's said to be deprecated. So maybe is the framework providing 
> > > support
> > > for parsing those properties? Another grep there and I found only 
> > > v4l2-fwnode.c
> > > has support for parsing serial/parallel bus properties, but my 
> > > understanding is
> > > that those functions are meant to be used by the platform driver when
> > > parsing the remote fw node.
> > >
> > > So please help me out here: where should I implement media bus 
> > > configuration
> > > for sensor drivers?
> > >
> > > Thanks
> > >j
> > >
> > > PS: being this just an RFC I have not updated dt bindings, and only
> > > compile-tested the patch
> > >
> > > ---
> > >  drivers/media/i2c/ov7670.c | 108 
> > > ++---
> > >  1 file changed, 101 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c
> > > index e88549f..7e2de7e 100644
> > > --- a/drivers/media/i2c/ov7670.c
> > > +++ b/drivers/media/i2c/ov7670.c
> > > @@ -88,6 +88,7 @@ MODULE_PARM_DESC(debug, "Debug level (0-1)");
> > >  #define REG_COM100x15/* Control 10 */
> > >  #define   COM10_HSYNC  0x40/* HSYNC instead of HREF */
> > >  #define   COM10_PCLK_HB0x20/* Suppress PCLK on horiz blank */
> > > +#define   COM10_PCLK_REV  0x10 /* Latch data on PCLK rising edge */
> > >  #define   COM10_HREF_REV  0x08 /* Reverse HREF */
> > >  #define   COM10_VS_LEAD0x04/* VSYNC on clock leading edge */
> > >  #define   COM10_VS_NEG 0x02/* VSYNC negative */
> > > @@ -233,6 +234,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) */
> > > @@ -985,7 +987,7 @@ static int ov7670_set_fmt(struct v4l2_subdev *sd,
> > >   struct ov7670_format_struct *ovfmt;
> > >   struct ov7670_win_size *wsize;
> > >   struct ov7670_info *info = to_state(sd);
> > > - unsigned char com7;
> > > + unsigned char com7, com10;
> > >   int ret;
> > >
> > >   if (format->pad)
> > > @@ -1021,6 +1023,9 @@ static int ov7670_set_fmt(struct v4l2_subdev *sd,
> > >   ret = 0;
> > >   if (wsize->regs)
> > >   ret = 

Re: [RFC] v4l: i2c: ov7670: Implement mbus configuration

[Sakari Ailus]

On Wed, Nov 29, 2017 at 01:04:30PM +0200, Sakari Ailus wrote:
> Hi Jacopo,
> 
> On Mon, Nov 27, 2017 at 11:26:53AM +0100, Jacopo Mondi wrote:
> > ov7670 currently supports configuration of a few parameters only through
> > platform data. Implement media bus configuration by parsing DT properties
> > at probe() time and opportunely configure REG_COM10 during s_format().
> > 
> > Signed-off-by: Jacopo Mondi 
> > 
> > ---
> > 
> > Hi linux-media,
> >I'm using this sensor to test the CEU driver I have submitted some time 
> > ago
> > and I would like to change synchronization signal polarities to test them in
> > combination with that driver.
> > 
> > So I added support for retrieving some properties listed in the device tree
> > bindings documentation from sensor's DT node and made a patch, BUT I'm
> > slightly confused about this (and that's why this is an RFC).
> > 
> > I did a grep for "sync-active" in drivers/media/i2c/ and no sensor driver
> > implements any property parsing, so I guess I'm doing something wrong here.
> 
> :-)
> 
> The standard properties are parsed in the V4L2 fwnode framework, and
> gathered to v4l2_fwnode_endpoint struct for drivers to use. Please see e.g.
> the smiapp driver how to do this.
> 
> You'll still need to parse device specific properties in the driver.
> 
> > 
> > I thought that maybe sensor media bus configuration should come from the
> > platform driver, through the s_mbus_config() operation in 
> > v4l2_subdev_video_ops,

It's specified in the sensor's local endpoint. The corresponding
configuration needs to be present in the remote endpoint. These are not
necessarily always the same due to e.g. wiring.

> > but that's said to be deprecated. So maybe is the framework providing 
> > support
> > for parsing those properties? Another grep there and I found only 
> > v4l2-fwnode.c
> > has support for parsing serial/parallel bus properties, but my 
> > understanding is
> > that those functions are meant to be used by the platform driver when
> > parsing the remote fw node.
> > 
> > So please help me out here: where should I implement media bus configuration
> > for sensor drivers?
> > 
> > Thanks
> >j
> > 
> > PS: being this just an RFC I have not updated dt bindings, and only
> > compile-tested the patch
> > 
> > ---
> >  drivers/media/i2c/ov7670.c | 108 
> > ++---
> >  1 file changed, 101 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c
> > index e88549f..7e2de7e 100644
> > --- a/drivers/media/i2c/ov7670.c
> > +++ b/drivers/media/i2c/ov7670.c
> > @@ -88,6 +88,7 @@ MODULE_PARM_DESC(debug, "Debug level (0-1)");
> >  #define REG_COM10  0x15/* Control 10 */
> >  #define   COM10_HSYNC0x40/* HSYNC instead of HREF */
> >  #define   COM10_PCLK_HB  0x20/* Suppress PCLK on horiz blank */
> > +#define   COM10_PCLK_REV  0x10   /* Latch data on PCLK rising edge */
> >  #define   COM10_HREF_REV  0x08   /* Reverse HREF */
> >  #define   COM10_VS_LEAD  0x04/* VSYNC on clock leading edge */
> >  #define   COM10_VS_NEG   0x02/* VSYNC negative */
> > @@ -233,6 +234,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) */
> > @@ -985,7 +987,7 @@ static int ov7670_set_fmt(struct v4l2_subdev *sd,
> > struct ov7670_format_struct *ovfmt;
> > struct ov7670_win_size *wsize;
> > struct ov7670_info *info = to_state(sd);
> > -   unsigned char com7;
> > +   unsigned char com7, com10;
> > int ret;
> > 
> > if (format->pad)
> > @@ -1021,6 +1023,9 @@ static int ov7670_set_fmt(struct v4l2_subdev *sd,
> > ret = 0;
> > if (wsize->regs)
> > ret = ov7670_write_array(sd, wsize->regs);
> > +   if (ret)
> > +   return ret;
> > +
> > info->fmt = ovfmt;
> > 
> > /*
> > @@ -1033,8 +1038,26 @@ 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;
> > +
> > +   /* Configure the media bus after the image format */
> > +   com10 = 0;
> > +   if (info->mbus_config & V4L2_MBUS_VSYNC_ACTIVE_LOW)
> > +   com10 |= COM10_VS_NEG;
> > +   if (info->mbus_config & V4L2_MBUS_HSYNC_ACTIVE_LOW)
> > +   com10 |= COM10_HS_NEG;
> > +   if (info->mbus_config & V4L2_MBUS_PCLK_SAMPLE_RISING)
> > +   com10 |= COM10_PCLK_REV;
> > +   if 

Re: [RFC] v4l: i2c: ov7670: Implement mbus configuration

[Sakari Ailus]

Hi Jacopo,

On Mon, Nov 27, 2017 at 11:26:53AM +0100, Jacopo Mondi wrote:
> ov7670 currently supports configuration of a few parameters only through
> platform data. Implement media bus configuration by parsing DT properties
> at probe() time and opportunely configure REG_COM10 during s_format().
> 
> Signed-off-by: Jacopo Mondi 
> 
> ---
> 
> Hi linux-media,
>I'm using this sensor to test the CEU driver I have submitted some time ago
> and I would like to change synchronization signal polarities to test them in
> combination with that driver.
> 
> So I added support for retrieving some properties listed in the device tree
> bindings documentation from sensor's DT node and made a patch, BUT I'm
> slightly confused about this (and that's why this is an RFC).
> 
> I did a grep for "sync-active" in drivers/media/i2c/ and no sensor driver
> implements any property parsing, so I guess I'm doing something wrong here.

:-)

The standard properties are parsed in the V4L2 fwnode framework, and
gathered to v4l2_fwnode_endpoint struct for drivers to use. Please see e.g.
the smiapp driver how to do this.

You'll still need to parse device specific properties in the driver.

> 
> I thought that maybe sensor media bus configuration should come from the
> platform driver, through the s_mbus_config() operation in 
> v4l2_subdev_video_ops,
> but that's said to be deprecated. So maybe is the framework providing support
> for parsing those properties? Another grep there and I found only 
> v4l2-fwnode.c
> has support for parsing serial/parallel bus properties, but my understanding 
> is
> that those functions are meant to be used by the platform driver when
> parsing the remote fw node.
> 
> So please help me out here: where should I implement media bus configuration
> for sensor drivers?
> 
> Thanks
>j
> 
> PS: being this just an RFC I have not updated dt bindings, and only
> compile-tested the patch
> 
> ---
>  drivers/media/i2c/ov7670.c | 108 
> ++---
>  1 file changed, 101 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c
> index e88549f..7e2de7e 100644
> --- a/drivers/media/i2c/ov7670.c
> +++ b/drivers/media/i2c/ov7670.c
> @@ -88,6 +88,7 @@ MODULE_PARM_DESC(debug, "Debug level (0-1)");
>  #define REG_COM100x15/* Control 10 */
>  #define   COM10_HSYNC  0x40/* HSYNC instead of HREF */
>  #define   COM10_PCLK_HB0x20/* Suppress PCLK on horiz blank */
> +#define   COM10_PCLK_REV  0x10 /* Latch data on PCLK rising edge */
>  #define   COM10_HREF_REV  0x08 /* Reverse HREF */
>  #define   COM10_VS_LEAD0x04/* VSYNC on clock leading edge */
>  #define   COM10_VS_NEG 0x02/* VSYNC negative */
> @@ -233,6 +234,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) */
> @@ -985,7 +987,7 @@ static int ov7670_set_fmt(struct v4l2_subdev *sd,
>   struct ov7670_format_struct *ovfmt;
>   struct ov7670_win_size *wsize;
>   struct ov7670_info *info = to_state(sd);
> - unsigned char com7;
> + unsigned char com7, com10;
>   int ret;
> 
>   if (format->pad)
> @@ -1021,6 +1023,9 @@ static int ov7670_set_fmt(struct v4l2_subdev *sd,
>   ret = 0;
>   if (wsize->regs)
>   ret = ov7670_write_array(sd, wsize->regs);
> + if (ret)
> + return ret;
> +
>   info->fmt = ovfmt;
> 
>   /*
> @@ -1033,8 +1038,26 @@ 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;
> +
> + /* Configure the media bus after the image format */
> + com10 = 0;
> + if (info->mbus_config & V4L2_MBUS_VSYNC_ACTIVE_LOW)
> + com10 |= COM10_VS_NEG;
> + if (info->mbus_config & V4L2_MBUS_HSYNC_ACTIVE_LOW)
> + com10 |= COM10_HS_NEG;
> + if (info->mbus_config & V4L2_MBUS_PCLK_SAMPLE_RISING)
> + com10 |= COM10_PCLK_REV;
> + if (info->pclk_hb_disable)
> + com10 |= COM10_PCLK_HB;
> +
> + if (com10)
> + ret = ov7670_write(sd, REG_COM10, com10);
> + if (ret)
> + return ret;
> +
>   return 0;
>  }
> 
> @@ -1572,6 +1595,29 @@ static int ov7670_init_gpio(struct i2c_client *client, 
> struct ov7670_info *info)
>   return 0;
>  }
> 
> +/**
> + * ov7670_parse_dt_prop() - parse 

[RFC] v4l: i2c: ov7670: Implement mbus configuration

[Jacopo Mondi]

ov7670 currently supports configuration of a few parameters only through
platform data. Implement media bus configuration by parsing DT properties
at probe() time and opportunely configure REG_COM10 during s_format().

Signed-off-by: Jacopo Mondi 

---

Hi linux-media,
   I'm using this sensor to test the CEU driver I have submitted some time ago
and I would like to change synchronization signal polarities to test them in
combination with that driver.

So I added support for retrieving some properties listed in the device tree
bindings documentation from sensor's DT node and made a patch, BUT I'm
slightly confused about this (and that's why this is an RFC).

I did a grep for "sync-active" in drivers/media/i2c/ and no sensor driver
implements any property parsing, so I guess I'm doing something wrong here.

I thought that maybe sensor media bus configuration should come from the
platform driver, through the s_mbus_config() operation in v4l2_subdev_video_ops,
but that's said to be deprecated. So maybe is the framework providing support
for parsing those properties? Another grep there and I found only v4l2-fwnode.c
has support for parsing serial/parallel bus properties, but my understanding is
that those functions are meant to be used by the platform driver when
parsing the remote fw node.

So please help me out here: where should I implement media bus configuration
for sensor drivers?

Thanks
   j

PS: being this just an RFC I have not updated dt bindings, and only
compile-tested the patch

---
 drivers/media/i2c/ov7670.c | 108 ++---
 1 file changed, 101 insertions(+), 7 deletions(-)

diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c
index e88549f..7e2de7e 100644
--- a/drivers/media/i2c/ov7670.c
+++ b/drivers/media/i2c/ov7670.c
@@ -88,6 +88,7 @@ MODULE_PARM_DESC(debug, "Debug level (0-1)");
 #define REG_COM10  0x15/* Control 10 */
 #define   COM10_HSYNC0x40/* HSYNC instead of HREF */
 #define   COM10_PCLK_HB  0x20/* Suppress PCLK on horiz blank */
+#define   COM10_PCLK_REV  0x10   /* Latch data on PCLK rising edge */
 #define   COM10_HREF_REV  0x08   /* Reverse HREF */
 #define   COM10_VS_LEAD  0x04/* VSYNC on clock leading edge */
 #define   COM10_VS_NEG   0x02/* VSYNC negative */
@@ -233,6 +234,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) */
@@ -985,7 +987,7 @@ static int ov7670_set_fmt(struct v4l2_subdev *sd,
struct ov7670_format_struct *ovfmt;
struct ov7670_win_size *wsize;
struct ov7670_info *info = to_state(sd);
-   unsigned char com7;
+   unsigned char com7, com10;
int ret;

if (format->pad)
@@ -1021,6 +1023,9 @@ static int ov7670_set_fmt(struct v4l2_subdev *sd,
ret = 0;
if (wsize->regs)
ret = ov7670_write_array(sd, wsize->regs);
+   if (ret)
+   return ret;
+
info->fmt = ovfmt;

/*
@@ -1033,8 +1038,26 @@ 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;
+
+   /* Configure the media bus after the image format */
+   com10 = 0;
+   if (info->mbus_config & V4L2_MBUS_VSYNC_ACTIVE_LOW)
+   com10 |= COM10_VS_NEG;
+   if (info->mbus_config & V4L2_MBUS_HSYNC_ACTIVE_LOW)
+   com10 |= COM10_HS_NEG;
+   if (info->mbus_config & V4L2_MBUS_PCLK_SAMPLE_RISING)
+   com10 |= COM10_PCLK_REV;
+   if (info->pclk_hb_disable)
+   com10 |= COM10_PCLK_HB;
+
+   if (com10)
+   ret = ov7670_write(sd, REG_COM10, com10);
+   if (ret)
+   return ret;
+
return 0;
 }

@@ -1572,6 +1595,29 @@ static int ov7670_init_gpio(struct i2c_client *client, 
struct ov7670_info *info)
return 0;
 }

+/**
+ * ov7670_parse_dt_prop() - parse property "prop_name" in OF node
+ *
+ * @return The property value or < 0 if property not present
+ *or wrongly specified.
+ */
+static int ov7670_parse_dt_prop(struct device *dev, char *prop_name)
+{
+   struct device_node *np = dev->of_node;
+   u32 prop_val;
+   int ret;
+
+   ret = of_property_read_u32(np, prop_name, _val);
+   if (ret) {
+   if (ret != -EINVAL)
+   dev_err(dev, "Unable to parse property %s: %d\n",
+