Messages in this thread | | | Date | Mon, 13 Aug 2012 11:06:41 +0200 (CEST) | From | Jiri Kosina <> | Subject | Re: [RFC ebeam PATCH v2 3/3] input: misc: New USB eBeam input driver. |
| |
On Fri, 3 Aug 2012, Yann Cantin wrote:
> >> +#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 ?
Sysfs always had a rule one value per file, so please stick to that.
> >> + 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] = ')';
Some variation on that, yes.
Thanks,
-- Jiri Kosina SUSE Labs
| |