[alsa-devel] [PATCH 00/14] soundwire: intel: implement new ASoC interfaces

[alsa-devel] [PATCH 00/14] soundwire: intel: implement new ASoC interfaces

[Pierre-Louis Bossart]

this patchset applies on top of the series "[PATCH 0/4] soundwire:
update ASoC interfaces" and previously submitted cleanups "[PATCH v3
0/5] soundwire: intel/cadence: better initialization"

The changes are essentially a removal of the platform devices 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.

Bard Liao (1):
  soundwire: add device driver to sdw_md_driver

Pierre-Louis Bossart (10):
  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: add initial definitions for sdw_master_device
  soundwire: intel: remove platform devices and provide new interface
  soundwire: intel: free all resources on hw_free()
  soundwire: intel_init: add implementation of sdw_intel_enable_irq()

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_type.c       |  60 +++---
 drivers/soundwire/intel.c          | 280 ++++++++++++++++++++++-----
 drivers/soundwire/intel.h          |   8 +-
 drivers/soundwire/intel_init.c     | 300 ++++++++++++++++++++++-------
 drivers/soundwire/master.c         |  64 ++++++
 drivers/soundwire/slave.c          |   9 +-
 include/linux/soundwire/sdw.h      |  39 +++-
 include/linux/soundwire/sdw_type.h |  34 +++-
 11 files changed, 642 insertions(+), 160 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

[alsa-devel] [PATCH 01/14] soundwire: renames to prepare support for master drivers/devices

[Pierre-Louis Bossart]

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

[alsa-devel] [PATCH 02/14] soundwire: rename dev_to_sdw_dev macro

[Pierre-Louis Bossart]

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

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 0c4e59dfaca3..d6e5a0e42819 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -570,7 +570,8 @@ struct sdw_slave {
 	struct completion enumeration_complete;
 };
 
-#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

Re: [alsa-devel] [PATCH 02/14] soundwire: rename dev_to_sdw_dev macro

[Vinod Koul]

On 23-10-19, 16:28, Pierre-Louis Bossart wrote:
> Since we want to introduce master devices, rename macro so that we
> have consistency between slave and master device access, following the
> Grey Bus example.
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  drivers/base/regmap/regmap-sdw.c |  4 ++--

This needs Mark's ACK

>  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 0c4e59dfaca3..d6e5a0e42819 100644
> --- a/include/linux/soundwire/sdw.h
> +++ b/include/linux/soundwire/sdw.h
> @@ -570,7 +570,8 @@ struct sdw_slave {
>  	struct completion enumeration_complete;
>  };
>  
> -#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

[alsa-devel] [PATCH 03/14] soundwire: rename drv_to_sdw_slave_driver macro

[Pierre-Louis Bossart]

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

[alsa-devel] [PATCH 04/14] soundwire: bus_type: rename sdw_drv_ to sdw_slave_drv

[Pierre-Louis Bossart]

Before we add master driver support, make sure there is no ambiguity
and no occirrences 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

Re: [alsa-devel] [PATCH 04/14] soundwire: bus_type: rename sdw_drv_ to sdw_slave_drv

[Vinod Koul]

On 23-10-19, 16:28, Pierre-Louis Bossart wrote:
> Before we add master driver support, make sure there is no ambiguity
> and no occirrences of sdw_drv_ functions.
        ^^^^^^^^^^^
typo

> 
> 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

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

Re: [alsa-devel] [PATCH 04/14] soundwire: bus_type: rename sdw_drv_ to sdw_slave_drv

[Pierre-Louis Bossart]

On 11/3/19 12:30 AM, Vinod Koul wrote:
> On 23-10-19, 16:28, Pierre-Louis Bossart wrote:
>> Before we add master driver support, make sure there is no ambiguity
>> and no occirrences of sdw_drv_ functions.
>          ^^^^^^^^^^^
> typo

Ack, will fix.

> 
>>
>> 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

[alsa-devel] [PATCH 05/14] soundwire: intel: rename res field as link_res

[Pierre-Louis Bossart]

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

[alsa-devel] [PATCH 06/14] soundwire: add support for sdw_slave_type

[Pierre-Louis Bossart]

Currently the bus does not have any explicit support for master
devices.  Add explicit support for sdw_slave_type, so that in
follow-up patches we can add support for the sdw_md_type (md==Master
Device), following the Grey Bus example.

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

diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c
index 9a0fd3ee1014..5df095f4e12f 100644
--- a/drivers/soundwire/bus_type.c
+++ b/drivers/soundwire/bus_type.c
@@ -49,9 +49,16 @@ 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];
 
+	if (is_sdw_slave(dev)) {
+		slave = to_sdw_slave_device(dev);
+	} else {
+		dev_warn(dev, "uevent for unknown Soundwire type\n");
+		return -EINVAL;
+	}
+
 	sdw_slave_modalias(slave, modalias, sizeof(modalias));
 
 	if (add_uevent_var(env, "MODALIAS=%s", modalias))
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

