Re: [PATCHv2 08/16] atmel-isi: document device tree bindings

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 02/01/2017 05:50 PM, Rob Herring wrote:
> On Mon, Jan 30, 2017 at 03:06:20PM +0100, Hans Verkuil wrote:
>> From: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>>
>> Document the device tree bindings for this driver.
> 
> Bindings document h/w not drivers.

Fixed.

> 
>>
>> Mostly copied from the atmel-isc bindings.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>> ---
>>  .../devicetree/bindings/media/atmel-isi.txt        | 91 +++++++++++++---------
>>  1 file changed, 56 insertions(+), 35 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/media/atmel-isi.txt b/Documentation/devicetree/bindings/media/atmel-isi.txt
>> index 251f008..d1934b4 100644
>> --- a/Documentation/devicetree/bindings/media/atmel-isi.txt
>> +++ b/Documentation/devicetree/bindings/media/atmel-isi.txt
>> @@ -1,51 +1,72 @@
>> -Atmel Image Sensor Interface (ISI) SoC Camera Subsystem
>> -----------------------------------------------
>> +Atmel Image Sensor Interface (ISI)
>> +----------------------------------
>>  
>> -Required properties:
>> -- compatible: must be "atmel,at91sam9g45-isi"
>> -- reg: physical base address and length of the registers set for the device;
>> -- interrupts: should contain IRQ line for the ISI;
>> -- clocks: list of clock specifiers, corresponding to entries in
>> -          the clock-names property;
>> -- clock-names: must contain "isi_clk", which is the isi peripherial clock.
>> +Required properties for ISI:
>> +- compatible
>> +	Must be "atmel,at91sam9g45-isi".
>> +- reg
>> +	Physical base address and length of the registers set for the device.
>> +- interrupts
>> +	Should contain IRQ line for the ISI.
>> +- clocks
>> +	List of clock specifiers, corresponding to entries in
>> +	the clock-names property;
>> +	Please refer to clock-bindings.txt.
>> +- clock-names
>> +	Required elements: "isi_clk".
>> +- #clock-cells
>> +	Should be 0.
> 
> This reformatting is unrelated and the old form was more standard for 
> bindings (not that we have any real standard).

OK, I went back to the old format.

> 
>> +- pinctrl-names, pinctrl-0
>> +	Please refer to pinctrl-bindings.txt.
>>  
>>  ISI supports a single port node with parallel bus. It should contain one
>>  'port' child node with child 'endpoint' node. Please refer to the bindings
>>  defined in Documentation/devicetree/bindings/media/video-interfaces.txt.
>>  
>>  Example:
>> -	isi: isi@f0034000 {
>> -		compatible = "atmel,at91sam9g45-isi";
>> -		reg = <0xf0034000 0x4000>;
>> -		interrupts = <37 IRQ_TYPE_LEVEL_HIGH 5>;
>>  
>> -		clocks = <&isi_clk>;
>> -		clock-names = "isi_clk";
>> +isi: isi@f0034000 {
>> +	compatible = "atmel,at91sam9g45-isi";
>> +	reg = <0xf0034000 0x4000>;
>> +	interrupts = <37 IRQ_TYPE_LEVEL_HIGH 5>;
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&pinctrl_isi_data_0_7>;
>> +	clocks = <&isi_clk>;
>> +	clock-names = "isi_clk";
>> +	status = "ok";
>> +	port {
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +		isi_0: endpoint {
>> +			reg = <0>;
> 
> Drop reg.

Is that because that is the default?

> 
>> +			remote-endpoint = <&ov2640_0>;
>> +			bus-width = <8>;
>> +			vsync-active = <1>;
>> +			hsync-active = <1>;
> 
> Which side of the connect is supposed to define these?

The Atmel ISI side. In the v3 of this patch these properties are
explicitly documented.

> 
>> +		};
>> +	};
>> +};
>> +
>> +i2c1: i2c@f0018000 {
>> +	status = "okay";
>>  
>> +	ov2640: camera@0x30 {
> 
> Drop the '0x'.

Done.

> 
>> +		compatible = "ovti,ov2640";
>> +		reg = <0x30>;
>>  		pinctrl-names = "default";
>> -		pinctrl-0 = <&pinctrl_isi>;
>> +		pinctrl-0 = <&pinctrl_pck0_as_isi_mck &pinctrl_sensor_power &pinctrl_sensor_reset>;
>> +		resetb-gpios = <&pioE 11 GPIO_ACTIVE_LOW>;
>> +		pwdn-gpios = <&pioE 13 GPIO_ACTIVE_HIGH>;
>> +		clocks = <&pck0>;
>> +		clock-names = "xvclk";
>> +		assigned-clocks = <&pck0>;
>> +		assigned-clock-rates = <25000000>;
>>  
>>  		port {
>> -			#address-cells = <1>;
>> -			#size-cells = <0>;
>> -
>> -			isi_0: endpoint {
>> -				remote-endpoint = <&ov2640_0>;
>> +			ov2640_0: endpoint {
>> +				remote-endpoint = <&isi_0>;
>>  				bus-width = <8>;
> 
> It is pointless to define bus-width at both ends.

No, this is separate for each end. See Sakari's reply.

Thanks for the review!

Regards,

	Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux