[PATCH RFC 0/2] Quickstart buttons driver and Toshiba Z830

[PATCH RFC 0/2] Quickstart buttons driver and Toshiba Z830

[Arvid Norlander]

Hi,

In the following patch series I implement support for three buttons on
the Toshiba Satellite/Portege Z830 (same laptop, different markets).

These buttons work via a PNP0C32 ACPI device. Hans de Goede pointed out
an old and flawed attempt to implement this as a staging driver.

With that staging driver as a starting point I have now implemented proper
support. I believe I have fixed the flaws with the original staging driver.
As it required almost a complete rewrite I have decided to present it as a
new driver instead of starting with a revert commit to restore the old
driver and then apply fixes on top.

The specification for PNP0C32 devices exist as a Microsoft specification.
It was previously available on their web site, but seems to have been taken
down during the last month. I had a local copy and I have uploaded it to
archive.org. It is available here for anyone interested (including a
conversion of the docx to PDF):

https://archive.org/details/microsoft-acpi-dirapplaunch

The old emails about support for these buttons can be found at:
https://marc.info/?l=linux-acpi&m=120550727131007
https://lkml.org/lkml/2010/5/28/327

Table of contents:
1. Summary of standard
2. Issues
2.1. Issue 1: Wake support
2.2. Issue 2: Button identification
2.3. Issue 3: GHID: 64-bit values?
2.4. Issue 4: MAINTAINERS?
2.5. Issue 5: Where does the driver go?
3. User space API
3.1. Input device
3.2. Sysfs file: button_id (Read only)
3.3. Sysfs file: wakeup_cause (Read write)
4. HCI_HOTKEY_EVENT register (toshiba_acpi)


1. Summary of standard
======================

Here is a brief high level summary of the standard for PNP0C32. See
https://archive.org/details/microsoft-acpi-dirapplaunch for the full
standard.

PNP0C32 devices are "Direct Application Launch" buttons. The idea is that
they should work while the laptop is in various sleep modes (or even off).
The Z830 does not support waking from any sleep mode using these buttons,
it only supports them while it is awake.

Each PNP0C32 device represents a single button. Their meaning is completely
vendor defined. On Windows you can either:
* Make them launch an application when pressed (defined in the registry)
* Or an application can subscribe to specific Window messages to get
  notified when they are pressed (this is how they are used by the Toshiba
  software).

2. Issues
=========
Unfortunately there are a few issues where I would like some input.

On top of that I'm sure there are lots of issues as I'm fairly new to
kernel programming!

2.1. Issue 1: Wake support
------------
This is untested as the Toshiba Z830 that I have simply does not support
this part in the firmware. I left the old behaviour in and only adapted it
slightly.

The driver adds a sysfs file "wakeup_cause" to each PNP0C32 device
(inspired by old approach) that would read "true" after causing the wakeup.
It would be up to user space query this and reset the value to false.
This is basically what the old staging driver did, only moved from an
(un-needed) platform driver to each ACPI driver.

As I cannot test it (the Z830 does not support the wakeup part of the spec)
I'm more inclined to just drop this feature, especially if the current
approach is suboptimal. It would then be up to someone else to implement
this in the future.

2.2. Issue 2: Button identification
------------
There is NO generic way to know what the buttons are "supposed" to do.
Each button has a vendor defined ID (an 8-, 16- or 32-bit unsigned integer).
This ID can be read with the GHID ACPI method.

As such I map all these to KEY_UNKNOWN. Then suitable hwdb entries can be
created for udev that remap these to some sort of meaningful values.

Here is an example hwdb file I created for my laptop:
$ cat /etc/udev/hwdb.d/quickstart.hwdb 
evdev:name:Quickstart Button 1:dmi:bvn*:bvr*:bd*:svnTOSHIBA:pn*Z830:*
 KEYBOARD_KEY_01=prog1

evdev:name:Quickstart Button 2:dmi:bvn*:bvr*:bd*:svnTOSHIBA:pn*Z830:*
 KEYBOARD_KEY_01=prog2

evdev:name:Quickstart Button 3:dmi:bvn*:bvr*:bd*:svnTOSHIBA:pn*Z830:*
 KEYBOARD_KEY_01=touchpad_toggle

