Re: [PATCH v3 02/11] dt-bindings: clock: imx-lpcg: add support to parse clocks from device tree

[Shawn Guo <shawnguo@kernel.org>]

On Mon, Aug 12, 2019 at 02:41:55PM +0000, Aisheng Dong wrote:
> > From: Shawn Guo <shawnguo@kernel.org>
> > Sent: Monday, August 12, 2019 9:01 PM 
> > On Mon, Aug 05, 2019 at 11:27:20AM +0800, Dong Aisheng wrote:
> > > > > +- compatible:                Should be one of:
> > > > > +                       "fsl,imx8qxp-lpcg"
> > > > > +                       "fsl,imx8qm-lpcg" followed by
> > "fsl,imx8qxp-lpcg".
> > > > > +- reg:                       Address and length of the register set.
> > > > > +- #clock-cells:              Should be 1. One LPCG supports multiple
> > clocks.
> > > > > +- clocks:            Input parent clocks phandle array for each clock.
> > > > > +- bit-offset:                An integer array indicating the bit offset
> > for each clock.
> > > >
> > > > I guess that the driver should be able to figure bit offset from
> > > > 'clock-indices' property.
> > > >
> > >
> > > Yes, it can be done in theory.
> > > Then the binding may look like:
> > > sdhc0_lpcg: clock-controller@5b200000 {
> > >         ...
> > >         #clock-cells = <1>;
> > >         clocks = <&sdhc0_clk IMX_SC_PM_CLK_PER>,
> > >                  <&conn_ipg_clk>, <&conn_axi_clk>;
> > >         clock-indices = <0>, <16>, <20>;
> > >         clock-output-names = "sdhc0_lpcg_per_clk",
> > >                              "sdhc0_lpcg_ipg_clk",
> > >                              "sdhc0_lpcg_ahb_clk";
> > >         power-domains = <&pd IMX_SC_R_SDHC_0>; };
> > >
> > > usdhc1: mmc@5b010000 {
> > >         ...
> > >         clocks = <&sdhc0_lpcg 16>,
> > >                  <&sdhc0_lpcg 0>,
> > >                  <&sdhc0_lpcg 20>;
> > >         clock-names = "ipg", "per", "ahb"; };
> > >
> > > However, after trying, i found  one limitation if using clock-indices
> > > that users have to do a secondary search for the indices value from
> > > clock names which is not very friendly.
> > >
> > > Formerly from the clock output names, user can easily get the clock
> > > index as they're in fixed orders as output names, so very easily to
> > > use.
> > > e.g.
> > > clocks = <&sdhc0_lpcg 1>,
> > >          <&sdhc0_lpcg 0>,
> > >          <&sdhc0_lpcg 2>;
> > >
> > > If using clock-indices, users have no way to know it's clock index
> > > from clock output names order unless they do a secondary search from
> > > the clock-indice array accordingly.
> > > For example, for "sdhc0_lpcg_ahb_clk", user can easily know its
> > > reference is <&sdhc0_lpcg 2>.
> > > But if using clock-indice, we need search clock-indices array to find
> > > its reference becomes <&sdhc0_lpcg 20>. So this seems like a drawback
> > > if using clock-indices.
> > 
> > Shouldn't we have constant macro defined for those numbers, so that both
> > 'clock-indices' and 'clocks' of client device can use?
> > 
> 
> I think we can do it.
> Does below one look ok to you?
> #define IMX_LPCG_ CLK_0	0
> #define IMX_LPCG_ CLK_1	4
> #define IMX_LPCG_ CLK_2	8
> #define IMX_LPCG_ CLK_3	12
> #define IMX_LPCG_ CLK_4	16
> #define IMX_LPCG_ CLK_5	20
> #define IMX_LPCG_ CLK_6	24
> #define IMX_LPCG_ CLK_7	28

Looks fine to me, except the space in the middle of macro name, which
compiler will complain anyway :)

Shawn

> 
> The usage will look like:
> <&sdhc0_lpcg IMX_LPCG_CLK_5>