[PATCH 1/2] gpio / ACPI: register to ACPI events automatically

[PATCH 1/2] gpio / ACPI: register to ACPI events automatically

[Mika Westerberg]

Instead of asking each driver to register to the ACPI events we can just
call acpi_gpiochip_register_interrupts() for each chip that has ACPI
handle. It checks chip->to_irq and if it is set to NULL (a GPIO driver that
doesn't do interrupts) the function does nothing.

Also make the event interface to be private to gpiolib-acpi and remove call
to the API from the one existing user (pinctrl-baytrail.c).

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/gpio/gpiolib-acpi.c        | 18 ++++++++++++++----
 drivers/gpio/gpiolib.c             |  3 +++
 drivers/pinctrl/pinctrl-baytrail.c |  4 ----
 include/linux/acpi_gpio.h          |  8 ++++----
 4 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 5c1ef2b..e12a08e 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -101,7 +101,7 @@ static void acpi_gpio_evt_dh(acpi_handle handle, void *data)
  * gpio pins have acpi event methods and assigns interrupt handlers that calls
  * the acpi event methods for those pins.
  */
-void acpi_gpiochip_request_interrupts(struct gpio_chip *chip)
+static void acpi_gpiochip_request_interrupts(struct gpio_chip *chip)
 {
 	struct acpi_buffer buf = {ACPI_ALLOCATE_BUFFER, NULL};
 	struct acpi_resource *res;
@@ -199,7 +199,6 @@ void acpi_gpiochip_request_interrupts(struct gpio_chip *chip)
 				irq);
 	}
 }
