Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200727112236.8199-1-lars.povlsen@microchip.com>
In-Reply-To: <20200727112236.8199-1-lars.povlsen@microchip.com>
From:   Arnd Bergmann <arnd@arndb.de>
Date:   Mon, 27 Jul 2020 14:02:56 +0200
Message-ID: <CAK8P3a3WJTKH7-KGgp7CuhZv_wtrtY1k+1LEL-62_Rj+wqbrGQ@mail.gmail.com>
Subject: Re: [PATCH linux-next] clk: sparx5: Review changes
To:     Lars Povlsen <lars.povlsen@microchip.com>
Cc:     SoC Team <soc@kernel.org>, Stephen Boyd <sboyd@kernel.org>,
        Steen Hegelund <Steen.Hegelund@microchip.com>,
        Microchip Linux Driver Support <UNGLinuxDriver@microchip.com>,
        linux-clk <linux-clk@vger.kernel.org>,
        Linux ARM <linux-arm-kernel@lists.infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Alexandre Belloni <alexandre.belloni@bootlin.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Jul 27, 2020 at 1:22 PM Lars Povlsen <lars.povlsen@microchip.com> wrote:
>
> This address the review comments for the sparx5 clk driver from Stephen
> Boyd <sboyd@kernel.org>:
>
>  - Remove unused include of of_address.h
>  - Remove unused member in s5_hw_clk struct
>  - Changed type (to unsigned long) for freq in s5_pll_conf struct
>  - Use .parent_data instead of looking up parent name
>  - Use devm_of_clk_add_hw_provider
>  - Some minor comsmetics
>
> The patch is intended for linux-next (incremental), as the original
> driver was already merged.
>
> Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com>

Hi Lars, thank you for addressing these!

Generally speaking, you should avoid having patches that list a
number of unrelated things that are done by a single commit.

Splitting this up into six patches is probably a little too much,
but maybe you can find a better balance. What I'd like to see
is to split the purely cosmetic changes from those that might
actually change the behavior, and then for each patch that
changes something non-obvious, explain why this was done.

     Arnd

>  drivers/clk/clk-sparx5.c | 31 +++++++------------------------
>  1 file changed, 7 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/clk/clk-sparx5.c b/drivers/clk/clk-sparx5.c
> index c2e7aa0214ebd..0fad0c1a01862 100644
> --- a/drivers/clk/clk-sparx5.c
> +++ b/drivers/clk/clk-sparx5.c
> @@ -12,7 +12,6 @@
>  #include <linux/clk-provider.h>
>  #include <linux/bitfield.h>
>  #include <linux/of.h>
> -#include <linux/of_address.h>
>  #include <linux/slab.h>
>  #include <linux/platform_device.h>
>  #include <dt-bindings/clock/microchip,sparx5.h>
> @@ -38,7 +37,6 @@ static const char *clk_names[N_CLOCKS] = {
>  struct s5_hw_clk {
>         struct clk_hw hw;
>         void __iomem *reg;
> -       int index;
>  };
>
>  struct s5_clk_data {
> @@ -47,7 +45,7 @@ struct s5_clk_data {
>  };
>
>  struct s5_pll_conf {
> -       int freq;
> +       unsigned long freq;
>         u8 div;
>         bool rot_ena;
>         u8 rot_sel;
> @@ -208,8 +206,9 @@ static unsigned long s5_pll_recalc_rate(struct clk_hw *hw,
>                 conf.rot_sel = FIELD_GET(PLL_ROT_SEL, val);
>
>                 conf.freq = s5_calc_freq(parent_rate, &conf);
> -       } else
> +       } else {
>                 conf.freq = 0;
> +       }
>
>         return conf.freq;
>  }
> @@ -246,14 +245,13 @@ static struct clk_hw *s5_clk_hw_get(struct of_phandle_args *clkspec, void *data)
>  static int s5_clk_probe(struct platform_device *pdev)
>  {
>         struct device *dev = &pdev->dev;
> -       struct device_node *np = dev->of_node;
>         int i, ret;
>         struct s5_clk_data *s5_clk;
> -       const char *parent_name;
> +       struct clk_parent_data pdata = { .index = 0 };
>         struct clk_init_data init = {
>                 .ops = &s5_pll_ops,
> -               .parent_names = &parent_name,
>                 .num_parents = 1,
> +               .parent_data = &pdata,
>         };
>
>         s5_clk = devm_kzalloc(dev, sizeof(*s5_clk), GFP_KERNEL);
> @@ -264,18 +262,11 @@ static int s5_clk_probe(struct platform_device *pdev)
>         if (IS_ERR(s5_clk->base))
>                 return PTR_ERR(s5_clk->base);
>
> -       parent_name = of_clk_get_parent_name(np, 0);
> -       if (!parent_name) {
> -               dev_err(dev, "%pOFn: missing parent clock\n", np);
> -               return -EINVAL;
> -       }
> -
>         for (i = 0; i < N_CLOCKS; i++) {
>                 struct s5_hw_clk *s5_hw = &s5_clk->s5_hw[i];
>
>                 init.name = clk_names[i];
> -               s5_hw->index = i;
> -               s5_hw->reg = s5_clk->base + (i * sizeof(u32));
> +               s5_hw->reg = s5_clk->base + (i * 4);
>                 s5_hw->hw.init = &init;
>                 ret = devm_clk_hw_register(dev, &s5_hw->hw);
>                 if (ret) {
> @@ -285,14 +276,7 @@ static int s5_clk_probe(struct platform_device *pdev)
>                 }
>         }
>
> -       return of_clk_add_hw_provider(np, s5_clk_hw_get, s5_clk);
> -}
> -
> -static int s5_clk_remove(struct platform_device *pdev)
> -{
> -       of_clk_del_provider(pdev->dev.of_node);
> -
> -       return 0;
> +       return devm_of_clk_add_hw_provider(dev, s5_clk_hw_get, s5_clk);
>  }
>
>  static const struct of_device_id s5_clk_dt_ids[] = {
> @@ -303,7 +287,6 @@ MODULE_DEVICE_TABLE(of, s5_clk_dt_ids);
>
>  static struct platform_driver s5_clk_driver = {
>         .probe  = s5_clk_probe,
> -       .remove = s5_clk_remove,
>         .driver = {
>                 .name = "sparx5-clk",
>                 .of_match_table = s5_clk_dt_ids,
> --
> 2.27.0