> 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 <[email protected]>
> >>
> >>>>> 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.
--
Best regards
Oleksandr Suvorov
Toradex AG
Altsagenstrasse 5 | 6048 Horw/Luzern | Switzerland | T: +41 41 500
4800 (main line)
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.
> 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.
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.
> 2. 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.
> 3. 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.
> > 2. 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.