Re: [PATCH v6 00/12] gpio: Tight IRQ chip integration

[Thierry Reding]

On Fri, Nov 03, 2017 at 05:30:30PM -0500, Grygorii Strashko wrote:
> Hi 
> 
> On 11/02/2017 12:49 PM, Thierry Reding wrote:
> > From: Thierry Reding <treding@xxxxxxxxxx>
> > 
> > Hi Linus,
> > 
> > here's the latest series of patches that implement the tighter IRQ chip
> > integration. I've dropped the banked infrastructure for now as per the
> > discussion with Grygorii.
> > 
> > The first couple of patches are mostly preparatory work in order to
> > consolidate all IRQ chip related fields in a new structure and create
> > the base functionality for adding IRQ chips.
> > 
> > After that, I've added the Tegra186 GPIO support patch that makes use of
> > the new tight integration.
> > 
> > Changes in v6:
> > - rebased on latest linux-gpio devel branch
> > - one patch dropped due to rebase
> > 
> > Changes in v5:
> > - dropped the banked infrastructure patches for now (Grygorii)
> > - allocate interrupts on demand, rather than upfront (Grygorii)
> > - split up the first patch further as requested by Grygorii
> > 
> > Not sure what happened in between here. Notes in commit logs indicate
> > that this is actually version 5, but I can't find the cover letter for
> > v3 and v4.
> > 
> > Changes in v2:
> > - rename pins to lines for consistent terminology
> > - rename gpio_irq_chip_banked_handler() to
> >    gpio_irq_chip_banked_chained_handler()
> > 
> 
> Sry, for delayed reply, very sorry.
> 
> Patches 1 - 9, 11 : looks good, so
> Acked-by: Grygorii Strashko <grygorii.strashko@xxxxxx>
> 
> Patch 10 - unfortunately not all my comment were incorporated [1], 
> so below diff on top of patch 10
> which illustrates what i want and also converts gpio-omap to use new infra as
> test for this new infra.
> 
> Pls, take a look
> 
> [1] https://www.spinics.net/lists/linux-tegra/msg31145.html

Most of the changes I had deemed to be material for follow-up patches
since they aren't immediately relevant to the gpio_irq_chip conversion
nor needed by the Tegra186 patch.

However, a couple of the hunks below seem like they should be part of
the initial series.

