All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] Bluetooth:  hidp: cleanup HID init and re-enable hidinput_input_event callback
@ 2013-07-11 13:41 Benjamin Tissoires
  2013-07-11 13:41   ` Benjamin Tissoires
  2013-07-11 13:41 ` [PATCH 2/2] Bluetooth: hidp: remove wrong send_report at init Benjamin Tissoires
  0 siblings, 2 replies; 15+ messages in thread
From: Benjamin Tissoires @ 2013-07-11 13:41 UTC (permalink / raw)
  To: Benjamin Tissoires, Marcel Holtmann, Gustavo Padovan,
	David Herrmann, Jiri Kosina, linux-bluetooth, linux-input,
	linux-kernel

Hi guys,

These two patches enhance a little bit the HIDp driver:
- the first one re-implement in a safer way the callback hidinput_input_event,
  allowing keyboards leds to be set over bluetooth
- the second one disable the send_report commands called during init, which
  are not part of the spec and may fail with some devices (the Wiimote is in
  this case, but David guarded it against this problem).

Cheers,
Benjamin

Benjamin Tissoires (2):
  Bluetooth: hidp: implement hidinput_input_event callback
  Bluetooth: hidp: remove wrong send_report at init

 net/bluetooth/hidp/core.c | 40 ++++++++++++++++++++++++++--------------
 1 file changed, 26 insertions(+), 14 deletions(-)

-- 
1.8.3.1


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 1/2] Bluetooth: hidp: implement hidinput_input_event callback
@ 2013-07-11 13:41   ` Benjamin Tissoires
  0 siblings, 0 replies; 15+ messages in thread
From: Benjamin Tissoires @ 2013-07-11 13:41 UTC (permalink / raw)
  To: Benjamin Tissoires, Marcel Holtmann, Gustavo Padovan,
	David Herrmann, Jiri Kosina, linux-bluetooth, linux-input,
	linux-kernel

We can re-enable hidinput_input_event to allow the leds of bluetooth
keyboards to be set.
Now the callbacks uses hid core to retrieve the right HID report to
send, so this version is safer.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 net/bluetooth/hidp/core.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index f13a8da..9c8b50d 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -238,6 +238,31 @@ static int hidp_send_report(struct hidp_session *session, struct hid_report *rep
 	return hidp_send_intr_message(session, hdr, buf, rsize);
 }
 
+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,
@@ -711,6 +736,7 @@ static struct hid_ll_driver hidp_hid_driver = {
 	.stop = hidp_stop,
 	.open  = hidp_open,
 	.close = hidp_close,
+	.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 related	[flat|nested] 15+ messages in thread

* [PATCH 1/2] Bluetooth: hidp: implement hidinput_input_event callback
@ 2013-07-11 13:41   ` Benjamin Tissoires
  0 siblings, 0 replies; 15+ messages in thread
From: Benjamin Tissoires @ 2013-07-11 13:41 UTC (permalink / raw)
  To: Benjamin Tissoires, Marcel Holtmann, Gustavo Padovan,
	David Herrmann, Jiri Kosina,
	linux-bluetooth-u79uwXL29TY76Z2rM5mHXA,
	linux-input-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA

We can re-enable hidinput_input_event to allow the leds of bluetooth
keyboards to be set.
Now the callbacks uses hid core to retrieve the right HID report to
send, so this version is safer.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 net/bluetooth/hidp/core.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index f13a8da..9c8b50d 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -238,6 +238,31 @@ static int hidp_send_report(struct hidp_session *session, struct hid_report *rep
 	return hidp_send_intr_message(session, hdr, buf, rsize);
 }
 
