Messages in this thread | | | Date | Fri, 03 Aug 2012 17:30:48 +0200 | From | Yann Cantin <> | Subject | Re: [RFC ebeam PATCH v2 3/3] input: misc: New USB eBeam input driver. |
| |
Hi,
>> +#include <linux/hid.h> > > As this driver is not a HID bus driver, why do you need this include?
Cinder, removed
>> +#define DRIVER_VERSION "v0.7" > > I don't think we need to be tracking driver versions for newly submitted > drivers, git is much better at tracking changes.
Old habit, removed.
>> + u16 X, Y; /* raw coordinates */ >> + int x, y; /* computed coordinates */ > > X,x being different fields seems confusing to me. How about, let's say, x, > raw_x?
Done.
>> +DEVICE_H_ATTR(1); >> +DEVICE_H_ATTR(2); >> +DEVICE_H_ATTR(3); >> +DEVICE_H_ATTR(4); >> +DEVICE_H_ATTR(5); >> +DEVICE_H_ATTR(6); >> +DEVICE_H_ATTR(7); >> +DEVICE_H_ATTR(8); >> +DEVICE_H_ATTR(9); > > You are adding a number of sysfs files. If they are really necessary, > you'll probably need to document those in Documentation/ABI.
Will do, in testing i suppose.
BTW : The driver need lot of parameters to be passed from user-space calibration tool. The best way to do it isn't decided yet : one sysfs file per parameter, or one sysfs file for all, with a big sscanf parsing. Any idea ?
>> + strlcat(ebeam->name, ")", sizeof(ebeam->name)); > > I'd suggest checking the length, making sure that you don't overflow the > ->name buffer.
Something like this ? :
if (strlcat(ebeam->name, ")", sizeof(ebeam->name))>=sizeof(ebeam->name)) { // overflowed, closing ) anyway ebeam->name[sizeof(ebeam->name)-2] = ')';
Thanks. -- Yann Cantin A4FEB47F --
| |