[PATCH 0/5] soundwire: add sdw_master_device support on Qualcomm platforms

[PATCH 0/5] soundwire: add sdw_master_device support on Qualcomm platforms

[Pierre-Louis Bossart]

This patchset provides the support for sdw_master_device and
sdw_master_driver reviewed earlier and accepted as 'sane' by Greg KH.

This patchset only focuses on Qualcomm platforms and addresses all
previous comments and objections from Vinod Koul: there is no driver
used and no overhead added by this patchset. Many thanks to Srinivas
Kandagatla for testing these patches on Qualcomm devices and
contributing the missing DT parsing fix.

The transition from platform devices to sdw_master_devices on Intel
platforms was already provided and will be re-sent separately when
this infrastructure change is agreed.

Pierre-Louis Bossart (4):
  soundwire: bus_type: add master_device/driver support
  soundwire: bus_type: protect cases where no driver name is provided
  soundwire: qcom: fix error handling in probe
  soundwire: qcom: add sdw_master_device support

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

 drivers/soundwire/Makefile         |   2 +-
 drivers/soundwire/bus_type.c       | 143 +++++++++++++++++++++++++++--
 drivers/soundwire/master.c         | 130 ++++++++++++++++++++++++++
 drivers/soundwire/qcom.c           |  49 ++++++++--
 drivers/soundwire/slave.c          |   7 +-
 include/linux/soundwire/sdw.h      |  59 ++++++++++++
 include/linux/soundwire/sdw_type.h |  36 +++++++-
 7 files changed, 407 insertions(+), 19 deletions(-)
 create mode 100644 drivers/soundwire/master.c


base-commit: 1ce7139436603dda9e155df0c3e275c87a725761
-- 
2.20.1

[PATCH 1/5] soundwire: bus_type: add master_device/driver support

[Pierre-Louis Bossart]

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 new objects (sdw_master_device and
sdw_master_driver).

A parent (such as the Intel audio controller or its equivalent on
Qualcomm devices) would use sdw_master_device_add() to create the
device, passing a driver name as a parameter. The master device would
be released when device_unregister() is invoked by the parent.

Note that since there is no standard for the Master host-facing
interface, so the bus matching relies on a simple string matching (as
previously done with platform devices).

The 'Master Device' driver exposes callbacks for
probe/startup/shutdown/remove/process_wake. The startup and process
wake need to be called by the parent directly (using wrappers), while
the probe/shutdown/remove are handled by the SoundWire bus core upon
device creation and release.

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

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/Makefile         |   2 +-
 drivers/soundwire/bus_type.c       | 139 +++++++++++++++++++++++++++--
 drivers/soundwire/master.c         | 129 ++++++++++++++++++++++++++
 drivers/soundwire/slave.c          |   7 +-
 include/linux/soundwire/sdw.h      |  59 ++++++++++++
 include/linux/soundwire/sdw_type.h |  36 +++++++-
 6 files changed, 363 insertions(+), 9 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_type.c b/drivers/soundwire/bus_type.c
index 17f096dd6806..09a25075e770 100644
--- a/drivers/soundwire/bus_type.c
+++ b/drivers/soundwire/bus_type.c
@@ -33,13 +33,33 @@ 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;
+	struct sdw_master_device *md;
+	struct sdw_master_driver *mdrv;
+	int ret = 0;
 
-	return !!sdw_get_device_id(slave, drv);
+	if (is_sdw_slave(dev)) {
+		slave = dev_to_sdw_dev(dev);
+		drv = drv_to_sdw_driver(ddrv);
+
+		ret = !!sdw_get_device_id(slave, drv);
+	} else {
+		md = dev_to_sdw_master_device(dev);
+		mdrv = drv_to_sdw_master_driver(ddrv);
+
+		/*
+		 * we don't have any hardware information so
+		 * match with a hopefully unique string
+		 */
+		ret = !strncmp(md->master_name, mdrv->driver.name,
+			       strlen(md->master_name));
+	}
+	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,12 +67,31 @@ 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_master_modalias(const struct sdw_master_device *md,
+			       char *buf, size_t size)
+{
+	/* modalias is sdw:<string> since we don't have any hardware info */
+
+	return snprintf(buf, size, "sdw:%s\n",
+			md->master_name);
+}
+
 static int sdw_uevent(struct device *dev, struct kobj_uevent_env *env)
 {
-	struct sdw_slave *slave = dev_to_sdw_dev(dev);
+	struct sdw_slave *slave;
+	struct sdw_master_device *md;
 	char modalias[32];
 
-	sdw_slave_modalias(slave, modalias, sizeof(modalias));
+	if (is_sdw_slave(dev)) {
+		slave = dev_to_sdw_dev(dev);
+
+		sdw_slave_modalias(slave, modalias, sizeof(modalias));
+
+	} else {
+		md = dev_to_sdw_master_device(dev);
+
+		sdw_master_modalias(md, modalias, sizeof(modalias));
+	}
 
 	if (add_uevent_var(env, "MODALIAS=%s", modalias))
 		return -ENOMEM;
@@ -181,6 +220,94 @@ void sdw_unregister_driver(struct sdw_driver *drv)
 }
 EXPORT_SYMBOL_GPL(sdw_unregister_driver);
 
