[alsa-devel] [PATCH] ASoC: dapm: Fix snd_soc_dapm_put_volsw() connect
snd_soc_dapm_put_volsw() sets connect incorrectly in the case max > 1 with invert. In that case, the raw disconnect value should be max, which corresponds to the userspace value 0. This use case currently does not appear upstream, but it could break SOC_DAPM_SINGLE() or SOC_DAPM_SINGLE_TLV() elsewhere or in the future. Cc: Liam Girdwood <lrg@ti.com> Cc: Mark Brown <broonie@opensource.wolfsonmicro.com> Cc: <alsa-devel@alsa-project.org> Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com> --- .../sound/soc/soc-dapm.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git linux-next-HEAD-6c86b58.orig/sound/soc/soc-dapm.c linux-next-HEAD-6c86b58/sound/soc/soc-dapm.c index 405841c..5ef082f 100644 --- linux-next-HEAD-6c86b58.orig/sound/soc/soc-dapm.c +++ linux-next-HEAD-6c86b58/sound/soc/soc-dapm.c @@ -2515,19 +2515,13 @@ int snd_soc_dapm_put_volsw(struct snd_kcontrol *kcontrol, int wi; val = (ucontrol->value.integer.value[0] & mask); + connect = !!val; if (invert) val = max - val; mask = mask << shift; val = val << shift; - if (val) - /* new connection */ - connect = invert ? 0 : 1; - else - /* old connection must be powered down */ - connect = invert ? 1 : 0; - mutex_lock_nested(&card->dapm_mutex, SND_SOC_DAPM_CLASS_RUNTIME); change = snd_soc_test_bits(widget->codec, reg, mask, val);
On Mon, Jun 18, 2012 at 10:41:28PM +0200, Benoît Thébaudeau wrote:
snd_soc_dapm_put_volsw() sets connect incorrectly in the case max > 1 with invert. In that case, the raw disconnect value should be max, which corresponds to the userspace value 0.
Applied, thanks.
At Mon, 18 Jun 2012 22:41:28 +0200 (CEST), Benoît Thébaudeau wrote:
snd_soc_dapm_put_volsw() sets connect incorrectly in the case max > 1 with invert. In that case, the raw disconnect value should be max, which corresponds to the userspace value 0.
This use case currently does not appear upstream, but it could break SOC_DAPM_SINGLE() or SOC_DAPM_SINGLE_TLV() elsewhere or in the future.
Cc: Liam Girdwood <lrg@ti.com> Cc: Mark Brown <broonie@opensource.wolfsonmicro.com> Cc: <alsa-devel@alsa-project.org> Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com> --- .../sound/soc/soc-dapm.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)
diff --git linux-next-HEAD-6c86b58.orig/sound/soc/soc-dapm.c linux-next-HEAD-6c86b58/sound/soc/soc-dapm.c index 405841c..5ef082f 100644 --- linux-next-HEAD-6c86b58.orig/sound/soc/soc-dapm.c +++ linux-next-HEAD-6c86b58/sound/soc/soc-dapm.c @@ -2515,19 +2515,13 @@ int snd_soc_dapm_put_volsw(struct snd_kcontrol *kcontrol, int wi;
val = (ucontrol->value.integer.value[0] & mask); + connect = !!val;
if (invert) val = max - val; mask = mask << shift; val = val << shift;
- if (val) - /* new connection */ - connect = invert ? 0 : 1; - else - /* old connection must be powered down */ - connect = invert ? 1 : 0; -
Doesn't this result in the same value of connect? (given value, invert) --> (raw value, connect) old code: (0, 0) --> (0, 0) (0, 1) --> (max, 0) (max, 0) -> (max, 1) (max, 1) -> (0, 1) new code: (0, 0) --> (0, 0) (0, 1) --> (max, 0) (max, 0) --> (max, 1) (max, 1) --> (0, 1) I'd understand if the line "connect = !!val;" were after the invert conversion... if (invert) val = max - val; connect = !!val; thanks, Takashi
On Fri, Jun 29, 2012 at 08:25:14AM +0200, Takashi Iwai wrote:
Doesn't this result in the same value of connect?
If nothing else it's a massive readability improvement... I've pulled the change over to 3.6 for now, I need to rush out the door right now and probably won't be able to do anything else for the rest of today.
At Fri, 29 Jun 2012 08:23:18 +0100, Mark Brown wrote:
On Fri, Jun 29, 2012 at 08:25:14AM +0200, Takashi Iwai wrote:
Doesn't this result in the same value of connect?
If nothing else it's a massive readability improvement...
Yeah, but it's a very secret bonus, not what the changelog says :)
I've pulled the change over to 3.6 for now, I need to rush out the door right now and probably won't be able to do anything else for the rest of today.
OK. thanks, Takashi
On Fri, Jun 29, 2012 at 08:25:14AM +0200, Takashi Iwai wrote:
At Mon, 18 Jun 2012 22:41:28 +0200 (CEST), Benoît Thébaudeau wrote:
snd_soc_dapm_put_volsw() sets connect incorrectly in the case max > 1 with invert. In that case, the raw disconnect value should be max, which corresponds to the userspace value 0.
This use case currently does not appear upstream, but it could break SOC_DAPM_SINGLE() or SOC_DAPM_SINGLE_TLV() elsewhere or in the future.
Cc: Liam Girdwood <lrg@ti.com> Cc: Mark Brown <broonie@opensource.wolfsonmicro.com> Cc: <alsa-devel@alsa-project.org> Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com> --- .../sound/soc/soc-dapm.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)
diff --git linux-next-HEAD-6c86b58.orig/sound/soc/soc-dapm.c linux-next-HEAD-6c86b58/sound/soc/soc-dapm.c index 405841c..5ef082f 100644 --- linux-next-HEAD-6c86b58.orig/sound/soc/soc-dapm.c +++ linux-next-HEAD-6c86b58/sound/soc/soc-dapm.c @@ -2515,19 +2515,13 @@ int snd_soc_dapm_put_volsw(struct snd_kcontrol *kcontrol, int wi;
val = (ucontrol->value.integer.value[0] & mask); + connect = !!val;
if (invert) val = max - val; mask = mask << shift; val = val << shift;
- if (val) - /* new connection */ - connect = invert ? 0 : 1; - else - /* old connection must be powered down */ - connect = invert ? 1 : 0; -
Doesn't this result in the same value of connect?
(given value, invert) --> (raw value, connect)
old code: (0, 0) --> (0, 0) (0, 1) --> (max, 0) (max, 0) -> (max, 1) (max, 1) -> (0, 1)
new code: (0, 0) --> (0, 0) (0, 1) --> (max, 0) (max, 0) --> (max, 1) (max, 1) --> (0, 1)
I'd understand if the line "connect = !!val;" were after the invert conversion...
if (invert) val = max - val; connect = !!val;
Take max = 5, invert = 1, user val = 2: old code: connect = 0 new code: connect = 1 Regards, Benoît
At Fri, 29 Jun 2012 13:53:56 +0200 (CEST), Benoît Thébaudeau wrote:
On Fri, Jun 29, 2012 at 08:25:14AM +0200, Takashi Iwai wrote:
At Mon, 18 Jun 2012 22:41:28 +0200 (CEST), Benoît Thébaudeau wrote:
snd_soc_dapm_put_volsw() sets connect incorrectly in the case max > 1 with invert. In that case, the raw disconnect value should be max, which corresponds to the userspace value 0.
This use case currently does not appear upstream, but it could break SOC_DAPM_SINGLE() or SOC_DAPM_SINGLE_TLV() elsewhere or in the future.
Cc: Liam Girdwood <lrg@ti.com> Cc: Mark Brown <broonie@opensource.wolfsonmicro.com> Cc: <alsa-devel@alsa-project.org> Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com> --- .../sound/soc/soc-dapm.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)
diff --git linux-next-HEAD-6c86b58.orig/sound/soc/soc-dapm.c linux-next-HEAD-6c86b58/sound/soc/soc-dapm.c index 405841c..5ef082f 100644 --- linux-next-HEAD-6c86b58.orig/sound/soc/soc-dapm.c +++ linux-next-HEAD-6c86b58/sound/soc/soc-dapm.c @@ -2515,19 +2515,13 @@ int snd_soc_dapm_put_volsw(struct snd_kcontrol *kcontrol, int wi;
val = (ucontrol->value.integer.value[0] & mask); + connect = !!val;
if (invert) val = max - val; mask = mask << shift; val = val << shift;
- if (val) - /* new connection */ - connect = invert ? 0 : 1; - else - /* old connection must be powered down */ - connect = invert ? 1 : 0; -
Doesn't this result in the same value of connect?
(given value, invert) --> (raw value, connect)
old code: (0, 0) --> (0, 0) (0, 1) --> (max, 0) (max, 0) -> (max, 1) (max, 1) -> (0, 1)
new code: (0, 0) --> (0, 0) (0, 1) --> (max, 0) (max, 0) --> (max, 1) (max, 1) --> (0, 1)
I'd understand if the line "connect = !!val;" were after the invert conversion...
if (invert) val = max - val; connect = !!val;
Take max = 5, invert = 1, user val = 2: old code: connect = 0 new code: connect = 1
OK, then you need to fix dapm_set_path_status() as well, too. Otherwise the logic becomes inconsistent. Takashi
On Fri, Jun 29, 2012 at 02:03:06PM +0200, Takashi Iwai wrote:
At Fri, 29 Jun 2012 13:53:56 +0200 (CEST), Benoît Thébaudeau wrote:
On Fri, Jun 29, 2012 at 08:25:14AM +0200, Takashi Iwai wrote:
At Mon, 18 Jun 2012 22:41:28 +0200 (CEST), Benoît Thébaudeau wrote:
snd_soc_dapm_put_volsw() sets connect incorrectly in the case max > 1 with invert. In that case, the raw disconnect value should be max, which corresponds to the userspace value 0.
This use case currently does not appear upstream, but it could break SOC_DAPM_SINGLE() or SOC_DAPM_SINGLE_TLV() elsewhere or in the future.
Cc: Liam Girdwood <lrg@ti.com> Cc: Mark Brown <broonie@opensource.wolfsonmicro.com> Cc: <alsa-devel@alsa-project.org> Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com> --- .../sound/soc/soc-dapm.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)
diff --git linux-next-HEAD-6c86b58.orig/sound/soc/soc-dapm.c linux-next-HEAD-6c86b58/sound/soc/soc-dapm.c index 405841c..5ef082f 100644 --- linux-next-HEAD-6c86b58.orig/sound/soc/soc-dapm.c +++ linux-next-HEAD-6c86b58/sound/soc/soc-dapm.c @@ -2515,19 +2515,13 @@ int snd_soc_dapm_put_volsw(struct snd_kcontrol *kcontrol, int wi;
val = (ucontrol->value.integer.value[0] & mask); + connect = !!val;
if (invert) val = max - val; mask = mask << shift; val = val << shift;
- if (val) - /* new connection */ - connect = invert ? 0 : 1; - else - /* old connection must be powered down */ - connect = invert ? 1 : 0; -
Doesn't this result in the same value of connect?
(given value, invert) --> (raw value, connect)
old code: (0, 0) --> (0, 0) (0, 1) --> (max, 0) (max, 0) -> (max, 1) (max, 1) -> (0, 1)
new code: (0, 0) --> (0, 0) (0, 1) --> (max, 0) (max, 0) --> (max, 1) (max, 1) --> (0, 1)
I'd understand if the line "connect = !!val;" were after the invert conversion...
if (invert) val = max - val; connect = !!val;
Take max = 5, invert = 1, user val = 2: old code: connect = 0 new code: connect = 1
OK, then you need to fix dapm_set_path_status() as well, too. Otherwise the logic becomes inconsistent.
Indeed, I missed that. But the issue is even worse in this function: It uses the control register value to determine if connect should be set while only the userspace value can tell that, and it has no way of deriving the userspace value apart from calling the get function, while here it assumes that the register value will be more or less compatible with snd_soc_dapm_get_volsw. Hence, I think that the fix here should be to call get, then to deduce connect from the returned value. Do you agree? Regards, Benoît
At Fri, 29 Jun 2012 16:29:22 +0200 (CEST), Benoît Thébaudeau wrote:
On Fri, Jun 29, 2012 at 02:03:06PM +0200, Takashi Iwai wrote:
At Fri, 29 Jun 2012 13:53:56 +0200 (CEST), Benoît Thébaudeau wrote:
On Fri, Jun 29, 2012 at 08:25:14AM +0200, Takashi Iwai wrote:
At Mon, 18 Jun 2012 22:41:28 +0200 (CEST), Benoît Thébaudeau wrote:
snd_soc_dapm_put_volsw() sets connect incorrectly in the case max > 1 with invert. In that case, the raw disconnect value should be max, which corresponds to the userspace value 0.
This use case currently does not appear upstream, but it could break SOC_DAPM_SINGLE() or SOC_DAPM_SINGLE_TLV() elsewhere or in the future.
Cc: Liam Girdwood <lrg@ti.com> Cc: Mark Brown <broonie@opensource.wolfsonmicro.com> Cc: <alsa-devel@alsa-project.org> Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com> --- .../sound/soc/soc-dapm.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)
diff --git linux-next-HEAD-6c86b58.orig/sound/soc/soc-dapm.c linux-next-HEAD-6c86b58/sound/soc/soc-dapm.c index 405841c..5ef082f 100644 --- linux-next-HEAD-6c86b58.orig/sound/soc/soc-dapm.c +++ linux-next-HEAD-6c86b58/sound/soc/soc-dapm.c @@ -2515,19 +2515,13 @@ int snd_soc_dapm_put_volsw(struct snd_kcontrol *kcontrol, int wi;
val = (ucontrol->value.integer.value[0] & mask); + connect = !!val;
if (invert) val = max - val; mask = mask << shift; val = val << shift;
- if (val) - /* new connection */ - connect = invert ? 0 : 1; - else - /* old connection must be powered down */ - connect = invert ? 1 : 0; -
Doesn't this result in the same value of connect?
(given value, invert) --> (raw value, connect)
old code: (0, 0) --> (0, 0) (0, 1) --> (max, 0) (max, 0) -> (max, 1) (max, 1) -> (0, 1)
new code: (0, 0) --> (0, 0) (0, 1) --> (max, 0) (max, 0) --> (max, 1) (max, 1) --> (0, 1)
I'd understand if the line "connect = !!val;" were after the invert conversion...
if (invert) val = max - val; connect = !!val;
Take max = 5, invert = 1, user val = 2: old code: connect = 0 new code: connect = 1
OK, then you need to fix dapm_set_path_status() as well, too. Otherwise the logic becomes inconsistent.
Indeed, I missed that. But the issue is even worse in this function: It uses the control register value to determine if connect should be set while only the userspace value can tell that, and it has no way of deriving the userspace value apart from calling the get function, while here it assumes that the register value will be more or less compatible with snd_soc_dapm_get_volsw.
That's a valid assumption. Usually get and put callbacks must be paired well.
Hence, I think that the fix here should be to call get, then to deduce connect from the returned value. Do you agree?
Well, calling a control callback internally is a bit worrisome. Takashi
At Fri, 29 Jun 2012 17:43:09 +0200, Takashi Iwai wrote:
At Fri, 29 Jun 2012 16:29:22 +0200 (CEST), Benoît Thébaudeau wrote:
On Fri, Jun 29, 2012 at 02:03:06PM +0200, Takashi Iwai wrote:
At Fri, 29 Jun 2012 13:53:56 +0200 (CEST), Benoît Thébaudeau wrote:
On Fri, Jun 29, 2012 at 08:25:14AM +0200, Takashi Iwai wrote:
At Mon, 18 Jun 2012 22:41:28 +0200 (CEST), Benoît Thébaudeau wrote:
snd_soc_dapm_put_volsw() sets connect incorrectly in the case max > 1 with invert. In that case, the raw disconnect value should be max, which corresponds to the userspace value 0.
This use case currently does not appear upstream, but it could break SOC_DAPM_SINGLE() or SOC_DAPM_SINGLE_TLV() elsewhere or in the future.
Cc: Liam Girdwood <lrg@ti.com> Cc: Mark Brown <broonie@opensource.wolfsonmicro.com> Cc: <alsa-devel@alsa-project.org> Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com> --- .../sound/soc/soc-dapm.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)
diff --git linux-next-HEAD-6c86b58.orig/sound/soc/soc-dapm.c linux-next-HEAD-6c86b58/sound/soc/soc-dapm.c index 405841c..5ef082f 100644 --- linux-next-HEAD-6c86b58.orig/sound/soc/soc-dapm.c +++ linux-next-HEAD-6c86b58/sound/soc/soc-dapm.c @@ -2515,19 +2515,13 @@ int snd_soc_dapm_put_volsw(struct snd_kcontrol *kcontrol, int wi;
val = (ucontrol->value.integer.value[0] & mask); + connect = !!val;
if (invert) val = max - val; mask = mask << shift; val = val << shift;
- if (val) - /* new connection */ - connect = invert ? 0 : 1; - else - /* old connection must be powered down */ - connect = invert ? 1 : 0; -
Doesn't this result in the same value of connect?
(given value, invert) --> (raw value, connect)
old code: (0, 0) --> (0, 0) (0, 1) --> (max, 0) (max, 0) -> (max, 1) (max, 1) -> (0, 1)
new code: (0, 0) --> (0, 0) (0, 1) --> (max, 0) (max, 0) --> (max, 1) (max, 1) --> (0, 1)
I'd understand if the line "connect = !!val;" were after the invert conversion...
if (invert) val = max - val; connect = !!val;
Take max = 5, invert = 1, user val = 2: old code: connect = 0 new code: connect = 1
OK, then you need to fix dapm_set_path_status() as well, too. Otherwise the logic becomes inconsistent.
Indeed, I missed that. But the issue is even worse in this function: It uses the control register value to determine if connect should be set while only the userspace value can tell that, and it has no way of deriving the userspace value apart from calling the get function, while here it assumes that the register value will be more or less compatible with snd_soc_dapm_get_volsw.
That's a valid assumption. Usually get and put callbacks must be paired well.
Hence, I think that the fix here should be to call get, then to deduce connect from the returned value. Do you agree?
Well, calling a control callback internally is a bit worrisome.
BTW, looking at snd_soc_dapm_vol_getsw(), I found that snd_soc_dapm_vol_putsw() doesn't handle a stereo case. This needs a fix, too... Takashi
On Fri, Jun 29, 2012 at 05:44:41PM +0200, Takashi Iwai wrote:
BTW, looking at snd_soc_dapm_vol_getsw(), I found that snd_soc_dapm_vol_putsw() doesn't handle a stereo case. This needs a fix, too...
I saw that too. Is it the stereo case that should be added to put or removed from get, i.e. is it possible to handle a stereo case with only one connect flag? Regards, Benoît
At Fri, 29 Jun 2012 18:09:35 +0200 (CEST), Benoît Thébaudeau wrote:
On Fri, Jun 29, 2012 at 05:44:41PM +0200, Takashi Iwai wrote:
BTW, looking at snd_soc_dapm_vol_getsw(), I found that snd_soc_dapm_vol_putsw() doesn't handle a stereo case. This needs a fix, too...
I saw that too. Is it the stereo case that should be added to put or removed from get, i.e. is it possible to handle a stereo case with only one connect flag?
I would assume that connect should be up when either left or right is on. A stereo widget doesn't mean multiple connections. Takashi
On Fri, Jun 29, 2012 at 06:22:28PM +0200, Takashi Iwai wrote:
At Fri, 29 Jun 2012 18:09:35 +0200 (CEST), Benoît Thébaudeau wrote:
On Fri, Jun 29, 2012 at 05:44:41PM +0200, Takashi Iwai wrote:
BTW, looking at snd_soc_dapm_vol_getsw(), I found that snd_soc_dapm_vol_putsw() doesn't handle a stereo case. This needs a fix, too...
I saw that too. Is it the stereo case that should be added to put or removed from get, i.e. is it possible to handle a stereo case with only one connect flag?
I would assume that connect should be up when either left or right is on. A stereo widget doesn't mean multiple connections.
I have started to look into that: - Give a choice between reg and shift stereo for snd_soc_dapm_get_volsw(), like in snd_soc_get_volsw(). - Add stereo to the first case of dapm_set_path_status(). - Add stereo to dapm_widget_update() and struct snd_soc_dapm_update. - Then comes snd_soc_dapm_get_volsw(): What should we do with "widget->value = val;"? Add an rvalue to struct snd_soc_dapm_widget? This field does not seem to be read anywhere except for enums. There is also saved_value and other fields that do not seem to be used anywhere. Regards, Benoît
On Fri, Jun 29, 2012 at 09:09:49PM +0200, Benoît Thébaudeau wrote:
On Fri, Jun 29, 2012 at 06:22:28PM +0200, Takashi Iwai wrote:
At Fri, 29 Jun 2012 18:09:35 +0200 (CEST), Benoît Thébaudeau wrote:
On Fri, Jun 29, 2012 at 05:44:41PM +0200, Takashi Iwai wrote:
BTW, looking at snd_soc_dapm_vol_getsw(), I found that snd_soc_dapm_vol_putsw() doesn't handle a stereo case. This needs a fix, too...
I saw that too. Is it the stereo case that should be added to put or removed from get, i.e. is it possible to handle a stereo case with only one connect flag?
I would assume that connect should be up when either left or right is on. A stereo widget doesn't mean multiple connections.
I have started to look into that: - Give a choice between reg and shift stereo for snd_soc_dapm_get_volsw(), like in snd_soc_get_volsw(). - Add stereo to the first case of dapm_set_path_status(). - Add stereo to dapm_widget_update() and struct snd_soc_dapm_update. - Then comes snd_soc_dapm_get_volsw(): What should we do with "widget->value = val;"? Add an rvalue to struct snd_soc_dapm_widget? This field does not seem to be read anywhere except for enums. There is also saved_value and other fields that do not seem to be used anywhere.
I've finished. The following patch should be applied after my patch "ASoC: dapm: Fix dapm_set_path_status() connect". Regards, Benoît [PATCH] ASoC: dapm: Fix/add support for stereo widgets This patch: * adds a choice in snd_soc_dapm_get_volsw_mut() for stereo between 1 and 2 registers, like in snd_soc_get_volsw(). * fixes the missing stereo in other parts of dapm. * removes the unused saved_value from struct snd_soc_dapm_widget. Cc: Liam Girdwood <lrg@ti.com> Cc: Mark Brown <broonie@opensource.wolfsonmicro.com> Cc: <alsa-devel@alsa-project.org> Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com> --- .../include/sound/soc-dapm.h | 10 +-- .../sound/soc/soc-dapm.c | 92 ++++++++++++++++---- 2 files changed, 82 insertions(+), 20 deletions(-) diff --git linux-next-HEAD-d1d2d3a.orig/include/sound/soc-dapm.h linux-next-HEAD-d1d2d3a/include/sound/soc-dapm.h index 05559e5..b5d38b9 100644 --- linux-next-HEAD-d1d2d3a.orig/include/sound/soc-dapm.h +++ linux-next-HEAD-d1d2d3a/include/sound/soc-dapm.h @@ -508,8 +508,8 @@ struct snd_soc_dapm_widget { /* dapm control */ int reg; /* negative reg = no direct dapm */ unsigned char shift; /* bits to shift */ - unsigned int saved_value; /* widget saved value */ - unsigned int value; /* widget current value */ + unsigned int value; /* widget current value */ + unsigned int rvalue; /* widget current value of right channel */ unsigned int mask; /* non-shifted mask */ unsigned int on_val; /* on state value */ unsigned int off_val; /* off state value */ @@ -552,9 +552,9 @@ struct snd_soc_dapm_widget { struct snd_soc_dapm_update { struct snd_soc_dapm_widget *widget; struct snd_kcontrol *kcontrol; - int reg; - int mask; - int val; + int reg, rreg; + int mask, rmask; + int val, rval; }; /* DAPM context */ diff --git linux-next-HEAD-d1d2d3a.orig/sound/soc/soc-dapm.c linux-next-HEAD-d1d2d3a/sound/soc/soc-dapm.c index 7f2a4bb..341dade 100644 --- linux-next-HEAD-d1d2d3a.orig/sound/soc/soc-dapm.c +++ linux-next-HEAD-d1d2d3a/sound/soc/soc-dapm.c @@ -313,11 +313,13 @@ static void dapm_set_path_status(struct snd_soc_dapm_widget *w, case snd_soc_dapm_switch: case snd_soc_dapm_mixer: case snd_soc_dapm_mixer_named_ctl: { - int val; + int val, val2 = 0; struct soc_mixer_control *mc = (struct soc_mixer_control *) w->kcontrol_news[i].private_value; unsigned int reg = mc->reg; + unsigned int reg2 = mc->rreg; unsigned int shift = mc->shift; + unsigned int rshift = mc->rshift; int max = mc->max; unsigned int mask = (1 << fls(max)) - 1; unsigned int invert = mc->invert; @@ -327,7 +329,19 @@ static void dapm_set_path_status(struct snd_soc_dapm_widget *w, if (invert) val = max - val; - p->connect = !!val; + if (snd_soc_volsw_is_stereo(mc)) { + if (reg == reg2) { + val2 = soc_widget_read(w, reg); + val2 = (val2 >> rshift) & mask; + } else { + val2 = soc_widget_read(w, reg2); + val2 = (val2 >> shift) & mask; + } + if (invert) + val2 = max - val2; + } + + p->connect = val || val2; } break; case snd_soc_dapm_mux: { @@ -1397,6 +1411,14 @@ static void dapm_widget_update(struct snd_soc_dapm_context *dapm) if (ret < 0) pr_err("%s DAPM update failed: %d\n", w->name, ret); + if (update->rmask) { + ret = soc_widget_update_bits_locked(w, update->rreg, + update->rmask, update->rval); + if (ret < 0) + pr_err("%s DAPM right channel update failed: %d\n", + w->name, ret); + } + if (w->event && (w->event_flags & SND_SOC_DAPM_POST_REG)) { ret = w->event(w, update->kcontrol, SND_SOC_DAPM_POST_REG); @@ -2462,21 +2484,29 @@ int snd_soc_dapm_get_volsw(struct snd_kcontrol *kcontrol, struct soc_mixer_control *mc = (struct soc_mixer_control *)kcontrol->private_value; unsigned int reg = mc->reg; + unsigned int reg2 = mc->rreg; unsigned int shift = mc->shift; unsigned int rshift = mc->rshift; int max = mc->max; - unsigned int invert = mc->invert; unsigned int mask = (1 << fls(max)) - 1; + unsigned int invert = mc->invert; ucontrol->value.integer.value[0] = (snd_soc_read(widget->codec, reg) >> shift) & mask; - if (shift != rshift) - ucontrol->value.integer.value[1] = - (snd_soc_read(widget->codec, reg) >> rshift) & mask; - if (invert) { + if (invert) ucontrol->value.integer.value[0] = max - ucontrol->value.integer.value[0]; - if (shift != rshift) + + if (snd_soc_volsw_is_stereo(mc)) { + if (reg == reg2) + ucontrol->value.integer.value[1] = + (snd_soc_read(widget->codec, reg) >> rshift) & + mask; + else + ucontrol->value.integer.value[1] = + (snd_soc_read(widget->codec, reg2) >> shift) & + mask; + if (invert) ucontrol->value.integer.value[1] = max - ucontrol->value.integer.value[1]; } @@ -2504,11 +2534,15 @@ int snd_soc_dapm_put_volsw(struct snd_kcontrol *kcontrol, struct soc_mixer_control *mc = (struct soc_mixer_control *)kcontrol->private_value; unsigned int reg = mc->reg; + unsigned int reg2 = mc->rreg; unsigned int shift = mc->shift; + unsigned int rshift = mc->rshift; int max = mc->max; unsigned int mask = (1 << fls(max)) - 1; unsigned int invert = mc->invert; - unsigned int val; + bool type_2r = 0; + unsigned int val2 = 0; + unsigned int val, val_mask; int connect, change; struct snd_soc_dapm_update update; int wi; @@ -2518,27 +2552,53 @@ int snd_soc_dapm_put_volsw(struct snd_kcontrol *kcontrol, if (invert) val = max - val; - mask = mask << shift; + val_mask = mask << shift; val = val << shift; + if (snd_soc_volsw_is_stereo(mc)) { + val2 = (ucontrol->value.integer.value[1] & mask); + connect |= !!val2; + + if (invert) + val2 = max - val2; + if (reg == reg2) { + val_mask |= mask << rshift; + val |= val2 << rshift; + } else { + val2 = val2 << shift; + type_2r = 1; + } + } + mutex_lock_nested(&card->dapm_mutex, SND_SOC_DAPM_CLASS_RUNTIME); - change = snd_soc_test_bits(widget->codec, reg, mask, val); + change = snd_soc_test_bits(widget->codec, reg, val_mask, val); + if (type_2r) + change |= snd_soc_test_bits(widget->codec, + reg2, val_mask, val2); + if (change) { for (wi = 0; wi < wlist->num_widgets; wi++) { widget = wlist->widgets[wi]; widget->value = val; - update.kcontrol = kcontrol; update.widget = widget; update.reg = reg; - update.mask = mask; + update.mask = val_mask; update.val = val; - widget->dapm->update = &update; - soc_dapm_mixer_update_power(widget, kcontrol, connect); + if (type_2r) { + widget->rvalue = val2; + update.rreg = reg2; + update.rmask = val_mask; + update.rval = val2; + } else { + update.rmask = 0; + } + widget->dapm->update = &update; + soc_dapm_mixer_update_power(widget, kcontrol, connect); widget->dapm->update = NULL; } } @@ -2627,6 +2687,7 @@ int snd_soc_dapm_put_enum_double(struct snd_kcontrol *kcontrol, update.reg = e->reg; update.mask = mask; update.val = val; + update.rmask = 0; widget->dapm->update = &update; soc_dapm_mux_update_power(widget, kcontrol, mux, e); @@ -2793,6 +2854,7 @@ int snd_soc_dapm_put_value_enum_double(struct snd_kcontrol *kcontrol, update.reg = e->reg; update.mask = mask; update.val = val; + update.rmask = 0; widget->dapm->update = &update; soc_dapm_mux_update_power(widget, kcontrol, mux, e);
On Fri, Jun 29, 2012 at 10:18:07PM +0200, Benoît Thébaudeau wrote:
I've finished. The following patch should be applied after my patch "ASoC: dapm: Fix dapm_set_path_status() connect".
Don't submit patches in the middle of discussions, but please bear in mind what I just said about not doing stereo.
On Sat, Jun 30, 2012 at 01:39:50PM +0200, Mark Brown wrote:
On Fri, Jun 29, 2012 at 10:18:07PM +0200, Benoît Thébaudeau wrote:
I've finished. The following patch should be applied after my patch "ASoC: dapm: Fix dapm_set_path_status() connect".
Don't submit patches in the middle of discussions, but please bear in mind what I just said about not doing stereo.
Actually I'd really expect it to mean exactly that. DAPM is fairly fundamentally dealing with single channel audio streams and I'd expect that if we did implement stereo controls they'd allow these streams to pass through without getting merged. There's a couple of things that stop us doing that right now but I don't think we should add stereo controls that don't do that.
Can you be more specific about what stops us doing that right now? Should we drop this idea for now and simply remove stereo from snd_soc_dapm_get_volsw()? Regards, Benoît
On Sat, Jun 30, 2012 at 03:03:09PM +0200, Benoît Thébaudeau wrote:
On Sat, Jun 30, 2012 at 01:39:50PM +0200, Mark Brown wrote:
Actually I'd really expect it to mean exactly that. DAPM is fairly fundamentally dealing with single channel audio streams and I'd expect that if we did implement stereo controls they'd allow these streams to pass through without getting merged. There's a couple of things that stop us doing that right now but I don't think we should add stereo controls that don't do that.
Can you be more specific about what stops us doing that right now?
Nothing if someone wants to implement it.
Should we drop this idea for now and simply remove stereo from snd_soc_dapm_get_volsw()?
It's going to be easier to remove the code if we're going to do anything at all, though there's no great urgency.
This patch: * adds a choice in snd_soc_dapm_get_volsw_mut() for stereo between 1 and 2 registers, like in snd_soc_get_volsw(). * fixes the missing stereo in other parts of dapm. * removes the unused saved_value from struct snd_soc_dapm_widget. Cc: Liam Girdwood <lrg@ti.com> Cc: Mark Brown <broonie@opensource.wolfsonmicro.com> Cc: <alsa-devel@alsa-project.org> Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com> --- .../include/sound/soc-dapm.h | 10 +-- .../sound/soc/soc-dapm.c | 92 ++++++++++++++++---- 2 files changed, 82 insertions(+), 20 deletions(-) diff --git linux-next-HEAD-d1d2d3a.orig/include/sound/soc-dapm.h linux-next-HEAD-d1d2d3a/include/sound/soc-dapm.h index 05559e5..b5d38b9 100644 --- linux-next-HEAD-d1d2d3a.orig/include/sound/soc-dapm.h +++ linux-next-HEAD-d1d2d3a/include/sound/soc-dapm.h @@ -508,8 +508,8 @@ struct snd_soc_dapm_widget { /* dapm control */ int reg; /* negative reg = no direct dapm */ unsigned char shift; /* bits to shift */ - unsigned int saved_value; /* widget saved value */ - unsigned int value; /* widget current value */ + unsigned int value; /* widget current value */ + unsigned int rvalue; /* widget current value of right channel */ unsigned int mask; /* non-shifted mask */ unsigned int on_val; /* on state value */ unsigned int off_val; /* off state value */ @@ -552,9 +552,9 @@ struct snd_soc_dapm_widget { struct snd_soc_dapm_update { struct snd_soc_dapm_widget *widget; struct snd_kcontrol *kcontrol; - int reg; - int mask; - int val; + int reg, rreg; + int mask, rmask; + int val, rval; }; /* DAPM context */ diff --git linux-next-HEAD-d1d2d3a.orig/sound/soc/soc-dapm.c linux-next-HEAD-d1d2d3a/sound/soc/soc-dapm.c index 7f2a4bb..341dade 100644 --- linux-next-HEAD-d1d2d3a.orig/sound/soc/soc-dapm.c +++ linux-next-HEAD-d1d2d3a/sound/soc/soc-dapm.c @@ -313,11 +313,13 @@ static void dapm_set_path_status(struct snd_soc_dapm_widget *w, case snd_soc_dapm_switch: case snd_soc_dapm_mixer: case snd_soc_dapm_mixer_named_ctl: { - int val; + int val, val2 = 0; struct soc_mixer_control *mc = (struct soc_mixer_control *) w->kcontrol_news[i].private_value; unsigned int reg = mc->reg; + unsigned int reg2 = mc->rreg; unsigned int shift = mc->shift; + unsigned int rshift = mc->rshift; int max = mc->max; unsigned int mask = (1 << fls(max)) - 1; unsigned int invert = mc->invert; @@ -327,7 +329,19 @@ static void dapm_set_path_status(struct snd_soc_dapm_widget *w, if (invert) val = max - val; - p->connect = !!val; + if (snd_soc_volsw_is_stereo(mc)) { + if (reg == reg2) { + val2 = soc_widget_read(w, reg); + val2 = (val2 >> rshift) & mask; + } else { + val2 = soc_widget_read(w, reg2); + val2 = (val2 >> shift) & mask; + } + if (invert) + val2 = max - val2; + } + + p->connect = val || val2; } break; case snd_soc_dapm_mux: { @@ -1397,6 +1411,14 @@ static void dapm_widget_update(struct snd_soc_dapm_context *dapm) if (ret < 0) pr_err("%s DAPM update failed: %d\n", w->name, ret); + if (update->rmask) { + ret = soc_widget_update_bits_locked(w, update->rreg, + update->rmask, update->rval); + if (ret < 0) + pr_err("%s DAPM right channel update failed: %d\n", + w->name, ret); + } + if (w->event && (w->event_flags & SND_SOC_DAPM_POST_REG)) { ret = w->event(w, update->kcontrol, SND_SOC_DAPM_POST_REG); @@ -2462,21 +2484,29 @@ int snd_soc_dapm_get_volsw(struct snd_kcontrol *kcontrol, struct soc_mixer_control *mc = (struct soc_mixer_control *)kcontrol->private_value; unsigned int reg = mc->reg; + unsigned int reg2 = mc->rreg; unsigned int shift = mc->shift; unsigned int rshift = mc->rshift; int max = mc->max; - unsigned int invert = mc->invert; unsigned int mask = (1 << fls(max)) - 1; + unsigned int invert = mc->invert; ucontrol->value.integer.value[0] = (snd_soc_read(widget->codec, reg) >> shift) & mask; - if (shift != rshift) - ucontrol->value.integer.value[1] = - (snd_soc_read(widget->codec, reg) >> rshift) & mask; - if (invert) { + if (invert) ucontrol->value.integer.value[0] = max - ucontrol->value.integer.value[0]; - if (shift != rshift) + + if (snd_soc_volsw_is_stereo(mc)) { + if (reg == reg2) + ucontrol->value.integer.value[1] = + (snd_soc_read(widget->codec, reg) >> rshift) & + mask; + else + ucontrol->value.integer.value[1] = + (snd_soc_read(widget->codec, reg2) >> shift) & + mask; + if (invert) ucontrol->value.integer.value[1] = max - ucontrol->value.integer.value[1]; } @@ -2504,11 +2534,15 @@ int snd_soc_dapm_put_volsw(struct snd_kcontrol *kcontrol, struct soc_mixer_control *mc = (struct soc_mixer_control *)kcontrol->private_value; unsigned int reg = mc->reg; + unsigned int reg2 = mc->rreg; unsigned int shift = mc->shift; + unsigned int rshift = mc->rshift; int max = mc->max; unsigned int mask = (1 << fls(max)) - 1; unsigned int invert = mc->invert; - unsigned int val; + bool type_2r = 0; + unsigned int val2 = 0; + unsigned int val, val_mask; int connect, change; struct snd_soc_dapm_update update; int wi; @@ -2518,27 +2552,53 @@ int snd_soc_dapm_put_volsw(struct snd_kcontrol *kcontrol, if (invert) val = max - val; - mask = mask << shift; + val_mask = mask << shift; val = val << shift; + if (snd_soc_volsw_is_stereo(mc)) { + val2 = (ucontrol->value.integer.value[1] & mask); + connect |= !!val2; + + if (invert) + val2 = max - val2; + if (reg == reg2) { + val_mask |= mask << rshift; + val |= val2 << rshift; + } else { + val2 = val2 << shift; + type_2r = 1; + } + } + mutex_lock_nested(&card->dapm_mutex, SND_SOC_DAPM_CLASS_RUNTIME); - change = snd_soc_test_bits(widget->codec, reg, mask, val); + change = snd_soc_test_bits(widget->codec, reg, val_mask, val); + if (type_2r) + change |= snd_soc_test_bits(widget->codec, + reg2, val_mask, val2); + if (change) { for (wi = 0; wi < wlist->num_widgets; wi++) { widget = wlist->widgets[wi]; widget->value = val; - update.kcontrol = kcontrol; update.widget = widget; update.reg = reg; - update.mask = mask; + update.mask = val_mask; update.val = val; - widget->dapm->update = &update; - soc_dapm_mixer_update_power(widget, kcontrol, connect); + if (type_2r) { + widget->rvalue = val2; + update.rreg = reg2; + update.rmask = val_mask; + update.rval = val2; + } else { + update.rmask = 0; + } + widget->dapm->update = &update; + soc_dapm_mixer_update_power(widget, kcontrol, connect); widget->dapm->update = NULL; } } @@ -2627,6 +2687,7 @@ int snd_soc_dapm_put_enum_double(struct snd_kcontrol *kcontrol, update.reg = e->reg; update.mask = mask; update.val = val; + update.rmask = 0; widget->dapm->update = &update; soc_dapm_mux_update_power(widget, kcontrol, mux, e); @@ -2793,6 +2854,7 @@ int snd_soc_dapm_put_value_enum_double(struct snd_kcontrol *kcontrol, update.reg = e->reg; update.mask = mask; update.val = val; + update.rmask = 0; widget->dapm->update = &update; soc_dapm_mux_update_power(widget, kcontrol, mux, e);
On Mon, Jul 02, 2012 at 01:46:42PM +0200, Benoît Thébaudeau wrote:
This patch: * adds a choice in snd_soc_dapm_get_volsw_mut() for stereo between 1 and 2 registers, like in snd_soc_get_volsw(). * fixes the missing stereo in other parts of dapm. * removes the unused saved_value from struct snd_soc_dapm_widget.
There's a couple of high level problems here. One is that you've got multiple different things in a single commit which isn't great practice for review, especially with such a vauge changelog. The other is that while you say that this "fixes the missing stereo in other parts of dapm" I can't see any sign of any changes to (for example) the path setup code which would seem to be essential for supporting more than one channel in controls.
At Sat, 30 Jun 2012 19:24:47 +0100, Mark Brown wrote:
On Sat, Jun 30, 2012 at 03:03:09PM +0200, Benoît Thébaudeau wrote:
On Sat, Jun 30, 2012 at 01:39:50PM +0200, Mark Brown wrote:
Actually I'd really expect it to mean exactly that. DAPM is fairly fundamentally dealing with single channel audio streams and I'd expect that if we did implement stereo controls they'd allow these streams to pass through without getting merged. There's a couple of things that stop us doing that right now but I don't think we should add stereo controls that don't do that.
Can you be more specific about what stops us doing that right now?
Nothing if someone wants to implement it.
Should we drop this idea for now and simply remove stereo from snd_soc_dapm_get_volsw()?
It's going to be easier to remove the code if we're going to do anything at all, though there's no great urgency.
Then it'd be also better to put a sanity check to warn if snd_soc_volsw_is_stereo() returns true. Takashi
On Fri, Jun 29, 2012 at 06:22:28PM +0200, Takashi Iwai wrote:
Benoît Thébaudeau wrote:
I saw that too. Is it the stereo case that should be added to put or removed from get, i.e. is it possible to handle a stereo case with only one connect flag?
I would assume that connect should be up when either left or right is on. A stereo widget doesn't mean multiple connections.
Actually I'd really expect it to mean exactly that. DAPM is fairly fundamentally dealing with single channel audio streams and I'd expect that if we did implement stereo controls they'd allow these streams to pass through without getting merged. There's a couple of things that stop us doing that right now but I don't think we should add stereo controls that don't do that.
On Fri, Jun 29, 2012 at 05:43:09PM +0200, Takashi Iwai wrote:
At Fri, 29 Jun 2012 16:29:22 +0200 (CEST), Benoît Thébaudeau wrote:
On Fri, Jun 29, 2012 at 02:03:06PM +0200, Takashi Iwai wrote:
At Fri, 29 Jun 2012 13:53:56 +0200 (CEST), Benoît Thébaudeau wrote:
On Fri, Jun 29, 2012 at 08:25:14AM +0200, Takashi Iwai wrote:
At Mon, 18 Jun 2012 22:41:28 +0200 (CEST), Benoît Thébaudeau wrote:
snd_soc_dapm_put_volsw() sets connect incorrectly in the case max > 1 with invert. In that case, the raw disconnect value should be max, which corresponds to the userspace value 0.
This use case currently does not appear upstream, but it could break SOC_DAPM_SINGLE() or SOC_DAPM_SINGLE_TLV() elsewhere or in the future.
Cc: Liam Girdwood <lrg@ti.com> Cc: Mark Brown <broonie@opensource.wolfsonmicro.com> Cc: <alsa-devel@alsa-project.org> Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com> --- .../sound/soc/soc-dapm.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)
diff --git linux-next-HEAD-6c86b58.orig/sound/soc/soc-dapm.c linux-next-HEAD-6c86b58/sound/soc/soc-dapm.c index 405841c..5ef082f 100644 --- linux-next-HEAD-6c86b58.orig/sound/soc/soc-dapm.c +++ linux-next-HEAD-6c86b58/sound/soc/soc-dapm.c @@ -2515,19 +2515,13 @@ int snd_soc_dapm_put_volsw(struct snd_kcontrol *kcontrol, int wi;
val = (ucontrol->value.integer.value[0] & mask); + connect = !!val;
if (invert) val = max - val; mask = mask << shift; val = val << shift;
- if (val) - /* new connection */ - connect = invert ? 0 : 1; - else - /* old connection must be powered down */ - connect = invert ? 1 : 0; -
Doesn't this result in the same value of connect?
(given value, invert) --> (raw value, connect)
old code: (0, 0) --> (0, 0) (0, 1) --> (max, 0) (max, 0) -> (max, 1) (max, 1) -> (0, 1)
new code: (0, 0) --> (0, 0) (0, 1) --> (max, 0) (max, 0) --> (max, 1) (max, 1) --> (0, 1)
I'd understand if the line "connect = !!val;" were after the invert conversion...
if (invert) val = max - val; connect = !!val;
Take max = 5, invert = 1, user val = 2: old code: connect = 0 new code: connect = 1
OK, then you need to fix dapm_set_path_status() as well, too. Otherwise the logic becomes inconsistent.
Indeed, I missed that. But the issue is even worse in this function: It uses the control register value to determine if connect should be set while only the userspace value can tell that, and it has no way of deriving the userspace value apart from calling the get function, while here it assumes that the register value will be more or less compatible with snd_soc_dapm_get_volsw.
That's a valid assumption. Usually get and put callbacks must be paired well.
Yes, except that some codec drivers customize these callbacks for specific register encodings (e.g. snd_soc_dapm_put_volsw_aic3x). By chance, only the put callbacks seem to have been customized so far. What is the point of having customizable get/put callbacks if dapm_set_path_status() ignores them and duplicates their core behavior? In the aic3x example, the bit-field is actually a mixer volume control, and not only a boolean, so I was planning to post a fix for that. The issue is that the encoding of register values for this volume has holes that should not be duplicated in userspace raw values, so custom get and put callbacks have to be used here that will not be compatible with snd_soc_dapm_get_volsw(), which will be blocking for dapm_set_path_status(). Do you have a simple solution for that?
Hence, I think that the fix here should be to call get, then to deduce connect from the returned value. Do you agree?
Well, calling a control callback internally is a bit worrisome.
Yes, and there is another issue: kcontrol may not be available for get at this point. In the meantime, please find below a quick patch for the consistency issue. Regards, Benoît [PATCH] ASoC: dapm: Fix dapm_set_path_status() connect dapm_set_path_status() sets connect incorrectly in the case max > 1 with invert. In that case, the raw disconnect value should be max, which corresponds to the userspace value 0. This use case currently does not appear upstream, but it could break SOC_DAPM_SINGLE() or SOC_DAPM_SINGLE_TLV() elsewhere or in the future. This patch completes commit 3a9abe8. Cc: Liam Girdwood <lrg@ti.com> Cc: Mark Brown <broonie@opensource.wolfsonmicro.com> Cc: <alsa-devel@alsa-project.org> Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com> --- .../sound/soc/soc-dapm.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git linux-next-HEAD-d1d2d3a.orig/sound/soc/soc-dapm.c linux-next-HEAD-d1d2d3a/sound/soc/soc-dapm.c index 9670668..7f2a4bb 100644 --- linux-next-HEAD-d1d2d3a.orig/sound/soc/soc-dapm.c +++ linux-next-HEAD-d1d2d3a/sound/soc/soc-dapm.c @@ -324,11 +324,10 @@ static void dapm_set_path_status(struct snd_soc_dapm_widget *w, val = soc_widget_read(w, reg); val = (val >> shift) & mask; + if (invert) + val = max - val; - if ((invert && !val) || (!invert && val)) - p->connect = 1; - else - p->connect = 0; + p->connect = !!val; } break; case snd_soc_dapm_mux: {
At Fri, 29 Jun 2012 18:05:44 +0200 (CEST), Benoît Thébaudeau wrote:
On Fri, Jun 29, 2012 at 05:43:09PM +0200, Takashi Iwai wrote:
At Fri, 29 Jun 2012 16:29:22 +0200 (CEST), Benoît Thébaudeau wrote:
On Fri, Jun 29, 2012 at 02:03:06PM +0200, Takashi Iwai wrote:
At Fri, 29 Jun 2012 13:53:56 +0200 (CEST), Benoît Thébaudeau wrote:
On Fri, Jun 29, 2012 at 08:25:14AM +0200, Takashi Iwai wrote:
At Mon, 18 Jun 2012 22:41:28 +0200 (CEST), Benoît Thébaudeau wrote: > > snd_soc_dapm_put_volsw() sets connect incorrectly in the > case > max > > 1 with > invert. In that case, the raw disconnect value should be > max, > which > corresponds > to the userspace value 0. > > This use case currently does not appear upstream, but it > could > break > SOC_DAPM_SINGLE() or SOC_DAPM_SINGLE_TLV() elsewhere or in > the > future. > > Cc: Liam Girdwood <lrg@ti.com> > Cc: Mark Brown <broonie@opensource.wolfsonmicro.com> > Cc: <alsa-devel@alsa-project.org> > Signed-off-by: Benoît Thébaudeau > <benoit.thebaudeau@advansee.com> > --- > .../sound/soc/soc-dapm.c | 8 > +------- > 1 file changed, 1 insertion(+), 7 deletions(-) > > diff --git > linux-next-HEAD-6c86b58.orig/sound/soc/soc-dapm.c > linux-next-HEAD-6c86b58/sound/soc/soc-dapm.c > index 405841c..5ef082f 100644 > --- linux-next-HEAD-6c86b58.orig/sound/soc/soc-dapm.c > +++ linux-next-HEAD-6c86b58/sound/soc/soc-dapm.c > @@ -2515,19 +2515,13 @@ int snd_soc_dapm_put_volsw(struct > snd_kcontrol *kcontrol, > int wi; > > val = (ucontrol->value.integer.value[0] & mask); > + connect = !!val; > > if (invert) > val = max - val; > mask = mask << shift; > val = val << shift; > > - if (val) > - /* new connection */ > - connect = invert ? 0 : 1; > - else > - /* old connection must be powered down */ > - connect = invert ? 1 : 0; > -
Doesn't this result in the same value of connect?
(given value, invert) --> (raw value, connect)
old code: (0, 0) --> (0, 0) (0, 1) --> (max, 0) (max, 0) -> (max, 1) (max, 1) -> (0, 1)
new code: (0, 0) --> (0, 0) (0, 1) --> (max, 0) (max, 0) --> (max, 1) (max, 1) --> (0, 1)
I'd understand if the line "connect = !!val;" were after the invert conversion...
if (invert) val = max - val; connect = !!val;
Take max = 5, invert = 1, user val = 2: old code: connect = 0 new code: connect = 1
OK, then you need to fix dapm_set_path_status() as well, too. Otherwise the logic becomes inconsistent.
Indeed, I missed that. But the issue is even worse in this function: It uses the control register value to determine if connect should be set while only the userspace value can tell that, and it has no way of deriving the userspace value apart from calling the get function, while here it assumes that the register value will be more or less compatible with snd_soc_dapm_get_volsw.
That's a valid assumption. Usually get and put callbacks must be paired well.
Yes, except that some codec drivers customize these callbacks for specific register encodings (e.g. snd_soc_dapm_put_volsw_aic3x). By chance, only the put callbacks seem to have been customized so far.
Hm, that's bad.
What is the point of having customizable get/put callbacks if dapm_set_path_status() ignores them and duplicates their core behavior?
It's a design flaw :)
In the aic3x example, the bit-field is actually a mixer volume control, and not only a boolean, so I was planning to post a fix for that. The issue is that the encoding of register values for this volume has holes that should not be duplicated in userspace raw values, so custom get and put callbacks have to be used here that will not be compatible with snd_soc_dapm_get_volsw(), which will be blocking for dapm_set_path_status(). Do you have a simple solution for that?
One option is to create a new hook in struct soc_mixer_control or whatever, e.g. int (*connect_update)(struct soc_mixer_control *mc); that returns the connect value. Then give the own callback when you set the customized get/put things that are incompatible with the default one.
Hence, I think that the fix here should be to call get, then to deduce connect from the returned value. Do you agree?
Well, calling a control callback internally is a bit worrisome.
Yes, and there is another issue: kcontrol may not be available for get at this point.
In the meantime, please find below a quick patch for the consistency issue.
Looks good to me. Reivewed-by: Takashi Iwai <tiwai@suse.de> Takashi
Regards, Benoît
[PATCH] ASoC: dapm: Fix dapm_set_path_status() connect
dapm_set_path_status() sets connect incorrectly in the case max > 1 with invert. In that case, the raw disconnect value should be max, which corresponds to the userspace value 0.
This use case currently does not appear upstream, but it could break SOC_DAPM_SINGLE() or SOC_DAPM_SINGLE_TLV() elsewhere or in the future.
This patch completes commit 3a9abe8.
Cc: Liam Girdwood <lrg@ti.com> Cc: Mark Brown <broonie@opensource.wolfsonmicro.com> Cc: <alsa-devel@alsa-project.org> Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com> --- .../sound/soc/soc-dapm.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
diff --git linux-next-HEAD-d1d2d3a.orig/sound/soc/soc-dapm.c linux-next-HEAD-d1d2d3a/sound/soc/soc-dapm.c index 9670668..7f2a4bb 100644 --- linux-next-HEAD-d1d2d3a.orig/sound/soc/soc-dapm.c +++ linux-next-HEAD-d1d2d3a/sound/soc/soc-dapm.c @@ -324,11 +324,10 @@ static void dapm_set_path_status(struct snd_soc_dapm_widget *w,
val = soc_widget_read(w, reg); val = (val >> shift) & mask; + if (invert) + val = max - val;
- if ((invert && !val) || (!invert && val)) - p->connect = 1; - else - p->connect = 0; + p->connect = !!val; } break; case snd_soc_dapm_mux: {
On Fri, Jun 29, 2012 at 06:05:44PM +0200, Benoît Thébaudeau wrote:
On Fri, Jun 29, 2012 at 05:43:09PM +0200, Takashi Iwai wrote:
That's a valid assumption. Usually get and put callbacks must be paired well.
What is the point of having customizable get/put callbacks if dapm_set_path_status() ignores them and duplicates their core behavior?
Nobody needed to do that yet.
In the aic3x example, the bit-field is actually a mixer volume control, and not only a boolean, so I was planning to post a fix for that. The issue is that the encoding of register values for this volume has holes that should not be duplicated in userspace raw values, so custom get and put callbacks have to be used here that will not be compatible with snd_soc_dapm_get_volsw(), which will be blocking for dapm_set_path_status(). Do you have a simple solution for that?
Clearly the driver is just buggy and should implement get for itself too. As far as reading back the path status is concerned so long as the mapping looks OK as a boolean we're fine, it doesn't matter what the individual volumes are as all the code is interested in is on/off.
In the meantime, please find below a quick patch for the consistency issue.
Stop doing this, send patches for review rather than burying them in the middle of e-mails where they're at best a pain to apply.
On Sat, Jun 30, 2012 at 01:54:25PM +0200, Mark Brown wrote:
In the meantime, please find below a quick patch for the consistency issue.
Stop doing this, send patches for review rather than burying them in the middle of e-mails where they're at best a pain to apply.
OK, I'll repost it separately on Monday. Regards, Benoît
dapm_set_path_status() sets connect incorrectly in the case max > 1 with invert. In that case, the raw disconnect value should be max, which corresponds to the userspace value 0. This use case currently does not appear upstream, but it could break SOC_DAPM_SINGLE() or SOC_DAPM_SINGLE_TLV() elsewhere or in the future. This patch completes commit 3a9abe8. Cc: Liam Girdwood <lrg@ti.com> Cc: Mark Brown <broonie@opensource.wolfsonmicro.com> Cc: <alsa-devel@alsa-project.org> Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com> --- .../sound/soc/soc-dapm.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git linux-next-HEAD-d1d2d3a.orig/sound/soc/soc-dapm.c linux-next-HEAD-d1d2d3a/sound/soc/soc-dapm.c index 9670668..7f2a4bb 100644 --- linux-next-HEAD-d1d2d3a.orig/sound/soc/soc-dapm.c +++ linux-next-HEAD-d1d2d3a/sound/soc/soc-dapm.c @@ -324,11 +324,10 @@ static void dapm_set_path_status(struct snd_soc_dapm_widget *w, val = soc_widget_read(w, reg); val = (val >> shift) & mask; + if (invert) + val = max - val; - if ((invert && !val) || (!invert && val)) - p->connect = 1; - else - p->connect = 0; + p->connect = !!val; } break; case snd_soc_dapm_mux: {
On Mon, Jul 02, 2012 at 01:45:21PM +0200, Benoît Thébaudeau wrote:
dapm_set_path_status() sets connect incorrectly in the case max > 1 with invert. In that case, the raw disconnect value should be max, which corresponds to the userspace value 0.
Applied, thanks.
participants (3)
-
Benoît Thébaudeau -
Mark Brown -
Takashi Iwai