linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] gpio: return NULL from gpiod_get_optional when GPIOLIB is disabled
@ 2017-02-13  1:13 Dmitry Torokhov
  2017-02-13  1:15 ` Dmitry Torokhov
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Dmitry Torokhov @ 2017-02-13  1:13 UTC (permalink / raw)
  To: Linus Walleij; +Cc: Alexandre Courbot, linux-gpio, linux-kernel

Given the intent behind gpiod_get_optional() and friends it does not make
sense to return -ENOSYS when GPIOLIB is disabled: the driver is expected to
work just fine without gpio so let's behave as if gpio was not found.
Otherwise we have to special-case -ENOSYS in drivers.

Note that there was objection that someone might forget to enable GPIOLIB
when dealing with a platform that has device that actually specifies
optional gpio and we'll break it. I find this unconvincing as that would
have to be the *only GPIO* in the system, which is extremely unlikely.

Suggested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---

This is a resend of a similar patch from a couple years ago.

V2:

- added paragraph to Documentation/gpio/consumer.txt at request of
  Alexandre Courbot
- added Suggested-by: Uwe Kleine-König at request of Alexandre Courbot
- hopefully addressed Uwe's concern about gpiolib being disabled in the
  patch description: I find it extremely unlikely that the optional GPIO
  would happen to be the only GPIO in the whole system.
- added handling for more _optional() calls appeared since V1 was
  posted.

 Documentation/gpio/consumer.txt |  6 ++++++
 include/linux/gpio/consumer.h   | 12 ++++++------
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/Documentation/gpio/consumer.txt b/Documentation/gpio/consumer.txt
index 05676fdacfe3..912568baabb9 100644
--- a/Documentation/gpio/consumer.txt
+++ b/Documentation/gpio/consumer.txt
@@ -70,6 +70,12 @@ instead of -ENOENT if no GPIO has been assigned to the requested function:
 						   unsigned int index,
 						   enum gpiod_flags flags)
 
+Note that gpio_get*_optional() functions (and their managed variants), unlike
+the rest of gpiolib API, also return NULL when gpiolib support is disabled.
+This is helpful to driver authors, since they do not need to special case
+-ENOSYS return codes.  System integrators should however be careful to enable
+gpiolib on systems that need it.
+
 For a function using multiple GPIOs all of those can be obtained with one call:
 
 	struct gpio_descs *gpiod_get_array(struct device *dev,
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index fb0fde686cb1..a0431f4aa6cc 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -165,14 +165,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 struct gpio_descs *__must_check
@@ -186,7 +186,7 @@ static inline struct gpio_descs *__must_check
 gpiod_get_array_optional(struct device *dev, const char *con_id,
 			 enum gpiod_flags flags)
 {
-	return ERR_PTR(-ENOSYS);
+	return NULL;
 }
 
 static inline void gpiod_put(struct gpio_desc *desc)
@@ -226,14 +226,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 struct gpio_descs *__must_check
@@ -247,7 +247,7 @@ static inline struct gpio_descs *__must_check
 devm_gpiod_get_array_optional(struct device *dev, const char *con_id,
 			      enum gpiod_flags flags)
 {
-	return ERR_PTR(-ENOSYS);
+	return NULL;
 }
 
 static inline void devm_gpiod_put(struct device *dev, struct gpio_desc *desc)
-- 
2.11.0.483.g087da7b7c-goog


-- 
Dmitry

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] gpio: return NULL from gpiod_get_optional when GPIOLIB is disabled
  2017-02-13  1:13 [PATCH v2] gpio: return NULL from gpiod_get_optional when GPIOLIB is disabled Dmitry Torokhov
@ 2017-02-13  1:15 ` Dmitry Torokhov
  2017-02-13  7:45   ` Uwe Kleine-König
  2017-02-22 16:06 ` Linus Walleij
  2017-03-14 13:31 ` Linus Walleij
  2 siblings, 1 reply; 12+ messages in thread
From: Dmitry Torokhov @ 2017-02-13  1:15 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Alexandre Courbot, linux-gpio, linux-kernel, Uwe Kleine-König

[ Forgot to add Uwe to the CC ]

On Sun, Feb 12, 2017 at 05:13:55PM -0800, Dmitry Torokhov wrote:
> Given the intent behind gpiod_get_optional() and friends it does not make
> sense to return -ENOSYS when GPIOLIB is disabled: the driver is expected to
> work just fine without gpio so let's behave as if gpio was not found.
> Otherwise we have to special-case -ENOSYS in drivers.
> 
> Note that there was objection that someone might forget to enable GPIOLIB
> when dealing with a platform that has device that actually specifies
> optional gpio and we'll break it. I find this unconvincing as that would
> have to be the *only GPIO* in the system, which is extremely unlikely.
> 
> Suggested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
> 
> This is a resend of a similar patch from a couple years ago.
> 
> V2:
> 
> - added paragraph to Documentation/gpio/consumer.txt at request of
>   Alexandre Courbot
> - added Suggested-by: Uwe Kleine-König at request of Alexandre Courbot
> - hopefully addressed Uwe's concern about gpiolib being disabled in the
>   patch description: I find it extremely unlikely that the optional GPIO
>   would happen to be the only GPIO in the whole system.
> - added handling for more _optional() calls appeared since V1 was
>   posted.
> 
>  Documentation/gpio/consumer.txt |  6 ++++++
>  include/linux/gpio/consumer.h   | 12 ++++++------
>  2 files changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/gpio/consumer.txt b/Documentation/gpio/consumer.txt
> index 05676fdacfe3..912568baabb9 100644
> --- a/Documentation/gpio/consumer.txt
> +++ b/Documentation/gpio/consumer.txt
> @@ -70,6 +70,12 @@ instead of -ENOENT if no GPIO has been assigned to the requested function:
>  						   unsigned int index,
>  						   enum gpiod_flags flags)
>  
> +Note that gpio_get*_optional() functions (and their managed variants), unlike
> +the rest of gpiolib API, also return NULL when gpiolib support is disabled.
> +This is helpful to driver authors, since they do not need to special case
> +-ENOSYS return codes.  System integrators should however be careful to enable
> +gpiolib on systems that need it.
> +
>  For a function using multiple GPIOs all of those can be obtained with one call:
>  
>  	struct gpio_descs *gpiod_get_array(struct device *dev,
> diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
> index fb0fde686cb1..a0431f4aa6cc 100644
> --- a/include/linux/gpio/consumer.h
> +++ b/include/linux/gpio/consumer.h
> @@ -165,14 +165,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 struct gpio_descs *__must_check
> @@ -186,7 +186,7 @@ static inline struct gpio_descs *__must_check
>  gpiod_get_array_optional(struct device *dev, const char *con_id,
>  			 enum gpiod_flags flags)
>  {
> -	return ERR_PTR(-ENOSYS);
> +	return NULL;
>  }
>  
>  static inline void gpiod_put(struct gpio_desc *desc)
> @@ -226,14 +226,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 struct gpio_descs *__must_check
> @@ -247,7 +247,7 @@ static inline struct gpio_descs *__must_check
>  devm_gpiod_get_array_optional(struct device *dev, const char *con_id,
>  			      enum gpiod_flags flags)
>  {
> -	return ERR_PTR(-ENOSYS);
> +	return NULL;
>  }
>  
>  static inline void devm_gpiod_put(struct device *dev, struct gpio_desc *desc)
> -- 
> 2.11.0.483.g087da7b7c-goog
> 
> 
> -- 
> Dmitry

-- 
Dmitry

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] gpio: return NULL from gpiod_get_optional when GPIOLIB is disabled
  2017-02-13  1:15 ` Dmitry Torokhov