Re: [alsa-devel] [PATCH 06/14] soundwire: add support for sdw_slave_type

[Vinod Koul]

On 23-10-19, 16:28, Pierre-Louis Bossart wrote:
> Currently the bus does not have any explicit support for master
> devices.  Add explicit support for sdw_slave_type, so that in
> follow-up patches we can add support for the sdw_md_type (md==Master
> Device), following the Grey Bus example.
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  drivers/soundwire/bus_type.c       | 9 ++++++++-
>  drivers/soundwire/slave.c          | 7 ++++++-
>  include/linux/soundwire/sdw_type.h | 6 ++++++
>  3 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c
> index 9a0fd3ee1014..5df095f4e12f 100644
> --- a/drivers/soundwire/bus_type.c
> +++ b/drivers/soundwire/bus_type.c
> @@ -49,9 +49,16 @@ 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];
>  
> +	if (is_sdw_slave(dev)) {
> +		slave = to_sdw_slave_device(dev);
> +	} else {
> +		dev_warn(dev, "uevent for unknown Soundwire type\n");
> +		return -EINVAL;

when is the case where this would be triggered?

> +	}
> +
>  	sdw_slave_modalias(slave, modalias, sizeof(modalias));
>  
>  	if (add_uevent_var(env, "MODALIAS=%s", modalias))
> 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

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

Re: [alsa-devel] [PATCH 06/14] soundwire: add support for sdw_slave_type

[Pierre-Louis Bossart]

On 11/3/19 12:32 AM, Vinod Koul wrote:
> On 23-10-19, 16:28, Pierre-Louis Bossart wrote:
>> Currently the bus does not have any explicit support for master
>> devices.  Add explicit support for sdw_slave_type, so that in
>> follow-up patches we can add support for the sdw_md_type (md==Master
>> Device), following the Grey Bus example.
>>
>> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>> ---
>>   drivers/soundwire/bus_type.c       | 9 ++++++++-
>>   drivers/soundwire/slave.c          | 7 ++++++-
>>   include/linux/soundwire/sdw_type.h | 6 ++++++
>>   3 files changed, 20 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c
>> index 9a0fd3ee1014..5df095f4e12f 100644
>> --- a/drivers/soundwire/bus_type.c
>> +++ b/drivers/soundwire/bus_type.c
>> @@ -49,9 +49,16 @@ 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];
>>   
>> +	if (is_sdw_slave(dev)) {
>> +		slave = to_sdw_slave_device(dev);
>> +	} else {
>> +		dev_warn(dev, "uevent for unknown Soundwire type\n");
>> +		return -EINVAL;
> 
> when is the case where this would be triggered?

this is in preparation for the master case, where you will have 2 types 
of devices, so need to check if the type is correct.

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

Re: [alsa-devel] [PATCH 06/14] soundwire: add support for sdw_slave_type

[Cezary Rojewski]

On 2019-10-23 23:28, Pierre-Louis Bossart wrote:
> diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c
> index 9a0fd3ee1014..5df095f4e12f 100644
> --- a/drivers/soundwire/bus_type.c
> +++ b/drivers/soundwire/bus_type.c
> @@ -49,9 +49,16 @@ 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];
>   
> +	if (is_sdw_slave(dev)) {
> +		slave = to_sdw_slave_device(dev);
> +	} else {
> +		dev_warn(dev, "uevent for unknown Soundwire type\n");
> +		return -EINVAL;
> +	}
> +
>   	sdw_slave_modalias(slave, modalias, sizeof(modalias));
>   
>   	if (add_uevent_var(env, "MODALIAS=%s", modalias))

Positive evaluation of is_sdw_slave() check is required for function to 
continue, thus you might as well do:

if (!is_sdw_slave(dev)) {
	dev_warn();
	return -EINVAL;
}

slave = to_sdw_slave_device(dev);
(...)

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

Re: [alsa-devel] [PATCH 06/14] soundwire: add support for sdw_slave_type

[Pierre-Louis Bossart]

>> @@ -49,9 +49,16 @@ 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];
>> +    if (is_sdw_slave(dev)) {
>> +        slave = to_sdw_slave_device(dev);
>> +    } else {
>> +        dev_warn(dev, "uevent for unknown Soundwire type\n");
>> +        return -EINVAL;
>> +    }
>> +
>>       sdw_slave_modalias(slave, modalias, sizeof(modalias));
>>       if (add_uevent_var(env, "MODALIAS=%s", modalias))
> 
> Positive evaluation of is_sdw_slave() check is required for function to 
> continue, thus you might as well do:
> 
> if (!is_sdw_slave(dev)) {
>      dev_warn();
>      return -EINVAL;
> }
> 
> slave = to_sdw_slave_device(dev);

