linux-gpio.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] gpio: keep the GPIO line names internal
@ 2015-09-23 23:27 Linus Walleij
  2015-09-24  7:40 ` Markus Pargmann
  2015-10-04 13:37 ` Johan Hovold
  0 siblings, 2 replies; 14+ messages in thread
From: Linus Walleij @ 2015-09-23 23:27 UTC (permalink / raw)
  To: linux-gpio
  Cc: Alexandre Courbot, Linus Walleij, Johan Hovold, Markus Pargmann

This refactors the changes to the GPIO line naming mechanism to
not have so widespread effects, instead we conclude the patch series
by having created a name attribute in the GPIO descriptor, that need
not be globally unique, and it will be initialized from the old
.names array in struct gpio_chip if it exists, then used in the legacy
sysfs code like the array was used previously.

The associated changes to name lines from the device tree are
controversial and need to stand alone from this. Resulting changes:

1. Remove the export and the header for the gpio_name_to_desc() as so
far the only use is inside gpiolib.c. Staticize gpio_name_to_desc()
and move it above the only function using it.

2. Only print a warning if there are two GPIO lines with the same name.
The reason is to preserve current behaviour: before the previous
changes to the naming mechanism this would not reject probing the
driver, instead the error would occur when trying to export the line
in sysfs, so restore this behaviour, but print a friendly warning
if names collide.

Cc: Johan Hovold <johan@kernel.org>
Cc: Markus Pargmann <mpa@pengutronix.de>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/gpiolib.c        | 72 +++++++++++++++++++++----------------------
 include/linux/gpio/consumer.h |  1 -
 2 files changed, 35 insertions(+), 38 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 764845c3a4b2..efe8a1072ed0 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -90,38 +90,6 @@ 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 * const 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,
@@ -250,6 +218,37 @@ static int gpiochip_add_to_list(struct gpio_chip *chip)
 	return err;
 }
 
+/**
+ * Convert a GPIO name to its descriptor
+ */
+static struct gpio_desc *gpio_name_to_desc(const char * const 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;
+}
+
 /*
  * Takes the names from gc->names and checks if they are all unique. If they
  * are, they are assigned to their gpio descriptors.
@@ -268,11 +267,10 @@ static int gpiochip_set_desc_names(struct gpio_chip *gc)
 		struct gpio_desc *gpio;
 
 		gpio = gpio_name_to_desc(gc->names[i]);
-		if (gpio) {
-			dev_err(gc->dev, "Detected name collision for GPIO name '%s'\n",
-				gc->names[i]);
-			return -EEXIST;
-		}
+		if (gpio)
+			dev_warn(gc->dev, "Detected name collision for "
+				 "GPIO name '%s'\n",
+				 gc->names[i]);
 	}
 
 	/* Then add all names to the GPIO descriptors */
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index 366a3fdbdbea..a879e3e62379 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -130,7 +130,6 @@ 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;
-- 
2.4.3


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH] gpio: keep the GPIO line names internal
  2015-09-23 23:27 [PATCH] gpio: keep the GPIO line names internal Linus Walleij
