Re: [PATCH 5.3 112/112] ASoC: sgtl5000: add ADC mute control

[Mark Brown <broonie@kernel.org>]

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

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]