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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAFBinCA+=aqE_=eMFtwpHYVX+8CrtecvHexpHNe_7nh_dDhKYw@mail.gmail.com>
Date: Sat, 1 Feb 2025 12:01:39 +0100
From: Martin Blumenstingl <martin.blumenstingl@...glemail.com>
To: Ferass El Hafidi <funderscore@...tmarketos.org>
Cc: Neil Armstrong <neil.armstrong@...aro.org>, Kevin Hilman <khilman@...libre.com>, 
	Jerome Brunet <jbrunet@...libre.com>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	linux-amlogic@...ts.infradead.org, linux-arm-kernel@...ts.infradead.org, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	~postmarketos/upstreaming@...ts.sr.ht, Artur Weber <aweber.kernel@...il.com>, 
	Karl Chan <exxxxkc@...googleoff.me>
Subject: Re: [PATCH 2/2] arm64: dts: amlogic: add support for
 xiaomi-aquaman/Mi TV Stick

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@...tmarketos.org> 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@googlemail.com/