[RFC 0/2] HID: Introduce .idle() to remove last usb dependence from hid-multitouch

[RFC 0/2] HID: Introduce .idle() to remove last usb dependence from hid-multitouch

[Benjamin Tissoires]

Hi guys,

Well, this series of two patches aims at removing the dependence between usb
and hid-multitouch.
I put a RFC and not a patch as I'm not satisfied with the result:
- adding this callback for one use may not be accurate
- the parameters may not be good either (HID_REQ_SET_IDLE is the only valid
  value for now as noone seems to be using HID_REQ_GET_IDLE)
- the return value is clearly not very interesting.
- maybe we should just add HID_REQ_SET_IDLE to .request, but this will require
  a few changes in the parameters.

The main purpose of this RFC was to share it with David this new ll callback, so
that we can have a global view of what is needed.

Thanks for the inputs.

Cheers,
Benjamin

Benjamin Tissoires (2):
  HID: Extend the interface with idle requests
  HID: multitouch: remove last usb dependency

 drivers/hid/hid-multitouch.c  | 17 +----------------
 drivers/hid/usbhid/hid-core.c | 15 +++++++++++++++
 include/linux/hid.h           | 19 +++++++++++++++++++
 3 files changed, 35 insertions(+), 16 deletions(-)

-- 
1.8.1.2

[RFC 1/2] HID: Extend the interface with idle requests

[Benjamin Tissoires]

Some drivers send the idle command directly to underlying device,
creating an unwanted dependency on the underlying transport layer.
This patch adds hid_hw_idle() to the interface, thereby removing
usbhid from the lion share of the drivers.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/usbhid/hid-core.c | 15 +++++++++++++++
 include/linux/hid.h           | 19 +++++++++++++++++++
 2 files changed, 34 insertions(+)

diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 420466b..effcd3d 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -1253,6 +1253,20 @@ static void usbhid_request(struct hid_device *hid, struct hid_report *rep, int r
 	}
 }
 
+static int usbhid_idle(struct hid_device *hid, int report, int idle,
+		int reqtype)
+{
+	struct usb_device *dev = hid_to_usb_dev(hid);
+	struct usb_interface *intf = to_usb_interface(hid->dev.parent);
+	struct usb_host_interface *interface = intf->cur_altsetting;
+	int ifnum = interface->desc.bInterfaceNumber;
+
+	if (reqtype != HID_REQ_SET_IDLE)
+		return -EINVAL;
+
+	return hid_set_idle(dev, ifnum, report, idle);
+}
+
 static struct hid_ll_driver usb_hid_driver = {
 	.parse = usbhid_parse,
 	.start = usbhid_start,
@@ -1263,6 +1277,7 @@ static struct hid_ll_driver usb_hid_driver = {
 	.hidinput_input_event = usb_hidinput_input_event,
 	.request = usbhid_request,
 	.wait = usbhid_wait_io,
+	.idle = usbhid_idle,
 };
 
 static int usbhid_probe(struct usb_interface *intf, const struct usb_device_id *id)
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 7071eb3..863744c 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -664,6 +664,7 @@ 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)
+ * @idle: send idle request to device
  */
 struct hid_ll_driver {
 	int (*start)(struct hid_device *hdev);
@@ -683,6 +684,7 @@ struct hid_ll_driver {
 			struct hid_report *report, int reqtype);
 
 	int (*wait)(struct hid_device *hdev);
+	int (*idle)(struct hid_device *hdev, int report, int idle, int reqtype);
 
 };
 
