Re: [PATCH 09/27] gpio: rockchip: Add RK3572 GPIO support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] gpio: rockchip: inverted factor of 2 in rockchip_gpio_set_debounce()
--

commit 779e47c3ede057555631e4f8433e01c234d3e7e3
Author: Cunhao Lu <1579567540@qq.com>

gpio: rockchip: Add RK3572 GPIO support

This commit adds support for the rk3572 gpio controller (version 2.7),
including handling four parent interrupts per bank and querying firmware
for interrupt-group pin masks via smccc.

> diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
> index 77b239a9a601e..35894a68b5446 100644
> --- a/drivers/gpio/gpio-rockchip.c
> +++ b/drivers/gpio/gpio-rockchip.c

[ ... ]

> @@ -209,12 +212,27 @@ static int rockchip_gpio_set_debounce(struct gpio_chip *gc,
>  	if (bank->gpio_type == GPIO_TYPE_V2 && !IS_ERR(bank->db_clk)) {
>  		div_debounce_support = true;
>  		freq = clk_get_rate(bank->db_clk);
> -		max_debounce = (GENMASK(23, 0) + 1) * 2 * 1000000 / freq;
> -		if (debounce > max_debounce)
> -			return -EINVAL;
>  
> -		div = debounce * freq;
> -		div_reg = DIV_ROUND_CLOSEST_ULL(div, 2 * USEC_PER_SEC) - 1;
> +		if (of_device_is_compatible(bank->of_node, "rockchip,rk3572-gpio-bank")) {
> +			if (!freq)
> +				return -EINVAL;
> +
> +			div = (u64)(GENMASK(23, 0) + 1) * USEC_PER_SEC;
> +			max_debounce = div_u64(div, 2 * freq);
> +			if (debounce > max_debounce)
> +				return -EINVAL;
> +
> +			div = (u64)debounce * freq * 2;
> +			div_reg = DIV_ROUND_CLOSEST_ULL(div, USEC_PER_SEC);
> +			div_reg = div_reg ? div_reg - 1 : 0;
> +		} else {
> +			max_debounce = (GENMASK(23, 0) + 1) * 2 * 1000000 / freq;
> +			if (debounce > max_debounce)
> +				return -EINVAL;
> +
> +			div = debounce * freq;
> +			div_reg = DIV_ROUND_CLOSEST_ULL(div, 2 * USEC_PER_SEC) - 1;
> +		}

[Severity: Medium]
Is the factor of 2 inverted in the new "rockchip,rk3572-gpio-bank" branch of
rockchip_gpio_set_debounce()?

In the existing v2 debounce calculation in the else branch, each step of the
24-bit debounce clock divider divides db_clk by 2 * (div_reg + 1), giving a
period of 2 * (div_reg + 1) / freq seconds:

	max_debounce = (GENMASK(23, 0) + 1) * 2 * 1000000 / freq;
	...
	div = debounce * freq;
	div_reg = DIV_ROUND_CLOSEST_ULL(div, 2 * USEC_PER_SEC) - 1;

In the new branch for "rockchip,rk3572-gpio-bank", max_debounce divides by
2 * freq instead of multiplying by 2 (making max_debounce 4x smaller), and
div_reg multiplies debounce * freq by 2 instead of dividing by
2 * USEC_PER_SEC (making the programmed divider 4x larger, which can also
truncate the 24-bit divider for larger debounce values and implies a
half-cycle resolution of 1 / (2 * freq) at div_reg == 0). The commit message
also does not mention a different hardware debounce divider formula for this
variant.

Note that in the existing else branch, the reason div_reg underflows when
db_clk is slow (such as 32.768 kHz) is that rockchip_gpio_set_config() passes
true (1 us) as the debounce argument:

drivers/gpio/gpio-rockchip.c:rockchip_gpio_set_config() {
	switch (param) {
	case PIN_CONFIG_INPUT_DEBOUNCE:
		rockchip_gpio_set_debounce(gc, offset, true);

causing DIV_ROUND_CLOSEST_ULL(div, 2 * USEC_PER_SEC) to evaluate to 0 before
subtracting 1 (while (GENMASK(23, 0) + 1) * 2 * 1000000 also overflows a
32-bit unsigned long, and the register update below in
rockchip_gpio_set_debounce() uses raw readl() and writel() on
reg->dbclk_div_con instead of rockchip_gpio_readl() and
rockchip_gpio_writel()).

Should the 64-bit calculation and the div_reg ? div_reg - 1 : 0 clamp keep
the non-inverted 2 * (div_reg + 1) divider formula?

>  	} else {
>  		div_debounce_support = false;
>  	}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-add-board-ok3572-c-v1-0-0b0fd85a31ee@qq.com?part=9