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 <jacopo+renesas@xxxxxxxxxx>
> > >
> > > ---
> > >
> > > 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_COM10 0x15 /* Control 10 */
> > > #define COM10_HSYNC 0x40 /* 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, &prop_val);
> > > + if (ret) {
> > > + if (ret != -EINVAL)
> > > + dev_err(dev, "Unable to parse property %s: %d\n",
> > > + prop_name, ret);
> > > + return ret;
> > > + }
> > > +
> > > + return prop_val;
> > > +}
> > > +
> > > static int ov7670_probe(struct i2c_client *client,
> > > const struct i2c_device_id *id)
> > > {
> > > @@ -1587,7 +1633,58 @@ static int ov7670_probe(struct i2c_client *client,
> > > v4l2_i2c_subdev_init(sd, client, &ov7670_ops);
> > >
> > > info->clock_speed = 30; /* default: a guess */
> > > - if (client->dev.platform_data) {
> > > +
> > > + if (IS_ENABLED(CONFIG_OF) && client->dev.of_node) {
> > > + /*
> > > + * Parse OF properties to initialize media bus configuration.
> > > + *
> > > + * Use sensor's default configuration if a property is not
> > > + * specified (ret == -EINVAL):
> > > + */
> > > + info->mbus_config = 0;
> > > +
> > > + ret = ov7670_parse_dt_prop(&client->dev, "hsync-active");
> > > + if (ret < 0 && ret != -EINVAL)
> > > + return ret;
> > > + else if (ret == 0)
> > > + info->mbus_config |= V4L2_MBUS_HSYNC_ACTIVE_LOW;
> > > + else
> > > + info->mbus_config |= V4L2_MBUS_HSYNC_ACTIVE_HIGH;
> > > +
> > > + ret = ov7670_parse_dt_prop(&client->dev, "vsync-active");
> > > + if (ret < 0 && ret != -EINVAL)
> > > + return ret;
> > > + else if (ret == 0)
> > > + info->mbus_config |= V4L2_MBUS_VSYNC_ACTIVE_LOW;
> > > + else
> > > + info->mbus_config |= V4L2_MBUS_VSYNC_ACTIVE_HIGH;
> > > +
> > > + ret = ov7670_parse_dt_prop(&client->dev, "pclk-sample");
> > > + if (ret < 0 && ret != -EINVAL)
> > > + return ret;
> > > + else if (ret > 0)
> > > + info->mbus_config |= V4L2_MBUS_PCLK_SAMPLE_RISING;
> > > + else
> > > + info->mbus_config |= V4L2_MBUS_PCLK_SAMPLE_FALLING;
> > > +
> > > + ret = ov7670_parse_dt_prop(&client->dev,
> > > + "ov7670,pclk-hb-disable");
> > > + if (ret < 0 && ret != -EINVAL)
> > > + return ret;
> > > + else if (ret > 0)
> > > + info->pclk_hb_disable = true;
> > > + else
> > > + info->pclk_hb_disable = false;
> > > +
> > > + ret = ov7670_parse_dt_prop(&client->dev, "ov7670,pll-bypass");
> > > + if (ret < 0 && ret != -EINVAL)
> > > + return ret;
> > > + else if (ret > 0)
> > > + info->pll_bypass = true;
> > > + else
> > > + info->pll_bypass = false;
> > > +
> > > + } else if (client->dev.platform_data) {
> > > struct ov7670_config *config = client->dev.platform_data;
> > >
> > > /*
> > > @@ -1649,9 +1746,6 @@ static int ov7670_probe(struct i2c_client *client,
> > > tpf.denominator = 30;
> > > info->devtype->set_framerate(sd, &tpf);
> > >
> > > - if (info->pclk_hb_disable)
> > > - ov7670_write(sd, REG_COM10, COM10_PCLK_HB);
> > > -
> > > v4l2_ctrl_handler_init(&info->hdl, 10);
> > > v4l2_ctrl_new_std(&info->hdl, &ov7670_ctrl_ops,
> > > V4L2_CID_BRIGHTNESS, 0, 255, 1, 128);
> > > --
> > > 2.7.4
> > >
> >
> > --
> > Sakari Ailus
> > e-mail: sakari.ailus@xxxxxx
>
> --
> Sakari Ailus
> e-mail: sakari.ailus@xxxxxx
