Re: [PATCHv3 05/15] ov7670: add devicetree support

[Sakari Ailus]

On Fri, Mar 10, 2017 at 10:32:41AM +0100, Hans Verkuil wrote:
> On 09/03/17 21:45, Sakari Ailus wrote:
> > Hi Hans,
> > 
> > On Mon, Mar 06, 2017 at 03:56:06PM +0100, Hans Verkuil wrote:
> >> From: Hans Verkuil 
> >>
> >> Add DT support. Use it to get the reset and pwdn pins (if there are any).
> >> Tested with one sensor requiring reset/pwdn and one sensor that doesn't
> >> have reset/pwdn pins.
> > 
> > If I read the datasheet right, lifting the reset signal up will reset the
> > sensor but the patch doesn't make use of that, it only ensures the reset
> > signal stays low. Should you lift it up for a while as well? The datasheet
> > doesn't say for how long that should be done, but that it should be usable
> > after 1 ms since pulling reset down.
> 
> There does not seem to be any need for that. This sensor also comes in two
> models: one with separate pwdn and reset pins, and one where it is just 
> hardwired.
> 
> If the hardwired variant doesn't need a reset pulse, then neither does the
> variant with pins. It works, and I am not really willing to experiment with 
> this.

Fine with me.

Acked-by: Sakari Ailus 

-- 
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk

Re: [PATCHv3 05/15] ov7670: add devicetree support

[Hans Verkuil]

On 09/03/17 21:45, Sakari Ailus wrote:
> Hi Hans,
> 
> On Mon, Mar 06, 2017 at 03:56:06PM +0100, Hans Verkuil wrote:
>> From: Hans Verkuil 
>>
>> Add DT support. Use it to get the reset and pwdn pins (if there are any).
>> Tested with one sensor requiring reset/pwdn and one sensor that doesn't
>> have reset/pwdn pins.
> 
> If I read the datasheet right, lifting the reset signal up will reset the
> sensor but the patch doesn't make use of that, it only ensures the reset
> signal stays low. Should you lift it up for a while as well? The datasheet
> doesn't say for how long that should be done, but that it should be usable
> after 1 ms since pulling reset down.

There does not seem to be any need for that. This sensor also comes in two
models: one with separate pwdn and reset pins, and one where it is just 
hardwired.

If the hardwired variant doesn't need a reset pulse, then neither does the
variant with pins. It works, and I am not really willing to experiment with 
this.

Regards,

Hans

