[PATCH 0/6] GPIO character device skeleton

[PATCH 0/6] GPIO character device skeleton

[Linus Walleij]

OK so this is it, I had no patience waiting for users to come up
with this new ABI, and the requests for a way for userspace to
use GPIOs properly is coming up again and again. So I created the
basics for it, so we can then build on top of this to get things
right. I want to get these very first things right before we go
wild with setting/getting pin values etc.

We add ONE ioctl() to get information on the gpiochip. Now we can
do this (example from ux500):

root@Ux500:/ lsgpio
GPIO chip: a03fe000.gpio, 32 GPIO lines
GPIO chip: 8011e080.gpio, 32 GPIO lines
GPIO chip: 8011e000.gpio, 32 GPIO lines
GPIO chip: 8000e180.gpio, 32 GPIO lines
GPIO chip: 8000e100.gpio, 32 GPIO lines
GPIO chip: 8000e080.gpio, 32 GPIO lines
GPIO chip: 8000e000.gpio, 32 GPIO lines
GPIO chip: 8012e080.gpio, 32 GPIO lines
GPIO chip: 8012e000.gpio, 32 GPIO lines
GPIO chip: abx500-gpio, 42 GPIO lines
GPIO chip: tc3589x, 20 GPIO lines

Johan: I don't have a hot-pluggable GPIO controller :( can you
do me the favour of testing this and fixing my stupid refcounts
and race conditions? I only used my brain to try to get things
right with pluggable GPIO controllers in this patch set, and it
is bound to fail.

How to identify and manipulate individual GPIO lines from this
ABI is a *LATER* *QUESTION*, this is the bare essentials for
getting there: basic operations on the gpiochip device level.

Linus Walleij (6):
  gpio: make the gpiochip a real device
  gpio: refer to gpio device in prints and debugfs
  gpio: add a userspace chardev ABI for GPIOs
  tools/gpio: create GPIO tools
  gpio: add a userspace character device ABI
  gpio: ABI: mark the sysfs ABI as obsolete

 Documentation/ABI/obsolete/sysfs-gpio |  30 ++++++
 Documentation/ABI/testing/gpio-cdev   |  26 +++++
 Documentation/ABI/testing/sysfs-gpio  |  28 -----
 MAINTAINERS                           |   4 +
 drivers/gpio/gpiolib-sysfs.c          |   2 +-
 drivers/gpio/gpiolib.c                | 193 +++++++++++++++++++++++++++++++---
 drivers/gpio/gpiolib.h                |  12 +--
 include/linux/gpio/driver.h           |  11 +-
 include/uapi/linux/Kbuild             |   1 +
 include/uapi/linux/gpio.h             |  28 +++++
 tools/Makefile                        |   7 +-
 tools/gpio/Makefile                   |  12 +++
 tools/gpio/gpio-utils.c               |  11 ++
 tools/gpio/gpio-utils.h               |  25 +++++
 tools/gpio/lsgpio.c                   | 128 ++++++++++++++++++++++
 15 files changed, 462 insertions(+), 56 deletions(-)
 create mode 100644 Documentation/ABI/obsolete/sysfs-gpio
 create mode 100644 Documentation/ABI/testing/gpio-cdev
 delete mode 100644 Documentation/ABI/testing/sysfs-gpio
 create mode 100644 include/uapi/linux/gpio.h
 create mode 100644 tools/gpio/Makefile
 create mode 100644 tools/gpio/gpio-utils.c
 create mode 100644 tools/gpio/gpio-utils.h
 create mode 100644 tools/gpio/lsgpio.c

-- 
2.4.3

[PATCH 1/6] gpio: make the gpiochip a real device

[Linus Walleij]

GPIO chips have been around for years, but were never real devices,
instead they were piggy-backing on a parent device (such as a
platform_device or amba_device) but this was always optional.
GPIO chips could also exist without any device at all, with its
struct device *dev pointer being set to null.

When sysfs was in use, a mock device would be created, with the
optional parent assigned, or just floating orphaned with NULL
as parent.

For a proper userspace ABI we need gpiochips to *always have a
populated struct device, so add this in the gpio_chip struct.
The name "dev" is unfortunately already take so we use "device"
to name it.

If sysfs is active, it will use this device as parent, and the
former parent device "dev" will be set as parent of the new
"device" struct member.

>From this point on, gpiochips are devices.

Cc: Johan Hovold <johan@kernel.org>
Cc: Michael Welling <mwelling@ieee.org>
Cc: Markus Pargmann <mpa@pengutronix.de>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/gpiolib-sysfs.c |  2 +-
 drivers/gpio/gpiolib.c       | 44 +++++++++++++++++++++++++++++++++++++++-----
 include/linux/gpio/driver.h  |  9 +++++++--
 3 files changed, 47 insertions(+), 8 deletions(-)

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index b57ed8e55ab5..7e5bc5736e47 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -605,7 +605,7 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
 	if (chip->names && chip->names[offset])
 		ioname = chip->names[offset];
 
