Re: [PATCH v3 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] The Broadcom SDIO Wi-Fi node uses an undocumented compatible string without the required generic fallback, breaking driver OF parsing.
- [Medium] The adc-keys press-threshold-microvolt values are calculated as midpoints rather than target voltages, causing the MENU button to be incorrectly detected as the ESC button.
- [Medium] Incomplete Bluetooth I2S audio configuration: missing pinctrl and sound card binding.
- [Low] Copy-paste error in bluetooth pinctrl node names indicating the wrong GPIO bank and pin numbers.
--

commit 808ebde69e3acf8280f05f45c66fcb212e1918cd
Author: Coia Prant <coiaprant@gmail.com>

arm64: dts: rockchip: Add devicetree for the Graperain G3568 v2

The Graperain G3568 v2 is an RK3568-based development board. This patch
introduces the device tree file to support its hardware features, including
Ethernet, wireless connectivity, and physical buttons.

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
> --- /dev/null
> +++ b/arch/arm64/boot/dts/rockchip/rk3568-graperain-g3568-v2.dts
> @@ -0,0 +1,931 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)

[ ... ]

> +	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 configuration correctly identify the MENU button? The commit message
notes the hardware voltage for MENU is 1400000 microvolts.

Because the adc-keys driver matches based on the smallest absolute difference,
a 1400000 microvolt reading is closer to the ESC threshold
(1400000 - 1250000 = 150000) than the MENU threshold
(1600000 - 1400000 = 200000).

Could this cause pressing the MENU button to register as an ESC key press
instead?

[ ... ]

> +/* used for AP6356S Bluetooth Sound */
> +&i2s3_2ch {
> +	status = "okay";
> +};

[Severity: Medium]
Does this node require pinctrl and a simple-audio-card binding to function?
It appears the I2S interface is enabled, but without routing the signals to
the external AP6356S chip pins and linking the I2S CPU DAI to the Bluetooth
codec DAI, ALSA might not register the sound card.

[ ... ]

> +&pinctrl {
> +	bluetooth {
> +		bt_reg_on_h_gpio3_a0: bt-reg-on-h-gpio4-b2 {
> +			rockchip,pins = <3 RK_PA0 RK_FUNC_GPIO &pcfg_pull_none>;
> +		};
> +
> +		bt_wake_host_h_gpio3_a1: bt-wake-host-h-gpio4-b4 {
> +			rockchip,pins = <3 RK_PA1 RK_FUNC_GPIO &pcfg_pull_down>;
> +		};
> +
> +		host_wake_bt_h_gpio3_a2: host-wake-bt-h-gpio4-b5 {
> +			rockchip,pins = <3 RK_PA2 RK_FUNC_GPIO &pcfg_pull_none>;
> +		};
> +	};

[Severity: Low]
This isn't a functional bug, but do the node names here have a copy-paste
error? They indicate gpio4-b2, gpio4-b4, and gpio4-b5, but the contents
configure pins on GPIO bank 3 (RK_PA0, RK_PA1, RK_PA2).

[ ... ]

> +/* 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>;
> +	status = "okay";
> +
> +	wifi@1 {
> +		compatible = "brcm,bcm4356-fmac";
> +		reg = <1>;
> +	};
> +};

[Severity: Medium]
Is the generic fallback compatible string missing here? The brcmfmac driver
explicitly checks for brcm,bcm4329-fmac during OF probing. Without this
fallback, the driver's OF parser aborts and properties like a
bootloader-injected local-mac-address might be ignored.

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