diff mbox

[4/7] clk: rockchip: abstract pll get-params and set-params operations

Message ID 1461849075-8310-5-git-send-email-heiko@sntech.de (mailing list archive)
State Changes Requested, archived
Delegated to: Stephen Boyd
Headers show

Commit Message

Heiko Stübner April 28, 2016, 1:11 p.m. UTC 
This moves the pll-specific get_params and set_params functions into a
per-pll struct that gets associated at init time and will help us reign
in some code duplication we're faced with right now.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/clk/rockchip/clk-pll.c | 54 ++++++++++++++++++++++++++++++++----------
 1 file changed, 42 insertions(+), 12 deletions(-)

Comments

Stephen Boyd May 6, 2016, 10:55 p.m. UTC | #1 
On 04/28, Heiko Stuebner wrote:
> This moves the pll-specific get_params and set_params functions into a
> per-pll struct that gets associated at init time and will help us reign
> in some code duplication we're faced with right now.
> 
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
>  drivers/clk/rockchip/clk-pll.c | 54 ++++++++++++++++++++++++++++++++----------
>  1 file changed, 42 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/clk/rockchip/clk-pll.c b/drivers/clk/rockchip/clk-pll.c
> index e56637d..2c30f52 100644
> --- a/drivers/clk/rockchip/clk-pll.c
> +++ b/drivers/clk/rockchip/clk-pll.c
> @@ -30,6 +30,14 @@
>  #define PLL_MODE_NORM		0x1
>  #define PLL_MODE_DEEP		0x2
>  
> +struct rockchip_clk_pll;
> +struct rockchip_pll_data {
> +	void (*get_params)(struct rockchip_clk_pll *pll,
> +			   struct rockchip_pll_rate_table *rate);
> +	int (*set_params)(struct rockchip_clk_pll *pll,
> +			  const struct rockchip_pll_rate_table *rate);
> +};

I'm not a huge fan of function pointer indirection on top of
function pointer indirection (clk_ops). It would be nice if this
was more flat design and different clk_ops existed for different
PLL types that all used the same rockchip_clk_pll structure. But
I don't care too much because I'm not looking at this code all
the time, so if you like this approach then I'm fine.
Heiko Stübner May 8, 2016, 8:24 p.m. UTC | #2 
Am Freitag, 6. Mai 2016, 15:55:43 schrieb Stephen Boyd:
> On 04/28, Heiko Stuebner wrote:
> > This moves the pll-specific get_params and set_params functions into a
> > per-pll struct that gets associated at init time and will help us reign
> > in some code duplication we're faced with right now.
> > 
> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> > ---
> > 
> >  drivers/clk/rockchip/clk-pll.c | 54
> >  ++++++++++++++++++++++++++++++++---------- 1 file changed, 42
> >  insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/clk/rockchip/clk-pll.c
> > b/drivers/clk/rockchip/clk-pll.c index e56637d..2c30f52 100644
> > --- a/drivers/clk/rockchip/clk-pll.c
> > +++ b/drivers/clk/rockchip/clk-pll.c
> > @@ -30,6 +30,14 @@
> > 
> >  #define PLL_MODE_NORM		0x1
> >  #define PLL_MODE_DEEP		0x2
> > 
> > +struct rockchip_clk_pll;
> > +struct rockchip_pll_data {
> > +	void (*get_params)(struct rockchip_clk_pll *pll,
> > +			   struct rockchip_pll_rate_table *rate);
> > +	int (*set_params)(struct rockchip_clk_pll *pll,
> > +			  const struct rockchip_pll_rate_table *rate);
> > +};
> 
> I'm not a huge fan of function pointer indirection on top of
> function pointer indirection (clk_ops). It would be nice if this
> was more flat design and different clk_ops existed for different
> PLL types that all used the same rockchip_clk_pll structure. But
> I don't care too much because I'm not looking at this code all
> the time, so if you like this approach then I'm fine.

I think the "flat design" is what we have right now. The problem is that the 
core code is pretty similar and only the hardware-specific accesses are 
special, thus the later indirection.

As we don't save to much right now with the currently handled PLLs, I guess 
we could simply only do the first 3 patches and revisit the pll-changes later 
once the next pll-type shows up.
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Boyd May 9, 2016, 11:03 p.m. UTC | #3 
On 05/08, Heiko Stuebner wrote:
> 
> I think the "flat design" is what we have right now. The problem is that the 
> core code is pretty similar and only the hardware-specific accesses are 
> special, thus the later indirection.

