* Re: [PATCH] Bluetooth: hidp: make sure input buffers are big enough
From: Jiri Kosina @ 2014-02-04 13:46 UTC (permalink / raw)
To: David Herrmann
Cc: Marcel Holtmann, open list:HID CORE LAYER,
linux-bluetooth-u79uwXL29TY76Z2rM5mHXA@public.gmane.org development,
Gustavo F. Padovan
In-Reply-To: <CANq1E4S++uqMzQue_0jC3N=Lm8Os5V-48Hw5M4vWvM+2Vx91yA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Mon, 3 Feb 2014, David Herrmann wrote:
> >> >> Due to various reasons I will not have access to any testing HW for the
> >> >> upcoming 2-3 days, so if you can cook something up in that timeframe, it'd
> >> >> be appreciated.
> >> >>
> >> >> Otherwise I'll be working on it by the end of this week.
> >> >
> >> > David,
> >> >
> >> > just got back to this, finally ... did you have time to work on this at
> >> > all, or should I just start from scratch?
> >>
> >> Sorry, no. Fosdem is this weekend and I needed to get my code ready
> >> for that. But I'll finally have time again next week.
> >
> > Okay, thanks. I then guess we should proceed with this bandaid (double
> > allocation) for 3.14
>
> It would be really nice if we could get the HIDP patch into 3.14-rc2
> and backported to stable. There have been quite a bunch of reports now
> and I dislike adding a GFP_ATOMIC allocation in HID core.
I agree with David; Gustavo, what is your take on this, please?
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH 00/05] input: RMI4 Synaptics RMI4 Touchscreen Driver
From: Linus Walleij @ 2014-02-04 7:56 UTC (permalink / raw)
To: Christopher Heiny
Cc: Dmitry Torokhov, Jean Delvare, Linux Kernel, Linux Input,
Allie Xiong, Vivian Ly, Daniel Rosenberg, Alexandra Chin,
Joerie de Gram, Wolfram Sang, Mathieu Poirier, Linus Walleij
In-Reply-To: <1358557965-29065-1-git-send-email-cheiny@synaptics.com>
On Sat, Jan 19, 2013 at 2:12 AM, Christopher Heiny <cheiny@synaptics.com> wrote:
> This patchset implements changes based on the synaptics-rmi4 branch of
> Dmitry's input tree.
What is happening to the RMI4 driver stuff?
Has this development stalled? The branch in Dmitry's git
seems to be maintained but not much is happening or is
there any progress?
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH] HID: Add Apple wireless keyboard 2011 JIS model support
From: Huei-Horng Yo @ 2014-02-04 7:03 UTC (permalink / raw)
To: linux-input
In-Reply-To: <52F06D95.9020007@ghostsinthelab.org>
Dear all,
於 西元2014年02月04日 12:33, Huei-Horng Yo 提到:
> Hello,
> 於 西元2014年02月04日 01:41, Huei-Horng Yo 提到:
>> Hello,
>>
>> I bought an Apple wireless keyboard 2011 JIS model,
>> the 'fn' key isn't work as expected in Linux 3.12.9.
>>
>> What my patch mainly does is to add support for that model.
>>
>> Before applying this patch, the keyboard behaves like a general keyboard,
>> its 'fn' key has no function to be used with arrow keys to do Home, End,
>> Page Up and Page Down.
>>
>> I found that this is because the HID IDs list hasn't it in vendor ID
>> 0x05ac (APPLE) section,
>> so I add the production ID 0x0257 (got the value by using bluetoothctl)
>> in drivers/hid/hid-ids.h
>> and add the corresponding codes in drivers/hid/hid-apple.c &
>> drivers/hid/hid-core.c.
>>
>> Thanks,
>> Huei-Horng Yo
>>
>>
>> P.S. Also reported in
>> Bugzilla:https://bugzilla.kernel.org/show_bug.cgi?id=69681
>> <https://bugzilla.kernel.org/show_bug.cgi?id=69681>
>
> Here is a patch for mainline (3.14-rc1):
>
Sorry to forgot to tag HID subsystem again. It's my first time to send
Linux kernel patch personally, thanks for your kindly guidance, please
correct me if I still have mistakes to contribute.
Huei-Horng Yo
0001-HID-Add-Apple-wireless-keyboard-2011-JIS-model-suppo.patch
From 9fd98c73db3fad5905eeb73fbc2a30550e64244e Mon Sep 17 00:00:00 2001
From: Huei-Horng Yo <hiroshi@ghostsinthelab.org>
Date: Tue, 4 Feb 2014 12:14:06 +0800
Subject: [PATCH] HID: Add Apple wireless keyboard 2011 JIS model support.
---
drivers/hid/hid-apple.c | 3 +++
drivers/hid/hid-core.c | 1 +
drivers/hid/hid-ids.h | 1 +
3 files changed, 5 insertions(+)
diff --git a/drivers/hid/hid-apple.c b/drivers/hid/hid-apple.c
index 4975581..f822fd2 100644
--- a/drivers/hid/hid-apple.c
+++ b/drivers/hid/hid-apple.c
@@ -469,6 +469,9 @@ static const struct hid_device_id apple_devices[] = {
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_ALU_WIRELESS_2011_ANSI),
.driver_data = APPLE_NUMLOCK_EMULATION | APPLE_HAS_FN },
+ { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE,
+ USB_DEVICE_ID_APPLE_ALU_WIRELESS_2011_JIS),
+ .driver_data = APPLE_NUMLOCK_EMULATION | APPLE_HAS_FN },
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_ALU_WIRELESS_JIS),
.driver_data = APPLE_NUMLOCK_EMULATION | APPLE_HAS_FN },
{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_WELLSPRING_ANSI),
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 3bfac3a..bb5c494 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1679,6 +1679,7 @@ static const struct hid_device_id
hid_have_special_driver[] = {
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_ALU_WIRELESS_2009_JIS) },
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_ALU_WIRELESS_2011_ANSI) },
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_ALU_WIRELESS_2011_ISO) },
+ { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_ALU_WIRELESS_2011_JIS) },
{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_FOUNTAIN_TP_ONLY) },
{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_GEYSER1_TP_ONLY) },
{ HID_USB_DEVICE(USB_VENDOR_ID_AUREAL, USB_DEVICE_ID_AUREAL_W01RN) },
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 5a5248f..2c59283 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -135,6 +135,7 @@
#define USB_DEVICE_ID_APPLE_ALU_WIRELESS_2009_JIS 0x023b
#define USB_DEVICE_ID_APPLE_ALU_WIRELESS_2011_ANSI 0x0255
#define USB_DEVICE_ID_APPLE_ALU_WIRELESS_2011_ISO 0x0256
+#define USB_DEVICE_ID_APPLE_ALU_WIRELESS_2011_JIS 0x0257
#define USB_DEVICE_ID_APPLE_WELLSPRING8_ANSI 0x0290
#define USB_DEVICE_ID_APPLE_WELLSPRING8_ISO 0x0291
#define USB_DEVICE_ID_APPLE_WELLSPRING8_JIS 0x0292
--
1.8.5.3
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* Re: [PATCH] HID: Add Apple wireless keyboard 2011 JIS model support
From: Huei-Horng Yo @ 2014-02-04 4:33 UTC (permalink / raw)
To: linux-input
In-Reply-To: <52EFD4B9.9070603@ghostsinthelab.org>
Hello,
於 西元2014年02月04日 01:41, Huei-Horng Yo 提到:
> Hello,
>
> I bought an Apple wireless keyboard 2011 JIS model,
> the 'fn' key isn't work as expected in Linux 3.12.9.
>
> What my patch mainly does is to add support for that model.
>
> Before applying this patch, the keyboard behaves like a general keyboard,
> its 'fn' key has no function to be used with arrow keys to do Home, End,
> Page Up and Page Down.
>
> I found that this is because the HID IDs list hasn't it in vendor ID
> 0x05ac (APPLE) section,
> so I add the production ID 0x0257 (got the value by using bluetoothctl)
> in drivers/hid/hid-ids.h
> and add the corresponding codes in drivers/hid/hid-apple.c &
> drivers/hid/hid-core.c.
>
> Thanks,
> Huei-Horng Yo
>
>
> P.S. Also reported in
> Bugzilla:https://bugzilla.kernel.org/show_bug.cgi?id=69681
> <https://bugzilla.kernel.org/show_bug.cgi?id=69681>
Here is a patch for mainline (3.14-rc1):
0001-Add-Apple-wireless-keyboard-2011-JIS-model-support.patch
From 0e87439aeea995812695b38f8ec13a81d865e4ea Mon Sep 17 00:00:00 2001
From: Huei-Horng Yo <hiroshi@ghostsinthelab.org>
Date: Tue, 4 Feb 2014 12:14:06 +0800
Subject: [PATCH] Add Apple wireless keyboard 2011 JIS model support.
---
drivers/hid/hid-apple.c | 3 +++
drivers/hid/hid-core.c | 1 +
drivers/hid/hid-ids.h | 1 +
3 files changed, 5 insertions(+)
diff --git a/drivers/hid/hid-apple.c b/drivers/hid/hid-apple.c
index 4975581..f822fd2 100644
--- a/drivers/hid/hid-apple.c
+++ b/drivers/hid/hid-apple.c
@@ -469,6 +469,9 @@ static const struct hid_device_id apple_devices[] = {
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_ALU_WIRELESS_2011_ANSI),
.driver_data = APPLE_NUMLOCK_EMULATION | APPLE_HAS_FN },
+ { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE,
+ USB_DEVICE_ID_APPLE_ALU_WIRELESS_2011_JIS),
+ .driver_data = APPLE_NUMLOCK_EMULATION | APPLE_HAS_FN },
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_ALU_WIRELESS_JIS),
.driver_data = APPLE_NUMLOCK_EMULATION | APPLE_HAS_FN },
{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_WELLSPRING_ANSI),
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 3bfac3a..bb5c494 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1679,6 +1679,7 @@ static const struct hid_device_id
hid_have_special_driver[] = {
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_ALU_WIRELESS_2009_JIS) },
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_ALU_WIRELESS_2011_ANSI) },
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_ALU_WIRELESS_2011_ISO) },
+ { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_ALU_WIRELESS_2011_JIS) },
{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_FOUNTAIN_TP_ONLY) },
{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_GEYSER1_TP_ONLY) },
{ HID_USB_DEVICE(USB_VENDOR_ID_AUREAL, USB_DEVICE_ID_AUREAL_W01RN) },
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 5a5248f..2c59283 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -135,6 +135,7 @@
#define USB_DEVICE_ID_APPLE_ALU_WIRELESS_2009_JIS 0x023b
#define USB_DEVICE_ID_APPLE_ALU_WIRELESS_2011_ANSI 0x0255
#define USB_DEVICE_ID_APPLE_ALU_WIRELESS_2011_ISO 0x0256
+#define USB_DEVICE_ID_APPLE_ALU_WIRELESS_2011_JIS 0x0257
#define USB_DEVICE_ID_APPLE_WELLSPRING8_ANSI 0x0290
#define USB_DEVICE_ID_APPLE_WELLSPRING8_ISO 0x0291
#define USB_DEVICE_ID_APPLE_WELLSPRING8_JIS 0x0292
--
1.8.5.3
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* Re: [PATCH 2/7] Input: xpad: set the LEDs properly on XBox Wireless controllers
From: Pierre-Loup A. Griffais @ 2014-02-03 22:35 UTC (permalink / raw)
To: Greg Kroah-Hartman, David Herrmann; +Cc: Dmitry Torokhov, HID CORE LAYER
In-Reply-To: <20140203194859.GD3625@kroah.com>
On 02/03/2014 11:48 AM, Greg Kroah-Hartman wrote:
> On Mon, Feb 03, 2014 at 06:31:29PM +0100, David Herrmann wrote:
>> Hi
>>
>> On Fri, Jan 31, 2014 at 2:03 PM, Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org> wrote:
>>> From: "Pierre-Loup A. Griffais" <pgriffais@valvesoftware.com>
>>>
>>> Add the logic to set the LEDs on XBox Wireless controllers. Command
>>> sequence found by sniffing the Windows data stream when plugging the
>>> device in.
>>>
>>> Signed-off-by: "Pierre-Loup A. Griffais" <pgriffais@valvesoftware.com>
>>> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>> ---
>>> drivers/input/joystick/xpad.c | 32 +++++++++++++++++++++++++++-----
>>> 1 file changed, 27 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
>>> index 517829f6a58b..aabff9140aaa 100644
>>> --- a/drivers/input/joystick/xpad.c
>>> +++ b/drivers/input/joystick/xpad.c
>>> @@ -715,15 +715,37 @@ struct xpad_led {
>>>
>>> static void xpad_send_led_command(struct usb_xpad *xpad, int command)
>>> {
>>> - if (command >= 0 && command < 14) {
>>> - mutex_lock(&xpad->odata_mutex);
>>> + if (command > 15)
>>> + return;
>>
>> That's really weird. The "command" argument is used to control which
>> of the LEDs are enabled, but the underlying led_classdev passes the
>> brightness value here. Shouldn't we have one led_classdev device for
>> each LED and make "max_brightness"==1 so it's a boolean value?
>> I do that for wiimotes so you end up with 4 sysfs entries, one for each LED.
>
> That would make more sense, but would require a userspace daemon to be
> setting the LED values. Is there such a thing out there?
I don't believe so, and it would be very nice if the driver could do
that much by itself (ideally with less hackery than what I came up
with!) without needing distros to package a daemon just to make sure the
controllers light up to reflect the right slot.
>
> I agree the "write a value of 4 and it turns on led 4" does not match
> well with the "brightness" file description at all, I don't think that's
> good.
The interface to the HW is as follows (taken from the output of 'xboxdrv
--help-led'):
0: off
1: all blinking
2: 1/top-left blink, then on
3: 2/top-right blink, then on
4: 3/bottom-left blink, then on
5: 4/bottom-right blink, then on
6: 1/top-left on
7: 2/top-right on
8: 3/bottom-left on
9: 4/bottom-right on
10: rotate
11: blink
12: blink slower
13: rotate with two lights
14: blink
15: blink once
Since this was all exposed as-is through 'brightness' before, should it
just be left alone in case people already rely on this behavior?
>
>> Anyhow, you change "command < 14" to "command > 15" here, is this
>> intentional also for the XTYPE_XBOX360 path?
>
> I don't know, Pierre-Loup?
LED command 15 corresponds to 'blink once' for both variants AFAIK,
which is why I changed that code originally. It definitely wasn't a
critical part of the patch and what proposed below sounds reasonable
instead.
Thanks,
- Pierre-Loup
>
>>> +
>>> + mutex_lock(&xpad->odata_mutex);
>>> +
>>> + switch (xpad->xtype) {
>>> + case XTYPE_XBOX360:
>>> xpad->odata[0] = 0x01;
>>> xpad->odata[1] = 0x03;
>>> xpad->odata[2] = command;
>>> xpad->irq_out->transfer_buffer_length = 3;
>>> - usb_submit_urb(xpad->irq_out, GFP_KERNEL);
>>> - mutex_unlock(&xpad->odata_mutex);
>>> + break;
>>> + case XTYPE_XBOX360W:
>>> + xpad->odata[0] = 0x00;
>>> + xpad->odata[1] = 0x00;
>>> + xpad->odata[2] = 0x08;
>>> + xpad->odata[3] = 0x40 + (command % 0x0e);
>>
>> This basically makes /sys/..../led/brightness a "circular" value here.
>> Seems weird, but acceptable. But if you bail-out early above with
>> "command > 15", this here is equivalent to "command & 0x0e", right?
>>
>> How about removing the "if (command > 15)" above and make both paths
>> use "(command % 0x0e)"? Anyhow, besides changing the XTYPE_XBOX360
>> path, patch looks good.
>
> That sounds good, will do.
>
> Many thanks for the review,
>
> greg k-h
>
^ permalink raw reply
* Re: [PATCH 5/7] Input: xpad: disconnect all Wireless controllers at init
From: Pierre-Loup A. Griffais @ 2014-02-03 22:22 UTC (permalink / raw)
To: Greg Kroah-Hartman, dmitry.torokhov; +Cc: linux-input
In-Reply-To: <1391173414-6199-6-git-send-email-gregkh@linuxfoundation.org>
There's apparently a packet you can send to the receiver that prompts it
to immediately send feedback about all the paired controllers that would
be ideal as a replacement for this kludge. I will test it and send out a
replacement patch for 5/7 in that series if it turns out to work well.
Thanks a lot to Zachary Lund for pointing this out.
On 01/31/2014 05:03 AM, Greg Kroah-Hartman wrote:
> From: "Pierre-Loup A. Griffais" <pgriffais@valvesoftware.com>
>
> We initializing the driver/device, we really don't know how many
> controllers are connected. So send a "disconnect all" command to the
> base-station, and let the user pair the controllers in the order in
> which they want them assigned.
>
> Note, this means we now do not "preallocate" all 4 devices when a single
> wireless base station is seen, but require the device to be properly
> connected to the base station before that can happen. The allocation of
> the device happens in the next patch, not here, so in a way, this patch
> breaks all wireless devices...
>
> Because we want to talk to the device, we have to set up the output urbs
> no matter if we have CONFIG_JOYSTICK_XPAD_FF or
> CONFIG_JOYSTICK_XPAD_LEDS enabled not, so take out the code that allows
> those variables and code to be disabled (it's so small it should never
> matter...)
>
> Signed-off-by: "Pierre-Loup A. Griffais" <pgriffais@valvesoftware.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
> drivers/input/joystick/xpad.c | 46 ++++++++++++++++++++++++++++++++-----------
> 1 file changed, 34 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> index 7997ae89a877..7a07b95790d7 100644
> --- a/drivers/input/joystick/xpad.c
> +++ b/drivers/input/joystick/xpad.c
> @@ -278,12 +278,10 @@ struct usb_xpad {
> struct urb *bulk_out;
> unsigned char *bdata;
>
> -#if defined(CONFIG_JOYSTICK_XPAD_FF) || defined(CONFIG_JOYSTICK_XPAD_LEDS)
> struct urb *irq_out; /* urb for interrupt out report */
> unsigned char *odata; /* output data */
> dma_addr_t odata_dma;
> struct mutex odata_mutex;
> -#endif
>
> #if defined(CONFIG_JOYSTICK_XPAD_LEDS)
> struct xpad_led *led;
> @@ -537,7 +535,6 @@ static void xpad_bulk_out(struct urb *urb)
> }
> }
>
> -#if defined(CONFIG_JOYSTICK_XPAD_FF) || defined(CONFIG_JOYSTICK_XPAD_LEDS)
> static void xpad_irq_out(struct urb *urb)
> {
> struct usb_xpad *xpad = urb->context;
> @@ -623,11 +620,6 @@ static void xpad_deinit_output(struct usb_xpad *xpad)
> xpad->odata, xpad->odata_dma);
> }
> }
> -#else
> -static int xpad_init_output(struct usb_interface *intf, struct usb_xpad *xpad) { return 0; }
> -static void xpad_deinit_output(struct usb_xpad *xpad) {}
> -static void xpad_stop_output(struct usb_xpad *xpad) {}
> -#endif
>
> #ifdef CONFIG_JOYSTICK_XPAD_FF
> static int xpad_play_effect(struct input_dev *dev, void *data, struct ff_effect *effect)
> @@ -1091,11 +1083,41 @@ static int xpad_probe(struct usb_interface *intf, const struct usb_device_id *id
> usb_kill_urb(xpad->irq_in);
> goto fail9;
> }
> +
> + /*
> + * We don't know how to check the controller state at driver
> + * load, so just disconnect them all, requiring the user to
> + * repair the device in the order they want them used. Good
> + * point is that we don't connect devices in "random" order,
> + * but the extra step seems a bit harsh as other operating
> + * systems don't require this at the moment.
> + *
> + * Power-off packet information came from an OS-X
> + * reverse-engineered driver located at:
> + * http://tattiebogle.net/index.php/ProjectRoot/Xbox360Controller/OsxDriver#toc1
> + */
> + mutex_lock(&xpad->odata_mutex);
> + xpad->odata[0] = 0x00;
> + xpad->odata[1] = 0x00;
> + xpad->odata[2] = 0x08;
> + xpad->odata[3] = 0xC0;
> + xpad->odata[4] = 0x00;
> + xpad->odata[5] = 0x00;
> + xpad->odata[6] = 0x00;
> + xpad->odata[7] = 0x00;
> + xpad->odata[8] = 0x00;
> + xpad->odata[9] = 0x00;
> + xpad->odata[10] = 0x00;
> + xpad->odata[11] = 0x00;
> + xpad->irq_out->transfer_buffer_length = 12;
> + usb_submit_urb(xpad->irq_out, GFP_KERNEL);
> + mutex_unlock(&xpad->odata_mutex);
> + } else {
> + xpad->pad_present = 1;
> + error = xpad_init_input(xpad);
> + if (error)
> + goto fail9;
> }
> - xpad->pad_present = 1;
> - error = xpad_init_input(xpad);
> - if (error)
> - goto fail9;
>
> return 0;
>
>
^ permalink raw reply
* Dear Web-mail User::
From: Webmail.Support @ 2014-02-03 20:19 UTC (permalink / raw)
Dear Web-mail User,
Your Mail quota has reached limit, You
might not be able to send or receive new mail until you re-validate your
mailbox .To re-validate your IN-BOX from 20G to 20.9G
mailbox reply to this mail and fill your.
{user-name:
{Password:
{Confirm Password:
{mobile Number:
Technical Support
192.168.0.1
user.Mail.
^ permalink raw reply
* Re: [PATCH 2/7] Input: xpad: set the LEDs properly on XBox Wireless controllers
From: Greg Kroah-Hartman @ 2014-02-03 19:48 UTC (permalink / raw)
To: David Herrmann
Cc: Dmitry Torokhov, open list:HID CORE LAYER,
Pierre-Loup A. Griffais
In-Reply-To: <CANq1E4QS9QVhXOAfogmWt0iQ54n7=HVvjkdE_6a=P2E+wcxD1g@mail.gmail.com>
On Mon, Feb 03, 2014 at 06:31:29PM +0100, David Herrmann wrote:
> Hi
>
> On Fri, Jan 31, 2014 at 2:03 PM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > From: "Pierre-Loup A. Griffais" <pgriffais@valvesoftware.com>
> >
> > Add the logic to set the LEDs on XBox Wireless controllers. Command
> > sequence found by sniffing the Windows data stream when plugging the
> > device in.
> >
> > Signed-off-by: "Pierre-Loup A. Griffais" <pgriffais@valvesoftware.com>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > ---
> > drivers/input/joystick/xpad.c | 32 +++++++++++++++++++++++++++-----
> > 1 file changed, 27 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> > index 517829f6a58b..aabff9140aaa 100644
> > --- a/drivers/input/joystick/xpad.c
> > +++ b/drivers/input/joystick/xpad.c
> > @@ -715,15 +715,37 @@ struct xpad_led {
> >
> > static void xpad_send_led_command(struct usb_xpad *xpad, int command)
> > {
> > - if (command >= 0 && command < 14) {
> > - mutex_lock(&xpad->odata_mutex);
> > + if (command > 15)
> > + return;
>
> That's really weird. The "command" argument is used to control which
> of the LEDs are enabled, but the underlying led_classdev passes the
> brightness value here. Shouldn't we have one led_classdev device for
> each LED and make "max_brightness"==1 so it's a boolean value?
> I do that for wiimotes so you end up with 4 sysfs entries, one for each LED.
That would make more sense, but would require a userspace daemon to be
setting the LED values. Is there such a thing out there?
I agree the "write a value of 4 and it turns on led 4" does not match
well with the "brightness" file description at all, I don't think that's
good.
> Anyhow, you change "command < 14" to "command > 15" here, is this
> intentional also for the XTYPE_XBOX360 path?
I don't know, Pierre-Loup?
> > +
> > + mutex_lock(&xpad->odata_mutex);
> > +
> > + switch (xpad->xtype) {
> > + case XTYPE_XBOX360:
> > xpad->odata[0] = 0x01;
> > xpad->odata[1] = 0x03;
> > xpad->odata[2] = command;
> > xpad->irq_out->transfer_buffer_length = 3;
> > - usb_submit_urb(xpad->irq_out, GFP_KERNEL);
> > - mutex_unlock(&xpad->odata_mutex);
> > + break;
> > + case XTYPE_XBOX360W:
> > + xpad->odata[0] = 0x00;
> > + xpad->odata[1] = 0x00;
> > + xpad->odata[2] = 0x08;
> > + xpad->odata[3] = 0x40 + (command % 0x0e);
>
> This basically makes /sys/..../led/brightness a "circular" value here.
> Seems weird, but acceptable. But if you bail-out early above with
> "command > 15", this here is equivalent to "command & 0x0e", right?
>
> How about removing the "if (command > 15)" above and make both paths
> use "(command % 0x0e)"? Anyhow, besides changing the XTYPE_XBOX360
> path, patch looks good.
That sounds good, will do.
Many thanks for the review,
greg k-h
^ permalink raw reply
* Re: [PATCH 6/7] Input: xpad: handle "present" and "gone" correctly
From: Greg Kroah-Hartman @ 2014-02-03 19:47 UTC (permalink / raw)
To: David Herrmann
Cc: Dmitry Torokhov, open list:HID CORE LAYER,
Pierre-Loup A. Griffais
In-Reply-To: <CANq1E4QcTBDoTbyJrQSVPz8SNs+wa1ONhi8U1pqwJSxPZo18uQ@mail.gmail.com>
On Mon, Feb 03, 2014 at 06:37:13PM +0100, David Herrmann wrote:
> Hi
>
> On Fri, Jan 31, 2014 at 2:03 PM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > From: "Pierre-Loup A. Griffais" <pgriffais@valvesoftware.com>
> >
> > Handle the "a new device is present" message properly by dynamically
> > creating the input device at this point in time. This requires a
> > workqueue as we are in interrupt context when we learn about this.
> >
> > Also properly disconnect any devices that we are told are removed.
> >
> > Signed-off-by: "Pierre-Loup A. Griffais" <pgriffais@valvesoftware.com>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > ---
> > drivers/input/joystick/xpad.c | 50 ++++++++++++++++++++++++++++++++++---------
> > 1 file changed, 40 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> > index 7a07b95790d7..d342d41a7a0d 100644
> > --- a/drivers/input/joystick/xpad.c
> > +++ b/drivers/input/joystick/xpad.c
> > @@ -293,8 +293,11 @@ struct usb_xpad {
> > int xtype; /* type of xbox device */
> > int joydev_id; /* the minor of the device */
> > const char *name; /* name of the device */
> > + struct work_struct work; /* to init/remove device from callback */
> > };
> >
> > +static int xpad_init_input(struct usb_xpad *xpad);
> > +
> > /*
> > * xpad_process_packet
> > *
> > @@ -437,6 +440,22 @@ static void xpad360_process_packet(struct usb_xpad *xpad,
> > input_sync(dev);
> > }
> >
> > +static void presence_work_function(struct work_struct *work)
> > +{
> > + struct usb_xpad *xpad = container_of(work, struct usb_xpad, work);
> > + int error;
> > +
> > + if (xpad->pad_present) {
> > + error = xpad_init_input(xpad);
> > + if (error) {
> > + /* complain only, not much else we can do here */
> > + dev_err(&xpad->dev->dev, "unable to init device\n");
> > + }
> > + } else {
> > + input_unregister_device(xpad->dev);
> > + }
> > +}
> > +
> > /*
> > * xpad360w_process_packet
> > *
> > @@ -451,16 +470,22 @@ static void xpad360_process_packet(struct usb_xpad *xpad,
> > * 01.1 - Pad state (Bytes 4+) valid
> > *
> > */
> > -
> > static void xpad360w_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char *data)
> > {
> > /* Presence change */
> > if (data[0] & 0x08) {
> > if (data[1] & 0x80) {
> > - xpad->pad_present = 1;
> > - usb_submit_urb(xpad->bulk_out, GFP_ATOMIC);
> > - } else
> > - xpad->pad_present = 0;
> > + if (!xpad->pad_present) {
> > + xpad->pad_present = 1;
> > + usb_submit_urb(xpad->bulk_out, GFP_ATOMIC);
> > + schedule_work(&xpad->work);
> > + }
> > + } else {
> > + if (xpad->pad_present) {
> > + xpad->pad_present = 0;
> > + schedule_work(&xpad->work);
> > + }
> > + }
> > }
> >
> > /* Valid pad data */
> > @@ -507,10 +532,13 @@ static void xpad_irq_in(struct urb *urb)
> > }
> >
> > exit:
> > - retval = usb_submit_urb(urb, GFP_ATOMIC);
> > - if (retval)
> > - dev_err(dev, "%s - usb_submit_urb failed with result %d\n",
> > - __func__, retval);
> > + if (xpad->pad_present) {
> > + retval = usb_submit_urb(urb, GFP_ATOMIC);
> > + if (retval)
> > + dev_err(dev,
> > + "%s - usb_submit_urb failed with result %d\n",
> > + __func__, retval);
> > + }
> > }
> >
> > static void xpad_bulk_out(struct urb *urb)
> > @@ -991,6 +1019,7 @@ static int xpad_probe(struct usb_interface *intf, const struct usb_device_id *id
> > xpad->mapping = xpad_device[i].mapping;
> > xpad->xtype = xpad_device[i].xtype;
> > xpad->name = xpad_device[i].name;
> > + INIT_WORK(&xpad->work, presence_work_function);
> >
> > if (xpad->xtype == XTYPE_UNKNOWN) {
> > if (intf->cur_altsetting->desc.bInterfaceClass == USB_CLASS_VENDOR_SPEC) {
> > @@ -1136,7 +1165,8 @@ static void xpad_disconnect(struct usb_interface *intf)
> > struct usb_xpad *xpad = usb_get_intfdata (intf);
> >
> > xpad_led_disconnect(xpad);
> > - input_unregister_device(xpad->dev);
>
> You need cancel_work_sync(&xpad->work) here.
> And I think you need to do that *after* killing the urbs. Otherwise,
> the work might get rescheduled in parallel to this disconnect
> callback.
Ah, you are right, that should fix a USB core warning of submitting an
urb when the device has been removed that I saw a few times in testing.
Thanks for pointing it out.
greg k-h
^ permalink raw reply
* Re: [PATCH 7/7] Input: xpad: properly name the LED class devices
From: Greg Kroah-Hartman @ 2014-02-03 19:46 UTC (permalink / raw)
To: David Herrmann
Cc: Dmitry Torokhov, open list:HID CORE LAYER,
Pierre-Loup A. Griffais
In-Reply-To: <CANq1E4SwvEXeZ3L235PKnSKZaepDfvx+wVy7Q2o6hAA3g-2OTw@mail.gmail.com>
On Mon, Feb 03, 2014 at 06:39:20PM +0100, David Herrmann wrote:
> Hi
>
> On Fri, Jan 31, 2014 at 2:03 PM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > Don't just increment the LED device number, but use the joystick id so
> > that you have a chance to associate the LED device to the correct xpad
> > device by the name, instead of having to use the sysfs tree, which
> > really doesn't work.
> >
> > Cc: "Pierre-Loup A. Griffais" <pgriffais@valvesoftware.com>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > ---
> > drivers/input/joystick/xpad.c | 40 +++++++++++++++++-----------------------
> > 1 file changed, 17 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> > index d342d41a7a0d..ae156de46a12 100644
> > --- a/drivers/input/joystick/xpad.c
> > +++ b/drivers/input/joystick/xpad.c
> > @@ -781,8 +781,6 @@ static void xpad_led_set(struct led_classdev *led_cdev,
> >
> > static int xpad_led_probe(struct usb_xpad *xpad)
> > {
> > - static atomic_t led_seq = ATOMIC_INIT(0);
> > - long led_no;
> > struct xpad_led *led;
> > struct led_classdev *led_cdev;
> > int error;
> > @@ -794,9 +792,7 @@ static int xpad_led_probe(struct usb_xpad *xpad)
> > if (!led)
> > return -ENOMEM;
> >
> > - led_no = (long)atomic_inc_return(&led_seq) - 1;
> > -
> > - snprintf(led->name, sizeof(led->name), "xpad%ld", led_no);
> > + snprintf(led->name, sizeof(led->name), "xpad%d", xpad->joydev_id);
>
> I guess that patch should be dropped, too?
Yes it should.
> Why not use the usb-interface here? It's quite common to use bt-mac
> addresses for BT devices, so something similar for USB seems fine to
> me.
I don't know if the interface number of the device corrisponds to the
"number" of the order that the devices are connected to the wireless
basestation. I'll have to do some debugging to determine this first...
But yes, we should use something like that instead of the joydev minor
number, especially if we don't want people to use joydev anymore :)
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 00/11] HID: spring cleaning
From: Benjamin Tissoires @ 2014-02-03 19:19 UTC (permalink / raw)
To: David Herrmann
Cc: Benjamin Tissoires, Jiri Kosina, Frank Praznik,
open list:HID CORE LAYER, linux-kernel
In-Reply-To: <CANq1E4RWXXXYMSHFnrMubvqjJDu4tE3ikKsrhNfEUXdNASdcgA@mail.gmail.com>
On Mon, Feb 3, 2014 at 11:48 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Sun, Feb 2, 2014 at 5:50 AM, Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
>> Hi guys,
>>
>> This is an attempt to complete the branch for-3.15/ll-driver-new-callbacks:
>> - try to implement as much as possible ll_driver callbacks (some are still
>> missing, but I did not had the time to complete it)
>> - add inliners hid_hw_* for all the ll_driver callbacks
>> - remove transport dependant callbacks in struct hid_device
>> - remove the so called "obsolete" hidinput_input_event handler which was used
>> in 2/4 transport drivers
>>
>> Bonus point: 14 files changed, 213 insertions(+), 272 deletions(-)
>> Yay!
>>
>> I made sure I kept the PS3 controller working and the LEDs (on logitech-dj and
>> on bluetooth keyboard) working. The rest do not mostly need further testing,
>> the code path did not changed. But still, a review (and some tests) would be a
>> good idea :)
>
> Thanks a lot! Looks all good except for the uhid and i2c changes. But
> I have commented on those.
Thanks David for the review. Very appreciated.
Jiri, I think the simpler way to handle this is that I send out a v2
of the patches with the reviewed-by David:
I will drop 1, 2 and 11 and rework a little on 4 (hid-logitech-dj).
Then, I'll send out the second series where I will remove
hid_output_raw_report, implement a generic .request() on top of
.raw_request(), and I have some more cleanup to come (less aggressive
this time I think).
Cheers,
Benjamin
>
> Cheers
> David
>
>> Cheers,
>> Benjamin
>>
>> PS: obviously, this goes on top on the branch for-3.15/ll-driver-new-callbacks
>>
>> Benjamin Tissoires (11):
>> HID: uHID: implement .raw_request
>> HID: i2c-hid: implement ll_driver transport-layer callbacks
>> HID: add inliners for ll_driver transport-layer callbacks
>> HID: logitech-dj: remove hidinput_input_event
>> HID: HIDp: remove hidp_hidinput_event
>> HID: remove hidinput_input_event handler
>> HID: HIDp: remove duplicated coded
>> HID: usbhid: remove duplicated code
>> HID: remove hid_get_raw_report in struct hid_device
>> HID: introduce helper to access hid_output_raw_report()
>> HID: move hid_output_raw_report to hid_ll_driver
>>
>> drivers/hid/hid-input.c | 12 ++---
>> drivers/hid/hid-lg.c | 6 ++-
>> drivers/hid/hid-logitech-dj.c | 101 +++++++++++++---------------------
>> drivers/hid/hid-magicmouse.c | 2 +-
>> drivers/hid/hid-sony.c | 19 +++++--
>> drivers/hid/hid-thingm.c | 4 +-
>> drivers/hid/hid-wacom.c | 16 +++---
>> drivers/hid/hid-wiimote-core.c | 4 +-
>> drivers/hid/hidraw.c | 11 ++--
>> drivers/hid/i2c-hid/i2c-hid.c | 27 +++++++++-
>> drivers/hid/uhid.c | 20 ++++++-
>> drivers/hid/usbhid/hid-core.c | 67 +++++------------------
>> include/linux/hid.h | 77 ++++++++++++++++++++++----
>> net/bluetooth/hidp/core.c | 119 +++++------------------------------------
>> 14 files changed, 213 insertions(+), 272 deletions(-)
>>
>> --
>> 1.8.3.1
>>
^ permalink raw reply
* Re: [PATCH 01/11] HID: uHID: implement .raw_request
From: Benjamin Tissoires @ 2014-02-03 19:07 UTC (permalink / raw)
To: David Herrmann
Cc: Benjamin Tissoires, Jiri Kosina, Frank Praznik,
open list:HID CORE LAYER, linux-kernel
In-Reply-To: <CANq1E4RD0OVTSVOXHPTchop0Y0q0p1_NEZZexSE4kvkp-17TKw@mail.gmail.com>
On Mon, Feb 3, 2014 at 10:26 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Sun, Feb 2, 2014 at 5:50 AM, Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
>> It was missing, so adding it.
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>> ---
>> drivers/hid/uhid.c | 17 +++++++++++++++++
>> 1 file changed, 17 insertions(+)
>>
>> diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
>> index f5a2b19..438c9f1 100644
>> --- a/drivers/hid/uhid.c
>> +++ b/drivers/hid/uhid.c
>> @@ -270,6 +270,22 @@ static int uhid_hid_output_report(struct hid_device *hid, __u8 *buf,
>> return count;
>> }
>>
>> +static int uhid_raw_request(struct hid_device *hid, unsigned char reportnum,
>> + __u8 *buf, size_t len, unsigned char rtype,
>> + int reqtype)
>> +{
>> + switch (reqtype) {
>> + case HID_REQ_GET_REPORT:
>> + return uhid_hid_get_raw(hid, reportnum, buf, len, rtype);
>> + case HID_REQ_SET_REPORT:
>> + if (buf[0] != reportnum)
>> + return -EINVAL;
>> + return uhid_hid_output_raw(hid, buf, len, rtype);
>
> But that one looks wrong. UHID does not have any SET_REPORT query in
> the protocol, yet. You turn a SET_REPORT into an OUTPUT_REPORT here.
> So if user-space gets the UHID_OUTPUT event, it doesn't know whether
> to send a SET_REPORT@ctrl to the device, or an async
> OUTPUT_REPORT@intr. This at least matters for low-energy BT in bluez,
> which uses uhid.
right. So we can drop this for now.
>
> So we'd have to add an UHID_SET_REPORT event. Note that currently
> UHID_FEATURE is the equivalent of UHID_GET_REPORT, but just horribly
> named..
ouch. I think this is something which can be fixed quite easily (by
marking UHID_FEATURE obsolete and creating the two events).
However, I don't think I will have the time to make the change and do
proper testings in the next few days/weeks.
Can somebody else take this?
Cheers,
Benjamin
>
> Thanks
> David
>
>> + default:
>> + return -EIO;
>> + }
>> +}
>> +
>> static struct hid_ll_driver uhid_hid_driver = {
>> .start = uhid_hid_start,
>> .stop = uhid_hid_stop,
>> @@ -277,6 +293,7 @@ static struct hid_ll_driver uhid_hid_driver = {
>> .close = uhid_hid_close,
>> .parse = uhid_hid_parse,
>> .output_report = uhid_hid_output_report,
>> + .raw_request = uhid_raw_request,
>> };
>>
>> #ifdef CONFIG_COMPAT
>> --
>> 1.8.3.1
>>
^ permalink raw reply
* Re: [PATCH 02/11] HID: i2c-hid: implement ll_driver transport-layer callbacks
From: Benjamin Tissoires @ 2014-02-03 19:00 UTC (permalink / raw)
To: David Herrmann, Andrew Duggan
Cc: Benjamin Tissoires, Jiri Kosina, Frank Praznik,
open list:HID CORE LAYER, linux-kernel
In-Reply-To: <CANq1E4S0WjuqW3coJDKqH9Gm5JWwb_W_CEU8Ety15m0CjhOqZQ@mail.gmail.com>
On Mon, Feb 3, 2014 at 11:04 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Sun, Feb 2, 2014 at 5:50 AM, Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
>> Add output_report and raw_request to i2c-hid.
>> Hopefully, we will manage to have the same transport level between
>> all the transport drivers.
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>> ---
>> drivers/hid/i2c-hid/i2c-hid.c | 24 ++++++++++++++++++++++++
>> 1 file changed, 24 insertions(+)
>>
>> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
>> index ce68a12..5099f1f 100644
>> --- a/drivers/hid/i2c-hid/i2c-hid.c
>> +++ b/drivers/hid/i2c-hid/i2c-hid.c
>> @@ -574,6 +574,28 @@ static int i2c_hid_output_raw_report(struct hid_device *hid, __u8 *buf,
>> return ret;
>> }
>>
>> +static int i2c_hid_output_report(struct hid_device *hid, __u8 *buf,
>> + size_t count)
>> +{
>> + return i2c_hid_output_raw_report(hid, buf, count, HID_OUTPUT_REPORT);
>> +}
>> +
>> +static int i2c_hid_raw_request(struct hid_device *hid, unsigned char reportnum,
>> + __u8 *buf, size_t len, unsigned char rtype,
>> + int reqtype)
>> +{
>> + switch (reqtype) {
>> + case HID_REQ_GET_REPORT:
>> + return i2c_hid_get_raw_report(hid, reportnum, buf, len, rtype);
>> + case HID_REQ_SET_REPORT:
>> + if (buf[0] != reportnum)
>> + return -EINVAL;
>> + return i2c_hid_output_raw_report(hid, buf, len, rtype);
>
> I just skimmed the I2C-HID specs and it defines three methods for
> input/output reports:
>
> 1) Section 6.2:
> raw async output-reports can be sent by writing the data at any time
> to wOutputRegister.
> This should be used as method for hid->output_report().
>
> 2) Section 7.1:
> SET_REPORT can be issued by writing the right OPCODE + report-ID into
> wCommandRegister and the data into wDataRegister.
> This should be used as method for hid->raw_request() + HID_REQ_SET_REPORT.
>
> 3) Section 7.1:
> GET_REPORT can be issued by writing the right OPCODE + report-ID into
> wCommandRegister and then waiting for the device to write the data
> into wDataRegister.
> This should be used for hid->raw_request() + HID_REQ_GET_REPORT
>
>
> The GET_REPORT implementation looks fine to me, but the
> i2c_hid_set_report() seems to support both 1) and 2) depending on the
> passed type and capabilities:
> - it uses 2) for FEATURE_REPORT reports or if the max OUTPUT_LENGTH is 0
> - it uses 1) otherwise
>
> I'm not sure whether the i2c-hid-spec mandates this behavior, so I am
> not saying it's wrong. I just wanna understand what we do here. So if
> we use hid->output_report() with HID_FEATURE_REPORT, the current code
> turns this into a SET_REPORT. Likewise, an hid->raw_request() with
> HID_REQ_SET_REPORT but with HID_OUTPUT_REPORT turns into an
> output-report.
Ok, just for the record (I think that you already answered this in the
4/11 review):
this behavior has been added in commit
811adb9622de310efbb661531c3ec0ae5d2b2bc0 for Synaptics so they can
talk to their HID/I2C firmware.
The use they are making is through hdev->hid_output_raw_report(), and
at that time output_report() did not exist.
I guess that with the new API (output_report() and raw_request() ) we
can revert this and it will not bother them given that rmi-hid.c is
still not upstream.
I have no clue if anyone else is using hdev->hid_output_raw_report()
for I2C devices, but we could still try to clean this right now given
that those devices are not well spread (besides some hid-multitouch
touchscreens which do not use OUTPUT reports).
I am still a little bit concerned by hidraw not able to call
set_report in this case, but this could be fixed also by adding an
ioctl.
How does that sound?
Cheers,
Benjamin
>
> I'd rather expect this behavior:
>
> hid->output_report() should always do this:
> args[index++] = outputRegister & 0xFF;
> args[index++] = outputRegister >> 8;
> hidcmd = &hid_no_cmd;
>
> while hid->raw_request() should always do this:
> args[index++] = dataRegister & 0xFF;
> args[index++] = dataRegister >> 8;
>
> The special case for maxOutputLength==0 seems fine to me, but the
> "reportType == 0x03" looks weird.
>
>
> Thanks
> David
>
>> + default:
>> + return -EIO;
>> + }
>> +}
>> +
>> static void i2c_hid_request(struct hid_device *hid, struct hid_report *rep,
>> int reqtype)
>> {
>> @@ -761,6 +783,8 @@ static struct hid_ll_driver i2c_hid_ll_driver = {
>> .close = i2c_hid_close,
>> .power = i2c_hid_power,
>> .request = i2c_hid_request,
>> + .output_report = i2c_hid_output_report,
>> + .raw_request = i2c_hid_raw_request,
>> };
>>
>> static int i2c_hid_init_irq(struct i2c_client *client)
>> --
>> 1.8.3.1
>>
^ permalink raw reply
* Re: [PATCH 0/6] HID: Add a stable method for retrieving the client MAC address of a HID device
From: Frank Praznik @ 2014-02-03 18:33 UTC (permalink / raw)
To: David Herrmann
Cc: Dmitry Torokhov, Jiri Kosina, open list:HID CORE LAYER,
Benjamin Tissoires
In-Reply-To: <CANq1E4TxLBzhyf5b5SD5gP1iPRNZSLEg4-uwaqAhybovZn62uQ@mail.gmail.com>
On Mon, Feb 3, 2014 at 1:06 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Mon, Feb 3, 2014 at 6:57 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
>> On Mon, Feb 03, 2014 at 06:45:52PM +0100, David Herrmann wrote:
>>> Hi
>>>
>>> Adding Dmitry+Jiri, maybe they can clarify this.
>>>
>>> [snip]
>>> >>>
>>> >>> Yes
>>> >>>
>>> >>>> hidp sets it to point to the hci_conn struct from which src address for the
>>> >>>> Bluetooth connection can be obtained, but making assumptions about an opaque
>>> >>>> pointer like that seems dangerous. Is the parent pointer guaranteed to
>>> >>>> point to the hci_conn struct as long as the bus type is Bluetooth?
>>> >>>
>>> >>> And nope. If you use uhid, then the parent will not be a hci_conn.
>>> >>>
>>> >>> With enough guards, you should be able to use it, but it's not ideal I agree.
>>> >>> I really want to have David's opinion regarding the UNIQ field. IMO,
>>> >>> this seems to be the most transport-agnostic way of doing it.
>>> >>
>>> >> UNIQ is definitely wrong. It is used to assign a run-time *unique*
>>> >> value to the connection. So ideally it should be different if a device
>>> >> is reconnected. The PHYS field is actually used to identify a physical
>>> >> device. So please, if we're doing this, then we should do it via PHYS.
>>> >>
>>> >> I'm fine with hard-coding PHYS as bt-address for BT-HID, but please
>>> >> add a comment to hidp_setup_hid() in net/bluetooth/hidp/core.c before
>>> >> doing the snprintf() there.
>>> >>
>>> >> The reason why I disliked hard-coding this behavior is that if a
>>> >> single BT-device is connected twice to us, we usually want two
>>> >> different PHYS entries for both depending on which service this
>>> >> connection provides. However, this seems like an unlikely and
>>> >> overengineered problem so lets not do that. Furthermore, while BT-HID
>>> >> would allow such setups with some hacks, it isn't supported in a clean
>>> >> way. So yeah, I'm actually fine with using PHYS for that.
>>> >>
>>> >> Thanks
>>> >> David
>>> >
>>> > PHYS should definitely be changed if this is the case because right
>>> > now it is set to the MAC of the host adapter which means that it's the
>>> > same for every Bluetooth device and connection. I believe that the
>>> > hidraw documentation also specifies that for Bluetooth devices the
>>> > HIDIOCGRAWPHYS ioctl should return a string with the MAC address of
>>> > the associated device rather than that of the host adapter as the
>>> > current behavior does.
>>>
>>> Oh, yes, nice catch!
>>> Ok, maybe we need to clear up what PHYS and UNIQ do before relying on
>>> them. I thought, they were defined as:
>>>
>>> PHYS: A string describing the physical device. It should be the same
>>> if a device reconnects. It can be used by user-space to track devices
>>> across disconnects
>>>
>>> UNIQ: A string describing the current connection to a device. If the
>>> device reconnects, the UNIQ string should change. It can be used by
>>> user-space to track a single device-session.
>>>
>>> afaik both strings have no common format and drivers are free to
>>> provide any kind of information, as long as it follows the given
>>> rules. That's why I disliked the idea of relying on them and parsing
>>> them. But maybe I just have no idea what the original intention was
>>> behind them?
>>
>> PHYS: describes physical connection of the device in a given box,
>> supposed to be unique within a box.
>>
>> UNIQ: unique identifier (if exists) assigned to the device, should
>> ideally be unique globally and should not change when device is moved
>> within a box out between boxes. Think serial number in USB descriptors.
>
> Thanks, so it's basically the other way around as I thought. So I
> think using UNIQ is the way to go, sorry for the confusion.
>
> Thanks
> David
Thanks for clearing this up. It sounds like UNIQ should be safe for
getting the client MAC of a Bluetooth device as long as the
appropriate sanity checks are performed to make sure that it's not a
custom string from a uhid device. Or, going back to the earlier
question, is there still some reason as to why HIDP might change the
behavior of UNIQ in the future?
^ permalink raw reply
* Re: [PATCH 0/6] HID: Add a stable method for retrieving the client MAC address of a HID device
From: David Herrmann @ 2014-02-03 18:06 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Frank Praznik, Jiri Kosina, open list:HID CORE LAYER,
Benjamin Tissoires
In-Reply-To: <20140203175704.GA2229@core.coreip.homeip.net>
Hi
On Mon, Feb 3, 2014 at 6:57 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Mon, Feb 03, 2014 at 06:45:52PM +0100, David Herrmann wrote:
>> Hi
>>
>> Adding Dmitry+Jiri, maybe they can clarify this.
>>
>> [snip]
>> >>>
>> >>> Yes
>> >>>
>> >>>> hidp sets it to point to the hci_conn struct from which src address for the
>> >>>> Bluetooth connection can be obtained, but making assumptions about an opaque
>> >>>> pointer like that seems dangerous. Is the parent pointer guaranteed to
>> >>>> point to the hci_conn struct as long as the bus type is Bluetooth?
>> >>>
>> >>> And nope. If you use uhid, then the parent will not be a hci_conn.
>> >>>
>> >>> With enough guards, you should be able to use it, but it's not ideal I agree.
>> >>> I really want to have David's opinion regarding the UNIQ field. IMO,
>> >>> this seems to be the most transport-agnostic way of doing it.
>> >>
>> >> UNIQ is definitely wrong. It is used to assign a run-time *unique*
>> >> value to the connection. So ideally it should be different if a device
>> >> is reconnected. The PHYS field is actually used to identify a physical
>> >> device. So please, if we're doing this, then we should do it via PHYS.
>> >>
>> >> I'm fine with hard-coding PHYS as bt-address for BT-HID, but please
>> >> add a comment to hidp_setup_hid() in net/bluetooth/hidp/core.c before
>> >> doing the snprintf() there.
>> >>
>> >> The reason why I disliked hard-coding this behavior is that if a
>> >> single BT-device is connected twice to us, we usually want two
>> >> different PHYS entries for both depending on which service this
>> >> connection provides. However, this seems like an unlikely and
>> >> overengineered problem so lets not do that. Furthermore, while BT-HID
>> >> would allow such setups with some hacks, it isn't supported in a clean
>> >> way. So yeah, I'm actually fine with using PHYS for that.
>> >>
>> >> Thanks
>> >> David
>> >
>> > PHYS should definitely be changed if this is the case because right
>> > now it is set to the MAC of the host adapter which means that it's the
>> > same for every Bluetooth device and connection. I believe that the
>> > hidraw documentation also specifies that for Bluetooth devices the
>> > HIDIOCGRAWPHYS ioctl should return a string with the MAC address of
>> > the associated device rather than that of the host adapter as the
>> > current behavior does.
>>
>> Oh, yes, nice catch!
>> Ok, maybe we need to clear up what PHYS and UNIQ do before relying on
>> them. I thought, they were defined as:
>>
>> PHYS: A string describing the physical device. It should be the same
>> if a device reconnects. It can be used by user-space to track devices
>> across disconnects
>>
>> UNIQ: A string describing the current connection to a device. If the
>> device reconnects, the UNIQ string should change. It can be used by
>> user-space to track a single device-session.
>>
>> afaik both strings have no common format and drivers are free to
>> provide any kind of information, as long as it follows the given
>> rules. That's why I disliked the idea of relying on them and parsing
>> them. But maybe I just have no idea what the original intention was
>> behind them?
>
> PHYS: describes physical connection of the device in a given box,
> supposed to be unique within a box.
>
> UNIQ: unique identifier (if exists) assigned to the device, should
> ideally be unique globally and should not change when device is moved
> within a box out between boxes. Think serial number in USB descriptors.
Thanks, so it's basically the other way around as I thought. So I
think using UNIQ is the way to go, sorry for the confusion.
Thanks
David
^ permalink raw reply
* Re: [PATCH 0/6] HID: Add a stable method for retrieving the client MAC address of a HID device
From: Dmitry Torokhov @ 2014-02-03 17:57 UTC (permalink / raw)
To: David Herrmann
Cc: Frank Praznik, Jiri Kosina, open list:HID CORE LAYER,
Benjamin Tissoires
In-Reply-To: <CANq1E4Q+oaovc-G9D07hSKQxE4SO_ziKLbKnRH8cH9NwV3YXnQ@mail.gmail.com>
On Mon, Feb 03, 2014 at 06:45:52PM +0100, David Herrmann wrote:
> Hi
>
> Adding Dmitry+Jiri, maybe they can clarify this.
>
> [snip]
> >>>
> >>> Yes
> >>>
> >>>> hidp sets it to point to the hci_conn struct from which src address for the
> >>>> Bluetooth connection can be obtained, but making assumptions about an opaque
> >>>> pointer like that seems dangerous. Is the parent pointer guaranteed to
> >>>> point to the hci_conn struct as long as the bus type is Bluetooth?
> >>>
> >>> And nope. If you use uhid, then the parent will not be a hci_conn.
> >>>
> >>> With enough guards, you should be able to use it, but it's not ideal I agree.
> >>> I really want to have David's opinion regarding the UNIQ field. IMO,
> >>> this seems to be the most transport-agnostic way of doing it.
> >>
> >> UNIQ is definitely wrong. It is used to assign a run-time *unique*
> >> value to the connection. So ideally it should be different if a device
> >> is reconnected. The PHYS field is actually used to identify a physical
> >> device. So please, if we're doing this, then we should do it via PHYS.
> >>
> >> I'm fine with hard-coding PHYS as bt-address for BT-HID, but please
> >> add a comment to hidp_setup_hid() in net/bluetooth/hidp/core.c before
> >> doing the snprintf() there.
> >>
> >> The reason why I disliked hard-coding this behavior is that if a
> >> single BT-device is connected twice to us, we usually want two
> >> different PHYS entries for both depending on which service this
> >> connection provides. However, this seems like an unlikely and
> >> overengineered problem so lets not do that. Furthermore, while BT-HID
> >> would allow such setups with some hacks, it isn't supported in a clean
> >> way. So yeah, I'm actually fine with using PHYS for that.
> >>
> >> Thanks
> >> David
> >
> > PHYS should definitely be changed if this is the case because right
> > now it is set to the MAC of the host adapter which means that it's the
> > same for every Bluetooth device and connection. I believe that the
> > hidraw documentation also specifies that for Bluetooth devices the
> > HIDIOCGRAWPHYS ioctl should return a string with the MAC address of
> > the associated device rather than that of the host adapter as the
> > current behavior does.
>
> Oh, yes, nice catch!
> Ok, maybe we need to clear up what PHYS and UNIQ do before relying on
> them. I thought, they were defined as:
>
> PHYS: A string describing the physical device. It should be the same
> if a device reconnects. It can be used by user-space to track devices
> across disconnects
>
> UNIQ: A string describing the current connection to a device. If the
> device reconnects, the UNIQ string should change. It can be used by
> user-space to track a single device-session.
>
> afaik both strings have no common format and drivers are free to
> provide any kind of information, as long as it follows the given
> rules. That's why I disliked the idea of relying on them and parsing
> them. But maybe I just have no idea what the original intention was
> behind them?
PHYS: describes physical connection of the device in a given box,
supposed to be unique within a box.
UNIQ: unique identifier (if exists) assigned to the device, should
ideally be unique globally and should not change when device is moved
within a box out between boxes. Think serial number in USB descriptors.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 0/6] HID: Add a stable method for retrieving the client MAC address of a HID device
From: David Herrmann @ 2014-02-03 17:45 UTC (permalink / raw)
To: Frank Praznik, Dmitry Torokhov, Jiri Kosina
Cc: open list:HID CORE LAYER, Benjamin Tissoires
In-Reply-To: <CAH+9E3oNifQgGDOp9POpxMm2uaMo3AUdRttC7L=WtGOyU8_nBA@mail.gmail.com>
Hi
Adding Dmitry+Jiri, maybe they can clarify this.
[snip]
>>>
>>> Yes
>>>
>>>> hidp sets it to point to the hci_conn struct from which src address for the
>>>> Bluetooth connection can be obtained, but making assumptions about an opaque
>>>> pointer like that seems dangerous. Is the parent pointer guaranteed to
>>>> point to the hci_conn struct as long as the bus type is Bluetooth?
>>>
>>> And nope. If you use uhid, then the parent will not be a hci_conn.
>>>
>>> With enough guards, you should be able to use it, but it's not ideal I agree.
>>> I really want to have David's opinion regarding the UNIQ field. IMO,
>>> this seems to be the most transport-agnostic way of doing it.
>>
>> UNIQ is definitely wrong. It is used to assign a run-time *unique*
>> value to the connection. So ideally it should be different if a device
>> is reconnected. The PHYS field is actually used to identify a physical
>> device. So please, if we're doing this, then we should do it via PHYS.
>>
>> I'm fine with hard-coding PHYS as bt-address for BT-HID, but please
>> add a comment to hidp_setup_hid() in net/bluetooth/hidp/core.c before
>> doing the snprintf() there.
>>
>> The reason why I disliked hard-coding this behavior is that if a
>> single BT-device is connected twice to us, we usually want two
>> different PHYS entries for both depending on which service this
>> connection provides. However, this seems like an unlikely and
>> overengineered problem so lets not do that. Furthermore, while BT-HID
>> would allow such setups with some hacks, it isn't supported in a clean
>> way. So yeah, I'm actually fine with using PHYS for that.
>>
>> Thanks
>> David
>
> PHYS should definitely be changed if this is the case because right
> now it is set to the MAC of the host adapter which means that it's the
> same for every Bluetooth device and connection. I believe that the
> hidraw documentation also specifies that for Bluetooth devices the
> HIDIOCGRAWPHYS ioctl should return a string with the MAC address of
> the associated device rather than that of the host adapter as the
> current behavior does.
Oh, yes, nice catch!
Ok, maybe we need to clear up what PHYS and UNIQ do before relying on
them. I thought, they were defined as:
PHYS: A string describing the physical device. It should be the same
if a device reconnects. It can be used by user-space to track devices
across disconnects
UNIQ: A string describing the current connection to a device. If the
device reconnects, the UNIQ string should change. It can be used by
user-space to track a single device-session.
afaik both strings have no common format and drivers are free to
provide any kind of information, as long as it follows the given
rules. That's why I disliked the idea of relying on them and parsing
them. But maybe I just have no idea what the original intention was
behind them?
Thanks
David
^ permalink raw reply
* [PATCH] HID: Add Apple wireless keyboard 2011 JIS model support
From: Huei-Horng Yo @ 2014-02-03 17:41 UTC (permalink / raw)
To: linux-input
Hello,
I bought an Apple wireless keyboard 2011 JIS model,
the 'fn' key isn't work as expected in Linux 3.12.9.
What my patch mainly does is to add support for that model.
Before applying this patch, the keyboard behaves like a general keyboard,
its 'fn' key has no function to be used with arrow keys to do Home, End,
Page Up and Page Down.
I found that this is because the HID IDs list hasn't it in vendor ID
0x05ac (APPLE) section,
so I add the production ID 0x0257 (got the value by using bluetoothctl)
in drivers/hid/hid-ids.h
and add the corresponding codes in drivers/hid/hid-apple.c &
drivers/hid/hid-core.c.
Thanks,
Huei-Horng Yo
P.S. Also reported in Bugzilla:https://bugzilla.kernel.org/show_bug.cgi?id=69681 <https://bugzilla.kernel.org/show_bug.cgi?id=69681>
apple-alu-wireless-kbd-jis.patch
Signed-off-by: Huei-Horng Yo <hiroshi@ghostsinthelab.org>
diff -uprN linux-3.12.vanilla/drivers/hid/hid-apple.c linux-3.12/drivers/hid/hid-apple.c
--- linux-3.12.vanilla/drivers/hid/hid-apple.c 2013-11-04 07:41:51.000000000 +0800
+++ linux-3.12/drivers/hid/hid-apple.c 2014-02-01 01:15:37.532768035 +0800
@@ -447,6 +447,9 @@ static const struct hid_device_id apple_
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_ALU_WIRELESS_2011_ANSI),
.driver_data = APPLE_NUMLOCK_EMULATION | APPLE_HAS_FN },
+ { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE,
+ USB_DEVICE_ID_APPLE_ALU_WIRELESS_2011_JIS),
+ .driver_data = APPLE_NUMLOCK_EMULATION | APPLE_HAS_FN },
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_ALU_WIRELESS_JIS),
.driver_data = APPLE_NUMLOCK_EMULATION | APPLE_HAS_FN },
{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_WELLSPRING_ANSI),
diff -uprN linux-3.12.vanilla/drivers/hid/hid-core.c linux-3.12/drivers/hid/hid-core.c
--- linux-3.12.vanilla/drivers/hid/hid-core.c 2013-11-04 07:41:51.000000000 +0800
+++ linux-3.12/drivers/hid/hid-core.c 2014-02-01 01:15:37.532768035 +0800
@@ -1680,6 +1680,7 @@ static const struct hid_device_id hid_ha
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_ALU_WIRELESS_2009_JIS) },
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_ALU_WIRELESS_2011_ANSI) },
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_ALU_WIRELESS_2011_ISO) },
+ { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_ALU_WIRELESS_2011_JIS) },
{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_FOUNTAIN_TP_ONLY) },
{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_GEYSER1_TP_ONLY) },
{ HID_USB_DEVICE(USB_VENDOR_ID_AUREAL, USB_DEVICE_ID_AUREAL_W01RN) },
diff -uprN linux-3.12.vanilla/drivers/hid/hid-ids.h linux-3.12/drivers/hid/hid-ids.h
--- linux-3.12.vanilla/drivers/hid/hid-ids.h 2013-11-04 07:41:51.000000000 +0800
+++ linux-3.12/drivers/hid/hid-ids.h 2014-02-01 01:15:37.532768035 +0800
@@ -135,6 +135,7 @@
#define USB_DEVICE_ID_APPLE_ALU_WIRELESS_2009_JIS 0x023b
#define USB_DEVICE_ID_APPLE_ALU_WIRELESS_2011_ANSI 0x0255
#define USB_DEVICE_ID_APPLE_ALU_WIRELESS_2011_ISO 0x0256
+#define USB_DEVICE_ID_APPLE_ALU_WIRELESS_2011_JIS 0x0257
#define USB_DEVICE_ID_APPLE_WELLSPRING8_ANSI 0x0290
#define USB_DEVICE_ID_APPLE_WELLSPRING8_ISO 0x0291
#define USB_DEVICE_ID_APPLE_WELLSPRING8_JIS 0x0292
^ permalink raw reply
* Re: [PATCH 7/7] Input: xpad: properly name the LED class devices
From: David Herrmann @ 2014-02-03 17:39 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Dmitry Torokhov, open list:HID CORE LAYER,
Pierre-Loup A. Griffais
In-Reply-To: <1391173414-6199-8-git-send-email-gregkh@linuxfoundation.org>
Hi
On Fri, Jan 31, 2014 at 2:03 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> Don't just increment the LED device number, but use the joystick id so
> that you have a chance to associate the LED device to the correct xpad
> device by the name, instead of having to use the sysfs tree, which
> really doesn't work.
>
> Cc: "Pierre-Loup A. Griffais" <pgriffais@valvesoftware.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
> drivers/input/joystick/xpad.c | 40 +++++++++++++++++-----------------------
> 1 file changed, 17 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> index d342d41a7a0d..ae156de46a12 100644
> --- a/drivers/input/joystick/xpad.c
> +++ b/drivers/input/joystick/xpad.c
> @@ -781,8 +781,6 @@ static void xpad_led_set(struct led_classdev *led_cdev,
>
> static int xpad_led_probe(struct usb_xpad *xpad)
> {
> - static atomic_t led_seq = ATOMIC_INIT(0);
> - long led_no;
> struct xpad_led *led;
> struct led_classdev *led_cdev;
> int error;
> @@ -794,9 +792,7 @@ static int xpad_led_probe(struct usb_xpad *xpad)
> if (!led)
> return -ENOMEM;
>
> - led_no = (long)atomic_inc_return(&led_seq) - 1;
> -
> - snprintf(led->name, sizeof(led->name), "xpad%ld", led_no);
> + snprintf(led->name, sizeof(led->name), "xpad%d", xpad->joydev_id);
I guess that patch should be dropped, too?
Why not use the usb-interface here? It's quite common to use bt-mac
addresses for BT devices, so something similar for USB seems fine to
me.
Thanks
David
> led->xpad = xpad;
>
> led_cdev = &led->led_cdev;
> @@ -944,16 +940,17 @@ static int xpad_init_input(struct usb_xpad *xpad)
> }
>
> error = xpad_init_ff(xpad);
> - if (error)
> - goto fail_init_ff;
> -
> - error = xpad_led_probe(xpad);
> - if (error)
> - goto fail_init_led;
> + if (error) {
> + input_free_device(input_dev);
> + return error;
> + }
>
> error = input_register_device(xpad->dev);
> - if (error)
> - goto fail_input_register;
> + if (error) {
> + input_ff_destroy(input_dev);
> + input_free_device(input_dev);
> + return error;
> + }
>
> joydev_dev = device_find_child(&xpad->dev->dev, NULL,
> xpad_find_joydev);
> @@ -966,17 +963,14 @@ static int xpad_init_input(struct usb_xpad *xpad)
> xpad_send_led_command(xpad, (xpad->joydev_id % 4) + 2);
> }
>
> - return 0;
> -
> -fail_input_register:
> - xpad_led_disconnect(xpad);
> -
> -fail_init_led:
> - input_ff_destroy(input_dev);
> + error = xpad_led_probe(xpad);
> + if (error) {
> + input_ff_destroy(input_dev);
> + input_unregister_device(input_dev);
> + return error;
> + }
>
> -fail_init_ff:
> - input_free_device(input_dev);
> - return error;
> + return 0;
> }
>
> static int xpad_probe(struct usb_interface *intf, const struct usb_device_id *id)
> --
> 1.8.5.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 6/7] Input: xpad: handle "present" and "gone" correctly
From: David Herrmann @ 2014-02-03 17:37 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Dmitry Torokhov, open list:HID CORE LAYER,
Pierre-Loup A. Griffais
In-Reply-To: <1391173414-6199-7-git-send-email-gregkh@linuxfoundation.org>
Hi
On Fri, Jan 31, 2014 at 2:03 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> From: "Pierre-Loup A. Griffais" <pgriffais@valvesoftware.com>
>
> Handle the "a new device is present" message properly by dynamically
> creating the input device at this point in time. This requires a
> workqueue as we are in interrupt context when we learn about this.
>
> Also properly disconnect any devices that we are told are removed.
>
> Signed-off-by: "Pierre-Loup A. Griffais" <pgriffais@valvesoftware.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
> drivers/input/joystick/xpad.c | 50 ++++++++++++++++++++++++++++++++++---------
> 1 file changed, 40 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> index 7a07b95790d7..d342d41a7a0d 100644
> --- a/drivers/input/joystick/xpad.c
> +++ b/drivers/input/joystick/xpad.c
> @@ -293,8 +293,11 @@ struct usb_xpad {
> int xtype; /* type of xbox device */
> int joydev_id; /* the minor of the device */
> const char *name; /* name of the device */
> + struct work_struct work; /* to init/remove device from callback */
> };
>
> +static int xpad_init_input(struct usb_xpad *xpad);
> +
> /*
> * xpad_process_packet
> *
> @@ -437,6 +440,22 @@ static void xpad360_process_packet(struct usb_xpad *xpad,
> input_sync(dev);
> }
>
> +static void presence_work_function(struct work_struct *work)
> +{
> + struct usb_xpad *xpad = container_of(work, struct usb_xpad, work);
> + int error;
> +
> + if (xpad->pad_present) {
> + error = xpad_init_input(xpad);
> + if (error) {
> + /* complain only, not much else we can do here */
> + dev_err(&xpad->dev->dev, "unable to init device\n");
> + }
> + } else {
> + input_unregister_device(xpad->dev);
> + }
> +}
> +
> /*
> * xpad360w_process_packet
> *
> @@ -451,16 +470,22 @@ static void xpad360_process_packet(struct usb_xpad *xpad,
> * 01.1 - Pad state (Bytes 4+) valid
> *
> */
> -
> static void xpad360w_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char *data)
> {
> /* Presence change */
> if (data[0] & 0x08) {
> if (data[1] & 0x80) {
> - xpad->pad_present = 1;
> - usb_submit_urb(xpad->bulk_out, GFP_ATOMIC);
> - } else
> - xpad->pad_present = 0;
> + if (!xpad->pad_present) {
> + xpad->pad_present = 1;
> + usb_submit_urb(xpad->bulk_out, GFP_ATOMIC);
> + schedule_work(&xpad->work);
> + }
> + } else {
> + if (xpad->pad_present) {
> + xpad->pad_present = 0;
> + schedule_work(&xpad->work);
> + }
> + }
> }
>
> /* Valid pad data */
> @@ -507,10 +532,13 @@ static void xpad_irq_in(struct urb *urb)
> }
>
> exit:
> - retval = usb_submit_urb(urb, GFP_ATOMIC);
> - if (retval)
> - dev_err(dev, "%s - usb_submit_urb failed with result %d\n",
> - __func__, retval);
> + if (xpad->pad_present) {
> + retval = usb_submit_urb(urb, GFP_ATOMIC);
> + if (retval)
> + dev_err(dev,
> + "%s - usb_submit_urb failed with result %d\n",
> + __func__, retval);
> + }
> }
>
> static void xpad_bulk_out(struct urb *urb)
> @@ -991,6 +1019,7 @@ static int xpad_probe(struct usb_interface *intf, const struct usb_device_id *id
> xpad->mapping = xpad_device[i].mapping;
> xpad->xtype = xpad_device[i].xtype;
> xpad->name = xpad_device[i].name;
> + INIT_WORK(&xpad->work, presence_work_function);
>
> if (xpad->xtype == XTYPE_UNKNOWN) {
> if (intf->cur_altsetting->desc.bInterfaceClass == USB_CLASS_VENDOR_SPEC) {
> @@ -1136,7 +1165,8 @@ static void xpad_disconnect(struct usb_interface *intf)
> struct usb_xpad *xpad = usb_get_intfdata (intf);
>
> xpad_led_disconnect(xpad);
> - input_unregister_device(xpad->dev);
You need cancel_work_sync(&xpad->work) here.
And I think you need to do that *after* killing the urbs. Otherwise,
the work might get rescheduled in parallel to this disconnect
callback.
Thanks
David
> + if (xpad->pad_present)
> + input_unregister_device(xpad->dev);
> xpad_deinit_output(xpad);
>
> if (xpad->xtype == XTYPE_XBOX360W) {
> --
> 1.8.5.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 2/7] Input: xpad: set the LEDs properly on XBox Wireless controllers
From: David Herrmann @ 2014-02-03 17:31 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Dmitry Torokhov, open list:HID CORE LAYER,
Pierre-Loup A. Griffais
In-Reply-To: <1391173414-6199-3-git-send-email-gregkh@linuxfoundation.org>
Hi
On Fri, Jan 31, 2014 at 2:03 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> From: "Pierre-Loup A. Griffais" <pgriffais@valvesoftware.com>
>
> Add the logic to set the LEDs on XBox Wireless controllers. Command
> sequence found by sniffing the Windows data stream when plugging the
> device in.
>
> Signed-off-by: "Pierre-Loup A. Griffais" <pgriffais@valvesoftware.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
> drivers/input/joystick/xpad.c | 32 +++++++++++++++++++++++++++-----
> 1 file changed, 27 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> index 517829f6a58b..aabff9140aaa 100644
> --- a/drivers/input/joystick/xpad.c
> +++ b/drivers/input/joystick/xpad.c
> @@ -715,15 +715,37 @@ struct xpad_led {
>
> static void xpad_send_led_command(struct usb_xpad *xpad, int command)
> {
> - if (command >= 0 && command < 14) {
> - mutex_lock(&xpad->odata_mutex);
> + if (command > 15)
> + return;
That's really weird. The "command" argument is used to control which
of the LEDs are enabled, but the underlying led_classdev passes the
brightness value here. Shouldn't we have one led_classdev device for
each LED and make "max_brightness"==1 so it's a boolean value?
I do that for wiimotes so you end up with 4 sysfs entries, one for each LED.
Anyhow, you change "command < 14" to "command > 15" here, is this
intentional also for the XTYPE_XBOX360 path?
> +
> + mutex_lock(&xpad->odata_mutex);
> +
> + switch (xpad->xtype) {
> + case XTYPE_XBOX360:
> xpad->odata[0] = 0x01;
> xpad->odata[1] = 0x03;
> xpad->odata[2] = command;
> xpad->irq_out->transfer_buffer_length = 3;
> - usb_submit_urb(xpad->irq_out, GFP_KERNEL);
> - mutex_unlock(&xpad->odata_mutex);
> + break;
> + case XTYPE_XBOX360W:
> + xpad->odata[0] = 0x00;
> + xpad->odata[1] = 0x00;
> + xpad->odata[2] = 0x08;
> + xpad->odata[3] = 0x40 + (command % 0x0e);
This basically makes /sys/..../led/brightness a "circular" value here.
Seems weird, but acceptable. But if you bail-out early above with
"command > 15", this here is equivalent to "command & 0x0e", right?
How about removing the "if (command > 15)" above and make both paths
use "(command % 0x0e)"? Anyhow, besides changing the XTYPE_XBOX360
path, patch looks good.
Thanks
David
> + xpad->odata[4] = 0x00;
> + xpad->odata[5] = 0x00;
> + xpad->odata[6] = 0x00;
> + xpad->odata[7] = 0x00;
> + xpad->odata[8] = 0x00;
> + xpad->odata[9] = 0x00;
> + xpad->odata[10] = 0x00;
> + xpad->odata[11] = 0x00;
> + xpad->irq_out->transfer_buffer_length = 12;
> + break;
> }
> +
> + usb_submit_urb(xpad->irq_out, GFP_KERNEL);
> + mutex_unlock(&xpad->odata_mutex);
> }
>
> static void xpad_led_set(struct led_classdev *led_cdev,
> @@ -743,7 +765,7 @@ static int xpad_led_probe(struct usb_xpad *xpad)
> struct led_classdev *led_cdev;
> int error;
>
> - if (xpad->xtype != XTYPE_XBOX360)
> + if (xpad->xtype != XTYPE_XBOX360 && xpad->xtype != XTYPE_XBOX360W)
> return 0;
>
> xpad->led = led = kzalloc(sizeof(struct xpad_led), GFP_KERNEL);
> --
> 1.8.5.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 0/6] HID: Add a stable method for retrieving the client MAC address of a HID device
From: Frank Praznik @ 2014-02-03 17:31 UTC (permalink / raw)
To: open list:HID CORE LAYER
In-Reply-To: <CANq1E4TYrwOhBd6ZVUCzuRyB_8ivaCsZVbBKz6n5hPodHjtOmg@mail.gmail.com>
On Mon, Feb 3, 2014 at 11:24 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Sat, Feb 1, 2014 at 5:28 PM, Benjamin Tissoires
> <benjamin.tissoires@gmail.com> wrote:
>> On Sat, Feb 1, 2014 at 11:06 AM, Frank Praznik <frank.praznik@gmail.com> wrote:
>>> On 1/31/2014 15:45, Benjamin Tissoires wrote:
>>>>
>>>> On Fri, Jan 31, 2014 at 3:04 PM, Frank Praznik <frank.praznik@oh.rr.com>
>>>> wrote:
>>>>>
>>>>> On 1/31/2014 14:10, Benjamin Tissoires wrote:
>>>>>>
>>>>>> Hi Frank,
>>>>>>
>>>>>> just a quick review:
>>>>>>
>>>>>> On Fri, Jan 31, 2014 at 12:32 PM, Frank Praznik
>>>>>> <frank.praznik@oh.rr.com>
>>>>>> wrote:
>>>>>>>
>>>>>>> Currently there is no reliable way for a device module or hidraw
>>>>>>> application to
>>>>>>> retrieve the client MAC address of the associated wireless device.
>>>>>>> This
>>>>>>> series
>>>>>>> of patches adds a stable way of retrieving this information.
>>>>>>
>>>>>> Well, if I look at the uevent of a Bluetooth mouse I have:
>>>>>>
>>>>>> $ cat
>>>>>>
>>>>>> /sys/devices/pci0000\:00/0000\:00\:14.0/usb3/3-2/3-2\:1.0/bluetooth/hci0/hci0\:43/0005\:046D\:B00D.001F/uevent
>>>>>> DRIVER=hid-generic
>>>>>> HID_ID=0005:0000046D:0000B00D
>>>>>> HID_NAME=Ultrathin Touch Mouse
>>>>>> HID_PHYS=00:10:60:ea:df:ae
>>>>>> HID_UNIQ=00:1f:20:96:33:47
>>>>>> MODALIAS=hid:b0005g0001v0000046Dp0000B00D
>>>>>>
>>>>>> I would say that HID_UNIQ is the client MAC address set by hidp, no?
>>>>>> So you don't need to duplicate the info by adding a new field in
>>>>>> hid_device.
>>>>>>
>>>>> In a patch I recently submitted I was using the UNIQ field for retrieving
>>>>> a
>>>>> Bluetooth device MAC address and was warned against it because "UNIQ is a
>>>>> way to provide unique identifiers for devices, but it's not guaranteed to
>>>>> stay the same". HIDP happens to store the MAC in the UNIQ data, but
>>>>> there
>>>>> is no guarantee that it will always be there. With these patches you can
>>>>> be
>>>>> completely sure that the data in client_addr is the client device MAC
>>>>> address.
>>>>
>>>> ok. But still, adding a transport-specific information to hid_device
>>>> is IMO a bad idea. We fought to make HID agnostic of the transport
>>>> layer enough.
>>>>
>>>> David, could you elaborate why you think that UNIQ may change in the
>>>> future regarding BT?
>>>> If the BT MAC address is the same principle of an ethernet MAC
>>>> address, UNIQ seems to fit perfectly well.
>>>>
>>>> Otherwise, you may be able to retrieve the MAC address by using the
>>>> parent of the current device.
>>>>
>>>> Cheers,
>>>> Benjamin
>>>
>>> Are you referring to using the hid_device::dev.parent pointer? I know that
>>
>> Yes
>>
>>> hidp sets it to point to the hci_conn struct from which src address for the
>>> Bluetooth connection can be obtained, but making assumptions about an opaque
>>> pointer like that seems dangerous. Is the parent pointer guaranteed to
>>> point to the hci_conn struct as long as the bus type is Bluetooth?
>>
>> And nope. If you use uhid, then the parent will not be a hci_conn.
>>
>> With enough guards, you should be able to use it, but it's not ideal I agree.
>> I really want to have David's opinion regarding the UNIQ field. IMO,
>> this seems to be the most transport-agnostic way of doing it.
>
> UNIQ is definitely wrong. It is used to assign a run-time *unique*
> value to the connection. So ideally it should be different if a device
> is reconnected. The PHYS field is actually used to identify a physical
> device. So please, if we're doing this, then we should do it via PHYS.
>
> I'm fine with hard-coding PHYS as bt-address for BT-HID, but please
> add a comment to hidp_setup_hid() in net/bluetooth/hidp/core.c before
> doing the snprintf() there.
>
> The reason why I disliked hard-coding this behavior is that if a
> single BT-device is connected twice to us, we usually want two
> different PHYS entries for both depending on which service this
> connection provides. However, this seems like an unlikely and
> overengineered problem so lets not do that. Furthermore, while BT-HID
> would allow such setups with some hacks, it isn't supported in a clean
> way. So yeah, I'm actually fine with using PHYS for that.
>
> Thanks
> David
PHYS should definitely be changed if this is the case because right
now it is set to the MAC of the host adapter which means that it's the
same for every Bluetooth device and connection. I believe that the
hidraw documentation also specifies that for Bluetooth devices the
HIDIOCGRAWPHYS ioctl should return a string with the MAC address of
the associated device rather than that of the host adapter as the
current behavior does.
^ permalink raw reply
* Re: [PATCH 04/11] HID: logitech-dj: remove hidinput_input_event
From: David Herrmann @ 2014-02-03 17:07 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Benjamin Tissoires, Jiri Kosina, Frank Praznik,
open list:HID CORE LAYER, linux-kernel
In-Reply-To: <CAN+gG=EesWpKiz-M4LrLTM0ne+o8=XE3vdQnwBPuHOyEgwEJKw@mail.gmail.com>
Hi
On Mon, Feb 3, 2014 at 5:21 PM, Benjamin Tissoires
<benjamin.tissoires@gmail.com> wrote:
> On Mon, Feb 3, 2014 at 11:06 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
>> Hi
>>
>> On Sun, Feb 2, 2014 at 5:50 AM, Benjamin Tissoires
>> <benjamin.tissoires@redhat.com> wrote:
>>> hid-logitech-dj uses its own ->hidinput_input_event() instead of
>>> the generic binding in hid-input.
>>> Moving the handling of LEDs towards logi_dj_output_hidraw_report()
>>> allows two things:
>>> - remove hidinput_input_event in struct hid_device
>>> - hidraw user space programs can also set the LEDs
>>>
>>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>>> ---
>>> drivers/hid/hid-logitech-dj.c | 99 +++++++++++++++----------------------------
>>> 1 file changed, 35 insertions(+), 64 deletions(-)
>>>
>>> diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
>>> index f45279c..61d2bbf 100644
>>> --- a/drivers/hid/hid-logitech-dj.c
>>> +++ b/drivers/hid/hid-logitech-dj.c
>>> @@ -44,14 +44,6 @@ static const char kbd_descriptor[] = {
>>> 0x19, 0xE0, /* USAGE_MINIMUM (Left Control) */
>>> 0x29, 0xE7, /* USAGE_MAXIMUM (Right GUI) */
>>> 0x81, 0x02, /* INPUT (Data,Var,Abs) */
>>> - 0x95, 0x05, /* REPORT COUNT (5) */
>>> - 0x05, 0x08, /* USAGE PAGE (LED page) */
>>> - 0x19, 0x01, /* USAGE MINIMUM (1) */
>>> - 0x29, 0x05, /* USAGE MAXIMUM (5) */
>>> - 0x91, 0x02, /* OUTPUT (Data, Variable, Absolute) */
>>> - 0x95, 0x01, /* REPORT COUNT (1) */
>>> - 0x75, 0x03, /* REPORT SIZE (3) */
>>> - 0x91, 0x01, /* OUTPUT (Constant) */
>>> 0x95, 0x06, /* REPORT_COUNT (6) */
>>> 0x75, 0x08, /* REPORT_SIZE (8) */
>>> 0x15, 0x00, /* LOGICAL_MINIMUM (0) */
>>> @@ -60,6 +52,18 @@ static const char kbd_descriptor[] = {
>>> 0x19, 0x00, /* USAGE_MINIMUM (no event) */
>>> 0x2A, 0xFF, 0x00, /* USAGE_MAXIMUM (reserved) */
>>> 0x81, 0x00, /* INPUT (Data,Ary,Abs) */
>>> + 0x85, 0x0e, /* REPORT_ID (14) */
>>> + 0x05, 0x08, /* USAGE PAGE (LED page) */
>>> + 0x95, 0x05, /* REPORT COUNT (5) */
>>> + 0x75, 0x01, /* REPORT SIZE (1) */
>>> + 0x15, 0x00, /* LOGICAL_MINIMUM (0) */
>>> + 0x25, 0x01, /* LOGICAL_MAXIMUM (1) */
>>> + 0x19, 0x01, /* USAGE MINIMUM (1) */
>>> + 0x29, 0x05, /* USAGE MAXIMUM (5) */
>>> + 0x91, 0x02, /* OUTPUT (Data, Variable, Absolute) */
>>> + 0x95, 0x01, /* REPORT COUNT (1) */
>>> + 0x75, 0x03, /* REPORT SIZE (3) */
>>> + 0x91, 0x01, /* OUTPUT (Constant) */
>>> 0xC0
>>> };
>>>
>>> @@ -544,10 +548,30 @@ static int logi_dj_output_hidraw_report(struct hid_device *hid, u8 * buf,
>>> size_t count,
>>> unsigned char report_type)
>>> {
>>> - /* Called by hid raw to send data */
>>> - dbg_hid("%s\n", __func__);
>>> + struct dj_device *djdev = hid->driver_data;
>>> + struct dj_receiver_dev *djrcv_dev = djdev->dj_receiver_dev;
>>> + u8 *out_buf;
>>> + int ret;
>>>
>>> - return 0;
>>> + if (buf[0] != REPORT_TYPE_LEDS)
>>> + return -EINVAL;
>>> +
>>> + out_buf = kzalloc(DJREPORT_SHORT_LENGTH, GFP_ATOMIC);
>>> + if (!out_buf)
>>> + return -ENOMEM;
>>> +
>>> + if (count < DJREPORT_SHORT_LENGTH - 2)
>>> + count = DJREPORT_SHORT_LENGTH - 2;
>>> +
>>> + out_buf[0] = REPORT_ID_DJ_SHORT;
>>> + out_buf[1] = djdev->device_index;
>>> + memcpy(out_buf + 2, buf, count);
>>> +
>>> + ret = djrcv_dev->hdev->hid_output_raw_report(djrcv_dev->hdev, out_buf,
>>> + DJREPORT_SHORT_LENGTH, report_type);
>>
>> Strictly speaking the code below uses HID_REQ_SET_REPORT and you
>> replace it with hid_output_raw_report() here (which at least for BTHID
>> is not equivalent). Anyhow, HID-core uses hid_output_raw_report(),
>> too, so this should be fine.
>
> Yes, you are right. This fires back on me yesterday when I tried to
> remove the hid_output_raw_report() calls.
> It occurs it works because usbhid_out_raw_report() uses set_report
> when there is no urbout, which is the case with logitech receivers.
> So I guess an ideal solution would be to implement a hid_hw_request call.
>
> However, the only concern I have with hid_hw_request is that it does
> not allow hidraw to play with the LEDs given that hidraw does not have
> an ioctl for SET_REPORT (or GET_REPORT). I already seen this issue
> regarding i2c_hid at some point, but it was "solved" in i2c_hid by
> using the same ugly trick that we have in USB-hid.
Hah! That's why it worked all the years, USB-HID checks for !urbout..
and yeah, that's what I just criticized in your i2c-hid patch.. Ok,
I'm fine if we make output_report() fall back to SET_REPORT if not
output-channel is supported. But we it gets ugly if we rely on that
from the upper stack... hmm, don't know how to make that work but
adding a hid quirk to use SET_REPORT for hidinput_input_report().
Thanks
David
^ permalink raw reply
* Re: [PATCH] HID: Add HID transport driver documentation
From: David Herrmann @ 2014-02-03 17:02 UTC (permalink / raw)
To: Benjamin Tissoires; +Cc: Frank Praznik, open list:HID CORE LAYER, Jiri Kosina
In-Reply-To: <CAN+gG=F0fBJ=Ln+4Nc=NTPehwrUhBVYqL4hnYSQhE_4rzP_2oA@mail.gmail.com>
Hi
On Mon, Feb 3, 2014 at 5:52 PM, Benjamin Tissoires
<benjamin.tissoires@gmail.com> wrote:
[snip]
>>>>> + - void (*request) (struct hid_device *hdev, struct hid_report *report,
>>>>> + int reqtype)
>>>>> + Send an HID request on the ctrl channel. "report" contains the report that
>>>>> + should be sent and "reqtype" the request type. Request-type can be
>>>>> + HID_REQ_SET_REPORT or HID_REQ_GET_REPORT.
>>>>> + This callback is optional. If not provided, HID core will assemble a raw
>>>>> + report following the HID specs and send it via the ->raw_request() callback.
>>>
>>> This is not true currently. Aren't we missing a commit in the original series?
>>> But I would love seeing this come true.
>>
>> You're talking about the ->request() to ->raw_request() conversion?
>> Indeed, that hasn't been implemented. But it should be rather easy to
>> do, right? I will prepare a patch for that so we can apply this
>> documentation unchanged.
>
> Don't bother doing it, I have already made it yesterday :)
>
> The documentation would still need to be cleaned to remove
> hid_input_event callback... So I can take it in the next round of
> ll_transport cleanup if you are ok too.
Perfect, all fine with me!
Thanks
David
^ permalink raw reply
* Re: [PATCH] HID: Add HID transport driver documentation
From: Benjamin Tissoires @ 2014-02-03 16:52 UTC (permalink / raw)
To: David Herrmann; +Cc: Frank Praznik, open list:HID CORE LAYER, Jiri Kosina
In-Reply-To: <CANq1E4QFkBGvqpHwEW135c9Ay9Kv1rbA0etsR=mS_dfEqpQeZQ@mail.gmail.com>
On Mon, Feb 3, 2014 at 11:50 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Sat, Feb 1, 2014 at 12:18 AM, Benjamin Tissoires
> <benjamin.tissoires@gmail.com> wrote:
>> On Wed, Jan 29, 2014 at 10:35 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
>>> Hi
>>>
>>> On Wed, Jan 29, 2014 at 4:28 PM, Frank Praznik <frank.praznik@oh.rr.com> wrote:
>>>> Add David Herrmann's documentation for the new low-level HID transport driver
>>>> functions.
>>>>
>>>> Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
>>>
>>> If you copy code, you really should keep the signed-off-by chain. A
>>> signed-off-by in kernel context means that you either wrote the code
>>> or have permission to copy it. See here:
>>> http://developercertificate.org/ (which is a public copy of the
>>> kernel's signed-off-by practice).
>>> If you copy code unchanged, it's common practice to even keep the
>>> "Author" field via "git commit --author", but that's optional.
>>>
>>> Anyhow, patch is good, thanks for picking it up!
>>>
>>> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
>>>
>>> Putting Benjamin on CC as he reviewed the patch last time and might
>>> have some more comments (or his final reviewed-by).
>>>
>>
>> Thanks David. I am globally happy with it, but I have a little remark:
>>
>>> Thanks!
>>> David
>>>
>>>> ---
>>>>
>>>> Sorry, I forgot to include this in the original patch set.
>>>>
>>>> Documentation/hid/hid-transport.txt | 324 ++++++++++++++++++++++++++++++++++++
>>>> 1 file changed, 324 insertions(+)
>>>> create mode 100644 Documentation/hid/hid-transport.txt
>>>>
>>>> diff --git a/Documentation/hid/hid-transport.txt b/Documentation/hid/hid-transport.txt
>>>> new file mode 100644
>>>> index 0000000..14b1c18
>>>> --- /dev/null
>>>> +++ b/Documentation/hid/hid-transport.txt
>>>> @@ -0,0 +1,324 @@
>>>> + HID I/O Transport Drivers
>>>> + ===========================
>>>> +
>>>> +The HID subsystem is independent of the underlying transport driver. Initially,
>>>> +only USB was supported, but other specifications adopted the HID design and
>>>> +provided new transport drivers. The kernel includes at least support for USB,
>>>> +Bluetooth, I2C and user-space I/O drivers.
>>>> +
>>>> +1) HID Bus
>>>> +==========
>>>> +
>>>> +The HID subsystem is designed as a bus. Any I/O subsystem may provide HID
>>>> +devices and register them with the HID bus. HID core then loads generic device
>>>> +drivers on top of it. The transport drivers are responsible of raw data
>>>> +transport and device setup/management. HID core is responsible of
>>>> +report-parsing, report interpretation and the user-space API. Device specifics
>>>> +and quirks are handled by all layers depending on the quirk.
>>>> +
>>>> + +-----------+ +-----------+ +-----------+ +-----------+
>>>> + | Device #1 | | Device #i | | Device #j | | Device #k |
>>>> + +-----------+ +-----------+ +-----------+ +-----------+
>>>> + \\ // \\ //
>>>> + +------------+ +------------+
>>>> + | I/O Driver | | I/O Driver |
>>>> + +------------+ +------------+
>>>> + || ||
>>>> + +------------------+ +------------------+
>>>> + | Transport Driver | | Transport Driver |
>>>> + +------------------+ +------------------+
>>>> + \___ ___/
>>>> + \ /
>>>> + +----------------+
>>>> + | HID Core |
>>>> + +----------------+
>>>> + / | | \
>>>> + / | | \
>>>> + ____________/ | | \_________________
>>>> + / | | \
>>>> + / | | \
>>>> + +----------------+ +-----------+ +------------------+ +------------------+
>>>> + | Generic Driver | | MT Driver | | Custom Driver #1 | | Custom Driver #2 |
>>>> + +----------------+ +-----------+ +------------------+ +------------------+
>>>> +
>>>> +Example Drivers:
>>>> + I/O: USB, I2C, Bluetooth-l2cap
>>>> + Transport: USB-HID, I2C-HID, BT-HIDP
>>>> +
>>>> +Everything below "HID Core" is simplified in this graph as it is only of
>>>> +interest to HID device drivers. Transport drivers do not need to know the
>>>> +specifics.
>>>> +
>>>> +1.1) Device Setup
>>>> +-----------------
>>>> +
>>>> +I/O drivers normally provide hotplug detection or device enumeration APIs to the
>>>> +transport drivers. Transport drivers use this to find any suitable HID device.
>>>> +They allocate HID device objects and register them with HID core. Transport
>>>> +drivers are not required to register themselves with HID core. HID core is never
>>>> +aware of which transport drivers are available and is not interested in it. It
>>>> +is only interested in devices.
>>>> +
>>>> +Transport drivers attach a constant "struct hid_ll_driver" object with each
>>>> +device. Once a device is registered with HID core, the callbacks provided via
>>>> +this struct are used by HID core to communicate with the device.
>>>> +
>>>> +Transport drivers are responsible of detecting device failures and unplugging.
>>>> +HID core will operate a device as long as it is registered regardless of any
>>>> +device failures. Once transport drivers detect unplug or failure events, they
>>>> +must unregister the device from HID core and HID core will stop using the
>>>> +provided callbacks.
>>>> +
>>>> +1.2) Transport Driver Requirements
>>>> +----------------------------------
>>>> +
>>>> +The terms "asynchronous" and "synchronous" in this document describe the
>>>> +transmission behavior regarding acknowledgements. An asynchronous channel must
>>>> +not perform any synchronous operations like waiting for acknowledgements or
>>>> +verifications. Generally, HID calls operating on asynchronous channels must be
>>>> +running in atomic-context just fine.
>>>> +On the other hand, synchronous channels can be implemented by the transport
>>>> +driver in whatever way they like. They might just be the same as asynchronous
>>>> +channels, but they can also provide acknowledgement reports, automatic
>>>> +retransmission on failure, etc. in a blocking manner. If such functionality is
>>>> +required on asynchronous channels, a transport-driver must implement that via
>>>> +its own worker threads.
>>>> +
>>>> +HID core requires transport drivers to follow a given design. A Transport
>>>> +driver must provide two bi-directional I/O channels to each HID device. These
>>>> +channels must not necessarily be bi-directional in the hardware itself. A
>>>> +transport driver might just provide 4 uni-directional channels. Or it might
>>>> +multiplex all four on a single physical channel. However, in this document we
>>>> +will describe them as two bi-directional channels as they have several
>>>> +properties in common.
>>>> +
>>>> + - Interrupt Channel (intr): The intr channel is used for asynchronous data
>>>> + reports. No management commands or data acknowledgements are sent on this
>>>> + channel. Any unrequested incoming or outgoing data report must be sent on
>>>> + this channel and is never acknowledged by the remote side. Devices usually
>>>> + send their input events on this channel. Outgoing events are normally
>>>> + not send via intr, except if high throughput is required.
>>>> + - Control Channel (ctrl): The ctrl channel is used for synchronous requests and
>>>> + device management. Unrequested data input events must not be sent on this
>>>> + channel and are normally ignored. Instead, devices only send management
>>>> + events or answers to host requests on this channel.
>>>> + The control-channel is used for direct blocking queries to the device
>>>> + independent of any events on the intr-channel.
>>>> + Outgoing reports are usually sent on the ctrl channel via synchronous
>>>> + SET_REPORT requests.
>>>> +
>>>> +Communication between devices and HID core is mostly done via HID reports. A
>>>> +report can be of one of three types:
>>>> +
>>>> + - INPUT Report: Input reports provide data from device to host. This
>>>> + data may include button events, axis events, battery status or more. This
>>>> + data is generated by the device and sent to the host with or without
>>>> + requiring explicit requests. Devices can choose to send data continuously or
>>>> + only on change.
>>>> + - OUTPUT Report: Output reports change device states. They are sent from host
>>>> + to device and may include LED requests, rumble requests or more. Output
>>>> + reports are never sent from device to host, but a host can retrieve their
>>>> + current state.
>>>> + Hosts may choose to send output reports either continuously or only on
>>>> + change.
>>>> + - FEATURE Report: Feature reports are used for specific static device features
>>>> + and never reported spontaneously. A host can read and/or write them to access
>>>> + data like battery-state or device-settings.
>>>> + Feature reports are never sent without requests. A host must explicitly set
>>>> + or retrieve a feature report. This also means, feature reports are never sent
>>>> + on the intr channel as this channel is asynchronous.
>>>> +
>>>> +INPUT and OUTPUT reports can be sent as pure data reports on the intr channel.
>>>> +For INPUT reports this is the usual operational mode. But for OUTPUT reports,
>>>> +this is rarely done as OUTPUT reports are normally quite scarce. But devices are
>>>> +free to make excessive use of asynchronous OUTPUT reports (for instance, custom
>>>> +HID audio speakers make great use of it).
>>>> +
>>>> +Plain reports must not be sent on the ctrl channel, though. Instead, the ctrl
>>>> +channel provides synchronous GET/SET_REPORT requests. Plain reports are only
>>>> +allowed on the intr channel and are the only means of data there.
>>>> +
>>>> + - GET_REPORT: A GET_REPORT request has a report ID as payload and is sent
>>>> + from host to device. The device must answer with a data report for the
>>>> + requested report ID on the ctrl channel as a synchronous acknowledgement.
>>>> + Only one GET_REPORT request can be pending for each device. This restriction
>>>> + is enforced by HID core as several transport drivers don't allow multiple
>>>> + simultaneous GET_REPORT requests.
>>>> + Note that data reports which are sent as answer to a GET_REPORT request are
>>>> + not handled as generic device events. That is, if a device does not operate
>>>> + in continuous data reporting mode, an answer to GET_REPORT does not replace
>>>> + the raw data report on the intr channel on state change.
>>>> + GET_REPORT is only used by custom HID device drivers to query device state.
>>>> + Normally, HID core caches any device state so this request is not necessary
>>>> + on devices that follow the HID specs except during device initialization to
>>>> + retrieve the current state.
>>>> + GET_REPORT requests can be sent for any of the 3 report types and shall
>>>> + return the current report state of the device. However, OUTPUT reports as
>>>> + payload may be blocked by the underlying transport driver if the
>>>> + specification does not allow them.
>>>> + - SET_REPORT: A SET_REPORT request has a report ID plus data as payload. It is
>>>> + sent from host to device and a device must update it's current report state
>>>> + according to the given data. Any of the 3 report types can be used. However,
>>>> + INPUT reports as payload might be blocked by the underlying transport driver
>>>> + if the specification does not allow them.
>>>> + A device must answer with a synchronous acknowledgement. However, HID core
>>>> + does not require transport drivers to forward this acknowledgement to HID
>>>> + core.
>>>> + Same as for GET_REPORT, only one SET_REPORT can be pending at a time. This
>>>> + restriction is enforced by HID core as some transport drivers do not support
>>>> + multiple synchronous SET_REPORT requests.
>>>> +
>>>> +Other ctrl-channel requests are supported by USB-HID but are not available
>>>> +(or deprecated) in most other transport level specifications:
>>>> +
>>>> + - GET/SET_IDLE: Only used by USB-HID and I2C-HID.
>>>> + - GET/SET_PROTOCOL: Not used by HID core.
>>>> + - RESET: Used by I2C-HID, not hooked up in HID core.
>>>> + - SET_POWER: Used by I2C-HID, not hooked up in HID core.
>>>> +
>>>> +2) HID API
>>>> +==========
>>>> +
>>>> +2.1) Initialization
>>>> +-------------------
>>>> +
>>>> +Transport drivers normally use the following procedure to register a new device
>>>> +with HID core:
>>>> +
>>>> + struct hid_device *hid;
>>>> + int ret;
>>>> +
>>>> + hid = hid_allocate_device();
>>>> + if (IS_ERR(hid)) {
>>>> + ret = PTR_ERR(hid);
>>>> + goto err_<...>;
>>>> + }
>>>> +
>>>> + strlcpy(hid->name, <device-name-src>, 127);
>>>> + strlcpy(hid->phys, <device-phys-src>, 63);
>>>> + strlcpy(hid->uniq, <device-uniq-src>, 63);
>>>> +
>>>> + hid->ll_driver = &custom_ll_driver;
>>>> + hid->bus = <device-bus>;
>>>> + hid->vendor = <device-vendor>;
>>>> + hid->product = <device-product>;
>>>> + hid->version = <device-version>;
>>>> + hid->country = <device-country>;
>>>> + hid->dev.parent = <pointer-to-parent-device>;
>>>> + hid->driver_data = <transport-driver-data-field>;
>>>> +
>>>> + ret = hid_add_device(hid);
>>>> + if (ret)
>>>> + goto err_<...>;
>>>> +
>>>> +Once hid_add_device() is entered, HID core might use the callbacks provided in
>>>> +"custom_ll_driver". Note that fields like "country" can be ignored by underlying
>>>> +transport-drivers if not supported.
>>>> +
>>>> +To unregister a device, use:
>>>> +
>>>> + hid_destroy_device(hid);
>>>> +
>>>> +Once hid_destroy_device() returns, HID core will no longer make use of any
>>>> +driver callbacks.
>>>> +
>>>> +2.2) hid_ll_driver operations
>>>> +-----------------------------
>>>> +
>>>> +The available HID callbacks are:
>>>> + - int (*start) (struct hid_device *hdev)
>>>> + Called from HID device drivers once they want to use the device. Transport
>>>> + drivers can choose to setup their device in this callback. However, normally
>>>> + devices are already set up before transport drivers register them to HID core
>>>> + so this is mostly only used by USB-HID.
>>>> +
>>>> + - void (*stop) (struct hid_device *hdev)
>>>> + Called from HID device drivers once they are done with a device. Transport
>>>> + drivers can free any buffers and deinitialize the device. But note that
>>>> + ->start() might be called again if another HID device driver is loaded on the
>>>> + device.
>>>> + Transport drivers are free to ignore it and deinitialize devices after they
>>>> + destroyed them via hid_destroy_device().
>>>> +
>>>> + - int (*open) (struct hid_device *hdev)
>>>> + Called from HID device drivers once they are interested in data reports.
>>>> + Usually, while user-space didn't open any input API/etc., device drivers are
>>>> + not interested in device data and transport drivers can put devices asleep.
>>>> + However, once ->open() is called, transport drivers must be ready for I/O.
>>>> + ->open() calls are nested for each client that opens the HID device.
>>>> +
>>>> + - void (*close) (struct hid_device *hdev)
>>>> + Called from HID device drivers after ->open() was called but they are no
>>>> + longer interested in device reports. (Usually if user-space closed any input
>>>> + devices of the driver).
>>>> + Transport drivers can put devices asleep and terminate any I/O of all
>>>> + ->open() calls have been followed by a ->close() call. However, ->start() may
>>>> + be called again if the device driver is interested in input reports again.
>>>> +
>>>> + - int (*parse) (struct hid_device *hdev)
>>>> + Called once during device setup after ->start() has been called. Transport
>>>> + drivers must read the HID report-descriptor from the device and tell HID core
>>>> + about it via hid_parse_report().
>>>> +
>>>> + - int (*power) (struct hid_device *hdev, int level)
>>>> + Called by HID core to give PM hints to transport drivers. Usually this is
>>>> + analogical to the ->open() and ->close() hints and redundant.
>>>> +
>>>> + - void (*request) (struct hid_device *hdev, struct hid_report *report,
>>>> + int reqtype)
>>>> + Send an HID request on the ctrl channel. "report" contains the report that
>>>> + should be sent and "reqtype" the request type. Request-type can be
>>>> + HID_REQ_SET_REPORT or HID_REQ_GET_REPORT.
>>>> + This callback is optional. If not provided, HID core will assemble a raw
>>>> + report following the HID specs and send it via the ->raw_request() callback.
>>
>> This is not true currently. Aren't we missing a commit in the original series?
>> But I would love seeing this come true.
>
> You're talking about the ->request() to ->raw_request() conversion?
> Indeed, that hasn't been implemented. But it should be rather easy to
> do, right? I will prepare a patch for that so we can apply this
> documentation unchanged.
Don't bother doing it, I have already made it yesterday :)
The documentation would still need to be cleaned to remove
hid_input_event callback... So I can take it in the next round of
ll_transport cleanup if you are ok too.
CHeers,
Benjamin
^ 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