diff mbox series

[3/5] gpiolib: add gpiochip_add_pinlist_range() function

Message ID 20241211-aaeon-up-board-pinctrl-support-v1-3-24719be27631@bootlin.com
State New
Headers show
Series Add support for the AAEON UP board FPGA | expand

Commit Message

Thomas Richard Dec. 11, 2024, 4:27 p.m. UTC 
Add gpiochip_add_pinlist_range() 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().

The struct pinctrl_gpio_range has a 'pins' member which allows to set a
list of pins (which can be non consecutive). gpiochip_add_pinlist_range()
is identical to gpiochip_add_pin_range(), except it set 'pins' member
instead of 'pin_base' member.

Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/gpio/gpiolib.c      | 74 +++++++++++++++++++++++++++++++++------------
 include/linux/gpio/driver.h | 12 ++++++++
 2 files changed, 66 insertions(+), 20 deletions(-)

Comments

Bartosz Golaszewski Dec. 16, 2024, 9:17 a.m. UTC | #1 
On Wed, Dec 11, 2024 at 5:27 PM Thomas Richard
<thomas.richard@bootlin.com> wrote:
>
> Add gpiochip_add_pinlist_range() 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().
>
> The struct pinctrl_gpio_range has a 'pins' member which allows to set a
> list of pins (which can be non consecutive). gpiochip_add_pinlist_range()
> is identical to gpiochip_add_pin_range(), except it set 'pins' member
> instead of 'pin_base' member.
>
> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
> ---

I don't have anything against this change so in any case:

Acked-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

But may I suggest the name to be changed to
gpiochip_add_pin_range_sparse() for a better indication of its
purpose?

Bart
Thomas Richard Dec. 16, 2024, 10:02 a.m. UTC | #2 
On 12/16/24 10:17, Bartosz Golaszewski wrote:
> On Wed, Dec 11, 2024 at 5:27 PM Thomas Richard
> <thomas.richard@bootlin.com> wrote:
>>
>> Add gpiochip_add_pinlist_range() 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().
>>
>> The struct pinctrl_gpio_range has a 'pins' member which allows to set a
>> list of pins (which can be non consecutive). gpiochip_add_pinlist_range()
>> is identical to gpiochip_add_pin_range(), except it set 'pins' member
>> instead of 'pin_base' member.
>>
>> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
>> ---
> 
> I don't have anything against this change so in any case:
> 
> Acked-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> But may I suggest the name to be changed to
> gpiochip_add_pin_range_sparse() for a better indication of its
> purpose?

Hi Bartosz,

Yes for sure, I will change the name for the v2.

Regards,

Thomas
Linus Walleij Dec. 20, 2024, 12:31 p.m. UTC | #3 
Hi Thomas,

thanks for your patch!

On Wed, Dec 11, 2024 at 5:27 PM Thomas Richard
<thomas.richard@bootlin.com> wrote:

> Add gpiochip_add_pinlist_range() 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().
>
> The struct pinctrl_gpio_range has a 'pins' member which allows to set a
> list of pins (which can be non consecutive). gpiochip_add_pinlist_range()
> is identical to gpiochip_add_pin_range(), except it set 'pins' member
> instead of 'pin_base' member.
>
> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>

(...)
> -int gpiochip_add_pin_range(struct gpio_chip *gc, const char *pinctl_name,
> -                          unsigned int gpio_offset, unsigned int pin_offset,
> -                          unsigned int npins)
> +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)

Uh this looks messy and I'm not a fan, sadly.

Overall I dislike __inner_function() syntax, so use some name that
describe what is going on please, but I don't think we wanna do
this at all.

Instead of this hack that start to soften the boundary between GPIO
and pin control drivers, I think it is better to do what we often do
and move the whole GPIO driver over into the pin control driver
and have it all inside one single file, since I suspect the hardware
is supposed to be used as one single unit.

Please look at other combined GPIO+pin control drivers
for inspiration, such as:
pinctrl-stmfx.c
pinctrl-sx150x.c

those just access any registers they need as they are in one
driver, but still maintains the separation by just calling the
existing gpiochip_add_pin_range() and be done with it.

This should remove your need for the strange hacks like this
patch and the gpiochip wrapper in the pin control driver.

Yours,
Linus Walleij
diff mbox series

Patch

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 3c8ec7f3a83b..e0fe07167e62 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2227,26 +2227,9 @@  int gpiochip_add_pingroup_range(struct gpio_chip *gc,
 }
 EXPORT_SYMBOL_GPL(gpiochip_add_pingroup_range);
 
-/**
- * gpiochip_add_pin_range() - 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_offset: the start offset in the pin controller number space
- * @npins: the number of pins from the offset of each pin space (GPIO and
- *	pin controller) 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.
- *
- * Returns:
- * 0 on success, or a negative errno on failure.
- */
-int gpiochip_add_pin_range(struct gpio_chip *gc, const char *pinctl_name,
-			   unsigned int gpio_offset, unsigned int pin_offset,
-			   unsigned int npins)
+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)
 {
 	struct gpio_pin_range *pin_range;
 	struct gpio_device *gdev = gc->gpiodev;
@@ -2264,6 +2247,7 @@  int gpiochip_add_pin_range(struct gpio_chip *gc, const char *pinctl_name,
 	pin_range->range.name = gc->label;
 	pin_range->range.base = gdev->base + gpio_offset;
 	pin_range->range.pin_base = pin_offset;
+	pin_range->range.pins = pins;
 	pin_range->range.npins = npins;
 	pin_range->pctldev = pinctrl_find_and_add_gpio_range(pinctl_name,
 			&pin_range->range);
@@ -2282,8 +2266,58 @@  int gpiochip_add_pin_range(struct gpio_chip *gc, const char *pinctl_name,
 
 	return 0;
 }
+
+/**
+ * gpiochip_add_pin_range() - 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_offset: the start offset in the pin controller number space
+ * @npins: the number of pins from the offset of each pin space (GPIO and
+ *	pin controller) 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.
+ *
+ * Returns:
+ * 0 on success, or a negative errno on failure.
+ */
+int gpiochip_add_pin_range(struct gpio_chip *gc, const char *pinctl_name,
+			   unsigned int gpio_offset, unsigned int pin_offset,
+			   unsigned int npins)
+{
+	return __gpiochip_add_pin_range(gc, pinctl_name, gpio_offset,
+					pin_offset, NULL, npins);
+}
 EXPORT_SYMBOL_GPL(gpiochip_add_pin_range);
 
+/**
+ * gpiochip_add_pinlist_range() - 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.
+ *
+ * Returns:
+ * 0 on success, or a negative errno on failure.
+ */
+int gpiochip_add_pinlist_range(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_pinlist_range);
+
 /**
  * gpiochip_remove_pin_ranges() - remove all the GPIO <-> pin mappings
  * @gc: the chip to remove all the mappings for
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 2dd7cb9cc270..8037f9a1e2a9 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -751,6 +751,9 @@  struct gpio_pin_range {
 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_pinlist_range(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);
@@ -765,6 +768,15 @@  gpiochip_add_pin_range(struct gpio_chip *gc, const char *pinctl_name,
 {
 	return 0;
 }
+
+static inline int
+gpiochip_add_pinlist_range(struct gpio_chip *gc, const char *pinctl_name,
+			   unsigned int gpio_offset, unsigned int const *pins,
+			   unsigned int npins)
+{
+	return 0;
+}
+
 static inline int
 gpiochip_add_pingroup_range(struct gpio_chip *gc,
 			struct pinctrl_dev *pctldev,