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 [off-list ref]
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