Re: [PATCH v2 2/3] arm64: dts: ti: Add k3-am62l3-beaglebadge

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Incorrect muxmode for MCP23S18 reset pin
- [Medium] Invalid PWM period for multicolor RGB LEDs
- [Medium] Over-voltage input configuration for 3.3V load switch
- [Low] Device Tree schema violation in `microchip,spi-present-mask`
- [Low] Unnecessary address and size cells in `sdhci2` node
- [Low] Typo in gpio-line-names
--

commit 7f076ae76f17c0b92ffa0b772d551d80e949dbd9
Author: Judith Mendez <jm@ti.com>

arm64: dts: ti: Add k3-am62l3-beaglebadge

BeagleBoard.org BeagleBadge is a compact, affordable open source hardware
single board computer based on the Texas Instruments AM62L3 SoC. Expansion
is provided over open standards based headers including QWIIC and GPIO
interfaces.

Link: https://beagleboard.org
Link: https://github.com/beagleboard/BeagleBadge

> diff --git a/arch/arm64/boot/dts/ti/k3-am62l3-beaglebadge.dts b/arch/arm64/boot/dts/ti/k3-am62l3-beaglebadge.dts
> new file mode 100644
> index 0000000000000..2bbced7bc2c9d
> --- /dev/null
> +++ b/arch/arm64/boot/dts/ti/k3-am62l3-beaglebadge.dts

[ ... ]

> +	multicolor-led {
> +		compatible = "pwm-leds-multicolor";
> +
> +		multi-led {
> +			color = <LED_COLOR_ID_RGB>;
> +			max-brightness = <255>;
> +
> +			led-red {
> +				pwms = <&ecap2 0 255 0>;

Is 255 nanoseconds the correct PWM period here?

This translates to a frequency of ~3.9 MHz, which is far too fast for LED
brightness control and provides virtually no duty cycle resolution.

Was the max-brightness value (255) incorrectly copied into the PWM period cell?

[ ... ]

> +	vsys_out: regulator-0 {
> +		/* output of BQ24070 */
> +		compatible = "regulator-fixed";
> +		regulator-name = "VSYS_OUT";
> +		regulator-min-microvolt = <4400000>;
> +		regulator-max-microvolt = <4400000>;
> +		regulator-always-on;
> +		regulator-boot-on;
> +		bootph-all;
> +	};
> +
> +	vcc_3v3_main: regulator-1 {
> +		/* output of TPS62A06 */
> +		compatible = "regulator-fixed";
> +		regulator-name = "VCC_3V3_MAIN";
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +		vin-supply = <&vsys_out>;
> +		regulator-always-on;
> +		regulator-boot-on;
> +		bootph-all;
> +	};
> +
> +	vdd_3v3: regulator-2 {
> +		/* output of TPS22965 */
> +		compatible = "regulator-fixed";
> +		regulator-name = "VDD_3V3";
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +		vin-supply = <&vsys_out>;

Should this load switch be connected to &vcc_3v3_main instead of &vsys_out?

Since it is a load switch, its output voltage matches its input voltage.
Connecting its vin-supply to &vsys_out (4.4V) contradicts the node's
regulator-max-microvolt = <3300000> configuration, causing it to pass
4.4V to a 3.3V rail.

[ ... ]

> +	mcp23s18_reset_pins_default: mcp23s18-reset-default-pins {
> +		pinctrl-single,pins = <
> +			AM62LX_IOPAD(0x00a0, PIN_OUTPUT, 4) /* (H21) GPMC0_AD10.GPIO0_25 */

Does this pin need to be configured for muxmode 7?

The comments and the reset-gpios property of the mcp23s18 node expect this pin
to act as GPIO0_25. On AM62Lx SoCs, the GPIO function is located at muxmode 7.

With muxmode 4, the pad won't be connected to the GPIO controller, and the
GPIO expander will remain in reset.

[ ... ]

> +&gpio0 {
> +	gpio-line-names ="","","","","","","","","","",									/* 0-9 */
> +			 "","","","","","BOOST_5V_ENA","VDD_3V3_SD_ENA","","","",					/* 10-19 */
> +			 "","","","","","MCP23S18_RESET","BTN_SELECT","","","",						/* 20-29 */
> +			 "","BTN_LEFT","BTN_UP","","LORA_RESET","","","","","",						/* 30-39 */
> +			 "FUEL_GUAGE_BATLOW","LORA_RFSW","BTN_DOWN","USB_HUB_RST","MIKROBUS_INT","","","","","",	/* 40-49 */

This isn't a bug, but there's a typo (FUEL_GUAGE_BATLOW) in the line name.

[ ... ]

> +&sdhci2 {
> +	vmmc-supply = <&wlan_en>;
> +	bus-width = <4>;
> +	non-removable;
> +	cap-power-off-card;
> +	keep-power-in-suspend;
> +	#address-cells = <1>;
> +	#size-cells = <0>;

Are these #address-cells and #size-cells properties needed here?

Since the sdhci2 node contains no child nodes, this will trigger an
avoid_unnecessary_addr_size dtc warning.

[ ... ]

> +&spi1 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&spi1_pins_default>;
> +	status = "okay";
> +
> +	mcp23s18: gpio@0 {
> +		compatible = "microchip,mcp23s18";
> +		reg = <0>;
> +		spi-max-frequency = <10000000>;
> +		gpio-controller;
> +		#gpio-cells = <2>;
> +		reset-gpios = <&gpio0 25 GPIO_ACTIVE_LOW>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&mcp23s18_reset_pins_default>;
> +		microchip,spi-present-mask = <0x01>;

Should microchip,spi-present-mask be formatted as /bits/ 8 <0x01>?

The microchip,mcp23s08.yaml device tree binding schema requires this property
to be a uint8, so keeping it as a 32-bit cell will trigger a dt_binding_check
validation error.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260508230341.1891450-1-jm@ti.com?part=2