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

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

[Markus Pargmann]

Hi,

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 '(null)' 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 (9):
  gpiolib: Fix possible use of wrong name
  gpiolib-of: Rename gpio_hog functions to be generic
  gpio: Allow hogged gpios to be requested
  gpio: Add 'name' to the gpio descriptor struct
  gpiolib: Implement gpio_name_to_desc()
  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     | 45 +++++++++++++++++++------------
 drivers/gpio/gpiolib-sysfs.c  | 61 ++++++++++++++++++++++++++++++++++---------
 drivers/gpio/gpiolib.c        | 46 +++++++++++++++++++++++++++++---
 drivers/gpio/gpiolib.h        |  3 +++
 include/linux/gpio/consumer.h |  7 +++++
 5 files changed, 128 insertions(+), 34 deletions(-)

-- 
2.1.4

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

[Markus Pargmann]

Hi,

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 '(null)' 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 (9):
  gpiolib: Fix possible use of wrong name
  gpiolib-of: Rename gpio_hog functions to be generic
  gpio: Allow hogged gpios to be requested
  gpio: Add 'name' to the gpio descriptor struct
  gpiolib: Implement gpio_name_to_desc()
  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     | 45 +++++++++++++++++++------------
 drivers/gpio/gpiolib-sysfs.c  | 61 ++++++++++++++++++++++++++++++++++---------
 drivers/gpio/gpiolib.c        | 46 +++++++++++++++++++++++++++++---
 drivers/gpio/gpiolib.h        |  3 +++
 include/linux/gpio/consumer.h |  7 +++++
 5 files changed, 128 insertions(+), 34 deletions(-)

-- 
2.1.4

[PATCH 1/9] gpiolib: Fix possible use of wrong name

[Markus Pargmann]

