[PATCH RFC/RFT 0/2] gpio: allow auto population of request/free

[PATCH RFC/RFT 0/2] gpio: allow auto population of request/free

[Jonas Gorski]

Many pinctrl/gpio combinations are fine with the generic gpio
request/free implementations, and some only have a subset of the
gpios actually support muxed.

The common occurrence is

a) populate request/free of gpiochip
b) if only some gcs have pinmux, store for the chip if it does
   and call pinctrl_request/free conditionally
c) register gpiochip
d) add ranges to chip

To reduce the amount of work for the drivers, introduce a
gpiochip_add_with_ranges(), that allows doing c and d at the same time.

Since gpiochip_add_with_ranges then has the information at hand whether
the chip ties into the pinctrl subsystem, we can then also automatically
populate the gpiochip's request/free in case ranges were passed or a
gpio range property is present on the of node of the chip. This handles
a) and b) for most known drivers.

This patchset is a proof-of-concept, with the pinctrl-cygnus-gpio driver
as the example conversion to it. It registers a total of 51 gpio ranges,
which I thought as an ideal maximum reduction example.

This patchset depends on "gpiochip: add and use generic request/free".

As last time only build tested, not run tested due to lacking hardware.

A few points of disscussion:

So far I have just reused the struct pinctrl_gpio_range, but ignore most
fields, which might bloat the kernel a bit. OTOH 0's should compress
fine.

Also currently the code assumes all ranges are handled by the same
pinctrl device. Currently this could be trivially fixed as
pinctrl_gpio_range has a field for the pinctrl device name.

So far I have assumed there are no actual platorms with such a setup.


Jonas Gorski (2):
  gpio: allow atomic registration of gpio chip with pin ranges
  pinctrl-cygnus-gpio: convert to use gpiochip_add_with_ranges

 drivers/gpio/gpiolib-of.c                 |   5 ++
 drivers/gpio/gpiolib.c                    |  43 ++++++++--
 drivers/gpio/gpiolib.h                    |   9 +++
 drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c | 126 +++++++-----------------------
 include/linux/gpio/driver.h               |  10 ++-
 5 files changed, 85 insertions(+), 108 deletions(-)

-- 
2.1.4

[PATCH RFC/RFT 1/2] gpio: allow atomic registration of gpio chip with pin ranges

[Jonas Gorski]

Allow registering gpio chip with ranges at the same time, so we know
at registration time whether there is an associated pin controller.

This allows us to automatically populate the request/free callbacks,
so that drivers are free to omit the assignment, if they do not need
any special handling.

Signed-off-by: Jonas Gorski <jogo@openwrt.org>
---
 drivers/gpio/gpiolib-of.c   |  5 +++++
 drivers/gpio/gpiolib.c      | 43 +++++++++++++++++++++++++++++++++++--------
 drivers/gpio/gpiolib.h      |  9 +++++++++
 include/linux/gpio/driver.h | 10 +++++++++-
 4 files changed, 58 insertions(+), 9 deletions(-)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 5fe34a9..e05f0c2e 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 8eba02d..09d87ae 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -280,30 +280,47 @@ static int gpiochip_set_desc_names(struct gpio_chip *gc)
 }
 
 /**
- * gpiochip_add() - register a gpio_chip
+ * gpiochip_add_with_ranges() - register a gpio_chip with pin ranges
  * @chip: the chip to register, with chip->base initialized
+ * @pinctrl_name: the dev_name() of the pin controller to map to
+ * @ranges: the mappings of relative gpio offsets to pins
+ * @nranges: the number of ranges
  * Context: potentially before irqs will work
  *
  * Returns a negative errno if the chip can't be registered, such as
  * because the chip->base is invalid or already associated with a
  * different chip.  Otherwise it returns zero as a success code.
  *
- * When gpiochip_add() is called very early during boot, so that GPIOs
- * can be freely used, the chip->dev device must be registered before
- * the gpio framework's arch_initcall().  Otherwise sysfs initialization
- * for GPIOs will fail rudely.
+ * When gpiochip_add_with_ranges() is called very early during boot, so
+ * that GPIOs can be freely used, the chip->dev device must be
+ * registered before the gpio framework's arch_initcall().  Otherwise
+ * sysfs initialization for GPIOs will fail rudely.
  *
  * If chip->base is negative, this requests dynamic assignment of
  * a range of valid GPIOs.
+ *
+ * If nranges is zero, pinctrl_name and ranges may be NULL.
+ * If nranges is not zero or chip->of_node is populated and has a
+ * "gpio-ranges" property, chip->request and chip->free will be populated
+ * with generic callbacks if not yet set.
  */
