[PATCH 0/2] Core driver enhancements

[PATCH 0/2] Core driver enhancements

[Srinivas Pandruvada]

Enhance core to support next generation of hub devices for fast response
and fusion sensors.

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

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

-- 
1.9.3

[PATCH 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>
---
 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.3

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

[Jonathan Cameron]

On 07/01/15 18:24, Srinivas Pandruvada wrote:
> 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-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Seems sensible to me.  I don't think this has any cross dependencies with
the IIO patches...

Acked-by: Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.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;
>  };
>  
>  /**
> 

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

[Srinivas Pandruvada]

On Wed, 2015-01-21 at 21:45 +0000, Jonathan Cameron wrote: 
> On 07/01/15 18:24, Srinivas Pandruvada wrote:
> > 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-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> Seems sensible to me.  I don't think this has any cross dependencies with
> the IIO patches...
Correct, no dependency with IIO. This is independent.
Thanks for the review.

-Srinivas

> 
> Acked-by: Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.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;
> >  };
> >  
> >  /**
> > 
> 

[PATCH 2/2] HID: hid-sensor-hub: Add collection device

[Srinivas Pandruvada]

HID sensor hub exports several sensor which are fusion sensors, where
data is interpreted from one or more sensors. Some of them can't be
exported via IIO like sysfs as the user space download some firmware,
which defines what sensor will look like. They can be a part of a
collection.
Creating a MFD cell for a collection to write a standalone driver
to manage collections. Most of the time they will be propritery drivers
may not be even upstreamed. This patch allows framework to have
capability to write such drivers.

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

diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
index 865cd56..83b6e15 100644
--- a/drivers/hid/hid-sensor-hub.c
+++ b/drivers/hid/hid-sensor-hub.c
@@ -119,11 +119,12 @@ 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 == HID_USAGE_SENSOR_TYPE_COLLECTION ||
+			(callback->usage_id == usage_id &&
 			(collection_index >=
 				callback->hsdev->start_collection_index) &&
 			(collection_index <
-				callback->hsdev->end_collection_index)) {
+				callback->hsdev->end_collection_index))) {
 			*priv = callback->priv;
 			*hsdev = callback->hsdev;
 			spin_unlock_irqrestore(&pdata->dyn_callback_lock,
@@ -159,7 +160,12 @@ 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);
+	/* Give higher priority to collection device, so add to front */
+	if (usage_id == HID_USAGE_SENSOR_TYPE_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;
@@ -547,6 +553,37 @@ static __u8 *sensor_hub_report_fixup(struct hid_device *hdev, __u8 *rdesc,
 	return rdesc;
 }
 
+static int sensor_hub_add_collection_device(struct hid_device *hdev,
+					    struct sensor_hub_data *sd)
+{
+	struct hid_sensor_hub_device *hsdev;
+	char *name;
+
+	hsdev = devm_kzalloc(&hdev->dev, sizeof(*hsdev), GFP_KERNEL);
+	if (!hsdev)
+		return -ENOMEM;
+
+	hsdev->hdev = hdev;
+	hsdev->vendor_id = hdev->vendor;
+	hsdev->product_id = hdev->product;
+	hsdev->usage = HID_USAGE_SENSOR_TYPE_COLLECTION;
+	mutex_init(&hsdev->mutex);
+	name = devm_kasprintf(&hdev->dev, GFP_KERNEL, "HID-SENSOR-%x",
+			      HID_USAGE_SENSOR_TYPE_COLLECTION);
+	if (name == NULL) {
+		hid_err(hdev, "Failed MFD device name\n");
+		return -ENOMEM;
+	}
+	sd->hid_sensor_hub_client_devs[sd->hid_sensor_client_cnt].name = name;
+	sd->hid_sensor_hub_client_devs[
+			sd->hid_sensor_client_cnt].platform_data = hsdev;
+	sd->hid_sensor_hub_client_devs[
+			sd->hid_sensor_client_cnt].pdata_size = sizeof(*hsdev);
+	sd->hid_sensor_client_cnt++;
+
+	return 0;
+}
+
 static int sensor_hub_probe(struct hid_device *hdev,
 				const struct hid_device_id *id)
 {
@@ -591,7 +628,8 @@ static int sensor_hub_probe(struct hid_device *hdev,
 		ret = -EINVAL;
 		goto err_stop_hw;
 	}
-	sd->hid_sensor_hub_client_devs = devm_kzalloc(&hdev->dev, dev_cnt *
+	sd->hid_sensor_hub_client_devs = devm_kzalloc(&hdev->dev,
+						      (dev_cnt + 1) *
 						      sizeof(struct mfd_cell),
 						      GFP_KERNEL);
 	if (sd->hid_sensor_hub_client_devs == NULL) {
@@ -645,6 +683,10 @@ static int sensor_hub_probe(struct hid_device *hdev,
 	if (last_hsdev)
 		last_hsdev->end_collection_index = i;
 
+	ret = sensor_hub_add_collection_device(hdev, sd);
+	if (ret)
+		goto err_stop_hw;
+
 	ret = mfd_add_hotplug_devices(&hdev->dev,
 			sd->hid_sensor_hub_client_devs,
 			sd->hid_sensor_client_cnt);
diff --git a/include/linux/hid-sensor-ids.h b/include/linux/hid-sensor-ids.h
index 109f0e6..300ffea 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_TYPE_COLLECTION			0x200001
+
 /* Accel 3D (200073) */
 #define HID_USAGE_SENSOR_ACCEL_3D				0x200073
 #define HID_USAGE_SENSOR_DATA_ACCELERATION			0x200452
-- 
1.9.3

Re: [PATCH 2/2] HID: hid-sensor-hub: Add collection device

[Jonathan Cameron]

On 07/01/15 18:24, Srinivas Pandruvada wrote:
> HID sensor hub exports several sensor which are fusion sensors, where
> data is interpreted from one or more sensors. Some of them can't be
> exported via IIO like sysfs as the user space download some firmware,
> which defines what sensor will look like. They can be a part of a
> collection.
This functionality is beginning to turn up on a number of devices.
I'd argue that where at all possible, we should try for standard
interfaces on the resulting 'fused sensors'.  Still you do indeed
always need a means of uploading the firmware!
> Creating a MFD cell for a collection to write a standalone driver
> to manage collections. Most of the time they will be propritery drivers
> may not be even upstreamed. This patch allows framework to have
> capability to write such drivers.
> 
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
>

A few comments inline...

How does userspace download a firmware?

 
 ---
>  drivers/hid/hid-sensor-hub.c   | 50 ++++++++++++++++++++++++++++++++++++++----
>  include/linux/hid-sensor-ids.h |  2 ++
>  2 files changed, 48 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
> index 865cd56..83b6e15 100644
> --- a/drivers/hid/hid-sensor-hub.c
> +++ b/drivers/hid/hid-sensor-hub.c
> @@ -119,11 +119,12 @@ 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 == HID_USAGE_SENSOR_TYPE_COLLECTION ||
> +			(callback->usage_id == usage_id &&
>  			(collection_index >=
>  				callback->hsdev->start_collection_index) &&
>  			(collection_index <
> -				callback->hsdev->end_collection_index)) {
> +				callback->hsdev->end_collection_index))) {
>  			*priv = callback->priv;
>  			*hsdev = callback->hsdev;
>  			spin_unlock_irqrestore(&pdata->dyn_callback_lock,
> @@ -159,7 +160,12 @@ 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);
> +	/* Give higher priority to collection device, so add to front */
Comment should probably state why we want to give them higher priorities.

> +	if (usage_id == HID_USAGE_SENSOR_TYPE_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;
> @@ -547,6 +553,37 @@ static __u8 *sensor_hub_report_fixup(struct hid_device *hdev, __u8 *rdesc,
>  	return rdesc;
>  }
>  
> +static int sensor_hub_add_collection_device(struct hid_device *hdev,
> +					    struct sensor_hub_data *sd)
> +{
> +	struct hid_sensor_hub_device *hsdev;
> +	char *name;
> +
> +	hsdev = devm_kzalloc(&hdev->dev, sizeof(*hsdev), GFP_KERNEL);
> +	if (!hsdev)
> +		return -ENOMEM;
> +
> +	hsdev->hdev = hdev;
> +	hsdev->vendor_id = hdev->vendor;
> +	hsdev->product_id = hdev->product;
> +	hsdev->usage = HID_USAGE_SENSOR_TYPE_COLLECTION;
> +	mutex_init(&hsdev->mutex);
> +	name = devm_kasprintf(&hdev->dev, GFP_KERNEL, "HID-SENSOR-%x",
> +			      HID_USAGE_SENSOR_TYPE_COLLECTION);
> +	if (name == NULL) {
> +		hid_err(hdev, "Failed MFD device name\n");
> +		return -ENOMEM;
> +	}
> +	sd->hid_sensor_hub_client_devs[sd->hid_sensor_client_cnt].name = name;
> +	sd->hid_sensor_hub_client_devs[
> +			sd->hid_sensor_client_cnt].platform_data = hsdev;
Odd breaking up of the line...  Perhaps break before .platform_data instead?
> +	sd->hid_sensor_hub_client_devs[
> +			sd->hid_sensor_client_cnt].pdata_size = sizeof(*hsdev);
> +	sd->hid_sensor_client_cnt++;
> +
> +	return 0;
> +}
> +
>  static int sensor_hub_probe(struct hid_device *hdev,
>  				const struct hid_device_id *id)
>  {
> @@ -591,7 +628,8 @@ static int sensor_hub_probe(struct hid_device *hdev,
>  		ret = -EINVAL;
>  		goto err_stop_hw;
>  	}
> -	sd->hid_sensor_hub_client_devs = devm_kzalloc(&hdev->dev, dev_cnt *
> +	sd->hid_sensor_hub_client_devs = devm_kzalloc(&hdev->dev,
> +						      (dev_cnt + 1) *
>  						      sizeof(struct mfd_cell),
>  						      GFP_KERNEL);
>  	if (sd->hid_sensor_hub_client_devs == NULL) {
> @@ -645,6 +683,10 @@ static int sensor_hub_probe(struct hid_device *hdev,
>  	if (last_hsdev)
>  		last_hsdev->end_collection_index = i;
>  
> +	ret = sensor_hub_add_collection_device(hdev, sd);
> +	if (ret)
> +		goto err_stop_hw;
> +
>  	ret = mfd_add_hotplug_devices(&hdev->dev,
>  			sd->hid_sensor_hub_client_devs,
>  			sd->hid_sensor_client_cnt);
> diff --git a/include/linux/hid-sensor-ids.h b/include/linux/hid-sensor-ids.h
> index 109f0e6..300ffea 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_TYPE_COLLECTION			0x200001
> +
>  /* Accel 3D (200073) */
>  #define HID_USAGE_SENSOR_ACCEL_3D				0x200073
>  #define HID_USAGE_SENSOR_DATA_ACCELERATION			0x200452
> 

Re: [PATCH 2/2] HID: hid-sensor-hub: Add collection device

[Jiri Kosina]

On Wed, 21 Jan 2015, Jonathan Cameron wrote:

> On 07/01/15 18:24, Srinivas Pandruvada wrote:
> > HID sensor hub exports several sensor which are fusion sensors, where
> > data is interpreted from one or more sensors. Some of them can't be
> > exported via IIO like sysfs as the user space download some firmware,
> > which defines what sensor will look like. They can be a part of a
> > collection.
> This functionality is beginning to turn up on a number of devices.
> I'd argue that where at all possible, we should try for standard
> interfaces on the resulting 'fused sensors'.  Still you do indeed
> always need a means of uploading the firmware!
> > Creating a MFD cell for a collection to write a standalone driver
> > to manage collections. Most of the time they will be propritery drivers
> > may not be even upstreamed. This patch allows framework to have
> > capability to write such drivers.

Srinivas, do you plan to respond to the comments Jonathan had on this 
patch?

Thanks.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 2/2] HID: hid-sensor-hub: Add collection device

[Srinivas Pandruvada]

On Wed, 2015-01-21 at 21:53 +0000, Jonathan Cameron wrote:
> On 07/01/15 18:24, Srinivas Pandruvada wrote:
> > HID sensor hub exports several sensor which are fusion sensors, where
> > data is interpreted from one or more sensors. Some of them can't be
> > exported via IIO like sysfs as the user space download some firmware,
> > which defines what sensor will look like. They can be a part of a
> > collection.
> This functionality is beginning to turn up on a number of devices.
> I'd argue that where at all possible, we should try for standard
> interfaces on the resulting 'fused sensors'.  Still you do indeed
> always need a means of uploading the firmware!
I will submitted RFCs, once I see in production releases.

Currently planning to use misc driver for FW update.
What do you think about using dev/iio:deviceX for such case to download
FW for each sensor? We also need mechanism to initiate and reset after
FW download.

Thanks,
Srinivas

> > Creating a MFD cell for a collection to write a standalone driver
> > to manage collections. Most of the time they will be propritery drivers
> > may not be even upstreamed. This patch allows framework to have
> > capability to write such drivers.
> > 
> > Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> >
> 
> A few comments inline...
> 
> How does userspace download a firmware?
> 
>  
>  ---
> >  drivers/hid/hid-sensor-hub.c   | 50 ++++++++++++++++++++++++++++++++++++++----
> >  include/linux/hid-sensor-ids.h |  2 ++
> >  2 files changed, 48 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
> > index 865cd56..83b6e15 100644
> > --- a/drivers/hid/hid-sensor-hub.c
> > +++ b/drivers/hid/hid-sensor-hub.c
> > @@ -119,11 +119,12 @@ 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 == HID_USAGE_SENSOR_TYPE_COLLECTION ||
> > +			(callback->usage_id == usage_id &&
> >  			(collection_index >=
> >  				callback->hsdev->start_collection_index) &&
> >  			(collection_index <
> > -				callback->hsdev->end_collection_index)) {
> > +				callback->hsdev->end_collection_index))) {
> >  			*priv = callback->priv;
> >  			*hsdev = callback->hsdev;
> >  			spin_unlock_irqrestore(&pdata->dyn_callback_lock,
> > @@ -159,7 +160,12 @@ 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);
> > +	/* Give higher priority to collection device, so add to front */
> Comment should probably state why we want to give them higher priorities.
> 
> > +	if (usage_id == HID_USAGE_SENSOR_TYPE_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;
> > @@ -547,6 +553,37 @@ static __u8 *sensor_hub_report_fixup(struct hid_device *hdev, __u8 *rdesc,
> >  	return rdesc;
> >  }
> >  
> > +static int sensor_hub_add_collection_device(struct hid_device *hdev,
> > +					    struct sensor_hub_data *sd)
> > +{
> > +	struct hid_sensor_hub_device *hsdev;
> > +	char *name;
> > +
> > +	hsdev = devm_kzalloc(&hdev->dev, sizeof(*hsdev), GFP_KERNEL);
> > +	if (!hsdev)
> > +		return -ENOMEM;
> > +
> > +	hsdev->hdev = hdev;
> > +	hsdev->vendor_id = hdev->vendor;
> > +	hsdev->product_id = hdev->product;
> > +	hsdev->usage = HID_USAGE_SENSOR_TYPE_COLLECTION;
> > +	mutex_init(&hsdev->mutex);
> > +	name = devm_kasprintf(&hdev->dev, GFP_KERNEL, "HID-SENSOR-%x",
> > +			      HID_USAGE_SENSOR_TYPE_COLLECTION);
> > +	if (name == NULL) {
> > +		hid_err(hdev, "Failed MFD device name\n");
> > +		return -ENOMEM;
> > +	}
> > +	sd->hid_sensor_hub_client_devs[sd->hid_sensor_client_cnt].name = name;
> > +	sd->hid_sensor_hub_client_devs[
> > +			sd->hid_sensor_client_cnt].platform_data = hsdev;
> Odd breaking up of the line...  Perhaps break before .platform_data instead?
> > +	sd->hid_sensor_hub_client_devs[
> > +			sd->hid_sensor_client_cnt].pdata_size = sizeof(*hsdev);
> > +	sd->hid_sensor_client_cnt++;
> > +
> > +	return 0;
> > +}
> > +
> >  static int sensor_hub_probe(struct hid_device *hdev,
> >  				const struct hid_device_id *id)
> >  {
> > @@ -591,7 +628,8 @@ static int sensor_hub_probe(struct hid_device *hdev,
> >  		ret = -EINVAL;
> >  		goto err_stop_hw;
> >  	}
> > -	sd->hid_sensor_hub_client_devs = devm_kzalloc(&hdev->dev, dev_cnt *
> > +	sd->hid_sensor_hub_client_devs = devm_kzalloc(&hdev->dev,
> > +						      (dev_cnt + 1) *
> >  						      sizeof(struct mfd_cell),
> >  						      GFP_KERNEL);
> >  	if (sd->hid_sensor_hub_client_devs == NULL) {
> > @@ -645,6 +683,10 @@ static int sensor_hub_probe(struct hid_device *hdev,
> >  	if (last_hsdev)
> >  		last_hsdev->end_collection_index = i;
> >  
> > +	ret = sensor_hub_add_collection_device(hdev, sd);
> > +	if (ret)
> > +		goto err_stop_hw;
> > +
> >  	ret = mfd_add_hotplug_devices(&hdev->dev,
> >  			sd->hid_sensor_hub_client_devs,
> >  			sd->hid_sensor_client_cnt);
> > diff --git a/include/linux/hid-sensor-ids.h b/include/linux/hid-sensor-ids.h
> > index 109f0e6..300ffea 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_TYPE_COLLECTION			0x200001
> > +
> >  /* Accel 3D (200073) */
> >  #define HID_USAGE_SENSOR_ACCEL_3D				0x200073
> >  #define HID_USAGE_SENSOR_DATA_ACCELERATION			0x200452
> > 
> 

Re: [PATCH 2/2] HID: hid-sensor-hub: Add collection device

[Jonathan Cameron]

On 19/02/15 23:54, Srinivas Pandruvada wrote:
> On Wed, 2015-01-21 at 21:53 +0000, Jonathan Cameron wrote:
>> On 07/01/15 18:24, Srinivas Pandruvada wrote:
>>> HID sensor hub exports several sensor which are fusion sensors, where
>>> data is interpreted from one or more sensors. Some of them can't be
>>> exported via IIO like sysfs as the user space download some firmware,
>>> which defines what sensor will look like. They can be a part of a
>>> collection.
>> This functionality is beginning to turn up on a number of devices.
>> I'd argue that where at all possible, we should try for standard
>> interfaces on the resulting 'fused sensors'.  Still you do indeed
>> always need a means of uploading the firmware!
> I will submitted RFCs, once I see in production releases.
Cool
> 
> Currently planning to use misc driver for FW update.
> What do you think about using dev/iio:deviceX for such case to download
> FW for each sensor? We also need mechanism to initiate and reset after
> FW download.
I think using the main iio device is pushing things a little.  We could add
the ability to get another anonymous cdev like we do for events I suppose.
That way, at least it will be tied to the normal iio device.

Sort of feels like there should be a generic / standard way of doing
this sort of firmware update, irrespective of what the device does.
I guess, maybe it's always non generic enough that this isn't
possible.
> 
> Thanks,
> Srinivas
> 
>>> Creating a MFD cell for a collection to write a standalone driver
>>> to manage collections. Most of the time they will be propritery drivers
>>> may not be even upstreamed. This patch allows framework to have
>>> capability to write such drivers.
>>>
>>> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
>>>
>>
>> A few comments inline...
>>
>> How does userspace download a firmware?
>>
>>  
>>  ---
>>>  drivers/hid/hid-sensor-hub.c   | 50 ++++++++++++++++++++++++++++++++++++++----
>>>  include/linux/hid-sensor-ids.h |  2 ++
>>>  2 files changed, 48 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
>>> index 865cd56..83b6e15 100644
>>> --- a/drivers/hid/hid-sensor-hub.c
>>> +++ b/drivers/hid/hid-sensor-hub.c
>>> @@ -119,11 +119,12 @@ 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 == HID_USAGE_SENSOR_TYPE_COLLECTION ||
>>> +			(callback->usage_id == usage_id &&
>>>  			(collection_index >=
>>>  				callback->hsdev->start_collection_index) &&
>>>  			(collection_index <
>>> -				callback->hsdev->end_collection_index)) {
>>> +				callback->hsdev->end_collection_index))) {
>>>  			*priv = callback->priv;
>>>  			*hsdev = callback->hsdev;
>>>  			spin_unlock_irqrestore(&pdata->dyn_callback_lock,
>>> @@ -159,7 +160,12 @@ 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);
>>> +	/* Give higher priority to collection device, so add to front */
>> Comment should probably state why we want to give them higher priorities.
>>
>>> +	if (usage_id == HID_USAGE_SENSOR_TYPE_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;
>>> @@ -547,6 +553,37 @@ static __u8 *sensor_hub_report_fixup(struct hid_device *hdev, __u8 *rdesc,
>>>  	return rdesc;
>>>  }
>>>  
>>> +static int sensor_hub_add_collection_device(struct hid_device *hdev,
>>> +					    struct sensor_hub_data *sd)
>>> +{
>>> +	struct hid_sensor_hub_device *hsdev;
>>> +	char *name;
>>> +
>>> +	hsdev = devm_kzalloc(&hdev->dev, sizeof(*hsdev), GFP_KERNEL);
>>> +	if (!hsdev)
>>> +		return -ENOMEM;
>>> +
>>> +	hsdev->hdev = hdev;
>>> +	hsdev->vendor_id = hdev->vendor;
>>> +	hsdev->product_id = hdev->product;
>>> +	hsdev->usage = HID_USAGE_SENSOR_TYPE_COLLECTION;
>>> +	mutex_init(&hsdev->mutex);
>>> +	name = devm_kasprintf(&hdev->dev, GFP_KERNEL, "HID-SENSOR-%x",
>>> +			      HID_USAGE_SENSOR_TYPE_COLLECTION);
>>> +	if (name == NULL) {
>>> +		hid_err(hdev, "Failed MFD device name\n");
>>> +		return -ENOMEM;
>>> +	}
>>> +	sd->hid_sensor_hub_client_devs[sd->hid_sensor_client_cnt].name = name;
>>> +	sd->hid_sensor_hub_client_devs[
>>> +			sd->hid_sensor_client_cnt].platform_data = hsdev;
>> Odd breaking up of the line...  Perhaps break before .platform_data instead?
>>> +	sd->hid_sensor_hub_client_devs[
>>> +			sd->hid_sensor_client_cnt].pdata_size = sizeof(*hsdev);
>>> +	sd->hid_sensor_client_cnt++;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>  static int sensor_hub_probe(struct hid_device *hdev,
>>>  				const struct hid_device_id *id)
>>>  {
>>> @@ -591,7 +628,8 @@ static int sensor_hub_probe(struct hid_device *hdev,
>>>  		ret = -EINVAL;
>>>  		goto err_stop_hw;
>>>  	}
>>> -	sd->hid_sensor_hub_client_devs = devm_kzalloc(&hdev->dev, dev_cnt *
>>> +	sd->hid_sensor_hub_client_devs = devm_kzalloc(&hdev->dev,
>>> +						      (dev_cnt + 1) *
>>>  						      sizeof(struct mfd_cell),
>>>  						      GFP_KERNEL);
>>>  	if (sd->hid_sensor_hub_client_devs == NULL) {
>>> @@ -645,6 +683,10 @@ static int sensor_hub_probe(struct hid_device *hdev,
>>>  	if (last_hsdev)
>>>  		last_hsdev->end_collection_index = i;
>>>  
>>> +	ret = sensor_hub_add_collection_device(hdev, sd);
>>> +	if (ret)
>>> +		goto err_stop_hw;
>>> +
>>>  	ret = mfd_add_hotplug_devices(&hdev->dev,
>>>  			sd->hid_sensor_hub_client_devs,
>>>  			sd->hid_sensor_client_cnt);
>>> diff --git a/include/linux/hid-sensor-ids.h b/include/linux/hid-sensor-ids.h
>>> index 109f0e6..300ffea 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_TYPE_COLLECTION			0x200001
>>> +
>>>  /* Accel 3D (200073) */
>>>  #define HID_USAGE_SENSOR_ACCEL_3D				0x200073
>>>  #define HID_USAGE_SENSOR_DATA_ACCELERATION			0x200452
>>>
>>
> 
>