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

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 2/23/21 6:20 PM, Mark Brown wrote:
> On Tue, Feb 23, 2021 at 05:14:32PM +0100, Jaroslav Kysela wrote:
>> Dne 23. 02. 21 v 15:21 Takashi Iwai napsal(a):
> 
>>> That's one of my concerns in the recent actions for putting the
>>> hard-coded mute LED controls.  So far, the only usage of led-audio
>>> trigger is HD-audio, and it's enabled only for selected devices and
>>> setups.  OTOH, if we apply the audio-led trigger generically in ASoC
>>> codec driver, it's always done and might misfit; e.g. what happens if
>>> two codecs are present on the system?.
> 
>> That's the abstraction issue. We can use PCI, ACPI, DMI or DT checks at the
>> _right_ place (machine top-level code) to mark those controls with the LED
>> flags in the kernel space. I've never said that the right place is the generic
>> ASoC codec driver.
> 
> We already need ACPI and DMI quirks in the CODEC drivers anyway due to
> the limitations of ACPI so it wouldn't be particularly surprising to
> have stuff there.  OTOH this would fix things per machine while with
> fancier hardware things might easily be flexible enough that there's
> multiple choices that could be made depending on use case.

I have a feeling from the discussion here that you would prefer this
per model/machine option over the current patch which unconditionally
sets the SNDRV_CTL_ELEM_ACCESS_SPK/MIC_LED flag on the main DAC/ADC
mute controls ?

So I believe that it would be best for me to respin this patch
series moving to making this a per model/machine thing, correct?

> I'd be a lot more comfortable with ASoC having some runtime control for
> overriding which controls get mapped, even if we try to pick defaults
> with quirks.

The drivers in question already allow overriding their quirks bitmap
via a module-parameter.  If we are going to enable the mixer-element
access-flag which enables LED control on a per-model basis based on
DMI quirks, then I plan to simply add 1 new flag in the quirks bitmap
for this for each mixer-element on which we need the flag.

So for now this would be just 2 new flags, since atm we only need
the SNDRV_CTL_ELEM_ACCESS_SPK_LED resp. SNDRV_CTL_ELEM_ACCESS_SPK_LED
flag on one mixer-element each.

And then the user can always override the settings using the quirk
module parameter. This is not exactly runtime control, but IMHO it
is close enough and anything else will just overcomplicate things.
I'm aware of only 3 model 2-in-1s which need this and on those
3 the implementation is very straight forward.

Regards,

Hans