| Message ID | 20220911194934.558019-2-lkml@vorpal.se (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Quickstart buttons driver and Toshiba Z830 | expand |
Hi 2022. szeptember 11., vasárnap 21:49 keltezéssel, Arvid Norlander írta: > This is loosly 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> > [...] > +#define QUICKSTART_VERSION "1.04" > + > +#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/input.h> > +#include <linux/input/sparse-keymap.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_CLASS "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 acpi_device *acpi_dev; > + struct input_dev *input_device; > + struct quickstart_button *button; > + /* Name of button for debug messages */ > + char *name; > + /* ID of button as returned by GHID */ > + u32 id; > + /* Flags for cleanup */ > + unsigned int input_registered : 1; This member is set, but never read. > + unsigned int sysfs_created : 1; > + /* Track if a wakeup event was received */ > + unsigned int wakeup_cause : 1; > + /* Name of input device */ > + char input_name[32]; > + /* Physical path for the input device */ > + char phys[32]; > +}; > + > +/* > + * 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 scnprintf(buf, PAGE_SIZE, "%s\n", > + (quickstart->wakeup_cause ? "true" : "false")); Please use `sysfs_emit()` preferably. And I think it would be easier to use 0/1 instead of true/false. And you could use `kstrtobool()` in the _store() function. > +} > + > +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 scnprintf(buf, PAGE_SIZE, "%d\n", quickstart->id); Since it is unsigned `%u` would probably be better. > +} > +static DEVICE_ATTR_RO(button_id); > + > +/* ACPI Driver functions */ > +static void quickstart_acpi_notify(struct acpi_device *acpi_dev, u32 event) > +{ > + struct quickstart_acpi *quickstart = acpi_driver_data(acpi_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_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(quickstart->acpi_dev->handle, "GHID", > + NULL, &buffer); > + if (ACPI_FAILURE(status)) { > + pr_err("%s GHID method failed\n", quickstart->name); > + return -EINVAL; > + } > + obj = buffer.pointer; > + > + /* > + * GHID returns buffers, sanity check that is the case. > + */ > + if (obj->type != ACPI_TYPE_BUFFER) { > + pr_err("%s GHID did not return buffer\n", quickstart->name); > + return -EINVAL; `buffer.pointer` is not freed here. Since you know the maximum size, you could consider using an on-stack buffer. > + } > + > + /* > + * 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 = le16_to_cpu(*(u16 *)obj->buffer.pointer); Probably does not matter here, but I personally like to use `get_unaligned_leN()` because those functions just always work. > + break; > + case 4: > + quickstart->id = le32_to_cpu(*(u32 *)obj->buffer.pointer); > + break; > + case 8: > + quickstart->id = le64_to_cpu(*(u64 *)obj->buffer.pointer); > + break; > + default: > + pr_err("%s GHID method returned buffer of unexpected length %lu\n", > + quickstart->name, (unsigned long)obj->buffer.length); > + ret = -EINVAL; > + break; > + } > + > + kfree(buffer.pointer); > + > + return ret; > +} > + > +static int quickstart_acpi_config(struct quickstart_acpi *quickstart) > +{ > + char *bid = acpi_device_bid(quickstart->acpi_dev); > + char *name; > + > + name = kmalloc(strlen(bid) + 1, GFP_KERNEL); > + if (!name) > + return -ENOMEM; > + > + quickstart->name = name; > + strcpy(quickstart->name, bid); You could use `kstrdup()` here, but you could probably even use `devm_kstrdup()` and then this function could be entirely removed. > + > + return 0; > +} > + > +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_acpi_remove(struct acpi_device *acpi_dev) > +{ > + struct quickstart_acpi *quickstart; > + > + if (!acpi_dev) > + return -EINVAL; > + > + quickstart = acpi_driver_data(acpi_dev); > + if (!quickstart) > + return -EINVAL; > + > + if (quickstart->sysfs_created) > + sysfs_remove_group(&quickstart->acpi_dev->dev.kobj, > + &quickstart_attr_group); > + > + kfree(quickstart->name); > + quickstart->name = NULL; > + > + kfree(quickstart); > + > + return 0; > +} > + > +static int quickstart_acpi_add(struct acpi_device *acpi_dev) > +{ > + int ret; > + struct quickstart_acpi *quickstart; > + > + if (!acpi_dev) > + return -EINVAL; > + > + quickstart = kzalloc(sizeof(*quickstart), GFP_KERNEL); Have you considered `devm_kzalloc()`? > + 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. > + */ > + acpi_dev->driver_data = quickstart; > + quickstart->acpi_dev = acpi_dev; > + dev_set_drvdata(&acpi_dev->dev, quickstart); > + > + strcpy(acpi_device_name(acpi_dev), QUICKSTART_ACPI_DEVICE_NAME); > + strcpy(acpi_device_class(acpi_dev), QUICKSTART_ACPI_CLASS); > + > + /* Initialize device name */ > + ret = quickstart_acpi_config(quickstart); > + if (ret < 0) > + goto error; > + > + /* Retrieve the GHID ID */ > + ret = quickstart_acpi_ghid(quickstart); > + if (ret < 0) > + goto error; > + > + /* Set up sysfs entries */ > + ret = sysfs_create_group(&quickstart->acpi_dev->dev.kobj, > + &quickstart_attr_group); You could use `devm_device_add_group()`. And then the `sysfs_created` member could be removed. > + if (ret) { > + quickstart->sysfs_created = 0; > + pr_err("Unable to setup sysfs entries\n"); > + goto error; > + } > + quickstart->sysfs_created = !ret; > + > + /* Set up input device */ > + quickstart->input_device = > + devm_input_allocate_device(&quickstart->acpi_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); > + if (ret) { > + quickstart->input_registered = 0; > + pr_err("Unable to register input device\n"); > + goto error; > + } > + quickstart->input_registered = !ret; > + > + return 0; > +error: > + quickstart_acpi_remove(acpi_dev); > + 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 acpi_driver quickstart_acpi_driver = { > + .name = "quickstart", > + .owner = THIS_MODULE, > + .class = QUICKSTART_ACPI_CLASS, > + .ids = quickstart_device_ids, > + .flags = ACPI_DRIVER_ALL_NOTIFY_EVENTS, > + .ops = { > + .add = quickstart_acpi_add, > + .remove = quickstart_acpi_remove, > + .notify = quickstart_acpi_notify > + }, > +}; > + > +/* Module functions */ > +static void quickstart_exit(void) > +{ > + acpi_bus_unregister_driver(&quickstart_acpi_driver); > +} > + > +static int __init quickstart_init(void) > +{ > + int ret; > + > + /* ACPI driver register */ > + ret = acpi_bus_register_driver(&quickstart_acpi_driver); > + if (ret) > + return ret; > + > + pr_info("ACPI Direct App Launch ver %s\n", QUICKSTART_VERSION); > + > + return 0; > +} > + > +module_init(quickstart_init); > +module_exit(quickstart_exit); You could use the `module_acpi_driver()` macro to generate the init/exit methods. > -- > 2.37.3 Regards, Barnabás Pőcze
Hi, On 2022-09-14 20:27, Barnabás Pőcze wrote: > Hi > > 2022. szeptember 11., vasárnap 21:49 keltezéssel, Arvid Norlander írta: > >> This is loosly 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> >> [...] >> +#define QUICKSTART_VERSION "1.04" >> + >> +#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/input.h> >> +#include <linux/input/sparse-keymap.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_CLASS "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 acpi_device *acpi_dev; >> + struct input_dev *input_device; >> + struct quickstart_button *button; >> + /* Name of button for debug messages */ >> + char *name; >> + /* ID of button as returned by GHID */ >> + u32 id; >> + /* Flags for cleanup */ >> + unsigned int input_registered : 1; > > This member is set, but never read. Right, I switched to the devm_ version, so I believe it is no longer needed. I'll remove it. > > >> + unsigned int sysfs_created : 1; >> + /* Track if a wakeup event was received */ >> + unsigned int wakeup_cause : 1; >> + /* Name of input device */ >> + char input_name[32]; >> + /* Physical path for the input device */ >> + char phys[32]; >> +}; >> + >> +/* >> + * 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 scnprintf(buf, PAGE_SIZE, "%s\n", >> + (quickstart->wakeup_cause ? "true" : "false")); > > Please use `sysfs_emit()` preferably. And I think it would be easier to use 0/1 > instead of true/false. And you could use `kstrtobool()` in the _store() function. Thanks! As I'm new to kernel development it is sometimes hard to know the proper API. While there is some API documentation for the most part I have not found any good tutorials or overviews. Basically you have to know what you are looking for already in order to find it. Thus I have taken to looking at existing drivers and copying the patterns from those. However it is very hard to know what drivers use the currently preferred patterns, and which ones use legacy patterns. Better documentation would be great. > > >> +} >> + >> +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 scnprintf(buf, PAGE_SIZE, "%d\n", quickstart->id); > > Since it is unsigned `%u` would probably be better. Good catch! Thanks. I wonder why I didn't get any -Wformat warnings about this? > > >> +} >> +static DEVICE_ATTR_RO(button_id); >> + >> +/* ACPI Driver functions */ >> +static void quickstart_acpi_notify(struct acpi_device *acpi_dev, u32 event) >> +{ >> + struct quickstart_acpi *quickstart = acpi_driver_data(acpi_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_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(quickstart->acpi_dev->handle, "GHID", >> + NULL, &buffer); >> + if (ACPI_FAILURE(status)) { >> + pr_err("%s GHID method failed\n", quickstart->name); >> + return -EINVAL; >> + } >> + obj = buffer.pointer; >> + >> + /* >> + * GHID returns buffers, sanity check that is the case. >> + */ >> + if (obj->type != ACPI_TYPE_BUFFER) { >> + pr_err("%s GHID did not return buffer\n", quickstart->name); >> + return -EINVAL; > > `buffer.pointer` is not freed here. Since you know the maximum size, you could > consider using an on-stack buffer. Good catch! I thought about onstack buffer but couldn't get it working. However I believe I was confused by the API at the point, and didn't realise that "acpi_buffer" is not in fact the same as a buffer object in ACPI. So it might be worth trying again. > > >> + } >> + >> + /* >> + * 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 = le16_to_cpu(*(u16 *)obj->buffer.pointer); > > Probably does not matter here, but I personally like to use `get_unaligned_leN()` > because those functions just always work. This goes back to the whole "I know what I want to do, but I don't know what the API is named, lets see what others are doing." thing I mentioned above. As you said, this is probably fine, as I doubt anyone has used this ACPI standard on anything except x86, where unaligned reads are fine. Even the whole endianness conversion will *probably* never come up in practise for this driver. > > >> + break; >> + case 4: >> + quickstart->id = le32_to_cpu(*(u32 *)obj->buffer.pointer); >> + break; >> + case 8: >> + quickstart->id = le64_to_cpu(*(u64 *)obj->buffer.pointer); >> + break; >> + default: >> + pr_err("%s GHID method returned buffer of unexpected length %lu\n", >> + quickstart->name, (unsigned long)obj->buffer.length); >> + ret = -EINVAL; >> + break; >> + } >> + >> + kfree(buffer.pointer); >> + >> + return ret; >> +} >> + >> +static int quickstart_acpi_config(struct quickstart_acpi *quickstart) >> +{ >> + char *bid = acpi_device_bid(quickstart->acpi_dev); >> + char *name; >> + >> + name = kmalloc(strlen(bid) + 1, GFP_KERNEL); >> + if (!name) >> + return -ENOMEM; >> + >> + quickstart->name = name; >> + strcpy(quickstart->name, bid); > > You could use `kstrdup()` here, but you could probably even use `devm_kstrdup()` > and then this function could be entirely removed. Again, we are back to copying patterns from other drivers. Would be great if there was a way to know which drivers were following current guidelines. Maybe each subsystem could have a "model" driver, that they could point to for current best practices? However, I realised there is in fact no need to store a copy at all. We can just define #define quickstart_name(dev) acpi_device_bid(dev->acpi_dev) and use that. As acpi_device_bid is itself just a macro that returns a member. > > >> + >> + return 0; >> +} >> + >> +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_acpi_remove(struct acpi_device *acpi_dev) >> +{ >> + struct quickstart_acpi *quickstart; >> + >> + if (!acpi_dev) >> + return -EINVAL; >> + >> + quickstart = acpi_driver_data(acpi_dev); >> + if (!quickstart) >> + return -EINVAL; >> + >> + if (quickstart->sysfs_created) >> + sysfs_remove_group(&quickstart->acpi_dev->dev.kobj, >> + &quickstart_attr_group); >> + >> + kfree(quickstart->name); >> + quickstart->name = NULL; >> + >> + kfree(quickstart); >> + >> + return 0; >> +} >> + >> +static int quickstart_acpi_add(struct acpi_device *acpi_dev) >> +{ >> + int ret; >> + struct quickstart_acpi *quickstart; >> + >> + if (!acpi_dev) >> + return -EINVAL; >> + >> + quickstart = kzalloc(sizeof(*quickstart), GFP_KERNEL); > > Have you considered `devm_kzalloc()`? Again no. In this case it didn't come from copying something, but from the old quickstart driver code from 2010. I'm switching to this devm_ stuff, seems awesome. > > >> + 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. >> + */ >> + acpi_dev->driver_data = quickstart; >> + quickstart->acpi_dev = acpi_dev; >> + dev_set_drvdata(&acpi_dev->dev, quickstart); >> + >> + strcpy(acpi_device_name(acpi_dev), QUICKSTART_ACPI_DEVICE_NAME); >> + strcpy(acpi_device_class(acpi_dev), QUICKSTART_ACPI_CLASS); >> + >> + /* Initialize device name */ >> + ret = quickstart_acpi_config(quickstart); >> + if (ret < 0) >> + goto error; >> + >> + /* Retrieve the GHID ID */ >> + ret = quickstart_acpi_ghid(quickstart); >> + if (ret < 0) >> + goto error; >> + >> + /* Set up sysfs entries */ >> + ret = sysfs_create_group(&quickstart->acpi_dev->dev.kobj, >> + &quickstart_attr_group); > > You could use `devm_device_add_group()`. And then the `sysfs_created` member > could be removed. Oh, nice. > > >> + if (ret) { >> + quickstart->sysfs_created = 0; >> + pr_err("Unable to setup sysfs entries\n"); >> + goto error; >> + } >> + quickstart->sysfs_created = !ret; >> + >> + /* Set up input device */ >> + quickstart->input_device = >> + devm_input_allocate_device(&quickstart->acpi_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); >> + if (ret) { >> + quickstart->input_registered = 0; >> + pr_err("Unable to register input device\n"); >> + goto error; >> + } >> + quickstart->input_registered = !ret; >> + >> + return 0; >> +error: >> + quickstart_acpi_remove(acpi_dev); >> + 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 acpi_driver quickstart_acpi_driver = { >> + .name = "quickstart", >> + .owner = THIS_MODULE, >> + .class = QUICKSTART_ACPI_CLASS, >> + .ids = quickstart_device_ids, >> + .flags = ACPI_DRIVER_ALL_NOTIFY_EVENTS, >> + .ops = { >> + .add = quickstart_acpi_add, >> + .remove = quickstart_acpi_remove, >> + .notify = quickstart_acpi_notify >> + }, >> +}; >> + >> +/* Module functions */ >> +static void quickstart_exit(void) >> +{ >> + acpi_bus_unregister_driver(&quickstart_acpi_driver); >> +} >> + >> +static int __init quickstart_init(void) >> +{ >> + int ret; >> + >> + /* ACPI driver register */ >> + ret = acpi_bus_register_driver(&quickstart_acpi_driver); >> + if (ret) >> + return ret; >> + >> + pr_info("ACPI Direct App Launch ver %s\n", QUICKSTART_VERSION); >> + >> + return 0; >> +} >> + >> +module_init(quickstart_init); >> +module_exit(quickstart_exit); > > You could use the `module_acpi_driver()` macro to generate the init/exit methods. Nice! > > >> -- >> 2.37.3 > > > Regards, > Barnabás Pőcze I have fixed these locally, and will submit a new patch at the beginning of next week. I was hoping to get some feedback on the open questions in the cover letter before that, but right now that doesn't seem likely to happen. Best regards, Arvid Norlander
Hi, On 9/11/22 20:49, Arvid Norlander wrote: > This is loosly 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> 2 high level remarks here: 1. I believe we should strive for having all issues with this driver fixed before merging it, at which point it should not sit under drivers/staging but rather under drivers/platform/x86 (as an added bonus this can also make toshiba_apci's Kconfig bit select it automatically). So for the next version please move this to drivers/platform/x86 2. This is using struct acpi_driver, but as Rafael (ACPI maintainer) always said that is really only for very special cases. The ACPI subsystem should instantiate standard platform devices for each PNP0C32 device, you can check this under: /sys/bus/devices/platform. And this driver should then be convered to a standard platform_driver binding to the platform devices instead of being a struct acpi_driver. Please address these 2 things as well as the remarks from Barnabás and then send out a version 2. Then I will do a more detailed review of version 2 once posted. Regards, Hans > --- > drivers/staging/Kconfig | 2 + > drivers/staging/Makefile | 1 + > drivers/staging/quickstart/Kconfig | 12 + > drivers/staging/quickstart/Makefile | 1 + > drivers/staging/quickstart/quickstart.c | 376 ++++++++++++++++++++++++ > 5 files changed, 392 insertions(+) > create mode 100644 drivers/staging/quickstart/Kconfig > create mode 100644 drivers/staging/quickstart/Makefile > create mode 100644 drivers/staging/quickstart/quickstart.c > > diff --git a/drivers/staging/Kconfig b/drivers/staging/Kconfig > index 3bd80f9695ac..db89ffbcd1ad 100644 > --- a/drivers/staging/Kconfig > +++ b/drivers/staging/Kconfig > @@ -50,6 +50,8 @@ source "drivers/staging/iio/Kconfig" > > source "drivers/staging/sm750fb/Kconfig" > > +source "drivers/staging/quickstart/Kconfig" > + > source "drivers/staging/emxx_udc/Kconfig" > > source "drivers/staging/nvec/Kconfig" > diff --git a/drivers/staging/Makefile b/drivers/staging/Makefile > index 1d9ae39fea14..cb92880f7db5 100644 > --- a/drivers/staging/Makefile > +++ b/drivers/staging/Makefile > @@ -16,6 +16,7 @@ obj-$(CONFIG_VT6656) += vt6656/ > obj-$(CONFIG_VME_BUS) += vme_user/ > obj-$(CONFIG_IIO) += iio/ > obj-$(CONFIG_FB_SM750) += sm750fb/ > +obj-$(CONFIG_ACPI_QUICKSTART) += quickstart/ > obj-$(CONFIG_USB_EMXX) += emxx_udc/ > obj-$(CONFIG_MFD_NVEC) += nvec/ > obj-$(CONFIG_STAGING_BOARD) += board/ > diff --git a/drivers/staging/quickstart/Kconfig b/drivers/staging/quickstart/Kconfig > new file mode 100644 > index 000000000000..e1cf1810e967 > --- /dev/null > +++ b/drivers/staging/quickstart/Kconfig > @@ -0,0 +1,12 @@ > +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. > + > diff --git a/drivers/staging/quickstart/Makefile b/drivers/staging/quickstart/Makefile > new file mode 100644 > index 000000000000..290e0e476797 > --- /dev/null > +++ b/drivers/staging/quickstart/Makefile > @@ -0,0 +1 @@ > +obj-$(CONFIG_ACPI_QUICKSTART) += quickstart.o > diff --git a/drivers/staging/quickstart/quickstart.c b/drivers/staging/quickstart/quickstart.c > new file mode 100644 > index 000000000000..8d76472c6b7f > --- /dev/null > +++ b/drivers/staging/quickstart/quickstart.c > @@ -0,0 +1,376 @@ > +// 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 QUICKSTART_VERSION "1.04" > + > +#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/input.h> > +#include <linux/input/sparse-keymap.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_CLASS "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 acpi_device *acpi_dev; > + struct input_dev *input_device; > + struct quickstart_button *button; > + /* Name of button for debug messages */ > + char *name; > + /* ID of button as returned by GHID */ > + u32 id; > + /* Flags for cleanup */ > + unsigned int input_registered : 1; > + unsigned int sysfs_created : 1; > + /* Track if a wakeup event was received */ > + unsigned int wakeup_cause : 1; > + /* Name of input device */ > + char input_name[32]; > + /* Physical path for the input device */ > + char phys[32]; > +}; > + > +/* > + * 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 scnprintf(buf, PAGE_SIZE, "%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 scnprintf(buf, PAGE_SIZE, "%d\n", quickstart->id); > +} > +static DEVICE_ATTR_RO(button_id); > + > +/* ACPI Driver functions */ > +static void quickstart_acpi_notify(struct acpi_device *acpi_dev, u32 event) > +{ > + struct quickstart_acpi *quickstart = acpi_driver_data(acpi_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_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(quickstart->acpi_dev->handle, "GHID", > + NULL, &buffer); > + if (ACPI_FAILURE(status)) { > + pr_err("%s GHID method failed\n", quickstart->name); > + return -EINVAL; > + } > + obj = buffer.pointer; > + > + /* > + * GHID returns buffers, sanity check that is the case. > + */ > + if (obj->type != ACPI_TYPE_BUFFER) { > + pr_err("%s GHID did not return buffer\n", quickstart->name); > + return -EINVAL; > + } > + > + /* > + * 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 = le16_to_cpu(*(u16 *)obj->buffer.pointer); > + break; > + case 4: > + quickstart->id = le32_to_cpu(*(u32 *)obj->buffer.pointer); > + break; > + case 8: > + quickstart->id = le64_to_cpu(*(u64 *)obj->buffer.pointer); > + break; > + default: > + pr_err("%s GHID method returned buffer of unexpected length %lu\n", > + quickstart->name, (unsigned long)obj->buffer.length); > + ret = -EINVAL; > + break; > + } > + > + kfree(buffer.pointer); > + > + return ret; > +} > + > +static int quickstart_acpi_config(struct quickstart_acpi *quickstart) > +{ > + char *bid = acpi_device_bid(quickstart->acpi_dev); > + char *name; > + > + name = kmalloc(strlen(bid) + 1, GFP_KERNEL); > + if (!name) > + return -ENOMEM; > + > + quickstart->name = name; > + strcpy(quickstart->name, bid); > + > + return 0; > +} > + > +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_acpi_remove(struct acpi_device *acpi_dev) > +{ > + struct quickstart_acpi *quickstart; > + > + if (!acpi_dev) > + return -EINVAL; > + > + quickstart = acpi_driver_data(acpi_dev); > + if (!quickstart) > + return -EINVAL; > + > + if (quickstart->sysfs_created) > + sysfs_remove_group(&quickstart->acpi_dev->dev.kobj, > + &quickstart_attr_group); > + > + kfree(quickstart->name); > + quickstart->name = NULL; > + > + kfree(quickstart); > + > + return 0; > +} > + > +static int quickstart_acpi_add(struct acpi_device *acpi_dev) > +{ > + int ret; > + struct quickstart_acpi *quickstart; > + > + if (!acpi_dev) > + return -EINVAL; > + > + quickstart = kzalloc(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. > + */ > + acpi_dev->driver_data = quickstart; > + quickstart->acpi_dev = acpi_dev; > + dev_set_drvdata(&acpi_dev->dev, quickstart); > + > + strcpy(acpi_device_name(acpi_dev), QUICKSTART_ACPI_DEVICE_NAME); > + strcpy(acpi_device_class(acpi_dev), QUICKSTART_ACPI_CLASS); > + > + /* Initialize device name */ > + ret = quickstart_acpi_config(quickstart); > + if (ret < 0) > + goto error; > + > + /* Retrieve the GHID ID */ > + ret = quickstart_acpi_ghid(quickstart); > + if (ret < 0) > + goto error; > + > + /* Set up sysfs entries */ > + ret = sysfs_create_group(&quickstart->acpi_dev->dev.kobj, > + &quickstart_attr_group); > + if (ret) { > + quickstart->sysfs_created = 0; > + pr_err("Unable to setup sysfs entries\n"); > + goto error; > + } > + quickstart->sysfs_created = !ret; > + > + /* Set up input device */ > + quickstart->input_device = > + devm_input_allocate_device(&quickstart->acpi_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); > + if (ret) { > + quickstart->input_registered = 0; > + pr_err("Unable to register input device\n"); > + goto error; > + } > + quickstart->input_registered = !ret; > + > + return 0; > +error: > + quickstart_acpi_remove(acpi_dev); > + 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 acpi_driver quickstart_acpi_driver = { > + .name = "quickstart", > + .owner = THIS_MODULE, > + .class = QUICKSTART_ACPI_CLASS, > + .ids = quickstart_device_ids, > + .flags = ACPI_DRIVER_ALL_NOTIFY_EVENTS, > + .ops = { > + .add = quickstart_acpi_add, > + .remove = quickstart_acpi_remove, > + .notify = quickstart_acpi_notify > + }, > +}; > + > +/* Module functions */ > +static void quickstart_exit(void) > +{ > + acpi_bus_unregister_driver(&quickstart_acpi_driver); > +} > + > +static int __init quickstart_init(void) > +{ > + int ret; > + > + /* ACPI driver register */ > + ret = acpi_bus_register_driver(&quickstart_acpi_driver); > + if (ret) > + return ret; > + > + pr_info("ACPI Direct App Launch ver %s\n", QUICKSTART_VERSION); > + > + return 0; > +} > + > +module_init(quickstart_init); > +module_exit(quickstart_exit);
On 2022-09-19 11:27, Hans de Goede wrote: > Hi, > > On 9/11/22 20:49, Arvid Norlander wrote: >> This is loosly 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> > > 2 high level remarks here: > > 1. I believe we should strive for having all issues with this driver fixed > before merging it, at which point it should not sit under drivers/staging > but rather under drivers/platform/x86 (as an added bonus this can also make > toshiba_apci's Kconfig bit select it automatically). So for the next version > please move this to drivers/platform/x86 Makes sense, will do. However, there is nothing x86 specific in theory with this driver. Would it not make more sense to put it under drivers/acpi? > > 2. This is using struct acpi_driver, but as Rafael (ACPI maintainer) always > said that is really only for very special cases. The ACPI subsystem should > instantiate standard platform devices for each PNP0C32 device, you can > check this under: /sys/bus/devices/platform. And this driver should then > be convered to a standard platform_driver binding to the platform devices > instead of being a struct acpi_driver. I had a look at this, and it seems like a much more complicated a approach, for example, there is no dedicated .ops.notify, which means I need to use acpi_install_notify_handler, and there is no devm_ version of that either. A lot of other things seem to be ever so slightly more complicated as well. What is the motivation behind this being preferred? And are most of the existing drivers using acpi_driver legacy (e.g. toshiba_acpi)? > > Please address these 2 things as well as the remarks from Barnabás and > then send out a version 2. Then I will do a more detailed review of > version 2 once posted. > > Regards, > > Hans > <snip>
Hi Arvid, On 9/20/22 11:48, Arvid Norlander wrote: > On 2022-09-19 11:27, Hans de Goede wrote: >> Hi, >> >> On 9/11/22 20:49, Arvid Norlander wrote: >>> This is loosly 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> >> >> 2 high level remarks here: >> >> 1. I believe we should strive for having all issues with this driver fixed >> before merging it, at which point it should not sit under drivers/staging >> but rather under drivers/platform/x86 (as an added bonus this can also make >> toshiba_apci's Kconfig bit select it automatically). So for the next version >> please move this to drivers/platform/x86 > > Makes sense, will do. However, there is nothing x86 specific in theory with > this driver. Would it not make more sense to put it under drivers/acpi? Since the spec is from Microsoft I expect it to be a x86 only thing (AFAIK this predates there couple of ARM attempts). Also since this has some tie-in with toshiba-acpi (at least for the laptops you are actually testing this on) keeping it in the same dir as toshiba-acpi seems to make the most sense to me. A lot of ACPI drivers actually live under drivers/platform/x86 for similar reasons. >> 2. This is using struct acpi_driver, but as Rafael (ACPI maintainer) always >> said that is really only for very special cases. The ACPI subsystem should >> instantiate standard platform devices for each PNP0C32 device, you can >> check this under: /sys/bus/devices/platform. And this driver should then >> be convered to a standard platform_driver binding to the platform devices >> instead of being a struct acpi_driver. > > I had a look at this, and it seems like a much more complicated a approach, > for example, there is no dedicated .ops.notify, which means I need to use > acpi_install_notify_handler, and there is no devm_ version of that either. > A lot of other things seem to be ever so slightly more complicated as well. > > What is the motivation behind this being preferred? And are most of the > existing drivers using acpi_driver legacy (e.g. toshiba_acpi)? I'm mostly just parroting (repeating) the party-line here. Not making new drivers an acpi_driver is typically requested by Rafael, the ACPI maintainer. Rafael can you explain why? From my own view point I guess this has to do with ACPI having changed over time from mostly offering firmware interfaces which mostly talk to the EC, to actually also describing the hw. Now a days of lot of ACPI devices are actually describing real hardware devices, e.g. PCI cards, I2C devices, SPI decices and UART attached devices including things like IO / MMIO addresses, I2C slave addresses, SPI chip selects, GPIOs, IRQs, etc. So the kernel now a days instantiates actual SPI / I2C / UART / PCI / other(platform) devices for all the devices in ACPI, with all the physicial resources attached to the struct platform_device / struct i2c_client / etc. using the kernels standard resource mechanisms. The struct platform_device / struct i2c_client / etc. devices then have a firmware_node pointer (aka companion device) pointing to the ACPI device in case the driver also needs to make some actual ACPI calls on either to query some extra information stored in ACPI, or sometimes to enable / disable special features driven through ACPI methods. The instantiation of these struct platform_device / struct i2c_client / etc. devices is done through a special default/fallback acpi_driver. If you attach your own acpi_driver to an acpi_device then this will not happen. So for any devices which also have some physical part (and not just pure firmware interface) using an acpi_driver is not what you want. At which point I guess we simply just want to avoid it even for pure virtual/fw devices like the PNP0C32 case for consistency. Note this is just my view on this, perhaps Rafael can explain this better? Regards, Hans
diff --git a/drivers/staging/Kconfig b/drivers/staging/Kconfig index 3bd80f9695ac..db89ffbcd1ad 100644 --- a/drivers/staging/Kconfig +++ b/drivers/staging/Kconfig @@ -50,6 +50,8 @@ source "drivers/staging/iio/Kconfig" source "drivers/staging/sm750fb/Kconfig" +source "drivers/staging/quickstart/Kconfig" + source "drivers/staging/emxx_udc/Kconfig" source "drivers/staging/nvec/Kconfig" diff --git a/drivers/staging/Makefile b/drivers/staging/Makefile index 1d9ae39fea14..cb92880f7db5 100644 --- a/drivers/staging/Makefile +++ b/drivers/staging/Makefile @@ -16,6 +16,7 @@ obj-$(CONFIG_VT6656) += vt6656/ obj-$(CONFIG_VME_BUS) += vme_user/ obj-$(CONFIG_IIO) += iio/ obj-$(CONFIG_FB_SM750) += sm750fb/ +obj-$(CONFIG_ACPI_QUICKSTART) += quickstart/ obj-$(CONFIG_USB_EMXX) += emxx_udc/ obj-$(CONFIG_MFD_NVEC) += nvec/ obj-$(CONFIG_STAGING_BOARD) += board/ diff --git a/drivers/staging/quickstart/Kconfig b/drivers/staging/quickstart/Kconfig new file mode 100644 index 000000000000..e1cf1810e967 --- /dev/null +++ b/drivers/staging/quickstart/Kconfig @@ -0,0 +1,12 @@ +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. + diff --git a/drivers/staging/quickstart/Makefile b/drivers/staging/quickstart/Makefile new file mode 100644 index 000000000000..290e0e476797 --- /dev/null +++ b/drivers/staging/quickstart/Makefile @@ -0,0 +1 @@ +obj-$(CONFIG_ACPI_QUICKSTART) += quickstart.o diff --git a/drivers/staging/quickstart/quickstart.c b/drivers/staging/quickstart/quickstart.c new file mode 100644 index 000000000000..8d76472c6b7f --- /dev/null +++ b/drivers/staging/quickstart/quickstart.c @@ -0,0 +1,376 @@ +// 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 QUICKSTART_VERSION "1.04" + +#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/input.h> +#include <linux/input/sparse-keymap.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_CLASS "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 acpi_device *acpi_dev; + struct input_dev *input_device; + struct quickstart_button *button; + /* Name of button for debug messages */ + char *name; + /* ID of button as returned by GHID */ + u32 id; + /* Flags for cleanup */ + unsigned int input_registered : 1; + unsigned int sysfs_created : 1; + /* Track if a wakeup event was received */ + unsigned int wakeup_cause : 1; + /* Name of input device */ + char input_name[32]; + /* Physical path for the input device */ + char phys[32]; +}; + +/* + * 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 scnprintf(buf, PAGE_SIZE, "%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 scnprintf(buf, PAGE_SIZE, "%d\n", quickstart->id); +} +static DEVICE_ATTR_RO(button_id); + +/* ACPI Driver functions */ +static void quickstart_acpi_notify(struct acpi_device *acpi_dev, u32 event) +{ + struct quickstart_acpi *quickstart = acpi_driver_data(acpi_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_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(quickstart->acpi_dev->handle, "GHID", + NULL, &buffer); + if (ACPI_FAILURE(status)) { + pr_err("%s GHID method failed\n", quickstart->name); + return -EINVAL; + } + obj = buffer.pointer; + + /* + * GHID returns buffers, sanity check that is the case. + */ + if (obj->type != ACPI_TYPE_BUFFER) { + pr_err("%s GHID did not return buffer\n", quickstart->name); + return -EINVAL; + } + + /* + * 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 = le16_to_cpu(*(u16 *)obj->buffer.pointer); + break; + case 4: + quickstart->id = le32_to_cpu(*(u32 *)obj->buffer.pointer); + break; + case 8: + quickstart->id = le64_to_cpu(*(u64 *)obj->buffer.pointer); + break; + default: + pr_err("%s GHID method returned buffer of unexpected length %lu\n", + quickstart->name, (unsigned long)obj->buffer.length); + ret = -EINVAL; + break; + } + + kfree(buffer.pointer); + + return ret; +} + +static int quickstart_acpi_config(struct quickstart_acpi *quickstart) +{ + char *bid = acpi_device_bid(quickstart->acpi_dev); + char *name; + + name = kmalloc(strlen(bid) + 1, GFP_KERNEL); + if (!name) + return -ENOMEM; + + quickstart->name = name; + strcpy(quickstart->name, bid); + + return 0; +} + +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_acpi_remove(struct acpi_device *acpi_dev) +{ + struct quickstart_acpi *quickstart; + + if (!acpi_dev) + return -EINVAL; + + quickstart = acpi_driver_data(acpi_dev); + if (!quickstart) + return -EINVAL; + + if (quickstart->sysfs_created) + sysfs_remove_group(&quickstart->acpi_dev->dev.kobj, + &quickstart_attr_group); + + kfree(quickstart->name); + quickstart->name = NULL; + + kfree(quickstart); + + return 0; +} + +static int quickstart_acpi_add(struct acpi_device *acpi_dev) +{ + int ret; + struct quickstart_acpi *quickstart; + + if (!acpi_dev) + return -EINVAL; + + quickstart = kzalloc(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. + */ + acpi_dev->driver_data = quickstart; + quickstart->acpi_dev = acpi_dev; + dev_set_drvdata(&acpi_dev->dev, quickstart); + + strcpy(acpi_device_name(acpi_dev), QUICKSTART_ACPI_DEVICE_NAME); + strcpy(acpi_device_class(acpi_dev), QUICKSTART_ACPI_CLASS); + + /* Initialize device name */ + ret = quickstart_acpi_config(quickstart); + if (ret < 0) + goto error; + + /* Retrieve the GHID ID */ + ret = quickstart_acpi_ghid(quickstart); + if (ret < 0) + goto error; + + /* Set up sysfs entries */ + ret = sysfs_create_group(&quickstart->acpi_dev->dev.kobj, + &quickstart_attr_group); + if (ret) { + quickstart->sysfs_created = 0; + pr_err("Unable to setup sysfs entries\n"); + goto error; + } + quickstart->sysfs_created = !ret; + + /* Set up input device */ + quickstart->input_device = + devm_input_allocate_device(&quickstart->acpi_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); + if (ret) { + quickstart->input_registered = 0; + pr_err("Unable to register input device\n"); + goto error; + } + quickstart->input_registered = !ret; + + return 0; +error: + quickstart_acpi_remove(acpi_dev); + 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 acpi_driver quickstart_acpi_driver = { + .name = "quickstart", + .owner = THIS_MODULE, + .class = QUICKSTART_ACPI_CLASS, + .ids = quickstart_device_ids, + .flags = ACPI_DRIVER_ALL_NOTIFY_EVENTS, + .ops = { + .add = quickstart_acpi_add, + .remove = quickstart_acpi_remove, + .notify = quickstart_acpi_notify + }, +}; + +/* Module functions */ +static void quickstart_exit(void) +{ + acpi_bus_unregister_driver(&quickstart_acpi_driver); +} + +static int __init quickstart_init(void) +{ + int ret; + + /* ACPI driver register */ + ret = acpi_bus_register_driver(&quickstart_acpi_driver); + if (ret) + return ret; + + pr_info("ACPI Direct App Launch ver %s\n", QUICKSTART_VERSION); + + return 0; +} + +module_init(quickstart_init); +module_exit(quickstart_exit);
This is loosly 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/staging/Kconfig | 2 + drivers/staging/Makefile | 1 + drivers/staging/quickstart/Kconfig | 12 + drivers/staging/quickstart/Makefile | 1 + drivers/staging/quickstart/quickstart.c | 376 ++++++++++++++++++++++++ 5 files changed, 392 insertions(+) create mode 100644 drivers/staging/quickstart/Kconfig create mode 100644 drivers/staging/quickstart/Makefile create mode 100644 drivers/staging/quickstart/quickstart.c