On 17/10/2019 01:23, Greg Kroah-Hartman wrote:
On Wed, Oct 16, 2019 at 11:35:18PM +0100, Mark Brown wrote:
On Wed, Oct 16, 2019 at 03:10:25PM -0700, Greg Kroah-Hartman wrote:
On Wed, Oct 16, 2019 at 11:00:44PM +0100, Mark Brown wrote:
On Wed, Oct 16, 2019 at 02:51:44PM -0700, Greg Kroah-Hartman wrote:
From: Oleksandr Suvorov oleksandr.suvorov@toradex.com
commit 694b14554d75f2a1ae111202e71860d58b434a21 upstream.
This control mute/unmute the ADC input of SGTL5000 using its CHIP_ANA_CTRL register.
This seems like a new feature and not an obvious candidate for stable?
there was a long email from Richard that said: Upstream commit 631bc8f0134a ("ASoC: sgtl5000: Fix of unmute outputs on probe"), which is e9f621efaebd in v5.3 replaced snd_soc_component_write with snd_soc_component_update_bits and therefore no longer cleared the MUTE_ADC flag. This caused the ADC to stay muted and recording doesn't work any longer. This patch fixes this problem by adding a Switch control for MUTE_ADC.
That's why I took this. If this isn't true, I'll be glad to drop this.
That's probably not an appropriate fix for stable - it's going to add a new control which users will need to manually set (or hope their userspace automatically figures out that it should set for them, more advanced userspaces like PulseAudio should) which isn't a drop in fix. You could either drop the backport that was done for zero cross or take a new patch that clears the MUTE_ADC flag (rather than punting to userspace to do so), or just be OK with what you've got at the minute which might be fine given the lack of user reports.
Mark, obviously this is not a NEW feature. This patch adds LOST standard control.
Please look into other codecs:
$ grep -R 'SOC_SINGLE("Capture Switch"' sound/soc/ sound/soc/codecs/sgtl5000.c.orig: SOC_SINGLE("Capture Switch", SGTL5000_CHIP_ANA_CTRL, 0, 1, 1), sound/soc/codecs/wm9705.c: SOC_SINGLE("Capture Switch", AC97_REC_GAIN, 15, 1, 1), sound/soc/codecs/wm9713.c:SOC_SINGLE("Capture Switch", AC97_CD, 15, 1, 1), sound/soc/codecs/wm9712.c:SOC_SINGLE("Capture Switch", AC97_REC_GAIN, 15, 1, 1),
And even:
$ grep -R 'SOC_SINGLE(".*Capture Switch"' sound/soc/ sound/soc/codecs/lm49453.c: SOC_SINGLE("Port1 Capture Switch", LM49453_P0_AUDIO_PORT1_BASIC_REG, sound/soc/codecs/lm49453.c: SOC_SINGLE("Port2 Capture Switch", LM49453_P0_AUDIO_PORT2_BASIC_REG, sound/soc/codecs/sgtl5000.c.orig: SOC_SINGLE("Capture Switch", SGTL5000_CHIP_ANA_CTRL, 0, 1, 1), sound/soc/codecs/mc13783.c: SOC_SINGLE("Line in Capture Switch", MC13783_AUDIO_RX1, 10, 1, 0), sound/soc/codecs/ak4642.c: SOC_SINGLE("ALC Capture Switch", ALC_CTL1, 5, 1, 0), sound/soc/codecs/adau1701.c: SOC_SINGLE("Master Capture Switch", ADAU1701_DSPCTRL, 4, 1, 0), sound/soc/codecs/alc5632.c: SOC_SINGLE("DMIC En Capture Switch", sound/soc/codecs/alc5632.c: SOC_SINGLE("DMIC PreFilter Capture Switch", sound/soc/codecs/wm9705.c: SOC_SINGLE("Capture Switch", AC97_REC_GAIN, 15, 1, 1), sound/soc/codecs/adau1977.c: SOC_SINGLE("ADC" #x " Highpass-Filter Capture Switch", \ sound/soc/codecs/adau1977.c: SOC_SINGLE("ADC" #x " DC Subtraction Capture Switch", \ sound/soc/codecs/isabelle.c: SOC_SINGLE("ULATX12 Capture Switch", ISABELLE_ULATX12_INTF_CFG_REG, sound/soc/codecs/ad1980.c:SOC_SINGLE("PCM Capture Switch", AC97_REC_GAIN, 15, 1, 1), sound/soc/codecs/ad1980.c:SOC_SINGLE("Phone Capture Switch", AC97_PHONE, 15, 1, 1), sound/soc/codecs/wm8731.c:SOC_SINGLE("Mic Capture Switch", WM8731_APANA, 1, 1, 1), sound/soc/codecs/uda1380.c:/**/ SOC_SINGLE("ADC Capture Switch", UDA1380_PGA, 15, 1, 1), /* MT_ADC */ sound/soc/codecs/wm9713.c:SOC_SINGLE("Capture Switch", AC97_CD, 15, 1, 1), sound/soc/codecs/es8316.c: SOC_SINGLE("ALC Capture Switch", ES8316_ADC_ALC1, 6, 1, 0), sound/soc/codecs/sgtl5000.c: SOC_SINGLE("Capture Switch", SGTL5000_CHIP_ANA_CTRL, 0, 1, 1), sound/soc/codecs/tlv320aic31xx.c: SOC_SINGLE("ADC Capture Switch", AIC31XX_ADCFGA, 7, 1, 1), sound/soc/codecs/da7210.c: SOC_SINGLE("Aux2 Capture Switch", DA7210_AUX2, 2, 1, 0), sound/soc/codecs/wm9712.c:SOC_SINGLE("Capture Switch", AC97_REC_GAIN, 15, 1, 1),
git blame sound/soc/codecs/wm9705.c ... 927b0aea93bb3 (Ian Molton 2009-01-19 17:23:11 +0000 87) SOC_SINGLE("Capture Switch", AC97_REC_GAIN, 15, 1, 1), ...
This control was added not later than 2009, so I doubt my patch could break something in current user-land.
Ok, I'll gladly go drop it, thanks!
Mark, thanks for the clarification! I haven't thought of breaking anything with the backport as it worked fine for our application.
On Thu, Oct 17, 2019 at 09:49:27AM +0000, Oleksandr Suvorov wrote:
Mark, obviously this is not a NEW feature. This patch adds LOST standard control.
It's a new feature for this CODEC to have control over the capture mute, other devices have of course had control over it for a long time but for this device it's a new feature.
On Thu, 2019-10-17 at 12:11 +0100, Mark Brown wrote:
On Thu, Oct 17, 2019 at 09:49:27AM +0000, Oleksandr Suvorov wrote:
Mark, obviously this is not a NEW feature. This patch adds LOST standard control.
It's a new feature for this CODEC to have control over the capture mute, other devices have of course had control over it for a long time but for this device it's a new feature.
All versions of driver sgtl5000 (since creating in 2011) has a bug in sgtl5000_probe(): ... snd_soc_write(codec, SGTL5000_CHIP_ANA_CTRL, SGTL5000_HP_ZCD_EN | SGTL5000_ADC_ZCD_EN); ... This command rewrites the whole register value instead of just enabling ZCD feature for headphone and adc.
This register has bits for HP/LineOut/ADC muting, thus sgtl5000_probe() always unmutes HP/LineOut/ADC.
Because of this bug, HP/LineOut/ADC somehow worked several years: these blocks unmuted upon the probing of device and never muted again.
And they could not be muted/unmuted - there were no controls for that.
[ IMHO unabling mute/unmute of main codec components is a bug too ].
On 2016, the commit 904a987345258 was introduced and added an ability to mute/unmute HP/LineOut, but not ADC unfortunately.
On the other hand, the bug in sgtl5000_probe() (HP/LineOut/ADC unmuting) leads to "pop" sound on driver loading (according to sgtl5000 manual, both HP/LineOut should be muted upon device initialization).
The bugfix 631bc8f0134ae should be applied only after 904a987345258 + 694b14554d75f.
So I see 3 ways:
1. drop this patch and revert 631bc8f0134ae in stable versions 4.19, 5.2, 5.3. So the bug with unmuting all outputs and ADC on device probing will still present in all kernel versions that include sgtl500 codec driver.
2. keep 631bc8f0134ae and add 694b14554d75f to 4.19, 5.2 and 5.3.
3. add 631bc8f0134ae to 4.4, 4.9 and 4.14 add 694b14554d75f to 4.4-5.3 add 904a987345258 to 4.4 So this bug will be fixed in all supported versions.
On Thu, Oct 17, 2019 at 02:16:09PM +0000, Oleksandr Suvorov wrote:
All versions of driver sgtl5000 (since creating in 2011) has a bug in sgtl5000_probe(): ... snd_soc_write(codec, SGTL5000_CHIP_ANA_CTRL, SGTL5000_HP_ZCD_EN | SGTL5000_ADC_ZCD_EN); ... This command rewrites the whole register value instead of just enabling ZCD feature for headphone and adc.
This register has bits for HP/LineOut/ADC muting, thus sgtl5000_probe() always unmutes HP/LineOut/ADC.
Yes, or at the very least this is a badly documented bit of intentional code. I suspect it may be the latter but at this point we can't tell.
- drop this patch and revert 631bc8f0134ae in stable versions 4.19,
5.2, 5.3. So the bug with unmuting all outputs and ADC on device probing will still present in all kernel versions that include sgtl500 codec driver.
This patch here being adding the userspace control of the switch and 631bc8f0134ae being the patch that made the ZC change only update the specific bits rather than write an absolute value to the register. This means that we end up with the audio unmuted but no user control over this at runtime. From a user perspective I think this is fine, it's not ideal that there's no control but they can still record.
Please include human readable descriptions of things like commits and issues being discussed in e-mail in your mails, this makes them much easier for humans to read.
- keep 631bc8f0134ae and add 694b14554d75f to 4.19, 5.2 and 5.3.
This means the patch that makes ZC only update the ZC bits and also the patch that makes the mutes user controllable, the default being muted. As I pointed out up thread this would mean that someone upgrading to a newer stable may need to change their userspace to do the unmute instead of having things unconditionally unmuted by the driver. This is not really what people expect from stable updates, we want them to be able to pull these in without thinking about it. i
To backport the addition of the controls to stable we'd need an additional change which sets the default for this control to unmuted, there's a case for having such a change upstream regardless but it's still not clear if any of these changes are really fixes in the sense that we mean for stable.
- add 631bc8f0134ae to 4.4, 4.9 and 4.14 add 694b14554d75f to 4.4-5.3 add 904a987345258 to 4.4
This is basically the same as 2 except it adds some more user controllable mute controls with 904a987345258 and does different things in different versions for reasons I'm not clear on. It has the same issue.
So this bug will be fixed in all supported versions.
It is not clear that this is even a bug in the first place, it's not full functionality but that doesn't mean that it's a bug it just means that there's some missing functionality.
On Thu, 2019-10-17 at 17:39 +0100, Mark Brown wrote:
All versions of driver sgtl5000 (since creating in 2011) has a bug in
sgtl5000_probe(): ... snd_soc_write(codec, SGTL5000_CHIP_ANA_CTRL, SGTL5000_HP_ZCD_EN | SGTL5000_ADC_ZCD_EN); ... This command rewrites the whole register value instead of just enabling ZCD feature for headphone and adc. This register has bits for HP/LineOut/ADC muting, thus sgtl5000_probe() always unmutes HP/LineOut/ADC.
Yes, or at the very least this is a badly documented bit of intentional code. I suspect it may be the latter but at this point we can't tell.
- keep 631bc8f0134ae and add 694b14554d75f to 4.19, 5.2 and 5.3.
This means the patch that makes ZC only update the ZC bits and also the patch that makes the mutes user controllable, the default being muted. As I pointed out up thread this would mean that someone upgrading to a newer stable may need to change their userspace to do the unmute instead of having things unconditionally unmuted by the driver. This is not really what people expect from stable updates, we want them to be able to pull these in without thinking about it. i
I think now I see, what you mean.
Of corse, we can change the original commit [ 631bc8f0134ae ASoC: sgtl5000: Fix of unmute outputs on probe ] for stable branches, unmuting ADC/HP/Lineout at the end of probing. We keep an original behaviour for users and reduce the possible 'pop' on probe.
But, to take significant advantage, we should add ADC control too, and, as this is a feature, it is undesirable for stable branches.
So probably the best decision is to roll original 631bc8f0134ae back in all stable branches where it was added.
Thank you, Mark. Your explanation is extremely useful.
To backport the addition of the controls to stable we'd need an additional change which sets the default for this control to unmuted, there's a case for having such a change upstream regardless but it's still not clear if any of these changes are really fixes in the sense that we mean for stable.
linux-stable-mirror@lists.linaro.org
-
Mark Brown -
Oleksandr Suvorov