[RFC PATCH] gpiolib: Provide and export gpiod_export_name

[RFC PATCH] gpiolib: Provide and export gpiod_export_name

[Guenter Roeck]

gpiod_export_name is similar to gpiod_export, but lets the user
determine the name used to export a gpio pin.

Currently, the pin name is determined by the chip driver with
the 'names' array in the gpio_chip data structure, or it is set
to gpioX, where X is the pin number, if no name is provided by
the chip driver.

It is, however, desirable to be able to provide the pin name when
exporting the pin, for example from platform code. In other words,
it would be useful to move the naming decision from the pin provider
to the pin consumer. The gpio-pca953x driver provides this capability
as part of its platform data. Other drivers could be enhanced in a
similar way; however, this is not always possible or easy to accomplish.
For example, mfd client drivers such as gpio-ich already use platform
data to pass information from the mfd master driver to the client driver.
Overloading this platform data to also provide an array of gpio pin names
would be a challenge if not impossible.

The alternative to use gpiod_export_link is also not always desirable,
since it only creates a named link to a different directory, meaning
the named gpio pin is not available in /sys/class/gpio but only
in some platform specific directory and thus not as generic as possible
and/or useful.

A specific example for a use case is a gpio pin which reports AC power
loss to user space. Depending on the platform and platform variant,
the pin can be provided by various gpio chip drivers and pin numbers.
It would be very desirable to have a well defined location such as
/sys/class/gpio/ac_power_loss for this pin, so user space knows where
to find the attribute without knowledge of the underlying platform
variant or oher hardware details.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
Applies to tip of linux-gpio/for-next.

 Documentation/gpio/sysfs.txt  | 12 ++++++++----
 drivers/gpio/gpiolib-sysfs.c  | 23 ++++++++++++++++-------
 include/linux/gpio/consumer.h |  9 +++++++++
 3 files changed, 33 insertions(+), 11 deletions(-)

diff --git a/Documentation/gpio/sysfs.txt b/Documentation/gpio/sysfs.txt
index c2c3a97..8e301b2 100644
--- a/Documentation/gpio/sysfs.txt
+++ b/Documentation/gpio/sysfs.txt
@@ -125,7 +125,11 @@ requested using gpio_request():
 	/* export the GPIO to userspace */
 	int gpiod_export(struct gpio_desc *desc, bool direction_may_change);
 
-	/* reverse gpio_export() */
+	/* export named GPIO to userspace */
+	int gpiod_export_name(struct gpio_desc *desc, const char *ioname,
+			bool direction_may_change);
+
+	/* reverse gpio_export() / gpiod_export_name() */
 	void gpiod_unexport(struct gpio_desc *desc);
 
 	/* create a sysfs link to an exported GPIO node */
@@ -136,9 +140,9 @@ requested using gpio_request():
 	int gpiod_sysfs_set_active_low(struct gpio_desc *desc, int value);
 
 After a kernel driver requests a GPIO, it may only be made available in