+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,
@@ -711,6 +736,7 @@ static struct hid_ll_driver hidp_hid_driver = {
 	.stop = hidp_stop,
 	.open  = hidp_open,
 	.close = hidp_close,
+	.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 related	[flat|nested] 15+ messages in thread

* [PATCH 2/2] Bluetooth: hidp: remove wrong send_report at init
  2013-07-11 13:41 [PATCH 0/2] Bluetooth: hidp: cleanup HID init and re-enable hidinput_input_event callback Benjamin Tissoires
  2013-07-11 13:41   ` Benjamin Tissoires
@ 2013-07-11 13:41 ` Benjamin Tissoires
  2013-07-11 13:51   ` David Herrmann
                     ` (2 more replies)
  1 sibling, 3 replies; 15+ messages in thread
From: Benjamin Tissoires @ 2013-07-11 13:41 UTC (permalink / raw)
  To: Benjamin Tissoires, Marcel Holtmann, Gustavo Padovan,
	David Herrmann, Jiri Kosina, linux-bluetooth, linux-input,
	linux-kernel

The USB hid implementation does retrieve the reports during the start.
However, this implementation does not call the HID command GET_REPORT
(which would fetch the current status of each report), but use the
DATA command, which is an Output Report (so transmitting data from the
host to the device).
The Wiimote controller is already guarded against this problem in the
protocol, but it is not conformant to the specification to set all the
reports to 0 on start.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 net/bluetooth/hidp/core.c | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index 9c8b50d..59d132a 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -703,20 +703,6 @@ static int hidp_parse(struct hid_device *hid)
 
 static int hidp_start(struct hid_device *hid)
 {
-	struct hidp_session *session = hid->driver_data;
-	struct hid_report *report;
-
-	if (hid->quirks & HID_QUIRK_NO_INIT_REPORTS)
-		return 0;
-
-	list_for_each_entry(report, &hid->report_enum[HID_INPUT_REPORT].
-			report_list, list)
-		hidp_send_report(session, report);
-
-	list_for_each_entry(report, &hid->report_enum[HID_FEATURE_REPORT].
-			report_list, list)
-		hidp_send_report(session, report);
-
 	return 0;
 }
 
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/2] Bluetooth: hidp: remove wrong send_report at init
  2013-07-11 13:41 ` [PATCH 2/2] Bluetooth: hidp: remove wrong send_report at init Benjamin Tissoires
@ 2013-07-11 13:51   ` David Herrmann
  2013-07-11 22:51   ` Jiri Kosina
  2013-07-12 10:09     ` Gustavo Padovan
  2 siblings, 0 replies; 15+ messages in thread
From: David Herrmann @ 2013-07-11 13:51 UTC (permalink / raw)
  To: Benjamin Tissoires
  Cc: Benjamin Tissoires, Marcel Holtmann, Gustavo Padovan,
	Jiri Kosina, linux-bluetooth, open list:HID CORE LAYER,
	linux-kernel

Hi

On Thu, Jul 11, 2013 at 3:41 PM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> The USB hid implementation does retrieve the reports during the start.
> However, this implementation does not call the HID command GET_REPORT
> (which would fetch the current status of each report), but use the
> DATA command, which is an Output Report (so transmitting data from the
> host to the device).
> The Wiimote controller is already guarded against this problem in the
> protocol, but it is not conformant to the specification to set all the
> reports to 0 on start.

I always wondered whether report-setup is really needed for BT-HIDP.
The BT profile doesn't mention it but I thought it was part of the
USBHID core specification.
During hid-wiimote development I added support for
HID_QUIRK_NO_INIT_REPORTS to HIDP to silence the wiimote errors. But
if you say that it's specific to USBHID, I am fine with this. I never
read the USBHID specs, though, I rely on your comment here.

Anyway, code looks good:
  Reviewed-by: David Herrmann <dh.herrmann@gmail.com>

Thanks!
David

> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
>  net/bluetooth/hidp/core.c | 14 --------------
>  1 file changed, 14 deletions(-)
>
> diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
> index 9c8b50d..59d132a 100644
> --- a/net/bluetooth/hidp/core.c
> +++ b/net/bluetooth/hidp/core.c
> @@ -703,20 +703,6 @@ static int hidp_parse(struct hid_device *hid)
>
>  static int hidp_start(struct hid_device *hid)
>  {
> -       struct hidp_session *session = hid->driver_data;
> -       struct hid_report *report;
> -
> -       if (hid->quirks & HID_QUIRK_NO_INIT_REPORTS)
> -               return 0;
> -
> -       list_for_each_entry(report, &hid->report_enum[HID_INPUT_REPORT].
> -                       report_list, list)
> -               hidp_send_report(session, report);
> -
> -       list_for_each_entry(report, &hid->report_enum[HID_FEATURE_REPORT].
> -                       report_list, list)
> -               hidp_send_report(session, report);
> -
>         return 0;
>  }
>
> --
> 1.8.3.1
>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] Bluetooth: hidp: implement hidinput_input_event callback
  2013-07-11 13:41   ` Benjamin Tissoires
  (?)
@ 2013-07-11 14:02   ` David Herrmann
  2013-07-11 14:10     ` Benjamin Tissoires
  -1 siblings, 1 reply; 15+ messages in thread
From: David Herrmann @ 2013-07-11 14:02 UTC (permalink / raw)
  To: Benjamin Tissoires
  Cc: Benjamin Tissoires, Marcel Holtmann, Gustavo Padovan,
	Jiri Kosina, linux-bluetooth, open list:HID CORE LAYER,
	linux-kernel

Hi

On Thu, Jul 11, 2013 at 3:41 PM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> We can re-enable hidinput_input_event to allow the leds of bluetooth
> keyboards to be set.
> Now the callbacks uses hid core to retrieve the right HID report to
> send, so this version is safer.
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
>  net/bluetooth/hidp/core.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>
> diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
> index f13a8da..9c8b50d 100644
> --- a/net/bluetooth/hidp/core.c
> +++ b/net/bluetooth/hidp/core.c
> @@ -238,6 +238,31 @@ static int hidp_send_report(struct hidp_session *session, struct hid_report *rep
>         return hidp_send_intr_message(session, hdr, buf, rsize);
>  }
>
> +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);

I dislike that we have to deal with input_get_drvdata() in a transport
driver. It is not obvious from the transport driver that this gives us
an hid_device. But ok, that's how we do it in usbhid and hid-input so
it's another issue..

> +       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);
> +}
> +

We had this discussion before (regarding uhid and hidpinput_event), it
would be nice to have a helper in hid-input.c which does this. We copy
the code into every transport driver, which bugs me. But no-one
stepped up to do this, so I am fine.

Reviewed-by: David Herrmann <dh.herrmann@gmail.com>

Thanks!
David

>  static int hidp_get_raw_report(struct hid_device *hid,
>                 unsigned char report_number,
>                 unsigned char *data, size_t count,
> @@ -711,6 +736,7 @@ static struct hid_ll_driver hidp_hid_driver = {
>         .stop = hidp_stop,
>         .open  = hidp_open,
>         .close = hidp_close,
> +       .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	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] Bluetooth: hidp: implement hidinput_input_event callback
  2013-07-11 14:02   ` David Herrmann
@ 2013-07-11 14:10     ` Benjamin Tissoires
  2013-07-11 16:07       ` David Herrmann
  0 siblings, 1 reply; 15+ messages in thread
From: Benjamin Tissoires @ 2013-07-11 14:10 UTC (permalink / raw)
  To: David Herrmann
  Cc: Benjamin Tissoires, Marcel Holtmann, Gustavo Padovan,
	Jiri Kosina, linux-bluetooth, open list:HID CORE LAYER,
	linux-kernel

Hi David,

On Thu, Jul 11, 2013 at 4:02 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
>> +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);
>
> I dislike that we have to deal with input_get_drvdata() in a transport
> driver. It is not obvious from the transport driver that this gives us
> an hid_device. But ok, that's how we do it in usbhid and hid-input so
> it's another issue..

right. So we should definitively change the callback signature here.

>
>> +       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);
>> +}
>> +
>
> We had this discussion before (regarding uhid and hidpinput_event), it
> would be nice to have a helper in hid-input.c which does this. We copy
> the code into every transport driver, which bugs me. But no-one
> stepped up to do this, so I am fine.

oh, yes, you are right. I'll add it to my todo list (now that I am
responsible of 2/4 code duplication :-S )

>
> Reviewed-by: David Herrmann <dh.herrmann@gmail.com>

Thanks for the review!

Cheers,
Benjamin

>
> Thanks!
> David
>
>>  static int hidp_get_raw_report(struct hid_device *hid,
>>                 unsigned char report_number,
>>                 unsigned char *data, size_t count,
>> @@ -711,6 +736,7 @@ static struct hid_ll_driver hidp_hid_driver = {
>>         .stop = hidp_stop,
>>         .open  = hidp_open,
>>         .close = hidp_close,
>> +       .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	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] Bluetooth: hidp: implement hidinput_input_event callback
  2013-07-11 14:10     ` Benjamin Tissoires
@ 2013-07-11 16:07       ` David Herrmann
  0 siblings, 0 replies; 15+ messages in thread
From: David Herrmann @ 2013-07-11 16:07 UTC (permalink / raw)
  To: Benjamin Tissoires
  Cc: Benjamin Tissoires, Marcel Holtmann, Gustavo Padovan,
	Jiri Kosina, linux-bluetooth, open list:HID CORE LAYER,
	linux-kernel

Hi

On Thu, Jul 11, 2013 at 4:10 PM, Benjamin Tissoires
<benjamin.tissoires@gmail.com> wrote:
> Hi David,
>
> On Thu, Jul 11, 2013 at 4:02 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
>> Hi
>>
>>> +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);
>>
>> I dislike that we have to deal with input_get_drvdata() in a transport
>> driver. It is not obvious from the transport driver that this gives us
>> an hid_device. But ok, that's how we do it in usbhid and hid-input so
>> it's another issue..
>
> right. So we should definitively change the callback signature here.
>
>>
>>> +       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);
>>> +}
>>> +
>>
>> We had this discussion before (regarding uhid and hidpinput_event), it
>> would be nice to have a helper in hid-input.c which does this. We copy
>> the code into every transport driver, which bugs me. But no-one
>> stepped up to do this, so I am fine.
>
> oh, yes, you are right. I'll add it to my todo list (now that I am
> responsible of 2/4 code duplication :-S )

