[RFC] surface-input

[RFC] surface-input

[Florian Echtler]

[-- Attachment #1.1: Type: text/plain, Size: 831 bytes --]

Hello everyone,

as mentioned earlier, I'm currently writing a multitouch input driver
for the Pixelsense (formerly Surface 2.0). It's now at a point where I'd
consider it mostly done, but a) I have very limited experience with
kernel drivers and b) there are some additional questions I have, so I'm
attaching the current state of my source code and would like to ask for
your comments.

Open question: it looks like just calling

input_mt_init_slots(poll_dev->input, MAX_CONTACTS, INPUT_MT_DIRECT);

in the initialization routine isn't enough. hid-multitouch doesn't seem
to use any other init commands, though, or did I overlook something?

Are there any other caveats of the input-mt library which I should be
aware of?

Thanks for your input, and best regards, Florian
-- 
SENT FROM MY DEC VT50 TERMINAL

[-- Attachment #1.2: Makefile --]
[-- Type: text/plain, Size: 1058 bytes --]

# Assumptions:
# 	This makefile assumes that kernel modules are installed in 
#	      /lib/modules/'uname -r'/.
#	Should the assumption be incorrect, please use:
#	    make MODULE_ROOT="/path/to/modules/for/your/kernel"
#	to override assumed path

MODULE=surface-input

srcs := $(MODULE).c
obj-m := $(srcs:%.c=%.o)

MODULE_ROOT:= /lib/modules/$(shell uname -r)
BUILDDIR   := $(MODULE_ROOT)/build
MODDIR     := $(MODULE_ROOT)/updates
PWD        := $(shell pwd)

override install_targets := $(srcs:%.c=install_%)

default:
	$(MAKE) -C $(BUILDDIR) SUBDIRS=$(PWD) "obj-m=$(obj-m)" modules

.PHONY: default install all clean help $(install_targets)

install:
	install -m 644 $(MODULE).ko $(MODDIR)
	depmod -a
	modprobe -r $(MODULE)
	modprobe $(MODULE)

debug:
	@echo
	@echo "srcs="$(srcs)
	@echo "obj-m="$(obj-m)
	@echo "MODULE_ROOT="$(MODULE_ROOT)
	@echo "PWD="$(PWD)
	@echo "install targets: " $(install_targets)
	@echo

clean:
	-rm -rf *.o *.ko $(MODULE)*.mod.c Module.symvers .$(MODULE)* .tmp_versions modules.order


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.3: surface-data.h --]
[-- Type: text/x-chdr; name="surface-data.h", Size: 2533 bytes --]

/* 
  Surface2.0/SUR40/PixelSense input driver v0.0.1

  This program is free software; you can redistribute it and/or
  modify it under the terms of the GNU General Public License as
  published by the Free Software Foundation; either version 2 of
  the License, or (at your option) any later version.

  Copyright (C) 2013 by Florian 'floe' Echtler <floe@butterbrot.org>

  Derived from the USB Skeleton driver 1.1,
  Copyright (C) 2003 Greg Kroah-Hartman (greg@kroah.com)

	Derived from the Apple USB BCM5974 multitouch driver,
	Copyright (C) 2008 Henrik Rydberg (rydberg@euromail.se)
*/

#ifndef _SURFACE_DATA_H_
#define _SURFACE_DATA_H_


#define ID_MICROSOFT 0x045e
#define ID_SURFACE   0x0775

#define VIDEO_RES_X 960
#define VIDEO_RES_Y 540

#define VIDEO_BUFFER_SIZE  VIDEO_RES_X * VIDEO_RES_Y


// read 512 bytes from endpoint 0x86 -> get header + blobs
struct surface_header {

	uint16_t type;  // always 0x0001
	uint16_t count; // count of blobs (if == 0: continue prev. packet ID)

	uint32_t packet_id;

	uint32_t timestamp; // milliseconds (increases by 16 or 17 each frame)
	uint32_t unknown;   // "epoch?" always 02/03 00 00 00 

};


struct surface_blob {

	uint16_t blob_id;

