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
