[Patch v5 0/2] gpio: add GPIO hogging mechanism

[Patch v5 0/2] gpio: add GPIO hogging mechanism

[Benoit Parrot]

This patch set re-introduces the gpio hogging concept first 
presented by Boris Brezillion.
This patch set provides a way to initially configure specific GPIO
when the GPIO controller is probed.

The actual DT scanning to collect the GPIO specific data is performed
as part of of_gpiochip_add().

The purpose of this is to allow specific GPIOs to be configured
without any driver specific code.
This is particularly useful because board design are getting
increasingly complex and given SoC pins can now have more
than 10 mux values a lot of connections are now dependent on
external IO muxes to switch various modes.

Specific drivers should not necessarily need to be aware of
what accounts to a specific board implementation. This board level
"description" should be best kept as part of the dts file.

Changes since v4:
 * Addressed review comments from Alexandre Courbot

Changes since v3:
 * Relocated the non-DT "hog" function to gpiolib.c.
 * Rename some of the function to be clearer and remove _ prefixes.
 * Replace the gpiod_request/gpiod_put usage with
   gpiochip_request_own_desc/free_own_desc version instead.
 * Refactor some of the logic to better handle error condition/reporting
 * Renamed the "direction" DT properties to "state".

Changes since v2:
 * Refactor the gpio-hog mechanism to split the DT related action
   from the actual "hogging" operation.
 * This allows non-DT providers to implement hogs as well.
 * Added FLAG_IS_HOGGED to mark hogged gpio and make gpiochip removal
   able to release hogged gpio.
 * Similarly to the hogging, the cleanup is performed as part of
   of_gpiochip_remove

Changes since v1:
 * Split the devicetree bindings documentation in its own patch.
 * Refactor the gpio-hog mechanism as private functions meant to
   be to invoked from of_gpiochip_add().

Benoit Parrot (2):
  gpio: add GPIO hogging mechanism
  gpio: Document GPIO hogging mechanism

 Documentation/devicetree/bindings/gpio/gpio.txt |  23 +++++
 drivers/gpio/gpiolib-of.c                       | 118 ++++++++++++++++++++++
 drivers/gpio/gpiolib.c                          | 128 ++++++++++++++++++++----
 drivers/gpio/gpiolib.h                          |   3 +
 4 files changed, 252 insertions(+), 20 deletions(-)

-- 
1.8.5.1

[Patch v5 1/2] gpio: add GPIO hogging mechanism

[Benoit Parrot]

Based on Boris Brezillion's work this is a reworked patch
of his initial GPIO hogging mechanism.
This patch provides a way to initially configure specific GPIO
when the GPIO controller is probed.

The actual DT scanning to collect the GPIO specific data is performed
as part of gpiochip_add().

The purpose of this is to allow specific GPIOs to be configured
without any driver specific code.
This is particularly useful because board design are getting
increasingly complex and given SoC pins can now have more
than 10 mux values, a lot of connections are now dependent on
external IO muxes to switch various modes.

Specific drivers should not necessarily need to be aware of
what accounts to a specific board implementation. This board level
"description" should be best kept as part of the dts file.

Signed-off-by: Benoit Parrot <bparrot@ti.com>
Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drivers/gpio/gpiolib-of.c | 118 ++++++++++++++++++++++++++++++++++++++++++
 drivers/gpio/gpiolib.c    | 128 ++++++++++++++++++++++++++++++++++++++--------
 drivers/gpio/gpiolib.h    |   3 ++
 3 files changed, 229 insertions(+), 20 deletions(-)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 604dbe6..cc33ba2 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -22,6 +22,7 @@
 #include <linux/of_gpio.h>
 #include <linux/pinctrl/pinctrl.h>
 #include <linux/slab.h>
+#include <linux/gpio/machine.h>
 
 #include "gpiolib.h"
 
