* Re: [PATCH 2/3 RESEND] mfd: tc3589x: Reform device tree probing
From: Linus Walleij @ 2014-02-03 10:32 UTC (permalink / raw)
To: Lee Jones
Cc: devicetree@vger.kernel.org, Dmitry Torokhov, Linux Input,
Samuel Ortiz, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Mark Rutland
In-Reply-To: <20140123151123.GE8586@lee--X1>
On Thu, Jan 23, 2014 at 4:11 PM, Lee Jones <lee.jones@linaro.org> wrote:
>> > Patch looks good to me. Is there any reason why we should rush this in
>> > for v3.14, or is it okay to go to -next?
>>
>> No rush, but it's been on review like forever so unless there is
>> some noise from the DT people at -rc1 I'd be very happy if you
>> could apply patches 1 & 2 by then.
>
> I'm just waiting for their Ack. If I don't have it soon I'll review it
> myself and any changes will have to come in via subsequent patch
> submissions.
>
> I think it's sensible to head for v3.15 for this set.
So now that v3.14-rc1 is out can we queue this stuff?
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH 2/3 RESEND] mfd: tc3589x: Reform device tree probing
From: Lee Jones @ 2014-02-03 11:01 UTC (permalink / raw)
To: Linus Walleij
Cc: devicetree, Dmitry Torokhov, linux-input, Samuel Ortiz,
linux-kernel, linux-arm-kernel, Mark Rutland
In-Reply-To: <1390481008-23900-1-git-send-email-linus.walleij@linaro.org>
> This changes the following mechanisms in the TC3589x device tree
> probing path:
>
> - Use the .of_match_table in struct device_driver to match the
> device in the device tree.
> - Add matches for the proper compatible strings "toshiba,..."
> and all sub-variants, just as is done for the .id matches.
> - Move over all the allocation of platform data etc to the
> tc3589x_of_probe() function and follow the pattern of passing
> a platform data pointer back, or an error pointer on error,
> as found in the STMPE driver.
> - Match the new (proper) compatible strings for the GPIO and
> keypad MFD cells.
> - Use of_device_is_compatible() rather than just !strcmp()
> to discover which cells to instantiate.
>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> drivers/mfd/tc3589x.c | 84 ++++++++++++++++++++++++++++++++++++---------------
> 1 file changed, 59 insertions(+), 25 deletions(-)
Looks good, applied.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 2/3 RESEND] mfd: tc3589x: Reform device tree probing
From: Lee Jones @ 2014-02-03 11:02 UTC (permalink / raw)
To: Linus Walleij
Cc: devicetree@vger.kernel.org, Dmitry Torokhov, Linux Input,
Samuel Ortiz, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Mark Rutland
In-Reply-To: <CACRpkdaX7AO3C-N+OMCqtQvFUmHmVRfMhPqGY2LciZQHT7iq+g@mail.gmail.com>
> >> > Patch looks good to me. Is there any reason why we should rush this in
> >> > for v3.14, or is it okay to go to -next?
> >>
> >> No rush, but it's been on review like forever so unless there is
> >> some noise from the DT people at -rc1 I'd be very happy if you
> >> could apply patches 1 & 2 by then.
> >
> > I'm just waiting for their Ack. If I don't have it soon I'll review it
> > myself and any changes will have to come in via subsequent patch
> > submissions.
> >
> > I think it's sensible to head for v3.15 for this set.
>
> So now that v3.14-rc1 is out can we queue this stuff?
Queued.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
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] Bluetooth: hidp: make sure input buffers are big enough
From: David Herrmann @ 2014-02-03 11:27 UTC (permalink / raw)
To: Jiri Kosina
Cc: Marcel Holtmann, open list:HID CORE LAYER,
linux-bluetooth@vger.kernel.org development, Gustavo F. Padovan
In-Reply-To: <alpine.LNX.2.00.1402031107310.14066@pobox.suse.cz>
Hi Jiri
On Mon, Feb 3, 2014 at 11:08 AM, Jiri Kosina <jkosina@suse.cz> wrote:
> On Wed, 29 Jan 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 am back
home now and will try to make HID core honor the length, but that's
quite invasive and shouldn't be used for stable.
Sorry for the delay,
David
^ permalink raw reply
* Re: [PATCH 07/11] HID: HIDp: remove duplicated coded
From: David Herrmann @ 2014-02-03 15:02 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Benjamin Tissoires, Jiri Kosina, Frank Praznik,
open list:HID CORE LAYER, linux-kernel
In-Reply-To: <1391316630-29541-8-git-send-email-benjamin.tissoires@redhat.com>
Hi
On Sun, Feb 2, 2014 at 5:50 AM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> - Move hidp_output_report() above
> - Removed duplicated code in hidp_output_raw_report()
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
> net/bluetooth/hidp/core.c | 68 ++++++++---------------------------------------
> 1 file changed, 11 insertions(+), 57 deletions(-)
>
> diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
> index 469e61b..02670b3 100644
> --- a/net/bluetooth/hidp/core.c
> +++ b/net/bluetooth/hidp/core.c
> @@ -373,62 +373,25 @@ err:
> return ret;
> }
>
> -static int hidp_output_raw_report(struct hid_device *hid, unsigned char *data, size_t count,
> - unsigned char report_type)
> +static int hidp_output_report(struct hid_device *hid, __u8 *data, size_t count)
> {
> struct hidp_session *session = hid->driver_data;
> - int ret;
>
> + return hidp_send_intr_message(session,
> + HIDP_TRANS_DATA | HIDP_DATA_RTYPE_OUPUT,
> + data, count);
> +}
> +
> +static int hidp_output_raw_report(struct hid_device *hid, unsigned char *data,
> + size_t count, unsigned char report_type)
> +{
> if (report_type == HID_OUTPUT_REPORT) {
> - report_type = HIDP_TRANS_DATA | HIDP_DATA_RTYPE_OUPUT;
> - return hidp_send_intr_message(session, report_type,
> - data, count);
> + return hidp_output_report(hid, data, count);
NAK, that does not work with BT. hidp_output_raw_report() can do both,
a synchronous SET_REPORT and an async OUTPUT_REPORT. It's currently
screwed up, as for RTYPE_OUTPUT it uses the intr-channel, for
RTYPE_FEATURE the ctrl-channel.
This function is exactly the reason why we added raw_report() and
output_report(). So I'd like to keep it as it and just remove it once
we have no more users of hidp_output_raw_report().
Thanks
David
> } else if (report_type != HID_FEATURE_REPORT) {
> return -EINVAL;
> }
>
> - if (mutex_lock_interruptible(&session->report_mutex))
> - return -ERESTARTSYS;
> -
> - /* Set up our wait, and send the report request to the device. */
> - set_bit(HIDP_WAITING_FOR_SEND_ACK, &session->flags);
> - report_type = HIDP_TRANS_SET_REPORT | HIDP_DATA_RTYPE_FEATURE;
> - ret = hidp_send_ctrl_message(session, report_type, data, count);
> - if (ret)
> - goto err;
> -
> - /* Wait for the ACK from the device. */
> - while (test_bit(HIDP_WAITING_FOR_SEND_ACK, &session->flags) &&
> - !atomic_read(&session->terminate)) {
> - int res;
> -
> - res = wait_event_interruptible_timeout(session->report_queue,
> - !test_bit(HIDP_WAITING_FOR_SEND_ACK, &session->flags)
> - || atomic_read(&session->terminate),
> - 10*HZ);
> - if (res == 0) {
> - /* timeout */
> - ret = -EIO;
> - goto err;
> - }
> - if (res < 0) {
> - /* signal */
> - ret = -ERESTARTSYS;
> - goto err;
> - }
> - }
> -
> - if (!session->output_report_success) {
> - ret = -EIO;
> - goto err;
> - }
> -
> - ret = count;
> -
> -err:
> - clear_bit(HIDP_WAITING_FOR_SEND_ACK, &session->flags);
> - mutex_unlock(&session->report_mutex);
> - return ret;
> + return hidp_set_raw_report(hid, data[0], data, count, report_type);
> }
>
> static int hidp_raw_request(struct hid_device *hid, unsigned char reportnum,
> @@ -445,15 +408,6 @@ static int hidp_raw_request(struct hid_device *hid, unsigned char reportnum,
> }
> }
>
> -static int hidp_output_report(struct hid_device *hid, __u8 *data, size_t count)
> -{
> - struct hidp_session *session = hid->driver_data;
> -
> - return hidp_send_intr_message(session,
> - HIDP_TRANS_DATA | HIDP_DATA_RTYPE_OUPUT,
> - data, count);
> -}
> -
> static void hidp_idle_timeout(unsigned long arg)
> {
> struct hidp_session *session = (struct hidp_session *) arg;
> --
> 1.8.3.1
>
^ permalink raw reply
* Re: [PATCH 05/11] HID: HIDp: remove hidp_hidinput_event
From: David Herrmann @ 2014-02-03 15:10 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Benjamin Tissoires, Jiri Kosina, Frank Praznik,
open list:HID CORE LAYER, linux-kernel
In-Reply-To: <1391316630-29541-6-git-send-email-benjamin.tissoires@redhat.com>
Hi
On Sun, Feb 2, 2014 at 5:50 AM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> hidp uses its own ->hidinput_input_event() instead of the generic binding
> in hid-input.
> Moving the handling of LEDs towards hidp_hidinput_event() allows two things:
> - remove hidinput_input_event definitively from struct hid_device
> - hidraw user space programs can also set the LEDs
This one looks good. hid-input uses output_raw_report(), which is on
INTR for BT-HID, which is equivalent to what hidp_send_report() does.
So:
Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
Btw., you might have to keep hidp_send_report() if you drop earlier
patches (haven't exactly looked at the order), but feel free to keep
my r-b anyway.
Thanks
David
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
> net/bluetooth/hidp/core.c | 46 ----------------------------------------------
> 1 file changed, 46 deletions(-)
>
> diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
> index b062cee..469e61b 100644
> --- a/net/bluetooth/hidp/core.c
> +++ b/net/bluetooth/hidp/core.c
> @@ -223,51 +223,6 @@ static void hidp_input_report(struct hidp_session *session, struct sk_buff *skb)
> input_sync(dev);
> }
>
> -static int hidp_send_report(struct hidp_session *session, struct hid_report *report)
> -{
> - unsigned char hdr;
> - u8 *buf;
> - int rsize, ret;
> -
> - buf = hid_alloc_report_buf(report, GFP_ATOMIC);
> - if (!buf)
> - return -EIO;
> -
> - hid_output_report(report, buf);
> - hdr = HIDP_TRANS_DATA | HIDP_DATA_RTYPE_OUPUT;
> -
> - rsize = ((report->size - 1) >> 3) + 1 + (report->id > 0);
> - ret = hidp_send_intr_message(session, hdr, buf, rsize);
> -
> - kfree(buf);
> - return ret;
> -}
> -
> -static int hidp_hidinput_event(struct input_dev *dev, unsigned int type,
> - unsigned int code, int value)
> -{
> - struct hid_device *hid = input_get_drvdata(dev);
> - struct hidp_session *session = hid->driver_data;
> - struct hid_field *field;
> - int offset;
> -
> - BT_DBG("session %p type %d code %d value %d",
> - session, type, code, value);
> -
> - if (type != EV_LED)
> - return -1;
> -
> - offset = hidinput_find_field(hid, type, code, &field);
> - if (offset == -1) {
> - hid_warn(dev, "event field not found\n");
> - return -1;
> - }
> -
> - hid_set_field(field, offset, value);
> -
> - return hidp_send_report(session, field->report);
> -}
> -
> static int hidp_get_raw_report(struct hid_device *hid,
> unsigned char report_number,
> unsigned char *data, size_t count,
> @@ -817,7 +772,6 @@ static struct hid_ll_driver hidp_hid_driver = {
> .close = hidp_close,
> .raw_request = hidp_raw_request,
> .output_report = hidp_output_report,
> - .hidinput_input_event = hidp_hidinput_event,
> };
>
> /* This function sets up the hid device. It does not add it
> --
> 1.8.3.1
>
^ permalink raw reply
* Re: [PATCH 07/11] HID: HIDp: remove duplicated coded
From: Benjamin Tissoires @ 2014-02-03 15:11 UTC (permalink / raw)
To: David Herrmann
Cc: Benjamin Tissoires, Jiri Kosina, Frank Praznik,
open list:HID CORE LAYER, linux-kernel
In-Reply-To: <CANq1E4S6jCc4Kd2EKoG0SgSc68TrBBSfnKChGV6Tn3fGKGRnSQ@mail.gmail.com>
On Mon, Feb 3, 2014 at 10:02 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:
>> - Move hidp_output_report() above
>> - Removed duplicated code in hidp_output_raw_report()
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>> ---
>> net/bluetooth/hidp/core.c | 68 ++++++++---------------------------------------
>> 1 file changed, 11 insertions(+), 57 deletions(-)
>>
>> diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
>> index 469e61b..02670b3 100644
>> --- a/net/bluetooth/hidp/core.c
>> +++ b/net/bluetooth/hidp/core.c
>> @@ -373,62 +373,25 @@ err:
>> return ret;
>> }
>>
>> -static int hidp_output_raw_report(struct hid_device *hid, unsigned char *data, size_t count,
>> - unsigned char report_type)
>> +static int hidp_output_report(struct hid_device *hid, __u8 *data, size_t count)
>> {
>> struct hidp_session *session = hid->driver_data;
>> - int ret;
>>
>> + return hidp_send_intr_message(session,
>> + HIDP_TRANS_DATA | HIDP_DATA_RTYPE_OUPUT,
>> + data, count);
>> +}
>> +
>> +static int hidp_output_raw_report(struct hid_device *hid, unsigned char *data,
>> + size_t count, unsigned char report_type)
>> +{
>> if (report_type == HID_OUTPUT_REPORT) {
>> - report_type = HIDP_TRANS_DATA | HIDP_DATA_RTYPE_OUPUT;
>> - return hidp_send_intr_message(session, report_type,
>> - data, count);
>> + return hidp_output_report(hid, data, count);
>
> NAK, that does not work with BT. hidp_output_raw_report() can do both,
> a synchronous SET_REPORT and an async OUTPUT_REPORT. It's currently
> screwed up, as for RTYPE_OUTPUT it uses the intr-channel, for
> RTYPE_FEATURE the ctrl-channel.
David, please, re-review it carefully, and see that there is _no_
change in the code path with this refactoring:
hidp_output_report() calls exactly one hidp_send_intr_message() with
the same arguments.
And for the rest, you will see that the code path is still the same also.
>
> This function is exactly the reason why we added raw_report() and
> output_report(). So I'd like to keep it as it and just remove it once
> we have no more users of hidp_output_raw_report().
I worked on that yesterday, so I can make this happening soon (not
today though).
Cheers,
Benjamin
>
> Thanks
> David
>
>> } else if (report_type != HID_FEATURE_REPORT) {
>> return -EINVAL;
>> }
>>
>> - if (mutex_lock_interruptible(&session->report_mutex))
>> - return -ERESTARTSYS;
>> -
>> - /* Set up our wait, and send the report request to the device. */
>> - set_bit(HIDP_WAITING_FOR_SEND_ACK, &session->flags);
>> - report_type = HIDP_TRANS_SET_REPORT | HIDP_DATA_RTYPE_FEATURE;
>> - ret = hidp_send_ctrl_message(session, report_type, data, count);
>> - if (ret)
>> - goto err;
>> -
>> - /* Wait for the ACK from the device. */
>> - while (test_bit(HIDP_WAITING_FOR_SEND_ACK, &session->flags) &&
>> - !atomic_read(&session->terminate)) {
>> - int res;
>> -
>> - res = wait_event_interruptible_timeout(session->report_queue,
>> - !test_bit(HIDP_WAITING_FOR_SEND_ACK, &session->flags)
>> - || atomic_read(&session->terminate),
>> - 10*HZ);
>> - if (res == 0) {
>> - /* timeout */
>> - ret = -EIO;
>> - goto err;
>> - }
>> - if (res < 0) {
>> - /* signal */
>> - ret = -ERESTARTSYS;
>> - goto err;
>> - }
>> - }
>> -
>> - if (!session->output_report_success) {
>> - ret = -EIO;
>> - goto err;
>> - }
>> -
>> - ret = count;
>> -
>> -err:
>> - clear_bit(HIDP_WAITING_FOR_SEND_ACK, &session->flags);
>> - mutex_unlock(&session->report_mutex);
>> - return ret;
>> + return hidp_set_raw_report(hid, data[0], data, count, report_type);
>> }
>>
>> static int hidp_raw_request(struct hid_device *hid, unsigned char reportnum,
>> @@ -445,15 +408,6 @@ static int hidp_raw_request(struct hid_device *hid, unsigned char reportnum,
>> }
>> }
>>
>> -static int hidp_output_report(struct hid_device *hid, __u8 *data, size_t count)
>> -{
>> - struct hidp_session *session = hid->driver_data;
>> -
>> - return hidp_send_intr_message(session,
>> - HIDP_TRANS_DATA | HIDP_DATA_RTYPE_OUPUT,
>> - data, count);
>> -}
>> -
>> static void hidp_idle_timeout(unsigned long arg)
>> {
>> struct hidp_session *session = (struct hidp_session *) arg;
>> --
>> 1.8.3.1
>>
^ permalink raw reply
* Re: [PATCH 07/11] HID: HIDp: remove duplicated coded
From: David Herrmann @ 2014-02-03 15:19 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=G3njr09E3Q_6kd1SSp5k-Wcn1+aeHee5-Q6tWwFtuA1w@mail.gmail.com>
Hi
On Mon, Feb 3, 2014 at 4:11 PM, Benjamin Tissoires
<benjamin.tissoires@gmail.com> wrote:
> On Mon, Feb 3, 2014 at 10:02 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:
>>> - Move hidp_output_report() above
>>> - Removed duplicated code in hidp_output_raw_report()
>>>
>>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>>> ---
>>> net/bluetooth/hidp/core.c | 68 ++++++++---------------------------------------
>>> 1 file changed, 11 insertions(+), 57 deletions(-)
>>>
>>> diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
>>> index 469e61b..02670b3 100644
>>> --- a/net/bluetooth/hidp/core.c
>>> +++ b/net/bluetooth/hidp/core.c
>>> @@ -373,62 +373,25 @@ err:
>>> return ret;
>>> }
>>>
>>> -static int hidp_output_raw_report(struct hid_device *hid, unsigned char *data, size_t count,
>>> - unsigned char report_type)
>>> +static int hidp_output_report(struct hid_device *hid, __u8 *data, size_t count)
>>> {
>>> struct hidp_session *session = hid->driver_data;
>>> - int ret;
>>>
>>> + return hidp_send_intr_message(session,
>>> + HIDP_TRANS_DATA | HIDP_DATA_RTYPE_OUPUT,
>>> + data, count);
>>> +}
>>> +
>>> +static int hidp_output_raw_report(struct hid_device *hid, unsigned char *data,
>>> + size_t count, unsigned char report_type)
>>> +{
>>> if (report_type == HID_OUTPUT_REPORT) {
>>> - report_type = HIDP_TRANS_DATA | HIDP_DATA_RTYPE_OUPUT;
>>> - return hidp_send_intr_message(session, report_type,
>>> - data, count);
>>> + return hidp_output_report(hid, data, count);
>>
>> NAK, that does not work with BT. hidp_output_raw_report() can do both,
>> a synchronous SET_REPORT and an async OUTPUT_REPORT. It's currently
>> screwed up, as for RTYPE_OUTPUT it uses the intr-channel, for
>> RTYPE_FEATURE the ctrl-channel.
>
> David, please, re-review it carefully, and see that there is _no_
> change in the code path with this refactoring:
> hidp_output_report() calls exactly one hidp_send_intr_message() with
> the same arguments.
>
> And for the rest, you will see that the code path is still the same also.
Sorry, my bad. I thought you changed hidp_output_raw_report() to call
hidp_output_report() unconditionally. You're indeed right, the
code-paths are the same and they now highlight the very horrible hack
hid_output_raw_report() has been in the past for HIDP.
Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
Thanks
David
>>
>> This function is exactly the reason why we added raw_report() and
>> output_report(). So I'd like to keep it as it and just remove it once
>> we have no more users of hidp_output_raw_report().
>
> I worked on that yesterday, so I can make this happening soon (not
> today though).
>
> Cheers,
> Benjamin
>
>>
>> Thanks
>> David
>>
>>> } else if (report_type != HID_FEATURE_REPORT) {
>>> return -EINVAL;
>>> }
>>>
>>> - if (mutex_lock_interruptible(&session->report_mutex))
>>> - return -ERESTARTSYS;
>>> -
>>> - /* Set up our wait, and send the report request to the device. */
>>> - set_bit(HIDP_WAITING_FOR_SEND_ACK, &session->flags);
>>> - report_type = HIDP_TRANS_SET_REPORT | HIDP_DATA_RTYPE_FEATURE;
>>> - ret = hidp_send_ctrl_message(session, report_type, data, count);
>>> - if (ret)
>>> - goto err;
>>> -
>>> - /* Wait for the ACK from the device. */
>>> - while (test_bit(HIDP_WAITING_FOR_SEND_ACK, &session->flags) &&
>>> - !atomic_read(&session->terminate)) {
>>> - int res;
>>> -
>>> - res = wait_event_interruptible_timeout(session->report_queue,
>>> - !test_bit(HIDP_WAITING_FOR_SEND_ACK, &session->flags)
>>> - || atomic_read(&session->terminate),
>>> - 10*HZ);
>>> - if (res == 0) {
>>> - /* timeout */
>>> - ret = -EIO;
>>> - goto err;
>>> - }
>>> - if (res < 0) {
>>> - /* signal */
>>> - ret = -ERESTARTSYS;
>>> - goto err;
>>> - }
>>> - }
>>> -
>>> - if (!session->output_report_success) {
>>> - ret = -EIO;
>>> - goto err;
>>> - }
>>> -
>>> - ret = count;
>>> -
>>> -err:
>>> - clear_bit(HIDP_WAITING_FOR_SEND_ACK, &session->flags);
>>> - mutex_unlock(&session->report_mutex);
>>> - return ret;
>>> + return hidp_set_raw_report(hid, data[0], data, count, report_type);
>>> }
>>>
>>> static int hidp_raw_request(struct hid_device *hid, unsigned char reportnum,
>>> @@ -445,15 +408,6 @@ static int hidp_raw_request(struct hid_device *hid, unsigned char reportnum,
>>> }
>>> }
>>>
>>> -static int hidp_output_report(struct hid_device *hid, __u8 *data, size_t count)
>>> -{
>>> - struct hidp_session *session = hid->driver_data;
>>> -
>>> - return hidp_send_intr_message(session,
>>> - HIDP_TRANS_DATA | HIDP_DATA_RTYPE_OUPUT,
>>> - data, count);
>>> -}
>>> -
>>> static void hidp_idle_timeout(unsigned long arg)
>>> {
>>> struct hidp_session *session = (struct hidp_session *) arg;
>>> --
>>> 1.8.3.1
>>>
^ permalink raw reply
* Re: [PATCH 01/11] HID: uHID: implement .raw_request
From: David Herrmann @ 2014-02-03 15:26 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Benjamin Tissoires, Jiri Kosina, Frank Praznik,
open list:HID CORE LAYER, linux-kernel
In-Reply-To: <1391316630-29541-2-git-send-email-benjamin.tissoires@redhat.com>
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.
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..
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 05/11] HID: HIDp: remove hidp_hidinput_event
From: Benjamin Tissoires @ 2014-02-03 15:28 UTC (permalink / raw)
To: David Herrmann
Cc: Benjamin Tissoires, Jiri Kosina, Frank Praznik,
open list:HID CORE LAYER, linux-kernel
In-Reply-To: <CANq1E4S+RLDpDd+zxgBRKsEw0-29un9mzS5jYBcoeJgQf5Upjg@mail.gmail.com>
On Mon, Feb 3, 2014 at 10:10 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:
>> hidp uses its own ->hidinput_input_event() instead of the generic binding
>> in hid-input.
>> Moving the handling of LEDs towards hidp_hidinput_event() allows two things:
>> - remove hidinput_input_event definitively from struct hid_device
>> - hidraw user space programs can also set the LEDs
>
> This one looks good. hid-input uses output_raw_report(), which is on
> INTR for BT-HID, which is equivalent to what hidp_send_report() does.
> So:
>
> Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
Thanks
>
> Btw., you might have to keep hidp_send_report() if you drop earlier
> patches (haven't exactly looked at the order), but feel free to keep
> my r-b anyway.
No need to keep it, patches 1 to 4 do not touch HIDp, so no one else
use hidp_send_report().
Cheers,
Benjamin
>
> Thanks
> David
>
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>> ---
>> net/bluetooth/hidp/core.c | 46 ----------------------------------------------
>> 1 file changed, 46 deletions(-)
>>
>> diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
>> index b062cee..469e61b 100644
>> --- a/net/bluetooth/hidp/core.c
>> +++ b/net/bluetooth/hidp/core.c
>> @@ -223,51 +223,6 @@ static void hidp_input_report(struct hidp_session *session, struct sk_buff *skb)
>> input_sync(dev);
>> }
>>
>> -static int hidp_send_report(struct hidp_session *session, struct hid_report *report)
>> -{
>> - unsigned char hdr;
>> - u8 *buf;
>> - int rsize, ret;
>> -
>> - buf = hid_alloc_report_buf(report, GFP_ATOMIC);
>> - if (!buf)
>> - return -EIO;
>> -
>> - hid_output_report(report, buf);
>> - hdr = HIDP_TRANS_DATA | HIDP_DATA_RTYPE_OUPUT;
>> -
>> - rsize = ((report->size - 1) >> 3) + 1 + (report->id > 0);
>> - ret = hidp_send_intr_message(session, hdr, buf, rsize);
>> -
>> - kfree(buf);
>> - return ret;
>> -}
>> -
>> -static int hidp_hidinput_event(struct input_dev *dev, unsigned int type,
>> - unsigned int code, int value)
>> -{
>> - struct hid_device *hid = input_get_drvdata(dev);
>> - struct hidp_session *session = hid->driver_data;
>> - struct hid_field *field;
>> - int offset;
>> -
>> - BT_DBG("session %p type %d code %d value %d",
>> - session, type, code, value);
>> -
>> - if (type != EV_LED)
>> - return -1;
>> -
>> - offset = hidinput_find_field(hid, type, code, &field);
>> - if (offset == -1) {
>> - hid_warn(dev, "event field not found\n");
>> - return -1;
>> - }
>> -
>> - hid_set_field(field, offset, value);
>> -
>> - return hidp_send_report(session, field->report);
>> -}
>> -
>> static int hidp_get_raw_report(struct hid_device *hid,
>> unsigned char report_number,
>> unsigned char *data, size_t count,
>> @@ -817,7 +772,6 @@ static struct hid_ll_driver hidp_hid_driver = {
>> .close = hidp_close,
>> .raw_request = hidp_raw_request,
>> .output_report = hidp_output_report,
>> - .hidinput_input_event = hidp_hidinput_event,
>> };
>>
>> /* This function sets up the hid device. It does not add it
>> --
>> 1.8.3.1
>>
^ permalink raw reply
* Re: [PATCH 03/11] HID: add inliners for ll_driver transport-layer callbacks
From: David Herrmann @ 2014-02-03 15:29 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Benjamin Tissoires, Jiri Kosina, Frank Praznik,
open list:HID CORE LAYER, linux-kernel
In-Reply-To: <1391316630-29541-4-git-send-email-benjamin.tissoires@redhat.com>
Hi
On Sun, Feb 2, 2014 at 5:50 AM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> Those callbacks are not mandatory, so it's better to add inliners
> to use them safely.
Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
Thanks
David
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
> include/linux/hid.h | 45 +++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 45 insertions(+)
>
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index 003cc8e..dddcad0 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -680,6 +680,8 @@ struct hid_driver {
> * shouldn't allocate anything to not leak memory
> * @request: send report request to device (e.g. feature report)
> * @wait: wait for buffered io to complete (send/recv reports)
> + * @raw_request: send raw report request to device (e.g. feature report)
> + * @output_report: send output report to device
> * @idle: send idle request to device
> */
> struct hid_ll_driver {
> @@ -974,6 +976,49 @@ static inline void hid_hw_request(struct hid_device *hdev,
> }
>
> /**
> + * hid_hw_raw_request - send report request to device
> + *
> + * @hdev: hid device
> + * @reportnum: report ID
> + * @buf: in/out data to transfer
> + * @len: length of buf
> + * @rtype: HID report type
> + * @reqtype: HID_REQ_GET_REPORT or HID_REQ_SET_REPORT
> + *
> + * @return: count of data transfered, negative if error
> + *
> + * Same behavior as hid_hw_request, but with raw buffers instead.
> + */
> +static inline int hid_hw_raw_request(struct hid_device *hdev,
> + unsigned char reportnum, __u8 *buf,
> + size_t len, unsigned char rtype, int reqtype)
> +{
> + if (hdev->ll_driver->raw_request)
> + return hdev->ll_driver->raw_request(hdev, reportnum, buf, len,
> + rtype, reqtype);
> +
> + return -ENOSYS;
> +}
> +
> +/**
> + * hid_hw_output_report - send output report to device
> + *
> + * @hdev: hid device
> + * @buf: raw data to transfer
> + * @len: length of buf
> + *
> + * @return: count of data transfered, negative if error
> + */
> +static inline int hid_hw_output_report(struct hid_device *hdev, __u8 *buf,
> + size_t len)
> +{
> + if (hdev->ll_driver->output_report)
> + return hdev->ll_driver->output_report(hdev, buf, len);
> +
> + return -ENOSYS;
> +}
> +
> +/**
> * hid_hw_idle - send idle request to device
> *
> * @hdev: hid device
> --
> 1.8.3.1
>
^ permalink raw reply
* Re: [PATCH 02/11] HID: i2c-hid: implement ll_driver transport-layer callbacks
From: David Herrmann @ 2014-02-03 16:04 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Benjamin Tissoires, Jiri Kosina, Frank Praznik,
open list:HID CORE LAYER, linux-kernel
In-Reply-To: <1391316630-29541-3-git-send-email-benjamin.tissoires@redhat.com>
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.
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 04/11] HID: logitech-dj: remove hidinput_input_event
From: David Herrmann @ 2014-02-03 16:06 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Benjamin Tissoires, Jiri Kosina, Frank Praznik,
open list:HID CORE LAYER, linux-kernel
In-Reply-To: <1391316630-29541-5-git-send-email-benjamin.tissoires@redhat.com>
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.
Thanks
David
> +
> + kfree(out_buf);
> + return ret;
> }
>
> static void rdcat(char *rdesc, unsigned int *rsize, const char *data, unsigned int size)
> @@ -613,58 +637,6 @@ static int logi_dj_ll_parse(struct hid_device *hid)
> return retval;
> }
>
> -static int logi_dj_ll_input_event(struct input_dev *dev, unsigned int type,
> - unsigned int code, int value)
> -{
> - /* Sent by the input layer to handle leds and Force Feedback */
> - struct hid_device *dj_hiddev = input_get_drvdata(dev);
> - struct dj_device *dj_dev = dj_hiddev->driver_data;
> -
> - struct dj_receiver_dev *djrcv_dev =
> - dev_get_drvdata(dj_hiddev->dev.parent);
> - struct hid_device *dj_rcv_hiddev = djrcv_dev->hdev;
> - struct hid_report_enum *output_report_enum;
> -
> - struct hid_field *field;
> - struct hid_report *report;
> - unsigned char *data;
> - int offset;
> -
> - dbg_hid("%s: %s, type:%d | code:%d | value:%d\n",
> - __func__, dev->phys, type, code, value);
> -
> - if (type != EV_LED)
> - return -1;
> -
> - offset = hidinput_find_field(dj_hiddev, type, code, &field);
> -
> - if (offset == -1) {
> - dev_warn(&dev->dev, "event field not found\n");
> - return -1;
> - }
> - hid_set_field(field, offset, value);
> -
> - data = hid_alloc_report_buf(field->report, GFP_ATOMIC);
> - if (!data) {
> - dev_warn(&dev->dev, "failed to allocate report buf memory\n");
> - return -1;
> - }
> -
> - hid_output_report(field->report, &data[0]);
> -
> - output_report_enum = &dj_rcv_hiddev->report_enum[HID_OUTPUT_REPORT];
> - report = output_report_enum->report_id_hash[REPORT_ID_DJ_SHORT];
> - hid_set_field(report->field[0], 0, dj_dev->device_index);
> - hid_set_field(report->field[0], 1, REPORT_TYPE_LEDS);
> - hid_set_field(report->field[0], 2, data[1]);
> -
> - hid_hw_request(dj_rcv_hiddev, report, HID_REQ_SET_REPORT);
> -
> - kfree(data);
> -
> - return 0;
> -}
> -
> static int logi_dj_ll_start(struct hid_device *hid)
> {
> dbg_hid("%s\n", __func__);
> @@ -683,7 +655,6 @@ static struct hid_ll_driver logi_dj_ll_driver = {
> .stop = logi_dj_ll_stop,
> .open = logi_dj_ll_open,
> .close = logi_dj_ll_close,
> - .hidinput_input_event = logi_dj_ll_input_event,
> };
>
>
> --
> 1.8.3.1
>
^ permalink raw reply
* Re: [PATCH 08/11] HID: usbhid: remove duplicated code
From: David Herrmann @ 2014-02-03 16:09 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Benjamin Tissoires, Jiri Kosina, Frank Praznik,
open list:HID CORE LAYER, linux-kernel
In-Reply-To: <1391316630-29541-9-git-send-email-benjamin.tissoires@redhat.com>
Hi
On Sun, Feb 2, 2014 at 5:50 AM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> Well, no use to keep twice the same code.
Yepp, I actually copied the code from that, so this is fine.
Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
Thanks
David
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
> drivers/hid/usbhid/hid-core.c | 64 ++++++++-----------------------------------
> 1 file changed, 11 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index f8ca312..406497b 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -915,59 +915,6 @@ static int usbhid_set_raw_report(struct hid_device *hid, unsigned int reportnum,
> return ret;
> }
>
> -
> -static int usbhid_output_raw_report(struct hid_device *hid, __u8 *buf, size_t count,
> - unsigned char report_type)
> -{
> - struct usbhid_device *usbhid = hid->driver_data;
> - struct usb_device *dev = hid_to_usb_dev(hid);
> - struct usb_interface *intf = usbhid->intf;
> - struct usb_host_interface *interface = intf->cur_altsetting;
> - int ret;
> -
> - if (usbhid->urbout && report_type != HID_FEATURE_REPORT) {
> - int actual_length;
> - int skipped_report_id = 0;
> -
> - if (buf[0] == 0x0) {
> - /* Don't send the Report ID */
> - buf++;
> - count--;
> - skipped_report_id = 1;
> - }
> - ret = usb_interrupt_msg(dev, usbhid->urbout->pipe,
> - buf, count, &actual_length,
> - USB_CTRL_SET_TIMEOUT);
> - /* return the number of bytes transferred */
> - if (ret == 0) {
> - ret = actual_length;
> - /* count also the report id */
> - if (skipped_report_id)
> - ret++;
> - }
> - } else {
> - int skipped_report_id = 0;
> - int report_id = buf[0];
> - if (buf[0] == 0x0) {
> - /* Don't send the Report ID */
> - buf++;
> - count--;
> - skipped_report_id = 1;
> - }
> - ret = usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
> - HID_REQ_SET_REPORT,
> - USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
> - ((report_type + 1) << 8) | report_id,
> - interface->desc.bInterfaceNumber, buf, count,
> - USB_CTRL_SET_TIMEOUT);
> - /* count also the report id, if this was a numbered report. */
> - if (ret > 0 && skipped_report_id)
> - ret++;
> - }
> -
> - return ret;
> -}
> -
> static int usbhid_output_report(struct hid_device *hid, __u8 *buf, size_t count)
> {
> struct usbhid_device *usbhid = hid->driver_data;
> @@ -998,6 +945,17 @@ static int usbhid_output_report(struct hid_device *hid, __u8 *buf, size_t count)
> return ret;
> }
>
> +static int usbhid_output_raw_report(struct hid_device *hid, __u8 *buf,
> + size_t count, unsigned char report_type)
> +{
> + struct usbhid_device *usbhid = hid->driver_data;
> +
> + if (usbhid->urbout && report_type != HID_FEATURE_REPORT)
> + return usbhid_output_report(hid, buf, count);
> +
> + return usbhid_set_raw_report(hid, buf[0], buf, count, report_type);
> +}
> +
> static void usbhid_restart_queues(struct usbhid_device *usbhid)
> {
> if (usbhid->urbout && !test_bit(HID_OUT_RUNNING, &usbhid->iofl))
> --
> 1.8.3.1
>
^ permalink raw reply
* Re: [PATCH 06/11] HID: remove hidinput_input_event handler
From: David Herrmann @ 2014-02-03 16:10 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Benjamin Tissoires, Jiri Kosina, Frank Praznik,
open list:HID CORE LAYER, linux-kernel
In-Reply-To: <1391316630-29541-7-git-send-email-benjamin.tissoires@redhat.com>
Hi
On Sun, Feb 2, 2014 at 5:50 AM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> All the different transport drivers use now the generic event handling
> in hid-input. We can remove the handler definitively now.
\o/
Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
Thanks
David
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
> drivers/hid/hid-input.c | 4 +---
> include/linux/hid.h | 4 ----
> 2 files changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index a713e62..594722d 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -1263,9 +1263,7 @@ static struct hid_input *hidinput_allocate(struct hid_device *hid)
> }
>
> input_set_drvdata(input_dev, hid);
> - if (hid->ll_driver->hidinput_input_event)
> - input_dev->event = hid->ll_driver->hidinput_input_event;
> - else if (hid->ll_driver->request || hid->hid_output_raw_report)
> + if (hid->ll_driver->request || hid->hid_output_raw_report)
> input_dev->event = hidinput_input_event;
> input_dev->open = hidinput_open;
> input_dev->close = hidinput_close;
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index dddcad0..38c307b 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -675,7 +675,6 @@ struct hid_driver {
> * @stop: called on remove
> * @open: called by input layer on open
> * @close: called by input layer on close
> - * @hidinput_input_event: event input event (e.g. ff or leds)
> * @parse: this method is called only once to parse the device data,
> * shouldn't allocate anything to not leak memory
> * @request: send report request to device (e.g. feature report)
> @@ -693,9 +692,6 @@ struct hid_ll_driver {
>
> int (*power)(struct hid_device *hdev, int level);
>
> - int (*hidinput_input_event) (struct input_dev *idev, unsigned int type,
> - unsigned int code, int value);
> -
> int (*parse)(struct hid_device *hdev);
>
> void (*request)(struct hid_device *hdev,
> --
> 1.8.3.1
>
^ permalink raw reply
* Re: [PATCH 09/11] HID: remove hid_get_raw_report in struct hid_device
From: David Herrmann @ 2014-02-03 16:12 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Benjamin Tissoires, Jiri Kosina, Frank Praznik,
open list:HID CORE LAYER, linux-kernel
In-Reply-To: <1391316630-29541-10-git-send-email-benjamin.tissoires@redhat.com>
Hi
On Sun, Feb 2, 2014 at 5:50 AM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> dev->hid_get_raw_report(X) and hid_hw_raw_request(X, HID_REQ_GET_REPORT)
> are strictly equivalent. Switch the hid subsystem to the hid_hw notation
> and remove the field .hid_get_raw_report in struct hid_device.
Finally we can remove that beast:
Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
Thanks
David
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
> drivers/hid/hid-input.c | 6 +++---
> drivers/hid/hid-sony.c | 3 ++-
> drivers/hid/hidraw.c | 7 ++++---
> drivers/hid/i2c-hid/i2c-hid.c | 1 -
> drivers/hid/uhid.c | 1 -
> drivers/hid/usbhid/hid-core.c | 1 -
> include/linux/hid.h | 3 ---
> net/bluetooth/hidp/core.c | 1 -
> 8 files changed, 9 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index 594722d..1de5997 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -350,9 +350,9 @@ static int hidinput_get_battery_property(struct power_supply *psy,
> ret = -ENOMEM;
> break;
> }
> - ret = dev->hid_get_raw_report(dev, dev->battery_report_id,
> - buf, 2,
> - dev->battery_report_type);
> + ret = hid_hw_raw_request(dev, dev->battery_report_id, buf, 2,
> + dev->battery_report_type,
> + HID_REQ_GET_REPORT);
>
> if (ret != 2) {
> ret = -ENODATA;
> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
> index 1235405..3930acb 100644
> --- a/drivers/hid/hid-sony.c
> +++ b/drivers/hid/hid-sony.c
> @@ -706,7 +706,8 @@ static int sixaxis_set_operational_usb(struct hid_device *hdev)
> if (!buf)
> return -ENOMEM;
>
> - ret = hdev->hid_get_raw_report(hdev, 0xf2, buf, 17, HID_FEATURE_REPORT);
> + ret = hid_hw_raw_request(hdev, 0xf2, buf, 17, HID_FEATURE_REPORT,
> + HID_REQ_GET_REPORT);
>
> if (ret < 0)
> hid_err(hdev, "can't set operational mode\n");
> diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
> index cb0137b..4b2dc95 100644
> --- a/drivers/hid/hidraw.c
> +++ b/drivers/hid/hidraw.c
> @@ -189,7 +189,7 @@ static ssize_t hidraw_get_report(struct file *file, char __user *buffer, size_t
>
> dev = hidraw_table[minor]->hid;
>
> - if (!dev->hid_get_raw_report) {
> + if (!dev->ll_driver->raw_request) {
> ret = -ENODEV;
> goto out;
> }
> @@ -216,14 +216,15 @@ static ssize_t hidraw_get_report(struct file *file, char __user *buffer, size_t
>
> /*
> * Read the first byte from the user. This is the report number,
> - * which is passed to dev->hid_get_raw_report().
> + * which is passed to hid_hw_raw_request().
> */
> if (copy_from_user(&report_number, buffer, 1)) {
> ret = -EFAULT;
> goto out_free;
> }
>
> - ret = dev->hid_get_raw_report(dev, report_number, buf, count, report_type);
> + ret = hid_hw_raw_request(dev, report_number, buf, count, report_type,
> + HID_REQ_GET_REPORT);
>
> if (ret < 0)
> goto out_free;
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> index 5099f1f..fe3b392 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -1029,7 +1029,6 @@ static int i2c_hid_probe(struct i2c_client *client,
>
> hid->driver_data = client;
> hid->ll_driver = &i2c_hid_ll_driver;
> - hid->hid_get_raw_report = i2c_hid_get_raw_report;
> hid->hid_output_raw_report = i2c_hid_output_raw_report;
> hid->dev.parent = &client->dev;
> ACPI_COMPANION_SET(&hid->dev, ACPI_COMPANION(&client->dev));
> diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
> index 438c9f1..7358346 100644
> --- a/drivers/hid/uhid.c
> +++ b/drivers/hid/uhid.c
> @@ -421,7 +421,6 @@ static int uhid_dev_create(struct uhid_device *uhid,
> hid->uniq[63] = 0;
>
> hid->ll_driver = &uhid_hid_driver;
> - hid->hid_get_raw_report = uhid_hid_get_raw;
> hid->hid_output_raw_report = uhid_hid_output_raw;
> hid->bus = ev->u.create.bus;
> hid->vendor = ev->u.create.vendor;
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index 406497b..b9a770f 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -1289,7 +1289,6 @@ static int usbhid_probe(struct usb_interface *intf, const struct usb_device_id *
>
> usb_set_intfdata(intf, hid);
> hid->ll_driver = &usb_hid_driver;
> - hid->hid_get_raw_report = usbhid_get_raw_report;
> hid->hid_output_raw_report = usbhid_output_raw_report;
> hid->ff_init = hid_pidff_init;
> #ifdef CONFIG_USB_HIDDEV
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index 38c307b..c56681a 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -508,9 +508,6 @@ struct hid_device { /* device report descriptor */
> struct hid_usage *, __s32);
> void (*hiddev_report_event) (struct hid_device *, struct hid_report *);
>
> - /* handler for raw input (Get_Report) data, used by hidraw */
> - int (*hid_get_raw_report) (struct hid_device *, unsigned char, __u8 *, size_t, unsigned char);
> -
> /* handler for raw output data, used by hidraw */
> int (*hid_output_raw_report) (struct hid_device *, __u8 *, size_t, unsigned char);
>
> diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
> index 02670b3..77c4bad 100644
> --- a/net/bluetooth/hidp/core.c
> +++ b/net/bluetooth/hidp/core.c
> @@ -773,7 +773,6 @@ static int hidp_setup_hid(struct hidp_session *session,
> hid->dev.parent = &session->conn->hcon->dev;
> hid->ll_driver = &hidp_hid_driver;
>
> - hid->hid_get_raw_report = hidp_get_raw_report;
> hid->hid_output_raw_report = hidp_output_raw_report;
>
> /* True if device is blacklisted in drivers/hid/hid-core.c */
> --
> 1.8.3.1
>
^ permalink raw reply
* Re: [PATCH 10/11] HID: introduce helper to access hid_output_raw_report()
From: David Herrmann @ 2014-02-03 16:14 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Benjamin Tissoires, Jiri Kosina, Frank Praznik,
open list:HID CORE LAYER, linux-kernel
In-Reply-To: <1391316630-29541-11-git-send-email-benjamin.tissoires@redhat.com>
Hi
On Sun, Feb 2, 2014 at 5:50 AM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> Add a helper to access hdev->hid_output_raw_report().
>
> To convert the drivers, use the following snippets:
>
> for i in drivers/hid/*.c
> do
> sed -i.bak "s/[^ \t]*->hid_output_raw_report(/hid_output_raw_report(/g" $i
> done
>
> Then manually fix for checkpatch.pl
Looks all good. I guess you didn't use the hid_hw_* prefix as it's not
a hdrv but hdev callback? But we hopefully can remove this soon,
anyway. So:
Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
Thanks
David
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
> drivers/hid/hid-input.c | 2 +-
> drivers/hid/hid-lg.c | 6 ++++--
> drivers/hid/hid-logitech-dj.c | 2 +-
> drivers/hid/hid-magicmouse.c | 2 +-
> drivers/hid/hid-sony.c | 5 +++--
> drivers/hid/hid-thingm.c | 4 ++--
> drivers/hid/hid-wacom.c | 16 +++++++---------
> drivers/hid/hid-wiimote-core.c | 2 +-
> drivers/hid/hidraw.c | 2 +-
> include/linux/hid.h | 16 ++++++++++++++++
> 10 files changed, 37 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index 1de5997..78293c3 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -1184,7 +1184,7 @@ static void hidinput_led_worker(struct work_struct *work)
>
> hid_output_report(report, buf);
> /* synchronous output report */
> - hid->hid_output_raw_report(hid, buf, len, HID_OUTPUT_REPORT);
> + hid_output_raw_report(hid, buf, len, HID_OUTPUT_REPORT);
> kfree(buf);
> }
>
> diff --git a/drivers/hid/hid-lg.c b/drivers/hid/hid-lg.c
> index 9fe9d4a..76ed7e5 100644
> --- a/drivers/hid/hid-lg.c
> +++ b/drivers/hid/hid-lg.c
> @@ -692,7 +692,8 @@ static int lg_probe(struct hid_device *hdev, const struct hid_device_id *id)
> if (hdev->product == USB_DEVICE_ID_LOGITECH_WII_WHEEL) {
> unsigned char buf[] = { 0x00, 0xAF, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 };
>
> - ret = hdev->hid_output_raw_report(hdev, buf, sizeof(buf), HID_FEATURE_REPORT);
> + ret = hid_output_raw_report(hdev, buf, sizeof(buf),
> + HID_FEATURE_REPORT);
>
> if (ret >= 0) {
> /* insert a little delay of 10 jiffies ~ 40ms */
> @@ -704,7 +705,8 @@ static int lg_probe(struct hid_device *hdev, const struct hid_device_id *id)
> buf[1] = 0xB2;
> get_random_bytes(&buf[2], 2);
>
> - ret = hdev->hid_output_raw_report(hdev, buf, sizeof(buf), HID_FEATURE_REPORT);
> + ret = hid_output_raw_report(hdev, buf, sizeof(buf),
> + HID_FEATURE_REPORT);
> }
> }
>
> diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
> index 61d2bbf..9347625 100644
> --- a/drivers/hid/hid-logitech-dj.c
> +++ b/drivers/hid/hid-logitech-dj.c
> @@ -567,7 +567,7 @@ static int logi_dj_output_hidraw_report(struct hid_device *hid, u8 * buf,
> 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,
> + ret = hid_output_raw_report(djrcv_dev->hdev, out_buf,
> DJREPORT_SHORT_LENGTH, report_type);
>
> kfree(out_buf);
> diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
> index 3b43d1c..cb5db3a 100644
> --- a/drivers/hid/hid-magicmouse.c
> +++ b/drivers/hid/hid-magicmouse.c
> @@ -538,7 +538,7 @@ static int magicmouse_probe(struct hid_device *hdev,
> * but there seems to be no other way of switching the mode.
> * Thus the super-ugly hacky success check below.
> */
> - ret = hdev->hid_output_raw_report(hdev, feature, sizeof(feature),
> + ret = hid_output_raw_report(hdev, feature, sizeof(feature),
> HID_FEATURE_REPORT);
> if (ret != -EIO && ret != sizeof(feature)) {
> hid_err(hdev, "unable to request touch data (%d)\n", ret);
> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
> index 3930acb..8494b8c 100644
> --- a/drivers/hid/hid-sony.c
> +++ b/drivers/hid/hid-sony.c
> @@ -720,7 +720,8 @@ static int sixaxis_set_operational_usb(struct hid_device *hdev)
> static int sixaxis_set_operational_bt(struct hid_device *hdev)
> {
> unsigned char buf[] = { 0xf4, 0x42, 0x03, 0x00, 0x00 };
> - return hdev->hid_output_raw_report(hdev, buf, sizeof(buf), HID_FEATURE_REPORT);
> + return hid_output_raw_report(hdev, buf, sizeof(buf),
> + HID_FEATURE_REPORT);
> }
>
> static void buzz_set_leds(struct hid_device *hdev, const __u8 *leds)
> @@ -942,7 +943,7 @@ static void sixaxis_state_worker(struct work_struct *work)
> buf[10] |= sc->led_state[2] << 3;
> buf[10] |= sc->led_state[3] << 4;
>
> - sc->hdev->hid_output_raw_report(sc->hdev, buf, sizeof(buf),
> + hid_output_raw_report(sc->hdev, buf, sizeof(buf),
> HID_OUTPUT_REPORT);
> }
>
> diff --git a/drivers/hid/hid-thingm.c b/drivers/hid/hid-thingm.c
> index 99342cf..7dd3197 100644
> --- a/drivers/hid/hid-thingm.c
> +++ b/drivers/hid/hid-thingm.c
> @@ -48,8 +48,8 @@ static int blink1_send_command(struct blink1_data *data,
> buf[0], buf[1], buf[2], buf[3], buf[4],
> buf[5], buf[6], buf[7], buf[8]);
>
> - ret = data->hdev->hid_output_raw_report(data->hdev, buf,
> - BLINK1_CMD_SIZE, HID_FEATURE_REPORT);
> + ret = hid_output_raw_report(data->hdev, buf, BLINK1_CMD_SIZE,
> + HID_FEATURE_REPORT);
>
> return ret < 0 ? ret : 0;
> }
> diff --git a/drivers/hid/hid-wacom.c b/drivers/hid/hid-wacom.c
> index 60c75dc..c720db9 100644
> --- a/drivers/hid/hid-wacom.c
> +++ b/drivers/hid/hid-wacom.c
> @@ -128,8 +128,7 @@ static void wacom_set_image(struct hid_device *hdev, const char *image,
>
> rep_data[0] = WAC_CMD_ICON_START_STOP;
> rep_data[1] = 0;
> - ret = hdev->hid_output_raw_report(hdev, rep_data, 2,
> - HID_FEATURE_REPORT);
> + ret = hid_output_raw_report(hdev, rep_data, 2, HID_FEATURE_REPORT);
> if (ret < 0)
> goto err;
>
> @@ -143,15 +142,14 @@ static void wacom_set_image(struct hid_device *hdev, const char *image,
> rep_data[j + 3] = p[(i << 6) + j];
>
> rep_data[2] = i;
> - ret = hdev->hid_output_raw_report(hdev, rep_data, 67,
> + ret = hid_output_raw_report(hdev, rep_data, 67,
> HID_FEATURE_REPORT);
> }
>
> rep_data[0] = WAC_CMD_ICON_START_STOP;
> rep_data[1] = 0;
>
> - ret = hdev->hid_output_raw_report(hdev, rep_data, 2,
> - HID_FEATURE_REPORT);
> + ret = hid_output_raw_report(hdev, rep_data, 2, HID_FEATURE_REPORT);
>
> err:
> return;
> @@ -183,7 +181,7 @@ static void wacom_leds_set_brightness(struct led_classdev *led_dev,
> buf[3] = value;
> /* use fixed brightness for OLEDs */
> buf[4] = 0x08;
> - hdev->hid_output_raw_report(hdev, buf, 9, HID_FEATURE_REPORT);
> + hid_output_raw_report(hdev, buf, 9, HID_FEATURE_REPORT);
> kfree(buf);
> }
>
> @@ -339,7 +337,7 @@ static void wacom_set_features(struct hid_device *hdev, u8 speed)
> rep_data[0] = 0x03 ; rep_data[1] = 0x00;
> limit = 3;
> do {
> - ret = hdev->hid_output_raw_report(hdev, rep_data, 2,
> + ret = hid_output_raw_report(hdev, rep_data, 2,
> HID_FEATURE_REPORT);
> } while (ret < 0 && limit-- > 0);
>
> @@ -352,7 +350,7 @@ static void wacom_set_features(struct hid_device *hdev, u8 speed)
> rep_data[1] = 0x00;
> limit = 3;
> do {
> - ret = hdev->hid_output_raw_report(hdev,
> + ret = hid_output_raw_report(hdev,
> rep_data, 2, HID_FEATURE_REPORT);
> } while (ret < 0 && limit-- > 0);
>
> @@ -378,7 +376,7 @@ static void wacom_set_features(struct hid_device *hdev, u8 speed)
> rep_data[0] = 0x03;
> rep_data[1] = wdata->features;
>
> - ret = hdev->hid_output_raw_report(hdev, rep_data, 2,
> + ret = hid_output_raw_report(hdev, rep_data, 2,
> HID_FEATURE_REPORT);
> if (ret >= 0)
> wdata->high_speed = speed;
> diff --git a/drivers/hid/hid-wiimote-core.c b/drivers/hid/hid-wiimote-core.c
> index abb20db..d7dc6c5b 100644
> --- a/drivers/hid/hid-wiimote-core.c
> +++ b/drivers/hid/hid-wiimote-core.c
> @@ -35,7 +35,7 @@ static int wiimote_hid_send(struct hid_device *hdev, __u8 *buffer,
> if (!buf)
> return -ENOMEM;
>
> - ret = hdev->hid_output_raw_report(hdev, buf, count, HID_OUTPUT_REPORT);
> + ret = hid_output_raw_report(hdev, buf, count, HID_OUTPUT_REPORT);
>
> kfree(buf);
> return ret;
> diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
> index 4b2dc95..f8708c9 100644
> --- a/drivers/hid/hidraw.c
> +++ b/drivers/hid/hidraw.c
> @@ -153,7 +153,7 @@ static ssize_t hidraw_send_report(struct file *file, const char __user *buffer,
> goto out_free;
> }
>
> - ret = dev->hid_output_raw_report(dev, buf, count, report_type);
> + ret = hid_output_raw_report(dev, buf, count, report_type);
> out_free:
> kfree(buf);
> out:
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index c56681a..a837ede 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -1012,6 +1012,22 @@ static inline int hid_hw_output_report(struct hid_device *hdev, __u8 *buf,
> }
>
> /**
> + * hid_output_raw_report - send an output or a feature report to the device
> + *
> + * @hdev: hid device
> + * @buf: raw data to transfer
> + * @len: length of buf
> + * @report_type: HID_FEATURE_REPORT or HID_OUTPUT_REPORT
> + *
> + * @return: count of data transfered, negative if error
> + */
> +static inline int hid_output_raw_report(struct hid_device *hdev, __u8 *buf,
> + size_t len, unsigned char report_type)
> +{
> + return hdev->hid_output_raw_report(hdev, buf, len, report_type);
> +}
> +
> +/**
> * hid_hw_idle - send idle request to device
> *
> * @hdev: hid device
> --
> 1.8.3.1
>
^ permalink raw reply
* Re: [PATCH 04/11] HID: logitech-dj: remove hidinput_input_event
From: Benjamin Tissoires @ 2014-02-03 16:21 UTC (permalink / raw)
To: David Herrmann
Cc: Benjamin Tissoires, Jiri Kosina, Frank Praznik,
open list:HID CORE LAYER, linux-kernel
In-Reply-To: <CANq1E4Rn=rijG8Xv97okj2k4iv=ZG6wa30xQrxgjhutQovD+dg@mail.gmail.com>
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.
Cheers,
Benjamin
>
> Thanks
> David
>
>> +
>> + kfree(out_buf);
>> + return ret;
>> }
>>
>> static void rdcat(char *rdesc, unsigned int *rsize, const char *data, unsigned int size)
>> @@ -613,58 +637,6 @@ static int logi_dj_ll_parse(struct hid_device *hid)
>> return retval;
>> }
>>
>> -static int logi_dj_ll_input_event(struct input_dev *dev, unsigned int type,
>> - unsigned int code, int value)
>> -{
>> - /* Sent by the input layer to handle leds and Force Feedback */
>> - struct hid_device *dj_hiddev = input_get_drvdata(dev);
>> - struct dj_device *dj_dev = dj_hiddev->driver_data;
>> -
>> - struct dj_receiver_dev *djrcv_dev =
>> - dev_get_drvdata(dj_hiddev->dev.parent);
>> - struct hid_device *dj_rcv_hiddev = djrcv_dev->hdev;
>> - struct hid_report_enum *output_report_enum;
>> -
>> - struct hid_field *field;
>> - struct hid_report *report;
>> - unsigned char *data;
>> - int offset;
>> -
>> - dbg_hid("%s: %s, type:%d | code:%d | value:%d\n",
>> - __func__, dev->phys, type, code, value);
>> -
>> - if (type != EV_LED)
>> - return -1;
>> -
>> - offset = hidinput_find_field(dj_hiddev, type, code, &field);
>> -
>> - if (offset == -1) {
>> - dev_warn(&dev->dev, "event field not found\n");
>> - return -1;
>> - }
>> - hid_set_field(field, offset, value);
>> -
>> - data = hid_alloc_report_buf(field->report, GFP_ATOMIC);
>> - if (!data) {
>> - dev_warn(&dev->dev, "failed to allocate report buf memory\n");
>> - return -1;
>> - }
>> -
>> - hid_output_report(field->report, &data[0]);
>> -
>> - output_report_enum = &dj_rcv_hiddev->report_enum[HID_OUTPUT_REPORT];
>> - report = output_report_enum->report_id_hash[REPORT_ID_DJ_SHORT];
>> - hid_set_field(report->field[0], 0, dj_dev->device_index);
>> - hid_set_field(report->field[0], 1, REPORT_TYPE_LEDS);
>> - hid_set_field(report->field[0], 2, data[1]);
>> -
>> - hid_hw_request(dj_rcv_hiddev, report, HID_REQ_SET_REPORT);
>> -
>> - kfree(data);
>> -
>> - return 0;
>> -}
>> -
>> static int logi_dj_ll_start(struct hid_device *hid)
>> {
>> dbg_hid("%s\n", __func__);
>> @@ -683,7 +655,6 @@ static struct hid_ll_driver logi_dj_ll_driver = {
>> .stop = logi_dj_ll_stop,
>> .open = logi_dj_ll_open,
>> .close = logi_dj_ll_close,
>> - .hidinput_input_event = logi_dj_ll_input_event,
>> };
>>
>>
>> --
>> 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: David Herrmann @ 2014-02-03 16:24 UTC (permalink / raw)
To: Benjamin Tissoires; +Cc: Frank Praznik, Frank Praznik, linux-input, Jiri Kosina
In-Reply-To: <CAN+gG=H6A93ozP=Q52GXnEjARC1tX4s6k=Oi6MoHK7Mh6kBPiw@mail.gmail.com>
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
^ permalink raw reply
* Re: [PATCH v2 1/6] HID: sony: Use low-level transport driver functions
From: David Herrmann @ 2014-02-03 16:26 UTC (permalink / raw)
To: Frank Praznik; +Cc: open list:HID CORE LAYER, Jiri Kosina
In-Reply-To: <1391102648-19381-2-git-send-email-frank.praznik@oh.rr.com>
Hi
On Thu, Jan 30, 2014 at 6:24 PM, Frank Praznik <frank.praznik@oh.rr.com> wrote:
> From: Franz Frank Praznik <frank.praznik@oh.rr.com>
>
> Change the dualshock4_state_worker function to use the low-level output_report
> function.
>
> Remove sony_set_output_report since it is no longer used.
>
> Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
> ---
> drivers/hid/hid-sony.c | 58 ++++++++++++++++----------------------------------
> 1 file changed, 18 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
> index 2bd3f13..7a6d7c9 100644
> --- a/drivers/hid/hid-sony.c
> +++ b/drivers/hid/hid-sony.c
> @@ -502,7 +502,6 @@ struct sony_sc {
> spinlock_t lock;
> struct hid_device *hdev;
> struct led_classdev *leds[MAX_LEDS];
> - struct hid_report *output_report;
> unsigned long quirks;
> struct work_struct state_worker;
> struct power_supply battery;
> @@ -1053,21 +1052,26 @@ static void dualshock4_state_worker(struct work_struct *work)
> {
> struct sony_sc *sc = container_of(work, struct sony_sc, state_worker);
> struct hid_device *hdev = sc->hdev;
> - struct hid_report *report = sc->output_report;
> - __s32 *value = report->field[0]->value;
> + int offset;
> +
> + __u8 buf[32] = { 0 };
>
> - value[0] = 0x03;
> + buf[0] = 0x05;
> + buf[1] = 0x03;
> + offset = 4;
>
> #ifdef CONFIG_SONY_FF
> - value[3] = sc->right;
> - value[4] = sc->left;
> + buf[offset++] = sc->right;
> + buf[offset++] = sc->left;
> +#else
> + offset += 2;
> #endif
>
> - value[5] = sc->led_state[0];
> - value[6] = sc->led_state[1];
> - value[7] = sc->led_state[2];
> + buf[offset++] = sc->led_state[0];
> + buf[offset++] = sc->led_state[1];
> + buf[offset++] = sc->led_state[2];
>
> - hid_hw_request(hdev, report, HID_REQ_SET_REPORT);
> + hdev->ll_driver->output_report(hdev, buf, sizeof(buf));
Ugh? You change SET_REPORT to output_report() here, which is not
supposed to be identical. Are you sure that works? A correct HID
device should accept both, but we're talking about gamepads here..
Thanks
David
> }
>
> #ifdef CONFIG_SONY_FF
> @@ -1200,33 +1204,6 @@ static void sony_battery_remove(struct sony_sc *sc)
> sc->battery.name = NULL;
> }
>
> -static int sony_set_output_report(struct sony_sc *sc, int req_id, int req_size)
> -{
> - struct list_head *head, *list;
> - struct hid_report *report;
> - struct hid_device *hdev = sc->hdev;
> -
> - list = &hdev->report_enum[HID_OUTPUT_REPORT].report_list;
> -
> - list_for_each(head, list) {
> - report = list_entry(head, struct hid_report, list);
> -
> - if (report->id == req_id) {
> - if (report->size < req_size) {
> - hid_err(hdev, "Output report 0x%02x (%i bits) is smaller than requested size (%i bits)\n",
> - req_id, report->size, req_size);
> - return -EINVAL;
> - }
> - sc->output_report = report;
> - return 0;
> - }
> - }
> -
> - hid_err(hdev, "Unable to locate output report 0x%02x\n", req_id);
> -
> - return -EINVAL;
> -}
> -
> static int sony_register_touchpad(struct sony_sc *sc, int touch_count,
> int w, int h)
> {
> @@ -1291,10 +1268,11 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
> else if (sc->quirks & SIXAXIS_CONTROLLER_BT)
> ret = sixaxis_set_operational_bt(hdev);
> else if (sc->quirks & DUALSHOCK4_CONTROLLER_USB) {
> - /* Report 5 (31 bytes) is used to send data to the controller via USB */
> - ret = sony_set_output_report(sc, 0x05, 248);
> - if (ret < 0)
> + if (hdev->ll_driver->output_report == NULL) {
> + hid_err(hdev, "NULL output_report handler\n");
> + ret = -EINVAL;
> goto err_stop;
> + }
>
> /* The Dualshock 4 touchpad supports 2 touches and has a
> * resolution of 1920x940.
> --
> 1.8.5.3
>
^ permalink raw reply
* Re: [PATCH v2 3/6] HID: sony: Add Bluetooth output report formatting
From: David Herrmann @ 2014-02-03 16:29 UTC (permalink / raw)
To: Frank Praznik; +Cc: open list:HID CORE LAYER, Jiri Kosina
In-Reply-To: <1391102648-19381-4-git-send-email-frank.praznik@oh.rr.com>
Hi
On Thu, Jan 30, 2014 at 6:24 PM, Frank Praznik <frank.praznik@oh.rr.com> wrote:
> Add formatting and reporting for Dualshock 4 output reports on Bluetooth.
>
> Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
>
> ---
> drivers/hid/hid-sony.c | 29 +++++++++++++++++++++++------
> 1 file changed, 23 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
> index 52162a9..4d12b4e 100644
> --- a/drivers/hid/hid-sony.c
> +++ b/drivers/hid/hid-sony.c
> @@ -1268,11 +1268,18 @@ static void dualshock4_state_worker(struct work_struct *work)
> struct hid_device *hdev = sc->hdev;
> int offset;
>
> - __u8 buf[32] = { 0 };
> + __u8 buf[78] = { 0 };
>
> - buf[0] = 0x05;
> - buf[1] = 0x03;
> - offset = 4;
> + if (sc->quirks & DUALSHOCK4_CONTROLLER_USB) {
> + buf[0] = 0x05;
> + buf[1] = 0x03;
> + offset = 4;
> + } else {
> + buf[0] = 0x11;
> + buf[1] = 0xB0;
> + buf[3] = 0x0F;
> + offset = 6;
> + }
>
> #ifdef CONFIG_SONY_FF
> buf[offset++] = sc->right;
> @@ -1285,7 +1292,11 @@ static void dualshock4_state_worker(struct work_struct *work)
> buf[offset++] = sc->led_state[1];
> buf[offset++] = sc->led_state[2];
>
> - hdev->ll_driver->output_report(hdev, buf, sizeof(buf));
> + if (sc->quirks & DUALSHOCK4_CONTROLLER_USB)
> + hdev->ll_driver->output_report(hdev, buf, 32);
> + else
> + hdev->ll_driver->raw_request(hdev, 0x11, buf, 78,
> + HID_OUTPUT_REPORT, HID_REQ_SET_REPORT);
Ok, now I get it. The USB stuff takes output-reports and BT uses
SET_REPORT. That's exactly the inverse of wiimotes.
> }
>
> #ifdef CONFIG_SONY_FF
> @@ -1482,10 +1493,16 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
> else if (sc->quirks & SIXAXIS_CONTROLLER_BT)
> ret = sixaxis_set_operational_bt(hdev);
> else if (sc->quirks & DUALSHOCK4_CONTROLLER_USB) {
> - if (hdev->ll_driver->output_report == NULL) {
> + if ((sc->quirks & DUALSHOCK4_CONTROLLER_USB) &&
> + hdev->ll_driver->output_report == NULL) {
> hid_err(hdev, "NULL output_report handler\n");
> ret = -EINVAL;
> goto err_stop;
> + } else if ((sc->quirks & DUALSHOCK4_CONTROLLER_BT) &&
> + hdev->ll_driver->raw_request == NULL) {
> + hid_err(hdev, "NULL raw_request handler\n");
> + ret = -EINVAL;
> + goto err_stop;
If you rebase your patches on Benjamins series, you can use
hid_hw_output_report() and hid_hw_raw_request() which already to the
NULL check. So you can drop this stuff here.
Thanks
David
> }
>
> /* The Dualshock 4 touchpad supports 2 touches and has a
> --
> 1.8.5.3
>
^ permalink raw reply
* Re: [PATCH v2 6/6] HID: sony: Add conditionals to enable all features in Bluetooth mode
From: David Herrmann @ 2014-02-03 16:31 UTC (permalink / raw)
To: Frank Praznik; +Cc: open list:HID CORE LAYER, Jiri Kosina
In-Reply-To: <1391102648-19381-7-git-send-email-frank.praznik@oh.rr.com>
Hi
On Thu, Jan 30, 2014 at 6:24 PM, Frank Praznik <frank.praznik@oh.rr.com> wrote:
> Add the necessary conditionals to enable battery reporting, rumble, LED
> settings and touchpad parsing.
>
> Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
>
> ---
> drivers/hid/hid-sony.c | 20 +++++++++++++-------
> 1 file changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
> index eee56d7..d920362 100644
> --- a/drivers/hid/hid-sony.c
> +++ b/drivers/hid/hid-sony.c
> @@ -44,8 +44,10 @@
> #define DUALSHOCK4_CONTROLLER_USB BIT(5)
> #define DUALSHOCK4_CONTROLLER_BT BIT(6)
>
> -#define SONY_LED_SUPPORT (SIXAXIS_CONTROLLER_USB | BUZZ_CONTROLLER | DUALSHOCK4_CONTROLLER_USB)
> -#define SONY_BATTERY_SUPPORT (SIXAXIS_CONTROLLER_USB | SIXAXIS_CONTROLLER_BT | DUALSHOCK4_CONTROLLER_USB)
> +#define SONY_LED_SUPPORT (SIXAXIS_CONTROLLER_USB | BUZZ_CONTROLLER | \
> + DUALSHOCK4_CONTROLLER_USB | DUALSHOCK4_CONTROLLER_BT)
> +#define SONY_BATTERY_SUPPORT (SIXAXIS_CONTROLLER_USB | SIXAXIS_CONTROLLER_BT | \
> + DUALSHOCK4_CONTROLLER_USB | DUALSHOCK4_CONTROLLER_BT)
How about adding:
#define DUALSHOCK4_CONTROLLER (DUALSHOCK4_CONTROLLER_USB |
DUALSHOCK4_CONTROLLER_BT)
and then simplifying the chunks below? Seems a bit redundant to always
check for both, _USB and _BT in the dualshock4 case.
Thanks
David
>
> #define MAX_LEDS 4
>
> @@ -939,8 +941,9 @@ static int sony_raw_event(struct hid_device *hdev, struct hid_report *report,
> swap(rd[47], rd[48]);
>
> sixaxis_parse_report(sc, rd, size);
> - } else if ((sc->quirks & DUALSHOCK4_CONTROLLER_USB) && rd[0] == 0x01 &&
> - size == 64) {
> + } else if (((sc->quirks & DUALSHOCK4_CONTROLLER_USB) && rd[0] == 0x01 &&
> + size == 64) || ((sc->quirks & DUALSHOCK4_CONTROLLER_BT)
> + && rd[0] == 0x11 && size == 78)) {
> dualshock4_parse_report(sc, rd, size);
> }
>
> @@ -1079,7 +1082,8 @@ static void sony_set_leds(struct hid_device *hdev, const __u8 *leds, int count)
> if (drv_data->quirks & BUZZ_CONTROLLER && count == 4) {
> buzz_set_leds(hdev, leds);
> } else if ((drv_data->quirks & SIXAXIS_CONTROLLER_USB) ||
> - (drv_data->quirks & DUALSHOCK4_CONTROLLER_USB)) {
> + (drv_data->quirks & DUALSHOCK4_CONTROLLER_USB) ||
> + (drv_data->quirks & DUALSHOCK4_CONTROLLER_BT)) {
> for (n = 0; n < count; n++)
> drv_data->led_state[n] = leds[n];
> schedule_work(&drv_data->state_worker);
> @@ -1184,7 +1188,8 @@ static int sony_leds_init(struct hid_device *hdev)
> /* Validate expected report characteristics. */
> if (!hid_validate_values(hdev, HID_OUTPUT_REPORT, 0, 0, 7))
> return -ENODEV;
> - } else if (drv_data->quirks & DUALSHOCK4_CONTROLLER_USB) {
> + } else if ((drv_data->quirks & DUALSHOCK4_CONTROLLER_USB) ||
> + (drv_data->quirks & DUALSHOCK4_CONTROLLER_BT)) {
> drv_data->led_count = 3;
> max_brightness = 255;
> use_colors = 1;
> @@ -1509,7 +1514,8 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
> }
> else if (sc->quirks & SIXAXIS_CONTROLLER_BT)
> ret = sixaxis_set_operational_bt(hdev);
> - else if (sc->quirks & DUALSHOCK4_CONTROLLER_USB) {
> + else if ((sc->quirks & DUALSHOCK4_CONTROLLER_USB) ||
> + (sc->quirks & DUALSHOCK4_CONTROLLER_BT)) {
> if ((sc->quirks & DUALSHOCK4_CONTROLLER_USB) &&
> hdev->ll_driver->output_report == NULL) {
> hid_err(hdev, "NULL output_report handler\n");
> --
> 1.8.5.3
>
^ permalink raw reply
* Re: [PATCH 00/11] HID: spring cleaning
From: David Herrmann @ 2014-02-03 16:48 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Benjamin Tissoires, Jiri Kosina, Frank Praznik,
open list:HID CORE LAYER, linux-kernel
In-Reply-To: <1391316630-29541-1-git-send-email-benjamin.tissoires@redhat.com>
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.
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] HID: Add HID transport driver documentation
From: David Herrmann @ 2014-02-03 16:50 UTC (permalink / raw)
To: Benjamin Tissoires; +Cc: Frank Praznik, open list:HID CORE LAYER, Jiri Kosina
In-Reply-To: <CAN+gG=Gi6XUkRED_Bah63TqT80G5x7wn2L33=BFsMYr94geWxA@mail.gmail.com>
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.
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