On Fri, 18 Jul 2014 10:35:35 +0100 (BST)
Jamie Lentin <jm@xxxxxxxxxxxx> wrote:
> On Mon, 14 Jul 2014, Antonio Ospite wrote:
>
> > On Sun, 13 Jul 2014 08:24:19 +0100
> > Jamie Lentin <jm@xxxxxxxxxxxx> wrote:
> >
> >> Rename module and all functions within so we can add support for other
> >> keyboards in the same file. Rename the _tp postfix to _tpkbd, to
> >> signify functions relevant to the TP USB keyboard.
> >>
> >> Signed-off-by: Jamie Lentin <jm@xxxxxxxxxxxx>
> >
> > Reviewed-by: Antonio Ospite <ao2@xxxxxx>
> >
> > Patch looks good to me, easier to validate now :)
> >
> > A few ideas below for some possible _future_ cleanups, but no reason to
> > hold this one any longer IMHO.
> >
> > Ciao,
> > Antonio
> >
> >> ---
> >> ...er-hid-lenovo-tpkbd => sysfs-driver-hid-lenovo} | 0
> >> drivers/hid/Kconfig | 14 +-
> >> drivers/hid/Makefile | 2 +-
> >> drivers/hid/hid-core.c | 2 +-
> >> drivers/hid/{hid-lenovo-tpkbd.c => hid-lenovo.c} | 185 +++++++++++----------
> >> 5 files changed, 102 insertions(+), 101 deletions(-)
> >> rename Documentation/ABI/testing/{sysfs-driver-hid-lenovo-tpkbd => sysfs-driver-hid-lenovo} (100%)
> >> rename drivers/hid/{hid-lenovo-tpkbd.c => hid-lenovo.c} (64%)
> >>
[...]
> >> -static int tpkbd_features_set(struct hid_device *hdev)
> >> +static int lenovo_features_set_tpkbd(struct hid_device *hdev)
> >> {
> >> struct hid_report *report;
> >> - struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
> >> + struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
> >
> >
> > I think the word "pointer" here is misleading, sometimes in other parts
> > of the driver it is used as a synonym of trackpoint, but
> > lenovo_drvdata_tpkbd also contains data about leds, so here it looks
> > like it refers to C pointers. I'd call the variable tpkbd_data or
> > tpkbd_drvdata, throughout the driver. What do you think?
>
> Yes, the rationale behind the name really isn't obvious but all meanings
> are sorta correct, so it's not that bad :P
>
> I like tpkbd_drvdata better though, and can correct it in the compact
> keyboard code too. I wanted to make the variable names match, but didn't
> get around to it, so will do it next time around.
>
IMHO you can very well use data_pointer for now so the names are
consistent with the current ones; a cleanup can be done later to make
the naming more straightforward too.
> > And the driver could standardize on the word "trackpoint" where
> > appropriate in order to eliminate any ambiguity.
>
> The key other place I can think of that this happens is for the USB
> compact keyboard, for it's HID_TYPE_USBMOUSE half (versus the keyboard
> half). Saying trackpoint there is a mis-truth, since it's also where all
> the extra keys hide. But I'll have a look through anyway.
>
I meant: where "pointer" means trackpoint let's just use "trackpoint",
where pointer means "not keyboard" (like in data_pointer apparently)
something else can be used if "trackpoint" is too specific.
But again, this is cleanup: it'd be nice to have it eventually but not
worth blocking this patchset.
Thanks,
Antonio
--
Antonio Ospite
http://ao2.it
A: Because it messes up the order in which people normally read text.
See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
