Alsa-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [alsa-devel] [PATCH v5 00/17] soundwire: intel: implement new ASoC interfaces
@ 2019-12-17 21:02 Pierre-Louis Bossart
  2019-12-17 21:02 ` [alsa-devel] [PATCH v5 01/17] soundwire: renames to prepare support for master drivers/devices Pierre-Louis Bossart
                   ` (16 more replies)
  0 siblings, 17 replies; 42+ messages in thread
From: Pierre-Louis Bossart @ 2019-12-17 21:02 UTC (permalink / raw)
  To: alsa-devel
  Cc: Pierre-Louis Bossart, tiwai, gregkh, linux-kernel,
	Ranjani Sridharan, vkoul, broonie, srinivas.kandagatla, jank,
	slawomir.blauciak, Bard liao, Rander Wang

This patchset applies on top of soundwire/next, now that the interface
definitions are merged.

The changes are essentially a removal of the platform devices,
replaced by explicit device(s)/driver for the SoundWire Master(s), and
the implementation of the new interfaces required to scan the ACPI
tables, probe the links and start them.

The missing prepare, trigger and setup ASoC callbacks are also
implemented. The hw_params and free callbacks use the new interfaces
as well.

While there are quite a few lines of code changed, this is mostly
about interface changes. The next series will contain more functional
changes and deal with race conditions on probe, enumeration and
suspend/resume issues.

Thanks to Vinod Kould and GregKH for the reviews on previous versions,
much appreciated.

Changes since v4: (feedback from GregKH except last point flagged by
Intel validation)

Clarified error handling with uevents
Added better commit messages and rationale behind exposing a Master Device.
Used 'master_device' instead of 'md' when possible (kept the shortcut
with really long function names)
Added namespace check support for Intel code (the Cadence and core
stuff should be done in a separate patchset).
Fixed GPLv2 license/EXPORT_SYMBOL_GPL error for the Master Device code.
Fixed missing error handling in sdw_md_add
Added kerneldoc comments for sdw_md_driver structure (with explanation
on what 'md' stands for)
Fixed NULL pointer assignment leading to issues with driver_unregister

Changes since v3:
One line change to re-add EXPORT_SYMBOL
Add missing driver_registration 

Changes since v2:
moved uevent handling to slave_type (Vinod)

Changes since v1:
fix typo (Vinod)
removed uevent open for Master (Vinod)
clarified commit messages (Cezary)
no functionality change

Bard Liao (1):
  soundwire: register master device driver

Pierre-Louis Bossart (13):
  soundwire: renames to prepare support for master drivers/devices
  soundwire: rename dev_to_sdw_dev macro
  soundwire: rename drv_to_sdw_slave_driver macro
  soundwire: bus_type: rename sdw_drv_ to sdw_slave_drv
  soundwire: intel: rename res field as link_res
  soundwire: add support for sdw_slave_type
  soundwire: slave: move uevent handling to slave device level
  soundwire: add initial definitions for sdw_master_device
  soundwire: intel: remove platform devices and use 'Master Devices'
    instead
  soundwire: intel: free all resources on hw_free()
  soundwire: intel_init: add implementation of sdw_intel_enable_irq()
  soundwire: intel_init: use EXPORT_SYMBOL_NS
  soundwire: intel: use EXPORT_SYMBOL_NS

Rander Wang (3):
  soundwire: intel: add prepare support in sdw dai driver
  soundwire: intel: add trigger support in sdw dai driver
  soundwire: intel: add sdw_stream_setup helper for .startup callback

 drivers/base/regmap/regmap-sdw.c   |   4 +-
 drivers/soundwire/Makefile         |   2 +-
 drivers/soundwire/bus.c            |   2 +-
 drivers/soundwire/bus.h            |   2 +
 drivers/soundwire/bus_type.c       |  70 ++++---
 drivers/soundwire/intel.c          | 281 +++++++++++++++++++++-----
 drivers/soundwire/intel.h          |   8 +-
 drivers/soundwire/intel_init.c     | 312 ++++++++++++++++++++++-------
 drivers/soundwire/master.c         |  64 ++++++
 drivers/soundwire/slave.c          |  10 +-
 include/linux/soundwire/sdw.h      |  42 +++-
 include/linux/soundwire/sdw_type.h |  34 +++-
 12 files changed, 668 insertions(+), 163 deletions(-)
 create mode 100644 drivers/soundwire/master.c

-- 
2.20.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

* [alsa-devel] [PATCH v5 01/17] soundwire: renames to prepare support for master drivers/devices
  2019-12-17 21:02 [alsa-devel] [PATCH v5 00/17] soundwire: intel: implement new ASoC interfaces Pierre-Louis Bossart
@ 2019-12-17 21:02 ` Pierre-Louis Bossart
  2019-12-17 21:02 ` [alsa-devel] [PATCH v5 02/17] soundwire: rename dev_to_sdw_dev macro Pierre-Louis Bossart
                   ` (15 subsequent siblings)
  16 siblings, 0 replies; 42+ messages in thread
From: Pierre-Louis Bossart @ 2019-12-17 21:02 UTC (permalink / raw)
  To: alsa-devel
  Cc: Pierre-Louis Bossart, tiwai, gregkh, linux-kernel,
	Ranjani Sridharan, vkoul, broonie, srinivas.kandagatla, jank,
	slawomir.blauciak, Sanyog Kale, Bard liao, Rander Wang

Add clearer references to sdw_slave_driver for internal macros

No change for sdw_driver and module_sdw_driver to avoid compatibility
issues with existing codec devices

No functionality change.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/bus_type.c       | 21 +++++++++++----------
 include/linux/soundwire/sdw_type.h | 18 ++++++++++--------
 2 files changed, 21 insertions(+), 18 deletions(-)

diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c
index 4a465f55039f..370b94752662 100644
--- a/drivers/soundwire/bus_type.c
+++ b/drivers/soundwire/bus_type.c
@@ -34,7 +34,7 @@ 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_driver *drv = drv_to_sdw_slave_driver(ddrv);
 
 	return !!sdw_get_device_id(slave, drv);
 }
@@ -70,7 +70,7 @@ EXPORT_SYMBOL_GPL(sdw_bus_type);
 static int sdw_drv_probe(struct device *dev)
 {
 	struct sdw_slave *slave = dev_to_sdw_dev(dev);
-	struct sdw_driver *drv = drv_to_sdw_driver(dev->driver);
+	struct sdw_driver *drv = drv_to_sdw_slave_driver(dev->driver);
 	const struct sdw_device_id *id;
 	int ret;
 
@@ -116,7 +116,7 @@ static int sdw_drv_probe(struct device *dev)
 static int sdw_drv_remove(struct device *dev)
 {
 	struct sdw_slave *slave = dev_to_sdw_dev(dev);
-	struct sdw_driver *drv = drv_to_sdw_driver(dev->driver);
+	struct sdw_driver *drv = drv_to_sdw_slave_driver(dev->driver);
 	int ret = 0;
 
 	if (drv->remove)
@@ -130,20 +130,21 @@ static int sdw_drv_remove(struct device *dev)
 static void sdw_drv_shutdown(struct device *dev)
 {
 	struct sdw_slave *slave = dev_to_sdw_dev(dev);
-	struct sdw_driver *drv = drv_to_sdw_driver(dev->driver);
+	struct sdw_driver *drv = drv_to_sdw_slave_driver(dev->driver);
 
 	if (drv->shutdown)
 		drv->shutdown(slave);
 }
 
 /**
- * __sdw_register_driver() - register a SoundWire Slave driver
+ * __sdw_register_slave_driver() - register a SoundWire Slave driver
  * @drv: driver to register
  * @owner: owning module/driver
  *
  * Return: zero on success, else a negative error code.
  */
-int __sdw_register_driver(struct sdw_driver *drv, struct module *owner)
+int __sdw_register_slave_driver(struct sdw_driver *drv,
+				struct module *owner)
 {
 	drv->driver.bus = &sdw_bus_type;
 
@@ -164,17 +165,17 @@ int __sdw_register_driver(struct sdw_driver *drv, struct module *owner)
 
 	return driver_register(&drv->driver);
 }
-EXPORT_SYMBOL_GPL(__sdw_register_driver);
+EXPORT_SYMBOL_GPL(__sdw_register_slave_driver);
 
 /**
- * sdw_unregister_driver() - unregisters the SoundWire Slave driver
+ * sdw_unregister_slave_driver() - unregisters the SoundWire Slave driver
  * @drv: driver to unregister
  */
-void sdw_unregister_driver(struct sdw_driver *drv)
+void sdw_unregister_slave_driver(struct sdw_driver *drv)
 {
 	driver_unregister(&drv->driver);
 }
-EXPORT_SYMBOL_GPL(sdw_unregister_driver);
+EXPORT_SYMBOL_GPL(sdw_unregister_slave_driver);
 
 static int __init sdw_bus_init(void)
 {
diff --git a/include/linux/soundwire/sdw_type.h b/include/linux/soundwire/sdw_type.h
index aaa7f4267c14..abaa21278152 100644
--- a/include/linux/soundwire/sdw_type.h
+++ b/include/linux/soundwire/sdw_type.h
@@ -6,13 +6,15 @@
 
 extern struct bus_type sdw_bus_type;
 
-#define drv_to_sdw_driver(_drv) container_of(_drv, struct sdw_driver, driver)
+#define drv_to_sdw_slave_driver(_drv) \
+	container_of(_drv, struct sdw_driver, driver)
 
-#define sdw_register_driver(drv) \
-	__sdw_register_driver(drv, THIS_MODULE)
+#define sdw_register_slave_driver(drv) \
+	__sdw_register_slave_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_register_slave_driver(struct sdw_driver *drv,
+				struct module *owner);
+void sdw_unregister_slave_driver(struct sdw_driver *drv);
 
 int sdw_slave_modalias(const struct sdw_slave *slave, char *buf, size_t size);
 
@@ -24,7 +26,7 @@ int sdw_slave_modalias(const struct sdw_slave *slave, char *buf, size_t size);
  * 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_driver(__sdw_driver) \
-	module_driver(__sdw_driver, sdw_register_driver, \
-			sdw_unregister_driver)
+#define module_sdw_driver(__sdw_slave_driver) \
+	module_driver(__sdw_slave_driver, sdw_register_slave_driver, \
+			sdw_unregister_slave_driver)
 #endif /* __SOUNDWIRE_TYPES_H */
-- 
2.20.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

* [alsa-devel] [PATCH v5 02/17] soundwire: rename dev_to_sdw_dev macro
  2019-12-17 21:02 [alsa-devel] [PATCH v5 00/17] soundwire: intel: implement new ASoC interfaces Pierre-Louis Bossart
  2019-12-17 21:02 ` [alsa-devel] [PATCH v5 01/17] soundwire: renames to prepare support for master drivers/devices Pierre-Louis Bossart
@ 2019-12-17 21:02 ` Pierre-Louis Bossart
  2019-12-27  6:54   ` Vinod Koul
  2019-12-17 21:03 ` [alsa-devel] [PATCH v5 03/17] soundwire: rename drv_to_sdw_slave_driver macro Pierre-Louis Bossart
                   ` (14 subsequent siblings)
  16 siblings, 1 reply; 42+ messages in thread
From: Pierre-Louis Bossart @ 2019-12-17 21:02 UTC (permalink / raw)
  To: alsa-devel
  Cc: Pierre-Louis Bossart, Rafael J. Wysocki, tiwai, gregkh,
	linux-kernel, Ranjani Sridharan, vkoul, broonie,
	srinivas.kandagatla, jank, slawomir.blauciak, Sanyog Kale,
	Bard liao, Rander Wang

Since we want to introduce master devices, rename macro so that we
have consistency between slave and master device access.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/base/regmap/regmap-sdw.c |  4 ++--
 drivers/soundwire/bus.c          |  2 +-
 drivers/soundwire/bus_type.c     | 11 ++++++-----
 drivers/soundwire/slave.c        |  2 +-
 include/linux/soundwire/sdw.h    |  3 ++-
 5 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/base/regmap/regmap-sdw.c b/drivers/base/regmap/regmap-sdw.c
index 50a66382d87d..d1fc0c22180a 100644
--- a/drivers/base/regmap/regmap-sdw.c
+++ b/drivers/base/regmap/regmap-sdw.c
@@ -10,7 +10,7 @@
 static int regmap_sdw_write(void *context, unsigned int reg, unsigned int val)
 {
 	struct device *dev = context;
-	struct sdw_slave *slave = dev_to_sdw_dev(dev);
+	struct sdw_slave *slave = to_sdw_slave_device(dev);
 
 	return sdw_write(slave, reg, val);
 }
