On Tue, Sep 18, 2018 at 8:36 AM Mika Westerberg
<mika.westerb...@linux.intel.com> wrote:
>
> For some reason I thought GPIOLIB handles translation from GPIO ranges
> to pinctrl pins but it turns out not to be the case. This means that
> when GPIOs operations are performed for a pin controller having a custom
> GPIO base such as Cannon Lake and Ice Lake incorrect pin number gets
> used internally.
>
> Fix this in the same way we did for lock/unlock IRQ operations and
> translate the GPIO number to pin before using it.
>
> Fixes: a60eac3239f0 ("pinctrl: intel: Allow custom GPIO base for pad groups")
> Reported-by: Rajat Jain <raja...@google.com>
> Signed-off-by: Mika Westerberg <mika.westerb...@linux.intel.com>
Tested-by: Rajat Jain <raja...@google.com>
This has fixed the issue for me.
One question, may not be related: I see this line in my logs everytime
I export a pin (GPIO40 = pin 16 in this case). Is that an indication
of a problem?
"gpio gpiochip0: Persistence not supported for GPIO 40"
Thanks,
Rajat
On Tue, Sep 18, 2018 at 8:36 AM Mika Westerberg
<mika.westerb...@linux.intel.com> wrote:
>
> For some reason I thought GPIOLIB handles translation from GPIO ranges
> to pinctrl pins but it turns out not to be the case. This means that
> when GPIOs operations are performed for a pin controller having a custom
> GPIO base such as Cannon Lake and Ice Lake incorrect pin number gets
> used internally.
>
> Fix this in the same way we did for lock/unlock IRQ operations and
> translate the GPIO number to pin before using it.
>
> Fixes: a60eac3239f0 ("pinctrl: intel: Allow custom GPIO base for pad groups")
> Reported-by: Rajat Jain <raja...@google.com>
> Signed-off-by: Mika Westerberg <mika.westerb...@linux.intel.com>
> ---
> drivers/pinctrl/intel/pinctrl-intel.c | 111 +++++++++++++++-----------
> 1 file changed, 63 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/pinctrl/intel/pinctrl-intel.c
> b/drivers/pinctrl/intel/pinctrl-intel.c
> index 62b009b27eda..ec8dafc94694 100644
> --- a/drivers/pinctrl/intel/pinctrl-intel.c
> +++ b/drivers/pinctrl/intel/pinctrl-intel.c
> @@ -747,13 +747,63 @@ static const struct pinctrl_desc intel_pinctrl_desc = {
> .owner = THIS_MODULE,
> };
>
> +/**
> + * intel_gpio_to_pin() - Translate from GPIO offset to pin number
> + * @pctrl: Pinctrl structure
> + * @offset: GPIO offset from gpiolib
> + * @commmunity: Community is filled here if not %NULL
> + * @padgrp: Pad group is filled here if not %NULL
> + *
> + * When coming through gpiolib irqchip, the GPIO offset is not
> + * automatically translated to pinctrl pin number. This function can be
> + * used to find out the corresponding pinctrl pin.
> + */
> +static int intel_gpio_to_pin(struct intel_pinctrl *pctrl, unsigned offset,
> + const struct intel_community **community,
> + const struct intel_padgroup **padgrp)
> +{
> + int i;
> +
> + for (i = 0; i < pctrl->ncommunities; i++) {
> + const struct intel_community *comm = &pctrl->communities[i];
> + int j;
> +
> + for (j = 0; j < comm->ngpps; j++) {
> + const struct intel_padgroup *pgrp = &comm->gpps[j];
> +
> + if (pgrp->gpio_base < 0)
> + continue;
> +
> + if (offset >= pgrp->gpio_base &&
> + offset < pgrp->gpio_base + pgrp->size) {
> + int pin;
> +
> + pin = pgrp->base + offset - pgrp->gpio_base;
> + if (community)
> + *community = comm;
> + if (padgrp)
> + *padgrp = pgrp;
> +
> + return pin;
> + }
> + }
> + }
> +
> + return -EINVAL;
> +}
> +
> static int intel_gpio_get(struct gpio_chip *chip, unsigned offset)
> {
> struct intel_pinctrl *pctrl = gpiochip_get_data(chip);
> void __iomem *reg;
> u32 padcfg0;
> + int pin;
> +
> + pin = intel_gpio_to_pin(pctrl, offset, NULL, NULL);
> + if (pin < 0)
> + return -EINVAL;
>
> - reg = intel_get_padcfg(pctrl, offset, PADCFG0);
> + reg = intel_get_padcfg(pctrl, pin, PADCFG0);
> if (!reg)
> return -EINVAL;
>
> @@ -770,8 +820,13 @@ static void intel_gpio_set(struct gpio_chip *chip,
> unsigned offset, int value)
> unsigned long flags;
> void __iomem *reg;
> u32 padcfg0;
> + int pin;
> +
> + pin = intel_gpio_to_pin(pctrl, offset, NULL, NULL);
> + if (pin < 0)
> + return;
>
> - reg = intel_get_padcfg(pctrl, offset, PADCFG0);
> + reg = intel_get_padcfg(pctrl, pin, PADCFG0);
> if (!reg)
> return;
>
> @@ -790,8 +845,13 @@ static int intel_gpio_get_direction(struct gpio_chip
> *chip, unsigned int offset)
> struct intel_pinctrl *pctrl = gpiochip_get_data(chip);
> void __iomem *reg;
> u32 padcfg0;
> + int pin;
>
> - reg = intel_get_padcfg(pctrl, offset, PADCFG0);
> + pin = intel_gpio_to_pin(pctrl, offset, NULL, NULL);
> + if (pin < 0)
> + return -EINVAL;
> +
> + reg = intel_get_padcfg(pctrl, pin, PADCFG0);
> if (!reg)
> return -EINVAL;
>
> @@ -827,51 +887,6 @@ static const struct gpio_chip intel_gpio_chip = {
> .set_config = gpiochip_generic_config,
> };
>
> -/**
> - * intel_gpio_to_pin() - Translate from GPIO offset to pin number
> - * @pctrl: Pinctrl structure
> - * @offset: GPIO offset from gpiolib
> - * @commmunity: Community is filled here if not %NULL
> - * @padgrp: Pad group is filled here if not %NULL
> - *
> - * When coming through gpiolib irqchip, the GPIO offset is not
> - * automatically translated to pinctrl pin number. This function can be
> - * used to find out the corresponding pinctrl pin.
> - */
> -static int intel_gpio_to_pin(struct intel_pinctrl *pctrl, unsigned offset,
> - const struct intel_community **community,
> - const struct intel_padgroup **padgrp)
> -{
> - int i;
> -
> - for (i = 0; i < pctrl->ncommunities; i++) {
> - const struct intel_community *comm = &pctrl->communities[i];
> - int j;
> -
> - for (j = 0; j < comm->ngpps; j++) {
> - const struct intel_padgroup *pgrp = &comm->gpps[j];
> -
> - if (pgrp->gpio_base < 0)
> - continue;
> -
> - if (offset >= pgrp->gpio_base &&
> - offset < pgrp->gpio_base + pgrp->size) {
> - int pin;
> -
> - pin = pgrp->base + offset - pgrp->gpio_base;
> - if (community)
> - *community = comm;
> - if (padgrp)
> - *padgrp = pgrp;
> -
> - return pin;
> - }
> - }
> - }
> -
> - return -EINVAL;
> -}
> -
> static int intel_gpio_irq_reqres(struct irq_data *d)
> {
> struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> --
> 2.18.0
>
