Suggestion on GPIO sysfs interface (gpio_export)

Suggestion on GPIO sysfs interface (gpio_export)

[Kim Kyuwon]

Hi All,

Exporting GPIOs by using gpio_export() is very useful to me.
But I want to access each GPIO by its name(or label) instead of
GPIO number, because GPIO label is more descriptive.

So I just modified gpio_export() to show the label information as shown below.
(This patch just shows the idea)

==
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 42fb2fd..392303d 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -363,7 +363,7 @@ static ssize_t export_store(struct class *class, const char *buf, size_t len)
 	if (status < 0)
 		goto done;
 
-	status = gpio_export(gpio, true);
+	status = gpio_export(gpio, true, false);
 	if (status < 0)
 		gpio_free(gpio);
 	else
@@ -422,6 +422,7 @@ static struct class gpio_class = {
  * gpio_export - export a GPIO through sysfs
  * @gpio: gpio to make available, already requested
  * @direction_may_change: true if userspace may change gpio direction
+ * @label_may_show: true if gpio label may show, instead of gpio number
  * Context: arch_initcall or later
  *
  * When drivers want to make a GPIO accessible to userspace after they
@@ -433,7 +434,7 @@ static struct class gpio_class = {
  *
  * Returns zero on success, else an error.
  */
-int gpio_export(unsigned gpio, bool direction_may_change)
+int gpio_export(unsigned gpio, bool direction_may_change, bool label_may_show)
 {
 	unsigned long		flags;
 	struct gpio_desc	*desc;
@@ -464,8 +465,15 @@ int gpio_export(unsigned gpio, bool direction_may_change)
 	if (status == 0) {
 		struct device	*dev;
 
-		dev = device_create(&gpio_class, desc->chip->dev, MKDEV(0, 0),
-					desc, "gpio%d", gpio);
+#ifdef CONFIG_DEBUG_FS
+		if (label_may_show && desc->label)
+			dev = device_create(&gpio_class, desc->chip->dev,
+				MKDEV(0, 0), desc, "%s", desc->label);
+		else
+#endif
+			dev = device_create(&gpio_class, desc->chip->dev,
+				MKDEV(0, 0), desc, "gpio%d", gpio);
+
 		if (dev) {
 			if (direction_may_change)
 				status = sysfs_create_group(&dev->kobj,
diff --git a/drivers/mfd/dm355evm_msp.c b/drivers/mfd/dm355evm_msp.c
index 7ac12cb..4a37ff1 100644
--- a/drivers/mfd/dm355evm_msp.c
+++ b/drivers/mfd/dm355evm_msp.c
@@ -304,7 +304,7 @@ static int add_children(struct i2c_client *client)
 		gpio_direction_input(gpio);
 
 		/* make it easy for userspace to see these */
-		gpio_export(gpio, false);
+		gpio_export(gpio, false, false);
 	}
 
 	/* MMC/SD inputs -- right after the last config input */
diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
index 81797ec..9852da4 100644
--- a/include/asm-generic/gpio.h
+++ b/include/asm-generic/gpio.h
@@ -135,7 +135,8 @@ extern int __gpio_to_irq(unsigned gpio);
  * A sysfs interface can be exported by individual drivers if they want,
  * but more typically is configured entirely from userspace.
  */
-extern int gpio_export(unsigned gpio, bool direction_may_change);
+extern int gpio_export(unsigned gpio, bool direction_may_change,
+							bool label_may_show);
 extern void gpio_unexport(unsigned gpio);
 
 #endif	/* CONFIG_GPIO_SYSFS */
@@ -175,7 +176,8 @@ static inline void gpio_set_value_cansleep(unsigned gpio, int value)
 
 /* sysfs support is only available with gpiolib, where it's optional */
 
-static inline int gpio_export(unsigned gpio, bool direction_may_change)
+static inline int gpio_export(unsigned gpio, bool direction_may_change,
+							bool label_may_show)
 {
 	return -ENOSYS;
 }
diff --git a/include/linux/gpio.h b/include/linux/gpio.h
index e10c49a..1209149 100644
--- a/include/linux/gpio.h
+++ b/include/linux/gpio.h
@@ -82,7 +82,8 @@ static inline void gpio_set_value_cansleep(unsigned gpio, int value)
 	WARN_ON(1);
 }
 
-static inline int gpio_export(unsigned gpio, bool direction_may_change)
+static inline int gpio_export(unsigned gpio, bool direction_may_change,
+							bool label_may_show)
 {
 	/* GPIO can never have been requested or set as {in,out}put */
 	WARN_ON(1);
==

Can I ask you opinion about this idea?
If I get the positive answer, I will send the full patch set which
changes all board files and documentation related to gpio_export.

Regards,
Kim Kyuwon

Re: Suggestion on GPIO sysfs interface (gpio_export)

[David Brownell]

On Sunday 19 April 2009, Kim Kyuwon wrote:
> Can I ask you opinion about this idea?

First issue:  labels aren't required to be unique, so
there's a certain level of unpredictability you're
introducing.  Exports using this new flag would fail
sometimes depending on what *other* exports did.
(That's part of the reason "gpio%d" names got used
in the first place!)

Another layer of unpredicatability comes from the
way those strings are only available given debugfs.


Second:

> -extern int gpio_export(unsigned gpio, bool direction_may_change);
> +extern int gpio_export(unsigned gpio, bool direction_may_change,
> +                                                       bool label_may_show);

It's generally confusing to add more parameters of the same type
like that; there's no fundamental reason for people to remember
which one means what, and the compiler can't help at all when (!)
they get confused.


Have you thought much about other options?  Like for example
adding a new call.  With each MMC card slot, for one example,
there would often be two GPIOs:  card_detect, write_protect.
With two such slots, the "label" wouldn't be much help unless
it were specifically made unique.

Instead of exporting the GPIOs in a "flat" namespace, maybe
something like

  gpio_export_dev(struct device *dev, const char *tag,
		unsigned gpio, bool direction_may_change);

would be more useful.  It could gpio_export() the standard
way, then set up a symlink using "tag" to set up a symlink
from /sys/.../dev/tag to /sys/class/gpio/gpioN node.  Easy
to see how that would work for those MMC examples.

I can imagine there would be times when the GPIO doesn't
have a logical coupling to any device, of course.  So maybe
that doesn't address your particular issue.

- Dave

Re: Suggestion on GPIO sysfs interface (gpio_export)

[Kim Kyuwon]

On Tue, Apr 21, 2009 at 1:52 AM, David Brownell <david-b@pacbell.net> wrote:
> On Sunday 19 April 2009, Kim Kyuwon wrote:
>> Can I ask you opinion about this idea?
>
> First issue:  labels aren't required to be unique, so
> there's a certain level of unpredictability you're
> introducing.  Exports using this new flag would fail
> sometimes depending on what *other* exports did.
> (That's part of the reason "gpio%d" names got used
> in the first place!)
>
> Another layer of unpredicatability comes from the
> way those strings are only available given debugfs.
>
>
> Second:
>
>> -extern int gpio_export(unsigned gpio, bool direction_may_change);
>> +extern int gpio_export(unsigned gpio, bool direction_may_change,
>> +                                                       bool label_may_show);
>
> It's generally confusing to add more parameters of the same type
> like that; there's no fundamental reason for people to remember
> which one means what, and the compiler can't help at all when (!)
> they get confused.

Thank you for pointing out problems :)

> Have you thought much about other options?  Like for example
> adding a new call.  With each MMC card slot, for one example,
> there would often be two GPIOs:  card_detect, write_protect.
> With two such slots, the "label" wouldn't be much help unless
> it were specifically made unique.
>
> Instead of exporting the GPIOs in a "flat" namespace, maybe
> something like
>
>  gpio_export_dev(struct device *dev, const char *tag,
>                unsigned gpio, bool direction_may_change);
>
> would be more useful.  It could gpio_export() the standard
> way, then set up a symlink using "tag" to set up a symlink
> from /sys/.../dev/tag to /sys/class/gpio/gpioN node.  Easy
> to see how that would work for those MMC examples.
>
> I can imagine there would be times when the GPIO doesn't
> have a logical coupling to any device, of course.  So maybe
> that doesn't address your particular issue.

Yes, we have a few devices which can be controlled by only 1 GPIOs. I
thought it is too small to make new drivers for these devices. So I
just tried to use gpio_export() function.
hmm.. I have to make do with new platform drivers for 1-GPIO controlled devices.
Anyway, thank you for your tip!

> - Dave
>

-- 
Kyuwon (규원)