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

[Macpaul Lin]

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@xxxxxxxxxxxx>
> > ---
> >   .../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