diff mbox series

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

Message ID 20241028134454.1156852-1-andriy.shevchenko@linux.intel.com
State New
Headers show
Series [v1,1/1] gpio: Use traditional pattern when checking error codes | expand

Commit Message

Andy Shevchenko Oct. 28, 2024, 1:43 p.m. UTC
Instead of 'if (ret == 0)' switch to "check for the error first" rule.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---

While it gives a "+" (plus) statistics it makes the code easier to read
and maintain (when, e.g., want to add somethning in between touched lines).

 drivers/gpio/gpiolib.c | 104 ++++++++++++++++++++++-------------------
 1 file changed, 56 insertions(+), 48 deletions(-)

Comments

Linus Walleij Oct. 28, 2024, 10:10 p.m. UTC | #1
On Mon, Oct 28, 2024 at 2:44 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> Instead of 'if (ret == 0)' switch to "check for the error first" rule.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

This makes a lot of sense.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij
Bartosz Golaszewski Oct. 30, 2024, 8:20 p.m. UTC | #2
On Mon, Oct 28, 2024 at 2:44 PM Andy Shevchenko
<andriy.shevchenko@linux.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.

>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>
> 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.

> 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.

>  drivers/gpio/gpiolib.c | 104 ++++++++++++++++++++++-------------------
>  1 file changed, 56 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 5666c462248c..a9a3e032ed5b 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -2674,10 +2674,11 @@ int gpio_set_debounce_timeout(struct gpio_desc *desc, unsigned int debounce)
>         ret = gpio_set_config_with_argument_optional(desc,
>                                                      PIN_CONFIG_INPUT_DEBOUNCE,
>                                                      debounce);
> -       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.

>
>  /**
> @@ -2697,16 +2698,17 @@ int gpiod_direction_input(struct gpio_desc *desc)
>         VALIDATE_DESC(desc);
>
>         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).

> -       return ret;
> +       gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
> +       return 0;
>  }
>  EXPORT_SYMBOL_GPL(gpiod_direction_input);
>
>  int gpiod_direction_input_nonotify(struct gpio_desc *desc)
>  {
> -       int ret = 0;
> +       int ret;
>
>         CLASS(gpio_chip_guard, guard)(desc);
>         if (!guard.gc)
> @@ -2733,6 +2735,8 @@ int gpiod_direction_input_nonotify(struct gpio_desc *desc)
>         if (guard.gc->direction_input) {
>                 ret = guard.gc->direction_input(guard.gc,
>                                                 gpio_chip_hwgpio(desc));
> +               if (ret)
> +                       goto out_trace_direction;
>         } else if (guard.gc->get_direction &&
>                   (guard.gc->get_direction(guard.gc,
>                                            gpio_chip_hwgpio(desc)) != 1)) {
> @@ -2741,11 +2745,11 @@ int gpiod_direction_input_nonotify(struct gpio_desc *desc)
>                            __func__);
>                 return -EIO;
>         }
> -       if (ret == 0) {
> -               clear_bit(FLAG_IS_OUT, &desc->flags);
> -               ret = gpio_set_bias(desc);
> -       }
>
> +       clear_bit(FLAG_IS_OUT, &desc->flags);
> +       ret = gpio_set_bias(desc);
> +
> +out_trace_direction:

This makes it worse! We can't just apply the "return fast" rule
indiscriminately. Sometimes it does make sense to go "if the previous
step worked, do this, otherwise keep going".

Also: adding more labels for no reason?

>         trace_gpio_direction(desc_to_gpio(desc), 1, ret);
>
>         return ret;
> @@ -2774,6 +2778,8 @@ static int gpiod_direction_output_raw_commit(struct gpio_desc *desc, int value)
>         if (guard.gc->direction_output) {
>                 ret = guard.gc->direction_output(guard.gc,
>                                                  gpio_chip_hwgpio(desc), val);
> +               if (ret)
> +                       goto out_trace_value_and_direction;
>         } else {
>                 /* Check that we are in output mode if we can */
>                 if (guard.gc->get_direction &&
> @@ -2790,8 +2796,9 @@ static int gpiod_direction_output_raw_commit(struct gpio_desc *desc, int value)
>                 guard.gc->set(guard.gc, gpio_chip_hwgpio(desc), val);
>         }
>
> -       if (!ret)
> -               set_bit(FLAG_IS_OUT, &desc->flags);
> +       set_bit(FLAG_IS_OUT, &desc->flags);
> +
> +out_trace_value_and_direction:
>         trace_gpio_value(desc_to_gpio(desc), 0, val);
>         trace_gpio_direction(desc_to_gpio(desc), 0, ret);
>         return ret;
> @@ -2816,10 +2823,11 @@ int gpiod_direction_output_raw(struct gpio_desc *desc, int value)
>         VALIDATE_DESC(desc);
>
>         ret = gpiod_direction_output_raw_commit(desc, value);
> -       if (ret == 0)
> -               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;
>  }
>  EXPORT_SYMBOL_GPL(gpiod_direction_output_raw);
>
> @@ -2843,10 +2851,11 @@ int gpiod_direction_output(struct gpio_desc *desc, int value)
>         VALIDATE_DESC(desc);
>
>         ret = gpiod_direction_output_nonotify(desc, value);
> -       if (ret == 0)
> -               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;
>  }
>  EXPORT_SYMBOL_GPL(gpiod_direction_output);
>
> @@ -2877,19 +2886,15 @@ int gpiod_direction_output_nonotify(struct gpio_desc *desc, int value)
>                 if (!ret)
>                         goto set_output_value;
>                 /* Emulate open drain by not actively driving the line high */
> -               if (value) {
> -                       ret = gpiod_direction_input_nonotify(desc);
> +               if (value)
>                         goto set_output_flag;
> -               }
>         } else if (test_bit(FLAG_OPEN_SOURCE, &flags)) {
>                 ret = gpio_set_config(desc, PIN_CONFIG_DRIVE_OPEN_SOURCE);
>                 if (!ret)
>                         goto set_output_value;
>                 /* Emulate open source by not actively driving the line low */
> -               if (!value) {
> -                       ret = gpiod_direction_input_nonotify(desc);
> +               if (!value)
>                         goto set_output_flag;
> -               }
>         } else {
>                 gpio_set_config(desc, PIN_CONFIG_DRIVE_PUSH_PULL);
>         }
> @@ -2901,15 +2906,17 @@ int gpiod_direction_output_nonotify(struct gpio_desc *desc, int value)
>         return gpiod_direction_output_raw_commit(desc, value);
>
>  set_output_flag:
> +       ret = gpiod_direction_input_nonotify(desc);

