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

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Wed, Dec 1, 2021 at 8:29 AM Joel Stanley <joel@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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@xxxxxxxxx>
> ---
>  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