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

From: Mark Brown <broonie@kernel.org>
To: Hans de Goede <hdegoede@redhat.com>
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 18:55:57 +0000	[thread overview]
Message-ID: <20210301185557.GG4628@sirena.org.uk> (raw)
In-Reply-To: <20210226143817.84287-4-hdegoede@redhat.com>

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

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 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.

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.  One
could equally argue that a software defined mute control should be
muting all the analogue outputs, it'd certainly seem more robust.

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