[PATCH 0/3] soundwire: bus_type: add sdw_master_device support

[PATCH 0/3] soundwire: bus_type: add sdw_master_device support

[Bard Liao]

This series adds sdw master devices support.

Pierre-Louis Bossart (3):
  soundwire: bus: rename sdw_bus_master_add/delete, add arguments
  soundwire: bus_type: introduce sdw_slave_type and sdw_master_type
  soundwire: bus_type: add sdw_master_device support

 .../driver-api/soundwire/summary.rst          |  7 +-
 drivers/soundwire/Makefile                    |  2 +-
 drivers/soundwire/bus.c                       | 27 ++++---
 drivers/soundwire/bus.h                       |  3 +
 drivers/soundwire/bus_type.c                  | 19 +++--
 drivers/soundwire/intel.c                     |  9 ++-
 drivers/soundwire/master.c                    | 79 +++++++++++++++++++
 drivers/soundwire/qcom.c                      |  7 +-
 drivers/soundwire/slave.c                     |  8 +-
 include/linux/soundwire/sdw.h                 | 22 +++++-
 include/linux/soundwire/sdw_type.h            |  9 ++-
 11 files changed, 160 insertions(+), 32 deletions(-)
 create mode 100644 drivers/soundwire/master.c

-- 
2.17.1

[PATCH 0/3] soundwire: bus_type: add sdw_master_device support

[Bard Liao]

This series adds sdw master devices support.

Pierre-Louis Bossart (3):
  soundwire: bus: rename sdw_bus_master_add/delete, add arguments
  soundwire: bus_type: introduce sdw_slave_type and sdw_master_type
  soundwire: bus_type: add sdw_master_device support

 .../driver-api/soundwire/summary.rst          |  7 +-
 drivers/soundwire/Makefile                    |  2 +-
 drivers/soundwire/bus.c                       | 27 ++++---
 drivers/soundwire/bus.h                       |  3 +
 drivers/soundwire/bus_type.c                  | 19 +++--
 drivers/soundwire/intel.c                     |  9 ++-
 drivers/soundwire/master.c                    | 79 +++++++++++++++++++
 drivers/soundwire/qcom.c                      |  7 +-
 drivers/soundwire/slave.c                     |  8 +-
 include/linux/soundwire/sdw.h                 | 22 +++++-
 include/linux/soundwire/sdw_type.h            |  9 ++-
 11 files changed, 160 insertions(+), 32 deletions(-)
 create mode 100644 drivers/soundwire/master.c

-- 
2.17.1

[PATCH 1/3] soundwire: bus: rename sdw_bus_master_add/delete, add arguments

[Bard Liao]

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

In preparation for future extensions, rename functions to use
sdw_bus_master prefix and add a parent and fwnode argument to
sdw_bus_master_add to help with device registration in follow-up
patches.

No functionality change, just renames and additional arguments.

The Intel code is currently unused, the two additional arguments are
only needed for compilation.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
---
 Documentation/driver-api/soundwire/summary.rst |  7 ++++---
 drivers/soundwire/bus.c                        | 15 +++++++++------
 drivers/soundwire/intel.c                      |  9 +++++----
 drivers/soundwire/qcom.c                       |  6 +++---
 include/linux/soundwire/sdw.h                  |  5 +++--
 5 files changed, 24 insertions(+), 18 deletions(-)

diff --git a/Documentation/driver-api/soundwire/summary.rst b/Documentation/driver-api/soundwire/summary.rst
index 8193125a2bfb..01dcb954f6d7 100644
--- a/Documentation/driver-api/soundwire/summary.rst
+++ b/Documentation/driver-api/soundwire/summary.rst
@@ -101,10 +101,11 @@ Following is the Bus API to register the SoundWire Bus:
 
 .. code-block:: c
 
-	int sdw_add_bus_master(struct sdw_bus *bus)
+	int sdw_bus_master_add(struct sdw_bus *bus,
+				struct device *parent,
+				struct fwnode_handle)
 	{
-		if (!bus->dev)
-			return -ENODEV;
+		sdw_master_device_add(bus, parent, fwnode);
 
 		mutex_init(&bus->lock);
 		INIT_LIST_HEAD(&bus->slaves);
diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index 488c3c9e4947..18024ff770f8 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -10,13 +10,16 @@
 #include "bus.h"
 
 /**
- * sdw_add_bus_master() - add a bus Master instance
+ * sdw_bus_master_add() - add a bus Master instance
  * @bus: bus instance
+ * @parent: parent device
+ * @fwnode: firmware node handle
  *
  * Initializes the bus instance, read properties and create child
  * devices.
  */
-int sdw_add_bus_master(struct sdw_bus *bus)
+int sdw_bus_master_add(struct sdw_bus *bus, struct device *parent,
+		       struct fwnode_handle *fwnode)
 {
 	struct sdw_master_prop *prop = NULL;
 	int ret;
@@ -107,7 +110,7 @@ int sdw_add_bus_master(struct sdw_bus *bus)
 
 	return 0;
 }
-EXPORT_SYMBOL(sdw_add_bus_master);
+EXPORT_SYMBOL(sdw_bus_master_add);
 
 static int sdw_delete_slave(struct device *dev, void *data)
 {
@@ -131,18 +134,18 @@ static int sdw_delete_slave(struct device *dev, void *data)
 }
 
 /**
- * sdw_delete_bus_master() - delete the bus master instance
+ * sdw_bus_master_delete() - delete the bus master instance
  * @bus: bus to be deleted
  *
  * Remove the instance, delete the child devices.
  */
-void sdw_delete_bus_master(struct sdw_bus *bus)
+void sdw_bus_master_delete(struct sdw_bus *bus)
 {
 	device_for_each_child(bus->dev, NULL, sdw_delete_slave);
 
 	sdw_bus_debugfs_exit(bus);
 }
-EXPORT_SYMBOL(sdw_delete_bus_master);
+EXPORT_SYMBOL(sdw_bus_master_delete);
 
 /*
  * SDW IO Calls
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index ed8d576bf5dc..1b600f423d8b 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -1110,9 +1110,10 @@ static int intel_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, sdw);
 
-	ret = sdw_add_bus_master(&sdw->cdns.bus);
+	/* 2nd and 3rd arguments are just added for compilation */
+	ret = sdw_bus_master_add(&sdw->cdns.bus, NULL, NULL);
 	if (ret) {
-		dev_err(&pdev->dev, "sdw_add_bus_master fail: %d\n", ret);
+		dev_err(&pdev->dev, "sdw_bus_master_add fail: %d\n", ret);
 		return ret;
 	}
 
@@ -1173,7 +1174,7 @@ static int intel_probe(struct platform_device *pdev)
 	sdw_cdns_enable_interrupt(&sdw->cdns, false);
 	free_irq(sdw->link_res->irq, sdw);
 err_init:
-	sdw_delete_bus_master(&sdw->cdns.bus);
+	sdw_bus_master_delete(&sdw->cdns.bus);
 	return ret;
 }
 
@@ -1189,7 +1190,7 @@ static int intel_remove(struct platform_device *pdev)
 		free_irq(sdw->link_res->irq, sdw);
 		snd_soc_unregister_component(sdw->cdns.dev);
 	}
-	sdw_delete_bus_master(&sdw->cdns.bus);
+	sdw_bus_master_delete(&sdw->cdns.bus);
 
 	return 0;
 }
diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
index e08a17c13f92..401811d6627e 100644
--- a/drivers/soundwire/qcom.c
+++ b/drivers/soundwire/qcom.c
@@ -821,7 +821,7 @@ static int qcom_swrm_probe(struct platform_device *pdev)
 		goto err_clk;
 	}
 
-	ret = sdw_add_bus_master(&ctrl->bus);
+	ret = sdw_bus_master_add(&ctrl->bus, dev, dev->fwnode);
 	if (ret) {
 		dev_err(dev, "Failed to register Soundwire controller (%d)\n",
 			ret);
@@ -840,7 +840,7 @@ static int qcom_swrm_probe(struct platform_device *pdev)
 	return 0;
 
 err_master_add:
-	sdw_delete_bus_master(&ctrl->bus);
+	sdw_bus_master_delete(&ctrl->bus);
 err_clk:
 	clk_disable_unprepare(ctrl->hclk);
 err_init:
@@ -851,7 +851,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);
+	sdw_bus_master_delete(&ctrl->bus);
 	clk_disable_unprepare(ctrl->hclk);
 
 	return 0;
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index 00f5826092e3..2003e8c55538 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -832,8 +832,9 @@ struct sdw_bus {
 	bool multi_link;
 };
 
