Re: [PATCH v10 07/10] qcom: cpuidle: Add cpuidle driver for QCOM cpus

[Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>]

Hi Kevin,

On Wed, Nov 26, 2014 at 07:43:01PM +0000, Kevin Hilman wrote:
> Daniel Lezcano <daniel.lezcano@linaro.org> writes:
> 
> > On 11/21/2014 07:03 PM, Lina Iyer wrote:
> >> Add cpuidle driver interface to allow cpus to go into C-States. Use the
> >> cpuidle DT interface, common across ARM architectures, to provide the
> >> idle state information to the cpuidle framework.
> >>
> >> Supported modes at this time are Standby and Standalone Power Collapse.
> >>
> >> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> >
> > One nit and one comment below. Other than that:
> >
> > Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> >
> > [ ... ]
> >
> >> +static int qcom_cpu_stby(struct cpuidle_device *dev,
> >> +				struct cpuidle_driver *drv, int index)
> >> +{
> >> +	lpm_ops->standby(NULL);
> >
> > In my last comment I was referring about a check for entering
> > successfully the idle state:
> >
> > 	if (lpm_ops->standby(NULL))
> > 		return -1;
> >
> >> +	return index;
> >> +}
> >> +
> >> +static int qcom_cpu_spc(struct cpuidle_device *dev,
> >> +				struct cpuidle_driver *drv, int index)
> >> +{
> >> +	lpm_ops->spc(NULL);
> >> +
> >> +	return index;
>  
> Similar to Daniel's comment above.  if lpm_ops->spc() fails, do you want
> to fall back to standby.
> 
> Hmm, using the DT idle states, it doesn't look as straight forward as it
> used to be to fall back to a "safe state."

What do you mean by "safe state" ? As in coupled idle states ?

It is just for me to understand why DT makes things more complicated :)

If the idle state enter fails it might also be because there is an
aborted power down owing to eg pending IRQ, there would be no reason
why you would revert to standby instead of going back to the kernel, so
error code must be explicit at least in that respect.

Thanks,
Lorenzo