| Message ID | 1540996688-23681-5-git-send-email-aisheng.dong@nxp.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | ARM: imx: add imx7ulp support | expand |
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
> -----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 --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;
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(+)