see the following patch 7, it becomes a 2-case test (slave and master).
I am all for optimizations but wait until the actual changes are added...
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[alsa-devel] [PATCH 07/14] soundwire: add initial definitions for sdw_master_device

[Pierre-Louis Bossart]

Since we want an explicit support for the SoundWire Master device, add
the definitions, following the Grey Bus example.

Open: do we need to set a variable when dealing with the master uevent?

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/Makefile         |  2 +-
 drivers/soundwire/bus_type.c       | 16 +++++---
 drivers/soundwire/master.c         | 62 ++++++++++++++++++++++++++++++
 include/linux/soundwire/sdw.h      | 35 +++++++++++++++++
 include/linux/soundwire/sdw_type.h |  9 +++++
 5 files changed, 117 insertions(+), 7 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 5df095f4e12f..cf33f63773f0 100644
--- a/drivers/soundwire/bus_type.c
+++ b/drivers/soundwire/bus_type.c
@@ -49,21 +49,25 @@ 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_master_device *md;
 	struct sdw_slave *slave;
 	char modalias[32];
 
-	if (is_sdw_slave(dev)) {
+	if (is_sdw_md(dev)) {
+		md = to_sdw_master_device(dev);
+		/* TODO: do we need to call add_uevent_var() ? */
+	} else 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 {
 		dev_warn(dev, "uevent for unknown Soundwire type\n");
 		return -EINVAL;
 	}
 
-	sdw_slave_modalias(slave, modalias, sizeof(modalias));
-
-	if (add_uevent_var(env, "MODALIAS=%s", modalias))
-		return -ENOMEM;
-
 	return 0;
 }
 
diff --git a/drivers/soundwire/master.c b/drivers/soundwire/master.c
new file mode 100644
index 000000000000..6210098c892b
--- /dev/null
+++ b/drivers/soundwire/master.c
@@ -0,0 +1,62 @@
+// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
+// 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 md;
+}
+EXPORT_SYMBOL(sdw_md_add);
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index d6e5a0e42819..6f8b6a0cbcb7 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -573,6 +573,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; /* core does not touch */
+};
+
+#define to_sdw_master_device(d)	\
+	container_of(d, struct sdw_master_device, dev)
+
 struct sdw_driver {
 	const char *name;
 
@@ -587,6 +597,26 @@ struct sdw_driver {
 	struct device_driver driver;
 };
 
+struct sdw_md_driver {
+	/* initializations and allocations */
+	int (*probe)(struct sdw_master_device *md, void *link_ctx);
+	/* hardware enablement, all clock/power dependencies are available */
+	int (*startup)(struct sdw_master_device *md);
+	/* hardware disabled */
+	int (*shutdown)(struct sdw_master_device *md);
+	/* free all resources */
+	int (*remove)(struct sdw_master_device *md);
+	/*
+	 * 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
+	 */
+	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) }
@@ -776,6 +806,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..463d6d018d56 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_md(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

Re: [alsa-devel] [PATCH 07/14] soundwire: add initial definitions for sdw_master_device

[Vinod Koul]

On 23-10-19, 16:28, Pierre-Louis Bossart wrote:
> Since we want an explicit support for the SoundWire Master device, add
> the definitions, following the Grey Bus example.
> 
> Open: do we need to set a variable when dealing with the master uevent?

I dont think we want that or we need that!

And to prevent that rather than adding a variable, can you please
modify the device_type and use separate ones for master_device and
slave_device

> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  drivers/soundwire/Makefile         |  2 +-
>  drivers/soundwire/bus_type.c       | 16 +++++---
>  drivers/soundwire/master.c         | 62 ++++++++++++++++++++++++++++++
>  include/linux/soundwire/sdw.h      | 35 +++++++++++++++++
>  include/linux/soundwire/sdw_type.h |  9 +++++
>  5 files changed, 117 insertions(+), 7 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 5df095f4e12f..cf33f63773f0 100644
> --- a/drivers/soundwire/bus_type.c
> +++ b/drivers/soundwire/bus_type.c
> @@ -49,21 +49,25 @@ 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_master_device *md;
>  	struct sdw_slave *slave;
>  	char modalias[32];
>  
> -	if (is_sdw_slave(dev)) {
> +	if (is_sdw_md(dev)) {
> +		md = to_sdw_master_device(dev);

md seems unused?

> +		/* TODO: do we need to call add_uevent_var() ? */
> +	} else 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 {
>  		dev_warn(dev, "uevent for unknown Soundwire type\n");
>  		return -EINVAL;
>  	}
>  
> -	sdw_slave_modalias(slave, modalias, sizeof(modalias));
> -
> -	if (add_uevent_var(env, "MODALIAS=%s", modalias))
> -		return -ENOMEM;
> -
>  	return 0;
>  }
>  
> diff --git a/drivers/soundwire/master.c b/drivers/soundwire/master.c
> new file mode 100644
> index 000000000000..6210098c892b
> --- /dev/null
> +++ b/drivers/soundwire/master.c
> @@ -0,0 +1,62 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
> +// 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 md;
> +}
> +EXPORT_SYMBOL(sdw_md_add);
> diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
> index d6e5a0e42819..6f8b6a0cbcb7 100644
> --- a/include/linux/soundwire/sdw.h
> +++ b/include/linux/soundwire/sdw.h
> @@ -573,6 +573,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; /* core does not touch */

