Skip to contentSkip to conversation

gpio: rdc321x: Fix off-by-one for ngpio setting

PATCH Superseded Axel Lin 3 participants 9 messages Apr 11, 2014 – Apr 23, 2014 GPIO

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Axel Lin

author
#0 lore raw

PATCH 1/2 gpio: rdc321x: Fix off-by-one for ngpio setting

The valid gpio is GPIO0 ~ GPIO58, so ngpio should be RDC321X_MAX_GPIO + 1.

Signed-off-by Axel Lin <[email protected]>
---
 drivers/gpio/gpio-rdc321x.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpio/gpio-rdc321x.c +1 -1
index 88577c3..9e7da2d 100644
@@ -176,7 +176,7 @@ static int rdc321x_gpio_probe(struct platform_device *pdev)
 	rdc321x_gpio_dev->chip.get = rdc_gpio_get_value;
 	rdc321x_gpio_dev->chip.set = rdc_gpio_set_value;
 	rdc321x_gpio_dev->chip.base = 0;
-	rdc321x_gpio_dev->chip.ngpio = pdata->max_gpios;
+	rdc321x_gpio_dev->chip.ngpio = pdata->max_gpios + 1;
 
 	platform_set_drvdata(pdev, rdc321x_gpio_dev);
Signature · 1 line
1.8.3.2
author
#1 lore raw

PATCH 2/2 gpio: rdc321x: Convert to use devm_kzalloc

This saves a few unwind code.

Signed-off-by Axel Lin <[email protected]>
---
 drivers/gpio/gpio-rdc321x.c | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)
drivers/gpio/gpio-rdc321x.c +6 -13
index 9e7da2d..3027591 100644
@@ -141,7 +141,8 @@ static int rdc321x_gpio_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
-	rdc321x_gpio_dev = kzalloc(sizeof(struct rdc321x_gpio), GFP_KERNEL);
+	rdc321x_gpio_dev = devm_kzalloc(&pdev->dev, sizeof(struct rdc321x_gpio),
+					GFP_KERNEL);
 	if (!rdc321x_gpio_dev) {
 		dev_err(&pdev->dev, "failed to allocate private data\n");
 		return -ENOMEM;

@@ -150,8 +151,7 @@ static int rdc321x_gpio_probe(struct platform_device *pdev)
 	r = platform_get_resource_byname(pdev, IORESOURCE_IO, "gpio-reg1");
 	if (!r) {
 		dev_err(&pdev->dev, "failed to get gpio-reg1 resource\n");
-		err = -ENODEV;
-		goto out_free;
+		return -ENODEV;
 	}
 
 	spin_lock_init(&rdc321x_gpio_dev->lock);

@@ -162,8 +162,7 @@ static int rdc321x_gpio_probe(struct platform_device *pdev)
 	r = platform_get_resource_byname(pdev, IORESOURCE_IO, "gpio-reg2");
 	if (!r) {
 		dev_err(&pdev->dev, "failed to get gpio-reg2 resource\n");
-		err = -ENODEV;
-		goto out_free;
+		return -ENODEV;
 	}
 
 	rdc321x_gpio_dev->reg2_ctrl_base = r->start;

@@ -187,21 +186,17 @@ static int rdc321x_gpio_probe(struct platform_device *pdev)
 					rdc321x_gpio_dev->reg1_data_base,
 					&rdc321x_gpio_dev->data_reg[0]);
 	if (err)
-		goto out_free;
+		return err;
 
 	err = pci_read_config_dword(rdc321x_gpio_dev->sb_pdev,
 					rdc321x_gpio_dev->reg2_data_base,
 					&rdc321x_gpio_dev->data_reg[1]);
 	if (err)
-		goto out_free;
+		return err;
 
 	dev_info(&pdev->dev, "registering %d GPIOs\n",
 					rdc321x_gpio_dev->chip.ngpio);
 	return gpiochip_add(&rdc321x_gpio_dev->chip);
-
-out_free:
-	kfree(rdc321x_gpio_dev);
-	return err;
 }
 
 static int rdc321x_gpio_remove(struct platform_device *pdev)

@@ -213,8 +208,6 @@ static int rdc321x_gpio_remove(struct platform_device *pdev)
 	if (ret)
 		dev_err(&pdev->dev, "failed to unregister chip\n");
 
-	kfree(rdc321x_gpio_dev);
-
 	return ret;
 }