	uint8_t action;     // 0x02 = enter/exit, 0x03 = update (?) 
	uint8_t unknown;    // always 0x01 or 0x02 (no idea what this is?)

	uint16_t bb_pos_x;  // upper left corner of bounding box
	uint16_t bb_pos_y;

	uint16_t bb_size_x; // size of bounding box
	uint16_t bb_size_y;

	uint16_t pos_x;     // finger tip position
	uint16_t pos_y;

	uint16_t ctr_x;     // centroid position
	uint16_t ctr_y; 

	uint16_t axis_x;    // somehow related to major/minor axis, mostly:
	uint16_t axis_y;    // axis_x == bb_size_y && axis_y == bb_size_x 

	float    angle;     // orientation in radians relative to x axis
	uint32_t area;      // size in pixels/pressure (?)

	uint8_t padding[32];
};


// read 512 bytes from endpoint 0x82 -> get header below
// continue reading 16k blocks until header.size bytes read
struct surface_image {
	uint32_t magic;     // "SUBF"
	uint32_t packet_id;
	uint32_t size;      // always 0x0007e900 = 960x540
	uint32_t timestamp; // milliseconds (increases by 16 or 17 each frame)
	uint32_t unknown;   // "epoch?" always 02/03 00 00 00 
};


// read 8 bytes using control message 0xc0,0xb1,0x00,0x00
struct surface_sensors {
	uint16_t temp;
	uint16_t acc_x;
	uint16_t acc_y;
	uint16_t acc_z;
};


#endif // _SURFACE_DATA_H_


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.4: surface-input.c --]
[-- Type: text/x-csrc; name="surface-input.c", Size: 11285 bytes --]

/* 
  Surface2.0/SUR40/PixelSense input driver v0.0.1

  This program is free software; you can redistribute it and/or
  modify it under the terms of the GNU General Public License as
  published by the Free Software Foundation; either version 2 of
  the License, or (at your option) any later version.

  Copyright (C) 2013 by Florian 'floe' Echtler <floe@butterbrot.org>

  Derived from the USB Skeleton driver 1.1,
  Copyright (C) 2003 Greg Kroah-Hartman (greg@kroah.com)

	Derived from the Apple USB BCM5974 multitouch driver,
	Copyright (C) 2008 Henrik Rydberg (rydberg@euromail.se)
*/

#include <linux/kernel.h>
#include <linux/errno.h>
#include <linux/delay.h>
#include <linux/init.h>
#include <linux/slab.h>
#include <linux/module.h>
#include <linux/completion.h>
#include <asm/uaccess.h>
#include <linux/usb.h>
#include <linux/printk.h>
#include <linux/input-polldev.h>
#include <linux/input/mt.h>
#include <linux/usb/input.h>

#include "surface-data.h"

/* version information */
#define DRIVER_VERSION "0.0.1"
#define DRIVER_SHORT   "surface-input"
#define DRIVER_AUTHOR  "Florian 'floe' Echtler <floe@butterbrot.org>"
#define DRIVER_DESC    "Surface2.0/SUR40/PixelSense input driver"

/* vendor and device IDs */
#define ID_MICROSOFT 0x045e
#define ID_SURFACE   0x0775

/* touch data endpoint */
#define TOUCH_ENDPOINT 0x86

/* polling interval (ms) */
#define POLL_INTERVAL 10

/* maximum number of contacts */
#define MAX_CONTACTS 64

/* device ID table */
static const struct usb_device_id surface_table[] = {
	{USB_DEVICE(ID_MICROSOFT, ID_SURFACE)}, /* Microsoft Surface 2.0  */
	{}                                      /* terminating null entry */
};

/* control commands */
#define SURFACE_GET_VERSION 0xb0 /* 12 bytes string    */
#define SURFACE_UNKNOWN1    0xb3 /*  5 bytes           */
#define SURFACE_UNKNOWN2    0xc1 /* 24 bytes           */

#define SURFACE_GET_STATE   0xc5 /*  4 bytes state (?) */
#define SURFACE_GET_SENSORS 0xb1 /*  8 bytes sensors   */

