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

[Perry Yuan]

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;