Right, which is where having some functions that manipulate
register values in a generic way lets us consolidate the common
code into one place while leaving the hardware specific accesses
in the clk ops before or after calling the common functions.
diff mbox

Patch

diff --git a/drivers/clk/rockchip/clk-pll.c b/drivers/clk/rockchip/clk-pll.c
index e56637d..2c30f52 100644
--- a/drivers/clk/rockchip/clk-pll.c
+++ b/drivers/clk/rockchip/clk-pll.c
@@ -30,6 +30,14 @@ 
 #define PLL_MODE_NORM		0x1
 #define PLL_MODE_DEEP		0x2
 
+struct rockchip_clk_pll;
+struct rockchip_pll_data {
+	void (*get_params)(struct rockchip_clk_pll *pll,
+			   struct rockchip_pll_rate_table *rate);
+	int (*set_params)(struct rockchip_clk_pll *pll,
+			  const struct rockchip_pll_rate_table *rate);
+};
+
 struct rockchip_clk_pll {
 	struct clk_hw		hw;
 
@@ -48,6 +56,7 @@  struct rockchip_clk_pll {
 	spinlock_t		*lock;
 
 	struct rockchip_clk_provider *ctx;
+	const struct rockchip_pll_data *data;
 };
 
 #define to_rockchip_clk_pll(_hw) container_of(_hw, struct rockchip_clk_pll, hw)
@@ -164,7 +173,7 @@  static unsigned long rockchip_rk3036_pll_recalc_rate(struct clk_hw *hw,
 	struct rockchip_pll_rate_table cur;
 	u64 rate64 = prate;
 
-	rockchip_rk3036_pll_get_params(pll, &cur);
+	pll->data->get_params(pll, &cur);
 
 	rate64 *= cur.fbdiv;
 	do_div(rate64, cur.refdiv);
@@ -259,7 +268,7 @@  static int rockchip_rk3036_pll_set_rate(struct clk_hw *hw, unsigned long drate,
 		return -EINVAL;
 	}
 
-	return rockchip_rk3036_pll_set_params(pll, rate);
+	return pll->data->set_params(pll, rate);
 }
 
 static int rockchip_rk3036_pll_enable(struct clk_hw *hw)
@@ -306,7 +315,7 @@  static void rockchip_rk3036_pll_init(struct clk_hw *hw)
 	if (!rate)
 		return;
 
-	rockchip_rk3036_pll_get_params(pll, &cur);
+	pll->data->get_params(pll, &cur);
 
 	pr_debug("%s: pll %s@%lu: Hz\n", __func__, __clk_get_name(hw->clk),
 		 drate);
@@ -330,10 +339,15 @@  static void rockchip_rk3036_pll_init(struct clk_hw *hw)
 
 		pr_debug("%s: pll %s: rate params do not match rate table, adjusting\n",
 			 __func__, __clk_get_name(hw->clk));
-		rockchip_rk3036_pll_set_params(pll, rate);
+		pll->data->set_params(pll, rate);
 	}
 }
 
+static const struct rockchip_pll_data rockchip_rk3036_pll_data = {
+	.get_params = rockchip_rk3036_pll_get_params,
+	.set_params = rockchip_rk3036_pll_set_params,
+};
+
 static const struct clk_ops rockchip_rk3036_pll_clk_norate_ops = {
 	.recalc_rate = rockchip_rk3036_pll_recalc_rate,
 	.enable = rockchip_rk3036_pll_enable,
@@ -405,7 +419,7 @@  static unsigned long rockchip_rk3066_pll_recalc_rate(struct clk_hw *hw,
 		return prate;
 	}
 
-	rockchip_rk3066_pll_get_params(pll, &cur);
+	pll->data->get_params(pll, &cur);
 
 	rate64 *= cur.nf;
 	do_div(rate64, cur.nr);
@@ -490,7 +504,7 @@  static int rockchip_rk3066_pll_set_rate(struct clk_hw *hw, unsigned long drate,
 		return -EINVAL;
 	}
 
-	return rockchip_rk3066_pll_set_params(pll, rate);
+	return pll->data->set_params(pll, rate);
 }
 
 static int rockchip_rk3066_pll_enable(struct clk_hw *hw)
@@ -537,7 +551,7 @@  static void rockchip_rk3066_pll_init(struct clk_hw *hw)
 	if (!rate)
 		return;
 