/* FIXME: previous version used USB_RECIP_ENDPOINT, may have corrupted eeprom? */
#define surface_command(dev, command, index, buffer, size) \
	usb_control_msg (dev->usbdev, usb_rcvctrlpipe (dev->usbdev, 0), command, \
	USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN, 0x00, index, buffer, size, 1000)

MODULE_DEVICE_TABLE(usb, surface_table);

/* structure to hold all of our device specific stuff */
struct usb_surface {

	struct usb_device *usbdev; /* save the usb device pointer */
	struct usb_interface *interface; /* the interface for this device */
	struct input_polled_dev *input; /* struct for polled input device */

	unsigned char *bulk_in_buffer; /* the buffer to receive data */
	size_t bulk_in_size; /* the maximum bulk packet size */
	__u8 bulk_in_endpointAddr; /* the address of the bulk in endpoint */
	char phys[64];
};

/* initialization routine, called from surface_open */
static int surface_init(struct usb_surface *dev)
{
	int result;
	__u8 buffer[24];

	/* stupidly replay the original MS driver init sequence */
	result = surface_command(dev, SURFACE_GET_VERSION, 0x00, buffer, 12 );
	if (result < 0)
		return result;

	result = surface_command(dev, SURFACE_GET_VERSION, 0x01, buffer, 12 );
	if (result < 0)
		return result;

	result = surface_command(dev, SURFACE_GET_VERSION, 0x02, buffer, 12 );
	if (result < 0)
		return result;

	result = surface_command(dev, SURFACE_UNKNOWN2,    0x00, buffer, 24 );
	if (result < 0)
		return result;

	result = surface_command(dev, SURFACE_UNKNOWN1,    0x00, buffer,  5 );
	if (result < 0)
		return result;

	result = surface_command(dev, SURFACE_GET_VERSION, 0x03, buffer, 12 );

	/* discard the result buffer - no known data inside except 
	some version strings, maybe extract these sometime..    */

	return result;
}

/*
 * callback routines from input_polled_dev
*/

/* enable the device, polling will now start */
void surface_open(struct input_polled_dev *polldev) {
	struct usb_surface *surface = polldev->private;
	pr_info("surface_open");
	surface_init(surface);
}

/* disable device, polling has stopped */
void surface_close(struct input_polled_dev *polldev) {
	/* no known way to stop the device, except to stop polling */
	pr_info("surface_close");
}

/*
 * this function is called when a whole contact has been processed,
 * so that it can assign it to a slot and store the data there
 */
static void report_blob(struct surface_blob *blob, struct input_dev *input)
{
	int wide, major, minor;

	int slotnum = input_mt_get_slot_by_key(input, blob->blob_id);
	if (slotnum < 0 || slotnum >= MAX_CONTACTS)
		return;

	/* FIXME: is this needed for the Pixelsense? */
	/*if ((td->mtclass.quirks & MT_QUIRK_IGNORE_DUPLICATES) && mt) {
		struct input_mt *mt = input->mt;
		struct input_mt_slot *slot = &mt->slots[slotnum];
		if (input_mt_is_active(slot) &&
				input_mt_is_used(mt, slot))
			return;
	}*/

	input_mt_slot(input, slotnum);
	input_mt_report_slot_state(input, MT_TOOL_FINGER, 1);
	wide = (blob->bb_size_x > blob->bb_size_y);
	major = max(blob->bb_size_x, blob->bb_size_y);
	minor = min(blob->bb_size_x, blob->bb_size_y);

	input_event(input, EV_ABS, ABS_MT_POSITION_X, blob->pos_x);
	input_event(input, EV_ABS, ABS_MT_POSITION_Y, blob->pos_y);
	input_event(input, EV_ABS, ABS_MT_TOOL_X, blob->ctr_x);
	input_event(input, EV_ABS, ABS_MT_TOOL_Y, blob->ctr_y);
	input_event(input, EV_ABS, ABS_MT_ORIENTATION, wide);
	input_event(input, EV_ABS, ABS_MT_TOUCH_MAJOR, major);
	input_event(input, EV_ABS, ABS_MT_TOUCH_MINOR, minor);
}


