Re: [PATCH 1/6] software node: Provide replacement for device_add_properties()

[Heikki Krogerus]

On Wed, Feb 03, 2021 at 02:50:24PM +0100, Rafael J. Wysocki wrote:
> On Wed, Feb 3, 2021 at 10:45 AM Heikki Krogerus
> <heikki.krogerus@xxxxxxxxxxxxxxx> wrote:
> >
> > On Tue, Feb 02, 2021 at 05:08:40PM +0100, Rafael J. Wysocki wrote:
> > > It looks like there is a use case that cannot be addressed by using
> > > device_add_properties() and that's why you need this new function.
> > >
> > > Can you describe that use case, please, and explain what the problem
> > > with using device_add_properties() in it is?
> >
> > The problem with device_add_properties() is that it gives false
> > impression that the device properties are somehow directly attached to
> > the devices, which is not true. Now, that should not be a major issue,
> > but it seems that it is. I think Lee Jones basically used that as an
> > argument to refuse changes (and pretty minor changes) that would have
> > allowed us to use software nodes with the MFD drivers.
> >
> > Nevertheless, I was not planning to provide a replacement for it
> > originally. We do in any case have the real issue caused by that
> > device_remove_properties() call in device_del() which has to be fixed.
> 
> What's that issue, specifically?

The problem is that we can't now reuse or share if necessary, or just
in general be in charge of the lifetime of the software nodes because
that call is in device_del(). Now the lifetime of the software nodes
is always tied to the devices they are attached, no questions asked.

> > I started to fix that by moving device_add_properties() under
> > drivers/base/swnode.c so I can implement it like I've implemented now
> > that new function, but after that when I started to tackle the second
> > issue by removing the subsystem wrappers like
> > platform_device_add_device_properties() and replacing them with things
> > like platform_device_add_software_node() in order to give the drivers
> > a chance to actually use software nodes, I realised that there isn't
> > much use for device_add_properties() after that.
> >
> > Also, even though I'm not super happy about adding more API to this
> > thing, this function - device_create_managed_software_node() - I do
> > like. Not only is it implemented so that we don't have to rely on
> > anything else in drivers core in order to achieve the lifetime link
> > with the device, it is also much more descriptive. The function name
> > alone does not leave any questions about what is going to happen if
> > that function is called. You'll end up with a software node that just
> > happens to be attached to the device.
> >
> > On top of those two things, by adding the new function it also gives
> > me a nice stepping stone to do these changes in the normal stages:
> >
> >         1. Add feature/modification.
> >         2. Convert users.
> >         3. Cleanup.
> >
> > And finally, even though we may not have any users left for
> > device_add_properties() after I have updated all the subsystems and
> > drivers, this new function will still be useful. That is because, even
> > though it can be used as a drop-in replacement for
> > device_add_properties(), it does add that one small but important
> > change. It allows the nodes created with it to be part of node
> > hierarchy, and that alone is useful to me, and I'm planning on using
> > that feature later.
> >
> > I'm sorry about the long explanation.
> 
> No need to be sorry,  now I know what this really is about. :-)
> 
> I'm not against this patch, but I IMO the "motivation" part of the
> changelog needs to be improved.
> 
> If the final goal is to get rid of device_add_properties(), please
> spell it out and say what problems there are with it and why the new
> function will be better.

Sure thing. Thanks Rafael.


Br,

-- 
heikki