Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v2 1/2] platform/x86: dell-privacy: Add support for Dell
 hardware privacy
To:     Perry Yuan <perry979106@gmail.com>,
        =?UTF-8?Q?Barnab=c3=a1s_P=c5=91cze?= <pobrn@protonmail.com>,
        Perry Yuan <Perry.Yuan@dell.com>
Cc:     "mgross@linux.intel.com" <mgross@linux.intel.com>,
        "platform-driver-x86@vger.kernel.org" 
        <platform-driver-x86@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Limonciello Mario <Mario.Limonciello@dell.com>
References: <20201228132855.17544-1-Perry_Yuan@Dell.com>
 <mXtNUreCVsPKizG-fnNKPId9_lQGit0S6pYEpk-aNT1hGQjVwblZDiO4E-YX3hxnrbbNN1-6hSntiv13wz66RxbhXjPJhpAVOIlZmmwUI7w=@protonmail.com>
 <7def1cdc-e275-9ae6-4941-55517359bb1e@gmail.com>
 <ce598a92-73ef-e9a6-d772-59f5da3d251e@redhat.com>
 <facc6270-1e04-b348-a1be-a97f87870c46@gmail.com>
From:   Hans de Goede <hdegoede@redhat.com>
Message-ID: <aaa0d156-3394-b8e3-9a48-196a1265cdd9@redhat.com>
Date:   Thu, 4 Mar 2021 11:49:56 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.7.0
MIME-Version: 1.0
In-Reply-To: <facc6270-1e04-b348-a1be-a97f87870c46@gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi,

On 2/16/21 8:24 AM, Perry Yuan wrote:
> Hi Hans:
> 
> On 2021/1/13 2:37, Hans de Goede wrote:
>> Hi,
>>
>> I know there already is a v3 out and I will try to get around to reviewing
>> that soon, still 1 remark about the discussion surrounding v2:
>>
>> On 1/11/21 2:42 PM, Perry Yuan wrote:
>>
>> <snip>
>>
>>>>> *The flow is like this:
>>>>> 1) User presses key. HW does stuff with this key (timeout is started)
>>>>> 2) Event is emitted from FW
>>>>> 3) Event received by dell-privacy
>>>>> 4) KEY_MICMUTE emitted from dell-privacy
>>>>> 5) Userland picks up key and modifies kcontrol for SW mute
>>>>> 6) Codec kernel driver catches and calls ledtrig_audio_set, like this:
>>>>>     ledtrig_audio_set(LED_AUDIO_MICMUTE,
>>>>>         rt715->micmute_led ? LED_ON :LED_OFF);
>>>>> 7) If "LED" is set to on dell-privacy notifies ec,
>>>>>    and timeout is cancelled,HW mic mute activated.
>>>>>
>>>> Please proofread the commit message again, and pay attention to capitalization
>>>> and spacing.
>>> I want to reformat it and move the commit info to cover letter.
>>
>> Please also put a copy of this as a comment in either the wmi or the
>> acpi driver (with a comment pointing to the comment in the other) this is
>> important info to have for someone reading the code and trying to understand
>> how this all fits together.
>>
>> Regards,
>>
>> Hans
>>
> Hans.
> I have added the comments to the dell-privacy  driver file in V4
> 
> -----------------------------------------------------------------------------------
> drivers/platform/x86/dell-privacy-wmi.c
> 
> EXPORT_SYMBOL_GPL(dell_privacy_valid);
> /*
>  * The flow of privacy event:
>  * 1) User presses key. HW does stuff with this key (timeout is started)
>  * 2) WMI event is emitted from BIOS
>  * 3) WMI event is received by dell-privacy
>  * 4) KEY_MICMUTE emitted from dell-privacy
>  * 5) Userland picks up key and modifies kcontrol for SW mute
>  * 6) Codec kernel driver catches and calls ledtrig_audio_set defined by
>  *    dell-privacy-acpi driver.
>  *    codec driver will call like this to switch micmute led state.
>  *      ledtrig_audio_set(LED_AUDIO_MICMUTE, micmute_led ? LED_ON :LED_OFF);
>  * 7) If "LED" is set to on dell-privacy notifies EC,and timeout is cancelled,
>  *      HW mic mute activated.
>  */
> void dell_privacy_process_event(int type, int code, int status)
> {
>         struct privacy_wmi_data *priv;
>         const struct key_entry *key;
> 
>         mutex_lock(&list_mutex);
> ....
> 
> -----------------------------------------------------------------------------------
> drivers/platform/x86/dell-privacy-acpi.c
> 
> /*
>  * Pressing the mute key activates a time delayed circuit to physically cut
>  * off the mute. The LED is in the same circuit, so it reflects the true
>  * state of the HW mute.  The reason for the EC "ack" is so that software
>  * can first invoke a SW mute before the HW circuit is cut off.  Without SW
>  * cutting this off first does not affect the time delayed muting or status
>  * of the LED but there is a possibility of a "popping" noise.
>  *
>  * If the EC receives the SW ack, the circuit will be activated before the
>  * delay completed.
>  *
>  * Exposing as an LED device allows the codec drivers notification path to
>  * EC ACK to work
>  */
> static int dell_privacy_leds_setup(struct device *dev)
> {
>         struct privacy_acpi_priv *priv = dev_get_drvdata(dev);
>         int ret = 0;
> 
> .....
> 

This looks good, thank you.

Regards,

Hans