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

[Jose Abreu]

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

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

[Sylwester Nawrocki]

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

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

[Jose Abreu]

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 
>> ---
>>   .../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 000..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 0x1>;
>> +interrupts = <1 2>;
>> +edid-phandle = <_hdmi_soc>;
>> +
>> +clocks = <_hdmi_refclk>;
>> +clock-names = "cfg";
>> +
>> +#address-cells = <1>;
>> +#size-cells = <0>;
>> +
>> +hdmi-phy@fc {
>> +compatible = "snps,dw-hdmi-phy-e405";
>> +reg = <0xfc>;
>> +
>> +clocks = <_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
>  

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

[Sylwester Nawrocki]

On 07/04/2017 04:11 PM, Jose Abreu wrote:
> Document the bindings for the Synopsys Designware HDMI RX.
> 
> Signed-off-by: Jose Abreu 

> ---
>   .../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 000..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 0x1>;
> + interrupts = <1 2>;
> + edid-phandle = <_hdmi_soc>;
> +
> + clocks = <_hdmi_refclk>;
> + clock-names = "cfg";
> +
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + hdmi-phy@fc {
> + compatible = "snps,dw-hdmi-phy-e405";
> + reg = <0xfc>;
> +
> + clocks = <_hdmi_refclk>;
> + clock-names = "cfg";
> + };
> + };
> +};

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

--
Regards,
Sylwester