This can't be right. Or am I just not getting it? In any case it's
completely unreadable now. Go set output flag but start by setting
direction to input first?

> +       if (ret)
> +               return ret;
>         /*
>          * When emulating open-source or open-drain functionalities by not
>          * actively driving the line (setting mode to input) we still need to
>          * set the IS_OUT flag or otherwise we won't be able to set the line
>          * value anymore.
>          */
> -       if (ret == 0)
> -               set_bit(FLAG_IS_OUT, &desc->flags);
> -       return ret;
> +       set_bit(FLAG_IS_OUT, &desc->flags);
> +       return 0;
>  }
>
>  /**
> @@ -2994,25 +3001,25 @@ int gpiod_set_config(struct gpio_desc *desc, unsigned long config)
>         VALIDATE_DESC(desc);
>
>         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.

> +       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;
>  }
>  EXPORT_SYMBOL_GPL(gpiod_set_config);
>
> @@ -3730,10 +3737,11 @@ int gpiod_set_consumer_name(struct gpio_desc *desc, const char *name)
>         VALIDATE_DESC(desc);
>
>         ret = desc_set_label(desc, name);
> -       if (ret == 0)
> -               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;
>  }
>  EXPORT_SYMBOL_GPL(gpiod_set_consumer_name);
>
> --
> 2.43.0.rc1.1336.g36b5255a03ac
>

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.

Bart
Bartosz Golaszewski Oct. 31, 2024, 6:23 p.m. UTC | #3
On Thu, Oct 31, 2024 at 10:15 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> 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@linux.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.
>

I want to reject most of the patch in this form. s/(ret == 0)/(!ret)/
is fine as a standalone change.

> > > -       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?
>

This doesn't really bother me but if it triggers you then feel free to
change it like:

if (!ret)
    gpio_set_config_line_state_notify(desc);

Bart

> > > +       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.
>
diff mbox series

Patch

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 5666c462248c..a9a3e032ed5b 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2674,10 +2674,11 @@  int gpio_set_debounce_timeout(struct gpio_desc *desc, unsigned int debounce)
 	ret = gpio_set_config_with_argument_optional(desc,
 						     PIN_CONFIG_INPUT_DEBOUNCE,
 						     debounce);
-	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;
 }
 
 /**
@@ -2697,16 +2698,17 @@  int gpiod_direction_input(struct gpio_desc *desc)
 	VALIDATE_DESC(desc);
 
 	ret = gpiod_direction_input_nonotify(desc);
-	if (ret == 0)
-		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;
 }
 EXPORT_SYMBOL_GPL(gpiod_direction_input);
 
 int gpiod_direction_input_nonotify(struct gpio_desc *desc)
 {
-	int ret = 0;
+	int ret;
 
 	CLASS(gpio_chip_guard, guard)(desc);
 	if (!guard.gc)
@@ -2733,6 +2735,8 @@  int gpiod_direction_input_nonotify(struct gpio_desc *desc)
 	if (guard.gc->direction_input) {
 		ret = guard.gc->direction_input(guard.gc,
 						gpio_chip_hwgpio(desc));
+		if (ret)
+			goto out_trace_direction;
 	} else if (guard.gc->get_direction &&
 		  (guard.gc->get_direction(guard.gc,
 					   gpio_chip_hwgpio(desc)) != 1)) {
@@ -2741,11 +2745,11 @@  int gpiod_direction_input_nonotify(struct gpio_desc *desc)
 			   __func__);
 		return -EIO;
 	}
-	if (ret == 0) {
-		clear_bit(FLAG_IS_OUT, &desc->flags);
-		ret = gpio_set_bias(desc);
-	}
 
+	clear_bit(FLAG_IS_OUT, &desc->flags);
+	ret = gpio_set_bias(desc);
+
+out_trace_direction:
 	trace_gpio_direction(desc_to_gpio(desc), 1, ret);
 
 	return ret;
@@ -2774,6 +2778,8 @@  static int gpiod_direction_output_raw_commit(struct gpio_desc *desc, int value)
 	if (guard.gc->direction_output) {
 		ret = guard.gc->direction_output(guard.gc,
 						 gpio_chip_hwgpio(desc), val);
+		if (ret)
+			goto out_trace_value_and_direction;
 	} else {
 		/* Check that we are in output mode if we can */
 		if (guard.gc->get_direction &&
@@ -2790,8 +2796,9 @@  static int gpiod_direction_output_raw_commit(struct gpio_desc *desc, int value)
 		guard.gc->set(guard.gc, gpio_chip_hwgpio(desc), val);
 	}
 
