Hi Mika,
On Sat, Nov 3, 2012 at 7:46 AM, Mika Westerberg
<mika.westerberg@xxxxxxxxxxxxxxx> wrote:
> From: Mathias Nyman <mathias.nyman@xxxxxxxxxxxxxxx>
>
> Add support for translating ACPI GPIO pin numbers to Linux GPIO API pins.
> Needs a gpio controller driver with the acpi handler hook set.
>
> Drivers can use acpi_get_gpio() to translate ACPI5 GpioIO and GpioInt
> resources to Linux GPIO's.
How does the mapping work? Is the ACPI gpio number space kept
completely separate from the Linux GPIO numbers, or is there any
attempt to line up ACPI and Linux GPIO numbering? From my reading, it
/looks/ like the ACPI GPIO numbering is controller-local (no single
large global space) because both a full path and a pin number are
specified, but I'd like to know for sure.
> Signed-off-by: Mathias Nyman <mathias.nyman@xxxxxxxxxxxxxxx>
> Signed-off-by: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> ---
> drivers/gpio/Kconfig | 4 +++
> drivers/gpio/Makefile | 1 +
> drivers/gpio/gpiolib-acpi.c | 60 +++++++++++++++++++++++++++++++++++++++++++
> include/linux/acpi_gpio.h | 19 ++++++++++++++
> 4 files changed, 84 insertions(+)
> create mode 100644 drivers/gpio/gpiolib-acpi.c
> create mode 100644 include/linux/acpi_gpio.h
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index d055cee..2f1905b 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -49,6 +49,10 @@ config OF_GPIO
> def_bool y
> depends on OF && !SPARC
>
> +config ACPI_GPIO
Nit: Can you flip this around to GPIO_ACPI? I know OF_GPIO is the
other way around, but it should be changed.
> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> new file mode 100644
> index 0000000..ef56ea4
> --- /dev/null
> +++ b/drivers/gpio/gpiolib-acpi.c
> @@ -0,0 +1,60 @@
> +/*
> + * ACPI helpers for GPIO API
> + *
> + * Copyright (C) 2012, Intel Corporation
> + * Author: Mathias Nyman <mathias.nyman@xxxxxxxxxxxxxxx>
> + *
> + * 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.
> + */
> +
> +#include <linux/errno.h>
> +#include <linux/gpio.h>
> +#include <linux/module.h>
> +#include <linux/acpi_gpio.h>
> +#include <linux/acpi.h>
> +
> +static int acpi_gpiochip_find(struct gpio_chip *gc, void *data)
> +{
> + acpi_handle handle = data;
> + acpi_handle gc_handle;
> +
> + if (!gc->dev)
> + return false;
> +
> + gc_handle = gc->dev->acpi_handle;
> + if (!gc_handle)
> + return false;
This test is redundant with the next one... unless 'handle' is also NULL :-)
Is it at all possible for multiple gpiochips to be used for a single
ACPI gpio controller node? Such as if the gpio controller has multiple
banks that should be controlled separately? If so then this won't be
sufficient. I've got the same issue with DT support where the find
function needs to also check if the pin is provided by that specific
gpiochip.
Overall the patch looks good, but I need to see how it is used. It
would be really nice if device drivers could use basically the same
interface to obtain Linux gpio numbers regardless of if the backing
data was ACPI or DT. This API is one level below that.
> +
> + return gc_handle == handle;
> +}
> +
> +/**
> + * acpi_get_gpio() - Translate ACPI GPIO pin to GPIO number usable with GPIO API
> + * @path: ACPI GPIO controller full path name, (e.g. "\\_SB.GPO1")
> + * @pin: ACPI GPIO pin number (0-based, controller-relative)
> + *
> + * Returns GPIO number to use with Linux generic GPIO API, or errno error value
> + */
> +
> +int acpi_get_gpio(char *path, int pin)
> +{
> + struct gpio_chip *chip;
> + acpi_handle handle;
> + acpi_status status;
> +
> + status = acpi_get_handle(NULL, path, &handle);
> + if (ACPI_FAILURE(status))
> + return -ENODEV;
> +
> + chip = gpiochip_find(handle, acpi_gpiochip_find);
> + if (!chip)
> + return -ENODEV;
> +
> + if (!gpio_is_valid(chip->base + pin))
> + return -EINVAL;
> +
> + return chip->base + pin;
> +}
> +EXPORT_SYMBOL(acpi_get_gpio);
> diff --git a/include/linux/acpi_gpio.h b/include/linux/acpi_gpio.h
> new file mode 100644
> index 0000000..e025664
> --- /dev/null
> +++ b/include/linux/acpi_gpio.h
> @@ -0,0 +1,19 @@
> +#ifndef _LINUX_ACPI_GPIO_H_
> +#define _LINUX_ACPI_GPIO_H_
> +
> +#include <linux/errno.h>
> +
> +#ifdef CONFIG_ACPI_GPIO
> +
> +int acpi_get_gpio(char *path, int pin);
> +
> +#else /* CONFIG_ACPI_GPIO */
> +
> +static inline int acpi_get_gpio(char *path, int pin)
> +{
> + return -ENODEV;
> +}
> +
> +#endif /* CONFIG_ACPI_GPIO */
> +
> +#endif /* _LINUX_ACPI_GPIO_H_ */
> --
> 1.7.10.4
>
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
