[PATCH 0/4] pinctrl: at91: Fixes in probe function

[PATCH 0/4] pinctrl: at91: Fixes in probe function

[Pramod Gurav]

This patchset modifies the probe function to do some changes such as 
switching to manage version of clk_get, fixing the error path by releasing
resources etc. The last patch adds a remove function for clean unload.

Pramod Gurav (4):
  pinctrl: at91: Switch to using managed clk_get
  pinctrl: at91: Fix failure path in at91_gpio_probe path
  pinctrl: at91: Fix error handling while doing gpiochio_irqchip_add
  pinctrl: at91: Add remove function in platform_driver
    at91_gpio_driver

 drivers/pinctrl/pinctrl-at91.c |   51 +++++++++++++++++++++++++++++-----------
 1 files changed, 37 insertions(+), 14 deletions(-)

[PATCH 1/4] pinctrl: at91: Switch to using managed clk_get

[Pramod Gurav]

This patch switches to using managed version of clk_get and hence
removes clk_put from failure path.

CC: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
CC: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Pramod Gurav <pramod.gurav@smartplayin.com>
---
 drivers/pinctrl/pinctrl-at91.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
index af1ba4f..480460d 100644
--- a/drivers/pinctrl/pinctrl-at91.c
+++ b/drivers/pinctrl/pinctrl-at91.c
@@ -1575,14 +1575,14 @@ static int at91_gpio_probe(struct platform_device *pdev)
 	at91_chip->pioc_virq = irq;
 	at91_chip->pioc_idx = alias_idx;
 
-	at91_chip->clock = clk_get(&pdev->dev, NULL);
+	at91_chip->clock = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(at91_chip->clock)) {
 		dev_err(&pdev->dev, "failed to get clock, ignoring.\n");
 		goto err;
 	}
 
 	if (clk_prepare(at91_chip->clock))
-		goto clk_prep_err;
+		goto err;
 
 	/* enable PIO controller's clock */
 	if (clk_enable(at91_chip->clock)) {
@@ -1645,8 +1645,6 @@ static int at91_gpio_probe(struct platform_device *pdev)
 
 clk_err:
 	clk_unprepare(at91_chip->clock);
-clk_prep_err:
-	clk_put(at91_chip->clock);
 err:
 	dev_err(&pdev->dev, "Failure %i for GPIO %i\n", ret, alias_idx);
 
-- 
1.7.0.4

Re: [PATCH 1/4] pinctrl: at91: Switch to using managed clk_get

[Linus Walleij]

On Sun, Aug 31, 2014 at 1:21 PM, Pramod Gurav
<pramod.gurav@smartplayin.com> wrote:

> This patch switches to using managed version of clk_get and hence
> removes clk_put from failure path.
>
> CC: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
> CC: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Pramod Gurav <pramod.gurav@smartplayin.com>

Not much that can go wrong.

Patch applied.

Yours,
Linus Walleij

[PATCH 2/4] pinctrl: at91: Fix failure path in at91_gpio_probe path

[Pramod Gurav]

This fixes the whole error handling in probe function by capturing and
returning error values on kernel function like clk_prepare,
clk_enable, gpiochip_add etc.

CC: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
CC: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Pramod Gurav <pramod.gurav@smartplayin.com>
---
 drivers/pinctrl/pinctrl-at91.c |   20 +++++++++++++-------
 1 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
index 480460d..7abe683 100644
--- a/drivers/pinctrl/pinctrl-at91.c
+++ b/drivers/pinctrl/pinctrl-at91.c
@@ -1578,16 +1578,19 @@ static int at91_gpio_probe(struct platform_device *pdev)
 	at91_chip->clock = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(at91_chip->clock)) {
 		dev_err(&pdev->dev, "failed to get clock, ignoring.\n");
+		ret = IS_ERR(at91_chip->clock);
 		goto err;
 	}
 
-	if (clk_prepare(at91_chip->clock))
-		goto err;
+	ret = clk_prepare(at91_chip->clock);
+	if (ret)
+		goto clk_prepare_err;
 
 	/* enable PIO controller's clock */
