Re: [RFC v2 3/5] mips: ralink: mt7620/76x8 use common clk framework

[Stephen Boyd <sboyd@kernel.org>]

Quoting NOGUCHI Hiroshi (2019-04-04 17:01:27)
> diff --git a/arch/mips/ralink/clk.c b/arch/mips/ralink/clk.c
> index 1b7df115eb60..0fd2f0091ea4 100644
> --- a/arch/mips/ralink/clk.c
> +++ b/arch/mips/ralink/clk.c
> @@ -15,8 +15,15 @@
>  
>  #include <asm/time.h>
>  
> +#if IS_ENABLED(CONFIG_COMMON_CLK)

Please drop this unless you need it, and move the linux/ include above
any asm/ includes.

> +#include <linux/clk-provider.h>
> +#endif
> +
>  #include "common.h"
>  
> +
> +#if !IS_ENABLED(CONFIG_COMMON_CLK)
> +
>  struct clk {
>         struct clk_lookup cl;
>         unsigned long rate;
> @@ -72,6 +79,28 @@ long clk_round_rate(struct clk *clk, unsigned long rate)
>  }
>  EXPORT_SYMBOL_GPL(clk_round_rate);
>  
> +#else  /* CONFIG_COMMON_CLK */
> +
> +struct clk_hw * __init add_sys_clkdev(const char *id, unsigned long rate)
> +{
> +       struct clk_hw *clk_hw;
> +       int err;
> +
> +       clk_hw = clk_hw_register_fixed_rate(NULL, id, NULL, 0, rate);
> +       if (IS_ERR(clk_hw))
> +               panic("failed to allocate %s clock structure", id);

This one panics but the other just continues. Why can't we continue from
here too? Is clkdev actually used by anything on this platform?

> +
> +       err = clk_hw_register_clkdev(clk_hw, NULL, id);
> +       if (err) {
> +               pr_err("unable to register %s clock device\n", id);
> +               clk_hw = ERR_PTR(err);
> +       }
> +
> +       return clk_hw;
> +}
> +
> +#endif /* CONFIG_COMMON_CLK */
> +
>  void __init plat_time_init(void)
>  {
>         struct clk *clk;
> diff --git a/arch/mips/ralink/mt7620.c b/arch/mips/ralink/mt7620.c
> index c1ce6f43642b..74aed8f7e502 100644
> --- a/arch/mips/ralink/mt7620.c
> +++ b/arch/mips/ralink/mt7620.c
> @@ -504,6 +505,12 @@ mt7620_get_sys_rate(unsigned long cpu_rate)
>         return cpu_rate / div;
>  }
>  
> +static struct clk_hw_onecell_data *clk_data;
> +
> +#define RFMT(label)    label ":%lu.%03luMHz "
> +#define RINT(x)                ((x) / 1000000)
> +#define RFRAC(x)       (((x) / 1000) % 1000)

Is this necessary to move?

> +
>  void __init ralink_clk_init(void)
>  {
>         unsigned long xtal_rate;
> @@ -586,6 +593,31 @@ void __init ralink_clk_init(void)
>         }
>  }
>  
> +#undef RFRAC
> +#undef RINT
> +#undef RFMT
> +
> +static int mt7620_pll_probe(struct platform_device *pdev)
> +{
> +       of_clk_add_hw_provider(pdev->dev.of_node,
> +                               of_clk_hw_onecell_get, clk_data);
> +       return 0;

Why not return of_clk_add_hw_provider()?

> +}
> +