Re: [PATCH v3 2/4] dt-bindings: gpio: rockchip,gpio-bank: Add rockchip,grf property

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The newly added decoding for `DRV_TYPE_IO_LEVEL_2_BIT` in `rockchip_get_drive_perpin()` exposes an overly broad pre-existing override, causing it to return corrupted drive strength values for 8-bit pins on RV1103B.
--

--- Patch [2]: [PATCH v3 2/4] dt-bindings: gpio: rockchip,gpio-bank: Add rockchip,grf property ---
commit b8824e91fa66d4af5685a314e266ea1b47feaa8a
Author: Simon Glass <sjg@chromium.org>

pinctrl: rockchip: Decode drive strength in the get function

This commit moves the decoding logic for 2-bit and 8-bit drive-strength
values from rockchip_set_drive_perpin() to rockchip_get_drive_perpin()
where it can actually be used to report configuration properly.

> diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
> index 7e0fcd45fd265..08a46a04a8157 100644
> --- a/drivers/pinctrl/pinctrl-rockchip.c
> +++ b/drivers/pinctrl/pinctrl-rockchip.c
[ ... ]
> @@ -3267,6 +3267,25 @@ static int rockchip_get_drive_perpin(struct rockchip_pin_bank *bank,
>  	case DRV_TYPE_IO_1V8_ONLY:
>  		rmask_bits = RK3288_DRV_BITS_PER_PIN;
>  		break;
> +	case DRV_TYPE_IO_LEVEL_2_BIT:
> +		ret = regmap_read(regmap, reg, &data);
> +		if (ret)
> +			return ret;
> +		data >>= bit;
> +
> +		return data & 0x3;

[Severity: Medium]
This is a pre-existing issue, but this newly added decoding exposes an
overly broad override earlier in rockchip_get_drive_perpin():

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

Since that check does not restrict itself to a specific bank, such as
bank->bank_num == 2, does it erroneously force Bank 0 and Bank 1 pins to
be evaluated as DRV_TYPE_IO_LEVEL_2_BIT?

Before this patch, DRV_TYPE_IO_LEVEL_2_BIT fell through to the default
case and safely returned -EINVAL. With this case now wired up, it silently
intercepts these pins and returns a truncated 2-bit value for 8-bit
register fields. Can this result in corrupted drive strength readbacks
for those other banks?

> +	case DRV_TYPE_IO_LEVEL_8_BIT:
> +		ret = regmap_read(regmap, reg, &data);
> +		if (ret)
> +			return ret;
> +		data >>= bit;
> +		data &= (1 << 8) - 1;
> +
> +		ret = hweight8(data);
> +		if (ret > 0)
> +			return ret - 1;
> +		else
> +			return -EINVAL;
>  	default:
>  		dev_err(dev, "unsupported pinctrl drive type: %d\n", drv_type);
>  		return -EINVAL;

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