-EXPORT_SYMBOL(acpi_gpiochip_request_interrupts);
 
 struct acpi_gpio_lookup {
 	struct acpi_gpio_info info;
@@ -288,7 +287,7 @@ EXPORT_SYMBOL_GPL(acpi_get_gpio_by_index);
  * The remaining ACPI event interrupts associated with the chip are freed
  * automatically.
  */
-void acpi_gpiochip_free_interrupts(struct gpio_chip *chip)
+static void acpi_gpiochip_free_interrupts(struct gpio_chip *chip)
 {
 	acpi_handle handle;
 	acpi_status status;
@@ -315,4 +314,15 @@ void acpi_gpiochip_free_interrupts(struct gpio_chip *chip)
 	acpi_detach_data(handle, acpi_gpio_evt_dh);
 	kfree(evt_pins);
 }
-EXPORT_SYMBOL(acpi_gpiochip_free_interrupts);
+
+void acpi_gpiochip_add(struct gpio_chip *chip)
+{
+	acpi_gpiochip_request_interrupts(chip);
+}
+EXPORT_SYMBOL_GPL(acpi_gpiochip_add);
+
+void acpi_gpiochip_remove(struct gpio_chip *chip)
+{
+	acpi_gpiochip_free_interrupts(chip);
+}
+EXPORT_SYMBOL_GPL(acpi_gpiochip_remove);
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index ff0fd65..5c83657 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -10,6 +10,7 @@
 #include <linux/seq_file.h>
 #include <linux/gpio.h>
 #include <linux/of_gpio.h>
+#include <linux/acpi_gpio.h>
 #include <linux/idr.h>
 #include <linux/slab.h>
 
@@ -1221,6 +1222,7 @@ int gpiochip_add(struct gpio_chip *chip)
 #endif
 
 	of_gpiochip_add(chip);
+	acpi_gpiochip_add(chip);
 
 	if (status)
 		goto fail;
@@ -1262,6 +1264,7 @@ int gpiochip_remove(struct gpio_chip *chip)
 
 	gpiochip_remove_pin_ranges(chip);
 	of_gpiochip_remove(chip);
+	acpi_gpiochip_remove(chip);
 
 	for (id = 0; id < chip->ngpio; id++) {
 		if (test_bit(FLAG_REQUESTED, &chip->desc[id].flags)) {
diff --git a/drivers/pinctrl/pinctrl-baytrail.c b/drivers/pinctrl/pinctrl-baytrail.c
index e9d735d..a49a070 100644
--- a/drivers/pinctrl/pinctrl-baytrail.c
+++ b/drivers/pinctrl/pinctrl-baytrail.c
@@ -29,7 +29,6 @@
 #include <linux/gpio.h>
 #include <linux/irqdomain.h>
 #include <linux/acpi.h>
-#include <linux/acpi_gpio.h>
 #include <linux/platform_device.h>
 #include <linux/seq_file.h>
 #include <linux/io.h>
@@ -481,9 +480,6 @@ static int byt_gpio_probe(struct platform_device *pdev)
 
 		irq_set_handler_data(hwirq, vg);
 		irq_set_chained_handler(hwirq, byt_gpio_irq_handler);
-
-		/* Register interrupt handlers for gpio signaled acpi events */
-		acpi_gpiochip_request_interrupts(gc);
 	}
 
 	pm_runtime_enable(dev);
diff --git a/include/linux/acpi_gpio.h b/include/linux/acpi_gpio.h
index 4c120a1..e38af63 100644
--- a/include/linux/acpi_gpio.h
+++ b/include/linux/acpi_gpio.h
@@ -18,8 +18,8 @@ struct acpi_gpio_info {
 int acpi_get_gpio(char *path, int pin);
 int acpi_get_gpio_by_index(struct device *dev, int index,
 			   struct acpi_gpio_info *info);
-void acpi_gpiochip_request_interrupts(struct gpio_chip *chip);
-void acpi_gpiochip_free_interrupts(struct gpio_chip *chip);
+void acpi_gpiochip_add(struct gpio_chip *chip);
+void acpi_gpiochip_remove(struct gpio_chip *chip);
 
 #else /* CONFIG_GPIO_ACPI */
 
@@ -34,8 +34,8 @@ static inline int acpi_get_gpio_by_index(struct device *dev, int index,
 	return -ENODEV;
 }
 
-static inline void acpi_gpiochip_request_interrupts(struct gpio_chip *chip) { }
-static inline void acpi_gpiochip_free_interrupts(struct gpio_chip *chip) { }
+static inline void acpi_gpiochip_add(struct gpio_chip *chip) {}
+static inline void acpi_gpiochip_remove(struct gpio_chip *chip) {}
 
 #endif /* CONFIG_GPIO_ACPI */
 
-- 
1.8.4.rc3

[PATCH 2/2] gpio / ACPI: add support for GPIO operation regions

[Mika Westerberg]

GPIO operation regions is a new feature introduced in ACPI 5.0
specification. In practise it means that now ASL code can toggle GPIOs with
the help of the OS GPIO driver.

An imaginary example of such ASL code:

	Device (\SB.GPIO)
	{
		// _REG is called when the operation region handler is
		// installed.
		Method (_REG)
		{
			...
		}

		OperationRegion (GPOP, GeneralPurposeIo, 0, 0xc)
		Field (GPOP, ByteAcc, NoLock, Preserve)
		{
			Connection
			(
	                    GpioIo (Exclusive, PullDefault, 0x0000, 0x0000,
				    IoRestrictionOutputOnly, "\\_SB.GPIO",
				    0x00, ResourceConsumer,,)
				{
				    0x0005
				}
			),
			PWR0, 1
		}

		...
	}

	Device (\SB.DEV0)
	{
		...

		Method (_PS0, 0, Serialized)
		{
			// Toggle the GPIO
			Store (1, \SB.GPIO.PWR0)
		}

		Method (_PS3, 0, Serialized)
		{
			Store (0, \SB.GPIO.PRW0)
		}
	}

The device \SB.DEV0 is powered on by asserting \SB.GPIO.PWR0 GPIO line. So
when the OS calls _PS0 or _PS3, that GPIO line should be set to 1 or 0 by
the actual GPIO driver.

In order to support GPIO operation regions we install ACPI address space
handler for the device (provided that it has an ACPI handle). This handler
uses the standard GPIO APIs to toggle the pin according to what the ASL
code does.

Since there is need to attach more data to the ACPI object, we create a new
structure acpi_gpio_chip_data that contains data for both GPIO operation
regions and for ACPI event handling and make the event handling code to use
this new structure.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/gpio/gpiolib-acpi.c | 131 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 126 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index e12a08e..f52536a 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -24,6 +24,17 @@ struct acpi_gpio_evt_pin {
 	unsigned int irq;
 };
 
+struct acpi_gpio_chip_data {
+	/*
+	 * acpi_install_address_space_handler() wants to put the connection
+	 * information at the start of the context structure we pass it, in
+	 * case we are dealing with GPIO operation regions.
+	 */
+	struct acpi_connection_info ci;
+	struct gpio_chip *chip;
+	struct list_head *evt_pins;
+};
+
 static int acpi_gpiochip_find(struct gpio_chip *gc, void *data)
 {
 	if (!gc->dev)
@@ -86,7 +97,7 @@ static irqreturn_t acpi_gpio_irq_handler_evt(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
-static void acpi_gpio_evt_dh(acpi_handle handle, void *data)
+static void acpi_gpio_chip_dh(acpi_handle handle, void *data)
 {
 	/* The address of this function is used as a key. */
 }
@@ -127,12 +138,16 @@ static void acpi_gpiochip_request_interrupts(struct gpio_chip *chip)
 	if (ACPI_SUCCESS(status)) {
 		evt_pins = kzalloc(sizeof(*evt_pins), GFP_KERNEL);
 		if (evt_pins) {
+			struct acpi_gpio_chip_data *data;
+
 			INIT_LIST_HEAD(evt_pins);
-			status = acpi_attach_data(handle, acpi_gpio_evt_dh,
-						  evt_pins);
+			status = acpi_get_data(handle, acpi_gpio_chip_dh,
+					       (void **)&data);
 			if (ACPI_FAILURE(status)) {
 				kfree(evt_pins);
 				evt_pins = NULL;
+			} else {
+				data->evt_pins = evt_pins;
 			}
 		}
 	}
@@ -293,6 +308,7 @@ static void acpi_gpiochip_free_interrupts(struct gpio_chip *chip)
 	acpi_status status;
 	struct list_head *evt_pins;
 	struct acpi_gpio_evt_pin *evt_pin, *ep;
+	struct acpi_gpio_chip_data *data;
 
 	if (!chip->dev || !chip->to_irq)
 		return;
@@ -301,28 +317,133 @@ static void acpi_gpiochip_free_interrupts(struct gpio_chip *chip)
 	if (!handle)
 		return;
 
-	status = acpi_get_data(handle, acpi_gpio_evt_dh, (void **)&evt_pins);
+	status = acpi_get_data(handle, acpi_gpio_chip_dh, (void **)&data);
 	if (ACPI_FAILURE(status))
 		return;
 
+	evt_pins = data->evt_pins;
+	data->evt_pins = NULL;
+
 	list_for_each_entry_safe_reverse(evt_pin, ep, evt_pins, node) {
 		devm_free_irq(chip->dev, evt_pin->irq, evt_pin);
 		list_del(&evt_pin->node);
 		kfree(evt_pin);
 	}
 
-	acpi_detach_data(handle, acpi_gpio_evt_dh);
 	kfree(evt_pins);
 }
 
+static acpi_status
+acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
+			    u32 bits, u64 *value, void *handler_context,
+			    void *reqion_context)
+{
+	struct acpi_gpio_chip_data *data = handler_context;
+	struct gpio_chip *chip = data->chip;
+	struct acpi_resource *ares;
+	int ret, gpio = -EINVAL;
+	unsigned long flags = 0;
+	acpi_status status;
+
+	status = acpi_buffer_to_resource(data->ci.connection, data->ci.length,
+					 &ares);
+	if (ACPI_FAILURE(status))
+		return status;
+
+	if (ares->type == ACPI_RESOURCE_TYPE_GPIO) {
+		const struct acpi_resource_gpio *agpio = &ares->data.gpio;
+
+		switch (agpio->io_restriction) {
+		case ACPI_IO_RESTRICT_NONE:
+			flags = GPIOF_DIR_IN | GPIOF_DIR_OUT;
+			break;
+		case ACPI_IO_RESTRICT_INPUT:
+			flags = GPIOF_DIR_IN;
+			break;
+		case ACPI_IO_RESTRICT_OUTPUT:
+			flags = GPIOF_DIR_OUT;
+			break;
+		}
+
+		gpio = acpi_get_gpio(agpio->resource_source.string_ptr,
+				     agpio->pin_table[0]);
+	}
+
+	ACPI_FREE(ares);
+
+	if (!gpio_is_valid(gpio)) {
+		dev_err(chip->dev, "GpioOpReg: invalid GPIO resource\n");
+		return AE_ERROR;
+	}
+
+	ret = gpio_request_one(gpio, flags, "acpi_gpio_adr_space");
+	if (ret) {
+		dev_err(chip->dev, "GpioOpReg: failed to request GPIO\n");
+		return AE_ERROR;
+	}
+
+	if (function == ACPI_WRITE)
+		gpio_set_value(gpio, (int)*value);
+	else
+		*value = gpio_get_value(gpio);
+
+	gpio_free(gpio);
+	return AE_OK;
+}
+
 void acpi_gpiochip_add(struct gpio_chip *chip)
 {
+	struct acpi_gpio_chip_data *data;
+	acpi_handle handle;
+	acpi_status status;
+
+	handle = ACPI_HANDLE(chip->dev);
+	if (!handle)
+		return;
+
+	data = kzalloc(sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return;
+
+	status = acpi_attach_data(handle, acpi_gpio_chip_dh, data);
+	if (ACPI_FAILURE(status)) {
+		kfree(data);
+		return;
+	}
+
+	data->chip = chip;
+
+	status = acpi_install_address_space_handler(handle, ACPI_ADR_SPACE_GPIO,
+						    acpi_gpio_adr_space_handler,
+						    NULL, data);
+	if (ACPI_FAILURE(status)) {
+		acpi_detach_data(handle, acpi_gpio_chip_dh);
+		kfree(data);
+		return;
+	}
+
 	acpi_gpiochip_request_interrupts(chip);
 }
 EXPORT_SYMBOL_GPL(acpi_gpiochip_add);
 
 void acpi_gpiochip_remove(struct gpio_chip *chip)
 {
+	struct acpi_gpio_chip_data *data;
+	acpi_handle handle;
+	acpi_status status;
+
+	handle = ACPI_HANDLE(chip->dev);
+	if (!handle)
+		return;
+
+	status = acpi_get_data(handle, acpi_gpio_chip_dh, (void **)&data);
+	if (ACPI_FAILURE(status))
+		return;
+
 	acpi_gpiochip_free_interrupts(chip);
+	acpi_remove_address_space_handler(handle, ACPI_ADR_SPACE_GPIO,
+					  acpi_gpio_adr_space_handler);
+	acpi_detach_data(handle, acpi_gpio_chip_dh);
+	kfree(data);
 }
 EXPORT_SYMBOL_GPL(acpi_gpiochip_remove);
-- 
1.8.4.rc3

Re: [PATCH 2/2] gpio / ACPI: add support for GPIO operation regions

[Andy Shevchenko]

On Fri, Sep 13, 2013 at 6:14 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> GPIO operation regions is a new feature introduced in ACPI 5.0
> specification. In practise it means that now ASL code can toggle GPIOs with
> the help of the OS GPIO driver.

[]

>  void acpi_gpiochip_add(struct gpio_chip *chip)
>  {
> +       struct acpi_gpio_chip_data *data;
> +       acpi_handle handle;
> +       acpi_status status;
> +
> +       handle = ACPI_HANDLE(chip->dev);
> +       if (!handle)
> +               return;
> +
> +       data = kzalloc(sizeof(*data), GFP_KERNEL);

May we use devm_kzalloc here?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] gpio / ACPI: add support for GPIO operation regions

[Mika Westerberg]

On Fri, Sep 13, 2013 at 06:55:11PM +0300, Andy Shevchenko wrote:
> On Fri, Sep 13, 2013 at 6:14 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > GPIO operation regions is a new feature introduced in ACPI 5.0
> > specification. In practise it means that now ASL code can toggle GPIOs with
> > the help of the OS GPIO driver.
> 
> []
> 
> >  void acpi_gpiochip_add(struct gpio_chip *chip)
> >  {
> > +       struct acpi_gpio_chip_data *data;
> > +       acpi_handle handle;
> > +       acpi_status status;
> > +
> > +       handle = ACPI_HANDLE(chip->dev);
> > +       if (!handle)
> > +               return;
> > +
> > +       data = kzalloc(sizeof(*data), GFP_KERNEL);
> 
> May we use devm_kzalloc here?

The ACPI event handling code still uses kzalloc() and we need to call
acpi_gpiolib_remove() anyway (which undoes this), so I think we should
stick with kzalloc() now.

RE: [PATCH 2/2] gpio / ACPI: add support for GPIO operation regions

[Zheng, Lv]

> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Mika Westerberg
> Sent: Friday, September 13, 2013 11:15 PM
> 
> GPIO operation regions is a new feature introduced in ACPI 5.0
> specification. In practise it means that now ASL code can toggle GPIOs with
> the help of the OS GPIO driver.
> 
> An imaginary example of such ASL code:
> 
> 	Device (\SB.GPIO)
> 	{
> 		// _REG is called when the operation region handler is
> 		// installed.
> 		Method (_REG)
> 		{
> 			...
> 		}
> 
> 		OperationRegion (GPOP, GeneralPurposeIo, 0, 0xc)
> 		Field (GPOP, ByteAcc, NoLock, Preserve)
> 		{
> 			Connection
> 			(
> 	                    GpioIo (Exclusive, PullDefault, 0x0000, 0x0000,
> 				    IoRestrictionOutputOnly, "\\_SB.GPIO",
> 				    0x00, ResourceConsumer,,)
> 				{
> 				    0x0005
> 				}
> 			),
> 			PWR0, 1
> 		}
> 
> 		...
> 	}
> 
> 	Device (\SB.DEV0)
> 	{
> 		...
> 
> 		Method (_PS0, 0, Serialized)
> 		{
> 			// Toggle the GPIO
> 			Store (1, \SB.GPIO.PWR0)
> 		}
> 
> 		Method (_PS3, 0, Serialized)
> 		{
> 			Store (0, \SB.GPIO.PRW0)
> 		}
> 	}
> 
> The device \SB.DEV0 is powered on by asserting \SB.GPIO.PWR0 GPIO line. So
> when the OS calls _PS0 or _PS3, that GPIO line should be set to 1 or 0 by
> the actual GPIO driver.
> 
> In order to support GPIO operation regions we install ACPI address space
> handler for the device (provided that it has an ACPI handle). This handler
> uses the standard GPIO APIs to toggle the pin according to what the ASL
> code does.
> 
> Since there is need to attach more data to the ACPI object, we create a new
> structure acpi_gpio_chip_data that contains data for both GPIO operation
> regions and for ACPI event handling and make the event handling code to use
> this new structure.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/gpio/gpiolib-acpi.c | 131 ++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 126 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> index e12a08e..f52536a 100644
> --- a/drivers/gpio/gpiolib-acpi.c
> +++ b/drivers/gpio/gpiolib-acpi.c
> @@ -24,6 +24,17 @@ struct acpi_gpio_evt_pin {
>  	unsigned int irq;
>  };
> 
> +struct acpi_gpio_chip_data {
> +	/*
> +	 * acpi_install_address_space_handler() wants to put the connection
> +	 * information at the start of the context structure we pass it, in
> +	 * case we are dealing with GPIO operation regions.
> +	 */
> +	struct acpi_connection_info ci;
> +	struct gpio_chip *chip;
> +	struct list_head *evt_pins;
> +};
> +
>  static int acpi_gpiochip_find(struct gpio_chip *gc, void *data)
>  {
>  	if (!gc->dev)
> @@ -86,7 +97,7 @@ static irqreturn_t acpi_gpio_irq_handler_evt(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
> 
> -static void acpi_gpio_evt_dh(acpi_handle handle, void *data)
> +static void acpi_gpio_chip_dh(acpi_handle handle, void *data)
>  {
>  	/* The address of this function is used as a key. */
>  }
> @@ -127,12 +138,16 @@ static void acpi_gpiochip_request_interrupts(struct gpio_chip *chip)
>  	if (ACPI_SUCCESS(status)) {
>  		evt_pins = kzalloc(sizeof(*evt_pins), GFP_KERNEL);
>  		if (evt_pins) {
> +			struct acpi_gpio_chip_data *data;
> +
>  			INIT_LIST_HEAD(evt_pins);
> -			status = acpi_attach_data(handle, acpi_gpio_evt_dh,
> -						  evt_pins);
> +			status = acpi_get_data(handle, acpi_gpio_chip_dh,
> +					       (void **)&data);
>  			if (ACPI_FAILURE(status)) {
>  				kfree(evt_pins);
>  				evt_pins = NULL;
> +			} else {
> +				data->evt_pins = evt_pins;
>  			}
>  		}
>  	}
> @@ -293,6 +308,7 @@ static void acpi_gpiochip_free_interrupts(struct gpio_chip *chip)
>  	acpi_status status;
>  	struct list_head *evt_pins;
>  	struct acpi_gpio_evt_pin *evt_pin, *ep;
> +	struct acpi_gpio_chip_data *data;
> 
>  	if (!chip->dev || !chip->to_irq)
>  		return;
> @@ -301,28 +317,133 @@ static void acpi_gpiochip_free_interrupts(struct gpio_chip *chip)
>  	if (!handle)
>  		return;
> 
> -	status = acpi_get_data(handle, acpi_gpio_evt_dh, (void **)&evt_pins);
> +	status = acpi_get_data(handle, acpi_gpio_chip_dh, (void **)&data);
>  	if (ACPI_FAILURE(status))
>  		return;
> 
> +	evt_pins = data->evt_pins;
> +	data->evt_pins = NULL;
> +
>  	list_for_each_entry_safe_reverse(evt_pin, ep, evt_pins, node) {
>  		devm_free_irq(chip->dev, evt_pin->irq, evt_pin);
>  		list_del(&evt_pin->node);
>  		kfree(evt_pin);
>  	}
> 
> -	acpi_detach_data(handle, acpi_gpio_evt_dh);
>  	kfree(evt_pins);
>  }
> 
> +static acpi_status
> +acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
> +			    u32 bits, u64 *value, void *handler_context,
> +			    void *reqion_context)
> +{
> +	struct acpi_gpio_chip_data *data = handler_context;
> +	struct gpio_chip *chip = data->chip;
> +	struct acpi_resource *ares;
> +	int ret, gpio = -EINVAL;
> +	unsigned long flags = 0;
> +	acpi_status status;
> +
> +	status = acpi_buffer_to_resource(data->ci.connection, data->ci.length,
> +					 &ares);
> +	if (ACPI_FAILURE(status))
> +		return status;
> +
> +	if (ares->type == ACPI_RESOURCE_TYPE_GPIO) {
> +		const struct acpi_resource_gpio *agpio = &ares->data.gpio;
> +
> +		switch (agpio->io_restriction) {
> +		case ACPI_IO_RESTRICT_NONE:
> +			flags = GPIOF_DIR_IN | GPIOF_DIR_OUT;
> +			break;
> +		case ACPI_IO_RESTRICT_INPUT:
> +			flags = GPIOF_DIR_IN;
> +			break;
> +		case ACPI_IO_RESTRICT_OUTPUT:
> +			flags = GPIOF_DIR_OUT;
> +			break;
> +		}
> +
> +		gpio = acpi_get_gpio(agpio->resource_source.string_ptr,
> +				     agpio->pin_table[0]);
> +	}
> +
> +	ACPI_FREE(ares);
> +
> +	if (!gpio_is_valid(gpio)) {
> +		dev_err(chip->dev, "GpioOpReg: invalid GPIO resource\n");
> +		return AE_ERROR;
> +	}
> +
> +	ret = gpio_request_one(gpio, flags, "acpi_gpio_adr_space");
> +	if (ret) {
> +		dev_err(chip->dev, "GpioOpReg: failed to request GPIO\n");
> +		return AE_ERROR;
> +	}
> +
> +	if (function == ACPI_WRITE)
> +		gpio_set_value(gpio, (int)*value);
> +	else
> +		*value = gpio_get_value(gpio);
> +
> +	gpio_free(gpio);
> +	return AE_OK;
> +}
> +
>  void acpi_gpiochip_add(struct gpio_chip *chip)
>  {
> +	struct acpi_gpio_chip_data *data;
> +	acpi_handle handle;
> +	acpi_status status;
> +
> +	handle = ACPI_HANDLE(chip->dev);
> +	if (!handle)
> +		return;
> +
> +	data = kzalloc(sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return;
> +
> +	status = acpi_attach_data(handle, acpi_gpio_chip_dh, data);
> +	if (ACPI_FAILURE(status)) {
> +		kfree(data);
> +		return;
> +	}
> +
> +	data->chip = chip;
> +
> +	status = acpi_install_address_space_handler(handle, ACPI_ADR_SPACE_GPIO,
> +						    acpi_gpio_adr_space_handler,
> +						    NULL, data);

Is it possible to install the handler for ACPI_ROOT_OBJECT?
Can it be achieved by implementing a setup callback?
Maybe you can also eliminate acpi_attach_data usages by doing so.

> +	if (ACPI_FAILURE(status)) {
> +		acpi_detach_data(handle, acpi_gpio_chip_dh);
> +		kfree(data);
> +		return;
> +	}
> +
>  	acpi_gpiochip_request_interrupts(chip);
>  }
>  EXPORT_SYMBOL_GPL(acpi_gpiochip_add);
> 
>  void acpi_gpiochip_remove(struct gpio_chip *chip)
>  {
> +	struct acpi_gpio_chip_data *data;
> +	acpi_handle handle;
> +	acpi_status status;
> +
> +	handle = ACPI_HANDLE(chip->dev);
> +	if (!handle)
> +		return;
> +
> +	status = acpi_get_data(handle, acpi_gpio_chip_dh, (void **)&data);
> +	if (ACPI_FAILURE(status))
> +		return;
> +
>  	acpi_gpiochip_free_interrupts(chip);
> +	acpi_remove_address_space_handler(handle, ACPI_ADR_SPACE_GPIO,
> +					  acpi_gpio_adr_space_handler);
> +	acpi_detach_data(handle, acpi_gpio_chip_dh);
> +	kfree(data);
>  }
>  EXPORT_SYMBOL_GPL(acpi_gpiochip_remove);
> --
> 1.8.4.rc3
> 
> --
> 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

Re: [PATCH 2/2] gpio / ACPI: add support for GPIO operation regions

[Mika Westerberg]

On Sat, Sep 14, 2013 at 12:10:37AM +0000, Zheng, Lv wrote:
> Is it possible to install the handler for ACPI_ROOT_OBJECT?
> Can it be achieved by implementing a setup callback?

Yes that can be done. However, that would mean that we always install the
operation region handler even if there is no suitable GPIO driver loaded.
With this patch we install the handler once the GPIO driver for this device
is registered. If nothing is registered no handlers will be installed.

What would be the advantage in doing what you propose?

> Maybe you can also eliminate acpi_attach_data usages by doing so.

I think we still need that for ACPI _EVT handling.

RE: [PATCH 2/2] gpio / ACPI: add support for GPIO operation regions

[Zheng, Lv]

> From: Mika Westerberg [mailto:mika.westerberg@linux.intel.com]
> Sent: Sunday, September 15, 2013 2:52 PM
> 
> On Sat, Sep 14, 2013 at 12:10:37AM +0000, Zheng, Lv wrote:
> > Is it possible to install the handler for ACPI_ROOT_OBJECT?
> > Can it be achieved by implementing a setup callback?
> 
> Yes that can be done. However, that would mean that we always install the
> operation region handler even if there is no suitable GPIO driver loaded.
> With this patch we install the handler once the GPIO driver for this device
> is registered. If nothing is registered no handlers will be installed.
> 
> What would be the advantage in doing what you propose?

A pseudo device may be created to access the GPIO operation region fields provided by one GPIO device.
The pseudo device may have other functions to access other GPIO operation region fields provided by other GPIO devices, or even worse to access other ACPI provided value-adds.
So hierarchically the pseudo device only requires CPU, thus should not be under the GPIO device, which means the GPIO operation regions have nothing to do with the GPIO devices' ACPI handle.
We cannot imply that the BIOS writers won't create such Frankenstein in the future.
So it's better to install address space handlers from ACPI_ROOT_OBJECT.

> > Maybe you can also eliminate acpi_attach_data usages by doing so.
> 
> I think we still need that for ACPI _EVT handling.

It could be good if we can find a way to eliminate all acpi_attach_data usages and make this function deprecated.
But that's fine. :-)

Thanks and best regards
-Lv

RE: [PATCH 2/2] gpio / ACPI: add support for GPIO operation regions

[Zheng, Lv]

> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Zheng, Lv
> Sent: Monday, September 16, 2013 8:47 AM
> 
> > From: Mika Westerberg [mailto:mika.westerberg@linux.intel.com]
> > Sent: Sunday, September 15, 2013 2:52 PM
> >
> > On Sat, Sep 14, 2013 at 12:10:37AM +0000, Zheng, Lv wrote:
> > > Is it possible to install the handler for ACPI_ROOT_OBJECT?
> > > Can it be achieved by implementing a setup callback?
> >
> > Yes that can be done. However, that would mean that we always install the
> > operation region handler even if there is no suitable GPIO driver loaded.
> > With this patch we install the handler once the GPIO driver for this device
> > is registered. If nothing is registered no handlers will be installed.
> >
> > What would be the advantage in doing what you propose?
> 
> A pseudo device may be created to access the GPIO operation region fields provided by one GPIO device.
> The pseudo device may have other functions to access other GPIO operation region fields provided by other GPIO devices, or even worse to
> access other ACPI provided value-adds.
> So hierarchically the pseudo device only requires CPU, thus should not be under the GPIO device, which means the GPIO operation regions
> have nothing to do with the GPIO devices' ACPI handle.

Sorry for the wording.
It's better to say the GPIO operation region users haven't strict relationship to the GPIO operation region providers.
As the installation is to provide GPIO operation regions to the users, it shouldn't relate to the providers' ACPI handle.

> We cannot imply that the BIOS writers won't create such Frankenstein in the future.
> So it's better to install address space handlers from ACPI_ROOT_OBJECT.

If we didn't do this and such a pseudo device was created, then the error message: "Region XXX has no handler" would be prompted.

Thanks
-Lv

> 
> > > Maybe you can also eliminate acpi_attach_data usages by doing so.
> >
> > I think we still need that for ACPI _EVT handling.
> 
> It could be good if we can find a way to eliminate all acpi_attach_data usages and make this function deprecated.
> But that's fine. :-)
> 
> Thanks and best regards
> -Lv
> --
> 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

Re: [PATCH 2/2] gpio / ACPI: add support for GPIO operation regions

[Mika Westerberg]

On Mon, Sep 16, 2013 at 01:21:53AM +0000, Zheng, Lv wrote:
> > A pseudo device may be created to access the GPIO operation region fields provided by one GPIO device.
> > The pseudo device may have other functions to access other GPIO operation region fields provided by other GPIO devices, or even worse to
> > access other ACPI provided value-adds.
> > So hierarchically the pseudo device only requires CPU, thus should not be under the GPIO device, which means the GPIO operation regions
> > have nothing to do with the GPIO devices' ACPI handle.
> 
> Sorry for the wording.
> It's better to say the GPIO operation region users haven't strict
> relationship to the GPIO operation region providers.
> As the installation is to provide GPIO operation regions to the users, it
> shouldn't relate to the providers' ACPI handle.

If I understand you correctly you mean that there might be multiple users
(different devices) for the same GPIO operation region, right?

That shouldn't be a problem as far as I can tell.

What comes to the hierarchy you refer, I'm not sure if that is a problem
either (unless I'm missing something). The GPIO can be used anywhere in the
ASL, it doesn't have to be descendant of the GPIO device. You only need to
do something like this:

	// Assert the GPIO
	Store(1, \_SB.PCI0.SHD3)

In other words, use the fully qualified name.

Typically when the GPIO device _REG() is called it sets some variable like
AVBL to true which is then checked in the code that uses the GPIO:

	If (LEqual (\_SB.PCI0.GPO0.AVBL, One))
	{
		Store (Zero, \_SB.PCI0..SHD3)
	}

So if there is no driver, this part of the code is never called.

RE: [PATCH 2/2] gpio / ACPI: add support for GPIO operation regions

[Zheng, Lv]

> From: Mika Westerberg [mailto:mika.westerberg@linux.intel.com]
> Sent: Monday, September 16, 2013 4:11 PM
> 
> On Mon, Sep 16, 2013 at 01:21:53AM +0000, Zheng, Lv wrote:
> > > A pseudo device may be created to access the GPIO operation region fields provided by one GPIO device.
> > > The pseudo device may have other functions to access other GPIO operation region fields provided by other GPIO devices, or even worse
> to
> > > access other ACPI provided value-adds.
> > > So hierarchically the pseudo device only requires CPU, thus should not be under the GPIO device, which means the GPIO operation
> regions
> > > have nothing to do with the GPIO devices' ACPI handle.
> >
> > Sorry for the wording.
> > It's better to say the GPIO operation region users haven't strict
> > relationship to the GPIO operation region providers.
> > As the installation is to provide GPIO operation regions to the users, it
> > shouldn't relate to the providers' ACPI handle.
> 
> If I understand you correctly you mean that there might be multiple users
> (different devices) for the same GPIO operation region, right?

No, this is not what I meant.
Can one vendor device uses two or more GPIO pins from different GPIO ports?

> That shouldn't be a problem as far as I can tell.
> 
> What comes to the hierarchy you refer, I'm not sure if that is a problem
> either (unless I'm missing something). The GPIO can be used anywhere in the
> ASL, it doesn't have to be descendant of the GPIO device. You only need to
> do something like this:
> 
> 	// Assert the GPIO
> 	Store(1, \_SB.PCI0.SHD3)
> 
> In other words, use the fully qualified name.

Yes, which means this line of code can be everywhere in the namespace.
It is not required to be under one particular GPIO device as long as there is an operation region defined in that scope.

The problem is the installation, the first parameter for acpi_install_address_space_handler() is a namespace node, the function will call _REG for all OperationRegions below the scope whose top most node is the specified node.
The nodes out of this scope are not affected.  So if an OperationRegion is defined out of this scope, problem happens.

> Typically when the GPIO device _REG() is called it sets some variable like
> AVBL to true which is then checked in the code that uses the GPIO:
> 
> 	If (LEqual (\_SB.PCI0.GPO0.AVBL, One))
> 	{
> 		Store (Zero, \_SB.PCI0..SHD3)
> 	}
> 
> So if there is no driver, this part of the code is never called.

This can trigger exceptions, which can be used to load the GPIO driver.
This seems to be another topic.

Thanks and best regards
-Lv

Re: [PATCH 2/2] gpio / ACPI: add support for GPIO operation regions

[Mika Westerberg]

On Mon, Sep 16, 2013 at 11:35:56PM +0000, Zheng, Lv wrote:
> > From: Mika Westerberg [mailto:mika.westerberg@linux.intel.com]
> > Sent: Monday, September 16, 2013 4:11 PM
> > 
> > On Mon, Sep 16, 2013 at 01:21:53AM +0000, Zheng, Lv wrote:
> > > > A pseudo device may be created to access the GPIO operation region fields provided by one GPIO device.
> > > > The pseudo device may have other functions to access other GPIO operation region fields provided by other GPIO devices, or even worse
> > to
> > > > access other ACPI provided value-adds.
> > > > So hierarchically the pseudo device only requires CPU, thus should not be under the GPIO device, which means the GPIO operation
> > regions
> > > > have nothing to do with the GPIO devices' ACPI handle.
> > >
> > > Sorry for the wording.
> > > It's better to say the GPIO operation region users haven't strict
> > > relationship to the GPIO operation region providers.
> > > As the installation is to provide GPIO operation regions to the users, it
> > > shouldn't relate to the providers' ACPI handle.
> > 
> > If I understand you correctly you mean that there might be multiple users
> > (different devices) for the same GPIO operation region, right?
> 
> No, this is not what I meant.
> Can one vendor device uses two or more GPIO pins from different GPIO ports?

Yes.

> > That shouldn't be a problem as far as I can tell.
> > 
> > What comes to the hierarchy you refer, I'm not sure if that is a problem
> > either (unless I'm missing something). The GPIO can be used anywhere in the
> > ASL, it doesn't have to be descendant of the GPIO device. You only need to
> > do something like this:
> > 
> > 	// Assert the GPIO
> > 	Store(1, \_SB.PCI0.SHD3)
> > 
> > In other words, use the fully qualified name.
> 
> Yes, which means this line of code can be everywhere in the namespace.
> It is not required to be under one particular GPIO device as long as there is an operation region defined in that scope.
> 
> The problem is the installation, the first parameter for
> acpi_install_address_space_handler() is a namespace node, the function
> will call _REG for all OperationRegions below the scope whose top most
> node is the specified node.
> The nodes out of this scope are not affected.  So if an OperationRegion
> is defined out of this scope, problem happens.

I suppose for that operation region another GPIO device should be
introduced then, no?

So if we don't have a GPIO driver for the given operation region, what can
we do? We don't want the ASL code to erroneusly think that there is an
operation region available when it is not.

Re: [PATCH 1/2] gpio / ACPI: register to ACPI events automatically

[Linus Walleij]

On Fri, Sep 13, 2013 at 5:14 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:

> Instead of asking each driver to register to the ACPI events we can just
> call acpi_gpiochip_register_interrupts() for each chip that has ACPI
> handle. It checks chip->to_irq and if it is set to NULL (a GPIO driver that
> doesn't do interrupts) the function does nothing.
>
> Also make the event interface to be private to gpiolib-acpi and remove call
> to the API from the one existing user (pinctrl-baytrail.c).
>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>

The concept looks sane...

> +void acpi_gpiochip_add(struct gpio_chip *chip)
> +{
> +       acpi_gpiochip_request_interrupts(chip);
> +}
> +EXPORT_SYMBOL_GPL(acpi_gpiochip_add);
> +
> +void acpi_gpiochip_remove(struct gpio_chip *chip)
> +{
> +       acpi_gpiochip_free_interrupts(chip);
> +}
> +EXPORT_SYMBOL_GPL(acpi_gpiochip_remove);

If you're only going to call this from within gpiolib, why are
you EXPORTing the APIs?

I think we should maybe create drivers/gpio/gpiolib.h
for such subsystem-local headers.

> @@ -1221,6 +1222,7 @@ int gpiochip_add(struct gpio_chip *chip)
>  #endif
>
>         of_gpiochip_add(chip);
> +       acpi_gpiochip_add(chip);
>
>         if (status)
>                 goto fail;
> @@ -1262,6 +1264,7 @@ int gpiochip_remove(struct gpio_chip *chip)
>
>         gpiochip_remove_pin_ranges(chip);
>         of_gpiochip_remove(chip);
> +       acpi_gpiochip_remove(chip);

What happens on a platform that is not using CONFIG_GPIO_ACPI
when they try to compile this?

You forgot to add static inline stubs for the non-ACPI case.

Yours,
Linus Walleij

Re: [PATCH 2/2] gpio / ACPI: add support for GPIO operation regions

[Linus Walleij]

On Fri, Sep 13, 2013 at 5:14 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:

> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
(...)
> +struct acpi_gpio_chip_data {
> +       /*
> +        * acpi_install_address_space_handler() wants to put the connection
> +        * information at the start of the context structure we pass it, in
> +        * case we are dealing with GPIO operation regions.
> +        */
> +       struct acpi_connection_info ci;
> +       struct gpio_chip *chip;
> +       struct list_head *evt_pins;
> +};

Consider just naming this acpi_gpio_chip, as it is obviously some
generic container that you will keep adding to.

I'm uncertain how things work, it wouldn't add something to have
struct gpio_chip be a true member (not a pointer) so you can
allocate one thing from the drivers, and e.g. use container_of()
to get from the gpio_chip to the acpi_gpio_chip[_data]?

Yours,
Linus Walleij

Re: [PATCH 2/2] gpio / ACPI: add support for GPIO operation regions

[Mika Westerberg]

On Fri, Sep 20, 2013 at 09:21:37PM +0200, Linus Walleij wrote:
> On Fri, Sep 13, 2013 at 5:14 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> 
> > diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> (...)
> > +struct acpi_gpio_chip_data {
> > +       /*
> > +        * acpi_install_address_space_handler() wants to put the connection
> > +        * information at the start of the context structure we pass it, in
> > +        * case we are dealing with GPIO operation regions.
> > +        */
> > +       struct acpi_connection_info ci;
> > +       struct gpio_chip *chip;
> > +       struct list_head *evt_pins;
> > +};
> 
> Consider just naming this acpi_gpio_chip, as it is obviously some
> generic container that you will keep adding to.

Sure.

> I'm uncertain how things work, it wouldn't add something to have
> struct gpio_chip be a true member (not a pointer) so you can
> allocate one thing from the drivers, and e.g. use container_of()
> to get from the gpio_chip to the acpi_gpio_chip[_data]?

The drivers are just normal platform drivers (for example gpio-lynxpoint.c)
and they shouldn't care if they got enumerated from ACPI. Allocating
acpi_gpio_chip from a driver would make it depend on ACPI, if I'm
understanding correctly.

Re: [PATCH 1/2] gpio / ACPI: register to ACPI events automatically

[Mika Westerberg]

On Fri, Sep 20, 2013 at 09:08:57PM +0200, Linus Walleij wrote:
> On Fri, Sep 13, 2013 at 5:14 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> 
> > Instead of asking each driver to register to the ACPI events we can just
> > call acpi_gpiochip_register_interrupts() for each chip that has ACPI
> > handle. It checks chip->to_irq and if it is set to NULL (a GPIO driver that
> > doesn't do interrupts) the function does nothing.
> >
> > Also make the event interface to be private to gpiolib-acpi and remove call
> > to the API from the one existing user (pinctrl-baytrail.c).
> >
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> 
> The concept looks sane...
> 
> > +void acpi_gpiochip_add(struct gpio_chip *chip)
> > +{
> > +       acpi_gpiochip_request_interrupts(chip);
> > +}
> > +EXPORT_SYMBOL_GPL(acpi_gpiochip_add);
> > +
> > +void acpi_gpiochip_remove(struct gpio_chip *chip)
> > +{
> > +       acpi_gpiochip_free_interrupts(chip);
> > +}
> > +EXPORT_SYMBOL_GPL(acpi_gpiochip_remove);
> 
> If you're only going to call this from within gpiolib, why are
> you EXPORTing the APIs?
> 
> I think we should maybe create drivers/gpio/gpiolib.h
> for such subsystem-local headers.

Good point. Will do that in the next version.

> > @@ -1221,6 +1222,7 @@ int gpiochip_add(struct gpio_chip *chip)
> >  #endif
> >
> >         of_gpiochip_add(chip);
> > +       acpi_gpiochip_add(chip);
> >
> >         if (status)
> >                 goto fail;
> > @@ -1262,6 +1264,7 @@ int gpiochip_remove(struct gpio_chip *chip)
> >
> >         gpiochip_remove_pin_ranges(chip);
> >         of_gpiochip_remove(chip);
> > +       acpi_gpiochip_remove(chip);
> 
> What happens on a platform that is not using CONFIG_GPIO_ACPI
> when they try to compile this?
> 
> You forgot to add static inline stubs for the non-ACPI case.

This patch adds them to <linux/acpi_gpio.h> which is included from
gpiolib.c.

RE: [PATCH 2/2] gpio / ACPI: add support for GPIO operation regions

[Zheng, Lv]

> From: Mika Westerberg [mailto:mika.westerberg@linux.intel.com]
> Sent: Tuesday, September 17, 2013 4:37 PM
> 
> On Mon, Sep 16, 2013 at 11:35:56PM +0000, Zheng, Lv wrote:
> > > From: Mika Westerberg [mailto:mika.westerberg@linux.intel.com]
> > > Sent: Monday, September 16, 2013 4:11 PM
> > >
> > > On Mon, Sep 16, 2013 at 01:21:53AM +0000, Zheng, Lv wrote:
> > > > > A pseudo device may be created to access the GPIO operation region fields provided by one GPIO device.
> > > > > The pseudo device may have other functions to access other GPIO operation region fields provided by other GPIO devices, or even
> worse
> > > to
> > > > > access other ACPI provided value-adds.
> > > > > So hierarchically the pseudo device only requires CPU, thus should not be under the GPIO device, which means the GPIO operation
> > > regions
> > > > > have nothing to do with the GPIO devices' ACPI handle.
> > > >
> > > > Sorry for the wording.
> > > > It's better to say the GPIO operation region users haven't strict
> > > > relationship to the GPIO operation region providers.
> > > > As the installation is to provide GPIO operation regions to the users, it
> > > > shouldn't relate to the providers' ACPI handle.
> > >
> > > If I understand you correctly you mean that there might be multiple users
> > > (different devices) for the same GPIO operation region, right?
> >
> > No, this is not what I meant.
> > Can one vendor device uses two or more GPIO pins from different GPIO ports?
> 
> Yes.

So how can such a device under one of these GPIO ports?
It can only be under one particular GPIO port device, right?

Thus I believe such device will appear below other devices rather than below a GPIO port device in the ACPI namespace.


> 
> > > That shouldn't be a problem as far as I can tell.
> > >
> > > What comes to the hierarchy you refer, I'm not sure if that is a problem
> > > either (unless I'm missing something). The GPIO can be used anywhere in the
> > > ASL, it doesn't have to be descendant of the GPIO device. You only need to
> > > do something like this:
> > >
> > > 	// Assert the GPIO
> > > 	Store(1, \_SB.PCI0.SHD3)
> > >
> > > In other words, use the fully qualified name.
> >
> > Yes, which means this line of code can be everywhere in the namespace.
> > It is not required to be under one particular GPIO device as long as there is an operation region defined in that scope.
> >
> > The problem is the installation, the first parameter for
> > acpi_install_address_space_handler() is a namespace node, the function
> > will call _REG for all OperationRegions below the scope whose top most
> > node is the specified node.
> > The nodes out of this scope are not affected.  So if an OperationRegion
> > is defined out of this scope, problem happens.
> 
> I suppose for that operation region another GPIO device should be
> introduced then, no?

I believe the answer is no.

> So if we don't have a GPIO driver for the given operation region, what can
> we do? We don't want the ASL code to erroneusly think that there is an
> operation region available when it is not.

I think GPIO "address_space_handler" should be provided by an ACPI GPIO address space module rather than provided by a particular GPIO driver.

Actually, without the readiness of the GPIO driver, currently ASL code can still access the GPIO fields, though it just results in "handler not found" ACPICA error message.
If the "setup" callback is implemented, then the proper GPIO driver can be matched in this "setup" callback.
If the GPIO driver hasn't been loaded by Linux and thus not matched in the "setup" callback, from users' point of view, the result is just changing the error message from "handler not found" to "driver not found".

Thanks and best regards
-Lv

Re: [PATCH 2/2] gpio / ACPI: add support for GPIO operation regions

[Mika Westerberg]

On Tue, Sep 24, 2013 at 12:47:56AM +0000, Zheng, Lv wrote:
> > From: Mika Westerberg [mailto:mika.westerberg@linux.intel.com]
> > Sent: Tuesday, September 17, 2013 4:37 PM
> > 
> > On Mon, Sep 16, 2013 at 11:35:56PM +0000, Zheng, Lv wrote:
> > > > From: Mika Westerberg [mailto:mika.westerberg@linux.intel.com]
> > > > Sent: Monday, September 16, 2013 4:11 PM
> > > >
> > > > On Mon, Sep 16, 2013 at 01:21:53AM +0000, Zheng, Lv wrote:
> > > > > > A pseudo device may be created to access the GPIO operation region fields provided by one GPIO device.
> > > > > > The pseudo device may have other functions to access other GPIO operation region fields provided by other GPIO devices, or even
> > worse
> > > > to
> > > > > > access other ACPI provided value-adds.
> > > > > > So hierarchically the pseudo device only requires CPU, thus should not be under the GPIO device, which means the GPIO operation
> > > > regions
> > > > > > have nothing to do with the GPIO devices' ACPI handle.
> > > > >
> > > > > Sorry for the wording.
> > > > > It's better to say the GPIO operation region users haven't strict
> > > > > relationship to the GPIO operation region providers.
> > > > > As the installation is to provide GPIO operation regions to the users, it
> > > > > shouldn't relate to the providers' ACPI handle.
> > > >
> > > > If I understand you correctly you mean that there might be multiple users
> > > > (different devices) for the same GPIO operation region, right?
> > >
> > > No, this is not what I meant.
> > > Can one vendor device uses two or more GPIO pins from different GPIO ports?
> > 
> > Yes.
> 
> So how can such a device under one of these GPIO ports?
> It can only be under one particular GPIO port device, right?
> 
> Thus I believe such device will appear below other devices rather than
> below a GPIO port device in the ACPI namespace.

I'm sorry, but I can't follow what you mean.

> > > > That shouldn't be a problem as far as I can tell.
> > > >
> > > > What comes to the hierarchy you refer, I'm not sure if that is a problem
> > > > either (unless I'm missing something). The GPIO can be used anywhere in the
> > > > ASL, it doesn't have to be descendant of the GPIO device. You only need to
> > > > do something like this:
> > > >
> > > > 	// Assert the GPIO
> > > > 	Store(1, \_SB.PCI0.SHD3)
> > > >
> > > > In other words, use the fully qualified name.
> > >
> > > Yes, which means this line of code can be everywhere in the namespace.
> > > It is not required to be under one particular GPIO device as long as there is an operation region defined in that scope.
> > >
> > > The problem is the installation, the first parameter for
> > > acpi_install_address_space_handler() is a namespace node, the function
> > > will call _REG for all OperationRegions below the scope whose top most
> > > node is the specified node.
> > > The nodes out of this scope are not affected.  So if an OperationRegion
> > > is defined out of this scope, problem happens.
> > 
> > I suppose for that operation region another GPIO device should be
> > introduced then, no?
> 
> I believe the answer is no.

Consider a situation, where you have two different GPIO controllers and the
ASL code needs to access GPIOs on both controllers. That requires
introducing two GPIO devices in ACPI namespace and two separate drivers as
well.

> 
> > So if we don't have a GPIO driver for the given operation region, what can
> > we do? We don't want the ASL code to erroneusly think that there is an
> > operation region available when it is not.
> 
> I think GPIO "address_space_handler" should be provided by an ACPI GPIO
> address space module rather than provided by a particular GPIO driver.
> 
> Actually, without the readiness of the GPIO driver, currently ASL code
> can still access the GPIO fields, though it just results in "handler not
> found" ACPICA error message.  If the "setup" callback is implemented,
> then the proper GPIO driver can be matched in this "setup" callback.  If
> the GPIO driver hasn't been loaded by Linux and thus not matched in the
> "setup" callback, from users' point of view, the result is just changing
> the error message from "handler not found" to "driver not found".

Well, I'm fine implementing a single global GPIO region handler (instead of
what is done in this series).

Rafael, do you have any comments on this?