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

[Matti Vaittinen <mazziesaccount@gmail.com>]

Hi Thomas,

Thanks for taking a look at this.

On 6/3/24 13:20, Thomas Gleixner wrote:
> On Fri, May 24 2024 at 11:18, Matti Vaittinen wrote:
>> When multiple IRQ domains are created from same device-tree node they
> 
> s/IRQ/interrupt/
> 
> Also most of your sentences lack a substantial amount of articles.

Usual ack for any language related comments. I'll try to proofread and 
improve.

>> will get same name based on the device-tree path. This will cause a
>> naming collision in debugFS when IRQ domain specific entries are
>> created.
>>
>> One use-case for being able to create multiple IRQ domains / single
>> device node is using regmap-IRQ controller code for devices which
>> provide more than one physical IRQ.
> 
> This does not make sense. Why do you need multiple interrupt domains if
> there is more than one physical interrupt?

I'll try to explain myself better below.

>> It seems much cleaner to instantiate
> 
> 'It seems' is not a technical argument.

I am always having problems claiming something is _absolutely_ cleaner, 
as "clean" or "messy" are subjective and depend on reader. To me this is 
cleaner. Also, Mark Brown thought it is cleaner. Still, it does not mean 
this is absolutely cleaner for everyone. I am open to suggestions how to 
rephrase.

>> own regmap-IRQ controller for each parent IRQ because most of the regmap
>> IRQ properties are really specific to parent IRQ.
> 
> 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 :)

>> -struct irq_domain *irq_domain_create_legacy(struct fwnode_handle *fwnode,
>> +struct irq_domain *irq_domain_create_legacy_named(struct fwnode_handle *fwnode,
>>   					 unsigned int size,
>>   					 unsigned int first_irq,
>>   					 irq_hw_number_t first_hwirq,
>>   					 const struct irq_domain_ops *ops,
>> -					 void *host_data)
>> +					 void *host_data, const char *name_suffix)
>>   {
>>   	struct irq_domain *domain;
>>   
>> -	domain = __irq_domain_add(fwnode, first_hwirq + size, first_hwirq + size, 0, ops, host_data);
>> +	domain = __irq_domain_add(fwnode, first_hwirq + size, first_hwirq + size,
>> +				  0, ops, host_data, name_suffix);
>>   	if (domain)
>>   		irq_domain_associate_many(domain, first_irq, first_hwirq, size);
>>   
>>   	return domain;
>>   }
>> +EXPORT_SYMBOL_GPL(irq_domain_create_legacy_named);
> 
> What for? This new stuff is not going to be used for legacy setups with
> hard coded Linux interrupt numbers. So there is no point to add a
> function plus an export which is never used.

Thanks for pointing this out. I am more than glad to drop this. It'll 
just mean that the regmap-IRQ (which supports using the legacy domains) 
should warn if user tries to use legacy domain with name suffix. Mark, 
does this sound reasonable to you?

Thanks for all the input!
	-- Matti

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

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