-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 ((ranges > 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;
@@ -359,6 +376,15 @@ 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, range->base,
+						range->pin_base, range->npins);
+		if (status)
+			goto err_remove_chip;
+	}
+
 	acpi_gpiochip_add(chip);
 
 	status = gpiochip_sysfs_register(chip);
@@ -373,6 +399,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);
 err_remove_from_list:
@@ -389,7 +416,7 @@ err_free_descs:
 		chip->label ? : "generic");
 	return status;
 }
-EXPORT_SYMBOL_GPL(gpiochip_add);
+EXPORT_SYMBOL_GPL(gpiochip_add_with_ranges);
 
 /**
  * gpiochip_remove() - unregister a gpio_chip
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 78e634d..6f49e25 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 d1baebf..727ae9e 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -166,7 +166,15 @@ extern const char *gpiochip_is_requested(struct gpio_chip *chip,
 			unsigned offset);
 
 /* add/remove chips */
-extern 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);
+
+static inline int gpiochip_add(struct gpio_chip *chip)
+{
+	return gpiochip_add_with_ranges(chip, NULL, NULL, 0);
+}
+
 extern void gpiochip_remove(struct gpio_chip *chip);
 extern struct gpio_chip *gpiochip_find(void *data,
 			      int (*match)(struct gpio_chip *chip, void *data));
-- 
2.1.4

Re: [PATCH RFC/RFT 1/2] gpio: allow atomic registration of gpio chip with pin ranges

[Ray Jui]

++ Broadcom engineers working on Cygnus GPIO driver

On 10/11/2015 8:48 AM, Jonas Gorski wrote:
> Allow registering gpio chip with ranges at the same time, so we know
> at registration time whether there is an associated pin controller.
> 
> This allows us to automatically populate the request/free callbacks,
> so that drivers are free to omit the assignment, if they do not need
> any special handling.
> 
> Signed-off-by: Jonas Gorski <jogo@openwrt.org>
> ---
>  drivers/gpio/gpiolib-of.c   |  5 +++++
>  drivers/gpio/gpiolib.c      | 43 +++++++++++++++++++++++++++++++++++--------
>  drivers/gpio/gpiolib.h      |  9 +++++++++
>  include/linux/gpio/driver.h | 10 +++++++++-
>  4 files changed, 58 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> index 5fe34a9..e05f0c2e 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 8eba02d..09d87ae 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -280,30 +280,47 @@ static int gpiochip_set_desc_names(struct gpio_chip *gc)
>  }
>  
>  /**
> - * gpiochip_add() - register a gpio_chip
> + * gpiochip_add_with_ranges() - register a gpio_chip with pin ranges
>   * @chip: the chip to register, with chip->base initialized
> + * @pinctrl_name: the dev_name() of the pin controller to map to
> + * @ranges: the mappings of relative gpio offsets to pins
> + * @nranges: the number of ranges
>   * Context: potentially before irqs will work
>   *
>   * Returns a negative errno if the chip can't be registered, such as
>   * because the chip->base is invalid or already associated with a
>   * different chip.  Otherwise it returns zero as a success code.
>   *
> - * When gpiochip_add() is called very early during boot, so that GPIOs
> - * can be freely used, the chip->dev device must be registered before
> - * the gpio framework's arch_initcall().  Otherwise sysfs initialization
> - * for GPIOs will fail rudely.
> + * When gpiochip_add_with_ranges() is called very early during boot, so
> + * that GPIOs can be freely used, the chip->dev device must be
> + * registered before the gpio framework's arch_initcall().  Otherwise
> + * sysfs initialization for GPIOs will fail rudely.
>   *
>   * If chip->base is negative, this requests dynamic assignment of
>   * a range of valid GPIOs.
> + *
> + * If nranges is zero, pinctrl_name and ranges may be NULL.
> + * If nranges is not zero or chip->of_node is populated and has a
> + * "gpio-ranges" property, chip->request and chip->free will be populated
> + * with generic callbacks if not yet set.
>   */
> -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 ((ranges > 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;
> @@ -359,6 +376,15 @@ 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, range->base,
> +						range->pin_base, range->npins);
> +		if (status)
> +			goto err_remove_chip;
> +	}
> +
>  	acpi_gpiochip_add(chip);
>  
>  	status = gpiochip_sysfs_register(chip);
> @@ -373,6 +399,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);
>  err_remove_from_list:
> @@ -389,7 +416,7 @@ err_free_descs:
>  		chip->label ? : "generic");
>  	return status;
>  }
> -EXPORT_SYMBOL_GPL(gpiochip_add);
> +EXPORT_SYMBOL_GPL(gpiochip_add_with_ranges);
>  
>  /**
>   * gpiochip_remove() - unregister a gpio_chip
> diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
> index 78e634d..6f49e25 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 d1baebf..727ae9e 100644
> --- a/include/linux/gpio/driver.h
> +++ b/include/linux/gpio/driver.h
> @@ -166,7 +166,15 @@ extern const char *gpiochip_is_requested(struct gpio_chip *chip,
>  			unsigned offset);
>  
>  /* add/remove chips */
> -extern 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);
> +
> +static inline int gpiochip_add(struct gpio_chip *chip)
> +{
> +	return gpiochip_add_with_ranges(chip, NULL, NULL, 0);
> +}
> +
>  extern void gpiochip_remove(struct gpio_chip *chip);
>  extern struct gpio_chip *gpiochip_find(void *data,
>  			      int (*match)(struct gpio_chip *chip, void *data));
> 

