[5/6] media: ov772x: add device tree binding

This adds a device tree binding documentation for OV7720/OV7725 sensor.

Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Hans Verkuil <hans.verkuil@cisco.com>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
Cc: Rob Herring <robh+dt@kernel.org>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 .../devicetree/bindings/media/i2c/ov772x.txt       | 36 ++++++++++++++++++++++
 MAINTAINERS                                        |  1 +
 2 files changed, 37 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ov772x.txt

Hi Akinobu,

On Sun, Apr 08, 2018 at 12:48:09AM +0900, Akinobu Mita wrote:
> This adds a device tree binding documentation for OV7720/OV7725 sensor.

Please use as patch subject
media: dt-bindings:

>
> Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Hans Verkuil <hans.verkuil@cisco.com>
> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> ---
>  .../devicetree/bindings/media/i2c/ov772x.txt       | 36 ++++++++++++++++++++++
>  MAINTAINERS                                        |  1 +
>  2 files changed, 37 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov772x.txt
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/ov772x.txt b/Documentation/devicetree/bindings/media/i2c/ov772x.txt
> new file mode 100644
> index 0000000..9b0df3b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/ov772x.txt
> @@ -0,0 +1,36 @@
> +* Omnivision OV7720/OV7725 CMOS sensor
> +

Could you please provide a brief description of the sensor (supported
resolution and formats is ok)

> +Required Properties:
> +- compatible: shall be one of
> +	"ovti,ov7720"
> +	"ovti,ov7725"
> +- clocks: reference to the xclk input clock.
> +- clock-names: shall be "xclk".
> +
> +Optional Properties:
> +- rstb-gpios: reference to the GPIO connected to the RSTB pin, if any.
> +- pwdn-gpios: reference to the GPIO connected to the PWDN pin, if any.

As a general note:
This is debated, and I'm not enforcing it, but please consider using
generic names for GPIOs with common functions. In this case
"reset-gpios" and "powerdown-gpios". Also please indicate the GPIO
active level in bindings description.

For this specific driver:
The probe routine already looks for a GPIO named 'pwdn', so I guess
the DT bindings should use the same name. Unless you're willing to
change it in the board files that register it (Migo-R only in mainline) and
use the generic 'powerdown' name for both. Either is fine with me.

There is no support for the reset GPIO in the driver code, it
supports soft reset only. Either ditch it from bindings or add support
for it in driver's code.

Thanks
   j

> +
> +The device node shall contain one 'port' child node with one child 'endpoint'
> +subnode for its digital output video port, in accordance with the video
> +interface bindings defined in Documentation/devicetree/bindings/media/
> +video-interfaces.txt.
> +
> +Example:
> +
> +&i2c0 {
> +	ov772x: camera@21 {
> +		compatible = "ovti,ov7725";
> +		reg = <0x21>;
> +		rstb-gpios = <&axi_gpio_0 0 GPIO_ACTIVE_LOW>;
> +		pwdn-gpios = <&axi_gpio_0 1 GPIO_ACTIVE_LOW>;
> +		clocks = <&xclk>;
> +		clock-names = "xclk";
> +
> +		port {
> +			ov772x_0: endpoint {
> +				remote-endpoint = <&vcap1_in0>;
> +			};
> +		};
> +	};
> +};
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7e48624..3e0224a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -10295,6 +10295,7 @@ T:	git git://linuxtv.org/media_tree.git
>  S:	Odd fixes
>  F:	drivers/media/i2c/ov772x.c
>  F:	include/media/i2c/ov772x.h
> +F:	Documentation/devicetree/bindings/media/i2c/ov772x.txt
>
>  OMNIVISION OV7740 SENSOR DRIVER
>  M:	Wenyou Yang <wenyou.yang@microchip.com>
> --
> 2.7.4
>

2018-04-09 18:06 GMT+09:00 jacopo mondi <jacopo@jmondi.org>:
> Hi Akinobu,
>
> On Sun, Apr 08, 2018 at 12:48:09AM +0900, Akinobu Mita wrote:
>> This adds a device tree binding documentation for OV7720/OV7725 sensor.
>
> Please use as patch subject
> media: dt-bindings:

OK.

>>
>> Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>
>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Cc: Hans Verkuil <hans.verkuil@cisco.com>
>> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
>> Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
>> ---
>>  .../devicetree/bindings/media/i2c/ov772x.txt       | 36 ++++++++++++++++++++++
>>  MAINTAINERS                                        |  1 +
>>  2 files changed, 37 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov772x.txt
>>
>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov772x.txt b/Documentation/devicetree/bindings/media/i2c/ov772x.txt
>> new file mode 100644
>> index 0000000..9b0df3b
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/i2c/ov772x.txt
>> @@ -0,0 +1,36 @@
>> +* Omnivision OV7720/OV7725 CMOS sensor
>> +
>
> Could you please provide a brief description of the sensor (supported
> resolution and formats is ok)

OK.

>> +Required Properties:
>> +- compatible: shall be one of
>> +     "ovti,ov7720"
>> +     "ovti,ov7725"
>> +- clocks: reference to the xclk input clock.
>> +- clock-names: shall be "xclk".
>> +
>> +Optional Properties:
>> +- rstb-gpios: reference to the GPIO connected to the RSTB pin, if any.
>> +- pwdn-gpios: reference to the GPIO connected to the PWDN pin, if any.
>
> As a general note:
> This is debated, and I'm not enforcing it, but please consider using
> generic names for GPIOs with common functions. In this case
> "reset-gpios" and "powerdown-gpios". Also please indicate the GPIO
> active level in bindings description.
>
> For this specific driver:
> The probe routine already looks for a GPIO named 'pwdn', so I guess
> the DT bindings should use the same name. Unless you're willing to
> change it in the board files that register it (Migo-R only in mainline) and
> use the generic 'powerdown' name for both. Either is fine with me.

I'll prepare anothre patch that renames the GPIO names to generic one in
this driver and Mingo-R board file.

> There is no support for the reset GPIO in the driver code, it
> supports soft reset only. Either ditch it from bindings or add support
> for it in driver's code.

Doesn't that reset GPIO exist in current ov772x driver code, does it?
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html