Re: [PATCH v4 6/6] ALSA: led control - add sysfs kcontrol LED marking layer

[Takashi Iwai <tiwai@suse.de>]

On Fri, 19 Mar 2021 23:08:33 +0100,
Hans de Goede wrote:
> 
> Hi,
> 
> On 3/19/21 6:22 PM, Takashi Iwai wrote:
> > On Fri, 19 Mar 2021 17:34:39 +0100,
> > Hans de Goede wrote:
> >>
> >> Hi,
> >>
> >> On 3/17/21 6:29 PM, Jaroslav Kysela wrote:
> >>> We need to manage the kcontrol entries association for the LED trigger
> >>> from the user space. This patch adds a layer to the sysfs tree like:
> >>>
> >>> /sys/devices/virtual/sound/ctl-led/mic
> >>>    + card0
> >>>    |  + attach
> >>>    |  + detach
> >>>    |  ...
> >>>    + card1
> >>>       + attach
> >>>       ...
> >>>
> >>> Operations:
> >>>
> >>>   attach and detach
> >>>     - amixer style ID is accepted and easy strings for numid and
> >>>       simple names
> >>>   reset
> >>>     - reset all associated kcontrol entries
> >>>   list
> >>>     - list associated kcontrol entries (numid values only)
> >>>
> >>> Additional symlinks:
> >>>
> >>> /sys/devices/virtual/sound/ctl-led/mic/card0/card ->
> >>>   /sys/class/sound/card0
> >>>
> >>> /sys/class/sound/card0/controlC0/led-mic ->
> >>>   /sys/devices/virtual/sound/ctl-led/mic/card0
> >>>
> >>> Signed-off-by: Jaroslav Kysela <perex@perex.cz>
> >>
> >> Thank you so much for this patch.
> >>
> >> I've given this new version a try, dropping my sound/soc/codecs/rt56??.c patches to set the access-flags directly.
> >>
> >> And with these 3 lines in /etc/rc.d/rc.local I get nicely working control of the mute
> >> LED build into the (detachable) USB-keyboard's mute hot-key:
> >>
> >> modprobe snd_ctl_led
> >> echo -n name="Speaker Channel Switch" > /sys/class/sound/card1/controlC1/led-speaker/attach
> >> echo -n name="HP Channel Switch" > /sys/class/sound/card1/controlC1/led-speaker/attach
> >>
> >> This needs to be replaced by some UCM profile code doing the equivalent of course,
> >> but for a proof-of-concept test of the kernel API this introduces the above will do.
> > 
> > IMO, that's the question: how we'll enable this in future.  If the
> > binding of the control/mute mapping is provided via UCM, it's supposed
> > to be changeable by each user.  Then the current sysfs permission
> > doesn't fit.  OTOH, if it's 0666, it's accessible to all users even
> > remotely, which is worse than the access with the normal sound device
> > file.  Or if it's supposed to be changed via udev stuff or systemd?
> > Or is it just for debugging?
> > 
> > Through a quick glance over the series, I'm fine to take those
> > patches, but the only concern is the sysfs entries.  Basically, once
> > when we use sysfs entries, it's set in stone.  So we should be very
> > clear about our strategy how to deploy the control/mute mapping
> > regarding using those sysfs entries.
> > 
> > OTOH, if the interface is thought for debugging or development
> > purpose, it could be done in debugfs, which we can keep playing in
> > further development, too.
> > 
> > And, BTW, the mute LED mode setup doesn't have to be sysfs entries;
> > we'd need primarily only the flags for inverted LED behavior, and
> > those are only two, so it could be simply module options.  Then it's
> > even easier for users to set up than tweaking sysfs entries.
> 
> The flexibility offered by this new sysfs API is necessary for the ASoC
> codec drivers, because Mark does not want to have which controls are
> tied to the LED triggers hard-coded inside the codec drivers.

The hard-coded mapping itself isn't always bad things, IMO.  Of
course, it's a question whether to be done in the codec driver in a
fixed routing.  A machine driver would fit well, instead; i.e. instead
of the control-access bit flag, just bind statically from the machine
driver after instantiating the kctl objects like sysfs does.

> So as Jaroslav mentions in his reply, the plan is to have the UCM
> profiles contain commands to setup the LED triggers to this new
> sysfs API.

IIUC, this won't be only UCM but also the combination of udev +
alsactl + UCM, right?

Would other OS can follow a similar pattern?  Let's check that first
(although I myself think this should be feasible).


thanks,

Takashi