diff mbox

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

Message ID 8ebe3dfcd61a1c8cfa99102c376ad26b2bfbd254.1497978963.git.joabreu@synopsys.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jose Abreu June 20, 2017, 5:26 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>

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

Rob Herring (Arm) June 23, 2017, 9:58 p.m. UTC | #1 
On Tue, Jun 20, 2017 at 06:26:12PM +0100, Jose Abreu wrote:
> 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>
> 
> 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
> 
> 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..efb0ac3
> --- /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-clk".

"-clk" is redundant.

Seems strange that this is the only clock. The only other clock is the 
HDMI clock from the HDMI transmitter.

> +
> +- 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-clk".
> +
> +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";

Not documented.

> +	reg = <0x11c00 0x1000>; /* EDIDs */
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +	ranges;
> +
> +	dw_hdmi_rx@0 {

hdmi-rx@0

> +		compatible = "snps,dw-hdmi-rx";
> +		reg = <0x0 0x10000>;
> +		interrupts = <1 2>;
> +		edid-phandle = <&dw_hdmi_soc>;

Don't need this if it is the parent node.

> +
> +		clocks = <&dw_hdmi_refclk>;
> +		clock-names = "cfg-clk";
> +
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		dw_hdmi_phy_e405@fc {

hdmi-phy@fc

> +			compatible = "snps,dw-hdmi-phy-e405";
> +			reg = <0xfc>;
> +
> +			clocks = <&dw_hdmi_refclk>;
> +			clock-names = "cfg-clk";
> +		};
> +	};
> +};
> -- 
> 1.9.1
> 
>
Jose Abreu June 26, 2017, 4:42 p.m. UTC | #2 
Hi Rob,


On 23-06-2017 22:58, Rob Herring wrote:
> On Tue, Jun 20, 2017 at 06:26:12PM +0100, Jose Abreu wrote:
>> 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>
>>
>> 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
>>
>> 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..efb0ac3
>> --- /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-clk".
> "-clk" is redundant.
>
> Seems strange that this is the only clock. The only other clock is the 
> HDMI clock from the HDMI transmitter.

Its a receiver so it gets driven by the transmitter. In my
implementation I only need to configure this clock in the
controller so that it knows the timebase. I will change to "cfg"
only then.

>
>> +
>> +- 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-clk".
>> +
>> +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";
> Not documented.

Yes, its a sample binding which reflects a wrapper driver that
shall instantiate the controller driver (and this wrapper driver
is not in this patch series), should I remove this?

>
>> +	reg = <0x11c00 0x1000>; /* EDIDs */
>> +	#address-cells = <1>;
>> +	#size-cells = <1>;
>> +	ranges;
>> +
>> +	dw_hdmi_rx@0 {
> hdmi-rx@0

Ok.

>
>> +		compatible = "snps,dw-hdmi-rx";
>> +		reg = <0x0 0x10000>;
>> +		interrupts = <1 2>;
>> +		edid-phandle = <&dw_hdmi_soc>;
> Don't need this if it is the parent node.

Sometimes it will not be the parent node (if edid handling is
done in a separate driver, for example).

>
>> +
>> +		clocks = <&dw_hdmi_refclk>;
>> +		clock-names = "cfg-clk";
>> +
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +
>> +		dw_hdmi_phy_e405@fc {
> hdmi-phy@fc

Ok.

>
>> +			compatible = "snps,dw-hdmi-phy-e405";
>> +			reg = <0xfc>;
>> +
>> +			clocks = <&dw_hdmi_refclk>;
>> +			clock-names = "cfg-clk";

I will also change this to "cfg" only.

Thanks for the review!

Best regards,
Jose Miguel Abreu

>> +		};
>> +	};
>> +};
>> -- 
>> 1.9.1
>>
>>
Rob Herring (Arm) July 10, 2017, 3:24 p.m. UTC | #3 
On Mon, Jun 26, 2017 at 11:42 AM, Jose Abreu <Jose.Abreu@synopsys.com> wrote:
> Hi Rob,
>
>
> On 23-06-2017 22:58, Rob Herring wrote:
>> On Tue, Jun 20, 2017 at 06:26:12PM +0100, Jose Abreu wrote:
>>> Document the bindings for the Synopsys Designware HDMI RX.
>>>

[...]

>>> +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";
>> Not documented.
>
> Yes, its a sample binding which reflects a wrapper driver that
> shall instantiate the controller driver (and this wrapper driver
> is not in this patch series), should I remove this?

Ah, I see. Please don't do this wrapper node like what was done on
DWC3. It should be all one node with the SoC specific part being a new
compatible string (and maybe additional properties). If there's really
some custom logic around the IP block, then maybe it makes sense, but
if it is just different clock connections, phys, resets, etc. those
don't need a separate node.

Rob
Jose Abreu July 10, 2017, 3:54 p.m. UTC | #4 
Hi Rob,


On 10-07-2017 16:24, Rob Herring wrote:
> On Mon, Jun 26, 2017 at 11:42 AM, Jose Abreu <Jose.Abreu@synopsys.com> wrote:
>> Hi Rob,
>>
>>
>> On 23-06-2017 22:58, Rob Herring wrote:
>>> On Tue, Jun 20, 2017 at 06:26:12PM +0100, Jose Abreu wrote:
>>>> Document the bindings for the Synopsys Designware HDMI RX.
>>>>
> [...]
>
>>>> +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";
>>> Not documented.
>> Yes, its a sample binding which reflects a wrapper driver that
>> shall instantiate the controller driver (and this wrapper driver
>> is not in this patch series), should I remove this?
> Ah, I see. Please don't do this wrapper node like what was done on
> DWC3. It should be all one node with the SoC specific part being a new
> compatible string (and maybe additional properties). If there's really
> some custom logic around the IP block, then maybe it makes sense, but
> if it is just different clock connections, phys, resets, etc. those
> don't need a separate node.

Ok. I guess I can just drop the SoC specific bindings as this was
more of a sample as how the EDID handle can be specified. I just
sent v8 now with the new bindings :) Thanks!

Best regards,
Jose Miguel Abreu

>
> Rob
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..efb0ac3
--- /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-clk".
+
+- 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-clk".
+
+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;
+
+	dw_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-clk";
+
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		dw_hdmi_phy_e405@fc {
+			compatible = "snps,dw-hdmi-phy-e405";
+			reg = <0xfc>;
+
+			clocks = <&dw_hdmi_refclk>;
+			clock-names = "cfg-clk";
+		};
+	};
+};