Re: [PATCH v2 1/3] regmap-irq: Add support for peripheral offsets

[Mark Brown <broonie@kernel.org>]

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

On Thu, Oct 22, 2020 at 02:35:40PM -0700, Guru Das Srinagesh wrote:

> Some MFD chips do not have the register space for their peripherals
> mapped out with a fixed stride. Add peripheral address offsets to the
> framework to support such address spaces.

> In this new scheme, the regmap-irq client registering with the framework
> shall have to define the *_base registers (e.g. status_base, mask_base,
> type_base, etc.) as those of the very first peripheral in the chip, and
> then specify address offsets of each subsequent peripheral so that their
> corresponding *_base registers may be calculated by the framework. The
> first element of the periph_offs array must be zero so that the first
> peripherals' addresses may be accessed.

> Some MFD chips define two registers in addition to the IRQ type
> registers: POLARITY_HI and POLARITY_LO, so add support to manage their
> data as well as write to them.

It is difficult to follow what this change is supposed to do, in part
because it looks like this is in fact two separate changes, one adding
the _base feature and another adding the polarity feature.  These should
each be in a separate patch if that is the case, and I think each needs
a clearer changelog - I'm not entirely sure what the polarity feature is
supposed to do.  Nothing here says what POLARITY_HI and POLARITY_LO are,
how they interact or anything.

For the address offsets I'm not sure that this is the best way to
represent things.  It looks like the hardware this is trying to describe
is essentially a bunch of separate interrupt controllers that happen to
share an upstream interrupt and I think that the code would be a lot
clearer if at least the implementation looked like this.  Instead of
having to check for this array of offsets at every use point (which is
going to be rarely used and hence prone to bugs) we'd have a set of
separate regmap-irqs and then we'd mostly only have to loop through them
on handling, the bulk of the implementation wouldn't have to worry about
this special case.

Historically genirq didn't support sharing threaded interrupts, if
that's not changed we'd need to open code everything inside regmap-irq
but it would be doable, or ideally genirq could grow this feature.  If
it's done inside regmap you'd have a separate API that took an array of
regmap-irq configurations instead of just one and then when an interrupt
is delivered just loops through all of them handling it.  A quick scan
through the interrupt code suggests it might be able to cope with shared
IRQs now though which would make life easier.

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