@ 2017-02-13  7:45   ` Uwe Kleine-König
  2017-02-13  8:20     ` Uwe Kleine-König
  2017-02-13  8:20     ` Dmitry Torokhov
  0 siblings, 2 replies; 12+ messages in thread
From: Uwe Kleine-König @ 2017-02-13  7:45 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Linus Walleij, Alexandre Courbot, linux-gpio, linux-kernel

Hello,

On Sun, Feb 12, 2017 at 05:15:01PM -0800, Dmitry Torokhov wrote:
> On Sun, Feb 12, 2017 at 05:13:55PM -0800, Dmitry Torokhov wrote:
> > Given the intent behind gpiod_get_optional() and friends it does not make
> > sense to return -ENOSYS when GPIOLIB is disabled: the driver is expected to
> > work just fine without gpio so let's behave as if gpio was not found.
> > Otherwise we have to special-case -ENOSYS in drivers.
> > 
> > Note that there was objection that someone might forget to enable GPIOLIB
> > when dealing with a platform that has device that actually specifies
> > optional gpio and we'll break it. I find this unconvincing as that would
> > have to be the *only GPIO* in the system, which is extremely unlikely.
> > 
> > Suggested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

I don't like this patch and so I wonder what I wrote that could be
interpreted as suggesting this patch. For now I'd say only

	Nacked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

is justified.

My concern is still there. This might break some setups. IMHO it's not
ok to request that a device in a certain configuration only works when
the (optional) Kconfig option GPIOLIB is enabled and silently breaks if
it's not. And you cannot rely on the person who configured the kernel.

When accepting this you will burn debug time of others who see their
device breaking with no or unrelated error messages.

The only reliable way out here is to enable enough of GPIOLIB to only
return NULL in ..._optional when there is no gpio required. You can have
a suggested-by for that.

The semantic of gpiod_get_optional is:

	if there is a gpio:
		give it to me
	else:
		give me a dummy

If the kernel is configured to be unable to answer the question "is
there a gpio?" that is worth a -ENOSYS.

Best regards
Uwe

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] gpio: return NULL from gpiod_get_optional when GPIOLIB is disabled
  2017-02-13  7:45   ` Uwe Kleine-König
