Re: [PATCH v2] pinctrl: sunxi: Implement gpiochip::get_direction()

[Andrey Skvortsov <andrej.skvortzov@gmail.com>]

On 26-02-24 17:24, Chen-Yu Tsai wrote:
> After commit 471e998c0e31 ("gpiolib: remove redundant callback check"),
> a warning will be printed if the gpio driver does not implement this
> callback. The warning was added in commit e623c4303ed1 ("gpiolib:
> sanitize the return value of gpio_chip::get_direction()"), but was
> masked by the "redundant" check.
> 
> The warning can be triggered by any action that calls the callback,
> such as dumping the GPIO state from /sys/kernel/debug/gpio.
> 
> Implement it for the sunxi driver. This is simply a matter of reading
> out the mux value from the registers, then checking if it is one of
> the GPIO functions and which direction it is.
> 
> Signed-off-by: Chen-Yu Tsai <wens@kernel.org>
> Reviewed-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
> ---
> Please merge for fixes.
> 
> Changes since v1:
> - Corrected commit attribution for the warning
> - Add example scenario that triggers the warning
> ---
>  drivers/pinctrl/sunxi/pinctrl-sunxi.c | 51 +++++++++++++++++++++++++++
>  1 file changed, 51 insertions(+)
> 
> diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> index 0fb057a07dcc..27b2a3e9d78d 100644
> --- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> +++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> @@ -204,6 +204,32 @@ sunxi_pinctrl_desc_find_function_by_pin(struct sunxi_pinctrl *pctl,
>  	return NULL;
>  }
>  
> +static struct sunxi_desc_function *
> +sunxi_pinctrl_desc_find_function_by_pin_and_mux(struct sunxi_pinctrl *pctl,
> +						const u16 pin_num,
> +						const u8 muxval)
> +{
> +	for (unsigned int i = 0; i < pctl->desc->npins; i++) {
> +		const struct sunxi_desc_pin *pin = pctl->desc->pins + i;
> +		struct sunxi_desc_function *func = pin->functions;
> +
> +		if (pin->pin.number != pin_num)
> +			continue;
> +
> +		if (pin->variant && !(pctl->variant & pin->variant))
> +			continue;
> +
> +		while (func->name) {
> +			if (func->muxval == muxval)
> +				return func;
> +
> +			func++;
> +		}
> +	}
> +
> +	return NULL;
> +}
> +
>  static int sunxi_pctrl_get_groups_count(struct pinctrl_dev *pctldev)
>  {
>  	struct sunxi_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
> @@ -930,6 +956,30 @@ static const struct pinmux_ops sunxi_pmx_ops = {
>  	.strict			= true,
>  };
>  
> +static int sunxi_pinctrl_gpio_get_direction(struct gpio_chip *chip,
> +					    unsigned int offset)
> +{
> +	struct sunxi_pinctrl *pctl = gpiochip_get_data(chip);
> +	const struct sunxi_desc_function *func;
> +	u32 pin = offset + chip->base;
> +	u32 reg, shift, mask;
> +	u8 muxval;
> +
> +	sunxi_mux_reg(pctl, offset, &reg, &shift, &mask);
> +
> +	muxval = (readl(pctl->membase + reg) & mask) >> shift;
> +
> +	func = sunxi_pinctrl_desc_find_function_by_pin_and_mux(pctl, pin, muxval);
> +	if (!func)
> +		return -ENODEV;


This change introduces regression in linux-next. Several drivers on
Allwinner A64 fail to probe. For example,

 gpio gpiochip1: (1c20800.pinctrl): gpiochip_lock_as_irq: cannot get GPIO direction
 sun50i-a64-pinctrl 1c20800.pinctrl: unable to lock HW IRQ 69 for IRQ
 genirq: Failed to request resources for inv_mpu (irq 163) on irqchip sunxi_pio_edge

inv_mpu6050_i2c tries to get interrupt without initializing gpio before.

muxval in the case of uninitialized gpio is 0x7 (IO Disable) and
returned func value is NULL.


Some pinctrl return error for alternate configuration. For example,
pinctrl-stm32 returns -EINVAL [1].


But other drivers return one of non-error values.

pinctrl-axp209 defaults to GPIO_LINE_DIRECTION_OUT [2] 
        /*
	 * This shouldn't really happen if the pin is in use already,
	 * or if it's not in use yet, it doesn't matter since we're
	 * going to change the value soon anyway. Default to output.
	 */
	if ((val & AXP20X_GPIO_FUNCTIONS) > 2)
		return GPIO_LINE_DIRECTION_OUT;

pinctrl-upboard defaults to GPIO_LINE_DIRECTION_IN [3].
       /* If the pin is in function mode or high-z, input direction is
       returned */


pinctrl-rza2 defaults to GPIO_LINE_DIRECTION_IN as well [4].
        /*
	 * This GPIO controller has a default Hi-Z state that is not input or
	 * output, so force the pin to input now.
	 */


Should default be here GPIO_LINE_DIRECTION_IN here as well instead of -ENODEV?

> +	if (!strcmp(func->name, "gpio_out"))
> +		return GPIO_LINE_DIRECTION_OUT;
> +	if (!strcmp(func->name, "gpio_in") || !strcmp(func->name, "irq"))
> +		return GPIO_LINE_DIRECTION_IN;
> +	return -EINVAL;



Alternative solution would be converting handling of return value in
gpiochip_lock_as_irq to warning level. Since the value dir isn't
used. The call flushes the direction setting if something changed
behind our back. [5]

--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -4107,11 +4107,9 @@ int gpiochip_lock_as_irq(struct gpio_chip *gc, unsigned int offset)
        if (!gc->can_sleep && gc->get_direction) {
                int dir = gpiod_get_direction(desc);
 
-               if (dir < 0) {
-                       gpiochip_err(gc, "%s: cannot get GPIO direction\n",
+               if (dir < 0)
+                       gpiochip_warn(gc, "%s: cannot get GPIO direction\n",
                                     __func__);
-                       return dir;
-               }
        }


1. https://elixir.bootlin.com/linux/v7.0-rc1/source/drivers/pinctrl/stm32/pinctrl-stm32.c#L437
2. https://elixir.bootlin.com/linux/v7.0-rc1/source/drivers/pinctrl/pinctrl-axp209.c#L175
3. https://elixir.bootlin.com/linux/v7.0-rc1/source/drivers/pinctrl/pinctrl-upboard.c#L840
4. https://elixir.bootlin.com/linux/v7.0-rc1/source/drivers/pinctrl/renesas/pinctrl-rza2.c#L148
5. https://elixir.bootlin.com/linux/v7.0-rc1/source/drivers/gpio/gpiolib.c#L4084

-- 
Best regards,
Andrey Skvortsov