RFC: let gpiod_get_optional et all return NULL when GPIOLIB is not enabled

PATCH New Uwe Kleine-König 3 participants 10 messages Feb 12, 2015 – Apr 28, 2015 GPIO

lore

Share & tools

lore

Uwe Kleine-König

author

2015-02-12 09:03 UTC #0 lore raw

PATCH 1/1 RFC: let gpiod_get_optional et all return NULL when GPIOLIB is not enabled

If you look for example at sound/soc/codecs/adau1977.c it has:

	adau1977->reset_gpio = devm_gpiod_get(dev, "reset");
	if (IS_ERR(adau1977->reset_gpio)) {
		ret = PTR_ERR(adau1977->reset_gpio);
		if (ret != -ENOENT && ret != -ENOSYS)
			return PTR_ERR(adau1977->reset_gpio);
		adau1977->reset_gpio = NULL;
	}

This code could better make use of devm_gpiod_get_optional like:

	adau1977->reset_gpio = devm_gpiod_get_optional(dev, "reset",
						       GPIOD_OUT_LOW);
	if (IS_ERR(adau1977->reset_gpio))
		return PTR_ERR(adau1977->reset_gpio);

but this slightly changes semantics. I.e. if GPIOLIB is not enabled devm_gpiod_get_optional unconditionally returns -ENOSYS which is ignored explicitly above but an error in the changed code.

I wonder if gpiod_get_optional et all should be changed to return NULL instead. The obvious downside is that if the device tree specifies a reset-gpio and the kernel just fails to use it because there is some code missing, this should better be an error. (The adau1977 code has this problem already know, but when changing devm_gpiod_get_optional all callers are affected.)

What do you think?

---
 include/linux/gpio/consumer.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

include/linux/gpio/consumer.h +4 -4

index fd85cb120ee0..f68244ffd3a9 100644
@@ -132,14 +132,14 @@ static inline struct gpio_desc *__must_check
 __gpiod_get_optional(struct device *dev, const char *con_id,
 		     enum gpiod_flags flags)
 {
-	return ERR_PTR(-ENOSYS);
+	return NULL;
 }
 
 static inline struct gpio_desc *__must_check
 __gpiod_get_index_optional(struct device *dev, const char *con_id,
 			   unsigned int index, enum gpiod_flags flags)
 {
-	return ERR_PTR(-ENOSYS);
+	return NULL;
 }
 
 static inline void gpiod_put(struct gpio_desc *desc)

@@ -171,14 +171,14 @@ static inline struct gpio_desc *__must_check
 __devm_gpiod_get_optional(struct device *dev, const char *con_id,
 			  enum gpiod_flags flags)
 {
-	return ERR_PTR(-ENOSYS);
+	return NULL;
 }
 
 static inline struct gpio_desc *__must_check
 __devm_gpiod_get_index_optional(struct device *dev, const char *con_id,
 				unsigned int index, enum gpiod_flags flags)
 {
-	return ERR_PTR(-ENOSYS);
+	return NULL;
 }
 
 static inline void devm_gpiod_put(struct device *dev, struct gpio_desc *desc)

Signature · 1 line

2.1.4

Linus Walleij

2015-03-06 08:26 UTC #1 lore raw

PATCH 1/1 RFC: let gpiod_get_optional et all return NULL when GPIOLIB is not enabled

Quoted 2 lines · Uwe Kleine-K?nig

I wonder if gpiod_get_optional et all should be changed to return NULL instead.

This is actually the norm in most subsystems returning cookie pointers, clk, regulator, pinctrl... a NULL pointer is a functional noop. But normally then NULL is returned from all stubs, not just optional.

Alexandre, what do you say?

Quoted 5 lines

The obvious downside is that if the device tree specifies a reset-gpio and the kernel just fails to use it because there is some code missing, this should better be an error. (The adau1977 code has this problem already know, but when changing devm_gpiod_get_optional all callers are affected.)

Device Tree-specific problems is not something we design subsystems for, we try to just accomodate them. I'm not sure I fully understand what you mean here.

Yours,
Linus Walleij

Uwe Kleine-König

author

2015-03-06 08:59 UTC #2 lore raw

