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 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.