diff mbox

[v6,4/4] dt-bindings: media: Document Synopsys Designware HDMI RX

Message ID d6da0a3ec47a46d30b74e9d41fb4bf9ef392d969.1499176790.git.joabreu@synopsys.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jose Abreu July 4, 2017, 2:11 p.m. UTC 
Document the bindings for the Synopsys Designware HDMI RX.

Signed-off-by: Jose Abreu <joabreu@synopsys.com>
Cc: Carlos Palminha <palminha@synopsys.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: Hans Verkuil <hans.verkuil@cisco.com>
Cc: Sylwester Nawrocki <snawrocki@kernel.org>
Cc: devicetree@vger.kernel.org

Changes from v4:
	- Use "cfg" instead of "cfg-clk" (Rob)
	- Change node names (Rob)
Changes from v3:
	- Document the new DT bindings suggested by Sylwester
Changes from v2:
	- Document edid-phandle property
---
 .../devicetree/bindings/media/snps,dw-hdmi-rx.txt  | 70 ++++++++++++++++++++++
 1 file changed, 70 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.txt

Comments

Sylwester Nawrocki July 5, 2017, 8:52 p.m. UTC | #1 
On 07/04/2017 04:11 PM, Jose Abreu wrote:
> Document the bindings for the Synopsys Designware HDMI RX.
> 
> Signed-off-by: Jose Abreu <joabreu@synopsys.com>

> ---
>   .../devicetree/bindings/media/snps,dw-hdmi-rx.txt  | 70 ++++++++++++++++++++++
>   1 file changed, 70 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.txt

Could you make the DT binding documentation patch first patch in the series?
Now checkpatch will complain about undocumented compatible string when 
the driver patches are applied alone.

> diff --git a/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.txt 
>b/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.txt
> new file mode 100644
> index 0000000..449b8a2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.txt
> @@ -0,0 +1,70 @@
> +Synopsys DesignWare HDMI RX Decoder
> +===================================
> +
> +This document defines device tree properties for the Synopsys DesignWare HDMI
> +RX Decoder (DWC HDMI RX). It doesn't constitute a device tree binding
> +specification by itself but is meant to be referenced by platform-specific
> +device tree bindings.
> +
> +When referenced from platform device tree bindings the properties defined in
> +this document are defined as follows.

It would be good to make it clear which properties are required and which are
optional. And also to mention the properties below belong to the HDMI RX node.

> +- compatible: Shall be "snps,dw-hdmi-rx".
> +
> +- reg: Memory mapped base address and length of the DWC HDMI RX registers.
> +
> +- interrupts: Reference to the DWC HDMI RX interrupt and 5v sense interrupt.

s/5v/HDMI 5V ?

> +
> +- clocks: Phandle to the config clock block.
> +
> +- clock-names: Shall be "cfg".
> +
> +- edid-phandle: phandle to the EDID handler block.

Could you make this property optional and when it is missing assume that device
corresponding to the parent node of this node handles EDID? This way we could
avoid having property pointing to the parent node.

> +- #address-cells: Shall be 1.
> +
> +- #size-cells: Shall be 0.
> +
> +You also have to create a subnode for phy driver. Phy properties are as follows.

s/phy driver. Phy/the PHY device. PHY ?

Might be also worth to make it explicit these are all required properties.

> +- compatible: Shall be "snps,dw-hdmi-phy-e405".
> +
> +- reg: Shall be JTAG address of phy.

s/phy/the PHY ?

> +- clocks: Phandle for cfg clock.
> +
> +- clock-names:Shall be "cfg".
> +
> +A sample binding is now provided. The compatible string is for a SoC which has
> +has a Synopsys DesignWare HDMI RX decoder inside.
> +
> +Example:
> +
> +dw_hdmi_soc: dw-hdmi-soc@0 {
> +	compatible = "snps,dw-hdmi-soc";

Perhaps just make it

	compatible = "...";
?

> +	reg = <0x11c00 0x1000>; /* EDIDs */

This is not relevant and undocumented, will likely be part of documentation 
of other binding thus I'd suggest dropping this reg property.

> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +	ranges;
> +
> +	hdmi-rx@0 {
> +		compatible = "snps,dw-hdmi-rx";
> +		reg = <0x0 0x10000>;
> +		interrupts = <1 2>;
> +		edid-phandle = <&dw_hdmi_soc>;
> +
> +		clocks = <&dw_hdmi_refclk>;
> +		clock-names = "cfg";
> +
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		hdmi-phy@fc {
> +			compatible = "snps,dw-hdmi-phy-e405";
> +			reg = <0xfc>;
> +
> +			clocks = <&dw_hdmi_refclk>;
> +			clock-names = "cfg";
> +		};
> +	};
> +};

