Re: [PATCH v3 1/2] gpio: mmio: Add flag for calling pinctrl back-end

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

Wed, Feb 19, 2025 at 10:04:33PM +0100, Linus Walleij kirjoitti:
> It turns out that with this flag we can switch over an entire
> driver to use gpio-mmio instead of a bunch of custom code,
> also providing get/set_multiple() to it in the process, so it
> seems like a reasonable feature to add.
> 
> The generic pin control backend requires us to call the
> gpiochip_generic_request(), gpiochip_generic_free(),
> pinctrl_gpio_direction_output() and pinctrl_gpio_direction_input()
> callbacks, so if the new flag for a pin control back-end
> is set, we make sure these functions get called as
> expected.

First of all, I like the series and esp. the second patch, thanks!
One small comment below, though.

...

>  static int bgpio_request(struct gpio_chip *chip, unsigned gpio_pin)
>  {
> -	if (gpio_pin < chip->ngpio)
> -		return 0;
> +	if (gpio_pin >= chip->ngpio)
> +		return -EINVAL;
>  
> -	return -EINVAL;
> +	if (chip->bgpio_pinctrl)
> +		return gpiochip_generic_request(chip, gpio_pin);
> +
> +	return 0;
>  }

While I understand the desire to avoid +LoCs, I still think it's better from
maintenance p.o.v. to have a symmetry in APIs, i.e. providing

bgpio_free()  // or whatever name suits with possibility to change the above
{
	if (chip->bgpio_pinctrl)
		gpiochip_generic_free(...);
}

...

> +	if (flags & BGPIOF_PINCTRL_BACKEND) {
> +		gc->bgpio_pinctrl = true;
> +		/* Currently this callback is only used for pincontrol */
> +		gc->free = gpiochip_generic_free;
> +	}

And

	gc->free = gpiochip_generic_free;
	...
	if (flags & BGPIOF_PINCTRL_BACKEND)
		gc->bgpio_pinctrl = true;

here.

The rationale that if we ever add something to the request part, we won't
forget to call it in the free part.

-- 
With Best Regards,
Andy Shevchenko