[PATCH v2 0/3] gpio: davinci: Add missing return checks

[PATCH v2 0/3] gpio: davinci: Add missing return checks

[Keerthy]

The patch series does a couple of important return handling which were missed
earlier in the driver probe.

This is based on Suman's:

http://marc.info/?l=linux-arm-kernel&m=150034845427555&w=2
http://marc.info/?l=linux-arm-kernel&m=150034856627618&w=2 

Changes in v2:

  * Removed the device tree patch for to keep the series independent.

Keerthy (3):
  gpio: davinci: Use devm_gpiochip_add_data in place of
    gpiochip_add_data
  gpio: davinci: Handle the return value of davinci_gpio_irq_setup
    function
  gpio: davinci: Convert prinkt to dev_err

 drivers/gpio/gpio-davinci.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

-- 
1.9.1

[PATCH v2 1/3] gpio: davinci: Use devm_gpiochip_add_data in place of gpiochip_add_data

[Keerthy]

Use the devm version of gpiochip_add_data and pass on the
return value. This avoids memory leak due to gpiochip_add_data
in case the driver is unbound.

Signed-off-by: Keerthy <j-keerthy@ti.com>
---

Changes in v2:

  * Restoring the previous values of ctrl_num and bank_base in case of an error.

 drivers/gpio/gpio-davinci.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
index 65cb359..27499ec 100644
--- a/drivers/gpio/gpio-davinci.c
+++ b/drivers/gpio/gpio-davinci.c
@@ -166,7 +166,7 @@ static int davinci_gpio_get(struct gpio_chip *chip, unsigned offset)
 static int davinci_gpio_probe(struct platform_device *pdev)
 {
 	static int ctrl_num, bank_base;
-	int gpio, bank;
+	int gpio, bank, ret = 0;
 	unsigned ngpio, nbank;
 	struct davinci_gpio_controller *chips;
 	struct davinci_gpio_platform_data *pdata;
@@ -232,10 +232,20 @@ static int davinci_gpio_probe(struct platform_device *pdev)
 	for (gpio = 0, bank = 0; gpio < ngpio; gpio += 32, bank++)
 		chips->regs[bank] = gpio_base + offset_array[bank];
 
-	gpiochip_add_data(&chips->chip, chips);
+	ret = devm_gpiochip_add_data(dev, &chips->chip, chips);
+	if (ret)
+		goto err;
+
 	platform_set_drvdata(pdev, chips);
 	davinci_gpio_irq_setup(pdev);
 	return 0;
+
+err:
+	/* Revert the static variable increments */
+	ctrl_num--;
+	bank_base -= ngpio;
+
+	return ret;
 }
 
 /*--------------------------------------------------------------------------*/
-- 
1.9.1

Re: [PATCH v2 1/3] gpio: davinci: Use devm_gpiochip_add_data in place of gpiochip_add_data

[Linus Walleij]

On Thu, Jul 20, 2017 at 11:42 AM, Keerthy <j-keerthy@ti.com> wrote:

> Use the devm version of gpiochip_add_data and pass on the
> return value. This avoids memory leak due to gpiochip_add_data
> in case the driver is unbound.
>
> Signed-off-by: Keerthy <j-keerthy@ti.com>

Patch applied.

Yours,
Linus Walleij

[PATCH v2 2/3] gpio: davinci: Handle the return value of davinci_gpio_irq_setup function

[Keerthy]

Currently davinci_gpio_irq_setup return value is ignored. Handle the
return value appropriately.

Signed-off-by: Keerthy <j-keerthy@ti.com>
---
 drivers/gpio/gpio-davinci.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
index 27499ec..d6fb1ce 100644
--- a/drivers/gpio/gpio-davinci.c
+++ b/drivers/gpio/gpio-davinci.c
@@ -237,7 +237,10 @@ static int davinci_gpio_probe(struct platform_device *pdev)
 		goto err;
 
 	platform_set_drvdata(pdev, chips);
-	davinci_gpio_irq_setup(pdev);
+	ret = davinci_gpio_irq_setup(pdev);
+	if (ret)
+		goto err;
+
 	return 0;
 
 err:
-- 
1.9.1

Re: [PATCH v2 2/3] gpio: davinci: Handle the return value of davinci_gpio_irq_setup function

[Keerthy]

