Message-ID: <20080415121849.GB24498@rakim.wolfsonmicro.main>
Date:	Tue, 15 Apr 2008 13:18:49 +0100
From:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
To:	David Brownell <david-b@...bell.net>
Cc:	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Jiri Kosina <jkosina@...e.cz>, linux-input@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] wm97xx-core: Support use as a wakeup source

On Mon, Apr 14, 2008 at 05:29:07PM -0700, David Brownell wrote:
> On Monday 14 April 2008, Dmitry Torokhov wrote:
> > Hmm, another unique module option... How about using device_can_wakeup()

The reason there are so many configuration options here is that many of
the features of the driver depend on exactly how the chip is connected
to the rest of the system and can't be automatically discovered.  In the
embedded applications where these devices are usually deployed this is
usually an OK user experience - if these hooks aren't provided what
tends to happen is that users patch the driver directly to get the
configurablilty they need.

> I don't quite follow the technical content of this "suspend_mode"
> mask, but it sure looks as if it should use the driver model flags
> in addition to whatever that mask is doing.

It's probably worth pointing out here that this part of the WM97xx
driver is split into two portions.  This part of the driver covers the
WM97xx itself but in order to actually use functionality like wakeup a
machine specific driver needs to be added.  The WM97xx chips have a
series of multi-function pins, some of which can be configured to
provide a signal mirroring the pen down status with most of the chip
powered down.  It can also be configured to do other things like try to
start the AC97 bus and/or start the touchscreen digitiser up if it
detects a pen down while suspended.

At the minute this is implemented by having the WM97xx register a child
device which platform code can provide.  When the child device probes it
configures both the WM97xx and the rest of the system.  The child device
can also provide other machine-specific things like use of the pen down
or interrupt signals from the chip to control when the touch screen is
read, synchronisation of the touch screen with writes to the display to
reduce interference from the video signals and accelerated reading of
the touchscreen if the AC97 controller is able to support it.

> There are two aspects to this:

>  (a) Whether on a given system the touchscreen *can* issue wakeup
>      events ... call device_init_wakeup(dev, true) to say it can.
>      In this case, presumably suspend_mode == 0 means it can't.
>      Board setup, or at latest probe(), should device_init_wakeup().

> So:  yes, this should initialize and use those driver model flags.

There is a bit of a question about which device this should be checked,
though the WM97xx itself is probably more sensible.

> > > +	reg = wm97xx_reg_read(wm, AC97_WM97XX_DIGITISER2);
> > > +	reg &= ~WM97XX_PRP_DET_DIG;
> > > +	if (wm->input_dev->users && suspend_mode)

> This should probably check device_may_wakeup(SOMEDEV), rather
> than testing suspend_mode.

That by itself isn't enough since if the system hasn't been configured
appropriately then the WM97xx won't be able to wake the system - we need
the suspend_mode configuration to know how to wake.  I'll do a spin of
the patch which pushes the configuration of suspend mode through an API
to let the core keep track of this.

> The wakeup mechanism here isn't clear to me.  Does this assume
> that the touchscreen IRQ is wakeup-capable?  I don't see any

No, it doesn't assume that.  For example, the system may wish to have
the chip initiate wakeup by bringing the AC97 bus up or have an output
from the WM97xx wired directly to some other component such as a PMU or
microcontroller rather than to an interrupt line visible to the CPU.

In theory this could also be used for some purpose other than waking the
system though I'm not aware of any such users so I don't think that's
worth worrying about right now.  In any case, the default is to enable
wakeup so most users won't notice this addition anyway.

> calls to enable_irq_wake() or disable_irq_wake() ... or for that
> matter any other calls that might affect system behavior.

Some combination of the machine-specific driver and other platform code
is expected to do this where required.

> > > +	/* WM97XX_PRP value to configure while system is suspended */
> > > +	u16 suspend_mode;

> I'm more accustomed to thinking that the driver will
> know "the" setup bit values to assign which will
> ensure that the chip issues wakeup events ... that
> stuff is usually not board-specific.

Unfortunately the fact that there are many different ways for the chip
to be used in a system (none of which can be automatically discovered)
means that this does need to be board-specific.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists