[PATCH] soundwire: intel: move to auxiliary bus

[PATCH] soundwire: intel: move to auxiliary bus

[Bard Liao]

From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Now that the auxiliary_bus exists, there's no reason to use platform
devices as children of a PCI device any longer.

This patch refactors the code by extending a basic auxiliary device
with Intel link-specific structures that need to be passed between
controller and link levels. This refactoring is much cleaner with no
need for cross-pointers between device and link structures.

Note that the auxiliary bus API has separate init and add steps, which
requires more attention in the error unwinding paths. The main loop
needs to deal with kfree() and auxiliary_device_uninit() for the
current iteration before jumping to the common label which releases
everything allocated in prior iterations.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
---
 drivers/soundwire/Kconfig           |   1 +
 drivers/soundwire/intel.c           |  52 ++++----
 drivers/soundwire/intel.h           |  14 +-
 drivers/soundwire/intel_init.c      | 190 +++++++++++++++++++---------
 include/linux/soundwire/sdw_intel.h |   6 +-
 5 files changed, 175 insertions(+), 88 deletions(-)

diff --git a/drivers/soundwire/Kconfig b/drivers/soundwire/Kconfig
index 016e74230bb7..2b7795233282 100644
--- a/drivers/soundwire/Kconfig
+++ b/drivers/soundwire/Kconfig
@@ -25,6 +25,7 @@ config SOUNDWIRE_INTEL
 	tristate "Intel SoundWire Master driver"
 	select SOUNDWIRE_CADENCE
 	select SOUNDWIRE_GENERIC_ALLOCATION
+	select AUXILIARY_BUS
 	depends on ACPI && SND_SOC
 	help
 	  SoundWire Intel Master driver.
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index d2254ee2fee2..039a101982c9 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -11,7 +11,7 @@
 #include <linux/module.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
-#include <linux/platform_device.h>
+#include <linux/auxiliary_bus.h>
 #include <sound/pcm_params.h>
 #include <linux/pm_runtime.h>
 #include <sound/soc.h>
@@ -1331,9 +1331,10 @@ static int intel_init(struct sdw_intel *sdw)
 /*
  * probe and init
  */
-static int intel_master_probe(struct platform_device *pdev)
+static int intel_link_probe(struct auxiliary_device *auxdev, const struct auxiliary_device_id *id)
 {
-	struct device *dev = &pdev->dev;
+	struct device *dev = &auxdev->dev;
+	struct sdw_intel_link_dev *ldev = auxiliary_dev_to_sdw_intel_link_dev(auxdev);
 	struct sdw_intel *sdw;
 	struct sdw_cdns *cdns;
 	struct sdw_bus *bus;
@@ -1346,14 +1347,14 @@ static int intel_master_probe(struct platform_device *pdev)
 	cdns = &sdw->cdns;
 	bus = &cdns->bus;
 
-	sdw->instance = pdev->id;
-	sdw->link_res = dev_get_platdata(dev);
+	sdw->instance = auxdev->id;
+	sdw->link_res = &ldev->link_res;
 	cdns->dev = dev;
 	cdns->registers = sdw->link_res->registers;
 	cdns->instance = sdw->instance;
 	cdns->msg_count = 0;
 
-	bus->link_id = pdev->id;
+	bus->link_id = auxdev->id;
 
 	sdw_cdns_probe(cdns);
 
@@ -1386,10 +1387,10 @@ static int intel_master_probe(struct platform_device *pdev)
 	return 0;
 }
 
-int intel_master_startup(struct platform_device *pdev)
+int intel_link_startup(struct auxiliary_device *auxdev)
 {
 	struct sdw_cdns_stream_config config;
-	struct device *dev = &pdev->dev;
+	struct device *dev = &auxdev->dev;
 	struct sdw_cdns *cdns = dev_get_drvdata(dev);
 	struct sdw_intel *sdw = cdns_to_intel(cdns);
 	struct sdw_bus *bus = &cdns->bus;
@@ -1526,9 +1527,9 @@ int intel_master_startup(struct platform_device *pdev)
 	return ret;
 }
 
-static int intel_master_remove(struct platform_device *pdev)
+static void intel_link_remove(struct auxiliary_device *auxdev)
 {
-	struct device *dev = &pdev->dev;
+	struct device *dev = &auxdev->dev;
 	struct sdw_cdns *cdns = dev_get_drvdata(dev);
 	struct sdw_intel *sdw = cdns_to_intel(cdns);
 	struct sdw_bus *bus = &cdns->bus;
@@ -1544,19 +1545,17 @@ static int intel_master_remove(struct platform_device *pdev)
 		snd_soc_unregister_component(dev);
 	}
 	sdw_bus_master_delete(bus);
-
-	return 0;
 }
 
