| Message ID | 20220624102719.4166125-1-windhl@126.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Neil Armstrong |
| Headers | show |
| Series | clk/meson: Hold reference returned by of_get_parent() | expand |
Hello, thank you for submitting this patch! On Fri, Jun 24, 2022 at 12:28 PM Liang He <windhl@126.com> wrote: [...] > these bugs are compiled tested in 5.19rc2 with at91_dt_defconfig Please note that you're changing some drivers which are only in the arch/arm/configs/multi_v7_defconfig while others are only in arch/arm64/configs/defconfig I think at91_dt_defconfig will not compile any of the drivers below [...] > diff --git a/drivers/clk/meson/meson8b.c b/drivers/clk/meson/meson8b.c > index 8f3b7a94a667..54188319f349 100644 > --- a/drivers/clk/meson/meson8b.c > +++ b/drivers/clk/meson/meson8b.c > @@ -3796,8 +3796,11 @@ static void __init meson8b_clkc_init_common(struct device_node *np, > struct clk *notifier_clk; > struct regmap *map; > int i, ret; > + struct device_node *tp; This function uses reverse christmas tree sorting, so the longest line goes on top while the shortest should stay on the bottom. Can you please explain what "tp" stands for? Personally I would call this variable parent_np, but maybe "tp" has a similar meaning. Best regards, Martin
At 2022-06-27 06:02:45, "Martin Blumenstingl" <martin.blumenstingl@googlemail.com> wrote: >Hello, > >thank you for submitting this patch! > >On Fri, Jun 24, 2022 at 12:28 PM Liang He <windhl@126.com> wrote: >[...] >> these bugs are compiled tested in 5.19rc2 with at91_dt_defconfig >Please note that you're changing some drivers which are only in the >arch/arm/configs/multi_v7_defconfig while others are only in >arch/arm64/configs/defconfig >I think at91_dt_defconfig will not compile any of the drivers below > Hi, Martin. To simplify the building test, I use the at91_dt_defconfig to build .config for these similar bugs in clk-at91, clk-sprd，etc. Then I use following command to complete different compile tests for clk-meson: make CONFIG_ARCH_MESON=y CONFIG_COMMON_CLK_MESON8B=y CROSS_COMPILE=arm-linux-gnueabi- ARCH=arm ./drivers/clk/meson/meson8b.o make CONFIG_ARCH_MESON=y CONFIG_COMMON_CLK_MESON_EE_CLKC=y CROSS_COMPILE=arm-linux-gnueabi- ARCH=arm ./drivers/clk/meson/meson-eeclk.o make CONFIG_ARCH_MESON=y CONFIG_COMMON_CLK_MESON_AO_CLKC=y CROSS_COMPILE=arm-linux-gnueabi- ARCH=arm ./drivers/clk/meson/meson-aoclk.o I will add these detailed compile commands in my version-2 patch and also in future patch commit. >[...] >> diff --git a/drivers/clk/meson/meson8b.c b/drivers/clk/meson/meson8b.c >> index 8f3b7a94a667..54188319f349 100644 >> --- a/drivers/clk/meson/meson8b.c >> +++ b/drivers/clk/meson/meson8b.c >> @@ -3796,8 +3796,11 @@ static void __init meson8b_clkc_init_common(struct device_node *np, >> struct clk *notifier_clk; >> struct regmap *map; >> int i, ret; >> + struct device_node *tp; >This function uses reverse christmas tree sorting, so the longest line >goes on top while the shortest should stay on the bottom. >Can you please explain what "tp" stands for? Personally I would call >this variable parent_np, but maybe "tp" has a similar meaning. > > >Best regards, >Martin Thanks very much, Martin. I will send version-2 patch to keep the reverse christmas tree. And I will follow this rule in my future patch code. I just want to ues 'tp' as 'tmp_pointer', but your advice is more reasonable and I will change that in version-2 patch. Liang
diff --git a/drivers/clk/meson/meson-aoclk.c b/drivers/clk/meson/meson-aoclk.c index 27cd2c1f3f61..434cd8f9de82 100644 --- a/drivers/clk/meson/meson-aoclk.c +++ b/drivers/clk/meson/meson-aoclk.c @@ -38,6 +38,7 @@ int meson_aoclkc_probe(struct platform_device *pdev) struct meson_aoclk_reset_controller *rstc; struct meson_aoclk_data *data; struct device *dev = &pdev->dev; + struct device_node *np; struct regmap *regmap; int ret, clkid; @@ -49,7 +50,9 @@ int meson_aoclkc_probe(struct platform_device *pdev) if (!rstc) return -ENOMEM; - regmap = syscon_node_to_regmap(of_get_parent(dev->of_node)); + np = of_get_parent(dev->of_node); + regmap = syscon_node_to_regmap(np); + of_node_put(np); if (IS_ERR(regmap)) { dev_err(dev, "failed to get regmap\n"); return PTR_ERR(regmap); diff --git a/drivers/clk/meson/meson-eeclk.c b/drivers/clk/meson/meson-eeclk.c index 8d5a5dab955a..0e5e6b57eb20 100644 --- a/drivers/clk/meson/meson-eeclk.c +++ b/drivers/clk/meson/meson-eeclk.c @@ -18,6 +18,7 @@ int meson_eeclkc_probe(struct platform_device *pdev) { const struct meson_eeclkc_data *data; struct device *dev = &pdev->dev; + struct device_node *np; struct regmap *map; int ret, i; @@ -26,7 +27,9 @@ int meson_eeclkc_probe(struct platform_device *pdev) return -EINVAL; /* Get the hhi system controller node */ - map = syscon_node_to_regmap(of_get_parent(dev->of_node)); + np = of_get_parent(dev->of_node); + map = syscon_node_to_regmap(np); + of_node_put(np); if (IS_ERR(map)) { dev_err(dev, "failed to get HHI regmap\n"); diff --git a/drivers/clk/meson/meson8b.c b/drivers/clk/meson/meson8b.c index 8f3b7a94a667..54188319f349 100644 --- a/drivers/clk/meson/meson8b.c +++ b/drivers/clk/meson/meson8b.c @@ -3796,8 +3796,11 @@ static void __init meson8b_clkc_init_common(struct device_node *np, struct clk *notifier_clk; struct regmap *map; int i, ret; + struct device_node *tp; - map = syscon_node_to_regmap(of_get_parent(np)); + tp = of_get_parent(np); + map = syscon_node_to_regmap(tp); + of_node_put(tp); if (IS_ERR(map)) { pr_err("failed to get HHI regmap - Trying obsolete regs\n"); return;
We should hold the reference returned by of_get_parent() and use it to call of_node_put() for refcount balance. Fixes: 88e2da81241e ("clk: meson: aoclk: refactor common code into dedicated file") Fixes: 6682bd4d443f ("clk: meson: factorise meson64 peripheral clock controller drivers") Fixes: bb6eddd1d28c ("clk: meson: meson8b: use the HHI syscon if available") Signed-off-by: Liang He <windhl@126.com> --- these bugs are found in 5.19rc2 these bugs are confirmed not fixed by lore.kernel.org these bugs are compiled tested in 5.19rc2 with at91_dt_defconfig drivers/clk/meson/meson-aoclk.c | 5 ++++- drivers/clk/meson/meson-eeclk.c | 5 ++++- drivers/clk/meson/meson8b.c | 5 ++++- 3 files changed, 12 insertions(+), 3 deletions(-)