Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1579340 > unrolled thread

[PATCH v2] gpio: return NULL from gpiod_get_optional when GPIOLIB is disabled

Back to article view | Back to linux.kernel

Contents

  [PATCH v2] gpio: return NULL from gpiod_get_optional when GPIOLIB is  disabled Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-02-13 02:20 +0100
    Re: [PATCH v2] gpio: return NULL from gpiod_get_optional when  GPIOLIB is disabled Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-02-13 02:20 +0100
      Re: [PATCH v2] gpio: return NULL from gpiod_get_optional when  GPIOLIB is disabled Uwe Kleine-König          <u.kleine-koenig@pengutronix.de> - 2017-02-13 08:50 +0100
        Re: [PATCH v2] gpio: return NULL from gpiod_get_optional when  GPIOLIB is disabled Uwe Kleine-König          <u.kleine-koenig@pengutronix.de> - 2017-02-13 09:30 +0100
        Re: [PATCH v2] gpio: return NULL from gpiod_get_optional when  GPIOLIB is disabled Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-02-13 09:30 +0100
          Re: [PATCH v2] gpio: return NULL from gpiod_get_optional when  GPIOLIB is disabled Uwe Kleine-König          <u.kleine-koenig@pengutronix.de> - 2017-02-13 10:00 +0100
            Re: [PATCH v2] gpio: return NULL from gpiod_get_optional when  GPIOLIB is disabled Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-02-13 18:40 +0100

#1579340 — [PATCH v2] gpio: return NULL from gpiod_get_optional when GPIOLIB is disabled

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

[toc] | [next] | [standalone]

#1579342 — Re: [PATCH v2] gpio: return NULL from gpiod_get_optional when GPIOLIB is disabled

[ 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

[toc] | [prev] | [next] | [standalone]

#1579489 — Re: [PATCH v2] gpio: return NULL from gpiod_get_optional when GPIOLIB is disabled

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

[toc] | [prev] | [next] | [standalone]

#1579517 — Re: [PATCH v2] gpio: return NULL from gpiod_get_optional when GPIOLIB is disabled

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

[toc] | [prev] | [next] | [standalone]

#1579518 — Re: [PATCH v2] gpio: return NULL from gpiod_get_optional when GPIOLIB is disabled

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

[toc] | [prev] | [next] | [standalone]

#1579532 — Re: [PATCH v2] gpio: return NULL from gpiod_get_optional when GPIOLIB is disabled

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

[toc] | [prev] | [next] | [standalone]

#1579964 — Re: [PATCH v2] gpio: return NULL from gpiod_get_optional when GPIOLIB is disabled

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web