Re: [PATCH v1 1/1] gpio: Use traditional pattern when checking error codes

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

On Wed, Oct 30, 2024 at 09:20:45PM +0100, Bartosz Golaszewski wrote:
> On Mon, Oct 28, 2024 at 2:44 PM Andy Shevchenko
> <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
> >
> > Instead of 'if (ret == 0)' switch to "check for the error first" rule.
> 
> Well there's much more to this patch than that and I have some issues with it.
> 
> > While it gives a "+" (plus) statistics it makes the code easier to read
> 
> Not only does it increase the footprint but it also adds completely
> unnecessary goto labels.

These pieces can be dropped.

...

> > and maintain (when, e.g., want to add somethning in between touched lines).
> 
> The single line calls to the notifier chain are unlikely to be
> extended anytime soon but even then I think we should cross that
> bridge when we get there.

Okay.

...

> > -       if (!ret)
> > -               gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
> > +       if (ret)
> > +               return ret;
> >
> > -       return ret;
> > +       gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
> > +       return 0;
> >  }
> 
> I really don't see how this makes it better. The logic here is: if the
> underlying set config worked fine - emit the event. Otherwise continue
> with the function (even if there's nothing there now). If anything
> you're making it more difficult to modify later because logically the
> notification is just an optional step on the way to returning from the
> function.

Optional steps are covered by flags, and not by checking the previous call for
failure. So, I barely see the "optionality" of the notifications in these calls.

...

> >         ret = gpiod_direction_input_nonotify(desc);
> > -       if (ret == 0)
> > -               gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
> > +       if (ret)
> > +               return ret;
> 
> Ok, for consistency I could take it but please put this into a
> separate commit doing just that (here and elsewhere).

Based on the other comments from you in this email I'm not sure I understood
this correctly. Do you want to reject the complete patch, or do you agree on
some pieces out of it.

> > -       return ret;
> > +       gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
> > +       return 0;

...

> >         ret = gpio_do_set_config(desc, config);
> > -       if (!ret) {
> > -               /* These are the only options we notify the userspace about. */
> > -               switch (pinconf_to_config_param(config)) {
> > -               case PIN_CONFIG_BIAS_DISABLE:
> > -               case PIN_CONFIG_BIAS_PULL_DOWN:
> > -               case PIN_CONFIG_BIAS_PULL_UP:
> > -               case PIN_CONFIG_DRIVE_OPEN_DRAIN:
> > -               case PIN_CONFIG_DRIVE_OPEN_SOURCE:
> > -               case PIN_CONFIG_DRIVE_PUSH_PULL:
> > -               case PIN_CONFIG_INPUT_DEBOUNCE:
> > -                       gpiod_line_state_notify(desc,
> > -                                               GPIO_V2_LINE_CHANGED_CONFIG);
> > -                       break;
> > -               default:
> > -                       break;
> > -               }
> 
> If you really want to get rid of one level of indentation here,
> I suggest moving it into a separate function.

Perhaps you suggested a separate change for that?

> > +       if (ret)
> > +               return ret;
> > +
> > +       /* These are the only options we notify the userspace about */
> > +       switch (pinconf_to_config_param(config)) {
> > +       case PIN_CONFIG_BIAS_DISABLE:
> > +       case PIN_CONFIG_BIAS_PULL_DOWN:
> > +       case PIN_CONFIG_BIAS_PULL_UP:
> > +       case PIN_CONFIG_DRIVE_OPEN_DRAIN:
> > +       case PIN_CONFIG_DRIVE_OPEN_SOURCE:
> > +       case PIN_CONFIG_DRIVE_PUSH_PULL:
> > +       case PIN_CONFIG_INPUT_DEBOUNCE:
> > +               gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
> > +               break;
> > +       default:
> > +               break;
> >         }
> >
> > -       return ret;
> > +       return 0;

...

> Most of this is IMO pointless churn. You typically do a lot of great
> cleanups but this just doesn't make sense. Sorry but NAK.

OK, I do one change out of that with deduplication of the direction input call,
the rest is up to you, let's it be less readable.

-- 
With Best Regards,
Andy Shevchenko