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

[Hans de Goede]

Hi,

On 3/1/21 8:15 PM, Mark Brown wrote:
> On Mon, Mar 01, 2021 at 02:39:46PM +0100, Hans de Goede wrote:
>> On 3/1/21 2:23 PM, Mark Brown wrote:
> 
>>> I don't want to get stuck in a cycle of "why can't my system just do
>>> what this other system does", or worse end up with problems due to
>>> competing system requirements when patches go in on more flexible
>>> devices because I didn't notice that the device wasn't a good fit for
>>> this sort of thing but people have the expectation that the kernel will
>>> transparently handle things.
> 
>> So what do you want / how do you want this to work ?
> 
> Off the top of my head something like writing a control name into a
> sysfs file might work, it doesn't scale if you need to use multiple
> controls as rt5640 does though.

Currently ALSA/UCM does not use sysfs files for anything, so this
feels very inconsistent with how all the rest of this currently works.

A sysfs file is also only accessible by root, making it impossible for
say pulseaudio or pipewire running as a normal user to set this up.

So then we would need a database for this somewhere outside of UCM and
a process running as root to write the sysfs file.

> We could also do something like have
> drivers make a list of all output stage mutes and then use that to build
> a standard global mute control which functions similarly to this one and
> could be force wired to the LED trigger input, seems like a big hammer
> but it'd be reasonably consistent.

This sounds like a better plan IMHO.

Jaroslav's current mixer-element based generic LED support patchset
already allows setting the "this mixer-element control the spk-mute led"
on multiple controls and then the LED will only turn on if all such
controls are set to muted.

So that part is covered and my initial plan for the rt5640 was actually
to do just this but the problem is that the mixer element names for
the outputs were poorly chosen initially:

        /* Speaker Output Volume */
        SOC_DOUBLE("Speaker Channel Switch", RT5640_SPK_VOL,
                RT5640_VOL_L_SFT, RT5640_VOL_R_SFT, 1, 1),
        SOC_DOUBLE_TLV("Speaker Playback Volume", RT5640_SPK_VOL,
                RT5640_L_VOL_SFT, RT5640_R_VOL_SFT, 39, 1, out_vol_tlv),

Where userspace expect "Speaker Channel Switch" to be named
"Speaker Playback Switch" (aligning it with the vol-control name)
instead.

And we cannot just rename this since the control names are
used in UCM profiles and if a UCM profile refers to a non-existing
control it won't work.

I recently did an alsa-lib patch-series to deal with other
non ideally named mixer controls:

https://lore.kernel.org/alsa-devel/20210228161304.241288-1-hdegoede@redhat.com/T/#t

I guess that we could deal with this unfortunate naming inside
alsa-lib too. I guess that the alsa-lib folks won't be thrilled
by doing this, but we need to deal with this mess without breaking
compatibility somewhere and alsa-lib might be the best place for this.

Or we could change the "Speaker Channel Switch" control into a dummy control,
the current UCM profiles already disable a bunch of other controls in the
DAPM graph too, so the whole chain will turn off anyways.

And then add a new non-dummy "Speaker Playback Switch" control.

I do know that we need to much more careful going forward to make sure
that control names match the conventions expected by userspace.

Regards,

Hans