[alsa-devel] [PATCH 0/4] soundwire: update ASoC interfaces

[alsa-devel] [PATCH 0/4] soundwire: update ASoC interfaces

[Pierre-Louis Bossart]

We need new fields in existing structures to
a) deal with race conditions on codec probe/enumeration
b) allow for multi-step ACPI scan/probe/startup on Intel plaforms

To avoid conflicts between ASoC and Soundwire trees, these 4 patches
are provided out-of-order, before the functionality enabled in these
header files is added in follow-up patch series which can be applied
separately in the ASoC and Soundwire trees (of course after Vinod and
Mark sync-up so that these patches are present in both trees).

Pierre-Louis Bossart (3):
  soundwire: sdw_slave: add new fields to track probe status
  soundwire: add enumeration_complete structure
  soundwire: intel: update interfaces between ASoC and SoundWire

Rander Wang (1):
  soundwire: intel: update stream callbacks for hwparams/free stream
    operations

 drivers/soundwire/intel.c           |  20 ++++--
 drivers/soundwire/intel.h           |  13 ++--
 drivers/soundwire/intel_init.c      |  31 +++------
 include/linux/soundwire/sdw.h       |   7 ++
 include/linux/soundwire/sdw_intel.h | 103 +++++++++++++++++++++++++---
 5 files changed, 131 insertions(+), 43 deletions(-)

-- 
2.20.1

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

[alsa-devel] [PATCH 1/4] soundwire: sdw_slave: add new fields to track probe status

[Pierre-Louis Bossart]

Changes to the sdw_slave structure needed to solve race conditions on
driver probe.

