Re: [PATCH v2 4/5] platform/x86: intel-vbtn: support KEY_ROTATE_LOCK_TOGGLE

From: Darren Hart <dvhart@infradead.org>
To: "Stefan Brüns" <stefan.bruens@rwth-aachen.de>
Cc: Bastien Nocera <hadess@hadess.net>,
	platform-driver-x86@vger.kernel.org, linux-input@vger.kernel.org,
	AceLan Kao <acelan.kao@canonical.com>,
	Andy Shevchenko <andy@infradead.org>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 4/5] platform/x86: intel-vbtn: support KEY_ROTATE_LOCK_TOGGLE
Date: Thu, 9 Nov 2017 16:53:07 -0800	[thread overview]
Message-ID: <20171110005307.GH21449@fury> (raw)
In-Reply-To: <6070680.Ajd9sKXNqZ@pebbles>

On Fri, Nov 10, 2017 at 01:15:09AM +0100, Stefan Brüns wrote:
> On Friday, November 10, 2017 12:30:46 AM CET Bastien Nocera wrote:
> > On Thu, 2017-11-09 at 23:44 +0100, Stefan Brüns wrote:
> > > The Rotate Lock button event is emitted on the XPS 12 (BIOS A8, but
> > > not
> > > on BIOS A2).
> > > 
> > > Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
> > > ---
> > > 
> > > Changes in v2:
> > > - Emit KEY_ROTATE_LOCK_TOGGLE instead of KEY_ROTATE_DISPLAY
> > > - Use separate up/down events
> > > 
> > >  drivers/platform/x86/intel-vbtn.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/platform/x86/intel-vbtn.c
> > > b/drivers/platform/x86/intel-vbtn.c index e3f6375af85c..a484bcc6393b
> > > 100644
> > > --- a/drivers/platform/x86/intel-vbtn.c
> > > +++ b/drivers/platform/x86/intel-vbtn.c
> > > @@ -42,6 +42,8 @@ static const struct key_entry intel_vbtn_keymap[] = {
> > > 
> > >  	{ KE_IGNORE, 0xC5, { KEY_VOLUMEUP } },		/* volume-up key release */
> > >  	{ KE_KEY, 0xC6, { KEY_VOLUMEDOWN } },		/* volume-down key press */
> > >  	{ KE_IGNORE, 0xC7, { KEY_VOLUMEDOWN } },	/* volume-down key release 
> */
> > > 
> > > +	{ KE_KEY,    0xC8, { KEY_ROTATE_LOCK_TOGGLE } },	/* rotate-lock key
> > > press */ +	{ KE_KEY,    0xC9, { KEY_ROTATE_LOCK_TOGGLE } },	/*
> > > rotate-lock key release */
> > How are those events sent? When pressing and releasing the key, do you
> > receive 0xC8 followed by 0xC9? Or do you receive 0xC8 when pressing and
> > releasing the first time, and 0xC9 when pressing and releasing a second
> > time?
> > 
> > If the former, then it's not going to work. The release is supposed to
> > be ignored, as you send the event with sparse_keymap_report_event().
> > 
> > If the latter, and there's an actual state, does it disable a device
> > on-board, or activate an LED? If so, then it would need to be a switch,
> > not a key.
> 
> Do you think I don't test the patches before sending? Let me tell you, it 
> *does* work.
> 
> You could also read the cover letter, where you find more details, putting the 
> patches in relation to each other.

Dude, Knock it off. Unacceptable.

The number of patches we receive which have not seen any testing are
higher than you might think. Bastien took the time to review and ask
questions in the context of his experience with patches dealing with
input. These are very reasonable and necessary questions given we
cannot independently verify patches for most of these platforms.

-- 
Darren Hart
VMware Open Source Technology Center