Signature · 1 line
1.8.3.2
Acked-by on v1 2/2
#2 lore raw

Re: [PATCH 2/2] gpio: rdc321x: Convert to use devm_kzalloc

Quoted 3 lines · Axel Lin

This saves a few unwind code.

Signed-off-by Axel Lin <[email protected]>
Acked-by Florian Fainelli <[email protected]>
Quoted 64 lines
---
  drivers/gpio/gpio-rdc321x.c | 19 ++++++-------------
  1 file changed, 6 insertions(+), 13 deletions(-)
drivers/gpio/gpio-rdc321x.c +6 -11
index 9e7da2d..3027591 100644
@@ -141,7 +141,8 @@ static int rdc321x_gpio_probe(struct platform_device *pdev)
  		return -ENODEV;
  	}
 
-	rdc321x_gpio_dev = kzalloc(sizeof(struct rdc321x_gpio), GFP_KERNEL);
+	rdc321x_gpio_dev = devm_kzalloc(&pdev->dev, sizeof(struct rdc321x_gpio),
+					GFP_KERNEL);
  	if (!rdc321x_gpio_dev) {
  		dev_err(&pdev->dev, "failed to allocate private data\n");
  		return -ENOMEM;

@@ -150,8 +151,7 @@ static int rdc321x_gpio_probe(struct platform_device *pdev)
  	r = platform_get_resource_byname(pdev, IORESOURCE_IO, "gpio-reg1");
  	if (!r) {
  		dev_err(&pdev->dev, "failed to get gpio-reg1 resource\n");
-		err = -ENODEV;
-		goto out_free;
+		return -ENODEV;
  	}
 
  	spin_lock_init(&rdc321x_gpio_dev->lock);

@@ -162,8 +162,7 @@ static int rdc321x_gpio_probe(struct platform_device *pdev)
  	r = platform_get_resource_byname(pdev, IORESOURCE_IO, "gpio-reg2");
  	if (!r) {
  		dev_err(&pdev->dev, "failed to get gpio-reg2 resource\n");
-		err = -ENODEV;
-		goto out_free;
+		return -ENODEV;
  	}
 
  	rdc321x_gpio_dev->reg2_ctrl_base = r->start;

@@ -187,21 +186,17 @@ static int rdc321x_gpio_probe(struct platform_device *pdev)
  					rdc321x_gpio_dev->reg1_data_base,
  					&rdc321x_gpio_dev->data_reg[0]);
  	if (err)
-		goto out_free;
+		return err;
 
  	err = pci_read_config_dword(rdc321x_gpio_dev->sb_pdev,
  					rdc321x_gpio_dev->reg2_data_base,
  					&rdc321x_gpio_dev->data_reg[1]);
  	if (err)
-		goto out_free;
+		return err;
 
  	dev_info(&pdev->dev, "registering %d GPIOs\n",
  					rdc321x_gpio_dev->chip.ngpio);
  	return gpiochip_add(&rdc321x_gpio_dev->chip);
-
-out_free:
-	kfree(rdc321x_gpio_dev);
-	return err;
  }
 
  static int rdc321x_gpio_remove(struct platform_device *pdev)

drivers/gpio/gpio-rdc321x.c static int rdc321x_gpio_remove(struct platform_device *pdev) · line 213

Quoted 8 lines
drivers/gpio/gpio-rdc321x.c +0 -2
@@ -213,8 +208,6 @@ static int rdc321x_gpio_remove(struct platform_device *pdev)
  	if (ret)
  		dev_err(&pdev->dev, "failed to unregister chip\n");
 
-	kfree(rdc321x_gpio_dev);
-
  	return ret;
  }

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Acked-by on v1 1/2
#4 lore raw
In reply to Axel Lin
Quoted 3 lines · Axel Lin

The valid gpio is GPIO0 ~ GPIO58, so ngpio should be RDC321X_MAX_GPIO + 1.

Signed-off-by Axel Lin <[email protected]>
Acked-by Florian Fainelli <[email protected]>
Quoted 10 lines
---
  drivers/gpio/gpio-rdc321x.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpio/gpio-rdc321x.c +0 -0
index 88577c3..9e7da2d 100644

drivers/gpio/gpio-rdc321x.c static int rdc321x_gpio_probe(struct platform_device *pdev) · line 182

