Re: [PATCH v2 2/2] arm64: dts: mediatek: mt8390-genio-700-evk: add keys and USB HUB

[Macpaul Lin <macpaul.lin@mediatek.com>]

On 10/8/24 16:20, AngeloGioacchino Del Regno wrote:
> Il 07/10/24 11:02, Macpaul Lin ha scritto:
>> 1. Add i2c, mmc to aliases.
>> 4. Add PMIC_KEY setting.
>> 5. Add USB HUB TUSB8020 to xhci1.
>> 6. Re-order spi2 node.
> 
> Please either add the aliases in a different commit, or add that to the 
> title.
> 
> arm64: dts: mediatek: mt8390-genio-700-evk: Add aliases, keys and USB HUB
> 
> Additionally, I'd really like to see a "conversation-like" description 
> instead
> of a kind-of-checkbox list.
> 
> Something like..
> 
> "
> Add aliases for the I2C and MMC/SD controllers to keep the numbering 
> consistent and
> describe the TUSB8020 hub present on the USB XHCI1 controller instance 
> to enable
> resetting it with its specific reset GPIO.
> 
> While at it, also move the spi2 node to keep nodes alphabetically ordered.
> "

Okay, Sorry for providing incorrect order of modified and added items.
I'll update in the next patch.

>>
>> Signed-off-by: Macpaul Lin <macpaul.lin@mediatek.com>
>> ---
>>   .../dts/mediatek/mt8390-genio-700-evk.dts     | 57 +++++++++++++++----
>>   1 file changed, 47 insertions(+), 10 deletions(-)
>>
>> Changes for v2:
>>   - Fix order of spi2.
>>   - Update pinctrl in i2c4 and rt1715.
>>   - Drop IT5205 and RT1715 nodes since the DTS are not completed yet.
>>   - Add #address-cells and #size-cells to xhci1 for supporting USB hubs.
>>
>> diff --git a/arch/arm64/boot/dts/mediatek/mt8390-genio-700-evk.dts 
>> b/arch/arm64/boot/dts/mediatek/mt8390-genio-700-evk.dts
>> index 96b272567cb1..3e77f59f2c74 100644
>> --- a/arch/arm64/boot/dts/mediatek/mt8390-genio-700-evk.dts
>> +++ b/arch/arm64/boot/dts/mediatek/mt8390-genio-700-evk.dts
>> @@ -23,6 +23,15 @@ / {
>>                "mediatek,mt8188";
>>       aliases {
>> +        i2c0 = &i2c0;
>> +        i2c1 = &i2c1;
>> +        i2c2 = &i2c2;
>> +        i2c3 = &i2c3;
>> +        i2c4 = &i2c4;
>> +        i2c5 = &i2c5;
>> +        i2c6 = &i2c6;
>> +        mmc0 = &mmc0;
>> +        mmc1 = &mmc1;
>>           serial0 = &uart0;
>>       };
>> @@ -249,7 +258,6 @@ &i2c3 {
>>   &i2c4 {
>>       pinctrl-names = "default";
>>       pinctrl-0 = <&i2c4_pins>;
>> -    pinctrl-1 = <&rt1715_int_pins>;
> 
> What is this extra change?
> Please describe it in the commit description.

okay, I'll fix this.

>>       clock-frequency = <1000000>;
>>       status = "okay";
>>   };
>> @@ -867,6 +875,17 @@ pins-wifi-enable {
>>   &pmic {
>>       interrupt-parent = <&pio>;
>>       interrupts = <222 IRQ_TYPE_LEVEL_HIGH>;
>> +
>> +    mt6359keys: keys {
> 
> Is there any reason why we can't just put this node in mt6359.dtsi?

That's a good point.
I think I can try to add this in a separate patch for mt6359.dtsi.

> Cheers,
> Angelo
> 
>> +        compatible = "mediatek,mt6359-keys";
>> +        mediatek,long-press-mode = <1>;
>> +        power-off-time-sec = <0>;
>> +
>> +        power-key {
>> +            linux,keycodes = <KEY_POWER>;
>> +            wakeup-source;
>> +        };
>> +    };
>>   };
>>   &scp {
>> @@ -874,6 +893,15 @@ &scp {
>>       status = "okay";
>>   };
>> +&spi2 {
>> +    pinctrl-0 = <&spi2_pins>;
>> +    pinctrl-names = "default";
>> +    mediatek,pad-select = <0>;
>> +    #address-cells = <1>;
>> +    #size-cells = <0>;
>> +    status = "okay";
>> +};
>> +
>>   &uart0 {
>>       pinctrl-0 = <&uart0_pins>;
>>       pinctrl-names = "default";
>> @@ -892,15 +920,6 @@ &uart2 {
>>       status = "okay";
>>   };
>> -&spi2 {
>> -    pinctrl-0 = <&spi2_pins>;
>> -    pinctrl-names = "default";
>> -    mediatek,pad-select = <0>;
>> -    #address-cells = <1>;
>> -    #size-cells = <0>;
>> -    status = "okay";
>> -};
>> -
>>   &u3phy0 {
>>       status = "okay";
>>   };
>> @@ -921,6 +940,24 @@ &xhci0 {
>>   &xhci1 {
>>       status = "okay";
>>       vusb33-supply = <&mt6359_vusb_ldo_reg>;
>> +    #address-cells = <1>;
>> +    #size-cells = <0>;
>> +
>> +    hub_2_0: hub@1 {
>> +        compatible = "usb451,8025";
>> +        reg = <1>;
>> +        peer-hub = <&hub_3_0>;
>> +        reset-gpios = <&pio 7 GPIO_ACTIVE_HIGH>;
>> +        vdd-supply = <&usb_hub_fixed_3v3>;
>> +    };
>> +
>> +    hub_3_0: hub@2 {
>> +        compatible = "usb451,8027";
>> +        reg = <2>;
>> +        peer-hub = <&hub_2_0>;
>> +        reset-gpios = <&pio 7 GPIO_ACTIVE_HIGH>;
>> +        vdd-supply = <&usb_hub_fixed_3v3>;
>> +    };
>>   };
>>   &xhci2 {
> 

Thanks
Macpaul Lin