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

[Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

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
>>>
>>
> 
>