As can be seen I always use the scancode 1 here. Would it be better to use
the ID from GHID instead? This can be an arbitrary 32-bit value.

Note also that prog1 and prog2 are poor approximations of the real buttons.
In reality the buttons are "Eco mode" and "Open Windows Mobility center on
screen about switching to projection mode". However Linux seem to lack
suitable key definitions for these.

2.3. Issue 3: GHID: 64-bit values?
------------
The old staging driver had support for GHID returning a 64-bit value. It is
not clear to me why, as it is not mentioned in the specification. I could
not find anything when reading the old emails either. As such, I'm unsure
if I should drop it. The variable this gets stored to is just 32-bit
anyway.

If we decide to use GHID for scancode (see "Issue 2"), 64-bit values
might be a problem, as the scan code field is only 32 bits.

2.4. Issue 4: MAINTAINERS?
------------
I got this from checkpatch.pl:
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?

I'm not sure? Advice would be welcome.

2.5. Issue 5: Where does the driver go?
------------
For now, I have left this in staging as with the original driver. I'm not
sure if this is the right approach. Apart from the question of wakeup
handling, I believe the driver to be feature-complete. Advise is welcome. 


3. User space API
=================
Currently the user space API is as a sparse keymap input device, plus two
unique sysfs files. Discussion on this is welcome!

3.1. Input device
----------------
The device produces KEY_UNKNOWN events when the button is pressed, with
the scan code 1. We could change the scan code to the button ID reported
by ACPI via GHID. See also "Issue 2" and "Issue 3" above.

3.2. Sysfs file: button_id (Read only)
-------------------------
This file can be read to get the button ID as reported by GHID. It is
returned in human readable ASCII with a trailing newline.

3.3. Sysfs file: wakeup_cause (Read write)
-----------------------------
Will return "true\n" when read after the button was the wakeup cause.
This is latched until user space writes "false" to the file.

See also "Issue 1" above. If this is not a suitable interface I'm inclined
to just drop the wakeup handling entirely.


4. HCI_HOTKEY_EVENT register (toshiba_acpi)
============================
To enable quickstart hotkeys, the HCI_HOTKEY_EVENT (0x1e) register needs
to be set correctly by toshiba_acpi. toshiba_acpi currently sets this to
HCI_HOTKEY_ENABLE (0x9) on the Z830. This is not suitable.

* Windows drivers reads the register then sets it to 0x5. Presumably there
  is some logic going on there.
* HCI_GET on HCI_HOTKEY_EVENT returns 0xf before first call to set it when
  booting Linux on this laptop.
* From my testing any value between 1 and 7 (inclusive) gives the correct
  behaviour for the quickstart buttons. However, for normal hotkeys to work
  in toshiba_acpi, only values with the least significant bit set work.

Toshiba_acpi already detects some laptops using SCI_KBD_FUNCTION_KEYS. That
call is not supported on this laptop (return status TOS_NOT_SUPPORTED).

It is not clear to me how to detect when to use the 0x5 value. In the
attached patch I use a quirk table to enable this. There may be a better
way to do it.

Best regards,
Arvid Norlander

Note! This series is based off the review-hans branch.

Arvid Norlander (2):
  staging: quickstart: Add ACPI quickstart button (PNP0C32) driver
  platform/x86: toshiba_acpi: Add quirk for buttons on Z830

 drivers/platform/x86/toshiba_acpi.c     |  31 +-
 drivers/staging/Kconfig                 |   2 +
 drivers/staging/Makefile                |   1 +
 drivers/staging/quickstart/Kconfig      |  12 +
 drivers/staging/quickstart/Makefile     |   1 +
 drivers/staging/quickstart/quickstart.c | 376 ++++++++++++++++++++++++
 6 files changed, 422 insertions(+), 1 deletion(-)
 create mode 100644 drivers/staging/quickstart/Kconfig
 create mode 100644 drivers/staging/quickstart/Makefile
 create mode 100644 drivers/staging/quickstart/quickstart.c


