[patch/rfc 2.6.25-git] gpio: sysfs interface

[patch/rfc 2.6.25-git] gpio: sysfs interface

[David Brownell]

Simple sysfs interface for GPIOs.

    /sys/class/gpio
        /gpio-N ... for each exported GPIO #N
	    /value ... always readable, writes fail except for output GPIOs
	    /direction ... writable as: in, out (default low), high, low
    	/control ... to request a GPIO be exported or unexported

GPIOs may be exported by kernel code using gpio_export(), which should
be most useful for driver debugging.  Userspace may also ask that they
be exported by writing to the sysfs control file, helping to cope with
incomplete board support:

  echo "export 23" > /sys/class/gpio/control
	... will gpio_request(23, "sysfs") and gpio_export(23); use
	/sys/class/gpio/gpio-23/direction to configure it.
  echo "unexport 23" > /sys/class/gpio/control
	... will gpio_free(23)

The D-space footprint is negligible, except for the sysfs resources
associated with each exported GPIO.  The additional I-space footprint
is about half of the current size of gpiolib.  No /dev node creation
involved, and no "udev" support is needed.

This adds a device pointer to "struct gpio_chip".  When GPIO providers
initialize that, sysfs gpio class devices become children of that device
instead of being "virtual" devices.  The (few) gpio_chip providers which
have such a device node have been updated.  (Some also needed to update
their module "owner" field ... for which missing kerneldoc was added.)

Based on a patch from Trent Piepho <tpiepho@freescale.com>, and comments
from various folk including Hartley Sweeten.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
 arch/avr32/mach-at32ap/pio.c |    2 
 drivers/gpio/Kconfig         |   16 ++
 drivers/gpio/gpiolib.c       |  281 +++++++++++++++++++++++++++++++++++++++++++
 drivers/gpio/mcp23s08.c      |    1 
 drivers/gpio/pca953x.c       |    1 
 drivers/gpio/pcf857x.c       |    1 
 drivers/i2c/chips/tps65010.c |    2 
 drivers/mfd/htc-egpio.c      |    2 
 include/asm-generic/gpio.h   |   28 ++++
 9 files changed, 333 insertions(+), 1 deletion(-)

--- g26.orig/arch/avr32/mach-at32ap/pio.c	2008-04-28 11:11:07.000000000 -0700
+++ g26/arch/avr32/mach-at32ap/pio.c	2008-04-28 11:31:21.000000000 -0700
@@ -358,6 +358,8 @@ static int __init pio_probe(struct platf
 	pio->chip.label = pio->name;
 	pio->chip.base = pdev->id * 32;
 	pio->chip.ngpio = 32;
+	pio->chip.dev = &pdev->dev;
+	pio->chip.owner = THIS_MODULE;
 
 	pio->chip.direction_input = direction_input;
 	pio->chip.get = gpio_get;
--- g26.orig/drivers/gpio/Kconfig	2008-04-28 11:11:08.000000000 -0700
+++ g26/drivers/gpio/Kconfig	2008-04-28 11:46:57.000000000 -0700
@@ -23,6 +23,22 @@ config DEBUG_GPIO
 	  slower.  The diagnostics help catch the type of setup errors
 	  that are most common when setting up new platforms or boards.
 
+config GPIO_SYSFS
+	bool "/sys/class/gpio/... (EXPERIMENTAL sysfs interface)"
+	depends on SYSFS && EXPERIMENTAL
+	help
+	  Say Y here to add a sysfs interface for GPIOs.
+
+	  This is mostly useful to work around omissions in a system's
+	  kernel support.  Those are common in custom and semicustom
+	  hardware assembled using standard kernels with a minimum of
+	  custom patches.  In those cases, userspace code may ask that
+	  a given GPIO be exported, if no kernel driver requested it.
+
+	  Kernel drivers may also request that a particular GPIO be
+	  made available to userspace as well as that driver.  This
+	  can be useful when debugging.
+
 # put expanders in the right section, in alphabetical order
 
 comment "I2C GPIO expanders:"
--- g26.orig/drivers/gpio/gpiolib.c	2008-04-28 11:17:42.000000000 -0700
+++ g26/drivers/gpio/gpiolib.c	2008-04-28 11:40:36.000000000 -0700
@@ -44,6 +44,8 @@ struct gpio_desc {
 #define FLAG_REQUESTED	0
 #define FLAG_IS_OUT	1
 #define FLAG_RESERVED	2
+#define FLAG_EXPORT	3
+#define FLAG_SYSFS	4
 
 #ifdef CONFIG_DEBUG_FS
 	const char		*label;
@@ -286,6 +288,283 @@ done:
 }
 EXPORT_SYMBOL_GPL(gpio_request);
 
+
+#ifdef CONFIG_GPIO_SYSFS
+
+/*
+ * /sys/class/gpio/gpio-N/... only for GPIOs that are exported
+ *  - direction
+ *      * is read/write as in/out
+ *      * may also be written as high/low, initializing output
+ *        value as specified (plain "out" implies "low")
+ *  - value
+ *      * always readable, subject to hardware behavior
+ *      * may be writable, as zero/nonzero
+ */
+
+#include <linux/device.h>
+#include <linux/err.h>
+
+static ssize_t gpio_direction_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	const struct gpio_desc	*desc = dev_get_drvdata(dev);
+
+	/* handle GPIOs being removed from underneath us... */
+	if (!test_bit(FLAG_EXPORT, &desc->flags))
+		return -EIO;
+
+	return sprintf(buf, "%s\n",
+		test_bit(FLAG_IS_OUT, &desc->flags) ? "out" : "in");
+}
+
+static ssize_t gpio_direction_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t size)
+{
+	const struct gpio_desc	*desc = dev_get_drvdata(dev);
+	unsigned		gpio = desc - gpio_desc;
+	unsigned		len = size;
+	ssize_t			status = -EINVAL;
+
+	/* handle GPIOs being removed from underneath us... */
+	if (!test_bit(FLAG_EXPORT, &desc->flags))
+		return -EIO;
+
+	if (buf[len - 1] == '\n')
+		len--;
+
+	if (len == 4 && strncmp(buf, "high", 4) == 0)
+		status = gpio_direction_output(gpio, 1);
+
+	else if (len == 3 && (strncmp(buf, "out", 3) == 0
+			|| strncmp(buf, "low", 3) == 0))
+		status = gpio_direction_output(gpio, 0);
+
+	else if (len == 2 && strncmp(buf, "in", 2) == 0)
+		status = gpio_direction_input(gpio);
+
+	return (status < 0) ? status : size;
+}
+
+static const DEVICE_ATTR(direction, 0644, gpio_direction_show, gpio_direction_store);
+
+static ssize_t gpio_value_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	const struct gpio_desc	*desc = dev_get_drvdata(dev);
+	unsigned		gpio = desc - gpio_desc;
+
+	/* handle GPIOs being removed from underneath us... */
+	if (!test_bit(FLAG_EXPORT, &desc->flags))
+		return -EIO;
+
+	return sprintf(buf, "%d\n", gpio_get_value_cansleep(gpio));
+}
+
+static ssize_t gpio_value_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t size)
+{
+	const struct gpio_desc	*desc = dev_get_drvdata(dev);
+	unsigned		gpio = desc - gpio_desc;
+	long			value;
+	int			ret;
+
+	/* handle GPIOs being removed from underneath us... */
+	if (!test_bit(FLAG_EXPORT, &desc->flags))
+		return -EIO;
+
+	if (!test_bit(FLAG_IS_OUT, &desc->flags))
+		return -EINVAL;
+
+	ret = strict_strtol(buf, 0, &value);
+	if (ret < 0)
+		return ret;
+	gpio_set_value_cansleep(gpio, value != 0);
+	return size;
+}
+
+static const DEVICE_ATTR(value, 0644, gpio_value_show, gpio_value_store);
+
+static const struct attribute *gpio_attrs[] = {
+	&dev_attr_direction.attr,
+	&dev_attr_value.attr,
+	NULL,
+};
+
+static const struct attribute_group gpio_attr_group = {
+	.attrs = (struct attribute **) gpio_attrs,
+};
+
+static struct class *gpio_class;
+
+/**
+ * gpio_export - export a GPIO through sysfs
+ * @gpio: gpio to make available, already requested
+ *
+ * When drivers want to make a GPIO accessible to userspace after they
+ * have requested it -- perhaps while debugging, or as part of their
+ * public interface -- they may use this routine.
+ *
+ * Returns zero on success, else an error.
+ */
+int gpio_export(unsigned gpio)
+{
+	unsigned long		flags;
+	struct gpio_desc	*desc;
+	int			status = -EINVAL;
+
+	if (!gpio_class)
+		return -ENOSYS;
+
+	if (!gpio_is_valid(gpio))
+		return -EINVAL;
+
+	/* REVISIT mode param to say if it direction may be changed */
+
+	spin_lock_irqsave(&gpio_lock, flags);
+	desc = &gpio_desc[gpio];
+	if (test_bit(FLAG_REQUESTED, &desc->flags)
+			&& !test_bit(FLAG_EXPORT, &desc->flags))
+		status = 0;
+	spin_unlock_irqrestore(&gpio_lock, flags);
+
+	if (status)
+		pr_debug("%s: gpio-%d status %d\n", __func__, gpio, status);
+	else {
+		struct device	*dev;
+
+		dev = device_create(gpio_class, desc->chip->dev, 0,
+				"gpio-%d", gpio);
+		if (dev) {
+			dev_set_drvdata(dev, desc);
+			status = sysfs_create_group(&dev->kobj,
+					&gpio_attr_group);
+		}
+		if (status == 0)
+			set_bit(FLAG_EXPORT, &desc->flags);
+	}
+
+	return status;
+}
+EXPORT_SYMBOL_GPL(gpio_export);
+
+static int match_export(struct device *dev, void *data)
+{
+	return dev_get_drvdata(dev) == data;
+}
+
+/**
+ * gpio_unexport - reverse effect of gpio_export()
+ * @gpio: gpio to make unavailable
+ *
+ * This is implicit on gpio_free().
+ */
+void gpio_unexport(unsigned gpio)
+{
+	unsigned long		flags;
+	struct gpio_desc	*desc;
+	int			status = -EINVAL;
+
+	if (!gpio_is_valid(gpio))
+		return;
+
+	spin_lock_irqsave(&gpio_lock, flags);
+	desc = &gpio_desc[gpio];
+	if (test_bit(FLAG_EXPORT, &desc->flags))
+		status = 0;
+	spin_unlock_irqrestore(&gpio_lock, flags);
+
+	if (status == 0) {
+		struct device	*dev = NULL;
+
+		dev = class_find_device(gpio_class, desc, match_export);
+		if (dev) {
+			clear_bit(FLAG_EXPORT, &desc->flags);
+			put_device(dev);
+			device_unregister(dev);
+		}
+	}
+
+}
+EXPORT_SYMBOL_GPL(gpio_unexport);
+
+/*
+ * /sys/class/gpio/control ... write-only
+ *	export N
+ *	unexport N
+ */
+static ssize_t control_store(struct class *class, const char *buf, size_t len)
+{
+	char *scratch = (char *)buf;
+	char *cmd, *tmp;
+	int status;
+	unsigned long gpio;
+
+	/* export/unexport */
+	cmd = strsep(&scratch, " \t\n");
+	if (!cmd)
+		goto fail;
+
+	/* N */
+	tmp = strsep(&scratch, " \t\n");
+	if (!tmp)
+		goto fail;
+	status = strict_strtoul(tmp, 0, &gpio);
+	if (status < 0)
+		goto done;
+
+	/* reject commands with garbage at end */
+	tmp = strsep(&scratch, " \t\n");
+	if ((tmp && *tmp) || scratch)
+		goto fail;
+
+	if (strcmp(cmd, "export") == 0) {
+		status = gpio_request(gpio, "sysfs");
+		if (status < 0)
+			goto done;
+
+		status = gpio_export(gpio);
+		if (status < 0)
+			gpio_free(gpio);
+		else
+			set_bit(FLAG_SYSFS, &gpio_desc[gpio].flags);
+
+	} else if (strcmp(cmd, "unexport") == 0) {
+		/* reject bogus commands (gpio_unexport ignores them) */
+		if (!gpio_is_valid(gpio))
+			goto fail;
+		if (!test_and_clear_bit(FLAG_SYSFS, &gpio_desc[gpio].flags))
+			goto fail;
+
+		gpio_free(gpio);
+	}
+done:
+	return (status < 0) ? status : len;
+fail:
+	return -EINVAL;
+}
+
+static CLASS_ATTR(control, 0200, NULL, control_store);
+
+static int __init gpiolib_sysfs_init(void)
+{
+	int	status;
+
+	gpio_class = class_create(THIS_MODULE, "gpio");
+	if (IS_ERR(gpio_class))
+		return PTR_ERR(gpio_class);
+
+	status = class_create_file(gpio_class, &class_attr_control);
+	if (status < 0) {
+		class_destroy(gpio_class);
+		gpio_class = NULL;
+	}
+	return status;
+}
+postcore_initcall(gpiolib_sysfs_init);
+
+#endif /* CONFIG_GPIO_SYSFS */
+
 void gpio_free(unsigned gpio)
 {
 	unsigned long		flags;
@@ -296,6 +575,8 @@ void gpio_free(unsigned gpio)
 		return;
 	}
 
+	gpio_unexport(gpio);
+
 	spin_lock_irqsave(&gpio_lock, flags);
 
 	desc = &gpio_desc[gpio];
--- g26.orig/drivers/gpio/mcp23s08.c	2008-04-28 11:17:42.000000000 -0700
+++ g26/drivers/gpio/mcp23s08.c	2008-04-28 11:31:21.000000000 -0700
@@ -239,6 +239,7 @@ static int mcp23s08_probe(struct spi_dev
 	mcp->chip.base = pdata->base;
 	mcp->chip.ngpio = 8;
 	mcp->chip.can_sleep = 1;
+	mcp->chip.dev = &spi->dev;
 	mcp->chip.owner = THIS_MODULE;
 
 	spi_set_drvdata(spi, mcp);
--- g26.orig/drivers/gpio/pca953x.c	2008-04-28 11:17:42.000000000 -0700
+++ g26/drivers/gpio/pca953x.c	2008-04-28 11:31:21.000000000 -0700
@@ -189,6 +189,7 @@ static void pca953x_setup_gpio(struct pc
 	gc->base = chip->gpio_start;
 	gc->ngpio = gpios;
 	gc->label = chip->client->name;
+	gc->dev = &chip->client->dev;
 	gc->owner = THIS_MODULE;
 }
 
--- g26.orig/drivers/gpio/pcf857x.c	2008-04-28 11:17:42.000000000 -0700
+++ g26/drivers/gpio/pcf857x.c	2008-04-28 11:31:21.000000000 -0700
@@ -159,6 +159,7 @@ static int pcf857x_probe(struct i2c_clie
 
 	gpio->chip.base = pdata->gpio_base;
 	gpio->chip.can_sleep = 1;
+	gpio->chip.dev = &client->dev;
 	gpio->chip.owner = THIS_MODULE;
 
 	/* NOTE:  the OnSemi jlc1562b is also largely compatible with
--- g26.orig/drivers/i2c/chips/tps65010.c	2008-04-28 11:11:07.000000000 -0700
+++ g26/drivers/i2c/chips/tps65010.c	2008-04-28 11:31:21.000000000 -0700
@@ -650,6 +650,8 @@ static int tps65010_probe(struct i2c_cli
 		tps->outmask = board->outmask;
 
 		tps->chip.label = client->name;
+		tps->chip.dev = &client->dev;
+		tps->chip.owner = THIS_MODULE;
 
 		tps->chip.set = tps65010_gpio_set;
 		tps->chip.direction_output = tps65010_output;
--- g26.orig/drivers/mfd/htc-egpio.c	2008-04-28 11:11:07.000000000 -0700
+++ g26/drivers/mfd/htc-egpio.c	2008-04-28 11:31:21.000000000 -0700
@@ -318,6 +318,8 @@ static int __init egpio_probe(struct pla
 		ei->chip[i].dev = &(pdev->dev);
 		chip = &(ei->chip[i].chip);
 		chip->label           = "htc-egpio";
+		chip->dev             = &pdev->dev;
+		chip->owner           = THIS_MODULE;
 		chip->get             = egpio_get;
 		chip->set             = egpio_set;
 		chip->direction_input = egpio_direction_input;
--- g26.orig/include/asm-generic/gpio.h	2008-04-28 11:17:44.000000000 -0700
+++ g26/include/asm-generic/gpio.h	2008-04-28 11:49:49.000000000 -0700
@@ -28,6 +28,8 @@ struct module;
 /**
  * struct gpio_chip - abstract a GPIO controller
  * @label: for diagnostics
+ * @dev: optional device providing the GPIOs
+ * @owner: helps prevent removal of modules exporting active GPIOs
  * @direction_input: configures signal "offset" as input, or returns error
  * @get: returns value for signal "offset"; for output signals this
  *	returns either the value actually sensed, or zero
@@ -55,6 +57,7 @@ struct module;
  */
 struct gpio_chip {
 	char			*label;
+	struct device		*dev;
 	struct module		*owner;
 
 	int			(*direction_input)(struct gpio_chip *chip,
@@ -104,7 +107,19 @@ extern void __gpio_set_value(unsigned gp
 extern int __gpio_cansleep(unsigned gpio);
 
 
-#else
+#ifdef CONFIG_GPIO_SYSFS
+#define HAVE_GPIO_SYSFS
+
+/*
+ * 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);
+extern void gpio_unexport(unsigned gpio);
+
+#endif	/* CONFIG_GPIO_SYSFS */
+
+#else	/* !CONFIG_HAVE_GPIO_LIB */
 
 static inline int gpio_is_valid(int number)
 {
@@ -135,4 +150,15 @@ static inline void gpio_set_value_cansle
 
 #endif
 
+#ifndef HAVE_GPIO_SYSFS
+static inline int gpio_export(unsigned gpio)
+{
+	return -ENOSYS;
+}
+
+static inline void gpio_unexport(unsigned gpio)
+{
+}
+#endif	/* HAVE_GPIO_SYSFS */
+
 #endif /* _ASM_GENERIC_GPIO_H */

Re: [patch/rfc 2.6.25-git] gpio: sysfs interface

[Andrew Morton]

On Mon, 28 Apr 2008 12:39:51 -0700 David Brownell <david-b@pacbell.net> wrote:

> Simple sysfs interface for GPIOs.
> 
>     /sys/class/gpio
>         /gpio-N ... for each exported GPIO #N
> 	    /value ... always readable, writes fail except for output GPIOs
> 	    /direction ... writable as: in, out (default low), high, low
>     	/control ... to request a GPIO be exported or unexported
> 
> GPIOs may be exported by kernel code using gpio_export(), which should
> be most useful for driver debugging.  Userspace may also ask that they
> be exported by writing to the sysfs control file, helping to cope with
> incomplete board support:
> 
>   echo "export 23" > /sys/class/gpio/control
> 	... will gpio_request(23, "sysfs") and gpio_export(23); use
> 	/sys/class/gpio/gpio-23/direction to configure it.
>   echo "unexport 23" > /sys/class/gpio/control
> 	... will gpio_free(23)

hm, does ths sysfs one-value-per-file rule apply to writes?

> The D-space footprint is negligible, except for the sysfs resources
> associated with each exported GPIO.  The additional I-space footprint
> is about half of the current size of gpiolib.  No /dev node creation
> involved, and no "udev" support is needed.
> 
> This adds a device pointer to "struct gpio_chip".  When GPIO providers
> initialize that, sysfs gpio class devices become children of that device
> instead of being "virtual" devices.  The (few) gpio_chip providers which
> have such a device node have been updated.  (Some also needed to update
> their module "owner" field ... for which missing kerneldoc was added.)
> 
> Based on a patch from Trent Piepho <tpiepho@freescale.com>, and comments
> from various folk including Hartley Sweeten.
> 
> Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
> ---
>  arch/avr32/mach-at32ap/pio.c |    2 
>  drivers/gpio/Kconfig         |   16 ++
>  drivers/gpio/gpiolib.c       |  281 +++++++++++++++++++++++++++++++++++++++++++
>  drivers/gpio/mcp23s08.c      |    1 
>  drivers/gpio/pca953x.c       |    1 
>  drivers/gpio/pcf857x.c       |    1 
>  drivers/i2c/chips/tps65010.c |    2 
>  drivers/mfd/htc-egpio.c      |    2 
>  include/asm-generic/gpio.h   |   28 ++++

Documentation for the interface?

>  }
>  EXPORT_SYMBOL_GPL(gpio_request);
>  
> +
> +#ifdef CONFIG_GPIO_SYSFS
> +
> +/*
> + * /sys/class/gpio/gpio-N/... only for GPIOs that are exported
> + *  - direction
> + *      * is read/write as in/out
> + *      * may also be written as high/low, initializing output
> + *        value as specified (plain "out" implies "low")
> + *  - value
> + *      * always readable, subject to hardware behavior
> + *      * may be writable, as zero/nonzero
> + */
> +
> +#include <linux/device.h>
> +#include <linux/err.h>

Putting includes inside ifdefs tends to increase the risk of compilation
errors.

> +static ssize_t gpio_direction_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	const struct gpio_desc	*desc = dev_get_drvdata(dev);
> +
> +	/* handle GPIOs being removed from underneath us... */
> +	if (!test_bit(FLAG_EXPORT, &desc->flags))
> +		return -EIO;
> +
> +	return sprintf(buf, "%s\n",
> +		test_bit(FLAG_IS_OUT, &desc->flags) ? "out" : "in");
> +}

What prevents FLAG_EXPORT from getting cleared just after we tested it?

iow: this looks racy.

> +static ssize_t gpio_direction_store(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t size)
> +{
> +	const struct gpio_desc	*desc = dev_get_drvdata(dev);
> +	unsigned		gpio = desc - gpio_desc;
> +	unsigned		len = size;
> +	ssize_t			status = -EINVAL;
> +
> +	/* handle GPIOs being removed from underneath us... */
> +	if (!test_bit(FLAG_EXPORT, &desc->flags))
> +		return -EIO;

Dittoes.

> +	if (buf[len - 1] == '\n')
> +		len--;
> +
> +	if (len == 4 && strncmp(buf, "high", 4) == 0)
> +		status = gpio_direction_output(gpio, 1);
> +
> +	else if (len == 3 && (strncmp(buf, "out", 3) == 0
> +			|| strncmp(buf, "low", 3) == 0))
> +		status = gpio_direction_output(gpio, 0);
> +
> +	else if (len == 2 && strncmp(buf, "in", 2) == 0)
> +		status = gpio_direction_input(gpio);

urgh.

If we had a strcmp() variant which treats a \n in the first arg as a \0
the above would become

	if (sysfs_streq(buf, "high"))
		status = gpio_direction_output(gpio, 1);
	else if (sysfs_streq(buf, "out") || sysfs_streq(buf, "low"))
		status = gpio_direction_output(gpio, 0);
	else if (sysfs_streq(buf, "in"))
		status = gpio_direction_input(gpio);

(note the removal of the unneeded and misleading blank lines)

> +	return (status < 0) ? status : size;
> +}
> +
> +static const DEVICE_ATTR(direction, 0644, gpio_direction_show, gpio_direction_store);
> +
> +static ssize_t gpio_value_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	const struct gpio_desc	*desc = dev_get_drvdata(dev);
> +	unsigned		gpio = desc - gpio_desc;
> +
> +	/* handle GPIOs being removed from underneath us... */
> +	if (!test_bit(FLAG_EXPORT, &desc->flags))
> +		return -EIO;
> +
> +	return sprintf(buf, "%d\n", gpio_get_value_cansleep(gpio));
> +}
> +
> +static ssize_t gpio_value_store(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t size)
> +{
> +	const struct gpio_desc	*desc = dev_get_drvdata(dev);
> +	unsigned		gpio = desc - gpio_desc;
> +	long			value;
> +	int			ret;
> +
> +	/* handle GPIOs being removed from underneath us... */
> +	if (!test_bit(FLAG_EXPORT, &desc->flags))
> +		return -EIO;
> +
> +	if (!test_bit(FLAG_IS_OUT, &desc->flags))
> +		return -EINVAL;

racy?

> +	ret = strict_strtol(buf, 0, &value);
> +	if (ret < 0)
> +		return ret;
> +	gpio_set_value_cansleep(gpio, value != 0);
> +	return size;
> +}
>
> ...
>
> +/**
> + * gpio_export - export a GPIO through sysfs
> + * @gpio: gpio to make available, already requested
> + *
> + * When drivers want to make a GPIO accessible to userspace after they
> + * have requested it -- perhaps while debugging, or as part of their
> + * public interface -- they may use this routine.
> + *
> + * Returns zero on success, else an error.
> + */
> +int gpio_export(unsigned gpio)
> +{
> +	unsigned long		flags;
> +	struct gpio_desc	*desc;
> +	int			status = -EINVAL;
> +
> +	if (!gpio_class)
> +		return -ENOSYS;
> +
> +	if (!gpio_is_valid(gpio))
> +		return -EINVAL;
> +
> +	/* REVISIT mode param to say if it direction may be changed */
> +
> +	spin_lock_irqsave(&gpio_lock, flags);
> +	desc = &gpio_desc[gpio];
> +	if (test_bit(FLAG_REQUESTED, &desc->flags)
> +			&& !test_bit(FLAG_EXPORT, &desc->flags))
> +		status = 0;
> +	spin_unlock_irqrestore(&gpio_lock, flags);

Well there's some locking.  But it's there to pin gpio_desc[].

> +	if (status)
> +		pr_debug("%s: gpio-%d status %d\n", __func__, gpio, status);
> +	else {
> +		struct device	*dev;
> +
> +		dev = device_create(gpio_class, desc->chip->dev, 0,
> +				"gpio-%d", gpio);
> +		if (dev) {
> +			dev_set_drvdata(dev, desc);
> +			status = sysfs_create_group(&dev->kobj,
> +					&gpio_attr_group);
> +		}
> +		if (status == 0)
> +			set_bit(FLAG_EXPORT, &desc->flags);
> +	}
> +
> +	return status;
> +}
> +EXPORT_SYMBOL_GPL(gpio_export);
> +
> +static int match_export(struct device *dev, void *data)
> +{
> +	return dev_get_drvdata(dev) == data;
> +}
> +
> +/**
> + * gpio_unexport - reverse effect of gpio_export()
> + * @gpio: gpio to make unavailable
> + *
> + * This is implicit on gpio_free().
> + */
> +void gpio_unexport(unsigned gpio)
> +{
> +	unsigned long		flags;
> +	struct gpio_desc	*desc;
> +	int			status = -EINVAL;
> +
> +	if (!gpio_is_valid(gpio))
> +		return;

Is that a programming error?

> +	spin_lock_irqsave(&gpio_lock, flags);
> +	desc = &gpio_desc[gpio];
> +	if (test_bit(FLAG_EXPORT, &desc->flags))
> +		status = 0;
> +	spin_unlock_irqrestore(&gpio_lock, flags);
> +
> +	if (status == 0) {
> +		struct device	*dev = NULL;
> +
> +		dev = class_find_device(gpio_class, desc, match_export);
> +		if (dev) {
> +			clear_bit(FLAG_EXPORT, &desc->flags);
> +			put_device(dev);
> +			device_unregister(dev);
> +		}
> +	}
> +
> +}
> +EXPORT_SYMBOL_GPL(gpio_unexport);
> +
> +/*
> + * /sys/class/gpio/control ... write-only
> + *	export N
> + *	unexport N
> + */
> +static ssize_t control_store(struct class *class, const char *buf, size_t len)
> +{
> +	char *scratch = (char *)buf;
> +	char *cmd, *tmp;
> +	int status;
> +	unsigned long gpio;
> +
> +	/* export/unexport */
> +	cmd = strsep(&scratch, " \t\n");

urgh.  We cast away the const and then smash up the caller's const string
with strsep.

> +	if (!cmd)
> +		goto fail;
> +
> +	/* N */
> +	tmp = strsep(&scratch, " \t\n");
> +	if (!tmp)
> +		goto fail;
> +	status = strict_strtoul(tmp, 0, &gpio);
> +	if (status < 0)
> +		goto done;
> +
> +	/* reject commands with garbage at end */

strict_strtoul() already does that?

> +	tmp = strsep(&scratch, " \t\n");
> +	if ((tmp && *tmp) || scratch)
> +		goto fail;
> +
> +	if (strcmp(cmd, "export") == 0) {
> +		status = gpio_request(gpio, "sysfs");
> +		if (status < 0)
> +			goto done;
> +
> +		status = gpio_export(gpio);
> +		if (status < 0)
> +			gpio_free(gpio);
> +		else
> +			set_bit(FLAG_SYSFS, &gpio_desc[gpio].flags);
> +
> +	} else if (strcmp(cmd, "unexport") == 0) {
> +		/* reject bogus commands (gpio_unexport ignores them) */
> +		if (!gpio_is_valid(gpio))
> +			goto fail;
> +		if (!test_and_clear_bit(FLAG_SYSFS, &gpio_desc[gpio].flags))
> +			goto fail;
> +
> +		gpio_free(gpio);
> +	}
> +done:
> +	return (status < 0) ? status : len;
> +fail:
> +	return -EINVAL;
> +}

The string handling in here seems way over-engineered.  All we're doing is
parting "export 42" or "unexport 42".  Surely it can be done better than
this.

The more sysfs-friendly way would be to create separate sysfs files for the
export and unexport operations, I expect.

> +static CLASS_ATTR(control, 0200, NULL, control_store);
> +
> +static int __init gpiolib_sysfs_init(void)
> +{
> +	int	status;
> +
> +	gpio_class = class_create(THIS_MODULE, "gpio");
> +	if (IS_ERR(gpio_class))
> +		return PTR_ERR(gpio_class);
> +
> +	status = class_create_file(gpio_class, &class_attr_control);
> +	if (status < 0) {
> +		class_destroy(gpio_class);
> +		gpio_class = NULL;
> +	}
> +	return status;
> +}
> +postcore_initcall(gpiolib_sysfs_init);
> +
> +#endif /* CONFIG_GPIO_SYSFS */
> +
>
> ...
>
> -#else
> +#ifdef CONFIG_GPIO_SYSFS
> +#define HAVE_GPIO_SYSFS

Can we find a way to make HAVE_GPIO_SYSFS go away?  Just use
CONFIG_GPIO_SYSFS?

> +/*
> + * 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);
> +extern void gpio_unexport(unsigned gpio);
> +
> +#endif	/* CONFIG_GPIO_SYSFS */
> +
> +#else	/* !CONFIG_HAVE_GPIO_LIB */
>  
>  static inline int gpio_is_valid(int number)
>  {
> @@ -135,4 +150,15 @@ static inline void gpio_set_value_cansle
>  
>  #endif
>  
> +#ifndef HAVE_GPIO_SYSFS
> +static inline int gpio_export(unsigned gpio)
> +{
> +	return -ENOSYS;
> +}
> +
> +static inline void gpio_unexport(unsigned gpio)
> +{
> +}
> +#endif	/* HAVE_GPIO_SYSFS */
> +
>  #endif /* _ASM_GENERIC_GPIO_H */

Then this can just be moved into a #else.

Re: [patch/rfc 2.6.25-git] gpio: sysfs interface

[Ben Nizette]

On Mon, 2008-04-28 at 12:39 -0700, David Brownell wrote:
> Simple sysfs interface for GPIOs.
> 
>     /sys/class/gpio
>         /gpio-N ... for each exported GPIO #N
> 	    /value ... always readable, writes fail except for output GPIOs
> 	    /direction ... writable as: in, out (default low), high, low
>     	/control ... to request a GPIO be exported or unexported
> 
> GPIOs may be exported by kernel code using gpio_export(), which should
> be most useful for driver debugging.  Userspace may also ask that they
> be exported by writing to the sysfs control file, helping to cope with
> incomplete board support:
> 
>   echo "export 23" > /sys/class/gpio/control
> 	... will gpio_request(23, "sysfs") and gpio_export(23); use
> 	/sys/class/gpio/gpio-23/direction to configure it.
>   echo "unexport 23" > /sys/class/gpio/control
> 	... will gpio_free(23)

Righteo, so if the kernel explicitly gpio_export()s something, it won't
be gpio_request()ed allowing multiple "owners" making driver debugging
easier.  Most of the time though we don't want to be able to clobber the
kernel's gpios so the control file wizardry isn't so much to cope with
incomplete board support, rather it's the way all regular (ie
non-driver-debugging) gpio access is started..?  Or do you class any
situation where userspace needs primary gpio control as a BSP omission
as there Should Be a formal in-kernel driver for it all?

Also, gpio number discovery can be done through the debugfs interface
already in gpiolib but once you've got a userspace gpio interface which
relies on gpio numbering like this that information ceases to be simple
debugging and becomes pretty mission-critical.  IMO it would make more
sense to shuffle it in to /sys/class/gpio with all this stuff or at
least offer a cut-down chip-to-range mapping in a file here.

> 
> The D-space footprint is negligible, except for the sysfs resources
> associated with each exported GPIO.  The additional I-space footprint
> is about half of the current size of gpiolib.  No /dev node creation
> involved, and no "udev" support is needed.

Which is good for simplicity but makes async notification kinda tricky.
I would suggest that a lack of pin-change signalling is going to be a
problem for people who've become accustomed to having it in their
out-of-tree interfaces.  Probably not a showstopper here but certainly
something which will slow the uptake of this interface.

> 
> This adds a device pointer to "struct gpio_chip".  When GPIO providers
> initialize that, sysfs gpio class devices become children of that device
> instead of being "virtual" devices.  The (few) gpio_chip providers which
> have such a device node have been updated.  (Some also needed to update
> their module "owner" field ... for which missing kerneldoc was added.)
> 
> Based on a patch from Trent Piepho <tpiepho@freescale.com>, and comments
> from various folk including Hartley Sweeten.
> 
> Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>

[had some comments regarding the code but it seems akpm covered them all
already :-)]

Re: [patch/rfc 2.6.25-git] gpio: sysfs interface

[Trent Piepho]

On Mon, 28 Apr 2008, David Brownell wrote:
> Simple sysfs interface for GPIOs.
>
>    /sys/class/gpio
>        /gpio-N ... for each exported GPIO #N

I liked it better they way I had it, "label:N".

When you have multiple GPIO sources, it's a lot easier to see where they are
comming from if they use the chip label.  Especially if support for dynamic
allocation of gpio numbers is written.


> 	    /value ... always readable, writes fail except for output GPIOs
> 	    /direction ... writable as: in, out (default low), high, low

You took away the code for the label field?  That was one of the features of
my code that Ben Nizette mentioned as an advantage over a char-device
interface.

>    	/control ... to request a GPIO be exported or unexported
>
> GPIOs may be exported by kernel code using gpio_export(), which should
> be most useful for driver debugging.  Userspace may also ask that they
> be exported by writing to the sysfs control file, helping to cope with
> incomplete board support:

Why can't all gpios appear read-only in sysfs by default?

> This adds a device pointer to "struct gpio_chip".  When GPIO providers
> initialize that, sysfs gpio class devices become children of that device
> instead of being "virtual" devices.  The (few) gpio_chip providers which
> have such a device node have been updated.  (Some also needed to update
> their module "owner" field ... for which missing kerneldoc was added.)

I don't see what's wrong with having devices add to gpiolib create a device
for the gpio's to be the children of.  You said that some devices can't do
this, but I don't see the difficulty.

platform_device_register_simple("my-gpio", 0, NULL, 0);

How hard is that?

> Based on a patch from Trent Piepho <tpiepho@freescale.com>, and comments
> from various folk including Hartley Sweeten.

I don't recall seeing those comments.  Where were they posted?

> +		status = gpio_export(gpio);
> +		if (status < 0)
> +			gpio_free(gpio);
> +		else
> +			set_bit(FLAG_SYSFS, &gpio_desc[gpio].flags);
> +
> +	} else if (strcmp(cmd, "unexport") == 0) {
> +		/* reject bogus commands (gpio_unexport ignores them) */
> +		if (!gpio_is_valid(gpio))
> +			goto fail;
> +		if (!test_and_clear_bit(FLAG_SYSFS, &gpio_desc[gpio].flags))
> +			goto fail;
> +
> +		gpio_free(gpio);

Maybe you could simplify the text parsing by having positive gpio numbers
export the gpio and negative numbers un-export the gpio?  Then there would not
be any need to parse a command with arguments.

Re: [patch/rfc 2.6.25-git] gpio: sysfs interface

[David Brownell]

On Monday 28 April 2008, Andrew Morton wrote:
> On Mon, 28 Apr 2008 12:39:51 -0700 David Brownell <david-b@pacbell.net> wrote:
> 
> > Simple sysfs interface for GPIOs.
> > 
> >     /sys/class/gpio
> >         /gpio-N ... for each exported GPIO #N
> > 	    /value ... always readable, writes fail except for output GPIOs
> > 	    /direction ... writable as: in, out (default low), high, low
> >     	/control ... to request a GPIO be exported or unexported
> > 
> > GPIOs may be exported by kernel code using gpio_export(), which should
> > be most useful for driver debugging.  Userspace may also ask that they
> > be exported by writing to the sysfs control file, helping to cope with
> > incomplete board support:
> > 
> >   echo "export 23" > /sys/class/gpio/control
> > 	... will gpio_request(23, "sysfs") and gpio_export(23); use
> > 	/sys/class/gpio/gpio-23/direction to configure it.
> >   echo "unexport 23" > /sys/class/gpio/control
> > 	... will gpio_free(23)
> 
> hm, does ths sysfs one-value-per-file rule apply to writes?

That *is* one value:  a single command, to execute atomically!  :)

ISTR seeing that done elsewhere, and have seen various proposals
that rely on that working.  In any case, the one-per-file rationale
was to make things easier for userspace.


> > The D-space footprint is negligible, except for the sysfs resources
> > associated with each exported GPIO.  The additional I-space footprint
> > is about half of the current size of gpiolib.  No /dev node creation
> > involved, and no "udev" support is needed.
> > 
> > 		...
> > 		
> 
> Documentation for the interface?

Next version.  If the "is this interface OK" part of the RFC flies,
then additional effort on docs would not be wasted.


> > +#include <linux/device.h>
> > +#include <linux/err.h>
> 
> Putting includes inside ifdefs tends to increase the risk of compilation
> errors.

Good point, I'll move such stuff out of #ifdefs.  I almost
did that this time, but this way made for a simpler patch.  ;)