@ 2015-09-24  7:40 ` Markus Pargmann
  2015-09-24 16:47   ` Linus Walleij
  2015-10-04 13:37 ` Johan Hovold
  1 sibling, 1 reply; 14+ messages in thread
From: Markus Pargmann @ 2015-09-24  7:40 UTC (permalink / raw)
  To: Linus Walleij; +Cc: linux-gpio, Alexandre Courbot, Johan Hovold

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

Hi,

On Wed, Sep 23, 2015 at 04:27:33PM -0700, Linus Walleij wrote:
> This refactors the changes to the GPIO line naming mechanism to
> not have so widespread effects, instead we conclude the patch series
> by having created a name attribute in the GPIO descriptor, that need
> not be globally unique, and it will be initialized from the old
> .names array in struct gpio_chip if it exists, then used in the legacy
> sysfs code like the array was used previously.
> 
> The associated changes to name lines from the device tree are
> controversial and need to stand alone from this. Resulting changes:
> 
> 1. Remove the export and the header for the gpio_name_to_desc() as so
> far the only use is inside gpiolib.c. Staticize gpio_name_to_desc()
> and move it above the only function using it.

It is used in gpiolib-of.c as well. So I think it should be in
drivers/gpio/gpiolib.h.

> 
> 2. Only print a warning if there are two GPIO lines with the same name.
> The reason is to preserve current behaviour: before the previous
> changes to the naming mechanism this would not reject probing the
> driver, instead the error would occur when trying to export the line
> in sysfs, so restore this behaviour, but print a friendly warning
> if names collide.

In gpiolib-of.c is the codepath that adds descriptor names from DT. That
uses a warning currently but does not assign the name.

Best Regards,

Markus

> 
> Cc: Johan Hovold <johan@kernel.org>
> Cc: Markus Pargmann <mpa@pengutronix.de>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/gpio/gpiolib.c        | 72 +++++++++++++++++++++----------------------
>  include/linux/gpio/consumer.h |  1 -
>  2 files changed, 35 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 764845c3a4b2..efe8a1072ed0 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -90,38 +90,6 @@ 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 * const 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,
> @@ -250,6 +218,37 @@ static int gpiochip_add_to_list(struct gpio_chip *chip)
>  	return err;
>  }
>  
> +/**
> + * Convert a GPIO name to its descriptor
> + */
> +static struct gpio_desc *gpio_name_to_desc(const char * const 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;
> +}
> +
>  /*
>   * Takes the names from gc->names and checks if they are all unique. If they
>   * are, they are assigned to their gpio descriptors.
> @@ -268,11 +267,10 @@ static int gpiochip_set_desc_names(struct gpio_chip *gc)
>  		struct gpio_desc *gpio;
>  
>  		gpio = gpio_name_to_desc(gc->names[i]);
> -		if (gpio) {
> -			dev_err(gc->dev, "Detected name collision for GPIO name '%s'\n",
> -				gc->names[i]);
> -			return -EEXIST;
> -		}
> +		if (gpio)
> +			dev_warn(gc->dev, "Detected name collision for "
> +				 "GPIO name '%s'\n",
> +				 gc->names[i]);
>  	}
>  
>  	/* Then add all names to the GPIO descriptors */
> diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
> index 366a3fdbdbea..a879e3e62379 100644
> --- a/include/linux/gpio/consumer.h
> +++ b/include/linux/gpio/consumer.h
> @@ -130,7 +130,6 @@ 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;
> -- 
> 2.4.3
> 
> 

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] gpio: keep the GPIO line names internal
  2015-09-24  7:40 ` Markus Pargmann
@ 2015-09-24 16:47   ` Linus Walleij
  0 siblings, 0 replies; 14+ messages in thread
From: Linus Walleij @ 2015-09-24 16:47 UTC (permalink / raw)
  To: Markus Pargmann
  Cc: linux-gpio@vger.kernel.org, Alexandre Courbot, Johan Hovold

On Thu, Sep 24, 2015 at 12:40 AM, Markus Pargmann <mpa@pengutronix.de> wrote:
> On Wed, Sep 23, 2015 at 04:27:33PM -0700, Linus Walleij wrote:
>> This refactors the changes to the GPIO line naming mechanism to
>> not have so widespread effects, instead we conclude the patch series
>> by having created a name attribute in the GPIO descriptor, that need
>> not be globally unique, and it will be initialized from the old
>> .names array in struct gpio_chip if it exists, then used in the legacy
>> sysfs code like the array was used previously.
>>
>> The associated changes to name lines from the device tree are
>> controversial and need to stand alone from this. Resulting changes:
>>
>> 1. Remove the export and the header for the gpio_name_to_desc() as so
>> far the only use is inside gpiolib.c. Staticize gpio_name_to_desc()
>> and move it above the only function using it.
>
> It is used in gpiolib-of.c as well. So I think it should be in
> drivers/gpio/gpiolib.h.

It's not, since I only merged patches 1,2,3,4.

So far, all that has changed is that the descriptor has a name property
and this is used by sysfs instead of the gc->names[] array.

Now the addition of setting names from device tree is controversial
and Johan is opposed to it, so we need to discuss more before
applying the rest of the patches, and therefore this patch puts in
a break after the refactorings (which are anyways very nice).

>> 2. Only print a warning if there are two GPIO lines with the same name.
>> The reason is to preserve current behaviour: before the previous
>> changes to the naming mechanism this would not reject probing the
>> driver, instead the error would occur when trying to export the line
>> in sysfs, so restore this behaviour, but print a friendly warning
>> if names collide.
>
> In gpiolib-of.c is the codepath that adds descriptor names from DT. That
> uses a warning currently but does not assign the name.

In later patches. But I do not apply them now. As per above.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] gpio: keep the GPIO line names internal
  2015-09-23 23:27 [PATCH] gpio: keep the GPIO line names internal Linus Walleij
  2015-09-24  7:40 ` Markus Pargmann