Quoted 8 lines
drivers/gpio/gpio-rdc321x.c +1 -1
@@ -176,7 +176,7 @@ static int rdc321x_gpio_probe(struct platform_device *pdev)
  	rdc321x_gpio_dev->chip.get = rdc_gpio_get_value;
  	rdc321x_gpio_dev->chip.set = rdc_gpio_set_value;
  	rdc321x_gpio_dev->chip.base = 0;
-	rdc321x_gpio_dev->chip.ngpio = pdata->max_gpios;
+	rdc321x_gpio_dev->chip.ngpio = pdata->max_gpios + 1;
 
  	platform_set_drvdata(pdev, rdc321x_gpio_dev);

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#5 lore raw
In reply to Axel Lin
Quoted 3 lines · Axel Lin

The valid gpio is GPIO0 ~ GPIO58, so ngpio should be RDC321X_MAX_GPIO + 1.

Signed-off-by Axel Lin <[email protected]>

(...)

drivers/gpio/gpio-rdc321x.c static int rdc321x_gpio_probe(struct platform_device *pdev) · line 179

Quoted 2 lines
drivers/gpio/gpio-rdc321x.c +1 -1
-       rdc321x_gpio_dev->chip.ngpio = pdata->max_gpios;
+       rdc321x_gpio_dev->chip.ngpio = pdata->max_gpios + 1;

This is counter-intuitive. "max_gpios" should be a name for the maximum number of gpios right? Not the maximum number of GPIOs minus one!

It is better to patch include/linux/mfd/rdc321x.h:

-#define RDC321X_MAX_GPIO 58
+#define RDC321X_MAX_GPIO 59

Which establishes the true character of this device.

Yours,
Linus Walleij

author
#6 lore raw

2014-04-23 21:25 GMT+08:00 Linus Walleij <[email protected]>:

Quoted 15 lines · Axel Lin
Quoted 4 lines

The valid gpio is GPIO0 ~ GPIO58, so ngpio should be RDC321X_MAX_GPIO + 1.

Signed-off-by Axel Lin <[email protected]>

(...)

drivers/gpio/gpio-rdc321x.c static int rdc321x_gpio_probe(struct platform_device *pdev) · line 179

Quoted 2 lines
drivers/gpio/gpio-rdc321x.c +1 -1
-       rdc321x_gpio_dev->chip.ngpio = pdata->max_gpios;
+       rdc321x_gpio_dev->chip.ngpio = pdata->max_gpios + 1;

This is counter-intuitive. "max_gpios" should be a name for the maximum number of gpios right? Not the maximum number of GPIOs minus one!

It is better to patch include/linux/mfd/rdc321x.h:

Review

Quoted 2 lines
(quoted diff) +1 -1
-#define RDC321X_MAX_GPIO    58
+#define RDC321X_MAX_GPIO    59

Hi Linus,

I'm wondering if it is better to define RDC321X_NUM_GPIO (59) rather than RDC321X_MAX_GPIO.
How do you think?

Regards,
Axel

#7 lore raw
Quoted 9 lines · Axel Lin

2014-04-23 21:25 GMT+08:00 Linus Walleij <[email protected]>:

Review

Quoted 2 lines
(quoted diff) +1 -1
-#define RDC321X_MAX_GPIO    58
+#define RDC321X_MAX_GPIO    59

Hi Linus,

I'm wondering if it is better to define RDC321X_NUM_GPIO (59) rather than RDC321X_MAX_GPIO.
How do you think?

I think yes. It's not a maximum really is it? It's the number available, they just mean the max enumerator you can use :-P

Yours,
Linus Walleij

#8 lore raw

2014-04-23 14:53 GMT-07:00 Linus Walleij <[email protected]>:

Quoted 7 lines · Axel Lin
Quoted 6 lines

2014-04-23 21:25 GMT+08:00 Linus Walleij <[email protected]>:

Review

Quoted 2 lines
(quoted diff) +1 -1
-#define RDC321X_MAX_GPIO    58
+#define RDC321X_MAX_GPIO    59

Hi Linus,

Quoted 6 lines
Quoted 3 lines

I'm wondering if it is better to define RDC321X_NUM_GPIO (59) rather than RDC321X_MAX_GPIO.
How do you think?

I think yes. It's not a maximum really is it? It's the number available, they just mean the max enumerator you can use :-P

Yes, NUM_GPIO is probably a better name for this, and it turns out to also be the maximum for that particular GPIO chip, then it's all good.

Signature · 1 line
Florian