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

[Perry Yuan]

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;

.....