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

Rafael J. Wysocki
2021-02-03T13:52:00Z

On Wed, Feb 3, 2021 at 10:45 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> 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?

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