Re: [PATCH] pinctrl: intel: Pass irqchip when adding gpiochip

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

On Sun, Oct 06, 2019 at 02:59:49AM +0200, Linus Walleij wrote:
> We need to convert all old gpio irqchips to pass the irqchip
> setup along when adding the gpio_chip. For more info see
> drivers/gpio/TODO.
> 
> This is an unchained irqchip so we use the method from
> drivers/gpio/gpio-mt7621.c that also requests its interrupt
> instead if chaining the interrupt handler.

One comment below, after addressing,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>

> 
> Cc: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
> Cc: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> ---
> This is based on Andy's patch to dynamically allocate the
> irqchio. Mika maybe you want to queue it with the rest of
> the stuff so you can sync fixes and new development?

My Rb tag Mika may use for fixes stuff in this cycle.

> ---
>  drivers/pinctrl/intel/pinctrl-intel.c | 52 ++++++++++++++++-----------
>  1 file changed, 31 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
> index bc013599a9a3..d63566e57f4c 100644
> --- a/drivers/pinctrl/intel/pinctrl-intel.c
> +++ b/drivers/pinctrl/intel/pinctrl-intel.c
> @@ -1206,6 +1206,37 @@ static int intel_gpio_probe(struct intel_pinctrl *pctrl, int irq)
>  	pctrl->irqchip.irq_set_wake = intel_gpio_irq_wake;
>  	pctrl->irqchip.flags = IRQCHIP_MASK_ON_SUSPEND;
>  
> +	/*
> +	 * We need to request the interrupt here (instead of using a
> +	 * chained handler) because on some platforms several GPIO
> +	 * controllers share the same interrupt line.
> +	 */
> +	ret = devm_request_irq(pctrl->dev, irq, intel_gpio_irq,
> +			       IRQF_SHARED | IRQF_NO_THREAD,
> +			       dev_name(pctrl->dev), pctrl);

> +	if (!ret) {

Why not positive condition?

> +		struct gpio_irq_chip *girq;
> +
> +		girq = &pctrl->chip.irq;
> +		girq->chip = &pctrl->irqchip;
> +		/*
> +		 * This is an unchained interrupt. Compare to
> +		 * drivers/gpio/gpio-mt7621.c that also does this:
> +		 * assign no parents.
> +		 *
> +		 * FIXME: make the gpiolib flag this and handle unchained
> +		 * GPIO interrupts better if need be.
> +		 */
> +		girq->parent_handler = NULL;
> +		girq->num_parents = 0;
> +		girq->parents = NULL;
> +		girq->default_type = IRQ_TYPE_NONE;
> +		girq->handler = handle_bad_irq;
> +	} else {
> +		/* Skip irqchip, register gpiochip anyway */
> +		dev_err(pctrl->dev, "failed to request interrupt\n");
> +	}
> +
>  	ret = devm_gpiochip_add_data(pctrl->dev, &pctrl->chip, pctrl);
>  	if (ret) {
>  		dev_err(pctrl->dev, "failed to register gpiochip\n");
> @@ -1222,27 +1253,6 @@ static int intel_gpio_probe(struct intel_pinctrl *pctrl, int irq)
>  		}
>  	}
>  
> -	/*
> -	 * We need to request the interrupt here (instead of providing chip
> -	 * to the irq directly) because on some platforms several GPIO
> -	 * controllers share the same interrupt line.
> -	 */
> -	ret = devm_request_irq(pctrl->dev, irq, intel_gpio_irq,
> -			       IRQF_SHARED | IRQF_NO_THREAD,
> -			       dev_name(pctrl->dev), pctrl);
> -	if (ret) {
> -		dev_err(pctrl->dev, "failed to request interrupt\n");
> -		return ret;
> -	}
> -
> -	ret = gpiochip_irqchip_add(&pctrl->chip, &pctrl->irqchip, 0,
> -				   handle_bad_irq, IRQ_TYPE_NONE);
> -	if (ret) {
> -		dev_err(pctrl->dev, "failed to add irqchip\n");
> -		return ret;
> -	}
> -
> -	gpiochip_set_chained_irqchip(&pctrl->chip, &pctrl->irqchip, irq, NULL);
>  	return 0;
>  }
>  
> -- 
> 2.21.0
> 

-- 
With Best Regards,
Andy Shevchenko