| Message ID | 20220922182424.934340-2-lkml@vorpal.se (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Quickstart buttons driver and Toshiba Z830 | expand |
Hi 2022. szeptember 22., csütörtök 20:24 keltezéssel, Arvid Norlander írta: > This is loosely based on a previous staging driver that was removed. See > links below for more info on that driver. The original commit ID was > 0be013e3dc2ee79ffab8a438bbb4e216837e3d52. > > However, here a completely different approach is taken to the user space > API (which should solve the issues the original driver had). Each PNP0C32 > device is a button, and each such button gets a separate input device > associated with it (instead of a shared platform input device). > > The button ID (as read from ACPI method GHID) is provided via a sysfs file > "button_id". > > If the button caused a wakeup it will "latch" the "wakeup_cause" sysfs file > to true. This can be reset by a user space process. > > Link: https://marc.info/?l=linux-acpi&m=120550727131007 > Link: https://lkml.org/lkml/2010/5/28/327 > Signed-off-by: Arvid Norlander <lkml@vorpal.se> > --- > [...] > diff --git a/drivers/platform/x86/quickstart.c b/drivers/platform/x86/quickstart.c > new file mode 100644 > index 000000000000..ce51abe012f7 > --- /dev/null > +++ b/drivers/platform/x86/quickstart.c > @@ -0,0 +1,320 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * quickstart.c - ACPI Direct App Launch driver > + * > + * Copyright (C) 2022 Arvid Norlander <lkml@vorapal.se> > + * Copyright (C) 2007-2010 Angelo Arrifano <miknix@gmail.com> > + * > + * Information gathered from disassembled dsdt and from here: > + * <https://archive.org/details/microsoft-acpi-dirapplaunch> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + */ > + > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > + > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/init.h> > +#include <linux/types.h> > +#include <linux/acpi.h> > +#include <linux/platform_device.h> > +#include <linux/input.h> > +#include <linux/input/sparse-keymap.h> > +#include <asm/unaligned.h> > + > +MODULE_AUTHOR("Arvid Norlander <lkml@vorpal.se>"); > +MODULE_AUTHOR("Angelo Arrifano"); > +MODULE_DESCRIPTION("ACPI Direct App Launch driver"); > +MODULE_LICENSE("GPL"); > + > +#define QUICKSTART_ACPI_DEVICE_NAME "quickstart" > +#define QUICKSTART_ACPI_HID "PNP0C32" > + > +/* > + * There will be two events: > + * 0x02 - A hot button was pressed while device was off/sleeping. > + * 0x80 - A hot button was pressed while device was up. > + */ > +#define QUICKSTART_EVENT_WAKE 0x02 > +#define QUICKSTART_EVENT_RUNTIME 0x80 > + > +/* > + * Each PNP0C32 device is an individual button. This structure > + * keeps track of data associated with said device. > + */ > +struct quickstart_acpi { > + struct platform_device *platform_dev; > + struct input_dev *input_device; > + struct quickstart_button *button; > + /* ID of button as returned by GHID */ > + u32 id; > + /* Name of input device */ > + char input_name[32]; > + /* Physical path for the input device */ > + char phys[32]; > + /* Track if a wakeup event was received */ > + bool wakeup_cause; > +}; > + > +#define quickstart_name(dev) acpi_device_bid(dev->acpi_dev) This does not seem to be used. > + > +/* > + * Knowing what these buttons do require system specific knowledge. > + * This could be done by matching on DMI data in a long quirk table. > + * However, it is easier to leave it up to user space to figure this out. > + * > + * Using for example udev hwdb the scancode 0x1 can be remapped suitably. > + */ > +static const struct key_entry quickstart_keymap[] = { > + { KE_KEY, 0x1, { KEY_UNKNOWN } }, > + { KE_END, 0 }, > +}; > + > +static ssize_t wakeup_cause_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct quickstart_acpi *quickstart = dev_get_drvdata(dev); > + > + return sysfs_emit(buf, "%s\n", > + (quickstart->wakeup_cause ? "true" : "false")); > +} > + > +static ssize_t wakeup_cause_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct quickstart_acpi *quickstart = dev_get_drvdata(dev); > + > + if (count < 2) > + return -EINVAL; > + > + if (strncasecmp(buf, "false", 4) != 0) > + return -EINVAL; > + If "true"/"false" will be used in the final version, then I think this check currently is too lax. You could use `sysfs_streq()`. And I think the `count < 2` check is not needed. > + quickstart->wakeup_cause = false; > + return count; > +} > +static DEVICE_ATTR_RW(wakeup_cause); > + > +static ssize_t button_id_show(struct device *dev, struct device_attribute *attr, > + char *buf) > +{ > + struct quickstart_acpi *quickstart = dev_get_drvdata(dev); > + > + return sysfs_emit(buf, "%u\n", quickstart->id); > +} > +static DEVICE_ATTR_RO(button_id); > + > +/* ACPI Driver functions */ > +static void quickstart_acpi_notify(acpi_handle handle, u32 event, void *context) > +{ > + struct platform_device *device = context; > + struct quickstart_acpi *quickstart = dev_get_drvdata(&device->dev); > + > + if (!quickstart) > + return; > + > + switch (event) { > + case QUICKSTART_EVENT_WAKE: > + quickstart->wakeup_cause = true; > + break; > + case QUICKSTART_EVENT_RUNTIME: > + if (!sparse_keymap_report_event(quickstart->input_device, 0x1, > + 1, true)) { > + pr_info("Key handling error\n"); I don't think this branch can ever be taken. > + } > + break; > + default: > + pr_err("Unexpected ACPI event notify (%u)\n", event); I think `dev_{err,info}()` should be preferred. > + break; > + } > +} > + > +/* > + * The GHID ACPI method is used to indicate the "role" of the button. > + * However, all the meanings of these values are vendor defined. > + * > + * We do however expose this value to user space. > + */ > +static int quickstart_acpi_ghid(struct quickstart_acpi *quickstart) > +{ > + acpi_handle handle = ACPI_HANDLE(&quickstart->platform_dev->dev); > + acpi_status status; > + struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL }; > + int ret = 0; > + union acpi_object *obj = NULL; > + > + /* > + * This returns a buffer telling the button usage ID, > + * and triggers pending notify events (The ones before booting). > + */ > + status = acpi_evaluate_object(handle, "GHID", NULL, &buffer); > + if (ACPI_FAILURE(status)) { > + dev_err(&quickstart->platform_dev->dev, > + "GHID method failed, ACPI status %u\n", status); > + return -EINVAL; > + } > + obj = buffer.pointer; > + > + /* > + * GHID returns buffers, sanity check that is the case. > + */ > + if (obj->type != ACPI_TYPE_BUFFER) { > + dev_err(&quickstart->platform_dev->dev, > + "GHID did not return buffer\n"); > + ret = -EINVAL; > + goto free_and_return; > + } > + > + /* > + * Quoting the specification: > + * "The GHID method can return a BYTE, WORD, or DWORD. > + * The value must be encoded in little-endian byte > + * order (least significant byte first)." > + */ > + switch (obj->buffer.length) { > + case 1: > + quickstart->id = *(u8 *)obj->buffer.pointer; > + break; > + case 2: > + quickstart->id = get_unaligned_le16(obj->buffer.pointer); > + break; > + case 4: > + quickstart->id = get_unaligned_le32(obj->buffer.pointer); > + break; > + case 8: > + quickstart->id = get_unaligned_le64(obj->buffer.pointer); > + break; > + default: > + dev_err(&quickstart->platform_dev->dev, > + "GHID method returned buffer of unexpected length %lu\n", > + (unsigned long)obj->buffer.length); > + ret = -EINVAL; > + break; > + } > + > +free_and_return: > + kfree(buffer.pointer); > + > + return ret; > +} > + > +static struct attribute *quickstart_attributes[] = { > + &dev_attr_wakeup_cause.attr, > + &dev_attr_button_id.attr, > + NULL, > +}; > + > +static const struct attribute_group quickstart_attr_group = { > + .attrs = quickstart_attributes, > +}; > + > +static int quickstart_remove(struct platform_device *device) > +{ > + acpi_handle handle = ACPI_HANDLE(&device->dev); > + > + acpi_remove_notify_handler(handle, ACPI_DEVICE_NOTIFY, > + quickstart_acpi_notify); > + > + return 0; > +} > + > +static int quickstart_probe(struct platform_device *device) > +{ > + int ret; > + acpi_handle handle = ACPI_HANDLE(&device->dev); > + acpi_status status; > + struct quickstart_acpi *quickstart; > + > + if (!device) > + return -EINVAL; > + > + quickstart = > + devm_kzalloc(&device->dev, sizeof(*quickstart), GFP_KERNEL); > + if (!quickstart) > + return -ENOMEM; > + > + /* > + * This must be set early for proper cleanup on error handling path. > + * After this point generic error handling can be used. > + */ > + quickstart->platform_dev = device; > + dev_set_drvdata(&device->dev, quickstart); > + > + /* Retrieve the GHID ID */ > + ret = quickstart_acpi_ghid(quickstart); > + if (ret < 0) > + goto error; You can replace all `goto`s in this function with `return`s. In fact, you should because `quickstart_remove()` does not do useful work until the ACPI notify handler is registered, but if that succeeds, this function can no longer fail. > + > + /* Set up sysfs entries */ > + ret = devm_device_add_group(&quickstart->platform_dev->dev, > + &quickstart_attr_group); In the meantime I realized there is a simpler solution. Use the `ATTRIBUTE_GROUPS()` macro and then simply set the `.dev_groups` member of `quickstart_platform_driver.driver`. (see drivers/platform/x86/hp-wmi.c) > + if (ret) { > + dev_err(&device->dev, "Unable to setup sysfs entries\n"); > + goto error; > + } > + > + /* Set up input device */ > + quickstart->input_device = > + devm_input_allocate_device(&quickstart->platform_dev->dev); > + if (!quickstart->input_device) { > + ret = -ENOMEM; > + goto error; > + } > + ret = sparse_keymap_setup(quickstart->input_device, quickstart_keymap, > + NULL); > + if (ret) > + goto error; > + > + snprintf(quickstart->input_name, sizeof(quickstart->phys), > + "Quickstart Button %u", quickstart->id); > + snprintf(quickstart->phys, sizeof(quickstart->phys), > + QUICKSTART_ACPI_DEVICE_NAME "/input%u", quickstart->id); > + > + quickstart->input_device->name = quickstart->input_name; > + quickstart->input_device->phys = quickstart->phys; > + quickstart->input_device->id.bustype = BUS_HOST; > + > + ret = input_register_device(quickstart->input_device); > + > + /* Set up notify handler */ > + status = acpi_install_notify_handler(handle, ACPI_DEVICE_NOTIFY, > + quickstart_acpi_notify, device); > + if (ACPI_FAILURE(status)) { > + dev_err(&device->dev, "Error installing notify handler\n"); > + return -EIO; > + } > + > + return 0; > +error: > + quickstart_remove(device); > + return ret; > +} > + > +static const struct acpi_device_id quickstart_device_ids[] = { > + { QUICKSTART_ACPI_HID, 0 }, > + { "", 0 }, Small thing, but usually the comma after the sentinel entry is omitted. (see quickstart_keymap, quickstart_attributes as well) > +}; > +MODULE_DEVICE_TABLE(acpi, quickstart_device_ids); > + > +static struct platform_driver quickstart_platform_driver = { > + .probe = quickstart_probe, > + .remove = quickstart_remove, > + .driver = { > + .name = QUICKSTART_ACPI_DEVICE_NAME, > + .acpi_match_table = quickstart_device_ids, > + .owner = THIS_MODULE, > + } > +}; > + > +module_platform_driver(quickstart_platform_driver); > -- > 2.37.3 Regards, Barnabás Pőcze
Hi, Thank you, I have incorperated your feedback in my local branch. On 2022-09-23 21:24, Barnabás Pőcze wrote: > Hi > > 2022. szeptember 22., csütörtök 20:24 keltezéssel, Arvid Norlander írta: > >> This is loosely based on a previous staging driver that was removed. See >> links below for more info on that driver. The original commit ID was >> 0be013e3dc2ee79ffab8a438bbb4e216837e3d52. >> >> However, here a completely different approach is taken to the user space >> API (which should solve the issues the original driver had). Each PNP0C32 >> device is a button, and each such button gets a separate input device >> associated with it (instead of a shared platform input device). >> >> The button ID (as read from ACPI method GHID) is provided via a sysfs file >> "button_id". >> >> If the button caused a wakeup it will "latch" the "wakeup_cause" sysfs file >> to true. This can be reset by a user space process. >> >> Link: https://marc.info/?l=linux-acpi&m=120550727131007 >> Link: https://lkml.org/lkml/2010/5/28/327 >> Signed-off-by: Arvid Norlander <lkml@vorpal.se> >> --- >> [...] >> diff --git a/drivers/platform/x86/quickstart.c b/drivers/platform/x86/quickstart.c >> new file mode 100644 >> index 000000000000..ce51abe012f7 >> --- /dev/null >> +++ b/drivers/platform/x86/quickstart.c >> @@ -0,0 +1,320 @@ <snip> >> + >> +static ssize_t wakeup_cause_store(struct device *dev, >> + struct device_attribute *attr, >> + const char *buf, size_t count) >> +{ >> + struct quickstart_acpi *quickstart = dev_get_drvdata(dev); >> + >> + if (count < 2) >> + return -EINVAL; >> + >> + if (strncasecmp(buf, "false", 4) != 0) >> + return -EINVAL; >> + > > If "true"/"false" will be used in the final version, then I think this check > currently is too lax. You could use `sysfs_streq()`. And I think the `count < 2` > check is not needed. Regarding the user space API I don't know, that is one of the open questions in the cover letter. I have yet to get any feedback on any of those questions. That is something that needs to happen before this driver can be included. I would appreciate your feedback on those. <snip> > > Regards, > Barnabás Pőcze Regards, Arvid Norlander
Hi, On 9/25/22 20:19, Arvid Norlander wrote: > Hi, > > Thank you, I have incorperated your feedback in my local branch. > > On 2022-09-23 21:24, Barnabás Pőcze wrote: >> Hi >> >> 2022. szeptember 22., csütörtök 20:24 keltezéssel, Arvid Norlander írta: >> >>> This is loosely based on a previous staging driver that was removed. See >>> links below for more info on that driver. The original commit ID was >>> 0be013e3dc2ee79ffab8a438bbb4e216837e3d52. >>> >>> However, here a completely different approach is taken to the user space >>> API (which should solve the issues the original driver had). Each PNP0C32 >>> device is a button, and each such button gets a separate input device >>> associated with it (instead of a shared platform input device). >>> >>> The button ID (as read from ACPI method GHID) is provided via a sysfs file >>> "button_id". >>> >>> If the button caused a wakeup it will "latch" the "wakeup_cause" sysfs file >>> to true. This can be reset by a user space process. >>> >>> Link: https://marc.info/?l=linux-acpi&m=120550727131007 >>> Link: https://lkml.org/lkml/2010/5/28/327 >>> Signed-off-by: Arvid Norlander <lkml@vorpal.se> >>> --- >>> [...] >>> diff --git a/drivers/platform/x86/quickstart.c b/drivers/platform/x86/quickstart.c >>> new file mode 100644 >>> index 000000000000..ce51abe012f7 >>> --- /dev/null >>> +++ b/drivers/platform/x86/quickstart.c >>> @@ -0,0 +1,320 @@ > > <snip> > >>> + >>> +static ssize_t wakeup_cause_store(struct device *dev, >>> + struct device_attribute *attr, >>> + const char *buf, size_t count) >>> +{ >>> + struct quickstart_acpi *quickstart = dev_get_drvdata(dev); >>> + >>> + if (count < 2) >>> + return -EINVAL; >>> + >>> + if (strncasecmp(buf, "false", 4) != 0) >>> + return -EINVAL; >>> + >> >> If "true"/"false" will be used in the final version, then I think this check >> currently is too lax. You could use `sysfs_streq()`. And I think the `count < 2` >> check is not needed. > > Regarding the user space API I don't know, that is one of the open > questions in the cover letter. I have yet to get any feedback on any of > those questions. That is something that needs to happen before this driver > can be included. I would appreciate your feedback on those. I will reply to this question in my general review of the driver. Regards, Hans > > <snip> > >> >> Regards, >> Barnabás Pőcze > > Regards, > Arvid Norlander >
Hi, On 9/25/22 20:19, Arvid Norlander wrote: > Hi, > > Thank you, I have incorperated your feedback in my local branch. > > On 2022-09-23 21:24, Barnabás Pőcze wrote: >> Hi >> >> 2022. szeptember 22., csütörtök 20:24 keltezéssel, Arvid Norlander írta: >> >>> This is loosely based on a previous staging driver that was removed. See >>> links below for more info on that driver. The original commit ID was >>> 0be013e3dc2ee79ffab8a438bbb4e216837e3d52. >>> >>> However, here a completely different approach is taken to the user space >>> API (which should solve the issues the original driver had). Each PNP0C32 >>> device is a button, and each such button gets a separate input device >>> associated with it (instead of a shared platform input device). >>> >>> The button ID (as read from ACPI method GHID) is provided via a sysfs file >>> "button_id". >>> >>> If the button caused a wakeup it will "latch" the "wakeup_cause" sysfs file >>> to true. This can be reset by a user space process. >>> >>> Link: https://marc.info/?l=linux-acpi&m=120550727131007 >>> Link: https://lkml.org/lkml/2010/5/28/327 >>> Signed-off-by: Arvid Norlander <lkml@vorpal.se> >>> --- >>> [...] >>> diff --git a/drivers/platform/x86/quickstart.c b/drivers/platform/x86/quickstart.c >>> new file mode 100644 >>> index 000000000000..ce51abe012f7 >>> --- /dev/null >>> +++ b/drivers/platform/x86/quickstart.c >>> @@ -0,0 +1,320 @@ > > <snip> > >>> + >>> +static ssize_t wakeup_cause_store(struct device *dev, >>> + struct device_attribute *attr, >>> + const char *buf, size_t count) >>> +{ >>> + struct quickstart_acpi *quickstart = dev_get_drvdata(dev); >>> + >>> + if (count < 2) >>> + return -EINVAL; >>> + >>> + if (strncasecmp(buf, "false", 4) != 0) >>> + return -EINVAL; >>> + >> >> If "true"/"false" will be used in the final version, then I think this check >> currently is too lax. You could use `sysfs_streq()`. And I think the `count < 2` >> check is not needed. > > Regarding the user space API I don't know, that is one of the open > questions in the cover letter. I have yet to get any feedback on any of > those questions. That is something that needs to happen before this driver > can be included. I would appreciate your feedback on those. I will reply to this question in my general review of the driver. Regards, Hans > > <snip> > >> >> Regards, >> Barnabás Pőcze > > Regards, > Arvid Norlander >
Hi, On 9/23/22 21:24, Barnabás Pőcze wrote: > Hi > > 2022. szeptember 22., csütörtök 20:24 keltezéssel, Arvid Norlander írta: > >> This is loosely based on a previous staging driver that was removed. See >> links below for more info on that driver. The original commit ID was >> 0be013e3dc2ee79ffab8a438bbb4e216837e3d52. >> >> However, here a completely different approach is taken to the user space >> API (which should solve the issues the original driver had). Each PNP0C32 >> device is a button, and each such button gets a separate input device >> associated with it (instead of a shared platform input device). >> >> The button ID (as read from ACPI method GHID) is provided via a sysfs file >> "button_id". >> >> If the button caused a wakeup it will "latch" the "wakeup_cause" sysfs file >> to true. This can be reset by a user space process. >> >> Link: https://marc.info/?l=linux-acpi&m=120550727131007 >> Link: https://lkml.org/lkml/2010/5/28/327 >> Signed-off-by: Arvid Norlander <lkml@vorpal.se> >> --- >> [...] >> diff --git a/drivers/platform/x86/quickstart.c b/drivers/platform/x86/quickstart.c >> new file mode 100644 >> index 000000000000..ce51abe012f7 >> --- /dev/null >> +++ b/drivers/platform/x86/quickstart.c >> @@ -0,0 +1,320 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * quickstart.c - ACPI Direct App Launch driver >> + * >> + * Copyright (C) 2022 Arvid Norlander <lkml@vorapal.se> >> + * Copyright (C) 2007-2010 Angelo Arrifano <miknix@gmail.com> >> + * >> + * Information gathered from disassembled dsdt and from here: >> + * <https://archive.org/details/microsoft-acpi-dirapplaunch> >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License as published by >> + * the Free Software Foundation; either version 2 of the License, or >> + * (at your option) any later version. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + * >> + */ >> + >> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt >> + >> +#include <linux/kernel.h> >> +#include <linux/module.h> >> +#include <linux/init.h> >> +#include <linux/types.h> >> +#include <linux/acpi.h> >> +#include <linux/platform_device.h> >> +#include <linux/input.h> >> +#include <linux/input/sparse-keymap.h> >> +#include <asm/unaligned.h> >> + >> +MODULE_AUTHOR("Arvid Norlander <lkml@vorpal.se>"); >> +MODULE_AUTHOR("Angelo Arrifano"); >> +MODULE_DESCRIPTION("ACPI Direct App Launch driver"); >> +MODULE_LICENSE("GPL"); >> + >> +#define QUICKSTART_ACPI_DEVICE_NAME "quickstart" >> +#define QUICKSTART_ACPI_HID "PNP0C32" >> + >> +/* >> + * There will be two events: >> + * 0x02 - A hot button was pressed while device was off/sleeping. >> + * 0x80 - A hot button was pressed while device was up. >> + */ >> +#define QUICKSTART_EVENT_WAKE 0x02 >> +#define QUICKSTART_EVENT_RUNTIME 0x80 >> + >> +/* >> + * Each PNP0C32 device is an individual button. This structure >> + * keeps track of data associated with said device. >> + */ >> +struct quickstart_acpi { >> + struct platform_device *platform_dev; >> + struct input_dev *input_device; >> + struct quickstart_button *button; >> + /* ID of button as returned by GHID */ >> + u32 id; >> + /* Name of input device */ >> + char input_name[32]; >> + /* Physical path for the input device */ >> + char phys[32]; >> + /* Track if a wakeup event was received */ >> + bool wakeup_cause; >> +}; >> + >> +#define quickstart_name(dev) acpi_device_bid(dev->acpi_dev) > > This does not seem to be used. > > >> + >> +/* >> + * Knowing what these buttons do require system specific knowledge. >> + * This could be done by matching on DMI data in a long quirk table. >> + * However, it is easier to leave it up to user space to figure this out. >> + * >> + * Using for example udev hwdb the scancode 0x1 can be remapped suitably. >> + */ >> +static const struct key_entry quickstart_keymap[] = { >> + { KE_KEY, 0x1, { KEY_UNKNOWN } }, >> + { KE_END, 0 }, >> +}; >> + >> +static ssize_t wakeup_cause_show(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + struct quickstart_acpi *quickstart = dev_get_drvdata(dev); >> + >> + return sysfs_emit(buf, "%s\n", >> + (quickstart->wakeup_cause ? "true" : "false")); >> +} >> + >> +static ssize_t wakeup_cause_store(struct device *dev, >> + struct device_attribute *attr, >> + const char *buf, size_t count) >> +{ >> + struct quickstart_acpi *quickstart = dev_get_drvdata(dev); >> + >> + if (count < 2) >> + return -EINVAL; >> + >> + if (strncasecmp(buf, "false", 4) != 0) >> + return -EINVAL; >> + > > If "true"/"false" will be used in the final version, then I think this check > currently is too lax. You could use `sysfs_streq()`. And I think the `count < 2` > check is not needed. > > >> + quickstart->wakeup_cause = false; >> + return count; >> +} >> +static DEVICE_ATTR_RW(wakeup_cause); >> + >> +static ssize_t button_id_show(struct device *dev, struct device_attribute *attr, >> + char *buf) >> +{ >> + struct quickstart_acpi *quickstart = dev_get_drvdata(dev); >> + >> + return sysfs_emit(buf, "%u\n", quickstart->id); >> +} >> +static DEVICE_ATTR_RO(button_id); >> + >> +/* ACPI Driver functions */ >> +static void quickstart_acpi_notify(acpi_handle handle, u32 event, void *context) >> +{ >> + struct platform_device *device = context; >> + struct quickstart_acpi *quickstart = dev_get_drvdata(&device->dev); >> + >> + if (!quickstart) >> + return; >> + >> + switch (event) { >> + case QUICKSTART_EVENT_WAKE: >> + quickstart->wakeup_cause = true; >> + break; >> + case QUICKSTART_EVENT_RUNTIME: >> + if (!sparse_keymap_report_event(quickstart->input_device, 0x1, >> + 1, true)) { >> + pr_info("Key handling error\n"); > > I don't think this branch can ever be taken. True, might just as well drop the if and simply call sparse_keymap_report_event() without checking the result. > > >> + } >> + break; >> + default: >> + pr_err("Unexpected ACPI event notify (%u)\n", event); > > I think `dev_{err,info}()` should be preferred. > > >> + break; >> + } >> +} >> + >> +/* >> + * The GHID ACPI method is used to indicate the "role" of the button. >> + * However, all the meanings of these values are vendor defined. >> + * >> + * We do however expose this value to user space. >> + */ >> +static int quickstart_acpi_ghid(struct quickstart_acpi *quickstart) >> +{ >> + acpi_handle handle = ACPI_HANDLE(&quickstart->platform_dev->dev); >> + acpi_status status; >> + struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL }; >> + int ret = 0; >> + union acpi_object *obj = NULL; >> + >> + /* >> + * This returns a buffer telling the button usage ID, >> + * and triggers pending notify events (The ones before booting). >> + */ >> + status = acpi_evaluate_object(handle, "GHID", NULL, &buffer); >> + if (ACPI_FAILURE(status)) { >> + dev_err(&quickstart->platform_dev->dev, >> + "GHID method failed, ACPI status %u\n", status); >> + return -EINVAL; >> + } >> + obj = buffer.pointer; >> + >> + /* >> + * GHID returns buffers, sanity check that is the case. >> + */ >> + if (obj->type != ACPI_TYPE_BUFFER) { >> + dev_err(&quickstart->platform_dev->dev, >> + "GHID did not return buffer\n"); >> + ret = -EINVAL; >> + goto free_and_return; >> + } >> + >> + /* >> + * Quoting the specification: >> + * "The GHID method can return a BYTE, WORD, or DWORD. >> + * The value must be encoded in little-endian byte >> + * order (least significant byte first)." >> + */ >> + switch (obj->buffer.length) { >> + case 1: >> + quickstart->id = *(u8 *)obj->buffer.pointer; >> + break; >> + case 2: >> + quickstart->id = get_unaligned_le16(obj->buffer.pointer); >> + break; >> + case 4: >> + quickstart->id = get_unaligned_le32(obj->buffer.pointer); >> + break; >> + case 8: >> + quickstart->id = get_unaligned_le64(obj->buffer.pointer); >> + break; >> + default: >> + dev_err(&quickstart->platform_dev->dev, >> + "GHID method returned buffer of unexpected length %lu\n", >> + (unsigned long)obj->buffer.length); >> + ret = -EINVAL; >> + break; >> + } >> + >> +free_and_return: >> + kfree(buffer.pointer); >> + >> + return ret; >> +} >> + >> +static struct attribute *quickstart_attributes[] = { >> + &dev_attr_wakeup_cause.attr, >> + &dev_attr_button_id.attr, >> + NULL, >> +}; >> + >> +static const struct attribute_group quickstart_attr_group = { >> + .attrs = quickstart_attributes, >> +}; >> + >> +static int quickstart_remove(struct platform_device *device) >> +{ >> + acpi_handle handle = ACPI_HANDLE(&device->dev); >> + >> + acpi_remove_notify_handler(handle, ACPI_DEVICE_NOTIFY, >> + quickstart_acpi_notify); >> + >> + return 0; >> +} >> + >> +static int quickstart_probe(struct platform_device *device) >> +{ >> + int ret; >> + acpi_handle handle = ACPI_HANDLE(&device->dev); >> + acpi_status status; >> + struct quickstart_acpi *quickstart; >> + >> + if (!device) >> + return -EINVAL; >> + >> + quickstart = >> + devm_kzalloc(&device->dev, sizeof(*quickstart), GFP_KERNEL); >> + if (!quickstart) >> + return -ENOMEM; >> + >> + /* >> + * This must be set early for proper cleanup on error handling path. >> + * After this point generic error handling can be used. >> + */ >> + quickstart->platform_dev = device; >> + dev_set_drvdata(&device->dev, quickstart); >> + >> + /* Retrieve the GHID ID */ >> + ret = quickstart_acpi_ghid(quickstart); >> + if (ret < 0) >> + goto error; > > You can replace all `goto`s in this function with `return`s. In fact, you should > because `quickstart_remove()` does not do useful work until the ACPI notify handler > is registered, but if that succeeds, this function can no longer fail. > > >> + >> + /* Set up sysfs entries */ >> + ret = devm_device_add_group(&quickstart->platform_dev->dev, >> + &quickstart_attr_group); > > In the meantime I realized there is a simpler solution. Use the `ATTRIBUTE_GROUPS()` > macro and then simply set the `.dev_groups` member of `quickstart_platform_driver.driver`. > (see drivers/platform/x86/hp-wmi.c) Ack yes please, using .dev_groups is greatly preferred. > > >> + if (ret) { >> + dev_err(&device->dev, "Unable to setup sysfs entries\n"); >> + goto error; >> + } >> + >> + /* Set up input device */ >> + quickstart->input_device = >> + devm_input_allocate_device(&quickstart->platform_dev->dev); >> + if (!quickstart->input_device) { >> + ret = -ENOMEM; >> + goto error; >> + } >> + ret = sparse_keymap_setup(quickstart->input_device, quickstart_keymap, >> + NULL); >> + if (ret) >> + goto error; >> + >> + snprintf(quickstart->input_name, sizeof(quickstart->phys), >> + "Quickstart Button %u", quickstart->id); >> + snprintf(quickstart->phys, sizeof(quickstart->phys), >> + QUICKSTART_ACPI_DEVICE_NAME "/input%u", quickstart->id); >> + >> + quickstart->input_device->name = quickstart->input_name; >> + quickstart->input_device->phys = quickstart->phys; >> + quickstart->input_device->id.bustype = BUS_HOST; >> + >> + ret = input_register_device(quickstart->input_device); >> + >> + /* Set up notify handler */ >> + status = acpi_install_notify_handler(handle, ACPI_DEVICE_NOTIFY, >> + quickstart_acpi_notify, device); >> + if (ACPI_FAILURE(status)) { >> + dev_err(&device->dev, "Error installing notify handler\n"); >> + return -EIO; >> + } >> + >> + return 0; >> +error: >> + quickstart_remove(device); >> + return ret; >> +} >> + >> +static const struct acpi_device_id quickstart_device_ids[] = { >> + { QUICKSTART_ACPI_HID, 0 }, >> + { "", 0 }, > > Small thing, but usually the comma after the sentinel entry is omitted. > (see quickstart_keymap, quickstart_attributes as well) > > >> +}; >> +MODULE_DEVICE_TABLE(acpi, quickstart_device_ids); >> + >> +static struct platform_driver quickstart_platform_driver = { >> + .probe = quickstart_probe, >> + .remove = quickstart_remove, >> + .driver = { >> + .name = QUICKSTART_ACPI_DEVICE_NAME, >> + .acpi_match_table = quickstart_device_ids, >> + .owner = THIS_MODULE, >> + } >> +}; >> + >> +module_platform_driver(quickstart_platform_driver); >> -- >> 2.37.3 > > > Regards, > Barnabás Pőcze > Regards, Hans
Hi, On 9/22/22 20:24, Arvid Norlander wrote: > This is loosely based on a previous staging driver that was removed. See > links below for more info on that driver. The original commit ID was > 0be013e3dc2ee79ffab8a438bbb4e216837e3d52. > > However, here a completely different approach is taken to the user space > API (which should solve the issues the original driver had). Each PNP0C32 > device is a button, and each such button gets a separate input device > associated with it (instead of a shared platform input device). > > The button ID (as read from ACPI method GHID) is provided via a sysfs file > "button_id". > > If the button caused a wakeup it will "latch" the "wakeup_cause" sysfs file > to true. This can be reset by a user space process. > > Link: https://marc.info/?l=linux-acpi&m=120550727131007 > Link: https://lkml.org/lkml/2010/5/28/327 > Signed-off-by: Arvid Norlander <lkml@vorpal.se> > --- > drivers/platform/x86/Kconfig | 13 ++ > drivers/platform/x86/Makefile | 3 + > drivers/platform/x86/quickstart.c | 320 ++++++++++++++++++++++++++++++ > 3 files changed, 336 insertions(+) > create mode 100644 drivers/platform/x86/quickstart.c > > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig > index f5312f51de19..eebd70ef4a7c 100644 > --- a/drivers/platform/x86/Kconfig > +++ b/drivers/platform/x86/Kconfig > @@ -685,6 +685,18 @@ config THINKPAD_LMI > > source "drivers/platform/x86/intel/Kconfig" > > +config ACPI_QUICKSTART > + tristate "ACPI Quickstart key driver" > + depends on ACPI > + depends on INPUT > + select INPUT_SPARSEKMAP > + help > + Say Y here if you have a platform that supports the ACPI > + quickstart key protocol. > + > + To compile this driver as a module, choose M here: the module will be > + called quickstart. > + > config MSI_LAPTOP > tristate "MSI Laptop Extras" > depends on ACPI > @@ -803,6 +815,7 @@ config ACPI_TOSHIBA > depends on RFKILL || RFKILL = n > depends on IIO > select INPUT_SPARSEKMAP > + select ACPI_QUICKSTART > help > This driver adds support for access to certain system settings > on "legacy free" Toshiba laptops. These laptops can be recognized by > diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile > index 5a428caa654a..a8a8e1ddb2a3 100644 > --- a/drivers/platform/x86/Makefile > +++ b/drivers/platform/x86/Makefile > @@ -72,6 +72,9 @@ obj-$(CONFIG_THINKPAD_LMI) += think-lmi.o > # Intel > obj-y += intel/ > > +# Microsoft > +obj-$(CONFIG_ACPI_QUICKSTART) += quickstart.o > + > # MSI > obj-$(CONFIG_MSI_LAPTOP) += msi-laptop.o > obj-$(CONFIG_MSI_WMI) += msi-wmi.o > diff --git a/drivers/platform/x86/quickstart.c b/drivers/platform/x86/quickstart.c > new file mode 100644 > index 000000000000..ce51abe012f7 > --- /dev/null > +++ b/drivers/platform/x86/quickstart.c > @@ -0,0 +1,320 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * quickstart.c - ACPI Direct App Launch driver > + * > + * Copyright (C) 2022 Arvid Norlander <lkml@vorapal.se> > + * Copyright (C) 2007-2010 Angelo Arrifano <miknix@gmail.com> > + * > + * Information gathered from disassembled dsdt and from here: > + * <https://archive.org/details/microsoft-acpi-dirapplaunch> SPDX fully covers all the license stuff, so please drop the below part of this comment block: > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * And stop dropping the comment here (obviously). > + */ > + > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > + > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/init.h> > +#include <linux/types.h> > +#include <linux/acpi.h> > +#include <linux/platform_device.h> > +#include <linux/input.h> > +#include <linux/input/sparse-keymap.h> > +#include <asm/unaligned.h> > + > +MODULE_AUTHOR("Arvid Norlander <lkml@vorpal.se>"); > +MODULE_AUTHOR("Angelo Arrifano"); > +MODULE_DESCRIPTION("ACPI Direct App Launch driver"); > +MODULE_LICENSE("GPL"); > + > +#define QUICKSTART_ACPI_DEVICE_NAME "quickstart" > +#define QUICKSTART_ACPI_HID "PNP0C32" > + > +/* > + * There will be two events: > + * 0x02 - A hot button was pressed while device was off/sleeping. > + * 0x80 - A hot button was pressed while device was up. > + */ > +#define QUICKSTART_EVENT_WAKE 0x02 > +#define QUICKSTART_EVENT_RUNTIME 0x80 > + > +/* > + * Each PNP0C32 device is an individual button. This structure > + * keeps track of data associated with said device. > + */ > +struct quickstart_acpi { > + struct platform_device *platform_dev; Everywhere where this is used you use platform_dev->dev, instead please just store &platform_dev->dev in a struct device *dev here. > + struct input_dev *input_device; > + struct quickstart_button *button; > + /* ID of button as returned by GHID */ > + u32 id; > + /* Name of input device */ > + char input_name[32]; > + /* Physical path for the input device */ > + char phys[32]; > + /* Track if a wakeup event was received */ > + bool wakeup_cause; > +}; > + > +#define quickstart_name(dev) acpi_device_bid(dev->acpi_dev) > + > +/* > + * Knowing what these buttons do require system specific knowledge. > + * This could be done by matching on DMI data in a long quirk table. > + * However, it is easier to leave it up to user space to figure this out. > + * > + * Using for example udev hwdb the scancode 0x1 can be remapped suitably. > + */ > +static const struct key_entry quickstart_keymap[] = { > + { KE_KEY, 0x1, { KEY_UNKNOWN } }, > + { KE_END, 0 }, > +}; > + > +static ssize_t wakeup_cause_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct quickstart_acpi *quickstart = dev_get_drvdata(dev); > + > + return sysfs_emit(buf, "%s\n", > + (quickstart->wakeup_cause ? "true" : "false")); > +} > + > +static ssize_t wakeup_cause_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct quickstart_acpi *quickstart = dev_get_drvdata(dev); > + > + if (count < 2) > + return -EINVAL; > + > + if (strncasecmp(buf, "false", 4) != 0) > + return -EINVAL; > + > + quickstart->wakeup_cause = false; > + return count; > +} > +static DEVICE_ATTR_RW(wakeup_cause); I think it would be best here to just use the standard kernel boolean convention like how it is also used for bool type module parameters, then you would get something like this: static ssize_t wakeup_cause_show(struct device *dev, struct device_attribute *attr, char *buf) { struct quickstart_acpi *quickstart = dev_get_drvdata(dev); return sysfs_emit(buf, "%c\n", quickstart->wakeup_cause ? 'Y' : 'N'); } static ssize_t wakeup_cause_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t count) { struct quickstart_acpi *quickstart = dev_get_drvdata(dev); bool value; int ret; ret = kstrtobool(buf, &value); if (ret) return ret; /* Only allow clearing, not setting */ if (value) return -EINVAL; quickstart->wakeup_cause = false; return count; } static DEVICE_ATTR_RW(wakeup_cause); Note I have chosen to show 'Y' and 'N' as values when reading because that is what the kernel does when showing bool module parameters under /sys/modules/*/parameters/* But if you prefer to keep the true/false strings that is fine too. Note you should also add a Documentation/ABI/testing/sysfs-platform-PNP0C32 or Documentation/ABI/testing/sysfs-platform-quickstart file documenting the sysfs attributes. So that whatever you chose (Y/N vs true/false) is documented. I think that Documentation/ABI/testing/sysfs-platform-PNP0C32 will be easier to find for users since that is the name of the devices under /sys/bus/platform/devices, but either suggested name works for me. > +static ssize_t button_id_show(struct device *dev, struct device_attribute *attr, > + char *buf) > +{ > + struct quickstart_acpi *quickstart = dev_get_drvdata(dev); > + > + return sysfs_emit(buf, "%u\n", quickstart->id); > +} > +static DEVICE_ATTR_RO(button_id); > + > +/* ACPI Driver functions */ > +static void quickstart_acpi_notify(acpi_handle handle, u32 event, void *context) > +{ > + struct platform_device *device = context; > + struct quickstart_acpi *quickstart = dev_get_drvdata(&device->dev); > + > + if (!quickstart) > + return; > + > + switch (event) { > + case QUICKSTART_EVENT_WAKE: > + quickstart->wakeup_cause = true; > + break; > + case QUICKSTART_EVENT_RUNTIME: > + if (!sparse_keymap_report_event(quickstart->input_device, 0x1, > + 1, true)) { > + pr_info("Key handling error\n"); > + } > + break; > + default: > + pr_err("Unexpected ACPI event notify (%u)\n", event); Also as Barnabás mentioned already please use dev_*, so: dev_err(quickstart->dev, "Unexpected ACPI event notify (%u)\n", event); > + break; > + } > +} > + > +/* > + * The GHID ACPI method is used to indicate the "role" of the button. > + * However, all the meanings of these values are vendor defined. > + * > + * We do however expose this value to user space. > + */ > +static int quickstart_acpi_ghid(struct quickstart_acpi *quickstart) > +{ > + acpi_handle handle = ACPI_HANDLE(&quickstart->platform_dev->dev); > + acpi_status status; > + struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL }; > + int ret = 0; > + union acpi_object *obj = NULL; > + > + /* > + * This returns a buffer telling the button usage ID, > + * and triggers pending notify events (The ones before booting). > + */ > + status = acpi_evaluate_object(handle, "GHID", NULL, &buffer); > + if (ACPI_FAILURE(status)) { > + dev_err(&quickstart->platform_dev->dev, > + "GHID method failed, ACPI status %u\n", status); > + return -EINVAL; > + } > + obj = buffer.pointer; > + > + /* > + * GHID returns buffers, sanity check that is the case. > + */ > + if (obj->type != ACPI_TYPE_BUFFER) { > + dev_err(&quickstart->platform_dev->dev, > + "GHID did not return buffer\n"); > + ret = -EINVAL; > + goto free_and_return; > + } > + > + /* > + * Quoting the specification: > + * "The GHID method can return a BYTE, WORD, or DWORD. > + * The value must be encoded in little-endian byte > + * order (least significant byte first)." > + */ > + switch (obj->buffer.length) { > + case 1: > + quickstart->id = *(u8 *)obj->buffer.pointer; > + break; > + case 2: > + quickstart->id = get_unaligned_le16(obj->buffer.pointer); > + break; > + case 4: > + quickstart->id = get_unaligned_le32(obj->buffer.pointer); > + break; > + case 8: > + quickstart->id = get_unaligned_le64(obj->buffer.pointer); > + break; > + default: > + dev_err(&quickstart->platform_dev->dev, > + "GHID method returned buffer of unexpected length %lu\n", > + (unsigned long)obj->buffer.length); > + ret = -EINVAL; > + break; > + } > + > +free_and_return: > + kfree(buffer.pointer); > + > + return ret; > +} > + > +static struct attribute *quickstart_attributes[] = { > + &dev_attr_wakeup_cause.attr, > + &dev_attr_button_id.attr, > + NULL, > +}; > + > +static const struct attribute_group quickstart_attr_group = { > + .attrs = quickstart_attributes, > +}; > + > +static int quickstart_remove(struct platform_device *device) > +{ > + acpi_handle handle = ACPI_HANDLE(&device->dev); > + > + acpi_remove_notify_handler(handle, ACPI_DEVICE_NOTIFY, > + quickstart_acpi_notify); > + > + return 0; > +} > + > +static int quickstart_probe(struct platform_device *device) > +{ > + int ret; > + acpi_handle handle = ACPI_HANDLE(&device->dev); > + acpi_status status; > + struct quickstart_acpi *quickstart; > + > + if (!device) > + return -EINVAL; > + > + quickstart = > + devm_kzalloc(&device->dev, sizeof(*quickstart), GFP_KERNEL); > + if (!quickstart) > + return -ENOMEM; > + > + /* > + * This must be set early for proper cleanup on error handling path. > + * After this point generic error handling can be used. > + */ > + quickstart->platform_dev = device; > + dev_set_drvdata(&device->dev, quickstart); > + > + /* Retrieve the GHID ID */ > + ret = quickstart_acpi_ghid(quickstart); > + if (ret < 0) > + goto error; > + > + /* Set up sysfs entries */ > + ret = devm_device_add_group(&quickstart->platform_dev->dev, > + &quickstart_attr_group); > + if (ret) { > + dev_err(&device->dev, "Unable to setup sysfs entries\n"); > + goto error; > + } > + > + /* Set up input device */ > + quickstart->input_device = > + devm_input_allocate_device(&quickstart->platform_dev->dev); > + if (!quickstart->input_device) { > + ret = -ENOMEM; > + goto error; > + } > + ret = sparse_keymap_setup(quickstart->input_device, quickstart_keymap, > + NULL); > + if (ret) > + goto error; > + > + snprintf(quickstart->input_name, sizeof(quickstart->phys), > + "Quickstart Button %u", quickstart->id); > + snprintf(quickstart->phys, sizeof(quickstart->phys), > + QUICKSTART_ACPI_DEVICE_NAME "/input%u", quickstart->id); > + > + quickstart->input_device->name = quickstart->input_name; > + quickstart->input_device->phys = quickstart->phys; > + quickstart->input_device->id.bustype = BUS_HOST; > + > + ret = input_register_device(quickstart->input_device); > + > + /* Set up notify handler */ > + status = acpi_install_notify_handler(handle, ACPI_DEVICE_NOTIFY, > + quickstart_acpi_notify, device); > + if (ACPI_FAILURE(status)) { > + dev_err(&device->dev, "Error installing notify handler\n"); > + return -EIO; > + } > + > + return 0; > +error: > + quickstart_remove(device); > + return ret; > +} > + > +static const struct acpi_device_id quickstart_device_ids[] = { > + { QUICKSTART_ACPI_HID, 0 }, > + { "", 0 }, > +}; > +MODULE_DEVICE_TABLE(acpi, quickstart_device_ids); > + > +static struct platform_driver quickstart_platform_driver = { > + .probe = quickstart_probe, > + .remove = quickstart_remove, > + .driver = { > + .name = QUICKSTART_ACPI_DEVICE_NAME, > + .acpi_match_table = quickstart_device_ids, > + .owner = THIS_MODULE, > + } > +}; > + > +module_platform_driver(quickstart_platform_driver); Otherwise this looks good to me, thanks. Regards, Hans
Hi, I think something may be slightly broken. I got the below email twice, once in reply to where it should be, once as a reply to the cover letter. Best regards, Arvid On 2022-09-27 15:49, Hans de Goede wrote: > Hi, > > On 9/25/22 20:19, Arvid Norlander wrote: >> Hi, >> >> Thank you, I have incorperated your feedback in my local branch. >> >> On 2022-09-23 21:24, Barnabás Pőcze wrote: >>> Hi >>> >>> 2022. szeptember 22., csütörtök 20:24 keltezéssel, Arvid Norlander írta: >>> >>>> This is loosely based on a previous staging driver that was removed. See >>>> links below for more info on that driver. The original commit ID was >>>> 0be013e3dc2ee79ffab8a438bbb4e216837e3d52. >>>> >>>> However, here a completely different approach is taken to the user space >>>> API (which should solve the issues the original driver had). Each PNP0C32 >>>> device is a button, and each such button gets a separate input device >>>> associated with it (instead of a shared platform input device). >>>> >>>> The button ID (as read from ACPI method GHID) is provided via a sysfs file >>>> "button_id". >>>> >>>> If the button caused a wakeup it will "latch" the "wakeup_cause" sysfs file >>>> to true. This can be reset by a user space process. >>>> >>>> Link: https://marc.info/?l=linux-acpi&m=120550727131007 >>>> Link: https://lkml.org/lkml/2010/5/28/327 >>>> Signed-off-by: Arvid Norlander <lkml@vorpal.se> >>>> --- >>>> [...] >>>> diff --git a/drivers/platform/x86/quickstart.c b/drivers/platform/x86/quickstart.c >>>> new file mode 100644 >>>> index 000000000000..ce51abe012f7 >>>> --- /dev/null >>>> +++ b/drivers/platform/x86/quickstart.c >>>> @@ -0,0 +1,320 @@ >> >> <snip> >> >>>> + >>>> +static ssize_t wakeup_cause_store(struct device *dev, >>>> + struct device_attribute *attr, >>>> + const char *buf, size_t count) >>>> +{ >>>> + struct quickstart_acpi *quickstart = dev_get_drvdata(dev); >>>> + >>>> + if (count < 2) >>>> + return -EINVAL; >>>> + >>>> + if (strncasecmp(buf, "false", 4) != 0) >>>> + return -EINVAL; >>>> + >>> >>> If "true"/"false" will be used in the final version, then I think this check >>> currently is too lax. You could use `sysfs_streq()`. And I think the `count < 2` >>> check is not needed. >> >> Regarding the user space API I don't know, that is one of the open >> questions in the cover letter. I have yet to get any feedback on any of >> those questions. That is something that needs to happen before this driver >> can be included. I would appreciate your feedback on those. > > I will reply to this question in my general review of the driver. > > Regards, > > Hans > > > >> >> <snip> >> >>> >>> Regards, >>> Barnabás Pőcze >> >> Regards, >> Arvid Norlander >> >
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig index f5312f51de19..eebd70ef4a7c 100644 --- a/drivers/platform/x86/Kconfig +++ b/drivers/platform/x86/Kconfig @@ -685,6 +685,18 @@ config THINKPAD_LMI source "drivers/platform/x86/intel/Kconfig" +config ACPI_QUICKSTART + tristate "ACPI Quickstart key driver" + depends on ACPI + depends on INPUT + select INPUT_SPARSEKMAP + help + Say Y here if you have a platform that supports the ACPI + quickstart key protocol. + + To compile this driver as a module, choose M here: the module will be + called quickstart. + config MSI_LAPTOP tristate "MSI Laptop Extras" depends on ACPI @@ -803,6 +815,7 @@ config ACPI_TOSHIBA depends on RFKILL || RFKILL = n depends on IIO select INPUT_SPARSEKMAP + select ACPI_QUICKSTART help This driver adds support for access to certain system settings on "legacy free" Toshiba laptops. These laptops can be recognized by diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile index 5a428caa654a..a8a8e1ddb2a3 100644 --- a/drivers/platform/x86/Makefile +++ b/drivers/platform/x86/Makefile @@ -72,6 +72,9 @@ obj-$(CONFIG_THINKPAD_LMI) += think-lmi.o # Intel obj-y += intel/ +# Microsoft +obj-$(CONFIG_ACPI_QUICKSTART) += quickstart.o + # MSI obj-$(CONFIG_MSI_LAPTOP) += msi-laptop.o obj-$(CONFIG_MSI_WMI) += msi-wmi.o diff --git a/drivers/platform/x86/quickstart.c b/drivers/platform/x86/quickstart.c new file mode 100644 index 000000000000..ce51abe012f7 --- /dev/null +++ b/drivers/platform/x86/quickstart.c @@ -0,0 +1,320 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * quickstart.c - ACPI Direct App Launch driver + * + * Copyright (C) 2022 Arvid Norlander <lkml@vorapal.se> + * Copyright (C) 2007-2010 Angelo Arrifano <miknix@gmail.com> + * + * Information gathered from disassembled dsdt and from here: + * <https://archive.org/details/microsoft-acpi-dirapplaunch> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + */ + +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt + +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/init.h> +#include <linux/types.h> +#include <linux/acpi.h> +#include <linux/platform_device.h> +#include <linux/input.h> +#include <linux/input/sparse-keymap.h> +#include <asm/unaligned.h> + +MODULE_AUTHOR("Arvid Norlander <lkml@vorpal.se>"); +MODULE_AUTHOR("Angelo Arrifano"); +MODULE_DESCRIPTION("ACPI Direct App Launch driver"); +MODULE_LICENSE("GPL"); + +#define QUICKSTART_ACPI_DEVICE_NAME "quickstart" +#define QUICKSTART_ACPI_HID "PNP0C32" + +/* + * There will be two events: + * 0x02 - A hot button was pressed while device was off/sleeping. + * 0x80 - A hot button was pressed while device was up. + */ +#define QUICKSTART_EVENT_WAKE 0x02 +#define QUICKSTART_EVENT_RUNTIME 0x80 + +/* + * Each PNP0C32 device is an individual button. This structure + * keeps track of data associated with said device. + */ +struct quickstart_acpi { + struct platform_device *platform_dev; + struct input_dev *input_device; + struct quickstart_button *button; + /* ID of button as returned by GHID */ + u32 id; + /* Name of input device */ + char input_name[32]; + /* Physical path for the input device */ + char phys[32]; + /* Track if a wakeup event was received */ + bool wakeup_cause; +}; + +#define quickstart_name(dev) acpi_device_bid(dev->acpi_dev) + +/* + * Knowing what these buttons do require system specific knowledge. + * This could be done by matching on DMI data in a long quirk table. + * However, it is easier to leave it up to user space to figure this out. + * + * Using for example udev hwdb the scancode 0x1 can be remapped suitably. + */ +static const struct key_entry quickstart_keymap[] = { + { KE_KEY, 0x1, { KEY_UNKNOWN } }, + { KE_END, 0 }, +}; + +static ssize_t wakeup_cause_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct quickstart_acpi *quickstart = dev_get_drvdata(dev); + + return sysfs_emit(buf, "%s\n", + (quickstart->wakeup_cause ? "true" : "false")); +} + +static ssize_t wakeup_cause_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + struct quickstart_acpi *quickstart = dev_get_drvdata(dev); + + if (count < 2) + return -EINVAL; + + if (strncasecmp(buf, "false", 4) != 0) + return -EINVAL; + + quickstart->wakeup_cause = false; + return count; +} +static DEVICE_ATTR_RW(wakeup_cause); + +static ssize_t button_id_show(struct device *dev, struct device_attribute *attr, + char *buf) +{ + struct quickstart_acpi *quickstart = dev_get_drvdata(dev); + + return sysfs_emit(buf, "%u\n", quickstart->id); +} +static DEVICE_ATTR_RO(button_id); + +/* ACPI Driver functions */ +static void quickstart_acpi_notify(acpi_handle handle, u32 event, void *context) +{ + struct platform_device *device = context; + struct quickstart_acpi *quickstart = dev_get_drvdata(&device->dev); + + if (!quickstart) + return; + + switch (event) { + case QUICKSTART_EVENT_WAKE: + quickstart->wakeup_cause = true; + break; + case QUICKSTART_EVENT_RUNTIME: + if (!sparse_keymap_report_event(quickstart->input_device, 0x1, + 1, true)) { + pr_info("Key handling error\n"); + } + break; + default: + pr_err("Unexpected ACPI event notify (%u)\n", event); + break; + } +} + +/* + * The GHID ACPI method is used to indicate the "role" of the button. + * However, all the meanings of these values are vendor defined. + * + * We do however expose this value to user space. + */ +static int quickstart_acpi_ghid(struct quickstart_acpi *quickstart) +{ + acpi_handle handle = ACPI_HANDLE(&quickstart->platform_dev->dev); + acpi_status status; + struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL }; + int ret = 0; + union acpi_object *obj = NULL; + + /* + * This returns a buffer telling the button usage ID, + * and triggers pending notify events (The ones before booting). + */ + status = acpi_evaluate_object(handle, "GHID", NULL, &buffer); + if (ACPI_FAILURE(status)) { + dev_err(&quickstart->platform_dev->dev, + "GHID method failed, ACPI status %u\n", status); + return -EINVAL; + } + obj = buffer.pointer; + + /* + * GHID returns buffers, sanity check that is the case. + */ + if (obj->type != ACPI_TYPE_BUFFER) { + dev_err(&quickstart->platform_dev->dev, + "GHID did not return buffer\n"); + ret = -EINVAL; + goto free_and_return; + } + + /* + * Quoting the specification: + * "The GHID method can return a BYTE, WORD, or DWORD. + * The value must be encoded in little-endian byte + * order (least significant byte first)." + */ + switch (obj->buffer.length) { + case 1: + quickstart->id = *(u8 *)obj->buffer.pointer; + break; + case 2: + quickstart->id = get_unaligned_le16(obj->buffer.pointer); + break; + case 4: + quickstart->id = get_unaligned_le32(obj->buffer.pointer); + break; + case 8: + quickstart->id = get_unaligned_le64(obj->buffer.pointer); + break; + default: + dev_err(&quickstart->platform_dev->dev, + "GHID method returned buffer of unexpected length %lu\n", + (unsigned long)obj->buffer.length); + ret = -EINVAL; + break; + } + +free_and_return: + kfree(buffer.pointer); + + return ret; +} + +static struct attribute *quickstart_attributes[] = { + &dev_attr_wakeup_cause.attr, + &dev_attr_button_id.attr, + NULL, +}; + +static const struct attribute_group quickstart_attr_group = { + .attrs = quickstart_attributes, +}; + +static int quickstart_remove(struct platform_device *device) +{ + acpi_handle handle = ACPI_HANDLE(&device->dev); + + acpi_remove_notify_handler(handle, ACPI_DEVICE_NOTIFY, + quickstart_acpi_notify); + + return 0; +} + +static int quickstart_probe(struct platform_device *device) +{ + int ret; + acpi_handle handle = ACPI_HANDLE(&device->dev); + acpi_status status; + struct quickstart_acpi *quickstart; + + if (!device) + return -EINVAL; + + quickstart = + devm_kzalloc(&device->dev, sizeof(*quickstart), GFP_KERNEL); + if (!quickstart) + return -ENOMEM; + + /* + * This must be set early for proper cleanup on error handling path. + * After this point generic error handling can be used. + */ + quickstart->platform_dev = device; + dev_set_drvdata(&device->dev, quickstart); + + /* Retrieve the GHID ID */ + ret = quickstart_acpi_ghid(quickstart); + if (ret < 0) + goto error; + + /* Set up sysfs entries */ + ret = devm_device_add_group(&quickstart->platform_dev->dev, + &quickstart_attr_group); + if (ret) { + dev_err(&device->dev, "Unable to setup sysfs entries\n"); + goto error; + } + + /* Set up input device */ + quickstart->input_device = + devm_input_allocate_device(&quickstart->platform_dev->dev); + if (!quickstart->input_device) { + ret = -ENOMEM; + goto error; + } + ret = sparse_keymap_setup(quickstart->input_device, quickstart_keymap, + NULL); + if (ret) + goto error; + + snprintf(quickstart->input_name, sizeof(quickstart->phys), + "Quickstart Button %u", quickstart->id); + snprintf(quickstart->phys, sizeof(quickstart->phys), + QUICKSTART_ACPI_DEVICE_NAME "/input%u", quickstart->id); + + quickstart->input_device->name = quickstart->input_name; + quickstart->input_device->phys = quickstart->phys; + quickstart->input_device->id.bustype = BUS_HOST; + + ret = input_register_device(quickstart->input_device); + + /* Set up notify handler */ + status = acpi_install_notify_handler(handle, ACPI_DEVICE_NOTIFY, + quickstart_acpi_notify, device); + if (ACPI_FAILURE(status)) { + dev_err(&device->dev, "Error installing notify handler\n"); + return -EIO; + } + + return 0; +error: + quickstart_remove(device); + return ret; +} + +static const struct acpi_device_id quickstart_device_ids[] = { + { QUICKSTART_ACPI_HID, 0 }, + { "", 0 }, +}; +MODULE_DEVICE_TABLE(acpi, quickstart_device_ids); + +static struct platform_driver quickstart_platform_driver = { + .probe = quickstart_probe, + .remove = quickstart_remove, + .driver = { + .name = QUICKSTART_ACPI_DEVICE_NAME, + .acpi_match_table = quickstart_device_ids, + .owner = THIS_MODULE, + } +}; + +module_platform_driver(quickstart_platform_driver);
This is loosely based on a previous staging driver that was removed. See links below for more info on that driver. The original commit ID was 0be013e3dc2ee79ffab8a438bbb4e216837e3d52. However, here a completely different approach is taken to the user space API (which should solve the issues the original driver had). Each PNP0C32 device is a button, and each such button gets a separate input device associated with it (instead of a shared platform input device). The button ID (as read from ACPI method GHID) is provided via a sysfs file "button_id". If the button caused a wakeup it will "latch" the "wakeup_cause" sysfs file to true. This can be reset by a user space process. Link: https://marc.info/?l=linux-acpi&m=120550727131007 Link: https://lkml.org/lkml/2010/5/28/327 Signed-off-by: Arvid Norlander <lkml@vorpal.se> --- drivers/platform/x86/Kconfig | 13 ++ drivers/platform/x86/Makefile | 3 + drivers/platform/x86/quickstart.c | 320 ++++++++++++++++++++++++++++++ 3 files changed, 336 insertions(+) create mode 100644 drivers/platform/x86/quickstart.c