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

[Jose Abreu]

Hi Sylwester,


Thanks for the feedback!


On 18-06-2017 18:34, Sylwester Nawrocki wrote:
> Hi Jose,
>
> On 06/16/2017 06:38 PM, Jose Abreu wrote:
>> Document the bindings for the Synopsys Designware HDMI RX.
>>
>> Signed-off-by: Jose Abreu <joabreu@xxxxxxxxxxxx>
>> new file mode 100644
>> index 0000000..d30cc1e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.txt
>> @@ -0,0 +1,45 @@
>> +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.
>> +
>> +- 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.
>> +
>> +- snps,hdmi-phy-jtag-addr: JTAG address of the phy to be used.
>> +
>> +- snps,hdmi-phy-version: Version of the phy to be used.
>> +
>> +- snps,hdmi-phy-cfg-clk: Value of the cfg clk that is delivered to phy.
>> +
>> +- snps,hdmi-phy-driver: *Exact* name of the phy driver to be used.
> I don't think we can put Linux driver name in DT like this, devicetree 
> is supposed to be describing hardware in OS agnostic way.

Yeah, I was aware about that, its just that I have a few
limitations that I need to address. I will highlight them bellow
but I think that with your proposed DT bindings and using
of_platform_populate I can address them :)

>
>> +- snps,hdmi-ctl-cfg-clk: Value of the cfg clk that is delivered to the
>> +controller.
> How about creating a separate node for the PHY? The binding could then 
> be something like:
>
>
> /* Fixed clock needed only if respective clock is not already defined    
>    in the system */
>
> refclk: clock {
> 	compatible = "fixed-clock";
> 	#clock-cells = <0>;
> 	clock-frequency = <25000000>;
> };
>
> hdmi_phy: phy@f3 {
> 	/* PHY version can be derived from the compatible string */
> 	compatible = "snps,dw-hdmi-phy-e405"; 	
> 	/* JTAG address of the PHY */
> 	reg = <0xf3>
>
> 	clocks = <&refclk>
> 	clock-names = "cfg-clk";
> }
>
> hdmi-controller@0 {
> 	compatible = "snps,dw-hdmi-rx-controller-vX.X";
> 	reg = <0x0 0x10000>;
> 	interrupts = <1 3>; 	
> 	phys = <&hdmi_phy>;
>
> 	clocks = <&refclk>
> 	clock-names = "cfg-clk";
> };
>
> Or it might be better to make the PHY node child node of the RX controller 
> node, since the RX controller could be considered a controller of the JTAG 
> bus IIUC:
>
> refclk: clock {
> 	compatible = "fixed-clock";
> 	#clock-cells = <0>;
> 	clock-frequency = <25000000>;
> };
>
> hdmi-controller@0 {
> 	compatible = "snps,dw-hdmi-rx-controller-xxx";
> 	reg = <0x0 0x10000>;
> 	interrupts = <1 3>; 	
> 	clocks = <&refclk>
> 	clock-names = "cfg-clk";
> 	#address-cells = <1>;
> 	#size-cells = <0>;
>
> 	phy@f3 {
> 		/* PHY version can be derived from the compatible string */
> 		compatible = "snps,dw-hdmi-phy-e405";
>  		/* address of the PHY on the JTAG bus */
> 		reg = <0xf3>
>
> 		clocks = <&refclk>
> 		clock-names = "cfg-clk";
> 	}
> };
>
> Then rather than creating platform device in the RX controller driver 
> for the PHY just of_platform_populate() could be used.

Using fixed-clock was already in my todo list. Regarding phy I
need to pass pdata so that the callbacks between controller and
phy are established. I also need to make sure that phy driver
will be loaded by the controller driver. Hmm, and also address of
the phy on th JTAG bus is fed to the controller driver not to the
phy driver. Maybe leave the property as is (the
"snps,hdmi-phy-jtag-addr") or parse it from the phy node?

I also need to pass pdata to the controller driver (the callbacks
for 5v handling) which are agnostic of the controller. These
reasons prevented me from adding compatible strings to both
drivers and just use a wrapper driver instead. This way i do
"modprobe wrapper_driver" and I get all the drivers loaded via
request_module(). Still, I like your approach much better. I saw
that I can pass pdata using of_platform_populate, could you
please confirm if I can still maintain this architecture (i.e.
prevent modules from loading until I get all the chain setup)?

Following your approach I could get something like this:

hdmi_system@YYYY {
    compatible = "snps,dw-hdmi-rx-wrapper";
    reg = <0xYYYY 0xZZZZ>;
    interrupts = <3>;
    #address-cells = <1>;
    #size-cells = <1>;

    hdmi_controller@0 {
        compatible = "snps,dw-hdmi-rx-controller";
        reg = <0x0 0x10000>;
        interrupts = <1>;
        edid-phandle = <&hdmi_system>;
        clocks = <&refclk>;
        clock-names = "ref-clk";
        #address-cells = <1>;
        #size-cells = <0>;

        hdmi_phy@f3 {
            compatible = "snps,dw-hdmi-phy-e405";
            reg = <0xf3>;
            clocks = <&cfgclk>;
            clock-names = "cfg-clk";
        }
    }
};

And then snps,dw-hdmi-rx-wrapper would call of_platform_populate
for controller which would instead call of_platform_populate for
phy. Is this possible, and maintainable? Isn't the controller
driver get auto loaded because of the compatible string match?
And one more thing: The reg address of the hdmi_controller: Isn't
this relative to the parent node? I mean isn't this going to be
0xYYYY + 0x0? Because I don't want that :/

Best regards,
Jose Miguel Abreu

>
>> +- edid-phandle: phandle to the EDID driver. It can be, for example, the main
>> +wrapper driver.
>> +
>> +A sample binding is now provided. The compatible string is for a wrapper driver
>> +which then instantiates the Synopsys Designware HDMI RX decoder driver.
> I would avoid talking about drivers in DT binding documentation, we should 
> rather refer to hardware blocks.
>
>> +Example:
>> +
>> +dw_hdmi_wrapper: dw-hdmi-wrapper@0 {
>> +	compatible = "snps,dw-hdmi-rx-wrapper";
>> +	reg = <0x0 0x10000>; /* controller regbank */
>> +	interrupts = <1 3>; /* controller interrupt + 5v sense interrupt */
>> +	snps,hdmi-phy-driver = "dw-hdmi-phy-e405";
>> +	snps,hdmi-phy-version = <405>;
>> +	snps,hdmi-phy-cfg-clk = <25>; /* MHz */
>> +	snps,hdmi-ctl-cfg-clk = <25>; /* MHz */
>> +	snps,hdmi-phy-jtag-addr = /bits/ 8 <0xfc>;
>> +	edid-phandle = <&dw_hdmi_wrapper>;
>> +};
> --
> Regards,
> Sylwester
>