Re: [RFC v3 3/5] hid-multitouch: support for PixCir-based panels
From: Benjamin Tissoires
Date: Mon Jan 10 2011 - 14:10:42 EST
On Fri, Jan 7, 2011 at 8:49 PM, Henrik Rydberg <rydberg@xxxxxxxxxxx> wrote:
> On Fri, Jan 07, 2011 at 07:42:40PM +0100, Benjamin Tissoires wrote:
>> Created a driver for PixCir based dual-touch panels, including the one
>> in the Hanvon tablet. This is done in a code structure aimed at unifying
>> support for several existing HID multitouch panels.
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@xxxxxxx>
>> Signed-off-by: Stéphane Chatty <chatty@xxxxxxx>
>> ---
>
> It looks much better now. Just a few comments and some nit-picks inline.
>
>> drivers/hid/Kconfig | 9 +
>> drivers/hid/Makefile | 1 +
>> drivers/hid/hid-core.c | 2 +
>> drivers/hid/hid-ids.h | 4 +
>> drivers/hid/hid-multitouch.c | 468 ++++++++++++++++++++++++++++++++++++++++++
>> 5 files changed, 484 insertions(+), 0 deletions(-)
>> create mode 100644 drivers/hid/hid-multitouch.c
>>
>> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
>> index 401acec..511554d 100644
>> --- a/drivers/hid/Kconfig
>> +++ b/drivers/hid/Kconfig
>> @@ -285,6 +285,15 @@ config HID_MONTEREY
>> ---help---
>> Support for Monterey Genius KB29E.
>>
>> +config HID_MULTITOUCH
>> + tristate "HID Multitouch panels"
>> + depends on USB_HID
>> + ---help---
>> + Generic support for HID multitouch panels.
>> +
>> + Say Y here if you have one of the following devices:
>> + - PixCir touchscreen
>
> Should also mention how to compile as module, and what the module will be called.
ok, will do in v4
>
>> +
>> config HID_NTRIG
>> tristate "N-Trig touch screen"
>> depends on USB_HID
>> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
>> index c335605..ec991d4 100644
>> --- a/drivers/hid/Makefile
>> +++ b/drivers/hid/Makefile
>> @@ -46,6 +46,7 @@ obj-$(CONFIG_HID_MAGICMOUSE) += hid-magicmouse.o
>> obj-$(CONFIG_HID_MICROSOFT) += hid-microsoft.o
>> obj-$(CONFIG_HID_MONTEREY) += hid-monterey.o
>> obj-$(CONFIG_HID_MOSART) += hid-mosart.o
>> +obj-$(CONFIG_HID_MULTITOUCH) += hid-multitouch.o
>> obj-$(CONFIG_HID_NTRIG) += hid-ntrig.o
>> obj-$(CONFIG_HID_ORTEK) += hid-ortek.o
>> obj-$(CONFIG_HID_PRODIKEYS) += hid-prodikeys.o
>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>> index 88668ae..2b4d9b9 100644
>> --- a/drivers/hid/hid-core.c
>> +++ b/drivers/hid/hid-core.c
>> @@ -1286,6 +1286,7 @@ static const struct hid_device_id hid_blacklist[] = {
>> { HID_USB_DEVICE(USB_VENDOR_ID_BELKIN, USB_DEVICE_ID_FLIP_KVM) },
>> { HID_USB_DEVICE(USB_VENDOR_ID_BTC, USB_DEVICE_ID_BTC_EMPREX_REMOTE) },
>> { HID_USB_DEVICE(USB_VENDOR_ID_BTC, USB_DEVICE_ID_BTC_EMPREX_REMOTE_2) },
>> + { HID_USB_DEVICE(USB_VENDOR_ID_CANDO, USB_DEVICE_ID_CANDO_PIXCIR_MULTI_TOUCH) },
>> { HID_USB_DEVICE(USB_VENDOR_ID_CANDO, USB_DEVICE_ID_CANDO_MULTI_TOUCH) },
>> { HID_USB_DEVICE(USB_VENDOR_ID_CANDO, USB_DEVICE_ID_CANDO_MULTI_TOUCH_11_6) },
>> { HID_USB_DEVICE(USB_VENDOR_ID_CANDO, USB_DEVICE_ID_CANDO_MULTI_TOUCH_15_6) },
>> @@ -1312,6 +1313,7 @@ static const struct hid_device_id hid_blacklist[] = {
>> { HID_USB_DEVICE(USB_VENDOR_ID_GYRATION, USB_DEVICE_ID_GYRATION_REMOTE) },
>> { HID_USB_DEVICE(USB_VENDOR_ID_GYRATION, USB_DEVICE_ID_GYRATION_REMOTE_2) },
>> { HID_USB_DEVICE(USB_VENDOR_ID_GYRATION, USB_DEVICE_ID_GYRATION_REMOTE_3) },
>> + { HID_USB_DEVICE(USB_VENDOR_ID_HANVON, USB_DEVICE_ID_HANVON_MULTITOUCH) },
>> { HID_USB_DEVICE(USB_VENDOR_ID_KENSINGTON, USB_DEVICE_ID_KS_SLIMBLADE) },
>> { HID_USB_DEVICE(USB_VENDOR_ID_KYE, USB_DEVICE_ID_KYE_ERGO_525V) },
>> { HID_USB_DEVICE(USB_VENDOR_ID_LABTEC, USB_DEVICE_ID_LABTEC_WIRELESS_KEYBOARD) },
>> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
>> index 0f150c7..17b444b 100644
>> --- a/drivers/hid/hid-ids.h
>> +++ b/drivers/hid/hid-ids.h
>> @@ -134,6 +134,7 @@
>> #define USB_DEVICE_ID_BTC_EMPREX_REMOTE_2 0x5577
>>
>> #define USB_VENDOR_ID_CANDO 0x2087
>> +#define USB_DEVICE_ID_CANDO_PIXCIR_MULTI_TOUCH 0x0703
>> #define USB_DEVICE_ID_CANDO_MULTI_TOUCH 0x0a01
>> #define USB_DEVICE_ID_CANDO_MULTI_TOUCH_11_6 0x0b03
>> #define USB_DEVICE_ID_CANDO_MULTI_TOUCH_15_6 0x0f01
>> @@ -306,6 +307,9 @@
>> #define USB_DEVICE_ID_HANWANG_TABLET_FIRST 0x5000
>> #define USB_DEVICE_ID_HANWANG_TABLET_LAST 0x8fff
>>
>> +#define USB_VENDOR_ID_HANVON 0x20b3
>> +#define USB_DEVICE_ID_HANVON_MULTITOUCH 0x0a18
>> +
>> #define USB_VENDOR_ID_HAPP 0x078b
>> #define USB_DEVICE_ID_UGCI_DRIVING 0x0010
>> #define USB_DEVICE_ID_UGCI_FLYING 0x0020
>> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
>> new file mode 100644
>> index 0000000..3b05dfe
>> --- /dev/null
>> +++ b/drivers/hid/hid-multitouch.c
>> @@ -0,0 +1,468 @@
>> +/*
>> + * HID driver for multitouch panels
>> + *
>> + * Copyright (c) 2010-2011 Stephane Chatty <chatty@xxxxxxx>
>> + * Copyright (c) 2010-2011 Benjamin Tissoires <benjamin.tissoires@xxxxxxxxx>
>> + * Copyright (c) 2010-2011 Ecole Nationale de l'Aviation Civile, France
>> + *
>> + */
>> +
>> +/*
>> + * 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.
>> + */
>> +
>> +#include <linux/device.h>
>> +#include <linux/hid.h>
>> +#include <linux/module.h>
>> +#include <linux/slab.h>
>> +#include <linux/usb.h>
>> +#include <linux/input/mt.h>
>> +#include "usbhid/usbhid.h"
>> +
>> +
>> +MODULE_AUTHOR("Stephane Chatty <chatty@xxxxxxx>");
>> +MODULE_DESCRIPTION("HID multitouch panels");
>> +MODULE_LICENSE("GPL");
>> +
>> +#include "hid-ids.h"
>> +
>> +/* quirks to control the device */
>> +#define MT_QUIRK_NOT_SEEN_MEANS_UP (1 << 0)
>> +#define MT_QUIRK_SLOT_IS_CONTACTID (1 << 1)
>> +
>> +struct mt_slot {
>> + __s32 x, y, p, w, h;
>> + __s32 contactid; /* the device ContactID assigned to this slot */
>> + bool touch_state; /* is the touch valid? */
>> + bool seen_in_this_frame;/* has this slot been updated */
>> +};
>> +
>> +struct mt_device {
>> + struct mt_slot curdata; /* placeholder of incoming data */
>> + struct mt_class *mtclass; /* our mt device class */
>> + unsigned last_field_index; /* last field index of the report */
>> + unsigned last_slot_field; /* the last field of a slot */
>> + __s8 inputmode; /* InputMode HID feature, -1 if non-existent */
>> + __u8 num_received; /* how many contacts we received */
>> + __u8 num_expected; /* expected last contact index */
>
> Any particular reason these are u8?
Just because we don't need s32 here.
>
>> + bool curvalid; /* is the current contact valid? */
>> + struct mt_slot slots[0]; /* first slot */
>> +};
>> +
>> +struct mt_class {
>> + __s32 quirks;
>> + __s32 sn_move; /* Signal/noise ratio for move events */
>> + __s32 sn_pressure; /* Signal/noise ratio for pressure events */
>> + __u8 maxcontacts;
>
> Same here - its not like we allocate a million of these anyways. Simple ints should do fine.
>
>> +};
>> +
>> +/* classes of device behavior */
>> +#define MT_CLS_DEFAULT 0
>> +#define MT_CLS_DUAL1 1
>> +
>> +/*
>> + * these device-dependent functions determine what slot corresponds
>> + * to a valid contact that was just read.
>> + */
>> +
>> +static int slot_is_contactid(struct mt_device *td)
>> +{
>> + return td->curdata.contactid;
>> +}
>> +
>> +static int find_slot_from_contactid(struct mt_device *td)
>> +{
>> + int i;
>> + for (i = 0; i < td->mtclass->maxcontacts; ++i) {
>> + if (td->slots[i].contactid == td->curdata.contactid &&
>> + td->slots[i].touch_state)
>> + return i;
>> + }
>> + for (i = 0; i < td->mtclass->maxcontacts; ++i) {
>> + if (!td->slots[i].seen_in_this_frame &&
>> + !td->slots[i].touch_state)
>> + return i;
>> + }
>> + return -1;
>> + /* should not occurs. If this happens that means
>> + * that the device sent more touches that it says
>> + * in the report descriptor. It is ignored then. */
>
> I would put the comment above the return statement.
ack.
>
>> +}
>> +
>> +struct mt_class mt_classes[] = {
>> + { 0, 0, 0, 10 }, /* MT_CLS_DEFAULT */
>> + { MT_QUIRK_SLOT_IS_CONTACTID, 0, 0, 2 }, /* MT_CLS_DUAL1 */
>> +};
>> +
>> +static void mt_feature_mapping(struct hid_device *hdev, struct hid_input *hi,
>> + struct hid_field *field, struct hid_usage *usage)
>> +{
>> + if (usage->hid == HID_DG_INPUTMODE) {
>> + struct mt_device *td = hid_get_drvdata(hdev);
>> + td->inputmode = field->report->id;
>> + }
>> +}
>> +
>> +static void set_abs(struct input_dev *input, unsigned int code,
>> + struct hid_field *field, int snratio)
>> +{
>> + int fmin = field->logical_minimum;
>> + int fmax = field->logical_maximum;
>> + int fuzz = snratio ? (fmax - fmin) / snratio : 0;
>> + input_set_abs_params(input, code, fmin, fmax, fuzz, 0);
>> +}
>> +
>> +static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>> + struct hid_field *field, struct hid_usage *usage,
>> + unsigned long **bit, int *max)
>> +{
>> + struct mt_device *td = hid_get_drvdata(hdev);
>> + struct mt_class *cls = td->mtclass;
>> + switch (usage->hid & HID_USAGE_PAGE) {
>> +
>> + case HID_UP_GENDESK:
>> + switch (usage->hid) {
>> + case HID_GD_X:
>> + hid_map_usage(hi, usage, bit, max,
>> + EV_ABS, ABS_MT_POSITION_X);
>> + set_abs(hi->input, ABS_MT_POSITION_X, field,
>> + cls->sn_move);
>> + /* touchscreen emulation */
>> + set_abs(hi->input, ABS_X, field, cls->sn_move);
>> + td->last_slot_field = usage->hid;
>> + return 1;
>> + case HID_GD_Y:
>> + hid_map_usage(hi, usage, bit, max,
>> + EV_ABS, ABS_MT_POSITION_Y);
>> + set_abs(hi->input, ABS_MT_POSITION_Y, field,
>> + cls->sn_move);
>> + /* touchscreen emulation */
>> + set_abs(hi->input, ABS_Y, field, cls->sn_move);
>> + td->last_slot_field = usage->hid;
>> + return 1;
>> + }
>> + return 0;
>> +
>> + case HID_UP_DIGITIZER:
>> + switch (usage->hid) {
>> + case HID_DG_INRANGE:
>> + td->last_slot_field = usage->hid;
>> + return 1;
>> + case HID_DG_CONFIDENCE:
>> + td->last_slot_field = usage->hid;
>> + return 1;
>> + case HID_DG_TIPSWITCH:
>> + hid_map_usage(hi, usage, bit, max, EV_KEY, BTN_TOUCH);
>> + input_set_capability(hi->input, EV_KEY, BTN_TOUCH);
>> + td->last_slot_field = usage->hid;
>> + return 1;
>> + case HID_DG_CONTACTID:
>> + input_mt_init_slots(hi->input,
>> + td->mtclass->maxcontacts);
>> + td->last_slot_field = usage->hid;
>> + return 1;
>> + case HID_DG_WIDTH:
>> + hid_map_usage(hi, usage, bit, max,
>> + EV_ABS, ABS_MT_TOUCH_MAJOR);
>> + td->last_slot_field = usage->hid;
>> + return 1;
>> + case HID_DG_HEIGHT:
>> + hid_map_usage(hi, usage, bit, max,
>> + EV_ABS, ABS_MT_TOUCH_MINOR);
>> + field->logical_maximum = 1;
>> + field->logical_minimum = 1;
>
> minimum should be zero here.
Maybe a typo in the original code: we hade set_abs_input_param(..., 1,1,0,0).
I just reorganized.
Changed in v4.
>
>> + set_abs(hi->input, ABS_MT_ORIENTATION, field, 0);
>> + td->last_slot_field = usage->hid;
>> + return 1;
>> + case HID_DG_TIPPRESSURE:
>> + hid_map_usage(hi, usage, bit, max,
>> + EV_ABS, ABS_MT_PRESSURE);
>> + set_abs(hi->input, ABS_MT_PRESSURE, field,
>> + cls->sn_pressure);
>> + /* touchscreen emulation */
>> + set_abs(hi->input, ABS_PRESSURE, field,
>> + cls->sn_pressure);
>> + td->last_slot_field = usage->hid;
>> + return 1;
>> + case HID_DG_CONTACTCOUNT:
>> + td->last_field_index = field->report->maxfield - 1;
>> + return 1;
>> + case HID_DG_CONTACTMAX:
>> + /* we don't set td->last_slot_field as contactcount and
>> + * contact max are global to the report */
>> + return -1;
>> + }
>> + /* let hid-input decide for the others */
>> + return 0;
>> +
>> + case 0xff000000:
>> + /* we do not want to map these: no input-oriented meaning */
>> + return -1;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int mt_input_mapped(struct hid_device *hdev, struct hid_input *hi,
>> + struct hid_field *field, struct hid_usage *usage,
>> + unsigned long **bit, int *max)
>> +{
>> + if (usage->type == EV_KEY || usage->type == EV_ABS)
>> + set_bit(usage->type, hi->input->evbit);
>> +
>> + return -1;
>> +}
>> +
>> +static int mt_compute_slot(struct mt_device *td)
>> +{
>> + struct mt_class *cls = td->mtclass;
>> +
>> + if (cls->quirks & MT_QUIRK_SLOT_IS_CONTACTID)
>> + return slot_is_contactid(td);
>
> No real need for a function call here...
just to prepare for the other patches
>
>> +
>> + return find_slot_from_contactid(td);
>> +}
>> +
>> +/*
>> + * 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 mt_complete_slot(struct mt_device *td)
>> +{
>
> Adding "td->curdata.seen_in_this_frame = true;" here...
>
>> + if (td->curvalid) {
>> + struct mt_slot *slot;
>
> Skipping this...
>
>> + int slotnum = mt_compute_slot(td);
>> +
>> + if (slotnum >= 0 && slotnum < td->mtclass->maxcontacts) {
>> + slot = td->slots + slotnum;
>
> and this...
>
>> +
>> + memcpy(slot, &(td->curdata), sizeof(struct mt_slot));
>
> and "td->slots[slotnum] = td->curdata" here...
>
>> + slot->seen_in_this_frame = true;
>
> and dropping this... looks simpler, ne?
>
>> + }
>> + }
>> + td->num_received++;
>> +}
>
> Writing it explicitly here so you can judge for yourself:
>
> td->curdata.seen_in_this_frame = true;
> if (td->curvalid) {
> int slot = mt_compute_slot(td);
>
> if (slot >= 0 && slot < td->mtclass->maxcontacts)
> td->slots[slot] = td->curdata;
> }
> td->num_received++;
arg. Elementary C: struct are passed by value. snif.
I'll take your code.
>
>> +
>> +
>> +/*
>> + * this function is called when a whole packet has been received and processed,
>> + * so that it can decide what to send to the input layer.
>> + */
>> +static void mt_emit_event(struct mt_device *td, struct input_dev *input)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < td->mtclass->maxcontacts; ++i) {
>> + struct mt_slot *s = &(td->slots[i]);
>> + if ((td->mtclass->quirks & MT_QUIRK_NOT_SEEN_MEANS_UP) &&
>> + !s->seen_in_this_frame) {
>> + /*
>> + * this slot does not contain useful data,
>> + * notify its closure
>> + */
>
> It does contain useful data, it is just assumed to be in the up
> state. I would drop the comment and the brackets here, the quirk name
> speaks for itself.
We can drop the comment if you want.
>
>> + s->touch_state = false;
>> + }
>> +
>> + input_mt_slot(input, i);
>> + input_mt_report_slot_state(input, MT_TOOL_FINGER,
>> + s->touch_state);
>
> The values below should not be updated for an inactive slot.
ack.
>
>> + input_event(input, EV_ABS, ABS_MT_POSITION_X, s->x);
>> + input_event(input, EV_ABS, ABS_MT_POSITION_Y, s->y);
>> + input_event(input, EV_ABS, ABS_MT_PRESSURE, s->p);
>> + input_event(input, EV_ABS, ABS_MT_TOUCH_MAJOR, s->w);
>> + input_event(input, EV_ABS, ABS_MT_TOUCH_MINOR, s->h);
>> + s->seen_in_this_frame = false;
>> +
>> + }
>> +
>> + input_mt_report_pointer_emulation(input, true);
>> + input_sync(input);
>> + td->num_received = 0;
>> +}
>> +
>> +
>> +
>> +static int mt_event(struct hid_device *hid, struct hid_field *field,
>> + struct hid_usage *usage, __s32 value)
>> +{
>> + struct mt_device *td = hid_get_drvdata(hid);
>> +
>> + if (hid->claimed & HID_CLAIMED_INPUT) {
>> + switch (usage->hid) {
>> + case HID_DG_INRANGE:
>> + break;
>> + case HID_DG_TIPSWITCH:
>> + td->curvalid = value;
>
> I do not think curvalid should depend on the touch state (which is
> what tipswitch is). Either move to INRANGE, or simply set to true.
INRANGE is not all the time used as curvalid as you are saying.
At least for Stantum, curvalid is given by CONFIDENCE.
The solution of giving the value true to curvalid is not working too:
Cypress devices can send their touches over 2 reports of 7 touches,
even if it handles 10 touches.
Solution:
1) td->curvalid = td->num_received < td->mtclass->maxcontacts;
(not sure it will work for all devices)
or 2) introduce MT_QUIRK_VALID_IS_INRANGE and MT_QUIRK_VALID_IS_CONFIDENCE
or 3) let curvalid=value and disable the quirk
MT_QUIRK_NOT_SEEN_MEANS_UP (adding a FixMe comment above)
For v4, I'll use solution 3 (safer for 2.6.38 and after)
>
>> + td->curdata.touch_state = value;
>> + break;
>> + case HID_DG_CONFIDENCE:
>> + break;
>> + case HID_DG_CONTACTID:
>> + td->curdata.contactid = value;
>> + break;
>> + case HID_DG_TIPPRESSURE:
>> + td->curdata.p = value;
>> + break;
>> + case HID_GD_X:
>> + td->curdata.x = value;
>> + break;
>> + case HID_GD_Y:
>> + td->curdata.y = value;
>> + break;
>> + case HID_DG_WIDTH:
>> + td->curdata.w = value;
>> + break;
>> + case HID_DG_HEIGHT:
>> + td->curdata.h = value;
>> + break;
>> + case HID_DG_CONTACTCOUNT:
>> + /*
>> + * We must not overwrite the previous value (some
>> + * devices send one sequence splitted over several
>> + * messages)
>> + */
>
> How about "Includes multi-packet support where subsequent packets are sent with zero contactcount."
if you want
>
>> + if (value)
>> + td->num_expected = value - 1;
>
> The - 1 should be dropped here.
Here is the really tricky one:
if I drop -1,
the test below will be:
if (field->index == td->last_field_index
&& td->num_received >= td->num_expected)
but during the init of the device, the kernel receive 1 event though
the driver is not ready.
During this time, num_expected is at 0, and when we receive the
last_field_index: segfault!
To sum up, no, we can't.
>
>> + break;
>> +
>> + default:
>> + /* fallback to the generic hidinput handling */
>> + return 0;
>> + }
>> + }
>> +
>> + if (usage->hid == td->last_slot_field)
>> + mt_complete_slot(td);
>> +
>> + if (field->index == td->last_field_index
>> + && td->num_received > td->num_expected)
>
> A ">=" here.
>
>> + mt_emit_event(td, field->hidinput->input);
>> +
>> + /* we have handled the hidinput part, now remains hiddev */
>> + if (hid->claimed & HID_CLAIMED_HIDDEV && hid->hiddev_hid_event)
>> + hid->hiddev_hid_event(hid, field, usage, value);
>> +
>> + return 1;
>> +}
>> +
>> +static void mt_set_input_mode(struct hid_device *hdev)
>> +{
>> + struct mt_device *td = hid_get_drvdata(hdev);
>> + struct hid_report *r;
>> + struct hid_report_enum *re;
>> +
>> + if (td->inputmode < 0)
>> + return;
>> +
>> + re = &(hdev->report_enum[HID_FEATURE_REPORT]);
>> + r = re->report_id_hash[td->inputmode];
>> + if (r) {
>> + r->field[0]->value[0] = 0x02;
>> + usbhid_submit_report(hdev, r, USB_DIR_OUT);
>> + }
>> +}
>> +
>> +static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
>> +{
>> + int ret;
>> + struct mt_device *td;
>> + struct mt_class *mtclass = mt_classes + id->driver_data;
>> +
>> + /* This allows the driver to correctly support devices
>> + * that emit events over several HID messages.
>> + */
>> + hdev->quirks |= HID_QUIRK_NO_INPUT_SYNC;
>> +
>> + td = kzalloc(sizeof(struct mt_device) +
>> + mtclass->maxcontacts * sizeof(struct mt_slot),
>> + GFP_KERNEL);
>> + if (!td) {
>> + dev_err(&hdev->dev, "cannot allocate multitouch data\n");
>> + return -ENOMEM;
>> + }
>> + td->mtclass = mtclass;
>> + td->inputmode = -1;
>> + hid_set_drvdata(hdev, td);
>> +
>> + ret = hid_parse(hdev);
>> + if (ret != 0)
>> + goto fail;
>
> "if (ret)" is very standard.
ok
>
>> +
>> + ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
>> + if (ret != 0)
>> + goto fail;
>> +
>> + mt_set_input_mode(hdev);
>> +
>> + return 0;
>> +
>> +fail:
>> + kfree(td);
>> + return ret;
>> +}
>> +
>> +#ifdef CONFIG_PM
>> +static int mt_reset_resume(struct hid_device *hdev)
>> +{
>> + mt_set_input_mode(hdev);
>> + return 0;
>> +}
>> +#endif
>> +
>> +static void mt_remove(struct hid_device *hdev)
>> +{
>> + struct mt_device *td = hid_get_drvdata(hdev);
>> + hid_hw_stop(hdev);
>> + kfree(td);
>> + hid_set_drvdata(hdev, NULL);
>> +}
>> +
>> +static const struct hid_device_id mt_devices[] = {
>> +
>> + /* PixCir-based panels */
>> + { .driver_data = MT_CLS_DUAL1,
>> + HID_USB_DEVICE(USB_VENDOR_ID_HANVON,
>> + USB_DEVICE_ID_HANVON_MULTITOUCH) },
>> + { .driver_data = MT_CLS_DUAL1,
>> + HID_USB_DEVICE(USB_VENDOR_ID_CANDO,
>> + USB_DEVICE_ID_CANDO_PIXCIR_MULTI_TOUCH) },
>> +
>> + { }
>> +};
>> +MODULE_DEVICE_TABLE(hid, mt_devices);
>> +
>> +static const struct hid_usage_id mt_grabbed_usages[] = {
>> + { HID_ANY_ID, HID_ANY_ID, HID_ANY_ID },
>> + { HID_ANY_ID - 1, HID_ANY_ID - 1, HID_ANY_ID - 1}
>> +};
>> +
>> +static struct hid_driver mt_driver = {
>> + .name = "hid-multitouch",
>> + .id_table = mt_devices,
>> + .probe = mt_probe,
>> + .remove = mt_remove,
>> + .input_mapping = mt_input_mapping,
>> + .input_mapped = mt_input_mapped,
>> + .feature_mapping = mt_feature_mapping,
>> + .usage_table = mt_grabbed_usages,
>> + .event = mt_event,
>> +#ifdef CONFIG_PM
>> + .reset_resume = mt_reset_resume,
>> +#endif
>> +};
>> +
>> +static int __init mt_init(void)
>> +{
>> + return hid_register_driver(&mt_driver);
>> +}
>> +
>> +static void __exit mt_exit(void)
>> +{
>> + hid_unregister_driver(&mt_driver);
>> +}
>> +
>> +module_init(mt_init);
>> +module_exit(mt_exit);
>> --
>> 1.7.3.4
>>
>
> Thanks!
> Henrik
Thanks for the review,
Benjamin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/