Regression caused by "ASoC: core: Rework SOC_DOUBLE_R_SX_TLV add SOC_SINGLE_SX_TLV"

Regression caused by "ASoC: core: Rework SOC_DOUBLE_R_SX_TLV add SOC_SINGLE_SX_TLV"

[Thomas Petazzoni]

Brian, Liam, Mark,

I am currently bringing up audio support on the Marvell Armada 370 ARM
SoC, for which the development board uses the CS42L51 audio codec.

It turns out that the commit 1d99f2436d0d1c7741d6dfd9d27b5376cdbbca40
("ASoC: core: Rework SOC_DOUBLE_R_SX_TLV add SOC_SINGLE_SX_TLV") causes
a problem here, and reverting this commit gets things back to normal.

Basically, when this commit is applied, the "PCM", "Analog" and "ADC
Mixer" volume controls in alsamixer cannot be changed to any other
value than 60 or 61. The volume control doesn't allow to go above 61
and below 60. If I remember correctly, at the codec level, the values
configured for PCM in registers 0x10 and 0x11 are either 0x0 or 0x1
depending on whether you select 60 or 61 in alsamixer.

Reverting the commit mentioned above makes alsamixer behave normally:
the volume can be adjusted between 0 and 100 as expected, the values in
register 0x10 and 0x11 are meaningful, and the behavior is as expected:
with a volume to 0, I hear nothing in my headphones, with a volume set
to 100, the volume is maximum, with a nice progressive volume increase
in-between.

Therefore, I believe that the commit has a problem. I haven't yet
investigated where the problem is, maybe just the values passed to
SOC_DOUBLE_R_SX_TLV(), or maybe in the implementation of the volume
control functions themselves. I can certainly start investigating, but
maybe the author of the commit will immediately realize where the
problem could be.

Thanks a lot!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: Regression caused by "ASoC: core: Rework SOC_DOUBLE_R_SX_TLV add SOC_SINGLE_SX_TLV"

[Brian Austin]

Hello Thomas,

On Thu, 30 Jan 2014, Thomas Petazzoni wrote:

> Brian, Liam, Mark,
>
> I am currently bringing up audio support on the Marvell Armada 370 ARM
> SoC, for which the development board uses the CS42L51 audio codec.
>
> It turns out that the commit 1d99f2436d0d1c7741d6dfd9d27b5376cdbbca40
> ("ASoC: core: Rework SOC_DOUBLE_R_SX_TLV add SOC_SINGLE_SX_TLV") causes
> a problem here, and reverting this commit gets things back to normal.
>
> Basically, when this commit is applied, the "PCM", "Analog" and "ADC
> Mixer" volume controls in alsamixer cannot be changed to any other
> value than 60 or 61. The volume control doesn't allow to go above 61
> and below 60. If I remember correctly, at the codec level, the values
> configured for PCM in registers 0x10 and 0x11 are either 0x0 or 0x1
> depending on whether you select 60 or 61 in alsamixer.

That is odd. My other devices that use this don't show that behavior, I 
will check on a different device, but I will see if I can get an L51 
today. So your saying the L51 for PCMA/B Mixer Volume COntrol, you can 
only select 2 values in the whole range?

>
> Reverting the commit mentioned above makes alsamixer behave normally:
> the volume can be adjusted between 0 and 100 as expected, the values in
> register 0x10 and 0x11 are meaningful, and the behavior is as expected:
> with a volume to 0, I hear nothing in my headphones, with a volume set
> to 100, the volume is maximum, with a nice progressive volume increase
> in-between.
>
> Therefore, I believe that the commit has a problem. I haven't yet
> investigated where the problem is, maybe just the values passed to
> SOC_DOUBLE_R_SX_TLV(), or maybe in the implementation of the volume
> control functions themselves. I can certainly start investigating, but
> maybe the author of the commit will immediately realize where the
> problem could be.
>

I do see however that the MAX value is not correct and should be 0x80 
instead of 0x7F. Off by 1...

I'll investigate today.

> Thanks a lot!
>
> Thomas
> -- 
> Thomas Petazzoni, CTO, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com
>
>
Thanks!

Re: Regression caused by "ASoC: core: Rework SOC_DOUBLE_R_SX_TLV add SOC_SINGLE_SX_TLV"

[Thomas Petazzoni]

Dear Brian Austin,

On Thu, 30 Jan 2014 08:59:44 -0600, Brian Austin wrote:

> That is odd. My other devices that use this don't show that behavior, I 
> will check on a different device, but I will see if I can get an L51 
> today. So your saying the L51 for PCMA/B Mixer Volume COntrol, you can 
> only select 2 values in the whole range?

