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

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

[Hans de Goede]

Hi All,

This series applies on top of the:
"[PATCH 0/4] ASoC: rt5670: Various kcontrol fixes"
series which I just send out;

And it also requires the:
"[PATCH 0/5] ALSA: control - add generic LED trigger code"
from Jaroslav. This second requirement is still being discussed /
reviewed, which is why I split out these 2 patches into a separate
RFC patch series.

Regards,

Hans


Hans de Goede (2):
  ASoC: Add new SOC_DOUBLE*_ACCESS() macros
  ASoC: rt5670: Add LED trigger support

 include/sound/soc.h       | 20 ++++++++++++++++----
 sound/soc/codecs/rt5670.c | 10 +++++++---
 2 files changed, 23 insertions(+), 7 deletions(-)

-- 
2.30.1

[RFC 1/2] ASoC: Add new SOC_DOUBLE*_ACCESS() macros

[Hans de Goede]

Add 3 new SOC_DOUBLE*_ACCESS() macros:

SOC_DOUBLE_ACCESS()
SOC_DOUBLE_EXT_ACCESS()
SOC_DOUBLE_TLV_ACCESS()

These work the same as their non _ACCESS postfixed counter-parts,
with an extra argument that allows specifying extra access flags.

This is meant to be used with the new SNDRV_CTL_ELEM_ACCESS_*_LED flags.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 include/sound/soc.h | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index 3fa6c40a63b7..0136650466f3 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -100,12 +100,15 @@
 		{.reg = xreg, .rreg = xreg, .shift = xshift, \
 		 .rshift = xshift, .min = xmin, .max = xmax, \
 		 .platform_max = xmax, .invert = xinvert} }
-#define SOC_DOUBLE(xname, reg, shift_left, shift_right, max, invert) \
+#define SOC_DOUBLE_ACCESS(xname, reg, shift_left, shift_right, max, invert, elem_access) \
 {	.iface = SNDRV_CTL_ELEM_IFACE_MIXER, .name = (xname),\
+	.access = SNDRV_CTL_ELEM_ACCESS_READWRITE | (elem_access),\
 	.info = snd_soc_info_volsw, .get = snd_soc_get_volsw, \
 	.put = snd_soc_put_volsw, \
 	.private_value = SOC_DOUBLE_VALUE(reg, shift_left, shift_right, \
 					  max, invert, 0) }
+#define SOC_DOUBLE(xname, reg, shift_left, shift_right, max, invert) \
+	SOC_DOUBLE_ACCESS(xname, reg, shift_left, shift_right, max, invert, 0)
 #define SOC_DOUBLE_STS(xname, reg, shift_left, shift_right, max, invert) \
 {									\
 	.iface = SNDRV_CTL_ELEM_IFACE_MIXER, .name = (xname),		\
@@ -127,15 +130,19 @@
 	.get = snd_soc_get_volsw_range, .put = snd_soc_put_volsw_range, \
 	.private_value = SOC_DOUBLE_R_RANGE_VALUE(reg_left, reg_right, \
 					    xshift, xmin, xmax, xinvert) }
-#define SOC_DOUBLE_TLV(xname, reg, shift_left, shift_right, max, invert, tlv_array) \
+#define SOC_DOUBLE_TLV_ACCESS(xname, reg, shift_left, shift_right, max, invert, \
+			      tlv_array, elem_access) \
 {	.iface = SNDRV_CTL_ELEM_IFACE_MIXER, .name = (xname),\
 	.access = SNDRV_CTL_ELEM_ACCESS_TLV_READ |\
-		 SNDRV_CTL_ELEM_ACCESS_READWRITE,\
+		 SNDRV_CTL_ELEM_ACCESS_READWRITE |\
+		 (elem_access),\
 	.tlv.p = (tlv_array), \
 	.info = snd_soc_info_volsw, .get = snd_soc_get_volsw, \
 	.put = snd_soc_put_volsw, \
 	.private_value = SOC_DOUBLE_VALUE(reg, shift_left, shift_right, \
 					  max, invert, 0) }
+#define SOC_DOUBLE_TLV(xname, reg, shift_left, shift_right, max, invert, tlv_array) \
+	SOC_DOUBLE_TLV_ACCESS(xname, reg, shift_left, shift_right, max, invert, tlv_array, 0)
 #define SOC_DOUBLE_R_TLV(xname, reg_left, reg_right, xshift, xmax, xinvert, tlv_array) \
 {	.iface = SNDRV_CTL_ELEM_IFACE_MIXER, .name = (xname),\
 	.access = SNDRV_CTL_ELEM_ACCESS_TLV_READ |\
@@ -225,13 +232,18 @@
 	.info = snd_soc_info_volsw, \
 	.get = xhandler_get, .put = xhandler_put, \
 	.private_value = SOC_SINGLE_VALUE(xreg, xshift, xmax, xinvert, 0) }
-#define SOC_DOUBLE_EXT(xname, reg, shift_left, shift_right, max, invert,\
+#define SOC_DOUBLE_EXT_ACCESS(xname, reg, shift_left, shift_right, max, invert, elem_access,\
 	 xhandler_get, xhandler_put) \
 {	.iface = SNDRV_CTL_ELEM_IFACE_MIXER, .name = (xname),\
+	.access = SNDRV_CTL_ELEM_ACCESS_READWRITE | (elem_access),\
 	.info = snd_soc_info_volsw, \
 	.get = xhandler_get, .put = xhandler_put, \
 	.private_value = \
 		SOC_DOUBLE_VALUE(reg, shift_left, shift_right, max, invert, 0) }
