[3/5] clk: divider: add divider_ro_round_rate helper

Message ID 20180105170959.17266-4-jbrunet@baylibre.com
State New
Headers show
Series
  • clk: read-only dividers and rate propagation fixup
Related show

Commit Message

Jerome Brunet Jan. 5, 2018, 5:09 p.m.
Like divider_round_rate, a couple a of driver are doing more or less
the same operation to round the rate of the divider when it is read-only.

We can factor this code so let's provide an helper function for this

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>

---
 drivers/clk/clk-divider.c    | 43 ++++++++++++++++++++++++++++---------------
 include/linux/clk-provider.h | 15 +++++++++++++++
 2 files changed, 43 insertions(+), 15 deletions(-)

-- 
2.14.3

--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

David Lechner Jan. 11, 2018, 11:08 p.m. | #1
On 01/05/2018 11:09 AM, Jerome Brunet wrote:
> Like divider_round_rate, a couple a of driver are doing more or less

> the same operation to round the rate of the divider when it is read-only.

> 

> We can factor this code so let's provide an helper function for this


Perhaps you could also make use of this new helper here (clk-divider.c):

const struct clk_ops clk_divider_ro_ops = {
	.recalc_rate = clk_divider_recalc_rate,
	.round_rate = clk_divider_round_rate,
};
EXPORT_SYMBOL_GPL(clk_divider_ro_ops);

> 

> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>

> ---

>   drivers/clk/clk-divider.c    | 43 ++++++++++++++++++++++++++++---------------

>   include/linux/clk-provider.h | 15 +++++++++++++++

>   2 files changed, 43 insertions(+), 15 deletions(-)

> 

> diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c

> index a851d3e04c7f..3eb2b27f3513 100644

> --- a/drivers/clk/clk-divider.c

> +++ b/drivers/clk/clk-divider.c

> @@ -344,29 +344,42 @@ long divider_round_rate_parent(struct clk_hw *hw, struct clk_hw *parent,

>   }

>   EXPORT_SYMBOL_GPL(divider_round_rate_parent);

>   


It is nice to have documentation comments on public functions. Especially
in this case where @prate is an in/out parameter and @table is optional.

> +long divider_ro_round_rate_parent(struct clk_hw *hw, struct clk_hw *parent,

> +				  unsigned long rate, unsigned long *prate,

> +				  const struct clk_div_table *table, u8 width,

> +				  unsigned long flags, unsigned int val)

> +{

> +	int div;

> +

> +	div = _get_div(table, val, flags, width);

> +

> +	/* Even a read-only clock can propagate a rate change */

> +	if (clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) {

> +		if (!parent)

> +			return -EINVAL;

> +

> +		*prate = clk_hw_round_rate(parent, rate * div);

> +	}

> +

> +	return DIV_ROUND_UP_ULL((u64)*prate, div);

> +}

> +EXPORT_SYMBOL_GPL(divider_ro_round_rate_parent);

> +

> +

>   static long clk_divider_round_rate(struct clk_hw *hw, unsigned long rate,

>   				unsigned long *prate)

