Re: [PATCH 3/3] arm64: dts: rockchip: Add devicetree for the Graperain G3568 v2

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] Incorrect `press-threshold-microvolt` values in `adc-keys` use boundary midpoints instead of nominal voltages, causing the MENU button to misregister as ESC.
- [Medium] The WiFi SDIO interface specifies `keep-power-in-suspend`, but its I/O voltage supply (`vcc_1v8`) is configured to turn off in suspend, breaking Wake-on-WLAN.
- [Medium] The AP6356S Bluetooth UART is missing hardware flow control properties and the required Bluetooth child node.
- [Low] The deprecated `gpio` property is used instead of `gpios` in fixed regulator nodes.
--

commit 13181564cb78799e1d57eb0995588d8e839f22a5
Author: Coia Prant <coiaprant@gmail.com>
arm64: dts: rockchip: Add devicetree for the Graperain G3568 v2

This patch introduces the device tree for the Graperain G3568 v2, an RK3568
based development board. It configures the hardware peripherals including
ethernet, storage, wireless, and power regulators.

Link: https://image.chukouplus.com/upload/C_153/product_file/20211022/6daddec9e400458816dd4c57ba807fc3.pdf
Link: https://blog.gov.cooking/archives/research-graperain-g3568-v2-and-flash.html

> diff --git a/arch/arm64/boot/dts/rockchip/rk3568-graperain-g3568-v2.dts b/arch/arm64/boot/dts/rockchip/rk3568-graperain-g3568-v2.dts
> new file mode 100644
> index 0000000000000..221992d6c6003
> --- /dev/null
> +++ b/arch/arm64/boot/dts/rockchip/rk3568-graperain-g3568-v2.dts
> @@ -0,0 +1,894 @@

[ ... ]

> +	adc-keys {
> +		compatible = "adc-keys";
> +		io-channels = <&saradc 0>;
> +		io-channel-names = "buttons";
> +		keyup-threshold-microvolt = <1800000>;
> +		poll-interval = <100>;
> +
> +		button-esc {
> +			label = "esc";
> +			linux,code = <KEY_ESC>;
> +			press-threshold-microvolt = <1250000>;
> +		};
> +
> +		button-menu {
> +			label = "menu";
> +			linux,code = <KEY_MENU>;
> +			press-threshold-microvolt = <1600000>;

[Severity: Medium]
Will this cause the menu button to misregister as esc?

The adc-keys driver uses a nearest-neighbor algorithm to match the ADC
reading to the button. Since the commit message indicates the nominal
voltages are 1.1V for ESC and 1.4V for MENU, an ADC reading of 1.4V from
pressing MENU is closer to the ESC threshold of 1.25V (0.15V difference)
than the MENU threshold of 1.6V (0.2V difference).

Could we use the exact expected nominal voltages here instead of the
boundary midpoints?

[ ... ]

> +	vcc5v0_usb_host: regulator-vcc5v0-usb-host {
> +		compatible = "regulator-fixed";
> +		enable-active-high;
> +		gpio = <&gpio0 RK_PA6 GPIO_ACTIVE_HIGH>;

[Severity: Low]
Is this using the deprecated gpio property? 

The standard regulator-fixed binding expects gpios instead of gpio, which
can cause device tree schema validation warnings.

> +		pinctrl-names = "default";
> +		pinctrl-0 = <&vcc5v0_usb_host_en>;
> +		regulator-name = "vcc5v0_usb_host";
> +		regulator-min-microvolt = <5000000>;
> +		regulator-max-microvolt = <5000000>;
> +		vin-supply = <&vcc5v0_usb>;
> +	};
> +
> +	vcc5v0_usb_otg: regulator-vcc5v0-usb-otg {
> +		compatible = "regulator-fixed";
> +		enable-active-high;
> +		gpio = <&gpio0 RK_PA5 GPIO_ACTIVE_HIGH>;

[Severity: Low]
Would it be better to use the gpios property here as well to match the
regulator-fixed device tree binding schema?

[ ... ]

> +/* used for AP6356S WiFi */
> +&sdmmc2 {
> +	bus-width = <4>;
> +	cap-sd-highspeed;
> +	cap-sdio-irq;
> +	keep-power-in-suspend;
> +	mmc-pwrseq = <&sdio_pwrseq>;
> +	non-removable;
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&sdmmc2m0_bus4 &sdmmc2m0_cmd &sdmmc2m0_clk>;
> +	sd-uhs-sdr104;
> +	vmmc-supply = <&vcc3v3_sys>;
> +	vqmmc-supply = <&vcc_1v8>;

[Severity: Medium]
Will this configuration break Wake-on-WLAN and SDIO communication on resume?

This node specifies keep-power-in-suspend, but the vqmmc-supply is set to
vcc_1v8. Looking at the vcc_1v8 regulator definition (DCDC_REG5) earlier in
the file, it is configured with regulator-off-in-suspend.

Cutting power to the SDIO I/O domain during suspend violates the power
sequencing for keep-power-in-suspend and prevents out-of-band wake interrupts
from signaling the host.

[ ... ]

> +/* used for WiFi/BT AP6356S */
> +&uart8 {
> +	pinctrl-0 = <&uart8m0_xfer &uart8m0_ctsn>;
> +	status = "okay";
> +};

[Severity: Medium]
Does this Bluetooth UART configuration need hardware flow control and a
device child node?

High-speed Bluetooth modules like the AP6356S generally rely on RTS/CTS to
prevent buffer overflows, but the uart-has-rtscts property and the
uart8m0_rtsn pin configuration are missing here.

Additionally, without a bluetooth child node (such as compatible =
"brcm,bcm4345c5"), the kernel's HCI UART driver might not be able to probe
and initialize the module automatically.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260627225755.1710837-2-coiaprant@gmail.com?part=3