subject:"Re\: \[RFC PATCH 5\/7\] ov7670\: add devicetree support"

Re: [RFC PATCH 5/7] ov7670: add devicetree support

2016-09-21 Thread Laurent Pinchart

Hi Hans,

On Friday 26 Aug 2016 09:45:25 Hans Verkuil wrote:
> On 08/17/2016 02:44 PM, Laurent Pinchart wrote:
> > On Wednesday 17 Aug 2016 08:29:41 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.
> >> 
> >> Signed-off-by: Hans Verkuil 
> >> ---
> >> 
> >>  .../devicetree/bindings/media/i2c/ov7670.txt   | 44 ++
> >>  MAINTAINERS|  1 +
> >>  drivers/media/i2c/ov7670.c | 51 
> >>  3 files changed, 96 insertions(+)
> >>  create mode 100644
> >>  Documentation/devicetree/bindings/media/i2c/ov7670.txt
> >> 
> >> diff --git a/Documentation/devicetree/bindings/media/i2c/ov7670.txt
> >> b/Documentation/devicetree/bindings/media/i2c/ov7670.txt new file mode
> >> 100644
> >> index 000..3231c47
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/media/i2c/ov7670.txt
> >> @@ -0,0 +1,44 @@
> >> +* Omnivision OV7670 CMOS sensor
> >> +
> >> +The Omnivision OV7670 sensor support multiple resolutions output, such
> >> as
> > 
> > s/support/supports/
> > 
> >> +CIF, SVGA, UXGA. It also can support YUV422/420, RGB565/555 or raw RGB
> >> +output format.
> > 
> > s/format/formats/ (and possibly s/can support/can support the/)
> > 
> >> +
> >> +Required Properties:
> >> +- compatible: should be "ovti,ov7670"
> >> +- clocks: reference to the xvclk input clock.
> >> +- clock-names: should be "xvclk".
> >> +
> >> +Optional Properties:
> >> +- resetb-gpios: reference to the GPIO connected to the resetb pin, if
> >> any.
> >> +- pwdn-gpios: reference to the GPIO connected to the pwdn pin, if any.
> >> +
> >> +The device node must contain one 'port' child node for its digital
> >> output
> >> +video port, in accordance with the video interface bindings defined in
> >> +Documentation/devicetree/bindings/media/video-interfaces.txt.
> >> +
> >> +Example:
> >> +
> >> +  i2c1: i2c@f0018000 {
> >> +  status = "okay";
> >> +
> >> +  ov7670: camera@0x21 {
> >> +  compatible = "ovti,ov7670";
> >> +  reg = <0x21>;
> >> +  pinctrl-names = "default";
> >> +  pinctrl-0 = <_pck0_as_isi_mck
> >> _sensor_power
> >> _sensor_reset>;
> > 
> > The pinctrl properties should be part of the clock provider DT node.
> 
> Do you have examples of that?

Sure, it's pretty simple. The pinctrl_pck0_as_isi_mck can just be moved to the 
isi DT node, as the isi is the clock provider for the sensor.

The two other properties, however, have to stay here (I think I've overlooked 
them in my previous review e-mail), so I'm also not totally opposed to keeping 
all three pinctrl entries together.

> I just copied this from existing atmel dts code
> (arch/arm/boot/dts/sama5d3xmb.dtsi).
>
> >> +  resetb-gpios = < 11 GPIO_ACTIVE_LOW>;
> >> +  pwdn-gpios = < 13 GPIO_ACTIVE_HIGH>;
> >> +  clocks = <>;
> >> +  clock-names = "xvclk";

I missed this, isn't the pin named "xclk" in the datasheet ?

> >> +  assigned-clocks = <>;
> >> +  assigned-clock-rates = <2400>;
> > 
> > You should compute and set the clock rate dynamically in the driver, not
> > hardcode it in DT.
> 
> Do you have an example of that? Again, I just copied this from the same
> sama5d3xmb.dtsi.

Please see my reply to Sakari's e-mail in the same thread.

-- 
Regards,

Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 5/7] ov7670: add devicetree support

2016-09-21 Thread Laurent Pinchart

Hello Sakari,

On Wednesday 21 Sep 2016 14:33:29 Sakari Ailus wrote:
> Hi Laurent,
> 
> On Wed, Aug 17, 2016 at 03:44:00PM +0300, Laurent Pinchart wrote:
> > > + assigned-clock-rates = <2400>;
> > 
> > You should compute and set the clock rate dynamically in the driver, not
> > hardcode it in DT.
> 
> This frequency is typically defined by hardware engineers and it's
> hand-picked from possible ranges. What counts is EMC so you don't disturb
> a GSM/3G/4G modem (if you have one) or GPS receiver, for instance. In order
> to freely choose this frequency, you'd also need to be aware of the limits
> of the sensor's internal clock tree, and make sure you still would be able
> to obtain the desired frame rates for there are often corner cases where the
> resulting maximum pixel clock may be substantially lower if your input
> frequency goes up.
> 
> I also haven't encountered a use case for more than a single, fixed
> frequency. In other words I'd keep this constant.

My review predates our recent discussion on this topic. I still believe that 
in the general case we could want to express allowable frequencies constraints 
in DT and let the driver compute the desired frequency based on timings. 
However, I also agree that there are very few cases for this, so it's not 
really worth tackling the issue at the moment. I'm thus fine with the usage of 
assigned-clock-rates.

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 5/7] ov7670: add devicetree support

