Re: [PATCH] Revert "gpio / ACPI: Drop unnecessary ACPI GPIO to Linux GPIO translation"

[Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>]

On Thu, Apr 19, 2018 at 01:17:55PM -0700, Sultan Alsawaf wrote:
> This reverts commit 03c4749dd6c7ff948a0ce59a44a1b97c015353c2.
> 
> This broke the keyboard on at least the Acer CB3-431 Chromebook (Edgar).

Can you provide acpidump of the system? I suspect this is another place
where Linux GPIO number is hardcoded into ACPI tables :-/

Also can you send me contents of /proc/interrupts before and after the
patch is reverted.

> Signed-off-by: Sultan Alsawaf <sultanxda@xxxxxxxxx>
> ---
>  drivers/gpio/gpiolib-acpi.c                | 75 +++++++++++++++++++++-
>  drivers/pinctrl/intel/pinctrl-cherryview.c | 59 +++++++++++------
>  2 files changed, 112 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> index e2232cbce..0ecffd172 100644
> --- a/drivers/gpio/gpiolib-acpi.c
> +++ b/drivers/gpio/gpiolib-acpi.c
> @@ -58,6 +58,58 @@ static int acpi_gpiochip_find(struct gpio_chip *gc, void *data)
>  	return ACPI_HANDLE(gc->parent) == data;
>  }
>  
> +#ifdef CONFIG_PINCTRL
> +/**
> + * acpi_gpiochip_pin_to_gpio_offset() - translates ACPI GPIO to Linux GPIO
> + * @gdev: GPIO device
> + * @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 contiguous the
> + * offset might be different.
> + */
> +static int acpi_gpiochip_pin_to_gpio_offset(struct gpio_device *gdev, int pin)
> +{
> +	struct gpio_pin_range *pin_range;
> +
> +	/* If there are no ranges in this chip, use 1:1 mapping */
> +	if (list_empty(&gdev->pin_ranges))
> +		return pin;
> +
> +	list_for_each_entry(pin_range, &gdev->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 - gdev->base;
> +			}
> +		} else {
> +			if (pin >= range->pin_base &&
> +			    pin < range->pin_base + range->npins) {
> +				unsigned gpio_base;
> +
> +				gpio_base = range->base - gdev->base;
> +				return gpio_base + pin - range->pin_base;
> +			}
> +		}
> +	}
> +
> +	return -EINVAL;
> +}
> +#else
> +static inline int acpi_gpiochip_pin_to_gpio_offset(struct gpio_device *gdev,
> +						   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")
> @@ -73,6 +125,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))
> @@ -82,7 +135,11 @@ static struct gpio_desc *acpi_get_gpiod(char *path, int pin)
>  	if (!chip)
>  		return ERR_PTR(-EPROBE_DEFER);
>  
> -	return gpiochip_get_desc(chip, pin);
> +	offset = acpi_gpiochip_pin_to_gpio_offset(chip->gpiodev, pin);
> +	if (offset < 0)
> +		return ERR_PTR(offset);
> +
> +	return gpiochip_get_desc(chip, offset);
>  }
>  
>  static irqreturn_t acpi_gpio_irq_handler(int irq, void *data)
> @@ -159,6 +216,10 @@ static acpi_status acpi_gpiochip_request_interrupt(struct acpi_resource *ares,
>  	if (!handler)
>  		return AE_OK;
>  
> +	pin = acpi_gpiochip_pin_to_gpio_offset(chip->gpiodev, pin);
> +	if (pin < 0)
> +		return AE_BAD_PARAMETER;
> +
>  	desc = gpiochip_request_own_desc(chip, pin, "ACPI:Event");
>  	if (IS_ERR(desc)) {
>  		dev_err(chip->parent, "Failed to request GPIO\n");
> @@ -810,6 +871,12 @@ acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
>  		struct gpio_desc *desc;
>  		bool found;
>  
> +		pin = acpi_gpiochip_pin_to_gpio_offset(chip->gpiodev, pin);
> +		if (pin < 0) {
> +			status = AE_BAD_PARAMETER;
> +			goto out;
> +		}
> +
>  		mutex_lock(&achip->conn_lock);
>  
>  		found = false;
> @@ -942,7 +1009,11 @@ static struct gpio_desc *acpi_gpiochip_parse_own_gpio(
>  	if (ret < 0)
>  		return ERR_PTR(ret);
>  
> -	desc = gpiochip_get_desc(chip, gpios[0]);
> +	ret = acpi_gpiochip_pin_to_gpio_offset(chip->gpiodev, gpios[0]);
> +	if (ret < 0)
> +		return ERR_PTR(ret);
> +
> +	desc = gpiochip_get_desc(chip, ret);
>  	if (IS_ERR(desc))
>  		return desc;
>  
> diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
> index b1ae1618f..4471fd94e 100644
> --- a/drivers/pinctrl/intel/pinctrl-cherryview.c
> +++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
> @@ -131,8 +131,10 @@ struct chv_gpio_pinrange {
>   * @ngroups: Number of groups
>   * @functions: All functions in this community
>   * @nfunctions: Number of functions
> + * @ngpios: Number of GPIOs in this community
>   * @gpio_ranges: An array of GPIO ranges in this community
>   * @ngpio_ranges: Number of GPIO ranges
> + * @ngpios: Total number of GPIOs in this community
>   * @nirqs: Total number of IRQs this community can generate
>   */
>  struct chv_community {
> @@ -145,6 +147,7 @@ struct chv_community {
>  	size_t nfunctions;
>  	const struct chv_gpio_pinrange *gpio_ranges;
>  	size_t ngpio_ranges;
> +	size_t ngpios;
>  	size_t nirqs;
>  	acpi_adr_space_type acpi_space_id;
>  };
> @@ -396,6 +399,7 @@ static const struct chv_community southwest_community = {
>  	.nfunctions = ARRAY_SIZE(southwest_functions),
>  	.gpio_ranges = southwest_gpio_ranges,
>  	.ngpio_ranges = ARRAY_SIZE(southwest_gpio_ranges),
> +	.ngpios = ARRAY_SIZE(southwest_pins),
>  	/*
>  	 * Southwest community can benerate GPIO interrupts only for the
>  	 * first 8 interrupts. The upper half (8-15) can only be used to
> @@ -485,6 +489,7 @@ static const struct chv_community north_community = {
>  	.npins = ARRAY_SIZE(north_pins),
>  	.gpio_ranges = north_gpio_ranges,
>  	.ngpio_ranges = ARRAY_SIZE(north_gpio_ranges),
> +	.ngpios = ARRAY_SIZE(north_pins),
>  	/*
>  	 * North community can generate GPIO interrupts only for the first
>  	 * 8 interrupts. The upper half (8-15) can only be used to trigger
> @@ -533,6 +538,7 @@ static const struct chv_community east_community = {
>  	.npins = ARRAY_SIZE(east_pins),
>  	.gpio_ranges = east_gpio_ranges,
>  	.ngpio_ranges = ARRAY_SIZE(east_gpio_ranges),
> +	.ngpios = ARRAY_SIZE(east_pins),
>  	.nirqs = 16,
>  	.acpi_space_id = 0x93,
>  };
> @@ -659,6 +665,7 @@ static const struct chv_community southeast_community = {
>  	.nfunctions = ARRAY_SIZE(southeast_functions),
>  	.gpio_ranges = southeast_gpio_ranges,
>  	.ngpio_ranges = ARRAY_SIZE(southeast_gpio_ranges),
> +	.ngpios = ARRAY_SIZE(southeast_pins),
>  	.nirqs = 16,
>  	.acpi_space_id = 0x94,
>  };
> @@ -1246,14 +1253,21 @@ static struct pinctrl_desc chv_pinctrl_desc = {
>  	.owner = THIS_MODULE,
>  };
>  
> +static unsigned chv_gpio_offset_to_pin(struct chv_pinctrl *pctrl,
> +				       unsigned offset)
> +{
> +	return pctrl->community->pins[offset].number;
> +}
> +
>  static int chv_gpio_get(struct gpio_chip *chip, unsigned offset)
>  {
>  	struct chv_pinctrl *pctrl = gpiochip_get_data(chip);
> +	int pin = chv_gpio_offset_to_pin(pctrl, offset);
>  	unsigned long flags;
>  	u32 ctrl0, cfg;
>  
>  	raw_spin_lock_irqsave(&chv_lock, flags);
> -	ctrl0 = readl(chv_padreg(pctrl, offset, CHV_PADCTRL0));
> +	ctrl0 = readl(chv_padreg(pctrl, pin, CHV_PADCTRL0));
>  	raw_spin_unlock_irqrestore(&chv_lock, flags);
>  
>  	cfg = ctrl0 & CHV_PADCTRL0_GPIOCFG_MASK;
> @@ -1267,13 +1281,14 @@ static int chv_gpio_get(struct gpio_chip *chip, unsigned offset)
>  static void chv_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
>  {
>  	struct chv_pinctrl *pctrl = gpiochip_get_data(chip);
> +	unsigned pin = chv_gpio_offset_to_pin(pctrl, offset);
>  	unsigned long flags;
>  	void __iomem *reg;
>  	u32 ctrl0;
>  
>  	raw_spin_lock_irqsave(&chv_lock, flags);
>  
> -	reg = chv_padreg(pctrl, offset, CHV_PADCTRL0);
> +	reg = chv_padreg(pctrl, pin, CHV_PADCTRL0);
>  	ctrl0 = readl(reg);
>  
>  	if (value)
> @@ -1289,11 +1304,12 @@ static void chv_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
>  static int chv_gpio_get_direction(struct gpio_chip *chip, unsigned offset)
>  {
>  	struct chv_pinctrl *pctrl = gpiochip_get_data(chip);
> +	unsigned pin = chv_gpio_offset_to_pin(pctrl, offset);
>  	u32 ctrl0, direction;
>  	unsigned long flags;
>  
>  	raw_spin_lock_irqsave(&chv_lock, flags);
> -	ctrl0 = readl(chv_padreg(pctrl, offset, CHV_PADCTRL0));
> +	ctrl0 = readl(chv_padreg(pctrl, pin, CHV_PADCTRL0));
>  	raw_spin_unlock_irqrestore(&chv_lock, flags);
>  
>  	direction = ctrl0 & CHV_PADCTRL0_GPIOCFG_MASK;
> @@ -1329,7 +1345,7 @@ static void chv_gpio_irq_ack(struct irq_data *d)
>  {
>  	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>  	struct chv_pinctrl *pctrl = gpiochip_get_data(gc);
> -	int pin = irqd_to_hwirq(d);
> +	int pin = chv_gpio_offset_to_pin(pctrl, irqd_to_hwirq(d));
>  	u32 intr_line;
>  
>  	raw_spin_lock(&chv_lock);
> @@ -1346,7 +1362,7 @@ 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_get_data(gc);
> -	int pin = irqd_to_hwirq(d);
> +	int pin = chv_gpio_offset_to_pin(pctrl, irqd_to_hwirq(d));
>  	u32 value, intr_line;
>  	unsigned long flags;
>  
> @@ -1391,7 +1407,8 @@ static unsigned chv_gpio_irq_startup(struct irq_data *d)
>  	if (irqd_get_trigger_type(d) == IRQ_TYPE_NONE) {
>  		struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>  		struct chv_pinctrl *pctrl = gpiochip_get_data(gc);
> -		unsigned pin = irqd_to_hwirq(d);
> +		unsigned offset = irqd_to_hwirq(d);
> +		int pin = chv_gpio_offset_to_pin(pctrl, offset);
>  		irq_flow_handler_t handler;
>  		unsigned long flags;
>  		u32 intsel, value;
> @@ -1409,7 +1426,7 @@ static unsigned chv_gpio_irq_startup(struct irq_data *d)
>  
>  		if (!pctrl->intr_lines[intsel]) {
>  			irq_set_handler_locked(d, handler);
> -			pctrl->intr_lines[intsel] = pin;
> +			pctrl->intr_lines[intsel] = offset;
>  		}
>  		raw_spin_unlock_irqrestore(&chv_lock, flags);
>  	}
> @@ -1422,7 +1439,8 @@ static int chv_gpio_irq_type(struct irq_data *d, unsigned type)
>  {
>  	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>  	struct chv_pinctrl *pctrl = gpiochip_get_data(gc);
> -	unsigned pin = irqd_to_hwirq(d);
> +	unsigned offset = irqd_to_hwirq(d);
> +	int pin = chv_gpio_offset_to_pin(pctrl, offset);
>  	unsigned long flags;
>  	u32 value;
>  
> @@ -1468,7 +1486,7 @@ static int chv_gpio_irq_type(struct irq_data *d, unsigned type)
>  	value &= CHV_PADCTRL0_INTSEL_MASK;
>  	value >>= CHV_PADCTRL0_INTSEL_SHIFT;
>  
> -	pctrl->intr_lines[value] = pin;
> +	pctrl->intr_lines[value] = offset;
>  
>  	if (type & IRQ_TYPE_EDGE_BOTH)
>  		irq_set_handler_locked(d, handle_edge_irq);
> @@ -1558,12 +1576,12 @@ static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq)
>  	const struct chv_gpio_pinrange *range;
>  	struct gpio_chip *chip = &pctrl->chip;
>  	bool need_valid_mask = !dmi_check_system(chv_no_valid_mask);
> -	const struct chv_community *community = pctrl->community;
> -	int ret, i, irq_base;
> +	int ret, i, offset;
> +	int irq_base;
>  
>  	*chip = chv_gpio_chip;
>  
> -	chip->ngpio = community->pins[community->npins - 1].number + 1;
> +	chip->ngpio = pctrl->community->ngpios;
>  	chip->label = dev_name(pctrl->dev);
>  	chip->parent = pctrl->dev;
>  	chip->base = -1;
> @@ -1575,29 +1593,30 @@ static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq)
>  		return ret;
>  	}
>  
> -	for (i = 0; i < community->ngpio_ranges; i++) {
> -		range = &community->gpio_ranges[i];
> -		ret = gpiochip_add_pin_range(chip, dev_name(pctrl->dev),
> -					     range->base, range->base,
> -					     range->npins);
> +	for (i = 0, offset = 0; i < pctrl->community->ngpio_ranges; i++) {
> +		range = &pctrl->community->gpio_ranges[i];
> +		ret = gpiochip_add_pin_range(chip, dev_name(pctrl->dev), offset,
> +					     range->base, range->npins);
>  		if (ret) {
>  			dev_err(pctrl->dev, "failed to add GPIO pin range\n");
>  			return ret;
>  		}
> +
> +		offset += range->npins;
>  	}
>  
>  	/* Do not add GPIOs that can only generate GPEs to the IRQ domain */
> -	for (i = 0; i < community->npins; i++) {
> +	for (i = 0; i < pctrl->community->npins; i++) {
>  		const struct pinctrl_pin_desc *desc;
>  		u32 intsel;
>  
> -		desc = &community->pins[i];
> +		desc = &pctrl->community->pins[i];
>  
>  		intsel = readl(chv_padreg(pctrl, desc->number, CHV_PADCTRL0));
>  		intsel &= CHV_PADCTRL0_INTSEL_MASK;
>  		intsel >>= CHV_PADCTRL0_INTSEL_SHIFT;
>  
> -		if (need_valid_mask && intsel >= community->nirqs)
> +		if (need_valid_mask && intsel >= pctrl->community->nirqs)
>  			clear_bit(i, chip->irq.valid_mask);
>  	}
>  
> -- 
> 2.17.0
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html