PATCH 1/1 RFC: let gpiod_get_optional et all return NULL when GPIOLIB is not enabled

Hello,

Quoted 19 lines · Linus Walleij

Quoted 2 lines · Uwe Kleine-K?nig
I wonder if gpiod_get_optional et all should be changed to return NULL instead.
This is actually the norm in most subsystems returning cookie pointers, clk, regulator, pinctrl... a NULL pointer is a functional noop. But normally then NULL is returned from all stubs, not just optional.
Alexandre, what do you say?
Quoted 5 lines
The obvious downside is that if the device tree specifies a reset-gpio and the kernel just fails to use it because there is some code missing, this should better be an error. (The adau1977 code has this problem already know, but when changing devm_gpiod_get_optional all callers are affected.)

Quoted 3 lines

Device Tree-specific problems is not something we design subsystems for, we try to just accomodate them. I'm not sure I fully understand what you mean here.

Consider you have a device that has:

	enable-gpio = <&gpio3 12 0>;

and you do in your driver:

	enablegpio = devm_gpiod_get_optional(... "enable" ...);

. If GPIOLIB is off, enablegpio gets assigned NULL and the driver continues happily without enabling the device which most likely is a bug.

So IMHO the logic in devm_gpiod_get_optional for the GPIOLIB=n case should be:

	if (device_has_gpio())
		return ERR_PTR(-ENOSYS);
	else
		return NULL;

. device_has_gpio should use similar logic like gpiod_get_index to check if there is a gpio. If this is considered to be too complicated for a disabled subsystem, returning -ENOSYS unconditionally is better than NULL.

Best regards
Uwe

Signature · 2 lines

Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Linus Walleij

2015-03-09 16:44 UTC #3 lore raw

PATCH 1/1 RFC: let gpiod_get_optional et all return NULL when GPIOLIB is not enabled

Quoted 43 lines · Uwe Kleine-K?nig

Quoted 19 lines · Linus Walleij
Quoted 2 lines · Uwe Kleine-K?nig
I wonder if gpiod_get_optional et all should be changed to return NULL instead.
This is actually the norm in most subsystems returning cookie pointers, clk, regulator, pinctrl... a NULL pointer is a functional noop. But normally then NULL is returned from all stubs, not just optional.
Alexandre, what do you say?
Quoted 5 lines
The obvious downside is that if the device tree specifies a reset-gpio and the kernel just fails to use it because there is some code missing, this should better be an error. (The adau1977 code has this problem already know, but when changing devm_gpiod_get_optional all callers are affected.)
Quoted 3 lines
Device Tree-specific problems is not something we design subsystems for, we try to just accomodate them. I'm not sure I fully understand what you mean here.
Consider you have a device that has:
        enable-gpio = <&gpio3 12 0>;
and you do in your driver:
        enablegpio = devm_gpiod_get_optional(... "enable" ...);
. If GPIOLIB is off, enablegpio gets assigned NULL and the driver continues happily without enabling the device which most likely is a bug.
So IMHO the logic in devm_gpiod_get_optional for the GPIOLIB=n case should be:
        if (device_has_gpio())
                return ERR_PTR(-ENOSYS);
        else
                return NULL;

Quoted 4 lines

. device_has_gpio should use similar logic like gpiod_get_index to check if there is a gpio. If this is considered to be too complicated for a disabled subsystem, returning -ENOSYS unconditionally is better than NULL.

Aha Oh that's complex.

I cannot answer that myself, it needs feedback from the OF/DT maintainers before I even dare to think anything, as it is related to how hardware is described :/

Added devicetree-list and DT/OF maintainers to To: line to get some feedback on this.

Yours,
Linus Walleij

Alexandre Courbot

2015-04-09 02:20 UTC #4 lore raw

PATCH 1/1 RFC: let gpiod_get_optional et all return NULL when GPIOLIB is not enabled

Quoted 45 lines · Uwe Kleine-K?nig

