RE: [PATCH] clk: clk-fixed-factor: Use new macro CLK_OF_DECLARE_DRIVER

[Rajan Vaja]

Hi Stephen,

Thanks for the review.

> -----Original Message-----
> From: Stephen Boyd [mailto:sboyd@xxxxxxxxxx]
> Sent: Friday, March 09, 2018 10:25 AM
> To: Rajan Vaja <RAJANV@xxxxxxxxxx>; mturquette@xxxxxxxxxxxx
> Cc: linux-clk@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Jolly Shah
> <JOLLYS@xxxxxxxxxx>; Michal Simek <michals@xxxxxxxxxx>; Rajan Vaja
> <RAJANV@xxxxxxxxxx>
> Subject: Re: [PATCH] clk: clk-fixed-factor: Use new macro
> CLK_OF_DECLARE_DRIVER
> 
> Quoting Rajan Vaja (2018-03-08 06:15:00)
> > Fixed factor clock has two initialization at of_clk_init() time and
> > also during platform driver probe. So declare the fixed factor clock
> > with CLK_OF_DECLARE_DRIVER instead of CLK_OF_DECLARE.
> >
> > See below commit for reference:
> > "clk: sunxi: apb0: Use new macro CLK_OF_DECLARE_DRIVER"
> > (sha1: 915128b621a05c63fa58ca9e4cbdf394bbe592f3)
> >
> > Signed-off-by: Rajan Vaja <rajanv@xxxxxxxxxx>
> > Suggested-by: Michal Simek <michal.simek@xxxxxxxxxx>
> > ---
> >  drivers/clk/clk-fixed-factor.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/clk/clk-fixed-factor.c
> > b/drivers/clk/clk-fixed-factor.c index a5d402d..d72ef2d 100644
> > --- a/drivers/clk/clk-fixed-factor.c
> > +++ b/drivers/clk/clk-fixed-factor.c
> > @@ -196,8 +196,9 @@ void __init of_fixed_factor_clk_setup(struct
> > device_node *node)  {
> >         _of_fixed_factor_clk_setup(node);  }
> > -CLK_OF_DECLARE(fixed_factor_clk, "fixed-factor-clock",
> > -               of_fixed_factor_clk_setup);
> > +
> > +CLK_OF_DECLARE_DRIVER(fixed_factor_clk, "fixed-factor-clock",
> > +                     of_fixed_factor_clk_setup);
> >
> 
> Is the intent to register the clk twice? I believe things are working as
> intended without this patch, so maybe you can explain a little more what
> you're trying to fix.
[Rajan] Yes. During of_clk_init() if some DT fixed factor clock has parent which is neither mentioned in output-clock-names of clock controller nor registered as clock provider, of_clk_init() will try to forcefully register in second loop. 

                        if (force || parent_ready(clk_provider->np)) {

                                /* Don't populate platform devices */
                                of_node_set_flag(clk_provider->np,
                                                 OF_POPULATED);

So registration of this DT fixed-factor clock would fail as parent would be NULL as below (called from _of_fixed_factor_clk_setup()):
parent_name = of_clk_get_parent_name(node, 0);

On the other hand, even if registration failed, that node will be marked as OF_POPULATED, so probe of clk-fixed-factor.c will also not be called and that DT fixed-factor clock would never be registered. 

Same thing is discussed at  https://lkml.org/lkml/2017/6/5/681 .