Hi, On 3/22/21 3:37 PM, Jaroslav Kysela wrote: > Dne 22. 03. 21 v 10:25 Yuan, Perry napsal(a): >> Hi Mark: >> >>> -----Original Message----- >>> From: Mark Brown <broonie@xxxxxxxxxx> >>> Sent: Tuesday, March 9, 2021 1:24 AM >>> To: Yuan, Perry >>> Cc: pobrn@xxxxxxxxxxxxxx; pierre-louis.bossart@xxxxxxxxxxxxxxx; >>> oder_chiou@xxxxxxxxxxx; perex@xxxxxxxx; tiwai@xxxxxxxx; >>> hdegoede@xxxxxxxxxx; mgross@xxxxxxxxxxxxxxx; Limonciello, Mario; >>> lgirdwood@xxxxxxxxx; alsa-devel@xxxxxxxxxxxxxxxx; linux- >>> kernel@xxxxxxxxxxxxxxx; platform-driver-x86@xxxxxxxxxxxxxxx >>> Subject: Re: [PATCH v4 2/2] ASoC: rt715:add micmute led state control >>> supports >>> >>> On Mon, Mar 01, 2021 at 05:38:34PM +0800, Perry Yuan wrote: >>> >>>> + /* Micmute LED state changed by muted/unmute switch */ >>>> + if (mc->invert) { >>>> + if (ucontrol->value.integer.value[0] || ucontrol- >>>> value.integer.value[1]) { >>>> + micmute_led = LED_OFF; >>>> + } else { >>>> + micmute_led = LED_ON; >>>> + } >>>> + ledtrig_audio_set(LED_AUDIO_MICMUTE, micmute_led); >>>> + } >>> >>> These conditionals on inversion seem weird and counterintuitive. If we're >>> going with this approach it would probably be clearer to define a custom >>> operation for the affected controls that wraps the standard one and adds the >>> LED setting rather than keying off invert like this. >> >> Currently the sof soundwire driver has no generic led control yet. >> This patch can handle the led control needs for MIC mute LED, definitely the patch is a short term solution. >> There is a feature request discussion when we started to implement this solution. >> https://github.com/thesofproject/linux/issues/2496#issuecomment-713892620 >> >> The workable way for now is that we put the LED mute control to the codec driver. >> When there is new and full sound LED solution implemented, this part will be also optimized. >> The Hardware privacy feature needs this patch to handle the Mic mute led state change. >> Before that full solution ready in kernel, could we take this as short term solution? > > Perry, it's about the machine detection. Your code is too much generic even > for the top-level LED trigger implementation. We need an extra check, if the > proper LED's are really controlled on the specific hardware. Other hardware > may use RT715 for a different purpose. Use DMI / ACPI checks to detect this > hardware and don't misuse the inversion flag to enable this code. I think this would be a goo candidate for the new generic LED handling: https://lore.kernel.org/alsa-devel/20210317172945.842280-1-perex@xxxxxxxx/ And then use a udev-rule + hwdb and/or UCM profiles to configure the LED trigger for specific models from userspace ? Regards, Hans