@@ -111,6 +112,121 @@ int of_get_named_gpio_flags(struct device_node *np, const char *list_name,
 EXPORT_SYMBOL(of_get_named_gpio_flags);
 
 /**
+ * of_get_gpio_hog() - Get a GPIO hog descriptor, names and flags for GPIO API
+ * @np:		device node to get GPIO from
+ * @name:	GPIO line name
+ * @flags:	a flags pointer to fill in
+ *
+ * Returns GPIO descriptor to use with Linux GPIO API, or one of the errno
+ * value on the error condition.
+ */
+
+static struct gpio_desc *of_get_gpio_hog(struct device_node *np,
+				  const char **name,
+				  enum gpio_lookup_flags *lflags,
+				  enum gpiod_flags *dflags)
+{
+	struct device_node *chip_np;
+	enum of_gpio_flags xlate_flags;
+	struct gpio_desc *desc;
+	const char *dir_val;
+	struct gg_data gg_data = {
+		.flags = &xlate_flags,
+	};
+	u32 tmp;
+	int i, ret;
+
+	chip_np = np->parent;
+	if (!chip_np)
+		return ERR_PTR(-EINVAL);
+
+	xlate_flags = 0;
+	*lflags = 0;
+	*dflags = 0;
+
+	ret = of_property_read_u32(chip_np, "#gpio-cells", &tmp);
+	if (ret)
+		return ERR_PTR(ret);
+
+	if (tmp > MAX_PHANDLE_ARGS)
+		return ERR_PTR(-EINVAL);
+
+	gg_data.gpiospec.args_count = tmp;
+	gg_data.gpiospec.np = chip_np;
+	for (i = 0; i < tmp; i++) {
+		ret = of_property_read_u32_index(np, "gpios", i,
+					   &gg_data.gpiospec.args[i]);
+		if (ret)
+			return ERR_PTR(ret);
+	}
+
+	gpiochip_find(&gg_data, of_gpiochip_find_and_xlate);
+	if (!gg_data.out_gpio) {
+		if (np->parent == np)
+			return ERR_PTR(-ENXIO);
+		else
+			return ERR_PTR(-EPROBE_DEFER);
+	}
+
+	if (xlate_flags & OF_GPIO_ACTIVE_LOW)
+		*lflags |= GPIO_ACTIVE_LOW;
+
+	if (of_property_read_string(np, "state", &dir_val)) {
+		pr_warn("%s: GPIO:%d (%s): no hogging state specified, bailing out\n",
+			 __func__, desc_to_gpio(gg_data.out_gpio), np->name);
+		return ERR_PTR(-EINVAL);
+	}
+
+	if (!strcmp(dir_val, "input"))
+		*dflags |= GPIOD_IN;
+	else if (!strcmp(dir_val, "output-low"))
+		*dflags |= GPIOD_OUT_LOW;
+	else if (!strcmp(dir_val, "output-high"))
+		*dflags |= GPIOD_OUT_HIGH;
+	else {
+		pr_warn("%s: GPIO:%d (%s): invalid state '%s' requested, bailing out\n",
+			 __func__, desc_to_gpio(gg_data.out_gpio),
+			np->name, dir_val);
+		return ERR_PTR(-EINVAL);
+	}
+
+	if (name && of_property_read_string(np, "line-name", name))
+		*name = np->name;
+
+	desc = gg_data.out_gpio;
+
+	return desc;
+}
+
+/**
+ * of_gpiochip_scan_hogs - Scan gpio-controller and apply GPIO hog as requested
+ * @chip:	gpio chip to act on
+ *
+ * This is only used by of_gpiochip_add to request/set GPIO initial
+ * configuration.
+ */
+static void of_gpiochip_scan_hogs(struct gpio_chip *chip)
+{
+	struct gpio_desc *desc = NULL;
+	struct device_node *np;
+	const char *name;
+	enum gpio_lookup_flags lflags;
+	enum gpiod_flags dflags;
+
+	for_each_child_of_node(chip->of_node, np) {
+		if (!of_property_read_bool(np, "gpio-hog"))
+			continue;
+
+		desc = of_get_gpio_hog(np, &name, &lflags, &dflags);
+		if (IS_ERR(desc))
+			continue;
+
+		if (gpiod_hog(desc, name, lflags, dflags))
+			continue;
+	}
+}
+
+/**
  * of_gpio_simple_xlate - translate gpio_spec to the GPIO number and flags
  * @gc:		pointer to the gpio_chip structure
  * @np:		device node of the GPIO chip
@@ -302,6 +418,8 @@ void of_gpiochip_add(struct gpio_chip *chip)
 
 	of_gpiochip_add_pin_range(chip);
 	of_node_get(chip->of_node);
+
+	of_gpiochip_scan_hogs(chip);
 }
 
 void of_gpiochip_remove(struct gpio_chip *chip)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 487afe6..d79c265 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -313,6 +313,7 @@ EXPORT_SYMBOL_GPL(gpiochip_add);
 
 /* Forward-declaration */
 static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip);
