[PATCH 0/7] clk: rockchip: cleanup some code duplication

[PATCH 0/7] clk: rockchip: cleanup some code duplication

[Heiko Stuebner]

This tries to generalize code paths that contain some (nearly)
duplicate code. While it's only a bit noticeable now for 3 different
plls, the effect should be more pronounced when the next pll-type
shows up.


Heiko Stuebner (7):
  clk: rockchip: lookup General Register Files in rockchip_clk_init
  clk: rockchip: simplify GRF handling in pll clocks
  clk: rockchip: drop old_rate calculation on pll rate changes
  clk: rockchip: abstract pll get-params and set-params operations
  clk: rockchip: generalize pll set-rate operation
  clk: rockchip: move pll rate-comparison into a callable function
  clk: rockchip: fold pll init functions into one common one

 drivers/clk/rockchip/clk-pll.c | 329 ++++++++++++++++-------------------------
 drivers/clk/rockchip/clk.c     |  10 +-
 drivers/clk/rockchip/clk.h     |   1 -
 3 files changed, 131 insertions(+), 209 deletions(-)

-- 
2.6.4

[PATCH 1/7] clk: rockchip: lookup General Register Files in rockchip_clk_init

[Heiko Stuebner]

In the distant past syscons were initialized pretty late and weren't
available at the time the clock init ran. As the GRF is mainly needed
for PLL lock-status checking, we had this lazy init that tried to grab
the syscon on PLL rate changes and denied these changes if it was not
available.

These days syscons are available very early and recent addition to
rockchip clocks, like the PLL clk_init actually also rely on them
being available at that time, so there is no need to keep that lazy
init around, as it will also result in some more simplifications in
other parts of the clock-code.

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

