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

[Matti Vaittinen <matti.vaittinen@xxxxxxxxxxxxxxxxx>]

Hello All,

On Thu, Dec 27, 2018 at 09:35:31AM +0200, Matti Vaittinen wrote:
> 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@xxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxx>
> > 
> > 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().

Right. Now I see what you mean. Original code did:

	if (!(irq_data->type_rising_mask | irq_data->type_falling_mask))
		return 0;

Eg, even when the driver was not able to perform the type-setting this
failure was silently ignored, right. So doing:
       if ((t->types_supported & type) != type)
               return 0;
would be functionally equal. It feels like utterly wrong thing to do
(to me) - if driver is written to work with edge or level active
interrupts - and if the irq controller is not supporting this - then we
should warn the user. Just silently ignoring this sounds like asking for
irq storm or missed interrupts - but maybe I just don't get this =)

I'll send a patch with 
	if (!(irq_data->type_rising_mask | irq_data->type_falling_mask))
		return 0;
in order to not break existing functionality - but it feels plain wrong
to me.


> Br,
> 	Matti Vaittinen
> 
> -- 
> Matti Vaittinen
> ROHM Semiconductors
> 
> ~~~ "I don't think so," said Rene Descartes.  Just then, he vanished ~~~

-- 
Matti Vaittinen
ROHM Semiconductors

~~~ "I don't think so," said Rene Descartes.  Just then, he vanished ~~~