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

[Jaroslav Kysela]

Dne 24. 02. 21 v 11:33 Takashi Iwai napsal(a):
> On Wed, 24 Feb 2021 10:49:41 +0100,
> Jaroslav Kysela wrote:
>>
>> Dne 24. 02. 21 v 10:38 Takashi Iwai napsal(a):
>>
>>>> It seems that you misunderstood the number of issues which my code is trying
>>>> to resolve:
>>>>
>>>> 1) set LED based on state from multiple cards (so you cannot trigger LED
>>>> inside single driver / single control element); we need one arbiter; this is
>>>> the main argument
>>>> 2) unifies the audio LED interface
>>>> 3) reduce the hardware driver code
>>>
>>> Those purposes are all fine.  But they don't need to be exposed for
>>> user controls that can be abused.  That's the very concern.
>>
>> So, how to handle this feature for AMD ACP without PA / PipeWire modifications?
>>
>> And if we add an user space channel to the audio LED arbiter code, how it
>> differs from my proposed control API extension?
> 
> As the early patch does, creating a kernel control (but not a generic
> "Capture" switch but specific to LED) and control it in UCM profile.
> What's the practical problem there?  That's what I might have missed
> as the starting point of the discussion.

UCM is just a database which does not do any state management for those
controls. I've not found a simple way to create an arbiter for UCM without
adding more layers to this API. Yes, we have enable/disable sequences, but for
LED, you need to create "group" of devices and do the OR state management:

Current device enable/disable scheme:

  Device1 -> enable (LED off)
  Device2 -> enable (LED off)
  Device2 -> disable (LED on) --- but Device1 is on, so LED should be off

  ... LED off - set LED control to off
  ... LED on - set LED control to on

Even the current mechanism fails here, we don't look into the mute switch
value in UCM at the moment, so the LED will reflect only device use - not the
mute switch. So, as you see, UCM does not cover this. It's just used to
activate and deactivate paths, but there's no state management (except for the
device on/off).

And, it will work only for UCM not for the standard/legacy ALSA setup.

Those are reasons for which I ruled out the UCM for the LED control.

>> We have already locking
>> mechanism for the user control element to one task, so it's possible to create
>> safe user space implementation (depending on the standard task priviledges) on
>> demand even with my proposal.
>>
>> Could you elaborate the abuse you mean? Three bits?
> 
> You can create up to 1028 user controls per card and each of them can
> fire the led trigger...  That's an interesting experiment :)
> 
> So far, a user control is merely storing the value, let read/write via
> the control API.  That's all, and nothing wrong can happen just by
> that.  Now if it interacts with other subsystem...
> 
> A more serious concern is rather the fragility of the setup; for
> enabling the mute LED control, you'd have to create a new user-space
> control, the function of the control has to be ignored by some
> application and some not, etc.  This has to be done on each machine

You're using "ignore", but as I explained before, the user space switch will
be used in the whole chain:

capture stream ->
  alsa-lib mute switch / silence PCM stream ->
  PA mute switch / silence PCM stream

So PA can use this switch like the traditional hardware mute switch.

And we cannot do much in the user space for a better mute support here.

> when the system got updated.  And, not everyone is using alsa-lib.
> Does tiny ALSA and other existing backend support the user control
> element management?  Some uncertainty here.

It's not an argument. Tiny alsa library does not have all features from
alsa-lib. Nobody is restricted to follow the similar mechanism.

					Jaroslav

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