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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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
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 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 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.

-- 
With Best Regards,
Andy Shevchenko