@ 2015-10-04 13:37 ` Johan Hovold
  2015-10-05  8:54   ` Linus Walleij
  2015-10-05  9:47   ` Markus Pargmann
  1 sibling, 2 replies; 14+ messages in thread
From: Johan Hovold @ 2015-10-04 13:37 UTC (permalink / raw)
  To: Linus Walleij
  Cc: linux-gpio, Alexandre Courbot, Johan Hovold, Markus Pargmann

On Wed, Sep 23, 2015 at 04:27:33PM -0700, Linus Walleij wrote:
> This refactors the changes to the GPIO line naming mechanism to
> not have so widespread effects, instead we conclude the patch series
> by having created a name attribute in the GPIO descriptor, that need
> not be globally unique, and it will be initialized from the old
> .names array in struct gpio_chip if it exists, then used in the legacy
> sysfs code like the array was used previously.
> 
> The associated changes to name lines from the device tree are
> controversial and need to stand alone from this. Resulting changes:
> 
> 1. Remove the export and the header for the gpio_name_to_desc() as so
> far the only use is inside gpiolib.c. Staticize gpio_name_to_desc()
> and move it above the only function using it.
> 
> 2. Only print a warning if there are two GPIO lines with the same name.
> The reason is to preserve current behaviour: before the previous
> changes to the naming mechanism this would not reject probing the
> driver, instead the error would occur when trying to export the line
> in sysfs, so restore this behaviour, but print a friendly warning
> if names collide.

This looks good (apart from the checkpatch warning for the warning
message string).

You also need to revert ddd5404007b8 ("gpio-sysfs: Use gpio descriptor
name instead of gpiochip names array") however as this is an ABI change.  
Otherwise pins with a name in DT will now be exported using the gpio name
rather than number as they used to be. [ The current behaviour is
maintained by exporting names from chip->names for hard coded names
only. ]

Thanks,
Johan

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] gpio: keep the GPIO line names internal
  2015-10-04 13:37 ` Johan Hovold
