Re: [PATCH] ACPI: Add GPIO-signaled event emulator.

[Tomasz Nowicki <tomasz.nowicki@linaro.org>]

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