@ 2017-02-13  8:20     ` Uwe Kleine-König
  2017-02-13  8:20     ` Dmitry Torokhov
  1 sibling, 0 replies; 12+ messages in thread
From: Uwe Kleine-König @ 2017-02-13  8:20 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Linus Walleij, Alexandre Courbot, linux-gpio, linux-kernel

On Mon, Feb 13, 2017 at 08:45:06AM +0100, Uwe Kleine-König wrote:
> Hello,
> 
> On Sun, Feb 12, 2017 at 05:15:01PM -0800, Dmitry Torokhov wrote:
> > On Sun, Feb 12, 2017 at 05:13:55PM -0800, Dmitry Torokhov wrote:
> > > Given the intent behind gpiod_get_optional() and friends it does not make
> > > sense to return -ENOSYS when GPIOLIB is disabled: the driver is expected to
> > > work just fine without gpio so let's behave as if gpio was not found.
> > > Otherwise we have to special-case -ENOSYS in drivers.
> > > 
> > > Note that there was objection that someone might forget to enable GPIOLIB
> > > when dealing with a platform that has device that actually specifies
> > > optional gpio and we'll break it. I find this unconvincing as that would
> > > have to be the *only GPIO* in the system, which is extremely unlikely.
> > > 
> > > Suggested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> I don't like this patch and so I wonder what I wrote that could be
> interpreted as suggesting this patch. For now I'd say only
> 
> 	Nacked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> is justified.

Oh, it seems I really sent such a RFC patch some time ago. Still I think
it's wrong to do that and that we need something like a
lookup-only-GPIOLIB that implements:

	def gpio_get_optional(...):
		if a gpio is specified:
			return -ENOSYS
		else:
			return NULL

if you really want save some bytes and disable the full-fledged GPIOLIB.

Best regards
Uwe

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] gpio: return NULL from gpiod_get_optional when GPIOLIB is disabled
  2017-02-13  7:45   ` Uwe Kleine-König
  2017-02-13  8:20     ` Uwe Kleine-König
@ 2017-02-13  8:20     ` Dmitry Torokhov
  2017-02-13  8:59       ` Uwe Kleine-König
  1 sibling, 1 reply; 12+ messages in thread
From: Dmitry Torokhov @ 2017-02-13  8:20 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Linus Walleij, Alexandre Courbot, linux-gpio, linux-kernel

On Mon, Feb 13, 2017 at 08:45:06AM +0100, Uwe Kleine-König wrote:
> Hello,
> 
> On Sun, Feb 12, 2017 at 05:15:01PM -0800, Dmitry Torokhov wrote:
> > On Sun, Feb 12, 2017 at 05:13:55PM -0800, Dmitry Torokhov wrote:
> > > Given the intent behind gpiod_get_optional() and friends it does not make
> > > sense to return -ENOSYS when GPIOLIB is disabled: the driver is expected to
> > > work just fine without gpio so let's behave as if gpio was not found.
> > > Otherwise we have to special-case -ENOSYS in drivers.
> > > 
> > > Note that there was objection that someone might forget to enable GPIOLIB
> > > when dealing with a platform that has device that actually specifies
> > > optional gpio and we'll break it. I find this unconvincing as that would
> > > have to be the *only GPIO* in the system, which is extremely unlikely.
> > > 
> > > Suggested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> I don't like this patch and so I wonder what I wrote that could be
> interpreted as suggesting this patch.

It came from my exchange with Alexandre:

On Fri, Feb 20, 2015 at 01:59:43PM +0900, Alexandre Courbot wrote:
> On Fri, Feb 20, 2015 at 9:30 AM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > Given the intent behind gpiod_get_optional() and friends it does not
> > make
> > sense to return -ENOSYS when GPIOLIB is disabled: the driver is
> > expected to
> > work just fine without gpio so let's behave as if gpio was not
> > found.
> > Otherwise we have to special-case -ENOSYS in drivers.
>
> Interestingly Uwe sent a RFC for this one week ago:
>
> http://patchwork.ozlabs.org/patch/439135/
>
> Maybe credit him with a Suggested-by.?

If you check out that patchwork entry you indeed suggested doing exactly
what this patch is doing. But if you insist I can certainly remove the
"suggested-by".

> For now I'd say only
> 
> 	Nacked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> is justified.
> 
> My concern is still there. This might break some setups. IMHO it's not
> ok to request that a device in a certain configuration only works when
> the (optional) Kconfig option GPIOLIB is enabled and silently breaks if
> it's not. And you cannot rely on the person who configured the kernel.