base-commit: b8bad0fbf4366928266eb9fba7430a011edc321e
prerequisite-patch-id: 5b404f0b72a3a4a5d38250578956213c8c7733bd
prerequisite-patch-id: be8533cbf34f7aa97f9d59a26a5edb6691096403
-- 
2.37.3

[PATCH RFC 1/2] staging: quickstart: Add ACPI quickstart button (PNP0C32) driver

[Arvid Norlander]

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

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);
-- 
2.37.3

[PATCH RFC 2/2] platform/x86: toshiba_acpi: Add quirk for buttons on Z830

[Arvid Norlander]

The Z830 has some buttons that will only work properly as "quickstart"
buttons. To enable them in that mode, a value between 1 and 7 must be
used for HCI_HOTKEY_EVENT. Windows uses 0x5 on this laptop so use that for
maximum predictability and compatibility.

As there is not yet a known way of auto detection, this patch uses a DMI
quirk table. A module parameter is exposed to allow setting this on other
models for testing.

Signed-off-by: Arvid Norlander <lkml@vorpal.se>
---
 drivers/platform/x86/toshiba_acpi.c | 31 ++++++++++++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
index 9f1394b73895..dce1f5049bf8 100644
--- a/drivers/platform/x86/toshiba_acpi.c
+++ b/drivers/platform/x86/toshiba_acpi.c
@@ -58,6 +58,11 @@ module_param(turn_on_panel_on_resume, int, 0644);
 MODULE_PARM_DESC(turn_on_panel_on_resume,
 	"Call HCI_PANEL_POWER_ON on resume (-1 = auto, 0 = no, 1 = yes");
 
+static int hci_hotkey_quickstart = -1;
+module_param(hci_hotkey_quickstart, int, 0644);
+MODULE_PARM_DESC(hci_hotkey_quickstart,
+	"Call HCI_HOTKEY_EVENT with value 0x5 for quickstart button support (-1 = auto, 0 = no, 1 = yes");
+
 #define TOSHIBA_WMI_EVENT_GUID "59142400-C6A3-40FA-BADB-8A2652834100"
 
 /* Scan code for Fn key on TOS1900 models */
@@ -137,6 +142,7 @@ MODULE_PARM_DESC(turn_on_panel_on_resume,
 #define HCI_ACCEL_MASK			0x7fff
 #define HCI_ACCEL_DIRECTION_MASK	0x8000
 #define HCI_HOTKEY_DISABLE		0x0b
+#define HCI_HOTKEY_ENABLE_QUICKSTART	0x05
 #define HCI_HOTKEY_ENABLE		0x09
 #define HCI_HOTKEY_SPECIAL_FUNCTIONS	0x10
 #define HCI_LCD_BRIGHTNESS_BITS		3
@@ -2731,10 +2737,15 @@ static int toshiba_acpi_enable_hotkeys(struct toshiba_acpi_dev *dev)
 		return -ENODEV;
 
 	/*
+	 * Enable quickstart buttons if supported.
+	 *
 	 * Enable the "Special Functions" mode only if they are
 	 * supported and if they are activated.
 	 */
-	if (dev->kbd_function_keys_supported && dev->special_functions)
+	if (hci_hotkey_quickstart)
+		result = hci_write(dev, HCI_HOTKEY_EVENT,
+				   HCI_HOTKEY_ENABLE_QUICKSTART);
+	else if (dev->kbd_function_keys_supported && dev->special_functions)
 		result = hci_write(dev, HCI_HOTKEY_EVENT,
 				   HCI_HOTKEY_SPECIAL_FUNCTIONS);
 	else
@@ -3287,6 +3298,21 @@ static const struct dmi_system_id turn_on_panel_on_resume_dmi_ids[] = {
 	},
 };
 
