RE: [PATCH] platform/x86: dell-privacy: Add support for new privacy driver

From: "Limonciello, Mario" <Mario.Limonciello@dell.com>
To: Matthew Garrett <mjg59@srcf.ucam.org>,
	"Yuan, Perry" <Perry.Yuan@dell.com>
Cc: "hdegoede@redhat.com" <hdegoede@redhat.com>,
	"mgross@linux.intel.com" <mgross@linux.intel.com>,
	"pali@kernel.org" <pali@kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"platform-driver-x86@vger.kernel.org" 
	<platform-driver-x86@vger.kernel.org>
Subject: RE: [PATCH]  platform/x86: dell-privacy: Add support for new privacy driver
Date: Wed, 11 Nov 2020 14:30:21 +0000	[thread overview]
Message-ID: <DM6PR19MB2636956DB58B0E4ECAD43549FAE80@DM6PR19MB2636.namprd19.prod.outlook.com> (raw)
In-Reply-To: <20201111072456.tkwdzuq2wa7zvbod@srcf.ucam.org>

> 
> On Wed, Nov 11, 2020 at 07:21:07AM +0000, Yuan, Perry wrote:
> > > > +    status = acpi_evaluate_object(NULL, ACPI_PRIVACY_EC_ACK, NULL,
> > > NULL);
> > > > +    if (ACPI_FAILURE(status)) {
> > > > +        dev_err(led_cdev->dev, "Error setting privacy audio EC ack
> > > value: %d\n",status);
> > > > +        return -EIO;
> > > > +    }
> > > > +    return 0;
> > > > +}
> > >
> > > What's actually being set here? You don't seem to be passing any
> arguments.
> >
> > Yes,  it is a EC ack notification without any arguments needed.
> 
> I'm confused why it's being exposed as an LED device in that case -
> there's an expectation that this is something that actually controls a
> real LED, which means responding to state. Are you able to share the
> acpidump of a machine with this device?
> 
> --

Matthew,

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 leading to a
poor user experience.

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.  Later follow up work is already envisioned that if HW
mute is already enacted but SW mute is modified (IE LED notifier goes
through) that a message can come back out to userspace to tell the user
something along the lines of

"Your laptop mic is muted, press fn+f4 to unmute". 

I don't believe that will be part of the first commits to land, but that's
why an LED is used, to know the state of SW.

Perry,

Some suggestions for v2:
* You should better explain this hardware design in the commit
message.
* I think the codec changes should be in same patch series as 1/2 and this
be 2/2 rather than them going separately.  It won't make sense for one of them
to go in without the other.  For example if codec change goes in and dell-laptop
driver tries to change legacy LED it won't do anything.  And if this goes in
but codec driver doesn't, nothing will ever send EC ack.