[PATCH 0/3] platform/surface: aggregator: Clean up client device removal

[PATCH 0/3] platform/surface: aggregator: Clean up client device removal

[Maximilian Luz]

Remove some duplicate code for Surface Aggregator client device removal and
rename a function for consistency.

Maximilian Luz (3):
  platform/surface: aggregator: Make client device removal more generic
  platform/surface: aggregator_registry: Use generic client removal
    function
  platform/surface: aggregator_registry: Rename device registration
    function

 drivers/platform/surface/aggregator/bus.c     | 24 +++++---------
 drivers/platform/surface/aggregator/bus.h     |  3 --
 drivers/platform/surface/aggregator/core.c    |  3 +-
 .../surface/surface_aggregator_registry.c     | 32 ++++++-------------
 include/linux/surface_aggregator/device.h     |  9 ++++++
 5 files changed, 28 insertions(+), 43 deletions(-)

-- 
2.33.1

[PATCH 1/3] platform/surface: aggregator: Make client device removal more generic

[Maximilian Luz]

Currently, there are similar functions defined in the Aggregator
Registry and the controller core.

Make client device removal more generic and export it. We can then use
this function later on to remove client devices from device hubs as well
as the controller and avoid re-defining similar things.

Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
---
 drivers/platform/surface/aggregator/bus.c  | 24 ++++++++--------------
 drivers/platform/surface/aggregator/bus.h  |  3 ---
 drivers/platform/surface/aggregator/core.c |  3 ++-
 include/linux/surface_aggregator/device.h  |  9 ++++++++
 4 files changed, 19 insertions(+), 20 deletions(-)

diff --git a/drivers/platform/surface/aggregator/bus.c b/drivers/platform/surface/aggregator/bus.c
index 0a40dd9c94ed..abbbb5b08b07 100644
--- a/drivers/platform/surface/aggregator/bus.c
+++ b/drivers/platform/surface/aggregator/bus.c
@@ -374,27 +374,19 @@ static int ssam_remove_device(struct device *dev, void *_data)
 }
 
 /**
- * ssam_controller_remove_clients() - Remove SSAM client devices registered as
- * direct children under the given controller.
- * @ctrl: The controller to remove all direct clients for.
+ * ssam_remove_clients() - Remove SSAM client devices registered as direct
+ * children under the given parent device.
+ * @dev: The (parent) device to remove all direct clients for.
  *
- * Remove all SSAM client devices registered as direct children under the
- * given controller. Note that this only accounts for direct children of the
- * controller device. This does not take care of any client devices where the
- * parent device has been manually set before calling ssam_device_add. Refer
- * to ssam_device_add()/ssam_device_remove() for more details on those cases.
- *
- * To avoid new devices being added in parallel to this call, the main
- * controller lock (not statelock) must be held during this (and if
- * necessary, any subsequent deinitialization) call.
+ * Remove all SSAM client devices registered as direct children under the given
+ * device. Note that this only accounts for direct children of the device.
+ * Refer to ssam_device_add()/ssam_device_remove() for more details.
  */
-void ssam_controller_remove_clients(struct ssam_controller *ctrl)
+void ssam_remove_clients(struct device *dev)
 {
-	struct device *dev;
-
-	dev = ssam_controller_device(ctrl);
 	device_for_each_child_reverse(dev, NULL, ssam_remove_device);
 }
+EXPORT_SYMBOL_GPL(ssam_remove_clients);
 
 /**
  * ssam_bus_register() - Register and set-up the SSAM client device bus.
diff --git a/drivers/platform/surface/aggregator/bus.h b/drivers/platform/surface/aggregator/bus.h
index ed032c2cbdb2..6964ee84e79c 100644
--- a/drivers/platform/surface/aggregator/bus.h
+++ b/drivers/platform/surface/aggregator/bus.h
@@ -12,14 +12,11 @@
 
 #ifdef CONFIG_SURFACE_AGGREGATOR_BUS
 
-void ssam_controller_remove_clients(struct ssam_controller *ctrl);
-
 int ssam_bus_register(void);
 void ssam_bus_unregister(void);
 
 #else /* CONFIG_SURFACE_AGGREGATOR_BUS */
 