2016-09-21 Thread Sakari Ailus

Hi Laurent,

On Wed, Aug 17, 2016 at 03:44:00PM +0300, Laurent Pinchart wrote:
> > +   assigned-clock-rates = <2400>;
> 
> You should compute and set the clock rate dynamically in the driver, not 
> hardcode it in DT.

This frequency is typically defined by hardware engineers and it's
hand-picked from possible ranges. What counts is EMC so you don't disturb
a GSM/3G/4G modem (if you have one) or GPS receiver, for instance. In order
to freely choose this frequency, you'd also need to be aware of the limits
of the sensor's internal clock tree, and make sure you still would be able
to obtain the desired frame rates for there are often corner cases where the
resulting maximum pixel clock may be substantially lower if your input
frequency goes up.

I also haven't encountered a use case for more than a single, fixed
frequency. In other words I'd keep this constant.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 5/7] ov7670: add devicetree support

2016-09-21 Thread Sakari Ailus

Hi Hans,

On Wed, Aug 17, 2016 at 08:29:41AM +0200, 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.
> 
> Signed-off-by: Hans Verkuil 
> ---
>  .../devicetree/bindings/media/i2c/ov7670.txt   | 44 +++
>  MAINTAINERS|  1 +
>  drivers/media/i2c/ov7670.c | 51 
> ++
>  3 files changed, 96 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov7670.txt
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/ov7670.txt 
> b/Documentation/devicetree/bindings/media/i2c/ov7670.txt
> new file mode 100644
> index 000..3231c47
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/ov7670.txt
> @@ -0,0 +1,44 @@
> +* Omnivision OV7670 CMOS sensor
> +
> +The Omnivision OV7670 sensor support multiple resolutions output, such as
> +CIF, SVGA, UXGA. It also can support YUV422/420, RGB565/555 or raw RGB
> +output format.
> +
> +Required Properties:
> +- compatible: should be "ovti,ov7670"
> +- clocks: reference to the xvclk input clock.
> +- clock-names: should be "xvclk".

Where does "xvclk" come from? Why not "xclk"?

> +
> +Optional Properties:
> +- resetb-gpios: reference to the GPIO connected to the resetb pin, if any.
> +- pwdn-gpios: reference to the GPIO connected to the pwdn pin, if any.
> +
> +The device node must contain one 'port' child node for its digital output
> +video port, in accordance with the video interface bindings defined in
> +Documentation/devicetree/bindings/media/video-interfaces.txt.
> +
> +Example:
> +
> + i2c1: i2c@f0018000 {
> + status = "okay";
> +
> + ov7670: camera@0x21 {
> + compatible = "ovti,ov7670";
> + reg = <0x21>;
> + pinctrl-names = "default";
> + pinctrl-0 = <_pck0_as_isi_mck 
> _sensor_power _sensor_reset>;
> + resetb-gpios = < 11 GPIO_ACTIVE_LOW>;
> + pwdn-gpios = < 13 GPIO_ACTIVE_HIGH>;
> + clocks = <>;
> + clock-names = "xvclk";
> + assigned-clocks = <>;
> + assigned-clock-rates = <2400>;

What's the difference between assigned-clocks and just clocks?

The example in video-interfaces.txt uses "clocks" and "clock-frequency" for
the purpose.

> +

How about the regulators? This sensor requires three. You probably have
fixed regulators there if things just work.

> + port {
> + ov7670_0: endpoint {
> + remote-endpoint = <_0>;
> + bus-width = <8>;
> + };
> + };
> + };
> + };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 20bb1d0..1fec3a6 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -8615,6 +8615,7 @@ L:  linux-media@vger.kernel.org
>  T:   git git://linuxtv.org/media_tree.git
>  S:   Maintained
>  F:   drivers/media/i2c/ov7670.c
> +F:   Documentation/devicetree/bindings/media/i2c/ov7670.txt
>  
>  ONENAND FLASH DRIVER
>  M:   Kyungmin Park 
> diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c
> index 57adf3d..a401b99 100644
> --- a/drivers/media/i2c/ov7670.c
> +++ b/drivers/media/i2c/ov7670.c
> @@ -17,6 +17,9 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +#include 
> +#include 

