Re: [PATCH 3/5] ASoC: rt5640: Add emulated 'DAC1 Playback Switch' control

From: Hans de Goede <hdegoede@redhat.com>
To: Mark Brown <broonie@kernel.org>
Cc: Oder Chiou <oder_chiou@realtek.com>,
	Cezary Rojewski <cezary.rojewski@intel.com>,
	alsa-devel@alsa-project.org, Jie Yang <yang.jie@linux.intel.com>,
	Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>,
	Liam Girdwood <lgirdwood@gmail.com>
Subject: Re: [PATCH 3/5] ASoC: rt5640: Add emulated 'DAC1 Playback Switch' control
Date: Mon, 1 Mar 2021 20:21:56 +0100	[thread overview]
Message-ID: <23ef6073-ffeb-c7ea-1365-63f3e78f241d@redhat.com> (raw)
In-Reply-To: <20210301185557.GG4628@sirena.org.uk>

Hi,

On 3/1/21 7:55 PM, Mark Brown wrote:
> On Fri, Feb 26, 2021 at 03:38:15PM +0100, Hans de Goede wrote:
>> When using AIF1 the 'DAC1 Playback Volume' control will be used as the
>> PlaybackMasterElem in UCM.
>>
>> We need a matching 'DAC1 Playback Switch' for 2 reasons:
>>
>> 1. To be able to truely fully mute the output (the softest volume setting
>>    is not fully muted).
>> 2. For reliable output-mute LED control.
>>
>> The path from the IF1_DAC data input to the 'Stereo DAC MIXL' /
>> 'Stereo DAC MIXR' digital mixer has a 'DAC MIXL' / 'DAC MIXR' digital
>> mixer with IF1_DAC data as one of its inputs direclty after the
>> 'DAC1 Playback Volume' control.
>>
>> This commit adds an emulated "DAC1 Playback Switch" control by:
> 
> This feels icky, it seems like if userspace needs to stitch together a
> stereo mute control that doesn't already exist in the hardware

But it does exist in the hardware the digital mixer bits around DAC1
have some more functionality then those around DAc2, including a mixer
directly behind the DAC1 volume-control which allows digital loopback.

The inputs to those mixer are all 4 (2 pairs of l/r) controlled by
mute bits. The codec designer has left out the mute switches normally
directly following the volume control since then there would be 2 mute
switches in series, one as part of the volume-control/mute combo which
is usually used and 1 directly behind that to mute/unmute the mixer
input.

This is a nice hw optimization, but annoying to deal with in software,
esp. since userspace generally expects a "Foo Playback Volume",
"Foo Playback Switch" pair. By for the easiest way to deal with this
is to undo the hw optimization in software and add the expected
"Foo Playback Switch"

> from
> existing mono controls then UCM ought to have support for figuring that
> out anyway or if we *must* bodge this in the kernel there should be some
> generic way of doing it rather than open coding in drivers.

This is highly codec specific. So far this has not really been an
issue because so far on asoc based systems regular Linux userspace has
always been using software volume-control. But now that we are starting
to use hardware volume-control it really is desirable to also have
a hardware mute switch available.

Also problematic here is that things like volume-controls and their
accompanying mute switches (often bit 15 + 8 of the same register)
are modules as "normal" mixer elements which are not seen as part of
the DAPM graph, where as the mixer in this case is part of the DAPM graph.

> It also makes the whole mute LED thing feel a lot messier even for
> existing systems than you seemed to be suggesting in the other thread.
> This device has two I2S interfaces, two DACs (only one of which seems to
> be affected by this control), and it appears that there's bypass path
> from the ADC to DAC1 which won't be muted by the newly added mute switch
> here so this reliable mute control won't be entirely reliable.  There
> look to also be some analogue bypass paths, I didn't fully check.

I don't believe that it is necessary to take bypass / loopback paths into
account for the playback mute LED. These are normally always off and they
don't involve the normal playback paths. So even if they are on any
audio played from within the OS is still muted.

> One
> could equally argue that a software defined mute control should be
> muting all the analogue outputs, it'd certainly seem more robust.

The mute switches in the analog output paths are part of the DAPM
graph, which means that they will get turned off automatically to
save power when the audio device is not playing audio (is not kept
open by userspace). AFAIK this makes them unsuitable to be used as a
source for the mute-led trigger, we want the mute LED to turn on
when the volume control is set to mute, not when the last app
closes the audio device.

I honestly don't understand your objections against the current
set of patches for dealing with the mute-led trigger. Your main
worry seems to be that this is not flexible enough, but it currently
is all handled inside the kernel. So if more complex cases come up
then we can easily adjust the code to deal with this, since there
is no userspace API part to worry about. And if these more complex
cases do require more userspace involvement then we can worry about
that then (and not now) when we actually have a concrete example of
what such a more complex setup could look like and thus also have
something to actually design any userspace api for this around.

Regards,

Hans