Really? So I guess we can tell people do just do "make randconfig"
and start reporting bugs when that kernel would not work on their
hardware?

> 
> When accepting this you will burn debug time of others who see their
> device breaking with no or unrelated error messages.
> 
> The only reliable way out here is to enable enough of GPIOLIB to only
> return NULL in ..._optional when there is no gpio required. You can have
> a suggested-by for that.
> 
> The semantic of gpiod_get_optional is:
> 
> 	if there is a gpio:
> 		give it to me
> 	else:
> 		give me a dummy
> 
> If the kernel is configured to be unable to answer the question "is
> there a gpio?" that is worth a -ENOSYS.

You know what every single driver for peripherals that are used on
variety of systems using has to do for gpios that are truly optional?

	gpio = gpio_get_optional();
	if (IS_ERR(gpio)) {
		error = PTR_ERR(gpio);
		if (error == -ENOSYS) {
			/*
			 * - It's OK, we said it's optional GPIO so
			 * if GPIOLIB is not there we should not fail
			 * to enable the device, because this gpio is
			 * *OPTIONAL*.
			 * - But what if it is actually there? What if the
			 * kernel is misconfigured?
			 * - And what if it is not? How would we know?
			 * - Let's parse device tree by hand! And check
			 * ACPI. And if ACPI is disabled reimplement it
			 * because we should not silently break users!
			 * - ... no, let's not.
			 */
			gpio = NULL;
		} else {
			return error;
		}
	}

Really, the argument that there is a *single* GPIO in the whole system,
that GPIO happens to be optional, but critical for the deice operation
and a random person is confused when enabling random options and
forgetting gpiolib? And to "save" this scenario you prefer to fail
loading drivers on perfectly configured systems that do not need gpiolib
and do not use gpios? Or to add -ENOSYS checks (that actually break
the scenario you described and make the driver code ugly)? I think you
have your priorities wrong.

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] gpio: return NULL from gpiod_get_optional when GPIOLIB is disabled
  2017-02-13  8:20     ` Dmitry Torokhov
@ 2017-02-13  8:59       ` Uwe Kleine-König
  2017-02-13 17:32         ` Dmitry Torokhov
  0 siblings, 1 reply; 12+ messages in thread
From: Uwe Kleine-König @ 2017-02-13  8:59 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Linus Walleij, Alexandre Courbot, linux-gpio, linux-kernel

Hello Dmitry,

On Mon, Feb 13, 2017 at 12:20:08AM -0800, Dmitry Torokhov wrote:
> On Mon, Feb 13, 2017 at 08:45:06AM +0100, Uwe Kleine-König wrote:
> > My concern is still there. This might break some setups. IMHO it's not
> > ok to request that a device in a certain configuration only works when
> > the (optional) Kconfig option GPIOLIB is enabled and silently breaks if
> > it's not. And you cannot rely on the person who configured the kernel.
> 
> Really? So I guess we can tell people do just do "make randconfig"
> and start reporting bugs when that kernel would not work on their
> hardware?

No, but if randconfig enabled a driver for your temperature sensor and
that driver manages to try binding to that device, it should fail if the
temperature sensor might have a GPIO that must be taken into account for
proper functioning if it's there and the driver cannot find out if it's
there.

If your driver can handle a GPIO that can also safely be ignored, then
sure, this function's semantic is a nuisance. But then given that this
semantic is what most drivers should use AFAICT the right thing is to
introduce another function with the semantic:

	if there is a gpio and GPIOLIB is enabled:
		return gpio
	else:
		return NULL

> > When accepting this you will burn debug time of others who see their
> > device breaking with no or unrelated error messages.
> > 
> > The only reliable way out here is to enable enough of GPIOLIB to only
> > return NULL in ..._optional when there is no gpio required. You can have
> > a suggested-by for that.
> > 
> > The semantic of gpiod_get_optional is:
> > 
> > 	if there is a gpio:
> > 		give it to me
> > 	else:
> > 		give me a dummy
> > 
> > If the kernel is configured to be unable to answer the question "is
> > there a gpio?" that is worth a -ENOSYS.
> 
> You know what every single driver for peripherals that are used on
> variety of systems using has to do for gpios that are truly optional?

What is the difference between "truly optional" and the meaning of
optional in gpiod_get_optional?

> 	gpio = gpio_get_optional();
> 	if (IS_ERR(gpio)) {
> 		error = PTR_ERR(gpio);
> 		if (error == -ENOSYS) {
> 			/*
> 			 * - It's OK, we said it's optional GPIO so
> 			 * if GPIOLIB is not there we should not fail
> 			 * to enable the device, because this gpio is
> 			 * *OPTIONAL*.
> 			 * - But what if it is actually there? What if the
> 			 * kernel is misconfigured?
> 			 * - And what if it is not? How would we know?
> 			 * - Let's parse device tree by hand! And check
> 			 * ACPI. And if ACPI is disabled reimplement it
> 			 * because we should not silently break users!
> 			 * - ... no, let's not.
> 			 */
> 			gpio = NULL;
> 		} else {
> 			return error;
> 		}
> 	}

I don't know a good name, but implementing a separate helper function
with this semantic is great. It simplifies all these drivers, makes them
easily greppable for, and lets the drivers that want the reliable
semantic just continue to use gpiod_get_optional().

Can you list a few drivers that should make use of this new function
instead of gpiod_get_optional()?

Or alternatively (if "truly optional" means: I don't need to do anything
even if it's available) just do the following instead:

	/*
	 * There might be a gpio but to not make the driver depend
	 * on GPIOLIB we don't make use of it.
	 */
	gpio = NULL;

Best regards
Uwe

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] gpio: return NULL from gpiod_get_optional when GPIOLIB is disabled
  2017-02-13  8:59       ` Uwe Kleine-König
