Re: [RFC ebeam PATCH v2 3/3] input: misc: New USB eBeam input driver.

On Fri, 3 Aug 2012, Yann Cantin wrote:

quoted

quoted

+#include <linux/hid.h>

As this driver is not a HID bus driver, why do you need this include?

Cinder, removed

quoted

quoted

+#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.

quoted

quoted

+	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.
 

quoted

quoted

+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.

quoted

quoted

+		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