Re: [PATCH v9 1/2] Add OV5647 device tree documentation

[Ramiro Oliveira <Ramiro.Oliveira@synopsys.com>]

Hi Sakari,

Thank you for your feedback.

On 2/21/2017 11:57 AM, Sakari Ailus wrote:
> Hi Ramiro,
> 
> On Fri, Feb 17, 2017 at 01:14:15PM +0000, Ramiro Oliveira wrote:
>> Create device tree bindings documentation.
>>
>> Signed-off-by: Ramiro Oliveira <roliveir@synopsys.com>
>> Acked-by: Rob Herring <robh@kernel.org>
>> ---
>>  .../devicetree/bindings/media/i2c/ov5647.txt       | 35 ++++++++++++++++++++++
>>  1 file changed, 35 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5647.txt
>>
>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5647.txt b/Documentation/devicetree/bindings/media/i2c/ov5647.txt
>> new file mode 100644
>> index 000000000000..31956426d3b9
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/i2c/ov5647.txt
>> @@ -0,0 +1,35 @@
>> +Omnivision OV5647 raw image sensor
>> +---------------------------------
>> +
>> +OV5647 is a raw image sensor with MIPI CSI-2 and CCP2 image data interfaces
>> +and CCI (I2C compatible) control bus.
>> +
>> +Required properties:
>> +
>> +- compatible		: "ovti,ov5647".
>> +- reg			: I2C slave address of the sensor.
>> +- clocks		: Reference to the xclk clock.
>> +- clock-names		: Should be "xclk".
>> +- clock-frequency	: Frequency of the xclk clock.
>> +
>> +The common video interfaces bindings (see video-interfaces.txt) should be
>> +used to specify link to the image data receiver. The OV5647 device
>> +node should contain one 'port' child node with an 'endpoint' subnode.
> 
> The remote-endpoint property in endpoint nodes should be mandatory,
> shouldn't it? Otherwise the sensor isn't connected to anything and hardly
> useful as such. The list of optional endpoint properties is a long one and
> it should be documented here which ones are recognised, either as optional
> or mandatory.
> 

I guess you're right, it should be mandatory, although at the moment I'm not
checking for it's presence in the driver so I'll add it to the driver.

At the moment that's the only property I think it should be mandatory, and I
don't believe I need any optional one.

Do you have a suggestion for any new property I should use?

>> +
>> +Example:
>> +
>> +	i2c@2000 {
>> +		...
>> +		ov: camera@36 {
>> +			compatible = "ovti,ov5647";
>> +			reg = <0x36>;
>> +			clocks = <&camera_clk>;
>> +			clock-names = "xclk";
>> +			clock-frequency = <25000000>;
>> +			port {
>> +				camera_1: endpoint {
>> +					remote-endpoint = <&csi1_ep1>;
>> +				};
>> +			};
>> +		};
>> +	};
> 

-- 
Best Regards

Ramiro Oliveira
Ramiro.Oliveira@synopsys.com