diff mbox series

ASoC: rt715:add Mic Mute LED control support

Message ID 20201103125859.8759-1-Perry_Yuan@Dell.com (mailing list archive)
State New, archived
Headers show
Series ASoC: rt715:add Mic Mute LED control support | expand

Commit Message

Yuan, Perry Nov. 3, 2020, 12:58 p.m. UTC 
From: perry_yuan <perry_yuan@dell.com>

Some new Dell system is going to support audio internal micphone
privacy setting from hardware level with micmute led state changing

This patch allow to change micmute led state through this micphone
led control interface like hda_generic provided.

Signed-off-by: Perry Yuan  <perry_yuan@dell.com>
Signed-off-by: Limonciello Mario <mario_limonciello@dell.com>
---
 sound/soc/codecs/rt715.c | 43 ++++++++++++++++++++++++++++++++++++++++
 sound/soc/codecs/rt715.h |  1 +
 2 files changed, 44 insertions(+)

Comments

Limonciello, Mario Nov. 3, 2020, 5:51 p.m. UTC | #1 
> -----Original Message-----
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Sent: Tuesday, November 3, 2020 10:13
> To: Mark Brown; Yuan, Perry
> Cc: oder_chiou@realtek.com; alsa-devel@alsa-project.org; lgirdwood@gmail.com;
> Limonciello, Mario; linux-kernel@vger.kernel.org; tiwai@suse.com
> Subject: Re: [PATCH] ASoC: rt715:add Mic Mute LED control support
> 
> 
> [EXTERNAL EMAIL]
> 
> Somehow this patch was filtered by alsa-devel servers?
> 
> On 11/3/20 7:12 AM, Mark Brown wrote:
> > On Tue, Nov 03, 2020 at 04:58:59AM -0800, Perry Yuan wrote:
> >> From: perry_yuan <perry_yuan@dell.com>
> >>
> >> Some new Dell system is going to support audio internal micphone
> >> privacy setting from hardware level with micmute led state changing
> >>
> >> This patch allow to change micmute led state through this micphone
> >> led control interface like hda_generic provided.
> >
> > If this is useful it should be done at the subsystem level rather than
> > open coded in a specific CODEC driver, however I don't undersand why it
> > is.
> >
> >> +static int rt715_micmute_led_mode_put(struct snd_kcontrol *kcontrol,
> >> +      struct snd_ctl_elem_value *ucontrol)
> >> +{
> >> +    struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
> >> +    struct rt715_priv *rt715 = snd_soc_component_get_drvdata(component);
> >> +    int led_mode = ucontrol->value.integer.value[0];
> >> +
> >> +    rt715->micmute_led = led_mode;
> >> +#if IS_ENABLED(CONFIG_LEDS_TRIGGER_AUDIO)
> >> +    ledtrig_audio_set(LED_AUDIO_MICMUTE,
> >> +               rt715->micmute_led ? LED_ON : LED_OFF);
> >> +#endif
> >> +  return 0;
> >> +}
> >
> > This is just adding a userspace API to set a LED via the standard LED
> > APIs.  Since the LED subsystem already has a perfectly good userspace
> > API why not use that?  There is no visible value in this being in the
> > sound subsystem.
> 
> I also don't quite follow. This looks as inspired from HDaudio code, but
> with a lot of simplifications.
> 
> If the intent was that when userspace decides to mute the LED is turned
> on, wouldn't it be enough to just track the state of a 'capture switch'
> responsible for mute, i.e. when the capture Switch is 'off' the LED is
> on. I don't see the point of having a new control, you would be adding
> more work for PulseAudio/UCM whereas connecting the capture switch to a
> led comes with zero work in userspace. See e.g. how the mute mic LED was
> handled in the SOF code handling DMICs, we didn't add a new control but
> turned the LED in the switch .put callback, see
> 
> https://elixir.bootlin.com/linux/latest/source/sound/soc/sof/control.c#L18
> 
> https://elixir.bootlin.com/linux/latest/source/sound/soc/sof/control.c#L153
> 
> Actually thinking more about it, having two controls for 'mute LED' and
> 'capture switch' could lead to inconsistent states where the LED is on
> without mute being activated. we should really bolt the LED activation
> to the capture switch, that way the mute and LED states are aligned.
> 

After giving it some thought I agree.  The UCM change that was opened
here https://github.com/alsa-project/alsa-ucm-conf/pull/60 wouldn't
be necessary at all if you just track capture switch directly like SOF does.
Mark Brown Nov. 3, 2020, 5:59 p.m. UTC | #2 
On Tue, Nov 03, 2020 at 10:13:03AM -0600, Pierre-Louis Bossart wrote:
> Somehow this patch was filtered by alsa-devel servers?

