Re: [PATCH] clk: socfpga: agilex: add clock driver for eASIC N5X platform

[Dinh Nguyen]

On 2/10/21 9:05 PM, Stephen Boyd wrote:
> Quoting Dinh Nguyen (2021-01-05 11:29:56)
> > Add support for Intel's eASIC N5X platform. The clock manager driver for
> > the N5X is very similar to the Agilex platform, we can re-use most of
> > the Agilex clock driver.
> >
> > This patch makes the necessary changes for the driver to differentiate
> > between the Agilex and the N5X platforms.
> >
> > Signed-off-by: Dinh Nguyen <dinguyen@xxxxxxxxxx>
> > ---
>
> Sorry this patch fell off my radar.
>
> I ran checkpatch
>
> WARNING: function definition argument 'struct platform_device *' should also have an identifier name
> #135: FILE: drivers/clk/socfpga/clk-agilex.c:500:
> +       int (*probe_func)(struct platform_device *);
>
> WARNING: Statements terminations use 1 semicolon
> #140: FILE: drivers/clk/socfpga/clk-agilex.c:505:
> +       return  probe_func(pdev);;
>
> WARNING: DT compatible string "intel,n5x-clkmgr" appears un-documented -- check ./Documentation/devicetree/bindings/
> #147: FILE: drivers/clk/socfpga/clk-agilex.c:511:
> +       { .compatible = "intel,n5x-clkmgr",
>
> WARNING: struct clk_ops should normally be const
> #290: FILE: drivers/clk/socfpga/clk-pll-s10.c:166:
> +static struct clk_ops n5x_clk_pll_ops = {
>
> WARNING: struct clk_ops should normally be const
> #296: FILE: drivers/clk/socfpga/clk-pll-s10.c:172:
> +static struct clk_ops agilex_clk_pll_ops = {
>
> WARNING: function definition argument 'const struct stratix10_pll_clock *' should also have an identifier name
> #367: FILE: drivers/clk/socfpga/stratix10-clk.h:78:
> +struct clk *n5x_register_pll(const struct stratix10_pll_clock *,
>
> WARNING: function definition argument 'void __iomem *' should also have an identifier name
> #367: FILE: drivers/clk/socfpga/stratix10-clk.h:78:
> +struct clk *n5x_register_pll(const struct stratix10_pll_clock *,
>
> WARNING: function definition argument 'const struct n5x_perip_c_clock *' should also have an identifier name
> #371: FILE: drivers/clk/socfpga/stratix10-clk.h:82:
> +struct clk *n5x_register_periph(const struct n5x_perip_c_clock *,
>
> WARNING: function definition argument 'void __iomem *' should also have an identifier name
> #371: FILE: drivers/clk/socfpga/stratix10-clk.h:82:
> +struct clk *n5x_register_periph(const struct n5x_perip_c_clock *,
>
> Can you fix these up and resend?
>
> >   drivers/clk/socfpga/clk-agilex.c     | 88 +++++++++++++++++++++++++++-
> >   drivers/clk/socfpga/clk-periph-s10.c | 53 +++++++++++++++++
> >   drivers/clk/socfpga/clk-pll-s10.c    | 85 ++++++++++++++++++++++++++-
> >   drivers/clk/socfpga/stratix10-clk.h  | 15 +++++
> >   4 files changed, 238 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/clk/socfpga/clk-periph-s10.c b/drivers/clk/socfpga/clk-periph-s10.c
> > index 397b77b89b16..135581c41c05 100644
> > --- a/drivers/clk/socfpga/clk-periph-s10.c
> > +++ b/drivers/clk/socfpga/clk-periph-s10.c
> > @@ -15,6 +15,21 @@
> >   
> >   #define to_periph_clk(p) container_of(p, struct socfpga_periph_clk, hw.hw)
> >   
> > +static unsigned long n5x_clk_peri_c_clk_recalc_rate(struct clk_hw *hwclk,
> > +                                            unsigned long parent_rate)
> > +{
> > +       struct socfpga_periph_clk *socfpgaclk = to_periph_clk(hwclk);
> > +       unsigned long div;
> > +       unsigned long shift = socfpgaclk->shift;
> > +       u32 val;
> > +
> > +       val = readl(socfpgaclk->hw.reg);
> > +       val &= (0x1F << shift);
>
> Prefer lowercase hex.
>
> > +       div = (val >> shift) + 1;
> > +
> > +       return parent_rate / div;
> > +}
> > +
> >   static unsigned long clk_peri_c_clk_recalc_rate(struct clk_hw *hwclk,
> >                                               unsigned long parent_rate)
> >   {
> > @@ -63,6 +78,11 @@ static u8 clk_periclk_get_parent(struct clk_hw *hwclk)
> >          return parent;
> >   }
> >   
> > +static const struct clk_ops n5x_peri_c_clk_ops = {
> > +       .recalc_rate = n5x_clk_peri_c_clk_recalc_rate,
> > +       .get_parent = clk_periclk_get_parent,
> > +};
> > +
> >   static const struct clk_ops peri_c_clk_ops = {
> >          .recalc_rate = clk_peri_c_clk_recalc_rate,
> >          .get_parent = clk_periclk_get_parent,
> > @@ -107,6 +127,39 @@ struct clk *s10_register_periph(const struct stratix10_perip_c_clock *clks,
> >          return clk;
> >   }
> >   
> > +struct clk *n5x_register_periph(const struct n5x_perip_c_clock *clks,
> > +                               void __iomem *regbase)
> > +{
> > +       struct clk *clk;
> > +       struct socfpga_periph_clk *periph_clk;
> > +       struct clk_init_data init;
> > +       const char *name = clks->name;
> > +       const char *parent_name = clks->parent_name;
> > +
> > +       periph_clk = kzalloc(sizeof(*periph_clk), GFP_KERNEL);
> > +       if (WARN_ON(!periph_clk))
> > +               return NULL;
> > +
> > +       periph_clk->hw.reg = regbase + clks->offset;
> > +       periph_clk->shift = clks->shift;
> > +
> > +       init.name = name;
> > +       init.ops = &n5x_peri_c_clk_ops;
> > +       init.flags = clks->flags;
> > +
> > +       init.num_parents = clks->num_parents;
> > +       init.parent_names = parent_name ? &parent_name : NULL;
> > +
> > +       periph_clk->hw.hw.init = &init;
> > +
> > +       clk = clk_register(NULL, &periph_clk->hw.hw);
>
> Can you use clk_hw_register?

Thanks for the review. If you don't mind, changing this to use 
clk_hw_register will need a few modifications to the driver, I'd like to 
submit that in a separate patch series after this.

Thanks,
Dinh