[v4,2/2] ASoC: rt715:add micmute led state control supports

Commit Message

Yuan, Perry March 1, 2021, 9:38 a.m. UTC

From: Perry Yuan <[e-mail]>

Some new Dell system is going to support audio internal micphone
privacy setting from hardware level with micmute led state changing
When micmute hotkey pressed by user, soft mute will need to be enabled
firstly in case of pop noise, and codec driver need to react to mic
mute event to EC(embedded controller) notifying that SW mute is completed
Then EC will do the hardware mute physically within the timeout reached

This patch allow codec rt715 and rt715 sdca driver to change the local micmute
led state. Dell privacy led trigger driver will ack EC when micmute key pressed
through this micphone led control interface like hda_generic provided
ACPI method defined in dell-privacy micmute led trigger will be called
for notifying the EC that software mute has been completed, then hardware
audio circuit solution controlled by EC will switch the audio input source off/on

Signed-off-by: Perry Yuan <[e-mail]>

--------
v3 -> v4:
* remove unused debug log
* remove compile flag of DELL privacy
* move the micmute_led to local from rt715_priv
* when Jaroslav upstream his gerneric LED trigger driver,I will rebase
  this patch,please consider merge this at first
  https://lore.kernel.org/alsa-devel/[e-mail]/
v2 -> v3:
* simplify the patch to reuse some val value
* add more detail to the commit info
v1 -> v2:
* fix some format issue
--------
---
 sound/soc/codecs/rt715-sdca.c | 12 ++++++++++++
 sound/soc/codecs/rt715.c      | 12 ++++++++++++
 2 files changed, 24 insertions(+)

Comments

Pierre-Louis Bossart March 1, 2021, 2:30 p.m. UTC | #1

On 3/1/21 3:38 AM, Perry Yuan wrote:
> From: Perry Yuan <[e-mail]>
> 
> Some new Dell system is going to support audio internal micphone
> privacy setting from hardware level with micmute led state changing
> When micmute hotkey pressed by user, soft mute will need to be enabled
> firstly in case of pop noise, and codec driver need to react to mic
> mute event to EC(embedded controller) notifying that SW mute is completed
> Then EC will do the hardware mute physically within the timeout reached
> 
> This patch allow codec rt715 and rt715 sdca driver to change the local micmute
> led state. Dell privacy led trigger driver will ack EC when micmute key pressed
> through this micphone led control interface like hda_generic provided
> ACPI method defined in dell-privacy micmute led trigger will be called
> for notifying the EC that software mute has been completed, then hardware
> audio circuit solution controlled by EC will switch the audio input source off/on
> 
> Signed-off-by: Perry Yuan <[e-mail]>
> 
> --------
> v3 -> v4:
> * remove unused debug log
> * remove compile flag of DELL privacy
> * move the micmute_led to local from rt715_priv
> * when Jaroslav upstream his gerneric LED trigger driver,I will rebase
>    this patch,please consider merge this at first
>    https://lore.kernel.org/alsa-devel/[e-mail]/
> v2 -> v3:
> * simplify the patch to reuse some val value
> * add more detail to the commit info
> v1 -> v2:
> * fix some format issue
> --------
> ---
>   sound/soc/codecs/rt715-sdca.c | 12 ++++++++++++

that file is not yet in Mark Brown's tree, so that patch wouldn't apply.

