Message-ID: <4e846cf1-e7c7-3bb9-d8b3-d266b9dfbc4e@linux.intel.com>
Date: Thu, 26 Jun 2025 18:09:47 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Vishnu Sankar <vishnuocv@...il.com>
cc: pali@...nel.org, dmitry.torokhov@...il.com, hmh@....eng.br, 
    hansg@...nel.org, tglx@...utronix.de, mingo@...nel.org, jon_xie@...art.com, 
    jay_lee@...art.com, zhoubinbin@...ngson.cn, linux-input@...r.kernel.org, 
    LKML <linux-kernel@...r.kernel.org>, ibm-acpi-devel@...ts.sourceforge.net, 
    platform-driver-x86@...r.kernel.org, vsankar@...ovo.com, 
    Mark Pearson <mpearson-lenovo@...ebb.ca>
Subject: Re: [PATCH] x86/Mouse: thinkpad_acpi/Trackpoint: Trackpoint Doubletap
 handling

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@...ux.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".
>       > + */
>       > +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()?


>       > +     return sysfs_emit(buf, "%s\n", status ? "enabled" : "disabled");
> 
>       I'm sure there's an existing helper for this bool -> "enabled"/"disabled"
>       conversion (make sure you add the #include if not yet there when you use
>       the helper).
> 
> Is "bool_to_enabled_disabled(status)" the one that you are referring to?

Please see linux/string_choices.h, but after you change the attribute 
name, this won't be required as you can use normal boolean style with 0/1.

>       > +}
>       > +
>       > +static ssize_t tp_doubletap_store(struct device *dev, struct
>       device_attribute *attr, const char *buf, size_t count)
>       > +{
>       > +     if (sysfs_streq(buf, "enable")) {
> 
>       Hmm, should this attribute be named doubletap_enabled and follow the
>       usual
>       boolean convention instead?
> 
> Will change the attribute name as suggested. 

Please also use kstrtobool() then to convert the value.

>       > +             /* enabling the doubletap here */
>       > +             if (!trackpoint_set_doubletap(true))
>       > +                     tp_features.trackpoint_doubletap_state = true;
>       > +     } else if (sysfs_streq(buf, "disable")) {
>       > +             /* disabling the doubletap here */
>       > +             if (!trackpoint_set_doubletap(false))
>       > +                     tp_features.trackpoint_doubletap_state = false;
>       > +     } else {
>       > +             pr_err("ThinkPad ACPI: thinkpad_acpi: Invalid value '%s'
>       for tp_doubletap\n", buf);
> 
>       No, sysfs store function should not spam log like this, returning -EINVAL
>       tells the very same thing already.
> 
> Acked.
> will be removing the log. 
> 
>       > +             return -EINVAL;
>       > +     }
>       > +
>       > +     return count;
>       > +}


-- 
 i.

Powered by blists - more mailing lists