Re: [PATCH] ACPI: Add GPIO-signaled event emulator.
From: Tomasz Nowicki
Date: Thu Aug 21 2014 - 10:40:09 EST
Hi Mika,
On 21.08.2014 12:45, Mika Westerberg wrote:
On Wed, Aug 20, 2014 at 04:58:20PM +0200, Tomasz Nowicki wrote:
GPIO-signaled events is quite new thing in Linux kernel.
There are not many board which can take advantage of it.
However, GPIO events are very useful feature during work on ACPI
subsystems.
This commit emulates GPIO h/w behaviour and consists on write
operation to debugfs file. GPIO device instance is still required in DSDT
table along with _AEI resources and event methods.
Please, see Kconfig help and driver head section for more details
regarding tool usage.
Signed-off-by: Tomasz Nowicki <tomasz.nowicki@xxxxxxxxxx>
---
drivers/acpi/Kconfig | 10 ++
drivers/acpi/Makefile | 1 +
drivers/acpi/gpio-acpi-evt-emu.c | 195 +++++++++++++++++++++++++++++++++++++++
3 files changed, 206 insertions(+)
create mode 100644 drivers/acpi/gpio-acpi-evt-emu.c
diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index fd54a74..8b9b74d 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -122,6 +122,16 @@ config ACPI_BUTTON
To compile this driver as a module, choose M here:
the module will be called button.
+config ACPI_GPIO_EVT_EMULATE
+ bool "ACPI GPIO-signaled Events Emulation Support"
Is there anything preventing building this as a module?
It should be tristate instead of bool statement here. Thanks for remind me.
+ depends on DEBUG_FS
Spaces -> Tab
+ default n
n is the default already, no need to specify it here.
+ help
+ This will enable your system to emulate GPIO-signaled event through
+ proc file system. User needs to trigger event method by
+ echo 1 > /sys/kernel/debug/acpi/events/<GPIO DEVICE>/<PIN>
+ (where, GPIO DEVICE is a GPIO device name and PIN is a pin number).
We should probably mention that this is dangerous and should be used for
debugging purposes only.
Good point!
+
config ACPI_VIDEO
tristate "Video"
depends on X86 && BACKLIGHT_CLASS_DEVICE
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index 9fa20ff..24f9d8f 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -55,6 +55,7 @@ acpi-$(CONFIG_ACPI_PROCFS_POWER) += cm_sbs.o
ifdef CONFIG_ACPI_VIDEO
acpi-y += video_detect.o
endif
+acpi-$(CONFIG_ACPI_GPIO_EVT_EMULATE) += gpio-acpi-evt-emu.o
# These are (potentially) separate modules
diff --git a/drivers/acpi/gpio-acpi-evt-emu.c b/drivers/acpi/gpio-acpi-evt-emu.c
new file mode 100644
index 0000000..c39f501
--- /dev/null
+++ b/drivers/acpi/gpio-acpi-evt-emu.c
@@ -0,0 +1,195 @@
+/*
+ * Code to emulate GPIO-signaled events.
+ *
+ * The sole purpose of this module is to help with GPIO event triggering.
+ * Usage:
+ * 1. See the list of available GPIO devices and associated pins under:
+ * /sys/kernel/debug/acpi/events/<GPIO DEVICE>/<PIN>. Only pins which are to
+ * be used as GPIO-signaled events will be listed (_AEI resources).
+ *
+ * 2. Trigger method corresponding to device pin number:
+ * $ echo 1 > /sys/kernel/debug/acpi/events/<GPIO DEVICE>/<PIN>
+ */
+
+/*
+ * Copyright (C) 2014, Linaro Ltd.
+ * Author: Tomasz Nowicki <tomasz.nowicki@xxxxxxxxxx>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * 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.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
You don't need to specify the FSF address here.
+ */
+
+#include <linux/module.h>
+#include <linux/uaccess.h>
+#include <linux/debugfs.h>
+#include <linux/acpi.h>
+
+#include "acpica/accommon.h"
I need accommon.h to operate on the acpi_namespace_node structure and
get the name of node.
+#include "acpica/acnamesp.h"
acnamesp.h is not necessary once I get rid of acpi_ns_validate_handle().
Are these actually needed?
+
+#include "internal.h"
+
+#define _COMPONENT ACPI_SYSTEM_COMPONENT
+ACPI_MODULE_NAME("gpio_acpi_evt_emu");
+
+struct gpio_pin_parent_data {
+ acpi_handle handle;
+ struct dentry *debugfs_dir;
+ char *name;
+ unsigned int evt_count;
+};
+
+struct gpio_pin_data {
+ struct list_head list;
+ acpi_handle handle;
+ unsigned int pin;
+};
+
+static struct dentry *acpi_evt_debugfs_dir;
+static LIST_HEAD(pin_data_list);
+
+static int gpio_evt_trigger(void *data, u64 val)
+{
+ struct gpio_pin_data *pin_data = (struct gpio_pin_data *)data;
+ int pin = pin_data->pin;
+
+ if (ACPI_FAILURE(acpi_execute_simple_method(pin_data->handle, NULL,
+ pin <= 255 ? 0 : pin)))
+ pr_err(PREFIX "evaluating event method failed\n");
acpi_execute_simple_method() passes one argument to the method. You
can't use it with _Lxx or _Exx which don't expect any arguments.
Otherwise you get this:
[ 122.258191] ACPI: \_SB_.GPO2._E12: Excess arguments - Caller passed 1, method requires 0 (20140724/nsarguments-263)
Right, I will fix it.
Also where does "PREFIX" come from?
It comes from internal.h header.
+
+ return 0;
+}
+
+DEFINE_SIMPLE_ATTRIBUTE(gpio_evt_emu_fops, NULL, gpio_evt_trigger, "%llu\n");
+
+static acpi_status gpio_list_resource(struct acpi_resource *ares, void *context)
+{
+ struct acpi_resource_gpio *agpio = &ares->data.gpio;
+ struct gpio_pin_parent_data *parent_data = context;
+ struct dentry *pin_debugfs_dir;
+ struct gpio_pin_data *pin_data;
+ acpi_handle evt_handle;
+ acpi_status status;
+ char str_pin[5];
+ char ev_name[5];
+ int pin;
+
+ if (ares->type != ACPI_RESOURCE_TYPE_GPIO)
+ return AE_OK;
+
+ if (agpio->connection_type != ACPI_RESOURCE_GPIO_TYPE_INT)
+ return AE_OK;
+
+ pin_data = kmalloc(sizeof(*pin_data), GFP_KERNEL);
+ if (!pin_data)
+ return AE_NO_MEMORY;
+
+ pin = agpio->pin_table[0];
+ snprintf(str_pin, 5, "%d", pin);
+ pin_debugfs_dir = debugfs_create_file(str_pin, S_IWUSR,
+ parent_data->debugfs_dir,
+ pin_data,
+ &gpio_evt_emu_fops);
+ if (!pin_debugfs_dir) {
+ status = AE_NULL_ENTRY;
+ goto fail;
+ }
+
+ if (pin <= 255)
+ sprintf(ev_name, "_%c%02X",
+ agpio->triggering == ACPI_EDGE_SENSITIVE ? 'E' : 'L',
+ pin);
+ else
+ sprintf(ev_name, "_EVT");
+
+ status = acpi_get_handle(parent_data->handle, ev_name, &evt_handle);
+ if (ACPI_FAILURE(status)) {
+ pr_err(PREFIX "getting handle to <%s> of <%s> failed, there is no method related to 0x%02X pin\n",
+ ev_name, parent_data->name, pin);
+ goto fail;
+ }
+
+ pin_data->handle = evt_handle;
+ pin_data->pin = pin;
+ list_add_tail(&pin_data->list, &pin_data_list);
Spaces -> tab
+
+ parent_data->evt_count++;
+
+ return AE_OK;
+fail:
+ kfree(pin_data);
+ return status;
+}
+
+static acpi_status gpio_find_resource(acpi_handle handle, u32 lvl,
+ void *context, void **rv)
+{
+ struct acpi_namespace_node *node;
+ struct dentry *gpio_debugfs_dir;
+ struct gpio_pin_parent_data parent_data;
+ char gpio_name[5];
+
+ node = acpi_ns_validate_handle(handle);
Hmm, why is this needed? Is uppose acpi_get_devices() or already makes
sure you have a valid handle, no?
Correct, acpi_get_devices() validates it for us. I will cast it to
"struct acpi_namespace_node" directly and then get the node name.
+ if (!node) {
+ pr_err(PREFIX "Mapping GPIO handle to node failed\n");
+ return AE_BAD_PARAMETER;
+ }
+
+ snprintf(gpio_name, 5, "%s", node->name.ascii);
+ gpio_debugfs_dir = debugfs_create_dir(gpio_name, acpi_evt_debugfs_dir);
+ if (gpio_debugfs_dir == NULL)
+ return AE_OK;
+
+ parent_data.debugfs_dir = gpio_debugfs_dir;
+ parent_data.handle = handle;
+ parent_data.name = gpio_name;
+ parent_data.evt_count = 0;
+
+ acpi_walk_resources(handle, METHOD_NAME__AEI, gpio_list_resource,
+ &parent_data);
+
+ if (!parent_data.evt_count)
+ debugfs_remove(gpio_debugfs_dir);
+
+ return AE_OK;
+}
+
+static int __init gpio_evt_emu_init(void)
+{
+ if (acpi_debugfs_dir == NULL)
+ return -ENOENT;
+
+ acpi_evt_debugfs_dir = debugfs_create_dir("events", acpi_debugfs_dir);
+ if (acpi_evt_debugfs_dir == NULL)
+ return -ENOENT;
+
+ acpi_get_devices(NULL, gpio_find_resource, NULL, NULL);
+
+ return 0;
+}
+
+static void __exit gpio_evt_emu_deinit(void)
call it gpio_evt_emu_exit() instead.
+{
+ struct gpio_pin_data *pin_data, *temp;
+
+ list_for_each_entry_safe(pin_data, temp, &pin_data_list, list)
+ kfree(pin_data);
I suppose you want to first remove the directory entries and then the
pin data. Otherwise if you get pre-empted at this point and someone
tries to use your debugfs files, bad things might happen.
Good catch!
+
+ debugfs_remove_recursive(acpi_evt_debugfs_dir);
Since this already removes everything below this dentry, why do you need
to store the pointer in gpio_pin_parent_data?
Not sure I got the question.
GPIO device instance (debugfs dir) is parent for all its pins (debugfs
nodes). I am using gpio_pin_parent_data as container to pass info for
all children so I can create debugfs node inside of parent related
debugfs dir.
pin_data_list, however, is used to keep pointers to allocated memory
(one for each pins). debugfs_remove_recursive won't free it.
+}
+
+module_init(gpio_evt_emu_init);
+module_exit(gpio_evt_emu_deinit);
These should follow their respective functions. E.g
static int __init gpio_evt_emu_init(void)
{
...
}
module_init(gpio_evt_emu_init);
+
+MODULE_DESCRIPTION("GPIO-signaled events emulator");
+MODULE_LICENSE("GPL");
GPL v2
Thanks for comments I will incorporate them all.
Regards,
Tomasz Nowicki
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/