diff mbox series

[V3,04/10] gpio: vf610: add optional clock support

Message ID 1540996688-23681-5-git-send-email-aisheng.dong@nxp.com
State New
Headers show
Series None | expand

Commit Message

Dong Aisheng Oct. 31, 2018, 2:43 p.m. UTC 
Some SoCs need the gpio clock to be enabled before accessing
HW registers. This patch add the optional clock handling.

Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Stefan Agner <stefan@agner.ch>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: linux-gpio@vger.kernel.org
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
v2->v3:
 * error checking updated according to Russell's suggestion:
   ptr == ERR_PTR(-EPROBE_DEFER)
 * clock independently checking
v1->v2:
 * new patch
---
 drivers/gpio/gpio-vf610.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

Comments

Linus Walleij Nov. 2, 2018, 9:32 a.m. UTC | #1 
On Wed, Oct 31, 2018 at 3:43 PM A.s. Dong <aisheng.dong@nxp.com> wrote:

> +       clk_port = devm_clk_get(&pdev->dev, "port");
> +       if (clk_port == ERR_PTR(-EPROBE_DEFER))
> +               return -EPROBE_DEFER;
> +
> +       clk_gpio = devm_clk_get(&pdev->dev, "gpio");
> +       if (clk_gpio == ERR_PTR(-EPROBE_DEFER))
> +               return -EPROBE_DEFER;
> +
> +       if (!IS_ERR_OR_NULL(clk_port)) {
> +               ret = clk_prepare_enable(clk_port);
> +               if (ret)
> +                       return ret;
> +       }
> +
> +       if (!IS_ERR_OR_NULL(clk_gpio)) {
> +               ret = clk_prepare_enable(clk_gpio);
> +               if (ret) {
> +                       clk_disable_unprepare(clk_port);
> +                       return ret;
> +               }
> +       }

I think IS_ERR_OR_NULL() is considered harmful among other
things.

What about this pattern:

        clk = devm_clk_get(dev, "foo");
        if (!IS_ERR(clk)) {
                ret = clk_prepare_enable(clk);
                if (ret)
                        return ret;
        } else if (PTR_ERR(clk) == -EPROBE_DEFER) {
                /*
                 * Percolate deferrals, for anything else,
                 * just live without the clocking.
                 */
                return PTR_ERR(clk);
        }

You also need to introduce code to disable the clock on
.remove() or the clock will always be on after this module
has probed. This applies also to builtin drivers, unless
you suppress the sysfs attributes and use
platform_driver_probe()

Yours,
Linus Walleij
Dong Aisheng Nov. 5, 2018, 1:34 p.m. UTC | #2 
> -----Original Message-----
> From: Linus Walleij [mailto:linus.walleij@linaro.org]
> Sent: Friday, November 2, 2018 5:33 PM
[...]
> On Wed, Oct 31, 2018 at 3:43 PM A.s. Dong <aisheng.dong@nxp.com> wrote:
> 
> > +       clk_port = devm_clk_get(&pdev->dev, "port");
> > +       if (clk_port == ERR_PTR(-EPROBE_DEFER))
> > +               return -EPROBE_DEFER;
> > +
> > +       clk_gpio = devm_clk_get(&pdev->dev, "gpio");
> > +       if (clk_gpio == ERR_PTR(-EPROBE_DEFER))
> > +               return -EPROBE_DEFER;
> > +
> > +       if (!IS_ERR_OR_NULL(clk_port)) {
> > +               ret = clk_prepare_enable(clk_port);
> > +               if (ret)
> > +                       return ret;
> > +       }
> > +
> > +       if (!IS_ERR_OR_NULL(clk_gpio)) {
> > +               ret = clk_prepare_enable(clk_gpio);
> > +               if (ret) {
> > +                       clk_disable_unprepare(clk_port);
> > +                       return ret;
> > +               }
> > +       }
> 
> I think IS_ERR_OR_NULL() is considered harmful among other things.
> 
> What about this pattern:
> 
>         clk = devm_clk_get(dev, "foo");
>         if (!IS_ERR(clk)) {
>                 ret = clk_prepare_enable(clk);
>                 if (ret)
>                         return ret;
>         } else if (PTR_ERR(clk) == -EPROBE_DEFER) {
>                 /*
>                  * Percolate deferrals, for anything else,
>                  * just live without the clocking.
>                  */
>                 return PTR_ERR(clk);
>         }
> 
> You also need to introduce code to disable the clock on
> .remove() or the clock will always be on after this module has probed. This
> applies also to builtin drivers, unless you suppress the sysfs attributes and use
> platform_driver_probe()
> 

Looks good to me. Will update the patch.
Thanks for the suggestion.

Regards
Dong Aisheng

> Yours,
> Linus Walleij
diff mbox series

Patch

diff --git a/drivers/gpio/gpio-vf610.c b/drivers/gpio/gpio-vf610.c
index d4ad6d0..02fb7d8 100644
--- a/drivers/gpio/gpio-vf610.c
+++ b/drivers/gpio/gpio-vf610.c
@@ -16,6 +16,7 @@ 
  */
 
 #include <linux/bitops.h>
+#include <linux/clk.h>
 #include <linux/err.h>
 #include <linux/gpio.h>
 #include <linux/init.h>
@@ -256,6 +257,7 @@  static int vf610_gpio_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node;
+	struct clk *clk_gpio, *clk_port;
 	struct vf610_gpio_port *port;
 	struct resource *iores;
 	struct gpio_chip *gc;
@@ -280,6 +282,28 @@  static int vf610_gpio_probe(struct platform_device *pdev)
 	if (port->irq < 0)
 		return port->irq;
 
+	clk_port = devm_clk_get(&pdev->dev, "port");
+	if (clk_port == ERR_PTR(-EPROBE_DEFER))
+		return -EPROBE_DEFER;
+
+	clk_gpio = devm_clk_get(&pdev->dev, "gpio");
+	if (clk_gpio == ERR_PTR(-EPROBE_DEFER))
+		return -EPROBE_DEFER;
+
+	if (!IS_ERR_OR_NULL(clk_port)) {
+		ret = clk_prepare_enable(clk_port);
+		if (ret)
+			return ret;
+	}
+
+	if (!IS_ERR_OR_NULL(clk_gpio)) {
+		ret = clk_prepare_enable(clk_gpio);
+		if (ret) {
+			clk_disable_unprepare(clk_port);
+			return ret;
+		}
+	}
+
 	gc = &port->gc;
 	gc->of_node = np;
 	gc->parent = dev;