diff mbox

[02/12] ASoC: dapm: Implement stereo mixer control support

Message ID 20161003110804.28235-3-wens@csie.org (mailing list archive)
State Superseded, archived
Headers show

Commit Message

Chen-Yu Tsai Oct. 3, 2016, 11:07 a.m. UTC 
While DAPM is mono or single channel, its controls can be shared between
widgets, such as sharing one stereo mixer control between the left and
right channel widgets.

This patch introduces support for such shared mixer controls.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 sound/soc/soc-dapm.c | 103 ++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 74 insertions(+), 29 deletions(-)

Comments

Mark Brown Oct. 26, 2016, 4:57 p.m. UTC | #1 
On Mon, Oct 03, 2016 at 07:07:54PM +0800, Chen-Yu Tsai wrote:

> While DAPM is mono or single channel, its controls can be shared between
> widgets, such as sharing one stereo mixer control between the left and
> right channel widgets.

> This patch introduces support for such shared mixer controls.

Based on this changelog I'm really not sure what the intended semantic
of this change is which makes it difficult to review.  What are you
expecting these controls to look like and how are you expecting them to
work?

> -static void dapm_set_mixer_path_status(struct snd_soc_dapm_path *p, int i)
> +static void dapm_set_mixer_path_status(struct snd_soc_dapm_path *p, int i,
> +				       int nth_path)

It looks like the goal is to attach more than one path to a single
control somehow?
Mark Brown Oct. 26, 2016, 5:50 p.m. UTC | #2 
On Mon, Oct 03, 2016 at 07:07:54PM +0800, Chen-Yu Tsai wrote:

