RE: [PATCH v8 2/2] ASoC: rt715:add micmute led state control supports

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Hans

> -----Original Message-----
> From: Hans de Goede <hdegoede@xxxxxxxxxx>
> Sent: 2021年5月7日 21:18
> To: Yuan, Perry; pobrn@xxxxxxxxxxxxxx; pierre-
> louis.bossart@xxxxxxxxxxxxxxx; oder_chiou@xxxxxxxxxxx; perex@xxxxxxxx;
> tiwai@xxxxxxxx; mgross@xxxxxxxxxxxxxxx
> Cc: lgirdwood@xxxxxxxxx; broonie@xxxxxxxxxx; alsa-devel@xxxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; platform-driver-x86@xxxxxxxxxxxxxxx;
> mario.limonciello@xxxxxxxxxxx; Dell Client Kernel
> Subject: Re: [PATCH v8 2/2] ASoC: rt715:add micmute led state control
> supports
> 
> 
> [EXTERNAL EMAIL]
> 
> Hi Perry,
> 
> On 5/6/21 1:56 PM, Perry Yuan wrote:
> > 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 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 <perry_yuan@xxxxxxxx>
> 
> NACK, as explained before we want the binding of the control to the LED-
> trigger to be done from the UCM profile.
> 
> Support for this has landed kernel-side in Linux tree now (this will be part of
> 5.13-rc1). Together with the latest git alsa-lib and alsa-utils code, you can now
> do what this patch does from an UCM profile file and AFAIK that is the
> preferred way to do this.
> 
> See here for an example UCM profile patch implementing this:
> 
> https://urldefense.com/v3/__https://lore.kernel.org/alsa-
> devel/20210507131139.10231-3-
> hdegoede@xxxxxxxxxx/T/*u__;Iw!!LpKI!zM9rVPYaTjLEYUmBrIayi3aj_QvB5p1
> u2luEckfQCUeVbnRPk7DL8hwdhIDLjEc$ [lore[.]kernel[.]org]
> 
> Note that if you test this under Fedora you will hit a selinux denial, to
> workaround that you can put selinux in permissive mode. This selinux issue is
> being tracked here:
> 
> https://urldefense.com/v3/__https://bugzilla.redhat.com/show_bug.cgi?id=1
> 958210__;!!LpKI!zM9rVPYaTjLEYUmBrIayi3aj_QvB5p1u2luEckfQCUeVbnRPk7
> DL8hwd_fmxo0I$ [bugzilla[.]redhat[.]com]
> 
> Regards,
> 
> Hans

Thanks for the feedback a lot.
I am trying  to make one new patch for the ucm2 profile
Could you help to check if there is some other changes I need to make for the privacy driver ?

Perry

> 
> 
> 
> 
> 
> 
> >
> > --------
> > v7 -> v8:
> > * N/A
> > v6 -> v7:
> > * addresed review comments from Jaroslav
> > * use device id in the quirk list
> > v5 -> v6:
> > * add quirks for micmute led control as short term solution to control
> >   micmute led state change
> > * add comments for the invert checking
> > v4 -> v5:
> > * rebase to latest 5.12 rc4 upstream kernel
> > 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://urldefense.com/v3/__https://lore.kernel.org/alsa-devel/2021021
> > 1111400.1131020-1-
> perex@xxxxxxxx/__;!!LpKI!zM9rVPYaTjLEYUmBrIayi3aj_Qv
> > B5p1u2luEckfQCUeVbnRPk7DL8hwdy240518$ [lore[.]kernel[.]org]
> > 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 | 42
> +++++++++++++++++++++++++++++++++++
> >  sound/soc/codecs/rt715.c      | 42
> +++++++++++++++++++++++++++++++++++
> >  2 files changed, 84 insertions(+)
> >
> > diff --git a/sound/soc/codecs/rt715-sdca.c
> > b/sound/soc/codecs/rt715-sdca.c index 936e3061ca1e..de46514e0207
> > 100644
> > --- a/sound/soc/codecs/rt715-sdca.c
> > +++ b/sound/soc/codecs/rt715-sdca.c
> > @@ -11,12 +11,14 @@
> >  #include <linux/moduleparam.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>
> >  #include <linux/regmap.h>
> >  #include <linux/slab.h>
> >  #include <linux/platform_device.h>
> > +#include <linux/dmi.h>
> >  #include <sound/core.h>
> >  #include <sound/pcm.h>
> >  #include <sound/pcm_params.h>
> > @@ -344,6 +346,32 @@ static int rt715_sdca_get_volsw(struct
> snd_kcontrol *kcontrol,
> >  	return 0;
> >  }
> >
> > +static bool micmute_led_set;
> > +static int  dmi_matched(const struct dmi_system_id *dmi) {
> > +	micmute_led_set = 1;
> > +	return 1;
> > +}
> > +
> > +/* Some systems will need to use this to trigger mic mute LED state
> > +changed */ static const struct dmi_system_id micmute_led_dmi_table[] = {
> > +	{
> > +		.callback = dmi_matched,
> > +		.matches = {
> > +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> > +			DMI_MATCH(DMI_PRODUCT_SKU, "0A32"),
> > +		},
> > +	},
> > +	{
> > +		.callback = dmi_matched,
> > +		.matches = {
> > +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> > +			DMI_MATCH(DMI_PRODUCT_SKU, "0A3E"),
> > +		},
> > +	},
> > +	{},
> > +};
> > +
> >  static int rt715_sdca_put_volsw(struct snd_kcontrol *kcontrol,
> >  	struct snd_ctl_elem_value *ucontrol)  { @@ -358,6 +386,7 @@ static
> > int rt715_sdca_put_volsw(struct snd_kcontrol *kcontrol,
> >  	unsigned int mask = (1 << fls(max)) - 1;
> >  	unsigned int invert = p->invert;
> >  	int err;
> > +	bool micmute_led;
> >
> >  	for (i = 0; i < 4; i++) {
> >  		if (ucontrol->value.integer.value[i] != rt715-
> >kctl_switch_orig[i])
> > { @@ -394,6 +423,18 @@ static int rt715_sdca_put_volsw(struct
> snd_kcontrol *kcontrol,
> >  			return err;
> >  	}
> >
> > +	/* Micmute LED state changed by muted/unmute switch
> > +	 * to keep syncing with actual hardware mic mute led state
> > +	 * invert will be checked to change the state switch
> > +	 */
> > +	if (invert && micmute_led_set) {
> > +		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 k_changed;
> >  }
> >
> > @@ -1069,6 +1110,7 @@ int rt715_sdca_io_init(struct device *dev,
> > struct sdw_slave *slave)
> >
> >  	pm_runtime_mark_last_busy(&slave->dev);
> >  	pm_runtime_put_autosuspend(&slave->dev);
> > +	dmi_check_system(micmute_led_dmi_table);
> >
> >  	return 0;
> >  }
> > diff --git a/sound/soc/codecs/rt715.c b/sound/soc/codecs/rt715.c index
> > 1352869cc086..4dbd870009b8 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>
> > @@ -25,6 +26,7 @@
> >  #include <linux/of.h>
> >  #include <linux/of_gpio.h>
> >  #include <linux/of_device.h>
> > +#include <linux/dmi.h>
> >  #include <sound/core.h>
> >  #include <sound/pcm.h>
> >  #include <sound/pcm_params.h>
> > @@ -70,6 +72,32 @@ static void rt715_get_gain(struct rt715_priv *rt715,
> unsigned int addr_h,
> >  		pr_err("Failed to get L channel gain.\n");  }
> >
> > +static bool micmute_led_set;
> > +static int  dmi_matched(const struct dmi_system_id *dmi) {
> > +	micmute_led_set = 1;
> > +	return 1;
> > +}
> > +
> > +/* Some systems will need to use this to trigger mic mute LED state
> > +changed */ static const struct dmi_system_id micmute_led_dmi_table[] = {
> > +	{
> > +		.callback = dmi_matched,
> > +		.matches = {
> > +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> > +			DMI_MATCH(DMI_PRODUCT_SKU, "0A32"),
> > +		},
> > +	},
> > +	{
> > +		.callback = dmi_matched,
> > +		.matches = {
> > +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> > +			DMI_MATCH(DMI_PRODUCT_SKU, "0A3E"),
> > +		},
> > +	},
> > +	{},
> > +};
> > +
> >  /* For Verb-Set Amplifier Gain (Verb ID = 3h) */  static int
> > rt715_set_amp_gain_put(struct snd_kcontrol *kcontrol,
> >  					struct snd_ctl_elem_value *ucontrol)
> @@ -83,6 +111,7 @@ static
> > int rt715_set_amp_gain_put(struct snd_kcontrol *kcontrol,
> >  	unsigned int addr_h, addr_l, val_h, val_ll, val_lr;
> >  	unsigned int read_ll, read_rl, i;
> >  	unsigned int k_vol_changed = 0;
> > +	bool micmute_led;
> >
> >  	for (i = 0; i < 2; i++) {
> >  		if (ucontrol->value.integer.value[i] != rt715-
> >kctl_2ch_vol_ori[i])
> > { @@ -155,6 +184,18 @@ static int rt715_set_amp_gain_put(struct
> snd_kcontrol *kcontrol,
> >  			break;
> >  	}
> >
> > +	/* Micmute LED state changed by muted/unmute switch
> > +	 * to keep syncing with actual hardware mic mute led state
> > +	 * invert will be checked to change the state switch
> > +	 */
> > +	if (micmute_led_set) {
> > +		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);
> > +	}
> > +
> >  	/* D0:power on state, D3: power saving mode */
> >  	if (dapm->bias_level <= SND_SOC_BIAS_STANDBY)
> >  		regmap_write(rt715->regmap,
> > @@ -1088,6 +1129,7 @@ int rt715_io_init(struct device *dev, struct
> > sdw_slave *slave)
> >
> >  	pm_runtime_mark_last_busy(&slave->dev);
> >  	pm_runtime_put_autosuspend(&slave->dev);
> > +	dmi_check_system(micmute_led_dmi_table);
> >
> >  	return 0;
> >  }
> >





[Index of Archives]     [ALSA User]     [Linux Audio Users]     [Pulse Audio]     [Kernel Archive]     [Asterisk PBX]     [Photo Sharing]     [Linux Sound]     [Video 4 Linux]     [Gimp]     [Yosemite News]

  Powered by Linux