* [PATCH] Input:Optimize events_per_packet count calculation
From: anshul.g @ 2014-12-10 11:10 UTC (permalink / raw)
To: dmitry.torokhov, dtor, linux-input; +Cc: aksgarg1989, anshul.g
From: Anshul Garg <anshul.g@samsung.com>
This patch avoids unnecessary operations while estimating events per
packet for an input device when event type is not set.
Signed-off-by: Anshul Garg <anshul.g@samsung.com>
---
drivers/input/input.c | 22 +++++++++++++---------
1 file changed, 13 insertions(+), 9 deletions(-)
diff --git a/drivers/input/input.c b/drivers/input/input.c
index 29ca0bb..8172296 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -1973,18 +1973,22 @@ static unsigned int input_estimate_events_per_packet(struct input_dev *dev)
events = mt_slots + 1; /* count SYN_MT_REPORT and SYN_REPORT */
- for (i = 0; i < ABS_CNT; i++) {
- if (test_bit(i, dev->absbit)) {
- if (input_is_mt_axis(i))
- events += mt_slots;
- else
- events++;
+ if (test_bit(EV_ABS, dev->evit)) {
+ for (i = 0; i < ABS_CNT; i++) {
+ if (test_bit(i, dev->absbit)) {
+ if (input_is_mt_axis(i))
+ events += mt_slots;
+ else
+ events++;
+ }
}
}
- for (i = 0; i < REL_CNT; i++)
- if (test_bit(i, dev->relbit))
- events++;
+ if (test_bit(EV_REL, dev->evit)) {
+ for (i = 0; i < REL_CNT; i++)
+ if (test_bit(i, dev->relbit))
+ events++;
+ }
/* Make room for KEY and MSC events */
events += 7;
--
1.7.9.5
^ permalink raw reply related
* [PATCH] Input:Optimize events_per_packet count calculation
From: anshul.g @ 2014-12-10 11:48 UTC (permalink / raw)
To: dmitry.torokhov, dtor, linux-input; +Cc: aksgarg1989, anshul.g
From: Anshul Garg <anshul.g@samsung.com>
This patch avoids unnecessary operations while estimating events per
packet for an input device when event type is not set.
Signed-off-by: Anshul Garg <anshul.g@samsung.com>
---
drivers/input/input.c | 22 +++++++++++++---------
1 file changed, 13 insertions(+), 9 deletions(-)
diff --git a/drivers/input/input.c b/drivers/input/input.c
index 29ca0bb..8172296 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -1973,18 +1973,22 @@ static unsigned int input_estimate_events_per_packet(struct input_dev *dev)
events = mt_slots + 1; /* count SYN_MT_REPORT and SYN_REPORT */
- for (i = 0; i < ABS_CNT; i++) {
- if (test_bit(i, dev->absbit)) {
- if (input_is_mt_axis(i))
- events += mt_slots;
- else
- events++;
+ if (test_bit(EV_ABS, dev->evbit)) {
+ for (i = 0; i < ABS_CNT; i++) {
+ if (test_bit(i, dev->absbit)) {
+ if (input_is_mt_axis(i))
+ events += mt_slots;
+ else
+ events++;
+ }
}
}
- for (i = 0; i < REL_CNT; i++)
- if (test_bit(i, dev->relbit))
- events++;
+ if (test_bit(EV_REL, dev->evbit)) {
+ for (i = 0; i < REL_CNT; i++)
+ if (test_bit(i, dev->relbit))
+ events++;
+ }
/* Make room for KEY and MSC events */
events += 7;
--
1.7.9.5
^ permalink raw reply related
* [PATCH] hid: Add battery quirk for USB_DEVICE_ID_APPLE_ALU_WIRELESS_2011_ISO keyboard
From: Karl Relton @ 2014-12-10 11:57 UTC (permalink / raw)
To: linux-input
Apple bluetooth wireless keyboard (sold in UK) has always reported zero
for battery strength no matter what condition the batteries are actually
in. With this patch applied (applying same quirk as other Apple
keyboards), the battery strength is now correctly reported.
Signed-off-by: Karl Relton <karllinuxtest.relton@ntlworld.com>
---
--- linux-3.16.0.orig/drivers/hid/hid-input.c 2014-12-10 10:59:37.864211493 +0000
+++ linux-3.16.0/drivers/hid/hid-input.c 2014-12-04 17:37:47.145113489 +0000
@@ -312,6 +312,9 @@ static const struct hid_device_id hid_ba
USB_DEVICE_ID_APPLE_ALU_WIRELESS_2011_ANSI),
HID_BATTERY_QUIRK_PERCENT | HID_BATTERY_QUIRK_FEATURE },
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE,
+ USB_DEVICE_ID_APPLE_ALU_WIRELESS_2011_ISO),
+ HID_BATTERY_QUIRK_PERCENT | HID_BATTERY_QUIRK_FEATURE },
+ { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_ALU_WIRELESS_ANSI),
HID_BATTERY_QUIRK_PERCENT | HID_BATTERY_QUIRK_FEATURE },
{}
^ permalink raw reply
* NULL pointer dereference in i2c-hid
From: Gabriele Mazzotta @ 2014-12-10 17:04 UTC (permalink / raw)
To: linux-input
Cc: linux-kernel, mika.westerberg, benjamin.tissoires, aduggan,
jkosina
Hi,
my laptop uses a touchpad that needs hid-rmi along with i2c-hid to work.
i2c-hid and hid-rmi can be loaded and unloaded independelty from each
other, however since 34f439e4afcd ("HID: i2c-hid: add runtime PM support")
if I unload hid-rmi and after it I also unload i2c-hid, I get a NULL
pointer dereference.
I have already reported this problem in the Bugzilla [1], but since that
report is about something else, I'm reporting this separately.
Here the dmesg:
[ 79.691459] BUG: unable to handle kernel NULL pointer dereference at (null)
[ 79.691532] IP: [<ffffffffa05bc049>] __i2c_hid_command+0x49/0x310 [i2c_hid]
[ 79.691591] PGD 0
[ 79.691611] Oops: 0002 [#1] SMP
[ 79.691641] Modules linked in: ctr ccm binfmt_misc rfcomm bnep vboxpci(O) vboxnetadp(O) vboxnetflt(O) vboxdrv(O) i2c_hid(-) nfsd auth_rpcgss oid_registry nfs_acl nfs lockd grace fscache sunrpc ecb btusb uvcvideo bluetooth videobuf2_vmalloc joydev videobuf2_memops videobuf2_core hid_multitouch v4l2_common videodev usbhid media hid dell_wmi sparse_keymap arc4 nls_utf8 nls_cp437 iTCO_wdt iTCO_vendor_support intel_rapl x86_pkg_temp_thermal intel_powerclamp coretemp kvm_intel kvm crct10dif_pclmul crc32_pclmul crc32c_intel ghash_clmulni_intel iwlmvm dell_laptop dcdbas aesni_intel mac80211 aes_x86_64 glue_helper snd_hda_codec_realtek lrw gf128mul snd_hda_codec_generic ablk_helper cryptd snd_hda_codec_hdmi iwlwifi psmouse cfg80211 serio_raw sg rfkill lpc_ich mfd_core ehci_pci i2c_i801 ehci_hcd
thermal wmi
[ 79.692330] battery sdhci_acpi sdhci mmc_core intel_rst snd_hda_intel snd_hda_controller snd_hda_codec snd_hwdep snd_pcm i2c_designware_platform xhci_pci i2c_designware_core xhci_hcd snd_timer usbcore snd mei_me soundcore ac evdev usb_common mei shpchp processor fuse parport_pc ppdev lp parport [last unloaded: hid_rmi]
[ 79.692602] CPU: 0 PID: 2898 Comm: rmmod Tainted: G O 3.18.0+ #1
[ 79.692655] Hardware name: Dell Inc. XPS13 9333/0GFTRT, BIOS A04 03/19/2014
[ 79.692705] task: ffff8801eae4a340 ti: ffff8800b4608000 task.ti: ffff8800b4608000
[ 79.692758] RIP: 0010:[<ffffffffa05bc049>] [<ffffffffa05bc049>] __i2c_hid_command+0x49/0x310 [i2c_hid]
[ 79.692830] RSP: 0018:ffff8800b460bce8 EFLAGS: 00010206
[ 79.692868] RAX: ffffffffa05be720 RBX: ffff880212cb2f80 RCX: 0000000000000000
[ 79.692919] RDX: 0000000000000000 RSI: 0000000000000022 RDI: 0000000000000011
[ 79.692968] RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000
[ 79.693018] R10: ffff880216400000 R11: 0000000000000000 R12: 0000000000000004
[ 79.693067] R13: 0000000000000000 R14: ffff880214c08400 R15: 0000000000000000
[ 79.693119] FS: 00007fd597c22700(0000) GS:ffff88021f200000(0000) knlGS:0000000000000000
[ 79.693175] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 79.693216] CR2: 0000000000000000 CR3: 00000000b46b5000 CR4: 00000000001407f0
[ 79.693266] Stack:
[ 79.693283] ffff880215b79800 ffff880214c92b00 ffff880214c084ce ffff880212d68920
[ 79.693344] 0000000000000004 ffffffff810424e1 0000000000000096 ffffffff81042855
[ 79.693405] 0000000000000292 ffff8800cfe77600 0000000000000096 ffff880214c08400
[ 79.693467] Call Trace:
[ 79.693494] [<ffffffff810424e1>] ? __unmask_ioapic+0x21/0x30
[ 79.693537] [<ffffffff81042855>] ? unmask_ioapic+0x25/0x40
[ 79.693581] [<ffffffffa05bc35b>] ? i2c_hid_set_power+0x4b/0xa0 [i2c_hid]
[ 79.693632] [<ffffffffa05bc3cf>] ? i2c_hid_runtime_resume+0x1f/0x30 [i2c_hid]
[ 79.693689] [<ffffffff814c08fb>] ? __rpm_callback+0x2b/0x70
[ 79.693733] [<ffffffff814c0961>] ? rpm_callback+0x21/0x90
[ 79.693776] [<ffffffff814c0dec>] ? rpm_resume+0x41c/0x600
[ 79.693820] [<ffffffff814c1e1c>] ? __pm_runtime_resume+0x4c/0x80
[ 79.693868] [<ffffffff814b8588>] ? __device_release_driver+0x28/0x100
[ 79.693917] [<ffffffff814b8d90>] ? driver_detach+0xa0/0xb0
[ 79.693959] [<ffffffff814b82cc>] ? bus_remove_driver+0x4c/0xb0
[ 79.694006] [<ffffffff810d1cfd>] ? SyS_delete_module+0x11d/0x1d0
[ 79.694054] [<ffffffff8165f107>] ? int_signal+0x12/0x17
[ 79.694095] [<ffffffff8165ee69>] ? system_call_fastpath+0x12/0x17
[ 79.694139] Code: 9f c0 00 00 00 44 8b 66 08 44 0f b6 6e 0c 8b 3e 48 8b 6b 40 48 81 fe 70 e7 5b a0 0f 84 51 02 00 00 89 fe 83 c7 01 0f b6 74 33 10 <40> 88 75 00 0f b6 74 3b 10 40 88 75 01 41 83 fc 02 7e 0f 0f b6
[ 79.694422] RIP [<ffffffffa05bc049>] __i2c_hid_command+0x49/0x310 [i2c_hid]
[ 79.694478] RSP <ffff8800b460bce8>
[ 79.694503] CR2: 0000000000000000
[ 79.712214] ---[ end trace e97e4d6468e56036 ]---
Regards,
Gabriele
[1] https://bugzilla.kernel.org/show_bug.cgi?id=81141
^ permalink raw reply
* Re: Supporting U2F over HID on Linux?
From: Andy Lutomirski @ 2014-12-10 17:07 UTC (permalink / raw)
To: Jiri Kosina
Cc: systemd Mailing List, Kay Sievers, open list:HID CORE LAYER,
Benjamin Tissoires
In-Reply-To: <CALCETrXf4wX5OoGJs2HWbzSLOz764XXg09mSHiTV=-GVCEUQuQ@mail.gmail.com>
On Tue, Dec 9, 2014 at 12:46 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Mon, Nov 3, 2014 at 12:41 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>> On Mon, Nov 3, 2014 at 12:21 PM, Jiri Kosina <jkosina@suse.cz> wrote:
>>> On Mon, 3 Nov 2014, David Herrmann wrote:
>>>
>>>> > Agreed, mostly. My only real concern is that this could be annoying
>>>> > for the userspace developers who will need to target Linux and HIDAPI
>>>> > separately. Admittedly the Linux support will be trivial.
>>>>
>>>> I see. I'll not stop you from using hidraw, I'd just not recommend it.
>>>> Especially for security stuff I dislike exposing the whole HID device
>>>> writable. But yeah, I guess you got my point.
>>>
>>> Alright, I am basically thinking loudly now, but how about we allow HID
>>> drivers that would override default hidraw interface?
>>>
>>> I am very well aware of the fact that this could be opening a can of
>>> worms, so we'll have to make it very restrictive. How about, let's say, we
>>> allow HID drivers to provide custom hidraw interface (completely
>>> overriding the one that HID core would normally create) only for cases
>>> such as:
>>>
>>> - the intent is to expose only certain parts of a combined device
>>> - the intent is to introduce some level of access control
>>>
>>> I.e. still no interference of kernel with data parsing allowed.
>>
>> Hmm. Would this be like a filter on hidraw actions?
>>
>> How would udev distinguish these special hidraw devices from normal
>> hidraw devices?
>>
>> Also, for U2F, this could be a little awkward. There's some crazy
>> fragmentation stuff to allow a U2F request to be split across HID
>> requests, and I think a kernel driver would much rather get the
>> original unfragmented application request.
>>
>
> I started writing a driver for this. I got enumeration working. I
> assume I should create a "u2f" device class, and then... ?
>
> Where am I supposed to get my character device from? Do I register my
> own chrdev major? Do I use misc? Is there some input thing I'm
> supposed to use?
Another question:
I'm hitting this in hid_input_report:
if (down_trylock(&hid->driver_input_lock)) {
pr_err("HID: trylock failed\n"); // I added this
return -EBUSY;
}
This is a problem for u2f: u2f reports are actual protocol messages,
and there isn't a retransmit mechanism. Losing messages randomly
causes the handshake to fail, and then nothing works.
I *think* that the only way I can hit that failure is during probe (or
if the USB stack somehow completes two transfers at once). This still
makes it quite awkward to start IO from the probe routine.
I could start a work item to take driver_input_lock, release it again,
and then start the handshake, but that sucks.
Ideas?
--Andy
>
> Thanks,
> Andy
>
>> --Andy
>>
>>>
>>>> > I can give the driver a try. It'll actually be simpler than the spec
>>>> > makes it out, since a real kernel driver will have no need to
>>>> > arbitrate with itself.
>>>>
>>>> I'll gladly review any custom HID drivers. Feel free to put me on CC
>>>> when sending to linux-input. There's also a lot of examples in
>>>> drivers/hid/ in case you need a head-start (especially if you need the
>>>> raw_event callback).
>>>
>>> Same here, of course.
>>>
>>> Please always CC me in parallel to sending to linux-input@ to make sure
>>> that the patch doesn't fall in between cracks.
>>>
>>> Thanks,
>>>
>>> --
>>> Jiri Kosina
>>> SUSE Labs
>>
>>
>>
>> --
>> Andy Lutomirski
>> AMA Capital Management, LLC
>
>
>
> --
> Andy Lutomirski
> AMA Capital Management, LLC
--
Andy Lutomirski
AMA Capital Management, LLC
_______________________________________________
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
^ permalink raw reply
* Re: [PATCH 2/2] Input: gpio_keys - replace timer and workqueue with delayed workqueue
From: Andy Shevchenko @ 2014-12-10 18:32 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: linux-input, Alexander Stein, Linus Walleij, Ezequiel Garcia,
Alexander Shiyan, Laxman Dewangan, devicetree, linux-kernel
In-Reply-To: <20141208072124.GA21031@dtor-ws>
On Sun, 2014-12-07 at 23:21 -0800, Dmitry Torokhov wrote:
> We do not need to roll our own implementation of delayed work now that we
> have proper implementation of mod_delayed_work.
>
> For interrupt-only driven buttons we retain the timer, but we rename
> it to release_timer to better reflect its purpose.
At least it doesn't break the driver on Intel Medfield device.
Tested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
> drivers/input/keyboard/gpio_keys.c | 65 +++++++++++++++++-------------------
> 1 file changed, 31 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
> index a5ece3f..eefd976 100644
> --- a/drivers/input/keyboard/gpio_keys.c
> +++ b/drivers/input/keyboard/gpio_keys.c
> @@ -35,9 +35,13 @@
> struct gpio_button_data {
> const struct gpio_keys_button *button;
> struct input_dev *input;
> - struct timer_list timer;
> - struct work_struct work;
> - unsigned int timer_debounce; /* in msecs */
> +
> + struct timer_list release_timer;
> + unsigned int release_delay; /* in msecs, for IRQ-only buttons */
> +
> + struct delayed_work work;
> + unsigned int software_debounce; /* in msecs, for GPIO-driven buttons */
> +
> unsigned int irq;
> spinlock_t lock;
> bool disabled;
> @@ -116,11 +120,14 @@ static void gpio_keys_disable_button(struct gpio_button_data *bdata)
> {
> if (!bdata->disabled) {
> /*
> - * Disable IRQ and possible debouncing timer.
> + * Disable IRQ and associated timer/work structure.
> */
> disable_irq(bdata->irq);
> - if (bdata->timer_debounce)
> - del_timer_sync(&bdata->timer);
> +
> + if (gpio_is_valid(bdata->button->gpio))
> + cancel_delayed_work_sync(&bdata->work);
> + else
> + del_timer_sync(&bdata->release_timer);
>
> bdata->disabled = true;
> }
> @@ -343,7 +350,7 @@ static void gpio_keys_gpio_report_event(struct gpio_button_data *bdata)
> static void gpio_keys_gpio_work_func(struct work_struct *work)
> {
> struct gpio_button_data *bdata =
> - container_of(work, struct gpio_button_data, work);
> + container_of(work, struct gpio_button_data, work.work);
>
> gpio_keys_gpio_report_event(bdata);
>
> @@ -351,13 +358,6 @@ static void gpio_keys_gpio_work_func(struct work_struct *work)
> pm_relax(bdata->input->dev.parent);
> }
>
> -static void gpio_keys_gpio_timer(unsigned long _data)
> -{
> - struct gpio_button_data *bdata = (struct gpio_button_data *)_data;
> -
> - schedule_work(&bdata->work);
> -}
> -
> static irqreturn_t gpio_keys_gpio_isr(int irq, void *dev_id)
> {
> struct gpio_button_data *bdata = dev_id;
> @@ -366,11 +366,10 @@ static irqreturn_t gpio_keys_gpio_isr(int irq, void *dev_id)
>
> if (bdata->button->wakeup)
> pm_stay_awake(bdata->input->dev.parent);
> - if (bdata->timer_debounce)
> - mod_timer(&bdata->timer,
> - jiffies + msecs_to_jiffies(bdata->timer_debounce));
> - else
> - schedule_work(&bdata->work);
> +
> + mod_delayed_work(system_wq,
> + &bdata->work,
> + msecs_to_jiffies(bdata->software_debounce));
>
> return IRQ_HANDLED;
> }
> @@ -408,7 +407,7 @@ static irqreturn_t gpio_keys_irq_isr(int irq, void *dev_id)
> input_event(input, EV_KEY, button->code, 1);
> input_sync(input);
>
> - if (!bdata->timer_debounce) {
> + if (!bdata->release_delay) {
> input_event(input, EV_KEY, button->code, 0);
> input_sync(input);
> goto out;
> @@ -417,9 +416,9 @@ static irqreturn_t gpio_keys_irq_isr(int irq, void *dev_id)
> bdata->key_pressed = true;
> }
>
> - if (bdata->timer_debounce)
> - mod_timer(&bdata->timer,
> - jiffies + msecs_to_jiffies(bdata->timer_debounce));
> + if (bdata->release_delay)
> + mod_timer(&bdata->release_timer,
> + jiffies + msecs_to_jiffies(bdata->release_delay));
> out:
> spin_unlock_irqrestore(&bdata->lock, flags);
> return IRQ_HANDLED;
> @@ -429,10 +428,10 @@ static void gpio_keys_quiesce_key(void *data)
> {
> struct gpio_button_data *bdata = data;
>
> - if (bdata->timer_debounce)
> - del_timer_sync(&bdata->timer);
> -
> - cancel_work_sync(&bdata->work);
> + if (gpio_is_valid(bdata->button->gpio))
> + cancel_delayed_work_sync(&bdata->work);
> + else
> + del_timer_sync(&bdata->release_timer);
> }
>
> static int gpio_keys_setup_key(struct platform_device *pdev,
> @@ -466,7 +465,7 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
> button->debounce_interval * 1000);
> /* use timer if gpiolib doesn't provide debounce */
> if (error < 0)
> - bdata->timer_debounce =
> + bdata->software_debounce =
> button->debounce_interval;
> }
>
> @@ -484,9 +483,7 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
> bdata->irq = irq;
> }
>
> - INIT_WORK(&bdata->work, gpio_keys_gpio_work_func);
> - setup_timer(&bdata->timer,
> - gpio_keys_gpio_timer, (unsigned long)bdata);
> + INIT_DELAYED_WORK(&bdata->work, gpio_keys_gpio_work_func);
>
> isr = gpio_keys_gpio_isr;
> irqflags = IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING;
> @@ -503,8 +500,8 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
> return -EINVAL;
> }
>
> - bdata->timer_debounce = button->debounce_interval;
> - setup_timer(&bdata->timer,
> + bdata->release_delay = button->debounce_interval;
> + setup_timer(&bdata->release_timer,
> gpio_keys_irq_timer, (unsigned long)bdata);
>
> isr = gpio_keys_irq_isr;
> @@ -514,7 +511,7 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
> input_set_capability(input, button->type ?: EV_KEY, button->code);
>
> /*
> - * Install custom action to cancel debounce timer and
> + * Install custom action to cancel release timer and
> * workqueue item.
> */
> error = devm_add_action(&pdev->dev, gpio_keys_quiesce_key, bdata);
>
--
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy
^ permalink raw reply
* Re: [PATCH v2 3/3] HID: wacom: Report input events for each finger on generic devices
From: Benjamin Tissoires @ 2014-12-10 20:25 UTC (permalink / raw)
To: Jason Gerecke; +Cc: Jiri Kosina, linux-input, Ping Cheng
In-Reply-To: <1417815453-10618-2-git-send-email-killertofu@gmail.com>
On Fri, Dec 5, 2014 at 4:37 PM, Jason Gerecke <killertofu@gmail.com> wrote:
> The existing generic touch code only reports events after reading an
> entire HID report, which practically means that only data about the last
> contact in a report will ever be provided to userspace. This patch uses
> a trick from hid-multitouch.c to discover what type of field is at the
> end of each contact; when such a field is encountered all the stored
> contact data will be reported.
>
> Signed-off-by: Jason Gerecke <killertofu@gmail.com>
> ---
Thanks for the v2 Jason.
I think we are closes to it. I have some comments on the style, but
now that I have read it more carefully, I have a better idea of it,
and the patch is not so scary.
Jiri. If you reviewed it and took it, that's fine. If not, then I
think there is still room for improvements.
Cheers,
Benjamin
> Changes from v1:
> * Patch renamed from "HID: wacom: Report input events immediately upon receipt"
> * Instead of reporting on-reciept, events are bundled and sent at the end of each contact
> * Adds code to discover 'last_slot_field' based on hid-multitouch.c implementation
>
> drivers/hid/wacom_wac.c | 94 ++++++++++++++++++++++++++++++++-----------------
> drivers/hid/wacom_wac.h | 1 +
> 2 files changed, 63 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
> index c33f889..75fc16e 100644
> --- a/drivers/hid/wacom_wac.c
> +++ b/drivers/hid/wacom_wac.c
> @@ -1381,35 +1381,68 @@ static void wacom_wac_finger_usage_mapping(struct hid_device *hdev,
> {
> struct wacom *wacom = hid_get_drvdata(hdev);
> struct wacom_wac *wacom_wac = &wacom->wacom_wac;
> - unsigned touch_max = wacom_wac->features.touch_max;
not sure we really need to remove the temporary variable touch_max here.
> + struct wacom_features *features = &wacom_wac->features;
>
> switch (usage->hid) {
> case HID_GD_X:
> - if (touch_max == 1)
> + features->last_slot_field = usage->hid;
> + if (features->touch_max == 1)
these changes (touchmax => features->touch_max) hinder a little bit
the actual change, that is worrisome :)
> wacom_map_usage(wacom, usage, field, EV_ABS, ABS_X, 4);
> else
> wacom_map_usage(wacom, usage, field, EV_ABS,
> ABS_MT_POSITION_X, 4);
> break;
> case HID_GD_Y:
> - if (touch_max == 1)
> + features->last_slot_field = usage->hid;
> + if (features->touch_max == 1)
> wacom_map_usage(wacom, usage, field, EV_ABS, ABS_Y, 4);
> else
> wacom_map_usage(wacom, usage, field, EV_ABS,
> ABS_MT_POSITION_Y, 4);
> break;
> case HID_DG_CONTACTID:
> + features->last_slot_field = usage->hid;
> break;
> case HID_DG_INRANGE:
> + features->last_slot_field = usage->hid;
> break;
> case HID_DG_INVERT:
> + features->last_slot_field = usage->hid;
> break;
> case HID_DG_TIPSWITCH:
> + features->last_slot_field = usage->hid;
> wacom_map_usage(wacom, usage, field, EV_KEY, BTN_TOUCH, 0);
> break;
> }
> }
>
> +static void wacom_wac_finger_slot(struct wacom_wac *wacom_wac,
> + struct input_dev *input)
> +{
> + struct hid_data *hid_data = &wacom_wac->hid_data;
> + bool mt = wacom_wac->features.touch_max > 1;
> + bool prox = hid_data->tipswitch &&
> + !wacom_wac->shared->stylus_in_proximity;
> +
> + if (mt) {
Personal taste. I don't like the "if (mt)" approach in this patch. I
preferred the old approach:
if (mt)
wacom_wac_finger_mt_slot()
else
wacom_wac_finger_touch()
i.e. one function per case.
> + int slot;
> +
> + slot = input_mt_get_slot_by_key(input, hid_data->id);
> + input_mt_slot(input, slot);
> + input_mt_report_slot_state(input, MT_TOOL_FINGER, prox);
> + }
> + else {
> + input_report_key(input, BTN_TOUCH, prox);
> + }
> +
> + if (prox) {
> + input_report_abs(input, mt ? ABS_MT_POSITION_X : ABS_X,
> + hid_data->x);
> + input_report_abs(input, mt ? ABS_MT_POSITION_Y : ABS_Y,
> + hid_data->y);
> + }
> +}
> +
> static int wacom_wac_finger_event(struct hid_device *hdev,
> struct hid_field *field, struct hid_usage *usage, __s32 value)
> {
> @@ -1432,36 +1465,35 @@ static int wacom_wac_finger_event(struct hid_device *hdev,
> }
>
>
> + if (usage->usage_index + 1 == field->report_count) {
> + if (usage->hid == wacom_wac->features.last_slot_field)
OK, I think it would be fair to assume that the FW guys will send N
full touches by report. IIRC, I had the case where sometimes they add
some random axis in the end (like X or Y), just to make me cry :)
should be good here though (and we can have access to the FW guys if
they do something too much funny :-P )
> + wacom_wac_finger_slot(wacom_wac, wacom_wac->input);
> + }
> +
> return 0;
> }
>
> -static void wacom_wac_finger_mt_report(struct wacom_wac *wacom_wac,
> - struct input_dev *input, bool touch)
> +static int wacom_wac_finger_touches(struct hid_device *hdev)
It took me a while to understand what this function was. It simply
returns the number of touches that has been recorded in the report (I
was mislead by the git diff I think).
We should rename this in something else
(wacom_wac_finger_get_touches_count or something better - I am bad at
naming things).
Second question, why the need to return the touch count?
> {
> - int slot;
> - struct hid_data *hid_data = &wacom_wac->hid_data;
> + struct wacom *wacom = hid_get_drvdata(hdev);
> + struct wacom_wac *wacom_wac = &wacom->wacom_wac;
> + struct input_dev *input = wacom_wac->input;
> + unsigned touch_max = wacom_wac->features.touch_max;
> + int count = 0;
> + int i;
>
> - slot = input_mt_get_slot_by_key(input, hid_data->id);
> + if (touch_max == 1)
> + return wacom_wac->hid_data.tipswitch &&
> + !wacom_wac->shared->stylus_in_proximity;
>
> - input_mt_slot(input, slot);
> - input_mt_report_slot_state(input, MT_TOOL_FINGER, touch);
> - if (touch) {
> - input_report_abs(input, ABS_MT_POSITION_X, hid_data->x);
> - input_report_abs(input, ABS_MT_POSITION_Y, hid_data->y);
> + for (i = 0; i < input->mt->num_slots; i++) {
> + struct input_mt_slot *ps = &input->mt->slots[i];
> + int id = input_mt_get_value(ps, ABS_MT_TRACKING_ID);
> + if (id >= 0)
> + count++;
> }
> - input_mt_sync_frame(input);
> -}
> -
> -static void wacom_wac_finger_single_touch_report(struct wacom_wac *wacom_wac,
> - struct input_dev *input, bool touch)
> -{
> - struct hid_data *hid_data = &wacom_wac->hid_data;
>
> - if (touch) {
> - input_report_abs(input, ABS_X, hid_data->x);
> - input_report_abs(input, ABS_Y, hid_data->y);
> - }
> - input_report_key(input, BTN_TOUCH, touch);
> + return count;
> }
>
> static void wacom_wac_finger_report(struct hid_device *hdev,
> @@ -1470,18 +1502,16 @@ static void wacom_wac_finger_report(struct hid_device *hdev,
> struct wacom *wacom = hid_get_drvdata(hdev);
> struct wacom_wac *wacom_wac = &wacom->wacom_wac;
> struct input_dev *input = wacom_wac->input;
> - bool touch = wacom_wac->hid_data.tipswitch &&
> - !wacom_wac->shared->stylus_in_proximity;
> - unsigned touch_max = wacom_wac->features.touch_max;
>
> - if (touch_max > 1)
> - wacom_wac_finger_mt_report(wacom_wac, input, touch);
> + if (wacom_wac->features.touch_max > 1)
I think removing the touch_max variable hides a little bit the changes too.
> + input_mt_sync_frame(input);
> else
> - wacom_wac_finger_single_touch_report(wacom_wac, input, touch);
> + wacom_wac_finger_slot(wacom_wac, input);
Not sure there is a need to call wacom_wac_finger_slot() for single
touch devices. Your current implementation in wacom_finger_event
should match also single finger tablets.
> +
> input_sync(input);
>
> /* keep touch state for pen event */
> - wacom_wac->shared->touch_down = touch;
> + wacom_wac->shared->touch_down = wacom_wac_finger_touches(hdev);
> }
>
> #define WACOM_PEN_FIELD(f) (((f)->logical == HID_DG_STYLUS) || \
> diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h
> index 128cbb3..6e256206 100644
> --- a/drivers/hid/wacom_wac.h
> +++ b/drivers/hid/wacom_wac.h
> @@ -144,6 +144,7 @@ struct wacom_features {
> int pktlen;
> bool check_for_hid_type;
> int hid_type;
> + int last_slot_field;
> };
>
> struct wacom_shared {
> --
> 2.1.3
>
^ permalink raw reply
* Re: [PATCH v2] HID: rmi: Scan the report descriptor to determine if the device is suitable for the hid-rmi driver
From: Benjamin Tissoires @ 2014-12-10 20:42 UTC (permalink / raw)
To: Andrew Duggan
Cc: linux-input, linux-kernel@vger.kernel.org, Jiri Kosina,
Benjamin Tissoires
In-Reply-To: <54863F72.1060401@synaptics.com>
Hi Andrew,
On Mon, Dec 8, 2014 at 7:16 PM, Andrew Duggan <aduggan@synaptics.com> wrote:
> On 11/24/2014 03:37 PM, Andrew Duggan wrote:
>>
>> On composite HID devices there may be multiple HID devices on separate
>> interfaces, but hid-rmi
>> should only bind to the touchpad. The previous version simply checked that
>> the interface protocol
>> was set to mouse. Unfortuately, it is not always the case that the
>> touchpad has the mouse interface
>> protocol set. This patch takes a different approach and scans the report
>> descriptor looking for
>> the Generic Desktop Pointer usage and the Vendor Specific Top Level
>> Collection needed by the
>> hid-rmi driver to interface with the device.
>>
>> Signed-off-by: Andrew Duggan <aduggan@synaptics.com>
>> ---
>> This is a second attempt at the patch I submitted back in October. Instead
>> of looking for specific
>> HID reports this patch looks for the Generic Desktop Pointer usage. Having
>> that usage along with
>> the vendor specific collection seems to be a distinctive enough to id the
>> Synaptics touchpad in
>> the composite USB device.
>
> Any comments on this patch?
Sorry for the lag on this one.
>
> Thanks,
> Andrew
>
>> drivers/hid/hid-core.c | 21 ++++++++++++++++-----
>> include/linux/hid.h | 4 +++-
>> 2 files changed, 19 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>> index 12b6e67..ba9dc59 100644
>> --- a/drivers/hid/hid-core.c
>> +++ b/drivers/hid/hid-core.c
>> @@ -698,10 +698,20 @@ static void hid_scan_feature_usage(struct hid_parser
>> *parser, u32 usage)
>> static void hid_scan_collection(struct hid_parser *parser, unsigned
>> type)
>> {
>> struct hid_device *hid = parser->device;
>> + int i;
>> if (((parser->global.usage_page << 16) == HID_UP_SENSOR) &&
>> type == HID_COLLECTION_PHYSICAL)
>> hid->group = HID_GROUP_SENSOR_HUB;
>> +
>> + if ((parser->global.usage_page << 16) == HID_UP_GENDESK)
>> + for (i = 0; i < parser->local.usage_index; i++)
>> + if (parser->local.usage[i] == HID_GD_POINTER)
>> + parser->scan_flags
>> + |= HID_SCAN_FLAG_GENDESK_POINTER;
Change GENDESK into GD (or remove it) and the line will fit on 80 chars :)
>> +
>> + if ((parser->global.usage_page << 16) == HID_UP_MSVENDOR)
>> + parser->scan_flags |= HID_SCAN_FLAG_VENDOR_SPECIFIC;
This, I don't completely like it.
We should have "if ((parser->global.usage_page << 16) >=
HID_UP_MSVENDOR)". The HUT 1.12 specifies that usage pages between
0xff00 and 0xffff are vendor specific. So if we want to put a flag on
it, then using ">="is more appropriate.
Side note, we should really rename this #define HID_UP_MSVENDOR, this
has nothing to do with Microsoft.
>> }
>> static int hid_scan_main(struct hid_parser *parser, struct hid_item
>> *item)
>> @@ -783,11 +793,12 @@ static int hid_scan_report(struct hid_device *hid)
>> * Vendor specific handlings
>> */
>> if ((hid->vendor == USB_VENDOR_ID_SYNAPTICS) &&
>> - (hid->group == HID_GROUP_GENERIC) &&
>> - /* only bind to the mouse interface of composite USB devices
>> */
>> - (hid->bus != BUS_USB || hid->type == HID_TYPE_USBMOUSE))
>> - /* hid-rmi should take care of them, not hid-generic */
>> - hid->group = HID_GROUP_RMI;
>> + (hid->group == HID_GROUP_GENERIC)) {
>> + if ((parser->scan_flags & HID_SCAN_FLAG_VENDOR_SPECIFIC)
>> + && (parser->scan_flags &
>> HID_SCAN_FLAG_GENDESK_POINTER))
>> + /* hid-rmi should take care of them, not
>> hid-generic */
>> + hid->group = HID_GROUP_RMI;
>> + }
I prefer it. The differentiation is done at the report descriptor
level, it's better for me.
>> /*
>> * Vendor specific handlings
>> diff --git a/include/linux/hid.h b/include/linux/hid.h
>> index f53c4a9..b019f15 100644
>> --- a/include/linux/hid.h
>> +++ b/include/linux/hid.h
>> @@ -547,7 +547,9 @@ static inline void hid_set_drvdata(struct hid_device
>> *hdev, void *data)
>> #define HID_GLOBAL_STACK_SIZE 4
>> #define HID_COLLECTION_STACK_SIZE 4
>> -#define HID_SCAN_FLAG_MT_WIN_8 0x00000001
>> +#define HID_SCAN_FLAG_MT_WIN_8 BIT(0)
>> +#define HID_SCAN_FLAG_VENDOR_SPECIFIC BIT(1)
>> +#define HID_SCAN_FLAG_GENDESK_POINTER BIT(2)
>> struct hid_parser {
>> struct hid_global global;
>
>
With the minors comments I expressed, this patch is:
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Thanks Andrew!
Cheers,
Benjamin
^ permalink raw reply
* Re: [PATCH] HID: rmi: Add support for the touchpad in the Razer Blade 14 laptop
From: Benjamin Tissoires @ 2014-12-10 20:51 UTC (permalink / raw)
To: Andrew Duggan
Cc: linux-input, linux-kernel@vger.kernel.org, Jiri Kosina,
Benjamin Tissoires
In-Reply-To: <54863F7A.30609@synaptics.com>
On Mon, Dec 8, 2014 at 7:16 PM, Andrew Duggan <aduggan@synaptics.com> wrote:
> On 11/24/2014 04:20 PM, Andrew Duggan wrote:
>>
>> The Razer Blade 14 has a Synaptic's TouchPad on one of the interfaces of
>> a composite USB device. This patch allows the hid-rmi driver to bind
>> to that interface. It also adds support for the external click buttons
>> on the Razer's touchpad. External buttons are reported using generic
>> mouse reports instead of through the F30 like it is on ClickPads.
>>
>> Signed-off-by: Andrew Duggan <aduggan@synaptics.com>
>> ---
>> This patch depends on the "HID: rmi: Scan the report descriptor to
>> determine if the device is
>> suitable for the hid-rmi driver" I submitted earlier today to correctly
>> bind to the touchpad HID
>> device in the composite USB device.
>
> Any comments on this patch?
Again, sorry for the lag on this series. I think now that I re-read
this one I understood why I did not put too many efforts to properly
review the series. This one is a little bit worrisome IMO.
>
> Thanks,
> Andrew
>
>> drivers/hid/hid-core.c | 4 +++-
>> drivers/hid/hid-ids.h | 3 +++
>> drivers/hid/hid-rmi.c | 15 ++++++++++++++-
>> 3 files changed, 20 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>> index ba9dc59..d69ea16 100644
>> --- a/drivers/hid/hid-core.c
>> +++ b/drivers/hid/hid-core.c
>> @@ -792,7 +792,9 @@ static int hid_scan_report(struct hid_device *hid)
>> /*
>> * Vendor specific handlings
>> */
>> - if ((hid->vendor == USB_VENDOR_ID_SYNAPTICS) &&
>> + if ((hid->vendor == USB_VENDOR_ID_SYNAPTICS
>> + || (hid->vendor == USB_VENDOR_ID_RAZER
>> + && hid->product == USB_DEVICE_ID_RAZER_BLADE_14)) &&
I don't like this. We already have a blacklist, an ignore list, and
there, we will have a new blacklist...
I understood why you put this here, the current have_special_driver
list will not fit 100%. Still, I find it not good.
It works, but I think we should still head for an entry in
have_special_driver, and a specific behavior in hid-rmi which would
rely on hid-input to handle the keyboard/mouse buttons and the rest.
>> (hid->group == HID_GROUP_GENERIC)) {
>> if ((parser->scan_flags & HID_SCAN_FLAG_VENDOR_SPECIFIC)
>> && (parser->scan_flags &
>> HID_SCAN_FLAG_GENDESK_POINTER))
>> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
>> index 25cd674..c677aad 100644
>> --- a/drivers/hid/hid-ids.h
>> +++ b/drivers/hid/hid-ids.h
>> @@ -751,6 +751,9 @@
>> #define USB_DEVICE_ID_QUANTA_OPTICAL_TOUCH_3001 0x3001
>> #define USB_DEVICE_ID_QUANTA_OPTICAL_TOUCH_3008 0x3008
>> +#define USB_VENDOR_ID_RAZER 0x1532
>> +#define USB_DEVICE_ID_RAZER_BLADE_14 0x011D
>> +
>> #define USB_VENDOR_ID_REALTEK 0x0bda
>> #define USB_DEVICE_ID_REALTEK_READER 0x0152
>> diff --git a/drivers/hid/hid-rmi.c b/drivers/hid/hid-rmi.c
>> index 3cccff7..1f131df 100644
>> --- a/drivers/hid/hid-rmi.c
>> +++ b/drivers/hid/hid-rmi.c
>> @@ -453,7 +453,15 @@ static int rmi_raw_event(struct hid_device *hdev,
>> case RMI_ATTN_REPORT_ID:
>> return rmi_input_event(hdev, data, size);
>> case RMI_MOUSE_REPORT_ID:
>> - rmi_schedule_reset(hdev);
>> + /*
>> + * touchpads with physical mouse buttons will report those
>> + * buttons in mouse reports even in RMI mode. Only reset
>> + * the device if we see reports which contain X or Y data.
>> + */
>> + if (data[2] != 0 || data[3] != 0)
this seems a little bit magical. There may not be an other solution,
but I would prefer that we look at alternatives before using this
magical numbers (will the touchpad always use the same report
descriptor on the mouse interface?)
>> + rmi_schedule_reset(hdev);
>> + else
>> + return 1;
>> break;
>> }
>> @@ -871,6 +879,11 @@ static int rmi_input_mapping(struct hid_device
>> *hdev,
>> struct hid_input *hi, struct hid_field *field,
>> struct hid_usage *usage, unsigned long **bit, int *max)
>> {
>> + if (field->application == HID_GD_POINTER
>> + && (usage->hid & HID_USAGE_PAGE) == HID_UP_BUTTON)
>> + /* Pass mouse button reports to generic code for
>> processing */
>> + return 0;
>> +
>> /* we want to make HID ignore the advertised HID collection */
>> return -1;
>> }
>
>
Cheers,
Benjamin
^ permalink raw reply
* [PATCH 1/2] HID: logitech-hidpp: do not return the name length
From: Benjamin Tissoires @ 2014-12-10 22:21 UTC (permalink / raw)
To: Jiri Kosina, Nestor Lopez Casado
Cc: Peter Hutterer, linux-input, linux-kernel
We do not make any use of the actual name length get through
hidpp_get_device_name().
We can drop the extra code and simplify the API a bit.
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
drivers/hid/hid-logitech-hidpp.c | 10 +++-------
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 1a6395d..3846305 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -461,7 +461,7 @@ static int hidpp_devicenametype_get_device_name(struct hidpp_device *hidpp,
return count;
}
-static char *hidpp_get_device_name(struct hidpp_device *hidpp, u8 *name_length)
+static char *hidpp_get_device_name(struct hidpp_device *hidpp)
{
u8 feature_type;
u8 feature_index;
@@ -484,7 +484,6 @@ static char *hidpp_get_device_name(struct hidpp_device *hidpp, u8 *name_length)
if (!name)
goto out_err;
- *name_length = __name_length + 1;
while (index < __name_length)
index += hidpp_devicenametype_get_device_name(hidpp,
feature_index, index, name + index,
@@ -493,7 +492,6 @@ static char *hidpp_get_device_name(struct hidpp_device *hidpp, u8 *name_length)
return name;
out_err:
- *name_length = 0;
return NULL;
}
@@ -989,7 +987,6 @@ static void hidpp_overwrite_name(struct hid_device *hdev, bool use_unifying)
{
struct hidpp_device *hidpp = hid_get_drvdata(hdev);
char *name;
- u8 name_length;
if (use_unifying)
/*
@@ -999,7 +996,7 @@ static void hidpp_overwrite_name(struct hid_device *hdev, bool use_unifying)
*/
name = hidpp_get_unifying_name(hidpp);
else
- name = hidpp_get_device_name(hidpp, &name_length);
+ name = hidpp_get_device_name(hidpp);
if (!name)
hid_err(hdev, "unable to retrieve the name of the device");
@@ -1053,7 +1050,6 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
bool connected = atomic_read(&hidpp->connected);
struct input_dev *input;
char *name, *devm_name;
- u8 name_length;
if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)
wtp_connect(hdev, connected);
@@ -1080,7 +1076,7 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
return;
}
- name = hidpp_get_device_name(hidpp, &name_length);
+ name = hidpp_get_device_name(hidpp);
if (!name) {
hid_err(hdev, "unable to retrieve the name of the device");
} else {
--
2.1.0
^ permalink raw reply related
* [PATCH 2/2] HID: logitech-hidpp: prefix the name with Logitech
From: Benjamin Tissoires @ 2014-12-10 22:21 UTC (permalink / raw)
To: Jiri Kosina, Nestor Lopez Casado
Cc: Peter Hutterer, linux-input, linux-kernel
In-Reply-To: <1418250070-28604-1-git-send-email-benjamin.tissoires@redhat.com>
Current names are reported as "K750", "M705", and it can be misleading
for the users when they look at their input device list.
Prefixing the names with "Logitech " makes things better.
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
Hi Jiri,
I'd love to see this one in 3.19 (after a strong review, of course).
The idea came with the mouse DPI database that Peter is currently working on
(see http://who-t.blogspot.com.au/2014/12/building-a-dpi-database-for-mice.html).
I think, if you do not qualify the series for 3.19, we should drop it entirely.
3.19 introduces the hidpp module and changes the labels. The DPI hwdb will check
on the label to match the actual mouse, so if this one does not get into 3.19,
we will end up in 3 entries for each Logitech device.
Cheers,
Benjamin
drivers/hid/hid-logitech-hidpp.c | 34 ++++++++++++++++++++++++++++++++++
1 file changed, 34 insertions(+)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 3846305..8cf4352 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -282,6 +282,33 @@ static inline bool hidpp_report_is_connect_event(struct hidpp_report *report)
(report->rap.sub_id == 0x41);
}
+/**
+ * hidpp_prefix_name() prefixes the current given name with "Logitech ".
+ */
+static void hidpp_prefix_name(char **name, int name_length)
+{
+#define PREFIX_SIZE 9 /* "Logitech " */
+
+ int new_length;
+ char *new_name;
+
+ if (name_length > PREFIX_SIZE &&
+ strncmp(*name, "Logitech ", PREFIX_SIZE) == 0)
+ /* The prefix has is already in the name */
+ return;
+
+ new_length = name_length + PREFIX_SIZE;
+ new_name = kzalloc(new_length, GFP_KERNEL);
+ if (!new_name)
+ return;
+
+ snprintf(new_name, new_length, "Logitech %s", *name);
+
+ kfree(*name);
+
+ *name = new_name;
+}
+
/* -------------------------------------------------------------------------- */
/* HIDP++ 1.0 commands */
/* -------------------------------------------------------------------------- */
@@ -318,6 +345,10 @@ static char *hidpp_get_unifying_name(struct hidpp_device *hidpp_dev)
return NULL;
memcpy(name, &response.rap.params[2], len);
+
+ /* include the terminating '\0' */
+ hidpp_prefix_name(&name, len + 1);
+
return name;
}
@@ -489,6 +520,9 @@ static char *hidpp_get_device_name(struct hidpp_device *hidpp)
feature_index, index, name + index,
__name_length - index);
+ /* include the terminating '\0' */
+ hidpp_prefix_name(&name, __name_length + 1);
+
return name;
out_err:
--
2.1.0
^ permalink raw reply related
* Re: [PATCH 1/2] HID: logitech-hidpp: do not return the name length
From: Peter Wu @ 2014-12-10 22:53 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Jiri Kosina, Nestor Lopez Casado, Peter Hutterer, linux-input,
linux-kernel
In-Reply-To: <1418250070-28604-1-git-send-email-benjamin.tissoires@redhat.com>
On Wednesday 10 December 2014 17:21:09 Benjamin Tissoires wrote:
> We do not make any use of the actual name length get through
> hidpp_get_device_name().
>
> We can drop the extra code and simplify the API a bit.
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>
> ---
> drivers/hid/hid-logitech-hidpp.c | 10 +++-------
> 1 file changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 1a6395d..3846305 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -461,7 +461,7 @@ static int hidpp_devicenametype_get_device_name(struct hidpp_device *hidpp,
> return count;
> }
>
> -static char *hidpp_get_device_name(struct hidpp_device *hidpp, u8 *name_length)
> +static char *hidpp_get_device_name(struct hidpp_device *hidpp)
> {
> u8 feature_type;
> u8 feature_index;
> @@ -484,7 +484,6 @@ static char *hidpp_get_device_name(struct hidpp_device *hidpp, u8 *name_length)
> if (!name)
> goto out_err;
>
> - *name_length = __name_length + 1;
> while (index < __name_length)
> index += hidpp_devicenametype_get_device_name(hidpp,
> feature_index, index, name + index,
hidpp_devicenametype_get_device_name can return a non-positive value if
the USB device is unplugged at the wrong time, or if a malicious device
is attached (and 0 is returned). An infinite loop is the result.
Can you apply this change in the patch or should I send you a separate
one?
while (index < __name_length) {
ret = hidpp_devicenametype_get_device_name(hidpp,
feature_index, index, name + index,
__name_length - index);
if (ret <= 0) {
kfree(name);
return NULL;
}
index += ret;
}
> @@ -493,7 +492,6 @@ static char *hidpp_get_device_name(struct hidpp_device *hidpp, u8 *name_length)
> return name;
>
> out_err:
> - *name_length = 0;
> return NULL;
What about dropping the label out_err here and returning NULL in the
previous places?
> }
>
> @@ -989,7 +987,6 @@ static void hidpp_overwrite_name(struct hid_device *hdev, bool use_unifying)
> {
> struct hidpp_device *hidpp = hid_get_drvdata(hdev);
> char *name;
> - u8 name_length;
>
> if (use_unifying)
> /*
> @@ -999,7 +996,7 @@ static void hidpp_overwrite_name(struct hid_device *hdev, bool use_unifying)
> */
> name = hidpp_get_unifying_name(hidpp);
> else
> - name = hidpp_get_device_name(hidpp, &name_length);
> + name = hidpp_get_device_name(hidpp);
>
> if (!name)
> hid_err(hdev, "unable to retrieve the name of the device");
> @@ -1053,7 +1050,6 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
> bool connected = atomic_read(&hidpp->connected);
> struct input_dev *input;
> char *name, *devm_name;
> - u8 name_length;
>
> if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)
> wtp_connect(hdev, connected);
> @@ -1080,7 +1076,7 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
> return;
> }
>
> - name = hidpp_get_device_name(hidpp, &name_length);
> + name = hidpp_get_device_name(hidpp);
> if (!name) {
> hid_err(hdev, "unable to retrieve the name of the device");
> } else {
>
^ permalink raw reply
* Re: [PATCH 2/2] HID: logitech-hidpp: prefix the name with Logitech
From: Peter Wu @ 2014-12-10 23:01 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Jiri Kosina, Nestor Lopez Casado, Peter Hutterer, linux-input,
linux-kernel
In-Reply-To: <1418250070-28604-2-git-send-email-benjamin.tissoires@redhat.com>
On Wednesday 10 December 2014 17:21:10 Benjamin Tissoires wrote:
> Current names are reported as "K750", "M705", and it can be misleading
> for the users when they look at their input device list.
>
> Prefixing the names with "Logitech " makes things better.
Doesn't this apply to all input devices? Like, every USB device can be
queried for the manufacturer which could then by included by the input
subsystem.
What about duplicate names? Can this patch cause a conflict with devices
having the same name?
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
>
> Hi Jiri,
>
> I'd love to see this one in 3.19 (after a strong review, of course).
> The idea came with the mouse DPI database that Peter is currently working on
> (see http://who-t.blogspot.com.au/2014/12/building-a-dpi-database-for-mice.html).
>
> I think, if you do not qualify the series for 3.19, we should drop it entirely.
> 3.19 introduces the hidpp module and changes the labels. The DPI hwdb will check
> on the label to match the actual mouse, so if this one does not get into 3.19,
> we will end up in 3 entries for each Logitech device.
>
> Cheers,
> Benjamin
>
>
> drivers/hid/hid-logitech-hidpp.c | 34 ++++++++++++++++++++++++++++++++++
> 1 file changed, 34 insertions(+)
>
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 3846305..8cf4352 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -282,6 +282,33 @@ static inline bool hidpp_report_is_connect_event(struct hidpp_report *report)
> (report->rap.sub_id == 0x41);
> }
>
> +/**
> + * hidpp_prefix_name() prefixes the current given name with "Logitech ".
> + */
> +static void hidpp_prefix_name(char **name, int name_length)
> +{
> +#define PREFIX_SIZE 9 /* "Logitech " */
> +
> + int new_length;
> + char *new_name;
> +
> + if (name_length > PREFIX_SIZE &&
> + strncmp(*name, "Logitech ", PREFIX_SIZE) == 0)
I think you meant || here, not &&.
> + /* The prefix has is already in the name */
> + return;
> +
> + new_length = name_length + PREFIX_SIZE;
Stylistic note, but 'PREFIX_SIZE + name_length' would match the order of
the string that gets prepended.
Kind regards,
Peter
> + new_name = kzalloc(new_length, GFP_KERNEL);
> + if (!new_name)
> + return;
> +
> + snprintf(new_name, new_length, "Logitech %s", *name);
> +
> + kfree(*name);
> +
> + *name = new_name;
> +}
> +
> /* -------------------------------------------------------------------------- */
> /* HIDP++ 1.0 commands */
> /* -------------------------------------------------------------------------- */
> @@ -318,6 +345,10 @@ static char *hidpp_get_unifying_name(struct hidpp_device *hidpp_dev)
> return NULL;
>
> memcpy(name, &response.rap.params[2], len);
> +
> + /* include the terminating '\0' */
> + hidpp_prefix_name(&name, len + 1);
> +
> return name;
> }
>
> @@ -489,6 +520,9 @@ static char *hidpp_get_device_name(struct hidpp_device *hidpp)
> feature_index, index, name + index,
> __name_length - index);
>
> + /* include the terminating '\0' */
> + hidpp_prefix_name(&name, __name_length + 1);
> +
> return name;
>
> out_err:
>
^ permalink raw reply
* Re: [PATCH v2 3/3] HID: wacom: Report input events for each finger on generic devices
From: Jason Gerecke @ 2014-12-10 23:01 UTC (permalink / raw)
To: Benjamin Tissoires; +Cc: Jiri Kosina, linux-input, Ping Cheng
In-Reply-To: <CAN+gG=Et-WHZcgLT2GRRJ8GKDt3E6D1yEwezotrJ=Bzd+dhMdg@mail.gmail.com>
On Wed, Dec 10, 2014 at 12:25 PM, Benjamin Tissoires
<benjamin.tissoires@gmail.com> wrote:
> On Fri, Dec 5, 2014 at 4:37 PM, Jason Gerecke <killertofu@gmail.com> wrote:
>> The existing generic touch code only reports events after reading an
>> entire HID report, which practically means that only data about the last
>> contact in a report will ever be provided to userspace. This patch uses
>> a trick from hid-multitouch.c to discover what type of field is at the
>> end of each contact; when such a field is encountered all the stored
>> contact data will be reported.
>>
>> Signed-off-by: Jason Gerecke <killertofu@gmail.com>
>> ---
>
> Thanks for the v2 Jason.
>
> I think we are closes to it. I have some comments on the style, but
> now that I have read it more carefully, I have a better idea of it,
> and the patch is not so scary.
>
> Jiri. If you reviewed it and took it, that's fine. If not, then I
> think there is still room for improvements.
>
I'm really hoping to get this in for 3.19 if its possible, so I'm
going to try to get this turned-around today...
> Cheers,
> Benjamin
>
>> Changes from v1:
>> * Patch renamed from "HID: wacom: Report input events immediately upon receipt"
>> * Instead of reporting on-reciept, events are bundled and sent at the end of each contact
>> * Adds code to discover 'last_slot_field' based on hid-multitouch.c implementation
>>
>> drivers/hid/wacom_wac.c | 94 ++++++++++++++++++++++++++++++++-----------------
>> drivers/hid/wacom_wac.h | 1 +
>> 2 files changed, 63 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
>> index c33f889..75fc16e 100644
>> --- a/drivers/hid/wacom_wac.c
>> +++ b/drivers/hid/wacom_wac.c
>> @@ -1381,35 +1381,68 @@ static void wacom_wac_finger_usage_mapping(struct hid_device *hdev,
>> {
>> struct wacom *wacom = hid_get_drvdata(hdev);
>> struct wacom_wac *wacom_wac = &wacom->wacom_wac;
>> - unsigned touch_max = wacom_wac->features.touch_max;
>
> not sure we really need to remove the temporary variable touch_max here.
>
>> + struct wacom_features *features = &wacom_wac->features;
>>
>> switch (usage->hid) {
>> case HID_GD_X:
>> - if (touch_max == 1)
>> + features->last_slot_field = usage->hid;
>> + if (features->touch_max == 1)
>
> these changes (touchmax => features->touch_max) hinder a little bit
> the actual change, that is worrisome :)
>
ACK.
>> wacom_map_usage(wacom, usage, field, EV_ABS, ABS_X, 4);
>> else
>> wacom_map_usage(wacom, usage, field, EV_ABS,
>> ABS_MT_POSITION_X, 4);
>> break;
>> case HID_GD_Y:
>> - if (touch_max == 1)
>> + features->last_slot_field = usage->hid;
>> + if (features->touch_max == 1)
>> wacom_map_usage(wacom, usage, field, EV_ABS, ABS_Y, 4);
>> else
>> wacom_map_usage(wacom, usage, field, EV_ABS,
>> ABS_MT_POSITION_Y, 4);
>> break;
>> case HID_DG_CONTACTID:
>> + features->last_slot_field = usage->hid;
>> break;
>> case HID_DG_INRANGE:
>> + features->last_slot_field = usage->hid;
>> break;
>> case HID_DG_INVERT:
>> + features->last_slot_field = usage->hid;
>> break;
>> case HID_DG_TIPSWITCH:
>> + features->last_slot_field = usage->hid;
>> wacom_map_usage(wacom, usage, field, EV_KEY, BTN_TOUCH, 0);
>> break;
>> }
>> }
>>
>> +static void wacom_wac_finger_slot(struct wacom_wac *wacom_wac,
>> + struct input_dev *input)
>> +{
>> + struct hid_data *hid_data = &wacom_wac->hid_data;
>> + bool mt = wacom_wac->features.touch_max > 1;
>> + bool prox = hid_data->tipswitch &&
>> + !wacom_wac->shared->stylus_in_proximity;
>> +
>> + if (mt) {
>
> Personal taste. I don't like the "if (mt)" approach in this patch. I
> preferred the old approach:
> if (mt)
> wacom_wac_finger_mt_slot()
> else
> wacom_wac_finger_touch()
>
> i.e. one function per case.
>
I don't really like the idea of breaking this into two functions. The
differences between how a each MT contact is reported and how the sole
ST contact is reported are trivial, and I feel that breaking them
apart leads to unnecessary duplication. I'll split it out if you feel
that is warranted though.
>> + int slot;
>> +
>> + slot = input_mt_get_slot_by_key(input, hid_data->id);
>> + input_mt_slot(input, slot);
>> + input_mt_report_slot_state(input, MT_TOOL_FINGER, prox);
>> + }
>> + else {
>> + input_report_key(input, BTN_TOUCH, prox);
>> + }
>> +
>> + if (prox) {
>> + input_report_abs(input, mt ? ABS_MT_POSITION_X : ABS_X,
>> + hid_data->x);
>> + input_report_abs(input, mt ? ABS_MT_POSITION_Y : ABS_Y,
>> + hid_data->y);
>> + }
>> +}
>> +
>> static int wacom_wac_finger_event(struct hid_device *hdev,
>> struct hid_field *field, struct hid_usage *usage, __s32 value)
>> {
>> @@ -1432,36 +1465,35 @@ static int wacom_wac_finger_event(struct hid_device *hdev,
>> }
>>
>>
>> + if (usage->usage_index + 1 == field->report_count) {
>> + if (usage->hid == wacom_wac->features.last_slot_field)
>
> OK, I think it would be fair to assume that the FW guys will send N
> full touches by report. IIRC, I had the case where sometimes they add
> some random axis in the end (like X or Y), just to make me cry :)
> should be good here though (and we can have access to the FW guys if
> they do something too much funny :-P )
>
That explains why the hid-multitouch code seemed so much more
convoluted than necessary :) Our FW team hasn't made any descriptors
which are that pathological, and I certainly hope they don't start.
>> + wacom_wac_finger_slot(wacom_wac, wacom_wac->input);
>> + }
>> +
>> return 0;
>> }
>>
>> -static void wacom_wac_finger_mt_report(struct wacom_wac *wacom_wac,
>> - struct input_dev *input, bool touch)
>> +static int wacom_wac_finger_touches(struct hid_device *hdev)
>
> It took me a while to understand what this function was. It simply
> returns the number of touches that has been recorded in the report (I
> was mislead by the git diff I think).
Almost correct -- it returns the number of touches that are currently
active (which may be larger than the number of touches recorded in the
report).
> We should rename this in something else
> (wacom_wac_finger_get_touches_count or something better - I am bad at
> naming things).
>
How about 'wacom_wac_finger_count_touches'?
> Second question, why the need to return the touch count?
>
This function is used only to update the (boolean) value of
'wacom_wac->shared->touch_down'. Strictly speaking it could save a
small amount of processing time and return a boolean directly, but it
seemed trivial enough to just return the count in case it was useful
in the future.
If the question was more of "why do we need to bother going through
the slot state?" then the answer is that we can't rely on the
information in any single report. Many of our MT devices use what
Microsoft calls "hybrid mode", so the complete sensor state may span
multiple reports. All the fingers in one report may go up even while
other fingers remain on the tablet.
>> {
>> - int slot;
>> - struct hid_data *hid_data = &wacom_wac->hid_data;
>> + struct wacom *wacom = hid_get_drvdata(hdev);
>> + struct wacom_wac *wacom_wac = &wacom->wacom_wac;
>> + struct input_dev *input = wacom_wac->input;
>> + unsigned touch_max = wacom_wac->features.touch_max;
>> + int count = 0;
>> + int i;
>>
>> - slot = input_mt_get_slot_by_key(input, hid_data->id);
>> + if (touch_max == 1)
>> + return wacom_wac->hid_data.tipswitch &&
>> + !wacom_wac->shared->stylus_in_proximity;
>>
>> - input_mt_slot(input, slot);
>> - input_mt_report_slot_state(input, MT_TOOL_FINGER, touch);
>> - if (touch) {
>> - input_report_abs(input, ABS_MT_POSITION_X, hid_data->x);
>> - input_report_abs(input, ABS_MT_POSITION_Y, hid_data->y);
>> + for (i = 0; i < input->mt->num_slots; i++) {
>> + struct input_mt_slot *ps = &input->mt->slots[i];
>> + int id = input_mt_get_value(ps, ABS_MT_TRACKING_ID);
>> + if (id >= 0)
>> + count++;
>> }
>> - input_mt_sync_frame(input);
>> -}
>> -
>> -static void wacom_wac_finger_single_touch_report(struct wacom_wac *wacom_wac,
>> - struct input_dev *input, bool touch)
>> -{
>> - struct hid_data *hid_data = &wacom_wac->hid_data;
>>
>> - if (touch) {
>> - input_report_abs(input, ABS_X, hid_data->x);
>> - input_report_abs(input, ABS_Y, hid_data->y);
>> - }
>> - input_report_key(input, BTN_TOUCH, touch);
>> + return count;
>> }
>>
>> static void wacom_wac_finger_report(struct hid_device *hdev,
>> @@ -1470,18 +1502,16 @@ static void wacom_wac_finger_report(struct hid_device *hdev,
>> struct wacom *wacom = hid_get_drvdata(hdev);
>> struct wacom_wac *wacom_wac = &wacom->wacom_wac;
>> struct input_dev *input = wacom_wac->input;
>> - bool touch = wacom_wac->hid_data.tipswitch &&
>> - !wacom_wac->shared->stylus_in_proximity;
>> - unsigned touch_max = wacom_wac->features.touch_max;
>>
>> - if (touch_max > 1)
>> - wacom_wac_finger_mt_report(wacom_wac, input, touch);
>> + if (wacom_wac->features.touch_max > 1)
>
> I think removing the touch_max variable hides a little bit the changes too.
>
>> + input_mt_sync_frame(input);
>> else
>> - wacom_wac_finger_single_touch_report(wacom_wac, input, touch);
>> + wacom_wac_finger_slot(wacom_wac, input);
>
> Not sure there is a need to call wacom_wac_finger_slot() for single
> touch devices. Your current implementation in wacom_finger_event
> should match also single finger tablets.
>
I'll double-check to make sure. I hadn't considered that even the ST
case might match in 'wacom_finger_event'.
>> +
>> input_sync(input);
>>
>> /* keep touch state for pen event */
>> - wacom_wac->shared->touch_down = touch;
>> + wacom_wac->shared->touch_down = wacom_wac_finger_touches(hdev);
>> }
>>
>> #define WACOM_PEN_FIELD(f) (((f)->logical == HID_DG_STYLUS) || \
>> diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h
>> index 128cbb3..6e256206 100644
>> --- a/drivers/hid/wacom_wac.h
>> +++ b/drivers/hid/wacom_wac.h
>> @@ -144,6 +144,7 @@ struct wacom_features {
>> int pktlen;
>> bool check_for_hid_type;
>> int hid_type;
>> + int last_slot_field;
>> };
>>
>> struct wacom_shared {
>> --
>> 2.1.3
>>
^ permalink raw reply
* Re: [PATCH 1/2] HID: logitech-hidpp: do not return the name length
From: Benjamin Tissoires @ 2014-12-10 23:01 UTC (permalink / raw)
To: Peter Wu
Cc: Jiri Kosina, Nestor Lopez Casado, Peter Hutterer, linux-input,
linux-kernel
In-Reply-To: <4561294.qneDZ51mgu@al>
On Dec 10 2014 or thereabouts, Peter Wu wrote:
> On Wednesday 10 December 2014 17:21:09 Benjamin Tissoires wrote:
> > We do not make any use of the actual name length get through
> > hidpp_get_device_name().
> >
> > We can drop the extra code and simplify the API a bit.
> >
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> >
> > ---
> > drivers/hid/hid-logitech-hidpp.c | 10 +++-------
> > 1 file changed, 3 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> > index 1a6395d..3846305 100644
> > --- a/drivers/hid/hid-logitech-hidpp.c
> > +++ b/drivers/hid/hid-logitech-hidpp.c
> > @@ -461,7 +461,7 @@ static int hidpp_devicenametype_get_device_name(struct hidpp_device *hidpp,
> > return count;
> > }
> >
> > -static char *hidpp_get_device_name(struct hidpp_device *hidpp, u8 *name_length)
> > +static char *hidpp_get_device_name(struct hidpp_device *hidpp)
> > {
> > u8 feature_type;
> > u8 feature_index;
> > @@ -484,7 +484,6 @@ static char *hidpp_get_device_name(struct hidpp_device *hidpp, u8 *name_length)
> > if (!name)
> > goto out_err;
> >
> > - *name_length = __name_length + 1;
> > while (index < __name_length)
> > index += hidpp_devicenametype_get_device_name(hidpp,
> > feature_index, index, name + index,
>
> hidpp_devicenametype_get_device_name can return a non-positive value if
> the USB device is unplugged at the wrong time, or if a malicious device
> is attached (and 0 is returned). An infinite loop is the result.
Oh, yes, you are definitively right.
>
> Can you apply this change in the patch or should I send you a separate
> one?
Feel free to send your patch directly to the list.
>
> while (index < __name_length) {
> ret = hidpp_devicenametype_get_device_name(hidpp,
> feature_index, index, name + index,
> __name_length - index);
> if (ret <= 0) {
> kfree(name);
> return NULL;
> }
> index += ret;
> }
>
> > @@ -493,7 +492,6 @@ static char *hidpp_get_device_name(struct hidpp_device *hidpp, u8 *name_length)
> > return name;
> >
> > out_err:
> > - *name_length = 0;
> > return NULL;
>
> What about dropping the label out_err here and returning NULL in the
> previous places?
Yep, good idea. You can either send a patch for that if you want to take
credits, or I can send a v2. It's up to you.
Cheers,
Benjamin
>
> > }
> >
> > @@ -989,7 +987,6 @@ static void hidpp_overwrite_name(struct hid_device *hdev, bool use_unifying)
> > {
> > struct hidpp_device *hidpp = hid_get_drvdata(hdev);
> > char *name;
> > - u8 name_length;
> >
> > if (use_unifying)
> > /*
> > @@ -999,7 +996,7 @@ static void hidpp_overwrite_name(struct hid_device *hdev, bool use_unifying)
> > */
> > name = hidpp_get_unifying_name(hidpp);
> > else
> > - name = hidpp_get_device_name(hidpp, &name_length);
> > + name = hidpp_get_device_name(hidpp);
> >
> > if (!name)
> > hid_err(hdev, "unable to retrieve the name of the device");
> > @@ -1053,7 +1050,6 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
> > bool connected = atomic_read(&hidpp->connected);
> > struct input_dev *input;
> > char *name, *devm_name;
> > - u8 name_length;
> >
> > if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)
> > wtp_connect(hdev, connected);
> > @@ -1080,7 +1076,7 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
> > return;
> > }
> >
> > - name = hidpp_get_device_name(hidpp, &name_length);
> > + name = hidpp_get_device_name(hidpp);
> > if (!name) {
> > hid_err(hdev, "unable to retrieve the name of the device");
> > } else {
> >
>
^ permalink raw reply
* Re: [PATCH 2/2] HID: logitech-hidpp: prefix the name with Logitech
From: Benjamin Tissoires @ 2014-12-10 23:17 UTC (permalink / raw)
To: Peter Wu
Cc: Jiri Kosina, Nestor Lopez Casado, Peter Hutterer, linux-input,
linux-kernel
In-Reply-To: <1511661.CjWHljXQbp@al>
On Dec 11 2014 or thereabouts, Peter Wu wrote:
> On Wednesday 10 December 2014 17:21:10 Benjamin Tissoires wrote:
> > Current names are reported as "K750", "M705", and it can be misleading
> > for the users when they look at their input device list.
> >
> > Prefixing the names with "Logitech " makes things better.
>
> Doesn't this apply to all input devices? Like, every USB device can be
> queried for the manufacturer which could then by included by the input
> subsystem.
Yes and no. Yes, it's good to have the manufacturer name in the input
subsystem, and no, because usbhid already prepend the name with the
manufacturer.
>
> What about duplicate names? Can this patch cause a conflict with devices
> having the same name?
I am not sure what you mean here. I am adding a prefix to the name, so I
do not see how a conflict can be possible. If there were a "M705" and a
"Logitech M705", with different wireless PID, that would be worrisome to
some extend.
But I am pretty sure this will not happen.
>
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > ---
> >
> > Hi Jiri,
> >
> > I'd love to see this one in 3.19 (after a strong review, of course).
> > The idea came with the mouse DPI database that Peter is currently working on
> > (see http://who-t.blogspot.com.au/2014/12/building-a-dpi-database-for-mice.html).
> >
> > I think, if you do not qualify the series for 3.19, we should drop it entirely.
> > 3.19 introduces the hidpp module and changes the labels. The DPI hwdb will check
> > on the label to match the actual mouse, so if this one does not get into 3.19,
> > we will end up in 3 entries for each Logitech device.
> >
> > Cheers,
> > Benjamin
> >
> >
> > drivers/hid/hid-logitech-hidpp.c | 34 ++++++++++++++++++++++++++++++++++
> > 1 file changed, 34 insertions(+)
> >
> > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> > index 3846305..8cf4352 100644
> > --- a/drivers/hid/hid-logitech-hidpp.c
> > +++ b/drivers/hid/hid-logitech-hidpp.c
> > @@ -282,6 +282,33 @@ static inline bool hidpp_report_is_connect_event(struct hidpp_report *report)
> > (report->rap.sub_id == 0x41);
> > }
> >
> > +/**
> > + * hidpp_prefix_name() prefixes the current given name with "Logitech ".
> > + */
> > +static void hidpp_prefix_name(char **name, int name_length)
> > +{
> > +#define PREFIX_SIZE 9 /* "Logitech " */
> > +
> > + int new_length;
> > + char *new_name;
> > +
> > + if (name_length > PREFIX_SIZE &&
> > + strncmp(*name, "Logitech ", PREFIX_SIZE) == 0)
>
> I think you meant || here, not &&.
No, I meant &&. The idea is to not add a prefix if the provided name
already contains the prefix. Read that as "if the size of the name may
contain the prefix and that the prefix is here, then we bail out".
>
> > + /* The prefix has is already in the name */
> > + return;
> > +
> > + new_length = name_length + PREFIX_SIZE;
>
> Stylistic note, but 'PREFIX_SIZE + name_length' would match the order of
> the string that gets prepended.
Works for me.
I will send a v2 with your comments addressed.
Cheers,
Benjamin
>
> Kind regards,
> Peter
>
> > + new_name = kzalloc(new_length, GFP_KERNEL);
> > + if (!new_name)
> > + return;
> > +
> > + snprintf(new_name, new_length, "Logitech %s", *name);
> > +
> > + kfree(*name);
> > +
> > + *name = new_name;
> > +}
> > +
> > /* -------------------------------------------------------------------------- */
> > /* HIDP++ 1.0 commands */
> > /* -------------------------------------------------------------------------- */
> > @@ -318,6 +345,10 @@ static char *hidpp_get_unifying_name(struct hidpp_device *hidpp_dev)
> > return NULL;
> >
> > memcpy(name, &response.rap.params[2], len);
> > +
> > + /* include the terminating '\0' */
> > + hidpp_prefix_name(&name, len + 1);
> > +
> > return name;
> > }
> >
> > @@ -489,6 +520,9 @@ static char *hidpp_get_device_name(struct hidpp_device *hidpp)
> > feature_index, index, name + index,
> > __name_length - index);
> >
> > + /* include the terminating '\0' */
> > + hidpp_prefix_name(&name, __name_length + 1);
> > +
> > return name;
> >
> > out_err:
> >
>
^ permalink raw reply
* Re: [PATCH v2 3/3] HID: wacom: Report input events for each finger on generic devices
From: Benjamin Tissoires @ 2014-12-10 23:27 UTC (permalink / raw)
To: Jason Gerecke; +Cc: Jiri Kosina, linux-input, Ping Cheng
In-Reply-To: <CANRwn3Sc_YkeGz-MzJtysULOCg_9zg3A0iWgvTDER=L83BCKYA@mail.gmail.com>
On Wed, Dec 10, 2014 at 6:01 PM, Jason Gerecke <killertofu@gmail.com> wrote:
> On Wed, Dec 10, 2014 at 12:25 PM, Benjamin Tissoires
> <benjamin.tissoires@gmail.com> wrote:
>> On Fri, Dec 5, 2014 at 4:37 PM, Jason Gerecke <killertofu@gmail.com> wrote:
>>> The existing generic touch code only reports events after reading an
>>> entire HID report, which practically means that only data about the last
>>> contact in a report will ever be provided to userspace. This patch uses
>>> a trick from hid-multitouch.c to discover what type of field is at the
>>> end of each contact; when such a field is encountered all the stored
>>> contact data will be reported.
>>>
>>> Signed-off-by: Jason Gerecke <killertofu@gmail.com>
>>> ---
>>
>> Thanks for the v2 Jason.
>>
>> I think we are closes to it. I have some comments on the style, but
>> now that I have read it more carefully, I have a better idea of it,
>> and the patch is not so scary.
>>
>> Jiri. If you reviewed it and took it, that's fine. If not, then I
>> think there is still room for improvements.
>>
>
> I'm really hoping to get this in for 3.19 if its possible, so I'm
> going to try to get this turned-around today...
>
>> Cheers,
>> Benjamin
>>
>>> Changes from v1:
>>> * Patch renamed from "HID: wacom: Report input events immediately upon receipt"
>>> * Instead of reporting on-reciept, events are bundled and sent at the end of each contact
>>> * Adds code to discover 'last_slot_field' based on hid-multitouch.c implementation
>>>
>>> drivers/hid/wacom_wac.c | 94 ++++++++++++++++++++++++++++++++-----------------
>>> drivers/hid/wacom_wac.h | 1 +
>>> 2 files changed, 63 insertions(+), 32 deletions(-)
>>>
>>> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
>>> index c33f889..75fc16e 100644
>>> --- a/drivers/hid/wacom_wac.c
>>> +++ b/drivers/hid/wacom_wac.c
>>> @@ -1381,35 +1381,68 @@ static void wacom_wac_finger_usage_mapping(struct hid_device *hdev,
>>> {
>>> struct wacom *wacom = hid_get_drvdata(hdev);
>>> struct wacom_wac *wacom_wac = &wacom->wacom_wac;
>>> - unsigned touch_max = wacom_wac->features.touch_max;
>>
>> not sure we really need to remove the temporary variable touch_max here.
>>
>>> + struct wacom_features *features = &wacom_wac->features;
>>>
>>> switch (usage->hid) {
>>> case HID_GD_X:
>>> - if (touch_max == 1)
>>> + features->last_slot_field = usage->hid;
>>> + if (features->touch_max == 1)
>>
>> these changes (touchmax => features->touch_max) hinder a little bit
>> the actual change, that is worrisome :)
>>
> ACK.
>
>>> wacom_map_usage(wacom, usage, field, EV_ABS, ABS_X, 4);
>>> else
>>> wacom_map_usage(wacom, usage, field, EV_ABS,
>>> ABS_MT_POSITION_X, 4);
>>> break;
>>> case HID_GD_Y:
>>> - if (touch_max == 1)
>>> + features->last_slot_field = usage->hid;
>>> + if (features->touch_max == 1)
>>> wacom_map_usage(wacom, usage, field, EV_ABS, ABS_Y, 4);
>>> else
>>> wacom_map_usage(wacom, usage, field, EV_ABS,
>>> ABS_MT_POSITION_Y, 4);
>>> break;
>>> case HID_DG_CONTACTID:
>>> + features->last_slot_field = usage->hid;
>>> break;
>>> case HID_DG_INRANGE:
>>> + features->last_slot_field = usage->hid;
>>> break;
>>> case HID_DG_INVERT:
>>> + features->last_slot_field = usage->hid;
>>> break;
>>> case HID_DG_TIPSWITCH:
>>> + features->last_slot_field = usage->hid;
>>> wacom_map_usage(wacom, usage, field, EV_KEY, BTN_TOUCH, 0);
>>> break;
>>> }
>>> }
>>>
>>> +static void wacom_wac_finger_slot(struct wacom_wac *wacom_wac,
>>> + struct input_dev *input)
>>> +{
>>> + struct hid_data *hid_data = &wacom_wac->hid_data;
>>> + bool mt = wacom_wac->features.touch_max > 1;
>>> + bool prox = hid_data->tipswitch &&
>>> + !wacom_wac->shared->stylus_in_proximity;
>>> +
>>> + if (mt) {
>>
>> Personal taste. I don't like the "if (mt)" approach in this patch. I
>> preferred the old approach:
>> if (mt)
>> wacom_wac_finger_mt_slot()
>> else
>> wacom_wac_finger_touch()
>>
>> i.e. one function per case.
>>
> I don't really like the idea of breaking this into two functions. The
> differences between how a each MT contact is reported and how the sole
> ST contact is reported are trivial, and I feel that breaking them
> apart leads to unnecessary duplication. I'll split it out if you feel
> that is warranted though.
No, do whatever you like. It was more a comment I added before reading
the entire patch, so after having the global view, it's fine this way
too.
>
>>> + int slot;
>>> +
>>> + slot = input_mt_get_slot_by_key(input, hid_data->id);
>>> + input_mt_slot(input, slot);
>>> + input_mt_report_slot_state(input, MT_TOOL_FINGER, prox);
>>> + }
>>> + else {
>>> + input_report_key(input, BTN_TOUCH, prox);
>>> + }
>>> +
>>> + if (prox) {
>>> + input_report_abs(input, mt ? ABS_MT_POSITION_X : ABS_X,
>>> + hid_data->x);
>>> + input_report_abs(input, mt ? ABS_MT_POSITION_Y : ABS_Y,
>>> + hid_data->y);
>>> + }
>>> +}
>>> +
>>> static int wacom_wac_finger_event(struct hid_device *hdev,
>>> struct hid_field *field, struct hid_usage *usage, __s32 value)
>>> {
>>> @@ -1432,36 +1465,35 @@ static int wacom_wac_finger_event(struct hid_device *hdev,
>>> }
>>>
>>>
>>> + if (usage->usage_index + 1 == field->report_count) {
>>> + if (usage->hid == wacom_wac->features.last_slot_field)
>>
>> OK, I think it would be fair to assume that the FW guys will send N
>> full touches by report. IIRC, I had the case where sometimes they add
>> some random axis in the end (like X or Y), just to make me cry :)
>> should be good here though (and we can have access to the FW guys if
>> they do something too much funny :-P )
>>
> That explains why the hid-multitouch code seemed so much more
> convoluted than necessary :) Our FW team hasn't made any descriptors
> which are that pathological, and I certainly hope they don't start.
Actually, I may not be fair with the FW guys. I think the problem we
had was that while we were parsing the multitouch report and then the
mouse emulation report, we ended up in having ABS_Y as the last slot
field. Either we (I) were (was) not smart enough to check on the type
of the report, or either it was difficult to detect in input_mapping
which collection was the mouse, and which was the touch (and this is
when the FW guys go nuts and do anything in the report descriptor :-P
)
>
>>> + wacom_wac_finger_slot(wacom_wac, wacom_wac->input);
>>> + }
>>> +
>>> return 0;
>>> }
>>>
>>> -static void wacom_wac_finger_mt_report(struct wacom_wac *wacom_wac,
>>> - struct input_dev *input, bool touch)
>>> +static int wacom_wac_finger_touches(struct hid_device *hdev)
>>
>> It took me a while to understand what this function was. It simply
>> returns the number of touches that has been recorded in the report (I
>> was mislead by the git diff I think).
> Almost correct -- it returns the number of touches that are currently
> active (which may be larger than the number of touches recorded in the
> report).
Yeah, that's what I meant, you understood me :)
>
>> We should rename this in something else
>> (wacom_wac_finger_get_touches_count or something better - I am bad at
>> naming things).
>>
> How about 'wacom_wac_finger_count_touches'?
works for me.
>
>> Second question, why the need to return the touch count?
>>
> This function is used only to update the (boolean) value of
> 'wacom_wac->shared->touch_down'. Strictly speaking it could save a
> small amount of processing time and return a boolean directly, but it
> seemed trivial enough to just return the count in case it was useful
> in the future.
>
> If the question was more of "why do we need to bother going through
> the slot state?" then the answer is that we can't rely on the
> information in any single report. Many of our MT devices use what
> Microsoft calls "hybrid mode", so the complete sensor state may span
> multiple reports. All the fingers in one report may go up even while
> other fingers remain on the tablet.
I like when you are able to decrypt my mind :)
ok, fine.
We could still abort earlier in the function to report a boolean, but
the finger count is fine too.
>
>>> {
>>> - int slot;
>>> - struct hid_data *hid_data = &wacom_wac->hid_data;
>>> + struct wacom *wacom = hid_get_drvdata(hdev);
>>> + struct wacom_wac *wacom_wac = &wacom->wacom_wac;
>>> + struct input_dev *input = wacom_wac->input;
>>> + unsigned touch_max = wacom_wac->features.touch_max;
>>> + int count = 0;
>>> + int i;
>>>
>>> - slot = input_mt_get_slot_by_key(input, hid_data->id);
>>> + if (touch_max == 1)
>>> + return wacom_wac->hid_data.tipswitch &&
>>> + !wacom_wac->shared->stylus_in_proximity;
>>>
>>> - input_mt_slot(input, slot);
>>> - input_mt_report_slot_state(input, MT_TOOL_FINGER, touch);
>>> - if (touch) {
>>> - input_report_abs(input, ABS_MT_POSITION_X, hid_data->x);
>>> - input_report_abs(input, ABS_MT_POSITION_Y, hid_data->y);
>>> + for (i = 0; i < input->mt->num_slots; i++) {
>>> + struct input_mt_slot *ps = &input->mt->slots[i];
>>> + int id = input_mt_get_value(ps, ABS_MT_TRACKING_ID);
>>> + if (id >= 0)
>>> + count++;
>>> }
>>> - input_mt_sync_frame(input);
>>> -}
>>> -
>>> -static void wacom_wac_finger_single_touch_report(struct wacom_wac *wacom_wac,
>>> - struct input_dev *input, bool touch)
>>> -{
>>> - struct hid_data *hid_data = &wacom_wac->hid_data;
>>>
>>> - if (touch) {
>>> - input_report_abs(input, ABS_X, hid_data->x);
>>> - input_report_abs(input, ABS_Y, hid_data->y);
>>> - }
>>> - input_report_key(input, BTN_TOUCH, touch);
>>> + return count;
>>> }
>>>
>>> static void wacom_wac_finger_report(struct hid_device *hdev,
>>> @@ -1470,18 +1502,16 @@ static void wacom_wac_finger_report(struct hid_device *hdev,
>>> struct wacom *wacom = hid_get_drvdata(hdev);
>>> struct wacom_wac *wacom_wac = &wacom->wacom_wac;
>>> struct input_dev *input = wacom_wac->input;
>>> - bool touch = wacom_wac->hid_data.tipswitch &&
>>> - !wacom_wac->shared->stylus_in_proximity;
>>> - unsigned touch_max = wacom_wac->features.touch_max;
>>>
>>> - if (touch_max > 1)
>>> - wacom_wac_finger_mt_report(wacom_wac, input, touch);
>>> + if (wacom_wac->features.touch_max > 1)
>>
>> I think removing the touch_max variable hides a little bit the changes too.
>>
>>> + input_mt_sync_frame(input);
>>> else
>>> - wacom_wac_finger_single_touch_report(wacom_wac, input, touch);
>>> + wacom_wac_finger_slot(wacom_wac, input);
>>
>> Not sure there is a need to call wacom_wac_finger_slot() for single
>> touch devices. Your current implementation in wacom_finger_event
>> should match also single finger tablets.
>>
> I'll double-check to make sure. I hadn't considered that even the ST
> case might match in 'wacom_finger_event'.
thanks
>
>>> +
>>> input_sync(input);
>>>
>>> /* keep touch state for pen event */
>>> - wacom_wac->shared->touch_down = touch;
>>> + wacom_wac->shared->touch_down = wacom_wac_finger_touches(hdev);
>>> }
>>>
>>> #define WACOM_PEN_FIELD(f) (((f)->logical == HID_DG_STYLUS) || \
>>> diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h
>>> index 128cbb3..6e256206 100644
>>> --- a/drivers/hid/wacom_wac.h
>>> +++ b/drivers/hid/wacom_wac.h
>>> @@ -144,6 +144,7 @@ struct wacom_features {
>>> int pktlen;
>>> bool check_for_hid_type;
>>> int hid_type;
>>> + int last_slot_field;
>>> };
>>>
>>> struct wacom_shared {
>>> --
>>> 2.1.3
>>>
Cheers,
Benjamin
^ permalink raw reply
* Re: [PATCH 1/2] HID: logitech-hidpp: do not return the name length
From: Peter Wu @ 2014-12-11 0:06 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Jiri Kosina, Nestor Lopez Casado, Peter Hutterer, linux-input,
linux-kernel
In-Reply-To: <20141210230152.GC4861@mail.corp.redhat.com>
On Wednesday 10 December 2014 18:01:52 Benjamin Tissoires wrote:
> On Dec 10 2014 or thereabouts, Peter Wu wrote:
> > On Wednesday 10 December 2014 17:21:09 Benjamin Tissoires wrote:
> > > We do not make any use of the actual name length get through
> > > hidpp_get_device_name().
> > >
> > > We can drop the extra code and simplify the API a bit.
> > >
> > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > >
> > > ---
> > > drivers/hid/hid-logitech-hidpp.c | 10 +++-------
> > > 1 file changed, 3 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> > > index 1a6395d..3846305 100644
> > > --- a/drivers/hid/hid-logitech-hidpp.c
> > > +++ b/drivers/hid/hid-logitech-hidpp.c
> > > @@ -461,7 +461,7 @@ static int hidpp_devicenametype_get_device_name(struct hidpp_device *hidpp,
> > > return count;
> > > }
> > >
> > > -static char *hidpp_get_device_name(struct hidpp_device *hidpp, u8 *name_length)
> > > +static char *hidpp_get_device_name(struct hidpp_device *hidpp)
> > > {
> > > u8 feature_type;
> > > u8 feature_index;
> > > @@ -484,7 +484,6 @@ static char *hidpp_get_device_name(struct hidpp_device *hidpp, u8 *name_length)
> > > if (!name)
> > > goto out_err;
> > >
> > > - *name_length = __name_length + 1;
> > > while (index < __name_length)
> > > index += hidpp_devicenametype_get_device_name(hidpp,
> > > feature_index, index, name + index,
> >
> > hidpp_devicenametype_get_device_name can return a non-positive value if
> > the USB device is unplugged at the wrong time, or if a malicious device
> > is attached (and 0 is returned). An infinite loop is the result.
>
> Oh, yes, you are definitively right.
>
> >
> > Can you apply this change in the patch or should I send you a separate
> > one?
>
> Feel free to send your patch directly to the list.
>
> >
> > while (index < __name_length) {
> > ret = hidpp_devicenametype_get_device_name(hidpp,
> > feature_index, index, name + index,
> > __name_length - index);
> > if (ret <= 0) {
> > kfree(name);
> > return NULL;
> > }
> > index += ret;
> > }
> >
> > > @@ -493,7 +492,6 @@ static char *hidpp_get_device_name(struct hidpp_device *hidpp, u8 *name_length)
> > > return name;
> > >
> > > out_err:
> > > - *name_length = 0;
> > > return NULL;
> >
> > What about dropping the label out_err here and returning NULL in the
> > previous places?
>
> Yep, good idea. You can either send a patch for that if you want to take
> credits, or I can send a v2. It's up to you.
>
> Cheers,
> Benjamin
I found another place where a boundary check is missing, but will treat
that in a separate patch. If you send a v2 with the return NULL fixed
up, I'll base my patch (to add a retval check) on yours.
Kind regards,
Peter
> >
> > > }
> > >
> > > @@ -989,7 +987,6 @@ static void hidpp_overwrite_name(struct hid_device *hdev, bool use_unifying)
> > > {
> > > struct hidpp_device *hidpp = hid_get_drvdata(hdev);
> > > char *name;
> > > - u8 name_length;
> > >
> > > if (use_unifying)
> > > /*
> > > @@ -999,7 +996,7 @@ static void hidpp_overwrite_name(struct hid_device *hdev, bool use_unifying)
> > > */
> > > name = hidpp_get_unifying_name(hidpp);
> > > else
> > > - name = hidpp_get_device_name(hidpp, &name_length);
> > > + name = hidpp_get_device_name(hidpp);
> > >
> > > if (!name)
> > > hid_err(hdev, "unable to retrieve the name of the device");
> > > @@ -1053,7 +1050,6 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
> > > bool connected = atomic_read(&hidpp->connected);
> > > struct input_dev *input;
> > > char *name, *devm_name;
> > > - u8 name_length;
> > >
> > > if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)
> > > wtp_connect(hdev, connected);
> > > @@ -1080,7 +1076,7 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
> > > return;
> > > }
> > >
> > > - name = hidpp_get_device_name(hidpp, &name_length);
> > > + name = hidpp_get_device_name(hidpp);
> > > if (!name) {
> > > hid_err(hdev, "unable to retrieve the name of the device");
> > > } else {
> > >
^ permalink raw reply
* [PATCH v3 3/3] HID: wacom: Report input events for each finger on generic devices
From: Jason Gerecke @ 2014-12-11 0:26 UTC (permalink / raw)
To: Jiri Kosina; +Cc: Benjamin Tissoires, linux-input, Ping Cheng, Jason Gerecke
In-Reply-To: <CAN+gG=Fw0BvNZ8DQtGtuCer42D0wzBzE0NX4rAe+54b94=X7RQ@mail.gmail.com>
The existing generic touch code only reports events after reading an
entire HID report, which practically means that only data about the last
contact in a report will ever be provided to userspace. This patch uses
a trick from hid-multitouch.c to discover what type of field is at the
end of each contact; when such a field is encountered all the stored
contact data will be reported.
Signed-off-by: Jason Gerecke <killertofu@gmail.com>
---
Changes from v2:
* No longer removing 'touch_max' variables to make changes clearer
* Renamed 'wacom_wac_finger_touches' to 'wacom_wac_finger_count_touches'
* Removed call to 'wacom_wac_finger_slot' in 'wacom_wac_finger_report'
since it is already called for the single-touch case within
'wacom_wac_finger_event'.
drivers/hid/wacom_wac.c | 86 +++++++++++++++++++++++++++++++++----------------
drivers/hid/wacom_wac.h | 1 +
2 files changed, 59 insertions(+), 28 deletions(-)
diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index 064fd6c..26f27bd 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -1381,10 +1381,12 @@ static void wacom_wac_finger_usage_mapping(struct hid_device *hdev,
{
struct wacom *wacom = hid_get_drvdata(hdev);
struct wacom_wac *wacom_wac = &wacom->wacom_wac;
+ struct wacom_features *features = &wacom_wac->features;
unsigned touch_max = wacom_wac->features.touch_max;
switch (usage->hid) {
case HID_GD_X:
+ features->last_slot_field = usage->hid;
if (touch_max == 1)
wacom_map_usage(wacom, usage, field, EV_ABS, ABS_X, 4);
else
@@ -1392,6 +1394,7 @@ static void wacom_wac_finger_usage_mapping(struct hid_device *hdev,
ABS_MT_POSITION_X, 4);
break;
case HID_GD_Y:
+ features->last_slot_field = usage->hid;
if (touch_max == 1)
wacom_map_usage(wacom, usage, field, EV_ABS, ABS_Y, 4);
else
@@ -1399,17 +1402,48 @@ static void wacom_wac_finger_usage_mapping(struct hid_device *hdev,
ABS_MT_POSITION_Y, 4);
break;
case HID_DG_CONTACTID:
+ features->last_slot_field = usage->hid;
break;
case HID_DG_INRANGE:
+ features->last_slot_field = usage->hid;
break;
case HID_DG_INVERT:
+ features->last_slot_field = usage->hid;
break;
case HID_DG_TIPSWITCH:
+ features->last_slot_field = usage->hid;
wacom_map_usage(wacom, usage, field, EV_KEY, BTN_TOUCH, 0);
break;
}
}
+static void wacom_wac_finger_slot(struct wacom_wac *wacom_wac,
+ struct input_dev *input)
+{
+ struct hid_data *hid_data = &wacom_wac->hid_data;
+ bool mt = wacom_wac->features.touch_max > 1;
+ bool prox = hid_data->tipswitch &&
+ !wacom_wac->shared->stylus_in_proximity;
+
+ if (mt) {
+ int slot;
+
+ slot = input_mt_get_slot_by_key(input, hid_data->id);
+ input_mt_slot(input, slot);
+ input_mt_report_slot_state(input, MT_TOOL_FINGER, prox);
+ }
+ else {
+ input_report_key(input, BTN_TOUCH, prox);
+ }
+
+ if (prox) {
+ input_report_abs(input, mt ? ABS_MT_POSITION_X : ABS_X,
+ hid_data->x);
+ input_report_abs(input, mt ? ABS_MT_POSITION_Y : ABS_Y,
+ hid_data->y);
+ }
+}
+
static int wacom_wac_finger_event(struct hid_device *hdev,
struct hid_field *field, struct hid_usage *usage, __s32 value)
{
@@ -1432,36 +1466,35 @@ static int wacom_wac_finger_event(struct hid_device *hdev,
}
+ if (usage->usage_index + 1 == field->report_count) {
+ if (usage->hid == wacom_wac->features.last_slot_field)
+ wacom_wac_finger_slot(wacom_wac, wacom_wac->input);
+ }
+
return 0;
}
-static void wacom_wac_finger_mt_report(struct wacom_wac *wacom_wac,
- struct input_dev *input, bool touch)
+static int wacom_wac_finger_count_touches(struct hid_device *hdev)
{
- int slot;
- struct hid_data *hid_data = &wacom_wac->hid_data;
+ struct wacom *wacom = hid_get_drvdata(hdev);
+ struct wacom_wac *wacom_wac = &wacom->wacom_wac;
+ struct input_dev *input = wacom_wac->input;
+ unsigned touch_max = wacom_wac->features.touch_max;
+ int count = 0;
+ int i;
- slot = input_mt_get_slot_by_key(input, hid_data->id);
+ if (touch_max == 1)
+ return wacom_wac->hid_data.tipswitch &&
+ !wacom_wac->shared->stylus_in_proximity;
- input_mt_slot(input, slot);
- input_mt_report_slot_state(input, MT_TOOL_FINGER, touch);
- if (touch) {
- input_report_abs(input, ABS_MT_POSITION_X, hid_data->x);
- input_report_abs(input, ABS_MT_POSITION_Y, hid_data->y);
+ for (i = 0; i < input->mt->num_slots; i++) {
+ struct input_mt_slot *ps = &input->mt->slots[i];
+ int id = input_mt_get_value(ps, ABS_MT_TRACKING_ID);
+ if (id >= 0)
+ count++;
}
- input_mt_sync_frame(input);
-}
-
-static void wacom_wac_finger_single_touch_report(struct wacom_wac *wacom_wac,
- struct input_dev *input, bool touch)
-{
- struct hid_data *hid_data = &wacom_wac->hid_data;
- if (touch) {
- input_report_abs(input, ABS_X, hid_data->x);
- input_report_abs(input, ABS_Y, hid_data->y);
- }
- input_report_key(input, BTN_TOUCH, touch);
+ return count;
}
static void wacom_wac_finger_report(struct hid_device *hdev,
@@ -1470,18 +1503,15 @@ static void wacom_wac_finger_report(struct hid_device *hdev,
struct wacom *wacom = hid_get_drvdata(hdev);
struct wacom_wac *wacom_wac = &wacom->wacom_wac;
struct input_dev *input = wacom_wac->input;
- bool touch = wacom_wac->hid_data.tipswitch &&
- !wacom_wac->shared->stylus_in_proximity;
unsigned touch_max = wacom_wac->features.touch_max;
if (touch_max > 1)
- wacom_wac_finger_mt_report(wacom_wac, input, touch);
- else
- wacom_wac_finger_single_touch_report(wacom_wac, input, touch);
+ input_mt_sync_frame(input);
+
input_sync(input);
/* keep touch state for pen event */
- wacom_wac->shared->touch_down = touch;
+ wacom_wac->shared->touch_down = wacom_wac_finger_count_touches(hdev);
}
#define WACOM_PEN_FIELD(f) (((f)->logical == HID_DG_STYLUS) || \
diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h
index 5384043..bfad815 100644
--- a/drivers/hid/wacom_wac.h
+++ b/drivers/hid/wacom_wac.h
@@ -145,6 +145,7 @@ struct wacom_features {
int pktlen;
bool check_for_hid_type;
int hid_type;
+ int last_slot_field;
};
struct wacom_shared {
--
2.1.3
^ permalink raw reply related
* Re: [PATCH] HID: rmi: Add support for the touchpad in the Razer Blade 14 laptop
From: Andrew Duggan @ 2014-12-11 1:00 UTC (permalink / raw)
To: Benjamin Tissoires; +Cc: linux-input, linux-kernel@vger.kernel.org, Jiri Kosina
In-Reply-To: <CAN+gG=Epx3m0=mg-xfKJveZBvo5ySe5cpFJgRbo79DvKgCFa4A@mail.gmail.com>
Hi Benjamin,
Thanks for reviewing my patch. Comments and questions below.
On 12/10/2014 12:51 PM, Benjamin Tissoires wrote:
> On Mon, Dec 8, 2014 at 7:16 PM, Andrew Duggan <aduggan@synaptics.com> wrote:
>> On 11/24/2014 04:20 PM, Andrew Duggan wrote:
>>> The Razer Blade 14 has a Synaptic's TouchPad on one of the interfaces of
>>> a composite USB device. This patch allows the hid-rmi driver to bind
>>> to that interface. It also adds support for the external click buttons
>>> on the Razer's touchpad. External buttons are reported using generic
>>> mouse reports instead of through the F30 like it is on ClickPads.
>>>
>>> Signed-off-by: Andrew Duggan <aduggan@synaptics.com>
>>> ---
>>> This patch depends on the "HID: rmi: Scan the report descriptor to
>>> determine if the device is
>>> suitable for the hid-rmi driver" I submitted earlier today to correctly
>>> bind to the touchpad HID
>>> device in the composite USB device.
>> Any comments on this patch?
> Again, sorry for the lag on this series. I think now that I re-read
> this one I understood why I did not put too many efforts to properly
> review the series. This one is a little bit worrisome IMO.
>
I wasn't sure about this patch either. But, after waiting a while and
not coming with with anything better I figured I would post it to at
least get some feedback.
>> Thanks,
>> Andrew
>>
>>> drivers/hid/hid-core.c | 4 +++-
>>> drivers/hid/hid-ids.h | 3 +++
>>> drivers/hid/hid-rmi.c | 15 ++++++++++++++-
>>> 3 files changed, 20 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>>> index ba9dc59..d69ea16 100644
>>> --- a/drivers/hid/hid-core.c
>>> +++ b/drivers/hid/hid-core.c
>>> @@ -792,7 +792,9 @@ static int hid_scan_report(struct hid_device *hid)
>>> /*
>>> * Vendor specific handlings
>>> */
>>> - if ((hid->vendor == USB_VENDOR_ID_SYNAPTICS) &&
>>> + if ((hid->vendor == USB_VENDOR_ID_SYNAPTICS
>>> + || (hid->vendor == USB_VENDOR_ID_RAZER
>>> + && hid->product == USB_DEVICE_ID_RAZER_BLADE_14)) &&
> I don't like this. We already have a blacklist, an ignore list, and
> there, we will have a new blacklist...
>
> I understood why you put this here, the current have_special_driver
> list will not fit 100%. Still, I find it not good.
>
> It works, but I think we should still head for an entry in
> have_special_driver, and a specific behavior in hid-rmi which would
> rely on hid-input to handle the keyboard/mouse buttons and the rest.
Are you suggesting that we have hid-rmi bind to all of the hid devices
on this USB device and then have hid-rmi decide what reports it should
process and let the remaining events continue to hid-input? I guess once
hid-rmi loads it could look at the hid report ids and determine if it is
one of our devices and if it should put the device into rmi mode. If not
simply act as a pass through.
>>> (hid->group == HID_GROUP_GENERIC)) {
>>> if ((parser->scan_flags & HID_SCAN_FLAG_VENDOR_SPECIFIC)
>>> && (parser->scan_flags &
>>> HID_SCAN_FLAG_GENDESK_POINTER))
>>> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
>>> index 25cd674..c677aad 100644
>>> --- a/drivers/hid/hid-ids.h
>>> +++ b/drivers/hid/hid-ids.h
>>> @@ -751,6 +751,9 @@
>>> #define USB_DEVICE_ID_QUANTA_OPTICAL_TOUCH_3001 0x3001
>>> #define USB_DEVICE_ID_QUANTA_OPTICAL_TOUCH_3008 0x3008
>>> +#define USB_VENDOR_ID_RAZER 0x1532
>>> +#define USB_DEVICE_ID_RAZER_BLADE_14 0x011D
>>> +
>>> #define USB_VENDOR_ID_REALTEK 0x0bda
>>> #define USB_DEVICE_ID_REALTEK_READER 0x0152
>>> diff --git a/drivers/hid/hid-rmi.c b/drivers/hid/hid-rmi.c
>>> index 3cccff7..1f131df 100644
>>> --- a/drivers/hid/hid-rmi.c
>>> +++ b/drivers/hid/hid-rmi.c
>>> @@ -453,7 +453,15 @@ static int rmi_raw_event(struct hid_device *hdev,
>>> case RMI_ATTN_REPORT_ID:
>>> return rmi_input_event(hdev, data, size);
>>> case RMI_MOUSE_REPORT_ID:
>>> - rmi_schedule_reset(hdev);
>>> + /*
>>> + * touchpads with physical mouse buttons will report those
>>> + * buttons in mouse reports even in RMI mode. Only reset
>>> + * the device if we see reports which contain X or Y data.
>>> + */
>>> + if (data[2] != 0 || data[3] != 0)
> this seems a little bit magical. There may not be an other solution,
> but I would prefer that we look at alternatives before using this
> magical numbers (will the touchpad always use the same report
> descriptor on the mouse interface?)
I did just look at the reports and see where X and Y where reported.
Since mouse mode is really only there for compatibility when no custom
driver is present I think it is unlikely that it will change in the
future. However, there is no guarantee that it won't change. I will see
if I can come up with is less of a hack. Maybe, I can use the event
callback instead of raw_event.
>>> + rmi_schedule_reset(hdev);
>>> + else
>>> + return 1;
>>> break;
>>> }
>>> @@ -871,6 +879,11 @@ static int rmi_input_mapping(struct hid_device
>>> *hdev,
>>> struct hid_input *hi, struct hid_field *field,
>>> struct hid_usage *usage, unsigned long **bit, int *max)
>>> {
>>> + if (field->application == HID_GD_POINTER
>>> + && (usage->hid & HID_USAGE_PAGE) == HID_UP_BUTTON)
>>> + /* Pass mouse button reports to generic code for
>>> processing */
>>> + return 0;
>>> +
>>> /* we want to make HID ignore the advertised HID collection */
>>> return -1;
>>> }
>>
> Cheers,
> Benjamin
Andrew
^ permalink raw reply
* Re: [PATCH] HID: rmi: Add support for the touchpad in the Razer Blade 14 laptop
From: Benjamin Tissoires @ 2014-12-11 1:32 UTC (permalink / raw)
To: Andrew Duggan; +Cc: linux-input, linux-kernel@vger.kernel.org, Jiri Kosina
In-Reply-To: <5488ECC8.8070802@synaptics.com>
On Wed, Dec 10, 2014 at 8:00 PM, Andrew Duggan <aduggan@synaptics.com> wrote:
> Hi Benjamin,
>
> Thanks for reviewing my patch. Comments and questions below.
>
> On 12/10/2014 12:51 PM, Benjamin Tissoires wrote:
>>
>> On Mon, Dec 8, 2014 at 7:16 PM, Andrew Duggan <aduggan@synaptics.com>
>> wrote:
>>>
>>> On 11/24/2014 04:20 PM, Andrew Duggan wrote:
>>>>
>>>> The Razer Blade 14 has a Synaptic's TouchPad on one of the interfaces of
>>>> a composite USB device. This patch allows the hid-rmi driver to bind
>>>> to that interface. It also adds support for the external click buttons
>>>> on the Razer's touchpad. External buttons are reported using generic
>>>> mouse reports instead of through the F30 like it is on ClickPads.
>>>>
>>>> Signed-off-by: Andrew Duggan <aduggan@synaptics.com>
>>>> ---
>>>> This patch depends on the "HID: rmi: Scan the report descriptor to
>>>> determine if the device is
>>>> suitable for the hid-rmi driver" I submitted earlier today to correctly
>>>> bind to the touchpad HID
>>>> device in the composite USB device.
>>>
>>> Any comments on this patch?
>>
>> Again, sorry for the lag on this series. I think now that I re-read
>> this one I understood why I did not put too many efforts to properly
>> review the series. This one is a little bit worrisome IMO.
>>
>
> I wasn't sure about this patch either. But, after waiting a while and not
> coming with with anything better I figured I would post it to at least get
> some feedback.
That's the right spirit :)
>
>
>>> Thanks,
>>> Andrew
>>>
>>>> drivers/hid/hid-core.c | 4 +++-
>>>> drivers/hid/hid-ids.h | 3 +++
>>>> drivers/hid/hid-rmi.c | 15 ++++++++++++++-
>>>> 3 files changed, 20 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>>>> index ba9dc59..d69ea16 100644
>>>> --- a/drivers/hid/hid-core.c
>>>> +++ b/drivers/hid/hid-core.c
>>>> @@ -792,7 +792,9 @@ static int hid_scan_report(struct hid_device *hid)
>>>> /*
>>>> * Vendor specific handlings
>>>> */
>>>> - if ((hid->vendor == USB_VENDOR_ID_SYNAPTICS) &&
>>>> + if ((hid->vendor == USB_VENDOR_ID_SYNAPTICS
>>>> + || (hid->vendor == USB_VENDOR_ID_RAZER
>>>> + && hid->product == USB_DEVICE_ID_RAZER_BLADE_14)) &&
>>
>> I don't like this. We already have a blacklist, an ignore list, and
>> there, we will have a new blacklist...
>>
>> I understood why you put this here, the current have_special_driver
>> list will not fit 100%. Still, I find it not good.
>>
>> It works, but I think we should still head for an entry in
>> have_special_driver, and a specific behavior in hid-rmi which would
>> rely on hid-input to handle the keyboard/mouse buttons and the rest.
>
>
> Are you suggesting that we have hid-rmi bind to all of the hid devices on
> this USB device and then have hid-rmi decide what reports it should process
> and let the remaining events continue to hid-input? I guess once hid-rmi
> loads it could look at the hid report ids and determine if it is one of our
> devices and if it should put the device into rmi mode. If not simply act as
> a pass through.
Yeah, that's basically what I am saying. Add the device in
hid_have_special_driver, add it to the list of supported devices by
hid-rmi, and add a quirk saying that it should be pass through for all
but the rmi report ID.
>
>
>>>> (hid->group == HID_GROUP_GENERIC)) {
>>>> if ((parser->scan_flags &
>>>> HID_SCAN_FLAG_VENDOR_SPECIFIC)
>>>> && (parser->scan_flags &
>>>> HID_SCAN_FLAG_GENDESK_POINTER))
>>>> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
>>>> index 25cd674..c677aad 100644
>>>> --- a/drivers/hid/hid-ids.h
>>>> +++ b/drivers/hid/hid-ids.h
>>>> @@ -751,6 +751,9 @@
>>>> #define USB_DEVICE_ID_QUANTA_OPTICAL_TOUCH_3001 0x3001
>>>> #define USB_DEVICE_ID_QUANTA_OPTICAL_TOUCH_3008 0x3008
>>>> +#define USB_VENDOR_ID_RAZER 0x1532
>>>> +#define USB_DEVICE_ID_RAZER_BLADE_14 0x011D
>>>> +
>>>> #define USB_VENDOR_ID_REALTEK 0x0bda
>>>> #define USB_DEVICE_ID_REALTEK_READER 0x0152
>>>> diff --git a/drivers/hid/hid-rmi.c b/drivers/hid/hid-rmi.c
>>>> index 3cccff7..1f131df 100644
>>>> --- a/drivers/hid/hid-rmi.c
>>>> +++ b/drivers/hid/hid-rmi.c
>>>> @@ -453,7 +453,15 @@ static int rmi_raw_event(struct hid_device *hdev,
>>>> case RMI_ATTN_REPORT_ID:
>>>> return rmi_input_event(hdev, data, size);
>>>> case RMI_MOUSE_REPORT_ID:
>>>> - rmi_schedule_reset(hdev);
>>>> + /*
>>>> + * touchpads with physical mouse buttons will report
>>>> those
>>>> + * buttons in mouse reports even in RMI mode. Only reset
>>>> + * the device if we see reports which contain X or Y
>>>> data.
>>>> + */
>>>> + if (data[2] != 0 || data[3] != 0)
>>
>> this seems a little bit magical. There may not be an other solution,
>> but I would prefer that we look at alternatives before using this
>> magical numbers (will the touchpad always use the same report
>> descriptor on the mouse interface?)
>
>
> I did just look at the reports and see where X and Y where reported. Since
> mouse mode is really only there for compatibility when no custom driver is
> present I think it is unlikely that it will change in the future. However,
> there is no guarantee that it won't change. I will see if I can come up with
> is less of a hack. Maybe, I can use the event callback instead of raw_event.
I would prefer an event callback (if it is not too much trouble).
>
>
>>>> + rmi_schedule_reset(hdev);
>>>> + else
>>>> + return 1;
>>>> break;
>>>> }
>>>> @@ -871,6 +879,11 @@ static int rmi_input_mapping(struct hid_device
>>>> *hdev,
>>>> struct hid_input *hi, struct hid_field *field,
>>>> struct hid_usage *usage, unsigned long **bit, int *max)
>>>> {
>>>> + if (field->application == HID_GD_POINTER
>>>> + && (usage->hid & HID_USAGE_PAGE) == HID_UP_BUTTON)
>>>> + /* Pass mouse button reports to generic code for
>>>> processing */
>>>> + return 0;
>>>> +
>>>> /* we want to make HID ignore the advertised HID collection */
>>>> return -1;
>>>> }
>>>
>>>
>> Cheers,
>> Benjamin
>
>
> Andrew
Cheers,
Benjamin
^ permalink raw reply
* Re: [PATCH 1/2] HID: logitech-hidpp: do not return the name length
From: Benjamin Tissoires @ 2014-12-11 1:33 UTC (permalink / raw)
To: Peter Wu
Cc: Jiri Kosina, Nestor Lopez Casado, Peter Hutterer, linux-input,
linux-kernel
In-Reply-To: <1906623.oO6IMXUbli@al>
On Dec 11 2014 or thereabouts, Peter Wu wrote:
> On Wednesday 10 December 2014 18:01:52 Benjamin Tissoires wrote:
> > On Dec 10 2014 or thereabouts, Peter Wu wrote:
> > > On Wednesday 10 December 2014 17:21:09 Benjamin Tissoires wrote:
> > > > We do not make any use of the actual name length get through
> > > > hidpp_get_device_name().
> > > >
> > > > We can drop the extra code and simplify the API a bit.
> > > >
> > > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > > >
> > > > ---
> > > > drivers/hid/hid-logitech-hidpp.c | 10 +++-------
> > > > 1 file changed, 3 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> > > > index 1a6395d..3846305 100644
> > > > --- a/drivers/hid/hid-logitech-hidpp.c
> > > > +++ b/drivers/hid/hid-logitech-hidpp.c
> > > > @@ -461,7 +461,7 @@ static int hidpp_devicenametype_get_device_name(struct hidpp_device *hidpp,
> > > > return count;
> > > > }
> > > >
> > > > -static char *hidpp_get_device_name(struct hidpp_device *hidpp, u8 *name_length)
> > > > +static char *hidpp_get_device_name(struct hidpp_device *hidpp)
> > > > {
> > > > u8 feature_type;
> > > > u8 feature_index;
> > > > @@ -484,7 +484,6 @@ static char *hidpp_get_device_name(struct hidpp_device *hidpp, u8 *name_length)
> > > > if (!name)
> > > > goto out_err;
> > > >
> > > > - *name_length = __name_length + 1;
> > > > while (index < __name_length)
> > > > index += hidpp_devicenametype_get_device_name(hidpp,
> > > > feature_index, index, name + index,
> > >
> > > hidpp_devicenametype_get_device_name can return a non-positive value if
> > > the USB device is unplugged at the wrong time, or if a malicious device
> > > is attached (and 0 is returned). An infinite loop is the result.
> >
> > Oh, yes, you are definitively right.
> >
> > >
> > > Can you apply this change in the patch or should I send you a separate
> > > one?
> >
> > Feel free to send your patch directly to the list.
> >
> > >
> > > while (index < __name_length) {
> > > ret = hidpp_devicenametype_get_device_name(hidpp,
> > > feature_index, index, name + index,
> > > __name_length - index);
> > > if (ret <= 0) {
> > > kfree(name);
> > > return NULL;
> > > }
> > > index += ret;
> > > }
> > >
> > > > @@ -493,7 +492,6 @@ static char *hidpp_get_device_name(struct hidpp_device *hidpp, u8 *name_length)
> > > > return name;
> > > >
> > > > out_err:
> > > > - *name_length = 0;
> > > > return NULL;
> > >
> > > What about dropping the label out_err here and returning NULL in the
> > > previous places?
> >
> > Yep, good idea. You can either send a patch for that if you want to take
> > credits, or I can send a v2. It's up to you.
> >
> > Cheers,
> > Benjamin
>
> I found another place where a boundary check is missing, but will treat
> that in a separate patch. If you send a v2 with the return NULL fixed
> up, I'll base my patch (to add a retval check) on yours.
OK. I am not sure I will be able to make it today or tomorrow. Feel free
to send the fixes right now if you can manage to send them now.
Cheers,
Benjamin
>
> Kind regards,
> Peter
>
> > >
> > > > }
> > > >
> > > > @@ -989,7 +987,6 @@ static void hidpp_overwrite_name(struct hid_device *hdev, bool use_unifying)
> > > > {
> > > > struct hidpp_device *hidpp = hid_get_drvdata(hdev);
> > > > char *name;
> > > > - u8 name_length;
> > > >
> > > > if (use_unifying)
> > > > /*
> > > > @@ -999,7 +996,7 @@ static void hidpp_overwrite_name(struct hid_device *hdev, bool use_unifying)
> > > > */
> > > > name = hidpp_get_unifying_name(hidpp);
> > > > else
> > > > - name = hidpp_get_device_name(hidpp, &name_length);
> > > > + name = hidpp_get_device_name(hidpp);
> > > >
> > > > if (!name)
> > > > hid_err(hdev, "unable to retrieve the name of the device");
> > > > @@ -1053,7 +1050,6 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
> > > > bool connected = atomic_read(&hidpp->connected);
> > > > struct input_dev *input;
> > > > char *name, *devm_name;
> > > > - u8 name_length;
> > > >
> > > > if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)
> > > > wtp_connect(hdev, connected);
> > > > @@ -1080,7 +1076,7 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
> > > > return;
> > > > }
> > > >
> > > > - name = hidpp_get_device_name(hidpp, &name_length);
> > > > + name = hidpp_get_device_name(hidpp);
> > > > if (!name) {
> > > > hid_err(hdev, "unable to retrieve the name of the device");
> > > > } else {
> > > >
>
^ permalink raw reply
* Re: [systemd-devel] Supporting U2F over HID on Linux?
From: David Herrmann @ 2014-12-11 8:49 UTC (permalink / raw)
To: Andy Lutomirski; +Cc: Jiri Kosina, open list:HID CORE LAYER, Benjamin Tissoires
In-Reply-To: <CALCETrU9Nfpt0Zch5Swi+OaK=KTjJoOw7GAx4B7q1B56SqrUbA@mail.gmail.com>
Hi
On Wed, Dec 10, 2014 at 6:07 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Tue, Dec 9, 2014 at 12:46 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>> On Mon, Nov 3, 2014 at 12:41 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>>> On Mon, Nov 3, 2014 at 12:21 PM, Jiri Kosina <jkosina@suse.cz> wrote:
>>>> On Mon, 3 Nov 2014, David Herrmann wrote:
>>>>
>>>>> > Agreed, mostly. My only real concern is that this could be annoying
>>>>> > for the userspace developers who will need to target Linux and HIDAPI
>>>>> > separately. Admittedly the Linux support will be trivial.
>>>>>
>>>>> I see. I'll not stop you from using hidraw, I'd just not recommend it.
>>>>> Especially for security stuff I dislike exposing the whole HID device
>>>>> writable. But yeah, I guess you got my point.
>>>>
>>>> Alright, I am basically thinking loudly now, but how about we allow HID
>>>> drivers that would override default hidraw interface?
>>>>
>>>> I am very well aware of the fact that this could be opening a can of
>>>> worms, so we'll have to make it very restrictive. How about, let's say, we
>>>> allow HID drivers to provide custom hidraw interface (completely
>>>> overriding the one that HID core would normally create) only for cases
>>>> such as:
>>>>
>>>> - the intent is to expose only certain parts of a combined device
>>>> - the intent is to introduce some level of access control
>>>>
>>>> I.e. still no interference of kernel with data parsing allowed.
>>>
>>> Hmm. Would this be like a filter on hidraw actions?
>>>
>>> How would udev distinguish these special hidraw devices from normal
>>> hidraw devices?
>>>
>>> Also, for U2F, this could be a little awkward. There's some crazy
>>> fragmentation stuff to allow a U2F request to be split across HID
>>> requests, and I think a kernel driver would much rather get the
>>> original unfragmented application request.
>>>
>>
>> I started writing a driver for this. I got enumeration working. I
>> assume I should create a "u2f" device class, and then... ?
>>
>> Where am I supposed to get my character device from? Do I register my
>> own chrdev major? Do I use misc? Is there some input thing I'm
>> supposed to use?
>
> Another question:
>
> I'm hitting this in hid_input_report:
>
> if (down_trylock(&hid->driver_input_lock)) {
> pr_err("HID: trylock failed\n"); // I added this
> return -EBUSY;
> }
>
> This is a problem for u2f: u2f reports are actual protocol messages,
> and there isn't a retransmit mechanism. Losing messages randomly
> causes the handshake to fail, and then nothing works.
>
> I *think* that the only way I can hit that failure is during probe (or
> if the USB stack somehow completes two transfers at once). This still
> makes it quite awkward to start IO from the probe routine.
>
> I could start a work item to take driver_input_lock, release it again,
> and then start the handshake, but that sucks.
HID reports are ordered. It's the responsibility of the
transport-layer to not provide multiple reports in parallel. The
reason we have this down_trylock() is to drop packages if no driver is
loaded, yet. I wonder how you can trigger this? Does this happen
during runtime or only driver load? Do you send GET_REPORT requests to
the device?
Thanks
David
^ permalink raw reply
* Re: [sur40] videobuf2 and/or DMA?
From: David Herrmann @ 2014-12-11 8:58 UTC (permalink / raw)
To: Florian Echtler; +Cc: linux-input, Oliver Neukum, Alan Stern, Hans de Goede
In-Reply-To: <54855856.3070207@butterbrot.org>
Hi
On Mon, Dec 8, 2014 at 8:50 AM, Florian Echtler <floe@butterbrot.org> wrote:
> Hello everyone,
>
> I'm preparing to finally add support for the raw sensor video stream to
> my driver for the SUR40 touchscreen. However, after an extensive amount
> of Googling, I'm still not clear on the relationship between DMA
> transfers, the USB core and the videobuf2 framework.
>
> Specifically, I'd like to know:
>
> - Can I always use DMA on the USB side (for bulk transfers), or does
> this in any way require support from the USB device's hardware? (I'm
> guessing no, but a definite answer would be great.)
I thought the USB API hides that from you? Nevertheless, this is a
question for linux-usb, not linux-input. I put some people on CC which
work on both, HID and USB. Maybe they can help you.
> - Regardless of the USB side of things, can I use the videobuf2
> framework without _having_ to use DMA? That way, I could get a basic
> driver running without DMA and switch later when it's convenient and/or
> the speedup is justified.
This is definitely a question for linux-media.
Thanks
David
^ permalink raw reply
* Re: NULL pointer dereference in i2c-hid
From: Mika Westerberg @ 2014-12-11 8:58 UTC (permalink / raw)
To: Gabriele Mazzotta
Cc: linux-input, linux-kernel, benjamin.tissoires, aduggan, jkosina
In-Reply-To: <31518562.V5Oyo0POsI@xps13>
On Wed, Dec 10, 2014 at 06:04:51PM +0100, Gabriele Mazzotta wrote:
> my laptop uses a touchpad that needs hid-rmi along with i2c-hid to work.
> i2c-hid and hid-rmi can be loaded and unloaded independelty from each
> other, however since 34f439e4afcd ("HID: i2c-hid: add runtime PM support")
> if I unload hid-rmi and after it I also unload i2c-hid, I get a NULL
> pointer dereference.
I'll look into this.
I can reproduce this easily with i2c-hid + hid-multitouch following your
directions.
^ 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