Re: [PATCH RFC/RFT 1/2] gpio: allow atomic registration of gpio chip with pin ranges

[Linus Walleij]

On Sun, Oct 11, 2015 at 5:48 PM, Jonas Gorski <jogo@openwrt.org> wrote:

> Allow registering gpio chip with ranges at the same time, so we know
> at registration time whether there is an associated pin controller.
>
> This allows us to automatically populate the request/free callbacks,
> so that drivers are free to omit the assignment, if they do not need
> any special handling.
>
> Signed-off-by: Jonas Gorski <jogo@openwrt.org>

I think this looks elegant. I guess this will work nicely for pl061
where we just set the range from DT?

Yours,
Linus Walleij

[PATCH RFC/RFT 2/2] pinctrl-cygnus-gpio: convert to use gpiochip_add_with_ranges

[Jonas Gorski]

Convert the driver to make use of passing ranges to gpiochip_add, and
drop the custom implemtation of gpio_ranges.
Since gpiochip_add_with_ranges also supports conditional setting of
request/free based on the existence of ranges, and pinmux_is_supported
held the same information, we can drop the custom request/free calbacks
at the same time and just use the generic implementations if needed.

Signed-off-by: Jonas Gorski <jogo@openwrt.org>
---
 drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c | 126 +++++++-----------------------
 1 file changed, 27 insertions(+), 99 deletions(-)

diff --git a/drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c b/drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c
index 1ca7830..d24218f 100644
--- a/drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c
+++ b/drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c
@@ -75,8 +75,6 @@
  * @lock: lock to protect access to I/O registers
  * @gc: GPIO chip
  * @num_banks: number of GPIO banks, each bank supports up to 32 GPIOs
- * @pinmux_is_supported: flag to indicate this GPIO controller contains pins
- * that can be individually muxed to GPIO
  * @pctl: pointer to pinctrl_dev
  * @pctldesc: pinctrl descriptor
  */
@@ -91,8 +89,6 @@ struct cygnus_gpio {
 	struct gpio_chip gc;
 	unsigned num_banks;
 
-	bool pinmux_is_supported;
-
 	struct pinctrl_dev *pctl;
 	struct pinctrl_desc pctldesc;
 };
