Re: [PATCH v4 61/68] clk: sprd: composite: Switch to determine_rate

[Stephen Boyd <sboyd@kernel.org>]

Quoting Harshit Mogalapalli (2023-06-13 10:11:43)
> Hi Maxime,
> 
> On 05/05/23 4:56 pm, Maxime Ripard wrote:
> > The Spreadtrum composite clocks implements a mux with a set_parent
> > hook, but doesn't provide a determine_rate implementation.
> > 
> > This is a bit odd, since set_parent() is there to, as its name implies,
> > change the parent of a clock. However, the most likely candidate to
> > trigger that parent change is a call to clk_set_rate(), with
> > determine_rate() figuring out which parent is the best suited for a
> > given rate.
> > 
> > The other trigger would be a call to clk_set_parent(), but it's far less
> > used, and it doesn't look like there's any obvious user for that clock.
> > 
> > So, the set_parent hook is effectively unused, possibly because of an
> > oversight. However, it could also be an explicit decision by the
> > original author to avoid any reparenting but through an explicit call to
> > clk_set_parent().
> > 
> > The driver does implement round_rate() though, which means that we can
> > change the rate of the clock, but we will never get to change the
> > parent.
> > 
> > However, It's hard to tell whether it's been done on purpose or not.
> > 
> > Since we'll start mandating a determine_rate() implementation, let's
> > convert the round_rate() implementation to a determine_rate(), which
> > will also make the current behavior explicit. And if it was an
> > oversight, the clock behaviour can be adjusted later on.
> > 
> > Cc: Baolin Wang <baolin.wang@linux.alibaba.com>
> > Cc: Chunyan Zhang <zhang.lyra@gmail.com>
> > Cc: Orson Zhai <orsonzhai@gmail.com>
> > Acked-by: Chunyan Zhang <zhang.lyra@gmail.com>
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > ---
> >   drivers/clk/sprd/composite.c | 16 +++++++++++-----
> >   1 file changed, 11 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/clk/sprd/composite.c b/drivers/clk/sprd/composite.c
> > index ebb644820b1e..d3a852720c07 100644
> > --- a/drivers/clk/sprd/composite.c
> > +++ b/drivers/clk/sprd/composite.c
> > @@ -9,13 +9,19 @@
> >   
> >   #include "composite.h"
> >   
> > -static long sprd_comp_round_rate(struct clk_hw *hw, unsigned long rate,
> > -                             unsigned long *parent_rate)
> > +static int sprd_comp_determine_rate(struct clk_hw *hw,
> > +                                 struct clk_rate_request *req)
> >   {
> >       struct sprd_comp *cc = hw_to_sprd_comp(hw);
> > +     unsigned long rate;
> >   
> > -     return sprd_div_helper_round_rate(&cc->common, &cc->div,
> > -                                      rate, parent_rate);
> > +     rate = sprd_div_helper_round_rate(&cc->common, &cc->div,
> > +                                       req->rate, &req->best_parent_rate);
> > +     if (rate < 0)
> > +             return rate;
> 
> As rate is unsigned long, it can never be less than zero, so the above 
> if condition is never true. Smatch detected this.
> 
> Also if we just change the type of rate from unsigned long to long, will 
> the return type of the function "sprd_comp_determine_rate" being int is 
> correct?

The return type of the determine_rate clk_op is int. We can't change
that. Are you asking if long will be converted properly to an int when
it is signed?

I see that sprd_div_helper_round_rate() calls divider_round_rate() which
calls divider_round_rate_parent() which calls divider_determine_rate(),
so we might as well change everything here to call
divider_determine_rate() directly. That significantly cleans things up.

----8<----
diff --git a/drivers/clk/sprd/composite.c b/drivers/clk/sprd/composite.c
index d3a852720c07..ad6b6383e21f 100644
--- a/drivers/clk/sprd/composite.c
+++ b/drivers/clk/sprd/composite.c
@@ -13,15 +13,8 @@ static int sprd_comp_determine_rate(struct clk_hw *hw,
 				    struct clk_rate_request *req)
 {
 	struct sprd_comp *cc = hw_to_sprd_comp(hw);
-	unsigned long rate;
 
-	rate = sprd_div_helper_round_rate(&cc->common, &cc->div,
-					  req->rate, &req->best_parent_rate);
-	if (rate < 0)
-		return rate;
-
-	req->rate = rate;
-	return 0;
+	return divider_determine_rate(hw, req, NULL, cc->div.width, 0);
 }
 
 static unsigned long sprd_comp_recalc_rate(struct clk_hw *hw,
diff --git a/drivers/clk/sprd/div.c b/drivers/clk/sprd/div.c
index 7621a1d1ab9c..c7261630cab4 100644
--- a/drivers/clk/sprd/div.c
+++ b/drivers/clk/sprd/div.c
@@ -9,23 +9,13 @@
 
 #include "div.h"
 
-long sprd_div_helper_round_rate(struct sprd_clk_common *common,
-				const struct sprd_div_internal *div,
-				unsigned long rate,
-				unsigned long *parent_rate)
-{
-	return divider_round_rate(&common->hw, rate, parent_rate,
-				  NULL, div->width, 0);
-}
-EXPORT_SYMBOL_GPL(sprd_div_helper_round_rate);
-
 static long sprd_div_round_rate(struct clk_hw *hw, unsigned long rate,
 				unsigned long *parent_rate)
 {
 	struct sprd_div *cd = hw_to_sprd_div(hw);
 
-	return sprd_div_helper_round_rate(&cd->common, &cd->div,
-					  rate, parent_rate);
+	return divider_round_rate(&cd->common.hw, rate, parent_rate, NULL,
+				  cd->div.width, 0);
 }
 
 unsigned long sprd_div_helper_recalc_rate(struct sprd_clk_common *common,
diff --git a/drivers/clk/sprd/div.h b/drivers/clk/sprd/div.h
index 6acfe6b179fc..f5d614b3dcf1 100644
--- a/drivers/clk/sprd/div.h
+++ b/drivers/clk/sprd/div.h
@@ -64,11 +64,6 @@ static inline struct sprd_div *hw_to_sprd_div(const struct clk_hw *hw)
 	return container_of(common, struct sprd_div, common);
 }
 
-long sprd_div_helper_round_rate(struct sprd_clk_common *common,
-				const struct sprd_div_internal *div,
-				unsigned long rate,
-				unsigned long *parent_rate);
-
 unsigned long sprd_div_helper_recalc_rate(struct sprd_clk_common *common,
 					  const struct sprd_div_internal *div,
 					  unsigned long parent_rate);