[RFC 2/2] ASoC: rt5670: Add LED trigger support

[Hans de Goede]

Hi,

On 2/23/21 5:14 PM, Jaroslav Kysela wrote:
> Dne 23. 02. 21 v 15:21 Takashi Iwai napsal(a):
>> On Tue, 23 Feb 2021 15:09:30 +0100,
>> Mark Brown wrote:
>>>
>>> On Tue, Feb 23, 2021 at 02:59:17PM +0100, Hans de Goede wrote:
>>>> On 2/23/21 2:45 PM, Mark Brown wrote:
>>>>> On Mon, Feb 15, 2021 at 03:24:19PM +0100, Hans de Goede wrote:
>>>
>>>>> Why just these particular controls - what if a system has separate mutes
>>>>> for speakers or something?
>>>
>>>> These are the main volume controls, which are always in the output / input
>>>> path independent on if we are outputting to e.g. speakers or the headphones.
>>>
>>>> We want to use the main volume control for this, because there always is
>>>> only 1 output mute LED and 1 input mute LED. Well at least that is the assumption
>>>> the current ledtrig-audio.c code has.
>>>
>>>> The idea is to only turn the single LED on if we are sure there will be not
>>>> sound output on any of the outputs, which is why we tie the LED to the
>>>> mute switch on the main volume control.
>>>
>>> Right, so that might work well on your particular system with your
>>> particular configuration but will it work well on other systems with
>>> different hardware?  It's not clear to me that it makes sense to go
>>> through all the drivers picking controls that might be used for this
>>> purpose - it seems both time consuming and error prone.  Consider a
>>> mostly digital device which has an ADC/DAC per input/output rather than
>>> a central ADC/DAC with analogue muxing for example, or a system with
>>> multiple DACs available for mixing together or analogue bypassess.  
>>
>> That's one of my concerns in the recent actions for putting the
>> hard-coded mute LED controls.  So far, the only usage of led-audio
>> trigger is HD-audio, and it's enabled only for selected devices and
>> setups.  OTOH, if we apply the audio-led trigger generically in ASoC
>> codec driver, it's always done and might misfit; e.g. what happens if
>> two codecs are present on the system?.
> 
> That's the abstraction issue. We can use PCI, ACPI, DMI or DT checks at the
> _right_ place (machine top-level code) to mark those controls with the LED
> flags in the kernel space. I've never said that the right place is the generic
> ASoC codec driver.

FWIW I'm more then happy to rework this RFC series to make the setting of the
SNDRV_CTL_ELEM_ACCESS_*_LED flag only happen on certain models based on the
existing DMI quirks mechanism in the driver.

So far I'm aware of only a few 2-in-1 models which actually have a mute LED
embedded in the (mic)mute key in their detachable keyboard.

Please let me know if you would prefer for me to rework my series
to handle the setting of the flag on a per model (DMI match) basis.

I see 2 additional advantages of this approach:

1. It would allow us to not modprobe the snd-ctl-led module on devices
where we don't need it.

2. The rt5640 codec has 2 I2S interfaces / 2 AIFs and that brings
some challenges with it. I currently have a working implementation
(which needs some more testing before posting it upstream as RFC) which
sets the SNDRV_CTL_ELEM_ACCESS_SPK_LED flag on the main volume control
for both the AIF1 and AIF2 paths to the outputs (which have separate
main volume controls) and then relies on userspace to always mute the
main volume of the unused AIF.

This requires the UCM profile to know which AIF is being used, if we
do this on a per model basis then we will know which AIF is used and
we can set the SNDRV_CTL_ELEM_ACCESS_SPK_LED flag on only one of the
2 main volume-controls, removing the requirement for userspace to
mute the unused main volume-control.

###

Note that userspace will still need to know which AIF is being used,
regardless of the mute-LED discussion, because it needs to know to
be able select the right volume-control for hardware volume control
(which is something which we really want in general, esp. for the
analog mic inputs and is necessary for the mute-LED to work).

Since userspace needs to know which AIF is being used, I've written
a kernel patch modifying the machine-driver to add a new "aif:1" or
"aif:2" string to the components string.

I have this all working now, and I will post a RFC upstream once
I've run some final tests, for the UCM changes for this, which
illustrate the need for userspace to know which AIF is used, see:

https://github.com/jwrdegoede/alsa-ucm-conf/commits/master

And specifically:

https://github.com/jwrdegoede/alsa-ucm-conf/commit/afce963f725a5659eb903508d924d3b19110244a
https://github.com/jwrdegoede/alsa-ucm-conf/commit/d2128e5380d9d6128dda67dd94f64bf7244cd3a3

Regards,

Hans