[v2,5/5] soundwire: intel: free all resources on hw_free()
diff mbox series

Message ID 20200114234257.14336-6-pierre-louis.bossart@linux.intel.com
State New
Headers show
Series
  • soundwire: intel: add DAI callbacks
Related show

Commit Message

Pierre-Louis Bossart Jan. 14, 2020, 11:42 p.m. UTC
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(-)

Comments

Vinod Koul Feb. 12, 2020, 10:15 a.m. UTC | #1
On 14-01-20, 17:42, 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 c498812522ab..e0c1fff7c4a0 100644
> --- a/drivers/soundwire/intel.c
> +++ b/drivers/soundwire/intel.c
> @@ -550,6 +550,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;

where is this struct sdw_intel_stream_free_data defined. I dont see it
in this patch or this series..

> +
> +	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
>   */
> @@ -817,6 +836,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;
>  
> @@ -824,12 +844,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);

I think, free the 'name' here would be apt
Pierre-Louis Bossart Feb. 12, 2020, 3:37 p.m. UTC | #2
Hi Vinod,

>> +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;
> 
> where is this struct sdw_intel_stream_free_data defined. I dont see it
> in this patch or this series..

the definition is already upstream :-)

It was added in December with

4b206d34b92224 ('soundwire: intel: update stream callbacks for 
hwparams/free stream operations')

>> -	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);
> 
> I think, free the 'name' here would be apt

Right, this is something we discussed with Rander shortly before Chinese 
New Year and we wanted to handle this with a follow-up patch, would that 
work for you? if not I can send a v3, your choice.

Thanks
-Pierre
Vinod Koul Feb. 13, 2020, 4:23 a.m. UTC | #3
On 12-02-20, 09:37, Pierre-Louis Bossart wrote:
> Hi Vinod,
> 
> > > +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;
> > 
> > where is this struct sdw_intel_stream_free_data defined. I dont see it
> > in this patch or this series..
> 
> the definition is already upstream :-)

Oops did i look at wrong branch, sorry!

> > > +	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);
> > 
> > I think, free the 'name' here would be apt
> 
> Right, this is something we discussed with Rander shortly before Chinese New
> Year and we wanted to handle this with a follow-up patch, would that work
> for you? if not I can send a v3, your choice.

It would be better if we fix this up in this series :)
Pierre-Louis Bossart Feb. 13, 2020, 3:05 p.m. UTC | #4
>>>> +	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);
>>>
>>> I think, free the 'name' here would be apt
>>
>> Right, this is something we discussed with Rander shortly before Chinese New
>> Year and we wanted to handle this with a follow-up patch, would that work
>> for you? if not I can send a v3, your choice.
> 
> It would be better if we fix this up in this series :)

ok, I will fix this later today.

Patch
diff mbox series

diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index c498812522ab..e0c1fff7c4a0 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -550,6 +550,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
  */
@@ -817,6 +836,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;
 
@@ -824,12 +844,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,