Message ID | 1455542435-3182-1-git-send-email-linus.walleij@linaro.org |
---|---|
State | Superseded |
Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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)
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(-) -- 2.4.3 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html