Re: [PATCH v3 3/4] dt-bindings: pinctrl: rockchip: Add RV1106 compatible

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing pinctrl driver match support for 'rockchip,rv1106-pinctrl'
- [High] Silent truncation and unreachable dead code in rockchip_get_drive_perpin()
- [Medium] Unused 'rockchip,grf' property in 'gpio-bank' child nodes
--

--- Patch [3]: [PATCH v3 3/4] dt-bindings: pinctrl: rockchip: Add RV1106 compatible ---
commit f508d7fe3fdaea1487deaf572cd3b667691010ce
Author: Simon Glass <sjg@chromium.org>

dt-bindings: pinctrl: rockchip: Add RV1106 compatible

Add the compatible for the pin controller of the Rockchip RV1106 and
its RV1103 package variant.

> diff --git a/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.yaml
> index 9b3cbeb54fed4..81747bb530569 100644
> --- a/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.yaml
> +++ b/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.yaml
> @@ -51,6 +51,7 @@ properties:
>        - rockchip,rk3576-pinctrl
>        - rockchip,rk3588-pinctrl
>        - rockchip,rv1103b-pinctrl
> +      - rockchip,rv1106-pinctrl

[Severity: High]
Does this compatible string need to be added to the driver match table as well?

The schema strictly enforces 'rockchip,rv1106-pinctrl' without an allowable
fallback compatible, but it appears to be missing from the driver:

drivers/pinctrl/pinctrl-rockchip.c:rockchip_pinctrl_dt_match[] {
	static const struct of_device_id rockchip_pinctrl_dt_match[] = {
		{ .compatible = "rockchip,px30-pinctrl",
    ...
}

Will the pinctrl driver silently fail to probe for any device tree using this
new compatible string?

[Severity: Medium]
Will this new hardware function correctly without parsing the rockchip,grf
property in gpio-bank child nodes?

The property appears to be correctly documented for gpio-bank subnodes:

Documentation/devicetree/bindings/gpio/rockchip,gpio-bank.yaml {
    ...
      rockchip,grf:
        $ref: /schemas/types.yaml#/definitions/phandle
    ...
}

But looking at the driver, it only parses rockchip,grf from the parent
pinctrl node and seems to ignore the subnodes entirely:

drivers/pinctrl/pinctrl-rockchip.c:rockchip_pinctrl_probe() {
    ...
	node = of_parse_phandle(np, "rockchip,grf", 0);
    ...
}

Could this cause the hardware feature for per-bank IOC control to be
non-functional?

[Severity: High]
Can the RV1103B override logic lead to silent truncation and unreachable code?

Looking at rockchip_get_drive_perpin(), an override forces
DRV_TYPE_IO_LEVEL_2_BIT:

drivers/pinctrl/pinctrl-rockchip.c:rockchip_get_drive_perpin() {
    ...
	if (ctrl->type == RV1103B && pin_num >= 12)
		drv_type = DRV_TYPE_IO_LEVEL_2_BIT;
    ...
}

This hits the 2-bit case block, which introduces an early return and performs a
2-bit truncation for Banks 0 and 1:

drivers/pinctrl/pinctrl-rockchip.c:rockchip_get_drive_perpin() {
    ...
	case DRV_TYPE_IO_LEVEL_2_BIT:
		ret = regmap_read(regmap, reg, &data);
		if (ret)
			return ret;
		data >>= bit;

		return data & 0x3;
    ...
}

Because of this early return, does the fallback logic for RV1103B at the
bottom of the function become structurally dead?

drivers/pinctrl/pinctrl-rockchip.c:rockchip_get_drive_perpin() {
    ...
	if (ctrl->type == RV1103B && bank->bank_num == 2 && pin_num >= 12) {
		data = data >> 10;
		return data & 0x3;
	}
    ...
}

Since the override lacks a bank number check, won't 8-bit drive strength pins
on Banks 0 and 1 with pin_num >= 12 now incorrectly hit the new 2-bit case
block and be silently bitwise-truncated to a 2-bit value?

>        - rockchip,rv1108-pinctrl
>        - rockchip,rv1126-pinctrl

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729132736.3807082-1-sjg@chromium.org?part=3