@ 2015-10-05  8:54   ` Linus Walleij
  2015-10-05 10:01     ` Johan Hovold
  2015-10-05  9:47   ` Markus Pargmann
  1 sibling, 1 reply; 14+ messages in thread
From: Linus Walleij @ 2015-10-05  8:54 UTC (permalink / raw)
  To: Johan Hovold
  Cc: linux-gpio@vger.kernel.org, Alexandre Courbot, Markus Pargmann

On Sun, Oct 4, 2015 at 3:37 PM, Johan Hovold <johan@kernel.org> wrote:

> This looks good (apart from the checkpatch warning for the warning
> message string).

Yeah the GPIO maintainer is liberal about that rule...

> You also need to revert ddd5404007b8 ("gpio-sysfs: Use gpio descriptor
> name instead of gpiochip names array") however as this is an ABI change.
> Otherwise pins with a name in DT will now be exported using the gpio name
> rather than number as they used to be. [ The current behaviour is
> maintained by exporting names from chip->names for hard coded names
> only. ]

I think it is ABI-correct: it uses desc->name if that is set, and
currently that is only set from chip->names[] so status quo.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] gpio: keep the GPIO line names internal
  2015-10-04 13:37 ` Johan Hovold
  2015-10-05  8:54   ` Linus Walleij
@ 2015-10-05  9:47   ` Markus Pargmann
  2015-10-05 10:07     ` Johan Hovold
  1 sibling, 1 reply; 14+ messages in thread
From: Markus Pargmann @ 2015-10-05  9:47 UTC (permalink / raw)
  To: Johan Hovold; +Cc: Linus Walleij, linux-gpio, Alexandre Courbot

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

On Sun, Oct 04, 2015 at 03:37:42PM +0200, Johan Hovold wrote:
> On Wed, Sep 23, 2015 at 04:27:33PM -0700, Linus Walleij wrote:
> > This refactors the changes to the GPIO line naming mechanism to
> > not have so widespread effects, instead we conclude the patch series
> > by having created a name attribute in the GPIO descriptor, that need
> > not be globally unique, and it will be initialized from the old
> > .names array in struct gpio_chip if it exists, then used in the legacy
> > sysfs code like the array was used previously.
> > 
> > The associated changes to name lines from the device tree are
> > controversial and need to stand alone from this. Resulting changes:
> > 
> > 1. Remove the export and the header for the gpio_name_to_desc() as so
> > far the only use is inside gpiolib.c. Staticize gpio_name_to_desc()
> > and move it above the only function using it.
> > 
> > 2. Only print a warning if there are two GPIO lines with the same name.
> > The reason is to preserve current behaviour: before the previous
> > changes to the naming mechanism this would not reject probing the
> > driver, instead the error would occur when trying to export the line
> > in sysfs, so restore this behaviour, but print a friendly warning
> > if names collide.
> 
> This looks good (apart from the checkpatch warning for the warning
> message string).
> 
> You also need to revert ddd5404007b8 ("gpio-sysfs: Use gpio descriptor
> name instead of gpiochip names array") however as this is an ABI change.  
> Otherwise pins with a name in DT will now be exported using the gpio name
> rather than number as they used to be. [ The current behaviour is
> maintained by exporting names from chip->names for hard coded names
> only. ]

Even for GPIOs from DT it is not a ABI change. The only GPIOs that have
a GPIO name at the moment are using the GPIO hogging mechanism. But
hogged GPIOs can't be exported to userspace so there is no difference
for these.

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: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] gpio: keep the GPIO line names internal
  2015-10-05  8:54   ` Linus Walleij
@ 2015-10-05 10:01     ` Johan Hovold
  2015-10-05 11:07       ` Linus Walleij
  0 siblings, 1 reply; 14+ messages in thread
From: Johan Hovold @ 2015-10-05 10:01 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Johan Hovold, linux-gpio@vger.kernel.org, Alexandre Courbot,
	Markus Pargmann

On Mon, Oct 05, 2015 at 10:54:52AM +0200, Linus Walleij wrote:
> On Sun, Oct 4, 2015 at 3:37 PM, Johan Hovold <johan@kernel.org> wrote:
> 
> > This looks good (apart from the checkpatch warning for the warning
> > message string).
> 
> Yeah the GPIO maintainer is liberal about that rule...

Heh. I'm sticking to the 80 column limit too, but having grepable error
messages do have its merit.

