Re: [PATCH v4 1/4] Rename hid-lenovo-tpkbd to hid-lenovo

[Antonio Ospite <ao2@ao2.it>]

On Fri, 18 Jul 2014 10:35:35 +0100 (BST)
Jamie Lentin <jm@lentin.co.uk> wrote:

> On Mon, 14 Jul 2014, Antonio Ospite wrote:
> 
> > On Sun, 13 Jul 2014 08:24:19 +0100
> > Jamie Lentin <jm@lentin.co.uk> 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@lentin.co.uk>
> >
> > Reviewed-by: Antonio Ospite <ao2@ao2.it>
> >
> > 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?