+#define SOC_DOUBLE_EXT(xname, reg, shift_left, shift_right, max, invert,\
+		       xhandler_get, xhandler_put) \
+	SOC_DOUBLE_EXT_ACCESS(xname, reg, shift_left, shift_right, max, invert, 0,\
+			      xhandler_get, xhandler_put)
 #define SOC_DOUBLE_R_EXT(xname, reg_left, reg_right, xshift, xmax, xinvert,\
 	 xhandler_get, xhandler_put) \
 {	.iface = SNDRV_CTL_ELEM_IFACE_MIXER, .name = (xname), \
-- 
2.30.1

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

[Hans de Goede]

Add support for controlling a speaker and/or microphone mute LED through
LED triggers using the new generic LED trigger module.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 sound/soc/codecs/rt5670.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/sound/soc/codecs/rt5670.c b/sound/soc/codecs/rt5670.c
index feab15d0686a..9233710e3a4f 100644
--- a/sound/soc/codecs/rt5670.c
+++ b/sound/soc/codecs/rt5670.c
@@ -690,7 +690,8 @@ static const struct snd_kcontrol_new rt5670_snd_controls[] = {
 	/* DAC Digital Volume */
 	SOC_DOUBLE("DAC2 Playback Switch", RT5670_DAC_CTRL,
 		RT5670_M_DAC_L2_VOL_SFT, RT5670_M_DAC_R2_VOL_SFT, 1, 1),
-	SOC_DOUBLE_EXT("DAC1 Playback Switch", SND_SOC_NOPM, 0, 1, 1, 0,
+	SOC_DOUBLE_EXT_ACCESS("DAC1 Playback Switch", SND_SOC_NOPM, 0, 1, 1, 0,
+			SNDRV_CTL_ELEM_ACCESS_SPK_LED,
 			rt5670_dac1_playback_switch_get, rt5670_dac1_playback_switch_put),
 	SOC_DOUBLE_TLV("DAC1 Playback Volume", RT5670_DAC1_DIG_VOL,
 			RT5670_L_VOL_SFT, RT5670_R_VOL_SFT,
@@ -708,8 +709,9 @@ static const struct snd_kcontrol_new rt5670_snd_controls[] = {
 			RT5670_INL_VOL_SFT, RT5670_INR_VOL_SFT,
 			31, 1, in_vol_tlv),
 	/* ADC Digital Volume Control */
-	SOC_DOUBLE("ADC Capture Switch", RT5670_STO1_ADC_DIG_VOL,
-		RT5670_L_MUTE_SFT, RT5670_R_MUTE_SFT, 1, 1),
+	SOC_DOUBLE_ACCESS("ADC Capture Switch", RT5670_STO1_ADC_DIG_VOL,
+			  RT5670_L_MUTE_SFT, RT5670_R_MUTE_SFT, 1, 1,
+			  SNDRV_CTL_ELEM_ACCESS_MIC_LED),
 	SOC_DOUBLE_TLV("ADC Capture Volume", RT5670_STO1_ADC_DIG_VOL,
 			RT5670_L_VOL_SFT, RT5670_R_VOL_SFT,
 			127, 0, adc_vol_tlv),
@@ -3253,6 +3255,8 @@ static int rt5670_i2c_probe(struct i2c_client *i2c,
 
 	}
 
+	snd_ctl_led_request();
+
 	pm_runtime_enable(&i2c->dev);
 	pm_request_idle(&i2c->dev);
 
-- 
2.30.1

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 827 bytes --]

On Mon, Feb 15, 2021 at 03:24:19PM +0100, Hans de Goede wrote:

> Add support for controlling a speaker and/or microphone mute LED through
> LED triggers using the new generic LED trigger module.

> -	SOC_DOUBLE_EXT("DAC1 Playback Switch", SND_SOC_NOPM, 0, 1, 1, 0,
> +	SOC_DOUBLE_EXT_ACCESS("DAC1 Playback Switch", SND_SOC_NOPM, 0, 1, 1, 0,
> +			SNDRV_CTL_ELEM_ACCESS_SPK_LED,
>  			rt5670_dac1_playback_switch_get, rt5670_dac1_playback_switch_put),

> -	SOC_DOUBLE("ADC Capture Switch", RT5670_STO1_ADC_DIG_VOL,
> -		RT5670_L_MUTE_SFT, RT5670_R_MUTE_SFT, 1, 1),
> +	SOC_DOUBLE_ACCESS("ADC Capture Switch", RT5670_STO1_ADC_DIG_VOL,
> +			  RT5670_L_MUTE_SFT, RT5670_R_MUTE_SFT, 1, 1,
> +			  SNDRV_CTL_ELEM_ACCESS_MIC_LED),

Why just these particular controls - what if a system has separate mutes
for speakers or something?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Hans de Goede]

Hi,

On 2/23/21 2:45 PM, Mark Brown wrote:
> On Mon, Feb 15, 2021 at 03:24:19PM +0100, Hans de Goede wrote:
> 
>> Add support for controlling a speaker and/or microphone mute LED through
>> LED triggers using the new generic LED trigger module.
> 
>> -	SOC_DOUBLE_EXT("DAC1 Playback Switch", SND_SOC_NOPM, 0, 1, 1, 0,
>> +	SOC_DOUBLE_EXT_ACCESS("DAC1 Playback Switch", SND_SOC_NOPM, 0, 1, 1, 0,
>> +			SNDRV_CTL_ELEM_ACCESS_SPK_LED,
>>  			rt5670_dac1_playback_switch_get, rt5670_dac1_playback_switch_put),
> 
>> -	SOC_DOUBLE("ADC Capture Switch", RT5670_STO1_ADC_DIG_VOL,
>> -		RT5670_L_MUTE_SFT, RT5670_R_MUTE_SFT, 1, 1),
>> +	SOC_DOUBLE_ACCESS("ADC Capture Switch", RT5670_STO1_ADC_DIG_VOL,
>> +			  RT5670_L_MUTE_SFT, RT5670_R_MUTE_SFT, 1, 1,
>> +			  SNDRV_CTL_ELEM_ACCESS_MIC_LED),
> 
> Why just these particular controls - what if a system has separate mutes
> for speakers or something?

These are the main volume controls, which are always in the output / input
path independent on if we are outputting to e.g. speakers or the headphones.

We want to use the main volume control for this, because there always is
only 1 output mute LED and 1 input mute LED. Well at least that is the assumption
the current ledtrig-audio.c code has.

The idea is to only turn the single LED on if we are sure there will be not
sound output on any of the outputs, which is why we tie the LED to the
mute switch on the main volume control.

Regards,

Hans

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1408 bytes --]

On Tue, Feb 23, 2021 at 02:59:17PM +0100, Hans de Goede wrote:
> On 2/23/21 2:45 PM, Mark Brown wrote:
> > On Mon, Feb 15, 2021 at 03:24:19PM +0100, Hans de Goede wrote:

> > Why just these particular controls - what if a system has separate mutes
> > for speakers or something?

> These are the main volume controls, which are always in the output / input
> path independent on if we are outputting to e.g. speakers or the headphones.

> We want to use the main volume control for this, because there always is
> only 1 output mute LED and 1 input mute LED. Well at least that is the assumption
> the current ledtrig-audio.c code has.

> The idea is to only turn the single LED on if we are sure there will be not
> sound output on any of the outputs, which is why we tie the LED to the
> mute switch on the main volume control.

Right, so that might work well on your particular system with your
particular configuration but will it work well on other systems with
different hardware?  It's not clear to me that it makes sense to go
through all the drivers picking controls that might be used for this
purpose - it seems both time consuming and error prone.  Consider a
mostly digital device which has an ADC/DAC per input/output rather than
a central ADC/DAC with analogue muxing for example, or a system with
multiple DACs available for mixing together or analogue bypassess.  

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Takashi Iwai]

On Tue, 23 Feb 2021 15:09:30 +0100,
Mark Brown wrote:
> 
> On Tue, Feb 23, 2021 at 02:59:17PM +0100, Hans de Goede wrote:
> > On 2/23/21 2:45 PM, Mark Brown wrote:
> > > On Mon, Feb 15, 2021 at 03:24:19PM +0100, Hans de Goede wrote:
> 
> > > Why just these particular controls - what if a system has separate mutes
> > > for speakers or something?
> 
> > These are the main volume controls, which are always in the output / input
> > path independent on if we are outputting to e.g. speakers or the headphones.
> 
> > We want to use the main volume control for this, because there always is
> > only 1 output mute LED and 1 input mute LED. Well at least that is the assumption
> > the current ledtrig-audio.c code has.
> 
> > The idea is to only turn the single LED on if we are sure there will be not
> > sound output on any of the outputs, which is why we tie the LED to the
> > mute switch on the main volume control.
> 
> Right, so that might work well on your particular system with your
> particular configuration but will it work well on other systems with
> different hardware?  It's not clear to me that it makes sense to go
> through all the drivers picking controls that might be used for this
> purpose - it seems both time consuming and error prone.  Consider a
> mostly digital device which has an ADC/DAC per input/output rather than
> a central ADC/DAC with analogue muxing for example, or a system with
> multiple DACs available for mixing together or analogue bypassess.  

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

Of course, this implementation would make the integration much easier,
and that's a big benefit.  So I have a mixed feeling and not decided
yet whether we should go for it right now...


thanks,

Takashi

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

[Jaroslav Kysela]

Dne 23. 02. 21 v 15:21 Takashi Iwai napsal(a):
> On Tue, 23 Feb 2021 15:09:30 +0100,
> Mark Brown wrote:
>>
>> On Tue, Feb 23, 2021 at 02:59:17PM +0100, Hans de Goede wrote:
>>> On 2/23/21 2:45 PM, Mark Brown wrote:
>>>> On Mon, Feb 15, 2021 at 03:24:19PM +0100, Hans de Goede wrote:
>>
>>>> Why just these particular controls - what if a system has separate mutes
>>>> for speakers or something?
>>
>>> These are the main volume controls, which are always in the output / input
>>> path independent on if we are outputting to e.g. speakers or the headphones.
>>
>>> We want to use the main volume control for this, because there always is
>>> only 1 output mute LED and 1 input mute LED. Well at least that is the assumption
>>> the current ledtrig-audio.c code has.
>>
>>> The idea is to only turn the single LED on if we are sure there will be not
>>> sound output on any of the outputs, which is why we tie the LED to the
>>> mute switch on the main volume control.
>>
>> Right, so that might work well on your particular system with your
>> particular configuration but will it work well on other systems with
>> different hardware?  It's not clear to me that it makes sense to go
>> through all the drivers picking controls that might be used for this
>> purpose - it seems both time consuming and error prone.  Consider a
>> mostly digital device which has an ADC/DAC per input/output rather than
>> a central ADC/DAC with analogue muxing for example, or a system with
>> multiple DACs available for mixing together or analogue bypassess.  
> 
> 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.

On the other hand, rt5670 is really multi-purpose codec like HDA codecs with
the recommended usage/routing from the designer, so it might make sense to add
the default LED marking as we do for HDA.

> Of course, this implementation would make the integration much easier,
> and that's a big benefit.  So I have a mixed feeling and not decided
> yet whether we should go for it right now...

I think that we can reconsider the LED handling implementation later, when
someone brings something better on the table.

						Jaroslav

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

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

[Takashi Iwai]

On Tue, 23 Feb 2021 17:14:32 +0100,
Jaroslav Kysela wrote:
> 
> Dne 23. 02. 21 v 15:21 Takashi Iwai napsal(a):
> > On Tue, 23 Feb 2021 15:09:30 +0100,
> > Mark Brown wrote:
> >>
> >> On Tue, Feb 23, 2021 at 02:59:17PM +0100, Hans de Goede wrote:
> >>> On 2/23/21 2:45 PM, Mark Brown wrote:
> >>>> On Mon, Feb 15, 2021 at 03:24:19PM +0100, Hans de Goede wrote:
> >>
> >>>> Why just these particular controls - what if a system has separate mutes
> >>>> for speakers or something?
> >>
> >>> These are the main volume controls, which are always in the output / input
> >>> path independent on if we are outputting to e.g. speakers or the headphones.
> >>
> >>> We want to use the main volume control for this, because there always is
> >>> only 1 output mute LED and 1 input mute LED. Well at least that is the assumption
> >>> the current ledtrig-audio.c code has.
> >>
> >>> The idea is to only turn the single LED on if we are sure there will be not
> >>> sound output on any of the outputs, which is why we tie the LED to the
> >>> mute switch on the main volume control.
> >>
> >> Right, so that might work well on your particular system with your
> >> particular configuration but will it work well on other systems with
> >> different hardware?  It's not clear to me that it makes sense to go
> >> through all the drivers picking controls that might be used for this
> >> purpose - it seems both time consuming and error prone.  Consider a
> >> mostly digital device which has an ADC/DAC per input/output rather than
> >> a central ADC/DAC with analogue muxing for example, or a system with
> >> multiple DACs available for mixing together or analogue bypassess.  
> > 
> > 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.
> 
> On the other hand, rt5670 is really multi-purpose codec like HDA codecs with
> the recommended usage/routing from the designer, so it might make sense to add
> the default LED marking as we do for HDA.

So do you mean that the LED feature should be selectively enabled like
the current HD-audio?

> > Of course, this implementation would make the integration much easier,
> > and that's a big benefit.  So I have a mixed feeling and not decided
> > yet whether we should go for it right now...
> 
> I think that we can reconsider the LED handling implementation later, when
> someone brings something better on the table.

What worried me is the plan to expose this capability to user-space.
If it's only a kernel-internal, we can fix it in the kernel and
nothing else broken, but if it's a part of API, that's not easy.

So, if any, I'd like to avoid exposing to the user-space at first.
(But then it comes to the question how to deal with a case like AMD
ACP...)


thanks,

Takashi

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

[Jaroslav Kysela]

Dne 23. 02. 21 v 17:20 Takashi Iwai napsal(a):

> So do you mean that the LED feature should be selectively enabled like
> the current HD-audio?

Yes, it should be enabled only when the machine has the audio LEDs.

>>> Of course, this implementation would make the integration much easier,
>>> and that's a big benefit.  So I have a mixed feeling and not decided
>>> yet whether we should go for it right now...
>>
>> I think that we can reconsider the LED handling implementation later, when
>> someone brings something better on the table.
> 
> What worried me is the plan to expose this capability to user-space.
> If it's only a kernel-internal, we can fix it in the kernel and
> nothing else broken, but if it's a part of API, that's not easy.
> 
> So, if any, I'd like to avoid exposing to the user-space at first.
> (But then it comes to the question how to deal with a case like AMD
> ACP...)

I tried to propose a complete solution and the ACP was one strong reason for
this kernel / user space API. So without the user space support, it's just
a half solution for known issues.

Frankly, I don't see any drawback or a problem even if we remove this API
later. The LED group bits are just informal for the user space and it's
expected to create the user controls tied to this LED functionality only in
alsa-lib/plugins at the moment. The kernel may return an error when the user
space tries to set those new bits when the API is deprecated and I believe
that the hardware design faults like AMD ACP (without the hardware mute) are rare.

Initial alsa-lib support: https://github.com/alsa-project/alsa-lib/pull/121

						Jaroslav

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

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

[Takashi Iwai]

On Tue, 23 Feb 2021 21:56:16 +0100,
Jaroslav Kysela wrote:
> 
> Dne 23. 02. 21 v 17:20 Takashi Iwai napsal(a):
> >>> Of course, this implementation would make the integration much easier,
> >>> and that's a big benefit.  So I have a mixed feeling and not decided
> >>> yet whether we should go for it right now...
> >>
> >> I think that we can reconsider the LED handling implementation later, when
> >> someone brings something better on the table.
> > 
> > What worried me is the plan to expose this capability to user-space.
> > If it's only a kernel-internal, we can fix it in the kernel and
> > nothing else broken, but if it's a part of API, that's not easy.
> > 
> > So, if any, I'd like to avoid exposing to the user-space at first.
> > (But then it comes to the question how to deal with a case like AMD
> > ACP...)
> 
> I tried to propose a complete solution and the ACP was one strong reason for
> this kernel / user space API. So without the user space support, it's just
> a half solution for known issues.
> 
> Frankly, I don't see any drawback or a problem even if we remove this API
> later.

Removing the user-space API is absolutely no-go.  The only exception
would be either the case really no one uses it or it's too buggy and
unfixable.

> The LED group bits are just informal for the user space and it's
> expected to create the user controls tied to this LED functionality only in
> alsa-lib/plugins at the moment. The kernel may return an error when the user
> space tries to set those new bits when the API is deprecated and I believe
> that the hardware design faults like AMD ACP (without the hardware mute) are rare.

The experience tells us that users are creative enough to (ab)use a
new ABI in any unexpected ways, and we have no control for it.  So
it's not about how alsa-lib is implemented but rather how ABI could be
abused :)


Takashi

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

[Jaroslav Kysela]

Dne 24. 02. 21 v 8:12 Takashi Iwai napsal(a):
> On Tue, 23 Feb 2021 21:56:16 +0100,
> Jaroslav Kysela wrote:
>>
>> Dne 23. 02. 21 v 17:20 Takashi Iwai napsal(a):
>>>>> Of course, this implementation would make the integration much easier,
>>>>> and that's a big benefit.  So I have a mixed feeling and not decided
>>>>> yet whether we should go for it right now...
>>>>
>>>> I think that we can reconsider the LED handling implementation later, when
>>>> someone brings something better on the table.
>>>
>>> What worried me is the plan to expose this capability to user-space.
>>> If it's only a kernel-internal, we can fix it in the kernel and
>>> nothing else broken, but if it's a part of API, that's not easy.
>>>
>>> So, if any, I'd like to avoid exposing to the user-space at first.
>>> (But then it comes to the question how to deal with a case like AMD
>>> ACP...)
>>
>> I tried to propose a complete solution and the ACP was one strong reason for
>> this kernel / user space API. So without the user space support, it's just
>> a half solution for known issues.
>>
>> Frankly, I don't see any drawback or a problem even if we remove this API
>> later.
> 
> Removing the user-space API is absolutely no-go.  The only exception
> would be either the case really no one uses it or it's too buggy and
> unfixable.

This is a special case. Even if those LED bits are ignored by kernel in
future, we expect to be replaced with another layer. Thus the functionality
must be retained.

>> The LED group bits are just informal for the user space and it's
>> expected to create the user controls tied to this LED functionality only in
>> alsa-lib/plugins at the moment. The kernel may return an error when the user
>> space tries to set those new bits when the API is deprecated and I believe
>> that the hardware design faults like AMD ACP (without the hardware mute) are rare.
> 
> The experience tells us that users are creative enough to (ab)use a
> new ABI in any unexpected ways, and we have no control for it.  So
> it's not about how alsa-lib is implemented but rather how ABI could be
> abused :)

