[PATCH] ASoC: rt715:add Mic Mute LED control support
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(+)
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 {
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.
Please also follow the kernel coding style, your code doesn't visually resemble the adjacent code.
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.
-----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.
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.
-----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/2020110312554... 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.
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?
-----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.
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/2020110312554... 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
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-... 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/7907f123e41159e71a0151518d1018dce3c5... 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
participants (5)
-
kernel test robot -
Limonciello, Mario -
Mark Brown -
Perry Yuan -
Pierre-Louis Bossart