diff mbox series

[v1,5/6] software nodes: Split software_node_notify()

Message ID 5627033.MhkbZ0Pkbq@kreacher
State New
Headers show
Series [v1,1/6] ACPI: glue: Rearrange acpi_device_notify() | expand

Commit Message

Rafael J. Wysocki July 12, 2021, 5:27 p.m. UTC
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(-)

Comments

Greg Kroah-Hartman July 12, 2021, 6:03 p.m. UTC | #1
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
Rafael J. Wysocki July 12, 2021, 6:30 p.m. UTC | #2
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!
Rafael J. Wysocki July 12, 2021, 7:04 p.m. UTC | #3
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!
Heikki Krogerus July 13, 2021, 7:46 a.m. UTC | #4
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
Rafael J. Wysocki July 14, 2021, 6:17 p.m. UTC | #5
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!
diff mbox series

Patch

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