Re: [PATCH] x86/Mouse: thinkpad_acpi/Trackpoint: Trackpoint Doubletap handling

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 7380 bytes --]

On Fri, 27 Jun 2025, Vishnu Sankar wrote:

> Hi Ilpo,
> 
> Thanks a lot for the review.
> 
> On Fri, Jun 27, 2025 at 12:09 AM Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:
>       On Thu, 26 Jun 2025, Vishnu Sankar wrote:
> 
>       > Hi Ilpo,
>       >
>       > Thanks a lot for the comments and guidance.
>       > Please find my comments inline below.
>       >
>       > On Wed, Jun 25, 2025 at 9:07 PM Ilpo Järvinen <
>       ilpo.jarvinen@linux.intel.com >
>       > wrote:
>       >       On Fri, 20 Jun 2025, Vishnu Sankar wrote:
>       >
>       >       I don't like the shortlog prefixes (in the subject), please don't
>       make
>       >       confusing prefixes up like that.
>       >
>       > Got it.
>       > I will correct this in V2 (as a patch series). 
>       >
>       >       > Newer ThinkPads have a doubletap feature that needs to be
>       turned
>       >       > ON/OFF via the trackpoint registers.
>       >
>       >       Don't leave lines short mid-paragraph. Either reflow the
>       paragraph or add
>       >       an empty line in between paragraphs.
>       >
>       > Acked.
>       > Will correct this in V2.  
>       >
>       >       > Systems released from 2023 have doubletap disabled by default
>       and
>       >       > need the feature enabling to be useful.
>       >       >
>       >       > This patch introduces support for exposing and controlling the
>       >       > trackpoint doubletap feature via a sysfs attribute.
>       >       > /sys/devices/platform/thinkpad_acpi/tp_doubletap
>       >       > This can be toggled by an "enable" or a "disable".
>       >
>       >       This too has too short lines.
>       >
>       > Sorry! 
>       > Will do the needful in v2.
>       >
>       >       >
>       >       > With this implemented we can remove the masking of events, and
>       rely on
>       >
>       >       "With this implemented" is way too vague wording.
>       >
>       > Sorry for this!
>       > I will make this better. 
>       >
>       >       > HW control instead, when the feature is disabled.
>       >       >
>       >       > Note - Early Thinkpads (pre 2015) used the same register for
>       hysteris
>       >
>       >       hysteresis ?
>       >
>       > Sorry for not being clear.
>       > It's the trackpoint drag hysteris functionality. Pre-2015 ThinkPads
>       used to have a
>       > user-configurable drag hysterisis register (0x58).
>       > Drag hysterisis is not user configurable now.
>       >
>       >       > control, Check the FW IDs to make sure these are not affected.
>       >
>       >       This Note feels very much disconnected from the rest. Should be
>       properly
>       >       described and perhaps in own patch (I don't know)?
>       >
>       > my bad, it's not FW ID, but PnP ID. 
>       > The older ThinkPad's trackpoint controllers use the same register
>       (0x58) to control
>       > the drag hysteresis, which is obsolete now.
>       > Now (on newer systems from 2023) the same register (0x58) is remapped
>       as
>       > doubletap register.  
>       > Just to exclude those older systems (with drag hysterisis control), we
>       check the PnP
>       > ID's.
>       >
>       > I will give a better commit message in V2.
>       >
>       >       > trackpoint.h is moved to linux/input/.
>       >
>       >       This patch doesn't look minimal and seems to be combining many
>       changes
>       >       into one. Please make a patch series out of changes that can be
>       separated
>       >       instead of putting all together.
>       >
>       > Understood.
>       > Will make a patch series on V2
>       > 0001: Move trackpoint.h to include/linux/input
>       > 0002: Add new doubletap set/status/capable logics to trackpoint.c
>       > 0003: Add new trackpoint api's and trackpoint sysfs in thinkpad_acpi.c
> 
>       Okay, sounds better.
> 
>       >       > +/* List of known incapable device PNP IDs */
>       >       > +static const char * const dt_incompatible_devices[] = {
>       >       > +     "LEN0304",
>       >       > +     "LEN0306",
>       >       > +     "LEN0317",
>       >       > +     "LEN031A",
>       >       > +     "LEN031B",
>       >       > +     "LEN031C",
>       >       > +     "LEN031D",
>       >       > +};
>       >       > +
>       >       > +/*
>       >       > + * checks if it’s a doubletap capable device
>       >       > + * The PNP ID format eg: is "PNP: LEN030d PNP0f13".

There's case difference between this comment and the list.

>       >       > + */
>       >       > +bool is_trackpoint_dt_capable(const char *pnp_id)
>       >       > +{
>       >       > +     char id[16];
>       >       > +
>       >       > +     /* Make sure string starts with "PNP: " */
>       >       > +     if (strncmp(pnp_id, "PNP: LEN03", 10) != 0)
>       >
>       >       We seem to have strstarts().
>       >
>       > Thanks a lot for the suggestion.
>       > Will make use of this. 
>       >
>       >       > +             return false;
>       >       > +
>       >       > +     /* Extract the first word after "PNP: " */
>       >       > +     if (sscanf(pnp_id + 5, "%15s", id) != 1)
>       >       > +             return false;
>       >       > +
>       >       > +     /* Check if it's blacklisted */
>       >       > +     for (size_t i = 0; i <
>       ARRAY_SIZE(dt_incompatible_devices); ++i)
>       >       {
>       >       > +             if (strcmp(pnp_id, dt_incompatible_devices[i]) ==
>       0)
>       >
>       >       Isn't this buggy wrt. the PNP: prefix??
>       >
>       >       Perhaps it would have been better to just do pnp_id += 5 before
>       sscanf()
>       >       as you don't care about the prefix after that.
>       >
>       >
>       > Understood.
>       > Shall we have something like the following:
>       >         if (!strstarts(pnp_id, "PNP: LEN03"))
>       >               return false;
>       >
>       >         id = pnp_id + 5;
>       >
>       >         for (size_t i = 0; i < ARRAY_SIZE(dt_incompatible_devices);
>       ++i) {
>       >                        if (strncmp(id, dt_incompatible_devices[i],
>       > strlen(dt_incompatible_devices[i])) == 0)
> 
>       Why did you change to strncmp()?
> 
> I switched to strncmp() to allow matching just the known prefix part in
> dt_incompatible_devices, rather than requiring an exact full string match.
> Will keep the original "if (strcmp(id, dt_incompatible_devices[i]) == 0) " logic as
> it serves the purpose.

I didn't mean to say the change is necessarily incorrect, I was just 
asking for reasonale as it was different from the original.

If you think you it needs to be broader to the match to a prefix only, 
I've no problem with that.

-- 
 i.