Re: [PATCH 2/2] arm64: dts: amlogic: add support for xiaomi-aquaman/Mi TV Stick

[Martin Blumenstingl <martin.blumenstingl@xxxxxxxxxxxxxx>]

Hello,

thanks for the patch - I have a few small comments below.

On Fri, Jan 31, 2025 at 9:05 PM Ferass El Hafidi
<funderscore@xxxxxxxxxxxxxxxx> wrote:
[...]
> +       cvbs-connector {
> +               /* No CVBS connector */
> +               status = "disabled";
> +       };
You're inheriting  "meson-gxl-s805x.dtsi" but I can't see that this -
or its parents - define a cvbs-connector node anywhere.
Can we just omit it completely

[...]
> +       vcc_5v: regulator-vcc-5v {
> +               compatible = "regulator-fixed";
> +               regulator-name = "VCC_5V";
> +               regulator-min-microvolt = <5000000>;
> +               regulator-max-microvolt = <5000000>;
> +       };
> +
> +
nit-pick: since you're planning to send a v2 anyways, keep it at one
blank line here

[...]
> +&ethmac {
> +       /* No Ethernet connector */
> +       status = "disabled";
> +};
Similar to the cvbs-connector: status = "disabled" should be the default here

> +&internal_phy {
> +       pinctrl-0 = <&eth_link_led_pins>, <&eth_act_led_pins>;
> +       pinctrl-names = "default";
> +};
why are there Ethernet LED pins if there's no Ethernet connector?

> +&ir {
> +       /* No IR */
> +       status = "disabled";
> +};
This is also disabled by default

[...]
> +&pwm_ef {
> +       status = "okay";
> +       pinctrl-0 = <&pwm_e_pins>;
> +       pinctrl-names = "default";
> +       clocks = <&clkc CLKID_FCLK_DIV4>;
> +       clock-names = "clkin0";
> +};
Please drop clocks and clock-names to make this work with linux-next.
Background: [0] is queued for the next release and it means that the
PWM controller driver will now pick the "best" clock on it's own and
overriding here will likely result in your board not booting.

[...]
> +&usb {
> +       status = "okay";
> +       dr_mode = "host";
> +};
> +
> +&usb2_phy0 {
> +       phy-supply = <&vcc_5v>;
> +};
I'm guilty of this myself and I think you can be better here:
Use vbus-supply = <&vcc_5v>; in the &usb node to correctly describe
where the USB VBUS signal is coming from.
(I believe that the PHY is not internally powered by 5V as the SoC
doesn't take any 5V inputs)


Best regards,
Martin


[0] https://lore.kernel.org/linux-amlogic/20241227212514.1376682-1-martin.blumenstingl@xxxxxxxxxxxxxx/