The name is set optionally from DT. Therefore we need to reset the name
variable for every loop iteration. Otherwise we could use a wrong or
uninitialized name.

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

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 9a0ec48a4737..60aebe4d7c26 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -209,11 +209,11 @@ static void of_gpiochip_scan_hogs(struct gpio_chip *chip)
 {
 	struct gpio_desc *desc = NULL;
 	struct device_node *np;
-	const char *name;
 	enum gpio_lookup_flags lflags;
 	enum gpiod_flags dflags;
 
 	for_each_child_of_node(chip->of_node, np) {
+		const char *name = NULL;
 		if (!of_property_read_bool(np, "gpio-hog"))
 			continue;
 
-- 
2.1.4

[PATCH 1/9] gpiolib: Fix possible use of wrong name

[Markus Pargmann]

The name is set optionally from DT. Therefore we need to reset the name
variable for every loop iteration. Otherwise we could use a wrong or
uninitialized name.

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

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 9a0ec48a4737..60aebe4d7c26 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -209,11 +209,11 @@ static void of_gpiochip_scan_hogs(struct gpio_chip *chip)
 {
 	struct gpio_desc *desc = NULL;
 	struct device_node *np;
-	const char *name;
 	enum gpio_lookup_flags lflags;
 	enum gpiod_flags dflags;
 
 	for_each_child_of_node(chip->of_node, np) {
+		const char *name = NULL;
 		if (!of_property_read_bool(np, "gpio-hog"))
 			continue;
 
-- 
2.1.4

Re: [PATCH 1/9] gpiolib: Fix possible use of wrong name

[Johan Hovold]

On Fri, Jul 17, 2015 at 11:32:42AM +0200, Markus Pargmann wrote:
> The name is set optionally from DT. Therefore we need to reset the name
> variable for every loop iteration. Otherwise we could use a wrong or
> uninitialized name.

This doesn't make sense. of_get_gpio_hog falls back to using the node
name so name will always be updated (unless there's an error).

Could be better documented, though.

> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> ---
>  drivers/gpio/gpiolib-of.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> index 9a0ec48a4737..60aebe4d7c26 100644
> --- a/drivers/gpio/gpiolib-of.c
> +++ b/drivers/gpio/gpiolib-of.c
> @@ -209,11 +209,11 @@ static void of_gpiochip_scan_hogs(struct gpio_chip *chip)
>  {
>  	struct gpio_desc *desc = NULL;
>  	struct device_node *np;
> -	const char *name;
>  	enum gpio_lookup_flags lflags;
>  	enum gpiod_flags dflags;
>  
>  	for_each_child_of_node(chip->of_node, np) {
> +		const char *name = NULL;
>  		if (!of_property_read_bool(np, "gpio-hog"))
>  			continue;

Johan

[PATCH 1/9] gpiolib: Fix possible use of wrong name

[Johan Hovold]

On Fri, Jul 17, 2015 at 11:32:42AM +0200, Markus Pargmann wrote:
> The name is set optionally from DT. Therefore we need to reset the name
> variable for every loop iteration. Otherwise we could use a wrong or
> uninitialized name.

This doesn't make sense. of_get_gpio_hog falls back to using the node
name so name will always be updated (unless there's an error).

Could be better documented, though.

> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> ---
>  drivers/gpio/gpiolib-of.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> index 9a0ec48a4737..60aebe4d7c26 100644
> --- a/drivers/gpio/gpiolib-of.c
> +++ b/drivers/gpio/gpiolib-of.c
> @@ -209,11 +209,11 @@ static void of_gpiochip_scan_hogs(struct gpio_chip *chip)
>  {
>  	struct gpio_desc *desc = NULL;
>  	struct device_node *np;
> -	const char *name;
>  	enum gpio_lookup_flags lflags;
>  	enum gpiod_flags dflags;
>  
>  	for_each_child_of_node(chip->of_node, np) {
> +		const char *name = NULL;
>  		if (!of_property_read_bool(np, "gpio-hog"))
>  			continue;

Johan

Re: [PATCH 1/9] gpiolib: Fix possible use of wrong name

[Markus Pargmann]

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

On Tue, Jul 28, 2015 at 11:03:31AM +0200, Johan Hovold wrote:
> On Fri, Jul 17, 2015 at 11:32:42AM +0200, Markus Pargmann wrote:
> > The name is set optionally from DT. Therefore we need to reset the name
> > variable for every loop iteration. Otherwise we could use a wrong or
> > uninitialized name.
> 
> This doesn't make sense. of_get_gpio_hog falls back to using the node
> name so name will always be updated (unless there's an error).
> 
> Could be better documented, though.

Thanks, just got the two lines wrong. Yes the name is always set
correctly.
Will fix it.

Best regards,

Markus

> 
> > Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> > ---
> >  drivers/gpio/gpiolib-of.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> > index 9a0ec48a4737..60aebe4d7c26 100644
> > --- a/drivers/gpio/gpiolib-of.c
> > +++ b/drivers/gpio/gpiolib-of.c
> > @@ -209,11 +209,11 @@ static void of_gpiochip_scan_hogs(struct gpio_chip *chip)
> >  {
> >  	struct gpio_desc *desc = NULL;
> >  	struct device_node *np;
> > -	const char *name;
> >  	enum gpio_lookup_flags lflags;
> >  	enum gpiod_flags dflags;
> >  
> >  	for_each_child_of_node(chip->of_node, np) {
> > +		const char *name = NULL;
> >  		if (!of_property_read_bool(np, "gpio-hog"))
> >  			continue;
> 
> Johan
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

-- 
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 1/9] gpiolib: Fix possible use of wrong name

[Markus Pargmann]

On Tue, Jul 28, 2015 at 11:03:31AM +0200, Johan Hovold wrote:
> On Fri, Jul 17, 2015 at 11:32:42AM +0200, Markus Pargmann wrote:
> > The name is set optionally from DT. Therefore we need to reset the name
> > variable for every loop iteration. Otherwise we could use a wrong or
> > uninitialized name.
> 
> This doesn't make sense. of_get_gpio_hog falls back to using the node
> name so name will always be updated (unless there's an error).
> 
> Could be better documented, though.

Thanks, just got the two lines wrong. Yes the name is always set
correctly.
Will fix it.

Best regards,

Markus

> 
> > Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> > ---
> >  drivers/gpio/gpiolib-of.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> > index 9a0ec48a4737..60aebe4d7c26 100644
> > --- a/drivers/gpio/gpiolib-of.c
> > +++ b/drivers/gpio/gpiolib-of.c
> > @@ -209,11 +209,11 @@ static void of_gpiochip_scan_hogs(struct gpio_chip *chip)
> >  {
> >  	struct gpio_desc *desc = NULL;
> >  	struct device_node *np;
> > -	const char *name;
> >  	enum gpio_lookup_flags lflags;
> >  	enum gpiod_flags dflags;
> >  
> >  	for_each_child_of_node(chip->of_node, np) {
> > +		const char *name = NULL;
> >  		if (!of_property_read_bool(np, "gpio-hog"))
> >  			continue;
> 
> Johan
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

-- 
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 |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150729/d49f77bb/attachment.sig>

[PATCH 2/9] 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 60aebe4d7c26..64950591a764 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

[PATCH 2/9] 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 60aebe4d7c26..64950591a764 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

[PATCH 3/9] gpio: Allow hogged gpios to be requested

[Markus Pargmann]

It can be useful to claim hogged gpios later, for example from
userspace. This allows to set defaults for GPIOs using the hogging
mechanism and override the setup later from userspace or a kernel driver.

This patch adds a check for hogged gpios to allow requesting them. If
the gpio is not hogged but marked as requested, it still fails with
-EBUSY.

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

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index bf4bd1d120c3..9f402b159cbe 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -798,7 +798,8 @@ static int __gpiod_request(struct gpio_desc *desc, const char *label)
 	 * before IRQs are enabled, for non-sleeping (SOC) GPIOs.
 	 */
 
-	if (test_and_set_bit(FLAG_REQUESTED, &desc->flags) == 0) {
+	if (test_and_set_bit(FLAG_REQUESTED, &desc->flags) == 0 ||
+	    test_and_clear_bit(FLAG_IS_HOGGED, &desc->flags) == 1) {
 		desc_set_label(desc, label ? : "?");
 		status = 0;
 	} else {
-- 
2.1.4

[PATCH 3/9] gpio: Allow hogged gpios to be requested

[Markus Pargmann]

It can be useful to claim hogged gpios later, for example from
userspace. This allows to set defaults for GPIOs using the hogging
mechanism and override the setup later from userspace or a kernel driver.

This patch adds a check for hogged gpios to allow requesting them. If
the gpio is not hogged but marked as requested, it still fails with
-EBUSY.

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

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index bf4bd1d120c3..9f402b159cbe 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -798,7 +798,8 @@ static int __gpiod_request(struct gpio_desc *desc, const char *label)
 	 * before IRQs are enabled, for non-sleeping (SOC) GPIOs.
 	 */
 
-	if (test_and_set_bit(FLAG_REQUESTED, &desc->flags) == 0) {
+	if (test_and_set_bit(FLAG_REQUESTED, &desc->flags) == 0 ||
+	    test_and_clear_bit(FLAG_IS_HOGGED, &desc->flags) == 1) {
 		desc_set_label(desc, label ? : "?");
 		status = 0;
 	} else {
-- 
2.1.4

Re: [PATCH 3/9] gpio: Allow hogged gpios to be requested

[Uwe Kleine-König]

Hello,

On Fri, Jul 17, 2015 at 11:32:44AM +0200, Markus Pargmann wrote:
> It can be useful to claim hogged gpios later, for example from
> userspace. This allows to set defaults for GPIOs using the hogging
> mechanism and override the setup later from userspace or a kernel driver.
> 
> This patch adds a check for hogged gpios to allow requesting them. If
> the gpio is not hogged but marked as requested, it still fails with
> -EBUSY.
> 
> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> ---
>  drivers/gpio/gpiolib.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index bf4bd1d120c3..9f402b159cbe 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -798,7 +798,8 @@ static int __gpiod_request(struct gpio_desc *desc, const char *label)
>  	 * before IRQs are enabled, for non-sleeping (SOC) GPIOs.
>  	 */
>  
> -	if (test_and_set_bit(FLAG_REQUESTED, &desc->flags) == 0) {
> +	if (test_and_set_bit(FLAG_REQUESTED, &desc->flags) == 0 ||
> +	    test_and_clear_bit(FLAG_IS_HOGGED, &desc->flags) == 1) {
>  		desc_set_label(desc, label ? : "?");
>  		status = 0;
I don't like this patch. IMHO hogging is a "use" of a GPIO that should
prevent it being requested.

While I think it's useful to be able to export some hogged pins I don't
think this should be possible for all hogged pins unconditionally. And
for gpios being used by drivers I'd expect they don't need to be hogged
at all.

I don't have a good idea how to solve that. Adding another property to a
gpio that should be allowd to be exported can hardly count as hardware
description and so doesn't belong in a device tree?!

Please don't consider this objection as a reason for a NACK, only as
starting point for a discussion.

Apart from that: Does this result in hogged gpios being able to be
requested by two additional drivers in parallel? Maybe the IS_HOGGED
flag should be dropped when the gpio is requested?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
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

[PATCH 3/9] gpio: Allow hogged gpios to be requested

[Uwe Kleine-König]

Hello,

On Fri, Jul 17, 2015 at 11:32:44AM +0200, Markus Pargmann wrote:
> It can be useful to claim hogged gpios later, for example from
> userspace. This allows to set defaults for GPIOs using the hogging
> mechanism and override the setup later from userspace or a kernel driver.
> 
> This patch adds a check for hogged gpios to allow requesting them. If
> the gpio is not hogged but marked as requested, it still fails with
> -EBUSY.
> 
> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> ---
>  drivers/gpio/gpiolib.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index bf4bd1d120c3..9f402b159cbe 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -798,7 +798,8 @@ static int __gpiod_request(struct gpio_desc *desc, const char *label)
>  	 * before IRQs are enabled, for non-sleeping (SOC) GPIOs.
>  	 */
>  
> -	if (test_and_set_bit(FLAG_REQUESTED, &desc->flags) == 0) {
> +	if (test_and_set_bit(FLAG_REQUESTED, &desc->flags) == 0 ||
> +	    test_and_clear_bit(FLAG_IS_HOGGED, &desc->flags) == 1) {
>  		desc_set_label(desc, label ? : "?");
>  		status = 0;
I don't like this patch. IMHO hogging is a "use" of a GPIO that should
prevent it being requested.

While I think it's useful to be able to export some hogged pins I don't
think this should be possible for all hogged pins unconditionally. And
for gpios being used by drivers I'd expect they don't need to be hogged
at all.

I don't have a good idea how to solve that. Adding another property to a
gpio that should be allowd to be exported can hardly count as hardware
description and so doesn't belong in a device tree?!

Please don't consider this objection as a reason for a NACK, only as
starting point for a discussion.

Apart from that: Does this result in hogged gpios being able to be
requested by two additional drivers in parallel? Maybe the IS_HOGGED
flag should be dropped when the gpio is requested?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 3/9] gpio: Allow hogged gpios to be requested

[Markus Pargmann]

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

Hi Uwe,

On Fri, Jul 17, 2015 at 10:27:02PM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> On Fri, Jul 17, 2015 at 11:32:44AM +0200, Markus Pargmann wrote:
> > It can be useful to claim hogged gpios later, for example from
> > userspace. This allows to set defaults for GPIOs using the hogging
> > mechanism and override the setup later from userspace or a kernel driver.
> > 
> > This patch adds a check for hogged gpios to allow requesting them. If
> > the gpio is not hogged but marked as requested, it still fails with
> > -EBUSY.
> > 
> > Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> > ---
> >  drivers/gpio/gpiolib.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index bf4bd1d120c3..9f402b159cbe 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -798,7 +798,8 @@ static int __gpiod_request(struct gpio_desc *desc, const char *label)
> >  	 * before IRQs are enabled, for non-sleeping (SOC) GPIOs.
> >  	 */
> >  
> > -	if (test_and_set_bit(FLAG_REQUESTED, &desc->flags) == 0) {
> > +	if (test_and_set_bit(FLAG_REQUESTED, &desc->flags) == 0 ||
> > +	    test_and_clear_bit(FLAG_IS_HOGGED, &desc->flags) == 1) {
> >  		desc_set_label(desc, label ? : "?");
> >  		status = 0;
> I don't like this patch. IMHO hogging is a "use" of a GPIO that should
> prevent it being requested.

I disagree with you here. The original patch stated in its description
that it was designed to initialize GPIOs. In my understanding this does
not necessarily mean that a hogged GPIO has to be blocked forever.

It may be a use case for example to initialize multiplexers to known
safe values to work with the system right from the beginning. Later it
may be necessary to change them.

> 
> While I think it's useful to be able to export some hogged pins I don't
> think this should be possible for all hogged pins unconditionally. And
> for gpios being used by drivers I'd expect they don't need to be hogged
> at all.
> 
> I don't have a good idea how to solve that. Adding another property to a
> gpio that should be allowd to be exported can hardly count as hardware
> description and so doesn't belong in a device tree?!
> 
> Please don't consider this objection as a reason for a NACK, only as
> starting point for a discussion.
> 
> Apart from that: Does this result in hogged gpios being able to be
> requested by two additional drivers in parallel? Maybe the IS_HOGGED
> flag should be dropped when the gpio is requested?

The IS_HOGGED flag is cleared at the same time it is tested so only one
consumer can request one hogged GPIO. The GPIO is not considered to be
hogged after it is normally requested.

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 3/9] gpio: Allow hogged gpios to be requested

[Markus Pargmann]

Hi Uwe,

On Fri, Jul 17, 2015 at 10:27:02PM +0200, Uwe Kleine-K?nig wrote:
> Hello,
> 
> On Fri, Jul 17, 2015 at 11:32:44AM +0200, Markus Pargmann wrote:
> > It can be useful to claim hogged gpios later, for example from
> > userspace. This allows to set defaults for GPIOs using the hogging
> > mechanism and override the setup later from userspace or a kernel driver.
> > 
> > This patch adds a check for hogged gpios to allow requesting them. If
> > the gpio is not hogged but marked as requested, it still fails with
> > -EBUSY.
> > 
> > Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> > ---
> >  drivers/gpio/gpiolib.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index bf4bd1d120c3..9f402b159cbe 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -798,7 +798,8 @@ static int __gpiod_request(struct gpio_desc *desc, const char *label)
> >  	 * before IRQs are enabled, for non-sleeping (SOC) GPIOs.
> >  	 */
> >  
> > -	if (test_and_set_bit(FLAG_REQUESTED, &desc->flags) == 0) {
> > +	if (test_and_set_bit(FLAG_REQUESTED, &desc->flags) == 0 ||
> > +	    test_and_clear_bit(FLAG_IS_HOGGED, &desc->flags) == 1) {
> >  		desc_set_label(desc, label ? : "?");
> >  		status = 0;
> I don't like this patch. IMHO hogging is a "use" of a GPIO that should
> prevent it being requested.

I disagree with you here. The original patch stated in its description
that it was designed to initialize GPIOs. In my understanding this does
not necessarily mean that a hogged GPIO has to be blocked forever.

It may be a use case for example to initialize multiplexers to known
safe values to work with the system right from the beginning. Later it
may be necessary to change them.

> 
> While I think it's useful to be able to export some hogged pins I don't
> think this should be possible for all hogged pins unconditionally. And
> for gpios being used by drivers I'd expect they don't need to be hogged
> at all.
> 
> I don't have a good idea how to solve that. Adding another property to a
> gpio that should be allowd to be exported can hardly count as hardware
> description and so doesn't belong in a device tree?!
> 
> Please don't consider this objection as a reason for a NACK, only as
> starting point for a discussion.
> 
> Apart from that: Does this result in hogged gpios being able to be
> requested by two additional drivers in parallel? Maybe the IS_HOGGED
> flag should be dropped when the gpio is requested?

The IS_HOGGED flag is cleared at the same time it is tested so only one
consumer can request one hogged GPIO. The GPIO is not considered to be
hogged after it is normally requested.

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 |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150719/b22ebe66/attachment.sig>

Re: [PATCH 3/9] gpio: Allow hogged gpios to be requested

[Uwe Kleine-König]

Hello Markus,

On Sun, Jul 19, 2015 at 04:01:42PM +0200, Markus Pargmann wrote:
> On Fri, Jul 17, 2015 at 10:27:02PM +0200, Uwe Kleine-König wrote:
> > On Fri, Jul 17, 2015 at 11:32:44AM +0200, Markus Pargmann wrote:
> > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > > index bf4bd1d120c3..9f402b159cbe 100644
> > > --- a/drivers/gpio/gpiolib.c
> > > +++ b/drivers/gpio/gpiolib.c
> > > @@ -798,7 +798,8 @@ static int __gpiod_request(struct gpio_desc *desc, const char *label)
> > >  	 * before IRQs are enabled, for non-sleeping (SOC) GPIOs.
> > >  	 */
> > >  
> > > -	if (test_and_set_bit(FLAG_REQUESTED, &desc->flags) == 0) {
> > > +	if (test_and_set_bit(FLAG_REQUESTED, &desc->flags) == 0 ||
> > > +	    test_and_clear_bit(FLAG_IS_HOGGED, &desc->flags) == 1) {
> > >  		desc_set_label(desc, label ? : "?");
> > >  		status = 0;
> > I don't like this patch. IMHO hogging is a "use" of a GPIO that should
> > prevent it being requested.
> 
> I disagree with you here. The original patch stated in its description
> that it was designed to initialize GPIOs. In my understanding this does
> not necessarily mean that a hogged GPIO has to be blocked forever.
Assume for a moment I can agree with "not necessarily". But now, what
about the cases where a hogged GPIO should be blocked?
IMHO, if you want to drive the GPIO from userspace anyhow, you don't
need to add a hog for it.

> The IS_HOGGED flag is cleared at the same time it is tested so only one
> consumer can request one hogged GPIO. The GPIO is not considered to be
> hogged after it is normally requested.
You're right here, I missed the and_clear_bit part on the test.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
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

[PATCH 3/9] gpio: Allow hogged gpios to be requested

[Uwe Kleine-König]

Hello Markus,

On Sun, Jul 19, 2015 at 04:01:42PM +0200, Markus Pargmann wrote:
> On Fri, Jul 17, 2015 at 10:27:02PM +0200, Uwe Kleine-K?nig wrote:
> > On Fri, Jul 17, 2015 at 11:32:44AM +0200, Markus Pargmann wrote:
> > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > > index bf4bd1d120c3..9f402b159cbe 100644
> > > --- a/drivers/gpio/gpiolib.c
> > > +++ b/drivers/gpio/gpiolib.c
> > > @@ -798,7 +798,8 @@ static int __gpiod_request(struct gpio_desc *desc, const char *label)
> > >  	 * before IRQs are enabled, for non-sleeping (SOC) GPIOs.
> > >  	 */
> > >  
> > > -	if (test_and_set_bit(FLAG_REQUESTED, &desc->flags) == 0) {
> > > +	if (test_and_set_bit(FLAG_REQUESTED, &desc->flags) == 0 ||
> > > +	    test_and_clear_bit(FLAG_IS_HOGGED, &desc->flags) == 1) {
> > >  		desc_set_label(desc, label ? : "?");
> > >  		status = 0;
> > I don't like this patch. IMHO hogging is a "use" of a GPIO that should
> > prevent it being requested.
> 
> I disagree with you here. The original patch stated in its description
> that it was designed to initialize GPIOs. In my understanding this does
> not necessarily mean that a hogged GPIO has to be blocked forever.
Assume for a moment I can agree with "not necessarily". But now, what
about the cases where a hogged GPIO should be blocked?
IMHO, if you want to drive the GPIO from userspace anyhow, you don't
need to add a hog for it.

> The IS_HOGGED flag is cleared at the same time it is tested so only one
> consumer can request one hogged GPIO. The GPIO is not considered to be
> hogged after it is normally requested.
You're right here, I missed the and_clear_bit part on the test.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 3/9] gpio: Allow hogged gpios to be requested

[Markus Pargmann]

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

On Mon, Jul 20, 2015 at 08:32:49AM +0200, Uwe Kleine-König wrote:
> Hello Markus,
> 
> On Sun, Jul 19, 2015 at 04:01:42PM +0200, Markus Pargmann wrote:
> > On Fri, Jul 17, 2015 at 10:27:02PM +0200, Uwe Kleine-König wrote:
> > > On Fri, Jul 17, 2015 at 11:32:44AM +0200, Markus Pargmann wrote:
> > > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > > > index bf4bd1d120c3..9f402b159cbe 100644
> > > > --- a/drivers/gpio/gpiolib.c
> > > > +++ b/drivers/gpio/gpiolib.c
> > > > @@ -798,7 +798,8 @@ static int __gpiod_request(struct gpio_desc *desc, const char *label)
> > > >  	 * before IRQs are enabled, for non-sleeping (SOC) GPIOs.
> > > >  	 */
> > > >  
> > > > -	if (test_and_set_bit(FLAG_REQUESTED, &desc->flags) == 0) {
> > > > +	if (test_and_set_bit(FLAG_REQUESTED, &desc->flags) == 0 ||
> > > > +	    test_and_clear_bit(FLAG_IS_HOGGED, &desc->flags) == 1) {
> > > >  		desc_set_label(desc, label ? : "?");
> > > >  		status = 0;
> > > I don't like this patch. IMHO hogging is a "use" of a GPIO that should
> > > prevent it being requested.
> > 
> > I disagree with you here. The original patch stated in its description
> > that it was designed to initialize GPIOs. In my understanding this does
> > not necessarily mean that a hogged GPIO has to be blocked forever.
> Assume for a moment I can agree with "not necessarily". But now, what
> about the cases where a hogged GPIO should be blocked?
> IMHO, if you want to drive the GPIO from userspace anyhow, you don't
> need to add a hog for it.

If I don't use a hog I leave the system in an undefined state until the
userspace can initialize the GPIOs. So all the userspace controlled
GPIOs are undefined when all the drivers probe which can lead to
problems if the GPIOs have any indirect influence on hardware components
which drivers probe before the userspace.
I think it is valid to somehow define a safe state for GPIOs in the
devicetree while you can later change this state from userspace?!

Best regards,

Markus

> 
> > The IS_HOGGED flag is cleared at the same time it is tested so only one
> > consumer can request one hogged GPIO. The GPIO is not considered to be
> > hogged after it is normally requested.
> You're right here, I missed the and_clear_bit part on the test.
> 
> Best regards
> Uwe
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> 

-- 
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 3/9] gpio: Allow hogged gpios to be requested

[Markus Pargmann]

On Mon, Jul 20, 2015 at 08:32:49AM +0200, Uwe Kleine-K?nig wrote:
> Hello Markus,
> 
> On Sun, Jul 19, 2015 at 04:01:42PM +0200, Markus Pargmann wrote:
> > On Fri, Jul 17, 2015 at 10:27:02PM +0200, Uwe Kleine-K?nig wrote:
> > > On Fri, Jul 17, 2015 at 11:32:44AM +0200, Markus Pargmann wrote:
> > > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > > > index bf4bd1d120c3..9f402b159cbe 100644
> > > > --- a/drivers/gpio/gpiolib.c
> > > > +++ b/drivers/gpio/gpiolib.c
> > > > @@ -798,7 +798,8 @@ static int __gpiod_request(struct gpio_desc *desc, const char *label)
> > > >  	 * before IRQs are enabled, for non-sleeping (SOC) GPIOs.
> > > >  	 */
> > > >  
> > > > -	if (test_and_set_bit(FLAG_REQUESTED, &desc->flags) == 0) {
> > > > +	if (test_and_set_bit(FLAG_REQUESTED, &desc->flags) == 0 ||
> > > > +	    test_and_clear_bit(FLAG_IS_HOGGED, &desc->flags) == 1) {
> > > >  		desc_set_label(desc, label ? : "?");
> > > >  		status = 0;
> > > I don't like this patch. IMHO hogging is a "use" of a GPIO that should
> > > prevent it being requested.
> > 
> > I disagree with you here. The original patch stated in its description
> > that it was designed to initialize GPIOs. In my understanding this does
> > not necessarily mean that a hogged GPIO has to be blocked forever.
> Assume for a moment I can agree with "not necessarily". But now, what
> about the cases where a hogged GPIO should be blocked?
> IMHO, if you want to drive the GPIO from userspace anyhow, you don't
> need to add a hog for it.

If I don't use a hog I leave the system in an undefined state until the
userspace can initialize the GPIOs. So all the userspace controlled
GPIOs are undefined when all the drivers probe which can lead to
problems if the GPIOs have any indirect influence on hardware components
which drivers probe before the userspace.
I think it is valid to somehow define a safe state for GPIOs in the
devicetree while you can later change this state from userspace?!

Best regards,

Markus

> 
> > The IS_HOGGED flag is cleared at the same time it is tested so only one
> > consumer can request one hogged GPIO. The GPIO is not considered to be
> > hogged after it is normally requested.
> You're right here, I missed the and_clear_bit part on the test.
> 
> Best regards
> Uwe
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-K?nig            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> 

-- 
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 |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150720/382d68cd/attachment.sig>

Re: [PATCH 3/9] gpio: Allow hogged gpios to be requested

[Johan Hovold]

On Sun, Jul 19, 2015 at 04:01:42PM +0200, Markus Pargmann wrote:
> Hi Uwe,
> 
> On Fri, Jul 17, 2015 at 10:27:02PM +0200, Uwe Kleine-König wrote:
> > Hello,
> > 
> > On Fri, Jul 17, 2015 at 11:32:44AM +0200, Markus Pargmann wrote:
> > > It can be useful to claim hogged gpios later, for example from
> > > userspace. This allows to set defaults for GPIOs using the hogging
> > > mechanism and override the setup later from userspace or a kernel driver.
> > > 
> > > This patch adds a check for hogged gpios to allow requesting them. If
> > > the gpio is not hogged but marked as requested, it still fails with
> > > -EBUSY.
> > > 
> > > Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> > > ---
> > >  drivers/gpio/gpiolib.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > > index bf4bd1d120c3..9f402b159cbe 100644
> > > --- a/drivers/gpio/gpiolib.c
> > > +++ b/drivers/gpio/gpiolib.c
> > > @@ -798,7 +798,8 @@ static int __gpiod_request(struct gpio_desc *desc, const char *label)
> > >  	 * before IRQs are enabled, for non-sleeping (SOC) GPIOs.
> > >  	 */
> > >  
> > > -	if (test_and_set_bit(FLAG_REQUESTED, &desc->flags) == 0) {
> > > +	if (test_and_set_bit(FLAG_REQUESTED, &desc->flags) == 0 ||
> > > +	    test_and_clear_bit(FLAG_IS_HOGGED, &desc->flags) == 1) {
> > >  		desc_set_label(desc, label ? : "?");
> > >  		status = 0;
> > I don't like this patch. IMHO hogging is a "use" of a GPIO that should
> > prevent it being requested.
> 
> I disagree with you here. The original patch stated in its description
> that it was designed to initialize GPIOs. In my understanding this does
> not necessarily mean that a hogged GPIO has to be blocked forever.

IIRC, this use case was discussed but was rejected by Linus when hogs
were added:

	https://lkml.kernel.org/r/CACRpkdZcNcPBYQM438CZJx1gYst9BFBSTj-3Qv2aPGF9pdWa5g@mail.gmail.com

Linus?

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

[PATCH 3/9] gpio: Allow hogged gpios to be requested

[Johan Hovold]

On Sun, Jul 19, 2015 at 04:01:42PM +0200, Markus Pargmann wrote:
> Hi Uwe,
> 
> On Fri, Jul 17, 2015 at 10:27:02PM +0200, Uwe Kleine-K?nig wrote:
> > Hello,
> > 
> > On Fri, Jul 17, 2015 at 11:32:44AM +0200, Markus Pargmann wrote:
> > > It can be useful to claim hogged gpios later, for example from
> > > userspace. This allows to set defaults for GPIOs using the hogging
> > > mechanism and override the setup later from userspace or a kernel driver.
> > > 
> > > This patch adds a check for hogged gpios to allow requesting them. If
> > > the gpio is not hogged but marked as requested, it still fails with
> > > -EBUSY.
> > > 
> > > Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> > > ---
> > >  drivers/gpio/gpiolib.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > > index bf4bd1d120c3..9f402b159cbe 100644
> > > --- a/drivers/gpio/gpiolib.c
> > > +++ b/drivers/gpio/gpiolib.c
> > > @@ -798,7 +798,8 @@ static int __gpiod_request(struct gpio_desc *desc, const char *label)
> > >  	 * before IRQs are enabled, for non-sleeping (SOC) GPIOs.
> > >  	 */
> > >  
> > > -	if (test_and_set_bit(FLAG_REQUESTED, &desc->flags) == 0) {
> > > +	if (test_and_set_bit(FLAG_REQUESTED, &desc->flags) == 0 ||
> > > +	    test_and_clear_bit(FLAG_IS_HOGGED, &desc->flags) == 1) {
> > >  		desc_set_label(desc, label ? : "?");
> > >  		status = 0;
> > I don't like this patch. IMHO hogging is a "use" of a GPIO that should
> > prevent it being requested.
> 
> I disagree with you here. The original patch stated in its description
> that it was designed to initialize GPIOs. In my understanding this does
> not necessarily mean that a hogged GPIO has to be blocked forever.

IIRC, this use case was discussed but was rejected by Linus when hogs
were added:

	https://lkml.kernel.org/r/CACRpkdZcNcPBYQM438CZJx1gYst9BFBSTj-3Qv2aPGF9pdWa5g at mail.gmail.com

Linus?

Johan

Re: [PATCH 3/9] gpio: Allow hogged gpios to be requested

[Markus Pargmann]

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

On Tue, Jul 28, 2015 at 11:17:53AM +0200, Johan Hovold wrote:
> On Sun, Jul 19, 2015 at 04:01:42PM +0200, Markus Pargmann wrote:
> > Hi Uwe,
> > 
> > On Fri, Jul 17, 2015 at 10:27:02PM +0200, Uwe Kleine-König wrote:
> > > Hello,
> > > 
> > > On Fri, Jul 17, 2015 at 11:32:44AM +0200, Markus Pargmann wrote:
> > > > It can be useful to claim hogged gpios later, for example from
> > > > userspace. This allows to set defaults for GPIOs using the hogging
> > > > mechanism and override the setup later from userspace or a kernel driver.
> > > > 
> > > > This patch adds a check for hogged gpios to allow requesting them. If
> > > > the gpio is not hogged but marked as requested, it still fails with
> > > > -EBUSY.
> > > > 
> > > > Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> > > > ---
> > > >  drivers/gpio/gpiolib.c | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > > > index bf4bd1d120c3..9f402b159cbe 100644
> > > > --- a/drivers/gpio/gpiolib.c
> > > > +++ b/drivers/gpio/gpiolib.c
> > > > @@ -798,7 +798,8 @@ static int __gpiod_request(struct gpio_desc *desc, const char *label)
> > > >  	 * before IRQs are enabled, for non-sleeping (SOC) GPIOs.
> > > >  	 */
> > > >  
> > > > -	if (test_and_set_bit(FLAG_REQUESTED, &desc->flags) == 0) {
> > > > +	if (test_and_set_bit(FLAG_REQUESTED, &desc->flags) == 0 ||
> > > > +	    test_and_clear_bit(FLAG_IS_HOGGED, &desc->flags) == 1) {
> > > >  		desc_set_label(desc, label ? : "?");
> > > >  		status = 0;
> > > I don't like this patch. IMHO hogging is a "use" of a GPIO that should
> > > prevent it being requested.
> > 
> > I disagree with you here. The original patch stated in its description
> > that it was designed to initialize GPIOs. In my understanding this does
> > not necessarily mean that a hogged GPIO has to be blocked forever.
> 
> IIRC, this use case was discussed but was rejected by Linus when hogs
> were added:
> 
> 	https://lkml.kernel.org/r/CACRpkdZcNcPBYQM438CZJx1gYst9BFBSTj-3Qv2aPGF9pdWa5g@mail.gmail.com

I see. Yes I really would like to have this 'initial settings' feature.
Any suggestions for a better way?

Best regards,

Markus

> 
> Linus?
> 
> Johan
> 

-- 
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 3/9] gpio: Allow hogged gpios to be requested

[Markus Pargmann]

On Tue, Jul 28, 2015 at 11:17:53AM +0200, Johan Hovold wrote:
> On Sun, Jul 19, 2015 at 04:01:42PM +0200, Markus Pargmann wrote:
> > Hi Uwe,
> > 
> > On Fri, Jul 17, 2015 at 10:27:02PM +0200, Uwe Kleine-K?nig wrote:
> > > Hello,
> > > 
> > > On Fri, Jul 17, 2015 at 11:32:44AM +0200, Markus Pargmann wrote:
> > > > It can be useful to claim hogged gpios later, for example from
> > > > userspace. This allows to set defaults for GPIOs using the hogging
> > > > mechanism and override the setup later from userspace or a kernel driver.
> > > > 
> > > > This patch adds a check for hogged gpios to allow requesting them. If
> > > > the gpio is not hogged but marked as requested, it still fails with
> > > > -EBUSY.
> > > > 
> > > > Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> > > > ---
> > > >  drivers/gpio/gpiolib.c | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > > > index bf4bd1d120c3..9f402b159cbe 100644
> > > > --- a/drivers/gpio/gpiolib.c
> > > > +++ b/drivers/gpio/gpiolib.c
> > > > @@ -798,7 +798,8 @@ static int __gpiod_request(struct gpio_desc *desc, const char *label)
> > > >  	 * before IRQs are enabled, for non-sleeping (SOC) GPIOs.
> > > >  	 */
> > > >  
> > > > -	if (test_and_set_bit(FLAG_REQUESTED, &desc->flags) == 0) {
> > > > +	if (test_and_set_bit(FLAG_REQUESTED, &desc->flags) == 0 ||
> > > > +	    test_and_clear_bit(FLAG_IS_HOGGED, &desc->flags) == 1) {
> > > >  		desc_set_label(desc, label ? : "?");
> > > >  		status = 0;
> > > I don't like this patch. IMHO hogging is a "use" of a GPIO that should
> > > prevent it being requested.
> > 
> > I disagree with you here. The original patch stated in its description
> > that it was designed to initialize GPIOs. In my understanding this does
> > not necessarily mean that a hogged GPIO has to be blocked forever.
> 
> IIRC, this use case was discussed but was rejected by Linus when hogs
> were added:
> 
> 	https://lkml.kernel.org/r/CACRpkdZcNcPBYQM438CZJx1gYst9BFBSTj-3Qv2aPGF9pdWa5g at mail.gmail.com

I see. Yes I really would like to have this 'initial settings' feature.
Any suggestions for a better way?

Best regards,

Markus

> 
> Linus?
> 
> Johan
> 

-- 
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 |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150729/9f394a80/attachment.sig>

Re: [PATCH 3/9] gpio: Allow hogged gpios to be requested

[Linus Walleij]

On Tue, Jul 28, 2015 at 11:17 AM, Johan Hovold <johan@kernel.org> wrote:
> On Sun, Jul 19, 2015 at 04:01:42PM +0200, Markus Pargmann wrote:

>> > I don't like this patch. IMHO hogging is a "use" of a GPIO that should
>> > prevent it being requested.
>>
>> I disagree with you here. The original patch stated in its description
>> that it was designed to initialize GPIOs. In my understanding this does
>> not necessarily mean that a hogged GPIO has to be blocked forever.
>
> IIRC, this use case was discussed but was rejected by Linus when hogs
> were added:
>
>         https://lkml.kernel.org/r/CACRpkdZcNcPBYQM438CZJx1gYst9BFBSTj-3Qv2aPGF9pdWa5g@mail.gmail.com
>
> Linus?

Yeah that is true. But maybe I softened up a bit.

Basically I want to avoid having two very similar mechanisms.

But that is maybe more of a question of how the code is
arranged and named than what it is called. It would be nicer
if this was referring to initial values rather than hogging,
the mechanism inside the kernel could be the same.

But then there is the path where something that is initially
a hog is turned into a userspace GPIO. Then it is essentially
not a hog anymore, so this property is not static.

Yours,
Linus Walleij

[PATCH 3/9] gpio: Allow hogged gpios to be requested

[Linus Walleij]

On Tue, Jul 28, 2015 at 11:17 AM, Johan Hovold <johan@kernel.org> wrote:
> On Sun, Jul 19, 2015 at 04:01:42PM +0200, Markus Pargmann wrote:

>> > I don't like this patch. IMHO hogging is a "use" of a GPIO that should
>> > prevent it being requested.
>>
>> I disagree with you here. The original patch stated in its description
>> that it was designed to initialize GPIOs. In my understanding this does
>> not necessarily mean that a hogged GPIO has to be blocked forever.
>
> IIRC, this use case was discussed but was rejected by Linus when hogs
> were added:
>
>         https://lkml.kernel.org/r/CACRpkdZcNcPBYQM438CZJx1gYst9BFBSTj-3Qv2aPGF9pdWa5g at mail.gmail.com
>
> Linus?

Yeah that is true. But maybe I softened up a bit.

Basically I want to avoid having two very similar mechanisms.

But that is maybe more of a question of how the code is
arranged and named than what it is called. It would be nicer
if this was referring to initial values rather than hogging,
the mechanism inside the kernel could be the same.

But then there is the path where something that is initially
a hog is turned into a userspace GPIO. Then it is essentially
not a hog anymore, so this property is not static.

Yours,
Linus Walleij

[PATCH 4/9] gpio: Add 'name' to the gpio descriptor struct

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

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

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);
-- 
2.1.4

[PATCH 4/9] gpio: Add 'name' to the gpio descriptor struct

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

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

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);
-- 
2.1.4

Re: [PATCH 4/9] gpio: Add 'name' to the gpio descriptor struct

[Johan Hovold]

On Fri, Jul 17, 2015 at 11:32:45AM +0200, Markus Pargmann 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.

You merge this one with whatever patch starts using the name field.

Johan

[PATCH 4/9] gpio: Add 'name' to the gpio descriptor struct

[Johan Hovold]

On Fri, Jul 17, 2015 at 11:32:45AM +0200, Markus Pargmann 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.

You merge this one with whatever patch starts using the name field.

Johan

[PATCH 5/9] gpiolib: Implement gpio_name_to_desc()

[Markus Pargmann]

Implement a function to translate a gpio name to a descriptor.

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

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 9f402b159cbe..dcac3bcf21dd 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/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

[PATCH 5/9] gpiolib: Implement gpio_name_to_desc()

[Markus Pargmann]

Implement a function to translate a gpio name to a descriptor.

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

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 9f402b159cbe..dcac3bcf21dd 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/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

[PATCH 6/9] 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 | 29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 64950591a764..cefc665a558c 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,31 @@ static void of_gpiochip_scan_gpios(struct gpio_chip *chip)
 
 	for_each_child_of_node(chip->of_node, np) {
 		const char *name = NULL;
-		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;
+		if (name) {
+			name_desc = gpio_name_to_desc(name);
+			if (name_desc)
+				pr_err("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

[PATCH 6/9] 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 | 29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 64950591a764..cefc665a558c 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,31 @@ static void of_gpiochip_scan_gpios(struct gpio_chip *chip)
 
 	for_each_child_of_node(chip->of_node, np) {
 		const char *name = NULL;
-		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;
+		if (name) {
+			name_desc = gpio_name_to_desc(name);
+			if (name_desc)
+				pr_err("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 6/9] gpiolib-of: Reuse 'line-name' from DT as gpio descriptor name

[Johan Hovold]

On Fri, Jul 17, 2015 at 11:32:47AM +0200, Markus Pargmann 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>
> ---
>  drivers/gpio/gpiolib-of.c | 29 ++++++++++++++++++++---------
>  1 file changed, 20 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> index 64950591a764..cefc665a558c 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,31 @@ static void of_gpiochip_scan_gpios(struct gpio_chip *chip)
>  
>  	for_each_child_of_node(chip->of_node, np) {
>  		const char *name = NULL;
> -		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;
> +		if (name) {
> +			name_desc = gpio_name_to_desc(name);
> +			if (name_desc)
> +				pr_err("GPIO name collision for '%s' detected at GPIO line %d (%s)\n",
> +				       name, desc_to_gpio(desc), np->name);
> +			else
> +				desc->name = name;

This will introduce a regression. As I mentioned in a comment on an
earlier patch, of_get_gpio_hog falls back to using the node name and
those need not be unique.

Johan

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

[Johan Hovold]

On Fri, Jul 17, 2015 at 11:32:47AM +0200, Markus Pargmann 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>
> ---
>  drivers/gpio/gpiolib-of.c | 29 ++++++++++++++++++++---------
>  1 file changed, 20 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> index 64950591a764..cefc665a558c 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,31 @@ static void of_gpiochip_scan_gpios(struct gpio_chip *chip)
>  
>  	for_each_child_of_node(chip->of_node, np) {
>  		const char *name = NULL;
> -		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;
> +		if (name) {
> +			name_desc = gpio_name_to_desc(name);
> +			if (name_desc)
> +				pr_err("GPIO name collision for '%s' detected at GPIO line %d (%s)\n",
> +				       name, desc_to_gpio(desc), np->name);
> +			else
> +				desc->name = name;

This will introduce a regression. As I mentioned in a comment on an
earlier patch, of_get_gpio_hog falls back to using the node name and
those need not be unique.

Johan

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

[Markus Pargmann]

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

On Tue, Jul 28, 2015 at 11:31:05AM +0200, Johan Hovold wrote:
> On Fri, Jul 17, 2015 at 11:32:47AM +0200, Markus Pargmann 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>
> > ---
> >  drivers/gpio/gpiolib-of.c | 29 ++++++++++++++++++++---------
> >  1 file changed, 20 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> > index 64950591a764..cefc665a558c 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,31 @@ static void of_gpiochip_scan_gpios(struct gpio_chip *chip)
> >  
> >  	for_each_child_of_node(chip->of_node, np) {
> >  		const char *name = NULL;
> > -		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;
> > +		if (name) {
> > +			name_desc = gpio_name_to_desc(name);
> > +			if (name_desc)
> > +				pr_err("GPIO name collision for '%s' detected at GPIO line %d (%s)\n",
> > +				       name, desc_to_gpio(desc), np->name);
> > +			else
> > +				desc->name = name;
> 
> This will introduce a regression. As I mentioned in a comment on an
> earlier patch, of_get_gpio_hog falls back to using the node name and
> those need not be unique.

Yes, will fix this as well.

Thanks,

Markus

> 
> Johan
> 

-- 
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 6/9] gpiolib-of: Reuse 'line-name' from DT as gpio descriptor name

[Markus Pargmann]

On Tue, Jul 28, 2015 at 11:31:05AM +0200, Johan Hovold wrote:
> On Fri, Jul 17, 2015 at 11:32:47AM +0200, Markus Pargmann 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>
> > ---
> >  drivers/gpio/gpiolib-of.c | 29 ++++++++++++++++++++---------
> >  1 file changed, 20 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> > index 64950591a764..cefc665a558c 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,31 @@ static void of_gpiochip_scan_gpios(struct gpio_chip *chip)
> >  
> >  	for_each_child_of_node(chip->of_node, np) {
> >  		const char *name = NULL;
> > -		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;
> > +		if (name) {
> > +			name_desc = gpio_name_to_desc(name);
> > +			if (name_desc)
> > +				pr_err("GPIO name collision for '%s' detected at GPIO line %d (%s)\n",
> > +				       name, desc_to_gpio(desc), np->name);
> > +			else
> > +				desc->name = name;
> 
> This will introduce a regression. As I mentioned in a comment on an
> earlier patch, of_get_gpio_hog falls back to using the node name and
> those need not be unique.

Yes, will fix this as well.

Thanks,

Markus

> 
> Johan
> 

-- 
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 |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150729/28df365e/attachment-0001.sig>

[PATCH 7/9] gpiolib-sysfs: Add gpio name parsing for sysfs export

[Markus Pargmann]

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

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index b57ed8e55ab5..c3b74440ca67 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -443,18 +443,25 @@ 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);
+
+		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 +492,25 @@ 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);
+
+		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 +524,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

[PATCH 7/9] gpiolib-sysfs: Add gpio name parsing for sysfs export

[Markus Pargmann]

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

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index b57ed8e55ab5..c3b74440ca67 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -443,18 +443,25 @@ 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);
+
+		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 +492,25 @@ 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);
+
+		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 +524,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 7/9] gpiolib-sysfs: Add gpio name parsing for sysfs export

[Johan Hovold]

On Fri, Jul 17, 2015 at 11:32:48AM +0200, Markus Pargmann wrote:
> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> ---
>  drivers/gpio/gpiolib-sysfs.c | 41 ++++++++++++++++++++++++++++-------------
>  1 file changed, 28 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
> index b57ed8e55ab5..c3b74440ca67 100644
> --- a/drivers/gpio/gpiolib-sysfs.c
> +++ b/drivers/gpio/gpiolib-sysfs.c
> @@ -443,18 +443,25 @@ 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);

Must check for allocation failures, but why use kstrdup at all?

> +
> +		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 +492,25 @@ 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);

Same here.

> +
> +		desc = gpio_name_to_desc(strim(gpio_name));
> +		kfree(gpio_name);
> +	}
> +

Random whitespace change.

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

Johan

[PATCH 7/9] gpiolib-sysfs: Add gpio name parsing for sysfs export

[Johan Hovold]

On Fri, Jul 17, 2015 at 11:32:48AM +0200, Markus Pargmann wrote:
> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> ---
>  drivers/gpio/gpiolib-sysfs.c | 41 ++++++++++++++++++++++++++++-------------
>  1 file changed, 28 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
> index b57ed8e55ab5..c3b74440ca67 100644
> --- a/drivers/gpio/gpiolib-sysfs.c
> +++ b/drivers/gpio/gpiolib-sysfs.c
> @@ -443,18 +443,25 @@ 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);

Must check for allocation failures, but why use kstrdup at all?

> +
> +		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 +492,25 @@ 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);

Same here.

> +
> +		desc = gpio_name_to_desc(strim(gpio_name));
> +		kfree(gpio_name);
> +	}
> +

Random whitespace change.

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

Johan

Re: [PATCH 7/9] gpiolib-sysfs: Add gpio name parsing for sysfs export

[Markus Pargmann]

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

On Tue, Jul 28, 2015 at 11:50:04AM +0200, Johan Hovold wrote:
> On Fri, Jul 17, 2015 at 11:32:48AM +0200, Markus Pargmann wrote:
> > Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> > ---
> >  drivers/gpio/gpiolib-sysfs.c | 41 ++++++++++++++++++++++++++++-------------
> >  1 file changed, 28 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
> > index b57ed8e55ab5..c3b74440ca67 100644
> > --- a/drivers/gpio/gpiolib-sysfs.c
> > +++ b/drivers/gpio/gpiolib-sysfs.c
> > @@ -443,18 +443,25 @@ 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);
> 
> Must check for allocation failures, but why use kstrdup at all?

Yes have to check that.
strim() may modify the buf variable which is constant.

Best regards,

Markus

> 
> > +
> > +		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 +492,25 @@ 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);
> 
> Same here.
> 
> > +
> > +		desc = gpio_name_to_desc(strim(gpio_name));
> > +		kfree(gpio_name);
> > +	}
> > +
> 
> Random whitespace change.
> 
> >  
> > -	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;
> >  	}
> 
> Johan
> 

-- 
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 7/9] gpiolib-sysfs: Add gpio name parsing for sysfs export

[Markus Pargmann]

On Tue, Jul 28, 2015 at 11:50:04AM +0200, Johan Hovold wrote:
> On Fri, Jul 17, 2015 at 11:32:48AM +0200, Markus Pargmann wrote:
> > Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> > ---
> >  drivers/gpio/gpiolib-sysfs.c | 41 ++++++++++++++++++++++++++++-------------
> >  1 file changed, 28 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
> > index b57ed8e55ab5..c3b74440ca67 100644
> > --- a/drivers/gpio/gpiolib-sysfs.c
> > +++ b/drivers/gpio/gpiolib-sysfs.c
> > @@ -443,18 +443,25 @@ 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);
> 
> Must check for allocation failures, but why use kstrdup at all?

Yes have to check that.
strim() may modify the buf variable which is constant.

Best regards,

Markus

> 
> > +
> > +		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 +492,25 @@ 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);
> 
> Same here.
> 
> > +
> > +		desc = gpio_name_to_desc(strim(gpio_name));
> > +		kfree(gpio_name);
> > +	}
> > +
> 
> Random whitespace change.
> 
> >  
> > -	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;
> >  	}
> 
> Johan
> 

-- 
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 |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150729/d8e01ab4/attachment-0001.sig>

Re: [PATCH 7/9] gpiolib-sysfs: Add gpio name parsing for sysfs export

[Johan Hovold]

On Wed, Jul 29, 2015 at 08:57:36AM +0200, Markus Pargmann wrote:
> On Tue, Jul 28, 2015 at 11:50:04AM +0200, Johan Hovold wrote:
> > On Fri, Jul 17, 2015 at 11:32:48AM +0200, Markus Pargmann wrote:

> > > +		char *gpio_name = kstrdup(buf, GFP_KERNEL);
> > 
> > Must check for allocation failures, but why use kstrdup at all?
> 
> Yes have to check that.
> strim() may modify the buf variable which is constant.
>
> > > +
> > > +		desc = gpio_name_to_desc(strim(gpio_name));

Ah, I missed that strim.

Johan

[PATCH 7/9] gpiolib-sysfs: Add gpio name parsing for sysfs export

[Johan Hovold]

On Wed, Jul 29, 2015 at 08:57:36AM +0200, Markus Pargmann wrote:
> On Tue, Jul 28, 2015 at 11:50:04AM +0200, Johan Hovold wrote:
> > On Fri, Jul 17, 2015 at 11:32:48AM +0200, Markus Pargmann wrote:

> > > +		char *gpio_name = kstrdup(buf, GFP_KERNEL);
> > 
> > Must check for allocation failures, but why use kstrdup at all?
> 
> Yes have to check that.
> strim() may modify the buf variable which is constant.
>
> > > +
> > > +		desc = gpio_name_to_desc(strim(gpio_name));

Ah, I missed that strim.

Johan

[PATCH 8/9] 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 | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index c3b74440ca67..058019879bab 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -139,6 +139,25 @@ static ssize_t value_store(struct device *dev,
 }
 static DEVICE_ATTR_RW(value);
 
+static ssize_t gpio_name_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct gpio_desc	*desc = dev_get_drvdata(dev);
+	ssize_t			status;
+
+	mutex_lock(&sysfs_lock);
+
+	if (!test_bit(FLAG_EXPORT, &desc->flags))
+		status = -EIO;
+	else
+		status = sprintf(buf, "%s\n", desc->name);
+
+	mutex_unlock(&sysfs_lock);
+	return status;
+}
+
+static DEVICE_ATTR(name, 0444, gpio_name_show, NULL);
+
 static irqreturn_t gpio_sysfs_irq(int irq, void *priv)
 {
 	struct gpiod_data *data = priv;
@@ -377,6 +396,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

[PATCH 8/9] 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 | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index c3b74440ca67..058019879bab 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -139,6 +139,25 @@ static ssize_t value_store(struct device *dev,
 }
 static DEVICE_ATTR_RW(value);
 
+static ssize_t gpio_name_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct gpio_desc	*desc = dev_get_drvdata(dev);
+	ssize_t			status;
+
+	mutex_lock(&sysfs_lock);
+
+	if (!test_bit(FLAG_EXPORT, &desc->flags))
+		status = -EIO;
+	else
+		status = sprintf(buf, "%s\n", desc->name);
+
+	mutex_unlock(&sysfs_lock);
+	return status;
+}
+
+static DEVICE_ATTR(name, 0444, gpio_name_show, NULL);
+
 static irqreturn_t gpio_sysfs_irq(int irq, void *priv)
 {
 	struct gpiod_data *data = priv;
@@ -377,6 +396,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 8/9] gpiolib-sysfs: Show gpio-name in /sys/class/gpio/gpio*/name

[Johan Hovold]

On Fri, Jul 17, 2015 at 11:32:49AM +0200, Markus Pargmann 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>
> ---
>  drivers/gpio/gpiolib-sysfs.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
> index c3b74440ca67..058019879bab 100644
> --- a/drivers/gpio/gpiolib-sysfs.c
> +++ b/drivers/gpio/gpiolib-sysfs.c
> @@ -139,6 +139,25 @@ static ssize_t value_store(struct device *dev,
>  }
>  static DEVICE_ATTR_RW(value);
>  
> +static ssize_t gpio_name_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	struct gpio_desc	*desc = dev_get_drvdata(dev);
> +	ssize_t			status;
> +
> +	mutex_lock(&sysfs_lock);
> +
> +	if (!test_bit(FLAG_EXPORT, &desc->flags))

No need to check flag export, and no need for the mutex either.

> +		status = -EIO;
> +	else
> +		status = sprintf(buf, "%s\n", desc->name);

Depending on sprintfs representation of NULL is not a good idea here.

> +
> +	mutex_unlock(&sysfs_lock);
> +	return status;
> +}
> +
> +static DEVICE_ATTR(name, 0444, gpio_name_show, NULL);

DEVICE_ATTR_RO

> +
>  static irqreturn_t gpio_sysfs_irq(int irq, void *priv)
>  {
>  	struct gpiod_data *data = priv;
> @@ -377,6 +396,7 @@ static struct attribute *gpio_attrs[] = {
>  	&dev_attr_edge.attr,
>  	&dev_attr_value.attr,
>  	&dev_attr_active_low.attr,
> +	&dev_attr_name.attr,
>  	NULL,
>  };

Johan

[PATCH 8/9] gpiolib-sysfs: Show gpio-name in /sys/class/gpio/gpio*/name

[Johan Hovold]

On Fri, Jul 17, 2015 at 11:32:49AM +0200, Markus Pargmann 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>
> ---
>  drivers/gpio/gpiolib-sysfs.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
> index c3b74440ca67..058019879bab 100644
> --- a/drivers/gpio/gpiolib-sysfs.c
> +++ b/drivers/gpio/gpiolib-sysfs.c
> @@ -139,6 +139,25 @@ static ssize_t value_store(struct device *dev,
>  }
>  static DEVICE_ATTR_RW(value);
>  
> +static ssize_t gpio_name_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	struct gpio_desc	*desc = dev_get_drvdata(dev);
> +	ssize_t			status;
> +
> +	mutex_lock(&sysfs_lock);
> +
> +	if (!test_bit(FLAG_EXPORT, &desc->flags))

No need to check flag export, and no need for the mutex either.

> +		status = -EIO;
> +	else
> +		status = sprintf(buf, "%s\n", desc->name);

Depending on sprintfs representation of NULL is not a good idea here.

> +
> +	mutex_unlock(&sysfs_lock);
> +	return status;
> +}
> +
> +static DEVICE_ATTR(name, 0444, gpio_name_show, NULL);

DEVICE_ATTR_RO

> +
>  static irqreturn_t gpio_sysfs_irq(int irq, void *priv)
>  {
>  	struct gpiod_data *data = priv;
> @@ -377,6 +396,7 @@ static struct attribute *gpio_attrs[] = {
>  	&dev_attr_edge.attr,
>  	&dev_attr_value.attr,
>  	&dev_attr_active_low.attr,
> +	&dev_attr_name.attr,
>  	NULL,
>  };

Johan

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

[Markus Pargmann]

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

On Tue, Jul 28, 2015 at 11:53:37AM +0200, Johan Hovold wrote:
> On Fri, Jul 17, 2015 at 11:32:49AM +0200, Markus Pargmann 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>
> > ---
> >  drivers/gpio/gpiolib-sysfs.c | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> > 
> > diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
> > index c3b74440ca67..058019879bab 100644
> > --- a/drivers/gpio/gpiolib-sysfs.c
> > +++ b/drivers/gpio/gpiolib-sysfs.c
> > @@ -139,6 +139,25 @@ static ssize_t value_store(struct device *dev,
> >  }
> >  static DEVICE_ATTR_RW(value);
> >  
> > +static ssize_t gpio_name_show(struct device *dev,
> > +		struct device_attribute *attr, char *buf)
> > +{
> > +	struct gpio_desc	*desc = dev_get_drvdata(dev);
> > +	ssize_t			status;
> > +
> > +	mutex_lock(&sysfs_lock);
> > +
> > +	if (!test_bit(FLAG_EXPORT, &desc->flags))
> 
> No need to check flag export, and no need for the mutex either.
> 
> > +		status = -EIO;
> > +	else
> > +		status = sprintf(buf, "%s\n", desc->name);
> 
> Depending on sprintfs representation of NULL is not a good idea here.
> 
> > +
> > +	mutex_unlock(&sysfs_lock);
> > +	return status;
> > +}
> > +
> > +static DEVICE_ATTR(name, 0444, gpio_name_show, NULL);
> 
> DEVICE_ATTR_RO
> 
> > +
> >  static irqreturn_t gpio_sysfs_irq(int irq, void *priv)
> >  {
> >  	struct gpiod_data *data = priv;
> > @@ -377,6 +396,7 @@ static struct attribute *gpio_attrs[] = {
> >  	&dev_attr_edge.attr,
> >  	&dev_attr_value.attr,
> >  	&dev_attr_active_low.attr,
> > +	&dev_attr_name.attr,
> >  	NULL,
> >  };

Thanks, will fix all of them.

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 8/9] gpiolib-sysfs: Show gpio-name in /sys/class/gpio/gpio*/name

[Markus Pargmann]

On Tue, Jul 28, 2015 at 11:53:37AM +0200, Johan Hovold wrote:
> On Fri, Jul 17, 2015 at 11:32:49AM +0200, Markus Pargmann 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>
> > ---
> >  drivers/gpio/gpiolib-sysfs.c | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> > 
> > diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
> > index c3b74440ca67..058019879bab 100644
> > --- a/drivers/gpio/gpiolib-sysfs.c
> > +++ b/drivers/gpio/gpiolib-sysfs.c
> > @@ -139,6 +139,25 @@ static ssize_t value_store(struct device *dev,
> >  }
> >  static DEVICE_ATTR_RW(value);
> >  
> > +static ssize_t gpio_name_show(struct device *dev,
> > +		struct device_attribute *attr, char *buf)
> > +{
> > +	struct gpio_desc	*desc = dev_get_drvdata(dev);
> > +	ssize_t			status;
> > +
> > +	mutex_lock(&sysfs_lock);
> > +
> > +	if (!test_bit(FLAG_EXPORT, &desc->flags))
> 
> No need to check flag export, and no need for the mutex either.
> 
> > +		status = -EIO;
> > +	else
> > +		status = sprintf(buf, "%s\n", desc->name);
> 
> Depending on sprintfs representation of NULL is not a good idea here.
> 
> > +
> > +	mutex_unlock(&sysfs_lock);
> > +	return status;
> > +}
> > +
> > +static DEVICE_ATTR(name, 0444, gpio_name_show, NULL);
> 
> DEVICE_ATTR_RO
> 
> > +
> >  static irqreturn_t gpio_sysfs_irq(int irq, void *priv)
> >  {
> >  	struct gpiod_data *data = priv;
> > @@ -377,6 +396,7 @@ static struct attribute *gpio_attrs[] = {
> >  	&dev_attr_edge.attr,
> >  	&dev_attr_value.attr,
> >  	&dev_attr_active_low.attr,
> > +	&dev_attr_name.attr,
> >  	NULL,
> >  };

Thanks, will fix all of them.

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 |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150729/f7122bb0/attachment-0001.sig>

[PATCH 9/9] 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 dcac3bcf21dd..0f1d1f5faf5d 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2308,14 +2308,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

[PATCH 9/9] 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 dcac3bcf21dd..0f1d1f5faf5d 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2308,14 +2308,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 9/9] gpiolib: Add gpio name information to /sys/kernel/debug/gpio

[Johan Hovold]

On Fri, Jul 17, 2015 at 11:32:50AM +0200, Markus Pargmann 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>
> ---
>  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 dcac3bcf21dd..0f1d1f5faf5d 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -2308,14 +2308,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);
> +			}

So now we'd no longer just be listing requested gpios, but on a similar
format to how requested ones used to be represented.

Then there's the debugfs as ABI discussion...

Johan

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

[Johan Hovold]

On Fri, Jul 17, 2015 at 11:32:50AM +0200, Markus Pargmann 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>
> ---
>  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 dcac3bcf21dd..0f1d1f5faf5d 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -2308,14 +2308,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);
> +			}

So now we'd no longer just be listing requested gpios, but on a similar
format to how requested ones used to be represented.

Then there's the debugfs as ABI discussion...

Johan

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

[Markus Pargmann]

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

On Tue, Jul 28, 2015 at 11:58:42AM +0200, Johan Hovold wrote:
> On Fri, Jul 17, 2015 at 11:32:50AM +0200, Markus Pargmann 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>
> > ---
> >  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 dcac3bcf21dd..0f1d1f5faf5d 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -2308,14 +2308,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);
> > +			}
> 
> So now we'd no longer just be listing requested gpios, but on a similar
> format to how requested ones used to be represented.

Better suggestions on how to display those extra information in debugfs?

> 
> Then there's the debugfs as ABI discussion...

I didn't consider debugfs as ABI as I thought it is just for debugging purposes?

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 9/9] gpiolib: Add gpio name information to /sys/kernel/debug/gpio

[Markus Pargmann]

On Tue, Jul 28, 2015 at 11:58:42AM +0200, Johan Hovold wrote:
> On Fri, Jul 17, 2015 at 11:32:50AM +0200, Markus Pargmann 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>
> > ---
> >  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 dcac3bcf21dd..0f1d1f5faf5d 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -2308,14 +2308,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);
> > +			}
> 
> So now we'd no longer just be listing requested gpios, but on a similar
> format to how requested ones used to be represented.

Better suggestions on how to display those extra information in debugfs?

> 
> Then there's the debugfs as ABI discussion...

I didn't consider debugfs as ABI as I thought it is just for debugging purposes?

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 |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150729/f78dab89/attachment.sig>

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

[Johan Hovold]

On Wed, Jul 29, 2015 at 09:08:42AM +0200, Markus Pargmann wrote:
> On Tue, Jul 28, 2015 at 11:58:42AM +0200, Johan Hovold wrote:
> > On Fri, Jul 17, 2015 at 11:32:50AM +0200, Markus Pargmann 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>
> > > ---
> > >  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 dcac3bcf21dd..0f1d1f5faf5d 100644
> > > --- a/drivers/gpio/gpiolib.c
> > > +++ b/drivers/gpio/gpiolib.c
> > > @@ -2308,14 +2308,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);
> > > +			}
> > 
> > So now we'd no longer just be listing requested gpios, but on a similar
> > format to how requested ones used to be represented.
> 
> Better suggestions on how to display those extra information in debugfs?

Perhaps a new file only listing the line names.

> > Then there's the debugfs as ABI discussion...
> 
> I didn't consider debugfs as ABI as I thought it is just for debugging
> purposes?

Some people seem to have expressed a different position:

	"The fact that something is documented (whether correctly or
        not) has absolutely _zero_ impact on anything at all. What makes
        something an ABI is that it's useful and available. The only way
        something isn't an ABI is by _explicitly_ making sure that it's
        not available even by mistake in a stable form for binary use.

        Example: kernel internal data structures and function calls. We
        make sure that you simply _cannot_ make a binary that works
        across kernel versions. That is the only way for an ABI to not
        form."

        https://lwn.net/Articles/309298/

Johan

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

[Johan Hovold]

On Wed, Jul 29, 2015 at 09:08:42AM +0200, Markus Pargmann wrote:
> On Tue, Jul 28, 2015 at 11:58:42AM +0200, Johan Hovold wrote:
> > On Fri, Jul 17, 2015 at 11:32:50AM +0200, Markus Pargmann 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>
> > > ---
> > >  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 dcac3bcf21dd..0f1d1f5faf5d 100644
> > > --- a/drivers/gpio/gpiolib.c
> > > +++ b/drivers/gpio/gpiolib.c
> > > @@ -2308,14 +2308,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);
> > > +			}
> > 
> > So now we'd no longer just be listing requested gpios, but on a similar
> > format to how requested ones used to be represented.
> 
> Better suggestions on how to display those extra information in debugfs?

Perhaps a new file only listing the line names.

> > Then there's the debugfs as ABI discussion...
> 
> I didn't consider debugfs as ABI as I thought it is just for debugging
> purposes?

Some people seem to have expressed a different position:

	"The fact that something is documented (whether correctly or
        not) has absolutely _zero_ impact on anything at all. What makes
        something an ABI is that it's useful and available. The only way
        something isn't an ABI is by _explicitly_ making sure that it's
        not available even by mistake in a stable form for binary use.

        Example: kernel internal data structures and function calls. We
        make sure that you simply _cannot_ make a binary that works
        across kernel versions. That is the only way for an ABI to not
        form."

        https://lwn.net/Articles/309298/

Johan

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

[Markus Pargmann]

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

On Fri, Jul 31, 2015 at 10:54:07AM +0200, Johan Hovold wrote:
> On Wed, Jul 29, 2015 at 09:08:42AM +0200, Markus Pargmann wrote:
> > On Tue, Jul 28, 2015 at 11:58:42AM +0200, Johan Hovold wrote:
> > > On Fri, Jul 17, 2015 at 11:32:50AM +0200, Markus Pargmann 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>
> > > > ---
> > > >  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 dcac3bcf21dd..0f1d1f5faf5d 100644
> > > > --- a/drivers/gpio/gpiolib.c
> > > > +++ b/drivers/gpio/gpiolib.c
> > > > @@ -2308,14 +2308,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);
> > > > +			}
> > > 
> > > So now we'd no longer just be listing requested gpios, but on a similar
> > > format to how requested ones used to be represented.
> > 
> > Better suggestions on how to display those extra information in debugfs?
> 
> Perhaps a new file only listing the line names.

Yes that's a possibility. On the other side having it all in one place
is nice. As it helps you to debug faster, you can directly see which
line name was requested for what purpose and so on.

> 
> > > Then there's the debugfs as ABI discussion...
> > 
> > I didn't consider debugfs as ABI as I thought it is just for debugging
> > purposes?
> 
> Some people seem to have expressed a different position:
> 
> 	"The fact that something is documented (whether correctly or
>         not) has absolutely _zero_ impact on anything at all. What makes
>         something an ABI is that it's useful and available. The only way
>         something isn't an ABI is by _explicitly_ making sure that it's
>         not available even by mistake in a stable form for binary use.
> 
>         Example: kernel internal data structures and function calls. We
>         make sure that you simply _cannot_ make a binary that works
>         across kernel versions. That is the only way for an ABI to not
>         form."
> 
>         https://lwn.net/Articles/309298/
> 

Oh, I see. But I think that debugfs should help us to debug issues.
If we start to care about breaking userspace tools it is not as helpful
anymore. If I develop some tool against information that are
clearly marked for debugging, I would somehow expect that my tool breaks
at some point. But thats just my opinion.

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 9/9] gpiolib: Add gpio name information to /sys/kernel/debug/gpio

[Markus Pargmann]

On Fri, Jul 31, 2015 at 10:54:07AM +0200, Johan Hovold wrote:
> On Wed, Jul 29, 2015 at 09:08:42AM +0200, Markus Pargmann wrote:
> > On Tue, Jul 28, 2015 at 11:58:42AM +0200, Johan Hovold wrote:
> > > On Fri, Jul 17, 2015 at 11:32:50AM +0200, Markus Pargmann 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>
> > > > ---
> > > >  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 dcac3bcf21dd..0f1d1f5faf5d 100644
> > > > --- a/drivers/gpio/gpiolib.c
> > > > +++ b/drivers/gpio/gpiolib.c
> > > > @@ -2308,14 +2308,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);
> > > > +			}
> > > 
> > > So now we'd no longer just be listing requested gpios, but on a similar
> > > format to how requested ones used to be represented.
> > 
> > Better suggestions on how to display those extra information in debugfs?
> 
> Perhaps a new file only listing the line names.

Yes that's a possibility. On the other side having it all in one place
is nice. As it helps you to debug faster, you can directly see which
line name was requested for what purpose and so on.

> 
> > > Then there's the debugfs as ABI discussion...
> > 
> > I didn't consider debugfs as ABI as I thought it is just for debugging
> > purposes?
> 
> Some people seem to have expressed a different position:
> 
> 	"The fact that something is documented (whether correctly or
>         not) has absolutely _zero_ impact on anything at all. What makes
>         something an ABI is that it's useful and available. The only way
>         something isn't an ABI is by _explicitly_ making sure that it's
>         not available even by mistake in a stable form for binary use.
> 
>         Example: kernel internal data structures and function calls. We
>         make sure that you simply _cannot_ make a binary that works
>         across kernel versions. That is the only way for an ABI to not
>         form."
> 
>         https://lwn.net/Articles/309298/
> 

Oh, I see. But I think that debugfs should help us to debug issues.
If we start to care about breaking userspace tools it is not as helpful
anymore. If I develop some tool against information that are
clearly marked for debugging, I would somehow expect that my tool breaks
at some point. But thats just my opinion.

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 |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150731/8596c7fb/attachment-0001.sig>

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

[Johan Hovold]

On Fri, Jul 31, 2015 at 12:41:01PM +0200, Markus Pargmann wrote:
> On Fri, Jul 31, 2015 at 10:54:07AM +0200, Johan Hovold wrote:
> > On Wed, Jul 29, 2015 at 09:08:42AM +0200, Markus Pargmann wrote:
> > > On Tue, Jul 28, 2015 at 11:58:42AM +0200, Johan Hovold wrote:
> > > > On Fri, Jul 17, 2015 at 11:32:50AM +0200, Markus Pargmann 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>
> > > > > ---
> > > > >  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 dcac3bcf21dd..0f1d1f5faf5d 100644
> > > > > --- a/drivers/gpio/gpiolib.c
> > > > > +++ b/drivers/gpio/gpiolib.c
> > > > > @@ -2308,14 +2308,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);
> > > > > +			}
> > > > 
> > > > So now we'd no longer just be listing requested gpios, but on a similar
> > > > format to how requested ones used to be represented.
> > > 
> > > Better suggestions on how to display those extra information in debugfs?
> > 
> > Perhaps a new file only listing the line names.
> 
> Yes that's a possibility. On the other side having it all in one place
> is nice. As it helps you to debug faster, you can directly see which
> line name was requested for what purpose and so on.

Certainly, but you'd change the meaning of debug/gpio which used to
contain only requested gpios.

Not saying we shouldn't, but yeah, let's were this discussion goes
first.

> > > > Then there's the debugfs as ABI discussion...
> > > 
> > > I didn't consider debugfs as ABI as I thought it is just for debugging
> > > purposes?
> > 
> > Some people seem to have expressed a different position:
> > 
> > 	"The fact that something is documented (whether correctly or
> >         not) has absolutely _zero_ impact on anything at all. What makes
> >         something an ABI is that it's useful and available. The only way
> >         something isn't an ABI is by _explicitly_ making sure that it's
> >         not available even by mistake in a stable form for binary use.
> > 
> >         Example: kernel internal data structures and function calls. We
> >         make sure that you simply _cannot_ make a binary that works
> >         across kernel versions. That is the only way for an ABI to not
> >         form."
> > 
> >         https://lwn.net/Articles/309298/
> > 
> 
> Oh, I see. But I think that debugfs should help us to debug issues.
> If we start to care about breaking userspace tools it is not as helpful
> anymore. If I develop some tool against information that are
> clearly marked for debugging, I would somehow expect that my tool breaks
> at some point. But thats just my opinion.

I tend to agree, and wish debugfs was never to be considered ABI, but
it's Torvald's words above...

Johan

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

[Johan Hovold]

On Fri, Jul 31, 2015 at 12:41:01PM +0200, Markus Pargmann wrote:
> On Fri, Jul 31, 2015 at 10:54:07AM +0200, Johan Hovold wrote:
> > On Wed, Jul 29, 2015 at 09:08:42AM +0200, Markus Pargmann wrote:
> > > On Tue, Jul 28, 2015 at 11:58:42AM +0200, Johan Hovold wrote:
> > > > On Fri, Jul 17, 2015 at 11:32:50AM +0200, Markus Pargmann 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>
> > > > > ---
> > > > >  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 dcac3bcf21dd..0f1d1f5faf5d 100644
> > > > > --- a/drivers/gpio/gpiolib.c
> > > > > +++ b/drivers/gpio/gpiolib.c
> > > > > @@ -2308,14 +2308,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);
> > > > > +			}
> > > > 
> > > > So now we'd no longer just be listing requested gpios, but on a similar
> > > > format to how requested ones used to be represented.
> > > 
> > > Better suggestions on how to display those extra information in debugfs?
> > 
> > Perhaps a new file only listing the line names.
> 
> Yes that's a possibility. On the other side having it all in one place
> is nice. As it helps you to debug faster, you can directly see which
> line name was requested for what purpose and so on.

Certainly, but you'd change the meaning of debug/gpio which used to
contain only requested gpios.

Not saying we shouldn't, but yeah, let's were this discussion goes
first.

> > > > Then there's the debugfs as ABI discussion...
> > > 
> > > I didn't consider debugfs as ABI as I thought it is just for debugging
> > > purposes?
> > 
> > Some people seem to have expressed a different position:
> > 
> > 	"The fact that something is documented (whether correctly or
> >         not) has absolutely _zero_ impact on anything at all. What makes
> >         something an ABI is that it's useful and available. The only way
> >         something isn't an ABI is by _explicitly_ making sure that it's
> >         not available even by mistake in a stable form for binary use.
> > 
> >         Example: kernel internal data structures and function calls. We
> >         make sure that you simply _cannot_ make a binary that works
> >         across kernel versions. That is the only way for an ABI to not
> >         form."
> > 
> >         https://lwn.net/Articles/309298/
> > 
> 
> Oh, I see. But I think that debugfs should help us to debug issues.
> If we start to care about breaking userspace tools it is not as helpful
> anymore. If I develop some tool against information that are
> clearly marked for debugging, I would somehow expect that my tool breaks
> at some point. But thats just my opinion.

I tend to agree, and wish debugfs was never to be considered ABI, but
it's Torvald's words above...

Johan

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

[Lucas Stach]

Am Freitag, den 31.07.2015, 10:54 +0200 schrieb Johan Hovold:
> On Wed, Jul 29, 2015 at 09:08:42AM +0200, Markus Pargmann wrote:
> > On Tue, Jul 28, 2015 at 11:58:42AM +0200, Johan Hovold wrote:
> > > On Fri, Jul 17, 2015 at 11:32:50AM +0200, Markus Pargmann 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>
> > > > ---
> > > >  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 dcac3bcf21dd..0f1d1f5faf5d 100644
> > > > --- a/drivers/gpio/gpiolib.c
> > > > +++ b/drivers/gpio/gpiolib.c
> > > > @@ -2308,14 +2308,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);
> > > > +			}
> > > 
> > > So now we'd no longer just be listing requested gpios, but on a similar
> > > format to how requested ones used to be represented.
> > 
> > Better suggestions on how to display those extra information in debugfs?
> 
> Perhaps a new file only listing the line names.
> 
> > > Then there's the debugfs as ABI discussion...
> > 
> > I didn't consider debugfs as ABI as I thought it is just for debugging
> > purposes?
> 
> Some people seem to have expressed a different position:
> 
> 	"The fact that something is documented (whether correctly or
>         not) has absolutely _zero_ impact on anything at all. What makes
>         something an ABI is that it's useful and available. The only way
>         something isn't an ABI is by _explicitly_ making sure that it's
>         not available even by mistake in a stable form for binary use.
> 
>         Example: kernel internal data structures and function calls. We
>         make sure that you simply _cannot_ make a binary that works
>         across kernel versions. That is the only way for an ABI to not
>         form."
> 
>         https://lwn.net/Articles/309298/