+/*
+ * Some Toshibas use "quickstart" keys. On these, HCI_HOTKEY_EVENT must use
+ * the value HCI_HOTKEY_ENABLE_QUICKSTART.
+ */
+static const struct dmi_system_id hci_hotkey_quickstart_dmi_ids[] = {
+	{
+	 /* Toshiba Satellite/Portégé Z830 */
+	 .matches = {
+		DMI_MATCH(DMI_SYS_VENDOR, "TOSHIBA"),
+		DMI_MATCH(DMI_PRODUCT_NAME, "Z830"),
+		},
+	},
+};
+
+
 static int toshiba_acpi_add(struct acpi_device *acpi_dev)
 {
 	struct toshiba_acpi_dev *dev;
@@ -3447,6 +3473,9 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
 	if (turn_on_panel_on_resume == -1)
 		turn_on_panel_on_resume = dmi_check_system(turn_on_panel_on_resume_dmi_ids);
 
+	if (hci_hotkey_quickstart == -1)
+		hci_hotkey_quickstart = dmi_check_system(hci_hotkey_quickstart_dmi_ids);
+
 	toshiba_wwan_available(dev);
 	if (dev->wwan_supported)
 		toshiba_acpi_setup_wwan_rfkill(dev);
-- 
2.37.3

Re: [PATCH RFC 0/2] Quickstart buttons driver and Toshiba Z830

[Barnabás Pőcze]

2022. szeptember 11., vasárnap 21:49 keltezéssel, Arvid Norlander írta:

> [...]
>
> 1. Summary of standard
> ======================
>
> Here is a brief high level summary of the standard for PNP0C32. See
> https://archive.org/details/microsoft-acpi-dirapplaunch for the full
> standard.
>
> PNP0C32 devices are "Direct Application Launch" buttons. The idea is that
> they should work while the laptop is in various sleep modes (or even off).
> The Z830 does not support waking from any sleep mode using these buttons,
> it only supports them while it is awake.
>

Hi

I might be way off here, but could it not be that one has to enable/allow
the device to be able to wake the system up? Or did you test it on windows
as well and it did not work there either?


Regards,
Barnabás Pőcze

> Each PNP0C32 device represents a single button. Their meaning is completely
> vendor defined. On Windows you can either:
> * Make them launch an application when pressed (defined in the registry)
> * Or an application can subscribe to specific Window messages to get
> notified when they are pressed (this is how they are used by the Toshiba
> software).
> [...]

Re: [PATCH RFC 0/2] Quickstart buttons driver and Toshiba Z830

[Arvid Norlander]

On 2022-09-12 00:58, Barnabás Pőcze wrote:
> 
> 2022. szeptember 11., vasárnap 21:49 keltezéssel, Arvid Norlander írta:
> 
>> [...]
>>
>> 1. Summary of standard
>> ======================
>>
>> Here is a brief high level summary of the standard for PNP0C32. See
>> https://archive.org/details/microsoft-acpi-dirapplaunch for the full
>> standard.
>>
>> PNP0C32 devices are "Direct Application Launch" buttons. The idea is that
>> they should work while the laptop is in various sleep modes (or even off).
>> The Z830 does not support waking from any sleep mode using these buttons,
>> it only supports them while it is awake.
>>
> 
> Hi
> 
> I might be way off here, but could it not be that one has to enable/allow
> the device to be able to wake the system up? Or did you test it on windows
> as well and it did not work there either?

Hi,

It does indeed not work under Windows either. In addition, the Toshiba ACPI
implementation is incomplete: the "required" _PRW method is missing. And
only about half of what would be needed to handle the wakeup button
identification is actually present in the DSDT as far as I can tell.

Furthermore, given what Toshiba uses the buttons for, waking from sleep
with them make little sense. One button is to toggle the touchpad on/off,
one is to select external display mode (mirrored, extended, ...) and one
is to toggle between normal and low power "eco mode" (it basically dims the
screen and limits the CPU speed under Windows).

I believe Toshiba for whatever reason simply decided to piggy back on this
standard for some strange reason for their buttons. This is odd as
toshiba_acpi already implements Fn key handling (which this laptop also
has). Who knows why they couldn't just hook up the physical buttons as
special keys. Because that is the only thing that sets these apart
physically: They are buttons, not keys.

Best regards,
Arvid Norlander

> 
> 
> Regards,
> Barnabás Pőcze
> 
>> Each PNP0C32 device represents a single button. Their meaning is completely
>> vendor defined. On Windows you can either:
>> * Make them launch an application when pressed (defined in the registry)
>> * Or an application can subscribe to specific Window messages to get
>> notified when they are pressed (this is how they are used by the Toshiba
>> software).
>> [...]

Re: [PATCH RFC 1/2] staging: quickstart: Add ACPI quickstart button (PNP0C32) driver

[Barnabás Pőcze]

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

Re: [PATCH RFC 1/2] staging: quickstart: Add ACPI quickstart button (PNP0C32) driver

[Arvid Norlander]

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

Re: [PATCH RFC 1/2] staging: quickstart: Add ACPI quickstart button (PNP0C32) driver

[Hans de Goede]

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);

