Re: [PATCH v3 1/5] tools/gpio: add gpio basic opereations

[Michael Welling <mwelling@xxxxxxxx>]

On Wed, Sep 28, 2016 at 02:35:17PM -0700, Bamvor Zhang Jian wrote:
> Hi, Michael
> 
> On 28 September 2016 at 12:16, Michael Welling <mwelling@xxxxxxxx> wrote:
> > On Wed, Aug 31, 2016 at 11:45:44AM +0200, bamvor.zhangjian@xxxxxxxxxx wrote:
> >> From: Bamvor Jian Zhang <bamvor.zhangjian@xxxxxxxxxx>
> [...]
> >> +     ret = ioctl(fd, GPIOHANDLE_SET_LINE_VALUES_IOCTL, data);
> >> +     if (ret == -1) {
> >> +             ret = -errno;
> >> +             fprintf(stderr, "Failed to issue %s (%d)\n",
> >> +                     "GPIOHANDLE_SET_LINE_VALUES_IOCTL", ret);
> >> +             goto exit_close_error;
> >
> > This goto is unnecessary because there is no code to be skipped.
> >
> >> +     }
> >> +
> >> +exit_close_error:
> >> +     return ret;
> >> +}
> >> +
> >> +int gpio_get_values(const int fd, struct gpiohandle_data *data)
> >> +{
> >> +     int ret;
> >> +
> >> +     ret = ioctl(fd, GPIOHANDLE_GET_LINE_VALUES_IOCTL, data);
> >> +     if (ret == -1) {
> >> +             ret = -errno;
> >> +             fprintf(stderr, "Failed to issue %s (%d)\n",
> >> +                     "GPIOHANDLE_GET_LINE_VALUES_IOCTL", ret);
> >> +             goto exit_close_error;
> >
> > Same here.
> Thanks. I will fix above two in next version.
> >
> >> +     }
> >> +
> >> +exit_close_error:
> >> +     return ret;
> >> +}
> >> +
> >> +int gpio_release(const int fd)
> >> +{
> >> +     int ret;
> >> +
> >> +     ret = close(fd);
> >> +     if (ret < -1)
> >> +             perror("Failed to close GPIO LINEHANDLE device file");
> >> +
> >> +     return ret;
> >> +}
> >> +
> >> +int gpio_gets(const char *device_name, unsigned int *lines, unsigned int nlines,
> >> +           unsigned int flag, struct gpiohandle_data *data)
> >> +{
> >> +     int fd;
> >> +     int ret;
> >> +
> >> +     ret = gpio_request(device_name, lines, nlines, flag, data, COMSUMER);
> >> +     if (ret < 0)
> >> +             return ret;
> >> +
> >> +     fd = ret;
> >> +     ret = gpio_get_values(fd, data);
> >
> > The error checking is missing here.
> Well, no matter the gpio_get_values fails or not. We both need to release the
> line we request. But, we need the return value if gpio_get_values fails. In the
> next version, I will return the errno of gpio_get_values if it fails.
> >
> >> +     ret = gpio_release(fd);
> >
> > Shouldn't we leave it up the user how they want the deal with the file handle?
> > There is more system call overhead if we open and close the GPIO device for
> > each access.
> Yes. I understand and agree the syscall overhead you mentioned. So, I provide
> the request, ops, release ops above. These gpio_[sg]ets? functions is the easy
> way if the user only access the gpio in a few times.
> >
> >> +     return ret;
> >> +}
> >> +
> >> +int gpio_sets(const char *device_name, unsigned int *lines, unsigned int nlines,
> >> +           unsigned int flag, struct gpiohandle_data *data)
> >> +{
> >> +     int ret;
> >> +
> >> +     ret = gpio_request(device_name, lines, nlines, flag, data, COMSUMER);
> >> +     if (ret < 0)
> >> +             return ret;
> >> +
> >> +     ret = gpio_release(ret);
> >> +     return ret;
> >> +}
> >> +
> >> +int gpio_get(const char *device_name, unsigned int line, unsigned int flag)
> >> +{
> >> +     struct gpiohandle_data data;
> >> +     unsigned int lines[] = {line};
> >
> > Is the lines variable really needed?
> Do you mean something like gpio_gets(device_name, {line}, 1, flag, &data);?
> Personally, I like the first version. Because it is more clear and compiler
> will optimize it anyway. Ideas?

gpio_gets(device_name, &line, 1, flag, &data);

It may be easier to understand the way you have it.

> 
> Regards
> 
> Bamvor
--
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