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

[Hans de Goede]

Hi,

On 3/5/21 2:02 PM, Jaroslav Kysela wrote:
> Dne 04. 03. 21 v 20:39 Hans de Goede napsal(a):
>> 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?
> 
> It depends if we want to have this feature as an independent add-on
> (implemented in the generic sound LED module) or if we tie this more to
> the ALSA's core control code.
> 
> My proposal creates virtual sound ctl-led driver - thus
> /sysfs/devices/virtual/sound/ctl-led/ tree. We can add a subdirectory per card
> there like:
> 
> /sysfs/devices/virtual/sound/ctl-led/speaker/card0/attach
> 
> ...

Ok, I see.

>> 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.
> 
> The list operations should be probably identified by separate sysfs files.
> 
> /sysfs/devices/virtual/sound/ctl-led/speaker/card0/attach
> /sysfs/devices/virtual/sound/ctl-led/speaker/card0/detach
> /sysfs/devices/virtual/sound/ctl-led/speaker/card0/reset
> /sysfs/devices/virtual/sound/ctl-led/speaker/card0/controls

Ack, that would be better.

> And /sys/class/sounds/card0/controlC0/led-speaker may be a symlink to
> /sysfs/devices/virtual/sound/ctl-led/speaker/card0/ .
> 
>>>   # 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 ?
> 
> It was just a complete example (element ID specification in a string from
> alsa-lib/amixer). Of course, the other element types won't be probably used
> for the LED functionality...

Ok, the reason I was asking is because if possible we should having to
avoid to do string-parsing inside the kernel. So if the sysfs files
just take a control-name without any of the name=value pair stuff going
on that would be better.

> 
>>> 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 ?
> 
> The alsactl does the BootSequence initialization when UCM is supported in
> alsa-lib. But, we have a little issue that the this sequence is executed only
> if some controls are changed (added or removed) between last alsa state config
> (/var/lib/alsa/asound.state) or if the state for the given card does not exist
> to not override the values which may be changed by the user. It really depends
> on the control API only.
> 
> Apparently, we need another sequence which will be forcefully executed on
> boot. ColdSequence , FixedBootSequence, ForcedSequence, ForcedBootSequence ?

FixedBootSequence seems the best.

This does seem to have quickly grown into something somewhat complex though.

While all that is necessary for the 2 ASoC using devices with actual mute-LEDs
which I'm trying to get supported is 2 very simple static assignments of
the LED flag.  I've just completed some alsa-lib work to deal with the
sometimes quirky control-names and with that in place, the spk-mute-led
flag can be statically set on the access flags of the Speaker + HP output
mute controls as Mark suggested as an alternative.  That seems a lot more KISS
to me.

But I guess the flexibility this gives us will also be helpful for some HDA /
other SOF cases like the Dell privacy stuff which also involves tying a mute
control on an ASoC codec to a led-trigger ?

Regards,

Hans