Assuming debugfs is ABI is completely backwards. debugfs makes no ABI
guarantees, that's why stuff that needs a stable ABI like tracing is
actively moving out of debugfs into other places, where ABI stability
can be assumed.

Regards,
Lucas 

-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |

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

[Lucas Stach]

Am Freitag, den 31.07.2015, 10:54 +0200 schrieb Johan Hovold:
> On Wed, Jul 29, 2015 at 09:08:42AM +0200, Markus Pargmann wrote:
> > On Tue, Jul 28, 2015 at 11:58:42AM +0200, Johan Hovold wrote:
> > > On Fri, Jul 17, 2015 at 11:32:50AM +0200, Markus Pargmann 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>
> > > > ---
> > > >  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 dcac3bcf21dd..0f1d1f5faf5d 100644
> > > > --- a/drivers/gpio/gpiolib.c
> > > > +++ b/drivers/gpio/gpiolib.c
> > > > @@ -2308,14 +2308,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);
> > > > +			}
> > > 
> > > So now we'd no longer just be listing requested gpios, but on a similar
> > > format to how requested ones used to be represented.
> > 
> > Better suggestions on how to display those extra information in debugfs?
> 
> Perhaps a new file only listing the line names.
> 
> > > Then there's the debugfs as ABI discussion...
> > 
> > I didn't consider debugfs as ABI as I thought it is just for debugging
> > purposes?
> 
> Some people seem to have expressed a different position:
> 
> 	"The fact that something is documented (whether correctly or
>         not) has absolutely _zero_ impact on anything at all. What makes
>         something an ABI is that it's useful and available. The only way
>         something isn't an ABI is by _explicitly_ making sure that it's
>         not available even by mistake in a stable form for binary use.
> 
>         Example: kernel internal data structures and function calls. We
>         make sure that you simply _cannot_ make a binary that works
>         across kernel versions. That is the only way for an ABI to not
>         form."
> 
>         https://lwn.net/Articles/309298/

