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 <[email protected]>
> >
> > ---
> >
> > 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_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: [email protected]
--
Sakari Ailus
e-mail: [email protected]