@ 2017-02-13 17:32         ` Dmitry Torokhov
  0 siblings, 0 replies; 12+ messages in thread
From: Dmitry Torokhov @ 2017-02-13 17:32 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Linus Walleij, Alexandre Courbot, linux-gpio, linux-kernel

Hi Uwe,

On Mon, Feb 13, 2017 at 09:59:35AM +0100, Uwe Kleine-König wrote:
> Hello Dmitry,
> 
> On Mon, Feb 13, 2017 at 12:20:08AM -0800, Dmitry Torokhov wrote:
> > On Mon, Feb 13, 2017 at 08:45:06AM +0100, Uwe Kleine-König wrote:
> > > My concern is still there. This might break some setups. IMHO it's not
> > > ok to request that a device in a certain configuration only works when
> > > the (optional) Kconfig option GPIOLIB is enabled and silently breaks if
> > > it's not. And you cannot rely on the person who configured the kernel.
> > 
> > Really? So I guess we can tell people do just do "make randconfig"
> > and start reporting bugs when that kernel would not work on their
> > hardware?
> 
> No, but if randconfig enabled a driver for your temperature sensor and
> that driver manages to try binding to that device, it should fail if the
> temperature sensor might have a GPIO that must be taken into account for
> proper functioning if it's there and the driver cannot find out if it's
> there.

I disagree. The gpio might be there, the regulator might be there, they
might be physically present, but not described in DT, ACPI might be
disabled, another subsystem might be disabled, kernel parameters might
be wrong, additional DT data might be wrong. We are not going to "fix"
everything.

> 
> If your driver can handle a GPIO that can also safely be ignored, then
> sure, this function's semantic is a nuisance. But then given that this
> semantic is what most drivers should use AFAICT the right thing is to
> introduce another function with the semantic:
> 
> 	if there is a gpio and GPIOLIB is enabled:
> 		return gpio
> 	else:
> 		return NULL

gpiod_get_optional_I_really_mean_it_scouts_honor()?

> 
> > > When accepting this you will burn debug time of others who see their
> > > device breaking with no or unrelated error messages.
> > > 
> > > The only reliable way out here is to enable enough of GPIOLIB to only
> > > return NULL in ..._optional when there is no gpio required. You can have
> > > a suggested-by for that.
> > > 
> > > The semantic of gpiod_get_optional is:
> > > 
> > > 	if there is a gpio:
> > > 		give it to me
> > > 	else:
> > > 		give me a dummy
> > > 
> > > If the kernel is configured to be unable to answer the question "is
> > > there a gpio?" that is worth a -ENOSYS.
> > 
> > You know what every single driver for peripherals that are used on
> > variety of systems using has to do for gpios that are truly optional?
> 
> What is the difference between "truly optional" and the meaning of
> optional in gpiod_get_optional?

I do not see there is anything different, at least for me. The drivers
that believe they need to know about "true" presence of optional GPIOs
are free to add "depend GPIOLIB" to their Kconfig entries.

> 
> > 	gpio = gpio_get_optional();
> > 	if (IS_ERR(gpio)) {
> > 		error = PTR_ERR(gpio);
> > 		if (error == -ENOSYS) {
> > 			/*
> > 			 * - It's OK, we said it's optional GPIO so
> > 			 * if GPIOLIB is not there we should not fail
> > 			 * to enable the device, because this gpio is
> > 			 * *OPTIONAL*.
> > 			 * - But what if it is actually there? What if the
> > 			 * kernel is misconfigured?
> > 			 * - And what if it is not? How would we know?
> > 			 * - Let's parse device tree by hand! And check
> > 			 * ACPI. And if ACPI is disabled reimplement it
> > 			 * because we should not silently break users!
> > 			 * - ... no, let's not.
> > 			 */
> > 			gpio = NULL;
> > 		} else {
> > 			return error;
> > 		}
> > 	}
> 
> I don't know a good name, but implementing a separate helper function
> with this semantic is great. It simplifies all these drivers, makes them
> easily greppable for, and lets the drivers that want the reliable
> semantic just continue to use gpiod_get_optional().

Or they can say "depends on GPIOLIB" and get that reliable semantics
without introducing new API.

> 
> Can you list a few drivers that should make use of this new function
> instead of gpiod_get_optional()?

Instead? None. But I consider all of the following:

drivers/input/misc/drv260x.c:   haptics->enable_gpio = devm_gpiod_get_optional(dev, "enable",
drivers/input/touchscreen/cyttsp_core.c:        ts->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
drivers/input/touchscreen/edt-ft5x06.c: tsdata->reset_gpio = devm_gpiod_get_optional(&client->dev,
drivers/input/touchscreen/edt-ft5x06.c: tsdata->wake_gpio = devm_gpiod_get_optional(&client->dev,
drivers/input/touchscreen/goodix.c:     gpiod = devm_gpiod_get_optional(dev, GOODIX_GPIO_INT_NAME, GPIOD_IN);
drivers/input/touchscreen/goodix.c:     gpiod = devm_gpiod_get_optional(dev, GOODIX_GPIO_RST_NAME, GPIOD_IN);
drivers/input/touchscreen/melfas_mip4.c:        ts->gpio_ce = devm_gpiod_get_optional(&client->dev,
drivers/input/touchscreen/pixcir_i2c_ts.c:      tsdata->gpio_reset = devm_gpiod_get_optional(dev, "reset",
drivers/input/touchscreen/pixcir_i2c_ts.c:      tsdata->gpio_wake = devm_gpiod_get_optional(dev, "wake",
drivers/input/touchscreen/pixcir_i2c_ts.c:      tsdata->gpio_enable = devm_gpiod_get_optional(dev, "enable",
drivers/input/touchscreen/raydium_i2c_ts.c:     ts->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset",
drivers/input/touchscreen/silead.c:     data->gpio_power = devm_gpiod_get_optional(dev, "power", GPIOD_OUT_LOW);
drivers/input/touchscreen/sis_i2c.c:    ts->attn_gpio = devm_gpiod_get_optional(&client->dev,
drivers/input/touchscreen/sis_i2c.c:    ts->reset_gpio = devm_gpiod_get_optional(&client->dev,
drivers/input/touchscreen/tsc200x-core.c:       ts->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
drivers/input/touchscreen/zforce_ts.c:  ts->gpio_rst = devm_gpiod_get_optional(&client->dev, "reset",
drivers/input/touchscreen/zforce_ts.c:          ts->gpio_int = devm_gpiod_get_optional(&client->dev, "irq",

slightly broken right now. Some of them are working around not having to
care about -ENOSYS by using "depends on GPIOLIB || COMPILE_TEST" and
forcing enabling gpiolib even on systems where there are no gpios
exposed to the drivers (let's say there is an ACPI system where all
gpios are ACPI owned and not to be touched by the kernel).

> 
> Or alternatively (if "truly optional" means: I don't need to do anything
> even if it's available) just do the following instead:
> 
> 	/*
> 	 * There might be a gpio but to not make the driver depend
> 	 * on GPIOLIB we don't make use of it.
> 	 */
> 	gpio = NULL;

That is not really helpful.

OK, I do not think we'll have a meeting of minds here, as I strongly
believe that configuring kernel is something that is not done randomly
and "randconfig" option is not something that we should try to make
working while you advocating for some developer blindly enabling and
disabling kernel options and the rest of the kernel developers taking
pains as to not inconvenience said developer. So let's leave this to
Linus and Alexandre to decide if they believe my (and your older)
proposal makes sense or not.

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] gpio: return NULL from gpiod_get_optional when GPIOLIB is disabled
  2017-02-13  1:13 [PATCH v2] gpio: return NULL from gpiod_get_optional when GPIOLIB is disabled Dmitry Torokhov
  2017-02-13  1:15 ` Dmitry Torokhov
