On Wednesday 10 December 2014 18:17:40 Benjamin Tissoires wrote: > On Dec 11 2014 or thereabouts, Peter Wu wrote: > > On Wednesday 10 December 2014 17:21:10 Benjamin Tissoires wrote: > > > Current names are reported as "K750", "M705", and it can be misleading > > > for the users when they look at their input device list. > > > > > > Prefixing the names with "Logitech " makes things better. > > > > Doesn't this apply to all input devices? Like, every USB device can be > > queried for the manufacturer which could then by included by the input > > subsystem. > > Yes and no. Yes, it's good to have the manufacturer name in the input > subsystem, and no, because usbhid already prepend the name with the > manufacturer. I see, previously (and now, if the device name cannot be retrieved because one unplugs the receiver during probe which I have actually tested) a string such as "Logitech Unifying Device. Wireless PID:XXX" shows up. Now the only string visible is "M512". Speaking of errors retrieving the name, if a device is turned off then the name cannot be queried using HID++ 2.0 (think of "T650" instead of "Wireless Rechargeable Touchpad T650"). Wouldn't this confuse userspace applications which rely on a constant name? > > > > What about duplicate names? Can this patch cause a conflict with devices > > having the same name? > > I am not sure what you mean here. I am adding a prefix to the name, so I > do not see how a conflict can be possible. If there were a "M705" and a > "Logitech M705", with different wireless PID, that would be worrisome to > some extend. > But I am pretty sure this will not happen. I thought that this name would be used for /dev/bus/hid/devices/XXX and therefore had to be unique, but this is not the case (the name is available under the input device). No worries then! > > > > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx> > > > --- > > > > > > Hi Jiri, > > > > > > I'd love to see this one in 3.19 (after a strong review, of course). > > > The idea came with the mouse DPI database that Peter is currently working on > > > (see http://who-t.blogspot.com.au/2014/12/building-a-dpi-database-for-mice.html). > > > > > > I think, if you do not qualify the series for 3.19, we should drop it entirely. > > > 3.19 introduces the hidpp module and changes the labels. The DPI hwdb will check > > > on the label to match the actual mouse, so if this one does not get into 3.19, > > > we will end up in 3 entries for each Logitech device. > > > > > > Cheers, > > > Benjamin > > > > > > > > > drivers/hid/hid-logitech-hidpp.c | 34 ++++++++++++++++++++++++++++++++++ > > > 1 file changed, 34 insertions(+) > > > > > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c > > > index 3846305..8cf4352 100644 > > > --- a/drivers/hid/hid-logitech-hidpp.c > > > +++ b/drivers/hid/hid-logitech-hidpp.c > > > @@ -282,6 +282,33 @@ static inline bool hidpp_report_is_connect_event(struct hidpp_report *report) > > > (report->rap.sub_id == 0x41); > > > } > > > > > > +/** > > > + * hidpp_prefix_name() prefixes the current given name with "Logitech ". > > > + */ > > > +static void hidpp_prefix_name(char **name, int name_length) > > > +{ > > > +#define PREFIX_SIZE 9 /* "Logitech " */ > > > + > > > + int new_length; > > > + char *new_name; > > > + > > > + if (name_length > PREFIX_SIZE && > > > + strncmp(*name, "Logitech ", PREFIX_SIZE) == 0) > > > > I think you meant || here, not &&. > > No, I meant &&. The idea is to not add a prefix if the provided name > already contains the prefix. Read that as "if the size of the name may > contain the prefix and that the prefix is here, then we bail out". Ah, I somehow read it as "if the name is longer than what can be stored". > > > > > + /* The prefix has is already in the name */ > > > + return; > > > + > > > + new_length = name_length + PREFIX_SIZE; > > > > Stylistic note, but 'PREFIX_SIZE + name_length' would match the order of > > the string that gets prepended. > > Works for me. > > I will send a v2 with your comments addressed. Thanks! Kind regards, Peter > Cheers, > Benjamin > > > > > Kind regards, > > Peter > > > > > + new_name = kzalloc(new_length, GFP_KERNEL); > > > + if (!new_name) > > > + return; > > > + > > > + snprintf(new_name, new_length, "Logitech %s", *name); > > > + > > > + kfree(*name); > > > + > > > + *name = new_name; > > > +} > > > + > > > /* -------------------------------------------------------------------------- */ > > > /* HIDP++ 1.0 commands */ > > > /* -------------------------------------------------------------------------- */ > > > @@ -318,6 +345,10 @@ static char *hidpp_get_unifying_name(struct hidpp_device *hidpp_dev) > > > return NULL; > > > > > > memcpy(name, &response.rap.params[2], len); > > > + > > > + /* include the terminating '\0' */ > > > + hidpp_prefix_name(&name, len + 1); > > > + > > > return name; > > > } > > > > > > @@ -489,6 +520,9 @@ static char *hidpp_get_device_name(struct hidpp_device *hidpp) > > > feature_index, index, name + index, > > > __name_length - index); > > > > > > + /* include the terminating '\0' */ > > > + hidpp_prefix_name(&name, __name_length + 1); > > > + > > > return name; > > > > > > out_err: > > > -- 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
