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

[Matti Vaittinen]

On Thu, Dec 27, 2018 at 09:56:48AM +0200, Matti Vaittinen wrote:
> 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.
> > > 
> > > 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.
> > 
> > > > @@ -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.

Geert, I did send this patch yesterday. It's here if you wish to try it:
https://lore.kernel.org/patchwork/patch/1027963/

Last night - just when I was about to get some sleep - it stroke me. I
think the correct thing to do would be leaving the irq_set_type to NULL
for those IRQ chips which do not support type setting. If we do that,
then the irq core will take care of situations where user requests type
setting but the chip does not support it. Which means the regmap-irq
would be no different from any other irq chip where type setting is not
supported.

/// irrelevant ///
I guess you know the moment of "Hypnagogia" when you are comfortably at
bed your head feels all dizzy and world is starting to faint away...
And just a second later you are fully awake thinking of a possible
solution - and definitely not able to sleep anymore :/

https://www.reddit.com/r/ProgrammerHumor/comments/93eq0e/everytime/
/// irrelevant ends ///

I just took a peek in 
kernel/irq/manage.c - and found following:

int __irq_set_trigger(struct irq_desc *desc, unsigned long flags)
{
	struct irq_chip *chip = desc->irq_data.chip;
	int ret, unmask = 0;

	if (!chip || !chip->irq_set_type) {
		/*
		 * IRQF_TRIGGER_* but the PIC does not support multiple
		 * flow-types?
		 */
		pr_debug("No set_type function for IRQ %d (%s)\n",
			 irq_desc_get_irq(desc),
			 chip ? (chip->name ? : "unknown") : "unknown");
		return 0;
	}

...

so at the moment the IRQ core is also just spilling out the warning and
then returning zero. But at least we would get the warning from core if
the irq-chip does not support type config.

So at the cost of removing "const" from regmap_irq_chip we could do:

diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
index 2a031743f31f..b6aef50ab378 100644
--- a/drivers/base/regmap/regmap-irq.c
+++ b/drivers/base/regmap/regmap-irq.c
@@ -298,12 +298,11 @@ static int regmap_irq_set_wake(struct irq_data *data, unsigned int on)
 	return 0;
 }
 
-static const struct irq_chip regmap_irq_chip = {
+static struct irq_chip regmap_irq_chip = {
 	.irq_bus_lock		= regmap_irq_lock,
 	.irq_bus_sync_unlock	= regmap_irq_sync_unlock,
 	.irq_disable		= regmap_irq_disable,
 	.irq_enable		= regmap_irq_enable,
-	.irq_set_type		= regmap_irq_set_type,
 	.irq_set_wake		= regmap_irq_set_wake,
 };
 
@@ -610,6 +609,7 @@ int regmap_add_irq_chip(struct regmap *map, int irq, int irq_flags,
 
 	num_type_reg = chip->type_in_mask ? chip->num_regs : chip->num_type_reg;
 	if (num_type_reg) {
+		regmap_irq_chip.irq_set_type = regmap_irq_set_type;
 		d->type_buf_def = kcalloc(num_type_reg,
 					  sizeof(unsigned int), GFP_KERNEL);
 		if (!d->type_buf_def)



Mark, Geert, what do you think? (And maybe same for the .irq_set_wake -
but I did omit this as I have never looked at the wake functionality
before).

Br,
	Matti Vaittinen

-- 
Matti Vaittinen
ROHM Semiconductors

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