what is the use for this, also please add kernel-doc style comments for
core structs

> +};
> +
> +#define to_sdw_master_device(d)	\
> +	container_of(d, struct sdw_master_device, dev)
> +
>  struct sdw_driver {
>  	const char *name;
>  
> @@ -587,6 +597,26 @@ struct sdw_driver {
>  	struct device_driver driver;
>  };
>  
> +struct sdw_md_driver {
> +	/* initializations and allocations */
> +	int (*probe)(struct sdw_master_device *md, void *link_ctx);
> +	/* hardware enablement, all clock/power dependencies are available */
> +	int (*startup)(struct sdw_master_device *md);
> +	/* hardware disabled */
> +	int (*shutdown)(struct sdw_master_device *md);
> +	/* free all resources */
> +	int (*remove)(struct sdw_master_device *md);
> +	/*
> +	 * 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
> +	 */
> +	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) }
> @@ -776,6 +806,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..463d6d018d56 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_md(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

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

Re: [alsa-devel] [PATCH 07/14] soundwire: add initial definitions for sdw_master_device

[Pierre-Louis Bossart]

On 11/3/19 1:30 AM, Vinod Koul wrote:
> On 23-10-19, 16:28, Pierre-Louis Bossart wrote:
>> Since we want an explicit support for the SoundWire Master device, add
>> the definitions, following the Grey Bus example.
>>
>> Open: do we need to set a variable when dealing with the master uevent?
> 
> I dont think we want that or we need that!

In GreyBus there are events and variables set, not sure what they were 
used for. The code works without setting an event, but we'd need to make 
a conscious design decision, and I am not too sure what usersace would 
use the informatio for.

> 
> And to prevent that rather than adding a variable, can you please
> modify the device_type and use separate ones for master_device and
> slave_device

sorry, I don't get the comment. There is only already a different device 
type


struct bus_type sdw_bus_type = {
	.name = "soundwire",
	.match = sdw_bus_match,
	.uevent = sdw_uevent,
};

struct device_type sdw_slave_type = {
	.name =		"sdw_slave",
	.release =	sdw_slave_release,
};

struct device_type sdw_md_type = {
	.name =		"soundwire_master",
	.release =	sdw_md_release,
};

> 
>>
>> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>> ---
>>   drivers/soundwire/Makefile         |  2 +-
>>   drivers/soundwire/bus_type.c       | 16 +++++---
>>   drivers/soundwire/master.c         | 62 ++++++++++++++++++++++++++++++
>>   include/linux/soundwire/sdw.h      | 35 +++++++++++++++++
>>   include/linux/soundwire/sdw_type.h |  9 +++++
>>   5 files changed, 117 insertions(+), 7 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 5df095f4e12f..cf33f63773f0 100644
>> --- a/drivers/soundwire/bus_type.c
>> +++ b/drivers/soundwire/bus_type.c
>> @@ -49,21 +49,25 @@ 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_master_device *md;
>>   	struct sdw_slave *slave;
>>   	char modalias[32];
>>   
>> -	if (is_sdw_slave(dev)) {
>> +	if (is_sdw_md(dev)) {
>> +		md = to_sdw_master_device(dev);
> 
> md seems unused?

well, yes, its use depends on whether we need to call add_event_var() as 
asked below

> 
>> +		/* TODO: do we need to call add_uevent_var() ? */
>> +	} else 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 {
>>   		dev_warn(dev, "uevent for unknown Soundwire type\n");
>>   		return -EINVAL;
>>   	}
>>   
>> -	sdw_slave_modalias(slave, modalias, sizeof(modalias));
>> -
>> -	if (add_uevent_var(env, "MODALIAS=%s", modalias))
>> -		return -ENOMEM;
>> -
>>   	return 0;
>>   }
>>   
>> diff --git a/drivers/soundwire/master.c b/drivers/soundwire/master.c
>> new file mode 100644
>> index 000000000000..6210098c892b
>> --- /dev/null
>> +++ b/drivers/soundwire/master.c
>> @@ -0,0 +1,62 @@
>> +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
>> +// 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 md;
>> +}
>> +EXPORT_SYMBOL(sdw_md_add);
>> diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
>> index d6e5a0e42819..6f8b6a0cbcb7 100644
>> --- a/include/linux/soundwire/sdw.h
>> +++ b/include/linux/soundwire/sdw.h
>> @@ -573,6 +573,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; /* core does not touch */
> 
> what is the use for this, also please add kernel-doc style comments for
> core structs
> 
>> +};
>> +
>> +#define to_sdw_master_device(d)	\
>> +	container_of(d, struct sdw_master_device, dev)
>> +
>>   struct sdw_driver {
>>   	const char *name;
>>   
>> @@ -587,6 +597,26 @@ struct sdw_driver {
>>   	struct device_driver driver;
>>   };
>>   
>> +struct sdw_md_driver {
>> +	/* initializations and allocations */
>> +	int (*probe)(struct sdw_master_device *md, void *link_ctx);
>> +	/* hardware enablement, all clock/power dependencies are available */
>> +	int (*startup)(struct sdw_master_device *md);
>> +	/* hardware disabled */
>> +	int (*shutdown)(struct sdw_master_device *md);
>> +	/* free all resources */
>> +	int (*remove)(struct sdw_master_device *md);
>> +	/*
>> +	 * 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
>> +	 */
>> +	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) }
>> @@ -776,6 +806,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..463d6d018d56 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_md(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

Re: [alsa-devel] [PATCH 07/14] soundwire: add initial definitions for sdw_master_device

[Vinod Koul]

On 04-11-19, 08:42, Pierre-Louis Bossart wrote:
> 
> 
> On 11/3/19 1:30 AM, Vinod Koul wrote:
> > On 23-10-19, 16:28, Pierre-Louis Bossart wrote:
> > > Since we want an explicit support for the SoundWire Master device, add
> > > the definitions, following the Grey Bus example.
> > > 
> > > Open: do we need to set a variable when dealing with the master uevent?
> > 
> > I dont think we want that or we need that!
> 
> In GreyBus there are events and variables set, not sure what they were used
> for. The code works without setting an event, but we'd need to make a
> conscious design decision, and I am not too sure what usersace would use the
> informatio for.
> 
> > 
> > And to prevent that rather than adding a variable, can you please
> > modify the device_type and use separate ones for master_device and
> > slave_device
> 
> sorry, I don't get the comment. There is only already a different device
> type
> 
> 
> struct bus_type sdw_bus_type = {
> 	.name = "soundwire",
> 	.match = sdw_bus_match,
> 	.uevent = sdw_uevent,

We can remove this

> };
> 
> struct device_type sdw_slave_type = {
> 	.name =		"sdw_slave",
> 	.release =	sdw_slave_release,

Add here:

        uevent = sdw_uevent,

> };
> 
> struct device_type sdw_md_type = {
> 	.name =		"soundwire_master",
> 	.release =	sdw_md_release,
> };

And not have here!

Problem solved!

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

Re: [alsa-devel] [PATCH 07/14] soundwire: add initial definitions for sdw_master_device

[Pierre-Louis Bossart]

On 11/7/19 10:04 PM, Vinod Koul wrote:
> On 04-11-19, 08:42, Pierre-Louis Bossart wrote:
>>
>>
>> On 11/3/19 1:30 AM, Vinod Koul wrote:
>>> On 23-10-19, 16:28, Pierre-Louis Bossart wrote:
>>>> Since we want an explicit support for the SoundWire Master device, add
>>>> the definitions, following the Grey Bus example.
>>>>
>>>> Open: do we need to set a variable when dealing with the master uevent?
>>>
>>> I dont think we want that or we need that!
>>
>> In GreyBus there are events and variables set, not sure what they were used
>> for. The code works without setting an event, but we'd need to make a
>> conscious design decision, and I am not too sure what usersace would use the
>> informatio for.
>>
>>>
>>> And to prevent that rather than adding a variable, can you please
>>> modify the device_type and use separate ones for master_device and
>>> slave_device
>>
>> sorry, I don't get the comment. There is only already a different device
>> type
>>
>>
>> struct bus_type sdw_bus_type = {
>> 	.name = "soundwire",
>> 	.match = sdw_bus_match,
>> 	.uevent = sdw_uevent,
> 
> We can remove this
> 
>> };
>>
>> struct device_type sdw_slave_type = {
>> 	.name =		"sdw_slave",
>> 	.release =	sdw_slave_release,
> 
> Add here:
> 
>          uevent = sdw_uevent,
> 
>> };
>>
>> struct device_type sdw_md_type = {
>> 	.name =		"soundwire_master",
>> 	.release =	sdw_md_release,
>> };
> 
> And not have here!
> 
> Problem solved!

I will give it a try but I don't know what the 'problem' was.
The code works as is, and btw you are commenting on the wrong version of 
the series, v2 was sent yesterday.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[alsa-devel] [PATCH 08/14] soundwire: intel: remove platform devices and provide new interface

[Pierre-Louis Bossart]

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 make it easier to verify support and allocate required resources
as early as possible, and conversely help make the startup
lighter-weight with only hardware register setup.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/intel.c      |  92 ++++++-----
 drivers/soundwire/intel.h      |   8 +-
 drivers/soundwire/intel_init.c | 276 ++++++++++++++++++++++++---------
 3 files changed, 268 insertions(+), 108 deletions(-)

diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index 64f97bb1a135..ba3bc410d816 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,18 @@ static int intel_remove(struct platform_device *pdev)
 	}
 	sdw_delete_bus_master(&sdw->cdns.bus);
 
