Re: [libgpiod][RFC/RFT 06/18] bindings: glib: add examples

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Tue, Apr 23, 2024 at 8:23 PM Andy Shevchenko
<andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
>
> On Fri, Apr 12, 2024 at 02:27:52PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> >
> > Add example programs showcasing the usage of GLib bindings to libgpiod.
>
> > +             ret = g_gpiod_chip_get_line_offset_from_name(chip, line_name,
> > +                                                          &offset, &err);
> > +             if (ret) {
> > +                     info = g_gpiod_chip_get_info(chip, &err);
> > +                     if (!info) {
> > +                             g_printerr("Failed to get chip info: %s\n",
> > +                                        err->message);
> > +                             return EXIT_FAILURE;
> > +                     }
> > +
> > +                     g_print("%s %u\n",
> > +                             g_gpiod_chip_info_get_name(info),
> > +                             offset);
> > +
> > +                     return EXIT_SUCCESS;
> > +             } else if (!ret && err) {
>
> Besides redundant 'else' the !ret is also redundant.
>

Indeed but isn't it premature nitpicking in the grand scheme of things? :)

> > +                     g_printerr("Failed to map the line name '%s' to offset: %s\n",
> > +                                line_name, err->message);
> > +                     return EXIT_FAILURE;
> > +             }
> > +     }
>
> ...
>
> > +             direction == G_GPIOD_LINE_DIRECTION_INPUT ?
> > +                                     "input" : "output",
>
> One line?
>
> ...
>
> > +     settings = g_gpiod_line_settings_new(
> > +                     "direction", G_GPIOD_LINE_DIRECTION_INPUT,
> > +                     NULL);
>
> Personally I do not like the open parenthesis style...
>
> I don't even know why you have done this way with having a plenty of room in
> the previous line at least for the first parameter.
>
> ...
>
> > +     req_cfg = g_gpiod_request_config_new(
> > +                     "consumer", "get-multiple-line-values", NULL);
>
> Ditto. And so on across the code of the entire series...
>

Personal taste.

> ...
>
> > +     ret = g_gpiod_line_request_set_value(data->request, data->line_offset,
> > +                                          data->value, &err);
> > +     if (!ret) {
>
> ret == 0 equals error?!
>

It returns gboolean where false means error. This is a pattern in GLib.

> > +             g_printerr("failed to set line value: %s\n", err->message);
> > +             exit(EXIT_FAILURE);
>
> Don't you have something like err->code to propagate?
>

What for? err->message is the human-readable string of the error.

Bart