| Message ID | 20211201072902.127542-1-joel@jms.id.au |
|---|---|
| State | New |
| Headers | show |
| Series | [libgpiod] gpioget: Add --line-name to lookup GPIO line | expand |
On Wed, 1 Dec 2021 at 08:29, Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > On Wed, Dec 1, 2021 at 8:29 AM Joel Stanley <joel@jms.id.au> wrote: > > > > Systems provide line names to make using GPIOs easier for userspace. Use > > this feature to make the tools user friendly by adding the ability to > > show the state of a named line. > > > > $ gpioget --line-name power-chassis-good > > 1 > > > > $ gpioget -L pcieslot-power > > 0 > I'm not very convinced to be honest. It's not like "gpioget gpiochip0 > `gpiofind gpiochip0 line-name`" requires much more typing than > "gpioget gpiochip --line-name=name". I'm taking on feedback from people working in our labs, and implementing userspace applications. We've been building BMCs with mainline Linux for about six years now, and it's been a long road re-training them from "back in the day we just did devmem <this> <that>" and "why can't we just do cat /sys/class/gpio/gpio305/value", and now "why does the level of the GPIO change back after I run the command?". This usability improvement is one more step towards them using and being happy with the "new world" of the gpiod API. Once we settle on a good API here, I plan on submitting a version of gpioget/gpioset added to busybox. > There are also other questions: > this uses getopt and only allows to specify a single line name. What > if we want to specify more lines like with offsets? Even if you allow > multiple names, getopt() doesn't guarantee ordering of arguments. If you're happy with the concept I'm happy to iterate on the implementation. Yes, it only allows a single line name. That tends to be how the tool is used, both from the command line and in scripts. Can you give me an example of your proposed command line API, so I can understand what you're suggesting here? Cheers, Joel
Hi Joel, > Systems provide line names to make using GPIOs easier for userspace. > Use this feature to make the tools user friendly by adding the ability to > show the state of a named line. > > $ gpioget --line-name power-chassis-good > 1 > > $ gpioget -L pcieslot-power > 0 Awesome! As someone who has had wasted hours of debugging the wrong GPIO lines because of incorrect chip/offset calculations, +100 from me. (or was that +101? I'll have to check the schematic again...) Cheers, Jeremy
On Wed, Dec 01, 2021 at 08:29:47PM PST, Joel Stanley wrote: >On Wed, 1 Dec 2021 at 08:29, Bartosz Golaszewski <brgl@bgdev.pl> wrote: >> >> On Wed, Dec 1, 2021 at 8:29 AM Joel Stanley <joel@jms.id.au> wrote: >> > >> > Systems provide line names to make using GPIOs easier for userspace. Use >> > this feature to make the tools user friendly by adding the ability to >> > show the state of a named line. >> > >> > $ gpioget --line-name power-chassis-good >> > 1 >> > >> > $ gpioget -L pcieslot-power >> > 0 > >> I'm not very convinced to be honest. It's not like "gpioget gpiochip0 >> `gpiofind gpiochip0 line-name`" requires much more typing than >> "gpioget gpiochip --line-name=name". > >I'm taking on feedback from people working in our labs, and >implementing userspace applications. We've been building BMCs with >mainline Linux for about six years now, and it's been a long road >re-training them from "back in the day we just did devmem <this> ><that>" and "why can't we just do cat /sys/class/gpio/gpio305/value", >and now "why does the level of the GPIO change back after I run the >command?". > >This usability improvement is one more step towards them using and >being happy with the "new world" of the gpiod API. > >Once we settle on a good API here, I plan on submitting a version of >gpioget/gpioset added to busybox. > >> There are also other questions: >> this uses getopt and only allows to specify a single line name. What >> if we want to specify more lines like with offsets? Even if you allow >> multiple names, getopt() doesn't guarantee ordering of arguments. > >If you're happy with the concept I'm happy to iterate on the implementation. > >Yes, it only allows a single line name. That tends to be how the tool >is used, both from the command line and in scripts. > >Can you give me an example of your proposed command line API, so I can >understand what you're suggesting here? > My two cents: like Jeremy, I would very much welcome the ability to specify GPIOs by name instead of number, but the one-line-only limitation does seem unfortunate. How about making a command-line flag that just means "line-specifier arguments should be interpreted as names instead of numbers"? So you could do: $ gpioget --by-name chassis-intrusion cpu1-prochot 0 1 $ gpioset --by-name led-green=1 led-red=0 Zev
On Fri, 3 Dec 2021, at 14:20, Zev Weiss wrote: > On Wed, Dec 01, 2021 at 08:29:47PM PST, Joel Stanley wrote: >>On Wed, 1 Dec 2021 at 08:29, Bartosz Golaszewski <brgl@bgdev.pl> wrote: >>> >>> On Wed, Dec 1, 2021 at 8:29 AM Joel Stanley <joel@jms.id.au> wrote: >>> > >>> > Systems provide line names to make using GPIOs easier for userspace. Use >>> > this feature to make the tools user friendly by adding the ability to >>> > show the state of a named line. >>> > >>> > $ gpioget --line-name power-chassis-good >>> > 1 >>> > >>> > $ gpioget -L pcieslot-power >>> > 0 >> >>> I'm not very convinced to be honest. It's not like "gpioget gpiochip0 >>> `gpiofind gpiochip0 line-name`" requires much more typing than >>> "gpioget gpiochip --line-name=name". >> >>I'm taking on feedback from people working in our labs, and >>implementing userspace applications. We've been building BMCs with >>mainline Linux for about six years now, and it's been a long road >>re-training them from "back in the day we just did devmem <this> >><that>" and "why can't we just do cat /sys/class/gpio/gpio305/value", >>and now "why does the level of the GPIO change back after I run the >>command?". >> >>This usability improvement is one more step towards them using and >>being happy with the "new world" of the gpiod API. >> >>Once we settle on a good API here, I plan on submitting a version of >>gpioget/gpioset added to busybox. >> >>> There are also other questions: >>> this uses getopt and only allows to specify a single line name. What >>> if we want to specify more lines like with offsets? Even if you allow >>> multiple names, getopt() doesn't guarantee ordering of arguments. >> >>If you're happy with the concept I'm happy to iterate on the implementation. >> >>Yes, it only allows a single line name. That tends to be how the tool >>is used, both from the command line and in scripts. >> >>Can you give me an example of your proposed command line API, so I can >>understand what you're suggesting here? >> > > My two cents: like Jeremy, I would very much welcome the ability to > specify GPIOs by name instead of number, but the one-line-only > limitation does seem unfortunate. How about making a command-line flag > that just means "line-specifier arguments should be interpreted as names > instead of numbers"? > > So you could do: > > $ gpioget --by-name chassis-intrusion cpu1-prochot > 0 1 > > $ gpioset --by-name led-green=1 led-red=0 > I came up with this approach as well (independently, just thinking about Joel's patch). I think it has good ergonomics. I hadn't figured out how we should interpret the arguments as line index vs line name, but your --by-name option solves that. I like it a lot. Andrew
On Fri, Dec 3, 2021 at 7:20 AM Andrew Jeffery <andrew@aj.id.au> wrote: > > > > On Fri, 3 Dec 2021, at 14:20, Zev Weiss wrote: > > On Wed, Dec 01, 2021 at 08:29:47PM PST, Joel Stanley wrote: > >>On Wed, 1 Dec 2021 at 08:29, Bartosz Golaszewski <brgl@bgdev.pl> wrote: > >>> > >>> On Wed, Dec 1, 2021 at 8:29 AM Joel Stanley <joel@jms.id.au> wrote: > >>> > > >>> > Systems provide line names to make using GPIOs easier for userspace. Use > >>> > this feature to make the tools user friendly by adding the ability to > >>> > show the state of a named line. > >>> > > >>> > $ gpioget --line-name power-chassis-good > >>> > 1 > >>> > > >>> > $ gpioget -L pcieslot-power > >>> > 0 > >> > >>> I'm not very convinced to be honest. It's not like "gpioget gpiochip0 > >>> `gpiofind gpiochip0 line-name`" requires much more typing than > >>> "gpioget gpiochip --line-name=name". > >> > >>I'm taking on feedback from people working in our labs, and > >>implementing userspace applications. We've been building BMCs with > >>mainline Linux for about six years now, and it's been a long road > >>re-training them from "back in the day we just did devmem <this> > >><that>" and "why can't we just do cat /sys/class/gpio/gpio305/value", > >>and now "why does the level of the GPIO change back after I run the > >>command?". > >> > >>This usability improvement is one more step towards them using and > >>being happy with the "new world" of the gpiod API. > >> > >>Once we settle on a good API here, I plan on submitting a version of > >>gpioget/gpioset added to busybox. > >> > >>> There are also other questions: > >>> this uses getopt and only allows to specify a single line name. What > >>> if we want to specify more lines like with offsets? Even if you allow > >>> multiple names, getopt() doesn't guarantee ordering of arguments. > >> > >>If you're happy with the concept I'm happy to iterate on the implementation. > >> Alright, it looks like this has passed by popular vote. > >>Yes, it only allows a single line name. That tends to be how the tool > >>is used, both from the command line and in scripts. > >> > >>Can you give me an example of your proposed command line API, so I can > >>understand what you're suggesting here? > >> > > > > My two cents: like Jeremy, I would very much welcome the ability to > > specify GPIOs by name instead of number, but the one-line-only > > limitation does seem unfortunate. How about making a command-line flag > > that just means "line-specifier arguments should be interpreted as names > > instead of numbers"? > > > > So you could do: > > > > $ gpioget --by-name chassis-intrusion cpu1-prochot > > 0 1 > > > > $ gpioset --by-name led-green=1 led-red=0 I like this more - that way we either allow offsets or names. Please make sure corner cases are covered. If you can add this to other tools too, I'm fine with that, but put the name lookup code into tools-common.c please. I would also like to see bats test cases covering this. Thanks! Bart > > > > I came up with this approach as well (independently, just thinking > about Joel's patch). I think it has good ergonomics. I hadn't figured > out how we should interpret the arguments as line index vs line name, > but your --by-name option solves that. I like it a lot. > > Andrew
diff --git a/tools/gpioget.c b/tools/gpioget.c index 51cecb6a18a9..cd27320b7f2b 100644 --- a/tools/gpioget.c +++ b/tools/gpioget.c @@ -6,6 +6,7 @@ #include <limits.h> #include <stdio.h> #include <string.h> +#include <errno.h> #include "tools-common.h" @@ -18,7 +19,7 @@ static const struct option longopts[] = { { GETOPT_NULL_LONGOPT }, }; -static const char *const shortopts = "+hvlnB:"; +static const char *const shortopts = "+hvlnB:L:"; static void print_help(void) { @@ -34,6 +35,7 @@ static void print_help(void) printf(" -n, --dir-as-is:\tdon't force-reconfigure line direction\n"); printf(" -B, --bias=[as-is|disable|pull-down|pull-up] (defaults to 'as-is'):\n"); printf(" set the line bias\n"); + printf(" -L, --line-name:\tUse this GPIO line (instead of chip name and offset)\n"); printf("\n"); print_bias_help(); } @@ -47,6 +49,7 @@ int main(int argc, char **argv) struct gpiod_line_bulk *lines; struct gpiod_chip *chip; char *device, *end; + char *name = NULL; for (;;) { optc = getopt_long(argc, argv, shortopts, longopts, &opti); @@ -69,8 +72,13 @@ int main(int argc, char **argv) case 'B': flags |= bias_flags(optarg); break; + case 'L': + name = optarg; + num_lines = 1; + break; case '?': die("try %s --help", get_progname()); + break; default: abort(); } @@ -79,30 +87,64 @@ int main(int argc, char **argv) argc -= optind; argv += optind; - if (argc < 1) - die("gpiochip must be specified"); - - if (argc < 2) - die("at least one GPIO line offset must be specified"); - - device = argv[0]; - num_lines = argc - 1; - - values = malloc(sizeof(*values) * num_lines); - offsets = malloc(sizeof(*offsets) * num_lines); - if (!values || !offsets) - die("out of memory"); + if (name) { + struct dirent **entries; + unsigned int num_chips; + int offset, n; + + n = scandir("/dev/", &entries, chip_dir_filter, alphasort); + if (n < 0) + die_perror("unable to scan /dev"); + num_chips = n; + + values = malloc(sizeof(*values) * 1); + offsets = malloc(sizeof(*offsets) * 1); + if (!values || !offsets) + die("out of memory"); + + for (i = 0; i < num_chips; i++) { + device = entries[i]->d_name; + chip = chip_open_by_name(device); + if (!chip) { + if (errno == EACCES) + continue; + + die_perror("unable to open %s", device); + } + offset = gpiod_chip_find_line(chip, name); + if (offset >= 0) { + offsets[0] = offset; + break; + } + } + if (i == num_chips) + die("unable to find line '%s'", name); + } else { + if (argc < 1) + die("gpiochip must be specified"); + + if (argc < 2) + die("at least one GPIO line offset must be specified"); + + device = argv[0]; + num_lines = argc - 1; + + values = malloc(sizeof(*values) * num_lines); + offsets = malloc(sizeof(*offsets) * num_lines); + if (!values || !offsets) + die("out of memory"); + + for (i = 0; i < num_lines; i++) { + offsets[i] = strtoul(argv[i + 1], &end, 10); + if (*end != '\0' || offsets[i] > INT_MAX) + die("invalid GPIO offset: %s", argv[i + 1]); + } - for (i = 0; i < num_lines; i++) { - offsets[i] = strtoul(argv[i + 1], &end, 10); - if (*end != '\0' || offsets[i] > INT_MAX) - die("invalid GPIO offset: %s", argv[i + 1]); + chip = chip_open_lookup(device); + if (!chip) + die_perror("unable to open %s", device); } - chip = chip_open_lookup(device); - if (!chip) - die_perror("unable to open %s", device); - lines = gpiod_chip_get_lines(chip, offsets, num_lines); if (!lines) die_perror("unable to retrieve GPIO lines from chip");