Re: [PATCH v12 3/6] dt-bindings: display: Add Loongson display controller

[Sui Jingfeng <15330273260@189.cn>]

On 2022/3/27 20:54, Jiaxun Yang wrote:
>
>
> 在 2022/3/27 12:38, Sui Jingfeng 写道:
>> Add DT bindings and simple usages for Loongson display controller
>> found in LS7A1000 bridges chip and LS2k1000 SoC.
>>
>> Signed-off-by: Sui Jingfeng <15330273260@189.cn>
> [...]
>> +
>> +  - |
>> +    #include <dt-bindings/interrupt-controller/irq.h>
>> +    bus {
>> +
>> +        #address-cells = <3>;
>> +        #size-cells = <2>;
>> +        #interrupt-cells = <2>;
>> +
>> +        display-controller@6,1 {
>> +            compatible = "loongson,ls7a1000-dc";
>> +            reg = <0x3100 0x0 0x0 0x0 0x0>;
>> +            interrupts = <28 IRQ_TYPE_LEVEL_HIGH>;
>> +
>> +            #address-cells = <1>;
>> +            #size-cells = <0>;
>> +
>> +            i2c@6 {
>> +                compatible = "loongson,gpio-i2c";
>> +                reg = <0x00001650 0x00000020>;
> Hi Jingfeng,
>
> Thanks for your patch.
>
> Just curious about what is this "reg" for?

Hi, Jiaxun

Thanks for you take valuable time to review my patch.

Without it make dt_binding_check generate warnings:

Documentation/devicetree/bindings/display/loongson/loongson,display-controller.example.dts:65.23-73.19: 
Warning (unit_address_vs_reg): 
/example-1/bus/display-controller@6,1/i2c@6: node has a unit name, but 
no reg or ranges property

reg are the control register offset and size of the dedicate GPIO, they 
are not get used by the driver currently,

put in there just for silence the warning .


>> +                loongson,nr = <6>;
> Why nr start from 6?


Bus number greater than 6 is safe  because ls7a1000 bridge have 6 hardware I2C controller integrated. but 
the driver for it is not upstream yet. To avoid potential conflict with the bus number of the hardware I2C driver.

In the future, if someone contribute the hardware I2C driver to upstream,
you don't need change it. Let me give you an example to show what it will be:

  	aliases {
		i2c0 = &i2c0;
		i2c1 = &i2c1;
		i2c2 = &i2c2;
		i2c3 = &i2c3;
		i2c4 = &i2c4;
		i2c5 = &i2c5;
		i2c6 = &i2c6;
		i2c7 = &i2c7;
	};

	i2c0: i2c@10090000 {
		compatible = "loongson,ls-i2c";
		reg = <0x10090000 0x8>;
		interrupts = <73>;
		interrupt-parent = <&platic>;
		#address-cells = <1>;
		#size-cells = <0>;
	};


	i2c1: i2c@10090100 {
		compatible = "loongson,ls-i2c";
		reg = <0x10090100 0x8>;
		interrupts = <73>;
		interrupt-parent = <&platic>;
		#address-cells = <1>;
		#size-cells = <0>;
	};

	i2c2: i2c@10090200 {
		compatible = "loongson,ls-i2c";
		reg = <0x10090200 0x8>;
		interrupts = <73>;
		interrupt-parent = <&platic>;
		#address-cells = <1>;
		#size-cells = <0>;
	};

	i2c3: i2c@10090300 {
		compatible = "loongson,ls-i2c";
		reg = <0x10090300 0x8>;
		interrupts = <73>;
		interrupt-parent = <&platic>;
		#address-cells = <1>;
		#size-cells = <0>;
	};

	i2c4: i2c@10090400 {
		compatible = "loongson,ls-i2c";
		reg = <0x10090400 0x8>;
		interrupts = <73>;
		interrupt-parent = <&platic>;
		#address-cells = <1>;
		#size-cells = <0>;
	};

	i2c5: i2c@10090500 {
		compatible = "loongson,ls-i2c";
		reg = <0x10090500 0x8>;
		interrupts = <73>;
		interrupt-parent = <&platic>;
		#address-cells = <1>;
		#size-cells = <0>;
	};

> The approach you are handling I2C seems to be wired..
>

It is not wired,  you can change it to 0 or 1 it you like currently,

you can even remove loongson,nr = <6> and loongson,nr = <7>,

then the i2c core driver will automatically  allocate one for you.

It is very flexible actually.

> Actually you can reference how network subsystem is handling
> MDIO controller built-in into Ethernet controller [1] in this case. It is
> basically the same problem.
>
> [1]: 
> https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/net/snps,dwmac.yaml
>
> Thanks.
> - Jiaxun
>
>> +                loongson,sda = <0>;
>> +                loongson,scl = <1>;
>> +                loongson,udelay = <5>;
>> +                loongson,timeout = <2200>;
>> +            };
>> +
>> +            i2c@7 {
>> +                compatible = "loongson,gpio-i2c";
>> +                reg = <0x00001650 0x00000020>;
>> +                loongson,nr = <7>;
>> +                loongson,sda = <2>;
>> +                loongson,scl = <3>;
>> +                loongson,udelay = <5>;
>> +                loongson,timeout = <2200>;
>> +            };
>> +
>> +            ports {
>> +                #address-cells = <1>;
>> +                #size-cells = <0>;
>> +                port@0 {
>> +                    reg = <0>;
>> +                    endpoint {
>> +                            remote-endpoint = <&vga_encoder_in>;
>> +                    };
>> +                };
>> +
>> +                port@1 {
>> +                    reg = <1>;
>> +                    endpoint {
>> +                            remote-endpoint = <&dvi_encoder_in>;
>> +                    };
>> +                };
>> +            };
>> +        };
>> +    };
>> +
>> +  - |
>> +    #include <dt-bindings/interrupt-controller/irq.h>
>> +    bus {
>> +
>> +        #address-cells = <3>;
>> +        #size-cells = <2>;
>> +        #interrupt-cells = <2>;
>> +
>> +        display-controller@6,0 {
>> +            compatible = "loongson,ls2k1000-dc";
>> +            reg = <0x3100 0x0 0x0 0x0 0x0>;
>> +            interrupts = <28 IRQ_TYPE_LEVEL_HIGH>;
>> +
>> +            ports {
>> +                #address-cells = <1>;
>> +                #size-cells = <0>;
>> +                port@0 {
>> +                    reg = <0>;
>> +                    endpoint {
>> +                            remote-endpoint = <&panel_in>;
>> +                    };
>> +                };
>> +
>> +                port@1 {
>> +                    reg = <1>;
>> +                    endpoint {
>> +                            remote-endpoint = <&hdmi_encoder_in>;
>> +                    };
>> +                };
>> +            };
>> +        };
>> +    };
>> +...
>