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

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

[-- Attachment #1: Type: text/plain, Size: 1510 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).
> 
> Change-Id: Iaa0d3848d63d9ce03f65ea76f263e4685a4c295e
> Signed-off-by: Lin Huang <hl@rock-chips.com>
> ---
>  drivers/devfreq/devfreq.c | 17 ++++++++++++++++-
>  include/linux/devfreq.h   |  9 +++++++++
>  2 files changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index bf3ea76..d1152eb 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -364,7 +364,10 @@ void devfreq_monitor_suspend(struct devfreq *devfreq)
>  		return;
>  	}
>  
> -	devfreq_update_status(devfreq, devfreq->previous_freq);
> +	if (devfreq->suspend_freq)
> +		devfreq_update_status(devfreq, devfreq->suspend_freq);
> +	else
> +		devfreq_update_status(devfreq, devfreq->previous_freq);

This is incorrect.

devfreq_update_status is to record the statistics.
This code intends to record the current status before entering suspend;
thus you should not record "suspend_freq" here.

If you really need to record the frequency during suspended state
for the statistics, you need to do that at resume; however, I object to
that idea either.

If you really want to set a predefined suspend-to-RAM mode frequency,
(probably because of HW instability during resume process)
you need to update, not udpate_status (statistics).




Cheers,
MyungJoo