[PATCH linux-next] clk: sparx5: Review changes

[PATCH linux-next] clk: sparx5: Review changes

[Lars Povlsen]

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>
---
 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

Re: [PATCH linux-next] clk: sparx5: Review changes

[Arnd Bergmann]

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

Re: [PATCH linux-next] clk: sparx5: Review changes

[Arnd Bergmann]

On Mon, Jul 27, 2020 at 9:39 PM Stephen Boyd <sboyd@kernel.org> wrote:
> Quoting Arnd Bergmann (2020-07-27 05:02:56)
> > 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.
> >
>
> Why was the clk driver merged to linux-next outside of the clk tree? Was
> there some sort of dependency?

I merged the entire series of the base platform support along with
a few core drivers. I had asked for the series to be submitted to
soc@kernel.org after all parts had been reviewed, but I missed that
the clk driver was still missing maintainer review (I saw you had
reviewed some patches, but apparently that was just the binding,
not the driver).

I rebased the 'arm/newsoc' branch the other day to fix another mistake,
so if you prefer, I can rebase it again and drop the clk driver or
all the sparx5 patches.

       Arnd

Re: [PATCH linux-next] clk: sparx5: Review changes

[Lars Povlsen]

Stephen Boyd writes:

> Quoting Arnd Bergmann (2020-07-27 13:11:49)
>> On Mon, Jul 27, 2020 at 9:39 PM Stephen Boyd <sboyd@kernel.org> wrote:
>> >
>> > Why was the clk driver merged to linux-next outside of the clk tree? Was
>> > there some sort of dependency?
>>
>> I merged the entire series of the base platform support along with
>> a few core drivers. I had asked for the series to be submitted to
>> soc@kernel.org after all parts had been reviewed, but I missed that
>> the clk driver was still missing maintainer review (I saw you had
>> reviewed some patches, but apparently that was just the binding,
>> not the driver).
>>
>> I rebased the 'arm/newsoc' branch the other day to fix another mistake,
>> so if you prefer, I can rebase it again and drop the clk driver or
>> all the sparx5 patches.
>>
>
> Yes, please just drop the clk driver and I can pick it up for the next
> merge window from the list and all the fixes can be rolled into one
> patch.

Sorry for all the commotion!

With Stephen's comments I assume I don't have to submit anything new,
at least not right now.

Otherwise, please let me know.

Cheers,

-- 
Lars Povlsen,
Microchip

Re: [PATCH linux-next] clk: sparx5: Review changes

[Arnd Bergmann]

On Tue, Jul 28, 2020 at 12:10 AM Stephen Boyd <sboyd@kernel.org> wrote:
> Quoting Arnd Bergmann (2020-07-27 13:11:49)
> > I rebased the 'arm/newsoc' branch the other day to fix another mistake,
> > so if you prefer, I can rebase it again and drop the clk driver or
> > all the sparx5 patches.
> >
>
> Yes, please just drop the clk driver and I can pick it up for the next
> merge window from the list and all the fixes can be rolled into one
> patch.

Done.

     Arnd