The functionality is added in the next patch.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 include/linux/soundwire/sdw.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index 688b40e65c89..a381a596212b 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -545,6 +545,10 @@ struct sdw_slave_ops {
  * @node: node for bus list
  * @port_ready: Port ready completion flag for each Slave port
  * @dev_num: Device Number assigned by Bus
+ * @probed: boolean tracking driver state
+ * @probe_complete: completion utility to control potential races
+ * on startup between driver probe/initialization and SoundWire
+ * Slave state changes/imp-def interrupts
  */
 struct sdw_slave {
 	struct sdw_slave_id id;
@@ -559,6 +563,8 @@ struct sdw_slave {
 	struct list_head node;
 	struct completion *port_ready;
 	u16 dev_num;
+	bool probed;
+	struct completion probe_complete;
 };
 
 #define dev_to_sdw_dev(_dev) container_of(_dev, struct sdw_slave, dev)
-- 
2.20.1

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

[alsa-devel] [PATCH 2/4] soundwire: add enumeration_complete structure

[Pierre-Louis Bossart]

We need an async mechanism to prevent access to Slaves that are not
fully-enumerated.

init_completion() will be invoked when the Slave becomes UNATTACHED,
and complete() will be invoked when the state become ATTACHED. Any
read/write before this status change will be delayed with a
wait_for_completion().

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 include/linux/soundwire/sdw.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index a381a596212b..1381edfaa206 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -565,6 +565,7 @@ struct sdw_slave {
 	u16 dev_num;
 	bool probed;
 	struct completion probe_complete;
+	struct completion enumeration_complete;
 };
 
 #define dev_to_sdw_dev(_dev) container_of(_dev, struct sdw_slave, dev)
-- 
2.20.1

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

[alsa-devel] [PATCH 3/4] soundwire: intel: update interfaces between ASoC and SoundWire

[Pierre-Louis Bossart]

Expose updated interfaces in sdw_intel.h, mainly to allow SoundWire
and ASoC parts to be merged separately once the header files are
shared between trees.

To avoid compilation issues, the conflicts in intel_init.c are blindly
removed. This would in theory prevent the code from working, but since
there are no users of the Intel Soundwire driver this has no
impact. Functionality will be restored when the removal of platform
devices is complete.

Support for SoundWire + SOF builds will only be provided once all the
required pieces are upstream.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/intel.h           |  9 ++--
 drivers/soundwire/intel_init.c      | 30 +++---------
 include/linux/soundwire/sdw_intel.h | 71 +++++++++++++++++++++++++++--
 3 files changed, 79 insertions(+), 31 deletions(-)

diff --git a/drivers/soundwire/intel.h b/drivers/soundwire/intel.h
index d923b6262330..e4cc1d3804ff 100644
--- a/drivers/soundwire/intel.h
+++ b/drivers/soundwire/intel.h
@@ -5,17 +5,20 @@
 #define __SDW_INTEL_LOCAL_H
 
 /**
- * struct sdw_intel_link_res - Soundwire link resources
+ * struct sdw_intel_link_res - Soundwire Intel link resource structure,
+ * typically populated by the controller driver.
+ * @pdev: platform_device
+ * @mmio_base: mmio base of SoundWire registers
  * @registers: Link IO registers base
  * @shim: Audio shim pointer
  * @alh: ALH (Audio Link Hub) pointer
  * @irq: Interrupt line
  * @ops: Shim callback ops
  * @arg: Shim callback ops argument
- *
- * This is set as pdata for each link instance.
  */
 struct sdw_intel_link_res {
+	struct platform_device *pdev;
+	void __iomem *mmio_base; /* not strictly needed, useful for debug */
 	void __iomem *registers;
 	void __iomem *shim;
 	void __iomem *alh;
diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c
index 2a2b4d8df462..f8ae199094d3 100644
--- a/drivers/soundwire/intel_init.c
+++ b/drivers/soundwire/intel_init.c
@@ -27,19 +27,9 @@ static int link_mask;
 module_param_named(sdw_link_mask, link_mask, int, 0444);
 MODULE_PARM_DESC(sdw_link_mask, "Intel link mask (one bit per link)");
 
-struct sdw_link_data {
-	struct sdw_intel_link_res res;
-	struct platform_device *pdev;
-};
-
-struct sdw_intel_ctx {
-	int count;
-	struct sdw_link_data *links;
-};
-
 static int sdw_intel_cleanup_pdev(struct sdw_intel_ctx *ctx)
 {
-	struct sdw_link_data *link = ctx->links;
+	struct sdw_intel_link_res *link = ctx->links;
 	int i;
 
 	if (!link)
@@ -62,7 +52,7 @@ static struct sdw_intel_ctx
 {
 	struct platform_device_info pdevinfo;
 	struct platform_device *pdev;
-	struct sdw_link_data *link;
+	struct sdw_intel_link_res *link;
 	struct sdw_intel_ctx *ctx;
 	struct acpi_device *adev;
 	int ret, i;
@@ -123,14 +113,12 @@ static struct sdw_intel_ctx
 			continue;
 		}
 
-		link->res.irq = res->irq;
-		link->res.registers = res->mmio_base + SDW_LINK_BASE
+		link->registers = res->mmio_base + SDW_LINK_BASE
 					+ (SDW_LINK_SIZE * i);
-		link->res.shim = res->mmio_base + SDW_SHIM_BASE;
-		link->res.alh = res->mmio_base + SDW_ALH_BASE;
+		link->shim = res->mmio_base + SDW_SHIM_BASE;
+		link->alh = res->mmio_base + SDW_ALH_BASE;
 
-		link->res.ops = res->ops;
-		link->res.arg = res->arg;
+		link->ops = res->ops;
 
 		memset(&pdevinfo, 0, sizeof(pdevinfo));
 
@@ -138,8 +126,6 @@ static struct sdw_intel_ctx
 		pdevinfo.name = "int-sdw";
 		pdevinfo.id = i;
 		pdevinfo.fwnode = acpi_fwnode_handle(adev);
-		pdevinfo.data = &link->res;
-		pdevinfo.size_data = sizeof(link->res);
 
 		pdev = platform_device_register_full(&pdevinfo);
 		if (IS_ERR(pdev)) {
@@ -224,10 +210,8 @@ EXPORT_SYMBOL(sdw_intel_init);
  *
  * Delete the controller instances created and cleanup
  */
-void sdw_intel_exit(void *arg)
+void sdw_intel_exit(struct sdw_intel_ctx *ctx)
 {
-	struct sdw_intel_ctx *ctx = arg;
-
 	sdw_intel_cleanup_pdev(ctx);
 	kfree(ctx);
 }
diff --git a/include/linux/soundwire/sdw_intel.h b/include/linux/soundwire/sdw_intel.h
index c9427cb6020b..2cb653299731 100644
--- a/include/linux/soundwire/sdw_intel.h
+++ b/include/linux/soundwire/sdw_intel.h
@@ -16,24 +16,85 @@ struct sdw_intel_ops {
 };
 
 /**
- * struct sdw_intel_res - Soundwire Intel resource structure
+ * struct sdw_intel_acpi_info - Soundwire Intel information found in ACPI tables
+ * @handle: ACPI controller handle
+ * @count: link count found with "sdw-master-count" property
+ * @link_mask: bit-wise mask listing links enabled by BIOS menu
+ *
+ * this structure could be expanded to e.g. provide all the _ADR
+ * information in case the link_mask is not sufficient to identify
+ * platform capabilities.
+ */
+struct sdw_intel_acpi_info {
+	acpi_handle handle;
+	int count;
+	u32 link_mask;
+};
+
+/**
+ * struct sdw_intel_res - Soundwire Intel global resource structure,
+ * typically populated by the DSP driver
+ *
+ * @count: link count (may be filtered by DSP driver)
  * @mmio_base: mmio base of SoundWire registers
  * @irq: interrupt number
  * @handle: ACPI parent handle
  * @parent: parent device
  * @ops: callback ops
- * @arg: callback arg
+ * @dev: device implementing hwparams and free callbacks
+ * @link_mask: bit-wise mask listing links selected by the DSP driver
  */
 struct sdw_intel_res {
+	int count;
 	void __iomem *mmio_base;
 	int irq;
 	acpi_handle handle;
 	struct device *parent;
 	const struct sdw_intel_ops *ops;
-	void *arg;
+	struct device *dev;
+	u32 link_mask;
 };
 
-void *sdw_intel_init(acpi_handle *parent_handle, struct sdw_intel_res *res);
-void sdw_intel_exit(void *arg);
+struct sdw_intel_link_res;
+
+/**
+ * struct sdw_intel_ctx - context allocated by the controller
+ * driver probe
+ * @count: link count
+ * @mmio_base: mmio base of SoundWire registers, only used to check
+ * hardware capabilities after all power dependencies are settled.
+ * @arg: Shim callback ops argument
+ */
+struct sdw_intel_ctx {
+	int count;
+	void __iomem *mmio_base;
+	u32 link_mask;
+	acpi_handle handle;
+	struct sdw_intel_link_res *links;
+};
+
+/*
+ * On Intel platforms, the SoundWire IP has dependencies on power
+ * rails shared with the DSP, and the initialization steps are split
+ * in three. First an ACPI scan to check what the firmware describes
+ * in DSDT tables, then an allocation step (with no hardware
+ * configuration but with all the relevant devices created) and last
+ * the actual hardware configuration. The final stage is a global
+ * interrupt enable which is controlled by the DSP driver. Splitting
+ * these phases helps simplify the boot flow and make early decisions
+ * on e.g. which machine driver to select (I2S mode, HDaudio or
+ * SoundWire).
+ */
+int sdw_intel_acpi_scan(acpi_handle *parent_handle,
+			struct sdw_intel_acpi_info *info);
+
+struct sdw_intel_ctx *
+sdw_intel_probe(struct sdw_intel_res *res);
+
+int sdw_intel_startup(struct sdw_intel_ctx *ctx);
+
+void sdw_intel_exit(struct sdw_intel_ctx *ctx);
+
+void sdw_intel_enable_irq(void __iomem *mmio_base, bool enable);
 
 #endif
-- 
2.20.1

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

[alsa-devel] [PATCH 4/4] soundwire: intel: update stream callbacks for hwparams/free stream operations

[Pierre-Louis Bossart]

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

Rename 'config_stream' as 'params_stream' to be closer to ALSA/ASoC
concepts.

Add a 'free_stream' callback in case any resources allocated in the
'params_stream' stage need to be released.

Define structures for callbacks, mainly to allow for extensions as
needed.

Add the link_id and alh_stream_id parameters which are needed to align
with firmware IPC needs.

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           | 20 ++++++++++++------
 drivers/soundwire/intel.h           |  4 ++--
 drivers/soundwire/intel_init.c      |  1 +
 include/linux/soundwire/sdw_intel.h | 32 +++++++++++++++++++++++++----
 4 files changed, 45 insertions(+), 12 deletions(-)

diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index acdec12d748d..10ff03d3a9e5 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -523,17 +523,24 @@ intel_pdi_alh_configure(struct sdw_intel *sdw, struct sdw_cdns_pdi *pdi)
 	intel_writel(alh, SDW_ALH_STRMZCFG(pdi->intel_alh_id), conf);
 }
 
-static int intel_config_stream(struct sdw_intel *sdw,
+static int intel_params_stream(struct sdw_intel *sdw,
 			       struct snd_pcm_substream *substream,
 			       struct snd_soc_dai *dai,
-			       struct snd_pcm_hw_params *hw_params, int link_id)
+			       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_stream_params_data params_data;
 
-	if (res->ops && res->ops->config_stream && res->arg)
-		return res->ops->config_stream(res->arg,
-				substream, dai, hw_params, link_id);
+	params_data.substream = substream;
+	params_data.dai = dai;
+	params_data.hw_params = hw_params;
+	params_data.link_id = link_id;
+	params_data.alh_stream_id = alh_stream_id;
 
+	if (res->ops && res->ops->params_stream && res->dev)
+		return res->ops->params_stream(res->dev,
+					       &params_data);
 	return -EIO;
 }
 
@@ -648,7 +655,8 @@ static int intel_hw_params(struct snd_pcm_substream *substream,
 
 
 	/* Inform DSP about PDI stream number */
-	ret = intel_config_stream(sdw, substream, dai, params,
+	ret = intel_params_stream(sdw, substream, dai, params,
+				  sdw->instance,
 				  pdi->intel_alh_id);
 	if (ret)
 		goto error;
diff --git a/drivers/soundwire/intel.h b/drivers/soundwire/intel.h
index e4cc1d3804ff..38b7c125fb10 100644
--- a/drivers/soundwire/intel.h
+++ b/drivers/soundwire/intel.h
@@ -14,7 +14,7 @@
  * @alh: ALH (Audio Link Hub) pointer
  * @irq: Interrupt line
  * @ops: Shim callback ops
- * @arg: Shim callback ops argument
+ * @dev: device implementing hw_params and free callbacks
  */
 struct sdw_intel_link_res {
 	struct platform_device *pdev;
@@ -24,7 +24,7 @@ struct sdw_intel_link_res {
 	void __iomem *alh;
 	int irq;
 	const struct sdw_intel_ops *ops;
-	void *arg;
+	struct device *dev;
 };
 
 #endif /* __SDW_INTEL_LOCAL_H */
diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c
index f8ae199094d3..6bc167c83b47 100644
--- a/drivers/soundwire/intel_init.c
+++ b/drivers/soundwire/intel_init.c
@@ -119,6 +119,7 @@ static struct sdw_intel_ctx
 		link->alh = res->mmio_base + SDW_ALH_BASE;
 
 		link->ops = res->ops;
+		link->dev = res->dev;
 
 		memset(&pdevinfo, 0, sizeof(pdevinfo));
 
diff --git a/include/linux/soundwire/sdw_intel.h b/include/linux/soundwire/sdw_intel.h
index 2cb653299731..36e2b522a749 100644
--- a/include/linux/soundwire/sdw_intel.h
+++ b/include/linux/soundwire/sdw_intel.h
@@ -4,15 +4,39 @@
 #ifndef __SDW_INTEL_H
 #define __SDW_INTEL_H
 
+/**
+ * struct sdw_intel_stream_params_data: configuration passed during
+ * the @params_stream callback, e.g. for interaction with DSP
+ * firmware.
+ */
+struct sdw_intel_stream_params_data {
+	struct snd_pcm_substream *substream;
+	struct snd_soc_dai *dai;
+	struct snd_pcm_hw_params *hw_params;
+	int link_id;
+	int alh_stream_id;
+};
+
+/**
+ * struct sdw_intel_stream_free_data: configuration passed during
+ * the @free_stream callback, e.g. for interaction with DSP
+ * firmware.
+ */
+struct sdw_intel_stream_free_data {
+	struct snd_pcm_substream *substream;
+	struct snd_soc_dai *dai;
+	int link_id;
+};
+
 /**
  * struct sdw_intel_ops: Intel audio driver callback ops
  *
- * @config_stream: configure the stream with the hw_params
- * the first argument containing the context is mandatory
  */
 struct sdw_intel_ops {
-	int (*config_stream)(void *arg, void *substream,
-			     void *dai, void *hw_params, int stream_num);
+	int (*params_stream)(struct device *dev,
+			     struct sdw_intel_stream_params_data *params_data);
+	int (*free_stream)(struct device *dev,
+			   struct sdw_intel_stream_free_data *free_data);
 };
 
 /**
-- 
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 1/4] soundwire: sdw_slave: add new fields to track probe status

[Vinod Koul]

On 23-10-19, 16:06, Pierre-Louis Bossart wrote:
> Changes to the sdw_slave structure needed to solve race conditions on
> driver probe.

Can you please explain the race you have observed, it would be a very
useful to document it as well

> 
> The functionality is added in the next patch.

which one..?

> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  include/linux/soundwire/sdw.h | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
> index 688b40e65c89..a381a596212b 100644
> --- a/include/linux/soundwire/sdw.h
> +++ b/include/linux/soundwire/sdw.h
> @@ -545,6 +545,10 @@ struct sdw_slave_ops {
>   * @node: node for bus list
>   * @port_ready: Port ready completion flag for each Slave port
>   * @dev_num: Device Number assigned by Bus
> + * @probed: boolean tracking driver state
> + * @probe_complete: completion utility to control potential races
> + * on startup between driver probe/initialization and SoundWire
> + * Slave state changes/imp-def interrupts
>   */
>  struct sdw_slave {
>  	struct sdw_slave_id id;
> @@ -559,6 +563,8 @@ struct sdw_slave {
>  	struct list_head node;
>  	struct completion *port_ready;
>  	u16 dev_num;
> +	bool probed;
> +	struct completion probe_complete;
>  };
>  
>  #define dev_to_sdw_dev(_dev) container_of(_dev, struct sdw_slave, dev)
> -- 
> 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 2/4] soundwire: add enumeration_complete structure

[Vinod Koul]

On 23-10-19, 16:06, Pierre-Louis Bossart wrote:
> We need an async mechanism to prevent access to Slaves that are not
> fully-enumerated.
> 
> init_completion() will be invoked when the Slave becomes UNATTACHED,
> and complete() will be invoked when the state become ATTACHED. Any
> read/write before this status change will be delayed with a
> wait_for_completion().
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  include/linux/soundwire/sdw.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
> index a381a596212b..1381edfaa206 100644
> --- a/include/linux/soundwire/sdw.h
> +++ b/include/linux/soundwire/sdw.h
> @@ -565,6 +565,7 @@ struct sdw_slave {
>  	u16 dev_num;
>  	bool probed;
>  	struct completion probe_complete;
> +	struct completion enumeration_complete;

Which series/patch uses this..?

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

Re: [alsa-devel] [PATCH 1/4] soundwire: sdw_slave: add new fields to track probe status

[Pierre-Louis Bossart]

On 11/2/19 11:56 PM, Vinod Koul wrote:
> On 23-10-19, 16:06, Pierre-Louis Bossart wrote:
>> Changes to the sdw_slave structure needed to solve race conditions on
>> driver probe.
> 
> Can you please explain the race you have observed, it would be a very
> useful to document it as well

the races are explained in the [PATCH 00/18] soundwire: code hardening 
and suspend-resume support series.

>>
>> The functionality is added in the next patch.
> 
> which one..?

[PATCH 00/18] soundwire: code hardening and suspend-resume support


> 
>>
>> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>> ---
>>   include/linux/soundwire/sdw.h | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
>> index 688b40e65c89..a381a596212b 100644
>> --- a/include/linux/soundwire/sdw.h
>> +++ b/include/linux/soundwire/sdw.h
>> @@ -545,6 +545,10 @@ struct sdw_slave_ops {
>>    * @node: node for bus list
>>    * @port_ready: Port ready completion flag for each Slave port
>>    * @dev_num: Device Number assigned by Bus
>> + * @probed: boolean tracking driver state
>> + * @probe_complete: completion utility to control potential races
>> + * on startup between driver probe/initialization and SoundWire
>> + * Slave state changes/imp-def interrupts
>>    */
>>   struct sdw_slave {
>>   	struct sdw_slave_id id;
>> @@ -559,6 +563,8 @@ struct sdw_slave {
>>   	struct list_head node;
>>   	struct completion *port_ready;
>>   	u16 dev_num;
>> +	bool probed;
>> +	struct completion probe_complete;
>>   };
>>   
>>   #define dev_to_sdw_dev(_dev) container_of(_dev, struct sdw_slave, dev)
>> -- 
>> 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 2/4] soundwire: add enumeration_complete structure

[Pierre-Louis Bossart]

On 11/3/19 12:22 AM, Vinod Koul wrote:
> On 23-10-19, 16:06, Pierre-Louis Bossart wrote:
>> We need an async mechanism to prevent access to Slaves that are not
>> fully-enumerated.
>>
>> init_completion() will be invoked when the Slave becomes UNATTACHED,
>> and complete() will be invoked when the state become ATTACHED. Any
>> read/write before this status change will be delayed with a
>> wait_for_completion().
>>
>> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>> ---
>>   include/linux/soundwire/sdw.h | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
>> index a381a596212b..1381edfaa206 100644
>> --- a/include/linux/soundwire/sdw.h
>> +++ b/include/linux/soundwire/sdw.h
>> @@ -565,6 +565,7 @@ struct sdw_slave {
>>   	u16 dev_num;
>>   	bool probed;
>>   	struct completion probe_complete;
>> +	struct completion enumeration_complete;
> 
> Which series/patch uses this..?

[PATCH 00/18] soundwire: code hardening and suspend-resume support
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH 1/4] soundwire: sdw_slave: add new fields to track probe status

[Vinod Koul]

On 04-11-19, 08:32, Pierre-Louis Bossart wrote:
> 
> 
> On 11/2/19 11:56 PM, Vinod Koul wrote:
> > On 23-10-19, 16:06, Pierre-Louis Bossart wrote:
> > > Changes to the sdw_slave structure needed to solve race conditions on
> > > driver probe.
> > 
> > Can you please explain the race you have observed, it would be a very
> > useful to document it as well
> 
> the races are explained in the [PATCH 00/18] soundwire: code hardening and
> suspend-resume support series.

It would make sense to explain it here as well to give details to
reviewers, there is nothing wrong with too much detail!

> > > 
> > > The functionality is added in the next patch.
> > 
> > which one..?
> 
> [PATCH 00/18] soundwire: code hardening and suspend-resume support

Yeah great! let me play detective with 18 patch series. I asked for a
patch and got a series!

Again, please help the maintainer to help you. We would love to see this
merged as well, but please step up and give more details in cover
letter and changelogs. I shouldn't need to do guesswork and scan through the
inbox to find the context!

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

Re: [alsa-devel] [PATCH 1/4] soundwire: sdw_slave: add new fields to track probe status

[Pierre-Louis Bossart]

On 11/7/19 10:29 PM, Vinod Koul wrote:
> On 04-11-19, 08:32, Pierre-Louis Bossart wrote:
>>
>>
>> On 11/2/19 11:56 PM, Vinod Koul wrote:
>>> On 23-10-19, 16:06, Pierre-Louis Bossart wrote:
>>>> Changes to the sdw_slave structure needed to solve race conditions on
>>>> driver probe.
>>>
>>> Can you please explain the race you have observed, it would be a very
>>> useful to document it as well
>>
>> the races are explained in the [PATCH 00/18] soundwire: code hardening and
>> suspend-resume support series.
> 
> It would make sense to explain it here as well to give details to
> reviewers, there is nothing wrong with too much detail!
> 
>>>>
>>>> The functionality is added in the next patch.
>>>
>>> which one..?
>>
>> [PATCH 00/18] soundwire: code hardening and suspend-resume support
> 
> Yeah great! let me play detective with 18 patch series. I asked for a
> patch and got a series!
> 
> Again, please help the maintainer to help you. We would love to see this
> merged as well, but please step up and give more details in cover
> letter and changelogs. I shouldn't need to do guesswork and scan through the
> inbox to find the context!

We are clearly not going anywhere.

I partitioned the patches to make your maintainer life easier and help 
the integration of SoundWire across two trees. All I get is negative 
feedback, grand-standing, and zero comments on actual changes.

For the record, I am mindful of reviewer/maintainer workload, and I did 
contact you in September to check your availability and provided a 
pointer to initial code changes. I did send a first version a week prior 
to your travel/vacation, I resend another version when you were back and 
waited yet another two weeks to resend a second version. I also 
contacted Takashi, Mark and you to suggest this code partition, and did 
not get any pushback. It's not like I am pushing stuff down your throat, 
I have been patient and considerate.

Please start with the patches "soundwire: code hardening and 
suspend-resume support" and come back to this interface description when 
you have reviewed these changes. It's not detective work, it's working 
around the consequences of having separate trees for Audio and SoundWire.

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

Re: [alsa-devel] [PATCH 1/4] soundwire: sdw_slave: add new fields to track probe status

[Liam Girdwood]

On Fri, 2019-11-08 at 08:55 -0600, Pierre-Louis Bossart wrote:
> Please start with the patches "soundwire: code hardening and 
> suspend-resume support" and come back to this interface description when 
> you have reviewed these changes. It's not detective work, it's working 
> around the consequences of having separate trees for Audio and SoundWire.

Separate trees seem to be clearly not working well for everyone
atm. I'm reading the discomfort on all sides here.

Vinod, how often do you merge on ALSA/ASoC ? Would it not make more
sense to be part of ALSA to ease workflow (including yours) ? 

This model worked well for soundwire's predecessors (AC97 and HDA)
which like soundwire were primarily audio busses with secondary non
audio features.

Liam

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

Re: [alsa-devel] [PATCH 1/4] soundwire: sdw_slave: add new fields to track probe status

[Vinod Koul]

On 08-11-19, 08:55, Pierre-Louis Bossart wrote:
> 
> 
> On 11/7/19 10:29 PM, Vinod Koul wrote:
> > On 04-11-19, 08:32, Pierre-Louis Bossart wrote:
> > > 
> > > 
> > > On 11/2/19 11:56 PM, Vinod Koul wrote:
> > > > On 23-10-19, 16:06, Pierre-Louis Bossart wrote:
> > > > > Changes to the sdw_slave structure needed to solve race conditions on
> > > > > driver probe.
> > > > 
> > > > Can you please explain the race you have observed, it would be a very
> > > > useful to document it as well
> > > 
> > > the races are explained in the [PATCH 00/18] soundwire: code hardening and
> > > suspend-resume support series.
> > 
> > It would make sense to explain it here as well to give details to
> > reviewers, there is nothing wrong with too much detail!
> > 
> > > > > 
> > > > > The functionality is added in the next patch.
> > > > 
> > > > which one..?
> > > 
> > > [PATCH 00/18] soundwire: code hardening and suspend-resume support
> > 
> > Yeah great! let me play detective with 18 patch series. I asked for a
> > patch and got a series!
> > 
> > Again, please help the maintainer to help you. We would love to see this
> > merged as well, but please step up and give more details in cover
> > letter and changelogs. I shouldn't need to do guesswork and scan through the
> > inbox to find the context!
> 
> We are clearly not going anywhere.

Correct as you don't seem to provide clear answers, I am asking again
which patch implements the new fields added here, how difficult is it to
provide the *specific* patch which implements this so that I can compare
the implementation and see why this is needed and apply if fine!

But no you will not provide a clear answer and start ranting!

> I partitioned the patches to make your maintainer life easier and help the
> integration of SoundWire across two trees. All I get is negative feedback,
> grand-standing, and zero comments on actual changes.

No you get asked specific question which you do not like and start off
on a tangent!

> For the record, I am mindful of reviewer/maintainer workload, and I did
> contact you in September to check your availability and provided a pointer
> to initial code changes. I did send a first version a week prior to your
> travel/vacation, I resend another version when you were back and waited yet
> another two weeks to resend a second version. I also contacted Takashi, Mark
> and you to suggest this code partition, and did not get any pushback. It's
> not like I am pushing stuff down your throat, I have been patient and
> considerate.
> 
> Please start with the patches "soundwire: code hardening and suspend-resume
> support" and come back to this interface description when you have reviewed
> these changes. It's not detective work, it's working around the consequences
> of having separate trees for Audio and SoundWire.

Again, which patch in the series does implement these new members!

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

Re: [alsa-devel] [PATCH 1/4] soundwire: sdw_slave: add new fields to track probe status

[Pierre-Louis Bossart]

On 11/9/19 5:12 AM, Vinod Koul wrote:
> On 08-11-19, 08:55, Pierre-Louis Bossart wrote:
>>
>>
>> On 11/7/19 10:29 PM, Vinod Koul wrote:
>>> On 04-11-19, 08:32, Pierre-Louis Bossart wrote:
>>>>
>>>>
>>>> On 11/2/19 11:56 PM, Vinod Koul wrote:
>>>>> On 23-10-19, 16:06, Pierre-Louis Bossart wrote:
>>>>>> Changes to the sdw_slave structure needed to solve race conditions on
>>>>>> driver probe.
>>>>>
>>>>> Can you please explain the race you have observed, it would be a very
>>>>> useful to document it as well
>>>>
>>>> the races are explained in the [PATCH 00/18] soundwire: code hardening and
>>>> suspend-resume support series.
>>>
>>> It would make sense to explain it here as well to give details to
>>> reviewers, there is nothing wrong with too much detail!
>>>
>>>>>>
>>>>>> The functionality is added in the next patch.
>>>>>
>>>>> which one..?
>>>>
>>>> [PATCH 00/18] soundwire: code hardening and suspend-resume support
>>>
>>> Yeah great! let me play detective with 18 patch series. I asked for a
>>> patch and got a series!
>>>
>>> Again, please help the maintainer to help you. We would love to see this
>>> merged as well, but please step up and give more details in cover
>>> letter and changelogs. I shouldn't need to do guesswork and scan through the
>>> inbox to find the context!
>>
>> We are clearly not going anywhere.
> 
> Correct as you don't seem to provide clear answers, I am asking again
> which patch implements the new fields added here, how difficult is it to
> provide the *specific* patch which implements this so that I can compare
> the implementation and see why this is needed and apply if fine!
> 
> But no you will not provide a clear answer and start ranting!
> 
>> I partitioned the patches to make your maintainer life easier and help the
>> integration of SoundWire across two trees. All I get is negative feedback,
>> grand-standing, and zero comments on actual changes.
> 
> No you get asked specific question which you do not like and start off
> on a tangent!
> 
>> For the record, I am mindful of reviewer/maintainer workload, and I did
>> contact you in September to check your availability and provided a pointer
>> to initial code changes. I did send a first version a week prior to your
>> travel/vacation, I resend another version when you were back and waited yet
>> another two weeks to resend a second version. I also contacted Takashi, Mark
>> and you to suggest this code partition, and did not get any pushback. It's
>> not like I am pushing stuff down your throat, I have been patient and
>> considerate.
>>
>> Please start with the patches "soundwire: code hardening and suspend-resume
>> support" and come back to this interface description when you have reviewed
>> these changes. It's not detective work, it's working around the consequences
>> of having separate trees for Audio and SoundWire.
> 
> Again, which patch in the series does implement these new members!

It's really straightforward...here is the match between headers and 
functionality:

[PATCH v2 1/5] soundwire: sdw_slave: add new fields to track probe status
[PATCH v2 02/19] soundwire: fix race between driver probe and 
update_status callback

[PATCH v2 2/5] soundwire: add enumeration_complete structure
[PATCH v2 12/19] soundwire: add enumeration_complete signaling

[PATCH v2 3/5] soundwire: add initialization_complete definition
[PATCH v2 13/19] soundwire: bus: add initialization_complete signaling

[PATCH v2 4/5] soundwire: intel: update interfaces between ASoC and 
SoundWire
[PATCH v2 5/5] soundwire: intel: update stream callbacks for 
hwparams/free stream operations
[PATCH v2 13/14] soundwire: intel: free all resources on hw_free()

I suggested an approach that you didn't comment on, and now I am not 
sure what to make of the heated wording and exclamation marks. You did 
not answer to Liam's question on links between ASoC/SoundWire - despite 
the fact that drivers/soundwire cannot be an isolated subsystem, both 
the Intel and Qualcomm solutions have a big fat 'depends on SND_SOC'.

At this point I am formally asking for your view and guidance on how we 
are going to do the SoundWire/ASoC integration. It's now your time to 
make suggestions on what the flow should be between you and 
Mark/Takashi. If you don't want this initial change to the header files, 
then what is your proposal?


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

Re: [alsa-devel] [PATCH 1/4] soundwire: sdw_slave: add new fields to track probe status

[Vinod Koul]

On 11-11-19, 10:34, Pierre-Louis Bossart wrote:
> 
> 
> On 11/9/19 5:12 AM, Vinod Koul wrote:
> > On 08-11-19, 08:55, Pierre-Louis Bossart wrote:
> > > 
> > > 
> > > On 11/7/19 10:29 PM, Vinod Koul wrote:
> > > > On 04-11-19, 08:32, Pierre-Louis Bossart wrote:
> > > > > 
> > > > > 
> > > > > On 11/2/19 11:56 PM, Vinod Koul wrote:
> > > > > > On 23-10-19, 16:06, Pierre-Louis Bossart wrote:
> > > > > > > Changes to the sdw_slave structure needed to solve race conditions on
> > > > > > > driver probe.
> > > > > > 
> > > > > > Can you please explain the race you have observed, it would be a very
> > > > > > useful to document it as well
> > > > > 
> > > > > the races are explained in the [PATCH 00/18] soundwire: code hardening and
> > > > > suspend-resume support series.
> > > > 
> > > > It would make sense to explain it here as well to give details to
> > > > reviewers, there is nothing wrong with too much detail!
> > > > 
> > > > > > > 
> > > > > > > The functionality is added in the next patch.
> > > > > > 
> > > > > > which one..?
> > > > > 
> > > > > [PATCH 00/18] soundwire: code hardening and suspend-resume support
> > > > 
> > > > Yeah great! let me play detective with 18 patch series. I asked for a
> > > > patch and got a series!
> > > > 
> > > > Again, please help the maintainer to help you. We would love to see this
> > > > merged as well, but please step up and give more details in cover
> > > > letter and changelogs. I shouldn't need to do guesswork and scan through the
> > > > inbox to find the context!
> > > 
> > > We are clearly not going anywhere.
> > 
> > Correct as you don't seem to provide clear answers, I am asking again
> > which patch implements the new fields added here, how difficult is it to
> > provide the *specific* patch which implements this so that I can compare
> > the implementation and see why this is needed and apply if fine!
> > 
> > But no you will not provide a clear answer and start ranting!
> > 
> > > I partitioned the patches to make your maintainer life easier and help the
> > > integration of SoundWire across two trees. All I get is negative feedback,
> > > grand-standing, and zero comments on actual changes.
> > 
> > No you get asked specific question which you do not like and start off
> > on a tangent!
> > 
> > > For the record, I am mindful of reviewer/maintainer workload, and I did
> > > contact you in September to check your availability and provided a pointer
> > > to initial code changes. I did send a first version a week prior to your
> > > travel/vacation, I resend another version when you were back and waited yet
> > > another two weeks to resend a second version. I also contacted Takashi, Mark
> > > and you to suggest this code partition, and did not get any pushback. It's
> > > not like I am pushing stuff down your throat, I have been patient and
> > > considerate.
> > > 
> > > Please start with the patches "soundwire: code hardening and suspend-resume
> > > support" and come back to this interface description when you have reviewed
> > > these changes. It's not detective work, it's working around the consequences
> > > of having separate trees for Audio and SoundWire.
> > 
> > Again, which patch in the series does implement these new members!
> 
> It's really straightforward...here is the match between headers and
> functionality:
> 
> [PATCH v2 1/5] soundwire: sdw_slave: add new fields to track probe status
> [PATCH v2 02/19] soundwire: fix race between driver probe and update_status
> callback
> 
> [PATCH v2 2/5] soundwire: add enumeration_complete structure
> [PATCH v2 12/19] soundwire: add enumeration_complete signaling
> 
> [PATCH v2 3/5] soundwire: add initialization_complete definition
> [PATCH v2 13/19] soundwire: bus: add initialization_complete signaling
> 
> [PATCH v2 4/5] soundwire: intel: update interfaces between ASoC and
> SoundWire
> [PATCH v2 5/5] soundwire: intel: update stream callbacks for hwparams/free
> stream operations
> [PATCH v2 13/14] soundwire: intel: free all resources on hw_free()

Thanks for the pointers, I will look at these patches and do the needful
for this series

> I suggested an approach that you didn't comment on, and now I am not sure
> what to make of the heated wording and exclamation marks. You did not answer
> to Liam's question on links between ASoC/SoundWire - despite the fact that
> drivers/soundwire cannot be an isolated subsystem, both the Intel and
> Qualcomm solutions have a big fat 'depends on SND_SOC'.
> 
> At this point I am formally asking for your view and guidance on how we are
> going to do the SoundWire/ASoC integration. It's now your time to make
> suggestions on what the flow should be between you and Mark/Takashi. If you
> don't want this initial change to the header files, then what is your
> proposal?

It is going to be as it would be for any other subsystem. Please mention
in the cover letter about required dependency. In case asoc needs this I
will create a immutable tag and in reverse case Mark will do so.

It is not really an issue if we get the information ahead of time

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

Re: [alsa-devel] [PATCH 1/4] soundwire: sdw_slave: add new fields to track probe status

[Pierre-Louis Bossart]

>> At this point I am formally asking for your view and guidance on how we are
>> going to do the SoundWire/ASoC integration. It's now your time to make
>> suggestions on what the flow should be between you and Mark/Takashi. If you
>> don't want this initial change to the header files, then what is your
>> proposal?
> 
> It is going to be as it would be for any other subsystem. Please mention
> in the cover letter about required dependency. In case asoc needs this I
> will create a immutable tag and in reverse case Mark will do so.
> 
> It is not really an issue if we get the information ahead of time

I added a whole set of comments on the race conditions and ran them by 
people with limited knowledge of SoundWire to see if the explanations 
made sense and why those header files were changed. Will send this later 
today as a v3.

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