Re: [External] Re: [PATCH 0/3] Lenovo lap and palm sensor support

From: Mark Pearson <markpearson@lenovo.com>
To: Hans de Goede <hdegoede@redhat.com>
Cc: <njoshi1@lenovo.com>, <dmitry.torokhov@gmail.com>,
	<platform-driver-x86@vger.kernel.org>,
	<linux-input@vger.kernel.org>, <jeff@labundy.com>,
	<anthony.wong@canonical.com>, <hadess@hadess.net>
Subject: Re: [External] Re: [PATCH 0/3] Lenovo lap and palm sensor support
Date: Mon, 19 Oct 2020 20:15:03 -0400	[thread overview]
Message-ID: <66b2ded0-44bc-759a-339b-91c25bf30b7f@lenovo.com> (raw)
In-Reply-To: <c49d5d8a-4c20-f130-8218-1928beb3b565@redhat.com>

On 19/10/2020 14:49, Hans de Goede wrote:
> Hi,
> 
> On 10/15/20 3:57 PM, Mark Pearson wrote:
>> This patch series is to add support for the Lenovo lap and palm sensors.
>> The original lap sensor implementation used a sysfs API but after
>> consultation with the kernel maintainers we agreed on using the input
>> subsystem instead.
>> The first patch just adds the new defines needed.
>> The second patch adds the implementation needed for the palm sensor.
>> The third patch adds the implementation needed for the lap sensor.
>>
>> This means currently thinkpad_acpi.c has both the sysfs and input dev
>> implementations. I will add a follow-on patch to remove the sysfs
>> interface once I've confirmed this is OK with the few people who are
>> using this in user space and given them some time to migrate to the
>> input dev implementation.
>>
>> Mark Pearson (3):
>>    Adding event codes for Lenovo lap and palm sensors
>>    Add support for Lenovo palm sensor.
>>    Add support for Lenovo lap sensor
> 
> Not a full review, but one short remark, all your patch
> subjects e.g. "Adding event codes for Lenovo lap and palm sensors"
> are missing subsystem prefixes, if you do e.g.
> 
> git log include/uapi/linux/input-event-codes.h
> 
> You see subjects like "Input: allocate keycode for Fn + right shift",
> etc. and for the the thinkpad_acpi.c code you get:
> "platform/x86: thinkpad_acpi: Map Clipping tool hotkey to KEY_SELECTIVE_SCREENSHOT"
> 
> So your patch subjects should look something like this:
> 
> "Input: add event codes for lap and palmrest proximity switches"
> (note I fixed more here then just the missing prefix)
> 
> "platform/x86: thinkpad_acpi: Add support for Lenovo palm sensor"
> (note no . at the end)
> 
> "platform/x86: thinkpad_acpi: Add support for Lenovo lap sensor"
> 
> If you can send out a v2 with this fixed, that might help to
> go Dmitry's attention for the first patch.
> 
> Regards,
> 
> Hans
> 
Thanks Hans,
I knew about that too detail, so slap on the wrist for me. I was too 
focused on doing a series of patches for the first time...

Updated version coming out shortly.

Mark
Mark