+	md->dev.driver = NULL;
+	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 6bc167c83b47..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,37 +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_init);
+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

[alsa-devel] [PATCH 09/14] soundwire: add device driver to sdw_md_driver

[Pierre-Louis Bossart]

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

Setting an device driver is necessary for ASoC to register DAI
components.

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     | 4 ++++
 drivers/soundwire/master.c    | 2 ++
 include/linux/soundwire/sdw.h | 1 +
 3 files changed, 7 insertions(+)

diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index ba3bc410d816..492684ba08c3 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -1059,6 +1059,10 @@ static int intel_master_remove(struct sdw_master_device *md)
 }
 
 struct sdw_md_driver intel_sdw_driver = {
+	.driver = {
+		.name = "intel-sdw",
+		.owner = THIS_MODULE,
+	},
 	.probe = intel_master_probe,
 	.startup = intel_master_startup,
 	.remove = intel_master_remove,
diff --git a/drivers/soundwire/master.c b/drivers/soundwire/master.c
index 6210098c892b..d23111d43fd6 100644
--- a/drivers/soundwire/master.c
+++ b/drivers/soundwire/master.c
@@ -57,6 +57,8 @@ struct sdw_master_device *sdw_md_add(struct sdw_md_driver *driver,
 		put_device(&md->dev);
 	}
 
+	md->dev.driver = &driver->driver;
+
 	return md;
 }
 EXPORT_SYMBOL(sdw_md_add);
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index 6f8b6a0cbcb7..d756e41c9466 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -615,6 +615,7 @@ struct sdw_md_driver {
 	 */
 	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

[alsa-devel] [PATCH 10/14] soundwire: intel: add prepare support in sdw dai driver

[Pierre-Louis Bossart]

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

It gets sdw runtime information from dai to prepare stream.

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 492684ba08c3..c50aebda7f4d 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -697,6 +697,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)
 {
@@ -743,6 +758,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,
@@ -750,6 +766,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

Re: [alsa-devel] [PATCH 10/14] soundwire: intel: add prepare support in sdw dai driver

[Cezary Rojewski]

On 2019-10-23 23:28, Pierre-Louis Bossart wrote:
> From: Rander Wang <rander.wang@linux.intel.com>
> 
> It gets sdw runtime information from dai to prepare stream.
> 
> Signed-off-by: Rander Wang <rander.wang@linux.intel.com>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

While the patch looks good, the commit message is questionable. You may 
simply state why it is added only just now. Judging from the commit 
title, it has been added to make the sdw dai driver interface complete.

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

Re: [alsa-devel] [PATCH 10/14] soundwire: intel: add prepare support in sdw dai driver

[Pierre-Louis Bossart]

On 11/4/19 1:45 PM, Cezary Rojewski wrote:
> On 2019-10-23 23:28, Pierre-Louis Bossart wrote:
>> From: Rander Wang <rander.wang@linux.intel.com>
>>
>> It gets sdw runtime information from dai to prepare stream.
>>
>> Signed-off-by: Rander Wang <rander.wang@linux.intel.com>
>> Signed-off-by: Pierre-Louis Bossart 
>> <pierre-louis.bossart@linux.intel.com>
> 
> While the patch looks good, the commit message is questionable. You may 
> simply state why it is added only just now. Judging from the commit 
> title, it has been added to make the sdw dai driver interface complete.

The commit message is not great but it's not wrong either...
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH 10/14] soundwire: intel: add prepare support in sdw dai driver

