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