/* core function: poll for new input data */
void surface_poll(struct input_polled_dev *polldev) {

	struct usb_surface *surface = polldev->private;
	struct input_dev *input = polldev->input;
	int result, bulk_read, need_blobs, packet_blobs, i;
	uint32_t packet_id;

	struct surface_header* header = (struct surface_header*)(surface->bulk_in_buffer);
	struct surface_blob*   inblob = (struct surface_blob*)(surface->bulk_in_buffer+sizeof(struct surface_header));

	need_blobs = -1;

	pr_info("surface_poll\n");

	do {

		/* perform a blocking bulk read to get data from the device */
		result = usb_bulk_msg (surface->usbdev,
			usb_rcvbulkpipe (surface->usbdev, surface->bulk_in_endpointAddr),
			surface->bulk_in_buffer, 512, &bulk_read, 1000);

		pr_info("got %d bytes\n",bulk_read);

		if (result < 0) { 
			pr_err("error in usb_bulk_read");
			return;
		}

		result = bulk_read - sizeof(struct surface_header);

		if (result % sizeof(struct surface_blob) != 0) {
			pr_err("transfer size mismatch");
			return;
		}

		/* first packet? */
		if (need_blobs == -1) {
			need_blobs = header->count;
			pr_info("expecting %d blobs\n",need_blobs);
			packet_id = header->packet_id;
		}

		/* sanity check. when video data is also being retrieved, the packet ID
		will usually increase in the middle of a series instead of at the end. */
		if (packet_id != header->packet_id)
			pr_warn("packet ID mismatch\n");

		packet_blobs = result / sizeof(struct surface_blob);
		pr_info("got %d blobs\n",packet_blobs);

		/* packets always contain at least 4 blobs, even if they are empty */
		if (packet_blobs > need_blobs)
			packet_blobs = need_blobs;

		for (i = 0; i < packet_blobs; i++) {
			need_blobs--;
			pr_info("got a blob\n");
			report_blob( &(inblob[i]), input );
		}

	}	while (need_blobs > 0);

	input_mt_sync_frame(input);
	input_sync(input);
}

/*
 * power management routines
*/

/* PM operations are nops as this driver does IO only during open() */
/*static int surface_suspend(struct usb_interface *intf, pm_message_t message)
{
	return 0;
}

static int surface_resume(struct usb_interface *intf)
{
	return 0;
}*/

/*
 * housekeeping routines
*/

/* clean up all allocated buffers/structs */
static inline void surface_delete(struct usb_surface *surface)
{
	input_free_polled_device(surface->input);
	kfree(surface->bulk_in_buffer);
	kfree(surface);
}

/* check candidate USB interface */
static int surface_probe(struct usb_interface *interface,
				const struct usb_device_id *id)
{
	struct usb_device *usbdev = interface_to_usbdev(interface);
	struct usb_surface *surface;
	struct usb_host_interface *iface_desc;
	struct usb_endpoint_descriptor *endpoint;
	struct input_polled_dev* poll_dev;

	/* check if we really have the right interface */
	iface_desc = &interface->altsetting[0];
	if (iface_desc->desc.bInterfaceClass != 0xFF)
		return -ENODEV;

	/* allocate memory for our device state and initialize it */
	surface = kzalloc(sizeof(struct usb_surface), GFP_KERNEL);
	poll_dev = input_allocate_polled_device();
	surface->input = poll_dev;
	if (!surface || !poll_dev)
		return -ENOMEM;

	poll_dev->private = surface;
	poll_dev->poll_interval = POLL_INTERVAL;
	poll_dev->open  = surface_open;
	poll_dev->poll  = surface_poll;
	poll_dev->close = surface_close;

	poll_dev->input->name = "Samsung SUR40";
	usb_to_input_id(usbdev,&(poll_dev->input->id));
	usb_make_path(usbdev, surface->phys, sizeof(surface->phys));
	strlcat(surface->phys, "/input0", sizeof(surface->phys));
	poll_dev->input->phys = surface->phys;

	input_mt_init_slots(poll_dev->input, MAX_CONTACTS, INPUT_MT_DIRECT);

	surface->usbdev = usbdev;
	surface->interface = interface;

	/* use endpoint #4 (0x86) */
	endpoint = &iface_desc->endpoint[4].desc;
	if (endpoint->bEndpointAddress == TOUCH_ENDPOINT) {
		/* we found a bulk in endpoint */
		surface->bulk_in_size = le16_to_cpu(endpoint->wMaxPacketSize);
		surface->bulk_in_endpointAddr = endpoint->bEndpointAddress;
		surface->bulk_in_buffer = kmalloc(2*surface->bulk_in_size, GFP_KERNEL);
		if (!surface->bulk_in_buffer) {
			pr_err("Unable to allocate input buffer.");
			surface_delete(surface);
			return -ENOMEM;
		}
	}

	if (!(surface->bulk_in_endpointAddr)) {
		pr_err("Unable to find bulk-in endpoint.");
		surface_delete(surface);
		return -ENODEV;
	}

	/* we can register the device now, as it is ready */
	usb_set_intfdata(interface, surface);

	if (input_register_polled_device(poll_dev)) {
		pr_err("Unable to register polled input device.");
		surface_delete(surface);
		return -ENODEV;
	}

	dev_info(&interface->dev,"%s now attached\n",DRIVER_DESC);

	return 0;
}