@@ -907,6 +909,23 @@ static inline void hid_hw_request(struct hid_device *hdev,
 }
 
 /**
+ * hid_hw_idle - send idle request to device
+ *
+ * @hdev: hid device
+ * @report: report to control
+ * @idle: idle state
+ * @reqtype: hid request type
+ */
+static inline int hid_hw_idle(struct hid_device *hdev, int report, int idle,
+		int reqtype)
+{
+	if (hdev->ll_driver->idle)
+		return hdev->ll_driver->idle(hdev, report, idle, reqtype);
+
+	return 0;
+}
+
+/**
  * hid_hw_wait - wait for buffered io to complete
  *
  * @hdev: hid device
-- 
1.8.1.2

Re: [RFC 1/2] HID: Extend the interface with idle requests

[Jiri Kosina]

On Wed, 27 Feb 2013, Benjamin Tissoires wrote:

> Some drivers send the idle command directly to underlying device,
> creating an unwanted dependency on the underlying transport layer.
> This patch adds hid_hw_idle() to the interface, thereby removing
> usbhid from the lion share of the drivers.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
>  drivers/hid/usbhid/hid-core.c | 15 +++++++++++++++
>  include/linux/hid.h           | 19 +++++++++++++++++++
>  2 files changed, 34 insertions(+)
> 
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index 420466b..effcd3d 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -1253,6 +1253,20 @@ static void usbhid_request(struct hid_device *hid, struct hid_report *rep, int r
>  	}
>  }
>  
> +static int usbhid_idle(struct hid_device *hid, int report, int idle,
> +		int reqtype)

Where does the need for passing the report argument come from please?

-- 
Jiri Kosina
SUSE Labs

Re: [RFC 1/2] HID: Extend the interface with idle requests

[Benjamin Tissoires]

On Thu, Mar 7, 2013 at 3:26 PM, Jiri Kosina <jkosina@suse.cz> wrote:
> On Wed, 27 Feb 2013, Benjamin Tissoires wrote:
>
>> Some drivers send the idle command directly to underlying device,
>> creating an unwanted dependency on the underlying transport layer.
>> This patch adds hid_hw_idle() to the interface, thereby removing
>> usbhid from the lion share of the drivers.
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>> ---
>>  drivers/hid/usbhid/hid-core.c | 15 +++++++++++++++
>>  include/linux/hid.h           | 19 +++++++++++++++++++
>>  2 files changed, 34 insertions(+)
>>
>> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
>> index 420466b..effcd3d 100644
>> --- a/drivers/hid/usbhid/hid-core.c
>> +++ b/drivers/hid/usbhid/hid-core.c
>> @@ -1253,6 +1253,20 @@ static void usbhid_request(struct hid_device *hid, struct hid_report *rep, int r
>>       }
>>  }
>>
>> +static int usbhid_idle(struct hid_device *hid, int report, int idle,
>> +             int reqtype)
>
> Where does the need for passing the report argument come from please?

Well, I haven't checked in the USB spec, but hid_set_idle() in
usbhid/hid-core.c does require the reportID, so I add it to the
request.
I just gave a quick look at the HID/I2C spec, and it also requires the
report ID. The set_idle request is report specific.

Benjamin

>
> --
> Jiri Kosina
> SUSE Labs

Re: [RFC 1/2] HID: Extend the interface with idle requests

[Jiri Kosina]

On Thu, 7 Mar 2013, Benjamin Tissoires wrote:

> >> +static int usbhid_idle(struct hid_device *hid, int report, int idle,
> >> +             int reqtype)
> >
> > Where does the need for passing the report argument come from please?
> 
> Well, I haven't checked in the USB spec, but hid_set_idle() in
> usbhid/hid-core.c does require the reportID, so I add it to the
> request.
> I just gave a quick look at the HID/I2C spec, and it also requires the
> report ID. The set_idle request is report specific.

You are right, I have missed the special meaning of lower byte of wValue.

Okay, I think it makes sense. I will be taking the patches.

Thanks Benjamin.

-- 
Jiri Kosina
SUSE Labs

[RFC 2/2] HID: multitouch: remove last usb dependency

[Benjamin Tissoires]

hid-multitouch used to direclty call for a set_idle in the .resume()
callback. With the new hid_hw_idle(), this can be dropped and
hid-multitouch is now freed from its transport layer.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-multitouch.c | 17 +----------------
 1 file changed, 1 insertion(+), 16 deletions(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 51d8938..1f544a4 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -896,26 +896,11 @@ static int mt_reset_resume(struct hid_device *hdev)
 
 static int mt_resume(struct hid_device *hdev)
 {
-	struct usb_interface *intf;
-	struct usb_host_interface *interface;
-	struct usb_device *dev;
-
-	if (hdev->bus != BUS_USB)
-		return 0;
-
-	intf = to_usb_interface(hdev->dev.parent);
-	interface = intf->cur_altsetting;
-	dev = interface_to_usbdev(intf);
-
 	/* Some Elan legacy devices require SET_IDLE to be set on resume.
 	 * It should be safe to send it to other devices too.
 	 * Tested on 3M, Stantum, Cypress, Zytronic, eGalax, and Elan panels. */
 