Yes, absolutely. Not only for the PCMA/B Mixer Volume Control but also
for the two other volume controls that were modified by your commit.
I'll try to get back to your commit (without my revert) to verify once
again my claims.

> > Therefore, I believe that the commit has a problem. I haven't yet
> > investigated where the problem is, maybe just the values passed to
> > SOC_DOUBLE_R_SX_TLV(), or maybe in the implementation of the volume
> > control functions themselves. I can certainly start investigating, but
> > maybe the author of the commit will immediately realize where the
> > problem could be.
> 
> I do see however that the MAX value is not correct and should be 0x80 
> instead of 0x7F. Off by 1...

Hum, ok, but I don't believe this should affect the range of values
that alsamixer sees, but only the fact that you're not using the full
range of volumes available at the codec level, right?

> I'll investigate today.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: Regression caused by "ASoC: core: Rework SOC_DOUBLE_R_SX_TLV add SOC_SINGLE_SX_TLV"

[Brian Austin]

On Thu, 30 Jan 2014, Thomas Petazzoni wrote:

> Dear Brian Austin,
>
> On Thu, 30 Jan 2014 08:59:44 -0600, Brian Austin wrote:
>
>> That is odd. My other devices that use this don't show that behavior, I
>> will check on a different device, but I will see if I can get an L51
>> today. So your saying the L51 for PCMA/B Mixer Volume COntrol, you can
>> only select 2 values in the whole range?
>
> Yes, absolutely. Not only for the PCMA/B Mixer Volume Control but also
> for the two other volume controls that were modified by your commit.
> I'll try to get back to your commit (without my revert) to verify once
> again my claims.
>
>>> Therefore, I believe that the commit has a problem. I haven't yet
>>> investigated where the problem is, maybe just the values passed to
>>> SOC_DOUBLE_R_SX_TLV(), or maybe in the implementation of the volume
>>> control functions themselves. I can certainly start investigating, but
>>> maybe the author of the commit will immediately realize where the
>>> problem could be.
>>
>> I do see however that the MAX value is not correct and should be 0x80
>> instead of 0x7F. Off by 1...
>
> Hum, ok, but I don't believe this should affect the range of values
> that alsamixer sees, but only the fact that you're not using the full
> range of volumes available at the codec level, right?

Yeah, that would not explain why you can only use 2 values for gain. That 
means something is for sure broken.

I have an L73 just up now and will check those register values and get 
back to you real soon.

sorry for the hassle

Re: Regression caused by "ASoC: core: Rework SOC_DOUBLE_R_SX_TLV add SOC_SINGLE_SX_TLV"

[Brian Austin]

On Thu, 30 Jan 2014, Brian Austin wrote:

> On Thu, 30 Jan 2014, Thomas Petazzoni wrote:
>
>> Dear Brian Austin,
>> 
>> On Thu, 30 Jan 2014 08:59:44 -0600, Brian Austin wrote:
>> 
>>> That is odd. My other devices that use this don't show that behavior, I
>>> will check on a different device, but I will see if I can get an L51
>>> today. So your saying the L51 for PCMA/B Mixer Volume COntrol, you can
>>> only select 2 values in the whole range?
>> 
>> Yes, absolutely. Not only for the PCMA/B Mixer Volume Control but also
>> for the two other volume controls that were modified by your commit.
>> I'll try to get back to your commit (without my revert) to verify once
>> again my claims.
>> 
>>>> Therefore, I believe that the commit has a problem. I haven't yet
>>>> investigated where the problem is, maybe just the values passed to
>>>> SOC_DOUBLE_R_SX_TLV(), or maybe in the implementation of the volume
>>>> control functions themselves. I can certainly start investigating, but
>>>> maybe the author of the commit will immediately realize where the
>>>> problem could be.
>>> 
>>> I do see however that the MAX value is not correct and should be 0x80
>>> instead of 0x7F. Off by 1...
>> 
>> Hum, ok, but I don't believe this should affect the range of values
>> that alsamixer sees, but only the fact that you're not using the full
>> range of volumes available at the codec level, right?
>
> Yeah, that would not explain why you can only use 2 values for gain. That 
> means something is for sure broken.
>
> I have an L73 just up now and will check those register values and get back 
> to you real soon.

So with v3.12 with the CS42L73 I can use SOC_DOUBLE_R_SX_TLV and adjust 
the gain correctly and the register settings are reflected correctly as 
well when I do an i2cdump of the device. Let me hunt down an L51 CDB and 
see what is going on with that device.

-brian

Re: Regression caused by "ASoC: core: Rework SOC_DOUBLE_R_SX_TLV add SOC_SINGLE_SX_TLV"

[Thomas Petazzoni]