>  	/* find dapm widget path assoc with kcontrol */
>  	dapm_kcontrol_for_each_path(path, kcontrol) {
> +		/*
> +		 * If status for the second channel was given ( >= 0 ),
> +		 * consider the second and later paths as the second
> +		 * channel.
> +		 */
> +		if (found && rconnect >= 0)
> +			soc_dapm_connect_path(path, rconnect, "mixer update");
> +		else
> +			soc_dapm_connect_path(path, connect, "mixer update");
>  		found = 1;
> -		soc_dapm_connect_path(path, connect, "mixer update");

This really only works for two channels with the current inteface - the
comment makes it sound like it'll work for more but we can only pass in
two (and there's only support for specifying two everywhere).

> -	change = dapm_kcontrol_set_value(kcontrol, val);
> +	/* This assumes field width < (bits in unsigned int / 2) */
> +	change = dapm_kcontrol_set_value(kcontrol, val | (rval << width));

That seems like a bit of an assumption in cases where we've got a single
control for both power and volume?  They're very rare though, I'm not
even sure any exist.  It'd be good to have a check in the code just in
case it does come up, it'll likely be a nightmare to debug if someone
does run into it.

Otherwise I think this makes sense.
Chen-Yu Tsai Oct. 27, 2016, 1:20 a.m. UTC | #3 
On Thu, Oct 27, 2016 at 12:57 AM, Mark Brown <broonie@kernel.org> wrote:
> On Mon, Oct 03, 2016 at 07:07:54PM +0800, Chen-Yu Tsai wrote:
>
>> While DAPM is mono or single channel, its controls can be shared between
>> widgets, such as sharing one stereo mixer control between the left and
>> right channel widgets.
>
>> This patch introduces support for such shared mixer controls.
>
> Based on this changelog I'm really not sure what the intended semantic
> of this change is which makes it difficult to review.  What are you
> expecting these controls to look like and how are you expecting them to
> work?
>
>> -static void dapm_set_mixer_path_status(struct snd_soc_dapm_path *p, int i)
>> +static void dapm_set_mixer_path_status(struct snd_soc_dapm_path *p, int i,
>> +                                    int nth_path)
>
> It looks like the goal is to attach more than one path to a single
> control somehow?

Correct. I'll try to expand the commit log and add an example diagram.

ChenYu
Chen-Yu Tsai Oct. 27, 2016, 2:02 p.m. UTC | #4 
On Thu, Oct 27, 2016 at 1:50 AM, Mark Brown <broonie@kernel.org> wrote:
> On Mon, Oct 03, 2016 at 07:07:54PM +0800, Chen-Yu Tsai wrote:
>
>>       /* find dapm widget path assoc with kcontrol */
>>       dapm_kcontrol_for_each_path(path, kcontrol) {
>> +             /*
>> +              * If status for the second channel was given ( >= 0 ),
>> +              * consider the second and later paths as the second
>> +              * channel.
>> +              */
>> +             if (found && rconnect >= 0)
>> +                     soc_dapm_connect_path(path, rconnect, "mixer update");
>> +             else
>> +                     soc_dapm_connect_path(path, connect, "mixer update");
>>               found = 1;
>> -             soc_dapm_connect_path(path, connect, "mixer update");
>
> This really only works for two channels with the current inteface - the
> comment makes it sound like it'll work for more but we can only pass in
> two (and there's only support for specifying two everywhere).

I could rework it to pass a list of connected status' and the number
of elements instead, but it wouldn't work well for situations where
the number of channels on the kcontrol != the number of paths. I'm not
sure if that's even a valid setup, but it does work with the current
core code.

On the other hand, are there kcontrols that are multi-channel
(> 2 channels)?

I'm inclined to just fixup the comment to make it clear that the
implementation supports stereo, i.e. 2 channels, only.

>
>> -     change = dapm_kcontrol_set_value(kcontrol, val);
>> +     /* This assumes field width < (bits in unsigned int / 2) */
>> +     change = dapm_kcontrol_set_value(kcontrol, val | (rval << width));
>
> That seems like a bit of an assumption in cases where we've got a single
> control for both power and volume?  They're very rare though, I'm not
> even sure any exist.  It'd be good to have a check in the code just in
> case it does come up, it'll likely be a nightmare to debug if someone
> does run into it.

Agreed. I'll put a check and warning before it.

>
> Otherwise I think this makes sense.

Thanks for the review!

Regards
ChenYu
diff mbox

Patch

diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
index 535c20e4fac9..1739018ef81d 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -723,7 +723,8 @@  static int dapm_connect_mux(struct snd_soc_dapm_context *dapm,
 }
 
 /* set up initial codec paths */
-static void dapm_set_mixer_path_status(struct snd_soc_dapm_path *p, int i)
+static void dapm_set_mixer_path_status(struct snd_soc_dapm_path *p, int i,
+				       int nth_path)
 {
 	struct soc_mixer_control *mc = (struct soc_mixer_control *)
 		p->sink->kcontrol_news[i].private_value;
@@ -736,7 +737,13 @@  static void dapm_set_mixer_path_status(struct snd_soc_dapm_path *p, int i)
 
 	if (reg != SND_SOC_NOPM) {
 		soc_dapm_read(p->sink->dapm, reg, &val);
-		val = (val >> shift) & mask;
+		if (snd_soc_volsw_is_stereo(mc) && nth_path > 0) {
+			if (reg != mc->rreg)
+				soc_dapm_read(p->sink->dapm, mc->rreg, &val);
+			val = (val >> mc->rshift) & mask;
+		} else {
+			val = (val >> shift) & mask;
+		}
 		if (invert)
 			val = max - val;
 		p->connect = !!val;
@@ -749,13 +756,13 @@  static void dapm_set_mixer_path_status(struct snd_soc_dapm_path *p, int i)
 static int dapm_connect_mixer(struct snd_soc_dapm_context *dapm,
 	struct snd_soc_dapm_path *path, const char *control_name)
 {
-	int i;
+	int i, nth_path = 0;
 
 	/* search for mixer kcontrol */
 	for (i = 0; i < path->sink->num_kcontrols; i++) {
 		if (!strcmp(control_name, path->sink->kcontrol_news[i].name)) {
 			path->name = path->sink->kcontrol_news[i].name;
-			dapm_set_mixer_path_status(path, i);
+			dapm_set_mixer_path_status(path, i, nth_path++);
 			return 0;
 		}
 	}
@@ -2195,7 +2202,8 @@  EXPORT_SYMBOL_GPL(snd_soc_dapm_mux_update_power);
 
 /* test and update the power status of a mixer or switch widget */
 static int soc_dapm_mixer_update_power(struct snd_soc_card *card,
-				   struct snd_kcontrol *kcontrol, int connect)
+				       struct snd_kcontrol *kcontrol,
+				       int connect, int rconnect)
 {
 	struct snd_soc_dapm_path *path;
 	int found = 0;
@@ -2204,8 +2212,16 @@  static int soc_dapm_mixer_update_power(struct snd_soc_card *card,
 
 	/* find dapm widget path assoc with kcontrol */
 	dapm_kcontrol_for_each_path(path, kcontrol) {
+		/*
+		 * If status for the second channel was given ( >= 0 ),
+		 * consider the second and later paths as the second
+		 * channel.
+		 */
+		if (found && rconnect >= 0)
+			soc_dapm_connect_path(path, rconnect, "mixer update");
+		else
+			soc_dapm_connect_path(path, connect, "mixer update");
 		found = 1;
-		soc_dapm_connect_path(path, connect, "mixer update");
 	}
 
 	if (found)
@@ -2223,7 +2239,7 @@  int snd_soc_dapm_mixer_update_power(struct snd_soc_dapm_context *dapm,
 
 	mutex_lock_nested(&card->dapm_mutex, SND_SOC_DAPM_CLASS_RUNTIME);
 	card->update = update;
-	ret = soc_dapm_mixer_update_power(card, kcontrol, connect);
+	ret = soc_dapm_mixer_update_power(card, kcontrol, connect, -1);
 	card->update = NULL;
 	mutex_unlock(&card->dapm_mutex);
 	if (ret > 0)
@@ -3048,22 +3064,28 @@  int snd_soc_dapm_get_volsw(struct snd_kcontrol *kcontrol,
 	int reg = mc->reg;
 	unsigned int shift = mc->shift;
 	int max = mc->max;
+	unsigned int width = fls(max);
 	unsigned int mask = (1 << fls(max)) - 1;
 	unsigned int invert = mc->invert;
-	unsigned int val;
+	unsigned int reg_val, val, rval = 0;
 	int ret = 0;
 
-	if (snd_soc_volsw_is_stereo(mc))
-		dev_warn(dapm->dev,
-			 "ASoC: Control '%s' is stereo, which is not supported\n",
-			 kcontrol->id.name);
-
 	mutex_lock_nested(&card->dapm_mutex, SND_SOC_DAPM_CLASS_RUNTIME);
 	if (dapm_kcontrol_is_powered(kcontrol) && reg != SND_SOC_NOPM) {
-		ret = soc_dapm_read(dapm, reg, &val);
-		val = (val >> shift) & mask;
+		ret = soc_dapm_read(dapm, reg, &reg_val);
+		val = (reg_val >> shift) & mask;
+
+		if (ret == 0 && reg != mc->rreg)
+			ret = soc_dapm_read(dapm, mc->rreg, &reg_val);
+
+		if (snd_soc_volsw_is_stereo(mc))
+			rval = (reg_val >> mc->rshift) & mask;
 	} else {
-		val = dapm_kcontrol_get_value(kcontrol);
+		reg_val = dapm_kcontrol_get_value(kcontrol);
+		val = reg_val & mask;
+
+		if (snd_soc_volsw_is_stereo(mc))
+			rval = (reg_val >> width) & mask;
 	}
 	mutex_unlock(&card->dapm_mutex);
 
@@ -3075,6 +3097,13 @@  int snd_soc_dapm_get_volsw(struct snd_kcontrol *kcontrol,
 	else
 		ucontrol->value.integer.value[0] = val;
 
+	if (snd_soc_volsw_is_stereo(mc)) {
+		if (invert)
+			ucontrol->value.integer.value[1] = max - rval;
+		else
+			ucontrol->value.integer.value[1] = rval;
+	}
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(snd_soc_dapm_get_volsw);
@@ -3098,46 +3127,62 @@  int snd_soc_dapm_put_volsw(struct snd_kcontrol *kcontrol,
 	int reg = mc->reg;
 	unsigned int shift = mc->shift;
 	int max = mc->max;
-	unsigned int mask = (1 << fls(max)) - 1;
+	unsigned int width = fls(max);
+	unsigned int mask = (1 << width) - 1;
 	unsigned int invert = mc->invert;
-	unsigned int val;
-	int connect, change, reg_change = 0;
+	unsigned int val, rval = 0;
+	int connect, rconnect = -1, change, reg_change = 0;
 	struct snd_soc_dapm_update update = { 0 };
 	int ret = 0;
 
-	if (snd_soc_volsw_is_stereo(mc))
-		dev_warn(dapm->dev,
-			 "ASoC: Control '%s' is stereo, which is not supported\n",
-			 kcontrol->id.name);
-
 	val = (ucontrol->value.integer.value[0] & mask);
 	connect = !!val;
 
 	if (invert)
 		val = max - val;
 
+	if (snd_soc_volsw_is_stereo(mc)) {
+		rval = (ucontrol->value.integer.value[1] & mask);
+		rconnect = !!rval;
+		if (invert)
+			rval = max - rval;
+	}
+
 	mutex_lock_nested(&card->dapm_mutex, SND_SOC_DAPM_CLASS_RUNTIME);
 
-	change = dapm_kcontrol_set_value(kcontrol, val);
+	/* This assumes field width < (bits in unsigned int / 2) */
+	change = dapm_kcontrol_set_value(kcontrol, val | (rval << width));
 
 	if (reg != SND_SOC_NOPM) {
-		mask = mask << shift;
 		val = val << shift;
+		rval = rval << mc->rshift;
 
-		reg_change = soc_dapm_test_bits(dapm, reg, mask, val);
+		reg_change = soc_dapm_test_bits(dapm, reg, mask << shift, val);
+
+		if (snd_soc_volsw_is_stereo(mc))
+			reg_change |= soc_dapm_test_bits(dapm, mc->rreg,
+							 mask << mc->rshift,
+							 rval);
 	}
 
 	if (change || reg_change) {
 		if (reg_change) {
+			if (snd_soc_volsw_is_stereo(mc)) {
+				update.has_second_set = true;
+				update.reg2 = mc->rreg;
+				update.mask2 = mask << mc->rshift;
+				update.val2 = rval;
+			}
 			update.kcontrol = kcontrol;
 			update.reg = reg;
-			update.mask = mask;
+			update.mask = mask << shift;
 			update.val = val;
 			card->update = &update;
 		}
 		change |= reg_change;
 
-		ret = soc_dapm_mixer_update_power(card, kcontrol, connect);
+		ret = soc_dapm_mixer_update_power(card, kcontrol, connect,
+						  rconnect);
 
 		card->update = NULL;
 	}