It'll be a post by a non-subscriber I guess, in which case it will
appear later.

> Actually thinking more about it, having two controls for 'mute LED' and
> 'capture switch' could lead to inconsistent states where the LED is on
> without mute being activated.  we should really bolt the LED activation to
> the capture switch, that way the mute and LED states are aligned.

Yeah, it's just asking for trouble and seems to defeat the point of
having the LED in the first place - aside from the general issues with
it being software controlled it'll require specific userspace support to
set it.  Users won't be able to trust that the LED state accurately
reflects if they're muted or not.  Your proposal is more what I'd expect
here, I'm not sure we can do much better with something software
controllable.
Limonciello, Mario Nov. 3, 2020, 6:04 p.m. UTC | #3 
> -----Original Message-----
> From: Mark Brown <broonie@kernel.org>
> Sent: Tuesday, November 3, 2020 12:00
> To: Pierre-Louis Bossart
> Cc: Yuan, Perry; oder_chiou@realtek.com; alsa-devel@alsa-project.org;
> lgirdwood@gmail.com; Limonciello, Mario; linux-kernel@vger.kernel.org;
> tiwai@suse.com
> Subject: Re: [PATCH] ASoC: rt715:add Mic Mute LED control support
> 
> On Tue, Nov 03, 2020 at 10:13:03AM -0600, Pierre-Louis Bossart wrote:
> > Somehow this patch was filtered by alsa-devel servers?
> 
> It'll be a post by a non-subscriber I guess, in which case it will
> appear later.
> 
> > Actually thinking more about it, having two controls for 'mute LED' and
> > 'capture switch' could lead to inconsistent states where the LED is on
> > without mute being activated.  we should really bolt the LED activation to
> > the capture switch, that way the mute and LED states are aligned.
> 
> Yeah, it's just asking for trouble and seems to defeat the point of
> having the LED in the first place - aside from the general issues with
> it being software controlled it'll require specific userspace support to
> set it.  Users won't be able to trust that the LED state accurately
> reflects if they're muted or not.  Your proposal is more what I'd expect
> here, I'm not sure we can do much better with something software
> controllable.

I don't think it came through in the commit message, but I wanted to mention
in the system that prompted this software does not control the LED.  The LED
is actually controlled by hardware, but has circuitry to delay the hardware
mute until software mute is complete to avoid any "popping noises".

This patch along with the platform/x86 patch:
https://patchwork.kernel.org/project/platform-driver-x86/patch/20201103125542.8572-1-Perry_Yuan@Dell.com/
complete that loop.

The flow is:
User presses mute key, dell-wmi receives event, passes to dell-privacy-wmi.
This emits to userspace as KEY_MICMUTE.  Userspace processes it and via UCM
switches get toggled.  The codec driver (or subsystem perhaps) will use LED
trigger to notify to change LED.  This gets picked up by dell-privacy-acpi.

dell-privacy-acpi doesn't actually change LED, but notifies that SW mute was
done.

If none of that flow was used the LED and mute function still work, but there
might be the popping noise.
Mark Brown Nov. 3, 2020, 6:31 p.m. UTC | #4 
On Tue, Nov 03, 2020 at 06:04:49PM +0000, Limonciello, Mario wrote:

> I don't think it came through in the commit message, but I wanted to mention
> in the system that prompted this software does not control the LED.  The LED
> is actually controlled by hardware, but has circuitry to delay the hardware
> mute until software mute is complete to avoid any "popping noises".

Ah, this doesn't correspond to the description at all.

> The flow is:
> User presses mute key, dell-wmi receives event, passes to dell-privacy-wmi.
> This emits to userspace as KEY_MICMUTE.  Userspace processes it and via UCM
> switches get toggled.  The codec driver (or subsystem perhaps) will use LED
> trigger to notify to change LED.  This gets picked up by dell-privacy-acpi.

> dell-privacy-acpi doesn't actually change LED, but notifies that SW mute was
> done.

> If none of that flow was used the LED and mute function still work, but there
> might be the popping noise.

With a timeout so that if things get lost somewhere then the mute button
is still functional, or can userspace block mute?  Also what happens if
userspace tries to set the state without having done anything about
muting, will it trigger the hardware level mute as though the key had
been pressed?
Pierre-Louis Bossart Nov. 3, 2020, 6:35 p.m. UTC | #5 
> I don't think it came through in the commit message, but I wanted to mention
> in the system that prompted this software does not control the LED.  The LED
> is actually controlled by hardware, but has circuitry to delay the hardware
> mute until software mute is complete to avoid any "popping noises".
> 
> This patch along with the platform/x86 patch:
> https://patchwork.kernel.org/project/platform-driver-x86/patch/20201103125542.8572-1-Perry_Yuan@Dell.com/
> complete that loop.
> 
> The flow is:
> User presses mute key, dell-wmi receives event, passes to dell-privacy-wmi.
> This emits to userspace as KEY_MICMUTE.  Userspace processes it and via UCM
> switches get toggled.  The codec driver (or subsystem perhaps) will use LED
> trigger to notify to change LED.  This gets picked up by dell-privacy-acpi.
> 
> dell-privacy-acpi doesn't actually change LED, but notifies that SW mute was
> done.
> 
> If none of that flow was used the LED and mute function still work, but there
> might be the popping noise.