-static inline void ssam_controller_remove_clients(struct ssam_controller *ctrl) {}
 static inline int ssam_bus_register(void) { return 0; }
 static inline void ssam_bus_unregister(void) {}
 
diff --git a/drivers/platform/surface/aggregator/core.c b/drivers/platform/surface/aggregator/core.c
index c61bbeeec2df..d384d36098c2 100644
--- a/drivers/platform/surface/aggregator/core.c
+++ b/drivers/platform/surface/aggregator/core.c
@@ -22,6 +22,7 @@
 #include <linux/sysfs.h>
 
 #include <linux/surface_aggregator/controller.h>
+#include <linux/surface_aggregator/device.h>
 
 #include "bus.h"
 #include "controller.h"
@@ -735,7 +736,7 @@ static void ssam_serial_hub_remove(struct serdev_device *serdev)
 	ssam_controller_lock(ctrl);
 
 	/* Remove all client devices. */
-	ssam_controller_remove_clients(ctrl);
+	ssam_remove_clients(&serdev->dev);
 
 	/* Act as if suspending to silence events. */
 	status = ssam_ctrl_notif_display_off(ctrl);
diff --git a/include/linux/surface_aggregator/device.h b/include/linux/surface_aggregator/device.h
index f636c5310321..cc257097eb05 100644
--- a/include/linux/surface_aggregator/device.h
+++ b/include/linux/surface_aggregator/device.h
@@ -319,6 +319,15 @@ void ssam_device_driver_unregister(struct ssam_device_driver *d);
 		      ssam_device_driver_unregister)
 
 
