Re: [PATCH 8/9] riscv: dts: ultrarisc: add Milk-V Titan board device tree

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The GPIO pins used for board-level multiplexers (`PA5`, `PA6`, `PA7`) are configured with the `gpio` pinmux function but are not driven to any defined state.
- [Low] The `gpio-poweroff` and `gpio-restart` nodes are explicitly defined in the board device tree but are left with `status = "disabled";`.
--

commit f0b973e0e230934a238fa1f5d39b91d61efc3d7b
Author: Jia Wang <wangjia@ultrarisc.com>

riscv: dts: ultrarisc: add Milk-V Titan board device tree

This commit adds the device tree for the Milk-V Titan motherboard based on
the dp1000 soc.

> diff --git a/arch/riscv/boot/dts/ultrarisc/dp1000-milkv-titan-pinctrl.dtsi b/arch/riscv/boot/dts/ultrarisc/dp1000-milkv-titan-pinctrl.dtsi
> index 0000000000000..053206190ec75
> --- /dev/null
> +++ b/arch/riscv/boot/dts/ultrarisc/dp1000-milkv-titan-pinctrl.dtsi
[ ... ]
> +	mux_dcdc_pins: mux-dcdc-pins {
> +		pins = "PA5";
> +		function = "gpio";
> +	};
> +
> +	mux_i2c3_pins: mux-i2c3-pins {
> +		pins = "PA6";
> +		function = "gpio";
> +	};
> +
> +	mux_uart0_pins: mux-uart0-pins {
> +		pins = "PA7";
> +		function = "gpio";
> +	};

[ ... ]

> diff --git a/arch/riscv/boot/dts/ultrarisc/dp1000-milkv-titan.dts b/arch/riscv/boot/dts/ultrarisc/dp1000-milkv-titan.dts
> index 0000000000000..21d85c03abe12
> --- /dev/null
> +++ b/arch/riscv/boot/dts/ultrarisc/dp1000-milkv-titan.dts
[ ... ]
> +	gpio-poweroff {
> +		compatible = "gpio-poweroff";
> +		gpios = <&gpio_b 0 GPIO_ACTIVE_LOW>;
> +		active-delay-ms = <100>;
> +
> +		status = "disabled";
> +	};
> +
> +	gpio-restart {
> +		compatible = "gpio-restart";
> +		gpios = <&gpio_b 1 GPIO_ACTIVE_LOW>;
> +		active-delay = <100>;
> +
> +		status = "disabled";
> +	};

Is it intended to leave these nodes disabled in a final board device tree?

Defining board-level features only to disable them might leave functionality
unexpectedly unavailable or litter the tree with dead code. If the hardware
supports these features, should they be enabled to provide power-off and
restart functionality?

[ ... ]
> +&gpio {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&io_pins &mux_dcdc_pins &mux_i2c3_pins &mux_uart0_pins>;
> +};

Does this configuration leave the multiplexer pins in a floating state?

The mux_dcdc_pins, mux_i2c3_pins, and mux_uart0_pins configure the PA5, PA6,
and PA7 pins to act as gpios and are applied to the default state of the gpio
controller here. However, configuring a pin to gpio mode only routes it to
the gpio controller without configuring the direction or output level.

Without a driver or gpio-hog nodes explicitly setting these pins as outputs
and driving them high or low, they will default to inputs. This might lead to
unpredictable hardware behavior, potentially disabling the dcdc converter,
i2c bus, or the system console. Could this be addressed by adding gpio-hog
configurations?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260515-ultrarisc-pinctrl-v1-0-bf559589ea8a@ultrarisc.com?part=8