Re: [PATCH RFC v2 1/6] gpiolib: add gpiochip_add_pin_range_sparse() function

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

On Mon, Mar 17, 2025 at 04:37:59PM +0100, Thomas Richard wrote:
> Add gpiochip_add_pin_range_sparse() function to add a range for GPIO<->pin
> mapping, using a list of non consecutive pins.
> Previously, it was only possible to add range of consecutive pins using
> gpiochip_add_pin_range_sparse().
> 
> The struct pinctrl_gpio_range has a 'pins' member which allows to set a
> list of pins (which can be non consecutive).
> gpiochip_add_pin_range_sparse() is identical to gpiochip_add_pin_range(),
> except it set 'pins' member instead of 'pin_base' member.

...

> +static int __gpiochip_add_pin_range(struct gpio_chip *gc, const char *pinctl_name,
> +				    unsigned int gpio_offset, unsigned int pin_offset,
> +				    unsigned int const *pins, unsigned int npins)

I really do not like the __ naming here.
Can we rather create a better one? E.g., gpiochip_add_pin_range_with_pins().

...

> +/**
> + * gpiochip_add_pin_range_sparse() - add a range for GPIO <-> pin mapping
> + * @gc: the gpiochip to add the range for
> + * @pinctl_name: the dev_name() of the pin controller to map to
> + * @gpio_offset: the start offset in the current gpio_chip number space
> + * @pin_list: the list of pins to accumulate in this range
> + * @npins: the number of pins to accumulate in this range

> + * Calling this function directly from a DeviceTree-supported
> + * pinctrl driver is DEPRECATED. Please see Section 2.1 of
> + * Documentation/devicetree/bindings/gpio/gpio.txt on how to
> + * bind pinctrl and gpio drivers via the "gpio-ranges" property.

New API can't be deprecated. You probably want to say
"NOTE, this API is not supposed to be used on DeviceTree-supported platforms."
or something like that.

Also it's not clear which function should be used to clean up this. I would
clarify that: "When tearing down the driver don't forget to remove added ranges
with help of gpiochip_remove_pin_ranges()."

> + * Returns:
> + * 0 on success, or a negative errno on failure.
> + */
> +int gpiochip_add_pin_range_sparse(struct gpio_chip *gc, const char *pinctl_name,
> +				  unsigned int gpio_offset, unsigned int const *pins,
> +				  unsigned int npins)
> +{
> +	return __gpiochip_add_pin_range(gc, pinctl_name, gpio_offset, 0, pins,
> +					npins);
> +}
> +EXPORT_SYMBOL_GPL(gpiochip_add_pin_range_sparse);

To me the gpiochip_add_sparse_pin_range() name sounds better.

...

>  int gpiochip_add_pin_range(struct gpio_chip *gc, const char *pinctl_name,
>  			   unsigned int gpio_offset, unsigned int pin_offset,
>  			   unsigned int npins);
> +int gpiochip_add_pin_range_sparse(struct gpio_chip *gc, const char *pinctl_name,
> +				  unsigned int gpio_offset, unsigned int const *pins,
> +				  unsigned int npins);
>  int gpiochip_add_pingroup_range(struct gpio_chip *gc,
>  			struct pinctrl_dev *pctldev,
>  			unsigned int gpio_offset, const char *pin_group);

> +static inline int
> +gpiochip_add_pin_range_sparse(struct gpio_chip *gc, const char *pinctl_name,
> +			      unsigned int gpio_offset, unsigned int const *pins,
> +			      unsigned int npins)
> +{
> +	return 0;
> +}

Yeah, two stubs, two almost identical doc sections, no explanations of pins in
the core function...

I would rather refactor this to just rename the current function while adding
parameter to it, but leave it is being exported, just add a description to the
new parameter into the kernel doc. Make two new out of it as static inline:rs.

-- 
With Best Regards,
Andy Shevchenko