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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 6 Jul 2017 11:24:36 +0100
From:   Jose Abreu <Jose.Abreu@...opsys.com>
To:     Sylwester Nawrocki <snawrocki@...nel.org>,
        Jose Abreu <Jose.Abreu@...opsys.com>
CC:     <linux-media@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        "Carlos Palminha" <CARLOS.PALMINHA@...opsys.com>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Hans Verkuil <hans.verkuil@...co.com>,
        <devicetree@...r.kernel.org>
Subject: Re: [PATCH v6 4/4] dt-bindings: media: Document Synopsys Designware
 HDMI RX

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@...opsys.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
>