Re: [PATCH V4 03/11] clk: imx: scu: add two cells binding support

From: Dong Aisheng <dongas86@gmail.com>
To: Stephen Boyd <sboyd@kernel.org>
Cc: Dong Aisheng <aisheng.dong@nxp.com>,
	linux-clk <linux-clk@vger.kernel.org>,
	"moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE" 
	<linux-arm-kernel@lists.infradead.org>,
	Michael Turquette <mturquette@baylibre.com>,
	Shawn Guo <shawnguo@kernel.org>,
	Fabio Estevam <fabio.estevam@nxp.com>,
	dl-linux-imx <linux-imx@nxp.com>,
	Sascha Hauer <kernel@pengutronix.de>
Subject: Re: [PATCH V4 03/11] clk: imx: scu: add two cells binding support
Date: Sun, 17 Nov 2019 20:07:11 +0800	[thread overview]
Message-ID: <CAA+hA=SbcophCfF3xGTe1R1awSrFxDRYqESut7uFQPQFOV86eA@mail.gmail.com> (raw)
In-Reply-To: <20190916184408.8A55720665@mail.kernel.org>

Hi Stephen,

Sorry for the delay due to a horrible busy months. Just a bit relax now.

On Tue, Sep 17, 2019 at 2:44 AM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Dong Aisheng (2019-09-09 03:23:25)
> > Hi Stephen,
> >
> > Thanks for the review.
> >
> > On Sat, Sep 7, 2019 at 5:29 PM Stephen Boyd <sboyd@kernel.org> wrote:
> > >
> > > Quoting Dong Aisheng (2019-08-20 04:13:17)
> > > > diff --git a/drivers/clk/imx/clk-imx8qxp.c b/drivers/clk/imx/clk-imx8qxp.c
> > > > index 5e2903e..1ad3f2a 100644
> > > > --- a/drivers/clk/imx/clk-imx8qxp.c
> > > > +++ b/drivers/clk/imx/clk-imx8qxp.c
> > > > @@ -134,7 +134,12 @@ static int imx8qxp_clk_probe(struct platform_device *pdev)
> > > >                                 i, PTR_ERR(clks[i]));
> > > >         }
> > > >
> > > > -       return of_clk_add_hw_provider(ccm_node, of_clk_hw_onecell_get, clk_data);
> > > > +       if (clock_cells == 2)
> > >
> > > Can you just read this from the DT node again instead of having a global
> > > variable called "clock_cells" for this?
> > >
> >
> > I tried thinking about it.
> > One problem is that we also need this information in the exist clk
> > registration API to
> > keep the backwards compatibility:
> > e.g.
> >  static inline struct clk_hw *imx_clk_scu(const char *name, u32 rsrc_id,
> >                                          u8 clk_type)
> >  {
> > -       return __imx_clk_scu(name, NULL, 0, rsrc_id, clk_type);
> > +       if (clock_cells == 2)
> > +               return imx_clk_scu_alloc_dev(name, NULL, 0, rsrc_id, clk_type);
> > +       else
> > +               return __imx_clk_scu(name, NULL, 0, rsrc_id, clk_type);
> >  }
> >
> > Parsing it for all clocks seems not good.
>
> Can you parse it once for the clock controller and then pass it to the
> registration function as the number of cells? I dislike the global and
> the name of the global.
>

Yes, i can do it.
Why i didn't do it before is because there're tens of APIs callers already
and finally we will back to the original API again after removing the
legacy users.
So i used a global variable as a temporarily workaround during transition phase.
But i do agree that make the code look ugly.

Regards
Aisheng

> >
> > In the future, i planned to totally remove the legacy binding support which
> > is a premature one and missing continued support.
> > Then we will also remove this unneeded clock_cells.
>
> Ok sure.
>