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

[Scott Wood <scottwood@freescale.com>]

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.
>=20
> For example,
> echo 5 > /sys/devices/system/mpic/timer_wakeup
> echo standby > /sys/power/state
>=20
> After 5 seconds the MPIC timer will generate an interrupt to wake up
> the system.
>=20
> Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com>
> Signed-off-by: Zhao Chenhui <chenhui.zhao@freescale.com>
> Signed-off-by: Li Yang <leoli@freescale.com>

Does this work with deep sleep (echo mem > /sys/power/state on mpc8536, =20
p1022, etc) or just regular sleep?

> ---
>  arch/powerpc/platforms/Kconfig              |    9 ++
>  arch/powerpc/sysdev/Makefile                |    1 +
>  arch/powerpc/sysdev/fsl_mpic_timer_wakeup.c |  185 =20
> +++++++++++++++++++++++++++
>  3 files changed, 195 insertions(+), 0 deletions(-)
>  create mode 100644 arch/powerpc/sysdev/fsl_mpic_timer_wakeup.c
>=20
> diff --git a/arch/powerpc/platforms/Kconfig =20
> 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.
>=20
> +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 =20
name and depends on "FSL_SOC && MPIC_TIMER".

> +static irqreturn_t fsl_mpic_timer_irq(int irq, void *dev_id)
> +{
> +	struct fsl_mpic_timer_wakeup *wakeup =3D dev_id;
> +
> +	schedule_work(&wakeup->free_work);
> +	return IRQ_HANDLED;
> +}

return wakeup->timer ? IRQ_HANDLED : IRQ_NONE;

> +
> +static ssize_t fsl_timer_wakeup_show(struct device *dev,
> +				struct device_attribute *attr,
> +				char *buf)
> +{
> +	struct timeval interval;
> +	int val =3D 0;
> +
> +	mutex_lock(&sysfs_lock);
> +	if (fsl_wakeup->timer) {
> +		mpic_get_remain_time(fsl_wakeup->timer, &interval);
> +		val =3D 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 =3D 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 =20
attributes
seem to, though...).

> +	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 =3D 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 =3D NULL;
	}

	if (!interval.tv_sec) {
		mutex_unlock(&sysfs_lock);
		return count;
	}

> +	ret =3D subsys_system_register(&mpic_subsys, NULL);
> +	if (ret)
> +		goto err;

Maybe arch/powerpc/sysdev/mpic.c should be doing this?

> +
> +	for (i =3D 0; mpic_attributes[i]; i++) {
> +		ret =3D device_create_file(mpic_subsys.dev_root,
> +					mpic_attributes[i]);
> +		if (ret)
> +			goto err2;
> +	}

Is this code ever going to register more than one?

-Scott=