Side note that the existing UCM config for RT715 does not do what I 
suggested, it seems we are using an incorrect configuration for 
CaptureSwitch and CaptureVolume:

CaptureSwitch "PGA5.0 5 Master Capture Switch"
	      CaptureVolume "PGA5.0 5 Master Capture Volume"		 
CaptureVolume "PGA5.0 5 Master Capture Volume"

That should be an RT715 control, not an SOF one. This was brought to our 
attention this morning. Probably a copy-paste from the DMIC case, likely 
needs to be changed for both RT715 and RT715-sdca cases.

https://github.com/thesofproject/linux/issues/2544#issuecomment-721231103
Limonciello, Mario Nov. 3, 2020, 7:01 p.m. UTC | #6 
> -----Original Message-----
> From: Mark Brown <broonie@kernel.org>
> Sent: Tuesday, November 3, 2020 12:32
> To: Limonciello, Mario
> Cc: Pierre-Louis Bossart; Yuan, Perry; oder_chiou@realtek.com; alsa-
> devel@alsa-project.org; lgirdwood@gmail.com; linux-kernel@vger.kernel.org;
> tiwai@suse.com
> Subject: Re: [PATCH] ASoC: rt715:add Mic Mute LED control support
> 
> On Tue, Nov 03, 2020 at 06:04:49PM +0000, Limonciello, Mario wrote:
> 
> > I don't think it came through in the commit message, but I wanted to mention
> > in the system that prompted this software does not control the LED.  The LED
> > is actually controlled by hardware, but has circuitry to delay the hardware
> > mute until software mute is complete to avoid any "popping noises".
> 
> Ah, this doesn't correspond to the description at all.
> 
> > The flow is:
> > User presses mute key, dell-wmi receives event, passes to dell-privacy-wmi.
> > This emits to userspace as KEY_MICMUTE.  Userspace processes it and via UCM
> > switches get toggled.  The codec driver (or subsystem perhaps) will use LED
> > trigger to notify to change LED.  This gets picked up by dell-privacy-acpi.
> 
> > dell-privacy-acpi doesn't actually change LED, but notifies that SW mute was
> > done.
> 
> > If none of that flow was used the LED and mute function still work, but
> there
> > might be the popping noise.
> 
> With a timeout so that if things get lost somewhere then the mute button
> is still functional, 

Exactly.

> or can userspace block mute?  Also what happens if
> userspace tries to set the state without having done anything about
> muting, will it trigger the hardware level mute as though the key had
> been pressed?

No, the hardware level mute is tied to the actual key.  If the software
mute is set, the LED won't change.  In this case this is what would happen:
1) Kcontrol flips
2) Codec / Subsystem notifies dell-privacy-acpi
3) Dell-privacy-acpi notifies SW mute is done
4) Nothing happens to HW mute / LED

There is functionality in dell-privacy to track the state of the HW mute
so later on this scenario can be expanded upon to emit an event from
dell-privacy that userspace can see and show a message along the lines of

"SW was muted, but HW mute still isn't set, press FN+F2 to set it".

That's follow up stuff for after the initial patch series is landed.
kernel test robot Nov. 3, 2020, 7:03 p.m. UTC | #7 
Hi Perry,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on asoc/for-next]
[also build test ERROR on v5.10-rc2 next-20201103]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Perry-Yuan/ASoC-rt715-add-Mic-Mute-LED-control-support/20201103-210037
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
config: m68k-randconfig-r021-20201103 (attached as .config)
compiler: m68k-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/7907f123e41159e71a0151518d1018dce3c5656d
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Perry-Yuan/ASoC-rt715-add-Mic-Mute-LED-control-support/20201103-210037
        git checkout 7907f123e41159e71a0151518d1018dce3c5656d
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=m68k 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   m68k-linux-ld: sound/soc/codecs/rt715.o: in function `rt715_micmute_led_mode_put':
>> sound/soc/codecs/rt715.c:248: undefined reference to `ledtrig_audio_set'
   m68k-linux-ld: sound/soc/codecs/rt715.o: in function `rt715_mic_mute_led_mode_get':
   sound/soc/codecs/rt715.c:233: undefined reference to `ledtrig_audio_set'

