[PATCH 3/3] powerpc/fsl: add MPIC timer wakeup support

[Wang Dongsheng-B40534]

Hi Scoot,

Thanks for reviewing.

> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Tuesday, March 19, 2013 8:31 AM
> To: Wang Dongsheng-B40534
> Cc: Gala Kumar-B11780; linuxppc-dev at lists.ozlabs.org; Wang Dongsheng-
> B40534; Zhao Chenhui-B35336; Li Yang-R58472
> Subject: Re: [PATCH 3/3] powerpc/fsl: add MPIC timer wakeup support
> 
> On 03/08/2013 01:38:47 AM, Wang Dongsheng wrote:
> > The driver provides a way to wake up the system by the MPIC timer.
> >
> > For example,
> > echo 5 > /sys/devices/system/mpic/timer_wakeup
> > echo standby > /sys/power/state
> >
> > After 5 seconds the MPIC timer will generate an interrupt to wake up
> > the system.
> >
> > Signed-off-by: Wang Dongsheng <dongsheng.wang at freescale.com>
> > Signed-off-by: Zhao Chenhui <chenhui.zhao at freescale.com>
> > Signed-off-by: Li Yang <leoli at freescale.com>
> 
> Does this work with deep sleep (echo mem > /sys/power/state on mpc8536,
> p1022, etc) or just regular sleep?
> 
The deep sleep can also work.

> > ---
> >  arch/powerpc/platforms/Kconfig              |    9 ++
> >  arch/powerpc/sysdev/Makefile                |    1 +
> >  arch/powerpc/sysdev/fsl_mpic_timer_wakeup.c |  185
> > +++++++++++++++++++++++++++
> >  3 files changed, 195 insertions(+), 0 deletions(-)  create mode
> > 100644 arch/powerpc/sysdev/fsl_mpic_timer_wakeup.c
> >
> > diff --git a/arch/powerpc/platforms/Kconfig
> > b/arch/powerpc/platforms/Kconfig index 5af04fa..487c37f 100644
> > --- a/arch/powerpc/platforms/Kconfig
> > +++ b/arch/powerpc/platforms/Kconfig
> > @@ -99,6 +99,15 @@ config MPIC_TIMER
> >  	  only tested on fsl chip, but it can potentially support
> >  	  other global timers complying to Open-PIC standard.
> >
> > +config FSL_MPIC_TIMER_WAKEUP
> > +	tristate "Freescale MPIC global timer wakeup driver"
> > +	depends on FSL_SOC &&  MPIC_TIMER
> > +	default n
> > +	help
> > +	  This is only for freescale powerpc platform.
> 
> This sentence is redundant... It already says "Freescale MPIC" in the
> name and depends on "FSL_SOC && MPIC_TIMER".
> 
Um...yes, I will remove it.
 
> > +static irqreturn_t fsl_mpic_timer_irq(int irq, void *dev_id) {
> > +	struct fsl_mpic_timer_wakeup *wakeup = dev_id;
> > +
> > +	schedule_work(&wakeup->free_work);
> > +	return IRQ_HANDLED;
> > +}
> 
> return wakeup->timer ? IRQ_HANDLED : IRQ_NONE;
> 
Looks better, thanks.

> > +
> > +static ssize_t fsl_timer_wakeup_show(struct device *dev,
> > +				struct device_attribute *attr,
> > +				char *buf)
> > +{
> > +	struct timeval interval;
> > +	int val = 0;
> > +
> > +	mutex_lock(&sysfs_lock);
> > +	if (fsl_wakeup->timer) {
> > +		mpic_get_remain_time(fsl_wakeup->timer, &interval);
> > +		val = interval.tv_sec + 1;
> > +	}
> > +	mutex_unlock(&sysfs_lock);
> > +
> > +	return sprintf(buf, "%d\n", val);
> > +}
> > +
> > +static ssize_t fsl_timer_wakeup_store(struct device *dev,
> > +				struct device_attribute *attr,
> > +				const char *buf,
> > +				size_t count)
> > +{
> > +	struct timeval interval;
> > +	int ret;
> > +
> > +	interval.tv_usec = 0;
> > +	if (kstrtol(buf, 0, &interval.tv_sec))
> > +		return -EINVAL;
> 
> I don't think the buffer will NUL-terminated...  Ordinarily there'll be
> an LF terminator, but you can't rely on that (many other sysfs attributes
> seem to, though...).
> 
I think we don't need to care about LF terminator.
The kstrtol--> _kstrtoull has been done.

> > +	mutex_lock(&sysfs_lock);
> > +
> > +	if (fsl_wakeup->timer && !interval.tv_sec) {
> > +		disable_irq_wake(fsl_wakeup->timer->irq);
> > +		mpic_free_timer(fsl_wakeup->timer);
> > +		fsl_wakeup->timer = NULL;
> > +		mutex_unlock(&sysfs_lock);
> > +
> > +		return count;
> > +	}
> > +
> > +	if (fsl_wakeup->timer) {
> > +		mutex_unlock(&sysfs_lock);
> > +		return -EBUSY;
> > +	}
> 
> So to change an already-set timer you have to set it to zero and then to
> what you want?  Why not just do:
> 
> 	if (fsl_wakeup->timer) {
> 		disable_irq_wake(...);
> 		mpic_free_timer(...);
> 		fsl_wakeup_timer = NULL;
> 	}
> 
> 	if (!interval.tv_sec) {
> 		mutex_unlock(&sysfs_lock);
> 		return count;
> 	}
> 
You can't break up the it.
if echo zero the code will cancel the timer that is currently running.
Not echo non-zero value just zero to cancel.

> > +	ret = subsys_system_register(&mpic_subsys, NULL);
> > +	if (ret)
> > +		goto err;
> 
> Maybe arch/powerpc/sysdev/mpic.c should be doing this?
> 
This can be registered by mpic. Maybe better than here.

> > +
> > +	for (i = 0; mpic_attributes[i]; i++) {
> > +		ret = device_create_file(mpic_subsys.dev_root,
> > +					mpic_attributes[i]);
> > +		if (ret)
> > +			goto err2;
> > +	}
> 
> Is this code ever going to register more than one?
> 
No, just one. I only keep the style here.
If you don't think it's necessary I can remove this loop.