Otherwise looks good. I'll likely not have comments to the other patches.

--
Regards,
Sylwester
Jose Abreu July 6, 2017, 10:24 a.m. UTC | #2 
Hi Sylwester,


On 05-07-2017 21:52, Sylwester Nawrocki wrote:
> On 07/04/2017 04:11 PM, Jose Abreu wrote:
>> Document the bindings for the Synopsys Designware HDMI RX.
>>
>> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
>> ---
>>   .../devicetree/bindings/media/snps,dw-hdmi-rx.txt  | 70 ++++++++++++++++++++++
>>   1 file changed, 70 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.txt
> Could you make the DT binding documentation patch first patch in the series?
> Now checkpatch will complain about undocumented compatible string when 
> the driver patches are applied alone.

Sure.

>
>> diff --git a/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.txt 
>> b/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.txt
>> new file mode 100644
>> index 0000000..449b8a2
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.txt
>> @@ -0,0 +1,70 @@
>> +Synopsys DesignWare HDMI RX Decoder
>> +===================================
>> +
>> +This document defines device tree properties for the Synopsys DesignWare HDMI
>> +RX Decoder (DWC HDMI RX). It doesn't constitute a device tree binding
>> +specification by itself but is meant to be referenced by platform-specific
>> +device tree bindings.
>> +
>> +When referenced from platform device tree bindings the properties defined in
>> +this document are defined as follows.
> It would be good to make it clear which properties are required and which are
> optional. And also to mention the properties below belong to the HDMI RX node.

Ok.

>
>> +- compatible: Shall be "snps,dw-hdmi-rx".
>> +
>> +- reg: Memory mapped base address and length of the DWC HDMI RX registers.
>> +
>> +- interrupts: Reference to the DWC HDMI RX interrupt and 5v sense interrupt.
> s/5v/HDMI 5V ?

Ok.

>
>> +
>> +- clocks: Phandle to the config clock block.
>> +
>> +- clock-names: Shall be "cfg".
>> +
>> +- edid-phandle: phandle to the EDID handler block.
> Could you make this property optional and when it is missing assume that device
> corresponding to the parent node of this node handles EDID? This way we could
> avoid having property pointing to the parent node.

Hmm, this is for the CEC notifier. Do you mean I should grab the
parent device for the notifier? This property is already optional
if cec is not enabled though.

>
>> +- #address-cells: Shall be 1.
>> +
>> +- #size-cells: Shall be 0.
>> +
>> +You also have to create a subnode for phy driver. Phy properties are as follows.
> s/phy driver. Phy/the PHY device. PHY ?
>
> Might be also worth to make it explicit these are all required properties.

Ok.

>
>> +- compatible: Shall be "snps,dw-hdmi-phy-e405".
>> +
>> +- reg: Shall be JTAG address of phy.
> s/phy/the PHY ?

Ok.

>
>> +- clocks: Phandle for cfg clock.
>> +
>> +- clock-names:Shall be "cfg".
>> +
>> +A sample binding is now provided. The compatible string is for a SoC which has
>> +has a Synopsys DesignWare HDMI RX decoder inside.
>> +
>> +Example:
>> +
>> +dw_hdmi_soc: dw-hdmi-soc@0 {
>> +	compatible = "snps,dw-hdmi-soc";
> Perhaps just make it
>
> 	compatible = "...";
> ?

Yeah, probably its better.

>
>> +	reg = <0x11c00 0x1000>; /* EDIDs */
> This is not relevant and undocumented, will likely be part of documentation 
> of other binding thus I'd suggest dropping this reg property.

Ok.

>
>> +	#address-cells = <1>;
>> +	#size-cells = <1>;
>> +	ranges;
>> +
>> +	hdmi-rx@0 {
>> +		compatible = "snps,dw-hdmi-rx";
>> +		reg = <0x0 0x10000>;
>> +		interrupts = <1 2>;
>> +		edid-phandle = <&dw_hdmi_soc>;
>> +
>> +		clocks = <&dw_hdmi_refclk>;
>> +		clock-names = "cfg";
>> +
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +
>> +		hdmi-phy@fc {
>> +			compatible = "snps,dw-hdmi-phy-e405";
>> +			reg = <0xfc>;
>> +
>> +			clocks = <&dw_hdmi_refclk>;
>> +			clock-names = "cfg";
>> +		};
>> +	};
>> +};
> Otherwise looks good. I'll likely not have comments to the other patches.