> > You also need to revert ddd5404007b8 ("gpio-sysfs: Use gpio descriptor
> > name instead of gpiochip names array") however as this is an ABI change.
> > Otherwise pins with a name in DT will now be exported using the gpio name
> > rather than number as they used to be. [ The current behaviour is
> > maintained by exporting names from chip->names for hard coded names
> > only. ]
> 
> I think it is ABI-correct: it uses desc->name if that is set, and
> currently that is only set from chip->names[] so status quo.

Yes, but this work was aiming at generalising the hogs so that they
could later be requested (and exported), right?

This means that a pin used today from userspace, can not be initialised
using this mechanism without breaking the ABI.

My suggestion is to leave gpiolib-sysfs.c as is, and make sure to limit
the name export to legacy board files as before.

Thanks,
Johan

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] gpio: keep the GPIO line names internal
  2015-10-05  9:47   ` Markus Pargmann
@ 2015-10-05 10:07     ` Johan Hovold
  2015-10-05 10:19       ` Markus Pargmann
  0 siblings, 1 reply; 14+ messages in thread
From: Johan Hovold @ 2015-10-05 10:07 UTC (permalink / raw)
  To: Markus Pargmann
  Cc: Johan Hovold, Linus Walleij, linux-gpio, Alexandre Courbot

On Mon, Oct 05, 2015 at 11:47:04AM +0200, Markus Pargmann wrote:
> On Sun, Oct 04, 2015 at 03:37:42PM +0200, Johan Hovold wrote:

> > You also need to revert ddd5404007b8 ("gpio-sysfs: Use gpio descriptor
> > name instead of gpiochip names array") however as this is an ABI change.  
> > Otherwise pins with a name in DT will now be exported using the gpio name
> > rather than number as they used to be. [ The current behaviour is
> > maintained by exporting names from chip->names for hard coded names
> > only. ]
> 
> Even for GPIOs from DT it is not a ABI change. The only GPIOs that have
> a GPIO name at the moment are using the GPIO hogging mechanism. But
> hogged GPIOs can't be exported to userspace so there is no difference
> for these.

Yes, but you're aiming at generalising the hogging mechanism so that
such pins can be requested, and that would break the ABI.

Let's keep the legacy chip->names and named-gpio exports separate from
defining line names in DT for now.

Thanks,
Johan

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] gpio: keep the GPIO line names internal
  2015-10-05 10:07     ` Johan Hovold
@ 2015-10-05 10:19       ` Markus Pargmann
  2015-10-05 11:10         ` Johan Hovold
  0 siblings, 1 reply; 14+ messages in thread
From: Markus Pargmann @ 2015-10-05 10:19 UTC (permalink / raw)
  To: Johan Hovold; +Cc: Linus Walleij, linux-gpio, Alexandre Courbot

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

On Mon, Oct 05, 2015 at 11:07:03AM +0100, Johan Hovold wrote:
> On Mon, Oct 05, 2015 at 11:47:04AM +0200, Markus Pargmann wrote:
> > On Sun, Oct 04, 2015 at 03:37:42PM +0200, Johan Hovold wrote:
> 
> > > You also need to revert ddd5404007b8 ("gpio-sysfs: Use gpio descriptor
> > > name instead of gpiochip names array") however as this is an ABI change.  
> > > Otherwise pins with a name in DT will now be exported using the gpio name
> > > rather than number as they used to be. [ The current behaviour is
> > > maintained by exporting names from chip->names for hard coded names
> > > only. ]
> > 
> > Even for GPIOs from DT it is not a ABI change. The only GPIOs that have
> > a GPIO name at the moment are using the GPIO hogging mechanism. But
> > hogged GPIOs can't be exported to userspace so there is no difference
> > for these.
> 
> Yes, but you're aiming at generalising the hogging mechanism so that
> such pins can be requested, and that would break the ABI.

No, hogged GPIOs can not be requested afterwards.

But you are right that GPIO names should probably not be added to
existing DTs. But that's essentially the same with the currently used
GPIO names array. Changing it would change the names in userspace as
well.

So this would be only useful for all newly created DTs.

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: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] gpio: keep the GPIO line names internal
  2015-10-05 10:01     ` Johan Hovold
@ 2015-10-05 11:07       ` Linus Walleij
  2015-10-06  9:13         ` Markus Pargmann
  0 siblings, 1 reply; 14+ messages in thread
From: Linus Walleij @ 2015-10-05 11:07 UTC (permalink / raw)
  To: Johan Hovold
  Cc: linux-gpio@vger.kernel.org, Alexandre Courbot, Markus Pargmann

On Mon, Oct 5, 2015 at 12:01 PM, Johan Hovold <johan@kernel.org> wrote:

>> I think it is ABI-correct: it uses desc->name if that is set, and
>> currently that is only set from chip->names[] so status quo.
>
> Yes, but this work was aiming at generalising the hogs so that they
> could later be requested (and exported), right?

Yeah something like so... thinking about it we should indeed
takes the names directly from chip->names[] for this, so we
can use the name field in the gpio descriptor for the "real"
chardev interface later.

I'll revert the patch.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] gpio: keep the GPIO line names internal
  2015-10-05 10:19       ` Markus Pargmann
