linux-kernel - RE: [PATCH] ASoC: rt715:add Mic Mute LED control support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <DM6PR19MB2636291E083E1843B0E0035CFA110@DM6PR19MB2636.namprd19.prod.outlook.com>
Date:   Tue, 3 Nov 2020 17:51:57 +0000
From:   "Limonciello, Mario" <Mario.Limonciello@...l.com>
To:     Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>,
        Mark Brown <broonie@...nel.org>,
        "Yuan, Perry" <Perry.Yuan@...l.com>
CC:     "oder_chiou@...ltek.com" <oder_chiou@...ltek.com>,
        "alsa-devel@...a-project.org" <alsa-devel@...a-project.org>,
        "lgirdwood@...il.com" <lgirdwood@...il.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "tiwai@...e.com" <tiwai@...e.com>
Subject: RE: [PATCH] ASoC: rt715:add Mic Mute LED control support

> -----Original Message-----
> From: Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
> Sent: Tuesday, November 3, 2020 10:13
> To: Mark Brown; Yuan, Perry
> Cc: oder_chiou@...ltek.com; alsa-devel@...a-project.org; lgirdwood@...il.com;
> Limonciello, Mario; linux-kernel@...r.kernel.org; tiwai@...e.com
> Subject: Re: [PATCH] ASoC: rt715:add Mic Mute LED control support
> 
> 
> [EXTERNAL EMAIL]
> 
> Somehow this patch was filtered by alsa-devel servers?
> 
> On 11/3/20 7:12 AM, Mark Brown wrote:
> > On Tue, Nov 03, 2020 at 04:58:59AM -0800, Perry Yuan wrote:
> >> From: perry_yuan <perry_yuan@...l.com>
> >>
> >> Some new Dell system is going to support audio internal micphone
> >> privacy setting from hardware level with micmute led state changing
> >>
> >> This patch allow to change micmute led state through this micphone
> >> led control interface like hda_generic provided.
> >
> > If this is useful it should be done at the subsystem level rather than
> > open coded in a specific CODEC driver, however I don't undersand why it
> > is.
> >
> >> +static int rt715_micmute_led_mode_put(struct snd_kcontrol *kcontrol,
> >> +      struct snd_ctl_elem_value *ucontrol)
> >> +{
> >> +    struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
> >> +    struct rt715_priv *rt715 = snd_soc_component_get_drvdata(component);
> >> +    int led_mode = ucontrol->value.integer.value[0];
> >> +
> >> +    rt715->micmute_led = led_mode;
> >> +#if IS_ENABLED(CONFIG_LEDS_TRIGGER_AUDIO)
> >> +    ledtrig_audio_set(LED_AUDIO_MICMUTE,
> >> +               rt715->micmute_led ? LED_ON : LED_OFF);
> >> +#endif
> >> +  return 0;
> >> +}
> >
> > This is just adding a userspace API to set a LED via the standard LED
> > APIs.  Since the LED subsystem already has a perfectly good userspace
> > API why not use that?  There is no visible value in this being in the
> > sound subsystem.
> 
> I also don't quite follow. This looks as inspired from HDaudio code, but
> with a lot of simplifications.
> 
> If the intent was that when userspace decides to mute the LED is turned
> on, wouldn't it be enough to just track the state of a 'capture switch'
> responsible for mute, i.e. when the capture Switch is 'off' the LED is
> on. I don't see the point of having a new control, you would be adding
> more work for PulseAudio/UCM whereas connecting the capture switch to a
> led comes with zero work in userspace. See e.g. how the mute mic LED was
> handled in the SOF code handling DMICs, we didn't add a new control but
> turned the LED in the switch .put callback, see
> 
> https://elixir.bootlin.com/linux/latest/source/sound/soc/sof/control.c#L18
> 
> https://elixir.bootlin.com/linux/latest/source/sound/soc/sof/control.c#L153
> 
> Actually thinking more about it, having two controls for 'mute LED' and
> 'capture switch' could lead to inconsistent states where the LED is on
> without mute being activated. we should really bolt the LED activation
> to the capture switch, that way the mute and LED states are aligned.
> 

After giving it some thought I agree.  The UCM change that was opened
here https://github.com/alsa-project/alsa-ucm-conf/pull/60 wouldn't
be necessary at all if you just track capture switch directly like SOF does.