-int sdw_add_bus_master(struct sdw_bus *bus);
-void sdw_delete_bus_master(struct sdw_bus *bus);
+int sdw_bus_master_add(struct sdw_bus *bus, struct device *parent,
+		       struct fwnode_handle *fwnode);
+void sdw_bus_master_delete(struct sdw_bus *bus);
 
 /**
  * sdw_port_config: Master or Slave Port configuration
-- 
2.17.1

[PATCH 1/3] soundwire: bus: rename sdw_bus_master_add/delete, add arguments

[Bard Liao]

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

In preparation for future extensions, rename functions to use
sdw_bus_master prefix and add a parent and fwnode argument to
sdw_bus_master_add to help with device registration in follow-up
patches.

No functionality change, just renames and additional arguments.

The Intel code is currently unused, the two additional arguments are
only needed for compilation.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
---
 Documentation/driver-api/soundwire/summary.rst |  7 ++++---
 drivers/soundwire/bus.c                        | 15 +++++++++------
 drivers/soundwire/intel.c                      |  9 +++++----
 drivers/soundwire/qcom.c                       |  6 +++---
 include/linux/soundwire/sdw.h                  |  5 +++--
 5 files changed, 24 insertions(+), 18 deletions(-)

diff --git a/Documentation/driver-api/soundwire/summary.rst b/Documentation/driver-api/soundwire/summary.rst
index 8193125a2bfb..01dcb954f6d7 100644
--- a/Documentation/driver-api/soundwire/summary.rst
+++ b/Documentation/driver-api/soundwire/summary.rst
@@ -101,10 +101,11 @@ Following is the Bus API to register the SoundWire Bus:
 
 .. code-block:: c
 
-	int sdw_add_bus_master(struct sdw_bus *bus)
+	int sdw_bus_master_add(struct sdw_bus *bus,
+				struct device *parent,
+				struct fwnode_handle)
 	{
-		if (!bus->dev)
-			return -ENODEV;
+		sdw_master_device_add(bus, parent, fwnode);
 
 		mutex_init(&bus->lock);
 		INIT_LIST_HEAD(&bus->slaves);
diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index 488c3c9e4947..18024ff770f8 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -10,13 +10,16 @@
 #include "bus.h"
 
 /**
- * sdw_add_bus_master() - add a bus Master instance
+ * sdw_bus_master_add() - add a bus Master instance
  * @bus: bus instance
+ * @parent: parent device
+ * @fwnode: firmware node handle
  *
  * Initializes the bus instance, read properties and create child
  * devices.
  */
-int sdw_add_bus_master(struct sdw_bus *bus)
+int sdw_bus_master_add(struct sdw_bus *bus, struct device *parent,
+		       struct fwnode_handle *fwnode)
 {
 	struct sdw_master_prop *prop = NULL;
 	int ret;
@@ -107,7 +110,7 @@ int sdw_add_bus_master(struct sdw_bus *bus)
 
 	return 0;
 }
-EXPORT_SYMBOL(sdw_add_bus_master);
+EXPORT_SYMBOL(sdw_bus_master_add);
 
 static int sdw_delete_slave(struct device *dev, void *data)
 {
@@ -131,18 +134,18 @@ static int sdw_delete_slave(struct device *dev, void *data)
 }
 
 /**
- * sdw_delete_bus_master() - delete the bus master instance
+ * sdw_bus_master_delete() - delete the bus master instance
  * @bus: bus to be deleted
  *
  * Remove the instance, delete the child devices.
  */
-void sdw_delete_bus_master(struct sdw_bus *bus)
+void sdw_bus_master_delete(struct sdw_bus *bus)
 {
 	device_for_each_child(bus->dev, NULL, sdw_delete_slave);
 
 	sdw_bus_debugfs_exit(bus);
 }
-EXPORT_SYMBOL(sdw_delete_bus_master);
+EXPORT_SYMBOL(sdw_bus_master_delete);
 
 /*
  * SDW IO Calls
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index ed8d576bf5dc..1b600f423d8b 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -1110,9 +1110,10 @@ static int intel_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, sdw);
 
-	ret = sdw_add_bus_master(&sdw->cdns.bus);
+	/* 2nd and 3rd arguments are just added for compilation */
+	ret = sdw_bus_master_add(&sdw->cdns.bus, NULL, NULL);
 	if (ret) {
-		dev_err(&pdev->dev, "sdw_add_bus_master fail: %d\n", ret);
+		dev_err(&pdev->dev, "sdw_bus_master_add fail: %d\n", ret);
 		return ret;
 	}
 
@@ -1173,7 +1174,7 @@ static int intel_probe(struct platform_device *pdev)
 	sdw_cdns_enable_interrupt(&sdw->cdns, false);
 	free_irq(sdw->link_res->irq, sdw);
 err_init:
-	sdw_delete_bus_master(&sdw->cdns.bus);
+	sdw_bus_master_delete(&sdw->cdns.bus);
 	return ret;
 }
 
@@ -1189,7 +1190,7 @@ static int intel_remove(struct platform_device *pdev)
 		free_irq(sdw->link_res->irq, sdw);
 		snd_soc_unregister_component(sdw->cdns.dev);
 	}
-	sdw_delete_bus_master(&sdw->cdns.bus);
+	sdw_bus_master_delete(&sdw->cdns.bus);
 
 	return 0;
 }
diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
index e08a17c13f92..401811d6627e 100644
--- a/drivers/soundwire/qcom.c
+++ b/drivers/soundwire/qcom.c
@@ -821,7 +821,7 @@ static int qcom_swrm_probe(struct platform_device *pdev)
 		goto err_clk;
 	}
 
-	ret = sdw_add_bus_master(&ctrl->bus);
+	ret = sdw_bus_master_add(&ctrl->bus, dev, dev->fwnode);
 	if (ret) {
 		dev_err(dev, "Failed to register Soundwire controller (%d)\n",
 			ret);
@@ -840,7 +840,7 @@ static int qcom_swrm_probe(struct platform_device *pdev)
 	return 0;
 
 err_master_add:
-	sdw_delete_bus_master(&ctrl->bus);
+	sdw_bus_master_delete(&ctrl->bus);
 err_clk:
 	clk_disable_unprepare(ctrl->hclk);
 err_init:
@@ -851,7 +851,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);
+	sdw_bus_master_delete(&ctrl->bus);
 	clk_disable_unprepare(ctrl->hclk);
 
 	return 0;
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index 00f5826092e3..2003e8c55538 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -832,8 +832,9 @@ struct sdw_bus {
 	bool multi_link;
 };
 
