Series add bd71837/bd71837 PMIC clock support + managed interfaces Few clk drivers appear to be leaking clkdev lookup registrations at driver remove. The patch series adds devm versions of lookup registrations and cleans up few drivers. Driver clean-up patches have not been tested as I lack the HW. All testing and comments if driver/device removal is even possible for changed drivers is highly appreciated. If removal is not possible I will gladly drop the patches from series - although leaking lookups may serve as bad example for new developers =) Patch 8 adds support for clock gate in ROHM bd71837 and bd71847 PMICs. This change is included in the series because it depends on new managed interfaces introduced in this series. bd718x7 driver and devm interfaces are tested on BeagleBoneBlack and bd71837 break-out board. Clk area register interface of bd71847 is identical to bd71837. Changed drivers are: clk-max77686, clk-st, clk-hi655x, rk808, clk-twl6040 and apcs-msm8916. New driver is clk-bd718x7 This series has been discussed for a while now. For those who want to see whole discussion: The bd71837 driver was originally proposed here https://lore.kernel.org/lkml/d99c8762b0fbbcb18ec4f4d104191364c0ea798c.1528117485.git.matti.vaittinen@fi.rohmeurope.com/ clk portion was separated from that series and devm variants were proposed here https://lore.kernel.org/linux-clk/cover.1535630942.git.matti.vaittinen@fi.rohmeurope.com/ Cleanup to other drivers was initiated in this series while waiting for MFD portions of bd718x7 to be applied. And now when MFD dependencies are in-tree his version (4) combines bd718x7 driver back to this series. Changelog (for this series) v4 - Add support for ROHM bd718x7 PMIC clock gate. Included in this patch series because it depends on managed interfaces added in patch 1. Changelog (for this series) v3 Address issues spotted by Krzysztof Kozlowski - Drop patch 3 for clk-s3c2410-dclk as this device can never be removed - Fix indentiation for clk-max77686 - Rest of the patches are unchanged. Changelog (for this series) v2 Issue spotted by 0-Day test suite - Add a stub function 'devm_of_clk_add_parent_hw_provider' for no OF config. - patches 2-8 are unchanged. This patch series is based on clk-next --- Matti Vaittinen (8): clk: clkdev/of_clk - add managed lookup and provider registrations clk: clk-max77686: Clean clkdev lookup leak and use devm clk: clk-st: avoid clkdev lookup leak at remove clk: clk-hi655x: Free of_provider at remove clk: rk808: use managed version of of_provider registration clk: clk-twl6040: Free of_provider at remove clk: apcs-msm8916: simplify probe cleanup by using devm clk: bd718x7: Initial support for ROHM bd71837/bd71847 PMIC clock Documentation/driver-model/devres.txt | 3 + drivers/clk/Kconfig | 7 ++ drivers/clk/Makefile | 1 + drivers/clk/clk-bd718x7.c | 131 ++++++++++++++++++++++++++++++++++ drivers/clk/clk-hi655x.c | 4 +- drivers/clk/clk-max77686.c | 29 ++------ drivers/clk/clk-rk808.c | 15 +--- drivers/clk/clk-twl6040.c | 5 +- drivers/clk/clk.c | 28 ++++++-- drivers/clk/clkdev.c | 122 ++++++++++++++++++++++++------- drivers/clk/qcom/apcs-msm8916.c | 5 +- drivers/clk/x86/clk-st.c | 3 +- include/linux/clk-provider.h | 11 +++ include/linux/clkdev.h | 4 ++ 14 files changed, 292 insertions(+), 76 deletions(-) create mode 100644 drivers/clk/clk-bd718x7.c -- 2.14.3 -- Matti Vaittinen ROHM Semiconductors ~~~ "I don't think so," said Rene Descartes. Just then, he vanished ~~~
With MFD devices the clk properties may be contained in MFD (parent) DT node. Current devm_of_clk_add_hw_provider assumes the clk is bound to MFD subdevice not to MFD device (parent). Add devm_of_clk_add_hw_provider_parent to tackle this issue. Also clkdev registration lacks of managed registration functions and it seems few drivers do not drop clkdev lookups at exit. Add devm_clk_hw_register_clkdev and devm_clk_release_clkdev to ease lookup releasing at exit. Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> --- Documentation/driver-model/devres.txt | 3 + drivers/clk/clk.c | 28 ++++++-- drivers/clk/clkdev.c | 122 ++++++++++++++++++++++++++-------- include/linux/clk-provider.h | 11 +++ include/linux/clkdev.h | 4 ++ 5 files changed, 136 insertions(+), 32 deletions(-) diff --git a/Documentation/driver-model/devres.txt b/Documentation/driver-model/devres.txt index 43681ca0837f..fac63760b01c 100644 --- a/Documentation/driver-model/devres.txt +++ b/Documentation/driver-model/devres.txt @@ -238,6 +238,9 @@ CLOCK devm_clk_put() devm_clk_hw_register() devm_of_clk_add_hw_provider() + devm_of_clk_add_parent_hw_provider() + devm_clk_hw_register_clkdev() + devm_clk_release_clkdev() DMA dmaenginem_async_device_register() diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index af011974d4ec..9bb921eb90f6 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -3893,12 +3893,12 @@ static void devm_of_clk_release_provider(struct device *dev, void *res) of_clk_del_provider(*(struct device_node **)res); } -int devm_of_clk_add_hw_provider(struct device *dev, +static int __devm_of_clk_add_hw_provider(struct device *dev, struct clk_hw *(*get)(struct of_phandle_args *clkspec, void *data), - void *data) + struct device_node *of_node, void *data) { - struct device_node **ptr, *np; + struct device_node **ptr; int ret; ptr = devres_alloc(devm_of_clk_release_provider, sizeof(*ptr), @@ -3906,10 +3906,9 @@ int devm_of_clk_add_hw_provider(struct device *dev, if (!ptr) return -ENOMEM; - np = dev->of_node; - ret = of_clk_add_hw_provider(np, get, data); + *ptr = of_node; + ret = of_clk_add_hw_provider(of_node, get, data); if (!ret) { - *ptr = np; devres_add(dev, ptr); } else { devres_free(ptr); @@ -3917,8 +3916,25 @@ int devm_of_clk_add_hw_provider(struct device *dev, return ret; } +int devm_of_clk_add_hw_provider(struct device *dev, + struct clk_hw *(*get)(struct of_phandle_args *clkspec, + void *data), + void *data) +{ + return __devm_of_clk_add_hw_provider(dev, get, dev->of_node, data); +} EXPORT_SYMBOL_GPL(devm_of_clk_add_hw_provider); +int devm_of_clk_add_parent_hw_provider(struct device *dev, + struct clk_hw *(*get)(struct of_phandle_args *clkspec, + void *data), + void *data) +{ + return __devm_of_clk_add_hw_provider(dev, get, dev->parent->of_node, + data); +} +EXPORT_SYMBOL_GPL(devm_of_clk_add_parent_hw_provider); + /** * of_clk_del_provider() - Remove a previously registered clock provider * @np: Device node pointer associated with clock provider diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c index 9ab3db8b3988..f6100b6e06fd 100644 --- a/drivers/clk/clkdev.c +++ b/drivers/clk/clkdev.c @@ -401,6 +401,25 @@ static struct clk_lookup *__clk_register_clkdev(struct clk_hw *hw, return cl; } +static int do_clk_register_clkdev(struct clk_hw *hw, + struct clk_lookup **cl, const char *con_id, const char *dev_id) +{ + + if (IS_ERR(hw)) + return PTR_ERR(hw); + /* + * Since dev_id can be NULL, and NULL is handled specially, we must + * pass it as either a NULL format string, or with "%s". + */ + if (dev_id) + *cl = __clk_register_clkdev(hw, con_id, "%s", + dev_id); + else + *cl = __clk_register_clkdev(hw, con_id, NULL); + + return *cl ? 0 : -ENOMEM; +} + /** * clk_register_clkdev - register one clock lookup for a struct clk * @clk: struct clk to associate with all clk_lookups @@ -420,20 +439,10 @@ int clk_register_clkdev(struct clk *clk, const char *con_id, { struct clk_lookup *cl; - if (IS_ERR(clk)) - return PTR_ERR(clk); - - /* - * Since dev_id can be NULL, and NULL is handled specially, we must - * pass it as either a NULL format string, or with "%s". - */ - if (dev_id) - cl = __clk_register_clkdev(__clk_get_hw(clk), con_id, "%s", - dev_id); - else - cl = __clk_register_clkdev(__clk_get_hw(clk), con_id, NULL); - - return cl ? 0 : -ENOMEM; + if (!IS_ERR(clk)) + return do_clk_register_clkdev(__clk_get_hw(clk), &cl, con_id, + dev_id); + return PTR_ERR(clk); } EXPORT_SYMBOL(clk_register_clkdev); @@ -456,18 +465,79 @@ int clk_hw_register_clkdev(struct clk_hw *hw, const char *con_id, { struct clk_lookup *cl; - if (IS_ERR(hw)) - return PTR_ERR(hw); + return do_clk_register_clkdev(hw, &cl, con_id, dev_id); +} +EXPORT_SYMBOL(clk_hw_register_clkdev); - /* - * Since dev_id can be NULL, and NULL is handled specially, we must - * pass it as either a NULL format string, or with "%s". - */ - if (dev_id) - cl = __clk_register_clkdev(hw, con_id, "%s", dev_id); - else - cl = __clk_register_clkdev(hw, con_id, NULL); +static void devm_clkdev_release(struct device *dev, void *res) +{ + clkdev_drop(*(struct clk_lookup **)res); +} - return cl ? 0 : -ENOMEM; +static int devm_clk_match_clkdev(struct device *dev, void *res, void *data) +{ + struct clk_lookup **l = res; + + if (!l || !*l) { + WARN_ON(!l || !*l); + return 0; + } + return *l == data; } -EXPORT_SYMBOL(clk_hw_register_clkdev); + +/** + * devm_clk_release_clkdev - Resource managed clkdev lookup release + * @dev: device this lookup is bound + * @con_id: connection ID string on device + * @dev_id: format string describing device name + * + * Drop the clkdev lookup created with devm_clk_hw_register_clkdev. + * Normally this function will not need to be called and the resource + * management code will ensure that the resource is freed. + */ +void devm_clk_release_clkdev(struct device *dev, const char *con_id, + const char *dev_id) +{ + struct clk_lookup *cl; + int rval; + + cl = clk_find(dev_id, con_id); + WARN_ON(!cl); + rval = devres_release(dev, devm_clkdev_release, + &devm_clk_match_clkdev, cl); + WARN_ON(rval); +} +EXPORT_SYMBOL(devm_clk_release_clkdev); + +/** + * devm_clk_hw_register_clkdev - managed clk lookup registration for clk_hw + * @dev: device this lookup is bound + * @hw: struct clk_hw to associate with all clk_lookups + * @con_id: connection ID string on device + * @dev_id: format string describing device name + * + * con_id or dev_id may be NULL as a wildcard, just as in the rest of + * clkdev. + * + * To make things easier for mass registration, we detect error clk_hws + * from a previous clk_hw_register_*() call, and return the error code for + * those. This is to permit this function to be called immediately + * after clk_hw_register_*(). + */ +int devm_clk_hw_register_clkdev(struct device *dev, struct clk_hw *hw, + const char *con_id, const char *dev_id) +{ + int rval = -ENOMEM; + struct clk_lookup **cl; + + cl = devres_alloc(devm_clkdev_release, sizeof(*cl), GFP_KERNEL); + if (cl) { + rval = do_clk_register_clkdev(hw, cl, con_id, dev_id); + if (!rval) + devres_add(dev, cl); + else + devres_free(cl); + } + return rval; +} +EXPORT_SYMBOL(devm_clk_hw_register_clkdev); diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h index 60c51871b04b..a6663f084cf1 100644 --- a/include/linux/clk-provider.h +++ b/include/linux/clk-provider.h @@ -916,6 +916,10 @@ int devm_of_clk_add_hw_provider(struct device *dev, struct clk_hw *(*get)(struct of_phandle_args *clkspec, void *data), void *data); +int devm_of_clk_add_parent_hw_provider(struct device *dev, + struct clk_hw *(*get)(struct of_phandle_args *clkspec, + void *data), + void *data); void of_clk_del_provider(struct device_node *np); void devm_of_clk_del_provider(struct device *dev); struct clk *of_clk_src_simple_get(struct of_phandle_args *clkspec, @@ -953,6 +957,13 @@ static inline int devm_of_clk_add_hw_provider(struct device *dev, { return 0; } +static inline int devm_of_clk_add_parent_hw_provider(struct device *dev, + struct clk_hw *(*get)(struct of_phandle_args *clkspec, + void *data), + void *data) +{ + return 0; +} static inline void of_clk_del_provider(struct device_node *np) {} static inline void devm_of_clk_del_provider(struct device *dev) {} static inline struct clk *of_clk_src_simple_get( diff --git a/include/linux/clkdev.h b/include/linux/clkdev.h index 4890ff033220..ccb32af5848b 100644 --- a/include/linux/clkdev.h +++ b/include/linux/clkdev.h @@ -52,4 +52,8 @@ int clk_add_alias(const char *, const char *, const char *, struct device *); int clk_register_clkdev(struct clk *, const char *, const char *); int clk_hw_register_clkdev(struct clk_hw *, const char *, const char *); +int devm_clk_hw_register_clkdev(struct device *dev, struct clk_hw *hw, + const char *con_id, const char *dev_id); +void devm_clk_release_clkdev(struct device *dev, const char *con_id, + const char *dev_id); #endif -- 2.14.3 -- Matti Vaittinen ROHM Semiconductors ~~~ "I don't think so," said Rene Descartes. Just then, he vanished ~~~
clk-max77686 never clean clkdev lookup at remove. This can cause oops if clk-max77686 is removed and inserted again. Fix leak by using new devm clkdev lookup registration. Simplify also error path by using new devm_of_clk_add_parent_hw_provider. Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org> --- drivers/clk/clk-max77686.c | 29 +++++++---------------------- 1 file changed, 7 insertions(+), 22 deletions(-) diff --git a/drivers/clk/clk-max77686.c b/drivers/clk/clk-max77686.c index 22c937644c93..e65925d5e412 100644 --- a/drivers/clk/clk-max77686.c +++ b/drivers/clk/clk-max77686.c @@ -235,8 +235,9 @@ static int max77686_clk_probe(struct platform_device *pdev) return ret; } - ret = clk_hw_register_clkdev(&max_clk_data->hw, - max_clk_data->clk_idata.name, NULL); + ret = devm_clk_hw_register_clkdev(dev, &max_clk_data->hw, + max_clk_data->clk_idata.name, + NULL); if (ret < 0) { dev_err(dev, "Failed to clkdev register: %d\n", ret); return ret; @@ -244,8 +245,9 @@ static int max77686_clk_probe(struct platform_device *pdev) } if (parent->of_node) { - ret = of_clk_add_hw_provider(parent->of_node, of_clk_max77686_get, - drv_data); + ret = devm_of_clk_add_parent_hw_provider(dev, + of_clk_max77686_get, + drv_data); if (ret < 0) { dev_err(dev, "Failed to register OF clock provider: %d\n", @@ -261,27 +263,11 @@ static int max77686_clk_probe(struct platform_device *pdev) 1 << MAX77802_CLOCK_LOW_JITTER_SHIFT); if (ret < 0) { dev_err(dev, "Failed to config low-jitter: %d\n", ret); - goto remove_of_clk_provider; + return ret; } } return 0; - -remove_of_clk_provider: - if (parent->of_node) - of_clk_del_provider(parent->of_node); - - return ret; -} - -static int max77686_clk_remove(struct platform_device *pdev) -{ - struct device *parent = pdev->dev.parent; - - if (parent->of_node) - of_clk_del_provider(parent->of_node); - - return 0; } static const struct platform_device_id max77686_clk_id[] = { @@ -297,7 +283,6 @@ static struct platform_driver max77686_clk_driver = { .name = "max77686-clk", }, .probe = max77686_clk_probe, - .remove = max77686_clk_remove, .id_table = max77686_clk_id, }; -- 2.14.3 -- Matti Vaittinen ROHM Semiconductors ~~~ "I don't think so," said Rene Descartes. Just then, he vanished ~~~
Use devm based clkdev lookup registration to avoid leaking lookup structures. Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> --- drivers/clk/x86/clk-st.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/clk/x86/clk-st.c b/drivers/clk/x86/clk-st.c index 3a0996f2d556..25d4b97aff9b 100644 --- a/drivers/clk/x86/clk-st.c +++ b/drivers/clk/x86/clk-st.c @@ -52,7 +52,8 @@ static int st_clk_probe(struct platform_device *pdev) 0, st_data->base + MISCCLKCNTL1, OSCCLKENB, CLK_GATE_SET_TO_DISABLE, NULL); - clk_hw_register_clkdev(hws[ST_CLK_GATE], "oscout1", NULL); + devm_clk_hw_register_clkdev(&pdev->dev, hws[ST_CLK_GATE], "oscout1", + NULL); return 0; } -- 2.14.3 -- Matti Vaittinen ROHM Semiconductors ~~~ "I don't think so," said Rene Descartes. Just then, he vanished ~~~
use devm variant for of_provider registration so provider is freed at exit. Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> --- drivers/clk/clk-hi655x.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/clk/clk-hi655x.c b/drivers/clk/clk-hi655x.c index 403a0188634a..394d0109104d 100644 --- a/drivers/clk/clk-hi655x.c +++ b/drivers/clk/clk-hi655x.c @@ -107,8 +107,8 @@ static int hi655x_clk_probe(struct platform_device *pdev) if (ret) return ret; - return of_clk_add_hw_provider(parent->of_node, of_clk_hw_simple_get, - &hi655x_clk->clk_hw); + return devm_of_clk_add_parent_hw_provider(&pdev->dev, + of_clk_hw_simple_get, &hi655x_clk->clk_hw); } static struct platform_driver hi655x_clk_driver = { -- 2.14.3 -- Matti Vaittinen ROHM Semiconductors ~~~ "I don't think so," said Rene Descartes. Just then, he vanished ~~~
Simplify clean-up for rk808 by using managed version of of_provider registration. Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> --- drivers/clk/clk-rk808.c | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-) diff --git a/drivers/clk/clk-rk808.c b/drivers/clk/clk-rk808.c index 6461f2820a5b..177340edaae5 100644 --- a/drivers/clk/clk-rk808.c +++ b/drivers/clk/clk-rk808.c @@ -138,23 +138,12 @@ static int rk808_clkout_probe(struct platform_device *pdev) if (ret) return ret; - return of_clk_add_hw_provider(node, of_clk_rk808_get, rk808_clkout); -} - -static int rk808_clkout_remove(struct platform_device *pdev) -{ - struct rk808 *rk808 = dev_get_drvdata(pdev->dev.parent); - struct i2c_client *client = rk808->i2c; - struct device_node *node = client->dev.of_node; - - of_clk_del_provider(node); - - return 0; + return devm_of_clk_add_parent_hw_provider(&pdev->dev, + of_clk_rk808_get, rk808_clkout); } static struct platform_driver rk808_clkout_driver = { .probe = rk808_clkout_probe, - .remove = rk808_clkout_remove, .driver = { .name = "rk808-clkout", }, -- 2.14.3 -- Matti Vaittinen ROHM Semiconductors ~~~ "I don't think so," said Rene Descartes. Just then, he vanished ~~~
use devm variant for of_provider registration so provider is freed at exit. Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> Acked-by: Peter Ujfalusi <peter.ujfalusi@ti.com> --- drivers/clk/clk-twl6040.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/drivers/clk/clk-twl6040.c b/drivers/clk/clk-twl6040.c index 25dfe050ae9f..e9da09453eb2 100644 --- a/drivers/clk/clk-twl6040.c +++ b/drivers/clk/clk-twl6040.c @@ -108,9 +108,8 @@ static int twl6040_pdmclk_probe(struct platform_device *pdev) platform_set_drvdata(pdev, clkdata); - return of_clk_add_hw_provider(pdev->dev.parent->of_node, - of_clk_hw_simple_get, - &clkdata->pdmclk_hw); + return devm_of_clk_add_parent_hw_provider(&pdev->dev, + of_clk_hw_simple_get, &clkdata->pdmclk_hw); } static struct platform_driver twl6040_pdmclk_driver = { -- 2.14.3 -- Matti Vaittinen ROHM Semiconductors ~~~ "I don't think so," said Rene Descartes. Just then, he vanished ~~~
use devm variant for of_provider registration. Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> --- drivers/clk/qcom/apcs-msm8916.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/drivers/clk/qcom/apcs-msm8916.c b/drivers/clk/qcom/apcs-msm8916.c index b1cc8dbcd327..f4e0c136ab1a 100644 --- a/drivers/clk/qcom/apcs-msm8916.c +++ b/drivers/clk/qcom/apcs-msm8916.c @@ -96,8 +96,8 @@ static int qcom_apcs_msm8916_clk_probe(struct platform_device *pdev) goto err; } - ret = of_clk_add_hw_provider(parent->of_node, of_clk_hw_simple_get, - &a53cc->clkr.hw); + ret = devm_of_clk_add_parent_hw_provider(dev, of_clk_hw_simple_get, + &a53cc->clkr.hw); if (ret) { dev_err(dev, "failed to add clock provider: %d\n", ret); goto err; @@ -118,7 +118,6 @@ static int qcom_apcs_msm8916_clk_remove(struct platform_device *pdev) struct device *parent = pdev->dev.parent; clk_notifier_unregister(a53cc->pclk, &a53cc->clk_nb); - of_clk_del_provider(parent->of_node); return 0; } -- 2.14.3 -- Matti Vaittinen ROHM Semiconductors ~~~ "I don't think so," said Rene Descartes. Just then, he vanished ~~~
ROHM bd71837 and bd71847 contain 32768Hz clock gate. Support the clock using generic clock framework. Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> --- drivers/clk/Kconfig | 7 +++ drivers/clk/Makefile | 1 + drivers/clk/clk-bd718x7.c | 131 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 139 insertions(+) create mode 100644 drivers/clk/clk-bd718x7.c diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig index 81cdb4eaca07..2dc12bf75b1b 100644 --- a/drivers/clk/Kconfig +++ b/drivers/clk/Kconfig @@ -283,6 +283,13 @@ config COMMON_CLK_STM32H7 ---help--- Support for stm32h7 SoC family clocks +config COMMON_CLK_BD718XX + tristate "Clock driver for ROHM BD718x7 PMIC" + depends on MFD_ROHM_BD718XX + help + This driver supports ROHM BD71837 and ROHM BD71847 + PMICs clock gates. + source "drivers/clk/actions/Kconfig" source "drivers/clk/bcm/Kconfig" source "drivers/clk/hisilicon/Kconfig" diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile index 72be7a38cff1..a47430b387db 100644 --- a/drivers/clk/Makefile +++ b/drivers/clk/Makefile @@ -21,6 +21,7 @@ endif obj-$(CONFIG_MACH_ASM9260) += clk-asm9260.o obj-$(CONFIG_COMMON_CLK_AXI_CLKGEN) += clk-axi-clkgen.o obj-$(CONFIG_ARCH_AXXIA) += clk-axm5516.o +obj-$(CONFIG_COMMON_CLK_BD718XX) += clk-bd718x7.o obj-$(CONFIG_COMMON_CLK_CDCE706) += clk-cdce706.o obj-$(CONFIG_COMMON_CLK_CDCE925) += clk-cdce925.o obj-$(CONFIG_ARCH_CLPS711X) += clk-clps711x.o diff --git a/drivers/clk/clk-bd718x7.c b/drivers/clk/clk-bd718x7.c new file mode 100644 index 000000000000..df5f1068ce8e --- /dev/null +++ b/drivers/clk/clk-bd718x7.c @@ -0,0 +1,131 @@ +// SPDX-License-Identifier: GPL-2.0 +// Copyright (C) 2018 ROHM Semiconductors +// bd71837.c -- ROHM BD71837MWV clock driver + +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/init.h> +#include <linux/err.h> +#include <linux/platform_device.h> +#include <linux/slab.h> +#include <linux/mfd/rohm-bd718x7.h> +#include <linux/clk-provider.h> +#include <linux/clkdev.h> +#include <linux/regmap.h> + +struct bd718xx_clk { + struct clk_hw hw; + u8 reg; + u8 mask; + struct platform_device *pdev; + struct bd718xx *mfd; +}; + +static int bd71837_clk_set(struct clk_hw *hw, int status) +{ + struct bd718xx_clk *c = container_of(hw, struct bd718xx_clk, hw); + + return regmap_update_bits(c->mfd->regmap, c->reg, c->mask, status); +} + +static void bd71837_clk_disable(struct clk_hw *hw) +{ + int rv; + struct bd718xx_clk *c = container_of(hw, struct bd718xx_clk, hw); + + rv = bd71837_clk_set(hw, 0); + if (rv) + dev_dbg(&c->pdev->dev, "Failed to disable 32K clk (%d)\n", rv); +} + +static int bd71837_clk_enable(struct clk_hw *hw) +{ + return bd71837_clk_set(hw, 1); +} + +static int bd71837_clk_is_enabled(struct clk_hw *hw) +{ + int enabled; + int rval; + struct bd718xx_clk *c = container_of(hw, struct bd718xx_clk, hw); + + rval = regmap_read(c->mfd->regmap, c->reg, &enabled); + + if (rval) + return rval; + + return enabled & c->mask; +} + +static const struct clk_ops bd71837_clk_ops = { + .prepare = &bd71837_clk_enable, + .unprepare = &bd71837_clk_disable, + .is_prepared = &bd71837_clk_is_enabled, +}; + +static int bd71837_clk_probe(struct platform_device *pdev) +{ + struct bd718xx_clk *c; + int rval = -ENOMEM; + const char *parent_clk; + struct device *parent = pdev->dev.parent; + struct bd718xx *mfd = dev_get_drvdata(parent); + struct clk_init_data init = { + .name = "bd718xx-32k-out", + .ops = &bd71837_clk_ops, + }; + + c = devm_kzalloc(&pdev->dev, sizeof(*c), GFP_KERNEL); + if (!c) + return -ENOMEM; + + init.num_parents = 1; + parent_clk = of_clk_get_parent_name(parent->of_node, 0); + + init.parent_names = &parent_clk; + if (!parent_clk) { + dev_err(&pdev->dev, "No parent clk found\n"); + return -EINVAL; + } + + c->reg = BD718XX_REG_OUT32K; + c->mask = BD718XX_OUT32K_EN; + c->mfd = mfd; + c->pdev = pdev; + c->hw.init = &init; + + of_property_read_string_index(parent->of_node, + "clock-output-names", 0, &init.name); + + rval = devm_clk_hw_register(&pdev->dev, &c->hw); + if (!rval) { + rval = devm_clk_hw_register_clkdev(&pdev->dev, + &c->hw, init.name, NULL); + if (rval) + dev_warn(&pdev->dev, "Failed to register clkdev\n"); + if (parent->of_node) { + rval = devm_of_clk_add_parent_hw_provider(&pdev->dev, + of_clk_hw_simple_get, &c->hw); + if (rval) + dev_err(&pdev->dev, + "adding clk provider failed\n"); + } + } else { + dev_err(&pdev->dev, "failed to register 32K clk"); + } + + return rval; +} + +static struct platform_driver bd71837_clk = { + .driver = { + .name = "bd718xx-clk", + }, + .probe = bd71837_clk_probe, +}; + +module_platform_driver(bd71837_clk); + +MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>"); +MODULE_DESCRIPTION("BD71837 chip clk driver"); +MODULE_LICENSE("GPL"); -- 2.14.3 -- Matti Vaittinen ROHM Semiconductors ~~~ "I don't think so," said Rene Descartes. Just then, he vanished ~~~
Quoting Matti Vaittinen (2018-11-13 03:55:58) > With MFD devices the clk properties may be contained in MFD (parent) DT > node. Current devm_of_clk_add_hw_provider assumes the clk is bound to MFD > subdevice not to MFD device (parent). Add > devm_of_clk_add_hw_provider_parent to tackle this issue. > > Also clkdev registration lacks of managed registration functions and it > seems few drivers do not drop clkdev lookups at exit. Add > devm_clk_hw_register_clkdev and devm_clk_release_clkdev to ease lookup > releasing at exit. Please split this into clkdev and non-clkdev devm functionality. > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > --- > Documentation/driver-model/devres.txt | 3 + > drivers/clk/clk.c | 28 ++++++-- > drivers/clk/clkdev.c | 122 ++++++++++++++++++++++++++-------- > include/linux/clk-provider.h | 11 +++ > include/linux/clkdev.h | 4 ++ > 5 files changed, 136 insertions(+), 32 deletions(-) > > diff --git a/Documentation/driver-model/devres.txt b/Documentation/driver-model/devres.txt > index 43681ca0837f..fac63760b01c 100644 > --- a/Documentation/driver-model/devres.txt > +++ b/Documentation/driver-model/devres.txt > @@ -238,6 +238,9 @@ CLOCK > devm_clk_put() > devm_clk_hw_register() > devm_of_clk_add_hw_provider() > + devm_of_clk_add_parent_hw_provider() > + devm_clk_hw_register_clkdev() > + devm_clk_release_clkdev() The 'release' or non-common functions shouldn't be documented here. > > DMA > dmaenginem_async_device_register() > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index af011974d4ec..9bb921eb90f6 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -3893,12 +3893,12 @@ static void devm_of_clk_release_provider(struct device *dev, void *res) > of_clk_del_provider(*(struct device_node **)res); > } > > -int devm_of_clk_add_hw_provider(struct device *dev, > +static int __devm_of_clk_add_hw_provider(struct device *dev, > struct clk_hw *(*get)(struct of_phandle_args *clkspec, > void *data), > - void *data) > + struct device_node *of_node, void *data) > { > - struct device_node **ptr, *np; > + struct device_node **ptr; > int ret; > > ptr = devres_alloc(devm_of_clk_release_provider, sizeof(*ptr), > @@ -3906,10 +3906,9 @@ int devm_of_clk_add_hw_provider(struct device *dev, > if (!ptr) > return -ENOMEM; > > - np = dev->of_node; > - ret = of_clk_add_hw_provider(np, get, data); > + *ptr = of_node; > + ret = of_clk_add_hw_provider(of_node, get, data); > if (!ret) { > - *ptr = np; Why is this moved outside of the if condition? In fact, why isn't just the first line in this hunk deleted and passed to this function as struct device_node *np? > devres_add(dev, ptr); > } else { > devres_free(ptr); > @@ -3917,8 +3916,25 @@ int devm_of_clk_add_hw_provider(struct device *dev, > > return ret; > } Nitpick: Add a newline here. > +int devm_of_clk_add_hw_provider(struct device *dev, > + struct clk_hw *(*get)(struct of_phandle_args *clkspec, > + void *data), > + void *data) > +{ > + return __devm_of_clk_add_hw_provider(dev, get, dev->of_node, data); > +} > EXPORT_SYMBOL_GPL(devm_of_clk_add_hw_provider); > > +int devm_of_clk_add_parent_hw_provider(struct device *dev, > + struct clk_hw *(*get)(struct of_phandle_args *clkspec, > + void *data), > + void *data) > +{ > + return __devm_of_clk_add_hw_provider(dev, get, dev->parent->of_node, I'm wondering if we can somehow auto-detect this in devm_of_clk_add_hw_provider() by looking for #clock-cells in the node. If it isn't there, then we go to the parent node and look for a #clock-cells property there in the DT node for that device. Does that make sense? Then there isn't any new API and we can attach the lifetime of the devm registration to the presence of the property indicating this is a clk controller or not. > + data); > +} > +EXPORT_SYMBOL_GPL(devm_of_clk_add_parent_hw_provider); Can we get some kernel doc on these functions? > + > /** > * of_clk_del_provider() - Remove a previously registered clock provider > * @np: Device node pointer associated with clock provider > diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c > index 9ab3db8b3988..f6100b6e06fd 100644 > --- a/drivers/clk/clkdev.c > +++ b/drivers/clk/clkdev.c [...] > + > +/** > + * devm_clk_release_clkdev - Resource managed clkdev lookup release > + * @dev: device this lookup is bound > + * @con_id: connection ID string on device > + * @dev_id: format string describing device name > + * > + * Drop the clkdev lookup created with devm_clk_hw_register_clkdev. > + * Normally this function will not need to be called and the resource > + * management code will ensure that the resource is freed. > + */ > +void devm_clk_release_clkdev(struct device *dev, const char *con_id, > + const char *dev_id) > +{ > + struct clk_lookup *cl; > + int rval; > + > + cl = clk_find(dev_id, con_id); > + WARN_ON(!cl); > + rval = devres_release(dev, devm_clkdev_release, > + &devm_clk_match_clkdev, cl); Nitpick: Drop & on functions taken as pointers. > + WARN_ON(rval); > +} > +EXPORT_SYMBOL(devm_clk_release_clkdev);
Hello Stephen, Thanks a bunch for taking the time and reviewing this! On Fri, Nov 30, 2018 at 12:54:10AM -0800, Stephen Boyd wrote: > Quoting Matti Vaittinen (2018-11-13 03:55:58) > > With MFD devices the clk properties may be contained in MFD (parent) DT > > node. Current devm_of_clk_add_hw_provider assumes the clk is bound to MFD > > subdevice not to MFD device (parent). Add > > devm_of_clk_add_hw_provider_parent to tackle this issue. > > > > Also clkdev registration lacks of managed registration functions and it > > seems few drivers do not drop clkdev lookups at exit. Add > > devm_clk_hw_register_clkdev and devm_clk_release_clkdev to ease lookup > > releasing at exit. > > Please split this into clkdev and non-clkdev devm functionality. Allright. I'll split this to two patches. > > --- a/Documentation/driver-model/devres.txt > > +++ b/Documentation/driver-model/devres.txt > > @@ -238,6 +238,9 @@ CLOCK > > devm_clk_put() > > devm_clk_hw_register() > > devm_of_clk_add_hw_provider() > > + devm_of_clk_add_parent_hw_provider() > > + devm_clk_hw_register_clkdev() > > + devm_clk_release_clkdev() > > The 'release' or non-common functions shouldn't be documented here. So I will drop the line mentioning devm_clk_release_clkdev() > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > > index af011974d4ec..9bb921eb90f6 100644 > > --- a/drivers/clk/clk.c > > +++ b/drivers/clk/clk.c > > @@ -3893,12 +3893,12 @@ static void devm_of_clk_release_provider(struct device *dev, void *res) > > of_clk_del_provider(*(struct device_node **)res); > > } > > > > -int devm_of_clk_add_hw_provider(struct device *dev, > > +static int __devm_of_clk_add_hw_provider(struct device *dev, > > struct clk_hw *(*get)(struct of_phandle_args *clkspec, > > void *data), > > - void *data) > > + struct device_node *of_node, void *data) > > { > > - struct device_node **ptr, *np; > > + struct device_node **ptr; > > int ret; > > > > ptr = devres_alloc(devm_of_clk_release_provider, sizeof(*ptr), > > @@ -3906,10 +3906,9 @@ int devm_of_clk_add_hw_provider(struct device *dev, > > if (!ptr) > > return -ENOMEM; > > > > - np = dev->of_node; > > - ret = of_clk_add_hw_provider(np, get, data); > > + *ptr = of_node; > > + ret = of_clk_add_hw_provider(of_node, get, data); > > if (!ret) { > > - *ptr = np; > > Why is this moved outside of the if condition? I completely removed the local variable np and just unconditionally set the allocated devres to point at the node (if allocation succeeded). We could of course only do this if the provider registration succeeded and save one assignment - but I guess I intended to remove the curly braces and thus decided to go for one liner after if. But apparently I didn't remove the braces O_o. Well, I can put the assignment inside the condition if you prefer that. > In fact, why isn't just > the first line in this hunk deleted and passed to this function as > struct device_node *np? I am sorry but I don't quite follow your suggestion here. Do you mean we could just pass the struct device_node *np in devres_add()? I thought the pointer passed to devress_add() should be allocated using devres_alloc. Can you please elaborate what you mean? > > > devres_add(dev, ptr); > > } else { > > devres_free(ptr); > > @@ -3917,8 +3916,25 @@ int devm_of_clk_add_hw_provider(struct device *dev, > > > > return ret; > > } > > Nitpick: Add a newline here. Will do. > > > +int devm_of_clk_add_hw_provider(struct device *dev, > > + struct clk_hw *(*get)(struct of_phandle_args *clkspec, > > + void *data), > > + void *data) > > +{ > > + return __devm_of_clk_add_hw_provider(dev, get, dev->of_node, data); > > +} > > EXPORT_SYMBOL_GPL(devm_of_clk_add_hw_provider); > > > > +int devm_of_clk_add_parent_hw_provider(struct device *dev, > > + struct clk_hw *(*get)(struct of_phandle_args *clkspec, > > + void *data), > > + void *data) > > +{ > > + return __devm_of_clk_add_hw_provider(dev, get, dev->parent->of_node, > > I'm wondering if we can somehow auto-detect this in > devm_of_clk_add_hw_provider() by looking for #clock-cells in the node. > If it isn't there, then we go to the parent node and look for a > #clock-cells property there in the DT node for that device. Does that > make sense? Then there isn't any new API and we can attach the lifetime > of the devm registration to the presence of the property indicating this > is a clk controller or not. Huh. I don't know why but building this kind of logic in core is a bit scary to me. I guess I can try implementing something like this - but I am not really a fan of this. (Accidentally) omit the #clock-cells from node and we go to parent node - I am a novice on this area but this sounds like a potential hazard to me. I believe the driver should know if it's properties should be in own or parent node - and if they are not, then there should be no guessing but error. The lifetime is topic where I would like to get information from you who know the kernel better than I do =) But I guess the parent node is there at least as long as the child device is alive. So for me the life time of get-callback is more crucial - but as I said, I don't understand the kernel in details so you probably know it better than me. But please let me know your final take on this and I will follow the guidance =) > > > + data); > > +} > > +EXPORT_SYMBOL_GPL(devm_of_clk_add_parent_hw_provider); > > Can we get some kernel doc on these functions? Sure. I will add the doc. Reason why I didn't do it is that the current devm_of_clk_add_hw_provider() did not have doc. But I'll add that in next patch. > > + rval = devres_release(dev, devm_clkdev_release, > > + &devm_clk_match_clkdev, cl); > > Nitpick: Drop & on functions taken as pointers. Ok. Will do. -- Matti Vaittinen ROHM Semiconductors ~~~ "I don't think so," said Rene Descartes. Just then, he vanished ~~~
On Fri, Nov 30, 2018 at 12:50:22PM +0200, Matti Vaittinen wrote: > Hello Stephen, > > Thanks a bunch for taking the time and reviewing this! > > On Fri, Nov 30, 2018 at 12:54:10AM -0800, Stephen Boyd wrote: > > Quoting Matti Vaittinen (2018-11-13 03:55:58) > > > With MFD devices the clk properties may be contained in MFD (parent) DT > > > node. Current devm_of_clk_add_hw_provider assumes the clk is bound to MFD > > > subdevice not to MFD device (parent). Add > > > devm_of_clk_add_hw_provider_parent to tackle this issue. > > > > > > Also clkdev registration lacks of managed registration functions and it > > > seems few drivers do not drop clkdev lookups at exit. Add > > > devm_clk_hw_register_clkdev and devm_clk_release_clkdev to ease lookup > > > releasing at exit. > > > > Please split this into clkdev and non-clkdev devm functionality. > Allright. I'll split this to two patches. Definitely, please note that clkdev is separate from the common clk stuff, and is actually maintained by a different person (me) although I currently defer much of the review to the common clk guys. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up
On Fri, Nov 30, 2018 at 11:06:53AM +0000, Russell King - ARM Linux wrote: > On Fri, Nov 30, 2018 at 12:50:22PM +0200, Matti Vaittinen wrote: > > Hello Stephen, > > > > Thanks a bunch for taking the time and reviewing this! > > > > On Fri, Nov 30, 2018 at 12:54:10AM -0800, Stephen Boyd wrote: > > > Quoting Matti Vaittinen (2018-11-13 03:55:58) > > > > With MFD devices the clk properties may be contained in MFD (parent) DT > > > > node. Current devm_of_clk_add_hw_provider assumes the clk is bound to MFD > > > > subdevice not to MFD device (parent). Add > > > > devm_of_clk_add_hw_provider_parent to tackle this issue. > > > > > > > > Also clkdev registration lacks of managed registration functions and it > > > > seems few drivers do not drop clkdev lookups at exit. Add > > > > devm_clk_hw_register_clkdev and devm_clk_release_clkdev to ease lookup > > > > releasing at exit. > > > > > > Please split this into clkdev and non-clkdev devm functionality. > > Allright. I'll split this to two patches. > > Definitely, please note that clkdev is separate from the common > clk stuff, and is actually maintained by a different person (me) > although I currently defer much of the review to the common clk > guys. Right. This clarifies how clkdev is maintained. Thanks for letting me know. Is it still Ok to do these changes through linux-clk tree or is there some other tree where the devm portion should be submitted? If so, how should the dependencies be handled? Br, Matti Vaittinen > > -- > RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ > FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up > According to speedtest.net: 11.9Mbps down 500kbps up -- Matti Vaittinen ROHM Semiconductors ~~~ "I don't think so," said Rene Descartes. Just then, he vanished ~~~
Hello Stephen & All, I created v5 where I fixed obvious issues. I'll send it in few minutes. Please note following topics: On Fri, Nov 30, 2018 at 12:50:22PM +0200, Matti Vaittinen wrote: > > On Fri, Nov 30, 2018 at 12:54:10AM -0800, Stephen Boyd wrote: > > Quoting Matti Vaittinen (2018-11-13 03:55:58) > > > > > > -int devm_of_clk_add_hw_provider(struct device *dev, > > > +static int __devm_of_clk_add_hw_provider(struct device *dev, > > > struct clk_hw *(*get)(struct of_phandle_args *clkspec, > > > void *data), > > > - void *data) > > > + struct device_node *of_node, void *data) > > > { > > > - struct device_node **ptr, *np; > > > + struct device_node **ptr; > > > int ret; > > > > > > ptr = devres_alloc(devm_of_clk_release_provider, sizeof(*ptr), > > > @@ -3906,10 +3906,9 @@ int devm_of_clk_add_hw_provider(struct device *dev, > > > if (!ptr) > > > return -ENOMEM; > > > > > > - np = dev->of_node; > > > - ret = of_clk_add_hw_provider(np, get, data); > > > + *ptr = of_node; > > > + ret = of_clk_add_hw_provider(of_node, get, data); > > > if (!ret) { > > > - *ptr = np; > > > > Why is this moved outside of the if condition? > I completely removed the local variable np and just unconditionally set > the allocated devres to point at the node (if allocation succeeded). We > could of course only do this if the provider registration succeeded and > save one assignment - but I guess I intended to remove the curly braces > and thus decided to go for one liner after if. But apparently I didn't > remove the braces O_o. Well, I can put the assignment inside the > condition if you prefer that. > > > In fact, why isn't just > > the first line in this hunk deleted and passed to this function as > > struct device_node *np? > > I am sorry but I don't quite follow your suggestion here. Do you mean we > could just pass the struct device_node *np in devres_add()? I thought > the pointer passed to devress_add() should be allocated using > devres_alloc. Can you please elaborate what you mean? I could not really spot what to fix in patched code (see below). static int __devm_of_clk_add_hw_provider(struct device *dev, struct clk_hw *(*get)(struct of_phandle_args *clkspec, void *data), struct device_node *of_node, void *data) { struct device_node **ptr; int ret; ptr = devres_alloc(devm_of_clk_release_provider, sizeof(*ptr), GFP_KERNEL); if (!ptr) return -ENOMEM; *ptr = of_node; ret = of_clk_add_hw_provider(of_node, get, data); if (!ret) devres_add(dev, ptr); else devres_free(ptr); return ret; } As far as I understand we need to allocate the ptr using devres_alloc. We also need to pass this ptr to of_clk_add_hw_provider - and we must assign our node to the *ptr. (I removed the extra braces - this change is laso included in v5 but I don't see how we should improve). Can you please explain me if you still wish to me change this further? > > > +int devm_of_clk_add_parent_hw_provider(struct device *dev, > > > + struct clk_hw *(*get)(struct of_phandle_args *clkspec, > > > + void *data), > > > + void *data) > > > +{ > > > + return __devm_of_clk_add_hw_provider(dev, get, dev->parent->of_node, > > > > I'm wondering if we can somehow auto-detect this in > > devm_of_clk_add_hw_provider() by looking for #clock-cells in the node. > > If it isn't there, then we go to the parent node and look for a > > #clock-cells property there in the DT node for that device. Does that > > make sense? Then there isn't any new API and we can attach the lifetime > > of the devm registration to the presence of the property indicating this > > is a clk controller or not. > > Huh. I don't know why but building this kind of logic in core is a bit > scary to me. I guess I can try implementing something like this - but I > am not really a fan of this. (Accidentally) omit the #clock-cells from > node and we go to parent node - I am a novice on this area but this > sounds like a potential hazard to me. I believe the driver should know > if it's properties should be in own or parent node - and if they are > not, then there should be no guessing but error. The lifetime is topic > where I would like to get information from you who know the kernel > better than I do =) But I guess the parent node is there at least as > long as the child device is alive. So for me the life time of > get-callback is more crucial - but as I said, I don't understand the > kernel in details so you probably know it better than me. But please let > me know your final take on this and I will follow the guidance =) I did not put the 'auto-detection' for provider node in the patch v5 as it really gives me bad vibes :) Maybe it is just my pessimistic nature but I do expect that problems will arise when we accidentally end up in parent node when this is not the purpose. I would rather keep this simple by adding one specific API function more - and keeping the existing API specific as well. But I can do v5 if you insist on having this auto-detection. -- Matti Vaittinen ROHM Semiconductors ~~~ "I don't think so," said Rene Descartes. Just then, he vanished ~~~
Quoting Matti Vaittinen (2018-11-30 02:50:22) > Hello Stephen, > > Thanks a bunch for taking the time and reviewing this! > > On Fri, Nov 30, 2018 at 12:54:10AM -0800, Stephen Boyd wrote: > > Quoting Matti Vaittinen (2018-11-13 03:55:58) > > > With MFD devices the clk properties may be contained in MFD (parent) DT > > > node. Current devm_of_clk_add_hw_provider assumes the clk is bound to MFD > > > subdevice not to MFD device (parent). Add > > > devm_of_clk_add_hw_provider_parent to tackle this issue. > > > > > > Also clkdev registration lacks of managed registration functions and it > > > seems few drivers do not drop clkdev lookups at exit. Add > > > devm_clk_hw_register_clkdev and devm_clk_release_clkdev to ease lookup > > > releasing at exit. > > > > Please split this into clkdev and non-clkdev devm functionality. > Allright. I'll split this to two patches. > > > > --- a/Documentation/driver-model/devres.txt > > > +++ b/Documentation/driver-model/devres.txt > > > @@ -238,6 +238,9 @@ CLOCK > > > devm_clk_put() > > > devm_clk_hw_register() > > > devm_of_clk_add_hw_provider() > > > + devm_of_clk_add_parent_hw_provider() > > > + devm_clk_hw_register_clkdev() > > > + devm_clk_release_clkdev() > > > > The 'release' or non-common functions shouldn't be documented here. > So I will drop the line mentioning devm_clk_release_clkdev() > > > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > > > index af011974d4ec..9bb921eb90f6 100644 > > > --- a/drivers/clk/clk.c > > > +++ b/drivers/clk/clk.c > > > @@ -3893,12 +3893,12 @@ static void devm_of_clk_release_provider(struct device *dev, void *res) > > > of_clk_del_provider(*(struct device_node **)res); > > > } > > > > > > -int devm_of_clk_add_hw_provider(struct device *dev, > > > +static int __devm_of_clk_add_hw_provider(struct device *dev, > > > struct clk_hw *(*get)(struct of_phandle_args *clkspec, > > > void *data), > > > - void *data) > > > + struct device_node *of_node, void *data) > > > { > > > - struct device_node **ptr, *np; > > > + struct device_node **ptr; > > > int ret; > > > > > > ptr = devres_alloc(devm_of_clk_release_provider, sizeof(*ptr), > > > @@ -3906,10 +3906,9 @@ int devm_of_clk_add_hw_provider(struct device *dev, > > > if (!ptr) > > > return -ENOMEM; > > > > > > - np = dev->of_node; > > > - ret = of_clk_add_hw_provider(np, get, data); > > > + *ptr = of_node; > > > + ret = of_clk_add_hw_provider(of_node, get, data); > > > if (!ret) { > > > - *ptr = np; > > > > Why is this moved outside of the if condition? > I completely removed the local variable np and just unconditionally set > the allocated devres to point at the node (if allocation succeeded). We > could of course only do this if the provider registration succeeded and > save one assignment - but I guess I intended to remove the curly braces > and thus decided to go for one liner after if. But apparently I didn't > remove the braces O_o. Well, I can put the assignment inside the > condition if you prefer that. > > > In fact, why isn't just > > the first line in this hunk deleted and passed to this function as > > struct device_node *np? > > I am sorry but I don't quite follow your suggestion here. Do you mean we > could just pass the struct device_node *np in devres_add()? I thought > the pointer passed to devress_add() should be allocated using > devres_alloc. Can you please elaborate what you mean? I'm just trying to reduce the diff in the patch. > > > > > > devres_add(dev, ptr); > > > } else { > > > devres_free(ptr); [..] > > > > > +int devm_of_clk_add_hw_provider(struct device *dev, > > > + struct clk_hw *(*get)(struct of_phandle_args *clkspec, > > > + void *data), > > > + void *data) > > > +{ > > > + return __devm_of_clk_add_hw_provider(dev, get, dev->of_node, data); > > > +} > > > EXPORT_SYMBOL_GPL(devm_of_clk_add_hw_provider); > > > > > > +int devm_of_clk_add_parent_hw_provider(struct device *dev, > > > + struct clk_hw *(*get)(struct of_phandle_args *clkspec, > > > + void *data), > > > + void *data) > > > +{ > > > + return __devm_of_clk_add_hw_provider(dev, get, dev->parent->of_node, > > > > I'm wondering if we can somehow auto-detect this in > > devm_of_clk_add_hw_provider() by looking for #clock-cells in the node. > > If it isn't there, then we go to the parent node and look for a > > #clock-cells property there in the DT node for that device. Does that > > make sense? Then there isn't any new API and we can attach the lifetime > > of the devm registration to the presence of the property indicating this > > is a clk controller or not. > > Huh. I don't know why but building this kind of logic in core is a bit > scary to me. I guess I can try implementing something like this - but I > am not really a fan of this. (Accidentally) omit the #clock-cells from > node and we go to parent node - I am a novice on this area but this > sounds like a potential hazard to me. I believe the driver should know > if it's properties should be in own or parent node - and if they are > not, then there should be no guessing but error. The lifetime is topic > where I would like to get information from you who know the kernel > better than I do =) But I guess the parent node is there at least as > long as the child device is alive. So for me the life time of > get-callback is more crucial - but as I said, I don't understand the > kernel in details so you probably know it better than me. But please let > me know your final take on this and I will follow the guidance =) Please do the magic instead of adding another API. It makes things simpler and will work for this case without having to change anything besides of_clk_add_provider(). If the DT doesn't have the #clock-cells property in the node being registered then calling clk_get() will fail for any consumer devices that point to the node with a phandle and clock specifier. I don't expect us to get very far into development if that's the case. Of course, we don't fail in of_clk_add_provider() if there isn't a #clock-cells property in the node, we just happily add the node to the provider list and carry on. I doubt anyone is failing to specify the DT property, but maybe they are, in which case we could keep not failing and just add the node of whatever we're called with originally if neither the parent or the passed node have the #clock-cells property. I wouldn't try to go any higher than one node above the current node and look for a #clock-cells though. If this all still seems scary then don't worry about it, I'll implement it myself.
Hello Again Stephen, I did already send v5 prior to your reply but I will create v6 today based on this discussion. On Mon, Dec 03, 2018 at 03:35:10PM -0800, Stephen Boyd wrote: > Quoting Matti Vaittinen (2018-11-30 02:50:22) > > Hello Stephen, > > > > Thanks a bunch for taking the time and reviewing this! > > > > On Fri, Nov 30, 2018 at 12:54:10AM -0800, Stephen Boyd wrote: > > > Quoting Matti Vaittinen (2018-11-13 03:55:58) > > > > -int devm_of_clk_add_hw_provider(struct device *dev, > > > > +static int __devm_of_clk_add_hw_provider(struct device *dev, > > > > struct clk_hw *(*get)(struct of_phandle_args *clkspec, > > > > void *data), > > > > - void *data) > > > > + struct device_node *of_node, void *data) > > > > { > > > > - struct device_node **ptr, *np; > > > > + struct device_node **ptr; > > > > int ret; > > > > > > > > ptr = devres_alloc(devm_of_clk_release_provider, sizeof(*ptr), > > > > @@ -3906,10 +3906,9 @@ int devm_of_clk_add_hw_provider(struct device *dev, > > > > if (!ptr) > > > > return -ENOMEM; > > > > > > > > - np = dev->of_node; > > > > - ret = of_clk_add_hw_provider(np, get, data); > > > > + *ptr = of_node; > > > > + ret = of_clk_add_hw_provider(of_node, get, data); > > > > if (!ret) { > > > > - *ptr = np; > > > > > > Why is this moved outside of the if condition? > > I completely removed the local variable np and just unconditionally set > > the allocated devres to point at the node (if allocation succeeded). We > > could of course only do this if the provider registration succeeded and > > save one assignment - but I guess I intended to remove the curly braces > > and thus decided to go for one liner after if. But apparently I didn't > > remove the braces O_o. Well, I can put the assignment inside the > > condition if you prefer that. > > > > > In fact, why isn't just > > > the first line in this hunk deleted and passed to this function as > > > struct device_node *np? > > > > I am sorry but I don't quite follow your suggestion here. Do you mean we > > could just pass the struct device_node *np in devres_add()? I thought > > the pointer passed to devress_add() should be allocated using > > devres_alloc. Can you please elaborate what you mean? > > I'm just trying to reduce the diff in the patch. Oh, right. I will see how renaming the argument to np would impact to patch size. iActually, I never consider the patch size at all - I have only been concentrating on how the resulting file looks like. It didn't ever cross my mind that patch size matters. But I guess the size of chanes is really meaningfull when the amount of changes is large. > > > > devres_add(dev, ptr); > > > > } else { > > > > devres_free(ptr); > [..] > > > > > > > +int devm_of_clk_add_hw_provider(struct device *dev, > > > > + struct clk_hw *(*get)(struct of_phandle_args *clkspec, > > > > + void *data), > > > > + void *data) > > > > +{ > > > > + return __devm_of_clk_add_hw_provider(dev, get, dev->of_node, data); > > > > +} > > > > EXPORT_SYMBOL_GPL(devm_of_clk_add_hw_provider); > > > > > > > > +int devm_of_clk_add_parent_hw_provider(struct device *dev, > > > > + struct clk_hw *(*get)(struct of_phandle_args *clkspec, > > > > + void *data), > > > > + void *data) > > > > +{ > > > > + return __devm_of_clk_add_hw_provider(dev, get, dev->parent->of_node, > > > > > > I'm wondering if we can somehow auto-detect this in > > > devm_of_clk_add_hw_provider() by looking for #clock-cells in the node. > > > If it isn't there, then we go to the parent node and look for a > > > #clock-cells property there in the DT node for that device. Does that > > > make sense? Then there isn't any new API and we can attach the lifetime > > > of the devm registration to the presence of the property indicating this > > > is a clk controller or not. > > > > Huh. I don't know why but building this kind of logic in core is a bit > > scary to me. I guess I can try implementing something like this - but I > > am not really a fan of this. (Accidentally) omit the #clock-cells from > > node and we go to parent node - I am a novice on this area but this > > sounds like a potential hazard to me. I believe the driver should know > > if it's properties should be in own or parent node - and if they are > > not, then there should be no guessing but error. The lifetime is topic > > where I would like to get information from you who know the kernel > > better than I do =) But I guess the parent node is there at least as > > long as the child device is alive. So for me the life time of > > get-callback is more crucial - but as I said, I don't understand the > > kernel in details so you probably know it better than me. But please let > > me know your final take on this and I will follow the guidance =) > > Please do the magic instead of adding another API. It makes things > simpler and will work for this case without having to change anything > besides of_clk_add_provider(). All right. Let's go on this direction then. > If the DT doesn't have the #clock-cells property in the node being > registered then calling clk_get() will fail for any consumer devices > that point to the node with a phandle and clock specifier. I don't > expect us to get very far into development if that's the case. Makes sense. So only potential thing to break is if someone out there has broken DT/driver - where they currently see this failure. Eg. they use node w/o #clock-cells as provider and where they try and fail controlling this clock - but ignore the error (and system just "works" with HW defaults). After this change they may actually succeed in controlling - but do control wrong clock. Not likely scenario (sure happens somewhere) - and it involves already broken design. So I agree with you. Besides, you are the maintainer for clk framework and thus get the most of the rain if **** hits the fan =D > Of course, we don't fail in of_clk_add_provider() if there isn't a > #clock-cells property in the node, we just happily add the node to the > provider list and carry on. I doubt anyone is failing to specify the DT > property, but maybe they are, in which case we could keep not failing > and just add the node of whatever we're called with originally if > neither the parent or the passed node have the #clock-cells property. I > wouldn't try to go any higher than one node above the current node and > look for a #clock-cells though. I think we should use parent device's node, not the paren node in DT, right? But I agree, we should only look "one level up in the chain". > > If this all still seems scary then don't worry about it, I'll implement > it myself. It still is somewhat "scary" - but I really would like to use the devm based provider registration in the bd718x7 driver so I will implement it in this series. The engineer version of the "living on the edge", you know =) Br, Matti Vaittinen -- Matti Vaittinen ROHM Semiconductors ~~~ "I don't think so," said Rene Descartes. Just then, he vanished ~~~
Quoting Matti Vaittinen (2018-12-03 23:13:15) > On Mon, Dec 03, 2018 at 03:35:10PM -0800, Stephen Boyd wrote: > > > If the DT doesn't have the #clock-cells property in the node being > > registered then calling clk_get() will fail for any consumer devices > > that point to the node with a phandle and clock specifier. I don't > > expect us to get very far into development if that's the case. > > Makes sense. So only potential thing to break is if someone out there > has broken DT/driver - where they currently see this failure. Eg. they > use node w/o #clock-cells as provider and where they try and fail > controlling this clock - but ignore the error (and system just "works" > with HW defaults). After this change they may actually succeed in > controlling - but do control wrong clock. > > Not likely scenario (sure happens somewhere) - and it involves already > broken design. So I agree with you. Besides, you are the maintainer for > clk framework and thus get the most of the rain if **** hits the fan =D Yes, exactly. > > > Of course, we don't fail in of_clk_add_provider() if there isn't a > > #clock-cells property in the node, we just happily add the node to the > > provider list and carry on. I doubt anyone is failing to specify the DT > > property, but maybe they are, in which case we could keep not failing > > and just add the node of whatever we're called with originally if > > neither the parent or the passed node have the #clock-cells property. I > > wouldn't try to go any higher than one node above the current node and > > look for a #clock-cells though. > > I think we should use parent device's node, not the paren node in DT, > right? But I agree, we should only look "one level up in the chain". Are these two things different? I'm suggesting looking at device_node::parent and trying to find a #clock-cells property.
Hello Stephen, . > > I think we should use parent device's node, not the paren node in DT, > > right? But I agree, we should only look "one level up in the chain". > Are these two things different? I'm suggesting looking at > device_node::parent and trying to find a #clock-cells property. I thought that MFD sub-devices may completely lack the DT node but I will verify this tomorrow. BTW, I already did the patch version 6 which checks the dev->parent->of_node if #clock-cells is not found from device's own node or if own node is NULL. Br. Matti Vaittinen