RE: [PATCH v3 3/3] ASoC: rt715:add micmute led state control supports

[Yuan, Perry]

> -----Original Message-----
> From: Limonciello, Mario <Mario_Limonciello@xxxxxxxx>
> Sent: Wednesday, January 20, 2021 12:33 AM
> To: Perry Yuan; Yuan, Perry; oder_chiou@xxxxxxxxxxx; perex@xxxxxxxx;
> tiwai@xxxxxxxx; hdegoede@xxxxxxxxxx; mgross@xxxxxxxxxxxxxxx
> Cc: lgirdwood@xxxxxxxxx; broonie@xxxxxxxxxx; alsa-devel@xxxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; platform-driver-x86@xxxxxxxxxxxxxxx
> Subject: RE: [PATCH v3 3/3] ASoC: rt715:add micmute led state control
> supports
> 
> > >> -----Original Message-----
> > >> From: Yuan, Perry <Perry_Yuan@xxxxxxxx>
> > >> Sent: Tuesday, January 12, 2021 11:18
> > >> To: oder_chiou@xxxxxxxxxxx; perex@xxxxxxxx; tiwai@xxxxxxxx;
> > >> hdegoede@xxxxxxxxxx; mgross@xxxxxxxxxxxxxxx
> > >> Cc: lgirdwood@xxxxxxxxx; broonie@xxxxxxxxxx;
> > >> alsa-devel@xxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> > >> platform-driver-x86@xxxxxxxxxxxxxxx; Yuan, Perry; Limonciello,
> > >> Mario
> > >> Subject: [PATCH v3 3/3] ASoC: rt715:add micmute led state control
> > >> supports
> > >>
> > >> From: Perry Yuan <perry_yuan@xxxxxxxx>
> > >>
> > >> 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@xxxxxxxx>
> > >>
> > >> --------
> > >> 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.
Hi Mario.
Jaroslav suggested me to move this part code to generic control layer.
I will adjust this patch and do some testing for the new patch.

Perry.