Re: [PATCH v2 2/3] arm64: dts: ti: Add support for Variscite VAR-SOM-AM62

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Missing clock rate assignment for `audio_refclk1` will cause a divide-by-zero panic in the `wm8904` codec driver.
- [Medium] Missing Bluetooth device node despite being advertised in the commit message.
- [Low] Unused pinctrl node `pinctrl_vdd_mmc2` left over from v1 regulator removal.
--

--- Patch [2]: [PATCH v2 2/3] arm64: dts: ti: Add support for Variscite VAR-SOM-AM62 ---
commit 6f8bdf4ac0be91ef53a90c5674c70f3e23423f8b
Author: Stefano Radaelli <stefano.r@variscite.com>

arm64: dts: ti: Add support for Variscite VAR-SOM-AM62

Add device tree support for the Variscite VAR-SOM-AM62 system on module.
This SOM is designed to be used with various carrier boards.

Link: https://variscite.com/system-on-module-som/ti-sitara-am62x/var-som-am62/

> diff --git a/arch/arm64/boot/dts/ti/k3-am625-var-som.dtsi b/arch/arm64/boot/dts/ti/k3-am625-var-som.dtsi
> new file mode 100644
> index 0000000000000..08afe751e4d6c
> --- /dev/null
> +++ b/arch/arm64/boot/dts/ti/k3-am625-var-som.dtsi

[ ... ]

> +&main_i2c2 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_i2c2>;
> +	clock-frequency = <400000>;
> +	status = "okay";
> +
> +	wm8904: audio-codec@1a {
> +		compatible = "wlf,wm8904";
> +		reg = <0x1a>;
> +		#sound-dai-cells = <0>;
> +		clocks = <&audio_refclk1>;
> +		clock-names = "mclk";

[Severity: Critical]
Is a clock rate assignment missing for audio_refclk1 here?

If the DT does not specify assigned-clock-rates for audio_refclk1,
clk_get_rate() will return 0. When a userspace program opens the ALSA PCM
device and triggers hardware parameters configuration, simple-audio-card
calls snd_soc_dai_set_sysclk().

This falls back to wm8904_set_fll() which will execute Ndiv = target / Fref
with Fref = 0, causing a kernel panic from a divide-by-zero.

[ ... ]

> +	pinctrl_uart5: main-uart5-default-pins {
> +		pinctrl-single,pins = <
> +			AM62X_IOPAD(0x00ec, PIN_INPUT, 4) /* (AA24) VOUT0_DATA13.UART5_CTSn */
> +			AM62X_IOPAD(0x00e8, PIN_OUTPUT, 4) /* (AB25) VOUT0_DATA12.UART5_RTSn */
> +			AM62X_IOPAD(0x00d0, PIN_INPUT, 4) /* (Y23) VOUT0_DATA6.UART5_RXD */
> +			AM62X_IOPAD(0x00d4, PIN_OUTPUT, 4) /* (AA25) VOUT0_DATA7.UART5_TXD */
> +		>;
> +	};
> +
> +	pinctrl_vdd_mmc2: main-vdd-mmc2-default-pins {
> +		pinctrl-single,pins = <
> +			AM62X_IOPAD(0x00d8, PIN_INPUT, 7) /* (V21) VOUT0_DATA8.GPIO0_53 */
> +			AM62X_IOPAD(0x00dc, PIN_INPUT, 7) /* (W21) VOUT0_DATA9.GPIO0_54 */
> +			AM62X_IOPAD(0x00f0, PIN_INPUT, 7) /* (Y22) VOUT0_DATA14.GPIO0_59 */
> +			AM62X_IOPAD(0x00f4, PIN_INPUT, 7) /* (AA21) VOUT0_DATA15.GPIO0_60 */
> +		>;
> +	};
> +};

[Severity: Low]
Is this pinctrl group still used?

It appears to be left over from the v1 removal of the reg_vdd_mmc2
regulator. The node is unreferenced, leaving dead code in the device tree.

If these GPIOs were previously used to sequence power for the wireless
modules, does removing the regulator without migrating them to an mmc-pwrseq
or shutdown-gpios cause issues with powering up the wireless modules?

[ ... ]

> +&main_uart5 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_uart5>;
> +	uart-has-rtscts;
> +	status = "okay";
> +};

[Severity: Medium]
Is the bluetooth child node missing from this UART interface?

The commit message mentions Bluetooth 5.2 support. Without a bluetooth
child node defined here, the serdev bus scan during boot won't find it,
the hci_bcm driver will not bind, and the required Broadcom firmware won't
be loaded.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1783687321.git.stefano.r@variscite.com?part=2