Re: [PATCH 4/4] pinctrl: Add Intel Cherryview/Braswell pin controller support

Re: [PATCH 4/4] pinctrl: Add Intel Cherryview/Braswell pin controller support

[Linus Walleij]

On Fri, Oct 24, 2014 at 2:16 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:

> This driver supports the pin/GPIO controllers found in newer Intel SoCs
> like Cherryview and Braswell. The driver provides full GPIO support and
> minimal set of pin controlling funtionality.
>
> The driver is based on the original Cherryview GPIO driver authored by Ning
> Li and Alan Cox.
>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>

*VERY* nice work Mika! Just minor nitpicks...

(...)
> +static int chv_config_get(struct pinctrl_dev *pctldev, unsigned pin,
> +                         unsigned long *config)
> +{
> +       struct chv_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> +       enum pin_config_param param = pinconf_to_config_param(*config);
> +       unsigned long flags;
> +       u32 ctrl0, ctrl1;
> +       u16 arg = 0;
> +       u32 term;
> +
> +       spin_lock_irqsave(&pctrl->lock, flags);
> +       ctrl0 = readl(chv_padreg(pctrl, pin, CHV_PADCTRL0));
> +       ctrl1 = readl(chv_padreg(pctrl, pin, CHV_PADCTRL1));
> +       spin_unlock_irqrestore(&pctrl->lock, flags);
> +
> +       term = (ctrl0 & CHV_PADCTRL0_TERM_MASK) >> CHV_PADCTRL0_TERM_SHIFT;
> +
> +       switch (param) {
> +       case PIN_CONFIG_BIAS_DISABLE:
> +               if (term)
> +                       return -EINVAL;
> +               break;
> +
> +       case PIN_CONFIG_BIAS_PULL_UP:
> +               if (!(ctrl0 & CHV_PADCTRL0_TERM_UP))
> +                       return -EINVAL;
> +
> +               switch (term) {
> +               case CHV_PADCTRL0_TERM_20K:
> +                       arg = 20;

These are in Ohms IIRC so should be 20000

> +                       break;
> +               case CHV_PADCTRL0_TERM_5K:
> +                       arg = 5;

5000

> +                       break;
> +               case CHV_PADCTRL0_TERM_1K:
> +                       arg = 1;

1000

> +                       break;
> +               }
> +
> +               break;
> +
> +       case PIN_CONFIG_BIAS_PULL_DOWN:
> +               if (!term || (ctrl0 & CHV_PADCTRL0_TERM_UP))
> +                       return -EINVAL;
> +
> +               switch (term) {
> +               case CHV_PADCTRL0_TERM_20K:
> +                       arg = 20;

20000

> +                       break;
> +               case CHV_PADCTRL0_TERM_5K:
> +                       arg = 5;

5000

(...)
> +static int chv_config_set_pull(struct chv_pinctrl *pctrl, unsigned pin,
> +                              enum pin_config_param param, u16 arg)
> +{
> +       void __iomem *reg = chv_padreg(pctrl, pin, CHV_PADCTRL0);
> +       unsigned long flags;
> +       u32 ctrl0, pull;
> +
> +       spin_lock_irqsave(&pctrl->lock, flags);
> +       ctrl0 = readl(reg);
> +
> +       pull = CHV_PADCTRL0_TERM_20K << CHV_PADCTRL0_TERM_SHIFT;
> +       switch (arg) {

This looks seriously convoluted: you can't inspect an argument before
checking what parameter you're dealing with. This should be
under a case PIN_CONFIG_BIAS_PULL_UP in the switch (param)
below I think?

> +       case 1:

case 1000

> +               /* For 1k there is only pull up */
> +               if (param == PIN_CONFIG_BIAS_PULL_UP)
> +                       pull = CHV_PADCTRL0_TERM_1K << CHV_PADCTRL0_TERM_SHIFT;

Well you do check it here but...0

> +               break;
> +       case 5:

case 5000

> +               pull = CHV_PADCTRL0_TERM_5K << CHV_PADCTRL0_TERM_SHIFT;

This will be applied to whatever config with arg == 5000 comes here!

(...)
> +static unsigned chv_gpio_offset_to_pin(struct chv_pinctrl *pctrl,
> +                                      unsigned offset)
> +{
> +       return pctrl->community->pins[offset].number;
> +}

I'm a bit worried about the massive pin<->offsets<->gpio# translations
happening in this and patch 1/4 etc. It's a bit unsettling. Are you
sure we are translating in the simplest way?

> +static int chv_gpio_get(struct gpio_chip *chip, unsigned offset)
> +{
> +       struct chv_pinctrl *pctrl = gpiochip_to_pinctrl(chip);
> +       int pin = chv_gpio_offset_to_pin(pctrl, offset);
> +       unsigned long flags;
> +       u32 ctrl0, cfg;
> +
> +       spin_lock_irqsave(&pctrl->lock, flags);
> +       ctrl0 = readl(chv_padreg(pctrl, pin, CHV_PADCTRL0));
> +       spin_unlock_irqrestore(&pctrl->lock, flags);

If you need a lock before and after reading every register in this
range, consider regmap-mmio, because that is part of what
it does. Just a hint...

(...)
> +static void chv_gpio_irq_mask_unmask(struct irq_data *d, bool mask)
> +{
> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +       struct chv_pinctrl *pctrl = gpiochip_to_pinctrl(gc);
> +       int pin = chv_gpio_offset_to_pin(pctrl, irqd_to_hwirq(d));
> +       u32 value, intr_line;
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&pctrl->lock, flags);
> +
> +       intr_line = readl(chv_padreg(pctrl, pin, CHV_PADCTRL0));
> +       intr_line &= CHV_PADCTRL0_INTSEL_MASK;
> +       intr_line >>= CHV_PADCTRL0_INTSEL_SHIFT;
> +
> +       value = readl(pctrl->regs + CHV_INTMASK);
> +       if (mask)
> +               value &= ~(1 << intr_line);

I usually do this kind of stuff with

#include <linux/bitops.h>

value &= ~BIT(intr_line);

> +       else
> +               value |= (1 << intr_line);

value |= BIT(intr_line);

(probably a few more occasions in the driver)

Yours,
Linus Walleij

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

[Linus Walleij]

On Fri, Oct 24, 2014 at 2:16 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> 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@linux.intel.com>

I'm not sure what this patch does so I try to rely on you guys here.
I would strongly like to have Rafael's ACK before proceeding.

Trying to get the GPIO offsets to map 1:1 to Linux GPIO numbers
is something that should be avoided since we want to get rid of the
GPIO number space altogether. GPIO chips should try to register
with .base = -1 so that the gpiolib just assigns some random GPIO
numbers to the lines.

What does actually exist?

- GPIO offsets for a certain gpio_chip 0..N
- Pin offsets for a certain pin controller 0..N

GPIO ranges are for translating between these two.

But I have a vague idea that there is something like a numberspace
concept inside ACPI as well, and that is all magic to me...
mixing that into this is a bit scary to me.

> +#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

So I guess I'm curious about this Gpiolo/GpioInt resource number space...

Yours,
Linus Walleij

Re: [PATCH 2/4] pinctrl: Move Intel Baytrail pinctrl driver under intel directory

[Linus Walleij]

On Fri, Oct 24, 2014 at 2:16 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:

> We are going to have more pinctrl drivers for Intel hardware so separate
> all our pin controller drivers to own directory.
>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Patch applied.

Yours,
Linus Walleij

Re: [PATCH 3/4] MAINTAINERS: Add entry for Intel pin controller drivers

[Linus Walleij]

On Fri, Oct 24, 2014 at 2:16 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:

> Add MAINTAINERS entry for Intel pin controller drivers. I will be
> maintaining them with Heikki, who kindly promised to help me with this.
>
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Patch applied.

Yours,
Linus Walleij

Re: [PATCH 4/4] pinctrl: Add Intel Cherryview/Braswell pin controller support

[Mika Westerberg]

On Wed, Oct 29, 2014 at 10:35:01AM +0100, Linus Walleij wrote:
> On Fri, Oct 24, 2014 at 2:16 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> 
> > This driver supports the pin/GPIO controllers found in newer Intel SoCs
> > like Cherryview and Braswell. The driver provides full GPIO support and
> > minimal set of pin controlling funtionality.
> >
> > The driver is based on the original Cherryview GPIO driver authored by Ning
> > Li and Alan Cox.
> >
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> 
> *VERY* nice work Mika! Just minor nitpicks...
> 
> (...)
> > +static int chv_config_get(struct pinctrl_dev *pctldev, unsigned pin,
> > +                         unsigned long *config)
> > +{
> > +       struct chv_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> > +       enum pin_config_param param = pinconf_to_config_param(*config);
> > +       unsigned long flags;
> > +       u32 ctrl0, ctrl1;
> > +       u16 arg = 0;
> > +       u32 term;
> > +
> > +       spin_lock_irqsave(&pctrl->lock, flags);
> > +       ctrl0 = readl(chv_padreg(pctrl, pin, CHV_PADCTRL0));
> > +       ctrl1 = readl(chv_padreg(pctrl, pin, CHV_PADCTRL1));
> > +       spin_unlock_irqrestore(&pctrl->lock, flags);
> > +
> > +       term = (ctrl0 & CHV_PADCTRL0_TERM_MASK) >> CHV_PADCTRL0_TERM_SHIFT;
> > +
> > +       switch (param) {
> > +       case PIN_CONFIG_BIAS_DISABLE:
> > +               if (term)
> > +                       return -EINVAL;
> > +               break;
> > +
> > +       case PIN_CONFIG_BIAS_PULL_UP:
> > +               if (!(ctrl0 & CHV_PADCTRL0_TERM_UP))
> > +                       return -EINVAL;
> > +
> > +               switch (term) {
> > +               case CHV_PADCTRL0_TERM_20K:
> > +                       arg = 20;
> 
> These are in Ohms IIRC so should be 20000
> 
> > +                       break;
> > +               case CHV_PADCTRL0_TERM_5K:
> > +                       arg = 5;
> 
> 5000
> 
> > +                       break;
> > +               case CHV_PADCTRL0_TERM_1K:
> > +                       arg = 1;
> 
> 1000
> 
> > +                       break;
> > +               }
> > +
> > +               break;
> > +
> > +       case PIN_CONFIG_BIAS_PULL_DOWN:
> > +               if (!term || (ctrl0 & CHV_PADCTRL0_TERM_UP))
> > +                       return -EINVAL;
> > +
> > +               switch (term) {
> > +               case CHV_PADCTRL0_TERM_20K:
> > +                       arg = 20;
> 
> 20000
> 
> > +                       break;
> > +               case CHV_PADCTRL0_TERM_5K:
> > +                       arg = 5;
> 
> 5000

Right, I'll change them.

> (...)
> > +static int chv_config_set_pull(struct chv_pinctrl *pctrl, unsigned pin,
> > +                              enum pin_config_param param, u16 arg)
> > +{
> > +       void __iomem *reg = chv_padreg(pctrl, pin, CHV_PADCTRL0);
> > +       unsigned long flags;
> > +       u32 ctrl0, pull;
> > +
> > +       spin_lock_irqsave(&pctrl->lock, flags);
> > +       ctrl0 = readl(reg);
> > +
> > +       pull = CHV_PADCTRL0_TERM_20K << CHV_PADCTRL0_TERM_SHIFT;
> > +       switch (arg) {
> 
> This looks seriously convoluted: you can't inspect an argument before
> checking what parameter you're dealing with. This should be
> under a case PIN_CONFIG_BIAS_PULL_UP in the switch (param)
> below I think?

OK.

> 
> > +       case 1:
> 
> case 1000
> 
> > +               /* For 1k there is only pull up */
> > +               if (param == PIN_CONFIG_BIAS_PULL_UP)
> > +                       pull = CHV_PADCTRL0_TERM_1K << CHV_PADCTRL0_TERM_SHIFT;
> 
> Well you do check it here but...0
> 
> > +               break;
> > +       case 5:
> 
> case 5000
> 
> > +               pull = CHV_PADCTRL0_TERM_5K << CHV_PADCTRL0_TERM_SHIFT;
> 
> This will be applied to whatever config with arg == 5000 comes here!
> 
> (...)
> > +static unsigned chv_gpio_offset_to_pin(struct chv_pinctrl *pctrl,
> > +                                      unsigned offset)
> > +{
> > +       return pctrl->community->pins[offset].number;
> > +}
> 
> I'm a bit worried about the massive pin<->offsets<->gpio# translations
> happening in this and patch 1/4 etc. It's a bit unsettling. Are you
> sure we are translating in the simplest way?

We are translating from ACPI GPIO number, which is relative to the GPIO
controller in question to pin controller space (which is not 1:1 in this
driver).

I'll double check this, just in case.

> > +static int chv_gpio_get(struct gpio_chip *chip, unsigned offset)
> > +{
> > +       struct chv_pinctrl *pctrl = gpiochip_to_pinctrl(chip);
> > +       int pin = chv_gpio_offset_to_pin(pctrl, offset);
> > +       unsigned long flags;
> > +       u32 ctrl0, cfg;
> > +
> > +       spin_lock_irqsave(&pctrl->lock, flags);
> > +       ctrl0 = readl(chv_padreg(pctrl, pin, CHV_PADCTRL0));
> > +       spin_unlock_irqrestore(&pctrl->lock, flags);
> 
> If you need a lock before and after reading every register in this
> range, consider regmap-mmio, because that is part of what
> it does. Just a hint...

Yeah, I don't think we need a lock for reading simple registers so I'll
remove such usage from the driver.

> (...)
> > +static void chv_gpio_irq_mask_unmask(struct irq_data *d, bool mask)
> > +{
> > +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> > +       struct chv_pinctrl *pctrl = gpiochip_to_pinctrl(gc);
> > +       int pin = chv_gpio_offset_to_pin(pctrl, irqd_to_hwirq(d));
> > +       u32 value, intr_line;
> > +       unsigned long flags;
> > +
> > +       spin_lock_irqsave(&pctrl->lock, flags);
> > +
> > +       intr_line = readl(chv_padreg(pctrl, pin, CHV_PADCTRL0));
> > +       intr_line &= CHV_PADCTRL0_INTSEL_MASK;
> > +       intr_line >>= CHV_PADCTRL0_INTSEL_SHIFT;
> > +
> > +       value = readl(pctrl->regs + CHV_INTMASK);
> > +       if (mask)
> > +               value &= ~(1 << intr_line);
> 
> I usually do this kind of stuff with
> 
> #include <linux/bitops.h>
> 
> value &= ~BIT(intr_line);
> 
> > +       else
> > +               value |= (1 << intr_line);
> 
> value |= BIT(intr_line);
> 
> (probably a few more occasions in the driver)

OK, will fix.

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

[Mika Westerberg]

On Wed, Oct 29, 2014 at 10:41:19AM +0100, Linus Walleij wrote:
> On Fri, Oct 24, 2014 at 2:16 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> 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@linux.intel.com>
> 
> I'm not sure what this patch does so I try to rely on you guys here.
> I would strongly like to have Rafael's ACK before proceeding.
> 
> Trying to get the GPIO offsets to map 1:1 to Linux GPIO numbers
> is something that should be avoided since we want to get rid of the
> GPIO number space altogether. GPIO chips should try to register
> with .base = -1 so that the gpiolib just assigns some random GPIO
> numbers to the lines.
> 
> What does actually exist?
> 
> - GPIO offsets for a certain gpio_chip 0..N
> - Pin offsets for a certain pin controller 0..N
> 
> GPIO ranges are for translating between these two.
> 
> But I have a vague idea that there is something like a numberspace
> concept inside ACPI as well, and that is all magic to me...
> mixing that into this is a bit scary to me.

Let me try to explain.

As an example, we have touch panel that has GPIO interrupt declared in
ACPI like this:

	GpioInt (Level, ActiveLow, Shared, PullDefault, 0x0000,
		 "\\_SB.GPO0", 0x00, ResourceConsumer)
	{   // Pin list
		76
	}

The number 76 in this case is GPIO controller relative GPIO number.

Now, we have pinctrl-cherryview.c which has gaps in the pin space (some
of the pins are not available to the OS at all). So for this particular
pin we have:

	# grep SATA_GP1 pins
	pin 76 (SATA_GP1) mode 1 ctrl0 0x00010200 ctrl1 0x04c00000

	# cat gpio-ranges 
	GPIO ranges handled:
	0: INT33FF:00 GPIOS [456 - 463] PINS [0 - 7]
	8: INT33FF:00 GPIOS [464 - 471] PINS [15 - 22]
	16: INT33FF:00 GPIOS [472 - 479] PINS [30 - 37]
	24: INT33FF:00 GPIOS [480 - 487] PINS [45 - 52]
	32: INT33FF:00 GPIOS [488 - 495] PINS [60 - 67]
	40: INT33FF:00 GPIOS [496 - 503] PINS [75 - 82]
	48: INT33FF:00 GPIOS [504 - 511] PINS [90 - 97]

In other words GPIO number for pin 76 is 496 (in Linux global number
space) and 41 in the GPIO driver local number space.

In order to translate the ACPI GPIO number 76 to the right GPIO number
we iterate over each range in the GPIO driver and look for the match.
Note that it actually uses the local number (41) here:

	...
        offset = acpi_gpiochip_pin_to_gpio_offset(chip, pin);
        if (offset < 0)
                return ERR_PTR(offset);

        return gpiochip_get_desc(chip, (u16)offset);

However, if you or others have better ideas how to do this I'm all ears
:-)

> > +#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
> 
> So I guess I'm curious about this Gpiolo/GpioInt resource number space...

In the above example:

	GpioInt (Level, ActiveLow, Shared, PullDefault, 0x0000,
		 "\\_SB.GPO0", 0x00, ResourceConsumer)
	{   // Pin list
		76
	}

The GPIO number (76) is relative to the GPIO controller in question
(\_SB.GPO0). 0 would be the first GPIO and so on...

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

[Linus Walleij]

On Wed, Oct 29, 2014 at 11:27 AM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Wed, Oct 29, 2014 at 10:41:19AM +0100, Linus Walleij wrote:

>> But I have a vague idea that there is something like a numberspace
>> concept inside ACPI as well, and that is all magic to me...
>> mixing that into this is a bit scary to me.
(...)
>         # grep SATA_GP1 pins
>         pin 76 (SATA_GP1) mode 1 ctrl0 0x00010200 ctrl1 0x04c00000
>
>         # cat gpio-ranges
>         GPIO ranges handled:
>         0: INT33FF:00 GPIOS [456 - 463] PINS [0 - 7]
>         8: INT33FF:00 GPIOS [464 - 471] PINS [15 - 22]
>         16: INT33FF:00 GPIOS [472 - 479] PINS [30 - 37]
>         24: INT33FF:00 GPIOS [480 - 487] PINS [45 - 52]
>         32: INT33FF:00 GPIOS [488 - 495] PINS [60 - 67]
>         40: INT33FF:00 GPIOS [496 - 503] PINS [75 - 82]
>         48: INT33FF:00 GPIOS [504 - 511] PINS [90 - 97]
>
> In other words GPIO number for pin 76 is 496 (in Linux global number
> space) and 41 in the GPIO driver local number space.
>
> In order to translate the ACPI GPIO number 76 to the right GPIO number
> we iterate over each range in the GPIO driver and look for the match.
> Note that it actually uses the local number (41) here:

OK what I was mainly worrying about was that it would go and
use the Linux GPIO numbers in any way. As long as the mechanism
relies on the local pin controller or GPIO chip offsets I'm happy.

Yours,
Linus Walleij

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@linux.intel.com>
> > ---
> > Rafael, are you OK with this change?
> 
> Yes, I am, so
> 
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

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.

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

[Rafael J. Wysocki]

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@linux.intel.com>
> ---
> Rafael, are you OK with this change?

Yes, I am, so

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

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.

> +	}
> +
> +	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?

>  }
>  
>  static irqreturn_t acpi_gpio_irq_handler(int irq, void *data)
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

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

[Mika Westerberg]

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@linux.intel.com>
---
Rafael, are you OK with this change?

 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;
+			}
+		}
+	}
+
+	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);
 }
 
 static irqreturn_t acpi_gpio_irq_handler(int irq, void *data)
-- 
2.1.1