Re: [RFC] regmap-irq: add "main register" and level-irq support

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3312 bytes --]

On Fri, Nov 30, 2018 at 10:59:08AM +0200, Matti Vaittinen wrote:

> IRQ handling is implemented so that each sub block has own
> status/ack/mask registers and then there is one main status
> register. When any of the sub blocks gets unmasked IRQ,
> corresponding 'main status register' bit is set in main status
> register.

This sounds exactly like the wm831x which uses cascaded irqchips for
this, though not regmap-irq.

> Problem I faced is that I don't know how to create two regmap-irq chips for
> the same i2c decvice - because the regmap-irq gets the DT node from regmnap
> - and thus both of the irq chips would have same DT node. My assumption is
> that the DT - xlate functionality for this setup would not be quite correct,
> right? I guess the "main irq" interrupts would be hookable via DT? But
> the devices are interested in "sub IRQs". So I thought that the correct
> solution would be to model the interrupts from this PMIC as just one
> irq-chip. So I wonder if addin this "main IRQ register" support for

I suspect the idiomatic way to handle this in DT is to make DT nodes
(and devices) for the subfunction interrupt controllers and expose the
internal structure of the chip to the DT.  This does make some sense as
the individual interrupt controllers within the chip are clearly
reusable IP blocks though it means that the chip starts looking a bit
weird externally so perhaps that's not ideal and your suggestion makes
sense.

> configuring IRQ pins as level triggered. So I thought we could add
> support for setting the type to level_low or level_high. This change
> would require adding 'supported_types' information to all drivers which
> are currently using regmap-irq for supporting type-setting - but quick
> grep for drivers allow me to think that it is currently only the 
> gpio-max77620 which uses the type setting in-tree.

That seems pluasible but definitely seems like a separate change.

> + * @main_status: Base main status register address. For chips which have
> + *		 interrupts arranged in separate sub-irq blocks with own IRQ
> + *		 registers and which have a main IRQ registers indicating
> + *		 sub-irq blocks with unhandled interrupts. For such chips fill
> + *		 sub-irq register information in status_base, mask_base and
> + *		 ack_base.
> + * @sub_reg_offsets: arrays of mappings from main register bits to sub irq
> + *		 registers. First item in array describes the registers
> + *		 for first main status bit. Second array for second bit etc.
> + *		 Offset is given as sub register status offset to status_base.
> + *		 Should contain num_regs arrays. Can be provided for chips with
> + *		 more complex mapping than 1.st bit to 1.st sub-reg, 2.nd bit to
> + *		 2.nd sub-reg, ...

This interface feels a little awkward in that we define the sub
interrupts in this separate array and require them all to be in separate
single registers which probably won't be true in general (IIRC it wasn't
how the wm831x worked).  How about instead we have the individual
interrupts mark which main status bits flag them, then build up a
mapping in the other direction during init of subregisters to read?
I've not tried to implement that so there might be annoying difficulties
during implementation though.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]