| Message ID | 20241206214315.3385033-1-zmw12306@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] pinctrl: stm32: Add check for clk_enable() | expand |
On 12/6/24 10:43 PM, Mingwei Zheng wrote: [...] > @@ -1646,8 +1645,8 @@ int stm32_pctl_probe(struct platform_device *pdev) > if (ret) { > fwnode_handle_put(child); > > - for (i = 0; i < pctl->nbanks; i++) > - clk_disable_unprepare(pctl->banks[i].clk); > + clk_bulk_disable(pctl->nbanks, pctl->clks); > + clk_bulk_unprepare(pctl->nbanks, pctl->clks); Use clk_bulk_disable_unprepare() include/linux/clk.h:static inline void clk_bulk_disable_unprepare(int num_clks, It looks pretty good otherwise, thanks !
On Fri, 2024-12-06 at 16:43 -0500, Mingwei Zheng wrote: > Convert the driver to clk_bulk*() API. > Add check for the return value of clk_bulk_enable() to catch > the potential error. > > Fixes: 05d8af449d93 ("pinctrl: stm32: Keep pinctrl block clock enabled when LEVEL IRQ requested") > Signed-off-by: Mingwei Zheng <zmw12306@gmail.com> > Signed-off-by: Jiasheng Jiang <jiashengjiangcool@gmail.com> > --- > Changelog: > > v2 -> v3: > > 1. Convert clk_disable_unprepare to clk_bulk_disable > and clk_bulk_unprepare. > > v1 -> v2: > > 1. Move int ret declaration into if block. > --- > drivers/pinctrl/stm32/pinctrl-stm32.c | 28 +++++++++++++-------------- > 1 file changed, 13 insertions(+), 15 deletions(-) > > diff --git a/drivers/pinctrl/stm32/pinctrl-stm32.c b/drivers/pinctrl/stm32/pinctrl-stm32.c > index 5b7fa77c1184..0ef912e82736 100644 > --- a/drivers/pinctrl/stm32/pinctrl-stm32.c > +++ b/drivers/pinctrl/stm32/pinctrl-stm32.c > @@ -86,7 +86,6 @@ struct stm32_pinctrl_group { > > struct stm32_gpio_bank { > void __iomem *base; > - struct clk *clk; > struct reset_control *rstc; > spinlock_t lock; > struct gpio_chip gpio_chip; > @@ -108,6 +107,7 @@ struct stm32_pinctrl { > unsigned ngroups; > const char **grp_names; > struct stm32_gpio_bank *banks; Thanks for your patch! While it's growing wider than your initial scope, it looks as a nice improvement. > + struct clk_bulk_data *clks; Maybe I've missed it but, where does this array get allocated? There exist clk_bulk_get_all() and devm_clk_bulk_get_all() that do the allocation for you, but they require all the clocks to be listed in the same "clocks" property, while this driver has a clock in each DT subnode! Some suggestion below. > unsigned nbanks; > const struct stm32_pinctrl_match_data *match_data; > struct irq_domain *domain; > @@ -1308,7 +1308,7 @@ static int stm32_gpiolib_register_bank(struct stm32_pinctrl *pctl, struct fwnode > if (IS_ERR(bank->base)) > return PTR_ERR(bank->base); > > - err = clk_prepare_enable(bank->clk); > + err = clk_prepare_enable(pctl->clks[pctl->nbanks].clk); > if (err) { > dev_err(dev, "failed to prepare_enable clk (%d)\n", err); > return err; > @@ -1397,7 +1397,7 @@ static int stm32_gpiolib_register_bank(struct stm32_pinctrl *pctl, struct fwnode > return 0; > > err_clk: > - clk_disable_unprepare(bank->clk); > + clk_disable_unprepare(pctl->clks[pctl->nbanks].clk); > return err; > } > > @@ -1631,11 +1631,10 @@ int stm32_pctl_probe(struct platform_device *pdev) > fwnode_handle_put(child); > return -EPROBE_DEFER; > } > - > - bank->clk = of_clk_get_by_name(np, NULL); > - if (IS_ERR(bank->clk)) { > + pctl->clks[i].clk = of_clk_get_by_name(np, NULL); This is the first time the array is used, during stm32_pctl_probe(), so the array should be already allocated before this line. The number of banks pctl->nbanks is computed few lines below, so it should be anticipated, or being replaced by a simpler pctl->nbanks = gpiochip_node_count(dev); then allocation could be done with pctl->clks = devm_kmalloc_array(dev, pctl->nbanks, sizeof(*pctl->clks), GFP_KERNEL); Also, you never assign `pctl->clks[i].id` which is then used by the error messages in, e.g., clk_bulk_enable(). We don't mandate the property "clock-names" in the DT subnodes, so no way to get a reasonable value from the DT. I presume it's fine to set it to NULL... to be tested. Regards, Antonio > + if (IS_ERR(pctl->clks[i].clk)) { > fwnode_handle_put(child); > - return dev_err_probe(dev, PTR_ERR(bank->clk), > + return dev_err_probe(dev, PTR_ERR(pctl->clks[i].clk), > "failed to get clk\n"); > } > i++; > @@ -1646,8 +1645,8 @@ int stm32_pctl_probe(struct platform_device *pdev) > if (ret) { > fwnode_handle_put(child); > > - for (i = 0; i < pctl->nbanks; i++) > - clk_disable_unprepare(pctl->banks[i].clk); > + clk_bulk_disable(pctl->nbanks, pctl->clks); > + clk_bulk_unprepare(pctl->nbanks, pctl->clks); > > return ret; > } > @@ -1726,10 +1725,8 @@ static int __maybe_unused stm32_pinctrl_restore_gpio_regs( > int __maybe_unused stm32_pinctrl_suspend(struct device *dev) > { > struct stm32_pinctrl *pctl = dev_get_drvdata(dev); > - int i; > > - for (i = 0; i < pctl->nbanks; i++) > - clk_disable(pctl->banks[i].clk); > + clk_bulk_disable(pctl->nbanks, pctl->clks); > > return 0; > } > @@ -1738,10 +1735,11 @@ int __maybe_unused stm32_pinctrl_resume(struct device *dev) > { > struct stm32_pinctrl *pctl = dev_get_drvdata(dev); > struct stm32_pinctrl_group *g = pctl->groups; > - int i; > + int i, ret; > > - for (i = 0; i < pctl->nbanks; i++) > - clk_enable(pctl->banks[i].clk); > + ret = clk_bulk_enable(pctl->nbanks, pctl->clks); > + if (ret) > + return ret; > > for (i = 0; i < pctl->ngroups; i++, g++) > stm32_pinctrl_restore_gpio_regs(pctl, g->pin);
Hi Antonio, On Sat, Dec 7, 2024 at 9:25 AM Antonio Borneo <antonio.borneo@foss.st.com> wrote: > > On Fri, 2024-12-06 at 16:43 -0500, Mingwei Zheng wrote: > > Convert the driver to clk_bulk*() API. > > Add check for the return value of clk_bulk_enable() to catch > > the potential error. > > > > Fixes: 05d8af449d93 ("pinctrl: stm32: Keep pinctrl block clock enabled when LEVEL IRQ requested") > > Signed-off-by: Mingwei Zheng <zmw12306@gmail.com> > > Signed-off-by: Jiasheng Jiang <jiashengjiangcool@gmail.com> > > --- > > Changelog: > > > > v2 -> v3: > > > > 1. Convert clk_disable_unprepare to clk_bulk_disable > > and clk_bulk_unprepare. > > > > v1 -> v2: > > > > 1. Move int ret declaration into if block. > > --- > > drivers/pinctrl/stm32/pinctrl-stm32.c | 28 +++++++++++++-------------- > > 1 file changed, 13 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/pinctrl/stm32/pinctrl-stm32.c b/drivers/pinctrl/stm32/pinctrl-stm32.c > > index 5b7fa77c1184..0ef912e82736 100644 > > --- a/drivers/pinctrl/stm32/pinctrl-stm32.c > > +++ b/drivers/pinctrl/stm32/pinctrl-stm32.c > > @@ -86,7 +86,6 @@ struct stm32_pinctrl_group { > > > > struct stm32_gpio_bank { > > void __iomem *base; > > - struct clk *clk; > > struct reset_control *rstc; > > spinlock_t lock; > > struct gpio_chip gpio_chip; > > @@ -108,6 +107,7 @@ struct stm32_pinctrl { > > unsigned ngroups; > > const char **grp_names; > > struct stm32_gpio_bank *banks; > > Thanks for your patch! > While it's growing wider than your initial scope, it looks as a nice improvement. > > > + struct clk_bulk_data *clks; > > Maybe I've missed it but, where does this array get allocated? > There exist clk_bulk_get_all() and devm_clk_bulk_get_all() that do the allocation for you, but they require all the clocks to be listed in the same "clocks" property, while this driver has a clock in > each DT subnode! > Some suggestion below. > > > unsigned nbanks; > > const struct stm32_pinctrl_match_data *match_data; > > struct irq_domain *domain; > > @@ -1308,7 +1308,7 @@ static int stm32_gpiolib_register_bank(struct stm32_pinctrl *pctl, struct fwnode > > if (IS_ERR(bank->base)) > > return PTR_ERR(bank->base); > > > > - err = clk_prepare_enable(bank->clk); > > + err = clk_prepare_enable(pctl->clks[pctl->nbanks].clk); > > if (err) { > > dev_err(dev, "failed to prepare_enable clk (%d)\n", err); > > return err; > > @@ -1397,7 +1397,7 @@ static int stm32_gpiolib_register_bank(struct stm32_pinctrl *pctl, struct fwnode > > return 0; > > > > err_clk: > > - clk_disable_unprepare(bank->clk); > > + clk_disable_unprepare(pctl->clks[pctl->nbanks].clk); > > return err; > > } > > > > @@ -1631,11 +1631,10 @@ int stm32_pctl_probe(struct platform_device *pdev) > > fwnode_handle_put(child); > > return -EPROBE_DEFER; > > } > > - > > - bank->clk = of_clk_get_by_name(np, NULL); > > - if (IS_ERR(bank->clk)) { > > + pctl->clks[i].clk = of_clk_get_by_name(np, NULL); > > This is the first time the array is used, during stm32_pctl_probe(), so the array should be already allocated before this line. > The number of banks pctl->nbanks is computed few lines below, so it should be anticipated, or being replaced by a simpler > pctl->nbanks = gpiochip_node_count(dev); > > then allocation could be done with > pctl->clks = devm_kmalloc_array(dev, pctl->nbanks, sizeof(*pctl->clks), GFP_KERNEL); > > Also, you never assign `pctl->clks[i].id` which is then used by the error messages in, e.g., clk_bulk_enable(). > We don't mandate the property "clock-names" in the DT subnodes, so no way to get a reasonable value from the DT. > I presume it's fine to set it to NULL... to be tested. > > Regards, > Antonio > > Thanks for your suggestion. We used devm_kcalloc to allocate clk. We set `pctl->clks[i].id` to "". Best, Mingwei > > + if (IS_ERR(pctl->clks[i].clk)) { > > fwnode_handle_put(child); > > - return dev_err_probe(dev, PTR_ERR(bank->clk), > > + return dev_err_probe(dev, PTR_ERR(pctl->clks[i].clk), > > "failed to get clk\n"); > > } > > i++; > > @@ -1646,8 +1645,8 @@ int stm32_pctl_probe(struct platform_device *pdev) > > if (ret) { > > fwnode_handle_put(child); > > > > - for (i = 0; i < pctl->nbanks; i++) > > - clk_disable_unprepare(pctl->banks[i].clk); > > + clk_bulk_disable(pctl->nbanks, pctl->clks); > > + clk_bulk_unprepare(pctl->nbanks, pctl->clks); > > > > return ret; > > } > > @@ -1726,10 +1725,8 @@ static int __maybe_unused stm32_pinctrl_restore_gpio_regs( > > int __maybe_unused stm32_pinctrl_suspend(struct device *dev) > > { > > struct stm32_pinctrl *pctl = dev_get_drvdata(dev); > > - int i; > > > > - for (i = 0; i < pctl->nbanks; i++) > > - clk_disable(pctl->banks[i].clk); > > + clk_bulk_disable(pctl->nbanks, pctl->clks); > > > > return 0; > > } > > @@ -1738,10 +1735,11 @@ int __maybe_unused stm32_pinctrl_resume(struct device *dev) > > { > > struct stm32_pinctrl *pctl = dev_get_drvdata(dev); > > struct stm32_pinctrl_group *g = pctl->groups; > > - int i; > > + int i, ret; > > > > - for (i = 0; i < pctl->nbanks; i++) > > - clk_enable(pctl->banks[i].clk); > > + ret = clk_bulk_enable(pctl->nbanks, pctl->clks); > > + if (ret) > > + return ret; > > > > for (i = 0; i < pctl->ngroups; i++, g++) > > stm32_pinctrl_restore_gpio_regs(pctl, g->pin);
Hi Marek, On Sat, Dec 7, 2024 at 7:59 AM Marek Vasut <marex@denx.de> wrote: > > On 12/6/24 10:43 PM, Mingwei Zheng wrote: > > [...] > > > @@ -1646,8 +1645,8 @@ int stm32_pctl_probe(struct platform_device *pdev) > > if (ret) { > > fwnode_handle_put(child); > > > > - for (i = 0; i < pctl->nbanks; i++) > > - clk_disable_unprepare(pctl->banks[i].clk); > > + clk_bulk_disable(pctl->nbanks, pctl->clks); > > + clk_bulk_unprepare(pctl->nbanks, pctl->clks); > > Use clk_bulk_disable_unprepare() > > include/linux/clk.h:static inline void clk_bulk_disable_unprepare(int > num_clks, Thanks! We used clk_bulk_disable_unprepare() in the patch v4. Best, Mingwei > > It looks pretty good otherwise, thanks !
diff --git a/drivers/pinctrl/stm32/pinctrl-stm32.c b/drivers/pinctrl/stm32/pinctrl-stm32.c index 5b7fa77c1184..0ef912e82736 100644 --- a/drivers/pinctrl/stm32/pinctrl-stm32.c +++ b/drivers/pinctrl/stm32/pinctrl-stm32.c @@ -86,7 +86,6 @@ struct stm32_pinctrl_group { struct stm32_gpio_bank { void __iomem *base; - struct clk *clk; struct reset_control *rstc; spinlock_t lock; struct gpio_chip gpio_chip; @@ -108,6 +107,7 @@ struct stm32_pinctrl { unsigned ngroups; const char **grp_names; struct stm32_gpio_bank *banks; + struct clk_bulk_data *clks; unsigned nbanks; const struct stm32_pinctrl_match_data *match_data; struct irq_domain *domain; @@ -1308,7 +1308,7 @@ static int stm32_gpiolib_register_bank(struct stm32_pinctrl *pctl, struct fwnode if (IS_ERR(bank->base)) return PTR_ERR(bank->base); - err = clk_prepare_enable(bank->clk); + err = clk_prepare_enable(pctl->clks[pctl->nbanks].clk); if (err) { dev_err(dev, "failed to prepare_enable clk (%d)\n", err); return err; @@ -1397,7 +1397,7 @@ static int stm32_gpiolib_register_bank(struct stm32_pinctrl *pctl, struct fwnode return 0; err_clk: - clk_disable_unprepare(bank->clk); + clk_disable_unprepare(pctl->clks[pctl->nbanks].clk); return err; } @@ -1631,11 +1631,10 @@ int stm32_pctl_probe(struct platform_device *pdev) fwnode_handle_put(child); return -EPROBE_DEFER; } - - bank->clk = of_clk_get_by_name(np, NULL); - if (IS_ERR(bank->clk)) { + pctl->clks[i].clk = of_clk_get_by_name(np, NULL); + if (IS_ERR(pctl->clks[i].clk)) { fwnode_handle_put(child); - return dev_err_probe(dev, PTR_ERR(bank->clk), + return dev_err_probe(dev, PTR_ERR(pctl->clks[i].clk), "failed to get clk\n"); } i++; @@ -1646,8 +1645,8 @@ int stm32_pctl_probe(struct platform_device *pdev) if (ret) { fwnode_handle_put(child); - for (i = 0; i < pctl->nbanks; i++) - clk_disable_unprepare(pctl->banks[i].clk); + clk_bulk_disable(pctl->nbanks, pctl->clks); + clk_bulk_unprepare(pctl->nbanks, pctl->clks); return ret; } @@ -1726,10 +1725,8 @@ static int __maybe_unused stm32_pinctrl_restore_gpio_regs( int __maybe_unused stm32_pinctrl_suspend(struct device *dev) { struct stm32_pinctrl *pctl = dev_get_drvdata(dev); - int i; - for (i = 0; i < pctl->nbanks; i++) - clk_disable(pctl->banks[i].clk); + clk_bulk_disable(pctl->nbanks, pctl->clks); return 0; } @@ -1738,10 +1735,11 @@ int __maybe_unused stm32_pinctrl_resume(struct device *dev) { struct stm32_pinctrl *pctl = dev_get_drvdata(dev); struct stm32_pinctrl_group *g = pctl->groups; - int i; + int i, ret; - for (i = 0; i < pctl->nbanks; i++) - clk_enable(pctl->banks[i].clk); + ret = clk_bulk_enable(pctl->nbanks, pctl->clks); + if (ret) + return ret; for (i = 0; i < pctl->ngroups; i++, g++) stm32_pinctrl_restore_gpio_regs(pctl, g->pin);
Convert the driver to clk_bulk*() API. Add check for the return value of clk_bulk_enable() to catch the potential error. Fixes: 05d8af449d93 ("pinctrl: stm32: Keep pinctrl block clock enabled when LEVEL IRQ requested") Signed-off-by: Mingwei Zheng <zmw12306@gmail.com> Signed-off-by: Jiasheng Jiang <jiashengjiangcool@gmail.com> --- Changelog: v2 -> v3: 1. Convert clk_disable_unprepare to clk_bulk_disable and clk_bulk_unprepare. v1 -> v2: 1. Move int ret declaration into if block. --- drivers/pinctrl/stm32/pinctrl-stm32.c | 28 +++++++++++++-------------- 1 file changed, 13 insertions(+), 15 deletions(-)