[RFC 0/5] soundwire: create master device and use it

[RFC 0/5] soundwire: create master device and use it

[Bard Liao]

This series adds sdw master devices and use it instead of platform devices.

An open needs to be addressed is that should we use differrent bus type for
sdw master and slave devices?

Pierre-Louis Bossart (4):
  soundwire: bus_type: add sdw_master_device support
  soundwire: qcom: fix error handling in probe
  soundwire: qcom: add sdw_master_device support
  soundwire: intel: transition to sdw_master_device

Srinivas Kandagatla (1):
  soundwire: master: use device node pointer from master device

 drivers/soundwire/Makefile         |   2 +-
 drivers/soundwire/bus.h            |   2 +
 drivers/soundwire/bus_type.c       |  19 +-
 drivers/soundwire/intel.c          |  98 ++++++----
 drivers/soundwire/intel.h          |   8 +-
 drivers/soundwire/intel_init.c     | 293 ++++++++++++++++++++++-------
 drivers/soundwire/master.c         | 168 +++++++++++++++++
 drivers/soundwire/qcom.c           |  59 ++++--
 drivers/soundwire/slave.c          |   8 +-
 include/linux/soundwire/sdw.h      |  60 ++++++
 include/linux/soundwire/sdw_type.h |  10 +-
 11 files changed, 601 insertions(+), 126 deletions(-)
 create mode 100644 drivers/soundwire/master.c

-- 
2.17.1

[RFC 1/5] soundwire: bus_type: add sdw_master_device support

[Bard Liao]

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

In the existing SoundWire code, Master Devices are not explicitly
represented - only SoundWire Slave Devices are exposed (the use of
capital letters follows the SoundWire specification conventions).

The SoundWire Master Device provides the clock, synchronization
information and command/control channels. When multiple links are
supported, a Controller may expose more than one Master Device; they
are typically embedded inside a larger audio cluster (be it in an
SOC/chipset or an external audio codec), and we need to describe it
using the Linux device and driver model.  This will allow for
configuration functions to account for external dependencies such as
power rails, clock sources or wake-up mechanisms. This transition will
also allow for better sysfs support without the reference count issues
mentioned in the initial reviews.

In this patch, we convert the existing code to use an explicit
sdw_slave_type, then define a sdw_master_device structure.

A parent (such as the Intel audio controller or its equivalent on
Qualcomm devices) would use sdw_master_device_add() to create the
device, the master device would be released when
sdw_master_device_del() is invoked by the parent.

The 'Master Device' device can be configured with optional link ops,
in case an implementation needs link-specific resources allocated or
power-management capabilities. The .add/.del callbacks are handled
internally by the SoundWire core, while the optional
.startup/.process_wake need to be called by the parent directly (using
wrappers). The uevent handling is moved to the slave side since it's
not relevant for master devices.

Additional callbacks will be added in the future for e.g. autonomous
clock stop modes.

Credits to Jaroslav Kysela <perex@perex.cz> for the idea of using link_ops.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
---
 drivers/soundwire/Makefile         |   2 +-
 drivers/soundwire/bus.h            |   2 +
 drivers/soundwire/bus_type.c       |  19 ++--
 drivers/soundwire/master.c         | 167 +++++++++++++++++++++++++++++
 drivers/soundwire/slave.c          |   8 +-
 include/linux/soundwire/sdw.h      |  60 +++++++++++
 include/linux/soundwire/sdw_type.h |  10 +-
 7 files changed, 258 insertions(+), 10 deletions(-)
 create mode 100644 drivers/soundwire/master.c

diff --git a/drivers/soundwire/Makefile b/drivers/soundwire/Makefile
index e2cdff990e9f..7319918e0aec 100644
--- a/drivers/soundwire/Makefile
+++ b/drivers/soundwire/Makefile
@@ -4,7 +4,7 @@
 #
 
 #Bus Objs
-soundwire-bus-objs := bus_type.o bus.o slave.o mipi_disco.o stream.o
+soundwire-bus-objs := bus_type.o bus.o master.o slave.o mipi_disco.o stream.o
 obj-$(CONFIG_SOUNDWIRE) += soundwire-bus.o
 
 ifdef CONFIG_DEBUG_FS
diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h
index 204204a26db8..99943d40222a 100644
--- a/drivers/soundwire/bus.h
+++ b/drivers/soundwire/bus.h
@@ -173,4 +173,6 @@ sdw_update(struct sdw_slave *slave, u32 addr, u8 mask, u8 val)
 
 void sdw_clear_slave_status(struct sdw_bus *bus, u32 request);
 
+int sdw_slave_uevent(struct device *dev, struct kobj_uevent_env *env);
+
 #endif /* __SDW_BUS_H */
diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c
index 17f096dd6806..2c1a19caba51 100644
--- a/drivers/soundwire/bus_type.c
+++ b/drivers/soundwire/bus_type.c
@@ -33,13 +33,21 @@ sdw_get_device_id(struct sdw_slave *slave, struct sdw_driver *drv)
 
 static int sdw_bus_match(struct device *dev, struct device_driver *ddrv)
 {
-	struct sdw_slave *slave = dev_to_sdw_dev(dev);
-	struct sdw_driver *drv = drv_to_sdw_driver(ddrv);
+	struct sdw_slave *slave;
+	struct sdw_driver *drv;
+	int ret = 0;
+
+	if (is_sdw_slave(dev)) {
+		slave = dev_to_sdw_dev(dev);
+		drv = drv_to_sdw_driver(ddrv);
 
-	return !!sdw_get_device_id(slave, drv);
+		ret = !!sdw_get_device_id(slave, drv);
+	}
+	return ret;
 }
 
-int sdw_slave_modalias(const struct sdw_slave *slave, char *buf, size_t size)
+static int sdw_slave_modalias(const struct sdw_slave *slave, char *buf,
+			      size_t size)
 {
 	/* modalias is sdw:m<mfg_id>p<part_id> */
 
@@ -47,7 +55,7 @@ int sdw_slave_modalias(const struct sdw_slave *slave, char *buf, size_t size)
 			slave->id.mfg_id, slave->id.part_id);
 }
 
