| Message ID | 20220703111057.23246-4-aidanmacdonald.0x0@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpio-regmap support for register fields and other hooks | expand |
On Sun, Jul 3, 2022 at 1:11 PM Aidan MacDonald <aidanmacdonald.0x0@gmail.com> wrote: > > Some GPIO chips require a custom to_irq() callback for mapping > their IRQs, eg. because their interrupts come from a parent IRQ > chip where the GPIO offset doesn't map 1-to-1 with hwirq number. Don't they follow a hierarchical IRQ domain in that case? And to be honest after the commit ef38237444ce ("gpiolib: add a warning on gpiochip->to_irq defined") I have no idea how it works in your case and also I feel this patch is a wrong direction of development.
Andy Shevchenko <andy.shevchenko@gmail.com> writes: > On Sun, Jul 3, 2022 at 1:11 PM Aidan MacDonald > <aidanmacdonald.0x0@gmail.com> wrote: >> >> Some GPIO chips require a custom to_irq() callback for mapping >> their IRQs, eg. because their interrupts come from a parent IRQ >> chip where the GPIO offset doesn't map 1-to-1 with hwirq number. > > Don't they follow a hierarchical IRQ domain in that case? > > And to be honest after the commit ef38237444ce ("gpiolib: add a > warning on gpiochip->to_irq defined") I have no idea how it works in > your case and also I feel this patch is a wrong direction of > development. My own use case is an MFD device with a shared IRQ chip that is used by other sub-drivers. This is a very common case that seems to map onto ->to_irq() cleanly. Do we really need an IRQ domain? What you're suggesting would be a 1-to-1 mapping from GPIO offset to hwirq number in a virtual domain, then remapping to the real hwirq number, which seems unnecessarily complicated when we can just change the GPIO offset -> hwirq mapping. The commit you mentioned is warning users of GPIOLIB_IRQCHIP when a custom ->to_irq() method is overridden. That's not relevant here. Using an IRQ domain also overrides ->to_irq() so I included a check in this patch to ensure gpio-regmap chips are well-behaved.
On Sun, Jul 3, 2022 at 1:10 PM Aidan MacDonald <aidanmacdonald.0x0@gmail.com> wrote: > Some GPIO chips require a custom to_irq() callback for mapping > their IRQs, eg. because their interrupts come from a parent IRQ > chip where the GPIO offset doesn't map 1-to-1 with hwirq number. > > Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com> What is the usecase for this? Since GPIO chips and IRQ chips are orthogonal there is absolutely no guarantee that ->to_irq() is called before a driver start to use an IRQ from the irqchip side: (quoting Documentation/driver-api/gpio/driver.rst) It is legal for any IRQ consumer to request an IRQ from any irqchip even if it 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. Always prepare the hardware and make it ready for action in respective callbacks from the GPIO and irq_chip APIs. Do not rely on gpiod_to_irq() having been called first. (end quote) Using ->to_irq() makes sense in a few cases such as when a GPIO key that can also poll for state want to get hold of an IRQ to react to edges. Now: if a consumer requests IRQ nr 3 from your driver say from ACPI or from a device tree, and as you say GPIOs and IRQs are not 1-to-1 mapped, so IRQ nr 3 may be coming from GPIO 314, isn't this going to be really messy for users? One local numberspace for GPIO and another local numberspace for IRQs? To me it seems like the reasoning is something like - I only use GPIO line numbers like <&gpio 3>; - Then I call gpiod_to_irq() on that number so I do not need to deal with looking up the IRQ some other way - request_irq(); - Profit. There is no guarantee that the API will be used like that at all, actually it is uncommon. Yours, Linus Walleij
Linus Walleij <linus.walleij@linaro.org> writes: > On Sun, Jul 3, 2022 at 1:10 PM Aidan MacDonald > <aidanmacdonald.0x0@gmail.com> wrote: > >> Some GPIO chips require a custom to_irq() callback for mapping >> their IRQs, eg. because their interrupts come from a parent IRQ >> chip where the GPIO offset doesn't map 1-to-1 with hwirq number. >> >> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com> > > What is the usecase for this? This is a generic GPIO chip; so I guess any use case for ->to_irq()? > Since GPIO chips and IRQ chips are orthogonal there is absolutely > no guarantee that ->to_irq() is called before a driver start to use > an IRQ from the irqchip side: > > (quoting Documentation/driver-api/gpio/driver.rst) > > It is legal for any IRQ consumer to request an IRQ from any irqchip even if it > 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. > > Always prepare the hardware and make it ready for action in respective > callbacks from the GPIO and irq_chip APIs. Do not rely on gpiod_to_irq() having > been called first. > > (end quote) That's fine, and I don't require ->to_irq() to be called. The IRQ chip in my case is provided by an MFD driver (axp20x to be specific) and it *does* work orthogonally to the GPIO driver -- the GPIO driver neither knows nor cares about the IRQ chip. > Using ->to_irq() makes sense in a few cases such as when > a GPIO key that can also poll for state want to get hold of an > IRQ to react to edges. This is my use case; specifically, GPIO keys and ASoC jack detection. > Now: if a consumer requests IRQ nr 3 from your driver say from ACPI or > from a device tree, and as you say GPIOs and IRQs are not 1-to-1 mapped, > so IRQ nr 3 may be coming from GPIO 314, isn't this going to be really > messy for users? One local numberspace for GPIO and another local > numberspace for IRQs? Well, this is how MFD drivers with GPIO functionality often work, they aren't creating a special IRQ sub-domain for GPIOs and it doesn't seem to be a problem there. Most likely because those MFD devices are being used for GPIO keys or something similar. Referring to the interrupt directly would make sense if the GPIO was wired to another chip's IRQ line, but that is unlikely to be the case for MFD devices because they're behind a slow bus. > To me it seems like the reasoning is something like > > - I only use GPIO line numbers like <&gpio 3>; > - Then I call gpiod_to_irq() on that number so I do not need to > deal with looking up the IRQ some other way > - request_irq(); > - Profit. > Yeah, that's accurate for my use case. > There is no guarantee that the API will be used like that at all, actually > it is uncommon. > > Yours, > Linus Walleij I'm not trying to argue that hierarchical IRQ domains are always a bad thing -- I'm just pointing out they're not always useful or necessary. All your points make sense when the GPIO controller is a large distinct block with potentially many GPIOs. When we're dealing with an MFD device with just a few GPIOs, maybe even just one, having a separate IRQ domain makes less sense; the added structure is generally not useful. Looking at other GPIO drivers using a hierarchical IRQ domain, they include their own IRQ chips with specialized ops. In my case I don't need any of that (and it'd be the same with other MFD devices) so it looks like using an IRQ domain would mean I'd have to create a fake IRQ chip and domain just to translate between two number spaces. Is that really better than simply using ->to_irq()? Regards, Aidan
On Tue, Jul 5, 2022 at 1:22 PM Aidan MacDonald
<aidanmacdonald.0x0@gmail.com> wrote:
...
> Is that really better than simply using ->to_irq()?
We have Intel PMIC drivers (that are in MFD) and they have respective
GPIO drivers, none of them is using ->to_irq() and all of them provide
IRQ functionality. Can it be taken as an example or is it something
quite different to your hardware?
On Tue, Jul 5, 2022 at 1:08 PM Aidan MacDonald <aidanmacdonald.0x0@gmail.com> wrote: > Linus Walleij <linus.walleij@linaro.org> writes: > I'm not trying to argue that hierarchical IRQ domains are always a bad > thing -- I'm just pointing out they're not always useful or necessary. > All your points make sense when the GPIO controller is a large distinct > block with potentially many GPIOs. When we're dealing with an MFD device > with just a few GPIOs, maybe even just one, having a separate IRQ domain > makes less sense; the added structure is generally not useful. Do you mean your driver does this: MFD main device MFD irqchip | +-> MFD gpiochip No irqchip here, so .to_irq() just refers ^ to that one up there IIUC you mean that if I want to use the irqchip directly then I have to refer to the MFD irqchip, I just cannot refer to the gpiochip subnode because that one does not have an irqchip. // Getting GPIO from gpiochip and irq from MFD device // for the same GPIO line gpios = <&gpio 3 GPIO_ACTIVE_LOW>; irqs = <&mfd 114 IRQ_EDGE_RISING>; Then for a Linux driver this can be papered over by using the .to_irq() callback and just defining gpios. This isn't very good, if you created a separate gpiochip then you should have a separate (hierarchical) irqchip associated with that gpiochip as well. // Getting GPIO and irq from the same gpiochip node gpios = <&gpio 3 GPIO_ACTIVE_LOW>; irqs = <&gpio 3 IRQ_EDGE_RISING>; I made this mistake with the ab8500 driver and I would not do it like this today. I would use hierarchical gpio irqchip. And I should go and fix it. (Is on my TODO.) > Looking at other GPIO drivers using a hierarchical IRQ domain, they > include their own IRQ chips with specialized ops. In my case I don't > need any of that (and it'd be the same with other MFD devices) so it > looks like using an IRQ domain would mean I'd have to create a fake > IRQ chip and domain just to translate between two number spaces. > > Is that really better than simply using ->to_irq()? To be honest most irqchips are "fake", what they mostly do is figure out which of a few internal sources that fired the irq, so it models the different things connected to a single IRQ line. So yeah, I think the hierarchical irqchip is worth it, especially if that means the offset of the irqs and gpios become the same. Maybe we can add more helpers in the core to make it dirt simple though? It would help others with the same problem. Yours, Linus Walleij
Linus Walleij <linus.walleij@linaro.org> writes: > On Tue, Jul 5, 2022 at 1:08 PM Aidan MacDonald > <aidanmacdonald.0x0@gmail.com> wrote: >> Linus Walleij <linus.walleij@linaro.org> writes: > >> I'm not trying to argue that hierarchical IRQ domains are always a bad >> thing -- I'm just pointing out they're not always useful or necessary. >> All your points make sense when the GPIO controller is a large distinct >> block with potentially many GPIOs. When we're dealing with an MFD device >> with just a few GPIOs, maybe even just one, having a separate IRQ domain >> makes less sense; the added structure is generally not useful. > > Do you mean your driver does this: > > MFD main device > MFD irqchip > | > +-> MFD gpiochip > No irqchip here, so .to_irq() just refers ^ to that one up there > > IIUC you mean that if I want to use the irqchip directly then > I have to refer to the MFD irqchip, I just cannot refer to the > gpiochip subnode because that one does not have an irqchip. Yep, that's right. > // Getting GPIO from gpiochip and irq from MFD device > // for the same GPIO line > gpios = <&gpio 3 GPIO_ACTIVE_LOW>; > irqs = <&mfd 114 IRQ_EDGE_RISING>; > > Then for a Linux driver this can be papered over by using the > .to_irq() callback and just defining gpios. > > This isn't very good, if you created a separate gpiochip then you > should have a separate (hierarchical) irqchip associated with that > gpiochip as well. > > // Getting GPIO and irq from the same gpiochip node > gpios = <&gpio 3 GPIO_ACTIVE_LOW>; > irqs = <&gpio 3 IRQ_EDGE_RISING>; > > I made this mistake with the ab8500 driver and > I would not do it like this today. I would use hierarchical gpio > irqchip. And I should go and fix it. (Is on my TODO.) > If moving to hierarchical IRQ chips is the plan, could we add a note to say .to_irq() is discouraged and shouldn't be used in new code? Based on what you're saying (which I agree makes sense) it sounds like there's really no reason to ever use .to_irq(). >> Looking at other GPIO drivers using a hierarchical IRQ domain, they >> include their own IRQ chips with specialized ops. In my case I don't >> need any of that (and it'd be the same with other MFD devices) so it >> looks like using an IRQ domain would mean I'd have to create a fake >> IRQ chip and domain just to translate between two number spaces. >> >> Is that really better than simply using ->to_irq()? > > To be honest most irqchips are "fake", what they mostly do is figure > out which of a few internal sources that fired the irq, so it models the > different things connected to a single IRQ line. > > So yeah, I think the hierarchical irqchip is worth it, especially if that > means the offset of the irqs and gpios become the same. > > Maybe we can add more helpers in the core to make it dirt simple > though? It would help others with the same problem. > > Yours, > Linus Walleij Okay, that sounds like a good plan. I'll look more carefully at the existing drivers and see if I can use existing gpiolib helpers. One potential issue (from reading the code) is that hierarchical IRQ domains seemingly can't have a non-hierarchical domain as the parent: irq_domain_alloc_irqs_parent() calls irq_domain_alloc_irqs_hierarchy() and the latter fails with -ENOSYS for a non-hierarchical domain. In my case I'm using a regmap IRQ chip, which is non-hierarchical, so perhaps that will need to be expanded? Regards, Aidan
Andy Shevchenko <andy.shevchenko@gmail.com> writes: > On Tue, Jul 5, 2022 at 1:22 PM Aidan MacDonald > <aidanmacdonald.0x0@gmail.com> wrote: > > ... > >> Is that really better than simply using ->to_irq()? > > We have Intel PMIC drivers (that are in MFD) and they have respective > GPIO drivers, none of them is using ->to_irq() and all of them provide > IRQ functionality. Can it be taken as an example or is it something > quite different to your hardware? In the Intel PMICs the MFD irqchip has a single interrupt for all GPIOs. The GPIO driver then has its own irqchip and it looks at other registers to find out which GPIO interrupt fired. It's a typical cascaded setup. In my case the MFD irqchip has one interrupt per GPIO. The GPIO driver does not need its own irqchip; everything is handled by the MFD irqchip. Existing examples include wm831x, wm8994, da9052, and tps6586x.
On Wed, Jul 6, 2022 at 4:21 PM Aidan MacDonald <aidanmacdonald.0x0@gmail.com> wrote: > If moving to hierarchical IRQ chips is the plan, could we add a note > to say .to_irq() is discouraged and shouldn't be used in new code? > Based on what you're saying (which I agree makes sense) it sounds > like there's really no reason to ever use .to_irq(). There is a reason to use .to_itq() to get the irq associated with a GPIO, in a Linux kernel context. Especially when doing exactly what IRQ keys are doing: finding the IRQ for a GPIO line, if any. But it is not intended to provide a random IRQ from anywhere else in the system. Maybe I could clarify that. There are many old boardfile-instances of GPIO where .to_irq() may be doing something really funky, these are not really up to date with the current idea of how we model systems in hardware description languages such as DT or ACPI DSDT. There may be archs that do not even have DT or ACPI or similar, there it would be fine to use it in a more liberal way. This may be the case for m68k or something so I cannot speak about everything here. I understand that it might be annoying that this is a bit fuzzy around the edges. > Okay, that sounds like a good plan. I'll look more carefully at the > existing drivers and see if I can use existing gpiolib helpers. > > One potential issue (from reading the code) is that hierarchical IRQ > domains seemingly can't have a non-hierarchical domain as the parent: > irq_domain_alloc_irqs_parent() calls irq_domain_alloc_irqs_hierarchy() > and the latter fails with -ENOSYS for a non-hierarchical domain. Yes that is a characteristic of hierarchical irq domains, they have to be hierarchical all the way down. It is very often what we want as well. > In my case I'm using a regmap IRQ chip, which is non-hierarchical, > so perhaps that will need to be expanded? Yes I'm sorry if it adds work to your plate :( However I think the end result would be very useful for everyone. Maybe Marc Z has something to add here? Yours, Linus Walleij
diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c index 4bc01329fb14..d11b202e51fd 100644 --- a/drivers/gpio/gpio-regmap.c +++ b/drivers/gpio/gpio-regmap.c @@ -34,6 +34,8 @@ struct gpio_regmap { unsigned int *reg, unsigned int *mask, unsigned int *values); + int (*to_irq)(struct gpio_regmap *gpio, unsigned int offset); + void *driver_data; }; @@ -193,6 +195,13 @@ static int gpio_regmap_direction_output(struct gpio_chip *chip, return gpio_regmap_set_direction(chip, offset, true); } +static int gpio_regmap_to_irq(struct gpio_chip *chip, unsigned int offset) +{ + struct gpio_regmap *gpio = gpiochip_get_data(chip); + + return gpio->to_irq(gpio, offset); +} + void *gpio_regmap_get_drvdata(struct gpio_regmap *gpio) { return gpio->driver_data; @@ -242,6 +251,10 @@ struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config if (config->reg_field_xlate && config->reg_mask_xlate) return ERR_PTR(-EINVAL); + /* an irq_domain will override the to_irq hook, so don't allow both */ + if (config->irq_domain && config->to_irq) + return ERR_PTR(-EINVAL); + gpio = kzalloc(sizeof(*gpio), GFP_KERNEL); if (!gpio) return ERR_PTR(-ENOMEM); @@ -253,6 +266,7 @@ struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config gpio->reg_stride = config->reg_stride; gpio->reg_mask_xlate = config->reg_mask_xlate; gpio->reg_field_xlate = config->reg_field_xlate; + gpio->to_irq = config->to_irq; gpio->reg_dat_base = config->reg_dat_base; gpio->reg_set_base = config->reg_set_base; gpio->reg_clr_base = config->reg_clr_base; @@ -302,6 +316,9 @@ struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config chip->direction_output = gpio_regmap_direction_output; } + if (gpio->to_irq) + chip->to_irq = gpio_regmap_to_irq; + ret = gpiochip_add_data(chip, gpio); if (ret < 0) goto err_free_gpio; diff --git a/include/linux/gpio/regmap.h b/include/linux/gpio/regmap.h index 47acea8cca32..9755854d6747 100644 --- a/include/linux/gpio/regmap.h +++ b/include/linux/gpio/regmap.h @@ -45,6 +45,8 @@ struct regmap; * to register, mask, and field values. If not given * the default gpio_regmap_field_xlate() is used, which * is implemented in terms of ->reg_mask_xlate. + * @to_irq: (Optional) hook for supporting custom IRQ mappings, + * behaves the same as the gpio_chip to_irq hook. * @drvdata: (Optional) Pointer to driver specific data which is * not used by gpio-remap but is provided "as is" to the * driver callback(s). @@ -102,6 +104,8 @@ struct gpio_regmap_config { unsigned int *reg, unsigned int *mask, unsigned int *values); + int (*to_irq)(struct gpio_regmap *gpio, unsigned int offset); + void *drvdata; };
Some GPIO chips require a custom to_irq() callback for mapping their IRQs, eg. because their interrupts come from a parent IRQ chip where the GPIO offset doesn't map 1-to-1 with hwirq number. Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com> --- drivers/gpio/gpio-regmap.c | 17 +++++++++++++++++ include/linux/gpio/regmap.h | 4 ++++ 2 files changed, 21 insertions(+)