diff --git a/drivers/clk/rockchip/clk.c b/drivers/clk/rockchip/clk.c
index 7345be4..af6d47b 100644
--- a/drivers/clk/rockchip/clk.c
+++ b/drivers/clk/rockchip/clk.c
@@ -347,6 +347,9 @@ struct rockchip_clk_provider * __init rockchip_clk_init(struct device_node *np,
 	ctx->grf = ERR_PTR(-EPROBE_DEFER);
 	spin_lock_init(&ctx->lock);
 
+	ctx->grf = syscon_regmap_lookup_by_phandle(ctx->cru_node,
+						   "rockchip,grf");
+
 	return ctx;
 
 err_free:
@@ -364,9 +367,6 @@ void __init rockchip_clk_of_add_provider(struct device_node *np,
 
 struct regmap *rockchip_clk_get_grf(struct rockchip_clk_provider *ctx)
 {
-	if (IS_ERR(ctx->grf))
-		ctx->grf = syscon_regmap_lookup_by_phandle(ctx->cru_node,
-							   "rockchip,grf");
 	return ctx->grf;
 }
 
-- 
2.6.4

[PATCH 2/7] clk: rockchip: simplify GRF handling in pll clocks

[Heiko Stuebner]

With the previous commit, the clock drivers now know at init time if the
GRF regmap is available. That means if it isn't available then, it also
won't become available later and we can therefore switch PLLs, that need
the GRF for the lock-status, to read-only mode - similar behaviour as the
aborting of rate changes we did before.

This saves some conditionals on every rate change and we can also drop
the rockchip_clk_get_grf function completely.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/clk/rockchip/clk-pll.c | 30 +++---------------------------
 drivers/clk/rockchip/clk.c     |  5 -----
 drivers/clk/rockchip/clk.h     |  1 -
 3 files changed, 3 insertions(+), 33 deletions(-)

diff --git a/drivers/clk/rockchip/clk-pll.c b/drivers/clk/rockchip/clk-pll.c
index 128a6b2..5c0b919 100644
--- a/drivers/clk/rockchip/clk-pll.c
+++ b/drivers/clk/rockchip/clk-pll.c
@@ -92,15 +92,10 @@ static long rockchip_pll_round_rate(struct clk_hw *hw,
  */
 static int rockchip_pll_wait_lock(struct rockchip_clk_pll *pll)
 {
-	struct regmap *grf = rockchip_clk_get_grf(pll->ctx);
+	struct regmap *grf = pll->ctx->grf;
 	unsigned int val;
 	int delay = 24000000, ret;
 
-	if (IS_ERR(grf)) {
-		pr_err("%s: grf regmap not available\n", __func__);
-		return PTR_ERR(grf);
-	}
-
 	while (delay > 0) {
 		ret = regmap_read(grf, pll->lock_offset, &val);
 		if (ret) {
@@ -253,13 +248,6 @@ static int rockchip_rk3036_pll_set_rate(struct clk_hw *hw, unsigned long drate,
 	struct rockchip_clk_pll *pll = to_rockchip_clk_pll(hw);
 	const struct rockchip_pll_rate_table *rate;
 	unsigned long old_rate = rockchip_rk3036_pll_recalc_rate(hw, prate);
-	struct regmap *grf = rockchip_clk_get_grf(pll->ctx);
-
-	if (IS_ERR(grf)) {
-		pr_debug("%s: grf regmap not available, aborting rate change\n",
-			 __func__);
-		return PTR_ERR(grf);
-	}
 
 	pr_debug("%s: changing %s from %lu to %lu with a parent rate of %lu\n",
 		 __func__, __clk_get_name(hw->clk), old_rate, drate, prate);
@@ -492,13 +480,6 @@ static int rockchip_rk3066_pll_set_rate(struct clk_hw *hw, unsigned long drate,
 	struct rockchip_clk_pll *pll = to_rockchip_clk_pll(hw);
 	const struct rockchip_pll_rate_table *rate;
 	unsigned long old_rate = rockchip_rk3066_pll_recalc_rate(hw, prate);
-	struct regmap *grf = rockchip_clk_get_grf(pll->ctx);
-
-	if (IS_ERR(grf)) {
-		pr_debug("%s: grf regmap not available, aborting rate change\n",
-			 __func__);
-		return PTR_ERR(grf);
-	}
 
 	pr_debug("%s: changing %s from %lu to %lu with a parent rate of %lu\n",
 		 __func__, clk_hw_get_name(hw), old_rate, drate, prate);
@@ -565,11 +546,6 @@ static void rockchip_rk3066_pll_init(struct clk_hw *hw)
 		 rate->no, cur.no, rate->nf, cur.nf, rate->nb, cur.nb);
 	if (rate->nr != cur.nr || rate->no != cur.no || rate->nf != cur.nf
 						     || rate->nb != cur.nb) {
-		struct regmap *grf = rockchip_clk_get_grf(pll->ctx);
-
-		if (IS_ERR(grf))
-			return;
-
 		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);
@@ -943,13 +919,13 @@ struct clk *rockchip_clk_register_pll(struct rockchip_clk_provider *ctx,
 
 	switch (pll_type) {
 	case pll_rk3036:
-		if (!pll->rate_table)
+		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:
-		if (!pll->rate_table)
+		if (!pll->rate_table || IS_ERR(ctx->grf))
 			init.ops = &rockchip_rk3066_pll_clk_norate_ops;
 		else
 			init.ops = &rockchip_rk3066_pll_clk_ops;
diff --git a/drivers/clk/rockchip/clk.c b/drivers/clk/rockchip/clk.c
index af6d47b..3a2936d 100644
--- a/drivers/clk/rockchip/clk.c
+++ b/drivers/clk/rockchip/clk.c
@@ -365,11 +365,6 @@ void __init rockchip_clk_of_add_provider(struct device_node *np,
 		pr_err("%s: could not register clk provider\n", __func__);
 }
 
-struct regmap *rockchip_clk_get_grf(struct rockchip_clk_provider *ctx)
-{
-	return ctx->grf;
-}
-
 void rockchip_clk_add_lookup(struct rockchip_clk_provider *ctx,
 			     struct clk *clk, unsigned int id)
 {
diff --git a/drivers/clk/rockchip/clk.h b/drivers/clk/rockchip/clk.h
index cb6a639..5468910 100644
--- a/drivers/clk/rockchip/clk.h
+++ b/drivers/clk/rockchip/clk.h
@@ -581,7 +581,6 @@ struct rockchip_clk_provider *rockchip_clk_init(struct device_node *np,
 			void __iomem *base, unsigned long nr_clks);
 void rockchip_clk_of_add_provider(struct device_node *np,
 				struct rockchip_clk_provider *ctx);
-struct regmap *rockchip_clk_get_grf(struct rockchip_clk_provider *ctx);
 void rockchip_clk_add_lookup(struct rockchip_clk_provider *ctx,
 			     struct clk *clk, unsigned int id);
 void rockchip_clk_register_branches(struct rockchip_clk_provider *ctx,
-- 
2.6.4

[PATCH 3/7] clk: rockchip: drop old_rate calculation on pll rate changes

[Heiko Stuebner]

Previously when everything happened in the set_rate callbacks itself we
needed the old_rate value for the possible rate rollback, so that made
it easy to also use it in the debug output.

Now with the param-handling being done in separate functions, reading and
recalculating the current pll rate only to use it in a debug message that
won't get displayed in regular cases anyway is quite a waste.

Therefore drop that value from the debug output. In the worst case that
previous rate will have been displayed on the rate change before.

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

diff --git a/drivers/clk/rockchip/clk-pll.c b/drivers/clk/rockchip/clk-pll.c
index 5c0b919..e56637d 100644
--- a/drivers/clk/rockchip/clk-pll.c
+++ b/drivers/clk/rockchip/clk-pll.c
@@ -247,10 +247,9 @@ static int rockchip_rk3036_pll_set_rate(struct clk_hw *hw, unsigned long drate,
 {
 	struct rockchip_clk_pll *pll = to_rockchip_clk_pll(hw);
 	const struct rockchip_pll_rate_table *rate;
-	unsigned long old_rate = rockchip_rk3036_pll_recalc_rate(hw, prate);
 
-	pr_debug("%s: changing %s from %lu to %lu with a parent rate of %lu\n",
-		 __func__, __clk_get_name(hw->clk), old_rate, drate, prate);
+	pr_debug("%s: changing %s to %lu with a parent rate of %lu\n",
+		 __func__, __clk_get_name(hw->clk), drate, prate);
 
 	/* Get required rate settings from table */
 	rate = rockchip_get_pll_settings(pll, drate);
@@ -479,10 +478,9 @@ static int rockchip_rk3066_pll_set_rate(struct clk_hw *hw, unsigned long drate,
 {
 	struct rockchip_clk_pll *pll = to_rockchip_clk_pll(hw);
 	const struct rockchip_pll_rate_table *rate;
-	unsigned long old_rate = rockchip_rk3066_pll_recalc_rate(hw, prate);
 
-	pr_debug("%s: changing %s from %lu to %lu with a parent rate of %lu\n",
-		 __func__, clk_hw_get_name(hw), old_rate, drate, prate);
+	pr_debug("%s: changing %s to %lu with a parent rate of %lu\n",
+		 __func__, clk_hw_get_name(hw), drate, prate);
 
 	/* Get required rate settings from table */
 	rate = rockchip_get_pll_settings(pll, drate);
@@ -725,10 +723,9 @@ static int rockchip_rk3399_pll_set_rate(struct clk_hw *hw, unsigned long drate,
 {
 	struct rockchip_clk_pll *pll = to_rockchip_clk_pll(hw);
 	const struct rockchip_pll_rate_table *rate;
-	unsigned long old_rate = rockchip_rk3399_pll_recalc_rate(hw, prate);
 
-	pr_debug("%s: changing %s from %lu to %lu with a parent rate of %lu\n",
-		 __func__, __clk_get_name(hw->clk), old_rate, drate, prate);
+	pr_debug("%s: changing %s to %lu with a parent rate of %lu\n",
+		 __func__, __clk_get_name(hw->clk), drate, prate);
 
 	/* Get required rate settings from table */
 	rate = rockchip_get_pll_settings(pll, drate);
-- 
2.6.4

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

[Heiko Stuebner]

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);
+};
+
 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
-- 
2.6.4

[PATCH 5/7] clk: rockchip: generalize pll set-rate operation

[Heiko Stuebner]

The currently pll-specific set_rate operations are now identical,
and with the actual register access already being abstracted away
will also stay that way.

Therefore merge them into one function used by each PLL.

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

diff --git a/drivers/clk/rockchip/clk-pll.c b/drivers/clk/rockchip/clk-pll.c
index 2c30f52..081713c 100644
--- a/drivers/clk/rockchip/clk-pll.c
+++ b/drivers/clk/rockchip/clk-pll.c
@@ -94,6 +94,26 @@ static long rockchip_pll_round_rate(struct clk_hw *hw,
 	return rate_table[i - 1].rate;
 }
 
+static int rockchip_pll_set_rate(struct clk_hw *hw, unsigned long drate,
+				 unsigned long prate)
+{
+	struct rockchip_clk_pll *pll = to_rockchip_clk_pll(hw);
+	const struct rockchip_pll_rate_table *rate;
+
+	pr_debug("%s: changing %s to %lu with a parent rate of %lu\n",
+		 __func__, clk_hw_get_name(hw), drate, prate);
+
+	/* Get required rate settings from table */
+	rate = rockchip_get_pll_settings(pll, drate);
+	if (!rate) {
+		pr_err("%s: Invalid rate : %lu for pll clk %s\n", __func__,
+			drate, clk_hw_get_name(hw));
+		return -EINVAL;
+	}
+
+	return pll->data->set_params(pll, rate);
+}
+
 /*
  * Wait for the pll to reach the locked state.
  * The calling set_rate function is responsible for making sure the
@@ -251,26 +271,6 @@ static int rockchip_rk3036_pll_set_params(struct rockchip_clk_pll *pll,
 	return ret;
 }
 
-static int rockchip_rk3036_pll_set_rate(struct clk_hw *hw, unsigned long drate,
-					unsigned long prate)
-{
-	struct rockchip_clk_pll *pll = to_rockchip_clk_pll(hw);
-	const struct rockchip_pll_rate_table *rate;
-
-	pr_debug("%s: changing %s to %lu with a parent rate of %lu\n",
-		 __func__, __clk_get_name(hw->clk), drate, prate);
-
-	/* Get required rate settings from table */
-	rate = rockchip_get_pll_settings(pll, drate);
-	if (!rate) {
-		pr_err("%s: Invalid rate : %lu for pll clk %s\n", __func__,
-			drate, __clk_get_name(hw->clk));
-		return -EINVAL;
-	}
-
-	return pll->data->set_params(pll, rate);
-}
-
 static int rockchip_rk3036_pll_enable(struct clk_hw *hw)
 {
 	struct rockchip_clk_pll *pll = to_rockchip_clk_pll(hw);
@@ -358,7 +358,7 @@ static const struct clk_ops rockchip_rk3036_pll_clk_norate_ops = {
 static const struct clk_ops rockchip_rk3036_pll_clk_ops = {
 	.recalc_rate = rockchip_rk3036_pll_recalc_rate,
 	.round_rate = rockchip_pll_round_rate,
-	.set_rate = rockchip_rk3036_pll_set_rate,
+	.set_rate = rockchip_pll_set_rate,
 	.enable = rockchip_rk3036_pll_enable,
 	.disable = rockchip_rk3036_pll_disable,
 	.is_enabled = rockchip_rk3036_pll_is_enabled,
@@ -487,26 +487,6 @@ static int rockchip_rk3066_pll_set_params(struct rockchip_clk_pll *pll,
 	return ret;
 }
 
-static int rockchip_rk3066_pll_set_rate(struct clk_hw *hw, unsigned long drate,
-					unsigned long prate)
-{
-	struct rockchip_clk_pll *pll = to_rockchip_clk_pll(hw);
-	const struct rockchip_pll_rate_table *rate;
-
-	pr_debug("%s: changing %s to %lu with a parent rate of %lu\n",
-		 __func__, clk_hw_get_name(hw), drate, prate);
-
-	/* Get required rate settings from table */
-	rate = rockchip_get_pll_settings(pll, drate);
-	if (!rate) {
-		pr_err("%s: Invalid rate : %lu for pll clk %s\n", __func__,
-			drate, clk_hw_get_name(hw));
-		return -EINVAL;
-	}
-
-	return pll->data->set_params(pll, rate);
-}
-
 static int rockchip_rk3066_pll_enable(struct clk_hw *hw)
 {
 	struct rockchip_clk_pll *pll = to_rockchip_clk_pll(hw);
@@ -579,7 +559,7 @@ static const struct clk_ops rockchip_rk3066_pll_clk_norate_ops = {
 static const struct clk_ops rockchip_rk3066_pll_clk_ops = {
 	.recalc_rate = rockchip_rk3066_pll_recalc_rate,
 	.round_rate = rockchip_pll_round_rate,
-	.set_rate = rockchip_rk3066_pll_set_rate,
+	.set_rate = rockchip_pll_set_rate,
 	.enable = rockchip_rk3066_pll_enable,
 	.disable = rockchip_rk3066_pll_disable,
 	.is_enabled = rockchip_rk3066_pll_is_enabled,
@@ -737,26 +717,6 @@ static int rockchip_rk3399_pll_set_params(struct rockchip_clk_pll *pll,
 	return ret;
 }
 
-static int rockchip_rk3399_pll_set_rate(struct clk_hw *hw, unsigned long drate,
-					unsigned long prate)
-{
-	struct rockchip_clk_pll *pll = to_rockchip_clk_pll(hw);
-	const struct rockchip_pll_rate_table *rate;
-
-	pr_debug("%s: changing %s to %lu with a parent rate of %lu\n",
-		 __func__, __clk_get_name(hw->clk), drate, prate);
-
-	/* Get required rate settings from table */
-	rate = rockchip_get_pll_settings(pll, drate);
-	if (!rate) {
-		pr_err("%s: Invalid rate : %lu for pll clk %s\n", __func__,
-			drate, __clk_get_name(hw->clk));
-		return -EINVAL;
-	}
-
-	return pll->data->set_params(pll, rate);
-}
-
 static int rockchip_rk3399_pll_enable(struct clk_hw *hw)
 {
 	struct rockchip_clk_pll *pll = to_rockchip_clk_pll(hw);
@@ -844,7 +804,7 @@ static const struct clk_ops rockchip_rk3399_pll_clk_norate_ops = {
 static const struct clk_ops rockchip_rk3399_pll_clk_ops = {
 	.recalc_rate = rockchip_rk3399_pll_recalc_rate,
 	.round_rate = rockchip_pll_round_rate,
-	.set_rate = rockchip_rk3399_pll_set_rate,
+	.set_rate = rockchip_pll_set_rate,
 	.enable = rockchip_rk3399_pll_enable,
 	.disable = rockchip_rk3399_pll_disable,
 	.is_enabled = rockchip_rk3399_pll_is_enabled,
-- 
2.6.4

[PATCH 6/7] clk: rockchip: move pll rate-comparison into a callable function

[Heiko Stuebner]

The init callback in our pll clk_ops allows us to re-set the same rate
of a pll with different parameters if these became updated with more
suitable values over time.

The init mechanism is similar over all pll types with the exception of
the comparison of old and new parameters. So move these out to callbable
functions to allow us to fold the generic init-handling into one.

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

diff --git a/drivers/clk/rockchip/clk-pll.c b/drivers/clk/rockchip/clk-pll.c
index 081713c..5bd0545 100644
--- a/drivers/clk/rockchip/clk-pll.c
+++ b/drivers/clk/rockchip/clk-pll.c
@@ -36,6 +36,9 @@ struct rockchip_pll_data {
 			   struct rockchip_pll_rate_table *rate);
 	int (*set_params)(struct rockchip_clk_pll *pll,
 			  const struct rockchip_pll_rate_table *rate);
+	int (*compare_params)(struct rockchip_clk_pll *pll,
+			const struct rockchip_pll_rate_table *rate,
+			const struct rockchip_pll_rate_table *cur);
 };
 
 struct rockchip_clk_pll {
@@ -271,6 +274,25 @@ static int rockchip_rk3036_pll_set_params(struct rockchip_clk_pll *pll,
 	return ret;
 }
 
+static int rockchip_rk3036_pll_compare_params(struct rockchip_clk_pll *pll,
+				const struct rockchip_pll_rate_table *rate,
+				const struct rockchip_pll_rate_table *cur)
+{
+	const struct clk_hw *hw = &pll->hw;
+
+	pr_debug("%s: pll %s\n", __func__, clk_hw_get_name(hw));
+	pr_debug("old - fbdiv: %d, postdiv1: %d, refdiv: %d, postdiv2: %d, dsmpd: %d, frac: %d\n",
+		 cur->fbdiv, cur->postdiv1, cur->refdiv, cur->postdiv2,
+		 cur->dsmpd, cur->frac);
+	pr_debug("new - fbdiv: %d, postdiv1: %d, refdiv: %d, postdiv2: %d, dsmpd: %d, frac: %d\n",
+		 rate->fbdiv, rate->postdiv1, rate->refdiv, rate->postdiv2,
+		 rate->dsmpd, rate->frac);
+
+	return (rate->fbdiv == cur->fbdiv && rate->postdiv1 == cur->postdiv1 &&
+		rate->refdiv == cur->refdiv && rate->postdiv2 == cur->postdiv2 &&
+		rate->dsmpd == cur->dsmpd && rate->frac == cur->frac);
+}
+
 static int rockchip_rk3036_pll_enable(struct clk_hw *hw)
 {
 	struct rockchip_clk_pll *pll = to_rockchip_clk_pll(hw);
@@ -316,19 +338,7 @@ static void rockchip_rk3036_pll_init(struct clk_hw *hw)
 		return;
 
 	pll->data->get_params(pll, &cur);
-
-	pr_debug("%s: pll %s@%lu: Hz\n", __func__, __clk_get_name(hw->clk),
-		 drate);
-	pr_debug("old - fbdiv: %d, postdiv1: %d, refdiv: %d, postdiv2: %d, dsmpd: %d, frac: %d\n",
-		 cur.fbdiv, cur.postdiv1, cur.refdiv, cur.postdiv2,
-		 cur.dsmpd, cur.frac);
-	pr_debug("new - fbdiv: %d, postdiv1: %d, refdiv: %d, postdiv2: %d, dsmpd: %d, frac: %d\n",
-		 rate->fbdiv, rate->postdiv1, rate->refdiv, rate->postdiv2,
-		 rate->dsmpd, rate->frac);
-
-	if (rate->fbdiv != cur.fbdiv || rate->postdiv1 != cur.postdiv1 ||
-		rate->refdiv != cur.refdiv || rate->postdiv2 != cur.postdiv2 ||
-		rate->dsmpd != cur.dsmpd || rate->frac != cur.frac) {
+	if (!pll->data->compare_params(pll, rate, &cur)) {
 		struct clk *parent = clk_get_parent(hw->clk);
 
 		if (!parent) {
@@ -346,6 +356,7 @@ static void rockchip_rk3036_pll_init(struct clk_hw *hw)
 static const struct rockchip_pll_data rockchip_rk3036_pll_data = {
 	.get_params = rockchip_rk3036_pll_get_params,
 	.set_params = rockchip_rk3036_pll_set_params,
+	.compare_params = rockchip_rk3036_pll_compare_params,
 };
 
 static const struct clk_ops rockchip_rk3036_pll_clk_norate_ops = {
@@ -487,6 +498,20 @@ static int rockchip_rk3066_pll_set_params(struct rockchip_clk_pll *pll,
 	return ret;
 }
 
+static int rockchip_rk3066_pll_compare_params(struct rockchip_clk_pll *pll,
+				const struct rockchip_pll_rate_table *rate,
+				const struct rockchip_pll_rate_table *cur)
+{
+	const struct clk_hw *hw = &pll->hw;
+
+	pr_debug("%s: pll %s: nr (%d:%d); no (%d:%d); nf(%d:%d), nb(%d:%d)\n",
+		 __func__, clk_hw_get_name(hw), rate->nr, cur->nr,
+		 rate->no, cur->no, rate->nf, cur->nf, rate->nb, cur->nb);
+
+	return (rate->nr == cur->nr && rate->no == cur->no &&
+				 rate->nf == cur->nf && rate->nb == cur->nb);
+}
+
 static int rockchip_rk3066_pll_enable(struct clk_hw *hw)
 {
 	struct rockchip_clk_pll *pll = to_rockchip_clk_pll(hw);
@@ -533,11 +558,7 @@ static void rockchip_rk3066_pll_init(struct clk_hw *hw)
 
 	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,
-		 rate->no, cur.no, rate->nf, cur.nf, rate->nb, cur.nb);
-	if (rate->nr != cur.nr || rate->no != cur.no || rate->nf != cur.nf
-						     || rate->nb != cur.nb) {
+	if (!pll->data->compare_params(pll, rate, &cur)) {
 		pr_debug("%s: pll %s: rate params do not match rate table, adjusting\n",
 			 __func__, clk_hw_get_name(hw));
 		pll->data->set_params(pll, rate);
@@ -547,6 +568,7 @@ static void rockchip_rk3066_pll_init(struct clk_hw *hw)
 static const struct rockchip_pll_data rockchip_rk3066_pll_data = {
 	.get_params = rockchip_rk3066_pll_get_params,
 	.set_params = rockchip_rk3066_pll_set_params,
+	.compare_params = rockchip_rk3066_pll_compare_params,
 };
 
 static const struct clk_ops rockchip_rk3066_pll_clk_norate_ops = {
@@ -717,6 +739,25 @@ static int rockchip_rk3399_pll_set_params(struct rockchip_clk_pll *pll,
 	return ret;
 }
 
+static int rockchip_rk3399_pll_compare_params(struct rockchip_clk_pll *pll,
+				const struct rockchip_pll_rate_table *rate,
+				const struct rockchip_pll_rate_table *cur)
+{
+	const struct clk_hw *hw = &pll->hw;
+
+	pr_debug("%s: pll %s: Hz\n", __func__, __clk_get_name(hw->clk));
+	pr_debug("old - fbdiv: %d, postdiv1: %d, refdiv: %d, postdiv2: %d, dsmpd: %d, frac: %d\n",
+		 cur->fbdiv, cur->postdiv1, cur->refdiv, cur->postdiv2,
+		 cur->dsmpd, cur->frac);
+	pr_debug("new - fbdiv: %d, postdiv1: %d, refdiv: %d, postdiv2: %d, dsmpd: %d, frac: %d\n",
+		 rate->fbdiv, rate->postdiv1, rate->refdiv, rate->postdiv2,
+		 rate->dsmpd, rate->frac);
+
+	return (rate->fbdiv == cur->fbdiv && rate->postdiv1 == cur->postdiv1 &&
+		rate->refdiv == cur->refdiv && rate->postdiv2 == cur->postdiv2 &&
+		rate->dsmpd == cur->dsmpd && rate->frac == cur->frac);
+}
+
 static int rockchip_rk3399_pll_enable(struct clk_hw *hw)
 {
 	struct rockchip_clk_pll *pll = to_rockchip_clk_pll(hw);
@@ -763,18 +804,7 @@ static void rockchip_rk3399_pll_init(struct clk_hw *hw)
 
 	pll->data->get_params(pll, &cur);
 
-	pr_debug("%s: pll %s@%lu: Hz\n", __func__, __clk_get_name(hw->clk),
-		 drate);
-	pr_debug("old - fbdiv: %d, postdiv1: %d, refdiv: %d, postdiv2: %d, dsmpd: %d, frac: %d\n",
-		 cur.fbdiv, cur.postdiv1, cur.refdiv, cur.postdiv2,
-		 cur.dsmpd, cur.frac);
-	pr_debug("new - fbdiv: %d, postdiv1: %d, refdiv: %d, postdiv2: %d, dsmpd: %d, frac: %d\n",
-		 rate->fbdiv, rate->postdiv1, rate->refdiv, rate->postdiv2,
-		 rate->dsmpd, rate->frac);
-
-	if (rate->fbdiv != cur.fbdiv || rate->postdiv1 != cur.postdiv1 ||
-		rate->refdiv != cur.refdiv || rate->postdiv2 != cur.postdiv2 ||
-		rate->dsmpd != cur.dsmpd || rate->frac != cur.frac) {
+	if (!pll->data->compare_params(pll, rate, &cur)) {
 		struct clk *parent = clk_get_parent(hw->clk);
 
 		if (!parent) {
@@ -792,6 +822,7 @@ static void rockchip_rk3399_pll_init(struct clk_hw *hw)
 static const struct rockchip_pll_data rockchip_rk3399_pll_data = {
 	.get_params = rockchip_rk3399_pll_get_params,
 	.set_params = rockchip_rk3399_pll_set_params,
+	.compare_params = rockchip_rk3399_pll_compare_params,
 };
 
 static const struct clk_ops rockchip_rk3399_pll_clk_norate_ops = {
-- 
2.6.4

[PATCH 7/7] clk: rockchip: fold pll init functions into a common one

[Heiko Stuebner]

The pll init functions are now all the same, so we can generalize them
into one common function used by all to save some duplication.

The parent-check in the original functions does not do anything useful
at all, as we're not doing any real rate-handling at this point anymore,
so can just go away.

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

diff --git a/drivers/clk/rockchip/clk-pll.c b/drivers/clk/rockchip/clk-pll.c
index 5bd0545..1223eee 100644
--- a/drivers/clk/rockchip/clk-pll.c
+++ b/drivers/clk/rockchip/clk-pll.c
@@ -117,6 +117,32 @@ static int rockchip_pll_set_rate(struct clk_hw *hw, unsigned long drate,
 	return pll->data->set_params(pll, rate);
 }
 
+static void rockchip_pll_init(struct clk_hw *hw)
+{
+	struct rockchip_clk_pll *pll = to_rockchip_clk_pll(hw);
+	const struct rockchip_pll_rate_table *rate;
+	struct rockchip_pll_rate_table cur;
+	unsigned long drate;
+
+	if (!(pll->flags & ROCKCHIP_PLL_SYNC_RATE))
+		return;
+
+	drate = clk_hw_get_rate(hw);
+	rate = rockchip_get_pll_settings(pll, drate);
+
+	/* when no rate setting for the current rate, rely on clk_set_rate */
+	if (!rate)
+		return;
+
+	pll->data->get_params(pll, &cur);
+
+	if (!pll->data->compare_params(pll, rate, &cur)) {
+		pr_debug("%s: pll %s: rate params do not match rate table, adjusting\n",
+			 __func__, clk_hw_get_name(hw));
+		pll->data->set_params(pll, rate);
+	}
+}
+
 /*
  * Wait for the pll to reach the locked state.
  * The calling set_rate function is responsible for making sure the
@@ -320,39 +346,6 @@ static int rockchip_rk3036_pll_is_enabled(struct clk_hw *hw)
 	return !(pllcon & RK3036_PLLCON1_PWRDOWN);
 }
 
-static void rockchip_rk3036_pll_init(struct clk_hw *hw)
-{
-	struct rockchip_clk_pll *pll = to_rockchip_clk_pll(hw);
-	const struct rockchip_pll_rate_table *rate;
-	struct rockchip_pll_rate_table cur;
-	unsigned long drate;
-
-	if (!(pll->flags & ROCKCHIP_PLL_SYNC_RATE))
-		return;
-
-	drate = clk_hw_get_rate(hw);
-	rate = rockchip_get_pll_settings(pll, drate);
-
-	/* when no rate setting for the current rate, rely on clk_set_rate */
-	if (!rate)
-		return;
-
-	pll->data->get_params(pll, &cur);
-	if (!pll->data->compare_params(pll, rate, &cur)) {
-		struct clk *parent = clk_get_parent(hw->clk);
-
-		if (!parent) {
-			pr_warn("%s: parent of %s not available\n",
-				__func__, __clk_get_name(hw->clk));
-			return;
-		}
-
-		pr_debug("%s: pll %s: rate params do not match rate table, adjusting\n",
-			 __func__, __clk_get_name(hw->clk));
-		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,
@@ -373,7 +366,7 @@ static const struct clk_ops rockchip_rk3036_pll_clk_ops = {
 	.enable = rockchip_rk3036_pll_enable,
 	.disable = rockchip_rk3036_pll_disable,
 	.is_enabled = rockchip_rk3036_pll_is_enabled,
-	.init = rockchip_rk3036_pll_init,
+	.init = rockchip_pll_init,
 };
 
 /**
@@ -539,32 +532,6 @@ static int rockchip_rk3066_pll_is_enabled(struct clk_hw *hw)
 	return !(pllcon & RK3066_PLLCON3_PWRDOWN);
 }
 
-static void rockchip_rk3066_pll_init(struct clk_hw *hw)
-{
-	struct rockchip_clk_pll *pll = to_rockchip_clk_pll(hw);
-	const struct rockchip_pll_rate_table *rate;
-	struct rockchip_pll_rate_table cur;
-	unsigned long drate;
-
-	if (!(pll->flags & ROCKCHIP_PLL_SYNC_RATE))
-		return;
-
-	drate = clk_hw_get_rate(hw);
-	rate = rockchip_get_pll_settings(pll, drate);
-
-	/* when no rate setting for the current rate, rely on clk_set_rate */
-	if (!rate)
-		return;
-
-	pll->data->get_params(pll, &cur);
-
-	if (!pll->data->compare_params(pll, rate, &cur)) {
-		pr_debug("%s: pll %s: rate params do not match rate table, adjusting\n",
-			 __func__, clk_hw_get_name(hw));
-		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,
@@ -585,7 +552,7 @@ static const struct clk_ops rockchip_rk3066_pll_clk_ops = {
 	.enable = rockchip_rk3066_pll_enable,
 	.disable = rockchip_rk3066_pll_disable,
 	.is_enabled = rockchip_rk3066_pll_is_enabled,
-	.init = rockchip_rk3066_pll_init,
+	.init = rockchip_pll_init,
 };
 
 /**
@@ -785,40 +752,6 @@ static int rockchip_rk3399_pll_is_enabled(struct clk_hw *hw)
 	return !(pllcon & RK3399_PLLCON3_PWRDOWN);
 }
 
-static void rockchip_rk3399_pll_init(struct clk_hw *hw)
-{
-	struct rockchip_clk_pll *pll = to_rockchip_clk_pll(hw);
-	const struct rockchip_pll_rate_table *rate;
-	struct rockchip_pll_rate_table cur;
-	unsigned long drate;
-
-	if (!(pll->flags & ROCKCHIP_PLL_SYNC_RATE))
-		return;
-
-	drate = clk_hw_get_rate(hw);
-	rate = rockchip_get_pll_settings(pll, drate);
-
-	/* when no rate setting for the current rate, rely on clk_set_rate */
-	if (!rate)
-		return;
-
-	pll->data->get_params(pll, &cur);
-
-	if (!pll->data->compare_params(pll, rate, &cur)) {
-		struct clk *parent = clk_get_parent(hw->clk);
-
-		if (!parent) {
-			pr_warn("%s: parent of %s not available\n",
-				__func__, __clk_get_name(hw->clk));
-			return;
-		}
-
-		pr_debug("%s: pll %s: rate params do not match rate table, adjusting\n",
-			 __func__, __clk_get_name(hw->clk));
-		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,
@@ -839,7 +772,7 @@ static const struct clk_ops rockchip_rk3399_pll_clk_ops = {
 	.enable = rockchip_rk3399_pll_enable,
 	.disable = rockchip_rk3399_pll_disable,
 	.is_enabled = rockchip_rk3399_pll_is_enabled,
-	.init = rockchip_rk3399_pll_init,
+	.init = rockchip_pll_init,
 };
 
 /*
-- 
2.6.4

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

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

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 0/7] clk: rockchip: cleanup some code duplication

[Stephen Boyd]

On 04/28, Heiko Stuebner wrote:
> This tries to generalize code paths that contain some (nearly)
> duplicate code. While it's only a bit noticeable now for 3 different
> plls, the effect should be more pronounced when the next pll-type
> shows up.
> 

Looks ok. Are you sending another pull? Or I can apply them
directly.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Heiko Stuebner]

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.

Re: [PATCH 0/7] clk: rockchip: cleanup some code duplication

[Heiko Stuebner]

Am Freitag, 6. Mai 2016, 15:56:45 schrieb Stephen Boyd:
> On 04/28, Heiko Stuebner wrote:
> > This tries to generalize code paths that contain some (nearly)
> > duplicate code. While it's only a bit noticeable now for 3 different
> > plls, the effect should be more pronounced when the next pll-type
> > shows up.
> 
> Looks ok. Are you sending another pull? Or I can apply them
> directly.

I've sent a pull request with the first 3 patches of the series - see comment 
to your doubts to patch 4 and further. Also including one other fix from Doug 
as well).


Heiko

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

[Stephen Boyd]

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.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project