[-- Attachment #1: Type: text/plain, Size: 875 bytes --]

Dear Brian Austin,

On Thu, 30 Jan 2014 09:37:44 -0600, Brian Austin wrote:

> > I have an L73 just up now and will check those register values and get back 
> > to you real soon.
> 
> So with v3.12 with the CS42L73 I can use SOC_DOUBLE_R_SX_TLV and adjust 
> the gain correctly and the register settings are reflected correctly as 
> well when I do an i2cdump of the device. Let me hunt down an L51 CDB and 
> see what is going on with that device.

I've tried again, and I confirm the problem. When the PCM is muted, I
can only toggle it between the values 62 and 64. When the PCM is
unmuted, I can only toggle it between the values 60 and 61.

The ugly attached patch fixes the problem for me (the patch is a
partial revert of your patch).

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: 0007-sound-partial-revert-of-1d99f2436d0d1c7741d6dfd9d27b.patch --]
[-- Type: text/x-patch, Size: 8445 bytes --]

>From bdc427d58a4fa7dbb25d770618776cc25aa0f184 Mon Sep 17 00:00:00 2001
From: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Date: Thu, 30 Jan 2014 15:14:41 +0100
Subject: [PATCH 7/9] sound: partial revert of
 1d99f2436d0d1c7741d6dfd9d27b5376cdbbca40

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 include/sound/soc.h        | 43 ++++++++++-----------
 sound/soc/codecs/cs42l51.c |  6 +--
 sound/soc/soc-core.c       | 94 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 116 insertions(+), 27 deletions(-)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index 1f741cb..118736b 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -70,18 +70,6 @@
 	.info = snd_soc_info_volsw, .get = snd_soc_get_volsw,\
 	.put = snd_soc_put_volsw, \
 	.private_value = SOC_SINGLE_VALUE(reg, shift, max, invert, 0) }
-#define SOC_SINGLE_SX_TLV(xname, xreg, xshift, xmin, xmax, tlv_array) \
-{       .iface = SNDRV_CTL_ELEM_IFACE_MIXER, .name = xname, \
-	.access = SNDRV_CTL_ELEM_ACCESS_TLV_READ | \
-	SNDRV_CTL_ELEM_ACCESS_READWRITE, \
-	.tlv.p  = (tlv_array),\
-	.info = snd_soc_info_volsw, \
-	.get = snd_soc_get_volsw_sx,\
-	.put = snd_soc_put_volsw_sx, \
-	.private_value = (unsigned long)&(struct soc_mixer_control) \
-		{.reg = xreg, .rreg = xreg, \
-		.shift = xshift, .rshift = xshift, \
-		.max = xmax, .min = xmin} }
 #define SOC_SINGLE_RANGE_TLV(xname, xreg, xshift, xmin, xmax, xinvert, tlv_array) \
 {	.iface = SNDRV_CTL_ELEM_IFACE_MIXER, .name = (xname),\
 	.access = SNDRV_CTL_ELEM_ACCESS_TLV_READ |\
@@ -140,18 +128,6 @@
 	.get = snd_soc_get_volsw_range, .put = snd_soc_put_volsw_range, \
 	.private_value = SOC_DOUBLE_R_RANGE_VALUE(reg_left, reg_right, \
 					    xshift, xmin, xmax, xinvert) }
-#define SOC_DOUBLE_R_SX_TLV(xname, xreg, xrreg, xshift, xmin, xmax, tlv_array) \
-{       .iface = SNDRV_CTL_ELEM_IFACE_MIXER, .name = (xname), \
-	.access = SNDRV_CTL_ELEM_ACCESS_TLV_READ | \
-	SNDRV_CTL_ELEM_ACCESS_READWRITE, \
-	.tlv.p  = (tlv_array), \
-	.info = snd_soc_info_volsw, \
-	.get = snd_soc_get_volsw_sx, \
-	.put = snd_soc_put_volsw_sx, \
-	.private_value = (unsigned long)&(struct soc_mixer_control) \
-		{.reg = xreg, .rreg = xrreg, \
-		.shift = xshift, .rshift = xshift, \
-		.max = xmax, .min = xmin} }
 #define SOC_DOUBLE_S8_TLV(xname, xreg, xmin, xmax, tlv_array) \
 {	.iface  = SNDRV_CTL_ELEM_IFACE_MIXER, .name = (xname), \
 	.access = SNDRV_CTL_ELEM_ACCESS_TLV_READ | \
@@ -162,6 +138,19 @@
 	.private_value = (unsigned long)&(struct soc_mixer_control) \
 		{.reg = xreg, .min = xmin, .max = xmax, \
 		 .platform_max = xmax} }
