| Message ID | 5627033.MhkbZ0Pkbq@kreacher |
|---|---|
| State | New |
| Headers | show |
| Series | [v1,1/6] ACPI: glue: Rearrange acpi_device_notify() | expand |
On Mon, Jul 12, 2021 at 07:27:12PM +0200, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Split software_node_notify_remove) out of software_node_notify() > and make device_platform_notify() call the latter on device addition > and the former on device removal. > > While at it, put the headers of the above functions into base.h, > because they don't need to be present in a global header file. > > No intentional functional impact. > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > drivers/base/base.h | 3 ++ > drivers/base/core.c | 9 +++--- > drivers/base/swnode.c | 61 ++++++++++++++++++++++++----------------------- > include/linux/property.h | 2 - > 4 files changed, 39 insertions(+), 36 deletions(-) > > Index: linux-pm/drivers/base/swnode.c > =================================================================== > --- linux-pm.orig/drivers/base/swnode.c > +++ linux-pm/drivers/base/swnode.c > @@ -11,6 +11,8 @@ > #include <linux/property.h> > #include <linux/slab.h> > > +#include "base.h" > + > struct swnode { > struct kobject kobj; > struct fwnode_handle fwnode; > @@ -1053,7 +1055,7 @@ int device_add_software_node(struct devi > * balance. > */ > if (device_is_registered(dev)) > - software_node_notify(dev, KOBJ_ADD); > + software_node_notify(dev); Should this now be called "software_node_notify_add()" to match up with: > if (device_is_registered(dev)) > - software_node_notify(dev, KOBJ_REMOVE); > + software_node_notify_remove(dev); The other being called "_remove"? Makes it more obvious to me :) thanks, greg k-h
On Mon, Jul 12, 2021 at 8:03 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Mon, Jul 12, 2021 at 07:27:12PM +0200, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > Split software_node_notify_remove) out of software_node_notify() > > and make device_platform_notify() call the latter on device addition > > and the former on device removal. > > > > While at it, put the headers of the above functions into base.h, > > because they don't need to be present in a global header file. > > > > No intentional functional impact. > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > --- > > drivers/base/base.h | 3 ++ > > drivers/base/core.c | 9 +++--- > > drivers/base/swnode.c | 61 ++++++++++++++++++++++++----------------------- > > include/linux/property.h | 2 - > > 4 files changed, 39 insertions(+), 36 deletions(-) > > > > Index: linux-pm/drivers/base/swnode.c > > =================================================================== > > --- linux-pm.orig/drivers/base/swnode.c > > +++ linux-pm/drivers/base/swnode.c > > @@ -11,6 +11,8 @@ > > #include <linux/property.h> > > #include <linux/slab.h> > > > > +#include "base.h" > > + > > struct swnode { > > struct kobject kobj; > > struct fwnode_handle fwnode; > > @@ -1053,7 +1055,7 @@ int device_add_software_node(struct devi > > * balance. > > */ > > if (device_is_registered(dev)) > > - software_node_notify(dev, KOBJ_ADD); > > + software_node_notify(dev); > > Should this now be called "software_node_notify_add()" to match up with: > > > if (device_is_registered(dev)) > > - software_node_notify(dev, KOBJ_REMOVE); > > + software_node_notify_remove(dev); > > The other being called "_remove"? > > Makes it more obvious to me :) The naming convention used here follows platform_notify() and platform_notify_remove(), and the analogous function names in ACPI for that matter. I thought that adding _add in just one case would be sort of odd, but of course I can do that, so please let me know what you want me to do. Cheers!
On Mon, Jul 12, 2021 at 8:57 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Mon, Jul 12, 2021 at 08:30:06PM +0200, Rafael J. Wysocki wrote: > > On Mon, Jul 12, 2021 at 8:03 PM Greg Kroah-Hartman > > <gregkh@linuxfoundation.org> wrote: > > > > > > On Mon, Jul 12, 2021 at 07:27:12PM +0200, Rafael J. Wysocki wrote: > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > > > Split software_node_notify_remove) out of software_node_notify() > > > > and make device_platform_notify() call the latter on device addition > > > > and the former on device removal. > > > > > > > > While at it, put the headers of the above functions into base.h, > > > > because they don't need to be present in a global header file. > > > > > > > > No intentional functional impact. > > > > > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > --- > > > > drivers/base/base.h | 3 ++ > > > > drivers/base/core.c | 9 +++--- > > > > drivers/base/swnode.c | 61 ++++++++++++++++++++++++----------------------- > > > > include/linux/property.h | 2 - > > > > 4 files changed, 39 insertions(+), 36 deletions(-) > > > > > > > > Index: linux-pm/drivers/base/swnode.c > > > > =================================================================== > > > > --- linux-pm.orig/drivers/base/swnode.c > > > > +++ linux-pm/drivers/base/swnode.c > > > > @@ -11,6 +11,8 @@ > > > > #include <linux/property.h> > > > > #include <linux/slab.h> > > > > > > > > +#include "base.h" > > > > + > > > > struct swnode { > > > > struct kobject kobj; > > > > struct fwnode_handle fwnode; > > > > @@ -1053,7 +1055,7 @@ int device_add_software_node(struct devi > > > > * balance. > > > > */ > > > > if (device_is_registered(dev)) > > > > - software_node_notify(dev, KOBJ_ADD); > > > > + software_node_notify(dev); > > > > > > Should this now be called "software_node_notify_add()" to match up with: > > > > > > > if (device_is_registered(dev)) > > > > - software_node_notify(dev, KOBJ_REMOVE); > > > > + software_node_notify_remove(dev); > > > > > > The other being called "_remove"? > > > > > > Makes it more obvious to me :) > > > > The naming convention used here follows platform_notify() and > > platform_notify_remove(), and the analogous function names in ACPI for > > that matter. > > > > I thought that adding _add in just one case would be sort of odd, but > > of course I can do that, so please let me know what you want me to do. > > Ah, ok, that makes more sense, let's just leave it as-is then: > > Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Thanks!
On Mon, Jul 12, 2021 at 08:30:06PM +0200, Rafael J. Wysocki wrote: > On Mon, Jul 12, 2021 at 8:03 PM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > > On Mon, Jul 12, 2021 at 07:27:12PM +0200, Rafael J. Wysocki wrote: > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > Split software_node_notify_remove) out of software_node_notify() > > > and make device_platform_notify() call the latter on device addition > > > and the former on device removal. > > > > > > While at it, put the headers of the above functions into base.h, > > > because they don't need to be present in a global header file. > > > > > > No intentional functional impact. > > > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > --- > > > drivers/base/base.h | 3 ++ > > > drivers/base/core.c | 9 +++--- > > > drivers/base/swnode.c | 61 ++++++++++++++++++++++++----------------------- > > > include/linux/property.h | 2 - > > > 4 files changed, 39 insertions(+), 36 deletions(-) > > > > > > Index: linux-pm/drivers/base/swnode.c > > > =================================================================== > > > --- linux-pm.orig/drivers/base/swnode.c > > > +++ linux-pm/drivers/base/swnode.c > > > @@ -11,6 +11,8 @@ > > > #include <linux/property.h> > > > #include <linux/slab.h> > > > > > > +#include "base.h" > > > + > > > struct swnode { > > > struct kobject kobj; > > > struct fwnode_handle fwnode; > > > @@ -1053,7 +1055,7 @@ int device_add_software_node(struct devi > > > * balance. > > > */ > > > if (device_is_registered(dev)) > > > - software_node_notify(dev, KOBJ_ADD); > > > + software_node_notify(dev); > > > > Should this now be called "software_node_notify_add()" to match up with: > > > > > if (device_is_registered(dev)) > > > - software_node_notify(dev, KOBJ_REMOVE); > > > + software_node_notify_remove(dev); > > > > The other being called "_remove"? > > > > Makes it more obvious to me :) > > The naming convention used here follows platform_notify() and > platform_notify_remove(), and the analogous function names in ACPI for > that matter. So why not rename those instead: platform_notify() to platform_notify_add() and so on? You are in any case modifying acpi_device_notify() in this series, and I think there is only one place left where .platform_notify is assigned. I believe you also wouldn't then need to worry about the function name collision (3/6). > I thought that adding _add in just one case would be sort of odd, but > of course I can do that, so please let me know what you want me to do. I would prefer the "_add" ending, but in any case, FWIW: Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> -- heikki
On Tue, Jul 13, 2021 at 9:46 AM Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote: > > On Mon, Jul 12, 2021 at 08:30:06PM +0200, Rafael J. Wysocki wrote: > > On Mon, Jul 12, 2021 at 8:03 PM Greg Kroah-Hartman > > <gregkh@linuxfoundation.org> wrote: > > > > > > On Mon, Jul 12, 2021 at 07:27:12PM +0200, Rafael J. Wysocki wrote: > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > > > Split software_node_notify_remove) out of software_node_notify() > > > > and make device_platform_notify() call the latter on device addition > > > > and the former on device removal. > > > > > > > > While at it, put the headers of the above functions into base.h, > > > > because they don't need to be present in a global header file. > > > > > > > > No intentional functional impact. > > > > > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > --- > > > > drivers/base/base.h | 3 ++ > > > > drivers/base/core.c | 9 +++--- > > > > drivers/base/swnode.c | 61 ++++++++++++++++++++++++----------------------- > > > > include/linux/property.h | 2 - > > > > 4 files changed, 39 insertions(+), 36 deletions(-) > > > > > > > > Index: linux-pm/drivers/base/swnode.c > > > > =================================================================== > > > > --- linux-pm.orig/drivers/base/swnode.c > > > > +++ linux-pm/drivers/base/swnode.c > > > > @@ -11,6 +11,8 @@ > > > > #include <linux/property.h> > > > > #include <linux/slab.h> > > > > > > > > +#include "base.h" > > > > + > > > > struct swnode { > > > > struct kobject kobj; > > > > struct fwnode_handle fwnode; > > > > @@ -1053,7 +1055,7 @@ int device_add_software_node(struct devi > > > > * balance. > > > > */ > > > > if (device_is_registered(dev)) > > > > - software_node_notify(dev, KOBJ_ADD); > > > > + software_node_notify(dev); > > > > > > Should this now be called "software_node_notify_add()" to match up with: > > > > > > > if (device_is_registered(dev)) > > > > - software_node_notify(dev, KOBJ_REMOVE); > > > > + software_node_notify_remove(dev); > > > > > > The other being called "_remove"? > > > > > > Makes it more obvious to me :) > > > > The naming convention used here follows platform_notify() and > > platform_notify_remove(), and the analogous function names in ACPI for > > that matter. > > So why not rename those instead: platform_notify() to > platform_notify_add() and so on? You are in any case modifying > acpi_device_notify() in this series, and I think there is only one > place left where .platform_notify is assigned. I believe you also > wouldn't then need to worry about the function name collision (3/6). > > > I thought that adding _add in just one case would be sort of odd, but > > of course I can do that, so please let me know what you want me to do. > > I would prefer the "_add" ending, but in any case, FWIW: > > Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> Thanks!
Index: linux-pm/drivers/base/swnode.c =================================================================== --- linux-pm.orig/drivers/base/swnode.c +++ linux-pm/drivers/base/swnode.c @@ -11,6 +11,8 @@ #include <linux/property.h> #include <linux/slab.h> +#include "base.h" + struct swnode { struct kobject kobj; struct fwnode_handle fwnode; @@ -1053,7 +1055,7 @@ int device_add_software_node(struct devi * balance. */ if (device_is_registered(dev)) - software_node_notify(dev, KOBJ_ADD); + software_node_notify(dev); return 0; } @@ -1074,7 +1076,8 @@ void device_remove_software_node(struct return; if (device_is_registered(dev)) - software_node_notify(dev, KOBJ_REMOVE); + software_node_notify_remove(dev); + set_secondary_fwnode(dev, NULL); kobject_put(&swnode->kobj); } @@ -1117,44 +1120,44 @@ int device_create_managed_software_node( } EXPORT_SYMBOL_GPL(device_create_managed_software_node); -int software_node_notify(struct device *dev, unsigned long action) +void software_node_notify(struct device *dev) { struct swnode *swnode; int ret; swnode = dev_to_swnode(dev); if (!swnode) - return 0; + return; - switch (action) { - case KOBJ_ADD: - ret = sysfs_create_link(&dev->kobj, &swnode->kobj, "software_node"); - if (ret) - break; + ret = sysfs_create_link(&dev->kobj, &swnode->kobj, "software_node"); + if (ret) + return; - ret = sysfs_create_link(&swnode->kobj, &dev->kobj, - dev_name(dev)); - if (ret) { - sysfs_remove_link(&dev->kobj, "software_node"); - break; - } - kobject_get(&swnode->kobj); - break; - case KOBJ_REMOVE: - sysfs_remove_link(&swnode->kobj, dev_name(dev)); + ret = sysfs_create_link(&swnode->kobj, &dev->kobj, dev_name(dev)); + if (ret) { sysfs_remove_link(&dev->kobj, "software_node"); - kobject_put(&swnode->kobj); - - if (swnode->managed) { - set_secondary_fwnode(dev, NULL); - kobject_put(&swnode->kobj); - } - break; - default: - break; + return; } - return 0; + kobject_get(&swnode->kobj); +} + +void software_node_notify_remove(struct device *dev) +{ + struct swnode *swnode; + + swnode = dev_to_swnode(dev); + if (!swnode) + return; + + sysfs_remove_link(&swnode->kobj, dev_name(dev)); + sysfs_remove_link(&dev->kobj, "software_node"); + kobject_put(&swnode->kobj); + + if (swnode->managed) { + set_secondary_fwnode(dev, NULL); + kobject_put(&swnode->kobj); + } } static int __init software_node_init(void) Index: linux-pm/include/linux/property.h =================================================================== --- linux-pm.orig/include/linux/property.h +++ linux-pm/include/linux/property.h @@ -484,8 +484,6 @@ void software_node_unregister_node_group int software_node_register(const struct software_node *node); void software_node_unregister(const struct software_node *node); -int software_node_notify(struct device *dev, unsigned long action); - struct fwnode_handle * fwnode_create_software_node(const struct property_entry *properties, const struct fwnode_handle *parent); Index: linux-pm/drivers/base/core.c =================================================================== --- linux-pm.orig/drivers/base/core.c +++ linux-pm/drivers/base/core.c @@ -2003,16 +2003,15 @@ static inline int device_is_not_partitio static int device_platform_notify(struct device *dev, enum kobject_action action) { - int ret; - if (action == KOBJ_ADD) acpi_device_notify(dev); else if (action == KOBJ_REMOVE) acpi_device_notify_remove(dev); - ret = software_node_notify(dev, action); - if (ret) - return ret; + if (action == KOBJ_ADD) + software_node_notify(dev); + else if (action == KOBJ_REMOVE) + software_node_notify_remove(dev); if (platform_notify && action == KOBJ_ADD) platform_notify(dev); Index: linux-pm/drivers/base/base.h =================================================================== --- linux-pm.orig/drivers/base/base.h +++ linux-pm/drivers/base/base.h @@ -202,3 +202,6 @@ int devtmpfs_delete_node(struct device * static inline int devtmpfs_create_node(struct device *dev) { return 0; } static inline int devtmpfs_delete_node(struct device *dev) { return 0; } #endif + +void software_node_notify(struct device *dev); +void software_node_notify_remove(struct device *dev);