Re: [PATCH v7 12/34] pinctrl: airoha: add missed get_direction() function for gpio_chip

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The newly added `airoha_gpio_get_direction` strictly checks a 2-bit direction field, but the existing `airoha_pinmux_set_direction` only updates a 1-bit mask, causing `get_direction` to return `-EINVAL`.
--

--- Patch [12]: [PATCH v7 12/34] pinctrl: airoha: add missed get_direction() function for gpio_chip ---
commit d5d33bb49ecef808432dc746e944b058a238f703
Author: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>

pinctrl: airoha: add missed get_direction() function for gpio_chip

This patch adds a missing get_direction function for the airoha gpio_chip.
It also updates pinconf to use this new function.

> diff --git a/drivers/pinctrl/airoha/pinctrl-airoha.c b/drivers/pinctrl/airoha/pinctrl-airoha.c
> index 4dae8c654f995..6a0ba54053fe7 100644
> --- a/drivers/pinctrl/airoha/pinctrl-airoha.c
> +++ b/drivers/pinctrl/airoha/pinctrl-airoha.c
[ ... ]
> @@ -2502,6 +2502,27 @@ static int airoha_gpio_get(struct gpio_chip *chip, unsigned int gpio)
>  	return err ? err : !!(val & BIT(pin));
>  }
>  
> +static int airoha_gpio_get_direction(struct gpio_chip *chip, unsigned int gpio)
> +{
> +	struct airoha_pinctrl *pinctrl = gpiochip_get_data(chip);
> +	u32 mask, index, val;
> +	int err, field_shift;
> +
> +	field_shift = 2 * (gpio % AIROHA_REG_GPIOCTRL_NUM_PIN);
> +	mask = GENMASK(field_shift + 1, field_shift);
> +	index = gpio / AIROHA_REG_GPIOCTRL_NUM_PIN;
> +
> +	err = regmap_read(pinctrl->regmap,
> +			  pinctrl->gpiochip.dir[index], &val);
> +	if (err)
> +		return err;
> +
> +	if ((val & mask) > BIT(field_shift))
> +		return -EINVAL;

[Severity: High]
Will this strict 2-bit check cause get_direction to permanently fail for pins 
configured with the upper bit set by the bootloader or hardware?

Looking at airoha_pinmux_set_direction() in the same file, it only uses a
1-bit mask when setting the direction:

drivers/pinctrl/airoha/pinctrl-airoha.c:airoha_pinmux_set_direction() {
    ...
    /* set direction */
    mask = BIT(2 * (pin % AIROHA_REG_GPIOCTRL_NUM_PIN));
    index = pin / AIROHA_REG_GPIOCTRL_NUM_PIN;
    return regmap_update_bits(pinctrl->regmap,
                              pinctrl->gpiochip.dir[index], mask,
                              !input ? mask : 0);
}

If the hardware or bootloader initially sets the upper direction bit for a
pin, setting the direction via airoha_pinmux_set_direction() will only
update the lower bit. When airoha_gpio_get_direction() is called later,
the upper bit will still be set. The expression (val & mask) would then be
greater than BIT(field_shift), causing it to return -EINVAL unconditionally.

Does airoha_pinmux_set_direction() need to be updated to clear the upper
bit, or should this newly added get_direction logic be more lenient?

> +
> +	return (val & mask) ? GPIO_LINE_DIRECTION_OUT : GPIO_LINE_DIRECTION_IN;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727074234.3761170-1-mikhail.kshevetskiy@iopsys.eu?part=12