>   {

>   	struct clk_divider *divider = to_clk_divider(hw);

> -	struct clk_hw *hw_parent = clk_hw_get_parent(hw);

> -	int bestdiv;

> +	u32 val;

>   

>   	/* if read only, just return current value */

>   	if (divider->flags & CLK_DIVIDER_READ_ONLY) {

> -		bestdiv = clk_readl(divider->reg) >> divider->shift;

> -		bestdiv &= div_mask(divider->width);

> -		bestdiv = _get_div(divider->table, bestdiv, divider->flags,

> -			divider->width);

> -

> -		/* Even a read-only clock can propagate a rate change */

> -		if (clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) {

> -			if (!hw_parent)

> -				return -EINVAL;

> -

> -			*prate = clk_hw_round_rate(hw_parent, rate * bestdiv);

> -		}

> +		val = clk_readl(divider->reg) >> divider->shift;

> +		val &= div_mask(divider->width);

>   

> -		return DIV_ROUND_UP_ULL((u64)*prate, bestdiv);

> +		return divider_ro_round_rate(hw, rate, prate, divider->table,

> +					     divider->width, divider->flags,

> +					     val);

>   	}

>   

>   	return divider_round_rate(hw, rate, prate, divider->table,

> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h

> index 175a62a15619..eb2c3a035e98 100644

> --- a/include/linux/clk-provider.h

> +++ b/include/linux/clk-provider.h

> @@ -417,6 +417,10 @@ long divider_round_rate_parent(struct clk_hw *hw, struct clk_hw *parent,

>   			       unsigned long rate, unsigned long *prate,

>   			       const struct clk_div_table *table,

>   			       u8 width, unsigned long flags);

> +long divider_ro_round_rate_parent(struct clk_hw *hw, struct clk_hw *parent,

> +				  unsigned long rate, unsigned long *prate,

> +				  const struct clk_div_table *table, u8 width,

> +				  unsigned long flags, unsigned int val);

>   int divider_get_val(unsigned long rate, unsigned long parent_rate,

>   		const struct clk_div_table *table, u8 width,

>   		unsigned long flags);

> @@ -772,6 +776,17 @@ static inline long divider_round_rate(struct clk_hw *hw, unsigned long rate,

>   					 rate, prate, table, width, flags);

>   }

>   


Same here (about documentation comment).

> +static inline long divider_ro_round_rate(struct clk_hw *hw, unsigned long rate,

> +					 unsigned long *prate,

> +					 const struct clk_div_table *table,

> +					 u8 width, unsigned long flags,

> +					 unsigned int val)

> +{

> +	return divider_ro_round_rate_parent(hw, clk_hw_get_parent(hw),

> +					    rate, prate, table, width, flags,

> +					    val);

> +}

> +

>   /*

>    * FIXME clock api without lock protection

>    */

> 


--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jerome Brunet Jan. 17, 2018, 5:47 p.m. | #2
On Thu, 2018-01-11 at 17:08 -0600, David Lechner wrote:
> On 01/05/2018 11:09 AM, Jerome Brunet wrote:

> > Like divider_round_rate, a couple a of driver are doing more or less

> > the same operation to round the rate of the divider when it is read-only.

> > 

> > We can factor this code so let's provide an helper function for this

> 

> Perhaps you could also make use of this new helper here (clk-divider.c):

> 

> const struct clk_ops clk_divider_ro_ops = {

> 	.recalc_rate = clk_divider_recalc_rate,

> 	.round_rate = clk_divider_round_rate,

> };

> EXPORT_SYMBOL_GPL(clk_divider_ro_ops);


The helper is used in this driver and ro_ops will use it.
I don't get this comment, could you be more specific

> 

> > 

> > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>

> > ---

> >   drivers/clk/clk-divider.c    | 43 ++++++++++++++++++++++++++++---------------

> >   include/linux/clk-provider.h | 15 +++++++++++++++

> >   2 files changed, 43 insertions(+), 15 deletions(-)

> > 

> > diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c

> > index a851d3e04c7f..3eb2b27f3513 100644

> > --- a/drivers/clk/clk-divider.c

> > +++ b/drivers/clk/clk-divider.c

> > @@ -344,29 +344,42 @@ long divider_round_rate_parent(struct clk_hw *hw, struct clk_hw *parent,

> >   }

> >   EXPORT_SYMBOL_GPL(divider_round_rate_parent);

> >   

> 

> It is nice to have documentation comments on public functions. Especially

> in this case where @prate is an in/out parameter and @table is optional.


Sure, but divider_round_rate_parent was already and undocumented. There is no
reason to change this is this particular patch (it is not the topic)

The RO helper being the counter part of this one, it did not do it differently

I suppose a documentation could be added in another patch.

However, I don't know if there is policy regarding the documentation of
functions internal to CCF. Maybe Stephen can tell us ?

Cheers
Jerome


> 

> > +long divider_ro_round_rate_parent(struct clk_hw *hw, struct clk_hw *parent,

> > +				  unsigned long rate, unsigned long *prate,

> > +				  const struct clk_div_table *table, u8 width,

> > +				  unsigned long flags, unsigned int val)

> > +{

> > +	int div;

> > +

> > +	div = _get_div(table, val, flags, width);

> > +

> > +	/* Even a read-only clock can propagate a rate change */

> > +	if (clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) {

> > +		if (!parent)

> > +			return -EINVAL;

> > +

> > +		*prate = clk_hw_round_rate(parent, rate * div);

> > +	}

> > +

> > +	return DIV_ROUND_UP_ULL((u64)*prate, div);

> > +}

> > +EXPORT_SYMBOL_GPL(divider_ro_round_rate_parent);

> > +

> > +

> >   static long clk_divider_round_rate(struct clk_hw *hw, unsigned long rate,

> >   				unsigned long *prate)

> >   {

> >   	struct clk_divider *divider = to_clk_divider(hw);

> > -	struct clk_hw *hw_parent = clk_hw_get_parent(hw);

> > -	int bestdiv;

> > +	u32 val;

> >   

> >   	/* if read only, just return current value */

> >   	if (divider->flags & CLK_DIVIDER_READ_ONLY) {

> > -		bestdiv = clk_readl(divider->reg) >> divider->shift;

> > -		bestdiv &= div_mask(divider->width);

> > -		bestdiv = _get_div(divider->table, bestdiv, divider->flags,

> > -			divider->width);

> > -

> > -		/* Even a read-only clock can propagate a rate change */

> > -		if (clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) {

> > -			if (!hw_parent)

> > -				return -EINVAL;

> > -

> > -			*prate = clk_hw_round_rate(hw_parent, rate * bestdiv);

> > -		}

> > +		val = clk_readl(divider->reg) >> divider->shift;

> > +		val &= div_mask(divider->width);

> >   

> > -		return DIV_ROUND_UP_ULL((u64)*prate, bestdiv);

> > +		return divider_ro_round_rate(hw, rate, prate, divider->table,

> > +					     divider->width, divider->flags,

> > +					     val);

> >   	}

> >   

> >   	return divider_round_rate(hw, rate, prate, divider->table,

> > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h

> > index 175a62a15619..eb2c3a035e98 100644

> > --- a/include/linux/clk-provider.h

> > +++ b/include/linux/clk-provider.h

> > @@ -417,6 +417,10 @@ long divider_round_rate_parent(struct clk_hw *hw, struct clk_hw *parent,

> >   			       unsigned long rate, unsigned long *prate,

> >   			       const struct clk_div_table *table,

> >   			       u8 width, unsigned long flags);

> > +long divider_ro_round_rate_parent(struct clk_hw *hw, struct clk_hw *parent,

> > +				  unsigned long rate, unsigned long *prate,

> > +				  const struct clk_div_table *table, u8 width,

> > +				  unsigned long flags, unsigned int val);

> >   int divider_get_val(unsigned long rate, unsigned long parent_rate,

> >   		const struct clk_div_table *table, u8 width,

> >   		unsigned long flags);

> > @@ -772,6 +776,17 @@ static inline long divider_round_rate(struct clk_hw *hw, unsigned long rate,

> >   					 rate, prate, table, width, flags);

> >   }

> >   

> 

> Same here (about documentation comment).

> 

> > +static inline long divider_ro_round_rate(struct clk_hw *hw, unsigned long rate,

> > +					 unsigned long *prate,

> > +					 const struct clk_div_table *table,

> > +					 u8 width, unsigned long flags,

> > +					 unsigned int val)

> > +{

> > +	return divider_ro_round_rate_parent(hw, clk_hw_get_parent(hw),

> > +					    rate, prate, table, width, flags,

> > +					    val);

> > +}

> > +

> >   /*

> >    * FIXME clock api without lock protection

> >    */

> > 

> 

> 


--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
index a851d3e04c7f..3eb2b27f3513 100644
--- a/drivers/clk/clk-divider.c
+++ b/drivers/clk/clk-divider.c
@@ -344,29 +344,42 @@  long divider_round_rate_parent(struct clk_hw *hw, struct clk_hw *parent,
 }
 EXPORT_SYMBOL_GPL(divider_round_rate_parent);
 
+long divider_ro_round_rate_parent(struct clk_hw *hw, struct clk_hw *parent,
+				  unsigned long rate, unsigned long *prate,
+				  const struct clk_div_table *table, u8 width,
+				  unsigned long flags, unsigned int val)
+{
+	int div;
+
+	div = _get_div(table, val, flags, width);
+
+	/* Even a read-only clock can propagate a rate change */
+	if (clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) {
+		if (!parent)
+			return -EINVAL;
+
+		*prate = clk_hw_round_rate(parent, rate * div);
+	}
+
+	return DIV_ROUND_UP_ULL((u64)*prate, div);
+}
+EXPORT_SYMBOL_GPL(divider_ro_round_rate_parent);
+
+
 static long clk_divider_round_rate(struct clk_hw *hw, unsigned long rate,
 				unsigned long *prate)
 {
 	struct clk_divider *divider = to_clk_divider(hw);
-	struct clk_hw *hw_parent = clk_hw_get_parent(hw);
-	int bestdiv;
+	u32 val;
 
 	/* if read only, just return current value */
 	if (divider->flags & CLK_DIVIDER_READ_ONLY) {
-		bestdiv = clk_readl(divider->reg) >> divider->shift;
-		bestdiv &= div_mask(divider->width);
-		bestdiv = _get_div(divider->table, bestdiv, divider->flags,
-			divider->width);
-
-		/* Even a read-only clock can propagate a rate change */
-		if (clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) {
-			if (!hw_parent)
-				return -EINVAL;
-
-			*prate = clk_hw_round_rate(hw_parent, rate * bestdiv);
-		}
+		val = clk_readl(divider->reg) >> divider->shift;
+		val &= div_mask(divider->width);
 
-		return DIV_ROUND_UP_ULL((u64)*prate, bestdiv);
+		return divider_ro_round_rate(hw, rate, prate, divider->table,
+					     divider->width, divider->flags,
+					     val);
 	}
 
 	return divider_round_rate(hw, rate, prate, divider->table,
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 175a62a15619..eb2c3a035e98 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -417,6 +417,10 @@  long divider_round_rate_parent(struct clk_hw *hw, struct clk_hw *parent,
 			       unsigned long rate, unsigned long *prate,
 			       const struct clk_div_table *table,
 			       u8 width, unsigned long flags);
+long divider_ro_round_rate_parent(struct clk_hw *hw, struct clk_hw *parent,
+				  unsigned long rate, unsigned long *prate,
+				  const struct clk_div_table *table, u8 width,
+				  unsigned long flags, unsigned int val);
 int divider_get_val(unsigned long rate, unsigned long parent_rate,
 		const struct clk_div_table *table, u8 width,
 		unsigned long flags);
@@ -772,6 +776,17 @@  static inline long divider_round_rate(struct clk_hw *hw, unsigned long rate,
 					 rate, prate, table, width, flags);
 }
 
+static inline long divider_ro_round_rate(struct clk_hw *hw, unsigned long rate,
+					 unsigned long *prate,
+					 const struct clk_div_table *table,
+					 u8 width, unsigned long flags,
+					 unsigned int val)
+{
+	return divider_ro_round_rate_parent(hw, clk_hw_get_parent(hw),
+					    rate, prate, table, width, flags,
+					    val);
+}
+
 /*
  * FIXME clock api without lock protection
  */