| Message ID | 20220628135230.166601-1-windhl@126.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [v2] clk: sprd: Hold reference returned by of_get_parent() | expand |
Hi Liang, On Tue, Jun 28, 2022 at 9:54 PM Liang He <windhl@126.com> wrote: > > We should hold the reference returned by of_get_parent() and use it > to call of_node_out() for refcount balance. typo. s/out/put > > Fixes: f95e8c7923d1 ("clk: sprd: support to get regmap from parent node") > > Signed-off-by: Liang He <windhl@126.com> > --- > changelog: > > v2: minimize the effective range of of_get_parent() advised by Orson > v1: hold reference returned by of_get_parent() > > v1-link: https://lore.kernel.org/all/20220624103809.4167753-1-windhl@126.com/ > > Patched file has been compiled test in 5.19rc2. > > drivers/clk/sprd/common.c | 37 +++++++++++++++++++++---------------- > 1 file changed, 21 insertions(+), 16 deletions(-) > > diff --git a/drivers/clk/sprd/common.c b/drivers/clk/sprd/common.c > index d620bbbcdfc8..d85ba80c5931 100644 > --- a/drivers/clk/sprd/common.c > +++ b/drivers/clk/sprd/common.c > @@ -50,23 +50,28 @@ int sprd_clk_regmap_init(struct platform_device *pdev, > pr_err("%s: failed to get syscon regmap\n", __func__); > return PTR_ERR(regmap); > } > - } else if (of_device_is_compatible(of_get_parent(dev->of_node), > - "syscon")) { > - regmap = device_node_to_regmap(of_get_parent(dev->of_node)); > - if (IS_ERR(regmap)) { > - dev_err(dev, "failed to get regmap from its parent.\n"); > - return PTR_ERR(regmap); > - } > } else { > - base = devm_platform_ioremap_resource(pdev, 0); > - if (IS_ERR(base)) > - return PTR_ERR(base); > - > - regmap = devm_regmap_init_mmio(&pdev->dev, base, > - &sprdclk_regmap_config); > - if (IS_ERR(regmap)) { > - pr_err("failed to init regmap\n"); > - return PTR_ERR(regmap); > + struct device_node *np = of_get_parent(dev->of_node); move the declaration of "np" to the beginning part without assigning any value. > + > + if (of_device_is_compatible(np, "syscon")) { There may be no need to split the origin structure of "if...else if...else". How about the following method? else if (of_device_is_compatible(np = of_get_parent(dev->of_node), "syscon") || (of_node_put(np), 0)) { > + regmap = device_node_to_regmap(np); > + of_node_put(np); > + if (IS_ERR(regmap)) { > + dev_err(dev, "failed to get regmap from its parent.\n"); > + return PTR_ERR(regmap); > + } > + } else { > + of_node_put(np); This line would not be necessary then. -Orson > + base = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(base)) > + return PTR_ERR(base); > + > + regmap = devm_regmap_init_mmio(&pdev->dev, base, > + &sprdclk_regmap_config); > + if (IS_ERR(regmap)) { > + pr_err("failed to init regmap\n"); > + return PTR_ERR(regmap); > + } > } > } > > -- > 2.25.1 >
At 2022-07-01 08:15:23, "Orson Zhai" <orsonzhai@gmail.com> wrote: >Hi Liang, > >On Tue, Jun 28, 2022 at 9:54 PM Liang He <windhl@126.com> wrote: >> >> We should hold the reference returned by of_get_parent() and use it >> to call of_node_out() for refcount balance. > >typo. s/out/put > Thanks, I will correct it in next version. >> >> Fixes: f95e8c7923d1 ("clk: sprd: support to get regmap from parent node") >> >> Signed-off-by: Liang He <windhl@126.com> >> --- >> changelog: >> >> v2: minimize the effective range of of_get_parent() advised by Orson >> v1: hold reference returned by of_get_parent() >> >> v1-link: https://lore.kernel.org/all/20220624103809.4167753-1-windhl@126.com/ >> >> Patched file has been compiled test in 5.19rc2. >> >> drivers/clk/sprd/common.c | 37 +++++++++++++++++++++---------------- >> 1 file changed, 21 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/clk/sprd/common.c b/drivers/clk/sprd/common.c >> index d620bbbcdfc8..d85ba80c5931 100644 >> --- a/drivers/clk/sprd/common.c >> +++ b/drivers/clk/sprd/common.c >> @@ -50,23 +50,28 @@ int sprd_clk_regmap_init(struct platform_device *pdev, >> pr_err("%s: failed to get syscon regmap\n", __func__); >> return PTR_ERR(regmap); >> } >> - } else if (of_device_is_compatible(of_get_parent(dev->of_node), >> - "syscon")) { >> - regmap = device_node_to_regmap(of_get_parent(dev->of_node)); >> - if (IS_ERR(regmap)) { >> - dev_err(dev, "failed to get regmap from its parent.\n"); >> - return PTR_ERR(regmap); >> - } >> } else { >> - base = devm_platform_ioremap_resource(pdev, 0); >> - if (IS_ERR(base)) >> - return PTR_ERR(base); >> - >> - regmap = devm_regmap_init_mmio(&pdev->dev, base, >> - &sprdclk_regmap_config); >> - if (IS_ERR(regmap)) { >> - pr_err("failed to init regmap\n"); >> - return PTR_ERR(regmap); >> + struct device_node *np = of_get_parent(dev->of_node); > >move the declaration of "np" to the beginning part without assigning any value. > OK. >> + >> + if (of_device_is_compatible(np, "syscon")) { > >There may be no need to split the origin structure of "if...else if...else". >How about the following method? > > else if (of_device_is_compatible(np = >of_get_parent(dev->of_node), "syscon") > || (of_node_put(np), 0)) { > Thanks, I will try it and compile test it before I send next version. >> + regmap = device_node_to_regmap(np); >> + of_node_put(np); >> + if (IS_ERR(regmap)) { >> + dev_err(dev, "failed to get regmap from its parent.\n"); >> + return PTR_ERR(regmap); >> + } >> + } else { >> + of_node_put(np); > >This line would not be necessary then. > >-Orson Thanks, I will remove this line. Liang > >> + base = devm_platform_ioremap_resource(pdev, 0); >> + if (IS_ERR(base)) >> + return PTR_ERR(base); >> + >> + regmap = devm_regmap_init_mmio(&pdev->dev, base, >> + &sprdclk_regmap_config); >> + if (IS_ERR(regmap)) { >> + pr_err("failed to init regmap\n"); >> + return PTR_ERR(regmap); >> + } >> } >> } >> >> -- >> 2.25.1 >>
diff --git a/drivers/clk/sprd/common.c b/drivers/clk/sprd/common.c index d620bbbcdfc8..d85ba80c5931 100644 --- a/drivers/clk/sprd/common.c +++ b/drivers/clk/sprd/common.c @@ -50,23 +50,28 @@ int sprd_clk_regmap_init(struct platform_device *pdev, pr_err("%s: failed to get syscon regmap\n", __func__); return PTR_ERR(regmap); } - } else if (of_device_is_compatible(of_get_parent(dev->of_node), - "syscon")) { - regmap = device_node_to_regmap(of_get_parent(dev->of_node)); - if (IS_ERR(regmap)) { - dev_err(dev, "failed to get regmap from its parent.\n"); - return PTR_ERR(regmap); - } } else { - base = devm_platform_ioremap_resource(pdev, 0); - if (IS_ERR(base)) - return PTR_ERR(base); - - regmap = devm_regmap_init_mmio(&pdev->dev, base, - &sprdclk_regmap_config); - if (IS_ERR(regmap)) { - pr_err("failed to init regmap\n"); - return PTR_ERR(regmap); + struct device_node *np = of_get_parent(dev->of_node); + + if (of_device_is_compatible(np, "syscon")) { + regmap = device_node_to_regmap(np); + of_node_put(np); + if (IS_ERR(regmap)) { + dev_err(dev, "failed to get regmap from its parent.\n"); + return PTR_ERR(regmap); + } + } else { + of_node_put(np); + base = devm_platform_ioremap_resource(pdev, 0); + if (IS_ERR(base)) + return PTR_ERR(base); + + regmap = devm_regmap_init_mmio(&pdev->dev, base, + &sprdclk_regmap_config); + if (IS_ERR(regmap)) { + pr_err("failed to init regmap\n"); + return PTR_ERR(regmap); + } } }
We should hold the reference returned by of_get_parent() and use it to call of_node_out() for refcount balance. Fixes: f95e8c7923d1 ("clk: sprd: support to get regmap from parent node") Signed-off-by: Liang He <windhl@126.com> --- changelog: v2: minimize the effective range of of_get_parent() advised by Orson v1: hold reference returned by of_get_parent() v1-link: https://lore.kernel.org/all/20220624103809.4167753-1-windhl@126.com/ Patched file has been compiled test in 5.19rc2. drivers/clk/sprd/common.c | 37 +++++++++++++++++++++---------------- 1 file changed, 21 insertions(+), 16 deletions(-)