@ 2015-10-05 11:10         ` Johan Hovold
  0 siblings, 0 replies; 14+ messages in thread
From: Johan Hovold @ 2015-10-05 11:10 UTC (permalink / raw)
  To: Markus Pargmann
  Cc: Johan Hovold, Linus Walleij, linux-gpio, Alexandre Courbot

On Mon, Oct 05, 2015 at 12:19:05PM +0200, Markus Pargmann wrote:
> On Mon, Oct 05, 2015 at 11:07:03AM +0100, Johan Hovold wrote:
> > On Mon, Oct 05, 2015 at 11:47:04AM +0200, Markus Pargmann wrote:
> > > On Sun, Oct 04, 2015 at 03:37:42PM +0200, Johan Hovold wrote:
> > 
> > > > You also need to revert ddd5404007b8 ("gpio-sysfs: Use gpio descriptor
> > > > name instead of gpiochip names array") however as this is an ABI change.  
> > > > Otherwise pins with a name in DT will now be exported using the gpio name
> > > > rather than number as they used to be. [ The current behaviour is
> > > > maintained by exporting names from chip->names for hard coded names
> > > > only. ]
> > > 
> > > Even for GPIOs from DT it is not a ABI change. The only GPIOs that have
> > > a GPIO name at the moment are using the GPIO hogging mechanism. But
> > > hogged GPIOs can't be exported to userspace so there is no difference
> > > for these.
> > 
> > Yes, but you're aiming at generalising the hogging mechanism so that
> > such pins can be requested, and that would break the ABI.
> 
> No, hogged GPIOs can not be requested afterwards.

Ok, you call it something different, but you're adding a way to
initialise a gpio during boot that can later be requested from userspace
(e.g. "[PATCH v2 3/3] gpiolib: Add GPIO initialization").

My point is that you cannot use this functionality for pins that are
currently used from userspace today without breaking the ABI.

> But you are right that GPIO names should probably not be added to
> existing DTs. But that's essentially the same with the currently used
> GPIO names array. Changing it would change the names in userspace as
> well.

But fortunately the names array is isolated to board files and some
legacy SOC controller drivers. We need to continue support that, but we
should not make things worse by allowing this to spread to DT.

Johan

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] gpio: keep the GPIO line names internal
  2015-10-05 11:07       ` Linus Walleij
@ 2015-10-06  9:13         ` Markus Pargmann
  2015-10-06 14:09           ` Johan Hovold
  2015-10-16 14:41           ` Linus Walleij
  0 siblings, 2 replies; 14+ messages in thread
From: Markus Pargmann @ 2015-10-06  9:13 UTC (permalink / raw)
  To: Linus Walleij; +Cc: Johan Hovold, linux-gpio@vger.kernel.org, Alexandre Courbot

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

On Mon, Oct 05, 2015 at 01:07:09PM +0200, Linus Walleij wrote:
> On Mon, Oct 5, 2015 at 12:01 PM, Johan Hovold <johan@kernel.org> wrote:
> 
> >> I think it is ABI-correct: it uses desc->name if that is set, and
> >> currently that is only set from chip->names[] so status quo.
> >
> > Yes, but this work was aiming at generalising the hogs so that they
> > could later be requested (and exported), right?
> 
> Yeah something like so... thinking about it we should indeed
> takes the names directly from chip->names[] for this, so we
> can use the name field in the gpio descriptor for the "real"
> chardev interface later.

So what is this "real" chardev interface all about? What are the
requirements and features for such an interface? How is it supposed to
work? Was this already discussed somewhere?

Any chance that you both are at the ELCE?

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: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] gpio: keep the GPIO line names internal
  2015-10-06  9:13         ` Markus Pargmann
