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

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

[Uwe Kleine-König]

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(-)

diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index fd85cb120ee0..f68244ffd3a9 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -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)
-- 
2.1.4

Re: [PATCH] RFC: let gpiod_get_optional et all return NULL when GPIOLIB is not enabled

[Linus Walleij]

On Thu, Feb 12, 2015 at 10:03 AM, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:

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

> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] RFC: let gpiod_get_optional et all return NULL when GPIOLIB is not enabled

[Uwe Kleine-König]

Hello,

On Fri, Mar 06, 2015 at 09:26:26AM +0100, Linus Walleij wrote:
> On Thu, Feb 12, 2015 at 10:03 AM, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> 
> > 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?
> 
> > 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.
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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] RFC: let gpiod_get_optional et all return NULL when GPIOLIB is not enabled

[Linus Walleij]

On Fri, Mar 6, 2015 at 9:59 AM, Uwe Kleine-König
<u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> On Fri, Mar 06, 2015 at 09:26:26AM +0100, Linus Walleij wrote:
>> On Thu, Feb 12, 2015 at 10:03 AM, Uwe Kleine-König
>> <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
>>
>> > 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?
>>
>> > 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.
> 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.

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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] RFC: let gpiod_get_optional et all return NULL when GPIOLIB is not enabled

[Alexandre Courbot]

On Fri, Mar 6, 2015 at 5:59 PM, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> Hello,
>
> On Fri, Mar 06, 2015 at 09:26:26AM +0100, Linus Walleij wrote:
>> On Thu, Feb 12, 2015 at 10:03 AM, Uwe Kleine-König
>> <u.kleine-koenig@pengutronix.de> wrote:
>>
>> > 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?
>>
>> > 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.
> 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.

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] RFC: let gpiod_get_optional et all return NULL when GPIOLIB is not enabled

[Linus Walleij]

On Thu, Apr 9, 2015 at 4:20 AM, Alexandre Courbot <gnurou@gmail.com> wrote:

> 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

Re: [PATCH] RFC: let gpiod_get_optional et all return NULL when GPIOLIB is not enabled

[Uwe Kleine-König]

Hello,

On Thu, Apr 09, 2015 at 11:20:55AM +0900, Alexandre Courbot wrote:
> 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 */

> 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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] RFC: let gpiod_get_optional et all return NULL when GPIOLIB is not enabled

[Alexandre Courbot]

On Tue, Apr 28, 2015 at 12:21 AM, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> Hello,
>
> On Thu, Apr 09, 2015 at 11:20:55AM +0900, Alexandre Courbot wrote:
>> 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 */

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] RFC: let gpiod_get_optional et all return NULL when GPIOLIB is not enabled

[Uwe Kleine-König]

Hello,

On Tue, Apr 28, 2015 at 12:31:37PM +0900, Alexandre Courbot wrote:
> On Tue, Apr 28, 2015 at 12:21 AM, Uwe Kleine-König
> <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> > On Thu, Apr 09, 2015 at 11:20:55AM +0900, Alexandre Courbot wrote:
> >> 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 */
> 
> 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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] RFC: let gpiod_get_optional et all return NULL when GPIOLIB is not enabled

[Alexandre Courbot]

On Tue, Apr 28, 2015 at 3:45 PM, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> Hello,
>
> On Tue, Apr 28, 2015 at 12:31:37PM +0900, Alexandre Courbot wrote:
>> On Tue, Apr 28, 2015 at 12:21 AM, Uwe Kleine-König
>> <u.kleine-koenig@pengutronix.de> wrote:
>> > On Thu, Apr 09, 2015 at 11:20:55AM +0900, Alexandre Courbot wrote:
>> >> 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 */
>>
>> 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. :)
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html