-	if (clk_enable(at91_chip->clock)) {
+	ret = clk_enable(at91_chip->clock);
+	if (ret) {
 		dev_err(&pdev->dev, "failed to enable clock, ignoring.\n");
-		goto clk_err;
+		goto clk_enable_err;
 	}
 
 	at91_chip->chip = at91_gpio_template;
@@ -1612,7 +1615,7 @@ static int at91_gpio_probe(struct platform_device *pdev)
 
 	if (!names) {
 		ret = -ENOMEM;
-		goto clk_err;
+		goto clk_enable_err;
 	}
 
 	for (i = 0; i < chip->ngpio; i++)
@@ -1630,7 +1633,7 @@ static int at91_gpio_probe(struct platform_device *pdev)
 
 	ret = gpiochip_add(chip);
 	if (ret)
-		goto clk_err;
+		goto gpiochip_add_err;
 
 	gpio_chips[alias_idx] = at91_chip;
 	gpio_banks = max(gpio_banks, alias_idx + 1);
@@ -1643,8 +1646,11 @@ static int at91_gpio_probe(struct platform_device *pdev)
 
 	return 0;
 
-clk_err:
+gpiochip_add_err:
+	clk_disable(at91_chip->clock);
+clk_enable_err:
 	clk_unprepare(at91_chip->clock);
+clk_prepare_err:
 err:
 	dev_err(&pdev->dev, "Failure %i for GPIO %i\n", ret, alias_idx);
 
-- 
1.7.0.4

Re: [PATCH 2/4] pinctrl: at91: Fix failure path in at91_gpio_probe path

[Linus Walleij]

On Sun, Aug 31, 2014 at 1:21 PM, Pramod Gurav
<pramod.gurav@smartplayin.com> wrote:

> This fixes the whole error handling in probe function by capturing and
> returning error values on kernel function like clk_prepare,
> clk_enable, gpiochip_add etc.
>
> CC: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
> CC: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Pramod Gurav <pramod.gurav@smartplayin.com>
> ---
>  drivers/pinctrl/pinctrl-at91.c |   20 +++++++++++++-------
>  1 files changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
> index 480460d..7abe683 100644
> --- a/drivers/pinctrl/pinctrl-at91.c
> +++ b/drivers/pinctrl/pinctrl-at91.c
> @@ -1578,16 +1578,19 @@ static int at91_gpio_probe(struct platform_device *pdev)
>         at91_chip->clock = devm_clk_get(&pdev->dev, NULL);
>         if (IS_ERR(at91_chip->clock)) {
>                 dev_err(&pdev->dev, "failed to get clock, ignoring.\n");
> +               ret = IS_ERR(at91_chip->clock);

No. IS_ERR() returns a bool.

You want to use PTR_ERR().

Yours,
Linus Walleij

[PATCH 3/4] pinctrl: at91: Fix error handling while doing gpiochio_irqchip_add

[Pramod Gurav]

This patch removes a call to panic function when gpiochio_irqchip_add
fails and just returns the error to the calling function.
Same return value is used to handle the error case and adds to lable
to release resources on error.

This also changes first argument to function at91_gpio_of_irq_setup from
struct device_node to struct platform_device. Because The device_node
argument was anyway not being used. Passed pdev so that on failure dev_err
can use &pdev->dev and log can be associated with proper device.

CC: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
CC: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Pramod Gurav <pramod.gurav@smartplayin.com>
---
 drivers/pinctrl/pinctrl-at91.c |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
index 7abe683..8b8772c 100644
--- a/drivers/pinctrl/pinctrl-at91.c
+++ b/drivers/pinctrl/pinctrl-at91.c
@@ -1445,7 +1445,7 @@ static void gpio_irq_handler(unsigned irq, struct irq_desc *desc)
 	/* now it may re-trigger */
 }
 
-static int at91_gpio_of_irq_setup(struct device_node *node,
+static int at91_gpio_of_irq_setup(struct platform_device *pdev,
 				  struct at91_gpio_chip *at91_gpio)
 {
 	struct at91_gpio_chip   *prev = NULL;
@@ -1470,9 +1470,11 @@ static int at91_gpio_of_irq_setup(struct device_node *node,
 				   0,
 				   handle_edge_irq,
 				   IRQ_TYPE_EDGE_BOTH);
-	if (ret)
-		panic("at91_gpio.%d: couldn't allocate irq domain (DT).\n",
+	if (ret) {
+		dev_err(&pdev->dev, "at91_gpio.%d: couldn't allocate irq domain (DT).\n",
 			at91_gpio->pioc_idx);
+		return ret;
+	}
 
 	/* Setup chained handler */
 	if (at91_gpio->pioc_idx)
@@ -1640,12 +1642,16 @@ static int at91_gpio_probe(struct platform_device *pdev)
 
 	at91_gpio_probe_fixup();
 
-	at91_gpio_of_irq_setup(np, at91_chip);
+	ret = at91_gpio_of_irq_setup(pdev, at91_chip);
+	if (ret)
+		goto irq_setup_err;
 
 	dev_info(&pdev->dev, "at address %p\n", at91_chip->regbase);
 
 	return 0;
 
+irq_setup_err:
+	gpiochip_remove(chip);
 gpiochip_add_err:
 	clk_disable(at91_chip->clock);
 clk_enable_err:
-- 
1.7.0.4

Re: [PATCH 3/4] pinctrl: at91: Fix error handling while doing gpiochio_irqchip_add

[Linus Walleij]

On Sun, Aug 31, 2014 at 1:21 PM, Pramod Gurav
<pramod.gurav@smartplayin.com> wrote:

> This patch removes a call to panic function when gpiochio_irqchip_add
> fails and just returns the error to the calling function.
> Same return value is used to handle the error case and adds to lable
> to release resources on error.
>
> This also changes first argument to function at91_gpio_of_irq_setup from
> struct device_node to struct platform_device. Because The device_node
> argument was anyway not being used. Passed pdev so that on failure dev_err
> can use &pdev->dev and log can be associated with proper device.
>
> CC: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
> CC: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Pramod Gurav <pramod.gurav@smartplayin.com>
(...)

> @@ -1470,9 +1470,11 @@ static int at91_gpio_of_irq_setup(struct device_node *node,
>                                    0,
>                                    handle_edge_irq,
>                                    IRQ_TYPE_EDGE_BOTH);
> -       if (ret)
> -               panic("at91_gpio.%d: couldn't allocate irq domain (DT).\n",
> +       if (ret) {
> +               dev_err(&pdev->dev, "at91_gpio.%d: couldn't allocate irq domain (DT).\n",
>                         at91_gpio->pioc_idx);
> +               return ret;
> +       }

If you're fixing this, also fix the error message. It's unable to allocate
a GPIOchip irqchip, not an irq domain. (Caused by other patch, but
please fix this if you're playing around here.)

Yours,
Linus Walleij

[PATCH 4/4] pinctrl: at91: Add remove function in platform_driver at91_gpio_driver

[Pramod Gurav]

This adds remove function in at91_gpio_driver platform_driver so that
driver can be unloaded cleanly.

CC: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
CC: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Pramod Gurav <pramod.gurav@smartplayin.com>
---
 drivers/pinctrl/pinctrl-at91.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
index 8b8772c..92641b6 100644
--- a/drivers/pinctrl/pinctrl-at91.c
+++ b/drivers/pinctrl/pinctrl-at91.c
@@ -1646,6 +1646,7 @@ static int at91_gpio_probe(struct platform_device *pdev)
 	if (ret)
 		goto irq_setup_err;
 
+	platform_set_drvdata(pdev, at91_chip);
 	dev_info(&pdev->dev, "at address %p\n", at91_chip->regbase);
 
 	return 0;
@@ -1663,6 +1664,17 @@ err:
 	return ret;
 }
 
+static int at91_gpio_remove(struct platform_device *pdev)
+{
+	struct at91_gpio_chip *at91_chip = platform_get_drvdata(pdev);
+
+	gpiochip_remove(&at91_chip->chip);
+	clk_disable(at91_chip->clock);
+	clk_unprepare(at91_chip->clock);
+
+	return 0;
+}
+
 static struct platform_driver at91_gpio_driver = {
 	.driver = {
 		.name = "gpio-at91",
@@ -1670,6 +1682,7 @@ static struct platform_driver at91_gpio_driver = {
 		.of_match_table = at91_gpio_of_match,
 	},
 	.probe = at91_gpio_probe,
+	.remove = at91_gpio_remove,
 };
 
 static struct platform_driver at91_pinctrl_driver = {
-- 
1.7.0.4

Re: [PATCH 4/4] pinctrl: at91: Add remove function in platform_driver at91_gpio_driver

[Linus Walleij]

On Sun, Aug 31, 2014 at 1:21 PM, Pramod Gurav
<pramod.gurav@smartplayin.com> wrote:

> This adds remove function in at91_gpio_driver platform_driver so that
> driver can be unloaded cleanly.
>
> CC: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
> CC: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Pramod Gurav <pramod.gurav@smartplayin.com>

Again, this driver is a bool so will never exercise the .remove()
callback.

Yours,
Linus Walleij

Re: [PATCH 0/4] pinctrl: at91: Fixes in probe function

[Nicolas FERRE]

Pramod Gurav <pramod.gurav <at> smartplayin.com> writes:

> 
> This patchset modifies the probe function to do some changes such as 
> switching to manage version of clk_get, fixing the error path by releasing
> resources etc. The last patch adds a remove function for clean unload.
> 
> Pramod Gurav (4):
>   pinctrl: at91: Switch to using managed clk_get
>   pinctrl: at91: Fix failure path in at91_gpio_probe path
>   pinctrl: at91: Fix error handling while doing gpiochio_irqchip_add
>   pinctrl: at91: Add remove function in platform_driver
>     at91_gpio_driver
> 
>  drivers/pinctrl/pinctrl-at91.c |   51
+++++++++++++++++++++++++++++-----------
>  1 files changed, 37 insertions(+), 14 deletions(-)

Hi,

Thanks a lot for your enhancement of the at91 pinctrl driver. In the future,
can you send your patch series to the Linux ARM kernel mailing-list as well
(linux-arm-kernel@lists.infradead.org). In fact it is where all the work
around AT91 platforms is being done: you may have more chances to get your
patch accepted by sending your work there.

Anyway, it seems that Linus got your emails and they look pretty good at a
first glance.

Best regards,
-- 
Nicolas Ferre