@@ -289,28 +285,6 @@ static struct irq_chip cygnus_gpio_irq_chip = {
 /*
  * Request the Cygnus IOMUX pinmux controller to mux individual pins to GPIO
  */
-static int cygnus_gpio_request(struct gpio_chip *gc, unsigned offset)
-{
-	struct cygnus_gpio *chip = to_cygnus_gpio(gc);
-	unsigned gpio = gc->base + offset;
-
-	/* not all Cygnus GPIO pins can be muxed individually */
-	if (!chip->pinmux_is_supported)
-		return 0;
-
-	return pinctrl_request_gpio(gpio);
-}
-
-static void cygnus_gpio_free(struct gpio_chip *gc, unsigned offset)
-{
-	struct cygnus_gpio *chip = to_cygnus_gpio(gc);
-	unsigned gpio = gc->base + offset;
-
-	if (!chip->pinmux_is_supported)
-		return;
-
-	pinctrl_free_gpio(gpio);
-}
 
 static int cygnus_gpio_direction_input(struct gpio_chip *gc, unsigned gpio)
 {
@@ -596,22 +570,12 @@ static const struct pinconf_ops cygnus_pconf_ops = {
 	.pin_config_set = cygnus_pin_config_set,
 };
 
-/*
- * Map a GPIO in the local gpio_chip pin space to a pin in the Cygnus IOMUX
- * pinctrl pin space
- */
-struct cygnus_gpio_pin_range {
-	unsigned offset;
-	unsigned pin_base;
-	unsigned num_pins;
-};
-
-#define CYGNUS_PINRANGE(o, p, n) { .offset = o, .pin_base = p, .num_pins = n }
+#define CYGNUS_PINRANGE(o, p, n) { .base = o, .pin_base = p, .npins = n }
 
 /*
  * Pin mapping table for mapping local GPIO pins to Cygnus IOMUX pinctrl pins
  */
-static const struct cygnus_gpio_pin_range cygnus_gpio_pintable[] = {
+static const struct pinctrl_gpio_range cygnus_gpio_pintable[] = {
 	CYGNUS_PINRANGE(0, 42, 1),
 	CYGNUS_PINRANGE(1, 44, 3),
 	CYGNUS_PINRANGE(4, 48, 1),
@@ -666,58 +630,6 @@ static const struct cygnus_gpio_pin_range cygnus_gpio_pintable[] = {
 };
 
 /*
- * The Cygnus IOMUX controller mainly supports group based mux configuration,
- * but certain pins can be muxed to GPIO individually. Only the ASIU GPIO
- * controller can support this, so it's an optional configuration
- *
- * Return -ENODEV means no support and that's fine
- */
-static int cygnus_gpio_pinmux_add_range(struct cygnus_gpio *chip)
-{
-	struct device_node *node = chip->dev->of_node;
-	struct device_node *pinmux_node;
-	struct platform_device *pinmux_pdev;
-	struct gpio_chip *gc = &chip->gc;
-	int i, ret = 0;
-
-	/* parse DT to find the phandle to the pinmux controller */
-	pinmux_node = of_parse_phandle(node, "pinmux", 0);
-	if (!pinmux_node)
-		return -ENODEV;
-
-	pinmux_pdev = of_find_device_by_node(pinmux_node);
-	/* no longer need the pinmux node */
-	of_node_put(pinmux_node);
-	if (!pinmux_pdev) {
-		dev_err(chip->dev, "failed to get pinmux device\n");
-		return -EINVAL;
-	}
-
-	/* now need to create the mapping between local GPIO and PINMUX pins */
-	for (i = 0; i < ARRAY_SIZE(cygnus_gpio_pintable); i++) {
-		ret = gpiochip_add_pin_range(gc, dev_name(&pinmux_pdev->dev),
-					     cygnus_gpio_pintable[i].offset,
-					     cygnus_gpio_pintable[i].pin_base,
-					     cygnus_gpio_pintable[i].num_pins);
-		if (ret) {
-			dev_err(chip->dev, "unable to add GPIO pin range\n");
-			goto err_put_device;
-		}
-	}
-
-	chip->pinmux_is_supported = true;
-
-	/* no need for pinmux_pdev device reference anymore */
-	put_device(&pinmux_pdev->dev);
-	return 0;
-
-err_put_device:
-	put_device(&pinmux_pdev->dev);
-	gpiochip_remove_pin_ranges(gc);
-	return ret;
-}
-
-/*
  * Cygnus GPIO controller supports some PINCONF related configurations such as
  * pull up, pull down, and drive strength, when the pin is configured to GPIO
  *
@@ -805,6 +717,10 @@ static int cygnus_gpio_probe(struct platform_device *pdev)
 	int irq, ret;
 	const struct of_device_id *match;
 	const struct cygnus_gpio_data *gpio_data;
+	struct device_node *pinmux_node;
+	const char *pinctrl_name = NULL;
+	const struct pinctrl_gpio_range *ranges = NULL;
+	unsigned int nranges = 0;
 
 	match = of_match_device(cygnus_gpio_of_match, dev);
 	if (!match)
@@ -844,25 +760,37 @@ static int cygnus_gpio_probe(struct platform_device *pdev)
 	gc->label = dev_name(dev);
 	gc->dev = dev;
 	gc->of_node = dev->of_node;
-	gc->request = cygnus_gpio_request;
-	gc->free = cygnus_gpio_free;
 	gc->direction_input = cygnus_gpio_direction_input;
 	gc->direction_output = cygnus_gpio_direction_output;
 	gc->set = cygnus_gpio_set;
 	gc->get = cygnus_gpio_get;
 
-	ret = gpiochip_add(gc);
+	/* parse DT to find the phandle to the pinmux controller */
+	pinmux_node = of_parse_phandle(dev->of_node, "pinmux", 0);
+	if (pinmux_node) {
+		struct platform_device *pinmux_pdev;
+
+		pinmux_pdev = of_find_device_by_node(pinmux_node);
+		/* no longer need the pinmux node */
+		of_node_put(pinmux_node);
+		if (!pinmux_pdev) {
+			dev_err(chip->dev, "failed to get pinmux device\n");
+			return -EINVAL;
+		}
+
+		pinctrl_name = dev_name(&pinmux_pdev->dev);
+		ranges = cygnus_gpio_pintable;
+		nranges = ARRAY_SIZE(cygnus_gpio_pintable);
+
+		put_device(&pinmux_pdev->dev);
+	}
+
+	ret = gpiochip_add_with_ranges(gc, pinctrl_name, ranges, nranges);
 	if (ret < 0) {
 		dev_err(dev, "unable to add GPIO chip\n");
 		return ret;
 	}
 
-	ret = cygnus_gpio_pinmux_add_range(chip);
-	if (ret && ret != -ENODEV) {
-		dev_err(dev, "unable to add GPIO pin range\n");
-		goto err_rm_gpiochip;
-	}
-
 	ret = cygnus_gpio_register_pinconf(chip);
 	if (ret) {
 		dev_err(dev, "unable to register pinconf\n");
-- 
2.1.4

Re: [PATCH RFC/RFT 2/2] pinctrl-cygnus-gpio: convert to use gpiochip_add_with_ranges

[Ray Jui]

++ Broadcom engineers working on Cygnus GPIO driver

We will discuss this change on the other email thread, i.e., [PATCH
RFC/RFT 0/2]...

On 10/11/2015 8:48 AM, Jonas Gorski wrote:
> Convert the driver to make use of passing ranges to gpiochip_add, and
> drop the custom implemtation of gpio_ranges.
> Since gpiochip_add_with_ranges also supports conditional setting of
> request/free based on the existence of ranges, and pinmux_is_supported
> held the same information, we can drop the custom request/free calbacks
> at the same time and just use the generic implementations if needed.
> 
> Signed-off-by: Jonas Gorski <jogo@openwrt.org>
> ---
>  drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c | 126 +++++++-----------------------
>  1 file changed, 27 insertions(+), 99 deletions(-)
> 
> diff --git a/drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c b/drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c
> index 1ca7830..d24218f 100644
> --- a/drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c
> +++ b/drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c
> @@ -75,8 +75,6 @@
>   * @lock: lock to protect access to I/O registers
>   * @gc: GPIO chip
>   * @num_banks: number of GPIO banks, each bank supports up to 32 GPIOs
> - * @pinmux_is_supported: flag to indicate this GPIO controller contains pins
> - * that can be individually muxed to GPIO
>   * @pctl: pointer to pinctrl_dev
>   * @pctldesc: pinctrl descriptor
>   */
> @@ -91,8 +89,6 @@ struct cygnus_gpio {
>  	struct gpio_chip gc;
>  	unsigned num_banks;
>  
> -	bool pinmux_is_supported;
> -
>  	struct pinctrl_dev *pctl;
>  	struct pinctrl_desc pctldesc;
>  };
> @@ -289,28 +285,6 @@ static struct irq_chip cygnus_gpio_irq_chip = {
>  /*
>   * Request the Cygnus IOMUX pinmux controller to mux individual pins to GPIO
>   */
> -static int cygnus_gpio_request(struct gpio_chip *gc, unsigned offset)
> -{
> -	struct cygnus_gpio *chip = to_cygnus_gpio(gc);
> -	unsigned gpio = gc->base + offset;
> -
> -	/* not all Cygnus GPIO pins can be muxed individually */
> -	if (!chip->pinmux_is_supported)
> -		return 0;
> -
> -	return pinctrl_request_gpio(gpio);
> -}
> -
> -static void cygnus_gpio_free(struct gpio_chip *gc, unsigned offset)
> -{
> -	struct cygnus_gpio *chip = to_cygnus_gpio(gc);
> -	unsigned gpio = gc->base + offset;
> -
> -	if (!chip->pinmux_is_supported)
> -		return;
> -
> -	pinctrl_free_gpio(gpio);
> -}
>  
>  static int cygnus_gpio_direction_input(struct gpio_chip *gc, unsigned gpio)
>  {
> @@ -596,22 +570,12 @@ static const struct pinconf_ops cygnus_pconf_ops = {
>  	.pin_config_set = cygnus_pin_config_set,
>  };
>  
> -/*
> - * Map a GPIO in the local gpio_chip pin space to a pin in the Cygnus IOMUX
> - * pinctrl pin space
> - */
> -struct cygnus_gpio_pin_range {
> -	unsigned offset;
> -	unsigned pin_base;
> -	unsigned num_pins;
> -};
> -
> -#define CYGNUS_PINRANGE(o, p, n) { .offset = o, .pin_base = p, .num_pins = n }
> +#define CYGNUS_PINRANGE(o, p, n) { .base = o, .pin_base = p, .npins = n }
>  
>  /*
>   * Pin mapping table for mapping local GPIO pins to Cygnus IOMUX pinctrl pins
>   */
> -static const struct cygnus_gpio_pin_range cygnus_gpio_pintable[] = {
> +static const struct pinctrl_gpio_range cygnus_gpio_pintable[] = {
>  	CYGNUS_PINRANGE(0, 42, 1),
>  	CYGNUS_PINRANGE(1, 44, 3),
>  	CYGNUS_PINRANGE(4, 48, 1),
> @@ -666,58 +630,6 @@ static const struct cygnus_gpio_pin_range cygnus_gpio_pintable[] = {
>  };
>  
>  /*
> - * The Cygnus IOMUX controller mainly supports group based mux configuration,
> - * but certain pins can be muxed to GPIO individually. Only the ASIU GPIO
> - * controller can support this, so it's an optional configuration
> - *
> - * Return -ENODEV means no support and that's fine
> - */
> -static int cygnus_gpio_pinmux_add_range(struct cygnus_gpio *chip)
> -{
> -	struct device_node *node = chip->dev->of_node;
> -	struct device_node *pinmux_node;
> -	struct platform_device *pinmux_pdev;
> -	struct gpio_chip *gc = &chip->gc;
> -	int i, ret = 0;
> -
> -	/* parse DT to find the phandle to the pinmux controller */
> -	pinmux_node = of_parse_phandle(node, "pinmux", 0);
> -	if (!pinmux_node)
> -		return -ENODEV;
> -
> -	pinmux_pdev = of_find_device_by_node(pinmux_node);
> -	/* no longer need the pinmux node */
> -	of_node_put(pinmux_node);
> -	if (!pinmux_pdev) {
> -		dev_err(chip->dev, "failed to get pinmux device\n");
> -		return -EINVAL;
> -	}
> -
> -	/* now need to create the mapping between local GPIO and PINMUX pins */
> -	for (i = 0; i < ARRAY_SIZE(cygnus_gpio_pintable); i++) {
> -		ret = gpiochip_add_pin_range(gc, dev_name(&pinmux_pdev->dev),
> -					     cygnus_gpio_pintable[i].offset,
> -					     cygnus_gpio_pintable[i].pin_base,
> -					     cygnus_gpio_pintable[i].num_pins);
> -		if (ret) {
> -			dev_err(chip->dev, "unable to add GPIO pin range\n");
> -			goto err_put_device;
> -		}
> -	}
> -
> -	chip->pinmux_is_supported = true;
> -
> -	/* no need for pinmux_pdev device reference anymore */
> -	put_device(&pinmux_pdev->dev);
> -	return 0;
> -
> -err_put_device:
> -	put_device(&pinmux_pdev->dev);
> -	gpiochip_remove_pin_ranges(gc);
> -	return ret;
> -}
> -
> -/*
>   * Cygnus GPIO controller supports some PINCONF related configurations such as
>   * pull up, pull down, and drive strength, when the pin is configured to GPIO
>   *
> @@ -805,6 +717,10 @@ static int cygnus_gpio_probe(struct platform_device *pdev)
>  	int irq, ret;
>  	const struct of_device_id *match;
>  	const struct cygnus_gpio_data *gpio_data;
> +	struct device_node *pinmux_node;
> +	const char *pinctrl_name = NULL;
> +	const struct pinctrl_gpio_range *ranges = NULL;
> +	unsigned int nranges = 0;
>  
>  	match = of_match_device(cygnus_gpio_of_match, dev);
>  	if (!match)
> @@ -844,25 +760,37 @@ static int cygnus_gpio_probe(struct platform_device *pdev)
>  	gc->label = dev_name(dev);
>  	gc->dev = dev;
>  	gc->of_node = dev->of_node;
> -	gc->request = cygnus_gpio_request;
> -	gc->free = cygnus_gpio_free;
>  	gc->direction_input = cygnus_gpio_direction_input;
>  	gc->direction_output = cygnus_gpio_direction_output;
>  	gc->set = cygnus_gpio_set;
>  	gc->get = cygnus_gpio_get;
>  
> -	ret = gpiochip_add(gc);
> +	/* parse DT to find the phandle to the pinmux controller */
> +	pinmux_node = of_parse_phandle(dev->of_node, "pinmux", 0);
> +	if (pinmux_node) {
> +		struct platform_device *pinmux_pdev;
> +
> +		pinmux_pdev = of_find_device_by_node(pinmux_node);
> +		/* no longer need the pinmux node */
> +		of_node_put(pinmux_node);
> +		if (!pinmux_pdev) {
> +			dev_err(chip->dev, "failed to get pinmux device\n");
> +			return -EINVAL;
> +		}
> +
> +		pinctrl_name = dev_name(&pinmux_pdev->dev);
> +		ranges = cygnus_gpio_pintable;
> +		nranges = ARRAY_SIZE(cygnus_gpio_pintable);
> +
> +		put_device(&pinmux_pdev->dev);
> +	}
> +
> +	ret = gpiochip_add_with_ranges(gc, pinctrl_name, ranges, nranges);
>  	if (ret < 0) {
>  		dev_err(dev, "unable to add GPIO chip\n");
>  		return ret;
>  	}
>  
> -	ret = cygnus_gpio_pinmux_add_range(chip);
> -	if (ret && ret != -ENODEV) {
> -		dev_err(dev, "unable to add GPIO pin range\n");
> -		goto err_rm_gpiochip;
> -	}
> -
>  	ret = cygnus_gpio_register_pinconf(chip);
>  	if (ret) {
>  		dev_err(dev, "unable to register pinconf\n");
> 

Re: [PATCH RFC/RFT 0/2] gpio: allow auto population of request/free

[Ray Jui]

Adding Broadcom engineers who are currently working on expanding this
Cygnus GPIO driver to support more SoCs.

On 10/11/2015 8:48 AM, Jonas Gorski wrote:
> Many pinctrl/gpio combinations are fine with the generic gpio
> request/free implementations, and some only have a subset of the
> gpios actually support muxed.
> 
> The common occurrence is
> 
> a) populate request/free of gpiochip
> b) if only some gcs have pinmux, store for the chip if it does
>    and call pinctrl_request/free conditionally
> c) register gpiochip
> d) add ranges to chip
> 
> To reduce the amount of work for the drivers, introduce a
> gpiochip_add_with_ranges(), that allows doing c and d at the same time.
> 
> Since gpiochip_add_with_ranges then has the information at hand whether
> the chip ties into the pinctrl subsystem, we can then also automatically
> populate the gpiochip's request/free in case ranges were passed or a
> gpio range property is present on the of node of the chip. This handles
> a) and b) for most known drivers.
> 
> This patchset is a proof-of-concept, with the pinctrl-cygnus-gpio driver
> as the example conversion to it. It registers a total of 51 gpio ranges,
> which I thought as an ideal maximum reduction example.

There's a bit complication here. Internally within Broadcom, the current
Cygnus GPIO driver is undergoing a lot of changes to make it more
generic, to support all other Broadcom iProc based SoCs that use the
same GPIO controller.

One of the changes we made is to make use of the DT based gpio-ranges
properly, i.e., we are setting GPIO->pinmux mapping in device tree
instead of hardcoded in the driver. This feature is already supported by
of_gpiochip_add_pin_range, called by of_gpiochip_add, called by
gpiochip_add.

Due to this, I'm not sure if the Cygnus GPIO driver is suitable as an
example for this change.

The internal work of the changes on this Cygnus GPIO driver is close to
be done and can be upstreamed very soon (e.g., sometime this week or
early next week).

Linus, please advise how we should proceed with this.

> 
> This patchset depends on "gpiochip: add and use generic request/free".
> 
> As last time only build tested, not run tested due to lacking hardware.
> 
> A few points of disscussion:
> 
> So far I have just reused the struct pinctrl_gpio_range, but ignore most
> fields, which might bloat the kernel a bit. OTOH 0's should compress
> fine.
> 
> Also currently the code assumes all ranges are handled by the same
> pinctrl device. Currently this could be trivially fixed as
> pinctrl_gpio_range has a field for the pinctrl device name.
> 
> So far I have assumed there are no actual platorms with such a setup.
> 
> 
> Jonas Gorski (2):
>   gpio: allow atomic registration of gpio chip with pin ranges
>   pinctrl-cygnus-gpio: convert to use gpiochip_add_with_ranges
> 
>  drivers/gpio/gpiolib-of.c                 |   5 ++
>  drivers/gpio/gpiolib.c                    |  43 ++++++++--
>  drivers/gpio/gpiolib.h                    |   9 +++
>  drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c | 126 +++++++-----------------------
>  include/linux/gpio/driver.h               |  10 ++-
>  5 files changed, 85 insertions(+), 108 deletions(-)
> 

Re: [PATCH RFC/RFT 0/2] gpio: allow auto population of request/free

[Linus Walleij]

On Tue, Oct 13, 2015 at 10:31 PM, Ray Jui <rjui@broadcom.com> wrote:

> One of the changes we made is to make use of the DT based gpio-ranges
> properly, i.e., we are setting GPIO->pinmux mapping in device tree
> instead of hardcoded in the driver. This feature is already supported by
> of_gpiochip_add_pin_range, called by of_gpiochip_add, called by
> gpiochip_add.
>
> Due to this, I'm not sure if the Cygnus GPIO driver is suitable as an
> example for this change.
(...)
> Linus, please advise how we should proceed with this.

It is more important that you switch to fetching the ranges from the
device tree. That is a very important clean-up.

Yours,
Linus Walleij

Re: [PATCH RFC/RFT 0/2] gpio: allow auto population of request/free

[Ray Jui]

On 10/16/2015 2:04 PM, Linus Walleij wrote:
> On Tue, Oct 13, 2015 at 10:31 PM, Ray Jui <rjui@broadcom.com> wrote:
> 
>> One of the changes we made is to make use of the DT based gpio-ranges
>> properly, i.e., we are setting GPIO->pinmux mapping in device tree
>> instead of hardcoded in the driver. This feature is already supported by
>> of_gpiochip_add_pin_range, called by of_gpiochip_add, called by
>> gpiochip_add.
>>
>> Due to this, I'm not sure if the Cygnus GPIO driver is suitable as an
>> example for this change.
> (...)
>> Linus, please advise how we should proceed with this.
> 
> It is more important that you switch to fetching the ranges from the
> device tree. That is a very important clean-up.
> 
> Yours,
> Linus Walleij
> 

Okay. Thanks for the clarification. Pramod (CCed on this email) will
likely be sending out various clean up patches early next week.

Thanks,

Ray