/* unregister device & clean up */
static void surface_disconnect(struct usb_interface *interface)
{
	struct usb_surface *surface = usb_get_intfdata(interface);

	input_unregister_polled_device(surface->input);

	usb_set_intfdata(interface, NULL);

	surface_delete(surface);

	dev_info(&interface->dev, "%s now disconnected\n",DRIVER_DESC);
}

/* usb specific object needed to register this driver with the usb subsystem */
static struct usb_driver surface_driver = {
	.name = DRIVER_SHORT,
	.probe = surface_probe,
	.disconnect = surface_disconnect,
	/*.suspend = surface_suspend,
	.resume = surface_resume,*/
	.id_table = surface_table,
	/*.supports_autosuspend = 1,*/
};

module_usb_driver(surface_driver);

MODULE_AUTHOR(DRIVER_AUTHOR);
MODULE_DESCRIPTION(DRIVER_DESC);
MODULE_LICENSE("GPL");


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [RFC] surface-input

[Benjamin Tissoires]

Hi Florian,

On 05/09/13 16:15, Florian Echtler wrote:
> Hello everyone,
> 
> as mentioned earlier, I'm currently writing a multitouch input driver
> for the Pixelsense (formerly Surface 2.0). It's now at a point where I'd
> consider it mostly done, but a) I have very limited experience with
> kernel drivers and b) there are some additional questions I have, so I'm
> attaching the current state of my source code and would like to ask for
> your comments.

Thanks for sharing this with us.

I will not do a proper review right now (just coming back from some days off, so I am quickly emptying my mail box).

Some generic comments:
- please always inline the code in the message, it is *much* easier to review and comment it
- please directly use a patch format: if the code is good, Dmitry can take it directly through his tree
- add the following people for further submissions:
  * Henrik - multitouch maintainer in the kernel, and I'm sure he will be happy to see this stuff
  * Dmitry - input maintainer, the driver will go through his tree, so it's better to let him know as soon as possible of the different discussions.
- please stick to the kernel formatting guidelines (without orders: do not use C99-style ("// ..."), do not mix tabs and spaces, stick to 80 columns, etc..). The whole documentation is in Documentation/CodingStyle, and use the script scripts/checkpatch.pl to validate most of these.
- I don't think a separate ".h" will be accepted as the declarations will not be used outside of the driver. Just merge the header on top of you .c file.

> 
> Open question: it looks like just calling
> 
> input_mt_init_slots(poll_dev->input, MAX_CONTACTS, INPUT_MT_DIRECT);
> 
> in the initialization routine isn't enough. hid-multitouch doesn't seem
> to use any other init commands, though, or did I overlook something?
> 
> Are there any other caveats of the input-mt library which I should be
> aware of?

You need to also set up the different axes (ABS_MT_POSITION_X and others) before calling input_mt_init_slots(). See setup_events_to_report() in bcm5974.c for an example.