Assuming debugfs is ABI is completely backwards. debugfs makes no ABI
guarantees, that's why stuff that needs a stable ABI like tracing is
actively moving out of debugfs into other places, where ABI stability
can be assumed.

Regards,
Lucas 

-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |

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

[Linus Walleij]

On Fri, Jul 17, 2015 at 11:32 AM, Markus Pargmann <mpa@pengutronix.de> wrote:

> 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 '(null)' 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.

I must say I like this series.

Because it makes the GPIO sysfs ABI *less* insane. Especially
I like the patch that makes a distinction between "label" (use)
and "name" (the name of the actual line). That illustrates clearly
that this is thought-through.

However I still have some doubts as it will expand the sysfs ABI,
and we have discussed a char device for GPIO chips as a better
alternative, for example since these can do get/set multiple GPIOs
with a single context switch (ioctl), whereas sysfs is "one value per file"
and would be able to expose some flags better.

Still this is very tempting. Adding some more people to To: to get
some opinions.

Yours,
Linus Walleij

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

[Linus Walleij]

On Fri, Jul 17, 2015 at 11:32 AM, Markus Pargmann <mpa@pengutronix.de> wrote:

> 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 '(null)' 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.

I must say I like this series.

Because it makes the GPIO sysfs ABI *less* insane. Especially
I like the patch that makes a distinction between "label" (use)
and "name" (the name of the actual line). That illustrates clearly
that this is thought-through.

