[RFC PATCH] gpio: add GPIO hogging mechanism

[RFC PATCH] gpio: add GPIO hogging mechanism

[Boris BREZILLON]

Hello,

This a proposal for the GPIO hog concept described by Linus (Walleij).

I know there were work in progress on this subject (Jiri and Ben at least:
https://www.mail-archive.com/linux-gpio@vger.kernel.org/msg00864.html),
so don't hesitate to point out if something similar has already been posted.

Best Regards,

Boris 

Boris BREZILLON (1):
  gpio: add GPIO hogging mechanism

 Documentation/devicetree/bindings/gpio/gpio.txt |   47 ++++++++
 drivers/base/Makefile                           |    1 +
 drivers/base/dd.c                               |    5 +
 drivers/base/gpio.c                             |   59 ++++++++++
 drivers/gpio/devres.c                           |   39 +++++++
 drivers/gpio/gpiolib-of.c                       |  134 +++++++++++++++++++++++
 drivers/gpio/gpiolib.c                          |  103 +++++++++++++++++
 include/linux/device.h                          |    5 +
 include/linux/gpio/consumer.h                   |   25 +++++
 include/linux/gpio/devinfo.h                    |   38 +++++++
 include/linux/of_gpio.h                         |   19 ++++
 11 files changed, 475 insertions(+)
 create mode 100644 drivers/base/gpio.c
 create mode 100644 include/linux/gpio/devinfo.h

-- 
1.7.9.5

[RFC PATCH] gpio: add GPIO hogging mechanism

[Boris BREZILLON]

GPIO hogging is a way to request and configure specific GPIO without
explicitly requesting it in the device driver.

The request and configuration procedure is handled in the core device
driver code before the driver probe function is called.

It allows specific GPIOs to be configured without any driver specific code.

Particularly usefull when a external device is connected to a bus and the
bus connections depends on an external switch controlled by a GPIO pin.
Or when some GPIOs have to be exported to sysfs without any userspace
intervention.

Signed-off-by: Boris BREZILLON <b.brezillon-ZNYIgs0QAGpBDgjK7y7TUQ@public.gmane.org>
---
 Documentation/devicetree/bindings/gpio/gpio.txt |   47 ++++++++
 drivers/base/Makefile                           |    1 +
 drivers/base/dd.c                               |    5 +
 drivers/base/gpio.c                             |   59 ++++++++++
 drivers/gpio/devres.c                           |   39 +++++++
 drivers/gpio/gpiolib-of.c                       |  134 +++++++++++++++++++++++
 drivers/gpio/gpiolib.c                          |  103 +++++++++++++++++
 include/linux/device.h                          |    5 +
 include/linux/gpio/consumer.h                   |   25 +++++
 include/linux/gpio/devinfo.h                    |   38 +++++++
 include/linux/of_gpio.h                         |   19 ++++
 11 files changed, 475 insertions(+)
 create mode 100644 drivers/base/gpio.c
 create mode 100644 include/linux/gpio/devinfo.h

diff --git a/Documentation/devicetree/bindings/gpio/gpio.txt b/Documentation/devicetree/bindings/gpio/gpio.txt
index 0c85bb6..e2295e3 100644
--- a/Documentation/devicetree/bindings/gpio/gpio.txt
+++ b/Documentation/devicetree/bindings/gpio/gpio.txt
@@ -59,6 +59,35 @@ and empty GPIO flags as accepted by the "qe_pio_e" gpio-controller.
 Every GPIO controller node must both an empty "gpio-controller"
 property, and have #gpio-cells contain the size of the gpio-specifier.
 
+It might contain GPIO hog definitions. GPIO hogging is a mechanism providing
+automatic GPIO request and configuration before the device is passed to its
+driver probe function (the same mechanism is used for pinctrl pin config).
+
+Each GPIO hog definition is represented as a child node of the GPIO controller.
+Required properties:
+- gpio: store the gpio informations (id, flags, ...). Shall contain the
+  number of cells specified in its parent node (GPIO controller node).
+- hog-as-input or hog-as-output: a boolean property specifying the GPIO
+  direction.
+- hog-init-high or hog-init-low: a boolean property specifying the GPIO
+  value in case the GPIO is hogged as output.
+
+Optional properties:
+- open-drain-line: GPIO is configured as an open drain pin.
+- open-source-line: GPIO is configured as an open source pin.
+- export-line or export-line-fixed: the GPIO is exported to userspace via
+  sysfs.
+- line-name: the GPIO label name.
+
+A GPIO consumer can request GPIO hogs using the "gpio-hogs" property, which
+encodes a list of requested GPIO hogs.
+If the "gpio-hog-names" property is specified and the GPIO hog export the GPIO
+to sysfs, links to the GPIO directory are created in the consumer sysfs device
+directory.
+
+If you just want to export a GPIO to sysfs without assigning it to a specific
+device, you can specify a "gpio-hogs" property in the GPIO controller node.
+
 Example of two SOC GPIO banks defined as gpio-controller nodes:
 
 	qe_pio_a: gpio-controller@1400 {
@@ -66,6 +95,19 @@ Example of two SOC GPIO banks defined as gpio-controller nodes:
 		compatible = "fsl,qe-pario-bank-a", "fsl,qe-pario-bank";
 		reg = <0x1400 0x18>;
 		gpio-controller;
+		gpio-hogs = <&exported_gpio>;
+
+		line_a: line-a {
+			gpio = <5 0>;
+			hog-as-input;
+			line-name = "line-a";
+		};
+
+		exported_gpio: exported-gpio {
+			gpio = <6 0>;
+			hog-as-output;
+			line-name = "exported-gpio";
+		};
 	};
 
 	qe_pio_e: gpio-controller@1460 {
@@ -75,6 +117,11 @@ Example of two SOC GPIO banks defined as gpio-controller nodes:
 		gpio-controller;
 	};
 
+	pio_consumer {
+		gpio-hogs = <&line_a>;
+		gpio-hog-names = "my-line-a";
+	};
+
 2.1) gpio- and pin-controller interaction
 -----------------------------------------
 
diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index 94e8a80..17940c3 100644
--- a/drivers/base/Makefile
+++ b/drivers/base/Makefile
@@ -22,6 +22,7 @@ obj-$(CONFIG_SYS_HYPERVISOR) += hypervisor.o
 obj-$(CONFIG_REGMAP)	+= regmap/
 obj-$(CONFIG_SOC_BUS) += soc.o
 obj-$(CONFIG_PINCTRL) += pinctrl.o
+obj-$(CONFIG_GPIOLIB) += gpio.o
 
 ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG
 
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 0605176..cfeceea 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -25,6 +25,7 @@
 #include <linux/async.h>
 #include <linux/pm_runtime.h>
 #include <linux/pinctrl/devinfo.h>
+#include <linux/gpio/devinfo.h>
 
 #include "base.h"
 #include "power/power.h"
@@ -278,6 +279,10 @@ static int really_probe(struct device *dev, struct device_driver *drv)
 	if (ret)
 		goto probe_failed;
 