Hello,
Quoted 19 lines · Linus Walleij
Quoted 2 lines · Uwe Kleine-K?nig
I wonder if gpiod_get_optional et all should be changed to return NULL instead.
This is actually the norm in most subsystems returning cookie pointers, clk, regulator, pinctrl... a NULL pointer is a functional noop. But normally then NULL is returned from all stubs, not just optional.
Alexandre, what do you say?
Quoted 5 lines
The obvious downside is that if the device tree specifies a reset-gpio and the kernel just fails to use it because there is some code missing, this should better be an error. (The adau1977 code has this problem already know, but when changing devm_gpiod_get_optional all callers are affected.)
Quoted 3 lines
Device Tree-specific problems is not something we design subsystems for, we try to just accomodate them. I'm not sure I fully understand what you mean here.
Consider you have a device that has:
        enable-gpio = <&gpio3 12 0>;
and you do in your driver:
        enablegpio = devm_gpiod_get_optional(... "enable" ...);
. If GPIOLIB is off, enablegpio gets assigned NULL and the driver continues happily without enabling the device which most likely is a bug.
So IMHO the logic in devm_gpiod_get_optional for the GPIOLIB=n case should be:
        if (device_has_gpio())
                return ERR_PTR(-ENOSYS);
        else
                return NULL;

Quoted 4 lines

. device_has_gpio should use similar logic like gpiod_get_index to check if there is a gpio. If this is considered to be too complicated for a disabled subsystem, returning -ENOSYS unconditionally is better than NULL.

I should have replied one month ago, but if gpiolib is disabled, how can we use gpiolib-like logic to check the existence of a GPIO?

