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