-	dev = device_create_with_groups(&gpio_class, chip->dev,
+	dev = device_create_with_groups(&gpio_class, &chip->device,
 					MKDEV(0, 0), data, gpio_groups,
 					ioname ? ioname : "gpio%u",
 					desc_to_gpio(desc));
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 6798355c61c6..0ab4f75b7f8e 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -16,6 +16,7 @@
 #include <linux/gpio/driver.h>
 #include <linux/gpio/machine.h>
 #include <linux/pinctrl/consumer.h>
+#include <linux/idr.h>
 
 #include "gpiolib.h"
 
@@ -42,6 +43,9 @@
 #define	extra_checks	0
 #endif
 
+/* Device and char device-related information */
+static DEFINE_IDA(gpio_ida);
+
 /* gpio_lock prevents conflicts during gpio_desc[] table updates.
  * While any GPIO is requested, its gpio_chip is not removable;
  * each GPIO's "requested" flag serves as a lock and refcount.
@@ -52,7 +56,6 @@ static DEFINE_MUTEX(gpio_lookup_lock);
 static LIST_HEAD(gpio_lookup_list);
 LIST_HEAD(gpio_chips);
 
-
 static void gpiochip_free_hogs(struct gpio_chip *chip);
 static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip);
 
@@ -300,7 +303,7 @@ int gpiochip_add(struct gpio_chip *chip)
 {
 	unsigned long	flags;
 	int		status = 0;
-	unsigned	id;
+	unsigned	i;
 	int		base = chip->base;
 	struct gpio_desc *descs;
 
@@ -308,6 +311,28 @@ int gpiochip_add(struct gpio_chip *chip)
 	if (!descs)
 		return -ENOMEM;
 
+	/*
+	 * The "dev" member of gpiochip is the parent, and the actual
+	 * device is named "device" for historical reasons.
+	 *
+	 * We memset the struct to zero to avoid reentrance issues.
+	 */
+	memset(&chip->device, 0, sizeof(chip->device));
+	if (chip->dev) {
+		chip->device.parent = chip->dev;
+		chip->device.of_node = chip->dev->of_node;
+	} else {
+#ifdef CONFIG_OF_GPIO
+	/* If the gpiochip has an assigned OF node this takes precedence */
+	if (chip->of_node)
+		chip->device.of_node = chip->of_node;
+#endif
+	}
+	chip->id = ida_simple_get(&gpio_ida, 0, 0, GFP_KERNEL);
+	dev_set_name(&chip->device, "gpiochip%d", chip->id);
+	device_initialize(&chip->device);
+	dev_set_drvdata(&chip->device, chip);
+
 	spin_lock_irqsave(&gpio_lock, flags);
 
 	if (base < 0) {
@@ -326,8 +351,8 @@ int gpiochip_add(struct gpio_chip *chip)
 		goto err_free_descs;
 	}
 
-	for (id = 0; id < chip->ngpio; id++) {
-		struct gpio_desc *desc = &descs[id];
+	for (i = 0; i < chip->ngpio; i++) {
+		struct gpio_desc *desc = &descs[i];
 
 		desc->chip = chip;
 
@@ -361,16 +386,22 @@ int gpiochip_add(struct gpio_chip *chip)
 
 	acpi_gpiochip_add(chip);
 
-	status = gpiochip_sysfs_register(chip);
+	status = device_add(&chip->device);
 	if (status)
 		goto err_remove_chip;
 
+	status = gpiochip_sysfs_register(chip);
+	if (status)
+		goto err_remove_device;
+
 	pr_debug("%s: registered GPIOs %d to %d on device: %s\n", __func__,
 		chip->base, chip->base + chip->ngpio - 1,
 		chip->label ? : "generic");
 
 	return 0;
 
+err_remove_device:
+	device_del(&chip->device);
 err_remove_chip:
 	acpi_gpiochip_remove(chip);
 	gpiochip_free_hogs(chip);
@@ -381,6 +412,7 @@ err_remove_from_list:
 	spin_unlock_irqrestore(&gpio_lock, flags);
 	chip->desc = NULL;
 err_free_descs:
+	ida_simple_remove(&gpio_ida, chip->id);
 	kfree(descs);
 
 	/* failures here can mean systems won't boot... */
@@ -426,6 +458,8 @@ void gpiochip_remove(struct gpio_chip *chip)
 	if (requested)
 		dev_crit(chip->dev, "REMOVING GPIOCHIP WITH GPIOS STILL REQUESTED\n");
 
+	device_del(&chip->device);
+	ida_simple_remove(&gpio_ida, chip->id);
 	kfree(chip->desc);
 	chip->desc = NULL;
 }
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index d1baebf350d8..cf3028eccc4b 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -1,6 +1,7 @@
 #ifndef __LINUX_GPIO_DRIVER_H
 #define __LINUX_GPIO_DRIVER_H
 
+#include <linux/device.h>
 #include <linux/types.h>
 #include <linux/module.h>
 #include <linux/irq.h>
@@ -8,8 +9,8 @@
 #include <linux/irqdomain.h>
 #include <linux/lockdep.h>
 #include <linux/pinctrl/pinctrl.h>
+#include <linux/cdev.h>
 
-struct device;
 struct gpio_desc;
 struct of_phandle_args;
 struct device_node;
@@ -20,7 +21,9 @@ struct seq_file;
 /**
  * struct gpio_chip - abstract a GPIO controller
  * @label: for diagnostics
- * @dev: optional device providing the GPIOs
+ * @id: numerical ID number for the GPIO chip
+ * @device: the GPIO device struct.
+ * @dev: optional parent device (hardware) providing the GPIOs
  * @cdev: class device used by sysfs interface (may be NULL)
  * @owner: helps prevent removal of modules exporting active GPIOs
  * @list: links gpio_chips together for traversal
@@ -89,6 +92,8 @@ struct seq_file;
  */
 struct gpio_chip {
 	const char		*label;
+	int			id;
+	struct device		device;
 	struct device		*dev;
 	struct device		*cdev;
 	struct module		*owner;
-- 
2.4.3

[PATCH 2/6] gpio: refer to gpio device in prints and debugfs

[Linus Walleij]

We use the new struct device inside gpio_chip to related debug
prints and warnings, and we also add it to the debugfs dump.

Cc: Johan Hovold <johan@kernel.org>
Cc: Michael Welling <mwelling@ieee.org>
Cc: Markus Pargmann <mpa@pengutronix.de>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/gpiolib.c | 25 ++++++++++++++-----------
 drivers/gpio/gpiolib.h | 12 ++++++------
 2 files changed, 20 insertions(+), 17 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 0ab4f75b7f8e..7861791657b5 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -208,7 +208,7 @@ static int gpiochip_add_to_list(struct gpio_chip *chip)
 	if (pos != &gpio_chips && pos->prev != &gpio_chips) {
 		_chip = list_entry(pos->prev, struct gpio_chip, list);
 		if (_chip->base + _chip->ngpio > chip->base) {
-			dev_err(chip->dev,
+			dev_err(&chip->device,
 			       "GPIO integer space overlap, cannot add chip\n");
 			err = -EBUSY;
 		}
@@ -270,7 +270,7 @@ static int gpiochip_set_desc_names(struct gpio_chip *gc)
 
 		gpio = gpio_name_to_desc(gc->names[i]);
 		if (gpio)
-			dev_warn(gc->dev, "Detected name collision for "
+			dev_warn(&gc->device, "Detected name collision for "
 				 "GPIO name '%s'\n",
 				 gc->names[i]);
 	}
@@ -456,7 +456,8 @@ void gpiochip_remove(struct gpio_chip *chip)
 	spin_unlock_irqrestore(&gpio_lock, flags);
 
 	if (requested)
-		dev_crit(chip->dev, "REMOVING GPIOCHIP WITH GPIOS STILL REQUESTED\n");
+		dev_crit(&chip->device,
+			 "REMOVING GPIOCHIP WITH GPIOS STILL REQUESTED\n");
 
 	device_del(&chip->device);
 	ida_simple_remove(&gpio_ida, chip->id);
@@ -2534,14 +2535,16 @@ static void gpiolib_seq_stop(struct seq_file *s, void *v)
 static int gpiolib_seq_show(struct seq_file *s, void *v)
 {
 	struct gpio_chip *chip = v;
-	struct device *dev;
-
-	seq_printf(s, "%sGPIOs %d-%d", (char *)s->private,
-			chip->base, chip->base + chip->ngpio - 1);
-	dev = chip->dev;
-	if (dev)
-		seq_printf(s, ", %s/%s", dev->bus ? dev->bus->name : "no-bus",
-			dev_name(dev));
+	struct device *parent;
+
+	seq_printf(s, "%s%s: GPIOs %d-%d", (char *)s->private,
+		   dev_name(&chip->device),
+		   chip->base, chip->base + chip->ngpio - 1);
+	parent = chip->dev;
+	if (parent)
+		seq_printf(s, ", parent: %s/%s",
+			   parent->bus ? parent->bus->name : "no-bus",
+			   dev_name(parent));
 	if (chip->label)
 		seq_printf(s, ", %s", chip->label);
 	if (chip->can_sleep)
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 78e634d1c719..be29037e56b9 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -132,17 +132,17 @@ static int __maybe_unused gpio_chip_hwgpio(const struct gpio_desc *desc)
 /* With chip prefix */
 
 #define chip_emerg(chip, fmt, ...)					\
-	pr_emerg("GPIO chip %s: " fmt, chip->label, ##__VA_ARGS__)
+	dev_emerg(&chip->device, "(%s): " fmt, chip->label, ##__VA_ARGS__)
 #define chip_crit(chip, fmt, ...)					\
-	pr_crit("GPIO chip %s: " fmt, chip->label, ##__VA_ARGS__)
+	dev_crit(&chip->device, "(%s): " fmt, chip->label, ##__VA_ARGS__)
 #define chip_err(chip, fmt, ...)					\
-	pr_err("GPIO chip %s: " fmt, chip->label, ##__VA_ARGS__)
+	dev_err(&chip->device, "(%s): " fmt, chip->label, ##__VA_ARGS__)
 #define chip_warn(chip, fmt, ...)					\
-	pr_warn("GPIO chip %s: " fmt, chip->label, ##__VA_ARGS__)
+	dev_warn(&chip->device, "(%s): " fmt, chip->label, ##__VA_ARGS__)
 #define chip_info(chip, fmt, ...)					\
-	pr_info("GPIO chip %s: " fmt, chip->label, ##__VA_ARGS__)
+	dev_info(&chip->device, "(%s): " fmt, chip->label, ##__VA_ARGS__)
 #define chip_dbg(chip, fmt, ...)					\
-	pr_debug("GPIO chip %s: " fmt, chip->label, ##__VA_ARGS__)
+	dev_dbg(&chip->device, "(%s): " fmt, chip->label, ##__VA_ARGS__)
 
 #ifdef CONFIG_GPIO_SYSFS
 
-- 
2.4.3

[PATCH 3/6] gpio: add a userspace chardev ABI for GPIOs

[Linus Walleij]

A new chardev that is to be used for userspace GPIO access is
added in this patch. It is intended to gradually replace the
horribly broken sysfs ABI.

Using a chardev has many upsides:

- All operations are per-gpiochip, which is the actual
  device underlying the GPIOs, making us tie in to the
  kernel device model properly.

- Hotpluggable GPIO controllers can come and go, as this
  kind of problem has been know to userspace for character
  devices since ages, and if a gpiochip handle is held in
  userspace we know we will break something, whereas the
  sysfs is stateless.

- The one-value-per-file rule of sysfs is really hard to
  maintain when you want to twist more than one knob at a time,
  for example have in-kernel APIs to switch several GPIO
  lines at the same time, and this will be possible to do
  with a single ioctl() from userspace, saving a lot of
  context switching.

We also need to add a new bus type for GPIO. This is
necessary for example for userspace coldplug, where sysfs is
traversed to find the boot-time device nodes and create the
character devices in /dev.

This new chardev ABI is *non* *optional* and can be counted
on to be present in the future, emphasizing the preference
of this ABI.

The ABI only implements one single ioctl() to get the name
and number of GPIO lines of a chip. Even this is debatable:
see it as a minimal example for review. This ABI shall be
ruthlessly reviewed and etched in stone.

The old /sys/class/gpio is still optional to compile in,
but will be deprecated.

Unique device IDs are created using IDR, which is overkill
and insanely scalable, but also well tested.

Cc: Johan Hovold <johan@kernel.org>
Cc: Michael Welling <mwelling@ieee.org>
Cc: Markus Pargmann <mpa@pengutronix.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 MAINTAINERS                 |   1 +
 drivers/gpio/gpiolib.c      | 126 +++++++++++++++++++++++++++++++++++++++++++-
 include/linux/gpio/driver.h |   2 +
 include/uapi/linux/Kbuild   |   1 +
 include/uapi/linux/gpio.h   |  28 ++++++++++
 5 files changed, 157 insertions(+), 1 deletion(-)
 create mode 100644 include/uapi/linux/gpio.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 274f85405584..4a12cd019511 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4646,6 +4646,7 @@ F:	drivers/gpio/
 F:	include/linux/gpio/
 F:	include/linux/gpio.h
 F:	include/asm-generic/gpio.h
+F:	include/uapi/linux/gpio.h
 
 GRE DEMULTIPLEXER DRIVER
 M:	Dmitry Kozlov <xeb@mail.ru>
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 7861791657b5..15c58956a2d0 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -17,6 +17,10 @@
 #include <linux/gpio/machine.h>
 #include <linux/pinctrl/consumer.h>
 #include <linux/idr.h>
+#include <linux/cdev.h>
+#include <linux/fs.h>
+#include <linux/uaccess.h>
+#include <uapi/linux/gpio.h>
 
 #include "gpiolib.h"
 
@@ -45,6 +49,11 @@
 
 /* Device and char device-related information */
 static DEFINE_IDA(gpio_ida);
+static dev_t gpio_devt;
+#define GPIO_DEV_MAX 256 /* 256 GPIO chip devices supported */
+static struct bus_type gpio_bus_type = {
+	.name = "gpio",
+};
 
 /* gpio_lock prevents conflicts during gpio_desc[] table updates.
  * While any GPIO is requested, its gpio_chip is not removable;
@@ -283,6 +292,80 @@ static int gpiochip_set_desc_names(struct gpio_chip *gc)
 }
 
 /**
+ * gpio_ioctl() - ioctl handler for the GPIO chardev
+ */
+static long gpio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
+{
+	struct gpio_chip *chip = filp->private_data;
+	int __user *ip = (int __user *)arg;
+	struct gpiochip_info chipinfo;
+
+	if (!chip)
+		return -ENODEV;
+
+	if (cmd == GPIO_GET_CHIPINFO_IOCTL) {
+		/* Fill in the struct and pass to userspace */
+		if (chip->label)
+			strncpy(chipinfo.name, chip->label,
+				sizeof(chipinfo.name));
+		else
+			strncpy(chipinfo.name, dev_name(&chip->device),
+				sizeof(chipinfo.name));
+		chipinfo.name[sizeof(chipinfo.name)-1] = '\0';
+		chipinfo.lines = chip->ngpio;
+		if (copy_to_user(ip, &chipinfo, sizeof(chipinfo)))
+			return -EFAULT;
+		return 0;
+	}
+	return -EINVAL;
+}
+
+/**
+ * gpio_chrdev_open() - open the chardev for ioctl operations
+ * @inode: inode for this chardev
+ * @filp: file struct for storing private data
+ * Returns 0 on success
+ */
+static int gpio_chrdev_open(struct inode *inode, struct file *filp)
+{
+	struct gpio_chip *chip = container_of(inode->i_cdev,
+					      struct gpio_chip, chrdev);
+
+	if (!chip)
+		return -ENODEV;
+	get_device(&chip->device);
+	filp->private_data = chip;
+	return 0;
+}
+
+/**
+ * gpio_chrdev_release() - close chardev after ioctl operations
+ * @inode: inode for this chardev
+ * @filp: file struct for storing private data
+ * Returns 0 on success
+ */
+static int gpio_chrdev_release(struct inode *inode, struct file *filp)
+{
+	struct gpio_chip *chip = container_of(inode->i_cdev,
+					      struct gpio_chip, chrdev);
+
+	if (!chip)
+		return -ENODEV;
+	put_device(&chip->device);
+	return 0;
+}
+
+
+static const struct file_operations gpio_fileops = {
+	.release = gpio_chrdev_release,
+	.open = gpio_chrdev_open,
+	.owner = THIS_MODULE,
+	.llseek = noop_llseek,
+	.unlocked_ioctl = gpio_ioctl,
+	.compat_ioctl = gpio_ioctl,
+};
+
+/**
  * gpiochip_add() - register a gpio_chip
  * @chip: the chip to register, with chip->base initialized
  * Context: potentially before irqs will work
@@ -318,6 +401,7 @@ int gpiochip_add(struct gpio_chip *chip)
 	 * We memset the struct to zero to avoid reentrance issues.
 	 */
 	memset(&chip->device, 0, sizeof(chip->device));
+	chip->device.bus = &gpio_bus_type;
 	if (chip->dev) {
 		chip->device.parent = chip->dev;
 		chip->device.of_node = chip->dev->of_node;
@@ -386,9 +470,26 @@ int gpiochip_add(struct gpio_chip *chip)
 
 	acpi_gpiochip_add(chip);
 
+	/*
+	 * By first adding the chardev, and then adding the device,
+	 * we get a device node entry in sysfs under
+	 * /sys/bus/gpio/devices/gpiochipN/dev that can be used for
+	 * coldplug of device nodes and other udev business.
+	 */
+	cdev_init(&chip->chrdev, &gpio_fileops);
+	chip->chrdev.owner = THIS_MODULE;
+	chip->chrdev.kobj.parent = &chip->device.kobj;
+	chip->device.devt = MKDEV(MAJOR(gpio_devt), chip->id);
+	status = cdev_add(&chip->chrdev, chip->device.devt, 1);
+	if (status < 0)
+		chip_warn(chip, "failed to add char device %d:%d\n",
+			  MAJOR(gpio_devt), chip->id);
+	else
+		chip_dbg(chip, "added GPIO chardev (%d:%d)\n",
+			 MAJOR(gpio_devt), chip->id);
 	status = device_add(&chip->device);
 	if (status)
-		goto err_remove_chip;
+		goto err_remove_chardev;
 
 	status = gpiochip_sysfs_register(chip);
 	if (status)
@@ -402,6 +503,8 @@ int gpiochip_add(struct gpio_chip *chip)
 
 err_remove_device:
 	device_del(&chip->device);
+err_remove_chardev:
+	cdev_del(&chip->chrdev);
 err_remove_chip:
 	acpi_gpiochip_remove(chip);
 	gpiochip_free_hogs(chip);
@@ -436,6 +539,7 @@ void gpiochip_remove(struct gpio_chip *chip)
 	unsigned	id;
 	bool		requested = false;
 
+	cdev_del(&chip->chrdev);
 	gpiochip_sysfs_unregister(chip);
 
 	gpiochip_irqchip_remove(chip);
@@ -2457,6 +2561,26 @@ void gpiod_put_array(struct gpio_descs *descs)
 }
 EXPORT_SYMBOL_GPL(gpiod_put_array);
 
+static int __init gpiolib_dev_init(void)
+{
+	int ret;
+
+	/* Register GPIO sysfs bus */
+	ret  = bus_register(&gpio_bus_type);
+	if (ret < 0) {
+		pr_err("gpiolib: could not register GPIO bus type\n");
+		return ret;
+	}
+
+	ret = alloc_chrdev_region(&gpio_devt, 0, GPIO_DEV_MAX, "gpiochip");
+	if (ret < 0) {
+		pr_err("gpiolib: failed to allocate char dev region\n");
+		bus_unregister(&gpio_bus_type);
+	}
+	return ret;
+}
+core_initcall(gpiolib_dev_init);
+
 #ifdef CONFIG_DEBUG_FS
 
 static void gpiolib_dbg_show(struct seq_file *s, struct gpio_chip *chip)
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index cf3028eccc4b..826791d51c80 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -23,6 +23,7 @@ struct seq_file;
  * @label: for diagnostics
  * @id: numerical ID number for the GPIO chip
  * @device: the GPIO device struct.
+ * @chrdev: character device for the GPIO device.
  * @dev: optional parent device (hardware) providing the GPIOs
  * @cdev: class device used by sysfs interface (may be NULL)
  * @owner: helps prevent removal of modules exporting active GPIOs
@@ -94,6 +95,7 @@ struct gpio_chip {
 	const char		*label;
 	int			id;
 	struct device		device;
+	struct cdev		chrdev;
 	struct device		*dev;
 	struct device		*cdev;
 	struct module		*owner;
diff --git a/include/uapi/linux/Kbuild b/include/uapi/linux/Kbuild
index f7b2db44eb4b..0cfa57693dcc 100644
--- a/include/uapi/linux/Kbuild
+++ b/include/uapi/linux/Kbuild
@@ -138,6 +138,7 @@ header-y += genetlink.h
 header-y += gen_stats.h
 header-y += gfs2_ondisk.h
 header-y += gigaset_dev.h
+header-y += gpio.h
 header-y += gsmmux.h
 header-y += hdlcdrv.h
 header-y += hdlc.h
diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h
new file mode 100644
index 000000000000..3188a87bdaa0
--- /dev/null
+++ b/include/uapi/linux/gpio.h
@@ -0,0 +1,28 @@
+/*
+ * <linux/gpio.h> - userspace ABI for the GPIO character devices
+ *
+ * Copyright (C) 2015 Linus Walleij
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ */
+#ifndef _UAPI_GPIO_H_
+#define _UAPI_GPIO_H_
+
+#include <linux/ioctl.h>
+#include <linux/types.h>
+
+/**
+ * struct gpiochip_info - Information about a certain GPIO chip
+ * @name: the name of this GPIO chip
+ * @lines: number of GPIO lines on this chip
+ */
+struct gpiochip_info {
+	char name[32];
+	__u32 lines;
+};
+
+#define GPIO_GET_CHIPINFO_IOCTL _IOR('o', 0x01, struct gpiochip_info)
+
+#endif /* _UAPI_GPIO_H_ */
-- 
2.4.3

[PATCH 4/6] tools/gpio: create GPIO tools

[Linus Walleij]

This creates GPIO tools under tools/gpio/* and adds a single
example program to list the GPIOs on a system. When proper
devices are created it provides this minimal output:

Cc: Johan Hovold <johan@kernel.org>
Cc: Michael Welling <mwelling@ieee.org>
Cc: Markus Pargmann <mpa@pengutronix.de>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 MAINTAINERS             |   1 +
 tools/Makefile          |   7 +--
 tools/gpio/Makefile     |  12 +++++
 tools/gpio/gpio-utils.c |  11 +++++
 tools/gpio/gpio-utils.h |  25 ++++++++++
 tools/gpio/lsgpio.c     | 128 ++++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 181 insertions(+), 3 deletions(-)
 create mode 100644 tools/gpio/Makefile
 create mode 100644 tools/gpio/gpio-utils.c
 create mode 100644 tools/gpio/gpio-utils.h
 create mode 100644 tools/gpio/lsgpio.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 4a12cd019511..3bb1f52d54e0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4647,6 +4647,7 @@ F:	include/linux/gpio/
 F:	include/linux/gpio.h
 F:	include/asm-generic/gpio.h
 F:	include/uapi/linux/gpio.h
+F:	tools/gpio/
 
 GRE DEMULTIPLEXER DRIVER
 M:	Dmitry Kozlov <xeb@mail.ru>
diff --git a/tools/Makefile b/tools/Makefile
index d6f307dfb1a3..8912759247b5 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -12,6 +12,7 @@ help:
 	@echo '  cgroup     - cgroup tools'
 	@echo '  cpupower   - a tool for all things x86 CPU power'
 	@echo '  firewire   - the userspace part of nosy, an IEEE-1394 traffic sniffer'
+	@echo '  gpio       - GPIO tools'
 	@echo '  hv         - tools used when in Hyper-V clients'
 	@echo '  iio        - IIO tools'
 	@echo '  lguest     - a minimal 32-bit x86 hypervisor'
@@ -48,7 +49,7 @@ acpi: FORCE
 cpupower: FORCE
 	$(call descend,power/$@)
 
-cgroup firewire hv guest usb virtio vm net iio: FORCE
+cgroup firewire hv guest usb virtio vm net iio gpio: FORCE
 	$(call descend,$@)
 
 liblockdep: FORCE
@@ -109,7 +110,7 @@ acpi_clean:
 cpupower_clean:
 	$(call descend,power/cpupower,clean)
 
-cgroup_clean hv_clean firewire_clean lguest_clean usb_clean virtio_clean vm_clean net_clean iio_clean:
+cgroup_clean hv_clean firewire_clean lguest_clean usb_clean virtio_clean vm_clean net_clean iio_clean gpio_clean:
 	$(call descend,$(@:_clean=),clean)
 
 liblockdep_clean:
@@ -136,6 +137,6 @@ freefall_clean:
 clean: acpi_clean cgroup_clean cpupower_clean hv_clean firewire_clean lguest_clean \
 		perf_clean selftests_clean turbostat_clean usb_clean virtio_clean \
 		vm_clean net_clean iio_clean x86_energy_perf_policy_clean tmon_clean \
-		freefall_clean
+		freefall_clean gpio_clean
 
 .PHONY: FORCE
diff --git a/tools/gpio/Makefile b/tools/gpio/Makefile
new file mode 100644
index 000000000000..4d198d5c4203
--- /dev/null
+++ b/tools/gpio/Makefile
@@ -0,0 +1,12 @@
+CC = $(CROSS_COMPILE)gcc
+CFLAGS += -Wall -g -D_GNU_SOURCE
+
+all: lsgpio
+
+lsgpio: lsgpio.o gpio-utils.o
+
+%.o: %.c gpio-utils.h
+
+.PHONY: clean
+clean:
+	rm -f *.o lsgpio
diff --git a/tools/gpio/gpio-utils.c b/tools/gpio/gpio-utils.c
new file mode 100644
index 000000000000..8208718f2c99
--- /dev/null
+++ b/tools/gpio/gpio-utils.c
@@ -0,0 +1,11 @@
+/*
+ * GPIO tools - helpers library for the GPIO tools
+ *
+ * Copyright (C) 2015 Linus Walleij
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ */
+
+#include "gpio-utils.h"
diff --git a/tools/gpio/gpio-utils.h b/tools/gpio/gpio-utils.h
new file mode 100644
index 000000000000..b18209a45ad3
--- /dev/null
+++ b/tools/gpio/gpio-utils.h
@@ -0,0 +1,25 @@
+/*
+ * GPIO tools - utility helpers library for the GPIO tools
+ *
+ * Copyright (C) 2015 Linus Walleij
+ *
+ * Portions copied from iio_utils and lssio:
+ * Copyright (c) 2010 Manuel Stahl <manuel.stahl@iis.fraunhofer.de>
+ * Copyright (c) 2008 Jonathan Cameron
+ * *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ */
+#ifndef _GPIO_UTILS_H_
+#define _GPIO_UTILS_H_
+
+#include <string.h>
+
+static inline int check_prefix(const char *str, const char *prefix)
+{
+	return strlen(str) > strlen(prefix) &&
+		strncmp(str, prefix, strlen(prefix)) == 0;
+}
+
+#endif /* _GPIO_UTILS_H_ */
diff --git a/tools/gpio/lsgpio.c b/tools/gpio/lsgpio.c
new file mode 100644
index 000000000000..4cfe29da279b
--- /dev/null
+++ b/tools/gpio/lsgpio.c
@@ -0,0 +1,128 @@
+/*
+ * lsgpio - example on how to list the GPIO lines on a system
+ *
+ * Copyright (C) 2015 Linus Walleij
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ *
+ * Usage:
+ *	lsgpio <-n device-name>
+ */
+
+#include <unistd.h>
+#include <stdlib.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <dirent.h>
+#include <errno.h>
+#include <string.h>
+#include <poll.h>
+#include <fcntl.h>
+#include <getopt.h>
+#include <sys/ioctl.h>
+#include <linux/gpio.h>
+
+#include "gpio-utils.h"
+
+int list_device(const char *device_name)
+{
+	struct gpiochip_info cinfo;
+	char *chrdev_name;
+	int fd;
+	int ret;
+
+	ret = asprintf(&chrdev_name, "/dev/%s", device_name);
+	if (ret < 0)
+		return -ENOMEM;
+
+	fd = open(chrdev_name, 0);
+	if (fd == -1) {
+		ret = -errno;
+		fprintf(stderr, "Failed to open %s\n", chrdev_name);
+		goto free_chrdev_name;
+	}
+
+	/* Inspect this GPIO chip */
+	ret = ioctl(fd, GPIO_GET_CHIPINFO_IOCTL, &cinfo);
+	if (ret == -1) {
+		ret = -errno;
+		fprintf(stderr, "Failed to retrieve GPIO fd\n");
+		if (close(fd) == -1)
+			perror("Failed to close GPIO character device file");
+
+		goto free_chrdev_name;
+	}
+	fprintf(stdout, "GPIO chip: %s, %u GPIO lines\n",
+		cinfo.name, cinfo.lines);
+
+	if (close(fd) == -1)  {
+		ret = -errno;
+		goto free_chrdev_name;
+	}
+
+free_chrdev_name:
+	free(chrdev_name);
+
+	return ret;
+
+}
+
+void print_usage(void)
+{
+	fprintf(stderr, "Usage: lsgpio [options]...\n"
+		"List GPIO chips, lines and states\n"
+		"  -n <name>  List GPIOs on a named device\n"
+		"  -?         This helptext\n"
+	);
+}
+
+int main(int argc, char **argv)
+{
+	const char *device_name;
+	int ret;
+	int c;
+
+	while ((c = getopt(argc, argv, "n:")) != -1) {
+		switch (c) {
+		case 'n':
+			device_name = optarg;
+			break;
+		case '?':
+			print_usage();
+			return -1;
+		}
+	}
+
+	if (device_name)
+		ret = list_device(device_name);
+	else {
+		const struct dirent *ent;
+		DIR *dp;
+
+		/* List all GPIO devices one at a time */
+		dp = opendir("/dev");
+		if (!dp) {
+			ret = -errno;
+			goto error_out;
+		}
+
+		ret = -ENOENT;
+		while (ent = readdir(dp), ent) {
+			if (check_prefix(ent->d_name, "gpiochip")) {
+				ret = list_device(ent->d_name);
+				if (ret)
+					break;
+			}
+		}
+
+		ret = 0;
+		if (closedir(dp) == -1) {
+			perror("scanning devices: Failed to close directory");
+			ret = -errno;
+		}
+	}
+error_out:
+	return ret;
+}
-- 
2.4.3

[PATCH 5/6] gpio: add a userspace character device ABI

[Linus Walleij]

Put in some documentation for the new character device ABI
so we can properly etch it in stone.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 Documentation/ABI/testing/gpio-cdev | 26 ++++++++++++++++++++++++++
 MAINTAINERS                         |  1 +
 2 files changed, 27 insertions(+)
 create mode 100644 Documentation/ABI/testing/gpio-cdev

diff --git a/Documentation/ABI/testing/gpio-cdev b/Documentation/ABI/testing/gpio-cdev
new file mode 100644
index 000000000000..7b265fbb47e3
--- /dev/null
+++ b/Documentation/ABI/testing/gpio-cdev
@@ -0,0 +1,26 @@
+What:		/dev/gpiochip[0-9]+
+Date:		November 2015
+KernelVersion:	4.4
+Contact:	linux-gpio@vger.kernel.org
+Description:
+		The character device files /dev/gpiochip* are the interface
+		between GPIO chips and userspace.
+
+		The ioctl(2)-based ABI is defined and documented in
+		[include/uapi]<linux/gpio.h>.
+
+		The following file operations are supported:
+
+		open(2)
+		Currently the only useful flags are O_RDWR.
+
+		ioctl(2)
+		Initiate various actions.
+		See the inline documentation in [include/uapi]<linux/gpio.h>
+		for descriptions of all ioctls.
+
+		close(2)
+		Stops and free up the I/O contexts that was associated
+		with the file descriptor.
+
+Users:		TBD
diff --git a/MAINTAINERS b/MAINTAINERS
index 3bb1f52d54e0..23cd966cb974 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4642,6 +4642,7 @@ L:	linux-gpio@vger.kernel.org
 T:	git git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git
 S:	Maintained
 F:	Documentation/gpio/
+F:	Documentation/ABI/testing/gpio-cdev
 F:	drivers/gpio/
 F:	include/linux/gpio/
 F:	include/linux/gpio.h
-- 
2.4.3

[PATCH 6/6] gpio: ABI: mark the sysfs ABI as obsolete

[Linus Walleij]

This marks the (optional) sysfs GPIO ABI as obsolete and schedules
it for removal in 2020.

Cc: Johan Hovold <johan@kernel.org>
Cc: Michael Welling <mwelling@ieee.org>
Cc: Markus Pargmann <mpa@pengutronix.de>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 Documentation/ABI/obsolete/sysfs-gpio | 30 ++++++++++++++++++++++++++++++
 Documentation/ABI/testing/sysfs-gpio  | 28 ----------------------------
 MAINTAINERS                           |  1 +
 3 files changed, 31 insertions(+), 28 deletions(-)
 create mode 100644 Documentation/ABI/obsolete/sysfs-gpio
 delete mode 100644 Documentation/ABI/testing/sysfs-gpio

diff --git a/Documentation/ABI/obsolete/sysfs-gpio b/Documentation/ABI/obsolete/sysfs-gpio
new file mode 100644
index 000000000000..867c1fab20e2
--- /dev/null
+++ b/Documentation/ABI/obsolete/sysfs-gpio
@@ -0,0 +1,30 @@
+What:		/sys/class/gpio/
+Date:		July 2008
+KernelVersion:	2.6.27
+Contact:	Linus Walleij <linusw@kernel.org>
+Description:
+
+  As a Kconfig option, individual GPIO signals may be accessed from
+  userspace.  GPIOs are only made available to userspace by an explicit
+  "export" operation.  If a given GPIO is not claimed for use by
+  kernel code, it may be exported by userspace (and unexported later).
+  Kernel code may export it for complete or partial access.
+
+  GPIOs are identified as they are inside the kernel, using integers in
+  the range 0..INT_MAX.  See Documentation/gpio.txt for more information.
+
+    /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 OR
+	/<LINE-NAME> ... for a properly named GPIO line
+	    /value ... always readable, writes fail for input GPIOs
+	    /direction ... r/w as: in, out (default low); write: high, low
+	    /edge ... r/w as: none, falling, rising, both
+	/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 to N + (ngpio - 1)
+
+  This ABI is deprecated and will be removed after 2020. It is
+  replaced with the GPIO character device.
diff --git a/Documentation/ABI/testing/sysfs-gpio b/Documentation/ABI/testing/sysfs-gpio
deleted file mode 100644
index 55ffa2df1c10..000000000000
--- a/Documentation/ABI/testing/sysfs-gpio
+++ /dev/null
@@ -1,28 +0,0 @@
-What:		/sys/class/gpio/
-Date:		July 2008
-KernelVersion:	2.6.27
-Contact:	David Brownell <dbrownell@users.sourceforge.net>
-Description:
-
-  As a Kconfig option, individual GPIO signals may be accessed from
-  userspace.  GPIOs are only made available to userspace by an explicit
-  "export" operation.  If a given GPIO is not claimed for use by
-  kernel code, it may be exported by userspace (and unexported later).
-  Kernel code may export it for complete or partial access.
-
-  GPIOs are identified as they are inside the kernel, using integers in
-  the range 0..INT_MAX.  See Documentation/gpio.txt for more information.
-
-    /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 OR
-	/<LINE-NAME> ... for a properly named GPIO line
-	    /value ... always readable, writes fail for input GPIOs
-	    /direction ... r/w as: in, out (default low); write: high, low
-	    /edge ... r/w as: none, falling, rising, both
-	/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 to N + (ngpio - 1)
-
diff --git a/MAINTAINERS b/MAINTAINERS
index 23cd966cb974..0efc993249a1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4643,6 +4643,7 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git
 S:	Maintained
 F:	Documentation/gpio/
 F:	Documentation/ABI/testing/gpio-cdev
+F:	Documentation/ABI/obsolete/sysfs-gpio
 F:	drivers/gpio/
 F:	include/linux/gpio/
 F:	include/linux/gpio.h
-- 
2.4.3

Re: [PATCH 0/6] GPIO character device skeleton

[Michael Welling]

On Thu, Oct 22, 2015 at 10:32:24AM +0200, Linus Walleij wrote:
> OK so this is it, I had no patience waiting for users to come up
> with this new ABI, and the requests for a way for userspace to
> use GPIOs properly is coming up again and again. So I created the
> basics for it, so we can then build on top of this to get things
> right. I want to get these very first things right before we go
> wild with setting/getting pin values etc.
> 
> We add ONE ioctl() to get information on the gpiochip. Now we can
> do this (example from ux500):
> 
> root@Ux500:/ lsgpio
> GPIO chip: a03fe000.gpio, 32 GPIO lines
> GPIO chip: 8011e080.gpio, 32 GPIO lines
> GPIO chip: 8011e000.gpio, 32 GPIO lines
> GPIO chip: 8000e180.gpio, 32 GPIO lines
> GPIO chip: 8000e100.gpio, 32 GPIO lines
> GPIO chip: 8000e080.gpio, 32 GPIO lines
> GPIO chip: 8000e000.gpio, 32 GPIO lines
> GPIO chip: 8012e080.gpio, 32 GPIO lines
> GPIO chip: 8012e000.gpio, 32 GPIO lines
> GPIO chip: abx500-gpio, 42 GPIO lines
> GPIO chip: tc3589x, 20 GPIO lines
>

Linus,

I have compiled and tested this patch series on an iMX6 based target.

root@som-imx6q:~# lsgpio
GPIO chip: 209c000.gpio, 32 GPIO lines
GPIO chip: 20a0000.gpio, 32 GPIO lines
GPIO chip: 20a4000.gpio, 32 GPIO lines
GPIO chip: 20a8000.gpio, 32 GPIO lines
GPIO chip: 20ac000.gpio, 32 GPIO lines
GPIO chip: 20b0000.gpio, 32 GPIO lines
GPIO chip: 20b4000.gpio, 32 GPIO lines
root@som-imx6q:~# uname -a
Linux som-imx6q 4.3.0-rc6-next-20151022-dirty #1 SMP Thu Oct 22 09:55:12 CDT 2015 armv7l GNU/Linux

Regards,
Michael
 
> Johan: I don't have a hot-pluggable GPIO controller :( can you
> do me the favour of testing this and fixing my stupid refcounts
> and race conditions? I only used my brain to try to get things
> right with pluggable GPIO controllers in this patch set, and it
> is bound to fail.
> 
> How to identify and manipulate individual GPIO lines from this
> ABI is a *LATER* *QUESTION*, this is the bare essentials for
> getting there: basic operations on the gpiochip device level.
> 
> Linus Walleij (6):
>   gpio: make the gpiochip a real device
>   gpio: refer to gpio device in prints and debugfs
>   gpio: add a userspace chardev ABI for GPIOs
>   tools/gpio: create GPIO tools
>   gpio: add a userspace character device ABI
>   gpio: ABI: mark the sysfs ABI as obsolete
> 
>  Documentation/ABI/obsolete/sysfs-gpio |  30 ++++++
>  Documentation/ABI/testing/gpio-cdev   |  26 +++++
>  Documentation/ABI/testing/sysfs-gpio  |  28 -----
>  MAINTAINERS                           |   4 +
>  drivers/gpio/gpiolib-sysfs.c          |   2 +-
>  drivers/gpio/gpiolib.c                | 193 +++++++++++++++++++++++++++++++---
>  drivers/gpio/gpiolib.h                |  12 +--
>  include/linux/gpio/driver.h           |  11 +-
>  include/uapi/linux/Kbuild             |   1 +
>  include/uapi/linux/gpio.h             |  28 +++++
>  tools/Makefile                        |   7 +-
>  tools/gpio/Makefile                   |  12 +++
>  tools/gpio/gpio-utils.c               |  11 ++
>  tools/gpio/gpio-utils.h               |  25 +++++
>  tools/gpio/lsgpio.c                   | 128 ++++++++++++++++++++++
>  15 files changed, 462 insertions(+), 56 deletions(-)
>  create mode 100644 Documentation/ABI/obsolete/sysfs-gpio
>  create mode 100644 Documentation/ABI/testing/gpio-cdev
>  delete mode 100644 Documentation/ABI/testing/sysfs-gpio
>  create mode 100644 include/uapi/linux/gpio.h
>  create mode 100644 tools/gpio/Makefile
>  create mode 100644 tools/gpio/gpio-utils.c
>  create mode 100644 tools/gpio/gpio-utils.h
>  create mode 100644 tools/gpio/lsgpio.c
> 
> -- 
> 2.4.3
> 

Re: [PATCH 3/6] gpio: add a userspace chardev ABI for GPIOs

[Michael Welling]

On Thu, Oct 22, 2015 at 10:32:27AM +0200, Linus Walleij wrote:
> - Hotpluggable GPIO controllers can come and go, as this
>   kind of problem has been know to userspace for character

.. has been known ..

Re: [PATCH 3/6] gpio: add a userspace chardev ABI for GPIOs

[Greg Kroah-Hartman]

On Thu, Oct 22, 2015 at 10:32:27AM +0200, Linus Walleij wrote:
> +/**
> + * struct gpiochip_info - Information about a certain GPIO chip
> + * @name: the name of this GPIO chip
> + * @lines: number of GPIO lines on this chip
> + */
> +struct gpiochip_info {
> +	char name[32];

To be pendantic, s/char/__u8/

Otherwise, looks good, but I don't see the read/write protocol here, is
that in a later patch?

thanks,

greg k-h

Re: [PATCH 0/6] GPIO character device skeleton

[Markus Pargmann]

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

Hi,

On Thu, Oct 22, 2015 at 10:32:24AM +0200, Linus Walleij wrote:
> OK so this is it, I had no patience waiting for users to come up
> with this new ABI, and the requests for a way for userspace to
> use GPIOs properly is coming up again and again. So I created the
> basics for it, so we can then build on top of this to get things
> right. I want to get these very first things right before we go
> wild with setting/getting pin values etc.

Thanks for taking the time for this. This basic implementation seems
nice.

> 
> We add ONE ioctl() to get information on the gpiochip. Now we can
> do this (example from ux500):
> 
> root@Ux500:/ lsgpio
> GPIO chip: a03fe000.gpio, 32 GPIO lines
> GPIO chip: 8011e080.gpio, 32 GPIO lines
> GPIO chip: 8011e000.gpio, 32 GPIO lines
> GPIO chip: 8000e180.gpio, 32 GPIO lines
> GPIO chip: 8000e100.gpio, 32 GPIO lines
> GPIO chip: 8000e080.gpio, 32 GPIO lines
> GPIO chip: 8000e000.gpio, 32 GPIO lines
> GPIO chip: 8012e080.gpio, 32 GPIO lines
> GPIO chip: 8012e000.gpio, 32 GPIO lines
> GPIO chip: abx500-gpio, 42 GPIO lines
> GPIO chip: tc3589x, 20 GPIO lines
> 
> Johan: I don't have a hot-pluggable GPIO controller :( can you
> do me the favour of testing this and fixing my stupid refcounts
> and race conditions? I only used my brain to try to get things
> right with pluggable GPIO controllers in this patch set, and it
> is bound to fail.
> 
> How to identify and manipulate individual GPIO lines from this
> ABI is a *LATER* *QUESTION*, this is the bare essentials for
> getting there: basic operations on the gpiochip device level.

Does this include identifying GPIO lines independent of gpiochips? I
think that is a use-case as well.

Best Regards,

Markus

> 
> Linus Walleij (6):
>   gpio: make the gpiochip a real device
>   gpio: refer to gpio device in prints and debugfs
>   gpio: add a userspace chardev ABI for GPIOs
>   tools/gpio: create GPIO tools
>   gpio: add a userspace character device ABI
>   gpio: ABI: mark the sysfs ABI as obsolete
> 
>  Documentation/ABI/obsolete/sysfs-gpio |  30 ++++++
>  Documentation/ABI/testing/gpio-cdev   |  26 +++++
>  Documentation/ABI/testing/sysfs-gpio  |  28 -----
>  MAINTAINERS                           |   4 +
>  drivers/gpio/gpiolib-sysfs.c          |   2 +-
>  drivers/gpio/gpiolib.c                | 193 +++++++++++++++++++++++++++++++---
>  drivers/gpio/gpiolib.h                |  12 +--
>  include/linux/gpio/driver.h           |  11 +-
>  include/uapi/linux/Kbuild             |   1 +
>  include/uapi/linux/gpio.h             |  28 +++++
>  tools/Makefile                        |   7 +-
>  tools/gpio/Makefile                   |  12 +++
>  tools/gpio/gpio-utils.c               |  11 ++
>  tools/gpio/gpio-utils.h               |  25 +++++
>  tools/gpio/lsgpio.c                   | 128 ++++++++++++++++++++++
>  15 files changed, 462 insertions(+), 56 deletions(-)
>  create mode 100644 Documentation/ABI/obsolete/sysfs-gpio
>  create mode 100644 Documentation/ABI/testing/gpio-cdev
>  delete mode 100644 Documentation/ABI/testing/sysfs-gpio
>  create mode 100644 include/uapi/linux/gpio.h
>  create mode 100644 tools/gpio/Makefile
>  create mode 100644 tools/gpio/gpio-utils.c
>  create mode 100644 tools/gpio/gpio-utils.h
>  create mode 100644 tools/gpio/lsgpio.c
> 
> -- 
> 2.4.3
> 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/6] gpio: make the gpiochip a real device

[Markus Pargmann]

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

On Thu, Oct 22, 2015 at 10:32:25AM +0200, Linus Walleij wrote:
> GPIO chips have been around for years, but were never real devices,
> instead they were piggy-backing on a parent device (such as a
> platform_device or amba_device) but this was always optional.
> GPIO chips could also exist without any device at all, with its
> struct device *dev pointer being set to null.
> 
> When sysfs was in use, a mock device would be created, with the
> optional parent assigned, or just floating orphaned with NULL
> as parent.
> 
> For a proper userspace ABI we need gpiochips to *always have a
> populated struct device, so add this in the gpio_chip struct.
> The name "dev" is unfortunately already take so we use "device"
> to name it.
> 
> If sysfs is active, it will use this device as parent, and the
> former parent device "dev" will be set as parent of the new
> "device" struct member.
> 
> From this point on, gpiochips are devices.
> 
> Cc: Johan Hovold <johan@kernel.org>
> Cc: Michael Welling <mwelling@ieee.org>
> Cc: Markus Pargmann <mpa@pengutronix.de>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/gpio/gpiolib-sysfs.c |  2 +-
>  drivers/gpio/gpiolib.c       | 44 +++++++++++++++++++++++++++++++++++++++-----
>  include/linux/gpio/driver.h  |  9 +++++++--
>  3 files changed, 47 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
> index b57ed8e55ab5..7e5bc5736e47 100644
> --- a/drivers/gpio/gpiolib-sysfs.c
> +++ b/drivers/gpio/gpiolib-sysfs.c
> @@ -605,7 +605,7 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
>  	if (chip->names && chip->names[offset])
>  		ioname = chip->names[offset];
>  
> -	dev = device_create_with_groups(&gpio_class, chip->dev,
> +	dev = device_create_with_groups(&gpio_class, &chip->device,
>  					MKDEV(0, 0), data, gpio_groups,
>  					ioname ? ioname : "gpio%u",
>  					desc_to_gpio(desc));
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 6798355c61c6..0ab4f75b7f8e 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -16,6 +16,7 @@
>  #include <linux/gpio/driver.h>
>  #include <linux/gpio/machine.h>
>  #include <linux/pinctrl/consumer.h>
> +#include <linux/idr.h>
>  
>  #include "gpiolib.h"
>  
> @@ -42,6 +43,9 @@
>  #define	extra_checks	0
>  #endif
>  
> +/* Device and char device-related information */
> +static DEFINE_IDA(gpio_ida);
> +
>  /* gpio_lock prevents conflicts during gpio_desc[] table updates.
>   * While any GPIO is requested, its gpio_chip is not removable;
>   * each GPIO's "requested" flag serves as a lock and refcount.
> @@ -52,7 +56,6 @@ static DEFINE_MUTEX(gpio_lookup_lock);
>  static LIST_HEAD(gpio_lookup_list);
>  LIST_HEAD(gpio_chips);
>  
> -
>  static void gpiochip_free_hogs(struct gpio_chip *chip);
>  static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip);
>  
> @@ -300,7 +303,7 @@ int gpiochip_add(struct gpio_chip *chip)
>  {
>  	unsigned long	flags;
>  	int		status = 0;
> -	unsigned	id;
> +	unsigned	i;
>  	int		base = chip->base;
>  	struct gpio_desc *descs;
>  
> @@ -308,6 +311,28 @@ int gpiochip_add(struct gpio_chip *chip)
>  	if (!descs)
>  		return -ENOMEM;
>  
> +	/*
> +	 * The "dev" member of gpiochip is the parent, and the actual
> +	 * device is named "device" for historical reasons.
> +	 *
> +	 * We memset the struct to zero to avoid reentrance issues.
> +	 */
> +	memset(&chip->device, 0, sizeof(chip->device));
> +	if (chip->dev) {
> +		chip->device.parent = chip->dev;
> +		chip->device.of_node = chip->dev->of_node;
> +	} else {
> +#ifdef CONFIG_OF_GPIO
> +	/* If the gpiochip has an assigned OF node this takes precedence */
> +	if (chip->of_node)
> +		chip->device.of_node = chip->of_node;

This is in the else case. I think this should be indented an additional
level.

Best Regards,

Markus

> +#endif
> +	}
> +	chip->id = ida_simple_get(&gpio_ida, 0, 0, GFP_KERNEL);
> +	dev_set_name(&chip->device, "gpiochip%d", chip->id);
> +	device_initialize(&chip->device);
> +	dev_set_drvdata(&chip->device, chip);
> +
>  	spin_lock_irqsave(&gpio_lock, flags);
>  
>  	if (base < 0) {
> @@ -326,8 +351,8 @@ int gpiochip_add(struct gpio_chip *chip)
>  		goto err_free_descs;
>  	}
>  
> -	for (id = 0; id < chip->ngpio; id++) {
> -		struct gpio_desc *desc = &descs[id];
> +	for (i = 0; i < chip->ngpio; i++) {
> +		struct gpio_desc *desc = &descs[i];
>  
>  		desc->chip = chip;
>  
> @@ -361,16 +386,22 @@ int gpiochip_add(struct gpio_chip *chip)
>  
>  	acpi_gpiochip_add(chip);
>  
> -	status = gpiochip_sysfs_register(chip);
> +	status = device_add(&chip->device);
>  	if (status)
>  		goto err_remove_chip;
>  
> +	status = gpiochip_sysfs_register(chip);
> +	if (status)
> +		goto err_remove_device;
> +
>  	pr_debug("%s: registered GPIOs %d to %d on device: %s\n", __func__,
>  		chip->base, chip->base + chip->ngpio - 1,
>  		chip->label ? : "generic");
>  
>  	return 0;
>  
> +err_remove_device:
> +	device_del(&chip->device);
>  err_remove_chip:
>  	acpi_gpiochip_remove(chip);
>  	gpiochip_free_hogs(chip);
> @@ -381,6 +412,7 @@ err_remove_from_list:
>  	spin_unlock_irqrestore(&gpio_lock, flags);
>  	chip->desc = NULL;
>  err_free_descs:
> +	ida_simple_remove(&gpio_ida, chip->id);
>  	kfree(descs);
>  
>  	/* failures here can mean systems won't boot... */
> @@ -426,6 +458,8 @@ void gpiochip_remove(struct gpio_chip *chip)
>  	if (requested)
>  		dev_crit(chip->dev, "REMOVING GPIOCHIP WITH GPIOS STILL REQUESTED\n");
>  
> +	device_del(&chip->device);
> +	ida_simple_remove(&gpio_ida, chip->id);
>  	kfree(chip->desc);
>  	chip->desc = NULL;
>  }
> diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
> index d1baebf350d8..cf3028eccc4b 100644
> --- a/include/linux/gpio/driver.h
> +++ b/include/linux/gpio/driver.h
> @@ -1,6 +1,7 @@
>  #ifndef __LINUX_GPIO_DRIVER_H
>  #define __LINUX_GPIO_DRIVER_H
>  
> +#include <linux/device.h>
>  #include <linux/types.h>
>  #include <linux/module.h>
>  #include <linux/irq.h>
> @@ -8,8 +9,8 @@
>  #include <linux/irqdomain.h>
>  #include <linux/lockdep.h>
>  #include <linux/pinctrl/pinctrl.h>
> +#include <linux/cdev.h>
>  
> -struct device;
>  struct gpio_desc;
>  struct of_phandle_args;
>  struct device_node;
> @@ -20,7 +21,9 @@ struct seq_file;
>  /**
>   * struct gpio_chip - abstract a GPIO controller
>   * @label: for diagnostics
> - * @dev: optional device providing the GPIOs
> + * @id: numerical ID number for the GPIO chip
> + * @device: the GPIO device struct.
> + * @dev: optional parent device (hardware) providing the GPIOs
>   * @cdev: class device used by sysfs interface (may be NULL)
>   * @owner: helps prevent removal of modules exporting active GPIOs
>   * @list: links gpio_chips together for traversal
> @@ -89,6 +92,8 @@ struct seq_file;
>   */
>  struct gpio_chip {
>  	const char		*label;
> +	int			id;
> +	struct device		device;
>  	struct device		*dev;
>  	struct device		*cdev;
>  	struct module		*owner;
> -- 
> 2.4.3
> 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 0/6] GPIO character device skeleton

[Markus Pargmann]

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

On Thu, Oct 22, 2015 at 10:32:24AM +0200, Linus Walleij wrote:
> OK so this is it, I had no patience waiting for users to come up
> with this new ABI, and the requests for a way for userspace to
> use GPIOs properly is coming up again and again. So I created the
> basics for it, so we can then build on top of this to get things
> right. I want to get these very first things right before we go
> wild with setting/getting pin values etc.
> 
> We add ONE ioctl() to get information on the gpiochip. Now we can
> do this (example from ux500):
> 
> root@Ux500:/ lsgpio
> GPIO chip: a03fe000.gpio, 32 GPIO lines
> GPIO chip: 8011e080.gpio, 32 GPIO lines
> GPIO chip: 8011e000.gpio, 32 GPIO lines
> GPIO chip: 8000e180.gpio, 32 GPIO lines
> GPIO chip: 8000e100.gpio, 32 GPIO lines
> GPIO chip: 8000e080.gpio, 32 GPIO lines
> GPIO chip: 8000e000.gpio, 32 GPIO lines
> GPIO chip: 8012e080.gpio, 32 GPIO lines
> GPIO chip: 8012e000.gpio, 32 GPIO lines
> GPIO chip: abx500-gpio, 42 GPIO lines
> GPIO chip: tc3589x, 20 GPIO lines

What happens if we have two I2C gpio expanders with the same I2C
addresses connected to different I2C busses? If I see this correctly
they would both show up with the same name. Is there an easy and
race-free way to see which GPIO chip is connected to which I2C bus?

Best Regards,

Markus

> 
> Johan: I don't have a hot-pluggable GPIO controller :( can you
> do me the favour of testing this and fixing my stupid refcounts
> and race conditions? I only used my brain to try to get things
> right with pluggable GPIO controllers in this patch set, and it
> is bound to fail.
> 
> How to identify and manipulate individual GPIO lines from this
> ABI is a *LATER* *QUESTION*, this is the bare essentials for
> getting there: basic operations on the gpiochip device level.
> 
> Linus Walleij (6):
>   gpio: make the gpiochip a real device
>   gpio: refer to gpio device in prints and debugfs
>   gpio: add a userspace chardev ABI for GPIOs
>   tools/gpio: create GPIO tools
>   gpio: add a userspace character device ABI
>   gpio: ABI: mark the sysfs ABI as obsolete
> 
>  Documentation/ABI/obsolete/sysfs-gpio |  30 ++++++
>  Documentation/ABI/testing/gpio-cdev   |  26 +++++
>  Documentation/ABI/testing/sysfs-gpio  |  28 -----
>  MAINTAINERS                           |   4 +
>  drivers/gpio/gpiolib-sysfs.c          |   2 +-
>  drivers/gpio/gpiolib.c                | 193 +++++++++++++++++++++++++++++++---
>  drivers/gpio/gpiolib.h                |  12 +--
>  include/linux/gpio/driver.h           |  11 +-
>  include/uapi/linux/Kbuild             |   1 +
>  include/uapi/linux/gpio.h             |  28 +++++
>  tools/Makefile                        |   7 +-
>  tools/gpio/Makefile                   |  12 +++
>  tools/gpio/gpio-utils.c               |  11 ++
>  tools/gpio/gpio-utils.h               |  25 +++++
>  tools/gpio/lsgpio.c                   | 128 ++++++++++++++++++++++
>  15 files changed, 462 insertions(+), 56 deletions(-)
>  create mode 100644 Documentation/ABI/obsolete/sysfs-gpio
>  create mode 100644 Documentation/ABI/testing/gpio-cdev
>  delete mode 100644 Documentation/ABI/testing/sysfs-gpio
>  create mode 100644 include/uapi/linux/gpio.h
>  create mode 100644 tools/gpio/Makefile
>  create mode 100644 tools/gpio/gpio-utils.c
>  create mode 100644 tools/gpio/gpio-utils.h
>  create mode 100644 tools/gpio/lsgpio.c
> 
> -- 
> 2.4.3
> 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 5/6] gpio: add a userspace character device ABI

[Markus Pargmann]

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

On Thu, Oct 22, 2015 at 10:32:29AM +0200, Linus Walleij wrote:
> Put in some documentation for the new character device ABI
> so we can properly etch it in stone.
> 
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  Documentation/ABI/testing/gpio-cdev | 26 ++++++++++++++++++++++++++
>  MAINTAINERS                         |  1 +
>  2 files changed, 27 insertions(+)
>  create mode 100644 Documentation/ABI/testing/gpio-cdev
> 
> diff --git a/Documentation/ABI/testing/gpio-cdev b/Documentation/ABI/testing/gpio-cdev
> new file mode 100644
> index 000000000000..7b265fbb47e3
> --- /dev/null
> +++ b/Documentation/ABI/testing/gpio-cdev
> @@ -0,0 +1,26 @@
> +What:		/dev/gpiochip[0-9]+
> +Date:		November 2015
> +KernelVersion:	4.4
> +Contact:	linux-gpio@vger.kernel.org
> +Description:
> +		The character device files /dev/gpiochip* are the interface
> +		between GPIO chips and userspace.
> +
> +		The ioctl(2)-based ABI is defined and documented in
> +		[include/uapi]<linux/gpio.h>.
> +
> +		The following file operations are supported:
> +
> +		open(2)
> +		Currently the only useful flags are O_RDWR.
> +
> +		ioctl(2)
> +		Initiate various actions.
> +		See the inline documentation in [include/uapi]<linux/gpio.h>
> +		for descriptions of all ioctls.

Minor thing: The one ioctl is obvious but has no inline documentation at
the moment.

Best Regards,

Markus

> +
> +		close(2)
> +		Stops and free up the I/O contexts that was associated
> +		with the file descriptor.
> +
> +Users:		TBD
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 3bb1f52d54e0..23cd966cb974 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4642,6 +4642,7 @@ L:	linux-gpio@vger.kernel.org
>  T:	git git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git
>  S:	Maintained
>  F:	Documentation/gpio/
> +F:	Documentation/ABI/testing/gpio-cdev
>  F:	drivers/gpio/
>  F:	include/linux/gpio/
>  F:	include/linux/gpio.h
> -- 
> 2.4.3
> 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/6] gpio: make the gpiochip a real device

[Alexandre Courbot]

On Thu, Oct 22, 2015 at 5:32 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> GPIO chips have been around for years, but were never real devices,
> instead they were piggy-backing on a parent device (such as a
> platform_device or amba_device) but this was always optional.
> GPIO chips could also exist without any device at all, with its
> struct device *dev pointer being set to null.
>
> When sysfs was in use, a mock device would be created, with the
> optional parent assigned, or just floating orphaned with NULL
> as parent.
>
> For a proper userspace ABI we need gpiochips to *always have a
> populated struct device, so add this in the gpio_chip struct.
> The name "dev" is unfortunately already take so we use "device"
> to name it.
>
> If sysfs is active, it will use this device as parent, and the
> former parent device "dev" will be set as parent of the new
> "device" struct member.

Why not rename "dev" to "parent" so "dev" becomes what we expect it to
be? The two members being of the same type, keeping it that way seems
error-prone to me.

Re: [PATCH 4/6] tools/gpio: create GPIO tools

[Alexandre Courbot]

On Thu, Oct 22, 2015 at 5:32 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> This creates GPIO tools under tools/gpio/* and adds a single
> example program to list the GPIOs on a system. When proper
> devices are created it provides this minimal output:
>

Output missing in the commit log, unless you wanted to say that it
produces no output. ;)

Re: [PATCH 3/6] gpio: add a userspace chardev ABI for GPIOs

[Mark Brown]

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

On Thu, Oct 22, 2015 at 10:32:27AM +0200, Linus Walleij wrote:

> +	if (cmd == GPIO_GET_CHIPINFO_IOCTL) {

> +		return 0;
> +	}
> +	return -EINVAL;

A switch statement might be more scalable as more ioctl()s are added?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/6] gpio: make the gpiochip a real device

[Mark Brown]

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

On Sun, Oct 25, 2015 at 04:06:26PM +0900, Alexandre Courbot wrote:
> On Thu, Oct 22, 2015 at 5:32 PM, Linus Walleij <linus.walleij@linaro.org> wrote:

> > If sysfs is active, it will use this device as parent, and the
> > former parent device "dev" will be set as parent of the new
> > "device" struct member.

> Why not rename "dev" to "parent" so "dev" becomes what we expect it to
> be? The two members being of the same type, keeping it that way seems
> error-prone to me.

Indeed, that's going to be hard to follow with nothing in the names that
says which is which.  Another option that some subsystems use is to add
the subsystem specific device with a name like gpio_dev that indicates
it's the subsystem version of the device.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 0/6] GPIO character device skeleton

[Mark Brown]

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

On Thu, Oct 22, 2015 at 10:32:24AM +0200, Linus Walleij wrote:
> OK so this is it, I had no patience waiting for users to come up
> with this new ABI, and the requests for a way for userspace to
> use GPIOs properly is coming up again and again. So I created the
> basics for it, so we can then build on top of this to get things
> right. I want to get these very first things right before we go
> wild with setting/getting pin values etc.

This all makes sense to me modulo the (fairly minor) comments that have
already been made.

> We add ONE ioctl() to get information on the gpiochip. Now we can
> do this (example from ux500):

I guess a basic set/get API is going to be need to deprecate the sysfs
ABI.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 0/6] GPIO character device skeleton

[Alexandre Courbot]

Hi Linus,

On Thu, Oct 22, 2015 at 5:32 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> OK so this is it, I had no patience waiting for users to come up
> with this new ABI, and the requests for a way for userspace to
> use GPIOs properly is coming up again and again. So I created the
> basics for it, so we can then build on top of this to get things
> right. I want to get these very first things right before we go
> wild with setting/getting pin values etc.
>
> We add ONE ioctl() to get information on the gpiochip. Now we can
> do this (example from ux500):
>
> root@Ux500:/ lsgpio
> GPIO chip: a03fe000.gpio, 32 GPIO lines
> GPIO chip: 8011e080.gpio, 32 GPIO lines
> GPIO chip: 8011e000.gpio, 32 GPIO lines
> GPIO chip: 8000e180.gpio, 32 GPIO lines
> GPIO chip: 8000e100.gpio, 32 GPIO lines
> GPIO chip: 8000e080.gpio, 32 GPIO lines
> GPIO chip: 8000e000.gpio, 32 GPIO lines
> GPIO chip: 8012e080.gpio, 32 GPIO lines
> GPIO chip: 8012e000.gpio, 32 GPIO lines
> GPIO chip: abx500-gpio, 42 GPIO lines
> GPIO chip: tc3589x, 20 GPIO lines

I see where we are going with the general idea and it would be nice to
finally get things right. Actually despite my initial reserves I am
starting to grow fond of this idea now that I see the code (tested
with success on Tegra K1 btw).

Here are a few general questions:

* This series creates one device per GPIO chip. I suppose that a
future ioctl will allow the user to change the state of a set of GPIOs
in one single syscall (mirroring gpiod_set_array_value()). In the
world of cute embedded nonsense, it is unfortunately not rare to have
related GPIOs (that we will want to change relatively at the same
time) on different chips. Such scenarios would require several ioctls
to update the set of GPIOs. Are we cool with this, or would we prefer
one global GPIO device that manages all chips and returns user-space
GPIO descriptors that mirror the kernel API more closely?

* For all the justified hate that sysfs got, it had the advantage of
being directly accessible using only the shell. If we replace it with
a character device interface, I think we will need a "libgpio" (that
provides easy access to C programs) and "gpio-tools" (simple
command-line tools that provide functionality similar to the sysfs
interface). Are we ready to start and maintain these? Granted, it
should not be too much work, but it's still an extra.

How these will be shaped will of course depend on how individual GPIOs
are represented in user-space. Security will be a concern, so let's
make sure we don't return global integer descriptors for these. ;)

Re: [PATCH 0/6] GPIO character device skeleton

[Alexandre Courbot]

On Sun, Oct 25, 2015 at 3:42 AM, Markus Pargmann <mpa@pengutronix.de> wrote:
> On Thu, Oct 22, 2015 at 10:32:24AM +0200, Linus Walleij wrote:
>> OK so this is it, I had no patience waiting for users to come up
>> with this new ABI, and the requests for a way for userspace to
>> use GPIOs properly is coming up again and again. So I created the
>> basics for it, so we can then build on top of this to get things
>> right. I want to get these very first things right before we go
>> wild with setting/getting pin values etc.
>>
>> We add ONE ioctl() to get information on the gpiochip. Now we can
>> do this (example from ux500):
>>
>> root@Ux500:/ lsgpio
>> GPIO chip: a03fe000.gpio, 32 GPIO lines
>> GPIO chip: 8011e080.gpio, 32 GPIO lines
>> GPIO chip: 8011e000.gpio, 32 GPIO lines
>> GPIO chip: 8000e180.gpio, 32 GPIO lines
>> GPIO chip: 8000e100.gpio, 32 GPIO lines
>> GPIO chip: 8000e080.gpio, 32 GPIO lines
>> GPIO chip: 8000e000.gpio, 32 GPIO lines
>> GPIO chip: 8012e080.gpio, 32 GPIO lines
>> GPIO chip: 8012e000.gpio, 32 GPIO lines
>> GPIO chip: abx500-gpio, 42 GPIO lines
>> GPIO chip: tc3589x, 20 GPIO lines
>
> What happens if we have two I2C gpio expanders with the same I2C
> addresses connected to different I2C busses? If I see this correctly
> they would both show up with the same name. Is there an easy and
> race-free way to see which GPIO chip is connected to which I2C bus?

I suppose the bus path could be part of the GPIO chip name to avoid
this ambiguity, something like: 7000c000.i2c/0-001c.gpio

Re: [PATCH 0/6] GPIO character device skeleton

[Linus Walleij]

On Sat, Oct 24, 2015 at 7:53 PM, Markus Pargmann <mpa@pengutronix.de> wrote:

>> How to identify and manipulate individual GPIO lines from this
>> ABI is a *LATER* *QUESTION*, this is the bare essentials for
>> getting there: basic operations on the gpiochip device level.
>
> Does this include identifying GPIO lines independent of gpiochips? I
> think that is a use-case as well.

My view is that we will NOT support any such GPIO lines.

The new API requires that everyone use gpiolib properly for all their
GPIOs if they want to use them from userspace.

We have cases with gpiochips with a single GPIO line, so I don't
see why we can't require that.

Yours,
Linus Walleij

Re: [PATCH 0/6] GPIO character device skeleton

[Linus Walleij]

On Sat, Oct 24, 2015 at 8:42 PM, Markus Pargmann <mpa@pengutronix.de> wrote:

> What happens if we have two I2C gpio expanders with the same I2C
> addresses connected to different I2C busses? If I see this correctly
> they would both show up with the same name. Is there an easy and
> race-free way to see which GPIO chip is connected to which I2C bus?

My spontaneous question is: how does ethernet interfaces or whatever
handle this nowadays to get unique device names?

Currently we are probe-order dependent. It would be good to fix
the plug-order business from day 1 but how?

Yours,
Linus Walleij

Re: [PATCH 0/6] GPIO character device skeleton

[Linus Walleij]

On Fri, Oct 30, 2015 at 2:55 AM, Alexandre Courbot <gnurou@gmail.com> wrote:

> I see where we are going with the general idea and it would be nice to
> finally get things right. Actually despite my initial reserves I am
> starting to grow fond of this idea now that I see the code (tested
> with success on Tegra K1 btw).

Thanks, give me your lsgpio :)

> * This series creates one device per GPIO chip. I suppose that a
> future ioctl will allow the user to change the state of a set of GPIOs
> in one single syscall (mirroring gpiod_set_array_value()).

Yeah that is mentioned somewhere even, I think.

> In the
> world of cute embedded nonsense, it is unfortunately not rare to have
> related GPIOs (that we will want to change relatively at the same
> time) on different chips. Such scenarios would require several ioctls
> to update the set of GPIOs. Are we cool with this, or would we prefer
> one global GPIO device that manages all chips and returns user-space
> GPIO descriptors that mirror the kernel API more closely?

Since GPIO chips need to be hot-pluggable (example: USB
expanders) I don't see this happening.

These systems should have not divided their stuff into different
devices/gpiochips in that case, and instead insisted to have one
device with many "banks".

Overall the assumptions to the kernel-intenal multiple-line API
assume any lines switched at the same time need to be in the
same "unsigned long" anyway. Typically a u32.

> * For all the justified hate that sysfs got, it had the advantage of
> being directly accessible using only the shell. If we replace it with
> a character device interface, I think we will need a "libgpio" (that
> provides easy access to C programs) and "gpio-tools" (simple
> command-line tools that provide functionality similar to the sysfs
> interface). Are we ready to start and maintain these? Granted, it
> should not be too much work, but it's still an extra.

I think the raw ioctl()s will be simple enough and intend to use
the tools/gpio/* as proper examples. If we need
tools/gpio/swiss-army-knife.c we will add it too.

ALSA and fbdev and MESA and libusb spawned huge userspace
libs, but I hope this ioctl() ABI will be simple enough that apps
can use it directly without a lib. But what do I know.

> How these will be shaped will of course depend on how individual GPIOs
> are represented in user-space. Security will be a concern, so let's
> make sure we don't return global integer descriptors for these. ;)

First-come-first-gets-to-use-it seems to be how chardevs etc
are handled. I imagine there will be ONE program using the gpiochip,
else userspace have to come up with an arbitration daemon too.

Yours,
Linus Walleij

Re: [PATCH 0/6] GPIO character device skeleton

[Linus Walleij]

On Fri, Oct 30, 2015 at 2:55 AM, Alexandre Courbot <gnurou@gmail.com> wrote:
> On Sun, Oct 25, 2015 at 3:42 AM, Markus Pargmann <mpa@pengutronix.de> wrote:

>> What happens if we have two I2C gpio expanders with the same I2C
>> addresses connected to different I2C busses? If I see this correctly
>> they would both show up with the same name. Is there an easy and
>> race-free way to see which GPIO chip is connected to which I2C bus?
>
> I suppose the bus path could be part of the GPIO chip name to avoid
> this ambiguity, something like: 7000c000.i2c/0-001c.gpio

For DT that is the simple solution.

Right now it used gpiochip->label if that is set, else the name of
the gpiochip device like gpiochip0, gpiochip1 etc.

Yours,
Linus Walleij

Re: [PATCH 0/6] GPIO character device skeleton

[Arnd Bergmann]

On Friday 30 October 2015 15:43:18 Linus Walleij wrote:
> On Sat, Oct 24, 2015 at 8:42 PM, Markus Pargmann <mpa@pengutronix.de> wrote:
> 
> > What happens if we have two I2C gpio expanders with the same I2C
> > addresses connected to different I2C busses? If I see this correctly
> > they would both show up with the same name. Is there an easy and
> > race-free way to see which GPIO chip is connected to which I2C bus?
> 
> My spontaneous question is: how does ethernet interfaces or whatever
> handle this nowadays to get unique device names?
> 
> Currently we are probe-order dependent. It would be good to fix
> the plug-order business from day 1 but how?

ethernet devices are in their own namespace but can get renamed according
to distro-specific policy. A common method is to keep a record of all
mac addresses that were seen in a file in /etc (e.g.
/etc/udev/rules.d/70-persistent-net.rules) and rename them to whatever
they were previously known as when they show up with a different name.

For character devices, udev has a complex set of (user-overridable) rules
to derive one or more stable name(s) from the sysfs attributes.

	Arnd

Re: [PATCH 0/6] GPIO character device skeleton

[Mark Brown]

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

On Fri, Oct 30, 2015 at 08:47:14PM +0100, Linus Walleij wrote:
> On Fri, Oct 30, 2015 at 2:55 AM, Alexandre Courbot <gnurou@gmail.com> wrote:

> > * For all the justified hate that sysfs got, it had the advantage of
> > being directly accessible using only the shell. If we replace it with
> > a character device interface, I think we will need a "libgpio" (that
> > provides easy access to C programs) and "gpio-tools" (simple
> > command-line tools that provide functionality similar to the sysfs
> > interface). Are we ready to start and maintain these? Granted, it
> > should not be too much work, but it's still an extra.

> I think the raw ioctl()s will be simple enough and intend to use
> the tools/gpio/* as proper examples. If we need
> tools/gpio/swiss-army-knife.c we will add it too.

> ALSA and fbdev and MESA and libusb spawned huge userspace
> libs, but I hope this ioctl() ABI will be simple enough that apps
> can use it directly without a lib. But what do I know.

That only works for very C like languages, for other languages (things
like scripting languages in particular) you're going to need a library
for the language to bind to in order to be usable.

> > How these will be shaped will of course depend on how individual GPIOs
> > are represented in user-space. Security will be a concern, so let's
> > make sure we don't return global integer descriptors for these. ;)

> First-come-first-gets-to-use-it seems to be how chardevs etc
> are handled. I imagine there will be ONE program using the gpiochip,
> else userspace have to come up with an arbitration daemon too.

That's another thing pointing to a library then...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 0/6] GPIO character device skeleton

[Linus Walleij]

On Fri, Oct 30, 2015 at 11:54 PM, Arnd Bergmann <arnd@arndb.de> wrote:

> For character devices, udev has a complex set of (user-overridable) rules
> to derive one or more stable name(s) from the sysfs attributes.

OK I'll go read these I think.

Doesn't help much with Busybox' mdev though, which I recently discussed
with Greg and turns out to need a rewrite :/

Yours,
Linus Walleij

Re: [PATCH 0/6] GPIO character device skeleton

[Johan Hovold]

On Fri, Oct 30, 2015 at 03:40:50PM +0100, Linus Walleij wrote:
> On Sat, Oct 24, 2015 at 7:53 PM, Markus Pargmann <mpa@pengutronix.de> wrote:
> 
> >> How to identify and manipulate individual GPIO lines from this
> >> ABI is a *LATER* *QUESTION*, this is the bare essentials for
> >> getting there: basic operations on the gpiochip device level.
> >
> > Does this include identifying GPIO lines independent of gpiochips? I
> > think that is a use-case as well.
> 
> My view is that we will NOT support any such GPIO lines.
> 
> The new API requires that everyone use gpiolib properly for all their
> GPIOs if they want to use them from userspace.
> 
> We have cases with gpiochips with a single GPIO line, so I don't
> see why we can't require that.

I think Markus question was how to *identify* lines independent of
gpiochips. That is given a line name how do we find the corresponding
chip and line (number)?

I know this series does not add such mechanisms, but we need to discuss
this from the start. Do we want user space to iterate over all gpio
chips to search for a line name? Note that this could result in more
than one gpio (chip + offset) being returned (consider hot-pluggable
devices or just dtsi).

Johan

Re: [PATCH 0/6] GPIO character device skeleton

[Johan Hovold]

On Fri, Oct 30, 2015 at 08:48:44PM +0100, Linus Walleij wrote:
> On Fri, Oct 30, 2015 at 2:55 AM, Alexandre Courbot <gnurou@gmail.com> wrote:
> > On Sun, Oct 25, 2015 at 3:42 AM, Markus Pargmann <mpa@pengutronix.de> wrote:
> 
> >> What happens if we have two I2C gpio expanders with the same I2C
> >> addresses connected to different I2C busses? If I see this correctly
> >> they would both show up with the same name. Is there an easy and
> >> race-free way to see which GPIO chip is connected to which I2C bus?
> >
> > I suppose the bus path could be part of the GPIO chip name to avoid
> > this ambiguity, something like: 7000c000.i2c/0-001c.gpio
> 
> For DT that is the simple solution.

Not all devices are platform devices, and the bus path can become quite
long, for example for usb to uniquely identify the gpio controller this
could be:

	platform/68000000.ocp/48064000.usbhshost/48064800.ehci/usb1/1-2/1-2.3/1-2.3:1.0/gpiochip7

> Right now it used gpiochip->label if that is set, else the name of
> the gpiochip device like gpiochip0, gpiochip1 etc.

Perhaps better to just stick to the bus unique names (e.g. gpopchip7),
and possibly export the label as an additional attribute.

This is the lsgpio output I get on an omap system by the way:

GPIO chip: gpio, 32 GPIO lines
GPIO chip: gpio, 32 GPIO lines
GPIO chip: gpio, 32 GPIO lines
GPIO chip: gpio, 32 GPIO lines
GPIO chip: gpio, 32 GPIO lines
GPIO chip: gpio, 32 GPIO lines
GPIO chip: twl4030, 20 GPIO lines
GPIO chip: pl2303, 2 GPIO lines

Johan

Re: [PATCH 0/6] GPIO character device skeleton

[Johan Hovold]

On Sun, Nov 01, 2015 at 10:37:28AM +0100, Linus Walleij wrote:
> On Fri, Oct 30, 2015 at 11:54 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> 
> > For character devices, udev has a complex set of (user-overridable) rules
> > to derive one or more stable name(s) from the sysfs attributes.
> 
> OK I'll go read these I think.
> 
> Doesn't help much with Busybox' mdev though, which I recently discussed
> with Greg and turns out to need a rewrite :/

Just stick to the bus unique names (e.g. gpiochip7) rather than try to
come up with udev rules to rename these. Look at the USB path I just
posted for an example of what you'd need to parse and encode.

Johan

Re: [PATCH 1/6] gpio: make the gpiochip a real device

[Johan Hovold]

On Thu, Oct 22, 2015 at 10:32:25AM +0200, Linus Walleij wrote:
> GPIO chips have been around for years, but were never real devices,
> instead they were piggy-backing on a parent device (such as a
> platform_device or amba_device) but this was always optional.
> GPIO chips could also exist without any device at all, with its
> struct device *dev pointer being set to null.
> 
> When sysfs was in use, a mock device would be created, with the
> optional parent assigned, or just floating orphaned with NULL
> as parent.
> 
> For a proper userspace ABI we need gpiochips to *always have a
> populated struct device, so add this in the gpio_chip struct.
> The name "dev" is unfortunately already take so we use "device"
> to name it.
> 
> If sysfs is active, it will use this device as parent, and the
> former parent device "dev" will be set as parent of the new
> "device" struct member.
> 
> From this point on, gpiochips are devices.
> 
> Cc: Johan Hovold <johan@kernel.org>
> Cc: Michael Welling <mwelling@ieee.org>
> Cc: Markus Pargmann <mpa@pengutronix.de>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/gpio/gpiolib-sysfs.c |  2 +-
>  drivers/gpio/gpiolib.c       | 44 +++++++++++++++++++++++++++++++++++++++-----
>  include/linux/gpio/driver.h  |  9 +++++++--
>  3 files changed, 47 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
> index b57ed8e55ab5..7e5bc5736e47 100644
> --- a/drivers/gpio/gpiolib-sysfs.c
> +++ b/drivers/gpio/gpiolib-sysfs.c
> @@ -605,7 +605,7 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
>  	if (chip->names && chip->names[offset])
>  		ioname = chip->names[offset];
>  
> -	dev = device_create_with_groups(&gpio_class, chip->dev,
> +	dev = device_create_with_groups(&gpio_class, &chip->device,
>  					MKDEV(0, 0), data, gpio_groups,
>  					ioname ? ioname : "gpio%u",
>  					desc_to_gpio(desc));
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 6798355c61c6..0ab4f75b7f8e 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -16,6 +16,7 @@
>  #include <linux/gpio/driver.h>
>  #include <linux/gpio/machine.h>
>  #include <linux/pinctrl/consumer.h>
> +#include <linux/idr.h>
>  
>  #include "gpiolib.h"
>  
> @@ -42,6 +43,9 @@
>  #define	extra_checks	0
>  #endif
>  
> +/* Device and char device-related information */
> +static DEFINE_IDA(gpio_ida);
> +
>  /* gpio_lock prevents conflicts during gpio_desc[] table updates.
>   * While any GPIO is requested, its gpio_chip is not removable;
>   * each GPIO's "requested" flag serves as a lock and refcount.
> @@ -52,7 +56,6 @@ static DEFINE_MUTEX(gpio_lookup_lock);
>  static LIST_HEAD(gpio_lookup_list);
>  LIST_HEAD(gpio_chips);
>  
> -
>  static void gpiochip_free_hogs(struct gpio_chip *chip);
>  static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip);
>  
> @@ -300,7 +303,7 @@ int gpiochip_add(struct gpio_chip *chip)
>  {
>  	unsigned long	flags;
>  	int		status = 0;
> -	unsigned	id;
> +	unsigned	i;
>  	int		base = chip->base;
>  	struct gpio_desc *descs;
>  
> @@ -308,6 +311,28 @@ int gpiochip_add(struct gpio_chip *chip)
>  	if (!descs)
>  		return -ENOMEM;
>  
> +	/*
> +	 * The "dev" member of gpiochip is the parent, and the actual
> +	 * device is named "device" for historical reasons.
> +	 *
> +	 * We memset the struct to zero to avoid reentrance issues.
> +	 */
> +	memset(&chip->device, 0, sizeof(chip->device));

This is an indication of a larger problem.

First of all, you must never register the same device structure twice.

And the larger problem is: With the current interface where a struct
gpio_chip is passed and registered, how would you prevent the device
from going away while in use?

You grab a reference to the chip->device when opening the node (in a
later patch), but it is not used to manage the life time of struct
gpio_chip.

> +	if (chip->dev) {
> +		chip->device.parent = chip->dev;
> +		chip->device.of_node = chip->dev->of_node;
> +	} else {
> +#ifdef CONFIG_OF_GPIO
> +	/* If the gpiochip has an assigned OF node this takes precedence */
> +	if (chip->of_node)
> +		chip->device.of_node = chip->of_node;
> +#endif
> +	}
> +	chip->id = ida_simple_get(&gpio_ida, 0, 0, GFP_KERNEL);

Missing error handling.

> +	dev_set_name(&chip->device, "gpiochip%d", chip->id);
> +	device_initialize(&chip->device);
> +	dev_set_drvdata(&chip->device, chip);
> +
>  	spin_lock_irqsave(&gpio_lock, flags);
>  
>  	if (base < 0) {

Johan

Re: [PATCH 1/6] gpio: make the gpiochip a real device

[Mark Brown]

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

On Mon, Nov 02, 2015 at 11:31:16AM +0100, Johan Hovold wrote:
> On Thu, Oct 22, 2015 at 10:32:25AM +0200, Linus Walleij wrote:

> > +	/*
> > +	 * The "dev" member of gpiochip is the parent, and the actual
> > +	 * device is named "device" for historical reasons.
> > +	 *
> > +	 * We memset the struct to zero to avoid reentrance issues.
> > +	 */
> > +	memset(&chip->device, 0, sizeof(chip->device));

> This is an indication of a larger problem.

> First of all, you must never register the same device structure twice.

Well, you can unregister and reregister (and it is reasonable practice
to make sure that the struct isn't full of noise) - we usually allocate
things out of kzalloc().

> And the larger problem is: With the current interface where a struct
> gpio_chip is passed and registered, how would you prevent the device
> from going away while in use?

Hrm, indeed.  Why aren't there complaints about a missing release
function there?

> You grab a reference to the chip->device when opening the node (in a
> later patch), but it is not used to manage the life time of struct
> gpio_chip.

That's a slightly separate thing and even with a different
implementation of the file we still have to assume that the driver core
might hold a reference to the device for longer (for example as a result
of sysfs interactions).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/6] gpio: make the gpiochip a real device

[Johan Hovold]

On Mon, Nov 02, 2015 at 12:25:14PM +0000, Mark Brown wrote:
> On Mon, Nov 02, 2015 at 11:31:16AM +0100, Johan Hovold wrote:
> > On Thu, Oct 22, 2015 at 10:32:25AM +0200, Linus Walleij wrote:
> 
> > > +	/*
> > > +	 * The "dev" member of gpiochip is the parent, and the actual
> > > +	 * device is named "device" for historical reasons.
> > > +	 *
> > > +	 * We memset the struct to zero to avoid reentrance issues.
> > > +	 */
> > > +	memset(&chip->device, 0, sizeof(chip->device));
> 
> > This is an indication of a larger problem.
> 
> > First of all, you must never register the same device structure twice.
> 
> Well, you can unregister and reregister (and it is reasonable practice
> to make sure that the struct isn't full of noise) - we usually allocate
> things out of kzalloc().

Actually, no. It's an explicitly forbidden practise to reregister the
same struct device.

> > And the larger problem is: With the current interface where a struct
> > gpio_chip is passed and registered, how would you prevent the device
> > from going away while in use?
> 
> Hrm, indeed.  Why aren't there complaints about a missing release
> function there?
> 
> > You grab a reference to the chip->device when opening the node (in a
> > later patch), but it is not used to manage the life time of struct
> > gpio_chip.
> 
> That's a slightly separate thing and even with a different
> implementation of the file we still have to assume that the driver core
> might hold a reference to the device for longer (for example as a result
> of sysfs interactions).

Yes, there may be other references, but sysfs should be ok due to the
kernfs active protection.

Johan

Re: [PATCH 1/6] gpio: make the gpiochip a real device

[Mark Brown]

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

On Mon, Nov 02, 2015 at 01:43:23PM +0100, Johan Hovold wrote:
> On Mon, Nov 02, 2015 at 12:25:14PM +0000, Mark Brown wrote:

> > > First of all, you must never register the same device structure twice.

> > Well, you can unregister and reregister (and it is reasonable practice
> > to make sure that the struct isn't full of noise) - we usually allocate
> > things out of kzalloc().

> Actually, no. It's an explicitly forbidden practise to reregister the
> same struct device.

A memset() should be enough, if not then we have problems with any
dynamically allocated struct device.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/6] gpio: make the gpiochip a real device

[Johan Hovold]

On Mon, Nov 02, 2015 at 12:47:37PM +0000, Mark Brown wrote:
> On Mon, Nov 02, 2015 at 01:43:23PM +0100, Johan Hovold wrote:
> > On Mon, Nov 02, 2015 at 12:25:14PM +0000, Mark Brown wrote:
> 
> > > > First of all, you must never register the same device structure twice.
> 
> > > Well, you can unregister and reregister (and it is reasonable practice
> > > to make sure that the struct isn't full of noise) - we usually allocate
> > > things out of kzalloc().
> 
> > Actually, no. It's an explicitly forbidden practise to reregister the
> > same struct device.
> 
> A memset() should be enough, if not then we have problems with any
> dynamically allocated struct device.

And how would you know that it is safe to memset that struct device?
There can still be references to it. And driver core explicitly forbids
this (see device_add() for example).

Dynamically allocated struct device are not the problem as then you're
not *reusing* the same device structure.

Johan

Re: [PATCH 1/6] gpio: make the gpiochip a real device

[Mark Brown]

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

On Mon, Nov 02, 2015 at 01:53:47PM +0100, Johan Hovold wrote:
> On Mon, Nov 02, 2015 at 12:47:37PM +0000, Mark Brown wrote:

> > A memset() should be enough, if not then we have problems with any
> > dynamically allocated struct device.

> And how would you know that it is safe to memset that struct device?
> There can still be references to it. And driver core explicitly forbids
> this (see device_add() for example).

My point here is that the memset() of something that was passed in
externally isn't really a problem, it's the suspicious lack of
integration with a release function (hence my question about why the
driver core isn't complaining when the device gets registered).  The
comment is more of a concern than the memset() itself.

> Dynamically allocated struct device are not the problem as then you're
> not *reusing* the same device structure.

You may end up doing exactly that depending on what you get back from
the allocator of course.  The point is to make sure that that the driver
model called release(), dynamically allocating and freeing in the
release function is the easiest way to do this but it's not magic.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/6] gpio: make the gpiochip a real device

[Johan Hovold]

On Mon, Nov 02, 2015 at 01:06:33PM +0000, Mark Brown wrote:
> On Mon, Nov 02, 2015 at 01:53:47PM +0100, Johan Hovold wrote:
> > On Mon, Nov 02, 2015 at 12:47:37PM +0000, Mark Brown wrote:
> 
> > > A memset() should be enough, if not then we have problems with any
> > > dynamically allocated struct device.
> 
> > And how would you know that it is safe to memset that struct device?
> > There can still be references to it. And driver core explicitly forbids
> > this (see device_add() for example).
> 
> My point here is that the memset() of something that was passed in
> externally isn't really a problem, it's the suspicious lack of
> integration with a release function (hence my question about why the
> driver core isn't complaining when the device gets registered).  The
> comment is more of a concern than the memset() itself.

Precisely, the memset itself is not the problem, but rather why was done
in the first place:

	*  We memset the struct to zero to avoid reentrance issues.
	*/

And to that point, I mentioned that it is illegal to register the same
struct device twice (and that this was indicative of a larger problem).

> > Dynamically allocated struct device are not the problem as then you're
> > not *reusing* the same device structure.
> 
> You may end up doing exactly that depending on what you get back from
> the allocator of course.

But then the memory has already been released. You're not deregistering
and reregistering the same device as in your example.

> The point is to make sure that that the driver
> model called release(), dynamically allocating and freeing in the
> release function is the easiest way to do this but it's not magic.

Agreed.

Johan

Re: [PATCH 1/6] gpio: make the gpiochip a real device

[Mark Brown]

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

On Mon, Nov 02, 2015 at 02:14:56PM +0100, Johan Hovold wrote:
> On Mon, Nov 02, 2015 at 01:06:33PM +0000, Mark Brown wrote:

> > > Dynamically allocated struct device are not the problem as then you're
> > > not *reusing* the same device structure.

> > You may end up doing exactly that depending on what you get back from
> > the allocator of course.

> But then the memory has already been released. You're not deregistering
> and reregistering the same device as in your example.

Depends on your definition of "same" :)  You might get the same address
back which has been known to confuse unhelpfully designed things.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/6] gpio: make the gpiochip a real device

[Johan Hovold]

On Mon, Nov 02, 2015 at 01:17:01PM +0000, Mark Brown wrote:
> On Mon, Nov 02, 2015 at 02:14:56PM +0100, Johan Hovold wrote:
> > On Mon, Nov 02, 2015 at 01:06:33PM +0000, Mark Brown wrote:
> 
> > > > Dynamically allocated struct device are not the problem as then you're
> > > > not *reusing* the same device structure.
> 
> > > You may end up doing exactly that depending on what you get back from
> > > the allocator of course.
> 
> > But then the memory has already been released. You're not deregistering
> > and reregistering the same device as in your example.
> 
> Depends on your definition of "same" :)  You might get the same address
> back which has been known to confuse unhelpfully designed things.

Yeah, I got your point, but again my claim was that 

	device_unregister(dev);
	memset(dev);
	device_initialise(dev);
	...
	device_add(dev);

is not legal (and the current patch does not prevent this).

Johan

Re: [PATCH 0/6] GPIO character device skeleton

[Markus Pargmann]

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

On Monday 02 November 2015 11:13:47 Johan Hovold wrote:
> On Fri, Oct 30, 2015 at 08:48:44PM +0100, Linus Walleij wrote:
> > On Fri, Oct 30, 2015 at 2:55 AM, Alexandre Courbot <gnurou@gmail.com> wrote:
> > > On Sun, Oct 25, 2015 at 3:42 AM, Markus Pargmann <mpa@pengutronix.de> wrote:
> > 
> > >> What happens if we have two I2C gpio expanders with the same I2C
> > >> addresses connected to different I2C busses? If I see this correctly
> > >> they would both show up with the same name. Is there an easy and
> > >> race-free way to see which GPIO chip is connected to which I2C bus?
> > >
> > > I suppose the bus path could be part of the GPIO chip name to avoid
> > > this ambiguity, something like: 7000c000.i2c/0-001c.gpio
> > 
> > For DT that is the simple solution.
> 
> Not all devices are platform devices, and the bus path can become quite
> long, for example for usb to uniquely identify the gpio controller this
> could be:
> 
> 	platform/68000000.ocp/48064000.usbhshost/48064800.ehci/usb1/1-2/1-2.3/1-2.3:1.0/gpiochip7
> 
> > Right now it used gpiochip->label if that is set, else the name of
> > the gpiochip device like gpiochip0, gpiochip1 etc.
> 
> Perhaps better to just stick to the bus unique names (e.g. gpopchip7),
> and possibly export the label as an additional attribute.

I think this wouldn't be enough. We would still have trouble identifying the
gpiochips, right?

As an idea: We could use the complete path to create some sort of unique id for
the device (perhaps hash or something different). This id can be exported as
device attribute and would allow udev to create some links as known from
/dev/disk/by-id for example. This would make identifying a single chip quite
easy for any userspace application and we would avoid having this really long
path somewhere.

Best Regards,

Markus

> 
> This is the lsgpio output I get on an omap system by the way:
> 
> GPIO chip: gpio, 32 GPIO lines
> GPIO chip: gpio, 32 GPIO lines
> GPIO chip: gpio, 32 GPIO lines
> GPIO chip: gpio, 32 GPIO lines
> GPIO chip: gpio, 32 GPIO lines
> GPIO chip: gpio, 32 GPIO lines
> GPIO chip: twl4030, 20 GPIO lines
> GPIO chip: pl2303, 2 GPIO lines
> 
> Johan
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 0/6] GPIO character device skeleton

[Markus Pargmann]

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

On Sunday 01 November 2015 11:41:44 Mark Brown wrote:
> On Fri, Oct 30, 2015 at 08:47:14PM +0100, Linus Walleij wrote:
> > On Fri, Oct 30, 2015 at 2:55 AM, Alexandre Courbot <gnurou@gmail.com> wrote:
> 
> > > * For all the justified hate that sysfs got, it had the advantage of
> > > being directly accessible using only the shell. If we replace it with
> > > a character device interface, I think we will need a "libgpio" (that
> > > provides easy access to C programs) and "gpio-tools" (simple
> > > command-line tools that provide functionality similar to the sysfs
> > > interface). Are we ready to start and maintain these? Granted, it
> > > should not be too much work, but it's still an extra.
> 
> > I think the raw ioctl()s will be simple enough and intend to use
> > the tools/gpio/* as proper examples. If we need
> > tools/gpio/swiss-army-knife.c we will add it too.
> 
> > ALSA and fbdev and MESA and libusb spawned huge userspace
> > libs, but I hope this ioctl() ABI will be simple enough that apps
> > can use it directly without a lib. But what do I know.
> 
> That only works for very C like languages, for other languages (things
> like scripting languages in particular) you're going to need a library
> for the language to bind to in order to be usable.

I think we need real gpio userspace tools for such a chardev interface and not
only a library. On the one hand there are all the kernel/userspace developers
that just want to test stuff. On the other hand there are really a lot of
people that are using small embedded devices (e.g. beaglebone, raspberry pi,
...) for really simple applications like using GPIOs to control something. I am
sure many of them are just using some really simple bash code to reach their
goals.

Best Regards,

Markus

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 0/6] GPIO character device skeleton

[Mark Brown]

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

On Tue, Nov 03, 2015 at 08:39:13AM +0100, Markus Pargmann wrote:

> I think we need real gpio userspace tools for such a chardev interface and not
> only a library. On the one hand there are all the kernel/userspace developers
> that just want to test stuff. On the other hand there are really a lot of
> people that are using small embedded devices (e.g. beaglebone, raspberry pi,
> ...) for really simple applications like using GPIOs to control something. I am
> sure many of them are just using some really simple bash code to reach their
> goals.

There's already some work going on on producing tooling for the maker
community which is currently working with the existing interfaces,
attempting to abstract them for users to handle things like base board
differences - I believe that's part of the motivation behind this work.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 0/6] GPIO character device skeleton

[Amit Kucheria]

On Tue, Nov 3, 2015 at 2:20 PM, Mark Brown <broonie@kernel.org> wrote:
> On Tue, Nov 03, 2015 at 08:39:13AM +0100, Markus Pargmann wrote:
>
>> I think we need real gpio userspace tools for such a chardev interface and not
>> only a library. On the one hand there are all the kernel/userspace developers
>> that just want to test stuff. On the other hand there are really a lot of
>> people that are using small embedded devices (e.g. beaglebone, raspberry pi,
>> ...) for really simple applications like using GPIOs to control something. I am
>> sure many of them are just using some really simple bash code to reach their
>> goals.
>
> There's already some work going on on producing tooling for the maker
> community which is currently working with the existing interfaces,
> attempting to abstract them for users to handle things like base board
> differences - I believe that's part of the motivation behind this work.

Here is an example[1] of the kind of workarounds needed to get a
consistent userspace programming interface by mapping SoC-specific
gpios to fixed gpio names on the expansion connector of a 96board.

Ideally, the kernel would allow us to do this through some DT properties.

Regards,
Amit

[1] https://github.com/96boards/96BoardsGPIO/blob/a17e58867ecfeba36d163580bc1839ffdbca8cca/lib/__gpio.h

Re: [PATCH 0/6] GPIO character device skeleton

[Johan Hovold]

On Tue, Nov 03, 2015 at 08:23:24AM +0100, Markus Pargmann wrote:
> On Monday 02 November 2015 11:13:47 Johan Hovold wrote:
> > On Fri, Oct 30, 2015 at 08:48:44PM +0100, Linus Walleij wrote:
> > > On Fri, Oct 30, 2015 at 2:55 AM, Alexandre Courbot <gnurou@gmail.com> wrote:
> > > > On Sun, Oct 25, 2015 at 3:42 AM, Markus Pargmann <mpa@pengutronix.de> wrote:
> > > 
> > > >> What happens if we have two I2C gpio expanders with the same I2C
> > > >> addresses connected to different I2C busses? If I see this correctly
> > > >> they would both show up with the same name. Is there an easy and
> > > >> race-free way to see which GPIO chip is connected to which I2C bus?
> > > >
> > > > I suppose the bus path could be part of the GPIO chip name to avoid
> > > > this ambiguity, something like: 7000c000.i2c/0-001c.gpio
> > > 
> > > For DT that is the simple solution.
> > 
> > Not all devices are platform devices, and the bus path can become quite
> > long, for example for usb to uniquely identify the gpio controller this
> > could be:
> > 
> > 	platform/68000000.ocp/48064000.usbhshost/48064800.ehci/usb1/1-2/1-2.3/1-2.3:1.0/gpiochip7
> > 
> > > Right now it used gpiochip->label if that is set, else the name of
> > > the gpiochip device like gpiochip0, gpiochip1 etc.
> > 
> > Perhaps better to just stick to the bus unique names (e.g. gpopchip7),
> > and possibly export the label as an additional attribute.
> 
> I think this wouldn't be enough. We would still have trouble identifying the
> gpiochips, right?

That information is already available through sysfs so there's no need
to try and re-encode it in device links etc.

> As an idea: We could use the complete path to create some sort of unique id for
> the device (perhaps hash or something different). This id can be exported as
> device attribute and would allow udev to create some links as known from
> /dev/disk/by-id for example. This would make identifying a single chip quite
> easy for any userspace application and we would avoid having this really long
> path somewhere.

The unique ids are already there in sysfs, for example:

$ for x in /sys/bus/gpio/devices/gpiochip*; do readlink $x; done
../../../devices/platform/68000000.ocp/48310000.gpio/gpiochip0
../../../devices/platform/68000000.ocp/49050000.gpio/gpiochip1
../../../devices/platform/68000000.ocp/49052000.gpio/gpiochip2
../../../devices/platform/68000000.ocp/49054000.gpio/gpiochip3
../../../devices/platform/68000000.ocp/49056000.gpio/gpiochip4
../../../devices/platform/68000000.ocp/49058000.gpio/gpiochip5
../../../devices/platform/68000000.ocp/48070000.i2c/i2c-0/0-0048/twl4030-gpio/gpiochip6
../../../devices/platform/68000000.ocp/48064000.usbhshost/48064800.ehci/usb1/1-2/1-2.3/1-2.3:1.0/gpiochip7

And libudev can be used to lookup devices based on (parent) attributes
(such as USB VID/PID, serial numbers, etc).

We could also export further attributes if that would help (e.g.
gpio-chip labels).

Johan

Re: [PATCH 0/6] GPIO character device skeleton

[Linus Walleij]

On Tue, Nov 3, 2015 at 8:23 AM, Markus Pargmann <mpa@pengutronix.de> wrote:

> As an idea: We could use the complete path to create some sort of unique id for
> the device (perhaps hash or something different). This id can be exported as
> device attribute and would allow udev to create some links as known from
> /dev/disk/by-id for example. This would make identifying a single chip quite
> easy for any userspace application and we would avoid having this really long
> path somewhere.

I like this idea.

I'll try to figure out some way to hash the bus name + chip name + label
or something down to a unique ID per-gpiochip.

Yours,
Linus Walleij

Re: [PATCH 0/6] GPIO character device skeleton

[Linus Walleij]

On Tue, Nov 3, 2015 at 9:50 AM, Mark Brown <broonie@kernel.org> wrote:
> On Tue, Nov 03, 2015 at 08:39:13AM +0100, Markus Pargmann wrote:
>
>> I think we need real gpio userspace tools for such a chardev interface and not
>> only a library. On the one hand there are all the kernel/userspace developers
>> that just want to test stuff. On the other hand there are really a lot of
>> people that are using small embedded devices (e.g. beaglebone, raspberry pi,
>> ...) for really simple applications like using GPIOs to control something. I am
>> sure many of them are just using some really simple bash code to reach their
>> goals.
>
> There's already some work going on on producing tooling for the maker
> community which is currently working with the existing interfaces,
> attempting to abstract them for users to handle things like base board
> differences - I believe that's part of the motivation behind this work.

You are right. It is adding to the long-term maintenance requirements for
the sysfs ABI which we want to ditch.

Yours,
Linus Walleij

Re: [PATCH 0/6] GPIO character device skeleton

[Linus Walleij]

On Tue, Nov 3, 2015 at 1:06 PM, Johan Hovold <johan@kernel.org> wrote:
> [Pargmann]
>> As an idea: We could use the complete path to create some sort of unique id for
>> the device (perhaps hash or something different). This id can be exported as
>> device attribute and would allow udev to create some links as known from
>> /dev/disk/by-id for example. This would make identifying a single chip quite
>> easy for any userspace application and we would avoid having this really long
>> path somewhere.
>
> The unique ids are already there in sysfs, for example:
>
> $ for x in /sys/bus/gpio/devices/gpiochip*; do readlink $x; done
> ../../../devices/platform/68000000.ocp/48310000.gpio/gpiochip0
> ../../../devices/platform/68000000.ocp/49050000.gpio/gpiochip1
> ../../../devices/platform/68000000.ocp/49052000.gpio/gpiochip2
> ../../../devices/platform/68000000.ocp/49054000.gpio/gpiochip3
> ../../../devices/platform/68000000.ocp/49056000.gpio/gpiochip4
> ../../../devices/platform/68000000.ocp/49058000.gpio/gpiochip5
> ../../../devices/platform/68000000.ocp/48070000.i2c/i2c-0/0-0048/twl4030-gpio/gpiochip6
> ../../../devices/platform/68000000.ocp/48064000.usbhshost/48064800.ehci/usb1/1-2/1-2.3/1-2.3:1.0/gpiochip7
>
> And libudev can be used to lookup devices based on (parent) attributes
> (such as USB VID/PID, serial numbers, etc).
>
> We could also export further attributes if that would help (e.g.
> gpio-chip labels).

Yeah sysfs does provide this.

This has the problem that everyone and their dog need to use udev
or something like it. Some library or so that run around in sysfs like
libudev+systemdlib does currently.

And since Android does not use udev, and Busybox does not use udev,
there are quite a few million users there. Or at least two big projects that
need to reimplement the same idea. It could be argued that they should,
since sysfs is indeed an ABI.

In the Busybox mdev case part of the goal is to minimize userspace code
size too, and it does not support the complex rules of udev for example.

It'd be nice if devices could be uniquely identified in the chardev alone
I think, then we don't need to much reliance on external assumptions
and traversing sysfs somehow for more info.

Yours,
Linus Walleij

Re: [PATCH 1/6] gpio: make the gpiochip a real device

[Linus Walleij]

On Sun, Oct 25, 2015 at 8:06 AM, Alexandre Courbot <gnurou@gmail.com> wrote:
> On Thu, Oct 22, 2015 at 5:32 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
>> GPIO chips have been around for years, but were never real devices,
>> instead they were piggy-backing on a parent device (such as a
>> platform_device or amba_device) but this was always optional.
>> GPIO chips could also exist without any device at all, with its
>> struct device *dev pointer being set to null.
>>
>> When sysfs was in use, a mock device would be created, with the
>> optional parent assigned, or just floating orphaned with NULL
>> as parent.
>>
>> For a proper userspace ABI we need gpiochips to *always have a
>> populated struct device, so add this in the gpio_chip struct.
>> The name "dev" is unfortunately already take so we use "device"
>> to name it.
>>
>> If sysfs is active, it will use this device as parent, and the
>> former parent device "dev" will be set as parent of the new
>> "device" struct member.
>
> Why not rename "dev" to "parent" so "dev" becomes what we expect it to
> be? The two members being of the same type, keeping it that way seems
> error-prone to me.

It's because that is set all over the universe. But I'll cook some
separate patch renaming it across the tree I guess... Could
use Cocinelle for it maybe.

Yours,
Linus Walleij

Re: [PATCH 1/6] gpio: make the gpiochip a real device

[Linus Walleij]

On Mon, Nov 2, 2015 at 11:31 AM, Johan Hovold <johan@kernel.org> wrote:
> On Thu, Oct 22, 2015 at 10:32:25AM +0200, Linus Walleij wrote:

>> +      * We memset the struct to zero to avoid reentrance issues.
>> +      */
>> +     memset(&chip->device, 0, sizeof(chip->device));
>
> This is an indication of a larger problem.
>
> First of all, you must never register the same device structure twice.
>
> And the larger problem is: With the current interface where a struct
> gpio_chip is passed and registered, how would you prevent the device
> from going away while in use?

OK I see. What about the design pattern elsewhere to pass a
gpiochip desc and have a new gpiochip_register() create the
gpiochip and delete gpiochip_add()?

That's the usual pattern in subsystems.

> You grab a reference to the chip->device when opening the node (in a
> later patch), but it is not used to manage the life time of struct
> gpio_chip.

OK I think the above approach should solve that?

Yours,
Linus Walleij

Re: [PATCH 1/6] gpio: make the gpiochip a real device

[Linus Walleij]

On Tue, Nov 3, 2015 at 10:24 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Mon, Nov 2, 2015 at 11:31 AM, Johan Hovold <johan@kernel.org> wrote:
>> On Thu, Oct 22, 2015 at 10:32:25AM +0200, Linus Walleij wrote:
>
>>> +      * We memset the struct to zero to avoid reentrance issues.
>>> +      */
>>> +     memset(&chip->device, 0, sizeof(chip->device));
>>
>> This is an indication of a larger problem.
>>
>> First of all, you must never register the same device structure twice.
>>
>> And the larger problem is: With the current interface where a struct
>> gpio_chip is passed and registered, how would you prevent the device
>> from going away while in use?
>
> OK I see. What about the design pattern elsewhere to pass a
> gpiochip desc and have a new gpiochip_register() create the
> gpiochip and delete gpiochip_add()?
>
> That's the usual pattern in subsystems.

Thinking about it maybe it's simplest to just make ->dev a pointer
and kzalloc() it at gpiochip_add().

That should solve this.

Yours,
Linus Walleij

Re: [PATCH 0/6] GPIO character device skeleton

[Michael Welling]

On Tue, Nov 03, 2015 at 06:18:42PM +0100, Linus Walleij wrote:
> On Tue, Nov 3, 2015 at 1:06 PM, Johan Hovold <johan@kernel.org> wrote:
> > [Pargmann]
> >> As an idea: We could use the complete path to create some sort of unique id for
> >> the device (perhaps hash or something different). This id can be exported as
> >> device attribute and would allow udev to create some links as known from
> >> /dev/disk/by-id for example. This would make identifying a single chip quite
> >> easy for any userspace application and we would avoid having this really long
> >> path somewhere.
> >
> > The unique ids are already there in sysfs, for example:
> >
> > $ for x in /sys/bus/gpio/devices/gpiochip*; do readlink $x; done
> > ../../../devices/platform/68000000.ocp/48310000.gpio/gpiochip0
> > ../../../devices/platform/68000000.ocp/49050000.gpio/gpiochip1
> > ../../../devices/platform/68000000.ocp/49052000.gpio/gpiochip2
> > ../../../devices/platform/68000000.ocp/49054000.gpio/gpiochip3
> > ../../../devices/platform/68000000.ocp/49056000.gpio/gpiochip4
> > ../../../devices/platform/68000000.ocp/49058000.gpio/gpiochip5
> > ../../../devices/platform/68000000.ocp/48070000.i2c/i2c-0/0-0048/twl4030-gpio/gpiochip6
> > ../../../devices/platform/68000000.ocp/48064000.usbhshost/48064800.ehci/usb1/1-2/1-2.3/1-2.3:1.0/gpiochip7
> >
> > And libudev can be used to lookup devices based on (parent) attributes
> > (such as USB VID/PID, serial numbers, etc).
> >
> > We could also export further attributes if that would help (e.g.
> > gpio-chip labels).
> 
> Yeah sysfs does provide this.
> 
> This has the problem that everyone and their dog need to use udev
> or something like it. Some library or so that run around in sysfs like
> libudev+systemdlib does currently.
> 
> And since Android does not use udev, and Busybox does not use udev,
> there are quite a few million users there. Or at least two big projects that
> need to reimplement the same idea. It could be argued that they should,
> since sysfs is indeed an ABI.
> 
> In the Busybox mdev case part of the goal is to minimize userspace code
> size too, and it does not support the complex rules of udev for example.
> 
> It'd be nice if devices could be uniquely identified in the chardev alone
> I think, then we don't need to much reliance on external assumptions
> and traversing sysfs somehow for more info.

So I added a few duplicate SPI gpio controllers to see how it looks in userspace.

root@som-imx6q:~# lsgpio
GPIO chip: 209c000.gpio, 32 GPIO lines
GPIO chip: 20a0000.gpio, 32 GPIO lines
GPIO chip: 20a4000.gpio, 32 GPIO lines
GPIO chip: 20a8000.gpio, 32 GPIO lines
GPIO chip: 20ac000.gpio, 32 GPIO lines
GPIO chip: 20b0000.gpio, 32 GPIO lines
GPIO chip: 20b4000.gpio, 32 GPIO lines
GPIO chip: mcp23s08, 8 GPIO lines
GPIO chip: mcp23s08, 8 GPIO lines

As expected the two are indistinguishable using the lsgpio.

Here is how they look in the /sys/bus/gpio directory:
root@som-imx6q:~# for x in /sys/bus/gpio/devices/gpiochip*; do readlink $x; done
../../../devices/soc0/soc/2000000.aips-bus/209c000.gpio/gpiochip0
../../../devices/soc0/soc/2000000.aips-bus/20a0000.gpio/gpiochip1
../../../devices/soc0/soc/2000000.aips-bus/20a4000.gpio/gpiochip2
../../../devices/soc0/soc/2000000.aips-bus/20a8000.gpio/gpiochip3
../../../devices/soc0/soc/2000000.aips-bus/20ac000.gpio/gpiochip4
../../../devices/soc0/soc/2000000.aips-bus/20b0000.gpio/gpiochip5
../../../devices/soc0/soc/2000000.aips-bus/20b4000.gpio/gpiochip6
../../../devices/soc0/soc/2000000.aips-bus/2000000.spba-bus/2008000.ecspi/spi_master/spi0/spi0.0/gpiochip7
../../../devices/soc0/soc/2000000.aips-bus/2000000.spba-bus/200c000.ecspi/spi_master/spi1/spi1.0/gpiochip8

Here is what the debug out looks like:
root@som-imx6q:~# cat /sys/kernel/debug/gpio 
gpiochip0: GPIOs 0-31, parent: platform/209c000.gpio, 209c000.gpio:

gpiochip1: GPIOs 32-63, parent: platform/20a0000.gpio, 20a0000.gpio:
 gpio-33  (                    |PCIe reset          ) out hi    

gpiochip2: GPIOs 64-95, parent: platform/20a4000.gpio, 20a4000.gpio:
 gpio-83  (                    |spi_imx             ) out hi    
 gpio-84  (                    |spi_imx             ) out hi    
 gpio-93  (                    |phy-reset           ) out hi    

gpiochip3: GPIOs 96-127, parent: platform/20a8000.gpio, 20a8000.gpio:

gpiochip4: GPIOs 128-159, parent: platform/20ac000.gpio, 20ac000.gpio:

gpiochip5: GPIOs 160-191, parent: platform/20b0000.gpio, 20b0000.gpio:

gpiochip6: GPIOs 192-223, parent: platform/20b4000.gpio, 20b4000.gpio:

gpiochip8: GPIOs 496-503, parent: spi/spi1.0, mcp23s08, can sleep:

gpiochip7: GPIOs 504-511, parent: spi/spi0.0, mcp23s08, can sleep:

The parent string here is perhaps a good way to distinguish between similar
registrations. Perhaps the bus part of parent string could be added to another
string in the struct that is passed to userspace with the ioctl.

> 
> Yours,
> Linus Walleij

Re: [PATCH 0/6] GPIO character device skeleton

[Johan Hovold]

On Tue, Nov 03, 2015 at 06:18:42PM +0100, Linus Walleij wrote:
> On Tue, Nov 3, 2015 at 1:06 PM, Johan Hovold <johan@kernel.org> wrote:
> > [Pargmann]
> >> As an idea: We could use the complete path to create some sort of unique id for
> >> the device (perhaps hash or something different). This id can be exported as
> >> device attribute and would allow udev to create some links as known from
> >> /dev/disk/by-id for example. This would make identifying a single chip quite
> >> easy for any userspace application and we would avoid having this really long
> >> path somewhere.
> >
> > The unique ids are already there in sysfs, for example:
> >
> > $ for x in /sys/bus/gpio/devices/gpiochip*; do readlink $x; done
> > ../../../devices/platform/68000000.ocp/48310000.gpio/gpiochip0
> > ../../../devices/platform/68000000.ocp/49050000.gpio/gpiochip1
> > ../../../devices/platform/68000000.ocp/49052000.gpio/gpiochip2
> > ../../../devices/platform/68000000.ocp/49054000.gpio/gpiochip3
> > ../../../devices/platform/68000000.ocp/49056000.gpio/gpiochip4
> > ../../../devices/platform/68000000.ocp/49058000.gpio/gpiochip5
> > ../../../devices/platform/68000000.ocp/48070000.i2c/i2c-0/0-0048/twl4030-gpio/gpiochip6
> > ../../../devices/platform/68000000.ocp/48064000.usbhshost/48064800.ehci/usb1/1-2/1-2.3/1-2.3:1.0/gpiochip7
> >
> > And libudev can be used to lookup devices based on (parent) attributes
> > (such as USB VID/PID, serial numbers, etc).
> >
> > We could also export further attributes if that would help (e.g.
> > gpio-chip labels).
> 
> Yeah sysfs does provide this.
>
> This has the problem that everyone and their dog need to use udev
> or something like it. Some library or so that run around in sysfs like
> libudev+systemdlib does currently.
> 
> And since Android does not use udev, and Busybox does not use udev,
> there are quite a few million users there. Or at least two big projects that
> need to reimplement the same idea. It could be argued that they should,
> since sysfs is indeed an ABI.
>
> In the Busybox mdev case part of the goal is to minimize userspace code
> size too, and it does not support the complex rules of udev for example.
> 
> It'd be nice if devices could be uniquely identified in the chardev alone
> I think, then we don't need to much reliance on external assumptions
> and traversing sysfs somehow for more info.

Point is that this is not a gpio-specific problem. Userspace needs to
deal with this for any resource it wants to access (e.g. i2c or spi).

And you should be able to use libudev for tree-traversal without actually
using udevd, right?

Johan

Re: [PATCH 1/6] gpio: make the gpiochip a real device

[Johan Hovold]

On Wed, Nov 04, 2015 at 11:48:47AM +0100, Linus Walleij wrote:
> On Tue, Nov 3, 2015 at 10:24 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Mon, Nov 2, 2015 at 11:31 AM, Johan Hovold <johan@kernel.org> wrote:
> >> On Thu, Oct 22, 2015 at 10:32:25AM +0200, Linus Walleij wrote:
> >
> >>> +      * We memset the struct to zero to avoid reentrance issues.
> >>> +      */
> >>> +     memset(&chip->device, 0, sizeof(chip->device));
> >>
> >> This is an indication of a larger problem.
> >>
> >> First of all, you must never register the same device structure twice.
> >>
> >> And the larger problem is: With the current interface where a struct
> >> gpio_chip is passed and registered, how would you prevent the device
> >> from going away while in use?
> >
> > OK I see. What about the design pattern elsewhere to pass a
> > gpiochip desc and have a new gpiochip_register() create the
> > gpiochip and delete gpiochip_add()?
> >
> > That's the usual pattern in subsystems.
> 
> Thinking about it maybe it's simplest to just make ->dev a pointer
> and kzalloc() it at gpiochip_add().
> 
> That should solve this.

You'd avoid ever reregistering the same struct device, but that would
not solve the bigger life-time issue by itself.

Johan

Re: [PATCH 1/6] gpio: make the gpiochip a real device

[Mark Brown]

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

On Thu, Nov 05, 2015 at 10:44:11AM +0100, Johan Hovold wrote:
> On Wed, Nov 04, 2015 at 11:48:47AM +0100, Linus Walleij wrote:

> > Thinking about it maybe it's simplest to just make ->dev a pointer
> > and kzalloc() it at gpiochip_add().

> > That should solve this.

> You'd avoid ever reregistering the same struct device, but that would
> not solve the bigger life-time issue by itself.

You'd need to do something like have the file reference things via
something attached to that device and then ensure that it's made safe
(eg, by setting a pointer to NULL and then checking for that on use)
when removing the device.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 0/6] GPIO character device skeleton

[Linus Walleij]

On Thu, Nov 5, 2015 at 10:40 AM, Johan Hovold <johan@kernel.org> wrote:
> On Tue, Nov 03, 2015 at 06:18:42PM +0100, Linus Walleij wrote:

>> It'd be nice if devices could be uniquely identified in the chardev alone
>> I think, then we don't need to much reliance on external assumptions
>> and traversing sysfs somehow for more info.
>
> Point is that this is not a gpio-specific problem. Userspace needs to
> deal with this for any resource it wants to access (e.g. i2c or spi).

Yeah let's see what others have to say.

> And you should be able to use libudev for tree-traversal without actually
> using udevd, right?

Well libudev depends on libsystemd and is using it's subcomponent
sd-device to traverse sysfs. It's a pretty fat dependency for a
space-constrained embedded system, and the source code is not
maintained outside systemd anymore, leading to an
all-or-nothing approach it seems to me.

I don't think it used to be like that but it is like that now...

Yours,
Linus Walleij

Re: [PATCH 0/6] GPIO character device skeleton

[Johan Hovold]

On Thu, Nov 05, 2015 at 03:11:48PM +0100, Linus Walleij wrote:
> On Thu, Nov 5, 2015 at 10:40 AM, Johan Hovold <johan@kernel.org> wrote:
> > On Tue, Nov 03, 2015 at 06:18:42PM +0100, Linus Walleij wrote:
> 
> >> It'd be nice if devices could be uniquely identified in the chardev alone
> >> I think, then we don't need to much reliance on external assumptions
> >> and traversing sysfs somehow for more info.
> >
> > Point is that this is not a gpio-specific problem. Userspace needs to
> > deal with this for any resource it wants to access (e.g. i2c or spi).
> 
> Yeah let's see what others have to say.
> 
> > And you should be able to use libudev for tree-traversal without actually
> > using udevd, right?
> 
> Well libudev depends on libsystemd and is using it's subcomponent
> sd-device to traverse sysfs. It's a pretty fat dependency for a
> space-constrained embedded system, and the source code is not
> maintained outside systemd anymore, leading to an
> all-or-nothing approach it seems to me.

Seems to me like you're trying to solve a user-space problem in the
kernel. Even the network-device links you mentioned is something that
udev provides.

> I don't think it used to be like that but it is like that now...

Yeah, I'm not using the latest and greatest, but the libudev0 182 I took
a look at has no such dependencies and the lib seemed reasonably small
(40k).

IIRC there has been discussions about a standalone (mdev) version of it
as well.

Johan

Re: [PATCH 0/6] GPIO character device skeleton

[Linus Walleij]

On Fri, Nov 6, 2015 at 11:21 AM, Johan Hovold <johan@kernel.org> wrote:
> On Thu, Nov 05, 2015 at 03:11:48PM +0100, Linus Walleij wrote:

>> Well libudev depends on libsystemd and is using it's subcomponent
>> sd-device to traverse sysfs. It's a pretty fat dependency for a
>> space-constrained embedded system, and the source code is not
>> maintained outside systemd anymore, leading to an
>> all-or-nothing approach it seems to me.
>
> Seems to me like you're trying to solve a user-space problem in the
> kernel. Even the network-device links you mentioned is something that
> udev provides.

Yeah :/

I will try to stay off trying to fix it for now, expecting people to make use of
the sysfs attributes. We can revisit it if problems arise for users.

Yours,
Linus Walleij

Re: [PATCH 1/6] gpio: make the gpiochip a real device

[Linus Walleij]

On Thu, Nov 5, 2015 at 10:44 AM, Johan Hovold <johan@kernel.org> wrote:
> On Wed, Nov 04, 2015 at 11:48:47AM +0100, Linus Walleij wrote:

>> Thinking about it maybe it's simplest to just make ->dev a pointer
>> and kzalloc() it at gpiochip_add().
>>
>> That should solve this.

Turns out that it's a bad idea to have a struct device as pointer because
it makes it impossible to use container_of() in e.g. .remove.

> You'd avoid ever reregistering the same struct device, but that would
> not solve the bigger life-time issue by itself.

Since device_del() unconditionally kills off the devince from the system
this is equivalent to saying device_add() and device_del() should really
not be used by subsystems, am I right?

Right now there is a cryptic comment in device_del() that says it should
"only be used if device_add() was also used manually" I wonder who this
man is that is using things manually inside the kernel, I guess it actually
means they must be paired.

If I don't use device_add()/device_del(), I see this must mean I have to use
device_register()/device_unregister() as the latter only decrease the refcount
and wait for the instance to die off.

So am I reading it correctly if I understand that this is what we should be
doing? I.e device_register()/device_unregister() and then wait for the
.remove() call to do its deed, so the kobj/struct device is kept around if
e.g. sysfs or the chardev has open files.

(It makes sense. Harder to code up, but makes sense.)

Yours,
Linus Walleij

Re: [PATCH 1/6] gpio: make the gpiochip a real device

[Linus Walleij]

On Mon, Nov 16, 2015 at 3:27 PM, Linus Walleij <linus.walleij@linaro.org> wrote:

> If I don't use device_add()/device_del(), I see this must mean I have to use
> device_register()/device_unregister() as the latter only decrease the refcount
> and wait for the instance to die off.
>
> So am I reading it correctly if I understand that this is what we should be
> doing? I.e device_register()/device_unregister() and then wait for the
> .remove() call to do its deed, so the kobj/struct device is kept around if
> e.g. sysfs or the chardev has open files.

Hard to get advice on design here I see. Anyways, the clean thing to
do (after some weeks of thinking) seems to be this:

1. Introduce a void *data into gpio_chip and gpiochip_set_data()
   gpiochip_get_data() set/get it.

2. Replace all occurences of container_of() dereferencing in all
   GPIO drivers with gpiochip_[set|get]_data() so as to avoid having
   gpio_chip to be embedded into state containers.

3-1. Split gpio_chip in a static descriptor with a vtable for callbacks
   and other static config called struct gpio_chip, and
   struct gpio_device that is returned as a pointer from
   gpiochip_add(). It will need to be free:ed by gpiodevice_remove()
   after that.

3-2. In the same patch set rename all functions to indicate that
   we are now operating on a gpio_device not a gpio_chip.

3-3. After a GPIO driver issues gpio_device_unregister()
   it may stay around if there are still references to the
   struct device.

This is a very tiresome refactoring, but I'm growing confident that
it is what needs to happen to manage gpio devices in the future.

After step 3-3 we have a clean gpio_device containing a
struct device, and from there we apply the initial chardev
interface.

Yours,
Linus Walleij

Re: [PATCH 1/6] gpio: make the gpiochip a real device

[Linus Walleij]

On Thu, Dec 3, 2015 at 3:04 PM, Linus Walleij <linus.walleij@linaro.org> wrote:

> 3-1. Split gpio_chip in a static descriptor with a vtable for callbacks
>    and other static config called struct gpio_chip, and
>    struct gpio_device that is returned as a pointer from
>    gpiochip_add(). It will need to be free:ed by gpiodevice_remove()
>    after that.

This will look something like this uglyhack:
https://git.kernel.org/cgit/linux/kernel/git/linusw/linux-gpio.git/commit/?h=gpiochip-desc&id=610b19c1832acfdf6ff62523addfa08b76f77343

Yours,
Linus Walleij

Re: [PATCH 1/6] gpio: make the gpiochip a real device

[Michael Welling]

On Thu, Dec 03, 2015 at 03:06:48PM +0100, Linus Walleij wrote:
> On Thu, Dec 3, 2015 at 3:04 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> 
> > 3-1. Split gpio_chip in a static descriptor with a vtable for callbacks
> >    and other static config called struct gpio_chip, and
> >    struct gpio_device that is returned as a pointer from
> >    gpiochip_add(). It will need to be free:ed by gpiodevice_remove()
> >    after that.
> 
> This will look something like this uglyhack:
> https://git.kernel.org/cgit/linux/kernel/git/linusw/linux-gpio.git/commit/?h=gpiochip-desc&id=610b19c1832acfdf6ff62523addfa08b76f77343
>

Lots of red and green just in time for Christmas.

Let me take a look at what it would take to recover from said theoretical stab.
 
> Yours,
> Linus Walleij

Re: [PATCH 1/6] gpio: make the gpiochip a real device

[Michael Welling]

On Thu, Dec 03, 2015 at 03:06:48PM +0100, Linus Walleij wrote:
> On Thu, Dec 3, 2015 at 3:04 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> 
> > 3-1. Split gpio_chip in a static descriptor with a vtable for callbacks
> >    and other static config called struct gpio_chip, and
> >    struct gpio_device that is returned as a pointer from
> >    gpiochip_add(). It will need to be free:ed by gpiodevice_remove()
> >    after that.
> 
> This will look something like this uglyhack:
> https://git.kernel.org/cgit/linux/kernel/git/linusw/linux-gpio.git/commit/?h=gpiochip-desc&id=610b19c1832acfdf6ff62523addfa08b76f77343
>

Still quite a few place where a gpio_device is being access as if a gpio_chip:
gpio_device_set_desc_names
gpio_device_set_multiple
gpiod_hog
gpio_device_free_hogs
gpiolib_dbg_show
gpiolib_seq_show
..

There is still some calls to gpiod_to_chip which was changed to gpiod_to_device.
gpiod_hog
..

The gpiochip_add function prototype was changed in the header but the definition did not change.

Lots of work to do still. Let me know if and how I can help.

> Yours,
> Linus Walleij

Re: [PATCH 1/6] gpio: make the gpiochip a real device

[Johan Hovold]

On Thu, Dec 03, 2015 at 03:04:14PM +0100, Linus Walleij wrote:
> On Mon, Nov 16, 2015 at 3:27 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> 
> > If I don't use device_add()/device_del(), I see this must mean I have to use
> > device_register()/device_unregister() as the latter only decrease the refcount
> > and wait for the instance to die off.
> >
> > So am I reading it correctly if I understand that this is what we should be
> > doing? I.e device_register()/device_unregister() and then wait for the
> > .remove() call to do its deed, so the kobj/struct device is kept around if
> > e.g. sysfs or the chardev has open files.
> 
> Hard to get advice on design here I see. Anyways, the clean thing to
> do (after some weeks of thinking) seems to be this:
> 
> 1. Introduce a void *data into gpio_chip and gpiochip_set_data()
>    gpiochip_get_data() set/get it.
> 
> 2. Replace all occurences of container_of() dereferencing in all
>    GPIO drivers with gpiochip_[set|get]_data() so as to avoid having
>    gpio_chip to be embedded into state containers.
> 
> 3-1. Split gpio_chip in a static descriptor with a vtable for callbacks
>    and other static config called struct gpio_chip, and
>    struct gpio_device that is returned as a pointer from
>    gpiochip_add(). It will need to be free:ed by gpiodevice_remove()
>    after that.
> 
> 3-2. In the same patch set rename all functions to indicate that
>    we are now operating on a gpio_device not a gpio_chip.
> 
> 3-3. After a GPIO driver issues gpio_device_unregister()
>    it may stay around if there are still references to the
>    struct device.
> 
> This is a very tiresome refactoring, but I'm growing confident that
> it is what needs to happen to manage gpio devices in the future.
> 
> After step 3-3 we have a clean gpio_device containing a
> struct device, and from there we apply the initial chardev
> interface.

This sounds unnecessarily complicated.

Why not use a more standard approach of

	gpiochip_create()

that allocates the state container that the driver can then initialise
further (e.g. callback pointers) and finally register with a call to:

	gpiochip_add()

When the parent device is going away, it calls

	gpiochip_remove()

which unregisters the gpiochip device (i.e. calls device_del), updates
its state to "disconnected" and drops the gpiolib's reference.

There may still be other devices (consumers) holding references to the
gpiochip device, which is not deallocated until the final reference is
dropped (i.e. at final gpiod_free).

Johan

Re: [PATCH 1/6] gpio: make the gpiochip a real device

[Linus Walleij]

On Fri, Dec 4, 2015 at 11:31 PM, Michael Welling <mwelling@ieee.org> wrote:
> On Thu, Dec 03, 2015 at 03:06:48PM +0100, Linus Walleij wrote:
>> On Thu, Dec 3, 2015 at 3:04 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
>>
>> > 3-1. Split gpio_chip in a static descriptor with a vtable for callbacks
>> >    and other static config called struct gpio_chip, and
>> >    struct gpio_device that is returned as a pointer from
>> >    gpiochip_add(). It will need to be free:ed by gpiodevice_remove()
>> >    after that.
>>
>> This will look something like this uglyhack:
>> https://git.kernel.org/cgit/linux/kernel/git/linusw/linux-gpio.git/commit/?h=gpiochip-desc&id=610b19c1832acfdf6ff62523addfa08b76f77343
>>
>
> Still quite a few place where a gpio_device is being access as if a gpio_chip:
> gpio_device_set_desc_names
> gpio_device_set_multiple
> gpiod_hog
> gpio_device_free_hogs
> gpiolib_dbg_show
> gpiolib_seq_show
> ..
>
> There is still some calls to gpiod_to_chip which was changed to gpiod_to_device.
> gpiod_hog
> ..
>
> The gpiochip_add function prototype was changed in the header but the definition did not change.
>
> Lots of work to do still. Let me know if and how I can help.

Yeah it doesn't even compile. It was more to illustrate.

I am now figuring out how to proceed after merging 187 patches
removing the use of container_of() from all GPIO drivers.

Yours,
Linus Walleij

Re: [PATCH 1/6] gpio: make the gpiochip a real device

[Linus Walleij]

On Tue, Dec 8, 2015 at 10:29 AM, Johan Hovold <johan@kernel.org> wrote:
> On Thu, Dec 03, 2015 at 03:04:14PM +0100, Linus Walleij wrote:
>> On Mon, Nov 16, 2015 at 3:27 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
>>
>> > If I don't use device_add()/device_del(), I see this must mean I have to use
>> > device_register()/device_unregister() as the latter only decrease the refcount
>> > and wait for the instance to die off.
>> >
>> > So am I reading it correctly if I understand that this is what we should be
>> > doing? I.e device_register()/device_unregister() and then wait for the
>> > .remove() call to do its deed, so the kobj/struct device is kept around if
>> > e.g. sysfs or the chardev has open files.
>>
>> Hard to get advice on design here I see. Anyways, the clean thing to
>> do (after some weeks of thinking) seems to be this:
>>
>> 1. Introduce a void *data into gpio_chip and gpiochip_set_data()
>>    gpiochip_get_data() set/get it.
>>
>> 2. Replace all occurences of container_of() dereferencing in all
>>    GPIO drivers with gpiochip_[set|get]_data() so as to avoid having
>>    gpio_chip to be embedded into state containers.
>>
>> 3-1. Split gpio_chip in a static descriptor with a vtable for callbacks
>>    and other static config called struct gpio_chip, and
>>    struct gpio_device that is returned as a pointer from
>>    gpiochip_add(). It will need to be free:ed by gpiodevice_remove()
>>    after that.
>>
>> 3-2. In the same patch set rename all functions to indicate that
>>    we are now operating on a gpio_device not a gpio_chip.
>>
>> 3-3. After a GPIO driver issues gpio_device_unregister()
>>    it may stay around if there are still references to the
>>    struct device.
>>
>> This is a very tiresome refactoring, but I'm growing confident that
>> it is what needs to happen to manage gpio devices in the future.
>>
>> After step 3-3 we have a clean gpio_device containing a
>> struct device, and from there we apply the initial chardev
>> interface.
>
> This sounds unnecessarily complicated.
>
> Why not use a more standard approach of
>
>         gpiochip_create()
>
> that allocates the state container that the driver can then initialise
> further (e.g. callback pointers) and finally register with a call to:
>
>         gpiochip_add()
>
> When the parent device is going away, it calls
>
>         gpiochip_remove()
>
> which unregisters the gpiochip device (i.e. calls device_del), updates
> its state to "disconnected" and drops the gpiolib's reference.
>
> There may still be other devices (consumers) holding references to the
> gpiochip device, which is not deallocated until the final reference is
> dropped (i.e. at final gpiod_free).

Yeah after discussing with Russell it seems something like this is better.
I really like what netdev is doing with alloc_netdev(), so I imagine:

struct gpio_device *alloc_gpiodev(sizeof_priv, name, setup)

Where sizeof_priv is the size of the state container for the GPIO driver
instance, setup() is a callback to fill in vtable etc and name is what we
now call label.

Then ther will be gpiodev_priv(dev) to get the private data as a void *
pointer.

The whole thing is memory managed by the gpiolib core.

gpiodev_remove() to get rid of the driver reference.

And this would come with a proper struct device and all.

Right now I'm thinking about how to put in this interface so that
we can convert drivers one by one and need not convert all in
a gigantice 20.000 line patch.

Yours,
Linus Walleij

Re: [PATCH 3/6] gpio: add a userspace chardev ABI for GPIOs

[Bamvor Zhang Jian]

Hi, Linus

On 10/22/2015 04:32 PM, Linus Walleij wrote:
> A new chardev that is to be used for userspace GPIO access is
[...]
> diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h
> new file mode 100644
> index 000000000000..3188a87bdaa0
> --- /dev/null
> +++ b/include/uapi/linux/gpio.h
> @@ -0,0 +1,28 @@
> +/*
> + * <linux/gpio.h> - userspace ABI for the GPIO character devices
> + *
> + * Copyright (C) 2015 Linus Walleij
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + */
> +#ifndef _UAPI_GPIO_H_
> +#define _UAPI_GPIO_H_
> +
> +#include <linux/ioctl.h>
> +#include <linux/types.h>
> +
> +/**
> + * struct gpiochip_info - Information about a certain GPIO chip
> + * @name: the name of this GPIO chip
> + * @lines: number of GPIO lines on this chip
> + */
> +struct gpiochip_info {
> + char name[32];
> + __u32 lines;
> +};
> +
> +#define GPIO_GET_CHIPINFO_IOCTL _IOR('o', 0x01, struct gpiochip_info)

I am trying to use this interface to do my gpio mockup test. I need to list
all the gpiochips attach to one gpio driver(aka gpio-mockup, there may
be more than one gpio drivers in the system). And then test some of the
pin of each gpio_chip.

>From the api(ioctl GPIO_GET_CHIPINFO_IOCTL: gpiochip_info), it seems
that I could list all the gpiochips or list only one gpiochip. But
I could not list gpiochip belongs to one gpio driver.

Do I understand correctly?
Will we add a new api to do it?

Regards

Bamvor

> +
> +#endif /* _UAPI_GPIO_H_ */
>

Re: [PATCH 3/6] gpio: add a userspace chardev ABI for GPIOs

[Linus Walleij]

On Sat, Oct 24, 2015 at 2:30 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Thu, Oct 22, 2015 at 10:32:27AM +0200, Linus Walleij wrote:
>> +/**
>> + * struct gpiochip_info - Information about a certain GPIO chip
>> + * @name: the name of this GPIO chip
>> + * @lines: number of GPIO lines on this chip
>> + */
>> +struct gpiochip_info {
>> +     char name[32];
>
> To be pendantic, s/char/__u8/

Thanks, will fix.

> Otherwise, looks good, but I don't see the read/write protocol here, is
> that in a later patch?

I wanted to get the very basic infrastructure in place first
so we can then add a careful selection of ioctl():s one by
one.

get/set is delicate as I want to be able to handle
reading/switching multiple lines at once from day one as
that reduce context switching nicely, and we also have
.set_multiple() in the driver backend for select chips.

Yours,
Linus Walleij

Re: [PATCH 3/6] gpio: add a userspace chardev ABI for GPIOs

[Linus Walleij]

On Wed, Jan 27, 2016 at 11:05 AM, Bamvor Zhang Jian
<bamvor.zhangjian@linaro.org> wrote:

> From the api(ioctl GPIO_GET_CHIPINFO_IOCTL: gpiochip_info), it seems
> that I could list all the gpiochips or list only one gpiochip. But
> I could not list gpiochip belongs to one gpio driver.

I think you can get that information from parsing sysfs?

Yours,
Linus Walleij

Re: [PATCH 3/6] gpio: add a userspace chardev ABI for GPIOs

[Bamvor Zhang Jian]

Hi, Linus

On 01/28/2016 07:14 PM, Linus Walleij wrote:
> On Wed, Jan 27, 2016 at 11:05 AM, Bamvor Zhang Jian
> <bamvor.zhangjian@linaro.org> wrote:
>
>> From the api(ioctl GPIO_GET_CHIPINFO_IOCTL: gpiochip_info), it seems
>> that I could list all the gpiochips or list only one gpiochip. But
>> I could not list gpiochip belongs to one gpio driver.
>
> I think you can get that information from parsing sysfs?
Yeap, I could get the directory:
"/sys/devices/platform/gpio-mockup/gpio/"

I notice that you mark the sysfs ABI as obsolete in these series. I
am not sure whether it is reasonable to read it from syfs when using
chardev.

Regars

Bamvor
>
> Yours,
> Linus Walleij
>

Re: [PATCH 3/6] gpio: add a userspace chardev ABI for GPIOs

[Linus Walleij]

On Fri, Jan 29, 2016 at 11:24 AM, Bamvor Zhang Jian
<bamvor.zhangjian@linaro.org> wrote:
> On 01/28/2016 07:14 PM, Linus Walleij wrote:
>> On Wed, Jan 27, 2016 at 11:05 AM, Bamvor Zhang Jian
>> <bamvor.zhangjian@linaro.org> wrote:
>>
>>> From the api(ioctl GPIO_GET_CHIPINFO_IOCTL: gpiochip_info), it seems
>>> that I could list all the gpiochips or list only one gpiochip. But
>>> I could not list gpiochip belongs to one gpio driver.
>>
>> I think you can get that information from parsing sysfs?
> Yeap, I could get the directory:
> "/sys/devices/platform/gpio-mockup/gpio/"
>
> I notice that you mark the sysfs ABI as obsolete in these series. I
> am not sure whether it is reasonable to read it from syfs when using
> chardev.

They can probably be used in parallel. I don't have a Kconfig
option for the chardev, it is always turned on, so as to make
people use it.

I even consider putting nasty stuff in dmesg and the boot crawl
for people using the sysfs at some point.

Yours,
Linus Walleij