[PATCH 0/6] gpiolib: Add GPIO name support

[PATCH 0/6] gpiolib: Add GPIO name support

[Markus Pargmann]

Hi,

Changes in v2:
 - Removed patch 'gpiolib: Fix possible use of wrong name'
 - Removed discussed patch 'gpio: Allow hogged gpios to be requested' from this
   series
 - Fixed show gpio name patch (locks, (null) printing, and getting the descriptor)
 - Merged gpio name patch into gpio_name_to_desc() patch

Description from v1:

This is a proposal to add GPIO names to the kernel based on devicetree
descriptions.

This series adds GPIO name support. Until now it is only possible to use names
for already requested GPIOs (for example what they are used for). It is not
possible to identify GPIOs by a name although most of them have a name for
example in the schematics of the board. This makes it difficult to identify
a specific GPIO from userspace.

As the GPIO name information is a hardware description this series uses the
devicetree bindings introduced by the GPIO hogging mechanism, specifically
'line-name', to identify GPIOs. The sysfs 'export' file is changed to accept
names as fallback. The gpio numbers still have a higher priority to ensure
backwards compatibility.

Exported GPIOs are still using their number as directory name (gpio<ID>). But the
directories now contain a 'name' file which is '' for non-existent names and
the name otherwise.

This series can be used to have an easy name mapping for udev with a quite
simple rule similar to this:
	SUBSYSTEM=="gpio", KERNEL=="gpio*", ATTR{name}!="(null)", ACTION=="add", \
	PROGRAM+="/bin/sh -c 'mkdir -p /dev/gpios; rm -f /dev/gpios/$attr{name}; ln -s /sys%p/ /dev/gpios/$attr{name}"
With this rule udev adds a link for each exported GPIO with a name into
/dev/gpios/. This way it is not necessary to know the number of a GPIO to use
it.

Best Regards,

Markus


Markus Pargmann (6):
  gpiolib-of: Rename gpio_hog functions to be generic
  gpio: Introduce gpio descriptor 'name'
  gpiolib-of: Reuse 'line-name' from DT as gpio descriptor name
  gpiolib-sysfs: Add gpio name parsing for sysfs export
  gpiolib-sysfs: Show gpio-name in /sys/class/gpio/gpio*/name
  gpiolib: Add gpio name information to /sys/kernel/debug/gpio

 drivers/gpio/gpiolib-of.c     | 41 ++++++++++++++++++------------
 drivers/gpio/gpiolib-sysfs.c  | 59 +++++++++++++++++++++++++++++++++----------
 drivers/gpio/gpiolib.c        | 43 ++++++++++++++++++++++++++++---
 drivers/gpio/gpiolib.h        |  3 +++
 include/linux/gpio/consumer.h |  7 +++++
 5 files changed, 121 insertions(+), 32 deletions(-)

-- 
2.1.4

[PATCH 1/6] gpiolib-of: Rename gpio_hog functions to be generic

[Markus Pargmann]

The gpio hogging functions are currently only used for gpio-hogging. But
these functions are widely generic ones which parse gpio device nodes in
the DT.

Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
 drivers/gpio/gpiolib-of.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 9a0ec48a4737..41a4945b346e 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -119,17 +119,17 @@ 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
+ * of_parse_gpio() - Get a GPIO hog descriptor, names and flags for GPIO API
  * @np:		device node to get GPIO from
  * @name:	GPIO line name
  * @lflags:	gpio_lookup_flags - returned from of_find_gpio() or
- *		of_get_gpio_hog()
+ *		of_parse_gpio()
  * @dflags:	gpiod_flags - optional GPIO initialization flags
  *
  * Returns GPIO descriptor to use with Linux GPIO API, or one of the errno
  * value on the error condition.
  */
