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

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

On Thu, Feb 27, 2025 at 10:01:49AM +0200, Matti Vaittinen wrote:
> 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.

???

The loops are working on

	struct device_node *np = pdev->dev.np;

which is the equivalent to

	struct device_node *np = dev_of_node(&pdev->dev);

which takes device pointer.

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

Are you forbidding yourself to change the function prototype to take a device
pointer instead of device_node one? :-)

> > 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 :)

Make gfar_of_group_count() to take device pointer. As simple as that.

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

Without doing the proposal we will never know what they will think of all
this...

-- 
With Best Regards,
Andy Shevchenko