Re: [PATCH v3 1/3] OPP: Add support to find OPP for a set of keys

[Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>]

On 8/19/2025 1:48 PM, Viresh Kumar wrote:
> On 19-08-25, 11:04, Krishna Chaitanya Chundru wrote:
>> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
>> index edbd60501cf00dfd1957f7d19b228d1c61bbbdcc..ce359a3d444b0b7099cdd2421ab1019963d05d9f 100644
>> --- a/drivers/opp/core.c
>> +++ b/drivers/opp/core.c
>> @@ -461,6 +461,15 @@ int dev_pm_opp_get_opp_count(struct device *dev)
>>   EXPORT_SYMBOL_GPL(dev_pm_opp_get_opp_count);
>>   
>>   /* Helpers to read keys */
>> +static unsigned long _read_opp_key(struct dev_pm_opp *opp, int index, struct dev_pm_opp_key *key)
> 
> Move this to the end of the list, after _read_bw() i.e.
> 
ack
>> +{
>> +	key->bandwidth = opp->bandwidth ? opp->bandwidth[index].peak : 0;
>> +	key->freq = opp->rates[index];
>> +	key->level = opp->level;
>> +
>> +	return true;
>> +}
>> +
>>   static unsigned long _read_freq(struct dev_pm_opp *opp, int index)
>>   {
>>   	return opp->rates[index];
>> @@ -488,6 +497,23 @@ static bool _compare_exact(struct dev_pm_opp **opp, struct dev_pm_opp *temp_opp,
>>   	return false;
>>   }
>>   
>> +static bool _compare_opp_key_exact(struct dev_pm_opp **opp, struct dev_pm_opp *temp_opp,
>> +				   struct dev_pm_opp_key opp_key, struct dev_pm_opp_key key)
>> +{
> 
> And this after _compare_floor().
> 
ack
>> +	bool level_match = (opp_key.level == OPP_LEVEL_UNSET ||
> 
> Why are we still checking this ? You removed this from freq check but
> not level and bandwidth ?
> 
ok I will change for level and bw also similar to freq.
>> +			    key.level == OPP_LEVEL_UNSET || opp_key.level == key.level);
>> +	bool bw_match = (opp_key.bandwidth == 0 ||
>> +			 key.bandwidth == 0 || opp_key.bandwidth == key.bandwidth);
>> +	bool freq_match = (key.freq == 0 || opp_key.freq == key.freq);
>> +
>> +	if (freq_match && level_match && bw_match) {
>> +		*opp = temp_opp;
>> +		return true;
>> +	}
>> +
>> +	return false;
>> +}
>> +
>>   static bool _compare_ceil(struct dev_pm_opp **opp, struct dev_pm_opp *temp_opp,
>>   			  unsigned long opp_key, unsigned long key)
>>   {
>> @@ -541,6 +567,40 @@ static struct dev_pm_opp *_opp_table_find_key(struct opp_table *opp_table,
>>   	return opp;
>>   }
>>   
>> +static struct dev_pm_opp *_opp_table_find_opp_key(struct opp_table *opp_table,
>> +		struct dev_pm_opp_key *key, int index, bool available,
>> +		unsigned long (*read)(struct dev_pm_opp *opp, int index,
>> +				      struct dev_pm_opp_key *key),
>> +		bool (*compare)(struct dev_pm_opp **opp, struct dev_pm_opp *temp_opp,
>> +				struct dev_pm_opp_key opp_key, struct dev_pm_opp_key key),
>> +		bool (*assert)(struct opp_table *opp_table, unsigned int index))
>> +{
>> +	struct dev_pm_opp *temp_opp, *opp = ERR_PTR(-ERANGE);
>> +	struct dev_pm_opp_key temp_key;
>> +
>> +	/* Assert that the requirement is met */
>> +	if (assert && !assert(opp_table, index))
> 
> Just drop the `assert` check, it isn't optional. Make the same change
> in _opp_table_find_key() too in a separate patch if you can.
> 
ack
>> +		return ERR_PTR(-EINVAL);
>> +
>> +	guard(mutex)(&opp_table->lock);
>> +
>> +	list_for_each_entry(temp_opp, &opp_table->opp_list, node) {
>> +		if (temp_opp->available == available) {
>> +			read(temp_opp, index, &temp_key);
>> +			if (compare(&opp, temp_opp, temp_key, *key))
> 
> Update *key and do dev_pm_opp_get() here itself. And same in
> _opp_table_find_key().
> 
ack
>> +				break;
>> +		}
>> +	}
>> +
>> +	/* Increment the reference count of OPP */
>> +	if (!IS_ERR(opp)) {
>> +		*key = temp_key;
>> +		dev_pm_opp_get(opp);
>> +	}
>> +
>> +	return opp;
>> +}
>> +
>>   static struct dev_pm_opp *
>>   _find_key(struct device *dev, unsigned long *key, int index, bool available,
>>   	  unsigned long (*read)(struct dev_pm_opp *opp, int index),
>> @@ -632,6 +692,46 @@ struct dev_pm_opp *dev_pm_opp_find_freq_exact(struct device *dev,
>>   }
>>   EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_exact);
>>   
>> +/**
>> + * dev_pm_opp_find_key_exact() - Search for an exact OPP key
>> + * @dev:                Device for which the OPP is being searched
>> + * @key:                OPP key to match
>> + * @available:          true/false - match for available OPP
>> + *
>> + * Return: Searches for an exact match the OPP key in the OPP table and returns
> 
> The `Return` section should only talk about the returned values. The
> above line for example should be present as a standalone line before
> the `Return` section.
> 
ack
>> + * pointer to the  matching opp if found, else returns ERR_PTR  in case of error
>> + * and should  be handled using IS_ERR. Error return values can be:
>> + * EINVAL:      for bad pointer
>> + * ERANGE:      no match found for search
>> + * ENODEV:      if device not found in list of registered devices
>> + *
>> + * Note: available is a modifier for the search. if available=true, then the
>> + * match is for exact matching key and is available in the stored OPP
>> + * table. if false, the match is for exact key which is not available.
>> + *
>> + * This provides a mechanism to enable an opp which is not available currently
>> + * or the opposite as well.
>> + *
>> + * The callers are required to call dev_pm_opp_put() for the returned OPP after
>> + * use.
> 
> There are various minor issues in the text here, like double spaces,
> not starting with a capital letter after a full stop, etc. Also put
> arguments in `` block, like `available`.
> 
ack
>> + */
>> +struct dev_pm_opp *dev_pm_opp_find_key_exact(struct device *dev,
>> +					     struct dev_pm_opp_key key,
>> +					     bool available)
>> +{
>> +	struct opp_table *opp_table __free(put_opp_table) = _find_opp_table(dev);
>> +
>> +	if (IS_ERR(opp_table)) {
>> +		dev_err(dev, "%s: OPP table not found (%ld)\n", __func__,
>> +			PTR_ERR(opp_table));
>> +		return ERR_CAST(opp_table);
>> +	}
>> +
>> +	return _opp_table_find_opp_key(opp_table, &key, 0, available, _read_opp_key,
>> +				       _compare_opp_key_exact, assert_single_clk);
> 
> Since the only user doesn't use `index` for now, I wonder if that
> should be added at all in all these functions.
> 
ok I will remove it.
>> +}
>> +EXPORT_SYMBOL_GPL(dev_pm_opp_find_key_exact);
>> +
>>   /**
>>    * dev_pm_opp_find_freq_exact_indexed() - Search for an exact freq for the
>>    *					 clock corresponding to the index
>> diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
>> index cf477beae4bbede88223566df5f43d85adc5a816..53e02098129d215970d0854b1f8ffaf4499f2bd4 100644
>> --- a/include/linux/pm_opp.h
>> +++ b/include/linux/pm_opp.h
>> @@ -98,6 +98,18 @@ struct dev_pm_opp_data {
>>   	unsigned long u_volt;
>>   };
>>   
>> +/**
>> + * struct dev_pm_opp_key - Key used to identify OPP entries
>> + * @freq:       Frequency in Hz
>> + * @level:      Performance level associated with the OPP entry
>> + * @bandwidth:  Bandwidth associated with the OPP entry
> 
> Also mention the NOP value of all these keys, i.e. what the user must
> fill them with if that key is not supposed to be matched.
> 
ack
>> + */
>> +struct dev_pm_opp_key {
>> +	unsigned long freq;
>> +	unsigned int level;
>> +	u32 bandwidth;
> 
> Maybe use `bw` here like in other APIs.
> 
ack.

- Krishna Chaitanya.
>> +};
>> +
>>   #if defined(CONFIG_PM_OPP)
>>   
>>   struct opp_table *dev_pm_opp_get_opp_table(struct device *dev);
>> @@ -131,6 +143,10 @@ struct dev_pm_opp *dev_pm_opp_find_freq_exact(struct device *dev,
>>   					      unsigned long freq,
>>   					      bool available);
>>   
>> +struct dev_pm_opp *dev_pm_opp_find_key_exact(struct device *dev,
>> +					     struct dev_pm_opp_key key,
>> +					     bool available);
>> +
>>   struct dev_pm_opp *
>>   dev_pm_opp_find_freq_exact_indexed(struct device *dev, unsigned long freq,
>>   				   u32 index, bool available);
>> @@ -289,6 +305,13 @@ static inline struct dev_pm_opp *dev_pm_opp_find_freq_exact(struct device *dev,
>>   	return ERR_PTR(-EOPNOTSUPP);
>>   }
>>   
>> +static inline struct dev_pm_opp *dev_pm_opp_find_key_exact(struct device *dev,
>> +							   struct dev_pm_opp_key key,
>> +							   bool available)
>> +{
>> +	return ERR_PTR(-EOPNOTSUPP);
>> +}
>> +
>>   static inline struct dev_pm_opp *
>>   dev_pm_opp_find_freq_exact_indexed(struct device *dev, unsigned long freq,
>>   				   u32 index, bool available)
>>
>> -- 
>> 2.34.1
>