-int sdw_add_bus_master(struct sdw_bus *bus);
-void sdw_delete_bus_master(struct sdw_bus *bus);
+int sdw_bus_master_add(struct sdw_bus *bus, struct device *parent,
+		       struct fwnode_handle *fwnode);
+void sdw_bus_master_delete(struct sdw_bus *bus);
 
 /**
  * sdw_port_config: Master or Slave Port configuration
-- 
2.17.1

[PATCH 2/3] soundwire: bus_type: introduce sdw_slave_type and sdw_master_type

[Bard Liao]

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

this is a preparatory patch before the introduction of the
sdw_master_type. The SoundWire slave support is slightly modified with
the use of a sdw_slave_type, and the uevent handling move to
slave.c (since it's not necessary for the master).

No functionality change other than moving code around.

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/bus_type.c       | 19 +++++++++++++------
 drivers/soundwire/slave.c          |  8 +++++++-
 include/linux/soundwire/sdw_type.h |  9 ++++++++-
 3 files changed, 28 insertions(+), 8 deletions(-)

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/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_type.h b/include/linux/soundwire/sdw_type.h
index aaa7f4267c14..52eb66cd11bc 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,7 +21,7 @@ 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);
+int sdw_slave_uevent(struct device *dev, struct kobj_uevent_env *env);
 
 /**
  * module_sdw_driver() - Helper macro for registering a Soundwire driver
-- 
2.17.1

[PATCH 2/3] soundwire: bus_type: introduce sdw_slave_type and sdw_master_type

[Bard Liao]

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

this is a preparatory patch before the introduction of the
sdw_master_type. The SoundWire slave support is slightly modified with
the use of a sdw_slave_type, and the uevent handling move to
slave.c (since it's not necessary for the master).

No functionality change other than moving code around.

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/bus_type.c       | 19 +++++++++++++------
 drivers/soundwire/slave.c          |  8 +++++++-
 include/linux/soundwire/sdw_type.h |  9 ++++++++-
 3 files changed, 28 insertions(+), 8 deletions(-)

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/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_type.h b/include/linux/soundwire/sdw_type.h
index aaa7f4267c14..52eb66cd11bc 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,7 +21,7 @@ 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);
+int sdw_slave_uevent(struct device *dev, struct kobj_uevent_env *env);
 
 /**
  * module_sdw_driver() - Helper macro for registering a Soundwire driver
-- 
2.17.1

[PATCH 3/3] 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 transition will avoid abusing platform devices and allow for
better sysfs support without the reference count issues mentioned in
the initial reviews.

The sdw_master_device addition is done with minimal internal plumbing
and not exposed externally. The existing API based on
sdw_bus_master_add() and sdw_bus_master_delete() will deal with the
sdw_master_device life cycle, which minimizes changes to existing
drivers.

Note that the Intel code will be modified in follow-up patches (no
impact on any platform since the connection with ASoC is not supported
upstream so far).

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.c       | 12 ++++--
 drivers/soundwire/bus.h       |  3 ++
 drivers/soundwire/master.c    | 79 +++++++++++++++++++++++++++++++++++
 drivers/soundwire/qcom.c      |  1 -
 include/linux/soundwire/sdw.h | 17 +++++++-
 6 files changed, 108 insertions(+), 6 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.c b/drivers/soundwire/bus.c
index 18024ff770f8..7eb1e6efd567 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -24,9 +24,14 @@ int sdw_bus_master_add(struct sdw_bus *bus, struct device *parent,
 	struct sdw_master_prop *prop = NULL;
 	int ret;
 
-	if (!bus->dev) {
-		pr_err("SoundWire bus has no device\n");
-		return -ENODEV;
+	if (!bus)
+		return -EINVAL;
+
+	ret = sdw_master_device_add(bus, parent, fwnode);
+	if (ret) {
+		dev_err(parent, "Failed to add master device at link %d\n",
+			bus->link_id);
+		return ret;
 	}
 
 	if (!bus->ops) {
@@ -142,6 +147,7 @@ static int sdw_delete_slave(struct device *dev, void *data)
 void sdw_bus_master_delete(struct sdw_bus *bus)
 {
 	device_for_each_child(bus->dev, NULL, sdw_delete_slave);
+	sdw_master_device_del(bus);
 
 	sdw_bus_debugfs_exit(bus);
 }
diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h
index 204204a26db8..93ab0234a491 100644
--- a/drivers/soundwire/bus.h
+++ b/drivers/soundwire/bus.h
@@ -19,6 +19,9 @@ static inline int sdw_acpi_find_slaves(struct sdw_bus *bus)
 int sdw_of_find_slaves(struct sdw_bus *bus);
 void sdw_extract_slave_id(struct sdw_bus *bus,
 			  u64 addr, struct sdw_slave_id *id);
+int sdw_master_device_add(struct sdw_bus *bus, struct device *parent,
+			  struct fwnode_handle *fwnode);
+int sdw_master_device_del(struct sdw_bus *bus);
 
 #ifdef CONFIG_DEBUG_FS
 void sdw_bus_debugfs_init(struct sdw_bus *bus);
diff --git a/drivers/soundwire/master.c b/drivers/soundwire/master.c
new file mode 100644
index 000000000000..2eeb2d7f56e0
--- /dev/null
+++ b/drivers/soundwire/master.c
@@ -0,0 +1,79 @@
+// 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"
+
+/* nothing to free but this function is mandatory */
+static void sdw_master_device_release(struct device *dev)
+{
+}
+
+struct device_type sdw_master_type = {
+	.name =		"soundwire_master",
+	.release =	sdw_master_device_release,
+};
+
+/**
+ * sdw_master_device_add() - create a Linux Master Device representation.
+ * @bus: SDW bus instance
+ * @parent: parent device
+ * @fwnode: firmware node handle
+ */
+int sdw_master_device_add(struct sdw_bus *bus, struct device *parent,
+			  struct fwnode_handle *fwnode)
+{
+	struct sdw_master_device *md;
+	int ret;
+
+	if (!bus)
+		return -EINVAL;
+
+	/*
+	 * Unlike traditional devices, there's no allocation here since the
+	 * sdw_master_device is embedded in the bus structure.
+	 */
+	md = &bus->md;
+	md->dev.bus = &sdw_bus_type;
+	md->dev.type = &sdw_master_type;
+	md->dev.parent = parent;
+	md->dev.of_node = parent->of_node;
+	md->dev.fwnode = fwnode;
+	md->dev.dma_mask = parent->dma_mask;
+
+	dev_set_name(&md->dev, "sdw-master-%d", bus->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);
+		goto device_register_err;
+	}
+
+	/* add shortcuts to improve code readability/compactness */
+	md->bus = bus;
+	bus->dev = &md->dev;
+
+device_register_err:
+	return ret;
+}
+
+/**
+ * sdw_master_device_del() - delete a Linux Master Device representation.
+ * @bus: bus handle
+ *
+ * This function is the dual of sdw_master_device_add()
+ */
+int sdw_master_device_del(struct sdw_bus *bus)
+{
+	device_unregister(bus->dev);
+
+	return 0;
+}
diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
index 401811d6627e..1c335ab1cd3f 100644
--- a/drivers/soundwire/qcom.c
+++ b/drivers/soundwire/qcom.c
@@ -784,7 +784,6 @@ 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;
 	ctrl->bus.ops = &qcom_swrm_ops;
 	ctrl->bus.port_ops = &qcom_swrm_port_ops;
 	ctrl->bus.compute_params = &qcom_swrm_compute_params;
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index 2003e8c55538..071adf2b463f 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -632,6 +632,19 @@ 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
+ * @bus: Bus handle shortcut to improve readability (same as container_of)
+ */
+struct sdw_master_device {
+	struct device dev;
+	struct sdw_bus *bus;
+};
+
+#define dev_to_sdw_master_device(d)	\
+	container_of(d, struct sdw_master_device, dev)
+
 struct sdw_driver {
 	const char *name;
 
@@ -787,7 +800,8 @@ struct sdw_master_ops {
 
 /**
  * struct sdw_bus - SoundWire bus
- * @dev: Master linux device
+ * @dev: shortcut to &md->dev to improve readability
+ * @md: Master device
  * @link_id: Link id number, can be 0 to N, unique for each Master
  * @slaves: list of Slaves on this bus
  * @assigned: Bitmap for Slave device numbers.
@@ -812,6 +826,7 @@ struct sdw_master_ops {
  */
 struct sdw_bus {
 	struct device *dev;
+	struct sdw_master_device md;
 	unsigned int link_id;
 	struct list_head slaves;
 	DECLARE_BITMAP(assigned, SDW_MAX_DEVICES);
-- 
2.17.1

[PATCH 3/3] 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 transition will avoid abusing platform devices and allow for
better sysfs support without the reference count issues mentioned in
the initial reviews.

The sdw_master_device addition is done with minimal internal plumbing
and not exposed externally. The existing API based on
sdw_bus_master_add() and sdw_bus_master_delete() will deal with the
sdw_master_device life cycle, which minimizes changes to existing
drivers.

Note that the Intel code will be modified in follow-up patches (no
impact on any platform since the connection with ASoC is not supported
upstream so far).

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.c       | 12 ++++--
 drivers/soundwire/bus.h       |  3 ++
 drivers/soundwire/master.c    | 79 +++++++++++++++++++++++++++++++++++
 drivers/soundwire/qcom.c      |  1 -
 include/linux/soundwire/sdw.h | 17 +++++++-
 6 files changed, 108 insertions(+), 6 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.c b/drivers/soundwire/bus.c
index 18024ff770f8..7eb1e6efd567 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -24,9 +24,14 @@ int sdw_bus_master_add(struct sdw_bus *bus, struct device *parent,
 	struct sdw_master_prop *prop = NULL;
 	int ret;
 
-	if (!bus->dev) {
-		pr_err("SoundWire bus has no device\n");
-		return -ENODEV;
+	if (!bus)
+		return -EINVAL;
+
+	ret = sdw_master_device_add(bus, parent, fwnode);
+	if (ret) {
+		dev_err(parent, "Failed to add master device at link %d\n",
+			bus->link_id);
+		return ret;
 	}
 
 	if (!bus->ops) {
@@ -142,6 +147,7 @@ static int sdw_delete_slave(struct device *dev, void *data)
 void sdw_bus_master_delete(struct sdw_bus *bus)
 {
 	device_for_each_child(bus->dev, NULL, sdw_delete_slave);
+	sdw_master_device_del(bus);
 
 	sdw_bus_debugfs_exit(bus);
 }
diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h
index 204204a26db8..93ab0234a491 100644
--- a/drivers/soundwire/bus.h
+++ b/drivers/soundwire/bus.h
@@ -19,6 +19,9 @@ static inline int sdw_acpi_find_slaves(struct sdw_bus *bus)
 int sdw_of_find_slaves(struct sdw_bus *bus);
 void sdw_extract_slave_id(struct sdw_bus *bus,
 			  u64 addr, struct sdw_slave_id *id);
+int sdw_master_device_add(struct sdw_bus *bus, struct device *parent,
+			  struct fwnode_handle *fwnode);
+int sdw_master_device_del(struct sdw_bus *bus);
 
 #ifdef CONFIG_DEBUG_FS
 void sdw_bus_debugfs_init(struct sdw_bus *bus);
diff --git a/drivers/soundwire/master.c b/drivers/soundwire/master.c
new file mode 100644
index 000000000000..2eeb2d7f56e0
--- /dev/null
+++ b/drivers/soundwire/master.c
@@ -0,0 +1,79 @@
+// 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"
+
+/* nothing to free but this function is mandatory */
+static void sdw_master_device_release(struct device *dev)
+{
+}
+
+struct device_type sdw_master_type = {
+	.name =		"soundwire_master",
+	.release =	sdw_master_device_release,
+};
+
+/**
+ * sdw_master_device_add() - create a Linux Master Device representation.
+ * @bus: SDW bus instance
+ * @parent: parent device
+ * @fwnode: firmware node handle
+ */
+int sdw_master_device_add(struct sdw_bus *bus, struct device *parent,
+			  struct fwnode_handle *fwnode)
+{
+	struct sdw_master_device *md;
+	int ret;
+
+	if (!bus)
+		return -EINVAL;
+
+	/*
+	 * Unlike traditional devices, there's no allocation here since the
+	 * sdw_master_device is embedded in the bus structure.
+	 */
+	md = &bus->md;
+	md->dev.bus = &sdw_bus_type;
+	md->dev.type = &sdw_master_type;
+	md->dev.parent = parent;
+	md->dev.of_node = parent->of_node;
+	md->dev.fwnode = fwnode;
+	md->dev.dma_mask = parent->dma_mask;
+
+	dev_set_name(&md->dev, "sdw-master-%d", bus->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);
+		goto device_register_err;
+	}
+
+	/* add shortcuts to improve code readability/compactness */
+	md->bus = bus;
+	bus->dev = &md->dev;
+
+device_register_err:
+	return ret;
+}
+
+/**
+ * sdw_master_device_del() - delete a Linux Master Device representation.
+ * @bus: bus handle
+ *
+ * This function is the dual of sdw_master_device_add()
+ */
+int sdw_master_device_del(struct sdw_bus *bus)
+{
+	device_unregister(bus->dev);
+
+	return 0;
+}
diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
index 401811d6627e..1c335ab1cd3f 100644
--- a/drivers/soundwire/qcom.c
+++ b/drivers/soundwire/qcom.c
@@ -784,7 +784,6 @@ 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;
 	ctrl->bus.ops = &qcom_swrm_ops;
 	ctrl->bus.port_ops = &qcom_swrm_port_ops;
 	ctrl->bus.compute_params = &qcom_swrm_compute_params;
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index 2003e8c55538..071adf2b463f 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -632,6 +632,19 @@ 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
+ * @bus: Bus handle shortcut to improve readability (same as container_of)
+ */
+struct sdw_master_device {
+	struct device dev;
+	struct sdw_bus *bus;
+};
+
+#define dev_to_sdw_master_device(d)	\
+	container_of(d, struct sdw_master_device, dev)
+
 struct sdw_driver {
 	const char *name;
 
@@ -787,7 +800,8 @@ struct sdw_master_ops {
 
 /**
  * struct sdw_bus - SoundWire bus
- * @dev: Master linux device
+ * @dev: shortcut to &md->dev to improve readability
+ * @md: Master device
  * @link_id: Link id number, can be 0 to N, unique for each Master
  * @slaves: list of Slaves on this bus
  * @assigned: Bitmap for Slave device numbers.
@@ -812,6 +826,7 @@ struct sdw_master_ops {
  */
 struct sdw_bus {
 	struct device *dev;
+	struct sdw_master_device md;
 	unsigned int link_id;
 	struct list_head slaves;
 	DECLARE_BITMAP(assigned, SDW_MAX_DEVICES);
-- 
2.17.1

Re: [PATCH 3/3] soundwire: bus_type: add sdw_master_device support

[Vinod Koul]

On 30-04-20, 02:51, Bard Liao wrote:
> @@ -24,9 +24,14 @@ int sdw_bus_master_add(struct sdw_bus *bus, struct device *parent,
>  	struct sdw_master_prop *prop = NULL;
>  	int ret;
>  
> -	if (!bus->dev) {
> -		pr_err("SoundWire bus has no device\n");
> -		return -ENODEV;

This check is removed and not moved into sdw_master_device_add() either,
can you add here or in patch 1 and keep checking the parent device please

> +int sdw_master_device_add(struct sdw_bus *bus, struct device *parent,
> +			  struct fwnode_handle *fwnode)
> +{
> +	struct sdw_master_device *md;
> +	int ret;
> +
> +	if (!bus)
> +		return -EINVAL;
> +
> +	/*
> +	 * Unlike traditional devices, there's no allocation here since the
> +	 * sdw_master_device is embedded in the bus structure.
> +	 */

Looking at this and empty sdw_master_device_release() above, makes me
wonder if it is a wise move? Should we rather allocate the
sdw_master_device() and then free that up in sdw_master_device_release()
or it is really overkill given that this is called when we remove the
sdw_bus instance as well...

> +	md = &bus->md;
> +	md->dev.bus = &sdw_bus_type;
> +	md->dev.type = &sdw_master_type;
> +	md->dev.parent = parent;
> +	md->dev.of_node = parent->of_node;
> +	md->dev.fwnode = fwnode;
> +	md->dev.dma_mask = parent->dma_mask;
> +
> +	dev_set_name(&md->dev, "sdw-master-%d", bus->link_id);

This give nice sdw-master-0. In DT this comes from reg property. I dont
seem to recall if the ACPI/Disco spec treats link_id as unique across
the system, can you check that please, if not we would need to update
this.

-- 
~Vinod

Re: [PATCH 3/3] soundwire: bus_type: add sdw_master_device support

[Vinod Koul]

On 30-04-20, 02:51, Bard Liao wrote:
> @@ -24,9 +24,14 @@ int sdw_bus_master_add(struct sdw_bus *bus, struct device *parent,
>  	struct sdw_master_prop *prop = NULL;
>  	int ret;
>  
> -	if (!bus->dev) {
> -		pr_err("SoundWire bus has no device\n");
> -		return -ENODEV;

This check is removed and not moved into sdw_master_device_add() either,
can you add here or in patch 1 and keep checking the parent device please

> +int sdw_master_device_add(struct sdw_bus *bus, struct device *parent,
> +			  struct fwnode_handle *fwnode)
> +{
> +	struct sdw_master_device *md;
> +	int ret;
> +
> +	if (!bus)
> +		return -EINVAL;
> +
> +	/*
> +	 * Unlike traditional devices, there's no allocation here since the
> +	 * sdw_master_device is embedded in the bus structure.
> +	 */

Looking at this and empty sdw_master_device_release() above, makes me
wonder if it is a wise move? Should we rather allocate the
sdw_master_device() and then free that up in sdw_master_device_release()
or it is really overkill given that this is called when we remove the
sdw_bus instance as well...

> +	md = &bus->md;
> +	md->dev.bus = &sdw_bus_type;
> +	md->dev.type = &sdw_master_type;
> +	md->dev.parent = parent;
> +	md->dev.of_node = parent->of_node;
> +	md->dev.fwnode = fwnode;
> +	md->dev.dma_mask = parent->dma_mask;
> +
> +	dev_set_name(&md->dev, "sdw-master-%d", bus->link_id);

This give nice sdw-master-0. In DT this comes from reg property. I dont
seem to recall if the ACPI/Disco spec treats link_id as unique across
the system, can you check that please, if not we would need to update
this.

-- 
~Vinod

RE: [PATCH 3/3] soundwire: bus_type: add sdw_master_device support

[Liao, Bard]

> -----Original Message-----
> From: Vinod Koul <vkoul@kernel.org>
> Sent: Monday, May 11, 2020 2:32 PM
> To: Bard Liao <yung-chuan.liao@linux.intel.com>
> Cc: alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org; tiwai@suse.de;
> broonie@kernel.org; gregkh@linuxfoundation.org; jank@cadence.com;
> srinivas.kandagatla@linaro.org; rander.wang@linux.intel.com;
> ranjani.sridharan@linux.intel.com; hui.wang@canonical.com; pierre-
> louis.bossart@linux.intel.com; Kale, Sanyog R <sanyog.r.kale@intel.com>;
> Blauciak, Slawomir <slawomir.blauciak@intel.com>; Lin, Mengdong
> <mengdong.lin@intel.com>; Liao, Bard <bard.liao@intel.com>
> Subject: Re: [PATCH 3/3] soundwire: bus_type: add sdw_master_device support
> 
> On 30-04-20, 02:51, Bard Liao wrote:
> > @@ -24,9 +24,14 @@ int sdw_bus_master_add(struct sdw_bus *bus, struct
> device *parent,
> >  	struct sdw_master_prop *prop = NULL;
> >  	int ret;
> >
> > -	if (!bus->dev) {
> > -		pr_err("SoundWire bus has no device\n");
> > -		return -ENODEV;
> 
> This check is removed and not moved into sdw_master_device_add() either, can
> you add here or in patch 1 and keep checking the parent device please

We will set bus->dev = &md->dev in the end of sdw_master_device_add().
That's why we remove the test. But now I am wandering does it make sense
to set bus->dev = &md->dev? Maybe it makes more sense to set bus->dev =
sdw control device. 
A follow up question is that should slave device a child of bus device or
master device? I would prefer bus device if it makes sense. 
I will check bus->dev and parent and remove bus->dev = &md->dev in the
next version.


> 
> > +int sdw_master_device_add(struct sdw_bus *bus, struct device *parent,
> > +			  struct fwnode_handle *fwnode)
> > +{
> > +	struct sdw_master_device *md;
> > +	int ret;
> > +
> > +	if (!bus)
> > +		return -EINVAL;
> > +
> > +	/*
> > +	 * Unlike traditional devices, there's no allocation here since the
> > +	 * sdw_master_device is embedded in the bus structure.
> > +	 */
> 
> Looking at this and empty sdw_master_device_release() above, makes me
> wonder if it is a wise move? Should we rather allocate the
> sdw_master_device() and then free that up in sdw_master_device_release() or it
> is really overkill given that this is called when we remove the sdw_bus instance
> as well...

Yes, I will allocate sdw_master_device here and free it in
sdw_master_device_release().

> 
> > +	md = &bus->md;
> > +	md->dev.bus = &sdw_bus_type;
> > +	md->dev.type = &sdw_master_type;
> > +	md->dev.parent = parent;
> > +	md->dev.of_node = parent->of_node;
> > +	md->dev.fwnode = fwnode;
> > +	md->dev.dma_mask = parent->dma_mask;
> > +
> > +	dev_set_name(&md->dev, "sdw-master-%d", bus->link_id);
> 
> This give nice sdw-master-0. In DT this comes from reg property. I dont seem to
> recall if the ACPI/Disco spec treats link_id as unique across the system, can you
> check that please, if not we would need to update this.

Sure, I will check it.

> 
> --
> ~Vinod

RE: [PATCH 3/3] soundwire: bus_type: add sdw_master_device support

[Liao, Bard]

> -----Original Message-----
> From: Vinod Koul <vkoul@kernel.org>
> Sent: Monday, May 11, 2020 2:32 PM
> To: Bard Liao <yung-chuan.liao@linux.intel.com>
> Cc: alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org; tiwai@suse.de;
> broonie@kernel.org; gregkh@linuxfoundation.org; jank@cadence.com;
> srinivas.kandagatla@linaro.org; rander.wang@linux.intel.com;
> ranjani.sridharan@linux.intel.com; hui.wang@canonical.com; pierre-
> louis.bossart@linux.intel.com; Kale, Sanyog R <sanyog.r.kale@intel.com>;
> Blauciak, Slawomir <slawomir.blauciak@intel.com>; Lin, Mengdong
> <mengdong.lin@intel.com>; Liao, Bard <bard.liao@intel.com>
> Subject: Re: [PATCH 3/3] soundwire: bus_type: add sdw_master_device support
> 
> On 30-04-20, 02:51, Bard Liao wrote:
> > @@ -24,9 +24,14 @@ int sdw_bus_master_add(struct sdw_bus *bus, struct
> device *parent,
> >  	struct sdw_master_prop *prop = NULL;
> >  	int ret;
> >
> > -	if (!bus->dev) {
> > -		pr_err("SoundWire bus has no device\n");
> > -		return -ENODEV;
> 
> This check is removed and not moved into sdw_master_device_add() either, can
> you add here or in patch 1 and keep checking the parent device please

We will set bus->dev = &md->dev in the end of sdw_master_device_add().
That's why we remove the test. But now I am wandering does it make sense
to set bus->dev = &md->dev? Maybe it makes more sense to set bus->dev =
sdw control device. 
A follow up question is that should slave device a child of bus device or
master device? I would prefer bus device if it makes sense. 
I will check bus->dev and parent and remove bus->dev = &md->dev in the
next version.


> 
> > +int sdw_master_device_add(struct sdw_bus *bus, struct device *parent,
> > +			  struct fwnode_handle *fwnode)
> > +{
> > +	struct sdw_master_device *md;
> > +	int ret;
> > +
> > +	if (!bus)
> > +		return -EINVAL;
> > +
> > +	/*
> > +	 * Unlike traditional devices, there's no allocation here since the
> > +	 * sdw_master_device is embedded in the bus structure.
> > +	 */
> 
> Looking at this and empty sdw_master_device_release() above, makes me
> wonder if it is a wise move? Should we rather allocate the
> sdw_master_device() and then free that up in sdw_master_device_release() or it
> is really overkill given that this is called when we remove the sdw_bus instance
> as well...

Yes, I will allocate sdw_master_device here and free it in
sdw_master_device_release().

> 
> > +	md = &bus->md;
> > +	md->dev.bus = &sdw_bus_type;
> > +	md->dev.type = &sdw_master_type;
> > +	md->dev.parent = parent;
> > +	md->dev.of_node = parent->of_node;
> > +	md->dev.fwnode = fwnode;
> > +	md->dev.dma_mask = parent->dma_mask;
> > +
> > +	dev_set_name(&md->dev, "sdw-master-%d", bus->link_id);
> 
> This give nice sdw-master-0. In DT this comes from reg property. I dont seem to
> recall if the ACPI/Disco spec treats link_id as unique across the system, can you
> check that please, if not we would need to update this.

Sure, I will check it.

> 
> --
> ~Vinod

Re: [PATCH 3/3] soundwire: bus_type: add sdw_master_device support

[Vinod Koul]

On 11-05-20, 08:04, Liao, Bard wrote:
> > -----Original Message-----
> > From: Vinod Koul <vkoul@kernel.org>
> > Sent: Monday, May 11, 2020 2:32 PM
> > To: Bard Liao <yung-chuan.liao@linux.intel.com>
> > Cc: alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org; tiwai@suse.de;
> > broonie@kernel.org; gregkh@linuxfoundation.org; jank@cadence.com;
> > srinivas.kandagatla@linaro.org; rander.wang@linux.intel.com;
> > ranjani.sridharan@linux.intel.com; hui.wang@canonical.com; pierre-
> > louis.bossart@linux.intel.com; Kale, Sanyog R <sanyog.r.kale@intel.com>;
> > Blauciak, Slawomir <slawomir.blauciak@intel.com>; Lin, Mengdong
> > <mengdong.lin@intel.com>; Liao, Bard <bard.liao@intel.com>
> > Subject: Re: [PATCH 3/3] soundwire: bus_type: add sdw_master_device support
> > 
> > On 30-04-20, 02:51, Bard Liao wrote:
> > > @@ -24,9 +24,14 @@ int sdw_bus_master_add(struct sdw_bus *bus, struct
> > device *parent,
> > >  	struct sdw_master_prop *prop = NULL;
> > >  	int ret;
> > >
> > > -	if (!bus->dev) {
> > > -		pr_err("SoundWire bus has no device\n");
> > > -		return -ENODEV;
> > 
> > This check is removed and not moved into sdw_master_device_add() either, can
> > you add here or in patch 1 and keep checking the parent device please
> 
> We will set bus->dev = &md->dev in the end of sdw_master_device_add().

We need to test if this is valid or not :)

> That's why we remove the test. But now I am wandering does it make sense
> to set bus->dev = &md->dev? Maybe it makes more sense to set bus->dev =
> sdw control device. 
> A follow up question is that should slave device a child of bus device or
> master device? I would prefer bus device if it makes sense. 
> I will check bus->dev and parent and remove bus->dev = &md->dev in the
> next version.

the parent is bus->dev and sdw_master_device created would be child of
this and should be set as such. You can remove it from bus object and
keep in sdw_master_device object, that is fine by me.

The sdw_slave is child of sdw_master_device now and looks to be set
correct.

-- 
~Vinod

Re: [PATCH 3/3] soundwire: bus_type: add sdw_master_device support

[Vinod Koul]

On 11-05-20, 08:04, Liao, Bard wrote:
> > -----Original Message-----
> > From: Vinod Koul <vkoul@kernel.org>
> > Sent: Monday, May 11, 2020 2:32 PM
> > To: Bard Liao <yung-chuan.liao@linux.intel.com>
> > Cc: alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org; tiwai@suse.de;
> > broonie@kernel.org; gregkh@linuxfoundation.org; jank@cadence.com;
> > srinivas.kandagatla@linaro.org; rander.wang@linux.intel.com;
> > ranjani.sridharan@linux.intel.com; hui.wang@canonical.com; pierre-
> > louis.bossart@linux.intel.com; Kale, Sanyog R <sanyog.r.kale@intel.com>;
> > Blauciak, Slawomir <slawomir.blauciak@intel.com>; Lin, Mengdong
> > <mengdong.lin@intel.com>; Liao, Bard <bard.liao@intel.com>
> > Subject: Re: [PATCH 3/3] soundwire: bus_type: add sdw_master_device support
> > 
> > On 30-04-20, 02:51, Bard Liao wrote:
> > > @@ -24,9 +24,14 @@ int sdw_bus_master_add(struct sdw_bus *bus, struct
> > device *parent,
> > >  	struct sdw_master_prop *prop = NULL;
> > >  	int ret;
> > >
> > > -	if (!bus->dev) {
> > > -		pr_err("SoundWire bus has no device\n");
> > > -		return -ENODEV;
> > 
> > This check is removed and not moved into sdw_master_device_add() either, can
> > you add here or in patch 1 and keep checking the parent device please
> 
> We will set bus->dev = &md->dev in the end of sdw_master_device_add().

We need to test if this is valid or not :)

> That's why we remove the test. But now I am wandering does it make sense
> to set bus->dev = &md->dev? Maybe it makes more sense to set bus->dev =
> sdw control device. 
> A follow up question is that should slave device a child of bus device or
> master device? I would prefer bus device if it makes sense. 
> I will check bus->dev and parent and remove bus->dev = &md->dev in the
> next version.

the parent is bus->dev and sdw_master_device created would be child of
this and should be set as such. You can remove it from bus object and
keep in sdw_master_device object, that is fine by me.

The sdw_slave is child of sdw_master_device now and looks to be set
correct.

-- 
~Vinod

RE: [PATCH 3/3] soundwire: bus_type: add sdw_master_device support

[Liao, Bard]

> -----Original Message-----
> From: Vinod Koul <vkoul@kernel.org>
> Sent: Monday, May 11, 2020 5:00 PM
> To: Liao, Bard <bard.liao@intel.com>
> Cc: Bard Liao <yung-chuan.liao@linux.intel.com>; alsa-devel@alsa-project.org;
> linux-kernel@vger.kernel.org; tiwai@suse.de; broonie@kernel.org;
> gregkh@linuxfoundation.org; jank@cadence.com;
> srinivas.kandagatla@linaro.org; rander.wang@linux.intel.com;
> ranjani.sridharan@linux.intel.com; hui.wang@canonical.com; pierre-
> louis.bossart@linux.intel.com; Kale, Sanyog R <sanyog.r.kale@intel.com>;
> Blauciak, Slawomir <slawomir.blauciak@intel.com>; Lin, Mengdong
> <mengdong.lin@intel.com>
> Subject: Re: [PATCH 3/3] soundwire: bus_type: add sdw_master_device support
> 
> On 11-05-20, 08:04, Liao, Bard wrote:
> > > -----Original Message-----
> > > From: Vinod Koul <vkoul@kernel.org>
> > > Sent: Monday, May 11, 2020 2:32 PM
> > > To: Bard Liao <yung-chuan.liao@linux.intel.com>
> > > Cc: alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org;
> > > tiwai@suse.de; broonie@kernel.org; gregkh@linuxfoundation.org;
> > > jank@cadence.com; srinivas.kandagatla@linaro.org;
> > > rander.wang@linux.intel.com; ranjani.sridharan@linux.intel.com;
> > > hui.wang@canonical.com; pierre- louis.bossart@linux.intel.com; Kale,
> > > Sanyog R <sanyog.r.kale@intel.com>; Blauciak, Slawomir
> > > <slawomir.blauciak@intel.com>; Lin, Mengdong
> > > <mengdong.lin@intel.com>; Liao, Bard <bard.liao@intel.com>
> > > Subject: Re: [PATCH 3/3] soundwire: bus_type: add sdw_master_device
> > > support
> > >
> > > On 30-04-20, 02:51, Bard Liao wrote:
> > > > @@ -24,9 +24,14 @@ int sdw_bus_master_add(struct sdw_bus *bus,
> > > > struct
> > > device *parent,
> > > >  	struct sdw_master_prop *prop = NULL;
> > > >  	int ret;
> > > >
> > > > -	if (!bus->dev) {
> > > > -		pr_err("SoundWire bus has no device\n");
> > > > -		return -ENODEV;
> > >
> > > This check is removed and not moved into sdw_master_device_add()
> > > either, can you add here or in patch 1 and keep checking the parent
> > > device please
> >
> > We will set bus->dev = &md->dev in the end of sdw_master_device_add().
> 
> We need to test if this is valid or not :)
> 
> > That's why we remove the test. But now I am wandering does it make
> > sense to set bus->dev = &md->dev? Maybe it makes more sense to set
> > bus->dev = sdw control device.
> > A follow up question is that should slave device a child of bus device
> > or master device? I would prefer bus device if it makes sense.
> > I will check bus->dev and parent and remove bus->dev = &md->dev in the
> > next version.
> 
> the parent is bus->dev and sdw_master_device created would be child of this
> and should be set as such. You can remove it from bus object and keep in
> sdw_master_device object, that is fine by me.

Looks like we don't need the parent and fwnode parameter since we can
get them from bus->dev 😊

> 
> The sdw_slave is child of sdw_master_device now and looks to be set correct.

So, it will be
bus device
    -> master device
         -> slave device
right?

I have a question here. We have pm supported on bus and slave devices,
but not master device. Will pm work with this architecture?
Can it be
bus device
    -> master device & slave device?


> 
> --
> ~Vinod

RE: [PATCH 3/3] soundwire: bus_type: add sdw_master_device support

[Liao, Bard]

> -----Original Message-----
> From: Vinod Koul <vkoul@kernel.org>
> Sent: Monday, May 11, 2020 5:00 PM
> To: Liao, Bard <bard.liao@intel.com>
> Cc: Bard Liao <yung-chuan.liao@linux.intel.com>; alsa-devel@alsa-project.org;
> linux-kernel@vger.kernel.org; tiwai@suse.de; broonie@kernel.org;
> gregkh@linuxfoundation.org; jank@cadence.com;
> srinivas.kandagatla@linaro.org; rander.wang@linux.intel.com;
> ranjani.sridharan@linux.intel.com; hui.wang@canonical.com; pierre-
> louis.bossart@linux.intel.com; Kale, Sanyog R <sanyog.r.kale@intel.com>;
> Blauciak, Slawomir <slawomir.blauciak@intel.com>; Lin, Mengdong
> <mengdong.lin@intel.com>
> Subject: Re: [PATCH 3/3] soundwire: bus_type: add sdw_master_device support
> 
> On 11-05-20, 08:04, Liao, Bard wrote:
> > > -----Original Message-----
> > > From: Vinod Koul <vkoul@kernel.org>
> > > Sent: Monday, May 11, 2020 2:32 PM
> > > To: Bard Liao <yung-chuan.liao@linux.intel.com>
> > > Cc: alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org;
> > > tiwai@suse.de; broonie@kernel.org; gregkh@linuxfoundation.org;
> > > jank@cadence.com; srinivas.kandagatla@linaro.org;
> > > rander.wang@linux.intel.com; ranjani.sridharan@linux.intel.com;
> > > hui.wang@canonical.com; pierre- louis.bossart@linux.intel.com; Kale,
> > > Sanyog R <sanyog.r.kale@intel.com>; Blauciak, Slawomir
> > > <slawomir.blauciak@intel.com>; Lin, Mengdong
> > > <mengdong.lin@intel.com>; Liao, Bard <bard.liao@intel.com>
> > > Subject: Re: [PATCH 3/3] soundwire: bus_type: add sdw_master_device
> > > support
> > >
> > > On 30-04-20, 02:51, Bard Liao wrote:
> > > > @@ -24,9 +24,14 @@ int sdw_bus_master_add(struct sdw_bus *bus,
> > > > struct
> > > device *parent,
> > > >  	struct sdw_master_prop *prop = NULL;
> > > >  	int ret;
> > > >
> > > > -	if (!bus->dev) {
> > > > -		pr_err("SoundWire bus has no device\n");
> > > > -		return -ENODEV;
> > >
> > > This check is removed and not moved into sdw_master_device_add()
> > > either, can you add here or in patch 1 and keep checking the parent
> > > device please
> >
> > We will set bus->dev = &md->dev in the end of sdw_master_device_add().
> 
> We need to test if this is valid or not :)
> 
> > That's why we remove the test. But now I am wandering does it make
> > sense to set bus->dev = &md->dev? Maybe it makes more sense to set
> > bus->dev = sdw control device.
> > A follow up question is that should slave device a child of bus device
> > or master device? I would prefer bus device if it makes sense.
> > I will check bus->dev and parent and remove bus->dev = &md->dev in the
> > next version.
> 
> the parent is bus->dev and sdw_master_device created would be child of this
> and should be set as such. You can remove it from bus object and keep in
> sdw_master_device object, that is fine by me.

Looks like we don't need the parent and fwnode parameter since we can
get them from bus->dev 😊

> 
> The sdw_slave is child of sdw_master_device now and looks to be set correct.

So, it will be
bus device
    -> master device
         -> slave device
right?

I have a question here. We have pm supported on bus and slave devices,
but not master device. Will pm work with this architecture?
Can it be
bus device
    -> master device & slave device?


> 
> --
> ~Vinod

Re: [PATCH 3/3] soundwire: bus_type: add sdw_master_device support

[Vinod Koul]

On 11-05-20, 11:34, Liao, Bard wrote:
> 
> 
> > -----Original Message-----
> > From: Vinod Koul <vkoul@kernel.org>
> > Sent: Monday, May 11, 2020 5:00 PM
> > To: Liao, Bard <bard.liao@intel.com>
> > Cc: Bard Liao <yung-chuan.liao@linux.intel.com>; alsa-devel@alsa-project.org;
> > linux-kernel@vger.kernel.org; tiwai@suse.de; broonie@kernel.org;
> > gregkh@linuxfoundation.org; jank@cadence.com;
> > srinivas.kandagatla@linaro.org; rander.wang@linux.intel.com;
> > ranjani.sridharan@linux.intel.com; hui.wang@canonical.com; pierre-
> > louis.bossart@linux.intel.com; Kale, Sanyog R <sanyog.r.kale@intel.com>;
> > Blauciak, Slawomir <slawomir.blauciak@intel.com>; Lin, Mengdong
> > <mengdong.lin@intel.com>
> > Subject: Re: [PATCH 3/3] soundwire: bus_type: add sdw_master_device support
> > 
> > On 11-05-20, 08:04, Liao, Bard wrote:
> > > > -----Original Message-----
> > > > From: Vinod Koul <vkoul@kernel.org>
> > > > Sent: Monday, May 11, 2020 2:32 PM
> > > > To: Bard Liao <yung-chuan.liao@linux.intel.com>
> > > > Cc: alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org;
> > > > tiwai@suse.de; broonie@kernel.org; gregkh@linuxfoundation.org;
> > > > jank@cadence.com; srinivas.kandagatla@linaro.org;
> > > > rander.wang@linux.intel.com; ranjani.sridharan@linux.intel.com;
> > > > hui.wang@canonical.com; pierre- louis.bossart@linux.intel.com; Kale,
> > > > Sanyog R <sanyog.r.kale@intel.com>; Blauciak, Slawomir
> > > > <slawomir.blauciak@intel.com>; Lin, Mengdong
> > > > <mengdong.lin@intel.com>; Liao, Bard <bard.liao@intel.com>
> > > > Subject: Re: [PATCH 3/3] soundwire: bus_type: add sdw_master_device
> > > > support
> > > >
> > > > On 30-04-20, 02:51, Bard Liao wrote:
> > > > > @@ -24,9 +24,14 @@ int sdw_bus_master_add(struct sdw_bus *bus,
> > > > > struct
> > > > device *parent,
> > > > >  	struct sdw_master_prop *prop = NULL;
> > > > >  	int ret;
> > > > >
> > > > > -	if (!bus->dev) {
> > > > > -		pr_err("SoundWire bus has no device\n");
> > > > > -		return -ENODEV;
> > > >
> > > > This check is removed and not moved into sdw_master_device_add()
> > > > either, can you add here or in patch 1 and keep checking the parent
> > > > device please
> > >
> > > We will set bus->dev = &md->dev in the end of sdw_master_device_add().
> > 
> > We need to test if this is valid or not :)
> > 
> > > That's why we remove the test. But now I am wandering does it make
> > > sense to set bus->dev = &md->dev? Maybe it makes more sense to set
> > > bus->dev = sdw control device.
> > > A follow up question is that should slave device a child of bus device
> > > or master device? I would prefer bus device if it makes sense.
> > > I will check bus->dev and parent and remove bus->dev = &md->dev in the
> > > next version.
> > 
> > the parent is bus->dev and sdw_master_device created would be child of this
> > and should be set as such. You can remove it from bus object and keep in
> > sdw_master_device object, that is fine by me.
> 
> Looks like we don't need the parent and fwnode parameter since we can
> get them from bus->dev 😊

Quite right

> > The sdw_slave is child of sdw_master_device now and looks to be set correct.
> 
> So, it will be
> bus device
>     -> master device
>          -> slave device
> right?

yes

> 
> I have a question here. We have pm supported on bus and slave devices,
> but not master device. Will pm work with this architecture?
> Can it be
> bus device
>     -> master device & slave device?

yes it would and you should check it out. The pm (runtime_pm) works well
with child devices and parents, so we need to ensure that parents are
set properly.

Thanks
-- 
~Vinod

Re: [PATCH 3/3] soundwire: bus_type: add sdw_master_device support

[Vinod Koul]

On 11-05-20, 11:34, Liao, Bard wrote:
> 
> 
> > -----Original Message-----
> > From: Vinod Koul <vkoul@kernel.org>
> > Sent: Monday, May 11, 2020 5:00 PM
> > To: Liao, Bard <bard.liao@intel.com>
> > Cc: Bard Liao <yung-chuan.liao@linux.intel.com>; alsa-devel@alsa-project.org;
> > linux-kernel@vger.kernel.org; tiwai@suse.de; broonie@kernel.org;
> > gregkh@linuxfoundation.org; jank@cadence.com;
> > srinivas.kandagatla@linaro.org; rander.wang@linux.intel.com;
> > ranjani.sridharan@linux.intel.com; hui.wang@canonical.com; pierre-
> > louis.bossart@linux.intel.com; Kale, Sanyog R <sanyog.r.kale@intel.com>;
> > Blauciak, Slawomir <slawomir.blauciak@intel.com>; Lin, Mengdong
> > <mengdong.lin@intel.com>
> > Subject: Re: [PATCH 3/3] soundwire: bus_type: add sdw_master_device support
> > 
> > On 11-05-20, 08:04, Liao, Bard wrote:
> > > > -----Original Message-----
> > > > From: Vinod Koul <vkoul@kernel.org>
> > > > Sent: Monday, May 11, 2020 2:32 PM
> > > > To: Bard Liao <yung-chuan.liao@linux.intel.com>
> > > > Cc: alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org;
> > > > tiwai@suse.de; broonie@kernel.org; gregkh@linuxfoundation.org;
> > > > jank@cadence.com; srinivas.kandagatla@linaro.org;
> > > > rander.wang@linux.intel.com; ranjani.sridharan@linux.intel.com;
> > > > hui.wang@canonical.com; pierre- louis.bossart@linux.intel.com; Kale,
> > > > Sanyog R <sanyog.r.kale@intel.com>; Blauciak, Slawomir
> > > > <slawomir.blauciak@intel.com>; Lin, Mengdong
> > > > <mengdong.lin@intel.com>; Liao, Bard <bard.liao@intel.com>
> > > > Subject: Re: [PATCH 3/3] soundwire: bus_type: add sdw_master_device
> > > > support
> > > >
> > > > On 30-04-20, 02:51, Bard Liao wrote:
> > > > > @@ -24,9 +24,14 @@ int sdw_bus_master_add(struct sdw_bus *bus,
> > > > > struct
> > > > device *parent,
> > > > >  	struct sdw_master_prop *prop = NULL;
> > > > >  	int ret;
> > > > >
> > > > > -	if (!bus->dev) {
> > > > > -		pr_err("SoundWire bus has no device\n");
> > > > > -		return -ENODEV;
> > > >
> > > > This check is removed and not moved into sdw_master_device_add()
> > > > either, can you add here or in patch 1 and keep checking the parent
> > > > device please
> > >
> > > We will set bus->dev = &md->dev in the end of sdw_master_device_add().
> > 
> > We need to test if this is valid or not :)
> > 
> > > That's why we remove the test. But now I am wandering does it make
> > > sense to set bus->dev = &md->dev? Maybe it makes more sense to set
> > > bus->dev = sdw control device.
> > > A follow up question is that should slave device a child of bus device
> > > or master device? I would prefer bus device if it makes sense.
> > > I will check bus->dev and parent and remove bus->dev = &md->dev in the
> > > next version.
> > 
> > the parent is bus->dev and sdw_master_device created would be child of this
> > and should be set as such. You can remove it from bus object and keep in
> > sdw_master_device object, that is fine by me.
> 
> Looks like we don't need the parent and fwnode parameter since we can
> get them from bus->dev 😊

Quite right

> > The sdw_slave is child of sdw_master_device now and looks to be set correct.
> 
> So, it will be
> bus device
>     -> master device
>          -> slave device
> right?

yes

> 
> I have a question here. We have pm supported on bus and slave devices,
> but not master device. Will pm work with this architecture?
> Can it be
> bus device
>     -> master device & slave device?

yes it would and you should check it out. The pm (runtime_pm) works well
with child devices and parents, so we need to ensure that parents are
set properly.

Thanks
-- 
~Vinod

Re: [PATCH 3/3] soundwire: bus_type: add sdw_master_device support

[Pierre-Louis Bossart]

>> +	md = &bus->md;
>> +	md->dev.bus = &sdw_bus_type;
>> +	md->dev.type = &sdw_master_type;
>> +	md->dev.parent = parent;
>> +	md->dev.of_node = parent->of_node;
>> +	md->dev.fwnode = fwnode;
>> +	md->dev.dma_mask = parent->dma_mask;
>> +
>> +	dev_set_name(&md->dev, "sdw-master-%d", bus->link_id);
> 
> This give nice sdw-master-0. In DT this comes from reg property. I dont
> seem to recall if the ACPI/Disco spec treats link_id as unique across
> the system, can you check that please, if not we would need to update
> this.
Table 3 in the Disco for Soundwire 1.0 spec: "all LinkID values are 
relative to the immediate parent Device."

There isn't any known implementation with more than one controller.

Re: [PATCH 3/3] soundwire: bus_type: add sdw_master_device support

[Vinod Koul]

On 11-05-20, 14:00, Pierre-Louis Bossart wrote:
> > > +	md = &bus->md;
> > > +	md->dev.bus = &sdw_bus_type;
> > > +	md->dev.type = &sdw_master_type;
> > > +	md->dev.parent = parent;
> > > +	md->dev.of_node = parent->of_node;
> > > +	md->dev.fwnode = fwnode;
> > > +	md->dev.dma_mask = parent->dma_mask;
> > > +
> > > +	dev_set_name(&md->dev, "sdw-master-%d", bus->link_id);
> > 
> > This give nice sdw-master-0. In DT this comes from reg property. I dont
> > seem to recall if the ACPI/Disco spec treats link_id as unique across
> > the system, can you check that please, if not we would need to update
> > this.
> Table 3 in the Disco for Soundwire 1.0 spec: "all LinkID values are relative
> to the immediate parent Device."
> 
> There isn't any known implementation with more than one controller.

But then it can come in "future" right. So lets try to make it future
proof by not using the link_id (we can expose that as a sysfs if people
want to know). So a global unique id needs to allocated (hint: idr or
equivalent) and used as master_id

Thanks
-- 
~Vinod

Re: [PATCH 3/3] soundwire: bus_type: add sdw_master_device support

[Vinod Koul]

On 11-05-20, 14:00, Pierre-Louis Bossart wrote:
> > > +	md = &bus->md;
> > > +	md->dev.bus = &sdw_bus_type;
> > > +	md->dev.type = &sdw_master_type;
> > > +	md->dev.parent = parent;
> > > +	md->dev.of_node = parent->of_node;
> > > +	md->dev.fwnode = fwnode;
> > > +	md->dev.dma_mask = parent->dma_mask;
> > > +
> > > +	dev_set_name(&md->dev, "sdw-master-%d", bus->link_id);
> > 
> > This give nice sdw-master-0. In DT this comes from reg property. I dont
> > seem to recall if the ACPI/Disco spec treats link_id as unique across
> > the system, can you check that please, if not we would need to update
> > this.
> Table 3 in the Disco for Soundwire 1.0 spec: "all LinkID values are relative
> to the immediate parent Device."
> 
> There isn't any known implementation with more than one controller.

But then it can come in "future" right. So lets try to make it future
proof by not using the link_id (we can expose that as a sysfs if people
want to know). So a global unique id needs to allocated (hint: idr or
equivalent) and used as master_id

Thanks
-- 
~Vinod

Re: [PATCH 3/3] soundwire: bus_type: add sdw_master_device support

[Pierre-Louis Bossart]

On 5/11/20 10:30 PM, Vinod Koul wrote:
> On 11-05-20, 14:00, Pierre-Louis Bossart wrote:
>>>> +	md = &bus->md;
>>>> +	md->dev.bus = &sdw_bus_type;
>>>> +	md->dev.type = &sdw_master_type;
>>>> +	md->dev.parent = parent;
>>>> +	md->dev.of_node = parent->of_node;
>>>> +	md->dev.fwnode = fwnode;
>>>> +	md->dev.dma_mask = parent->dma_mask;
>>>> +
>>>> +	dev_set_name(&md->dev, "sdw-master-%d", bus->link_id);
>>>
>>> This give nice sdw-master-0. In DT this comes from reg property. I dont
>>> seem to recall if the ACPI/Disco spec treats link_id as unique across
>>> the system, can you check that please, if not we would need to update
>>> this.
>> Table 3 in the Disco for Soundwire 1.0 spec: "all LinkID values are relative
>> to the immediate parent Device."
>>
>> There isn't any known implementation with more than one controller.
> 
> But then it can come in "future" right. So lets try to make it future
> proof by not using the link_id (we can expose that as a sysfs if people
> want to know). So a global unique id needs to allocated (hint: idr or
> equivalent) and used as master_id

Can you clarify if you are asking for a global ID for Intel/ACPI 
platforms, or for DT as well? I can't figure out from the 
soundwire-controller.yaml definitions if there is already a notion of 
unique ID.

properties:
   $nodename:
     pattern: "^soundwire(@.*)?$"

    soundwire@c2d0000 {
         #address-cells = <2>;
         #size-cells = <0>;
         reg = <0x0c2d0000 0x2000>;

Re: [PATCH 3/3] soundwire: bus_type: add sdw_master_device support

[Vinod Koul]

On 12-05-20, 09:36, Pierre-Louis Bossart wrote:
> On 5/11/20 10:30 PM, Vinod Koul wrote:
> > On 11-05-20, 14:00, Pierre-Louis Bossart wrote:
> > > > > +	md = &bus->md;
> > > > > +	md->dev.bus = &sdw_bus_type;
> > > > > +	md->dev.type = &sdw_master_type;
> > > > > +	md->dev.parent = parent;
> > > > > +	md->dev.of_node = parent->of_node;
> > > > > +	md->dev.fwnode = fwnode;
> > > > > +	md->dev.dma_mask = parent->dma_mask;
> > > > > +
> > > > > +	dev_set_name(&md->dev, "sdw-master-%d", bus->link_id);
> > > > 
> > > > This give nice sdw-master-0. In DT this comes from reg property. I dont
> > > > seem to recall if the ACPI/Disco spec treats link_id as unique across
> > > > the system, can you check that please, if not we would need to update
> > > > this.
> > > Table 3 in the Disco for Soundwire 1.0 spec: "all LinkID values are relative
> > > to the immediate parent Device."
> > > 
> > > There isn't any known implementation with more than one controller.
> > 
> > But then it can come in "future" right. So lets try to make it future
> > proof by not using the link_id (we can expose that as a sysfs if people
> > want to know). So a global unique id needs to allocated (hint: idr or
> > equivalent) and used as master_id
> 
> Can you clarify if you are asking for a global ID for Intel/ACPI platforms,
> or for DT as well? I can't figure out from the soundwire-controller.yaml
> definitions if there is already a notion of unique ID.

If ACPI was unique, then I was planning to update the definition below
to include that. Given that it is not the case, let's make it agnostic to
underlying firmware.

> 
> properties:
>   $nodename:
>     pattern: "^soundwire(@.*)?$"
> 
>    soundwire@c2d0000 {
>         #address-cells = <2>;
>         #size-cells = <0>;
>         reg = <0x0c2d0000 0x2000>;

-- 
~Vinod

Re: [PATCH 3/3] soundwire: bus_type: add sdw_master_device support

[Pierre-Louis Bossart]

On 5/12/20 10:59 AM, Vinod Koul wrote:
> On 12-05-20, 09:36, Pierre-Louis Bossart wrote:
>> On 5/11/20 10:30 PM, Vinod Koul wrote:
>>> On 11-05-20, 14:00, Pierre-Louis Bossart wrote:
>>>>>> +	md = &bus->md;
>>>>>> +	md->dev.bus = &sdw_bus_type;
>>>>>> +	md->dev.type = &sdw_master_type;
>>>>>> +	md->dev.parent = parent;
>>>>>> +	md->dev.of_node = parent->of_node;
>>>>>> +	md->dev.fwnode = fwnode;
>>>>>> +	md->dev.dma_mask = parent->dma_mask;
>>>>>> +
>>>>>> +	dev_set_name(&md->dev, "sdw-master-%d", bus->link_id);
>>>>>
>>>>> This give nice sdw-master-0. In DT this comes from reg property. I dont
>>>>> seem to recall if the ACPI/Disco spec treats link_id as unique across
>>>>> the system, can you check that please, if not we would need to update
>>>>> this.
>>>> Table 3 in the Disco for Soundwire 1.0 spec: "all LinkID values are relative
>>>> to the immediate parent Device."
>>>>
>>>> There isn't any known implementation with more than one controller.
>>>
>>> But then it can come in "future" right. So lets try to make it future
>>> proof by not using the link_id (we can expose that as a sysfs if people
>>> want to know). So a global unique id needs to allocated (hint: idr or
>>> equivalent) and used as master_id
>>
>> Can you clarify if you are asking for a global ID for Intel/ACPI platforms,
>> or for DT as well? I can't figure out from the soundwire-controller.yaml
>> definitions if there is already a notion of unique ID.
> 
> If ACPI was unique, then I was planning to update the definition below
> to include that. Given that it is not the case, let's make it agnostic to
> underlying firmware.

I am not sure I understand how this would be done.

The call sequence is

sdw_bus_master_add(bus)
     sdw_master_device_add(bus, parent, fw_node)

At the bus level, we don't have any information on which controller the 
bus is related to.

We'd need to add an argument to sdw_bus_master_add() and have the 
controller unique ID be allocated outside of the SoundWire core, hence 
my question on whether the DT definition should not be extended.

> 
>>
>> properties:
>>    $nodename:
>>      pattern: "^soundwire(@.*)?$"
>>
>>     soundwire@c2d0000 {
>>          #address-cells = <2>;
>>          #size-cells = <0>;
>>          reg = <0x0c2d0000 0x2000>;
> 

Re: [PATCH 3/3] soundwire: bus_type: add sdw_master_device support

[Pierre-Louis Bossart]

On 5/12/20 11:08 AM, Pierre-Louis Bossart wrote:
> 
> 
> On 5/12/20 10:59 AM, Vinod Koul wrote:
>> On 12-05-20, 09:36, Pierre-Louis Bossart wrote:
>>> On 5/11/20 10:30 PM, Vinod Koul wrote:
>>>> On 11-05-20, 14:00, Pierre-Louis Bossart wrote:
>>>>>>> +    md = &bus->md;
>>>>>>> +    md->dev.bus = &sdw_bus_type;
>>>>>>> +    md->dev.type = &sdw_master_type;
>>>>>>> +    md->dev.parent = parent;
>>>>>>> +    md->dev.of_node = parent->of_node;
>>>>>>> +    md->dev.fwnode = fwnode;
>>>>>>> +    md->dev.dma_mask = parent->dma_mask;
>>>>>>> +
>>>>>>> +    dev_set_name(&md->dev, "sdw-master-%d", bus->link_id);
>>>>>>
>>>>>> This give nice sdw-master-0. In DT this comes from reg property. I 
>>>>>> dont
>>>>>> seem to recall if the ACPI/Disco spec treats link_id as unique across
>>>>>> the system, can you check that please, if not we would need to update
>>>>>> this.
>>>>> Table 3 in the Disco for Soundwire 1.0 spec: "all LinkID values are 
>>>>> relative
>>>>> to the immediate parent Device."
>>>>>
>>>>> There isn't any known implementation with more than one controller.
>>>>
>>>> But then it can come in "future" right. So lets try to make it future
>>>> proof by not using the link_id (we can expose that as a sysfs if people
>>>> want to know). So a global unique id needs to allocated (hint: idr or
>>>> equivalent) and used as master_id
>>>
>>> Can you clarify if you are asking for a global ID for Intel/ACPI 
>>> platforms,
>>> or for DT as well? I can't figure out from the soundwire-controller.yaml
>>> definitions if there is already a notion of unique ID.
>>
>> If ACPI was unique, then I was planning to update the definition below
>> to include that. Given that it is not the case, let's make it agnostic to
>> underlying firmware.
> 
> I am not sure I understand how this would be done.
> 
> The call sequence is
> 
> sdw_bus_master_add(bus)
>      sdw_master_device_add(bus, parent, fw_node)
> 
> At the bus level, we don't have any information on which controller the 
> bus is related to.
> 
> We'd need to add an argument to sdw_bus_master_add() and have the 
> controller unique ID be allocated outside of the SoundWire core, hence 
> my question on whether the DT definition should not be extended.

And btw I don't think it makes sense to add a new definition for Intel. 
We already have a notion of HDaudio bus->idx that's set to zero since we 
don't have a case for multiple HDaudio controllers.

if we ever do have more than once controller, then we should rely on 
HDaudio bus->idx as the identifier and not create one specifically for 
SoundWire - which means as I mentioned above passing an argument and not 
defining a controller ID in the SoundWire core.

Re: [PATCH 3/3] soundwire: bus_type: add sdw_master_device support

[Vinod Koul]

On 12-05-20, 12:01, Pierre-Louis Bossart wrote:
> > > > > > 
> > > > > > There isn't any known implementation with more than one controller.
> > > > > 
> > > > > But then it can come in "future" right. So lets try to make it future
> > > > > proof by not using the link_id (we can expose that as a sysfs if people
> > > > > want to know). So a global unique id needs to allocated (hint: idr or
> > > > > equivalent) and used as master_id
> > > > 
> > > > Can you clarify if you are asking for a global ID for Intel/ACPI
> > > > platforms,
> > > > or for DT as well? I can't figure out from the soundwire-controller.yaml
> > > > definitions if there is already a notion of unique ID.
> > > 
> > > If ACPI was unique, then I was planning to update the definition below
> > > to include that. Given that it is not the case, let's make it agnostic to
> > > underlying firmware.
> > 
> > I am not sure I understand how this would be done.
> > 
> > The call sequence is
> > 
> > sdw_bus_master_add(bus)
> >      sdw_master_device_add(bus, parent, fw_node)
> > 
> > At the bus level, we don't have any information on which controller the
> > bus is related to.

This should be done inside the sdw_bus. controller should not care about
this IMO.

> > We'd need to add an argument to sdw_bus_master_add() and have the
> > controller unique ID be allocated outside of the SoundWire core, hence
> > my question on whether the DT definition should not be extended.
> 
> And btw I don't think it makes sense to add a new definition for Intel. We
> already have a notion of HDaudio bus->idx that's set to zero since we don't
> have a case for multiple HDaudio controllers.
> 
> if we ever do have more than once controller, then we should rely on HDaudio
> bus->idx as the identifier and not create one specifically for SoundWire -
> which means as I mentioned above passing an argument and not defining a
> controller ID in the SoundWire core.

I was thinking of following code in bus.c

static DEFINE_IDA(sdw_ida);

static sdw_get_id(struct sdw_bus *bus)
{
        int rc = ida_alloc(&sdw_ida, GFP_KERNEL);

        if (rc < 0)
                return rc;

        bus->id = rc;
        return 0;
}

int sdw_add_bus_master(struct sdw_bus *bus)
{
        ...

        ret = sdw_get_id(bus);

        ...
}

void sdw_delete_bus_master(struct sdw_bus *bus)
{
        da_free(&sdw_ida, bus->id);
}

This way you get a unique master number across all devices and this has
nothing to do with link/of ids and is used for numbering masters in
sysfs uniquely.

HTH
-- 
~Vinod