Re: [PATCH 0/5] ALSA: control - add generic LED trigger code

[Takashi Iwai <tiwai@suse.de>]

On Thu, 11 Feb 2021 12:13:55 +0100,
Jaroslav Kysela wrote:
> 
> This patchset tries to resolve the diversity in the audio LED
> control among the ALSA drivers.
> 
> A new control layer registration is introduced which allows
> to run additional operations on top of the elementary ALSA
> sound controls.
> 
> A new control access group (three bits in the access flags)
> was introduced to carry the LED group information for
> the sound controls. The low-level sound drivers can just
> mark those controls using this access group. This information
> is exported to the user space and eventually the user space
> can create sound controls which can belong to a LED group.
> 
> The actual state ('route') evaluation is really easy
> (the minimal value check for all channels / controls / cards).
> If there's more complicated logic for a given hardware,
> the card driver may eventually export a new read-only
> sound control for the LED group and do the logic itself.
> 
> The new LED trigger control code is completely separated
> and possibly optional (there's no symbol dependency).
> The full code separation allows eventually to move this
> LED trigger control to the user space in future.
> Actually it replaces the already present functionality
> in the kernel space (HDA drivers) and allows a quick adoption
> for the recent hardware (SoundWire ASoC codecs).
> 
> # lsmod | grep snd_ctl_led
> snd_ctl_led            16384  0
> 
> The sound driver implementation is really easy:
> 
> 1) call snd_ctl_led_request() when control LED layer should be
>    automatically activated
>    / it calls module_request("snd-ctl-led") on demand /
> 2) mark all related kcontrols with
> 	SNDRV_CTL_ELEM_ACCESS_SPK_LED or
> 	SNDRV_CTL_ELEM_ACCESS_MIC_LED
> 
> Original RFC: https://lore.kernel.org/alsa-devel/20210207201157.869972-1-perex@perex.cz/
> 
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Perry Yuan <Perry.Yuan@dell.com>
> 
> Jaroslav Kysela (5):
>   ALSA: control - introduce snd_ctl_notify_one() helper
>   ALSA: control - add layer registration routines
>   ALSA: control - add generic LED trigger module as the new control
>     layer
>   ALSA: HDA - remove the custom implementation for the audio LED trigger
>   ALSA: control - add sysfs support to the LED trigger module

Thanks for the patch.

I'm afraid that it's a bit too late for 5.12, as the merge window will
be likely closed soon.  For 5.13, we'll have enough time for get a
consensus about the design.  Whether this is the best way to go, or we
should rather consider user-space solution as Sakamoto-san mentioned:
that has to be decided.

Back to the design: your new implementation allows the separation and
the dynamic opt-in, which is nice.  Thanks for that.  This looks
generic and may be extended for other purposes in future, too.

One thing I still miss from the picture is how to deal with the case
like AMD ACP.  It has no mixer control to bundle with the LED trigger.
Your idea is to make a (dummy) user element and tie the LED trigger
with it?


Another slight concern is the possible regression: by moving the
mute-LED mode enum stuff into the sysfs, user will get
incompatibilities after the kernel update.  And it's not that trivial
to change the sysfs entry as default for each user.
It needs some detailed documentation or some temporary workaround
(e.g. keep providing the controls for now but warns if the value is
changed from the default value via the controls).


thanks,

Takashi