> > +static ssize_t gpio_direction_show(struct device *dev,
> > +		struct device_attribute *attr, char *buf)
> > +{
> > +	const struct gpio_desc	*desc = dev_get_drvdata(dev);
> > +
> > +	/* handle GPIOs being removed from underneath us... */
> > +	if (!test_bit(FLAG_EXPORT, &desc->flags))
> > +		return -EIO;
> > +
> > +	return sprintf(buf, "%s\n",
> > +		test_bit(FLAG_IS_OUT, &desc->flags) ? "out" : "in");
> > +}
> 
> What prevents FLAG_EXPORT from getting cleared just after we tested it?
> 
> iow: this looks racy.

Yeah, the isue there is that the attribute files may
still be open after the GPIO has been unexported.  For
this specific method the existing spinlock can solve
that problem ... but not in general.

Seems like a general fix for that should involve a mutex
covering all those sysfs actions:  read, write, export,
and unexport.  The unexport logic would need reworking,
but the rest could work pretty much as they do now (but
while holding that lock, which would protect that flag).

But that wouldn't change the user experience; the sysfs
attributes would still look and act the same.


> > +	if (buf[len - 1] == '\n')
> > +		len--;
> > +
> > +	if (len == 4 && strncmp(buf, "high", 4) == 0)
> > +		status = gpio_direction_output(gpio, 1);
> > +
> > +	else if (len == 3 && (strncmp(buf, "out", 3) == 0
> > +			|| strncmp(buf, "low", 3) == 0))
> > +		status = gpio_direction_output(gpio, 0);
> > +
> > +	else if (len == 2 && strncmp(buf, "in", 2) == 0)
> > +		status = gpio_direction_input(gpio);
> 
> urgh.
> 
> If we had a strcmp() variant which treats a \n in the first arg as a \0
> the above would become
> 
> 	if (sysfs_streq(buf, "high"))
> 		status = gpio_direction_output(gpio, 1);
> 	else if (sysfs_streq(buf, "out") || sysfs_streq(buf, "low"))
> 		status = gpio_direction_output(gpio, 0);
> 	else if (sysfs_streq(buf, "in"))
> 		status = gpio_direction_input(gpio);