-	rockchip_rk3066_pll_get_params(pll, &cur);
+	pll->data->get_params(pll, &cur);
 
 	pr_debug("%s: pll %s@%lu: nr (%d:%d); no (%d:%d); nf(%d:%d), nb(%d:%d)\n",
 		 __func__, clk_hw_get_name(hw), drate, rate->nr, cur.nr,
@@ -546,10 +560,15 @@  static void rockchip_rk3066_pll_init(struct clk_hw *hw)
 						     || rate->nb != cur.nb) {
 		pr_debug("%s: pll %s: rate params do not match rate table, adjusting\n",
 			 __func__, clk_hw_get_name(hw));
-		rockchip_rk3066_pll_set_params(pll, rate);
+		pll->data->set_params(pll, rate);
 	}
 }
 
+static const struct rockchip_pll_data rockchip_rk3066_pll_data = {
+	.get_params = rockchip_rk3066_pll_get_params,
+	.set_params = rockchip_rk3066_pll_set_params,
+};
+
 static const struct clk_ops rockchip_rk3066_pll_clk_norate_ops = {
 	.recalc_rate = rockchip_rk3066_pll_recalc_rate,
 	.enable = rockchip_rk3066_pll_enable,
@@ -638,7 +657,7 @@  static unsigned long rockchip_rk3399_pll_recalc_rate(struct clk_hw *hw,
 	struct rockchip_pll_rate_table cur;
 	u64 rate64 = prate;
 
-	rockchip_rk3399_pll_get_params(pll, &cur);
+	pll->data->get_params(pll, &cur);
 
 	rate64 *= cur.fbdiv;
 	do_div(rate64, cur.refdiv);
@@ -735,7 +754,7 @@  static int rockchip_rk3399_pll_set_rate(struct clk_hw *hw, unsigned long drate,
 		return -EINVAL;
 	}
 
-	return rockchip_rk3399_pll_set_params(pll, rate);
+	return pll->data->set_params(pll, rate);
 }
 
 static int rockchip_rk3399_pll_enable(struct clk_hw *hw)
@@ -782,7 +801,7 @@  static void rockchip_rk3399_pll_init(struct clk_hw *hw)
 	if (!rate)
 		return;
 
-	rockchip_rk3399_pll_get_params(pll, &cur);
+	pll->data->get_params(pll, &cur);
 
 	pr_debug("%s: pll %s@%lu: Hz\n", __func__, __clk_get_name(hw->clk),
 		 drate);
@@ -806,10 +825,15 @@  static void rockchip_rk3399_pll_init(struct clk_hw *hw)
 
 		pr_debug("%s: pll %s: rate params do not match rate table, adjusting\n",
 			 __func__, __clk_get_name(hw->clk));
-		rockchip_rk3399_pll_set_params(pll, rate);
+		pll->data->set_params(pll, rate);
 	}
 }
 
+static const struct rockchip_pll_data rockchip_rk3399_pll_data = {
+	.get_params = rockchip_rk3399_pll_get_params,
+	.set_params = rockchip_rk3399_pll_set_params,
+};
+
 static const struct clk_ops rockchip_rk3399_pll_clk_norate_ops = {
 	.recalc_rate = rockchip_rk3399_pll_recalc_rate,
 	.enable = rockchip_rk3399_pll_enable,
@@ -916,18 +940,24 @@  struct clk *rockchip_clk_register_pll(struct rockchip_clk_provider *ctx,
 
 	switch (pll_type) {
 	case pll_rk3036:
+		pll->data = &rockchip_rk3036_pll_data;
+
 		if (!pll->rate_table || IS_ERR(ctx->grf))
 			init.ops = &rockchip_rk3036_pll_clk_norate_ops;
 		else
 			init.ops = &rockchip_rk3036_pll_clk_ops;
 		break;
 	case pll_rk3066:
+		pll->data = &rockchip_rk3066_pll_data;
+
 		if (!pll->rate_table || IS_ERR(ctx->grf))
 			init.ops = &rockchip_rk3066_pll_clk_norate_ops;
 		else
 			init.ops = &rockchip_rk3066_pll_clk_ops;
 		break;
 	case pll_rk3399:
+		pll->data = &rockchip_rk3399_pll_data;
+
 		if (!pll->rate_table)
 			init.ops = &rockchip_rk3399_pll_clk_norate_ops;
 		else