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 for Android: free password hash cracker in your pocket

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

Message-ID: <5f2c1282-4401-276a-8dad-127fa1f449fd@linux.intel.com>
Date:   Tue, 3 Nov 2020 10:13:03 -0600
From:   Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
To:     Mark Brown <broonie@...nel.org>, Perry Yuan <Perry.Yuan@...l.com>
Cc:     oder_chiou@...ltek.com, alsa-devel@...a-project.org,
        lgirdwood@...il.com,
        Limonciello Mario <Mario.Limonciello@...l.com>,
        linux-kernel@...r.kernel.org, tiwai@...e.com
Subject: Re: [PATCH] ASoC: rt715:add Mic Mute LED control support

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.