> 
> Thanks for your input, and best regards, Florian
> 

Few comments inlined:

> /* 

>   Surface2.0/SUR40/PixelSense input driver v0.0.1

> 

>   This program is free software; you can redistribute it and/or

>   modify it under the terms of the GNU General Public License as

>   published by the Free Software Foundation; either version 2 of

>   the License, or (at your option) any later version.

> 

>   Copyright (C) 2013 by Florian 'floe' Echtler <floe@butterbrot.org>

> 

>   Derived from the USB Skeleton driver 1.1,

>   Copyright (C) 2003 Greg Kroah-Hartman (greg@kroah.com)

> 

> 	Derived from the Apple USB BCM5974 multitouch driver,

> 	Copyright (C) 2008 Henrik Rydberg (rydberg@euromail.se)

> */

> 

>[snipped]
> 

> /*

>  * this function is called when a whole contact has been processed,

>  * so that it can assign it to a slot and store the data there

>  */

> static void report_blob(struct surface_blob *blob, struct input_dev *input)

> {

> 	int wide, major, minor;

> 

> 	int slotnum = input_mt_get_slot_by_key(input, blob->blob_id);

> 	if (slotnum < 0 || slotnum >= MAX_CONTACTS)

> 		return;

> 

> 	/* FIXME: is this needed for the Pixelsense? */


I doubt. This was required in hid-multitouch because we sometimes have "blobs" without valuable information. So we drop them.
Here it looks like each blob contains information, so it should be fine without this.

> 	/*if ((td->mtclass.quirks & MT_QUIRK_IGNORE_DUPLICATES) && mt) {

> 		struct input_mt *mt = input->mt;

> 		struct input_mt_slot *slot = &mt->slots[slotnum];

> 		if (input_mt_is_active(slot) &&

> 				input_mt_is_used(mt, slot))

> 			return;

> 	}*/

> 

> 	input_mt_slot(input, slotnum);

> 	input_mt_report_slot_state(input, MT_TOOL_FINGER, 1);


looks like you never release the touch... :(
See below.

> 	wide = (blob->bb_size_x > blob->bb_size_y);

> 	major = max(blob->bb_size_x, blob->bb_size_y);

> 	minor = min(blob->bb_size_x, blob->bb_size_y);

> 

> 	input_event(input, EV_ABS, ABS_MT_POSITION_X, blob->pos_x);

> 	input_event(input, EV_ABS, ABS_MT_POSITION_Y, blob->pos_y);

> 	input_event(input, EV_ABS, ABS_MT_TOOL_X, blob->ctr_x);

> 	input_event(input, EV_ABS, ABS_MT_TOOL_Y, blob->ctr_y);

> 	input_event(input, EV_ABS, ABS_MT_ORIENTATION, wide);

> 	input_event(input, EV_ABS, ABS_MT_TOUCH_MAJOR, major);

> 	input_event(input, EV_ABS, ABS_MT_TOUCH_MINOR, minor);

> }

> 

> 
[snipped]
> 

> /* check candidate USB interface */

> static int surface_probe(struct usb_interface *interface,

> 				const struct usb_device_id *id)

> {

> 	struct usb_device *usbdev = interface_to_usbdev(interface);

> 	struct usb_surface *surface;

> 	struct usb_host_interface *iface_desc;

> 	struct usb_endpoint_descriptor *endpoint;

> 	struct input_polled_dev* poll_dev;

> 

> 	/* check if we really have the right interface */

> 	iface_desc = &interface->altsetting[0];

> 	if (iface_desc->desc.bInterfaceClass != 0xFF)

> 		return -ENODEV;

> 

> 	/* allocate memory for our device state and initialize it */

> 	surface = kzalloc(sizeof(struct usb_surface), GFP_KERNEL);

> 	poll_dev = input_allocate_polled_device();

> 	surface->input = poll_dev;

> 	if (!surface || !poll_dev)

> 		return -ENOMEM;

> 

> 	poll_dev->private = surface;

> 	poll_dev->poll_interval = POLL_INTERVAL;

> 	poll_dev->open  = surface_open;

> 	poll_dev->poll  = surface_poll;

> 	poll_dev->close = surface_close;

> 

> 	poll_dev->input->name = "Samsung SUR40";

> 	usb_to_input_id(usbdev,&(poll_dev->input->id));

> 	usb_make_path(usbdev, surface->phys, sizeof(surface->phys));

> 	strlcat(surface->phys, "/input0", sizeof(surface->phys));

> 	poll_dev->input->phys = surface->phys;

> 

> 	input_mt_init_slots(poll_dev->input, MAX_CONTACTS, INPUT_MT_DIRECT);


If the device always reports actual touches and discards the release information, you may need to use the flag INPUT_MT_DROP_UNUSED.

> 

> 	surface->usbdev = usbdev;

> 	surface->interface = interface;

> 

> 	/* use endpoint #4 (0x86) */

> 	endpoint = &iface_desc->endpoint[4].desc;

> 	if (endpoint->bEndpointAddress == TOUCH_ENDPOINT) {

> 		/* we found a bulk in endpoint */

> 		surface->bulk_in_size = le16_to_cpu(endpoint->wMaxPacketSize);

> 		surface->bulk_in_endpointAddr = endpoint->bEndpointAddress;

> 		surface->bulk_in_buffer = kmalloc(2*surface->bulk_in_size, GFP_KERNEL);

> 		if (!surface->bulk_in_buffer) {

> 			pr_err("Unable to allocate input buffer.");

> 			surface_delete(surface);

> 			return -ENOMEM;

> 		}

> 	}

> 

> 	if (!(surface->bulk_in_endpointAddr)) {

> 		pr_err("Unable to find bulk-in endpoint.");

> 		surface_delete(surface);

> 		return -ENODEV;

> 	}

> 

> 	/* we can register the device now, as it is ready */

> 	usb_set_intfdata(interface, surface);

> 

> 	if (input_register_polled_device(poll_dev)) {

> 		pr_err("Unable to register polled input device.");

> 		surface_delete(surface);

> 		return -ENODEV;

> 	}

> 

> 	dev_info(&interface->dev,"%s now attached\n",DRIVER_DESC);

> 

> 	return 0;

> }

> 

> [snipped]
>
> /* usb specific object needed to register this driver with the usb subsystem */

> static struct usb_driver surface_driver = {

> 	.name = DRIVER_SHORT,

> 	.probe = surface_probe,

> 	.disconnect = surface_disconnect,

> 	/*.suspend = surface_suspend,

> 	.resume = surface_resume,*/


if they are commented, they are not required, so just drop it :)

> 	.id_table = surface_table,

> 	/*.supports_autosuspend = 1,*/


ditto

> };

