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@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.w...@freescale.com>
> > Signed-off-by: Zhao Chenhui <chenhui.z...@freescale.com>
> > Signed-off-by: Li Yang <le...@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.
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
