Date:   Sun, 14 Feb 2021 14:42:51 +0800
From:   Perry Yuan <perry979106@...il.com>
To:     Mark Brown <broonie@...nel.org>, Perry Yuan <Perry.Yuan@...l.com>
Cc:     oder_chiou@...ltek.com, perex@...ex.cz, tiwai@...e.com,
        hdegoede@...hat.com, mgross@...ux.intel.com, lgirdwood@...il.com,
        alsa-devel@...a-project.org, linux-kernel@...r.kernel.org,
        platform-driver-x86@...r.kernel.org, Mario.Limonciello@...l.com
Subject: Re: [PATCH v3 3/3] ASoC: rt715:add micmute led state control supports

Hi Mark:
Thanks for your review.

On 2021/1/13 1:54, Mark Brown wrote:
> On Wed, Jan 13, 2021 at 01:18:14AM +0800, Perry Yuan wrote:
> 
>> 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
> 
> It feels like there's an abstraction problem here with this being hard
> coded in a specific CODEC driver.
> 
>>   #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;
> 
> This shouldn't be in the patch.
Removed in V4, I forget to clear this debug code
> 
>> @@ -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
>> +
> 
> This doesn't look good.  There's nothing Dell specific here, and nothing
> about this is conditional on any sort of runtime detection of Dell
> systems, it's not obvious why this is conditional on DELL_PRIVACY or why
> we only report the state if the control is inverted.
> 
I will remove the CONFIG_DELL_PRIVACY from V4 patch and allow it to run 
if CONFIG_DELL_PRIVACY is not set, the result will be a no-op.
> I'm also not convinced that it's a good idea to set the mute LED if only
> one channel in a stereo microphone is muted, that seems likely to lead
> to surprising behaviour for users.
> 
https://github.com/thesofproject/linux/pull/2660#discussion_r555480210
There is a discussion for the channel mute changing behavior.
If the anyone of value[0] or value[1] is 1, it means mic is NOT muted
The muted state will be LED_ON state need to set.

> TBH I don't understand why this isn't being done in generic code.
> 
>> +	bool micmute_led;
> 
> What is this for, it never seems to be read except for in the function
> where it's set?
> 

I have moved this part code to the local definition of 
rt715_set_amp_gain_put and removed from rt715_priv.
new code will be like this in V4.

@@ -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;
+       bool micmute_led;

Powered by blists - more mailing lists