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

[AngeloGioacchino Del Regno]

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.
"

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

>   	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?

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 {