-	if (!ret)
-		set_bit(FLAG_IS_OUT, &desc->flags);
+	set_bit(FLAG_IS_OUT, &desc->flags);
+
+out_trace_value_and_direction:
 	trace_gpio_value(desc_to_gpio(desc), 0, val);
 	trace_gpio_direction(desc_to_gpio(desc), 0, ret);
 	return ret;
@@ -2816,10 +2823,11 @@  int gpiod_direction_output_raw(struct gpio_desc *desc, int value)
 	VALIDATE_DESC(desc);
 
 	ret = gpiod_direction_output_raw_commit(desc, value);
-	if (ret == 0)
-		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;
 }
 EXPORT_SYMBOL_GPL(gpiod_direction_output_raw);
 
@@ -2843,10 +2851,11 @@  int gpiod_direction_output(struct gpio_desc *desc, int value)
 	VALIDATE_DESC(desc);
 
 	ret = gpiod_direction_output_nonotify(desc, value);
-	if (ret == 0)
-		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;
 }
 EXPORT_SYMBOL_GPL(gpiod_direction_output);
 
@@ -2877,19 +2886,15 @@  int gpiod_direction_output_nonotify(struct gpio_desc *desc, int value)
 		if (!ret)
 			goto set_output_value;
 		/* Emulate open drain by not actively driving the line high */
-		if (value) {
-			ret = gpiod_direction_input_nonotify(desc);
+		if (value)
 			goto set_output_flag;
-		}
 	} else if (test_bit(FLAG_OPEN_SOURCE, &flags)) {
 		ret = gpio_set_config(desc, PIN_CONFIG_DRIVE_OPEN_SOURCE);
 		if (!ret)
 			goto set_output_value;
 		/* Emulate open source by not actively driving the line low */
-		if (!value) {
-			ret = gpiod_direction_input_nonotify(desc);
+		if (!value)
 			goto set_output_flag;
-		}
 	} else {
 		gpio_set_config(desc, PIN_CONFIG_DRIVE_PUSH_PULL);
 	}
@@ -2901,15 +2906,17 @@  int gpiod_direction_output_nonotify(struct gpio_desc *desc, int value)
 	return gpiod_direction_output_raw_commit(desc, value);
 
 set_output_flag:
+	ret = gpiod_direction_input_nonotify(desc);
+	if (ret)
+		return ret;
 	/*
 	 * When emulating open-source or open-drain functionalities by not
 	 * actively driving the line (setting mode to input) we still need to
 	 * set the IS_OUT flag or otherwise we won't be able to set the line
 	 * value anymore.
 	 */
-	if (ret == 0)
-		set_bit(FLAG_IS_OUT, &desc->flags);
-	return ret;
+	set_bit(FLAG_IS_OUT, &desc->flags);
+	return 0;
 }
 
 /**
@@ -2994,25 +3001,25 @@  int gpiod_set_config(struct gpio_desc *desc, unsigned long config)
 	VALIDATE_DESC(desc);
 
 	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 (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;
 }
 EXPORT_SYMBOL_GPL(gpiod_set_config);
 
@@ -3730,10 +3737,11 @@  int gpiod_set_consumer_name(struct gpio_desc *desc, const char *name)
 	VALIDATE_DESC(desc);
 
 	ret = desc_set_label(desc, name);
-	if (ret == 0)
-		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;
 }
 EXPORT_SYMBOL_GPL(gpiod_set_consumer_name);