+#define SOC_DOUBLE_R_SX_TLV(xname, xreg_left, xreg_right, xshift,\
+               xmin, xmax, tlv_array) \
+{      .iface = SNDRV_CTL_ELEM_IFACE_MIXER, .name = (xname), \
+       .access = SNDRV_CTL_ELEM_ACCESS_TLV_READ |	     \
+       SNDRV_CTL_ELEM_ACCESS_READWRITE,			     \
+       .tlv.p = (tlv_array),				     \
+       .info = snd_soc_info_volsw_2r_sx,		     \
+       .get = snd_soc_get_volsw_2r_sx,			     \
+       .put = snd_soc_put_volsw_2r_sx,				   \
+       .private_value = (unsigned long)&(struct soc_mixer_control) \
+       {.reg = xreg_left,					   \
+	.rreg = xreg_right, .shift = xshift,			   \
+	.min = xmin, .max = xmax} }
 #define SOC_ENUM_DOUBLE(xreg, xshift_l, xshift_r, xmax, xtexts) \
 {	.reg = xreg, .shift_l = xshift_l, .shift_r = xshift_r, \
 	.max = xmax, .texts = xtexts, \
@@ -518,6 +507,12 @@ int snd_soc_get_volsw_range(struct snd_kcontrol *kcontrol,
 	struct snd_ctl_elem_value *ucontrol);
 int snd_soc_limit_volume(struct snd_soc_codec *codec,
 	const char *name, int max);
+int snd_soc_info_volsw_2r_sx(struct snd_kcontrol *kcontrol,
+       struct snd_ctl_elem_info *uinfo);
+int snd_soc_get_volsw_2r_sx(struct snd_kcontrol *kcontrol,
+       struct snd_ctl_elem_value *ucontrol);
+int snd_soc_put_volsw_2r_sx(struct snd_kcontrol *kcontrol,
+       struct snd_ctl_elem_value *ucontrol);
 int snd_soc_bytes_info(struct snd_kcontrol *kcontrol,
 		       struct snd_ctl_elem_info *uinfo);
 int snd_soc_bytes_get(struct snd_kcontrol *kcontrol,
diff --git a/sound/soc/codecs/cs42l51.c b/sound/soc/codecs/cs42l51.c
index 4a7a733..d1a4ffb 100644
--- a/sound/soc/codecs/cs42l51.c
+++ b/sound/soc/codecs/cs42l51.c
@@ -141,15 +141,15 @@ static const struct soc_enum cs42l51_chan_mix =
 static const struct snd_kcontrol_new cs42l51_snd_controls[] = {
 	SOC_DOUBLE_R_SX_TLV("PCM Playback Volume",
 			CS42L51_PCMA_VOL, CS42L51_PCMB_VOL,
-			6, 0x19, 0x7F, adc_pcm_tlv),
+			7, 0xffffff99, 0x18, adc_pcm_tlv),
 	SOC_DOUBLE_R("PCM Playback Switch",
 			CS42L51_PCMA_VOL, CS42L51_PCMB_VOL, 7, 1, 1),
 	SOC_DOUBLE_R_SX_TLV("Analog Playback Volume",
 			CS42L51_AOUTA_VOL, CS42L51_AOUTB_VOL,
-			0, 0x34, 0xE4, aout_tlv),
+			8, 0xffffff19, 0x18, aout_tlv),
 	SOC_DOUBLE_R_SX_TLV("ADC Mixer Volume",
 			CS42L51_ADCA_VOL, CS42L51_ADCB_VOL,
-			6, 0x19, 0x7F, adc_pcm_tlv),
+			7, 0xffffff99, 0x18, adc_pcm_tlv),
 	SOC_DOUBLE_R("ADC Mixer Switch",
 			CS42L51_ADCA_VOL, CS42L51_ADCB_VOL, 7, 1, 1),
 	SOC_SINGLE("Playback Deemphasis Switch", CS42L51_DAC_CTL, 3, 1, 0),
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index a66783e..bc476a8 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -3177,6 +3177,100 @@ int snd_soc_limit_volume(struct snd_soc_codec *codec,
 }
 EXPORT_SYMBOL_GPL(snd_soc_limit_volume);
 