-static int sdw_uevent(struct device *dev, struct kobj_uevent_env *env)
+int sdw_slave_uevent(struct device *dev, struct kobj_uevent_env *env)
 {
 	struct sdw_slave *slave = dev_to_sdw_dev(dev);
 	char modalias[32];
@@ -63,7 +71,6 @@ static int sdw_uevent(struct device *dev, struct kobj_uevent_env *env)
 struct bus_type sdw_bus_type = {
 	.name = "soundwire",
 	.match = sdw_bus_match,
-	.uevent = sdw_uevent,
 };
 EXPORT_SYMBOL_GPL(sdw_bus_type);
 
diff --git a/drivers/soundwire/master.c b/drivers/soundwire/master.c
new file mode 100644
index 000000000000..094518817601
--- /dev/null
+++ b/drivers/soundwire/master.c
@@ -0,0 +1,167 @@
+// SPDX-License-Identifier: GPL-2.0-only
+// Copyright(c) 2019-2020 Intel Corporation.
+
+#include <linux/device.h>
+#include <linux/acpi.h>
+#include <linux/soundwire/sdw.h>
+#include <linux/soundwire/sdw_type.h>
+#include "bus.h"
+
+static void sdw_master_device_release(struct device *dev)
+{
+	struct sdw_master_device *md = dev_to_sdw_master_device(dev);
+
+	kfree(md);
+}
+
+struct device_type sdw_master_type = {
+	.name =		"soundwire_master",
+	.release =	sdw_master_device_release,
+};
+
+/**
+ * sdw_master_device_add() - create a Linux Master Device representation.
+ * @parent: the parent Linux device (e.g. a PCI device)
+ * @fwnode: the parent fwnode (e.g. an ACPI companion device to the parent)
+ * @link_ops: link-specific ops (optional)
+ * @link_id: link index as defined by MIPI DisCo specification
+ * @pdata: private data (e.g. register base, offsets, platform quirks, etc).
+ *
+ * The link_ops argument can be NULL, it is only used when link-specific
+ * initializations and power-management are required.
+ */
+struct sdw_master_device
+*sdw_master_device_add(struct device *parent,
+		       struct fwnode_handle *fwnode,
+		       struct sdw_link_ops *link_ops,
+		       int link_id,
+		       void *pdata)
+{
+	struct sdw_master_device *md;
+	int ret;
+
+	md = kzalloc(sizeof(*md), GFP_KERNEL);
+	if (!md)
+		return ERR_PTR(-ENOMEM);
+
+	md->link_id = link_id;
+	md->pdata = pdata;
+	md->link_ops = link_ops;
+
+	md->dev.parent = parent;
+	md->dev.fwnode = fwnode;
+	md->dev.bus = &sdw_bus_type;
+	md->dev.type = &sdw_master_type;
+	md->dev.dma_mask = md->dev.parent->dma_mask;
+	dev_set_name(&md->dev, "sdw-master-%d", md->link_id);
+
+	if (link_ops && link_ops->driver) {
+		/*
+		 * A driver is only needed for ASoC integration (need
+		 * driver->name) and for link-specific power management
+		 * w/ a pm_dev_ops structure.
+		 *
+		 * The driver needs to be registered by the parent
+		 */
+		md->dev.driver = link_ops->driver;
+	}
+
+	ret = device_register(&md->dev);
+	if (ret) {
+		dev_err(parent, "Failed to add master: ret %d\n", ret);
+		/*
+		 * On err, don't free but drop ref as this will be freed
+		 * when release method is invoked.
+		 */
+		put_device(&md->dev);
+		goto device_register_err;
+	}
+
+	if (link_ops && link_ops->add) {
+		ret = link_ops->add(md, pdata);
+		if (ret < 0) {
+			dev_err(&md->dev, "link_ops add callback failed: %d\n",
+				ret);
+			goto link_add_err;
+		}
+	}
+
+	return md;
+
+link_add_err:
+	device_unregister(&md->dev);
+device_register_err:
+	return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(sdw_master_device_add);
+
+/**
+ * sdw_master_device_del() - delete a Linux Master Device representation.
+ * @md: the master device
+ *
+ * This function is the dual of sdw_master_device_add(), itreleases
+ * all link-specific resources and unregisters the device.
+ */
+int sdw_master_device_del(struct sdw_master_device *md)
+{
+	int ret = 0;
+
+	if (md && md->link_ops && md->link_ops->del) {
+		ret = md->link_ops->del(md);
+		if (ret < 0) {
+			dev_err(&md->dev, "link_ops del callback failed: %d\n",
+				ret);
+			return ret;
+		}
+	}
+
+	device_unregister(&md->dev);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(sdw_master_device_del);
+
+/**
+ * sdw_master_device_startup() - startup hardware
+ *
+ * @md: Linux Soundwire master device
+ */
+int sdw_master_device_startup(struct sdw_master_device *md)
+{
+	struct sdw_link_ops *link_ops;
+	int ret = 0;
+
+	if (IS_ERR_OR_NULL(md))
+		return -EINVAL;
+
+	link_ops = md->link_ops;
+
+	if (link_ops && link_ops->startup)
+		ret = link_ops->startup(md);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(sdw_master_device_startup);
+
+/**
+ * sdw_master_device_process_wake_event() - handle external wake
+ * event, e.g. handled at the PCI level
+ *
+ * @md: Linux Soundwire master device
+ */
+int sdw_master_device_process_wake_event(struct sdw_master_device *md)
+{
+	struct sdw_link_ops *link_ops;
+	int ret = 0;
+
+	if (IS_ERR_OR_NULL(md))
+		return -EINVAL;
+
+	link_ops = md->link_ops;
+
+	if (link_ops && link_ops->process_wake_event)
+		ret = link_ops->process_wake_event(md);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(sdw_master_device_process_wake_event);
diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c
index aace57fae7f8..ed068a004bd9 100644
--- a/drivers/soundwire/slave.c
+++ b/drivers/soundwire/slave.c
@@ -14,6 +14,12 @@ static void sdw_slave_release(struct device *dev)
 	kfree(slave);
 }
 
+struct device_type sdw_slave_type = {
+	.name =		"sdw_slave",
+	.release =	sdw_slave_release,
+	.uevent =	sdw_slave_uevent,
+};
+
 static int sdw_slave_add(struct sdw_bus *bus,
 			 struct sdw_slave_id *id, struct fwnode_handle *fwnode)
 {
@@ -41,9 +47,9 @@ static int sdw_slave_add(struct sdw_bus *bus,
 			     id->class_id, id->unique_id);
 	}
 
-	slave->dev.release = sdw_slave_release;
 	slave->dev.bus = &sdw_bus_type;
 	slave->dev.of_node = of_node_get(to_of_node(fwnode));
+	slave->dev.type = &sdw_slave_type;
 	slave->bus = bus;
 	slave->status = SDW_SLAVE_UNATTACHED;
 	init_completion(&slave->enumeration_complete);
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index 00f5826092e3..ecd070df4ae9 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -632,6 +632,53 @@ struct sdw_slave {
 
 #define dev_to_sdw_dev(_dev) container_of(_dev, struct sdw_slave, dev)
 
+struct sdw_link_ops;
+
+/**
+ * struct sdw_master_device - SoundWire 'Master Device' representation
+ *
+ * @dev: Linux device for this Master
+ * @bus: Bus handle
+ * @link_ops: link-specific ops, initialized with sdw_master_device_add()
+ * @link_id: link index as defined by MIPI DisCo specification
+ * @pdata: private data typically provided with sdw_master_device_add()
+ *
+ * link_ops can be NULL when link-level initializations and power-management
+ * are not desired.
+ */
+struct sdw_master_device {
+	struct device dev;
+	struct sdw_bus *bus;
+	struct sdw_link_ops *link_ops;
+	int link_id;
+	void *pdata;
+};
+
+/**
+ * struct sdw_link_ops - SoundWire link-specific ops
+ * @add: initializations and allocation (hardware may not be enabled yet)
+ * @startup: initialization handled after the hardware is enabled, all
+ * clock/power dependencies are available
+ * @del: free all remaining resources
+ * @process_wake_event: handle external wake
+ * @driver: raw structure used for name/PM hooks.
+ *
+ * This optional structure is provided for link specific
+ * operations. All members are optional, but if .add() is supported the
+ * dual .del() function shall be used to release all resources allocated
+ * in .add().
+ */
+struct sdw_link_ops {
+	int (*add)(struct sdw_master_device *md, void *link_ctx);
+	int (*startup)(struct sdw_master_device *md);
+	int (*del)(struct sdw_master_device *md);
+	int (*process_wake_event)(struct sdw_master_device *md);
+	struct device_driver *driver;
+};
+
+#define dev_to_sdw_master_device(d)	\
+	container_of(d, struct sdw_master_device, dev)
+
 struct sdw_driver {
 	const char *name;
 
@@ -835,6 +882,19 @@ struct sdw_bus {
 int sdw_add_bus_master(struct sdw_bus *bus);
 void sdw_delete_bus_master(struct sdw_bus *bus);
 
+struct sdw_master_device
+*sdw_master_device_add(struct device *parent,
+		       struct fwnode_handle *fwnode,
+		       struct sdw_link_ops *master_ops,
+		       int link_id,
+		       void *pdata);
+
+int sdw_master_device_del(struct sdw_master_device *md);
+
+int sdw_master_device_startup(struct sdw_master_device *md);
+
+int sdw_master_device_process_wake_event(struct sdw_master_device *md);
+
 /**
  * sdw_port_config: Master or Slave Port configuration
  *
diff --git a/include/linux/soundwire/sdw_type.h b/include/linux/soundwire/sdw_type.h
index aaa7f4267c14..990857eddff8 100644
--- a/include/linux/soundwire/sdw_type.h
+++ b/include/linux/soundwire/sdw_type.h
@@ -5,6 +5,13 @@
 #define __SOUNDWIRE_TYPES_H
 
 extern struct bus_type sdw_bus_type;
+extern struct device_type sdw_slave_type;
+extern struct device_type sdw_master_type;
+
+static inline int is_sdw_slave(const struct device *dev)
+{
+	return dev->type == &sdw_slave_type;
+}
 
 #define drv_to_sdw_driver(_drv) container_of(_drv, struct sdw_driver, driver)
 
@@ -14,8 +21,6 @@ extern struct bus_type sdw_bus_type;
 int __sdw_register_driver(struct sdw_driver *drv, struct module *owner);
 void sdw_unregister_driver(struct sdw_driver *drv);
 
-int sdw_slave_modalias(const struct sdw_slave *slave, char *buf, size_t size);
-
 /**
  * module_sdw_driver() - Helper macro for registering a Soundwire driver
  * @__sdw_driver: soundwire slave driver struct
@@ -27,4 +32,5 @@ int sdw_slave_modalias(const struct sdw_slave *slave, char *buf, size_t size);
 #define module_sdw_driver(__sdw_driver) \
 	module_driver(__sdw_driver, sdw_register_driver, \
 			sdw_unregister_driver)
+
 #endif /* __SOUNDWIRE_TYPES_H */
-- 
2.17.1

Re: [RFC 1/5] soundwire: bus_type: add sdw_master_device support

[Vinod Koul]

Hello Bard,

On 17-04-20, 04:55, Bard Liao wrote:
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> 
> In the existing SoundWire code, Master Devices are not explicitly
> represented - only SoundWire Slave Devices are exposed (the use of
> capital letters follows the SoundWire specification conventions).
> 
> The SoundWire Master Device provides the clock, synchronization
> information and command/control channels. When multiple links are
> supported, a Controller may expose more than one Master Device; they
> are typically embedded inside a larger audio cluster (be it in an
> SOC/chipset or an external audio codec), and we need to describe it
> using the Linux device and driver model.  This will allow for
> configuration functions to account for external dependencies such as
> power rails, clock sources or wake-up mechanisms. This transition will
> also allow for better sysfs support without the reference count issues
> mentioned in the initial reviews.

Well the primary reason for doing sdw_master_device for creating a
adding sysfs representation. It *also* helps some vendors due to
inherent model should not be constructed as the primary approach for the
sdw_master_device.

> In this patch, we convert the existing code to use an explicit
> sdw_slave_type, then define a sdw_master_device structure.

Please split that up, we should do the conversions required first and
then do addition of new things.

> +struct device_type sdw_master_type = {
> +	.name =		"soundwire_master",
> +	.release =	sdw_master_device_release,
> +};
> +
> +/**
> + * sdw_master_device_add() - create a Linux Master Device representation.
> + * @parent: the parent Linux device (e.g. a PCI device)
> + * @fwnode: the parent fwnode (e.g. an ACPI companion device to the parent)
> + * @link_ops: link-specific ops (optional)
> + * @link_id: link index as defined by MIPI DisCo specification
> + * @pdata: private data (e.g. register base, offsets, platform quirks, etc).
> + *
> + * The link_ops argument can be NULL, it is only used when link-specific
> + * initializations and power-management are required.
> + */
> +struct sdw_master_device
> +*sdw_master_device_add(struct device *parent,
> +		       struct fwnode_handle *fwnode,
> +		       struct sdw_link_ops *link_ops,
> +		       int link_id,
> +		       void *pdata)
> +{
> +	struct sdw_master_device *md;
> +	int ret;
> +
> +	md = kzalloc(sizeof(*md), GFP_KERNEL);
> +	if (!md)
> +		return ERR_PTR(-ENOMEM);
> +
> +	md->link_id = link_id;
> +	md->pdata = pdata;
> +	md->link_ops = link_ops;
> +
> +	md->dev.parent = parent;
> +	md->dev.fwnode = fwnode;
> +	md->dev.bus = &sdw_bus_type;
> +	md->dev.type = &sdw_master_type;
> +	md->dev.dma_mask = md->dev.parent->dma_mask;
> +	dev_set_name(&md->dev, "sdw-master-%d", md->link_id);
> +
> +	if (link_ops && link_ops->driver) {
> +		/*
> +		 * A driver is only needed for ASoC integration (need
> +		 * driver->name) and for link-specific power management
> +		 * w/ a pm_dev_ops structure.

That is not true for everyone, it is only true for Intel, pls call that
out as well...

> +		 *
> +		 * The driver needs to be registered by the parent
> +		 */
> +		md->dev.driver = link_ops->driver;
> +	}
> +
> +	ret = device_register(&md->dev);
> +	if (ret) {
> +		dev_err(parent, "Failed to add master: ret %d\n", ret);
> +		/*
> +		 * On err, don't free but drop ref as this will be freed
> +		 * when release method is invoked.
> +		 */
> +		put_device(&md->dev);
> +		goto device_register_err;
> +	}
> +
> +	if (link_ops && link_ops->add) {
> +		ret = link_ops->add(md, pdata);
> +		if (ret < 0) {
> +			dev_err(&md->dev, "link_ops add callback failed: %d\n",
> +				ret);
> +			goto link_add_err;
> +		}
> +	}
> +
> +	return md;
> +
> +link_add_err:
> +	device_unregister(&md->dev);
> +device_register_err:
> +	return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL_GPL(sdw_master_device_add);

This looks better than last version for sure. But I would like this to
be split into two parts, a generic sdw_master_device addition without
the link_ops parts. And then the link_ops parts..

As discussed earlier with you, I see no reason why users should have two
APIs. We should fold the sdw_master_device_add() within the
sdw_add_bus_master() afterall as part of adding bus, we should be
creating the sdw_master_dev as well as sdw_slave.

Since you have additional link_ops, we can pass that to
sdw_add_bus_master() (set to NULL for rest) and then call
sdw_master_device_add() internally..

As requested above, please split this to separate patches, first generic
sdw_master_device addition and calling from sdw_add_bus_master() and
then adding link_ops parts for Intel.

Ofcourse any preparatory patches should come before that.

-- 
~Vinod

Re: [RFC 1/5] soundwire: bus_type: add sdw_master_device support

[Greg KH]

On Mon, Apr 20, 2020 at 12:56:31PM +0530, Vinod Koul wrote:
> Hello Bard,
> 
> On 17-04-20, 04:55, Bard Liao wrote:
> > From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > 
> > In the existing SoundWire code, Master Devices are not explicitly
> > represented - only SoundWire Slave Devices are exposed (the use of
> > capital letters follows the SoundWire specification conventions).
> > 
> > The SoundWire Master Device provides the clock, synchronization
> > information and command/control channels. When multiple links are
> > supported, a Controller may expose more than one Master Device; they
> > are typically embedded inside a larger audio cluster (be it in an
> > SOC/chipset or an external audio codec), and we need to describe it
> > using the Linux device and driver model.  This will allow for
> > configuration functions to account for external dependencies such as
> > power rails, clock sources or wake-up mechanisms. This transition will
> > also allow for better sysfs support without the reference count issues
> > mentioned in the initial reviews.
> 
> Well the primary reason for doing sdw_master_device for creating a
> adding sysfs representation.

-ENOPARSE :(

> It *also* helps some vendors due to
> inherent model should not be constructed as the primary approach for the
> sdw_master_device.

No, the PRIMARY reason is "it is the correct thing to do".  It's how to
tie into the driver model correctly, without it, crazy things happen as
we have seen.

> > In this patch, we convert the existing code to use an explicit
> > sdw_slave_type, then define a sdw_master_device structure.
> 
> Please split that up, we should do the conversions required first and
> then do addition of new things.

Can you really do that in two different steps?

> > +struct device_type sdw_master_type = {
> > +	.name =		"soundwire_master",
> > +	.release =	sdw_master_device_release,
> > +};
> > +
> > +/**
> > + * sdw_master_device_add() - create a Linux Master Device representation.
> > + * @parent: the parent Linux device (e.g. a PCI device)
> > + * @fwnode: the parent fwnode (e.g. an ACPI companion device to the parent)
> > + * @link_ops: link-specific ops (optional)
> > + * @link_id: link index as defined by MIPI DisCo specification
> > + * @pdata: private data (e.g. register base, offsets, platform quirks, etc).
> > + *
> > + * The link_ops argument can be NULL, it is only used when link-specific
> > + * initializations and power-management are required.
> > + */
> > +struct sdw_master_device
> > +*sdw_master_device_add(struct device *parent,
> > +		       struct fwnode_handle *fwnode,
> > +		       struct sdw_link_ops *link_ops,
> > +		       int link_id,
> > +		       void *pdata)
> > +{
> > +	struct sdw_master_device *md;
> > +	int ret;
> > +
> > +	md = kzalloc(sizeof(*md), GFP_KERNEL);
> > +	if (!md)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	md->link_id = link_id;
> > +	md->pdata = pdata;
> > +	md->link_ops = link_ops;
> > +
> > +	md->dev.parent = parent;
> > +	md->dev.fwnode = fwnode;
> > +	md->dev.bus = &sdw_bus_type;
> > +	md->dev.type = &sdw_master_type;
> > +	md->dev.dma_mask = md->dev.parent->dma_mask;
> > +	dev_set_name(&md->dev, "sdw-master-%d", md->link_id);
> > +
> > +	if (link_ops && link_ops->driver) {
> > +		/*
> > +		 * A driver is only needed for ASoC integration (need
> > +		 * driver->name) and for link-specific power management
> > +		 * w/ a pm_dev_ops structure.
> 
> That is not true for everyone, it is only true for Intel, pls call that
> out as well...

Why is it not true for everyone?  How else do you get the pm stuff back
to your hardware?

thanks,

greg k-h

Re: [RFC 1/5] soundwire: bus_type: add sdw_master_device support

[Vinod Koul]

Hi Greg,

On 23-04-20, 16:24, Greg KH wrote:
> On Mon, Apr 20, 2020 at 12:56:31PM +0530, Vinod Koul wrote:
> > Hello Bard,
> > 
> > On 17-04-20, 04:55, Bard Liao wrote:
> > > From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > > 
> > > In the existing SoundWire code, Master Devices are not explicitly
> > > represented - only SoundWire Slave Devices are exposed (the use of
> > > capital letters follows the SoundWire specification conventions).
> > > 
> > > The SoundWire Master Device provides the clock, synchronization
> > > information and command/control channels. When multiple links are
> > > supported, a Controller may expose more than one Master Device; they
> > > are typically embedded inside a larger audio cluster (be it in an
> > > SOC/chipset or an external audio codec), and we need to describe it
> > > using the Linux device and driver model.  This will allow for
> > > configuration functions to account for external dependencies such as
> > > power rails, clock sources or wake-up mechanisms. This transition will
> > > also allow for better sysfs support without the reference count issues
> > > mentioned in the initial reviews.
> > 
> > Well the primary reason for doing sdw_master_device for creating a
> > adding sysfs representation.
> 
> -ENOPARSE :(

Oops, sorry!

> > It *also* helps some vendors due to
> > inherent model should not be constructed as the primary approach for the
> > sdw_master_device.
> 
> No, the PRIMARY reason is "it is the correct thing to do".  It's how to
> tie into the driver model correctly, without it, crazy things happen as
> we have seen.

I agree it is *the* right this to do!

> > > In this patch, we convert the existing code to use an explicit
> > > sdw_slave_type, then define a sdw_master_device structure.
> > 
> > Please split that up, we should do the conversions required first and
> > then do addition of new things.
> 
> Can you really do that in two different steps?

Looking at it, the move of existing types first and then adding the new
type

> 
> > > +struct device_type sdw_master_type = {
> > > +	.name =		"soundwire_master",
> > > +	.release =	sdw_master_device_release,
> > > +};
> > > +
> > > +/**
> > > + * sdw_master_device_add() - create a Linux Master Device representation.
> > > + * @parent: the parent Linux device (e.g. a PCI device)
> > > + * @fwnode: the parent fwnode (e.g. an ACPI companion device to the parent)
> > > + * @link_ops: link-specific ops (optional)
> > > + * @link_id: link index as defined by MIPI DisCo specification
> > > + * @pdata: private data (e.g. register base, offsets, platform quirks, etc).
> > > + *
> > > + * The link_ops argument can be NULL, it is only used when link-specific
> > > + * initializations and power-management are required.
> > > + */
> > > +struct sdw_master_device
> > > +*sdw_master_device_add(struct device *parent,
> > > +		       struct fwnode_handle *fwnode,
> > > +		       struct sdw_link_ops *link_ops,
> > > +		       int link_id,
> > > +		       void *pdata)
> > > +{
> > > +	struct sdw_master_device *md;
> > > +	int ret;
> > > +
> > > +	md = kzalloc(sizeof(*md), GFP_KERNEL);
> > > +	if (!md)
> > > +		return ERR_PTR(-ENOMEM);
> > > +
> > > +	md->link_id = link_id;
> > > +	md->pdata = pdata;
> > > +	md->link_ops = link_ops;
> > > +
> > > +	md->dev.parent = parent;
> > > +	md->dev.fwnode = fwnode;
> > > +	md->dev.bus = &sdw_bus_type;
> > > +	md->dev.type = &sdw_master_type;
> > > +	md->dev.dma_mask = md->dev.parent->dma_mask;
> > > +	dev_set_name(&md->dev, "sdw-master-%d", md->link_id);
> > > +
> > > +	if (link_ops && link_ops->driver) {
> > > +		/*
> > > +		 * A driver is only needed for ASoC integration (need
> > > +		 * driver->name) and for link-specific power management
> > > +		 * w/ a pm_dev_ops structure.
> > 
> > That is not true for everyone, it is only true for Intel, pls call that
> > out as well...
> 
> Why is it not true for everyone?  How else do you get the pm stuff back
> to your hardware?

The rest of the world would do using the real controller device. For
example the soundwire controller on Qualcomm devices is enumerated as a
DT device and is using these...

If Intel had a standalone controller or enumerated as individual
functions, it would have been a PCI device and would manage as such

Thanks
-- 
~Vinod

Re: [RFC 1/5] soundwire: bus_type: add sdw_master_device support

[Greg KH]

On Tue, Apr 28, 2020 at 10:01:44AM +0530, Vinod Koul wrote:
> > > That is not true for everyone, it is only true for Intel, pls call that
> > > out as well...
> > 
> > Why is it not true for everyone?  How else do you get the pm stuff back
> > to your hardware?
> 
> The rest of the world would do using the real controller device. For
> example the soundwire controller on Qualcomm devices is enumerated as a
> DT device and is using these...
> 
> If Intel had a standalone controller or enumerated as individual
> functions, it would have been a PCI device and would manage as such

If it is not a standalone controller, what exactly is it?  I thought it
was an acpi device, am I mistaken?

What is the device that the proper soundwire controller driver binds to
on an Intel-based system?

thanks,

greg k-h

Re: [RFC 1/5] soundwire: bus_type: add sdw_master_device support

[Vinod Koul]

On 28-04-20, 08:37, Greg KH wrote:
> On Tue, Apr 28, 2020 at 10:01:44AM +0530, Vinod Koul wrote:
> > > > That is not true for everyone, it is only true for Intel, pls call that
> > > > out as well...
> > > 
> > > Why is it not true for everyone?  How else do you get the pm stuff back
> > > to your hardware?
> > 
> > The rest of the world would do using the real controller device. For
> > example the soundwire controller on Qualcomm devices is enumerated as a
> > DT device and is using these...
> > 
> > If Intel had a standalone controller or enumerated as individual
> > functions, it would have been a PCI device and would manage as such
> 
> If it is not a standalone controller, what exactly is it?  I thought it
> was an acpi device, am I mistaken?
> 
> What is the device that the proper soundwire controller driver binds to
> on an Intel-based system?

The HDA controller which is a PCI device. The device represent HDA
function, DSP and Soundwire controller instances (yes it is typically
more than one instance)

Thanks
-- 
~Vinod

Re: [RFC 1/5] soundwire: bus_type: add sdw_master_device support

[Greg KH]

On Tue, Apr 28, 2020 at 12:19:51PM +0530, Vinod Koul wrote:
> On 28-04-20, 08:37, Greg KH wrote:
> > On Tue, Apr 28, 2020 at 10:01:44AM +0530, Vinod Koul wrote:
> > > > > That is not true for everyone, it is only true for Intel, pls call that
> > > > > out as well...
> > > > 
> > > > Why is it not true for everyone?  How else do you get the pm stuff back
> > > > to your hardware?
> > > 
> > > The rest of the world would do using the real controller device. For
> > > example the soundwire controller on Qualcomm devices is enumerated as a
> > > DT device and is using these...
> > > 
> > > If Intel had a standalone controller or enumerated as individual
> > > functions, it would have been a PCI device and would manage as such
> > 
> > If it is not a standalone controller, what exactly is it?  I thought it
> > was an acpi device, am I mistaken?
> > 
> > What is the device that the proper soundwire controller driver binds to
> > on an Intel-based system?
> 
> The HDA controller which is a PCI device. The device represent HDA
> function, DSP and Soundwire controller instances (yes it is typically
> more than one instance)

Then those "instances" should be split up into individual devices that a
driver can bind to.  See the work happening on the "virtual" bus for
examples of how that can be done.

A platform device better not be being used here, I'm afraid to look at
the code now...

greg k-h

Re: [RFC 1/5] soundwire: bus_type: add sdw_master_device support

[Vinod Koul]

On 28-04-20, 08:55, Greg KH wrote:
> On Tue, Apr 28, 2020 at 12:19:51PM +0530, Vinod Koul wrote:
> > On 28-04-20, 08:37, Greg KH wrote:
> > > On Tue, Apr 28, 2020 at 10:01:44AM +0530, Vinod Koul wrote:
> > > > > > That is not true for everyone, it is only true for Intel, pls call that
> > > > > > out as well...
> > > > > 
> > > > > Why is it not true for everyone?  How else do you get the pm stuff back
> > > > > to your hardware?
> > > > 
> > > > The rest of the world would do using the real controller device. For
> > > > example the soundwire controller on Qualcomm devices is enumerated as a
> > > > DT device and is using these...
> > > > 
> > > > If Intel had a standalone controller or enumerated as individual
> > > > functions, it would have been a PCI device and would manage as such
> > > 
> > > If it is not a standalone controller, what exactly is it?  I thought it
> > > was an acpi device, am I mistaken?
> > > 
> > > What is the device that the proper soundwire controller driver binds to
> > > on an Intel-based system?
> > 
> > The HDA controller which is a PCI device. The device represent HDA
> > function, DSP and Soundwire controller instances (yes it is typically
> > more than one instance)
> 
> Then those "instances" should be split up into individual devices that a
> driver can bind to.  See the work happening on the "virtual" bus for
> examples of how that can be done.

Yes removing platform devices is the goal for Intel now :) Pierre & Bard
have been diligently trying to solve this.

Only difference is the means to end goal. I am not convinced that this
should be in soundwire subsystem.

Looks like folks are trying to review and port to use this bus. Makes
sense to me..
https://lore.kernel.org/netdev/c5197d2f-3840-d304-6b09-d334cae81294@linux.intel.com/

> A platform device better not be being used here, I'm afraid to look at
> the code now...

Well if the plan for 'virtual-bus' goes well, it should be  a simple
replacement of platform->virtual for Intel driver. Rest of the driver
should not be impacted :)

Thanks
-- 
~Vinod

Re: [RFC 1/5] soundwire: bus_type: add sdw_master_device support

[Vinod Koul]

On 30-04-20, 11:24, Bard liao wrote:
> 
> On 4/28/2020 3:51 PM, Vinod Koul wrote:
> > On 28-04-20, 08:55, Greg KH wrote:
> > > On Tue, Apr 28, 2020 at 12:19:51PM +0530, Vinod Koul wrote:
> > > > On 28-04-20, 08:37, Greg KH wrote:
> > > > > On Tue, Apr 28, 2020 at 10:01:44AM +0530, Vinod Koul wrote:
> > > > > > > > That is not true for everyone, it is only true for Intel, pls call that
> > > > > > > > out as well...
> > > > > > > Why is it not true for everyone?  How else do you get the pm stuff back
> > > > > > > to your hardware?
> > > > > > The rest of the world would do using the real controller device. For
> > > > > > example the soundwire controller on Qualcomm devices is enumerated as a
> > > > > > DT device and is using these...
> > > > > > 
> > > > > > If Intel had a standalone controller or enumerated as individual
> > > > > > functions, it would have been a PCI device and would manage as such
> > > > > If it is not a standalone controller, what exactly is it?  I thought it
> > > > > was an acpi device, am I mistaken?
> > > > > 
> > > > > What is the device that the proper soundwire controller driver binds to
> > > > > on an Intel-based system?
> > > > The HDA controller which is a PCI device. The device represent HDA
> > > > function, DSP and Soundwire controller instances (yes it is typically
> > > > more than one instance)
> > > Then those "instances" should be split up into individual devices that a
> > > driver can bind to.  See the work happening on the "virtual" bus for
> > > examples of how that can be done.
> > Yes removing platform devices is the goal for Intel now :) Pierre & Bard
> > have been diligently trying to solve this.
> > 
> > Only difference is the means to end goal. I am not convinced that this
> > should be in soundwire subsystem.
> > 
> > Looks like folks are trying to review and port to use this bus. Makes
> > sense to me..
> > https://lore.kernel.org/netdev/c5197d2f-3840-d304-6b09-d334cae81294@linux.intel.com/
> > 
> > > A platform device better not be being used here, I'm afraid to look at
> > > the code now...
> > Well if the plan for 'virtual-bus' goes well, it should be  a simple
> > replacement of platform->virtual for Intel driver. Rest of the driver
> > should not be impacted :)
> 
> We can't expect when will 'virtual-bus' be upstream and it's not feasible
> to wait forever. Can we move forward with current solution and switch to
> 'virtual-bus' whenever it is upstream?

the move from platform-device to virtual-device should happen once
the virtual-bus' is accepted upstream. till then imo you should continue
with current platform device and once you have virtual-bus upstream,
replace it with virtual-device. Note: I am going to hold you on that :)

Rest of the pieces like sdw_master_device and sysfs parts are not
dependent upon this and should be sent for review and we can merge when
ready, hopefully for 5.8.

Thanks
-- 
~Vinod

[RFC 2/5] soundwire: master: use device node pointer from master device

[Bard Liao]

From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

device_node pointer is required for scanning slave devices, update
it from the master device.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
---
 drivers/soundwire/master.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/soundwire/master.c b/drivers/soundwire/master.c
index 094518817601..c930c871caae 100644
--- a/drivers/soundwire/master.c
+++ b/drivers/soundwire/master.c
@@ -49,6 +49,7 @@ struct sdw_master_device
 	md->link_ops = link_ops;
 
 	md->dev.parent = parent;
+	md->dev.of_node = parent->of_node;
 	md->dev.fwnode = fwnode;
 	md->dev.bus = &sdw_bus_type;
 	md->dev.type = &sdw_master_type;
-- 
2.17.1

[RFC 3/5] soundwire: qcom: fix error handling in probe

[Bard Liao]

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

Make sure all error cases are properly handled and all resources freed.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
---
 drivers/soundwire/qcom.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
index d6c9ad231873..e08a17c13f92 100644
--- a/drivers/soundwire/qcom.c
+++ b/drivers/soundwire/qcom.c
@@ -765,12 +765,16 @@ static int qcom_swrm_probe(struct platform_device *pdev)
 	}
 
 	ctrl->irq = of_irq_get(dev->of_node, 0);
-	if (ctrl->irq < 0)
-		return ctrl->irq;
+	if (ctrl->irq < 0) {
+		ret = ctrl->irq;
+		goto err_init;
+	}
 
 	ctrl->hclk = devm_clk_get(dev, "iface");
-	if (IS_ERR(ctrl->hclk))
-		return PTR_ERR(ctrl->hclk);
+	if (IS_ERR(ctrl->hclk)) {
+		ret = PTR_ERR(ctrl->hclk);
+		goto err_init;
+	}
 
 	clk_prepare_enable(ctrl->hclk);
 
@@ -787,7 +791,7 @@ static int qcom_swrm_probe(struct platform_device *pdev)
 
 	ret = qcom_swrm_get_port_config(ctrl);
 	if (ret)
-		return ret;
+		goto err_clk;
 
 	params = &ctrl->bus.params;
 	params->max_dr_freq = DEFAULT_CLK_FREQ;
@@ -814,28 +818,32 @@ static int qcom_swrm_probe(struct platform_device *pdev)
 					"soundwire", ctrl);
 	if (ret) {
 		dev_err(dev, "Failed to request soundwire irq\n");
-		goto err;
+		goto err_clk;
 	}
 
 	ret = sdw_add_bus_master(&ctrl->bus);
 	if (ret) {
 		dev_err(dev, "Failed to register Soundwire controller (%d)\n",
 			ret);
-		goto err;
+		goto err_clk;
 	}
 
 	qcom_swrm_init(ctrl);
 	ret = qcom_swrm_register_dais(ctrl);
 	if (ret)
-		goto err;
+		goto err_master_add;
 
 	dev_info(dev, "Qualcomm Soundwire controller v%x.%x.%x Registered\n",
 		 (ctrl->version >> 24) & 0xff, (ctrl->version >> 16) & 0xff,
 		 ctrl->version & 0xffff);
 
 	return 0;
-err:
+
+err_master_add:
+	sdw_delete_bus_master(&ctrl->bus);
+err_clk:
 	clk_disable_unprepare(ctrl->hclk);
+err_init:
 	return ret;
 }
 
-- 
2.17.1

Re: [RFC 3/5] soundwire: qcom: fix error handling in probe

[Srinivas Kandagatla]

On 16/04/2020 21:55, Bard Liao wrote:
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> 
> Make sure all error cases are properly handled and all resources freed.
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
> ---
Thanks for the patch,

Reviewed-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

>   drivers/soundwire/qcom.c | 26 +++++++++++++++++---------
>   1 file changed, 17 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
> index d6c9ad231873..e08a17c13f92 100644
> --- a/drivers/soundwire/qcom.c
> +++ b/drivers/soundwire/qcom.c
> @@ -765,12 +765,16 @@ static int qcom_swrm_probe(struct platform_device *pdev)
>   	}
>   
>   	ctrl->irq = of_irq_get(dev->of_node, 0);
> -	if (ctrl->irq < 0)
> -		return ctrl->irq;
> +	if (ctrl->irq < 0) {
> +		ret = ctrl->irq;
> +		goto err_init;
> +	}
>   
>   	ctrl->hclk = devm_clk_get(dev, "iface");
> -	if (IS_ERR(ctrl->hclk))
> -		return PTR_ERR(ctrl->hclk);
> +	if (IS_ERR(ctrl->hclk)) {
> +		ret = PTR_ERR(ctrl->hclk);
> +		goto err_init;
> +	}
>   
>   	clk_prepare_enable(ctrl->hclk);
>   
> @@ -787,7 +791,7 @@ static int qcom_swrm_probe(struct platform_device *pdev)
>   
>   	ret = qcom_swrm_get_port_config(ctrl);
>   	if (ret)
> -		return ret;
> +		goto err_clk;
>   
>   	params = &ctrl->bus.params;
>   	params->max_dr_freq = DEFAULT_CLK_FREQ;
> @@ -814,28 +818,32 @@ static int qcom_swrm_probe(struct platform_device *pdev)
>   					"soundwire", ctrl);
>   	if (ret) {
>   		dev_err(dev, "Failed to request soundwire irq\n");
> -		goto err;
> +		goto err_clk;
>   	}
>   
>   	ret = sdw_add_bus_master(&ctrl->bus);
>   	if (ret) {
>   		dev_err(dev, "Failed to register Soundwire controller (%d)\n",
>   			ret);
> -		goto err;
> +		goto err_clk;
>   	}
>   
>   	qcom_swrm_init(ctrl);
>   	ret = qcom_swrm_register_dais(ctrl);
>   	if (ret)
> -		goto err;
> +		goto err_master_add;
>   
>   	dev_info(dev, "Qualcomm Soundwire controller v%x.%x.%x Registered\n",
>   		 (ctrl->version >> 24) & 0xff, (ctrl->version >> 16) & 0xff,
>   		 ctrl->version & 0xffff);
>   
>   	return 0;
> -err:
> +
> +err_master_add:
> +	sdw_delete_bus_master(&ctrl->bus);
> +err_clk:
>   	clk_disable_unprepare(ctrl->hclk);
> +err_init:
>   	return ret;
>   }
>   
> 

[RFC 4/5] soundwire: qcom: add sdw_master_device support

[Bard Liao]

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

Add new device as a child of the platform device, following the
following hierarchy:

platform_device
    sdw_master_device
        sdw_slave0
	...
	sdw_slaveN

For the Qualcomm implementation no sdw_link_ops are provided. All the
dais have to be registered using the platform_device and all power
management is handled at the platform device level.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
---
 drivers/soundwire/qcom.c | 39 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 34 insertions(+), 5 deletions(-)

diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
index e08a17c13f92..96306044aa84 100644
--- a/drivers/soundwire/qcom.c
+++ b/drivers/soundwire/qcom.c
@@ -89,6 +89,7 @@ struct qcom_swrm_port_config {
 struct qcom_swrm_ctrl {
 	struct sdw_bus bus;
 	struct device *dev;
+	struct sdw_master_device *md;
 	struct regmap *regmap;
 	struct completion *comp;
 	struct work_struct slave_work;
@@ -746,7 +747,7 @@ static int qcom_swrm_probe(struct platform_device *pdev)
 	struct sdw_master_prop *prop;
 	struct sdw_bus_params *params;
 	struct qcom_swrm_ctrl *ctrl;
-	int ret;
+	int ret, err;
 	u32 val;
 
 	ctrl = devm_kzalloc(dev, sizeof(*ctrl), GFP_KERNEL);
@@ -784,14 +785,34 @@ static int qcom_swrm_probe(struct platform_device *pdev)
 	mutex_init(&ctrl->port_lock);
 	INIT_WORK(&ctrl->slave_work, qcom_swrm_slave_wq);
 
-	ctrl->bus.dev = dev;
+	/*
+	 * add sdw_master_device.
+	 * For the Qualcomm implementation there is no driver.
+	 */
+	ctrl->md = sdw_master_device_add(dev,	/* platform device is parent */
+					 dev->fwnode,
+					 NULL,	/* no link ops */
+					 0,	/* only one link supported */
+					 NULL);	/* no context */
+	if (IS_ERR(ctrl->md)) {
+		dev_err(dev, "Could not create sdw_master_device\n");
+		ret = PTR_ERR(ctrl->md);
+		goto err_clk;
+	}
+
+	/* add bus handle */
+	ctrl->md->bus = &ctrl->bus;
+
+	/* the bus uses the sdw_master_device, not the platform device */
+	ctrl->bus.dev = &ctrl->md->dev;
+
 	ctrl->bus.ops = &qcom_swrm_ops;
 	ctrl->bus.port_ops = &qcom_swrm_port_ops;
 	ctrl->bus.compute_params = &qcom_swrm_compute_params;
 
 	ret = qcom_swrm_get_port_config(ctrl);
 	if (ret)
-		goto err_clk;
+		goto err_md;
 
 	params = &ctrl->bus.params;
 	params->max_dr_freq = DEFAULT_CLK_FREQ;
@@ -818,14 +839,14 @@ static int qcom_swrm_probe(struct platform_device *pdev)
 					"soundwire", ctrl);
 	if (ret) {
 		dev_err(dev, "Failed to request soundwire irq\n");
-		goto err_clk;
+		goto err_md;
 	}
 
 	ret = sdw_add_bus_master(&ctrl->bus);
 	if (ret) {
 		dev_err(dev, "Failed to register Soundwire controller (%d)\n",
 			ret);
-		goto err_clk;
+		goto err_md;
 	}
 
 	qcom_swrm_init(ctrl);
@@ -841,6 +862,10 @@ static int qcom_swrm_probe(struct platform_device *pdev)
 
 err_master_add:
 	sdw_delete_bus_master(&ctrl->bus);
+err_md:
+	err = sdw_master_device_del(ctrl->md);
+	if (err < 0) /* log but return initial status */
+		dev_err(dev, "master device del failed %d\n", err);
 err_clk:
 	clk_disable_unprepare(ctrl->hclk);
 err_init:
@@ -850,8 +875,12 @@ static int qcom_swrm_probe(struct platform_device *pdev)
 static int qcom_swrm_remove(struct platform_device *pdev)
 {
 	struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(&pdev->dev);
+	int err;
 
 	sdw_delete_bus_master(&ctrl->bus);
+	err = sdw_master_device_del(ctrl->md);
+	if (err < 0)
+		dev_err(&pdev->dev, "master device del failed %d\n", err);
 	clk_disable_unprepare(ctrl->hclk);
 
 	return 0;
-- 
2.17.1

[RFC 5/5] soundwire: intel: transition to sdw_master_device

[Bard Liao]

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

Use sdw_master_device instead of platform devices

In addition, rather than a plain-vanilla init/exit, this patch
provides 3 steps in the initialization (ACPI scan, probe, startup)
which makes it easier to detect platform support for SoundWire,
allocate required resources as early as possible, and conversely help
make the startup() callback lighter-weight with only hardware register
setup.

Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/intel.c      |  98 +++++++----
 drivers/soundwire/intel.h      |   8 +-
 drivers/soundwire/intel_init.c | 293 +++++++++++++++++++++++++--------
 3 files changed, 294 insertions(+), 105 deletions(-)

diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index 3c83e76c6bf9..ea4131f54d30 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -11,12 +11,12 @@
 #include <linux/module.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
-#include <linux/platform_device.h>
 #include <sound/pcm_params.h>
 #include <sound/soc.h>
 #include <linux/soundwire/sdw_registers.h>
 #include <linux/soundwire/sdw.h>
 #include <linux/soundwire/sdw_intel.h>
+#include <linux/soundwire/sdw_type.h>
 #include "cadence_master.h"
 #include "bus.h"
 #include "intel.h"
@@ -92,8 +92,6 @@
 #define SDW_ALH_STRMZCFG_DMAT		GENMASK(7, 0)
 #define SDW_ALH_STRMZCFG_CHN		GENMASK(19, 16)
 
-#define SDW_INTEL_QUIRK_MASK_BUS_DISABLE	BIT(1)
-
 enum intel_pdi_type {
 	INTEL_PDI_IN = 0,
 	INTEL_PDI_OUT = 1,
@@ -1083,24 +1081,23 @@ static int intel_init(struct sdw_intel *sdw)
 /*
  * probe and init
  */
-static int intel_probe(struct platform_device *pdev)
+static int intel_master_probe(struct sdw_master_device *md, void *link_ctx)
 {
-	struct sdw_cdns_stream_config config;
 	struct sdw_intel *sdw;
 	int ret;
 
-	sdw = devm_kzalloc(&pdev->dev, sizeof(*sdw), GFP_KERNEL);
+	sdw = devm_kzalloc(&md->dev, sizeof(*sdw), GFP_KERNEL);
 	if (!sdw)
 		return -ENOMEM;
 
-	sdw->instance = pdev->id;
-	sdw->link_res = dev_get_platdata(&pdev->dev);
-	sdw->cdns.dev = &pdev->dev;
+	sdw->instance = md->link_id;
+	sdw->link_res = link_ctx;
+	sdw->cdns.dev = &md->dev;
 	sdw->cdns.registers = sdw->link_res->registers;
-	sdw->cdns.instance = sdw->instance;
+	sdw->cdns.instance = md->link_id;
 	sdw->cdns.msg_count = 0;
-	sdw->cdns.bus.dev = &pdev->dev;
-	sdw->cdns.bus.link_id = pdev->id;
+	sdw->cdns.bus.dev = &md->dev;
+	sdw->cdns.bus.link_id = md->link_id;
 
 	sdw_cdns_probe(&sdw->cdns);
 
@@ -1108,16 +1105,51 @@ static int intel_probe(struct platform_device *pdev)
 	sdw_intel_ops.read_prop = intel_prop_read;
 	sdw->cdns.bus.ops = &sdw_intel_ops;
 
-	platform_set_drvdata(pdev, sdw);
+	md->bus = &sdw->cdns.bus;
+	md->pdata = sdw;
+
+	/* set driver data, accessed by snd_soc_dai_set_drvdata() */
+	dev_set_drvdata(&md->dev, &sdw->cdns);
 
 	ret = sdw_add_bus_master(&sdw->cdns.bus);
 	if (ret) {
-		dev_err(&pdev->dev, "sdw_add_bus_master fail: %d\n", ret);
+		dev_err(&md->dev, "sdw_add_bus_master fail: %d\n", ret);
 		return ret;
 	}
 
+	if (sdw->cdns.bus.prop.hw_disabled)
+		dev_info(&md->dev,
+			 "SoundWire master %d is disabled, will be ignored\n",
+			 sdw->cdns.bus.link_id);
+
+	/* Acquire IRQ */
+	ret = request_threaded_irq(sdw->link_res->irq,
+				   sdw_cdns_irq, sdw_cdns_thread,
+				   IRQF_SHARED, KBUILD_MODNAME, &sdw->cdns);
+	if (ret < 0) {
+		dev_err(sdw->cdns.dev, "unable to grab IRQ %d, disabling device\n",
+			sdw->link_res->irq);
+		goto err_init;
+	}
+
+	return 0;
+
+err_init:
+	sdw_delete_bus_master(&sdw->cdns.bus);
+	return ret;
+}
+
+static int intel_master_startup(struct sdw_master_device *md)
+{
+	struct sdw_cdns_stream_config config;
+	struct sdw_intel *sdw;
+	int ret;
+
+	sdw = md->pdata;
+
 	if (sdw->cdns.bus.prop.hw_disabled) {
-		dev_info(&pdev->dev, "SoundWire master %d is disabled, ignoring\n",
+		dev_info(&md->dev,
+			 "SoundWire master %d is disabled, ignoring\n",
 			 sdw->cdns.bus.link_id);
 		return 0;
 	}
@@ -1135,16 +1167,6 @@ static int intel_probe(struct platform_device *pdev)
 
 	intel_pdi_ch_update(sdw);
 
-	/* Acquire IRQ */
-	ret = request_threaded_irq(sdw->link_res->irq,
-				   sdw_cdns_irq, sdw_cdns_thread,
-				   IRQF_SHARED, KBUILD_MODNAME, &sdw->cdns);
-	if (ret < 0) {
-		dev_err(sdw->cdns.dev, "unable to grab IRQ %d, disabling device\n",
-			sdw->link_res->irq);
-		goto err_init;
-	}
-
 	ret = sdw_cdns_enable_interrupt(&sdw->cdns, true);
 	if (ret < 0) {
 		dev_err(sdw->cdns.dev, "cannot enable interrupts\n");
@@ -1171,17 +1193,17 @@ static int intel_probe(struct platform_device *pdev)
 
 err_interrupt:
 	sdw_cdns_enable_interrupt(&sdw->cdns, false);
-	free_irq(sdw->link_res->irq, sdw);
 err_init:
+	free_irq(sdw->link_res->irq, sdw);
 	sdw_delete_bus_master(&sdw->cdns.bus);
 	return ret;
 }
 
-static int intel_remove(struct platform_device *pdev)
+static int intel_master_remove(struct sdw_master_device *md)
 {
 	struct sdw_intel *sdw;
 
-	sdw = platform_get_drvdata(pdev);
+	sdw = md->pdata;
 
 	if (!sdw->cdns.bus.prop.hw_disabled) {
 		intel_debugfs_exit(sdw);
@@ -1194,17 +1216,21 @@ static int intel_remove(struct platform_device *pdev)
 	return 0;
 }
 
-static struct platform_driver sdw_intel_drv = {
-	.probe = intel_probe,
-	.remove = intel_remove,
-	.driver = {
-		.name = "int-sdw",
 
-	},
+static struct device_driver sdw_intel_driver = {
+	.name = "intel-master",
+	.bus = &sdw_bus_type,
 };
 
-module_platform_driver(sdw_intel_drv);
+struct sdw_link_ops sdw_intel_link_ops = {
+	.driver = &sdw_intel_driver,
+	.add = intel_master_probe,
+	.startup = intel_master_startup,
+	.del = intel_master_remove,
+	.process_wake_event = intel_master_process_wakeen_event,
+};
+EXPORT_SYMBOL(sdw_intel_link_ops);
 
 MODULE_LICENSE("Dual BSD/GPL");
-MODULE_ALIAS("platform:int-sdw");
+MODULE_ALIAS("sdw:intel-master");
 MODULE_DESCRIPTION("Intel Soundwire Master Driver");
diff --git a/drivers/soundwire/intel.h b/drivers/soundwire/intel.h
index 38b7c125fb10..6a1c37ab3fd1 100644
--- a/drivers/soundwire/intel.h
+++ b/drivers/soundwire/intel.h
@@ -7,7 +7,7 @@
 /**
  * struct sdw_intel_link_res - Soundwire Intel link resource structure,
  * typically populated by the controller driver.
- * @pdev: platform_device
+ * @md: master device
  * @mmio_base: mmio base of SoundWire registers
  * @registers: Link IO registers base
  * @shim: Audio shim pointer
@@ -17,7 +17,7 @@
  * @dev: device implementing hw_params and free callbacks
  */
 struct sdw_intel_link_res {
-	struct platform_device *pdev;
+	struct sdw_master_device *md;
 	void __iomem *mmio_base; /* not strictly needed, useful for debug */
 	void __iomem *registers;
 	void __iomem *shim;
@@ -27,4 +27,8 @@ struct sdw_intel_link_res {
 	struct device *dev;
 };
 
+#define SDW_INTEL_QUIRK_MASK_BUS_DISABLE      BIT(1)
+
+extern struct sdw_link_ops sdw_intel_link_ops;
+
 #endif /* __SDW_INTEL_LOCAL_H */
diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c
index 4b769409f6f8..851a14a5f310 100644
--- a/drivers/soundwire/intel_init.c
+++ b/drivers/soundwire/intel_init.c
@@ -11,7 +11,7 @@
 #include <linux/export.h>
 #include <linux/io.h>
 #include <linux/module.h>
-#include <linux/platform_device.h>
+#include <linux/soundwire/sdw.h>
 #include <linux/soundwire/sdw_intel.h>
 #include "intel.h"
 
@@ -23,22 +23,56 @@
 #define SDW_LINK_BASE		0x30000
 #define SDW_LINK_SIZE		0x10000
 
-static int link_mask;
-module_param_named(sdw_link_mask, link_mask, int, 0444);
+static int ctrl_link_mask;
+module_param_named(sdw_link_mask, ctrl_link_mask, int, 0444);
 MODULE_PARM_DESC(sdw_link_mask, "Intel link mask (one bit per link)");
 
-static int sdw_intel_cleanup_pdev(struct sdw_intel_ctx *ctx)
+static bool is_link_enabled(struct fwnode_handle *fw_node, int i)
+{
+	struct fwnode_handle *link;
+	char name[32];
+	u32 quirk_mask = 0;
+
+	/* Find master handle */
+	snprintf(name, sizeof(name),
+		 "mipi-sdw-link-%d-subproperties", i);
+
+	link = fwnode_get_named_child_node(fw_node, name);
+	if (!link)
+		return false;
+
+	fwnode_property_read_u32(link,
+				 "intel-quirk-mask",
+				 &quirk_mask);
+
+	if (quirk_mask & SDW_INTEL_QUIRK_MASK_BUS_DISABLE)
+		return false;
+
+	return true;
+}
+
+static int sdw_intel_cleanup(struct sdw_intel_ctx *ctx)
 {
 	struct sdw_intel_link_res *link = ctx->links;
-	int i;
+	u32 link_mask;
+	int i, ret;
 
 	if (!link)
 		return 0;
 
-	for (i = 0; i < ctx->count; i++) {
-		if (link->pdev)
-			platform_device_unregister(link->pdev);
-		link++;
+	link_mask = ctx->link_mask;
+
+	for (i = 0; i < ctx->count; i++, link++) {
+		if (link_mask && !(link_mask & BIT(i)))
+			continue;
+
+		if (!IS_ERR_OR_NULL(link->md)) {
+			ret = sdw_master_device_del(link->md);
+			if (ret < 0)
+				dev_err(&link->md->dev,
+					"master device del failed %d\n",
+					ret);
+		}
 	}
 
 	kfree(ctx->links);
@@ -47,112 +81,207 @@ static int sdw_intel_cleanup_pdev(struct sdw_intel_ctx *ctx)
 	return 0;
 }
 
-static struct sdw_intel_ctx
-*sdw_intel_add_controller(struct sdw_intel_res *res)
+static int
+sdw_intel_scan_controller(struct sdw_intel_acpi_info *info)
 {
-	struct platform_device_info pdevinfo;
-	struct platform_device *pdev;
-	struct sdw_intel_link_res *link;
-	struct sdw_intel_ctx *ctx;
 	struct acpi_device *adev;
 	int ret, i;
 	u8 count;
-	u32 caps;
 
-	if (acpi_bus_get_device(res->handle, &adev))
-		return NULL;
+	if (acpi_bus_get_device(info->handle, &adev))
+		return -EINVAL;
 
 	/* Found controller, find links supported */
 	count = 0;
 	ret = fwnode_property_read_u8_array(acpi_fwnode_handle(adev),
 					    "mipi-sdw-master-count", &count, 1);
 
-	/* Don't fail on error, continue and use hw value */
+	/*
+	 * In theory we could check the number of links supported in
+	 * hardware, but in that step we cannot assume SoundWire IP is
+	 * powered.
+	 *
+	 * In addition, if the BIOS doesn't even provide this
+	 * 'master-count' property then all the inits based on link
+	 * masks will fail as well.
+	 *
+	 * We will check the hardware capabilities in the startup() step
+	 */
+
 	if (ret) {
 		dev_err(&adev->dev,
 			"Failed to read mipi-sdw-master-count: %d\n", ret);
-		count = SDW_MAX_LINKS;
+		return -EINVAL;
 	}
 
-	/* Check SNDWLCAP.LCOUNT */
-	caps = ioread32(res->mmio_base + SDW_SHIM_BASE + SDW_SHIM_LCAP);
-	caps &= GENMASK(2, 0);
-
-	/* Check HW supported vs property value and use min of two */
-	count = min_t(u8, caps, count);
-
 	/* Check count is within bounds */
 	if (count > SDW_MAX_LINKS) {
 		dev_err(&adev->dev, "Link count %d exceeds max %d\n",
 			count, SDW_MAX_LINKS);
-		return NULL;
+		return -EINVAL;
 	} else if (!count) {
 		dev_warn(&adev->dev, "No SoundWire links detected\n");
-		return NULL;
+		return -EINVAL;
+	}
+	dev_dbg(&adev->dev, "ACPI reports %d SDW Link devices\n", count);
+
+	info->count = count;
+
+	for (i = 0; i < count; i++) {
+		if (ctrl_link_mask && !(ctrl_link_mask & BIT(i))) {
+			dev_dbg(&adev->dev,
+				"Link %d masked, will not be enabled\n", i);
+			continue;
+		}
+
+		if (!is_link_enabled(acpi_fwnode_handle(adev), i)) {
+			dev_dbg(&adev->dev,
+				"Link %d not selected in firmware\n", i);
+			continue;
+		}
+
+		info->link_mask |= BIT(i);
 	}
 
+	return 0;
+}
+
+static struct sdw_intel_ctx
+*sdw_intel_probe_controller(struct sdw_intel_res *res)
+{
+	struct sdw_intel_link_res *link;
+	struct sdw_intel_ctx *ctx;
+	struct acpi_device *adev;
+	struct sdw_master_device *md;
+	u32 link_mask;
+	int count;
+	int ret;
+	int i;
+
+	if (!res)
+		return NULL;
+
+	if (acpi_bus_get_device(res->handle, &adev))
+		return NULL;
+
+	if (!res->count)
+		return NULL;
+
+	count = res->count;
 	dev_dbg(&adev->dev, "Creating %d SDW Link devices\n", count);
 
 	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
 	if (!ctx)
 		return NULL;
 
-	ctx->count = count;
-	ctx->links = kcalloc(ctx->count, sizeof(*ctx->links), GFP_KERNEL);
+	ctx->links = kcalloc(count, sizeof(*ctx->links), GFP_KERNEL);
 	if (!ctx->links)
 		goto link_err;
 
+	ctx->count = count;
+	ctx->mmio_base = res->mmio_base;
+	ctx->link_mask = res->link_mask;
+	ctx->handle = res->handle;
+
 	link = ctx->links;
+	link_mask = ctx->link_mask;
+
+	ret = driver_register(sdw_intel_link_ops.driver);
+	if (ret) {
+		dev_err(&adev->dev,
+			"failed to register sdw master driver: %d\n",
+			ret);
+		goto register_err;
+	}
 
 	/* Create SDW Master devices */
-	for (i = 0; i < count; i++) {
-		if (link_mask && !(link_mask & BIT(i))) {
-			dev_dbg(&adev->dev,
-				"Link %d masked, will not be enabled\n", i);
-			link++;
+	for (i = 0; i < count; i++, link++) {
+		if (link_mask && !(link_mask & BIT(i)))
 			continue;
-		}
 
+		link->mmio_base = res->mmio_base;
 		link->registers = res->mmio_base + SDW_LINK_BASE
-					+ (SDW_LINK_SIZE * i);
+			+ (SDW_LINK_SIZE * i);
 		link->shim = res->mmio_base + SDW_SHIM_BASE;
 		link->alh = res->mmio_base + SDW_ALH_BASE;
-
+		link->irq = res->irq;
 		link->ops = res->ops;
 		link->dev = res->dev;
+		link->clock_stop_quirks = res->clock_stop_quirks;
 
-		memset(&pdevinfo, 0, sizeof(pdevinfo));
-
-		pdevinfo.parent = res->parent;
-		pdevinfo.name = "int-sdw";
-		pdevinfo.id = i;
-		pdevinfo.fwnode = acpi_fwnode_handle(adev);
+		md = sdw_master_device_add(res->parent,
+					   acpi_fwnode_handle(adev),
+					   &sdw_intel_link_ops,
+					   i,
+					   link);
 
-		pdev = platform_device_register_full(&pdevinfo);
-		if (IS_ERR(pdev)) {
-			dev_err(&adev->dev,
-				"platform device creation failed: %ld\n",
-				PTR_ERR(pdev));
-			goto pdev_err;
+		if (IS_ERR(md)) {
+			dev_err(&adev->dev, "Could not create link %d\n", i);
+			goto err;
 		}
 
-		link->pdev = pdev;
-		link++;
+		link->md = md;
 	}
 
 	return ctx;
 
-pdev_err:
-	sdw_intel_cleanup_pdev(ctx);
+err:
+	ctx->count = i;
+	driver_unregister(sdw_intel_link_ops.driver);
+register_err:
+	sdw_intel_cleanup(ctx);
 link_err:
 	kfree(ctx);
 	return NULL;
 }
 
+static int
+sdw_intel_startup_controller(struct sdw_intel_ctx *ctx)
+{
+	struct acpi_device *adev;
+	struct sdw_intel_link_res *link;
+	struct sdw_master_device *md;
+	u32 caps;
+	u32 link_mask;
+	int i;
+
+	if (acpi_bus_get_device(ctx->handle, &adev))
+		return -EINVAL;
+
+	/* Check SNDWLCAP.LCOUNT */
+	caps = ioread32(ctx->mmio_base + SDW_SHIM_BASE + SDW_SHIM_LCAP);
+	caps &= GENMASK(2, 0);
+
+	/* Check HW supported vs property value */
+	if (caps < ctx->count) {
+		dev_err(&adev->dev,
+			"BIOS master count is larger than hardware capabilities\n");
+		return -EINVAL;
+	}
+
+	if (!ctx->links)
+		return -EINVAL;
+
+	link = ctx->links;
+	link_mask = ctx->link_mask;
+
+	/* Startup SDW Master devices */
+	for (i = 0; i < ctx->count; i++, link++) {
+		if (link_mask && !(link_mask & BIT(i)))
+			continue;
+
+		md = link->md;
+
+		sdw_master_device_startup(md);
+	}
+
+	return 0;
+}
+
 static acpi_status sdw_intel_acpi_cb(acpi_handle handle, u32 level,
 				     void *cdata, void **return_value)
 {
-	struct sdw_intel_res *res = cdata;
+	struct sdw_intel_acpi_info *info = cdata;
 	struct acpi_device *adev;
 	acpi_status status;
 	u64 adr;
@@ -166,7 +295,7 @@ static acpi_status sdw_intel_acpi_cb(acpi_handle handle, u32 level,
 		return AE_NOT_FOUND;
 	}
 
-	res->handle = handle;
+	info->handle = handle;
 
 	/*
 	 * On some Intel platforms, multiple children of the HDAS
@@ -183,36 +312,66 @@ static acpi_status sdw_intel_acpi_cb(acpi_handle handle, u32 level,
 }
 
 /**
- * sdw_intel_init() - SoundWire Intel init routine
+ * sdw_intel_acpi_scan() - SoundWire Intel init routine
  * @parent_handle: ACPI parent handle
- * @res: resource data
+ * @info: description of what firmware/DSDT tables expose
  *
- * This scans the namespace and creates SoundWire link controller devices
- * based on the info queried.
+ * This scans the namespace and queries firmware to figure out which
+ * links to enable. A follow-up use of sdw_intel_probe() and
+ * sdw_intel_startup() is required for creation of devices and bus
+ * startup
  */
-void *sdw_intel_init(acpi_handle *parent_handle, struct sdw_intel_res *res)
+int sdw_intel_acpi_scan(acpi_handle *parent_handle,
+			struct sdw_intel_acpi_info *info)
 {
 	acpi_status status;
 
 	status = acpi_walk_namespace(ACPI_TYPE_DEVICE,
 				     parent_handle, 1,
 				     sdw_intel_acpi_cb,
-				     NULL, res, NULL);
+				     NULL, info, NULL);
 	if (ACPI_FAILURE(status))
-		return NULL;
+		return -ENODEV;
 
-	return sdw_intel_add_controller(res);
+	return sdw_intel_scan_controller(info);
 }
+EXPORT_SYMBOL(sdw_intel_acpi_scan);
 
+/**
+ * sdw_intel_probe() - SoundWire Intel probe routine
+ * @res: resource data
+ *
+ * This creates SoundWire Master and Slave devices below the controller.
+ * All the information necessary is stored in the context, and the res
+ * argument pointer can be freed after this step.
+ */
+struct sdw_intel_ctx
+*sdw_intel_probe(struct sdw_intel_res *res)
+{
+	return sdw_intel_probe_controller(res);
+}
+EXPORT_SYMBOL(sdw_intel_probe);
+
+/**
+ * sdw_intel_startup() - SoundWire Intel startup
+ * @ctx: SoundWire context allocated in the probe
+ *
+ */
+int sdw_intel_startup(struct sdw_intel_ctx *ctx)
+{
+	return sdw_intel_startup_controller(ctx);
+}
+EXPORT_SYMBOL(sdw_intel_startup);
 /**
  * sdw_intel_exit() - SoundWire Intel exit
- * @arg: callback context
+ * @ctx: SoundWire context allocated in the probe
  *
  * Delete the controller instances created and cleanup
  */
 void sdw_intel_exit(struct sdw_intel_ctx *ctx)
 {
-	sdw_intel_cleanup_pdev(ctx);
+	sdw_intel_cleanup(ctx);
+	driver_unregister(sdw_intel_link_ops.driver);
 	kfree(ctx);
 }
 EXPORT_SYMBOL(sdw_intel_exit);
-- 
2.17.1