[PATCH v2] gpio: add userspace ABI for GPIO line information

[PATCH v2] gpio: add userspace ABI for GPIO line information

[Linus Walleij]

This adds a GPIO line ABI for getting name, label and a few select
flags from the kernel.

This hides the kernel internals and only tells userspace what it
may need to know: the different in-kernel consumers are masked
behind the flag "kernel" and that is all userspace needs to know.

However electric characteristics like active low, open drain etc
are reflected to userspace, as this is important information.

We provide information on all lines on all chips, later on we will
likely add a flag for the chardev consumer so we can filter and
display only the lines userspace actually uses in e.g. lsgpio,
but then we first need an ABI for userspace to grab and use
(get/set/select direction) a GPIO line.

Sample output from "lsgpio" on ux500:

GPIO chip: gpiochip7, "8011e000.gpio", 32 GPIO lines
        line 0: unnamed unlabeled
        line 1: unnamed unlabeled
(...)
        line 25: unnamed "SFH7741 Proximity Sensor" [kernel output open-drain]
        line 26: unnamed unlabeled
(...)

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- Fix unitialized lineinfo variables, especially flags,
  on both the userspace and kernelspace side.
- Pretty print.
---
 drivers/gpio/gpiolib.c    | 51 ++++++++++++++++++++++++--
 include/uapi/linux/gpio.h | 26 ++++++++++++++
 tools/gpio/gpio-utils.h   |  2 ++
 tools/gpio/lsgpio.c       | 91 ++++++++++++++++++++++++++++++++++++++++-------
 4 files changed, 156 insertions(+), 14 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index a022e9249f69..a38265dec794 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -331,14 +331,15 @@ static long gpio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 	struct gpio_device *gdev = filp->private_data;
 	struct gpio_chip *chip = gdev->chip;
 	int __user *ip = (int __user *)arg;
-	struct gpiochip_info chipinfo;
 
 	/* We fail any subsequent ioctl():s when the chip is gone */
 	if (!chip)
 		return -ENODEV;
 
+	/* Fill in the struct and pass to userspace */
 	if (cmd == GPIO_GET_CHIPINFO_IOCTL) {
-		/* Fill in the struct and pass to userspace */
+		struct gpiochip_info chipinfo;
+
 		strncpy(chipinfo.name, dev_name(&gdev->dev),
 			sizeof(chipinfo.name));
 		chipinfo.name[sizeof(chipinfo.name)-1] = '\0';
@@ -349,6 +350,52 @@ static long gpio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		if (copy_to_user(ip, &chipinfo, sizeof(chipinfo)))
 			return -EFAULT;
 		return 0;
+	} else if (cmd == GPIO_GET_LINEINFO_IOCTL) {
+		struct gpioline_info lineinfo;
+		struct gpio_desc *desc;
+
+		if (copy_from_user(&lineinfo, ip, sizeof(lineinfo)))
+			return -EFAULT;
+		if (lineinfo.line_offset > gdev->ngpio)
+			return -EINVAL;
+
+		desc = &gdev->descs[lineinfo.line_offset];
+		if (desc->name) {
+			strncpy(lineinfo.name, desc->name,
+				sizeof(lineinfo.name));
+			lineinfo.name[sizeof(lineinfo.name)-1] = '\0';
+		} else {
+			lineinfo.name[0] = '\0';
+		}
+		if (desc->label) {
+			strncpy(lineinfo.label, desc->label,
+				sizeof(lineinfo.label));
+			lineinfo.label[sizeof(lineinfo.label)-1] = '\0';
+		} else {
+			lineinfo.label[0] = '\0';
+		}
+
+		/*
+		 * Userspace only need to know that the kernel is using
+		 * this GPIO so it can't use it.
+		 */
+		lineinfo.flags = 0;
+		if (desc->flags & (FLAG_REQUESTED | FLAG_IS_HOGGED |
+				   FLAG_USED_AS_IRQ | FLAG_EXPORT |
+				   FLAG_SYSFS))
+			lineinfo.flags |= GPIOLINE_FLAG_KERNEL;
+		if (desc->flags & FLAG_IS_OUT)
+			lineinfo.flags |= GPIOLINE_FLAG_IS_OUT;
+		if (desc->flags & FLAG_ACTIVE_LOW)
+			lineinfo.flags |= GPIOLINE_FLAG_ACTIVE_LOW;
+		if (desc->flags & FLAG_OPEN_DRAIN)
+			lineinfo.flags |= GPIOLINE_FLAG_OPEN_DRAIN;
+		if (desc->flags & FLAG_OPEN_SOURCE)
+			lineinfo.flags |= GPIOLINE_FLAG_OPEN_SOURCE;
+
+		if (copy_to_user(ip, &lineinfo, sizeof(lineinfo)))
+			return -EFAULT;
+		return 0;
 	}
 	return -EINVAL;
 }
diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h
index 3f93e1bcd3dd..416ce47f2291 100644
--- a/include/uapi/linux/gpio.h
+++ b/include/uapi/linux/gpio.h
@@ -25,6 +25,32 @@ struct gpiochip_info {
 	__u32 lines;
 };
 
+/* Line is in use by the kernel */
+#define GPIOLINE_FLAG_KERNEL		(1UL << 0)
+#define GPIOLINE_FLAG_IS_OUT		(1UL << 1)
+#define GPIOLINE_FLAG_ACTIVE_LOW	(1UL << 2)
+#define GPIOLINE_FLAG_OPEN_DRAIN	(1UL << 3)
+#define GPIOLINE_FLAG_OPEN_SOURCE	(1UL << 4)
+
+/**
+ * struct gpioline_info - Information about a certain GPIO line
+ * @line_offset: the local offset on this GPIO device, fill in when
+ * requesting information from the kernel
+ * @flags: various flags for this line
+ * @name: the name of this GPIO line
+ * @label: a functional name for this GPIO line
+ * @kernel: this GPIO is in use by the kernel
+ * @out: this GPIO is an output line (false means it is an input)
+ * @active_low: this GPIO is active low
+ */
+struct gpioline_info {
+	__u32 line_offset;
+	__u32 flags;
+	char name[32];
+	char label[32];
+};
+
 #define GPIO_GET_CHIPINFO_IOCTL _IOR('o', 0x01, struct gpiochip_info)
+#define GPIO_GET_LINEINFO_IOCTL _IOWR('o', 0x02, struct gpioline_info)
 
 #endif /* _UAPI_GPIO_H_ */
diff --git a/tools/gpio/gpio-utils.h b/tools/gpio/gpio-utils.h
index b18209a45ad3..5f57133b8c04 100644
--- a/tools/gpio/gpio-utils.h
+++ b/tools/gpio/gpio-utils.h
@@ -16,6 +16,8 @@
 
 #include <string.h>
 