+/**
+ * snd_soc_info_volsw_2r_sx - double with tlv and variable data size
+ *  mixer info callback
+ * @kcontrol: mixer control
+ * @uinfo: control element information
+ *
+ * Returns 0 for success.
+ */
+int snd_soc_info_volsw_2r_sx(struct snd_kcontrol *kcontrol,
+                       struct snd_ctl_elem_info *uinfo)
+{
+       struct soc_mixer_control *mc =
+               (struct soc_mixer_control *)kcontrol->private_value;
+       int max = mc->max;
+       int min = mc->min;
+
+       uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER;
+       uinfo->count = 2;
+       uinfo->value.integer.min = 0;
+       uinfo->value.integer.max = max-min;
+
+       return 0;
+}
+EXPORT_SYMBOL_GPL(snd_soc_info_volsw_2r_sx);
+
+/**
+ * snd_soc_get_volsw_2r_sx - double with tlv and variable data size
+ *  mixer get callback
+ * @kcontrol: mixer control
+ * @uinfo: control element information
+ *
+ * Returns 0 for success.
+ */
+int snd_soc_get_volsw_2r_sx(struct snd_kcontrol *kcontrol,
+                       struct snd_ctl_elem_value *ucontrol)
+{
+       struct soc_mixer_control *mc =
+               (struct soc_mixer_control *)kcontrol->private_value;
+       struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);
+       unsigned int mask = (1<<mc->shift)-1;
+       int min = mc->min;
+       int val = snd_soc_read(codec, mc->reg) & mask;
+       int valr = snd_soc_read(codec, mc->rreg) & mask;
+
+       ucontrol->value.integer.value[0] = ((val & 0xff)-min) & mask;
+       ucontrol->value.integer.value[1] = ((valr & 0xff)-min) & mask;
+       return 0;
+}
+EXPORT_SYMBOL_GPL(snd_soc_get_volsw_2r_sx);
+
+
+/**
+ * snd_soc_put_volsw_2r_sx - double with tlv and variable data size
+ *  mixer put callback
+ * @kcontrol: mixer control
+ * @uinfo: control element information
+ *
+ * Returns 0 for success.
+ */
+int snd_soc_put_volsw_2r_sx(struct snd_kcontrol *kcontrol,
+                       struct snd_ctl_elem_value *ucontrol)
+{
+       struct soc_mixer_control *mc =
+               (struct soc_mixer_control *)kcontrol->private_value;
+       struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);
+       unsigned int mask = (1<<mc->shift)-1;
+       int min = mc->min;
+       int ret;
+       unsigned int val, valr, oval, ovalr;
+
+       val = ((ucontrol->value.integer.value[0]+min) & 0xff);
+       val &= mask;
+       valr = ((ucontrol->value.integer.value[1]+min) & 0xff);
+       valr &= mask;
+
+       oval = snd_soc_read(codec, mc->reg) & mask;
+       ovalr = snd_soc_read(codec, mc->rreg) & mask;
+
+       ret = 0;
+       if (oval != val) {
+               ret = snd_soc_write(codec, mc->reg, val);
+               if (ret < 0)
+                       return ret;
+       }
+       if (ovalr != valr) {
+               ret = snd_soc_write(codec, mc->rreg, valr);
+               if (ret < 0)
+                       return ret;
+       }
+
+       return 0;
+}
+EXPORT_SYMBOL_GPL(snd_soc_put_volsw_2r_sx);
+
 int snd_soc_bytes_info(struct snd_kcontrol *kcontrol,
 		       struct snd_ctl_elem_info *uinfo)
 {
-- 
1.8.3.2


[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: Regression caused by "ASoC: core: Rework SOC_DOUBLE_R_SX_TLV add SOC_SINGLE_SX_TLV"

[Brian Austin]

> I've tried again, and I confirm the problem. When the PCM is muted, I
> can only toggle it between the values 62 and 64. When the PCM is
> unmuted, I can only toggle it between the values 60 and 61.
OK, that is very odd.  I will take a look when I get a chance. I have an 
L51 board, but need to do some other stuff first.

>
> The ugly attached patch fixes the problem for me (the patch is a
> partial revert of your patch).
>

Thanks! but...

         SOC_DOUBLE_R_SX_TLV("Analog Playback Volume",
                         CS42L51_AOUTA_VOL, CS42L51_AOUTB_VOL,
-                       0, 0x34, 0xE4, aout_tlv),
+                       8, 0xffffff19, 0x18, aout_tlv),
         SOC_DOUBLE_R_SX_TLV("ADC Mixer Volume",

shifting 8 on an 8bit register?

I'll have to look into this. Thanks for the feedback!

-Brian

Re: Regression caused by "ASoC: core: Rework SOC_DOUBLE_R_SX_TLV add SOC_SINGLE_SX_TLV"

[Thomas Petazzoni]

Dear Brian Austin,

On Thu, 30 Jan 2014 11:13:36 -0600, Brian Austin wrote:

> > I've tried again, and I confirm the problem. When the PCM is muted, I
> > can only toggle it between the values 62 and 64. When the PCM is
> > unmuted, I can only toggle it between the values 60 and 61.
> OK, that is very odd.  I will take a look when I get a chance. I have an 
> L51 board, but need to do some other stuff first.

Ok, thanks!

> > The ugly attached patch fixes the problem for me (the patch is a
> > partial revert of your patch).
> >
> 
> Thanks! but...
> 
>          SOC_DOUBLE_R_SX_TLV("Analog Playback Volume",
>                          CS42L51_AOUTA_VOL, CS42L51_AOUTB_VOL,
> -                       0, 0x34, 0xE4, aout_tlv),
> +                       8, 0xffffff19, 0x18, aout_tlv),
>          SOC_DOUBLE_R_SX_TLV("ADC Mixer Volume",
> 
> shifting 8 on an 8bit register?

These values are just the ones that were here before your commit
1d99f2436d0d1c7741d6dfd9d27b5376cdbbca40. I didn't invent anything,
just reverted partially your commit.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: Regression caused by "ASoC: core: Rework SOC_DOUBLE_R_SX_TLV add SOC_SINGLE_SX_TLV"

[Brian Austin]

>> Thanks! but...
>>
>>          SOC_DOUBLE_R_SX_TLV("Analog Playback Volume",
>>                          CS42L51_AOUTA_VOL, CS42L51_AOUTB_VOL,
>> -                       0, 0x34, 0xE4, aout_tlv),
>> +                       8, 0xffffff19, 0x18, aout_tlv),
>>          SOC_DOUBLE_R_SX_TLV("ADC Mixer Volume",
>>
>> shifting 8 on an 8bit register?
>
> These values are just the ones that were here before your commit
> 1d99f2436d0d1c7741d6dfd9d27b5376cdbbca40. I didn't invent anything,
> just reverted partially your commit.
>
Oh yes, of course. I was just making the comment that it looks very 
strange to have that shift value. I'm glad it works for your system and 
you can go forward with your development.

Once I get it figured out, I'll CC you on the submission for your review 
and test.

Thanks again,

Re: Regression caused by "ASoC: core: Rework SOC_DOUBLE_R_SX_TLV add SOC_SINGLE_SX_TLV"

[Thomas Petazzoni]

Dear Brian Austin,

On Thu, 30 Jan 2014 11:23:57 -0600, Brian Austin wrote:

> >>          SOC_DOUBLE_R_SX_TLV("Analog Playback Volume",
> >>                          CS42L51_AOUTA_VOL, CS42L51_AOUTB_VOL,
> >> -                       0, 0x34, 0xE4, aout_tlv),
> >> +                       8, 0xffffff19, 0x18, aout_tlv),
> >>          SOC_DOUBLE_R_SX_TLV("ADC Mixer Volume",
> >>
> >> shifting 8 on an 8bit register?
> >
> > These values are just the ones that were here before your commit
> > 1d99f2436d0d1c7741d6dfd9d27b5376cdbbca40. I didn't invent anything,
> > just reverted partially your commit.
> >
> Oh yes, of course. I was just making the comment that it looks very 
> strange to have that shift value. I'm glad it works for your system and 
> you can go forward with your development.
> 
> Once I get it figured out, I'll CC you on the submission for your review 
> and test.

Any news about this volume control problem on cs42l51 ?

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: Regression caused by "ASoC: core: Rework SOC_DOUBLE_R_SX_TLV add SOC_SINGLE_SX_TLV"

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 463 bytes --]

On Mon, Feb 10, 2014 at 03:22:44PM +0100, Thomas Petazzoni wrote:
> On Thu, 30 Jan 2014 11:23:57 -0600, Brian Austin wrote:

> > Once I get it figured out, I'll CC you on the submission for your review 
> > and test.

> Any news about this volume control problem on cs42l51 ?

What does the datasheet say about these bitfields?  It should be
straightforward to figure out how to fix them, it seems clear that the
original code only worked by accident.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: Regression caused by "ASoC: core: Rework SOC_DOUBLE_R_SX_TLV add SOC_SINGLE_SX_TLV"

[Thomas Petazzoni]

Dear Mark Brown,

On Fri, 14 Feb 2014 20:17:09 +0000, Mark Brown wrote:
> On Mon, Feb 10, 2014 at 03:22:44PM +0100, Thomas Petazzoni wrote:
> > On Thu, 30 Jan 2014 11:23:57 -0600, Brian Austin wrote:
> 
> > > Once I get it figured out, I'll CC you on the submission for your review 
> > > and test.
> 
> > Any news about this volume control problem on cs42l51 ?
> 
> What does the datasheet say about these bitfields?  It should be
> straightforward to figure out how to fix them, it seems clear that the
> original code only worked by accident.

For the PCMA and PCMB registers (addresses 0x10 and 0x11), the
datasheet says that bits 0 to 6 contain the volume, and bit 7 contains
the mute/unmute attribute.

The volume can be adjusted in 0.5 dB increments, with the following
values described in the datasheet:

 001 1000		+12.0 dB
 ........		........
 000 0000		0 dB
 111 1111		-0.5 dB
 111 1110		-1.0 dB
 ........		........
 001 1001		-51.5 dB

Note that I haven't looked myself in details at the code of the audio
codec. I simply noticed there was a volume control problem and that
reverting Brian's patch made the volume control work again. I have not
investigated at all to fix the codec code, as I was suspecting that
Brian may have immediately an idea on what could be wrong. If he
doesn't have the time to look into this, I can certainly spend a bit of
time to investigate what's going on.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: Regression caused by "ASoC: core: Rework SOC_DOUBLE_R_SX_TLV add SOC_SINGLE_SX_TLV"

[Michael Trimarchi]

Hi

On Mon, Feb 17, 2014 at 2:14 PM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Dear Mark Brown,
>
> On Fri, 14 Feb 2014 20:17:09 +0000, Mark Brown wrote:
>> On Mon, Feb 10, 2014 at 03:22:44PM +0100, Thomas Petazzoni wrote:
>> > On Thu, 30 Jan 2014 11:23:57 -0600, Brian Austin wrote:
>>
>> > > Once I get it figured out, I'll CC you on the submission for your review
>> > > and test.
>>
>> > Any news about this volume control problem on cs42l51 ?
>>
>> What does the datasheet say about these bitfields?  It should be
>> straightforward to figure out how to fix them, it seems clear that the
>> original code only worked by accident.
>
> For the PCMA and PCMB registers (addresses 0x10 and 0x11), the
> datasheet says that bits 0 to 6 contain the volume, and bit 7 contains
> the mute/unmute attribute.
>
> The volume can be adjusted in 0.5 dB increments, with the following
> values described in the datasheet:
>
>  001 1000               +12.0 dB
>  ........               ........
>  000 0000               0 dB
>  111 1111               -0.5 dB
>  111 1110               -1.0 dB
>  ........               ........
>  001 1001               -51.5 dB
>
> Note that I haven't looked myself in details at the code of the audio
> codec. I simply noticed there was a volume control problem and that
> reverting Brian's patch made the volume control work again. I have not
> investigated at all to fix the codec code, as I was suspecting that
> Brian may have immediately an idea on what could be wrong. If he
> doesn't have the time to look into this, I can certainly spend a bit of
> time to investigate what's going on.
>
why you don't just use regmap and check what happen on registers?


Michael

> Thanks!
>
> Thomas
> --
> Thomas Petazzoni, CTO, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: Regression caused by "ASoC: core: Rework SOC_DOUBLE_R_SX_TLV add SOC_SINGLE_SX_TLV"

[Michael Trimarchi]

Hi

On Mon, Feb 17, 2014 at 3:59 PM, Michael Trimarchi
<michael@amarulasolutions.com> wrote:
> Hi
>
> On Mon, Feb 17, 2014 at 2:14 PM, Thomas Petazzoni
> <thomas.petazzoni@free-electrons.com> wrote:
>> Dear Mark Brown,
>>
>> On Fri, 14 Feb 2014 20:17:09 +0000, Mark Brown wrote:
>>> On Mon, Feb 10, 2014 at 03:22:44PM +0100, Thomas Petazzoni wrote:
>>> > On Thu, 30 Jan 2014 11:23:57 -0600, Brian Austin wrote:
>>>
>>> > > Once I get it figured out, I'll CC you on the submission for your review
>>> > > and test.
>>>
>>> > Any news about this volume control problem on cs42l51 ?
>>>
>>> What does the datasheet say about these bitfields?  It should be
>>> straightforward to figure out how to fix them, it seems clear that the
>>> original code only worked by accident.
>>
>> For the PCMA and PCMB registers (addresses 0x10 and 0x11), the
>> datasheet says that bits 0 to 6 contain the volume, and bit 7 contains
>> the mute/unmute attribute.
>>
>> The volume can be adjusted in 0.5 dB increments, with the following
>> values described in the datasheet:
>>
>>  001 1000               +12.0 dB
>>  ........               ........
>>  000 0000               0 dB
>>  111 1111               -0.5 dB
>>  111 1110               -1.0 dB
>>  ........               ........
>>  001 1001               -51.5 dB
>>
>> Note that I haven't looked myself in details at the code of the audio
>> codec. I simply noticed there was a volume control problem and that
>> reverting Brian's patch made the volume control work again. I have not
>> investigated at all to fix the codec code, as I was suspecting that
>> Brian may have immediately an idea on what could be wrong. If he
>> doesn't have the time to look into this, I can certainly spend a bit of
>> time to investigate what's going on.
>>
> why you don't just use regmap and check what happen on registers?
>

should be /sys/kernel/debug/regmap/...

Michael

>
> Michael
>
>> Thanks!
>>
>> Thomas
>> --
>> Thomas Petazzoni, CTO, Free Electrons
>> Embedded Linux, Kernel and Android engineering
>> http://free-electrons.com
>> _______________________________________________
>> Alsa-devel mailing list
>> Alsa-devel@alsa-project.org
>> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: Regression caused by "ASoC: core: Rework SOC_DOUBLE_R_SX_TLV add SOC_SINGLE_SX_TLV"

[Thomas Petazzoni]

Dear Michael Trimarchi,

On Mon, 17 Feb 2014 15:59:35 +0100, Michael Trimarchi wrote:

> > Note that I haven't looked myself in details at the code of the audio
> > codec. I simply noticed there was a volume control problem and that
> > reverting Brian's patch made the volume control work again. I have not
> > investigated at all to fix the codec code, as I was suspecting that
> > Brian may have immediately an idea on what could be wrong. If he
> > doesn't have the time to look into this, I can certainly spend a bit of
> > time to investigate what's going on.
> >
> why you don't just use regmap and check what happen on registers?

I did have a look at the codec_reg debugfs file from ASoC, and the
values were indeed not correct, as far as I remember. But I don't have
the board powered up right now, so I'd need to set it up again to make
a more precise report of what's going on.

Thanks,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: Regression caused by "ASoC: core: Rework SOC_DOUBLE_R_SX_TLV add SOC_SINGLE_SX_TLV"

[Brian Austin]

On Mon, 10 Feb 2014, Thomas Petazzoni wrote:

> Dear Brian Austin,
>
> On Thu, 30 Jan 2014 11:23:57 -0600, Brian Austin wrote:
>
>>>>          SOC_DOUBLE_R_SX_TLV("Analog Playback Volume",
>>>>                          CS42L51_AOUTA_VOL, CS42L51_AOUTB_VOL,
>>>> -                       0, 0x34, 0xE4, aout_tlv),
>>>> +                       8, 0xffffff19, 0x18, aout_tlv),
>>>>          SOC_DOUBLE_R_SX_TLV("ADC Mixer Volume",
>>>>
>>>> shifting 8 on an 8bit register?
>>>
>>> These values are just the ones that were here before your commit
>>> 1d99f2436d0d1c7741d6dfd9d27b5376cdbbca40. I didn't invent anything,
>>> just reverted partially your commit.
>>>
>> Oh yes, of course. I was just making the comment that it looks very
>> strange to have that shift value. I'm glad it works for your system and
>> you can go forward with your development.
>>
>> Once I get it figured out, I'll CC you on the submission for your review
>> and test.
>
> Any news about this volume control problem on cs42l51 ?
>
> Thanks!
>
> Thomas
Hello Thomas,

I found the issue WRT the CS42L51. It was actually the shift values for 
the kcontrols were wrong :)