-the sysfs interface by gpiod_export(). The driver can control whether the
-signal direction may change. This helps drivers prevent userspace code
-from accidentally clobbering important system state.
+the sysfs interface by gpiod_export() or gpiod_export_name(). The driver
+can control whether the signal direction may change. This helps drivers
+prevent userspace code from accidentally clobbering important system state.
 
 This explicit exporting can help with debugging (by making some kinds
 of experiments easier), or can provide an always-there interface that's
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index be45a92..7d36230 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -523,13 +523,12 @@ static struct class gpio_class = {
  *
  * Returns zero on success, else an error.
  */
-int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
+int gpiod_export_name(struct gpio_desc *desc, const char *ioname,
+		      bool direction_may_change)
 {
 	unsigned long		flags;
 	int			status;
-	const char		*ioname = NULL;
 	struct device		*dev;
-	int			offset;
 
 	/* can't export until sysfs is available ... */
 	if (!gpio_class.p) {
@@ -560,10 +559,6 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
 		direction_may_change = false;
 	spin_unlock_irqrestore(&gpio_lock, flags);
 
-	offset = gpio_chip_hwgpio(desc);
-	if (desc->chip->names && desc->chip->names[offset])
-		ioname = desc->chip->names[offset];
-
 	dev = device_create(&gpio_class, desc->chip->dev, MKDEV(0, 0),
 			    desc, ioname ? ioname : "gpio%u",
 			    desc_to_gpio(desc));
@@ -600,6 +595,20 @@ fail_unlock:
 	gpiod_dbg(desc, "%s: status %d\n", __func__, status);
 	return status;
 }
+EXPORT_SYMBOL_GPL(gpiod_export_name);
+
+int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
+{
+	const char *ioname = NULL;
+
+	if (desc) {
+		int offset = gpio_chip_hwgpio(desc);
+
+		if (desc->chip->names && desc->chip->names[offset])
+			ioname = desc->chip->names[offset];
+	}
+	return gpiod_export_name(desc, ioname, direction_may_change);
+}
 EXPORT_SYMBOL_GPL(gpiod_export);
 
 static int match_export(struct device *dev, const void *data)
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index 05e53cc..986da3e 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -260,6 +260,8 @@ static inline int desc_to_gpio(const struct gpio_desc *desc)
 #if IS_ENABLED(CONFIG_GPIOLIB) && IS_ENABLED(CONFIG_GPIO_SYSFS)
 
 int gpiod_export(struct gpio_desc *desc, bool direction_may_change);
+int gpiod_export_name(struct gpio_desc *desc, const char *ioname,
+		      bool direction_may_change);
 int gpiod_export_link(struct device *dev, const char *name,
 		      struct gpio_desc *desc);
 int gpiod_sysfs_set_active_low(struct gpio_desc *desc, int value);
@@ -273,6 +275,13 @@ static inline int gpiod_export(struct gpio_desc *desc,
 	return -ENOSYS;
 }
 
+static inline int gpiod_export_name(struct gpio_desc *desc,
+				    const char *ioname,
+				    bool direction_may_change)
+{
+	return -ENOSYS;
+}
+
 static inline int gpiod_export_link(struct device *dev, const char *name,
 				    struct gpio_desc *desc)
 {
-- 
1.9.1

Re: [RFC PATCH] gpiolib: Provide and export gpiod_export_name

[Alexandre Courbot]

On Thu, Jul 24, 2014 at 3:12 AM, Guenter Roeck <linux@roeck-us.net> wrote:
> gpiod_export_name is similar to gpiod_export, but lets the user
> determine the name used to export a gpio pin.
>
> Currently, the pin name is determined by the chip driver with
> the 'names' array in the gpio_chip data structure, or it is set
> to gpioX, where X is the pin number, if no name is provided by
> the chip driver.

Oh, my. I did not even know about this 'names' array. This is pretty
bad - a GPIO provider should not decide what its GPIOs are used for.

Luckily for you, this creates a precedent that makes this patch more
acceptable, in that it is not making the situation worse. Even though
I consider both solutions to be bad, I actually prefer your
gpiod_export_name() function to that 'names' array in gpio_chip...

>
> It is, however, desirable to be able to provide the pin name when
> exporting the pin, for example from platform code. In other words,
> it would be useful to move the naming decision from the pin provider
> to the pin consumer. The gpio-pca953x driver provides this capability
> as part of its platform data. Other drivers could be enhanced in a
> similar way; however, this is not always possible or easy to accomplish.
> For example, mfd client drivers such as gpio-ich already use platform
> data to pass information from the mfd master driver to the client driver.
> Overloading this platform data to also provide an array of gpio pin names
> would be a challenge if not impossible.
>
> The alternative to use gpiod_export_link is also not always desirable,
> since it only creates a named link to a different directory, meaning
> the named gpio pin is not available in /sys/class/gpio but only
> in some platform specific directory and thus not as generic as possible
> and/or useful.
>
> A specific example for a use case is a gpio pin which reports AC power
> loss to user space. Depending on the platform and platform variant,
> the pin can be provided by various gpio chip drivers and pin numbers.
> It would be very desirable to have a well defined location such as
> /sys/class/gpio/ac_power_loss for this pin, so user space knows where
> to find the attribute without knowledge of the underlying platform
> variant or oher hardware details.

As I explained on the other thread, I still encourage you to have
these GPIOs under device nodes that give a hint of who is provided the
GPIO (effectively exporting the (dev, function) tuple to user-space)
instead of having them popping out under /sys/class/gpio where nobody
knows where they come from and name collisions are much more likely.

Your message sounds like you have no choice but have the named GPIO
link under the gpiochip's device node, but this is not the case -
gpio_export_link() let's you specify the device under which the link
should appear. Make that device be your "scu" device and you can have
a consistent sysfs path to access your GPIOs.

Allowing GPIOs to pop up in the same directory with an arbitrary name
is just a recipe for a mess. But that's a recipe that is already
allowed thanks to that 'names' array. So I'm really confused about
what to do with this patch. If you can do with gpio_export_link() (and
I have not seen evidence of the contrary), please go that way instead.

>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> Applies to tip of linux-gpio/for-next.
>
>  Documentation/gpio/sysfs.txt  | 12 ++++++++----
>  drivers/gpio/gpiolib-sysfs.c  | 23 ++++++++++++++++-------
>  include/linux/gpio/consumer.h |  9 +++++++++
>  3 files changed, 33 insertions(+), 11 deletions(-)
>
> diff --git a/Documentation/gpio/sysfs.txt b/Documentation/gpio/sysfs.txt
> index c2c3a97..8e301b2 100644
> --- a/Documentation/gpio/sysfs.txt
> +++ b/Documentation/gpio/sysfs.txt
> @@ -125,7 +125,11 @@ requested using gpio_request():
>         /* export the GPIO to userspace */
>         int gpiod_export(struct gpio_desc *desc, bool direction_may_change);
>
> -       /* reverse gpio_export() */
> +       /* export named GPIO to userspace */
> +       int gpiod_export_name(struct gpio_desc *desc, const char *ioname,
> +                       bool direction_may_change);
> +
> +       /* reverse gpio_export() / gpiod_export_name() */
>         void gpiod_unexport(struct gpio_desc *desc);
>
>         /* create a sysfs link to an exported GPIO node */
> @@ -136,9 +140,9 @@ requested using gpio_request():
>         int gpiod_sysfs_set_active_low(struct gpio_desc *desc, int value);
>
>  After a kernel driver requests a GPIO, it may only be made available in
> -the sysfs interface by gpiod_export(). The driver can control whether the
> -signal direction may change. This helps drivers prevent userspace code
> -from accidentally clobbering important system state.
> +the sysfs interface by gpiod_export() or gpiod_export_name(). The driver
> +can control whether the signal direction may change. This helps drivers
> +prevent userspace code from accidentally clobbering important system state.
>
>  This explicit exporting can help with debugging (by making some kinds
>  of experiments easier), or can provide an always-there interface that's
> diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
> index be45a92..7d36230 100644
> --- a/drivers/gpio/gpiolib-sysfs.c
> +++ b/drivers/gpio/gpiolib-sysfs.c
> @@ -523,13 +523,12 @@ static struct class gpio_class = {
>   *
>   * Returns zero on success, else an error.
>   */
> -int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
> +int gpiod_export_name(struct gpio_desc *desc, const char *ioname,
> +                     bool direction_may_change)
>  {
>         unsigned long           flags;
>         int                     status;
> -       const char              *ioname = NULL;
>         struct device           *dev;
> -       int                     offset;
>
>         /* can't export until sysfs is available ... */
>         if (!gpio_class.p) {
> @@ -560,10 +559,6 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
>                 direction_may_change = false;
>         spin_unlock_irqrestore(&gpio_lock, flags);
>
> -       offset = gpio_chip_hwgpio(desc);
> -       if (desc->chip->names && desc->chip->names[offset])
> -               ioname = desc->chip->names[offset];
> -
>         dev = device_create(&gpio_class, desc->chip->dev, MKDEV(0, 0),
>                             desc, ioname ? ioname : "gpio%u",
>                             desc_to_gpio(desc));
> @@ -600,6 +595,20 @@ fail_unlock:
>         gpiod_dbg(desc, "%s: status %d\n", __func__, status);
>         return status;
>  }
> +EXPORT_SYMBOL_GPL(gpiod_export_name);
> +
> +int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
> +{
> +       const char *ioname = NULL;
> +
> +       if (desc) {
> +               int offset = gpio_chip_hwgpio(desc);
> +
> +               if (desc->chip->names && desc->chip->names[offset])
> +                       ioname = desc->chip->names[offset];

I'd add a

else
    ioname = "gpio%u";

so all the name-chosing code is grouped in the same place. Then you
can remove that same check from gpiod_export_name().

> +       }
> +       return gpiod_export_name(desc, ioname, direction_may_change);
> +}
>  EXPORT_SYMBOL_GPL(gpiod_export);
>
>  static int match_export(struct device *dev, const void *data)
> diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
> index 05e53cc..986da3e 100644
> --- a/include/linux/gpio/consumer.h
> +++ b/include/linux/gpio/consumer.h
> @@ -260,6 +260,8 @@ static inline int desc_to_gpio(const struct gpio_desc *desc)
>  #if IS_ENABLED(CONFIG_GPIOLIB) && IS_ENABLED(CONFIG_GPIO_SYSFS)
>
>  int gpiod_export(struct gpio_desc *desc, bool direction_may_change);
> +int gpiod_export_name(struct gpio_desc *desc, const char *ioname,
> +                     bool direction_may_change);
>  int gpiod_export_link(struct device *dev, const char *name,
>                       struct gpio_desc *desc);
>  int gpiod_sysfs_set_active_low(struct gpio_desc *desc, int value);
> @@ -273,6 +275,13 @@ static inline int gpiod_export(struct gpio_desc *desc,
>         return -ENOSYS;
>  }
>
> +static inline int gpiod_export_name(struct gpio_desc *desc,
> +                                   const char *ioname,
> +                                   bool direction_may_change)
> +{
> +       return -ENOSYS;
> +}
> +
>  static inline int gpiod_export_link(struct device *dev, const char *name,
>                                     struct gpio_desc *desc)
>  {
> --
> 1.9.1
>

Re: [RFC PATCH] gpiolib: Provide and export gpiod_export_name

[Jiří Prchal]

Hi,
just to append to this: is in plan some kind of exporting named GPIOs from device tree to /sys/* or /dev/*? It would be 
very useful not only for me. Because I thing what GPIO is used for what is hardware design dependent same as for example 
what SPI chips are used. And SPI chips are in DT.

Dne 24.7.2014 v 07:44 Alexandre Courbot napsal(a):
> On Thu, Jul 24, 2014 at 3:12 AM, Guenter Roeck <linux@roeck-us.net> wrote:
>> gpiod_export_name is similar to gpiod_export, but lets the user
>> determine the name used to export a gpio pin.
>>
>> Currently, the pin name is determined by the chip driver with
>> the 'names' array in the gpio_chip data structure, or it is set
>> to gpioX, where X is the pin number, if no name is provided by
>> the chip driver.
>
> Oh, my. I did not even know about this 'names' array. This is pretty
> bad - a GPIO provider should not decide what its GPIOs are used for.
>
> Luckily for you, this creates a precedent that makes this patch more
> acceptable, in that it is not making the situation worse. Even though
> I consider both solutions to be bad, I actually prefer your
> gpiod_export_name() function to that 'names' array in gpio_chip...
>
>>
>> It is, however, desirable to be able to provide the pin name when
>> exporting the pin, for example from platform code. In other words,
>> it would be useful to move the naming decision from the pin provider
>> to the pin consumer. The gpio-pca953x driver provides this capability
>> as part of its platform data. Other drivers could be enhanced in a
>> similar way; however, this is not always possible or easy to accomplish.
>> For example, mfd client drivers such as gpio-ich already use platform
>> data to pass information from the mfd master driver to the client driver.
>> Overloading this platform data to also provide an array of gpio pin names
>> would be a challenge if not impossible.
>>
>> The alternative to use gpiod_export_link is also not always desirable,
>> since it only creates a named link to a different directory, meaning
>> the named gpio pin is not available in /sys/class/gpio but only
>> in some platform specific directory and thus not as generic as possible
>> and/or useful.
>>
>> A specific example for a use case is a gpio pin which reports AC power
>> loss to user space. Depending on the platform and platform variant,
>> the pin can be provided by various gpio chip drivers and pin numbers.
>> It would be very desirable to have a well defined location such as
>> /sys/class/gpio/ac_power_loss for this pin, so user space knows where
>> to find the attribute without knowledge of the underlying platform
>> variant or oher hardware details.
>
> As I explained on the other thread, I still encourage you to have
> these GPIOs under device nodes that give a hint of who is provided the
> GPIO (effectively exporting the (dev, function) tuple to user-space)
> instead of having them popping out under /sys/class/gpio where nobody
> knows where they come from and name collisions are much more likely.
>
> Your message sounds like you have no choice but have the named GPIO
> link under the gpiochip's device node, but this is not the case -
> gpio_export_link() let's you specify the device under which the link
> should appear. Make that device be your "scu" device and you can have
> a consistent sysfs path to access your GPIOs.
>
> Allowing GPIOs to pop up in the same directory with an arbitrary name
> is just a recipe for a mess. But that's a recipe that is already
> allowed thanks to that 'names' array. So I'm really confused about
> what to do with this patch. If you can do with gpio_export_link() (and
> I have not seen evidence of the contrary), please go that way instead.
>
>>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>> Applies to tip of linux-gpio/for-next.
>>
>>   Documentation/gpio/sysfs.txt  | 12 ++++++++----
>>   drivers/gpio/gpiolib-sysfs.c  | 23 ++++++++++++++++-------
>>   include/linux/gpio/consumer.h |  9 +++++++++
>>   3 files changed, 33 insertions(+), 11 deletions(-)
>>
>> diff --git a/Documentation/gpio/sysfs.txt b/Documentation/gpio/sysfs.txt
>> index c2c3a97..8e301b2 100644
>> --- a/Documentation/gpio/sysfs.txt
>> +++ b/Documentation/gpio/sysfs.txt
>> @@ -125,7 +125,11 @@ requested using gpio_request():
>>          /* export the GPIO to userspace */
>>          int gpiod_export(struct gpio_desc *desc, bool direction_may_change);
>>
>> -       /* reverse gpio_export() */
>> +       /* export named GPIO to userspace */
>> +       int gpiod_export_name(struct gpio_desc *desc, const char *ioname,
>> +                       bool direction_may_change);
>> +
>> +       /* reverse gpio_export() / gpiod_export_name() */
>>          void gpiod_unexport(struct gpio_desc *desc);
>>
>>          /* create a sysfs link to an exported GPIO node */
>> @@ -136,9 +140,9 @@ requested using gpio_request():
>>          int gpiod_sysfs_set_active_low(struct gpio_desc *desc, int value);
>>
>>   After a kernel driver requests a GPIO, it may only be made available in
>> -the sysfs interface by gpiod_export(). The driver can control whether the
>> -signal direction may change. This helps drivers prevent userspace code
>> -from accidentally clobbering important system state.
>> +the sysfs interface by gpiod_export() or gpiod_export_name(). The driver
>> +can control whether the signal direction may change. This helps drivers
>> +prevent userspace code from accidentally clobbering important system state.
>>
>>   This explicit exporting can help with debugging (by making some kinds
>>   of experiments easier), or can provide an always-there interface that's
>> diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
>> index be45a92..7d36230 100644
>> --- a/drivers/gpio/gpiolib-sysfs.c
>> +++ b/drivers/gpio/gpiolib-sysfs.c
>> @@ -523,13 +523,12 @@ static struct class gpio_class = {
>>    *
>>    * Returns zero on success, else an error.
>>    */
>> -int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
>> +int gpiod_export_name(struct gpio_desc *desc, const char *ioname,
>> +                     bool direction_may_change)
>>   {
>>          unsigned long           flags;
>>          int                     status;
>> -       const char              *ioname = NULL;
>>          struct device           *dev;
>> -       int                     offset;
>>
>>          /* can't export until sysfs is available ... */
>>          if (!gpio_class.p) {
>> @@ -560,10 +559,6 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
>>                  direction_may_change = false;
>>          spin_unlock_irqrestore(&gpio_lock, flags);
>>
>> -       offset = gpio_chip_hwgpio(desc);
>> -       if (desc->chip->names && desc->chip->names[offset])
>> -               ioname = desc->chip->names[offset];
>> -
>>          dev = device_create(&gpio_class, desc->chip->dev, MKDEV(0, 0),
>>                              desc, ioname ? ioname : "gpio%u",
>>                              desc_to_gpio(desc));
>> @@ -600,6 +595,20 @@ fail_unlock:
>>          gpiod_dbg(desc, "%s: status %d\n", __func__, status);
>>          return status;
>>   }
>> +EXPORT_SYMBOL_GPL(gpiod_export_name);
>> +
>> +int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
>> +{
>> +       const char *ioname = NULL;
>> +
>> +       if (desc) {
>> +               int offset = gpio_chip_hwgpio(desc);
>> +
>> +               if (desc->chip->names && desc->chip->names[offset])
>> +                       ioname = desc->chip->names[offset];
>
> I'd add a
>
> else
>      ioname = "gpio%u";
>
> so all the name-chosing code is grouped in the same place. Then you
> can remove that same check from gpiod_export_name().
>
>> +       }
>> +       return gpiod_export_name(desc, ioname, direction_may_change);
>> +}
>>   EXPORT_SYMBOL_GPL(gpiod_export);
>>
>>   static int match_export(struct device *dev, const void *data)
>> diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
>> index 05e53cc..986da3e 100644
>> --- a/include/linux/gpio/consumer.h
>> +++ b/include/linux/gpio/consumer.h
>> @@ -260,6 +260,8 @@ static inline int desc_to_gpio(const struct gpio_desc *desc)
>>   #if IS_ENABLED(CONFIG_GPIOLIB) && IS_ENABLED(CONFIG_GPIO_SYSFS)
>>
>>   int gpiod_export(struct gpio_desc *desc, bool direction_may_change);
>> +int gpiod_export_name(struct gpio_desc *desc, const char *ioname,
>> +                     bool direction_may_change);
>>   int gpiod_export_link(struct device *dev, const char *name,
>>                        struct gpio_desc *desc);
>>   int gpiod_sysfs_set_active_low(struct gpio_desc *desc, int value);
>> @@ -273,6 +275,13 @@ static inline int gpiod_export(struct gpio_desc *desc,
>>          return -ENOSYS;
>>   }
>>
>> +static inline int gpiod_export_name(struct gpio_desc *desc,
>> +                                   const char *ioname,
>> +                                   bool direction_may_change)
>> +{
>> +       return -ENOSYS;
>> +}
>> +
>>   static inline int gpiod_export_link(struct device *dev, const char *name,
>>                                      struct gpio_desc *desc)
>>   {
>> --
>> 1.9.1
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [RFC PATCH] gpiolib: Provide and export gpiod_export_name

[Guenter Roeck]

On 07/23/2014 10:44 PM, Alexandre Courbot wrote:
> On Thu, Jul 24, 2014 at 3:12 AM, Guenter Roeck <linux@roeck-us.net> wrote:
>> gpiod_export_name is similar to gpiod_export, but lets the user
>> determine the name used to export a gpio pin.
>>
>> Currently, the pin name is determined by the chip driver with
>> the 'names' array in the gpio_chip data structure, or it is set
>> to gpioX, where X is the pin number, if no name is provided by
>> the chip driver.
>
> Oh, my. I did not even know about this 'names' array. This is pretty
> bad - a GPIO provider should not decide what its GPIOs are used for.
>
> Luckily for you, this creates a precedent that makes this patch more
> acceptable, in that it is not making the situation worse. Even though
> I consider both solutions to be bad, I actually prefer your
> gpiod_export_name() function to that 'names' array in gpio_chip...
>
>>
>> It is, however, desirable to be able to provide the pin name when
>> exporting the pin, for example from platform code. In other words,
>> it would be useful to move the naming decision from the pin provider
>> to the pin consumer. The gpio-pca953x driver provides this capability
>> as part of its platform data. Other drivers could be enhanced in a
>> similar way; however, this is not always possible or easy to accomplish.
>> For example, mfd client drivers such as gpio-ich already use platform
>> data to pass information from the mfd master driver to the client driver.
>> Overloading this platform data to also provide an array of gpio pin names
>> would be a challenge if not impossible.
>>
>> The alternative to use gpiod_export_link is also not always desirable,
>> since it only creates a named link to a different directory, meaning
>> the named gpio pin is not available in /sys/class/gpio but only
>> in some platform specific directory and thus not as generic as possible
>> and/or useful.
>>
>> A specific example for a use case is a gpio pin which reports AC power
>> loss to user space. Depending on the platform and platform variant,
>> the pin can be provided by various gpio chip drivers and pin numbers.
>> It would be very desirable to have a well defined location such as
>> /sys/class/gpio/ac_power_loss for this pin, so user space knows where
>> to find the attribute without knowledge of the underlying platform
>> variant or oher hardware details.
>
> As I explained on the other thread, I still encourage you to have
> these GPIOs under device nodes that give a hint of who is provided the
> GPIO (effectively exporting the (dev, function) tuple to user-space)
> instead of having them popping out under /sys/class/gpio where nobody
> knows where they come from and name collisions are much more likely.
>
> Your message sounds like you have no choice but have the named GPIO
> link under the gpiochip's device node, but this is not the case -
> gpio_export_link() let's you specify the device under which the link
> should appear. Make that device be your "scu" device and you can have
> a consistent sysfs path to access your GPIOs.
>
Yes, I understand, but that is not acceptable for the users - see below.

> Allowing GPIOs to pop up in the same directory with an arbitrary name
> is just a recipe for a mess. But that's a recipe that is already
> allowed thanks to that 'names' array. So I'm really confused about
> what to do with this patch. If you can do with gpio_export_link() (and
> I have not seen evidence of the contrary), please go that way instead.
>

I personally don't think it is that much of a mess. Sure, once has to be
careful when selecting names, but I don't see a problem with that.

I have two users for this. Interestingly the problem is pretty
much the same, though the applications are completely different.

One is the company using the scu.c file. They are currently using the
pca953x driver approach (using the names array), but they also have
a new version of their product which also uses gpio pins from gpio-ich.
For consistency, they want all gpio pins in the same directory, meaning
/sys/class/gpio.

The currently implemented solution is to have a weak pointer to the names
array in gpio-ch.c and override it with a pointer from scu.c.

/* SCU specific gpio pin names. Only works if module is built into kernel. */
static const char * const scu_ichx_gpiolib_names[128] = {
         [0] = "switch_interrupt",       /* GPI0 */
         [3] = "ac_loss_detect",         /* GPI3 */
         [16] = "debug_out",             /* GPO0 */
         [20] = "switch_reset",          /* GPO3 */
};

[ switch_interrupt and switch_reset will ultimately make it into the kernel,
   to be used by a dsa driver, but that is besides the point. ]

const char * const (* const ichx_gpiolib_names)[] = &scu_ichx_gpiolib_names;

and ichx_gpiolib_names is declared as

/* Allow overriding default gpio pin names */
const char * const (* const __weak ichx_gpiolib_names)[];

in gpio-ich.c. Pretty hackish, but at least it works. Slim chance though
to get this accepted upstream. Since the ultimate idea is to submit all
this code upstream, I would prefer to find a solution which is
acceptable for both the user and for upstream integration.

The second user is my employer. Same thing here, though even more complex
as there are several different platforms to support with different platform
drivers. Each platform exports a number of gpio pins to user space, often with
the same functionality across platforms. The request here is to have all
those pins in the same directory, for all platforms, which again suggests
using /sys/class/gpio.

Current approach is to use the "export" file to request pin exports,
which has its own challenges such as having to search for the pin numbers.
Well defined names and pins exported from the kernel would be much cleaner
and be easier to handle.

Of course, I could try to come up with a new class named "gpios" or similar,
put everything there, and start selling that idea. Somehow that doesn't
sound like a good idea to me.

>>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>> Applies to tip of linux-gpio/for-next.
>>
>>   Documentation/gpio/sysfs.txt  | 12 ++++++++----
>>   drivers/gpio/gpiolib-sysfs.c  | 23 ++++++++++++++++-------
>>   include/linux/gpio/consumer.h |  9 +++++++++
>>   3 files changed, 33 insertions(+), 11 deletions(-)
>>
>> diff --git a/Documentation/gpio/sysfs.txt b/Documentation/gpio/sysfs.txt
>> index c2c3a97..8e301b2 100644
>> --- a/Documentation/gpio/sysfs.txt
>> +++ b/Documentation/gpio/sysfs.txt
>> @@ -125,7 +125,11 @@ requested using gpio_request():
>>          /* export the GPIO to userspace */
>>          int gpiod_export(struct gpio_desc *desc, bool direction_may_change);
>>
>> -       /* reverse gpio_export() */
>> +       /* export named GPIO to userspace */
>> +       int gpiod_export_name(struct gpio_desc *desc, const char *ioname,
>> +                       bool direction_may_change);
>> +
>> +       /* reverse gpio_export() / gpiod_export_name() */
>>          void gpiod_unexport(struct gpio_desc *desc);
>>
>>          /* create a sysfs link to an exported GPIO node */
>> @@ -136,9 +140,9 @@ requested using gpio_request():
>>          int gpiod_sysfs_set_active_low(struct gpio_desc *desc, int value);
>>
>>   After a kernel driver requests a GPIO, it may only be made available in
>> -the sysfs interface by gpiod_export(). The driver can control whether the
>> -signal direction may change. This helps drivers prevent userspace code
>> -from accidentally clobbering important system state.
>> +the sysfs interface by gpiod_export() or gpiod_export_name(). The driver
>> +can control whether the signal direction may change. This helps drivers
>> +prevent userspace code from accidentally clobbering important system state.
>>
>>   This explicit exporting can help with debugging (by making some kinds
>>   of experiments easier), or can provide an always-there interface that's
>> diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
>> index be45a92..7d36230 100644
>> --- a/drivers/gpio/gpiolib-sysfs.c
>> +++ b/drivers/gpio/gpiolib-sysfs.c
>> @@ -523,13 +523,12 @@ static struct class gpio_class = {
>>    *
>>    * Returns zero on success, else an error.
>>    */
>> -int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
>> +int gpiod_export_name(struct gpio_desc *desc, const char *ioname,
>> +                     bool direction_may_change)
>>   {
>>          unsigned long           flags;
>>          int                     status;
>> -       const char              *ioname = NULL;
>>          struct device           *dev;
>> -       int                     offset;
>>
>>          /* can't export until sysfs is available ... */
>>          if (!gpio_class.p) {
>> @@ -560,10 +559,6 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
>>                  direction_may_change = false;
>>          spin_unlock_irqrestore(&gpio_lock, flags);
>>
>> -       offset = gpio_chip_hwgpio(desc);
>> -       if (desc->chip->names && desc->chip->names[offset])
>> -               ioname = desc->chip->names[offset];
>> -
>>          dev = device_create(&gpio_class, desc->chip->dev, MKDEV(0, 0),
>>                              desc, ioname ? ioname : "gpio%u",
>>                              desc_to_gpio(desc));
>> @@ -600,6 +595,20 @@ fail_unlock:
>>          gpiod_dbg(desc, "%s: status %d\n", __func__, status);
>>          return status;
>>   }
>> +EXPORT_SYMBOL_GPL(gpiod_export_name);
>> +
>> +int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
>> +{
>> +       const char *ioname = NULL;
>> +
>> +       if (desc) {
>> +               int offset = gpio_chip_hwgpio(desc);
>> +
>> +               if (desc->chip->names && desc->chip->names[offset])
>> +                       ioname = desc->chip->names[offset];
>
> I'd add a
>
> else
>      ioname = "gpio%u";
>
> so all the name-chosing code is grouped in the same place. Then you
> can remove that same check from gpiod_export_name().
>
Sure, no problem, though in that case I should probably add a check
to gpiod_export_name to ensure that the passed ioname is not NULL
and return -EINVAL if it is.

Guenter

Re: [RFC PATCH] gpiolib: Provide and export gpiod_export_name

[Guenter Roeck]

On 07/23/2014 11:29 PM, Jiří Prchal wrote:
> Hi,
> just to append to this: is in plan some kind of exporting named GPIOs from device tree to /sys/* or /dev/*? It would be very useful not only for me. Because I thing what GPIO is used for what is hardware design dependent same as for example what SPI chips are used. And SPI chips are in DT.
>

Yes, for one of my use cases that is how I would probably configure it;
alternatively it would be configured with platform data. It is
somewhat questionable, however, if this approach would be acceptable
for the Linux dt community, as it is a corner case between system
(hardware) description and configuration.

Guenter

> Dne 24.7.2014 v 07:44 Alexandre Courbot napsal(a):
>> On Thu, Jul 24, 2014 at 3:12 AM, Guenter Roeck <linux@roeck-us.net> wrote:
>>> gpiod_export_name is similar to gpiod_export, but lets the user
>>> determine the name used to export a gpio pin.
>>>
>>> Currently, the pin name is determined by the chip driver with
>>> the 'names' array in the gpio_chip data structure, or it is set
>>> to gpioX, where X is the pin number, if no name is provided by
>>> the chip driver.
>>
>> Oh, my. I did not even know about this 'names' array. This is pretty
>> bad - a GPIO provider should not decide what its GPIOs are used for.
>>
>> Luckily for you, this creates a precedent that makes this patch more
>> acceptable, in that it is not making the situation worse. Even though
>> I consider both solutions to be bad, I actually prefer your
>> gpiod_export_name() function to that 'names' array in gpio_chip...
>>
>>>
>>> It is, however, desirable to be able to provide the pin name when
>>> exporting the pin, for example from platform code. In other words,
>>> it would be useful to move the naming decision from the pin provider
>>> to the pin consumer. The gpio-pca953x driver provides this capability
>>> as part of its platform data. Other drivers could be enhanced in a
>>> similar way; however, this is not always possible or easy to accomplish.
>>> For example, mfd client drivers such as gpio-ich already use platform
>>> data to pass information from the mfd master driver to the client driver.
>>> Overloading this platform data to also provide an array of gpio pin names
>>> would be a challenge if not impossible.
>>>
>>> The alternative to use gpiod_export_link is also not always desirable,
>>> since it only creates a named link to a different directory, meaning
>>> the named gpio pin is not available in /sys/class/gpio but only
>>> in some platform specific directory and thus not as generic as possible
>>> and/or useful.
>>>
>>> A specific example for a use case is a gpio pin which reports AC power
>>> loss to user space. Depending on the platform and platform variant,
>>> the pin can be provided by various gpio chip drivers and pin numbers.
>>> It would be very desirable to have a well defined location such as
>>> /sys/class/gpio/ac_power_loss for this pin, so user space knows where
>>> to find the attribute without knowledge of the underlying platform
>>> variant or oher hardware details.
>>
>> As I explained on the other thread, I still encourage you to have
>> these GPIOs under device nodes that give a hint of who is provided the
>> GPIO (effectively exporting the (dev, function) tuple to user-space)
>> instead of having them popping out under /sys/class/gpio where nobody
>> knows where they come from and name collisions are much more likely.
>>
>> Your message sounds like you have no choice but have the named GPIO
>> link under the gpiochip's device node, but this is not the case -
>> gpio_export_link() let's you specify the device under which the link
>> should appear. Make that device be your "scu" device and you can have
>> a consistent sysfs path to access your GPIOs.
>>
>> Allowing GPIOs to pop up in the same directory with an arbitrary name
>> is just a recipe for a mess. But that's a recipe that is already
>> allowed thanks to that 'names' array. So I'm really confused about
>> what to do with this patch. If you can do with gpio_export_link() (and
>> I have not seen evidence of the contrary), please go that way instead.
>>
>>>
>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>> ---
>>> Applies to tip of linux-gpio/for-next.
>>>
>>>   Documentation/gpio/sysfs.txt  | 12 ++++++++----
>>>   drivers/gpio/gpiolib-sysfs.c  | 23 ++++++++++++++++-------
>>>   include/linux/gpio/consumer.h |  9 +++++++++
>>>   3 files changed, 33 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/Documentation/gpio/sysfs.txt b/Documentation/gpio/sysfs.txt
>>> index c2c3a97..8e301b2 100644
>>> --- a/Documentation/gpio/sysfs.txt
>>> +++ b/Documentation/gpio/sysfs.txt
>>> @@ -125,7 +125,11 @@ requested using gpio_request():
>>>          /* export the GPIO to userspace */
>>>          int gpiod_export(struct gpio_desc *desc, bool direction_may_change);
>>>
>>> -       /* reverse gpio_export() */
>>> +       /* export named GPIO to userspace */
>>> +       int gpiod_export_name(struct gpio_desc *desc, const char *ioname,
>>> +                       bool direction_may_change);
>>> +
>>> +       /* reverse gpio_export() / gpiod_export_name() */
>>>          void gpiod_unexport(struct gpio_desc *desc);
>>>
>>>          /* create a sysfs link to an exported GPIO node */
>>> @@ -136,9 +140,9 @@ requested using gpio_request():
>>>          int gpiod_sysfs_set_active_low(struct gpio_desc *desc, int value);
>>>
>>>   After a kernel driver requests a GPIO, it may only be made available in
>>> -the sysfs interface by gpiod_export(). The driver can control whether the
>>> -signal direction may change. This helps drivers prevent userspace code
>>> -from accidentally clobbering important system state.
>>> +the sysfs interface by gpiod_export() or gpiod_export_name(). The driver
>>> +can control whether the signal direction may change. This helps drivers
>>> +prevent userspace code from accidentally clobbering important system state.
>>>
>>>   This explicit exporting can help with debugging (by making some kinds
>>>   of experiments easier), or can provide an always-there interface that's
>>> diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
>>> index be45a92..7d36230 100644
>>> --- a/drivers/gpio/gpiolib-sysfs.c
>>> +++ b/drivers/gpio/gpiolib-sysfs.c
>>> @@ -523,13 +523,12 @@ static struct class gpio_class = {
>>>    *
>>>    * Returns zero on success, else an error.
>>>    */
>>> -int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
>>> +int gpiod_export_name(struct gpio_desc *desc, const char *ioname,
>>> +                     bool direction_may_change)
>>>   {
>>>          unsigned long           flags;
>>>          int                     status;
>>> -       const char              *ioname = NULL;
>>>          struct device           *dev;
>>> -       int                     offset;
>>>
>>>          /* can't export until sysfs is available ... */
>>>          if (!gpio_class.p) {
>>> @@ -560,10 +559,6 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
>>>                  direction_may_change = false;
>>>          spin_unlock_irqrestore(&gpio_lock, flags);
>>>
>>> -       offset = gpio_chip_hwgpio(desc);
>>> -       if (desc->chip->names && desc->chip->names[offset])
>>> -               ioname = desc->chip->names[offset];
>>> -
>>>          dev = device_create(&gpio_class, desc->chip->dev, MKDEV(0, 0),
>>>                              desc, ioname ? ioname : "gpio%u",
>>>                              desc_to_gpio(desc));
>>> @@ -600,6 +595,20 @@ fail_unlock:
>>>          gpiod_dbg(desc, "%s: status %d\n", __func__, status);
>>>          return status;
>>>   }
>>> +EXPORT_SYMBOL_GPL(gpiod_export_name);
>>> +
>>> +int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
>>> +{
>>> +       const char *ioname = NULL;
>>> +
>>> +       if (desc) {
>>> +               int offset = gpio_chip_hwgpio(desc);
>>> +
>>> +               if (desc->chip->names && desc->chip->names[offset])
>>> +                       ioname = desc->chip->names[offset];
>>
>> I'd add a
>>
>> else
>>      ioname = "gpio%u";
>>
>> so all the name-chosing code is grouped in the same place. Then you
>> can remove that same check from gpiod_export_name().
>>
>>> +       }
>>> +       return gpiod_export_name(desc, ioname, direction_may_change);
>>> +}
>>>   EXPORT_SYMBOL_GPL(gpiod_export);
>>>
>>>   static int match_export(struct device *dev, const void *data)
>>> diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
>>> index 05e53cc..986da3e 100644
>>> --- a/include/linux/gpio/consumer.h
>>> +++ b/include/linux/gpio/consumer.h
>>> @@ -260,6 +260,8 @@ static inline int desc_to_gpio(const struct gpio_desc *desc)
>>>   #if IS_ENABLED(CONFIG_GPIOLIB) && IS_ENABLED(CONFIG_GPIO_SYSFS)
>>>
>>>   int gpiod_export(struct gpio_desc *desc, bool direction_may_change);
>>> +int gpiod_export_name(struct gpio_desc *desc, const char *ioname,
>>> +                     bool direction_may_change);
>>>   int gpiod_export_link(struct device *dev, const char *name,
>>>                        struct gpio_desc *desc);
>>>   int gpiod_sysfs_set_active_low(struct gpio_desc *desc, int value);
>>> @@ -273,6 +275,13 @@ static inline int gpiod_export(struct gpio_desc *desc,
>>>          return -ENOSYS;
>>>   }
>>>
>>> +static inline int gpiod_export_name(struct gpio_desc *desc,
>>> +                                   const char *ioname,
>>> +                                   bool direction_may_change)
>>> +{
>>> +       return -ENOSYS;
>>> +}
>>> +
>>>   static inline int gpiod_export_link(struct device *dev, const char *name,
>>>                                      struct gpio_desc *desc)
>>>   {
>>> --
>>> 1.9.1
>>>
>> --
>> 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
>>
>
>

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH] gpiolib: Provide and export gpiod_export_name

[Guenter Roeck]

On 07/23/2014 11:29 PM, Jiří Prchal wrote:
> Hi,
> just to append to this: is in plan some kind of exporting named GPIOs from device tree to /sys/* or /dev/*? It would be very useful not only for me. Because I thing what GPIO is used for what is hardware design dependent same as for example what SPI chips are used. And SPI chips are in DT.
>

Yes, for one of my use cases that is how I would probably configure it;
alternatively it would be configured with platform data. It is
somewhat questionable, however, if this approach would be acceptable
for the Linux dt community, as it is a corner case between system
(hardware) description and configuration.

Guenter

> Dne 24.7.2014 v 07:44 Alexandre Courbot napsal(a):
>> On Thu, Jul 24, 2014 at 3:12 AM, Guenter Roeck <linux@roeck-us.net> wrote:
>>> gpiod_export_name is similar to gpiod_export, but lets the user
>>> determine the name used to export a gpio pin.
>>>
>>> Currently, the pin name is determined by the chip driver with
>>> the 'names' array in the gpio_chip data structure, or it is set
>>> to gpioX, where X is the pin number, if no name is provided by
>>> the chip driver.
>>
>> Oh, my. I did not even know about this 'names' array. This is pretty
>> bad - a GPIO provider should not decide what its GPIOs are used for.
>>
>> Luckily for you, this creates a precedent that makes this patch more
>> acceptable, in that it is not making the situation worse. Even though
>> I consider both solutions to be bad, I actually prefer your
>> gpiod_export_name() function to that 'names' array in gpio_chip...
>>
>>>
>>> It is, however, desirable to be able to provide the pin name when
>>> exporting the pin, for example from platform code. In other words,
>>> it would be useful to move the naming decision from the pin provider
>>> to the pin consumer. The gpio-pca953x driver provides this capability
>>> as part of its platform data. Other drivers could be enhanced in a
>>> similar way; however, this is not always possible or easy to accomplish.
>>> For example, mfd client drivers such as gpio-ich already use platform
>>> data to pass information from the mfd master driver to the client driver.
>>> Overloading this platform data to also provide an array of gpio pin names
>>> would be a challenge if not impossible.
>>>
>>> The alternative to use gpiod_export_link is also not always desirable,
>>> since it only creates a named link to a different directory, meaning
>>> the named gpio pin is not available in /sys/class/gpio but only
>>> in some platform specific directory and thus not as generic as possible
>>> and/or useful.
>>>
>>> A specific example for a use case is a gpio pin which reports AC power
>>> loss to user space. Depending on the platform and platform variant,
>>> the pin can be provided by various gpio chip drivers and pin numbers.
>>> It would be very desirable to have a well defined location such as
>>> /sys/class/gpio/ac_power_loss for this pin, so user space knows where
>>> to find the attribute without knowledge of the underlying platform
>>> variant or oher hardware details.
>>
>> As I explained on the other thread, I still encourage you to have
>> these GPIOs under device nodes that give a hint of who is provided the
>> GPIO (effectively exporting the (dev, function) tuple to user-space)
>> instead of having them popping out under /sys/class/gpio where nobody
>> knows where they come from and name collisions are much more likely.
>>
>> Your message sounds like you have no choice but have the named GPIO
>> link under the gpiochip's device node, but this is not the case -
>> gpio_export_link() let's you specify the device under which the link
>> should appear. Make that device be your "scu" device and you can have
>> a consistent sysfs path to access your GPIOs.
>>
>> Allowing GPIOs to pop up in the same directory with an arbitrary name
>> is just a recipe for a mess. But that's a recipe that is already
>> allowed thanks to that 'names' array. So I'm really confused about
>> what to do with this patch. If you can do with gpio_export_link() (and
>> I have not seen evidence of the contrary), please go that way instead.
>>
>>>
>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>> ---
>>> Applies to tip of linux-gpio/for-next.
>>>
>>>   Documentation/gpio/sysfs.txt  | 12 ++++++++----
>>>   drivers/gpio/gpiolib-sysfs.c  | 23 ++++++++++++++++-------
>>>   include/linux/gpio/consumer.h |  9 +++++++++
>>>   3 files changed, 33 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/Documentation/gpio/sysfs.txt b/Documentation/gpio/sysfs.txt
>>> index c2c3a97..8e301b2 100644
>>> --- a/Documentation/gpio/sysfs.txt
>>> +++ b/Documentation/gpio/sysfs.txt
>>> @@ -125,7 +125,11 @@ requested using gpio_request():
>>>          /* export the GPIO to userspace */
>>>          int gpiod_export(struct gpio_desc *desc, bool direction_may_change);
>>>
>>> -       /* reverse gpio_export() */
>>> +       /* export named GPIO to userspace */
>>> +       int gpiod_export_name(struct gpio_desc *desc, const char *ioname,
>>> +                       bool direction_may_change);
>>> +
>>> +       /* reverse gpio_export() / gpiod_export_name() */
>>>          void gpiod_unexport(struct gpio_desc *desc);
>>>
>>>          /* create a sysfs link to an exported GPIO node */
>>> @@ -136,9 +140,9 @@ requested using gpio_request():
>>>          int gpiod_sysfs_set_active_low(struct gpio_desc *desc, int value);
>>>
>>>   After a kernel driver requests a GPIO, it may only be made available in
>>> -the sysfs interface by gpiod_export(). The driver can control whether the
>>> -signal direction may change. This helps drivers prevent userspace code
>>> -from accidentally clobbering important system state.
>>> +the sysfs interface by gpiod_export() or gpiod_export_name(). The driver
>>> +can control whether the signal direction may change. This helps drivers
>>> +prevent userspace code from accidentally clobbering important system state.
>>>
>>>   This explicit exporting can help with debugging (by making some kinds
>>>   of experiments easier), or can provide an always-there interface that's
>>> diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
>>> index be45a92..7d36230 100644
>>> --- a/drivers/gpio/gpiolib-sysfs.c
>>> +++ b/drivers/gpio/gpiolib-sysfs.c
>>> @@ -523,13 +523,12 @@ static struct class gpio_class = {
>>>    *
>>>    * Returns zero on success, else an error.
>>>    */
>>> -int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
>>> +int gpiod_export_name(struct gpio_desc *desc, const char *ioname,
>>> +                     bool direction_may_change)
>>>   {
>>>          unsigned long           flags;
>>>          int                     status;
>>> -       const char              *ioname = NULL;
>>>          struct device           *dev;
>>> -       int                     offset;
>>>
>>>          /* can't export until sysfs is available ... */
>>>          if (!gpio_class.p) {
>>> @@ -560,10 +559,6 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
>>>                  direction_may_change = false;
>>>          spin_unlock_irqrestore(&gpio_lock, flags);
>>>
>>> -       offset = gpio_chip_hwgpio(desc);
>>> -       if (desc->chip->names && desc->chip->names[offset])
>>> -               ioname = desc->chip->names[offset];
>>> -
>>>          dev = device_create(&gpio_class, desc->chip->dev, MKDEV(0, 0),
>>>                              desc, ioname ? ioname : "gpio%u",
>>>                              desc_to_gpio(desc));
>>> @@ -600,6 +595,20 @@ fail_unlock:
>>>          gpiod_dbg(desc, "%s: status %d\n", __func__, status);
>>>          return status;
>>>   }
>>> +EXPORT_SYMBOL_GPL(gpiod_export_name);
>>> +
>>> +int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
>>> +{
>>> +       const char *ioname = NULL;
>>> +
>>> +       if (desc) {
>>> +               int offset = gpio_chip_hwgpio(desc);
>>> +
>>> +               if (desc->chip->names && desc->chip->names[offset])
>>> +                       ioname = desc->chip->names[offset];
>>
>> I'd add a
>>
>> else
>>      ioname = "gpio%u";
>>
>> so all the name-chosing code is grouped in the same place. Then you
>> can remove that same check from gpiod_export_name().
>>
>>> +       }
>>> +       return gpiod_export_name(desc, ioname, direction_may_change);
>>> +}
>>>   EXPORT_SYMBOL_GPL(gpiod_export);
>>>
>>>   static int match_export(struct device *dev, const void *data)
>>> diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
>>> index 05e53cc..986da3e 100644
>>> --- a/include/linux/gpio/consumer.h
>>> +++ b/include/linux/gpio/consumer.h
>>> @@ -260,6 +260,8 @@ static inline int desc_to_gpio(const struct gpio_desc *desc)
>>>   #if IS_ENABLED(CONFIG_GPIOLIB) && IS_ENABLED(CONFIG_GPIO_SYSFS)
>>>
>>>   int gpiod_export(struct gpio_desc *desc, bool direction_may_change);
>>> +int gpiod_export_name(struct gpio_desc *desc, const char *ioname,
>>> +                     bool direction_may_change);
>>>   int gpiod_export_link(struct device *dev, const char *name,
>>>                        struct gpio_desc *desc);
>>>   int gpiod_sysfs_set_active_low(struct gpio_desc *desc, int value);
>>> @@ -273,6 +275,13 @@ static inline int gpiod_export(struct gpio_desc *desc,
>>>          return -ENOSYS;
>>>   }
>>>
>>> +static inline int gpiod_export_name(struct gpio_desc *desc,
>>> +                                   const char *ioname,
>>> +                                   bool direction_may_change)
>>> +{
>>> +       return -ENOSYS;
>>> +}
>>> +
>>>   static inline int gpiod_export_link(struct device *dev, const char *name,
>>>                                      struct gpio_desc *desc)
>>>   {
>>> --
>>> 1.9.1
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>
>

Re: [RFC PATCH] gpiolib: Provide and export gpiod_export_name

[Jiří Prchal]

What about this modification? If is defined label, use it prioritlly, at second use name in chip description.

@@ -842,7 +842,9 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
  	spin_unlock_irqrestore(&gpio_lock, flags);

  	offset = gpio_chip_hwgpio(desc);
-	if (desc->chip->names && desc->chip->names[offset])
+	if (desc->label)
+		ioname = desc->label;
+	else if (desc->chip->names && desc->chip->names[offset])
  		ioname = desc->chip->names[offset];

  	dev = device_create(&gpio_class, desc->chip->dev, MKDEV(0, 0),



Dne 24.7.2014 v 08:36 Guenter Roeck napsal(a):
> On 07/23/2014 11:29 PM, Jiří Prchal wrote:
>> Hi,
>> just to append to this: is in plan some kind of exporting named GPIOs from device tree to /sys/* or /dev/*? It would
>> be very useful not only for me. Because I thing what GPIO is used for what is hardware design dependent same as for
>> example what SPI chips are used. And SPI chips are in DT.
>>
>
> Yes, for one of my use cases that is how I would probably configure it;
> alternatively it would be configured with platform data. It is
> somewhat questionable, however, if this approach would be acceptable
> for the Linux dt community, as it is a corner case between system
> (hardware) description and configuration.
>
> Guenter
>
>> Dne 24.7.2014 v 07:44 Alexandre Courbot napsal(a):
>>> On Thu, Jul 24, 2014 at 3:12 AM, Guenter Roeck <linux@roeck-us.net> wrote:
>>>> gpiod_export_name is similar to gpiod_export, but lets the user
>>>> determine the name used to export a gpio pin.
>>>>
>>>> Currently, the pin name is determined by the chip driver with
>>>> the 'names' array in the gpio_chip data structure, or it is set
>>>> to gpioX, where X is the pin number, if no name is provided by
>>>> the chip driver.
>>>
>>> Oh, my. I did not even know about this 'names' array. This is pretty
>>> bad - a GPIO provider should not decide what its GPIOs are used for.
>>>
>>> Luckily for you, this creates a precedent that makes this patch more
>>> acceptable, in that it is not making the situation worse. Even though
>>> I consider both solutions to be bad, I actually prefer your
>>> gpiod_export_name() function to that 'names' array in gpio_chip...
>>>
>>>>
>>>> It is, however, desirable to be able to provide the pin name when
>>>> exporting the pin, for example from platform code. In other words,
>>>> it would be useful to move the naming decision from the pin provider
>>>> to the pin consumer. The gpio-pca953x driver provides this capability
>>>> as part of its platform data. Other drivers could be enhanced in a
>>>> similar way; however, this is not always possible or easy to accomplish.
>>>> For example, mfd client drivers such as gpio-ich already use platform
>>>> data to pass information from the mfd master driver to the client driver.
>>>> Overloading this platform data to also provide an array of gpio pin names
>>>> would be a challenge if not impossible.
>>>>
>>>> The alternative to use gpiod_export_link is also not always desirable,
>>>> since it only creates a named link to a different directory, meaning
>>>> the named gpio pin is not available in /sys/class/gpio but only
>>>> in some platform specific directory and thus not as generic as possible
>>>> and/or useful.
>>>>
>>>> A specific example for a use case is a gpio pin which reports AC power
>>>> loss to user space. Depending on the platform and platform variant,
>>>> the pin can be provided by various gpio chip drivers and pin numbers.
>>>> It would be very desirable to have a well defined location such as
>>>> /sys/class/gpio/ac_power_loss for this pin, so user space knows where
>>>> to find the attribute without knowledge of the underlying platform
>>>> variant or oher hardware details.
>>>
>>> As I explained on the other thread, I still encourage you to have
>>> these GPIOs under device nodes that give a hint of who is provided the
>>> GPIO (effectively exporting the (dev, function) tuple to user-space)
>>> instead of having them popping out under /sys/class/gpio where nobody
>>> knows where they come from and name collisions are much more likely.
>>>
>>> Your message sounds like you have no choice but have the named GPIO
>>> link under the gpiochip's device node, but this is not the case -
>>> gpio_export_link() let's you specify the device under which the link
>>> should appear. Make that device be your "scu" device and you can have
>>> a consistent sysfs path to access your GPIOs.
>>>
>>> Allowing GPIOs to pop up in the same directory with an arbitrary name
>>> is just a recipe for a mess. But that's a recipe that is already
>>> allowed thanks to that 'names' array. So I'm really confused about
>>> what to do with this patch. If you can do with gpio_export_link() (and
>>> I have not seen evidence of the contrary), please go that way instead.
>>>
>>>>
>>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>>> ---
>>>> Applies to tip of linux-gpio/for-next.
>>>>
>>>>   Documentation/gpio/sysfs.txt  | 12 ++++++++----
>>>>   drivers/gpio/gpiolib-sysfs.c  | 23 ++++++++++++++++-------
>>>>   include/linux/gpio/consumer.h |  9 +++++++++
>>>>   3 files changed, 33 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/Documentation/gpio/sysfs.txt b/Documentation/gpio/sysfs.txt
>>>> index c2c3a97..8e301b2 100644
>>>> --- a/Documentation/gpio/sysfs.txt
>>>> +++ b/Documentation/gpio/sysfs.txt
>>>> @@ -125,7 +125,11 @@ requested using gpio_request():
>>>>          /* export the GPIO to userspace */
>>>>          int gpiod_export(struct gpio_desc *desc, bool direction_may_change);
>>>>
>>>> -       /* reverse gpio_export() */
>>>> +       /* export named GPIO to userspace */
>>>> +       int gpiod_export_name(struct gpio_desc *desc, const char *ioname,
>>>> +                       bool direction_may_change);
>>>> +
>>>> +       /* reverse gpio_export() / gpiod_export_name() */
>>>>          void gpiod_unexport(struct gpio_desc *desc);
>>>>
>>>>          /* create a sysfs link to an exported GPIO node */
>>>> @@ -136,9 +140,9 @@ requested using gpio_request():
>>>>          int gpiod_sysfs_set_active_low(struct gpio_desc *desc, int value);
>>>>
>>>>   After a kernel driver requests a GPIO, it may only be made available in
>>>> -the sysfs interface by gpiod_export(). The driver can control whether the
>>>> -signal direction may change. This helps drivers prevent userspace code
>>>> -from accidentally clobbering important system state.
>>>> +the sysfs interface by gpiod_export() or gpiod_export_name(). The driver
>>>> +can control whether the signal direction may change. This helps drivers
>>>> +prevent userspace code from accidentally clobbering important system state.
>>>>
>>>>   This explicit exporting can help with debugging (by making some kinds
>>>>   of experiments easier), or can provide an always-there interface that's
>>>> diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
>>>> index be45a92..7d36230 100644
>>>> --- a/drivers/gpio/gpiolib-sysfs.c
>>>> +++ b/drivers/gpio/gpiolib-sysfs.c
>>>> @@ -523,13 +523,12 @@ static struct class gpio_class = {
>>>>    *
>>>>    * Returns zero on success, else an error.
>>>>    */
>>>> -int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
>>>> +int gpiod_export_name(struct gpio_desc *desc, const char *ioname,
>>>> +                     bool direction_may_change)
>>>>   {
>>>>          unsigned long           flags;
>>>>          int                     status;
>>>> -       const char              *ioname = NULL;
>>>>          struct device           *dev;
>>>> -       int                     offset;
>>>>
>>>>          /* can't export until sysfs is available ... */
>>>>          if (!gpio_class.p) {
>>>> @@ -560,10 +559,6 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
>>>>                  direction_may_change = false;
>>>>          spin_unlock_irqrestore(&gpio_lock, flags);
>>>>
>>>> -       offset = gpio_chip_hwgpio(desc);
>>>> -       if (desc->chip->names && desc->chip->names[offset])
>>>> -               ioname = desc->chip->names[offset];
>>>> -
>>>>          dev = device_create(&gpio_class, desc->chip->dev, MKDEV(0, 0),
>>>>                              desc, ioname ? ioname : "gpio%u",
>>>>                              desc_to_gpio(desc));
>>>> @@ -600,6 +595,20 @@ fail_unlock:
>>>>          gpiod_dbg(desc, "%s: status %d\n", __func__, status);
>>>>          return status;
>>>>   }
>>>> +EXPORT_SYMBOL_GPL(gpiod_export_name);
>>>> +
>>>> +int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
>>>> +{
>>>> +       const char *ioname = NULL;
>>>> +
>>>> +       if (desc) {
>>>> +               int offset = gpio_chip_hwgpio(desc);
>>>> +
>>>> +               if (desc->chip->names && desc->chip->names[offset])
>>>> +                       ioname = desc->chip->names[offset];
>>>
>>> I'd add a
>>>
>>> else
>>>      ioname = "gpio%u";
>>>
>>> so all the name-chosing code is grouped in the same place. Then you
>>> can remove that same check from gpiod_export_name().
>>>
>>>> +       }
>>>> +       return gpiod_export_name(desc, ioname, direction_may_change);
>>>> +}
>>>>   EXPORT_SYMBOL_GPL(gpiod_export);
>>>>
>>>>   static int match_export(struct device *dev, const void *data)
>>>> diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
>>>> index 05e53cc..986da3e 100644
>>>> --- a/include/linux/gpio/consumer.h
>>>> +++ b/include/linux/gpio/consumer.h
>>>> @@ -260,6 +260,8 @@ static inline int desc_to_gpio(const struct gpio_desc *desc)
>>>>   #if IS_ENABLED(CONFIG_GPIOLIB) && IS_ENABLED(CONFIG_GPIO_SYSFS)
>>>>
>>>>   int gpiod_export(struct gpio_desc *desc, bool direction_may_change);
>>>> +int gpiod_export_name(struct gpio_desc *desc, const char *ioname,
>>>> +                     bool direction_may_change);
>>>>   int gpiod_export_link(struct device *dev, const char *name,
>>>>                        struct gpio_desc *desc);
>>>>   int gpiod_sysfs_set_active_low(struct gpio_desc *desc, int value);
>>>> @@ -273,6 +275,13 @@ static inline int gpiod_export(struct gpio_desc *desc,
>>>>          return -ENOSYS;
>>>>   }
>>>>
>>>> +static inline int gpiod_export_name(struct gpio_desc *desc,
>>>> +                                   const char *ioname,
>>>> +                                   bool direction_may_change)
>>>> +{
>>>> +       return -ENOSYS;
>>>> +}
>>>> +
>>>>   static inline int gpiod_export_link(struct device *dev, const char *name,
>>>>                                      struct gpio_desc *desc)
>>>>   {
>>>> --
>>>> 1.9.1
>>>>
>>> --
>>> 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
>>>
>>
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [RFC PATCH] gpiolib: Provide and export gpiod_export_name

[Guenter Roeck]

On 07/25/2014 12:46 AM, Jiří Prchal wrote:
> What about this modification? If is defined label, use it prioritlly, at second use name in chip description.
>
> @@ -842,7 +842,9 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
>       spin_unlock_irqrestore(&gpio_lock, flags);
>
>       offset = gpio_chip_hwgpio(desc);
> -    if (desc->chip->names && desc->chip->names[offset])
> +    if (desc->label)
> +        ioname = desc->label;
> +    else if (desc->chip->names && desc->chip->names[offset])
>           ioname = desc->chip->names[offset];
>

Label is not unique. It is, for example, used by the sysfs export function,
and all pins exported from user space have the same label. The first
pin exported through sysfs would be named "sysfs", the second export
would fail.

Guenter

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH] gpiolib: Provide and export gpiod_export_name

[Guenter Roeck]

On 07/25/2014 12:46 AM, Jiří Prchal wrote:
> What about this modification? If is defined label, use it prioritlly, at second use name in chip description.
>
> @@ -842,7 +842,9 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
>       spin_unlock_irqrestore(&gpio_lock, flags);
>
>       offset = gpio_chip_hwgpio(desc);
> -    if (desc->chip->names && desc->chip->names[offset])
> +    if (desc->label)
> +        ioname = desc->label;
> +    else if (desc->chip->names && desc->chip->names[offset])
>           ioname = desc->chip->names[offset];
>

Label is not unique. It is, for example, used by the sysfs export function,
and all pins exported from user space have the same label. The first
pin exported through sysfs would be named "sysfs", the second export
would fail.

Guenter

Re: [RFC PATCH] gpiolib: Provide and export gpiod_export_name

[Alexandre Courbot]

Sorry for the delayed reply...

On Thu, Jul 24, 2014 at 3:33 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>> As I explained on the other thread, I still encourage you to have
>> these GPIOs under device nodes that give a hint of who is provided the
>> GPIO (effectively exporting the (dev, function) tuple to user-space)
>> instead of having them popping out under /sys/class/gpio where nobody
>> knows where they come from and name collisions are much more likely.
>>
>> Your message sounds like you have no choice but have the named GPIO
>> link under the gpiochip's device node, but this is not the case -
>> gpio_export_link() let's you specify the device under which the link
>> should appear. Make that device be your "scu" device and you can have
>> a consistent sysfs path to access your GPIOs.
>>
> Yes, I understand, but that is not acceptable for the users - see below.
>
>
>> Allowing GPIOs to pop up in the same directory with an arbitrary name
>> is just a recipe for a mess. But that's a recipe that is already
>> allowed thanks to that 'names' array. So I'm really confused about
>> what to do with this patch. If you can do with gpio_export_link() (and
>> I have not seen evidence of the contrary), please go that way instead.
>>
>
> I personally don't think it is that much of a mess. Sure, once has to be
> careful when selecting names, but I don't see a problem with that.
>
> I have two users for this. Interestingly the problem is pretty
> much the same, though the applications are completely different.
>
> One is the company using the scu.c file. They are currently using the
> pca953x driver approach (using the names array), but they also have
> a new version of their product which also uses gpio pins from gpio-ich.
> For consistency, they want all gpio pins in the same directory, meaning
> /sys/class/gpio.
>
> The currently implemented solution is to have a weak pointer to the names
> array in gpio-ch.c and override it with a pointer from scu.c.
>
> /* SCU specific gpio pin names. Only works if module is built into kernel.
> */
> static const char * const scu_ichx_gpiolib_names[128] = {
>         [0] = "switch_interrupt",       /* GPI0 */
>         [3] = "ac_loss_detect",         /* GPI3 */
>         [16] = "debug_out",             /* GPO0 */
>         [20] = "switch_reset",          /* GPO3 */
> };
>
> [ switch_interrupt and switch_reset will ultimately make it into the kernel,
>   to be used by a dsa driver, but that is besides the point. ]
>
> const char * const (* const ichx_gpiolib_names)[] = &scu_ichx_gpiolib_names;
>
> and ichx_gpiolib_names is declared as
>
> /* Allow overriding default gpio pin names */
> const char * const (* const __weak ichx_gpiolib_names)[];
>
> in gpio-ich.c. Pretty hackish, but at least it works. Slim chance though
> to get this accepted upstream. Since the ultimate idea is to submit all
> this code upstream, I would prefer to find a solution which is
> acceptable for both the user and for upstream integration.
>
> The second user is my employer. Same thing here, though even more complex
> as there are several different platforms to support with different platform
> drivers. Each platform exports a number of gpio pins to user space, often
> with
> the same functionality across platforms. The request here is to have all
> those pins in the same directory, for all platforms, which again suggests
> using /sys/class/gpio.
>
> Current approach is to use the "export" file to request pin exports,
> which has its own challenges such as having to search for the pin numbers.
> Well defined names and pins exported from the kernel would be much cleaner
> and be easier to handle.
>
> Of course, I could try to come up with a new class named "gpios" or similar,
> put everything there, and start selling that idea. Somehow that doesn't
> sound like a good idea to me.

Or you could have a platform device which sole purpose is to request
the GPIOs to export and export them using gpio_export() and
gpio_export_link() using itself as the device parameters. All your
links would then appear under the same sysfs directory, and that
directory name would be consistent across platforms. What is wrong
with this approach?

As for the patch itself, as I said before I am not a huge fan of
putting randomly-named exports under /sys/class/gpio, but since there
is a precedent with the driver GPIO names array I don't think this
makes the situation much worse. Still I'd like you to explain me what
I missed and why you cannot use the technique described above to
achieve your goal with the currently existing interfaces.

Re: [RFC PATCH] gpiolib: Provide and export gpiod_export_name

[Guenter Roeck]

On 08/06/2014 11:25 PM, Alexandre Courbot wrote:
> Sorry for the delayed reply...
>
> On Thu, Jul 24, 2014 at 3:33 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>>> As I explained on the other thread, I still encourage you to have
>>> these GPIOs under device nodes that give a hint of who is provided the
>>> GPIO (effectively exporting the (dev, function) tuple to user-space)
>>> instead of having them popping out under /sys/class/gpio where nobody
>>> knows where they come from and name collisions are much more likely.
>>>
>>> Your message sounds like you have no choice but have the named GPIO
>>> link under the gpiochip's device node, but this is not the case -
>>> gpio_export_link() let's you specify the device under which the link
>>> should appear. Make that device be your "scu" device and you can have
>>> a consistent sysfs path to access your GPIOs.
>>>
>> Yes, I understand, but that is not acceptable for the users - see below.
>>
>>
>>> Allowing GPIOs to pop up in the same directory with an arbitrary name
>>> is just a recipe for a mess. But that's a recipe that is already
>>> allowed thanks to that 'names' array. So I'm really confused about
>>> what to do with this patch. If you can do with gpio_export_link() (and
>>> I have not seen evidence of the contrary), please go that way instead.
>>>
>>
>> I personally don't think it is that much of a mess. Sure, once has to be
>> careful when selecting names, but I don't see a problem with that.
>>
>> I have two users for this. Interestingly the problem is pretty
>> much the same, though the applications are completely different.
>>
>> One is the company using the scu.c file. They are currently using the
>> pca953x driver approach (using the names array), but they also have
>> a new version of their product which also uses gpio pins from gpio-ich.
>> For consistency, they want all gpio pins in the same directory, meaning
>> /sys/class/gpio.
>>
>> The currently implemented solution is to have a weak pointer to the names
>> array in gpio-ch.c and override it with a pointer from scu.c.
>>
>> /* SCU specific gpio pin names. Only works if module is built into kernel.
>> */
>> static const char * const scu_ichx_gpiolib_names[128] = {
>>          [0] = "switch_interrupt",       /* GPI0 */
>>          [3] = "ac_loss_detect",         /* GPI3 */
>>          [16] = "debug_out",             /* GPO0 */
>>          [20] = "switch_reset",          /* GPO3 */
>> };
>>
>> [ switch_interrupt and switch_reset will ultimately make it into the kernel,
>>    to be used by a dsa driver, but that is besides the point. ]
>>
>> const char * const (* const ichx_gpiolib_names)[] = &scu_ichx_gpiolib_names;
>>
>> and ichx_gpiolib_names is declared as
>>
>> /* Allow overriding default gpio pin names */
>> const char * const (* const __weak ichx_gpiolib_names)[];
>>
>> in gpio-ich.c. Pretty hackish, but at least it works. Slim chance though
>> to get this accepted upstream. Since the ultimate idea is to submit all
>> this code upstream, I would prefer to find a solution which is
>> acceptable for both the user and for upstream integration.
>>
>> The second user is my employer. Same thing here, though even more complex
>> as there are several different platforms to support with different platform
>> drivers. Each platform exports a number of gpio pins to user space, often
>> with
>> the same functionality across platforms. The request here is to have all
>> those pins in the same directory, for all platforms, which again suggests
>> using /sys/class/gpio.
>>
>> Current approach is to use the "export" file to request pin exports,
>> which has its own challenges such as having to search for the pin numbers.
>> Well defined names and pins exported from the kernel would be much cleaner
>> and be easier to handle.
>>
>> Of course, I could try to come up with a new class named "gpios" or similar,
>> put everything there, and start selling that idea. Somehow that doesn't
>> sound like a good idea to me.
>
> Or you could have a platform device which sole purpose is to request
> the GPIOs to export and export them using gpio_export() and
> gpio_export_link() using itself as the device parameters. All your
> links would then appear under the same sysfs directory, and that
> directory name would be consistent across platforms. What is wrong
> with this approach?
>
The link does not appear in /sys/class/gpio. I understand you don't like the
idea of having named gpio pins in that directory, but that is supported today,
it works, and others do like it.

> As for the patch itself, as I said before I am not a huge fan of
> putting randomly-named exports under /sys/class/gpio, but since there
> is a precedent with the driver GPIO names array I don't think this
> makes the situation much worse. Still I'd like you to explain me what
> I missed and why you cannot use the technique described above to
> achieve your goal with the currently existing interfaces.
>

I thought I explained it before; my users (ie the people writing user space
applications accessing the pins) expect to see the exported pins in /sys/class/gpio
and not in a more or less arbitrary directory. They are used to this
approach, and they are less than enthusiastic to change it. The code needed
to generate the exported pin names is a bit messy, it being tied to the driver,
but it does both exist and work. This patch was an attempt to provide a
cleaner API to accomplish the same without having to touch various gpio
drivers which don't provide the ability to configure the names array.

Note that I don't consider the names "random". They are much less random
than gpioX, where X can change each time the system boots or, for OIR
capable systems, each time a gpio driver is instantiated or removed.
In today's system, without well defined names, one never knows if gpioX points
to the pin the user is looking for. If the pin is named "this-is-your-pin"
it is much easier to write user space code using it.

Oddly enough, if I would use the platform driver approach you suggest,
I would still need a well defined directory, say, /sys/class/named_gpios,
and I would still have to handle the potential situation that some genius
tries to export two different pins with the same name, and handle
the resulting failure. I would still need an API to tell that driver to export
a GPIO pin under a specific name. I don't really see how that would be
different to providing that API in the gpio subsystem, and to using
/sys/class/gpio to start with and just fail the export if a duplicate
name is specified ... which happens to be exactly what is done today
already. On top of that, this would result in two different means
to export gpio pins to user space, one triggered through the gpio driver
and one triggered from outside through a new driver and API.
I can not imagine that anyone would be happy about that.
So, quite frankly, I don't entirely understand your objection.

Of course, thinking about it, maybe I should just export the gpio_class
variable in my kernel tree. Looks like I'll have to maintain local kernel
changes anyway, and it seems to be easier to export gpio_class and use a
symlink to gpio_class than to use driver based names, per driver hacks,
or a new class / driver. Hmmm ... I'll start looking into that.
A two-line change is much easier to maintain for me than all those
other options. I should have had that idea before. And I guess people
can live with symlinks from /sys/class/gpio/this-is-your-pin to
/sys/class/gpio/gpioX.

Guenter

Re: [RFC PATCH] gpiolib: Provide and export gpiod_export_name

[Alexandre Courbot]

On Thu, Aug 7, 2014 at 4:34 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>> Or you could have a platform device which sole purpose is to request
>> the GPIOs to export and export them using gpio_export() and
>> gpio_export_link() using itself as the device parameters. All your
>> links would then appear under the same sysfs directory, and that
>> directory name would be consistent across platforms. What is wrong
>> with this approach?
>>
> The link does not appear in /sys/class/gpio. I understand you don't like the
> idea of having named gpio pins in that directory, but that is supported
> today,
> it works, and others do like it.

Who are these "others" that like it?

>
>
>> As for the patch itself, as I said before I am not a huge fan of
>> putting randomly-named exports under /sys/class/gpio, but since there
>> is a precedent with the driver GPIO names array I don't think this
>> makes the situation much worse. Still I'd like you to explain me what
>> I missed and why you cannot use the technique described above to
>> achieve your goal with the currently existing interfaces.
>>
>
> I thought I explained it before; my users (ie the people writing user space
> applications accessing the pins) expect to see the exported pins in
> /sys/class/gpio
> and not in a more or less arbitrary directory. They are used to this
> approach, and they are less than enthusiastic to change it. The code needed
> to generate the exported pin names is a bit messy, it being tied to the
> driver,
> but it does both exist and work. This patch was an attempt to provide a
> cleaner API to accomplish the same without having to touch various gpio
> drivers which don't provide the ability to configure the names array.
>
> Note that I don't consider the names "random". They are much less random
> than gpioX, where X can change each time the system boots or, for OIR
> capable systems, each time a gpio driver is instantiated or removed.
> In today's system, without well defined names, one never knows if gpioX
> points
> to the pin the user is looking for. If the pin is named "this-is-your-pin"
> it is much easier to write user space code using it.
>
> Oddly enough, if I would use the platform driver approach you suggest,
> I would still need a well defined directory, say, /sys/class/named_gpios,

Which you would have through your platform device, and I guess you are
not denying that it is possible to do it that way using the existing
APIs. The discussion now seems to be "let's allow named GPIOs in
/sys/class/gpio". Why? Because the customer wants it this way. The
point is, that in mainline we don't merge changes because to make the
customer happy, but because they generally make sense. I fail to see
how it makes more sense to allow named GPIOs in /sys/class/gpio
instead of the node of the device controlled by these GPIOs.

Let me elaborate some more on why: GPIOs are generally tied to fulfil
a precise function for a given device. Having them appearing under the
node of the device in question makes it clear what their relationship
to that device is ; and the name of the node can then be a reflection
of that function. On the contrary, letting them all bloom under a
single directory forces you to resort to complex and confusing names
to differenciate your GPIOs, with everyone coming with their own
naming pattern and so on. If it doesn't look like a bad idea to you,
I'm afraid I cannot be any more convincing.

Also, it did not occur to me until now, but with this patch alone you
are going to be hit by the objection that we don't add APIs that do
not have upstream users. That policy is quite strongly enforced, and
I'm afraid this makes this patch even more unlikely to be accepted.

So my final word on this is that it seems quite clear that exporting
GPIOs under the device they control is a more elegant solution for
mainline, and a solution that is already doable using currently
existing APIs. I encourage you to explore that direction and work with
your customer to make them accept that small change ; if that is not
possible this will have to be maintained out of mainline ; sorry.

Re: [RFC PATCH] gpiolib: Provide and export gpiod_export_name

[Guenter Roeck]

On 08/11/2014 08:01 AM, Alexandre Courbot wrote:
> On Thu, Aug 7, 2014 at 4:34 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>>> Or you could have a platform device which sole purpose is to request
>>> the GPIOs to export and export them using gpio_export() and
>>> gpio_export_link() using itself as the device parameters. All your
>>> links would then appear under the same sysfs directory, and that
>>> directory name would be consistent across platforms. What is wrong
>>> with this approach?
>>>
>> The link does not appear in /sys/class/gpio. I understand you don't like the
>> idea of having named gpio pins in that directory, but that is supported
>> today,
>> it works, and others do like it.
>
> Who are these "others" that like it?
>
>>
>>
>>> As for the patch itself, as I said before I am not a huge fan of
>>> putting randomly-named exports under /sys/class/gpio, but since there
>>> is a precedent with the driver GPIO names array I don't think this
>>> makes the situation much worse. Still I'd like you to explain me what
>>> I missed and why you cannot use the technique described above to
>>> achieve your goal with the currently existing interfaces.
>>>
>>
>> I thought I explained it before; my users (ie the people writing user space
>> applications accessing the pins) expect to see the exported pins in
>> /sys/class/gpio
>> and not in a more or less arbitrary directory. They are used to this
>> approach, and they are less than enthusiastic to change it. The code needed
>> to generate the exported pin names is a bit messy, it being tied to the
>> driver,
>> but it does both exist and work. This patch was an attempt to provide a
>> cleaner API to accomplish the same without having to touch various gpio
>> drivers which don't provide the ability to configure the names array.
>>
>> Note that I don't consider the names "random". They are much less random
>> than gpioX, where X can change each time the system boots or, for OIR
>> capable systems, each time a gpio driver is instantiated or removed.
>> In today's system, without well defined names, one never knows if gpioX
>> points
>> to the pin the user is looking for. If the pin is named "this-is-your-pin"
>> it is much easier to write user space code using it.
>>
>> Oddly enough, if I would use the platform driver approach you suggest,
>> I would still need a well defined directory, say, /sys/class/named_gpios,
>
> Which you would have through your platform device, and I guess you are
> not denying that it is possible to do it that way using the existing
> APIs. The discussion now seems to be "let's allow named GPIOs in
> /sys/class/gpio". Why? Because the customer wants it this way. The
> point is, that in mainline we don't merge changes because to make the
> customer happy, but because they generally make sense. I fail to see
> how it makes more sense to allow named GPIOs in /sys/class/gpio
> instead of the node of the device controlled by these GPIOs.
>
> Let me elaborate some more on why: GPIOs are generally tied to fulfil
> a precise function for a given device. Having them appearing under the
> node of the device in question makes it clear what their relationship
> to that device is ; and the name of the node can then be a reflection
> of that function. On the contrary, letting them all bloom under a
> single directory forces you to resort to complex and confusing names
> to differenciate your GPIOs, with everyone coming with their own
> naming pattern and so on. If it doesn't look like a bad idea to you,
> I'm afraid I cannot be any more convincing.
>
> Also, it did not occur to me until now, but with this patch alone you
> are going to be hit by the objection that we don't add APIs that do
> not have upstream users. That policy is quite strongly enforced, and
> I'm afraid this makes this patch even more unlikely to be accepted.
>
This is just one of many patches which would make it possible to submit
the rest, which would make use of it. What you are saying is that it won't
make sense to submit that series into the kernel, because one of the very
first patches needed to enable that won't be accepted. Kind of a circular
argument, but I guess I'll have to live with it.

> So my final word on this is that it seems quite clear that exporting
> GPIOs under the device they control is a more elegant solution for
> mainline, and a solution that is already doable using currently
> existing APIs. I encourage you to explore that direction and work with
> your customer to make them accept that small change ; if that is not
> possible this will have to be maintained out of mainline ; sorry.
>

Guess so.

Guenter

Re: [RFC PATCH] gpiolib: Provide and export gpiod_export_name

[Alexandre Courbot]

On Tue, Aug 12, 2014 at 12:15 AM, Guenter Roeck <linux@roeck-us.net> wrote:
> On 08/11/2014 08:01 AM, Alexandre Courbot wrote:
>>
>> On Thu, Aug 7, 2014 at 4:34 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>>>>
>>>> Or you could have a platform device which sole purpose is to request
>>>> the GPIOs to export and export them using gpio_export() and
>>>> gpio_export_link() using itself as the device parameters. All your
>>>> links would then appear under the same sysfs directory, and that
>>>> directory name would be consistent across platforms. What is wrong
>>>> with this approach?
>>>>
>>> The link does not appear in /sys/class/gpio. I understand you don't like
>>> the
>>> idea of having named gpio pins in that directory, but that is supported
>>> today,
>>> it works, and others do like it.
>>
>>
>> Who are these "others" that like it?
>>
>>>
>>>
>>>> As for the patch itself, as I said before I am not a huge fan of
>>>> putting randomly-named exports under /sys/class/gpio, but since there
>>>> is a precedent with the driver GPIO names array I don't think this
>>>> makes the situation much worse. Still I'd like you to explain me what
>>>> I missed and why you cannot use the technique described above to
>>>> achieve your goal with the currently existing interfaces.
>>>>
>>>
>>> I thought I explained it before; my users (ie the people writing user
>>> space
>>> applications accessing the pins) expect to see the exported pins in
>>> /sys/class/gpio
>>> and not in a more or less arbitrary directory. They are used to this
>>> approach, and they are less than enthusiastic to change it. The code
>>> needed
>>> to generate the exported pin names is a bit messy, it being tied to the
>>> driver,
>>> but it does both exist and work. This patch was an attempt to provide a
>>> cleaner API to accomplish the same without having to touch various gpio
>>> drivers which don't provide the ability to configure the names array.
>>>
>>> Note that I don't consider the names "random". They are much less random
>>> than gpioX, where X can change each time the system boots or, for OIR
>>> capable systems, each time a gpio driver is instantiated or removed.
>>> In today's system, without well defined names, one never knows if gpioX
>>> points
>>> to the pin the user is looking for. If the pin is named
>>> "this-is-your-pin"
>>> it is much easier to write user space code using it.
>>>
>>> Oddly enough, if I would use the platform driver approach you suggest,
>>> I would still need a well defined directory, say, /sys/class/named_gpios,
>>
>>
>> Which you would have through your platform device, and I guess you are
>> not denying that it is possible to do it that way using the existing
>> APIs. The discussion now seems to be "let's allow named GPIOs in
>> /sys/class/gpio". Why? Because the customer wants it this way. The
>> point is, that in mainline we don't merge changes because to make the
>> customer happy, but because they generally make sense. I fail to see
>> how it makes more sense to allow named GPIOs in /sys/class/gpio
>> instead of the node of the device controlled by these GPIOs.
>>
>> Let me elaborate some more on why: GPIOs are generally tied to fulfil
>> a precise function for a given device. Having them appearing under the
>> node of the device in question makes it clear what their relationship
>> to that device is ; and the name of the node can then be a reflection
>> of that function. On the contrary, letting them all bloom under a
>> single directory forces you to resort to complex and confusing names
>> to differenciate your GPIOs, with everyone coming with their own
>> naming pattern and so on. If it doesn't look like a bad idea to you,
>> I'm afraid I cannot be any more convincing.
>>
>> Also, it did not occur to me until now, but with this patch alone you
>> are going to be hit by the objection that we don't add APIs that do
>> not have upstream users. That policy is quite strongly enforced, and
>> I'm afraid this makes this patch even more unlikely to be accepted.
>>
> This is just one of many patches which would make it possible to submit
> the rest, which would make use of it. What you are saying is that it won't
> make sense to submit that series into the kernel, because one of the very
> first patches needed to enable that won't be accepted. Kind of a circular
> argument, but I guess I'll have to live with it.

Well I have not seen the other patches you mention and cannot guess
their existence. If you send the full series it will of course be
considered as such, but right now this lone patch does not hint any
upstream user for this interface.

Note that this doesn't change anything to the core of the argument ;
we have not heard what Linus thinks about named GPIOs in
/sys/class/gpio yet, maybe he will have a different opinion...

Re: [RFC PATCH] gpiolib: Provide and export gpiod_export_name

[Guenter Roeck]

On 08/11/2014 08:20 AM, Alexandre Courbot wrote:
[ ... ]
>
> Note that this doesn't change anything to the core of the argument ;
> we have not heard what Linus thinks about named GPIOs in
> /sys/class/gpio yet, maybe he will have a different opinion...
>

Well, please let me know if/when you are planning to take away
that existing ABI so I can plan accordingly. FWIW, out-of-kernel
users I am currently aware of are Juniper Networks and Zodiac
Aerospace. The latter asked me to help them upstreaming their code.

If you plan to remove the ABI, you might want to inform its current
in-kernel users. You can look those up yourself.

Thanks,
Guenter

Re: [RFC PATCH] gpiolib: Provide and export gpiod_export_name

[Alexandre Courbot]

On Mon, Aug 11, 2014 at 9:13 AM, Guenter Roeck <linux@roeck-us.net> wrote:
> On 08/11/2014 08:20 AM, Alexandre Courbot wrote:
> [ ... ]
>
>>
>> Note that this doesn't change anything to the core of the argument ;
>> we have not heard what Linus thinks about named GPIOs in
>> /sys/class/gpio yet, maybe he will have a different opinion...
>>
>
> Well, please let me know if/when you are planning to take away
> that existing ABI so I can plan accordingly. FWIW, out-of-kernel
> users I am currently aware of are Juniper Networks and Zodiac
> Aerospace. The latter asked me to help them upstreaming their code.
>
> If you plan to remove the ABI, you might want to inform its current
> in-kernel users. You can look those up yourself.

We are not taking existing user-space ABIs away. But we are also not
encouraging people to use bad ABIs by introducing new ways to use
them, hence my restraint towards this patch.

Re: [RFC PATCH] gpiolib: Provide and export gpiod_export_name

[Guenter Roeck]

On Mon, Aug 11, 2014 at 03:05:54PM -0700, Alexandre Courbot wrote:
> On Mon, Aug 11, 2014 at 9:13 AM, Guenter Roeck <linux@roeck-us.net> wrote:
> > On 08/11/2014 08:20 AM, Alexandre Courbot wrote:
> > [ ... ]
> >
> >>
> >> Note that this doesn't change anything to the core of the argument ;
> >> we have not heard what Linus thinks about named GPIOs in
> >> /sys/class/gpio yet, maybe he will have a different opinion...
> >>
> >
> > Well, please let me know if/when you are planning to take away
> > that existing ABI so I can plan accordingly. FWIW, out-of-kernel
> > users I am currently aware of are Juniper Networks and Zodiac
> > Aerospace. The latter asked me to help them upstreaming their code.
> >
> > If you plan to remove the ABI, you might want to inform its current
> > in-kernel users. You can look those up yourself.
> 
> We are not taking existing user-space ABIs away. But we are also not
> encouraging people to use bad ABIs by introducing new ways to use
> them, hence my restraint towards this patch.
> 

You bring it to the point. You believe that the gpio ABI, or maybe just
named gpio pins, is a bad idea. Nothing I am going to say will influence
that opinion; all we do is to keep turning in circles.

So let's stop wasting our time, agree to disagree, and move on.

Thanks,
Guenter

Re: [RFC PATCH] gpiolib: Provide and export gpiod_export_name

[Linus Walleij]

On Mon, Aug 11, 2014 at 5:20 PM, Alexandre Courbot <gnurou@gmail.com> wrote:
> On Tue, Aug 12, 2014 at 12:15 AM, Guenter Roeck <linux@roeck-us.net> wrote:

>> This is just one of many patches which would make it possible to submit
>> the rest, which would make use of it. What you are saying is that it won't
>> make sense to submit that series into the kernel, because one of the very
>> first patches needed to enable that won't be accepted. Kind of a circular
>> argument, but I guess I'll have to live with it.
>
> Well I have not seen the other patches you mention and cannot guess
> their existence. If you send the full series it will of course be
> considered as such, but right now this lone patch does not hint any
> upstream user for this interface.
>
> Note that this doesn't change anything to the core of the argument ;
> we have not heard what Linus thinks about named GPIOs in
> /sys/class/gpio yet, maybe he will have a different opinion...

The sysfs is sort of broken by design because of things like this and
some other stuff.

I think the sysfs is scary, for example since it's not hierarchical
but flat and build on the assumption that there is one single
GPIO numberspace. As pointed out in some other message
in the thread it would be nicer to have:

/dev/gpiochip0/gpio0
/dev/gpiochip0/gpio1
....

instead of the horrid sysfs ABI that will have to maintain forever.

Still it is true that there is a precedent for named GPIOs in sysfs.

And in the end, userspace needs a way to figure out how to
get what it needs, a unique string is as good as anything.
I would be feeling better if userspace got that name from an
ioctl() on some /dev/* node than by plain filename search in
sysfs. I somewhat feel a named gpio in sysfs is more
helpful than just "gpio25".

But I'm not happy about merging the patch if Alexandre is
hesitant.

Yours,
Linus Walleij

Re: [RFC PATCH] gpiolib: Provide and export gpiod_export_name

[Guenter Roeck]

On 08/28/2014 10:44 PM, Linus Walleij wrote:
> On Mon, Aug 11, 2014 at 5:20 PM, Alexandre Courbot <gnurou@gmail.com> wrote:
>> On Tue, Aug 12, 2014 at 12:15 AM, Guenter Roeck <linux@roeck-us.net> wrote:
>
>>> This is just one of many patches which would make it possible to submit
>>> the rest, which would make use of it. What you are saying is that it won't
>>> make sense to submit that series into the kernel, because one of the very
>>> first patches needed to enable that won't be accepted. Kind of a circular
>>> argument, but I guess I'll have to live with it.
>>
>> Well I have not seen the other patches you mention and cannot guess
>> their existence. If you send the full series it will of course be
>> considered as such, but right now this lone patch does not hint any
>> upstream user for this interface.
>>
>> Note that this doesn't change anything to the core of the argument ;
>> we have not heard what Linus thinks about named GPIOs in
>> /sys/class/gpio yet, maybe he will have a different opinion...
>
> The sysfs is sort of broken by design because of things like this and
> some other stuff.
>
> I think the sysfs is scary, for example since it's not hierarchical
> but flat and build on the assumption that there is one single
> GPIO numberspace. As pointed out in some other message
> in the thread it would be nicer to have:
>
> /dev/gpiochip0/gpio0
> /dev/gpiochip0/gpio1
> ....
>
> instead of the horrid sysfs ABI that will have to maintain forever.
>
Blocking any attempts to make it more useful doesn't help much, though.

> Still it is true that there is a precedent for named GPIOs in sysfs.
>
> And in the end, userspace needs a way to figure out how to
> get what it needs, a unique string is as good as anything.
> I would be feeling better if userspace got that name from an
> ioctl() on some /dev/* node than by plain filename search in
> sysfs. I somewhat feel a named gpio in sysfs is more
> helpful than just "gpio25".
>

That was the point. The code necessary to find out that a specific
function on a specific board is tied to gpiochip X pin Y is quite
complex. It seemed to be much easier to be able to rely on a well
defined pin name.

The argument of "there can be duplicate names" is, in my opinion,
quite bogus. Sure, there can be duplicate names. That is why one
has error codes. With such an argument you can block anything
you want.

Regarding ioctls, I thought sysfs was supposed to replace ioctls,
and that adding new ioctls was discouraged. Are we reverting on that ?

> But I'm not happy about merging the patch if Alexandre is
> hesitant.
>

Guess that's life. I'll just have to live with it.

Guenter

Re: [RFC PATCH] gpiolib: Provide and export gpiod_export_name

[Alexandre Courbot]

On Thu, Aug 28, 2014 at 10:54 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On 08/28/2014 10:44 PM, Linus Walleij wrote:
>>
>> On Mon, Aug 11, 2014 at 5:20 PM, Alexandre Courbot <gnurou@gmail.com>
>> wrote:
>>>
>>> On Tue, Aug 12, 2014 at 12:15 AM, Guenter Roeck <linux@roeck-us.net>
>>> wrote:
>>
>>
>>>> This is just one of many patches which would make it possible to submit
>>>> the rest, which would make use of it. What you are saying is that it
>>>> won't
>>>> make sense to submit that series into the kernel, because one of the
>>>> very
>>>> first patches needed to enable that won't be accepted. Kind of a
>>>> circular
>>>> argument, but I guess I'll have to live with it.
>>>
>>>
>>> Well I have not seen the other patches you mention and cannot guess
>>> their existence. If you send the full series it will of course be
>>> considered as such, but right now this lone patch does not hint any
>>> upstream user for this interface.
>>>
>>> Note that this doesn't change anything to the core of the argument ;
>>> we have not heard what Linus thinks about named GPIOs in
>>> /sys/class/gpio yet, maybe he will have a different opinion...
>>
>>
>> The sysfs is sort of broken by design because of things like this and
>> some other stuff.
>>
>> I think the sysfs is scary, for example since it's not hierarchical
>> but flat and build on the assumption that there is one single
>> GPIO numberspace. As pointed out in some other message
>> in the thread it would be nicer to have:
>>
>> /dev/gpiochip0/gpio0
>> /dev/gpiochip0/gpio1
>> ....
>>
>> instead of the horrid sysfs ABI that will have to maintain forever.
>>
> Blocking any attempts to make it more useful doesn't help much, though.

This patch is not making it more useful. It just introduces an
inferior way to do something that is already possible.

I have stated it countless times already, but again:
"/sys/bus/.../device/gpio_function" is better than
"/sys/class/gpio/device_function_foo_whatnot" (and actually,
"/sys/bus/.../device/gpios/function" would be even better).

You can already do the former. I just don't see the need to introduce
an API to do the latter.

Why is the former better? Because it uses the sysfs hierarchy to make
the GPIO visible under their consumer's node. That's how sysfs is
intended to be used.

Just explain me why you cannot live with this or why your proposal is
better, and my concerns about this patch will be lifted.

Re: [RFC PATCH] gpiolib: Provide and export gpiod_export_name

[Guenter Roeck]

On Fri, Aug 29, 2014 at 10:00:47AM -0700, Alexandre Courbot wrote:
> On Thu, Aug 28, 2014 at 10:54 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> > On 08/28/2014 10:44 PM, Linus Walleij wrote:
> >>
> >> On Mon, Aug 11, 2014 at 5:20 PM, Alexandre Courbot <gnurou@gmail.com>
> >> wrote:
> >>>
> >>> On Tue, Aug 12, 2014 at 12:15 AM, Guenter Roeck <linux@roeck-us.net>
> >>> wrote:
> >>
> >>
> >>>> This is just one of many patches which would make it possible to submit
> >>>> the rest, which would make use of it. What you are saying is that it
> >>>> won't
> >>>> make sense to submit that series into the kernel, because one of the
> >>>> very
> >>>> first patches needed to enable that won't be accepted. Kind of a
> >>>> circular
> >>>> argument, but I guess I'll have to live with it.
> >>>
> >>>
> >>> Well I have not seen the other patches you mention and cannot guess
> >>> their existence. If you send the full series it will of course be
> >>> considered as such, but right now this lone patch does not hint any
> >>> upstream user for this interface.
> >>>
> >>> Note that this doesn't change anything to the core of the argument ;
> >>> we have not heard what Linus thinks about named GPIOs in
> >>> /sys/class/gpio yet, maybe he will have a different opinion...
> >>
> >>
> >> The sysfs is sort of broken by design because of things like this and
> >> some other stuff.
> >>
> >> I think the sysfs is scary, for example since it's not hierarchical
> >> but flat and build on the assumption that there is one single
> >> GPIO numberspace. As pointed out in some other message
> >> in the thread it would be nicer to have:
> >>
> >> /dev/gpiochip0/gpio0
> >> /dev/gpiochip0/gpio1
> >> ....
> >>
> >> instead of the horrid sysfs ABI that will have to maintain forever.
> >>
> > Blocking any attempts to make it more useful doesn't help much, though.
> 
> This patch is not making it more useful. It just introduces an
> inferior way to do something that is already possible.
> 
> I have stated it countless times already, but again:
> "/sys/bus/.../device/gpio_function" is better than
> "/sys/class/gpio/device_function_foo_whatnot" (and actually,
> "/sys/bus/.../device/gpios/function" would be even better).
> 
> You can already do the former. I just don't see the need to introduce
> an API to do the latter.
> 
> Why is the former better? Because it uses the sysfs hierarchy to make
> the GPIO visible under their consumer's node. That's how sysfs is
> intended to be used.
> 
> Just explain me why you cannot live with this or why your proposal is
> better, and my concerns about this patch will be lifted.
> 

"device" is a platform driver and thus platform specific, which defeats
the purpose of having a well defined path and makes it even more difficult
to find a pin across multiple platform variants (which will have different
platform drivers). It means user space will still need platform specific
configuration data to find the pin. If user space needs such configuration
data anyway, it may as well be a "<gpio chip, pin>" tuple instead of
"<platform driver path, gpio pin name>". Even worse, in one of my use
cases some of the platform drivers may have multiple instances with dynamic
instance numbers, meaning I don't even have a well defined path to start with.

On the other side, as it turns out, most of the gpio chips I deal with are
company specific, and you can not prevent me from populating the 'names'
array for those. It is a bit on the clumsy side, as I would prefer to have
the consumer select the name instead of the provider, but it works. For the
other drivers (so far only one) I only need an out-of-tree patch with a
couple of lines of code to be able to populate the 'names' field. So unless
you take the 'names' field away, which would break the existing ABI and is
thus at least somewhat unlikely, I can still do what I need to do.

Guenter

Re: [RFC PATCH] gpiolib: Provide and export gpiod_export_name

[Alexandre Courbot]

On Fri, Aug 29, 2014 at 10:37 AM, Guenter Roeck <linux@roeck-us.net> wrote:
> On Fri, Aug 29, 2014 at 10:00:47AM -0700, Alexandre Courbot wrote:
>> On Thu, Aug 28, 2014 at 10:54 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>> > On 08/28/2014 10:44 PM, Linus Walleij wrote:
>> >>
>> >> On Mon, Aug 11, 2014 at 5:20 PM, Alexandre Courbot <gnurou@gmail.com>
>> >> wrote:
>> >>>
>> >>> On Tue, Aug 12, 2014 at 12:15 AM, Guenter Roeck <linux@roeck-us.net>
>> >>> wrote:
>> >>
>> >>
>> >>>> This is just one of many patches which would make it possible to submit
>> >>>> the rest, which would make use of it. What you are saying is that it
>> >>>> won't
>> >>>> make sense to submit that series into the kernel, because one of the
>> >>>> very
>> >>>> first patches needed to enable that won't be accepted. Kind of a
>> >>>> circular
>> >>>> argument, but I guess I'll have to live with it.
>> >>>
>> >>>
>> >>> Well I have not seen the other patches you mention and cannot guess
>> >>> their existence. If you send the full series it will of course be
>> >>> considered as such, but right now this lone patch does not hint any
>> >>> upstream user for this interface.
>> >>>
>> >>> Note that this doesn't change anything to the core of the argument ;
>> >>> we have not heard what Linus thinks about named GPIOs in
>> >>> /sys/class/gpio yet, maybe he will have a different opinion...
>> >>
>> >>
>> >> The sysfs is sort of broken by design because of things like this and
>> >> some other stuff.
>> >>
>> >> I think the sysfs is scary, for example since it's not hierarchical
>> >> but flat and build on the assumption that there is one single
>> >> GPIO numberspace. As pointed out in some other message
>> >> in the thread it would be nicer to have:
>> >>
>> >> /dev/gpiochip0/gpio0
>> >> /dev/gpiochip0/gpio1
>> >> ....
>> >>
>> >> instead of the horrid sysfs ABI that will have to maintain forever.
>> >>
>> > Blocking any attempts to make it more useful doesn't help much, though.
>>
>> This patch is not making it more useful. It just introduces an
>> inferior way to do something that is already possible.
>>
>> I have stated it countless times already, but again:
>> "/sys/bus/.../device/gpio_function" is better than
>> "/sys/class/gpio/device_function_foo_whatnot" (and actually,
>> "/sys/bus/.../device/gpios/function" would be even better).
>>
>> You can already do the former. I just don't see the need to introduce
>> an API to do the latter.
>>
>> Why is the former better? Because it uses the sysfs hierarchy to make
>> the GPIO visible under their consumer's node. That's how sysfs is
>> intended to be used.
>>
>> Just explain me why you cannot live with this or why your proposal is
>> better, and my concerns about this patch will be lifted.
>>
>
> "device" is a platform driver and thus platform specific, which defeats
> the purpose of having a well defined path and makes it even more difficult
> to find a pin across multiple platform variants (which will have different
> platform drivers). It means user space will still need platform specific
> configuration data to find the pin. If user space needs such configuration
> data anyway, it may as well be a "<gpio chip, pin>" tuple instead of
> "<platform driver path, gpio pin name>". Even worse, in one of my use
> cases some of the platform drivers may have multiple instances with dynamic
> instance numbers, meaning I don't even have a well defined path to start with.
>
> On the other side, as it turns out, most of the gpio chips I deal with are
> company specific, and you can not prevent me from populating the 'names'
> array for those. It is a bit on the clumsy side, as I would prefer to have
> the consumer select the name instead of the provider, but it works. For the
> other drivers (so far only one) I only need an out-of-tree patch with a
> couple of lines of code to be able to populate the 'names' field. So unless
> you take the 'names' field away, which would break the existing ABI and is
> thus at least somewhat unlikely, I can still do what I need to do.

We won't take the names field away, as you mentioned we will have to
support the user-space ABI forever and ever. I hope that at some point
we will come with a better alternative, but the old one is here to
stay anyway. Linus and myself explained why this ABI is bad ; however,
it is also true that there is no better alternative besides
implementing a custom solution.

Which allows us to reduce the argument about your patch to: "it's a
more comfortable way of doing something bad". Since that something bad
is not going away, we might as well have another (arguably better) way
of doing it.

Also:
- This patch is limited to gpiolib-sysfs.c, so damage is contained there
- The patch is small
- I cannot help but notice that Linus did not scream about it
- I did not have a strong reject about it either, but was looking for
a better way to achieve the desired  result. Looks like there is none
at the moment.

So I'm somehow ok if this patch makes it in ; I still think we should
not encourage usage of this ABI, but you do not introduce any new
harmful toy - just provide another way to use one that already exists.

Linus, please consider my position about this patch as neutral, and
feel free to take it if you think it is worth.

/me thinks about moving the sysfs interface declaration into its own
header file so as to not mix the pure and the impure... :)

Re: [RFC PATCH] gpiolib: Provide and export gpiod_export_name

[Linus Walleij]

On Sun, Aug 31, 2014 at 2:57 AM, Alexandre Courbot <gnurou@gmail.com> wrote:

> Linus, please consider my position about this patch as neutral, and
> feel free to take it if you think it is worth.

I will meditate on this.

Yours,
Linus Walleij