You don't need at least of_gpio here. Probably linux/gpio.h is redundant as
well.

>  #include 
>  #include 
>  #include 
> @@ -231,6 +234,8 @@ struct ov7670_info {
>   };
>   struct ov7670_format_struct *fmt;  /* Current format */
>   struct v4l2_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) */
> @@ -1551,6 +1556,40 @@ static const struct ov7670_devtype ov7670_devdata[] = {
>   },
>  };
>  
> +static int ov7670_probe_dt(struct i2c_client *client,
> + struct ov7670_info *info)
> +{
> + /* Request the reset GPIO deasserted */
> + info->resetb_gpio = devm_gpiod_get_optional(>dev, "resetb",
> + GPIOD_OUT_LOW);
> + if (!info->resetb_gpio)
> + dev_dbg(>dev, "resetb gpio is not assigned!\n");

AFAIR the GPIO framework already complains vocally if the GPIO can't be
found.

> + else if (IS_ERR(info->resetb_gpio)) {
> + dev_info(>dev, "no resetb\n");
> + return PTR_ERR(info->resetb_gpio);
> + }
> +
> + /* Request the power

Re: [RFC PATCH 5/7] ov7670: add devicetree support

2016-08-26 Thread Hans Verkuil

Hi Laurent,

On 08/17/2016 02:44 PM, Laurent Pinchart wrote:
> Hi Hans,
> 
> Thank you for the patch.
> 
> On Wednesday 17 Aug 2016 08:29:41 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.
>>
>> Signed-off-by: Hans Verkuil 
>> ---
>>  .../devicetree/bindings/media/i2c/ov7670.txt   | 44 +
>>  MAINTAINERS|  1 +
>>  drivers/media/i2c/ov7670.c | 51 +++
>>  3 files changed, 96 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov7670.txt
>>
>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov7670.txt
>> b/Documentation/devicetree/bindings/media/i2c/ov7670.txt new file mode
>> 100644
>> index 000..3231c47
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/i2c/ov7670.txt
>> @@ -0,0 +1,44 @@
>> +* Omnivision OV7670 CMOS sensor
>> +
>> +The Omnivision OV7670 sensor support multiple resolutions output, such as
> 
> s/support/supports/
> 
>> +CIF, SVGA, UXGA. It also can support YUV422/420, RGB565/555 or raw RGB
>> +output format.
> 
> s/format/formats/ (and possibly s/can support/can support the/)
> 
>> +
>> +Required Properties:
>> +- compatible: should be "ovti,ov7670"
>> +- clocks: reference to the xvclk input clock.
>> +- clock-names: should be "xvclk".
>> +
>> +Optional Properties:
>> +- resetb-gpios: reference to the GPIO connected to the resetb pin, if any.
>> +- pwdn-gpios: reference to the GPIO connected to the pwdn pin, if any.
>> +
>> +The device node must contain one 'port' child node for its digital output
>> +video port, in accordance with the video interface bindings defined in
>> +Documentation/devicetree/bindings/media/video-interfaces.txt.
>> +
>> +Example:
>> +
>> +i2c1: i2c@f0018000 {
>> +status = "okay";
>> +
>> +ov7670: camera@0x21 {
>> +compatible = "ovti,ov7670";
>> +reg = <0x21>;
>> +pinctrl-names = "default";
>> +pinctrl-0 = <_pck0_as_isi_mck
>> _sensor_power
>> _sensor_reset>;
> 
> The pinctrl properties should be part of the clock provider DT node.

Do you have examples of that?

I just copied this from existing atmel dts code 
(arch/arm/boot/dts/sama5d3xmb.dtsi).

> 
>> +resetb-gpios = < 11 GPIO_ACTIVE_LOW>;
>> +pwdn-gpios = < 13 GPIO_ACTIVE_HIGH>;
>> +clocks = <>;
>> +clock-names = "xvclk";
>> +assigned-clocks = <>;
>> +assigned-clock-rates = <2400>;
> 
> You should compute and set the clock rate dynamically in the driver, not 
> hardcode it in DT.

Do you have an example of that? Again, I just copied this from the same 
sama5d3xmb.dtsi.

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 5/7] ov7670: add devicetree support

2016-08-17 Thread Laurent Pinchart

Hi Hans,

Thank you for the patch.

On Wednesday 17 Aug 2016 08:29:41 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.
> 
> Signed-off-by: Hans Verkuil 
> ---
>  .../devicetree/bindings/media/i2c/ov7670.txt   | 44 +
>  MAINTAINERS|  1 +
>  drivers/media/i2c/ov7670.c | 51 +++
>  3 files changed, 96 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov7670.txt
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/ov7670.txt
> b/Documentation/devicetree/bindings/media/i2c/ov7670.txt new file mode
> 100644
> index 000..3231c47
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/ov7670.txt
> @@ -0,0 +1,44 @@
> +* Omnivision OV7670 CMOS sensor
> +
> +The Omnivision OV7670 sensor support multiple resolutions output, such as

s/support/supports/

> +CIF, SVGA, UXGA. It also can support YUV422/420, RGB565/555 or raw RGB
> +output format.

s/format/formats/ (and possibly s/can support/can support the/)

> +
> +Required Properties:
> +- compatible: should be "ovti,ov7670"
> +- clocks: reference to the xvclk input clock.
> +- clock-names: should be "xvclk".
> +
> +Optional Properties:
> +- resetb-gpios: reference to the GPIO connected to the resetb pin, if any.
> +- pwdn-gpios: reference to the GPIO connected to the pwdn pin, if any.
> +
> +The device node must contain one 'port' child node for its digital output
> +video port, in accordance with the video interface bindings defined in
> +Documentation/devicetree/bindings/media/video-interfaces.txt.
> +
> +Example:
> +
> + i2c1: i2c@f0018000 {
> + status = "okay";
> +
> + ov7670: camera@0x21 {
> + compatible = "ovti,ov7670";
> + reg = <0x21>;
> + pinctrl-names = "default";
> + pinctrl-0 = <_pck0_as_isi_mck
> _sensor_power
> _sensor_reset>;

The pinctrl properties should be part of the clock provider DT node.

> + resetb-gpios = < 11 GPIO_ACTIVE_LOW>;
> + pwdn-gpios = < 13 GPIO_ACTIVE_HIGH>;
> + clocks = <>;
> + clock-names = "xvclk";
> + assigned-clocks = <>;
> + assigned-clock-rates = <2400>;

You should compute and set the clock rate dynamically in the driver, not 
hardcode it in DT.

> + port {
> + ov7670_0: endpoint {
> + remote-endpoint = <_0>;
> + bus-width = <8>;
> + };
> + };
> + };
> + };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 20bb1d0..1fec3a6 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -8615,6 +8615,7 @@ L:  linux-media@vger.kernel.org
>  T:   git git://linuxtv.org/media_tree.git
>  S:   Maintained
>  F:   drivers/media/i2c/ov7670.c
> +F:   Documentation/devicetree/bindings/media/i2c/ov7670.txt
> 
>  ONENAND FLASH DRIVER
>  M:   Kyungmin Park 
> diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c
> index 57adf3d..a401b99 100644
> --- a/drivers/media/i2c/ov7670.c
> +++ b/drivers/media/i2c/ov7670.c
> @@ -17,6 +17,9 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +#include 
> +#include 

I don't think you need of_gpio.h.

>  #include 
>  #include 
>  #include 
> @@ -231,6 +234,8 @@ struct ov7670_info {
>   };
>   struct ov7670_format_struct *fmt;  /* Current format */
>   struct v4l2_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) */
> @@ -1551,6 +1556,40 @@ static const struct ov7670_devtype ov7670_devdata[] =
> { },
>  };
> 
> +static int ov7670_probe_dt(struct i2c_client *client,

The function doesn't actually depend on DT, how about calling it 
ov7670_init_gpio() or something similar ?

> + struct ov7670_info *info)
> +{
> + /* Request the reset GPIO deasserted */
> + info->resetb_gpio = devm_gpiod_get_optional(>dev, "resetb",
> + GPIOD_OUT_LOW);
> + if (!info->resetb_gpio)
> + dev_dbg(>dev, "resetb gpio is not assigned!\n");

I don't think the debug message is really worth it, but that's up to you.

> + else if (IS_ERR(info->resetb_gpio)) {
> + dev_info(>dev, "no resetb\n");

If you write this "no %s\n", "resetb" and do the same for the "no pwdn\n" 
string below you will save a

Re: [RFC PATCH 5/7] ov7670: add devicetree support

Re: [RFC PATCH 5/7] ov7670: add devicetree support

Re: [RFC PATCH 5/7] ov7670: add devicetree support

Re: [RFC PATCH 5/7] ov7670: add devicetree support

Re: [RFC PATCH 5/7] ov7670: add devicetree support

Re: [RFC PATCH 5/7] ov7670: add devicetree support

6 matches

Site Navigation

Mail list logo

Footer information