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

[Limonciello, Mario]

> -----Original Message-----
> From: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>
> Sent: Tuesday, November 3, 2020 10:13
> To: Mark Brown; Yuan, Perry
> Cc: oder_chiou@xxxxxxxxxxx; alsa-devel@xxxxxxxxxxxxxxxx; lgirdwood@xxxxxxxxx;
> Limonciello, Mario; linux-kernel@xxxxxxxxxxxxxxx; tiwai@xxxxxxxx
> 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@xxxxxxxx>
> >>
> >> 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.