+	ret = gpio_get_hogs(dev);
+	if (ret)
+		goto probe_failed;
+
 	if (driver_sysfs_add(dev)) {
 		printk(KERN_ERR "%s: driver_sysfs_add(%s) failed\n",
 			__func__, dev_name(dev));
diff --git a/drivers/base/gpio.c b/drivers/base/gpio.c
new file mode 100644
index 0000000..565c31c
--- /dev/null
+++ b/drivers/base/gpio.c
@@ -0,0 +1,59 @@
+/*
+ * Driver core interface to the GPIO subsystem.
+ *
+ * Copyright (C) Boris BREZILLON <b.brezillon-ZNYIgs0QAGpBDgjK7y7TUQ@public.gmane.org>
+ *
+ * Author: Boris BREZILLON <b.brezillon-ZNYIgs0QAGpBDgjK7y7TUQ@public.gmane.org>
+ *
+ * License terms: GNU General Public License (GPL) version 2
+ */
+
+#include <linux/device.h>
+#include <linux/gpio/devinfo.h>
+#include <linux/gpio/consumer.h>
+#include <linux/slab.h>
+
+/**
+ * gpio_get_hogs() - called by the device core before probe
+ * @dev: the device that is just about to probe
+ */
+int gpio_get_hogs(struct device *dev)
+{
+	struct gpio_desc *desc;
+	int ret;
+	int count;
+	int i = 0;
+
+	count = gpio_hog_count(dev);
+	if (!count)
+		return 0;
+
+	dev->gpios = devm_kzalloc(dev,
+				  sizeof(*(dev->gpios)) +
+				  (count * sizeof(struct gpio_desc *)),
+				  GFP_KERNEL);
+	if (!dev->gpios)
+		return -ENOMEM;
+
+	dev->gpios->ngpios = count;
+	for (i = 0; i < count; i++) {
+		desc = devm_gpiod_get_hog_index(dev, i);
+		if (IS_ERR(desc)) {
+			ret = PTR_ERR(desc);
+			if (ret == -EPROBE_DEFER)
+				goto cleanup;
+			desc = NULL;
+		}
+		dev->gpios->gpios[i] = desc;
+	}
+
+	return 0;
+
+cleanup:
+	for (; i > 0; i--)
+		devm_gpiod_put(dev, dev->gpios->gpios[i - 1]);
+	devm_kfree(dev, dev->gpios);
+	dev->gpios = NULL;
+
+	return ret;
+}
diff --git a/drivers/gpio/devres.c b/drivers/gpio/devres.c
index 307464f..ad0ebc5 100644
--- a/drivers/gpio/devres.c
+++ b/drivers/gpio/devres.c
@@ -87,6 +87,39 @@ struct gpio_desc *__must_check devm_gpiod_get_index(struct device *dev,
 EXPORT_SYMBOL(devm_gpiod_get_index);
 
 /**
+ * devm_gpiod_get_hog_index - Resource-managed gpiod_get_hog_index()
+ * @dev:	GPIO consumer
+ * @idx:	index of the GPIO to obtain in the consumer
+ *
+ * Managed gpiod_get_hog_index(). GPIO descriptors returned from this function
+ * are automatically disposed on driver detach. See gpiod_get_index() for
+ * detailed information about behavior and return values.
+ */
+struct gpio_desc *__must_check devm_gpiod_get_hog_index(struct device *dev,
+							unsigned int idx)
+{
+	struct gpio_desc **dr;
+	struct gpio_desc *desc;
+
+	dr = devres_alloc(devm_gpiod_release, sizeof(struct gpiod_desc *),
+			  GFP_KERNEL);
+	if (!dr)
+		return ERR_PTR(-ENOMEM);
+
+	desc = gpiod_get_hog_index(dev, idx);
+	if (IS_ERR(desc)) {
+		devres_free(dr);
+		return desc;
+	}
+
+	*dr = desc;
+	devres_add(dev, dr);
+
+	return desc;
+}
+EXPORT_SYMBOL(devm_gpiod_get_hog_index);
+
+/**
  * devm_gpiod_put - Resource-managed gpiod_put()
  * @desc:	GPIO descriptor to dispose of
  *
@@ -142,6 +175,9 @@ int devm_gpio_request(struct device *dev, unsigned gpio, const char *label)
 	if (!dr)
 		return -ENOMEM;
 
+	if (gpio_is_hogged(dev, gpio))
+		return 0;
+
 	rc = gpio_request(gpio, label);
 	if (rc) {
 		devres_free(dr);
@@ -172,6 +208,9 @@ int devm_gpio_request_one(struct device *dev, unsigned gpio,
 	if (!dr)
 		return -ENOMEM;
 
+	if (gpio_is_hogged(dev, gpio))
+		return 0;
+
 	rc = gpio_request_one(gpio, flags, label);
 	if (rc) {
 		devres_free(dr);
diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index e0a98f5..8e9fbef 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -96,6 +96,140 @@ struct gpio_desc *of_get_named_gpiod_flags(struct device_node *np,
 EXPORT_SYMBOL(of_get_named_gpiod_flags);
 
 /**
+ * of_gpio_hog_count() - Count a GPIO hogs on a specific device node
+ * @np:		device node to get GPIO from
+ *
+ * Returns the number of GPIO hogs requested by this device node.
+ */
+int of_gpio_hog_count(struct device_node *np)
+{
+	int size;
+
+	if (!of_get_property(np, "gpio-hogs", &size))
+		return 0;
+
+	return size / sizeof(phandle);
+}
+EXPORT_SYMBOL(of_gpio_hog_count);
+
+/**
+ * of_get_gpio_hog() - Get a GPIO hog descriptor, names and flags for GPIO API
+ * @np:		device node to get GPIO from
+ * @index:	index of the GPIO
+ * @name:	GPIO line name
+ * @lnk_name	GPIO link name (for sysfs link)
+ * @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.
+ */
+struct gpio_desc *of_get_gpio_hog(struct device_node *np, int index,
+				  const char **name, const char **lnk_name,
+				  unsigned long *flags)
+{
+	struct device_node *hog_np, *chip_np;
+	enum of_gpio_flags xlate_flags;
+	unsigned long req_flags = 0;
+	struct gpio_desc *desc;
+	struct gg_data gg_data = {
+		.flags = &xlate_flags,
+		.out_gpio = NULL,
+	};
+	u32 tmp;
+	int i;
+	int ret;
+
+	hog_np = of_parse_phandle(np, "gpio-hogs", index);
+	if (!hog_np)
+		return ERR_PTR(-EINVAL);
+
+	chip_np = hog_np->parent;
+	if (!chip_np) {
+		desc = ERR_PTR(-EINVAL);
+		goto out;
+	}
+
+	ret = of_property_read_u32(chip_np, "#gpio-cells", &tmp);
+	if (ret) {
+		desc = ERR_PTR(ret);
+		goto out;
+	}
+
+	if (tmp > MAX_PHANDLE_ARGS) {
+		desc = ERR_PTR(-EINVAL);
+		goto out;
+	}
+
+	gg_data.gpiospec.args_count = tmp;
+	gg_data.gpiospec.np = chip_np;
+	for (i = 0; i < tmp; i++) {
+		ret = of_property_read_u32(hog_np, "gpio",
+					   &gg_data.gpiospec.args[i]);
+		if (ret) {
+			desc = ERR_PTR(ret);
+			goto out;
+		}
+	}
+
+	gpiochip_find(&gg_data, of_gpiochip_find_and_xlate);
+	if (!gg_data.out_gpio) {
+		if (hog_np->parent == np)
+			desc = ERR_PTR(-ENXIO);
+		else
+			desc = ERR_PTR(-EPROBE_DEFER);
+
+		goto out;
+	}
+
+	if (xlate_flags & OF_GPIO_ACTIVE_LOW)
+		req_flags |= GPIOF_ACTIVE_LOW;
+
+	if (of_property_read_bool(hog_np, "hog-as-input")) {
+		req_flags |= GPIOF_DIR_IN;
+	} else if (of_property_read_bool(hog_np, "hog-as-output")) {
+		if (of_property_read_bool(hog_np, "hog-init-high")) {
+			req_flags |= GPIOF_INIT_HIGH;
+		} else if (!of_property_read_bool(hog_np, "hog-init-low")) {
+			desc = ERR_PTR(-EINVAL);
+			goto out;
+		}
+	} else {
+		desc = ERR_PTR(-EINVAL);
+		goto out;
+	}
+
+	if (of_property_read_bool(hog_np, "open-drain-line"))
+		req_flags |= GPIOF_OPEN_DRAIN;
+
+	if (of_property_read_bool(hog_np, "open-source-line"))
+		req_flags |= GPIOF_OPEN_DRAIN;
+
+	if (of_property_read_bool(hog_np, "export-line"))
+		req_flags |= GPIOF_EXPORT | GPIOF_EXPORT_CHANGEABLE;
+	else if (of_property_read_bool(hog_np, "export-line-fixed"))
+		req_flags |= GPIOF_EXPORT;
+
+	if (name && of_property_read_string(hog_np, "line-name", name))
+		*name = hog_np->name;
+
+	if (lnk_name) {
+		*lnk_name = NULL;
+		of_property_read_string_index(np, "gpio-hog-names", index, lnk_name);
+	}
+
+	if (flags)
+		*flags = req_flags;
+
+	desc = gg_data.out_gpio;
+
+out:
+	of_node_put(hog_np);
+
+	return desc;
+}
+EXPORT_SYMBOL(of_get_gpio_hog);
+
+/**
  * 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
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 85f772c..147b503 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1166,6 +1166,8 @@ int gpiochip_add(struct gpio_chip *chip)
 	int		status = 0;
 	unsigned	id;
 	int		base = chip->base;
+	struct gpio_desc	*desc;
+	int 		i;
 
 	if ((!gpio_is_valid(base) || !gpio_is_valid(base + chip->ngpio - 1))
 			&& base >= 0) {
@@ -1214,6 +1216,17 @@ int gpiochip_add(struct gpio_chip *chip)
 
 	of_gpiochip_add(chip);
 
+	/* Instantiate missing GPIO hogs */
+	if (chip->dev->gpios) {
+		for (i = 0; i < chip->dev->gpios->ngpios; i++) {
+			if (chip->dev->gpios->gpios[i])
+				continue;
+			desc = devm_gpiod_get_hog_index(chip->dev, i);
+			if (!IS_ERR(desc))
+				chip->dev->gpios->gpios[i] = desc;
+		}
+	}
+
 	if (status)
 		goto fail;
 
@@ -2421,6 +2434,7 @@ struct gpio_desc *__must_check gpiod_get_index(struct device *dev,
 	struct gpio_desc *desc = NULL;
 	int status;
 	enum gpio_lookup_flags flags = 0;
+	int i;
 
 	dev_dbg(dev, "GPIO lookup for consumer %s\n", con_id);
 
@@ -2451,6 +2465,13 @@ struct gpio_desc *__must_check gpiod_get_index(struct device *dev,
 		return desc;
 	}
 
+	if (dev->gpios) {
+		for (i = 0; i < dev->gpios->ngpios; i++) {
+			if (dev->gpios->gpios[i] == desc)
+				return desc;
+		}
+	}
+
 	status = gpiod_request(desc, con_id);
 
 	if (status < 0)
@@ -2468,6 +2489,88 @@ struct gpio_desc *__must_check gpiod_get_index(struct device *dev,
 EXPORT_SYMBOL_GPL(gpiod_get_index);
 
 /**
+ * gpiod_get_hog_index - obtain a GPIO from a multi-index GPIO hog function
+ * @dev:	GPIO consumer
+ * @idx:	index of the GPIO to obtain in the consumer
+ *
+ * This should only be used by core device driver code to request GPIO hogs
+ * before probing a device.
+ *
+ * Return a valid GPIO descriptor, or an IS_ERR() condition in case of error.
+ */
+struct gpio_desc *__must_check gpiod_get_hog_index(struct device *dev,
+						   unsigned int idx)
+{
+	struct gpio_desc *desc = NULL;
+	int status;
+	unsigned long flags;
+	const char *name, *lnk_name;
+
+	/* Using device tree? */
+	if (IS_ENABLED(CONFIG_OF) && dev && dev->of_node) {
+		dev_dbg(dev, "using device tree for GPIO hog retrieval\n");
+		desc = of_get_gpio_hog(dev->of_node, idx, &name, &lnk_name,
+				       &flags);
+	}
+
+	if (!desc)
+		return ERR_PTR(-ENOTSUPP);
+	else if (IS_ERR(desc))
+		return desc;
+
+	status = gpio_request_one(desc_to_gpio(desc), flags, name);
+	if (status)
+		return ERR_PTR(status);
+
+	if (!lnk_name || !(flags & GPIOF_EXPORT))
+		return desc;
+
+	status = gpiod_export_link(dev, lnk_name, desc);
+	if (status) {
+		gpiod_free(desc);
+		return ERR_PTR(status);
+	}
+
+	return desc;
+}
+EXPORT_SYMBOL_GPL(gpiod_get_hog_index);
+
+static bool gpiod_is_hogged(struct device *dev, struct gpio_desc *desc)
+{
+	int i;
+
+	if (dev->gpios) {
+		for (i = 0; i < dev->gpios->ngpios; i++) {
+			if (dev->gpios->gpios[i] == desc)
+				return true;
+		}
+	}
+
+	return false;
+}
+
+bool gpio_is_hogged(struct device *dev, unsigned gpio)
+{
+	return gpiod_is_hogged(dev, gpio_to_desc(gpio));
+}
+EXPORT_SYMBOL_GPL(gpio_is_hogged);
+
+/**
+ * gpio_hog_count - count number of GPIO hogs requested by a specific device
+ * @dev:	GPIO consumer
+ *
+ * Return the number of GPIO hogs.
+ */
+int gpio_hog_count(struct device *dev)
+{
+	/* Using device tree? */
+	if (IS_ENABLED(CONFIG_OF) && dev && dev->of_node)
+		return of_gpio_hog_count(dev->of_node);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(gpio_hog_count);
+
+/**
  * gpiod_put - dispose of a GPIO descriptor
  * @desc:	GPIO descriptor to dispose of
  *
diff --git a/include/linux/device.h b/include/linux/device.h
index 952b010..df9ea62 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -22,6 +22,7 @@
 #include <linux/types.h>
 #include <linux/mutex.h>
 #include <linux/pinctrl/devinfo.h>
+#include <linux/gpio/devinfo.h>
 #include <linux/pm.h>
 #include <linux/atomic.h>
 #include <linux/ratelimit.h>
@@ -744,6 +745,10 @@ struct device {
 	struct dev_pin_info	*pins;
 #endif
 
+#ifdef CONFIG_GPIOLIB
+	struct dev_gpio_info	*gpios;
+#endif
+
 #ifdef CONFIG_NUMA
 	int		numa_node;	/* NUMA node this device is close to */
 #endif
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index 4d34dbb..69d1e99 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -24,6 +24,10 @@ struct gpio_desc *__must_check gpiod_get(struct device *dev,
 struct gpio_desc *__must_check gpiod_get_index(struct device *dev,
 					       const char *con_id,
 					       unsigned int idx);
+struct gpio_desc *__must_check gpiod_get_hog_index(struct device *dev,
+						   unsigned int idx);
+int gpio_hog_count(struct device *dev);
+bool gpio_is_hogged(struct device *dev, unsigned gpio);
 void gpiod_put(struct gpio_desc *desc);
 
 struct gpio_desc *__must_check devm_gpiod_get(struct device *dev,
@@ -31,6 +35,8 @@ struct gpio_desc *__must_check devm_gpiod_get(struct device *dev,
 struct gpio_desc *__must_check devm_gpiod_get_index(struct device *dev,
 						    const char *con_id,
 						    unsigned int idx);
+struct gpio_desc *__must_check devm_gpiod_get_hog_index(struct device *dev,
+							unsigned int idx);
 void devm_gpiod_put(struct device *dev, struct gpio_desc *desc);
 
 int gpiod_get_direction(const struct gpio_desc *desc);
@@ -74,6 +80,19 @@ static inline struct gpio_desc *__must_check gpiod_get_index(struct device *dev,
 {
 	return ERR_PTR(-ENOSYS);
 }
+static inline struct gpio_desc *__must_check
+gpiod_get_hog_index(struct device *dev, unsigned int idx)
+{
+	return ERR_PTR(-ENOSYS);
+}
+static inline int gpio_hog_count(struct device *dev)
+{
+	return 0;
+}
+static inline bool gpio_is_hogged(struct device *dev, unsigned gpio)
+{
+	return false;
+}
 static inline void gpiod_put(struct gpio_desc *desc)
 {
 	might_sleep();
@@ -94,6 +113,12 @@ struct gpio_desc *__must_check devm_gpiod_get_index(struct device *dev,
 {
 	return ERR_PTR(-ENOSYS);
 }
+static inline
+struct gpio_desc *__must_check devm_gpiod_get_hog_index(struct device *dev,
+							unsigned int idx)
+{
+	return ERR_PTR(-ENOSYS);
+}
 static inline void devm_gpiod_put(struct device *dev, struct gpio_desc *desc)
 {
 	might_sleep();
diff --git a/include/linux/gpio/devinfo.h b/include/linux/gpio/devinfo.h
new file mode 100644
index 0000000..010c59b
--- /dev/null
+++ b/include/linux/gpio/devinfo.h
@@ -0,0 +1,38 @@
+/*
+ * Per-device information from the GPIO system.
+ * This is the stuff that get included into the device
+ * core.
+ *
+ * Copyright (C) Boris BREZILLON <b.brezillon-ZNYIgs0QAGpBDgjK7y7TUQ@public.gmane.org>
+ *
+ * Author: Boris BREZILLON <b.brezillon-ZNYIgs0QAGpBDgjK7y7TUQ@public.gmane.org>
+ *
+ * License terms: GNU General Public License (GPL) version 2
+ */
+
+#ifndef GPIO_DEVINFO_H
+#define GPIO_DEVINFO_H
+
+#ifdef CONFIG_GPIOLIB
+
+/* The device core acts as a consumer toward GPIO */
+#include <linux/gpio/consumer.h>
+
+struct dev_gpio_info {
+	int ngpios;
+	struct gpio_desc *gpios[0];
+};
+
+extern int gpio_get_hogs(struct device *dev);
+
+#else
+
+/* Stubs if we're not using GPIO hogs */
+
+static inline int gpio_get_hogs(struct device *dev)
+{
+	return 0;
+}
+
+#endif /* CONFIG_GPIOLIB */
+#endif /* GPIO_DEVINFO_H */
diff --git a/include/linux/of_gpio.h b/include/linux/of_gpio.h
index f14123a..43f7f86 100644
--- a/include/linux/of_gpio.h
+++ b/include/linux/of_gpio.h
@@ -60,6 +60,11 @@ extern int of_gpio_simple_xlate(struct gpio_chip *gc,
 				const struct of_phandle_args *gpiospec,
 				u32 *flags);
 
+extern int of_gpio_hog_count(struct device_node *np);
+extern struct gpio_desc *of_get_gpio_hog(struct device_node *np, int index,
+					 const char **name,
+					 const char **lnk_name,
+					 unsigned long *flags);
 #else /* CONFIG_OF_GPIO */
 
 /* Drivers may not strictly depend on the GPIO support, so let them link. */
@@ -79,6 +84,20 @@ static inline int of_gpio_simple_xlate(struct gpio_chip *gc,
 static inline void of_gpiochip_add(struct gpio_chip *gc) { }
 static inline void of_gpiochip_remove(struct gpio_chip *gc) { }
 
+static inline int of_gpio_hog_count(struct device_node *np)
+{
+	return 0;
+}
+
+static inline struct gpio_desc *of_get_gpio_hog(struct device_node *np,
+						int index,
+						const char **name,
+						const char **lnk_name,
+						unsigned long *flags)
+{
+	return ERR_PTR(-ENOSYS);
+}
+
 #endif /* CONFIG_OF_GPIO */
 
 static inline int of_get_named_gpio_flags(struct device_node *np,
-- 
1.7.9.5

--
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

Re: [RFC PATCH] gpio: add GPIO hogging mechanism

[Greg Kroah-Hartman]

On Thu, Dec 19, 2013 at 03:34:31PM +0100, Boris BREZILLON wrote:
> GPIO hogging is a way to request and configure specific GPIO without
> explicitly requesting it in the device driver.
> 
> The request and configuration procedure is handled in the core device
> driver code before the driver probe function is called.
> 
> It allows specific GPIOs to be configured without any driver specific code.
> 
> Particularly usefull when a external device is connected to a bus and the
> bus connections depends on an external switch controlled by a GPIO pin.
> Or when some GPIOs have to be exported to sysfs without any userspace
> intervention.
> 
> Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>
> ---
>  Documentation/devicetree/bindings/gpio/gpio.txt |   47 ++++++++
>  drivers/base/Makefile                           |    1 +
>  drivers/base/dd.c                               |    5 +
>  drivers/base/gpio.c                             |   59 ++++++++++

I don't understand what makes GPIO's "special" enough to get included in
the driver core like this, and called for each and every device that is
added to the system.

What's wrong with the generic device callbacks/notifiers we already
have?  Why does this need to be in the driver core?  And what exactly
are you doing all of this for in the first place?

greg k-h

Re: [RFC PATCH] gpio: add GPIO hogging mechanism

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 735 bytes --]

On Thu, Dec 19, 2013 at 08:41:09AM -0800, Greg Kroah-Hartman wrote:
> On Thu, Dec 19, 2013 at 03:34:31PM +0100, Boris BREZILLON wrote:
> > GPIO hogging is a way to request and configure specific GPIO without
> > explicitly requesting it in the device driver.
> > 
> > The request and configuration procedure is handled in the core device
> > driver code before the driver probe function is called.
> > 
> > It allows specific GPIOs to be configured without any driver specific code.
> > 
> > Particularly usefull when a external device is connected to a bus and the
> > bus connections depends on an external switch controlled by a GPIO pin.

for external switches, you probably need a pinctrl-gpio driver.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC PATCH] gpio: add GPIO hogging mechanism

[boris brezillon]

Hello Felipe,

On 19/12/2013 17:47, Felipe Balbi wrote:
> On Thu, Dec 19, 2013 at 08:41:09AM -0800, Greg Kroah-Hartman wrote:
>> On Thu, Dec 19, 2013 at 03:34:31PM +0100, Boris BREZILLON wrote:
>>> GPIO hogging is a way to request and configure specific GPIO without
>>> explicitly requesting it in the device driver.
>>>
>>> The request and configuration procedure is handled in the core device
>>> driver code before the driver probe function is called.
>>>
>>> It allows specific GPIOs to be configured without any driver specific code.
>>>
>>> Particularly usefull when a external device is connected to a bus and the
>>> bus connections depends on an external switch controlled by a GPIO pin.
> for external switches, you probably need a pinctrl-gpio driver.
>
Do you mean using pinctrl pinconf to configure the PIN as output-high or
output-low ?

This was my first proposal
(see https://www.mail-archive.com/devicetree@vger.kernel.org/msg05829.html).


Best Regards,

Boris

Re: [RFC PATCH] gpio: add GPIO hogging mechanism

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1478 bytes --]

On Thu, Dec 19, 2013 at 06:18:40PM +0100, boris brezillon wrote:
> Hello Felipe,
> 
> On 19/12/2013 17:47, Felipe Balbi wrote:
> >On Thu, Dec 19, 2013 at 08:41:09AM -0800, Greg Kroah-Hartman wrote:
> >>On Thu, Dec 19, 2013 at 03:34:31PM +0100, Boris BREZILLON wrote:
> >>>GPIO hogging is a way to request and configure specific GPIO without
> >>>explicitly requesting it in the device driver.
> >>>
> >>>The request and configuration procedure is handled in the core device
> >>>driver code before the driver probe function is called.
> >>>
> >>>It allows specific GPIOs to be configured without any driver specific code.
> >>>
> >>>Particularly usefull when a external device is connected to a bus and the
> >>>bus connections depends on an external switch controlled by a GPIO pin.
> >for external switches, you probably need a pinctrl-gpio driver.
> >
> Do you mean using pinctrl pinconf to configure the PIN as output-high or
> output-low ?
> 
> This was my first proposal
> (see https://www.mail-archive.com/devicetree@vger.kernel.org/msg05829.html).

that's quite a weird argument from Linus W, considering you _do_ have a
discrete mux on the board.

We have quite a few of such "crazy" scenarios here at TI and we were
going to send a pinctrl-gpio driver. If that's not acceptable, then I
suppose there is no way to boot from NAND on a board where NAND signals
go through a discrete mux where the select signal is a GPIO pin.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC PATCH] gpio: add GPIO hogging mechanism

[boris brezillon]

Hello,

On 19/12/2013 19:22, Felipe Balbi wrote:
> On Thu, Dec 19, 2013 at 06:18:40PM +0100, boris brezillon wrote:
>> Hello Felipe,
>>
>> On 19/12/2013 17:47, Felipe Balbi wrote:
>>> On Thu, Dec 19, 2013 at 08:41:09AM -0800, Greg Kroah-Hartman wrote:
>>>> On Thu, Dec 19, 2013 at 03:34:31PM +0100, Boris BREZILLON wrote:
>>>>> GPIO hogging is a way to request and configure specific GPIO without
>>>>> explicitly requesting it in the device driver.
>>>>>
>>>>> The request and configuration procedure is handled in the core device
>>>>> driver code before the driver probe function is called.
>>>>>
>>>>> It allows specific GPIOs to be configured without any driver specific code.
>>>>>
>>>>> Particularly usefull when a external device is connected to a bus and the
>>>>> bus connections depends on an external switch controlled by a GPIO pin.
>>> for external switches, you probably need a pinctrl-gpio driver.
>> Do you mean using pinctrl pinconf to configure the PIN as output-high or
>> output-low ?
>>
>> This was my first proposal
>> (see https://www.mail-archive.com/devicetree@vger.kernel.org/msg05829.html).

My mistake: this is not exactly what I proposed in my patch series.

Actually, I was directly requesting the pin connected to the external 
switch as
OUTPUT + (OUTPUT-LEVEL) according the the device needs:
- output high for MMC slot
- output low for SPI device

And, I guess this is why Linus asked me to find a better solution.

IMHO your approach is, by far, much better. You expose the external switch
as a PIN muxing device and the devices connected to through this PIN mux
block just have to request the appropriate PIN states.

In my specific case this would give the following:
- MMC conf for mmc slot 0
- SPI conf for the SPI device

With your approach the HW representation is better: the different signals
controlled by the external switch can be seen using debugfs, and device
tree definition is closer to the real HW design.

> that's quite a weird argument from Linus W, considering you _do_ have a
> discrete mux on the board.

> We have quite a few of such "crazy" scenarios here at TI and we were
> going to send a pinctrl-gpio driver. If that's not acceptable, then I
> suppose there is no way to boot from NAND on a board where NAND signals
> go through a discrete mux where the select signal is a GPIO pin.
>

Please keep going with the submission process: I'm really interested in this
driver (BTW could you add me in the CC list ?).

Linus, tell me if I'm wrong, but I think, the pinctrl-gpio is the right 
way to solve
the at91rm9200ek board use case.

This does not mean, the gpio hogs approach does not solve other issues (see
https://lkml.org/lkml/2013/8/29/333 and
https://www.mail-archive.com/linux-gpio@vger.kernel.org/msg01084.html), 
but in
my specific case, I'd rather use the pinctrl-gpio driver.

Best Regards,

Boris

Re: [RFC PATCH] gpio: add GPIO hogging mechanism

[Linus Walleij]

On Mon, Dec 30, 2013 at 10:48 AM, boris brezillon
<b.brezillon@overkiz.com> wrote:
> On 19/12/2013 19:22, Felipe Balbi wrote:

>> that's quite a weird argument from Linus W, considering you _do_ have a
>> discrete mux on the board.
>
>> We have quite a few of such "crazy" scenarios here at TI and we were
>> going to send a pinctrl-gpio driver.

Hm I'm all in the blue as to what a "pinctrl-gpio driver" is ... I'm
confused :-)

>> If that's not acceptable, then I
>> suppose there is no way to boot from NAND on a board where NAND signals
>> go through a discrete mux where the select signal is a GPIO pin.

One problem I have is that I still don't really understand if this is a pin
mux, i.e. changing the connection to a certain device onto some actual
*PIN* or just some other mux muxing some certain line from one silicon
block to another.

> Linus, tell me if I'm wrong, but I think, the pinctrl-gpio is the right way
> to solve
> the at91rm9200ek board use case.

I don't know, because I don't know exactly what you mean by
"pinctrl-gpio".

Yours,
Linus Walleij

Re: [RFC PATCH] gpio: add GPIO hogging mechanism

[boris brezillon]

Hello Greg,

On 19/12/2013 17:41, Greg Kroah-Hartman wrote:
> On Thu, Dec 19, 2013 at 03:34:31PM +0100, Boris BREZILLON wrote:
>> GPIO hogging is a way to request and configure specific GPIO without
>> explicitly requesting it in the device driver.
>>
>> The request and configuration procedure is handled in the core device
>> driver code before the driver probe function is called.
>>
>> It allows specific GPIOs to be configured without any driver specific code.
>>
>> Particularly usefull when a external device is connected to a bus and the
>> bus connections depends on an external switch controlled by a GPIO pin.
>> Or when some GPIOs have to be exported to sysfs without any userspace
>> intervention.
>>
>> Signed-off-by: Boris BREZILLON <b.brezillon-ZNYIgs0QAGpBDgjK7y7TUQ@public.gmane.org>
>> ---
>>   Documentation/devicetree/bindings/gpio/gpio.txt |   47 ++++++++
>>   drivers/base/Makefile                           |    1 +
>>   drivers/base/dd.c                               |    5 +
>>   drivers/base/gpio.c                             |   59 ++++++++++
> I don't understand what makes GPIO's "special" enough to get included in
> the driver core like this, and called for each and every device that is
> added to the system.


> What's wrong with the generic device callbacks/notifiers we already
> have?  Why does this need to be in the driver core?  And what exactly
> are you doing all of this for in the first place?

Nothing's wrong with the generic device callbacks/notifiers, but in some
cases we don't want a generic driver to handle board specificities.

This is the case for the at91rm9200ek board (see this thread:
https://www.mail-archive.com/devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg06838.html).

Since we are moving all boards to dt, we can't configure each board at
startup, and the rm9200ek board used to set a pin to a specific value 
(this pin
is connected to an external switch which connects MMC signals to the MMC
connector or SPI signals to an SPI device depending on the pin value) in 
order
to enable the MMC0 port or an SPI device.

I first proposed to handle this using a pinctrl pinconf definition (with 
pinctrl
output-high config). As pinctrl are requesting pinconf before calling 
driver probe
function, this was absolutely transparent to the generic driver.
But, as Linus pointed out, this pin is not really related to the SPI or 
MMC device
itself.
This is why Linus suggested the GPIO hog approach.

Anyway, I'm open to any other solution that wouldn't introduce specific
cases in arch specific code or generic drivers.

Best Regards,

Boris
> greg k-h

--
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

Re: [RFC PATCH] gpio: add GPIO hogging mechanism

[Ben Gamari]

[-- Attachment #1: Type: text/plain, Size: 420 bytes --]

Hi Boris,

I'm just returning to this now.


Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:

> I don't understand what makes GPIO's "special" enough to get included in
> the driver core like this, and called for each and every device that is
> added to the system.
>
I'm also a bit confused why GPIOs ended up in the driver core but
perhaps I'm missing something. What was your motivation here?

Cheers,

- Ben

[-- Attachment #2: Type: application/pgp-signature, Size: 472 bytes --]

Re: [RFC PATCH] gpio: add GPIO hogging mechanism

[Ben Gamari]

[-- Attachment #1: Type: text/plain, Size: 568 bytes --]

Ben Gamari <bgamari.foss@gmail.com> writes:

> Hi Boris,
>
> I'm just returning to this now.
>
>
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
>
>> I don't understand what makes GPIO's "special" enough to get included in
>> the driver core like this, and called for each and every device that is
>> added to the system.
>>
> I'm also a bit confused why GPIOs ended up in the driver core but
> perhaps I'm missing something. What was your motivation here?
>
Somehow I overlooked the message in which you responded to this. Ignore
the above.

Cheers,

- Ben

[-- Attachment #2: Type: application/pgp-signature, Size: 472 bytes --]

Re: [RFC PATCH] gpio: add GPIO hogging mechanism

[Linus Walleij]

On Thu, Dec 19, 2013 at 3:34 PM, Boris BREZILLON
<b.brezillon@overkiz.com> wrote:

> GPIO hogging is a way to request and configure specific GPIO without
> explicitly requesting it in the device driver.
>
> The request and configuration procedure is handled in the core device
> driver code before the driver probe function is called.

Why?

Why can't these hogs be handled right after the gpio chip has been
added in the end of the gpiochio_add() function?

> +It might contain GPIO hog definitions. GPIO hogging is a mechanism providing
> +automatic GPIO request and configuration before the device is passed to its
> +driver probe function (the same mechanism is used for pinctrl pin config).
> +
> +Each GPIO hog definition is represented as a child node of the GPIO controller.
> +Required properties:
> +- gpio: store the gpio informations (id, flags, ...). Shall contain the
> +  number of cells specified in its parent node (GPIO controller node).

This property is alway plural "gpios".

> +- hog-as-input or hog-as-output: a boolean property specifying the GPIO
> +  direction.
> +- hog-init-high or hog-init-low: a boolean property specifying the GPIO
> +  value in case the GPIO is hogged as output.

What about just having these three:

hog-as-input
hog-as-output-high
hog-as-output-low

> +Optional properties:
> +- open-drain-line: GPIO is configured as an open drain pin.
> +- open-source-line: GPIO is configured as an open source pin.

Can you not simply us the flags in the second cell for the gpios
property for this, normally?

> +- export-line or export-line-fixed: the GPIO is exported to userspace via
> +  sysfs.

Move this feature to a separate patch. It is much more controversial
than the hog concept as such.

> +- line-name: the GPIO label name.

OK.

> @@ -66,6 +95,19 @@ Example of two SOC GPIO banks defined as gpio-controller nodes:
>                 compatible = "fsl,qe-pario-bank-a", "fsl,qe-pario-bank";
>                 reg = <0x1400 0x18>;
>                 gpio-controller;
> +               gpio-hogs = <&exported_gpio>;
> +
> +               line_a: line-a {
> +                       gpio = <5 0>;
> +                       hog-as-input;
> +                       line-name = "line-a";
> +               };
> +
> +               exported_gpio: exported-gpio {
> +                       gpio = <6 0>;
> +                       hog-as-output;
> +                       line-name = "exported-gpio";
> +               };

This looks pretty straight-forward to use, but need the DT maintainers
to provide input on this.

>         };
>
>         qe_pio_e: gpio-controller@1460 {
> @@ -75,6 +117,11 @@ Example of two SOC GPIO banks defined as gpio-controller nodes:
>                 gpio-controller;
>         };
>
> +       pio_consumer {
> +               gpio-hogs = <&line_a>;
> +               gpio-hog-names = "my-line-a";
> +       };

No. No way. That is not what hogs is about. It is not to make consumers
get their GPIOs grabbed automatically when probing. It is *only* about
making it possible for the core to set up a few GPIO lines *not* belonging
to any particular in-kernel device when probing the GPIO chip.

For providing GPIOs to consumers, use the existing OF bindings and
add code to the drivers to handle them. A device needs to keep track
of its resources.

In any case, such concepts would be a totally separate patch.

> diff --git a/drivers/base/dd.c b/drivers/base/dd.c

NAK on this, hogs shall be a GPIO-intrinsic.

> @@ -142,6 +175,9 @@ int devm_gpio_request(struct device *dev, unsigned gpio, const char *label)
>         if (!dr)
>                 return -ENOMEM;
>
> +       if (gpio_is_hogged(dev, gpio))
> +               return 0;
> +
>         rc = gpio_request(gpio, label);
>         if (rc) {
>                 devres_free(dr);
> @@ -172,6 +208,9 @@ int devm_gpio_request_one(struct device *dev, unsigned gpio,
>         if (!dr)
>                 return -ENOMEM;
>
> +       if (gpio_is_hogged(dev, gpio))
> +               return 0;
> +

This just makes it impossible to release these resources. Don't do this.

Hogs should be tied to a certain GPIO chip, get hogged when the
chip is added and get removed when the chip is removed.

> + * of_gpio_hog_count() - Count a GPIO hogs on a specific device node
> + * @np:                device node to get GPIO from
> + *
> + * Returns the number of GPIO hogs requested by this device node.
> + */
> +int of_gpio_hog_count(struct device_node *np)
> +{
> +       int size;
> +
> +       if (!of_get_property(np, "gpio-hogs", &size))
> +               return 0;
> +
> +       return size / sizeof(phandle);
> +}
> +EXPORT_SYMBOL(of_gpio_hog_count);

There is no reason to export this, it should be static, gpiolib-internal.

> +EXPORT_SYMBOL(of_get_gpio_hog);

Dito.

> @@ -1214,6 +1216,17 @@ int gpiochip_add(struct gpio_chip *chip)
>
>         of_gpiochip_add(chip);
>
> +       /* Instantiate missing GPIO hogs */
> +       if (chip->dev->gpios) {
> +               for (i = 0; i < chip->dev->gpios->ngpios; i++) {
> +                       if (chip->dev->gpios->gpios[i])
> +                               continue;
> +                       desc = devm_gpiod_get_hog_index(chip->dev, i);
> +                       if (!IS_ERR(desc))
> +                               chip->dev->gpios->gpios[i] = desc;
> +               }
> +       }

This is the only thing you should be doing.

> @@ -2421,6 +2434,7 @@ struct gpio_desc *__must_check gpiod_get_index(struct device *dev,
>         struct gpio_desc *desc = NULL;
>         int status;
>         enum gpio_lookup_flags flags = 0;
> +       int i;
>
>         dev_dbg(dev, "GPIO lookup for consumer %s\n", con_id);
>
> @@ -2451,6 +2465,13 @@ struct gpio_desc *__must_check gpiod_get_index(struct device *dev,
>                 return desc;
>         }
>
> +       if (dev->gpios) {
> +               for (i = 0; i < dev->gpios->ngpios; i++) {
> +                       if (dev->gpios->gpios[i] == desc)
> +                               return desc;
> +               }
> +       }

Don't do this. We already have explicit gpio resource management.
Don't attempt to make it implicit.

> +EXPORT_SYMBOL_GPL(gpiod_get_hog_index);

I don't understand this function at all.

> +bool gpio_is_hogged(struct device *dev, unsigned gpio)
> +{
> +       return gpiod_is_hogged(dev, gpio_to_desc(gpio));
> +}
> +EXPORT_SYMBOL_GPL(gpio_is_hogged);

This should not be exported.

> +/**
> + * gpio_hog_count - count number of GPIO hogs requested by a specific device
> + * @dev:       GPIO consumer
> + *
> + * Return the number of GPIO hogs.
> + */
> +int gpio_hog_count(struct device *dev)
> +{
> +       /* Using device tree? */
> +       if (IS_ENABLED(CONFIG_OF) && dev && dev->of_node)
> +               return of_gpio_hog_count(dev->of_node);
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(gpio_hog_count);

Get rid of this.

> +++ b/include/linux/device.h
> @@ -22,6 +22,7 @@
>  #include <linux/types.h>
>  #include <linux/mutex.h>
>  #include <linux/pinctrl/devinfo.h>
> +#include <linux/gpio/devinfo.h>
>  #include <linux/pm.h>
>  #include <linux/atomic.h>
>  #include <linux/ratelimit.h>
> @@ -744,6 +745,10 @@ struct device {
>         struct dev_pin_info     *pins;
>  #endif
>
> +#ifdef CONFIG_GPIOLIB
> +       struct dev_gpio_info    *gpios;
> +#endif

Don't do this.

Overall it appears you try to make the device core grab all GPIOs automatically
when devices are probed, this is not what GPIO hogs are about, and we already
have a resource management model for GPIOs using the descriptors explicitly
in the drivers, I see no reason to try to push that over to the device core.

Yours,
Linus Walleij

Re: [RFC PATCH] gpio: add GPIO hogging mechanism

[boris brezillon]

On 08/01/2014 10:37, Linus Walleij wrote:
> On Thu, Dec 19, 2013 at 3:34 PM, Boris BREZILLON
> <b.brezillon@overkiz.com> wrote:
>
>> GPIO hogging is a way to request and configure specific GPIO without
>> explicitly requesting it in the device driver.
>>
>> The request and configuration procedure is handled in the core device
>> driver code before the driver probe function is called.
> Why?

Because I obviously misunderstood your suggestion in our previous
discussion regarding the at91rm9200-ek board case :-)
( https://www.mail-archive.com/devicetree@vger.kernel.org/msg06838.html).

I thought you were suggesting to request GPIOs as PINCTRL configs are
requested, but apparently, you were just suggesting to directly request
the pins within the gpio controller.

Regarding the specific at91rm920-ek case, your solution should work
(as long as the gpio is configured before the SPI or MMC device is probed,
which should be the case), but the dependency between the SPI or MMC
device and the SPI/MMC switch pin is not represented in DT, and I'm not
happy with this.


Anyway, do you want me to rework the gpio hog as described below ?

Best Regards,

Boris
>
> Why can't these hogs be handled right after the gpio chip has been
> added in the end of the gpiochio_add() function?
>
>> +It might contain GPIO hog definitions. GPIO hogging is a mechanism providing
>> +automatic GPIO request and configuration before the device is passed to its
>> +driver probe function (the same mechanism is used for pinctrl pin config).
>> +
>> +Each GPIO hog definition is represented as a child node of the GPIO controller.
>> +Required properties:
>> +- gpio: store the gpio informations (id, flags, ...). Shall contain the
>> +  number of cells specified in its parent node (GPIO controller node).
> This property is alway plural "gpios".
>
>> +- hog-as-input or hog-as-output: a boolean property specifying the GPIO
>> +  direction.
>> +- hog-init-high or hog-init-low: a boolean property specifying the GPIO
>> +  value in case the GPIO is hogged as output.
> What about just having these three:
>
> hog-as-input
> hog-as-output-high
> hog-as-output-low
>
>> +Optional properties:
>> +- open-drain-line: GPIO is configured as an open drain pin.
>> +- open-source-line: GPIO is configured as an open source pin.
> Can you not simply us the flags in the second cell for the gpios
> property for this, normally?
>
>> +- export-line or export-line-fixed: the GPIO is exported to userspace via
>> +  sysfs.
> Move this feature to a separate patch. It is much more controversial
> than the hog concept as such.
>
>> +- line-name: the GPIO label name.
> OK.
>
>> @@ -66,6 +95,19 @@ Example of two SOC GPIO banks defined as gpio-controller nodes:
>>                  compatible = "fsl,qe-pario-bank-a", "fsl,qe-pario-bank";
>>                  reg = <0x1400 0x18>;
>>                  gpio-controller;
>> +               gpio-hogs = <&exported_gpio>;
>> +
>> +               line_a: line-a {
>> +                       gpio = <5 0>;
>> +                       hog-as-input;
>> +                       line-name = "line-a";
>> +               };
>> +
>> +               exported_gpio: exported-gpio {
>> +                       gpio = <6 0>;
>> +                       hog-as-output;
>> +                       line-name = "exported-gpio";
>> +               };
> This looks pretty straight-forward to use, but need the DT maintainers
> to provide input on this.
>
>>          };
>>
>>          qe_pio_e: gpio-controller@1460 {
>> @@ -75,6 +117,11 @@ Example of two SOC GPIO banks defined as gpio-controller nodes:
>>                  gpio-controller;
>>          };
>>
>> +       pio_consumer {
>> +               gpio-hogs = <&line_a>;
>> +               gpio-hog-names = "my-line-a";
>> +       };
> No. No way. That is not what hogs is about. It is not to make consumers
> get their GPIOs grabbed automatically when probing. It is *only* about
> making it possible for the core to set up a few GPIO lines *not* belonging
> to any particular in-kernel device when probing the GPIO chip.
>
> For providing GPIOs to consumers, use the existing OF bindings and
> add code to the drivers to handle them. A device needs to keep track
> of its resources.
>
> In any case, such concepts would be a totally separate patch.
>
>> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> NAK on this, hogs shall be a GPIO-intrinsic.
>
>> @@ -142,6 +175,9 @@ int devm_gpio_request(struct device *dev, unsigned gpio, const char *label)
>>          if (!dr)
>>                  return -ENOMEM;
>>
>> +       if (gpio_is_hogged(dev, gpio))
>> +               return 0;
>> +
>>          rc = gpio_request(gpio, label);
>>          if (rc) {
>>                  devres_free(dr);
>> @@ -172,6 +208,9 @@ int devm_gpio_request_one(struct device *dev, unsigned gpio,
>>          if (!dr)
>>                  return -ENOMEM;
>>
>> +       if (gpio_is_hogged(dev, gpio))
>> +               return 0;
>> +
> This just makes it impossible to release these resources. Don't do this.
>
> Hogs should be tied to a certain GPIO chip, get hogged when the
> chip is added and get removed when the chip is removed.
>
>> + * of_gpio_hog_count() - Count a GPIO hogs on a specific device node
>> + * @np:                device node to get GPIO from
>> + *
>> + * Returns the number of GPIO hogs requested by this device node.
>> + */
>> +int of_gpio_hog_count(struct device_node *np)
>> +{
>> +       int size;
>> +
>> +       if (!of_get_property(np, "gpio-hogs", &size))
>> +               return 0;
>> +
>> +       return size / sizeof(phandle);
>> +}
>> +EXPORT_SYMBOL(of_gpio_hog_count);
> There is no reason to export this, it should be static, gpiolib-internal.
>
>> +EXPORT_SYMBOL(of_get_gpio_hog);
> Dito.
>
>> @@ -1214,6 +1216,17 @@ int gpiochip_add(struct gpio_chip *chip)
>>
>>          of_gpiochip_add(chip);
>>
>> +       /* Instantiate missing GPIO hogs */
>> +       if (chip->dev->gpios) {
>> +               for (i = 0; i < chip->dev->gpios->ngpios; i++) {
>> +                       if (chip->dev->gpios->gpios[i])
>> +                               continue;
>> +                       desc = devm_gpiod_get_hog_index(chip->dev, i);
>> +                       if (!IS_ERR(desc))
>> +                               chip->dev->gpios->gpios[i] = desc;
>> +               }
>> +       }
> This is the only thing you should be doing.
>
>> @@ -2421,6 +2434,7 @@ struct gpio_desc *__must_check gpiod_get_index(struct device *dev,
>>          struct gpio_desc *desc = NULL;
>>          int status;
>>          enum gpio_lookup_flags flags = 0;
>> +       int i;
>>
>>          dev_dbg(dev, "GPIO lookup for consumer %s\n", con_id);
>>
>> @@ -2451,6 +2465,13 @@ struct gpio_desc *__must_check gpiod_get_index(struct device *dev,
>>                  return desc;
>>          }
>>
>> +       if (dev->gpios) {
>> +               for (i = 0; i < dev->gpios->ngpios; i++) {
>> +                       if (dev->gpios->gpios[i] == desc)
>> +                               return desc;
>> +               }
>> +       }
> Don't do this. We already have explicit gpio resource management.
> Don't attempt to make it implicit.
>
>> +EXPORT_SYMBOL_GPL(gpiod_get_hog_index);
> I don't understand this function at all.
>
>> +bool gpio_is_hogged(struct device *dev, unsigned gpio)
>> +{
>> +       return gpiod_is_hogged(dev, gpio_to_desc(gpio));
>> +}
>> +EXPORT_SYMBOL_GPL(gpio_is_hogged);
> This should not be exported.
>
>> +/**
>> + * gpio_hog_count - count number of GPIO hogs requested by a specific device
>> + * @dev:       GPIO consumer
>> + *
>> + * Return the number of GPIO hogs.
>> + */
>> +int gpio_hog_count(struct device *dev)
>> +{
>> +       /* Using device tree? */
>> +       if (IS_ENABLED(CONFIG_OF) && dev && dev->of_node)
>> +               return of_gpio_hog_count(dev->of_node);
>> +       return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(gpio_hog_count);
> Get rid of this.
>
>> +++ b/include/linux/device.h
>> @@ -22,6 +22,7 @@
>>   #include <linux/types.h>
>>   #include <linux/mutex.h>
>>   #include <linux/pinctrl/devinfo.h>
>> +#include <linux/gpio/devinfo.h>
>>   #include <linux/pm.h>
>>   #include <linux/atomic.h>
>>   #include <linux/ratelimit.h>
>> @@ -744,6 +745,10 @@ struct device {
>>          struct dev_pin_info     *pins;
>>   #endif
>>
>> +#ifdef CONFIG_GPIOLIB
>> +       struct dev_gpio_info    *gpios;
>> +#endif
> Don't do this.
>
> Overall it appears you try to make the device core grab all GPIOs automatically
> when devices are probed, this is not what GPIO hogs are about, and we already
> have a resource management model for GPIOs using the descriptors explicitly
> in the drivers, I see no reason to try to push that over to the device core.
>
> Yours,
> Linus Walleij

Re: [RFC PATCH] gpio: add GPIO hogging mechanism

[Linus Walleij]

On Wed, Jan 8, 2014 at 11:18 AM, boris brezillon
<b.brezillon@overkiz.com> wrote:

> Anyway, do you want me to rework the gpio hog as described below ?

If you feel you have time, drive and a proper usecase for testing it,
sure. I just want it to be driven my someone who *really* needs
that feature.

Yours,
Linus Walleij

[RFC Patch] gpio: add GPIO hogging mechanism

[Benoit Parrot]

Based on Boris Brezillion work this is a reworked patch
of his initial GPIO hogging mechanism.
This patch provides a way to initally configure specific GPIO
when the gpio controller is probe.

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

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

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>
---
 Documentation/devicetree/bindings/gpio/gpio.txt | 33 +++++++++
 drivers/gpio/gpiolib-of.c                       | 99 +++++++++++++++++++++++++
 drivers/gpio/gpiolib.c                          | 81 ++++++++++++++++++++
 include/linux/of_gpio.h                         | 11 +++
 4 files changed, 224 insertions(+)

diff --git a/Documentation/devicetree/bindings/gpio/gpio.txt b/Documentation/devicetree/bindings/gpio/gpio.txt
index 3fb8f53..a9700d8 100644
--- a/Documentation/devicetree/bindings/gpio/gpio.txt
+++ b/Documentation/devicetree/bindings/gpio/gpio.txt
@@ -103,6 +103,26 @@ 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.
 
+It might 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:
+- gpios: store the gpio information (id, flags, ...). Shall contain the
+  number of cells specified in its parent node (GPIO controller node).
+- input: a property specifying to set the GPIO direction as input.
+- output-high: a property specifying to set the GPIO direction to output with
+  the value high.
+- output-low: a property specifying to set the GPIO direction to output with
+  the value low.
+
+Optional properties:
+- line-name: the GPIO label name. If not present the node name is used.
+
+A GPIO controller can request GPIO hogs using the "gpio-hogs" property, which
+encodes a list of requested GPIO hogs.
+
 Example of two SOC GPIO banks defined as gpio-controller nodes:
 
 	qe_pio_a: gpio-controller@1400 {
@@ -110,6 +130,19 @@ Example of two SOC GPIO banks defined as gpio-controller nodes:
 		reg = <0x1400 0x18>;
 		gpio-controller;
 		#gpio-cells = <2>;
+		gpio-hogs = <&line_b>;
+
+		/* line_a hog is defined but not enabled in this example*/
+		line_a: line_a {
+			gpios = <5 0>;
+			input;
+		};
+
+		line_b: line_b {
+			gpios = <6 0>;
+			output-low;
+			line-name = "foo-bar-gpio";
+		};
 	};
 
 	qe_pio_e: gpio-controller@1460 {
diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 604dbe6..7308dfc 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -111,6 +111,105 @@ 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
+ * @index:	index of the GPIO
+ * @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.
+ */
+struct gpio_desc *of_get_gpio_hog(struct device_node *np, int index,
+				  const char **name, unsigned long *flags)
+{
+	struct device_node *cfg_np, *chip_np;
+	enum of_gpio_flags xlate_flags;
+	unsigned long req_flags = 0;
+	struct gpio_desc *desc;
+	struct gg_data gg_data = {
+		.flags = &xlate_flags,
+		.out_gpio = NULL,
+	};
+	u32 tmp;
+	int i;
+	int ret;
+
+	cfg_np = of_parse_phandle(np, "gpio-hogs", index);
+	if (!cfg_np)
+		return ERR_PTR(-EINVAL);
+
+	chip_np = cfg_np->parent;
+	if (!chip_np) {
+		desc = ERR_PTR(-EINVAL);
+		goto out;
+	}
+
+	ret = of_property_read_u32(chip_np, "#gpio-cells", &tmp);
+	if (ret) {
+		desc = ERR_PTR(ret);
+		goto out;
+	}
+
+	if (tmp > MAX_PHANDLE_ARGS) {
+		desc = ERR_PTR(-EINVAL);
+		goto out;
+	}
+
+	gg_data.gpiospec.args_count = tmp;
+	gg_data.gpiospec.np = chip_np;
+	for (i = 0; i < tmp; i++) {
+		ret = of_property_read_u32(cfg_np, "gpios",
+					   &gg_data.gpiospec.args[i]);
+		if (ret) {
+			desc = ERR_PTR(ret);
+			goto out;
+		}
+	}
+
+	gpiochip_find(&gg_data, of_gpiochip_find_and_xlate);
+	if (!gg_data.out_gpio) {
+		if (cfg_np->parent == np)
+			desc = ERR_PTR(-ENXIO);
+		else
+			desc = ERR_PTR(-EPROBE_DEFER);
+
+		goto out;
+	}
+
+	if (xlate_flags & OF_GPIO_ACTIVE_LOW)
+		req_flags |= GPIOF_ACTIVE_LOW;
+
+	if (of_property_read_bool(cfg_np, "input")) {
+		req_flags |= GPIOF_DIR_IN;
+	} else if (of_property_read_bool(cfg_np, "output-high")) {
+		req_flags |= GPIOF_INIT_HIGH;
+	} else if (!of_property_read_bool(cfg_np, "output-low")) {
+		desc = ERR_PTR(-EINVAL);
+		goto out;
+	}
+
+	if (of_property_read_bool(cfg_np, "open-drain-line"))
+		req_flags |= GPIOF_OPEN_DRAIN;
+
+	if (of_property_read_bool(cfg_np, "open-source-line"))
+		req_flags |= GPIOF_OPEN_DRAIN;
+
+	if (name && of_property_read_string(cfg_np, "line-name", name))
+		*name = cfg_np->name;
+
+	if (flags)
+		*flags = req_flags;
+
+	desc = gg_data.out_gpio;
+
+out:
+	of_node_put(cfg_np);
+
+	return desc;
+}
+
+/**
  * 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
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index e8e98ca..b6f5bdb 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -55,6 +55,9 @@ static DEFINE_MUTEX(gpio_lookup_lock);
 static LIST_HEAD(gpio_lookup_list);
 LIST_HEAD(gpio_chips);
 
+struct gpio_desc *__must_check gpiod_get_hog_index(struct device *dev,
+						   unsigned int idx);
+
 static inline void desc_set_label(struct gpio_desc *d, const char *label)
 {
 	d->label = label;
@@ -226,6 +229,8 @@ int gpiochip_add(struct gpio_chip *chip)
 	int		status = 0;
 	unsigned	id;
 	int		base = chip->base;
+	struct gpio_desc	*desc;
+	int		i;
 
 	if ((!gpio_is_valid(base) || !gpio_is_valid(base + chip->ngpio - 1))
 			&& base >= 0) {
@@ -275,6 +280,16 @@ int gpiochip_add(struct gpio_chip *chip)
 	of_gpiochip_add(chip);
 	acpi_gpiochip_add(chip);
 
+	/*
+	 * Instantiate GPIO hogs
+	 * There shouldn't be mores hogs then gpio available on this chip
+	 */
+	for (i = 0; i < chip->ngpio; i++) {
+		desc = gpiod_get_hog_index(chip->dev, i);
+		if (IS_ERR(desc))
+			break;
+	}
+
 	if (status)
 		goto fail;
 
@@ -1742,6 +1757,72 @@ struct gpio_desc *__must_check __gpiod_get_index_optional(struct device *dev,
 EXPORT_SYMBOL_GPL(__gpiod_get_index_optional);
 
 /**
+ * gpiod_get_hog_index - obtain a GPIO from a multi-index GPIO hog
+ * @dev:	GPIO consumer
+ * @idx:	index of the GPIO to obtain
+ *
+ * This should only be used by the gpiochip_add to request/set GPIO initial
+ * configuration.
+ *
+ * Return a valid GPIO descriptor, or an IS_ERR() condition in case of error.
+ */
+struct gpio_desc *__must_check gpiod_get_hog_index(struct device *dev,
+						   unsigned int idx)
+{
+	struct gpio_desc *desc = NULL;
+	int err;
+	unsigned long flags;
+	const char *name;
+
+	/* Using device tree? */
+	if (IS_ENABLED(CONFIG_OF) && dev && dev->of_node)
+		desc = of_get_gpio_hog(dev->of_node, idx, &name, &flags);
+
+	if (!desc)
+		return ERR_PTR(-ENOTSUPP);
+	else if (IS_ERR(desc))
+		return desc;
+
+	dev_dbg(dev, "gpio-hog: GPIO:%d (%s) as %s%s\n",
+		desc_to_gpio(desc), name, (flags&GPIOF_DIR_IN)?"input":"output",
+		(flags&GPIOF_DIR_IN)?"":(flags&GPIOF_INIT_HIGH)?"/high":"/low");
+
+	err = gpiod_request(desc, name);
+	if (err)
+		return ERR_PTR(err);
+
+	if (flags & GPIOF_OPEN_DRAIN)
+		set_bit(FLAG_OPEN_DRAIN, &desc->flags);
+
+	if (flags & GPIOF_OPEN_SOURCE)
+		set_bit(FLAG_OPEN_SOURCE, &desc->flags);
+
+	if (flags & GPIOF_ACTIVE_LOW)
+		set_bit(FLAG_ACTIVE_LOW, &desc->flags);
+
+	if (flags & GPIOF_DIR_IN)
+		err = gpiod_direction_input(desc);
+	else
+		err = gpiod_direction_output_raw(desc,
+				(flags & GPIOF_INIT_HIGH) ? 1 : 0);
+
+	if (err)
+		goto free_gpio;
+
+	if (flags & GPIOF_EXPORT) {
+		err = gpiod_export(desc, flags & GPIOF_EXPORT_CHANGEABLE);
+		if (err)
+			goto free_gpio;
+	}
+
+	return desc;
+
+free_gpio:
+	gpiod_free(desc);
+	return ERR_PTR(err);
+}
+
+/**
  * gpiod_put - dispose of a GPIO descriptor
  * @desc:	GPIO descriptor to dispose of
  *
diff --git a/include/linux/of_gpio.h b/include/linux/of_gpio.h
index 38fc050..52d154c 100644
--- a/include/linux/of_gpio.h
+++ b/include/linux/of_gpio.h
@@ -59,6 +59,9 @@ extern int of_gpio_simple_xlate(struct gpio_chip *gc,
 				const struct of_phandle_args *gpiospec,
 				u32 *flags);
 
+extern struct gpio_desc *of_get_gpio_hog(struct device_node *np, int index,
+					 const char **name,
+					 unsigned long *flags);
 #else /* CONFIG_OF_GPIO */
 
 /* Drivers may not strictly depend on the GPIO support, so let them link. */
@@ -78,6 +81,14 @@ static inline int of_gpio_simple_xlate(struct gpio_chip *gc,
 static inline void of_gpiochip_add(struct gpio_chip *gc) { }
 static inline void of_gpiochip_remove(struct gpio_chip *gc) { }
 
+static inline struct gpio_desc *of_get_gpio_hog(struct device_node *np,
+						int index,
+						const char **name,
+						unsigned long *flags)
+{
+	return ERR_PTR(-ENOSYS);
+}
+
 #endif /* CONFIG_OF_GPIO */
 
 /**
-- 
1.8.5.1

Re: [RFC Patch] gpio: add GPIO hogging mechanism

[Alexandre Courbot]

Sorry for the delay in reviewing. Adding Jiri and Pantelis who might
want to extend over this patch and thus will likely have interesting
comments to make.

On Wed, Oct 22, 2014 at 5:09 AM, Benoit Parrot <bparrot@ti.com> wrote:
> Based on Boris Brezillion work this is a reworked patch
> of his initial GPIO hogging mechanism.
> This patch provides a way to initally configure specific GPIO
> when the gpio controller is probe.

Typo nit: s/probe/probed

>
> The actual DT scanning to collect the GPIO specific data is performed
> as part of the gpiochip_add().
>
> The purpose of this is to allows specific GPIOs to be configured
> without any driver specific code.
> This particularly usueful because board design are getting

s/suseful/useful

> increassingly complex and given SoC pins can now have upward

s/increassingly/increasingly

> of 10 mux values a lot of connections are now dependent on
> external IO muxes to switch various modes and combination.
>
> 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>
> ---
>  Documentation/devicetree/bindings/gpio/gpio.txt | 33 +++++++++
>  drivers/gpio/gpiolib-of.c                       | 99 +++++++++++++++++++++++++
>  drivers/gpio/gpiolib.c                          | 81 ++++++++++++++++++++
>  include/linux/of_gpio.h                         | 11 +++
>  4 files changed, 224 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/gpio/gpio.txt b/Documentation/devicetree/bindings/gpio/gpio.txt
> index 3fb8f53..a9700d8 100644
> --- a/Documentation/devicetree/bindings/gpio/gpio.txt
> +++ b/Documentation/devicetree/bindings/gpio/gpio.txt

Note: changes to DT bindings documentation should generally come as a
separate patch.

> @@ -103,6 +103,26 @@ 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.
>
> +It might 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:
> +- gpios: store the gpio information (id, flags, ...). Shall contain the
> +  number of cells specified in its parent node (GPIO controller node).

If would be nice to be able to specify several GPIO references to that
they can be all set to the same configuration in one row instead of
requiring one node each.

> +- input: a property specifying to set the GPIO direction as input.
> +- output-high: a property specifying to set the GPIO direction to output with
> +  the value high.
> +- output-low: a property specifying to set the GPIO direction to output with
> +  the value low.

Wouldn't a "direction" property taking one of the values "input",
"output-low" or "output-high" be easier to parse and generally
clearer?

> +
> +Optional properties:
> +- line-name: the GPIO label name. If not present the node name is used.

I guess we also could use this for naming the GPIO during its export
if we decide to allow this in a near-future patch.

> +
> +A GPIO controller can request GPIO hogs using the "gpio-hogs" property, which
> +encodes a list of requested GPIO hogs.
> +
>  Example of two SOC GPIO banks defined as gpio-controller nodes:
>
>         qe_pio_a: gpio-controller@1400 {
> @@ -110,6 +130,19 @@ Example of two SOC GPIO banks defined as gpio-controller nodes:
>                 reg = <0x1400 0x18>;
>                 gpio-controller;
>                 #gpio-cells = <2>;
> +               gpio-hogs = <&line_b>;
> +
> +               /* line_a hog is defined but not enabled in this example*/
> +               line_a: line_a {
> +                       gpios = <5 0>;
> +                       input;
> +               };
> +
> +               line_b: line_b {
> +                       gpios = <6 0>;
> +                       output-low;
> +                       line-name = "foo-bar-gpio";
> +               };

I think it might be good to group the hog nodes such as to allow
complex configurations to be set in one go, à la pinmux:

                gpio-controller;
                #gpio-cells = <2>;
+               gpio-hogs = <&line_b>;
+
+               line_a: line_a {
+                      line_a {
+                               gpios = <5 0>;
+                               input;
+                      };
+                      line_c {
+                               gpios = <7 0>;
+                               inputl
+                      };
+               };
+
+               line_b: line_b {
+                       line_b {
+                               gpios = <6 0>;
+                               output-low;
+                               line-name = "foo-bar-gpio";
+                       };
+               };

Here if you want to enable the first configuration you don't need to
specify all the lines one by one, you just need to select the right
group.

I wonder if such usage of child nodes could not interfere with GPIO
drivers (existing or to be) that need to use child nodes for other
purposes. Right now there is no way to distinguish a hog node from a
node that serves another purpose, and that might become a problem in
the future. Not sure how that should be addressed though - maybe by
enforcing a "-hog" suffix for the group nodes, e.g. line_a-hog?
Comments from people more experienced with DT would be nice.

>         };
>
>         qe_pio_e: gpio-controller@1460 {
> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> index 604dbe6..7308dfc 100644
> --- a/drivers/gpio/gpiolib-of.c
> +++ b/drivers/gpio/gpiolib-of.c
> @@ -111,6 +111,105 @@ 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
> + * @index:     index of the GPIO
> + * @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.
> + */
> +struct gpio_desc *of_get_gpio_hog(struct device_node *np, int index,
> +                                 const char **name, unsigned long *flags)
> +{
> +       struct device_node *cfg_np, *chip_np;
> +       enum of_gpio_flags xlate_flags;
> +       unsigned long req_flags = 0;
> +       struct gpio_desc *desc;
> +       struct gg_data gg_data = {
> +               .flags = &xlate_flags,
> +               .out_gpio = NULL,
> +       };
> +       u32 tmp;
> +       int i;
> +       int ret;
> +
> +       cfg_np = of_parse_phandle(np, "gpio-hogs", index);
> +       if (!cfg_np)
> +               return ERR_PTR(-EINVAL);
> +
> +       chip_np = cfg_np->parent;
> +       if (!chip_np) {
> +               desc = ERR_PTR(-EINVAL);
> +               goto out;
> +       }
> +
> +       ret = of_property_read_u32(chip_np, "#gpio-cells", &tmp);
> +       if (ret) {
> +               desc = ERR_PTR(ret);
> +               goto out;
> +       }
> +
> +       if (tmp > MAX_PHANDLE_ARGS) {
> +               desc = ERR_PTR(-EINVAL);
> +               goto out;
> +       }
> +
> +       gg_data.gpiospec.args_count = tmp;
> +       gg_data.gpiospec.np = chip_np;
> +       for (i = 0; i < tmp; i++) {
> +               ret = of_property_read_u32(cfg_np, "gpios",
> +                                          &gg_data.gpiospec.args[i]);
> +               if (ret) {
> +                       desc = ERR_PTR(ret);
> +                       goto out;
> +               }
> +       }
> +
> +       gpiochip_find(&gg_data, of_gpiochip_find_and_xlate);
> +       if (!gg_data.out_gpio) {
> +               if (cfg_np->parent == np)
> +                       desc = ERR_PTR(-ENXIO);
> +               else
> +                       desc = ERR_PTR(-EPROBE_DEFER);
> +
> +               goto out;
> +       }
> +
> +       if (xlate_flags & OF_GPIO_ACTIVE_LOW)
> +               req_flags |= GPIOF_ACTIVE_LOW;
> +
> +       if (of_property_read_bool(cfg_np, "input")) {
> +               req_flags |= GPIOF_DIR_IN;
> +       } else if (of_property_read_bool(cfg_np, "output-high")) {
> +               req_flags |= GPIOF_INIT_HIGH;
> +       } else if (!of_property_read_bool(cfg_np, "output-low")) {
> +               desc = ERR_PTR(-EINVAL);
> +               goto out;
> +       }

That's why I would prefer a "direction" property instead of these 3
ones: because if you have the following node:

line_foo {
        gpios = <1 GPIO_ACTIVE_HIGH>;
        input;
        output-high;
};

Then this code will not trigger an error and will just configure the
GPIO as input.

> +
> +       if (of_property_read_bool(cfg_np, "open-drain-line"))
> +               req_flags |= GPIOF_OPEN_DRAIN;
> +
> +       if (of_property_read_bool(cfg_np, "open-source-line"))
> +               req_flags |= GPIOF_OPEN_DRAIN;
> +
> +       if (name && of_property_read_string(cfg_np, "line-name", name))
> +               *name = cfg_np->name;
> +
> +       if (flags)
> +               *flags = req_flags;
> +
> +       desc = gg_data.out_gpio;
> +
> +out:
> +       of_node_put(cfg_np);
> +
> +       return desc;
> +}
> +
> +/**
>   * 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
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index e8e98ca..b6f5bdb 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -55,6 +55,9 @@ static DEFINE_MUTEX(gpio_lookup_lock);
>  static LIST_HEAD(gpio_lookup_list);
>  LIST_HEAD(gpio_chips);
>
> +struct gpio_desc *__must_check gpiod_get_hog_index(struct device *dev,
> +                                                  unsigned int idx);
> +
>  static inline void desc_set_label(struct gpio_desc *d, const char *label)
>  {
>         d->label = label;
> @@ -226,6 +229,8 @@ int gpiochip_add(struct gpio_chip *chip)
>         int             status = 0;
>         unsigned        id;
>         int             base = chip->base;
> +       struct gpio_desc        *desc;
> +       int             i;
>
>         if ((!gpio_is_valid(base) || !gpio_is_valid(base + chip->ngpio - 1))
>                         && base >= 0) {
> @@ -275,6 +280,16 @@ int gpiochip_add(struct gpio_chip *chip)
>         of_gpiochip_add(chip);
>         acpi_gpiochip_add(chip);
>
> +       /*
> +        * Instantiate GPIO hogs
> +        * There shouldn't be mores hogs then gpio available on this chip

s/then/than

> +        */
> +       for (i = 0; i < chip->ngpio; i++) {
> +               desc = gpiod_get_hog_index(chip->dev, i);
> +               if (IS_ERR(desc))
> +                       break;
> +       }

chip->ngpio? Isn't there a better way to know the number of phandles
in your gpio-hogs property?

Also you may have an error returned either because you reached the end
of the list, or because the hog configuration itself failed. In the
latter case don't you want to continue to go through the list?

Finally your desc variable should be declared within this loop since
it is not used elsewhere.

> +
>         if (status)
>                 goto fail;
>
> @@ -1742,6 +1757,72 @@ struct gpio_desc *__must_check __gpiod_get_index_optional(struct device *dev,
>  EXPORT_SYMBOL_GPL(__gpiod_get_index_optional);
>
>  /**
> + * gpiod_get_hog_index - obtain a GPIO from a multi-index GPIO hog
> + * @dev:       GPIO consumer
> + * @idx:       index of the GPIO to obtain
> + *
> + * This should only be used by the gpiochip_add to request/set GPIO initial
> + * configuration.
> + *
> + * Return a valid GPIO descriptor, or an IS_ERR() condition in case of error.
> + */
> +struct gpio_desc *__must_check gpiod_get_hog_index(struct device *dev,
> +                                                  unsigned int idx)
> +{
> +       struct gpio_desc *desc = NULL;
> +       int err;
> +       unsigned long flags;
> +       const char *name;
> +
> +       /* Using device tree? */
> +       if (IS_ENABLED(CONFIG_OF) && dev && dev->of_node)
> +               desc = of_get_gpio_hog(dev->of_node, idx, &name, &flags);
> +
> +       if (!desc)
> +               return ERR_PTR(-ENOTSUPP);
> +       else if (IS_ERR(desc))
> +               return desc;
> +
> +       dev_dbg(dev, "gpio-hog: GPIO:%d (%s) as %s%s\n",
> +               desc_to_gpio(desc), name, (flags&GPIOF_DIR_IN)?"input":"output",
> +               (flags&GPIOF_DIR_IN)?"":(flags&GPIOF_INIT_HIGH)?"/high":"/low");
> +

...

> +       err = gpiod_request(desc, name);
> +       if (err)
> +               return ERR_PTR(err);
> +
> +       if (flags & GPIOF_OPEN_DRAIN)
> +               set_bit(FLAG_OPEN_DRAIN, &desc->flags);
> +
> +       if (flags & GPIOF_OPEN_SOURCE)
> +               set_bit(FLAG_OPEN_SOURCE, &desc->flags);
> +
> +       if (flags & GPIOF_ACTIVE_LOW)
> +               set_bit(FLAG_ACTIVE_LOW, &desc->flags);
> +
> +       if (flags & GPIOF_DIR_IN)
> +               err = gpiod_direction_input(desc);
> +       else
> +               err = gpiod_direction_output_raw(desc,
> +                               (flags & GPIOF_INIT_HIGH) ? 1 : 0);
> +

This part of the code is very similar to what is found in
__gpiod_get_index. Could you maybe try to extract the common code into
its own function and call it from both sites instead of duplicating
code?

> +       if (err)
> +               goto free_gpio;
> +
> +       if (flags & GPIOF_EXPORT) {
> +               err = gpiod_export(desc, flags & GPIOF_EXPORT_CHANGEABLE);
> +               if (err)
> +                       goto free_gpio;

Right now export is not possible so I don't think you need that block.

> +       }
> +
> +       return desc;
> +
> +free_gpio:
> +       gpiod_free(desc);
> +       return ERR_PTR(err);
> +}
> +
> +/**
>   * gpiod_put - dispose of a GPIO descriptor
>   * @desc:      GPIO descriptor to dispose of
>   *
> diff --git a/include/linux/of_gpio.h b/include/linux/of_gpio.h
> index 38fc050..52d154c 100644
> --- a/include/linux/of_gpio.h
> +++ b/include/linux/of_gpio.h
> @@ -59,6 +59,9 @@ extern int of_gpio_simple_xlate(struct gpio_chip *gc,
>                                 const struct of_phandle_args *gpiospec,
>                                 u32 *flags);
>
> +extern struct gpio_desc *of_get_gpio_hog(struct device_node *np, int index,
> +                                        const char **name,
> +                                        unsigned long *flags);

This function is gpiolib-private, therefore it should be declared in
drivers/gpio/gpiolib.h alongside with the declaration of
of_get_named_gpiod_flags.

If I understood the latest discussions correctly we might want to add
an export (and named export) feature on top of this patch. Linus, am I
correct to understand that you are not opposed to both features? In
this case, we might want to go ahead with the merging of one of the
named GPIOs patches, so that Jiri and Pantelis can implement export
based on this patch.
--
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: [RFC Patch] gpio: add GPIO hogging mechanism

[Benoit Parrot]

Alexandre,

Thanks for the feedback.

Alexandre Courbot <gnurou@gmail.com> wrote on Wed [2014-Oct-29 16:09:59 +0900]:
> Sorry for the delay in reviewing. Adding Jiri and Pantelis who might
> want to extend over this patch and thus will likely have interesting
> comments to make.
> 
> On Wed, Oct 22, 2014 at 5:09 AM, Benoit Parrot <bparrot@ti.com> wrote:
> > Based on Boris Brezillion work this is a reworked patch
> > of his initial GPIO hogging mechanism.
> > This patch provides a way to initally configure specific GPIO
> > when the gpio controller is probe.
> 
> Typo nit: s/probe/probed
Will fix.

> 
> >
> > The actual DT scanning to collect the GPIO specific data is performed
> > as part of the gpiochip_add().
> >
> > The purpose of this is to allows specific GPIOs to be configured
> > without any driver specific code.
> > This particularly usueful because board design are getting
> 
> s/suseful/useful
> 
> > increassingly complex and given SoC pins can now have upward
> 
> s/increassingly/increasingly

Will fix.

> 
> > of 10 mux values a lot of connections are now dependent on
> > external IO muxes to switch various modes and combination.
> >
> > 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>
> > ---
> >  Documentation/devicetree/bindings/gpio/gpio.txt | 33 +++++++++
> >  drivers/gpio/gpiolib-of.c                       | 99 +++++++++++++++++++++++++
> >  drivers/gpio/gpiolib.c                          | 81 ++++++++++++++++++++
> >  include/linux/of_gpio.h                         | 11 +++
> >  4 files changed, 224 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/gpio/gpio.txt b/Documentation/devicetree/bindings/gpio/gpio.txt
> > index 3fb8f53..a9700d8 100644
> > --- a/Documentation/devicetree/bindings/gpio/gpio.txt
> > +++ b/Documentation/devicetree/bindings/gpio/gpio.txt
> 
> Note: changes to DT bindings documentation should generally come as a
> separate patch.
Ok.

> 
> > @@ -103,6 +103,26 @@ 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.
> >
> > +It might 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:
> > +- gpios: store the gpio information (id, flags, ...). Shall contain the
> > +  number of cells specified in its parent node (GPIO controller node).
> 
> If would be nice to be able to specify several GPIO references to that
> they can be all set to the same configuration in one row instead of
> requiring one node each.
> 
> > +- input: a property specifying to set the GPIO direction as input.
> > +- output-high: a property specifying to set the GPIO direction to output with
> > +  the value high.
> > +- output-low: a property specifying to set the GPIO direction to output with
> > +  the value low.
> 
> Wouldn't a "direction" property taking one of the values "input",
> "output-low" or "output-high" be easier to parse and generally
> clearer?

I guess here you mean something like this:
	...
	line_y: line_y {
		gpios = <15 0>;
		direction = <output-low>;
	};

Not sure about easier to parse, but it would be clearer.

> 
> > +
> > +Optional properties:
> > +- line-name: the GPIO label name. If not present the node name is used.
> 
> I guess we also could use this for naming the GPIO during its export
> if we decide to allow this in a near-future patch.
> 
Right.

> > +
> > +A GPIO controller can request GPIO hogs using the "gpio-hogs" property, which
> > +encodes a list of requested GPIO hogs.
> > +
> >  Example of two SOC GPIO banks defined as gpio-controller nodes:
> >
> >         qe_pio_a: gpio-controller@1400 {
> > @@ -110,6 +130,19 @@ Example of two SOC GPIO banks defined as gpio-controller nodes:
> >                 reg = <0x1400 0x18>;
> >                 gpio-controller;
> >                 #gpio-cells = <2>;
> > +               gpio-hogs = <&line_b>;
> > +
> > +               /* line_a hog is defined but not enabled in this example*/
> > +               line_a: line_a {
> > +                       gpios = <5 0>;
> > +                       input;
> > +               };
> > +
> > +               line_b: line_b {
> > +                       gpios = <6 0>;
> > +                       output-low;
> > +                       line-name = "foo-bar-gpio";
> > +               };
> 
> I think it might be good to group the hog nodes such as to allow
> complex configurations to be set in one go, à la pinmux:

See below.
> 
>                 gpio-controller;
>                 #gpio-cells = <2>;
> +               gpio-hogs = <&line_b>;
> +
> +               line_a: line_a {
> +                      line_a {
> +                               gpios = <5 0>;
> +                               input;
> +                      };
> +                      line_c {
> +                               gpios = <7 0>;
> +                               inputl
> +                      };
> +               };
> +
> +               line_b: line_b {
> +                       line_b {
> +                               gpios = <6 0>;
> +                               output-low;
> +                               line-name = "foo-bar-gpio";
> +                       };
> +               };
> 
> Here if you want to enable the first configuration you don't need to
> specify all the lines one by one, you just need to select the right
> group.
> 
> I wonder if such usage of child nodes could not interfere with GPIO
> drivers (existing or to be) that need to use child nodes for other
> purposes. Right now there is no way to distinguish a hog node from a
> node that serves another purpose, and that might become a problem in
> the future. Not sure how that should be addressed though - maybe by
> enforcing a "-hog" suffix for the group nodes, e.g. line_a-hog?
> Comments from people more experienced with DT would be nice.

I did consider a "pinmux" flavored format (not sure how hard to parse it would be).
It would allow grouping if nothing else.

	/* Line syntax: line_name <gpio# flags> direction-value [export] */
	gpio-hogs = <&group_y>;

	group_y: group_y {
		gpio-hogs-group = <
			line_x <15 0> output-low
			line_y <16 0> output-high export
			line_z <17 0> input
		>;
	};

> 
> >         };
> >
> >         qe_pio_e: gpio-controller@1460 {
> > diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> > index 604dbe6..7308dfc 100644
> > --- a/drivers/gpio/gpiolib-of.c
> > +++ b/drivers/gpio/gpiolib-of.c
> > @@ -111,6 +111,105 @@ 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
> > + * @index:     index of the GPIO
> > + * @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.
> > + */
> > +struct gpio_desc *of_get_gpio_hog(struct device_node *np, int index,
> > +                                 const char **name, unsigned long *flags)
> > +{
> > +       struct device_node *cfg_np, *chip_np;
> > +       enum of_gpio_flags xlate_flags;
> > +       unsigned long req_flags = 0;
> > +       struct gpio_desc *desc;
> > +       struct gg_data gg_data = {
> > +               .flags = &xlate_flags,
> > +               .out_gpio = NULL,
> > +       };
> > +       u32 tmp;
> > +       int i;
> > +       int ret;
> > +
> > +       cfg_np = of_parse_phandle(np, "gpio-hogs", index);
> > +       if (!cfg_np)
> > +               return ERR_PTR(-EINVAL);
> > +
> > +       chip_np = cfg_np->parent;
> > +       if (!chip_np) {
> > +               desc = ERR_PTR(-EINVAL);
> > +               goto out;
> > +       }
> > +
> > +       ret = of_property_read_u32(chip_np, "#gpio-cells", &tmp);
> > +       if (ret) {
> > +               desc = ERR_PTR(ret);
> > +               goto out;
> > +       }
> > +
> > +       if (tmp > MAX_PHANDLE_ARGS) {
> > +               desc = ERR_PTR(-EINVAL);
> > +               goto out;
> > +       }
> > +
> > +       gg_data.gpiospec.args_count = tmp;
> > +       gg_data.gpiospec.np = chip_np;
> > +       for (i = 0; i < tmp; i++) {
> > +               ret = of_property_read_u32(cfg_np, "gpios",
> > +                                          &gg_data.gpiospec.args[i]);
> > +               if (ret) {
> > +                       desc = ERR_PTR(ret);
> > +                       goto out;
> > +               }
> > +       }
> > +
> > +       gpiochip_find(&gg_data, of_gpiochip_find_and_xlate);
> > +       if (!gg_data.out_gpio) {
> > +               if (cfg_np->parent == np)
> > +                       desc = ERR_PTR(-ENXIO);
> > +               else
> > +                       desc = ERR_PTR(-EPROBE_DEFER);
> > +
> > +               goto out;
> > +       }
> > +
> > +       if (xlate_flags & OF_GPIO_ACTIVE_LOW)
> > +               req_flags |= GPIOF_ACTIVE_LOW;
> > +
> > +       if (of_property_read_bool(cfg_np, "input")) {
> > +               req_flags |= GPIOF_DIR_IN;
> > +       } else if (of_property_read_bool(cfg_np, "output-high")) {
> > +               req_flags |= GPIOF_INIT_HIGH;
> > +       } else if (!of_property_read_bool(cfg_np, "output-low")) {
> > +               desc = ERR_PTR(-EINVAL);
> > +               goto out;
> > +       }
> 
> That's why I would prefer a "direction" property instead of these 3
> ones: because if you have the following node:
> 
> line_foo {
>         gpios = <1 GPIO_ACTIVE_HIGH>;
>         input;
>         output-high;
> };
> 
> Then this code will not trigger an error and will just configure the
> GPIO as input.

Understood.

> 
> > +
> > +       if (of_property_read_bool(cfg_np, "open-drain-line"))
> > +               req_flags |= GPIOF_OPEN_DRAIN;
> > +
> > +       if (of_property_read_bool(cfg_np, "open-source-line"))
> > +               req_flags |= GPIOF_OPEN_DRAIN;
> > +
> > +       if (name && of_property_read_string(cfg_np, "line-name", name))
> > +               *name = cfg_np->name;
> > +
> > +       if (flags)
> > +               *flags = req_flags;
> > +
> > +       desc = gg_data.out_gpio;
> > +
> > +out:
> > +       of_node_put(cfg_np);
> > +
> > +       return desc;
> > +}
> > +
> > +/**
> >   * 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
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index e8e98ca..b6f5bdb 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -55,6 +55,9 @@ static DEFINE_MUTEX(gpio_lookup_lock);
> >  static LIST_HEAD(gpio_lookup_list);
> >  LIST_HEAD(gpio_chips);
> >
> > +struct gpio_desc *__must_check gpiod_get_hog_index(struct device *dev,
> > +                                                  unsigned int idx);
> > +
> >  static inline void desc_set_label(struct gpio_desc *d, const char *label)
> >  {
> >         d->label = label;
> > @@ -226,6 +229,8 @@ int gpiochip_add(struct gpio_chip *chip)
> >         int             status = 0;
> >         unsigned        id;
> >         int             base = chip->base;
> > +       struct gpio_desc        *desc;
> > +       int             i;
> >
> >         if ((!gpio_is_valid(base) || !gpio_is_valid(base + chip->ngpio - 1))
> >                         && base >= 0) {
> > @@ -275,6 +280,16 @@ int gpiochip_add(struct gpio_chip *chip)
> >         of_gpiochip_add(chip);
> >         acpi_gpiochip_add(chip);
> >
> > +       /*
> > +        * Instantiate GPIO hogs
> > +        * There shouldn't be mores hogs then gpio available on this chip
> 
> s/then/than

Will fix.

> 
> > +        */
> > +       for (i = 0; i < chip->ngpio; i++) {
> > +               desc = gpiod_get_hog_index(chip->dev, i);
> > +               if (IS_ERR(desc))
> > +                       break;
> > +       }
> 
> chip->ngpio? Isn't there a better way to know the number of phandles
> in your gpio-hogs property?

Maybe there is. I'll look into it.

> 
> Also you may have an error returned either because you reached the end
> of the list, or because the hog configuration itself failed. In the
> latter case don't you want to continue to go through the list?
Understood.

> 
> Finally your desc variable should be declared within this loop since
> it is not used elsewhere.
> 
Understood.

> > +
> >         if (status)
> >                 goto fail;
> >
> > @@ -1742,6 +1757,72 @@ struct gpio_desc *__must_check __gpiod_get_index_optional(struct device *dev,
> >  EXPORT_SYMBOL_GPL(__gpiod_get_index_optional);
> >
> >  /**
> > + * gpiod_get_hog_index - obtain a GPIO from a multi-index GPIO hog
> > + * @dev:       GPIO consumer
> > + * @idx:       index of the GPIO to obtain
> > + *
> > + * This should only be used by the gpiochip_add to request/set GPIO initial
> > + * configuration.
> > + *
> > + * Return a valid GPIO descriptor, or an IS_ERR() condition in case of error.
> > + */
> > +struct gpio_desc *__must_check gpiod_get_hog_index(struct device *dev,
> > +                                                  unsigned int idx)
> > +{
> > +       struct gpio_desc *desc = NULL;
> > +       int err;
> > +       unsigned long flags;
> > +       const char *name;
> > +
> > +       /* Using device tree? */
> > +       if (IS_ENABLED(CONFIG_OF) && dev && dev->of_node)
> > +               desc = of_get_gpio_hog(dev->of_node, idx, &name, &flags);
> > +
> > +       if (!desc)
> > +               return ERR_PTR(-ENOTSUPP);
> > +       else if (IS_ERR(desc))
> > +               return desc;
> > +
> > +       dev_dbg(dev, "gpio-hog: GPIO:%d (%s) as %s%s\n",
> > +               desc_to_gpio(desc), name, (flags&GPIOF_DIR_IN)?"input":"output",
> > +               (flags&GPIOF_DIR_IN)?"":(flags&GPIOF_INIT_HIGH)?"/high":"/low");
> > +
> 
> ...

I guess this means to remove the dev_dbg code?

> 
> > +       err = gpiod_request(desc, name);
> > +       if (err)
> > +               return ERR_PTR(err);
> > +
> > +       if (flags & GPIOF_OPEN_DRAIN)
> > +               set_bit(FLAG_OPEN_DRAIN, &desc->flags);
> > +
> > +       if (flags & GPIOF_OPEN_SOURCE)
> > +               set_bit(FLAG_OPEN_SOURCE, &desc->flags);
> > +
> > +       if (flags & GPIOF_ACTIVE_LOW)
> > +               set_bit(FLAG_ACTIVE_LOW, &desc->flags);
> > +
> > +       if (flags & GPIOF_DIR_IN)
> > +               err = gpiod_direction_input(desc);
> > +       else
> > +               err = gpiod_direction_output_raw(desc,
> > +                               (flags & GPIOF_INIT_HIGH) ? 1 : 0);
> > +
> 
> This part of the code is very similar to what is found in
> __gpiod_get_index. Could you maybe try to extract the common code into
> its own function and call it from both sites instead of duplicating
> code?

Agreed.
Originally I was making use of gpio_request_one, but then I noticed it was move to the "_legacy" and would probably dissapear.

> 
> > +       if (err)
> > +               goto free_gpio;
> > +
> > +       if (flags & GPIOF_EXPORT) {
> > +               err = gpiod_export(desc, flags & GPIOF_EXPORT_CHANGEABLE);
> > +               if (err)
> > +                       goto free_gpio;
> 
> Right now export is not possible so I don't think you need that block.

Unless the export feature is requested along with this pacth.

> 
> > +       }
> > +
> > +       return desc;
> > +
> > +free_gpio:
> > +       gpiod_free(desc);
> > +       return ERR_PTR(err);
> > +}
> > +
> > +/**
> >   * gpiod_put - dispose of a GPIO descriptor
> >   * @desc:      GPIO descriptor to dispose of
> >   *
> > diff --git a/include/linux/of_gpio.h b/include/linux/of_gpio.h
> > index 38fc050..52d154c 100644
> > --- a/include/linux/of_gpio.h
> > +++ b/include/linux/of_gpio.h
> > @@ -59,6 +59,9 @@ extern int of_gpio_simple_xlate(struct gpio_chip *gc,
> >                                 const struct of_phandle_args *gpiospec,
> >                                 u32 *flags);
> >
> > +extern struct gpio_desc *of_get_gpio_hog(struct device_node *np, int index,
> > +                                        const char **name,
> > +                                        unsigned long *flags);
> 
> This function is gpiolib-private, therefore it should be declared in
> drivers/gpio/gpiolib.h alongside with the declaration of
> of_get_named_gpiod_flags.

Ah yes would be much better.

> 
> If I understood the latest discussions correctly we might want to add
> an export (and named export) feature on top of this patch. Linus, am I
> correct to understand that you are not opposed to both features? In
> this case, we might want to go ahead with the merging of one of the
> named GPIOs patches, so that Jiri and Pantelis can implement export
> based on this patch.
--
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: [RFC Patch] gpio: add GPIO hogging mechanism

[Alexandre Courbot]

On Thu, Oct 30, 2014 at 1:21 AM, Benoit Parrot <bparrot@ti.com> wrote:
>> > +
>> >         if (status)
>> >                 goto fail;
>> >
>> > @@ -1742,6 +1757,72 @@ struct gpio_desc *__must_check __gpiod_get_index_optional(struct device *dev,
>> >  EXPORT_SYMBOL_GPL(__gpiod_get_index_optional);
>> >
>> >  /**
>> > + * gpiod_get_hog_index - obtain a GPIO from a multi-index GPIO hog
>> > + * @dev:       GPIO consumer
>> > + * @idx:       index of the GPIO to obtain
>> > + *
>> > + * This should only be used by the gpiochip_add to request/set GPIO initial
>> > + * configuration.
>> > + *
>> > + * Return a valid GPIO descriptor, or an IS_ERR() condition in case of error.
>> > + */
>> > +struct gpio_desc *__must_check gpiod_get_hog_index(struct device *dev,
>> > +                                                  unsigned int idx)
>> > +{
>> > +       struct gpio_desc *desc = NULL;
>> > +       int err;
>> > +       unsigned long flags;
>> > +       const char *name;
>> > +
>> > +       /* Using device tree? */
>> > +       if (IS_ENABLED(CONFIG_OF) && dev && dev->of_node)
>> > +               desc = of_get_gpio_hog(dev->of_node, idx, &name, &flags);
>> > +
>> > +       if (!desc)
>> > +               return ERR_PTR(-ENOTSUPP);
>> > +       else if (IS_ERR(desc))
>> > +               return desc;
>> > +
>> > +       dev_dbg(dev, "gpio-hog: GPIO:%d (%s) as %s%s\n",
>> > +               desc_to_gpio(desc), name, (flags&GPIOF_DIR_IN)?"input":"output",
>> > +               (flags&GPIOF_DIR_IN)?"":(flags&GPIOF_INIT_HIGH)?"/high":"/low");
>> > +
>>
>> ...
>
> I guess this means to remove the dev_dbg code?

No, it was just to delimitate the code which I suggested to factorize
into its own function below. :) This dev_dbg  is fine IMHO.

Re: [RFC Patch] gpio: add GPIO hogging mechanism

[Linus Walleij]

On Wed, Oct 29, 2014 at 8:09 AM, Alexandre Courbot <gnurou@gmail.com> wrote:
> On Wed, Oct 22, 2014 at 5:09 AM, Benoit Parrot <bparrot@ti.com> wrote:

> +               line_b: line_b {
> +                       line_b {
> +                               gpios = <6 0>;
> +                               output-low;
> +                               line-name = "foo-bar-gpio";
> +                       };
> +               };
>
(...)
>
> I wonder if such usage of child nodes could not interfere with GPIO
> drivers (existing or to be) that need to use child nodes for other
> purposes. Right now there is no way to distinguish a hog node from a
> node that serves another purpose, and that might become a problem in
> the future.

Yes, so I have suggested a hog-something; keyword in there.

As long as the children don't have any compatible-strings we can
decide pretty much how they should be handled internally.

Are there custom drivers with child nodes inside the main chip
today?

Yours,
Linus Walleij

Re: [RFC Patch] gpio: add GPIO hogging mechanism

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1380 bytes --]

On Fri, Nov 14, 2014 at 10:19:47AM +0100, Linus Walleij wrote:
> On Wed, Oct 29, 2014 at 8:09 AM, Alexandre Courbot <gnurou@gmail.com> wrote:
> > On Wed, Oct 22, 2014 at 5:09 AM, Benoit Parrot <bparrot@ti.com> wrote:
> 
> > +               line_b: line_b {
> > +                       line_b {
> > +                               gpios = <6 0>;
> > +                               output-low;
> > +                               line-name = "foo-bar-gpio";
> > +                       };
> > +               };
> >
> (...)
> >
> > I wonder if such usage of child nodes could not interfere with GPIO
> > drivers (existing or to be) that need to use child nodes for other
> > purposes. Right now there is no way to distinguish a hog node from a
> > node that serves another purpose, and that might become a problem in
> > the future.
> 
> Yes, so I have suggested a hog-something; keyword in there.
> 
> As long as the children don't have any compatible-strings we can
> decide pretty much how they should be handled internally.
> 
> Are there custom drivers with child nodes inside the main chip
> today?

o/

Our pinctrl driver is also our GPIO driver, so they both share the
same node.

Our pinctrl definitions are there.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC Patch] gpio: add GPIO hogging mechanism

[Pantelis Antoniou]

Hi Benoit,

> On Oct 21, 2014, at 23:09 , Benoit Parrot <bparrot@ti.com> wrote:
> 
> Based on Boris Brezillion work this is a reworked patch
> of his initial GPIO hogging mechanism.
> This patch provides a way to initally configure specific GPIO
> when the gpio controller is probe.
> 
> The actual DT scanning to collect the GPIO specific data is performed
> as part of the gpiochip_add().
> 
> The purpose of this is to allows specific GPIOs to be configured
> without any driver specific code.
> This particularly usueful because board design are getting
> increassingly complex and given SoC pins can now have upward
> of 10 mux values a lot of connections are now dependent on
> external IO muxes to switch various modes and combination.
> 
> 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.
> 

This look like it’s going to the right direction. I have a few general
comments at first.

1) It relies on dubious DT binding of having sub-nodes of the
gpio device implicitly defining hogs.

2) There is no way for having hogs inserted dynamically as far as I can tell, and
no way to remove a hog either.

3) I’m not very fond of having this being part of the gpio controller. This
configuration conceptually has little to do with the gpio controller per se,
it is more of a board specific thing. Why not come up with a gpio-hog driver that
references GPIOs? That way with a single gpio-hog driver instance you could setup
all the GPIO-hogging configuration for all GPIOs on the board, even one that
lie on different GPIO controllers.


> Signed-off-by: Benoit Parrot <bparrot@ti.com>
> ---
> Documentation/devicetree/bindings/gpio/gpio.txt | 33 +++++++++
> drivers/gpio/gpiolib-of.c                       | 99 +++++++++++++++++++++++++
> drivers/gpio/gpiolib.c                          | 81 ++++++++++++++++++++
> include/linux/of_gpio.h                         | 11 +++
> 4 files changed, 224 insertions(+)
> 

Regards

— Pantelis

--
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: [RFC Patch] gpio: add GPIO hogging mechanism

[Benoit Parrot]

Pantelis,

Thanks for the feedback.

Pantelis Antoniou <pantelis.antoniou@gmail.com> wrote on Wed [2014-Oct-29 10:53:44 +0200]:
> Hi Benoit,
> 
> > On Oct 21, 2014, at 23:09 , Benoit Parrot <bparrot@ti.com> wrote:
> > 
> > Based on Boris Brezillion work this is a reworked patch
> > of his initial GPIO hogging mechanism.
> > This patch provides a way to initally configure specific GPIO
> > when the gpio controller is probe.
> > 
> > The actual DT scanning to collect the GPIO specific data is performed
> > as part of the gpiochip_add().
> > 
> > The purpose of this is to allows specific GPIOs to be configured
> > without any driver specific code.
> > This particularly usueful because board design are getting
> > increassingly complex and given SoC pins can now have upward
> > of 10 mux values a lot of connections are now dependent on
> > external IO muxes to switch various modes and combination.
> > 
> > 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.
> > 
> 
> This look like it’s going to the right direction. I have a few general
> comments at first.
> 
> 1) It relies on dubious DT binding of having sub-nodes of the
> gpio device implicitly defining hogs.

I think in this instance the nodes are explicitly defining hogs.
Please clarify. What would you like to see here?
> 
> 2) There is no way for having hogs inserted dynamically as far as I can tell, and
> no way to remove a hog either.

The original patch was allowing that but, Linus's review comment suggested this feature be 
part of the gpio-controller's gpiochip_add() hook only.

> 
> 3) I’m not very fond of having this being part of the gpio controller. This
> configuration conceptually has little to do with the gpio controller per se,
> it is more of a board specific thing. Why not come up with a gpio-hog driver that
> references GPIOs? That way with a single gpio-hog driver instance you could setup
> all the GPIO-hogging configuration for all GPIOs on the board, even one that
> lie on different GPIO controllers.

Again this follows Linus's review comment.
I agree that it prevent a centralize spot where all hog would be defined but it has the advantages of not relying on PROBE_DEFER.
I mean since all "gpio-hogs" would be defined in a single board dts file it would not be that hard to figure out the big picture anyways.

> 
> 
> > Signed-off-by: Benoit Parrot <bparrot@ti.com>
> > ---
> > Documentation/devicetree/bindings/gpio/gpio.txt | 33 +++++++++
> > drivers/gpio/gpiolib-of.c                       | 99 +++++++++++++++++++++++++
> > drivers/gpio/gpiolib.c                          | 81 ++++++++++++++++++++
> > include/linux/of_gpio.h                         | 11 +++
> > 4 files changed, 224 insertions(+)
> > 
> 
> Regards
> 
> — Pantelis
> 

Regards,
Benoit
--
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: [RFC Patch] gpio: add GPIO hogging mechanism

[Pantelis Antoniou]

Hi Benoit,

> On Oct 29, 2014, at 18:34 , Benoit Parrot <bparrot@ti.com> wrote:
> 
> Pantelis,
> 
> Thanks for the feedback.
> 
> Pantelis Antoniou <pantelis.antoniou@gmail.com> wrote on Wed [2014-Oct-29 10:53:44 +0200]:
>> Hi Benoit,
>> 
>>> On Oct 21, 2014, at 23:09 , Benoit Parrot <bparrot@ti.com> wrote:
>>> 
>>> Based on Boris Brezillion work this is a reworked patch
>>> of his initial GPIO hogging mechanism.
>>> This patch provides a way to initally configure specific GPIO
>>> when the gpio controller is probe.
>>> 
>>> The actual DT scanning to collect the GPIO specific data is performed
>>> as part of the gpiochip_add().
>>> 
>>> The purpose of this is to allows specific GPIOs to be configured
>>> without any driver specific code.
>>> This particularly usueful because board design are getting
>>> increassingly complex and given SoC pins can now have upward
>>> of 10 mux values a lot of connections are now dependent on
>>> external IO muxes to switch various modes and combination.
>>> 
>>> 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.
>>> 
>> 
>> This look like it’s going to the right direction. I have a few general
>> comments at first.
>> 
>> 1) It relies on dubious DT binding of having sub-nodes of the
>> gpio device implicitly defining hogs.
> 
> I think in this instance the nodes are explicitly defining hogs.
> Please clarify. What would you like to see here?
>> 

Any subnodes are implicitly taken as hog definitions. This is not right because
gpio controllers might have subnodes that they use for another purpose.

>> 2) There is no way for having hogs inserted dynamically as far as I can tell, and
>> no way to remove a hog either.
> 
> The original patch was allowing that but, Linus's review comment suggested this feature be 
> part of the gpio-controller's gpiochip_add() hook only.
> 

If it’s not possible to remove a hog, then it’s no good for my use case in which
the gpios get exported and then removed.

>> 
>> 3) I’m not very fond of having this being part of the gpio controller. This
>> configuration conceptually has little to do with the gpio controller per se,
>> it is more of a board specific thing. Why not come up with a gpio-hog driver that
>> references GPIOs? That way with a single gpio-hog driver instance you could setup
>> all the GPIO-hogging configuration for all GPIOs on the board, even one that
>> lie on different GPIO controllers.
> 
> Again this follows Linus's review comment.
> I agree that it prevent a centralize spot where all hog would be defined but it has the advantages of not relying on PROBE_DEFER.
> I mean since all "gpio-hogs" would be defined in a single board dts file it would not be that hard to figure out the big picture anyways.
> 

There won’t be a single board dts file if you’re using things like overlays.

>> 
>> 
>>> Signed-off-by: Benoit Parrot <bparrot@ti.com>
>>> ---
>>> Documentation/devicetree/bindings/gpio/gpio.txt | 33 +++++++++
>>> drivers/gpio/gpiolib-of.c                       | 99 +++++++++++++++++++++++++
>>> drivers/gpio/gpiolib.c                          | 81 ++++++++++++++++++++
>>> include/linux/of_gpio.h                         | 11 +++
>>> 4 files changed, 224 insertions(+)
>>> 
>> 
>> Regards
>> 
>> — Pantelis
>> 
> 
> Regards,
> Benoit

Regards

— Pantelis

Re: [RFC Patch] gpio: add GPIO hogging mechanism

[Benoit Parrot]

Pantelis Antoniou <pantelis.antoniou@gmail.com> wrote on Wed [2014-Oct-29 18:42:48 +0200]:
> Hi Benoit,
> 
> > On Oct 29, 2014, at 18:34 , Benoit Parrot <bparrot@ti.com> wrote:
> > 
> > Pantelis,
> > 
> > Thanks for the feedback.
> > 
> > Pantelis Antoniou <pantelis.antoniou@gmail.com> wrote on Wed [2014-Oct-29 10:53:44 +0200]:
> >> Hi Benoit,
> >> 
> >>> On Oct 21, 2014, at 23:09 , Benoit Parrot <bparrot@ti.com> wrote:
> >>> 
> >>> Based on Boris Brezillion work this is a reworked patch
> >>> of his initial GPIO hogging mechanism.
> >>> This patch provides a way to initally configure specific GPIO
> >>> when the gpio controller is probe.
> >>> 
> >>> The actual DT scanning to collect the GPIO specific data is performed
> >>> as part of the gpiochip_add().
> >>> 
> >>> The purpose of this is to allows specific GPIOs to be configured
> >>> without any driver specific code.
> >>> This particularly usueful because board design are getting
> >>> increassingly complex and given SoC pins can now have upward
> >>> of 10 mux values a lot of connections are now dependent on
> >>> external IO muxes to switch various modes and combination.
> >>> 
> >>> 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.
> >>> 
> >> 
> >> This look like it’s going to the right direction. I have a few general
> >> comments at first.
> >> 
> >> 1) It relies on dubious DT binding of having sub-nodes of the
> >> gpio device implicitly defining hogs.
> > 
> > I think in this instance the nodes are explicitly defining hogs.
> > Please clarify. What would you like to see here?
> >> 
> 
> Any subnodes are implicitly taken as hog definitions. This is not right because
> gpio controllers might have subnodes that they use for another purpose.

I think I see what you mean.
Even though "gpio-hog = <&phandle>" guarantee that only those sub-node
are going to be parsed, if a particular gpio-controller has sub-node for
some other reason then the "hog" related sub-node might be in the way.

Do you know of such an example currently in mainline I can take a look at?

> 
> >> 2) There is no way for having hogs inserted dynamically as far as I can tell, and
> >> no way to remove a hog either.
> > 
> > The original patch was allowing that but, Linus's review comment suggested this feature be 
> > part of the gpio-controller's gpiochip_add() hook only.
> > 
> 
> If it’s not possible to remove a hog, then it’s no good for my use case in which
> the gpios get exported and then removed.

I guess if we add the export feature then that could be possible.
But if I am not mistaken the hog concept was to allow gpio setting
for gpio not belonging to any particular drivers.
If you have gpio which needs to be released then would they fall in
the "own" by a driver category and which case use the exixting method? 
> 
> >> 
> >> 3) I’m not very fond of having this being part of the gpio controller. This
> >> configuration conceptually has little to do with the gpio controller per se,
> >> it is more of a board specific thing. Why not come up with a gpio-hog driver that
> >> references GPIOs? That way with a single gpio-hog driver instance you could setup
> >> all the GPIO-hogging configuration for all GPIOs on the board, even one that
> >> lie on different GPIO controllers.
> > 
> > Again this follows Linus's review comment.
> > I agree that it prevent a centralize spot where all hog would be defined but it has the advantages of not relying on PROBE_DEFER.
> > I mean since all "gpio-hogs" would be defined in a single board dts file it would not be that hard to figure out the big picture anyways.
> > 
> 
> There won’t be a single board dts file if you’re using things like overlays.
I see, I had not consider that.

A generic board level gpio driver would be a different option I guess.

> 
> >> 
> >> 
> >>> Signed-off-by: Benoit Parrot <bparrot@ti.com>
> >>> ---
> >>> Documentation/devicetree/bindings/gpio/gpio.txt | 33 +++++++++
> >>> drivers/gpio/gpiolib-of.c                       | 99 +++++++++++++++++++++++++
> >>> drivers/gpio/gpiolib.c                          | 81 ++++++++++++++++++++
> >>> include/linux/of_gpio.h                         | 11 +++
> >>> 4 files changed, 224 insertions(+)
> >>> 
> >> 
> >> Regards
> >> 
> >> — Pantelis
> >> 
> > 
> > Regards,
> > Benoit
> 
> Regards
> 
> — Pantelis
> 

Regards,
Benoit
--
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: [RFC Patch] gpio: add GPIO hogging mechanism

[Alexandre Courbot]

On Thu, Oct 30, 2014 at 1:42 AM, Pantelis Antoniou
<pantelis.antoniou@gmail.com> wrote:
> Hi Benoit,
>
>> On Oct 29, 2014, at 18:34 , Benoit Parrot <bparrot@ti.com> wrote:
>>
>> Pantelis,
>>
>> Thanks for the feedback.
>>
>> Pantelis Antoniou <pantelis.antoniou@gmail.com> wrote on Wed [2014-Oct-29 10:53:44 +0200]:
>>> Hi Benoit,
>>>
>>>> On Oct 21, 2014, at 23:09 , Benoit Parrot <bparrot@ti.com> wrote:
>>>>
>>>> Based on Boris Brezillion work this is a reworked patch
>>>> of his initial GPIO hogging mechanism.
>>>> This patch provides a way to initally configure specific GPIO
>>>> when the gpio controller is probe.
>>>>
>>>> The actual DT scanning to collect the GPIO specific data is performed
>>>> as part of the gpiochip_add().
>>>>
>>>> The purpose of this is to allows specific GPIOs to be configured
>>>> without any driver specific code.
>>>> This particularly usueful because board design are getting
>>>> increassingly complex and given SoC pins can now have upward
>>>> of 10 mux values a lot of connections are now dependent on
>>>> external IO muxes to switch various modes and combination.
>>>>
>>>> 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.
>>>>
>>>
>>> This look like it’s going to the right direction. I have a few general
>>> comments at first.
>>>
>>> 1) It relies on dubious DT binding of having sub-nodes of the
>>> gpio device implicitly defining hogs.
>>
>> I think in this instance the nodes are explicitly defining hogs.
>> Please clarify. What would you like to see here?
>>>
>
> Any subnodes are implicitly taken as hog definitions. This is not right because
> gpio controllers might have subnodes that they use for another purpose.
>
>>> 2) There is no way for having hogs inserted dynamically as far as I can tell, and
>>> no way to remove a hog either.
>>
>> The original patch was allowing that but, Linus's review comment suggested this feature be
>> part of the gpio-controller's gpiochip_add() hook only.
>>
>
> If it’s not possible to remove a hog, then it’s no good for my use case in which
> the gpios get exported and then removed.

Why would you want to remove a GPIO hog at runtime, and what is the
point of having it set in the DT in that case?
--
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: [RFC Patch] gpio: add GPIO hogging mechanism

[Linus Walleij]

On Wed, Oct 29, 2014 at 9:53 AM, Pantelis Antoniou
<pantelis.antoniou@gmail.com> wrote:

> 3) I’m not very fond of having this being part of the gpio controller. This
> configuration conceptually has little to do with the gpio controller per se,
> it is more of a board specific thing. Why not come up with a gpio-hog driver that
> references GPIOs? That way with a single gpio-hog driver instance you could setup
> all the GPIO-hogging configuration for all GPIOs on the board, even one that
> lie on different GPIO controllers.

You have a point. But it's vital that we set up hogs at the same time as the
gpio_chip is probed, not later, or there may be a race as to who
gets the GPIO line, and that should be avoided at any cost.

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: [RFC Patch] gpio: add GPIO hogging mechanism

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2028 bytes --]

Hi,

On Tue, Oct 21, 2014 at 03:09:58PM -0500, Benoit Parrot wrote:
> Based on Boris Brezillion work this is a reworked patch
> of his initial GPIO hogging mechanism.
> This patch provides a way to initally configure specific GPIO
> when the gpio controller is probe.
> 
> The actual DT scanning to collect the GPIO specific data is performed
> as part of the gpiochip_add().
> 
> The purpose of this is to allows specific GPIOs to be configured
> without any driver specific code.
> This particularly usueful because board design are getting
> increassingly complex and given SoC pins can now have upward
> of 10 mux values a lot of connections are now dependent on
> external IO muxes to switch various modes and combination.
> 
> 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>

I've been thinking about this for quite some time, it's good to see
some progress on that :)

However, I have a slightly different use case for it: the Allwinner
SoCs have a vdd pin coming in for every gpio bank. Nothing out of the
ordinary so far, except that some of the boards are using a
GPIO-controlled regulator to feed another bank vdd. That obviously
causes a chicken-egg issue, since for the gpio-regulator driver to
probe, it needs to gpio driver, and for the gpio driver to probe, it
needs the regulator driver.

I was thinking of solving this by enforcing gpio hogs, in order to
have the gpio driver loading, grabing its gpios setting them to the
right value to enable the current to flow in, and then let the
regulator driver probe later on.

I don't think it's possible with your current code, would it be
something worth considering, or would someone have a better solution?

Maxime
-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC Patch] gpio: add GPIO hogging mechanism

[Benoit Parrot]

Maxime Ripard <maxime.ripard@free-electrons.com> wrote on Wed [2014-Oct-29 11:45:59 +0100]:
> Hi,
> 
> On Tue, Oct 21, 2014 at 03:09:58PM -0500, Benoit Parrot wrote:
> > Based on Boris Brezillion work this is a reworked patch
> > of his initial GPIO hogging mechanism.
> > This patch provides a way to initally configure specific GPIO
> > when the gpio controller is probe.
> > 
> > The actual DT scanning to collect the GPIO specific data is performed
> > as part of the gpiochip_add().
> > 
> > The purpose of this is to allows specific GPIOs to be configured
> > without any driver specific code.
> > This particularly usueful because board design are getting
> > increassingly complex and given SoC pins can now have upward
> > of 10 mux values a lot of connections are now dependent on
> > external IO muxes to switch various modes and combination.
> > 
> > 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>
> 
> I've been thinking about this for quite some time, it's good to see
> some progress on that :)
> 
> However, I have a slightly different use case for it: the Allwinner
> SoCs have a vdd pin coming in for every gpio bank. Nothing out of the
> ordinary so far, except that some of the boards are using a
> GPIO-controlled regulator to feed another bank vdd. That obviously
> causes a chicken-egg issue, since for the gpio-regulator driver to
> probe, it needs to gpio driver, and for the gpio driver to probe, it
> needs the regulator driver.

Unless the gpio controlling the vdd pin is from the same bank your trying to power up
I do not see the issue here.

In this patch the gpio-hogs are setup as part of the gpio-controller probe function.

> 
> I was thinking of solving this by enforcing gpio hogs, in order to
> have the gpio driver loading, grabing its gpios setting them to the
> right value to enable the current to flow in, and then let the
> regulator driver probe later on.
> 
> I don't think it's possible with your current code, would it be
> something worth considering, or would someone have a better solution?

Unless I am missing something, this should work.

> 
> Maxime
> -- 
> Maxime Ripard, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com

Regards,
Benoit

Re: [RFC Patch] gpio: add GPIO hogging mechanism

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2108 bytes --]

On Wed, Oct 29, 2014 at 11:41:22AM -0500, Benoit Parrot wrote:
> Maxime Ripard <maxime.ripard@free-electrons.com> wrote on Wed [2014-Oct-29 11:45:59 +0100]:
> > Hi,
> > 
> > On Tue, Oct 21, 2014 at 03:09:58PM -0500, Benoit Parrot wrote:
> > > Based on Boris Brezillion work this is a reworked patch
> > > of his initial GPIO hogging mechanism.
> > > This patch provides a way to initally configure specific GPIO
> > > when the gpio controller is probe.
> > > 
> > > The actual DT scanning to collect the GPIO specific data is performed
> > > as part of the gpiochip_add().
> > > 
> > > The purpose of this is to allows specific GPIOs to be configured
> > > without any driver specific code.
> > > This particularly usueful because board design are getting
> > > increassingly complex and given SoC pins can now have upward
> > > of 10 mux values a lot of connections are now dependent on
> > > external IO muxes to switch various modes and combination.
> > > 
> > > 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>
> > 
> > I've been thinking about this for quite some time, it's good to see
> > some progress on that :)
> > 
> > However, I have a slightly different use case for it: the Allwinner
> > SoCs have a vdd pin coming in for every gpio bank. Nothing out of the
> > ordinary so far, except that some of the boards are using a
> > GPIO-controlled regulator to feed another bank vdd. That obviously
> > causes a chicken-egg issue, since for the gpio-regulator driver to
> > probe, it needs to gpio driver, and for the gpio driver to probe, it
> > needs the regulator driver.
> 
> Unless the gpio controlling the vdd pin is from the same bank your
> trying to power up I do not see the issue here.

Not the same bank, but the same driver.

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC Patch] gpio: add GPIO hogging mechanism

[Benoit Parrot]

Maxime Ripard <maxime.ripard@free-electrons.com> wrote on Wed [2014-Oct-29 17:47:46 +0100]:
> On Wed, Oct 29, 2014 at 11:41:22AM -0500, Benoit Parrot wrote:
> > Maxime Ripard <maxime.ripard@free-electrons.com> wrote on Wed [2014-Oct-29 11:45:59 +0100]:
> > > Hi,
> > > 
> > > On Tue, Oct 21, 2014 at 03:09:58PM -0500, Benoit Parrot wrote:
> > > > Based on Boris Brezillion work this is a reworked patch
> > > > of his initial GPIO hogging mechanism.
> > > > This patch provides a way to initally configure specific GPIO
> > > > when the gpio controller is probe.
> > > > 
> > > > The actual DT scanning to collect the GPIO specific data is performed
> > > > as part of the gpiochip_add().
> > > > 
> > > > The purpose of this is to allows specific GPIOs to be configured
> > > > without any driver specific code.
> > > > This particularly usueful because board design are getting
> > > > increassingly complex and given SoC pins can now have upward
> > > > of 10 mux values a lot of connections are now dependent on
> > > > external IO muxes to switch various modes and combination.
> > > > 
> > > > 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>
> > > 
> > > I've been thinking about this for quite some time, it's good to see
> > > some progress on that :)
> > > 
> > > However, I have a slightly different use case for it: the Allwinner
> > > SoCs have a vdd pin coming in for every gpio bank. Nothing out of the
> > > ordinary so far, except that some of the boards are using a
> > > GPIO-controlled regulator to feed another bank vdd. That obviously
> > > causes a chicken-egg issue, since for the gpio-regulator driver to
> > > probe, it needs to gpio driver, and for the gpio driver to probe, it
> > > needs the regulator driver.
> > 
> > Unless the gpio controlling the vdd pin is from the same bank your
> > trying to power up I do not see the issue here.
> 
> Not the same bank, but the same driver.

How are you currently working around this issue?

> 
> -- 
> Maxime Ripard, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com

Re: [RFC Patch] gpio: add GPIO hogging mechanism

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3233 bytes --]

On Wed, Oct 29, 2014 at 06:09:12PM -0500, Benoit Parrot wrote:
> Maxime Ripard <maxime.ripard@free-electrons.com> wrote on Wed [2014-Oct-29 17:47:46 +0100]:
> > On Wed, Oct 29, 2014 at 11:41:22AM -0500, Benoit Parrot wrote:
> > > Maxime Ripard <maxime.ripard@free-electrons.com> wrote on Wed [2014-Oct-29 11:45:59 +0100]:
> > > > Hi,
> > > > 
> > > > On Tue, Oct 21, 2014 at 03:09:58PM -0500, Benoit Parrot wrote:
> > > > > Based on Boris Brezillion work this is a reworked patch
> > > > > of his initial GPIO hogging mechanism.
> > > > > This patch provides a way to initally configure specific GPIO
> > > > > when the gpio controller is probe.
> > > > > 
> > > > > The actual DT scanning to collect the GPIO specific data is performed
> > > > > as part of the gpiochip_add().
> > > > > 
> > > > > The purpose of this is to allows specific GPIOs to be configured
> > > > > without any driver specific code.
> > > > > This particularly usueful because board design are getting
> > > > > increassingly complex and given SoC pins can now have upward
> > > > > of 10 mux values a lot of connections are now dependent on
> > > > > external IO muxes to switch various modes and combination.
> > > > > 
> > > > > 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>
> > > > 
> > > > I've been thinking about this for quite some time, it's good to see
> > > > some progress on that :)
> > > > 
> > > > However, I have a slightly different use case for it: the Allwinner
> > > > SoCs have a vdd pin coming in for every gpio bank. Nothing out of the
> > > > ordinary so far, except that some of the boards are using a
> > > > GPIO-controlled regulator to feed another bank vdd. That obviously
> > > > causes a chicken-egg issue, since for the gpio-regulator driver to
> > > > probe, it needs to gpio driver, and for the gpio driver to probe, it
> > > > needs the regulator driver.
> > > 
> > > Unless the gpio controlling the vdd pin is from the same bank your
> > > trying to power up I do not see the issue here.
> > 
> > Not the same bank, but the same driver.
> 
> How are you currently working around this issue?

For now, we are not, which is exactly why I'm interested in such a
mechanism :)

What I was thinking about for this case would be to "fake" the fact
that the GPIO is even there. The regulator driver would probe, claim
the GPIO, without actually doing anything more than just storing the
value to set, which would be set in the hardware only when the GPIO
driver probes.

I'm not very happy about it though, because that would mean that
drivers that require more than just a value assignment (for example
set high, wait, set low, for a reset for example) wouldn't even know
that what they expect didn't happen.

Maybe that could work by passing some kind of flag to gpio_request to
trigger that mecanism, I don't know.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC Patch] gpio: add GPIO hogging mechanism

[Linus Walleij]

On Tue, Oct 21, 2014 at 10:09 PM, Benoit Parrot <bparrot@ti.com> wrote:

> Based on Boris Brezillion work this is a reworked patch
> of his initial GPIO hogging mechanism.
> This patch provides a way to initally configure specific GPIO
> when the gpio controller is probe.
>
> The actual DT scanning to collect the GPIO specific data is performed
> as part of the gpiochip_add().
>
> The purpose of this is to allows specific GPIOs to be configured
> without any driver specific code.
> This particularly usueful because board design are getting
> increassingly complex and given SoC pins can now have upward
> of 10 mux values a lot of connections are now dependent on
> external IO muxes to switch various modes and combination.
>
> 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>

Thanks for working on this.

> diff --git a/Documentation/devicetree/bindings/gpio/gpio.txt b/Documentation/devicetree/bindings/gpio/gpio.txt
> index 3fb8f53..a9700d8 100644
> --- a/Documentation/devicetree/bindings/gpio/gpio.txt
> +++ b/Documentation/devicetree/bindings/gpio/gpio.txt
> @@ -103,6 +103,26 @@ 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.
>
> +It might 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.

The GPIO chip may contain... you cannot start a sentence with "It"

> +Each GPIO hog definition is represented as a child node of the GPIO controller.
> +Required properties:
> +- gpios: store the gpio information (id, flags, ...). Shall contain the
> +  number of cells specified in its parent node (GPIO controller node).
> +- input: a property specifying to set the GPIO direction as input.
> +- output-high: a property specifying to set the GPIO direction to output with
> +  the value high.
> +- output-low: a property specifying to set the GPIO direction to output with
> +  the value low.
> +
> +Optional properties:
> +- line-name: the GPIO label name. If not present the node name is used.
> +
> +A GPIO controller can request GPIO hogs using the "gpio-hogs" property, which
> +encodes a list of requested GPIO hogs.
> +
>  Example of two SOC GPIO banks defined as gpio-controller nodes:
>
>         qe_pio_a: gpio-controller@1400 {
> @@ -110,6 +130,19 @@ Example of two SOC GPIO banks defined as gpio-controller nodes:
>                 reg = <0x1400 0x18>;
>                 gpio-controller;
>                 #gpio-cells = <2>;
> +               gpio-hogs = <&line_b>;
> +
> +               /* line_a hog is defined but not enabled in this example*/
> +               line_a: line_a {
> +                       gpios = <5 0>;
> +                       input;
> +               };
> +
> +               line_b: line_b {
> +                       gpios = <6 0>;
> +                       output-low;
> +                       line-name = "foo-bar-gpio";
> +               };


I don't see the point of having unused hogs and enabling them using
phandles.

Just let the core walk over all children nodes of a GPIO controller
and hog them. Put in a bool property saying it's a hog.

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

I don't quite see the point with input hogs that noone can use
but whatever.

I am thinking that maybe the line name should be compulsory
so as to improbe readability. I mean there is always a reason
why you're hogging a pin and the name should say it.

>  /**
> + * of_get_gpio_hog() - Get a GPIO hog descriptor, names and flags for GPIO API
> + * @np:                device node to get GPIO from
> + * @index:     index of the GPIO
> + * @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.
> + */
> +struct gpio_desc *of_get_gpio_hog(struct device_node *np, int index,
> +                                 const char **name, unsigned long *flags)
> +{
> +       struct device_node *cfg_np, *chip_np;
> +       enum of_gpio_flags xlate_flags;
> +       unsigned long req_flags = 0;
> +       struct gpio_desc *desc;
> +       struct gg_data gg_data = {
> +               .flags = &xlate_flags,
> +               .out_gpio = NULL,
> +       };
> +       u32 tmp;
> +       int i;
> +       int ret;
> +
> +       cfg_np = of_parse_phandle(np, "gpio-hogs", index);
> +       if (!cfg_np)
> +               return ERR_PTR(-EINVAL);

So no phandles please.

> +       chip_np = cfg_np->parent;
> +       if (!chip_np) {
> +               desc = ERR_PTR(-EINVAL);
> +               goto out;
> +       }
> +
> +       ret = of_property_read_u32(chip_np, "#gpio-cells", &tmp);
> +       if (ret) {
> +               desc = ERR_PTR(ret);
> +               goto out;
> +       }
> +
> +       if (tmp > MAX_PHANDLE_ARGS) {
> +               desc = ERR_PTR(-EINVAL);
> +               goto out;
> +       }
> +
> +       gg_data.gpiospec.args_count = tmp;
> +       gg_data.gpiospec.np = chip_np;
> +       for (i = 0; i < tmp; i++) {
> +               ret = of_property_read_u32(cfg_np, "gpios",
> +                                          &gg_data.gpiospec.args[i]);
> +               if (ret) {
> +                       desc = ERR_PTR(ret);
> +                       goto out;
> +               }
> +       }
> +
> +       gpiochip_find(&gg_data, of_gpiochip_find_and_xlate);
> +       if (!gg_data.out_gpio) {
> +               if (cfg_np->parent == np)
> +                       desc = ERR_PTR(-ENXIO);
> +               else
> +                       desc = ERR_PTR(-EPROBE_DEFER);
> +
> +               goto out;
> +       }
> +
> +       if (xlate_flags & OF_GPIO_ACTIVE_LOW)
> +               req_flags |= GPIOF_ACTIVE_LOW;
> +
> +       if (of_property_read_bool(cfg_np, "input")) {
> +               req_flags |= GPIOF_DIR_IN;
> +       } else if (of_property_read_bool(cfg_np, "output-high")) {
> +               req_flags |= GPIOF_INIT_HIGH;
> +       } else if (!of_property_read_bool(cfg_np, "output-low")) {
> +               desc = ERR_PTR(-EINVAL);
> +               goto out;
> +       }
> +
> +       if (of_property_read_bool(cfg_np, "open-drain-line"))
> +               req_flags |= GPIOF_OPEN_DRAIN;
> +
> +       if (of_property_read_bool(cfg_np, "open-source-line"))
> +               req_flags |= GPIOF_OPEN_DRAIN;
> +
> +       if (name && of_property_read_string(cfg_np, "line-name", name))
> +               *name = cfg_np->name;
> +
> +       if (flags)
> +               *flags = req_flags;
> +
> +       desc = gg_data.out_gpio;
> +
> +out:
> +       of_node_put(cfg_np);
> +
> +       return desc;
> +}
> +
> +/**
>   * 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
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index e8e98ca..b6f5bdb 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -55,6 +55,9 @@ static DEFINE_MUTEX(gpio_lookup_lock);
>  static LIST_HEAD(gpio_lookup_list);
>  LIST_HEAD(gpio_chips);
>
> +struct gpio_desc *__must_check gpiod_get_hog_index(struct device *dev,
> +                                                  unsigned int idx);
> +
>  static inline void desc_set_label(struct gpio_desc *d, const char *label)
>  {
>         d->label = label;
> @@ -226,6 +229,8 @@ int gpiochip_add(struct gpio_chip *chip)
>         int             status = 0;
>         unsigned        id;
>         int             base = chip->base;
> +       struct gpio_desc        *desc;
> +       int             i;
>
>         if ((!gpio_is_valid(base) || !gpio_is_valid(base + chip->ngpio - 1))
>                         && base >= 0) {
> @@ -275,6 +280,16 @@ int gpiochip_add(struct gpio_chip *chip)
>         of_gpiochip_add(chip);

Alter the body of of_gpiochip_add() instead of adding more code here
in the core gpiolib. You need not patch a single line of this code.

>         acpi_gpiochip_add(chip);
>
> +       /*
> +        * Instantiate GPIO hogs
> +        * There shouldn't be mores hogs then gpio available on this chip
> +        */
> +       for (i = 0; i < chip->ngpio; i++) {
> +               desc = gpiod_get_hog_index(chip->dev, i);
> +               if (IS_ERR(desc))
> +                       break;
> +       }

Ick that is not elegant.

Instead use

struct device_node *child;

for_each_child_of_node(chip->dev.of_node, np) {
    if (!of_property_read_bool(np, "gpio-hog"))
       continue;
    /* Do the hogging */
}

> @@ -1742,6 +1757,72 @@ struct gpio_desc *__must_check __gpiod_get_index_optional(struct device *dev,
>  EXPORT_SYMBOL_GPL(__gpiod_get_index_optional);
>
>  /**
> + * gpiod_get_hog_index - obtain a GPIO from a multi-index GPIO hog
> + * @dev:       GPIO consumer
> + * @idx:       index of the GPIO to obtain
> + *
> + * This should only be used by the gpiochip_add to request/set GPIO initial
> + * configuration.
> + *
> + * Return a valid GPIO descriptor, or an IS_ERR() condition in case of error.
> + */
> +struct gpio_desc *__must_check gpiod_get_hog_index(struct device *dev,
> +                                                  unsigned int idx)
> +{
> +       struct gpio_desc *desc = NULL;
> +       int err;
> +       unsigned long flags;
> +       const char *name;
> +
> +       /* Using device tree? */
> +       if (IS_ENABLED(CONFIG_OF) && dev && dev->of_node)
> +               desc = of_get_gpio_hog(dev->of_node, idx, &name, &flags);
> +
> +       if (!desc)
> +               return ERR_PTR(-ENOTSUPP);
> +       else if (IS_ERR(desc))
> +               return desc;
> +
> +       dev_dbg(dev, "gpio-hog: GPIO:%d (%s) as %s%s\n",
> +               desc_to_gpio(desc), name, (flags&GPIOF_DIR_IN)?"input":"output",
> +               (flags&GPIOF_DIR_IN)?"":(flags&GPIOF_INIT_HIGH)?"/high":"/low");
> +
> +       err = gpiod_request(desc, name);
> +       if (err)
> +               return ERR_PTR(err);
> +
> +       if (flags & GPIOF_OPEN_DRAIN)
> +               set_bit(FLAG_OPEN_DRAIN, &desc->flags);
> +
> +       if (flags & GPIOF_OPEN_SOURCE)
> +               set_bit(FLAG_OPEN_SOURCE, &desc->flags);
> +
> +       if (flags & GPIOF_ACTIVE_LOW)
> +               set_bit(FLAG_ACTIVE_LOW, &desc->flags);
> +
> +       if (flags & GPIOF_DIR_IN)
> +               err = gpiod_direction_input(desc);
> +       else
> +               err = gpiod_direction_output_raw(desc,
> +                               (flags & GPIOF_INIT_HIGH) ? 1 : 0);
> +
> +       if (err)
> +               goto free_gpio;
> +
> +       if (flags & GPIOF_EXPORT) {
> +               err = gpiod_export(desc, flags & GPIOF_EXPORT_CHANGEABLE);
> +               if (err)
> +                       goto free_gpio;
> +       }
> +
> +       return desc;
> +
> +free_gpio:
> +       gpiod_free(desc);
> +       return ERR_PTR(err);
> +}

I don't see the point of the device tree code calling into the core to
set up the hog if currently only device tree can do this.

Keep all the code in gpiolib-of.c and make it a static call for now.

If ACPI needs the same we can refactor it later.

> +
> +/**
>   * gpiod_put - dispose of a GPIO descriptor
>   * @desc:      GPIO descriptor to dispose of
>   *
> diff --git a/include/linux/of_gpio.h b/include/linux/of_gpio.h
> index 38fc050..52d154c 100644
> --- a/include/linux/of_gpio.h
> +++ b/include/linux/of_gpio.h
> @@ -59,6 +59,9 @@ extern int of_gpio_simple_xlate(struct gpio_chip *gc,
>                                 const struct of_phandle_args *gpiospec,
>                                 u32 *flags);
>
> +extern struct gpio_desc *of_get_gpio_hog(struct device_node *np, int index,
> +                                        const char **name,
> +                                        unsigned long *flags);


No, if this is a gpiolib-internal call it should not be in the public
header at all.

Move to drivers/gpio/gpiolib.h, stubs and all.

Yours,
Linus Walleij

Re: [RFC Patch] gpio: add GPIO hogging mechanism

[Benoit Parrot]

Linus,

Thanks for the feedback.

To summarize the hog feature should be local to gpiolib-of.c, correct?

I also also need some clarifications, see below.


Linus Walleij <linus.walleij@linaro.org> wrote on Mon [2014-Nov-03 10:59:53 +0100]:
> On Tue, Oct 21, 2014 at 10:09 PM, Benoit Parrot <bparrot@ti.com> wrote:
> 
> >         qe_pio_a: gpio-controller@1400 {
> > @@ -110,6 +130,19 @@ Example of two SOC GPIO banks defined as gpio-controller nodes:
> >                 reg = <0x1400 0x18>;
> >                 gpio-controller;
> >                 #gpio-cells = <2>;
> > +               gpio-hogs = <&line_b>;
> > +
> > +               /* line_a hog is defined but not enabled in this example*/
> > +               line_a: line_a {
> > +                       gpios = <5 0>;
> > +                       input;
> > +               };
> > +
> > +               line_b: line_b {
> > +                       gpios = <6 0>;
> > +                       output-low;
> > +                       line-name = "foo-bar-gpio";
> > +               };
> 
> 
> I don't see the point of having unused hogs and enabling them using
> phandles.
> 
> Just let the core walk over all children nodes of a GPIO controller
> and hog them. Put in a bool property saying it's a hog.
> 
> +               line_b: line_b {
> +                       gpio-hog;
> +                       gpios = <6 0>;
> +                       output-low;
> +                       line-name = "foo-bar-gpio";
> +               };
> 
> I don't quite see the point with input hogs that noone can use
> but whatever.
> 
> I am thinking that maybe the line name should be compulsory
> so as to improbe readability. I mean there is always a reason
> why you're hogging a pin and the name should say it.

Ok, so as an alternative I had presented something like this in my reply
to Alexandre Courbot's review comments:

  I did consider a "pinmux" flavored format (not sure how hard to parse it would be).
  It would allow grouping if nothing else.

  	/* Line syntax: line_name <gpio# flags> direction-value [export] */
	gpio-hogs = <&group_y>;

	group_y: group_y {
		gpio-hogs-group = <
			line_x <15 0> output-low
			line_y <16 0> output-high export
			line_z <17 0> input
		>;
	};

Now based on your comment would something like this work?

     qe_pio_a: gpio-controller@1400 {
	reg = <0x1400 0x18>;
	gpio-controller;
	#gpio-cells = <2>;

  	/* Line syntax: line_name <gpio# flags> direction-value [export] */
	gpio-hogs: {
		gpio-hogs-group = <
			foo-bar-gpio <15 0> output-low
			bar-foo-gpio <16 0> output-high export
		>;
	};
     };

This would group all hogs for one controller under a single child node.
Again I am not sure how feasible or easy to implement the DT parsing would be. 

I guess for completeness if you could also comment on my reply to Alexandre from Oct 29th,
that would be great, before I head in the wrong directions. 

Regards,
Benoit

Re: [RFC Patch] gpio: add GPIO hogging mechanism

[Linus Walleij]

On Tue, Nov 4, 2014 at 1:38 AM, Benoit Parrot <bparrot-l0cyMroinI0@public.gmane.org> wrote:

Sorry for slow replies...

> Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote on Mon [2014-Nov-03 10:59:53 +0100]:
>> On Tue, Oct 21, 2014 at 10:09 PM, Benoit Parrot <bparrot-l0cyMroinI0@public.gmane.org> wrote:
>>
>> >         qe_pio_a: gpio-controller@1400 {
>> > @@ -110,6 +130,19 @@ Example of two SOC GPIO banks defined as gpio-controller nodes:
>> >                 reg = <0x1400 0x18>;
>> >                 gpio-controller;
>> >                 #gpio-cells = <2>;
>> > +               gpio-hogs = <&line_b>;
>> > +
>> > +               /* line_a hog is defined but not enabled in this example*/
>> > +               line_a: line_a {
>> > +                       gpios = <5 0>;
>> > +                       input;
>> > +               };
>> > +
>> > +               line_b: line_b {
>> > +                       gpios = <6 0>;
>> > +                       output-low;
>> > +                       line-name = "foo-bar-gpio";
>> > +               };
>>
>>
>> I don't see the point of having unused hogs and enabling them using
>> phandles.
>>
>> Just let the core walk over all children nodes of a GPIO controller
>> and hog them. Put in a bool property saying it's a hog.
>>
>> +               line_b: line_b {
>> +                       gpio-hog;
>> +                       gpios = <6 0>;
>> +                       output-low;
>> +                       line-name = "foo-bar-gpio";
>> +               };
>>
>> I don't quite see the point with input hogs that noone can use
>> but whatever.
>>
>> I am thinking that maybe the line name should be compulsory
>> so as to improbe readability. I mean there is always a reason
>> why you're hogging a pin and the name should say it.
>
> Ok, so as an alternative I had presented something like this in my reply
> to Alexandre Courbot's review comments:
>
>   I did consider a "pinmux" flavored format (not sure how hard to parse it would be).
>   It would allow grouping if nothing else.
>
>         /* Line syntax: line_name <gpio# flags> direction-value [export] */
>         gpio-hogs = <&group_y>;
>
>         group_y: group_y {
>                 gpio-hogs-group = <
>                         line_x <15 0> output-low
>                         line_y <16 0> output-high export
>                         line_z <17 0> input
>                 >;
>         };
>
> Now based on your comment would something like this work?
>
>      qe_pio_a: gpio-controller@1400 {
>         reg = <0x1400 0x18>;
>         gpio-controller;
>         #gpio-cells = <2>;
>
>         /* Line syntax: line_name <gpio# flags> direction-value [export] */
>         gpio-hogs: {
>                 gpio-hogs-group = <
>                         foo-bar-gpio <15 0> output-low
>                         bar-foo-gpio <16 0> output-high export
>                 >;
>         };
>      };

I *DON'T* want to mix up the exporting interface with the hogging
mechanism. These have to be two different things and different
patches.

But it looks strange and a bit convoluted. I don't see the point
of the grouping concept. There are ages old mails where I suggest
a very flat mechanism like this:

qe_pio_a: gpio-controller@1400 {
   reg = <0x1400 0x18>;
   gpio-controller;
   #gpio-cells = <2>;
   gpio-hogs-output-high = <15 0>, <12 0>;
   gpio-hogs-output-low = <16 0>;
};

I understand that if you want to give names to the pins that
is maybe a bit terse, then I suggest these named nodes:

qe_pio_a: gpio-controller@1400 {
   reg = <0x1400 0x18>;
   gpio-controller;
   #gpio-cells = <2>;
   {
       gpio-hog-output-high = <15 0>;
       line-name = "foo";
   };
   {
       gpio-hog-output-high = <12 0>;
       line-name = "bar";
   };
   {
       gpio-hog-output-low = <16 0>;
       line-name = "baz";
   };
};

This is pretty straight-forward to parse from the device
tree by just walking over the children of a GPIO controller
node and looking for the hog keywords and optional
line names.

This mechanism can later add some per-pin export
keyword too, if that is desired. But that is a separate
discussion.

Still no need for groups or phandles or stuff like that...

It's a terser version of what I suggested in the last
reply from me:

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

Just that I combine gpio-hog, gpios and output-low
into one property.

Any version works, I just don't get this grouping and
phandle business, if such complexity is needed it has
to be motivated.

> This would group all hogs for one controller under a single child node.

Why is that a desireable feature?

I will try to find the other mail thread...

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