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

[Hans de Goede]

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

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

Regards,

Hans