Ok, I don't have other ideas. I don't agree with your argumentation for this
particular case, where the functionality is marginal. Ideally, the AMD driver
may be recoded to use double-buffering and software mute switch, so we should
handle everything in the kernel space.

					Jaroslav

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

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

[Takashi Iwai]

On Wed, 24 Feb 2021 09:14:41 +0100,
Jaroslav Kysela wrote:
> 
> Dne 24. 02. 21 v 8:12 Takashi Iwai napsal(a):
> > On Tue, 23 Feb 2021 21:56:16 +0100,
> > Jaroslav Kysela wrote:
> >>
> >> Dne 23. 02. 21 v 17:20 Takashi Iwai napsal(a):
> >>>>> Of course, this implementation would make the integration much easier,
> >>>>> and that's a big benefit.  So I have a mixed feeling and not decided
> >>>>> yet whether we should go for it right now...
> >>>>
> >>>> I think that we can reconsider the LED handling implementation later, when
> >>>> someone brings something better on the table.
> >>>
> >>> What worried me is the plan to expose this capability to user-space.
> >>> If it's only a kernel-internal, we can fix it in the kernel and
> >>> nothing else broken, but if it's a part of API, that's not easy.
> >>>
> >>> So, if any, I'd like to avoid exposing to the user-space at first.
> >>> (But then it comes to the question how to deal with a case like AMD
> >>> ACP...)
> >>
> >> I tried to propose a complete solution and the ACP was one strong reason for
> >> this kernel / user space API. So without the user space support, it's just
> >> a half solution for known issues.
> >>
> >> Frankly, I don't see any drawback or a problem even if we remove this API
> >> later.
> > 
> > Removing the user-space API is absolutely no-go.  The only exception
> > would be either the case really no one uses it or it's too buggy and
> > unfixable.
> 
> This is a special case. Even if those LED bits are ignored by kernel in
> future, we expect to be replaced with another layer. Thus the functionality
> must be retained.

Well, we cannot know whether the replacement really happens or
happened, and hence we never kill the old one.  That's the problem.

> >> The LED group bits are just informal for the user space and it's
> >> expected to create the user controls tied to this LED functionality only in
> >> alsa-lib/plugins at the moment. The kernel may return an error when the user
> >> space tries to set those new bits when the API is deprecated and I believe
> >> that the hardware design faults like AMD ACP (without the hardware mute) are rare.
> > 
> > The experience tells us that users are creative enough to (ab)use a
> > new ABI in any unexpected ways, and we have no control for it.  So
> > it's not about how alsa-lib is implemented but rather how ABI could be
> > abused :)
> 
> Ok, I don't have other ideas. I don't agree with your argumentation for this
> particular case, where the functionality is marginal. Ideally, the AMD driver
> may be recoded to use double-buffering and software mute switch, so we should
> handle everything in the kernel space.

My argument is that we're trying to add too much freedom just for this
"marginal" problem.  Honestly speaking, I would feel rather more
comfortable if it were a kernel control element that just does trigger
the LED like the original patch from AMD guys.  Then you cannot do
much wrong.  OTOH, creating a virtual capture switch and let alsa-lib
handling the software mute, while PA should ignores the soft-mute but
dealing only with the assigned mute LED...  Sounds too complex to me.


thanks,

Takashi

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

[Jaroslav Kysela]

Dne 24. 02. 21 v 9:52 Takashi Iwai napsal(a):
> On Wed, 24 Feb 2021 09:14:41 +0100,
> Jaroslav Kysela wrote:
>>
>> Dne 24. 02. 21 v 8:12 Takashi Iwai napsal(a):
>>> On Tue, 23 Feb 2021 21:56:16 +0100,
>>> Jaroslav Kysela wrote:
>>>>
>>>> Dne 23. 02. 21 v 17:20 Takashi Iwai napsal(a):
>>>>>>> Of course, this implementation would make the integration much easier,
>>>>>>> and that's a big benefit.  So I have a mixed feeling and not decided
>>>>>>> yet whether we should go for it right now...
>>>>>>
>>>>>> I think that we can reconsider the LED handling implementation later, when
>>>>>> someone brings something better on the table.
>>>>>
>>>>> What worried me is the plan to expose this capability to user-space.
>>>>> If it's only a kernel-internal, we can fix it in the kernel and
>>>>> nothing else broken, but if it's a part of API, that's not easy.
>>>>>
>>>>> So, if any, I'd like to avoid exposing to the user-space at first.
>>>>> (But then it comes to the question how to deal with a case like AMD
>>>>> ACP...)
>>>>
>>>> I tried to propose a complete solution and the ACP was one strong reason for
>>>> this kernel / user space API. So without the user space support, it's just
>>>> a half solution for known issues.
>>>>
>>>> Frankly, I don't see any drawback or a problem even if we remove this API
>>>> later.
>>>
>>> Removing the user-space API is absolutely no-go.  The only exception
>>> would be either the case really no one uses it or it's too buggy and
>>> unfixable.
>>
>> This is a special case. Even if those LED bits are ignored by kernel in
>> future, we expect to be replaced with another layer. Thus the functionality
>> must be retained.
> 
> Well, we cannot know whether the replacement really happens or
> happened, and hence we never kill the old one.  That's the problem.
> 
>>>> The LED group bits are just informal for the user space and it's
>>>> expected to create the user controls tied to this LED functionality only in
>>>> alsa-lib/plugins at the moment. The kernel may return an error when the user
>>>> space tries to set those new bits when the API is deprecated and I believe
>>>> that the hardware design faults like AMD ACP (without the hardware mute) are rare.
>>>
>>> The experience tells us that users are creative enough to (ab)use a
>>> new ABI in any unexpected ways, and we have no control for it.  So
>>> it's not about how alsa-lib is implemented but rather how ABI could be
>>> abused :)
>>
>> Ok, I don't have other ideas. I don't agree with your argumentation for this
>> particular case, where the functionality is marginal. Ideally, the AMD driver
>> may be recoded to use double-buffering and software mute switch, so we should
>> handle everything in the kernel space.
> 
> My argument is that we're trying to add too much freedom just for this
> "marginal" problem.  Honestly speaking, I would feel rather more
> comfortable if it were a kernel control element that just does trigger
> the LED like the original patch from AMD guys.  Then you cannot do
> much wrong.  OTOH, creating a virtual capture switch and let alsa-lib
> handling the software mute, while PA should ignores the soft-mute but

We can force the softvol even if PA set the skip flag for this particular PCM
stream.

> dealing only with the assigned mute LED...  Sounds too complex to me.

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

					Jaroslav

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

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

[Takashi Iwai]

