[v11,6/9] clk: renesas: r9a06g032: Probe possible children

Commit Message

Miquel Raynal April 21, 2022, 8:51 a.m. UTC

The clock controller device on r9a06g032 takes all the memory range that
is described as being a system controller. This range contains many
different (unrelated?) registers besides the ones belonging to the clock
controller, that can necessitate to be accessed from other peripherals.

For instance, the dmamux registers are there. The dmamux "device" will
be described as a child node of the clock/system controller node, which
means we need the top device driver (the clock controller driver in this
case) to populate its children manually.

Signed-off-by: Miquel Raynal <[e-mail]>
Acked-by: Stephen Boyd <[e-mail]>
---
 drivers/clk/renesas/r9a06g032-clocks.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Geert Uytterhoeven April 25, 2022, 4:18 p.m. UTC | #1

Hi Miquel,

On Thu, Apr 21, 2022 at 10:51 AM Miquel Raynal
<[e-mail]> wrote:
> The clock controller device on r9a06g032 takes all the memory range that
> is described as being a system controller. This range contains many
> different (unrelated?) registers besides the ones belonging to the clock
> controller, that can necessitate to be accessed from other peripherals.
>
> For instance, the dmamux registers are there. The dmamux "device" will
> be described as a child node of the clock/system controller node, which
> means we need the top device driver (the clock controller driver in this
> case) to populate its children manually.
>
> Signed-off-by: Miquel Raynal <[e-mail]>
> Acked-by: Stephen Boyd <[e-mail]>

Thanks for your patch!

> --- a/drivers/clk/renesas/r9a06g032-clocks.c
> +++ b/drivers/clk/renesas/r9a06g032-clocks.c
> @@ -996,7 +997,7 @@ static int __init r9a06g032_clocks_probe(struct platform_device *pdev)
>
>         sysctrl_priv = clocks;
>
> -       return 0;
> +       return of_platform_populate(np, NULL, NULL, dev);

This is a bit dangerous: in the (very unlikely) case that
of_platform_populate() fails, the clock driver will fail to probe,
and all managed cleanup will be done (not everything will be cleant
up, though), while sysctrl_priv will still point to the now-freed
r9a06g032_priv structure.

So I think you just want to ignore the failure from
of_platform_populate(), and return zero anyway.

>  }
>
>  static const struct of_device_id r9a06g032_match[] = {

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Miquel Raynal April 27, 2022, 9:02 a.m. UTC | #2

Hi Geert,

[e-mail] wrote on Mon, 25 Apr 2022 18:18:28 +0200:

> Hi Miquel,
> 
> On Thu, Apr 21, 2022 at 10:51 AM Miquel Raynal
> <[e-mail]> wrote:
> > The clock controller device on r9a06g032 takes all the memory range that
> > is described as being a system controller. This range contains many
> > different (unrelated?) registers besides the ones belonging to the clock
> > controller, that can necessitate to be accessed from other peripherals.
> >
> > For instance, the dmamux registers are there. The dmamux "device" will
> > be described as a child node of the clock/system controller node, which
> > means we need the top device driver (the clock controller driver in this
> > case) to populate its children manually.
> >
> > Signed-off-by: Miquel Raynal <[e-mail]>
> > Acked-by: Stephen Boyd <[e-mail]>  
> 
> Thanks for your patch!
> 
> > --- a/drivers/clk/renesas/r9a06g032-clocks.c
> > +++ b/drivers/clk/renesas/r9a06g032-clocks.c
> > @@ -996,7 +997,7 @@ static int __init r9a06g032_clocks_probe(struct platform_device *pdev)
> >
> >         sysctrl_priv = clocks;
> >
> > -       return 0;
> > +       return of_platform_populate(np, NULL, NULL, dev);  
> 
> This is a bit dangerous: in the (very unlikely) case that
> of_platform_populate() fails, the clock driver will fail to probe,
> and all managed cleanup will be done (not everything will be cleant
> up, though), while sysctrl_priv will still point to the now-freed
> r9a06g032_priv structure.
> 
> So I think you just want to ignore the failure from
> of_platform_populate(), and return zero anyway.

That is a very good point. I've changed the logic to just print an
error message and return 0 anyway.

Thanks,
Miquèl

Patch

diff --git a/drivers/clk/renesas/r9a06g032-clocks.c b/drivers/clk/renesas/r9a06g032-clocks.c
index 052d99059981..1df56d7ab3e1 100644
--- a/drivers/clk/renesas/r9a06g032-clocks.c
+++ b/drivers/clk/renesas/r9a06g032-clocks.c
@@ -16,6 +16,7 @@ 
 #include <linux/math64.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
+#include <linux/of_platform.h>
 #include <linux/platform_device.h>
 #include <linux/pm_clock.h>
 #include <linux/pm_domain.h>
@@ -996,7 +997,7 @@  static int __init r9a06g032_clocks_probe(struct platform_device *pdev)
 
 	sysctrl_priv = clocks;
 
-	return 0;
+	return of_platform_populate(np, NULL, NULL, dev);
 }
 
 static const struct of_device_id r9a06g032_match[] = {