| Message ID | 1408546700-19309-1-git-send-email-tomasz.nowicki@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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@linaro.org> > --- > 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? > + 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. > + > 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@linaro.org> > + * > + * 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" > +#include "acpica/acnamesp.h" 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) Also where does "PREFIX" come from? > + > + 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? > + 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. > + > + 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? > +} > + > +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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@linaro.org> >> --- >> 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@linaro.org> >> + * >> + * 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-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Aug 21, 2014 at 04:39:46PM +0200, Tomasz Nowicki wrote: > >>+{ > >>+ 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. Yes but it removes the debugfs entries below so you don't need to store the children debugfs dentries. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 21.08.2014 16:54, Mika Westerberg wrote: > On Thu, Aug 21, 2014 at 04:39:46PM +0200, Tomasz Nowicki wrote: >>>> +{ >>>> + 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. > > Yes but it removes the debugfs entries below so you don't need to store > the children debugfs dentries. > I don't, gpio_pin_parent_data (local variable) keeps pointer to *parent debugfs dentries* only. gpio_pin_parent_data name might be misleading, I will rename it to gpio_parent_data in next version. Regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday, August 21, 2014 04:39:46 PM Tomasz Nowicki wrote: > Hi Mika, > > On 21.08.2014 12:45, Mika Westerberg wrote: > > On Wed, Aug 20, 2014 at 04:58:20PM +0200, Tomasz Nowicki wrote: [cut] > >> + > >> +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. OK, so here's my concern. If AML does any kind of tracking of state in _Exx/_Lxx, you'll likely totally confuse it by calling those things at random. I'm not sure I'm seeing a compelling reason to put this thing into the tree for this reason. Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 26.08.2014 00:39, Rafael J. Wysocki wrote: > On Thursday, August 21, 2014 04:39:46 PM Tomasz Nowicki wrote: >> Hi Mika, >> >> On 21.08.2014 12:45, Mika Westerberg wrote: >>> On Wed, Aug 20, 2014 at 04:58:20PM +0200, Tomasz Nowicki wrote: > > [cut] > >>>> + >>>> +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. > > OK, so here's my concern. > > If AML does any kind of tracking of state in _Exx/_Lxx, you'll likely totally > confuse it by calling those things at random. > > I'm not sure I'm seeing a compelling reason to put this thing into the tree > for this reason. Yes you are right, but this emulator is only for debugging/development purposes and goes with clear statement "DANGER to use on production kernel", like APEI error injection feature. IMO, this emulator would help with: 1. testing and developing AML methods 2. working on ACPI subsystems that need GPIO-signaled events without GPIO h/w Regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday, August 28, 2014 01:19:48 PM Tomasz Nowicki wrote: > On 26.08.2014 00:39, Rafael J. Wysocki wrote: > > On Thursday, August 21, 2014 04:39:46 PM Tomasz Nowicki wrote: > >> Hi Mika, > >> > >> On 21.08.2014 12:45, Mika Westerberg wrote: > >>> On Wed, Aug 20, 2014 at 04:58:20PM +0200, Tomasz Nowicki wrote: > > > > [cut] > > > >>>> + > >>>> +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. > > > > OK, so here's my concern. > > > > If AML does any kind of tracking of state in _Exx/_Lxx, you'll likely totally > > confuse it by calling those things at random. > > > > I'm not sure I'm seeing a compelling reason to put this thing into the tree > > for this reason. > > Yes you are right, but this emulator is only for debugging/development > purposes and goes with clear statement "DANGER to use on production > kernel", like APEI error injection feature. > > IMO, this emulator would help with: > 1. testing and developing AML methods > 2. working on ACPI subsystems that need GPIO-signaled events without > GPIO h/w OK And why does that belong to the kernel source tree and not to an ACPI/UEFI test suite? Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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" + depends on DEBUG_FS + default n + 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). + 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@linaro.org> + * + * 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 + */ + +#include <linux/module.h> +#include <linux/uaccess.h> +#include <linux/debugfs.h> +#include <linux/acpi.h> + +#include "acpica/accommon.h" +#include "acpica/acnamesp.h" + +#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"); + + 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); + + 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); + 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) +{ + struct gpio_pin_data *pin_data, *temp; + + list_for_each_entry_safe(pin_data, temp, &pin_data_list, list) + kfree(pin_data); + + debugfs_remove_recursive(acpi_evt_debugfs_dir); +} + +module_init(gpio_evt_emu_init); +module_exit(gpio_evt_emu_deinit); + +MODULE_DESCRIPTION("GPIO-signaled events emulator"); +MODULE_LICENSE("GPL");
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@linaro.org> --- 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