* Re: Input sync flag when registering.
From: Dmitry Torokhov @ 2014-08-25 18:19 UTC (permalink / raw)
To: Thomas Poussevin; +Cc: linux-input
In-Reply-To: <53FAF963.9020403@parrot.com>
Hi Thomas,
On Mon, Aug 25, 2014 at 10:52:51AM +0200, Thomas Poussevin wrote:
> Hi,
> I noticed that with touchscreen drivers that don't explicitly call a
> input_sync after input_register_device, suspend to ram is blocked if
> no event is sent, because the input_dev created is considered as not
> synchronized. The problem was seen with Atmel mxt driver. When any
> event is sent, the driver explicitly ask a sync, so the problem is
> solved.
> The problem occurs only when the screen has never send any event
> before suspend to ram.
> I solved it setting the sync element to true (when input dev is
> created, no element is pending).
> Is there a better way to solve the problem ?
It is not clear to me what the problem is. The kernel as far as I
remember never checked state of 'sync' field when executing suspend
callbacks. Moreover there is no longer 'sync' field at all in mainline.
It sounds like some userspace code makes assumptions that are not always
valid.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: Lenovo T60 and X60: psmouse serio1: alps: Unknown ALPS touchpad: E7=10 00 64, EC=10 00 64
From: Dmitry Torokhov @ 2014-08-25 18:23 UTC (permalink / raw)
To: Paul Menzel; +Cc: linux-input, Yunkang Tang
In-Reply-To: <1408788987.2724.25.camel@mattotaupa>
On Sat, Aug 23, 2014 at 12:16:27PM +0200, Paul Menzel wrote:
> Dear Linux developers,
>
>
> with at least Linux 3.12, 3.13 and 3.14 the Linux kernel log contains a
> message about an unknown ALPS touchpad.
>
> […]
> [ 8.793232] psmouse serio1: alps: Unknown ALPS touchpad: E7=10 00 64, EC=10 00 64
> [ 9.068102] Console: switching to colour frame buffer device 128x48
> [ 9.072053] i915 0000:00:02.0: fb0: inteldrmfb frame buffer device
> [ 9.072056] i915 0000:00:02.0: registered panic notifier
> [ 9.072075] [drm] Initialized i915 1.6.0 20080730 for 0000:00:02.0 on minor 0
> [ 9.073703] hda_intel: probe_mask set to 0x1 for device 17aa:2010
> [ 9.074158] snd_hda_intel 0000:00:1b.0: irq 47 for MSI/MSI-X
> [ 9.104435] input: HDA Digital PCBeep as /devices/pci0000:00/0000:00:1b.0/input/input8
> [ 9.107546] input: HDA Intel Front Headphone as /devices/pci0000:00/0000:00:1b.0/sound/card0/input13
> [ 9.107678] input: HDA Intel Line Out as /devices/pci0000:00/0000:00:1b.0/sound/card0/input12
> [ 9.107793] input: HDA Intel Line as /devices/pci0000:00/0000:00:1b.0/sound/card0/input11
> [ 9.107909] input: HDA Intel Front Mic as /devices/pci0000:00/0000:00:1b.0/sound/card0/input10
> [ 9.108044] input: HDA Intel Rear Mic as /devices/pci0000:00/0000:00:1b.0/sound/card0/input9
> [ 9.168843] psmouse serio1: trackpoint: IBM TrackPoint firmware: 0x0e, buttons: 3/3
> [ 9.189695] input: TPPS/2 IBM TrackPoint as /devices/platform/i8042/serio1/input/input6
> […]
>
> The Lenovo T60 actually has a touchpad, a trackpoint and three buttons,
> where the Lenovo X60 only has a trackpoint and three buttons.
>
> Searching the Web with this message, there are also hits for other
> laptop models.
>
> Looking at `drivers/input/mouse/alps.c` the values checked there for E7
> and EC are not related at all. It’s also interesting that E7 and EC
> contain the same values.
>
> Is that the trackpoint device detected later and should therefore be
> ignored?
Yes. I'll add a patch to suppress this message (or rather turn it to a
debug message) as it confuses users.
Thanks.
--
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH] Input: ALPS - suppress message about 'Unknown touchpad'
From: Dmitry Torokhov @ 2014-08-25 18:30 UTC (permalink / raw)
To: linux-input; +Cc: Hans de Goede, Yunkang Tang, Paul Menzel, linux-kernel
When we fail to match data returned by E7 and EC reports we state that we
found "Unknown ALPS touchpad" whereas it is most likely it is not ALPS
touchpad at all. Change wording a bit and reduce the message to debug so
that it does not litter users logs and confuse them.
Reported-by: Paul Menzel <paulepanter@users.sourceforge.net>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/mouse/alps.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index a59a1a6..a956b98 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -2234,8 +2234,8 @@ static int alps_identify(struct psmouse *psmouse, struct alps_data *priv)
return 0;
}
- psmouse_info(psmouse,
- "Unknown ALPS touchpad: E7=%3ph, EC=%3ph\n", e7, ec);
+ psmouse_dbg(psmouse,
+ "Likely not an ALPS touchpad: E7=%3ph, EC=%3ph\n", e7, ec);
return -EINVAL;
}
--
2.1.0.rc2.206.gedb03e5
--
Dmitry
^ permalink raw reply related
* Re: [PATCH] HID: input: force generic axis to be mapped to their user space axis
From: Jiri Kosina @ 2014-08-26 7:25 UTC (permalink / raw)
To: Benjamin Tissoires; +Cc: Éric Brunet, linux-input, linux-kernel
In-Reply-To: <1408986430-24451-1-git-send-email-benjamin.tissoires@redhat.com>
On Mon, 25 Aug 2014, Benjamin Tissoires wrote:
> Atmel 840B digitizer presents a stylus interface which reports twice
> the X coordinate and then twice the Y coordinate. In its current
> implementation, hid-input assign the first X to X, then the second to Y,
> then the first Y to Z, then the second one to RX.
>
> This is wrong, and X should always be mapped to X, no matter what.
> A solution consists in forcing X, Y, Z, RX, RY, RZ to be mapped to their
> correct user space counter part.
>
> Reported-by: Éric Brunet <Eric.Brunet@lps.ens.fr>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Applied, thanks.
--
Jiri Kosina
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] HID: add support for PenMount HID TouchScreen Driver
From: Christian Gmeiner @ 2014-08-26 7:29 UTC (permalink / raw)
To: Benjamin Tissoires; +Cc: linux-input
In-Reply-To: <CAN+gG=Gpze-CA_LRa-69+M_rL3HzSiM_DwMAW1TW8xkA_Kc82w@mail.gmail.com>
Hi Benjamin.
2014-08-25 17:43 GMT+02:00 Benjamin Tissoires <benjamin.tissoires@gmail.com>:
> On Mon, Aug 25, 2014 at 10:48 AM, Christian Gmeiner
> <christian.gmeiner@gmail.com> wrote:
>> This driver is a cleaned up version of
>> http://git.android-x86.org/?p=kernel/cdv.git;a=blob_plain;f=drivers/hid/hid-penmount.c;hb=HEAD
>
> This is definitively weird. According to your driver, it should go by
> default through hid-multitouch and behave correctly (at least the
> multi-touch versions, and I would say yours too).
>
I thought I have tried it with hid-multitouch, but I it never gets used.
[ 94.342542] usb 2-1.7: USB disconnect, device number 6
[ 96.312636] usb 2-1.7: new full-speed USB device number 7 using ehci-pci
[ 96.399606] usb 2-1.7: New USB device found, idVendor=14e1, idProduct=6000
[ 96.399611] usb 2-1.7: New USB device strings: Mfr=1, Product=2,
SerialNumber=0
[ 96.399614] usb 2-1.7: Product: PenMount USB
[ 96.399617] usb 2-1.7: Manufacturer: DIALOGUE INC
[ 96.402777] input: DIALOGUE INC PenMount USB as
/devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.7/2-1.7:1.0/0003:14E1:6000.0004/input/input12
[ 96.403533] hid-generic 0003:14E1:6000.0004: input,hidraw2: USB HID
v1.01 Mouse [DIALOGUE INC PenMount USB] on usb-0000:00:1d.0-1.7/input0
> Can you send me some touch recordings of your device with
> hid-recorder[1] please? No need to remove your driver, it will capture
> raw input events, so you should be just fine.
[christian@chgm-pc hid]$ sudo hid-recorder
Available devices:
/dev/hidraw0: Microsoft Microsoft 3-Button Mouse with IntelliEye(TM)
/dev/hidraw1: LITE-ON Technology USB NetVista Full Width Keyboard.
/dev/hidraw2: DIALOGUE INC PenMount USB
Select the device event number [0-2]: 2
R: 76 05 01 09 02 a1 01 09 01 a1 00 09 00 15 00 25 01 95 06 75 01 81
01 05 09 19 01 29 03 15 00 25 01 95 02 75 01 81 02 05 01 09 30 09 31
15 00 26 ff 03 36 80 00 46 ff 7f 75 10 95 02 81 02 09 00 15 00 26 ff
00 75 08 95 05 b1 02 c0 c0
N: DIALOGUE INC PenMount USB
P: usb-0000:00:1d.0-1.7/input0
I: 3 14e1 6000
E: 0.000000 5 30 d0 00 d1 01
E: 0.000945 5 70 d0 00 d1 01
E: 0.006984 5 70 d0 00 d1 01
E: 0.012961 5 70 d0 00 d1 01
E: 0.019001 5 70 d0 00 d1 01
E: 0.025965 5 70 d0 00 d1 01
E: 0.031960 5 70 d0 00 d1 01
E: 0.037964 5 70 d0 00 d1 01
E: 0.043964 5 70 d0 00 d1 01
E: 0.050988 5 70 d0 00 d1 01
E: 0.056945 5 70 d0 00 d1 01
E: 0.062985 5 70 d0 00 d1 01
E: 0.068965 5 70 d0 00 d1 01
E: 0.075974 5 70 d0 00 d1 01
E: 0.081961 5 70 d0 00 d1 01
E: 0.112985 5 70 d0 00 d1 01
E: 0.118978 5 70 d0 00 d0 01
E: 0.124978 5 70 d1 00 d0 01
E: 0.131973 5 70 d3 00 d0 01
E: 0.137971 5 70 d4 00 d0 01
E: 0.143919 5 70 d5 00 cf 01
E: 0.149938 5 70 d7 00 cf 01
E: 0.155971 5 70 d9 00 cf 01
E: 0.163011 5 70 db 00 ce 01
E: 0.168984 5 70 dc 00 ce 01
E: 0.174996 5 70 de 00 cd 01
E: 0.180957 5 70 e0 00 cd 01
E: 0.187985 5 70 e1 00 cc 01
E: 0.193991 5 70 e3 00 cc 01
E: 0.199984 5 70 e5 00 cb 01
E: 0.206006 5 70 e7 00 cb 01
E: 0.212986 5 70 e9 00 ca 01
E: 0.218988 5 70 eb 00 ca 01
E: 0.224955 5 70 ed 00 c9 01
E: 0.230943 5 70 f0 00 c9 01
E: 0.237986 5 70 f2 00 c8 01
E: 0.243951 5 70 f5 00 c7 01
E: 0.262959 5 70 f8 00 c6 01
E: 0.269008 5 70 fc 00 c5 01
E: 0.274963 5 70 ff 00 c3 01
E: 0.280913 5 70 03 01 c1 01
E: 0.287962 5 70 07 01 c0 01
E: 0.293989 5 70 0a 01 bf 01
E: 0.299971 5 70 0e 01 be 01
E: 0.305973 5 70 12 01 bd 01
E: 0.313004 5 70 15 01 bc 01
E: 0.318966 5 70 18 01 bb 01
E: 0.324979 5 70 1b 01 ba 01
E: 0.330975 5 70 1f 01 b9 01
E: 0.337957 5 70 22 01 b8 01
E: 0.343986 5 70 25 01 b7 01
E: 0.349924 5 70 28 01 b5 01
E: 0.355963 5 70 2c 01 b4 01
E: 0.362928 5 70 2f 01 b3 01
E: 0.368975 5 70 33 01 b1 01
E: 0.374911 5 70 36 01 b0 01
E: 0.380961 5 70 39 01 af 01
E: 0.387969 5 70 3d 01 af 01
E: 0.393916 5 70 40 01 ae 01
E: 0.399961 5 70 43 01 ad 01
E: 0.405957 5 70 47 01 ad 01
E: 0.412947 5 70 4a 01 ac 01
E: 0.418982 5 70 4d 01 ab 01
E: 0.424983 5 70 50 01 aa 01
E: 0.430989 5 70 54 01 a9 01
E: 0.437946 5 70 57 01 a8 01
E: 0.443999 5 70 5a 01 a7 01
E: 0.449980 5 70 5d 01 a6 01
E: 0.455972 5 70 61 01 a5 01
E: 0.462986 5 70 64 01 a4 01
E: 0.468994 5 70 67 01 a3 01
E: 0.474981 5 70 6a 01 a3 01
E: 0.480990 5 70 6c 01 a2 01
E: 0.487974 5 70 6f 01 a1 01
E: 0.493988 5 70 72 01 a1 01
E: 0.500009 5 70 75 01 a0 01
E: 0.505973 5 70 77 01 9f 01
E: 0.512990 5 70 7a 01 9f 01
E: 0.518971 5 70 7d 01 9e 01
E: 0.524988 5 70 7f 01 9e 01
E: 0.530969 5 70 81 01 9e 01
E: 0.537991 5 70 83 01 9d 01
E: 0.543979 5 70 86 01 9d 01
E: 0.549988 5 70 88 01 9c 01
E: 0.555973 5 70 8a 01 9c 01
E: 0.562958 5 70 8c 01 9c 01
E: 0.568917 5 70 8d 01 9b 01
E: 0.574984 5 70 8f 01 9b 01
E: 0.580986 5 70 92 01 9b 01
E: 0.587984 5 70 94 01 9a 01
E: 0.593990 5 70 95 01 9a 01
E: 0.599959 5 70 97 01 99 01
E: 0.605980 5 70 97 01 99 01
E: 0.612983 5 70 98 01 99 01
E: 0.618990 5 70 9a 01 98 01
E: 0.624972 5 70 9b 01 98 01
E: 0.630983 5 70 9c 01 97 01
E: 0.638007 5 70 9e 01 97 01
E: 0.643953 5 70 9f 01 97 01
E: 0.649986 5 70 a1 01 96 01
E: 0.655975 5 70 a1 01 96 01
E: 0.662980 5 70 a3 01 96 01
E: 0.668993 5 70 a4 01 95 01
> If the default binding does not work and if we can not make this work
> through hid-multitouch, then I'll have some comments on your patch,
> but I keep that for later :)
>
Greets
--
Christian Gmeiner, MSc
https://soundcloud.com/christian-gmeiner
> Cheers,
> Benjamin
>
> [1] http://bentiss.github.io/hid-replay-docs/
>
>
>
>>
>> As I only have a PenMount 6000 to test I removed the multi touch support from the dirver.
>>
>> Bus 002 Device 006: ID 14e1:6000 Dialogue Technology Corp.
>> Device Descriptor:
>> bLength 18
>> bDescriptorType 1
>> bcdUSB 1.10
>> bDeviceClass 0 (Defined at Interface level)
>> bDeviceSubClass 0
>> bDeviceProtocol 0
>> bMaxPacketSize0 64
>> idVendor 0x14e1 Dialogue Technology Corp.
>> idProduct 0x6000
>> bcdDevice a4.b4
>> iManufacturer 1 DIALOGUE INC
>> iProduct 2 PenMount USB
>> iSerial 0
>> bNumConfigurations 1
>> Configuration Descriptor:
>> bLength 9
>> bDescriptorType 2
>> wTotalLength 41
>> bNumInterfaces 1
>> bConfigurationValue 1
>> iConfiguration 4 full speed
>> bmAttributes 0xa0
>> (Bus Powered)
>> Remote Wakeup
>> MaxPower 500mA
>> Interface Descriptor:
>> bLength 9
>> bDescriptorType 4
>> bInterfaceNumber 0
>> bAlternateSetting 0
>> bNumEndpoints 2
>> bInterfaceClass 3 Human Interface Device
>> bInterfaceSubClass 0 No Subclass
>> bInterfaceProtocol 0 None
>> iInterface 3 EndPoint1 Interrupt Pipe
>> HID Device Descriptor:
>> bLength 9
>> bDescriptorType 33
>> bcdHID 1.01
>> bCountryCode 0 Not supported
>> bNumDescriptors 1
>> bDescriptorType 34 Report
>> wDescriptorLength 76
>> Report Descriptors:
>> ** UNAVAILABLE **
>> Endpoint Descriptor:
>> bLength 7
>> bDescriptorType 5
>> bEndpointAddress 0x81 EP 1 IN
>> bmAttributes 3
>> Transfer Type Interrupt
>> Synch Type None
>> Usage Type Data
>> wMaxPacketSize 0x0005 1x 5 bytes
>> bInterval 1
>> Endpoint Descriptor:
>> bLength 7
>> bDescriptorType 5
>> bEndpointAddress 0x02 EP 2 OUT
>> bmAttributes 3
>> Transfer Type Interrupt
>> Synch Type None
>> Usage Type Data
>> wMaxPacketSize 0x0005 1x 5 bytes
>> bInterval 1
>> Device Status: 0x0000
>> (Bus Powered)
>>
>> Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com>
>> ---
>> drivers/hid/Kconfig | 6 ++
>> drivers/hid/Makefile | 1 +
>> drivers/hid/hid-core.c | 1 +
>> drivers/hid/hid-ids.h | 1 +
>> drivers/hid/hid-penmount.c | 200 +++++++++++++++++++++++++++++++++++++++++++++
>> 5 files changed, 209 insertions(+)
>> create mode 100644 drivers/hid/hid-penmount.c
>>
>> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
>> index c18d5d7..0351b66 100644
>> --- a/drivers/hid/Kconfig
>> +++ b/drivers/hid/Kconfig
>> @@ -530,6 +530,12 @@ config PANTHERLORD_FF
>> Say Y here if you have a PantherLord/GreenAsia based game controller
>> or adapter and want to enable force feedback support for it.
>>
>> +config HID_PENMOUNT
>> + tristate "Penmount touch device"
>> + depends on USB_HID
>> + ---help---
>> + Say Y here if you have a Penmount based touch controller.
>> +
>> config HID_PETALYNX
>> tristate "Petalynx Maxter remote control"
>> depends on HID
>> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
>> index 4dbac7f..e2850d8 100644
>> --- a/drivers/hid/Makefile
>> +++ b/drivers/hid/Makefile
>> @@ -71,6 +71,7 @@ obj-$(CONFIG_HID_NTRIG) += hid-ntrig.o
>> obj-$(CONFIG_HID_ORTEK) += hid-ortek.o
>> obj-$(CONFIG_HID_PRODIKEYS) += hid-prodikeys.o
>> obj-$(CONFIG_HID_PANTHERLORD) += hid-pl.o
>> +obj-$(CONFIG_HID_PENMOUNT) += hid-penmount.o
>> obj-$(CONFIG_HID_PETALYNX) += hid-petalynx.o
>> obj-$(CONFIG_HID_PICOLCD) += hid-picolcd.o
>> hid-picolcd-y += hid-picolcd_core.o
>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>> index 12b6e67..6827196 100644
>> --- a/drivers/hid/hid-core.c
>> +++ b/drivers/hid/hid-core.c
>> @@ -1880,6 +1880,7 @@ static const struct hid_device_id hid_have_special_driver[] = {
>> { HID_USB_DEVICE(USB_VENDOR_ID_NTRIG, USB_DEVICE_ID_NTRIG_TOUCH_SCREEN_18) },
>> { HID_USB_DEVICE(USB_VENDOR_ID_ORTEK, USB_DEVICE_ID_ORTEK_PKB1700) },
>> { HID_USB_DEVICE(USB_VENDOR_ID_ORTEK, USB_DEVICE_ID_ORTEK_WKB2000) },
>> + { HID_USB_DEVICE(USB_VENDOR_ID_PENMOUNT, USB_DEVICE_ID_PENMOUNT_6000) },
>> { HID_USB_DEVICE(USB_VENDOR_ID_PETALYNX, USB_DEVICE_ID_PETALYNX_MAXTER_REMOTE) },
>> { HID_USB_DEVICE(USB_VENDOR_ID_PRIMAX, USB_DEVICE_ID_PRIMAX_KEYBOARD) },
>> #if IS_ENABLED(CONFIG_HID_ROCCAT)
>> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
>> index 25cd674..3943ffe 100644
>> --- a/drivers/hid/hid-ids.h
>> +++ b/drivers/hid/hid-ids.h
>> @@ -722,6 +722,7 @@
>> #define USB_DEVICE_ID_PENMOUNT_PCI 0x3500
>> #define USB_DEVICE_ID_PENMOUNT_1610 0x1610
>> #define USB_DEVICE_ID_PENMOUNT_1640 0x1640
>> +#define USB_DEVICE_ID_PENMOUNT_6000 0x6000
>>
>> #define USB_VENDOR_ID_PETALYNX 0x18b1
>> #define USB_DEVICE_ID_PETALYNX_MAXTER_REMOTE 0x0037
>> diff --git a/drivers/hid/hid-penmount.c b/drivers/hid/hid-penmount.c
>> new file mode 100644
>> index 0000000..3a630d1
>> --- /dev/null
>> +++ b/drivers/hid/hid-penmount.c
>> @@ -0,0 +1,200 @@
>> +/*
>> + * HID driver for PenMount touchscreens
>> + *
>> + * Copyright (c) 2011 PenMount Touch Solutions <penmount@seed.net.tw>
>> + *
>> + */
>> +
>> +/*
>> + * 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/module.h>
>> +#include <linux/hid.h>
>> +#include <linux/version.h>
>> +#include <linux/input/mt.h>
>> +#include "hid-ids.h"
>> +
>> +struct penmount_priv {
>> + u8 touch;
>> + bool touching;
>> + u16 x;
>> + u16 y;
>> +};
>> +
>> +static int penmount_input_mapping(struct hid_device *hdev,
>> + struct hid_input *hi, struct hid_field *field,
>> + struct hid_usage *usage, unsigned long **bit, int *max)
>> +{
>> + struct input_dev *dev = hi->input;
>> + int mapped = 0;
>> +
>> + 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_X);
>> + mapped = 1;
>> + break;
>> + case HID_GD_Y:
>> + hid_map_usage(hi, usage, bit, max, EV_ABS, ABS_Y);
>> + mapped = 1;
>> + break;
>> + }
>> + break;
>> + case HID_UP_BUTTON:
>> + hid_map_usage(hi, usage, bit, max, EV_KEY, BTN_TOUCH);
>> + mapped = 1;
>> + break;
>> + case HID_UP_DIGITIZER:
>> + switch (usage->hid) {
>> + case HID_DG_TIPSWITCH:
>> + hid_map_usage(hi, usage, bit, max, EV_KEY, BTN_TOUCH);
>> + mapped = 1;
>> + break;
>> + case HID_DG_CONTACTID:
>> + input_mt_init_slots(dev, 1, 0);
>> + mapped = -1;
>> + break;
>> + case HID_DG_INRANGE:
>> + case HID_DG_CONFIDENCE:
>> + mapped = -1;
>> + break;
>> + }
>> + break;
>> + }
>> +
>> + return mapped;
>> +}
>> +
>> +static void penmount_process(struct input_dev *dev, struct penmount_priv *priv)
>> +{
>> + if (!priv->touching) {
>> + if (priv->touch) {
>> + input_report_key(dev, BTN_TOUCH, 1);
>> + priv->touching = true;
>> + }
>> + } else {
>> + if (!priv->touch) {
>> + input_report_key(dev, BTN_TOUCH, 0);
>> + priv->touching = false;
>> + }
>> + }
>> +
>> + input_report_abs(dev, ABS_X, priv->x);
>> + input_report_abs(dev, ABS_Y, priv->y);
>> + input_sync(dev);
>> +
>> + priv->touch = 0;
>> +}
>> +
>> +static int penmount_event(struct hid_device *hdev, struct hid_field *field,
>> + struct hid_usage *usage, __s32 value)
>> +{
>> + if (hdev->claimed & HID_CLAIMED_INPUT) {
>> + struct penmount_priv *priv = hid_get_drvdata(hdev);
>> + struct input_dev *dev = field->hidinput->input;
>> +
>> + switch (usage->hid) {
>> + case HID_DG_TIPSWITCH:
>> + priv->touch = value;
>> + break;
>> + case HID_GD_X:
>> + priv->x = value;
>> + break;
>> + case HID_GD_Y:
>> + priv->y = value;
>> + break;
>> + default:
>> + /* fallback to the generic hidinput handling */
>> + return 0;
>> + }
>> +
>> + penmount_process(dev, priv);
>> + }
>> +
>> + if ((hdev->claimed & HID_CLAIMED_HIDDEV) && (hdev->hiddev_hid_event))
>> + hdev->hiddev_hid_event(hdev, field, usage, value);
>> +
>> + return 1;
>> +}
>> +
>> +static int penmount_probe(struct hid_device *hdev,
>> + const struct hid_device_id *id)
>> +{
>> + struct penmount_priv *priv;
>> + int ret = 0;
>> +
>> + priv = kzalloc(sizeof(*priv), GFP_KERNEL);
>> + if (!priv)
>> + return -ENOMEM;
>> +
>> + hid_set_drvdata(hdev, priv);
>> +
>> + ret = hid_parse(hdev);
>> + if (ret) {
>> + hid_err(hdev, "parse failed\n");
>> + goto err_free;
>> + }
>> +
>> + ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
>> + if (ret) {
>> + hid_err(hdev, "hw start failed\n");
>> + goto err_free;
>> + }
>> +
>> + return 0;
>> +
>> +err_free:
>> + kfree(priv);
>> + return ret;
>> +}
>> +
>> +static void penmount_remove(struct hid_device *hdev)
>> +{
>> + struct penmount_priv *priv = hid_get_drvdata(hdev);
>> +
>> + hid_hw_stop(hdev);
>> + kfree(priv);
>> +}
>> +
>> +static const struct hid_device_id penmount_devices[] = {
>> + { HID_USB_DEVICE(USB_VENDOR_ID_PENMOUNT, USB_DEVICE_ID_PENMOUNT_6000) },
>> + { }
>> +};
>> +MODULE_DEVICE_TABLE(hid, penmount_devices);
>> +
>> +static const struct hid_usage_id penmount_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 penmount_driver = {
>> + .name = "hid-penmount",
>> + .id_table = penmount_devices,
>> + .probe = penmount_probe,
>> + .remove = penmount_remove,
>> + .input_mapping = penmount_input_mapping,
>> + .usage_table = penmount_usages,
>> + .event = penmount_event,
>> +};
>> +
>> +static int __init penmount_init(void)
>> +{
>> + return hid_register_driver(&penmount_driver);
>> +}
>> +
>> +static void __exit penmount_exit(void)
>> +{
>> + hid_unregister_driver(&penmount_driver);
>> +}
>> +
>> +module_init(penmount_init);
>> +module_exit(penmount_exit);
>> +
>> +MODULE_AUTHOR("PenMount Touch Solutions <penmount@seed.net.tw>");
>> +MODULE_DESCRIPTION("PenMount HID TouchScreen Driver");
>> +MODULE_LICENSE("GPL");
>> --
>> 1.9.3
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-input" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] Input: ALPS - suppress message about 'Unknown touchpad'
From: Hans de Goede @ 2014-08-26 8:20 UTC (permalink / raw)
To: Dmitry Torokhov, linux-input; +Cc: Yunkang Tang, Paul Menzel, linux-kernel
In-Reply-To: <20140825183031.GA5151@core.coreip.homeip.net>
Hi,
On 08/25/2014 08:30 PM, Dmitry Torokhov wrote:
> When we fail to match data returned by E7 and EC reports we state that we
> found "Unknown ALPS touchpad" whereas it is most likely it is not ALPS
> touchpad at all. Change wording a bit and reduce the message to debug so
> that it does not litter users logs and confuse them.
>
> Reported-by: Paul Menzel <paulepanter@users.sourceforge.net>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
I noticed this too, and I agree that it is a confusing message:
Acked-by: Hans de Goede <hdegoede@redhat.com>
Regards,
Hans
> ---
> drivers/input/mouse/alps.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
> index a59a1a6..a956b98 100644
> --- a/drivers/input/mouse/alps.c
> +++ b/drivers/input/mouse/alps.c
> @@ -2234,8 +2234,8 @@ static int alps_identify(struct psmouse *psmouse, struct alps_data *priv)
> return 0;
> }
>
> - psmouse_info(psmouse,
> - "Unknown ALPS touchpad: E7=%3ph, EC=%3ph\n", e7, ec);
> + psmouse_dbg(psmouse,
> + "Likely not an ALPS touchpad: E7=%3ph, EC=%3ph\n", e7, ec);
>
> return -EINVAL;
> }
>
^ permalink raw reply
* Re: [PATCH v2 0/7] Second batch of cleanups for cros_ec
From: Javier Martinez Canillas @ 2014-08-26 8:49 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Lee Jones, Wolfram Sang, Doug Anderson, Simon Glass,
Bill Richardson, Andrew Bresticker, Derek Basehore, Todd Broch,
Olof Johansson, linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20140825180152.GA31693-WlK9ik9hQGAhIp7JRqBPierSzoNAToWh@public.gmane.org>
Hello Dmitry,
On 08/25/2014 08:01 PM, Dmitry Torokhov wrote:
> On Mon, Aug 25, 2014 at 07:28:01PM +0200, Javier Martinez Canillas wrote:
>> >
>> > #7 does not apply to my tree (I guess it depends on the 1st batch which I
>> > expect will go through MFD tree?). Maybe you could rebase it on top of my next
>> > so it can be applied sooner? Or it really needs parts of patchset #1?
>> >
>>
>> The first batch sent by Doug did indeed touch this driver and the patches
>> were merged through the MFD tree for 3.17 as you said. I see that your
>> next branch is based on 3.16-rc6 and that is why it does not apply cleanly.
>>
>> I guess you will rebase your next branch for 3.18 on top of 3.17-rc1
>> anyways which will fix this issue? If not please let me know and I can of
>> course re-spin the patch so it applies cleanly on top of 3.16-rc6.
>
> I normally merge with mainline around -rc3 so please ping me around that time
> if you do not see the patch in my tree.
>
Will do, thanks a lot for your help!
> Thanks.
>
Best regards,
Javier
^ permalink raw reply
* Re: [PATCH] HID: add support for PenMount HID TouchScreen Driver
From: Benjamin Tissoires @ 2014-08-26 18:47 UTC (permalink / raw)
To: Christian Gmeiner; +Cc: linux-input
In-Reply-To: <CAH9NwWe7FSmvVx+BNf7gsjOm9oWLEO7CunHrZjPUwkxiKWLb9w@mail.gmail.com>
On Tue, Aug 26, 2014 at 3:29 AM, Christian Gmeiner
<christian.gmeiner@gmail.com> wrote:
> Hi Benjamin.
>
> 2014-08-25 17:43 GMT+02:00 Benjamin Tissoires <benjamin.tissoires@gmail.com>:
>> On Mon, Aug 25, 2014 at 10:48 AM, Christian Gmeiner
>> <christian.gmeiner@gmail.com> wrote:
>>> This driver is a cleaned up version of
>>> http://git.android-x86.org/?p=kernel/cdv.git;a=blob_plain;f=drivers/hid/hid-penmount.c;hb=HEAD
>>
>> This is definitively weird. According to your driver, it should go by
>> default through hid-multitouch and behave correctly (at least the
>> multi-touch versions, and I would say yours too).
>>
>
> I thought I have tried it with hid-multitouch, but I it never gets used.
>
> [ 94.342542] usb 2-1.7: USB disconnect, device number 6
> [ 96.312636] usb 2-1.7: new full-speed USB device number 7 using ehci-pci
> [ 96.399606] usb 2-1.7: New USB device found, idVendor=14e1, idProduct=6000
> [ 96.399611] usb 2-1.7: New USB device strings: Mfr=1, Product=2,
> SerialNumber=0
> [ 96.399614] usb 2-1.7: Product: PenMount USB
> [ 96.399617] usb 2-1.7: Manufacturer: DIALOGUE INC
> [ 96.402777] input: DIALOGUE INC PenMount USB as
> /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.7/2-1.7:1.0/0003:14E1:6000.0004/input/input12
> [ 96.403533] hid-generic 0003:14E1:6000.0004: input,hidraw2: USB HID
> v1.01 Mouse [DIALOGUE INC PenMount USB] on usb-0000:00:1d.0-1.7/input0
Yep, it will never go through hid-multitouch because the report
descriptor does not contain the ContactID usage. So hid-generic is
fine for now.
>
>
>> Can you send me some touch recordings of your device with
>> hid-recorder[1] please? No need to remove your driver, it will capture
>> raw input events, so you should be just fine.
>
> [christian@chgm-pc hid]$ sudo hid-recorder
> Available devices:
> /dev/hidraw0: Microsoft Microsoft 3-Button Mouse with IntelliEye(TM)
> /dev/hidraw1: LITE-ON Technology USB NetVista Full Width Keyboard.
> /dev/hidraw2: DIALOGUE INC PenMount USB
> Select the device event number [0-2]: 2
> R: 76 05 01 09 02 a1 01 09 01 a1 00 09 00 15 00 25 01 95 06 75 01 81
> 01 05 09 19 01 29 03 15 00 25 01 95 02 75 01 81 02 05 01 09 30 09 31
> 15 00 26 ff 03 36 80 00 46 ff 7f 75 10 95 02 81 02 09 00 15 00 26 ff
> 00 75 08 95 05 b1 02 c0 c0
> N: DIALOGUE INC PenMount USB
> P: usb-0000:00:1d.0-1.7/input0
> I: 3 14e1 6000
> E: 0.000000 5 30 d0 00 d1 01
> E: 0.000945 5 70 d0 00 d1 01
> E: 0.006984 5 70 d0 00 d1 01
> E: 0.012961 5 70 d0 00 d1 01
> E: 0.019001 5 70 d0 00 d1 01
> E: 0.025965 5 70 d0 00 d1 01
> E: 0.031960 5 70 d0 00 d1 01
> E: 0.037964 5 70 d0 00 d1 01
> E: 0.043964 5 70 d0 00 d1 01
> E: 0.050988 5 70 d0 00 d1 01
> E: 0.056945 5 70 d0 00 d1 01
> E: 0.062985 5 70 d0 00 d1 01
> E: 0.068965 5 70 d0 00 d1 01
> E: 0.075974 5 70 d0 00 d1 01
> E: 0.081961 5 70 d0 00 d1 01
> E: 0.112985 5 70 d0 00 d1 01
> E: 0.118978 5 70 d0 00 d0 01
> E: 0.124978 5 70 d1 00 d0 01
> E: 0.131973 5 70 d3 00 d0 01
> E: 0.137971 5 70 d4 00 d0 01
> E: 0.143919 5 70 d5 00 cf 01
> E: 0.149938 5 70 d7 00 cf 01
> E: 0.155971 5 70 d9 00 cf 01
> E: 0.163011 5 70 db 00 ce 01
> E: 0.168984 5 70 dc 00 ce 01
> E: 0.174996 5 70 de 00 cd 01
> E: 0.180957 5 70 e0 00 cd 01
> E: 0.187985 5 70 e1 00 cc 01
> E: 0.193991 5 70 e3 00 cc 01
> E: 0.199984 5 70 e5 00 cb 01
> E: 0.206006 5 70 e7 00 cb 01
> E: 0.212986 5 70 e9 00 ca 01
> E: 0.218988 5 70 eb 00 ca 01
> E: 0.224955 5 70 ed 00 c9 01
> E: 0.230943 5 70 f0 00 c9 01
> E: 0.237986 5 70 f2 00 c8 01
> E: 0.243951 5 70 f5 00 c7 01
> E: 0.262959 5 70 f8 00 c6 01
> E: 0.269008 5 70 fc 00 c5 01
> E: 0.274963 5 70 ff 00 c3 01
> E: 0.280913 5 70 03 01 c1 01
> E: 0.287962 5 70 07 01 c0 01
> E: 0.293989 5 70 0a 01 bf 01
> E: 0.299971 5 70 0e 01 be 01
> E: 0.305973 5 70 12 01 bd 01
> E: 0.313004 5 70 15 01 bc 01
> E: 0.318966 5 70 18 01 bb 01
> E: 0.324979 5 70 1b 01 ba 01
> E: 0.330975 5 70 1f 01 b9 01
> E: 0.337957 5 70 22 01 b8 01
> E: 0.343986 5 70 25 01 b7 01
> E: 0.349924 5 70 28 01 b5 01
> E: 0.355963 5 70 2c 01 b4 01
> E: 0.362928 5 70 2f 01 b3 01
> E: 0.368975 5 70 33 01 b1 01
> E: 0.374911 5 70 36 01 b0 01
> E: 0.380961 5 70 39 01 af 01
> E: 0.387969 5 70 3d 01 af 01
> E: 0.393916 5 70 40 01 ae 01
> E: 0.399961 5 70 43 01 ad 01
> E: 0.405957 5 70 47 01 ad 01
> E: 0.412947 5 70 4a 01 ac 01
> E: 0.418982 5 70 4d 01 ab 01
> E: 0.424983 5 70 50 01 aa 01
> E: 0.430989 5 70 54 01 a9 01
> E: 0.437946 5 70 57 01 a8 01
> E: 0.443999 5 70 5a 01 a7 01
> E: 0.449980 5 70 5d 01 a6 01
> E: 0.455972 5 70 61 01 a5 01
> E: 0.462986 5 70 64 01 a4 01
> E: 0.468994 5 70 67 01 a3 01
> E: 0.474981 5 70 6a 01 a3 01
> E: 0.480990 5 70 6c 01 a2 01
> E: 0.487974 5 70 6f 01 a1 01
> E: 0.493988 5 70 72 01 a1 01
> E: 0.500009 5 70 75 01 a0 01
> E: 0.505973 5 70 77 01 9f 01
> E: 0.512990 5 70 7a 01 9f 01
> E: 0.518971 5 70 7d 01 9e 01
> E: 0.524988 5 70 7f 01 9e 01
> E: 0.530969 5 70 81 01 9e 01
> E: 0.537991 5 70 83 01 9d 01
> E: 0.543979 5 70 86 01 9d 01
> E: 0.549988 5 70 88 01 9c 01
> E: 0.555973 5 70 8a 01 9c 01
> E: 0.562958 5 70 8c 01 9c 01
> E: 0.568917 5 70 8d 01 9b 01
> E: 0.574984 5 70 8f 01 9b 01
> E: 0.580986 5 70 92 01 9b 01
> E: 0.587984 5 70 94 01 9a 01
> E: 0.593990 5 70 95 01 9a 01
> E: 0.599959 5 70 97 01 99 01
> E: 0.605980 5 70 97 01 99 01
> E: 0.612983 5 70 98 01 99 01
> E: 0.618990 5 70 9a 01 98 01
> E: 0.624972 5 70 9b 01 98 01
> E: 0.630983 5 70 9c 01 97 01
> E: 0.638007 5 70 9e 01 97 01
> E: 0.643953 5 70 9f 01 97 01
> E: 0.649986 5 70 a1 01 96 01
> E: 0.655975 5 70 a1 01 96 01
> E: 0.662980 5 70 a3 01 96 01
> E: 0.668993 5 70 a4 01 95 01
>
Weird, this trace is missing the release from the finger (it should
start with 5 30 ...)
As I understand, the only problem which your driver is trying to solve
is to map the button 0 emitted by your touchscreen to BTN_TOUCH (I
have the feeling that you already said that, but I can not find it in
the thread :/ -> found your other thread).
You have 2 possibilities:
1. fix the report descriptor and rely on the generic implementations
2. just handle the button mapping in this penmount driver and leave
the rest to the generic code.
The choice 2. might be easier if you never played with report
descriptors. You will just have to heavily remove stuffs from your
current version:
- struct penmount_priv is not required anymore
- in penmount_input_mapping() just map HID_UP_BUTTON to BTN_TOUCH, and
return 0 for everything else
- remove penmount_process() and penmount_event()
- no need to have a probe or a remove function anymore
- use the macro module_hid_driver(penmount_driver) instead of having
your own penmount_init() and penmount_exit()
You might also want to say that you are the author of the module and
also just say that you based your work on a hid-penmount copyrighted
by PenMount Touch Solution <...>.
The code should be very different by now, so there is no need IMO to
keep the author when you completely have rewritten the code.
Cheers,
Benjamin
^ permalink raw reply
* [PATCH 1/1] Input: elantech: Correct the sign of the x/y movement for trackpoint
From: Ulrik De Bie @ 2014-08-26 19:30 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: linux-input, Hans de Goede, David Herrmann, ulrik.debie-os
In-Reply-To: <20140824221424.GA15090@core.coreip.homeip.net>
Hi Dmitry,
Thank you for your feedback !
The x/y movement calculation was wrong, probably caused by my wrong documentation at the beginning of the elantech_report_trackpoint function.
When applying this patch to your patch, it works.
Thanks !
--
Ulrik
Input: elantech: Correct the sign of the x/y movement for trackpoint
From: Ulrik De Bie <ulrik.debie-os@e2big.org>
The sign bit in the documentation of the trackpoint x/y movement is corrected.
The sign bit in the calculation of the x/y movement for trackpoint is corrected.
Signed-off-by: Ulrik De Bie <ulrik.debie-os@e2big.org>
---
drivers/input/mouse/elantech.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/input/mouse/elantech.c b/drivers/input/mouse/elantech.c
index b4d645a..63533e5 100644
--- a/drivers/input/mouse/elantech.c
+++ b/drivers/input/mouse/elantech.c
@@ -408,10 +408,10 @@ static void elantech_report_trackpoint(struct psmouse *psmouse,
int packet_type)
{
/*
- * byte 0: 0 0 ~sx ~sy 0 M R L
- * byte 1: sx 0 0 0 0 0 0 0
- * byte 2: sy 0 0 0 0 0 0 0
- * byte 3: 0 0 sy sx 0 1 1 0
+ * byte 0: 0 0 sx sy 0 M R L
+ * byte 1:~sx 0 0 0 0 0 0 0
+ * byte 2:~sy 0 0 0 0 0 0 0
+ * byte 3: 0 0 ~sy ~sx 0 1 1 0
* byte 4: x7 x6 x5 x4 x3 x2 x1 x0
* byte 5: y7 y6 y5 y4 y3 y2 y1 y0
*
@@ -443,8 +443,8 @@ static void elantech_report_trackpoint(struct psmouse *psmouse,
case 0x16008020U:
case 0x26800010U:
case 0x36808000U:
- x = packet[4] - (int)(packet[1] << 1);
- y = (int)(packet[2] << 1) - packet[5];
+ x = packet[4] - (int)((packet[1]^0x80) << 1);
+ y = (int)((packet[2]^0x80) << 1) - packet[5];
input_report_key(tp_dev, BTN_LEFT, packet[0] & 0x01);
input_report_key(tp_dev, BTN_RIGHT, packet[0] & 0x02);
--
2.1.0.rc1
^ permalink raw reply related
* Re: [PATCH 2/2] Input: atmel_mxt_ts - mXT224 DMA quirk was fixed in firmware v2.0.AA
From: Benson Leung @ 2014-08-26 20:10 UTC (permalink / raw)
To: Nick Dyer
Cc: Dmitry Torokhov, Yufeng Shen, Daniel Kurtz, Henrik Rydberg,
Joonyoung Shim, Alan Bowens, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org, Peter Meerwald, Olof Johansson,
Sekhar Nori, Stephen Warren
In-Reply-To: <1407420438-6289-3-git-send-email-nick.dyer@itdev.co.uk>
On Thu, Aug 7, 2014 at 7:07 AM, <nick.dyer@itdev.co.uk> wrote:
>
> From: Nick Dyer <nick.dyer@itdev.co.uk>
>
> Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk>
Reviewed-by: Benson Leung <bleung@chromium.org>
--
Benson Leung
Software Engineer, Chrom* OS
bleung@chromium.org
^ permalink raw reply
* Re: [PATCH 3/3] Add logitech m560 driver
From: Benjamin Tissoires @ 2014-08-26 21:22 UTC (permalink / raw)
To: Nestor Lopez Casado
Cc: Goffredo Baroncelli, open list:HID CORE LAYER,
Goffredo Baroncelli
In-Reply-To: <CAE7qMrpgPULMq95wZhLgsLWXJ+=DiVoe9XzxZB5poU0ZmqFiXg@mail.gmail.com>
Hi guys,
On Mon, Aug 25, 2014 at 4:38 AM, Nestor Lopez Casado
<nlopezcasad@logitech.com> wrote:
> Hi,
>
> If you want to have a special driver for a specific Unifying device
> you should put in place the right structure to have a separate driver
> for it.
>
> hid-logitech-dj is a "bus" driver for the Unifying *receiver*, not a
> driver for any specific Unifying device.
>
> Benjamin Tisssoires (github bentiss) has uploaded a few patches to his
> github account to finish the infrastructure for specific Unifying
> devices drivers. Look for the more complete branch 'for-whot' See how
> he created a hid-logitech-wtp driver for the Different wireless
> touchpads.
>
> This would be the right approach to what you are trying to do.
>
I have been willing to review/answer to this patch series since
Monday, but did not found the time to do so. I have a small spot now,
so here I am.
So, Goffredo, as Nestor said, I wouldn't have coded this in the way of
your patch. However, do not take Nestor's words as a strong rejection
and that we do not want you to submit anything else :)
I am in fault here because I should have sent upstream the branch
Nestor pointed at a long time ago now, and this would have avoided you
a lot of pain.
The thing which prevented me to send this upstream is that Jiri tries
to group the various hid drivers by vendor, and this branch creates 2
new for supporting various dj/hid++ devices.
I have though one specific comment Goffredo with your patch (again I
did not went in depth of them, but this jumped into my eyes). Why
don't you simply use the wireless ID instead of the name of the
device. The wireless id is unique across all Logitech unifying
devices, and it's definitively easier to compare than a string.
One other thing is may be answered by Nestor: is there any command we
can send to the device for it to send only regular buttons? This may
just be an easier solution than having to remap the keyboards events
into buttons.
Cheers,
Benjamin
> On Sat, Aug 23, 2014 at 2:40 PM, Goffredo Baroncelli <kreijack@gmail.com> wrote:
>> Add logitech m560 support. In the init phase the driver
>> send a sequence which avoid some unnecessary key sending
>> from the mouse.
>>
>> The mouse appears as a couple of mouse and keyboard. Some buttons
>> emit a key event instead of the "mouse button" event. However
>> some event (like the middle button release) aren't generated.
>> Fortunately, the device generates an additional event to
>> track it. The event type is 0x0a.
>>
>> Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it>
>> ---
>> drivers/hid/hid-logitech-dj.c | 92 +++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 92 insertions(+)
>>
>> diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
>> index feddd3d..40c5ea1 100644
>> --- a/drivers/hid/hid-logitech-dj.c
>> +++ b/drivers/hid/hid-logitech-dj.c
>> @@ -222,7 +222,99 @@ static inline void call_destroy(struct dj_device *djdev)
>> djdev->methods->destroy(djdev);
>> }
>>
>> +/*
>> + * Send the sequence 10xx0a35 00af03
>> + * to the mouse id xx. It disables the key sending by the central button.
>> + */
>> +
>> +static int lg_m560_init_device(struct dj_device *dj_device)
>> +{
>> + struct dj_report *dj_report;
>> + int retval;
>> + static u8 reset_data[] = {0x35, 0x00, 0xaf, 0x03};
>> +
>> + dj_report = kzalloc(sizeof(struct dj_report), GFP_KERNEL);
>> + if (!dj_report)
>> + return -ENOMEM;
>> +
>> + dj_device->userdata = dj_report;
>> +
>> + dj_report->report_id = REPORT_ID_RECV_SHORT;
>> + dj_report->device_index = dj_device->device_index;
>> + dj_report->report_type = 0x0a;
>> +
>> + memcpy(dj_report->report_params, reset_data, sizeof(reset_data));
>> +
>> + retval = hid_hw_raw_request(dj_device->dj_receiver_dev->hdev,
>> + dj_report->report_id,
>> + (void *)dj_report, REPORT_ID_RECV_SHORT_LENGTH,
>> + HID_OUTPUT_REPORT, HID_REQ_SET_REPORT);
>> +
>> + memset(dj_report, 0, sizeof(struct dj_report));
>> +
>> + return retval;
>> +}
>> +
>> +static int lg_m560_parse_raw_event(struct dj_device *dj_device,
>> + struct dj_report *dj_report)
>> +{
>> + u8 *m506_last_mouse_report = dj_device->userdata;
>> +
>> + BUG_ON(!dj_device);
>> +
>> + /* don't pass keys when the mouse is a m560 */
>> + if (dj_report->report_type == REPORT_TYPE_KEYBOARD)
>> + return true;
>> +
>> + if (dj_report->report_id == 0x11 && dj_report->report_type == 0x0a) {
>> + int btn;
>> +
>> + /* check if the event is a button */
>> + btn = dj_report->report_params[2];
>> + if (btn != 0x00 && btn != 0xb0 && btn != 0xae && btn != 0xaf)
>> + return true;
>> +
>> + if (btn == 0xaf)
>> + m506_last_mouse_report[1] |= 4;
>> + if (btn == 0xae)
>> + m506_last_mouse_report[2] |= 2;
>> + if (btn == 0xb0)
>> + m506_last_mouse_report[2] |= 4;
>> + if (btn == 0x00) {
>> + m506_last_mouse_report[1] &= ~4;
>> + m506_last_mouse_report[2] &= ~(4|2);
>> + }
>> +
>> + if (hid_input_report(dj_device->hdev, HID_INPUT_REPORT,
>> + m506_last_mouse_report, 8, 1)) {
>> + dbg_hid("hid_input_report error\n");
>> + }
>> + return true;
>> + }
>> +
>> + /* copy the button status */
>> + if (dj_report->report_type == REPORT_TYPE_MOUSE) {
>> + /* copy only the first 3 bytes: type, btn0..7, btn8..16 */
>> + memcpy(dj_device->userdata,
>> + &dj_report->report_type, 3);
>> + }
>> +
>> + /* continue with the standard handler */
>> + return false;
>> +}
>> +
>> +static void lg_m560_destroy(struct dj_device *dev)
>> +{
>> + kfree(dev->userdata);
>> +}
>> +
>> static struct dj_device_method dj_device_method[] = {
>> + { /* logitech M560 */
>> + .device_names = (char *[]){ "M560", NULL },
>> + .init_device = lg_m560_init_device,
>> + .parse_raw_event = lg_m560_parse_raw_event,
>> + .destroy = lg_m560_destroy
>> + },
>>
>> /* last element */
>> { NULL, }
>> --
>> 1.9.3
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-input" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 1/3] Don't copy more than 'count' byte
From: Benjamin Tissoires @ 2014-08-26 21:23 UTC (permalink / raw)
To: Goffredo Baroncelli; +Cc: linux-input, Goffredo Baroncelli
In-Reply-To: <1408797629-3474-2-git-send-email-kreijack@inwind.it>
On Sat, Aug 23, 2014 at 8:40 AM, Goffredo Baroncelli <kreijack@gmail.com> wrote:
> Don't copy more than 'count' byte, because *buf may be
> shorter than DJREPORT_SHORT_LENGTH,and this means an
> access violation. Of course count cannot be greather
> than the result buffer.
That's a really good catch, but one week late :)
See https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=51217e69697fba92a06e07e16f55c9a52d8e8945
Cheers,
Benjamin
>
> Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it>
> ---
> drivers/hid/hid-logitech-dj.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
> index 486dbde..ca0ab51 100644
> --- a/drivers/hid/hid-logitech-dj.c
> +++ b/drivers/hid/hid-logitech-dj.c
> @@ -557,7 +557,7 @@ static int logi_dj_ll_raw_request(struct hid_device *hid,
> if (!out_buf)
> return -ENOMEM;
>
> - if (count < DJREPORT_SHORT_LENGTH - 2)
> + if (count > DJREPORT_SHORT_LENGTH - 2)
> count = DJREPORT_SHORT_LENGTH - 2;
>
> out_buf[0] = REPORT_ID_DJ_SHORT;
> --
> 1.9.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 2/3] Add support for specific device methods.
From: Benjamin Tissoires @ 2014-08-26 22:27 UTC (permalink / raw)
To: Goffredo Baroncelli; +Cc: linux-input, Goffredo Baroncelli
In-Reply-To: <1408797629-3474-3-git-send-email-kreijack@inwind.it>
Hi Goffredo,
On Sat, Aug 23, 2014 at 8:40 AM, Goffredo Baroncelli <kreijack@gmail.com> wrote:
> This patch adds hook into the code to add support for specific
> device driver. The right driver is select on the basis of the name.
>
> 4 hooks are added:
> - init_device() called during the device initialization
> - parse() called during the device description generation phase
> - parse_raw_event() which handles the events from the device
> - destroy() called when the device is destroyed
We already said that, I don't like it. There is no need to re-invent a
sub-driver mechanism when we can simply use the hid subsystem. Instead
of using hid-generic for your mouse, we should use a
hid-logitech-generic-device-or-whatever because really, the DJ part of
the Logitech specification is really only a transport layer on top of
hid for hid devices.
>
> The construction of the device is dived in two phases:
> 1) the device is discovered by the receiver. The struct dj_device
> is allocated BUT the device is not enabled.
> It is asked the device name top the device itself
> 2) when the device name arrived from the device, the
> the right driver is selected, structure dj_device is finalized,
> the hid-device id created.
> If the device name returned doesn't match, the generic driver
> (e.g. the one provided until now) is used.
>
>
I have some general comments, and some specific ones (inlined in the code):
- I don't know if you used the gmail web interface or git send-email,
but I am pretty sure that your patch does not pass the tests found in
./scripts/check_patch.pl at the root of the kernel tree. This way, it
will fix ident issues, and some basic coding style issues
- this patch is just too huge to be included as this. You should
consider splitting it in some small subpatches, each of them should
make sense by itself. You can already start looking into having a
separate patch to retrieve the name, one to introduce
logi_djrcv2djdev(), etc...
- do not try to fix extra blank lines in a patch which have functional
changes. This just had some noise and does not help understanding
where the patch focuses.
- please rebase the changes to latest Jiri's branch for-3.18/logitech
which is still not pushed apparently :) I had one cleanup patch from
last week which will clash with your series.
the rest is inlined
> Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it>
> ---
> drivers/hid/hid-logitech-dj.c | 422 ++++++++++++++++++++++++++++++++++--------
> drivers/hid/hid-logitech-dj.h | 41 ++++
> 2 files changed, 388 insertions(+), 75 deletions(-)
>
> diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
> index ca0ab51..feddd3d 100644
> --- a/drivers/hid/hid-logitech-dj.c
> +++ b/drivers/hid/hid-logitech-dj.c
> @@ -193,7 +193,51 @@ static const u8 hid_reportid_size_map[NUMBER_OF_HID_REPORTS] = {
>
> static struct hid_ll_driver logi_dj_ll_driver;
>
> +static inline struct dj_device *logi_djrcv2djdev(
> + struct dj_receiver_dev *djrcv_dev,
> + int index);
Hmm, there is no need to forward declare a static function in the
general case. Especially if you are adding it. Just put your function
on top and you are good (I know, this is sometime preferred, but AFAIK
not in the kernel).
> static int logi_dj_recv_query_paired_devices(struct dj_receiver_dev *djrcv_dev);
> +static int logi_dj_recv_send_report(struct dj_receiver_dev *djrcv_dev,
> + struct dj_report *dj_report);
> +
I _think_ my previous comment applies here also.
> +static inline int call_init_device(struct dj_device *djdev)
> +{
> + if (!djdev || !djdev->methods || !djdev->methods->init_device)
> + return false;
> + return djdev->methods->init_device(djdev);
> +}
Why do you need to put the inline marker? If the function is static,
the compiler should be smart enough to guess that by itself.
> +
> +static inline int call_parse_raw_event(struct dj_device *djdev,
> + struct dj_report *djrep)
> +{
> + if (!djdev || !djdev->methods || !djdev->methods->parse_raw_event)
> + return false;
> + return djdev->methods->parse_raw_event(djdev, djrep);
> +}
> +
> +static inline void call_destroy(struct dj_device *djdev)
> +{
> + if (!djdev || !djdev->methods || !djdev->methods->destroy)
> + return;
> + djdev->methods->destroy(djdev);
> +}
> +
> +static struct dj_device_method dj_device_method[] = {
> +
> + /* last element */
> + { NULL, }
> +};
> +
> +static inline struct dj_device *logi_djrcv2djdev(
I do not like the name. Everywhere else the functions are
logi_dj_rcv_*, so please keep it in the same way.
> + struct dj_receiver_dev *djrcv_dev,
> + int index)
> +{
> + if ((index < DJ_DEVICE_INDEX_MIN) ||
> + (index > DJ_DEVICE_INDEX_MAX))
> + return NULL;
With the latest commits in Jiri's tree, I think we should not have to
care about this test. we should be smart enough to test this once and
then guarantee that it is never changed.
> +
> + return djrcv_dev->paired_dj_devices[index];
> +}
This whole function and various calls should go into its own patch.
>
> static void logi_dj_recv_destroy_djhid_device(struct dj_receiver_dev *djrcv_dev,
> struct dj_report *dj_report)
> @@ -203,7 +247,11 @@ static void logi_dj_recv_destroy_djhid_device(struct dj_receiver_dev *djrcv_dev,
> unsigned long flags;
>
> spin_lock_irqsave(&djrcv_dev->lock, flags);
> - dj_dev = djrcv_dev->paired_dj_devices[dj_report->device_index];
> + dj_dev = logi_djrcv2djdev(djrcv_dev, dj_report->device_index);
> + if (!dj_dev)
> + return;
I am not sure this test should be required. If we guarantee early
enough that the device_index is valid and that the device exists, then
this should not be necessary.
> + call_destroy(dj_dev);
> +
> djrcv_dev->paired_dj_devices[dj_report->device_index] = NULL;
> spin_unlock_irqrestore(&djrcv_dev->lock, flags);
>
> @@ -216,93 +264,270 @@ static void logi_dj_recv_destroy_djhid_device(struct dj_receiver_dev *djrcv_dev,
> }
> }
>
> -static void logi_dj_recv_add_djhid_device(struct dj_receiver_dev *djrcv_dev,
> +/*
> + * This function send a request of the device name
> + * see "Logitech hidpp 1.0 excerpt for public release" chapter 4.5.3
> + * for further information.
> + *
> + * The results are read in logi_dj_raw_event()
> + */
> +static int logi_send_name_request(struct dj_device *dj_device)
> +{
> + struct dj_report *dj_report;
> + int retval;
> +
> + dj_report = kzalloc(sizeof(struct dj_report), GFP_KERNEL);
> + if (!dj_report)
> + return -ENOMEM;
> +
> + dj_report->report_id = REPORT_ID_RECV_SHORT;
> + dj_report->device_index = REPORT_RECEIVER_INDEX;
> + dj_report->report_type = REPORT_GET_LONG_REGISTER_REQ;
> + dj_report->report_params[0] = REPORT_REG_PAIRING_INFORMATION;
> +
> + /*
> + * 0x40 device id 1, 0x41 device id 2....
> + */
> + dj_report->report_params[1] = 0x40 + dj_device->device_index - 1;
> +
> + retval = hid_hw_raw_request(dj_device->dj_receiver_dev->hdev,
> + dj_report->report_id,
> + (void *)dj_report, REPORT_ID_RECV_SHORT_LENGTH,
> + HID_OUTPUT_REPORT, HID_REQ_SET_REPORT);
> +
> + kfree(dj_report);
> + return retval;
> +}
> +
> +static int logi_dj_recv_create_dj_device(struct dj_receiver_dev *djrcv_dev,
> struct dj_report *dj_report)
> {
> /* Called in delayed work context */
> struct hid_device *djrcv_hdev = djrcv_dev->hdev;
> - struct usb_interface *intf = to_usb_interface(djrcv_hdev->dev.parent);
> - struct usb_device *usbdev = interface_to_usbdev(intf);
> - struct hid_device *dj_hiddev;
> struct dj_device *dj_dev;
>
> - /* Device index goes from 1 to 6, we need 3 bytes to store the
> - * semicolon, the index, and a null terminator
> - */
> - unsigned char tmpstr[3];
> -
> if (dj_report->report_params[DEVICE_PAIRED_PARAM_SPFUNCTION] &
> SPFUNCTION_DEVICE_LIST_EMPTY) {
> dbg_hid("%s: device list is empty\n", __func__);
> djrcv_dev->querying_devices = false;
> - return;
> + return -EINVAL;
> }
>
> if ((dj_report->device_index < DJ_DEVICE_INDEX_MIN) ||
> (dj_report->device_index > DJ_DEVICE_INDEX_MAX)) {
> dev_err(&djrcv_hdev->dev, "%s: invalid device index:%d\n",
> __func__, dj_report->device_index);
> - return;
> + return -EINVAL;
> }
>
> if (djrcv_dev->paired_dj_devices[dj_report->device_index]) {
> /* The device is already known. No need to reallocate it. */
> dbg_hid("%s: device is already known\n", __func__);
> - return;
> + return -EEXIST;
> + }
> +
> + dj_dev = kzalloc(sizeof(struct dj_device), GFP_KERNEL);
> +
> + if (!dj_dev) {
> + dev_err(&djrcv_hdev->dev, "%s: failed allocating dj_device\n",
> + __func__);
> + return -ENOMEM;
> }
>
> + dj_dev->reports_supported = get_unaligned_le32(
> + dj_report->report_params + DEVICE_PAIRED_RF_REPORT_TYPE);
> + dj_dev->dj_receiver_dev = djrcv_dev;
> + dj_dev->device_index = dj_report->device_index;
> + dj_dev->enabled = false;
> +
> + dj_dev->wpid =
> + dj_report->report_params[DEVICE_PAIRED_PARAM_EQUAD_ID_MSB]<<8|
seems like some spaces are missing around '<<'
> + dj_report->report_params[DEVICE_PAIRED_PARAM_EQUAD_ID_LSB];
> +
> + djrcv_dev->paired_dj_devices[dj_report->device_index] = dj_dev;
> +
> + return 0;
> +
> +}
> +
> +/*
> + * this function finalize the device
> + */
> +static int logi_dj_recv_add_djhid_device(struct dj_device *dj_device)
If the function finalizes the device, why is it called 'add_djhid_device'?
> +{
> + struct hid_device *dj_hiddev;
> + struct dj_receiver_dev *djrcv_dev;
> +
> + struct hid_device *djrcv_hdev;
> + struct usb_interface *intf;
> + struct usb_device *usbdev;
> +
> + /* Device index goes from 1 to 6, we need 3 bytes to store the
> + * semicolon, the index, and a null terminator
> + */
> + unsigned char tmpstr[3];
> +
> + BUG_ON(!dj_device);
> + djrcv_dev = dj_device->dj_receiver_dev;
> + djrcv_hdev = djrcv_dev->hdev;
> +
> + /* Called in delayed work context */
> + intf = to_usb_interface(djrcv_hdev->dev.parent);
> + usbdev = interface_to_usbdev(intf);
> +
> dj_hiddev = hid_allocate_device();
> if (IS_ERR(dj_hiddev)) {
> dev_err(&djrcv_hdev->dev, "%s: hid_allocate_device failed\n",
> __func__);
> - return;
> + return -ENOMEM;
> }
>
> dj_hiddev->ll_driver = &logi_dj_ll_driver;
> -
I'd prefer keep this white line to separate the transport description
from the rest
> + dj_device->hdev = dj_hiddev;
> + dj_hiddev->driver_data = dj_device;
> dj_hiddev->dev.parent = &djrcv_hdev->dev;
> dj_hiddev->bus = BUS_USB;
> dj_hiddev->vendor = le16_to_cpu(usbdev->descriptor.idVendor);
> dj_hiddev->product = le16_to_cpu(usbdev->descriptor.idProduct);
> snprintf(dj_hiddev->name, sizeof(dj_hiddev->name),
> - "Logitech Unifying Device. Wireless PID:%02x%02x",
> - dj_report->report_params[DEVICE_PAIRED_PARAM_EQUAD_ID_MSB],
> - dj_report->report_params[DEVICE_PAIRED_PARAM_EQUAD_ID_LSB]);
> + "Logitech Unifying Device. Wireless PID:%04x",
> + dj_device->wpid);
Why bothering setting a complex name if you are changing it after?
>
> usb_make_path(usbdev, dj_hiddev->phys, sizeof(dj_hiddev->phys));
> - snprintf(tmpstr, sizeof(tmpstr), ":%d", dj_report->device_index);
> + snprintf(tmpstr, sizeof(tmpstr), ":%d", dj_device->device_index);
> strlcat(dj_hiddev->phys, tmpstr, sizeof(dj_hiddev->phys));
>
> - dj_dev = kzalloc(sizeof(struct dj_device), GFP_KERNEL);
> -
> - if (!dj_dev) {
> - dev_err(&djrcv_hdev->dev, "%s: failed allocating dj_device\n",
> + if (hid_add_device(dj_hiddev)) {
> + dev_err(&djrcv_dev->hdev->dev,
> + "%s: failed adding dj_device\n",
> __func__);
> - goto dj_device_allocate_fail;
> + djrcv_dev->paired_dj_devices[dj_device->device_index] = NULL;
> + kfree(dj_device);
> + hid_destroy_device(dj_hiddev);
> + return -ENODEV;
> }
>
> - dj_dev->reports_supported = get_unaligned_le32(
> - dj_report->report_params + DEVICE_PAIRED_RF_REPORT_TYPE);
> - dj_dev->hdev = dj_hiddev;
> - dj_dev->dj_receiver_dev = djrcv_dev;
> - dj_dev->device_index = dj_report->device_index;
> - dj_hiddev->driver_data = dj_dev;
> + hid_info(djrcv_dev->hdev, "Device %d: '%s' added\n",
> + dj_device->device_index, dj_device->device_name);
>
> - djrcv_dev->paired_dj_devices[dj_report->device_index] = dj_dev;
> + return 0;
> +}
>
> - if (hid_add_device(dj_hiddev)) {
> - dev_err(&djrcv_hdev->dev, "%s: failed adding dj_device\n",
> - __func__);
> - goto hid_add_device_fail;
> +static void logi_set_device_name(struct dj_device *dj_device,
> + struct dj_report *dj_report)
> +{
> +
> + struct dj_device_method *m;
> + struct dj_receiver_dev *djrcv_dev = dj_device->dj_receiver_dev;
> +
> + strlcpy(dj_device->device_name, dj_report->report_params+3,
> + sizeof(dj_device->device_name));
This just feel wrong. I remember problems with some long device names
not null terminated.
> +
> + hid_info(djrcv_dev->hdev, "Device %d: '%s'\n",
> + dj_device->device_index,
> + dj_device->device_name);
> +
> + for (m = dj_device_method ; m->device_names ; m++) {
> + char **name;
> +
> + for (name = m->device_names ; name ; name++) {
> + if (!strcmp(*name, dj_device->device_name)) {
> + dj_device->methods = m;
> + hid_info(djrcv_dev->hdev,
> + "Found methods for device '%s'\n",
> + dj_device->device_name);
> + return;
> + }
> + }
> }
This whole for loop has nothing to do with the set_device_name
advertised by the name of the function. Please put this into its own.
>
> - return;
> +}
> +
> +/*
> + * finalize the dj_device creation, because now we know the driver name
> + */
> +static inline cvoid __dj_device_init_2(struct dj_receiver_dev *djrcv_dev,
> + struct dj_report *dj_report)
Ouch. What is this name? And why cvoid?
Please keep it simple and with the current driver naming style.
> +{
> + int dev_index = dj_report->report_params[1]-0x40+1;
Instead of ' - 0x40' I think a mask would be better.
> + struct dj_device *dj_device = logi_djrcv2djdev(djrcv_dev, dev_index);
> + int retval;
>
> -hid_add_device_fail:
> - djrcv_dev->paired_dj_devices[dj_report->device_index] = NULL;
> - kfree(dj_dev);
> -dj_device_allocate_fail:
> - hid_destroy_device(dj_hiddev);
> + if (!dj_device) {
> + dbg_hid("%s: cannot find %d device\n",
> + __func__, dev_index);
> + /* do rescan */
> + retval = logi_dj_recv_query_paired_devices(djrcv_dev);
> + if (!retval)
> + dev_err(&djrcv_dev->hdev->dev,
> + "%s:logi_dj_recv_query_paired_devices error:%d\n",
> + __func__, retval);
> +
With the cleanup patch soon-to-be-included-by-Jiri, I think this would
not be needed anymore.
> + return;
> + }
> +
> + if (dj_device->enabled) {
> + dbg_hid("%s: device %d already initializated\n",
> + __func__, dev_index);
> + return;
> + }
> +
> + logi_set_device_name(dj_device, dj_report);
> +
> + if (logi_dj_recv_add_djhid_device(dj_device) < 0) {
> + logi_dj_recv_destroy_djhid_device(djrcv_dev, dj_report);
> + dev_err(&djrcv_dev->hdev->dev,
> + "Cannot create the hid device\n");
> + return;
> + }
> + if (call_init_device(dj_device)) {
> + logi_dj_recv_destroy_djhid_device(djrcv_dev, dj_report);
> + dev_err(&djrcv_dev->hdev->dev,
> + "Cannot init the hid device\n");
> + return;
> + }
> +
> + barrier();
> + dj_device->enabled = true;
> +}
> +
> +/*
> + * Initialize the dj_device structure, but a) doesn't enable it, and b)
> + * doesn't crete the hid device, because we don't now the driver.
> + */
> +static inline void __dj_device_init_1(struct dj_receiver_dev *djrcv_dev,
> + struct dj_report *dj_report)
ditto, the name is awful.
> +{
> + int dev_index;
> + struct dj_device *dj_device;
> + int ret;
> +
> + dev_index = dj_report->device_index;
> + dj_device = logi_djrcv2djdev(djrcv_dev, dev_index);
> + if (dj_device) {
> + dev_warn(&djrcv_dev->hdev->dev,
> + "This device (%d) already exist\n", dev_index);
> + return;
> + }
> +
> + ret = logi_dj_recv_create_dj_device(djrcv_dev, dj_report);
> + if (ret == -EINVAL) {
> + /* This report is not valid, ignore it */
> + return;
> + }
> + if (ret < 0) {
> + dev_err(&djrcv_dev->hdev->dev,
> + "Cannot allocate the hid device %d.%d\n",
> + dj_report->device_index, dj_report->report_type);
> + return;
> + }
> + /* we have to re-taken it, because before was not created */
> + dj_device = logi_djrcv2djdev(djrcv_dev, dev_index);
> + if (logi_send_name_request(dj_device) < 0) {
> + logi_dj_recv_destroy_djhid_device(djrcv_dev, dj_report);
> + dev_err(&djrcv_dev->hdev->dev,
> + "Cannot query the device name\n");
> + }
> }
>
> static void delayedwork_callback(struct work_struct *work)
> @@ -314,6 +539,8 @@ static void delayedwork_callback(struct work_struct *work)
> unsigned long flags;
> int count;
> int retval;
> + int dev_index;
> + struct dj_device *dj_device;
>
> dbg_hid("%s\n", __func__);
>
> @@ -339,9 +566,14 @@ static void delayedwork_callback(struct work_struct *work)
> spin_unlock_irqrestore(&djrcv_dev->lock, flags);
>
> switch (dj_report.report_type) {
> + case REPORT_GET_LONG_REGISTER_REQ:
> + __dj_device_init_2(djrcv_dev, &dj_report);
> + break;
> +
> case REPORT_TYPE_NOTIF_DEVICE_PAIRED:
> - logi_dj_recv_add_djhid_device(djrcv_dev, &dj_report);
> + __dj_device_init_1(djrcv_dev, &dj_report);
> break;
> +
> case REPORT_TYPE_NOTIF_DEVICE_UNPAIRED:
> logi_dj_recv_destroy_djhid_device(djrcv_dev, &dj_report);
> break;
> @@ -353,20 +585,23 @@ static void delayedwork_callback(struct work_struct *work)
> * to this dj_device never arrived to this driver. The reason is that
> * hid-core discards all packets coming from a device while probe() is
> * executing. */
> - if (!djrcv_dev->paired_dj_devices[dj_report.device_index]) {
> - /* ok, we don't know the device, just re-ask the
> - * receiver for the list of connected devices. */
> - retval = logi_dj_recv_query_paired_devices(djrcv_dev);
> - if (!retval) {
> - /* everything went fine, so just leave */
> - break;
> - }
> - dev_err(&djrcv_dev->hdev->dev,
> - "%s:logi_dj_recv_query_paired_devices "
> - "error:%d\n", __func__, retval);
> - }
> - dbg_hid("%s: unexpected report type\n", __func__);
> + dev_index = dj_report.device_index;
> + dj_device = logi_djrcv2djdev(djrcv_dev, dev_index);
> + if (!dj_device) {
> + /* ok, we don't know the device, just re-ask the
> + * receiver for the list of connected devices. */
> + retval = logi_dj_recv_query_paired_devices(djrcv_dev);
> + if (!retval) {
> + /* everything went fine, so just leave */
> + break;
> + }
> + dev_err(&djrcv_dev->hdev->dev,
> + "%s:logi_dj_recv_query_paired_devices error:%d\n",
> + __func__, retval);
> + }
> + dbg_hid("%s: unexpected report type\n", __func__);
It looks like this whole hunk fixes just an indent problem and should
not be in a functional patch.
> }
> +
> }
>
> static void logi_dj_recv_queue_notification(struct dj_receiver_dev *djrcv_dev,
> @@ -390,7 +625,7 @@ static void logi_dj_recv_forward_null_report(struct dj_receiver_dev *djrcv_dev,
> u8 reportbuffer[MAX_REPORT_SIZE];
> struct dj_device *djdev;
>
> - djdev = djrcv_dev->paired_dj_devices[dj_report->device_index];
> + djdev = logi_djrcv2djdev(djrcv_dev, dj_report->device_index);
>
> if (!djdev) {
> dbg_hid("djrcv_dev->paired_dj_devices[dj_report->device_index]"
> @@ -404,6 +639,10 @@ static void logi_dj_recv_forward_null_report(struct dj_receiver_dev *djrcv_dev,
> return;
> }
>
> + /* device not enabled */
> + if (!djdev->enabled)
> + return;
> +
> memset(reportbuffer, 0, sizeof(reportbuffer));
>
> for (i = 0; i < NUMBER_OF_HID_REPORTS; i++) {
> @@ -440,6 +679,10 @@ static void logi_dj_recv_forward_report(struct dj_receiver_dev *djrcv_dev,
> return;
> }
>
> + /* device not enabled */
> + if (!dj_device->enabled)
> + return;
> +
> if ((dj_report->report_type > ARRAY_SIZE(hid_reportid_size_map) - 1) ||
> (hid_reportid_size_map[dj_report->report_type] == 0)) {
> dbg_hid("invalid report type:%x\n", dj_report->report_type);
> @@ -451,8 +694,8 @@ static void logi_dj_recv_forward_report(struct dj_receiver_dev *djrcv_dev,
> hid_reportid_size_map[dj_report->report_type], 1)) {
> dbg_hid("hid_input_report error\n");
> }
> -}
>
> +}
Hmm... whitespace collateral damage?
>
> static int logi_dj_recv_send_report(struct dj_receiver_dev *djrcv_dev,
> struct dj_report *dj_report)
> @@ -584,6 +827,10 @@ static int logi_dj_ll_parse(struct hid_device *hid)
> char *rdesc;
> int retval;
>
> + /* try the specific hook */
> + if (djdev->methods && djdev->methods->parse)
> + return djdev->methods->parse(djdev);
> +
I think send_report can be called anytime when someone try to send
information to the device. Putting an init function here is doomed to
be bugged.
> dbg_hid("%s\n", __func__);
>
> djdev->hdev->version = 0x0111;
> @@ -655,7 +902,6 @@ static struct hid_ll_driver logi_dj_ll_driver = {
> .raw_request = logi_dj_ll_raw_request,
> };
>
> -
Please refrain to make such changes in a big patch.
> static int logi_dj_raw_event(struct hid_device *hdev,
> struct hid_report *report, u8 *data,
> int size)
> @@ -692,25 +938,51 @@ static int logi_dj_raw_event(struct hid_device *hdev,
> */
>
> spin_lock_irqsave(&djrcv_dev->lock, flags);
> - if (dj_report->report_id == REPORT_ID_DJ_SHORT) {
> - switch (dj_report->report_type) {
> - case REPORT_TYPE_NOTIF_DEVICE_PAIRED:
> - case REPORT_TYPE_NOTIF_DEVICE_UNPAIRED:
> - logi_dj_recv_queue_notification(djrcv_dev, dj_report);
> - break;
> - case REPORT_TYPE_NOTIF_CONNECTION_STATUS:
> - if (dj_report->report_params[CONNECTION_STATUS_PARAM_STATUS] ==
> - STATUS_LINKLOSS) {
> - logi_dj_recv_forward_null_report(djrcv_dev, dj_report);
> - }
> - break;
> - default:
> - logi_dj_recv_forward_report(djrcv_dev, dj_report);
> +
> + if (dj_report->report_id == REPORT_ID_RECV_LONG &&
> + dj_report->device_index == REPORT_RECEIVER_INDEX &&
> + dj_report->report_type == REPORT_GET_LONG_REGISTER_REQ &&
> + dj_report->report_params[0] == REPORT_REG_PAIRING_INFORMATION &&
> + dj_report->report_params[1] >= REPORT_REG2_NOTIFY_DEVICE_NAME &&
> + dj_report->report_params[1] < (REPORT_REG2_NOTIFY_DEVICE_NAME+
> + DJ_DEVICE_INDEX_COUNT)) {
Ouch, very much ouch. There might be a better way to test that.
> +
> + logi_dj_recv_queue_notification(djrcv_dev, dj_report);
> + report_processed = true;
> + } else if (dj_report->report_id == REPORT_ID_DJ_SHORT &&
> + (dj_report->report_type ==
> + REPORT_TYPE_NOTIF_DEVICE_PAIRED ||
> + dj_report->report_type ==
> + REPORT_TYPE_NOTIF_DEVICE_UNPAIRED)) {
> +
> + logi_dj_recv_queue_notification(djrcv_dev, dj_report);
> + report_processed = true;
> + } else if (dj_report->report_id == REPORT_ID_DJ_SHORT &&
> + dj_report->report_type ==
> + REPORT_TYPE_NOTIF_CONNECTION_STATUS) {
> +
> + if (dj_report->report_params[CONNECTION_STATUS_PARAM_STATUS] ==
> + STATUS_LINKLOSS) {
> +
> + logi_dj_recv_forward_null_report(djrcv_dev, dj_report);
> }
> report_processed = true;
> + } else {
I guess a switch/case on the report_id + some extra functions would
greatly help understanding the code.
> + bool skip = false;
> + struct dj_device *djdev;
> +
> + djdev = logi_djrcv2djdev(djrcv_dev, dj_report->device_index);
> + if (djdev && djdev->enabled) {
> + skip = call_parse_raw_event(djdev, dj_report);
> + report_processed = skip;
> + }
> + if (!skip && dj_report->report_id == REPORT_ID_DJ_SHORT) {
> + logi_dj_recv_forward_report(djrcv_dev, dj_report);
> + report_processed = true;
> + }
> }
> - spin_unlock_irqrestore(&djrcv_dev->lock, flags);
>
> + spin_unlock_irqrestore(&djrcv_dev->lock, flags);
Seems like the withespace hunter stroke again.
> return report_processed;
> }
>
> @@ -865,12 +1137,12 @@ static void logi_dj_remove(struct hid_device *hdev)
> for (i = 0; i < (DJ_MAX_PAIRED_DEVICES + DJ_DEVICE_INDEX_MIN); i++) {
> dj_dev = djrcv_dev->paired_dj_devices[i];
> if (dj_dev != NULL) {
> - hid_destroy_device(dj_dev->hdev);
> + if (dj_dev->hdev)
> + hid_destroy_device(dj_dev->hdev);
> kfree(dj_dev);
> djrcv_dev->paired_dj_devices[i] = NULL;
> }
> }
> -
I am not saying anything.... :-)
> kfifo_free(&djrcv_dev->notif_fifo);
> kfree(djrcv_dev);
> hid_set_drvdata(hdev, NULL);
> diff --git a/drivers/hid/hid-logitech-dj.h b/drivers/hid/hid-logitech-dj.h
> index 4a40003..d81ed7d 100644
> --- a/drivers/hid/hid-logitech-dj.h
> +++ b/drivers/hid/hid-logitech-dj.h
> @@ -29,12 +29,17 @@
> #define DJ_MAX_NUMBER_NOTIFICATIONS 8
> #define DJ_DEVICE_INDEX_MIN 1
> #define DJ_DEVICE_INDEX_MAX 6
> +#define DJ_DEVICE_INDEX_COUNT (DJ_DEVICE_INDEX_MAX - \
> + DJ_DEVICE_INDEX_MIN+1)
>
> #define DJREPORT_SHORT_LENGTH 15
> #define DJREPORT_LONG_LENGTH 32
> +#define REPORT_ID_RECV_SHORT_LENGTH 7
>
> #define REPORT_ID_DJ_SHORT 0x20
> #define REPORT_ID_DJ_LONG 0x21
> +#define REPORT_ID_RECV_SHORT 0x10
> +#define REPORT_ID_RECV_LONG 0x11
>
> #define REPORT_TYPE_RFREPORT_FIRST 0x01
> #define REPORT_TYPE_RFREPORT_LAST 0x1F
> @@ -60,6 +65,10 @@
> /* Device Un-Paired Notification */
> #define REPORT_TYPE_NOTIF_DEVICE_UNPAIRED 0x40
>
> +/* Device "Unifying Device name" Notification */
> +#define REPORT_GET_LONG_REGISTER_REQ 0x83
> +#define REPORT_REG_PAIRING_INFORMATION 0xb5
> +#define REPORT_REG2_NOTIFY_DEVICE_NAME 0x40
>
> /* Connection Status Notification */
> #define REPORT_TYPE_NOTIF_CONNECTION_STATUS 0x42
> @@ -79,6 +88,9 @@
> #define REPORT_TYPE_MEDIA_CENTER 0x08
> #define REPORT_TYPE_LEDS 0x0E
>
> +/* receiver device index */
> +#define REPORT_RECEIVER_INDEX 0xff
> +
> /* RF Report types bitfield */
> #define STD_KEYBOARD 0x00000002
> #define STD_MOUSE 0x00000004
> @@ -104,11 +116,40 @@ struct dj_receiver_dev {
> bool querying_devices;
> };
>
> +struct dj_device;
> +
> +/**
> + * these methods are invoked instead the default ones
> + * @devices_name array of device name supported by this driver
> + * @init_device init method for the device. If the returned
> + * value is 0, the device was initiated correctly.
> + * Otherwise the device is not created.
> + * @parse_raw_event this method is called when a raw event arrived
> + * it permits to change the event.
> + * If the value returned is not zero the standard
> + * handler is called after.
> + * @parse this method allow to generate and pass a
> + * device descriptor to the higher level.
> + * @destroy called when the device has to be deleted
> + */
> +struct dj_device_method {
> + char **device_names;
> + int (*init_device)(struct dj_device *);
> + int (*parse_raw_event)(struct dj_device *, struct dj_report *);
> + int (*parse)(struct dj_device *);
> + void (*destroy)(struct dj_device *);
> +};
> +
> struct dj_device {
> struct hid_device *hdev;
> struct dj_receiver_dev *dj_receiver_dev;
> u32 reports_supported;
> u8 device_index;
> + int enabled;
> + char device_name[16];
> + u16 wpid;
> + struct dj_device_method *methods;
> + void *userdata;
> };
>
> /**
> --
> 1.9.3
>
OK, done. So, if possible, I would like you to work first on the
various cleanings and the proper naming element in your patch.
I would like to see this cleaned up before we start investigating the
new 'driver' option. You can of course take some ideas from the github
Nestor pointed, or just keep modifying your own code.
Thanks for the patch submission, and keep pushing!
Cheers,
Benjamin
^ permalink raw reply
* Re: [PATCH 1/1] Input: elantech: Correct the sign of the x/y movement for trackpoint
From: Dmitry Torokhov @ 2014-08-26 22:45 UTC (permalink / raw)
To: Ulrik De Bie; +Cc: linux-input, Hans de Goede, David Herrmann
In-Reply-To: <1409081423-2259-1-git-send-email-ulrik.debie-os@e2big.org>
Hi Ulrik,
On Tue, Aug 26, 2014 at 09:30:23PM +0200, Ulrik De Bie wrote:
> Hi Dmitry,
>
> Thank you for your feedback !
>
> The x/y movement calculation was wrong, probably caused by my wrong
> documentation at the beginning of the elantech_report_trackpoint
> function. When applying this patch to your patch, it works.
Thank you for testing and providing both the original patch and the
fixup. I folded everything together and applied, I'll try to get it in
3.17.
Thanks!
--
Dmitry
^ permalink raw reply
* Re: [PATCH v2] doc: dt/bindings: input: Fix drv260x binding document
From: Dmitry Torokhov @ 2014-08-26 22:45 UTC (permalink / raw)
To: Felipe Balbi
Cc: Dan Murphy, devicetree, linux-input, linux-kernel,
linux-arm-kernel
In-Reply-To: <20140822171641.GE27616@saruman.home>
On Fri, Aug 22, 2014 at 12:16:41PM -0500, Felipe Balbi wrote:
> On Fri, Aug 22, 2014 at 12:12:19PM -0500, Dan Murphy wrote:
> > Update the drv260x dt binding document:
> > - Change the node name to the devices function not the
> > device name.
> > - Add vbat-supply to the example.
> > - Fix indentation of the example.
> >
> > Signed-off-by: Dan Murphy <dmurphy@ti.com>
>
> Reviewed-by: Felipe Balbi <balbi@ti.com>
Applied, thank you.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] input: misc: drv260x remove defines not used
From: Dmitry Torokhov @ 2014-08-26 22:46 UTC (permalink / raw)
To: Dan Murphy; +Cc: linux-input, linux-kernel, linux-arm-kernel
In-Reply-To: <1408716675-1093-1-git-send-email-dmurphy@ti.com>
On Fri, Aug 22, 2014 at 09:11:15AM -0500, Dan Murphy wrote:
> Removing some #defines that are not and should
> never be used pertaining to I2C.
>
> Removing:
> define DRV260X_ALLOWED_R_BYTES 25
> define DRV260X_ALLOWED_W_BYTES 2
> define DRV260X_MAX_RW_RETRIES 5
> define DRV260X_I2C_RETRY_DELAY 10
>
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
Applied, thank you.
> ---
> drivers/input/misc/drv260x.c | 5 -----
> 1 file changed, 5 deletions(-)
>
> diff --git a/drivers/input/misc/drv260x.c b/drivers/input/misc/drv260x.c
> index e90e3b8..a7a19e6 100644
> --- a/drivers/input/misc/drv260x.c
> +++ b/drivers/input/misc/drv260x.c
> @@ -66,11 +66,6 @@
> #define DRV260X_LRA_RES_PERIOD 0x22
> #define DRV260X_MAX_REG 0x23
>
> -#define DRV260X_ALLOWED_R_BYTES 25
> -#define DRV260X_ALLOWED_W_BYTES 2
> -#define DRV260X_MAX_RW_RETRIES 5
> -#define DRV260X_I2C_RETRY_DELAY 10
> -
> #define DRV260X_GO_BIT 0x01
>
> /* Library Selection */
> --
> 1.7.9.5
>
--
Dmitry
^ permalink raw reply
* Re: [PATCH] input: misc: drv260x: add check for ERM mode and LRA Libraries
From: Dmitry Torokhov @ 2014-08-26 22:46 UTC (permalink / raw)
To: Dan Murphy; +Cc: linux-input, linux-kernel, linux-arm-kernel
In-Reply-To: <1408716696-1126-1-git-send-email-dmurphy@ti.com>
On Fri, Aug 22, 2014 at 09:11:36AM -0500, Dan Murphy wrote:
> Add a check to ensure that LRA libraries are not mixed with
> the ERM mode.
>
> If ERM mode and the Library is empty "OR" the
> LRA library then exit.
>
> As the LRA and empty libraries are not applicable for
> the ERM actuator.
>
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
Applied, thank you.
> ---
> drivers/input/misc/drv260x.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/input/misc/drv260x.c b/drivers/input/misc/drv260x.c
> index a7a19e6..d6a26a7 100644
> --- a/drivers/input/misc/drv260x.c
> +++ b/drivers/input/misc/drv260x.c
> @@ -564,6 +564,14 @@ static int drv260x_probe(struct i2c_client *client,
> return -EINVAL;
> }
>
> + if (haptics->mode == DRV260X_ERM_MODE &&
> + haptics->library == DRV260X_LIB_EMPTY ||
> + haptics->library == DRV260X_LIB_LRA) {
> + dev_err(&client->dev,
> + "ERM Mode with LRA Library mismatch\n");
> + return -EINVAL;
> + }
> +
> haptics->regulator = devm_regulator_get(&client->dev, "vbat");
> if (IS_ERR(haptics->regulator)) {
> error = PTR_ERR(haptics->regulator);
> --
> 1.7.9.5
>
--
Dmitry
^ permalink raw reply
* [BUG] hid-multitouch ignores all event after recent patch in 3.17-rc1 to input_mt_sync_frame()
From: Reyad Attiyat @ 2014-08-27 5:45 UTC (permalink / raw)
To: rydberg, bleung, dmitry.torokhov, Benjamin Tissoires, linux-input
Hello Henrik Rydberg,
I noticed a recent patch to input-mt.c breaks hid-multitouch on my MS Surface:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/input/input-mt.c?id=f8ec894945e7d205ce62be59e55e72c4304e4739
This device uses a special quirk in the hid-multitouch driver to
ignore duplicate input events. This quirk uses the inline function
input_mt_is_used() to determine if an input frame is a duplicate. In
the patch you remove the line, mt->frame++, from the function
input_mt_sync_frame(). This causes the is_used function to return true
as the frame number never increases.
What would be the best way to solve this, is it ok to simply re add that line?
Thank You,
Reyad Attiyat
^ permalink raw reply
* Re: [BUG] hid-multitouch ignores all event after recent patch in 3.17-rc1 to input_mt_sync_frame()
From: Henrik Rydberg @ 2014-08-27 6:20 UTC (permalink / raw)
To: Reyad Attiyat, bleung, dmitry.torokhov, Benjamin Tissoires,
linux-input
In-Reply-To: <CA+BWVURnxeQUJnDDhO=q=67sCK2NGq7_W=8wAN5qDjTKk14yAg@mail.gmail.com>
> What would be the best way to solve this, is it ok to simply re add that line?
Not quite, but put an "else" in front and it is ok. A patch to that effect is
already on its way to Linus, so you will soon be able to cherry-pick it.
Thanks,
Henrik
^ permalink raw reply
* [PATCH 1/2] HID: magicmouse: sanity check report size in raw_event() callback
From: Jiri Kosina @ 2014-08-27 7:12 UTC (permalink / raw)
To: linux-input; +Cc: linux-kernel
The report passed to us from transport driver could potentially be
arbitrarily large, therefore we better sanity-check it so that
magicmouse_emit_touch() gets only valid values of raw_id.
Cc: stable@vger.kernel.org
Reported-by: Steven Vittitoe <scvitti@google.com>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---
drivers/hid/hid-magicmouse.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
index ecc2cbf..29a74c1 100644
--- a/drivers/hid/hid-magicmouse.c
+++ b/drivers/hid/hid-magicmouse.c
@@ -290,6 +290,11 @@ static int magicmouse_raw_event(struct hid_device *hdev,
if (size < 4 || ((size - 4) % 9) != 0)
return 0;
npoints = (size - 4) / 9;
+ if (npoints > 15) {
+ hid_warn(hdev, "invalid size value (%d) for TRACKPAD_REPORT_ID\n",
+ size);
+ return 0;
+ }
msc->ntouches = 0;
for (ii = 0; ii < npoints; ii++)
magicmouse_emit_touch(msc, ii, data + ii * 9 + 4);
@@ -307,6 +312,11 @@ static int magicmouse_raw_event(struct hid_device *hdev,
if (size < 6 || ((size - 6) % 8) != 0)
return 0;
npoints = (size - 6) / 8;
+ if (npoints > 15) {
+ hid_warn(hdev, "invalid size value (%d) for MOUSE_REPORT_ID\n",
+ size);
+ return 0;
+ }
msc->ntouches = 0;
for (ii = 0; ii < npoints; ii++)
magicmouse_emit_touch(msc, ii, data + ii * 8 + 6);
--
1.9.2
^ permalink raw reply related
* [PATCH 2/2] HID: picolcd: sanity check report size in raw_event() callback
From: Jiri Kosina @ 2014-08-27 7:13 UTC (permalink / raw)
To: linux-input; +Cc: linux-kernel, Bruno Prémont
In-Reply-To: <alpine.LNX.2.00.1408270911130.2992@pobox.suse.cz>
The report passed to us from transport driver could potentially be
arbitrarily large, therefore we better sanity-check it so that raw_data
that we hold in picolcd_pending structure are always kept within proper
bounds.
Cc: stable@vger.kernel.org
Reported-by: Steven Vittitoe <scvitti@google.com>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---
drivers/hid/hid-picolcd_core.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/hid/hid-picolcd_core.c b/drivers/hid/hid-picolcd_core.c
index acbb0210..020df3c 100644
--- a/drivers/hid/hid-picolcd_core.c
+++ b/drivers/hid/hid-picolcd_core.c
@@ -350,6 +350,12 @@ static int picolcd_raw_event(struct hid_device *hdev,
if (!data)
return 1;
+ if (size > 64) {
+ hid_warn(hdev, "invalid size value (%d) for picolcd raw event\n",
+ size);
+ return 0;
+ }
+
if (report->id == REPORT_KEY_STATE) {
if (data->input_keys)
ret = picolcd_raw_keypad(data, report, raw_data+1, size-1);
--
1.9.2
^ permalink raw reply related
* Re: [PATCH 2/2] HID: picolcd: sanity check report size in raw_event() callback
From: Bruno Prémont @ 2014-08-27 8:13 UTC (permalink / raw)
To: Jiri Kosina; +Cc: linux-input, linux-kernel
In-Reply-To: <alpine.LNX.2.00.1408270912310.2992@pobox.suse.cz>
On Wed, 27 Aug 2014 09:13:15 +0200 (CEST) Jiri Kosina wrote:
> The report passed to us from transport driver could potentially be
> arbitrarily large, therefore we better sanity-check it so that raw_data
> that we hold in picolcd_pending structure are always kept within proper
> bounds.
>
> Cc: stable@vger.kernel.org
> Reported-by: Steven Vittitoe <scvitti@google.com>
> Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Acked-by: Bruno Prémont <bonbons@linux-vserver.org>
> ---
> drivers/hid/hid-picolcd_core.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/hid/hid-picolcd_core.c b/drivers/hid/hid-picolcd_core.c
> index acbb0210..020df3c 100644
> --- a/drivers/hid/hid-picolcd_core.c
> +++ b/drivers/hid/hid-picolcd_core.c
> @@ -350,6 +350,12 @@ static int picolcd_raw_event(struct hid_device *hdev,
> if (!data)
> return 1;
>
> + if (size > 64) {
> + hid_warn(hdev, "invalid size value (%d) for picolcd raw event\n",
> + size);
Is it worth adding report->id to this hid_warn()?
A valid device is not expected to ever send >64 bytes reports but in
case a firmware update would do so it would help to know for which
report it was.
> + return 0;
> + }
> +
> if (report->id == REPORT_KEY_STATE) {
> if (data->input_keys)
> ret = picolcd_raw_keypad(data, report, raw_data+1, size-1);
^ permalink raw reply
* [PATCH] HID: add support for PenMount HID TouchScreen Driver v2
From: Christian Gmeiner @ 2014-08-27 8:20 UTC (permalink / raw)
To: linux-input; +Cc: benjamin.tissoires, Christian Gmeiner
This patch adds a seperate hid-penmount driver to work
around an issue with the HID report descriptor. The
descriptor does not contain the ContactID usage and as
result the touchscreen is represented as normal mouse
to the system.
This driver maps the button 0 emitted by the touchscreen
to BTN_TOUCH. This makes it possible to use touch events
in userspace.
changes from v1 to v2
- incorporated feedback from Benjamin Tissoires
Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com>
---
drivers/hid/Kconfig | 6 ++++++
drivers/hid/Makefile | 1 +
drivers/hid/hid-penmount.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 58 insertions(+)
create mode 100644 drivers/hid/hid-penmount.c
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index c18d5d7..0351b66 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -530,6 +530,12 @@ config PANTHERLORD_FF
Say Y here if you have a PantherLord/GreenAsia based game controller
or adapter and want to enable force feedback support for it.
+config HID_PENMOUNT
+ tristate "Penmount touch device"
+ depends on USB_HID
+ ---help---
+ Say Y here if you have a Penmount based touch controller.
+
config HID_PETALYNX
tristate "Petalynx Maxter remote control"
depends on HID
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index 4dbac7f..e2850d8 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -71,6 +71,7 @@ obj-$(CONFIG_HID_NTRIG) += hid-ntrig.o
obj-$(CONFIG_HID_ORTEK) += hid-ortek.o
obj-$(CONFIG_HID_PRODIKEYS) += hid-prodikeys.o
obj-$(CONFIG_HID_PANTHERLORD) += hid-pl.o
+obj-$(CONFIG_HID_PENMOUNT) += hid-penmount.o
obj-$(CONFIG_HID_PETALYNX) += hid-petalynx.o
obj-$(CONFIG_HID_PICOLCD) += hid-picolcd.o
hid-picolcd-y += hid-picolcd_core.o
diff --git a/drivers/hid/hid-penmount.c b/drivers/hid/hid-penmount.c
new file mode 100644
index 0000000..02de4b8
--- /dev/null
+++ b/drivers/hid/hid-penmount.c
@@ -0,0 +1,51 @@
+/*
+ * HID driver for PenMount touchscreens
+ *
+ * Copyright (c) 2014 Christian Gmeiner <christian.gmeiner@gmail.com>
+ *
+ * based on hid-penmount copyrighted by
+ * PenMount Touch Solutions <penmount@seed.net.tw>
+ */
+
+/*
+ * 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/module.h>
+#include <linux/hid.h>
+#include "hid-ids.h"
+
+static int penmount_input_mapping(struct hid_device *hdev,
+ struct hid_input *hi, struct hid_field *field,
+ struct hid_usage *usage, unsigned long **bit, int *max)
+{
+ int mapped = 0;
+
+ if ((usage->hid & HID_USAGE_PAGE) == HID_UP_BUTTON) {
+ hid_map_usage(hi, usage, bit, max, EV_KEY, BTN_TOUCH);
+ mapped = 1;
+ }
+
+ return mapped;
+}
+
+static const struct hid_device_id penmount_devices[] = {
+ { HID_USB_DEVICE(USB_VENDOR_ID_PENMOUNT, USB_DEVICE_ID_PENMOUNT_6000) },
+ { }
+};
+MODULE_DEVICE_TABLE(hid, penmount_devices);
+
+static struct hid_driver penmount_driver = {
+ .name = "hid-penmount",
+ .id_table = penmount_devices,
+ .input_mapping = penmount_input_mapping,
+};
+
+module_hid_driver(penmount_driver);
+
+MODULE_AUTHOR("Christian Gmeiner <christian.gmeiner@gmail.com>");
+MODULE_DESCRIPTION("PenMount HID TouchScreen driver");
+MODULE_LICENSE("GPL");
--
1.9.3
^ permalink raw reply related
* Re: [PATCH] HID: add support for PenMount HID TouchScreen Driver
From: Christian Gmeiner @ 2014-08-27 8:23 UTC (permalink / raw)
To: Benjamin Tissoires; +Cc: linux-input
In-Reply-To: <CAN+gG=E=k7JCeh=V4O6nzkzoogRj=q+C8Np_h5SHNhLympH1GA@mail.gmail.com>
Hi Benjamin.
2014-08-26 20:47 GMT+02:00 Benjamin Tissoires <benjamin.tissoires@gmail.com>:
> On Tue, Aug 26, 2014 at 3:29 AM, Christian Gmeiner
> <christian.gmeiner@gmail.com> wrote:
>> Hi Benjamin.
>>
>> 2014-08-25 17:43 GMT+02:00 Benjamin Tissoires <benjamin.tissoires@gmail.com>:
>>> On Mon, Aug 25, 2014 at 10:48 AM, Christian Gmeiner
>>> <christian.gmeiner@gmail.com> wrote:
>>>> This driver is a cleaned up version of
>>>> http://git.android-x86.org/?p=kernel/cdv.git;a=blob_plain;f=drivers/hid/hid-penmount.c;hb=HEAD
>>>
>>> This is definitively weird. According to your driver, it should go by
>>> default through hid-multitouch and behave correctly (at least the
>>> multi-touch versions, and I would say yours too).
>>>
>>
>> I thought I have tried it with hid-multitouch, but I it never gets used.
>>
>> [ 94.342542] usb 2-1.7: USB disconnect, device number 6
>> [ 96.312636] usb 2-1.7: new full-speed USB device number 7 using ehci-pci
>> [ 96.399606] usb 2-1.7: New USB device found, idVendor=14e1, idProduct=6000
>> [ 96.399611] usb 2-1.7: New USB device strings: Mfr=1, Product=2,
>> SerialNumber=0
>> [ 96.399614] usb 2-1.7: Product: PenMount USB
>> [ 96.399617] usb 2-1.7: Manufacturer: DIALOGUE INC
>> [ 96.402777] input: DIALOGUE INC PenMount USB as
>> /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.7/2-1.7:1.0/0003:14E1:6000.0004/input/input12
>> [ 96.403533] hid-generic 0003:14E1:6000.0004: input,hidraw2: USB HID
>> v1.01 Mouse [DIALOGUE INC PenMount USB] on usb-0000:00:1d.0-1.7/input0
>
> Yep, it will never go through hid-multitouch because the report
> descriptor does not contain the ContactID usage. So hid-generic is
> fine for now.
>
ok
>>
>>
>>> Can you send me some touch recordings of your device with
>>> hid-recorder[1] please? No need to remove your driver, it will capture
>>> raw input events, so you should be just fine.
>>
>> [christian@chgm-pc hid]$ sudo hid-recorder
>> Available devices:
>> /dev/hidraw0: Microsoft Microsoft 3-Button Mouse with IntelliEye(TM)
>> /dev/hidraw1: LITE-ON Technology USB NetVista Full Width Keyboard.
>> /dev/hidraw2: DIALOGUE INC PenMount USB
>> Select the device event number [0-2]: 2
>> R: 76 05 01 09 02 a1 01 09 01 a1 00 09 00 15 00 25 01 95 06 75 01 81
>> 01 05 09 19 01 29 03 15 00 25 01 95 02 75 01 81 02 05 01 09 30 09 31
>> 15 00 26 ff 03 36 80 00 46 ff 7f 75 10 95 02 81 02 09 00 15 00 26 ff
>> 00 75 08 95 05 b1 02 c0 c0
>> N: DIALOGUE INC PenMount USB
>> P: usb-0000:00:1d.0-1.7/input0
>> I: 3 14e1 6000
>> E: 0.000000 5 30 d0 00 d1 01
>> E: 0.000945 5 70 d0 00 d1 01
>> E: 0.006984 5 70 d0 00 d1 01
>> E: 0.012961 5 70 d0 00 d1 01
>> E: 0.019001 5 70 d0 00 d1 01
>> E: 0.025965 5 70 d0 00 d1 01
>> E: 0.031960 5 70 d0 00 d1 01
>> E: 0.037964 5 70 d0 00 d1 01
>> E: 0.043964 5 70 d0 00 d1 01
>> E: 0.050988 5 70 d0 00 d1 01
>> E: 0.056945 5 70 d0 00 d1 01
>> E: 0.062985 5 70 d0 00 d1 01
>> E: 0.068965 5 70 d0 00 d1 01
>> E: 0.075974 5 70 d0 00 d1 01
>> E: 0.081961 5 70 d0 00 d1 01
>> E: 0.112985 5 70 d0 00 d1 01
>> E: 0.118978 5 70 d0 00 d0 01
>> E: 0.124978 5 70 d1 00 d0 01
>> E: 0.131973 5 70 d3 00 d0 01
>> E: 0.137971 5 70 d4 00 d0 01
>> E: 0.143919 5 70 d5 00 cf 01
>> E: 0.149938 5 70 d7 00 cf 01
>> E: 0.155971 5 70 d9 00 cf 01
>> E: 0.163011 5 70 db 00 ce 01
>> E: 0.168984 5 70 dc 00 ce 01
>> E: 0.174996 5 70 de 00 cd 01
>> E: 0.180957 5 70 e0 00 cd 01
>> E: 0.187985 5 70 e1 00 cc 01
>> E: 0.193991 5 70 e3 00 cc 01
>> E: 0.199984 5 70 e5 00 cb 01
>> E: 0.206006 5 70 e7 00 cb 01
>> E: 0.212986 5 70 e9 00 ca 01
>> E: 0.218988 5 70 eb 00 ca 01
>> E: 0.224955 5 70 ed 00 c9 01
>> E: 0.230943 5 70 f0 00 c9 01
>> E: 0.237986 5 70 f2 00 c8 01
>> E: 0.243951 5 70 f5 00 c7 01
>> E: 0.262959 5 70 f8 00 c6 01
>> E: 0.269008 5 70 fc 00 c5 01
>> E: 0.274963 5 70 ff 00 c3 01
>> E: 0.280913 5 70 03 01 c1 01
>> E: 0.287962 5 70 07 01 c0 01
>> E: 0.293989 5 70 0a 01 bf 01
>> E: 0.299971 5 70 0e 01 be 01
>> E: 0.305973 5 70 12 01 bd 01
>> E: 0.313004 5 70 15 01 bc 01
>> E: 0.318966 5 70 18 01 bb 01
>> E: 0.324979 5 70 1b 01 ba 01
>> E: 0.330975 5 70 1f 01 b9 01
>> E: 0.337957 5 70 22 01 b8 01
>> E: 0.343986 5 70 25 01 b7 01
>> E: 0.349924 5 70 28 01 b5 01
>> E: 0.355963 5 70 2c 01 b4 01
>> E: 0.362928 5 70 2f 01 b3 01
>> E: 0.368975 5 70 33 01 b1 01
>> E: 0.374911 5 70 36 01 b0 01
>> E: 0.380961 5 70 39 01 af 01
>> E: 0.387969 5 70 3d 01 af 01
>> E: 0.393916 5 70 40 01 ae 01
>> E: 0.399961 5 70 43 01 ad 01
>> E: 0.405957 5 70 47 01 ad 01
>> E: 0.412947 5 70 4a 01 ac 01
>> E: 0.418982 5 70 4d 01 ab 01
>> E: 0.424983 5 70 50 01 aa 01
>> E: 0.430989 5 70 54 01 a9 01
>> E: 0.437946 5 70 57 01 a8 01
>> E: 0.443999 5 70 5a 01 a7 01
>> E: 0.449980 5 70 5d 01 a6 01
>> E: 0.455972 5 70 61 01 a5 01
>> E: 0.462986 5 70 64 01 a4 01
>> E: 0.468994 5 70 67 01 a3 01
>> E: 0.474981 5 70 6a 01 a3 01
>> E: 0.480990 5 70 6c 01 a2 01
>> E: 0.487974 5 70 6f 01 a1 01
>> E: 0.493988 5 70 72 01 a1 01
>> E: 0.500009 5 70 75 01 a0 01
>> E: 0.505973 5 70 77 01 9f 01
>> E: 0.512990 5 70 7a 01 9f 01
>> E: 0.518971 5 70 7d 01 9e 01
>> E: 0.524988 5 70 7f 01 9e 01
>> E: 0.530969 5 70 81 01 9e 01
>> E: 0.537991 5 70 83 01 9d 01
>> E: 0.543979 5 70 86 01 9d 01
>> E: 0.549988 5 70 88 01 9c 01
>> E: 0.555973 5 70 8a 01 9c 01
>> E: 0.562958 5 70 8c 01 9c 01
>> E: 0.568917 5 70 8d 01 9b 01
>> E: 0.574984 5 70 8f 01 9b 01
>> E: 0.580986 5 70 92 01 9b 01
>> E: 0.587984 5 70 94 01 9a 01
>> E: 0.593990 5 70 95 01 9a 01
>> E: 0.599959 5 70 97 01 99 01
>> E: 0.605980 5 70 97 01 99 01
>> E: 0.612983 5 70 98 01 99 01
>> E: 0.618990 5 70 9a 01 98 01
>> E: 0.624972 5 70 9b 01 98 01
>> E: 0.630983 5 70 9c 01 97 01
>> E: 0.638007 5 70 9e 01 97 01
>> E: 0.643953 5 70 9f 01 97 01
>> E: 0.649986 5 70 a1 01 96 01
>> E: 0.655975 5 70 a1 01 96 01
>> E: 0.662980 5 70 a3 01 96 01
>> E: 0.668993 5 70 a4 01 95 01
>>
>
> Weird, this trace is missing the release from the finger (it should
> start with 5 30 ...)
>
here it is:
E: 0.387936 5 70 59 02 cd 01
E: 0.394915 5 70 59 02 cd 01
E: 0.400929 5 70 59 02 cd 01
E: 0.437981 5 30 59 02 cd 01
> As I understand, the only problem which your driver is trying to solve
> is to map the button 0 emitted by your touchscreen to BTN_TOUCH (I
> have the feeling that you already said that, but I can not find it in
> the thread :/ -> found your other thread).
>
Yeah that is what I want. userspace should see a touch event. In the end
the browser will react to this touch events. And this work enabled this
for the penmount 6000.
> You have 2 possibilities:
> 1. fix the report descriptor and rely on the generic implementations
> 2. just handle the button mapping in this penmount driver and leave
> the rest to the generic code.
>
I am not sure what you would prefer but as I have never played with
report descriptors I went with possibility #2.
> The choice 2. might be easier if you never played with report
> descriptors. You will just have to heavily remove stuffs from your
> current version:
> - struct penmount_priv is not required anymore
> - in penmount_input_mapping() just map HID_UP_BUTTON to BTN_TOUCH, and
> return 0 for everything else
> - remove penmount_process() and penmount_event()
> - no need to have a probe or a remove function anymore
> - use the macro module_hid_driver(penmount_driver) instead of having
> your own penmount_init() and penmount_exit()
>
> You might also want to say that you are the author of the module and
> also just say that you based your work on a hid-penmount copyrighted
> by PenMount Touch Solution <...>.
> The code should be very different by now, so there is no need IMO to
> keep the author when you completely have rewritten the code.
>
I have send a v2.
Greets
--
Christian Gmeiner, MSc
https://soundcloud.com/christian-gmeiner
^ permalink raw reply
* Re: [PATCH 2/2] HID: picolcd: sanity check report size in raw_event() callback
From: Jiri Kosina @ 2014-08-27 8:25 UTC (permalink / raw)
To: Bruno Prémont; +Cc: linux-input, linux-kernel
In-Reply-To: <20140827101311.098f3fd1@pluto>
On Wed, 27 Aug 2014, Bruno Prémont wrote:
> > The report passed to us from transport driver could potentially be
> > arbitrarily large, therefore we better sanity-check it so that raw_data
> > that we hold in picolcd_pending structure are always kept within proper
> > bounds.
> >
> > Cc: stable@vger.kernel.org
> > Reported-by: Steven Vittitoe <scvitti@google.com>
> > Signed-off-by: Jiri Kosina <jkosina@suse.cz>
>
> Acked-by: Bruno Prémont <bonbons@linux-vserver.org>
Thanks.
> > ---
> > drivers/hid/hid-picolcd_core.c | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/hid/hid-picolcd_core.c b/drivers/hid/hid-picolcd_core.c
> > index acbb0210..020df3c 100644
> > --- a/drivers/hid/hid-picolcd_core.c
> > +++ b/drivers/hid/hid-picolcd_core.c
> > @@ -350,6 +350,12 @@ static int picolcd_raw_event(struct hid_device *hdev,
> > if (!data)
> > return 1;
> >
> > + if (size > 64) {
> > + hid_warn(hdev, "invalid size value (%d) for picolcd raw event\n",
> > + size);
>
> Is it worth adding report->id to this hid_warn()?
>
> A valid device is not expected to ever send >64 bytes reports but in
> case a firmware update would do so it would help to know for which
> report it was.
It definitely wouldn't hurt. Pull request with the original patch is now
on its way to Linus though, so let's do this as a followup patch on top
once this is merged.
--
Jiri Kosina
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox