Re: [PATCH] tools/gpio: add gpio util to get or set gpio lines

[Linus Walleij <linus.walleij@linaro.org>]

On Mon, Jul 24, 2017 at 4:32 PM, Martin Hundebøll <mnhu@prevas.dk> wrote:

> Add a new tool to set or get the value of one or more gpio lines, which
> can as either chip offsets or line names. This makes it easier to
> control gpio lines from user space without using the cumbersome sysfs
> interface. It also demostrates how gpio-line-names can be used to
> decouple software from hardware layout.
>
> Signed-off-by: Martin Hundebøll <mnhu@prevas.dk>

How does this relate to Bartosz userspace library libgpiod?
Have you checked it and considered using it?
https://github.com/brgl/libgpiod

>  tools/gpio/gpio.c     | 261 ++++++++++++++++++++++++++++++++++++++++++++++++++

We can't have a tool simply named "gpio" it is too unspecific.

Come up with a better name for this, such as "gpiog-get-set" or so.

Maybe it should be two tools? One fo setting and one for getting a line?

> + * lsgpio - example on how to list the GPIO lines on a system
> + *
> + * Copyright (C) 2015 Linus Walleij
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + *
> + * Usage:
> + *     lsgpio <-n device-name>

There is a lot of copy-paste going on here. Please fix it up.

> +int print_offsets(const char *device_name, unsigned int *lines, int nlines)
> +{
> +       struct gpiohandle_data data;
> +       int i, line, value, ret;
> +
> +       ret = gpiotools_gets(device_name, lines, nlines, &data);
> +       if (ret)
> +               return ret;
> +
> +       /* print value for each configured line */
> +       printf("GPIO chip: %s\n", device_name);
> +       for (i = 0; i < nlines; i++) {
> +               line = lines[i];
> +               value = data.values[line];
> +               printf("\tline %i: %i\n", lines[i], value);
> +       }
> +
> +       return ret;
> +}
> +
> +int print_labels(const char *device_name, unsigned int *lines,
> +                 const char *labels[], int nlabels)
> +{
> +       struct gpiohandle_data data;
> +       int i, value, ret;
> +       const char *label;
> +
> +       ret = gpiotools_gets(device_name, lines, nlabels, &data);
> +       if (ret)
> +               return ret;
> +
> +       /* print value for each configured label */
> +       printf("GPIO chip: %s\n", device_name);
> +       for (i = 0; i < nlabels; i++) {
> +               label = labels[i];
> +               value = data.values[i];
> +               printf("\t%s: %i\n", label, value);
> +       }
> +
> +       return ret;
> +}
> +
> +int control_offsets(const char *device_name, unsigned int *lines, int nlines,
> +                   int flags, int value)
> +{
> +       struct gpiohandle_data data = {{ value }};
> +
> +       if (flags & GPIOHANDLE_REQUEST_INPUT)
> +               return print_offsets(device_name, lines, nlines);
> +       else
> +               return gpiotools_sets(device_name, lines, nlines, &data);
> +}
> +
> +int control_labels(const char *device_name, const char *labels[], int nlabels,
> +                  int flags, int value)
> +{
> +       struct gpiochip_info cinfo = {0};
> +       char *chrdev_name = NULL;
> +       unsigned int lines[GPIOHANDLES_MAX];
> +       int nlines = 0;
> +       int ret, fd, i, j;
> +
> +       ret = asprintf(&chrdev_name, "/dev/%s", device_name);
> +       if (ret < 0)
> +               return -ENOMEM;
> +
> +       fd = open(chrdev_name, 0);
> +       if (fd == -1) {
> +               ret = -errno;
> +               fprintf(stderr, "Failed to open %s\n", chrdev_name);
> +               goto exit_error;
> +       }
> +
> +       /* Inspect this GPIO chip */
> +       ret = ioctl(fd, GPIO_GET_CHIPINFO_IOCTL, &cinfo);
> +       if (ret == -1) {
> +               ret = -errno;
> +               perror("Failed to issue CHIPINFO IOCTL\n");
> +               goto exit_close_error;
> +       }
> +
> +       /* Loop over the lines and match 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;
> +               }
> +
> +               /* Check if line name is included requested names */
> +               for (j = 0; j < nlabels; j++) {
> +                       if (strcmp(linfo.name, labels[j]) == 0) {
> +                               lines[nlines++] = i;
> +                               break;
> +                       }
> +               }
> +       }
> +
> +       if (nlines == 0)
> +               goto exit_close_error;
> +
> +       if (flags & GPIOHANDLE_REQUEST_INPUT) {
> +               print_labels(device_name, lines, labels, nlabels);
> +       } else {
> +               struct gpiohandle_data data = {{ value }};
> +               ret = gpiotools_sets(device_name, lines, nlines, &data);
> +       }
> +
> +exit_close_error:
> +       if (fd >= 0 && close(fd) == -1)
> +               perror("Failed to close GPIO character device file");
> +
> +exit_error:
> +       free(chrdev_name);
> +
> +       return ret;
> +}

All of this also looks copypasted.

Please go through your program and make something very
specific and to the point that does exactly what you want instead of
randomly reusing lsgpio.

Yours,
Linus Walleij