Re: [PATCH 2/2] Revert "pinctrl: intel: Do pin translation when lock IRQ"

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

On Mon, Sep 24, 2018 at 05:32:12PM +0300, Mika Westerberg wrote:
> This reverts commit 55aedef50d4d810670916d9fce4a40d5da2079e7.
> 
> Commit 55aedef50d4d ("pinctrl: intel: Do pin translation when lock IRQ")
> added special translation from GPIO number to hardware pin number to
> irq_reqres/relres hooks to avoid failure when IRQs are requested. The
> actual failure happened inside gpiochip_lock_as_irq() because it calls
> gpiod_get_direction() and pinctrl-intel.c::intel_gpio_get_direction()
> implementation originally missed the translation so the two hooks made
> it work by skipping the ->get_direction() call entirely (it overwrote
> the default GPIOLIB provided functions).
> 
> The proper fix that adds translation to GPIO callbacks was merged with
> commit 96147db1e1df ("pinctrl: intel: Do pin translation in other GPIO
> operations as well"). This allows us to use the default GPIOLIB provided
> functions again.
> 
> In addition as find out by Benjamin Tissoires the two functions
> (intel_gpio_irq_reqres()/intel_gpio_irq_relres()) now cause problems of
> their own because they operate on pin numbers and pass that pin number
> to gpiochip_lock_as_irq() which actually expects a GPIO number.
> 

Since I was participating in the investigation,

Acked-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>

> Link: https://bugzilla.kernel.org/show_bug.cgi?id=199911
> Fixes: 55aedef50d4d ("pinctrl: intel: Do pin translation when lock IRQ")
> Reported-and-tested-by: Benjamin Tissoires <benjamin.tissoires@xxxxxxxxx>
> Signed-off-by: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
> ---
>  drivers/pinctrl/intel/pinctrl-intel.c | 32 ---------------------------
>  1 file changed, 32 deletions(-)
> 
> diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
> index f848db65169b..692c2bd1faed 100644
> --- a/drivers/pinctrl/intel/pinctrl-intel.c
> +++ b/drivers/pinctrl/intel/pinctrl-intel.c
> @@ -890,36 +890,6 @@ static const struct gpio_chip intel_gpio_chip = {
>  	.set_config = gpiochip_generic_config,
>  };
>  
> -static int intel_gpio_irq_reqres(struct irq_data *d)
> -{
> -	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> -	struct intel_pinctrl *pctrl = gpiochip_get_data(gc);
> -	int pin;
> -	int ret;
> -
> -	pin = intel_gpio_to_pin(pctrl, irqd_to_hwirq(d), NULL, NULL);
> -	if (pin >= 0) {
> -		ret = gpiochip_lock_as_irq(gc, pin);
> -		if (ret) {
> -			dev_err(pctrl->dev, "unable to lock HW IRQ %d for IRQ\n",
> -				pin);
> -			return ret;
> -		}
> -	}
> -	return 0;
> -}
> -
> -static void intel_gpio_irq_relres(struct irq_data *d)
> -{
> -	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> -	struct intel_pinctrl *pctrl = gpiochip_get_data(gc);
> -	int pin;
> -
> -	pin = intel_gpio_to_pin(pctrl, irqd_to_hwirq(d), NULL, NULL);
> -	if (pin >= 0)
> -		gpiochip_unlock_as_irq(gc, pin);
> -}
> -
>  static void intel_gpio_irq_ack(struct irq_data *d)
>  {
>  	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> @@ -1135,8 +1105,6 @@ static irqreturn_t intel_gpio_irq(int irq, void *data)
>  
>  static struct irq_chip intel_gpio_irqchip = {
>  	.name = "intel-gpio",
> -	.irq_request_resources = intel_gpio_irq_reqres,
> -	.irq_release_resources = intel_gpio_irq_relres,
>  	.irq_enable = intel_gpio_irq_enable,
>  	.irq_ack = intel_gpio_irq_ack,
>  	.irq_mask = intel_gpio_irq_mask,
> -- 
> 2.18.0
> 

-- 
With Best Regards,
Andy Shevchenko