Re: [PATCH v7 2/7] OPP: Add helpers for reading the binding properties

[Viresh Kumar]

On 24-04-20, 10:30, Matthias Kaehlcke wrote:
> Hi Georgi,
> 
> On Fri, Apr 24, 2020 at 06:53:59PM +0300, Georgi Djakov wrote:
> > From: Saravana Kannan <saravanak@xxxxxxxxxx>
> > 
> > The opp-hz DT property is not mandatory and we may use another property
> > as a key in the OPP table. Add helper functions to simplify the reading
> > and comparing the keys.
> > 
> > Signed-off-by: Saravana Kannan <saravanak@xxxxxxxxxx>
> > Signed-off-by: Georgi Djakov <georgi.djakov@xxxxxxxxxx>
> > ---
> > v7:
> > * Extracted just the helpers from patch v6, as Viresh advised to split it.
> > 
> > v6: https://lore.kernel.org/r/20191207002424.201796-3-saravanak@xxxxxxxxxx
> > 
> >  drivers/opp/core.c | 15 +++++++++++++--
> >  drivers/opp/of.c   | 42 ++++++++++++++++++++++++++----------------
> >  drivers/opp/opp.h  |  1 +
> >  3 files changed, 40 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> > index ba43e6a3dc0a..c9c1bbe6ae27 100644
> > --- a/drivers/opp/core.c
> > +++ b/drivers/opp/core.c
> > @@ -1272,11 +1272,21 @@ static bool _opp_supported_by_regulators(struct dev_pm_opp *opp,
> >  	return true;
> >  }
> >  
> > +int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2)
> > +{
> > +	if (opp1->rate != opp2->rate)
> > +		return opp1->rate < opp2->rate ? -1 : 1;
> > +	if (opp1->level != opp2->level)
> > +		return opp1->level < opp2->level ? -1 : 1;
> > +	return 0;
> > +}
> > +
> >  static int _opp_is_duplicate(struct device *dev, struct dev_pm_opp *new_opp,
> >  			     struct opp_table *opp_table,
> >  			     struct list_head **head)
> >  {
> >  	struct dev_pm_opp *opp;
> > +	int opp_cmp;
> >  
> >  	/*
> >  	 * Insert new OPP in order of increasing frequency and discard if
> > @@ -1287,12 +1297,13 @@ static int _opp_is_duplicate(struct device *dev, struct dev_pm_opp *new_opp,
> >  	 * loop.
> >  	 */
> >  	list_for_each_entry(opp, &opp_table->opp_list, node) {
> > -		if (new_opp->rate > opp->rate) {
> > +		opp_cmp = _opp_compare_key(new_opp, opp);
> > +		if (opp_cmp > 0) {
> >  			*head = &opp->node;
> >  			continue;
> >  		}
> >  
> > -		if (new_opp->rate < opp->rate)
> > +		if (opp_cmp < 0)
> >  			return 0;
> >  
> >  		/* Duplicate OPPs */
> > diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> > index 9cd8f0adacae..e33169c7e045 100644
> > --- a/drivers/opp/of.c
> > +++ b/drivers/opp/of.c
> > @@ -521,6 +521,28 @@ void dev_pm_opp_of_remove_table(struct device *dev)
> >  }
> >  EXPORT_SYMBOL_GPL(dev_pm_opp_of_remove_table);
> >  
> > +static int _read_opp_key(struct dev_pm_opp *new_opp, struct device_node *np,
> > +			 bool *rate_not_available)
> > +{
> > +	u64 rate;
> > +	int ret;
> > +
> > +	ret = of_property_read_u64(np, "opp-hz", &rate);
> > +	if (!ret) {
> > +		/*
> > +		 * Rate is defined as an unsigned long in clk API, and so
> > +		 * casting explicitly to its type. Must be fixed once rate is 64
> > +		 * bit guaranteed in clk API.
> > +		 */
> > +		new_opp->rate = (unsigned long)rate;
> > +	}
> 
> nit: curly braces are not needed

In fact they are as the comment is present within the if block (which
is the right thing to do). Yes the code will compile fine without
braces, but coding guideline suggests it around multi-line-statements.

> > +	*rate_not_available = !!ret;
> > +
> > +	of_property_read_u32(np, "opp-level", &new_opp->level);
> > +
> > +	return ret;
> > +}
> > +
> >  /**
> >   * _opp_add_static_v2() - Allocate static OPPs (As per 'v2' DT bindings)
> >   * @opp_table:	OPP table
> > @@ -558,26 +580,14 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,
> >  	if (!new_opp)
> >  		return ERR_PTR(-ENOMEM);
> >  
> > -	ret = of_property_read_u64(np, "opp-hz", &rate);
> > +	ret = _read_opp_key(new_opp, np, &rate_not_available);
> >  	if (ret < 0) {
> > -		/* "opp-hz" is optional for devices like power domains. */
> > -		if (!opp_table->is_genpd) {
> > -			dev_err(dev, "%s: opp-hz not found\n", __func__);
> > -			goto free_opp;
> > -		}
> > +		if (!opp_table->is_genpd)
> > +			dev_err(dev, "%s: opp key field not found\n", __func__);

Looks like the logic got changed here ? We used to goto free_opp only
if !is_genpd earlier..

> >  
> > -		rate_not_available = true;
> > -	} else {
> > -		/*
> > -		 * Rate is defined as an unsigned long in clk API, and so
> > -		 * casting explicitly to its type. Must be fixed once rate is 64
> > -		 * bit guaranteed in clk API.
> > -		 */
> > -		new_opp->rate = (unsigned long)rate;
> > +		goto free_opp;
> >  	}
> >  
> > -	of_property_read_u32(np, "opp-level", &new_opp->level);
> > -
> >  	/* Check if the OPP supports hardware's hierarchy of versions or not */
> >  	if (!_opp_is_supported(dev, opp_table, np)) {
> >  		dev_dbg(dev, "OPP not supported by hardware: %llu\n", rate);
> > diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
> > index d14e27102730..bcadb1e328a4 100644
> > --- a/drivers/opp/opp.h
> > +++ b/drivers/opp/opp.h
> > @@ -211,6 +211,7 @@ struct opp_device *_add_opp_dev(const struct device *dev, struct opp_table *opp_
> >  void _dev_pm_opp_find_and_remove_table(struct device *dev);
> >  struct dev_pm_opp *_opp_allocate(struct opp_table *opp_table);
> >  void _opp_free(struct dev_pm_opp *opp);
> > +int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2);
> >  int _opp_add(struct device *dev, struct dev_pm_opp *new_opp, struct opp_table *opp_table, bool rate_not_available);
> >  int _opp_add_v1(struct opp_table *opp_table, struct device *dev, unsigned long freq, long u_volt, bool dynamic);
> >  void _dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask, int last_cpu);
> 
> Reviewed-by: Matthias Kaehlcke <mka@xxxxxxxxxxxx>

-- 
viresh