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

[Hans de Goede]

Hi,

On 3/1/21 8:21 PM, Hans de Goede wrote:
> 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.

I think we might be conversion on a solution for this in the other
thread (see the email which I am about to send but have not sent
yet), so lets continue this discussion there.

Regards,

Hans