+/* -- Helpers for controller and hub devices. ------------------------------- */
+
+#ifdef CONFIG_SURFACE_AGGREGATOR_BUS
+void ssam_remove_clients(struct device *dev);
+#else /* CONFIG_SURFACE_AGGREGATOR_BUS */
+static inline void ssam_remove_clients(struct device *dev) {}
+#endif /* CONFIG_SURFACE_AGGREGATOR_BUS */
+
+
 /* -- Helpers for client-device requests. ----------------------------------- */
 
 /**
-- 
2.33.1

[PATCH 2/3] platform/surface: aggregator_registry: Use generic client removal function

[Maximilian Luz]

Use generic client removal function introduced in the previous commit
instead of defining our own one.

Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
---
 .../surface/surface_aggregator_registry.c     | 24 ++++---------------
 1 file changed, 5 insertions(+), 19 deletions(-)

diff --git a/drivers/platform/surface/surface_aggregator_registry.c b/drivers/platform/surface/surface_aggregator_registry.c
index 1679811eff50..d63f975bfd4c 100644
--- a/drivers/platform/surface/surface_aggregator_registry.c
+++ b/drivers/platform/surface/surface_aggregator_registry.c
@@ -249,20 +249,6 @@ static int ssam_uid_from_string(const char *str, struct ssam_device_uid *uid)
 	return 0;
 }
 
-static int ssam_hub_remove_devices_fn(struct device *dev, void *data)
-{
-	if (!is_ssam_device(dev))
-		return 0;
-
-	ssam_device_remove(to_ssam_device(dev));
-	return 0;
-}
-
-static void ssam_hub_remove_devices(struct device *parent)
-{
-	device_for_each_child_reverse(parent, NULL, ssam_hub_remove_devices_fn);
-}
-
 static int ssam_hub_add_device(struct device *parent, struct ssam_controller *ctrl,
 			       struct fwnode_handle *node)
 {
@@ -308,7 +294,7 @@ static int ssam_hub_add_devices(struct device *parent, struct ssam_controller *c
 
 	return 0;
 err:
-	ssam_hub_remove_devices(parent);
+	ssam_remove_clients(parent);
 	return status;
 }
 
@@ -405,7 +391,7 @@ static void ssam_base_hub_update_workfn(struct work_struct *work)
 	if (hub->state == SSAM_BASE_HUB_CONNECTED)
 		status = ssam_hub_add_devices(&hub->sdev->dev, hub->sdev->ctrl, node);
 	else
-		ssam_hub_remove_devices(&hub->sdev->dev);
+		ssam_remove_clients(&hub->sdev->dev);
 
 	if (status)
 		dev_err(&hub->sdev->dev, "failed to update base-hub devices: %d\n", status);
@@ -487,7 +473,7 @@ static int ssam_base_hub_probe(struct ssam_device *sdev)
 err:
 	ssam_notifier_unregister(sdev->ctrl, &hub->notif);
 	cancel_delayed_work_sync(&hub->update_work);
-	ssam_hub_remove_devices(&sdev->dev);
+	ssam_remove_clients(&sdev->dev);
 	return status;
 }
 
@@ -499,7 +485,7 @@ static void ssam_base_hub_remove(struct ssam_device *sdev)
 
 	ssam_notifier_unregister(sdev->ctrl, &hub->notif);
 	cancel_delayed_work_sync(&hub->update_work);
-	ssam_hub_remove_devices(&sdev->dev);
+	ssam_remove_clients(&sdev->dev);
 }
 
 static const struct ssam_device_id ssam_base_hub_match[] = {
@@ -613,7 +599,7 @@ static int ssam_platform_hub_remove(struct platform_device *pdev)
 {
 	const struct software_node **nodes = platform_get_drvdata(pdev);
 
-	ssam_hub_remove_devices(&pdev->dev);
+	ssam_remove_clients(&pdev->dev);
 	set_secondary_fwnode(&pdev->dev, NULL);
 	software_node_unregister_node_group(nodes);
 	return 0;
-- 
2.33.1

[PATCH 3/3] platform/surface: aggregator_registry: Rename device registration function

[Maximilian Luz]

Rename the device registration function to better align names with the
newly introduced device removal function.

Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
---
 drivers/platform/surface/surface_aggregator_registry.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/surface/surface_aggregator_registry.c b/drivers/platform/surface/surface_aggregator_registry.c
index d63f975bfd4c..2e0d3a808d47 100644
--- a/drivers/platform/surface/surface_aggregator_registry.c
+++ b/drivers/platform/surface/surface_aggregator_registry.c
@@ -274,8 +274,8 @@ static int ssam_hub_add_device(struct device *parent, struct ssam_controller *ct
 	return status;
 }
 
-static int ssam_hub_add_devices(struct device *parent, struct ssam_controller *ctrl,
-				struct fwnode_handle *node)
+static int ssam_hub_register_clients(struct device *parent, struct ssam_controller *ctrl,
+				     struct fwnode_handle *node)
 {
 	struct fwnode_handle *child;
 	int status;
@@ -389,7 +389,7 @@ static void ssam_base_hub_update_workfn(struct work_struct *work)
 	hub->state = state;
 
 	if (hub->state == SSAM_BASE_HUB_CONNECTED)
-		status = ssam_hub_add_devices(&hub->sdev->dev, hub->sdev->ctrl, node);
+		status = ssam_hub_register_clients(&hub->sdev->dev, hub->sdev->ctrl, node);
 	else
 		ssam_remove_clients(&hub->sdev->dev);
 
@@ -585,7 +585,7 @@ static int ssam_platform_hub_probe(struct platform_device *pdev)
 
 	set_secondary_fwnode(&pdev->dev, root);
 
-	status = ssam_hub_add_devices(&pdev->dev, ctrl, root);
+	status = ssam_hub_register_clients(&pdev->dev, ctrl, root);
 	if (status) {
 		set_secondary_fwnode(&pdev->dev, NULL);
 		software_node_unregister_node_group(nodes);
-- 
2.33.1

Re: [PATCH 0/3] platform/surface: aggregator: Clean up client device removal

[Hans de Goede]

Hi,

On 10/28/21 02:22, Maximilian Luz wrote:
> Remove some duplicate code for Surface Aggregator client device removal and
> rename a function for consistency.
> 
> Maximilian Luz (3):
>   platform/surface: aggregator: Make client device removal more generic
>   platform/surface: aggregator_registry: Use generic client removal
>     function
>   platform/surface: aggregator_registry: Rename device registration
>     function

Thanks, the series looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

I'll merge this once 5.16-rc1 is out.

Regards,

Hans