On Wed, 24 Feb 2021 10:27:13 +0100,
Jaroslav Kysela wrote:
> 
> Dne 24. 02. 21 v 9:52 Takashi Iwai napsal(a):
> > On Wed, 24 Feb 2021 09:14:41 +0100,
> > Jaroslav Kysela wrote:
> >>
> >> Dne 24. 02. 21 v 8:12 Takashi Iwai napsal(a):
> >>> On Tue, 23 Feb 2021 21:56:16 +0100,
> >>> Jaroslav Kysela wrote:
> >>>>
> >>>> Dne 23. 02. 21 v 17:20 Takashi Iwai napsal(a):
> >>>>>>> Of course, this implementation would make the integration much easier,
> >>>>>>> and that's a big benefit.  So I have a mixed feeling and not decided
> >>>>>>> yet whether we should go for it right now...
> >>>>>>
> >>>>>> I think that we can reconsider the LED handling implementation later, when
> >>>>>> someone brings something better on the table.
> >>>>>
> >>>>> What worried me is the plan to expose this capability to user-space.
> >>>>> If it's only a kernel-internal, we can fix it in the kernel and
> >>>>> nothing else broken, but if it's a part of API, that's not easy.
> >>>>>
> >>>>> So, if any, I'd like to avoid exposing to the user-space at first.
> >>>>> (But then it comes to the question how to deal with a case like AMD
> >>>>> ACP...)
> >>>>
> >>>> I tried to propose a complete solution and the ACP was one strong reason for
> >>>> this kernel / user space API. So without the user space support, it's just
> >>>> a half solution for known issues.
> >>>>
> >>>> Frankly, I don't see any drawback or a problem even if we remove this API
> >>>> later.
> >>>
> >>> Removing the user-space API is absolutely no-go.  The only exception
> >>> would be either the case really no one uses it or it's too buggy and
> >>> unfixable.
> >>
> >> This is a special case. Even if those LED bits are ignored by kernel in
> >> future, we expect to be replaced with another layer. Thus the functionality
> >> must be retained.
> > 
> > Well, we cannot know whether the replacement really happens or
> > happened, and hence we never kill the old one.  That's the problem.
> > 
> >>>> The LED group bits are just informal for the user space and it's
> >>>> expected to create the user controls tied to this LED functionality only in
> >>>> alsa-lib/plugins at the moment. The kernel may return an error when the user
> >>>> space tries to set those new bits when the API is deprecated and I believe
> >>>> that the hardware design faults like AMD ACP (without the hardware mute) are rare.
> >>>
> >>> The experience tells us that users are creative enough to (ab)use a
> >>> new ABI in any unexpected ways, and we have no control for it.  So
> >>> it's not about how alsa-lib is implemented but rather how ABI could be
> >>> abused :)
> >>
> >> Ok, I don't have other ideas. I don't agree with your argumentation for this
> >> particular case, where the functionality is marginal. Ideally, the AMD driver
> >> may be recoded to use double-buffering and software mute switch, so we should
> >> handle everything in the kernel space.
> > 
> > My argument is that we're trying to add too much freedom just for this
> > "marginal" problem.  Honestly speaking, I would feel rather more
> > comfortable if it were a kernel control element that just does trigger
> > the LED like the original patch from AMD guys.  Then you cannot do
> > much wrong.  OTOH, creating a virtual capture switch and let alsa-lib
> > handling the software mute, while PA should ignores the soft-mute but
> 
> We can force the softvol even if PA set the skip flag for this particular PCM
> stream.
> 
> > dealing only with the assigned mute LED...  Sounds too complex to me.
> 
> 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.


Takashi

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

[Jaroslav Kysela]

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

						Jaroslav

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

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

[Takashi Iwai]

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.

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


thanks,

Takashi

