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". > > + */ > > +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.