On Thursday 20 July 2017 03:12 PM, Keerthy wrote:
> Currently davinci_gpio_irq_setup return value is ignored. Handle the
> return value appropriately.

Linus,

Any comments on this? Sorry if i pinged too soon. The rest 2 of this
series were pulled by you. So just wanted to check on this one if you
had specific comments.

Regards,
Keerthy

> 
> Signed-off-by: Keerthy <j-keerthy@ti.com>
> ---
>  drivers/gpio/gpio-davinci.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
> index 27499ec..d6fb1ce 100644
> --- a/drivers/gpio/gpio-davinci.c
> +++ b/drivers/gpio/gpio-davinci.c
> @@ -237,7 +237,10 @@ static int davinci_gpio_probe(struct platform_device *pdev)
>  		goto err;
>  
>  	platform_set_drvdata(pdev, chips);
> -	davinci_gpio_irq_setup(pdev);
> +	ret = davinci_gpio_irq_setup(pdev);
> +	if (ret)
> +		goto err;
> +
>  	return 0;
>  
>  err:
> 

Re: [PATCH v2 2/3] gpio: davinci: Handle the return value of davinci_gpio_irq_setup function

[Linus Walleij]

On Wed, Aug 2, 2017 at 12:59 PM, Keerthy <j-keerthy@ti.com> wrote:
> On Thursday 20 July 2017 03:12 PM, Keerthy wrote:
>> Currently davinci_gpio_irq_setup return value is ignored. Handle the
>> return value appropriately.
>
> Linus,
>
> Any comments on this? Sorry if i pinged too soon. The rest 2 of this
> series were pulled by you. So just wanted to check on this one if you
> had specific comments.

I got the impression you would send this in a series with a new
patch requested by Johan.

Do we have consensus that this patch should be applied as-is?

Yours,
Linus Walleij

Re: [PATCH v2 2/3] gpio: davinci: Handle the return value of davinci_gpio_irq_setup function

[Keerthy]

On Monday 07 August 2017 05:42 PM, Linus Walleij wrote:
> On Wed, Aug 2, 2017 at 12:59 PM, Keerthy <j-keerthy@ti.com> wrote:
>> On Thursday 20 July 2017 03:12 PM, Keerthy wrote:
>>> Currently davinci_gpio_irq_setup return value is ignored. Handle the
>>> return value appropriately.
>>
>> Linus,
>>
>> Any comments on this? Sorry if i pinged too soon. The rest 2 of this
>> series were pulled by you. So just wanted to check on this one if you
>> had specific comments.
> 
> I got the impression you would send this in a series with a new
> patch requested by Johan.
> 
> Do we have consensus that this patch should be applied as-is?

IIUC a separate patch is needed to implement remove function. This can
still be independent of that.

https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1450395.html

> 
> Yours,
> Linus Walleij
> 

Re: [PATCH v2 2/3] gpio: davinci: Handle the return value of davinci_gpio_irq_setup function

[Linus Walleij]

On Thu, Jul 20, 2017 at 11:42 AM, Keerthy <j-keerthy@ti.com> wrote:

> Currently davinci_gpio_irq_setup return value is ignored. Handle the
> return value appropriately.
>
> Signed-off-by: Keerthy <j-keerthy@ti.com>

Patch applied.

Yours,
Linus Walleij

[PATCH v2 3/3] gpio: davinci: Convert prinkt to dev_err

[Keerthy]

In case of devm_clk_get failure use dev_err instead of printk

Signed-off-by: Keerthy <j-keerthy@ti.com>
---
 drivers/gpio/gpio-davinci.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
index d6fb1ce..f75d844 100644
--- a/drivers/gpio/gpio-davinci.c
+++ b/drivers/gpio/gpio-davinci.c
@@ -490,8 +490,7 @@ static int davinci_gpio_irq_setup(struct platform_device *pdev)
 
 	clk = devm_clk_get(dev, "gpio");
 	if (IS_ERR(clk)) {
-		printk(KERN_ERR "Error %ld getting gpio clock?\n",
-		       PTR_ERR(clk));
+		dev_err(dev, "Error %ld getting gpio clock\n", PTR_ERR(clk));
 		return PTR_ERR(clk);
 	}
 	ret = clk_prepare_enable(clk);
-- 
1.9.1

Re: [PATCH v2 3/3] gpio: davinci: Convert prinkt to dev_err

[Linus Walleij]

