lkml.org 
[lkml]   [2019]   [Nov]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 13/14] soundwire: intel: free all resources on hw_free()
From
Date
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@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 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.

>
> static void intel_shutdown(struct snd_pcm_substream *substream,
>

\
 
 \ /
  Last update: 2019-11-04 21:09    [W:0.167 / U:0.152 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site