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

Matti Vaittinen
2024-04-22T10:52:32Z

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! ~~