[Vinod Koul]

On 04-11-19, 15:31, Pierre-Louis Bossart wrote:
> 
> 
> On 11/4/19 1:45 PM, Cezary Rojewski wrote:
> > On 2019-10-23 23:28, Pierre-Louis Bossart wrote:
> > > From: Rander Wang <rander.wang@linux.intel.com>
> > > 
> > > It gets sdw runtime information from dai to prepare stream.
> > > 
> > > Signed-off-by: Rander Wang <rander.wang@linux.intel.com>
> > > Signed-off-by: Pierre-Louis Bossart
> > > <pierre-louis.bossart@linux.intel.com>
> > 
> > While the patch looks good, the commit message is questionable. You may
> > simply state why it is added only just now. Judging from the commit
> > title, it has been added to make the sdw dai driver interface complete.
> 
> The commit message is not great but it's not wrong either...

And it doesn't harm to elaborate and explain things rather than have
reviewer play detective!

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

[alsa-devel] [PATCH 11/14] soundwire: intel: add trigger support in sdw dai driver

[Pierre-Louis Bossart]

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

Sdw 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 c50aebda7f4d..862dde9a9b86 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -712,6 +712,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)
 {
@@ -759,6 +796,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,
@@ -767,6 +805,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

[alsa-devel] [PATCH 12/14] soundwire: intel: add sdw_stream_setup helper for .startup callback

[Pierre-Louis Bossart]

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 862dde9a9b86..af24fa048add 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -615,6 +615,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)
@@ -794,6 +857,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,
@@ -803,6 +867,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