That would indeed be better.  Maybe I should whip up a sysfs
patch adding that, and have this depend on that patch.  (I've
CC'd Greg in case he has comments on that...)

Alternatively:  strict_streq(), analogy to strict_strto*()?


> > +void gpio_unexport(unsigned gpio)
> > +{
> > +	unsigned long		flags;
> > +	struct gpio_desc	*desc;
> > +	int			status = -EINVAL;
> > +
> > +	if (!gpio_is_valid(gpio))
> > +		return;
> 
> Is that a programming error?

Yes.  But as with quite a lot of "rmmod" type paths, there's
no way to report it to the caller.  I'll add a pr_debug().


> > +/*
> > + * /sys/class/gpio/control ... write-only
> > + *	export N
> > + *	unexport N
> > + */
> > +static ssize_t control_store(struct class *class, const char *buf, size_t len)
> > +{
> > +	char *scratch = (char *)buf;
> > +	char *cmd, *tmp;
> > +	int status;
> > +	unsigned long gpio;
> > +
> > +	/* export/unexport */
> > +	cmd = strsep(&scratch, " \t\n");
> 
> urgh.  We cast away the const and then smash up the caller's const string
> with strsep.

Yeah, kind of ugly.  Not particularly happy with that, but
I wasn't keen on allocating a temporary copy of that string
either...


> > +	if (!cmd)
> > +		goto fail;
> > +
> > +	/* N */
> > +	tmp = strsep(&scratch, " \t\n");
> > +	if (!tmp)
> > +		goto fail;
> > +	status = strict_strtoul(tmp, 0, &gpio);
> > +	if (status < 0)
> > +		goto done;
> > +
> > +	/* reject commands with garbage at end */
> 
> strict_strtoul() already does that?

So it does.  That was leftover from a version with
a more complex control interface.  Easy to remove
the lines'o'code following *that* comment!


> > +	...
> 
> The string handling in here seems way over-engineered.  All we're doing is
> parting "export 42" or "unexport 42".  Surely it can be done better than
> this.
> 
> The more sysfs-friendly way would be to create separate sysfs files for the
> export and unexport operations, I expect.

Or just use negative numbers to mean "unexport";
ugly and hackish, but functional.

I don't want to see the components of one command
split into separate files, where they could be
clobbered by a second userspace activity ...
 

> > ...
> >
> > -#else
> > +#ifdef CONFIG_GPIO_SYSFS
> > +#define HAVE_GPIO_SYSFS
> 
> Can we find a way to make HAVE_GPIO_SYSFS go away?  Just use
> CONFIG_GPIO_SYSFS?

Yes, see below ... there's a minor penalty to be paid.


> > @@ -135,4 +150,15 @@ static inline void gpio_set_value_cansle
> >  
> >  #endif
> >  
> > +#ifndef HAVE_GPIO_SYSFS
> > +static inline int gpio_export(unsigned gpio)
> > +{
> > +	return -ENOSYS;
> > +}
> > +
> > +static inline void gpio_unexport(unsigned gpio)
> > +{
> > +}
> > +#endif	/* HAVE_GPIO_SYSFS */
> > +
> >  #endif /* _ASM_GENERIC_GPIO_H */
> 
> Then this can just be moved into a #else.

I'll just make it #ifndef CONFIG_GPIO_SYSFS ... that will make the
interface impossible to provide without gpiolib, but I'm not much
concerned about that.

Note that putting it *here* covers the case of platforms that provide
standard GPIO interfaces but don't use the newish gpiolib code to do
that ... which is why putting it in an #else (above) didn't suffice.

Re: [patch/rfc 2.6.25-git] gpio: sysfs interface

[David Brownell]

On Monday 28 April 2008, Ben Nizette wrote:
> 
> On Mon, 2008-04-28 at 12:39 -0700, David Brownell wrote:
> > Simple sysfs interface for GPIOs.
> > 
> >     /sys/class/gpio
> >         /gpio-N ... for each exported GPIO #N
> > 	    /value ... always readable, writes fail except for output GPIOs
> > 	    /direction ... writable as: in, out (default low), high, low
> >     	/control ... to request a GPIO be exported or unexported
> > 
> > GPIOs may be exported by kernel code using gpio_export(), which should
> > be most useful for driver debugging.  Userspace may also ask that they
> > be exported by writing to the sysfs control file, helping to cope with
> > incomplete board support:
> > 
> >   echo "export 23" > /sys/class/gpio/control
> > 	... will gpio_request(23, "sysfs") and gpio_export(23); use
> > 	/sys/class/gpio/gpio-23/direction to configure it.
> >   echo "unexport 23" > /sys/class/gpio/control
> > 	... will gpio_free(23)
> 
> Righteo, so if the kernel explicitly gpio_export()s something, it won't
> be gpio_request()ed allowing multiple "owners" making driver debugging 
> easier. 

The gpio_export() call requires someone (the caller!) to
have requested the GPIO already.  The "one owner" rule is
not being changed.


> Most of the time though we don't want to be able to clobber the 
> kernel's gpios

Right.  Not unless we're debugging the driver managing
those GPIOs.


> so the control file wizardry isn't so much to cope with 
> incomplete board support, rather it's the way all regular (ie
> non-driver-debugging) gpio access is started..?

Well, I wouldn't call that "regular"!  But then, I don't
have this "use GPIOs from userspace" focus.  To me, that's
the exception not the rule.


> Or do you class any 
> situation where userspace needs primary gpio control as a BSP omission
> as there Should Be a formal in-kernel driver for it all?

I suppose I'd prefer to see a formal gpio_export() call from
the kernel as part of the BSP, if that's the model for how that
particular board stack should be used.  But I suspect there will
be differing opinions on that point, especially from folk who
avoid custom kernels for whatever reasons.  (Like, "that binary
has been qualified, this one hasn't.")

 
> Also, gpio number discovery can be done through the debugfs interface
> already in gpiolib

... intended purely for debugging, not for use with production
systems ...


> but once you've got a userspace gpio interface which 
> relies on gpio numbering like this that information ceases to be simple
> debugging and becomes pretty mission-critical.  IMO it would make more
> sense to shuffle it in to /sys/class/gpio with all this stuff or at
> least offer a cut-down chip-to-range mapping in a file here.

I don't follow what you're saying here.  GPIO numbering is deeply
specific to the hardware; so I'd say the relevant hardware docs are
what become mission-critical.  The kernel can't know when some
field update has rewired a bunch of GPIOs, for example...

Trent pointed out that dynamic range assignment can make trouble,
so I can see some help might be needed here.  Were you suggesting
something like a /sys/class/gpio/chips file with contents like

	0-15	gpio
	16-31	gpio
	32-47	gpio
	48-63	gpio
	192-207	mpuio
	208-213	tps65010

(Matching a stock OMAP 5912 OSK board, for what it's worth.)


> > The D-space footprint is negligible, except for the sysfs resources
> > associated with each exported GPIO.  The additional I-space footprint
> > is about half of the current size of gpiolib.  No /dev node creation
> > involved, and no "udev" support is needed.
> 
> Which is good for simplicity but makes async notification kinda tricky.

Sysfs attributes are supposed to be pollable.  I've not done it,
but fs/sysfs/file.c::sysfs_notify() looks relevant ...


> I would suggest that a lack of pin-change signalling is going to be a
> problem for people who've become accustomed to having it in their
> out-of-tree interfaces.  Probably not a showstopper here but certainly
> something which will slow the uptake of this interface.

We accept patches.   Even patches on top of patches.  ;)

- Dave
 

Re: [patch/rfc 2.6.25-git] gpio: sysfs interface

[David Brownell]

On Monday 28 April 2008, Trent Piepho wrote:
> On Mon, 28 Apr 2008, David Brownell wrote:
> > Simple sysfs interface for GPIOs.
> >
> >    /sys/class/gpio
> >        /gpio-N ... for each exported GPIO #N
> 
> I liked it better they way I had it, "label:N".

Those labels may not be available though; or valid in pathnames.

 
> When you have multiple GPIO sources, it's a lot easier to see where they are
> comming from if they use the chip label.  Especially if support for dynamic
> allocation of gpio numbers is written.

I'd rather see such stuff in a "chip_label" attribute.  Easy enough to
add such a thing.  (The dynamic allocation code is now kernel GIT...)

 
> > 	    /value ... always readable, writes fail except for output GPIOs
> > 	    /direction ... writable as: in, out (default low), high, low
> 
> You took away the code for the label field?  That was one of the features of
> my code that Ben Nizette mentioned as an advantage over a char-device
> interface.

Since it's not always available, I removed it.  Note that you're
talking about the label passed to gpio_request() here, not the one
applied to the gpio_chip.  I could restore this as a "gpio_label"
attribute, that's not always present ... but I'd rather not have
such "optional" stuff.

 
> >    	/control ... to request a GPIO be exported or unexported
> >
> > GPIOs may be exported by kernel code using gpio_export(), which should
> > be most useful for driver debugging.  Userspace may also ask that they
> > be exported by writing to the sysfs control file, helping to cope with
> > incomplete board support:
> 
> Why can't all gpios appear read-only in sysfs by default?

Typical SOC based systems have up to a few hundred pins that could
be used as GPIOs.  The number actually used as GPIOs is rarely more
than a dozen or two, with other pins generally muxed to a peripheral
function ... or not even connected.

There's no point in having a few hundred useless nodes in sysfs!


> > This adds a device pointer to "struct gpio_chip".  When GPIO providers
> > initialize that, sysfs gpio class devices become children of that device
> > instead of being "virtual" devices.  The (few) gpio_chip providers which
> > have such a device node have been updated.  (Some also needed to update
> > their module "owner" field ... for which missing kerneldoc was added.)
> 
> I don't see what's wrong with having devices add to gpiolib create a device

It can't know where in the device tree to put such device nodes,
or how to implement operations like suspend() and resume() for
such nodes.  Creating such nodes, and their drivers, is NOT a role
for generic code like gpiolib.


> for the gpio's to be the children of.  You said that some devices can't do
> this, but I don't see the difficulty.
> 
> platform_device_register_simple("my-gpio", 0, NULL, 0);
> 
> How hard is that?

Most GPIOs come from platform code that doesn't create such a device
today.  In cases like OMAP, GPIOs are usable even before memory
allocations can work, so creating and using device nodes for GPIO
controllers would involve updating code running VERY early in boot...

You're free to write patches creating such device nodes, and work
with the platform maintainers to convert their GPIO support to use
standard driver model devices and drivers, then merge the results.

 
> > Based on a patch from Trent Piepho <tpiepho@freescale.com>, and comments
> > from various folk including Hartley Sweeten.
> 
> I don't recall seeing those comments.  Where were they posted?

Some were on-list, some were off-list.  The comments have been
coming off and on for a few years now, so I'm certain you would
not have seen all of them.  (I wouldn't have, either!)


> > +		status = gpio_export(gpio);
> > +		if (status < 0)
> > +			gpio_free(gpio);
> > +		else
> > +			set_bit(FLAG_SYSFS, &gpio_desc[gpio].flags);
> > +
> > +	} else if (strcmp(cmd, "unexport") == 0) {
> > +		/* reject bogus commands (gpio_unexport ignores them) */
> > +		if (!gpio_is_valid(gpio))
> > +			goto fail;
> > +		if (!test_and_clear_bit(FLAG_SYSFS, &gpio_desc[gpio].flags))
> > +			goto fail;
> > +
> > +		gpio_free(gpio);
> 
> Maybe you could simplify the text parsing by having positive gpio numbers
> export the gpio and negative numbers un-export the gpio?  Then there would not
> be any need to parse a command with arguments.

I had that thought too.  Even though I think that solution
is kind o fugly.  ;)

If need be, that can be changed.

- Dave

Re: [patch/rfc 2.6.25-git] gpio: sysfs interface

[Ben Nizette]

On Mon, 2008-04-28 at 16:09 -0700, Trent Piepho wrote:
> On Mon, 28 Apr 2008, David Brownell wrote:
> > Simple sysfs interface for GPIOs.
> >
> >    /sys/class/gpio
> >        /gpio-N ... for each exported GPIO #N
> 
> I liked it better they way I had it, "label:N".
> 
> When you have multiple GPIO sources, it's a lot easier to see where they are
> comming from if they use the chip label.  Especially if support for dynamic
> allocation of gpio numbers is written.
> 
> 
> > 	    /value ... always readable, writes fail except for output GPIOs
> > 	    /direction ... writable as: in, out (default low), high, low
> 
> You took away the code for the label field?  That was one of the features of
> my code that Ben Nizette mentioned as an advantage over a char-device
> interface.

If all gpios are exported read only by default then keeping the label
either in the folder naming or as a file is certainly useful to identify
what you actually want to talk to.  If you're debugging a driver or
you've already manually exported a gpio then I'd assume you already know
pretty much all you need to know about that pin.

	--Ben.

Re: [patch/rfc 2.6.25-git] gpio: sysfs interface

[Ben Nizette]

On Mon, 2008-04-28 at 17:44 -0700, David Brownell wrote:
> On Monday 28 April 2008, Ben Nizette wrote:
> > On Mon, 2008-04-28 at 12:39 -0700, David Brownell wrote:
> > > 
> > >   echo "export 23" > /sys/class/gpio/control
> > > 	... will gpio_request(23, "sysfs") and gpio_export(23); use
> > > 	/sys/class/gpio/gpio-23/direction to configure it.
> > >   echo "unexport 23" > /sys/class/gpio/control
> > > 	... will gpio_free(23)
> > 
> > Righteo, so if the kernel explicitly gpio_export()s something, it won't
> > be gpio_request()ed allowing multiple "owners" making driver debugging 
> > easier. 
> 
> The gpio_export() call requires someone (the caller!) to
> have requested the GPIO already.  The "one owner" rule is
> not being changed.

Owner's probably the wrong word.  I was just confirming that in that
case the request/export caller and userspace can both fiddle with the
pin.  Though of course the caller should expect this and behave
appropriately.

> 
> 
> > Most of the time though we don't want to be able to clobber the 
> > kernel's gpios
> 
> Right.  Not unless we're debugging the driver managing
> those GPIOs.
> 
> 
> > so the control file wizardry isn't so much to cope with 
> > incomplete board support, rather it's the way all regular (ie
> > non-driver-debugging) gpio access is started..?
> 
> Well, I wouldn't call that "regular"!  But then, I don't
> have this "use GPIOs from userspace" focus.  To me, that's
> the exception not the rule.
> 

Ah well we're backwards there, though now I think of it I can't think of
a great many valid use-cases on my side.  Just for funzies I'll post on
the avrfreaks AVR32 support forum and see how many I can actually dig
up.

> 
> > Or do you class any 
> > situation where userspace needs primary gpio control as a BSP omission
> > as there Should Be a formal in-kernel driver for it all?
> 
> I suppose I'd prefer to see a formal gpio_export() call from
> the kernel as part of the BSP, if that's the model for how that
> particular board stack should be used.  But I suspect there will
> be differing opinions on that point, especially from folk who
> avoid custom kernels for whatever reasons.  (Like, "that binary
> has been qualified, this one hasn't.")
> 

Keeping track of which pins aren't used for peripherals then requesting
and exporting them is going to be a pain even for smaller SoCs.  But as
you say, the BSP designer can just opt out if they can't be bothered.

Actually, in, eg AVR32 at32ap we'd have to keep track of all this anyway
so we can call at32_select_gpio() and make the pins ready for pio access
anyway.  Bugger.

>  
> > Also, gpio number discovery can be done through the debugfs interface
> > already in gpiolib
> 
> ... intended purely for debugging, not for use with production
> systems ...
> 
> 
> > but once you've got a userspace gpio interface which 
> > relies on gpio numbering like this that information ceases to be simple
> > debugging and becomes pretty mission-critical.  IMO it would make more
> > sense to shuffle it in to /sys/class/gpio with all this stuff or at
> > least offer a cut-down chip-to-range mapping in a file here.
> 
> I don't follow what you're saying here.  GPIO numbering is deeply
> specific to the hardware; so I'd say the relevant hardware docs are
> what become mission-critical.  The kernel can't know when some
> field update has rewired a bunch of GPIOs, for example...
> 
> Trent pointed out that dynamic range assignment can make trouble,
> so I can see some help might be needed here.  Were you suggesting
> something like a /sys/class/gpio/chips file with contents like
> 
> 	0-15	gpio
> 	16-31	gpio
> 	32-47	gpio
> 	48-63	gpio
> 	192-207	mpuio
> 	208-213	tps65010
> 
> (Matching a stock OMAP 5912 OSK board, for what it's worth.)

Yeah that's the kind of a thing.  Would be well worth having that info
especially for dynamically allocated chip bases.

> 
> 
> > > The D-space footprint is negligible, except for the sysfs resources
> > > associated with each exported GPIO.  The additional I-space footprint
> > > is about half of the current size of gpiolib.  No /dev node creation
> > > involved, and no "udev" support is needed.
> > 
> > Which is good for simplicity but makes async notification kinda tricky.
> 
> Sysfs attributes are supposed to be pollable.  I've not done it,
> but fs/sysfs/file.c::sysfs_notify() looks relevant ...

Right, that'll work.

> 
> 
> > I would suggest that a lack of pin-change signalling is going to be a
> > problem for people who've become accustomed to having it in their
> > out-of-tree interfaces.  Probably not a showstopper here but certainly
> > something which will slow the uptake of this interface.
> 
> We accept patches.   Even patches on top of patches.  ;)

I'll wait for a final(ish) version and a spare millisecond and see what
might be done :-)

--Ben.

> 
> - Dave
>  

Re: [patch/rfc 2.6.25-git] gpio: sysfs interface

[Andrew Morton]

On Mon, 28 Apr 2008 16:28:13 -0700 David Brownell <david-b@pacbell.net> wrote:

> > If we had a strcmp() variant which treats a \n in the first arg as a \0
> > the above would become
> > 
> > 	if (sysfs_streq(buf, "high"))
> > 		status = gpio_direction_output(gpio, 1);
> > 	else if (sysfs_streq(buf, "out") || sysfs_streq(buf, "low"))
> > 		status = gpio_direction_output(gpio, 0);
> > 	else if (sysfs_streq(buf, "in"))
> > 		status = gpio_direction_input(gpio);
> 
> That would indeed be better.  Maybe I should whip up a sysfs
> patch adding that, and have this depend on that patch.  (I've
> CC'd Greg in case he has comments on that...)

Yes, it would be a standalone patch.  The sort which generates oceans of
useful feedback ;) The sort which also generates hundreds of
use-new-toy-to-clean-up-old-code patches for me to merge :(

> Alternatively:  strict_streq(), analogy to strict_strto*()?

Yeah, I couldn't think of a decent name.  I do think it should return true
on finding a match so callers don't need to use !  or ==0.  So its name
shouldn't look anything like "strcmp".

Re: [patch/rfc 2.6.25-git] gpio: sysfs interface

[Greg KH]

On Mon, Apr 28, 2008 at 07:54:55PM -0700, Andrew Morton wrote:
> On Mon, 28 Apr 2008 16:28:13 -0700 David Brownell <david-b@pacbell.net> wrote:
> 
> > > If we had a strcmp() variant which treats a \n in the first arg as a \0
> > > the above would become
> > > 
> > > 	if (sysfs_streq(buf, "high"))
> > > 		status = gpio_direction_output(gpio, 1);
> > > 	else if (sysfs_streq(buf, "out") || sysfs_streq(buf, "low"))
> > > 		status = gpio_direction_output(gpio, 0);
> > > 	else if (sysfs_streq(buf, "in"))
> > > 		status = gpio_direction_input(gpio);
> > 
> > That would indeed be better.  Maybe I should whip up a sysfs
> > patch adding that, and have this depend on that patch.  (I've
> > CC'd Greg in case he has comments on that...)
> 
> Yes, it would be a standalone patch.  The sort which generates oceans of
> useful feedback ;) The sort which also generates hundreds of
> use-new-toy-to-clean-up-old-code patches for me to merge :(

Heh, sounds good to me :)

Becides, with linux-next, that merge mess is my problem now, not -mm...

thanks,

greg k-h

Re: [patch/rfc 2.6.25-git] gpio: sysfs interface

[David Brownell]

On Monday 28 April 2008, Ben Nizette wrote:
>
> Ah well we're backwards there, though now I think of it I can't think of
> a great many valid use-cases on my side.  Just for funzies I'll post on
> the avrfreaks AVR32 support forum and see how many I can actually dig
> up.

Use cases would always help clarify things.  I've seen just
enough to make me understand this is a useful feature, and
for more reasons than just "feature equality" letting us
obsolete three drivers/i2c/chips/*.c drivers and help vanish
half a dozen (at least!) out-of-tree drivers doing that.

The Gumstix user forums and wiki may help too.  ISTR they
have such a GPIO widget (maybe that's the one I saw which
supports polling?) and have shipped it for ages ... so they
will surely have some (PXA-specific) examples lurking.


> > Trent pointed out that dynamic range assignment can make trouble,
> > so I can see some help might be needed here.  Were you suggesting
> > something like a /sys/class/gpio/chips file with contents like
> > 
> > 	0-15	gpio
> > 	16-31	gpio
> > 	32-47	gpio
> > 	48-63	gpio
> > 	192-207	mpuio
> > 	208-213	tps65010
> > 
> > (Matching a stock OMAP 5912 OSK board, for what it's worth.)
> 
> Yeah that's the kind of a thing.  Would be well worth having that info
> especially for dynamically allocated chip bases.

I'd have no problem with that.  Some people surely would though;
it has more than one value in that file!  OMG, it's readable! We
can't have any of that!!  The Earth will turn in its grave!  And
Slashdot will be decorated in Pink!  Teh End Daze arrive!  :)

 
> > > > The D-space footprint is negligible, except for the sysfs resources
> > > > associated with each exported GPIO.  The additional I-space footprint
> > > > is about half of the current size of gpiolib.  No /dev node creation
> > > > involved, and no "udev" support is needed.
> > > 
> > > Which is good for simplicity but makes async notification kinda tricky.
> > 
> > Sysfs attributes are supposed to be pollable.  I've not done it,
> > but fs/sysfs/file.c::sysfs_notify() looks relevant ...
> 
> Right, that'll work.

OK.  In that case, I think I should plan to rename the "direction"
attribute as "configuration" or something a bit broader ... so that
writing "irq" (or maybe "rising", "falling", "bothedges", "poll")
would eventually configure it as an input with an IRQ handler.

Whenever someone contributes such an async notification scheme,
that is.  ;)

- Dave

Re: [patch/rfc 2.6.25-git] gpio: sysfs interface

[Ben Nizette]

On Mon, 2008-04-28 at 20:44 -0700, David Brownell wrote:
> On Monday 28 April 2008, Ben Nizette wrote:
> >
> > Ah well we're backwards there, though now I think of it I can't think of
> > a great many valid use-cases on my side.  Just for funzies I'll post on
> > the avrfreaks AVR32 support forum and see how many I can actually dig
> > up.
> 
> Use cases would always help clarify things.  I've seen just
> enough to make me understand this is a useful feature, and
> for more reasons than just "feature equality" letting us
> obsolete three drivers/i2c/chips/*.c drivers and help vanish
> half a dozen (at least!) out-of-tree drivers doing that.

Oh yeah, nearly every vendor of small not-a-simple-PC Linux boards would
have their own solution to this problem.  About time they were put to
the knackers.

> 
> The Gumstix user forums and wiki may help too.  ISTR they
> have such a GPIO widget (maybe that's the one I saw which
> supports polling?) and have shipped it for ages ... so they
> will surely have some (PXA-specific) examples lurking.

At a glance there's a bunch of how-to but very little why-to.  Bugger.
In fact their driver looks to be mostly obsoleted by gpio-keys anyway so
not only can't a see a specific use-case of their driver, I can't see
the point of it's existence at all :-/

> 
> 
> > > Trent pointed out that dynamic range assignment can make trouble,
> > > so I can see some help might be needed here.  Were you suggesting
> > > something like a /sys/class/gpio/chips file with contents like
> > > 
> > > 	0-15	gpio
> > > 	16-31	gpio
> > > 	32-47	gpio
> > > 	48-63	gpio
> > > 	192-207	mpuio
> > > 	208-213	tps65010
> > > 
> > > (Matching a stock OMAP 5912 OSK board, for what it's worth.)
> > 
> > Yeah that's the kind of a thing.  Would be well worth having that info
> > especially for dynamically allocated chip bases.
> 
> I'd have no problem with that.  Some people surely would though;
> it has more than one value in that file!  OMG, it's readable! We
> can't have any of that!!  The Earth will turn in its grave!  And
> Slashdot will be decorated in Pink!  Teh End Daze arrive!  :)

xD

Where would the doom mongers prefer it live?  /proc? ;-)

> 
>  
> > > > > The D-space footprint is negligible, except for the sysfs resources
> > > > > associated with each exported GPIO.  The additional I-space footprint
> > > > > is about half of the current size of gpiolib.  No /dev node creation
> > > > > involved, and no "udev" support is needed.
> > > > 
> > > > Which is good for simplicity but makes async notification kinda tricky.
> > > 
> > > Sysfs attributes are supposed to be pollable.  I've not done it,
> > > but fs/sysfs/file.c::sysfs_notify() looks relevant ...
> > 
> > Right, that'll work.
> 
> OK.  In that case, I think I should plan to rename the "direction"
> attribute as "configuration" or something a bit broader ... so that
> writing "irq" (or maybe "rising", "falling", "bothedges", "poll")
> would eventually configure it as an input with an IRQ handler.

Good plan, unless you'd prefer to see "direction" and "interrupt" config
separate.  I have no real preference but IMO
echo "falling" > interrupt
makes more immediate sense than
echo "falling" > configuration

> 
> Whenever someone contributes such an async notification scheme,
> that is.  ;)

;)

--Ben.

> 
> - Dave

Re: [patch/rfc 2.6.25-git] gpio: sysfs interface

[Trent Piepho]

On Mon, 28 Apr 2008, David Brownell wrote:
> On Monday 28 April 2008, Trent Piepho wrote:
>> On Mon, 28 Apr 2008, David Brownell wrote:
>>> Simple sysfs interface for GPIOs.
>>>
>>>    /sys/class/gpio
>>>        /gpio-N ... for each exported GPIO #N
>>
>> I liked it better they way I had it, "label:N".
>
> Those labels may not be available though; or valid in pathnames.

So just fall back to "gpio" if there is no label?  The only character that's
not valid in a pathname is '/', so that's trivial to check for.

const char *label = chip->label && !strchr(chip->label, '/') ?
 	chip->label : "gpio"; /* or "generic" or "unknown", or ...*/

This means you don't need a file with number to device assignents.  It makes
shell scripting a lot easier too.  Say I want the first gpio on a pca9557 gpio
expander?  It's will be something like:  /sys/class/gpio/pca9557-0:0

You don't have to worry about dynamic assigments.  You don't have to resort to
convoluted shell script code to extract the proper range from a mapping file
and then construct the name.

>>> 	    /value ... always readable, writes fail except for output GPIOs
>>> 	    /direction ... writable as: in, out (default low), high, low
>>
>> You took away the code for the label field?  That was one of the features of
>> my code that Ben Nizette mentioned as an advantage over a char-device
>> interface.
>
> Since it's not always available, I removed it.  Note that you're
> talking about the label passed to gpio_request() here, not the one
> applied to the gpio_chip.  I could restore this as a "gpio_label"
> attribute, that's not always present ... but I'd rather not have
> such "optional" stuff.

It's nice to be able to see what a driver is using a gpio for.  You could also
assign labels from userspace this way.

>>>    	/control ... to request a GPIO be exported or unexported
>>>
>>> GPIOs may be exported by kernel code using gpio_export(), which should
>>> be most useful for driver debugging.  Userspace may also ask that they
>>> be exported by writing to the sysfs control file, helping to cope with
>>> incomplete board support:
>>
>> Why can't all gpios appear read-only in sysfs by default?
>
> Typical SOC based systems have up to a few hundred pins that could
> be used as GPIOs.  The number actually used as GPIOs is rarely more
> than a dozen or two, with other pins generally muxed to a peripheral

So make them appear when something in the kernel requests them, explictly
exports them to user-space, or they are requested from user space.  The last
two can offer write access, the first only read.  I don't see why the explicit
request is necessary for something to show up in sysfs.  Nothing else in sysfs
seems to work this way.  At least, I see plenty of files in there that I
didn't have to manually make appear.

> There's no point in having a few hundred useless nodes in sysfs!

$ ls /sys/class/tty/ | wc
     579     579    4042

What's a few hundred more?

>>> This adds a device pointer to "struct gpio_chip".  When GPIO providers
>>> initialize that, sysfs gpio class devices become children of that device
>>> instead of being "virtual" devices.  The (few) gpio_chip providers which
>>> have such a device node have been updated.  (Some also needed to update
>>> their module "owner" field ... for which missing kerneldoc was added.)
>>
>> I don't see what's wrong with having devices add to gpiolib create a device
>
> It can't know where in the device tree to put such device nodes,
> or how to implement operations like suspend() and resume() for
> such nodes.  Creating such nodes, and their drivers, is NOT a role
> for generic code like gpiolib.

I didn't mean for gpiolib to create that device, that's obviously wrong.  What
I meant was the platform code, e.g. the code the calls gpiochip_add(), could
just call that one function and then it would have a device for the gpios to
appear under in sysfs.  You said that many systems "can't" have a device for
the gpios and I don't see how this is so.  Could you give me an example of
something that calls gpiochip_add() and can't provide a dev field in the
gpio_chip struct?

>> for the gpio's to be the children of.  You said that some devices can't do
>> this, but I don't see the difficulty.
>>
>> platform_device_register_simple("my-gpio", 0, NULL, 0);
>>
>> How hard is that?
>
> Most GPIOs come from platform code that doesn't create such a device
> today.  In cases like OMAP, GPIOs are usable even before memory
> allocations can work, so creating and using device nodes for GPIO
> controllers would involve updating code running VERY early in boot...

If memory allocations don't work, then gpiochip_add() can't possibly do
anything with sysfs, so having a device to parent the sysfs files from is a
moot point.

> You're free to write patches creating such device nodes, and work
> with the platform maintainers to convert their GPIO support to use
> standard driver model devices and drivers, then merge the results.

It's one line of code.

>>> Based on a patch from Trent Piepho <tpiepho@freescale.com>, and comments
>>> from various folk including Hartley Sweeten.
>>
>> I don't recall seeing those comments.  Where were they posted?
>
> Some were on-list, some were off-list.  The comments have been
> coming off and on for a few years now, so I'm certain you would
> not have seen all of them.  (I wouldn't have, either!)

I thought you meant comments to my patch.

Re: [patch/rfc 2.6.25-git] gpio: sysfs interface

[Trent Piepho]

On Mon, 28 Apr 2008, David Brownell wrote:
> On Monday 28 April 2008, Ben Nizette wrote:
>>
>> Ah well we're backwards there, though now I think of it I can't think of
>> a great many valid use-cases on my side.  Just for funzies I'll post on
>> the avrfreaks AVR32 support forum and see how many I can actually dig
>> up.
>
> Use cases would always help clarify things.  I've seen just
> enough to make me understand this is a useful feature, and
> for more reasons than just "feature equality" letting us
> obsolete three drivers/i2c/chips/*.c drivers and help vanish
> half a dozen (at least!) out-of-tree drivers doing that.

If you want use cases, how about mine?  I didn't write the code originally for
fun, I wrote it for a real product.

We have a flash chip with a hardware write protected boot block controlled by
a gpio.  If we want to flash this block, we need a way to change the gpio. 
patching the mtd driver to do this automatically would require maintaining an
out-of-tree patch, I like to avoid those.  We'd also rather mtd didn't
automatically un-write-protect the boot block; kinda defeats the purpose.

The device may have a daughtercard installed in it.  There is a gpio used as a
presence detect.  We want to be able, from userspace (any maybe kernel too),
to print out "card installed" or "no card installed".  There is also certain
stuff that should run if the card is present when the machine boots.

We have some PCA9557 I2C gpio expanders that encode a device version number. 
We want to print this number out in userspace (e.g.  show it in the web
interface, various other application specific interfaces, etc.).  Maybe the
kernel will need to know too, we'll see what happens when there is a version
two.  The daughter card also has a PCA9557 expander, but of course it might
not be connected, the pca9557 driver can probe the bus for this.

>>> 	0-15	gpio
>>> 	16-31	gpio
>>> 	32-47	gpio
>>> 	48-63	gpio
>>> 	192-207	mpuio
>>> 	208-213	tps65010
>>>
>>> (Matching a stock OMAP 5912 OSK board, for what it's worth.)
>>
>> Yeah that's the kind of a thing.  Would be well worth having that info
>> especially for dynamically allocated chip bases.
>
> I'd have no problem with that.  Some people surely would though;
> it has more than one value in that file!  OMG, it's readable! We
> can't have any of that!!  The Earth will turn in its grave!  And
> Slashdot will be decorated in Pink!  Teh End Daze arrive!  :)

Suppose I want line 5 from the mpuio gpios?  I'd make it so this was: 
/sys/class/gpio/mpuio:5

How would you get the sysfs location, with an automatic shell script running
on a lightweight embedded system (no perl!)?

> OK.  In that case, I think I should plan to rename the "direction"
> attribute as "configuration" or something a bit broader ... so that
> writing "irq" (or maybe "rising", "falling", "bothedges", "poll")
> would eventually configure it as an input with an IRQ handler.

Why not have an irq file for that?

Re: [patch/rfc 2.6.25-git] gpio: sysfs interface

[Ben Nizette]

On Mon, 2008-04-28 at 22:48 -0700, Trent Piepho wrote:
> On Mon, 28 Apr 2008, David Brownell wrote:
> > On Monday 28 April 2008, Trent Piepho wrote:
> >> On Mon, 28 Apr 2008, David Brownell wrote:
> >>> Simple sysfs interface for GPIOs.
> >>>
> >>>    /sys/class/gpio
> >>>        /gpio-N ... for each exported GPIO #N
> >>
> >> I liked it better they way I had it, "label:N".
> >
> > Those labels may not be available though; or valid in pathnames.
> 
> So just fall back to "gpio" if there is no label?  The only character that's
> not valid in a pathname is '/', so that's trivial to check for.
> 
> const char *label = chip->label && !strchr(chip->label, '/') ?
>  	chip->label : "gpio"; /* or "generic" or "unknown", or ...*/
> 
> This means you don't need a file with number to device assignents.  It makes
> shell scripting a lot easier too.  Say I want the first gpio on a pca9557 gpio
> expander?  It's will be something like:  /sys/class/gpio/pca9557-0:0
> 
> You don't have to worry about dynamic assigments.  You don't have to resort to
> convoluted shell script code to extract the proper range from a mapping file
> and then construct the name.

Sorry if I'm being dense; how do you want this bit to work?  As I see
it, there are a few options:

1) Have the files named as you suggest and all of them always present,
albeit read-only until export.  Very easy to use, easy to discover which
file is which, a decent bit of memory usage having them all listed.

2) Have the files named as you suggest and you have to explicitly
request them or have the kernel explicitly export them.  To request them
yourself you're going to need the gpio number so having the created file
labelled nicely isn't a win over having it labelled with the gpio
number.  I 'spose there's a win for kernel exported gpios, they're more
human readable, but you're still going to have to have the mappings
available somewhere for the manually exported gpios anyway.

3) Have the files named as you suggest, explicit export/request but
better parsing behind the control file so something like
echo "export pca9557-0:5" > control
works.  Very very nice for the user, big heavy back end.

4) Status quo.  Easy, efficient, potentially hard to discover which gpio
you actually want.

My vote's for 1 or 4.  The first one is heavier but easier.  The last
one will need something like the discussed file mapping ranges to gpios.
Do your expectations/ideas fit in cleanly anywhere above?

Thanks,
	--Ben  

Re: [patch/rfc 2.6.25-git] gpio: sysfs interface

[Trent Piepho]

On Tue, 29 Apr 2008, Ben Nizette wrote:
> On Mon, 2008-04-28 at 22:48 -0700, Trent Piepho wrote:
>> On Mon, 28 Apr 2008, David Brownell wrote:
>>> On Monday 28 April 2008, Trent Piepho wrote:
>>>> I liked it better they way I had it, "label:N".
>>>
>>> Those labels may not be available though; or valid in pathnames.
>>
>> So just fall back to "gpio" if there is no label?  The only character that's
>> not valid in a pathname is '/', so that's trivial to check for.
>>
>> const char *label = chip->label && !strchr(chip->label, '/') ?
>>  	chip->label : "gpio"; /* or "generic" or "unknown", or ...*/
>>
>> This means you don't need a file with number to device assignents.  It makes
>> shell scripting a lot easier too.  Say I want the first gpio on a pca9557 gpio
>> expander?  It's will be something like:  /sys/class/gpio/pca9557-0:0
>>
>> You don't have to worry about dynamic assigments.  You don't have to resort to
>> convoluted shell script code to extract the proper range from a mapping file
>> and then construct the name.
>
> Sorry if I'm being dense; how do you want this bit to work?  As I see
> it, there are a few options:
>
> 1) Have the files named as you suggest and all of them always present,
> albeit read-only until export.  Very easy to use, easy to discover which
> file is which, a decent bit of memory usage having them all listed.

Well, is it really that much?  There are 579 files under /sys/class/tty.  But
suppose it is too much (why isn't tty too much then?), then we can do 3.

> 3) Have the files named as you suggest, explicit export/request but
> better parsing behind the control file so something like
> echo "export pca9557-0:5" > control
> works.  Very very nice for the user, big heavy back end.

The back end doesn't seem that big to me.  Here's code for it.  If anything,
the parsing code is simpler than what David has.  It's certainly not huge. 
David's code for parsing the control file plus code for generating a mapping
range file would certainly be larger.


