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

[Hans de Goede]

Hi,

On 3/20/21 8:41 AM, Takashi Iwai wrote:
> 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 at 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.

Yes setting the new LED-access flags from the machine driver(s) would
work too for the 3 devices (spanning 2 machine drivers) on which
I'm trying to get the mute-LED to work, assuming Mark is going to be
ok with that approach. But those are pretty simple devices.

There also is the recently posted Dell privacy stuff which is
also using LED-triggers on a "full-blown" Intel core series laptop,
which use codecs in much more varied ways. And I've the feeling that
we will see more of this stuff coming up and in those cases the extra
flexibility which going through UCM gives us would be good I think.

I believe that that Dell privacy stuff is actually the reason why
Jaroslav started this whole series, right Jaroslav ?

I'm just piggy backing along with my own use-cases which I had
on my wishlist / itches-list for a while now :)

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

Right.

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

With other OS you mean e.g. Android?  Android has device-specific
init-scripts which can either call alsactl or directly do the
echo-s.

Regards,

Hans