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

[Dinh Nguyen <dinguyen@kernel.org>]

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@kernel.org>
>> ---
> 
> 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