Re: [PATCH v2 07/10] irqdomain: Allow giving name suffix for domain

[Matti Vaittinen]

On 6/3/24 19:38, Thomas Gleixner wrote:
> Matti!
>
> On Mon, Jun 03 2024 at 15:19, Matti Vaittinen wrote:
> > On 6/3/24 13:20, Thomas Gleixner wrote:
> > > On Fri, May 24 2024 at 11:18, Matti Vaittinen wrote:
> > > Now you start talking about parent interrupts. Can you please make your
> > > mind up and concisely explain what this is about?
> >
> > I hope I can explain what I am after here. I am also very happy when
> > incorrect terminology is pointed out! So, it'd be great to know if I
> > should use 'parent' or 'physical IRQ' here after I explain this further.
> >
> > What I am dealing with is an I2C device (PMIC) which provides two GPIO
> > IRQ lines for SOC. This is what I meant by "physical IRQs".
> >
> > ----------------	INTB IRQ	-----------------
> > |		|-----------------------|		|
> > |	PMIC	|			|    SOC	|
> > |		|-----------------------|		|
> > -----------------	ERRB IRQ	-----------------
> >
> >
> > Both the INTB and ERRB can report various events, and correct event can
> > be further read from the PMIC registers when IRQ line is asserted. I
> > think this is business as usual.
> >
> > I'd like to use the regmap_irq for representing these events as separate
> > 'IRQs' (which can be handled using handlers registered with
> > request_threaded_irq() as usual).
> >
> > Here, when talking about 'parent IRQ', I was referring to ERRB or INTB
> > as 'parent IRQ'. My thinking was that, the regmap IRQ instance uses INTB
> > or ERRB as it's parent IRQ, which it splits (demuxes) to separate "child
> > IRQs" for the rest of the PMIC drivers to use. I'd be grateful if better
> > terms were suggested so that readers can stay on same page with me.
> >
> > After talking with Mark:
> >
> > we both thought it'd be cleaner to have separate regmap IRQ instances
> > for the ERRB and INTB lines. This makes sense (to me) because a lot of
> > (almost all of?) the regmap IRQ internal data describe the IRQ line
> > related things like registers related to the IRQ line, IRQ line polarity
> > etc. Hence, making single regmap-IRQ instance to support more than one
> > <please, add what is the correct term for INTB / ERRB like line> would
> > require duplicating a plenty of the regmap data. This would make
> > registering an regmap-IRQ entity much more complex and additionally it'd
> > also complicate the internals of the regmap-IRQ. It'd be a bit like
> > trying to fill and drink a six-pack of lemonade at once, instead of
> > going a bottle by bottle :)
>
> This makes a lot more sense now. So what I read out of this in change
> log style is:
>
>    Devices can have subfunctions where each has its own interrupt
>    line. Each interrupt line acts as demultiplex interrupt for
>    subfunction specific interrupts and has its distinct set of registers.
>
>    regmap can support such a setup, but the interrupt domain code ends up
>    to assign the same device name when creating the underlying per
>    subfunction interrupt domains. This causes name space collision in the
>    debugfs code and also leads to confusion in other places, e.g.
>    /proc/interrupts would show two distinct interrupts with the same
>    domain name and hardware interrupt number.
>
>    Instead of working around this in the driver or the regmap code, allow
>    to provide a name suffix for the domain name when creating the
>    interrupt domains.
>
>    Add the infrastructure to __irq_domain_add() and expose it via
>    irq_domain_create_linear_named() which is the only function required
>    by the regmap code to support such setups.
>
> Did I get it halfways right?

I think you got it perfectly right. :) And, I really appreciate the 
extra mile you went when spelling it out in this way. I will send a new 
version where the legacy domain function is dropped, and the commit 
message will use what you wrote here  :)

Thanks a ton!

Yours,
	-- Matti

--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~