Re: [PATCH V2 6/6] arm64: dts: rockchip: Add Anbernic RG Vita-Pro

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 12 potential issue(s) to consider:
- [High] Missing DTB source files for unrelated Makefile additions.
- [High] Undefined `&pmic_pins` phandle causes compilation failure.
- [High] PCIe reset GPIO polarity is incorrectly configured as active-high.
- [High] Bluetooth GPIO pins are mismatched with their pinctrl definitions.
- [High] Hardware description contradicts the commit message regarding the Bluetooth module.
- [High] Battery charge current is dangerously high and capacity contradicts the commit message.
- [Medium] SPI bus lacks a child device node, breaking userspace joystick control.
- [Medium] Incorrect `pinctrl` assignment for the `&sdio` controller.
- [Medium] Contradictory regulator states defined for `vdd_cpu_lit_s0` and `vdd_gpu_s0`.
- [Medium] Misplaced `vbus-supply` property in the Type-C Port Controller node.
- [Medium] LCD panel supply regulator is incorrectly marked as always-on.
- [Low] Undocumented enablement of HDMI components.
--

commit f6eae2256befc6eb3871393bb039d8a06e21e7b0
Author: Chris Morgan <macromorgan@hotmail.com>

arm64: dts: rockchip: Add Anbernic RG Vita-Pro

This commit adds the device tree for the Anbernic RG Vita-Pro, based on the
Rockchip RK3576. It includes support for the display, buttons, joysticks,
LEDs, eMMC, SDMMC, WiFi/Bluetooth, and battery.

>     The Anbernic RG Vita-Pro includes the following hardware:
[ ... ]
>      - RTL8852BE WiFi/Bluetooth combo

The commit message claims the device uses an RTL8852BE Wi-Fi/Bluetooth combo,
which interfaces Bluetooth over USB. However, the device tree defines a
realtek,rtl8822cs-bt compatible string under a UART interface (&uart6).

Does this hardware mismatch cause the probe to fail?

>      - 5000mAH battery

The commit message specifies a 5000mAh battery, but the device tree specifies
a charge-full-design-microamp-hours of 8000000 (8000mAh).

Additionally, the commit message completely omits HDMI from its detailed list
of hardware, but the device tree enables HDMI components (hdmi, hdptxphy,
and hdmi-con). Is the commit message missing this hardware description, or
is this leftover code?

> diff --git a/arch/arm64/boot/dts/rockchip/Makefile b/arch/arm64/boot/dts/rockchip/Makefile
> index cb55c6b70d0e5..78c3778eaf4e7 100644
> --- a/arch/arm64/boot/dts/rockchip/Makefile
> +++ b/arch/arm64/boot/dts/rockchip/Makefile
[ ... ]
> @@ -215,6 +216,8 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588s-coolpi-4b.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588s-evb1-v10.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588s-gameforce-ace.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588s-indiedroid-nova.dtb
> +dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588s-indiedroid-nova-tower.dtb
> +dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588s-indiedroid-nova-test.dtb

The .dts source files for these two additions are missing from the patch.
Will this cause the kernel build to fail when make dtbs is executed?

