Re: [PATCH v3] regmap: regmap-irq/gpio-max77620: add level-irq support

Matti Vaittinen
2018-12-27T07:35:42Z

Hello Geert,

Sorry for waiting - I just opened my computer after the holidays.

On Wed, Dec 26, 2018 at 12:39:17PM +0100, Geert Uytterhoeven wrote:
> Hi Matti,
>
> On Tue, Dec 18, 2018 at 1:00 PM Matti Vaittinen
> <matti.vaittinen@fi.rohmeurope.com> wrote:
> > Add level active IRQ support to regmap-irq irqchip. Change breaks
> > existing regmap-irq type setting. Convert the existing drivers which
>
> Indeed it does.
>
> > use regmap-irq with trigger type setting (gpio-max77620) to work
> > with this new approach. So we do not magically support level-active
> > IRQs on gpio-max77620 - but add support to the regmap-irq for chips
> > which support them =)
> >
> > We do not support distinguishing situation where HW supports rising
> > and falling edge detection but not both. Separating this would require
> > inventing yet another flags for IRQ types.
> >
> > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
>
> This is now upstream as commit 1c2928e3e3212252 ("regmap:
> regmap-irq/gpio-max77620: add level-irq support"), and breaks da9063-rtc
> on the Renesas Koelsch board:
>
> genirq: Setting trigger mode 8 for irq 157 failed
> (regmap_irq_set_type+0x0/0x140)
> da9063-rtc da9063-rtc: Failed to request ALARM IRQ 157: -524
> da9063-rtc: probe of da9063-rtc failed with error -524

This is strange as I do not see any type setting support code in
drivers/mfd/da9063-irq.c. The type setting registers are neither
specified in static const struct regmap_irq_chip da9063l_irq_chip nor
in static const struct regmap_irq_chip da9063_irq_chip. Hence I don't
understand how the da9063 could have been supporting IRQ type setting in
first place.

> > ---
> >
> > Version 3 of this patch is intended to be functionally identical to v2.
> > This patch is rebased on top of a tree which contains changes:
> > "regmap: irq: handle HW using separate rising/falling edge interrupts"
> > from Bartosz Golaszewski and the change
> > "regmap: regmap-irq: Remove default irq type setting from core"
> > (proposed here):
> > https://lore.kernel.org/lkml/20181218105813.GA6957@localhost.localdomain/
> >
> > There should not be direct dependency to "regmap: regmap-irq: Remove
> > default irq type setting from core" though. Patch was also tested to
> > apply cleany on regmap-tree.
> >
> > Same statement regarding testing applies - gpio-max77620 are only
> > tested to compile. All real testing would be _HIGHLY_ appreciated.
> >
> > drivers/base/regmap/regmap-irq.c | 35 ++++++++++-----
> > drivers/gpio/gpio-max77620.c | 96 ++++++++++++++++++++++++++--------------
> > include/linux/regmap.h | 27 ++++++++---
> > 3 files changed, 110 insertions(+), 48 deletions(-)
> >
> > diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
> > index 8b216b2e2c19..31d23c9a5ae7 100644
> > --- a/drivers/base/regmap/regmap-irq.c
> > +++ b/drivers/base/regmap/regmap-irq.c
> > @@ -199,7 +199,7 @@ static void regmap_irq_enable(struct irq_data *data)
> > const struct regmap_irq *irq_data = irq_to_regmap_irq(d, data->hwirq);
> > unsigned int mask, type;
> >
> > - type = irq_data->type_falling_mask | irq_data->type_rising_mask;
> > + type = irq_data->type.type_falling_val | irq_data->type.type_rising_val;
> >
> > /*
> > * The type_in_mask flag means that the underlying hardware uses
> > @@ -234,27 +234,42 @@ static int regmap_irq_set_type(struct irq_data *data, unsigned int type)
> > struct regmap_irq_chip_data *d = irq_data_get_irq_chip_data(data);
> > struct regmap *map = d->map;
> > const struct regmap_irq *irq_data = irq_to_regmap_irq(d, data->hwirq);
> > - int reg = irq_data->type_reg_offset / map->reg_stride;
> > + int reg;
> > + const struct regmap_irq_type *t = &irq_data->type;
> >
> > - if (!(irq_data->type_rising_mask | irq_data->type_falling_mask))
> > - return 0;
> > + if ((t->types_supported & type) != type)
> > + return -ENOTSUPP;
>
> Given types_supported defaults to zero, I think this breaks every existing
> setup using REGMAP_IRQ_REG().

Not really. The type setting support is not too old or widely used in
regmap_irq. If the type_base and num_type_reg in struct regmap_irq_chip
have not been given the type setting has not been supported. And the
macro REGMAP_IRQ_REG() has not been initializing those fields - they
must have been explicitly set by the driver. Only in-tree driver which
really used the regmap_irq type-setting was gpio-max77620 (if my
grepping did not go totally wrong). Is your version of
drivers/mfd/da9063-irq.c same I can see in
https://elixir.bootlin.com/linux/v4.20/source/drivers/mfd/da9063-irq.c ?

I will try to see if the regmap_irq code has just silently ignored irq
type setting requests if type setting information has not been populated
by driver. May that has been changed by my patch. I wonder what would be
the correct action if there is no type-setting information in
struct regmap_irq_chip - and if type setting irq callbacks are called.

Br,