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

[jacopo mondi <jacopo@jmondi.org>]

[-- Attachment #1: Type: text/plain, Size: 3370 bytes --]

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
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]