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

[Martin Hundebøll <martin.hundeboll@xxxxxxxxx>]

On August 3, 2017 9:56:00 AM GMT+02:00, Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
>On Mon, Jul 24, 2017 at 4:32 PM, Martin Hundebøll <mnhu@xxxxxxxxx>
>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@xxxxxxxxx>
>
>How does this relate to Bartosz userspace library libgpiod?
>Have you checked it and considered using it?
>https://github.com/brgl/libgpiod

I tried with the best of my Google Foo, but "gpio" is a bit too generic. libgpiod is exactly what I need, thanks.

>
>>  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?

Correct. I can update the patch into two utils, but with libgpiod in the mix, we can just drop this?

// Martin

>
>> + * 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

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html