> 

> module_usb_driver(surface_driver);

> 

> MODULE_AUTHOR(DRIVER_AUTHOR);

> MODULE_DESCRIPTION(DRIVER_DESC);

> MODULE_LICENSE("GPL");



Cheers,
Benjamin

Re: [RFC] surface-input

[David Herrmann]

Hi

On Thu, Sep 5, 2013 at 4:15 PM, Florian Echtler <floe@butterbrot.org> wrote:
> Hello everyone,
>
> as mentioned earlier, I'm currently writing a multitouch input driver
> for the Pixelsense (formerly Surface 2.0). It's now at a point where I'd
> consider it mostly done, but a) I have very limited experience with
> kernel drivers and b) there are some additional questions I have, so I'm
> attaching the current state of my source code and would like to ask for
> your comments.
>
> Open question: it looks like just calling
>
> input_mt_init_slots(poll_dev->input, MAX_CONTACTS, INPUT_MT_DIRECT);
>
> in the initialization routine isn't enough. hid-multitouch doesn't seem
> to use any other init commands, though, or did I overlook something?
>
> Are there any other caveats of the input-mt library which I should be
> aware of?
>
> Thanks for your input, and best regards, Florian

Please inline the code as Benjamin noted. Otherwise, it's cumbersome
to comment on specific lines (please see "git help format-patch" and
"git help send-email").
I did review the core setup/lifetime/destruction code and it looks
good. But you need to fix several error-paths. If a core function
returns "int", you should check it for "r < 0" and then forward the
code. Don't overwrite it with EINVAL.
Furthermore, correctly free your memory. Things like this leak memory
(surface might be non-NULL):
  if (!surface || !poll_dev)
    return -ENOMEM;