However I still have some doubts as it will expand the sysfs ABI,
and we have discussed a char device for GPIO chips as a better
alternative, for example since these can do get/set multiple GPIOs
with a single context switch (ioctl), whereas sysfs is "one value per file"
and would be able to expose some flags better.

Still this is very tempting. Adding some more people to To: to get
some opinions.

Yours,
Linus Walleij

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

[Alexandre Courbot]

On Sat, Jul 18, 2015 at 5:05 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Fri, Jul 17, 2015 at 11:32 AM, Markus Pargmann <mpa@pengutronix.de> wrote:
>
>> 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 '(null)' 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.
>
> I must say I like this series.
>
> Because it makes the GPIO sysfs ABI *less* insane. Especially
> I like the patch that makes a distinction between "label" (use)
> and "name" (the name of the actual line). That illustrates clearly
> that this is thought-through.
>
> However I still have some doubts as it will expand the sysfs ABI,
> and we have discussed a char device for GPIO chips as a better
> alternative, for example since these can do get/set multiple GPIOs
> with a single context switch (ioctl), whereas sysfs is "one value per file"
> and would be able to expose some flags better.

Even if we introduce a new interface (and I don't see traction towards
this yet), sysfs is just too convenient for 90% of the cases to leave
it borderline-unusable as it is currently. The obligation to use
global numbers that potentially change every boot is getting us
complaints every week or so - this series sounds like a good way to
reduce our inbox volume, and if only for that, I encourage it being
pursued. :)