+static void gpiochip_free_hogs(struct gpio_chip *chip);
 
 /**
  * gpiochip_remove() - unregister a gpio_chip
@@ -331,6 +332,7 @@ void gpiochip_remove(struct gpio_chip *chip)
 
 	gpiochip_irqchip_remove(chip);
 	gpiochip_remove_pin_ranges(chip);
+	gpiochip_free_hogs(chip);
 	of_gpiochip_remove(chip);
 
 	for (id = 0; id < chip->ngpio; id++) {
@@ -864,6 +866,7 @@ static bool __gpiod_free(struct gpio_desc *desc)
 		clear_bit(FLAG_REQUESTED, &desc->flags);
 		clear_bit(FLAG_OPEN_DRAIN, &desc->flags);
 		clear_bit(FLAG_OPEN_SOURCE, &desc->flags);
+		clear_bit(FLAG_IS_HOGGED, &desc->flags);
 		ret = true;
 	}
 
@@ -1836,6 +1839,47 @@ struct gpio_desc *__must_check __gpiod_get_optional(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(__gpiod_get_optional);
 
+
+/**
+ * __gpiod_set_helper - helper function to configure a given GPIO
+ * @desc:	gpio whose value will be assigned
+ * @con_id:	function within the GPIO consumer
+ * @lflags:	gpio_lookup_flags - returned from of_find_gpio() or
+ *		of_get_gpio_hog()
+ * @dflags:	gpiod_flags - optional GPIO initialization flags
+ *
+ * Return 0 on success, -ENOENT if no GPIO has been assigned to the
+ * requested function and/or index, or another IS_ERR() code if an error
+ * occurred while trying to acquire the GPIO.
+ */
+static int __gpiod_set_helper(struct gpio_desc *desc, const char *con_id,
+		unsigned long lflags, enum gpiod_flags dflags)
+{
+	int status;
+
+	if (lflags & GPIO_ACTIVE_LOW)
+		set_bit(FLAG_ACTIVE_LOW, &desc->flags);
+	if (lflags & GPIO_OPEN_DRAIN)
+		set_bit(FLAG_OPEN_DRAIN, &desc->flags);
+	if (lflags & GPIO_OPEN_SOURCE)
+		set_bit(FLAG_OPEN_SOURCE, &desc->flags);
+
+	/* No particular flag request, return here... */
+	if (!(dflags & GPIOD_FLAGS_BIT_DIR_SET)) {
+		pr_debug("no flags found for %s\n", con_id);
+		return 0;
+	}
+
+	/* Process flags */
+	if (dflags & GPIOD_FLAGS_BIT_DIR_OUT)
+		status = gpiod_direction_output(desc,
+					      dflags & GPIOD_FLAGS_BIT_DIR_VAL);
+	else
+		status = gpiod_direction_input(desc);
+
+	return status;
+}
+
 /**
  * gpiod_get_index - obtain a GPIO from a multi-index GPIO function
  * @dev:	GPIO consumer, can be NULL for system-global GPIOs
@@ -1885,30 +1929,12 @@ struct gpio_desc *__must_check __gpiod_get_index(struct device *dev,
 	}
 
 	status = gpiod_request(desc, con_id);
-
 	if (status < 0)
 		return ERR_PTR(status);
 
-	if (lookupflags & GPIO_ACTIVE_LOW)
-		set_bit(FLAG_ACTIVE_LOW, &desc->flags);
-	if (lookupflags & GPIO_OPEN_DRAIN)
-		set_bit(FLAG_OPEN_DRAIN, &desc->flags);
-	if (lookupflags & GPIO_OPEN_SOURCE)
-		set_bit(FLAG_OPEN_SOURCE, &desc->flags);
-
-	/* No particular flag request, return here... */
-	if (!(flags & GPIOD_FLAGS_BIT_DIR_SET))
-		return desc;
-
-	/* Process flags */
-	if (flags & GPIOD_FLAGS_BIT_DIR_OUT)
-		status = gpiod_direction_output(desc,
-					      flags & GPIOD_FLAGS_BIT_DIR_VAL);
-	else
-		status = gpiod_direction_input(desc);
-
+	status = __gpiod_set_helper(desc, con_id, lookupflags, flags);
 	if (status < 0) {
-		dev_dbg(dev, "setup of GPIO %s failed\n", con_id);
+		pr_debug("setup of GPIO %s failed\n", con_id);
 		gpiod_put(desc);
 		return ERR_PTR(status);
 	}