@ 2017-02-22 16:06 ` Linus Walleij
  2017-02-22 18:27   ` Dmitry Torokhov
  2017-02-22 18:44   ` Mark Brown
  2017-03-14 13:31 ` Linus Walleij
  2 siblings, 2 replies; 12+ messages in thread
From: Linus Walleij @ 2017-02-22 16:06 UTC (permalink / raw)
  To: Dmitry Torokhov, Mark Brown; +Cc: Alexandre Courbot, linux-gpio, linux-kernel

On Mon, Feb 13, 2017 at 2:13 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:

> Given the intent behind gpiod_get_optional() and friends it does not make
> sense to return -ENOSYS when GPIOLIB is disabled: the driver is expected to
> work just fine without gpio so let's behave as if gpio was not found.
> Otherwise we have to special-case -ENOSYS in drivers.
>
> Note that there was objection that someone might forget to enable GPIOLIB
> when dealing with a platform that has device that actually specifies
> optional gpio and we'll break it. I find this unconvincing as that would
> have to be the *only GPIO* in the system, which is extremely unlikely.
>
> (...)
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>
> This is a resend of a similar patch from a couple years ago.

I want to get an indication from Mark Brown on how he see this
semantic compared to how regulators work.

It makes most sense to developers to have the same kind of
semantics in optional GPIOs as in optional regulators.