/* Format:  -?(chiplabel:)?number
  * The optional leading - unexports the gpio, without it the gpio is exported.
  * The optional chip label followed by a : gives you the Nth gpio of that
  * chip.  With no label you get gpio "number".
  */

static ssize_t control_store(struct class *class, const char *buf, size_t len)
{
 	const char *numstr;
 	unsigned long num;
 	int mode = 0;

 	if (buf[0] == '-') { /* un-export? */
 		mode = 1;
 		buf++;
 	}

 	numstr = strrchr(buf, ':');

 	/* Get GPIO number */
 	if (strict_strtoul(numstr ? numstr + 1 : buf, 0, &num))
 		return -EINVAL;

 	/* Match chip label, if one was specified */
 	if (numstr) {
 		/* No + 1 in len to not include the ':' at the end */
 		int i, len = numstr - buf;
 		const struct gpio_chip *chip = NULL;

 		for (i = 0; gpio_is_valid(i); i++) {
 			if (chip == gpio_desc[i].chip)
 				continue;
 			chip = gpio_desc[i].chip;
 			if (!chip)
 				continue;

 			if (!strncmp(buf, chip->label, len) &&
 			    chip->label[len] == '\0')
 				goto found_chip;
 		}
 		return -EINVAL;
found_chip:
 		if (num >= chip->ngpio)
 			return -EINVAL;
 		num += chip->base;
 	}

 	if (mode) {
 		/* Unexport */
 		if (!gpio_is_valid(num))
 			return -EINVAL;
 		if (!test_and_clear_bit(FLAG_SYSFS, &gpio_desc[num].flags))
 			return -EINVAL;

 		gpio_free(num);
 	} else {
 		/* Export */
 		int status = gpio_request(num, "sysfs");

 		if (status < 0)
 			return status;

 		status = gpio_export(num);
 		if (status < 0) {
 			gpio_free(num);
 			return status;
 		}

 		set_bit(FLAG_SYSFS, &gpio_desc[num].flags);
 	}

 	return len;
}

Re: [patch/rfc 2.6.25-git] gpio: sysfs interface

[David Brownell]

On Monday 28 April 2008, Greg KH wrote:
> On Mon, Apr 28, 2008 at 07:54:55PM -0700, Andrew Morton wrote:
> > On Mon, 28 Apr 2008 16:28:13 -0700 David Brownell <david-b@pacbell.net> wrote:
> > 
> > > > If we had a strcmp() variant which treats a \n in the first arg as a \0
> > > > the above would become
> > > > 
> > > > 	if (sysfs_streq(buf, "high"))
> > > > 		status = gpio_direction_output(gpio, 1);
> > > > 	else if (sysfs_streq(buf, "out") || sysfs_streq(buf, "low"))
> > > > 		status = gpio_direction_output(gpio, 0);
> > > > 	else if (sysfs_streq(buf, "in"))
> > > > 		status = gpio_direction_input(gpio);
> > > 
> > > That would indeed be better.  Maybe I should whip up a sysfs
> > > patch adding that, and have this depend on that patch.  (I've
> > > CC'd Greg in case he has comments on that...)
> > 
> > Yes, it would be a standalone patch.  The sort which generates oceans of
> > useful feedback ;) The sort which also generates hundreds of
> > use-new-toy-to-clean-up-old-code patches for me to merge :(
> 
> Heh, sounds good to me :)

Hard to say where should live, but lib/strings.c seemed fair.
See the appended patch.  I made it not care which string has
newline termination, since caring seems very error-prone.

- Dave

========= CUT HERE
Add a new sysfs_streq() string comparison function, which ignores
the trailing newlines found in sysfs inputs.  By example:

	sysfs_streq("a", "b")	==> false
	sysfs_streq("a", "a")	==> true
	sysfs_streq("a", "a\n")	==> true
	sysfs_streq("a\n", "a")	==> true

This is intended to simplify parsing of sysfs inputs, letting them
avoid the need to manually strip off newlines from inputs.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
 include/linux/string.h |    2 ++
 lib/string.c           |   27 +++++++++++++++++++++++++++
 2 files changed, 29 insertions(+)

--- g26.orig/include/linux/string.h	2008-04-29 05:45:53.000000000 -0700
+++ g26/include/linux/string.h	2008-04-29 05:55:14.000000000 -0700
@@ -109,5 +109,7 @@ extern void *kmemdup(const void *src, si
 extern char **argv_split(gfp_t gfp, const char *str, int *argcp);
 extern void argv_free(char **argv);
 
+extern bool sysfs_streq(const char *s1, const char *s2);
+
 #endif
 #endif /* _LINUX_STRING_H_ */
--- g26.orig/lib/string.c	2008-04-29 05:15:52.000000000 -0700
+++ g26/lib/string.c	2008-04-29 05:55:32.000000000 -0700
@@ -493,6 +493,33 @@ char *strsep(char **s, const char *ct)
 EXPORT_SYMBOL(strsep);
 #endif
 
+/**
+ * sysfs_streq - return true if strings are equal, modulo trailing newline
+ * @s1: one string
+ * @s2: another string
+ *
+ * This routine returns true iff two strings are equal, treating both
+ * NUL and newline-then-NUL as equivalent string terminations.  It's
+ * geared for use with sysfs input strings, which generally terminate
+ * with newlines but are compared against values without newlines.
+ */
+bool sysfs_streq(const char *s1, const char *s2)
+{
+	while (*s1 && *s1 == *s2) {
+		s1++;
+		s2++;
+	}
+
+	if (*s1 == *s2)
+		return true;
+	if (!*s1 && *s2 == '\n' && !s2[1])
+		return true;
+	if (*s1 == '\n' && !s1[1] && !*s2)
+		return true;
+	return false;
+}
+EXPORT_SYMBOL(sysfs_streq);
+
 #ifndef __HAVE_ARCH_MEMSET
 /**
  * memset - Fill a region of memory with the given value

Re: [patch/rfc 2.6.25-git] gpio: sysfs interface

[Andrew Morton]

On Tue, 29 Apr 2008 11:45:13 -0700
David Brownell <david-b@pacbell.net> wrote:

> On Monday 28 April 2008, Greg KH wrote:
> > On Mon, Apr 28, 2008 at 07:54:55PM -0700, Andrew Morton wrote:
> > > On Mon, 28 Apr 2008 16:28:13 -0700 David Brownell <david-b@pacbell.net> wrote:
> > > 
> > > > > If we had a strcmp() variant which treats a \n in the first arg as a \0
> > > > > the above would become
> > > > > 
> > > > > 	if (sysfs_streq(buf, "high"))
> > > > > 		status = gpio_direction_output(gpio, 1);
> > > > > 	else if (sysfs_streq(buf, "out") || sysfs_streq(buf, "low"))
> > > > > 		status = gpio_direction_output(gpio, 0);
> > > > > 	else if (sysfs_streq(buf, "in"))
> > > > > 		status = gpio_direction_input(gpio);
> > > > 
> > > > That would indeed be better.  Maybe I should whip up a sysfs
> > > > patch adding that, and have this depend on that patch.  (I've
> > > > CC'd Greg in case he has comments on that...)
> > > 
> > > Yes, it would be a standalone patch.  The sort which generates oceans of
> > > useful feedback ;) The sort which also generates hundreds of
> > > use-new-toy-to-clean-up-old-code patches for me to merge :(
> > 
> > Heh, sounds good to me :)
> 
> Hard to say where should live, but lib/strings.c seemed fair.
> See the appended patch.  I made it not care which string has
> newline termination, since caring seems very error-prone.
> 
> - Dave
> 
> ========= CUT HERE
> Add a new sysfs_streq() string comparison function, which ignores
> the trailing newlines found in sysfs inputs.  By example:
> 
> 	sysfs_streq("a", "b")	==> false
> 	sysfs_streq("a", "a")	==> true
> 	sysfs_streq("a", "a\n")	==> true
> 	sysfs_streq("a\n", "a")	==> true

And

	sysfs_streq("a\n", "a\n") ==> true

which actually isn't very interesting.

> This is intended to simplify parsing of sysfs inputs, letting them
> avoid the need to manually strip off newlines from inputs.
> 
> Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
> ---
>  include/linux/string.h |    2 ++
>  lib/string.c           |   27 +++++++++++++++++++++++++++
>  2 files changed, 29 insertions(+)
> 
> --- g26.orig/include/linux/string.h	2008-04-29 05:45:53.000000000 -0700
> +++ g26/include/linux/string.h	2008-04-29 05:55:14.000000000 -0700
> @@ -109,5 +109,7 @@ extern void *kmemdup(const void *src, si
>  extern char **argv_split(gfp_t gfp, const char *str, int *argcp);
>  extern void argv_free(char **argv);
>  
> +extern bool sysfs_streq(const char *s1, const char *s2);
> +
>  #endif
>  #endif /* _LINUX_STRING_H_ */
> --- g26.orig/lib/string.c	2008-04-29 05:15:52.000000000 -0700
> +++ g26/lib/string.c	2008-04-29 05:55:32.000000000 -0700
> @@ -493,6 +493,33 @@ char *strsep(char **s, const char *ct)
>  EXPORT_SYMBOL(strsep);
>  #endif
>  
> +/**
> + * sysfs_streq - return true if strings are equal, modulo trailing newline
> + * @s1: one string
> + * @s2: another string
> + *
> + * This routine returns true iff two strings are equal, treating both
> + * NUL and newline-then-NUL as equivalent string terminations.  It's
> + * geared for use with sysfs input strings, which generally terminate
> + * with newlines but are compared against values without newlines.
> + */
> +bool sysfs_streq(const char *s1, const char *s2)
> +{
> +	while (*s1 && *s1 == *s2) {
> +		s1++;
> +		s2++;
> +	}
> +
> +	if (*s1 == *s2)
> +		return true;
> +	if (!*s1 && *s2 == '\n' && !s2[1])
> +		return true;
> +	if (*s1 == '\n' && !s1[1] && !*s2)
> +		return true;
> +	return false;
> +}
> +EXPORT_SYMBOL(sysfs_streq);
> +

Looks good to me.  I'll plan on sliding it into 2.6.26 a few days
hence.

Re: [patch/rfc 2.6.25-git] gpio: sysfs interface

[David Brownell]

On Monday 28 April 2008, Ben Nizette wrote:
> > > > 
> > > > Sysfs attributes are supposed to be pollable.  I've not done it,
> > > > but fs/sysfs/file.c::sysfs_notify() looks relevant ...
> > > 
> > > Right, that'll work.
> > 
> > OK.  In that case, I think I should plan to rename the "direction"
> > attribute as "configuration" or something a bit broader ... so that
> > writing "irq" (or maybe "rising", "falling", "bothedges", "poll")
> > would eventually configure it as an input with an IRQ handler.
> 
> Good plan, unless you'd prefer to see "direction" and "interrupt" config
> separate.  I have no real preference but IMO
>   echo "falling" > interrupt
> makes more immediate sense than
>   echo "falling" > configuration

OK, I'll leave it be then.  Given that not all GPIOs support
interrupts, I used the "poll" example intentionally ... being
able to poll every N seconds (milliseconds?) may be important.


> > 
> > Whenever someone contributes such an async notification scheme,
> > that is.  ;)
> 
> ;)

Re: [patch/rfc 2.6.25-git] gpio: sysfs interface

[David Brownell]

On Tuesday 29 April 2008, Ben Nizette wrote:
> 4) Status quo.  Easy, efficient, potentially hard to discover which gpio
> you actually want.
> 
> My vote's for 1 or 4.  The first one is heavier but easier.  The last
> one will need something like the discussed file mapping ranges to gpios.

My vote is for #4 with a chip listing file.

I don't like the hacked names ... none of the other /sys/class/*/name
files on any of my systems use hacked names.  The entire motivation for
name hacking seems wrong to me, and by observation it's been rejected
for all other class names.

- Dave

Re: [patch/rfc 2.6.25-git] gpio: sysfs interface

[David Brownell]

On Tuesday 29 April 2008, Trent Piepho wrote:
> > Sorry if I'm being dense; how do you want this bit to work?  As I see
> > it, there are a few options:
> >
> > 1) Have the files named as you suggest and all of them always present,
> > albeit read-only until export.  Very easy to use, easy to discover which
> > file is which, a decent bit of memory usage having them all listed.
> 
> Well, is it really that much?  There are 579 files under /sys/class/tty.  But
> suppose it is too much (why isn't tty too much then?), then we can do 3.

I just ssh'd into three embedded boards I have handy, and they have
respectively four, four, and seven entries there.  That "seven"
case is actually incorrect ... the other three serial ports aren't
connected to anything.

So:  yes, adding a few hundred useless sysfs nodes *IS* a problem
in the target environment of embedded boards.

Note that "read-only until export" is far from straightforward
to achieve.


> > 3) Have the files named as you suggest, explicit export/request but
> > better parsing behind the control file so something like
> > echo "export pca9557-0:5" > control
> > works.  Very very nice for the user, big heavy back end.
> 
> The back end doesn't seem that big to me.  Here's code for it.

Which fails in a common case:  chip labels are not unique.


> If anything, 
> the parsing code is simpler than what David has.

Apples vs oranges.  Use the same command syntax if you're going
to make comparisons; I can save even more with "+export/-unexport"
syntax.  For comparable syntax, your stuff *IS* bigger.


> David's code for parsing the control file plus code for generating a mapping
> range file would certainly be larger.

The #3 option presumes some file listing chips and ranges too,
since GPIOs are exported only on demand.  Ditto #2 and #4...

- Dave

Re: [patch/rfc 2.6.25-git] gpio: sysfs interface

[David Brownell]

On Monday 28 April 2008, Trent Piepho wrote:
> We have a flash chip with a hardware write protected boot block controlled by
> a gpio.  If we want to flash this block, we need a way to change the gpio. 
> patching the mtd driver to do this automatically would require maintaining an
> out-of-tree patch, I like to avoid those.  We'd also rather mtd didn't
> automatically un-write-protect the boot block; kinda defeats the purpose.
> 
> The device may have a daughtercard installed in it.  There is a gpio used as a
> presence detect.  We want to be able, from userspace (any maybe kernel too),
> to print out "card installed" or "no card installed".  There is also certain
> stuff that should run if the card is present when the machine boots.
> 
> We have some PCA9557 I2C gpio expanders that encode a device version number. 
> We want to print this number out in userspace (e.g.  show it in the web
> interface, various other application specific interfaces, etc.).  Maybe the
> kernel will need to know too, we'll see what happens when there is a version
> two.  The daughter card also has a PCA9557 expander, but of course it might
> not be connected, the pca9557 driver can probe the bus for this.

Good examples.  Note that the "daughtercard installed" cases generalize
somewhat ... it's not uncommon to do like DRAM sticks do with SPD EEPROMS,
and have a cheap EEPROM identifying characteristics of that card, since
different cards need different initialization/setup.

However, I think a slightly more common practice in current embedded
Linux systems is to build custom kernels that know which daughtercard(s)
are available.  That's mostly what gets pushed upstream, anyway...

ISTR that Gumstix distro kernels use a more retro scheme to identify what's
in the card stack:  they poke at various peripheral addresses to see if the
device on a given card is there.  In the PC space, that's what "ISA Legacy
Drivers" do, albeit at the level of individual peripherals, not cards.

- Dave

Re: [patch/rfc 2.6.25-git] gpio: sysfs interface

[Ben Nizette]

On Tue, 2008-04-29 at 14:55 -0700, David Brownell wrote:
> On Tuesday 29 April 2008, Ben Nizette wrote:
> > 4) Status quo.  Easy, efficient, potentially hard to discover which gpio
> > you actually want.
> > 
> > My vote's for 1 or 4.  The first one is heavier but easier.  The last
> > one will need something like the discussed file mapping ranges to gpios.
> 
> My vote is for #4 with a chip listing file.
> 
> I don't like the hacked names ... none of the other /sys/class/*/name
> files on any of my systems use hacked names.  The entire motivation for
> name hacking seems wrong to me, and by observation it's been rejected
> for all other class names.

Right, agreed.

I guess one last option (which is made hard by chip label non-uniqueness
but I'll throw out anyway) would be

/sys/class/gpio
	/chipa
		/gpio-n
			/value
			/direction
		/control
	/chipb
		:
		:

I guess this doesn't gain much over labelling files chipname:N (and has
the same pitfalls) but does at least seem less hackish.


	--Ben.
> 
> - Dave
> 

Re: [patch/rfc 2.6.25-git] gpio: sysfs interface

[Trent Piepho]

On Tue, 29 Apr 2008, David Brownell wrote:
>
>> If anything,
>> the parsing code is simpler than what David has.
>
> Apples vs oranges.  Use the same command syntax if you're going
> to make comparisons; I can save even more with "+export/-unexport"
> syntax.  For comparable syntax, your stuff *IS* bigger.

If the code you wrote it not too complex, then why is the code I wrote, which
is not larger, too complex?


>> David's code for parsing the control file plus code for generating a mapping
>> range file would certainly be larger.
>
> The #3 option presumes some file listing chips and ranges too,
> since GPIOs are exported only on demand.  Ditto #2 and #4...

You never answered how one was supposed to get the proper device from a
script.

Re: [patch/rfc 2.6.25-git] gpio: sysfs interface

[David Brownell]

On Tuesday 29 April 2008, Ben Nizette wrote:
> I guess one last option (which is made hard by chip label non-uniqueness
> but I'll throw out anyway) would be
> 
> /sys/class/gpio
>         /chipa
>                 /gpio-n
>                         /value
>                         /direction
>                 /control
>         /chipb
>                 :
>                 :
> 

Or maybe:

  /sys/class/gpio
	/gpiochip-X			<-- range X..(X+ngpio)
		/device			<-- symlink, if it's known
		/ngpio
		/label
		/start			<-- maybe; start == X

with the gpio-N links probably going where you showed.  That'd be
best in terms of Purity Of Essence.

- Dave

Re: [patch/rfc 2.6.25-git] gpio: sysfs interface

[Ben Nizette]

On Tue, 2008-04-29 at 18:04 -0700, David Brownell wrote:
> On Tuesday 29 April 2008, Ben Nizette wrote:
> > I guess one last option (which is made hard by chip label non-uniqueness
> > but I'll throw out anyway) would be
> > 
> > /sys/class/gpio
> >         /chipa
> >                 /gpio-n
> >                         /value
> >                         /direction
> >                 /control
> >         /chipb
> >                 :
> >                 :
> > 
> 
> Or maybe:
> 
>   /sys/class/gpio
> 	/gpiochip-X			<-- range X..(X+ngpio)
> 		/device			<-- symlink, if it's known
> 		/ngpio
> 		/label
> 		/start			<-- maybe; start == X
> 
> with the gpio-N links probably going where you showed.  That'd be
> best in terms of Purity Of Essence.

So you're suggesting that the gpio-N links and control file live inside
the gpiochip-X folder along with info about the chip to which they're
attached?  I don't mind this, sounds good.  Certainly feels most
sysfsish.

Scripting would be pretty simple assuming there's one control file per
chip and the gpio number written to said control file is relative to
that chip's base.  i.e. finding pcf9557:5 (assuming only one such
device) would just be

- find the gpiochip-X folder whose /label == pcf9557
- echo "export 5" > <that_folder>/control
- read/write <that_folder>/gpio-5/{value,direction}

If you've got multiple pca9557s then you're always going to have a hard
time distinguishing them but you've been given all the information
available to allow you to discover which is which.

In fact more than enough; if the base is dynamically allocated then you
don't know what to expect /start to be, you know what /ngpio will be and
you never need to find the full gpio number so those 2 files are
redundant yeah?

--Ben.

Re: [patch/rfc 2.6.25-git] gpio: sysfs interface

[Trent Piepho]

On Wed, 30 Apr 2008, Ben Nizette wrote:
>> On Tuesday 29 April 2008, Ben Nizette wrote:
>>> I guess one last option (which is made hard by chip label non-uniqueness
>>> but I'll throw out anyway) would be
>>>
>>> /sys/class/gpio
>>>         /chipa
>>>                 /gpio-n
>>>                         /value
>>>                         /direction
>>>                 /control
>>>         /chipb
>>>                 :
>>>                 :
>>>
>>
>> Or maybe:
>>
>>   /sys/class/gpio
>> 	/gpiochip-X			<-- range X..(X+ngpio)
>> 		/device			<-- symlink, if it's known
>> 		/ngpio
>> 		/label
>> 		/start			<-- maybe; start == X
>>
>> with the gpio-N links probably going where you showed.  That'd be
>> best in terms of Purity Of Essence.
>
> So you're suggesting that the gpio-N links and control file live inside
> the gpiochip-X folder along with info about the chip to which they're
> attached?  I don't mind this, sounds good.  Certainly feels most
> sysfsish.
>
> Scripting would be pretty simple assuming there's one control file per
> chip and the gpio number written to said control file is relative to
> that chip's base.  i.e. finding pcf9557:5 (assuming only one such
> device) would just be
>
> - find the gpiochip-X folder whose /label == pcf9557
> - echo "export 5" > <that_folder>/control
> - read/write <that_folder>/gpio-5/{value,direction}

I don't suppose you could actually write the code to do this?

I already wrote the code, and am using it, for the way I have it work.

cat /sys/class/gpio/pcf9557-0:5

But I guess this is too easy.  "We can't have any of that!!  The Earth will
turn in its grave!  And Slashdot will be decorated in Pink!  Teh End Daze
arrive!  :)"

Re: [patch/rfc 2.6.25-git] gpio: sysfs interface

[Ben Nizette]

On Tue, 2008-04-29 at 20:13 -0700, Trent Piepho wrote:
> On Wed, 30 Apr 2008, Ben Nizette wrote:
> >
> > So you're suggesting that the gpio-N links and control file live inside
> > the gpiochip-X folder along with info about the chip to which they're
> > attached?  I don't mind this, sounds good.  Certainly feels most
> > sysfsish.
> >
> > Scripting would be pretty simple assuming there's one control file per
> > chip and the gpio number written to said control file is relative to
> > that chip's base.  i.e. finding pcf9557:5 (assuming only one such
> > device) would just be
> >
> > - find the gpiochip-X folder whose /label == pcf9557
> > - echo "export 5" > <that_folder>/control
> > - read/write <that_folder>/gpio-5/{value,direction}
> 
> I don't suppose you could actually write the code to do this?
> 
> I already wrote the code, and am using it, for the way I have it work.
> 
> cat /sys/class/gpio/pcf9557-0:5
> 
> But I guess this is too easy.  "We can't have any of that!!  The Earth will
> turn in its grave!  And Slashdot will be decorated in Pink!  Teh End Daze
> arrive!  :)"
> 

I'm sure I could but I'd prefer to have us all come (as close as
possible) to an agreed theoretical interface first :-)

What you've got working there is nice but it still downsides (unless you
can set me straight).  First, we still have the problem of either 100s
of unused files or an index to use in a request.  Also, if you've got
many pcf9557s in a system you'll have a bunch of pcf9557-n; how do you
know which is which?

I think with the above solution we're coming very close to a sexy
system.  We don't need an index file, we have all the information we
need to find which device is which, there's no need to calculate any
full gpio number, there's no files which aren't going to be used, it's
safe, extensible, scriptable, sysfsish..  Am I missing anything?

--Ben

Re: [patch/rfc 2.6.25-git] gpio: sysfs interface

[David Brownell]

On Tuesday 29 April 2008, Ben Nizette wrote:
> 
> On Tue, 2008-04-29 at 18:04 -0700, David Brownell wrote:
> > On Tuesday 29 April 2008, Ben Nizette wrote:
> > > I guess one last option (which is made hard by chip label non-uniqueness
> > > but I'll throw out anyway) would be
> > > 
> > > /sys/class/gpio
> > >         /chipa
> > >                 /gpio-n
> > >                         /value
> > >                         /direction
> > >                 /control
> > >         /chipb
> > >                 :
> > >                 :
> > > 
> > 
> > Or maybe:
> > 
> >   /sys/class/gpio
> > 	/gpiochip-X			<-- range X..(X+ngpio)

Make that range X to X + ngpio - 1, of course.  :)

> > 		/device			<-- symlink, if it's known
> > 		/ngpio
> > 		/label
> > 		/start			<-- maybe; start == X
> > 
> > with the gpio-N links probably going where you showed.  That'd be
> > best in terms of Purity Of Essence.
> 
> So you're suggesting that the gpio-N links and control file live inside
> the gpiochip-X folder along with info about the chip to which they're
> attached?  I don't mind this, sounds good.  Certainly feels most
> sysfsish.

What I have working right now is like the original patch,
but with "gpiochipN" (and "gpioN") nodes.  The "*-N" style
doesn't match other sysfs usage.

So the "control" and "gpioN" files aren't nesting like that.

It could be argued which feels more natural.  For external
gpiochips it might feel more natural to nest the gpioN nodes.
On some SOCs, that also true of the integrated GPIOs (docs
refer to "PB29" highlighting port B bit 29).  But on many
others (OMAP and PXA for starters), they're referenced in
docs by a number 0..hundreds, ignoring details like which
register bank holds the relevant control bits.

That's why I decided not to use nesting.  Either choice can
seem a bit odd on some platform; this way is easier and takes
less kernel code.

- Dave

Re: [patch/rfc 2.6.25-git] gpio: sysfs interface

[David Brownell]

On Tuesday 29 April 2008, Trent Piepho wrote:
> On Tue, 29 Apr 2008, David Brownell wrote:
> >
> >> If anything,
> >> the parsing code is simpler than what David has.
> >
> > Apples vs oranges.  Use the same command syntax if you're going
> > to make comparisons; I can save even more with "+export/-unexport"
> > syntax.  For comparable syntax, your stuff *IS* bigger.
> 
> If the code you wrote it not too complex, then why is the code I wrote, which
> is not larger, too complex?

Well, Andrew *did* object to the complexity.  But that wasn't
the point I was making there:  you were comparing apples and
oranges ... which makes it particularly easy to reach desired
conclusions like "only *this* one tastes like oranges!".


> >> David's code for parsing the control file plus code for generating a mapping
> >> range file would certainly be larger.
> >
> > The #3 option presumes some file listing chips and ranges too,
> > since GPIOs are exported only on demand.  Ditto #2 and #4...
> 
> You never answered how one was supposed to get the proper device from a
> script.