Re: [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@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

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

[Takashi Iwai]

On Wed, 24 Feb 2021 11:56:01 +0100,
Jaroslav Kysela wrote:
> 
> 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.

Besides the lack of some features in UCM, having the binding of the
led trigger managed in the ALSA core is good, and I have no objection.
However, again, my concern is the user controls.

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

Does it mean PA would work as of now without any change?  Or does it
need patching?

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

Right, and I'm not asking about the mute control here.  It's all about
the LED control.

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

Yes, but OTOH, if it were implemented in a kernel control, there is no
need to worry about the extra setup.  So the easiness of the setup is
a significant key point.


thanks,

Takashi

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

[Jaroslav Kysela]

Dne 24. 02. 21 v 12:43 Takashi Iwai napsal(a):

>>> 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.
> 
> Does it mean PA would work as of now without any change?  Or does it
> need patching?

Yes, no PA modifications are required with my mechanism. The PA will just see
the new user space control - mute switch - created in alsa-lib - which will be
synced the internal PA path mute state like for the hardware mute switch. I
also think that handling LEDs independently (outside the upper layers like PA)
is more flexible.

					Jaroslav

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

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

[Takashi Iwai]

On Wed, 24 Feb 2021 13:08:55 +0100,
Jaroslav Kysela wrote:
> 
> Dne 24. 02. 21 v 12:43 Takashi Iwai napsal(a):
> 
> >>> 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.
> > 
> > Does it mean PA would work as of now without any change?  Or does it
> > need patching?
> 
> Yes, no PA modifications are required with my mechanism. The PA will just see
> the new user space control - mute switch - created in alsa-lib - which will be
> synced the internal PA path mute state like for the hardware mute
> switch.

OK, but how would we create and manage the user control element?  And
why it has to be user control?


thanks,

Takashi

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

[Jaroslav Kysela]

Dne 24. 02. 21 v 13:42 Takashi Iwai napsal(a):
> On Wed, 24 Feb 2021 13:08:55 +0100,
> Jaroslav Kysela wrote:
>>
>> Dne 24. 02. 21 v 12:43 Takashi Iwai napsal(a):
>>
>>>>> 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.
>>>
>>> Does it mean PA would work as of now without any change?  Or does it
>>> need patching?
>>
>> Yes, no PA modifications are required with my mechanism. The PA will just see
>> the new user space control - mute switch - created in alsa-lib - which will be
>> synced the internal PA path mute state like for the hardware mute
>> switch.
> 
> OK, but how would we create and manage the user control element?  And
> why it has to be user control?

The softvol or alsactl can create the user control element. The alsa-lib
softvol plugin and PA can silence stream according the state.

I see your point to create this control in the kernel space, but any other
name than "Mic Capture Switch" (in the ACP case) will be misleading for users,
because the user-space does the appropriate real silencing job instead of hw.

And if we create "Mic Capture Switch" in the kernel space, it may be
misleading for applications (they can think that there's hardware mute control).

Perhaps, we can create "Mic Phantom Capture Switch" in kernel which may
resolve both problems (no hw mute information + no user confusion).

				Jaroslav

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

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

[Takashi Iwai]

On Wed, 24 Feb 2021 18:57:42 +0100,
Jaroslav Kysela wrote:
> 
> Dne 24. 02. 21 v 13:42 Takashi Iwai napsal(a):
> > On Wed, 24 Feb 2021 13:08:55 +0100,
> > Jaroslav Kysela wrote:
> >>
> >> Dne 24. 02. 21 v 12:43 Takashi Iwai napsal(a):
> >>
> >>>>> 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.
> >>>
> >>> Does it mean PA would work as of now without any change?  Or does it
> >>> need patching?
> >>
> >> Yes, no PA modifications are required with my mechanism. The PA will just see
> >> the new user space control - mute switch - created in alsa-lib - which will be
> >> synced the internal PA path mute state like for the hardware mute
> >> switch.
> > 
> > OK, but how would we create and manage the user control element?  And
> > why it has to be user control?
> 
> The softvol or alsactl can create the user control element. The alsa-lib
> softvol plugin and PA can silence stream according the state.

And that's tricky if it's only with PA, as PA won't open a softvol PCM
stream...

> I see your point to create this control in the kernel space, but any other
> name than "Mic Capture Switch" (in the ACP case) will be misleading for users,
> because the user-space does the appropriate real silencing job instead of hw.
> 
> And if we create "Mic Capture Switch" in the kernel space, it may be
> misleading for applications (they can think that there's hardware mute control).
> 
> Perhaps, we can create "Mic Phantom Capture Switch" in kernel which may
> resolve both problems (no hw mute information + no user confusion).

Yes, something like that would work.
The advantage of in-kernel implementation is that it's self-contained,
so just deploying the new kernel makes everything working.


thanks,

Takashi

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

[Jaroslav Kysela]

Dne 25. 02. 21 v 12:00 Takashi Iwai napsal(a):
> On Wed, 24 Feb 2021 18:57:42 +0100,
> Jaroslav Kysela wrote:
>>
>> Dne 24. 02. 21 v 13:42 Takashi Iwai napsal(a):
>>> On Wed, 24 Feb 2021 13:08:55 +0100,
>>> Jaroslav Kysela wrote:
>>>>
>>>> Dne 24. 02. 21 v 12:43 Takashi Iwai napsal(a):
>>>>
>>>>>>> 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.
>>>>>
>>>>> Does it mean PA would work as of now without any change?  Or does it
>>>>> need patching?
>>>>
>>>> Yes, no PA modifications are required with my mechanism. The PA will just see
>>>> the new user space control - mute switch - created in alsa-lib - which will be
>>>> synced the internal PA path mute state like for the hardware mute
>>>> switch.
>>>
>>> OK, but how would we create and manage the user control element?  And
>>> why it has to be user control?
>>
>> The softvol or alsactl can create the user control element. The alsa-lib
>> softvol plugin and PA can silence stream according the state.
> 
> And that's tricky if it's only with PA, as PA won't open a softvol PCM
> stream...

The protection is in alsa-lib, so we can skip to check this hint flag for this
particular case like:

https://github.com/alsa-project/alsa-lib/pull/121/commits/1acc1c7eccab0359996b25de54a6b6e0aa1e0c17

So it may depend on the softvol config not PA itself.

Even for the solution bellow, we need to modify softvol to handle the kernel
control elements, too. Actually softvol is not active, when the specified
control element is found and this element is not from the user space.

>> I see your point to create this control in the kernel space, but any other
>> name than "Mic Capture Switch" (in the ACP case) will be misleading for users,
>> because the user-space does the appropriate real silencing job instead of hw.
>>
>> And if we create "Mic Capture Switch" in the kernel space, it may be
>> misleading for applications (they can think that there's hardware mute control).
>>
>> Perhaps, we can create "Mic Phantom Capture Switch" in kernel which may
>> resolve both problems (no hw mute information + no user confusion).
> 
> Yes, something like that would work.
> The advantage of in-kernel implementation is that it's self-contained,
> so just deploying the new kernel makes everything working.

Ok, so let settle the naming for those controls which depends on the user
space code which does the real work (silencing). Is "Phantom" prefix good -
we're using it for jacks, or someone has a better idea?

						Jaroslav

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

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

[Takashi Iwai]

On Thu, 25 Feb 2021 19:09:44 +0100,
Jaroslav Kysela wrote:
> 
> Dne 25. 02. 21 v 12:00 Takashi Iwai napsal(a):
> > On Wed, 24 Feb 2021 18:57:42 +0100,
> > Jaroslav Kysela wrote:
> >>
> >> Dne 24. 02. 21 v 13:42 Takashi Iwai napsal(a):
> >>> On Wed, 24 Feb 2021 13:08:55 +0100,
> >>> Jaroslav Kysela wrote:
> >>>>
> >>>> Dne 24. 02. 21 v 12:43 Takashi Iwai napsal(a):
> >>>>
> >>>>>>> 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.
> >>>>>
> >>>>> Does it mean PA would work as of now without any change?  Or does it
> >>>>> need patching?
> >>>>
> >>>> Yes, no PA modifications are required with my mechanism. The PA will just see
> >>>> the new user space control - mute switch - created in alsa-lib - which will be
> >>>> synced the internal PA path mute state like for the hardware mute
> >>>> switch.
> >>>
> >>> OK, but how would we create and manage the user control element?  And
> >>> why it has to be user control?
> >>
> >> The softvol or alsactl can create the user control element. The alsa-lib
> >> softvol plugin and PA can silence stream according the state.
> > 
> > And that's tricky if it's only with PA, as PA won't open a softvol PCM
> > stream...
> 
> The protection is in alsa-lib, so we can skip to check this hint flag for this
> particular case like:
> 
> https://github.com/alsa-project/alsa-lib/pull/121/commits/1acc1c7eccab0359996b25de54a6b6e0aa1e0c17
> 
> So it may depend on the softvol config not PA itself.

Thanks, that's what I missed from the big picture.

> Even for the solution bellow, we need to modify softvol to handle the kernel
> control elements, too. Actually softvol is not active, when the specified
> control element is found and this element is not from the user space.

Hm, that would certainly work, but as we discussed before, it enforces
the softvol PCM process for PA.  That isn't too bad for the capture,
fortunately, but not ideal, OTOH.

And, I'm not sure how PA can take any control as its main capture mute
switch, if it's named differently.  Wouldn't we need to change mixer
path in PA as well?  And, if we may change PA side, it sounds more
natural to change a control directly in PA's mixer path.  The softvol
doesn't fit well with PA, after all.

> >> I see your point to create this control in the kernel space, but any other
> >> name than "Mic Capture Switch" (in the ACP case) will be misleading for users,
> >> because the user-space does the appropriate real silencing job instead of hw.
> >>
> >> And if we create "Mic Capture Switch" in the kernel space, it may be
> >> misleading for applications (they can think that there's hardware mute control).
> >>
> >> Perhaps, we can create "Mic Phantom Capture Switch" in kernel which may
> >> resolve both problems (no hw mute information + no user confusion).
> > 
> > Yes, something like that would work.
> > The advantage of in-kernel implementation is that it's self-contained,
> > so just deploying the new kernel makes everything working.
> 
> Ok, so let settle the naming for those controls which depends on the user
> space code which does the real work (silencing). Is "Phantom" prefix good -
> we're using it for jacks, or someone has a better idea?

If we have to go in this way, that's an acceptable name, I think.


thanks,

Takashi

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

[Jaroslav Kysela]

Dne 26. 02. 21 v 9:41 Takashi Iwai napsal(a):
> On Thu, 25 Feb 2021 19:09:44 +0100,
> Jaroslav Kysela wrote:
>>
>> Dne 25. 02. 21 v 12:00 Takashi Iwai napsal(a):
>>> On Wed, 24 Feb 2021 18:57:42 +0100,
>>> Jaroslav Kysela wrote:
>>>>
>>>> Dne 24. 02. 21 v 13:42 Takashi Iwai napsal(a):
>>>>> On Wed, 24 Feb 2021 13:08:55 +0100,
>>>>> Jaroslav Kysela wrote:
>>>>>>
>>>>>> Dne 24. 02. 21 v 12:43 Takashi Iwai napsal(a):
>>>>>>
>>>>>>>>> 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.
>>>>>>>
>>>>>>> Does it mean PA would work as of now without any change?  Or does it
>>>>>>> need patching?
>>>>>>
>>>>>> Yes, no PA modifications are required with my mechanism. The PA will just see
>>>>>> the new user space control - mute switch - created in alsa-lib - which will be
>>>>>> synced the internal PA path mute state like for the hardware mute
>>>>>> switch.
>>>>>
>>>>> OK, but how would we create and manage the user control element?  And
>>>>> why it has to be user control?
>>>>
>>>> The softvol or alsactl can create the user control element. The alsa-lib
>>>> softvol plugin and PA can silence stream according the state.
>>>
>>> And that's tricky if it's only with PA, as PA won't open a softvol PCM
>>> stream...
>>
>> The protection is in alsa-lib, so we can skip to check this hint flag for this
>> particular case like:
>>
>> https://github.com/alsa-project/alsa-lib/pull/121/commits/1acc1c7eccab0359996b25de54a6b6e0aa1e0c17
>>
>> So it may depend on the softvol config not PA itself.
> 
> Thanks, that's what I missed from the big picture.
> 
>> Even for the solution bellow, we need to modify softvol to handle the kernel
>> control elements, too. Actually softvol is not active, when the specified
>> control element is found and this element is not from the user space.
> 
> Hm, that would certainly work, but as we discussed before, it enforces
> the softvol PCM process for PA.  That isn't too bad for the capture,
> fortunately, but not ideal, OTOH.

PA can work without softvol in this case, but in my opinion, it may be better
to force the silence in UCM (do not depend only on the implementation of the
UCM application - like PA - more security).

As I wrote before, the silencing can be done in alsa-lib AND in PA
(simultaneously). If we use only mute switch control configuration (on/off -
not volume) for sofvol, then the code in alsa-lib is really effective without
any performance penalty.

> And, I'm not sure how PA can take any control as its main capture mute
> switch, if it's named differently.  Wouldn't we need to change mixer
> path in PA as well?  And, if we may change PA side, it sounds more
> natural to change a control directly in PA's mixer path.  The softvol
> doesn't fit well with PA, after all.

The ACP microphone is handled via UCM, so we can add the new control switch
identifier to UCM. No PA modifications should be required. And the LED state
can be handled via the phantom control (AMD ACP) + legacy HDA capture switch.
So two controls from two different sound cards may be marked with the LED
microphone group, if it's the correct way for users.

					Jaroslav

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

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

[Hans de Goede]

Hi,

On 2/23/21 5:14 PM, Jaroslav Kysela wrote:
> Dne 23. 02. 21 v 15:21 Takashi Iwai napsal(a):
>> On Tue, 23 Feb 2021 15:09:30 +0100,
>> Mark Brown wrote:
>>>
>>> On Tue, Feb 23, 2021 at 02:59:17PM +0100, Hans de Goede wrote:
>>>> On 2/23/21 2:45 PM, Mark Brown wrote:
>>>>> On Mon, Feb 15, 2021 at 03:24:19PM +0100, Hans de Goede wrote:
>>>
>>>>> Why just these particular controls - what if a system has separate mutes
>>>>> for speakers or something?
>>>
>>>> These are the main volume controls, which are always in the output / input
>>>> path independent on if we are outputting to e.g. speakers or the headphones.
>>>
>>>> We want to use the main volume control for this, because there always is
>>>> only 1 output mute LED and 1 input mute LED. Well at least that is the assumption
>>>> the current ledtrig-audio.c code has.
>>>
>>>> The idea is to only turn the single LED on if we are sure there will be not
>>>> sound output on any of the outputs, which is why we tie the LED to the
>>>> mute switch on the main volume control.
>>>
>>> Right, so that might work well on your particular system with your
>>> particular configuration but will it work well on other systems with
>>> different hardware?  It's not clear to me that it makes sense to go
>>> through all the drivers picking controls that might be used for this
>>> purpose - it seems both time consuming and error prone.  Consider a
>>> mostly digital device which has an ADC/DAC per input/output rather than
>>> a central ADC/DAC with analogue muxing for example, or a system with
>>> multiple DACs available for mixing together or analogue bypassess.  
>>
>> 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.

FWIW I'm more then happy to rework this RFC series to make the setting of the
SNDRV_CTL_ELEM_ACCESS_*_LED flag only happen on certain models based on the
existing DMI quirks mechanism in the driver.

So far I'm aware of only a few 2-in-1 models which actually have a mute LED
embedded in the (mic)mute key in their detachable keyboard.

Please let me know if you would prefer for me to rework my series
to handle the setting of the flag on a per model (DMI match) basis.

I see 2 additional advantages of this approach:

1. It would allow us to not modprobe the snd-ctl-led module on devices
where we don't need it.

2. The rt5640 codec has 2 I2S interfaces / 2 AIFs and that brings
some challenges with it. I currently have a working implementation
(which needs some more testing before posting it upstream as RFC) which
sets the SNDRV_CTL_ELEM_ACCESS_SPK_LED flag on the main volume control
for both the AIF1 and AIF2 paths to the outputs (which have separate
main volume controls) and then relies on userspace to always mute the
main volume of the unused AIF.

This requires the UCM profile to know which AIF is being used, if we
do this on a per model basis then we will know which AIF is used and
we can set the SNDRV_CTL_ELEM_ACCESS_SPK_LED flag on only one of the
2 main volume-controls, removing the requirement for userspace to
mute the unused main volume-control.

###

Note that userspace will still need to know which AIF is being used,
regardless of the mute-LED discussion, because it needs to know to
be able select the right volume-control for hardware volume control
(which is something which we really want in general, esp. for the
analog mic inputs and is necessary for the mute-LED to work).

Since userspace needs to know which AIF is being used, I've written
a kernel patch modifying the machine-driver to add a new "aif:1" or
"aif:2" string to the components string.

I have this all working now, and I will post a RFC upstream once
I've run some final tests, for the UCM changes for this, which
illustrate the need for userspace to know which AIF is used, see:

https://github.com/jwrdegoede/alsa-ucm-conf/commits/master

And specifically:

https://github.com/jwrdegoede/alsa-ucm-conf/commit/afce963f725a5659eb903508d924d3b19110244a
https://github.com/jwrdegoede/alsa-ucm-conf/commit/d2128e5380d9d6128dda67dd94f64bf7244cd3a3

Regards,

Hans

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1268 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Hans de Goede]

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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1908 bytes --]

On Tue, Feb 23, 2021 at 08:03:58PM +0100, Hans de Goede wrote:
> On 2/23/21 6:20 PM, Mark Brown wrote:

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

Yes, we at least need to be able to do that even if we end up hard
coding it in some CODEC drivers as the device is inflexible.  There are
devices where the concept of "main DAC/ADC" just doesn't apply.

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

I'm not a big fan of module parameters TBH, it's not great for
usability.

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

The problem I was thinking of is the situation where there are multiple
options for the mute control in the hardware and it's a configuration
decision which one to use.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Hans de Goede]

Hi,

On 2/24/21 1:59 PM, Mark Brown wrote:
> On Tue, Feb 23, 2021 at 08:03:58PM +0100, Hans de Goede wrote:
>> On 2/23/21 6:20 PM, Mark Brown wrote:
> 
>>> 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?
> 
> Yes, we at least need to be able to do that even if we end up hard
> coding it in some CODEC drivers as the device is inflexible.  There are
> devices where the concept of "main DAC/ADC" just doesn't apply.
> 
>>> 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
> 
> I'm not a big fan of module parameters TBH, it's not great for
> usability.
> 
>> 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.
> 
> The problem I was thinking of is the situation where there are multiple
> options for the mute control in the hardware and it's a configuration
> decision which one to use.

ATM we have no device where this situation happens, so I would prefer
to cross that bridge when we come to it.

Regards,

Hans

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 500 bytes --]

On Wed, Feb 24, 2021 at 08:14:12PM +0100, Hans de Goede wrote:
> On 2/24/21 1:59 PM, Mark Brown wrote:

> > The problem I was thinking of is the situation where there are multiple
> > options for the mute control in the hardware and it's a configuration
> > decision which one to use.

> ATM we have no device where this situation happens, so I would prefer
> to cross that bridge when we come to it.

You just added wm5012 machine drivers, that device is going to present
issues with this approach.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Hans de Goede]

