Linux-Clk Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH linux-next] clk: sparx5: Review changes
@ 2020-07-27 11:22 Lars Povlsen
  2020-07-27 12:02 ` Arnd Bergmann
  0 siblings, 1 reply; 5+ messages in thread
From: Lars Povlsen @ 2020-07-27 11:22 UTC (permalink / raw)
  To: SoC Team, Arnd Bergmann, Stephen Boyd
  Cc: Lars Povlsen, Steen Hegelund, Microchip Linux Driver Support,
	linux-clk, linux-arm-kernel, linux-kernel, Alexandre Belloni

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH linux-next] clk: sparx5: Review changes
  2020-07-27 11:22 [PATCH linux-next] clk: sparx5: Review changes Lars Povlsen
@ 2020-07-27 12:02 ` Arnd Bergmann
       [not found]   ` <159587873833.1360974.11729154337431621644@swboyd.mtv.corp.google.com>
  0 siblings, 1 reply; 5+ messages in thread
From: Arnd Bergmann @ 2020-07-27 12:02 UTC (permalink / raw)
  To: Lars Povlsen
  Cc: SoC Team, Stephen Boyd, Steen Hegelund,
	Microchip Linux Driver Support, linux-clk, Linux ARM,
	linux-kernel, Alexandre Belloni

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH linux-next] clk: sparx5: Review changes
       [not found]   ` <159587873833.1360974.11729154337431621644@swboyd.mtv.corp.google.com>
@ 2020-07-27 20:11     ` Arnd Bergmann
       [not found]       ` <159588781925.1360974.3928941757935200801@swboyd.mtv.corp.google.com>
  0 siblings, 1 reply; 5+ messages in thread
From: Arnd Bergmann @ 2020-07-27 20:11 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Lars Povlsen, SoC Team, Steen Hegelund,
	Microchip Linux Driver Support, linux-clk, Linux ARM,
	linux-kernel, Alexandre Belloni

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH linux-next] clk: sparx5: Review changes
       [not found]       ` <159588781925.1360974.3928941757935200801@swboyd.mtv.corp.google.com>
@ 2020-07-28  7:53         ` Lars Povlsen
  2020-07-28  9:23         ` Arnd Bergmann
  1 sibling, 0 replies; 5+ messages in thread
From: Lars Povlsen @ 2020-07-28  7:53 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Arnd Bergmann, Lars Povlsen, SoC Team, Steen Hegelund,
	Microchip Linux Driver Support, linux-clk, Linux ARM,
	linux-kernel, Alexandre Belloni


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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH linux-next] clk: sparx5: Review changes
       [not found]       ` <159588781925.1360974.3928941757935200801@swboyd.mtv.corp.google.com>
  2020-07-28  7:53         ` Lars Povlsen
@ 2020-07-28  9:23         ` Arnd Bergmann
  1 sibling, 0 replies; 5+ messages in thread
From: Arnd Bergmann @ 2020-07-28  9:23 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Lars Povlsen, SoC Team, Steen Hegelund,
	Microchip Linux Driver Support, linux-clk, Linux ARM,
	linux-kernel, Alexandre Belloni

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-27 11:22 [PATCH linux-next] clk: sparx5: Review changes Lars Povlsen
2020-07-27 12:02 ` Arnd Bergmann
     [not found]   ` <159587873833.1360974.11729154337431621644@swboyd.mtv.corp.google.com>
2020-07-27 20:11     ` Arnd Bergmann
     [not found]       ` <159588781925.1360974.3928941757935200801@swboyd.mtv.corp.google.com>
2020-07-28  7:53         ` Lars Povlsen
2020-07-28  9:23         ` Arnd Bergmann

Linux-Clk Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-clk/0 linux-clk/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-clk linux-clk/ https://lore.kernel.org/linux-clk \
		linux-clk@vger.kernel.org
	public-inbox-index linux-clk

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-clk


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git