[PATCH 0/7] ASoC: soundwire: Move sdw stream operations to
sdw stream operation APIs can be called once per stream. dailink callbacks are good places to call these APIs.
Pierre-Louis Bossart (7): ASoC: soc-dai: clarify return value for get_sdw_stream() soundwire: stream: fix NULL/IS_ERR confusion soundwire: intel: fix NULL/ERR_PTR confusion ASOC: Intel: sof_sdw: add dailink .trigger callback ASOC: Intel: sof_sdw: add dailink .prepare and .hw_free callback soundwire: intel: remove .trigger operation soundwire: intel: remove stream handling from .prepare and .hw_free
drivers/soundwire/intel.c | 60 ++++------------------- drivers/soundwire/stream.c | 2 +- include/sound/soc-dai.h | 3 +- sound/soc/intel/boards/sof_sdw.c | 81 ++++++++++++++++++++++++++++++++ 4 files changed, 92 insertions(+), 54 deletions(-)
From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
Previous changes move to use ERR_PTR(-ENOTSUPP), but it's not clear what implementations can return in case of errors. Explicitly document that NULL is not a possible return value, only ERR_PTR with a negative error code is valid.
Fixes: 308811a327c38 ('ASoC: soc-dai: return proper error for get_sdw_stream()') Cc: Srinivas Kandagatla srinivas.kandagatla@linaro.org Reported-by: Bard Liao yung-chuan.liao@linux.intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Rander Wang rander.wang@linux.intel.com Signed-off-by: Bard Liao yung-chuan.liao@linux.intel.com --- include/sound/soc-dai.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/include/sound/soc-dai.h b/include/sound/soc-dai.h index 776a60529e70..8b693dade9c6 100644 --- a/include/sound/soc-dai.h +++ b/include/sound/soc-dai.h @@ -471,7 +471,8 @@ static inline int snd_soc_dai_set_sdw_stream(struct snd_soc_dai *dai, * This routine only retrieves that was previously configured * with snd_soc_dai_get_sdw_stream() * - * Returns pointer to stream or -ENOTSUPP if callback is not supported; + * Returns pointer to stream or an ERR_PTR value, e.g. + * ERR_PTR(-ENOTSUPP) if callback is not supported; */ static inline void *snd_soc_dai_get_sdw_stream(struct snd_soc_dai *dai, int direction)
From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
snd_soc_dai_get_sdw_stream() can only return -ENOTSUPP or the stream, NULL is not a possible value.
Fixes: 4550569bd779f ('soundwire: stream: add helper to startup/shutdown streams') Reported-by: Bard Liao yung-chuan.liao@linux.intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Rander Wang rander.wang@linux.intel.com Signed-off-by: Bard Liao yung-chuan.liao@linux.intel.com --- drivers/soundwire/stream.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c index 37290a799023..3f54db259f45 100644 --- a/drivers/soundwire/stream.c +++ b/drivers/soundwire/stream.c @@ -1911,7 +1911,7 @@ void sdw_shutdown_stream(void *sdw_substream)
sdw_stream = snd_soc_dai_get_sdw_stream(dai, substream->stream);
- if (!sdw_stream) { + if (IS_ERR(sdw_stream)) { dev_err(rtd->dev, "no stream found for DAI %s", dai->name); return; }
From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
snd_soc_dai_get_sdw_stream() can only return the pointer to stream or an ERR_PTR value, NULL is not a possible value.
Fixes: 09553140c8d7b ('soundwire: intel: implement get_sdw_stream() operations') Reported-by: Bard Liao yung-chuan.liao@linux.intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Rander Wang rander.wang@linux.intel.com Signed-off-by: Bard Liao yung-chuan.liao@linux.intel.com --- drivers/soundwire/intel.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index ebca8ced59ec..bbfb86ffa653 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -1106,7 +1106,7 @@ static void *intel_get_sdw_stream(struct snd_soc_dai *dai, dma = dai->capture_dma_data;
if (!dma) - return NULL; + return ERR_PTR(-EINVAL);
return dma->stream; }
From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
Add trigger functionality to dailink, so far only .startup() and .shutdown() were implemented at the machine driver level.
The companion patch for this patch is the removal of the trigger callback at the DAI level in drivers/soundwire/intel.c
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Rander Wang rander.wang@linux.intel.com Signed-off-by: Bard Liao yung-chuan.liao@linux.intel.com --- sound/soc/intel/boards/sof_sdw.c | 41 ++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+)
diff --git a/sound/soc/intel/boards/sof_sdw.c b/sound/soc/intel/boards/sof_sdw.c index 2463d432bf4d..f251e046d74d 100644 --- a/sound/soc/intel/boards/sof_sdw.c +++ b/sound/soc/intel/boards/sof_sdw.c @@ -195,6 +195,46 @@ int sdw_startup(struct snd_pcm_substream *substream) return sdw_startup_stream(substream); }
+static int sdw_trigger(struct snd_pcm_substream *substream, int cmd) +{ + struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream); + struct sdw_stream_runtime *sdw_stream; + struct snd_soc_dai *dai; + int ret; + + /* Find stream from first CPU DAI */ + dai = asoc_rtd_to_cpu(rtd, 0); + + sdw_stream = snd_soc_dai_get_sdw_stream(dai, substream->stream); + + if (IS_ERR(sdw_stream)) { + dev_err(rtd->dev, "no stream found for DAI %s", dai->name); + return PTR_ERR(sdw_stream); + } + + switch (cmd) { + case SNDRV_PCM_TRIGGER_START: + case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: + case SNDRV_PCM_TRIGGER_RESUME: + ret = sdw_enable_stream(sdw_stream); + break; + + case SNDRV_PCM_TRIGGER_PAUSE_PUSH: + case SNDRV_PCM_TRIGGER_SUSPEND: + case SNDRV_PCM_TRIGGER_STOP: + ret = sdw_disable_stream(sdw_stream); + break; + default: + ret = -EINVAL; + break; + } + + if (ret) + dev_err(rtd->dev, "%s trigger %d failed: %d", __func__, cmd, ret); + + return ret; +} + void sdw_shutdown(struct snd_pcm_substream *substream) { sdw_shutdown_stream(substream); @@ -202,6 +242,7 @@ void sdw_shutdown(struct snd_pcm_substream *substream)
static const struct snd_soc_ops sdw_ops = { .startup = sdw_startup, + .trigger = sdw_trigger, .shutdown = sdw_shutdown, };
From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
Add .prepare and .hw_free callback to dailink.
The companion patch for this patch is the removal of stream operations in the .prepare and .hw_free callbacks at the DAI level in drivers/soundwire/intel.c
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Rander Wang rander.wang@linux.intel.com Signed-off-by: Bard Liao yung-chuan.liao@linux.intel.com --- sound/soc/intel/boards/sof_sdw.c | 40 ++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+)
diff --git a/sound/soc/intel/boards/sof_sdw.c b/sound/soc/intel/boards/sof_sdw.c index f251e046d74d..16503772965c 100644 --- a/sound/soc/intel/boards/sof_sdw.c +++ b/sound/soc/intel/boards/sof_sdw.c @@ -195,6 +195,25 @@ int sdw_startup(struct snd_pcm_substream *substream) return sdw_startup_stream(substream); }
+static int sdw_prepare(struct snd_pcm_substream *substream) +{ + struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream); + struct sdw_stream_runtime *sdw_stream; + struct snd_soc_dai *dai; + + /* Find stream from first CPU DAI */ + dai = asoc_rtd_to_cpu(rtd, 0); + + sdw_stream = snd_soc_dai_get_sdw_stream(dai, substream->stream); + + if (IS_ERR(sdw_stream)) { + dev_err(rtd->dev, "no stream found for DAI %s", dai->name); + return PTR_ERR(sdw_stream); + } + + return sdw_prepare_stream(sdw_stream); +} + static int sdw_trigger(struct snd_pcm_substream *substream, int cmd) { struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream); @@ -235,6 +254,25 @@ static int sdw_trigger(struct snd_pcm_substream *substream, int cmd) return ret; }
+static int sdw_hw_free(struct snd_pcm_substream *substream) +{ + struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream); + struct sdw_stream_runtime *sdw_stream; + struct snd_soc_dai *dai; + + /* Find stream from first CPU DAI */ + dai = asoc_rtd_to_cpu(rtd, 0); + + sdw_stream = snd_soc_dai_get_sdw_stream(dai, substream->stream); + + if (IS_ERR(sdw_stream)) { + dev_err(rtd->dev, "no stream found for DAI %s", dai->name); + return PTR_ERR(sdw_stream); + } + + return sdw_deprepare_stream(sdw_stream); +} + void sdw_shutdown(struct snd_pcm_substream *substream) { sdw_shutdown_stream(substream); @@ -242,7 +280,9 @@ void sdw_shutdown(struct snd_pcm_substream *substream)
static const struct snd_soc_ops sdw_ops = { .startup = sdw_startup, + .prepare = sdw_prepare, .trigger = sdw_trigger, + .hw_free = sdw_hw_free, .shutdown = sdw_shutdown, };
From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
Now that the stream trigger is handled at the dai-link level, there is no need for a dai-level trigger any longer.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Signed-off-by: Bard Liao yung-chuan.liao@linux.intel.com --- drivers/soundwire/intel.c | 39 --------------------------------------- 1 file changed, 39 deletions(-)
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index bbfb86ffa653..39d3186335ac 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -977,43 +977,6 @@ static int intel_prepare(struct snd_pcm_substream *substream, return ret; }
-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) { @@ -1115,7 +1078,6 @@ 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, .hw_free = intel_hw_free, .shutdown = intel_shutdown, .set_sdw_stream = intel_pcm_set_sdw_stream, @@ -1126,7 +1088,6 @@ 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, .hw_free = intel_hw_free, .shutdown = intel_shutdown, .set_sdw_stream = intel_pdm_set_sdw_stream,
From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
Now that the stream is handled at the dai-link level (in the machine driver), we can remove the stream handling at the dai level. We still need these callbacks to perform dai-level resource handling (i.e. addition/removal of a master).
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Signed-off-by: Bard Liao yung-chuan.liao@linux.intel.com --- drivers/soundwire/intel.c | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-)
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index 39d3186335ac..631c425ba430 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -931,7 +931,7 @@ static int intel_prepare(struct snd_pcm_substream *substream, struct sdw_intel *sdw = cdns_to_intel(cdns); struct sdw_cdns_dma_data *dma; int ch, dir; - int ret; + int ret = 0;
dma = snd_soc_dai_get_dma_data(dai, substream); if (!dma) { @@ -967,13 +967,8 @@ static int intel_prepare(struct snd_pcm_substream *substream, dma->hw_params, sdw->instance, dma->pdi->intel_alh_id); - if (ret) - goto err; }
- ret = sdw_prepare_stream(dma->stream); - -err: return ret; }
@@ -989,12 +984,12 @@ 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; - } - + /* + * The sdw stream state will transition to RELEASED when stream-> + * master_list is empty. So the stream state will transition to + * DEPREPARED for the first cpu-dai and to RELEASED for the last + * cpu-dai. + */ ret = sdw_stream_remove_master(&cdns->bus, dma->stream); if (ret < 0) { dev_err(dai->dev, "remove master from stream %s failed: %d\n",
On 01-09-20, 23:02, Bard Liao wrote:
sdw stream operation APIs can be called once per stream. dailink callbacks are good places to call these APIs.
Again, please mention here if this is to be merged thru sdw tree or ASoC tree
Pierre-Louis Bossart (7): ASoC: soc-dai: clarify return value for get_sdw_stream() soundwire: stream: fix NULL/IS_ERR confusion soundwire: intel: fix NULL/ERR_PTR confusion ASOC: Intel: sof_sdw: add dailink .trigger callback ASOC: Intel: sof_sdw: add dailink .prepare and .hw_free callback
These should be ASoC
soundwire: intel: remove .trigger operation soundwire: intel: remove stream handling from .prepare and .hw_free
drivers/soundwire/intel.c | 60 ++++------------------- drivers/soundwire/stream.c | 2 +- include/sound/soc-dai.h | 3 +- sound/soc/intel/boards/sof_sdw.c | 81 ++++++++++++++++++++++++++++++++ 4 files changed, 92 insertions(+), 54 deletions(-)
-- 2.17.1
On 9/3/20 5:42 AM, Vinod Koul wrote:
On 01-09-20, 23:02, Bard Liao wrote:
sdw stream operation APIs can be called once per stream. dailink callbacks are good places to call these APIs.
Again, please mention here if this is to be merged thru sdw tree or ASoC tree
Good point, I thought it wouldn't matter but it does. I just gave it a try and there seems to be a conflict on Mark's tree w/ drivers/soundwire/intel.c (likely due to missing patches already added to Vinod's tree).
So this should go to Vinod's tree with Mark's Acked-by tag on the ASoC changes.
Alternatively we can also split this in two, with ASoC-only and SoundWire-only patches in separate series if it's easier for maintainers. We would lose the rationale for the changes but that's not essential.
Pierre-Louis Bossart (7): ASoC: soc-dai: clarify return value for get_sdw_stream() soundwire: stream: fix NULL/IS_ERR confusion soundwire: intel: fix NULL/ERR_PTR confusion ASOC: Intel: sof_sdw: add dailink .trigger callback ASOC: Intel: sof_sdw: add dailink .prepare and .hw_free callback
These should be ASoC
Right. if you are fine with the content and this goes in your tree, can this be modified while applying? Or do want a v2?
soundwire: intel: remove .trigger operation soundwire: intel: remove stream handling from .prepare and .hw_free
drivers/soundwire/intel.c | 60 ++++------------------- drivers/soundwire/stream.c | 2 +- include/sound/soc-dai.h | 3 +- sound/soc/intel/boards/sof_sdw.c | 81 ++++++++++++++++++++++++++++++++ 4 files changed, 92 insertions(+), 54 deletions(-)
-- 2.17.1
On 03-09-20, 09:05, Pierre-Louis Bossart wrote:
On 9/3/20 5:42 AM, Vinod Koul wrote:
On 01-09-20, 23:02, Bard Liao wrote:
sdw stream operation APIs can be called once per stream. dailink callbacks are good places to call these APIs.
Again, please mention here if this is to be merged thru sdw tree or ASoC tree
Good point, I thought it wouldn't matter but it does. I just gave it a try and there seems to be a conflict on Mark's tree w/ drivers/soundwire/intel.c (likely due to missing patches already added to Vinod's tree).
So this should go to Vinod's tree with Mark's Acked-by tag on the ASoC changes.
Alternatively we can also split this in two, with ASoC-only and SoundWire-only patches in separate series if it's easier for maintainers. We would lose the rationale for the changes but that's not essential.
If there are no dependencies on each other, that is best preferred option. One should mention in cover-letter about the linked series though.
Pierre-Louis Bossart (7): ASoC: soc-dai: clarify return value for get_sdw_stream() soundwire: stream: fix NULL/IS_ERR confusion soundwire: intel: fix NULL/ERR_PTR confusion ASOC: Intel: sof_sdw: add dailink .trigger callback ASOC: Intel: sof_sdw: add dailink .prepare and .hw_free callback
These should be ASoC
Right. if you are fine with the content and this goes in your tree, can this be modified while applying? Or do want a v2?
soundwire: intel: remove .trigger operation soundwire: intel: remove stream handling from .prepare and .hw_free
drivers/soundwire/intel.c | 60 ++++------------------- drivers/soundwire/stream.c | 2 +- include/sound/soc-dai.h | 3 +- sound/soc/intel/boards/sof_sdw.c | 81 ++++++++++++++++++++++++++++++++ 4 files changed, 92 insertions(+), 54 deletions(-)
-- 2.17.1
-----Original Message----- From: Vinod Koul vkoul@kernel.org Sent: Friday, September 4, 2020 1:11 PM To: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Cc: Bard Liao yung-chuan.liao@linux.intel.com; alsa-devel@alsa- project.org; tiwai@suse.de; gregkh@linuxfoundation.org; linux- kernel@vger.kernel.org; ranjani.sridharan@linux.intel.com; hui.wang@canonical.com; broonie@kernel.org; srinivas.kandagatla@linaro.org; jank@cadence.com; Lin, Mengdong mengdong.lin@intel.com; Kale, Sanyog R sanyog.r.kale@intel.com; rander.wang@linux.intel.com; Liao, Bard bard.liao@intel.com Subject: Re: [PATCH 0/7] ASoC: soundwire: Move sdw stream operations to
On 03-09-20, 09:05, Pierre-Louis Bossart wrote:
On 9/3/20 5:42 AM, Vinod Koul wrote:
On 01-09-20, 23:02, Bard Liao wrote:
sdw stream operation APIs can be called once per stream. dailink callbacks are good places to call these APIs.
Again, please mention here if this is to be merged thru sdw tree or ASoC tree
Good point, I thought it wouldn't matter but it does. I just gave it a try and there seems to be a conflict on Mark's tree w/ drivers/soundwire/intel.c (likely due to missing patches already added to
Vinod's tree).
So this should go to Vinod's tree with Mark's Acked-by tag on the ASoC changes.
Alternatively we can also split this in two, with ASoC-only and SoundWire-only patches in separate series if it's easier for maintainers. We would lose the rationale for the changes but that's not
essential.
If there are no dependencies on each other, that is best preferred option. One should mention in cover-letter about the linked series though.
Sent as v2
participants (4)
-
Bard Liao -
Liao, Bard -
Pierre-Louis Bossart -
Vinod Koul