-	usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
-			HID_REQ_SET_IDLE,
-			USB_TYPE_CLASS | USB_RECIP_INTERFACE,
-			0, interface->desc.bInterfaceNumber,
-			NULL, 0, USB_CTRL_SET_TIMEOUT);
+	hid_hw_idle(hdev, 0, 0, HID_REQ_SET_IDLE);
 
 	return 0;
 }
-- 
1.8.1.2

Re: [RFC 0/2] HID: Introduce .idle() to remove last usb dependence from hid-multitouch

[David Herrmann]

Hi Benjamin

On Wed, Feb 27, 2013 at 4:38 PM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> Hi guys,
>
> Well, this series of two patches aims at removing the dependence between usb
> and hid-multitouch.
> I put a RFC and not a patch as I'm not satisfied with the result:
> - adding this callback for one use may not be accurate
> - the parameters may not be good either (HID_REQ_SET_IDLE is the only valid
>   value for now as noone seems to be using HID_REQ_GET_IDLE)
> - the return value is clearly not very interesting.
> - maybe we should just add HID_REQ_SET_IDLE to .request, but this will require
>   a few changes in the parameters.
>
> The main purpose of this RFC was to share it with David this new ll callback, so
> that we can have a global view of what is needed.

I actually never read the specs for SET/GET_IDLE. The Bluetooth HID
Profile deprecated these and I haven't seen a Bluetooth device that
relies on it. Any reasons why I should implement them?

Also, for things like UHID I think we can safely drop this request.
Considering that, I think the patches can be applied as they are, so:
  Reviewed-by: David Herrmann <dh.herrmann@gmail.com>

The Bluetooth HID Profile actually implements some other SUSPEND
calls,  but these are supposed to be handled by the Bluetooth stack
independently of the HID driver. So if there is no other magic going
on with SET_IDLE, I'd just leave it for usbhid.

(does anyone know whether i2c-hid support these?)

Thanks
David

Re: [RFC 0/2] HID: Introduce .idle() to remove last usb dependence from hid-multitouch

[Benjamin Tissoires]

On Wed, Feb 27, 2013 at 6:48 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi Benjamin
>
> On Wed, Feb 27, 2013 at 4:38 PM, Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
>> Hi guys,
>>
>> Well, this series of two patches aims at removing the dependence between usb
>> and hid-multitouch.
>> I put a RFC and not a patch as I'm not satisfied with the result:
>> - adding this callback for one use may not be accurate
>> - the parameters may not be good either (HID_REQ_SET_IDLE is the only valid
>>   value for now as noone seems to be using HID_REQ_GET_IDLE)
>> - the return value is clearly not very interesting.
>> - maybe we should just add HID_REQ_SET_IDLE to .request, but this will require
>>   a few changes in the parameters.
>>
>> The main purpose of this RFC was to share it with David this new ll callback, so
>> that we can have a global view of what is needed.
>
> I actually never read the specs for SET/GET_IDLE. The Bluetooth HID
> Profile deprecated these and I haven't seen a Bluetooth device that
> relies on it. Any reasons why I should implement them?

I don't want you to absolutely implement it. I just wanted to share it
with you because all the commands set/get report, set/get idle,
set/get protocol and set power are part of the "class specific
requests".
So I don't know if we should have distinct hid_hw_* calls for each of
them, or if we should implement a big hid_hw_request() function.

>
> Also, for things like UHID I think we can safely drop this request.

sure.

> Considering that, I think the patches can be applied as they are, so:
>   Reviewed-by: David Herrmann <dh.herrmann@gmail.com>

Thanks.

>
> The Bluetooth HID Profile actually implements some other SUSPEND
> calls,  but these are supposed to be handled by the Bluetooth stack
> independently of the HID driver. So if there is no other magic going
> on with SET_IDLE, I'd just leave it for usbhid.
>
> (does anyone know whether i2c-hid support these?)

It does, but it's also marked as deprecated. :) So I don't want to
implement it either until we found weird devices requesting it.

Cheers,
Benjamin