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

[Thierry Reding]

On Tue, Nov 07, 2017 at 11:00:41AM -0600, Grygorii Strashko wrote:
> 
> 
> On 11/07/2017 05:13 AM, Thierry Reding wrote:
> > On Mon, Nov 06, 2017 at 05:13:33PM -0600, Grygorii Strashko wrote:
> > > 
> > > 
> > > On 11/06/2017 05:18 AM, Thierry Reding wrote:
> > > > 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?
> > > 
> > > Yes. It required for HW/drivers not supported (or partially supported)
> > > SPARSE_IRQ. And it's not exactly the same as dropped base_irq.
> > > If SPARSE_IRQ not supported - driver should ensure that proper
> > >   Linux IRQ range allocated (or reserved) and pass first_irq number to the gpiolib
> > > For example, in case of OMAP GPIO this is required for OMAP1 support and
> > > driver has code
> > >    irq_base = devm_irq_alloc_descs(bank->chip.parent, -1, 0, bank->width, 0);
> > > 
> > > irq_base makes no sense in case of SPARSE_IRQ compatible driver and
> > > therefore was removed from gpiolib to avoid mis-usage - drivers should
> > > maintain it by itself if requred.
> > 
> > But this is not something that is currently being used. I understand
> > that you'll want to add this in order to support fixed IRQ numbers on
> > OMAP, but I don't see why it would need to be part of this series. It
> > will simply be dead code until the OMAP patches get merged.
> 
> My opinion is that if this modification will be merged in gpiolib - drivers
> should be able to use it out of the box.
> 
> Any way finial decision is up to Linus.

Alright, I've sent v7 that should address all of the remaining issues.
I've squashed a couple of the fixes you had pointed out into the tight
integration patch and added another three patches on top for the first
IRQ support, nested vs. threaded disambiguation and one for the
automatic lock dep key creation.

Thierry

Attachment: signature.asc
Description: PGP signature