[alsa-devel] Regression caused by "ASoC: core: Rework SOC_DOUBLE_R_SX_TLV add SOC_SINGLE_SX_TLV"
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
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!
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
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
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
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
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
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
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,
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
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.
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
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
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
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
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
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
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
participants (4)
-
Brian Austin -
Mark Brown -
Michael Trimarchi -
Thomas Petazzoni