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

[Bamvor Zhang Jian <bamvor.zhangjian@xxxxxxxxxx>]

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?

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