[alsa-devel] [PATCH 13/14] soundwire: intel: free all resources on hw_free()

[Pierre-Louis Bossart]

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 af24fa048add..cad1c0b64ee3 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -548,6 +548,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
  */
@@ -816,6 +835,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;
 
@@ -823,12 +843,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

Re: [alsa-devel] [PATCH 13/14] soundwire: intel: free all resources on hw_free()

[Cezary Rojewski]

On 2019-10-23 23:28, Pierre-Louis Bossart wrote:
> 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 af24fa048add..cad1c0b64ee3 100644
> --- a/drivers/soundwire/intel.c
> +++ b/drivers/soundwire/intel.c
> @@ -548,6 +548,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)

Can res->dev even be null?

> +		return res->ops->free_stream(res->dev,
> +					     &free_data);
> +
> +	return 0;
> +}
> +
>   /*
>    * bank switch routines
>    */
> @@ -816,6 +835,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;
>   
> @@ -823,12 +843,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;
> +	}
> +

I understand that you want to be transparent to caller with failure 
reasons via dev_err/_warn. However, sdw_deprepare_stream already dumps 
all the logs we need. The same applies for most of the other calls (and 
not just in this patch..).

Do we really need to be that verbose? Maybe just agree on caller -or- 
subject being the source for the messaging, align existing usages and 
thus preventing further duplication?

Not forcing anything, just asking for your opinion on this.

>   	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;
>   }

Given the structure of this function, shouldn't the generic flow be 
handled by upper layer i.e. part of sdw core?. Apart from 
intel_free_stream, the rest looks pretty generic to me and this sole 
call could easily be extracted into ops.

>   
>   static void intel_shutdown(struct snd_pcm_substream *substream,
> 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH 13/14] soundwire: intel: free all resources on hw_free()

[Pierre-Louis Bossart]

On 11/4/19 2:08 PM, Cezary Rojewski wrote:
> On 2019-10-23 23:28, Pierre-Louis Bossart wrote:
>> 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 af24fa048add..cad1c0b64ee3 100644
>> --- a/drivers/soundwire/intel.c
>> +++ b/drivers/soundwire/intel.c
>> @@ -548,6 +548,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)
> 
> Can res->dev even be null?

in error cases yes. this 'res' structure is setup by the DSP driver, and 
it could be wrong or not set.

Note that in the previous solution we tested the res->arg pointer, we 
did find a case where we could oops here.

> 
>> +        return res->ops->free_stream(res->dev,
>> +                         &free_data);
>> +
>> +    return 0;
>> +}
>> +
>>   /*
>>    * bank switch routines
>>    */
>> @@ -816,6 +835,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;
>> @@ -823,12 +843,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;
>> +    }
>> +
> 
> I understand that you want to be transparent to caller with failure 
> reasons via dev_err/_warn. However, sdw_deprepare_stream already dumps 
> all the logs we need. The same applies for most of the other calls (and 
> not just in this patch..).
> 
> Do we really need to be that verbose? Maybe just agree on caller -or- 
> subject being the source for the messaging, align existing usages and 
> thus preventing further duplication?
> 
> Not forcing anything, just asking for your opinion on this.

the sdw_prepare/deprepare_stream calls provide error logs, but they are 
not mapped to specific devices/dais (pr_err vs. dev_dbg). I found it was 
easier to check for which dai the error was reported.

We are also in the middle of integration with new hardware/boards, and 
erring on the side of more traces will help everyone involved. We can 
revisit later which ones are strictly necessary.

> 
>>       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;
>>   }
> 
> Given the structure of this function, shouldn't the generic flow be 
> handled by upper layer i.e. part of sdw core?. Apart from 
> intel_free_stream, the rest looks pretty generic to me and this sole 
> call could easily be extracted into ops.

The mapping between DAI and stream is not necessarily the same for all 
platforms, we just had this discussion while reviewing the QCOM patches 
last week. whether you release the resources in .hw_free() or 
.shutdown() is also platform dependent.