> 
>>
>> Signed-off-by: Hans Verkuil 
>> ---
>>  drivers/media/i2c/ov7670.c | 40 ++--
>>  1 file changed, 38 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c
>> index da0843617a49..f9d977430dcf 100644
>> --- a/drivers/media/i2c/ov7670.c
>> +++ b/drivers/media/i2c/ov7670.c
>> @@ -17,6 +17,8 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>> +#include 
>>  #include 
>>  #include 
>>  #include 
>> @@ -229,6 +231,8 @@ struct ov7670_info {
>>  };
>>  struct ov7670_format_struct *fmt;  /* Current format */
>>  struct clk *clk;
>> +struct gpio_desc *resetb_gpio;
>> +struct gpio_desc *pwdn_gpio;
>>  int min_width;  /* Filter out smaller sizes */
>>  int min_height; /* Filter out smaller sizes */
>>  int clock_speed;/* External clock speed (MHz) */
>> @@ -591,8 +595,6 @@ static int ov7670_init(struct v4l2_subdev *sd, u32 val)
>>  return ov7670_write_array(sd, ov7670_default_regs);
>>  }
>>  
>> -
>> -
>>  static int ov7670_detect(struct v4l2_subdev *sd)
>>  {
>>  unsigned char v;
>> @@ -1549,6 +1551,27 @@ static const struct ov7670_devtype ov7670_devdata[] = 
>> {
>>  },
>>  };
>>  
>> +static int ov7670_init_gpio(struct i2c_client *client, struct ov7670_info 
>> *info)
>> +{
>> +info->pwdn_gpio = devm_gpiod_get_optional(>dev, "powerdown",
>> +GPIOD_OUT_LOW);
>> +if (IS_ERR(info->pwdn_gpio)) {
>> +dev_info(>dev, "can't get %s GPIO\n", "powerdown");
>> +return PTR_ERR(info->pwdn_gpio);
>> +}
>> +
>> +info->resetb_gpio = devm_gpiod_get_optional(>dev, "reset",
>> +GPIOD_OUT_LOW);
>> +if (IS_ERR(info->resetb_gpio)) {
>> +dev_info(>dev, "can't get %s GPIO\n", "reset");
>> +return PTR_ERR(info->resetb_gpio);
>> +}
>> +
>> +usleep_range(3000, 5000);
>> +
>> +return 0;
>> +}
>> +
>>  static int ov7670_probe(struct i2c_client *client,
>>  const struct i2c_device_id *id)
>>  {
>> @@ -1594,6 +1617,10 @@ static int ov7670_probe(struct i2c_client *client,
>>  return -EPROBE_DEFER;
>>  clk_prepare_enable(info->clk);
>>  
>> +ret = ov7670_init_gpio(client, info);
>> +if (ret)
>> +return ret;
>> +
>>  info->clock_speed = clk_get_rate(info->clk) / 100;
>>  if (info->clock_speed < 10 || info->clock_speed > 48)
>>  return -EINVAL;
>> @@ -1689,9 +1716,18 @@ static const struct i2c_device_id ov7670_id[] = {
>>  };
>>  MODULE_DEVICE_TABLE(i2c, ov7670_id);
>>  
>> +#if IS_ENABLED(CONFIG_OF)
>> +static const struct of_device_id ov7670_of_match[] = {
>> +{ .compatible = "ovti,ov7670", },
>> +{ /* sentinel */ },
>> +};
>> +MODULE_DEVICE_TABLE(of, ov7670_of_match);
>> +#endif
>> +
>>  static struct i2c_driver ov7670_driver = {
>>  .driver = {
>>  .name   = "ov7670",
>> +.of_match_table = of_match_ptr(ov7670_of_match),
>>  },
>>  .probe  = ov7670_probe,
>>  .remove = ov7670_remove,
> 

Re: [PATCHv3 05/15] ov7670: add devicetree support

[Sakari Ailus]

Hi Hans,

On Mon, Mar 06, 2017 at 03:56:06PM +0100, Hans Verkuil wrote:
> From: Hans Verkuil 
> 
> Add DT support. Use it to get the reset and pwdn pins (if there are any).
> Tested with one sensor requiring reset/pwdn and one sensor that doesn't
> have reset/pwdn pins.

If I read the datasheet right, lifting the reset signal up will reset the
sensor but the patch doesn't make use of that, it only ensures the reset
signal stays low. Should you lift it up for a while as well? The datasheet
doesn't say for how long that should be done, but that it should be usable
after 1 ms since pulling reset down.

> 
> Signed-off-by: Hans Verkuil 
> ---
>  drivers/media/i2c/ov7670.c | 40 ++--
>  1 file changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c
> index da0843617a49..f9d977430dcf 100644
> --- a/drivers/media/i2c/ov7670.c
> +++ b/drivers/media/i2c/ov7670.c
> @@ -17,6 +17,8 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -229,6 +231,8 @@ struct ov7670_info {
>   };
>   struct ov7670_format_struct *fmt;  /* Current format */
>   struct clk *clk;
> + struct gpio_desc *resetb_gpio;
> + struct gpio_desc *pwdn_gpio;
>   int min_width;  /* Filter out smaller sizes */
>   int min_height; /* Filter out smaller sizes */
>   int clock_speed;/* External clock speed (MHz) */
> @@ -591,8 +595,6 @@ static int ov7670_init(struct v4l2_subdev *sd, u32 val)
>   return ov7670_write_array(sd, ov7670_default_regs);
>  }
>  
> -
> -
>  static int ov7670_detect(struct v4l2_subdev *sd)
>  {
>   unsigned char v;
> @@ -1549,6 +1551,27 @@ static const struct ov7670_devtype ov7670_devdata[] = {
>   },
>  };
>  
> +static int ov7670_init_gpio(struct i2c_client *client, struct ov7670_info 
> *info)
> +{
> + info->pwdn_gpio = devm_gpiod_get_optional(>dev, "powerdown",
> + GPIOD_OUT_LOW);
> + if (IS_ERR(info->pwdn_gpio)) {
> + dev_info(>dev, "can't get %s GPIO\n", "powerdown");
> + return PTR_ERR(info->pwdn_gpio);
> + }
> +
> + info->resetb_gpio = devm_gpiod_get_optional(>dev, "reset",
> + GPIOD_OUT_LOW);
> + if (IS_ERR(info->resetb_gpio)) {
> + dev_info(>dev, "can't get %s GPIO\n", "reset");
> + return PTR_ERR(info->resetb_gpio);
> + }
> +
> + usleep_range(3000, 5000);
> +
> + return 0;
> +}
> +
>  static int ov7670_probe(struct i2c_client *client,
>   const struct i2c_device_id *id)
>  {
> @@ -1594,6 +1617,10 @@ static int ov7670_probe(struct i2c_client *client,
>   return -EPROBE_DEFER;
>   clk_prepare_enable(info->clk);
>  
> + ret = ov7670_init_gpio(client, info);
> + if (ret)
> + return ret;
> +
>   info->clock_speed = clk_get_rate(info->clk) / 100;
>   if (info->clock_speed < 10 || info->clock_speed > 48)
>   return -EINVAL;
> @@ -1689,9 +1716,18 @@ static const struct i2c_device_id ov7670_id[] = {
>  };
>  MODULE_DEVICE_TABLE(i2c, ov7670_id);
>  
> +#if IS_ENABLED(CONFIG_OF)
> +static const struct of_device_id ov7670_of_match[] = {
> + { .compatible = "ovti,ov7670", },
> + { /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, ov7670_of_match);
> +#endif
> +
>  static struct i2c_driver ov7670_driver = {
>   .driver = {
>   .name   = "ov7670",
> + .of_match_table = of_match_ptr(ov7670_of_match),
>   },
>   .probe  = ov7670_probe,
>   .remove = ov7670_remove,

-- 
Regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk