Skip to contentSkip to conversation

gpio: pisosr: Use devm_gpiod_get_optional for gpio->load_gpio

PATCH In review Axel Lin 3 participants 5 messages Feb 22, 2016 – Feb 25, 2016 GPIO

lore
Share & tools
lore

Axel Lin

author
#0 lore raw

PATCH 1/1 gpio: pisosr: Use devm_gpiod_get_optional for gpio->load_gpio

gpio->load_gpio is optional, so use devm_gpiod_get_optional instead.

Signed-off-by Axel Lin <[email protected]>
---
 drivers/gpio/gpio-pisosr.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)
drivers/gpio/gpio-pisosr.c +4 -7
index f9f1074..8b8bf8f 100644
@@ -125,15 +125,12 @@ static int pisosr_gpio_probe(struct spi_device *spi)
 	if (!gpio->buffer)
 		return -ENOMEM;
 
-	gpio->load_gpio = devm_gpiod_get(dev, "load", GPIOD_OUT_LOW);
+	gpio->load_gpio = devm_gpiod_get_optional(dev, "load", GPIOD_OUT_LOW);
 	if (IS_ERR(gpio->load_gpio)) {
 		ret = PTR_ERR(gpio->load_gpio);
-		if (ret != -ENOENT && ret != -ENOSYS) {
-			if (ret != -EPROBE_DEFER)
-				dev_err(dev, "Unable to allocate load GPIO\n");
-			return ret;
-		}
-		gpio->load_gpio = NULL;
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "Unable to allocate load GPIO\n");
+		return ret;
 	}
 
 	mutex_init(&gpio->lock);
Signature · 1 line
2.1.4
#1 lore raw
Quoted 22 lines · Axel Lin

gpio->load_gpio is optional, so use devm_gpiod_get_optional instead.

Signed-off-by Axel Lin <[email protected]>
---
  drivers/gpio/gpio-pisosr.c | 11 ++++-------
  1 file changed, 4 insertions(+), 7 deletions(-)
drivers/gpio/gpio-pisosr.c +1 -3
index f9f1074..8b8bf8f 100644
@@ -125,15 +125,12 @@ static int pisosr_gpio_probe(struct spi_device *spi)
  	if (!gpio->buffer)
  		return -ENOMEM;
 
-	gpio->load_gpio = devm_gpiod_get(dev, "load", GPIOD_OUT_LOW);
+	gpio->load_gpio = devm_gpiod_get_optional(dev, "load", GPIOD_OUT_LOW);
  	if (IS_ERR(gpio->load_gpio)) {
  		ret = PTR_ERR(gpio->load_gpio);
-		if (ret != -ENOENT && ret != -ENOSYS) {
-			if (ret != -EPROBE_DEFER)

drivers/gpio/gpio-pisosr.c static int pisosr_gpio_probe(struct spi_device *spi) · line 136

Quoted 10 lines
drivers/gpio/gpio-pisosr.c +3 -4
-				dev_err(dev, "Unable to allocate load GPIO\n");
-			return ret;
-		}
-		gpio->load_gpio = NULL;
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "Unable to allocate load GPIO\n");
+		return ret;
  	}
 
  	mutex_init(&gpio->lock);

How does this work when the GPIO subsystem is disabled? It looks like we get will get -ENOSYS and fail probe. The issue is then that devm_gpiod_get_optional returns an error when it cant get the optional GPIO in that case, when it should probably just return NULL, I would think all the optional functions should.

author
#2 lore raw

2016-02-22 23:43 GMT+08:00 Andrew F. Davis <[email protected]>:

Quoted 34 lines · Axel Lin
Quoted 33 lines

gpio->load_gpio is optional, so use devm_gpiod_get_optional instead.

Signed-off-by Axel Lin <[email protected]>
---
  drivers/gpio/gpio-pisosr.c | 11 ++++-------
  1 file changed, 4 insertions(+), 7 deletions(-)
drivers/gpio/gpio-pisosr.c +4 -7
index f9f1074..8b8bf8f 100644
@@ -125,15 +125,12 @@ static int pisosr_gpio_probe(struct spi_device *spi)
        if (!gpio->buffer)
                return -ENOMEM;
 
-       gpio->load_gpio = devm_gpiod_get(dev, "load", GPIOD_OUT_LOW);
+       gpio->load_gpio = devm_gpiod_get_optional(dev, "load",
 GPIOD_OUT_LOW);
        if (IS_ERR(gpio->load_gpio)) {
                ret = PTR_ERR(gpio->load_gpio);
-               if (ret != -ENOENT && ret != -ENOSYS) {
-                       if (ret != -EPROBE_DEFER)
-                               dev_err(dev, "Unable to allocate load
 GPIO\n");
-                       return ret;
-               }
-               gpio->load_gpio = NULL;
+               if (ret != -EPROBE_DEFER)
+                       dev_err(dev, "Unable to allocate load GPIO\n");
+               return ret;
        }
Quoted 5 lines
Quoted 2 lines
        mutex_init(&gpio->lock);

How does this work when the GPIO subsystem is disabled? It looks like we get will get -ENOSYS and fail probe. The issue is then that

I think it's no problem for this driver as it is in drivers/gpio/ folder, it's always build with GPIOLIB.

Quoted 3 lines

devm_gpiod_get_optional returns an error when it cant get the optional GPIO in that case, when it should probably just return NULL, I would think all the optional functions should.

I had similar question when I mad this change.
When !GPIOLIB, all the gpio APIs return error.
But for (devm_)gpiod_get_optional, it seems reasonable to return NULL in !GPIOLIB case
because it's optional anyway.

Acked-by
#3 lore raw
Quoted 48 lines · Axel Lin

2016-02-22 23:43 GMT+08:00 Andrew F. Davis <[email protected]>:

Quoted 34 lines · Axel Lin
Quoted 33 lines

gpio->load_gpio is optional, so use devm_gpiod_get_optional instead.

Signed-off-by Axel Lin <[email protected]>
---
   drivers/gpio/gpio-pisosr.c | 11 ++++-------
   1 file changed, 4 insertions(+), 7 deletions(-)
drivers/gpio/gpio-pisosr.c +4 -7
index f9f1074..8b8bf8f 100644
@@ -125,15 +125,12 @@ static int pisosr_gpio_probe(struct spi_device *spi)
         if (!gpio->buffer)
                 return -ENOMEM;
 
-       gpio->load_gpio = devm_gpiod_get(dev, "load", GPIOD_OUT_LOW);
+       gpio->load_gpio = devm_gpiod_get_optional(dev, "load",
 GPIOD_OUT_LOW);
         if (IS_ERR(gpio->load_gpio)) {
                 ret = PTR_ERR(gpio->load_gpio);
-               if (ret != -ENOENT && ret != -ENOSYS) {
-                       if (ret != -EPROBE_DEFER)
-                               dev_err(dev, "Unable to allocate load
 GPIO\n");
-                       return ret;
-               }
-               gpio->load_gpio = NULL;
+               if (ret != -EPROBE_DEFER)
+                       dev_err(dev, "Unable to allocate load GPIO\n");
+               return ret;
         }
Quoted 5 lines
Quoted 2 lines
         mutex_init(&gpio->lock);

How does this work when the GPIO subsystem is disabled? It looks like we get will get -ENOSYS and fail probe. The issue is then that

I think it's no problem for this driver as it is in drivers/gpio/ folder, it's always build with GPIOLIB.

Quoted 3 lines

devm_gpiod_get_optional returns an error when it cant get the optional GPIO in that case, when it should probably just return NULL, I would think all the optional functions should.

Quoted 6 lines

I had similar question when I mad this change.
When !GPIOLIB, all the gpio APIs return error.
But for (devm_)gpiod_get_optional, it seems reasonable to return NULL in !GPIOLIB case
because it's optional anyway.

Well I guess that's work for another patch, for this one I see no issues.

Acked-by Andrew F. Davis <[email protected]>