@@ -2002,6 +2028,68 @@ struct gpio_desc *__must_check __gpiod_get_index_optional(struct device *dev,
 EXPORT_SYMBOL_GPL(__gpiod_get_index_optional);
 
 /**
+ * gpiod_hog - Hog the specified GPIO desc given the provided flags
+ * @desc:	gpio whose value will be assigned
+ * @name:	gpio line name
+ * @lflags:	gpio_lookup_flags - returned from of_find_gpio() or
+ *		of_get_gpio_hog()
+ * @dflags:	gpiod_flags - optional GPIO initialization flags
+ *
+ */
+int gpiod_hog(struct gpio_desc *desc, const char *name,
+	      unsigned long lflags, enum gpiod_flags dflags)
+{
+	struct gpio_chip *chip;
+	struct gpio_desc *local_desc;
+	int hwnum;
+	int status;
+
+	chip = gpiod_to_chip(desc);
+	hwnum = gpio_chip_hwgpio(desc);
+
+	local_desc = gpiochip_request_own_desc(chip, hwnum, name);
+	if (IS_ERR(local_desc)) {
+		pr_debug("requesting own GPIO %s failed\n", name);
+		return PTR_ERR(local_desc);
+	}
+
+	status = __gpiod_set_helper(desc, name, lflags, dflags);
+	if (status < 0) {
+		pr_debug("setup of GPIO %s failed\n", name);
+		gpiochip_free_own_desc(desc);
+		return status;
+	}
+
+	/* Mark GPIO as hogged so it can be identified and removed later */
+	set_bit(FLAG_IS_HOGGED, &desc->flags);
+
+	pr_debug("%s: GPIO:%d (%s) as %s%s\n", __func__,
+		desc_to_gpio(desc), name,
+		(dflags&GPIOD_FLAGS_BIT_DIR_OUT) ? "output" : "input",
+		(dflags&GPIOD_FLAGS_BIT_DIR_OUT) ?
+		  (dflags&GPIOD_FLAGS_BIT_DIR_VAL) ? "/high" : "/low":"");
+
+	return 0;
+}
+
+/**
+ * gpiochip_free_hogs - Scan gpio-controller chip and release GPIO hog
+ * @chip:	gpio chip to act on
+ *
+ * This is only used by of_gpiochip_remove to free hogged gpios
+ *
+ */
+static void gpiochip_free_hogs(struct gpio_chip *chip)
+{
+	int id;
+
+	for (id = 0; id < chip->ngpio; id++) {
+		if (test_bit(FLAG_IS_HOGGED, &chip->desc[id].flags))
+			gpiochip_free_own_desc(&chip->desc[id]);
+	}
+}
+
+/**
  * gpiod_put - dispose of a GPIO descriptor
  * @desc:	GPIO descriptor to dispose of
  *
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index e3a5211..c62b489 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -77,6 +77,7 @@ struct gpio_desc {
 #define FLAG_OPEN_DRAIN	7	/* Gpio is open drain type */
 #define FLAG_OPEN_SOURCE 8	/* Gpio is open source type */
 #define FLAG_USED_AS_IRQ 9	/* GPIO is connected to an IRQ */
+#define FLAG_IS_HOGGED	10	/* GPIO is hogged */
 
 #define ID_SHIFT	16	/* add new flags before this one */
 
@@ -88,6 +89,8 @@ struct gpio_desc {
 
 int gpiod_request(struct gpio_desc *desc, const char *label);
 void gpiod_free(struct gpio_desc *desc);
+int gpiod_hog(struct gpio_desc *desc, const char *name,
+		unsigned long lflags, enum gpiod_flags dflags);
 
 /*
  * Return the GPIO number of the passed descriptor relative to its chip
-- 
1.8.5.1

Re: [Patch v5 1/2] gpio: add GPIO hogging mechanism

[Linus Walleij]

On Fri, Dec 19, 2014 at 9:07 PM, Benoit Parrot <bparrot@ti.com> wrote:

> Based on Boris Brezillion's work this is a reworked patch
> of his initial GPIO hogging mechanism.
> This patch provides a way to initially configure specific GPIO
> when the GPIO controller is probed.
>
> The actual DT scanning to collect the GPIO specific data is performed
> as part of gpiochip_add().
>
> The purpose of this is to allow specific GPIOs to be configured
> without any driver specific code.
> This is particularly useful because board design are getting
> increasingly complex and given SoC pins can now have more
> than 10 mux values, a lot of connections are now dependent on
> external IO muxes to switch various modes.
>
> Specific drivers should not necessarily need to be aware of
> what accounts to a specific board implementation. This board level
> "description" should be best kept as part of the dts file.
>
> Signed-off-by: Benoit Parrot <bparrot@ti.com>
> Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>

(...)
>  /**
> + * of_get_gpio_hog() - Get a GPIO hog descriptor, names and flags for GPIO API
> + * @np:                device node to get GPIO from
> + * @name:      GPIO line name
> + * @flags:     a flags pointer to fill in

kerneldoc is unsynchronized, document both flags.

> + *
> + * Returns GPIO descriptor to use with Linux GPIO API, or one of the errno
> + * value on the error condition.
> + */
> +

No newline after kerneldoc. What is lflags and dflags?

> +static struct gpio_desc *of_get_gpio_hog(struct device_node *np,
> +                                 const char **name,
> +                                 enum gpio_lookup_flags *lflags,
> +                                 enum gpiod_flags *dflags)
> +{
> +       struct device_node *chip_np;
> +       enum of_gpio_flags xlate_flags;
> +       struct gpio_desc *desc;
> +       const char *dir_val;
> +       struct gg_data gg_data = {
> +               .flags = &xlate_flags,
> +       };
> +       u32 tmp;
> +       int i, ret;
> +
> +       chip_np = np->parent;
> +       if (!chip_np)
> +               return ERR_PTR(-EINVAL);
> +
> +       xlate_flags = 0;
> +       *lflags = 0;
> +       *dflags = 0;
> +
> +       ret = of_property_read_u32(chip_np, "#gpio-cells", &tmp);
> +       if (ret)
> +               return ERR_PTR(ret);
> +       if (tmp > MAX_PHANDLE_ARGS)
> +               return ERR_PTR(-EINVAL);
> +       gg_data.gpiospec.args_count = tmp;
> +       gg_data.gpiospec.np = chip_np;

Are we duplicating this code elsewhere? No need to
fix now, but space for refactor later on...

> +       for (i = 0; i < tmp; i++) {
> +               ret = of_property_read_u32_index(np, "gpios", i,
> +                                          &gg_data.gpiospec.args[i]);
> +               if (ret)
> +                       return ERR_PTR(ret);
> +       }
> +
> +       gpiochip_find(&gg_data, of_gpiochip_find_and_xlate);
> +       if (!gg_data.out_gpio) {
> +               if (np->parent == np)
> +                       return ERR_PTR(-ENXIO);
> +               else
> +                       return ERR_PTR(-EPROBE_DEFER);

Is this really reasonable, as we know we're instantiating the only
gpio_chip this hog can be on?

> +       }
> +
> +       if (xlate_flags & OF_GPIO_ACTIVE_LOW)
> +               *lflags |= GPIO_ACTIVE_LOW;
> +
> +       if (of_property_read_string(np, "state", &dir_val)) {
> +               pr_warn("%s: GPIO:%d (%s): no hogging state specified, bailing out\n",
> +                        __func__, desc_to_gpio(gg_data.out_gpio), np->name);
> +               return ERR_PTR(-EINVAL);
> +       }
> +
> +       if (!strcmp(dir_val, "input"))
> +               *dflags |= GPIOD_IN;
> +       else if (!strcmp(dir_val, "output-low"))
> +               *dflags |= GPIOD_OUT_LOW;
> +       else if (!strcmp(dir_val, "output-high"))
> +               *dflags |= GPIOD_OUT_HIGH;
> +       else {
> +               pr_warn("%s: GPIO:%d (%s): invalid state '%s' requested, bailing out\n",
> +                        __func__, desc_to_gpio(gg_data.out_gpio),
> +                       np->name, dir_val);
> +               return ERR_PTR(-EINVAL);
> +       }

Hmmmm need to check the bindings first. I'm not yet sure I
like how we represent this.

> +       if (name && of_property_read_string(np, "line-name", name))
> +               *name = np->name;

I quite like this.

> +
> +       desc = gg_data.out_gpio;
> +
> +       return desc;
> +}
> +
> +/**
> + * of_gpiochip_scan_hogs - Scan gpio-controller and apply GPIO hog as requested
> + * @chip:      gpio chip to act on
> + *
> + * This is only used by of_gpiochip_add to request/set GPIO initial
> + * configuration.
> + */
> +static void of_gpiochip_scan_hogs(struct gpio_chip *chip)
> +{
> +       struct gpio_desc *desc = NULL;
> +       struct device_node *np;
> +       const char *name;
> +       enum gpio_lookup_flags lflags;
> +       enum gpiod_flags dflags;
> +
> +       for_each_child_of_node(chip->of_node, np) {
> +               if (!of_property_read_bool(np, "gpio-hog"))
> +                       continue;
> +
> +               desc = of_get_gpio_hog(np, &name, &lflags, &dflags);
> +               if (IS_ERR(desc))
> +                       continue;
> +
> +               if (gpiod_hog(desc, name, lflags, dflags))
> +                       continue;
> +       }
> +}
> +
> +/**
>   * of_gpio_simple_xlate - translate gpio_spec to the GPIO number and flags
>   * @gc:                pointer to the gpio_chip structure
>   * @np:                device node of the GPIO chip
> @@ -302,6 +418,8 @@ void of_gpiochip_add(struct gpio_chip *chip)
>
>         of_gpiochip_add_pin_range(chip);
>         of_node_get(chip->of_node);
> +
> +       of_gpiochip_scan_hogs(chip);
>  }
>
>  void of_gpiochip_remove(struct gpio_chip *chip)
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 487afe6..d79c265 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -313,6 +313,7 @@ EXPORT_SYMBOL_GPL(gpiochip_add);
>
>  /* Forward-declaration */
>  static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip);
> +static void gpiochip_free_hogs(struct gpio_chip *chip);
>
>  /**
>   * gpiochip_remove() - unregister a gpio_chip
> @@ -331,6 +332,7 @@ void gpiochip_remove(struct gpio_chip *chip)
>
>         gpiochip_irqchip_remove(chip);
>         gpiochip_remove_pin_ranges(chip);
> +       gpiochip_free_hogs(chip);
>         of_gpiochip_remove(chip);
>
>         for (id = 0; id < chip->ngpio; id++) {
> @@ -864,6 +866,7 @@ static bool __gpiod_free(struct gpio_desc *desc)
>                 clear_bit(FLAG_REQUESTED, &desc->flags);
>                 clear_bit(FLAG_OPEN_DRAIN, &desc->flags);
>                 clear_bit(FLAG_OPEN_SOURCE, &desc->flags);
> +               clear_bit(FLAG_IS_HOGGED, &desc->flags);
>                 ret = true;
>         }
>
> @@ -1836,6 +1839,47 @@ struct gpio_desc *__must_check __gpiod_get_optional(struct device *dev,
>  }
>  EXPORT_SYMBOL_GPL(__gpiod_get_optional);
>
> +
> +/**
> + * __gpiod_set_helper - helper function to configure a given GPIO
> + * @desc:      gpio whose value will be assigned
> + * @con_id:    function within the GPIO consumer
> + * @lflags:    gpio_lookup_flags - returned from of_find_gpio() or
> + *             of_get_gpio_hog()
> + * @dflags:    gpiod_flags - optional GPIO initialization flags
> + *
> + * Return 0 on success, -ENOENT if no GPIO has been assigned to the
> + * requested function and/or index, or another IS_ERR() code if an error
> + * occurred while trying to acquire the GPIO.
> + */
> +static int __gpiod_set_helper(struct gpio_desc *desc, const char *con_id,
> +               unsigned long lflags, enum gpiod_flags dflags)

This function has no sensible name.

Skip the arbitrary __ prefix and give it a descriptive name
like "gpiod_configure_flags" or something.

> +{
> +       int status;
> +
> +       if (lflags & GPIO_ACTIVE_LOW)
> +               set_bit(FLAG_ACTIVE_LOW, &desc->flags);
> +       if (lflags & GPIO_OPEN_DRAIN)
> +               set_bit(FLAG_OPEN_DRAIN, &desc->flags);
> +       if (lflags & GPIO_OPEN_SOURCE)
> +               set_bit(FLAG_OPEN_SOURCE, &desc->flags);
> +
> +       /* No particular flag request, return here... */
> +       if (!(dflags & GPIOD_FLAGS_BIT_DIR_SET)) {
> +               pr_debug("no flags found for %s\n", con_id);
> +               return 0;
> +       }
> +
> +       /* Process flags */
> +       if (dflags & GPIOD_FLAGS_BIT_DIR_OUT)
> +               status = gpiod_direction_output(desc,
> +                                             dflags & GPIOD_FLAGS_BIT_DIR_VAL);
> +       else
> +               status = gpiod_direction_input(desc);
> +
> +       return status;
> +}
> +
>  /**
>   * gpiod_get_index - obtain a GPIO from a multi-index GPIO function
>   * @dev:       GPIO consumer, can be NULL for system-global GPIOs
> @@ -1885,30 +1929,12 @@ struct gpio_desc *__must_check __gpiod_get_index(struct device *dev,
>         }
>
>         status = gpiod_request(desc, con_id);
> -
>         if (status < 0)
>                 return ERR_PTR(status);
>
> -       if (lookupflags & GPIO_ACTIVE_LOW)
> -               set_bit(FLAG_ACTIVE_LOW, &desc->flags);
> -       if (lookupflags & GPIO_OPEN_DRAIN)
> -               set_bit(FLAG_OPEN_DRAIN, &desc->flags);
> -       if (lookupflags & GPIO_OPEN_SOURCE)
> -               set_bit(FLAG_OPEN_SOURCE, &desc->flags);
> -
> -       /* No particular flag request, return here... */
> -       if (!(flags & GPIOD_FLAGS_BIT_DIR_SET))
> -               return desc;
> -
> -       /* Process flags */
> -       if (flags & GPIOD_FLAGS_BIT_DIR_OUT)
> -               status = gpiod_direction_output(desc,
> -                                             flags & GPIOD_FLAGS_BIT_DIR_VAL);
> -       else
> -               status = gpiod_direction_input(desc);
> -
> +       status = __gpiod_set_helper(desc, con_id, lookupflags, flags);
>         if (status < 0) {
> -               dev_dbg(dev, "setup of GPIO %s failed\n", con_id);
> +               pr_debug("setup of GPIO %s failed\n", con_id);
>                 gpiod_put(desc);
>                 return ERR_PTR(status);
>         }
> @@ -2002,6 +2028,68 @@ struct gpio_desc *__must_check __gpiod_get_index_optional(struct device *dev,
>  EXPORT_SYMBOL_GPL(__gpiod_get_index_optional);
>
>  /**
> + * gpiod_hog - Hog the specified GPIO desc given the provided flags
> + * @desc:      gpio whose value will be assigned
> + * @name:      gpio line name
> + * @lflags:    gpio_lookup_flags - returned from of_find_gpio() or
> + *             of_get_gpio_hog()
> + * @dflags:    gpiod_flags - optional GPIO initialization flags
> + *
> + */
> +int gpiod_hog(struct gpio_desc *desc, const char *name,
> +             unsigned long lflags, enum gpiod_flags dflags)
> +{
> +       struct gpio_chip *chip;
> +       struct gpio_desc *local_desc;
> +       int hwnum;
> +       int status;
> +
> +       chip = gpiod_to_chip(desc);
> +       hwnum = gpio_chip_hwgpio(desc);
> +
> +       local_desc = gpiochip_request_own_desc(chip, hwnum, name);
> +       if (IS_ERR(local_desc)) {
> +               pr_debug("requesting own GPIO %s failed\n", name);
> +               return PTR_ERR(local_desc);
> +       }
> +
> +       status = __gpiod_set_helper(desc, name, lflags, dflags);
> +       if (status < 0) {
> +               pr_debug("setup of GPIO %s failed\n", name);
> +               gpiochip_free_own_desc(desc);
> +               return status;
> +       }
> +
> +       /* Mark GPIO as hogged so it can be identified and removed later */
> +       set_bit(FLAG_IS_HOGGED, &desc->flags);
> +
> +       pr_debug("%s: GPIO:%d (%s) as %s%s\n", __func__,

Convert to pr_info(), as this *should* be printed to console, skip printing
__func__ please.

Modify string to "GPIO line %d (%s) hogged as %s%s\n"

> +               desc_to_gpio(desc), name,
> +               (dflags&GPIOD_FLAGS_BIT_DIR_OUT) ? "output" : "input",
> +               (dflags&GPIOD_FLAGS_BIT_DIR_OUT) ?
> +                 (dflags&GPIOD_FLAGS_BIT_DIR_VAL) ? "/high" : "/low":"");
> +
> +       return 0;
> +}
> +
> +/**
> + * gpiochip_free_hogs - Scan gpio-controller chip and release GPIO hog
> + * @chip:      gpio chip to act on
> + *
> + * This is only used by of_gpiochip_remove to free hogged gpios
> + *

Skip this comment newline.

Overall the changes to gpiolib.c looks very nice.

Yours,
Linus Walleij

[Patch v5 2/2] gpio: Document GPIO hogging mechanism

[Benoit Parrot]

Add GPIO hogging documentation to gpio.txt

Signed-off-by: Benoit Parrot <bparrot@ti.com>
Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
---
 Documentation/devicetree/bindings/gpio/gpio.txt | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/Documentation/devicetree/bindings/gpio/gpio.txt b/Documentation/devicetree/bindings/gpio/gpio.txt
index b9bd1d6..f5d2717 100644
--- a/Documentation/devicetree/bindings/gpio/gpio.txt
+++ b/Documentation/devicetree/bindings/gpio/gpio.txt
@@ -115,6 +115,22 @@ Every GPIO controller node must contain both an empty "gpio-controller"
 property, and a #gpio-cells integer property, which indicates the number of
 cells in a gpio-specifier.
 
+The GPIO chip may contain GPIO hog definitions. GPIO hogging is a mechanism
+providing automatic GPIO request and configuration as part of the
+gpio-controller's driver probe function.
+
+Each GPIO hog definition is represented as a child node of the GPIO controller.
+Required properties:
+- gpio-hog:  A property specifying that this child node represent a GPIO hog.
+- gpios:     Store the GPIO information (id, flags, ...). Shall contain the
+	     number of cells specified in its parent node (GPIO controller
+	     node).
+- state:     A property specifying the direction/value needed. This property
+	     can take the following values: input, output-high, output-low.
+
+Optional properties:
+- line-name: The GPIO label name. If not present the node name is used.
+
 Example of two SOC GPIO banks defined as gpio-controller nodes:
 
 	qe_pio_a: gpio-controller@1400 {
@@ -122,6 +138,13 @@ Example of two SOC GPIO banks defined as gpio-controller nodes:
 		reg = <0x1400 0x18>;
 		gpio-controller;
 		#gpio-cells = <2>;
+
+		line_b {
+			gpio-hog;
+			gpios = <6 0>;
+			state = "output-low";
+			line-name = "foo-bar-gpio";
+		};
 	};
 
 	qe_pio_e: gpio-controller@1460 {
-- 
1.8.5.1

Re: [Patch v5 2/2] gpio: Document GPIO hogging mechanism

[Linus Walleij]

On Fri, Dec 19, 2014 at 9:07 PM, Benoit Parrot <bparrot@ti.com> wrote:

> Add GPIO hogging documentation to gpio.txt
>
> Signed-off-by: Benoit Parrot <bparrot@ti.com>
> Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>

This is starting to look good ...

> +               line_b {
> +                       gpio-hog;
> +                       gpios = <6 0>;
> +                       state = "output-low";

I don't like the state string.

Instead have boolean properties for all states.

line_b {
    gpio-hog;
    gpios = <6 0>;
    output-low;
    line-name = "foo-bar-gpio";
}

Then use of_property_read_bool() in the code to check which
state is to be selected intially. You can check that no mutually
exclusive state are selected, I don't like that an arbitrary string
select the state like that, if we do it that way an enumerator would
be better, I prefer bools.

Yours,
Linus Walleij

Re: [Patch v5 2/2] gpio: Document GPIO hogging mechanism

[Arend van Spriel]

On 01/12/15 11:20, Linus Walleij wrote:
> On Fri, Dec 19, 2014 at 9:07 PM, Benoit Parrot<bparrot@ti.com>  wrote:
>
>> Add GPIO hogging documentation to gpio.txt
>>
>> Signed-off-by: Benoit Parrot<bparrot@ti.com>
>> Reviewed-by: Alexandre Courbot<acourbot@nvidia.com>
>
> This is starting to look good ...
>
>> +               line_b {
>> +                       gpio-hog;
>> +                       gpios =<6 0>;
>> +                       state = "output-low";
>
> I don't like the state string.
>
> Instead have boolean properties for all states.
>
> line_b {
>      gpio-hog;
>      gpios =<6 0>;
>      output-low;
>      line-name = "foo-bar-gpio";
> }
>
> Then use of_property_read_bool() in the code to check which
> state is to be selected intially. You can check that no mutually
> exclusive state are selected, I don't like that an arbitrary string
> select the state like that, if we do it that way an enumerator would
> be better, I prefer bools.

To avoid the mutual exclusive state checking, would it not be more 
straightforward to use numeric enum values defined in boot/dts/include.

Regards,
Arend

> Yours,
> Linus Walleij
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [Patch v5 2/2] gpio: Document GPIO hogging mechanism

[Benoit Parrot]

Linus Walleij <linus.walleij@linaro.org> wrote on Mon [2015-Jan-12 11:20:14 +0100]:
> On Fri, Dec 19, 2014 at 9:07 PM, Benoit Parrot <bparrot@ti.com> wrote:
> 
> > Add GPIO hogging documentation to gpio.txt
> >
> > Signed-off-by: Benoit Parrot <bparrot@ti.com>
> > Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
> 
> This is starting to look good ...
> 
> > +               line_b {
> > +                       gpio-hog;
> > +                       gpios = <6 0>;
> > +                       state = "output-low";
> 
> I don't like the state string.
> 
> Instead have boolean properties for all states.
> 
> line_b {
>     gpio-hog;
>     gpios = <6 0>;
>     output-low;
>     line-name = "foo-bar-gpio";
> }
> 
> Then use of_property_read_bool() in the code to check which
> state is to be selected intially. You can check that no mutually
> exclusive state are selected, I don't like that an arbitrary string
> select the state like that, if we do it that way an enumerator would
> be better, I prefer bools.

I am sorry but that is how it was originally in the first patch.
Alexandre's review comment suggested this method in [1] and [2] (below).

Alexandre, any comments?

[1] http://marc.info/?l=linux-gpio&m=141456662426151&w=2

[2] http://marc.info/?l=linux-gpio&m=141715982424744&w=2

> 
> Yours,
> Linus Walleij
> --
> To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Patch v5 2/2] gpio: Document GPIO hogging mechanism

[Alexandre Courbot]

On Tue, Jan 13, 2015 at 1:39 AM, Benoit Parrot <bparrot@ti.com> wrote:
> Linus Walleij <linus.walleij@linaro.org> wrote on Mon [2015-Jan-12 11:20:14 +0100]:
>> On Fri, Dec 19, 2014 at 9:07 PM, Benoit Parrot <bparrot@ti.com> wrote:
>>
>> > Add GPIO hogging documentation to gpio.txt
>> >
>> > Signed-off-by: Benoit Parrot <bparrot@ti.com>
>> > Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
>>
>> This is starting to look good ...
>>
>> > +               line_b {
>> > +                       gpio-hog;
>> > +                       gpios = <6 0>;
>> > +                       state = "output-low";
>>
>> I don't like the state string.
>>
>> Instead have boolean properties for all states.
>>
>> line_b {
>>     gpio-hog;
>>     gpios = <6 0>;
>>     output-low;
>>     line-name = "foo-bar-gpio";
>> }
>>
>> Then use of_property_read_bool() in the code to check which
>> state is to be selected intially. You can check that no mutually
>> exclusive state are selected, I don't like that an arbitrary string
>> select the state like that, if we do it that way an enumerator would
>> be better, I prefer bools.
>
> I am sorry but that is how it was originally in the first patch.
> Alexandre's review comment suggested this method in [1] and [2] (below).
>
> Alexandre, any comments?
>
> [1] http://marc.info/?l=linux-gpio&m=141456662426151&w=2
>
> [2] http://marc.info/?l=linux-gpio&m=141715982424744&w=2

When Linus and I are in conflict, follow Linus. Arnd's suggestion of
having enums defined in (IIUC) include/dt-bindings/gpio and using them
sounds good to me too and might make everyone happy (no possibility of
conflicting definitions + no strings). Linus, could you comment on it?

Re: [Patch v5 2/2] gpio: Document GPIO hogging mechanism

[Benoit Parrot]

Alexandre Courbot <gnurou@gmail.com> wrote on Tue [2015-Jan-13 06:43:33 +0900]:
> On Tue, Jan 13, 2015 at 1:39 AM, Benoit Parrot <bparrot@ti.com> wrote:
> > Linus Walleij <linus.walleij@linaro.org> wrote on Mon [2015-Jan-12 11:20:14 +0100]:
> >> On Fri, Dec 19, 2014 at 9:07 PM, Benoit Parrot <bparrot@ti.com> wrote:
> >>
> >> > Add GPIO hogging documentation to gpio.txt
> >> >
> >> > Signed-off-by: Benoit Parrot <bparrot@ti.com>
> >> > Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
> >>
> >> This is starting to look good ...
> >>
> >> > +               line_b {
> >> > +                       gpio-hog;
> >> > +                       gpios = <6 0>;
> >> > +                       state = "output-low";
> >>
> >> I don't like the state string.
> >>
> >> Instead have boolean properties for all states.
> >>
> >> line_b {
> >>     gpio-hog;
> >>     gpios = <6 0>;
> >>     output-low;
> >>     line-name = "foo-bar-gpio";
> >> }
> >>
> >> Then use of_property_read_bool() in the code to check which
> >> state is to be selected intially. You can check that no mutually
> >> exclusive state are selected, I don't like that an arbitrary string
> >> select the state like that, if we do it that way an enumerator would
> >> be better, I prefer bools.
> >
> > I am sorry but that is how it was originally in the first patch.
> > Alexandre's review comment suggested this method in [1] and [2] (below).
> >
> > Alexandre, any comments?
> >
> > [1] http://marc.info/?l=linux-gpio&m=141456662426151&w=2
> >
> > [2] http://marc.info/?l=linux-gpio&m=141715982424744&w=2
> 
> When Linus and I are in conflict, follow Linus. Arnd's suggestion of
> having enums defined in (IIUC) include/dt-bindings/gpio and using them
> sounds good to me too and might make everyone happy (no possibility of
> conflicting definitions + no strings). Linus, could you comment on it?

Understood.
Now given Linus prefers the bools that is the direction I am going to follow.

Re: [Patch v5 2/2] gpio: Document GPIO hogging mechanism

[Linus Walleij]

On Mon, Jan 12, 2015 at 10:43 PM, Alexandre Courbot <gnurou-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Tue, Jan 13, 2015 at 1:39 AM, Benoit Parrot <bparrot-l0cyMroinI0@public.gmane.org> wrote:
>> Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote on Mon [2015-Jan-12 11:20:14 +0100]:

>>> line_b {
>>>     gpio-hog;
>>>     gpios = <6 0>;
>>>     output-low;
>>>     line-name = "foo-bar-gpio";
>>> }
>>>
>>> Then use of_property_read_bool() in the code to check which
>>> state is to be selected intially. You can check that no mutually
>>> exclusive state are selected, I don't like that an arbitrary string
>>> select the state like that, if we do it that way an enumerator would
>>> be better, I prefer bools.
>>
>> I am sorry but that is how it was originally in the first patch.
>> Alexandre's review comment suggested this method in [1] and [2] (below).
>>
>> Alexandre, any comments?
>>
>> [1] http://marc.info/?l=linux-gpio&m=141456662426151&w=2
>>
>> [2] http://marc.info/?l=linux-gpio&m=141715982424744&w=2
>
> When Linus and I are in conflict, follow Linus. Arnd's suggestion of
> having enums defined in (IIUC) include/dt-bindings/gpio and using them
> sounds good to me too and might make everyone happy (no possibility of
> conflicting definitions + no strings). Linus, could you comment on it?

I'm fine with bools or enums, just not strings, sorry for the mess.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html