Re: [PATCH v7 1/2] media: ov2680: dt: Add bindings for OV2680

[jacopo mondi <jacopo@jmondi.org>]

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

Hi Rui,
   sorry, I'm a bit late, you're already at v7 and I don't want to
slow down inclusion with a few minor comments.

Please bear with me and see below...

On Tue, Jul 03, 2018 at 03:08:02PM +0100, Rui Miguel Silva wrote:
> Add device tree binding documentation for the OV2680 camera sensor.
>
> CC: devicetree@vger.kernel.org
> Signed-off-by: Rui Miguel Silva <rui.silva@linaro.org>
> ---
>  .../devicetree/bindings/media/i2c/ov2680.txt  | 46 +++++++++++++++++++
>  1 file changed, 46 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov2680.txt
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/ov2680.txt b/Documentation/devicetree/bindings/media/i2c/ov2680.txt
> new file mode 100644
> index 000000000000..11e925ed9dad
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/ov2680.txt
> @@ -0,0 +1,46 @@
> +* Omnivision OV2680 MIPI CSI-2 sensor
> +
> +Required Properties:
> +- compatible: should be "ovti,ov2680".
> +- clocks: reference to the xvclk input clock.
> +- clock-names: should be "xvclk".

Having a single clock source I think you can omit 'clock-names' (or at
least not marking it as required)

> +- DOVDD-supply: Digital I/O voltage supply.
> +- DVDD-supply: Digital core voltage supply.
> +- AVDD-supply: Analog voltage supply.
> +
> +Optional Properties:
> +- reset-gpios: reference to the GPIO connected to the powerdown/reset pin,
> +               if any. This is an active low signal to the OV2680.
> +
> +The device node must contain one 'port' child node for its digital output
> +video port, and this port must have a single endpoint in accordance with
> + the video interface bindings defined in
> +Documentation/devicetree/bindings/media/video-interfaces.txt.
> +
> +Endpoint node required properties for CSI-2 connection are:
> +- remote-endpoint: a phandle to the bus receiver's endpoint node.
> +- clock-lanes: should be set to <0> (clock lane on hardware lane 0).
> +- data-lanes: should be set to <1> (one CSI-2 lane supported).

What is the value of marking as required two properties which can only have
default values (the sensor does not support clock on different lanes,
nor it supports more than 1 data lane) ?

Thanks
   j

> +
> +Example:
> +
> +&i2c2 {
> +	ov2680: camera-sensor@36 {
> +		compatible = "ovti,ov2680";
> +		reg = <0x36>;
> +		clocks = <&osc>;
> +		clock-names = "xvclk";
> +		reset-gpios = <&gpio1 3 GPIO_ACTIVE_LOW>;
> +		DOVDD-supply = <&sw2_reg>;
> +		DVDD-supply = <&sw2_reg>;
> +		AVDD-supply = <&reg_peri_3p15v>;
> +
> +		port {
> +			ov2680_to_mipi: endpoint {
> +				remote-endpoint = <&mipi_from_sensor>;
> +				clock-lanes = <0>;
> +				data-lanes = <1>;
> +			};
> +		};
> +	};
> +};
> --
> 2.18.0
>

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