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

[Perry Yuan <perry979106@gmail.com>]

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;

.....