I have the changes and will be sending the patch shortly.  Your CC'd as 
reporter if that's OK.

Thanks,
Brian

Re: Regression caused by "ASoC: core: Rework SOC_DOUBLE_R_SX_TLV add SOC_SINGLE_SX_TLV"

[Thomas Petazzoni]

Dear Brian Austin,

On Wed, 19 Mar 2014 10:31:12 -0500, Brian Austin wrote:

> I found the issue WRT the CS42L51. It was actually the shift values for 
> the kcontrols were wrong :)
> 
> I have the changes and will be sending the patch shortly.  Your CC'd as 
> reporter if that's OK.

Excellent! Thanks for the investigation. I'll be testing your patch
next week as I get access to the appropriate platform.

Can you ensure to Cc: stable@ to get the patch included in the
appropriate previous kernel releases?

Thanks a lot!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: Regression caused by "ASoC: core: Rework SOC_DOUBLE_R_SX_TLV add SOC_SINGLE_SX_TLV"

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 193 bytes --]

On Wed, Mar 19, 2014 at 05:46:02PM +0100, Thomas Petazzoni wrote:

> Can you ensure to Cc: stable@ to get the patch included in the
> appropriate previous kernel releases?

I did that already.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: Regression caused by "ASoC: core: Rework SOC_DOUBLE_R_SX_TLV add SOC_SINGLE_SX_TLV"

[Thomas Petazzoni]

Dear Mark Brown,

On Wed, 19 Mar 2014 16:50:50 +0000, Mark Brown wrote:
> On Wed, Mar 19, 2014 at 05:46:02PM +0100, Thomas Petazzoni wrote:
> 
> > Can you ensure to Cc: stable@ to get the patch included in the
> > appropriate previous kernel releases?
> 
> I did that already.

Great, thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com