[PATCH v4 02/10] property: Add device_get_child_node_count_named()

[Matti Vaittinen]

On 26/02/2025 16:11, Andy Shevchenko wrote:
> On Wed, Feb 26, 2025 at 04:04:02PM +0200, Matti Vaittinen wrote:
>> On 25/02/2025 15:59, Andy Shevchenko wrote:
>>> On Tue, Feb 25, 2025 at 03:29:17PM +0200, Matti Vaittinen wrote:
>>>> On 25/02/2025 12:39, Andy Shevchenko wrote:
>>>>> On Tue, Feb 25, 2025 at 12:29:31PM +0200, Matti Vaittinen wrote:
>>>>>> On 25/02/2025 12:21, Andy Shevchenko wrote:
>>>>>>> On Tue, Feb 25, 2025 at 11:40:16AM +0200, Heikki Krogerus wrote:
> 
> ...
> 
>>>>>>>>
>>>>>>>> I did not check how many users are you proposing for this, but if
>>>>>>>> there's only one, then IMO this should not be a global function yet.
>>>>>>>> It just feels to special case to me. But let's see what the others
>>>>>>>> think.
>>>>>>>
>>>>>>> The problem is that if somebody hides it, we might potentially see
>>>>>>> a duplication in the future. So I _slightly_ prefer to publish and
>>>>>>> then drop that after a few cycles if no users appear.
>>>>>>
>>>>>> After taking a very quick grep I spotted one other existing place where we
>>>>>> might be able to do direct conversion to use this function.
>>>>>>
>>>>>> drivers/net/ethernet/freescale/gianfar.c
>>>>>>
>>>>>> That'd be 2 users.
>>>>>
>>>>> I haven't checked myself, I believe your judgement,
>>>>
>>>> I took a better look and you obviously shouldn't believe :) The gianfar used
>>>> of_node instead of the fwnode. So, it'd be a single caller at starters.
>>>
>>> ...which is the same as dev_of_node(), which means that you can use your
>>> function there.
>>
>> I'm unsure what you mean. The proposed function
>> device_get_child_node_count_named() takes device pointer. I don't see how
>> dev_of_node() helps converting node to device?
> 
> dev_of_node() takes the device pointer and dev_fwnode() takes that as well,
> it means that there is no difference which one to use OF-centric or fwnode

The proposed device_get_child_node_count_named() takes a device pointer. 
I don't see how dev_of_node() helps if there is just of_node and no 
device pointer available in the calling code. (Well, as I wrote below, I 
could alter the gianfar code by dropping the gfar_of_group_count(), so 
that I have the device pointer in caller). Anyways, I don't see how 
dev_of_node() should help unless you're proposing I add a 
of_get_child_node_count_named() or somesuch - which I don't think makes 
sense.

> API in this particular case. Just make sure that the function (and there
> is also a second loop AFAICS) takes struct device *dev instead of struct
> device_node *np as a parameter.

I think I lost the track here :)

>> I think I could actually kill the whole gfar_of_group_count() function and
>> replace it with a direct call to the device_get_child_node_count_named() -
>> but I am not at all convinced that'd be worth including the property.h to a
>> file which is currently using only of_* -stuff. Well, I suppose it can be
>> asked from netdev peeps but I am not convinced they see it as a great idea.
>>
>> If I misunderstood your meaning - please elaborate.
> 
> The driver is quite old

I remember having to modify this driver somewhere around 2010 or so. :) 
Time flies.

> and has a lot of room to improve. Briefly looking it
> may be almost fully converted to fwnode, but it's not your call (only if you
> wish). Nevertheless, using agnostic APIs if they reduce code base is fine.
> We have drivers that do OF and fwnode mixed approach (for various reasons,
> one of which is the new API that is absent in OF realm.

Well, we can propose this to netdev people but I wouldn't be surprized 
if they requested full of_node => fwnode rewrite instead of removing 
simple looking loop and bringing mixture of fwnode and of_node in driver.

Yours,
	-- Matti