For optional regulators, if I understand correctly, these are
*electrically optional* as in: a voltage input pin on a chip that
the chip can very well live without. The regulator may be there,
it may not, it may affect the function of the chip but it's still OK
without it.

For this reason regulators will return an error if an optional regulator
is not present, and the code is expected to deal with it.

It is a common misconception that the "optional" part of the
regulator API call means "software optional". It is not the semantic
of this call.

It is also tagged __must_check and return an error when compiled
out. They return -ENODEV, see <linux/regulator/consumer.h>

I would be happy for a patch switching out return value to -ENODEV
for sure :)

Now second: why should the GPIO semantic be different from what
regulators is using?

Consistency is important with regards to Rusty Russell's API
rules, so we should try to have the same semantic:
http://ozlabs.org/~rusty/index.cgi/tech/2008-03-30.html

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] gpio: return NULL from gpiod_get_optional when GPIOLIB is disabled
  2017-02-22 16:06 ` Linus Walleij
@ 2017-02-22 18:27   ` Dmitry Torokhov
  2017-02-22 18:49     ` Mark Brown
  2017-02-22 18:44   ` Mark Brown
  1 sibling, 1 reply; 12+ messages in thread
From: Dmitry Torokhov @ 2017-02-22 18:27 UTC (permalink / raw)
  To: Linus Walleij; +Cc: Mark Brown, Alexandre Courbot, linux-gpio, linux-kernel

Hi Linus,

On Wed, Feb 22, 2017 at 05:06:35PM +0100, Linus Walleij wrote:
> On Mon, Feb 13, 2017 at 2:13 AM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> 
> > Given the intent behind gpiod_get_optional() and friends it does not make
> > sense to return -ENOSYS when GPIOLIB is disabled: the driver is expected to
> > work just fine without gpio so let's behave as if gpio was not found.
> > Otherwise we have to special-case -ENOSYS in drivers.
> >
> > Note that there was objection that someone might forget to enable GPIOLIB
> > when dealing with a platform that has device that actually specifies
> > optional gpio and we'll break it. I find this unconvincing as that would
> > have to be the *only GPIO* in the system, which is extremely unlikely.
> >
> > (...)
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> >
> > This is a resend of a similar patch from a couple years ago.
> 
> I want to get an indication from Mark Brown on how he see this
> semantic compared to how regulators work.

This is completely orthogonal to this patch though. This patch tries to
solve problem that API behaves differently when GPIOLIB is disabled, so
the driver has to check for both NULL and ENOSYS. I would like to make
gpiod_get_optional() return NULL when GPIOLIB is disabled.

> 
> It makes most sense to developers to have the same kind of
> semantics in optional GPIOs as in optional regulators.

I agree, here. Unfortunately optional regulators return -ENOENT and it
will take an effort to change it to NULL if we can persuade Mark that it
makes sense.

However, there are more optional GPIOs than regulators, so switching
gpio to return -ENODEV even bigger task. Besides, I believe that doing:

	gpio = gpiod_get_optional();
	if (IS_ERR(gpio))
		return PTR_ERR(gpio);

is better than:

	gpio = gpiod_get_optional();
	if ((IS_ERR(gpio)) {
		error = PTR_ERR(gpio);
		if (error != -ENODEV)
			return error;
		gpio = NULL;
	}

> 
> For optional regulators, if I understand correctly, these are
> *electrically optional* as in: a voltage input pin on a chip that
> the chip can very well live without. The regulator may be there,
> it may not, it may affect the function of the chip but it's still OK
> without it.
> 
> For this reason regulators will return an error if an optional regulator
> is not present, and the code is expected to deal with it.
> 
> It is a common misconception that the "optional" part of the
> regulator API call means "software optional". It is not the semantic
> of this call.
> 
> It is also tagged __must_check and return an error when compiled
> out. They return -ENODEV, see <linux/regulator/consumer.h>

The point is that they return the same value when regulator is not
present and when regulator support is compiled out. GPIOLIB returns NULL
and -ENOSYS respectively, and I contend that this is wrong and needs to
be resolved first.

> 
> I would be happy for a patch switching out return value to -ENODEV
> for sure :)

I as shown is the example above I think NULL is better for optionals.

> 
> Now second: why should the GPIO semantic be different from what
> regulators is using?
> 
> Consistency is important with regards to Rusty Russell's API
> rules, so we should try to have the same semantic:
> http://ozlabs.org/~rusty/index.cgi/tech/2008-03-30.html

