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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Unmanaged Board Control GPIOs (Missing GPIO Hogs / Regulators) for `BOOST_5V_ENA` and `USB_HUB_RST`.
- [High] Driver/DTB Interface Violation: `microchip,spi-present-mask` length mismatch.
- [High] Missing `bootph-all` property on referenced pinctrl nodes will cause U-Boot SPL probe failures.
- [Medium] Misplaced Consumer Pinctrl Configurations on the `&gpio0` provider node.
- [Low] Deprecated `gpio` property used instead of `gpios` in `regulator-fixed` nodes.
--

commit 535e2cddc8d18ba0025f3f27f6e67f150156d239
Author: Judith Mendez <jm@ti.com>
arm64: dts: ti: Add k3-am62l3-beaglebadge

This patch introduces the device tree for the BeagleBoard.org BeagleBadge,
a compact single board computer based on the TI AM62L3 SoC. It provides
the initial hardware description including regulators, GPIOs, pinmux
configurations, and basic peripherals.

> 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..30fc9c83b1f44
> --- /dev/null
> +++ b/arch/arm64/boot/dts/ti/k3-am62l3-beaglebadge.dts

[ ... ]

> +	vdd_3v3_sd: regulator-3 {
> +		/* TPS22918DBVR */
> +		compatible = "regulator-fixed";
> +		regulator-name = "VDD_3V3_SD";
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +		vin-supply = <&vdd_3v3>;
> +		regulator-boot-on;
> +		enable-active-high;
> +		gpio = <&gpio0 16 GPIO_ACTIVE_HIGH>;

This isn't a bug, but could the gpios property be used here instead of the
deprecated gpio property? The wlan_en node later in this file correctly uses
the plural form.

> +		pinctrl-names = "default";
> +		pinctrl-0 = <&vdd_3v3_sd_ena_pins_default>;
> +		bootph-all;
> +	};
> +
> +	sensor_3v3: regulator-4 {
> +		/* TPS22918DBVR */
> +		compatible = "regulator-fixed";
> +		regulator-name = "Sensor_3V3";
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +		vin-supply = <&vdd_3v3>;
> +		regulator-boot-on;
> +		regulator-always-on;
> +		enable-active-high;
> +		gpio = <&wkup_gpio0 1 GPIO_ACTIVE_HIGH>;

Could this also be updated to use the gpios property?

[ ... ]

> +	usr_button_pins_default: usr-button-default-pins {
> +		pinctrl-single,pins = <
> +			AM62LX_IOPAD(0x00a4, PIN_INPUT, 7) /* (H18) GPMC0_AD11.GPIO0_26 */
> +			AM62LX_IOPAD(0x01e4, PIN_INPUT, 7) /* (D16) EXT_REFCLK1.GPIO0_104 */
> +			AM62LX_IOPAD(0x00c0, PIN_INPUT, 7) /* (N19) GPMC0_ADVn_ALE.GPIO0_32 */
> +			AM62LX_IOPAD(0x00e8, PIN_INPUT, 7) /* (L19) GPMC0_CSn1.GPIO0_42 */
> +			AM62LX_IOPAD(0x00b8, PIN_INPUT, 7) /* (L21) GPMC0_CLK.GPIO0_31 */
> +			AM62LX_IOPAD(0x01c0, PIN_INPUT, 7) /* (B13) UART0_RTSn.GPIO0_95 */
> +		>;
> +	};

Because the gpio0 node later in the file includes the bootph-all property and
references this node, does this node also need bootph-all? Without it,
U-Boot's fdtgrep might strip this node, leaving a dangling phandle and
causing probe failures in SPL.

[ ... ]

> +	sensor_3v3_ena_pins_default: sensor-3v3-ena-default-pins {
> +		pinctrl-single,pins = <
> +			AM62LX_IOPAD(0x004, PIN_OUTPUT, 7) /* (AA23) WKUP_UART0_TXD.WKUP_GPIO0_1 */
> +		>;
> +	};

Does this node also need the bootph-all property? The sensor_3v3 regulator
node uses this pinctrl configuration and has bootph-all.

[ ... ]

> +	lora_control_pins_default: lora-control-default-pins {
> +		pinctrl-single,pins = <
> +			AM62LX_IOPAD(0x00c8, PIN_OUTPUT, 7) /* (M19) GPMC0_WEn.GPIO0_34 */
> +			AM62LX_IOPAD(0x00e4, PIN_OUTPUT, 7) /* (L20) GPMC0_CSn0.GPIO0_41 */
> +			AM62LX_IOPAD(0x01a4, PIN_INPUT, 7) /* (D11) SPI0_CS1.GPIO0_88 */
> +			AM62LX_IOPAD(0x01bc, PIN_INPUT, 7) /* (B14) UART0_CTSn.GPIO0_94 */
> +		>;
> +	};

Same question here regarding the bootph-all property since this is referenced
by the gpio0 node.

> +
> +	gpio0_pins_default: gpio0-default-pins {
> +		pinctrl-single,pins = <
> +			AM62LX_IOPAD(0x0078, PIN_OUTPUT, 7) /* (L22) GPMC0_AD0.GPIO0_15 */
> +			AM62LX_IOPAD(0x00e0, PIN_INPUT, 7) /* (M21) GPMC0_DIR.GPIO0_40 */
> +			AM62LX_IOPAD(0x00ec, PIN_OUTPUT, 7) /* (M23) GPMC0_CSn2.GPIO0_43 */
> +			AM62LX_IOPAD(0x00f0, PIN_INPUT, 7) /* (M22) GPMC0_CSn3.GPIO0_44 */
> +			AM62LX_IOPAD(0x0194, PIN_OUTPUT, 7) /* (B11) MCASP0_AFSX.GPIO0_84 */
> +		>;
> +	};

Does this node also need bootph-all to match the gpio0 node?

Also, I noticed that GPIO0_15 (BOOST_5V_ENA) and GPIO0_43 (USB_HUB_RST)
are configured as PIN_OUTPUT here. Because configuring PIN_OUTPUT in pinctrl
only sets the pad multiplexing and does not actively drive a logic level on
the GPIO controller, will these pins remain floating?

Should there be a gpio-hog or consumer regulator node to manage the direction
and logic level for these control pins?

[ ... ]

> +&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_GAUGE_BATLOW","LORA_RFSW","BTN_DOWN","USB_HUB_RST","MIKROBUS_INT","","","","","",	/* 40-49 */
> +			 "","WLAN_EN","","","","","","","","",								/* 50-59 */
> +			 "","","","","","","","","","",									/* 60-69 */
> +			 "","","","","","","","","","",									/* 70-79 */
> +			 "","","","","MIKROBUS_RST","","","","LORA_BUSY","",						/* 80-89 */
> +			 "","","","","LORA_DIO","BTN_RIGHT","","","","",						/* 90-99 */
> +			 "","","","","BTN_BACK","","","","","",								/* 100-109 */
> +			 "","","","","","","","","","",									/* 110-119 */
> +			 "","","SD_CD","","","";									/* 120-125 */
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&gpio0_pins_default>, <&usr_button_pins_default>, <&lora_control_pins_default>;

Is it expected to place the pinctrl configurations for consumer devices
(usr_button_pins_default and lora_control_pins_default) on the gpio0
provider node?

This causes the pins to be claimed when the GPIO controller probes. Could
these be moved to the respective consumer device nodes instead?

[ ... ]

> +	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 = /bits/ 8 <0x01>;

Will this cause a probe failure in the pinctrl-mcp23s08_spi driver?

The driver uses device_property_read_u32() to parse the
microchip,spi-present-mask property. Because the property is defined here
as an 8-bit value, the property read API will see a length mismatch and
might return -EOVERFLOW.

Could this be specified as a standard 32-bit cell (i.e., <0x01>)?

> +	};
> +};

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