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

From: Andy Shevchenko
Date: Thu Oct 31 2024 - 05:15:41 EST


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