Re: [PATCH RFT 4/5] gpio: gpio-pl061: use the generic request/free implementations

[Jonas Gorski <jogo@xxxxxxxxxxx>]

On 05.10.2015 11:19, Linus Walleij wrote:
> On Sun, Sep 13, 2015 at 3:21 PM, Jonas Gorski <jogo@xxxxxxxxxxx> wrote:
> 
>>         spin_lock_init(&chip->lock);
>> -       if (of_property_read_bool(dev->of_node, "gpio-ranges"))
>> -               chip->uses_pinctrl = true;
>> +       if (of_property_read_bool(dev->of_node, "gpio-ranges")) {
>> +               chip->gc.request = gpiochip_generic_request;
>> +               chip->gc.free = gpiochip_generic_free;
>> +       }
> 
> This is way better.
> 
> But now this code is starting to multiply in drivers.
> 
> Can we think of a way to do this even more generic:
> could gpiochip_generic_request() check if the range is
> there instead?

I thought about this, and AFAICT this would open a window in which gpio
requests could bypass the pinctrl subsystem, as ranges are only registered
after the gpio chip was registered.
pinctrl_request_gpio() -> pinctrl_get_device_gpio_range() returns -EPROBEDEFER if
it can't find a range for that reason.

To solve this we could introduce a gpiochip_add_with_range(), then we can assign
the generic _request/_free in gpiochip_add in case the callbacks aren't set yet
(and keep the way _request/_free are implemented).

If the gpio-ranges property is used, we could then check for its existence to
assign the callbacks.

So my idea is something like the following. The conversion would be then

chip->request = foo_request;
chip->free = foo_free;
gpiochip_add(chip);
gpiochip_add_pin_range(chip, "foo", 0, 0, npins);

=>

static const struct __initconst pinctrl_gpio_range foo_range = {
	.base = 0,
	.pinbase = 0,
	.npins = npins,
};

gpiochip_add_with_ranges(chip, "foo", &foo_range, 1);

which would be a bit more verbose, but for drivers like pinctrl-cygnus-gpio,
which registers 51(!) pinranges for the same gpio chip, it would mean quite a
bit of code reduction.

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index fa6e3c8..b4c6119 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -415,6 +415,11 @@ static int of_gpiochip_add_pin_range(struct gpio_chip *chip)
 	return 0;
 }
 
+bool of_gpiochip_has_pin_range(struct gpio_chip *chip)
+{
+	return !!of_find_property(chip->of_node, "gpio-ranges", NULL);
+}
+
 #else
 static int of_gpiochip_add_pin_range(struct gpio_chip *chip) { return 0; }
 #endif
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index e0853fb..33f79b0 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -236,14 +236,23 @@ static int gpiochip_add_to_list(struct gpio_chip *chip)
  * If chip->base is negative, this requests dynamic assignment of
  * a range of valid GPIOs.
  */
-int gpiochip_add(struct gpio_chip *chip)
+int gpiochip_add_with_ranges(struct gpio_chip *chip, const char *pinctl_name,
+			     const struct pinctrl_gpio_range *ranges,
+			     unsigned int nranges)
 {
 	unsigned long	flags;
 	int		status = 0;
-	unsigned	id;
+	unsigned	id, i;
 	int		base = chip->base;
 	struct gpio_desc *descs;
 
+	if ((nranges > 0) || of_gpiochip_has_pin_range(chip)) {
+		if (!chip->request)
+			chip->request = gpiochip_generic_request;
+		if (!chip->free)
+			chip->free = gpiochip_generic_free;
+	}
+
 	descs = kcalloc(chip->ngpio, sizeof(descs[0]), GFP_KERNEL);
 	if (!descs)
 		return -ENOMEM;
@@ -295,6 +304,16 @@ int gpiochip_add(struct gpio_chip *chip)
 	if (status)
 		goto err_remove_chip;
 
+	for (i = 0; i < nranges; i++) {
+		const struct pinctrl_gpio_range *range = ranges[i];
+
+		status = gpiochip_add_pin_range(chip, pinctl_name,
+						chip->base + range->base,
+						range->pinbase, range->npins);
+		if (status)
+			goto err_remove_chip;
+	}
+
 	acpi_gpiochip_add(chip);
 
 	status = gpiochip_sysfs_register(chip);
@@ -309,6 +328,7 @@ int gpiochip_add(struct gpio_chip *chip)
 
 err_remove_chip:
 	acpi_gpiochip_remove(chip);
+	gpiochip_remove_pin_ranges(chip);
 	gpiochip_free_hogs(chip);
 	of_gpiochip_remove(chip);
 	spin_lock_irqsave(&gpio_lock, flags);
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index bf34300..05a71da 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -72,6 +72,15 @@ struct gpio_desc *of_get_named_gpiod_flags(struct device_node *np,
 
 struct gpio_desc *gpiochip_get_desc(struct gpio_chip *chip, u16 hwnum);
 
+#if defined(CONFIG_OF_GPIO) && defined(CONFIG_PINCTRL)
+bool of_gpiochip_has_pin_range(struct gpio_chip *chip);
+#else
+static inline bool of_gpiochip_has_pin_range(struct gpio_chip *chip)
+{
+	return false;
+}
+#endif
+
 extern struct spinlock gpio_lock;
 extern struct list_head gpio_chips;
 
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 0857c28..ea8a630 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -166,7 +166,14 @@ extern const char *gpiochip_is_requested(struct gpio_chip *chip,
 			unsigned offset);
 
 /* add/remove chips */
-extern int gpiochip_add(struct gpio_chip *chip);
+static inline int gpiochip_add(struct gpio_chip *chip)
+{
+	return gpiochip_add_with_ranges(chip, NULL, NULL, 0);
+}
+
+int gpiochip_add_with_ranges(struct gpio_chip *chip, const char *pinctl_name,
+			     const struct pinctrl_gpio_range *ranges,
+			     unsigned int nranges);
 extern void gpiochip_remove(struct gpio_chip *chip);
 extern struct gpio_chip *gpiochip_find(void *data,
 			      int (*match)(struct gpio_chip *chip, void *data));
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html