-int intel_master_process_wakeen_event(struct platform_device *pdev)
+int intel_link_process_wakeen_event(struct auxiliary_device *auxdev)
 {
-	struct device *dev = &pdev->dev;
+	struct device *dev = &auxdev->dev;
 	struct sdw_intel *sdw;
 	struct sdw_bus *bus;
 	void __iomem *shim;
 	u16 wake_sts;
 
-	sdw = platform_get_drvdata(pdev);
+	sdw = dev_get_drvdata(dev);
 	bus = &sdw->cdns.bus;
 
 	if (bus->prop.hw_disabled) {
@@ -1978,17 +1977,22 @@ static const struct dev_pm_ops intel_pm = {
 	SET_RUNTIME_PM_OPS(intel_suspend_runtime, intel_resume_runtime, NULL)
 };
 
-static struct platform_driver sdw_intel_drv = {
-	.probe = intel_master_probe,
-	.remove = intel_master_remove,
+static const struct auxiliary_device_id intel_link_id_table[] = {
+	{ .name = "soundwire_intel.link" },
+	{},
+};
+MODULE_DEVICE_TABLE(auxiliary, intel_link_id_table);
+
+static struct auxiliary_driver sdw_intel_drv = {
+	.probe = intel_link_probe,
+	.remove = intel_link_remove,
 	.driver = {
-		.name = "intel-sdw",
+		/* auxiliary_driver_register() sets .name to be the modname */
 		.pm = &intel_pm,
-	}
+	},
+	.id_table = intel_link_id_table
 };
-
-module_platform_driver(sdw_intel_drv);
+module_auxiliary_driver(sdw_intel_drv);
 
 MODULE_LICENSE("Dual BSD/GPL");
-MODULE_ALIAS("platform:intel-sdw");
-MODULE_DESCRIPTION("Intel Soundwire Master Driver");
+MODULE_DESCRIPTION("Intel Soundwire Link Driver");
diff --git a/drivers/soundwire/intel.h b/drivers/soundwire/intel.h
index 06bac8ba14e9..0b47b148da3f 100644
--- a/drivers/soundwire/intel.h
+++ b/drivers/soundwire/intel.h
@@ -7,7 +7,6 @@
 /**
  * struct sdw_intel_link_res - Soundwire Intel link resource structure,
  * typically populated by the controller driver.
- * @pdev: platform_device
  * @mmio_base: mmio base of SoundWire registers
  * @registers: Link IO registers base
  * @shim: Audio shim pointer
@@ -23,7 +22,6 @@
  * @list: used to walk-through all masters exposed by the same controller
  */
 struct sdw_intel_link_res {
-	struct platform_device *pdev;
 	void __iomem *mmio_base; /* not strictly needed, useful for debug */
 	void __iomem *registers;
 	void __iomem *shim;
@@ -48,7 +46,15 @@ struct sdw_intel {
 #endif
 };
 
-int intel_master_startup(struct platform_device *pdev);
-int intel_master_process_wakeen_event(struct platform_device *pdev);
+int intel_link_startup(struct auxiliary_device *auxdev);
+int intel_link_process_wakeen_event(struct auxiliary_device *auxdev);
+
+struct sdw_intel_link_dev {
+	struct auxiliary_device auxdev;
+	struct sdw_intel_link_res link_res;
+};
+
+#define auxiliary_dev_to_sdw_intel_link_dev(auxiliary_dev) \
+	container_of(auxiliary_dev, struct sdw_intel_link_dev, auxdev)
 
 #endif /* __SDW_INTEL_LOCAL_H */
diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c
index 05b726cdfebc..3cb74060ccae 100644
--- a/drivers/soundwire/intel_init.c
+++ b/drivers/soundwire/intel_init.c
@@ -12,7 +12,7 @@
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/module.h>
-#include <linux/platform_device.h>
+#include <linux/auxiliary_bus.h>
 #include <linux/pm_runtime.h>
 #include <linux/soundwire/sdw_intel.h>
 #include "cadence_master.h"
@@ -24,28 +24,65 @@
 #define SDW_LINK_BASE		0x30000
 #define SDW_LINK_SIZE		0x10000
 
+static void intel_link_auxdev_release(struct device *dev)
+{
+	struct auxiliary_device *auxdev = to_auxiliary_dev(dev);
+	struct sdw_intel_link_dev *ldev = auxiliary_dev_to_sdw_intel_link_dev(auxdev);
+
+	kfree(ldev);
+}
+
+static int intel_link_dev_init(struct sdw_intel_link_dev *ldev,
+			       struct device *parent,
+			       struct fwnode_handle *fwnode,
+			       const char *name,
+			       int link_id)
+{
+	struct auxiliary_device *auxdev;
+	int ret;
+
+	auxdev = &ldev->auxdev;
+	auxdev->name = name;
+	auxdev->dev.parent = parent;
+	auxdev->dev.fwnode = fwnode;
+	auxdev->dev.release = intel_link_auxdev_release;
+
+	/* we don't use an IDA since we already have a link ID */
+	auxdev->id = link_id;
+
+	ret = auxiliary_device_init(auxdev);
+	if (ret < 0)
+		dev_err(parent, "failed to initialize link dev %s link_id %d\n",
+			name, link_id);
+
+	return ret;
+}
+
+static void intel_link_dev_unregister(struct sdw_intel_link_dev *ldev)
+{
+	auxiliary_device_delete(&ldev->auxdev);
+	auxiliary_device_uninit(&ldev->auxdev);
+}
+
 static int sdw_intel_cleanup(struct sdw_intel_ctx *ctx)
 {
-	struct sdw_intel_link_res *link = ctx->links;
+	struct sdw_intel_link_dev *ldev;
 	u32 link_mask;
 	int i;
 
-	if (!link)
-		return 0;
-
 	link_mask = ctx->link_mask;
 
-	for (i = 0; i < ctx->count; i++, link++) {
+	for (i = 0; i < ctx->count; i++) {
 		if (!(link_mask & BIT(i)))
 			continue;
 
-		if (link->pdev) {
-			pm_runtime_disable(&link->pdev->dev);
-			platform_device_unregister(link->pdev);
-		}
+		ldev = ctx->ldev[i];
 
-		if (!link->clock_stop_quirks)
-			pm_runtime_put_noidle(link->dev);
+		pm_runtime_disable(&ldev->auxdev.dev);
+		intel_link_dev_unregister(ldev);
+
+		if (!ldev->link_res.clock_stop_quirks)
+			pm_runtime_put_noidle(ldev->link_res.dev);
 	}
 
 	return 0;
@@ -91,9 +128,8 @@ EXPORT_SYMBOL_NS(sdw_intel_thread, SOUNDWIRE_INTEL_INIT);
 static struct sdw_intel_ctx
 *sdw_intel_probe_controller(struct sdw_intel_res *res)
 {
-	struct platform_device_info pdevinfo;
-	struct platform_device *pdev;
 	struct sdw_intel_link_res *link;
+	struct sdw_intel_link_dev *ldev;
 	struct sdw_intel_ctx *ctx;
 	struct acpi_device *adev;
 	struct sdw_slave *slave;
@@ -103,6 +139,7 @@ static struct sdw_intel_ctx
 	int num_slaves = 0;
 	int count;
 	int i;
+	int ret;
 
 	if (!res)
 		return NULL;
@@ -116,35 +153,72 @@ static struct sdw_intel_ctx
 	count = res->count;
 	dev_dbg(&adev->dev, "Creating %d SDW Link devices\n", count);
 
-	ctx = devm_kzalloc(&adev->dev, sizeof(*ctx), GFP_KERNEL);
+	/*
+	 * we need to alloc/free memory manually and can't use devm:
+	 * this routine may be called from a workqueue, and not from
+	 * the parent .probe.
+	 * If devm_ was used, the memory might never be freed on errors.
+	 */
+	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
 	if (!ctx)
 		return NULL;
 
 	ctx->count = count;
-	ctx->links = devm_kcalloc(&adev->dev, ctx->count,
-				  sizeof(*ctx->links), GFP_KERNEL);
-	if (!ctx->links)
+
+	/*
+	 * allocate the array of pointers. The link-specific data is allocated
+	 * as part of the first loop below and released with the auxiliary_device_uninit().
+	 * If some links are disabled, the link pointer will remain NULL. Given that the
+	 * number of links is small, this is simpler than using a list to keep track of links.
+	 */
+	ctx->ldev = kcalloc(ctx->count, sizeof(*ctx->ldev), GFP_KERNEL);
+	if (!ctx->ldev) {
+		kfree(ctx);
 		return NULL;
+	}
 
-	ctx->count = count;
 	ctx->mmio_base = res->mmio_base;
 	ctx->link_mask = res->link_mask;
 	ctx->handle = res->handle;
 	mutex_init(&ctx->shim_lock);
 
-	link = ctx->links;
 	link_mask = ctx->link_mask;
 
 	INIT_LIST_HEAD(&ctx->link_list);
 
-	/* Create SDW Master devices */
-	for (i = 0; i < count; i++, link++) {
-		if (!(link_mask & BIT(i))) {
-			dev_dbg(&adev->dev,
-				"Link %d masked, will not be enabled\n", i);
+	for (i = 0; i < count; i++) {
+		if (!(link_mask & BIT(i)))
 			continue;
+
+		/* Alloc and init devices */
+		ldev = kzalloc(sizeof(*ldev), GFP_KERNEL);
+		if (!ldev)
+			goto err;
+
+		/*
+		 * keep a handle on the allocated memory, to be used in all other functions.
+		 * Since the same pattern is used to skip links that are not enabled, there is
+		 * not need to check if ctx->ldev[i] is NULL later on.
+		 */
+		ctx->ldev[i] = ldev;
+
+		ret = intel_link_dev_init(ldev,
+					  res->parent,
+					  acpi_fwnode_handle(adev),
+					  "link", /* prefixed by core with "soundwire_intel." */
+					  i);
+		if (ret < 0) {
+			/*
+			 * We need to free the memory allocated in the current iteration
+			 */
+			kfree(ldev);
+
+			goto err;
 		}
 
+		link = &ldev->link_res;
+
+		/* now set link information */
 		link->mmio_base = res->mmio_base;
 		link->registers = res->mmio_base + SDW_LINK_BASE
 			+ (SDW_LINK_SIZE * i);
@@ -159,24 +233,16 @@ static struct sdw_intel_ctx
 		link->shim_mask = &ctx->shim_mask;
 		link->link_mask = link_mask;
 
-		memset(&pdevinfo, 0, sizeof(pdevinfo));
-
-		pdevinfo.parent = res->parent;
-		pdevinfo.name = "intel-sdw";
-		pdevinfo.id = i;
-		pdevinfo.fwnode = acpi_fwnode_handle(adev);
-		pdevinfo.data = link;
-		pdevinfo.size_data = sizeof(*link);
-
-		pdev = platform_device_register_full(&pdevinfo);
-		if (IS_ERR(pdev)) {
-			dev_err(&adev->dev,
-				"platform device creation failed: %ld\n",
-				PTR_ERR(pdev));
+		ret = auxiliary_device_add(&ldev->auxdev);
+		if (ret < 0) {
+			dev_err(&adev->dev, "failed to add link dev %s link_id %d\n",
+				ldev->auxdev.name, i);
+			/* ldev will be freed with the put_device() and .release sequence */
+			auxiliary_device_uninit(&ldev->auxdev);
 			goto err;
 		}
-		link->pdev = pdev;
-		link->cdns = platform_get_drvdata(pdev);
+
+		link->cdns = dev_get_drvdata(&ldev->auxdev.dev);
 
 		list_add_tail(&link->list, &ctx->link_list);
 		bus = &link->cdns->bus;
@@ -185,8 +251,7 @@ static struct sdw_intel_ctx
 			num_slaves++;
 	}
 
-	ctx->ids = devm_kcalloc(&adev->dev, num_slaves,
-				sizeof(*ctx->ids), GFP_KERNEL);
+	ctx->ids = kcalloc(num_slaves, sizeof(*ctx->ids), GFP_KERNEL);
 	if (!ctx->ids)
 		goto err;
 
@@ -204,8 +269,14 @@ static struct sdw_intel_ctx
 	return ctx;
 
 err:
-	ctx->count = i;
-	sdw_intel_cleanup(ctx);
+	while (i--) {
+		if (!(link_mask & BIT(i)))
+			continue;
+		ldev = ctx->ldev[i];
+		intel_link_dev_unregister(ldev);
+	}
+	kfree(ctx->ldev);
+	kfree(ctx);
 	return NULL;
 }
 
@@ -213,7 +284,7 @@ static int
 sdw_intel_startup_controller(struct sdw_intel_ctx *ctx)
 {
 	struct acpi_device *adev;
-	struct sdw_intel_link_res *link;
+	struct sdw_intel_link_dev *ldev;
 	u32 caps;
 	u32 link_mask;
 	int i;
@@ -232,27 +303,28 @@ sdw_intel_startup_controller(struct sdw_intel_ctx *ctx)
 		return -EINVAL;
 	}
 
-	if (!ctx->links)
+	if (!ctx->ldev)
 		return -EINVAL;
 
-	link = ctx->links;
 	link_mask = ctx->link_mask;
 
 	/* Startup SDW Master devices */
-	for (i = 0; i < ctx->count; i++, link++) {
+	for (i = 0; i < ctx->count; i++) {
 		if (!(link_mask & BIT(i)))
 			continue;
 
-		intel_master_startup(link->pdev);
+		ldev = ctx->ldev[i];
 
-		if (!link->clock_stop_quirks) {
+		intel_link_startup(&ldev->auxdev);
+
+		if (!ldev->link_res.clock_stop_quirks) {
 			/*
 			 * we need to prevent the parent PCI device
 			 * from entering pm_runtime suspend, so that
 			 * power rails to the SoundWire IP are not
 			 * turned off.
 			 */
-			pm_runtime_get_noresume(link->dev);
+			pm_runtime_get_noresume(ldev->link_res.dev);
 		}
 	}
 
@@ -297,27 +369,31 @@ EXPORT_SYMBOL_NS(sdw_intel_startup, SOUNDWIRE_INTEL_INIT);
 void sdw_intel_exit(struct sdw_intel_ctx *ctx)
 {
 	sdw_intel_cleanup(ctx);
+	kfree(ctx->ids);
+	kfree(ctx->ldev);
+	kfree(ctx);
 }
 EXPORT_SYMBOL_NS(sdw_intel_exit, SOUNDWIRE_INTEL_INIT);
 
 void sdw_intel_process_wakeen_event(struct sdw_intel_ctx *ctx)
 {
-	struct sdw_intel_link_res *link;
+	struct sdw_intel_link_dev *ldev;
 	u32 link_mask;
 	int i;
 
-	if (!ctx->links)
+	if (!ctx->ldev)
 		return;
 
-	link = ctx->links;
 	link_mask = ctx->link_mask;
 
 	/* Startup SDW Master devices */
-	for (i = 0; i < ctx->count; i++, link++) {
+	for (i = 0; i < ctx->count; i++) {
 		if (!(link_mask & BIT(i)))
 			continue;
 
-		intel_master_process_wakeen_event(link->pdev);
+		ldev = ctx->ldev[i];
+
+		intel_link_process_wakeen_event(&ldev->auxdev);
 	}
 }
 EXPORT_SYMBOL_NS(sdw_intel_process_wakeen_event, SOUNDWIRE_INTEL_INIT);
diff --git a/include/linux/soundwire/sdw_intel.h b/include/linux/soundwire/sdw_intel.h
index 3a5446ac014a..1ebea7764011 100644
--- a/include/linux/soundwire/sdw_intel.h
+++ b/include/linux/soundwire/sdw_intel.h
@@ -58,7 +58,7 @@ struct sdw_intel_acpi_info {
 	u32 link_mask;
 };
 
-struct sdw_intel_link_res;
+struct sdw_intel_link_dev;
 
 /* Intel clock-stop/pm_runtime quirk definitions */
 
@@ -109,7 +109,7 @@ struct sdw_intel_slave_id {
  * Controller
  * @num_slaves: total number of devices exposed across all enabled links
  * @handle: ACPI parent handle
- * @links: information for each link (controller-specific and kept
+ * @ldev: information for each link (controller-specific and kept
  * opaque here)
  * @ids: array of slave_id, representing Slaves exposed across all enabled
  * links
@@ -123,7 +123,7 @@ struct sdw_intel_ctx {
 	u32 link_mask;
 	int num_slaves;
 	acpi_handle handle;
-	struct sdw_intel_link_res *links;
+	struct sdw_intel_link_dev **ldev;
 	struct sdw_intel_slave_id *ids;
 	struct list_head link_list;
 	struct mutex shim_lock; /* lock for access to shared SHIM registers */
-- 
2.17.1

[PATCH] soundwire: intel: move to auxiliary bus

[Bard Liao]

From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Now that the auxiliary_bus exists, there's no reason to use platform
devices as children of a PCI device any longer.

This patch refactors the code by extending a basic auxiliary device
with Intel link-specific structures that need to be passed between
controller and link levels. This refactoring is much cleaner with no
need for cross-pointers between device and link structures.

Note that the auxiliary bus API has separate init and add steps, which
requires more attention in the error unwinding paths. The main loop
needs to deal with kfree() and auxiliary_device_uninit() for the
current iteration before jumping to the common label which releases
everything allocated in prior iterations.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
---
 drivers/soundwire/Kconfig           |   1 +
 drivers/soundwire/intel.c           |  52 ++++----
 drivers/soundwire/intel.h           |  14 +-
 drivers/soundwire/intel_init.c      | 190 +++++++++++++++++++---------
 include/linux/soundwire/sdw_intel.h |   6 +-
 5 files changed, 175 insertions(+), 88 deletions(-)

diff --git a/drivers/soundwire/Kconfig b/drivers/soundwire/Kconfig
index 016e74230bb7..2b7795233282 100644
--- a/drivers/soundwire/Kconfig
+++ b/drivers/soundwire/Kconfig
@@ -25,6 +25,7 @@ config SOUNDWIRE_INTEL
 	tristate "Intel SoundWire Master driver"
 	select SOUNDWIRE_CADENCE
 	select SOUNDWIRE_GENERIC_ALLOCATION
+	select AUXILIARY_BUS
 	depends on ACPI && SND_SOC
 	help
 	  SoundWire Intel Master driver.
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index d2254ee2fee2..039a101982c9 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -11,7 +11,7 @@
 #include <linux/module.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
-#include <linux/platform_device.h>
+#include <linux/auxiliary_bus.h>
 #include <sound/pcm_params.h>
 #include <linux/pm_runtime.h>
 #include <sound/soc.h>
@@ -1331,9 +1331,10 @@ static int intel_init(struct sdw_intel *sdw)
 /*
  * probe and init
  */
-static int intel_master_probe(struct platform_device *pdev)
+static int intel_link_probe(struct auxiliary_device *auxdev, const struct auxiliary_device_id *id)
 {
-	struct device *dev = &pdev->dev;
+	struct device *dev = &auxdev->dev;
+	struct sdw_intel_link_dev *ldev = auxiliary_dev_to_sdw_intel_link_dev(auxdev);
 	struct sdw_intel *sdw;
 	struct sdw_cdns *cdns;
 	struct sdw_bus *bus;
@@ -1346,14 +1347,14 @@ static int intel_master_probe(struct platform_device *pdev)
 	cdns = &sdw->cdns;
 	bus = &cdns->bus;
 
-	sdw->instance = pdev->id;
-	sdw->link_res = dev_get_platdata(dev);
+	sdw->instance = auxdev->id;
+	sdw->link_res = &ldev->link_res;
 	cdns->dev = dev;
 	cdns->registers = sdw->link_res->registers;
 	cdns->instance = sdw->instance;
 	cdns->msg_count = 0;
 
-	bus->link_id = pdev->id;
+	bus->link_id = auxdev->id;
 
 	sdw_cdns_probe(cdns);
 
@@ -1386,10 +1387,10 @@ static int intel_master_probe(struct platform_device *pdev)
 	return 0;
 }
 
-int intel_master_startup(struct platform_device *pdev)
+int intel_link_startup(struct auxiliary_device *auxdev)
 {
 	struct sdw_cdns_stream_config config;
-	struct device *dev = &pdev->dev;
+	struct device *dev = &auxdev->dev;
 	struct sdw_cdns *cdns = dev_get_drvdata(dev);
 	struct sdw_intel *sdw = cdns_to_intel(cdns);
 	struct sdw_bus *bus = &cdns->bus;
@@ -1526,9 +1527,9 @@ int intel_master_startup(struct platform_device *pdev)
 	return ret;
 }
 
-static int intel_master_remove(struct platform_device *pdev)
+static void intel_link_remove(struct auxiliary_device *auxdev)
 {
-	struct device *dev = &pdev->dev;
+	struct device *dev = &auxdev->dev;
 	struct sdw_cdns *cdns = dev_get_drvdata(dev);
 	struct sdw_intel *sdw = cdns_to_intel(cdns);
 	struct sdw_bus *bus = &cdns->bus;
@@ -1544,19 +1545,17 @@ static int intel_master_remove(struct platform_device *pdev)
 		snd_soc_unregister_component(dev);
 	}
 	sdw_bus_master_delete(bus);
-
-	return 0;
 }
 
-int intel_master_process_wakeen_event(struct platform_device *pdev)
+int intel_link_process_wakeen_event(struct auxiliary_device *auxdev)
 {
-	struct device *dev = &pdev->dev;
+	struct device *dev = &auxdev->dev;
 	struct sdw_intel *sdw;
 	struct sdw_bus *bus;
 	void __iomem *shim;
 	u16 wake_sts;
 
-	sdw = platform_get_drvdata(pdev);
+	sdw = dev_get_drvdata(dev);
 	bus = &sdw->cdns.bus;
 
 	if (bus->prop.hw_disabled) {
@@ -1978,17 +1977,22 @@ static const struct dev_pm_ops intel_pm = {
 	SET_RUNTIME_PM_OPS(intel_suspend_runtime, intel_resume_runtime, NULL)
 };
 
-static struct platform_driver sdw_intel_drv = {
-	.probe = intel_master_probe,
-	.remove = intel_master_remove,
+static const struct auxiliary_device_id intel_link_id_table[] = {
+	{ .name = "soundwire_intel.link" },
+	{},
+};
+MODULE_DEVICE_TABLE(auxiliary, intel_link_id_table);
+
+static struct auxiliary_driver sdw_intel_drv = {
+	.probe = intel_link_probe,
+	.remove = intel_link_remove,
 	.driver = {
-		.name = "intel-sdw",
+		/* auxiliary_driver_register() sets .name to be the modname */
 		.pm = &intel_pm,
-	}
+	},
+	.id_table = intel_link_id_table
 };
-
-module_platform_driver(sdw_intel_drv);
+module_auxiliary_driver(sdw_intel_drv);
 
 MODULE_LICENSE("Dual BSD/GPL");
-MODULE_ALIAS("platform:intel-sdw");
-MODULE_DESCRIPTION("Intel Soundwire Master Driver");
+MODULE_DESCRIPTION("Intel Soundwire Link Driver");
diff --git a/drivers/soundwire/intel.h b/drivers/soundwire/intel.h
index 06bac8ba14e9..0b47b148da3f 100644
--- a/drivers/soundwire/intel.h
+++ b/drivers/soundwire/intel.h
@@ -7,7 +7,6 @@
 /**
  * struct sdw_intel_link_res - Soundwire Intel link resource structure,
  * typically populated by the controller driver.
- * @pdev: platform_device
  * @mmio_base: mmio base of SoundWire registers
  * @registers: Link IO registers base
  * @shim: Audio shim pointer
@@ -23,7 +22,6 @@
  * @list: used to walk-through all masters exposed by the same controller
  */
 struct sdw_intel_link_res {
-	struct platform_device *pdev;
 	void __iomem *mmio_base; /* not strictly needed, useful for debug */
 	void __iomem *registers;
 	void __iomem *shim;
@@ -48,7 +46,15 @@ struct sdw_intel {
 #endif
 };
 
-int intel_master_startup(struct platform_device *pdev);
-int intel_master_process_wakeen_event(struct platform_device *pdev);
+int intel_link_startup(struct auxiliary_device *auxdev);
+int intel_link_process_wakeen_event(struct auxiliary_device *auxdev);
+
+struct sdw_intel_link_dev {
+	struct auxiliary_device auxdev;
+	struct sdw_intel_link_res link_res;
+};
+
+#define auxiliary_dev_to_sdw_intel_link_dev(auxiliary_dev) \
+	container_of(auxiliary_dev, struct sdw_intel_link_dev, auxdev)
 
 #endif /* __SDW_INTEL_LOCAL_H */
diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c
index 05b726cdfebc..3cb74060ccae 100644
--- a/drivers/soundwire/intel_init.c
+++ b/drivers/soundwire/intel_init.c
@@ -12,7 +12,7 @@
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/module.h>
-#include <linux/platform_device.h>
+#include <linux/auxiliary_bus.h>
 #include <linux/pm_runtime.h>
 #include <linux/soundwire/sdw_intel.h>
 #include "cadence_master.h"
@@ -24,28 +24,65 @@
 #define SDW_LINK_BASE		0x30000
 #define SDW_LINK_SIZE		0x10000
 
+static void intel_link_auxdev_release(struct device *dev)
+{
+	struct auxiliary_device *auxdev = to_auxiliary_dev(dev);
+	struct sdw_intel_link_dev *ldev = auxiliary_dev_to_sdw_intel_link_dev(auxdev);
+
+	kfree(ldev);
+}
+
+static int intel_link_dev_init(struct sdw_intel_link_dev *ldev,
+			       struct device *parent,
+			       struct fwnode_handle *fwnode,
+			       const char *name,
+			       int link_id)
+{
+	struct auxiliary_device *auxdev;
+	int ret;
+
+	auxdev = &ldev->auxdev;
+	auxdev->name = name;
+	auxdev->dev.parent = parent;
+	auxdev->dev.fwnode = fwnode;
+	auxdev->dev.release = intel_link_auxdev_release;
+
+	/* we don't use an IDA since we already have a link ID */
+	auxdev->id = link_id;
+
+	ret = auxiliary_device_init(auxdev);
+	if (ret < 0)
+		dev_err(parent, "failed to initialize link dev %s link_id %d\n",
+			name, link_id);
+
+	return ret;
+}
+
+static void intel_link_dev_unregister(struct sdw_intel_link_dev *ldev)
+{
+	auxiliary_device_delete(&ldev->auxdev);
+	auxiliary_device_uninit(&ldev->auxdev);
+}
+
 static int sdw_intel_cleanup(struct sdw_intel_ctx *ctx)
 {
-	struct sdw_intel_link_res *link = ctx->links;
+	struct sdw_intel_link_dev *ldev;
 	u32 link_mask;
 	int i;
 
-	if (!link)
-		return 0;
-
 	link_mask = ctx->link_mask;
 
-	for (i = 0; i < ctx->count; i++, link++) {
+	for (i = 0; i < ctx->count; i++) {
 		if (!(link_mask & BIT(i)))
 			continue;
 
-		if (link->pdev) {
-			pm_runtime_disable(&link->pdev->dev);
-			platform_device_unregister(link->pdev);
-		}
+		ldev = ctx->ldev[i];
 
-		if (!link->clock_stop_quirks)
-			pm_runtime_put_noidle(link->dev);
+		pm_runtime_disable(&ldev->auxdev.dev);
+		intel_link_dev_unregister(ldev);
+
+		if (!ldev->link_res.clock_stop_quirks)
+			pm_runtime_put_noidle(ldev->link_res.dev);
 	}
 
 	return 0;
@@ -91,9 +128,8 @@ EXPORT_SYMBOL_NS(sdw_intel_thread, SOUNDWIRE_INTEL_INIT);
 static struct sdw_intel_ctx
 *sdw_intel_probe_controller(struct sdw_intel_res *res)
 {
-	struct platform_device_info pdevinfo;
-	struct platform_device *pdev;
 	struct sdw_intel_link_res *link;
+	struct sdw_intel_link_dev *ldev;
 	struct sdw_intel_ctx *ctx;
 	struct acpi_device *adev;
 	struct sdw_slave *slave;
@@ -103,6 +139,7 @@ static struct sdw_intel_ctx
 	int num_slaves = 0;
 	int count;
 	int i;
+	int ret;
 
 	if (!res)
 		return NULL;
@@ -116,35 +153,72 @@ static struct sdw_intel_ctx
 	count = res->count;
 	dev_dbg(&adev->dev, "Creating %d SDW Link devices\n", count);
 
-	ctx = devm_kzalloc(&adev->dev, sizeof(*ctx), GFP_KERNEL);
+	/*
+	 * we need to alloc/free memory manually and can't use devm:
+	 * this routine may be called from a workqueue, and not from
+	 * the parent .probe.
+	 * If devm_ was used, the memory might never be freed on errors.
+	 */
+	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
 	if (!ctx)
 		return NULL;
 
 	ctx->count = count;
-	ctx->links = devm_kcalloc(&adev->dev, ctx->count,
-				  sizeof(*ctx->links), GFP_KERNEL);
-	if (!ctx->links)
+
+	/*
+	 * allocate the array of pointers. The link-specific data is allocated
+	 * as part of the first loop below and released with the auxiliary_device_uninit().
+	 * If some links are disabled, the link pointer will remain NULL. Given that the
+	 * number of links is small, this is simpler than using a list to keep track of links.
+	 */
+	ctx->ldev = kcalloc(ctx->count, sizeof(*ctx->ldev), GFP_KERNEL);
+	if (!ctx->ldev) {
+		kfree(ctx);
 		return NULL;
+	}
 
-	ctx->count = count;
 	ctx->mmio_base = res->mmio_base;
 	ctx->link_mask = res->link_mask;
 	ctx->handle = res->handle;
 	mutex_init(&ctx->shim_lock);
 
-	link = ctx->links;
 	link_mask = ctx->link_mask;
 
 	INIT_LIST_HEAD(&ctx->link_list);
 
-	/* Create SDW Master devices */
-	for (i = 0; i < count; i++, link++) {
-		if (!(link_mask & BIT(i))) {
-			dev_dbg(&adev->dev,
-				"Link %d masked, will not be enabled\n", i);
+	for (i = 0; i < count; i++) {
+		if (!(link_mask & BIT(i)))
 			continue;
+
+		/* Alloc and init devices */
+		ldev = kzalloc(sizeof(*ldev), GFP_KERNEL);
+		if (!ldev)
+			goto err;
+
+		/*
+		 * keep a handle on the allocated memory, to be used in all other functions.
+		 * Since the same pattern is used to skip links that are not enabled, there is
+		 * not need to check if ctx->ldev[i] is NULL later on.
+		 */
+		ctx->ldev[i] = ldev;
+
+		ret = intel_link_dev_init(ldev,
+					  res->parent,
+					  acpi_fwnode_handle(adev),
+					  "link", /* prefixed by core with "soundwire_intel." */
+					  i);
+		if (ret < 0) {
+			/*
+			 * We need to free the memory allocated in the current iteration
+			 */
+			kfree(ldev);
+
+			goto err;
 		}
 
+		link = &ldev->link_res;
+
+		/* now set link information */
 		link->mmio_base = res->mmio_base;
 		link->registers = res->mmio_base + SDW_LINK_BASE
 			+ (SDW_LINK_SIZE * i);
@@ -159,24 +233,16 @@ static struct sdw_intel_ctx
 		link->shim_mask = &ctx->shim_mask;
 		link->link_mask = link_mask;
 
-		memset(&pdevinfo, 0, sizeof(pdevinfo));
-
-		pdevinfo.parent = res->parent;
-		pdevinfo.name = "intel-sdw";
-		pdevinfo.id = i;
-		pdevinfo.fwnode = acpi_fwnode_handle(adev);
-		pdevinfo.data = link;
-		pdevinfo.size_data = sizeof(*link);
-
-		pdev = platform_device_register_full(&pdevinfo);
-		if (IS_ERR(pdev)) {
-			dev_err(&adev->dev,
-				"platform device creation failed: %ld\n",
-				PTR_ERR(pdev));
+		ret = auxiliary_device_add(&ldev->auxdev);
+		if (ret < 0) {
+			dev_err(&adev->dev, "failed to add link dev %s link_id %d\n",
+				ldev->auxdev.name, i);
+			/* ldev will be freed with the put_device() and .release sequence */
+			auxiliary_device_uninit(&ldev->auxdev);
 			goto err;
 		}
-		link->pdev = pdev;
-		link->cdns = platform_get_drvdata(pdev);
+
+		link->cdns = dev_get_drvdata(&ldev->auxdev.dev);
 
 		list_add_tail(&link->list, &ctx->link_list);
 		bus = &link->cdns->bus;
@@ -185,8 +251,7 @@ static struct sdw_intel_ctx
 			num_slaves++;
 	}
 
-	ctx->ids = devm_kcalloc(&adev->dev, num_slaves,
-				sizeof(*ctx->ids), GFP_KERNEL);
+	ctx->ids = kcalloc(num_slaves, sizeof(*ctx->ids), GFP_KERNEL);
 	if (!ctx->ids)
 		goto err;
 
@@ -204,8 +269,14 @@ static struct sdw_intel_ctx
 	return ctx;
 
 err:
-	ctx->count = i;
-	sdw_intel_cleanup(ctx);
+	while (i--) {
+		if (!(link_mask & BIT(i)))
+			continue;
+		ldev = ctx->ldev[i];
+		intel_link_dev_unregister(ldev);
+	}
+	kfree(ctx->ldev);
+	kfree(ctx);
 	return NULL;
 }
 
@@ -213,7 +284,7 @@ static int
 sdw_intel_startup_controller(struct sdw_intel_ctx *ctx)
 {
 	struct acpi_device *adev;
-	struct sdw_intel_link_res *link;
+	struct sdw_intel_link_dev *ldev;
 	u32 caps;
 	u32 link_mask;
 	int i;
@@ -232,27 +303,28 @@ sdw_intel_startup_controller(struct sdw_intel_ctx *ctx)
 		return -EINVAL;
 	}
 
-	if (!ctx->links)
+	if (!ctx->ldev)
 		return -EINVAL;
 
-	link = ctx->links;
 	link_mask = ctx->link_mask;
 
 	/* Startup SDW Master devices */
-	for (i = 0; i < ctx->count; i++, link++) {
+	for (i = 0; i < ctx->count; i++) {
 		if (!(link_mask & BIT(i)))
 			continue;
 
-		intel_master_startup(link->pdev);
+		ldev = ctx->ldev[i];
 
-		if (!link->clock_stop_quirks) {
+		intel_link_startup(&ldev->auxdev);
+
+		if (!ldev->link_res.clock_stop_quirks) {
 			/*
 			 * we need to prevent the parent PCI device
 			 * from entering pm_runtime suspend, so that
 			 * power rails to the SoundWire IP are not
 			 * turned off.
 			 */
-			pm_runtime_get_noresume(link->dev);
+			pm_runtime_get_noresume(ldev->link_res.dev);
 		}
 	}
 
@@ -297,27 +369,31 @@ EXPORT_SYMBOL_NS(sdw_intel_startup, SOUNDWIRE_INTEL_INIT);
 void sdw_intel_exit(struct sdw_intel_ctx *ctx)
 {
 	sdw_intel_cleanup(ctx);
+	kfree(ctx->ids);
+	kfree(ctx->ldev);
+	kfree(ctx);
 }
 EXPORT_SYMBOL_NS(sdw_intel_exit, SOUNDWIRE_INTEL_INIT);
 
 void sdw_intel_process_wakeen_event(struct sdw_intel_ctx *ctx)
 {
-	struct sdw_intel_link_res *link;
+	struct sdw_intel_link_dev *ldev;
 	u32 link_mask;
 	int i;
 
-	if (!ctx->links)
+	if (!ctx->ldev)
 		return;
 
-	link = ctx->links;
 	link_mask = ctx->link_mask;
 
 	/* Startup SDW Master devices */
-	for (i = 0; i < ctx->count; i++, link++) {
+	for (i = 0; i < ctx->count; i++) {
 		if (!(link_mask & BIT(i)))
 			continue;
 
-		intel_master_process_wakeen_event(link->pdev);
+		ldev = ctx->ldev[i];
+
+		intel_link_process_wakeen_event(&ldev->auxdev);
 	}
 }
 EXPORT_SYMBOL_NS(sdw_intel_process_wakeen_event, SOUNDWIRE_INTEL_INIT);
diff --git a/include/linux/soundwire/sdw_intel.h b/include/linux/soundwire/sdw_intel.h
index 3a5446ac014a..1ebea7764011 100644
--- a/include/linux/soundwire/sdw_intel.h
+++ b/include/linux/soundwire/sdw_intel.h
@@ -58,7 +58,7 @@ struct sdw_intel_acpi_info {
 	u32 link_mask;
 };
 
-struct sdw_intel_link_res;
+struct sdw_intel_link_dev;
 
 /* Intel clock-stop/pm_runtime quirk definitions */
 
@@ -109,7 +109,7 @@ struct sdw_intel_slave_id {
  * Controller
  * @num_slaves: total number of devices exposed across all enabled links
  * @handle: ACPI parent handle
- * @links: information for each link (controller-specific and kept
+ * @ldev: information for each link (controller-specific and kept
  * opaque here)
  * @ids: array of slave_id, representing Slaves exposed across all enabled
  * links
@@ -123,7 +123,7 @@ struct sdw_intel_ctx {
 	u32 link_mask;
 	int num_slaves;
 	acpi_handle handle;
-	struct sdw_intel_link_res *links;
+	struct sdw_intel_link_dev **ldev;
 	struct sdw_intel_slave_id *ids;
 	struct list_head link_list;
 	struct mutex shim_lock; /* lock for access to shared SHIM registers */
-- 
2.17.1

Re: [PATCH] soundwire: intel: move to auxiliary bus

[Vinod Koul]

On 23-03-21, 08:43, Bard Liao wrote:
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> 
> Now that the auxiliary_bus exists, there's no reason to use platform
> devices as children of a PCI device any longer.
> 
> This patch refactors the code by extending a basic auxiliary device
> with Intel link-specific structures that need to be passed between
> controller and link levels. This refactoring is much cleaner with no
> need for cross-pointers between device and link structures.
> 
> Note that the auxiliary bus API has separate init and add steps, which
> requires more attention in the error unwinding paths. The main loop
> needs to deal with kfree() and auxiliary_device_uninit() for the
> current iteration before jumping to the common label which releases
> everything allocated in prior iterations.
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Reviewed-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
> Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
> Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
> ---
>  drivers/soundwire/Kconfig           |   1 +
>  drivers/soundwire/intel.c           |  52 ++++----
>  drivers/soundwire/intel.h           |  14 +-
>  drivers/soundwire/intel_init.c      | 190 +++++++++++++++++++---------
>  include/linux/soundwire/sdw_intel.h |   6 +-
>  5 files changed, 175 insertions(+), 88 deletions(-)
> 
> diff --git a/drivers/soundwire/Kconfig b/drivers/soundwire/Kconfig
> index 016e74230bb7..2b7795233282 100644
> --- a/drivers/soundwire/Kconfig
> +++ b/drivers/soundwire/Kconfig
> @@ -25,6 +25,7 @@ config SOUNDWIRE_INTEL
>  	tristate "Intel SoundWire Master driver"
>  	select SOUNDWIRE_CADENCE
>  	select SOUNDWIRE_GENERIC_ALLOCATION
> +	select AUXILIARY_BUS
>  	depends on ACPI && SND_SOC
>  	help
>  	  SoundWire Intel Master driver.
> diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
> index d2254ee2fee2..039a101982c9 100644
> --- a/drivers/soundwire/intel.c
> +++ b/drivers/soundwire/intel.c
> @@ -11,7 +11,7 @@
>  #include <linux/module.h>
>  #include <linux/interrupt.h>
>  #include <linux/io.h>
> -#include <linux/platform_device.h>
> +#include <linux/auxiliary_bus.h>
>  #include <sound/pcm_params.h>
>  #include <linux/pm_runtime.h>
>  #include <sound/soc.h>
> @@ -1331,9 +1331,10 @@ static int intel_init(struct sdw_intel *sdw)
>  /*
>   * probe and init
>   */
> -static int intel_master_probe(struct platform_device *pdev)
> +static int intel_link_probe(struct auxiliary_device *auxdev, const struct auxiliary_device_id *id)
>  {
> -	struct device *dev = &pdev->dev;
> +	struct device *dev = &auxdev->dev;
> +	struct sdw_intel_link_dev *ldev = auxiliary_dev_to_sdw_intel_link_dev(auxdev);

Do we need another abstractions for resources here, why not aux dev
creation set the resources required and we skip this step...

>  	struct sdw_intel *sdw;
>  	struct sdw_cdns *cdns;
>  	struct sdw_bus *bus;
> @@ -1346,14 +1347,14 @@ static int intel_master_probe(struct platform_device *pdev)
>  	cdns = &sdw->cdns;
>  	bus = &cdns->bus;
>  
> -	sdw->instance = pdev->id;
> -	sdw->link_res = dev_get_platdata(dev);
> +	sdw->instance = auxdev->id;

so auxdev has id and still we pass id as argument :( Not sure if folks
can fix this now

> +	sdw->link_res = &ldev->link_res;
>  	cdns->dev = dev;
>  	cdns->registers = sdw->link_res->registers;
>  	cdns->instance = sdw->instance;
>  	cdns->msg_count = 0;
>  
> -	bus->link_id = pdev->id;
> +	bus->link_id = auxdev->id;
>  
>  	sdw_cdns_probe(cdns);
>  
> @@ -1386,10 +1387,10 @@ static int intel_master_probe(struct platform_device *pdev)
>  	return 0;
>  }
>  
> -int intel_master_startup(struct platform_device *pdev)
> +int intel_link_startup(struct auxiliary_device *auxdev)
>  {
>  	struct sdw_cdns_stream_config config;
> -	struct device *dev = &pdev->dev;
> +	struct device *dev = &auxdev->dev;
>  	struct sdw_cdns *cdns = dev_get_drvdata(dev);
>  	struct sdw_intel *sdw = cdns_to_intel(cdns);
>  	struct sdw_bus *bus = &cdns->bus;
> @@ -1526,9 +1527,9 @@ int intel_master_startup(struct platform_device *pdev)
>  	return ret;
>  }
>  
> -static int intel_master_remove(struct platform_device *pdev)
> +static void intel_link_remove(struct auxiliary_device *auxdev)
>  {
> -	struct device *dev = &pdev->dev;
> +	struct device *dev = &auxdev->dev;
>  	struct sdw_cdns *cdns = dev_get_drvdata(dev);
>  	struct sdw_intel *sdw = cdns_to_intel(cdns);
>  	struct sdw_bus *bus = &cdns->bus;
> @@ -1544,19 +1545,17 @@ static int intel_master_remove(struct platform_device *pdev)
>  		snd_soc_unregister_component(dev);
>  	}
>  	sdw_bus_master_delete(bus);
> -
> -	return 0;
>  }
>  
> -int intel_master_process_wakeen_event(struct platform_device *pdev)
> +int intel_link_process_wakeen_event(struct auxiliary_device *auxdev)
>  {
> -	struct device *dev = &pdev->dev;
> +	struct device *dev = &auxdev->dev;
>  	struct sdw_intel *sdw;
>  	struct sdw_bus *bus;
>  	void __iomem *shim;
>  	u16 wake_sts;
>  
> -	sdw = platform_get_drvdata(pdev);
> +	sdw = dev_get_drvdata(dev);

No auxdev_get_drvdata() ?

>  	bus = &sdw->cdns.bus;
>  
>  	if (bus->prop.hw_disabled) {
> @@ -1978,17 +1977,22 @@ static const struct dev_pm_ops intel_pm = {
>  	SET_RUNTIME_PM_OPS(intel_suspend_runtime, intel_resume_runtime, NULL)
>  };
>  
> -static struct platform_driver sdw_intel_drv = {
> -	.probe = intel_master_probe,
> -	.remove = intel_master_remove,
> +static const struct auxiliary_device_id intel_link_id_table[] = {
> +	{ .name = "soundwire_intel.link" },

Curious why name with .link..?

> +	{},
> +};
> +MODULE_DEVICE_TABLE(auxiliary, intel_link_id_table);
> +
> +static struct auxiliary_driver sdw_intel_drv = {
> +	.probe = intel_link_probe,
> +	.remove = intel_link_remove,
>  	.driver = {
> -		.name = "intel-sdw",
> +		/* auxiliary_driver_register() sets .name to be the modname */
>  		.pm = &intel_pm,
> -	}
> +	},
> +	.id_table = intel_link_id_table
>  };
> -
> -module_platform_driver(sdw_intel_drv);
> +module_auxiliary_driver(sdw_intel_drv);
>  
>  MODULE_LICENSE("Dual BSD/GPL");
> -MODULE_ALIAS("platform:intel-sdw");
> -MODULE_DESCRIPTION("Intel Soundwire Master Driver");
> +MODULE_DESCRIPTION("Intel Soundwire Link Driver");
> diff --git a/drivers/soundwire/intel.h b/drivers/soundwire/intel.h
> index 06bac8ba14e9..0b47b148da3f 100644
> --- a/drivers/soundwire/intel.h
> +++ b/drivers/soundwire/intel.h
> @@ -7,7 +7,6 @@
>  /**
>   * struct sdw_intel_link_res - Soundwire Intel link resource structure,
>   * typically populated by the controller driver.
> - * @pdev: platform_device
>   * @mmio_base: mmio base of SoundWire registers
>   * @registers: Link IO registers base
>   * @shim: Audio shim pointer
> @@ -23,7 +22,6 @@
>   * @list: used to walk-through all masters exposed by the same controller
>   */
>  struct sdw_intel_link_res {
> -	struct platform_device *pdev;
>  	void __iomem *mmio_base; /* not strictly needed, useful for debug */
>  	void __iomem *registers;
>  	void __iomem *shim;
> @@ -48,7 +46,15 @@ struct sdw_intel {
>  #endif
>  };
>  
> -int intel_master_startup(struct platform_device *pdev);
> -int intel_master_process_wakeen_event(struct platform_device *pdev);
> +int intel_link_startup(struct auxiliary_device *auxdev);
> +int intel_link_process_wakeen_event(struct auxiliary_device *auxdev);
> +
> +struct sdw_intel_link_dev {
> +	struct auxiliary_device auxdev;
> +	struct sdw_intel_link_res link_res;
> +};

I was hoping that with auxdev we can get rid of this init layer, can
that still be done?

The auxdev is created by Intel controller, so it sets up resources. I am
not really happy that we need to continue having this layer.. can we add
something is auxdev core to handle these situations.

What I would like to see the end result is that sdw driver for Intel
controller here is a simple auxdev device and no additional custom setup
layer required... which implies that this handling should be moved into
auxdev or Intel code setting up auxdev...

-- 
~Vinod

Re: [PATCH] soundwire: intel: move to auxiliary bus

[Vinod Koul]

On 23-03-21, 08:43, Bard Liao wrote:
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> 
> Now that the auxiliary_bus exists, there's no reason to use platform
> devices as children of a PCI device any longer.
> 
> This patch refactors the code by extending a basic auxiliary device
> with Intel link-specific structures that need to be passed between
> controller and link levels. This refactoring is much cleaner with no
> need for cross-pointers between device and link structures.
> 
> Note that the auxiliary bus API has separate init and add steps, which
> requires more attention in the error unwinding paths. The main loop
> needs to deal with kfree() and auxiliary_device_uninit() for the
> current iteration before jumping to the common label which releases
> everything allocated in prior iterations.
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Reviewed-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
> Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
> Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
> ---
>  drivers/soundwire/Kconfig           |   1 +
>  drivers/soundwire/intel.c           |  52 ++++----
>  drivers/soundwire/intel.h           |  14 +-
>  drivers/soundwire/intel_init.c      | 190 +++++++++++++++++++---------
>  include/linux/soundwire/sdw_intel.h |   6 +-
>  5 files changed, 175 insertions(+), 88 deletions(-)
> 
> diff --git a/drivers/soundwire/Kconfig b/drivers/soundwire/Kconfig
> index 016e74230bb7..2b7795233282 100644
> --- a/drivers/soundwire/Kconfig
> +++ b/drivers/soundwire/Kconfig
> @@ -25,6 +25,7 @@ config SOUNDWIRE_INTEL
>  	tristate "Intel SoundWire Master driver"
>  	select SOUNDWIRE_CADENCE
>  	select SOUNDWIRE_GENERIC_ALLOCATION
> +	select AUXILIARY_BUS
>  	depends on ACPI && SND_SOC
>  	help
>  	  SoundWire Intel Master driver.
> diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
> index d2254ee2fee2..039a101982c9 100644
> --- a/drivers/soundwire/intel.c
> +++ b/drivers/soundwire/intel.c
> @@ -11,7 +11,7 @@
>  #include <linux/module.h>
>  #include <linux/interrupt.h>
>  #include <linux/io.h>
> -#include <linux/platform_device.h>
> +#include <linux/auxiliary_bus.h>
>  #include <sound/pcm_params.h>
>  #include <linux/pm_runtime.h>
>  #include <sound/soc.h>
> @@ -1331,9 +1331,10 @@ static int intel_init(struct sdw_intel *sdw)
>  /*
>   * probe and init
>   */
> -static int intel_master_probe(struct platform_device *pdev)
> +static int intel_link_probe(struct auxiliary_device *auxdev, const struct auxiliary_device_id *id)
>  {
> -	struct device *dev = &pdev->dev;
> +	struct device *dev = &auxdev->dev;
> +	struct sdw_intel_link_dev *ldev = auxiliary_dev_to_sdw_intel_link_dev(auxdev);

Do we need another abstractions for resources here, why not aux dev
creation set the resources required and we skip this step...

>  	struct sdw_intel *sdw;
>  	struct sdw_cdns *cdns;
>  	struct sdw_bus *bus;
> @@ -1346,14 +1347,14 @@ static int intel_master_probe(struct platform_device *pdev)
>  	cdns = &sdw->cdns;
>  	bus = &cdns->bus;
>  
> -	sdw->instance = pdev->id;
> -	sdw->link_res = dev_get_platdata(dev);
> +	sdw->instance = auxdev->id;

so auxdev has id and still we pass id as argument :( Not sure if folks
can fix this now

> +	sdw->link_res = &ldev->link_res;
>  	cdns->dev = dev;
>  	cdns->registers = sdw->link_res->registers;
>  	cdns->instance = sdw->instance;
>  	cdns->msg_count = 0;
>  
> -	bus->link_id = pdev->id;
> +	bus->link_id = auxdev->id;
>  
>  	sdw_cdns_probe(cdns);
>  
> @@ -1386,10 +1387,10 @@ static int intel_master_probe(struct platform_device *pdev)
>  	return 0;
>  }
>  
> -int intel_master_startup(struct platform_device *pdev)
> +int intel_link_startup(struct auxiliary_device *auxdev)
>  {
>  	struct sdw_cdns_stream_config config;
> -	struct device *dev = &pdev->dev;
> +	struct device *dev = &auxdev->dev;
>  	struct sdw_cdns *cdns = dev_get_drvdata(dev);
>  	struct sdw_intel *sdw = cdns_to_intel(cdns);
>  	struct sdw_bus *bus = &cdns->bus;
> @@ -1526,9 +1527,9 @@ int intel_master_startup(struct platform_device *pdev)
>  	return ret;
>  }
>  
> -static int intel_master_remove(struct platform_device *pdev)
> +static void intel_link_remove(struct auxiliary_device *auxdev)
>  {
> -	struct device *dev = &pdev->dev;
> +	struct device *dev = &auxdev->dev;
>  	struct sdw_cdns *cdns = dev_get_drvdata(dev);
>  	struct sdw_intel *sdw = cdns_to_intel(cdns);
>  	struct sdw_bus *bus = &cdns->bus;
> @@ -1544,19 +1545,17 @@ static int intel_master_remove(struct platform_device *pdev)
>  		snd_soc_unregister_component(dev);
>  	}
>  	sdw_bus_master_delete(bus);
> -
> -	return 0;
>  }
>  
> -int intel_master_process_wakeen_event(struct platform_device *pdev)
> +int intel_link_process_wakeen_event(struct auxiliary_device *auxdev)
>  {
> -	struct device *dev = &pdev->dev;
> +	struct device *dev = &auxdev->dev;
>  	struct sdw_intel *sdw;
>  	struct sdw_bus *bus;
>  	void __iomem *shim;
>  	u16 wake_sts;
>  
> -	sdw = platform_get_drvdata(pdev);
> +	sdw = dev_get_drvdata(dev);

No auxdev_get_drvdata() ?

>  	bus = &sdw->cdns.bus;
>  
>  	if (bus->prop.hw_disabled) {
> @@ -1978,17 +1977,22 @@ static const struct dev_pm_ops intel_pm = {
>  	SET_RUNTIME_PM_OPS(intel_suspend_runtime, intel_resume_runtime, NULL)
>  };
>  
> -static struct platform_driver sdw_intel_drv = {
> -	.probe = intel_master_probe,
> -	.remove = intel_master_remove,
> +static const struct auxiliary_device_id intel_link_id_table[] = {
> +	{ .name = "soundwire_intel.link" },

Curious why name with .link..?

> +	{},
> +};
> +MODULE_DEVICE_TABLE(auxiliary, intel_link_id_table);
> +
> +static struct auxiliary_driver sdw_intel_drv = {
> +	.probe = intel_link_probe,
> +	.remove = intel_link_remove,
>  	.driver = {
> -		.name = "intel-sdw",
> +		/* auxiliary_driver_register() sets .name to be the modname */
>  		.pm = &intel_pm,
> -	}
> +	},
> +	.id_table = intel_link_id_table
>  };
> -
> -module_platform_driver(sdw_intel_drv);
> +module_auxiliary_driver(sdw_intel_drv);
>  
>  MODULE_LICENSE("Dual BSD/GPL");
> -MODULE_ALIAS("platform:intel-sdw");
> -MODULE_DESCRIPTION("Intel Soundwire Master Driver");
> +MODULE_DESCRIPTION("Intel Soundwire Link Driver");
> diff --git a/drivers/soundwire/intel.h b/drivers/soundwire/intel.h
> index 06bac8ba14e9..0b47b148da3f 100644
> --- a/drivers/soundwire/intel.h
> +++ b/drivers/soundwire/intel.h
> @@ -7,7 +7,6 @@
>  /**
>   * struct sdw_intel_link_res - Soundwire Intel link resource structure,
>   * typically populated by the controller driver.
> - * @pdev: platform_device
>   * @mmio_base: mmio base of SoundWire registers
>   * @registers: Link IO registers base
>   * @shim: Audio shim pointer
> @@ -23,7 +22,6 @@
>   * @list: used to walk-through all masters exposed by the same controller
>   */
>  struct sdw_intel_link_res {
> -	struct platform_device *pdev;
>  	void __iomem *mmio_base; /* not strictly needed, useful for debug */
>  	void __iomem *registers;
>  	void __iomem *shim;
> @@ -48,7 +46,15 @@ struct sdw_intel {
>  #endif
>  };
>  
> -int intel_master_startup(struct platform_device *pdev);
> -int intel_master_process_wakeen_event(struct platform_device *pdev);
> +int intel_link_startup(struct auxiliary_device *auxdev);
> +int intel_link_process_wakeen_event(struct auxiliary_device *auxdev);
> +
> +struct sdw_intel_link_dev {
> +	struct auxiliary_device auxdev;
> +	struct sdw_intel_link_res link_res;
> +};

I was hoping that with auxdev we can get rid of this init layer, can
that still be done?

The auxdev is created by Intel controller, so it sets up resources. I am
not really happy that we need to continue having this layer.. can we add
something is auxdev core to handle these situations.

What I would like to see the end result is that sdw driver for Intel
controller here is a simple auxdev device and no additional custom setup
layer required... which implies that this handling should be moved into
auxdev or Intel code setting up auxdev...

-- 
~Vinod

Re: [PATCH] soundwire: intel: move to auxiliary bus

[Greg KH]

On Tue, Mar 23, 2021 at 08:43:25AM +0800, Bard Liao wrote:
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> 
> Now that the auxiliary_bus exists, there's no reason to use platform
> devices as children of a PCI device any longer.
> 
> This patch refactors the code by extending a basic auxiliary device
> with Intel link-specific structures that need to be passed between
> controller and link levels. This refactoring is much cleaner with no
> need for cross-pointers between device and link structures.
> 
> Note that the auxiliary bus API has separate init and add steps, which
> requires more attention in the error unwinding paths. The main loop
> needs to deal with kfree() and auxiliary_device_uninit() for the
> current iteration before jumping to the common label which releases
> everything allocated in prior iterations.

The init/add steps can be moved together in the aux bus code if that
makes this usage simpler.  Please do that instead.

thanks,

greg k-h

Re: [PATCH] soundwire: intel: move to auxiliary bus

[Greg KH]

On Tue, Mar 23, 2021 at 08:43:25AM +0800, Bard Liao wrote:
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> 
> Now that the auxiliary_bus exists, there's no reason to use platform
> devices as children of a PCI device any longer.
> 
> This patch refactors the code by extending a basic auxiliary device
> with Intel link-specific structures that need to be passed between
> controller and link levels. This refactoring is much cleaner with no
> need for cross-pointers between device and link structures.
> 
> Note that the auxiliary bus API has separate init and add steps, which
> requires more attention in the error unwinding paths. The main loop
> needs to deal with kfree() and auxiliary_device_uninit() for the
> current iteration before jumping to the common label which releases
> everything allocated in prior iterations.

The init/add steps can be moved together in the aux bus code if that
makes this usage simpler.  Please do that instead.

thanks,

greg k-h

Re: [PATCH] soundwire: intel: move to auxiliary bus

[Greg KH]

On Tue, Mar 23, 2021 at 12:18:46PM +0530, Vinod Koul wrote:
> On 23-03-21, 08:43, Bard Liao wrote:
> > From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > 
> > Now that the auxiliary_bus exists, there's no reason to use platform
> > devices as children of a PCI device any longer.
> > 
> > This patch refactors the code by extending a basic auxiliary device
> > with Intel link-specific structures that need to be passed between
> > controller and link levels. This refactoring is much cleaner with no
> > need for cross-pointers between device and link structures.
> > 
> > Note that the auxiliary bus API has separate init and add steps, which
> > requires more attention in the error unwinding paths. The main loop
> > needs to deal with kfree() and auxiliary_device_uninit() for the
> > current iteration before jumping to the common label which releases
> > everything allocated in prior iterations.
> > 
> > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > Reviewed-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
> > Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
> > Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
> > ---
> >  drivers/soundwire/Kconfig           |   1 +
> >  drivers/soundwire/intel.c           |  52 ++++----
> >  drivers/soundwire/intel.h           |  14 +-
> >  drivers/soundwire/intel_init.c      | 190 +++++++++++++++++++---------
> >  include/linux/soundwire/sdw_intel.h |   6 +-
> >  5 files changed, 175 insertions(+), 88 deletions(-)
> > 
> > diff --git a/drivers/soundwire/Kconfig b/drivers/soundwire/Kconfig
> > index 016e74230bb7..2b7795233282 100644
> > --- a/drivers/soundwire/Kconfig
> > +++ b/drivers/soundwire/Kconfig
> > @@ -25,6 +25,7 @@ config SOUNDWIRE_INTEL
> >  	tristate "Intel SoundWire Master driver"
> >  	select SOUNDWIRE_CADENCE
> >  	select SOUNDWIRE_GENERIC_ALLOCATION
> > +	select AUXILIARY_BUS
> >  	depends on ACPI && SND_SOC
> >  	help
> >  	  SoundWire Intel Master driver.
> > diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
> > index d2254ee2fee2..039a101982c9 100644
> > --- a/drivers/soundwire/intel.c
> > +++ b/drivers/soundwire/intel.c
> > @@ -11,7 +11,7 @@
> >  #include <linux/module.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/io.h>
> > -#include <linux/platform_device.h>
> > +#include <linux/auxiliary_bus.h>
> >  #include <sound/pcm_params.h>
> >  #include <linux/pm_runtime.h>
> >  #include <sound/soc.h>
> > @@ -1331,9 +1331,10 @@ static int intel_init(struct sdw_intel *sdw)
> >  /*
> >   * probe and init
> >   */
> > -static int intel_master_probe(struct platform_device *pdev)
> > +static int intel_link_probe(struct auxiliary_device *auxdev, const struct auxiliary_device_id *id)
> >  {
> > -	struct device *dev = &pdev->dev;
> > +	struct device *dev = &auxdev->dev;
> > +	struct sdw_intel_link_dev *ldev = auxiliary_dev_to_sdw_intel_link_dev(auxdev);
> 
> Do we need another abstractions for resources here, why not aux dev
> creation set the resources required and we skip this step...
> 
> >  	struct sdw_intel *sdw;
> >  	struct sdw_cdns *cdns;
> >  	struct sdw_bus *bus;
> > @@ -1346,14 +1347,14 @@ static int intel_master_probe(struct platform_device *pdev)
> >  	cdns = &sdw->cdns;
> >  	bus = &cdns->bus;
> >  
> > -	sdw->instance = pdev->id;
> > -	sdw->link_res = dev_get_platdata(dev);
> > +	sdw->instance = auxdev->id;
> 
> so auxdev has id and still we pass id as argument :( Not sure if folks
> can fix this now

That's odd, yeah, it should be fixed.

greg k-h

Re: [PATCH] soundwire: intel: move to auxiliary bus

[Greg KH]

On Tue, Mar 23, 2021 at 12:18:46PM +0530, Vinod Koul wrote:
> On 23-03-21, 08:43, Bard Liao wrote:
> > From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > 
> > Now that the auxiliary_bus exists, there's no reason to use platform
> > devices as children of a PCI device any longer.
> > 
> > This patch refactors the code by extending a basic auxiliary device
> > with Intel link-specific structures that need to be passed between
> > controller and link levels. This refactoring is much cleaner with no
> > need for cross-pointers between device and link structures.
> > 
> > Note that the auxiliary bus API has separate init and add steps, which
> > requires more attention in the error unwinding paths. The main loop
> > needs to deal with kfree() and auxiliary_device_uninit() for the
> > current iteration before jumping to the common label which releases
> > everything allocated in prior iterations.
> > 
> > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > Reviewed-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
> > Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
> > Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
> > ---
> >  drivers/soundwire/Kconfig           |   1 +
> >  drivers/soundwire/intel.c           |  52 ++++----
> >  drivers/soundwire/intel.h           |  14 +-
> >  drivers/soundwire/intel_init.c      | 190 +++++++++++++++++++---------
> >  include/linux/soundwire/sdw_intel.h |   6 +-
> >  5 files changed, 175 insertions(+), 88 deletions(-)
> > 
> > diff --git a/drivers/soundwire/Kconfig b/drivers/soundwire/Kconfig
> > index 016e74230bb7..2b7795233282 100644
> > --- a/drivers/soundwire/Kconfig
> > +++ b/drivers/soundwire/Kconfig
> > @@ -25,6 +25,7 @@ config SOUNDWIRE_INTEL
> >  	tristate "Intel SoundWire Master driver"
> >  	select SOUNDWIRE_CADENCE
> >  	select SOUNDWIRE_GENERIC_ALLOCATION
> > +	select AUXILIARY_BUS
> >  	depends on ACPI && SND_SOC
> >  	help
> >  	  SoundWire Intel Master driver.
> > diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
> > index d2254ee2fee2..039a101982c9 100644
> > --- a/drivers/soundwire/intel.c
> > +++ b/drivers/soundwire/intel.c
> > @@ -11,7 +11,7 @@
> >  #include <linux/module.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/io.h>
> > -#include <linux/platform_device.h>
> > +#include <linux/auxiliary_bus.h>
> >  #include <sound/pcm_params.h>
> >  #include <linux/pm_runtime.h>
> >  #include <sound/soc.h>
> > @@ -1331,9 +1331,10 @@ static int intel_init(struct sdw_intel *sdw)
> >  /*
> >   * probe and init
> >   */
> > -static int intel_master_probe(struct platform_device *pdev)
> > +static int intel_link_probe(struct auxiliary_device *auxdev, const struct auxiliary_device_id *id)
> >  {
> > -	struct device *dev = &pdev->dev;
> > +	struct device *dev = &auxdev->dev;
> > +	struct sdw_intel_link_dev *ldev = auxiliary_dev_to_sdw_intel_link_dev(auxdev);
> 
> Do we need another abstractions for resources here, why not aux dev
> creation set the resources required and we skip this step...
> 
> >  	struct sdw_intel *sdw;
> >  	struct sdw_cdns *cdns;
> >  	struct sdw_bus *bus;
> > @@ -1346,14 +1347,14 @@ static int intel_master_probe(struct platform_device *pdev)
> >  	cdns = &sdw->cdns;
> >  	bus = &cdns->bus;
> >  
> > -	sdw->instance = pdev->id;
> > -	sdw->link_res = dev_get_platdata(dev);
> > +	sdw->instance = auxdev->id;
> 
> so auxdev has id and still we pass id as argument :( Not sure if folks
> can fix this now

That's odd, yeah, it should be fixed.

greg k-h

Re: [PATCH] soundwire: intel: move to auxiliary bus

[Pierre-Louis Bossart]

Thanks Greg and Vinod for the reviews

>>> -static int intel_master_probe(struct platform_device *pdev)
>>> +static int intel_link_probe(struct auxiliary_device *auxdev, const struct auxiliary_device_id *id)
>>>   {
>>> -	struct device *dev = &pdev->dev;
>>> +	struct device *dev = &auxdev->dev;
>>> +	struct sdw_intel_link_dev *ldev = auxiliary_dev_to_sdw_intel_link_dev(auxdev);
>>
>> Do we need another abstractions for resources here, why not aux dev
>> creation set the resources required and we skip this step...

Not sure what resources you are referring to?

this is just a container_of() and the documented way of extending the 
auxbus (see 
https://www.kernel.org/doc/html/latest/driver-api/auxiliary_bus.html#example-usage)


struct sdw_intel_link_dev {
	struct auxiliary_device auxdev;
	struct sdw_intel_link_res link_res;
};

#define auxiliary_dev_to_sdw_intel_link_dev(auxiliary_dev) \
	container_of(auxiliary_dev, struct sdw_intel_link_dev, auxdev)

>>>   	struct sdw_intel *sdw;
>>>   	struct sdw_cdns *cdns;
>>>   	struct sdw_bus *bus;
>>> @@ -1346,14 +1347,14 @@ static int intel_master_probe(struct platform_device *pdev)
>>>   	cdns = &sdw->cdns;
>>>   	bus = &cdns->bus;
>>>   
>>> -	sdw->instance = pdev->id;
>>> -	sdw->link_res = dev_get_platdata(dev);
>>> +	sdw->instance = auxdev->id;
>>
>> so auxdev has id and still we pass id as argument :( Not sure if folks
>> can fix this now
> 
> That's odd, yeah, it should be fixed.

I think we are talking about different things?

this is defined in mod_devicetable.h:

struct auxiliary_device_id {
	char name[AUXILIARY_NAME_SIZE];
	kernel_ulong_t driver_data;
};

and used for the driver probe:

	ret = auxdrv->probe(auxdev, auxiliary_match_id(auxdrv->id_table, auxdev));

but there is a separate id:

struct auxiliary_device {
	struct device dev;
	const char *name;
	u32 id;
};

which is set during the device initialization in intel_init.c

	/* we don't use an IDA since we already have a link ID */
	auxdev->id = link_id;

In the auxiliary bus design, the parent has to take care of managing 
this id, be it with an IDA or as we do here with a physical link ID that 
is unique.

in short, I don't see how I could change the code given the differences 
in concepts?

Re: [PATCH] soundwire: intel: move to auxiliary bus

[Pierre-Louis Bossart]

>> Note that the auxiliary bus API has separate init and add steps, which
>> requires more attention in the error unwinding paths. The main loop
>> needs to deal with kfree() and auxiliary_device_uninit() for the
>> current iteration before jumping to the common label which releases
>> everything allocated in prior iterations.
> 
> The init/add steps can be moved together in the aux bus code if that
> makes this usage simpler.  Please do that instead.

IIRC the two steps were separated during the auxbus reviews to allow the 
parent to call kfree() on an init failure, and auxiliary_device_uninit() 
afterwards.

https://www.kernel.org/doc/html/latest/driver-api/auxiliary_bus.html#auxiliary-device

With a single auxbus_register(), the parent wouldn't know whether to use 
kfree() or auxiliary_device_uinit() when an error is returned, would it?

Re: [PATCH] soundwire: intel: move to auxiliary bus

[Pierre-Louis Bossart]

>> Note that the auxiliary bus API has separate init and add steps, which
>> requires more attention in the error unwinding paths. The main loop
>> needs to deal with kfree() and auxiliary_device_uninit() for the
>> current iteration before jumping to the common label which releases
>> everything allocated in prior iterations.
> 
> The init/add steps can be moved together in the aux bus code if that
> makes this usage simpler.  Please do that instead.

IIRC the two steps were separated during the auxbus reviews to allow the 
parent to call kfree() on an init failure, and auxiliary_device_uninit() 
afterwards.

https://www.kernel.org/doc/html/latest/driver-api/auxiliary_bus.html#auxiliary-device

With a single auxbus_register(), the parent wouldn't know whether to use 
kfree() or auxiliary_device_uinit() when an error is returned, would it?

Re: [PATCH] soundwire: intel: move to auxiliary bus

[Greg KH]

On Tue, Mar 23, 2021 at 01:04:49PM -0500, Pierre-Louis Bossart wrote:
> 
> > > Note that the auxiliary bus API has separate init and add steps, which
> > > requires more attention in the error unwinding paths. The main loop
> > > needs to deal with kfree() and auxiliary_device_uninit() for the
> > > current iteration before jumping to the common label which releases
> > > everything allocated in prior iterations.
> > 
> > The init/add steps can be moved together in the aux bus code if that
> > makes this usage simpler.  Please do that instead.
> 
> IIRC the two steps were separated during the auxbus reviews to allow the
> parent to call kfree() on an init failure, and auxiliary_device_uninit()
> afterwards.
> 
> https://www.kernel.org/doc/html/latest/driver-api/auxiliary_bus.html#auxiliary-device
> 
> With a single auxbus_register(), the parent wouldn't know whether to use
> kfree() or auxiliary_device_uinit() when an error is returned, would it?
> 

It should, you know the difference when you call device_register() vs.
device_initialize()/device_add(), for what to do, right?

Should be no difference here either :)

Re: [PATCH] soundwire: intel: move to auxiliary bus

[Greg KH]

On Tue, Mar 23, 2021 at 01:04:49PM -0500, Pierre-Louis Bossart wrote:
> 
> > > Note that the auxiliary bus API has separate init and add steps, which
> > > requires more attention in the error unwinding paths. The main loop
> > > needs to deal with kfree() and auxiliary_device_uninit() for the
> > > current iteration before jumping to the common label which releases
> > > everything allocated in prior iterations.
> > 
> > The init/add steps can be moved together in the aux bus code if that
> > makes this usage simpler.  Please do that instead.
> 
> IIRC the two steps were separated during the auxbus reviews to allow the
> parent to call kfree() on an init failure, and auxiliary_device_uninit()
> afterwards.
> 
> https://www.kernel.org/doc/html/latest/driver-api/auxiliary_bus.html#auxiliary-device
> 
> With a single auxbus_register(), the parent wouldn't know whether to use
> kfree() or auxiliary_device_uinit() when an error is returned, would it?
> 

It should, you know the difference when you call device_register() vs.
device_initialize()/device_add(), for what to do, right?

Should be no difference here either :)

Re: [PATCH] soundwire: intel: move to auxiliary bus

[Pierre-Louis Bossart]

On 3/23/21 1:32 PM, Greg KH wrote:
> On Tue, Mar 23, 2021 at 01:04:49PM -0500, Pierre-Louis Bossart wrote:
>>
>>>> Note that the auxiliary bus API has separate init and add steps, which
>>>> requires more attention in the error unwinding paths. The main loop
>>>> needs to deal with kfree() and auxiliary_device_uninit() for the
>>>> current iteration before jumping to the common label which releases
>>>> everything allocated in prior iterations.
>>>
>>> The init/add steps can be moved together in the aux bus code if that
>>> makes this usage simpler.  Please do that instead.
>>
>> IIRC the two steps were separated during the auxbus reviews to allow the
>> parent to call kfree() on an init failure, and auxiliary_device_uninit()
>> afterwards.
>>
>> https://www.kernel.org/doc/html/latest/driver-api/auxiliary_bus.html#auxiliary-device
>>
>> With a single auxbus_register(), the parent wouldn't know whether to use
>> kfree() or auxiliary_device_uinit() when an error is returned, would it?
>>
> 
> It should, you know the difference when you call device_register() vs.
> device_initialize()/device_add(), for what to do, right?
> 
> Should be no difference here either :)

sorry, not following.

with the regular devices, the errors can only happen on the second "add" 
stage.

int device_register(struct device *dev)
{
	device_initialize(dev);
	return device_add(dev);
}

that's not what is currently implemented for the auxiliary bus

the current flow is

ldev = kzalloc(..)
some inits
ret = auxiliary_device_init(&ldev->auxdev)
if (ret < 0) {
     kfree(ldev);
     goto err1;
}

ret = auxiliary_device_add(&ldev->auxdev)
if (ret < 0)
     auxiliary_device_uninit(&ldev->auxdev)
     goto err2;
}
...
err2:
err1:

How would I convert this to

ldev = kzalloc(..)
some inits
ret = auxiliary_device_register()
if (ret) {
    kfree(ldev) or not?
    unit or not?
}

IIRC during reviews there was an ask that the parent and name be 
checked, and that's why the code added the two checks below:

int auxiliary_device_init(struct auxiliary_device *auxdev)
{
	struct device *dev = &auxdev->dev;

	if (!dev->parent) {
		pr_err("auxiliary_device has a NULL dev->parent\n");
		return -EINVAL;
	}

	if (!auxdev->name) {
		pr_err("auxiliary_device has a NULL name\n");
		return -EINVAL;
	}

	dev->bus = &auxiliary_bus_type;
	device_initialize(&auxdev->dev);
	return 0;
}

does this clarify the sequence?

Re: [PATCH] soundwire: intel: move to auxiliary bus

[Greg KH]

On Tue, Mar 23, 2021 at 02:14:18PM -0500, Pierre-Louis Bossart wrote:
> 
> 
> On 3/23/21 1:32 PM, Greg KH wrote:
> > On Tue, Mar 23, 2021 at 01:04:49PM -0500, Pierre-Louis Bossart wrote:
> > > 
> > > > > Note that the auxiliary bus API has separate init and add steps, which
> > > > > requires more attention in the error unwinding paths. The main loop
> > > > > needs to deal with kfree() and auxiliary_device_uninit() for the
> > > > > current iteration before jumping to the common label which releases
> > > > > everything allocated in prior iterations.
> > > > 
> > > > The init/add steps can be moved together in the aux bus code if that
> > > > makes this usage simpler.  Please do that instead.
> > > 
> > > IIRC the two steps were separated during the auxbus reviews to allow the
> > > parent to call kfree() on an init failure, and auxiliary_device_uninit()
> > > afterwards.
> > > 
> > > https://www.kernel.org/doc/html/latest/driver-api/auxiliary_bus.html#auxiliary-device
> > > 
> > > With a single auxbus_register(), the parent wouldn't know whether to use
> > > kfree() or auxiliary_device_uinit() when an error is returned, would it?
> > > 
> > 
> > It should, you know the difference when you call device_register() vs.
> > device_initialize()/device_add(), for what to do, right?
> > 
> > Should be no difference here either :)
> 
> sorry, not following.
> 
> with the regular devices, the errors can only happen on the second "add"
> stage.
> 
> int device_register(struct device *dev)
> {
> 	device_initialize(dev);
> 	return device_add(dev);
> }
> 
> that's not what is currently implemented for the auxiliary bus
> 
> the current flow is
> 
> ldev = kzalloc(..)
> some inits
> ret = auxiliary_device_init(&ldev->auxdev)
> if (ret < 0) {
>     kfree(ldev);
>     goto err1;
> }
> 
> ret = auxiliary_device_add(&ldev->auxdev)
> if (ret < 0)
>     auxiliary_device_uninit(&ldev->auxdev)
>     goto err2;
> }
> ...
> err2:
> err1:
> 
> How would I convert this to
> 
> ldev = kzalloc(..)
> some inits
> ret = auxiliary_device_register()
> if (ret) {
>    kfree(ldev) or not?
>    unit or not?
> }
> 
> IIRC during reviews there was an ask that the parent and name be checked,
> and that's why the code added the two checks below:
> 
> int auxiliary_device_init(struct auxiliary_device *auxdev)
> {
> 	struct device *dev = &auxdev->dev;
> 
> 	if (!dev->parent) {
> 		pr_err("auxiliary_device has a NULL dev->parent\n");
> 		return -EINVAL;
> 	}
> 
> 	if (!auxdev->name) {
> 		pr_err("auxiliary_device has a NULL name\n");
> 		return -EINVAL;
> 	}
> 
> 	dev->bus = &auxiliary_bus_type;
> 	device_initialize(&auxdev->dev);
> 	return 0;
> }
> 
> does this clarify the sequence?

Yes, thanks, but I don't know the answer to your question, sorry.  This
feels more complex than it should be, but I do not have the time at the
moment to look into it, sorry.

Try getting the authors of this code to fix it up :)

thanks,

greg k-h

Re: [PATCH] soundwire: intel: move to auxiliary bus

[Vinod Koul]

On 23-03-21, 12:29, Pierre-Louis Bossart wrote:
> Thanks Greg and Vinod for the reviews
> 
> > > > -static int intel_master_probe(struct platform_device *pdev)
> > > > +static int intel_link_probe(struct auxiliary_device *auxdev, const struct auxiliary_device_id *id)
> > > >   {
> > > > -	struct device *dev = &pdev->dev;
> > > > +	struct device *dev = &auxdev->dev;
> > > > +	struct sdw_intel_link_dev *ldev = auxiliary_dev_to_sdw_intel_link_dev(auxdev);
> > > 
> > > Do we need another abstractions for resources here, why not aux dev
> > > creation set the resources required and we skip this step...
> 
> Not sure what resources you are referring to?

Resources in the sdw_intel_link_dev which are sdw_intel_link_res. They
should be resources for auxdev and if you do that lets you get rid of
this abstraction.

> 
> this is just a container_of() and the documented way of extending the auxbus
> (see https://www.kernel.org/doc/html/latest/driver-api/auxiliary_bus.html#example-usage)
> 
> 
> struct sdw_intel_link_dev {
> 	struct auxiliary_device auxdev;
> 	struct sdw_intel_link_res link_res;
> };
> 
> #define auxiliary_dev_to_sdw_intel_link_dev(auxiliary_dev) \
> 	container_of(auxiliary_dev, struct sdw_intel_link_dev, auxdev)
> 
> > > >   	struct sdw_intel *sdw;
> > > >   	struct sdw_cdns *cdns;
> > > >   	struct sdw_bus *bus;
> > > > @@ -1346,14 +1347,14 @@ static int intel_master_probe(struct platform_device *pdev)
> > > >   	cdns = &sdw->cdns;
> > > >   	bus = &cdns->bus;
> > > > -	sdw->instance = pdev->id;
> > > > -	sdw->link_res = dev_get_platdata(dev);
> > > > +	sdw->instance = auxdev->id;
> > > 
> > > so auxdev has id and still we pass id as argument :( Not sure if folks
> > > can fix this now
> > 
> > That's odd, yeah, it should be fixed.
> 
> I think we are talking about different things?
> 
> this is defined in mod_devicetable.h:
> 
> struct auxiliary_device_id {
> 	char name[AUXILIARY_NAME_SIZE];
> 	kernel_ulong_t driver_data;
> };
> 
> and used for the driver probe:
> 
> 	ret = auxdrv->probe(auxdev, auxiliary_match_id(auxdrv->id_table, auxdev));
> 
> but there is a separate id:
> 
> struct auxiliary_device {
> 	struct device dev;
> 	const char *name;
> 	u32 id;
> };
> 
> which is set during the device initialization in intel_init.c
> 
> 	/* we don't use an IDA since we already have a link ID */
> 	auxdev->id = link_id;
> 
> In the auxiliary bus design, the parent has to take care of managing this
> id, be it with an IDA or as we do here with a physical link ID that is
> unique.

Aha, maybe both of them should not be 'id' to avoid this confusion!

That also reminds me that we have duplicate info:

+       sdw->instance = auxdev->id;
+       bus->link_id = auxdev->id;

drop the local driver instance and use bus->link_id please

> in short, I don't see how I could change the code given the differences in
> concepts?

-- 
~Vinod

Re: [PATCH] soundwire: intel: move to auxiliary bus

[Vinod Koul]

On 23-03-21, 12:29, Pierre-Louis Bossart wrote:
> Thanks Greg and Vinod for the reviews
> 
> > > > -static int intel_master_probe(struct platform_device *pdev)
> > > > +static int intel_link_probe(struct auxiliary_device *auxdev, const struct auxiliary_device_id *id)
> > > >   {
> > > > -	struct device *dev = &pdev->dev;
> > > > +	struct device *dev = &auxdev->dev;
> > > > +	struct sdw_intel_link_dev *ldev = auxiliary_dev_to_sdw_intel_link_dev(auxdev);
> > > 
> > > Do we need another abstractions for resources here, why not aux dev
> > > creation set the resources required and we skip this step...
> 
> Not sure what resources you are referring to?

Resources in the sdw_intel_link_dev which are sdw_intel_link_res. They
should be resources for auxdev and if you do that lets you get rid of
this abstraction.

> 
> this is just a container_of() and the documented way of extending the auxbus
> (see https://www.kernel.org/doc/html/latest/driver-api/auxiliary_bus.html#example-usage)
> 
> 
> struct sdw_intel_link_dev {
> 	struct auxiliary_device auxdev;
> 	struct sdw_intel_link_res link_res;
> };
> 
> #define auxiliary_dev_to_sdw_intel_link_dev(auxiliary_dev) \
> 	container_of(auxiliary_dev, struct sdw_intel_link_dev, auxdev)
> 
> > > >   	struct sdw_intel *sdw;
> > > >   	struct sdw_cdns *cdns;
> > > >   	struct sdw_bus *bus;
> > > > @@ -1346,14 +1347,14 @@ static int intel_master_probe(struct platform_device *pdev)
> > > >   	cdns = &sdw->cdns;
> > > >   	bus = &cdns->bus;
> > > > -	sdw->instance = pdev->id;
> > > > -	sdw->link_res = dev_get_platdata(dev);
> > > > +	sdw->instance = auxdev->id;
> > > 
> > > so auxdev has id and still we pass id as argument :( Not sure if folks
> > > can fix this now
> > 
> > That's odd, yeah, it should be fixed.
> 
> I think we are talking about different things?
> 
> this is defined in mod_devicetable.h:
> 
> struct auxiliary_device_id {
> 	char name[AUXILIARY_NAME_SIZE];
> 	kernel_ulong_t driver_data;
> };
> 
> and used for the driver probe:
> 
> 	ret = auxdrv->probe(auxdev, auxiliary_match_id(auxdrv->id_table, auxdev));
> 
> but there is a separate id:
> 
> struct auxiliary_device {
> 	struct device dev;
> 	const char *name;
> 	u32 id;
> };
> 
> which is set during the device initialization in intel_init.c
> 
> 	/* we don't use an IDA since we already have a link ID */
> 	auxdev->id = link_id;
> 
> In the auxiliary bus design, the parent has to take care of managing this
> id, be it with an IDA or as we do here with a physical link ID that is
> unique.

Aha, maybe both of them should not be 'id' to avoid this confusion!

That also reminds me that we have duplicate info:

+       sdw->instance = auxdev->id;
+       bus->link_id = auxdev->id;

drop the local driver instance and use bus->link_id please

> in short, I don't see how I could change the code given the differences in
> concepts?

-- 
~Vinod

Re: [PATCH] soundwire: intel: move to auxiliary bus

[Pierre-Louis Bossart]

>>>>>> Note that the auxiliary bus API has separate init and add steps, which
>>>>>> requires more attention in the error unwinding paths. The main loop
>>>>>> needs to deal with kfree() and auxiliary_device_uninit() for the
>>>>>> current iteration before jumping to the common label which releases
>>>>>> everything allocated in prior iterations.
>>>>>
>>>>> The init/add steps can be moved together in the aux bus code if that
>>>>> makes this usage simpler.  Please do that instead.
>>>>
>>>> IIRC the two steps were separated during the auxbus reviews to allow the
>>>> parent to call kfree() on an init failure, and auxiliary_device_uninit()
>>>> afterwards.
>>>>
>>>> https://www.kernel.org/doc/html/latest/driver-api/auxiliary_bus.html#auxiliary-device
>>>>
>>>> With a single auxbus_register(), the parent wouldn't know whether to use
>>>> kfree() or auxiliary_device_uinit() when an error is returned, would it?
>>>>
>>>
>>> It should, you know the difference when you call device_register() vs.
>>> device_initialize()/device_add(), for what to do, right?
>>>
>>> Should be no difference here either :)
>>
>> sorry, not following.
>>
>> with the regular devices, the errors can only happen on the second "add"
>> stage.
>>
>> int device_register(struct device *dev)
>> {
>> 	device_initialize(dev);
>> 	return device_add(dev);
>> }
>>
>> that's not what is currently implemented for the auxiliary bus
>>
>> the current flow is
>>
>> ldev = kzalloc(..)
>> some inits
>> ret = auxiliary_device_init(&ldev->auxdev)
>> if (ret < 0) {
>>      kfree(ldev);
>>      goto err1;
>> }
>>
>> ret = auxiliary_device_add(&ldev->auxdev)
>> if (ret < 0)
>>      auxiliary_device_uninit(&ldev->auxdev)
>>      goto err2;
>> }
>> ...
>> err2:
>> err1:
>>
>> How would I convert this to
>>
>> ldev = kzalloc(..)
>> some inits
>> ret = auxiliary_device_register()
>> if (ret) {
>>     kfree(ldev) or not?
>>     unit or not?
>> }
>>
>> IIRC during reviews there was an ask that the parent and name be checked,
>> and that's why the code added the two checks below:
>>
>> int auxiliary_device_init(struct auxiliary_device *auxdev)
>> {
>> 	struct device *dev = &auxdev->dev;
>>
>> 	if (!dev->parent) {
>> 		pr_err("auxiliary_device has a NULL dev->parent\n");
>> 		return -EINVAL;
>> 	}
>>
>> 	if (!auxdev->name) {
>> 		pr_err("auxiliary_device has a NULL name\n");
>> 		return -EINVAL;
>> 	}
>>
>> 	dev->bus = &auxiliary_bus_type;
>> 	device_initialize(&auxdev->dev);
>> 	return 0;
>> }
>>
>> does this clarify the sequence?
> 
> Yes, thanks, but I don't know the answer to your question, sorry.  This
> feels more complex than it should be, but I do not have the time at the
> moment to look into it, sorry.
> 
> Try getting the authors of this code to fix it up :)

We can try to check why those two tests were added before initialize(), 
I don't fully recall these details

If we could move these tests after device_initialize() then we could add 
a _register function.

Note at this point it would mean an API change and impact the existing 
Nvidia/Mellanox code, we are using the same sequence as them

https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/mellanox/mlx5/core/dev.c#L262

Re: [PATCH] soundwire: intel: move to auxiliary bus

[Pierre-Louis Bossart]

On 3/24/21 5:50 AM, Vinod Koul wrote:
> On 23-03-21, 12:29, Pierre-Louis Bossart wrote:
>> Thanks Greg and Vinod for the reviews
>>
>>>>> -static int intel_master_probe(struct platform_device *pdev)
>>>>> +static int intel_link_probe(struct auxiliary_device *auxdev, const struct auxiliary_device_id *id)
>>>>>    {
>>>>> -	struct device *dev = &pdev->dev;
>>>>> +	struct device *dev = &auxdev->dev;
>>>>> +	struct sdw_intel_link_dev *ldev = auxiliary_dev_to_sdw_intel_link_dev(auxdev);
>>>>
>>>> Do we need another abstractions for resources here, why not aux dev
>>>> creation set the resources required and we skip this step...
>>
>> Not sure what resources you are referring to?
> 
> Resources in the sdw_intel_link_dev which are sdw_intel_link_res. They
> should be resources for auxdev and if you do that lets you get rid of
> this abstraction.

Sorry Vinod, I am not following your line of thought. We must be talking 
of different things or having a different understanding of what the 
auxiliary device is.

The auxiliary device is deliberately minimal by design and does not 
contain domain-specific information/resources/pointers/pdata as the 
platform_device does. You extend it by defining a larger structure that 
contains an auxiliary device and whatever domain-specific 
fields/structures/domains are needed, then use container_of to access it.

It's not just Intel doing this, the first example from Mellanox uses the 
same pattern, albeit with a single pointer instead of the structure we used.

see e.g. 
https://elixir.bootlin.com/linux/latest/source/include/linux/mlx5/driver.h#L545

So I am not sure what you mean by 'rid of this abstraction' when this 
abstraction is pretty much the way things were designed?

Maybe an example of what sort of structure you had in mind would help?


>> this is just a container_of() and the documented way of extending the auxbus
>> (see https://www.kernel.org/doc/html/latest/driver-api/auxiliary_bus.html#example-usage)
>>
>>
>> struct sdw_intel_link_dev {
>> 	struct auxiliary_device auxdev;
>> 	struct sdw_intel_link_res link_res;
>> };
>>
>> #define auxiliary_dev_to_sdw_intel_link_dev(auxiliary_dev) \
>> 	container_of(auxiliary_dev, struct sdw_intel_link_dev, auxdev)
>>
>>>>>    	struct sdw_intel *sdw;
>>>>>    	struct sdw_cdns *cdns;
>>>>>    	struct sdw_bus *bus;
>>>>> @@ -1346,14 +1347,14 @@ static int intel_master_probe(struct platform_device *pdev)
>>>>>    	cdns = &sdw->cdns;
>>>>>    	bus = &cdns->bus;
>>>>> -	sdw->instance = pdev->id;
>>>>> -	sdw->link_res = dev_get_platdata(dev);
>>>>> +	sdw->instance = auxdev->id;
>>>>
>>>> so auxdev has id and still we pass id as argument :( Not sure if folks
>>>> can fix this now
>>>
>>> That's odd, yeah, it should be fixed.
>>
>> I think we are talking about different things?
>>
>> this is defined in mod_devicetable.h:
>>
>> struct auxiliary_device_id {
>> 	char name[AUXILIARY_NAME_SIZE];
>> 	kernel_ulong_t driver_data;
>> };
>>
>> and used for the driver probe:
>>
>> 	ret = auxdrv->probe(auxdev, auxiliary_match_id(auxdrv->id_table, auxdev));
>>
>> but there is a separate id:
>>
>> struct auxiliary_device {
>> 	struct device dev;
>> 	const char *name;
>> 	u32 id;
>> };
>>
>> which is set during the device initialization in intel_init.c
>>
>> 	/* we don't use an IDA since we already have a link ID */
>> 	auxdev->id = link_id;
>>
>> In the auxiliary bus design, the parent has to take care of managing this
>> id, be it with an IDA or as we do here with a physical link ID that is
>> unique.
> 
> Aha, maybe both of them should not be 'id' to avoid this confusion!

the function definition follows the expected prototype

struct auxiliary_driver {
         int (*probe)(struct auxiliary_device *,
                      const struct auxiliary_device_id *id);

we can rename the argument to e.g. dev_id if that helps. Suggestions 
welcome.

> That also reminds me that we have duplicate info:
> 
> +       sdw->instance = auxdev->id;
> +       bus->link_id = auxdev->id;
> 
> drop the local driver instance and use bus->link_id please

if you are referring to sdw->instance, it could probably be removed, but 
that would need to be a separate cleanup changing cadence_master.c as 
well. this patch only changes pdev->id with auxdev->id and provides only 
the transition from platform device to auxiliary device.

Re: [PATCH] soundwire: intel: move to auxiliary bus

[Greg KH]

On Wed, Mar 24, 2021 at 09:55:01AM -0500, Pierre-Louis Bossart wrote:
> Note at this point it would mean an API change and impact the existing
> Nvidia/Mellanox code, we are using the same sequence as them

THere is no "stable api" in the kernel, so if something has to change,
that's fine, we can change the users at the same time, not an issue.

thanks,

greg k-h

Re: [PATCH] soundwire: intel: move to auxiliary bus

[Pierre-Louis Bossart]

On 3/24/21 10:36 AM, Greg KH wrote:
> On Wed, Mar 24, 2021 at 09:55:01AM -0500, Pierre-Louis Bossart wrote:
>> Note at this point it would mean an API change and impact the existing
>> Nvidia/Mellanox code, we are using the same sequence as them
> 
> THere is no "stable api" in the kernel, so if something has to change,
> that's fine, we can change the users at the same time, not an issue.

What I meant is that this requires consensus to make a change, and so 
far I haven't seen any burning desire from the contributors to revisit 
the 2-step sequence.

I will however modify the code in this patch to implement a SoundWire 
'linkdev' register/unregister function, it'll be much easier to review 
and maintain, and will follow the same pattern as the mlx5 code (all 
errors and domain-specific initializations handled in the same 
function). Draft code being tested is at 
https://github.com/thesofproject/linux/pull/2809