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

From: Hans de Goede
Date: Tue Jan 19 2021 - 15:35:36 EST

Next message: Kari Argillander: "Re: [PATCH v17 01/10] fs/ntfs3: Add headers and misc files"
Previous message: Vincenzo Frascino: "Re: [PATCH] kasan: Add explicit preconditions to kasan_report()"
In reply to: Limonciello, Mario: "RE: [PATCH v3 3/3] ASoC: rt715:add micmute led state control supports"
Next in thread: Takashi Iwai: "Re: [PATCH v3 3/3] ASoC: rt715:add micmute led state control supports"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi,

On 1/19/21 5:33 PM, Limonciello, Mario wrote:
>>>> -----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.

Right:

On 1/15/21 6:45 PM, Jaroslav Kysela wrote:
> Dne 12. 01. 21 v 18:18 Perry Yuan napsal(a):
>
>> +#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
>
> [removing lkml and x86-platform lists]
>
> Looking to this code, perhaps, it may be a good idea to move the LED trigger
> handling from the low-level drivers and do it in the top-level control code
> (sound/core/control.c). Adding a new flag SNDRV_CTL_ELEM_ACCESS_MIC_LED may be
> an option and we can nicely handle the situation for multiple microphones (OR
> operation). The motivation is to remove the duplicated code.
>
> The same situation is for the playback LED.

I believe that this is an excellent suggestion and that this is the way to go.

Regards,

Hans

Next message: Kari Argillander: "Re: [PATCH v17 01/10] fs/ntfs3: Add headers and misc files"
Previous message: Vincenzo Frascino: "Re: [PATCH] kasan: Add explicit preconditions to kasan_report()"
In reply to: Limonciello, Mario: "RE: [PATCH v3 3/3] ASoC: rt715:add micmute led state control supports"
Next in thread: Takashi Iwai: "Re: [PATCH v3 3/3] ASoC: rt715:add micmute led state control supports"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]