RE: [RESEND PATCH v3 1/2] PM/devfreq: add suspend frequency support

["MyungJoo Ham" <myungjoo.ham@samsung.com>]

[-- Attachment #1: Type: text/plain, Size: 2160 bytes --]

> Add suspend frequency support and if needed set it to
> the frequency obtained from the suspend opp (can be defined
> using opp-v2 bindings and is optional).
> 
> Signed-off-by: Lin Huang <hl@rock-chips.com>
> ---
> Changes in v2:
> - use update_devfreq() instead devfreq_update_status()
> Changes in v3:
> - fix build error
> 
>  drivers/devfreq/devfreq.c                 | 18 ++++++++++++++++--
>  drivers/devfreq/governor_simpleondemand.c |  9 +++++++++
>  include/linux/devfreq.h                   |  8 ++++++++
>  3 files changed, 33 insertions(+), 2 deletions(-)
> 
>  
[]
> +void devfreq_opp_get_suspend_opp(struct device *dev, struct devfreq *devfreq)
> +{
> +	struct dev_pm_opp *suspend_opp;
> +
> +	rcu_read_lock();
> +	suspend_opp = dev_pm_opp_get_suspend_opp(dev);
> +	if (suspend_opp)
> +		devfreq->suspend_freq = dev_pm_opp_get_freq(suspend_opp);
> +	rcu_read_unlock();
> +}
> +EXPORT_SYMBOL(devfreq_opp_get_suspend_opp);
> +

Why do we need this function?

This could be done at the init time (devfreq_add_device).

Plus, when it does not have dev_pm_opp_get_suspend_opp() available,

>  /**
>   * devfreq_register_opp_notifier() - Helper function to get devfreq notified
>   *				   for any changes in the OPP availability
> diff --git a/drivers/devfreq/governor_simpleondemand.c b/drivers/devfreq/governor_simpleondemand.c
> index ae72ba5..a3efee0 100644
> --- a/drivers/devfreq/governor_simpleondemand.c
> +++ b/drivers/devfreq/governor_simpleondemand.c
> @@ -29,6 +29,15 @@ static int devfreq_simple_ondemand_func(struct devfreq *df,
>  	struct devfreq_simple_ondemand_data *data = df->data;
>  	unsigned long max = (df->max_freq) ? df->max_freq : UINT_MAX;
>  
> +	/*
> +	 * if devfreq in suspend status and have suspend_freq,
> +	 * the frequency need to set to suspend_freq
> +	 */
> +	if (df->stop_polling && df->suspend_freq) {
> +		*freq =	df->suspend_freq;
> +		return 0;
> +	}
> +

Why are you adding this? "Stop polling" does not necessarily mean that
we are doing suspend-to-RAM/disk. It may really mean just to stop at
the current frequency.


Cheers,
MyungJoo