Hi,

On 2/24/21 8:36 PM, Mark Brown wrote:
> On Wed, Feb 24, 2021 at 08:14:12PM +0100, Hans de Goede wrote:
>> On 2/24/21 1:59 PM, Mark Brown wrote:
> 
>>> The problem I was thinking of is the situation where there are multiple
>>> options for the mute control in the hardware and it's a configuration
>>> decision which one to use.
> 
>> ATM we have no device where this situation happens, so I would prefer
>> to cross that bridge when we come to it.
> 
> You just added wm5012 machine drivers, that device is going to present
> issues with this approach.

I've no intention to add led-trigger support to the bytcr-wm5102 driver,
since to the best of my knowledge there are no mute LEDs on any designs
using the bytcr + wm5102 combination.

I'm aware of there actually being a mute LED on only 3 2-in-1 models
with detachable keyboards (with the mute LEDs sitting inside the
media-keys to toggle the mute, like how it is done on thinkpads):

1. The Thinkpad10 bytcr+rt5670 tablet with its USB ultrabook keyboard dock
2. The HP x2 10" clamshell designs with detachable keyboards in
   2 variants, bytcr + rt5640 and cht + rt5640.

Thats it, and in both cases the codecs have main ADC / DAC volume
controls which are the only ones suited for using as the control
to drive the led-trigger. Which is why this RFC just hardcoded
the trigger to that control.

As I said I'll happily respin this RFC series (and the not yet
posted similar rt5640 series) to tie the led-trigger to specific
controls based on DMI quirks.

Given that the use of mute LEDs itself is actually rare and especially
the use of mute LEDs in combination with ASoC coming up with some
generic configuration mechanism to allow userspace to tie the
led-trigger to any random 'Switch' type control is way overkill /
overengineering and I've no desire to spend a huge amount of time
on implementing this.

Not to mention that this would just be punting the actual problem
of figuring out which control to use to userspace, while the kernel
is actually in a better place to make this decision since the kernel
already uses DMI based quirks to deal with model specific configuration.

Regards,

Hans

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 892 bytes --]

On Wed, Feb 24, 2021 at 09:09:36PM +0100, Hans de Goede wrote:

> Given that the use of mute LEDs itself is actually rare and especially
> the use of mute LEDs in combination with ASoC coming up with some
> generic configuration mechanism to allow userspace to tie the

This seems like an optimistic set of assumptions - it may reflect
current laptops but it sounds like the sort of thing people might
deploy on future devices, never mind all the non-laptops that could end
up wanting to use this mechanism.

> Not to mention that this would just be punting the actual problem
> of figuring out which control to use to userspace, while the kernel
> is actually in a better place to make this decision since the kernel
> already uses DMI based quirks to deal with model specific configuration.

Again, this only works in cases where there's only one option for the
control that could be used.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Hans de Goede]

Hi,

On 2/25/21 3:59 PM, Mark Brown wrote:
> On Wed, Feb 24, 2021 at 09:09:36PM +0100, Hans de Goede wrote:
> 
>> Given that the use of mute LEDs itself is actually rare and especially
>> the use of mute LEDs in combination with ASoC coming up with some
>> generic configuration mechanism to allow userspace to tie the
> 
> This seems like an optimistic set of assumptions - it may reflect
> current laptops but it sounds like the sort of thing people might
> deploy on future devices, never mind all the non-laptops that could end
> up wanting to use this mechanism.
> 
>> Not to mention that this would just be punting the actual problem
>> of figuring out which control to use to userspace, while the kernel
>> is actually in a better place to make this decision since the kernel
>> already uses DMI based quirks to deal with model specific configuration.
> 
> Again, this only works in cases where there's only one option for the
> control that could be used.

Which is the case in all the current models which I'm trying to get
the mute LED supported on.  In its current form this is all purely
handled inside the kernel, so we can easily change / extend the mechanism
later without any problems.

OTOH adding an interface where userspace can runtime set which control
to use for this, would require adding some userspace API for this.

To me it seems like a really bad idea to add userspace API for this now,
when we don't actually have hardware which needs this. Introducing
userspace API for this now introduces a significant risk that we get the
API wrong, since we don't actuall have a use-case where we actually need
the suggested flexibility. And then if such a use-case does eventually
pop-up we might very well have gotten the userspace API for this wrong.

I'm not saying that we will never need such flexibility, but we do not
need it *now*, so as I said before lets cross that bridge when we reach it. 

Regards,

Hans

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1050 bytes --]

On Thu, Feb 25, 2021 at 07:45:15PM +0100, Hans de Goede wrote:

> To me it seems like a really bad idea to add userspace API for this now,
> when we don't actually have hardware which needs this. Introducing
> userspace API for this now introduces a significant risk that we get the
> API wrong, since we don't actuall have a use-case where we actually need
> the suggested flexibility. And then if such a use-case does eventually
> pop-up we might very well have gotten the userspace API for this wrong.

> I'm not saying that we will never need such flexibility, but we do not
> need it *now*, so as I said before lets cross that bridge when we reach it. 

I don't want to get stuck in a cycle of "why can't my system just do
what this other system does", or worse end up with problems due to
competing system requirements when patches go in on more flexible
devices because I didn't notice that the device wasn't a good fit for
this sort of thing but people have the expectation that the kernel will
transparently handle things.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Hans de Goede]

Hi,

On 3/1/21 2:23 PM, Mark Brown wrote:
> On Thu, Feb 25, 2021 at 07:45:15PM +0100, Hans de Goede wrote:
> 
>> To me it seems like a really bad idea to add userspace API for this now,
>> when we don't actually have hardware which needs this. Introducing
>> userspace API for this now introduces a significant risk that we get the
>> API wrong, since we don't actuall have a use-case where we actually need
>> the suggested flexibility. And then if such a use-case does eventually
>> pop-up we might very well have gotten the userspace API for this wrong.
> 
>> I'm not saying that we will never need such flexibility, but we do not
>> need it *now*, so as I said before lets cross that bridge when we reach it. 
> 
> I don't want to get stuck in a cycle of "why can't my system just do
> what this other system does", or worse end up with problems due to
> competing system requirements when patches go in on more flexible
> devices because I didn't notice that the device wasn't a good fit for
> this sort of thing but people have the expectation that the kernel will
> transparently handle things.

So what do you want / how do you want this to work ?

Regards,

Hans

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1015 bytes --]

On Mon, Mar 01, 2021 at 02:39:46PM +0100, Hans de Goede wrote:
> On 3/1/21 2:23 PM, Mark Brown wrote:

> > I don't want to get stuck in a cycle of "why can't my system just do
> > what this other system does", or worse end up with problems due to
> > competing system requirements when patches go in on more flexible
> > devices because I didn't notice that the device wasn't a good fit for
> > this sort of thing but people have the expectation that the kernel will
> > transparently handle things.

> So what do you want / how do you want this to work ?

Off the top of my head something like writing a control name into a
sysfs file might work, it doesn't scale if you need to use multiple
controls as rt5640 does though.  We could also do something like have
drivers make a list of all output stage mutes and then use that to build
a standard global mute control which functions similarly to this one and
could be force wired to the LED trigger input, seems like a big hammer
but it'd be reasonably consistent.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Hans de Goede]

Hi,

On 3/1/21 8:15 PM, Mark Brown wrote:
> On Mon, Mar 01, 2021 at 02:39:46PM +0100, Hans de Goede wrote:
>> On 3/1/21 2:23 PM, Mark Brown wrote:
> 
>>> I don't want to get stuck in a cycle of "why can't my system just do
>>> what this other system does", or worse end up with problems due to
>>> competing system requirements when patches go in on more flexible
>>> devices because I didn't notice that the device wasn't a good fit for
>>> this sort of thing but people have the expectation that the kernel will
>>> transparently handle things.
> 
>> So what do you want / how do you want this to work ?
> 
> Off the top of my head something like writing a control name into a
> sysfs file might work, it doesn't scale if you need to use multiple
> controls as rt5640 does though.

Currently ALSA/UCM does not use sysfs files for anything, so this
feels very inconsistent with how all the rest of this currently works.

A sysfs file is also only accessible by root, making it impossible for
say pulseaudio or pipewire running as a normal user to set this up.

So then we would need a database for this somewhere outside of UCM and
a process running as root to write the sysfs file.

> We could also do something like have
> drivers make a list of all output stage mutes and then use that to build
> a standard global mute control which functions similarly to this one and
> could be force wired to the LED trigger input, seems like a big hammer
> but it'd be reasonably consistent.

This sounds like a better plan IMHO.

Jaroslav's current mixer-element based generic LED support patchset
already allows setting the "this mixer-element control the spk-mute led"
on multiple controls and then the LED will only turn on if all such
controls are set to muted.

So that part is covered and my initial plan for the rt5640 was actually
to do just this but the problem is that the mixer element names for
the outputs were poorly chosen initially:

        /* Speaker Output Volume */
        SOC_DOUBLE("Speaker Channel Switch", RT5640_SPK_VOL,
                RT5640_VOL_L_SFT, RT5640_VOL_R_SFT, 1, 1),
        SOC_DOUBLE_TLV("Speaker Playback Volume", RT5640_SPK_VOL,
                RT5640_L_VOL_SFT, RT5640_R_VOL_SFT, 39, 1, out_vol_tlv),

Where userspace expect "Speaker Channel Switch" to be named
"Speaker Playback Switch" (aligning it with the vol-control name)
instead.

And we cannot just rename this since the control names are
used in UCM profiles and if a UCM profile refers to a non-existing
control it won't work.

I recently did an alsa-lib patch-series to deal with other
non ideally named mixer controls:

https://lore.kernel.org/alsa-devel/20210228161304.241288-1-hdegoede@redhat.com/T/#t

I guess that we could deal with this unfortunate naming inside
alsa-lib too. I guess that the alsa-lib folks won't be thrilled
by doing this, but we need to deal with this mess without breaking
compatibility somewhere and alsa-lib might be the best place for this.

Or we could change the "Speaker Channel Switch" control into a dummy control,
the current UCM profiles already disable a bunch of other controls in the
DAPM graph too, so the whole chain will turn off anyways.

And then add a new non-dummy "Speaker Playback Switch" control.

I do know that we need to much more careful going forward to make sure
that control names match the conventions expected by userspace.

Regards,

Hans

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2541 bytes --]

On Mon, Mar 01, 2021 at 08:49:34PM +0100, Hans de Goede wrote:
> On 3/1/21 8:15 PM, Mark Brown wrote:

