Messages in this thread | | | Subject | Re: [PATCH v3 3/3] ASoC: rt715:add micmute led state control supports | From | Perry Yuan <> | Date | Sun, 14 Feb 2021 14:42:51 +0800 |
| |
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;
| |