From: Hans de Goede <hdegoede@redhat.com>
To: "Barnabás Pőcze" <pobrn@protonmail.com>,
"Arvid Norlander" <lkml@vorpal.se>
Cc: platform-driver-x86@vger.kernel.org, linux-acpi@vger.kernel.org,
Len Brown <lenb@kernel.org>,
"Rafael J. Wysocki" <rafael@kernel.org>,
linux-input@vger.kernel.org,
Azael Avalos <coproscefalo@gmail.com>
Subject: Re: [PATCH RFC v2 1/2] platform/x86: quickstart: Add ACPI quickstart button (PNP0C32) driver
Date: Tue, 27 Sep 2022 15:54:18 +0200 [thread overview]
Message-ID: <8b8af307-c0b3-f42b-f470-a4778e814893@redhat.com> (raw)
In-Reply-To: <4-mkye9NM7L93IKQAGjd8BmHi1_2zEnx4F8L3AvKk9RsNBtuoS5cpNCKV-nyb1Xpb1jmAZQDdpNlyvjoUfrFKkq4V-EOfXo9b_gRbyC1hSs=@protonmail.com>
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
next prev parent reply other threads:[~2022-09-27 13:54 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-22 18:24 [PATCH RFC v2 0/2] Quickstart buttons driver and Toshiba Z830 Arvid Norlander
2022-09-22 18:24 ` [PATCH RFC v2 1/2] platform/x86: quickstart: Add ACPI quickstart button (PNP0C32) driver Arvid Norlander
2022-09-23 19:24 ` Barnabás Pőcze
2022-09-25 18:19 ` Arvid Norlander
2022-09-27 13:35 ` Hans de Goede
2022-09-27 13:49 ` Hans de Goede
2022-09-27 14:54 ` Arvid Norlander
2022-09-27 13:54 ` Hans de Goede [this message]
2022-09-27 13:54 ` Hans de Goede
2022-09-22 18:24 ` [PATCH v2 2/2] platform/x86: toshiba_acpi: Add quirk for buttons on Z830 Arvid Norlander
2022-09-27 13:56 ` Hans de Goede
2022-10-07 11:42 ` [PATCH RFC v2 0/2] Quickstart buttons driver and Toshiba Z830 Hans de Goede
2022-10-07 23:44 ` Hans de Goede
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8b8af307-c0b3-f42b-f470-a4778e814893@redhat.com \
--to=hdegoede@redhat.com \
--cc=coproscefalo@gmail.com \
--cc=lenb@kernel.org \
--cc=linux-acpi@vger.kernel.org \
--cc=linux-input@vger.kernel.org \
--cc=lkml@vorpal.se \
--cc=platform-driver-x86@vger.kernel.org \
--cc=pobrn@protonmail.com \
--cc=rafael@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).