Re: [PATCH] ACPI: Platform driver to support App Hot Startup (PNP0C32)

Ângelo Miguel Arrifano
2008-03-28T19:26:08Z

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On Fri, 28 Mar 2008 09:36:55 +0800
Shaohua Li <shaohua.li@intel.com> wrote:

>
> On Thu, 2008-03-27 at 16:03 +0000, Ângelo Miguel Arrifano wrote:
> > On Thu, 27 Mar 2008 10:30:13 +0800
> > Shaohua Li <shaohua.li@intel.com> wrote:
> >
> > >
> > > On Thu, 2008-03-27 at 01:51 +0800, Ângelo Miguel Arrifano wrote:
> > > > PATCH
> > > > - -----------------------------------------------------------------
> > > > diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> > > > index f688c21..fb096ee 100644
> > > > - --- a/drivers/acpi/Kconfig
> > > > +++ b/drivers/acpi/Kconfig
> > > > @@ -196,6 +196,14 @@ config ACPI_THERMAL
> > > > recommended that this option be enabled, as your
> > > > processor(s)
> > > > may be damaged without it.
> > > >
> > > > +config ACPI_QUICKSTART
> > > > + tristate "Quickstart"
> > > > + default y
> > > default m?
> > >
> > >
> > > > +
> > > > +static struct quickstart_driver_data {
> > > > + struct quickstart_btn *btn_lst;
> > > > + struct quickstart_btn *pressed;
> > > > +} quickstart_data = {
> > > > + .btn_lst = NULL,
> > > > + .pressed = NULL,
> > > > +};
> > > They are NULL, you don't need initialize them.
> > >
> > >
> > > > + * Platform driver structs
> > > > + */
> > > > +static ssize_t buttons_show(struct device *dev,
> > > > + struct device_attribute *attr,
> > > > + char *buf);
> > > > +static ssize_t pressed_button_show(struct device *dev,
> > > > + struct device_attribute *attr,
> > > > + char *buf);
> > > > +static ssize_t pressed_button_store(struct device *dev,
> > > > + struct device_attribute *attr,
> > > > + const char *buf,
> > > > + size_t count);
> > > > +static DEVICE_ATTR(pressed_button, 0666, pressed_button_show,
> > > > + pressed_button_store);
> > > this file can be changed by non-root?
> >
> > The only thing that writing to this file does is clearing the
> > pressed button.
> > Is it a bad idea that can be changed by non-root?
> Just abnormal, eg. non-root can clear it but root might not read it
> (though it doesn't happen in current case but possible if runtime event
> support).
>
> > > > +
> > > > +/* ACPI Driver functions */
> > > > +static void quickstart_acpi_notify(acpi_handle handle, u32 event,
> > > > void *data)
> > > > +{
> > > > + struct quickstart_acpi *quickstart = data;
> > > > +
> > > > + if (!quickstart)
> > > > + return;
> > > > +
> > > > + if (event == QUICKSTART_EVENT_WAKE) {
> > > > + quickstart_data.pressed = quickstart->btn;
> > > > + printk(KERN_ERR "quickstart: Quickbutton %s
> > > > pressed.\n",
> > > > +
> > > > acpi_device_bid(quickstart->device));
> > > > + } else if (event == QUICKSTART_EVENT_RUNTIME) {
> > > > + printk(KERN_ERR "quickstart: Runtime button %s
> > > > pressed.\n\t"
> > > > + "please report on linux-acpi@vger.kernel.org",
> > > > +
> > > > acpi_device_bid(quickstart->device));
> > > > + }
> > > Please remove above printks. they are misleading, which isn't an error.
> > > please send an ACPI event to userspace in the notify, we need it for
> > > runtime.
> >
> > According to the Microsoft paper, runtime hot button reporting is optional.
> > My laptop doesn't report them and on the SF.net project page there was no
> > reports of people with hardware supporting this.
> > Sorry, I don't have a way to test this..
> Did you tried suspend/resume. From the doc, resume can send a
> notification too. Just calling acpi event eject routines, an event eject
> model can be well integrated into existing acpi daemons.

Yes, suspend to mem works.
When the laptop is in suspend state, if DVD button is pressed the laptop resumes
and the platform driver receives a notify. The driver then exports the
button name to the sysfs pressed_button file.

>
> > > > + }
> > > I'd suggest the GHID info should be exported to sysfs too. It's hard to
> > > judge what a button is just per its name. You know it but an
> > > inexperienced user doesn't.
> >
> > I also would like to have a more descriptive information about the button but
> > the returned buffer is a BYTE/WORD/DWORD with a decimal number telling the button
> > usage ID.
> >
> > Looking at my DSDT piece of code:
> >
> > Method (GHID, 0, NotSerialized)
> > {
> > If (LEqual (HOTB, 0x05))
> > {
> > Notify (DBTN, 0x02)
> > Store (0x00, HOTB)
> > }
> >
> > Return (Buffer (0x01)
> > {
> > /* 0000 */ 0x04
> > })
> > }
> >
> > IMHO, a value like 0x04 for button usageID is totally useless..
> Ok, I got it. Maybe we need tool to help user identify what purpose a
> button is, but sounds impossible till the button can trigger runtime
> event.

I've noticed that HP laptops have the same names for button
devices (at least dv6k, dv8k and tx series). Maybe other vendors do this as well?
If this holds true, maybe building a vendor table for getting
button name -> description correspondence? .. just an idea ..

>
> Thanks,
> Shaohua
>

Thanks all,
- --
Angelo Arrifano AKA MiKNiX
CSE Student at UBI, Portugal
Gentoo Linux AMD64 Arch Tester
Linwizard Developer
http://miknix.homelinux.com
PGP Pubkey 0x3D92BB0B

- - -
Computers are like air conditioners. Both stop working, if you open windows.
-- Adam Heath
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.7 (GNU/Linux)

iD8DBQFH7UlnNahyoD2SuwsRApPtAJ9GcFwvwuAgCyCnS87XaGI+nHAbYgCeM65K
lvOIGf01TPFzAgqFbSoFJYY=
=vrGR
-----END PGP SIGNATURE-----