No I didn't.  But that's why I liked Ben's suggestion of creating
sysfs nodes for each gpio_chip.  That's actually a good example
of why folk like the one-value-per-attribute model with sysfs, at
least for information that would be used with scripting.

- Dave

[patch/rfc 2.6.25-git v2] gpio: sysfs interface

[David Brownell]

Simple sysfs interface for GPIOs.

    /sys/class/gpio
    	/control ... to request a GPIO be imported or returned
        /gpioN ... for each exported GPIO #N
	    /value ... always readable, writes fail for input GPIOs
	    /direction ... r/w as: in, out (default low); write high, low
	/gpiochipN ... for each gpiochip; #N is its first GPIO
	    /base ... (r/o) same as N
	    /label ... (r/o) descriptive, not necessarily unique
	    /ngpio ... (r/o) number of GPIOs; numbered N .. N+(ngpio - 1)

GPIOs may be exported by kernel code using gpio_export(), which should
be most useful for driver debugging.  Userspace may also ask that they
be imported by writing to the sysfs control file, helping to cope with
incomplete board support:

  echo "23" > /sys/class/gpio/control
	... will gpio_request(23, "sysfs") and gpio_export(23); use
	/sys/class/gpio/gpio-23/direction to configure it.
  echo "-23" > /sys/class/gpio/control
	... will gpio_free(23)

The extra D-space footprint is a few hundred bytes, except for the sysfs
resources associated with each exported GPIO.  The additional I-space
footprint is about two thirds of the current size of gpiolib (!).  Since
no /dev node creation is involved, no "udev" support is needed.

This adds a device pointer to "struct gpio_chip".  When GPIO providers
initialize that, sysfs gpio class devices become children of that device
instead of being "virtual" devices.  The (few) gpio_chip providers which
have such a device node have been updated.  (Some also needed to update
their module "owner" field ... for which missing kerneldoc was added.)

Based on a patch from Trent Piepho <tpiepho@freescale.com>, and comments
from various folk including most recently Hartley Sweeten and Ben Nizette.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
Notable updates from previous version:  doc update, mutex protection
for sysfs access, export basic gpio_chip info ("what GPIOs exist"),
depend on new sysfs_streq() call, simplified control file syntax.

 Documentation/gpio.txt       |  101 ++++++++-
 arch/arm/plat-omap/gpio.c    |    3 
 arch/avr32/mach-at32ap/pio.c |    2 
 drivers/gpio/Kconfig         |   15 +
 drivers/gpio/gpiolib.c       |  473 ++++++++++++++++++++++++++++++++++++++++++-
 drivers/gpio/mcp23s08.c      |    1 
 drivers/gpio/pca953x.c       |    1 
 drivers/gpio/pcf857x.c       |    1 
 drivers/i2c/chips/tps65010.c |    2 
 drivers/mfd/htc-egpio.c      |    2 
 include/asm-generic/gpio.h   |   30 ++
 11 files changed, 616 insertions(+), 15 deletions(-)

--- a/Documentation/gpio.txt	2008-04-30 13:00:03.000000000 -0700
+++ b/Documentation/gpio.txt	2008-04-30 14:18:03.000000000 -0700
@@ -347,15 +347,12 @@ necessarily be nonportable.
 Dynamic definition of GPIOs is not currently standard; for example, as
 a side effect of configuring an add-on board with some GPIO expanders.
 
-These calls are purely for kernel space, but a userspace API could be built
-on top of them.
-
 
 GPIO implementor's framework (OPTIONAL)
 =======================================
 As noted earlier, there is an optional implementation framework making it
 easier for platforms to support different kinds of GPIO controller using
-the same programming interface.
+the same programming interface.  This framework is called "gpiolib".
 
 As a debugging aid, if debugfs is available a /sys/kernel/debug/gpio file
 will be found there.  That will list all the controllers registered through
@@ -439,4 +436,98 @@ becomes available.  That may mean the de
 calls for that GPIO can work.  One way to address such dependencies is for
 such gpio_chip controllers to provide setup() and teardown() callbacks to
 board specific code; those board specific callbacks would register devices
-once all the necessary resources are available.
+once all the necessary resources are available, and remove them later when
+the GPIO controller device becomes unavailable.
+
+
+Sysfs Interface for Userspace (OPTIONAL)
+========================================
+Platforms which use the "gpiolib" implementors framework may choose to
+configure a sysfs user interface to GPIOs.  This is different from the
+debugfs interface, since it provides control over GPIO direction and
+value instead of just showing a gpio state summary.  Plus, it would be
+present on production systems without debugging support.
+
+Given approprate hardware documentation for the system, userspace could
+know for example that GPIO #23 controls the write protect line used to
+protect boot loader segments in flash memory.  System upgrade procedures
+may need to temporarily remove that protection, first importing a GPIO,
+then changing its output state, then updating the code before re-enabling
+the write protection.  In normal use, GPIO #23 would never be touched,
+and the kernel would have no need to know about it.
+
+Again depending on appropriate hardware documentation, on some systems
+userspace GPIO can be used to determine system configuration data that
+standard kernels won't know about.  And for some tasks, simple userspace
+GPIO drivers could be all that the system really needs.
+
+
+Paths in Sysfs
+--------------
+There are three kinds of entry in /sys/class/gpio:  GPIO controllers
+("gpio_chip" instances), GPIOs themselves, and a control interface
+used to import GPIOs to userspace (and later return them).
+
+The control interface is write-only.  Userspace may ask to import
+control of a GPIO by writing its number to the control file:
+
+    /sys/class/gpio/control
+
+Write negative numbers to release userspace control.  For example,
+"echo 19 > control" to create a "gpio19" node, which can then be
+configured and modified.  Then "echo -19 > control" to remove the
+userspace controls for that signal.
+
+GPIO signals have paths like /sys/class/gpio/gpio42/ (for GPIO #42)
+and have the following read/write attributes:
+
+    /sys/class/gpio/gpioN/
+
+	"direction" ... reads as either "in" or "out".  This value may
+		normally be written.  Writing as "out" defaults to
+		initializing the value as low.  To ensure glitch free
+		operation, values "low" and "high" may be written to
+		configure the GPIO as an output with that initial value.
+
+	"value" ... reads as either 0 (low) or 1 (high).  If the GPIO
+		is configured as an output, this value may be written;
+		any nonzero value is treated as high.
+
+GPIO controllers have paths like /sys/class/gpio/chipchip42/ (for the
+controller implementing GPIOs starting at #42) and have the following
+read-only attributes:
+
+    /sys/class/gpio/gpiochipN/
+
+    	"base" ... same as N, the first GPIO managed by this chip
+
+    	"label" ... provided for diagnostics (not unique)
+
+    	"ngpio" ... how many GPIOs this manges (N to N + ngpio - 1)
+
+Board documentation should in most cases cover what GPIOs are used
+for what purposes.  However, those numbers are not always stable;
+GPIOs on a daughterboard might be different depending on the base
+board being used, or other cards in the stack.  In such cases, you
+may need to use the gpiochip nodes to determine th correct GPIO
+number to use for a given signal.
+
+
+Exporting from Kernel code
+--------------------------
+Kernel code can explicitly manage exports of GPIOs which have already been
+requested using gpio_request():
+
+	/* export the GPIO to userspace */
+	int gpio_export(unsigned gpio);
+
+	/* reverse gpio_export() */
+	void gpio_unexport();
+
+When a kernel driver has requested a GPIO, it may only be made available
+in the sysfs interface by gpio_export().  This prevents userspace code
+from clobbering important state.
+
+This explicit exporting can help with debugging (by making some kinds
+of experiments easier), or can provide an always-there interface that's
+suitable for documenting as part of a board support package.
--- a/arch/arm/plat-omap/gpio.c	2008-04-22 15:19:26.000000000 -0700
+++ b/arch/arm/plat-omap/gpio.c	2008-04-30 13:01:47.000000000 -0700
@@ -1488,6 +1488,9 @@ static int __init _omap_gpio_init(void)
 		bank->chip.set = gpio_set;
 		if (bank_is_mpuio(bank)) {
 			bank->chip.label = "mpuio";
+#ifdef CONFIG_ARCH_OMAP1
+			bank->chip.dev = &omap_mpuio_device.dev;
+#endif
 			bank->chip.base = OMAP_MPUIO(0);
 		} else {
 			bank->chip.label = "gpio";
--- a/arch/avr32/mach-at32ap/pio.c	2008-04-30 13:00:03.000000000 -0700
+++ b/arch/avr32/mach-at32ap/pio.c	2008-04-30 13:01:47.000000000 -0700
@@ -358,6 +358,8 @@ static int __init pio_probe(struct platf
 	pio->chip.label = pio->name;
 	pio->chip.base = pdev->id * 32;
 	pio->chip.ngpio = 32;
+	pio->chip.dev = &pdev->dev;
+	pio->chip.owner = THIS_MODULE;
 
 	pio->chip.direction_input = direction_input;
 	pio->chip.get = gpio_get;
--- a/drivers/gpio/Kconfig	2008-04-30 13:00:03.000000000 -0700
+++ b/drivers/gpio/Kconfig	2008-04-30 14:20:32.000000000 -0700
@@ -23,6 +23,21 @@ config DEBUG_GPIO
 	  slower.  The diagnostics help catch the type of setup errors
 	  that are most common when setting up new platforms or boards.
 
+config GPIO_SYSFS
+	bool "/sys/class/gpio/... (sysfs interface)"
+	depends on SYSFS && EXPERIMENTAL
+	help
+	  Say Y here to add a sysfs interface for GPIOs.
+
+	  This is mostly useful to work around omissions in a system's
+	  kernel support.  Those are common in custom and semicustom
+	  hardware assembled using standard kernels with a minimum of
+	  custom patches.  In those cases, userspace code may import
+	  a given GPIO from the kernel, if no kernel driver requested it.
+
+	  Kernel drivers may also request that a particular GPIO be
+	  exported to userspace; this can be useful when debugging.
+
 # put expanders in the right section, in alphabetical order
 
 comment "I2C GPIO expanders:"
--- a/drivers/gpio/gpiolib.c	2008-04-30 13:00:03.000000000 -0700
+++ b/drivers/gpio/gpiolib.c	2008-04-30 13:01:47.000000000 -0700
@@ -2,8 +2,11 @@
 #include <linux/module.h>
 #include <linux/irq.h>
 #include <linux/spinlock.h>
-
-#include <asm/gpio.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/debugfs.h>
+#include <linux/seq_file.h>
+#include <linux/gpio.h>
 
 
 /* Optional implementation infrastructure for GPIO interfaces.
@@ -44,6 +47,8 @@ struct gpio_desc {
 #define FLAG_REQUESTED	0
 #define FLAG_IS_OUT	1
 #define FLAG_RESERVED	2
+#define FLAG_EXPORT	3	/* protected by sysfs_lock */
+#define FLAG_SYSFS	4	/* exported via /sys/class/gpio/control */
 
 #ifdef CONFIG_DEBUG_FS
 	const char		*label;
@@ -151,6 +156,452 @@ err:
 	return ret;
 }
 
+#ifdef CONFIG_GPIO_SYSFS
+
+/* lock protects against unexport_gpio() being called while
+ * sysfs files are open.
+ */
+static DEFINE_MUTEX(sysfs_lock);
+
+/*
+ * /sys/class/gpio/gpioN... only for GPIOs that are exported
+ *   /direction
+ *      * is read/write as "in" or "out"
+ *      * may also be written as "high" or "low", initializing
+ *        output value as specified ("out" implies "low")
+ *   /value
+ *      * always readable, subject to hardware behavior
+ *      * may be writable, as zero/nonzero
+ *
+ * REVISIT there will likely be an attribute for configuring async
+ * notifications, e.g. to specify polling interval or IRQ trigger type
+ */
+
+static ssize_t gpio_direction_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	const struct gpio_desc	*desc = dev_get_drvdata(dev);
+	ssize_t			status;
+
+	mutex_lock(&sysfs_lock);
+
+	if (!test_bit(FLAG_EXPORT, &desc->flags))
+		status = -EIO;
+	else
+		status = sprintf(buf, "%s\n",
+			test_bit(FLAG_IS_OUT, &desc->flags)
+				? "out" : "in");
+
+	mutex_unlock(&sysfs_lock);
+	return status;
+}
+
+static ssize_t gpio_direction_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t size)
+{
+	const struct gpio_desc	*desc = dev_get_drvdata(dev);
+	unsigned		gpio = desc - gpio_desc;
+	ssize_t			status;
+
+	mutex_lock(&sysfs_lock);
+
+	if (!test_bit(FLAG_EXPORT, &desc->flags))
+		status = -EIO;
+	else if (sysfs_streq(buf, "high"))
+		status = gpio_direction_output(gpio, 1);
+	else if (sysfs_streq(buf, "out") || sysfs_streq(buf, "low"))
+		status = gpio_direction_output(gpio, 0);
+	else if (sysfs_streq(buf, "in"))
+		status = gpio_direction_input(gpio);
+	else
+		status = -EINVAL;
+
+	mutex_unlock(&sysfs_lock);
+	return status ? : size;
+}
+
+static const DEVICE_ATTR(direction, 0644,
+		gpio_direction_show, gpio_direction_store);
+
+static ssize_t gpio_value_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	const struct gpio_desc	*desc = dev_get_drvdata(dev);
+	unsigned		gpio = desc - gpio_desc;
+	ssize_t			status;
+
+	mutex_lock(&sysfs_lock);
+
+	if (!test_bit(FLAG_EXPORT, &desc->flags))
+		status = -EIO;
+	else
+		status = sprintf(buf, "%d\n", gpio_get_value_cansleep(gpio));
+
+	mutex_unlock(&sysfs_lock);
+	return status;
+}
+
+static ssize_t gpio_value_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t size)
+{
+	const struct gpio_desc	*desc = dev_get_drvdata(dev);
+	unsigned		gpio = desc - gpio_desc;
+	ssize_t			status;
+
+	mutex_lock(&sysfs_lock);
+
+	if (!test_bit(FLAG_EXPORT, &desc->flags))
+		status = -EIO;
+	else if (!test_bit(FLAG_IS_OUT, &desc->flags))
+		status = -EINVAL;
+	else {
+		long		value;
+
+		status = strict_strtol(buf, 0, &value);
+		if (status == 0) {
+			gpio_set_value_cansleep(gpio, value != 0);
+			status = size;
+		}
+	}
+
+	mutex_unlock(&sysfs_lock);
+	return status;
+}
+
+static const DEVICE_ATTR(value, 0644,
+		gpio_value_show, gpio_value_store);
+
+static const struct attribute *gpio_attrs[] = {
+	&dev_attr_direction.attr,
+	&dev_attr_value.attr,
+	NULL,
+};
+
+static const struct attribute_group gpio_attr_group = {
+	.attrs = (struct attribute **) gpio_attrs,
+};
+
+/*
+ * /sys/class/gpio/gpiochipN/
+ *   /base ... matching gpio_chip.base (N)
+ *   /label ... matching gpio_chip.label
+ *   /ngpio ... matching gpio_chip.ngpio
+ */
+
+static ssize_t chip_base_show(struct device *dev,
+			       struct device_attribute *attr, char *buf)
+{
+	const struct gpio_chip	*chip = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%d\n", chip->base);
+}
+static DEVICE_ATTR(base, 0444, chip_base_show, NULL);
+
+static ssize_t chip_label_show(struct device *dev,
+			       struct device_attribute *attr, char *buf)
+{
+	const struct gpio_chip	*chip = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%s\n", chip->label ? : "");
+}
+static DEVICE_ATTR(label, 0444, chip_label_show, NULL);
+
+static ssize_t chip_ngpio_show(struct device *dev,
+			       struct device_attribute *attr, char *buf)
+{
+	const struct gpio_chip	*chip = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%u\n", chip->ngpio);
+}
+static DEVICE_ATTR(ngpio, 0444, chip_ngpio_show, NULL);
+
+static const struct attribute *gpiochip_attrs[] = {
+	&dev_attr_base.attr,
+	&dev_attr_label.attr,
+	&dev_attr_ngpio.attr,
+	NULL,
+};
+
+static const struct attribute_group gpiochip_attr_group = {
+	.attrs = (struct attribute **) gpiochip_attrs,
+};
+
+/*
+ * /sys/class/gpio/control ... write-only
+ *	integer N:  non-negative == export; negative == unexport
+ */
+static ssize_t control_store(struct class *class, const char *buf, size_t len)
+{
+	long	gpio;
+	int	status;
+
+	status = strict_strtol(buf, 0, &gpio);
+	if (status < 0)
+		goto done;
+
+	/* No extra locking here; FLAG_SYSFS just signifies that the
+	 * request and export were done by on behalf of userspace, so
+	 * they may be undone on its behalf too.
+	 */
+
+	if (gpio >= 0) {			/* export */
+		status = gpio_request(gpio, "sysfs");
+		if (status < 0)
+			goto done;
+
+		status = gpio_export(gpio);
+		if (status < 0)
+			gpio_free(gpio);
+		else
+			set_bit(FLAG_SYSFS, &gpio_desc[gpio].flags);
+
+	} else {				/* unexport */
+		gpio = -gpio;
+
+		/* reject bogus commands (gpio_unexport ignores them) */
+		if (!gpio_is_valid(gpio))
+			goto fail;
+		if (!test_and_clear_bit(FLAG_SYSFS, &gpio_desc[gpio].flags))
+			goto fail;
+		status = 0;
+		gpio_free(gpio);
+	}
+done:
+	if (status)
+		pr_debug("%s: status %d\n", __func__, status);
+	return status ? : len;
+fail:
+	pr_debug("%s: fail\n", __func__);
+	return -EINVAL;
+}
+
+static struct class_attribute gpio_class_attrs[] = {
+	__ATTR(control, 0200, NULL, control_store),
+	{},
+};
+
+static struct class gpio_class = {
+	.name =		"gpio",
+	.owner =	THIS_MODULE,
+
+	.class_attrs =	gpio_class_attrs,
+};
+
+
+/**
+ * gpio_export - export a GPIO through sysfs
+ * @gpio: gpio to make available, already requested
+ * Context: arch_initcall or later
+ *
+ * When drivers want to make a GPIO accessible to userspace after they
+ * have requested it -- perhaps while debugging, or as part of their
+ * public interface -- they may use this routine.
+ *
+ * Returns zero on success, else an error.
+ */
+int gpio_export(unsigned gpio)
+{
+	unsigned long		flags;
+	struct gpio_desc	*desc;
+	int			status = -EINVAL;
+
+	/* REVISIT param to say if userspace may change direction? */
+
+	/* can't export until sysfs is available ... */
+	if (!gpio_class.children.next) {
+		pr_debug("%s: called too early!\n", __func__);
+		return -ENOENT;
+	}
+
+	if (!gpio_is_valid(gpio))
+		goto done;
+
+	mutex_lock(&sysfs_lock);
+
+	spin_lock_irqsave(&gpio_lock, flags);
+	desc = &gpio_desc[gpio];
+	if (test_bit(FLAG_REQUESTED, &desc->flags)
+			&& !test_bit(FLAG_EXPORT, &desc->flags))
+		status = 0;
+	spin_unlock_irqrestore(&gpio_lock, flags);
+
+	if (status == 0) {
+		struct device	*dev;
+
+		dev = device_create(&gpio_class, desc->chip->dev, 0,
+				"gpio%d", gpio);
+		if (dev) {
+			dev_set_drvdata(dev, desc);
+			status = sysfs_create_group(&dev->kobj,
+					&gpio_attr_group);
+		} else
+			status = -ENODEV;
+		if (status == 0)
+			set_bit(FLAG_EXPORT, &desc->flags);
+	}
+
+	mutex_unlock(&sysfs_lock);
+
+done:
+	if (status)
+		pr_debug("%s: gpio%d status %d\n", __func__, gpio, status);
+
+	return status;
+}
+EXPORT_SYMBOL_GPL(gpio_export);
+
+static int match_export(struct device *dev, void *data)
+{
+	return dev_get_drvdata(dev) == data;
+}
+
+/**
+ * gpio_unexport - reverse effect of gpio_export()
+ * @gpio: gpio to make unavailable
+ *
+ * This is implicit on gpio_free().
+ */
+void gpio_unexport(unsigned gpio)
+{
+	struct gpio_desc	*desc;
+	int			status = -EINVAL;
+
+	if (!gpio_is_valid(gpio))
+		goto done;
+
+	mutex_lock(&sysfs_lock);
+
+	desc = &gpio_desc[gpio];
+	if (test_bit(FLAG_EXPORT, &desc->flags)) {
+		struct device	*dev = NULL;
+
+		dev = class_find_device(&gpio_class, desc, match_export);
+		if (dev) {
+			clear_bit(FLAG_EXPORT, &desc->flags);
+			put_device(dev);
+			device_unregister(dev);
+			status = 0;
+		} else
+			status = -ENODEV;
+	}
+
+	mutex_unlock(&sysfs_lock);
+done:
+	if (status)
+		pr_debug("%s: gpio%d status %d\n", __func__, gpio, status);
+}
+EXPORT_SYMBOL_GPL(gpio_unexport);
+
+static int gpiochip_export(struct gpio_chip *chip)
+{
+	int		status;
+	struct device	*dev;
+
+	/* Many systems register gpio chips for SOC support very early,
+	 * before driver model support is available.  In those cases we
+	 * export this later, in gpiolib_sysfs_init()
+	 */
+	if (!gpio_class.children.next)
+		return 0;
+
+	/* use chip->base for the ID; it's already known to be unique */
+	mutex_lock(&sysfs_lock);
+	dev = device_create(&gpio_class, chip->dev, 0,
+			"gpiochip%d", chip->base);
+	if (dev) {
+		dev_set_drvdata(dev, chip);
+		status = sysfs_create_group(&dev->kobj,
+				&gpiochip_attr_group);
+	} else
+		status = -ENODEV;
+	chip->exported = (status == 0);
+	mutex_unlock(&sysfs_lock);
+
+	if (status) {
+		unsigned long	flags;
+		unsigned	gpio;
+
+		spin_lock_irqsave(&gpio_lock, flags);
+		gpio = chip->base;
+		while (gpio_desc[gpio].chip == chip)
+			gpio_desc[gpio++].chip = NULL;
+		spin_unlock_irqrestore(&gpio_lock, flags);
+
+		pr_debug("%s: chip %s status %d\n", __func__,
+				chip->label, status);
+	}
+
+	return status;
+}
+
+static void gpiochip_unexport(struct gpio_chip *chip)
+{
+	int			status;
+	struct device		*dev;
+
+	mutex_lock(&sysfs_lock);
+	dev = class_find_device(&gpio_class, chip, match_export);
+	if (dev) {
+		put_device(dev);
+		device_unregister(dev);
+		chip->exported = 0;
+		status = 0;
+	} else
+		status = -ENODEV;
+	mutex_unlock(&sysfs_lock);
+
+	if (status)
+		pr_debug("%s: chip %s status %d\n", __func__,
+				chip->label, status);
+}
+
+static int __init gpiolib_sysfs_init(void)
+{
+	int		status;
+	unsigned long	flags;
+	unsigned	gpio;
+
+	status = class_register(&gpio_class);
+	if (status < 0)
+		return status;
+
+	/* Scan and register the gpio_chips which registered very
+	 * early (e.g. before the class_register above was called).
+	 *
+	 * Note that we run at arch_initcall() to help ensure that
+	 * the chip->dev devices have had a fair chance to register.
+	 */
+	spin_lock_irqsave(&gpio_lock, flags);
+	for (gpio = 0; gpio < ARCH_NR_GPIOS; gpio++) {
+		struct gpio_chip	*chip;
+
+		chip = gpio_desc[gpio].chip;
+		if (!chip || chip->exported)
+			continue;
+
+		spin_unlock_irqrestore(&gpio_lock, flags);
+		status = gpiochip_export(chip);
+		spin_lock_irqsave(&gpio_lock, flags);
+	}
+	spin_unlock_irqrestore(&gpio_lock, flags);
+
+
+	return status;
+}
+arch_initcall(gpiolib_sysfs_init);
+
+#else
+static inline int gpiochip_export(struct gpio_chip *chip)
+{
+	return 0;
+}
+
+static inline void gpiochip_unexport(struct gpio_chip *chip)
+{
+}
+
+#endif /* CONFIG_GPIO_SYSFS */
+
 /**
  * gpiochip_add() - register a gpio_chip
  * @chip: the chip to register, with chip->base initialized
@@ -160,6 +611,11 @@ err:
  * because the chip->base is invalid or already associated with a
  * different chip.  Otherwise it returns zero as a success code.
  *
+ * When gpiochip_add() is called very early during boot, so that GPIOs
+ * can be freely used, the chip->dev device must be registered before
+ * the gpio framework's arch_initcall().  Otherwise sysfs initialization
+ * for GPIOs will fail rudely.
+ *
  * If chip->base is negative, this requests dynamic assignment of
  * a range of valid GPIOs.
  */
@@ -182,7 +638,7 @@ int gpiochip_add(struct gpio_chip *chip)
 		base = gpiochip_find_base(chip->ngpio);
 		if (base < 0) {
 			status = base;
-			goto fail_unlock;
+			goto unlock;
 		}
 		chip->base = base;
 	}
@@ -201,8 +657,10 @@ int gpiochip_add(struct gpio_chip *chip)
 		}
 	}
 
-fail_unlock:
+unlock:
 	spin_unlock_irqrestore(&gpio_lock, flags);
+	if (status == 0)
+		status = gpiochip_export(chip);
 fail:
 	/* failures here can mean systems won't boot... */
 	if (status)
@@ -234,6 +692,7 @@ int gpiochip_remove(struct gpio_chip *ch
 		}
 	}
 	if (status == 0) {
+		gpiochip_unexport(chip);
 		for (id = chip->base; id < chip->base + chip->ngpio; id++)
 			gpio_desc[id].chip = NULL;
 	}
@@ -296,6 +755,8 @@ void gpio_free(unsigned gpio)
 		return;
 	}
 
+	gpio_unexport(gpio);
+
 	spin_lock_irqsave(&gpio_lock, flags);
 
 	desc = &gpio_desc[gpio];
@@ -534,10 +995,6 @@ EXPORT_SYMBOL_GPL(gpio_set_value_canslee
 
 #ifdef CONFIG_DEBUG_FS
 
-#include <linux/debugfs.h>
-#include <linux/seq_file.h>
-
-
 static void gpiolib_dbg_show(struct seq_file *s, struct gpio_chip *chip)
 {
 	unsigned		i;
--- a/drivers/gpio/mcp23s08.c	2008-04-30 13:00:03.000000000 -0700
+++ b/drivers/gpio/mcp23s08.c	2008-04-30 13:01:47.000000000 -0700
@@ -239,6 +239,7 @@ static int mcp23s08_probe(struct spi_dev
 	mcp->chip.base = pdata->base;
 	mcp->chip.ngpio = 8;
 	mcp->chip.can_sleep = 1;
+	mcp->chip.dev = &spi->dev;
 	mcp->chip.owner = THIS_MODULE;
 
 	spi_set_drvdata(spi, mcp);
--- a/drivers/gpio/pca953x.c	2008-04-30 13:00:03.000000000 -0700
+++ b/drivers/gpio/pca953x.c	2008-04-30 13:01:47.000000000 -0700
@@ -185,6 +185,7 @@ static void pca953x_setup_gpio(struct pc
 	gc->base = chip->gpio_start;
 	gc->ngpio = gpios;
 	gc->label = chip->client->name;
+	gc->dev = &chip->client->dev;
 	gc->owner = THIS_MODULE;
 }
 
--- a/drivers/gpio/pcf857x.c	2008-04-30 13:00:03.000000000 -0700
+++ b/drivers/gpio/pcf857x.c	2008-04-30 13:01:47.000000000 -0700
@@ -175,6 +175,7 @@ static int pcf857x_probe(struct i2c_clie
 
 	gpio->chip.base = pdata->gpio_base;
 	gpio->chip.can_sleep = 1;
+	gpio->chip.dev = &client->dev;
 	gpio->chip.owner = THIS_MODULE;
 
 	/* NOTE:  the OnSemi jlc1562b is also largely compatible with
--- a/drivers/i2c/chips/tps65010.c	2008-04-30 13:00:03.000000000 -0700
+++ b/drivers/i2c/chips/tps65010.c	2008-04-30 13:01:47.000000000 -0700
@@ -636,6 +636,8 @@ static int tps65010_probe(struct i2c_cli
 		tps->outmask = board->outmask;
 
 		tps->chip.label = client->name;
+		tps->chip.dev = &client->dev;
+		tps->chip.owner = THIS_MODULE;
 
 		tps->chip.set = tps65010_gpio_set;
 		tps->chip.direction_output = tps65010_output;
--- a/drivers/mfd/htc-egpio.c	2008-04-30 13:00:03.000000000 -0700
+++ b/drivers/mfd/htc-egpio.c	2008-04-30 13:01:47.000000000 -0700
@@ -318,6 +318,8 @@ static int __init egpio_probe(struct pla
 		ei->chip[i].dev = &(pdev->dev);
 		chip = &(ei->chip[i].chip);
 		chip->label           = "htc-egpio";
+		chip->dev             = &pdev->dev;
+		chip->owner           = THIS_MODULE;
 		chip->get             = egpio_get;
 		chip->set             = egpio_set;
 		chip->direction_input = egpio_direction_input;
--- a/include/asm-generic/gpio.h	2008-04-30 13:00:03.000000000 -0700
+++ b/include/asm-generic/gpio.h	2008-04-30 13:01:47.000000000 -0700
@@ -28,6 +28,8 @@ struct module;
 /**
  * struct gpio_chip - abstract a GPIO controller
  * @label: for diagnostics
+ * @dev: optional device providing the GPIOs
+ * @owner: helps prevent removal of modules exporting active GPIOs
  * @direction_input: configures signal "offset" as input, or returns error
  * @get: returns value for signal "offset"; for output signals this
  *	returns either the value actually sensed, or zero
@@ -55,6 +57,7 @@ struct module;
  */
 struct gpio_chip {
 	char			*label;
+	struct device		*dev;
 	struct module		*owner;
 
 	int			(*direction_input)(struct gpio_chip *chip,
@@ -70,6 +73,7 @@ struct gpio_chip {
 	int			base;
 	u16			ngpio;
 	unsigned		can_sleep:1;
+	unsigned		exported:1;
 };
 
 extern const char *gpiochip_is_requested(struct gpio_chip *chip,
@@ -104,7 +108,18 @@ extern void __gpio_set_value(unsigned gp
 extern int __gpio_cansleep(unsigned gpio);
 
 
-#else
+#ifdef CONFIG_GPIO_SYSFS
+
+/*
+ * 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);
+extern void gpio_unexport(unsigned gpio);
+
+#endif	/* CONFIG_GPIO_SYSFS */
+
+#else	/* !CONFIG_HAVE_GPIO_LIB */
 
 static inline int gpio_is_valid(int number)
 {
@@ -133,6 +148,17 @@ static inline void gpio_set_value_cansle
 	gpio_set_value(gpio, value);
 }
 
-#endif
+#endif /* !CONFIG_HAVE_GPIO_LIB */
+
+#ifndef CONFIG_GPIO_SYSFS
+static inline int gpio_export(unsigned gpio)
+{
+	return -ENOSYS;
+}
+
+static inline void gpio_unexport(unsigned gpio)
+{
+}
+#endif	/* CONFIG_GPIO_SYSFS */
 
 #endif /* _ASM_GENERIC_GPIO_H */

Re: [patch/rfc 2.6.25-git v2] gpio: sysfs interface

[Trent Piepho]

On Wed, 30 Apr 2008, David Brownell wrote:
> Simple sysfs interface for GPIOs.
>
>    /sys/class/gpio
>    	/control ... to request a GPIO be imported or returned
>        /gpioN ... for each exported GPIO #N
> 	    /value ... always readable, writes fail for input GPIOs
> 	    /direction ... r/w as: in, out (default low); write high, low
> 	/gpiochipN ... for each gpiochip; #N is its first GPIO
> 	    /base ... (r/o) same as N
> 	    /label ... (r/o) descriptive, not necessarily unique
> 	    /ngpio ... (r/o) number of GPIOs; numbered N .. N+(ngpio - 1)

"simple"?  What I had was a lot simpler.

So, I want to set gpio 5 on a pcf9557 extender.

cat 1 > /sys/class/gpio/pcf9557-0:0

But now I can't do this anymore, it has to be harder.  So how do I do it?  It
doesn't seem very considerate to ignore the real use cases of the person who
wrote the code to begin with.

> GPIOs may be exported by kernel code using gpio_export(), which should
> be most useful for driver debugging.  Userspace may also ask that they

I have a JTAG interface implemented via GPIOs.  With my system, one can see
the gpios used in sysfs with the proper labels (TCK, TDO, TDI, etc.).  This is
very helpful for people connecting something to the interface to know they
have the write gpio lines connected.  What's the point of allowing
one to label gpio lines if it's not going to be easy to see?

It also means that if they have trouble getting what their connecting to work,
they can always control the line via sysfs and see with a probe that it
changes.  They can raise TRST (which they know is the right line from the
label) and see the system reset.

Adding gpio_export() calls to the kernel source and recompiling and
re-flashing just isn't going to happen for a large portion of users.

Why can't the gpio lines just show up when something requests them?

> be imported by writing to the sysfs control file, helping to cope with
> incomplete board support:
>
>  echo "23" > /sys/class/gpio/control
> 	... will gpio_request(23, "sysfs") and gpio_export(23); use

So if a driver is already using gpio 23, then there is no way to see it in
sysfs, since the gpio_request() will fail?

> 	/sys/class/gpio/gpio-23/direction to configure it.
>  echo "-23" > /sys/class/gpio/control
> 	... will gpio_free(23)

So if a driver was already using gpio 23 and you wanted to look at it from
userspace, you'll free it from both sysfs and the driver that was using it
when you're done?

> +When a kernel driver has requested a GPIO, it may only be made available
> +in the sysfs interface by gpio_export().  This prevents userspace code
> +from clobbering important state.

You could say the same about 90% of the writable files in sysfs.

Re: [patch/rfc 2.6.25-git v2] gpio: sysfs interface

[Ben Nizette]

On Wed, 2008-04-30 at 15:47 -0700, Trent Piepho wrote:
> On Wed, 30 Apr 2008, David Brownell wrote:
> > Simple sysfs interface for GPIOs.
> >
> >    /sys/class/gpio
> >    	/control ... to request a GPIO be imported or returned
> >        /gpioN ... for each exported GPIO #N
> > 	    /value ... always readable, writes fail for input GPIOs
> > 	    /direction ... r/w as: in, out (default low); write high, low
> > 	/gpiochipN ... for each gpiochip; #N is its first GPIO
> > 	    /base ... (r/o) same as N
> > 	    /label ... (r/o) descriptive, not necessarily unique
> > 	    /ngpio ... (r/o) number of GPIOs; numbered N .. N+(ngpio - 1)
> 
> "simple"?  What I had was a lot simpler.
> 
> So, I want to set gpio 5 on a pcf9557 extender.
> 
> cat 1 > /sys/class/gpio/pcf9557-0:0
> 
> But now I can't do this anymore, it has to be harder.  So how do I do it?

We've discussed the down sides of your model.  It's got up sides, eg
your use case is made very easy, but it ain't all roses.

Now, if the kernel has requested that pin and hasn't exported it you
can't touch it.  This is what we want.

Otherwise you will walk the gpiochips, find your chip's base and use it
to calculate the gpio number.  If the kernel's exported it then the file
will be there to use.  If the kernel hasn't requested it then you can do
so by using the control file.

Yes it's longer and harder and more convoluted.  It's also much more
safe.

>   It
> doesn't seem very considerate to ignore the real use cases of the person who
> wrote the code to begin with.
> 
> > GPIOs may be exported by kernel code using gpio_export(), which should
> > be most useful for driver debugging.  Userspace may also ask that they
> 
> I have a JTAG interface implemented via GPIOs.  With my system, one can see
> the gpios used in sysfs with the proper labels (TCK, TDO, TDI, etc.).  This is
> very helpful for people connecting something to the interface to know they
> have the write gpio lines connected.  What's the point of allowing
> one to label gpio lines if it's not going to be easy to see?

That label was always just supposed to be a debugging aid, i.e.
something to show up in debugfs.  This is used to reduce D footprint.
Maybe if the labels are being stored anyway they can be made available
through sysfs as well as debugfs?

> 
> It also means that if they have trouble getting what their connecting to work,
> they can always control the line via sysfs and see with a probe that it
> changes.  They can raise TRST (which they know is the right line from the
> label) and see the system reset.

That's very useful indeed, but if you don't want userspace to be able to
do that it's also dangerous.  This is why the kernel has to explicitly
allow it.

> 
> Adding gpio_export() calls to the kernel source and recompiling and
> re-flashing just isn't going to happen for a large portion of users.
> 
> Why can't the gpio lines just show up when something requests them?

So you want userspace to be able to clobber any gpio which is used by
the kernel?  That's what this version of the interface is designed to
avoid!

> 
> > be imported by writing to the sysfs control file, helping to cope with
> > incomplete board support:
> >
> >  echo "23" > /sys/class/gpio/control
> > 	... will gpio_request(23, "sysfs") and gpio_export(23); use
> 
> So if a driver is already using gpio 23, then there is no way to see it in
> sysfs, since the gpio_request() will fail?

The way to see it in sysfs is to have the kernel export it.  This is the
point.  The kernel should request it for it's own use then export it if
it wants userspace to be able to touch it as well.

> 
> > 	/sys/class/gpio/gpio-23/direction to configure it.
> >  echo "-23" > /sys/class/gpio/control
> > 	... will gpio_free(23)
> 
> So if a driver was already using gpio 23 and you wanted to look at it from
> userspace, you'll free it from both sysfs and the driver that was using it
> when you're done?

If driver was using it, it will have requested it.  If you are looking
at it then the kernel must have also exported it.  If the kernel's
exported it then nothing you can do will unexport it.  You can only
unexport gpios you've exported yourself manually and the only gpios you
can do this to are ones unused by the driver.

> 
> > +When a kernel driver has requested a GPIO, it may only be made available
> > +in the sysfs interface by gpio_export().  This prevents userspace code
> > +from clobbering important state.
> 
> You could say the same about 90% of the writable files in sysfs.

Re: [patch/rfc 2.6.25-git v2] gpio: sysfs interface

[Ben Nizette]

On Wed, 2008-04-30 at 14:34 -0700, David Brownell wrote:
> Simple sysfs interface for GPIOs.
> 
>     /sys/class/gpio
>     	/control ... to request a GPIO be imported or returned
>         /gpioN ... for each exported GPIO #N
> 	    /value ... always readable, writes fail for input GPIOs
> 	    /direction ... r/w as: in, out (default low); write high, low
> 	/gpiochipN ... for each gpiochip; #N is its first GPIO
> 	    /base ... (r/o) same as N
> 	    /label ... (r/o) descriptive, not necessarily unique
> 	    /ngpio ... (r/o) number of GPIOs; numbered N .. N+(ngpio - 1)

You dropped the 'device' symlink?  Sure it won't always be available but
I did quite like the idea of being able to walk back through sysfs and
discover, for example, the SPI chip select to which it's attached.

> 
> GPIOs may be exported by kernel code using gpio_export(), which should
> be most useful for driver debugging.  Userspace may also ask that they
> be imported by writing to the sysfs control file, helping to cope with
> incomplete board support:
> 
>   echo "23" > /sys/class/gpio/control
> 	... will gpio_request(23, "sysfs") and gpio_export(23); use
> 	/sys/class/gpio/gpio-23/direction to configure it.
>   echo "-23" > /sys/class/gpio/control
> 	... will gpio_free(23)
> 
> The extra D-space footprint is a few hundred bytes, except for the sysfs
> resources associated with each exported GPIO.  The additional I-space
> footprint is about two thirds of the current size of gpiolib (!).  Since
> no /dev node creation is involved, no "udev" support is needed.
> 
> This adds a device pointer to "struct gpio_chip".  When GPIO providers
> initialize that, sysfs gpio class devices become children of that device
> instead of being "virtual" devices.  The (few) gpio_chip providers which
> have such a device node have been updated.  (Some also needed to update
> their module "owner" field ... for which missing kerneldoc was added.)
> 
> Based on a patch from Trent Piepho <tpiepho@freescale.com>, and comments
> from various folk including most recently Hartley Sweeten and Ben Nizette.
> 
> Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
> ---
> Notable updates from previous version:  doc update, mutex protection
> for sysfs access, export basic gpio_chip info ("what GPIOs exist"),
> depend on new sysfs_streq() call, simplified control file syntax.
> 
[...]
> +
> +/*
> + * /sys/class/gpio/control ... write-only
> + *	integer N:  non-negative == export; negative == unexport
> + */
> +static ssize_t control_store(struct class *class, const char *buf, size_t len)
> +{
> +	long	gpio;
> +	int	status;
> +
> +	status = strict_strtol(buf, 0, &gpio);
> +	if (status < 0)
> +		goto done;
> +
> +	/* No extra locking here; FLAG_SYSFS just signifies that the
> +	 * request and export were done by on behalf of userspace, so
> +	 * they may be undone on its behalf too.
> +	 */
> +
> +	if (gpio >= 0) {			/* export */
> +		status = gpio_request(gpio, "sysfs");
> +		if (status < 0)
> +			goto done;
> +
> +		status = gpio_export(gpio);
> +		if (status < 0)
> +			gpio_free(gpio);
> +		else
> +			set_bit(FLAG_SYSFS, &gpio_desc[gpio].flags);
> +
> +	} else {				/* unexport */

Unexport gpio 0?

> +		gpio = -gpio;
> +
> +		/* reject bogus commands (gpio_unexport ignores them) */
> +		if (!gpio_is_valid(gpio))
> +			goto fail;
> +		if (!test_and_clear_bit(FLAG_SYSFS, &gpio_desc[gpio].flags))
> +			goto fail;
> +		status = 0;
> +		gpio_free(gpio);
> +	}
> +done:
> +	if (status)
> +		pr_debug("%s: status %d\n", __func__, status);
> +	return status ? : len;
> +fail:
> +	pr_debug("%s: fail\n", __func__);
> +	return -EINVAL;
> +}
> +

Re: [patch/rfc 2.6.25-git v2] gpio: sysfs interface

[David Brownell]

On Wednesday 30 April 2008, Trent Piepho wrote:
> On Wed, 30 Apr 2008, David Brownell wrote:
> > Simple sysfs interface for GPIOs.
> >
> >    /sys/class/gpio
> >    	...
> 
> "simple"?  What I had was a lot simpler.

But it had some "must fix" problems, which I told you when you
posted your first patch.  You didn't fix them.  And since then,
your pushback seems to rely very heavily on ignoring issues I
pointed out are "must fix" or "can't/doesn't work that way".

It's easy to be simple if you don't have to be correct or safe.


> So, I want to set gpio 5 on a pcf9557 extender.

Which isn't exactly where most folk start.  If it's something
done routinely, they'll likely have a script to run; either
the native language type, or something for bash or Python.

If it's a one-off, they probably start from schematics and
are fully capable of adding 5 to the base number associated
with the particular pcf chip in question (say, the third one
on this card stack) ... and doing the same for other pins
they need to diddle.  Note taking is always a good practice
when sorting out such issues, but adding five is in your head
is far from rocket science.


> cat 1 > /sys/class/gpio/pcf9557-0:0
> 
> But now I can't do this anymore,

"Any more"?  You never *could* do that before.  The original
patch you sent didn't do that, and the last code snippet I
saw from you didn't either.


> it has to be harder.  So how do I do it?  It 
> doesn't seem very considerate to ignore the real use cases

I didn't ignore *use cases* at all.  You can certainly set
the value of such a GPIO with the $SUBJECT interface.

Just ... not using that syntax.  Use cases are several steps
above solution details, such as a particular syntax.


> of the person who wrote the code to begin with.

I've seen at least half a dozen different userspace models for
how to work with GPIOs.  You didn't write all of them.  Nor
were yours the only use cases.


> 	 What's the point of allowing
> one to label gpio lines if it's not going to be easy to see?

You can "cat /sys/kernel/debug/gpio" to see them...


> >  echo "23" > /sys/class/gpio/control
> > 	... will gpio_request(23, "sysfs") and gpio_export(23); use
> 
> So if a driver is already using gpio 23, then there is no way to see it in
> sysfs, since the gpio_request() will fail?

Right.  If that driver wanted to cope with userspace mucking
around with its internal state (GPIOs) it would have explicitly
enabled it by calling gpio_export().  If it's not prepared to
cope with the various flavors of breakage that would facilitate,
it doesn't call gpio_export() ... and is safe.


> > 	/sys/class/gpio/gpio-23/direction to configure it.
> >  echo "-23" > /sys/class/gpio/control
> > 	... will gpio_free(23)
> 
> So if a driver was already using gpio 23 and you wanted to look at it from
> userspace, you'll free it from both sysfs and the driver that was using it
> when you're done?

No, that was the one-line summary.  It's only freed if it was
originally requested from userspace (by "echo 23 > control").
As you can see in the code...

- Dave

Re: [patch/rfc 2.6.25-git v2] gpio: sysfs interface

[David Brownell]

> > 	 This is
> > very helpful for people connecting something to the interface to know they
> > have the write gpio lines connected.  What's the point of allowing
> > one to label gpio lines if it's not going to be easy to see?
> 
> That label was always just supposed to be a debugging aid, i.e.
> something to show up in debugfs.  This is used to reduce D footprint.
> Maybe if the labels are being stored anyway they can be made available
> through sysfs as well as debugfs?

They could be; I've had code to do it.  It never stays in
very long, because for a pure userspace interace it can't
be helpful unless userspace makes the labels:  the labels
displayed otherwise just reuse the same constant.

For a "kernel cooperates with userspace" model that might
be more useful.  And I've certainly found the debugfs info
hard to interpret without such labels!!  But Trent has
said he doesn't see anyone modifying enough code to adopt
such a "cooperates" model.  Which is why this particular
side-argument seems like a waste of time to me:  even he
has not proposed the key change his argument depends on.


The original control file syntax I thought about was

	export NN mode label

where it would gpio_request(NN, "label") and then set the
direction (and maybe value) according to "mode".  But that
was just a PITA to cope with ... so it's now been trimmed
down to just "NN" (and "-NN").

- Dave

Re: [patch/rfc 2.6.25-git v2] gpio: sysfs interface

[Trent Piepho]

On Wed, 30 Apr 2008, David Brownell wrote:
> On Wednesday 30 April 2008, Trent Piepho wrote:
>> On Wed, 30 Apr 2008, David Brownell wrote:
>>> Simple sysfs interface for GPIOs.
>>>
>>>    /sys/class/gpio
>>>    	...
>>
>> "simple"?  What I had was a lot simpler.
>
> But it had some "must fix" problems, which I told you when you
> posted your first patch.  You didn't fix them.  And since then,
> your pushback seems to rely very heavily on ignoring issues I
> pointed out are "must fix" or "can't/doesn't work that way".

So what are these must fix problems?  You don't want all the gpios to appear
in sysfs by default in case there are many.  So have them only appear when
they are requested by something.  Problem solved.

Someone might label the chip with a label using a '/'.  First of all, that's
nothing but speculation on a problem that will probably never happen.  Do any
existing chip labels use a '/'?  Is it really that hard to just not label the
chip that way?  But suppose someone just can't avoid it, one line of code,
that I posted, is all it takes to fix this.

Multiple chips with the same label.  That's more annoying, but still
easy to fix in a number of ways:
1) If the label isn't unique, don't register that chip with sysfs.
2) If the label isn't unique, appended a suffix to make it so.
3) Always add a sequence number the end of the chip label.  This is quite
common in fact.

The sysfs control interface won't be able to handle gpios named this way.  Or
doing so will be "too hard".  Not true, I wrote and posted code that did it. 
It was simpler than the initial code you wrote for the control interface, and
much simpler than the latest with all the gpiochip stuff.  BTW, my code also
worked for unexporting gpio 0....

You say nothing in sysfs works this way, but I don't agree.  Take a look at
/sys/class/net, you have names like "tap0", "eth0", "eth1", "lo", etc.  This
is exactly what I'm saying to do.  The first "eth" gpio chip you register is
appears as eth0 in sysfs, the second as eth1, and so on.

The directories are not just named "netdev0", "netdev1", "netdev2", with some
sort of mapping file telling you ethernet devices are using numbers 2-3.

Look at PCI, you have files like "/sys/bus/pci/devices/0000:01:05.0" and
"/bus/pci/devices/0000:00:0d.2".  It's not "pci1", "pci2", "pci3", with a
"bus0" directory telling you that the bus starts at device 3 and has 6
devices.

Look at ACPI, it's /sys/bus/acpi/devices/{LNXSLPBN:00,LNXPWRBN:00,LNXSYSTM:00}
and "device:00", "device:01".  It uses useful names before the number when
they exist, and just "device" when it doesn't.  This is what I'm saying to do
for gpios.

In fact, what you want - just naming gpios from each device sequentially as
the are dynamically assigned, and then having another set of directories that
allow one to calculate what which one belongs to a given chip - seems to be a
lot harder to find an existing example of.

>> So, I want to set gpio 5 on a pcf9557 extender.
>
> Which isn't exactly where most folk start.  If it's something

It's where I started.  You say your system works for everything that matters
and just dismiss the problems I'm solving as irrelevant with a wave of your
hand.  That's very convenient for you, but where does it leave me?

> done routinely, they'll likely have a script to run; either
> the native language type, or something for bash or Python.

Your worried about the memory usage of some extra sysfs files, but include
python on your device?  IMHO, if you can't do this simple task in 5 minutes
with just busybox, the system is making things too hard.

>> cat 1 > /sys/class/gpio/pcf9557-0:0
>>
>> But now I can't do this anymore,
>
> "Any more"?  You never *could* do that before.  The original
> patch you sent didn't do that, and the last code snippet I
> saw from you didn't either.

I seem to be doing it just fine.  I never posted the latest version of my
code since you didn't respond to my April 7th email.

>> 	 What's the point of allowing
>> one to label gpio lines if it's not going to be easy to see?
>
> You can "cat /sys/kernel/debug/gpio" to see them...

What is the reason to not have the label with gpio in sysfs?

>>>  echo "23" > /sys/class/gpio/control
>>> 	... will gpio_request(23, "sysfs") and gpio_export(23); use
>>
>> So if a driver is already using gpio 23, then there is no way to see it in
>> sysfs, since the gpio_request() will fail?
>
> Right.  If that driver wanted to cope with userspace mucking
> around with its internal state (GPIOs) it would have explicitly
> enabled it by calling gpio_export().  If it's not prepared to
> cope with the various flavors of breakage that would facilitate,
> it doesn't call gpio_export() ... and is safe.

How does seeing the value, direction, and label of a gpio "muck around with
its internal state?"

Of course, one can still change a gpio via /dev/mem or i2c-dev, depending on
the source.  How is being able to do this via sysfs any different?  Yes, the
user could mess the driver up, if they don't know what they are doing.  Just
like there are a hundred other ways to do this via raw device access,
/dev/mem, i2c-dev, and so on.  I guess instead of looking down on everyone and
assuming no one else knows what they are doing and must be protected, I prefer
to assume users aren't idiots, and if they can be trusted with /dev/hda they
can be trusted with gpios in sysfs.

Re: [patch/rfc 2.6.25-git v2] gpio: sysfs interface

[David Brownell]

On Wednesday 30 April 2008, Trent Piepho wrote:
> >>>    /sys/class/gpio
> >>>    	...
> >>
> >> "simple"?  What I had was a lot simpler.
> >
> > But it had some "must fix" problems, which I told you when you
> > posted your first patch.  You didn't fix them.  And since then,
> > your pushback seems to rely very heavily on ignoring issues I
> > pointed out are "must fix" or "can't/doesn't work that way".
> 
> So what are these must fix problems? 

Not fixed in any update you sent to your original patch, in
the past three weeks, that's the key point.  In particular,
the number one issue was completely ignored.  (Your recent
responses haven't come across to me as helpful either..)


> You say nothing in sysfs works this way, but I don't agree.  Take a look at
> /sys/class/net, you have names like "tap0", "eth0", "eth1", "lo", etc.  This
> is exactly what I'm saying to do.  The first "eth" gpio chip you register is
> appears as eth0 in sysfs, the second as eth1, and so on.

So, when I said that sysfs doesn't use names like "gpio-19" but instead
uses names like "gpio19" ... exactly what don't you agree with?

Your examples prove what I *did* say.  I don't know who you're trying
to argue with on other points, however.


> >> So, I want to set gpio 5 on a pcf9557 extender.
> >
> > Which isn't exactly where most folk start.  If it's something
> 
> It's where I started.

Which is fine.  The $SUBJECT patch provides a simple solution for it.
As well as for the other use cases I sketched.  With identifiers that
are much simpler, and harder to get wrong.


> You say your system works for everything that matters 
> and just dismiss the problems I'm solving as irrelevant with a wave of your
> hand.  That's very convenient for you, but where does it leave me?

Those are *NOT* my words.  Again, I don't know what straw man you
are arguing with, but please stop painting my face on it, putting
your words in its mouth, and using that to claim you're disproving
something I've "said" (but haven't).

I did say your problem could be solved ... but with different
identifier syntax than you suggested.


> Your worried about the memory usage of some extra sysfs files, but include
> python on your device?  IMHO, if you can't do this simple task in 5 minutes
> with just busybox, the system is making things too hard.

Right, and since I *can* do stuff like that in busybox with ASH,
that quickly, I hope you'll agree your flamage has been blowing
things way out of proportion here.

 
> What is the reason to not have the label with gpio in sysfs?

Elaborated in my reply to Ben; see that.


> How does seeing the value, direction, and label of a gpio "muck around with
> its internal state?"

Userspace being able to MODIFY the direction and output value
is most certainly mucking around!!!


> Of course, one can still change a gpio via /dev/mem or i2c-dev, depending on
> the source.  How is being able to do this via sysfs any different?

It's a question of how easy you make it to break things.
You seem to draw the "too easy" line differently than I do.
I don't want *anything* else mucking around with state my
drivers are responsible for managing.

Re: [patch/rfc 2.6.25-git v2] gpio: sysfs interface

[Trent Piepho]

On Wed, 30 Apr 2008, David Brownell wrote:
> On Wednesday 30 April 2008, Trent Piepho wrote:
>>>>>    /sys/class/gpio
>>>>>    	...
>>>>
>>>> "simple"?  What I had was a lot simpler.
>>>
>>> But it had some "must fix" problems, which I told you when you
>>> posted your first patch.  You didn't fix them.  And since then,
>>> your pushback seems to rely very heavily on ignoring issues I
>>> pointed out are "must fix" or "can't/doesn't work that way".
>>
>> So what are these must fix problems?
>
> Not fixed in any update you sent to your original patch, in
> the past three weeks, that's the key point.  In particular,

Let's be fair about who was waiting for who.  You didn't like some things
about my original system, I proposed changes here,
http://article.gmane.org/gmane.linux.kernel/662862, on April 7th.  You didn't
respond until April 27th, then decided to take my patch and change it they way
you wanted, ignoring what I wrote it accomplish, the very next day.

> the number one issue was completely ignored.  (Your recent
> responses haven't come across to me as helpful either..)

And this issue is?

>> You say nothing in sysfs works this way, but I don't agree.  Take a look at
>> /sys/class/net, you have names like "tap0", "eth0", "eth1", "lo", etc.  This
>> is exactly what I'm saying to do.  The first "eth" gpio chip you register is
>> appears as eth0 in sysfs, the second as eth1, and so on.
>
> So, when I said that sysfs doesn't use names like "gpio-19" but instead
> uses names like "gpio19" ... exactly what don't you agree with?

Instead of gpio19, it should be the chip label followed by 19.  I want "eth0",
"tap0".  You want "net0", "net1", and then a whole extra system to figure out
what's eth and what's tap.

>> You say your system works for everything that matters
>> and just dismiss the problems I'm solving as irrelevant with a wave of your
>> hand.  That's very convenient for you, but where does it leave me?
>
> Those are *NOT* my words.  Again, I don't know what straw man you

 	"However, I think a slightly more common practice in current embedded
 	Linux systems is to build custom kernels that know which
 	daughtercard(s) are available.  That's mostly what gets pushed
 	upstream, anyway..."

 	"Which is why this particular side-argument seems like a waste of time
 	to me:"

 	"Which isn't exactly where most folk start."

>> Your worried about the memory usage of some extra sysfs files, but include
>> python on your device?  IMHO, if you can't do this simple task in 5 minutes
>> with just busybox, the system is making things too hard.
>
> Right, and since I *can* do stuff like that in busybox with ASH,
> that quickly, I hope you'll agree your flamage has been blowing
> things way out of proportion here.

It's so easy, yet no one posts the code to do it.

>
>
>> What is the reason to not have the label with gpio in sysfs?
>
> Elaborated in my reply to Ben; see that.

 	because for a pure userspace interace it can't be helpful unless
 	userspace makes the labels:

I just don't see that.  I've got a JTAG interface on the gpio pins.  The user
wants to see what gpio pin is assigned to what function.  The information
could be right there with the direction and value.  They could have a script
that searches the gpios for one with a matching label.

I've got a board revision that so far the kernel doesn't care about.  I
could put something like this in my init scripts:
for i in 0 1 2 3
do
    echo "Board Rev [bit $i]" > /sys/class/gpio/pca9557-1:$i/label
done

Yes, I could only have the information in some docs and force someone to look
it up every time.  But the gpios can have labels, so why not use them?

>> How does seeing the value, direction, and label of a gpio "muck around with
>> its internal state?"
>
> Userspace being able to MODIFY the direction and output value
> is most certainly mucking around!!!

Now you're the one with the straw-man.  I've proposed many times that gpios
that have been requested by anything be exported READ-ONLY automatically.  I
said "*seeing* the value", not "*changing* the value".  I've yet to see you
come up with any reason why this is bad.

>> Of course, one can still change a gpio via /dev/mem or i2c-dev, depending on
>> the source.  How is being able to do this via sysfs any different?
>
> It's a question of how easy you make it to break things.
> You seem to draw the "too easy" line differently than I do.
> I don't want *anything* else mucking around with state my
> drivers are responsible for managing.

Will allowing userspace to request a gpio used by the kernel allow one to break
things easier than:
cat /dev/zero > /dev/hda

I don't think so.  Do you think allowing root to use a gpio after root has
specifically exported it will allow someone to break their system more easily
and to a greater extent than that command?  Yet that command has been around
since the existence of Linux, and the world hasn't come to an end.  Has there
even been a patch to remove a block device from userspace access once a
filesystem is mounted on it?  No?  Well then it's obviously not too dangerous,
if it's been there all along an no one has cared enough to remote it.  And if
gpio access is less dangerous than someone which is not too dangerous, then
gpio can't be too dangerous either then, can it?

Suppose I want to modify a i2c gpio extender's gpio value.  I can do
that *right now* like this:

i2cset 0 0x18 1 255 b 0x10

This ability has been around since i2c gpio drivers were first written!  Even
non-root can do that if the permissions are such.  Again, the world has not
ended.  No one is concerned enough about those foolish non-kernel programmers
modifying a gpio from userspace that they are doing anything about it.

It simply is not logical that a complex multi-step process of locating a gpio,
manually exporting it, and changing the value will be too dangerous, when
simpler means of doing the same thing have always existed and will continue to
exist and have not caused any problems.

The existing i2cset interface would allow one to change an input to an output
or write to the complelely wrong device if just one bit is written incorrectly
in the command.  Did you forget the datasheet uses 8-bit address and linux
uses 7-bit?  Well, you just overwrote your eeprom and bricked your system.  If
only a sysfs based system that doesn't require one to calculate hex constants
in their head existed, that would easily prevent mistakes like that.

Re: [patch/rfc 2.6.25-git v2] gpio: sysfs interface

[David Brownell]

On Wednesday 30 April 2008, Ben Nizette wrote:
> 
> On Wed, 2008-04-30 at 14:34 -0700, David Brownell wrote:
> > Simple sysfs interface for GPIOs.
> > 
> > /sys/class/gpio
> >     /control ... to request a GPIO be imported or returned
> >     /gpioN ... for each exported GPIO #N
> > 	    /value ... always readable, writes fail for input GPIOs
> > 	    /direction ... r/w as: in, out (default low); write high, low
> > 	/gpiochipN ... for each gpiochip; #N is its first GPIO
> > 	    /base ... (r/o) same as N
> > 	    /label ... (r/o) descriptive, not necessarily unique
> > 	    /ngpio ... (r/o) number of GPIOs; numbered N .. N+(ngpio - 1)
> 
> You dropped the 'device' symlink?  Sure it won't always be available but
> I did quite like the idea of being able to walk back through sysfs and
> discover, for example, the SPI chip select to which it's attached.

Just didn't describe it in that summary ... like several other
standard attributes, like "uevent" and "subsystem".  The only
thing different about "device" is that when the gpio_chip doesn't
say what device it uses, that will (necessarily) be missing.
And again, that's just like all other class device nodes.


> > +/*
> > + * /sys/class/gpio/control ... write-only
> > + *	integer N:  non-negative == export; negative == unexport
> > + */
> > +static ssize_t control_store(struct class *class, const char *buf, size_t len)
> 
> Unexport gpio 0?

Yeah, gotta fix that.  Unfortunately "-0" == "0" ... I'll have to
handle any leading "-" by hand.  Trivial fix, in all.

- Dave

Re: [patch/rfc 2.6.25-git] gpio: sysfs interface

[Pavel Machek]

Hi!

> > Simple sysfs interface for GPIOs.
> > 
> >     /sys/class/gpio
> >         /gpio-N ... for each exported GPIO #N
> > 	    /value ... always readable, writes fail except for output GPIOs
> > 	    /direction ... writable as: in, out (default low), high, low
> >     	/control ... to request a GPIO be exported or unexported
> > 
> > GPIOs may be exported by kernel code using gpio_export(), which should
> > be most useful for driver debugging.  Userspace may also ask that they
> > be exported by writing to the sysfs control file, helping to cope with
> > incomplete board support:
> > 
> >   echo "export 23" > /sys/class/gpio/control
> > 	... will gpio_request(23, "sysfs") and gpio_export(23); use
> > 	/sys/class/gpio/gpio-23/direction to configure it.
> >   echo "unexport 23" > /sys/class/gpio/control
> > 	... will gpio_free(23)
> 
> hm, does ths sysfs one-value-per-file rule apply to writes?

Having seen ibm acpi interface... yes, that rule should be aplied for
writes, too.

What about mkdir gpio-N to export it?

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [patch/rfc 2.6.25-git v2] gpio: sysfs interface

[David Brownell]

On Thursday 01 May 2008, Trent Piepho wrote:
> >> 	IMHO, if you can't do this simple task in 5 minutes
> >> with just busybox, the system is making things too hard.
> >
> > Right, and since I *can* do stuff like that in busybox with ASH,
> > that quickly, I hope you'll agree your flamage has been blowing
> > things way out of proportion here.
> 
> It's so easy, yet no one posts the code to do it.

I think that's because it's a trivial "Shell Programming 101"
homework assignment, and we have a general policy against doing
people's schoolwork for them ...

Here are two clues for you:  "for DIR in gpiochip*"; and then
"LABEL=$(cat $DIR/label)".  I'm quite certain that if you put
aside your demonstrably strong desire to flame, you can solve
that little problem.

Or perhaps the issue here is just bikeshedding.  Regardless,
the signal-to-noise ratio is way too low..

Re: [patch/rfc 2.6.25-git v2] gpio: sysfs interface

[David Brownell]

On Thursday 01 May 2008, Trent Piepho wrote:
> I've proposed many times that gpios that have been
> requested by anything be exported READ-ONLY automatically.  

Hmm, maybe that was buried in the volume of other stuff you
posted ... this is the first time I recall your mentioning
read-only exports.  Or limiting exports like that.  Certainly
no code I've seen from you worked like that.

What I recall was your original approach of exporting absolutely
everything -- hundreds of (potential) GPIOs, even pins that
were configured for other purposes.

I don't have any particular objection to exporting things
readonly.  Maybe you noticed the comment in my patches that
suggesting such an export mode might be useful.  But I'd
rather not export things that don't need exporting, and
bias things towards low per-gpio costs.

It's easy to add features later, if they turn out to be needed.
But removing interfaces is very hard ... which is another reason
I'm not very receptive to all those bells'n'whistles you propose.

Re: [patch/rfc 2.6.25-git] gpio: sysfs interface

[David Brownell]

On Friday 02 May 2008, Pavel Machek wrote:
> 
> > >   echo "export 23" > /sys/class/gpio/control
> > >     ... will gpio_request(23, "sysfs") and gpio_export(23); use
> > >     /sys/class/gpio/gpio-23/direction to configure it.
> > >   echo "unexport 23" > /sys/class/gpio/control
> > >     ... will gpio_free(23)
> > 
> > hm, does ths sysfs one-value-per-file rule apply to writes?
> 
> Having seen ibm acpi interface... yes, that rule should be aplied for
> writes, too.

The latest version of this patch, which I'll post soonish,
follows the suggestion to have one file for the "export"
operation and another for "unexport".


> What about mkdir gpio-N to export it?

Ugh.  That would create way more complication than I want
to see here.  I thought about that early on, and decided
that was Not The Way To Go.

- Dave

[patch 2.6.26-rc2-git] gpio: sysfs interface

[David Brownell]

This adds a simple sysfs interface for GPIOs.

    /sys/class/gpio
    	/export ... asks the kernel to export a GPIO to userspace
    	/unexport ... to return a GPIO to the kernel
        /gpioN ... for each exported GPIO #N
	    /value ... always readable, writes fail for input GPIOs
	    /direction ... r/w as: in, out (default low); write high, low
	/gpiochipN ... for each gpiochip; #N is its first GPIO
	    /base ... (r/o) same as N
	    /label ... (r/o) descriptive, not necessarily unique
	    /ngpio ... (r/o) number of GPIOs; numbered N .. N+(ngpio - 1)

GPIOs claimed by kernel code may be exported by its owner using a new
gpio_export() call, which should be most useful for driver debugging.
Such exports may optionally be done without a "direction" attribute.

Userspace may ask to take over a GPIO by writing to a sysfs control
file, helping to cope with incomplete board support or other "one-off"
requirements that don't merit full kernel support:

  echo 23 > /sys/class/gpio/export
	... will gpio_request(23, "sysfs") and gpio_export(23);
	use /sys/class/gpio/gpio-23/direction to (re)configure it,
	when that GPIO can be used as both input and output.
  echo 23 > /sys/class/gpio/unexport
	... will gpio_free(23), when it was exported as above

The extra D-space footprint is a few hundred bytes, except for the sysfs
resources associated with each exported GPIO.  The additional I-space
footprint is about two thirds of the current size of gpiolib (!).  Since
no /dev node creation is involved, no "udev" support is needed.

Related changes:

  * This adds a device pointer to "struct gpio_chip".  When GPIO
    providers initialize that, sysfs gpio class devices become
    children of that device instead of being "virtual" devices.

  * The (few) gpio_chip providers which have such a device node
    have been updated.
    
  * Some gpio_chip drivers also needed to update their module
    "owner" field ... for which missing kerneldoc was added.

  * Some gpio_chips don't support input GPIOs.  Those GPIOs are
    now flagged appropriately when the chip is registered.

Based on previous patches, and discussion both on and off LKML.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
Updates since the last version:  split "control" file in two;
gpio_export() allows fixed-direction export; doc updates; cope
with minor driver model change (we must test a different field
to see if we're called too early in system init); added this to
stubs for gpio-deprived platforms in <linux/gpio.h>.

 Documentation/gpio.txt       |  123 +++++++++-
 arch/arm/plat-omap/gpio.c    |    3 
 arch/avr32/mach-at32ap/pio.c |    2 
 drivers/gpio/Kconfig         |   15 +
 drivers/gpio/gpiolib.c       |  509 ++++++++++++++++++++++++++++++++++++++++++-
 drivers/gpio/mcp23s08.c      |    1 
 drivers/gpio/pca953x.c       |    1 
 drivers/gpio/pcf857x.c       |    1 
 drivers/i2c/chips/tps65010.c |    2 
 drivers/mfd/htc-egpio.c      |    2 
 include/asm-generic/gpio.h   |   33 ++
 include/linux/gpio.h         |   13 +
 12 files changed, 689 insertions(+), 16 deletions(-)

--- g26.orig/Documentation/gpio.txt	2008-05-17 17:12:11.000000000 -0700
+++ g26/Documentation/gpio.txt	2008-05-17 17:15:04.000000000 -0700
@@ -347,15 +347,12 @@ necessarily be nonportable.
 Dynamic definition of GPIOs is not currently standard; for example, as
 a side effect of configuring an add-on board with some GPIO expanders.
 
-These calls are purely for kernel space, but a userspace API could be built
-on top of them.
-
 
 GPIO implementor's framework (OPTIONAL)
 =======================================
 As noted earlier, there is an optional implementation framework making it
 easier for platforms to support different kinds of GPIO controller using
-the same programming interface.
+the same programming interface.  This framework is called "gpiolib".
 
 As a debugging aid, if debugfs is available a /sys/kernel/debug/gpio file
 will be found there.  That will list all the controllers registered through
@@ -439,4 +436,120 @@ becomes available.  That may mean the de
 calls for that GPIO can work.  One way to address such dependencies is for
 such gpio_chip controllers to provide setup() and teardown() callbacks to
 board specific code; those board specific callbacks would register devices
-once all the necessary resources are available.
+once all the necessary resources are available, and remove them later when
+the GPIO controller device becomes unavailable.
+
+
+Sysfs Interface for Userspace (OPTIONAL)
+========================================
+Platforms which use the "gpiolib" implementors framework may choose to
+configure a sysfs user interface to GPIOs.  This is different from the
+debugfs interface, since it provides control over GPIO direction and
+value instead of just showing a gpio state summary.  Plus, it could be
+present on production systems without debugging support.
+
+Given approprate hardware documentation for the system, userspace could
+know for example that GPIO #23 controls the write protect line used to
+protect boot loader segments in flash memory.  System upgrade procedures
+may need to temporarily remove that protection, first importing a GPIO,
+then changing its output state, then updating the code before re-enabling
+the write protection.  In normal use, GPIO #23 would never be touched,
+and the kernel would have no need to know about it.
+
+Again depending on appropriate hardware documentation, on some systems
+userspace GPIO can be used to determine system configuration data that
+standard kernels won't know about.  And for some tasks, simple userspace
+GPIO drivers could be all that the system really needs.
+
+Note that standard kernel drivers exist for common "LEDs and Buttons"
+GPIO tasks:  "leds-gpio" and "gpio_keys", respectively.  Use those
+instead of talking directly to the GPIOs; they integrate with kernel
+frameworks better than your userspace code could.
+
+
+Paths in Sysfs
+--------------
+There are three kinds of entry in /sys/class/gpio:
+
+   -	Control interfaces used to get userspace control over GPIOs;
+
+   -	GPIOs themselves; and
+
+   -	GPIO controllers ("gpio_chip" instances).
+
+That's in addition to standard files including the "device" symlink.
+
+The control interfaces are write-only:
+
+    /sys/class/gpio/
+
+    	"export" ... Userspace may ask the kernel to export control of
+		a GPIO to userspace by writing its number to this file.
+
+		Example:  "echo 19 > export" will create a "gpio19" node
+		for GPIO #19, if that's not requested by kernel code.
+
+    	"unexport" ... Reverses the effect of exporting to userspace.
+
+		Example:  "echo 19 > unexport" will remove a "gpio19"
+		node exported using the "export" file.
+
+GPIO signals have paths like /sys/class/gpio/gpio42/ (for GPIO #42)
+and have the following read/write attributes:
+
+    /sys/class/gpio/gpioN/
+
+	"direction" ... reads as either "in" or "out".  This value may
+		normally be written.  Writing as "out" defaults to
+		initializing the value as low.  To ensure glitch free
+		operation, values "low" and "high" may be written to
+		configure the GPIO as an output with that initial value.
+
+		Note that this attribute *will not exist* if the kernel
+		doesn't support changing the direction of a GPIO, or
+		it was exported by kernel code that didn't explicitly
+		allow userspace to reconfigure this GPIO's direction.
+
+	"value" ... reads as either 0 (low) or 1 (high).  If the GPIO
+		is configured as an output, this value may be written;
+		any nonzero value is treated as high.
+
+GPIO controllers have paths like /sys/class/gpio/chipchip42/ (for the
+controller implementing GPIOs starting at #42) and have the following
+read-only attributes:
+
+    /sys/class/gpio/gpiochipN/
+
+    	"base" ... same as N, the first GPIO managed by this chip
+
+    	"label" ... provided for diagnostics (not always unique)
+
+    	"ngpio" ... how many GPIOs this manges (N to N + ngpio - 1)
+
+Board documentation should in most cases cover what GPIOs are used for
+what purposes.  However, those numbers are not always stable; GPIOs on
+a daughtercard might be different depending on the base board being used,
+or other cards in the stack.  In such cases, you may need to use the
+gpiochip nodes (possibly in conjunction with schematics) to determine
+the correct GPIO number to use for a given signal.
+
+
+Exporting from Kernel code
+--------------------------
+Kernel code can explicitly manage exports of GPIOs which have already been
+requested using gpio_request():
+
+	/* export the GPIO to userspace */
+	int gpio_export(unsigned gpio, bool direction_may_change);
+
+	/* reverse gpio_export() */
+	void gpio_unexport();
+
+After a kernel driver requests a GPIO, it may only be made available in
+the sysfs interface by gpio_export().  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
+suitable for documenting as part of a board support package.
--- g26.orig/arch/arm/plat-omap/gpio.c	2008-05-17 17:12:11.000000000 -0700
+++ g26/arch/arm/plat-omap/gpio.c	2008-05-17 17:15:04.000000000 -0700
@@ -1488,6 +1488,9 @@ static int __init _omap_gpio_init(void)
 		bank->chip.set = gpio_set;
 		if (bank_is_mpuio(bank)) {
 			bank->chip.label = "mpuio";
+#ifdef CONFIG_ARCH_OMAP1
+			bank->chip.dev = &omap_mpuio_device.dev;
+#endif
 			bank->chip.base = OMAP_MPUIO(0);
 		} else {
 			bank->chip.label = "gpio";
--- g26.orig/arch/avr32/mach-at32ap/pio.c	2008-05-17 17:12:11.000000000 -0700
+++ g26/arch/avr32/mach-at32ap/pio.c	2008-05-17 17:15:04.000000000 -0700
@@ -361,6 +361,8 @@ static int __init pio_probe(struct platf
 	pio->chip.label = pio->name;
 	pio->chip.base = pdev->id * 32;
 	pio->chip.ngpio = 32;
+	pio->chip.dev = &pdev->dev;
+	pio->chip.owner = THIS_MODULE;
 
 	pio->chip.direction_input = direction_input;
 	pio->chip.get = gpio_get;
--- g26.orig/drivers/gpio/Kconfig	2008-05-17 17:12:11.000000000 -0700
+++ g26/drivers/gpio/Kconfig	2008-05-17 17:15:04.000000000 -0700
@@ -23,6 +23,21 @@ config DEBUG_GPIO
 	  slower.  The diagnostics help catch the type of setup errors
 	  that are most common when setting up new platforms or boards.
 
+config GPIO_SYSFS
+	bool "/sys/class/gpio/... (sysfs interface)"
+	depends on SYSFS && EXPERIMENTAL
+	help
+	  Say Y here to add a sysfs interface for GPIOs.
+
+	  This is mostly useful to work around omissions in a system's
+	  kernel support.  Those are common in custom and semicustom
+	  hardware assembled using standard kernels with a minimum of
+	  custom patches.  In those cases, userspace code may import
+	  a given GPIO from the kernel, if no kernel driver requested it.
+
+	  Kernel drivers may also request that a particular GPIO be
+	  exported to userspace; this can be useful when debugging.
+
 # put expanders in the right section, in alphabetical order
 
 comment "I2C GPIO expanders:"
--- g26.orig/drivers/gpio/gpiolib.c	2008-05-17 17:12:11.000000000 -0700
+++ g26/drivers/gpio/gpiolib.c	2008-05-17 17:15:04.000000000 -0700
@@ -2,8 +2,11 @@
 #include <linux/module.h>
 #include <linux/irq.h>
 #include <linux/spinlock.h>
-
-#include <asm/gpio.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/debugfs.h>
+#include <linux/seq_file.h>
+#include <linux/gpio.h>
 
 
 /* Optional implementation infrastructure for GPIO interfaces.
@@ -44,6 +47,8 @@ struct gpio_desc {
 #define FLAG_REQUESTED	0
 #define FLAG_IS_OUT	1
 #define FLAG_RESERVED	2
+#define FLAG_EXPORT	3	/* protected by sysfs_lock */
+#define FLAG_SYSFS	4	/* exported via /sys/class/gpio/control */
 
 #ifdef CONFIG_DEBUG_FS
 	const char		*label;
@@ -151,6 +156,484 @@ err:
 	return ret;
 }
 
+#ifdef CONFIG_GPIO_SYSFS
+
+/* lock protects against unexport_gpio() being called while
+ * sysfs files are active.
+ */
+static DEFINE_MUTEX(sysfs_lock);
+
+/*
+ * /sys/class/gpio/gpioN... only for GPIOs that are exported
+ *   /direction
+ *      * MAY BE OMITTED if kernel won't allow direction changes
+ *      * is read/write as "in" or "out"
+ *      * may also be written as "high" or "low", initializing
+ *        output value as specified ("out" implies "low")
+ *   /value
+ *      * always readable, subject to hardware behavior
+ *      * may be writable, as zero/nonzero
+ *
+ * REVISIT there will likely be an attribute for configuring async
+ * notifications, e.g. to specify polling interval or IRQ trigger type
+ * that would for example trigger a poll() on the "value".
+ */
+
+static ssize_t gpio_direction_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	const struct gpio_desc	*desc = dev_get_drvdata(dev);
+	ssize_t			status;
+
+	mutex_lock(&sysfs_lock);
+
+	if (!test_bit(FLAG_EXPORT, &desc->flags))
+		status = -EIO;
+	else
+		status = sprintf(buf, "%s\n",
+			test_bit(FLAG_IS_OUT, &desc->flags)
+				? "out" : "in");
+
+	mutex_unlock(&sysfs_lock);
+	return status;
+}
+
+static ssize_t gpio_direction_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t size)
+{
+	const struct gpio_desc	*desc = dev_get_drvdata(dev);
+	unsigned		gpio = desc - gpio_desc;
+	ssize_t			status;
+
+	mutex_lock(&sysfs_lock);
+
+	if (!test_bit(FLAG_EXPORT, &desc->flags))
+		status = -EIO;
+	else if (sysfs_streq(buf, "high"))
+		status = gpio_direction_output(gpio, 1);
+	else if (sysfs_streq(buf, "out") || sysfs_streq(buf, "low"))
+		status = gpio_direction_output(gpio, 0);
+	else if (sysfs_streq(buf, "in"))
+		status = gpio_direction_input(gpio);
+	else
+		status = -EINVAL;
+
+	mutex_unlock(&sysfs_lock);
+	return status ? : size;
+}
+
+static const DEVICE_ATTR(direction, 0644,
+		gpio_direction_show, gpio_direction_store);
+
+static ssize_t gpio_value_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	const struct gpio_desc	*desc = dev_get_drvdata(dev);
+	unsigned		gpio = desc - gpio_desc;
+	ssize_t			status;
+
+	mutex_lock(&sysfs_lock);
+
+	if (!test_bit(FLAG_EXPORT, &desc->flags))
+		status = -EIO;
+	else
+		status = sprintf(buf, "%d\n", gpio_get_value_cansleep(gpio));
+
+	mutex_unlock(&sysfs_lock);
+	return status;
+}
+
+static ssize_t gpio_value_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t size)
+{
+	const struct gpio_desc	*desc = dev_get_drvdata(dev);
+	unsigned		gpio = desc - gpio_desc;
+	ssize_t			status;
+
+	mutex_lock(&sysfs_lock);
+
+	if (!test_bit(FLAG_EXPORT, &desc->flags))
+		status = -EIO;
+	else if (!test_bit(FLAG_IS_OUT, &desc->flags))
+		status = -EPERM;
+	else {
+		long		value;
+
+		status = strict_strtol(buf, 0, &value);
+		if (status == 0) {
+			gpio_set_value_cansleep(gpio, value != 0);
+			status = size;
+		}
+	}
+
+	mutex_unlock(&sysfs_lock);
+	return status;
+}
+
+static /*const*/ DEVICE_ATTR(value, 0644,
+		gpio_value_show, gpio_value_store);
+
+static const struct attribute *gpio_attrs[] = {
+	&dev_attr_direction.attr,
+	&dev_attr_value.attr,
+	NULL,
+};
+
+static const struct attribute_group gpio_attr_group = {
+	.attrs = (struct attribute **) gpio_attrs,
+};
+
+/*
+ * /sys/class/gpio/gpiochipN/
+ *   /base ... matching gpio_chip.base (N)
+ *   /label ... matching gpio_chip.label
+ *   /ngpio ... matching gpio_chip.ngpio
+ */
+
+static ssize_t chip_base_show(struct device *dev,
+			       struct device_attribute *attr, char *buf)
+{
+	const struct gpio_chip	*chip = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%d\n", chip->base);
+}
+static DEVICE_ATTR(base, 0444, chip_base_show, NULL);
+
+static ssize_t chip_label_show(struct device *dev,
+			       struct device_attribute *attr, char *buf)
+{
+	const struct gpio_chip	*chip = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%s\n", chip->label ? : "");
+}
+static DEVICE_ATTR(label, 0444, chip_label_show, NULL);
+
+static ssize_t chip_ngpio_show(struct device *dev,
+			       struct device_attribute *attr, char *buf)
+{
+	const struct gpio_chip	*chip = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%u\n", chip->ngpio);
+}
+static DEVICE_ATTR(ngpio, 0444, chip_ngpio_show, NULL);
+
+static const struct attribute *gpiochip_attrs[] = {
+	&dev_attr_base.attr,
+	&dev_attr_label.attr,
+	&dev_attr_ngpio.attr,
+	NULL,
+};
+
+static const struct attribute_group gpiochip_attr_group = {
+	.attrs = (struct attribute **) gpiochip_attrs,
+};
+
+/*
+ * /sys/class/gpio/export ... write-only
+ *	integer N ... number of GPIO to export (full access)
+ * /sys/class/gpio/unexport ... write-only
+ *	integer N ... number of GPIO to unexport
+ */
+static ssize_t export_store(struct class *class, const char *buf, size_t len)
+{
+	long	gpio;
+	int	status;
+
+	status = strict_strtol(buf, 0, &gpio);
+	if (status < 0)
+		goto done;
+
+	/* No extra locking here; FLAG_SYSFS just signifies that the
+	 * request and export were done by on behalf of userspace, so
+	 * they may be undone on its behalf too.
+	 */
+
+	status = gpio_request(gpio, "sysfs");
+	if (status < 0)
+		goto done;
+
+	status = gpio_export(gpio, true);
+	if (status < 0)
+		gpio_free(gpio);
+	else
+		set_bit(FLAG_SYSFS, &gpio_desc[gpio].flags);
+
+done:
+	if (status)
+		pr_debug("%s: status %d\n", __func__, status);
+	return status ? : len;
+}
+
+static ssize_t unexport_store(struct class *class, const char *buf, size_t len)
+{
+	long	gpio;
+	int	status;
+
+	status = strict_strtol(buf, 0, &gpio);
+	if (status < 0)
+		goto done;
+
+	status = -EINVAL;
+
+	/* reject bogus commands (gpio_unexport ignores them) */
+	if (!gpio_is_valid(gpio))
+		goto done;
+
+	/* No extra locking here; FLAG_SYSFS just signifies that the
+	 * request and export were done by on behalf of userspace, so
+	 * they may be undone on its behalf too.
+	 */
+	if (test_and_clear_bit(FLAG_SYSFS, &gpio_desc[gpio].flags)) {
+		status = 0;
+		gpio_free(gpio);
+	}
+done:
+	if (status)
+		pr_debug("%s: status %d\n", __func__, status);
+	return status ? : len;
+}
+
+static struct class_attribute gpio_class_attrs[] = {
+	__ATTR(export, 0200, NULL, export_store),
+	__ATTR(unexport, 0200, NULL, unexport_store),
+	{},
+};
+
+static struct class gpio_class = {
+	.name =		"gpio",
+	.owner =	THIS_MODULE,
+
+	.class_attrs =	gpio_class_attrs,
+};
+
+
+/**
+ * gpio_export - export a GPIO through sysfs
+ * @gpio: gpio to make available, already requested
+ * @direction_may_change: true if userspace may change gpio direction
+ * Context: arch_initcall or later
+ *
+ * When drivers want to make a GPIO accessible to userspace after they
+ * have requested it -- perhaps while debugging, or as part of their
+ * public interface -- they may use this routine.  If the GPIO can
+ * change direction (some can't) and the caller allows it, userspace
+ * will see "direction" sysfs attribute which may be used to change
+ * the gpio's direction.  A "value" attribute will always be provided.
+ *
+ * Returns zero on success, else an error.
+ */
+int gpio_export(unsigned gpio, bool direction_may_change)
+{
+	unsigned long		flags;
+	struct gpio_desc	*desc;
+	int			status = -EINVAL;
+
+	/* can't export until sysfs is available ... */
+	if (!gpio_class.devices.next) {
+		pr_debug("%s: called too early!\n", __func__);
+		return -ENOENT;
+	}
+
+	if (!gpio_is_valid(gpio))
+		goto done;
+
+	mutex_lock(&sysfs_lock);
+
+	spin_lock_irqsave(&gpio_lock, flags);
+	desc = &gpio_desc[gpio];
+	if (test_bit(FLAG_REQUESTED, &desc->flags)
+			&& !test_bit(FLAG_EXPORT, &desc->flags)) {
+		status = 0;
+		if (!desc->chip->direction_input
+				|| !desc->chip->direction_output)
+			direction_may_change = false;
+	}
+	spin_unlock_irqrestore(&gpio_lock, flags);
+
+	if (status == 0) {
+		struct device	*dev;
+
+		dev = device_create(&gpio_class, desc->chip->dev, 0,
+				"gpio%d", gpio);
+		if (dev) {
+			dev_set_drvdata(dev, desc);
+			if (direction_may_change)
+				status = sysfs_create_group(&dev->kobj,
+						&gpio_attr_group);
+			else
+				status = device_create_file(dev,
+						&dev_attr_value);
+			if (status != 0)
+				device_unregister(dev);
+		} else
+			status = -ENODEV;
+		if (status == 0)
+			set_bit(FLAG_EXPORT, &desc->flags);
+	}
+
+	mutex_unlock(&sysfs_lock);
+
+done:
+	if (status)
+		pr_debug("%s: gpio%d status %d\n", __func__, gpio, status);
+
+	return status;
+}
+EXPORT_SYMBOL_GPL(gpio_export);
+
+static int match_export(struct device *dev, void *data)
+{
+	return dev_get_drvdata(dev) == data;
+}
+
+/**
+ * gpio_unexport - reverse effect of gpio_export()
+ * @gpio: gpio to make unavailable
+ *
+ * This is implicit on gpio_free().
+ */
+void gpio_unexport(unsigned gpio)
+{
+	struct gpio_desc	*desc;
+	int			status = -EINVAL;
+
+	if (!gpio_is_valid(gpio))
+		goto done;
+
+	mutex_lock(&sysfs_lock);
+
+	desc = &gpio_desc[gpio];
+	if (test_bit(FLAG_EXPORT, &desc->flags)) {
+		struct device	*dev = NULL;
+
+		dev = class_find_device(&gpio_class, desc, match_export);
+		if (dev) {
+			clear_bit(FLAG_EXPORT, &desc->flags);
+			put_device(dev);
+			device_unregister(dev);
+			status = 0;
+		} else
+			status = -ENODEV;
+	}
+
+	mutex_unlock(&sysfs_lock);
+done:
+	if (status)
+		pr_debug("%s: gpio%d status %d\n", __func__, gpio, status);
+}
+EXPORT_SYMBOL_GPL(gpio_unexport);
+
+static int gpiochip_export(struct gpio_chip *chip)
+{
+	int		status;
+	struct device	*dev;
+
+	/* Many systems register gpio chips for SOC support very early,
+	 * before driver model support is available.  In those cases we
+	 * export this later, in gpiolib_sysfs_init() ... here we just
+	 * verify that _some_ field of gpio_class got initialized.
+	 */
+	if (!gpio_class.devices.next)
+		return 0;
+
+	/* use chip->base for the ID; it's already known to be unique */
+	mutex_lock(&sysfs_lock);
+	dev = device_create(&gpio_class, chip->dev, 0,
+			"gpiochip%d", chip->base);
+	if (dev) {
+		dev_set_drvdata(dev, chip);
+		status = sysfs_create_group(&dev->kobj,
+				&gpiochip_attr_group);
+	} else
+		status = -ENODEV;
+	chip->exported = (status == 0);
+	mutex_unlock(&sysfs_lock);
+
+	if (status) {
+		unsigned long	flags;
+		unsigned	gpio;
+
+		spin_lock_irqsave(&gpio_lock, flags);
+		gpio = chip->base;
+		while (gpio_desc[gpio].chip == chip)
+			gpio_desc[gpio++].chip = NULL;
+		spin_unlock_irqrestore(&gpio_lock, flags);
+
+		pr_debug("%s: chip %s status %d\n", __func__,
+				chip->label, status);
+	}
+
+	return status;
+}
+
+static void gpiochip_unexport(struct gpio_chip *chip)
+{
+	int			status;
+	struct device		*dev;
+
+	mutex_lock(&sysfs_lock);
+	dev = class_find_device(&gpio_class, chip, match_export);
+	if (dev) {
+		put_device(dev);
+		device_unregister(dev);
+		chip->exported = 0;
+		status = 0;
+	} else
+		status = -ENODEV;
+	mutex_unlock(&sysfs_lock);
+
+	if (status)
+		pr_debug("%s: chip %s status %d\n", __func__,
+				chip->label, status);
+}
+
+static int __init gpiolib_sysfs_init(void)
+{
+	int		status;
+	unsigned long	flags;
+	unsigned	gpio;
+
+	status = class_register(&gpio_class);
+	if (status < 0)
+		return status;
+
+	/* Scan and register the gpio_chips which registered very
+	 * early (e.g. before the class_register above was called).
+	 *
+	 * We run before arch_initcall() so chip->dev nodes can have
+	 * registered, and so arch_initcall() can always gpio_export().
+	 */
+	spin_lock_irqsave(&gpio_lock, flags);
+	for (gpio = 0; gpio < ARCH_NR_GPIOS; gpio++) {
+		struct gpio_chip	*chip;
+
+		chip = gpio_desc[gpio].chip;
+		if (!chip || chip->exported)
+			continue;
+
+		spin_unlock_irqrestore(&gpio_lock, flags);
+		status = gpiochip_export(chip);
+		spin_lock_irqsave(&gpio_lock, flags);
+	}
+	spin_unlock_irqrestore(&gpio_lock, flags);
+
+
+	return status;
+}
+postcore_initcall(gpiolib_sysfs_init);
+
+#else
+static inline int gpiochip_export(struct gpio_chip *chip)
+{
+	return 0;
+}
+
+static inline void gpiochip_unexport(struct gpio_chip *chip)
+{
+}
+
+#endif /* CONFIG_GPIO_SYSFS */
+
 /**
  * gpiochip_add() - register a gpio_chip
  * @chip: the chip to register, with chip->base initialized
@@ -160,6 +643,11 @@ err:
  * because the chip->base is invalid or already associated with a
  * different chip.  Otherwise it returns zero as a success code.
  *
+ * When gpiochip_add() is called very early during boot, so that GPIOs
+ * can be freely used, the chip->dev device must be registered before
+ * the gpio framework's arch_initcall().  Otherwise sysfs initialization
+ * for GPIOs will fail rudely.
+ *
  * If chip->base is negative, this requests dynamic assignment of
  * a range of valid GPIOs.
  */
@@ -182,7 +670,7 @@ int gpiochip_add(struct gpio_chip *chip)
 		base = gpiochip_find_base(chip->ngpio);
 		if (base < 0) {
 			status = base;
-			goto fail_unlock;
+			goto unlock;
 		}
 		chip->base = base;
 	}
@@ -197,12 +685,16 @@ int gpiochip_add(struct gpio_chip *chip)
 	if (status == 0) {
 		for (id = base; id < base + chip->ngpio; id++) {
 			gpio_desc[id].chip = chip;
-			gpio_desc[id].flags = 0;
+			gpio_desc[id].flags = chip->direction_input
+				? (1 << FLAG_IS_OUT)
+				: 0;
 		}
 	}
 
-fail_unlock:
+unlock:
 	spin_unlock_irqrestore(&gpio_lock, flags);
+	if (status == 0)
+		status = gpiochip_export(chip);
 fail:
 	/* failures here can mean systems won't boot... */
 	if (status)
@@ -234,6 +726,7 @@ int gpiochip_remove(struct gpio_chip *ch
 		}
 	}
 	if (status == 0) {
+		gpiochip_unexport(chip);
 		for (id = chip->base; id < chip->base + chip->ngpio; id++)
 			gpio_desc[id].chip = NULL;
 	}
@@ -296,6 +789,8 @@ void gpio_free(unsigned gpio)
 		return;
 	}
 
+	gpio_unexport(gpio);
+
 	spin_lock_irqsave(&gpio_lock, flags);
 
 	desc = &gpio_desc[gpio];
@@ -534,10 +1029,6 @@ EXPORT_SYMBOL_GPL(gpio_set_value_canslee
 
 #ifdef CONFIG_DEBUG_FS
 
-#include <linux/debugfs.h>
-#include <linux/seq_file.h>
-
-
 static void gpiolib_dbg_show(struct seq_file *s, struct gpio_chip *chip)
 {
 	unsigned		i;
--- g26.orig/drivers/gpio/mcp23s08.c	2008-05-17 17:12:11.000000000 -0700
+++ g26/drivers/gpio/mcp23s08.c	2008-05-17 17:15:04.000000000 -0700
@@ -239,6 +239,7 @@ static int mcp23s08_probe(struct spi_dev
 	mcp->chip.base = pdata->base;
 	mcp->chip.ngpio = 8;
 	mcp->chip.can_sleep = 1;
+	mcp->chip.dev = &spi->dev;
 	mcp->chip.owner = THIS_MODULE;
 
 	spi_set_drvdata(spi, mcp);
--- g26.orig/drivers/gpio/pca953x.c	2008-05-17 17:12:11.000000000 -0700
+++ g26/drivers/gpio/pca953x.c	2008-05-17 17:15:04.000000000 -0700
@@ -187,6 +187,7 @@ static void pca953x_setup_gpio(struct pc
 	gc->base = chip->gpio_start;
 	gc->ngpio = gpios;
 	gc->label = chip->client->name;
+	gc->dev = &chip->client->dev;
 	gc->owner = THIS_MODULE;
 }
 
--- g26.orig/drivers/gpio/pcf857x.c	2008-05-17 17:12:11.000000000 -0700
+++ g26/drivers/gpio/pcf857x.c	2008-05-17 17:15:04.000000000 -0700
@@ -175,6 +175,7 @@ static int pcf857x_probe(struct i2c_clie
 
 	gpio->chip.base = pdata->gpio_base;
 	gpio->chip.can_sleep = 1;
+	gpio->chip.dev = &client->dev;
 	gpio->chip.owner = THIS_MODULE;
 
 	/* NOTE:  the OnSemi jlc1562b is also largely compatible with
--- g26.orig/drivers/i2c/chips/tps65010.c	2008-05-17 17:12:11.000000000 -0700
+++ g26/drivers/i2c/chips/tps65010.c	2008-05-17 17:15:04.000000000 -0700
@@ -636,6 +636,8 @@ static int tps65010_probe(struct i2c_cli
 		tps->outmask = board->outmask;
 
 		tps->chip.label = client->name;
+		tps->chip.dev = &client->dev;
+		tps->chip.owner = THIS_MODULE;
 
 		tps->chip.set = tps65010_gpio_set;
 		tps->chip.direction_output = tps65010_output;
--- g26.orig/drivers/mfd/htc-egpio.c	2008-05-17 17:12:11.000000000 -0700
+++ g26/drivers/mfd/htc-egpio.c	2008-05-17 17:15:04.000000000 -0700
@@ -318,6 +318,8 @@ static int __init egpio_probe(struct pla
 		ei->chip[i].dev = &(pdev->dev);
 		chip = &(ei->chip[i].chip);
 		chip->label           = "htc-egpio";
+		chip->dev             = &pdev->dev;
+		chip->owner           = THIS_MODULE;
 		chip->get             = egpio_get;
 		chip->set             = egpio_set;
 		chip->direction_input = egpio_direction_input;
--- g26.orig/include/asm-generic/gpio.h	2008-05-17 17:12:11.000000000 -0700
+++ g26/include/asm-generic/gpio.h	2008-05-17 17:15:04.000000000 -0700
@@ -28,6 +28,8 @@ struct module;
 /**
  * struct gpio_chip - abstract a GPIO controller
  * @label: for diagnostics
+ * @dev: optional device providing the GPIOs
+ * @owner: helps prevent removal of modules exporting active GPIOs
  * @direction_input: configures signal "offset" as input, or returns error
  * @get: returns value for signal "offset"; for output signals this
  *	returns either the value actually sensed, or zero
@@ -55,6 +57,7 @@ struct module;
  */
 struct gpio_chip {
 	char			*label;
+	struct device		*dev;
 	struct module		*owner;
 
 	int			(*direction_input)(struct gpio_chip *chip,
@@ -70,6 +73,7 @@ struct gpio_chip {
 	int			base;
 	u16			ngpio;
 	unsigned		can_sleep:1;
+	unsigned		exported:1;
 };
 
 extern const char *gpiochip_is_requested(struct gpio_chip *chip,
@@ -104,7 +108,18 @@ extern void __gpio_set_value(unsigned gp
 extern int __gpio_cansleep(unsigned gpio);
 
 
-#else
+#ifdef CONFIG_GPIO_SYSFS
+
+/*
+ * 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 void gpio_unexport(unsigned gpio);
+
+#endif	/* CONFIG_GPIO_SYSFS */
+
+#else	/* !CONFIG_HAVE_GPIO_LIB */
 
 static inline int gpio_is_valid(int number)
 {
@@ -133,6 +148,20 @@ static inline void gpio_set_value_cansle
 	gpio_set_value(gpio, value);
 }
 
-#endif
+#endif /* !CONFIG_HAVE_GPIO_LIB */
+
+#ifndef CONFIG_GPIO_SYSFS
+
+/* sysfs support is only available with gpiolib, where it's optional */
+
+static inline int gpio_export(unsigned gpio, bool direction_may_change)
+{
+	return -ENOSYS;
+}
+
+static inline void gpio_unexport(unsigned gpio)
+{
+}
+#endif	/* CONFIG_GPIO_SYSFS */
 
 #endif /* _ASM_GENERIC_GPIO_H */
--- g26.orig/include/linux/gpio.h	2008-05-17 17:12:11.000000000 -0700
+++ g26/include/linux/gpio.h	2008-05-17 17:15:04.000000000 -0700
@@ -76,6 +76,19 @@ static inline void gpio_set_value_cansle
 	WARN_ON(1);
 }
 
+static inline int gpio_export(unsigned gpio, bool direction_may_change)
+{
+	/* GPIO can never have been requested or set as {in,out}put */
+	WARN_ON(1);
+	return -EINVAL;
+}
+
+static inline void gpio_unexport(unsigned gpio)
+{
+	/* GPIO can never have been exported */
+	WARN_ON(1);
+}
+
 static inline int gpio_to_irq(unsigned gpio)
 {
 	/* GPIO can never have been requested or set as input */

Re: [patch/rfc 2.6.25-git] gpio: sysfs interface

[Ben Nizette]

On Sat, 2008-05-17 at 15:14 -0700, David Brownell wrote:
> On Friday 02 May 2008, Pavel Machek wrote:
> > What about mkdir gpio-N to export it?
> 
> Ugh.  That would create way more complication than I want
> to see here.  I thought about that early on, and decided
> that was Not The Way To Go.

I have a userspace gpio interface which uses /dev and configfs.  The
configfs side was really nice (and used mkdir, as configfs does) but I
wasn't a fan of needing udev..  On balance, I like the all-in-one sysfs
way more.

> 
> - Dave
> 

Re: [patch/rfc 2.6.25-git] gpio: sysfs interface

[Pavel Machek]

On Sat 2008-05-17 15:14:06, David Brownell wrote:
> On Friday 02 May 2008, Pavel Machek wrote:
> > 
> > > >   echo "export 23" > /sys/class/gpio/control
> > > >     ... will gpio_request(23, "sysfs") and gpio_export(23); use
> > > >     /sys/class/gpio/gpio-23/direction to configure it.
> > > >   echo "unexport 23" > /sys/class/gpio/control
> > > >     ... will gpio_free(23)
> > > 
> > > hm, does ths sysfs one-value-per-file rule apply to writes?
> > 
> > Having seen ibm acpi interface... yes, that rule should be aplied for
> > writes, too.
> 
> The latest version of this patch, which I'll post soonish,
> follows the suggestion to have one file for the "export"
> operation and another for "unexport".
> 
> 
> > What about mkdir gpio-N to export it?
> 
> Ugh.  That would create way more complication than I want
> to see here.  I thought about that early on, and decided
> that was Not The Way To Go.

...but it would be consistent with configfs... and face it... doing
echo > file to make a directory is seriously ugly.
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [patch/rfc 2.6.25-git] gpio: sysfs interface

[David Brownell]

On Monday 19 May 2008, Pavel Machek wrote:
> 
> > 
> > > What about mkdir gpio-N to export it?
> > 
> > Ugh.  That would create way more complication than I want
> > to see here.  I thought about that early on, and decided
> > that was Not The Way To Go.
> 
> ...but it would be consistent with configfs... and face it... doing
> echo > file to make a directory is seriously ugly.

This isn't configfs, and I didn't happen to notice any polite
way for sysfs users to intercept "mkdir", parse the relevant
directory name, reject illegal names, and pre-populate the
just-created directory with relevant contents.


Were you implying it should go into configfs?  If so, do you
maybe have an update to the patch I sent a day or so ago?

Re: [patch 2.6.26-rc2-git] gpio: sysfs interface

[Andrew Morton]

On Sat, 17 May 2008 17:36:55 -0700 David Brownell <david-b@pacbell.net> wrote:

> +static const struct attribute *gpiochip_attrs[] = {
> +	&dev_attr_base.attr,
> +	&dev_attr_label.attr,
> +	&dev_attr_ngpio.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group gpiochip_attr_group = {
> +	.attrs = (struct attribute **) gpiochip_attrs,
> +};

ick.  So we got caught partway through constification and we need to
patch it up with a cast.

Re: [patch/rfc 2.6.25-git] gpio: sysfs interface

[Pavel Machek]

Hi!

> > > > What about mkdir gpio-N to export it?
> > > 
> > > Ugh.  That would create way more complication than I want
> > > to see here.  I thought about that early on, and decided
> > > that was Not The Way To Go.
> > 
> > ...but it would be consistent with configfs... and face it... doing
> > echo > file to make a directory is seriously ugly.
> 
> This isn't configfs, and I didn't happen to notice any polite
> way for sysfs users to intercept "mkdir", parse the relevant
> directory name, reject illegal names, and pre-populate the
> just-created directory with relevant contents.
> 
> 
> Were you implying it should go into configfs?  If so, do you
> maybe have an update to the patch I sent a day or so ago?

No, sorry.

I'm not sure if it should go to configfs, or if sysfs should be
improved. But "something" should be done, because we will be stuck
with this interface for a long while.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html