-static struct gpio_desc *of_get_gpio_hog(struct device_node *np,
+static struct gpio_desc *of_parse_gpio(struct device_node *np,
 				  const char **name,
 				  enum gpio_lookup_flags *lflags,
 				  enum gpiod_flags *dflags)
@@ -199,13 +199,13 @@ static struct gpio_desc *of_get_gpio_hog(struct device_node *np,
 }
 
 /**
- * of_gpiochip_scan_hogs - Scan gpio-controller and apply GPIO hog as requested
+ * of_gpiochip_scan_gpios - Scan gpio-controller for gpio definitions
  * @chip:	gpio chip to act on
  *
  * This is only used by of_gpiochip_add to request/set GPIO initial
  * configuration.
  */
-static void of_gpiochip_scan_hogs(struct gpio_chip *chip)
+static void of_gpiochip_scan_gpios(struct gpio_chip *chip)
 {
 	struct gpio_desc *desc = NULL;
 	struct device_node *np;
@@ -217,7 +217,7 @@ static void of_gpiochip_scan_hogs(struct gpio_chip *chip)
 		if (!of_property_read_bool(np, "gpio-hog"))
 			continue;
 
-		desc = of_get_gpio_hog(np, &name, &lflags, &dflags);
+		desc = of_parse_gpio(np, &name, &lflags, &dflags);
 		if (IS_ERR(desc))
 			continue;
 
@@ -436,7 +436,7 @@ void of_gpiochip_add(struct gpio_chip *chip)
 	of_gpiochip_add_pin_range(chip);
 	of_node_get(chip->of_node);
 
-	of_gpiochip_scan_hogs(chip);
+	of_gpiochip_scan_gpios(chip);
 }
 
 void of_gpiochip_remove(struct gpio_chip *chip)
-- 
2.1.4

Re: [PATCH 1/6] gpiolib-of: Rename gpio_hog functions to be generic

[Linus Walleij]

On Tue, Aug 4, 2015 at 11:23 AM, Markus Pargmann <mpa@pengutronix.de> wrote:

> The gpio hogging functions are currently only used for gpio-hogging. But
> these functions are widely generic ones which parse gpio device nodes in
> the DT.
>
> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
(...)
>  /**
> - * of_get_gpio_hog() - Get a GPIO hog descriptor, names and flags for GPIO API
> + * of_parse_gpio() - Get a GPIO hog descriptor, names and flags for GPIO API

It's not a good name, too generic. It could easily be assued this is
for something
wider.

of_parse_own_gpio() or something, indicating that this pertains to the
chip itself and not to outside users, would be better.

Yours,
Linus Walleij

Re: [PATCH 1/6] gpiolib-of: Rename gpio_hog functions to be generic

[Markus Pargmann]

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

On Mon, Aug 10, 2015 at 11:29:58AM +0200, Linus Walleij wrote:
> On Tue, Aug 4, 2015 at 11:23 AM, Markus Pargmann <mpa@pengutronix.de> wrote:
> 
> > The gpio hogging functions are currently only used for gpio-hogging. But
> > these functions are widely generic ones which parse gpio device nodes in
> > the DT.
> >
> > Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> (...)
> >  /**
> > - * of_get_gpio_hog() - Get a GPIO hog descriptor, names and flags for GPIO API
> > + * of_parse_gpio() - Get a GPIO hog descriptor, names and flags for GPIO API
> 
> It's not a good name, too generic. It could easily be assued this is
> for something
> wider.
> 
> of_parse_own_gpio() or something, indicating that this pertains to the
> chip itself and not to outside users, would be better.

Yes, changed to of_parse_own_gpio() for next version.

Best Regards,

Markus

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

[PATCH 2/6] gpio: Introduce gpio descriptor 'name'

[Markus Pargmann]

The latest gpio hogging mechanism assigns each gpio a 'line-name' in the
devicetree. The 'name' field is different from the 'label' field.
'label' is only used for requested GPIOs to describe its current use by
driver or userspace.

The 'name' field describes the GPIO itself, not the use. This is most
likely identical to the label in the schematic on the GPIO line and
should help to find this particular GPIO.

This patch adds a helper function to find gpio descriptors by name
instead of gpio number.

Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
 drivers/gpio/gpiolib.c        | 32 ++++++++++++++++++++++++++++++++
 drivers/gpio/gpiolib.h        |  3 +++
 include/linux/gpio/consumer.h |  7 +++++++
 3 files changed, 42 insertions(+)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index bf4bd1d120c3..edfeb0485112 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -90,6 +90,38 @@ struct gpio_desc *gpio_to_desc(unsigned gpio)
 EXPORT_SYMBOL_GPL(gpio_to_desc);
 
 /**
+ * Convert a GPIO name to its descriptor
+ */
+struct gpio_desc *gpio_name_to_desc(const char *name)
+{
+	struct gpio_chip *chip;
+	unsigned long flags;
+
+	spin_lock_irqsave(&gpio_lock, flags);
+
+	list_for_each_entry(chip, &gpio_chips, list) {
+		int i;
+
+		for (i = 0; i != chip->ngpio; ++i) {
+			struct gpio_desc *gpio = &chip->desc[i];
+
+			if (!gpio->name)
+				continue;
+
+			if (!strcmp(gpio->name, name)) {
+				spin_unlock_irqrestore(&gpio_lock, flags);
+				return gpio;
+			}
+		}
+	}
+
+	spin_unlock_irqrestore(&gpio_lock, flags);
+
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(gpio_name_to_desc);
+
+/**
  * Get the GPIO descriptor corresponding to the given hw number for this chip.
  */
 struct gpio_desc *gpiochip_get_desc(struct gpio_chip *chip,
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index bf343004b008..78e634d1c719 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -89,7 +89,10 @@ struct gpio_desc {
 #define FLAG_USED_AS_IRQ 9	/* GPIO is connected to an IRQ */
 #define FLAG_IS_HOGGED	11	/* GPIO is hogged */
 
+	/* Connection label */
 	const char		*label;
+	/* Name of the GPIO */
+	const char		*name;
 };
 
 int gpiod_request(struct gpio_desc *desc, const char *label);
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index adac255aee86..a873b8b47ab3 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -130,6 +130,7 @@ int gpiod_to_irq(const struct gpio_desc *desc);
 /* Convert between the old gpio_ and new gpiod_ interfaces */
 struct gpio_desc *gpio_to_desc(unsigned gpio);
 int desc_to_gpio(const struct gpio_desc *desc);
+struct gpio_desc *gpio_name_to_desc(const char *name);
 
 /* Child properties interface */
 struct fwnode_handle;
@@ -400,6 +401,12 @@ static inline struct gpio_desc *gpio_to_desc(unsigned gpio)
 {
 	return ERR_PTR(-EINVAL);
 }
+
+static inline struct gpio_desc *gpio_name_to_desc(const char *name)
+{
+	return ERR_PTR(-EINVAL);
+}
+
 static inline int desc_to_gpio(const struct gpio_desc *desc)
 {
 	/* GPIO can never have been requested */
-- 
2.1.4

Re: [PATCH 2/6] gpio: Introduce gpio descriptor 'name'

[Linus Walleij]

On Tue, Aug 4, 2015 at 11:23 AM, Markus Pargmann <mpa@pengutronix.de> wrote:

> The latest gpio hogging mechanism assigns each gpio a 'line-name' in the
> devicetree. The 'name' field is different from the 'label' field.
> 'label' is only used for requested GPIOs to describe its current use by
> driver or userspace.
>
> The 'name' field describes the GPIO itself, not the use. This is most
> likely identical to the label in the schematic on the GPIO line and
> should help to find this particular GPIO.
>
> This patch adds a helper function to find gpio descriptors by name
> instead of gpio number.
>
> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>

Oh I realized there is a big problem with this.

struct gpio_chip already contains this:

        const char              *const *names;

Now it seems like names can be stored in two places:
in an array in the gpio_chip and in a name tag in struct
gpio_desc.

So how do we handle this?

I guess we need to keep just *one* of them, so maybe remove
the names array from the struct gpio_chip and add a helper function
that sets the names on the descs for the lines like:

int gpiochip_set_names(struct gpio_chip *gc, const char * const names);

And then refactor all code and chips that use the old names
array to use this instead. I don't think they are too many, really.

(Maybe I got it all wrong, hammer me down, it is monday.)

Yours,
Linus Walleij

Re: [PATCH 2/6] gpio: Introduce gpio descriptor 'name'

[Markus Pargmann]

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

On Mon, Aug 10, 2015 at 11:37:06AM +0200, Linus Walleij wrote:
> On Tue, Aug 4, 2015 at 11:23 AM, Markus Pargmann <mpa@pengutronix.de> wrote:
> 
> > The latest gpio hogging mechanism assigns each gpio a 'line-name' in the
> > devicetree. The 'name' field is different from the 'label' field.
> > 'label' is only used for requested GPIOs to describe its current use by
> > driver or userspace.
> >
> > The 'name' field describes the GPIO itself, not the use. This is most
> > likely identical to the label in the schematic on the GPIO line and
> > should help to find this particular GPIO.
> >
> > This patch adds a helper function to find gpio descriptors by name
> > instead of gpio number.
> >
> > Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> 
> Oh I realized there is a big problem with this.
> 
> struct gpio_chip already contains this:
> 
>         const char              *const *names;
> 
> Now it seems like names can be stored in two places:
> in an array in the gpio_chip and in a name tag in struct
> gpio_desc.
> 
> So how do we handle this?
> 
> I guess we need to keep just *one* of them, so maybe remove
> the names array from the struct gpio_chip and add a helper function
> that sets the names on the descs for the lines like:
> 
> int gpiochip_set_names(struct gpio_chip *gc, const char * const names);
> 
> And then refactor all code and chips that use the old names
> array to use this instead. I don't think they are too many, really.

That sounds good. There are just a few users of this names array, so it
should all be fine.

Just to clarify: The line names will then be the same as this name
array? So all GPIOs with name/line-names are then exported by their name
and not the number?

Best Regards,

Markus

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

Re: [PATCH 2/6] gpio: Introduce gpio descriptor 'name'

[Linus Walleij]

On Thu, Aug 13, 2015 at 3:28 PM, Markus Pargmann <mpa@pengutronix.de> wrote:
> On Mon, Aug 10, 2015 at 11:37:06AM +0200, Linus Walleij wrote:
>> On Tue, Aug 4, 2015 at 11:23 AM, Markus Pargmann <mpa@pengutronix.de> wrote:
>>
>> > The latest gpio hogging mechanism assigns each gpio a 'line-name' in the
>> > devicetree. The 'name' field is different from the 'label' field.
>> > 'label' is only used for requested GPIOs to describe its current use by
>> > driver or userspace.
>> >
>> > The 'name' field describes the GPIO itself, not the use. This is most
>> > likely identical to the label in the schematic on the GPIO line and
>> > should help to find this particular GPIO.
>> >
>> > This patch adds a helper function to find gpio descriptors by name
>> > instead of gpio number.
>> >
>> > Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
>>
>> Oh I realized there is a big problem with this.
>>
>> struct gpio_chip already contains this:
>>
>>         const char              *const *names;
>>
>> Now it seems like names can be stored in two places:
>> in an array in the gpio_chip and in a name tag in struct
>> gpio_desc.
>>
>> So how do we handle this?
>>
>> I guess we need to keep just *one* of them, so maybe remove
>> the names array from the struct gpio_chip and add a helper function
>> that sets the names on the descs for the lines like:
>>
>> int gpiochip_set_names(struct gpio_chip *gc, const char * const names);
>>
>> And then refactor all code and chips that use the old names
>> array to use this instead. I don't think they are too many, really.
>
> That sounds good. There are just a few users of this names array, so it
> should all be fine.

OK thanks.

> Just to clarify: The line names will then be the same as this name
> array? So all GPIOs with name/line-names are then exported by their name
> and not the number?

Yeah I consider it a legacy mechanism to achieve the same thing.

The problem with it is that it has a flat namespace and we
need to conform to that.

Yours,
Linus Walleij

[PATCH 3/6] gpiolib-of: Reuse 'line-name' from DT as gpio descriptor name

[Markus Pargmann]

The DT describes gpios with subnodes. These subnodes can contain a
property 'line-name'. This information can be useful in other areas
where we want to identify a GPIO by its name.

This patch stores the line-name in the gpio descriptor.

Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
 drivers/gpio/gpiolib-of.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 41a4945b346e..fbf449a4120b 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -184,11 +184,6 @@ static struct gpio_desc *of_parse_gpio(struct device_node *np,
 		*dflags |= GPIOD_OUT_LOW;
 	else if (of_property_read_bool(np, "output-high"))
 		*dflags |= GPIOD_OUT_HIGH;
-	else {
-		pr_warn("GPIO line %d (%s): no hogging state specified, bailing out\n",
-			desc_to_gpio(gg_data.out_gpio), np->name);
-		return ERR_PTR(-EINVAL);
-	}
 
 	if (name && of_property_read_string(np, "line-name", name))
 		*name = np->name;
@@ -214,15 +209,29 @@ static void of_gpiochip_scan_gpios(struct gpio_chip *chip)
 	enum gpiod_flags dflags;
 
 	for_each_child_of_node(chip->of_node, np) {
-		if (!of_property_read_bool(np, "gpio-hog"))
-			continue;
+		struct gpio_desc *name_desc;
 
 		desc = of_parse_gpio(np, &name, &lflags, &dflags);
 		if (IS_ERR(desc))
 			continue;
 
-		if (gpiod_hog(desc, name, lflags, dflags))
-			continue;
+		name_desc = gpio_name_to_desc(name);
+		if (name_desc)
+			pr_warn("GPIO name collision for '%s' detected at GPIO line %d (%s)\n",
+				name, desc_to_gpio(desc), np->name);
+		else
+			desc->name = name;
+
+		if (of_property_read_bool(np, "gpio-hog")) {
+			if (!dflags) {
+				pr_warn("GPIO line %d (%s): no hogging state specified, bailing out\n",
+					desc_to_gpio(desc), np->name);
+				continue;
+			}
+
+			if (gpiod_hog(desc, name, lflags, dflags))
+				continue;
+		}
 	}
 }
 
-- 
2.1.4

Re: [PATCH 3/6] gpiolib-of: Reuse 'line-name' from DT as gpio descriptor name

[Linus Walleij]

On Tue, Aug 4, 2015 at 11:23 AM, Markus Pargmann <mpa@pengutronix.de> wrote:

> The DT describes gpios with subnodes. These subnodes can contain a
> property 'line-name'. This information can be useful in other areas
> where we want to identify a GPIO by its name.
>
> This patch stores the line-name in the gpio descriptor.
>
> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>

This patch looks correct given the context works as intended.

Yours,
Linus Walleij

[PATCH 4/6] gpiolib-sysfs: Add gpio name parsing for sysfs export

[Markus Pargmann]

Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
 drivers/gpio/gpiolib-sysfs.c | 47 ++++++++++++++++++++++++++++++++------------
 1 file changed, 34 insertions(+), 13 deletions(-)

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index b57ed8e55ab5..e22a86e31c90 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -443,18 +443,28 @@ static ssize_t export_store(struct class *class,
 				const char *buf, size_t len)
 {
 	long			gpio;
-	struct gpio_desc	*desc;
+	struct gpio_desc	*desc = NULL;
 	int			status;
 
 	status = kstrtol(buf, 0, &gpio);
-	if (status < 0)
-		goto done;
+	if (!status)
+		desc = gpio_to_desc(gpio);
 
-	desc = gpio_to_desc(gpio);
-	/* reject invalid GPIOs */
+	/* Fall back on detection by name */
 	if (!desc) {
-		pr_warn("%s: invalid GPIO %ld\n", __func__, gpio);
-		return -EINVAL;
+		char *gpio_name = kstrdup(buf, GFP_KERNEL);
+
+		if (!gpio_name)
+			return -ENOMEM;
+
+		desc = gpio_name_to_desc(strim(gpio_name));
+		kfree(gpio_name);
+
+		/* reject invalid GPIOs */
+		if (!desc) {
+			pr_warn("%s: invalid GPIO %s\n", __func__, buf);
+			return -EINVAL;
+		}
 	}
 
 	/* No extra locking here; FLAG_SYSFS just signifies that the
@@ -485,17 +495,28 @@ static ssize_t unexport_store(struct class *class,
 				const char *buf, size_t len)
 {
 	long			gpio;
-	struct gpio_desc	*desc;
+	struct gpio_desc	*desc = NULL;
 	int			status;
 
 	status = kstrtol(buf, 0, &gpio);
-	if (status < 0)
-		goto done;
+	if (!status)
+		desc = gpio_to_desc(gpio);
+
+	/* Fall back on detection by name */
+	if (!desc) {
+		char *gpio_name = kstrdup(buf, GFP_KERNEL);
+
+		if (!gpio_name)
+			return -ENOMEM;
+
+		desc = gpio_name_to_desc(strim(gpio_name));
+		kfree(gpio_name);
+	}
+
 
-	desc = gpio_to_desc(gpio);
 	/* reject bogus commands (gpio_unexport ignores them) */
 	if (!desc) {
-		pr_warn("%s: invalid GPIO %ld\n", __func__, gpio);
+		pr_warn("%s: invalid GPIO %s\n", __func__, buf);
 		return -EINVAL;
 	}
 
@@ -509,7 +530,7 @@ static ssize_t unexport_store(struct class *class,
 		status = 0;
 		gpiod_free(desc);
 	}
-done:
+
 	if (status)
 		pr_debug("%s: status %d\n", __func__, status);
 	return status ? : len;
-- 
2.1.4

Re: [PATCH 4/6] gpiolib-sysfs: Add gpio name parsing for sysfs export

[Linus Walleij]

On Tue, Aug 4, 2015 at 11:23 AM, Markus Pargmann <mpa@pengutronix.de> wrote:

> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>

Needs a bigger commit message, but looks correct in this context.

Yours,
Linus Walleij

[PATCH 5/6] gpiolib-sysfs: Show gpio-name in /sys/class/gpio/gpio*/name

[Markus Pargmann]

This patch adds a sysfs attribute 'name' to gpios that were exported. It
exposes the newly added name property of gpio descriptors.

Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
 drivers/gpio/gpiolib-sysfs.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index e22a86e31c90..c5e8224428ac 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -139,6 +139,17 @@ static ssize_t value_store(struct device *dev,
 }
 static DEVICE_ATTR_RW(value);
 
+static ssize_t name_show(struct device *dev, struct device_attribute *attr,
+			 char *buf)
+{
+	struct gpiod_data *data = dev_get_drvdata(dev);
+	struct gpio_desc *desc = data->desc;
+
+	return sprintf(buf, "%s\n", desc->name ? : "");
+}
+
+static DEVICE_ATTR_RO(name);
+
 static irqreturn_t gpio_sysfs_irq(int irq, void *priv)
 {
 	struct gpiod_data *data = priv;
@@ -377,6 +388,7 @@ static struct attribute *gpio_attrs[] = {
 	&dev_attr_edge.attr,
 	&dev_attr_value.attr,
 	&dev_attr_active_low.attr,
+	&dev_attr_name.attr,
 	NULL,
 };
 
-- 
2.1.4

Re: [PATCH 5/6] gpiolib-sysfs: Show gpio-name in /sys/class/gpio/gpio*/name

[Linus Walleij]

On Tue, Aug 4, 2015 at 11:23 AM, Markus Pargmann <mpa@pengutronix.de> wrote:

> This patch adds a sysfs attribute 'name' to gpios that were exported. It
> exposes the newly added name property of gpio descriptors.
>
> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>

This needs to also patch Documentation/ABI/testing/sysfs-gpio
if we should go with it. It says this:

    /sys/class/gpio
        /export ... asks the kernel to export a GPIO to userspace
        /unexport ... to return a GPIO to the kernel
        /gpioN ... for each exported GPIO #N
            /value ... always readable, writes fail for input GPIOs
            /direction ... r/w as: in, out (default low); write: high, low
            /edge ... r/w as: none, falling, rising, both

Anyways I don't know if this is right, and that ABI doc is also lying.

Look at this in gpiolib-sysfs.c:

        if (chip->names && chip->names[offset])
                ioname = chip->names[offset];

        dev = device_create_with_groups(&gpio_class, chip->dev,
                                        MKDEV(0, 0), data, gpio_groups,
                                        ioname ? ioname : "gpio%u",
                                        desc_to_gpio(desc));

I.e. what the ABI doc say about the dirs being named "gpioN" is
a plain lie, it can have a descriptive name as its directory  name
under /sys/class/gpio/foo-line or so.

Since this already exist and is a flat namespace ... we should
use that.

However it has the implication like I said before that two names
cannot be the same. I think Johan's comment that they could
be non-unique did not take into account the fact that two chips
could use the same .names array (and that would already fail,
by the way) so the .names in the struct gpio_chip *MUST* be
unique as compared to all other names.

We *could* deprecate the old line naming mechanism (that create
dirs named after the pin), and from here on only use gpioN and
"name" in a separate file like this patch does. However that is
not really OK either: we want to move away from the GPIO numbers
and to descriptors and descriptive names, so I currently feel
we should name the directories after the line instead, and
require them to be unique.

I'll have to patch this document now anyways because it is
lying about the ABI :(

Yours,
Linus Walleij

Re: [PATCH 5/6] gpiolib-sysfs: Show gpio-name in /sys/class/gpio/gpio*/name

[Markus Pargmann]

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

On Mon, Aug 10, 2015 at 11:50:16AM +0200, Linus Walleij wrote:
> On Tue, Aug 4, 2015 at 11:23 AM, Markus Pargmann <mpa@pengutronix.de> wrote:
> 
> > This patch adds a sysfs attribute 'name' to gpios that were exported. It
> > exposes the newly added name property of gpio descriptors.
> >
> > Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> 
> This needs to also patch Documentation/ABI/testing/sysfs-gpio
> if we should go with it. It says this:

Right, forgot about that documentation.

> 
>     /sys/class/gpio
>         /export ... asks the kernel to export a GPIO to userspace
>         /unexport ... to return a GPIO to the kernel
>         /gpioN ... for each exported GPIO #N
>             /value ... always readable, writes fail for input GPIOs
>             /direction ... r/w as: in, out (default low); write: high, low
>             /edge ... r/w as: none, falling, rising, both
> 
> Anyways I don't know if this is right, and that ABI doc is also lying.
> 
> Look at this in gpiolib-sysfs.c:
> 
>         if (chip->names && chip->names[offset])
>                 ioname = chip->names[offset];
> 
>         dev = device_create_with_groups(&gpio_class, chip->dev,
>                                         MKDEV(0, 0), data, gpio_groups,
>                                         ioname ? ioname : "gpio%u",
>                                         desc_to_gpio(desc));
> 
> I.e. what the ABI doc say about the dirs being named "gpioN" is
> a plain lie, it can have a descriptive name as its directory  name
> under /sys/class/gpio/foo-line or so.
> 
> Since this already exist and is a flat namespace ... we should
> use that.
> 
> However it has the implication like I said before that two names
> cannot be the same. I think Johan's comment that they could
> be non-unique did not take into account the fact that two chips
> could use the same .names array (and that would already fail,
> by the way) so the .names in the struct gpio_chip *MUST* be
> unique as compared to all other names.
> 
> We *could* deprecate the old line naming mechanism (that create
> dirs named after the pin), and from here on only use gpioN and
> "name" in a separate file like this patch does. However that is
> not really OK either: we want to move away from the GPIO numbers
> and to descriptors and descriptive names, so I currently feel
> we should name the directories after the line instead, and
> require them to be unique.

Ok, this also answers the question I just had in the other mail.

Best Regards,

Markus

> 
> I'll have to patch this document now anyways because it is
> lying about the ABI :(
> 
> Yours,
> Linus Walleij
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

Re: [PATCH 5/6] gpiolib-sysfs: Show gpio-name in /sys/class/gpio/gpio*/name

[Johan Hovold]

On Mon, Aug 10, 2015 at 11:50:16AM +0200, Linus Walleij wrote:
> On Tue, Aug 4, 2015 at 11:23 AM, Markus Pargmann <mpa@pengutronix.de> wrote:
> 
> > This patch adds a sysfs attribute 'name' to gpios that were exported. It
> > exposes the newly added name property of gpio descriptors.
> >
> > Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> 
> This needs to also patch Documentation/ABI/testing/sysfs-gpio
> if we should go with it. It says this:
> 
>     /sys/class/gpio
>         /export ... asks the kernel to export a GPIO to userspace
>         /unexport ... to return a GPIO to the kernel
>         /gpioN ... for each exported GPIO #N
>             /value ... always readable, writes fail for input GPIOs
>             /direction ... r/w as: in, out (default low); write: high, low
>             /edge ... r/w as: none, falling, rising, both
> 
> Anyways I don't know if this is right, and that ABI doc is also lying.
> 
> Look at this in gpiolib-sysfs.c:
>
>         if (chip->names && chip->names[offset])
>                 ioname = chip->names[offset];
> 
>         dev = device_create_with_groups(&gpio_class, chip->dev,
>                                         MKDEV(0, 0), data, gpio_groups,
>                                         ioname ? ioname : "gpio%u",
>                                         desc_to_gpio(desc));
> 
> I.e. what the ABI doc say about the dirs being named "gpioN" is
> a plain lie, it can have a descriptive name as its directory  name
> under /sys/class/gpio/foo-line or so.
> 
> Since this already exist and is a flat namespace ... we should
> use that.
> 
> However it has the implication like I said before that two names
> cannot be the same. I think Johan's comment that they could
> be non-unique did not take into account the fact that two chips
> could use the same .names array (and that would already fail,
> by the way) so the .names in the struct gpio_chip *MUST* be
> unique as compared to all other names.

It's is unfortunate that we need to potentially maintain this forever,
but it does not mean that we should not allow non-unique names in DT
going forward.

> We *could* deprecate the old line naming mechanism (that create
> dirs named after the pin), and from here on only use gpioN and
> "name" in a separate file like this patch does. However that is
> not really OK either: we want to move away from the GPIO numbers
> and to descriptors and descriptive names, so I currently feel
> we should name the directories after the line instead, and
> require them to be unique.

We have already moved away from the numbers through the introduction of
descriptors. What remains to define is a sane userspace interface. Until
then, let the sysfs-interface be as cumbersome to use as it always has
rather than introducing more (intermediate) ABI to maintain.

Johan

[PATCH 6/6] gpiolib: Add gpio name information to /sys/kernel/debug/gpio

[Markus Pargmann]

Add some information about gpio names to the debugfs gpio file. name and
label of a GPIO are then displayed next to each other. This way it is
easy to see what the real name of GPIO is and what the driver requested
it for.

Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
 drivers/gpio/gpiolib.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index edfeb0485112..926a1507431f 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2307,14 +2307,19 @@ static void gpiolib_dbg_show(struct seq_file *s, struct gpio_chip *chip)
 	int			is_irq;
 
 	for (i = 0; i < chip->ngpio; i++, gpio++, gdesc++) {
-		if (!test_bit(FLAG_REQUESTED, &gdesc->flags))
+		if (!test_bit(FLAG_REQUESTED, &gdesc->flags)) {
+			if (gdesc->name) {
+				seq_printf(s, " gpio-%-3d (%-20.20s)\n",
+					   gpio, gdesc->name);
+			}
 			continue;
+		}
 
 		gpiod_get_direction(gdesc);
 		is_out = test_bit(FLAG_IS_OUT, &gdesc->flags);
 		is_irq = test_bit(FLAG_USED_AS_IRQ, &gdesc->flags);
-		seq_printf(s, " gpio-%-3d (%-20.20s) %s %s %s",
-			gpio, gdesc->label,
+		seq_printf(s, " gpio-%-3d (%-20.20s|%-20.20s) %s %s %s",
+			gpio, gdesc->name ? gdesc->name : "", gdesc->label,
 			is_out ? "out" : "in ",
 			chip->get
 				? (chip->get(chip, i) ? "hi" : "lo")
-- 
2.1.4

Re: [PATCH 6/6] gpiolib: Add gpio name information to /sys/kernel/debug/gpio

[Linus Walleij]

On Tue, Aug 4, 2015 at 11:23 AM, Markus Pargmann <mpa@pengutronix.de> wrote:

> Add some information about gpio names to the debugfs gpio file. name and
> label of a GPIO are then displayed next to each other. This way it is
> easy to see what the real name of GPIO is and what the driver requested
> it for.
>
> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>

This patch looks correct in the context.

Yours,
Linus Walleij

Re: [PATCH 6/6] gpiolib: Add gpio name information to /sys/kernel/debug/gpio

[Alexandre Courbot]

On Mon, Aug 10, 2015 at 7:02 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Tue, Aug 4, 2015 at 11:23 AM, Markus Pargmann <mpa@pengutronix.de> wrote:
>
>> Add some information about gpio names to the debugfs gpio file. name and
>> label of a GPIO are then displayed next to each other. This way it is
>> easy to see what the real name of GPIO is and what the driver requested
>> it for.
>>
>> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
>
> This patch looks correct in the context.

Looks correct indeed, but on a related note one might question the
need to have both a "name" and a "label" for a GPIO?

Re: [PATCH 6/6] gpiolib: Add gpio name information to /sys/kernel/debug/gpio

[Linus Walleij]

On Mon, Aug 17, 2015 at 9:29 AM, Alexandre Courbot <gnurou@gmail.com> wrote:
> On Mon, Aug 10, 2015 at 7:02 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Tue, Aug 4, 2015 at 11:23 AM, Markus Pargmann <mpa@pengutronix.de> wrote:
>>
>>> Add some information about gpio names to the debugfs gpio file. name and
>>> label of a GPIO are then displayed next to each other. This way it is
>>> easy to see what the real name of GPIO is and what the driver requested
>>> it for.
>>>
>>> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
>>
>> This patch looks correct in the context.
>
> Looks correct indeed, but on a related note one might question the
> need to have both a "name" and a "label" for a GPIO?

The way I see it a certain GPIO controller has names for the
GPIO lines, like gpio0,1,2...n, while the label is for the actual
use of the GPIO line. It's a bit like the regulator name, rail name
and consumer endpoints guess. Just we only have two names,
not three :P

Yours,
Linus Walleij

Re: [PATCH 0/6 v2] gpiolib: Add GPIO name support

[Markus Pargmann]

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

On Tue, Aug 04, 2015 at 11:23:17AM +0200, Markus Pargmann wrote:
> Hi,
> 
> Changes in v2:

Sorry, forgot to add v2 into the subject.

Best regards,

Markus

>  - Removed patch 'gpiolib: Fix possible use of wrong name'
>  - Removed discussed patch 'gpio: Allow hogged gpios to be requested' from this
>    series
>  - Fixed show gpio name patch (locks, (null) printing, and getting the descriptor)
>  - Merged gpio name patch into gpio_name_to_desc() patch
> 
> Description from v1:
> 
> This is a proposal to add GPIO names to the kernel based on devicetree
> descriptions.
> 
> This series adds GPIO name support. Until now it is only possible to use names
> for already requested GPIOs (for example what they are used for). It is not
> possible to identify GPIOs by a name although most of them have a name for
> example in the schematics of the board. This makes it difficult to identify
> a specific GPIO from userspace.
> 
> As the GPIO name information is a hardware description this series uses the
> devicetree bindings introduced by the GPIO hogging mechanism, specifically
> 'line-name', to identify GPIOs. The sysfs 'export' file is changed to accept
> names as fallback. The gpio numbers still have a higher priority to ensure
> backwards compatibility.
> 
> Exported GPIOs are still using their number as directory name (gpio<ID>). But the
> directories now contain a 'name' file which is '' for non-existent names and
> the name otherwise.
> 
> This series can be used to have an easy name mapping for udev with a quite
> simple rule similar to this:
> 	SUBSYSTEM=="gpio", KERNEL=="gpio*", ATTR{name}!="(null)", ACTION=="add", \
> 	PROGRAM+="/bin/sh -c 'mkdir -p /dev/gpios; rm -f /dev/gpios/$attr{name}; ln -s /sys%p/ /dev/gpios/$attr{name}"
> With this rule udev adds a link for each exported GPIO with a name into
> /dev/gpios/. This way it is not necessary to know the number of a GPIO to use
> it.
> 
> Best Regards,
> 
> Markus
> 
> 
> Markus Pargmann (6):
>   gpiolib-of: Rename gpio_hog functions to be generic
>   gpio: Introduce gpio descriptor 'name'
>   gpiolib-of: Reuse 'line-name' from DT as gpio descriptor name
>   gpiolib-sysfs: Add gpio name parsing for sysfs export
>   gpiolib-sysfs: Show gpio-name in /sys/class/gpio/gpio*/name
>   gpiolib: Add gpio name information to /sys/kernel/debug/gpio
> 
>  drivers/gpio/gpiolib-of.c     | 41 ++++++++++++++++++------------
>  drivers/gpio/gpiolib-sysfs.c  | 59 +++++++++++++++++++++++++++++++++----------
>  drivers/gpio/gpiolib.c        | 43 ++++++++++++++++++++++++++++---
>  drivers/gpio/gpiolib.h        |  3 +++
>  include/linux/gpio/consumer.h |  7 +++++
>  5 files changed, 121 insertions(+), 32 deletions(-)
> 
> -- 
> 2.1.4
> 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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