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

[Rui Miguel Silva <rui.silva@linaro.org>]

Hi Jacopo,
Hope your fine.
Thanks for the review.

On Wed 04 Jul 2018 at 09:58, jacopo mondi wrote:
> 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)

yeah, I see you point, but really all other OV sensors share this 
and
the bellow clock/data-lanes properties as required, I will let Rob 
or
Sakari take a call in this one.

---
Cheers,
	Rui

>
>> +- 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
>>