+#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
+
 static inline int check_prefix(const char *str, const char *prefix)
 {
 	return strlen(str) > strlen(prefix) &&
diff --git a/tools/gpio/lsgpio.c b/tools/gpio/lsgpio.c
index 692233f561fb..5535ce81f8f7 100644
--- a/tools/gpio/lsgpio.c
+++ b/tools/gpio/lsgpio.c
@@ -26,12 +26,56 @@
 
 #include "gpio-utils.h"
 
+struct gpio_flag {
+	char *name;
+	unsigned long mask;
+};
+
+struct gpio_flag flagnames[] = {
+	{
+		.name = "kernel",
+		.mask = GPIOLINE_FLAG_KERNEL,
+	},
+	{
+		.name = "output",
+		.mask = GPIOLINE_FLAG_IS_OUT,
+	},
+	{
+		.name = "active-low",
+		.mask = GPIOLINE_FLAG_ACTIVE_LOW,
+	},
+	{
+		.name = "open-drain",
+		.mask = GPIOLINE_FLAG_OPEN_DRAIN,
+	},
+	{
+		.name = "open-source",
+		.mask = GPIOLINE_FLAG_OPEN_SOURCE,
+	},
+};
+
+void print_flags(unsigned long flags)
+{
+	int i;
+	int printed = 0;
+
+	for (i = 0; i < ARRAY_SIZE(flagnames); i++) {
+		if (flags & flagnames[i].mask) {
+			if (printed)
+				fprintf(stdout, " ");
+			fprintf(stdout, "%s", flagnames[i].name);
+			printed++;
+		}
+	}
+}
+
 int list_device(const char *device_name)
 {
 	struct gpiochip_info cinfo;
 	char *chrdev_name;
 	int fd;
 	int ret;
+	int i;
 
 	ret = asprintf(&chrdev_name, "/dev/%s", device_name);
 	if (ret < 0)
@@ -41,32 +85,55 @@ int list_device(const char *device_name)
 	if (fd == -1) {
 		ret = -errno;
 		fprintf(stderr, "Failed to open %s\n", chrdev_name);
-		goto free_chrdev_name;
+		goto exit_close_error;
 	}
 
 	/* 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;
+		perror("Failed to issue CHIPINFO IOCTL\n");
+		goto exit_close_error;
 	}
 	fprintf(stdout, "GPIO chip: %s, \"%s\", %u GPIO lines\n",
 		cinfo.name, cinfo.label, cinfo.lines);
 
-	if (close(fd) == -1)  {
-		ret = -errno;
-		goto free_chrdev_name;
+	/* Loop over the lines and print info */
+	for (i = 0; i < cinfo.lines; i++) {
+		struct gpioline_info linfo;
+
+		memset(&linfo, 0, sizeof(linfo));
+		linfo.line_offset = i;
+
+		ret = ioctl(fd, GPIO_GET_LINEINFO_IOCTL, &linfo);
+		if (ret == -1) {
+			ret = -errno;
+			perror("Failed to issue LINEINFO IOCTL\n");
+			goto exit_close_error;
+		}
+		fprintf(stdout, "\tline %d:", linfo.line_offset);
+		if (linfo.name[0])
+			fprintf(stdout, " %s", linfo.name);
+		else
+			fprintf(stdout, " unnamed");
+		if (linfo.label[0])
+			fprintf(stdout, " \"%s\"", linfo.label);
+		else
+			fprintf(stdout, " unlabeled");
+		if (linfo.flags) {
+			fprintf(stdout, " [");
+			print_flags(linfo.flags);
+			fprintf(stdout, "]");
+		}
+		fprintf(stdout, "\n");
+
 	}
 
-free_chrdev_name:
+exit_close_error:
+	if (close(fd) == -1)
+		perror("Failed to close GPIO character device file");
 	free(chrdev_name);
-
 	return ret;
-
 }
 
 void print_usage(void)
-- 
2.4.3

Re: [PATCH v2] gpio: add userspace ABI for GPIO line information

[Michael Welling]

On Mon, Feb 15, 2016 at 02:20:35PM +0100, Linus Walleij wrote:
> This adds a GPIO line ABI for getting name, label and a few select
> flags from the kernel.
> 
> This hides the kernel internals and only tells userspace what it
> may need to know: the different in-kernel consumers are masked
> behind the flag "kernel" and that is all userspace needs to know.
> 
> However electric characteristics like active low, open drain etc
> are reflected to userspace, as this is important information.
> 
> We provide information on all lines on all chips, later on we will
> likely add a flag for the chardev consumer so we can filter and
> display only the lines userspace actually uses in e.g. lsgpio,
> but then we first need an ABI for userspace to grab and use
> (get/set/select direction) a GPIO line.
> 
> Sample output from "lsgpio" on ux500:
> 
> GPIO chip: gpiochip7, "8011e000.gpio", 32 GPIO lines
>         line 0: unnamed unlabeled
>         line 1: unnamed unlabeled
> (...)
>         line 25: unnamed "SFH7741 Proximity Sensor" [kernel output open-drain]
>         line 26: unnamed unlabeled
> (...)
> 
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

I attempted to test the gpio char implementation on the Dragonboard 410c and it
fails to boot.

I just pulled the linux-gpio/chardev-more branch and built from there. The board
boots from the mainline 4.5-rc2.

If there were any boot messages I would give them to you but it fails early.
Tried to get more from earlycon but still nothing.

Do you have a 64 bit arm target to test this on to see if it fails for you as
well?

Re: [PATCH v2] gpio: add userspace ABI for GPIO line information

[Michael Welling]

On Mon, Feb 15, 2016 at 02:20:35PM +0100, Linus Walleij wrote:
> This adds a GPIO line ABI for getting name, label and a few select
> flags from the kernel.
> 
> This hides the kernel internals and only tells userspace what it
> may need to know: the different in-kernel consumers are masked
> behind the flag "kernel" and that is all userspace needs to know.
> 
> However electric characteristics like active low, open drain etc
> are reflected to userspace, as this is important information.
> 
> We provide information on all lines on all chips, later on we will
> likely add a flag for the chardev consumer so we can filter and
> display only the lines userspace actually uses in e.g. lsgpio,
> but then we first need an ABI for userspace to grab and use
> (get/set/select direction) a GPIO line.
> 
> Sample output from "lsgpio" on ux500:
> 
> GPIO chip: gpiochip7, "8011e000.gpio", 32 GPIO lines
>         line 0: unnamed unlabeled
>         line 1: unnamed unlabeled
> (...)
>         line 25: unnamed "SFH7741 Proximity Sensor" [kernel output open-drain]
>         line 26: unnamed unlabeled
> (...)
>

After applying the patch from Josh Cartwright:
http://www.spinics.net/lists/arm-kernel/msg484070.html

root@dragonboard-410c:/sys/class/gpio# ~/lsgpio 
GPIO chip: gpiochip0, "1000000.pinctrl", 122 GPIO lines
        line 0: unnamed unlabeled
(...)

Tested-by: Michael Welling <mwelling@ieee.org>
 
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v2:
> - Fix unitialized lineinfo variables, especially flags,
>   on both the userspace and kernelspace side.
> - Pretty print.
> ---
>  drivers/gpio/gpiolib.c    | 51 ++++++++++++++++++++++++--
>  include/uapi/linux/gpio.h | 26 ++++++++++++++
>  tools/gpio/gpio-utils.h   |  2 ++
>  tools/gpio/lsgpio.c       | 91 ++++++++++++++++++++++++++++++++++++++++-------
>  4 files changed, 156 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index a022e9249f69..a38265dec794 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -331,14 +331,15 @@ static long gpio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  	struct gpio_device *gdev = filp->private_data;
>  	struct gpio_chip *chip = gdev->chip;
>  	int __user *ip = (int __user *)arg;
> -	struct gpiochip_info chipinfo;
>  
>  	/* We fail any subsequent ioctl():s when the chip is gone */
>  	if (!chip)
>  		return -ENODEV;
>  
> +	/* Fill in the struct and pass to userspace */
>  	if (cmd == GPIO_GET_CHIPINFO_IOCTL) {
> -		/* Fill in the struct and pass to userspace */
> +		struct gpiochip_info chipinfo;
> +
>  		strncpy(chipinfo.name, dev_name(&gdev->dev),
>  			sizeof(chipinfo.name));
>  		chipinfo.name[sizeof(chipinfo.name)-1] = '\0';
> @@ -349,6 +350,52 @@ static long gpio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  		if (copy_to_user(ip, &chipinfo, sizeof(chipinfo)))
>  			return -EFAULT;
>  		return 0;
> +	} else if (cmd == GPIO_GET_LINEINFO_IOCTL) {
> +		struct gpioline_info lineinfo;
> +		struct gpio_desc *desc;
> +
> +		if (copy_from_user(&lineinfo, ip, sizeof(lineinfo)))
> +			return -EFAULT;
> +		if (lineinfo.line_offset > gdev->ngpio)
> +			return -EINVAL;
> +
> +		desc = &gdev->descs[lineinfo.line_offset];
> +		if (desc->name) {
> +			strncpy(lineinfo.name, desc->name,
> +				sizeof(lineinfo.name));
> +			lineinfo.name[sizeof(lineinfo.name)-1] = '\0';
> +		} else {
> +			lineinfo.name[0] = '\0';
> +		}
> +		if (desc->label) {
> +			strncpy(lineinfo.label, desc->label,
> +				sizeof(lineinfo.label));
> +			lineinfo.label[sizeof(lineinfo.label)-1] = '\0';
> +		} else {
> +			lineinfo.label[0] = '\0';
> +		}
> +
> +		/*
> +		 * Userspace only need to know that the kernel is using
> +		 * this GPIO so it can't use it.
> +		 */
> +		lineinfo.flags = 0;
> +		if (desc->flags & (FLAG_REQUESTED | FLAG_IS_HOGGED |
> +				   FLAG_USED_AS_IRQ | FLAG_EXPORT |
> +				   FLAG_SYSFS))
> +			lineinfo.flags |= GPIOLINE_FLAG_KERNEL;
> +		if (desc->flags & FLAG_IS_OUT)
> +			lineinfo.flags |= GPIOLINE_FLAG_IS_OUT;
> +		if (desc->flags & FLAG_ACTIVE_LOW)
> +			lineinfo.flags |= GPIOLINE_FLAG_ACTIVE_LOW;
> +		if (desc->flags & FLAG_OPEN_DRAIN)
> +			lineinfo.flags |= GPIOLINE_FLAG_OPEN_DRAIN;
> +		if (desc->flags & FLAG_OPEN_SOURCE)
> +			lineinfo.flags |= GPIOLINE_FLAG_OPEN_SOURCE;
> +
> +		if (copy_to_user(ip, &lineinfo, sizeof(lineinfo)))
> +			return -EFAULT;
> +		return 0;
>  	}
>  	return -EINVAL;
>  }
> diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h
> index 3f93e1bcd3dd..416ce47f2291 100644
> --- a/include/uapi/linux/gpio.h
> +++ b/include/uapi/linux/gpio.h
> @@ -25,6 +25,32 @@ struct gpiochip_info {
>  	__u32 lines;
>  };
>  
> +/* Line is in use by the kernel */
> +#define GPIOLINE_FLAG_KERNEL		(1UL << 0)
> +#define GPIOLINE_FLAG_IS_OUT		(1UL << 1)
> +#define GPIOLINE_FLAG_ACTIVE_LOW	(1UL << 2)
> +#define GPIOLINE_FLAG_OPEN_DRAIN	(1UL << 3)
> +#define GPIOLINE_FLAG_OPEN_SOURCE	(1UL << 4)
> +
> +/**
> + * struct gpioline_info - Information about a certain GPIO line
> + * @line_offset: the local offset on this GPIO device, fill in when
> + * requesting information from the kernel
> + * @flags: various flags for this line
> + * @name: the name of this GPIO line
> + * @label: a functional name for this GPIO line
> + * @kernel: this GPIO is in use by the kernel
> + * @out: this GPIO is an output line (false means it is an input)
> + * @active_low: this GPIO is active low
> + */
> +struct gpioline_info {
> +	__u32 line_offset;
> +	__u32 flags;
> +	char name[32];
> +	char label[32];
> +};
> +
>  #define GPIO_GET_CHIPINFO_IOCTL _IOR('o', 0x01, struct gpiochip_info)
> +#define GPIO_GET_LINEINFO_IOCTL _IOWR('o', 0x02, struct gpioline_info)
>  
>  #endif /* _UAPI_GPIO_H_ */
> diff --git a/tools/gpio/gpio-utils.h b/tools/gpio/gpio-utils.h
> index b18209a45ad3..5f57133b8c04 100644
> --- a/tools/gpio/gpio-utils.h
> +++ b/tools/gpio/gpio-utils.h
> @@ -16,6 +16,8 @@
>  
>  #include <string.h>
>  
> +#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
> +
>  static inline int check_prefix(const char *str, const char *prefix)
>  {
>  	return strlen(str) > strlen(prefix) &&
> diff --git a/tools/gpio/lsgpio.c b/tools/gpio/lsgpio.c
> index 692233f561fb..5535ce81f8f7 100644
> --- a/tools/gpio/lsgpio.c
> +++ b/tools/gpio/lsgpio.c
> @@ -26,12 +26,56 @@
>  
>  #include "gpio-utils.h"
>  
> +struct gpio_flag {
> +	char *name;
> +	unsigned long mask;
> +};
> +
> +struct gpio_flag flagnames[] = {
> +	{
> +		.name = "kernel",
> +		.mask = GPIOLINE_FLAG_KERNEL,
> +	},
> +	{
> +		.name = "output",
> +		.mask = GPIOLINE_FLAG_IS_OUT,
> +	},
> +	{
> +		.name = "active-low",
> +		.mask = GPIOLINE_FLAG_ACTIVE_LOW,
> +	},
> +	{
> +		.name = "open-drain",
> +		.mask = GPIOLINE_FLAG_OPEN_DRAIN,
> +	},
> +	{
> +		.name = "open-source",
> +		.mask = GPIOLINE_FLAG_OPEN_SOURCE,
> +	},
> +};
> +
> +void print_flags(unsigned long flags)
> +{
> +	int i;
> +	int printed = 0;
> +
> +	for (i = 0; i < ARRAY_SIZE(flagnames); i++) {
> +		if (flags & flagnames[i].mask) {
> +			if (printed)
> +				fprintf(stdout, " ");
> +			fprintf(stdout, "%s", flagnames[i].name);
> +			printed++;
> +		}
> +	}
> +}
> +
>  int list_device(const char *device_name)
>  {
>  	struct gpiochip_info cinfo;
>  	char *chrdev_name;
>  	int fd;
>  	int ret;
> +	int i;
>  
>  	ret = asprintf(&chrdev_name, "/dev/%s", device_name);
>  	if (ret < 0)
> @@ -41,32 +85,55 @@ int list_device(const char *device_name)
>  	if (fd == -1) {
>  		ret = -errno;
>  		fprintf(stderr, "Failed to open %s\n", chrdev_name);
> -		goto free_chrdev_name;
> +		goto exit_close_error;
>  	}
>  
>  	/* 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;
> +		perror("Failed to issue CHIPINFO IOCTL\n");
> +		goto exit_close_error;
>  	}
>  	fprintf(stdout, "GPIO chip: %s, \"%s\", %u GPIO lines\n",
>  		cinfo.name, cinfo.label, cinfo.lines);
>  
> -	if (close(fd) == -1)  {
> -		ret = -errno;
> -		goto free_chrdev_name;
> +	/* Loop over the lines and print info */
> +	for (i = 0; i < cinfo.lines; i++) {
> +		struct gpioline_info linfo;
> +
> +		memset(&linfo, 0, sizeof(linfo));
> +		linfo.line_offset = i;
> +
> +		ret = ioctl(fd, GPIO_GET_LINEINFO_IOCTL, &linfo);
> +		if (ret == -1) {
> +			ret = -errno;
> +			perror("Failed to issue LINEINFO IOCTL\n");
> +			goto exit_close_error;
> +		}
> +		fprintf(stdout, "\tline %d:", linfo.line_offset);
> +		if (linfo.name[0])
> +			fprintf(stdout, " %s", linfo.name);
> +		else
> +			fprintf(stdout, " unnamed");
> +		if (linfo.label[0])
> +			fprintf(stdout, " \"%s\"", linfo.label);
> +		else
> +			fprintf(stdout, " unlabeled");
> +		if (linfo.flags) {
> +			fprintf(stdout, " [");
> +			print_flags(linfo.flags);
> +			fprintf(stdout, "]");
> +		}
> +		fprintf(stdout, "\n");
> +
>  	}
>  
> -free_chrdev_name:
> +exit_close_error:
> +	if (close(fd) == -1)
> +		perror("Failed to close GPIO character device file");
>  	free(chrdev_name);
> -
>  	return ret;
> -
>  }
>  
>  void print_usage(void)
> -- 
> 2.4.3
> 

Re: [PATCH v2] gpio: add userspace ABI for GPIO line information

[Linus Walleij]

On Wed, Feb 17, 2016 at 9:54 PM, Michael Welling <mwelling@ieee.org> wrote:

> After applying the patch from Josh Cartwright:
> http://www.spinics.net/lists/arm-kernel/msg484070.html

Sorry for screwing up :(

> root@dragonboard-410c:/sys/class/gpio# ~/lsgpio
> GPIO chip: gpiochip0, "1000000.pinctrl", 122 GPIO lines
>         line 0: unnamed unlabeled
> (...)
>
> Tested-by: Michael Welling <mwelling@ieee.org>

I hope that means you also like where this is going :D

I merged this to my main branch now.

Yours,
Linus Walleij

Re: [PATCH v2] gpio: add userspace ABI for GPIO line information

[Michael Welling]

On Fri, Feb 19, 2016 at 09:55:41AM +0100, Linus Walleij wrote:
> On Wed, Feb 17, 2016 at 9:54 PM, Michael Welling <mwelling@ieee.org> wrote:
> 
> > After applying the patch from Josh Cartwright:
> > http://www.spinics.net/lists/arm-kernel/msg484070.html
> 
> Sorry for screwing up :(
> 
> > root@dragonboard-410c:/sys/class/gpio# ~/lsgpio
> > GPIO chip: gpiochip0, "1000000.pinctrl", 122 GPIO lines
> >         line 0: unnamed unlabeled
> > (...)
> >
> > Tested-by: Michael Welling <mwelling@ieee.org>
> 
> I hope that means you also like where this is going :D
>

I am very happy to see the progress that is being made.

I am looking forward to see how the GPIOs will be modified from userspace.
The fact that single controllers can host hundreds of GPIOs is going to
make it an interesting problem.

Do you have a sketch of how the accesses with be performed?
ioctl, read, write.

Thanks for all of the work so far.
> I merged this to my main branch now.
> 
> Yours,
> Linus Walleij

Re: [PATCH v2] gpio: add userspace ABI for GPIO line information

[Linus Walleij]

On Fri, Feb 19, 2016 at 10:46 AM, Michael Welling <mwelling@ieee.org> wrote:

> I am looking forward to see how the GPIOs will be modified from userspace.
> The fact that single controllers can host hundreds of GPIOs is going to
> make it an interesting problem.
>
> Do you have a sketch of how the accesses with be performed?
> ioctl, read, write.

No. Suggestions welcome. Looking at drivers/iio for inspiration.

Yours,
Linus Walleij

Re: [PATCH v2] gpio: add userspace ABI for GPIO line information

[Michael Welling]

On Fri, Feb 19, 2016 at 10:47:45AM +0100, Linus Walleij wrote:
> On Fri, Feb 19, 2016 at 10:46 AM, Michael Welling <mwelling@ieee.org> wrote:
> 
> > I am looking forward to see how the GPIOs will be modified from userspace.
> > The fact that single controllers can host hundreds of GPIOs is going to
> > make it an interesting problem.
> >
> > Do you have a sketch of how the accesses with be performed?
> > ioctl, read, write.
> 
> No. Suggestions welcome. Looking at drivers/iio for inspiration.
>

Well one thing I never liked about the iio userspace is the mixed API.
You have to use sysfs to setup buffered reads and character device for
the reads.

I suppose individual GPIOs could be configured by simply calling
specific ioctls passing the (offset, value) as the argument.

GPIO_EXPORT_LINE_IOCTL - Allocate the GPIO to userspace.
GPIO_SET_DIRECTION_IOCTL - Set GPIO direction.
GPIO_SET_VALUE_IOCTL - Set GPIO value.
GPIO_GET_VALUE_IOCTL - Get GPIO value.
GPIO_SET_ACTIVE_IOCTL - Set GPIO active high/low.
GPIO_SET_OPEN_DRAIN_IOCTL - Set GPIO open drain.
GPIO_SET_OPEN_SOURCE_IOCTL - Set GPIO open source.

Then all exported GPIOs could be accessible using read and write callbacks.

Handling interrupts may prove a little trickier.
The poll could be used on the character device but then there would have to
be a way to resolve which GPIO triggered the interrupt.

What do you think?

> Yours,
> Linus Walleij

Re: [PATCH v2] gpio: add userspace ABI for GPIO line information

[Linus Walleij]

On Fri, Feb 19, 2016 at 11:26 AM, Michael Welling <mwelling@ieee.org> wrote:
> On Fri, Feb 19, 2016 at 10:47:45AM +0100, Linus Walleij wrote:
>> On Fri, Feb 19, 2016 at 10:46 AM, Michael Welling <mwelling@ieee.org> wrote:
>>
>> > I am looking forward to see how the GPIOs will be modified from userspace.
>> > The fact that single controllers can host hundreds of GPIOs is going to
>> > make it an interesting problem.
>> >
>> > Do you have a sketch of how the accesses with be performed?
>> > ioctl, read, write.
>>
>> No. Suggestions welcome. Looking at drivers/iio for inspiration.
>>
>
> Well one thing I never liked about the iio userspace is the mixed API.
> You have to use sysfs to setup buffered reads and character device for
> the reads.
>
> I suppose individual GPIOs could be configured by simply calling
> specific ioctls passing the (offset, value) as the argument.

I think so too... I guess we can safely assume that only one
userspace process will need to access a GPIO chip at the same
time, and in case there'd be several of them, they have to
mediate access through a daemon (as is already done with other
things).

> GPIO_EXPORT_LINE_IOCTL - Allocate the GPIO to userspace.

I suspect this should take an argument with a list of
GPIOs from the start, so you can request several GPIO
lines, with flags. Requesting several lines makes it
possible to use devm_gpiod_get_array() and handle
a bunch of GPIOs similtaneously.

> GPIO_SET_DIRECTION_IOCTL - Set GPIO direction.
> GPIO_SET_VALUE_IOCTL - Set GPIO value.
> GPIO_GET_VALUE_IOCTL - Get GPIO value.

Those we probably need.

> GPIO_SET_ACTIVE_IOCTL - Set GPIO active high/low.
> GPIO_SET_OPEN_DRAIN_IOCTL - Set GPIO open drain.
> GPIO_SET_OPEN_SOURCE_IOCTL - Set GPIO open source.

Those need to be flags passed when exporting a line due
to the nature of the GPIO driver subsystem. Also I don't think
it makes sense to change any of these dynamically anyway,
you set this up when requesting a line.

> Then all exported GPIOs could be accessible using
> read and write callbacks.

The kernel will just take the gpio descriptors and manage
them, it'll "just work" I think.

> Handling interrupts may prove a little trickier.
> The poll could be used on the character device but then there would have to
> be a way to resolve which GPIO triggered the interrupt.

Here I lean toward the IIO event interface, that if you want to
monitor a GPIO line you should ask for an event file
descriptor and select() it waiting for events in userspace,
i.e. every such request comes with obtaining a new fd
and watching it.

Yours,
Linus Walleij

Re: [PATCH v2] gpio: add userspace ABI for GPIO line information

[Grant Likely]

On Fri, Feb 19, 2016 at 11:25 AM, Linus Walleij
<linus.walleij@linaro.org> wrote:
> On Fri, Feb 19, 2016 at 11:26 AM, Michael Welling <mwelling@ieee.org> wrote:
>> On Fri, Feb 19, 2016 at 10:47:45AM +0100, Linus Walleij wrote:
>>> On Fri, Feb 19, 2016 at 10:46 AM, Michael Welling <mwelling@ieee.org> wrote:
>>>
>>> > I am looking forward to see how the GPIOs will be modified from userspace.
>>> > The fact that single controllers can host hundreds of GPIOs is going to
>>> > make it an interesting problem.
>>> >
>>> > Do you have a sketch of how the accesses with be performed?
>>> > ioctl, read, write.
>>>
>>> No. Suggestions welcome. Looking at drivers/iio for inspiration.
>>>
>>
>> Well one thing I never liked about the iio userspace is the mixed API.
>> You have to use sysfs to setup buffered reads and character device for
>> the reads.
>>
>> I suppose individual GPIOs could be configured by simply calling
>> specific ioctls passing the (offset, value) as the argument.
>
> I think so too... I guess we can safely assume that only one
> userspace process will need to access a GPIO chip at the same
> time, and in case there'd be several of them, they have to
> mediate access through a daemon (as is already done with other
> things).
>
>> GPIO_EXPORT_LINE_IOCTL - Allocate the GPIO to userspace.
>
> I suspect this should take an argument with a list of
> GPIOs from the start, so you can request several GPIO
> lines, with flags. Requesting several lines makes it
> possible to use devm_gpiod_get_array() and handle
> a bunch of GPIOs similtaneously.
>
>> GPIO_SET_DIRECTION_IOCTL - Set GPIO direction.
>> GPIO_SET_VALUE_IOCTL - Set GPIO value.
>> GPIO_GET_VALUE_IOCTL - Get GPIO value.
>
> Those we probably need.
>
>> GPIO_SET_ACTIVE_IOCTL - Set GPIO active high/low.
>> GPIO_SET_OPEN_DRAIN_IOCTL - Set GPIO open drain.
>> GPIO_SET_OPEN_SOURCE_IOCTL - Set GPIO open source.
>
> Those need to be flags passed when exporting a line due
> to the nature of the GPIO driver subsystem. Also I don't think
> it makes sense to change any of these dynamically anyway,
> you set this up when requesting a line.
>
>> Then all exported GPIOs could be accessible using
>> read and write callbacks.
>
> The kernel will just take the gpio descriptors and manage
> them, it'll "just work" I think.
>
>> Handling interrupts may prove a little trickier.
>> The poll could be used on the character device but then there would have to
>> be a way to resolve which GPIO triggered the interrupt.
>
> Here I lean toward the IIO event interface, that if you want to
> monitor a GPIO line you should ask for an event file
> descriptor and select() it waiting for events in userspace,
> i.e. every such request comes with obtaining a new fd
> and watching it.

That seems overly heavyweight. It would be reasonable to select on a
single file descriptor, and then on read it will also return a bitmap
of which pin(s) actually raised the event.

g.

Re: [PATCH v2] gpio: add userspace ABI for GPIO line information

[jic23]

On 19.02.2016 11:25, Linus Walleij wrote:
> On Fri, Feb 19, 2016 at 11:26 AM, Michael Welling <mwelling@ieee.org> 
> wrote:
>> On Fri, Feb 19, 2016 at 10:47:45AM +0100, Linus Walleij wrote:
>>> On Fri, Feb 19, 2016 at 10:46 AM, Michael Welling <mwelling@ieee.org> 
>>> wrote:
>>> 
>>> > I am looking forward to see how the GPIOs will be modified from userspace.
>>> > The fact that single controllers can host hundreds of GPIOs is going to
>>> > make it an interesting problem.
>>> >
>>> > Do you have a sketch of how the accesses with be performed?
>>> > ioctl, read, write.
>>> 
>>> No. Suggestions welcome. Looking at drivers/iio for inspiration.
>>> 
>> 
>> Well one thing I never liked about the iio userspace is the mixed API.
>> You have to use sysfs to setup buffered reads and character device for
>> the reads.
>> 
>> I suppose individual GPIOs could be configured by simply calling
>> specific ioctls passing the (offset, value) as the argument.
> 
> I think so too... I guess we can safely assume that only one
> userspace process will need to access a GPIO chip at the same
> time, and in case there'd be several of them, they have to
> mediate access through a daemon (as is already done with other
> things).
> 
>> GPIO_EXPORT_LINE_IOCTL - Allocate the GPIO to userspace.
> 
> I suspect this should take an argument with a list of
> GPIOs from the start, so you can request several GPIO
> lines, with flags. Requesting several lines makes it
> possible to use devm_gpiod_get_array() and handle
> a bunch of GPIOs similtaneously.
> 
>> GPIO_SET_DIRECTION_IOCTL - Set GPIO direction.
>> GPIO_SET_VALUE_IOCTL - Set GPIO value.
>> GPIO_GET_VALUE_IOCTL - Get GPIO value.
> 
> Those we probably need.
> 
>> GPIO_SET_ACTIVE_IOCTL - Set GPIO active high/low.
>> GPIO_SET_OPEN_DRAIN_IOCTL - Set GPIO open drain.
>> GPIO_SET_OPEN_SOURCE_IOCTL - Set GPIO open source.
> 
> Those need to be flags passed when exporting a line due
> to the nature of the GPIO driver subsystem. Also I don't think
> it makes sense to change any of these dynamically anyway,
> you set this up when requesting a line.
> 
>> Then all exported GPIOs could be accessible using
>> read and write callbacks.
> 
> The kernel will just take the gpio descriptors and manage
> them, it'll "just work" I think.
> 
>> Handling interrupts may prove a little trickier.
>> The poll could be used on the character device but then there would 
>> have to
>> be a way to resolve which GPIO triggered the interrupt.
> 
> Here I lean toward the IIO event interface, that if you want to
> monitor a GPIO line you should ask for an event file
> descriptor and select() it waiting for events in userspace,
> i.e. every such request comes with obtaining a new fd
> and watching it.
> 
On events, it might be worth doing it a bit more input evdev
like and allowing mor than one consumer.

Do we need to describe to userspace what GPIOs are available?
Right now I don't have a board to hand to see what info is
already available on that front.  Strikes me as that side of
things may be more complex than the interface to actually get hole
of them.  It's this side of things that makes IOCTLs on highly
varied devices a pain (any why we ended up with the split interface
in IIO which is sort of an input / hwmon hybrid).

Jonathan

> Yours,
> Linus Walleij

Re: [PATCH v2] gpio: add userspace ABI for GPIO line information

[Linus Walleij]

On Fri, Feb 19, 2016 at 12:35 PM,  <jic23@jic23.retrosnub.co.uk> wrote:
> [Me]
>> Here I lean toward the IIO event interface, that if you want to
>> monitor a GPIO line you should ask for an event file
>> descriptor and select() it waiting for events in userspace,
>> i.e. every such request comes with obtaining a new fd
>> and watching it.
>>
> On events, it might be worth doing it a bit more input evdev
> like and allowing mor than one consumer.

Hm. I'm not familiar with how that works... I guess I just
have to go and read the code.

> Do we need to describe to userspace what GPIOs are available?
> Right now I don't have a board to hand to see what info is
> already available on that front.  Strikes me as that side of
> things may be more complex than the interface to actually get hole
> of them.  It's this side of things that makes IOCTLs on highly
> varied devices a pain (any why we ended up with the split interface
> in IIO which is sort of an input / hwmon hybrid).

So the usecase that has been around the recent months is
basically Arduino-type usecases, where you want a Linux
SoC+board to be used for clever electronics prototyping
(Internet of Things-yada yada thingofabobs).

Those are by definition one-off kind of things, and people
want to go around having to implement kernelspace stuff
for their one-offs.

Another typical example is industrial
automation, PLCs. Those currently mmap() their GPIO
address range to userspace and hammers them from
there, because they think Linux GPIO suck and they
rather break down the kernelspace/userspace barrier
than try to fix it. (Whether this stance come from
laziness, ignorance or plain "not my problem" attitude,
I don't know.)

Industrial automation with relays and shutter and
valves and whatnot are probably better off in userspace
than in kernelspace, a bunch of GPIO onebit reading
and writing drivers in the kernel is not gonna be helpful.
If it's something more complex like a sensor they should
use IIO, if it's a LED then they should use that
subsystem etc. But for all these binary things that
are really dumb analog components, like relays doing
something misc.

A typical case is a PLC or lab board such as
BeagleBone or 96board, where there is a number of
GPIO lines, that all come out on the same identical
header on the board (96board has this). Then you want
to put an accessory on this header and access it from
userspace. But you want it to work the same no matter
whether it is SoC A or SoC B.

So in order to satisfy that usecase, you need to look up
the GPIO by name. And that mechanism is now in
place, just that we need some DT bindings or board
data to name the lines (it can currently be done using
the char *names[] array in struct gpio_chip).

Yours,
Linus Walleij

Re: [PATCH v2] gpio: add userspace ABI for GPIO line information

[Linus Walleij]

On Fri, Feb 19, 2016 at 12:29 PM, Grant Likely <grant.likely@linaro.org> wrote:
> [Me]
>> Here I lean toward the IIO event interface, that if you want to
>> monitor a GPIO line you should ask for an event file
>> descriptor and select() it waiting for events in userspace,
>> i.e. every such request comes with obtaining a new fd
>> and watching it.
>
> That seems overly heavyweight. It would be reasonable to select on a
> single file descriptor, and then on read it will also return a bitmap
> of which pin(s) actually raised the event.

Makes sense. But since GPIO controllers can supply hundreds of
GPIO lines we would however have make sure that this bitmap is
dynamic in size, a u32 or u64 won't work. So we should just pass the
offset number back with each event.

Yours,
Linus Walleij

Re: [PATCH v2] gpio: add userspace ABI for GPIO line information

[Markus Pargmann]

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

Hi,

On Friday 19 February 2016 12:51:48 Linus Walleij wrote:
> On Fri, Feb 19, 2016 at 12:35 PM,  <jic23@jic23.retrosnub.co.uk> wrote:
> > [Me]
> >> Here I lean toward the IIO event interface, that if you want to
> >> monitor a GPIO line you should ask for an event file
> >> descriptor and select() it waiting for events in userspace,
> >> i.e. every such request comes with obtaining a new fd
> >> and watching it.
> >>
> > On events, it might be worth doing it a bit more input evdev
> > like and allowing mor than one consumer.
> 
> Hm. I'm not familiar with how that works... I guess I just
> have to go and read the code.
> 
> > Do we need to describe to userspace what GPIOs are available?
> > Right now I don't have a board to hand to see what info is
> > already available on that front.  Strikes me as that side of
> > things may be more complex than the interface to actually get hole
> > of them.  It's this side of things that makes IOCTLs on highly
> > varied devices a pain (any why we ended up with the split interface
> > in IIO which is sort of an input / hwmon hybrid).
> 
> So the usecase that has been around the recent months is
> basically Arduino-type usecases, where you want a Linux
> SoC+board to be used for clever electronics prototyping
> (Internet of Things-yada yada thingofabobs).
> 
> Those are by definition one-off kind of things, and people
> want to go around having to implement kernelspace stuff
> for their one-offs.
> 
> Another typical example is industrial
> automation, PLCs. Those currently mmap() their GPIO
> address range to userspace and hammers them from
> there, because they think Linux GPIO suck and they
> rather break down the kernelspace/userspace barrier
> than try to fix it. (Whether this stance come from
> laziness, ignorance or plain "not my problem" attitude,
> I don't know.)
> 
> Industrial automation with relays and shutter and
> valves and whatnot are probably better off in userspace
> than in kernelspace, a bunch of GPIO onebit reading
> and writing drivers in the kernel is not gonna be helpful.
> If it's something more complex like a sensor they should
> use IIO, if it's a LED then they should use that
> subsystem etc. But for all these binary things that
> are really dumb analog components, like relays doing
> something misc.
> 
> A typical case is a PLC or lab board such as
> BeagleBone or 96board, where there is a number of
> GPIO lines, that all come out on the same identical
> header on the board (96board has this). Then you want
> to put an accessory on this header and access it from
> userspace. But you want it to work the same no matter
> whether it is SoC A or SoC B.
> 
> So in order to satisfy that usecase, you need to look up
> the GPIO by name. And that mechanism is now in
> place, just that we need some DT bindings or board
> data to name the lines (it can currently be done using
> the char *names[] array in struct gpio_chip).

The gpio-hogging DT bindings still seem perfectly fine for me:

	qe_pio_a: gpio-controller@1400 {
		compatible = "fsl,qe-pario-bank-a", "fsl,qe-pario-bank";
		reg = <0x1400 0x18>;
		gpio-controller;
		#gpio-cells = <2>;

		line_b {
			gpio-hog;
			gpios = <6 0>;
			output-low;
			line-name = "foo-bar-gpio";
		};
	};

This could be something like this without hogging:
		...
		line_b {
			gpios = <6 0>;
			line-name = "foo-bar-gpio";
		};

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 v2] gpio: add userspace ABI for GPIO line information

[Linus Walleij]

On Sun, Feb 21, 2016 at 2:13 PM, Markus Pargmann <mpa@pengutronix.de> wrote:

> The gpio-hogging DT bindings still seem perfectly fine for me:
>
>         qe_pio_a: gpio-controller@1400 {
>                 compatible = "fsl,qe-pario-bank-a", "fsl,qe-pario-bank";
>                 reg = <0x1400 0x18>;
>                 gpio-controller;
>                 #gpio-cells = <2>;
>
>                 line_b {
>                         gpio-hog;
>                         gpios = <6 0>;
>                         output-low;
>                         line-name = "foo-bar-gpio";
>                 };
>         };
>
> This could be something like this without hogging:
>                 ...
>                 line_b {
>                         gpios = <6 0>;
>                         line-name = "foo-bar-gpio";
>                 };

I agree. Didn't you even make a patch for this that we didn't merge
because we didn't have a use case?

(I have a memory problem, admittedly, too much people and patches
flying around ...)

If you rebase the patch that make it possible to simply name lines
without hogging them, I'm certainly game for merging it.

Enclosing a lsgpio sample output gives extra points.

Yours,
Linus Walleij

Re: [PATCH v2] gpio: add userspace ABI for GPIO line information

[Markus Pargmann]

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

Hi,

On Monday, February 15, 2016 02:20:35 PM Linus Walleij wrote:
> This adds a GPIO line ABI for getting name, label and a few select
> flags from the kernel.
> 
> This hides the kernel internals and only tells userspace what it
> may need to know: the different in-kernel consumers are masked
> behind the flag "kernel" and that is all userspace needs to know.
> 
> However electric characteristics like active low, open drain etc
> are reflected to userspace, as this is important information.
> 
> We provide information on all lines on all chips, later on we will
> likely add a flag for the chardev consumer so we can filter and
> display only the lines userspace actually uses in e.g. lsgpio,
> but then we first need an ABI for userspace to grab and use
> (get/set/select direction) a GPIO line.
> 
> Sample output from "lsgpio" on ux500:
> 
> GPIO chip: gpiochip7, "8011e000.gpio", 32 GPIO lines
>         line 0: unnamed unlabeled
>         line 1: unnamed unlabeled
> (...)
>         line 25: unnamed "SFH7741 Proximity Sensor" [kernel output open-drain]
>         line 26: unnamed unlabeled
> (...)
> 
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v2:
> - Fix unitialized lineinfo variables, especially flags,
>   on both the userspace and kernelspace side.
> - Pretty print.
> ---
>  drivers/gpio/gpiolib.c    | 51 ++++++++++++++++++++++++--
>  include/uapi/linux/gpio.h | 26 ++++++++++++++
>  tools/gpio/gpio-utils.h   |  2 ++
>  tools/gpio/lsgpio.c       | 91 ++++++++++++++++++++++++++++++++++++++++-------
>  4 files changed, 156 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index a022e9249f69..a38265dec794 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -331,14 +331,15 @@ static long gpio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  	struct gpio_device *gdev = filp->private_data;
>  	struct gpio_chip *chip = gdev->chip;
>  	int __user *ip = (int __user *)arg;
> -	struct gpiochip_info chipinfo;
>  
>  	/* We fail any subsequent ioctl():s when the chip is gone */
>  	if (!chip)
>  		return -ENODEV;
>  
> +	/* Fill in the struct and pass to userspace */
>  	if (cmd == GPIO_GET_CHIPINFO_IOCTL) {
> -		/* Fill in the struct and pass to userspace */
> +		struct gpiochip_info chipinfo;
> +
>  		strncpy(chipinfo.name, dev_name(&gdev->dev),
>  			sizeof(chipinfo.name));
>  		chipinfo.name[sizeof(chipinfo.name)-1] = '\0';
> @@ -349,6 +350,52 @@ static long gpio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  		if (copy_to_user(ip, &chipinfo, sizeof(chipinfo)))
>  			return -EFAULT;
>  		return 0;
> +	} else if (cmd == GPIO_GET_LINEINFO_IOCTL) {
> +		struct gpioline_info lineinfo;
> +		struct gpio_desc *desc;
> +
> +		if (copy_from_user(&lineinfo, ip, sizeof(lineinfo)))
> +			return -EFAULT;
> +		if (lineinfo.line_offset > gdev->ngpio)
> +			return -EINVAL;
> +
> +		desc = &gdev->descs[lineinfo.line_offset];
> +		if (desc->name) {
> +			strncpy(lineinfo.name, desc->name,
> +				sizeof(lineinfo.name));
> +			lineinfo.name[sizeof(lineinfo.name)-1] = '\0';
> +		} else {
> +			lineinfo.name[0] = '\0';
> +		}
> +		if (desc->label) {
> +			strncpy(lineinfo.label, desc->label,
> +				sizeof(lineinfo.label));
> +			lineinfo.label[sizeof(lineinfo.label)-1] = '\0';
> +		} else {
> +			lineinfo.label[0] = '\0';
> +		}
> +
> +		/*
> +		 * Userspace only need to know that the kernel is using
> +		 * this GPIO so it can't use it.
> +		 */
> +		lineinfo.flags = 0;
> +		if (desc->flags & (FLAG_REQUESTED | FLAG_IS_HOGGED |
> +				   FLAG_USED_AS_IRQ | FLAG_EXPORT |
> +				   FLAG_SYSFS))
> +			lineinfo.flags |= GPIOLINE_FLAG_KERNEL;
> +		if (desc->flags & FLAG_IS_OUT)
> +			lineinfo.flags |= GPIOLINE_FLAG_IS_OUT;
> +		if (desc->flags & FLAG_ACTIVE_LOW)
> +			lineinfo.flags |= GPIOLINE_FLAG_ACTIVE_LOW;
> +		if (desc->flags & FLAG_OPEN_DRAIN)
> +			lineinfo.flags |= GPIOLINE_FLAG_OPEN_DRAIN;
> +		if (desc->flags & FLAG_OPEN_SOURCE)
> +			lineinfo.flags |= GPIOLINE_FLAG_OPEN_SOURCE;

I just noticed while working on DT name parsing that these flags are bit
numbers and not the real bits. So we have to use test_bit() here. I will
send a patch later.

Best Regards,

Markus

> +
> +		if (copy_to_user(ip, &lineinfo, sizeof(lineinfo)))
> +			return -EFAULT;
> +		return 0;
>  	}
>  	return -EINVAL;
>  }
> diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h
> index 3f93e1bcd3dd..416ce47f2291 100644
> --- a/include/uapi/linux/gpio.h
> +++ b/include/uapi/linux/gpio.h
> @@ -25,6 +25,32 @@ struct gpiochip_info {
>  	__u32 lines;
>  };
>  
> +/* Line is in use by the kernel */
> +#define GPIOLINE_FLAG_KERNEL		(1UL << 0)
> +#define GPIOLINE_FLAG_IS_OUT		(1UL << 1)
> +#define GPIOLINE_FLAG_ACTIVE_LOW	(1UL << 2)
> +#define GPIOLINE_FLAG_OPEN_DRAIN	(1UL << 3)
> +#define GPIOLINE_FLAG_OPEN_SOURCE	(1UL << 4)
> +
> +/**
> + * struct gpioline_info - Information about a certain GPIO line
> + * @line_offset: the local offset on this GPIO device, fill in when
> + * requesting information from the kernel
> + * @flags: various flags for this line
> + * @name: the name of this GPIO line
> + * @label: a functional name for this GPIO line
> + * @kernel: this GPIO is in use by the kernel
> + * @out: this GPIO is an output line (false means it is an input)
> + * @active_low: this GPIO is active low
> + */
> +struct gpioline_info {
> +	__u32 line_offset;
> +	__u32 flags;
> +	char name[32];
> +	char label[32];
> +};
> +
>  #define GPIO_GET_CHIPINFO_IOCTL _IOR('o', 0x01, struct gpiochip_info)
> +#define GPIO_GET_LINEINFO_IOCTL _IOWR('o', 0x02, struct gpioline_info)
>  
>  #endif /* _UAPI_GPIO_H_ */
> diff --git a/tools/gpio/gpio-utils.h b/tools/gpio/gpio-utils.h
> index b18209a45ad3..5f57133b8c04 100644
> --- a/tools/gpio/gpio-utils.h
> +++ b/tools/gpio/gpio-utils.h
> @@ -16,6 +16,8 @@
>  
>  #include <string.h>
>  
> +#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
> +
>  static inline int check_prefix(const char *str, const char *prefix)
>  {
>  	return strlen(str) > strlen(prefix) &&
> diff --git a/tools/gpio/lsgpio.c b/tools/gpio/lsgpio.c
> index 692233f561fb..5535ce81f8f7 100644
> --- a/tools/gpio/lsgpio.c
> +++ b/tools/gpio/lsgpio.c
> @@ -26,12 +26,56 @@
>  
>  #include "gpio-utils.h"
>  
> +struct gpio_flag {
> +	char *name;
> +	unsigned long mask;
> +};
> +
> +struct gpio_flag flagnames[] = {
> +	{
> +		.name = "kernel",
> +		.mask = GPIOLINE_FLAG_KERNEL,
> +	},
> +	{
> +		.name = "output",
> +		.mask = GPIOLINE_FLAG_IS_OUT,
> +	},
> +	{
> +		.name = "active-low",
> +		.mask = GPIOLINE_FLAG_ACTIVE_LOW,
> +	},
> +	{
> +		.name = "open-drain",
> +		.mask = GPIOLINE_FLAG_OPEN_DRAIN,
> +	},
> +	{
> +		.name = "open-source",
> +		.mask = GPIOLINE_FLAG_OPEN_SOURCE,
> +	},
> +};
> +
> +void print_flags(unsigned long flags)
> +{
> +	int i;
> +	int printed = 0;
> +
> +	for (i = 0; i < ARRAY_SIZE(flagnames); i++) {
> +		if (flags & flagnames[i].mask) {
> +			if (printed)
> +				fprintf(stdout, " ");
> +			fprintf(stdout, "%s", flagnames[i].name);
> +			printed++;
> +		}
> +	}
> +}
> +
>  int list_device(const char *device_name)
>  {
>  	struct gpiochip_info cinfo;
>  	char *chrdev_name;
>  	int fd;
>  	int ret;
> +	int i;
>  
>  	ret = asprintf(&chrdev_name, "/dev/%s", device_name);
>  	if (ret < 0)
> @@ -41,32 +85,55 @@ int list_device(const char *device_name)
>  	if (fd == -1) {
>  		ret = -errno;
>  		fprintf(stderr, "Failed to open %s\n", chrdev_name);
> -		goto free_chrdev_name;
> +		goto exit_close_error;
>  	}
>  
>  	/* 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;
> +		perror("Failed to issue CHIPINFO IOCTL\n");
> +		goto exit_close_error;
>  	}
>  	fprintf(stdout, "GPIO chip: %s, \"%s\", %u GPIO lines\n",
>  		cinfo.name, cinfo.label, cinfo.lines);
>  
> -	if (close(fd) == -1)  {
> -		ret = -errno;
> -		goto free_chrdev_name;
> +	/* Loop over the lines and print info */
> +	for (i = 0; i < cinfo.lines; i++) {
> +		struct gpioline_info linfo;
> +
> +		memset(&linfo, 0, sizeof(linfo));
> +		linfo.line_offset = i;
> +
> +		ret = ioctl(fd, GPIO_GET_LINEINFO_IOCTL, &linfo);
> +		if (ret == -1) {
> +			ret = -errno;
> +			perror("Failed to issue LINEINFO IOCTL\n");
> +			goto exit_close_error;
> +		}
> +		fprintf(stdout, "\tline %d:", linfo.line_offset);
> +		if (linfo.name[0])
> +			fprintf(stdout, " %s", linfo.name);
> +		else
> +			fprintf(stdout, " unnamed");
> +		if (linfo.label[0])
> +			fprintf(stdout, " \"%s\"", linfo.label);
> +		else
> +			fprintf(stdout, " unlabeled");
> +		if (linfo.flags) {
> +			fprintf(stdout, " [");
> +			print_flags(linfo.flags);
> +			fprintf(stdout, "]");
> +		}
> +		fprintf(stdout, "\n");
> +
>  	}
>  
> -free_chrdev_name:
> +exit_close_error:
> +	if (close(fd) == -1)
> +		perror("Failed to close GPIO character device file");
>  	free(chrdev_name);
> -
>  	return ret;
> -
>  }
>  
>  void print_usage(void)
> 

-- 
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 v2] gpio: add userspace ABI for GPIO line information

[Linus Walleij]

On Mon, Feb 22, 2016 at 1:05 PM, Markus Pargmann <mpa@pengutronix.de> wrote:

>> +             /*
>> +              * Userspace only need to know that the kernel is using
>> +              * this GPIO so it can't use it.
>> +              */
>> +             lineinfo.flags = 0;
>> +             if (desc->flags & (FLAG_REQUESTED | FLAG_IS_HOGGED |
>> +                                FLAG_USED_AS_IRQ | FLAG_EXPORT |
>> +                                FLAG_SYSFS))
>> +                     lineinfo.flags |= GPIOLINE_FLAG_KERNEL;
>> +             if (desc->flags & FLAG_IS_OUT)
>> +                     lineinfo.flags |= GPIOLINE_FLAG_IS_OUT;
>> +             if (desc->flags & FLAG_ACTIVE_LOW)
>> +                     lineinfo.flags |= GPIOLINE_FLAG_ACTIVE_LOW;
>> +             if (desc->flags & FLAG_OPEN_DRAIN)
>> +                     lineinfo.flags |= GPIOLINE_FLAG_OPEN_DRAIN;
>> +             if (desc->flags & FLAG_OPEN_SOURCE)
>> +                     lineinfo.flags |= GPIOLINE_FLAG_OPEN_SOURCE;
>
> I just noticed while working on DT name parsing that these flags are bit
> numbers and not the real bits. So we have to use test_bit() here. I will
> send a patch later.

Ooops don't worry I will fix it and send a v3. If there are no
further comments I will then  APPLY it so we have this in
place for v4.6.

Yours,
Linus Walleij