Also this code will change when we support the multi-CPU dais, more work 
will be handled at the dailink level than at the dai.
We can (and will) refactor at a later point.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH 13/14] soundwire: intel: free all resources on hw_free()

[Vinod Koul]

On 04-11-19, 15:46, Pierre-Louis Bossart wrote:
> On 11/4/19 2:08 PM, Cezary Rojewski wrote:
> > On 2019-10-23 23:28, Pierre-Louis Bossart wrote:
> > > @@ -816,6 +835,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;
> > > @@ -823,12 +843,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;
> > > +    }
> > > +
> > 
> > I understand that you want to be transparent to caller with failure
> > reasons via dev_err/_warn. However, sdw_deprepare_stream already dumps
> > all the logs we need. The same applies for most of the other calls (and
> > not just in this patch..).

I think this is a valid concern! In linux we do not do that, for example
we ask people to not log errors on kmalloc as it will be logged on
failures so drivers do not need to do that.

> > Do we really need to be that verbose? Maybe just agree on caller -or-
> > subject being the source for the messaging, align existing usages and
> > thus preventing further duplication?
> > 
> > Not forcing anything, just asking for your opinion on this.
> 
> the sdw_prepare/deprepare_stream calls provide error logs, but they are not
> mapped to specific devices/dais (pr_err vs. dev_dbg). I found it was easier
> to check for which dai the error was reported.

Well in that case we should fix pr_err, there are only 17 instances of
these in core, and few of them are justified in core (no dev pointer)
and 11 in stream (few of them valid (no stream pointer) but rest can be
converted to use dev_err! Even then they print stream name, so checking
error is not justified argument!

> We are also in the middle of integration with new hardware/boards, and
> erring on the side of more traces will help everyone involved. We can
> revisit later which ones are strictly necessary.

Naah you are having duplicate logs, it does *not* help in debug seems
1000 line logs and few lines conveying duplicate info, I would rather
have each line unique so that I dont have to skip duplicate ones while
debugging!

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

Re: [alsa-devel] [PATCH 13/14] soundwire: intel: free all resources on hw_free()

[Pierre-Louis Bossart]

On 11/7/19 10:14 PM, Vinod Koul wrote:
> On 04-11-19, 15:46, Pierre-Louis Bossart wrote:
>> On 11/4/19 2:08 PM, Cezary Rojewski wrote:
>>> On 2019-10-23 23:28, Pierre-Louis Bossart wrote:
>>>> @@ -816,6 +835,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;
>>>> @@ -823,12 +843,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;
>>>> +    }
>>>> +
>>>
>>> I understand that you want to be transparent to caller with failure
>>> reasons via dev_err/_warn. However, sdw_deprepare_stream already dumps
>>> all the logs we need. The same applies for most of the other calls (and
>>> not just in this patch..).
> 
> I think this is a valid concern! In linux we do not do that, for example
> we ask people to not log errors on kmalloc as it will be logged on
> failures so drivers do not need to do that.
> 
>>> Do we really need to be that verbose? Maybe just agree on caller -or-
>>> subject being the source for the messaging, align existing usages and
>>> thus preventing further duplication?
>>>
>>> Not forcing anything, just asking for your opinion on this.
>>
>> the sdw_prepare/deprepare_stream calls provide error logs, but they are not
>> mapped to specific devices/dais (pr_err vs. dev_dbg). I found it was easier
>> to check for which dai the error was reported.
> 
> Well in that case we should fix pr_err, there are only 17 instances of
> these in core, and few of them are justified in core (no dev pointer)
> and 11 in stream (few of them valid (no stream pointer) but rest can be
> converted to use dev_err! Even then they print stream name, so checking
> error is not justified argument!

the stream has no notion of device, it can be made of multiple devices, 
so which one would you choose?

> 
>> We are also in the middle of integration with new hardware/boards, and
>> erring on the side of more traces will help everyone involved. We can
>> revisit later which ones are strictly necessary.
> 
> Naah you are having duplicate logs, it does *not* help in debug seems
> 1000 line logs and few lines conveying duplicate info, I would rather
> have each line unique so that I dont have to skip duplicate ones while
> debugging!

They are not all duplicates.

Again, if I remove the logs in stream.c, then I do lose valuable 
information on bad state machines transitions, etc. An error code is not 
enough to reconstruct the issues from intel.c

If I remove the logs in intel.c, I can't know which dai had an error and 
what caused it.

seriously, these are all details, you have over 50 patches to review 
with a complete rework of this subsystem and we argue about dev_err 
verbosity?
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[alsa-devel] [PATCH 14/14] soundwire: intel_init: add implementation of sdw_intel_enable_irq()

[Pierre-Louis Bossart]

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 42f7ae034bea..14ffe9ce2929 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