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

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

[Axel Lin]

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

Signed-off-by: Axel Lin <axel.lin@ingics.com>
---
 drivers/gpio/gpio-rdc321x.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-rdc321x.c b/drivers/gpio/gpio-rdc321x.c
index 88577c3..9e7da2d 100644
--- a/drivers/gpio/gpio-rdc321x.c
+++ b/drivers/gpio/gpio-rdc321x.c
@@ -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);
 
-- 
1.8.3.2

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

[Axel Lin]

This saves a few unwind code.

Signed-off-by: Axel Lin <axel.lin@ingics.com>
---
 drivers/gpio/gpio-rdc321x.c | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/drivers/gpio/gpio-rdc321x.c b/drivers/gpio/gpio-rdc321x.c
index 9e7da2d..3027591 100644
--- a/drivers/gpio/gpio-rdc321x.c
+++ b/drivers/gpio/gpio-rdc321x.c
@@ -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;
 }
 
-- 
1.8.3.2

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

[Florian Fainelli]

Le 10/04/2014 23:14, Axel Lin a écrit :
> The valid gpio is GPIO0 ~ GPIO58, so ngpio should be RDC321X_MAX_GPIO + 1.
>
> Signed-off-by: Axel Lin <axel.lin@ingics.com>

Acked-by: Florian Fainelli <florian@openwrt.org>

> ---
>   drivers/gpio/gpio-rdc321x.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/gpio-rdc321x.c b/drivers/gpio/gpio-rdc321x.c
> index 88577c3..9e7da2d 100644
> --- a/drivers/gpio/gpio-rdc321x.c
> +++ b/drivers/gpio/gpio-rdc321x.c
> @@ -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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Florian Fainelli]

Le 10/04/2014 23:16, Axel Lin a écrit :
> This saves a few unwind code.
>
> Signed-off-by: Axel Lin <axel.lin@ingics.com>

Acked-by: Florian Fainelli <florian@openwrt.org>

> ---
>   drivers/gpio/gpio-rdc321x.c | 19 ++++++-------------
>   1 file changed, 6 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpio/gpio-rdc321x.c b/drivers/gpio/gpio-rdc321x.c
> index 9e7da2d..3027591 100644
> --- a/drivers/gpio/gpio-rdc321x.c
> +++ b/drivers/gpio/gpio-rdc321x.c
> @@ -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;
>   }
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Linus Walleij]

On Fri, Apr 11, 2014 at 8:14 AM, Axel Lin <axel.lin@ingics.com> wrote:

> The valid gpio is GPIO0 ~ GPIO58, so ngpio should be RDC321X_MAX_GPIO + 1.
>
> Signed-off-by: Axel Lin <axel.lin@ingics.com>
(...)
> -       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

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

[Linus Walleij]

On Fri, Apr 11, 2014 at 8:16 AM, Axel Lin <axel.lin@ingics.com> wrote:

> This saves a few unwind code.
>
> Signed-off-by: Axel Lin <axel.lin@ingics.com>

Patch applied with Florian's ACK.

Yours,
Linus Walleij

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

[Axel Lin]

2014-04-23 21:25 GMT+08:00 Linus Walleij <linus.walleij@linaro.org>:
> On Fri, Apr 11, 2014 at 8:14 AM, Axel Lin <axel.lin@ingics.com> wrote:
>
>> The valid gpio is GPIO0 ~ GPIO58, so ngpio should be RDC321X_MAX_GPIO + 1.
>>
>> Signed-off-by: Axel Lin <axel.lin@ingics.com>
> (...)
>> -       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
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

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

[Linus Walleij]

On Wed, Apr 23, 2014 at 4:00 PM, Axel Lin <axel.lin@ingics.com> wrote:
> 2014-04-23 21:25 GMT+08:00 Linus Walleij <linus.walleij@linaro.org>:

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

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

[Florian Fainelli]

2014-04-23 14:53 GMT-07:00 Linus Walleij <linus.walleij@linaro.org>:
> On Wed, Apr 23, 2014 at 4:00 PM, Axel Lin <axel.lin@ingics.com> wrote:
>> 2014-04-23 21:25 GMT+08:00 Linus Walleij <linus.walleij@linaro.org>:
>
>>> -#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

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.
-- 
Florian