Re: [PATCH v10 1/8] opp: Add dev_pm_opp_get_current()

[Viresh Kumar]

On 31-08-21, 16:54, Dmitry Osipenko wrote:
> Add dev_pm_opp_get_current() helper that returns OPP corresponding
> to the current clock rate of a device.
> 
> Signed-off-by: Dmitry Osipenko <digetx@xxxxxxxxx>
> ---
>  drivers/opp/core.c     | 43 +++++++++++++++++++++++++++++++++++++++---
>  include/linux/pm_opp.h |  6 ++++++
>  2 files changed, 46 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 04b4691a8aac..dde8a5cc948c 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -939,7 +939,7 @@ static int _set_required_opps(struct device *dev,
>  	return ret;
>  }
>  
> -static void _find_current_opp(struct device *dev, struct opp_table *opp_table)
> +static struct dev_pm_opp *_find_current_opp(struct opp_table *opp_table)
>  {
>  	struct dev_pm_opp *opp = ERR_PTR(-ENODEV);
>  	unsigned long freq;
> @@ -949,6 +949,18 @@ static void _find_current_opp(struct device *dev, struct opp_table *opp_table)
>  		opp = _find_freq_ceil(opp_table, &freq);
>  	}
>  
> +	return opp;
> +}
> +
> +static void _find_and_set_current_opp(struct opp_table *opp_table)
> +{
> +	struct dev_pm_opp *opp;
> +
> +	if (opp_table->current_opp)
> +		return;

Why move this from caller as well ?

> +
> +	opp = _find_current_opp(opp_table);
> +
>  	/*
>  	 * Unable to find the current OPP ? Pick the first from the list since
>  	 * it is in ascending order, otherwise rest of the code will need to

If we aren't able to find current OPP based on current freq, then this
picks the first value from the list. Why shouldn't this be done in
your case as well ?

> @@ -1002,8 +1014,7 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
>  		return _disable_opp_table(dev, opp_table);
>  
>  	/* Find the currently set OPP if we don't know already */
> -	if (unlikely(!opp_table->current_opp))
> -		_find_current_opp(dev, opp_table);
> +	_find_and_set_current_opp(opp_table);
>  
>  	old_opp = opp_table->current_opp;
>  
> @@ -2931,3 +2942,29 @@ int dev_pm_opp_sync_regulators(struct device *dev)
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(dev_pm_opp_sync_regulators);
> +
> +/**
> + * dev_pm_opp_get_current() - Get current OPP
> + * @dev:	device for which we do this operation
> + *
> + * Get current OPP of a device based on current clock rate or by other means.
> + *
> + * Return: pointer to 'struct dev_pm_opp' on success and errorno otherwise.
> + */
> +struct dev_pm_opp *dev_pm_opp_get_current(struct device *dev)
> +{
> +	struct opp_table *opp_table;
> +	struct dev_pm_opp *opp;
> +
> +	opp_table = _find_opp_table(dev);
> +	if (IS_ERR(opp_table))
> +		return ERR_CAST(opp_table);
> +
> +	opp = _find_current_opp(opp_table);

This should not just go find the OPP based on current freq. This first
needs to check if curret_opp is set or not. If set, then just return
it, else run the _find_current_opp() function and update the
current_opp pointer as well.

-- 
viresh