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

[Mika Westerberg]

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?

> +        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@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"
> +#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-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/