[PATCH v1 0/2] Core driver enhancements

[PATCH v1 0/2] Core driver enhancements

[Srinivas Pandruvada]

Enhance core to support next generation of hub devices for fast response
and application collection.

v1
Added acked by tag for parallel read patch
For collection support, changed the commit message and relying on FW to explicitlty
define a collection, then only create a collection and allow callback registration

v0
Base version

Srinivas Pandruvada (2):
  HID: hid-sensor-hub: Allow parallel synchronous reads
  HID: hid-sensor-hub: Add support for application collection

 drivers/hid/hid-sensor-hub.c   | 120 +++++++++++++++++++++++------------------
 include/linux/hid-sensor-hub.h |  24 +++++++++
 include/linux/hid-sensor-ids.h |   2 +
 3 files changed, 93 insertions(+), 53 deletions(-)

-- 
1.9.1

[PATCH v1 1/2] HID: hid-sensor-hub: Allow parallel synchronous reads

[Srinivas Pandruvada]

Current implementation only allows one outstanding synchronous read.
This is a performance hit when user mode is requesting raw reads
of sensor attributes on multiple sensors together.
This change changes the mutex lock to per hid sensor hub device instead
of global lock. Although request to hid sensor hub is serialized, there
can be multiple outstanding read requests pending for responses via
hid reports.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Acked-by: Jonathan Cameron <jic23@kernel.org>
---
 drivers/hid/hid-sensor-hub.c   | 90 +++++++++++++++++++-----------------------
 include/linux/hid-sensor-hub.h | 24 +++++++++++
 2 files changed, 65 insertions(+), 49 deletions(-)

diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
index e54ce10..865cd56 100644
--- a/drivers/hid/hid-sensor-hub.c
+++ b/drivers/hid/hid-sensor-hub.c
@@ -29,29 +29,10 @@
 #define HID_SENSOR_HUB_ENUM_QUIRK	0x01
 
 /**
- * struct sensor_hub_pending - Synchronous read pending information
- * @status:		Pending status true/false.
- * @ready:		Completion synchronization data.
- * @usage_id:		Usage id for physical device, E.g. Gyro usage id.
- * @attr_usage_id:	Usage Id of a field, E.g. X-AXIS for a gyro.
- * @raw_size:		Response size for a read request.
- * @raw_data:		Place holder for received response.
- */
-struct sensor_hub_pending {
-	bool status;
-	struct completion ready;
-	u32 usage_id;
-	u32 attr_usage_id;
-	int raw_size;
-	u8  *raw_data;
-};
-
-/**
  * struct sensor_hub_data - Hold a instance data for a HID hub device
  * @hsdev:		Stored hid instance for current hub device.
  * @mutex:		Mutex to serialize synchronous request.
  * @lock:		Spin lock to protect pending request structure.
- * @pending:		Holds information of pending sync read request.
  * @dyn_callback_list:	Holds callback function
  * @dyn_callback_lock:	spin lock to protect callback list
  * @hid_sensor_hub_client_devs:	Stores all MFD cells for a hub instance.
@@ -61,7 +42,6 @@ struct sensor_hub_pending {
 struct sensor_hub_data {
 	struct mutex mutex;
 	spinlock_t lock;
-	struct sensor_hub_pending pending;
 	struct list_head dyn_callback_list;
 	spinlock_t dyn_callback_lock;
 	struct mfd_cell *hid_sensor_hub_client_devs;
@@ -266,40 +246,42 @@ int sensor_hub_input_attr_get_raw_value(struct hid_sensor_hub_device *hsdev,
 	struct hid_report *report;
 	int ret_val = 0;
 
-	mutex_lock(&data->mutex);
-	memset(&data->pending, 0, sizeof(data->pending));
-	init_completion(&data->pending.ready);
-	data->pending.usage_id = usage_id;
-	data->pending.attr_usage_id = attr_usage_id;
-	data->pending.raw_size = 0;
+	mutex_lock(&hsdev->mutex);
+	memset(&hsdev->pending, 0, sizeof(hsdev->pending));
+	init_completion(&hsdev->pending.ready);
+	hsdev->pending.usage_id = usage_id;
+	hsdev->pending.attr_usage_id = attr_usage_id;
+	hsdev->pending.raw_size = 0;
 
 	spin_lock_irqsave(&data->lock, flags);
-	data->pending.status = true;
+	hsdev->pending.status = true;
 	spin_unlock_irqrestore(&data->lock, flags);
 	report = sensor_hub_report(report_id, hsdev->hdev, HID_INPUT_REPORT);
 	if (!report)
 		goto err_free;
 
+	mutex_lock(&data->mutex);
 	hid_hw_request(hsdev->hdev, report, HID_REQ_GET_REPORT);
-	wait_for_completion_interruptible_timeout(&data->pending.ready, HZ*5);
-	switch (data->pending.raw_size) {
+	mutex_unlock(&data->mutex);
+	wait_for_completion_interruptible_timeout(&hsdev->pending.ready, HZ*5);
+	switch (hsdev->pending.raw_size) {
 	case 1:
-		ret_val = *(u8 *)data->pending.raw_data;
+		ret_val = *(u8 *)hsdev->pending.raw_data;
 		break;
 	case 2:
-		ret_val = *(u16 *)data->pending.raw_data;
+		ret_val = *(u16 *)hsdev->pending.raw_data;
 		break;
 	case 4:
-		ret_val = *(u32 *)data->pending.raw_data;
+		ret_val = *(u32 *)hsdev->pending.raw_data;
 		break;
 	default:
 		ret_val = 0;
 	}
-	kfree(data->pending.raw_data);
+	kfree(hsdev->pending.raw_data);
 
 err_free:
-	data->pending.status = false;
-	mutex_unlock(&data->mutex);
+	hsdev->pending.status = false;
+	mutex_unlock(&hsdev->mutex);
 
 	return ret_val;
 }
@@ -455,16 +437,6 @@ static int sensor_hub_raw_event(struct hid_device *hdev,
 					report->field[i]->report_count)/8);
 		sz = (report->field[i]->report_size *
 					report->field[i]->report_count)/8;
-		if (pdata->pending.status && pdata->pending.attr_usage_id ==
-				report->field[i]->usage->hid) {
-			hid_dbg(hdev, "data was pending ...\n");
-			pdata->pending.raw_data = kmemdup(ptr, sz, GFP_ATOMIC);
-			if (pdata->pending.raw_data)
-				pdata->pending.raw_size = sz;
-			else
-				pdata->pending.raw_size = 0;
-			complete(&pdata->pending.ready);
-		}
 		collection = &hdev->collection[
 				report->field[i]->usage->collection_index];
 		hid_dbg(hdev, "collection->usage %x\n",
@@ -474,8 +446,21 @@ static int sensor_hub_raw_event(struct hid_device *hdev,
 				report->field[i]->physical,
 				report->field[i]->usage[0].collection_index,
 				&hsdev, &priv);
-
-		if (callback && callback->capture_sample) {
+		if (!callback) {
+			ptr += sz;
+			continue;
+		}
+		if (hsdev->pending.status && hsdev->pending.attr_usage_id ==
+				report->field[i]->usage->hid) {
+			hid_dbg(hdev, "data was pending ...\n");
+			hsdev->pending.raw_data = kmemdup(ptr, sz, GFP_ATOMIC);
+			if (hsdev->pending.raw_data)
+				hsdev->pending.raw_size = sz;
+			else
+				hsdev->pending.raw_size = 0;
+			complete(&hsdev->pending.ready);
+		}
+		if (callback->capture_sample) {
 			if (report->field[i]->logical)
 				callback->capture_sample(hsdev,
 					report->field[i]->logical, sz, ptr,
@@ -630,6 +615,8 @@ static int sensor_hub_probe(struct hid_device *hdev,
 			hsdev->hdev = hdev;
 			hsdev->vendor_id = hdev->vendor;
 			hsdev->product_id = hdev->product;
+			hsdev->usage = collection->usage;
+			mutex_init(&hsdev->mutex);
 			hsdev->start_collection_index = i;
 			if (last_hsdev)
 				last_hsdev->end_collection_index = i;
@@ -676,13 +663,18 @@ static void sensor_hub_remove(struct hid_device *hdev)
 {
 	struct sensor_hub_data *data = hid_get_drvdata(hdev);
 	unsigned long flags;
+	int i;
 
 	hid_dbg(hdev, " hardware removed\n");
 	hid_hw_close(hdev);
 	hid_hw_stop(hdev);
 	spin_lock_irqsave(&data->lock, flags);
-	if (data->pending.status)
-		complete(&data->pending.ready);
+	for (i = 0; i < data->hid_sensor_client_cnt; ++i) {
+		struct hid_sensor_hub_device *hsdev =
+			data->hid_sensor_hub_client_devs[i].platform_data;
+		if (hsdev->pending.status)
+			complete(&hsdev->pending.ready);
+	}
 	spin_unlock_irqrestore(&data->lock, flags);
 	mfd_remove_devices(&hdev->dev);
 	hid_set_drvdata(hdev, NULL);
diff --git a/include/linux/hid-sensor-hub.h b/include/linux/hid-sensor-hub.h
index 4173a8f..0b21c4f 100644
--- a/include/linux/hid-sensor-hub.h
+++ b/include/linux/hid-sensor-hub.h
@@ -49,19 +49,43 @@ struct hid_sensor_hub_attribute_info {
 };
 
 /**
+ * struct sensor_hub_pending - Synchronous read pending information
+ * @status:		Pending status true/false.
+ * @ready:		Completion synchronization data.
+ * @usage_id:		Usage id for physical device, E.g. Gyro usage id.
+ * @attr_usage_id:	Usage Id of a field, E.g. X-AXIS for a gyro.
+ * @raw_size:		Response size for a read request.
+ * @raw_data:		Place holder for received response.
+ */
+struct sensor_hub_pending {
+	bool status;
+	struct completion ready;
+	u32 usage_id;
+	u32 attr_usage_id;
+	int raw_size;
+	u8  *raw_data;
+};
+
+/**
  * struct hid_sensor_hub_device - Stores the hub instance data
  * @hdev:		Stores the hid instance.
  * @vendor_id:		Vendor id of hub device.
  * @product_id:		Product id of hub device.
+ * @usage:		Usage id for this hub device instance.
  * @start_collection_index: Starting index for a phy type collection
  * @end_collection_index: Last index for a phy type collection
+ * @mutex:		synchronizing mutex.
+ * @pending:		Holds information of pending sync read request.
  */
 struct hid_sensor_hub_device {
 	struct hid_device *hdev;
 	u32 vendor_id;
 	u32 product_id;
+	u32 usage;
 	int start_collection_index;
 	int end_collection_index;
+	struct mutex mutex;
+	struct sensor_hub_pending pending;
 };
 
 /**
-- 
1.9.1

[PATCH v1 2/2] HID: hid-sensor-hub: Add support for application collection

[Srinivas Pandruvada]

Section 4.2.5 of HID Sensor hub specification allows two methods
defining sensor devices.
- Each sensor device by its own collection
- A top level application collection object, including multiple
sensors.
In the first method, each sensor can be in its own sensor application
collection without a physical collection.
In the second method there is a usage id for collection type, which
is defined as an application collection, with multiple physical
collections in it. It is possible to define fusion sensor with this
and may have its own handler. If there is a callback registered
for the collection type, then forward all reports for sensors in
its collection to this handler.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/hid/hid-sensor-hub.c   | 30 ++++++++++++++++++++++++++----
 include/linux/hid-sensor-ids.h |  2 ++
 2 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
index 865cd56..b855a60 100644
--- a/drivers/hid/hid-sensor-hub.c
+++ b/drivers/hid/hid-sensor-hub.c
@@ -86,7 +86,8 @@ static int sensor_hub_get_physical_device_count(struct hid_device *hdev)
 
 	for (i = 0; i < hdev->maxcollection; ++i) {
 		struct hid_collection *collection = &hdev->collection[i];
-		if (collection->type == HID_COLLECTION_PHYSICAL)
+		if (collection->type == HID_COLLECTION_PHYSICAL ||
+		    collection->type == HID_COLLECTION_APPLICATION)
 			++count;
 	}
 
@@ -119,7 +120,8 @@ static struct hid_sensor_hub_callbacks *sensor_hub_get_callback(
 
 	spin_lock_irqsave(&pdata->dyn_callback_lock, flags);
 	list_for_each_entry(callback, &pdata->dyn_callback_list, list)
-		if (callback->usage_id == usage_id &&
+		if ((callback->usage_id == usage_id ||
+		     callback->usage_id == HID_USAGE_SENSOR_COLLECTION) &&
 			(collection_index >=
 				callback->hsdev->start_collection_index) &&
 			(collection_index <
@@ -159,7 +161,18 @@ int sensor_hub_register_callback(struct hid_sensor_hub_device *hsdev,
 	callback->usage_callback = usage_callback;
 	callback->usage_id = usage_id;
 	callback->priv = NULL;
-	list_add_tail(&callback->list, &pdata->dyn_callback_list);
+	/*
+	 * If there is a handler registered for the collection type, then
+	 * it will handle all reports for sensors in this collection. If
+	 * there is also an individual sensor handler registration, then
+	 * we want to make sure that the reports are directed to collection
+	 * handler, as this may be a fusion sensor. So add collection handlers
+	 * to the beginning of the list, so that they are matched first.
+	 */
+	if (usage_id == HID_USAGE_SENSOR_COLLECTION)
+		list_add(&callback->list, &pdata->dyn_callback_list);
+	else
+		list_add_tail(&callback->list, &pdata->dyn_callback_list);
 	spin_unlock_irqrestore(&pdata->dyn_callback_lock, flags);
 
 	return 0;
