| Message ID | 20201015135717.384610-1-markpearson@lenovo.com |
|---|---|
| Headers | show |
| Series | Lenovo lap and palm sensor support | expand |
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
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