All of lore.kernel.org
 help / color / mirror / Atom feed
From: Charles Keepax <ckeepax@opensource.cirrus.com>
To: broonie@kernel.org
Cc: vinod.koul@intel.com, patches@opensource.cirrus.com,
	alsa-devel@alsa-project.org, lgirdwood@gmail.com,
	kuninori.morimoto.gx@renesas.com
Subject: [PATCH 2/4] ASoC: compress: Clarify the intent of current compressed ops handling
Date: Thu, 26 Apr 2018 17:30:05 +0100	[thread overview]
Message-ID: <20180426163007.5632-2-ckeepax@opensource.cirrus.com> (raw)
In-Reply-To: <20180426163007.5632-1-ckeepax@opensource.cirrus.com>

It is proposed that the model adopted for compressed component
support currently should be that multiple components are supported
on a compressed DAI but that they must provide a unified set of
capabilities. So for example having multiple components involved in
the decode is fine but the core will not presently attempt to make
smart decisions like offloading MP3 to this component and AAC another.

To implement this it is suggested that callbacks configuring the state
of the components (trigger, set_params, ack and set_metadata) should be
called on all components and required to succeed on all components
before being considered to have succeeded. However for callbacks that
return information from the driver (copy, get_metadata, pointer,
get_codec_caps, get_caps and get_params) it is expected that either
there is only a single provider on the link or that all components
will return identical results.

Essentially this matches the current implementation of the code and
only small clean ups are undertaken here.

For callbacks configuring the state of the components simplify the
code a little and make intention clearer by aborting as soon as an
error is encountered. The operation has already failed and there is
nothing to be gained from processing the additional callbacks.

For callbacks returning information from the driver only look for the
first callback provided, currently the code will call every callback
only returning the information provided by the last. Again this makes
the currently supported feature set a little more clear.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 sound/soc/soc-compress.c | 105 +++++++++++++++++++++--------------------------
 1 file changed, 47 insertions(+), 58 deletions(-)

diff --git a/sound/soc/soc-compress.c b/sound/soc/soc-compress.c
index 62875c6a93a14..b411143f21ccc 100644
--- a/sound/soc/soc-compress.c
+++ b/sound/soc/soc-compress.c
@@ -363,7 +363,7 @@ static int soc_compr_trigger(struct snd_compr_stream *cstream, int cmd)
 	struct snd_soc_rtdcom_list *rtdcom;
 	struct snd_soc_dai *codec_dai = rtd->codec_dai;
 	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
-	int ret = 0, __ret;
+	int ret = 0;
 
 	mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
 
@@ -374,12 +374,10 @@ static int soc_compr_trigger(struct snd_compr_stream *cstream, int cmd)
 		    !component->driver->compr_ops->trigger)
 			continue;
 
-		__ret = component->driver->compr_ops->trigger(cstream, cmd);
-		if (__ret < 0)
-			ret = __ret;
+		ret = component->driver->compr_ops->trigger(cstream, cmd);
+		if (ret < 0)
+			goto out;
 	}
-	if (ret < 0)
-		goto out;
 
 	if (cpu_dai->driver->cops && cpu_dai->driver->cops->trigger)
 		cpu_dai->driver->cops->trigger(cstream, cmd, cpu_dai);
@@ -404,7 +402,7 @@ static int soc_compr_trigger_fe(struct snd_compr_stream *cstream, int cmd)
 	struct snd_soc_component *component;
 	struct snd_soc_rtdcom_list *rtdcom;
 	struct snd_soc_dai *cpu_dai = fe->cpu_dai;
-	int ret = 0, __ret, stream;
+	int ret, stream;
 
 	if (cmd == SND_COMPR_TRIGGER_PARTIAL_DRAIN ||
 		cmd == SND_COMPR_TRIGGER_DRAIN) {
@@ -416,9 +414,9 @@ static int soc_compr_trigger_fe(struct snd_compr_stream *cstream, int cmd)
 			    !component->driver->compr_ops->trigger)
 				continue;
 
-			__ret = component->driver->compr_ops->trigger(cstream, cmd);
-			if (__ret < 0)
-				ret = __ret;
+			ret = component->driver->compr_ops->trigger(cstream, cmd);
+			if (ret < 0)
+				return ret;
 		}
 		return ret;
 	}
@@ -444,12 +442,10 @@ static int soc_compr_trigger_fe(struct snd_compr_stream *cstream, int cmd)
 		    !component->driver->compr_ops->trigger)
 			continue;
 
-		__ret = component->driver->compr_ops->trigger(cstream, cmd);
-		if (__ret < 0)
-			ret = __ret;
+		ret = component->driver->compr_ops->trigger(cstream, cmd);
+		if (ret < 0)
+			goto out;
 	}
-	if (ret < 0)
-		goto out;
 
 	fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_FE;
 
