lkml.org 
[lkml]   [2021]   [Feb]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v3 3/3] ASoC: rt715:add micmute led state control supports
From
Date
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;

\
 
 \ /
  Last update: 2021-02-14 07:46    [W:0.275 / U:0.392 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site