> > Off the top of my head something like writing a control name into a
> > sysfs file might work, it doesn't scale if you need to use multiple
> > controls as rt5640 does though.

> Currently ALSA/UCM does not use sysfs files for anything, so this
> feels very inconsistent with how all the rest of this currently works.

Yes, you'd really want to add string controls in ALSA.

> > drivers make a list of all output stage mutes and then use that to build
> > a standard global mute control which functions similarly to this one and
> > could be force wired to the LED trigger input, seems like a big hammer
> > but it'd be reasonably consistent.

>         /* Speaker Output Volume */
>         SOC_DOUBLE("Speaker Channel Switch", RT5640_SPK_VOL,
>                 RT5640_VOL_L_SFT, RT5640_VOL_R_SFT, 1, 1),
>         SOC_DOUBLE_TLV("Speaker Playback Volume", RT5640_SPK_VOL,
>                 RT5640_L_VOL_SFT, RT5640_R_VOL_SFT, 39, 1, out_vol_tlv),

> Where userspace expect "Speaker Channel Switch" to be named
> "Speaker Playback Switch" (aligning it with the vol-control name)
> instead.

This isn't great but be aware that the control names stuff breaks down
very, very easily in the presence of general hardware - things like
multiple general purpose outputs can cause problems.

In any case a big hammer virtual control which mapped straight onto the
LED would sidestep some of that, though it does assume there are useful
mute controls in all the paths which may or may not be the case.

> And we cannot just rename this since the control names are
> used in UCM profiles and if a UCM profile refers to a non-existing
> control it won't work.

I thought UCM already had support for remapping control names?  It was
certainly something discussed very early on - a mechanism to allow the
UCM file to say "treat control X as name Y in this use case", where the
X used for Y might vary between use cases.

> I do know that we need to much more careful going forward to make sure
> that control names match the conventions expected by userspace.

That in general won't scale well, ideally we'd be exposing the routing
graph to userspace and annotating the non-DAPM controls onto that
routing graph so that userspace can figure out where everything sits -
that'd make several things a lot easier.  It does require somoene with
the time and enthusiasm to define a new ABI though which isn't something
you should hold your breath for.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Hans de Goede]

Hi,

On 3/1/21 9:43 PM, Mark Brown wrote:
> On Mon, Mar 01, 2021 at 08:49:34PM +0100, Hans de Goede wrote:
>> On 3/1/21 8:15 PM, Mark Brown wrote:
> 
>>> Off the top of my head something like writing a control name into a
>>> sysfs file might work, it doesn't scale if you need to use multiple
>>> controls as rt5640 does though.
> 
>> Currently ALSA/UCM does not use sysfs files for anything, so this
>> feels very inconsistent with how all the rest of this currently works.
> 
> Yes, you'd really want to add string controls in ALSA.

Hmm, we already have SNDRV_CTL_ELEM_TYPE_BYTES controls. I think that will
work nicely actually, we can have the UCM conf file send a 0 terminated
string to the driver that way. It would be nice to have some syntactic
sugar on the UCM side to be able to actually specify a string instead
of an array of bytes, but I don't think we need any new userspace API
for this.

This can be combined with a SND_SOC_... macro + helper to add an
entry to the usual snd_kcontrol_new <codec-name>_snd_controls[] table for
this, which when the control gets set/put will walk over all the controls
and find one with a matching name and then add the access flag which
Jaroslav's code uses to control the led-trigger to the matching control.

I think that that should work nicely for the use-cases which I have
ATM and should be flexible enough for future cases. The same control
can even be written multiple times to set the flag on multiple controls
and we could have a write of an empty string clear the flag on all controls.

So in UCM (with the syntactic sugar) for the rt5640 we could then e.g.
use something like this:

# Clear Speaker Mute LED flags from all controls, then set it
# on the Speaker and HP Channel Switches
cset "name='Speaker Mute LED Control' ''"
cset "name='Speaker Mute LED Control' 'Speaker Channel Switch'"
cset "name='Speaker Mute LED Control' 'HP Channel Switch'"

Mark, does this sound like an acceptable solution to you ?

I know you will want to see the actual code before you can give me
a definitive yes on this, but I would like to know that this at least
looks like it would be acceptable to you before spending time on coding
this out and testing it.

Regards,

Hans

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 178 bytes --]

On Mon, Mar 01, 2021 at 10:26:40PM +0100, Hans de Goede wrote:

> Mark, does this sound like an acceptable solution to you ?

Yes, I think this looks like a reasonable approach.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Jaroslav Kysela]

Dne 01. 03. 21 v 22:26 Hans de Goede napsal(a):
> Hi,
> 
> On 3/1/21 9:43 PM, Mark Brown wrote:
>> On Mon, Mar 01, 2021 at 08:49:34PM +0100, Hans de Goede wrote:
>>> On 3/1/21 8:15 PM, Mark Brown wrote:
>>
>>>> Off the top of my head something like writing a control name into a
>>>> sysfs file might work, it doesn't scale if you need to use multiple
>>>> controls as rt5640 does though.
>>
>>> Currently ALSA/UCM does not use sysfs files for anything, so this
>>> feels very inconsistent with how all the rest of this currently works.
>>
>> Yes, you'd really want to add string controls in ALSA.
> 
> Hmm, we already have SNDRV_CTL_ELEM_TYPE_BYTES controls. I think that will
> work nicely actually, we can have the UCM conf file send a 0 terminated
> string to the driver that way. It would be nice to have some syntactic
> sugar on the UCM side to be able to actually specify a string instead
> of an array of bytes, but I don't think we need any new userspace API
> for this.

The LEDs are controlled per machine not per card. So do we need to create the 'Speaker/Mic LED Control' control for all cards?

Also, this change sounds really generic. The interface may be implemented in my proposed control led kernel module, not in the codec drivers.

The Mark's sysfs idea is not bad in my opinion. The sequences may be extended in UCM, we have already 'exec' command. Yes, this command is a little heavy for the sysfs writes, but we can add command like 'sysset' or so for sysfs like:

  # detach all speaker LED controls for card 1
  # similar to 'echo -n "card=1,*" > /sysfs/devices/virtual/sound/ctl-led/speaker/detach'
  sysset "devices/virtual/sound/ctl-led/speaker/detach:card=1,*"

  # attach the 'Speaker Playback Switch',10 control to speaker LED trigger in card 1
  # similar to 'echo -n "card=1,iface=MIXER,name='Speaker Playback Switch',index=10" > /sysfs/devices/virtual/sound/ctl-led/speaker/attach
  sysset "devices/virtual/sound/ctl-led/speaker/attach:card=1,iface=MIXER,name='Speaker Playback Switch',index=10"

Security: The LED-control bindings should be handled only in the boot / init phase (thus in UCM BootSequence section) and the sysfs interface files should be read-only for normal users.

				Jaroslav

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

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

[Hans de Goede]

Hi,

On 3/2/21 10:14 PM, Jaroslav Kysela wrote:
> Dne 01. 03. 21 v 22:26 Hans de Goede napsal(a):
>> Hi,
>>
>> On 3/1/21 9:43 PM, Mark Brown wrote:
>>> On Mon, Mar 01, 2021 at 08:49:34PM +0100, Hans de Goede wrote:
>>>> On 3/1/21 8:15 PM, Mark Brown wrote:
>>>
>>>>> Off the top of my head something like writing a control name into a
>>>>> sysfs file might work, it doesn't scale if you need to use multiple
>>>>> controls as rt5640 does though.
>>>
>>>> Currently ALSA/UCM does not use sysfs files for anything, so this
>>>> feels very inconsistent with how all the rest of this currently works.
>>>
>>> Yes, you'd really want to add string controls in ALSA.
>>
>> Hmm, we already have SNDRV_CTL_ELEM_TYPE_BYTES controls. I think that will
>> work nicely actually, we can have the UCM conf file send a 0 terminated
>> string to the driver that way. It would be nice to have some syntactic
>> sugar on the UCM side to be able to actually specify a string instead
>> of an array of bytes, but I don't think we need any new userspace API
>> for this.
> 
> The LEDs are controlled per machine not per card. So do we need to create the 'Speaker/Mic LED Control' control for all cards?
> 
> Also, this change sounds really generic. The interface may be implemented in my proposed control led kernel module, not in the codec drivers.
> 
> The Mark's sysfs idea is not bad in my opinion. The sequences may be extended in UCM, we have already 'exec' command. Yes, this command is a little heavy for the sysfs writes, but we can add command like 'sysset' or so for sysfs like:

Okay, so this would be a sysfs file per card then? Sol we would have for example
2 new sysfs files like this show up when your module is loaded:

/sys/class/sounds/card0/spk_mute_led_control
/sys/class/sounds/card0/mic_mute_led_control

And reading would iterate over all mixer-elements of the card and print
the names of those which have the relevant access LED flag set, where
as a write would be taken as a control-name to add the access LED flag
too?


And an empty write would be special and clear the flag on all controls?
I guess we don't strictly need that if we only set things up at boot once,
but it might still be handy to force things to a clean state.

> 
>   # detach all speaker LED controls for card 1
>   # similar to 'echo -n "card=1,*" > /sysfs/devices/virtual/sound/ctl-led/speaker/detach'
>   sysset "devices/virtual/sound/ctl-led/speaker/detach:card=1,*"
> 
>   # attach the 'Speaker Playback Switch',10 control to speaker LED trigger in card 1
>   # similar to 'echo -n "card=1,iface=MIXER,name='Speaker Playback Switch',index=10" > /sysfs/devices/virtual/sound/ctl-led/speaker/attach
>   sysset "devices/virtual/sound/ctl-led/speaker/attach:card=1,iface=MIXER,name='Speaker Playback Switch',index=10"

I think a sysfs file per card would work better, that would certainly be
a lot more inline with how sysfs is normally used...

Also do we need the iface=MIXER part ?

> Security: The LED-control bindings should be handled only in the boot / init phase (thus in UCM BootSequence section) and the sysfs interface files should be read-only for normal users.

Yes that make sense, but it will require some extra helper to that, I guess it
could be an extra flag to the alsactl restore command which already gets run
at boot, or an extra alsactl command ?

Regards,

Hans

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

[Jaroslav Kysela]

Dne 04. 03. 21 v 20:39 Hans de Goede napsal(a):
> Hi,
> 
> On 3/2/21 10:14 PM, Jaroslav Kysela wrote:
>> Dne 01. 03. 21 v 22:26 Hans de Goede napsal(a):
>>> Hi,
>>>
>>> On 3/1/21 9:43 PM, Mark Brown wrote:
>>>> On Mon, Mar 01, 2021 at 08:49:34PM +0100, Hans de Goede wrote:
>>>>> On 3/1/21 8:15 PM, Mark Brown wrote:
>>>>
>>>>>> Off the top of my head something like writing a control name into a
>>>>>> sysfs file might work, it doesn't scale if you need to use multiple
>>>>>> controls as rt5640 does though.
>>>>
>>>>> Currently ALSA/UCM does not use sysfs files for anything, so this
>>>>> feels very inconsistent with how all the rest of this currently works.
>>>>
>>>> Yes, you'd really want to add string controls in ALSA.
>>>
>>> Hmm, we already have SNDRV_CTL_ELEM_TYPE_BYTES controls. I think that will
>>> work nicely actually, we can have the UCM conf file send a 0 terminated
>>> string to the driver that way. It would be nice to have some syntactic
>>> sugar on the UCM side to be able to actually specify a string instead
>>> of an array of bytes, but I don't think we need any new userspace API
>>> for this.
>>
>> The LEDs are controlled per machine not per card. So do we need to create the 'Speaker/Mic LED Control' control for all cards?
>>
>> Also, this change sounds really generic. The interface may be implemented in my proposed control led kernel module, not in the codec drivers.
>>
>> The Mark's sysfs idea is not bad in my opinion. The sequences may be extended in UCM, we have already 'exec' command. Yes, this command is a little heavy for the sysfs writes, but we can add command like 'sysset' or so for sysfs like:
> 
> Okay, so this would be a sysfs file per card then? Sol we would have for example
> 2 new sysfs files like this show up when your module is loaded:
> 
> /sys/class/sounds/card0/spk_mute_led_control
> /sys/class/sounds/card0/mic_mute_led_control
> 
> And reading would iterate over all mixer-elements of the card and print
> the names of those which have the relevant access LED flag set, where
> as a write would be taken as a control-name to add the access LED flag
> too?

It depends if we want to have this feature as an independent add-on
(implemented in the generic sound LED module) or if we tie this more to
the ALSA's core control code.

My proposal creates virtual sound ctl-led driver - thus
/sysfs/devices/virtual/sound/ctl-led/ tree. We can add a subdirectory per card
there like:

/sysfs/devices/virtual/sound/ctl-led/speaker/card0/attach

...

> And an empty write would be special and clear the flag on all controls?
> I guess we don't strictly need that if we only set things up at boot once,
> but it might still be handy to force things to a clean state.

The list operations should be probably identified by separate sysfs files.

/sysfs/devices/virtual/sound/ctl-led/speaker/card0/attach
/sysfs/devices/virtual/sound/ctl-led/speaker/card0/detach
/sysfs/devices/virtual/sound/ctl-led/speaker/card0/reset
/sysfs/devices/virtual/sound/ctl-led/speaker/card0/controls

And /sys/class/sounds/card0/controlC0/led-speaker may be a symlink to
/sysfs/devices/virtual/sound/ctl-led/speaker/card0/ .

>>   # detach all speaker LED controls for card 1
>>   # similar to 'echo -n "card=1,*" > /sysfs/devices/virtual/sound/ctl-led/speaker/detach'
>>   sysset "devices/virtual/sound/ctl-led/speaker/detach:card=1,*"
>>
>>   # attach the 'Speaker Playback Switch',10 control to speaker LED trigger in card 1
>>   # similar to 'echo -n "card=1,iface=MIXER,name='Speaker Playback Switch',index=10" > /sysfs/devices/virtual/sound/ctl-led/speaker/attach
>>   sysset "devices/virtual/sound/ctl-led/speaker/attach:card=1,iface=MIXER,name='Speaker Playback Switch',index=10"
> 
> I think a sysfs file per card would work better, that would certainly be
> a lot more inline with how sysfs is normally used...
> 
> Also do we need the iface=MIXER part ?

It was just a complete example (element ID specification in a string from
alsa-lib/amixer). Of course, the other element types won't be probably used
for the LED functionality...

>> Security: The LED-control bindings should be handled only in the boot / init phase (thus in UCM BootSequence section) and the sysfs interface files should be read-only for normal users.
> 
> Yes that make sense, but it will require some extra helper to that, I guess it
> could be an extra flag to the alsactl restore command which already gets run
> at boot, or an extra alsactl command ?

The alsactl does the BootSequence initialization when UCM is supported in
alsa-lib. But, we have a little issue that the this sequence is executed only
if some controls are changed (added or removed) between last alsa state config
(/var/lib/alsa/asound.state) or if the state for the given card does not exist
to not override the values which may be changed by the user. It really depends
on the control API only.

Apparently, we need another sequence which will be forcefully executed on
boot. ColdSequence , FixedBootSequence, ForcedSequence, ForcedBootSequence ?

					Jaroslav

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

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

[Hans de Goede]

Hi,

On 3/5/21 2:02 PM, Jaroslav Kysela wrote:
> Dne 04. 03. 21 v 20:39 Hans de Goede napsal(a):
>> Hi,
>>
>> On 3/2/21 10:14 PM, Jaroslav Kysela wrote:
>>> Dne 01. 03. 21 v 22:26 Hans de Goede napsal(a):
>>>> Hi,
>>>>
>>>> On 3/1/21 9:43 PM, Mark Brown wrote:
>>>>> On Mon, Mar 01, 2021 at 08:49:34PM +0100, Hans de Goede wrote:
>>>>>> On 3/1/21 8:15 PM, Mark Brown wrote:
>>>>>
>>>>>>> Off the top of my head something like writing a control name into a
>>>>>>> sysfs file might work, it doesn't scale if you need to use multiple
>>>>>>> controls as rt5640 does though.
>>>>>
>>>>>> Currently ALSA/UCM does not use sysfs files for anything, so this
>>>>>> feels very inconsistent with how all the rest of this currently works.
>>>>>
>>>>> Yes, you'd really want to add string controls in ALSA.
>>>>
>>>> Hmm, we already have SNDRV_CTL_ELEM_TYPE_BYTES controls. I think that will
>>>> work nicely actually, we can have the UCM conf file send a 0 terminated
>>>> string to the driver that way. It would be nice to have some syntactic
>>>> sugar on the UCM side to be able to actually specify a string instead
>>>> of an array of bytes, but I don't think we need any new userspace API
>>>> for this.
>>>
>>> The LEDs are controlled per machine not per card. So do we need to create the 'Speaker/Mic LED Control' control for all cards?
>>>
>>> Also, this change sounds really generic. The interface may be implemented in my proposed control led kernel module, not in the codec drivers.
>>>
>>> The Mark's sysfs idea is not bad in my opinion. The sequences may be extended in UCM, we have already 'exec' command. Yes, this command is a little heavy for the sysfs writes, but we can add command like 'sysset' or so for sysfs like:
>>
>> Okay, so this would be a sysfs file per card then? Sol we would have for example
>> 2 new sysfs files like this show up when your module is loaded:
>>
>> /sys/class/sounds/card0/spk_mute_led_control
>> /sys/class/sounds/card0/mic_mute_led_control
>>
>> And reading would iterate over all mixer-elements of the card and print
>> the names of those which have the relevant access LED flag set, where
>> as a write would be taken as a control-name to add the access LED flag
>> too?
> 
> It depends if we want to have this feature as an independent add-on
> (implemented in the generic sound LED module) or if we tie this more to
> the ALSA's core control code.
> 
> My proposal creates virtual sound ctl-led driver - thus
> /sysfs/devices/virtual/sound/ctl-led/ tree. We can add a subdirectory per card
> there like:
> 
> /sysfs/devices/virtual/sound/ctl-led/speaker/card0/attach
> 
> ...

Ok, I see.

>> And an empty write would be special and clear the flag on all controls?
>> I guess we don't strictly need that if we only set things up at boot once,
>> but it might still be handy to force things to a clean state.
> 
> The list operations should be probably identified by separate sysfs files.
> 
> /sysfs/devices/virtual/sound/ctl-led/speaker/card0/attach
> /sysfs/devices/virtual/sound/ctl-led/speaker/card0/detach
> /sysfs/devices/virtual/sound/ctl-led/speaker/card0/reset
> /sysfs/devices/virtual/sound/ctl-led/speaker/card0/controls

Ack, that would be better.

> And /sys/class/sounds/card0/controlC0/led-speaker may be a symlink to
> /sysfs/devices/virtual/sound/ctl-led/speaker/card0/ .
> 
>>>   # detach all speaker LED controls for card 1
>>>   # similar to 'echo -n "card=1,*" > /sysfs/devices/virtual/sound/ctl-led/speaker/detach'
>>>   sysset "devices/virtual/sound/ctl-led/speaker/detach:card=1,*"
>>>
>>>   # attach the 'Speaker Playback Switch',10 control to speaker LED trigger in card 1
>>>   # similar to 'echo -n "card=1,iface=MIXER,name='Speaker Playback Switch',index=10" > /sysfs/devices/virtual/sound/ctl-led/speaker/attach
>>>   sysset "devices/virtual/sound/ctl-led/speaker/attach:card=1,iface=MIXER,name='Speaker Playback Switch',index=10"
>>
>> I think a sysfs file per card would work better, that would certainly be
>> a lot more inline with how sysfs is normally used...
>>
>> Also do we need the iface=MIXER part ?
> 
> It was just a complete example (element ID specification in a string from
> alsa-lib/amixer). Of course, the other element types won't be probably used
> for the LED functionality...

Ok, the reason I was asking is because if possible we should having to
avoid to do string-parsing inside the kernel. So if the sysfs files
just take a control-name without any of the name=value pair stuff going
on that would be better.

> 
>>> Security: The LED-control bindings should be handled only in the boot / init phase (thus in UCM BootSequence section) and the sysfs interface files should be read-only for normal users.
>>
>> Yes that make sense, but it will require some extra helper to that, I guess it
>> could be an extra flag to the alsactl restore command which already gets run
>> at boot, or an extra alsactl command ?
> 
> The alsactl does the BootSequence initialization when UCM is supported in
> alsa-lib. But, we have a little issue that the this sequence is executed only
> if some controls are changed (added or removed) between last alsa state config
> (/var/lib/alsa/asound.state) or if the state for the given card does not exist
> to not override the values which may be changed by the user. It really depends
> on the control API only.
> 
> Apparently, we need another sequence which will be forcefully executed on
> boot. ColdSequence , FixedBootSequence, ForcedSequence, ForcedBootSequence ?

FixedBootSequence seems the best.

This does seem to have quickly grown into something somewhat complex though.

While all that is necessary for the 2 ASoC using devices with actual mute-LEDs
which I'm trying to get supported is 2 very simple static assignments of
the LED flag.  I've just completed some alsa-lib work to deal with the
sometimes quirky control-names and with that in place, the spk-mute-led
flag can be statically set on the access flags of the Speaker + HP output
mute controls as Mark suggested as an alternative.  That seems a lot more KISS
to me.

But I guess the flexibility this gives us will also be helpful for some HDA /
other SOF cases like the Dell privacy stuff which also involves tying a mute
control on an ASoC codec to a led-trigger ?

Regards,

Hans