I agree that if we can converge on the similar behavior it would be
best, but it is bigger (and separate) task. First I think we need to
make API behave sane when support is enabled and when it is disabled.

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] gpio: return NULL from gpiod_get_optional when GPIOLIB is disabled
  2017-02-22 16:06 ` Linus Walleij
  2017-02-22 18:27   ` Dmitry Torokhov
@ 2017-02-22 18:44   ` Mark Brown
  1 sibling, 0 replies; 12+ messages in thread
From: Mark Brown @ 2017-02-22 18:44 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Dmitry Torokhov, Alexandre Courbot, linux-gpio, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 1272 bytes --]

On Wed, Feb 22, 2017 at 05:06:35PM +0100, Linus Walleij wrote:

> For optional regulators, if I understand correctly, these are
> *electrically optional* as in: a voltage input pin on a chip that
> the chip can very well live without. The regulator may be there,
> it may not, it may affect the function of the chip but it's still OK
> without it.

> For this reason regulators will return an error if an optional regulator
> is not present, and the code is expected to deal with it.

Yes.

> It is a common misconception that the "optional" part of the
> regulator API call means "software optional". It is not the semantic
> of this call.

> It is also tagged __must_check and return an error when compiled
> out. They return -ENODEV, see <linux/regulator/consumer.h>

Right.  Dmitry actually sent a patch a few weeks ago proposing a change
in this behaviour for the regulator API which I didn't apply.  I'm
mainly worried that this would the already encourage common broken
patterns with people just not writing error handling code properly and
the incorrect software optional assumption you mention above.  The
expectation is that the majority of drivers with optional regulators
will need to do some explicit handling whenever they would interact with
the regulator.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 484 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] gpio: return NULL from gpiod_get_optional when GPIOLIB is disabled
  2017-02-22 18:27   ` Dmitry Torokhov
@ 2017-02-22 18:49     ` Mark Brown
  0 siblings, 0 replies; 12+ messages in thread
From: Mark Brown @ 2017-02-22 18:49 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Linus Walleij, Alexandre Courbot, linux-gpio, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 335 bytes --]

On Wed, Feb 22, 2017 at 10:27:25AM -0800, Dmitry Torokhov wrote:

> The point is that they return the same value when regulator is not
> present and when regulator support is compiled out. GPIOLIB returns NULL
> and -ENOSYS respectively, and I contend that this is wrong and needs to
> be resolved first.

FWIW this makes sense to me.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] gpio: return NULL from gpiod_get_optional when GPIOLIB is disabled
  2017-02-13  1:13 [PATCH v2] gpio: return NULL from gpiod_get_optional when GPIOLIB is disabled Dmitry Torokhov
  2017-02-13  1:15 ` Dmitry Torokhov
  2017-02-22 16:06 ` Linus Walleij
@ 2017-03-14 13:31 ` Linus Walleij
  2 siblings, 0 replies; 12+ messages in thread
From: Linus Walleij @ 2017-03-14 13:31 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: Alexandre Courbot, linux-gpio, linux-kernel

On Mon, Feb 13, 2017 at 2:13 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:

> Given the intent behind gpiod_get_optional() and friends it does not make
> sense to return -ENOSYS when GPIOLIB is disabled: the driver is expected to
> work just fine without gpio so let's behave as if gpio was not found.
> Otherwise we have to special-case -ENOSYS in drivers.
>
> Note that there was objection that someone might forget to enable GPIOLIB
> when dealing with a platform that has device that actually specifies
> optional gpio and we'll break it. I find this unconvincing as that would
> have to be the *only GPIO* in the system, which is extremely unlikely.
>
> Suggested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>
> This is a resend of a similar patch from a couple years ago.
>
> V2:

After a lot of decision angst I applied this patch, dropping Uwe's
Suggested-by since he doesn't seem to like it.

I think we should eventually return proper error codes,
but as you say: the API should be consistent first, we need
to change both the lib and the stubs to return error codes
for that, this is about something else, just keeping it consistent.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2017-03-14 13:31 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-13  1:13 [PATCH v2] gpio: return NULL from gpiod_get_optional when GPIOLIB is disabled Dmitry Torokhov
2017-02-13  1:15 ` Dmitry Torokhov
2017-02-13  7:45   ` Uwe Kleine-König
2017-02-13  8:20     ` Uwe Kleine-König
2017-02-13  8:20     ` Dmitry Torokhov
2017-02-13  8:59       ` Uwe Kleine-König
2017-02-13 17:32         ` Dmitry Torokhov
2017-02-22 16:06 ` Linus Walleij
2017-02-22 18:27   ` Dmitry Torokhov
2017-02-22 18:49     ` Mark Brown
2017-02-22 18:44   ` Mark Brown
2017-03-14 13:31 ` Linus Walleij

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).