> diff --git a/arch/arm64/boot/dts/rockchip/rk3576-anbernic-rg-vita-pro.dts b/arch/arm64/boot/dts/rockchip/rk3576-anbernic-rg-vita-pro.dts
> new file mode 100644
> index 0000000000000..e0a90719bdcaf
> --- /dev/null
> +++ b/arch/arm64/boot/dts/rockchip/rk3576-anbernic-rg-vita-pro.dts
[ ... ]
> +	battery: battery {
> +		compatible = "simple-battery";
> +		charge-full-design-microamp-hours = <8000000>;
> +		charge-term-current-microamp = <300000>;
> +		constant-charge-current-max-microamp = <10000000>;

The constant-charge-current-max-microamp is set to 10 Amps (10000000). Does
this exceed the physical limits of the hardware and pose a safety risk for a
handheld single-cell battery?

[ ... ]
> +	vdd_lcd: regulator-vdd-lcd {
> +		compatible = "regulator-fixed";
> +		regulator-name = "vdd_lcd";
> +		regulator-always-on;

By marking this regulator as always-on, the panel's power rail cannot be shut
down by the DRM/panel driver framework during screen blanking or system
suspend. Does this result in continuous unnecessary battery drain?

[ ... ]
> +	pmic@23 {
> +		compatible = "rockchip,rk806";
> +		reg = <0x23>;
> +		#gpio-cells = <2>;
> +		gpio-controller;
> +		interrupt-parent = <&gpio0>;
> +		interrupts = <RK_PA6 IRQ_TYPE_LEVEL_LOW>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pmic_pins
> +			     &rk806_dvs1_null

This pinctrl-0 property references &pmic_pins, but this node does not appear
to be defined in the .dts file. Will this missing definition cause a fatal
device tree compilation error?

[ ... ]
> +			vdd_cpu_lit_s0: dcdc-reg3 {
> +				regulator-always-on;
> +				regulator-boot-on;
> +				regulator-min-microvolt = <550000>;
> +				regulator-max-microvolt = <950000>;
> +				regulator-name = "vdd_cpu_lit_s0";
> +				regulator-ramp-delay = <12500>;
> +				regulator-state-mem {
> +					regulator-off-in-suspend;
> +					regulator-suspend-microvolt = <750000>;
> +				};
> +			};

Is it semantically valid to request a specific voltage level
(regulator-suspend-microvolt) during suspend for a regulator that is explicitly
commanded to shut off (regulator-off-in-suspend)? Can this lead to undefined
behavior in the PMIC framework?

[ ... ]
> +			vdd_gpu_s0: dcdc-reg5 {
> +				regulator-boot-on;
> +				regulator-enable-ramp-delay = <400>;
> +				regulator-min-microvolt = <550000>;
> +				regulator-max-microvolt = <900000>;
> +				regulator-name = "vdd_gpu_s0";
> +				regulator-ramp-delay = <12500>;
> +				regulator-state-mem {
> +					regulator-off-in-suspend;
> +					regulator-suspend-microvolt = <850000>;
> +				};
> +			};

Similarly, could this combination of regulator-off-in-suspend and
regulator-suspend-microvolt for the GPU regulator cause issues during suspend?

[ ... ]
> +	husb311: typec-portc@4e {
> +		compatible = "hynetek,husb311", "richtek,rt1711h";
> +		reg = <0x4e>;
> +		interrupt-parent = <&gpio0>;
> +		interrupts = <RK_PD1 IRQ_TYPE_LEVEL_LOW>;
> +		pinctrl-0 = <&usbc_int>;
> +		pinctrl-names = "default";
> +		status = "okay";
> +		vbus-supply = <&vbus5v0_typec>;
> +
> +		connector {

According to standard USB Type-C bindings, shouldn't the vbus-supply property
reside inside the connector child node? Will the TCPM framework fail to find
the VBUS regulator if it is placed in the root of the husb311 node, preventing
the device from supplying power in OTG mode?

[ ... ]
> +&pcie0 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pcie0_perstn>;
> +	reset-gpios = <&gpio2 RK_PB4 GPIO_ACTIVE_HIGH>;

The PCIe specification dictates that the PERST# reset signal is active-low,
which is supported by the pcie0_perstn pin name. By configuring it as
active-high, will the PCIe controller invert the reset logic and permanently
hold the Wi-Fi card in reset, causing link training to fail?

[ ... ]
> +&sdio {
> +	bus-width = <4>;
> +	cap-mmc-highspeed;
> +	cap-sd-highspeed;
> +	disable-wp;
> +	max-frequency = <150000000>;
> +	no-mmc;
> +	no-sdio;
> +	pinctrl-0 = <&sdmmc1m0_bus4>, <&sdmmc1m0_clk>,
> +		    <&sdmmc1m0_cmd>, <&sdmmc1m0_det>;

The &sdio node configures its pins using sdmmc1m0 properties. Since the SDIO
controller is a distinct IP block from SDMMC1, does routing the SDIO block
through pins intended for another controller disconnect it from the physical
SD card slot?

[ ... ]
> +&spi1 {
> +	num-cs = <1>;
> +	pinctrl-0 = <&spi1m2_pins>, <&spi1m2_csn0>;
> +	status = "okay";
> +
> +	/* SPI controlled MCU for joystick and joystick LEDs. */
> +};

The &spi1 bus is enabled for userspace-controlled analog joysticks, but no
child device node (such as spidev or a dedicated driver compatible) is defined.
Since the Linux SPI core will not instantiate any devices on an empty bus, does
this completely prevent userspace from communicating with the microcontroller?

[ ... ]
> +&uart6 {
> +	pinctrl-0 = <&uart6m1_xfer>, <&uart6m1_ctsn>, <&uart6m1_rtsn>;
> +	pinctrl-names = "default";
> +	uart-has-rtscts;
> +	status = "okay";
> +
> +	bluetooth {
> +		compatible = "realtek,rtl8822cs-bt";
> +		device-wake-gpios = <&gpio2 RK_PB6 GPIO_ACTIVE_HIGH>;
> +		enable-gpios = <&gpio2 RK_PB5 GPIO_ACTIVE_HIGH>;
> +		host-wake-gpios = <&gpio2 RK_PC1 GPIO_ACTIVE_HIGH>;

The enable-gpios is assigned to RK_PB5 and device-wake-gpios to RK_PB6.
However, the corresponding pinctrl configurations map RK_PB5 to bt_wake_gpio
and RK_PB6 to bt_reset_gpio.

Does this logical swap mean the driver will toggle the wake line when it
intends to reset the device, causing Bluetooth initialization to fail?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260515221947.299229-1-macroalpha82@gmail.com?part=6