Re: [PATCH v2 2/2] arm64: dts: rockchip: add ALIENTEK QuarkPi-CA2

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] dt: missing enable-active-high property in vbus5v0_typec forces incorrect active-low polarity
- [Medium] dt: encoding Linux driver preference into compatible string instead of describing hardware
--

commit fc725bef630578d1f6fd74bd033f6824e7bb9013
Author: BG9OXA <bg9oxa@163.com>

arm64: dts: rockchip: add ALIENTEK QuarkPi-CA2

Add a device tree for the ALIENTEK QuarkPi-CA2, an RK3588S based
single board computer.

> diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-quarkpi-ca2.dts b/arch/arm64/boot/dts/rockchip/rk3588s-quarkpi-ca2.dts
> new file mode 100644
> index 0000000000000..f6bfce3680bf2
> --- /dev/null
> +++ b/arch/arm64/boot/dts/rockchip/rk3588s-quarkpi-ca2.dts

[ ... ]

> +	/*
> +	 * Type-C port VBUS 5V -- the vendor dtb has gpio = <&gpio1 0x1d 0>,
> +	 * i.e. gpio1 PD5 (enable-active-high; ACTIVE_HIGH is equivalent).
> +	 * Note: PC5 is i2s0_lrck (the audio LRCLK), so driving the regulator
> +	 *    from it takes the audio pin over and leaves the real PD5
> +	 *    undriven, which means VBUS is never switched on (self-powered
> +	 *    devices still work, so such a mistake is easy to miss).
> +	 */
> +	vbus5v0_typec: vbus5v0-typec {
> +		compatible = "regulator-fixed";
> +		regulator-name = "vbus5v0_typec";
> +		regulator-min-microvolt = <5000000>;
> +		regulator-max-microvolt = <5000000>;
> +		gpios = <&gpio1 RK_PD5 GPIO_ACTIVE_HIGH>;

[Severity: High]
Does this node need the enable-active-high property?

Because vbus5v0-typec is a regulator-fixed node, omitting enable-active-high
causes the kernel's of_gpio_quirk_polarity() in drivers/gpio/gpiolib-of.c to
ignore the GPIO_ACTIVE_HIGH flag. 

When the Type-C or USB host controller enables the regulator at runtime,
gpiolib will force the pin to a logical active-low polarity, driving the pin
to physical 0 (low) instead of high. This will prevent the Type-C VBUS power
from turning on, preventing connected peripherals from receiving power.

> +		vin-supply = <&vcc5v0_sys>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&typec5v_pwren>;
> +	};
> +};
> +
> +/*
> + * ===== ES8388 audio (ported from the vendor BSP) =====
> + *

[ ... ]

> +&i2c1 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&i2c1m4_xfer>;
> +	status = "okay";
> +
> +	es8388: audio-codec@11 {
> +		/* everest,es8323 must be listed first.
> +		 *   Reasons (driver source plus the module present on the board):
> +		 *   - Mainline es8328.c is the simplified driver (888 lines),
> +		 *     es8323.c is the complete one (791 lines, with set_dai_fmt /
> +		 *     set_dai_sysclk / pcm_hw_params and the coeff_div table).
> +		 *   - compatible lists are matched in order: with
> +		 *     'everest,es8388' first, es8328 claims the device, so es8323
> +		 *     has to come first for the complete driver to bind.
> +		 *   - The driver is built as a module (module alias
> +		 *     of:N*T*Ceverest,es8323), so it binds once the node exists.
> +		 *   - The upstream board with the same codec,
> +		 *     rk3588-youyeetoo-yy3588.dts, also lists only
> +		 *     es8323.
> +		 *   - The vendor 6.1 BSP dtb uses the same pair,
> +		 *     "everest,es8388","everest,es8323".
> +		 */
> +		compatible = "everest,es8323", "everest,es8388";

[Severity: Medium]
Is it appropriate to list everest,es8323 ahead of everest,es8388 for an ES8388
codec merely to trick Linux into binding the more fully-featured es8323 driver?

This violates device tree architectural principles by describing a software
preference rather than the actual hardware. Should the compatible string purely
describe the hardware, while any driver workarounds are handled in the kernel?

> +		reg = <0x11>;

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/179051708845.31632.10915026963421643453@163.com?part=2