Feel free to push this series forward unchanged, if it is urgent. I
have an experimental series here which implements a generic
hidinput_input_event() handler (including the scheduled worker) that I
will send later today or tomorrow.

Cheers
David

>>
>> Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
>
> Thanks for the review!
>
> Cheers,
> Benjamin
>
>>
>> Thanks!
>> David
>>
>>>  static int hidp_get_raw_report(struct hid_device *hid,
>>>                 unsigned char report_number,
>>>                 unsigned char *data, size_t count,
>>> @@ -711,6 +736,7 @@ static struct hid_ll_driver hidp_hid_driver = {
>>>         .stop = hidp_stop,
>>>         .open  = hidp_open,
>>>         .close = hidp_close,
>>> +       .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	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] Bluetooth: hidp: implement hidinput_input_event callback
@ 2013-07-11 22:50     ` Jiri Kosina
  0 siblings, 0 replies; 15+ messages in thread
From: Jiri Kosina @ 2013-07-11 22:50 UTC (permalink / raw)
  To: Benjamin Tissoires
  Cc: Benjamin Tissoires, Marcel Holtmann, Gustavo Padovan,
	David Herrmann, linux-bluetooth, linux-input, linux-kernel

On Thu, 11 Jul 2013, Benjamin Tissoires wrote:

> We can re-enable hidinput_input_event to allow the leds of bluetooth
> keyboards to be set.
> Now the callbacks uses hid core to retrieve the right HID report to
> send, so this version is safer.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Acked-by: Jiri Kosina <jkosina@suse.cz>

> ---
>  net/bluetooth/hidp/core.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
> index f13a8da..9c8b50d 100644
> --- a/net/bluetooth/hidp/core.c
> +++ b/net/bluetooth/hidp/core.c
> @@ -238,6 +238,31 @@ static int hidp_send_report(struct hidp_session *session, struct hid_report *rep
>  	return hidp_send_intr_message(session, hdr, buf, rsize);
>  }
>  
> +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,
> @@ -711,6 +736,7 @@ static struct hid_ll_driver hidp_hid_driver = {
>  	.stop = hidp_stop,
>  	.open  = hidp_open,
>  	.close = hidp_close,
> +	.hidinput_input_event = hidp_hidinput_event,
>  };
>  
>  /* This function sets up the hid device. It does not add it
> -- 
> 1.8.3.1
> 

-- 
Jiri Kosina
SUSE Labs

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] Bluetooth: hidp: implement hidinput_input_event callback
@ 2013-07-11 22:50     ` Jiri Kosina
  0 siblings, 0 replies; 15+ messages in thread
From: Jiri Kosina @ 2013-07-11 22:50 UTC (permalink / raw)
  To: Benjamin Tissoires
  Cc: Benjamin Tissoires, Marcel Holtmann, Gustavo Padovan,
	David Herrmann, linux-bluetooth-u79uwXL29TY76Z2rM5mHXA,
	linux-input-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA

On Thu, 11 Jul 2013, Benjamin Tissoires wrote:

> We can re-enable hidinput_input_event to allow the leds of bluetooth
> keyboards to be set.
> Now the callbacks uses hid core to retrieve the right HID report to
> send, so this version is safer.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Acked-by: Jiri Kosina <jkosina-AlSwsSmVLrQ@public.gmane.org>

> ---
>  net/bluetooth/hidp/core.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
> index f13a8da..9c8b50d 100644
> --- a/net/bluetooth/hidp/core.c
> +++ b/net/bluetooth/hidp/core.c
> @@ -238,6 +238,31 @@ static int hidp_send_report(struct hidp_session *session, struct hid_report *rep
>  	return hidp_send_intr_message(session, hdr, buf, rsize);
>  }
>  
> +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,
> @@ -711,6 +736,7 @@ static struct hid_ll_driver hidp_hid_driver = {
>  	.stop = hidp_stop,
>  	.open  = hidp_open,
>  	.close = hidp_close,
> +	.hidinput_input_event = hidp_hidinput_event,
>  };
>  
>  /* This function sets up the hid device. It does not add it
> -- 
> 1.8.3.1
> 

-- 
Jiri Kosina
SUSE Labs

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/2] Bluetooth: hidp: remove wrong send_report at init
  2013-07-11 13:41 ` [PATCH 2/2] Bluetooth: hidp: remove wrong send_report at init Benjamin Tissoires
  2013-07-11 13:51   ` David Herrmann
@ 2013-07-11 22:51   ` Jiri Kosina
  2013-07-12 10:09     ` Gustavo Padovan
  2 siblings, 0 replies; 15+ messages in thread
From: Jiri Kosina @ 2013-07-11 22:51 UTC (permalink / raw)
  To: Benjamin Tissoires
  Cc: Benjamin Tissoires, Marcel Holtmann, Gustavo Padovan,
	David Herrmann, linux-bluetooth, linux-input, linux-kernel

On Thu, 11 Jul 2013, Benjamin Tissoires wrote:

> The USB hid implementation does retrieve the reports during the start.
> However, this implementation does not call the HID command GET_REPORT
> (which would fetch the current status of each report), but use the
> DATA command, which is an Output Report (so transmitting data from the
> host to the device).
> The Wiimote controller is already guarded against this problem in the
> protocol, but it is not conformant to the specification to set all the
> reports to 0 on start.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Acked-by: Jiri Kosina <jkosina@suse.cz>

Gustavo, could you please take it through your tree?

Thanks Benjamin, thanks David.

> ---
>  net/bluetooth/hidp/core.c | 14 --------------
>  1 file changed, 14 deletions(-)
> 
> diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
> index 9c8b50d..59d132a 100644
> --- a/net/bluetooth/hidp/core.c
> +++ b/net/bluetooth/hidp/core.c
> @@ -703,20 +703,6 @@ static int hidp_parse(struct hid_device *hid)
>  
>  static int hidp_start(struct hid_device *hid)
>  {
> -	struct hidp_session *session = hid->driver_data;
> -	struct hid_report *report;
> -
> -	if (hid->quirks & HID_QUIRK_NO_INIT_REPORTS)
> -		return 0;
> -
> -	list_for_each_entry(report, &hid->report_enum[HID_INPUT_REPORT].
> -			report_list, list)
> -		hidp_send_report(session, report);
> -
> -	list_for_each_entry(report, &hid->report_enum[HID_FEATURE_REPORT].
> -			report_list, list)
> -		hidp_send_report(session, report);
> -
>  	return 0;
>  }
>  
> -- 
> 1.8.3.1
> 

-- 
Jiri Kosina
SUSE Labs

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/2] Bluetooth: hidp: remove wrong send_report at init
@ 2013-07-12 10:09     ` Gustavo Padovan
  0 siblings, 0 replies; 15+ messages in thread
From: Gustavo Padovan @ 2013-07-12 10:09 UTC (permalink / raw)
  To: Benjamin Tissoires
  Cc: Benjamin Tissoires, Marcel Holtmann, David Herrmann, Jiri Kosina,
	linux-bluetooth, linux-input, linux-kernel

Hi Benjamin,

* Benjamin Tissoires <benjamin.tissoires@redhat.com> [2013-07-11 15:41:30 +0200]:

> The USB hid implementation does retrieve the reports during the start.
> However, this implementation does not call the HID command GET_REPORT
> (which would fetch the current status of each report), but use the
> DATA command, which is an Output Report (so transmitting data from the
> host to the device).
> The Wiimote controller is already guarded against this problem in the
> protocol, but it is not conformant to the specification to set all the
> reports to 0 on start.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
>  net/bluetooth/hidp/core.c | 14 --------------
>  1 file changed, 14 deletions(-)

Both patches have been applied to bluetooth-next. Thanks.

	Gustavo

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/2] Bluetooth: hidp: remove wrong send_report at init
@ 2013-07-12 10:09     ` Gustavo Padovan
  0 siblings, 0 replies; 15+ messages in thread
From: Gustavo Padovan @ 2013-07-12 10:09 UTC (permalink / raw)
  To: Benjamin Tissoires
  Cc: Benjamin Tissoires, Marcel Holtmann, David Herrmann, Jiri Kosina,
	linux-bluetooth-u79uwXL29TY76Z2rM5mHXA,
	linux-input-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA

Hi Benjamin,

* Benjamin Tissoires <benjamin.tissoires-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> [2013-07-11 15:41:30 +0200]:

> The USB hid implementation does retrieve the reports during the start.
> However, this implementation does not call the HID command GET_REPORT
> (which would fetch the current status of each report), but use the
> DATA command, which is an Output Report (so transmitting data from the
> host to the device).
> The Wiimote controller is already guarded against this problem in the
> protocol, but it is not conformant to the specification to set all the
> reports to 0 on start.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  net/bluetooth/hidp/core.c | 14 --------------
>  1 file changed, 14 deletions(-)

Both patches have been applied to bluetooth-next. Thanks.

	Gustavo

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/2] Bluetooth: hidp: remove wrong send_report at init
  2013-07-12 10:09     ` Gustavo Padovan
@ 2013-07-12 12:55       ` Benjamin Tissoires
  -1 siblings, 0 replies; 15+ messages in thread
From: Benjamin Tissoires @ 2013-07-12 12:55 UTC (permalink / raw)
  To: Gustavo Padovan, linux-bluetooth, linux-input, linux-kernel

On 07/12/2013 12:09 PM, Gustavo Padovan wrote:
> Hi Benjamin,
> 
> * Benjamin Tissoires <benjamin.tissoires@redhat.com> [2013-07-11 15:41:30 +0200]:
> 
>> The USB hid implementation does retrieve the reports during the start.
>> However, this implementation does not call the HID command GET_REPORT
>> (which would fetch the current status of each report), but use the
>> DATA command, which is an Output Report (so transmitting data from the
>> host to the device).
>> The Wiimote controller is already guarded against this problem in the
>> protocol, but it is not conformant to the specification to set all the
>> reports to 0 on start.
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>> ---
>>  net/bluetooth/hidp/core.c | 14 --------------
>>  1 file changed, 14 deletions(-)
> 
> Both patches have been applied to bluetooth-next. Thanks.
> 
> 	Gustavo
> 

Thanks Gustavo!

Cheers,
Benjamin

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/2] Bluetooth: hidp: remove wrong send_report at init
@ 2013-07-12 12:55       ` Benjamin Tissoires
  0 siblings, 0 replies; 15+ messages in thread
From: Benjamin Tissoires @ 2013-07-12 12:55 UTC (permalink / raw)
  To: Gustavo Padovan, linux-bluetooth-u79uwXL29TY76Z2rM5mHXA,
	linux-input-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA

On 07/12/2013 12:09 PM, Gustavo Padovan wrote:
> Hi Benjamin,
> 
> * Benjamin Tissoires <benjamin.tissoires-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> [2013-07-11 15:41:30 +0200]:
> 
>> The USB hid implementation does retrieve the reports during the start.
>> However, this implementation does not call the HID command GET_REPORT
>> (which would fetch the current status of each report), but use the
>> DATA command, which is an Output Report (so transmitting data from the
>> host to the device).
>> The Wiimote controller is already guarded against this problem in the
>> protocol, but it is not conformant to the specification to set all the
>> reports to 0 on start.
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> ---
>>  net/bluetooth/hidp/core.c | 14 --------------
>>  1 file changed, 14 deletions(-)
> 
> Both patches have been applied to bluetooth-next. Thanks.
> 
> 	Gustavo
> 

Thanks Gustavo!

Cheers,
Benjamin

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2013-07-12 12:55 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-07-11 13:41 [PATCH 0/2] Bluetooth: hidp: cleanup HID init and re-enable hidinput_input_event callback Benjamin Tissoires
2013-07-11 13:41 ` [PATCH 1/2] Bluetooth: hidp: implement " Benjamin Tissoires
2013-07-11 13:41   ` Benjamin Tissoires
2013-07-11 14:02   ` David Herrmann
2013-07-11 14:10     ` Benjamin Tissoires
2013-07-11 16:07       ` David Herrmann
2013-07-11 22:50   ` Jiri Kosina
2013-07-11 22:50     ` Jiri Kosina
2013-07-11 13:41 ` [PATCH 2/2] Bluetooth: hidp: remove wrong send_report at init Benjamin Tissoires
2013-07-11 13:51   ` David Herrmann
2013-07-11 22:51   ` Jiri Kosina
2013-07-12 10:09   ` Gustavo Padovan
2013-07-12 10:09     ` Gustavo Padovan
2013-07-12 12:55     ` Benjamin Tissoires
2013-07-12 12:55       ` Benjamin Tissoires

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.