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-input" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