>   sound/soc/codecs/rt715.c      | 12 ++++++++++++
>   2 files changed, 24 insertions(+)
> 
> diff --git a/sound/soc/codecs/rt715-sdca.c b/sound/soc/codecs/rt715-sdca.c
> index b43ac8559e45..816348ae11a1 100644
> --- a/sound/soc/codecs/rt715-sdca.c
> +++ b/sound/soc/codecs/rt715-sdca.c
> @@ -12,6 +12,7 @@
>   #include <linux/version.h>
>   #include <linux/kernel.h>
>   #include <linux/init.h>
> +#include <linux/leds.h>
>   #include <linux/pm_runtime.h>
>   #include <linux/pm.h>
>   #include <linux/soundwire/sdw.h>
> @@ -269,6 +270,7 @@ static int rt715_sdca_put_volsw(struct snd_kcontrol *kcontrol,
>   	unsigned int reg = mc->reg;
>   	unsigned int max = mc->max;
>   	int err;
> +	bool micmute_led;
>   
>   	val = ucontrol->value.integer.value[0];
>   	if (invert)
> @@ -287,6 +289,16 @@ static int rt715_sdca_put_volsw(struct snd_kcontrol *kcontrol,
>   			return err;
>   	}
>   
> +	/* Micmute LED state changed by muted/unmute switch */
> +	if (mc->invert) {
> +		if (ucontrol->value.integer.value[0] || ucontrol->value.integer.value[1]) {
> +			micmute_led = LED_OFF;
> +		} else {
> +			micmute_led = LED_ON;
> +		}
> +		ledtrig_audio_set(LED_AUDIO_MICMUTE, micmute_led);
> +	}
> +
>   	return 0;
>   }
>   
> diff --git a/sound/soc/codecs/rt715.c b/sound/soc/codecs/rt715.c
> index cdcba70146da..db2c0d2ff9d2 100644
> --- a/sound/soc/codecs/rt715.c
> +++ b/sound/soc/codecs/rt715.c
> @@ -13,6 +13,7 @@
>   #include <linux/init.h>
>   #include <linux/delay.h>
>   #include <linux/i2c.h>
> +#include <linux/leds.h>
>   #include <linux/pm_runtime.h>
>   #include <linux/pm.h>
>   #include <linux/soundwire/sdw.h>
> @@ -88,6 +89,7 @@ static int rt715_set_amp_gain_put(struct snd_kcontrol *kcontrol,
>   		RT715_SET_GAIN_MIX_ADC2_L};
>   	unsigned int addr_h, addr_l, val_h, val_ll, val_lr;
>   	unsigned int read_ll, read_rl, i, j, loop_cnt;
> +	bool micmute_led;
>   
>   	if (strstr(ucontrol->id.name, "Main Capture Switch") ||
>   		strstr(ucontrol->id.name, "Main Capture Volume"))
> @@ -95,6 +97,16 @@ static int rt715_set_amp_gain_put(struct snd_kcontrol *kcontrol,
>   	else
>   		loop_cnt = 1;
>   
> +	/* Micmute LED state changed by muted/unmute switch */
> +	if (mc->invert) {
> +		if (ucontrol->value.integer.value[0] || ucontrol->value.integer.value[1]) {
> +			micmute_led = LED_OFF;
> +		} else {
> +			micmute_led = LED_ON;
> +		}
> +		ledtrig_audio_set(LED_AUDIO_MICMUTE, micmute_led);
> +	}
> +
>   	for (j = 0; j < loop_cnt; j++) {
>   		/* Can't use update bit function, so read the original value first */
>   		if (loop_cnt == 1) {
>

Mark Brown March 8, 2021, 5:24 p.m. UTC | #2

On Mon, Mar 01, 2021 at 05:38:34PM +0800, Perry Yuan wrote:

> +	/* Micmute LED state changed by muted/unmute switch */
> +	if (mc->invert) {
> +		if (ucontrol->value.integer.value[0] || ucontrol->value.integer.value[1]) {
> +			micmute_led = LED_OFF;
> +		} else {
> +			micmute_led = LED_ON;
> +		}
> +		ledtrig_audio_set(LED_AUDIO_MICMUTE, micmute_led);
> +	}

These conditionals on inversion seem weird and counterintuitive.  If
we're going with this approach it would probably be clearer to define a
custom operation for the affected controls that wraps the standard one
and adds the LED setting rather than keying off invert like this.

Yuan, Perry March 22, 2021, 9:25 a.m. UTC | #3

Hi Mark:

> -----Original Message-----
> From: Mark Brown <[e-mail]>
> Sent: Tuesday, March 9, 2021 1:24 AM
> To: Yuan, Perry
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; Limonciello, Mario;
> [e-mail]; [e-mail]; linux-
> [e-mail]; [e-mail]
> Subject: Re: [PATCH v4 2/2] ASoC: rt715:add micmute led state control
> supports
> 
> On Mon, Mar 01, 2021 at 05:38:34PM +0800, Perry Yuan wrote:
> 
> > +	/* Micmute LED state changed by muted/unmute switch */
> > +	if (mc->invert) {
> > +		if (ucontrol->value.integer.value[0] || ucontrol-
> >value.integer.value[1]) {
> > +			micmute_led = LED_OFF;
> > +		} else {
> > +			micmute_led = LED_ON;
> > +		}
> > +		ledtrig_audio_set(LED_AUDIO_MICMUTE, micmute_led);
> > +	}
> 
> These conditionals on inversion seem weird and counterintuitive.  If we're
> going with this approach it would probably be clearer to define a custom
> operation for the affected controls that wraps the standard one and adds the
> LED setting rather than keying off invert like this.

Currently the sof soundwire driver has no generic led control yet.
This patch can handle the led control needs for MIC mute LED, definitely the patch is a short term solution.
There is a feature request discussion when we started to implement this solution.
https://github.com/thesofproject/linux/issues/2496#issuecomment-713892620

The workable way for now is that we put the LED mute control to the codec driver.
When there is new and full sound LED solution implemented, this part will be also optimized.
The Hardware privacy feature needs this patch to handle the Mic mute led state change.
Before that full solution ready in kernel, could we take this as short term solution?

Perry

Jaroslav Kysela March 22, 2021, 2:37 p.m. UTC | #4

Dne 22. 03. 21 v 10:25 Yuan, Perry napsal(a):
> Hi Mark:
> 
>> -----Original Message-----
>> From: Mark Brown <[e-mail]>
>> Sent: Tuesday, March 9, 2021 1:24 AM
>> To: Yuan, Perry
>> Cc: [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; Limonciello, Mario;
>> [e-mail]; [e-mail]; linux-
>> [e-mail]; [e-mail]
>> Subject: Re: [PATCH v4 2/2] ASoC: rt715:add micmute led state control
>> supports
>>
>> On Mon, Mar 01, 2021 at 05:38:34PM +0800, Perry Yuan wrote:
>>
>>> +	/* Micmute LED state changed by muted/unmute switch */
>>> +	if (mc->invert) {
>>> +		if (ucontrol->value.integer.value[0] || ucontrol-
>>> value.integer.value[1]) {
>>> +			micmute_led = LED_OFF;
>>> +		} else {
>>> +			micmute_led = LED_ON;
>>> +		}
>>> +		ledtrig_audio_set(LED_AUDIO_MICMUTE, micmute_led);
>>> +	}
>>
>> These conditionals on inversion seem weird and counterintuitive.  If we're
>> going with this approach it would probably be clearer to define a custom
>> operation for the affected controls that wraps the standard one and adds the
>> LED setting rather than keying off invert like this.
> 
> Currently the sof soundwire driver has no generic led control yet.
> This patch can handle the led control needs for MIC mute LED, definitely the patch is a short term solution.
> There is a feature request discussion when we started to implement this solution.
> https://github.com/thesofproject/linux/issues/2496#issuecomment-713892620
> 
> The workable way for now is that we put the LED mute control to the codec driver.
> When there is new and full sound LED solution implemented, this part will be also optimized.
> The Hardware privacy feature needs this patch to handle the Mic mute led state change.
> Before that full solution ready in kernel, could we take this as short term solution?

Perry, it's about the machine detection. Your code is too much generic even
for the top-level LED trigger implementation. We need an extra check, if the
proper LED's are really controlled on the specific hardware. Other hardware
may use RT715 for a different purpose. Use DMI / ACPI checks to detect this
hardware and don't misuse the inversion flag to enable this code.

						Jaroslav

Hans de Goede March 22, 2021, 3:01 p.m. UTC | #5

Hi,

On 3/22/21 3:37 PM, Jaroslav Kysela wrote:
> Dne 22. 03. 21 v 10:25 Yuan, Perry napsal(a):
>> Hi Mark:
>>
>>> -----Original Message-----
>>> From: Mark Brown <[e-mail]>
>>> Sent: Tuesday, March 9, 2021 1:24 AM
>>> To: Yuan, Perry
>>> Cc: [e-mail]; [e-mail];
>>> [e-mail]; [e-mail]; [e-mail];
>>> [e-mail]; [e-mail]; Limonciello, Mario;
>>> [e-mail]; [e-mail]; linux-
>>> [e-mail]; [e-mail]
>>> Subject: Re: [PATCH v4 2/2] ASoC: rt715:add micmute led state control
>>> supports
>>>
>>> On Mon, Mar 01, 2021 at 05:38:34PM +0800, Perry Yuan wrote:
>>>
>>>> +	/* Micmute LED state changed by muted/unmute switch */
>>>> +	if (mc->invert) {
>>>> +		if (ucontrol->value.integer.value[0] || ucontrol-
>>>> value.integer.value[1]) {
>>>> +			micmute_led = LED_OFF;
>>>> +		} else {
>>>> +			micmute_led = LED_ON;
>>>> +		}
>>>> +		ledtrig_audio_set(LED_AUDIO_MICMUTE, micmute_led);
>>>> +	}
>>>
>>> These conditionals on inversion seem weird and counterintuitive.  If we're
>>> going with this approach it would probably be clearer to define a custom
>>> operation for the affected controls that wraps the standard one and adds the
>>> LED setting rather than keying off invert like this.
>>
>> Currently the sof soundwire driver has no generic led control yet.
>> This patch can handle the led control needs for MIC mute LED, definitely the patch is a short term solution.
>> There is a feature request discussion when we started to implement this solution.
>> https://github.com/thesofproject/linux/issues/2496#issuecomment-713892620
>>
>> The workable way for now is that we put the LED mute control to the codec driver.
>> When there is new and full sound LED solution implemented, this part will be also optimized.
>> The Hardware privacy feature needs this patch to handle the Mic mute led state change.
>> Before that full solution ready in kernel, could we take this as short term solution?
> 
> Perry, it's about the machine detection. Your code is too much generic even
> for the top-level LED trigger implementation. We need an extra check, if the
> proper LED's are really controlled on the specific hardware. Other hardware
> may use RT715 for a different purpose. Use DMI / ACPI checks to detect this
> hardware and don't misuse the inversion flag to enable this code.

I think this would be a goo candidate for the new generic LED handling:

https://lore.kernel.org/alsa-devel/[e-mail]/

And then use a udev-rule + hwdb and/or UCM profiles to configure the LED trigger
for specific models from userspace ?

Regards,

Hans

Yuan, Perry March 25, 2021, 8:19 a.m. UTC | #6

Hi Jaroslav:

> -----Original Message-----
> From: Jaroslav Kysela <[e-mail]>
> Sent: Monday, March 22, 2021 10:38 PM
> To: Yuan, Perry; Mark Brown; [e-mail];
> Limonciello, Mario; [e-mail]
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; alsa-devel@alsa-
> project.org; [e-mail]; platform-driver-
> [e-mail]
> Subject: Re: [PATCH v4 2/2] ASoC: rt715:add micmute led state control
> supports
> 
> 
> [EXTERNAL EMAIL]
> 
> Dne 22. 03. 21 v 10:25 Yuan, Perry napsal(a):
> > Hi Mark:
> >
> >> -----Original Message-----
> >> From: Mark Brown <[e-mail]>
> >> Sent: Tuesday, March 9, 2021 1:24 AM
> >> To: Yuan, Perry
> >> Cc: [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]; Limonciello, Mario;
> >> [e-mail]; [e-mail]; linux-
> >> [e-mail]; [e-mail]
> >> Subject: Re: [PATCH v4 2/2] ASoC: rt715:add micmute led state control
> >> supports
> >>
> >> On Mon, Mar 01, 2021 at 05:38:34PM +0800, Perry Yuan wrote:
> >>
> >>> +	/* Micmute LED state changed by muted/unmute switch */
> >>> +	if (mc->invert) {
> >>> +		if (ucontrol->value.integer.value[0] || ucontrol-
> >>> value.integer.value[1]) {
> >>> +			micmute_led = LED_OFF;
> >>> +		} else {
> >>> +			micmute_led = LED_ON;
> >>> +		}
> >>> +		ledtrig_audio_set(LED_AUDIO_MICMUTE, micmute_led);
> >>> +	}
> >>
> >> These conditionals on inversion seem weird and counterintuitive.  If
> >> we're going with this approach it would probably be clearer to define
> >> a custom operation for the affected controls that wraps the standard
> >> one and adds the LED setting rather than keying off invert like this.
> >
> > Currently the sof soundwire driver has no generic led control yet.
> > This patch can handle the led control needs for MIC mute LED, definitely
> the patch is a short term solution.
> > There is a feature request discussion when we started to implement this
> solution.
> > https://github.com/thesofproject/linux/issues/2496#issuecomment-
> 713892
> > 620
> >
> > The workable way for now is that we put the LED mute control to the
> codec driver.
> > When there is new and full sound LED solution implemented, this part
> will be also optimized.
> > The Hardware privacy feature needs this patch to handle the Mic mute
> led state change.
> > Before that full solution ready in kernel, could we take this as short term
> solution?
> 
> Perry, it's about the machine detection. Your code is too much generic even
> for the top-level LED trigger implementation. We need an extra check, if the
> proper LED's are really controlled on the specific hardware. Other hardware
> may use RT715 for a different purpose. Use DMI / ACPI checks to detect this
> hardware and don't misuse the inversion flag to enable this code.
> 
> 						Jaroslav
> 
> --
> Jaroslav Kysela <[e-mail]>
> Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

In the V2 patch, I have added the machine detection, but some guys thought that I should remove the detection for it is harmless to other system
So I remove it in the following patches.

Is it Ok for you if I add below detection of Dell system which enable the privacy feature ?
 
Then the mute led control will be called normally and Mic mute will be successfully configured.
There is no any impaction to other systems.


+#if IS_ENABLED(CONFIG_DELL_PRIVACY) 
.....
+#endif

Jaroslav Kysela March 25, 2021, 9:37 a.m. UTC | #7

Dne 25. 03. 21 v 9:19 Yuan, Perry napsal(a):
> Hi Jaroslav:
> 
>> -----Original Message-----
>> From: Jaroslav Kysela <[e-mail]>
>> Sent: Monday, March 22, 2021 10:38 PM
>> To: Yuan, Perry; Mark Brown; [e-mail];
>> Limonciello, Mario; [e-mail]
>> Cc: [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; alsa-devel@alsa-
>> project.org; [e-mail]; platform-driver-
>> [e-mail]
>> Subject: Re: [PATCH v4 2/2] ASoC: rt715:add micmute led state control
>> supports
>>
>>
>> [EXTERNAL EMAIL]
>>
>> Dne 22. 03. 21 v 10:25 Yuan, Perry napsal(a):
>>> Hi Mark:
>>>
>>>> -----Original Message-----
>>>> From: Mark Brown <[e-mail]>
>>>> Sent: Tuesday, March 9, 2021 1:24 AM
>>>> To: Yuan, Perry
>>>> Cc: [e-mail]; [e-mail];
>>>> [e-mail]; [e-mail]; [e-mail];
>>>> [e-mail]; [e-mail]; Limonciello, Mario;
>>>> [e-mail]; [e-mail]; linux-
>>>> [e-mail]; [e-mail]
>>>> Subject: Re: [PATCH v4 2/2] ASoC: rt715:add micmute led state control
>>>> supports
>>>>
>>>> On Mon, Mar 01, 2021 at 05:38:34PM +0800, Perry Yuan wrote:
>>>>
>>>>> +	/* Micmute LED state changed by muted/unmute switch */
>>>>> +	if (mc->invert) {
>>>>> +		if (ucontrol->value.integer.value[0] || ucontrol-
>>>>> value.integer.value[1]) {
>>>>> +			micmute_led = LED_OFF;
>>>>> +		} else {
>>>>> +			micmute_led = LED_ON;
>>>>> +		}
>>>>> +		ledtrig_audio_set(LED_AUDIO_MICMUTE, micmute_led);
>>>>> +	}
>>>>
>>>> These conditionals on inversion seem weird and counterintuitive.  If
>>>> we're going with this approach it would probably be clearer to define
>>>> a custom operation for the affected controls that wraps the standard
>>>> one and adds the LED setting rather than keying off invert like this.
>>>
>>> Currently the sof soundwire driver has no generic led control yet.
>>> This patch can handle the led control needs for MIC mute LED, definitely
>> the patch is a short term solution.
>>> There is a feature request discussion when we started to implement this
>> solution.
>>> https://github.com/thesofproject/linux/issues/2496#issuecomment-
>> 713892
>>> 620
>>>
>>> The workable way for now is that we put the LED mute control to the
>> codec driver.
>>> When there is new and full sound LED solution implemented, this part
>> will be also optimized.
>>> The Hardware privacy feature needs this patch to handle the Mic mute
>> led state change.
>>> Before that full solution ready in kernel, could we take this as short term
>> solution?
>>
>> Perry, it's about the machine detection. Your code is too much generic even
>> for the top-level LED trigger implementation. We need an extra check, if the
>> proper LED's are really controlled on the specific hardware. Other hardware
>> may use RT715 for a different purpose. Use DMI / ACPI checks to detect this
>> hardware and don't misuse the inversion flag to enable this code.
>>
>> 						Jaroslav
>>
>> --
>> Jaroslav Kysela <[e-mail]>
>> Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
> 
> In the V2 patch, I have added the machine detection, but some guys thought that I should remove the detection for it is harmless to other system
> So I remove it in the following patches.
> 
> Is it Ok for you if I add below detection of Dell system which enable the privacy feature ?
>  
> Then the mute led control will be called normally and Mic mute will be successfully configured.
> There is no any impaction to other systems.
> 
> 
> +#if IS_ENABLED(CONFIG_DELL_PRIVACY) 
> .....
> +#endif

I think that you misunderstood the request. It's about the runtime detection
not about the compile time detection. An example for DMI check is in
sound/soc/intel/boards/sof_sdw.c / sof_sdw_quirk_table .

					Jaroslav

Yuan, Perry March 25, 2021, 2:11 p.m. UTC | #8

Hi Hans

> -----Original Message-----
> From: Hans de Goede <[e-mail]>
> Sent: Monday, March 22, 2021 11:02 PM
> To: Jaroslav Kysela; Yuan, Perry; Mark Brown; pierre-
> [e-mail]; Limonciello, Mario
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]
> Subject: Re: [PATCH v4 2/2] ASoC: rt715:add micmute led state control
> supports
> 
> 
> [EXTERNAL EMAIL]
> 
> Hi,
> 
> On 3/22/21 3:37 PM, Jaroslav Kysela wrote:
> > Dne 22. 03. 21 v 10:25 Yuan, Perry napsal(a):
> >> Hi Mark:
> >>
> >>> -----Original Message-----
> >>> From: Mark Brown <[e-mail]>
> >>> Sent: Tuesday, March 9, 2021 1:24 AM
> >>> To: Yuan, Perry
> >>> Cc: [e-mail]; [e-mail];
> >>> [e-mail]; [e-mail]; [e-mail];
> >>> [e-mail]; [e-mail]; Limonciello, Mario;
> >>> [e-mail]; [e-mail]; linux-
> >>> [e-mail]; [e-mail]
> >>> Subject: Re: [PATCH v4 2/2] ASoC: rt715:add micmute led state
> >>> control supports
> >>>
> >>> On Mon, Mar 01, 2021 at 05:38:34PM +0800, Perry Yuan wrote:
> >>>
> >>>> +	/* Micmute LED state changed by muted/unmute switch */
> >>>> +	if (mc->invert) {
> >>>> +		if (ucontrol->value.integer.value[0] || ucontrol-
> >>>> value.integer.value[1]) {
> >>>> +			micmute_led = LED_OFF;
> >>>> +		} else {
> >>>> +			micmute_led = LED_ON;
> >>>> +		}
> >>>> +		ledtrig_audio_set(LED_AUDIO_MICMUTE, micmute_led);
> >>>> +	}
> >>>
> >>> These conditionals on inversion seem weird and counterintuitive.  If
> >>> we're going with this approach it would probably be clearer to
> >>> define a custom operation for the affected controls that wraps the
> >>> standard one and adds the LED setting rather than keying off invert like
> this.
> >>
> >> Currently the sof soundwire driver has no generic led control yet.
> >> This patch can handle the led control needs for MIC mute LED, definitely
> the patch is a short term solution.
> >> There is a feature request discussion when we started to implement this
> solution.
> >> https://github.com/thesofproject/linux/issues/2496#issuecomment-
> 71389
> >> 2620
> >>
> >> The workable way for now is that we put the LED mute control to the
> codec driver.
> >> When there is new and full sound LED solution implemented, this part will
> be also optimized.
> >> The Hardware privacy feature needs this patch to handle the Mic mute led
> state change.
> >> Before that full solution ready in kernel, could we take this as short term
> solution?
> >
> > Perry, it's about the machine detection. Your code is too much generic
> > even for the top-level LED trigger implementation. We need an extra
> > check, if the proper LED's are really controlled on the specific
> > hardware. Other hardware may use RT715 for a different purpose. Use
> > DMI / ACPI checks to detect this hardware and don't misuse the inversion
> flag to enable this code.
> 
> I think this would be a goo candidate for the new generic LED handling:
> 
> https://lore.kernel.org/alsa-devel/20210317172945.842280-1-
> [e-mail]/
> 
> And then use a udev-rule + hwdb and/or UCM profiles to configure the LED
> trigger for specific models from userspace ?
> 
> Regards,
> 
> Hans
> 
> 
> 
Because the SOF SDW design has no mic mute led control yet.
So I add one short term solution to make Dell privacy working for now 
Definitely , that is new solution I can add my patch on that to test as one user case .
We really need to take the short term solution work for some system which support new SOF soundwire hardware which have  dependence on the MIC mute feature
Meanwhile we can continue working on the new  "udev-rule + hwdb and/or UCM profiles" solution as to replace this one.
If we agree that, I will submit another V6 addressing new feedback. 

Perry

Hans de Goede March 25, 2021, 3:07 p.m. UTC | #9

Hi,

On 3/25/21 3:11 PM, Yuan, Perry wrote:
> Hi Hans
> 
>> -----Original Message-----
>> From: Hans de Goede <[e-mail]>
>> Sent: Monday, March 22, 2021 11:02 PM
>> To: Jaroslav Kysela; Yuan, Perry; Mark Brown; pierre-
>> [e-mail]; Limonciello, Mario
>> Cc: [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail]
>> Subject: Re: [PATCH v4 2/2] ASoC: rt715:add micmute led state control
>> supports
>>
>>
>> [EXTERNAL EMAIL]
>>
>> Hi,
>>
>> On 3/22/21 3:37 PM, Jaroslav Kysela wrote:
>>> Dne 22. 03. 21 v 10:25 Yuan, Perry napsal(a):
>>>> Hi Mark:
>>>>
>>>>> -----Original Message-----
>>>>> From: Mark Brown <[e-mail]>
>>>>> Sent: Tuesday, March 9, 2021 1:24 AM
>>>>> To: Yuan, Perry
>>>>> Cc: [e-mail]; [e-mail];
>>>>> [e-mail]; [e-mail]; [e-mail];
>>>>> [e-mail]; [e-mail]; Limonciello, Mario;
>>>>> [e-mail]; [e-mail]; linux-
>>>>> [e-mail]; [e-mail]
>>>>> Subject: Re: [PATCH v4 2/2] ASoC: rt715:add micmute led state
>>>>> control supports
>>>>>
>>>>> On Mon, Mar 01, 2021 at 05:38:34PM +0800, Perry Yuan wrote:
>>>>>
>>>>>> +	/* Micmute LED state changed by muted/unmute switch */
>>>>>> +	if (mc->invert) {
>>>>>> +		if (ucontrol->value.integer.value[0] || ucontrol-
>>>>>> value.integer.value[1]) {
>>>>>> +			micmute_led = LED_OFF;
>>>>>> +		} else {
>>>>>> +			micmute_led = LED_ON;
>>>>>> +		}
>>>>>> +		ledtrig_audio_set(LED_AUDIO_MICMUTE, micmute_led);
>>>>>> +	}
>>>>>
>>>>> These conditionals on inversion seem weird and counterintuitive.  If
>>>>> we're going with this approach it would probably be clearer to
>>>>> define a custom operation for the affected controls that wraps the
>>>>> standard one and adds the LED setting rather than keying off invert like
>> this.
>>>>
>>>> Currently the sof soundwire driver has no generic led control yet.
>>>> This patch can handle the led control needs for MIC mute LED, definitely
>> the patch is a short term solution.
>>>> There is a feature request discussion when we started to implement this
>> solution.
>>>> https://github.com/thesofproject/linux/issues/2496#issuecomment-
>> 71389
>>>> 2620
>>>>
>>>> The workable way for now is that we put the LED mute control to the
>> codec driver.
>>>> When there is new and full sound LED solution implemented, this part will
>> be also optimized.
>>>> The Hardware privacy feature needs this patch to handle the Mic mute led
>> state change.
>>>> Before that full solution ready in kernel, could we take this as short term
>> solution?
>>>
>>> Perry, it's about the machine detection. Your code is too much generic
>>> even for the top-level LED trigger implementation. We need an extra
>>> check, if the proper LED's are really controlled on the specific
>>> hardware. Other hardware may use RT715 for a different purpose. Use
>>> DMI / ACPI checks to detect this hardware and don't misuse the inversion
>> flag to enable this code.
>>
>> I think this would be a goo candidate for the new generic LED handling:
>>
>> https://lore.kernel.org/alsa-devel/20210317172945.842280-1-
>> [e-mail]/
>>
>> And then use a udev-rule + hwdb and/or UCM profiles to configure the LED
>> trigger for specific models from userspace ?
>>
>> Regards,
>>
>> Hans
>>
>>
>>
> Because the SOF SDW design has no mic mute led control yet.
> So I add one short term solution to make Dell privacy working for now 
> Definitely , that is new solution I can add my patch on that to test as one user case .
> We really need to take the short term solution work for some system which support new SOF soundwire hardware which have  dependence on the MIC mute feature
> Meanwhile we can continue working on the new  "udev-rule + hwdb and/or UCM profiles" solution as to replace this one.
> If we agree that, I will submit another V6 addressing new feedback. 

The triggering of the LED trigger and the code registering the led_classdev
are in 2 separate subsystems; and should be merged separately.

If you post a new version of patch 1/2 addressing my review remarks then
I can merge that.

For merging the sound side of this you need to talk to the sound folks
(Jaroslav Kysela, Takashi Iwai, Mark Brown for files under sound/soc).

Regards,

Hans

Yuan, Perry March 26, 2021, 7:20 a.m. UTC | #10

Hi Hans,Jaroslav


> -----Original Message-----
> From: Hans de Goede <[e-mail]>
> Sent: 2021年3月25日 23:07
> To: Yuan, Perry; Jaroslav Kysela; Mark Brown; pierre-
> [e-mail]; Limonciello, Mario
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]
> Subject: Re: [PATCH v4 2/2] ASoC: rt715:add micmute led state control
> supports
> 
> 
> [EXTERNAL EMAIL]
> 
> Hi,
> 
> On 3/25/21 3:11 PM, Yuan, Perry wrote:
> > Hi Hans
> >
> >> -----Original Message-----
> >> From: Hans de Goede <[e-mail]>
> >> Sent: Monday, March 22, 2021 11:02 PM
> >> To: Jaroslav Kysela; Yuan, Perry; Mark Brown; pierre-
> >> [e-mail]; Limonciello, Mario
> >> Cc: [e-mail]; [e-mail]; [e-mail];
> >> [e-mail]; [e-mail];
> >> [e-mail]; [e-mail];
> >> [e-mail]
> >> Subject: Re: [PATCH v4 2/2] ASoC: rt715:add micmute led state control
> >> supports
> >>
> >>
> >> [EXTERNAL EMAIL]
> >>
> >> Hi,
> >>
> >> On 3/22/21 3:37 PM, Jaroslav Kysela wrote:
> >>> Dne 22. 03. 21 v 10:25 Yuan, Perry napsal(a):
> >>>> Hi Mark:
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: Mark Brown <[e-mail]>
> >>>>> Sent: Tuesday, March 9, 2021 1:24 AM
> >>>>> To: Yuan, Perry
> >>>>> Cc: [e-mail]; [e-mail];
> >>>>> [e-mail]; [e-mail]; [e-mail];
> >>>>> [e-mail]; [e-mail]; Limonciello, Mario;
> >>>>> [e-mail]; [e-mail]; linux-
> >>>>> [e-mail]; [e-mail]
> >>>>> Subject: Re: [PATCH v4 2/2] ASoC: rt715:add micmute led state
> >>>>> control supports
> >>>>>
> >>>>> On Mon, Mar 01, 2021 at 05:38:34PM +0800, Perry Yuan wrote:
> >>>>>
> >>>>>> +	/* Micmute LED state changed by muted/unmute switch */
> >>>>>> +	if (mc->invert) {
> >>>>>> +		if (ucontrol->value.integer.value[0] || ucontrol-
> >>>>>> value.integer.value[1]) {
> >>>>>> +			micmute_led = LED_OFF;
> >>>>>> +		} else {
> >>>>>> +			micmute_led = LED_ON;
> >>>>>> +		}
> >>>>>> +		ledtrig_audio_set(LED_AUDIO_MICMUTE,
> micmute_led);
> >>>>>> +	}
> >>>>>
> >>>>> These conditionals on inversion seem weird and counterintuitive.
> >>>>> If we're going with this approach it would probably be clearer to
> >>>>> define a custom operation for the affected controls that wraps the
> >>>>> standard one and adds the LED setting rather than keying off
> >>>>> invert like
> >> this.
> >>>>
> >>>> Currently the sof soundwire driver has no generic led control yet.
> >>>> This patch can handle the led control needs for MIC mute LED,
> >>>> definitely
> >> the patch is a short term solution.
> >>>> There is a feature request discussion when we started to implement
> >>>> this
> >> solution.
> >>>> https://github.com/thesofproject/linux/issues/2496#issuecomment-
> >> 71389
> >>>> 2620
> >>>>
> >>>> The workable way for now is that we put the LED mute control to the
> >> codec driver.
> >>>> When there is new and full sound LED solution implemented, this
> >>>> part will
> >> be also optimized.
> >>>> The Hardware privacy feature needs this patch to handle the Mic
> >>>> mute led
> >> state change.
> >>>> Before that full solution ready in kernel, could we take this as
> >>>> short term
> >> solution?
> >>>
> >>> Perry, it's about the machine detection. Your code is too much
> >>> generic even for the top-level LED trigger implementation. We need
> >>> an extra check, if the proper LED's are really controlled on the
> >>> specific hardware. Other hardware may use RT715 for a different
> >>> purpose. Use DMI / ACPI checks to detect this hardware and don't
> >>> misuse the inversion
> >> flag to enable this code.
> >>
> >> I think this would be a goo candidate for the new generic LED handling:
> >>
> >> https://lore.kernel.org/alsa-devel/20210317172945.842280-1-
> >> [e-mail]/
> >>
> >> And then use a udev-rule + hwdb and/or UCM profiles to configure the
> >> LED trigger for specific models from userspace ?
> >>
> >> Regards,
> >>
> >> Hans
> >>
> >>
> >>
> > Because the SOF SDW design has no mic mute led control yet.
> > So I add one short term solution to make Dell privacy working for now
> > Definitely , that is new solution I can add my patch on that to test as one
> user case .
> > We really need to take the short term solution work for some system
> > which support new SOF soundwire hardware which have  dependence on
> the MIC mute feature Meanwhile we can continue working on the new  "udev-
> rule + hwdb and/or UCM profiles" solution as to replace this one.
> > If we agree that, I will submit another V6 addressing new feedback.
> 
> The triggering of the LED trigger and the code registering the led_classdev are
> in 2 separate subsystems; and should be merged separately.
> 
> If you post a new version of patch 1/2 addressing my review remarks then I
> can merge that.
> 
> For merging the sound side of this you need to talk to the sound folks
> (Jaroslav Kysela, Takashi Iwai, Mark Brown for files under sound/soc).
> 
> Regards,
> 
> Hans

Got it!
I am working the V6 today, will post it ASAP.
* addressing Hans Feedback 
* add runtime machine detection with DMI checking (from Jaroslav Feedback)

Perry  Yuan
Dell | Client Software Group | CDC Linux OS

Patch

diff --git a/sound/soc/codecs/rt715-sdca.c b/sound/soc/codecs/rt715-sdca.c
index b43ac8559e45..816348ae11a1 100644
--- a/sound/soc/codecs/rt715-sdca.c
+++ b/sound/soc/codecs/rt715-sdca.c
@@ -12,6 +12,7 @@ 
 #include <linux/version.h>
 #include <linux/kernel.h>
 #include <linux/init.h>
+#include <linux/leds.h>
 #include <linux/pm_runtime.h>
 #include <linux/pm.h>
 #include <linux/soundwire/sdw.h>
@@ -269,6 +270,7 @@  static int rt715_sdca_put_volsw(struct snd_kcontrol *kcontrol,
 	unsigned int reg = mc->reg;
 	unsigned int max = mc->max;
 	int err;
+	bool micmute_led;
 
 	val = ucontrol->value.integer.value[0];
 	if (invert)
@@ -287,6 +289,16 @@  static int rt715_sdca_put_volsw(struct snd_kcontrol *kcontrol,
 			return err;
 	}
 
+	/* Micmute LED state changed by muted/unmute switch */
+	if (mc->invert) {
+		if (ucontrol->value.integer.value[0] || ucontrol->value.integer.value[1]) {
+			micmute_led = LED_OFF;
+		} else {
+			micmute_led = LED_ON;
+		}
+		ledtrig_audio_set(LED_AUDIO_MICMUTE, micmute_led);
+	}
+
 	return 0;
 }
 
diff --git a/sound/soc/codecs/rt715.c b/sound/soc/codecs/rt715.c
index cdcba70146da..db2c0d2ff9d2 100644
--- a/sound/soc/codecs/rt715.c
+++ b/sound/soc/codecs/rt715.c
@@ -13,6 +13,7 @@ 
 #include <linux/init.h>
 #include <linux/delay.h>
 #include <linux/i2c.h>
+#include <linux/leds.h>
 #include <linux/pm_runtime.h>
 #include <linux/pm.h>
 #include <linux/soundwire/sdw.h>
@@ -88,6 +89,7 @@  static int rt715_set_amp_gain_put(struct snd_kcontrol *kcontrol,
 		RT715_SET_GAIN_MIX_ADC2_L};
 	unsigned int addr_h, addr_l, val_h, val_ll, val_lr;
 	unsigned int read_ll, read_rl, i, j, loop_cnt;
+	bool micmute_led;
 
 	if (strstr(ucontrol->id.name, "Main Capture Switch") ||
 		strstr(ucontrol->id.name, "Main Capture Volume"))
@@ -95,6 +97,16 @@  static int rt715_set_amp_gain_put(struct snd_kcontrol *kcontrol,
 	else
 		loop_cnt = 1;
 
+	/* Micmute LED state changed by muted/unmute switch */
+	if (mc->invert) {
+		if (ucontrol->value.integer.value[0] || ucontrol->value.integer.value[1]) {
+			micmute_led = LED_OFF;
+		} else {
+			micmute_led = LED_ON;
+		}
+		ledtrig_audio_set(LED_AUDIO_MICMUTE, micmute_led);
+	}
+
 	for (j = 0; j < loop_cnt; j++) {
 		/* Can't use update bit function, so read the original value first */
 		if (loop_cnt == 1) {