[PATCH 0/2] gpiolib: Improve open-drain and open-source GPIO support

[PATCH 0/2] gpiolib: Improve open-drain and open-source GPIO support

[Laurent Pinchart]

Hello,

These two patches try to improve open-drain and open-source (collectively
referred to as single-ended) GPIO support in the gpiolib core.

The first patch splits GPIO flag parsing and GPIO configuration into two
operation to allow gpiochip drivers to reject GPIO requests with flags that
don't match the device capabilities.

The second patch builds on top of "[PATCH v2] gpio: add DT bindings for
existing consumer flags" (http://marc.info/?l=linux-gpio&m=144378527414721&w=2)
to enable usage of the single-ended flag.

Laurent Pinchart (2):
  gpiolib: Split GPIO flags parsing and GPIO configuration
  gpiolib: Add and use OF_GPIO_SINGLE_ENDED flag

 drivers/gpio/gpiolib-legacy.c |  8 ++---
 drivers/gpio/gpiolib.c        | 70 ++++++++++++++++++++++++++++++++-----------
 include/linux/of_gpio.h       |  1 +
 3 files changed, 58 insertions(+), 21 deletions(-)

-- 
Regards,

Laurent Pinchart

[PATCH 1/2] gpiolib: Split GPIO flags parsing and GPIO configuration

[Laurent Pinchart]

When requesting a GPIO through the legacy or the gpiod_* API the
gpiochip request operation is first called and then the GPIO flags are
parsed and the GPIO is configured. This prevents the gpiochip from
rejecting the request if the flags are not supported by the device.

To fix this split the parse-and-configure operation in two and parse
flags before requesting the GPIO.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpio/gpiolib-legacy.c |  8 +++----
 drivers/gpio/gpiolib.c        | 52 ++++++++++++++++++++++++++++++-------------
 2 files changed, 40 insertions(+), 20 deletions(-)

diff --git a/drivers/gpio/gpiolib-legacy.c b/drivers/gpio/gpiolib-legacy.c
index 8b830996fe02..3a5c7011ad3b 100644
--- a/drivers/gpio/gpiolib-legacy.c
+++ b/drivers/gpio/gpiolib-legacy.c
@@ -28,10 +28,6 @@ int gpio_request_one(unsigned gpio, unsigned long flags, const char *label)
 	if (!desc && gpio_is_valid(gpio))
 		return -EPROBE_DEFER;
 
-	err = gpiod_request(desc, label);
-	if (err)
-		return err;
-
 	if (flags & GPIOF_OPEN_DRAIN)
 		set_bit(FLAG_OPEN_DRAIN, &desc->flags);
 
@@ -41,6 +37,10 @@ int gpio_request_one(unsigned gpio, unsigned long flags, const char *label)
 	if (flags & GPIOF_ACTIVE_LOW)
 		set_bit(FLAG_ACTIVE_LOW, &desc->flags);
 
+	err = gpiod_request(desc, label);
+	if (err)
+		return err;
+
 	if (flags & GPIOF_DIR_IN)
 		err = gpiod_direction_input(desc);
 	else
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index bf4bd1d120c3..de93fc0a380f 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -825,6 +825,14 @@ static int __gpiod_request(struct gpio_desc *desc, const char *label)
 		spin_lock_irqsave(&gpio_lock, flags);
 	}
 done:
+	if (status < 0) {
+		/* Clear flags that might have been set by the caller before
+		 * requesting the GPIO.
+		 */
+		clear_bit(FLAG_ACTIVE_LOW, &desc->flags);
+		clear_bit(FLAG_OPEN_DRAIN, &desc->flags);
+		clear_bit(FLAG_OPEN_SOURCE, &desc->flags);
+	}
 	spin_unlock_irqrestore(&gpio_lock, flags);
 	return status;
 }