@@ -18,7 +18,7 @@ static int regmap_sdw_write(void *context, unsigned int reg, unsigned int val)
 static int regmap_sdw_read(void *context, unsigned int reg, unsigned int *val)
 {
 	struct device *dev = context;
-	struct sdw_slave *slave = dev_to_sdw_dev(dev);
+	struct sdw_slave *slave = to_sdw_slave_device(dev);
 	int read;
 
 	read = sdw_read(slave, reg);
diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index be5d437058ed..4b22ee996a65 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -110,7 +110,7 @@ EXPORT_SYMBOL(sdw_add_bus_master);
 
 static int sdw_delete_slave(struct device *dev, void *data)
 {
-	struct sdw_slave *slave = dev_to_sdw_dev(dev);
+	struct sdw_slave *slave = to_sdw_slave_device(dev);
 	struct sdw_bus *bus = slave->bus;
 
 	sdw_slave_debugfs_exit(slave);
diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c
index 370b94752662..c0585bcc8a41 100644
--- a/drivers/soundwire/bus_type.c
+++ b/drivers/soundwire/bus_type.c
@@ -33,7 +33,7 @@ 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_slave *slave = to_sdw_slave_device(dev);
 	struct sdw_driver *drv = drv_to_sdw_slave_driver(ddrv);
 
 	return !!sdw_get_device_id(slave, drv);
@@ -49,7 +49,7 @@ int sdw_slave_modalias(const struct sdw_slave *slave, char *buf, size_t size)
 
 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 = to_sdw_slave_device(dev);
 	char modalias[32];
 
 	sdw_slave_modalias(slave, modalias, sizeof(modalias));
@@ -69,7 +69,7 @@ EXPORT_SYMBOL_GPL(sdw_bus_type);
 
 static int sdw_drv_probe(struct device *dev)
 {
-	struct sdw_slave *slave = dev_to_sdw_dev(dev);
+	struct sdw_slave *slave = to_sdw_slave_device(dev);
 	struct sdw_driver *drv = drv_to_sdw_slave_driver(dev->driver);
 	const struct sdw_device_id *id;
 	int ret;
@@ -115,8 +115,9 @@ static int sdw_drv_probe(struct device *dev)
 
 static int sdw_drv_remove(struct device *dev)
 {
-	struct sdw_slave *slave = dev_to_sdw_dev(dev);
+	struct sdw_slave *slave = to_sdw_slave_device(dev);
 	struct sdw_driver *drv = drv_to_sdw_slave_driver(dev->driver);
+
 	int ret = 0;
 
 	if (drv->remove)
@@ -129,7 +130,7 @@ static int sdw_drv_remove(struct device *dev)
 
 static void sdw_drv_shutdown(struct device *dev)
 {
-	struct sdw_slave *slave = dev_to_sdw_dev(dev);
+	struct sdw_slave *slave = to_sdw_slave_device(dev);
 	struct sdw_driver *drv = drv_to_sdw_slave_driver(dev->driver);
 
 	if (drv->shutdown)
diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c
index 19919975bb6d..48a513680db6 100644
--- a/drivers/soundwire/slave.c
+++ b/drivers/soundwire/slave.c
@@ -9,7 +9,7 @@
 
 static void sdw_slave_release(struct device *dev)
 {
-	struct sdw_slave *slave = dev_to_sdw_dev(dev);
+	struct sdw_slave *slave = to_sdw_slave_device(dev);
 
 	kfree(slave);
 }
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index b7c9eca4332a..5b1180f1e6b5 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -582,7 +582,8 @@ struct sdw_slave {
 	u32 unattach_request;
 };
 
-#define dev_to_sdw_dev(_dev) container_of(_dev, struct sdw_slave, dev)
+#define to_sdw_slave_device(d) \
+	container_of(d, struct sdw_slave, dev)
 
 struct sdw_driver {
 	const char *name;
-- 
2.20.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

* [alsa-devel] [PATCH v5 03/17] soundwire: rename drv_to_sdw_slave_driver macro
  2019-12-17 21:02 [alsa-devel] [PATCH v5 00/17] soundwire: intel: implement new ASoC interfaces Pierre-Louis Bossart
  2019-12-17 21:02 ` [alsa-devel] [PATCH v5 01/17] soundwire: renames to prepare support for master drivers/devices Pierre-Louis Bossart
  2019-12-17 21:02 ` [alsa-devel] [PATCH v5 02/17] soundwire: rename dev_to_sdw_dev macro Pierre-Louis Bossart
@ 2019-12-17 21:03 ` Pierre-Louis Bossart
  2019-12-27  7:00   ` Vinod Koul
  2019-12-17 21:03 ` [alsa-devel] [PATCH v5 04/17] soundwire: bus_type: rename sdw_drv_ to sdw_slave_drv Pierre-Louis Bossart
                   ` (13 subsequent siblings)
  16 siblings, 1 reply; 42+ messages in thread
From: Pierre-Louis Bossart @ 2019-12-17 21:03 UTC (permalink / raw)
  To: alsa-devel
  Cc: Pierre-Louis Bossart, tiwai, gregkh, linux-kernel,
	Ranjani Sridharan, vkoul, broonie, srinivas.kandagatla, jank,
	slawomir.blauciak, Sanyog Kale, Bard liao, Rander Wang

Align with previous renames and shorten macro

No functionality change

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/bus_type.c       | 9 ++++-----
 include/linux/soundwire/sdw_type.h | 3 ++-
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c
index c0585bcc8a41..2b2830b622fa 100644
--- a/drivers/soundwire/bus_type.c
+++ b/drivers/soundwire/bus_type.c
@@ -34,7 +34,7 @@ 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 = to_sdw_slave_device(dev);
-	struct sdw_driver *drv = drv_to_sdw_slave_driver(ddrv);
+	struct sdw_driver *drv = to_sdw_slave_driver(ddrv);
 
 	return !!sdw_get_device_id(slave, drv);
 }
@@ -70,7 +70,7 @@ EXPORT_SYMBOL_GPL(sdw_bus_type);
 static int sdw_drv_probe(struct device *dev)
 {
 	struct sdw_slave *slave = to_sdw_slave_device(dev);
-	struct sdw_driver *drv = drv_to_sdw_slave_driver(dev->driver);
+	struct sdw_driver *drv = to_sdw_slave_driver(dev->driver);
 	const struct sdw_device_id *id;
 	int ret;
 
@@ -116,8 +116,7 @@ static int sdw_drv_probe(struct device *dev)
 static int sdw_drv_remove(struct device *dev)
 {
 	struct sdw_slave *slave = to_sdw_slave_device(dev);
-	struct sdw_driver *drv = drv_to_sdw_slave_driver(dev->driver);
-
+	struct sdw_driver *drv = to_sdw_slave_driver(dev->driver);
 	int ret = 0;
 
 	if (drv->remove)
@@ -131,7 +130,7 @@ static int sdw_drv_remove(struct device *dev)
 static void sdw_drv_shutdown(struct device *dev)
 {
 	struct sdw_slave *slave = to_sdw_slave_device(dev);
-	struct sdw_driver *drv = drv_to_sdw_slave_driver(dev->driver);
+	struct sdw_driver *drv = to_sdw_slave_driver(dev->driver);
 
 	if (drv->shutdown)
 		drv->shutdown(slave);
diff --git a/include/linux/soundwire/sdw_type.h b/include/linux/soundwire/sdw_type.h
index abaa21278152..7d4bc6a979bf 100644
--- a/include/linux/soundwire/sdw_type.h
+++ b/include/linux/soundwire/sdw_type.h
@@ -6,7 +6,7 @@
 
 extern struct bus_type sdw_bus_type;
 
-#define drv_to_sdw_slave_driver(_drv) \
+#define to_sdw_slave_driver(_drv) \
 	container_of(_drv, struct sdw_driver, driver)
 
 #define sdw_register_slave_driver(drv) \
@@ -29,4 +29,5 @@ int sdw_slave_modalias(const struct sdw_slave *slave, char *buf, size_t size);
 #define module_sdw_driver(__sdw_slave_driver) \
 	module_driver(__sdw_slave_driver, sdw_register_slave_driver, \
 			sdw_unregister_slave_driver)
+
 #endif /* __SOUNDWIRE_TYPES_H */
-- 
2.20.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

* [alsa-devel] [PATCH v5 04/17] soundwire: bus_type: rename sdw_drv_ to sdw_slave_drv
  2019-12-17 21:02 [alsa-devel] [PATCH v5 00/17] soundwire: intel: implement new ASoC interfaces Pierre-Louis Bossart
                   ` (2 preceding siblings ...)
  2019-12-17 21:03 ` [alsa-devel] [PATCH v5 03/17] soundwire: rename drv_to_sdw_slave_driver macro Pierre-Louis Bossart
@ 2019-12-17 21:03 ` Pierre-Louis Bossart
  2019-12-17 21:03 ` [alsa-devel] [PATCH v5 05/17] soundwire: intel: rename res field as link_res Pierre-Louis Bossart
                   ` (12 subsequent siblings)
  16 siblings, 0 replies; 42+ messages in thread
From: Pierre-Louis Bossart @ 2019-12-17 21:03 UTC (permalink / raw)
  To: alsa-devel
  Cc: Pierre-Louis Bossart, tiwai, gregkh, linux-kernel,
	Ranjani Sridharan, vkoul, broonie, srinivas.kandagatla, jank,
	slawomir.blauciak, Sanyog Kale, Bard liao, Rander Wang

Before we add master driver support, make sure there is no ambiguity
and no occurrences of sdw_drv_ functions.

No functionality change.

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

diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c
index 2b2830b622fa..9a0fd3ee1014 100644
--- a/drivers/soundwire/bus_type.c
+++ b/drivers/soundwire/bus_type.c
@@ -67,7 +67,7 @@ struct bus_type sdw_bus_type = {
 };
 EXPORT_SYMBOL_GPL(sdw_bus_type);
 
-static int sdw_drv_probe(struct device *dev)
+static int sdw_slave_drv_probe(struct device *dev)
 {
 	struct sdw_slave *slave = to_sdw_slave_device(dev);
 	struct sdw_driver *drv = to_sdw_slave_driver(dev->driver);
@@ -113,7 +113,7 @@ static int sdw_drv_probe(struct device *dev)
 	return 0;
 }
 
-static int sdw_drv_remove(struct device *dev)
+static int sdw_slave_drv_remove(struct device *dev)
 {
 	struct sdw_slave *slave = to_sdw_slave_device(dev);
 	struct sdw_driver *drv = to_sdw_slave_driver(dev->driver);
@@ -127,7 +127,7 @@ static int sdw_drv_remove(struct device *dev)
 	return ret;
 }
 
-static void sdw_drv_shutdown(struct device *dev)
+static void sdw_slave_drv_shutdown(struct device *dev)
 {
 	struct sdw_slave *slave = to_sdw_slave_device(dev);
 	struct sdw_driver *drv = to_sdw_slave_driver(dev->driver);
@@ -155,13 +155,13 @@ int __sdw_register_slave_driver(struct sdw_driver *drv,
 	}
 
 	drv->driver.owner = owner;
-	drv->driver.probe = sdw_drv_probe;
+	drv->driver.probe = sdw_slave_drv_probe;
 
 	if (drv->remove)
-		drv->driver.remove = sdw_drv_remove;
+		drv->driver.remove = sdw_slave_drv_remove;
 
 	if (drv->shutdown)
-		drv->driver.shutdown = sdw_drv_shutdown;
+		drv->driver.shutdown = sdw_slave_drv_shutdown;
 
 	return driver_register(&drv->driver);
 }
-- 
2.20.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

* [alsa-devel] [PATCH v5 05/17] soundwire: intel: rename res field as link_res
  2019-12-17 21:02 [alsa-devel] [PATCH v5 00/17] soundwire: intel: implement new ASoC interfaces Pierre-Louis Bossart
                   ` (3 preceding siblings ...)
  2019-12-17 21:03 ` [alsa-devel] [PATCH v5 04/17] soundwire: bus_type: rename sdw_drv_ to sdw_slave_drv Pierre-Louis Bossart
@ 2019-12-17 21:03 ` Pierre-Louis Bossart
  2019-12-17 21:03 ` [alsa-devel] [PATCH v5 06/17] soundwire: add support for sdw_slave_type Pierre-Louis Bossart
                   ` (11 subsequent siblings)
  16 siblings, 0 replies; 42+ messages in thread
From: Pierre-Louis Bossart @ 2019-12-17 21:03 UTC (permalink / raw)
  To: alsa-devel
  Cc: Pierre-Louis Bossart, tiwai, gregkh, linux-kernel,
	Ranjani Sridharan, vkoul, broonie, srinivas.kandagatla, jank,
	slawomir.blauciak, Sanyog Kale, Bard liao, Rander Wang

There are too many fields called 'res' so add prefix to make it easier
to track what the structures are.

Pure rename, no functionality change

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

diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index 0371d3d5501a..64f97bb1a135 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -103,7 +103,7 @@ enum intel_pdi_type {
 struct sdw_intel {
 	struct sdw_cdns cdns;
 	int instance;
-	struct sdw_intel_link_res *res;
+	struct sdw_intel_link_res *link_res;
 #ifdef CONFIG_DEBUG_FS
 	struct dentry *debugfs;
 #endif
@@ -193,8 +193,8 @@ static ssize_t intel_sprintf(void __iomem *mem, bool l,
 static int intel_reg_show(struct seq_file *s_file, void *data)
 {
 	struct sdw_intel *sdw = s_file->private;
-	void __iomem *s = sdw->res->shim;
-	void __iomem *a = sdw->res->alh;
+	void __iomem *s = sdw->link_res->shim;
+	void __iomem *a = sdw->link_res->alh;
 	char *buf;
 	ssize_t ret;
 	int i, j;
@@ -289,7 +289,7 @@ static void intel_debugfs_exit(struct sdw_intel *sdw) {}
 static int intel_link_power_up(struct sdw_intel *sdw)
 {
 	unsigned int link_id = sdw->instance;
-	void __iomem *shim = sdw->res->shim;
+	void __iomem *shim = sdw->link_res->shim;
 	int spa_mask, cpa_mask;
 	int link_control, ret;
 
@@ -309,7 +309,7 @@ static int intel_link_power_up(struct sdw_intel *sdw)
 
 static int intel_shim_init(struct sdw_intel *sdw)
 {
-	void __iomem *shim = sdw->res->shim;
+	void __iomem *shim = sdw->link_res->shim;
 	unsigned int link_id = sdw->instance;
 	int sync_reg, ret;
 	u16 ioctl = 0, act = 0;
@@ -370,7 +370,7 @@ static int intel_shim_init(struct sdw_intel *sdw)
 static void intel_pdi_init(struct sdw_intel *sdw,
 			   struct sdw_cdns_stream_config *config)
 {
-	void __iomem *shim = sdw->res->shim;
+	void __iomem *shim = sdw->link_res->shim;
 	unsigned int link_id = sdw->instance;
 	int pcm_cap, pdm_cap;
 
@@ -404,7 +404,7 @@ static void intel_pdi_init(struct sdw_intel *sdw,
 static int
 intel_pdi_get_ch_cap(struct sdw_intel *sdw, unsigned int pdi_num, bool pcm)
 {
-	void __iomem *shim = sdw->res->shim;
+	void __iomem *shim = sdw->link_res->shim;
 	unsigned int link_id = sdw->instance;
 	int count;
 
@@ -476,7 +476,7 @@ static int intel_pdi_ch_update(struct sdw_intel *sdw)
 static void
 intel_pdi_shim_configure(struct sdw_intel *sdw, struct sdw_cdns_pdi *pdi)
 {
-	void __iomem *shim = sdw->res->shim;
+	void __iomem *shim = sdw->link_res->shim;
 	unsigned int link_id = sdw->instance;
 	int pdi_conf = 0;
 
@@ -508,7 +508,7 @@ intel_pdi_shim_configure(struct sdw_intel *sdw, struct sdw_cdns_pdi *pdi)
 static void
 intel_pdi_alh_configure(struct sdw_intel *sdw, struct sdw_cdns_pdi *pdi)
 {
-	void __iomem *alh = sdw->res->alh;
+	void __iomem *alh = sdw->link_res->alh;
 	unsigned int link_id = sdw->instance;
 	unsigned int conf;
 
@@ -535,7 +535,7 @@ static int intel_params_stream(struct sdw_intel *sdw,
 			       struct snd_pcm_hw_params *hw_params,
 			       int link_id, int alh_stream_id)
 {
-	struct sdw_intel_link_res *res = sdw->res;
+	struct sdw_intel_link_res *res = sdw->link_res;
 	struct sdw_intel_stream_params_data params_data;
 
 	params_data.substream = substream;
@@ -558,7 +558,7 @@ static int intel_pre_bank_switch(struct sdw_bus *bus)
 {
 	struct sdw_cdns *cdns = bus_to_cdns(bus);
 	struct sdw_intel *sdw = cdns_to_intel(cdns);
-	void __iomem *shim = sdw->res->shim;
+	void __iomem *shim = sdw->link_res->shim;
 	int sync_reg;
 
 	/* Write to register only for multi-link */
@@ -577,7 +577,7 @@ static int intel_post_bank_switch(struct sdw_bus *bus)
 {
 	struct sdw_cdns *cdns = bus_to_cdns(bus);
 	struct sdw_intel *sdw = cdns_to_intel(cdns);
-	void __iomem *shim = sdw->res->shim;
+	void __iomem *shim = sdw->link_res->shim;
 	int sync_reg, ret;
 
 	/* Write to register only for multi-link */
@@ -934,9 +934,9 @@ static int intel_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	sdw->instance = pdev->id;
-	sdw->res = dev_get_platdata(&pdev->dev);
+	sdw->link_res = dev_get_platdata(&pdev->dev);
 	sdw->cdns.dev = &pdev->dev;
-	sdw->cdns.registers = sdw->res->registers;
+	sdw->cdns.registers = sdw->link_res->registers;
 	sdw->cdns.instance = sdw->instance;
 	sdw->cdns.msg_count = 0;
 	sdw->cdns.bus.dev = &pdev->dev;
@@ -976,11 +976,12 @@ static int intel_probe(struct platform_device *pdev)
 	intel_pdi_ch_update(sdw);
 
 	/* Acquire IRQ */
-	ret = request_threaded_irq(sdw->res->irq, sdw_cdns_irq, sdw_cdns_thread,
+	ret = request_threaded_irq(sdw->link_res->irq,
+				   sdw_cdns_irq, sdw_cdns_thread,
 				   IRQF_SHARED, KBUILD_MODNAME, &sdw->cdns);
 	if (ret < 0) {
 		dev_err(sdw->cdns.dev, "unable to grab IRQ %d, disabling device\n",
-			sdw->res->irq);
+			sdw->link_res->irq);
 		goto err_init;
 	}
 
@@ -1010,7 +1011,7 @@ static int intel_probe(struct platform_device *pdev)
 
 err_interrupt:
 	sdw_cdns_enable_interrupt(&sdw->cdns, false);
-	free_irq(sdw->res->irq, sdw);
+	free_irq(sdw->link_res->irq, sdw);
 err_init:
 	sdw_delete_bus_master(&sdw->cdns.bus);
 	return ret;
@@ -1025,7 +1026,7 @@ static int intel_remove(struct platform_device *pdev)
 	if (!sdw->cdns.bus.prop.hw_disabled) {
 		intel_debugfs_exit(sdw);
 		sdw_cdns_enable_interrupt(&sdw->cdns, false);
-		free_irq(sdw->res->irq, sdw);
+		free_irq(sdw->link_res->irq, sdw);
 		snd_soc_unregister_component(sdw->cdns.dev);
 	}
 	sdw_delete_bus_master(&sdw->cdns.bus);
-- 
2.20.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

* [alsa-devel] [PATCH v5 06/17] soundwire: add support for sdw_slave_type
  2019-12-17 21:02 [alsa-devel] [PATCH v5 00/17] soundwire: intel: implement new ASoC interfaces Pierre-Louis Bossart
                   ` (4 preceding siblings ...)
  2019-12-17 21:03 ` [alsa-devel] [PATCH v5 05/17] soundwire: intel: rename res field as link_res Pierre-Louis Bossart
@ 2019-12-17 21:03 ` Pierre-Louis Bossart
  2019-12-27  7:03   ` Vinod Koul
  2019-12-17 21:03 ` [alsa-devel] [PATCH v5 07/17] soundwire: slave: move uevent handling to slave device level Pierre-Louis Bossart
                   ` (10 subsequent siblings)
  16 siblings, 1 reply; 42+ messages in thread
From: Pierre-Louis Bossart @ 2019-12-17 21:03 UTC (permalink / raw)
  To: alsa-devel
  Cc: Pierre-Louis Bossart, tiwai, gregkh, linux-kernel,
	Ranjani Sridharan, vkoul, broonie, srinivas.kandagatla, jank,
	slawomir.blauciak, Sanyog Kale, Bard liao, Rander Wang

In the existing SoundWire code, the bus does not have any explicit
representation for Master Devices - only SoundWire Slaves are exposed.

In SoundWire, the 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 first convert the existing code to use an explicit
sdw_slave_type and add error checks if this type is not set.

In follow-up patches we can add support for the sdw_master_type.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/bus_type.c       | 23 ++++++++++++++++++-----
 drivers/soundwire/slave.c          |  7 ++++++-
 include/linux/soundwire/sdw_type.h |  6 ++++++
 3 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c
index 9a0fd3ee1014..9719680a1e48 100644
--- a/drivers/soundwire/bus_type.c
+++ b/drivers/soundwire/bus_type.c
@@ -49,13 +49,26 @@ int sdw_slave_modalias(const struct sdw_slave *slave, char *buf, size_t size)
 
 static int sdw_uevent(struct device *dev, struct kobj_uevent_env *env)
 {
-	struct sdw_slave *slave = to_sdw_slave_device(dev);
+	struct sdw_slave *slave;
 	char modalias[32];
 
-	sdw_slave_modalias(slave, modalias, sizeof(modalias));
-
-	if (add_uevent_var(env, "MODALIAS=%s", modalias))
-		return -ENOMEM;
+	if (is_sdw_slave(dev)) {
+		slave = to_sdw_slave_device(dev);
+
+		sdw_slave_modalias(slave, modalias, sizeof(modalias));
+
+		if (add_uevent_var(env, "MODALIAS=%s", modalias))
+			return -ENOMEM;
+	} else {
+		/*
+		 * We only need to handle uevents for the Slave device
+		 * type. This error cannot happen unless the .uevent
+		 * callback is set to use this function for a
+		 * different device type (e.g. Master or Monitor)
+		 */
+		dev_err(dev, "uevent for unknown Soundwire type\n");
+		return -EINVAL;
+	}
 
 	return 0;
 }
diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c
index 48a513680db6..c87267f12a3b 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;
 	slave->dev_num = 0;
diff --git a/include/linux/soundwire/sdw_type.h b/include/linux/soundwire/sdw_type.h
index 7d4bc6a979bf..c681b3426478 100644
--- a/include/linux/soundwire/sdw_type.h
+++ b/include/linux/soundwire/sdw_type.h
@@ -5,6 +5,12 @@
 #define __SOUNDWIRE_TYPES_H
 
 extern struct bus_type sdw_bus_type;
+extern struct device_type sdw_slave_type;
+
+static inline int is_sdw_slave(const struct device *dev)
+{
+	return dev->type == &sdw_slave_type;
+}
 
 #define to_sdw_slave_driver(_drv) \
 	container_of(_drv, struct sdw_driver, driver)
-- 
2.20.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

* [alsa-devel] [PATCH v5 07/17] soundwire: slave: move uevent handling to slave device level
  2019-12-17 21:02 [alsa-devel] [PATCH v5 00/17] soundwire: intel: implement new ASoC interfaces Pierre-Louis Bossart
                   ` (5 preceding siblings ...)
  2019-12-17 21:03 ` [alsa-devel] [PATCH v5 06/17] soundwire: add support for sdw_slave_type Pierre-Louis Bossart
@ 2019-12-17 21:03 ` Pierre-Louis Bossart
  2019-12-17 21:03 ` [alsa-devel] [PATCH v5 08/17] soundwire: add initial definitions for sdw_master_device Pierre-Louis Bossart
                   ` (9 subsequent siblings)
  16 siblings, 0 replies; 42+ messages in thread
From: Pierre-Louis Bossart @ 2019-12-17 21:03 UTC (permalink / raw)
  To: alsa-devel
  Cc: Pierre-Louis Bossart, tiwai, gregkh, linux-kernel,
	Ranjani Sridharan, vkoul, broonie, srinivas.kandagatla, jank,
	slawomir.blauciak, Sanyog Kale, Bard liao, Rander Wang

Currently the .uevent callback is set at the bus level. This is not
necessary, we only really need to deal with uevents for the Slave
device, so move the uevent handling at that level.

Suggested-by: Vinod Koul <vkoul@kernel.org>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/bus.h      | 2 ++
 drivers/soundwire/bus_type.c | 3 +--
 drivers/soundwire/slave.c    | 1 +
 3 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h
index cb482da914da..be01a5f3d00b 100644
--- a/drivers/soundwire/bus.h
+++ b/drivers/soundwire/bus.h
@@ -6,6 +6,8 @@
 
 #define DEFAULT_BANK_SWITCH_TIMEOUT 3000
 
+int sdw_uevent(struct device *dev, struct kobj_uevent_env *env);
+
 #if IS_ENABLED(CONFIG_ACPI)
 int sdw_acpi_find_slaves(struct sdw_bus *bus);
 #else
diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c
index 9719680a1e48..605bc7ae57a8 100644
--- a/drivers/soundwire/bus_type.c
+++ b/drivers/soundwire/bus_type.c
@@ -47,7 +47,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_uevent(struct device *dev, struct kobj_uevent_env *env)
 {
 	struct sdw_slave *slave;
 	char modalias[32];
@@ -76,7 +76,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 c87267f12a3b..014c3ece1f17 100644
--- a/drivers/soundwire/slave.c
+++ b/drivers/soundwire/slave.c
@@ -17,6 +17,7 @@ static void sdw_slave_release(struct device *dev)
 struct device_type sdw_slave_type = {
 	.name =		"sdw_slave",
 	.release =	sdw_slave_release,
+	.uevent = sdw_uevent,
 };
 
 static int sdw_slave_add(struct sdw_bus *bus,
-- 
2.20.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

* [alsa-devel] [PATCH v5 08/17] soundwire: add initial definitions for sdw_master_device
  2019-12-17 21:02 [alsa-devel] [PATCH v5 00/17] soundwire: intel: implement new ASoC interfaces Pierre-Louis Bossart
                   ` (6 preceding siblings ...)
  2019-12-17 21:03 ` [alsa-devel] [PATCH v5 07/17] soundwire: slave: move uevent handling to slave device level Pierre-Louis Bossart
@ 2019-12-17 21:03 ` Pierre-Louis Bossart
  2019-12-27  7:14   ` Vinod Koul
  2019-12-17 21:03 ` [alsa-devel] [PATCH v5 09/17] soundwire: intel: remove platform devices and use 'Master Devices' instead Pierre-Louis Bossart
                   ` (8 subsequent siblings)
  16 siblings, 1 reply; 42+ messages in thread
From: Pierre-Louis Bossart @ 2019-12-17 21:03 UTC (permalink / raw)
  To: alsa-devel
  Cc: Pierre-Louis Bossart, tiwai, gregkh, linux-kernel,
	Ranjani Sridharan, vkoul, broonie, srinivas.kandagatla, jank,
	slawomir.blauciak, Sanyog Kale, Bard liao, Rander Wang

Since we want an explicit support for the SoundWire Master device, add
the definitions, following the Greybus example of a 'Host Device'.

A parent (such as the Intel audio controller) would use sdw_md_add()
to create the device, passing a driver as a parameter. The
sdw_md_release() would be called when put_device() is invoked by the
parent. We use the shortcut 'md' for 'master device' to avoid very
long function names.

The 'Master Device' driver exposes interfaces for
probe/startup/shutdown/remove and a placeholder for autonomous clock
stop support (when the bus control is handed over to a lower-power
solution, typically in D0ix modes).

Module namespace support is added in a separate patch.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/Makefile         |  2 +-
 drivers/soundwire/bus_type.c       |  5 ++-
 drivers/soundwire/master.c         | 63 ++++++++++++++++++++++++++++++
 include/linux/soundwire/sdw.h      | 38 ++++++++++++++++++
 include/linux/soundwire/sdw_type.h |  9 +++++
 5 files changed, 115 insertions(+), 2 deletions(-)
 create mode 100644 drivers/soundwire/master.c

diff --git a/drivers/soundwire/Makefile b/drivers/soundwire/Makefile
index 563894e5ecaf..89b29819dd3a 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 605bc7ae57a8..2e4e8c2e629f 100644
--- a/drivers/soundwire/bus_type.c
+++ b/drivers/soundwire/bus_type.c
@@ -66,7 +66,10 @@ int sdw_uevent(struct device *dev, struct kobj_uevent_env *env)
 		 * callback is set to use this function for a
 		 * different device type (e.g. Master or Monitor)
 		 */
-		dev_err(dev, "uevent for unknown Soundwire type\n");
+		if (is_sdw_master_device(dev))
+			dev_err(dev, "uevent for SoundWire Master type\n");
+		else
+			dev_err(dev, "uevent for unknown Soundwire type\n");
 		return -EINVAL;
 	}
 
diff --git a/drivers/soundwire/master.c b/drivers/soundwire/master.c
new file mode 100644
index 000000000000..b018c561708e
--- /dev/null
+++ b/drivers/soundwire/master.c
@@ -0,0 +1,63 @@
+// SPDX-License-Identifier: (GPL-2.0)
+// Copyright(c) 2019 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_md_release(struct device *dev)
+{
+	struct sdw_master_device *md = to_sdw_master_device(dev);
+
+	kfree(md);
+}
+
+struct device_type sdw_md_type = {
+	.name =		"soundwire_master",
+	.release =	sdw_md_release,
+};
+
+struct sdw_master_device *sdw_md_add(struct sdw_md_driver *driver,
+				     struct device *parent,
+				     struct fwnode_handle *fwnode,
+				     int link_id)
+{
+	struct sdw_master_device *md;
+	int ret;
+
+	if (!driver->probe) {
+		dev_err(parent, "mandatory probe callback missing\n");
+		return ERR_PTR(-EINVAL);
+	}
+
+	md = kzalloc(sizeof(*md), GFP_KERNEL);
+	if (!md)
+		return ERR_PTR(-ENOMEM);
+
+	md->link_id = link_id;
+
+	md->driver = driver;
+
+	md->dev.parent = parent;
+	md->dev.fwnode = fwnode;
+	md->dev.bus = &sdw_bus_type;
+	md->dev.type = &sdw_md_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(-ENOMEM);
+	}
+
+	return md;
+}
+EXPORT_SYMBOL_GPL(sdw_md_add);
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index 5b1180f1e6b5..f73c355de5e5 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -585,6 +585,16 @@ struct sdw_slave {
 #define to_sdw_slave_device(d) \
 	container_of(d, struct sdw_slave, dev)
 
+struct sdw_master_device {
+	struct device dev;
+	int link_id;
+	struct sdw_md_driver *driver;
+	void *pdata;
+};
+
+#define to_sdw_master_device(d)	\
+	container_of(d, struct sdw_master_device, dev)
+
 struct sdw_driver {
 	const char *name;
 
@@ -599,6 +609,29 @@ struct sdw_driver {
 	struct device_driver driver;
 };
 
+/**
+ * struct sdw_md_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
+ * @shutdown: cleanups before hardware is disabled (optional)
+ * @free: free all remaining resources
+ * @autonomous_clock_stop_enable: enable/disable driver control while
+ * in clock-stop mode, typically in always-on/D0ix modes. When the driver
+ * yields control, another entity in the system (typically firmware
+ * running on an always-on microprocessor) is responsible to tracking
+ * Slave-initiated wakes
+ */
+struct sdw_md_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 (*autonomous_clock_stop_enable)(struct sdw_master_device *md,
+					    bool state);
+};
+
 #define SDW_SLAVE_ENTRY(_mfg_id, _part_id, _drv_data) \
 	{ .mfg_id = (_mfg_id), .part_id = (_part_id), \
 	  .driver_data = (unsigned long)(_drv_data) }
@@ -788,6 +821,11 @@ 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_md_add(struct sdw_md_driver *driver,
+				     struct device *parent,
+				     struct fwnode_handle *fwnode,
+				     int link_id);
+
 /**
  * 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 c681b3426478..a2a5ea7843a6 100644
--- a/include/linux/soundwire/sdw_type.h
+++ b/include/linux/soundwire/sdw_type.h
@@ -6,15 +6,24 @@
 
 extern struct bus_type sdw_bus_type;
 extern struct device_type sdw_slave_type;
+extern struct device_type sdw_md_type;
 
 static inline int is_sdw_slave(const struct device *dev)
 {
 	return dev->type == &sdw_slave_type;
 }
 
+static inline int is_sdw_master_device(const struct device *dev)
+{
+	return dev->type == &sdw_md_type;
+}
+
 #define to_sdw_slave_driver(_drv) \
 	container_of(_drv, struct sdw_driver, driver)
 
+#define to_sdw_md_driver(_drv) \
+	container_of(_drv, struct sdw_md_driver, driver)
+
 #define sdw_register_slave_driver(drv) \
 	__sdw_register_slave_driver(drv, THIS_MODULE)
 
-- 
2.20.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

* [alsa-devel] [PATCH v5 09/17] soundwire: intel: remove platform devices and use 'Master Devices' instead
  2019-12-17 21:02 [alsa-devel] [PATCH v5 00/17] soundwire: intel: implement new ASoC interfaces Pierre-Louis Bossart
                   ` (7 preceding siblings ...)
  2019-12-17 21:03 ` [alsa-devel] [PATCH v5 08/17] soundwire: add initial definitions for sdw_master_device Pierre-Louis Bossart
@ 2019-12-17 21:03 ` Pierre-Louis Bossart
  2019-12-27  9:08   ` Vinod Koul
  2020-01-14  6:09   ` Vinod Koul
  2019-12-17 21:03 ` [alsa-devel] [PATCH v5 10/17] soundwire: register master device driver Pierre-Louis Bossart
                   ` (7 subsequent siblings)
  16 siblings, 2 replies; 42+ messages in thread
From: Pierre-Louis Bossart @ 2019-12-17 21:03 UTC (permalink / raw)
  To: alsa-devel
  Cc: Pierre-Louis Bossart, tiwai, gregkh, linux-kernel,
	Ranjani Sridharan, vkoul, broonie, srinivas.kandagatla, jank,
	slawomir.blauciak, Sanyog Kale, Bard liao, Rander Wang

Use sdw_master_device and driver instead of platform devices

To quote GregKH:

"Don't mess with a platform device unless you really have no other
possible choice. And even then, don't do it and try to do something
else. Platform devices are really abused, don't perpetuate it "

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

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

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

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

* [alsa-devel] [PATCH v5 10/17] soundwire: register master device driver
  2019-12-17 21:02 [alsa-devel] [PATCH v5 00/17] soundwire: intel: implement new ASoC interfaces Pierre-Louis Bossart
                   ` (8 preceding siblings ...)
  2019-12-17 21:03 ` [alsa-devel] [PATCH v5 09/17] soundwire: intel: remove platform devices and use 'Master Devices' instead Pierre-Louis Bossart
@ 2019-12-17 21:03 ` Pierre-Louis Bossart
  2019-12-17 21:03 ` [alsa-devel] [PATCH v5 11/17] soundwire: intel: add prepare support in sdw dai driver Pierre-Louis Bossart
                   ` (6 subsequent siblings)
  16 siblings, 0 replies; 42+ messages in thread
From: Pierre-Louis Bossart @ 2019-12-17 21:03 UTC (permalink / raw)
  To: alsa-devel
  Cc: Pierre-Louis Bossart, tiwai, gregkh, linux-kernel,
	Ranjani Sridharan, vkoul, broonie, srinivas.kandagatla, jank,
	slawomir.blauciak, Sanyog Kale, Bard liao, Rander Wang

From: Bard Liao <yung-chuan.liao@linux.intel.com>

While we don't have a matching function, setting an device driver is
still necessary for ASoC to register DAI components as well as power
management.

Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/intel.c      |  6 ++++++
 drivers/soundwire/intel_init.c | 10 ++++++++++
 drivers/soundwire/master.c     |  1 +
 include/linux/soundwire/sdw.h  |  1 +
 4 files changed, 18 insertions(+)

diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index 36dbcbab0d65..29ebdb07e622 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -17,6 +17,7 @@
 #include <linux/soundwire/sdw_registers.h>
 #include <linux/soundwire/sdw.h>
 #include <linux/soundwire/sdw_intel.h>
+#include <linux/soundwire/sdw_type.h>
 #include "cadence_master.h"
 #include "bus.h"
 #include "intel.h"
@@ -1058,6 +1059,11 @@ static int intel_master_remove(struct sdw_master_device *md)
 }
 
 struct sdw_md_driver intel_sdw_driver = {
+	.driver = {
+		.name = "intel-sdw",
+		.owner = THIS_MODULE,
+		.bus = &sdw_bus_type,
+	},
 	.probe = intel_master_probe,
 	.startup = intel_master_startup,
 	.remove = intel_master_remove,
diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c
index 42f7ae034bea..a30d95ee71b7 100644
--- a/drivers/soundwire/intel_init.c
+++ b/drivers/soundwire/intel_init.c
@@ -146,6 +146,7 @@ static struct sdw_intel_ctx
 	struct sdw_master_device *md;
 	u32 link_mask;
 	int count;
+	int err;
 	int i;
 
 	if (!res)
@@ -176,6 +177,12 @@ static struct sdw_intel_ctx
 	link = ctx->links;
 	link_mask = ctx->link_mask;
 
+	err = driver_register(&intel_sdw_driver.driver);
+	if (err) {
+		dev_err(&adev->dev, "failed to register sdw master driver\n");
+		goto register_err;
+	}
+
 	/* Create SDW Master devices */
 	for (i = 0; i < count; i++, link++) {
 		if (link_mask && !(link_mask & BIT(i)))
@@ -209,6 +216,8 @@ static struct sdw_intel_ctx
 err:
 	sdw_intel_cleanup(ctx);
 link_err:
+	driver_unregister(&intel_sdw_driver.driver);
+register_err:
 	kfree(ctx);
 	return NULL;
 }
@@ -350,6 +359,7 @@ EXPORT_SYMBOL(sdw_intel_startup);
 void sdw_intel_exit(struct sdw_intel_ctx *ctx)
 {
 	sdw_intel_cleanup(ctx);
+	driver_unregister(&intel_sdw_driver.driver);
 	kfree(ctx);
 }
 EXPORT_SYMBOL(sdw_intel_exit);
diff --git a/drivers/soundwire/master.c b/drivers/soundwire/master.c
index b018c561708e..1c6bb293468c 100644
--- a/drivers/soundwire/master.c
+++ b/drivers/soundwire/master.c
@@ -46,6 +46,7 @@ struct sdw_master_device *sdw_md_add(struct sdw_md_driver *driver,
 	md->dev.type = &sdw_md_type;
 	md->dev.dma_mask = md->dev.parent->dma_mask;
 	dev_set_name(&md->dev, "sdw-master-%d", md->link_id);
+	md->dev.driver = &driver->driver;
 
 	ret = device_register(&md->dev);
 	if (ret) {
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index f73c355de5e5..58f50257dfa8 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -630,6 +630,7 @@ struct sdw_md_driver {
 	int (*remove)(struct sdw_master_device *md);
 	int (*autonomous_clock_stop_enable)(struct sdw_master_device *md,
 					    bool state);
+	struct device_driver driver;
 };
 
 #define SDW_SLAVE_ENTRY(_mfg_id, _part_id, _drv_data) \
-- 
2.20.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

* [alsa-devel] [PATCH v5 11/17] soundwire: intel: add prepare support in sdw dai driver
  2019-12-17 21:02 [alsa-devel] [PATCH v5 00/17] soundwire: intel: implement new ASoC interfaces Pierre-Louis Bossart
                   ` (9 preceding siblings ...)
  2019-12-17 21:03 ` [alsa-devel] [PATCH v5 10/17] soundwire: register master device driver Pierre-Louis Bossart
@ 2019-12-17 21:03 ` Pierre-Louis Bossart
  2019-12-17 21:03 ` [alsa-devel] [PATCH v5 12/17] soundwire: intel: add trigger " Pierre-Louis Bossart
                   ` (5 subsequent siblings)
  16 siblings, 0 replies; 42+ messages in thread
From: Pierre-Louis Bossart @ 2019-12-17 21:03 UTC (permalink / raw)
  To: alsa-devel
  Cc: Pierre-Louis Bossart, tiwai, gregkh, linux-kernel,
	Ranjani Sridharan, vkoul, broonie, srinivas.kandagatla, jank,
	slawomir.blauciak, Sanyog Kale, Bard liao, Rander Wang

From: Rander Wang <rander.wang@linux.intel.com>

The existing code does not expose a prepare operation, which is very
much needed to deal with underflow and resume operations.

Signed-off-by: Rander Wang <rander.wang@linux.intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/intel.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index 29ebdb07e622..689eb350f15e 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -698,6 +698,21 @@ static int intel_hw_params(struct snd_pcm_substream *substream,
 	return ret;
 }
 
+static int intel_prepare(struct snd_pcm_substream *substream,
+			 struct snd_soc_dai *dai)
+{
+	struct sdw_cdns_dma_data *dma;
+
+	dma = snd_soc_dai_get_dma_data(dai, substream);
+	if (!dma) {
+		dev_err(dai->dev, "failed to get dma data in %s",
+			__func__);
+		return -EIO;
+	}
+
+	return sdw_prepare_stream(dma->stream);
+}
+
 static int
 intel_hw_free(struct snd_pcm_substream *substream, struct snd_soc_dai *dai)
 {
@@ -744,6 +759,7 @@ static int intel_pdm_set_sdw_stream(struct snd_soc_dai *dai,
 
 static const struct snd_soc_dai_ops intel_pcm_dai_ops = {
 	.hw_params = intel_hw_params,
+	.prepare = intel_prepare,
 	.hw_free = intel_hw_free,
 	.shutdown = intel_shutdown,
 	.set_sdw_stream = intel_pcm_set_sdw_stream,
@@ -751,6 +767,7 @@ static const struct snd_soc_dai_ops intel_pcm_dai_ops = {
 
 static const struct snd_soc_dai_ops intel_pdm_dai_ops = {
 	.hw_params = intel_hw_params,
+	.prepare = intel_prepare,
 	.hw_free = intel_hw_free,
 	.shutdown = intel_shutdown,
 	.set_sdw_stream = intel_pdm_set_sdw_stream,
-- 
2.20.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

* [alsa-devel] [PATCH v5 12/17] soundwire: intel: add trigger support in sdw dai driver
  2019-12-17 21:02 [alsa-devel] [PATCH v5 00/17] soundwire: intel: implement new ASoC interfaces Pierre-Louis Bossart
                   ` (10 preceding siblings ...)
  2019-12-17 21:03 ` [alsa-devel] [PATCH v5 11/17] soundwire: intel: add prepare support in sdw dai driver Pierre-Louis Bossart
@ 2019-12-17 21:03 ` " Pierre-Louis Bossart
  2019-12-17 21:03 ` [alsa-devel] [PATCH v5 13/17] soundwire: intel: add sdw_stream_setup helper for .startup callback Pierre-Louis Bossart
                   ` (4 subsequent siblings)
  16 siblings, 0 replies; 42+ messages in thread
From: Pierre-Louis Bossart @ 2019-12-17 21:03 UTC (permalink / raw)
  To: alsa-devel
  Cc: Pierre-Louis Bossart, tiwai, gregkh, linux-kernel,
	Ranjani Sridharan, vkoul, broonie, srinivas.kandagatla, jank,
	slawomir.blauciak, Sanyog Kale, Bard liao, Rander Wang

From: Rander Wang <rander.wang@linux.intel.com>

The existing code does not expose a trigger callback, which is very
much required for streaming.

The SoundWire stream is enabled and disabled in trigger function.

Signed-off-by: Rander Wang <rander.wang@linux.intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/intel.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index 689eb350f15e..4ff15327fbd7 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -713,6 +713,43 @@ static int intel_prepare(struct snd_pcm_substream *substream,
 	return sdw_prepare_stream(dma->stream);
 }
 
+static int intel_trigger(struct snd_pcm_substream *substream, int cmd,
+			 struct snd_soc_dai *dai)
+{
+	struct sdw_cdns_dma_data *dma;
+	int ret;
+
+	dma = snd_soc_dai_get_dma_data(dai, substream);
+	if (!dma) {
+		dev_err(dai->dev, "failed to get dma data in %s", __func__);
+		return -EIO;
+	}
+
+	switch (cmd) {
+	case SNDRV_PCM_TRIGGER_START:
+	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+	case SNDRV_PCM_TRIGGER_RESUME:
+		ret = sdw_enable_stream(dma->stream);
+		break;
+
+	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+	case SNDRV_PCM_TRIGGER_SUSPEND:
+	case SNDRV_PCM_TRIGGER_STOP:
+		ret = sdw_disable_stream(dma->stream);
+		break;
+
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	if (ret)
+		dev_err(dai->dev,
+			"%s trigger %d failed: %d",
+			__func__, cmd, ret);
+	return ret;
+}
+
 static int
 intel_hw_free(struct snd_pcm_substream *substream, struct snd_soc_dai *dai)
 {
@@ -760,6 +797,7 @@ static int intel_pdm_set_sdw_stream(struct snd_soc_dai *dai,
 static const struct snd_soc_dai_ops intel_pcm_dai_ops = {
 	.hw_params = intel_hw_params,
 	.prepare = intel_prepare,
+	.trigger = intel_trigger,
 	.hw_free = intel_hw_free,
 	.shutdown = intel_shutdown,
 	.set_sdw_stream = intel_pcm_set_sdw_stream,
@@ -768,6 +806,7 @@ static const struct snd_soc_dai_ops intel_pcm_dai_ops = {
 static const struct snd_soc_dai_ops intel_pdm_dai_ops = {
 	.hw_params = intel_hw_params,
 	.prepare = intel_prepare,
+	.trigger = intel_trigger,
 	.hw_free = intel_hw_free,
 	.shutdown = intel_shutdown,
 	.set_sdw_stream = intel_pdm_set_sdw_stream,
-- 
2.20.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

* [alsa-devel] [PATCH v5 13/17] soundwire: intel: add sdw_stream_setup helper for .startup callback
  2019-12-17 21:02 [alsa-devel] [PATCH v5 00/17] soundwire: intel: implement new ASoC interfaces Pierre-Louis Bossart
                   ` (11 preceding siblings ...)
  2019-12-17 21:03 ` [alsa-devel] [PATCH v5 12/17] soundwire: intel: add trigger " Pierre-Louis Bossart
@ 2019-12-17 21:03 ` Pierre-Louis Bossart
  2019-12-17 21:03 ` [alsa-devel] [PATCH v5 14/17] soundwire: intel: free all resources on hw_free() Pierre-Louis Bossart
                   ` (3 subsequent siblings)
  16 siblings, 0 replies; 42+ messages in thread
From: Pierre-Louis Bossart @ 2019-12-17 21:03 UTC (permalink / raw)
  To: alsa-devel
  Cc: Pierre-Louis Bossart, tiwai, gregkh, linux-kernel,
	Ranjani Sridharan, vkoul, broonie, srinivas.kandagatla, jank,
	slawomir.blauciak, Sanyog Kale, Bard liao, Rander Wang

From: Rander Wang <rander.wang@linux.intel.com>

The sdw stream is allocated and stored in dai to share the sdw runtime
information.

Signed-off-by: Rander Wang <rander.wang@linux.intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/intel.c | 65 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 65 insertions(+)

diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index 4ff15327fbd7..b4a9337b64d2 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -616,6 +616,69 @@ static int intel_post_bank_switch(struct sdw_bus *bus)
  * DAI routines
  */
 
+static int sdw_stream_setup(struct snd_pcm_substream *substream,
+			    struct snd_soc_dai *dai)
+{
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	struct sdw_stream_runtime *sdw_stream = NULL;
+	char *name;
+	int i, ret;
+
+	name = kzalloc(32, GFP_KERNEL);
+	if (!name)
+		return -ENOMEM;
+
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+		snprintf(name, 32, "%s-Playback", dai->name);
+	else
+		snprintf(name, 32, "%s-Capture", dai->name);
+
+	sdw_stream = sdw_alloc_stream(name);
+	if (!sdw_stream) {
+		dev_err(dai->dev, "alloc stream failed for DAI %s", dai->name);
+		ret = -ENOMEM;
+		goto error;
+	}
+
+	/* Set stream pointer on CPU DAI */
+	ret = snd_soc_dai_set_sdw_stream(dai, sdw_stream, substream->stream);
+	if (ret < 0) {
+		dev_err(dai->dev, "failed to set stream pointer on cpu dai %s",
+			dai->name);
+		goto release_stream;
+	}
+
+	/* Set stream pointer on all CODEC DAIs */
+	for (i = 0; i < rtd->num_codecs; i++) {
+		ret = snd_soc_dai_set_sdw_stream(rtd->codec_dais[i], sdw_stream,
+						 substream->stream);
+		if (ret < 0) {
+			dev_err(dai->dev, "failed to set stream pointer on codec dai %s",
+				rtd->codec_dais[i]->name);
+			goto release_stream;
+		}
+	}
+
+	return 0;
+
+release_stream:
+	sdw_release_stream(sdw_stream);
+error:
+	kfree(name);
+	return ret;
+}
+
+static int intel_startup(struct snd_pcm_substream *substream,
+			 struct snd_soc_dai *dai)
+{
+	/*
+	 * TODO: add pm_runtime support here, the startup callback
+	 * will make sure the IP is 'active'
+	 */
+
+	return sdw_stream_setup(substream, dai);
+}
+
 static int intel_hw_params(struct snd_pcm_substream *substream,
 			   struct snd_pcm_hw_params *params,
 			   struct snd_soc_dai *dai)
@@ -795,6 +858,7 @@ static int intel_pdm_set_sdw_stream(struct snd_soc_dai *dai,
 }
 
 static const struct snd_soc_dai_ops intel_pcm_dai_ops = {
+	.startup = intel_startup,
 	.hw_params = intel_hw_params,
 	.prepare = intel_prepare,
 	.trigger = intel_trigger,
@@ -804,6 +868,7 @@ static const struct snd_soc_dai_ops intel_pcm_dai_ops = {
 };
 
 static const struct snd_soc_dai_ops intel_pdm_dai_ops = {
+	.startup = intel_startup,
 	.hw_params = intel_hw_params,
 	.prepare = intel_prepare,
 	.trigger = intel_trigger,
-- 
2.20.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

* [alsa-devel] [PATCH v5 14/17] soundwire: intel: free all resources on hw_free()
  2019-12-17 21:02 [alsa-devel] [PATCH v5 00/17] soundwire: intel: implement new ASoC interfaces Pierre-Louis Bossart
                   ` (12 preceding siblings ...)
  2019-12-17 21:03 ` [alsa-devel] [PATCH v5 13/17] soundwire: intel: add sdw_stream_setup helper for .startup callback Pierre-Louis Bossart
@ 2019-12-17 21:03 ` Pierre-Louis Bossart
  2019-12-17 21:03 ` [alsa-devel] [PATCH v5 15/17] soundwire: intel_init: add implementation of sdw_intel_enable_irq() Pierre-Louis Bossart
                   ` (2 subsequent siblings)
  16 siblings, 0 replies; 42+ messages in thread
From: Pierre-Louis Bossart @ 2019-12-17 21:03 UTC (permalink / raw)
  To: alsa-devel
  Cc: Pierre-Louis Bossart, tiwai, gregkh, linux-kernel,
	Ranjani Sridharan, vkoul, broonie, srinivas.kandagatla, jank,
	slawomir.blauciak, Sanyog Kale, Bard liao, Rander Wang

Make sure all calls to the SoundWire stream API are done and involve
callback

Signed-off-by: Rander Wang <rander.wang@linux.intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/intel.c | 40 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 38 insertions(+), 2 deletions(-)

diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index b4a9337b64d2..0e77df0a7760 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -549,6 +549,25 @@ static int intel_params_stream(struct sdw_intel *sdw,
 	return -EIO;
 }
 
+static int intel_free_stream(struct sdw_intel *sdw,
+			     struct snd_pcm_substream *substream,
+			     struct snd_soc_dai *dai,
+			     int link_id)
+{
+	struct sdw_intel_link_res *res = sdw->link_res;
+	struct sdw_intel_stream_free_data free_data;
+
+	free_data.substream = substream;
+	free_data.dai = dai;
+	free_data.link_id = link_id;
+
+	if (res->ops && res->ops->free_stream && res->dev)
+		return res->ops->free_stream(res->dev,
+					     &free_data);
+
+	return 0;
+}
+
 /*
  * bank switch routines
  */
@@ -817,6 +836,7 @@ static int
 intel_hw_free(struct snd_pcm_substream *substream, struct snd_soc_dai *dai)
 {
 	struct sdw_cdns *cdns = snd_soc_dai_get_drvdata(dai);
+	struct sdw_intel *sdw = cdns_to_intel(cdns);
 	struct sdw_cdns_dma_data *dma;
 	int ret;
 
@@ -824,12 +844,28 @@ intel_hw_free(struct snd_pcm_substream *substream, struct snd_soc_dai *dai)
 	if (!dma)
 		return -EIO;
 
+	ret = sdw_deprepare_stream(dma->stream);
+	if (ret) {
+		dev_err(dai->dev, "sdw_deprepare_stream: failed %d", ret);
+		return ret;
+	}
+
 	ret = sdw_stream_remove_master(&cdns->bus, dma->stream);
-	if (ret < 0)
+	if (ret < 0) {
 		dev_err(dai->dev, "remove master from stream %s failed: %d\n",
 			dma->stream->name, ret);
+		return ret;
+	}
 
-	return ret;
+	ret = intel_free_stream(sdw, substream, dai, sdw->instance);
+	if (ret < 0) {
+		dev_err(dai->dev, "intel_free_stream: failed %d", ret);
+		return ret;
+	}
+
+	sdw_release_stream(dma->stream);
+
+	return 0;
 }
 
 static void intel_shutdown(struct snd_pcm_substream *substream,
-- 
2.20.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

* [alsa-devel] [PATCH v5 15/17] soundwire: intel_init: add implementation of sdw_intel_enable_irq()
  2019-12-17 21:02 [alsa-devel] [PATCH v5 00/17] soundwire: intel: implement new ASoC interfaces Pierre-Louis Bossart
                   ` (13 preceding siblings ...)
  2019-12-17 21:03 ` [alsa-devel] [PATCH v5 14/17] soundwire: intel: free all resources on hw_free() Pierre-Louis Bossart
@ 2019-12-17 21:03 ` Pierre-Louis Bossart
  2019-12-17 21:03 ` [alsa-devel] [PATCH v5 16/17] soundwire: intel_init: use EXPORT_SYMBOL_NS Pierre-Louis Bossart
  2019-12-17 21:03 ` [alsa-devel] [PATCH v5 17/17] soundwire: intel: " Pierre-Louis Bossart
  16 siblings, 0 replies; 42+ messages in thread
From: Pierre-Louis Bossart @ 2019-12-17 21:03 UTC (permalink / raw)
  To: alsa-devel
  Cc: Pierre-Louis Bossart, tiwai, gregkh, linux-kernel,
	Ranjani Sridharan, vkoul, broonie, srinivas.kandagatla, jank,
	slawomir.blauciak, Sanyog Kale, Bard liao, Rander Wang

This function is required to enable all interrupts across all links.

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

diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c
index a30d95ee71b7..391e8763638f 100644
--- a/drivers/soundwire/intel_init.c
+++ b/drivers/soundwire/intel_init.c
@@ -137,6 +137,30 @@ sdw_intel_scan_controller(struct sdw_intel_acpi_info *info)
 	return 0;
 }
 
+#define HDA_DSP_REG_ADSPIC2             (0x10)
+#define HDA_DSP_REG_ADSPIS2             (0x14)
+#define HDA_DSP_REG_ADSPIC2_SNDW        BIT(5)
+
+/**
+ * sdw_intel_enable_irq() - enable/disable Intel SoundWire IRQ
+ * @mmio_base: The mmio base of the control register
+ * @enable: true if enable
+ */
+void sdw_intel_enable_irq(void __iomem *mmio_base, bool enable)
+{
+	u32 val;
+
+	val = readl(mmio_base + HDA_DSP_REG_ADSPIC2);
+
+	if (enable)
+		val |= HDA_DSP_REG_ADSPIC2_SNDW;
+	else
+		val &= ~HDA_DSP_REG_ADSPIC2_SNDW;
+
+	writel(val, mmio_base + HDA_DSP_REG_ADSPIC2);
+}
+EXPORT_SYMBOL(sdw_intel_enable_irq);
+
 static struct sdw_intel_ctx
 *sdw_intel_probe_controller(struct sdw_intel_res *res)
 {
-- 
2.20.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

* [alsa-devel] [PATCH v5 16/17] soundwire: intel_init: use EXPORT_SYMBOL_NS
  2019-12-17 21:02 [alsa-devel] [PATCH v5 00/17] soundwire: intel: implement new ASoC interfaces Pierre-Louis Bossart
                   ` (14 preceding siblings ...)
  2019-12-17 21:03 ` [alsa-devel] [PATCH v5 15/17] soundwire: intel_init: add implementation of sdw_intel_enable_irq() Pierre-Louis Bossart
@ 2019-12-17 21:03 ` Pierre-Louis Bossart
  2019-12-17 21:03 ` [alsa-devel] [PATCH v5 17/17] soundwire: intel: " Pierre-Louis Bossart
  16 siblings, 0 replies; 42+ messages in thread
From: Pierre-Louis Bossart @ 2019-12-17 21:03 UTC (permalink / raw)
  To: alsa-devel
  Cc: Pierre-Louis Bossart, tiwai, gregkh, linux-kernel,
	Ranjani Sridharan, vkoul, broonie, srinivas.kandagatla, jank,
	slawomir.blauciak, Sanyog Kale, Bard liao, Rander Wang

Make sure all symbols in this soundwire-intel-init module are exported
with a namespace.

The MODULE_IMPORT_NS will be used in Intel/SOF HDaudio modules to be
posted in a separate series.

Namespaces are only introduced for the Intel parts of the SoundWire
code at this time, in future patches we should also add namespaces for
Cadence parts and the SoundWire core.

Suggested-by: Greg KH <gregkh@linuxfoundation.org>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/intel_init.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c
index 391e8763638f..0f83907e1bc7 100644
--- a/drivers/soundwire/intel_init.c
+++ b/drivers/soundwire/intel_init.c
@@ -159,7 +159,7 @@ void sdw_intel_enable_irq(void __iomem *mmio_base, bool enable)
 
 	writel(val, mmio_base + HDA_DSP_REG_ADSPIC2);
 }
-EXPORT_SYMBOL(sdw_intel_enable_irq);
+EXPORT_SYMBOL_NS(sdw_intel_enable_irq, SOUNDWIRE_INTEL_INIT);
 
 static struct sdw_intel_ctx
 *sdw_intel_probe_controller(struct sdw_intel_res *res)
@@ -346,7 +346,7 @@ int sdw_intel_acpi_scan(acpi_handle *parent_handle,
 
 	return sdw_intel_scan_controller(info);
 }
-EXPORT_SYMBOL(sdw_intel_acpi_scan);
+EXPORT_SYMBOL_NS(sdw_intel_acpi_scan, SOUNDWIRE_INTEL_INIT);
 
 /**
  * sdw_intel_probe() - SoundWire Intel probe routine
@@ -362,7 +362,7 @@ struct sdw_intel_ctx
 {
 	return sdw_intel_probe_controller(res);
 }
-EXPORT_SYMBOL(sdw_intel_probe);
+EXPORT_SYMBOL_NS(sdw_intel_probe, SOUNDWIRE_INTEL_INIT);
 
 /**
  * sdw_intel_startup() - SoundWire Intel startup
@@ -373,7 +373,7 @@ int sdw_intel_startup(struct sdw_intel_ctx *ctx)
 {
 	return sdw_intel_startup_controller(ctx);
 }
-EXPORT_SYMBOL(sdw_intel_startup);
+EXPORT_SYMBOL_NS(sdw_intel_startup, SOUNDWIRE_INTEL_INIT);
 /**
  * sdw_intel_exit() - SoundWire Intel exit
  * @ctx: SoundWire context allocated in the probe
@@ -386,7 +386,7 @@ void sdw_intel_exit(struct sdw_intel_ctx *ctx)
 	driver_unregister(&intel_sdw_driver.driver);
 	kfree(ctx);
 }
-EXPORT_SYMBOL(sdw_intel_exit);
+EXPORT_SYMBOL_NS(sdw_intel_exit, SOUNDWIRE_INTEL_INIT);
 
 MODULE_LICENSE("Dual BSD/GPL");
 MODULE_DESCRIPTION("Intel Soundwire Init Library");
-- 
2.20.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

* [alsa-devel] [PATCH v5 17/17] soundwire: intel: use EXPORT_SYMBOL_NS
  2019-12-17 21:02 [alsa-devel] [PATCH v5 00/17] soundwire: intel: implement new ASoC interfaces Pierre-Louis Bossart
                   ` (15 preceding siblings ...)
  2019-12-17 21:03 ` [alsa-devel] [PATCH v5 16/17] soundwire: intel_init: use EXPORT_SYMBOL_NS Pierre-Louis Bossart
@ 2019-12-17 21:03 ` " Pierre-Louis Bossart
  16 siblings, 0 replies; 42+ messages in thread
From: Pierre-Louis Bossart @ 2019-12-17 21:03 UTC (permalink / raw)
  To: alsa-devel
  Cc: Pierre-Louis Bossart, tiwai, gregkh, linux-kernel,
	Ranjani Sridharan, vkoul, broonie, srinivas.kandagatla, jank,
	slawomir.blauciak, Sanyog Kale, Bard liao, Rander Wang

The soundwire-intel module exports an 'intel_sdw_driver' structure,
which is declared with a namespace explicitly imported by the
soundwire-intel-init module.

The use of namespaces might be deemed overkill here, but it did help
enforce a proper code partitioning for follow-up patches on clock-stop
support.

Suggested-by: Greg KH <gregkh@linuxfoundation.org>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/intel.c      | 2 +-
 drivers/soundwire/intel_init.c | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index 0e77df0a7760..c24b5f30789d 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -1225,7 +1225,7 @@ struct sdw_md_driver intel_sdw_driver = {
 	.startup = intel_master_startup,
 	.remove = intel_master_remove,
 };
-EXPORT_SYMBOL(intel_sdw_driver);
+EXPORT_SYMBOL_NS(intel_sdw_driver, SOUNDWIRE_INTEL);
 
 MODULE_LICENSE("Dual BSD/GPL");
 MODULE_ALIAS("platform:int-sdw");
diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c
index 0f83907e1bc7..0acb92a3c6d1 100644
--- a/drivers/soundwire/intel_init.c
+++ b/drivers/soundwire/intel_init.c
@@ -390,3 +390,4 @@ EXPORT_SYMBOL_NS(sdw_intel_exit, SOUNDWIRE_INTEL_INIT);
 
 MODULE_LICENSE("Dual BSD/GPL");
 MODULE_DESCRIPTION("Intel Soundwire Init Library");
+MODULE_IMPORT_NS(SOUNDWIRE_INTEL);
-- 
2.20.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [alsa-devel] [PATCH v5 02/17] soundwire: rename dev_to_sdw_dev macro
  2019-12-17 21:02 ` [alsa-devel] [PATCH v5 02/17] soundwire: rename dev_to_sdw_dev macro Pierre-Louis Bossart
@ 2019-12-27  6:54   ` Vinod Koul
  0 siblings, 0 replies; 42+ messages in thread
From: Vinod Koul @ 2019-12-27  6:54 UTC (permalink / raw)
  To: Pierre-Louis Bossart, broonie
  Cc: alsa-devel, Rafael J. Wysocki, tiwai, gregkh, linux-kernel,
	Ranjani Sridharan, srinivas.kandagatla, jank, slawomir.blauciak,
	Sanyog Kale, Bard liao, Rander Wang

On 17-12-19, 15:02, Pierre-Louis Bossart wrote:
> Since we want to introduce master devices, rename macro so that we
> have consistency between slave and master device access.
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  drivers/base/regmap/regmap-sdw.c |  4 ++--

need ack from Mark on this...

>  drivers/soundwire/bus.c          |  2 +-
>  drivers/soundwire/bus_type.c     | 11 ++++++-----
>  drivers/soundwire/slave.c        |  2 +-
>  include/linux/soundwire/sdw.h    |  3 ++-
>  5 files changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/base/regmap/regmap-sdw.c b/drivers/base/regmap/regmap-sdw.c
> index 50a66382d87d..d1fc0c22180a 100644
> --- a/drivers/base/regmap/regmap-sdw.c
> +++ b/drivers/base/regmap/regmap-sdw.c
> @@ -10,7 +10,7 @@
>  static int regmap_sdw_write(void *context, unsigned int reg, unsigned int val)
>  {
>  	struct device *dev = context;
> -	struct sdw_slave *slave = dev_to_sdw_dev(dev);
> +	struct sdw_slave *slave = to_sdw_slave_device(dev);
>  
>  	return sdw_write(slave, reg, val);
>  }
> @@ -18,7 +18,7 @@ static int regmap_sdw_write(void *context, unsigned int reg, unsigned int val)
>  static int regmap_sdw_read(void *context, unsigned int reg, unsigned int *val)
>  {
>  	struct device *dev = context;
> -	struct sdw_slave *slave = dev_to_sdw_dev(dev);
> +	struct sdw_slave *slave = to_sdw_slave_device(dev);
>  	int read;
>  
>  	read = sdw_read(slave, reg);
> diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
> index be5d437058ed..4b22ee996a65 100644
> --- a/drivers/soundwire/bus.c
> +++ b/drivers/soundwire/bus.c
> @@ -110,7 +110,7 @@ EXPORT_SYMBOL(sdw_add_bus_master);
>  
>  static int sdw_delete_slave(struct device *dev, void *data)
>  {
> -	struct sdw_slave *slave = dev_to_sdw_dev(dev);
> +	struct sdw_slave *slave = to_sdw_slave_device(dev);
>  	struct sdw_bus *bus = slave->bus;
>  
>  	sdw_slave_debugfs_exit(slave);
> diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c
> index 370b94752662..c0585bcc8a41 100644
> --- a/drivers/soundwire/bus_type.c
> +++ b/drivers/soundwire/bus_type.c
> @@ -33,7 +33,7 @@ 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_slave *slave = to_sdw_slave_device(dev);
>  	struct sdw_driver *drv = drv_to_sdw_slave_driver(ddrv);
>  
>  	return !!sdw_get_device_id(slave, drv);
> @@ -49,7 +49,7 @@ int sdw_slave_modalias(const struct sdw_slave *slave, char *buf, size_t size)
>  
>  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 = to_sdw_slave_device(dev);
>  	char modalias[32];
>  
>  	sdw_slave_modalias(slave, modalias, sizeof(modalias));
> @@ -69,7 +69,7 @@ EXPORT_SYMBOL_GPL(sdw_bus_type);
>  
>  static int sdw_drv_probe(struct device *dev)
>  {
> -	struct sdw_slave *slave = dev_to_sdw_dev(dev);
> +	struct sdw_slave *slave = to_sdw_slave_device(dev);
>  	struct sdw_driver *drv = drv_to_sdw_slave_driver(dev->driver);
>  	const struct sdw_device_id *id;
>  	int ret;
> @@ -115,8 +115,9 @@ static int sdw_drv_probe(struct device *dev)
>  
>  static int sdw_drv_remove(struct device *dev)
>  {
> -	struct sdw_slave *slave = dev_to_sdw_dev(dev);
> +	struct sdw_slave *slave = to_sdw_slave_device(dev);
>  	struct sdw_driver *drv = drv_to_sdw_slave_driver(dev->driver);
> +
>  	int ret = 0;
>  
>  	if (drv->remove)
> @@ -129,7 +130,7 @@ static int sdw_drv_remove(struct device *dev)
>  
>  static void sdw_drv_shutdown(struct device *dev)
>  {
> -	struct sdw_slave *slave = dev_to_sdw_dev(dev);
> +	struct sdw_slave *slave = to_sdw_slave_device(dev);
>  	struct sdw_driver *drv = drv_to_sdw_slave_driver(dev->driver);
>  
>  	if (drv->shutdown)
> diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c
> index 19919975bb6d..48a513680db6 100644
> --- a/drivers/soundwire/slave.c
> +++ b/drivers/soundwire/slave.c
> @@ -9,7 +9,7 @@
>  
>  static void sdw_slave_release(struct device *dev)
>  {
> -	struct sdw_slave *slave = dev_to_sdw_dev(dev);
> +	struct sdw_slave *slave = to_sdw_slave_device(dev);
>  
>  	kfree(slave);
>  }
> diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
> index b7c9eca4332a..5b1180f1e6b5 100644
> --- a/include/linux/soundwire/sdw.h
> +++ b/include/linux/soundwire/sdw.h
> @@ -582,7 +582,8 @@ struct sdw_slave {
>  	u32 unattach_request;
>  };
>  
> -#define dev_to_sdw_dev(_dev) container_of(_dev, struct sdw_slave, dev)
> +#define to_sdw_slave_device(d) \
> +	container_of(d, struct sdw_slave, dev)
>  
>  struct sdw_driver {
>  	const char *name;
> -- 
> 2.20.1

-- 
~Vinod
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [alsa-devel] [PATCH v5 03/17] soundwire: rename drv_to_sdw_slave_driver macro
  2019-12-17 21:03 ` [alsa-devel] [PATCH v5 03/17] soundwire: rename drv_to_sdw_slave_driver macro Pierre-Louis Bossart
@ 2019-12-27  7:00   ` Vinod Koul
  2019-12-27 23:23     ` Pierre-Louis Bossart
  0 siblings, 1 reply; 42+ messages in thread
From: Vinod Koul @ 2019-12-27  7:00 UTC (permalink / raw)
  To: Pierre-Louis Bossart
  Cc: alsa-devel, tiwai, gregkh, linux-kernel, Ranjani Sridharan,
	broonie, srinivas.kandagatla, jank, slawomir.blauciak,
	Sanyog Kale, Bard liao, Rander Wang

On 17-12-19, 15:03, Pierre-Louis Bossart wrote:
> Align with previous renames and shorten macro
> 
> No functionality change
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  drivers/soundwire/bus_type.c       | 9 ++++-----
>  include/linux/soundwire/sdw_type.h | 3 ++-
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c
> index c0585bcc8a41..2b2830b622fa 100644
> --- a/drivers/soundwire/bus_type.c
> +++ b/drivers/soundwire/bus_type.c
> @@ -34,7 +34,7 @@ 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 = to_sdw_slave_device(dev);
> -	struct sdw_driver *drv = drv_to_sdw_slave_driver(ddrv);
> +	struct sdw_driver *drv = to_sdw_slave_driver(ddrv);

so patch 1 does:

-       struct sdw_driver *drv = drv_to_sdw_driver(dev->driver);
+       struct sdw_driver *drv = drv_to_sdw_slave_driver(dev->driver);

and here we move drv_to_sdw_slave_driver to to_sdw_slave_driver... why
not do this in first patch and save step1... or did i miss something??

-- 
~Vinod
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [alsa-devel] [PATCH v5 06/17] soundwire: add support for sdw_slave_type
  2019-12-17 21:03 ` [alsa-devel] [PATCH v5 06/17] soundwire: add support for sdw_slave_type Pierre-Louis Bossart
@ 2019-12-27  7:03   ` Vinod Koul
  2019-12-27 23:26     ` Pierre-Louis Bossart
  0 siblings, 1 reply; 42+ messages in thread
From: Vinod Koul @ 2019-12-27  7:03 UTC (permalink / raw)
  To: Pierre-Louis Bossart
  Cc: alsa-devel, tiwai, gregkh, linux-kernel, Ranjani Sridharan,
	broonie, srinivas.kandagatla, jank, slawomir.blauciak,
	Sanyog Kale, Bard liao, Rander Wang

On 17-12-19, 15:03, Pierre-Louis Bossart wrote:
> In the existing SoundWire code, the bus does not have any explicit
> representation for Master Devices - only SoundWire Slaves are exposed.
> 
> In SoundWire, the 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 first convert the existing code to use an explicit
> sdw_slave_type and add error checks if this type is not set.
> 
> In follow-up patches we can add support for the sdw_master_type.
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  drivers/soundwire/bus_type.c       | 23 ++++++++++++++++++-----
>  drivers/soundwire/slave.c          |  7 ++++++-
>  include/linux/soundwire/sdw_type.h |  6 ++++++
>  3 files changed, 30 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c
> index 9a0fd3ee1014..9719680a1e48 100644
> --- a/drivers/soundwire/bus_type.c
> +++ b/drivers/soundwire/bus_type.c
> @@ -49,13 +49,26 @@ int sdw_slave_modalias(const struct sdw_slave *slave, char *buf, size_t size)
>  
>  static int sdw_uevent(struct device *dev, struct kobj_uevent_env *env)
>  {
> -	struct sdw_slave *slave = to_sdw_slave_device(dev);
> +	struct sdw_slave *slave;
>  	char modalias[32];
>  
> -	sdw_slave_modalias(slave, modalias, sizeof(modalias));
> -
> -	if (add_uevent_var(env, "MODALIAS=%s", modalias))
> -		return -ENOMEM;
> +	if (is_sdw_slave(dev)) {
> +		slave = to_sdw_slave_device(dev);
> +
> +		sdw_slave_modalias(slave, modalias, sizeof(modalias));
> +
> +		if (add_uevent_var(env, "MODALIAS=%s", modalias))
> +			return -ENOMEM;
> +	} else {
> +		/*
> +		 * We only need to handle uevents for the Slave device
> +		 * type. This error cannot happen unless the .uevent
> +		 * callback is set to use this function for a
> +		 * different device type (e.g. Master or Monitor)
> +		 */
> +		dev_err(dev, "uevent for unknown Soundwire type\n");
> +		return -EINVAL;

At this point and after next patch, the above code would be a no-op, do
we want this here, if so why?

-- 
~Vinod
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [alsa-devel] [PATCH v5 08/17] soundwire: add initial definitions for sdw_master_device
  2019-12-17 21:03 ` [alsa-devel] [PATCH v5 08/17] soundwire: add initial definitions for sdw_master_device Pierre-Louis Bossart
@ 2019-12-27  7:14   ` Vinod Koul
  2019-12-27 23:38     ` Pierre-Louis Bossart
  0 siblings, 1 reply; 42+ messages in thread
From: Vinod Koul @ 2019-12-27  7:14 UTC (permalink / raw)
  To: Pierre-Louis Bossart
  Cc: alsa-devel, tiwai, gregkh, linux-kernel, Ranjani Sridharan,
	broonie, srinivas.kandagatla, jank, slawomir.blauciak,
	Sanyog Kale, Bard liao, Rander Wang

On 17-12-19, 15:03, Pierre-Louis Bossart wrote:
> Since we want an explicit support for the SoundWire Master device, add
> the definitions, following the Greybus example of a 'Host Device'.
> 
> A parent (such as the Intel audio controller) would use sdw_md_add()
> to create the device, passing a driver as a parameter. The
> sdw_md_release() would be called when put_device() is invoked by the
> parent. We use the shortcut 'md' for 'master device' to avoid very
> long function names.

I agree we should not have long name :) but md does not sound great. Can
we drop the device and use sdw_slave and sdw_master for devices and
append _driver when we are talking about drivers... 

we dont use sd for slave and imo this would gel well with existing names

> --- a/drivers/soundwire/bus_type.c
> +++ b/drivers/soundwire/bus_type.c
> @@ -66,7 +66,10 @@ int sdw_uevent(struct device *dev, struct kobj_uevent_env *env)
>  		 * callback is set to use this function for a
>  		 * different device type (e.g. Master or Monitor)
>  		 */
> -		dev_err(dev, "uevent for unknown Soundwire type\n");
> +		if (is_sdw_master_device(dev))
> +			dev_err(dev, "uevent for SoundWire Master type\n");

see below [1]:

> +static void sdw_md_release(struct device *dev)

sdw_master_release() won't be too long!

> +{
> +	struct sdw_master_device *md = to_sdw_master_device(dev);
> +
> +	kfree(md);
> +}
> +
> +struct device_type sdw_md_type = {

sdw_master_type and so on :)

> +	.name =		"soundwire_master",
> +	.release =	sdw_md_release,

[1]:
There is no uevent added here, so sdw_uevent() will never be called for
this, can you check again if you see the print you added?

>  
> +struct sdw_master_device {

we have sdw_slave, so would be better if we call this sdw_master

> +	struct device dev;
> +	int link_id;
> +	struct sdw_md_driver *driver;
> +	void *pdata;

no sdw_bus pointer and I dont see bus adding this object.. Is there no
link between bus and master device objects?

-- 
~Vinod
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [alsa-devel] [PATCH v5 09/17] soundwire: intel: remove platform devices and use 'Master Devices' instead
  2019-12-17 21:03 ` [alsa-devel] [PATCH v5 09/17] soundwire: intel: remove platform devices and use 'Master Devices' instead Pierre-Louis Bossart
@ 2019-12-27  9:08   ` Vinod Koul
  2019-12-28  0:13     ` Pierre-Louis Bossart
  2020-01-14  6:09   ` Vinod Koul
  1 sibling, 1 reply; 42+ messages in thread
From: Vinod Koul @ 2019-12-27  9:08 UTC (permalink / raw)
  To: Pierre-Louis Bossart
  Cc: alsa-devel, tiwai, gregkh, linux-kernel, Ranjani Sridharan,
	broonie, srinivas.kandagatla, jank, slawomir.blauciak,
	Sanyog Kale, Bard liao, Rander Wang

On 17-12-19, 15:03, Pierre-Louis Bossart wrote:
> Use sdw_master_device and driver instead of platform devices
> 
> To quote GregKH:
> 
> "Don't mess with a platform device unless you really have no other
> possible choice. And even then, don't do it and try to do something
> else. Platform devices are really abused, don't perpetuate it "
> 
> In addition, rather than a plain-vanilla init/exit, this patch
> provides 3 steps in the initialization (ACPI scan, probe, startup)
> which makes it easier to verify hardware support for SoundWire,
> allocate required resources as early as possible, and conversely help
> make the startup() callback lighter-weight with only hardware register
> setup.

...

> +struct sdw_md_driver intel_sdw_driver = {
> +	.probe = intel_master_probe,
> +	.startup = intel_master_startup,
> +	.remove = intel_master_remove,
>  };

...

> +extern struct sdw_md_driver intel_sdw_driver;

who uses this intel_sdw_driver? I would assumed someone would register
this with the core...

> +static struct sdw_intel_ctx
> +*sdw_intel_probe_controller(struct sdw_intel_res *res)
> +{
> +	struct sdw_intel_link_res *link;
> +	struct sdw_intel_ctx *ctx;
> +	struct acpi_device *adev;
> +	struct sdw_master_device *md;
> +	u32 link_mask;
> +	int count;
> +	int i;
> +
> +	if (!res)
> +		return NULL;
> +
> +	if (acpi_bus_get_device(res->handle, &adev))
> +		return NULL;
> +
> +	if (!res->count)
> +		return NULL;
> +
> +	count = res->count;
>  	dev_dbg(&adev->dev, "Creating %d SDW Link devices\n", count);
>  
>  	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
>  	if (!ctx)
>  		return NULL;
>  
> -	ctx->count = count;
> -	ctx->links = kcalloc(ctx->count, sizeof(*ctx->links), GFP_KERNEL);
> +	ctx->links = kcalloc(count, sizeof(*ctx->links), GFP_KERNEL);
>  	if (!ctx->links)
>  		goto link_err;
>  
> +	ctx->count = count;
> +	ctx->mmio_base = res->mmio_base;
> +	ctx->link_mask = res->link_mask;
> +	ctx->handle = res->handle;
> +
>  	link = ctx->links;
> +	link_mask = ctx->link_mask;
>  
>  	/* Create SDW Master devices */
> -	for (i = 0; i < count; i++) {
> -		if (link_mask && !(link_mask & BIT(i))) {
> -			dev_dbg(&adev->dev,
> -				"Link %d masked, will not be enabled\n", i);
> -			link++;
> +	for (i = 0; i < count; i++, link++) {
> +		if (link_mask && !(link_mask & BIT(i)))
>  			continue;
> -		}
>  
> +		md = sdw_md_add(&intel_sdw_driver,
> +				res->parent,
> +				acpi_fwnode_handle(adev),
> +				i);
> +
> +		if (IS_ERR(md)) {
> +			dev_err(&adev->dev, "Could not create link %d\n", i);
> +			goto err;
> +		}
> +		link->md = md;
> +		link->mmio_base = res->mmio_base;
>  		link->registers = res->mmio_base + SDW_LINK_BASE
> -					+ (SDW_LINK_SIZE * i);
> +			+ (SDW_LINK_SIZE * i);
>  		link->shim = res->mmio_base + SDW_SHIM_BASE;
>  		link->alh = res->mmio_base + SDW_ALH_BASE;
> -
> +		link->irq = res->irq;
>  		link->ops = res->ops;
>  		link->dev = res->dev;
>  
> -		memset(&pdevinfo, 0, sizeof(pdevinfo));
> -
> -		pdevinfo.parent = res->parent;
> -		pdevinfo.name = "int-sdw";
> -		pdevinfo.id = i;
> -		pdevinfo.fwnode = acpi_fwnode_handle(adev);
> -
> -		pdev = platform_device_register_full(&pdevinfo);
> -		if (IS_ERR(pdev)) {
> -			dev_err(&adev->dev,
> -				"platform device creation failed: %ld\n",
> -				PTR_ERR(pdev));
> -			goto pdev_err;
> -		}
> -
> -		link->pdev = pdev;
> -		link++;
> +		/* let the SoundWire master driver to its probe */
> +		md->driver->probe(md, link);

So you are invoking driver probe here.. That is typically role of driver
core to do that.. If we need that, make driver core do that for you!

That reminds me I am missing match code for master driver...

So we seem to be somewhere is middle wrt driver probing here! IIUC this
is not a full master driver, thats okay, but then it is not
completely transparent either...

I was somehow thinking that the driver will continue to be
'platform/acpi/of' driver and master device abstraction will be
handled in the core (for example see how the busses like i2c handle
this). The master device is created and used to represent but driver
probing etc is not done

Thoughts..?

-- 
~Vinod
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [alsa-devel] [PATCH v5 03/17] soundwire: rename drv_to_sdw_slave_driver macro
  2019-12-27  7:00   ` Vinod Koul
@ 2019-12-27 23:23     ` Pierre-Louis Bossart
  2019-12-28 12:03       ` Vinod Koul
  0 siblings, 1 reply; 42+ messages in thread
From: Pierre-Louis Bossart @ 2019-12-27 23:23 UTC (permalink / raw)
  To: Vinod Koul
  Cc: alsa-devel, tiwai, gregkh, linux-kernel, Ranjani Sridharan,
	broonie, srinivas.kandagatla, jank, slawomir.blauciak,
	Sanyog Kale, Bard liao, Rander Wang



On 12/27/19 1:00 AM, Vinod Koul wrote:
> On 17-12-19, 15:03, Pierre-Louis Bossart wrote:
>> Align with previous renames and shorten macro
>>
>> No functionality change
>>
>> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>> ---
>>   drivers/soundwire/bus_type.c       | 9 ++++-----
>>   include/linux/soundwire/sdw_type.h | 3 ++-
>>   2 files changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c
>> index c0585bcc8a41..2b2830b622fa 100644
>> --- a/drivers/soundwire/bus_type.c
>> +++ b/drivers/soundwire/bus_type.c
>> @@ -34,7 +34,7 @@ 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 = to_sdw_slave_device(dev);
>> -	struct sdw_driver *drv = drv_to_sdw_slave_driver(ddrv);
>> +	struct sdw_driver *drv = to_sdw_slave_driver(ddrv);
> 
> so patch 1 does:
> 
> -       struct sdw_driver *drv = drv_to_sdw_driver(dev->driver);
> +       struct sdw_driver *drv = drv_to_sdw_slave_driver(dev->driver);
> 
> and here we move drv_to_sdw_slave_driver to to_sdw_slave_driver... why
> not do this in first patch and save step1... or did i miss something??

because patch1 introduces replaces 'sdw_' by 'sdw_slave_' in several 
places, not just for drv_to_sdw_driver()

I can squash all these patches if you want to but then you'll tell me 
one step at a time...
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [alsa-devel] [PATCH v5 06/17] soundwire: add support for sdw_slave_type
  2019-12-27  7:03   ` Vinod Koul
@ 2019-12-27 23:26     ` Pierre-Louis Bossart
  2019-12-28 12:05       ` Vinod Koul
  0 siblings, 1 reply; 42+ messages in thread
From: Pierre-Louis Bossart @ 2019-12-27 23:26 UTC (permalink / raw)
  To: Vinod Koul
  Cc: alsa-devel, tiwai, gregkh, linux-kernel, Ranjani Sridharan,
	broonie, srinivas.kandagatla, jank, slawomir.blauciak,
	Sanyog Kale, Bard liao, Rander Wang


>>   static int sdw_uevent(struct device *dev, struct kobj_uevent_env *env)
>>   {
>> -	struct sdw_slave *slave = to_sdw_slave_device(dev);
>> +	struct sdw_slave *slave;
>>   	char modalias[32];
>>   
>> -	sdw_slave_modalias(slave, modalias, sizeof(modalias));
>> -
>> -	if (add_uevent_var(env, "MODALIAS=%s", modalias))
>> -		return -ENOMEM;
>> +	if (is_sdw_slave(dev)) {
>> +		slave = to_sdw_slave_device(dev);
>> +
>> +		sdw_slave_modalias(slave, modalias, sizeof(modalias));
>> +
>> +		if (add_uevent_var(env, "MODALIAS=%s", modalias))
>> +			return -ENOMEM;
>> +	} else {
>> +		/*
>> +		 * We only need to handle uevents for the Slave device
>> +		 * type. This error cannot happen unless the .uevent
>> +		 * callback is set to use this function for a
>> +		 * different device type (e.g. Master or Monitor)
>> +		 */
>> +		dev_err(dev, "uevent for unknown Soundwire type\n");
>> +		return -EINVAL;
> 
> At this point and after next patch, the above code would be a no-op, do
> we want this here, if so why?

to be future proof if someone wants to add support for a monitor, as 
explained above.
I can remove this if you don't want it.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [alsa-devel] [PATCH v5 08/17] soundwire: add initial definitions for sdw_master_device
  2019-12-27  7:14   ` Vinod Koul
@ 2019-12-27 23:38     ` Pierre-Louis Bossart
  2019-12-28 12:09       ` Vinod Koul
  0 siblings, 1 reply; 42+ messages in thread
From: Pierre-Louis Bossart @ 2019-12-27 23:38 UTC (permalink / raw)
  To: Vinod Koul
  Cc: alsa-devel, tiwai, gregkh, linux-kernel, Ranjani Sridharan,
	broonie, srinivas.kandagatla, jank, slawomir.blauciak,
	Sanyog Kale, Bard liao, Rander Wang



On 12/27/19 1:14 AM, Vinod Koul wrote:
> On 17-12-19, 15:03, Pierre-Louis Bossart wrote:
>> Since we want an explicit support for the SoundWire Master device, add
>> the definitions, following the Greybus example of a 'Host Device'.
>>
>> A parent (such as the Intel audio controller) would use sdw_md_add()
>> to create the device, passing a driver as a parameter. The
>> sdw_md_release() would be called when put_device() is invoked by the
>> parent. We use the shortcut 'md' for 'master device' to avoid very
>> long function names.
> 
> I agree we should not have long name :) but md does not sound great. Can
> we drop the device and use sdw_slave and sdw_master for devices and
> append _driver when we are talking about drivers...
> 
> we dont use sd for slave and imo this would gel well with existing names

In SoundWire parlance, both 'Slave' and 'Master' are 'Devices', so yes 
we do in the standard talk about 'Slave Devices' and 'Master Devices'.

Then we have Linux 'Devices' which can be used for both.

If we use 'sdw_slave', would we be referring to the actual physical part 
or the Linux device?

FWIW the Greybus example used 'Host Device' and 'hd' as shortcut.

> 
>> --- a/drivers/soundwire/bus_type.c
>> +++ b/drivers/soundwire/bus_type.c
>> @@ -66,7 +66,10 @@ int sdw_uevent(struct device *dev, struct kobj_uevent_env *env)
>>   		 * callback is set to use this function for a
>>   		 * different device type (e.g. Master or Monitor)
>>   		 */
>> -		dev_err(dev, "uevent for unknown Soundwire type\n");
>> +		if (is_sdw_master_device(dev))
>> +			dev_err(dev, "uevent for SoundWire Master type\n");
> 
> see below [1]:
> 
>> +static void sdw_md_release(struct device *dev)
> 
> sdw_master_release() won't be too long!

yes, but there is no such operation as 'Master Release' in SoundWire.
At least the 'md' shortcut conveys the implicit convention that this is 
a Linux device only.

I really would like to avoid overloading the standard definitions with 
the Linux ones...

> 
>> +{
>> +	struct sdw_master_device *md = to_sdw_master_device(dev);
>> +
>> +	kfree(md);
>> +}
>> +
>> +struct device_type sdw_md_type = {
> 
> sdw_master_type and so on :)
> 
>> +	.name =		"soundwire_master",
>> +	.release =	sdw_md_release,
> 
> [1]:
> There is no uevent added here, so sdw_uevent() will never be called for
> this, can you check again if you see the print you added?

as explained this is to avoid errors at a later point. I don't see any 
harm in providing error checks for a routine that can only be used for 1 
of the 3 devices defined in the standard?

>>   
>> +struct sdw_master_device {
> 
> we have sdw_slave, so would be better if we call this sdw_master
> 
>> +	struct device dev;
>> +	int link_id;
>> +	struct sdw_md_driver *driver;
>> +	void *pdata;
> 
> no sdw_bus pointer and I dont see bus adding this object.. Is there no
> link between bus and master device objects?

There is currently no bus device object, see the structure definition 
it's a pointer to a device (which leads to all sorts of issues because 
we can't use container_of).

when the master device gets added, that's where the Linux device is 
created and the pointer saved in the bus structure, with IIRC 
sdw_add_bus_master().


  	ret = sdw_add_bus_master(&sdw->cdns.bus);
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [alsa-devel] [PATCH v5 09/17] soundwire: intel: remove platform devices and use 'Master Devices' instead
  2019-12-27  9:08   ` Vinod Koul
@ 2019-12-28  0:13     ` Pierre-Louis Bossart
  2020-01-06  5:42       ` Vinod Koul
  0 siblings, 1 reply; 42+ messages in thread
From: Pierre-Louis Bossart @ 2019-12-28  0:13 UTC (permalink / raw)
  To: Vinod Koul
  Cc: alsa-devel, tiwai, gregkh, linux-kernel, Ranjani Sridharan,
	broonie, srinivas.kandagatla, jank, slawomir.blauciak,
	Sanyog Kale, Bard liao, Rander Wang



>> +extern struct sdw_md_driver intel_sdw_driver;
> 
> who uses this intel_sdw_driver? I would assumed someone would register
> this with the core...

this is a structure used by intel_init(), see the following code.

+		md = sdw_md_add(&intel_sdw_driver,
+				res->parent,
+				acpi_fwnode_handle(adev),
+				i);

that will in turn call intel_master_probe() as defined below:

+struct sdw_md_driver intel_sdw_driver = {
+	.probe = intel_master_probe,
+	.startup = intel_master_startup,
+	

>> -		link->pdev = pdev;
>> -		link++;
>> +		/* let the SoundWire master driver to its probe */
>> +		md->driver->probe(md, link);
> 
> So you are invoking driver probe here.. That is typically role of driver
> core to do that.. If we need that, make driver core do that for you!
> 
> That reminds me I am missing match code for master driver...

There is no match for the master because it doesn't have an existence in 
ACPI. There are no _ADR or HID that can be used, the only thing that 
exists is the Controller which has 4 sublinks. Each master must be added 
  by hand.

Also the SoundWire master cannot be enumerated or matched against a 
SoundWire bus, since it controls the bus itself (that would be a chicken 
and egg problem). The SoundWire master would need to be matched on a 
parent bus (which does not exist for Intel) since the hardware is 
embedded in a larger audio cluster that's visible on PCI only.

Currently for Intel platforms, the SoundWire master device is created by 
the SOF driver (via the abstraction in intel_init.c).

> So we seem to be somewhere is middle wrt driver probing here! IIUC this
> is not a full master driver, thats okay, but then it is not
> completely transparent either...
> 
> I was somehow thinking that the driver will continue to be
> 'platform/acpi/of' driver and master device abstraction will be
> handled in the core (for example see how the busses like i2c handle
> this). The master device is created and used to represent but driver
> probing etc is not done

I2C controllers are typically PCI devices or have some sort of ACPI 
description. This is not the case for SoundWire masters on Intel 
platforms, so even if I wanted to I would have no ability to implement 
any matching or parent bus registration.

Also the notion of 'probe' does not necessarily mean that the device is 
attached to a bus, we use DAI 'drivers' in ASoC and still have 
probe/remove callbacks.

And if you look at the definitions, we added additional callbacks since 
probe/remove are not enough to deal with hardware restrictions:

For Intel platforms, we have a startup() callback which is only invoked 
once the DSP is powered and the rails stable. Likewise we added an 
'autonomous_clock_stop()' callback which will be needed when the Linux 
driver hands-over control of the hardware to the DSP firmware, e.g. to 
deal with in-band wakes in D0i3.

FWIW, the implementation here follows what was suggested for Greybus 
'Host Devices' [1] [2], so it's not like I am creating any sort of 
dangerous precedent.

[1] 
https://elixir.bootlin.com/linux/latest/source/drivers/greybus/es2.c#L1275
[2] https://elixir.bootlin.com/linux/latest/source/drivers/greybus/hd.c#L124

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [alsa-devel] [PATCH v5 03/17] soundwire: rename drv_to_sdw_slave_driver macro
  2019-12-27 23:23     ` Pierre-Louis Bossart
@ 2019-12-28 12:03       ` Vinod Koul
  0 siblings, 0 replies; 42+ messages in thread
From: Vinod Koul @ 2019-12-28 12:03 UTC (permalink / raw)
  To: Pierre-Louis Bossart
  Cc: alsa-devel, tiwai, gregkh, linux-kernel, Ranjani Sridharan,
	broonie, srinivas.kandagatla, jank, slawomir.blauciak,
	Sanyog Kale, Bard liao, Rander Wang

On 27-12-19, 17:23, Pierre-Louis Bossart wrote:
> 
> 
> On 12/27/19 1:00 AM, Vinod Koul wrote:
> > On 17-12-19, 15:03, Pierre-Louis Bossart wrote:
> > > Align with previous renames and shorten macro
> > > 
> > > No functionality change
> > > 
> > > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > > ---
> > >   drivers/soundwire/bus_type.c       | 9 ++++-----
> > >   include/linux/soundwire/sdw_type.h | 3 ++-
> > >   2 files changed, 6 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c
> > > index c0585bcc8a41..2b2830b622fa 100644
> > > --- a/drivers/soundwire/bus_type.c
> > > +++ b/drivers/soundwire/bus_type.c
> > > @@ -34,7 +34,7 @@ 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 = to_sdw_slave_device(dev);
> > > -	struct sdw_driver *drv = drv_to_sdw_slave_driver(ddrv);
> > > +	struct sdw_driver *drv = to_sdw_slave_driver(ddrv);
> > 
> > so patch 1 does:
> > 
> > -       struct sdw_driver *drv = drv_to_sdw_driver(dev->driver);
> > +       struct sdw_driver *drv = drv_to_sdw_slave_driver(dev->driver);
> > 
> > and here we move drv_to_sdw_slave_driver to to_sdw_slave_driver... why
> > not do this in first patch and save step1... or did i miss something??
> 
> because patch1 introduces replaces 'sdw_' by 'sdw_slave_' in several places,
> not just for drv_to_sdw_driver()
> 
> I can squash all these patches if you want to but then you'll tell me one
> step at a time...

Yes but that does not mean we add an intermediate step just to remove it
later... So please remove the instances of drv_to_sdw_slave_driver() in
patch1 and move them to patch3 (this) and convert to
to_sdw_slave_driver()

Thanks
-- 
~Vinod
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [alsa-devel] [PATCH v5 06/17] soundwire: add support for sdw_slave_type
  2019-12-27 23:26     ` Pierre-Louis Bossart
@ 2019-12-28 12:05       ` Vinod Koul
  0 siblings, 0 replies; 42+ messages in thread
From: Vinod Koul @ 2019-12-28 12:05 UTC (permalink / raw)
  To: Pierre-Louis Bossart
  Cc: alsa-devel, tiwai, gregkh, linux-kernel, Ranjani Sridharan,
	broonie, srinivas.kandagatla, jank, slawomir.blauciak,
	Sanyog Kale, Bard liao, Rander Wang

On 27-12-19, 17:26, Pierre-Louis Bossart wrote:
> 
> > >   static int sdw_uevent(struct device *dev, struct kobj_uevent_env *env)
> > >   {
> > > -	struct sdw_slave *slave = to_sdw_slave_device(dev);
> > > +	struct sdw_slave *slave;
> > >   	char modalias[32];
> > > -	sdw_slave_modalias(slave, modalias, sizeof(modalias));
> > > -
> > > -	if (add_uevent_var(env, "MODALIAS=%s", modalias))
> > > -		return -ENOMEM;
> > > +	if (is_sdw_slave(dev)) {
> > > +		slave = to_sdw_slave_device(dev);
> > > +
> > > +		sdw_slave_modalias(slave, modalias, sizeof(modalias));
> > > +
> > > +		if (add_uevent_var(env, "MODALIAS=%s", modalias))
> > > +			return -ENOMEM;
> > > +	} else {
> > > +		/*
> > > +		 * We only need to handle uevents for the Slave device
> > > +		 * type. This error cannot happen unless the .uevent
> > > +		 * callback is set to use this function for a
> > > +		 * different device type (e.g. Master or Monitor)
> > > +		 */
> > > +		dev_err(dev, "uevent for unknown Soundwire type\n");
> > > +		return -EINVAL;
> > 
> > At this point and after next patch, the above code would be a no-op, do
> > we want this here, if so why?
> 
> to be future proof if someone wants to add support for a monitor, as
> explained above.
> I can remove this if you don't want it.

It can be added with monitor support whenever that comes. We dont like
dead code in kernel, the piece can come when future arrives :)

-- 
~Vinod
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [alsa-devel] [PATCH v5 08/17] soundwire: add initial definitions for sdw_master_device
  2019-12-27 23:38     ` Pierre-Louis Bossart
@ 2019-12-28 12:09       ` Vinod Koul
  2020-01-02 17:36         ` Pierre-Louis Bossart
  0 siblings, 1 reply; 42+ messages in thread
From: Vinod Koul @ 2019-12-28 12:09 UTC (permalink / raw)
  To: Pierre-Louis Bossart
  Cc: alsa-devel, tiwai, gregkh, linux-kernel, Ranjani Sridharan,
	broonie, srinivas.kandagatla, jank, slawomir.blauciak,
	Sanyog Kale, Bard liao, Rander Wang

On 27-12-19, 17:38, Pierre-Louis Bossart wrote:
> 
> 
> On 12/27/19 1:14 AM, Vinod Koul wrote:
> > On 17-12-19, 15:03, Pierre-Louis Bossart wrote:
> > > Since we want an explicit support for the SoundWire Master device, add
> > > the definitions, following the Greybus example of a 'Host Device'.
> > > 
> > > A parent (such as the Intel audio controller) would use sdw_md_add()
> > > to create the device, passing a driver as a parameter. The
> > > sdw_md_release() would be called when put_device() is invoked by the
> > > parent. We use the shortcut 'md' for 'master device' to avoid very
> > > long function names.
> > 
> > I agree we should not have long name :) but md does not sound great. Can
> > we drop the device and use sdw_slave and sdw_master for devices and
> > append _driver when we are talking about drivers...
> > 
> > we dont use sd for slave and imo this would gel well with existing names
> 
> In SoundWire parlance, both 'Slave' and 'Master' are 'Devices', so yes we do
> in the standard talk about 'Slave Devices' and 'Master Devices'.
> 
> Then we have Linux 'Devices' which can be used for both.
> 
> If we use 'sdw_slave', would we be referring to the actual physical part or
> the Linux device?
> 
> FWIW the Greybus example used 'Host Device' and 'hd' as shortcut.

But this messes up consistency in the naming of sdw objects. I am all for
it, if we do sd for slave and name all structs and APIs accordingly. The key
is consistency!

So it needs to be sd/md and so on or sdw_slave and sdw_master and so
on... not a mix of both

> > > --- a/drivers/soundwire/bus_type.c
> > > +++ b/drivers/soundwire/bus_type.c
> > > @@ -66,7 +66,10 @@ int sdw_uevent(struct device *dev, struct kobj_uevent_env *env)
> > >   		 * callback is set to use this function for a
> > >   		 * different device type (e.g. Master or Monitor)
> > >   		 */
> > > -		dev_err(dev, "uevent for unknown Soundwire type\n");
> > > +		if (is_sdw_master_device(dev))
> > > +			dev_err(dev, "uevent for SoundWire Master type\n");
> > 
> > see below [1]:
> > 
> > > +static void sdw_md_release(struct device *dev)
> > 
> > sdw_master_release() won't be too long!
> 
> yes, but there is no such operation as 'Master Release' in SoundWire.
> At least the 'md' shortcut conveys the implicit convention that this is a
> Linux device only.
> 
> I really would like to avoid overloading the standard definitions with the
> Linux ones...

I agree with you on not overloading but from a linux pov, we need names
which are consistent with each other...

> > > +{
> > > +	struct sdw_master_device *md = to_sdw_master_device(dev);
> > > +
> > > +	kfree(md);
> > > +}
> > > +
> > > +struct device_type sdw_md_type = {
> > 
> > sdw_master_type and so on :)
> > 
> > > +	.name =		"soundwire_master",
> > > +	.release =	sdw_md_release,
> > 
> > [1]:
> > There is no uevent added here, so sdw_uevent() will never be called for
> > this, can you check again if you see the print you added?
> 
> as explained this is to avoid errors at a later point. I don't see any harm
> in providing error checks for a routine that can only be used for 1 of the 3
> devices defined in the standard?
> 
> > > +struct sdw_master_device {
> > 
> > we have sdw_slave, so would be better if we call this sdw_master
> > 
> > > +	struct device dev;
> > > +	int link_id;
> > > +	struct sdw_md_driver *driver;
> > > +	void *pdata;
> > 
> > no sdw_bus pointer and I dont see bus adding this object.. Is there no
> > link between bus and master device objects?
> 
> There is currently no bus device object, see the structure definition it's a
> pointer to a device (which leads to all sorts of issues because we can't use
> container_of).
> 
> when the master device gets added, that's where the Linux device is created
> and the pointer saved in the bus structure, with IIRC sdw_add_bus_master().
> 
> 
>  	ret = sdw_add_bus_master(&sdw->cdns.bus);
-- 
~Vinod
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [alsa-devel] [PATCH v5 08/17] soundwire: add initial definitions for sdw_master_device
  2019-12-28 12:09       ` Vinod Koul
@ 2020-01-02 17:36         ` Pierre-Louis Bossart
  2020-01-06  5:32           ` Vinod Koul
  0 siblings, 1 reply; 42+ messages in thread
From: Pierre-Louis Bossart @ 2020-01-02 17:36 UTC (permalink / raw)
  To: Vinod Koul
  Cc: alsa-devel, tiwai, gregkh, linux-kernel, Ranjani Sridharan,
	broonie, srinivas.kandagatla, jank, slawomir.blauciak,
	Sanyog Kale, Bard liao, Rander Wang


>>>> A parent (such as the Intel audio controller) would use sdw_md_add()
>>>> to create the device, passing a driver as a parameter. The
>>>> sdw_md_release() would be called when put_device() is invoked by the
>>>> parent. We use the shortcut 'md' for 'master device' to avoid very
>>>> long function names.
>>>
>>> I agree we should not have long name :) but md does not sound great. Can
>>> we drop the device and use sdw_slave and sdw_master for devices and
>>> append _driver when we are talking about drivers...
>>>
>>> we dont use sd for slave and imo this would gel well with existing names
>>
>> In SoundWire parlance, both 'Slave' and 'Master' are 'Devices', so yes we do
>> in the standard talk about 'Slave Devices' and 'Master Devices'.
>>
>> Then we have Linux 'Devices' which can be used for both.
>>
>> If we use 'sdw_slave', would we be referring to the actual physical part or
>> the Linux device?
>>
>> FWIW the Greybus example used 'Host Device' and 'hd' as shortcut.
> 
> But this messes up consistency in the naming of sdw objects. I am all for
> it, if we do sd for slave and name all structs and APIs accordingly. The key
> is consistency!
> 
> So it needs to be sd/md and so on or sdw_slave and sdw_master and so
> on... not a mix of both


Well the problem is that the existing code took a shortcut and only 
modeled the slave part, e.g.

struct sdw_slave *slave = dev_to_sdw_dev(dev);

so now it's difficult to add 'sdw_slave_device' and 'sdw_master_device' 
without quite a few changes.

Would this work for you if we used the following names:

sdw_slave (legacy shortcut for sdw_slave_device, which could be removed 
in a a future cleanup if desired).
sdw_slave_driver
sdw_master_device
sdw_master_driver

and all the 'md' replaced by the full 'master_device'.

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [alsa-devel] [PATCH v5 08/17] soundwire: add initial definitions for sdw_master_device
  2020-01-02 17:36         ` Pierre-Louis Bossart
@ 2020-01-06  5:32           ` Vinod Koul
  0 siblings, 0 replies; 42+ messages in thread
From: Vinod Koul @ 2020-01-06  5:32 UTC (permalink / raw)
  To: Pierre-Louis Bossart
  Cc: alsa-devel, tiwai, gregkh, linux-kernel, Ranjani Sridharan,
	broonie, srinivas.kandagatla, jank, slawomir.blauciak,
	Sanyog Kale, Bard liao, Rander Wang

On 02-01-20, 11:36, Pierre-Louis Bossart wrote:

> Would this work for you if we used the following names:
> 
> sdw_slave (legacy shortcut for sdw_slave_device, which could be removed in a
> a future cleanup if desired).
> sdw_slave_driver
> sdw_master_device
> sdw_master_driver
> 
> and all the 'md' replaced by the full 'master_device'.

That does sound nicer, thanks

-- 
~Vinod
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [alsa-devel] [PATCH v5 09/17] soundwire: intel: remove platform devices and use 'Master Devices' instead
  2019-12-28  0:13     ` Pierre-Louis Bossart
@ 2020-01-06  5:42       ` Vinod Koul
  2020-01-06 14:51         ` Pierre-Louis Bossart
  0 siblings, 1 reply; 42+ messages in thread
From: Vinod Koul @ 2020-01-06  5:42 UTC (permalink / raw)
  To: Pierre-Louis Bossart
  Cc: alsa-devel, tiwai, gregkh, linux-kernel, Ranjani Sridharan,
	broonie, srinivas.kandagatla, jank, slawomir.blauciak,
	Sanyog Kale, Bard liao, Rander Wang

On 27-12-19, 18:13, Pierre-Louis Bossart wrote:
> 
> 
> > > +extern struct sdw_md_driver intel_sdw_driver;
> > 
> > who uses this intel_sdw_driver? I would assumed someone would register
> > this with the core...
> 
> this is a structure used by intel_init(), see the following code.
> 
> +		md = sdw_md_add(&intel_sdw_driver,
> +				res->parent,
> +				acpi_fwnode_handle(adev),
> +				i);
> 
> that will in turn call intel_master_probe() as defined below:
> 
> +struct sdw_md_driver intel_sdw_driver = {
> +	.probe = intel_master_probe,
> +	.startup = intel_master_startup,
> +	
> 
> > > -		link->pdev = pdev;
> > > -		link++;
> > > +		/* let the SoundWire master driver to its probe */
> > > +		md->driver->probe(md, link);
> > 
> > So you are invoking driver probe here.. That is typically role of driver
> > core to do that.. If we need that, make driver core do that for you!
> > 
> > That reminds me I am missing match code for master driver...
> 
> There is no match for the master because it doesn't have an existence in
> ACPI. There are no _ADR or HID that can be used, the only thing that exists
> is the Controller which has 4 sublinks. Each master must be added  by hand.
> 
> Also the SoundWire master cannot be enumerated or matched against a
> SoundWire bus, since it controls the bus itself (that would be a chicken and
> egg problem). The SoundWire master would need to be matched on a parent bus
> (which does not exist for Intel) since the hardware is embedded in a larger
> audio cluster that's visible on PCI only.
> 
> Currently for Intel platforms, the SoundWire master device is created by the
> SOF driver (via the abstraction in intel_init.c).

That is okay for me, the thing that is bit confusing is having a probe
etc and no match.. (more below)..

> > So we seem to be somewhere is middle wrt driver probing here! IIUC this
> > is not a full master driver, thats okay, but then it is not
> > completely transparent either...
> > 
> > I was somehow thinking that the driver will continue to be
> > 'platform/acpi/of' driver and master device abstraction will be
> > handled in the core (for example see how the busses like i2c handle
> > this). The master device is created and used to represent but driver
> > probing etc is not done
> 
> I2C controllers are typically PCI devices or have some sort of ACPI
> description. This is not the case for SoundWire masters on Intel platforms,

Well the world is not PCI/ACPI... We have controllers which are DT
described and work in same manner as a PCI device.

> so even if I wanted to I would have no ability to implement any matching or
> parent bus registration.
> 
> Also the notion of 'probe' does not necessarily mean that the device is
> attached to a bus, we use DAI 'drivers' in ASoC and still have probe/remove
> callbacks.

The "big" difference is that probe is called by core (asoc) and not by
driver onto themselves.. IMO that needs to go away.

> And if you look at the definitions, we added additional callbacks since
> probe/remove are not enough to deal with hardware restrictions:
> 
> For Intel platforms, we have a startup() callback which is only invoked once
> the DSP is powered and the rails stable. Likewise we added an
> 'autonomous_clock_stop()' callback which will be needed when the Linux
> driver hands-over control of the hardware to the DSP firmware, e.g. to deal
> with in-band wakes in D0i3.
> 
> FWIW, the implementation here follows what was suggested for Greybus 'Host
> Devices' [1] [2], so it's not like I am creating any sort of dangerous
> precedent.
> 
> [1]
> https://elixir.bootlin.com/linux/latest/source/drivers/greybus/es2.c#L1275
> [2] https://elixir.bootlin.com/linux/latest/source/drivers/greybus/hd.c#L124

And if you look closely all this work is done by core not by drivers!
Drivers _should_ never do all this, it is the job of core to do that for
you.

-- 
~Vinod
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [alsa-devel] [PATCH v5 09/17] soundwire: intel: remove platform devices and use 'Master Devices' instead
  2020-01-06  5:42       ` Vinod Koul
@ 2020-01-06 14:51         ` Pierre-Louis Bossart
  2020-01-10  6:43           ` Vinod Koul
  0 siblings, 1 reply; 42+ messages in thread
From: Pierre-Louis Bossart @ 2020-01-06 14:51 UTC (permalink / raw)
  To: Vinod Koul
  Cc: alsa-devel, tiwai, gregkh, linux-kernel, Ranjani Sridharan,
	broonie, srinivas.kandagatla, jank, slawomir.blauciak,
	Sanyog Kale, Bard liao, Rander Wang


>>>> +		/* let the SoundWire master driver to its probe */
>>>> +		md->driver->probe(md, link);
>>>
>>> So you are invoking driver probe here.. That is typically role of driver
>>> core to do that.. If we need that, make driver core do that for you!
>>>
>>> That reminds me I am missing match code for master driver...
>>
>> There is no match for the master because it doesn't have an existence in
>> ACPI. There are no _ADR or HID that can be used, the only thing that exists
>> is the Controller which has 4 sublinks. Each master must be added  by hand.
>>
>> Also the SoundWire master cannot be enumerated or matched against a
>> SoundWire bus, since it controls the bus itself (that would be a chicken and
>> egg problem). The SoundWire master would need to be matched on a parent bus
>> (which does not exist for Intel) since the hardware is embedded in a larger
>> audio cluster that's visible on PCI only.
>>
>> Currently for Intel platforms, the SoundWire master device is created by the
>> SOF driver (via the abstraction in intel_init.c).
> 
> That is okay for me, the thing that is bit confusing is having a probe
> etc and no match.. (more below)..
> 
>>> So we seem to be somewhere is middle wrt driver probing here! IIUC this
>>> is not a full master driver, thats okay, but then it is not
>>> completely transparent either...
>>>
>>> I was somehow thinking that the driver will continue to be
>>> 'platform/acpi/of' driver and master device abstraction will be
>>> handled in the core (for example see how the busses like i2c handle
>>> this). The master device is created and used to represent but driver
>>> probing etc is not done
>>
>> I2C controllers are typically PCI devices or have some sort of ACPI
>> description. This is not the case for SoundWire masters on Intel platforms,
> 
> Well the world is not PCI/ACPI... We have controllers which are DT
> described and work in same manner as a PCI device.
Both DT and PCI would use a DIFFERENT matching on the parent bus, not a 
matching provided by the SoundWire subsystem itself.

> 
>> so even if I wanted to I would have no ability to implement any matching or
>> parent bus registration.
>>
>> Also the notion of 'probe' does not necessarily mean that the device is
>> attached to a bus, we use DAI 'drivers' in ASoC and still have probe/remove
>> callbacks.
> 
> The "big" difference is that probe is called by core (asoc) and not by
> driver onto themselves.. IMO that needs to go away.

What I did is not different from what existed already with platform 
devices. They were manually created, weren't they?

> 
>> And if you look at the definitions, we added additional callbacks since
>> probe/remove are not enough to deal with hardware restrictions:
>>
>> For Intel platforms, we have a startup() callback which is only invoked once
>> the DSP is powered and the rails stable. Likewise we added an
>> 'autonomous_clock_stop()' callback which will be needed when the Linux
>> driver hands-over control of the hardware to the DSP firmware, e.g. to deal
>> with in-band wakes in D0i3.
>>
>> FWIW, the implementation here follows what was suggested for Greybus 'Host
>> Devices' [1] [2], so it's not like I am creating any sort of dangerous
>> precedent.
>>
>> [1]
>> https://elixir.bootlin.com/linux/latest/source/drivers/greybus/es2.c#L1275
>> [2] https://elixir.bootlin.com/linux/latest/source/drivers/greybus/hd.c#L124
> 
> And if you look closely all this work is done by core not by drivers!
> Drivers _should_ never do all this, it is the job of core to do that for
> you.

Please look at the code again, you have a USB probe that will manually 
call the GreyBus device creation.

static int ap_probe(struct usb_interface *interface,
		    const struct usb_device_id *id)
{
	hd = gb_hd_create(&es2_driver, &udev->dev, 	


static struct usb_driver es2_ap_driver = {
	.name =		"es2_ap_driver",
	.probe =	ap_probe, <<< code above
	.disconnect =	ap_disconnect,
	.id_table =	id_table,
	.soft_unbind =	1,
};

The master device probe suggested here is also called as part of the 
parent SOF PCI device probe, same as this USB example. I really don't 
see what your objection is, given that there is no way to deal with the 
SoundWire controller as a independent entity for Intel platforms.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [alsa-devel] [PATCH v5 09/17] soundwire: intel: remove platform devices and use 'Master Devices' instead
  2020-01-06 14:51         ` Pierre-Louis Bossart
@ 2020-01-10  6:43           ` Vinod Koul
  2020-01-10 16:08             ` Pierre-Louis Bossart
  0 siblings, 1 reply; 42+ messages in thread
From: Vinod Koul @ 2020-01-10  6:43 UTC (permalink / raw)
  To: Pierre-Louis Bossart
  Cc: alsa-devel, tiwai, gregkh, linux-kernel, Ranjani Sridharan,
	broonie, srinivas.kandagatla, jank, slawomir.blauciak,
	Sanyog Kale, Bard liao, Rander Wang

On 06-01-20, 08:51, Pierre-Louis Bossart wrote:
> 
> > > > > +		/* let the SoundWire master driver to its probe */
> > > > > +		md->driver->probe(md, link);
> > > > 
> > > > So you are invoking driver probe here.. That is typically role of driver
> > > > core to do that.. If we need that, make driver core do that for you!
> > > > 
> > > > That reminds me I am missing match code for master driver...
> > > 
> > > There is no match for the master because it doesn't have an existence in
> > > ACPI. There are no _ADR or HID that can be used, the only thing that exists
> > > is the Controller which has 4 sublinks. Each master must be added  by hand.
> > > 
> > > Also the SoundWire master cannot be enumerated or matched against a
> > > SoundWire bus, since it controls the bus itself (that would be a chicken and
> > > egg problem). The SoundWire master would need to be matched on a parent bus
> > > (which does not exist for Intel) since the hardware is embedded in a larger
> > > audio cluster that's visible on PCI only.
> > > 
> > > Currently for Intel platforms, the SoundWire master device is created by the
> > > SOF driver (via the abstraction in intel_init.c).
> > 
> > That is okay for me, the thing that is bit confusing is having a probe
> > etc and no match.. (more below)..
> > 
> > > > So we seem to be somewhere is middle wrt driver probing here! IIUC this
> > > > is not a full master driver, thats okay, but then it is not
> > > > completely transparent either...
> > > > 
> > > > I was somehow thinking that the driver will continue to be
> > > > 'platform/acpi/of' driver and master device abstraction will be
> > > > handled in the core (for example see how the busses like i2c handle
> > > > this). The master device is created and used to represent but driver
> > > > probing etc is not done
> > > 
> > > I2C controllers are typically PCI devices or have some sort of ACPI
> > > description. This is not the case for SoundWire masters on Intel platforms,
> > 
> > Well the world is not PCI/ACPI... We have controllers which are DT
> > described and work in same manner as a PCI device.
> Both DT and PCI would use a DIFFERENT matching on the parent bus, not a
> matching provided by the SoundWire subsystem itself.
> 
> > 
> > > so even if I wanted to I would have no ability to implement any matching or
> > > parent bus registration.
> > > 
> > > Also the notion of 'probe' does not necessarily mean that the device is
> > > attached to a bus, we use DAI 'drivers' in ASoC and still have probe/remove
> > > callbacks.
> > 
> > The "big" difference is that probe is called by core (asoc) and not by
> > driver onto themselves.. IMO that needs to go away.
> 
> What I did is not different from what existed already with platform devices.
> They were manually created, weren't they?

Manual creation of device based on a requirement is different, did I ask
you why you are creating device :)

I am simple asking you not to call probe in the driver. If you need
that, move it to core! We do not want these kind of things in the
drivers...

> > > And if you look at the definitions, we added additional callbacks since
> > > probe/remove are not enough to deal with hardware restrictions:
> > > 
> > > For Intel platforms, we have a startup() callback which is only invoked once
> > > the DSP is powered and the rails stable. Likewise we added an
> > > 'autonomous_clock_stop()' callback which will be needed when the Linux
> > > driver hands-over control of the hardware to the DSP firmware, e.g. to deal
> > > with in-band wakes in D0i3.
> > > 
> > > FWIW, the implementation here follows what was suggested for Greybus 'Host
> > > Devices' [1] [2], so it's not like I am creating any sort of dangerous
> > > precedent.
> > > 
> > > [1]
> > > https://elixir.bootlin.com/linux/latest/source/drivers/greybus/es2.c#L1275
> > > [2] https://elixir.bootlin.com/linux/latest/source/drivers/greybus/hd.c#L124
> > 
> > And if you look closely all this work is done by core not by drivers!
> > Drivers _should_ never do all this, it is the job of core to do that for
> > you.
> 
> Please look at the code again, you have a USB probe that will manually call
> the GreyBus device creation.
> 
> static int ap_probe(struct usb_interface *interface,
> 		    const struct usb_device_id *id)
> {
> 	hd = gb_hd_create(&es2_driver, &udev->dev, 	
> 
> 
> static struct usb_driver es2_ap_driver = {
> 	.name =		"es2_ap_driver",
> 	.probe =	ap_probe, <<< code above
> 	.disconnect =	ap_disconnect,
> 	.id_table =	id_table,
> 	.soft_unbind =	1,
> };

Look closely the driver es2 calls into greybus core hd.c and gets the
work done, subtle but a big differances in the approaches..

> The master device probe suggested here is also called as part of the parent
> SOF PCI device probe, same as this USB example. I really don't see what your
> objection is, given that there is no way to deal with the SoundWire
> controller as a independent entity for Intel platforms.

-- 
~Vinod
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [alsa-devel] [PATCH v5 09/17] soundwire: intel: remove platform devices and use 'Master Devices' instead
  2020-01-10  6:43           ` Vinod Koul
@ 2020-01-10 16:08             ` Pierre-Louis Bossart
  2020-01-13  5:18               ` Vinod Koul
  0 siblings, 1 reply; 42+ messages in thread
From: Pierre-Louis Bossart @ 2020-01-10 16:08 UTC (permalink / raw)
  To: Vinod Koul
  Cc: alsa-devel, tiwai, gregkh, linux-kernel, Ranjani Sridharan,
	broonie, srinivas.kandagatla, jank, slawomir.blauciak,
	Sanyog Kale, Bard liao, Rander Wang


>>> The "big" difference is that probe is called by core (asoc) and not by
>>> driver onto themselves.. IMO that needs to go away.
>>
>> What I did is not different from what existed already with platform devices.
>> They were manually created, weren't they?
> 
> Manual creation of device based on a requirement is different, did I ask
> you why you are creating device :)
> 
> I am simple asking you not to call probe in the driver. If you need
> that, move it to core! We do not want these kind of things in the
> drivers...

What core are you talking about?

The SOF intel driver needs to create a device, which will then be bound 
with a SoundWire master driver.

What I am doing is no different from what your team did with 
platform_register_device, I am really lost on what you are asking.


>>>> FWIW, the implementation here follows what was suggested for Greybus 'Host
>>>> Devices' [1] [2], so it's not like I am creating any sort of dangerous
>>>> precedent.
>>>>
>>>> [1]
>>>> https://elixir.bootlin.com/linux/latest/source/drivers/greybus/es2.c#L1275
>>>> [2] https://elixir.bootlin.com/linux/latest/source/drivers/greybus/hd.c#L124
>>>
>>> And if you look closely all this work is done by core not by drivers!
>>> Drivers _should_ never do all this, it is the job of core to do that for
>>> you.
>>
>> Please look at the code again, you have a USB probe that will manually call
>> the GreyBus device creation.
>>
>> static int ap_probe(struct usb_interface *interface,
>> 		    const struct usb_device_id *id)
>> {
>> 	hd = gb_hd_create(&es2_driver, &udev->dev, 	
>>
>>
>> static struct usb_driver es2_ap_driver = {
>> 	.name =		"es2_ap_driver",
>> 	.probe =	ap_probe, <<< code above
>> 	.disconnect =	ap_disconnect,
>> 	.id_table =	id_table,
>> 	.soft_unbind =	1,
>> };
> 
> Look closely the driver es2 calls into greybus core hd.c and gets the
> work done, subtle but a big differances in the approaches..

I am sorry, I have absolutely no idea what you are referring to.

The code I copy/pasted here makes no call to the greybus core, it's 
ap_probe -> gb_hd_create. No core involved. If I am mistaken, please 
show me what I got wrong.

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [alsa-devel] [PATCH v5 09/17] soundwire: intel: remove platform devices and use 'Master Devices' instead
  2020-01-10 16:08             ` Pierre-Louis Bossart
@ 2020-01-13  5:18               ` Vinod Koul
  2020-01-13 15:22                 ` Pierre-Louis Bossart
  0 siblings, 1 reply; 42+ messages in thread
From: Vinod Koul @ 2020-01-13  5:18 UTC (permalink / raw)
  To: Pierre-Louis Bossart
  Cc: alsa-devel, tiwai, gregkh, linux-kernel, Ranjani Sridharan,
	broonie, srinivas.kandagatla, jank, slawomir.blauciak,
	Sanyog Kale, Bard liao, Rander Wang

On 10-01-20, 10:08, Pierre-Louis Bossart wrote:
> 
> > > > The "big" difference is that probe is called by core (asoc) and not by
> > > > driver onto themselves.. IMO that needs to go away.
> > > 
> > > What I did is not different from what existed already with platform devices.
> > > They were manually created, weren't they?
> > 
> > Manual creation of device based on a requirement is different, did I ask
> > you why you are creating device :)
> > 
> > I am simple asking you not to call probe in the driver. If you need
> > that, move it to core! We do not want these kind of things in the
> > drivers...
> 
> What core are you talking about?

soundwire core ofcourse! IMO All that which goes into soundwire-bus-objs is
considered as soundwire core part and rest are drivers intel, qc, so on!
> 
> The SOF intel driver needs to create a device, which will then be bound with
> a SoundWire master driver.
> 
> What I am doing is no different from what your team did with
> platform_register_device, I am really lost on what you are asking.

Again repeating myself, you call an API to do that is absolutely fine,
but we don't do that in drivers or open code these things

> > > > > FWIW, the implementation here follows what was suggested for Greybus 'Host
> > > > > Devices' [1] [2], so it's not like I am creating any sort of dangerous
> > > > > precedent.
> > > > > 
> > > > > [1]
> > > > > https://elixir.bootlin.com/linux/latest/source/drivers/greybus/es2.c#L1275
> > > > > [2] https://elixir.bootlin.com/linux/latest/source/drivers/greybus/hd.c#L124
> > > > 
> > > > And if you look closely all this work is done by core not by drivers!
> > > > Drivers _should_ never do all this, it is the job of core to do that for
> > > > you.
> > > 
> > > Please look at the code again, you have a USB probe that will manually call
> > > the GreyBus device creation.
> > > 
> > > static int ap_probe(struct usb_interface *interface,
> > > 		    const struct usb_device_id *id)
> > > {
> > > 	hd = gb_hd_create(&es2_driver, &udev->dev, 	
> > > 
> > > 
> > > static struct usb_driver es2_ap_driver = {
> > > 	.name =		"es2_ap_driver",
> > > 	.probe =	ap_probe, <<< code above
> > > 	.disconnect =	ap_disconnect,
> > > 	.id_table =	id_table,
> > > 	.soft_unbind =	1,
> > > };
> > 
> > Look closely the driver es2 calls into greybus core hd.c and gets the
> > work done, subtle but a big differances in the approaches..
> 
> I am sorry, I have absolutely no idea what you are referring to.
> 
> The code I copy/pasted here makes no call to the greybus core, it's ap_probe
> -> gb_hd_create. No core involved. If I am mistaken, please show me what I
> got wrong.

1. es2_ap_driver is host controller driver

2. gb_hd_create() is an API provided by greybus core!

es2 driver doesn't open code creation like you are doing in intel driver,
it doesn't call probe on its own, greybus does that

This is very common pattern in linux kernel subsytems, drivers dont do
these things, the respective subsystem core does that... see about es2
driver and implementation of gb_hd_create(). See callers of
platform_register_device() and its implementation.

I don't know how else I can explain this to you, is something wrong in
how I conveyed this info or you... or something else, I dont know!!!

-- 
~Vinod
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [alsa-devel] [PATCH v5 09/17] soundwire: intel: remove platform devices and use 'Master Devices' instead
  2020-01-13  5:18               ` Vinod Koul
@ 2020-01-13 15:22                 ` Pierre-Louis Bossart
  0 siblings, 0 replies; 42+ messages in thread
From: Pierre-Louis Bossart @ 2020-01-13 15:22 UTC (permalink / raw)
  To: Vinod Koul
  Cc: alsa-devel, tiwai, gregkh, linux-kernel, Ranjani Sridharan,
	broonie, srinivas.kandagatla, jank, slawomir.blauciak,
	Sanyog Kale, Bard liao, Rander Wang



On 1/12/20 11:18 PM, Vinod Koul wrote:
> On 10-01-20, 10:08, Pierre-Louis Bossart wrote:
>>
>>>>> The "big" difference is that probe is called by core (asoc) and not by
>>>>> driver onto themselves.. IMO that needs to go away.
>>>>
>>>> What I did is not different from what existed already with platform devices.
>>>> They were manually created, weren't they?
>>>
>>> Manual creation of device based on a requirement is different, did I ask
>>> you why you are creating device :)
>>>
>>> I am simple asking you not to call probe in the driver. If you need
>>> that, move it to core! We do not want these kind of things in the
>>> drivers...
>>
>> What core are you talking about?
> 
> soundwire core ofcourse! IMO All that which goes into soundwire-bus-objs is
> considered as soundwire core part and rest are drivers intel, qc, so on!
This master code was added to the bus:   v
                                          v
soundwire-bus-objs := bus_type.o bus.o master.o slave.o mipi_disco.o 
stream.o
obj-$(CONFIG_SOUNDWIRE) += soundwire-bus.o

and the API is also part of the sdw.h include file. That seems to meet 
exactly what you describe above, no?

git grep sdw_master_device_add (reformatted output)

drivers/soundwire/intel_init.c:
md = sdw_master_device_add(&intel_sdw_driver,

drivers/soundwire/master.c:
*sdw_master_device_add(struct sdw_master_driver *driver,

drivers/soundwire/master.c:
EXPORT_SYMBOL_GPL(sdw_master_device_add);

include/linux/soundwire/sdw.h:
*sdw_master_device_add(struct sdw_master_driver *driver,

So, what exactly is the issue?

We are not 'calling the probe in the [Intel] driver' as you state it, we 
use a SoundWire core API which in turn will create a device. The device 
core takes care of calling the probe, see the master.c code which is NOT 
Intel-specific.

>>
>> The SOF intel driver needs to create a device, which will then be bound with
>> a SoundWire master driver.
>>
>> What I am doing is no different from what your team did with
>> platform_register_device, I am really lost on what you are asking.
> 
> Again repeating myself, you call an API to do that is absolutely fine,
> but we don't do that in drivers or open code these things
That is still quite unclear, what 'open-coding' are you referring to?

I am starting to wonder if you missed the addition of the master 
functionality in the previous patch:

[PATCH v5 08/17] soundwire: add initial definitions for sdw_master_device

What this patch 9 does is call the core-defined API and implement the 
intel-specific master driver.

> 
>>>>>> FWIW, the implementation here follows what was suggested for Greybus 'Host
>>>>>> Devices' [1] [2], so it's not like I am creating any sort of dangerous
>>>>>> precedent.
>>>>>>
>>>>>> [1]
>>>>>> https://elixir.bootlin.com/linux/latest/source/drivers/greybus/es2.c#L1275
>>>>>> [2] https://elixir.bootlin.com/linux/latest/source/drivers/greybus/hd.c#L124
>>>>>
>>>>> And if you look closely all this work is done by core not by drivers!
>>>>> Drivers _should_ never do all this, it is the job of core to do that for
>>>>> you.
>>>>
>>>> Please look at the code again, you have a USB probe that will manually call
>>>> the GreyBus device creation.
>>>>
>>>> static int ap_probe(struct usb_interface *interface,
>>>> 		    const struct usb_device_id *id)
>>>> {
>>>> 	hd = gb_hd_create(&es2_driver, &udev->dev, 	
>>>>
>>>>
>>>> static struct usb_driver es2_ap_driver = {
>>>> 	.name =		"es2_ap_driver",
>>>> 	.probe =	ap_probe, <<< code above
>>>> 	.disconnect =	ap_disconnect,
>>>> 	.id_table =	id_table,
>>>> 	.soft_unbind =	1,
>>>> };
>>>
>>> Look closely the driver es2 calls into greybus core hd.c and gets the
>>> work done, subtle but a big differances in the approaches..
>>
>> I am sorry, I have absolutely no idea what you are referring to.
>>
>> The code I copy/pasted here makes no call to the greybus core, it's ap_probe
>> -> gb_hd_create. No core involved. If I am mistaken, please show me what I
>> got wrong.
> 
> 1. es2_ap_driver is host controller driver
> 
> 2. gb_hd_create() is an API provided by greybus core!

same in my code...

> 
> es2 driver doesn't open code creation like you are doing in intel driver,
> it doesn't call probe on its own, greybus does that
> 
> This is very common pattern in linux kernel subsytems, drivers dont do
> these things, the respective subsystem core does that... see about es2
> driver and implementation of gb_hd_create(). See callers of
> platform_register_device() and its implementation.
> 
> I don't know how else I can explain this to you, is something wrong in
> how I conveyed this info or you... or something else, I dont know!!!
the new 'master' functionality is part of the bus code, so please 
clarify what you see as problematic for the partition.

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [alsa-devel] [PATCH v5 09/17] soundwire: intel: remove platform devices and use 'Master Devices' instead
  2019-12-17 21:03 ` [alsa-devel] [PATCH v5 09/17] soundwire: intel: remove platform devices and use 'Master Devices' instead Pierre-Louis Bossart
  2019-12-27  9:08   ` Vinod Koul
@ 2020-01-14  6:09   ` Vinod Koul
  2020-01-14 16:01     ` Pierre-Louis Bossart
  1 sibling, 1 reply; 42+ messages in thread
From: Vinod Koul @ 2020-01-14  6:09 UTC (permalink / raw)
  To: Pierre-Louis Bossart
  Cc: alsa-devel, tiwai, gregkh, linux-kernel, Ranjani Sridharan,
	broonie, srinivas.kandagatla, jank, slawomir.blauciak,
	Sanyog Kale, Bard liao, Rander Wang

On 13-01-20, 09:22, Pierre-Louis Bossart wrote:
> 
> 
> On 1/12/20 11:18 PM, Vinod Koul wrote:
> > On 10-01-20, 10:08, Pierre-Louis Bossart wrote:
> > > 
> > > > > > The "big" difference is that probe is called by core (asoc) and not by
> > > > > > driver onto themselves.. IMO that needs to go away.
> > > > > 
> > > > > What I did is not different from what existed already with platform devices.
> > > > > They were manually created, weren't they?
> > > > 
> > > > Manual creation of device based on a requirement is different, did I ask
> > > > you why you are creating device :)
> > > > 
> > > > I am simple asking you not to call probe in the driver. If you need
> > > > that, move it to core! We do not want these kind of things in the
> > > > drivers...
> > > 
> > > What core are you talking about?
> > 
> > soundwire core ofcourse! IMO All that which goes into soundwire-bus-objs is
> > considered as soundwire core part and rest are drivers intel, qc, so on!
> This master code was added to the bus:   v
>                                          v
> soundwire-bus-objs := bus_type.o bus.o master.o slave.o mipi_disco.o
> stream.o
> obj-$(CONFIG_SOUNDWIRE) += soundwire-bus.o
> 
> and the API is also part of the sdw.h include file. That seems to meet
> exactly what you describe above, no?
> 
> git grep sdw_master_device_add (reformatted output)
> 
> drivers/soundwire/intel_init.c:
> md = sdw_master_device_add(&intel_sdw_driver,
> 
> drivers/soundwire/master.c:
> *sdw_master_device_add(struct sdw_master_driver *driver,
> 
> drivers/soundwire/master.c:
> EXPORT_SYMBOL_GPL(sdw_master_device_add);
> 
> include/linux/soundwire/sdw.h:
> *sdw_master_device_add(struct sdw_master_driver *driver,
> 
> So, what exactly is the issue?
> 
> We are not 'calling the probe in the [Intel] driver' as you state it, we use
> a SoundWire core API which in turn will create a device. The device core
> takes care of calling the probe, see the master.c code which is NOT
> Intel-specific.
> 
> > > 
> > > The SOF intel driver needs to create a device, which will then be bound with
> > > a SoundWire master driver.
> > > 
> > > What I am doing is no different from what your team did with
> > > platform_register_device, I am really lost on what you are asking.
> > 
> > Again repeating myself, you call an API to do that is absolutely fine,
> > but we don't do that in drivers or open code these things
> That is still quite unclear, what 'open-coding' are you referring to?
> 
> I am starting to wonder if you missed the addition of the master
> functionality in the previous patch:
> 
> [PATCH v5 08/17] soundwire: add initial definitions for sdw_master_device
> 
> What this patch 9 does is call the core-defined API and implement the
> intel-specific master driver.

Yes and no, it does call things introduced in patch 8, I questioned calling probe!
See below!

> > > > > > > FWIW, the implementation here follows what was suggested for Greybus 'Host
> > > > > > > Devices' [1] [2], so it's not like I am creating any sort of dangerous
> > > > > > > precedent.
> > > > > > > 
> > > > > > > [1]
> > > > > > > https://elixir.bootlin.com/linux/latest/source/drivers/greybus/es2.c#L1275
> > > > > > > [2] https://elixir.bootlin.com/linux/latest/source/drivers/greybus/hd.c#L124
> > > > > > 
> > > > > > And if you look closely all this work is done by core not by drivers!
> > > > > > Drivers _should_ never do all this, it is the job of core to do that for
> > > > > > you.
> > > > > 
> > > > > Please look at the code again, you have a USB probe that will manually call
> > > > > the GreyBus device creation.
> > > > > 
> > > > > static int ap_probe(struct usb_interface *interface,
> > > > > 		    const struct usb_device_id *id)
> > > > > {
> > > > > 	hd = gb_hd_create(&es2_driver, &udev->dev, 	
> > > > > 
> > > > > 
> > > > > static struct usb_driver es2_ap_driver = {
> > > > > 	.name =		"es2_ap_driver",
> > > > > 	.probe =	ap_probe, <<< code above
> > > > > 	.disconnect =	ap_disconnect,
> > > > > 	.id_table =	id_table,
> > > > > 	.soft_unbind =	1,
> > > > > };
> > > > 
> > > > Look closely the driver es2 calls into greybus core hd.c and gets the
> > > > work done, subtle but a big differances in the approaches..
> > > 
> > > I am sorry, I have absolutely no idea what you are referring to.
> > > 
> > > The code I copy/pasted here makes no call to the greybus core, it's ap_probe
> > > -> gb_hd_create. No core involved. If I am mistaken, please show me what I
> > > got wrong.
> > 
> > 1. es2_ap_driver is host controller driver
> > 
> > 2. gb_hd_create() is an API provided by greybus core!
> 
> same in my code...
> 
> > 
> > es2 driver doesn't open code creation like you are doing in intel driver,
> > it doesn't call probe on its own, greybus does that
> > 
> > This is very common pattern in linux kernel subsytems, drivers dont do
> > these things, the respective subsystem core does that... see about es2
> > driver and implementation of gb_hd_create(). See callers of
> > platform_register_device() and its implementation.
> > 
> > I don't know how else I can explain this to you, is something wrong in
> > how I conveyed this info or you... or something else, I dont know!!!
> the new 'master' functionality is part of the bus code, so please clarify
> what you see as problematic for the partition.

I am quoting the code in patch, which i pointed in my first reply!

On 17-12-19, 15:03, Pierre-Louis Bossart wrote:

> diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c
> index 4b769409f6f8..42f7ae034bea 100644
> --- a/drivers/soundwire/intel_init.c

This is intel specific file...

> +++ b/drivers/soundwire/intel_init.c

snip ...

> +static struct sdw_intel_ctx
> +*sdw_intel_probe_controller(struct sdw_intel_res *res)

this is intel driver, intel function!

> -
> -		link->pdev = pdev;
> -		link++;
> +		/* let the SoundWire master driver to its probe */
> +		md->driver->probe(md, link);
                            ^^^^^^
which does this... calls a probe()!

And my first reply was:

> > +             /* let the SoundWire master driver to its probe */
> > +             md->driver->probe(md, link);
> 
> So you are invoking driver probe here.. That is typically role of driver
> core to do that.. If we need that, make driver core do that for you!

I rest my case!

-- 
~Vinod
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [alsa-devel] [PATCH v5 09/17] soundwire: intel: remove platform devices and use 'Master Devices' instead
  2020-01-14  6:09   ` Vinod Koul
@ 2020-01-14 16:01     ` Pierre-Louis Bossart
  2020-01-18  7:12       ` Vinod Koul
  0 siblings, 1 reply; 42+ messages in thread
From: Pierre-Louis Bossart @ 2020-01-14 16:01 UTC (permalink / raw)
  To: Vinod Koul
  Cc: alsa-devel, tiwai, gregkh, linux-kernel, Ranjani Sridharan,
	broonie, srinivas.kandagatla, jank, slawomir.blauciak,
	Sanyog Kale, Bard liao, Rander Wang


> I am quoting the code in patch, which i pointed in my first reply!
> 
> On 17-12-19, 15:03, Pierre-Louis Bossart wrote:
> 
>> diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c
>> index 4b769409f6f8..42f7ae034bea 100644
>> --- a/drivers/soundwire/intel_init.c
> 
> This is intel specific file...
> 
>> +++ b/drivers/soundwire/intel_init.c
> 
> snip ...
> 
>> +static struct sdw_intel_ctx
>> +*sdw_intel_probe_controller(struct sdw_intel_res *res)
> 
> this is intel driver, intel function!
> 
>> -
>> -		link->pdev = pdev;
>> -		link++;
>> +		/* let the SoundWire master driver to its probe */
>> +		md->driver->probe(md, link);
>                              ^^^^^^
> which does this... calls a probe()!
> 
> And my first reply was:
> 
>>> +             /* let the SoundWire master driver to its probe */
>>> +             md->driver->probe(md, link);
>>
>> So you are invoking driver probe here.. That is typically role of driver
>> core to do that.. If we need that, make driver core do that for you!
> 
> I rest my case!

I think you are too focused on the probe case and not realizing the 
extensions suggested by this patchset. A "driver" is not limited to 
'probe' and 'remove' cases.

As mentioned since mid-September, there is a need for an initialization 
of software/kernel structures (which I called probe but should have been 
called init really), and a second step where the hardware is actually 
configured - after all power rail dependencies are under control.

Can you please look at the .startup callback and let me know how a 
'driver core' would handle this?

To the best of my knowledge, there is no .startup in any device model 
functionality, so the only thing I could do to avoid a direct call is 
add a wrapper to avoid a direct call, e.g.

static inline sdw_master_device_startup(struct sdw_master_device *md)
{
     if (md && md->driver && md->driver->startup)
         md->driver->startup(md);
}


_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [alsa-devel] [PATCH v5 09/17] soundwire: intel: remove platform devices and use 'Master Devices' instead
  2020-01-14 16:01     ` Pierre-Louis Bossart
@ 2020-01-18  7:12       ` Vinod Koul
  2020-01-21 17:31         ` Pierre-Louis Bossart
  0 siblings, 1 reply; 42+ messages in thread
From: Vinod Koul @ 2020-01-18  7:12 UTC (permalink / raw)
  To: Pierre-Louis Bossart
  Cc: alsa-devel, tiwai, gregkh, linux-kernel, Ranjani Sridharan,
	broonie, srinivas.kandagatla, jank, slawomir.blauciak,
	Sanyog Kale, Bard liao, Rander Wang

On 14-01-20, 10:01, Pierre-Louis Bossart wrote:
> 
> > I am quoting the code in patch, which i pointed in my first reply!
> > 
> > On 17-12-19, 15:03, Pierre-Louis Bossart wrote:
> > 
> > > diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c
> > > index 4b769409f6f8..42f7ae034bea 100644
> > > --- a/drivers/soundwire/intel_init.c
> > 
> > This is intel specific file...
> > 
> > > +++ b/drivers/soundwire/intel_init.c
> > 
> > snip ...
> > 
> > > +static struct sdw_intel_ctx
> > > +*sdw_intel_probe_controller(struct sdw_intel_res *res)
> > 
> > this is intel driver, intel function!
> > 
> > > -
> > > -		link->pdev = pdev;
> > > -		link++;
> > > +		/* let the SoundWire master driver to its probe */
> > > +		md->driver->probe(md, link);
> >                              ^^^^^^
> > which does this... calls a probe()!
> > 
> > And my first reply was:
> > 
> > > > +             /* let the SoundWire master driver to its probe */
> > > > +             md->driver->probe(md, link);
> > > 
> > > So you are invoking driver probe here.. That is typically role of driver
> > > core to do that.. If we need that, make driver core do that for you!
> > 
> > I rest my case!
> 
> I think you are too focused on the probe case and not realizing the
> extensions suggested by this patchset. A "driver" is not limited to 'probe'
> and 'remove' cases.
> 
> As mentioned since mid-September, there is a need for an initialization of
> software/kernel structures (which I called probe but should have been called
> init really), and a second step where the hardware is actually configured -

I find it amusing that a person whom i admired for strict use of terms
can get this differently!

A rename away from probe will certainly be very helpful as
you would also agree that terms 'probe' and 'remove' have a very
special meaning in kernel, so let us avoid these

-- 
~Vinod
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [alsa-devel] [PATCH v5 09/17] soundwire: intel: remove platform devices and use 'Master Devices' instead
  2020-01-18  7:12       ` Vinod Koul
@ 2020-01-21 17:31         ` Pierre-Louis Bossart
  0 siblings, 0 replies; 42+ messages in thread
From: Pierre-Louis Bossart @ 2020-01-21 17:31 UTC (permalink / raw)
  To: Vinod Koul
  Cc: alsa-devel, tiwai, gregkh, linux-kernel, Ranjani Sridharan,
	broonie, srinivas.kandagatla, jank, slawomir.blauciak,
	Sanyog Kale, Bard liao, Rander Wang


> A rename away from probe will certainly be very helpful as
> you would also agree that terms 'probe' and 'remove' have a very
> special meaning in kernel, so let us avoid these

ok, so would the following be ok with you?

/**
  * struct sdw_md_driver - SoundWire 'Master Device' driver
  *
  * @init: allocations and initializations (hardware may not be enabled yet)
  * @startup: initialization handled after the hardware is enabled, all
  * clock/power dependencies are available
  * @shutdown: cleanups before hardware is disabled (optional)
  * @exit: free all remaining resources
  * @autonomous_clock_stop_enable: enable/disable driver control while
  * in clock-stop mode, typically in always-on/D0ix modes. When the driver
  * yields control, another entity in the system (typically firmware
  * running on an always-on microprocessor) is responsible to tracking
  * Slave-initiated wakes
  */
struct sdw_md_driver {
	int (*init)(struct sdw_master_device *md, void *link_ctx);
	int (*startup)(struct sdw_master_device *md);
	int (*shutdown)(struct sdw_master_device *md);
	int (*exit)(struct sdw_master_device *md);
	int (*autonomous_clock_stop_enable)(struct sdw_master_device *md,
					    bool state);
};
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

end of thread, back to index

Thread overview: 42+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-17 21:02 [alsa-devel] [PATCH v5 00/17] soundwire: intel: implement new ASoC interfaces Pierre-Louis Bossart
2019-12-17 21:02 ` [alsa-devel] [PATCH v5 01/17] soundwire: renames to prepare support for master drivers/devices Pierre-Louis Bossart
2019-12-17 21:02 ` [alsa-devel] [PATCH v5 02/17] soundwire: rename dev_to_sdw_dev macro Pierre-Louis Bossart
2019-12-27  6:54   ` Vinod Koul
2019-12-17 21:03 ` [alsa-devel] [PATCH v5 03/17] soundwire: rename drv_to_sdw_slave_driver macro Pierre-Louis Bossart
2019-12-27  7:00   ` Vinod Koul
2019-12-27 23:23     ` Pierre-Louis Bossart
2019-12-28 12:03       ` Vinod Koul
2019-12-17 21:03 ` [alsa-devel] [PATCH v5 04/17] soundwire: bus_type: rename sdw_drv_ to sdw_slave_drv Pierre-Louis Bossart
2019-12-17 21:03 ` [alsa-devel] [PATCH v5 05/17] soundwire: intel: rename res field as link_res Pierre-Louis Bossart
2019-12-17 21:03 ` [alsa-devel] [PATCH v5 06/17] soundwire: add support for sdw_slave_type Pierre-Louis Bossart
2019-12-27  7:03   ` Vinod Koul
2019-12-27 23:26     ` Pierre-Louis Bossart
2019-12-28 12:05       ` Vinod Koul
2019-12-17 21:03 ` [alsa-devel] [PATCH v5 07/17] soundwire: slave: move uevent handling to slave device level Pierre-Louis Bossart
2019-12-17 21:03 ` [alsa-devel] [PATCH v5 08/17] soundwire: add initial definitions for sdw_master_device Pierre-Louis Bossart
2019-12-27  7:14   ` Vinod Koul
2019-12-27 23:38     ` Pierre-Louis Bossart
2019-12-28 12:09       ` Vinod Koul
2020-01-02 17:36         ` Pierre-Louis Bossart
2020-01-06  5:32           ` Vinod Koul
2019-12-17 21:03 ` [alsa-devel] [PATCH v5 09/17] soundwire: intel: remove platform devices and use 'Master Devices' instead Pierre-Louis Bossart
2019-12-27  9:08   ` Vinod Koul
2019-12-28  0:13     ` Pierre-Louis Bossart
2020-01-06  5:42       ` Vinod Koul
2020-01-06 14:51         ` Pierre-Louis Bossart
2020-01-10  6:43           ` Vinod Koul
2020-01-10 16:08             ` Pierre-Louis Bossart
2020-01-13  5:18               ` Vinod Koul
2020-01-13 15:22                 ` Pierre-Louis Bossart
2020-01-14  6:09   ` Vinod Koul
2020-01-14 16:01     ` Pierre-Louis Bossart
2020-01-18  7:12       ` Vinod Koul
2020-01-21 17:31         ` Pierre-Louis Bossart
2019-12-17 21:03 ` [alsa-devel] [PATCH v5 10/17] soundwire: register master device driver Pierre-Louis Bossart
2019-12-17 21:03 ` [alsa-devel] [PATCH v5 11/17] soundwire: intel: add prepare support in sdw dai driver Pierre-Louis Bossart
2019-12-17 21:03 ` [alsa-devel] [PATCH v5 12/17] soundwire: intel: add trigger " Pierre-Louis Bossart
2019-12-17 21:03 ` [alsa-devel] [PATCH v5 13/17] soundwire: intel: add sdw_stream_setup helper for .startup callback Pierre-Louis Bossart
2019-12-17 21:03 ` [alsa-devel] [PATCH v5 14/17] soundwire: intel: free all resources on hw_free() Pierre-Louis Bossart
2019-12-17 21:03 ` [alsa-devel] [PATCH v5 15/17] soundwire: intel_init: add implementation of sdw_intel_enable_irq() Pierre-Louis Bossart
2019-12-17 21:03 ` [alsa-devel] [PATCH v5 16/17] soundwire: intel_init: use EXPORT_SYMBOL_NS Pierre-Louis Bossart
2019-12-17 21:03 ` [alsa-devel] [PATCH v5 17/17] soundwire: intel: " Pierre-Louis Bossart

Alsa-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/alsa-devel/0 alsa-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 alsa-devel alsa-devel/ https://lore.kernel.org/alsa-devel \
		alsa-devel@alsa-project.org
	public-inbox-index alsa-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.alsa-project.alsa-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git