It will also not get in the way of a new user-space interface, so why
refrain from these extra ~90 lines of code?

Alex.

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

[Alexandre Courbot]

On Sat, Jul 18, 2015 at 5:05 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Fri, Jul 17, 2015 at 11:32 AM, Markus Pargmann <mpa@pengutronix.de> wrote:
>
>> 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 '(null)' 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.
>
> I must say I like this series.
>
> Because it makes the GPIO sysfs ABI *less* insane. Especially
> I like the patch that makes a distinction between "label" (use)
> and "name" (the name of the actual line). That illustrates clearly
> that this is thought-through.
>
> However I still have some doubts as it will expand the sysfs ABI,
> and we have discussed a char device for GPIO chips as a better
> alternative, for example since these can do get/set multiple GPIOs
> with a single context switch (ioctl), whereas sysfs is "one value per file"
> and would be able to expose some flags better.

Even if we introduce a new interface (and I don't see traction towards
this yet), sysfs is just too convenient for 90% of the cases to leave
it borderline-unusable as it is currently. The obligation to use
global numbers that potentially change every boot is getting us
complaints every week or so - this series sounds like a good way to
reduce our inbox volume, and if only for that, I encourage it being
pursued. :)

It will also not get in the way of a new user-space interface, so why
refrain from these extra ~90 lines of code?

