Hi Mark,
Thanks for your review feedback.
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.
I will remove the CONFIG_DELL_PRIVACY ,because other vendors and other
platforms maybe need to set their Mic mute led state when they enable
soundwire and sof driver for audio function.
We can avoid hardcode in codec driver, indeed it is need for this new
soundwire & sof architecture of alsa.
#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.
It is my mistake to leave the debug line here.
will be removed in next V4 .
@@ -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 for the mic mute led operation is
not only needed for dell platforms but also other Vendors all need to
set micmute led necessaryily.
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.
At first, i just want to handle val0 == 1 && val1 == 1 and val0 == 0 &&
val1 == 0 only. Intel suggested i need to cover all about other cases,
the mute led should be off if any channel of mic is on.
So i change the led state when any one channel is muted or unmuted.
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?
Do you mean i can use a local micmute_led var in the function?