vim +248 sound/soc/codecs/rt715.c

   238	
   239	static int rt715_micmute_led_mode_put(struct snd_kcontrol *kcontrol,
   240	      struct snd_ctl_elem_value *ucontrol)
   241	{
   242	    struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
   243	    struct rt715_priv *rt715 = snd_soc_component_get_drvdata(component);
   244	    int led_mode = ucontrol->value.integer.value[0];
   245	
   246	    rt715->micmute_led = led_mode;
   247	#if IS_ENABLED(CONFIG_LEDS_TRIGGER_AUDIO)
 > 248	    ledtrig_audio_set(LED_AUDIO_MICMUTE,
   249	               rt715->micmute_led ? LED_ON : LED_OFF);
   250	#endif
   251	  return 0;
   252	}
   253	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff mbox series

Patch

diff --git a/sound/soc/codecs/rt715.c b/sound/soc/codecs/rt715.c
index 099c8bd20006..2df2895d0092 100644
--- a/sound/soc/codecs/rt715.c
+++ b/sound/soc/codecs/rt715.c
@@ -26,6 +26,7 @@ 
 #include <linux/of.h>
 #include <linux/of_gpio.h>
 #include <linux/of_device.h>
+#include <linux/leds.h>
 #include <sound/core.h>
 #include <sound/pcm.h>
 #include <sound/pcm_params.h>
@@ -213,6 +214,45 @@  static const DECLARE_TLV_DB_SCALE(mic_vol_tlv, 0, 1000, 0);
 	.private_value = SOC_DOUBLE_R_VALUE(reg_left, reg_right, xshift, \
 					    xmax, xinvert) }
 
+static const char *rt715_micmute_led_mode[] = {
+  "Off", "On"
+};
+
+static const struct soc_enum rt715_micmute_led_mode_enum =
+  SOC_ENUM_SINGLE_EXT(ARRAY_SIZE(rt715_micmute_led_mode),
+              rt715_micmute_led_mode);
+
+static int rt715_mic_mute_led_mode_get(struct snd_kcontrol *kcontrol,
+      struct snd_ctl_elem_value *ucontrol)
+{
+    struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
+    struct rt715_priv *rt715 = snd_soc_component_get_drvdata(component);
+    int led_mode = rt715->micmute_led;
+
+    ucontrol->value.integer.value[0] = led_mode;
+#if IS_ENABLED(CONFIG_LEDS_TRIGGER_AUDIO)
+    ledtrig_audio_set(LED_AUDIO_MICMUTE,
+            rt715->micmute_led ? LED_ON : LED_OFF);
+#endif
+    return 0;
+}
+
+static int rt715_micmute_led_mode_put(struct snd_kcontrol *kcontrol,
+      struct snd_ctl_elem_value *ucontrol)
+{
+    struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
+    struct rt715_priv *rt715 = snd_soc_component_get_drvdata(component);
+    int led_mode = ucontrol->value.integer.value[0];
+
+    rt715->micmute_led = led_mode;
+#if IS_ENABLED(CONFIG_LEDS_TRIGGER_AUDIO)
+    ledtrig_audio_set(LED_AUDIO_MICMUTE,
+               rt715->micmute_led ? LED_ON : LED_OFF);
+#endif
+  return 0;
+}
+
+
 static const struct snd_kcontrol_new rt715_snd_controls[] = {
 	/* Capture switch */
 	SOC_DOUBLE_R_EXT("ADC 07 Capture Switch", RT715_SET_GAIN_MIC_ADC_H,
@@ -277,6 +317,9 @@  static const struct snd_kcontrol_new rt715_snd_controls[] = {
 			RT715_SET_GAIN_LINE2_L, RT715_DIR_IN_SFT, 3, 0,
 			rt715_set_amp_gain_get, rt715_set_amp_gain_put,
 			mic_vol_tlv),
+    /*Micmute Led Control*/
+    SOC_ENUM_EXT("Micmute Led Mode", rt715_micmute_led_mode_enum,
+            rt715_mic_mute_led_mode_get, rt715_micmute_led_mode_put),
 };
 
 static int rt715_mux_get(struct snd_kcontrol *kcontrol,
diff --git a/sound/soc/codecs/rt715.h b/sound/soc/codecs/rt715.h
index df0f24f9bc0c..32917b7846b4 100644
--- a/sound/soc/codecs/rt715.h
+++ b/sound/soc/codecs/rt715.h
@@ -22,6 +22,7 @@  struct rt715_priv {
 	struct sdw_bus_params params;
 	bool hw_init;
 	bool first_hw_init;
+    int micmute_led;
 };
 
 struct sdw_stream_data {