Thanks for the review!

Best regards,
Jose Miguel Abreu

>
> --
> Regards,
> Sylwester
>
Sylwester Nawrocki July 6, 2017, 8:30 p.m. UTC | #3 
On 07/06/2017 12:24 PM, Jose Abreu wrote:
>>> +- edid-phandle: phandle to the EDID handler block.
>>
>> Could you make this property optional and when it is missing assume that device
>> corresponding to the parent node of this node handles EDID? This way we could
>> avoid having property pointing to the parent node.
>
> Hmm, this is for the CEC notifier. Do you mean I should grab the
> parent device for the notifier? This property is already optional
> if cec is not enabled though.
 
Yes, device associated with the parent node. Something like:

 - edid-phandle - phandle to the EDID handler block; if this property is
  not specified it is assumed that EDID is handled by device described 
  by parent node of the HDMI RX node

Not sure if it is any better than always requiring edid-phandle property,
even when it is pointing to the parent node. We would need a DT maintainer's
opinion on that.

--
Thanks,
Sylwester
Jose Abreu July 7, 2017, 9:31 a.m. UTC | #4 
On 06-07-2017 21:30, Sylwester Nawrocki wrote:
> On 07/06/2017 12:24 PM, Jose Abreu wrote:
>>>> +- edid-phandle: phandle to the EDID handler block.
>>> Could you make this property optional and when it is missing assume that device
>>> corresponding to the parent node of this node handles EDID? This way we could
>>> avoid having property pointing to the parent node.
>> Hmm, this is for the CEC notifier. Do you mean I should grab the
>> parent device for the notifier? This property is already optional
>> if cec is not enabled though.
>  
> Yes, device associated with the parent node. Something like:
>
>  - edid-phandle - phandle to the EDID handler block; if this property is
>   not specified it is assumed that EDID is handled by device described 
>   by parent node of the HDMI RX node
>
> Not sure if it is any better than always requiring edid-phandle property,
> even when it is pointing to the parent node. We would need a DT maintainer's
> opinion on that.

I will change and resend. I also have to fix a kbuild error when
cec is not enabled.

Best regards,
Jose Miguel Abreu

>
> --
> Thanks,
> Sylwester
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.txt b/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.txt
new file mode 100644
index 0000000..449b8a2
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.txt
@@ -0,0 +1,70 @@ 
+Synopsys DesignWare HDMI RX Decoder
+===================================
+
+This document defines device tree properties for the Synopsys DesignWare HDMI
+RX Decoder (DWC HDMI RX). It doesn't constitute a device tree binding
+specification by itself but is meant to be referenced by platform-specific
+device tree bindings.
+
+When referenced from platform device tree bindings the properties defined in
+this document are defined as follows.
+
+- compatible: Shall be "snps,dw-hdmi-rx".
+
+- reg: Memory mapped base address and length of the DWC HDMI RX registers.
+
+- interrupts: Reference to the DWC HDMI RX interrupt and 5v sense interrupt.
+
+- clocks: Phandle to the config clock block.
+
+- clock-names: Shall be "cfg".
+
+- edid-phandle: phandle to the EDID handler block.
+
+- #address-cells: Shall be 1.
+
+- #size-cells: Shall be 0.
+
+You also have to create a subnode for phy driver. Phy properties are as follows.
+
+- compatible: Shall be "snps,dw-hdmi-phy-e405".
+
+- reg: Shall be JTAG address of phy.
+
+- clocks: Phandle for cfg clock.
+
+- clock-names:Shall be "cfg".
+
+A sample binding is now provided. The compatible string is for a SoC which has
+has a Synopsys DesignWare HDMI RX decoder inside.
+
+Example:
+
+dw_hdmi_soc: dw-hdmi-soc@0 {
+	compatible = "snps,dw-hdmi-soc";
+	reg = <0x11c00 0x1000>; /* EDIDs */
+	#address-cells = <1>;
+	#size-cells = <1>;
+	ranges;
+
+	hdmi-rx@0 {
+		compatible = "snps,dw-hdmi-rx";
+		reg = <0x0 0x10000>;
+		interrupts = <1 2>;
+		edid-phandle = <&dw_hdmi_soc>;
+
+		clocks = <&dw_hdmi_refclk>;
+		clock-names = "cfg";
+
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		hdmi-phy@fc {
+			compatible = "snps,dw-hdmi-phy-e405";
+			reg = <0xfc>;
+
+			clocks = <&dw_hdmi_refclk>;
+			clock-names = "cfg";
+		};
+	};
+};