linux-kernel - Re: [PATCH v4 2/2] ASoC: rt715:add micmute led state control supports

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: <11d7916c-5e8a-f3a2-5906-ed007ed146a3@redhat.com>
Date:   Thu, 25 Mar 2021 16:07:14 +0100
From:   Hans de Goede <hdegoede@...hat.com>
To:     "Yuan, Perry" <Perry.Yuan@...l.com>,
        Jaroslav Kysela <perex@...ex.cz>,
        Mark Brown <broonie@...nel.org>,
        "pierre-louis.bossart@...ux.intel.com" 
        <pierre-louis.bossart@...ux.intel.com>,
        "Limonciello, Mario" <Mario.Limonciello@...l.com>
Cc:     "pobrn@...tonmail.com" <pobrn@...tonmail.com>,
        "oder_chiou@...ltek.com" <oder_chiou@...ltek.com>,
        "tiwai@...e.com" <tiwai@...e.com>,
        "mgross@...ux.intel.com" <mgross@...ux.intel.com>,
        "lgirdwood@...il.com" <lgirdwood@...il.com>,
        "alsa-devel@...a-project.org" <alsa-devel@...a-project.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "platform-driver-x86@...r.kernel.org" 
        <platform-driver-x86@...r.kernel.org>
Subject: Re: [PATCH v4 2/2] ASoC: rt715:add micmute led state control supports

Hi,

On 3/25/21 3:11 PM, Yuan, Perry wrote:
> Hi Hans
> 
>> -----Original Message-----
>> From: Hans de Goede <hdegoede@...hat.com>
>> Sent: Monday, March 22, 2021 11:02 PM
>> To: Jaroslav Kysela; Yuan, Perry; Mark Brown; pierre-
>> louis.bossart@...ux.intel.com; Limonciello, Mario
>> Cc: pobrn@...tonmail.com; oder_chiou@...ltek.com; tiwai@...e.com;
>> mgross@...ux.intel.com; lgirdwood@...il.com; alsa-devel@...a-project.org;
>> linux-kernel@...r.kernel.org; platform-driver-x86@...r.kernel.org
>> Subject: Re: [PATCH v4 2/2] ASoC: rt715:add micmute led state control
>> supports
>>
>>
>> [EXTERNAL EMAIL]
>>
>> 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@...nel.org>
>>>>> Sent: Tuesday, March 9, 2021 1:24 AM
>>>>> To: Yuan, Perry
>>>>> Cc: pobrn@...tonmail.com; pierre-louis.bossart@...ux.intel.com;
>>>>> oder_chiou@...ltek.com; perex@...ex.cz; tiwai@...e.com;
>>>>> hdegoede@...hat.com; mgross@...ux.intel.com; Limonciello, Mario;
>>>>> lgirdwood@...il.com; alsa-devel@...a-project.org; linux-
>>>>> kernel@...r.kernel.org; platform-driver-x86@...r.kernel.org
>>>>> 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-
>> 71389
>>>> 2620
>>>>
>>>> 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@...ex.cz/
>>
>> And then use a udev-rule + hwdb and/or UCM profiles to configure the LED
>> trigger for specific models from userspace ?
>>
>> Regards,
>>
>> Hans
>>
>>
>>
> Because the SOF SDW design has no mic mute led control yet.
> So I add one short term solution to make Dell privacy working for now 
> Definitely , that is new solution I can add my patch on that to test as one user case .
> We really need to take the short term solution work for some system which support new SOF soundwire hardware which have  dependence on the MIC mute feature
> Meanwhile we can continue working on the new  "udev-rule + hwdb and/or UCM profiles" solution as to replace this one.
> If we agree that, I will submit another V6 addressing new feedback. 

The triggering of the LED trigger and the code registering the led_classdev
are in 2 separate subsystems; and should be merged separately.

If you post a new version of patch 1/2 addressing my review remarks then
I can merge that.

For merging the sound side of this you need to talk to the sound folks
(Jaroslav Kysela, Takashi Iwai, Mark Brown for files under sound/soc).

Regards,

Hans