[v2] ASOC: dapm: add params_select callback

Commit Message

[e-mail] Feb. 27, 2014, 4:18 p.m. UTC

From: nikesh <[e-mail]>

dai-link params for codec-codec links were fixed. The fixed link between
codec and another chip which may be another codec, baseband, bluetooth
codec etc may require run time configuaration changes. This change
provides an optional callback to select one of the param from a list
of params.

Signed-off-by: nikesh <[e-mail]>
---

Change since v1:
Used an index to select from a list of params, instead of
overwriting the param.

Thanks,
Nikesh

 include/sound/soc-dapm.h |    3 ++-
 include/sound/soc.h      |    3 +++
 sound/soc/soc-core.c     |    4 ++--
 sound/soc/soc-dapm.c     |   20 ++++++++++++++++++--
 4 files changed, 25 insertions(+), 5 deletions(-)

Comments

Mark Brown Feb. 28, 2014, 2:23 a.m. UTC | #1

On Thu, Feb 27, 2014 at 04:18:20PM +0000, [e-mail] wrote:
> From: nikesh <[e-mail]>

As I said in my reply to your earlier patch:

| You need to fix both your git and e-mail setups, you should be using
| "Nikesh Oswal" or similar as your real name and more importantly your

Please don't ignore review comments, it tends not to be helpful.

Please do also use subject lines matching the style for the subsystem.

> dai-link params for codec-codec links were fixed. The fixed link between
> codec and another chip which may be another codec, baseband, bluetooth
> codec etc may require run time configuaration changes. This change
> provides an optional callback to select one of the param from a list
> of params.

OK, so the question now is why is this being done using a callback and
why is that callback picking from a list of predefined configurations?
You've not motivated this decision at all and it's not obvious to me
that it's the best approach (for example, why not just let the machine
driver set the parameters at any time rather than have it wait for a
callback)?

The picking from a list is especially odd, what I said in my previous
review was:

| If you look at the existing interface you'll also see that it takes an
| array of parameters rather than just a single parameter.  The idea was
| to extend the interface to provide a control to userspace allowing
| selection of one of the configurations from a list for use with cases
| like modems which can switch between 8kHz and 16kHz modes.

but if the selection is done purely in kernel space and we're not
constructing an enum then it's less clear that this is helpful.

Patch

diff --git a/include/sound/soc-dapm.h b/include/sound/soc-dapm.h
index 68d92e3..12b0d9e 100644
--- a/include/sound/soc-dapm.h
+++ b/include/sound/soc-dapm.h
@@ -416,7 +416,8 @@  void snd_soc_dapm_connect_dai_link_widgets(struct snd_soc_card *card);
 int snd_soc_dapm_new_pcm(struct snd_soc_card *card,
 			 const struct snd_soc_pcm_stream *params,
 			 struct snd_soc_dapm_widget *source,
-			 struct snd_soc_dapm_widget *sink);
+			 struct snd_soc_dapm_widget *sink,
+			 struct snd_soc_dai_link *dai_link);
 
 /* dapm path setup */
 int snd_soc_dapm_new_widgets(struct snd_soc_card *card);
diff --git a/include/sound/soc.h b/include/sound/soc.h
index 9a00147..20110aa 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -876,6 +876,9 @@  struct snd_soc_dai_link {
 	int be_id;	/* optional ID for machine driver BE identification */
 
 	const struct snd_soc_pcm_stream *params;
+	/* optional params selection for dai links */
+	int (*params_select)(const struct snd_soc_pcm_stream *params,
+			unsigned int *param_index, int event);
 
 	unsigned int dai_fmt;           /* format to set on init */
 
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index fe1df50..4f03e88 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -1469,7 +1469,7 @@  static int soc_probe_link_dais(struct snd_soc_card *card, int num, int order)
 			capture_w = cpu_dai->capture_widget;
 			if (play_w && capture_w) {
 				ret = snd_soc_dapm_new_pcm(card, dai_link->params,
-						   capture_w, play_w);
+						   capture_w, play_w, dai_link);
 				if (ret != 0) {
 					dev_err(card->dev, "ASoC: Can't link %s to %s: %d\n",
 						play_w->name, capture_w->name, ret);
@@ -1481,7 +1481,7 @@  static int soc_probe_link_dais(struct snd_soc_card *card, int num, int order)
 			capture_w = codec_dai->capture_widget;
 			if (play_w && capture_w) {
 				ret = snd_soc_dapm_new_pcm(card, dai_link->params,
-						   capture_w, play_w);
+						   capture_w, play_w, dai_link);
 				if (ret != 0) {
 					dev_err(card->dev, "ASoC: Can't link %s to %s: %d\n",
 						play_w->name, capture_w->name, ret);
diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
index dc8ff13..2e2f633 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -3379,10 +3379,12 @@  static int snd_soc_dai_link_event(struct snd_soc_dapm_widget *w,
 	struct snd_soc_dapm_path *source_p, *sink_p;
 	struct snd_soc_dai *source, *sink;
 	const struct snd_soc_pcm_stream *config = w->params;
+	struct snd_soc_dai_link *dai_link = w->priv;
 	struct snd_pcm_substream substream;
 	struct snd_pcm_hw_params *params = NULL;
 	u64 fmt;
-	int ret;
+	int ret = 0;
+	unsigned int param_index;
 
 	if (WARN_ON(!config) ||
 	    WARN_ON(list_empty(&w->sources) || list_empty(&w->sinks)))
@@ -3402,6 +3404,18 @@  static int snd_soc_dai_link_event(struct snd_soc_dapm_widget *w,
 	source = source_p->source->priv;
 	sink = sink_p->sink->priv;
 
+	if (dai_link && dai_link->params_select) {
+		ret = dai_link->params_select(config, &param_index, event);
+		if (ret < 0) {
+			dev_err(w->dapm->dev,
+			"ASoC: params_select for dai link widget failed %d\n",
+			ret);
+			goto out;
+		}
+		if (param_index > 0)
+			config += param_index;
+	}
+
 	/* Be a little careful as we don't want to overflow the mask array */
 	if (config->formats) {
 		fmt = ffs(config->formats) - 1;
@@ -3485,7 +3499,8 @@  out:
 int snd_soc_dapm_new_pcm(struct snd_soc_card *card,
 			 const struct snd_soc_pcm_stream *params,
 			 struct snd_soc_dapm_widget *source,
-			 struct snd_soc_dapm_widget *sink)
+			 struct snd_soc_dapm_widget *sink,
+			 struct snd_soc_dai_link *dai_link)
 {
 	struct snd_soc_dapm_route routes[2];
 	struct snd_soc_dapm_widget template;
@@ -3517,6 +3532,7 @@  int snd_soc_dapm_new_pcm(struct snd_soc_card *card,
 	}
 
 	w->params = params;
+	w->priv = (void *)dai_link;
 
 	memset(&routes, 0, sizeof(routes));