Alex.

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

[Uwe Kleine-König]

Hello,

On Tue, Jul 21, 2015 at 06:00:37PM +0900, Alexandre Courbot wrote:
> On Sat, Jul 18, 2015 at 5:05 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> > I must say I like this series.
I think apart from patch 3 which I'd like to have discussed a bit more,
I like it, too. I didn't try, but the series should work fine without
patch 3 such that this patch can be discussed separately and the
remaining 8 patches can go in?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
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

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

[Uwe Kleine-König]

Hello,

On Tue, Jul 21, 2015 at 06:00:37PM +0900, Alexandre Courbot wrote:
> On Sat, Jul 18, 2015 at 5:05 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> > I must say I like this series.
I think apart from patch 3 which I'd like to have discussed a bit more,
I like it, too. I didn't try, but the series should work fine without
patch 3 such that this patch can be discussed separately and the
remaining 8 patches can go in?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

[Markus Pargmann]

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

Hi,

On Tue, Jul 21, 2015 at 11:54:57AM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> On Tue, Jul 21, 2015 at 06:00:37PM +0900, Alexandre Courbot wrote:
> > On Sat, Jul 18, 2015 at 5:05 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> > > I must say I like this series.
> I think apart from patch 3 which I'd like to have discussed a bit more,
> I like it, too. I didn't try, but the series should work fine without
> patch 3 such that this patch can be discussed separately and the
> remaining 8 patches can go in?

This series should work without patch 3, yes. Just remove the hogging
keyword from the gpios defined in the devicetree.
However some test coverage for this series would be good.

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 0/9] gpiolib: Add GPIO name support

[Markus Pargmann]

Hi,

On Tue, Jul 21, 2015 at 11:54:57AM +0200, Uwe Kleine-K?nig wrote:
> Hello,
> 
> On Tue, Jul 21, 2015 at 06:00:37PM +0900, Alexandre Courbot wrote:
> > On Sat, Jul 18, 2015 at 5:05 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> > > I must say I like this series.
> I think apart from patch 3 which I'd like to have discussed a bit more,
> I like it, too. I didn't try, but the series should work fine without
> patch 3 such that this patch can be discussed separately and the
> remaining 8 patches can go in?

This series should work without patch 3, yes. Just remove the hogging
keyword from the gpios defined in the devicetree.
However some test coverage for this series would be good.

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 |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150721/93f5e420/attachment.sig>

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

[Johan Hovold]

On Thu, Jul 30, 2015 at 05:13:37PM -0700, Arun Bharadwaj wrote:

> 2) Some times I get a kernel dump when I try to unexport the gpio or even
> when I try to read the "name" file. This is not very repeatable and seems
> to happen at random. I haven't had the time to look into this in more
> detail yet, but here's the dump FWIW: http://pastebin.com/xq9cpCQE

This is probably because of patch 8/9 erroneously taking the sysfs_lock
in gpio_name_show.

Johan

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

[Johan Hovold]

On Thu, Jul 30, 2015 at 05:13:37PM -0700, Arun Bharadwaj wrote:

> 2) Some times I get a kernel dump when I try to unexport the gpio or even
> when I try to read the "name" file. This is not very repeatable and seems
> to happen at random. I haven't had the time to look into this in more
> detail yet, but here's the dump FWIW: http://pastebin.com/xq9cpCQE

This is probably because of patch 8/9 erroneously taking the sysfs_lock
in gpio_name_show.

Johan

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

[Markus Pargmann]

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

On Fri, Jul 31, 2015 at 11:49:28AM +0200, Johan Hovold wrote:
> On Thu, Jul 30, 2015 at 05:13:37PM -0700, Arun Bharadwaj wrote:
> 
> > 2) Some times I get a kernel dump when I try to unexport the gpio or even
> > when I try to read the "name" file. This is not very repeatable and seems
> > to happen at random. I haven't had the time to look into this in more
> > detail yet, but here's the dump FWIW: http://pastebin.com/xq9cpCQE
> 
> This is probably because of patch 8/9 erroneously taking the sysfs_lock
> in gpio_name_show.

Arun, thank you for testing. I will send a fixed series at the beginning
of next week.

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 0/9] gpiolib: Add GPIO name support

[Markus Pargmann]

On Fri, Jul 31, 2015 at 11:49:28AM +0200, Johan Hovold wrote:
> On Thu, Jul 30, 2015 at 05:13:37PM -0700, Arun Bharadwaj wrote:
> 
> > 2) Some times I get a kernel dump when I try to unexport the gpio or even
> > when I try to read the "name" file. This is not very repeatable and seems
> > to happen at random. I haven't had the time to look into this in more
> > detail yet, but here's the dump FWIW: http://pastebin.com/xq9cpCQE
> 
> This is probably because of patch 8/9 erroneously taking the sysfs_lock
> in gpio_name_show.

Arun, thank you for testing. I will send a fixed series at the beginning
of next week.

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 |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150731/409a0439/attachment.sig>

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

