[PATCH] gpiolib: return -ENOENT when no GPIO mapping exists

[PATCH] gpiolib: return -ENOENT when no GPIO mapping exists

[Alexandre Courbot]

Some devices drivers make use of optional GPIO parameters. For such
drivers, it is important to discriminate between the case where no
GPIO mapping has been defined for the function they are requesting, and
the case where a mapping exists but an error occured while resolving it
or when acquiring the GPIO.

This patch changes the family of gpiod_get() functions such that they
will return -ENOENT if and only if no GPIO mapping is defined for the
requested function. Other error codes are used when an actual error
occured during the GPIO resolution.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
I think this change should be merged early as not having it may prevent
some users to switch to gpiod. I stumbled upon this issue while
considering porting a simple driver (pwm_bl) that has an optional GPIO
parameter.

Mika, Andy: if Linus agrees with this change, could you take care of
having -ENOENT returned as well for the ACPI and SFI GPIOs lookup?
My understanding of ACPI was not sufficient to allow me to do it myself.
SFI OTOH should be trivial as it is a simple table.

 Documentation/gpio/consumer.txt |  6 +++++-
 drivers/gpio/gpiolib.c          | 31 ++++++++++++++++---------------
 2 files changed, 21 insertions(+), 16 deletions(-)

diff --git a/Documentation/gpio/consumer.txt b/Documentation/gpio/consumer.txt
index 07c74a3765a0..e42f77d8d4ca 100644
--- a/Documentation/gpio/consumer.txt
+++ b/Documentation/gpio/consumer.txt
@@ -38,7 +38,11 @@ device that displays digits), an additional index argument can be specified:
 					  const char *con_id, unsigned int idx)
 
 Both functions return either a valid GPIO descriptor, or an error code checkable
-with IS_ERR(). They will never return a NULL pointer.
+with IS_ERR() (they will never return a NULL pointer). -ENOENT will be returned
+if and only if no GPIO has been assigned to the device/function/index triplet,
+other error codes are used for cases where a GPIO has been assigned but an error
+occured while trying to acquire it. This is useful to discriminate between mere
+errors and an absence of GPIO for optional GPIO parameters.
 
 Device-managed variants of these functions are also defined:
 
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 5fad38fcd701..e96d4a90c0c3 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2358,7 +2358,7 @@ static struct gpio_desc *gpiod_find(struct device *dev, const char *con_id,
 				    unsigned int idx,
 				    enum gpio_lookup_flags *flags)
 {
-	struct gpio_desc *desc = ERR_PTR(-ENODEV);
+	struct gpio_desc *desc = ERR_PTR(-ENOENT);
 	struct gpiod_lookup_table *table;
 	struct gpiod_lookup *p;
 
@@ -2380,19 +2380,21 @@ static struct gpio_desc *gpiod_find(struct device *dev, const char *con_id,
 		chip = find_chip_by_name(p->chip_label);
 
 		if (!chip) {
-			dev_warn(dev, "cannot find GPIO chip %s\n",
-				 p->chip_label);
-			continue;
+			dev_err(dev, "cannot find GPIO chip %s\n",
+				p->chip_label);
+			return ERR_PTR(-ENODEV);
 		}
 
 		if (chip->ngpio <= p->chip_hwnum) {
-			dev_warn(dev, "GPIO chip %s has %d GPIOs\n",
-				 chip->label, chip->ngpio);
-			continue;
+			dev_err(dev, "requested GPIO %d but chip %s has %d\n",
+				idx, chip->label, chip->ngpio);
+			return ERR_PTR(-EINVAL);
 		}
 
 		desc = gpiochip_offset_to_desc(chip, p->chip_hwnum);
 		*flags = p->flags;
+
+		return desc;
 	}
 
 	return desc;
@@ -2404,7 +2406,8 @@ static struct gpio_desc *gpiod_find(struct device *dev, const char *con_id,
  * @con_id:	function within the GPIO consumer
  *
  * Return the GPIO descriptor corresponding to the function con_id of device
- * dev, or an IS_ERR() condition if an error occured.
+ * dev, -ENOENT if no GPIO has been assigned to the requested function, or
+ * another IS_ERR() code if an error occured while trying to acquire the GPIO.
  */
 struct gpio_desc *__must_check gpiod_get(struct device *dev, const char *con_id)
 {
@@ -2421,7 +2424,9 @@ EXPORT_SYMBOL_GPL(gpiod_get);
  * This variant of gpiod_get() allows to access GPIOs other than the first
  * defined one for functions that define several GPIOs.
  *
- * Return a valid GPIO descriptor, or an IS_ERR() condition in case of error.
+ * Return a valid GPIO descriptor, -ENOENT if no GPIO has been assigned to the
+ * requested function and/or index, or another IS_ERR() code if an error
+ * occured while trying to acquire the GPIO.
  */
 struct gpio_desc *__must_check gpiod_get_index(struct device *dev,
 					       const char *con_id,
@@ -2446,13 +2451,9 @@ struct gpio_desc *__must_check gpiod_get_index(struct device *dev,
 	 * Either we are not using DT or ACPI, or their lookup did not return
 	 * a result. In that case, use platform lookup as a fallback.
 	 */
-	if (!desc || IS_ERR(desc)) {
-		struct gpio_desc *pdesc;
+	if (!desc || desc == ERR_PTR(-ENOENT)) {
 		dev_dbg(dev, "using lookup tables for GPIO lookup");
-		pdesc = gpiod_find(dev, con_id, idx, &flags);
-		/* If used as fallback, do not replace the previous error */
-		if (!IS_ERR(pdesc) || !desc)
-			desc = pdesc;
+		desc = gpiod_find(dev, con_id, idx, &flags);
 	}
 
 	if (IS_ERR(desc)) {
-- 
1.8.4.2

Re: [PATCH] gpiolib: return -ENOENT when no GPIO mapping exists

[Andy Shevchenko]

On Fri, 2013-12-06 at 11:06 +0900, Alexandre Courbot wrote:
> Some devices drivers make use of optional GPIO parameters. For such
> drivers, it is important to discriminate between the case where no
> GPIO mapping has been defined for the function they are requesting, and
> the case where a mapping exists but an error occured while resolving it
> or when acquiring the GPIO.
> 
> This patch changes the family of gpiod_get() functions such that they
> will return -ENOENT if and only if no GPIO mapping is defined for the
> requested function. Other error codes are used when an actual error
> occured during the GPIO resolution.
> 

I like the idea.
One minor comment below (in code).

> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
> I think this change should be merged early as not having it may prevent
> some users to switch to gpiod. I stumbled upon this issue while
> considering porting a simple driver (pwm_bl) that has an optional GPIO
> parameter.
> 
> Mika, Andy: if Linus agrees with this change, could you take care of
> having -ENOENT returned as well for the ACPI and SFI GPIOs lookup?

I have already switched to -ENOENT, so, consider done.

> My understanding of ACPI was not sufficient to allow me to do it myself.
> SFI OTOH should be trivial as it is a simple table.
> 
>  Documentation/gpio/consumer.txt |  6 +++++-
>  drivers/gpio/gpiolib.c          | 31 ++++++++++++++++---------------
>  2 files changed, 21 insertions(+), 16 deletions(-)
> 
> diff --git a/Documentation/gpio/consumer.txt b/Documentation/gpio/consumer.txt
> index 07c74a3765a0..e42f77d8d4ca 100644
> --- a/Documentation/gpio/consumer.txt
> +++ b/Documentation/gpio/consumer.txt
> @@ -38,7 +38,11 @@ device that displays digits), an additional index argument can be specified:
>  					  const char *con_id, unsigned int idx)
>  
>  Both functions return either a valid GPIO descriptor, or an error code checkable
> -with IS_ERR(). They will never return a NULL pointer.
> +with IS_ERR() (they will never return a NULL pointer). -ENOENT will be returned
> +if and only if no GPIO has been assigned to the device/function/index triplet,
> +other error codes are used for cases where a GPIO has been assigned but an error
> +occured while trying to acquire it. This is useful to discriminate between mere
> +errors and an absence of GPIO for optional GPIO parameters.
>  
>  Device-managed variants of these functions are also defined:
>  
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 5fad38fcd701..e96d4a90c0c3 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -2358,7 +2358,7 @@ static struct gpio_desc *gpiod_find(struct device *dev, const char *con_id,
>  				    unsigned int idx,
>  				    enum gpio_lookup_flags *flags)
>  {
> -	struct gpio_desc *desc = ERR_PTR(-ENODEV);
> +	struct gpio_desc *desc = ERR_PTR(-ENOENT);
>  	struct gpiod_lookup_table *table;
>  	struct gpiod_lookup *p;
>  
> @@ -2380,19 +2380,21 @@ static struct gpio_desc *gpiod_find(struct device *dev, const char *con_id,
>  		chip = find_chip_by_name(p->chip_label);
>  
>  		if (!chip) {
> -			dev_warn(dev, "cannot find GPIO chip %s\n",
> -				 p->chip_label);
> -			continue;
> +			dev_err(dev, "cannot find GPIO chip %s\n",
> +				p->chip_label);
> +			return ERR_PTR(-ENODEV);
>  		}
>  
>  		if (chip->ngpio <= p->chip_hwnum) {
> -			dev_warn(dev, "GPIO chip %s has %d GPIOs\n",
> -				 chip->label, chip->ngpio);
> -			continue;
> +			dev_err(dev, "requested GPIO %d but chip %s has %d\n",

The proposed message may confuse user. This lead to question in my head:
"what gpio chip has that referred by %d at the end of line".

Maybe something like "requested GPIO %d is out of range [0..%d] for chip
%s\n" ?

> +				idx, chip->label, chip->ngpio);
> +			return ERR_PTR(-EINVAL);
>  		}
>  
>  		desc = gpiochip_offset_to_desc(chip, p->chip_hwnum);
>  		*flags = p->flags;
> +
> +		return desc;
>  	}
>  
>  	return desc;
> @@ -2404,7 +2406,8 @@ static struct gpio_desc *gpiod_find(struct device *dev, const char *con_id,
>   * @con_id:	function within the GPIO consumer
>   *
>   * Return the GPIO descriptor corresponding to the function con_id of device
> - * dev, or an IS_ERR() condition if an error occured.
> + * dev, -ENOENT if no GPIO has been assigned to the requested function, or
> + * another IS_ERR() code if an error occured while trying to acquire the GPIO.
>   */
>  struct gpio_desc *__must_check gpiod_get(struct device *dev, const char *con_id)
>  {
> @@ -2421,7 +2424,9 @@ EXPORT_SYMBOL_GPL(gpiod_get);
>   * This variant of gpiod_get() allows to access GPIOs other than the first
>   * defined one for functions that define several GPIOs.
>   *
> - * Return a valid GPIO descriptor, or an IS_ERR() condition in case of error.
> + * Return a valid GPIO descriptor, -ENOENT if no GPIO has been assigned to the
> + * requested function and/or index, or another IS_ERR() code if an error
> + * occured while trying to acquire the GPIO.
>   */
>  struct gpio_desc *__must_check gpiod_get_index(struct device *dev,
>  					       const char *con_id,
> @@ -2446,13 +2451,9 @@ struct gpio_desc *__must_check gpiod_get_index(struct device *dev,
>  	 * Either we are not using DT or ACPI, or their lookup did not return
>  	 * a result. In that case, use platform lookup as a fallback.
>  	 */
> -	if (!desc || IS_ERR(desc)) {
> -		struct gpio_desc *pdesc;
> +	if (!desc || desc == ERR_PTR(-ENOENT)) {
>  		dev_dbg(dev, "using lookup tables for GPIO lookup");
> -		pdesc = gpiod_find(dev, con_id, idx, &flags);
> -		/* If used as fallback, do not replace the previous error */
> -		if (!IS_ERR(pdesc) || !desc)
> -			desc = pdesc;
> +		desc = gpiod_find(dev, con_id, idx, &flags);
>  	}
>  
>  	if (IS_ERR(desc)) {

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH] gpiolib: return -ENOENT when no GPIO mapping exists

[Alex Courbot]

On 12/09/2013 07:28 PM, Andy Shevchenko wrote:
> On Fri, 2013-12-06 at 11:06 +0900, Alexandre Courbot wrote:
>> Some devices drivers make use of optional GPIO parameters. For such
>> drivers, it is important to discriminate between the case where no
>> GPIO mapping has been defined for the function they are requesting, and
>> the case where a mapping exists but an error occured while resolving it
>> or when acquiring the GPIO.
>>
>> This patch changes the family of gpiod_get() functions such that they
>> will return -ENOENT if and only if no GPIO mapping is defined for the
>> requested function. Other error codes are used when an actual error
>> occured during the GPIO resolution.
>>
>
> I like the idea.
> One minor comment below (in code).
>
>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>> ---
>> I think this change should be merged early as not having it may prevent
>> some users to switch to gpiod. I stumbled upon this issue while
>> considering porting a simple driver (pwm_bl) that has an optional GPIO
>> parameter.
>>
>> Mika, Andy: if Linus agrees with this change, could you take care of
>> having -ENOENT returned as well for the ACPI and SFI GPIOs lookup?
>
> I have already switched to -ENOENT, so, consider done.

Great, thanks!

>> My understanding of ACPI was not sufficient to allow me to do it myself.
>> SFI OTOH should be trivial as it is a simple table.
>>
>>   Documentation/gpio/consumer.txt |  6 +++++-
>>   drivers/gpio/gpiolib.c          | 31 ++++++++++++++++---------------
>>   2 files changed, 21 insertions(+), 16 deletions(-)
>>
>> diff --git a/Documentation/gpio/consumer.txt b/Documentation/gpio/consumer.txt
>> index 07c74a3765a0..e42f77d8d4ca 100644
>> --- a/Documentation/gpio/consumer.txt
>> +++ b/Documentation/gpio/consumer.txt
>> @@ -38,7 +38,11 @@ device that displays digits), an additional index argument can be specified:
>>   					  const char *con_id, unsigned int idx)
>>
>>   Both functions return either a valid GPIO descriptor, or an error code checkable
>> -with IS_ERR(). They will never return a NULL pointer.
>> +with IS_ERR() (they will never return a NULL pointer). -ENOENT will be returned
>> +if and only if no GPIO has been assigned to the device/function/index triplet,
>> +other error codes are used for cases where a GPIO has been assigned but an error
>> +occured while trying to acquire it. This is useful to discriminate between mere
>> +errors and an absence of GPIO for optional GPIO parameters.
>>
>>   Device-managed variants of these functions are also defined:
>>
>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>> index 5fad38fcd701..e96d4a90c0c3 100644
>> --- a/drivers/gpio/gpiolib.c
>> +++ b/drivers/gpio/gpiolib.c
>> @@ -2358,7 +2358,7 @@ static struct gpio_desc *gpiod_find(struct device *dev, const char *con_id,
>>   				    unsigned int idx,
>>   				    enum gpio_lookup_flags *flags)
>>   {
>> -	struct gpio_desc *desc = ERR_PTR(-ENODEV);
>> +	struct gpio_desc *desc = ERR_PTR(-ENOENT);
>>   	struct gpiod_lookup_table *table;
>>   	struct gpiod_lookup *p;
>>
>> @@ -2380,19 +2380,21 @@ static struct gpio_desc *gpiod_find(struct device *dev, const char *con_id,
>>   		chip = find_chip_by_name(p->chip_label);
>>
>>   		if (!chip) {
>> -			dev_warn(dev, "cannot find GPIO chip %s\n",
>> -				 p->chip_label);
>> -			continue;
>> +			dev_err(dev, "cannot find GPIO chip %s\n",
>> +				p->chip_label);
>> +			return ERR_PTR(-ENODEV);
>>   		}
>>
>>   		if (chip->ngpio <= p->chip_hwnum) {
>> -			dev_warn(dev, "GPIO chip %s has %d GPIOs\n",
>> -				 chip->label, chip->ngpio);
>> -			continue;
>> +			dev_err(dev, "requested GPIO %d but chip %s has %d\n",
>
> The proposed message may confuse user. This lead to question in my head:
> "what gpio chip has that referred by %d at the end of line".
>
> Maybe something like "requested GPIO %d is out of range [0..%d] for chip
> %s\n" ?

I agree it would be better. My concern here is to have the line fit 
within 80 characters. :P

But you're right, I will improve this in v2.

Thanks,
Alex.

Re: [PATCH] gpiolib: return -ENOENT when no GPIO mapping exists

[Mika Westerberg]

On Fri, Dec 06, 2013 at 11:06:56AM +0900, Alexandre Courbot wrote:
> Some devices drivers make use of optional GPIO parameters. For such
> drivers, it is important to discriminate between the case where no
> GPIO mapping has been defined for the function they are requesting, and
> the case where a mapping exists but an error occured while resolving it
> or when acquiring the GPIO.
> 
> This patch changes the family of gpiod_get() functions such that they
> will return -ENOENT if and only if no GPIO mapping is defined for the
> requested function. Other error codes are used when an actual error
> occured during the GPIO resolution.
> 
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
> I think this change should be merged early as not having it may prevent
> some users to switch to gpiod. I stumbled upon this issue while
> considering porting a simple driver (pwm_bl) that has an optional GPIO
> parameter.
> 
> Mika, Andy: if Linus agrees with this change, could you take care of
> having -ENOENT returned as well for the ACPI and SFI GPIOs lookup?

Sure. I have a patch for this already so once this gets merged, I'll send
out the ACPI version.

Re: [PATCH] gpiolib: return -ENOENT when no GPIO mapping exists

[Alex Courbot]

On 12/09/2013 09:40 PM, Mika Westerberg wrote:
> On Fri, Dec 06, 2013 at 11:06:56AM +0900, Alexandre Courbot wrote:
>> Some devices drivers make use of optional GPIO parameters. For such
>> drivers, it is important to discriminate between the case where no
>> GPIO mapping has been defined for the function they are requesting, and
>> the case where a mapping exists but an error occured while resolving it
>> or when acquiring the GPIO.
>>
>> This patch changes the family of gpiod_get() functions such that they
>> will return -ENOENT if and only if no GPIO mapping is defined for the
>> requested function. Other error codes are used when an actual error
>> occured during the GPIO resolution.
>>
>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>> ---
>> I think this change should be merged early as not having it may prevent
>> some users to switch to gpiod. I stumbled upon this issue while
>> considering porting a simple driver (pwm_bl) that has an optional GPIO
>> parameter.
>>
>> Mika, Andy: if Linus agrees with this change, could you take care of
>> having -ENOENT returned as well for the ACPI and SFI GPIOs lookup?
>
> Sure. I have a patch for this already so once this gets merged, I'll send
> out the ACPI version.

Please feel free to send it now, as it should not break anything if your 
patch is merged before mine anyway.

Re: [PATCH] gpiolib: return -ENOENT when no GPIO mapping exists

[Linus Walleij]

On Fri, Dec 6, 2013 at 3:06 AM, Alexandre Courbot <acourbot@nvidia.com> wrote:

> Some devices drivers make use of optional GPIO parameters. For such
> drivers, it is important to discriminate between the case where no
> GPIO mapping has been defined for the function they are requesting, and
> the case where a mapping exists but an error occured while resolving it
> or when acquiring the GPIO.
>
> This patch changes the family of gpiod_get() functions such that they
> will return -ENOENT if and only if no GPIO mapping is defined for the
> requested function. Other error codes are used when an actual error
> occured during the GPIO resolution.
>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
> I think this change should be merged early as not having it may prevent
> some users to switch to gpiod. I stumbled upon this issue while
> considering porting a simple driver (pwm_bl) that has an optional GPIO
> parameter.
>
> Mika, Andy: if Linus agrees with this change, could you take care of
> having -ENOENT returned as well for the ACPI and SFI GPIOs lookup?
> My understanding of ACPI was not sufficient to allow me to do it myself.
> SFI OTOH should be trivial as it is a simple table.

I'm aligned with this change, just waiting for a v2 with the fix
requested by Andy to appear (maybe it's further up in the mail log
I don't know...)

Yours,
Linus Walleij