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

[Matti Vaittinen <matti.vaittinen@xxxxxxxxxxxxxxxxx>]

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@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().

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,
	Matti Vaittinen

-- 
Matti Vaittinen
ROHM Semiconductors

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