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

[Jaroslav Kysela <perex@perex.cz>]

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

...

> 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

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

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

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.