Re: [PATCH 1/3] gpio / ACPI: add ACPI support

[Mathias Nyman <mathias.nyman@linux.intel.com>]

On 11/08/2012 09:38 PM, Mika Westerberg wrote:
...
>>> +#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.
>
> AFAIK no but I'll let Mathias to answer that as he knows this better.

I'm interpreting it the same way as Mika, max one actual controller per 
ACPI device node

The path (called ResourceSource in ACPI5 specs) in GpioIO/GpioInt 
resources is a "string which uniquely identifies the GPIO controller 
referred to by this descriptor."  The pin number is zero based 
controller relative.

The  ACPI device controller node includes all other resources needed by 
the controller driver (ioport/mem base, range, interrupt, and Hardware 
ID used to pair with a driver)

Checked a board with two identical gpio controllers on it and it had two 
separate ACPI device node entries. (with only different io address base 
and interrupt resources)

-Mathias

>
>> 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.
>
> Yeah, this patch just mimics the DT version but in general it would be
> better if there was only one API to get the GPIO. There has been discussion
> about adding gpio_get() or something similar which could perhaps be used to
> abstract away DT or ACPI.
>
> We use this in a driver so that we walk through the ACPI resources for a
> given device (if we have the ACPI handle) and parse the GpioIO/GpioInt
> resources like:
>
> 	struct acpi_resource_gpio *acpi_gpio;
> 	struct acpi_device *adev;
> 	acpi_resource *res;
> 	int gpio;
>
> 	/* obtain the ACPI device from handle */
> 	...
>
> 	/* walk through the resources attached to adev */
> 	...
> 		switch (res->type) {
> 		case ACPI_RESOURCE_TYPE_GPIO:
> 			acpi_gpio =&res->data.gpio;
>
> 			gpio = acpi_get_gpio(acpi_gpio->resource_source.string_ptr,
> 					     acpi_gpio->pin_table[0]);
> 			...
> 		}