> -----------><-------------
> From 210fe3ad7a642691a1b7603e441f6980b10ff2b4 Mon Sep 17 00:00:00 2001
> From: Grygorii Strashko <grygorii.strashko@xxxxxx>
> Date: Fri, 3 Nov 2017 17:24:51 -0500
> Subject: [PATCH] [not for merge] gpiolib: update and test new gpioirq chip
>  infra
> 
> changes in gpiolib:
>  - move set_parent to gpiochip_irq_map() and use parents instead of map for only
>    one parent
>  - add gpio_irq_chip->first_irq for static IRQ allocation
>  - fix nested = true if parent_handler not set
>  - fix gpiochip_irqchip_remove() if parent_handler not set
>  - get of_node from gpiodev
>  - fix lock_key: drivers do not need to set it, but looks a bit overcomplicated
>    as lock_class_key will be created for each gpiochip_add_data() call.
>    Honestly, do not seem better way :(
> 
> GPIO OMAP driver updated to use new gpioirq chip infra and tested on am335x-evm.
> seems it's working - can see irqs from keys.
> 
> Signed-off-by: Grygorii Strashko <grygorii.strashko@xxxxxx>
> ---
>  drivers/gpio/gpio-omap.c    | 36 ++++++++++++++--------------
>  drivers/gpio/gpiolib.c      | 58 +++++++++++++++++++--------------------------
>  include/linux/gpio/driver.h | 32 ++++++++++++++++++++++++-
>  3 files changed, 73 insertions(+), 53 deletions(-)
[...]
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
[...]
> @@ -1642,6 +1644,7 @@ static int gpiochip_irq_map(struct irq_domain *d, unsigned int irq,
>  			    irq_hw_number_t hwirq)
>  {
>  	struct gpio_chip *chip = d->host_data;
> +	int err = 0;
>  
>  	if (!gpiochip_irqchip_irq_valid(chip, hwirq))
>  		return -ENXIO;
> @@ -1658,6 +1661,12 @@ static int gpiochip_irq_map(struct irq_domain *d, unsigned int irq,
>  		irq_set_nested_thread(irq, 1);
>  	irq_set_noprobe(irq);
>  
> +	if (chip->irq.num_parents == 1)
> +		err = irq_set_parent(irq, chip->irq.parents[0]);
> +	else if (chip->irq.map)
> +		err = irq_set_parent(irq, chip->irq.map[hwirq]);
> +	if (err < 0)
> +		return err;

Yeah, this looks sensible. Both in that this is a slightly better place
to call it and that the handling for num_parents == 1 is necessary, too.

>  	/*
>  	 * No set-up of the hardware will happen if IRQ_TYPE_NONE
>  	 * is passed as default type.
> @@ -1712,30 +1721,18 @@ static void gpiochip_irq_relres(struct irq_data *d)
>  
>  static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset)
>  {
> -	unsigned int irq;
> -	int err;
> -
>  	if (!gpiochip_irqchip_irq_valid(chip, offset))
>  		return -ENXIO;
>  
> -	irq = irq_create_mapping(chip->irq.domain, offset);
> -	if (!irq)
> -		return 0;
> -
> -	if (chip->irq.map) {
> -		err = irq_set_parent(irq, chip->irq.map[offset]);
> -		if (err < 0)
> -			return err;
> -	}
> -
> -	return irq;
> +	return irq_create_mapping(chip->irq.domain, offset);
>  }
>  
>  /**
>   * gpiochip_add_irqchip() - adds an IRQ chip to a GPIO chip
>   * @gpiochip: the GPIO chip to add the IRQ chip to
>   */
> -static int gpiochip_add_irqchip(struct gpio_chip *gpiochip)
> +static int gpiochip_add_irqchip_key(struct gpio_chip *gpiochip,
> +				    struct lock_class_key *lock_key)
>  {
>  	struct irq_chip *irqchip = gpiochip->irq.chip;
>  	const struct irq_domain_ops *ops;
> @@ -1753,17 +1750,8 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip)
>  	}
>  
>  	type = gpiochip->irq.default_type;
> -	np = gpiochip->parent->of_node;
> -
> -#ifdef CONFIG_OF_GPIO
> -	/*
> -	 * If the gpiochip has an assigned OF node this takes precedence
> -	 * FIXME: get rid of this and use gpiochip->parent->of_node
> -	 * everywhere
> -	 */
> -	if (gpiochip->of_node)
> -		np = gpiochip->of_node;
> -#endif
> +	/* called from gpiochip_add_data() and is set properly */
> +	np = gpiochip->gpiodev->dev.of_node;

Indeed, looks like we have this twice now.

>  
>  	/*
>  	 * Specifying a default trigger is a terrible idea if DT or ACPI is
> @@ -1789,7 +1777,8 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip)
>  		ops = &gpiochip_domain_ops;
>  
>  	gpiochip->irq.domain = irq_domain_add_simple(np, gpiochip->ngpio,
> -						     0, ops, gpiochip);
> +						     gpiochip->irq.first_irq,
> +						     ops, gpiochip);
>  	if (!gpiochip->irq.domain)
>  		return -EINVAL;

This seems backward. You just spent a lot of time getting rid of all
users of first_irq (it's actually the reason why I dropped one of the
patches from the series, since first_irq is no longer used), so why
reintroduce it?

>  
> @@ -1818,9 +1807,9 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip)
>  		}
>  
>  		gpiochip->irq.nested = false;
> -	} else {
> -		gpiochip->irq.nested = true;
>  	}
> +	/* GPIO driver might not specify parent_handler, but it doesn't mean
> +	 * it's irq_nested, as driver may use request_irq() */

That's certainly how irq_nested is used in gpiolib, though. Interrupts
are considered either cascaded or nested. Maybe this needs clarification
in general?

>  
>  	acpi_gpiochip_request_interrupts(gpiochip);
>  
> @@ -1839,7 +1828,7 @@ static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip)
>  
>  	acpi_gpiochip_free_interrupts(gpiochip);
>  
> -	if (gpiochip->irq.chip) {
> +	if (gpiochip->irq.chip && gpiochip->irq.parent_handler) {
>  		struct gpio_irq_chip *irq = &gpiochip->irq;
>  		unsigned int i;
>  

Yeah, this seems like a good idea, though I think this is safe
regardless of irq.parent_handler.

> @@ -1972,7 +1961,8 @@ EXPORT_SYMBOL_GPL(gpiochip_irqchip_add_key);
>  
>  #else /* CONFIG_GPIOLIB_IRQCHIP */
>  
> -static inline int gpiochip_add_irqchip(struct gpio_chip *gpiochip)
> +static inline int gpiochip_add_irqchip_key(struct gpio_chip *gpiochip,
> +					   struct lock_class_key *lock_key)
>  {
>  	return 0;
>  }
> diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
> index 51fc7b0..3254996 100644
> --- a/include/linux/gpio/driver.h
> +++ b/include/linux/gpio/driver.h
> @@ -128,6 +128,15 @@ struct gpio_irq_chip {
>  	 * in IRQ domain of the chip.
>  	 */
>  	unsigned long *valid_mask;
> +
> +	/**
> +	 * @first_irq:
> +	 *
> +	 * Required for static irq allocation.
> +	 * if set irq_domain_add_simple() will allocate and map all IRQs
> +	 * during initialization.
> +	 */
> +	unsigned int first_irq;

Again, what was the point of removing all users of this if we need to
add it again?

It seems to me like dynamic allocation should be a prerequisite for
drivers to use this new code, otherwise we'll just end up adding special
cases to this otherwise generic code.

>  };
>  
>  static inline struct gpio_irq_chip *to_gpio_irq_chip(struct irq_chip *chip)
> @@ -312,8 +321,29 @@ struct gpio_chip {
>  extern const char *gpiochip_is_requested(struct gpio_chip *chip,
>  			unsigned offset);
>  
> +extern int gpiochip_add_data_key(struct gpio_chip *chip, void *data,
> +				 struct lock_class_key *irq_lock_key);
> +#ifdef CONFIG_LOCKDEP
> +/*
> + * Lockdep requires that each irqchip instance be created with a
> + * unique key so as to avoid unnecessary warnings. This upfront
> + * boilerplate static inlines provides such a key for each
> + * unique instance which is created now from inside gpiochip_add_data_key().
> + */
> +static inline int gpiochip_add_data(struct gpio_chip *chip, void *data)
> +{
> +	static struct lock_class_key key;
> +
> +	return gpiochip_add_data_key(chip, data, key);
> +}

This looks like a neat improvement, but I think it can be done in a
follow-up to remove the boilerplate in drivers.

Thierry

Attachment: signature.asc
Description: PGP signature