Re: [PATCH 09/18] irqdomain: Rename _add functions to _add_*_of_node

[Jiri Slaby <jirislaby@kernel.org>]

Thomas,

sorry for the delay, I drowned in tty.

On 06. 02. 25, 17:22, tglx wrote:
> On Wed, Jan 15 2025 at 09:53, Jiri Slaby (SUSE) wrote:
>> For readers, it is very confusing that:
>> * irq_domain_add_*() functions are dedicated to of_nodes,
>> * irq_domain_create_*() ones to fwnodes, and
>> * irq_domain_instantiate() to the universal struct irq_domain_info.
>>
>> Neither _create, nor _add designate any of those nodes. Despite the
>> naming, the functionality of them is the same: add an irq domain (by
>> generic irq_domain_instantiate()). So the source of the confusion is the
>> naming proper -- making the distinction based on _create, _add, and
>> _instantiate.
>>
>> Therefore, here an "_of_node" suffix is added to all "_add" functions
>> (of_node ones). In the next patch, "_create" (fwnode ones) are switched
>> to "_add_fwnode". And finally, "_instantiate" is renamed to "_add".
>>
>> So when all are applied, the interface is much easier to follow:
>> * dom = irq_domain_add_linear_of_node(of_node, ...)
>> * dom = irq_domain_add_linear_fwnode(fwnode, ...)
>> * dom = irq_domain_add(info)
>> * irq_domain_remove(dom)
> 
> I'm not convinced that this _of_node() _fwnode() churn is actually
> valuable. I rather go and consolidate the code so that the core
> functions take a fwnode argument, i.e.
> 
>     - irq_domain_add_xxx(node, ...)
>     + irq_domain_add_xxx(of_fwnode_handle(node), ....)
> 
> It's not asked too much from the developer to use of_fwnode_handle() at
> the call site and the resulting treewide churn is pretty much the same
> as in any case all call sites need to be touched.

OK, NP. I am only confused by your "I rather go". Does it mean you are 
already on it? Or should I translate that as "I'd rather go", ie. /me 
doing the work -- I expect this case and can indeed do the job. I just 
don't want to duplicate the work.

> But that brings me to the question of logistics for this overhaul. As
> this is a treewide change, there is quite some potential to create
> conflicts all over the place.
> 
> So the obvious solution is to consolidate on the existing
> irq_domain_create_*() API, which is not the worst naming once everything
> is unified, i.e.
> 
>     - irq_domain_add_xxx(node, ...)
>     + irq_domain_create_xxx(of_fwnode_handle(node), ....)
> 
> It allows to distribute these changes (except for the _nomap() oddity,
> which is OF only) right now to the relevant subsystems and I can collect
> the ignored changes in the irq tree. The final removal of the _add*()
> interfaces can then be done towards the end of the merge window.
> 
> Thoughts?

Sounds good to me.

thanks,
-- 
js
suse labs