Cc:ing Neil Brown who might hit me on the fingers for some statements here, let's see :) On Tue, Dec 18, 2018 at 11:06 PM Thierry Reding <thierry.reding@xxxxxxxxx> wrote: > On Fri, Dec 14, 2018 at 02:41:01PM +0100, Linus Walleij wrote: > > > - gpiochip->to_irq = gpiochip_to_irq; > > > + /* > > > + * Allow GPIO chips to override the ->to_irq() if they really need to. > > > + * This should only be very rarely needed, the majority should be fine > > > + * with gpiochip_to_irq(). > > > + */ > > > + if (!gpiochip->to_irq) > > > + gpiochip->to_irq = gpiochip_to_irq; > > > > And I see this is what your driver does, which leaves the default > > domain hierarchy callback path unused. > > Actually this is only temporary until the patch that uses a sparse > number space (with the valid masks). After the last patch in the series > the need to override this goes away, so I could follow-up with a patch > to revert this. Or alternatively we could squash the two Tegra patches. > I think keeping them separate is slightly nicer. OK then! > Of course, I would even prefer to not move to the sparse number space, > but you seemed to feel very strongly about it last time we discussed > this. Admittedly it is one of those things where I am working on intuition and as such it may be wrong. The main reason in this case is to keep interfaces simple. Of course "simple" is a bit subjective. But keeping to a certain predictable pattern makes things easier to understand as long as the pattern is somewhat reasonable. This pattern I think it is related to the irqdomains: in the irqdomains a linear irqdomain with dynamically allocated IRQ descriptors from a sparse number space is clearly preferred. By similarity and the fact that we are mapping GPIOs to IRQs here, I think it is more intuitive and less confusing to use a linear GPIO offset range and dynamically allocated GPIO descriptors. Today we do not allocate GPIO descriptors dynamically, they are allocated as part of the gpiochip struct. But I do see it as an end goal to allocate them dynamically or at least not allocate GPIO descriptors for the "holes" in the .valid_mask. > > It's better if you indirect the pointer like we do with some other > > irqchip stuff (see Hans Verkuils patches), i.e. if ->to_irq > > is defined, we save that function pointer and call that at the > > end of the gpiochip_to_irq() pointer, then we override it > > with our own. > > > > Since the Tegra186 clearly .to_irq() clearly is mostly the > > same plus some extra, this should work fine and give > > lesser lines of code in that driver. > > That sounds an awful lot like a midlayer. I'm not a big fan of that at > all. There are various reasons for this, but others have described it in > much better detail than I could. See here for example: > > https://lwn.net/Articles/336262/ Reading that article I do not think gpiolib qualifies as midlayer, since it is after all optional and not every driver uses it, so it is available on a case-by-case basis. The article even says: "That common functionality which it is so tempting to put in a midlayer should instead be provided as library routines which can used, augmented, or ignored by each bottom level driver independently." Contrast that by the SCSI emulation in libata: is it even possible to bypass? Not really, no. gpiolib is not like that. > In this particular case one potential problem is that gpiochip_to_irq() > might not always do the right things for everyone, and that it turn may > incentivize people to work around it creatively. For example, one driver > may want to perform some operation before gpiochip_to_irq() is called, > while another driver may have to perform an operation after the call. If > you move towards a midlayer there's no way to satisfy both, unless you > go and add pre- and post-operations of some sort. I don't really buy into that. gpiochip_to_irq() is *ONLY* a translation mechanism. The main abstraction is and should be irqchip. In usecases like device tree the same device exposes a gpiochip API and an irqchip API and those two are supposed to be orthogonal. The Documentation/driver-api/gpio/driver.rst says: --------8<----------8<--------8<--------8<-------- It is legal for any IRQ consumer to request an IRQ from any irqchip no matter if that is a combined GPIO+IRQ driver. The basic premise is that gpio_chip and irq_chip are orthogonal, and offering their services independent of each other. gpiod_to_irq() is just a convenience function to figure out the IRQ for a certain GPIO line and should not be relied upon to have been called before the IRQ is used. So always prepare the hardware and make it ready for action in respective callbacks from the GPIO and irqchip APIs. Do not rely on gpiod_to_irq() having been called first. This orthogonality leads to ambiguities that we need to solve: if there is competition inside the subsystem which side is using the resource (a certain GPIO line and register for example) it needs to deny certain operations and keep track of usage inside of the gpiolib subsystem. This is why the API below exists. --------8<----------8<--------8<--------8<-------- So now indeed it seems that your implementation actually has a bug here. I didn't see it before: if we make use of the irq off the irqchip without gpiochip_to_irq() having been called before, will the (hierarchical) IRQ still work? Much of the idea and reason behind GPIOLIB_IRQCHIP is to make sure this can hold true. So this hunk of your original patch: + if (irq_domain_is_hierarchy(domain)) { + struct irq_fwspec spec; + + spec.fwnode = domain->fwnode; + spec.param_count = 2; + spec.param[0] = offset; + spec.param[1] = 0; + + return irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE, &spec); + } Is probably misplaced: gpiochip_to_irq() should really only contain irq_create_mapping() which is what will also be done by the device tree (or ACPI, I hope) core when providing an irq as a resource. The snippet above needs to go somewhere else, if it needs to happen before any of the irqchip callbacks then just loop over all the IRQs in gpiochip_add_irqchip() and allocate IRQs for all valid IRQs from the start as a last resort I suppose. A last alternative would be to assume that all other ways an IRQ is mapped (besides using gpiochip_to_irq()) will also do the same thing. E.g, you would have to go into drivers/of/irq.c and patch irq_of_parse_and_map() to do the same thing. This is the site that creates the mapping when .to_irq() is not called in the DT case. Maybe the DT or irqchip maintainers can give a better answer to how this should happen. Relying on .to_irq() to always be called is certainly not the answer. We already did that mistake in the past :( > I'd like to propose that we instead provide gpiochip_to_irq() as a > helper that drivers can call into. > static int foo_gpio_to_irq(struct gpio_chip *chip, unsigned int offset) > { > /* do something before */ > ... > > err = gpiochip_to_irq(chip, offset); > if (err < 0) > return err; > > /* do something after */ > ... > } Under your assumptions that is fine, and the same reason as to why we control pins and regulators and clock in callbacks from say .suspend() instead of enforcing a certain order. It could be necessary if the translation gets complicated enough. But right now I am mostly worried that this implementation does not cover for the fact that we have to make sure that hierarchical irqchips in gpiolib work even if .to_irq() was not called first. Yours, Linus Walleij
