Re: [PATCH v2 1/2] platform/x86: dell-privacy: Add support for Dell hardware privacy

[Hans de Goede <hdegoede@redhat.com>]

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