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

From: Takashi Iwai <tiwai@suse.de>
To: Jaroslav Kysela <perex@perex.cz>
Cc: Curtis Malainey <cujomalainey@google.com>,
	Hans de Goede <hdegoede@redhat.com>,
	ALSA development <alsa-devel@alsa-project.org>,
	Mark Brown <broonie@kernel.org>, Dylan Reid <dgreid@google.com>
Subject: Re: [PATCH v4 6/6] ALSA: led control - add sysfs kcontrol LED marking layer
Date: Tue, 23 Mar 2021 11:50:59 +0100	[thread overview]
Message-ID: <s5h35wmcfe4.wl-tiwai@suse.de> (raw)
In-Reply-To: <65943f72-6489-24fa-f6af-a2bae8824d90@perex.cz>

On Tue, 23 Mar 2021 11:31:30 +0100,
Jaroslav Kysela wrote:
> 
> Dne 23. 03. 21 v 10:49 Takashi Iwai napsal(a):
> > On Tue, 23 Mar 2021 10:38:46 +0100,
> > Takashi Iwai wrote:
> >>
> >> On Mon, 22 Mar 2021 15:16:30 +0100,
> >> Jaroslav Kysela wrote:
> >>>
> >>> Dne 20. 03. 21 v 10:48 Takashi Iwai napsal(a):
> >>>
> >>>>> 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.
> >>>>
> >>>> Also ChromeOS.  I'd like to get a general consensus before moving
> >>>> forward.
> >>>
> >>> Where are ChromeOS people? They could join to the discussion which is floating
> >>> few months now. Perhaps, the gmail's spam filter does not allow them to
> >>> communicate with us ;-)
> >>
> >> Also adding Dylan and Mark to Cc.
> >>
> >> FYI, the patch set is:
> >>   https://lore.kernel.org/alsa-devel/20210317172945.842280-1-perex@perex.cz/
> > 
> > ... and now back to the topic.
> > 
> > So the primary question is whether we want the sysfs entries to allow
> > user-space defining the mute-LED vs control binding externally.  With
> > this, the mute LED is supposed to be set up via udev rules that
> > triggers some alsactl stuff, and the rest is handled in an extension
> > in UCM profile.  If this approach is acceptable on all platforms, we
> > can go for it.  That was the question to other platforms like Android
> > and ChromeOS.
> > 
> > 
> > And, now looking into the details, I have a few more questions:
> > 
> > - The binding with SNDRV_CTL_ELEM_* bit flag is handy for some drivers
> >   but not for everything; e.g. if we want to add the binding in ASoC
> >   machine driver, an API like
> >     snd_ctl_bind_mute_led(card, elem_id, inverted);
> >   would be easier.  It'd be essentially an internal call of the sysfs
> >   binding.
> 
> I would probably create more universal helper for the access field. It may be
> handy to update other flags like INACTIVE or so. Something like:
> 
>   snd_ctl_update_access(card, elem_id, access_mask, access_bits);
> 
> If we decide to move this information out of access field, we can replace
> those calls with another function.
> 
> For ASoC codecs, it may be difficult to do such calls in the init phase,
> because the card is not bound to the component. But yes, I agree that this
> setting should be handled in the upper layer (machine) than the component layer.
> 
> >  (I haven't checked, but might this be also more
> >   straightforward conversion for HD-audio case, too?)
> 
> I don't think that it brings a simplification. The id composition is more
> complex than 'if (codec->led_flag) access |= LED_GROUP'.

I guess it'll simply replace the existing call of
snd_hda_add_vmaster_hook() with snd_ctl_update_something().
But it's a minor thing and can be refactored later.

> > - The binding in the kernel could (should?) be shown in the sysfs
> >   output.  Currently it seems handled differently?
> 
> It isn't. The LED group is stored in the access field and my implementation
> tracks those bits per elements. So, the sysfs LED code updates those bits,
> too. The settings is preserved even if you reload the ctl-led module.
> 
> > - Specifying the numid may the code simpler in kernel side?
> >   alsactl has already the string parser.
> 
> Yes, but it's not so handy for scripting / UCM. I can add find-ctl-numid
> lookup to UCM, of course, but what about standard shell scripting?

Hmm, would UCM itself touch the sysfs entry?  That sounds a bit awful.

The simpler implementation in the kernel side is always nicer, but of
course only if it works sufficiently.  So it depends on how much we
want to support this feature.  The parse of control name can be done
by scripting, but it's cumbersome for now, indeed, so if the shell
scripting is seen as the major usage, it'd be more convenient if the
kernel parses the string, yeah.

> > - Do we have to deal with binding with multiple controls to a single
> >   mute LED?  Might a single exclusive binding make things easier?
> >   Then we don't have to create sysfs entries per card, and it'll be
> >   something like
> >      echo 1:10 > /sys/devices/virtual/sound/ctl-led/mic/bind
> >   which is equivalent with the API call above.
> >   If multiple bindings are attempted, it can simply give an error.
> >   In the driver side, it catches the unexpected binding, too.
> 
> AMD ACP digital + HDA analog headset microphone. If we follow the standard HDA
> behaviour, both inputs should trigger the mic LED. Two cards are in the game.

And that brings yet another question.  If the Dell privacy thing comes
to play here, for example, the mute LED is tied with the hardware
control of the built-in mic.  Then do we influence on this depending
on the headset mic mute state, too?

thanks,

Takashi