+static int sdw_master_drv_probe(struct device *dev)
+{
+	struct sdw_master_device *md = dev_to_sdw_master_device(dev);
+	struct sdw_master_driver *mdrv = drv_to_sdw_master_driver(dev->driver);
+	int ret;
+
+	/*
+	 * attach to power domain but don't turn on (last arg)
+	 */
+	ret = dev_pm_domain_attach(dev, false);
+	if (ret)
+		return ret;
+
+	ret = mdrv->probe(md, md->pdata);
+	if (ret) {
+		dev_err(dev, "Probe of %s failed: %d\n",
+			mdrv->driver.name, ret);
+		dev_pm_domain_detach(dev, false);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int sdw_master_drv_remove(struct device *dev)
+{
+	struct sdw_master_device *md = dev_to_sdw_master_device(dev);
+	struct sdw_master_driver *mdrv = drv_to_sdw_master_driver(dev->driver);
+	int ret = 0;
+
+	if (mdrv->remove)
+		ret = mdrv->remove(md);
+
+	dev_pm_domain_detach(dev, false);
+
+	return ret;
+}
+
+static void sdw_master_drv_shutdown(struct device *dev)
+{
+	struct sdw_master_device *md = dev_to_sdw_master_device(dev);
+	struct sdw_master_driver *mdrv = drv_to_sdw_master_driver(dev->driver);
+
+	if (mdrv->shutdown)
+		mdrv->shutdown(md);
+}
+
+/**
+ * __sdw_register_master_driver() - register a SoundWire Master driver
+ * @mdrv: 'Master driver' to register
+ * @owner: owning module/driver
+ *
+ * Return: zero on success, else a negative error code.
+ */
+int __sdw_register_master_driver(struct sdw_master_driver *mdrv,
+				 struct module *owner)
+{
+	mdrv->driver.bus = &sdw_bus_type;
+
+	if (!mdrv->probe) {
+		pr_err("driver %s didn't provide SDW probe routine\n",
+		       mdrv->driver.name);
+		return -EINVAL;
+	}
+
+	mdrv->driver.owner = owner;
+	mdrv->driver.probe = sdw_master_drv_probe;
+
+	if (mdrv->remove)
+		mdrv->driver.remove = sdw_master_drv_remove;
+
+	if (mdrv->shutdown)
+		mdrv->driver.shutdown = sdw_master_drv_shutdown;
+
+	return driver_register(&mdrv->driver);
+}
+EXPORT_SYMBOL_GPL(__sdw_register_master_driver);
+
+/**
+ * sdw_unregister_master_driver() - unregisters the SoundWire Master driver
+ * @mdrv: driver to unregister
+ */
+void sdw_unregister_master_driver(struct sdw_master_driver *mdrv)
+{
+	driver_unregister(&mdrv->driver);
+}
+EXPORT_SYMBOL_GPL(sdw_unregister_master_driver);
+
 static int __init sdw_bus_init(void)
 {
 	sdw_debugfs_init();
diff --git a/drivers/soundwire/master.c b/drivers/soundwire/master.c
new file mode 100644
index 000000000000..fbfa1c35137d
--- /dev/null
+++ b/drivers/soundwire/master.c
@@ -0,0 +1,129 @@
+// SPDX-License-Identifier: (GPL-2.0)
+// 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.
+ * @master_name: Linux driver name
+ * @parent: the parent Linux device (e.g. a PCI device)
+ * @fwnode: the parent fwnode (e.g. an ACPI companion device to the parent)
+ * @link_id: link index as defined by MIPI DisCo specification
+ * @pdata: private data (e.g. register base, offsets, platform quirks, etc).
+ *
+ * This function shall be called prior to the called to sdw_add_bus_master()
+ * with the bus.dev pointer set to the device allocated in this function.
+ */
+struct sdw_master_device
+*sdw_master_device_add(const char *master_name,
+		       struct device *parent,
+		       struct fwnode_handle *fwnode,
+		       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->master_name = master_name;
+
+	init_completion(&md->probe_complete);
+
+	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);
+
+	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);
+		return ERR_PTR(ret);
+	}
+
+	return md;
+}
+EXPORT_SYMBOL_GPL(sdw_master_device_add);
+
+/**
+ * sdw_master_device_startup() - startup hardware
+ * @md: Linux Soundwire master device
+ *
+ * This use of this function is optional. It is only needed if the
+ * hardware cannot be started during a driver probe, e.g. due to power
+ * rail dependencies. The implementation is platform-specific but the
+ * bus will typically go through a hardware-reset sequence and devices
+ * will be enumerated once they report as ATTACHED.
+ */
+int sdw_master_device_startup(struct sdw_master_device *md)
+{
+	struct sdw_master_driver *mdrv;
+	struct device *dev;
+	int ret = 0;
+
+	if (IS_ERR_OR_NULL(md))
+		return -EINVAL;
+
+	dev = &md->dev;
+	mdrv = drv_to_sdw_master_driver(dev->driver);
+
+	if (mdrv && mdrv->startup)
+		ret = mdrv->startup(md);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(sdw_master_device_startup);
+
+/**
+ * sdw_master_device_process_wake_event() - handle external wake event
+ * @md: Linux Soundwire master device
+ *
+ * The use of this function is optional, and only needed when e.g. an
+ * external wake event is provided by another subsystem, such as PCI.
+ */
+
+int sdw_master_device_process_wake_event(struct sdw_master_device *md)
+{
+	struct sdw_master_driver *mdrv;
+	struct device *dev;
+	int ret = 0;
+
+	if (IS_ERR_OR_NULL(md))
+		return -EINVAL;
+
+	dev = &md->dev;
+	mdrv = drv_to_sdw_master_driver(dev->driver);
+
+	if (mdrv && mdrv->process_wake_event)
+		ret = mdrv->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..7ca4f2d9bfa6 100644
--- a/drivers/soundwire/slave.c
+++ b/drivers/soundwire/slave.c
@@ -14,6 +14,11 @@ static void sdw_slave_release(struct device *dev)
 	kfree(slave);
 }
 
+struct device_type sdw_slave_type = {
+	.name =		"sdw_slave",
+	.release =	sdw_slave_release,
+};
+
 static int sdw_slave_add(struct sdw_bus *bus,
 			 struct sdw_slave_id *id, struct fwnode_handle *fwnode)
 {
@@ -41,9 +46,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..523b8fc86f7d 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -632,6 +632,31 @@ struct sdw_slave {
 
 #define dev_to_sdw_dev(_dev) container_of(_dev, struct sdw_slave, dev)
 
+/**
+ * struct sdw_master_device - SoundWire 'Master Device' representation
+ * @dev: Linux device for this Master
+ * @master_name: Linux driver name
+ * @driver: Linux driver for this Master (set by SoundWire core during probe)
+ * @probe_complete: used by parent if synchronous probe behavior is needed
+ * @link_id: link index as defined by MIPI DisCo specification
+ * @pm_runtime_suspended: flag set on system suspend and used on system resume.
+ * This is an optimization to avoid calling pm_runtime_suspended() twice.
+ * @pdata: private data typically provided with sdw_master_device_add()
+ */
+
+struct sdw_master_device {
+	struct device dev;
+	const char *master_name;
+	struct sdw_master_driver *driver;
+	struct completion probe_complete;
+	int link_id;
+	bool pm_runtime_suspended;
+	void *pdata;
+};
+
+#define dev_to_sdw_master_device(d)	\
+	container_of(d, struct sdw_master_device, dev)
+
 struct sdw_driver {
 	const char *name;
 
@@ -646,6 +671,29 @@ struct sdw_driver {
 	struct device_driver driver;
 };
 
+/**
+ * struct sdw_master_driver - SoundWire 'Master Device' driver
+ * @probe: initializations and allocation (hardware may not be enabled yet)
+ * @startup: initialization handled after the hardware is enabled, all
+ * clock/power dependencies are available (optional)
+ * @shutdown: cleanups before hardware is disabled (optional)
+ * @remove: free all remaining resources
+ * @process_wake_event: handle external wake (optional)
+ * @driver: baseline structure used for name/PM hooks.
+ *
+ * The use of sdw_master_driver is optional, and typically only needed
+ * when a controller has multiple links and needs to deal with power
+ * management at the link level.
+ */
+struct sdw_master_driver {
+	int (*probe)(struct sdw_master_device *md, void *link_ctx);
+	int (*startup)(struct sdw_master_device *md);
+	int (*shutdown)(struct sdw_master_device *md);
+	int (*remove)(struct sdw_master_device *md);
+	int (*process_wake_event)(struct sdw_master_device *md);
+	struct device_driver driver;
+};
+
 #define SDW_SLAVE_ENTRY(_mfg_id, _part_id, _drv_data) \
 	{ .mfg_id = (_mfg_id), .part_id = (_part_id), \
 	  .driver_data = (unsigned long)(_drv_data) }
@@ -835,6 +883,17 @@ 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(const char *master_name,
+		       struct device *parent,
+		       struct fwnode_handle *fwnode,
+		       int link_id,
+		       void *pdata);
+
+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..331ba58bda27 100644
--- a/include/linux/soundwire/sdw_type.h
+++ b/include/linux/soundwire/sdw_type.h
@@ -5,16 +5,36 @@
 #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)
 
 #define sdw_register_driver(drv) \
 	__sdw_register_driver(drv, THIS_MODULE)
 
+static inline int is_sdw_master_device(const struct device *dev)
+{
+	return dev->type == &sdw_master_type;
+}
+
+#define drv_to_sdw_master_driver(_drv) \
+	container_of(_drv, struct sdw_master_driver, driver)
+
+#define sdw_register_master_driver(drv) \
+	__sdw_register_master_driver(drv, THIS_MODULE)
+
 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);
+int __sdw_register_master_driver(struct sdw_master_driver *mdrv,
+				 struct module *owner);
+void sdw_unregister_master_driver(struct sdw_master_driver *mdrv);
 
 /**
  * module_sdw_driver() - Helper macro for registering a Soundwire driver
@@ -27,4 +47,18 @@ 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)
+
+/**
+ * module_sdw_master_driver() - Helper macro for registering a Soundwire
+ * Master driver
+ * @__sdw_master_driver: soundwire Master driver struct
+ *
+ * Helper macro for Soundwire Master drivers which do not do anything special in
+ * module init/exit. This eliminates a lot of boilerplate. Each module may only
+ * use this macro once, and calling it replaces module_init() and module_exit()
+ */
+#define module_sdw_master_driver(__sdw_master_driver) \
+	module_driver(__sdw_master_driver, sdw_register_master_driver, \
+			sdw_unregister_master_driver)
+
 #endif /* __SOUNDWIRE_TYPES_H */
-- 
2.20.1

Re: [PATCH 1/5] soundwire: bus_type: add master_device/driver support

[Srinivas Kandagatla]

On 20/03/2020 16:29, Pierre-Louis Bossart wrote:
> 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 new objects (sdw_master_device and
> sdw_master_driver).
> 
> A parent (such as the Intel audio controller or its equivalent on
> Qualcomm devices) would use sdw_master_device_add() to create the
> device, passing a driver name as a parameter. The master device would
> be released when device_unregister() is invoked by the parent.
> 
> Note that since there is no standard for the Master host-facing
> interface, so the bus matching relies on a simple string matching (as
> previously done with platform devices).
> 
> The 'Master Device' driver exposes callbacks for
> probe/startup/shutdown/remove/process_wake. The startup and process
> wake need to be called by the parent directly (using wrappers), while
> the probe/shutdown/remove are handled by the SoundWire bus core upon
> device creation and release.
> 
> Additional callbacks will be added in the future for e.g. autonomous
> clock stop modes.
> 
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>   drivers/soundwire/Makefile         |   2 +-
>   drivers/soundwire/bus_type.c       | 139 +++++++++++++++++++++++++++--
>   drivers/soundwire/master.c         | 129 ++++++++++++++++++++++++++
>   drivers/soundwire/slave.c          |   7 +-
>   include/linux/soundwire/sdw.h      |  59 ++++++++++++
>   include/linux/soundwire/sdw_type.h |  36 +++++++-
>   6 files changed, 363 insertions(+), 9 deletions(-)
>   create mode 100644 drivers/soundwire/master.c
> 


This patch in general is missing device tree support for both matching 
and uevent so this will not clearly work for Qualcomm controller unless 
we do via platform bus, which does not sound right!


> 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_type.c b/drivers/soundwire/bus_type.c
> index 17f096dd6806..09a25075e770 100644
> --- a/drivers/soundwire/bus_type.c
> +++ b/drivers/soundwire/bus_type.c
> @@ -33,13 +33,33 @@ 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;
> +	struct sdw_master_device *md;
> +	struct sdw_master_driver *mdrv;
> +	int ret = 0;
>   
> -	return !!sdw_get_device_id(slave, drv);
> +	if (is_sdw_slave(dev)) {
> +		slave = dev_to_sdw_dev(dev);
> +		drv = drv_to_sdw_driver(ddrv);
> +
> +		ret = !!sdw_get_device_id(slave, drv);
> +	} else {
> +		md = dev_to_sdw_master_device(dev);
> +		mdrv = drv_to_sdw_master_driver(ddrv);
> +
> +		/*
> +		 * we don't have any hardware information so
> +		 * match with a hopefully unique string
> +		 */
> +		ret = !strncmp(md->master_name, mdrv->driver.name,
> +			       strlen(md->master_name));
> +	}
> +	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,12 +67,31 @@ 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_master_modalias(const struct sdw_master_device *md,
> +			       char *buf, size_t size)
> +{
> +	/* modalias is sdw:<string> since we don't have any hardware info */
> +
> +	return snprintf(buf, size, "sdw:%s\n",
> +			md->master_name);
> +}
> +
>   static int sdw_uevent(struct device *dev, struct kobj_uevent_env *env)
>   {
> -	struct sdw_slave *slave = dev_to_sdw_dev(dev);
> +	struct sdw_slave *slave;
> +	struct sdw_master_device *md;
>   	char modalias[32];
>   
> -	sdw_slave_modalias(slave, modalias, sizeof(modalias));
> +	if (is_sdw_slave(dev)) {
> +		slave = dev_to_sdw_dev(dev);
> +
> +		sdw_slave_modalias(slave, modalias, sizeof(modalias));
> +
> +	} else {
> +		md = dev_to_sdw_master_device(dev);
> +
> +		sdw_master_modalias(md, modalias, sizeof(modalias));
> +	}
>   
>   	if (add_uevent_var(env, "MODALIAS=%s", modalias))
>   		return -ENOMEM;
> @@ -181,6 +220,94 @@ void sdw_unregister_driver(struct sdw_driver *drv)
>   }
>   EXPORT_SYMBOL_GPL(sdw_unregister_driver);
>   
> +static int sdw_master_drv_probe(struct device *dev)
> +{
> +	struct sdw_master_device *md = dev_to_sdw_master_device(dev);
> +	struct sdw_master_driver *mdrv = drv_to_sdw_master_driver(dev->driver);
> +	int ret;
> +
> +	/*
> +	 * attach to power domain but don't turn on (last arg)
> +	 */
> +	ret = dev_pm_domain_attach(dev, false);
> +	if (ret)
> +		return ret;
> +
> +	ret = mdrv->probe(md, md->pdata);
> +	if (ret) {
> +		dev_err(dev, "Probe of %s failed: %d\n",
> +			mdrv->driver.name, ret);
> +		dev_pm_domain_detach(dev, false);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int sdw_master_drv_remove(struct device *dev)
> +{
> +	struct sdw_master_device *md = dev_to_sdw_master_device(dev);
> +	struct sdw_master_driver *mdrv = drv_to_sdw_master_driver(dev->driver);
> +	int ret = 0;
> +
> +	if (mdrv->remove)
> +		ret = mdrv->remove(md);
> +
> +	dev_pm_domain_detach(dev, false);
> +
> +	return ret;
> +}
> +

...

> diff --git a/drivers/soundwire/master.c b/drivers/soundwire/master.c
> new file mode 100644
> index 000000000000..fbfa1c35137d
> --- /dev/null
> +++ b/drivers/soundwire/master.c
> @@ -0,0 +1,129 @@
> +// SPDX-License-Identifier: (GPL-2.0)
> +// 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"
> +

...

> +
> +/**
> + * sdw_master_device_startup() - startup hardware
> + * @md: Linux Soundwire master device
> + *
> + * This use of this function is optional. It is only needed if the
> + * hardware cannot be started during a driver probe, e.g. due to power
> + * rail dependencies. The implementation is platform-specific but the
> + * bus will typically go through a hardware-reset sequence and devices
> + * will be enumerated once they report as ATTACHED.
> + */
> +int sdw_master_device_startup(struct sdw_master_device *md)
> +{
> +	struct sdw_master_driver *mdrv;
> +	struct device *dev;
> +	int ret = 0;
> +
> +	if (IS_ERR_OR_NULL(md))
> +		return -EINVAL;
> +
> +	dev = &md->dev;
> +	mdrv = drv_to_sdw_master_driver(dev->driver);
> +
> +	if (mdrv && mdrv->startup)
> +		ret = mdrv->startup(md);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(sdw_master_device_startup);

Who would call this function? and How would it get hold of master device 
instance ?


How would soundwire core also ensure that we do not actively use this 
master if it is not ready. Similar comment for shutdown callback.

> +
> +/**
> + * sdw_master_device_process_wake_event() - handle external wake event
> + * @md: Linux Soundwire master device
> + *
> + * The use of this function is optional, and only needed when e.g. an
> + * external wake event is provided by another subsystem, such as PCI.
> + */
> +
> +int sdw_master_device_process_wake_event(struct sdw_master_device *md)
> +{
> +	struct sdw_master_driver *mdrv;
> +	struct device *dev;
> +	int ret = 0;
> +
> +	if (IS_ERR_OR_NULL(md))
> +		return -EINVAL;
> +
> +	dev = &md->dev;
> +	mdrv = drv_to_sdw_master_driver(dev->driver);
> +
> +	if (mdrv && mdrv->process_wake_event)
> +		ret = mdrv->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..7ca4f2d9bfa6 100644
> --- a/drivers/soundwire/slave.c
> +++ b/drivers/soundwire/slave.c
> @@ -14,6 +14,11 @@ static void sdw_slave_release(struct device *dev)
>   	kfree(slave);
>   }
>   
> +struct device_type sdw_slave_type = {
> +	.name =		"sdw_slave",
> +	.release =	sdw_slave_release,
> +};
> +
>   static int sdw_slave_add(struct sdw_bus *bus,
>   			 struct sdw_slave_id *id, struct fwnode_handle *fwnode)
>   {
> @@ -41,9 +46,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..523b8fc86f7d 100644
> --- a/include/linux/soundwire/sdw.h
> +++ b/include/linux/soundwire/sdw.h
> @@ -632,6 +632,31 @@ struct sdw_slave {
>   
>   #define dev_to_sdw_dev(_dev) container_of(_dev, struct sdw_slave, dev)
>   
> +/**
> + * struct sdw_master_device - SoundWire 'Master Device' representation
> + * @dev: Linux device for this Master
> + * @master_name: Linux driver name
> + * @driver: Linux driver for this Master (set by SoundWire core during probe)
> + * @probe_complete: used by parent if synchronous probe behavior is needed
> + * @link_id: link index as defined by MIPI DisCo specification
> + * @pm_runtime_suspended: flag set on system suspend and used on system resume.
> + * This is an optimization to avoid calling pm_runtime_suspended() twice.
> + * @pdata: private data typically provided with sdw_master_device_add()
> + */
> +
> +struct sdw_master_device {
> +	struct device dev;
> +	const char *master_name;
> +	struct sdw_master_driver *driver;
> +	struct completion probe_complete;
> +	int link_id;
> +	bool pm_runtime_suspended;
> +	void *pdata;
> +};
> +
> +#define dev_to_sdw_master_device(d)	\
> +	container_of(d, struct sdw_master_device, dev)
> +
>   struct sdw_driver {
>   	const char *name;
>   
> @@ -646,6 +671,29 @@ struct sdw_driver {
>   	struct device_driver driver;
>   };
>   
> +/**
> + * struct sdw_master_driver - SoundWire 'Master Device' driver
> + * @probe: initializations and allocation (hardware may not be enabled yet)
> + * @startup: initialization handled after the hardware is enabled, all
> + * clock/power dependencies are available (optional)
> + * @shutdown: cleanups before hardware is disabled (optional)
> + * @remove: free all remaining resources
> + * @process_wake_event: handle external wake (optional)
> + * @driver: baseline structure used for name/PM hooks.
> + *
> + * The use of sdw_master_driver is optional, and typically only needed
> + * when a controller has multiple links and needs to deal with power
> + * management at the link level.
> + */
> +struct sdw_master_driver {
> +	int (*probe)(struct sdw_master_device *md, void *link_ctx);
> +	int (*startup)(struct sdw_master_device *md);
> +	int (*shutdown)(struct sdw_master_device *md);
> +	int (*remove)(struct sdw_master_device *md);
> +	int (*process_wake_event)(struct sdw_master_device *md);
> +	struct device_driver driver;
> +};
> +
>   #define SDW_SLAVE_ENTRY(_mfg_id, _part_id, _drv_data) \
>   	{ .mfg_id = (_mfg_id), .part_id = (_part_id), \
>   	  .driver_data = (unsigned long)(_drv_data) }
> @@ -835,6 +883,17 @@ 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(const char *master_name,
> +		       struct device *parent,
> +		       struct fwnode_handle *fwnode,
> +		       int link_id,
> +		       void *pdata);
> +
> +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..331ba58bda27 100644
> --- a/include/linux/soundwire/sdw_type.h
> +++ b/include/linux/soundwire/sdw_type.h
> @@ -5,16 +5,36 @@
>   #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)
>   
>   #define sdw_register_driver(drv) \
>   	__sdw_register_driver(drv, THIS_MODULE)
>   
> +static inline int is_sdw_master_device(const struct device *dev)
> +{
> +	return dev->type == &sdw_master_type;
> +}
> +
> +#define drv_to_sdw_master_driver(_drv) \
> +	container_of(_drv, struct sdw_master_driver, driver)
> +
> +#define sdw_register_master_driver(drv) \
> +	__sdw_register_master_driver(drv, THIS_MODULE)
> +
>   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);
> +int __sdw_register_master_driver(struct sdw_master_driver *mdrv,
> +				 struct module *owner);
> +void sdw_unregister_master_driver(struct sdw_master_driver *mdrv);
>   
>   /**
>    * module_sdw_driver() - Helper macro for registering a Soundwire driver
> @@ -27,4 +47,18 @@ 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)
> +
> +/**
> + * module_sdw_master_driver() - Helper macro for registering a Soundwire
> + * Master driver
> + * @__sdw_master_driver: soundwire Master driver struct
> + *
> + * Helper macro for Soundwire Master drivers which do not do anything special in
> + * module init/exit. This eliminates a lot of boilerplate. Each module may only
> + * use this macro once, and calling it replaces module_init() and module_exit()
> + */
> +#define module_sdw_master_driver(__sdw_master_driver) \
> +	module_driver(__sdw_master_driver, sdw_register_master_driver, \
> +			sdw_unregister_master_driver)
> +
>   #endif /* __SOUNDWIRE_TYPES_H */
> 

Re: [PATCH 1/5] soundwire: bus_type: add master_device/driver support

[Pierre-Louis Bossart]

Thanks for the quick review Srinivas,

> This patch in general is missing device tree support for both matching 
> and uevent so this will not clearly work for Qualcomm controller unless 
> we do via platform bus, which does not sound right!

see other email, the platform bus is handled by a platform 
device/driver. There was no intention to change that, it's by design 
rather than an omission/error.
>> +
>> +/**
>> + * sdw_master_device_startup() - startup hardware
>> + * @md: Linux Soundwire master device
>> + *
>> + * This use of this function is optional. It is only needed if the
>> + * hardware cannot be started during a driver probe, e.g. due to power
>> + * rail dependencies. The implementation is platform-specific but the
>> + * bus will typically go through a hardware-reset sequence and devices
>> + * will be enumerated once they report as ATTACHED.
>> + */
>> +int sdw_master_device_startup(struct sdw_master_device *md)
>> +{
>> +    struct sdw_master_driver *mdrv;
>> +    struct device *dev;
>> +    int ret = 0;
>> +
>> +    if (IS_ERR_OR_NULL(md))
>> +        return -EINVAL;
>> +
>> +    dev = &md->dev;
>> +    mdrv = drv_to_sdw_master_driver(dev->driver);
>> +
>> +    if (mdrv && mdrv->startup)
>> +        ret = mdrv->startup(md);
>> +
>> +    return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(sdw_master_device_startup);
> 
> Who would call this function? and How would it get hold of master device 
> instance ?

sdw_master_device_add() returns a struct_master_device *md, so the 
parent has a handle to that device. See the device creation here:

https://github.com/thesofproject/linux/blob/9c7487b33072040ab755d32ca173b75151c0160c/drivers/soundwire/intel_init.c#L238

This startup() would be called by the parent when all 
requirements/dependencies are met. Put differently, it allows the probe 
to run much earlier and check for platform firmware information (which 
links are enabled, what devices are exposed by platform firmware), while 
the startup is really when the bus clk/data lines will start toggling.

https://github.com/thesofproject/linux/blob/9c7487b33072040ab755d32ca173b75151c0160c/drivers/soundwire/intel_init.c#L341

and the call from the SOF layer is here:

https://github.com/thesofproject/linux/blob/9c7487b33072040ab755d32ca173b75151c0160c/sound/soc/sof/intel/hda-loader.c#L418

Again, if everything is ready at probe time there's no need to delay the 
actual bus startup. This is not needed for Qualcomm platforms where the 
Master device is part of a codec. It's actually irrelevant since there 
is no driver, so the startup callback does not even exist :-)

> How would soundwire core also ensure that we do not actively use this 
> master if it is not ready. Similar comment for shutdown callback.

That's a fair point, we could add a state variable and a check that the 
probe happened before.

In practice the two cases of device creation and startup are different 
phases so it'd more of a paranoia check.

Thanks,
-Pierre

Re: [PATCH 1/5] soundwire: bus_type: add master_device/driver support

[Srinivas Kandagatla]

On 20/03/2020 18:17, Pierre-Louis Bossart wrote:
> Thanks for the quick review Srinivas,
> 
>> This patch in general is missing device tree support for both matching 
>> and uevent so this will not clearly work for Qualcomm controller 
>> unless we do via platform bus, which does not sound right!
> 
> see other email, the platform bus is handled by a platform 
> device/driver. There was no intention to change that, it's by design 
> rather than an omission/error.

I understand this partly now!

This can be probably made better/clear by:
renaming sdw_master_device_add to sdw_master_alloc and do a 
device_initialize() as part of this function in subsequent call to 
sdw_add_bus_master() we can do a device_add(). Doing this way will avoid 
a bit of unnecessary call to device_unregister by the controller driver, 
tbh which is confusing.

If the intended call sequence for controller is this (by keeping the 
parent bus type intact):

sdw_master_alloc/sdw_master_device_add()
sdw_add_bus_master()

Then we should also remove sdw_unregister_master_driver() and 
module_sdw_master_driver() all together. Having them makes the reader 
think that they can use module_sdw_master_driver directly without any 
parent bus like platform bus in this case.


>>> +
>>> +/**
>>> + * sdw_master_device_startup() - startup hardware
>>> + * @md: Linux Soundwire master device
>>> + *
>>> + * This use of this function is optional. It is only needed if the
>>> + * hardware cannot be started during a driver probe, e.g. due to power
>>> + * rail dependencies. The implementation is platform-specific but the
>>> + * bus will typically go through a hardware-reset sequence and devices
>>> + * will be enumerated once they report as ATTACHED.
>>> + */
>>> +int sdw_master_device_startup(struct sdw_master_device *md)
>>> +{
>>> +    struct sdw_master_driver *mdrv;
>>> +    struct device *dev;
>>> +    int ret = 0;
>>> +
>>> +    if (IS_ERR_OR_NULL(md))
>>> +        return -EINVAL;
>>> +
>>> +    dev = &md->dev;
>>> +    mdrv = drv_to_sdw_master_driver(dev->driver);
>>> +
>>> +    if (mdrv && mdrv->startup)
>>> +        ret = mdrv->startup(md);
>>> +
>>> +    return ret;
>>> +}
>>> +EXPORT_SYMBOL_GPL(sdw_master_device_startup);
>>
>> Who would call this function? and How would it get hold of master 
>> device instance ?
> 
> sdw_master_device_add() returns a struct_master_device *md, so the 
> parent has a handle to that device. See the device creation here:
> 
> https://github.com/thesofproject/linux/blob/9c7487b33072040ab755d32ca173b75151c0160c/drivers/soundwire/intel_init.c#L238 
> 
> 
> This startup() would be called by the parent when all 
> requirements/dependencies are met. Put differently, it allows the probe 
> to run much earlier and check for platform firmware information (which 
> links are enabled, what devices are exposed by platform firmware), while 
> the startup is really when the bus clk/data lines will start toggling.

Why would this even need to be in the SoundWire core layer, if parent 
can call its local startup function whenever its necessary?
Unless we have some kinda state-machine in the core which can really 
deal with this, i see no point in this callback.


--srini

> 
> https://github.com/thesofproject/linux/blob/9c7487b33072040ab755d32ca173b75151c0160c/drivers/soundwire/intel_init.c#L341 
> 
> 
> and the call from the SOF layer is here:
> 
> https://github.com/thesofproject/linux/blob/9c7487b33072040ab755d32ca173b75151c0160c/sound/soc/sof/intel/hda-loader.c#L418 
> 
> 
> Again, if everything is ready at probe time there's no need to delay the 
> actual bus startup. This is not needed for Qualcomm platforms where the 
> Master device is part of a codec. It's actually irrelevant since there 
> is no driver, so the startup callback does not even exist :-)
> 
>> How would soundwire core also ensure that we do not actively use this 
>> master if it is not ready. Similar comment for shutdown callback.
> 
> That's a fair point, we could add a state variable and a check that the 
> probe happened before.
> 
> In practice the two cases of device creation and startup are different 
> phases so it'd more of a paranoia check.
> 
> Thanks,
> -Pierre

Re: [PATCH 1/5] soundwire: bus_type: add master_device/driver support

[Vinod Koul]

On 23-03-20, 11:06, Srinivas Kandagatla wrote:
> 
> 
> On 20/03/2020 18:17, Pierre-Louis Bossart wrote:
> > Thanks for the quick review Srinivas,
> > 
> > > This patch in general is missing device tree support for both
> > > matching and uevent so this will not clearly work for Qualcomm
> > > controller unless we do via platform bus, which does not sound
> > > right!
> > 
> > see other email, the platform bus is handled by a platform
> > device/driver. There was no intention to change that, it's by design
> > rather than an omission/error.
> 
> I understand this partly now!
> 
> This can be probably made better/clear by:
> renaming sdw_master_device_add to sdw_master_alloc and do a
> device_initialize() as part of this function in subsequent call to
> sdw_add_bus_master() we can do a device_add(). Doing this way will avoid a
> bit of unnecessary call to device_unregister by the controller driver, tbh
> which is confusing.
> 
> If the intended call sequence for controller is this (by keeping the parent
> bus type intact):
> 
> sdw_master_alloc/sdw_master_device_add()
> sdw_add_bus_master()

why not have single bus api which does all this :)

> Then we should also remove sdw_unregister_master_driver() and
> module_sdw_master_driver() all together. Having them makes the reader think
> that they can use module_sdw_master_driver directly without any parent bus
> like platform bus in this case.

Precisely, this is one of the reasons for not liking the
sdw_master_driver! It doesnt get used by anyone except Intel.

-- 
~Vinod

[PATCH 2/5] soundwire: bus_type: protect cases where no driver name is provided

[Pierre-Louis Bossart]

When the implementation only creates a sdw_master_device and does not
provide a master_name, we have a risk of kernel oopses with dereferences
of a NULL pointer.

Protect with explicit tests.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/bus_type.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c
index 09a25075e770..c01d74c709d5 100644
--- a/drivers/soundwire/bus_type.c
+++ b/drivers/soundwire/bus_type.c
@@ -48,12 +48,14 @@ static int sdw_bus_match(struct device *dev, struct device_driver *ddrv)
 		md = dev_to_sdw_master_device(dev);
 		mdrv = drv_to_sdw_master_driver(ddrv);
 
-		/*
-		 * we don't have any hardware information so
-		 * match with a hopefully unique string
-		 */
-		ret = !strncmp(md->master_name, mdrv->driver.name,
-			       strlen(md->master_name));
+		if (md->master_name) {
+			/*
+			 * we don't have any hardware information so
+			 * match with a hopefully unique string
+			 */
+			ret = !strncmp(md->master_name, mdrv->driver.name,
+				       strlen(md->master_name));
+		}
 	}
 	return ret;
 }
@@ -71,9 +73,11 @@ static int sdw_master_modalias(const struct sdw_master_device *md,
 			       char *buf, size_t size)
 {
 	/* modalias is sdw:<string> since we don't have any hardware info */
-
-	return snprintf(buf, size, "sdw:%s\n",
-			md->master_name);
+	if (md->master_name)
+		return snprintf(buf, size, "sdw:%s\n",
+				md->master_name);
+	else
+		return snprintf(buf, size, "sdw:no_master_driver\n");
 }
 
 static int sdw_uevent(struct device *dev, struct kobj_uevent_env *env)
-- 
2.20.1

[PATCH 3/5] soundwire: master: use device node pointer from master device

[Pierre-Louis Bossart]

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>
---
 drivers/soundwire/master.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/soundwire/master.c b/drivers/soundwire/master.c
index fbfa1c35137d..817ad434d4af 100644
--- a/drivers/soundwire/master.c
+++ b/drivers/soundwire/master.c
@@ -51,6 +51,7 @@ struct sdw_master_device
 	init_completion(&md->probe_complete);
 
 	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.20.1

[PATCH 4/5] soundwire: qcom: fix error handling in probe

[Pierre-Louis Bossart]

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

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@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 1c6c6a2e0def..77783ae4b71d 100644
--- a/drivers/soundwire/qcom.c
+++ b/drivers/soundwire/qcom.c
@@ -756,12 +756,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);
 
@@ -778,7 +782,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;
@@ -805,28 +809,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.20.1

[PATCH 5/5] soundwire: qcom: add sdw_master_device support

[Pierre-Louis Bossart]

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_master_driver is registered so
the dais have to be registered using the platform_device and likely
all power management is handled at the platform device level.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/qcom.c | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
index 77783ae4b71d..86b46415e50b 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;
@@ -775,14 +776,31 @@ 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(NULL,	/* no driver name */
+					 dev,	/* platform device is parent */
+					 dev->fwnode,
+					 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;
+	}
+
+	/* 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;
@@ -809,14 +827,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);
@@ -832,6 +850,8 @@ static int qcom_swrm_probe(struct platform_device *pdev)
 
 err_master_add:
 	sdw_delete_bus_master(&ctrl->bus);
+err_md:
+	device_unregister(&ctrl->md->dev);
 err_clk:
 	clk_disable_unprepare(ctrl->hclk);
 err_init:
@@ -843,6 +863,7 @@ static int qcom_swrm_remove(struct platform_device *pdev)
 	struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(&pdev->dev);
 
 	sdw_delete_bus_master(&ctrl->bus);
+	device_unregister(&ctrl->md->dev);
 	clk_disable_unprepare(ctrl->hclk);
 
 	return 0;
-- 
2.20.1

Re: [PATCH 5/5] soundwire: qcom: add sdw_master_device support

[Srinivas Kandagatla]

On 20/03/2020 16:29, Pierre-Louis Bossart wrote:
> Add new device as a child of the platform device, following the
> following hierarchy:
> 
> platform_device
>      sdw_master_device
>          sdw_slave0

Why can't we just remove the platform device layer here and add 
sdw_master_device directly?

What is it stopping doing that?

--srini


> 	...
> 	sdw_slaveN
> 
> For the Qualcomm implementation no sdw_master_driver is registered so
> the dais have to be registered using the platform_device and likely
> all power management is handled at the platform device level.
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>   drivers/soundwire/qcom.c | 29 +++++++++++++++++++++++++----
>   1 file changed, 25 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
> index 77783ae4b71d..86b46415e50b 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;
> @@ -775,14 +776,31 @@ 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(NULL,	/* no driver name */
> +					 dev,	/* platform device is parent */
> +					 dev->fwnode,
> +					 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;
> +	}
> +
> +	/* 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;
> @@ -809,14 +827,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);
> @@ -832,6 +850,8 @@ static int qcom_swrm_probe(struct platform_device *pdev)
>   
>   err_master_add:
>   	sdw_delete_bus_master(&ctrl->bus);
> +err_md:
> +	device_unregister(&ctrl->md->dev);
>   err_clk:
>   	clk_disable_unprepare(ctrl->hclk);
>   err_init:
> @@ -843,6 +863,7 @@ static int qcom_swrm_remove(struct platform_device *pdev)
>   	struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(&pdev->dev);
>   
>   	sdw_delete_bus_master(&ctrl->bus);
> +	device_unregister(&ctrl->md->dev);
>   	clk_disable_unprepare(ctrl->hclk);
>   
>   	return 0;
> 

Re: [PATCH 5/5] soundwire: qcom: add sdw_master_device support

[Pierre-Louis Bossart]

>> Add new device as a child of the platform device, following the
>> following hierarchy:
>>
>> platform_device
>>      sdw_master_device
>>          sdw_slave0
> 
> Why can't we just remove the platform device layer here and add 
> sdw_master_device directly?
> 
> What is it stopping doing that?

The guidance from Greg was "no platform devices, unless you really are 
on a platform bus (i.e. Device tree.)". We never discussed changing the 
way the Device Tree parts are handled.

The main idea was to leave the parent (be it platform-device or PCI 
device) alone and not add new attributes or references to it.

The scheme here is similar to I2C/SPI, you have a platform device 
handled by the Device Tree baseline, and a driver create an 
i2c_adapter/spi_controller/sdw_master_device.

Re: [PATCH 5/5] soundwire: qcom: add sdw_master_device support

[Vinod Koul]

On 20-03-20, 17:01, Srinivas Kandagatla wrote:
> 
> 
> On 20/03/2020 16:29, Pierre-Louis Bossart wrote:
> > Add new device as a child of the platform device, following the
> > following hierarchy:
> > 
> > platform_device
> >      sdw_master_device
> >          sdw_slave0
> 
> Why can't we just remove the platform device layer here and add
> sdw_master_device directly?

In the case platform_device is the OF device your controller gets probed
on.

My thinking on this is that drivers should not be directly creating
sdw_master_device but it should be done by core as this device is for
core to use and handle. Ideally I would love that sdw_master_device is
created/handled by core, preferably this be handled as part of
sdw_add_bus_master().

But Pierre is trying to solve the limitation of the devices given by
ACPI and trying to add sdw_master_driver to handle that. I am not
convinced that we should do that.

-- 
~Vinod