@ 2015-10-06 14:09           ` Johan Hovold
  2015-10-16 14:41           ` Linus Walleij
  1 sibling, 0 replies; 14+ messages in thread
From: Johan Hovold @ 2015-10-06 14:09 UTC (permalink / raw)
  To: Markus Pargmann
  Cc: Linus Walleij, Johan Hovold, linux-gpio@vger.kernel.org,
	Alexandre Courbot

On Tue, Oct 06, 2015 at 11:13:16AM +0200, Markus Pargmann wrote:
> On Mon, Oct 05, 2015 at 01:07:09PM +0200, Linus Walleij wrote:
> > On Mon, Oct 5, 2015 at 12:01 PM, Johan Hovold <johan@kernel.org> wrote:
> > 
> > >> I think it is ABI-correct: it uses desc->name if that is set, and
> > >> currently that is only set from chip->names[] so status quo.
> > >
> > > Yes, but this work was aiming at generalising the hogs so that they
> > > could later be requested (and exported), right?
> > 
> > Yeah something like so... thinking about it we should indeed
> > takes the names directly from chip->names[] for this, so we
> > can use the name field in the gpio descriptor for the "real"
> > chardev interface later.
> 
> So what is this "real" chardev interface all about? What are the
> requirements and features for such an interface? How is it supposed to
> work? Was this already discussed somewhere?

It has been discussed in various threads, often when someone proposes to
tweak the current sysfs interface to cater to their specific needs.

Several people have expressed ideas, but no more formal proposal for a
new userspace interface has been put forward yet. Since this will also
become ABI we need to get it right (this time).

> Any chance that you both are at the ELCE?

I'm here, but not Linus. I'll see if I can spot you.

Johan

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] gpio: keep the GPIO line names internal
  2015-10-06  9:13         ` Markus Pargmann
  2015-10-06 14:09           ` Johan Hovold
@ 2015-10-16 14:41           ` Linus Walleij
  1 sibling, 0 replies; 14+ messages in thread
From: Linus Walleij @ 2015-10-16 14:41 UTC (permalink / raw)
  To: Markus Pargmann
  Cc: Johan Hovold, linux-gpio@vger.kernel.org, Alexandre Courbot

On Tue, Oct 6, 2015 at 11:13 AM, Markus Pargmann <mpa@pengutronix.de> wrote:


> So what is this "real" chardev interface all about? What are the
> requirements and features for such an interface? How is it supposed to
> work? Was this already discussed somewhere?

Comes up all the time. Obviously noone find the time or focus
to do this, so I will make a try now to get the ball rolling. Expect
a rough chardev patch in a week or so.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2015-10-16 14:41 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-09-23 23:27 [PATCH] gpio: keep the GPIO line names internal Linus Walleij
2015-09-24  7:40 ` Markus Pargmann
2015-09-24 16:47   ` Linus Walleij
2015-10-04 13:37 ` Johan Hovold
2015-10-05  8:54   ` Linus Walleij
2015-10-05 10:01     ` Johan Hovold
2015-10-05 11:07       ` Linus Walleij
2015-10-06  9:13         ` Markus Pargmann
2015-10-06 14:09           ` Johan Hovold
2015-10-16 14:41           ` Linus Walleij
2015-10-05  9:47   ` Markus Pargmann
2015-10-05 10:07     ` Johan Hovold
2015-10-05 10:19       ` Markus Pargmann
2015-10-05 11:10         ` Johan Hovold

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).