[PATCH v11 01/10] clk: Add RISC-V Canaan Kendryte K210 clock driver

[Damien Le Moal]

On Wed, 2021-01-13 at 00:25 -0800, Stephen Boyd wrote:
> Quoting Damien Le Moal (2021-01-11 17:02:38)
> > Add a clock provider driver for the Canaan Kendryte K210 RISC-V SoC.
> > This new driver with the compatible string "canaan,k210-clk" implements
> > support for the full clock structure of the K210 SoC. Since it is
> > required for the correct operation of the SoC, this driver is
> > selected by default for compilation when the SOC_CANAAN option is
> > selected.
> > 
> > With this change, the k210-sysctl driver is turned into a simple
> > platform driver which enables its power bus clock and triggers
> > populating its child nodes. This driver is also automatically selected
> > for compilation with the selection of SOC_CANAAN.
> 
> That's stated twice.
> 
> > The sysctl driver
> > retains the SOC early initialization code, but the implementation now
> > relies on the new function k210_clk_early_init() provided by the new
> > clk-k210 driver.
> > 
> > The clock structure implemented and many of the coding ideas for the
> > driver come from the work by Sean Anderson on the K210 support for the
> > U-Boot project.
> > 
> > Cc: Stephen Boyd <sboyd at kernel.org>
> > Cc: Michael Turquette <mturquette at baylibre.com>
> > Cc: linux-clk at vger.kernel.org
> > Signed-off-by: Damien Le Moal <damien.lemoal at wdc.com>
> > ---
> >  MAINTAINERS                      |    1 +
> >  drivers/clk/Kconfig              |    8 +
> >  drivers/clk/Makefile             |    1 +
> >  drivers/clk/clk-k210.c           | 1005 ++++++++++++++++++++++++++++++
> >  drivers/soc/canaan/Kconfig       |   18 +-
> >  drivers/soc/canaan/Makefile      |    2 +-
> >  drivers/soc/canaan/k210-sysctl.c |  205 ++----
> >  include/soc/canaan/k210-sysctl.h |    2 +
> >  8 files changed, 1064 insertions(+), 178 deletions(-)
> >  create mode 100644 drivers/clk/clk-k210.c

[...]
> > +/**
> > + * struct k210_clk - Driver data
> > + * @regs: system controller registers start address
> > + * @clk_lock: clock setting spinlock
> > + * @plls: SoC PLLs descriptors
> > + * @aclk: ACLK clock
> > + * @clks: All other clocks
> > + * @parents: array of pointers to clocks used as parents for muxed clocks.
> > + * @clk_data: clock specifier translation for all clocks
> > + */
> > +struct k210_clk {
> > +       void __iomem                    *regs;
> > +       spinlock_t                      clk_lock;
> > +       struct k210_pll                 plls[K210_PLL_NUM];
> > +       struct clk_hw                   aclk;
> > +       struct clk_hw                   clks[K210_NUM_CLKS];
> > +       const struct clk_hw             *parents[K210_PARENT_NUM_CLKS];
> > +       struct clk_hw_onecell_data      *clk_data;
> > +};
> > +
> > +static struct k210_clk *kcl;
> 
> Please remove this and get to it from the clk hw structure.

I am struggling with this one: I do not see how to get it from clk_hw struct
since there is no "private" pointer I can use and I do not see how
container_of() can be used with it. Suppressing this variable would also force
passing along the driver data pointer to most functions. That complicates the
code a little. Since there can only be a single instance of this driver, isn't
it simpler to keep this as is ?

I addressed all other comments you sent and will resend a v12 soon. Thanks !


-- 
Damien Le Moal
Western Digital