lkml.org 
[lkml]   [2021]   [Jan]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRE: [PATCH v3 3/3] ASoC: rt715:add micmute led state control supports
    Date
    > >> -----Original Message-----
    > >> From: Yuan, Perry <Perry_Yuan@Dell.com>
    > >> Sent: Tuesday, January 12, 2021 11:18
    > >> To: oder_chiou@realtek.com; perex@perex.cz; tiwai@suse.com;
    > >> hdegoede@redhat.com; mgross@linux.intel.com
    > >> Cc: lgirdwood@gmail.com; broonie@kernel.org; alsa-devel@alsa-project.org;
    > >> linux-kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org; Yuan,
    > >> Perry; Limonciello, Mario
    > >> Subject: [PATCH v3 3/3] ASoC: rt715:add micmute led state control supports
    > >>
    > >> 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
    > >> 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 driver to 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
    > >>
    > >> Signed-off-by: Perry Yuan <perry_yuan@dell.com>
    > >>
    > >> --------
    > >> 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 | 16 ++++++++++++++++
    > >> sound/soc/codecs/rt715-sdca.h | 1 +
    > >> sound/soc/codecs/rt715.c | 14 ++++++++++++++
    > >> sound/soc/codecs/rt715.h | 1 +
    > >> 4 files changed, 32 insertions(+)
    > >>
    > >> diff --git a/sound/soc/codecs/rt715-sdca.c b/sound/soc/codecs/rt715-sdca.c
    > >> index b43ac8559e45..861a0d2a8957 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>
    > >> @@ -244,6 +245,7 @@ static int rt715_sdca_get_volsw(struct snd_kcontrol
    > >> *kcontrol,
    > >> unsigned int max = mc->max;
    > >> int val;
    > >>
    > >> + pr_err("++++++rt715_sdca_get_volsw++\n");
    > >> val = snd_soc_component_read(component, mc->reg);
    > >> if (val < 0)
    > >> return -EINVAL;
    > >> @@ -261,6 +263,7 @@ static int rt715_sdca_put_volsw(struct snd_kcontrol
    > >> *kcontrol,
    > >> struct snd_ctl_elem_value *ucontrol)
    > >> {
    > >> struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
    > >> + struct rt715_sdca_priv *rt715 =
    > >> snd_soc_component_get_drvdata(component);
    > >> struct soc_mixer_control *mc =
    > >> (struct soc_mixer_control *)kcontrol->private_value;
    > >> unsigned int val, val2, loop_cnt = 2, i;
    > >> @@ -268,6 +271,7 @@ static int rt715_sdca_put_volsw(struct snd_kcontrol
    > >> *kcontrol,
    > >> unsigned int reg2 = mc->rreg;
    > >> unsigned int reg = mc->reg;
    > >> unsigned int max = mc->max;
    > >> + unsigned int val0, val1;
    > >> int err;
    > >>
    > >> val = ucontrol->value.integer.value[0];
    > >> @@ -287,6 +291,18 @@ static int rt715_sdca_put_volsw(struct snd_kcontrol
    > >> *kcontrol,
    > >> return err;
    > >> }
    > >>
    > >> +#if IS_ENABLED(CONFIG_DELL_PRIVACY)
    > >> + /* dell privacy LED trigger state changed by muted/unmute switch */
    > >> + if (mc->invert) {
    > >> + if (ucontrol->value.integer.value[0] || ucontrol-
    > >>> value.integer.value[1]) {
    > >> + rt715->micmute_led = LED_OFF;
    > >> + } else {
    > >> + rt715->micmute_led = LED_ON;
    > >> + }
    > >> + ledtrig_audio_set(LED_AUDIO_MICMUTE, rt715->micmute_led);
    > >> + }
    > >> +#endif
    > >> +
    > >> return 0;
    > >> }
    > >>
    > >> diff --git a/sound/soc/codecs/rt715-sdca.h b/sound/soc/codecs/rt715-sdca.h
    > >> index 840c237895dd..f8988ab88f80 100644
    > >> --- a/sound/soc/codecs/rt715-sdca.h
    > >> +++ b/sound/soc/codecs/rt715-sdca.h
    > >> @@ -31,6 +31,7 @@ struct rt715_sdca_priv {
    > >> int l_is_unmute;
    > >> int r_is_unmute;
    > >> int hw_sdw_ver;
    > >> + bool micmute_led;
    > >> };
    > >>
    > >> struct rt715_sdw_stream_data {
    > >> diff --git a/sound/soc/codecs/rt715.c b/sound/soc/codecs/rt715.c
    > >> index cdcba70146da..b4e480744c94 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;
    > >> + unsigned int val0, val1;
    > >>
    > >> if (strstr(ucontrol->id.name, "Main Capture Switch") ||
    > >> strstr(ucontrol->id.name, "Main Capture Volume"))
    > >> @@ -95,6 +97,18 @@ static int rt715_set_amp_gain_put(struct snd_kcontrol
    > >> *kcontrol,
    > >> else
    > >> loop_cnt = 1;
    > >>
    > >> +#if IS_ENABLED(CONFIG_DELL_PRIVACY)
    > >> + /* Micmute LED state changed by muted/unmute switch */
    > >> + if (mc->invert) {
    > >> + if (ucontrol->value.integer.value[0] || ucontrol-
    > >>> value.integer.value[1]) {
    > >> + rt715->micmute_led = LED_OFF;
    > >> + } else {
    > >> + rt715->micmute_led = LED_ON;
    > >> + }
    > >> + ledtrig_audio_set(LED_AUDIO_MICMUTE, rt715->micmute_led);
    > >> + }
    > >> +#endif
    > >> +
    > > You might have missed my other comment on v2 feedback, but is there a reason
    > > to keep it behind a compile time flag for dell privacy module? In practice
    > > any other future led backend provider should work too. Another way to think
    > > about it - if dell privacy wasn't enabled would this cause a problem to run
    > > this code? I think it would just be a no-op.
    > >
    > >> for (j = 0; j < loop_cnt; j++) {
    > >> /* Can't use update bit function, so read the original value first
    > >> */
    > >> if (loop_cnt == 1) {
    > >> diff --git a/sound/soc/codecs/rt715.h b/sound/soc/codecs/rt715.h
    > >> index 009a8266f606..57c9af041181 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;
    > >> + bool micmute_led;
    > >> };
    > >>
    > >> struct sdw_stream_data {
    > >> --
    > >> 2.25.1
    >
    > Pierre Louis suggested to just set the mic mute led state unconditionally .
    > It is more common interface to allow other platforms to change micmute led.
    > The discussion result is from "[PATCH v2 2/2] ASoC: rt715:add Mic Mute LED
    > control support"
    >
    > Here is the change compared to V3 ,the CONFIG_DELL_PRIVACY will be removed
    >
    > -#if IS_ENABLED(CONFIG_DELL_PRIVACY)
    > - /* dell privacy LED trigger state changed by muted/unmute switch */
    > + /* MicMute LED state changed by muted/unmute switch */
    > if (mc->invert) {
    > if (ucontrol->value.integer.value[0] || ucontrol-
    > >value.integer.value[1]) {
    > rt715->micmute_led = LED_OFF;
    > diff --git a/sound/soc/codecs/rt715.c b/sound/soc/codecs/rt715.c
    > index b4e480744c94..60bb3d98103e 100644
    > --- a/sound/soc/codecs/rt715.c
    > +++ b/sound/soc/codecs/rt715.c
    > @@ -97,8 +97,7 @@ static int rt715_set_amp_gain_put(struct snd_kcontrol
    > *kcontrol,
    > else
    > loop_cnt = 1;
    >
    > -#if IS_ENABLED(CONFIG_DELL_PRIVACY)
    > - /* Micmute LED state changed by muted/unmute switch */
    > + /* MicMute LED state changed by muted/unmute switch */
    > if (mc->invert) {
    > if (ucontrol->value.integer.value[0] || ucontrol-
    > >value.integer.value[1]) {
    > rt715->micmute_led = LED_OFF;
    > @@ -107,7 +106,6 @@ static int rt715_set_amp_gain_put(struct snd_kcontrol
    > *kcontrol,
    > }
    > ledtrig_audio_set(LED_AUDIO_MICMUTE, rt715->micmute_led);
    > }
    > -#endif

    I'm not sure which thread, but I believe there was still also another suggestion
    that this should be made more "generic" and to work for all codecs. So that when
    we have lets say a hypothetical rt717 we don't need the same in that module.

    \
     
     \ /
      Last update: 2021-01-19 17:37    [W:5.392 / U:0.908 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site