Re: [PATCH RFC 2/2] platform/x86: toshiba_acpi: Add quirk for buttons on Z830

[Hans de Goede]

Hi,

On 9/11/22 20:49, Arvid Norlander wrote:
> The Z830 has some buttons that will only work properly as "quickstart"
> buttons. To enable them in that mode, a value between 1 and 7 must be
> used for HCI_HOTKEY_EVENT. Windows uses 0x5 on this laptop so use that for
> maximum predictability and compatibility.
> 
> As there is not yet a known way of auto detection, this patch uses a DMI
> quirk table. A module parameter is exposed to allow setting this on other
> models for testing.
> 
> Signed-off-by: Arvid Norlander <lkml@vorpal.se>

Instead if adding a second dmi_system_id table please convert the existing one
into a generic quirk table. The idea is to make it look something like this:

#define QUIRK_TURN_ON_PANEL_ON_RESUME		BIT(0)
#define QUIRK_HCI_HOTKEY_QUICKSTART		BIT(1)

static const struct dmi_system_id toshiba_dmi_quirks[] = {
	{
	 /* Toshiba Portégé R700 */
	 /* https://bugzilla.kernel.org/show_bug.cgi?id=21012 */
	 .matches = {
		DMI_MATCH(DMI_SYS_VENDOR, "TOSHIBA"),
		DMI_MATCH(DMI_PRODUCT_NAME, "PORTEGE R700"),
		},
	 .driver_data = (void *)QUIRK_TURN_ON_PANEL_ON_RESUME,
	},
	{
	 /* Toshiba Satellite/Portégé R830 */
	 /* Portégé: https://bugs.freedesktop.org/show_bug.cgi?id=82634 */
	 /* Satellite: https://bugzilla.kernel.org/show_bug.cgi?id=21012 */
	 .matches = {
		DMI_MATCH(DMI_SYS_VENDOR, "TOSHIBA"),
		DMI_MATCH(DMI_PRODUCT_NAME, "R830"),
		},
	 .driver_data = (void *)QUIRK_TURN_ON_PANEL_ON_RESUME,
	},
	{
	 /* Toshiba Satellite/Portégé Z830 */
	 .matches = {
		DMI_MATCH(DMI_SYS_VENDOR, "TOSHIBA"),
		DMI_MATCH(DMI_PRODUCT_NAME, "Z830"),
		},
	 .driver_data = (void *)QUIRK_TURN_ON_PANEL_ON_RESUME | QUIRK_HCI_HOTKEY_QUICKSTART,
	},
};

And then in toshiba_acpi_add() use:

	const struct dmi_system_id *dmi_id;
	long quirks = 0;

	dmi_id = dmi_first_match(toshiba_dmi_quirks);
	if (dmi_id)
		quirks = (long)dmi_id->driver_data;

	if (turn_on_panel_on_resume == -1)
		turn_on_panel_on_resume = !!(quirks & QUIRK_TURN_ON_PANEL_ON_RESUME);

	if (hci_hotkey_quickstart == -1)
		hci_hotkey_quickstart = !!(quirks & QUIRK_HCI_HOTKEY_QUICKSTART);

This avoids duplicating the DMI match for the Z830 models and will also make it easier
to add more DMI based quirks in the future.

Regards,

Hans




