[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZyNKpC_3E3GFsyXL@smile.fi.intel.com>
Date: Thu, 31 Oct 2024 11:15:16 +0200
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Bartosz Golaszewski <brgl@...ev.pl>
Cc: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>,
linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org,
Linus Walleij <linus.walleij@...aro.org>
Subject: Re: [PATCH v1 1/1] gpio: Use traditional pattern when checking error
codes
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@...ux.intel.com> 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
Powered by blists - more mailing lists