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 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_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