On Thu, Jul 20, 2017 at 11:42 AM, Keerthy <j-keerthy@ti.com> wrote:

> In case of devm_clk_get failure use dev_err instead of printk
>
> Signed-off-by: Keerthy <j-keerthy@ti.com>

Patch applied.

Yours,
Linus Walleij

Re: [PATCH v2 0/3] gpio: davinci: Add missing return checks

[Keerthy]

On Thursday 20 July 2017 03:12 PM, Keerthy wrote:
> The patch series does a couple of important return handling which were missed
> earlier in the driver probe.
> 
> This is based on Suman's:
> 
> http://marc.info/?l=linux-arm-kernel&m=150034845427555&w=2
> http://marc.info/?l=linux-arm-kernel&m=150034856627618&w=2 
> 

My bad! The series has no dependency on the above. I have split the
device tree patches to keep it independent.

> Changes in v2:
> 
>   * Removed the device tree patch for to keep the series independent.
> 
> Keerthy (3):
>   gpio: davinci: Use devm_gpiochip_add_data in place of
>     gpiochip_add_data
>   gpio: davinci: Handle the return value of davinci_gpio_irq_setup
>     function
>   gpio: davinci: Convert prinkt to dev_err
> 
>  drivers/gpio/gpio-davinci.c | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
> 

Re: [PATCH v2 0/3] gpio: davinci: Add missing return checks

[Suman Anna]

Hi Keerthy,

> 
> On Thursday 20 July 2017 03:12 PM, Keerthy wrote:
>> The patch series does a couple of important return handling which were missed
>> earlier in the driver probe.
>>
>> This is based on Suman's:
>>
>> http://marc.info/?l=linux-arm-kernel&m=150034845427555&w=2
>> http://marc.info/?l=linux-arm-kernel&m=150034856627618&w=2 
>>
> 
> My bad! The series has no dependency on the above. I have split the
> device tree patches to keep it independent.

The new patches looks clean. The davinci_gpio_irq_setup() function
though could also do with couple more cleanups. Not all failure paths
are doing the necessary cleanup (there are some non devres functions
also called, but not cleaned up).

regards
Suman

> 
>> Changes in v2:
>>
>>   * Removed the device tree patch for to keep the series independent.
>>
>> Keerthy (3):
>>   gpio: davinci: Use devm_gpiochip_add_data in place of
>>     gpiochip_add_data
>>   gpio: davinci: Handle the return value of davinci_gpio_irq_setup
>>     function
>>   gpio: davinci: Convert prinkt to dev_err
>>
>>  drivers/gpio/gpio-davinci.c | 22 +++++++++++++++++-----
>>  1 file changed, 17 insertions(+), 5 deletions(-)
>>

Re: [PATCH v2 0/3] gpio: davinci: Add missing return checks

[Keerthy]

On Friday 21 July 2017 01:27 AM, Suman Anna wrote:
> Hi Keerthy,
> 
>>
>> On Thursday 20 July 2017 03:12 PM, Keerthy wrote:
>>> The patch series does a couple of important return handling which were missed
>>> earlier in the driver probe.
>>>
>>> This is based on Suman's:
>>>
>>> http://marc.info/?l=linux-arm-kernel&m=150034845427555&w=2
>>> http://marc.info/?l=linux-arm-kernel&m=150034856627618&w=2 
>>>
>>
>> My bad! The series has no dependency on the above. I have split the
>> device tree patches to keep it independent.
> 
> The new patches looks clean. The davinci_gpio_irq_setup() function
> though could also do with couple more cleanups. Not all failure paths
> are doing the necessary cleanup (there are some non devres functions
> also called, but not cleaned up).

Linus,

Any comments on this series?

Regards,
Keerthy

> 
> regards
> Suman
> 
>>
>>> Changes in v2:
>>>
>>>   * Removed the device tree patch for to keep the series independent.
>>>
>>> Keerthy (3):
>>>   gpio: davinci: Use devm_gpiochip_add_data in place of
>>>     gpiochip_add_data
>>>   gpio: davinci: Handle the return value of davinci_gpio_irq_setup
>>>     function
>>>   gpio: davinci: Convert prinkt to dev_err
>>>
>>>  drivers/gpio/gpio-davinci.c | 22 +++++++++++++++++-----
>>>  1 file changed, 17 insertions(+), 5 deletions(-)
>>>
>