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@xxxxxxxxxxxxxxx>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>
---
 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.