@@ -483,7 +479,7 @@ static int soc_compr_set_params(struct snd_compr_stream *cstream,
 	struct snd_soc_component *component;
 	struct snd_soc_rtdcom_list *rtdcom;
 	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
-	int ret = 0, __ret;
+	int ret;
 
 	mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
 
@@ -507,12 +503,10 @@ static int soc_compr_set_params(struct snd_compr_stream *cstream,
 		    !component->driver->compr_ops->set_params)
 			continue;
 
-		__ret = component->driver->compr_ops->set_params(cstream, params);
-		if (__ret < 0)
-			ret = __ret;
+		ret = component->driver->compr_ops->set_params(cstream, params);
+		if (ret < 0)
+			goto err;
 	}
-	if (ret < 0)
-		goto err;
 
 	if (rtd->dai_link->compr_ops && rtd->dai_link->compr_ops->set_params) {
 		ret = rtd->dai_link->compr_ops->set_params(cstream);
@@ -533,7 +527,7 @@ static int soc_compr_set_params(struct snd_compr_stream *cstream,
 
 	cancel_delayed_work_sync(&rtd->delayed_work);
 
-	return ret;
+	return 0;
 
 err:
 	mutex_unlock(&rtd->pcm_mutex);
@@ -549,7 +543,7 @@ static int soc_compr_set_params_fe(struct snd_compr_stream *cstream,
 	struct snd_soc_component *component;
 	struct snd_soc_rtdcom_list *rtdcom;
 	struct snd_soc_dai *cpu_dai = fe->cpu_dai;
-	int ret = 0, __ret, stream;
+	int ret, stream;
 
 	if (cstream->direction == SND_COMPRESS_PLAYBACK)
 		stream = SNDRV_PCM_STREAM_PLAYBACK;
@@ -571,12 +565,10 @@ static int soc_compr_set_params_fe(struct snd_compr_stream *cstream,
 		    !component->driver->compr_ops->set_params)
 			continue;
 
-		__ret = component->driver->compr_ops->set_params(cstream, params);
-		if (__ret < 0)
-			ret = __ret;
+		ret = component->driver->compr_ops->set_params(cstream, params);
+		if (ret < 0)
+			goto out;
 	}
-	if (ret < 0)
-		goto out;
 
 	if (fe->dai_link->compr_ops && fe->dai_link->compr_ops->set_params) {
 		ret = fe->dai_link->compr_ops->set_params(cstream);
@@ -618,7 +610,7 @@ static int soc_compr_get_params(struct snd_compr_stream *cstream,
 	struct snd_soc_component *component;
 	struct snd_soc_rtdcom_list *rtdcom;
 	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
-	int ret = 0, __ret;
+	int ret = 0;
 
 	mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
 
@@ -635,9 +627,8 @@ static int soc_compr_get_params(struct snd_compr_stream *cstream,
 		    !component->driver->compr_ops->get_params)
 			continue;
 
-		__ret = component->driver->compr_ops->get_params(cstream, params);
-		if (__ret < 0)
-			ret = __ret;
+		ret = component->driver->compr_ops->get_params(cstream, params);
+		break;
 	}
 
 err:
@@ -651,7 +642,7 @@ static int soc_compr_get_caps(struct snd_compr_stream *cstream,
 	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
 	struct snd_soc_component *component;
 	struct snd_soc_rtdcom_list *rtdcom;
-	int ret = 0, __ret;
+	int ret = 0;
 
 	mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
 
@@ -662,9 +653,8 @@ static int soc_compr_get_caps(struct snd_compr_stream *cstream,
 		    !component->driver->compr_ops->get_caps)
 			continue;
 
-		__ret = component->driver->compr_ops->get_caps(cstream, caps);
-		if (__ret < 0)
-			ret = __ret;
+		ret = component->driver->compr_ops->get_caps(cstream, caps);
+		break;
 	}
 
 	mutex_unlock(&rtd->pcm_mutex);
@@ -677,7 +667,7 @@ static int soc_compr_get_codec_caps(struct snd_compr_stream *cstream,
 	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
 	struct snd_soc_component *component;
 	struct snd_soc_rtdcom_list *rtdcom;
-	int ret = 0, __ret;
+	int ret = 0;
 
 	mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
 
@@ -688,9 +678,9 @@ static int soc_compr_get_codec_caps(struct snd_compr_stream *cstream,
 		    !component->driver->compr_ops->get_codec_caps)
 			continue;
 
-		__ret = component->driver->compr_ops->get_codec_caps(cstream, codec);
-		if (__ret < 0)
-			ret = __ret;
+		ret = component->driver->compr_ops->get_codec_caps(cstream,
+								   codec);
+		break;
 	}
 
 	mutex_unlock(&rtd->pcm_mutex);
@@ -703,7 +693,7 @@ static int soc_compr_ack(struct snd_compr_stream *cstream, size_t bytes)
 	struct snd_soc_component *component;
 	struct snd_soc_rtdcom_list *rtdcom;
 	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
-	int ret = 0, __ret;
+	int ret = 0;
 
 	mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
 
@@ -720,9 +710,9 @@ static int soc_compr_ack(struct snd_compr_stream *cstream, size_t bytes)
 		    !component->driver->compr_ops->ack)
 			continue;
 
-		__ret = component->driver->compr_ops->ack(cstream, bytes);
-		if (__ret < 0)
-			ret = __ret;
+		ret = component->driver->compr_ops->ack(cstream, bytes);
+		if (ret < 0)
+			goto err;
 	}
 
 err:
@@ -736,7 +726,7 @@ static int soc_compr_pointer(struct snd_compr_stream *cstream,
 	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
 	struct snd_soc_component *component;
 	struct snd_soc_rtdcom_list *rtdcom;
-	int ret = 0, __ret;
+	int ret = 0;
 	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
 
 	mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
@@ -751,9 +741,8 @@ static int soc_compr_pointer(struct snd_compr_stream *cstream,
 		    !component->driver->compr_ops->pointer)
 			continue;
 
-		__ret = component->driver->compr_ops->pointer(cstream, tstamp);
-		if (__ret < 0)
-			ret = __ret;
+		ret = component->driver->compr_ops->pointer(cstream, tstamp);
+		break;
 	}
 
 	mutex_unlock(&rtd->pcm_mutex);
@@ -792,7 +781,7 @@ static int soc_compr_set_metadata(struct snd_compr_stream *cstream,
 	struct snd_soc_component *component;
 	struct snd_soc_rtdcom_list *rtdcom;
 	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
-	int ret = 0, __ret;
+	int ret;
 
 	if (cpu_dai->driver->cops && cpu_dai->driver->cops->set_metadata) {
 		ret = cpu_dai->driver->cops->set_metadata(cstream, metadata, cpu_dai);
@@ -807,12 +796,13 @@ static int soc_compr_set_metadata(struct snd_compr_stream *cstream,
 		    !component->driver->compr_ops->set_metadata)
 			continue;
 
-		__ret = component->driver->compr_ops->set_metadata(cstream, metadata);
-		if (__ret < 0)
-			ret = __ret;
+		ret = component->driver->compr_ops->set_metadata(cstream,
+								 metadata);
+		if (ret < 0)
+			return ret;
 	}
 
-	return ret;
+	return 0;
 }
 
 static int soc_compr_get_metadata(struct snd_compr_stream *cstream,
@@ -822,7 +812,7 @@ static int soc_compr_get_metadata(struct snd_compr_stream *cstream,
 	struct snd_soc_component *component;
 	struct snd_soc_rtdcom_list *rtdcom;
 	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
-	int ret = 0, __ret;
+	int ret;
 
 	if (cpu_dai->driver->cops && cpu_dai->driver->cops->get_metadata) {
 		ret = cpu_dai->driver->cops->get_metadata(cstream, metadata, cpu_dai);
@@ -837,12 +827,11 @@ static int soc_compr_get_metadata(struct snd_compr_stream *cstream,
 		    !component->driver->compr_ops->get_metadata)
 			continue;
 
-		__ret = component->driver->compr_ops->get_metadata(cstream, metadata);
-		if (__ret < 0)
-			ret = __ret;
+		return component->driver->compr_ops->get_metadata(cstream,
+								  metadata);
 	}
 
-	return ret;
+	return 0;
 }
 
 /* ASoC Compress operations */
-- 
2.11.0

  reply	other threads:[~2018-04-26 16:30 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-26 16:30 [PATCH 1/4] ASoC: compress: Only assign compr->ops->copy once Charles Keepax
2018-04-26 16:30 ` Charles Keepax [this message]
2018-05-03 16:27   ` [PATCH 2/4] ASoC: compress: Clarify the intent of current compressed ops handling Vinod Koul
2018-05-04 11:59     ` Charles Keepax
2018-05-04 12:04       ` Charles Keepax
2018-05-08  8:33       ` Vinod Koul
2018-05-08 10:52         ` Charles Keepax
2018-05-08 12:04           ` Vinod Koul
2018-05-08 13:09             ` Charles Keepax
2018-04-26 16:30 ` [PATCH 3/4] ASoC: compress: Add helper functions for component trigger/set_params Charles Keepax
2018-04-26 16:30 ` [PATCH 4/4] ASoC: compress: Fix up some trivial formatting issues Charles Keepax
2018-05-11  3:25   ` Applied "ASoC: compress: Fix up some trivial formatting issues" to the asoc tree Mark Brown
2018-05-03 16:28 ` [PATCH 1/4] ASoC: compress: Only assign compr->ops->copy once Vinod Koul
2018-05-11  3:25 ` Applied "ASoC: compress: Only assign compr->ops->copy once" to the asoc tree Mark Brown

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180426163007.5632-2-ckeepax@opensource.cirrus.com \
    --to=ckeepax@opensource.cirrus.com \
    --cc=alsa-devel@alsa-project.org \
    --cc=broonie@kernel.org \
    --cc=kuninori.morimoto.gx@renesas.com \
    --cc=lgirdwood@gmail.com \
    --cc=patches@opensource.cirrus.com \
    --cc=vinod.koul@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.