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

From: Takashi Iwai <tiwai@suse.de>
To: Hans de Goede <hdegoede@redhat.com>
Cc: Curtis Malainey <cujomalainey@google.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 12:03:39 +0100	[thread overview]
Message-ID: <s5h1rc6cet0.wl-tiwai@suse.de> (raw)
In-Reply-To: <a437a531-61ae-c064-94a3-938195577c7d@redhat.com>

On Tue, 23 Mar 2021 11:42:26 +0100,
Hans de Goede wrote:
> 
> Hi,
> 
> On 3/23/21 11:31 AM, 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);
> 
> For the ASoC machine drivers this functions would ideally take an element-name
> not the numeric id, because the machine-driver has no idea of the ids and
> the ids are not really stable (they may change when e.g. a new mixer
> element is added to the codec).

In the kernel side, what we need is rather a simple helper function
like snd_ctl_find_elem(card, iface, name, index) that returns kcontrol
object.  A similar code has been already implemented everywhere, so
it'd make sense to have a common helper instead.

> > 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'.
> > 
> >> - 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?
> 
> I would prefer for the sysfs API to accept element-names too, as
> I mentioned above that would even be better for in kernel use, let
> alone for a userspace API.

In the kernel side API, we don't need the string parser for the
(iface, name, index) tuple.  So, the only question is whether the
string parsing is the mandatory for the sysfs interface.

I'm not entirely objecting to it; such a parser could be used in other
places generically, too.  But, looking at the current "list" output,
it shows also only numid, so from the symmetry POV, using the numid
for binding would make sense, too.

thanks,

Takashi