Re: [RFC PATCH 0/6] Support ROHM BD96801 scalable PMIC

[Matti Vaittinen]

Hi dee Ho peeps,

On 4/5/24 12:19, Matti Vaittinen wrote:
> On 4/4/24 16:15, Matti Vaittinen wrote:
> > Hi Mark,
> >
> > On 4/4/24 15:09, Mark Brown wrote:
> > > On Thu, Apr 04, 2024 at 10:26:34AM +0300, Matti Vaittinen wrote:
> > >
> > > > 1. Should we be able to have more than 1 IRQ domain / device?
> > > > 2. Should regmap_irq support having more than 1 HWIRQ
> > >
> > > I would expect each parent interrupt to show up as a separate remap_irq.

..
> > > So if we arrange to supply a name when we register multiple domains
> > > things should work fine?
>
> After my latest findings, yes, I think so. How to do this correctly is 
> beyond me though. The __irq_domain_create() seems to me that the name is 
> meant to be the dt-node name when the controller is backed by a real 
> dt-node. Naming of the irq_domain_alloc_named_fwnode() sounds to me like 
> it is only intended to be used when there is no real fwnode. All 
> suggestions appreciated. Using the:
> irq_domain_update_bus_token(intb_domain, DOMAIN_BUS_WIRED);
> feels like a dirty hack, and won't scale if there is more HWIRQs.

I tried taking a look at this again.

If we wanted to support multiple HWIRQs / regmap-IRQ controller, it 
would require us to duplicate almost everything in the struct 
regmap_irq_chip for every new parent IRQ. The status/mask register 
information, IRQ type, etc. Naturally, it would require also duplicating 
lot of the data contained in the struct regmap_irq_chip_data. I am not 
sure if this could be done so the change is not reflected in the 
existing IRQ data initialization macros etc. Furthermore, some API 
changes would be required like changes to regmap_irq_get_domain().

I am a bit afraid this change, if implemented in regmap-IRQ, would be 
very intrusive and potentially impact large amount of callers. But more 
importantly, looking the amount of data that should be duplicated per 
new HWIRQ makes me think that an IRQ controller is really a product of a 
parent IRQ, not product of the device. Hence, assuming there is only one 
IRQ controller instance / device does not feel any more correct than 
assuming there is an IRQ controller instance / parent IRQ. Same thinking 
applies to IRQ domains.

Thus, forcing the regmap-IRQ to support multiple parents instead of 
having own regmap-IRQ instance / parent IRQ feels like fitting square 
item to a round hole. I am sure fixing all the bugs I caused would give 
donate a lot of EXP-points though :rolleyes:

Question is, should I still try?

Another option I see, is trying to think if irq-domain name could be 
changed. (This is what the RFC v3 does, [ab]using the 
irq_domain_update_bus_token()). I was a bit put off by the idea of 
'instantiating' multiple domains (or regmap-IRQ controllers) from a 
single node, but more I think of this, more I lean towards it. Besides, 
this is not something completely odd, I think MFD devices do this 
anyways. (Instantiate multiple [sub]devices from single DT-node). I 
would love to get an opinion from someone who knows the 'fundamentals' 
of the IRQ domains, and possibly some pointer for the right approach.

Finally we might also consider adding own sub-node in DT for each parent 
IRQ - but this feels very wrong to me.

All in all, I am having very hard time trying to think how to proceed. 
The last option for me is to skip support for the ERRB IRQ from the 
BD96801 driver, which would leave this problem to the next person 
working with a device providing multiple physical IRQs.

Yours,
	-- Matti

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

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