Re: [PATCH 1/4] gpio / ACPI: Add knowledge about pin controllers to acpi_get_gpiod()

[Mika Westerberg]

On Wed, Oct 29, 2014 at 11:11:00PM +0100, Rafael J. Wysocki wrote:
> On Monday, October 27, 2014 10:08:29 AM Mika Westerberg wrote:
> > The GPIO resources (GpioIo/GpioInt) used in ACPI contain a GPIO number
> > which is relative to the hardware GPIO controller. Typically this number
> > can be translated directly to Linux GPIO number because the mapping is
> > pretty much 1:1.
> > 
> > However, when the GPIO driver is using pins exported by a pin controller
> > driver via set of GPIO ranges, the mapping might not be 1:1 anymore and
> > direct translation does not work.
> > 
> > In such cases we need to translate the ACPI GPIO number to be suitable for
> > the GPIO controller driver in question by checking all the pin controller
> > GPIO ranges under the given device and using those to get the proper GPIO
> > number.
> > 
> > Signed-off-by: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
> > ---
> > Rafael, are you OK with this change?
> 
> Yes, I am, so
> 
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>

Thanks.

> And I have no idea how to do that in a more straightforward way.
> 
> Of course ->
> 
> > 
> >  drivers/gpio/gpiolib-acpi.c | 62 ++++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 59 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> > index 05c6275da224..4f2c4adccb8f 100644
> > --- a/drivers/gpio/gpiolib-acpi.c
> > +++ b/drivers/gpio/gpiolib-acpi.c
> > @@ -11,12 +11,14 @@
> >   */
> >  
> >  #include <linux/errno.h>
> > +#include <linux/gpio.h>
> >  #include <linux/gpio/consumer.h>
> >  #include <linux/gpio/driver.h>
> >  #include <linux/export.h>
> >  #include <linux/acpi.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/mutex.h>
> > +#include <linux/pinctrl/pinctrl.h>
> >  
> >  #include "gpiolib.h"
> >  
> > @@ -55,6 +57,58 @@ static int acpi_gpiochip_find(struct gpio_chip *gc, void *data)
> >  	return ACPI_HANDLE(gc->dev) == data;
> >  }
> >  
> > +#ifdef CONFIG_PINCTRL
> > +/**
> > + * acpi_gpiochip_pin_to_gpio_offset() - translates ACPI GPIO to Linux GPIO
> > + * @chip: GPIO chip
> > + * @pin: ACPI GPIO pin number from GpioIo/GpioInt resource
> > + *
> > + * Function takes ACPI GpioIo/GpioInt pin number as a parameter and
> > + * translates it to a corresponding offset suitable to be passed to a
> > + * GPIO controller driver.
> > + *
> > + * Typically the returned offset is same as @pin, but if the GPIO
> > + * controller uses pin controller and the mapping is not contigous the
> > + * offset might be different.
> > + */
> > +static int acpi_gpiochip_pin_to_gpio_offset(struct gpio_chip *chip, int pin)
> > +{
> > +	struct gpio_pin_range *pin_range;
> > +
> > +	/* If there are no ranges in this chip, use 1:1 mapping */
> > +	if (list_empty(&chip->pin_ranges))
> > +		return pin;
> > +
> > +	list_for_each_entry(pin_range, &chip->pin_ranges, node) {
> > +		const struct pinctrl_gpio_range *range = &pin_range->range;
> > +		int i;
> > +
> > +		if (range->pins) {
> > +			for (i = 0; i < range->npins; i++) {
> > +				if (range->pins[i] == pin)
> > +					return range->base + i - chip->base;
> > +			}
> > +		} else {
> > +			if (pin >= range->pin_base &&
> > +			    pin < range->pin_base + range->npins) {
> > +				unsigned gpio_base;
> > +
> > +				gpio_base = range->base - chip->base;
> > +				return gpio_base + pin - range->pin_base;
> > +			}
> > +		}
> 
> -> this is only going to work if the pin mapping in chip->pin_ranges doesn't
> change after it has returned the offset, but I'm assiming that this is the case.

I think that is the case here. At least the pinctrl-cherryview.c sets up
GPIO <-> pin mappings only once in probe().

> > +	}
> > +
> > +	return -EINVAL;
> > +}
> > +#else
> > +static inline int acpi_gpiochip_pin_to_gpio_offset(struct gpio_chip *chip,
> > +						   int pin)
> > +{
> > +	return pin;
> > +}
> > +#endif
> > +
> >  /**
> >   * acpi_get_gpiod() - Translate ACPI GPIO pin to GPIO descriptor usable with GPIO API
> >   * @path:	ACPI GPIO controller full path name, (e.g. "\\_SB.GPO1")
> > @@ -69,6 +123,7 @@ static struct gpio_desc *acpi_get_gpiod(char *path, int pin)
> >  	struct gpio_chip *chip;
> >  	acpi_handle handle;
> >  	acpi_status status;
> > +	int offset;
> >  
> >  	status = acpi_get_handle(NULL, path, &handle);
> >  	if (ACPI_FAILURE(status))
> > @@ -78,10 +133,11 @@ static struct gpio_desc *acpi_get_gpiod(char *path, int pin)
> >  	if (!chip)
> >  		return ERR_PTR(-ENODEV);
> >  
> > -	if (pin < 0 || pin > chip->ngpio)
> > -		return ERR_PTR(-EINVAL);
> > +	offset = acpi_gpiochip_pin_to_gpio_offset(chip, pin);
> > +	if (offset < 0)
> > +		return ERR_PTR(offset);
> >  
> > -	return gpiochip_get_desc(chip, pin);
> > +	return gpiochip_get_desc(chip, (u16)offset);
> 
> Silly question: Why do we need the explicit u16 cast now?

It seems that we don't need it after all.

I'll fix this up in the next version, given that Linus W. is happy about
the approach I'm using here.

> 
> >  }
> >  
> >  static irqreturn_t acpi_gpio_irq_handler(int irq, void *data)
> > 
> 
> -- 
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.
--
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/