@@ -1919,13 +1927,28 @@ struct gpio_desc *__must_check __gpiod_get_optional(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(__gpiod_get_optional);
 
+/**
+ * gpiod_parse_flags - helper function to parse GPIO lookup flags
+ * @desc:	gpio to be setup
+ * @lflags:	gpio_lookup_flags - returned from of_find_gpio() or
+ *		of_get_gpio_hog()
+ *
+ * Set the GPIO descriptor flags based on the given GPIO lookup flags.
+ */
+static void gpiod_parse_flags(struct gpio_desc *desc, unsigned long lflags)
+{
+	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);
+}
 
 /**
  * gpiod_configure_flags - 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
@@ -1933,17 +1956,10 @@ EXPORT_SYMBOL_GPL(__gpiod_get_optional);
  * occurred while trying to acquire the GPIO.
  */
 static int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
-		unsigned long lflags, enum gpiod_flags dflags)
+				 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);
@@ -2010,11 +2026,13 @@ struct gpio_desc *__must_check __gpiod_get_index(struct device *dev,
 		return desc;
 	}
 
+	gpiod_parse_flags(desc, lookupflags);
+
 	status = gpiod_request(desc, con_id);
 	if (status < 0)
 		return ERR_PTR(status);
 
-	status = gpiod_configure_flags(desc, con_id, lookupflags, flags);
+	status = gpiod_configure_flags(desc, con_id, flags);
 	if (status < 0) {
 		dev_dbg(dev, "setup of GPIO %s failed\n", con_id);
 		gpiod_put(desc);
@@ -2068,14 +2086,14 @@ struct gpio_desc *fwnode_get_named_gpiod(struct fwnode_handle *fwnode,
 	if (IS_ERR(desc))
 		return desc;
 
-	ret = gpiod_request(desc, NULL);
-	if (ret)
-		return ERR_PTR(ret);
-
 	/* Only value flag can be set from both DT and ACPI is active_low */
 	if (active_low)
 		set_bit(FLAG_ACTIVE_LOW, &desc->flags);
 
+	ret = gpiod_request(desc, NULL);
+	if (ret)
+		return ERR_PTR(ret);
+
 	return desc;
 }
 EXPORT_SYMBOL_GPL(fwnode_get_named_gpiod);
@@ -2128,6 +2146,8 @@ int gpiod_hog(struct gpio_desc *desc, const char *name,
 	chip = gpiod_to_chip(desc);
 	hwnum = gpio_chip_hwgpio(desc);
 
+	gpiod_parse_flags(desc, lflags);
+
 	local_desc = gpiochip_request_own_desc(chip, hwnum, name);
 	if (IS_ERR(local_desc)) {
 		pr_err("requesting hog GPIO %s (chip %s, offset %d) failed\n",
@@ -2135,7 +2155,7 @@ int gpiod_hog(struct gpio_desc *desc, const char *name,
 		return PTR_ERR(local_desc);
 	}
 
-	status = gpiod_configure_flags(desc, name, lflags, dflags);
+	status = gpiod_configure_flags(desc, name, dflags);
 	if (status < 0) {
 		pr_err("setup of hog GPIO %s (chip %s, offset %d) failed\n",
 		       name, chip->label, hwnum);
-- 
2.4.9

[PATCH 2/2] gpiolib: Add and use OF_GPIO_SINGLE_ENDED flag

[Laurent Pinchart]

The flag matches the DT GPIO_SINGLE_ENDED flag and allows drivers to
parse and use the DT flag to handle single-ended (open-drain or
open-source) GPIOs.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpio/gpiolib.c  | 20 ++++++++++++++++++--
 include/linux/of_gpio.h |  1 +
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index de93fc0a380f..53aa9c80604d 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1709,6 +1709,13 @@ static struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
 	if (of_flags & OF_GPIO_ACTIVE_LOW)
 		*flags |= GPIO_ACTIVE_LOW;
 
+	if (of_flags & OF_GPIO_SINGLE_ENDED) {
+		if (of_flags & OF_GPIO_ACTIVE_LOW)
+			*flags |= GPIO_OPEN_DRAIN;
+		else
+			*flags |= GPIO_OPEN_SOURCE;
+	}
+
 	return desc;
 }
 
@@ -2062,6 +2069,7 @@ struct gpio_desc *fwnode_get_named_gpiod(struct fwnode_handle *fwnode,
 {
 	struct gpio_desc *desc = ERR_PTR(-ENODEV);
 	bool active_low = false;
+	bool single_ended = false;
 	int ret;
 
 	if (!fwnode)
@@ -2072,8 +2080,10 @@ struct gpio_desc *fwnode_get_named_gpiod(struct fwnode_handle *fwnode,
 
 		desc = of_get_named_gpiod_flags(to_of_node(fwnode), propname, 0,
 						&flags);
-		if (!IS_ERR(desc))
+		if (!IS_ERR(desc)) {
 			active_low = flags & OF_GPIO_ACTIVE_LOW;
+			single_ended = flags & OF_GPIO_SINGLE_ENDED;
+		}
 	} else if (is_acpi_node(fwnode)) {
 		struct acpi_gpio_info info;
 
@@ -2086,10 +2096,16 @@ struct gpio_desc *fwnode_get_named_gpiod(struct fwnode_handle *fwnode,
 	if (IS_ERR(desc))
 		return desc;
 
-	/* Only value flag can be set from both DT and ACPI is active_low */
 	if (active_low)
 		set_bit(FLAG_ACTIVE_LOW, &desc->flags);
 
+	if (single_ended) {
+		if (active_low)
+			set_bit(FLAG_OPEN_DRAIN, &desc->flags);
+		else
+			set_bit(FLAG_OPEN_SOURCE, &desc->flags);
+	}
+
 	ret = gpiod_request(desc, NULL);
 	if (ret)
 		return ERR_PTR(ret);
diff --git a/include/linux/of_gpio.h b/include/linux/of_gpio.h
index 69dbe312b11b..e8d662a1b6d1 100644
--- a/include/linux/of_gpio.h
+++ b/include/linux/of_gpio.h
@@ -29,6 +29,7 @@ struct device_node;
  */
 enum of_gpio_flags {
 	OF_GPIO_ACTIVE_LOW = 0x1,
+	OF_GPIO_SINGLE_ENDED = 0x2,
 };
 
 #ifdef CONFIG_OF_GPIO
-- 
2.4.9

Re: [PATCH 1/2] gpiolib: Split GPIO flags parsing and GPIO configuration

[Linus Walleij]

On Mon, Oct 12, 2015 at 11:20 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:

> When requesting a GPIO through the legacy or the gpiod_* API the
> gpiochip request operation is first called and then the GPIO flags are
> parsed and the GPIO is configured. This prevents the gpiochip from
> rejecting the request if the flags are not supported by the device.
>
> To fix this split the parse-and-configure operation in two and parse
> flags before requesting the GPIO.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Very elegant, this is exactly how we need things to happen.
Patch applied.

Yours,
Linus Walleij

Re: [PATCH 2/2] gpiolib: Add and use OF_GPIO_SINGLE_ENDED flag

[Linus Walleij]

On Mon, Oct 12, 2015 at 11:20 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:

> The flag matches the DT GPIO_SINGLE_ENDED flag and allows drivers to
> parse and use the DT flag to handle single-ended (open-drain or
> open-source) GPIOs.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Patch applied!

I feel a bit guilty for fixing the OF docs (with help from Nikolaus Schaller)
and then just leaving it open as an exercise for someone else to implement
it in gpiolib ... hats of for immediately grasping this and implementing it
exactly as it should be!

All of a sudden I now have an influx of GPIO drivers supporting open drain.
I wonder why it happens right now, probably sun spot activity.

Yours,
Linus Walleij