I can do a more complete review for v2 if you send it inline. But
looks good overall.

Thanks
David

Re: [RFC] surface-input

[Florian Echtler]

[-- Attachment #1: Type: text/plain, Size: 1712 bytes --]

On 06.09.2013 15:25, Benjamin Tissoires wrote:
> Some generic comments:
> - please always inline the code in the message, it is *much* easier to review and comment it
> - please directly use a patch format: if the code is good, Dmitry can take it directly through his tree
> - add the following people for further submissions:
>   * Henrik - multitouch maintainer in the kernel, and I'm sure he will be happy to see this stuff
>   * Dmitry - input maintainer, the driver will go through his tree, so it's better to let him know as soon as possible of the different discussions.
> - please stick to the kernel formatting guidelines (without orders: do not use C99-style ("// ..."), do not mix tabs and spaces, stick to 80 columns, etc..). The whole documentation is in Documentation/CodingStyle, and use the script scripts/checkpatch.pl to validate most of these.
> - I don't think a separate ".h" will be accepted as the declarations will not be used outside of the driver. Just merge the header on top of you .c file.

Benjamin and David, thanks for your feedback. I will integrate this and
get back to you with a proper patch ASAP.

I have one additional question which is more about "future-proofing"
this driver. The SUR40 hardware does also provide a raw video image from
the touch sensor over another USB _endpoint_. Since it's not a different
_interface_, this can only be handled in the same driver as far as I can
tell. At some point in the future, I would like to add a V4L2 interface
to also support the video endpoint - are there any issues I should
already try to watch out for in the input part?

Thanks again, and best regards, Florian
-- 
SENT FROM MY DEC VT50 TERMINAL


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [RFC] surface-input

[David Herrmann]

Hi

On Mon, Sep 9, 2013 at 10:25 AM, Florian Echtler <floe@butterbrot.org> wrote:
> On 06.09.2013 15:25, Benjamin Tissoires wrote:
>> Some generic comments:
>> - please always inline the code in the message, it is *much* easier to review and comment it
>> - please directly use a patch format: if the code is good, Dmitry can take it directly through his tree
>> - add the following people for further submissions:
>>   * Henrik - multitouch maintainer in the kernel, and I'm sure he will be happy to see this stuff
>>   * Dmitry - input maintainer, the driver will go through his tree, so it's better to let him know as soon as possible of the different discussions.
>> - please stick to the kernel formatting guidelines (without orders: do not use C99-style ("// ..."), do not mix tabs and spaces, stick to 80 columns, etc..). The whole documentation is in Documentation/CodingStyle, and use the script scripts/checkpatch.pl to validate most of these.
>> - I don't think a separate ".h" will be accepted as the declarations will not be used outside of the driver. Just merge the header on top of you .c file.
>
> Benjamin and David, thanks for your feedback. I will integrate this and
> get back to you with a proper patch ASAP.
>
> I have one additional question which is more about "future-proofing"
> this driver. The SUR40 hardware does also provide a raw video image from
> the touch sensor over another USB _endpoint_. Since it's not a different
> _interface_, this can only be handled in the same driver as far as I can
> tell. At some point in the future, I would like to add a V4L2 interface
> to also support the video endpoint - are there any issues I should
> already try to watch out for in the input part?

Media drivers use input-devices to report input data. So no, there is
no reason to change your driver's design. Once you want
media-functionality, you simply add a v4l2 device during hid_probe()
and remove it in hid_remove(). User-space can see the relation between
both via sysfs.
It is never wrong to think about the API _now_. But as long as it's
only a single input-dev and one v4l2-dev, I'd recommend to put both
below your usb-device and you're fine.
Regarding internal driver-design, you should not bother now. Make it
work for input and make it work well.

Cheers
David