[libgpiod PATCH] gpioget: Add --line-name to lookup GPIO line

[libgpiod PATCH] gpioget: Add --line-name to lookup GPIO line

[Joel Stanley]

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

Signed-off-by: Joel Stanley <joel@jms.id.au>
---
While users do have the ability to chain together gpioinfo and gpioget,
this is less discoverable for people new to the tools, and harder to
explain to eg. technicians, and requires more typing.

Please consider this enhancement. If you are happy with it I can send
a patch for gpioset too.

Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 tools/gpioget.c | 86 ++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 64 insertions(+), 22 deletions(-)

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");
-- 
2.33.0

Re: [libgpiod PATCH] gpioget: Add --line-name to lookup GPIO line

[Bartosz Golaszewski]

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
>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
> While users do have the ability to chain together gpioinfo and gpioget,
> this is less discoverable for people new to the tools, and harder to
> explain to eg. technicians, and requires more typing.
>
> Please consider this enhancement. If you are happy with it I can send
> a patch for gpioset too.
>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
>  tools/gpioget.c | 86 ++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 64 insertions(+), 22 deletions(-)
>
> 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");
> --
> 2.33.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". 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.

Bart

Re: [libgpiod PATCH] gpioget: Add --line-name to lookup GPIO line

[Joel Stanley]

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

Re: [libgpiod PATCH] gpioget: Add --line-name to lookup GPIO line

[Zev Weiss]

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

Re: [libgpiod PATCH] gpioget: Add --line-name to lookup GPIO line

[Andrew Jeffery]

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

Re: [libgpiod PATCH] gpioget: Add --line-name to lookup GPIO line

[Bartosz Golaszewski]

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

Re: [libgpiod PATCH] gpioget: Add --line-name to lookup GPIO line

[Joel Stanley]

On Fri, 3 Dec 2021 at 10:18, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> > > 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.

Good plan. Thanks Jeremy, Zev, Andrew and Bart. I'll get hacking.

Cheers,

Joel

Re: [libgpiod PATCH] gpioget: Add --line-name to lookup GPIO line

[Jeremy Kerr]

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