[Johan Hovold]

On Fri, Jul 17, 2015 at 10:05:52PM +0200, Linus Walleij wrote:
> On Fri, Jul 17, 2015 at 11:32 AM, Markus Pargmann <mpa@pengutronix.de> wrote:
> 
> > 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 '(null)' 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.
> 
> I must say I like this series.
> 
> Because it makes the GPIO sysfs ABI *less* insane. Especially
> I like the patch that makes a distinction between "label" (use)
> and "name" (the name of the actual line). That illustrates clearly
> that this is thought-through.
> 
> However I still have some doubts as it will expand the sysfs ABI,
> and we have discussed a char device for GPIO chips as a better
> alternative, for example since these can do get/set multiple GPIOs
> with a single context switch (ioctl), whereas sysfs is "one value per file"
> and would be able to expose some flags better.

I'm also reluctant to band-aiding the sysfs interface with more ABI that
we need to maintain forever. Making it easier to use also reduces the
incentives to come up with a saner interface.

As I mentioned in a comment to patch 6/9, this series introduces an
incompatibility with current DT since it no longer allows two hogs to
use the same name, while the current hog implementation is documented to
fall back to using the (not necessarily unique) node name when a
line-name property isn't specified.

Even though this looks like it could be worked around, it's an example
of the kind of issues we risk running into if we keep on incrementally
patching this interface.

That said, this is a step along the lines that we have discussed in the
past. Adding pin functions (line names) to generic pin nodes in DT makes
sense, but we need to think through the details first. For example:

 - Should we allow duplicate pin functions (line names)? We need to
   consider not just on-chip controllers, but also hot-pluggable
   controllers and device-tree overlays.

 - Should we allow initial configurations to be specified and still
   allow (some) pins to be requested later ("soft" hogging)?

 - Should we specify pin directionality? I've suggested elsewhere that
   adding such limitations (e.g. as a mask) may make sense in cases were
   changing pin direction is known to cause damage.

 - What would a new gpio interface look like?

Johan 

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

[Johan Hovold]

On Fri, Jul 17, 2015 at 10:05:52PM +0200, Linus Walleij wrote:
> On Fri, Jul 17, 2015 at 11:32 AM, Markus Pargmann <mpa@pengutronix.de> wrote:
> 
> > 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 '(null)' 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.
> 
> I must say I like this series.
> 
> Because it makes the GPIO sysfs ABI *less* insane. Especially
> I like the patch that makes a distinction between "label" (use)
> and "name" (the name of the actual line). That illustrates clearly
> that this is thought-through.
> 
> However I still have some doubts as it will expand the sysfs ABI,
> and we have discussed a char device for GPIO chips as a better
> alternative, for example since these can do get/set multiple GPIOs
> with a single context switch (ioctl), whereas sysfs is "one value per file"
> and would be able to expose some flags better.

I'm also reluctant to band-aiding the sysfs interface with more ABI that
we need to maintain forever. Making it easier to use also reduces the
incentives to come up with a saner interface.

As I mentioned in a comment to patch 6/9, this series introduces an
incompatibility with current DT since it no longer allows two hogs to
use the same name, while the current hog implementation is documented to
fall back to using the (not necessarily unique) node name when a
line-name property isn't specified.

Even though this looks like it could be worked around, it's an example
of the kind of issues we risk running into if we keep on incrementally
patching this interface.

That said, this is a step along the lines that we have discussed in the
past. Adding pin functions (line names) to generic pin nodes in DT makes
sense, but we need to think through the details first. For example:

 - Should we allow duplicate pin functions (line names)? We need to
   consider not just on-chip controllers, but also hot-pluggable
   controllers and device-tree overlays.

 - Should we allow initial configurations to be specified and still
   allow (some) pins to be requested later ("soft" hogging)?

 - Should we specify pin directionality? I've suggested elsewhere that
   adding such limitations (e.g. as a mask) may make sense in cases were
   changing pin direction is known to cause damage.

 - What would a new gpio interface look like?

Johan 

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

[Linus Walleij]

On Tue, Jul 28, 2015 at 4:16 PM, Johan Hovold <johan@kernel.org> wrote:

> That said, this is a step along the lines that we have discussed in the
> past. Adding pin functions (line names) to generic pin nodes in DT makes
> sense, but we need to think through the details first. For example:
>
>  - Should we allow duplicate pin functions (line names)? We need to
>    consider not just on-chip controllers, but also hot-pluggable
>    controllers and device-tree overlays.

I guess the typical example is if we have two identical GPIO
expanders. We have this with arch/arm/boot/dts/ste-nomadik-nhk15.dts
for example.

With this scheme, they will simply have to name their lines
uniquely for things to work since /sys/class/gpio is a flat
space, not hierarchic per-gpiochip (as would be preferred,
should this be sane to maintain).

>  - Should we allow initial configurations to be specified and still
>    allow (some) pins to be requested later ("soft" hogging)?

That is a key idea in this patch set.

Maybe we need a "userspace hog" to mark these specifically as
not used by the operating system. (Bringing me back to my
tirade about people doing stupid things in userspace just because
they don't know about all the nice GPIO stuff in the kernel.)

>  - Should we specify pin directionality? I've suggested elsewhere that
>    adding such limitations (e.g. as a mask) may make sense in cases were
>    changing pin direction is known to cause damage.

We have the flag jamming IRQ lines to be input, but input-only
and output-only seems reasonable for hogs in reaction to the
current "input", "output-low" and "output-high" properties.

Could be a separate patch though.

>  - What would a new gpio interface look like?

Yeah :/ people are scared of adding chardevs it seems.

I think they are kind of nice, the biggest problem seems to be
totally confused userspace with udev and mdev and none of
them ever knows about new stuff and people get problems
becuase of old rootfs:es.

Yours,
Linus Walleij

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

[Linus Walleij]

On Tue, Jul 28, 2015 at 4:16 PM, Johan Hovold <johan@kernel.org> wrote:

> That said, this is a step along the lines that we have discussed in the
> past. Adding pin functions (line names) to generic pin nodes in DT makes
> sense, but we need to think through the details first. For example:
>
>  - Should we allow duplicate pin functions (line names)? We need to
>    consider not just on-chip controllers, but also hot-pluggable
>    controllers and device-tree overlays.

I guess the typical example is if we have two identical GPIO
expanders. We have this with arch/arm/boot/dts/ste-nomadik-nhk15.dts
for example.

With this scheme, they will simply have to name their lines
uniquely for things to work since /sys/class/gpio is a flat
space, not hierarchic per-gpiochip (as would be preferred,
should this be sane to maintain).

>  - Should we allow initial configurations to be specified and still
>    allow (some) pins to be requested later ("soft" hogging)?

That is a key idea in this patch set.

Maybe we need a "userspace hog" to mark these specifically as
not used by the operating system. (Bringing me back to my
tirade about people doing stupid things in userspace just because
they don't know about all the nice GPIO stuff in the kernel.)

>  - Should we specify pin directionality? I've suggested elsewhere that
>    adding such limitations (e.g. as a mask) may make sense in cases were
>    changing pin direction is known to cause damage.

We have the flag jamming IRQ lines to be input, but input-only
and output-only seems reasonable for hogs in reaction to the
current "input", "output-low" and "output-high" properties.

Could be a separate patch though.

>  - What would a new gpio interface look like?

Yeah :/ people are scared of adding chardevs it seems.

I think they are kind of nice, the biggest problem seems to be
totally confused userspace with udev and mdev and none of
them ever knows about new stuff and people get problems
becuase of old rootfs:es.

Yours,
Linus Walleij

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

[Johan Hovold]

On Wed, Jul 29, 2015 at 11:23:10AM +0200, Linus Walleij wrote:
> On Tue, Jul 28, 2015 at 4:16 PM, Johan Hovold <johan@kernel.org> wrote:
> 
> > That said, this is a step along the lines that we have discussed in the
> > past. Adding pin functions (line names) to generic pin nodes in DT makes
> > sense, but we need to think through the details first. For example:
> >
> >  - Should we allow duplicate pin functions (line names)? We need to
> >    consider not just on-chip controllers, but also hot-pluggable
> >    controllers and device-tree overlays.
> 
> I guess the typical example is if we have two identical GPIO
> expanders. We have this with arch/arm/boot/dts/ste-nomadik-nhk15.dts
> for example.
> 
> With this scheme, they will simply have to name their lines
> uniquely for things to work since /sys/class/gpio is a flat
> space, not hierarchic per-gpiochip (as would be preferred,
> should this be sane to maintain).

/sys/class/gpio and the pin numbers is a flat space, but the line names
must not necessarily be unique. With a new userspace interface duplicate
line names could be handled.

Consider also DT overlays and hot-pluggable buses (e.g. USB when it gets
a DT representation). A driver for a device on such a bus, be it USB or
Greybus, could even be providing line names through some other means
when registering the controller.

> >  - Should we allow initial configurations to be specified and still
> >    allow (some) pins to be requested later ("soft" hogging)?
> 
> That is a key idea in this patch set.
> 
> Maybe we need a "userspace hog" to mark these specifically as
> not used by the operating system. (Bringing me back to my
> tirade about people doing stupid things in userspace just because
> they don't know about all the nice GPIO stuff in the kernel.)

Should we then refuse such "userspace hogs" from being requested from
the kernel? This can't really be considered hardware description...

> >  - Should we specify pin directionality? I've suggested elsewhere that
> >    adding such limitations (e.g. as a mask) may make sense in cases were
> >    changing pin direction is known to cause damage.
> 
> We have the flag jamming IRQ lines to be input, but input-only
> and output-only seems reasonable for hogs in reaction to the
> current "input", "output-low" and "output-high" properties.
> 
> Could be a separate patch though.

Indeed. I'm just suggesting we not rush into adding more ABI, before
really determining what these interfaces should look like.

> >  - What would a new gpio interface look like?
> 
> Yeah :/ people are scared of adding chardevs it seems.
> 
> I think they are kind of nice, the biggest problem seems to be
> totally confused userspace with udev and mdev and none of
> them ever knows about new stuff and people get problems
> becuase of old rootfs:es.

Really? But that's not our problem, right?

I keep rejecting vendor-specific gpio ioctl-interfaces for usb-serial
devices, while the current sysfs interface really isn't a good
replacement (but we must use standard interfaces of course). It's not
just about the line names.

Johan

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

[Johan Hovold]

On Wed, Jul 29, 2015 at 11:23:10AM +0200, Linus Walleij wrote:
> On Tue, Jul 28, 2015 at 4:16 PM, Johan Hovold <johan@kernel.org> wrote:
> 
> > That said, this is a step along the lines that we have discussed in the
> > past. Adding pin functions (line names) to generic pin nodes in DT makes
> > sense, but we need to think through the details first. For example:
> >
> >  - Should we allow duplicate pin functions (line names)? We need to
> >    consider not just on-chip controllers, but also hot-pluggable
> >    controllers and device-tree overlays.
> 
> I guess the typical example is if we have two identical GPIO
> expanders. We have this with arch/arm/boot/dts/ste-nomadik-nhk15.dts
> for example.
> 
> With this scheme, they will simply have to name their lines
> uniquely for things to work since /sys/class/gpio is a flat
> space, not hierarchic per-gpiochip (as would be preferred,
> should this be sane to maintain).

/sys/class/gpio and the pin numbers is a flat space, but the line names
must not necessarily be unique. With a new userspace interface duplicate
line names could be handled.

Consider also DT overlays and hot-pluggable buses (e.g. USB when it gets
a DT representation). A driver for a device on such a bus, be it USB or
Greybus, could even be providing line names through some other means
when registering the controller.

> >  - Should we allow initial configurations to be specified and still
> >    allow (some) pins to be requested later ("soft" hogging)?
> 
> That is a key idea in this patch set.
> 
> Maybe we need a "userspace hog" to mark these specifically as
> not used by the operating system. (Bringing me back to my
> tirade about people doing stupid things in userspace just because
> they don't know about all the nice GPIO stuff in the kernel.)

Should we then refuse such "userspace hogs" from being requested from
the kernel? This can't really be considered hardware description...

> >  - Should we specify pin directionality? I've suggested elsewhere that
> >    adding such limitations (e.g. as a mask) may make sense in cases were
> >    changing pin direction is known to cause damage.
> 
> We have the flag jamming IRQ lines to be input, but input-only
> and output-only seems reasonable for hogs in reaction to the
> current "input", "output-low" and "output-high" properties.
> 
> Could be a separate patch though.

Indeed. I'm just suggesting we not rush into adding more ABI, before
really determining what these interfaces should look like.

> >  - What would a new gpio interface look like?
> 
> Yeah :/ people are scared of adding chardevs it seems.
> 
> I think they are kind of nice, the biggest problem seems to be
> totally confused userspace with udev and mdev and none of
> them ever knows about new stuff and people get problems
> becuase of old rootfs:es.

Really? But that's not our problem, right?

I keep rejecting vendor-specific gpio ioctl-interfaces for usb-serial
devices, while the current sysfs interface really isn't a good
replacement (but we must use standard interfaces of course). It's not
just about the line names.

Johan