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(-)
--
2.17.1
From: Pierre-Louis Bossart <[email protected]>
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 <[email protected]>
Reviewed-by: Rander Wang <[email protected]>
Signed-off-by: Bard Liao <[email protected]>
---
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,
};
--
2.17.1
From: Pierre-Louis Bossart <[email protected]>
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 <[email protected]>
Signed-off-by: Bard Liao <[email protected]>
---
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,
--
2.17.1
From: Pierre-Louis Bossart <[email protected]>
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 <[email protected]>
Reported-by: Bard Liao <[email protected]>
Signed-off-by: Pierre-Louis Bossart <[email protected]>
Reviewed-by: Rander Wang <[email protected]>
Signed-off-by: Bard Liao <[email protected]>
---
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)
--
2.17.1
From: Pierre-Louis Bossart <[email protected]>
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 <[email protected]>
Signed-off-by: Pierre-Louis Bossart <[email protected]>
Reviewed-by: Rander Wang <[email protected]>
Signed-off-by: Bard Liao <[email protected]>
---
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;
}
--
2.17.1
From: Pierre-Louis Bossart <[email protected]>
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 <[email protected]>
Signed-off-by: Bard Liao <[email protected]>
---
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",
--
2.17.1
From: Pierre-Louis Bossart <[email protected]>
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 <[email protected]>
Signed-off-by: Pierre-Louis Bossart <[email protected]>
Reviewed-by: Rander Wang <[email protected]>
Signed-off-by: Bard Liao <[email protected]>
---
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;
}
--
2.17.1
From: Pierre-Louis Bossart <[email protected]>
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 <[email protected]>
Reviewed-by: Rander Wang <[email protected]>
Signed-off-by: Bard Liao <[email protected]>
---
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,
};
--
2.17.1
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
--
~Vinod
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
> >
--
~Vinod
> -----Original Message-----
> From: Vinod Koul <[email protected]>
> Sent: Friday, September 4, 2020 1:11 PM
> To: Pierre-Louis Bossart <[email protected]>
> Cc: Bard Liao <[email protected]>; alsa-devel@alsa-
> project.org; [email protected]; [email protected]; linux-
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; Lin, Mengdong
> <[email protected]>; Kale, Sanyog R <[email protected]>;
> [email protected]; Liao, Bard <[email protected]>
> 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