Having GPIO disabled means there is no GPIO support, including the ability to look for GPIOs. -ENOSYS is a well-documented error-code which meaning also applies to the gpio_*_optional functions (we don't have support for the operation you requested). If a driver or architecture really, really needs GPIO support they can require or depend on CONFIG_GPIOLIB, and the problem goes away. If they can work with and without gpiolib, then they should check for -ENOSYS when they request GPIOs and behave accordingly.

Moving the interpretation of what the absence of gpiolib means down to the GPIO functions themselves is actually what might lead consumers to not know the result of their request. For this reason I would say that -ENOSYS is appropriate here.

Linus Walleij

2015-04-27 13:05 UTC #5 lore raw

PATCH 1/1 RFC: let gpiod_get_optional et all return NULL when GPIOLIB is not enabled

Quoted 8 lines · Alexandre Courbot

Having GPIO disabled means there is no GPIO support, including the ability to look for GPIOs. -ENOSYS is a well-documented error-code which meaning also applies to the gpio_*_optional functions (we don't have support for the operation you requested). If a driver or architecture really, really needs GPIO support they can require or depend on CONFIG_GPIOLIB, and the problem goes away. If they can work with and without gpiolib, then they should check for -ENOSYS when they request GPIOs and behave accordingly.

Makes sense. We have ARCH_WANT_OPTIONAL_GPIOLIB and ARCH_REQUIRE_GPIOLIB for this reason.

Yours,
Linus Walleij

Uwe Kleine-König

author

2015-04-27 15:21 UTC #6 lore raw

PATCH 1/1 RFC: let gpiod_get_optional et all return NULL when GPIOLIB is not enabled

In reply to Alexandre Courbot

Hello,

Quoted 11 lines · Alexandre Courbot

I should have replied one month ago, but if gpiolib is disabled, how can we use gpiolib-like logic to check the existence of a GPIO?
Having GPIO disabled means there is no GPIO support, including the ability to look for GPIOs. -ENOSYS is a well-documented error-code which meaning also applies to the gpio_*_optional functions (we don't have support for the operation you requested). If a driver or architecture really, really needs GPIO support they can require or depend on CONFIG_GPIOLIB, and the problem goes away. If they can work with and without gpiolib, then they should check for -ENOSYS when they request GPIOs and behave accordingly.

What whould be the right behaviour in your eyes? I hope it's not

	if (ret != -ENOSYS)
		return ret;

	/* continue and ignore error */

Quoted 4 lines

Moving the interpretation of what the absence of gpiolib means down to the GPIO functions themselves is actually what might lead consumers to not know the result of their request. For this reason I would say that -ENOSYS is appropriate here.

Yeah, I fully agree.

Best regards
Uwe

Signature · 2 lines

Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Alexandre Courbot

2015-04-28 03:31 UTC #7 lore raw

PATCH 1/1 RFC: let gpiod_get_optional et all return NULL when GPIOLIB is not enabled

Quoted 14 lines · Uwe Kleine-K?nig

Hello,
Quoted 11 lines · Alexandre Courbot
I should have replied one month ago, but if gpiolib is disabled, how can we use gpiolib-like logic to check the existence of a GPIO?
Having GPIO disabled means there is no GPIO support, including the ability to look for GPIOs. -ENOSYS is a well-documented error-code which meaning also applies to the gpio_*_optional functions (we don't have support for the operation you requested). If a driver or architecture really, really needs GPIO support they can require or depend on CONFIG_GPIOLIB, and the problem goes away. If they can work with and without gpiolib, then they should check for -ENOSYS when they request GPIOs and behave accordingly.

Quoted 6 lines

What whould be the right behaviour in your eyes? I hope it's not
        if (ret != -ENOSYS)
                return ret;

        /* continue and ignore error */

If a consumer absolutely needs a GPIO (most of the drivers out there I believe), then -ENOSYS can be handled like any other error. If it doesn't, and the driver is fine without GPIO support as well (meaning that it can somehow work even if a GPIO is declared, but GPIO support is not compiled in), then it will need to explicitly handle that particular error. That case should be rare though - most drivers will want to propagate -ENOSYS.

Uwe Kleine-König

author

2015-04-28 06:45 UTC #8 lore raw

PATCH 1/1 RFC: let gpiod_get_optional et all return NULL when GPIOLIB is not enabled

Hello,

Quoted 18 lines · Alexandre Courbot

Quoted 9 lines · Uwe Kleine-K?nig
Quoted 8 lines · Alexandre Courbot
Having GPIO disabled means there is no GPIO support, including the ability to look for GPIOs. -ENOSYS is a well-documented error-code which meaning also applies to the gpio_*_optional functions (we don't have support for the operation you requested). If a driver or architecture really, really needs GPIO support they can require or depend on CONFIG_GPIOLIB, and the problem goes away. If they can work with and without gpiolib, then they should check for -ENOSYS when they request GPIOs and behave accordingly.
Quoted 6 lines
What whould be the right behaviour in your eyes? I hope it's not
        if (ret != -ENOSYS)
                return ret;

        /* continue and ignore error */

Quoted 7 lines

If a consumer absolutely needs a GPIO (most of the drivers out there I believe), then -ENOSYS can be handled like any other error. If it doesn't, and the driver is fine without GPIO support as well (meaning that it can somehow work even if a GPIO is declared, but GPIO support is not compiled in), then it will need to explicitly handle that particular error. That case should be rare though - most drivers will want to propagate -ENOSYS.

Ack.

Uwe

Signature · 2 lines

Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Alexandre Courbot

2015-04-28 06:46 UTC #9 lore raw

PATCH 1/1 RFC: let gpiod_get_optional et all return NULL when GPIOLIB is not enabled

Quoted 21 lines · Uwe Kleine-K?nig

Hello,
Quoted 18 lines · Alexandre Courbot
Quoted 15 lines · Uwe Kleine-K?nig
Quoted 8 lines · Alexandre Courbot
Having GPIO disabled means there is no GPIO support, including the ability to look for GPIOs. -ENOSYS is a well-documented error-code which meaning also applies to the gpio_*_optional functions (we don't have support for the operation you requested). If a driver or architecture really, really needs GPIO support they can require or depend on CONFIG_GPIOLIB, and the problem goes away. If they can work with and without gpiolib, then they should check for -ENOSYS when they request GPIOs and behave accordingly.
What whould be the right behaviour in your eyes? I hope it's not
        if (ret != -ENOSYS)
                return ret;

        /* continue and ignore error */

Quoted 8 lines

Quoted 7 lines
If a consumer absolutely needs a GPIO (most of the drivers out there I believe), then -ENOSYS can be handled like any other error. If it doesn't, and the driver is fine without GPIO support as well (meaning that it can somehow work even if a GPIO is declared, but GPIO support is not compiled in), then it will need to explicitly handle that particular error. That case should be rare though - most drivers will want to propagate -ENOSYS.
Ack.

Of course if you can think of a more elegant way to handle this, by all means, please let us know. :)