@@ -557,6 +570,7 @@ static int sensor_hub_probe(struct hid_device *hdev,
 	int dev_cnt;
 	struct hid_sensor_hub_device *hsdev;
 	struct hid_sensor_hub_device *last_hsdev = NULL;
+	struct hid_sensor_hub_device *collection_hsdev = NULL;
 
 	sd = devm_kzalloc(&hdev->dev, sizeof(*sd), GFP_KERNEL);
 	if (!sd) {
@@ -603,7 +617,8 @@ static int sensor_hub_probe(struct hid_device *hdev,
 	for (i = 0; i < hdev->maxcollection; ++i) {
 		struct hid_collection *collection = &hdev->collection[i];
 
-		if (collection->type == HID_COLLECTION_PHYSICAL) {
+		if (collection->type == HID_COLLECTION_PHYSICAL ||
+		    collection->type == HID_COLLECTION_APPLICATION) {
 
 			hsdev = devm_kzalloc(&hdev->dev, sizeof(*hsdev),
 					     GFP_KERNEL);
@@ -640,10 +655,17 @@ static int sensor_hub_probe(struct hid_device *hdev,
 			hid_dbg(hdev, "Adding %s:%d\n", name,
 					hsdev->start_collection_index);
 			sd->hid_sensor_client_cnt++;
+			if (collection_hsdev)
+				collection_hsdev->end_collection_index = i;
+			if (collection->type == HID_COLLECTION_APPLICATION &&
+			    collection->usage == HID_USAGE_SENSOR_COLLECTION)
+				collection_hsdev = hsdev;
 		}
 	}
 	if (last_hsdev)
 		last_hsdev->end_collection_index = i;
+	if (collection_hsdev)
+		collection_hsdev->end_collection_index = i;
 
 	ret = mfd_add_hotplug_devices(&hdev->dev,
 			sd->hid_sensor_hub_client_devs,
diff --git a/include/linux/hid-sensor-ids.h b/include/linux/hid-sensor-ids.h
index 109f0e6..f2ee90a 100644
--- a/include/linux/hid-sensor-ids.h
+++ b/include/linux/hid-sensor-ids.h
@@ -21,6 +21,8 @@
 
 #define HID_MAX_PHY_DEVICES					0xFF
 
+#define HID_USAGE_SENSOR_COLLECTION				0x200001
+
 /* Accel 3D (200073) */
 #define HID_USAGE_SENSOR_ACCEL_3D				0x200073
 #define HID_USAGE_SENSOR_DATA_ACCELERATION			0x200452
-- 
1.9.1

Re: [PATCH v1 0/2] Core driver enhancements

[Jiri Kosina]

On Thu, 19 Feb 2015, Srinivas Pandruvada wrote:

> Enhance core to support next generation of hub devices for fast response
> and application collection.
> 
> v1
> Added acked by tag for parallel read patch
> For collection support, changed the commit message and relying on FW to explicitlty
> define a collection, then only create a collection and allow callback registration
> 
> v0
> Base version
> 
> Srinivas Pandruvada (2):
>   HID: hid-sensor-hub: Allow parallel synchronous reads
>   HID: hid-sensor-hub: Add support for application collection
> 
>  drivers/hid/hid-sensor-hub.c   | 120 +++++++++++++++++++++++------------------
>  include/linux/hid-sensor-hub.h |  24 +++++++++
>  include/linux/hid-sensor-ids.h |   2 +
>  3 files changed, 93 insertions(+), 53 deletions(-)

Applied, thanks.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v1 2/2] HID: hid-sensor-hub: Add support for application collection

[Jonathan Cameron]

On 19/02/15 23:31, Srinivas Pandruvada wrote:
> Section 4.2.5 of HID Sensor hub specification allows two methods
> defining sensor devices.
> - Each sensor device by its own collection
> - A top level application collection object, including multiple
> sensors.
> In the first method, each sensor can be in its own sensor application
> collection without a physical collection.
> In the second method there is a usage id for collection type, which
> is defined as an application collection, with multiple physical
> collections in it. It is possible to define fusion sensor with this
> and may have its own handler. If there is a callback registered
> for the collection type, then forward all reports for sensors in
> its collection to this handler.
> 
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Whilst superficially seems fine to me, this is somewhat out of my
area so I'd rather not give a formal ack / reviewed by.

> ---
>  drivers/hid/hid-sensor-hub.c   | 30 ++++++++++++++++++++++++++----
>  include/linux/hid-sensor-ids.h |  2 ++
>  2 files changed, 28 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
> index 865cd56..b855a60 100644
> --- a/drivers/hid/hid-sensor-hub.c
> +++ b/drivers/hid/hid-sensor-hub.c
> @@ -86,7 +86,8 @@ static int sensor_hub_get_physical_device_count(struct hid_device *hdev)
>  
>  	for (i = 0; i < hdev->maxcollection; ++i) {
>  		struct hid_collection *collection = &hdev->collection[i];
> -		if (collection->type == HID_COLLECTION_PHYSICAL)
> +		if (collection->type == HID_COLLECTION_PHYSICAL ||
> +		    collection->type == HID_COLLECTION_APPLICATION)
>  			++count;
>  	}
>  
> @@ -119,7 +120,8 @@ static struct hid_sensor_hub_callbacks *sensor_hub_get_callback(
>  
>  	spin_lock_irqsave(&pdata->dyn_callback_lock, flags);
>  	list_for_each_entry(callback, &pdata->dyn_callback_list, list)
> -		if (callback->usage_id == usage_id &&
> +		if ((callback->usage_id == usage_id ||
> +		     callback->usage_id == HID_USAGE_SENSOR_COLLECTION) &&
>  			(collection_index >=
>  				callback->hsdev->start_collection_index) &&
>  			(collection_index <
> @@ -159,7 +161,18 @@ int sensor_hub_register_callback(struct hid_sensor_hub_device *hsdev,
>  	callback->usage_callback = usage_callback;
>  	callback->usage_id = usage_id;
>  	callback->priv = NULL;
> -	list_add_tail(&callback->list, &pdata->dyn_callback_list);
> +	/*
> +	 * If there is a handler registered for the collection type, then
> +	 * it will handle all reports for sensors in this collection. If
> +	 * there is also an individual sensor handler registration, then
> +	 * we want to make sure that the reports are directed to collection
> +	 * handler, as this may be a fusion sensor. So add collection handlers
> +	 * to the beginning of the list, so that they are matched first.
> +	 */
> +	if (usage_id == HID_USAGE_SENSOR_COLLECTION)
> +		list_add(&callback->list, &pdata->dyn_callback_list);
> +	else
> +		list_add_tail(&callback->list, &pdata->dyn_callback_list);
>  	spin_unlock_irqrestore(&pdata->dyn_callback_lock, flags);
>  
>  	return 0;
> @@ -557,6 +570,7 @@ static int sensor_hub_probe(struct hid_device *hdev,
>  	int dev_cnt;
>  	struct hid_sensor_hub_device *hsdev;
>  	struct hid_sensor_hub_device *last_hsdev = NULL;
> +	struct hid_sensor_hub_device *collection_hsdev = NULL;
>  
>  	sd = devm_kzalloc(&hdev->dev, sizeof(*sd), GFP_KERNEL);
>  	if (!sd) {
> @@ -603,7 +617,8 @@ static int sensor_hub_probe(struct hid_device *hdev,
>  	for (i = 0; i < hdev->maxcollection; ++i) {
>  		struct hid_collection *collection = &hdev->collection[i];
>  
> -		if (collection->type == HID_COLLECTION_PHYSICAL) {
> +		if (collection->type == HID_COLLECTION_PHYSICAL ||
> +		    collection->type == HID_COLLECTION_APPLICATION) {
>  
>  			hsdev = devm_kzalloc(&hdev->dev, sizeof(*hsdev),
>  					     GFP_KERNEL);
> @@ -640,10 +655,17 @@ static int sensor_hub_probe(struct hid_device *hdev,
>  			hid_dbg(hdev, "Adding %s:%d\n", name,
>  					hsdev->start_collection_index);
>  			sd->hid_sensor_client_cnt++;
> +			if (collection_hsdev)
> +				collection_hsdev->end_collection_index = i;
> +			if (collection->type == HID_COLLECTION_APPLICATION &&
> +			    collection->usage == HID_USAGE_SENSOR_COLLECTION)
> +				collection_hsdev = hsdev;
>  		}
>  	}
>  	if (last_hsdev)
>  		last_hsdev->end_collection_index = i;
> +	if (collection_hsdev)
> +		collection_hsdev->end_collection_index = i;
>  
>  	ret = mfd_add_hotplug_devices(&hdev->dev,
>  			sd->hid_sensor_hub_client_devs,
> diff --git a/include/linux/hid-sensor-ids.h b/include/linux/hid-sensor-ids.h
> index 109f0e6..f2ee90a 100644
> --- a/include/linux/hid-sensor-ids.h
> +++ b/include/linux/hid-sensor-ids.h
> @@ -21,6 +21,8 @@
>  
>  #define HID_MAX_PHY_DEVICES					0xFF
>  
> +#define HID_USAGE_SENSOR_COLLECTION				0x200001
> +
>  /* Accel 3D (200073) */
>  #define HID_USAGE_SENSOR_ACCEL_3D				0x200073
>  #define HID_USAGE_SENSOR_DATA_ACCELERATION			0x200452
>