> ---
>  drivers/platform/x86/toshiba_acpi.c | 31 ++++++++++++++++++++++++++++-
>  1 file changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
> index 9f1394b73895..dce1f5049bf8 100644
> --- a/drivers/platform/x86/toshiba_acpi.c
> +++ b/drivers/platform/x86/toshiba_acpi.c
> @@ -58,6 +58,11 @@ module_param(turn_on_panel_on_resume, int, 0644);
>  MODULE_PARM_DESC(turn_on_panel_on_resume,
>  	"Call HCI_PANEL_POWER_ON on resume (-1 = auto, 0 = no, 1 = yes");
>  
> +static int hci_hotkey_quickstart = -1;
> +module_param(hci_hotkey_quickstart, int, 0644);
> +MODULE_PARM_DESC(hci_hotkey_quickstart,
> +	"Call HCI_HOTKEY_EVENT with value 0x5 for quickstart button support (-1 = auto, 0 = no, 1 = yes");
> +
>  #define TOSHIBA_WMI_EVENT_GUID "59142400-C6A3-40FA-BADB-8A2652834100"
>  
>  /* Scan code for Fn key on TOS1900 models */
> @@ -137,6 +142,7 @@ MODULE_PARM_DESC(turn_on_panel_on_resume,
>  #define HCI_ACCEL_MASK			0x7fff
>  #define HCI_ACCEL_DIRECTION_MASK	0x8000
>  #define HCI_HOTKEY_DISABLE		0x0b
> +#define HCI_HOTKEY_ENABLE_QUICKSTART	0x05
>  #define HCI_HOTKEY_ENABLE		0x09
>  #define HCI_HOTKEY_SPECIAL_FUNCTIONS	0x10
>  #define HCI_LCD_BRIGHTNESS_BITS		3
> @@ -2731,10 +2737,15 @@ static int toshiba_acpi_enable_hotkeys(struct toshiba_acpi_dev *dev)
>  		return -ENODEV;
>  
>  	/*
> +	 * Enable quickstart buttons if supported.
> +	 *
>  	 * Enable the "Special Functions" mode only if they are
>  	 * supported and if they are activated.
>  	 */
> -	if (dev->kbd_function_keys_supported && dev->special_functions)
> +	if (hci_hotkey_quickstart)
> +		result = hci_write(dev, HCI_HOTKEY_EVENT,
> +				   HCI_HOTKEY_ENABLE_QUICKSTART);
> +	else if (dev->kbd_function_keys_supported && dev->special_functions)
>  		result = hci_write(dev, HCI_HOTKEY_EVENT,
>  				   HCI_HOTKEY_SPECIAL_FUNCTIONS);
>  	else
> @@ -3287,6 +3298,21 @@ static const struct dmi_system_id turn_on_panel_on_resume_dmi_ids[] = {
>  	},
>  };
>  
> +/*
> + * Some Toshibas use "quickstart" keys. On these, HCI_HOTKEY_EVENT must use
> + * the value HCI_HOTKEY_ENABLE_QUICKSTART.
> + */
> +static const struct dmi_system_id hci_hotkey_quickstart_dmi_ids[] = {
> +	{
> +	 /* Toshiba Satellite/Portégé Z830 */
> +	 .matches = {
> +		DMI_MATCH(DMI_SYS_VENDOR, "TOSHIBA"),
> +		DMI_MATCH(DMI_PRODUCT_NAME, "Z830"),
> +		},
> +	},
> +};
> +
> +
>  static int toshiba_acpi_add(struct acpi_device *acpi_dev)
>  {
>  	struct toshiba_acpi_dev *dev;
> @@ -3447,6 +3473,9 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
>  	if (turn_on_panel_on_resume == -1)
>  		turn_on_panel_on_resume = dmi_check_system(turn_on_panel_on_resume_dmi_ids);
>  
> +	if (hci_hotkey_quickstart == -1)
> +		hci_hotkey_quickstart = dmi_check_system(hci_hotkey_quickstart_dmi_ids);
> +
>  	toshiba_wwan_available(dev);
>  	if (dev->wwan_supported)
>  		toshiba_acpi_setup_wwan_rfkill(dev);

Re: [PATCH RFC 1/2] staging: quickstart: Add ACPI quickstart button (PNP0C32) driver

[Arvid Norlander]

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>

Re: [PATCH RFC 1/2] staging: quickstart: Add ACPI quickstart button (PNP0C32) driver

[Hans de Goede]

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