Re: [PATCH 4/4] arm64: dts: allwinner: h700: Add RG35XX-H DTS

[Andre Przywara <andre.przywara@arm.com>]

On Sun, 14 Apr 2024 20:33:48 +1200
Ryan Walklin <ryan@testtoast.com> wrote:

Hi Ryan,

> From: Ryan Walklin <ryan@testtoast.com>
> 
> The RG35XX-H adds thumbsticks, a stereo speaker, and a second USB port
> to the RG35XX-Plus, and has a horizontal form factor.
> 
> Enabled in this DTS:
> - Thumbsticks
> - Second USB port
> 
> Signed-off-by: Ryan Walklin <ryan@testtoast.com>
> ---
>  .../sun50i-h700-anbernic-rg35xx-h.dts         | 38 +++++++++++++++++++
>  1 file changed, 38 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-h.dts
> 
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-h.dts b/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-h.dts
> new file mode 100644
> index 000000000000..5b7de7bfc458
> --- /dev/null
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-h.dts
> @@ -0,0 +1,38 @@
> +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +/*
> + * Copyright (C) 2024 Andre Przywara <andre.przywara@arm.com>.

Please remove my copyright, I didn't have my hands in this. Copyrights
are not that important anyway, since it's the license that rules.

> + * Copyright (C) 2024 Ryan Walklin <ryan@testtoast.com>.
> + * Copyright (C) 2024 Chris Morgan <macroalpha82@gmail.com>.
> + */
> +
> +
> +/dts-v1/;

Same as in 3/4: redundant line.

> +#include "sun50i-h700-anbernic-rg35xx-plus.dts"
> +
> +/ {
> +    model = "Anbernic RG35XX H";
> +    compatible = "anbernic,rg35xx-h", "allwinner,sun50i-h700";
> +
> +    gpio-keys: gpio-keys-thumb {

Is it intended to be in a separate node from the other keys? Just
reference the existing node (below, outside of the root node) and add
the keys in there.

> +       compatible = "gpio-keys";
> +
> +        keyThumbLeft {
> +            label = "GPIO Thumb Left";
> +            gpios = <&pio 4 8 GPIO_ACTIVE_LOW>; /* PE8 */
> +            linux,input-type = <EV_KEY>;
> +            linux,code = <BTN_THUMBL>;
> +        };
> +
> +        keyThumbRight {
> +            label = "GPIO Thumb Right";
> +            gpios = <&pio 4 9 GPIO_ACTIVE_LOW>; /* PE9 */
> +            linux,input-type = <EV_KEY>;
> +            linux,code = <BTN_THUMBR>;
> +        };
> +    };
> +};

I missed that in the first DTS, but you should move the 'status =
"okay";' lines for EHCI1/OHCI1 from patch 2/4 into here, since the
second USB port should stay disabled on those other two boards.

> +
> +&usbotg {
> +    dr_mode = "peripheral";

That looks odd. I do understand that a second USB port allows the
first to be dedicated to OTG, but I feels still weird that the default
for the only one on the other two boards is host.
Can you say what the expected use case is? Are people connecting things
like controllers to the only USB port? Otherwise I would expect this
more to be a charging port, to which peripheral would be a better fit.
I guess ultimately this would be "otg", but we need the AXP717 USB-C
support for that, which is not ready yet.

> +    status = "okay";
> +};

If there is an enable GPIO for VBUS, then the respective regulator
should be referenced here.

Cheers,
Andre