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

[Jose Abreu]

Hi Rob,


On 10-07-2017 16:24, Rob Herring wrote:
> On Mon, Jun 26, 2017 at 11:42 AM, Jose Abreu  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

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

[Rob Herring]

On Mon, Jun 26, 2017 at 11:42 AM, Jose Abreu  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

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

[Jose Abreu]

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 
>> Cc: Carlos Palminha 
>> Cc: Rob Herring 
>> Cc: Mark Rutland 
>> Cc: Mauro Carvalho Chehab 
>> Cc: Hans Verkuil 
>> Cc: Sylwester Nawrocki 
>>
>> 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 000..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 0x1>;
>> +interrupts = <1 2>;
>> +edid-phandle = <_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 = <_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 = <_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
>>
>>

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

[Rob Herring]

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 
> Cc: Carlos Palminha 
> Cc: Rob Herring 
> Cc: Mark Rutland 
> Cc: Mauro Carvalho Chehab 
> Cc: Hans Verkuil 
> Cc: Sylwester Nawrocki 
> 
> 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 000..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 0x1>;
> + interrupts = <1 2>;
> + edid-phandle = <_hdmi_soc>;

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

> +
> + clocks = <_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 = <_hdmi_refclk>;
> + clock-names = "cfg-clk";
> + };
> + };
> +};
> -- 
> 1.9.1
> 
>