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

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 3/2/21 10:14 PM, Jaroslav Kysela wrote:
> Dne 01. 03. 21 v 22:26 Hans de Goede napsal(a):
>> Hi,
>>
>> On 3/1/21 9:43 PM, Mark Brown wrote:
>>> On Mon, Mar 01, 2021 at 08:49:34PM +0100, Hans de Goede wrote:
>>>> On 3/1/21 8:15 PM, Mark Brown wrote:
>>>
>>>>> 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.
>>>
>>> Yes, you'd really want to add string controls in ALSA.
>>
>> Hmm, we already have SNDRV_CTL_ELEM_TYPE_BYTES controls. I think that will
>> work nicely actually, we can have the UCM conf file send a 0 terminated
>> string to the driver that way. It would be nice to have some syntactic
>> sugar on the UCM side to be able to actually specify a string instead
>> of an array of bytes, but I don't think we need any new userspace API
>> for this.
> 
> The LEDs are controlled per machine not per card. So do we need to create the 'Speaker/Mic LED Control' control for all cards?
> 
> Also, this change sounds really generic. The interface may be implemented in my proposed control led kernel module, not in the codec drivers.
> 
> The Mark's sysfs idea is not bad in my opinion. The sequences may be extended in UCM, we have already 'exec' command. Yes, this command is a little heavy for the sysfs writes, but we can add command like 'sysset' or so for sysfs like:

Okay, so this would be a sysfs file per card then? Sol we would have for example
2 new sysfs files like this show up when your module is loaded:

/sys/class/sounds/card0/spk_mute_led_control
/sys/class/sounds/card0/mic_mute_led_control

And reading would iterate over all mixer-elements of the card and print
the names of those which have the relevant access LED flag set, where
as a write would be taken as a control-name to add the access LED flag
too?


And an empty write would be special and clear the flag on all controls?
I guess we don't strictly need that if we only set things up at boot once,
but it might still be handy to force things to a clean state.

> 
>   # detach all speaker LED controls for card 1
>   # similar to 'echo -n "card=1,*" > /sysfs/devices/virtual/sound/ctl-led/speaker/detach'
>   sysset "devices/virtual/sound/ctl-led/speaker/detach:card=1,*"
> 
>   # attach the 'Speaker Playback Switch',10 control to speaker LED trigger in card 1
>   # similar to 'echo -n "card=1,iface=MIXER,name='Speaker Playback Switch',index=10" > /sysfs/devices/virtual/sound/ctl-led/speaker/attach
>   sysset "devices/virtual/sound/ctl-led/speaker/attach:card=1,iface=MIXER,name='Speaker Playback Switch',index=10"

I think a sysfs file per card would work better, that would certainly be
a lot more inline with how sysfs is normally used...

Also do we need the iface=MIXER part ?

> Security: The LED-control bindings should be handled only in the boot / init phase (thus in UCM BootSequence section) and the sysfs interface files should be read-only for normal users.

Yes that make sense, but it will require some extra helper to that, I guess it
could be an extra flag to the alsactl restore command which already gets run
at boot, or an extra alsactl command ?

Regards,

Hans