[PATCH v2] iio: hid-sensor-hub: Implement batch mode

[PATCH v2] iio: hid-sensor-hub: Implement batch mode

[Srinivas Pandruvada]

HID sensor hubs using Integrated Senor Hub (ISH) has added capability to
support batch mode. This allows host processor to go to sleep for extended
duration, while the sensor hub is storing samples in its internal buffers.

'Commit f4f4673b7535 ("iio: add support for hardware fifo")' implements
feature in IIO core to implement such feature. This feature is used in
bmc150-accel-core.c to implement batch mode. This implementation allows
software device buffer watermark to be used as a hint to adjust hardware
FIFO.

But HID sensor hubs don't allow to change internal buffer size of FIFOs.
Instead an additional usage id to set "maximum report latency" is defined.
This allows host to go to sleep upto this latency period without getting
any report. Since there is no ABI to set this latency, a new attribute
"hwfifo_timeout" is added so that user mode can specify a latency.

This change checks presence of usage id to get/set maximum report latency
and if present, it will expose hwfifo_timeout.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
v2:
- Fifo timeout can be in fractions also.
- Restore latency on resume from S3.

Changed compared to RFC:
- Removed min/max Fifo size attributes
- Removed flush callback. Since this usage id is not present in any hubs,
we can add later when we have some HW to test. This was causing some
ugliness.
- Moved the hwfifo_timout documentation to sysfs-bus-iio

 Documentation/ABI/testing/sysfs-bus-iio            | 11 +++
 .../iio/common/hid-sensors/hid-sensor-attributes.c | 44 ++++++++++++
 .../iio/common/hid-sensors/hid-sensor-trigger.c    | 80 ++++++++++++++++++++++
 include/linux/hid-sensor-hub.h                     |  5 ++
 include/linux/hid-sensor-ids.h                     |  3 +
 5 files changed, 143 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
index 530809c..b09f373 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -1424,6 +1424,17 @@ Description:
 		guarantees that the hardware fifo is flushed to the device
 		buffer.
 
+What:		/sys/bus/iio/devices/iio:device*/buffer/hwfifo_timeout
+KernelVersion:	4.12
+Contact:	linux-iio@vger.kernel.org
+Description:
+		A read/write property to provide capability to delay reporting of
+		samples till a timeout is reached. This allows host processors to
+		sleep, while the sensor is storing samples in its internal fifo.
+		The maximum timeout in seconds can be specified by setting
+		hwfifo_timeout.The current delay can be read by reading
+		hwfifo_timeout. A value of 0 means that there is no timout.
+
 What:		/sys/bus/iio/devices/iio:deviceX/buffer/hwfifo_watermark
 KernelVersion: 4.2
 Contact:	linux-iio@vger.kernel.org
diff --git a/drivers/iio/common/hid-sensors/hid-sensor-attributes.c b/drivers/iio/common/hid-sensors/hid-sensor-attributes.c
index efd3151..89c3f9c 100644
--- a/drivers/iio/common/hid-sensors/hid-sensor-attributes.c
+++ b/drivers/iio/common/hid-sensors/hid-sensor-attributes.c
@@ -393,6 +393,48 @@ int hid_sensor_get_reporting_interval(struct hid_sensor_hub_device *hsdev,
 
 }
 
+static void hid_sensor_get_report_latency_info(struct hid_sensor_hub_device *hsdev,
+					       u32 usage_id,
+					       struct hid_sensor_common *st)
+{
+	sensor_hub_input_get_attribute_info(hsdev, HID_FEATURE_REPORT,
+					    usage_id,
+					    HID_USAGE_SENSOR_PROP_REPORT_LATENCY,
+					    &st->report_latency);
+
+	hid_dbg(hsdev->hdev, "Report latency attributes: %x:%x\n",
+		st->report_latency.index, st->report_latency.report_id);
+}
+
+int hid_sensor_get_report_latency(struct hid_sensor_common *st)
+{
+	int ret;
+	int value;
+
+	ret = sensor_hub_get_feature(st->hsdev, st->report_latency.report_id,
+				     st->report_latency.index, sizeof(value),
+				     &value);
+	if (ret < 0)
+		return ret;
+
+	return value;
+}
+EXPORT_SYMBOL(hid_sensor_get_report_latency);
+
+int hid_sensor_set_report_latency(struct hid_sensor_common *st, int latency_ms)
+{
+	return sensor_hub_set_feature(st->hsdev, st->report_latency.report_id,
+				      st->report_latency.index,
+				      sizeof(latency_ms), &latency_ms);
+}
+EXPORT_SYMBOL(hid_sensor_set_report_latency);
+
+bool hid_sensor_batch_mode_supported(struct hid_sensor_common *st)
+{
+	return st->report_latency.index > 0 && st->report_latency.report_id > 0;
+}
+EXPORT_SYMBOL(hid_sensor_batch_mode_supported);
+
 int hid_sensor_parse_common_attributes(struct hid_sensor_hub_device *hsdev,
 					u32 usage_id,
 					struct hid_sensor_common *st)
@@ -432,6 +474,8 @@ int hid_sensor_parse_common_attributes(struct hid_sensor_hub_device *hsdev,
 	} else
 		st->timestamp_ns_scale = 1000000000;
 
+	hid_sensor_get_report_latency_info(hsdev, usage_id, st);
+
 	hid_dbg(hsdev->hdev, "common attributes: %x:%x, %x:%x, %x:%x %x:%x %x:%x\n",
 		st->poll.index, st->poll.report_id,
 		st->report_state.index, st->report_state.report_id,
diff --git a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
index 0b5dea0..16ade0a 100644
--- a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
+++ b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
@@ -26,9 +26,84 @@
 #include <linux/hid-sensor-hub.h>
 #include <linux/iio/iio.h>
 #include <linux/iio/trigger.h>
+#include <linux/iio/buffer.h>
 #include <linux/iio/sysfs.h>
 #include "hid-sensor-trigger.h"
 
+static ssize_t _hid_sensor_set_report_latency(struct device *dev,
+					      struct device_attribute *attr,
+					      const char *buf, size_t len)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct hid_sensor_common *attrb = iio_device_get_drvdata(indio_dev);
+	int integer, fract, ret;
+	int latency;
+
+	ret = iio_str_to_fixpoint(buf, 100000, &integer, &fract);
+	if (ret)
+		return ret;
+
+	latency = integer * 1000 + fract / 1000;
+	ret = hid_sensor_set_report_latency(attrb, latency);
+	if (ret < 0)
+		return len;
+
+	attrb->latency_ms = hid_sensor_get_report_latency(attrb);
+
+	return len;
+}
+
+static ssize_t _hid_sensor_get_report_latency(struct device *dev,
+					      struct device_attribute *attr,
+					      char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct hid_sensor_common *attrb = iio_device_get_drvdata(indio_dev);
+	int latency;
+
+	latency = hid_sensor_get_report_latency(attrb);
+	if (latency < 0)
+		return latency;
+
+	return sprintf(buf, "%d.%06u\n", latency / 1000, (latency % 1000) * 1000);
+}
+
+static ssize_t _hid_sensor_get_fifo_state(struct device *dev,
+					  struct device_attribute *attr,
+					  char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct hid_sensor_common *attrb = iio_device_get_drvdata(indio_dev);
+	int latency;
+
+	latency = hid_sensor_get_report_latency(attrb);
+	if (latency < 0)
+		return latency;
+
+	return sprintf(buf, "%d\n", !!latency);
+}
+
+static IIO_DEVICE_ATTR(hwfifo_timeout, 0644,
+		       _hid_sensor_get_report_latency,
+		       _hid_sensor_set_report_latency, 0);
+static IIO_DEVICE_ATTR(hwfifo_enabled, 0444,
+		       _hid_sensor_get_fifo_state, NULL, 0);
+
+static const struct attribute *hid_sensor_fifo_attributes[] = {
+	&iio_dev_attr_hwfifo_timeout.dev_attr.attr,
+	&iio_dev_attr_hwfifo_enabled.dev_attr.attr,
+	NULL,
+};
+
+static void hid_sensor_setup_batch_mode(struct iio_dev *indio_dev,
+					struct hid_sensor_common *st)
+{
+	if (!hid_sensor_batch_mode_supported(st))
+		return;
+
+	iio_buffer_set_attrs(indio_dev->buffer, hid_sensor_fifo_attributes);
+}
+
 static int _hid_sensor_power_state(struct hid_sensor_common *st, bool state)
 {
 	int state_val;
@@ -141,6 +216,9 @@ static void hid_sensor_set_power_work(struct work_struct *work)
 				       sizeof(attrb->raw_hystersis),
 				       &attrb->raw_hystersis);
 
+	if (attrb->latency_ms > 0)
+		hid_sensor_set_report_latency(attrb, attrb->latency_ms);
+
 	_hid_sensor_power_state(attrb, true);
 }
 
@@ -192,6 +270,8 @@ int hid_sensor_setup_trigger(struct iio_dev *indio_dev, const char *name,
 	attrb->trigger = trig;
 	indio_dev->trig = iio_trigger_get(trig);
 
+	hid_sensor_setup_batch_mode(indio_dev, attrb);
+
 	ret = pm_runtime_set_active(&indio_dev->dev);
 	if (ret)
 		goto error_unreg_trigger;
diff --git a/include/linux/hid-sensor-hub.h b/include/linux/hid-sensor-hub.h
index f32d7c3..fc7aae6 100644
--- a/include/linux/hid-sensor-hub.h
+++ b/include/linux/hid-sensor-hub.h
@@ -233,12 +233,14 @@ struct hid_sensor_common {
 	atomic_t user_requested_state;
 	int poll_interval;
 	int raw_hystersis;
+	int latency_ms;
 	struct iio_trigger *trigger;
 	int timestamp_ns_scale;
 	struct hid_sensor_hub_attribute_info poll;
 	struct hid_sensor_hub_attribute_info report_state;
 	struct hid_sensor_hub_attribute_info power_state;
 	struct hid_sensor_hub_attribute_info sensitivity;
+	struct hid_sensor_hub_attribute_info report_latency;
 	struct work_struct work;
 };
 
@@ -276,5 +278,8 @@ s32 hid_sensor_read_poll_value(struct hid_sensor_common *st);
 
 int64_t hid_sensor_convert_timestamp(struct hid_sensor_common *st,
 				     int64_t raw_value);
+bool hid_sensor_batch_mode_supported(struct hid_sensor_common *st);
+int hid_sensor_set_report_latency(struct hid_sensor_common *st, int latency);
+int hid_sensor_get_report_latency(struct hid_sensor_common *st);
 
 #endif
diff --git a/include/linux/hid-sensor-ids.h b/include/linux/hid-sensor-ids.h
index 30c7dc4..d7caf8c 100644
--- a/include/linux/hid-sensor-ids.h
+++ b/include/linux/hid-sensor-ids.h
@@ -142,6 +142,9 @@
 #define HID_USAGE_SENSOR_PROP_REPORT_STATE			0x200316
 #define HID_USAGE_SENSOR_PROY_POWER_STATE			0x200319
 
+/* Batch mode selectors */
+#define HID_USAGE_SENSOR_PROP_REPORT_LATENCY			0x20031B
+
 /* Per data field properties */
 #define HID_USAGE_SENSOR_DATA_MOD_NONE					0x00
 #define HID_USAGE_SENSOR_DATA_MOD_CHANGE_SENSITIVITY_ABS		0x1000
-- 
2.9.3

Re: [PATCH v2] iio: hid-sensor-hub: Implement batch mode

[Jonathan Cameron]

On 08/04/17 03:22, Srinivas Pandruvada wrote:
> HID sensor hubs using Integrated Senor Hub (ISH) has added capability to
> support batch mode. This allows host processor to go to sleep for extended
> duration, while the sensor hub is storing samples in its internal buffers.
> 
> 'Commit f4f4673b7535 ("iio: add support for hardware fifo")' implements
> feature in IIO core to implement such feature. This feature is used in
> bmc150-accel-core.c to implement batch mode. This implementation allows
> software device buffer watermark to be used as a hint to adjust hardware
> FIFO.
> 
> But HID sensor hubs don't allow to change internal buffer size of FIFOs.
> Instead an additional usage id to set "maximum report latency" is defined.
> This allows host to go to sleep upto this latency period without getting
> any report. Since there is no ABI to set this latency, a new attribute
> "hwfifo_timeout" is added so that user mode can specify a latency.
> 
> This change checks presence of usage id to get/set maximum report latency
> and if present, it will expose hwfifo_timeout.
> 
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
I'm happy with this. Spotted one typo inline.

However, it's run into the back of the two fixes you posted earlier so
doesn't currently apply to the togreg branch. We'll have to let those
unwind first and rebase as necessary.  Feel free to poke me if I
seem to have forgotten this!

I guess this gives it a bit longer for others to comment which is perhaps
no bad thing.

Thanks,

Jonathan
> ---
> v2:
> - Fifo timeout can be in fractions also.
> - Restore latency on resume from S3.
> 
> Changed compared to RFC:
> - Removed min/max Fifo size attributes
> - Removed flush callback. Since this usage id is not present in any hubs,
> we can add later when we have some HW to test. This was causing some
> ugliness.
> - Moved the hwfifo_timout documentation to sysfs-bus-iio
> 
>  Documentation/ABI/testing/sysfs-bus-iio            | 11 +++
>  .../iio/common/hid-sensors/hid-sensor-attributes.c | 44 ++++++++++++
>  .../iio/common/hid-sensors/hid-sensor-trigger.c    | 80 ++++++++++++++++++++++
>  include/linux/hid-sensor-hub.h                     |  5 ++
>  include/linux/hid-sensor-ids.h                     |  3 +
>  5 files changed, 143 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> index 530809c..b09f373 100644
> --- a/Documentation/ABI/testing/sysfs-bus-iio
> +++ b/Documentation/ABI/testing/sysfs-bus-iio
> @@ -1424,6 +1424,17 @@ Description:
>  		guarantees that the hardware fifo is flushed to the device
>  		buffer.
>  
> +What:		/sys/bus/iio/devices/iio:device*/buffer/hwfifo_timeout
> +KernelVersion:	4.12
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		A read/write property to provide capability to delay reporting of
> +		samples till a timeout is reached. This allows host processors to
> +		sleep, while the sensor is storing samples in its internal fifo.
> +		The maximum timeout in seconds can be specified by setting
> +		hwfifo_timeout.The current delay can be read by reading
> +		hwfifo_timeout. A value of 0 means that there is no timout.
timeout. I'll fix.
> +
>  What:		/sys/bus/iio/devices/iio:deviceX/buffer/hwfifo_watermark
>  KernelVersion: 4.2
>  Contact:	linux-iio@vger.kernel.org
> diff --git a/drivers/iio/common/hid-sensors/hid-sensor-attributes.c b/drivers/iio/common/hid-sensors/hid-sensor-attributes.c
> index efd3151..89c3f9c 100644
> --- a/drivers/iio/common/hid-sensors/hid-sensor-attributes.c
> +++ b/drivers/iio/common/hid-sensors/hid-sensor-attributes.c
> @@ -393,6 +393,48 @@ int hid_sensor_get_reporting_interval(struct hid_sensor_hub_device *hsdev,
>  
>  }
>  
> +static void hid_sensor_get_report_latency_info(struct hid_sensor_hub_device *hsdev,
> +					       u32 usage_id,
> +					       struct hid_sensor_common *st)
> +{
> +	sensor_hub_input_get_attribute_info(hsdev, HID_FEATURE_REPORT,
> +					    usage_id,
> +					    HID_USAGE_SENSOR_PROP_REPORT_LATENCY,
> +					    &st->report_latency);
> +
> +	hid_dbg(hsdev->hdev, "Report latency attributes: %x:%x\n",
> +		st->report_latency.index, st->report_latency.report_id);
> +}
> +
> +int hid_sensor_get_report_latency(struct hid_sensor_common *st)
> +{
> +	int ret;
> +	int value;
> +
> +	ret = sensor_hub_get_feature(st->hsdev, st->report_latency.report_id,
> +				     st->report_latency.index, sizeof(value),
> +				     &value);
> +	if (ret < 0)
> +		return ret;
> +
> +	return value;
> +}
> +EXPORT_SYMBOL(hid_sensor_get_report_latency);
> +
> +int hid_sensor_set_report_latency(struct hid_sensor_common *st, int latency_ms)
> +{
> +	return sensor_hub_set_feature(st->hsdev, st->report_latency.report_id,
> +				      st->report_latency.index,
> +				      sizeof(latency_ms), &latency_ms);
> +}
> +EXPORT_SYMBOL(hid_sensor_set_report_latency);
> +
> +bool hid_sensor_batch_mode_supported(struct hid_sensor_common *st)
> +{
> +	return st->report_latency.index > 0 && st->report_latency.report_id > 0;
> +}
> +EXPORT_SYMBOL(hid_sensor_batch_mode_supported);
> +
>  int hid_sensor_parse_common_attributes(struct hid_sensor_hub_device *hsdev,
>  					u32 usage_id,
>  					struct hid_sensor_common *st)
> @@ -432,6 +474,8 @@ int hid_sensor_parse_common_attributes(struct hid_sensor_hub_device *hsdev,
>  	} else
>  		st->timestamp_ns_scale = 1000000000;
>  
> +	hid_sensor_get_report_latency_info(hsdev, usage_id, st);
> +
>  	hid_dbg(hsdev->hdev, "common attributes: %x:%x, %x:%x, %x:%x %x:%x %x:%x\n",
>  		st->poll.index, st->poll.report_id,
>  		st->report_state.index, st->report_state.report_id,
> diff --git a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> index 0b5dea0..16ade0a 100644
> --- a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> +++ b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> @@ -26,9 +26,84 @@
>  #include <linux/hid-sensor-hub.h>
>  #include <linux/iio/iio.h>
>  #include <linux/iio/trigger.h>
> +#include <linux/iio/buffer.h>
>  #include <linux/iio/sysfs.h>
>  #include "hid-sensor-trigger.h"
>  
> +static ssize_t _hid_sensor_set_report_latency(struct device *dev,
> +					      struct device_attribute *attr,
> +					      const char *buf, size_t len)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct hid_sensor_common *attrb = iio_device_get_drvdata(indio_dev);
> +	int integer, fract, ret;
> +	int latency;
> +
> +	ret = iio_str_to_fixpoint(buf, 100000, &integer, &fract);
> +	if (ret)
> +		return ret;
> +
> +	latency = integer * 1000 + fract / 1000;
> +	ret = hid_sensor_set_report_latency(attrb, latency);
> +	if (ret < 0)
> +		return len;
> +
> +	attrb->latency_ms = hid_sensor_get_report_latency(attrb);
> +
> +	return len;
> +}
> +
> +static ssize_t _hid_sensor_get_report_latency(struct device *dev,
> +					      struct device_attribute *attr,
> +					      char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct hid_sensor_common *attrb = iio_device_get_drvdata(indio_dev);
> +	int latency;
> +
> +	latency = hid_sensor_get_report_latency(attrb);
> +	if (latency < 0)
> +		return latency;
> +
> +	return sprintf(buf, "%d.%06u\n", latency / 1000, (latency % 1000) * 1000);
> +}
> +
> +static ssize_t _hid_sensor_get_fifo_state(struct device *dev,
> +					  struct device_attribute *attr,
> +					  char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct hid_sensor_common *attrb = iio_device_get_drvdata(indio_dev);
> +	int latency;
> +
> +	latency = hid_sensor_get_report_latency(attrb);
> +	if (latency < 0)
> +		return latency;
> +
> +	return sprintf(buf, "%d\n", !!latency);
> +}
> +
> +static IIO_DEVICE_ATTR(hwfifo_timeout, 0644,
> +		       _hid_sensor_get_report_latency,
> +		       _hid_sensor_set_report_latency, 0);
> +static IIO_DEVICE_ATTR(hwfifo_enabled, 0444,
> +		       _hid_sensor_get_fifo_state, NULL, 0);
> +
> +static const struct attribute *hid_sensor_fifo_attributes[] = {
> +	&iio_dev_attr_hwfifo_timeout.dev_attr.attr,
> +	&iio_dev_attr_hwfifo_enabled.dev_attr.attr,
> +	NULL,
> +};
> +
> +static void hid_sensor_setup_batch_mode(struct iio_dev *indio_dev,
> +					struct hid_sensor_common *st)
> +{
> +	if (!hid_sensor_batch_mode_supported(st))
> +		return;
> +
> +	iio_buffer_set_attrs(indio_dev->buffer, hid_sensor_fifo_attributes);
> +}
> +
>  static int _hid_sensor_power_state(struct hid_sensor_common *st, bool state)
>  {
>  	int state_val;
> @@ -141,6 +216,9 @@ static void hid_sensor_set_power_work(struct work_struct *work)
>  				       sizeof(attrb->raw_hystersis),
>  				       &attrb->raw_hystersis);
>  
> +	if (attrb->latency_ms > 0)
> +		hid_sensor_set_report_latency(attrb, attrb->latency_ms);
> +
>  	_hid_sensor_power_state(attrb, true);
>  }
>  
> @@ -192,6 +270,8 @@ int hid_sensor_setup_trigger(struct iio_dev *indio_dev, const char *name,
>  	attrb->trigger = trig;
>  	indio_dev->trig = iio_trigger_get(trig);
>  
> +	hid_sensor_setup_batch_mode(indio_dev, attrb);
> +
>  	ret = pm_runtime_set_active(&indio_dev->dev);
>  	if (ret)
>  		goto error_unreg_trigger;
> diff --git a/include/linux/hid-sensor-hub.h b/include/linux/hid-sensor-hub.h
> index f32d7c3..fc7aae6 100644
> --- a/include/linux/hid-sensor-hub.h
> +++ b/include/linux/hid-sensor-hub.h
> @@ -233,12 +233,14 @@ struct hid_sensor_common {
>  	atomic_t user_requested_state;
>  	int poll_interval;
>  	int raw_hystersis;
> +	int latency_ms;
>  	struct iio_trigger *trigger;
>  	int timestamp_ns_scale;
>  	struct hid_sensor_hub_attribute_info poll;
>  	struct hid_sensor_hub_attribute_info report_state;
>  	struct hid_sensor_hub_attribute_info power_state;
>  	struct hid_sensor_hub_attribute_info sensitivity;
> +	struct hid_sensor_hub_attribute_info report_latency;
>  	struct work_struct work;
>  };
>  
> @@ -276,5 +278,8 @@ s32 hid_sensor_read_poll_value(struct hid_sensor_common *st);
>  
>  int64_t hid_sensor_convert_timestamp(struct hid_sensor_common *st,
>  				     int64_t raw_value);
> +bool hid_sensor_batch_mode_supported(struct hid_sensor_common *st);
> +int hid_sensor_set_report_latency(struct hid_sensor_common *st, int latency);
> +int hid_sensor_get_report_latency(struct hid_sensor_common *st);
>  
>  #endif
> diff --git a/include/linux/hid-sensor-ids.h b/include/linux/hid-sensor-ids.h
> index 30c7dc4..d7caf8c 100644
> --- a/include/linux/hid-sensor-ids.h
> +++ b/include/linux/hid-sensor-ids.h
> @@ -142,6 +142,9 @@
>  #define HID_USAGE_SENSOR_PROP_REPORT_STATE			0x200316
>  #define HID_USAGE_SENSOR_PROY_POWER_STATE			0x200319
>  
> +/* Batch mode selectors */
> +#define HID_USAGE_SENSOR_PROP_REPORT_LATENCY			0x20031B
> +
>  /* Per data field properties */
>  #define HID_USAGE_SENSOR_DATA_MOD_NONE					0x00
>  #define HID_USAGE_SENSOR_DATA_MOD_CHANGE_SENSITIVITY_ABS		0x1000
> 

Re: [PATCH v2] iio: hid-sensor-hub: Implement batch mode

[Srinivas Pandruvada]

Hi Jonathan,

On Sat, 2017-04-08 at 15:42 +0100, Jonathan Cameron wrote:
> On 08/04/17 03:22, Srinivas Pandruvada wrote:
> > 
> > HID sensor hubs using Integrated Senor Hub (ISH) has added
> > capability to
> > support batch mode. This allows host processor to go to sleep for
> > extended
> > duration, while the sensor hub is storing samples in its internal
> > buffers.
> > 
> > 'Commit f4f4673b7535 ("iio: add support for hardware fifo")'
> > implements
> > feature in IIO core to implement such feature. This feature is used
> > in
> > bmc150-accel-core.c to implement batch mode. This implementation
> > allows
> > software device buffer watermark to be used as a hint to adjust
> > hardware
> > FIFO.
> > 
> > But HID sensor hubs don't allow to change internal buffer size of
> > FIFOs.
> > Instead an additional usage id to set "maximum report latency" is
> > defined.
> > This allows host to go to sleep upto this latency period without
> > getting
> > any report. Since there is no ABI to set this latency, a new
> > attribute
> > "hwfifo_timeout" is added so that user mode can specify a latency.
> > 
> > This change checks presence of usage id to get/set maximum report
> > latency
> > and if present, it will expose hwfifo_timeout.
> > 
> > Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel
> > .com>
> I'm happy with this. Spotted one typo inline.
> 
> However, it's run into the back of the two fixes you posted earlier
> so
> doesn't currently apply to the togreg branch. We'll have to let those
> unwind first and rebase as necessary.  Feel free to poke me if I
> seem to have forgotten this!
> 
> I guess this gives it a bit longer for others to comment which is
> perhaps
> no bad thing.

Looks like this didn't make to 4.12-rc1. Can we target for 4.13 then?

Thanks,
Srinivas

> 
> Thanks,
> 
> Jonathan
> > 
> > ---
> > v2:
> > - Fifo timeout can be in fractions also.
> > - Restore latency on resume from S3.
> > 
> > Changed compared to RFC:
> > - Removed min/max Fifo size attributes
> > - Removed flush callback. Since this usage id is not present in any
> > hubs,
> > we can add later when we have some HW to test. This was causing
> > some
> > ugliness.
> > - Moved the hwfifo_timout documentation to sysfs-bus-iio
> > 
> >  Documentation/ABI/testing/sysfs-bus-iio            | 11 +++
> >  .../iio/common/hid-sensors/hid-sensor-attributes.c | 44
> > ++++++++++++
> >  .../iio/common/hid-sensors/hid-sensor-trigger.c    | 80
> > ++++++++++++++++++++++
> >  include/linux/hid-sensor-hub.h                     |  5 ++
> >  include/linux/hid-sensor-ids.h                     |  3 +
> >  5 files changed, 143 insertions(+)
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-bus-iio
> > b/Documentation/ABI/testing/sysfs-bus-iio
> > index 530809c..b09f373 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-iio
> > +++ b/Documentation/ABI/testing/sysfs-bus-iio
> > @@ -1424,6 +1424,17 @@ Description:
> >  		guarantees that the hardware fifo is flushed to
> > the device
> >  		buffer.
> >  
> > +What:		/sys/bus/iio/devices/iio:device*/buffer/hwfif
> > o_timeout
> > +KernelVersion:	4.12
> > +Contact:	linux-iio@vger.kernel.org
> > +Description:
> > +		A read/write property to provide capability to
> > delay reporting of
> > +		samples till a timeout is reached. This allows
> > host processors to
> > +		sleep, while the sensor is storing samples in its
> > internal fifo.
> > +		The maximum timeout in seconds can be specified by
> > setting
> > +		hwfifo_timeout.The current delay can be read by
> > reading
> > +		hwfifo_timeout. A value of 0 means that there is
> > no timout.
> timeout. I'll fix.
> > 
> > +
> >  What:		/sys/bus/iio/devices/iio:deviceX/buffer/hwfif
> > o_watermark
> >  KernelVersion: 4.2
> >  Contact:	linux-iio@vger.kernel.org
> > diff --git a/drivers/iio/common/hid-sensors/hid-sensor-attributes.c 
> > b/drivers/iio/common/hid-sensors/hid-sensor-attributes.c
> > index efd3151..89c3f9c 100644
> > --- a/drivers/iio/common/hid-sensors/hid-sensor-attributes.c
> > +++ b/drivers/iio/common/hid-sensors/hid-sensor-attributes.c
> > @@ -393,6 +393,48 @@ int hid_sensor_get_reporting_interval(struct
> > hid_sensor_hub_device *hsdev,
> >  
> >  }
> >  
> > +static void hid_sensor_get_report_latency_info(struct
> > hid_sensor_hub_device *hsdev,
> > +					       u32 usage_id,
> > +					       struct
> > hid_sensor_common *st)
> > +{
> > +	sensor_hub_input_get_attribute_info(hsdev,
> > HID_FEATURE_REPORT,
> > +					    usage_id,
> > +					    HID_USAGE_SENSOR_PROP_
> > REPORT_LATENCY,
> > +					    &st->report_latency);
> > +
> > +	hid_dbg(hsdev->hdev, "Report latency attributes: %x:%x\n",
> > +		st->report_latency.index, st-
> > >report_latency.report_id);
> > +}
> > +
> > +int hid_sensor_get_report_latency(struct hid_sensor_common *st)
> > +{
> > +	int ret;
> > +	int value;
> > +
> > +	ret = sensor_hub_get_feature(st->hsdev, st-
> > >report_latency.report_id,
> > +				     st->report_latency.index,
> > sizeof(value),
> > +				     &value);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return value;
> > +}
> > +EXPORT_SYMBOL(hid_sensor_get_report_latency);
> > +
> > +int hid_sensor_set_report_latency(struct hid_sensor_common *st,
> > int latency_ms)
> > +{
> > +	return sensor_hub_set_feature(st->hsdev, st-
> > >report_latency.report_id,
> > +				      st->report_latency.index,
> > +				      sizeof(latency_ms),
> > &latency_ms);
> > +}
> > +EXPORT_SYMBOL(hid_sensor_set_report_latency);
> > +
> > +bool hid_sensor_batch_mode_supported(struct hid_sensor_common *st)
> > +{
> > +	return st->report_latency.index > 0 && st-
> > >report_latency.report_id > 0;
> > +}
> > +EXPORT_SYMBOL(hid_sensor_batch_mode_supported);
> > +
> >  int hid_sensor_parse_common_attributes(struct
> > hid_sensor_hub_device *hsdev,
> >  					u32 usage_id,
> >  					struct hid_sensor_common
> > *st)
> > @@ -432,6 +474,8 @@ int hid_sensor_parse_common_attributes(struct
> > hid_sensor_hub_device *hsdev,
> >  	} else
> >  		st->timestamp_ns_scale = 1000000000;
> >  
> > +	hid_sensor_get_report_latency_info(hsdev, usage_id, st);
> > +
> >  	hid_dbg(hsdev->hdev, "common attributes: %x:%x, %x:%x,
> > %x:%x %x:%x %x:%x\n",
> >  		st->poll.index, st->poll.report_id,
> >  		st->report_state.index, st-
> > >report_state.report_id,
> > diff --git a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> > b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> > index 0b5dea0..16ade0a 100644
> > --- a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> > +++ b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> > @@ -26,9 +26,84 @@
> >  #include <linux/hid-sensor-hub.h>
> >  #include <linux/iio/iio.h>
> >  #include <linux/iio/trigger.h>
> > +#include <linux/iio/buffer.h>
> >  #include <linux/iio/sysfs.h>
> >  #include "hid-sensor-trigger.h"
> >  
> > +static ssize_t _hid_sensor_set_report_latency(struct device *dev,
> > +					      struct
> > device_attribute *attr,
> > +					      const char *buf,
> > size_t len)
> > +{
> > +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > +	struct hid_sensor_common *attrb =
> > iio_device_get_drvdata(indio_dev);
> > +	int integer, fract, ret;
> > +	int latency;
> > +
> > +	ret = iio_str_to_fixpoint(buf, 100000, &integer, &fract);
> > +	if (ret)
> > +		return ret;
> > +
> > +	latency = integer * 1000 + fract / 1000;
> > +	ret = hid_sensor_set_report_latency(attrb, latency);
> > +	if (ret < 0)
> > +		return len;
> > +
> > +	attrb->latency_ms = hid_sensor_get_report_latency(attrb);
> > +
> > +	return len;
> > +}
> > +
> > +static ssize_t _hid_sensor_get_report_latency(struct device *dev,
> > +					      struct
> > device_attribute *attr,
> > +					      char *buf)
> > +{
> > +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > +	struct hid_sensor_common *attrb =
> > iio_device_get_drvdata(indio_dev);
> > +	int latency;
> > +
> > +	latency = hid_sensor_get_report_latency(attrb);
> > +	if (latency < 0)
> > +		return latency;
> > +
> > +	return sprintf(buf, "%d.%06u\n", latency / 1000, (latency
> > % 1000) * 1000);
> > +}
> > +
> > +static ssize_t _hid_sensor_get_fifo_state(struct device *dev,
> > +					  struct device_attribute
> > *attr,
> > +					  char *buf)
> > +{
> > +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > +	struct hid_sensor_common *attrb =
> > iio_device_get_drvdata(indio_dev);
> > +	int latency;
> > +
> > +	latency = hid_sensor_get_report_latency(attrb);
> > +	if (latency < 0)
> > +		return latency;
> > +
> > +	return sprintf(buf, "%d\n", !!latency);
> > +}
> > +
> > +static IIO_DEVICE_ATTR(hwfifo_timeout, 0644,
> > +		       _hid_sensor_get_report_latency,
> > +		       _hid_sensor_set_report_latency, 0);
> > +static IIO_DEVICE_ATTR(hwfifo_enabled, 0444,
> > +		       _hid_sensor_get_fifo_state, NULL, 0);
> > +
> > +static const struct attribute *hid_sensor_fifo_attributes[] = {
> > +	&iio_dev_attr_hwfifo_timeout.dev_attr.attr,
> > +	&iio_dev_attr_hwfifo_enabled.dev_attr.attr,
> > +	NULL,
> > +};
> > +
> > +static void hid_sensor_setup_batch_mode(struct iio_dev *indio_dev,
> > +					struct hid_sensor_common
> > *st)
> > +{
> > +	if (!hid_sensor_batch_mode_supported(st))
> > +		return;
> > +
> > +	iio_buffer_set_attrs(indio_dev->buffer,
> > hid_sensor_fifo_attributes);
> > +}
> > +
> >  static int _hid_sensor_power_state(struct hid_sensor_common *st,
> > bool state)
> >  {
> >  	int state_val;
> > @@ -141,6 +216,9 @@ static void hid_sensor_set_power_work(struct
> > work_struct *work)
> >  				       sizeof(attrb-
> > >raw_hystersis),
> >  				       &attrb->raw_hystersis);
> >  
> > +	if (attrb->latency_ms > 0)
> > +		hid_sensor_set_report_latency(attrb, attrb-
> > >latency_ms);
> > +
> >  	_hid_sensor_power_state(attrb, true);
> >  }
> >  
> > @@ -192,6 +270,8 @@ int hid_sensor_setup_trigger(struct iio_dev
> > *indio_dev, const char *name,
> >  	attrb->trigger = trig;
> >  	indio_dev->trig = iio_trigger_get(trig);
> >  
> > +	hid_sensor_setup_batch_mode(indio_dev, attrb);
> > +
> >  	ret = pm_runtime_set_active(&indio_dev->dev);
> >  	if (ret)
> >  		goto error_unreg_trigger;
> > diff --git a/include/linux/hid-sensor-hub.h b/include/linux/hid-
> > sensor-hub.h
> > index f32d7c3..fc7aae6 100644
> > --- a/include/linux/hid-sensor-hub.h
> > +++ b/include/linux/hid-sensor-hub.h
> > @@ -233,12 +233,14 @@ struct hid_sensor_common {
> >  	atomic_t user_requested_state;
> >  	int poll_interval;
> >  	int raw_hystersis;
> > +	int latency_ms;
> >  	struct iio_trigger *trigger;
> >  	int timestamp_ns_scale;
> >  	struct hid_sensor_hub_attribute_info poll;
> >  	struct hid_sensor_hub_attribute_info report_state;
> >  	struct hid_sensor_hub_attribute_info power_state;
> >  	struct hid_sensor_hub_attribute_info sensitivity;
> > +	struct hid_sensor_hub_attribute_info report_latency;
> >  	struct work_struct work;
> >  };
> >  
> > @@ -276,5 +278,8 @@ s32 hid_sensor_read_poll_value(struct
> > hid_sensor_common *st);
> >  
> >  int64_t hid_sensor_convert_timestamp(struct hid_sensor_common *st,
> >  				     int64_t raw_value);
> > +bool hid_sensor_batch_mode_supported(struct hid_sensor_common
> > *st);
> > +int hid_sensor_set_report_latency(struct hid_sensor_common *st,
> > int latency);
> > +int hid_sensor_get_report_latency(struct hid_sensor_common *st);
> >  
> >  #endif
> > diff --git a/include/linux/hid-sensor-ids.h b/include/linux/hid-
> > sensor-ids.h
> > index 30c7dc4..d7caf8c 100644
> > --- a/include/linux/hid-sensor-ids.h
> > +++ b/include/linux/hid-sensor-ids.h
> > @@ -142,6 +142,9 @@
> >  #define HID_USAGE_SENSOR_PROP_REPORT_STATE			
> > 0x200316
> >  #define HID_USAGE_SENSOR_PROY_POWER_STATE			0
> > x200319
> >  
> > +/* Batch mode selectors */
> > +#define HID_USAGE_SENSOR_PROP_REPORT_LATENCY			
> > 0x20031B
> > +
> >  /* Per data field properties */
> >  #define HID_USAGE_SENSOR_DATA_MOD_NONE				
> > 	0x00
> >  #define HID_USAGE_SENSOR_DATA_MOD_CHANGE_SENSITIVITY_ABS		
> > 0x1000
> > 
> 

Re: [PATCH v2] iio: hid-sensor-hub: Implement batch mode

[Jonathan Cameron]

On 15/05/17 22:16, Srinivas Pandruvada wrote:
> Hi Jonathan,
> 
> On Sat, 2017-04-08 at 15:42 +0100, Jonathan Cameron wrote:
>> On 08/04/17 03:22, Srinivas Pandruvada wrote:
>>>
>>> HID sensor hubs using Integrated Senor Hub (ISH) has added
>>> capability to
>>> support batch mode. This allows host processor to go to sleep for
>>> extended
>>> duration, while the sensor hub is storing samples in its internal
>>> buffers.
>>>
>>> 'Commit f4f4673b7535 ("iio: add support for hardware fifo")'
>>> implements
>>> feature in IIO core to implement such feature. This feature is used
>>> in
>>> bmc150-accel-core.c to implement batch mode. This implementation
>>> allows
>>> software device buffer watermark to be used as a hint to adjust
>>> hardware
>>> FIFO.
>>>
>>> But HID sensor hubs don't allow to change internal buffer size of
>>> FIFOs.
>>> Instead an additional usage id to set "maximum report latency" is
>>> defined.
>>> This allows host to go to sleep upto this latency period without
>>> getting
>>> any report. Since there is no ABI to set this latency, a new
>>> attribute
>>> "hwfifo_timeout" is added so that user mode can specify a latency.
>>>
>>> This change checks presence of usage id to get/set maximum report
>>> latency
>>> and if present, it will expose hwfifo_timeout.
>>>
>>> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel
>>> .com>
>> I'm happy with this. Spotted one typo inline.
>>
>> However, it's run into the back of the two fixes you posted earlier
>> so
>> doesn't currently apply to the togreg branch. We'll have to let those
>> unwind first and rebase as necessary.  Feel free to poke me if I
>> seem to have forgotten this!
>>
>> I guess this gives it a bit longer for others to comment which is
>> perhaps
>> no bad thing.
> 
> Looks like this didn't make to 4.12-rc1. Can we target for 4.13 then?
> 
Sorry. It's clear my old approach of just marking emails for things
I need to remember later is not working for stuff like this.  They end
up so far back I assume there is nothing of interest and don't look
there any more.

Will need to work out a new system!

Anyhow, applied to the togreg branch of iio.git and pushed out as
testing for the autobuilders to play with it.

Thanks,

Jonathan
> Thanks,
> Srinivas
> 
>>
>> Thanks,
>>
>> Jonathan
>>>
>>> ---
>>> v2:
>>> - Fifo timeout can be in fractions also.
>>> - Restore latency on resume from S3.
>>>
>>> Changed compared to RFC:
>>> - Removed min/max Fifo size attributes
>>> - Removed flush callback. Since this usage id is not present in any
>>> hubs,
>>> we can add later when we have some HW to test. This was causing
>>> some
>>> ugliness.
>>> - Moved the hwfifo_timout documentation to sysfs-bus-iio
>>>
>>>   Documentation/ABI/testing/sysfs-bus-iio            | 11 +++
>>>   .../iio/common/hid-sensors/hid-sensor-attributes.c | 44
>>> ++++++++++++
>>>   .../iio/common/hid-sensors/hid-sensor-trigger.c    | 80
>>> ++++++++++++++++++++++
>>>   include/linux/hid-sensor-hub.h                     |  5 ++
>>>   include/linux/hid-sensor-ids.h                     |  3 +
>>>   5 files changed, 143 insertions(+)
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-bus-iio
>>> b/Documentation/ABI/testing/sysfs-bus-iio
>>> index 530809c..b09f373 100644
>>> --- a/Documentation/ABI/testing/sysfs-bus-iio
>>> +++ b/Documentation/ABI/testing/sysfs-bus-iio
>>> @@ -1424,6 +1424,17 @@ Description:
>>>   		guarantees that the hardware fifo is flushed to
>>> the device
>>>   		buffer.
>>>   
>>> +What:		/sys/bus/iio/devices/iio:device*/buffer/hwfif
>>> o_timeout
>>> +KernelVersion:	4.12
>>> +Contact:	linux-iio@vger.kernel.org
>>> +Description:
>>> +		A read/write property to provide capability to
>>> delay reporting of
>>> +		samples till a timeout is reached. This allows
>>> host processors to
>>> +		sleep, while the sensor is storing samples in its
>>> internal fifo.
>>> +		The maximum timeout in seconds can be specified by
>>> setting
>>> +		hwfifo_timeout.The current delay can be read by
>>> reading
>>> +		hwfifo_timeout. A value of 0 means that there is
>>> no timout.
>> timeout. I'll fix.
>>>
>>> +
>>>   What:		/sys/bus/iio/devices/iio:deviceX/buffer/hwfif
>>> o_watermark
>>>   KernelVersion: 4.2
>>>   Contact:	linux-iio@vger.kernel.org
>>> diff --git a/drivers/iio/common/hid-sensors/hid-sensor-attributes.c
>>> b/drivers/iio/common/hid-sensors/hid-sensor-attributes.c
>>> index efd3151..89c3f9c 100644
>>> --- a/drivers/iio/common/hid-sensors/hid-sensor-attributes.c
>>> +++ b/drivers/iio/common/hid-sensors/hid-sensor-attributes.c
>>> @@ -393,6 +393,48 @@ int hid_sensor_get_reporting_interval(struct
>>> hid_sensor_hub_device *hsdev,
>>>   
>>>   }
>>>   
>>> +static void hid_sensor_get_report_latency_info(struct
>>> hid_sensor_hub_device *hsdev,
>>> +					       u32 usage_id,
>>> +					       struct
>>> hid_sensor_common *st)
>>> +{
>>> +	sensor_hub_input_get_attribute_info(hsdev,
>>> HID_FEATURE_REPORT,
>>> +					    usage_id,
>>> +					    HID_USAGE_SENSOR_PROP_
>>> REPORT_LATENCY,
>>> +					    &st->report_latency);
>>> +
>>> +	hid_dbg(hsdev->hdev, "Report latency attributes: %x:%x\n",
>>> +		st->report_latency.index, st-
>>>> report_latency.report_id);
>>> +}
>>> +
>>> +int hid_sensor_get_report_latency(struct hid_sensor_common *st)
>>> +{
>>> +	int ret;
>>> +	int value;
>>> +
>>> +	ret = sensor_hub_get_feature(st->hsdev, st-
>>>> report_latency.report_id,
>>> +				     st->report_latency.index,
>>> sizeof(value),
>>> +				     &value);
>>> +	if (ret < 0)
>>> +		return ret;
>>> +
>>> +	return value;
>>> +}
>>> +EXPORT_SYMBOL(hid_sensor_get_report_latency);
>>> +
>>> +int hid_sensor_set_report_latency(struct hid_sensor_common *st,
>>> int latency_ms)
>>> +{
>>> +	return sensor_hub_set_feature(st->hsdev, st-
>>>> report_latency.report_id,
>>> +				      st->report_latency.index,
>>> +				      sizeof(latency_ms),
>>> &latency_ms);
>>> +}
>>> +EXPORT_SYMBOL(hid_sensor_set_report_latency);
>>> +
>>> +bool hid_sensor_batch_mode_supported(struct hid_sensor_common *st)
>>> +{
>>> +	return st->report_latency.index > 0 && st-
>>>> report_latency.report_id > 0;
>>> +}
>>> +EXPORT_SYMBOL(hid_sensor_batch_mode_supported);
>>> +
>>>   int hid_sensor_parse_common_attributes(struct
>>> hid_sensor_hub_device *hsdev,
>>>   					u32 usage_id,
>>>   					struct hid_sensor_common
>>> *st)
>>> @@ -432,6 +474,8 @@ int hid_sensor_parse_common_attributes(struct
>>> hid_sensor_hub_device *hsdev,
>>>   	} else
>>>   		st->timestamp_ns_scale = 1000000000;
>>>   
>>> +	hid_sensor_get_report_latency_info(hsdev, usage_id, st);
>>> +
>>>   	hid_dbg(hsdev->hdev, "common attributes: %x:%x, %x:%x,
>>> %x:%x %x:%x %x:%x\n",
>>>   		st->poll.index, st->poll.report_id,
>>>   		st->report_state.index, st-
>>>> report_state.report_id,
>>> diff --git a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
>>> b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
>>> index 0b5dea0..16ade0a 100644
>>> --- a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
>>> +++ b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
>>> @@ -26,9 +26,84 @@
>>>   #include <linux/hid-sensor-hub.h>
>>>   #include <linux/iio/iio.h>
>>>   #include <linux/iio/trigger.h>
>>> +#include <linux/iio/buffer.h>
>>>   #include <linux/iio/sysfs.h>
>>>   #include "hid-sensor-trigger.h"
>>>   
>>> +static ssize_t _hid_sensor_set_report_latency(struct device *dev,
>>> +					      struct
>>> device_attribute *attr,
>>> +					      const char *buf,
>>> size_t len)
>>> +{
>>> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>>> +	struct hid_sensor_common *attrb =
>>> iio_device_get_drvdata(indio_dev);
>>> +	int integer, fract, ret;
>>> +	int latency;
>>> +
>>> +	ret = iio_str_to_fixpoint(buf, 100000, &integer, &fract);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	latency = integer * 1000 + fract / 1000;
>>> +	ret = hid_sensor_set_report_latency(attrb, latency);
>>> +	if (ret < 0)
>>> +		return len;
>>> +
>>> +	attrb->latency_ms = hid_sensor_get_report_latency(attrb);
>>> +
>>> +	return len;
>>> +}
>>> +
>>> +static ssize_t _hid_sensor_get_report_latency(struct device *dev,
>>> +					      struct
>>> device_attribute *attr,
>>> +					      char *buf)
>>> +{
>>> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>>> +	struct hid_sensor_common *attrb =
>>> iio_device_get_drvdata(indio_dev);
>>> +	int latency;
>>> +
>>> +	latency = hid_sensor_get_report_latency(attrb);
>>> +	if (latency < 0)
>>> +		return latency;
>>> +
>>> +	return sprintf(buf, "%d.%06u\n", latency / 1000, (latency
>>> % 1000) * 1000);
>>> +}
>>> +
>>> +static ssize_t _hid_sensor_get_fifo_state(struct device *dev,
>>> +					  struct device_attribute
>>> *attr,
>>> +					  char *buf)
>>> +{
>>> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>>> +	struct hid_sensor_common *attrb =
>>> iio_device_get_drvdata(indio_dev);
>>> +	int latency;
>>> +
>>> +	latency = hid_sensor_get_report_latency(attrb);
>>> +	if (latency < 0)
>>> +		return latency;
>>> +
>>> +	return sprintf(buf, "%d\n", !!latency);
>>> +}
>>> +
>>> +static IIO_DEVICE_ATTR(hwfifo_timeout, 0644,
>>> +		       _hid_sensor_get_report_latency,
>>> +		       _hid_sensor_set_report_latency, 0);
>>> +static IIO_DEVICE_ATTR(hwfifo_enabled, 0444,
>>> +		       _hid_sensor_get_fifo_state, NULL, 0);
>>> +
>>> +static const struct attribute *hid_sensor_fifo_attributes[] = {
>>> +	&iio_dev_attr_hwfifo_timeout.dev_attr.attr,
>>> +	&iio_dev_attr_hwfifo_enabled.dev_attr.attr,
>>> +	NULL,
>>> +};
>>> +
>>> +static void hid_sensor_setup_batch_mode(struct iio_dev *indio_dev,
>>> +					struct hid_sensor_common
>>> *st)
>>> +{
>>> +	if (!hid_sensor_batch_mode_supported(st))
>>> +		return;
>>> +
>>> +	iio_buffer_set_attrs(indio_dev->buffer,
>>> hid_sensor_fifo_attributes);
>>> +}
>>> +
>>>   static int _hid_sensor_power_state(struct hid_sensor_common *st,
>>> bool state)
>>>   {
>>>   	int state_val;
>>> @@ -141,6 +216,9 @@ static void hid_sensor_set_power_work(struct
>>> work_struct *work)
>>>   				       sizeof(attrb-
>>>> raw_hystersis),
>>>   				       &attrb->raw_hystersis);
>>>   
>>> +	if (attrb->latency_ms > 0)
>>> +		hid_sensor_set_report_latency(attrb, attrb-
>>>> latency_ms);
>>> +
>>>   	_hid_sensor_power_state(attrb, true);
>>>   }
>>>   
>>> @@ -192,6 +270,8 @@ int hid_sensor_setup_trigger(struct iio_dev
>>> *indio_dev, const char *name,
>>>   	attrb->trigger = trig;
>>>   	indio_dev->trig = iio_trigger_get(trig);
>>>   
>>> +	hid_sensor_setup_batch_mode(indio_dev, attrb);
>>> +
>>>   	ret = pm_runtime_set_active(&indio_dev->dev);
>>>   	if (ret)
>>>   		goto error_unreg_trigger;
>>> diff --git a/include/linux/hid-sensor-hub.h b/include/linux/hid-
>>> sensor-hub.h
>>> index f32d7c3..fc7aae6 100644
>>> --- a/include/linux/hid-sensor-hub.h
>>> +++ b/include/linux/hid-sensor-hub.h
>>> @@ -233,12 +233,14 @@ struct hid_sensor_common {
>>>   	atomic_t user_requested_state;
>>>   	int poll_interval;
>>>   	int raw_hystersis;
>>> +	int latency_ms;
>>>   	struct iio_trigger *trigger;
>>>   	int timestamp_ns_scale;
>>>   	struct hid_sensor_hub_attribute_info poll;
>>>   	struct hid_sensor_hub_attribute_info report_state;
>>>   	struct hid_sensor_hub_attribute_info power_state;
>>>   	struct hid_sensor_hub_attribute_info sensitivity;
>>> +	struct hid_sensor_hub_attribute_info report_latency;
>>>   	struct work_struct work;
>>>   };
>>>   
>>> @@ -276,5 +278,8 @@ s32 hid_sensor_read_poll_value(struct
>>> hid_sensor_common *st);
>>>   
>>>   int64_t hid_sensor_convert_timestamp(struct hid_sensor_common *st,
>>>   				     int64_t raw_value);
>>> +bool hid_sensor_batch_mode_supported(struct hid_sensor_common
>>> *st);
>>> +int hid_sensor_set_report_latency(struct hid_sensor_common *st,
>>> int latency);
>>> +int hid_sensor_get_report_latency(struct hid_sensor_common *st);
>>>   
>>>   #endif
>>> diff --git a/include/linux/hid-sensor-ids.h b/include/linux/hid-
>>> sensor-ids.h
>>> index 30c7dc4..d7caf8c 100644
>>> --- a/include/linux/hid-sensor-ids.h
>>> +++ b/include/linux/hid-sensor-ids.h
>>> @@ -142,6 +142,9 @@
>>>   #define HID_USAGE_SENSOR_PROP_REPORT_STATE			
>>> 0x200316
>>>   #define HID_USAGE_SENSOR_PROY_POWER_STATE			0
>>> x200319
>>>   
>>> +/* Batch mode selectors */
>>> +#define HID_USAGE_SENSOR_PROP_REPORT_LATENCY			
>>> 0x20031B
>>> +
>>>   /* Per data field properties */
>>>   #define HID_USAGE_SENSOR_DATA_MOD_NONE				
>>> 	0x00
>>>   #define HID_USAGE_SENSOR_DATA_MOD_CHANGE_SENSITIVITY_ABS		
>>> 0x1000
>>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v2] iio: hid-sensor-hub: Implement batch mode

[Srinivas Pandruvada]

On Tue, 2017-05-16 at 19:45 +0100, Jonathan Cameron wrote:
[...]

> Looks like this didn't make to 4.12-rc1. Can we target for 4.13
> > then?
> > 
> Sorry. It's clear my old approach of just marking emails for things
> I need to remember later is not working for stuff like this.  They
> end
> up so far back I assume there is nothing of interest and don't look
> there any more.
No problem.

> 
> Will need to work out a new system!
> 
> Anyhow, applied to the togreg branch of iio.git and pushed out as
> testing for the autobuilders to play with it.
> 
Thanks for your help.

- Srinivas

> Thanks,
> 
> Jonathan
> > 
> > Thanks,
> > Srinivas
> > 
> > > 
> > > 
> > > Thanks,
> > > 
> > > Jonathan
> > > > 
> > > > 
> > > > ---
> > > > v2:
> > > > - Fifo timeout can be in fractions also.
> > > > - Restore latency on resume from S3.
> > > > 
> > > > Changed compared to RFC:
> > > > - Removed min/max Fifo size attributes
> > > > - Removed flush callback. Since this usage id is not present in
> > > > any
> > > > hubs,
> > > > we can add later when we have some HW to test. This was causing
> > > > some
> > > > ugliness.
> > > > - Moved the hwfifo_timout documentation to sysfs-bus-iio
> > > > 
> > > >   Documentation/ABI/testing/sysfs-bus-iio            | 11 +++
> > > >   .../iio/common/hid-sensors/hid-sensor-attributes.c | 44
> > > > ++++++++++++
> > > >   .../iio/common/hid-sensors/hid-sensor-trigger.c    | 80
> > > > ++++++++++++++++++++++
> > > >   include/linux/hid-sensor-hub.h                     |  5 ++
> > > >   include/linux/hid-sensor-ids.h                     |  3 +
> > > >   5 files changed, 143 insertions(+)
> > > > 
> > > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio
> > > > b/Documentation/ABI/testing/sysfs-bus-iio
> > > > index 530809c..b09f373 100644
> > > > --- a/Documentation/ABI/testing/sysfs-bus-iio
> > > > +++ b/Documentation/ABI/testing/sysfs-bus-iio
> > > > @@ -1424,6 +1424,17 @@ Description:
> > > >   		guarantees that the hardware fifo is flushed
> > > > to
> > > > the device
> > > >   		buffer.
> > > >   
> > > > +What:		/sys/bus/iio/devices/iio:device*/buffer/h
> > > > wfif
> > > > o_timeout
> > > > +KernelVersion:	4.12
> > > > +Contact:	linux-iio@vger.kernel.org
> > > > +Description:
> > > > +		A read/write property to provide capability to
> > > > delay reporting of
> > > > +		samples till a timeout is reached. This allows
> > > > host processors to
> > > > +		sleep, while the sensor is storing samples in
> > > > its
> > > > internal fifo.
> > > > +		The maximum timeout in seconds can be
> > > > specified by
> > > > setting
> > > > +		hwfifo_timeout.The current delay can be read
> > > > by
> > > > reading
> > > > +		hwfifo_timeout. A value of 0 means that there
> > > > is
> > > > no timout.
> > > timeout. I'll fix.
> > > > 
> > > > 
> > > > +
> > > >   What:		/sys/bus/iio/devices/iio:deviceX/buffer/
> > > > hwfif
> > > > o_watermark
> > > >   KernelVersion: 4.2
> > > >   Contact:	linux-iio@vger.kernel.org
> > > > diff --git a/drivers/iio/common/hid-sensors/hid-sensor-
> > > > attributes.c
> > > > b/drivers/iio/common/hid-sensors/hid-sensor-attributes.c
> > > > index efd3151..89c3f9c 100644
> > > > --- a/drivers/iio/common/hid-sensors/hid-sensor-attributes.c
> > > > +++ b/drivers/iio/common/hid-sensors/hid-sensor-attributes.c
> > > > @@ -393,6 +393,48 @@ int
> > > > hid_sensor_get_reporting_interval(struct
> > > > hid_sensor_hub_device *hsdev,
> > > >   
> > > >   }
> > > >   
> > > > +static void hid_sensor_get_report_latency_info(struct
> > > > hid_sensor_hub_device *hsdev,
> > > > +					       u32 usage_id,
> > > > +					       struct
> > > > hid_sensor_common *st)
> > > > +{
> > > > +	sensor_hub_input_get_attribute_info(hsdev,
> > > > HID_FEATURE_REPORT,
> > > > +					    usage_id,
> > > > +					    HID_USAGE_SENSOR_P
> > > > ROP_
> > > > REPORT_LATENCY,
> > > > +					    &st-
> > > > >report_latency);
> > > > +
> > > > +	hid_dbg(hsdev->hdev, "Report latency attributes:
> > > > %x:%x\n",
> > > > +		st->report_latency.index, st-
> > > > > 
> > > > > report_latency.report_id);
> > > > +}
> > > > +
> > > > +int hid_sensor_get_report_latency(struct hid_sensor_common
> > > > *st)
> > > > +{
> > > > +	int ret;
> > > > +	int value;
> > > > +
> > > > +	ret = sensor_hub_get_feature(st->hsdev, st-
> > > > > 
> > > > > report_latency.report_id,
> > > > +				     st->report_latency.index,
> > > > sizeof(value),
> > > > +				     &value);
> > > > +	if (ret < 0)
> > > > +		return ret;
> > > > +
> > > > +	return value;
> > > > +}
> > > > +EXPORT_SYMBOL(hid_sensor_get_report_latency);
> > > > +
> > > > +int hid_sensor_set_report_latency(struct hid_sensor_common
> > > > *st,
> > > > int latency_ms)
> > > > +{
> > > > +	return sensor_hub_set_feature(st->hsdev, st-
> > > > > 
> > > > > report_latency.report_id,
> > > > +				      st-
> > > > >report_latency.index,
> > > > +				      sizeof(latency_ms),
> > > > &latency_ms);
> > > > +}
> > > > +EXPORT_SYMBOL(hid_sensor_set_report_latency);
> > > > +
> > > > +bool hid_sensor_batch_mode_supported(struct hid_sensor_common
> > > > *st)
> > > > +{
> > > > +	return st->report_latency.index > 0 && st-
> > > > > 
> > > > > report_latency.report_id > 0;
> > > > +}
> > > > +EXPORT_SYMBOL(hid_sensor_batch_mode_supported);
> > > > +
> > > >   int hid_sensor_parse_common_attributes(struct
> > > > hid_sensor_hub_device *hsdev,
> > > >   					u32 usage_id,
> > > >   					struct
> > > > hid_sensor_common
> > > > *st)
> > > > @@ -432,6 +474,8 @@ int
> > > > hid_sensor_parse_common_attributes(struct
> > > > hid_sensor_hub_device *hsdev,
> > > >   	} else
> > > >   		st->timestamp_ns_scale = 1000000000;
> > > >   
> > > > +	hid_sensor_get_report_latency_info(hsdev, usage_id,
> > > > st);
> > > > +
> > > >   	hid_dbg(hsdev->hdev, "common attributes: %x:%x,
> > > > %x:%x,
> > > > %x:%x %x:%x %x:%x\n",
> > > >   		st->poll.index, st->poll.report_id,
> > > >   		st->report_state.index, st-
> > > > > 
> > > > > report_state.report_id,
> > > > diff --git a/drivers/iio/common/hid-sensors/hid-sensor-
> > > > trigger.c
> > > > b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> > > > index 0b5dea0..16ade0a 100644
> > > > --- a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> > > > +++ b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> > > > @@ -26,9 +26,84 @@
> > > >   #include <linux/hid-sensor-hub.h>
> > > >   #include <linux/iio/iio.h>
> > > >   #include <linux/iio/trigger.h>
> > > > +#include <linux/iio/buffer.h>
> > > >   #include <linux/iio/sysfs.h>
> > > >   #include "hid-sensor-trigger.h"
> > > >   
> > > > +static ssize_t _hid_sensor_set_report_latency(struct device
> > > > *dev,
> > > > +					      struct
> > > > device_attribute *attr,
> > > > +					      const char *buf,
> > > > size_t len)
> > > > +{
> > > > +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > > > +	struct hid_sensor_common *attrb =
> > > > iio_device_get_drvdata(indio_dev);
> > > > +	int integer, fract, ret;
> > > > +	int latency;
> > > > +
> > > > +	ret = iio_str_to_fixpoint(buf, 100000, &integer,
> > > > &fract);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	latency = integer * 1000 + fract / 1000;
> > > > +	ret = hid_sensor_set_report_latency(attrb, latency);
> > > > +	if (ret < 0)
> > > > +		return len;
> > > > +
> > > > +	attrb->latency_ms =
> > > > hid_sensor_get_report_latency(attrb);
> > > > +
> > > > +	return len;
> > > > +}
> > > > +
> > > > +static ssize_t _hid_sensor_get_report_latency(struct device
> > > > *dev,
> > > > +					      struct
> > > > device_attribute *attr,
> > > > +					      char *buf)
> > > > +{
> > > > +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > > > +	struct hid_sensor_common *attrb =
> > > > iio_device_get_drvdata(indio_dev);
> > > > +	int latency;
> > > > +
> > > > +	latency = hid_sensor_get_report_latency(attrb);
> > > > +	if (latency < 0)
> > > > +		return latency;
> > > > +
> > > > +	return sprintf(buf, "%d.%06u\n", latency / 1000,
> > > > (latency
> > > > % 1000) * 1000);
> > > > +}
> > > > +
> > > > +static ssize_t _hid_sensor_get_fifo_state(struct device *dev,
> > > > +					  struct
> > > > device_attribute
> > > > *attr,
> > > > +					  char *buf)
> > > > +{
> > > > +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > > > +	struct hid_sensor_common *attrb =
> > > > iio_device_get_drvdata(indio_dev);
> > > > +	int latency;
> > > > +
> > > > +	latency = hid_sensor_get_report_latency(attrb);
> > > > +	if (latency < 0)
> > > > +		return latency;
> > > > +
> > > > +	return sprintf(buf, "%d\n", !!latency);
> > > > +}
> > > > +
> > > > +static IIO_DEVICE_ATTR(hwfifo_timeout, 0644,
> > > > +		       _hid_sensor_get_report_latency,
> > > > +		       _hid_sensor_set_report_latency, 0);
> > > > +static IIO_DEVICE_ATTR(hwfifo_enabled, 0444,
> > > > +		       _hid_sensor_get_fifo_state, NULL, 0);
> > > > +
> > > > +static const struct attribute *hid_sensor_fifo_attributes[] =
> > > > {
> > > > +	&iio_dev_attr_hwfifo_timeout.dev_attr.attr,
> > > > +	&iio_dev_attr_hwfifo_enabled.dev_attr.attr,
> > > > +	NULL,
> > > > +};
> > > > +
> > > > +static void hid_sensor_setup_batch_mode(struct iio_dev
> > > > *indio_dev,
> > > > +					struct
> > > > hid_sensor_common
> > > > *st)
> > > > +{
> > > > +	if (!hid_sensor_batch_mode_supported(st))
> > > > +		return;
> > > > +
> > > > +	iio_buffer_set_attrs(indio_dev->buffer,
> > > > hid_sensor_fifo_attributes);
> > > > +}
> > > > +
> > > >   static int _hid_sensor_power_state(struct hid_sensor_common
> > > > *st,
> > > > bool state)
> > > >   {
> > > >   	int state_val;
> > > > @@ -141,6 +216,9 @@ static void
> > > > hid_sensor_set_power_work(struct
> > > > work_struct *work)
> > > >   				       sizeof(attrb-
> > > > > 
> > > > > raw_hystersis),
> > > >   				       &attrb-
> > > > >raw_hystersis);
> > > >   
> > > > +	if (attrb->latency_ms > 0)
> > > > +		hid_sensor_set_report_latency(attrb, attrb-
> > > > > 
> > > > > latency_ms);
> > > > +
> > > >   	_hid_sensor_power_state(attrb, true);
> > > >   }
> > > >   
> > > > @@ -192,6 +270,8 @@ int hid_sensor_setup_trigger(struct iio_dev
> > > > *indio_dev, const char *name,
> > > >   	attrb->trigger = trig;
> > > >   	indio_dev->trig = iio_trigger_get(trig);
> > > >   
> > > > +	hid_sensor_setup_batch_mode(indio_dev, attrb);
> > > > +
> > > >   	ret = pm_runtime_set_active(&indio_dev->dev);
> > > >   	if (ret)
> > > >   		goto error_unreg_trigger;
> > > > diff --git a/include/linux/hid-sensor-hub.h
> > > > b/include/linux/hid-
> > > > sensor-hub.h
> > > > index f32d7c3..fc7aae6 100644
> > > > --- a/include/linux/hid-sensor-hub.h
> > > > +++ b/include/linux/hid-sensor-hub.h
> > > > @@ -233,12 +233,14 @@ struct hid_sensor_common {
> > > >   	atomic_t user_requested_state;
> > > >   	int poll_interval;
> > > >   	int raw_hystersis;
> > > > +	int latency_ms;
> > > >   	struct iio_trigger *trigger;
> > > >   	int timestamp_ns_scale;
> > > >   	struct hid_sensor_hub_attribute_info poll;
> > > >   	struct hid_sensor_hub_attribute_info report_state;
> > > >   	struct hid_sensor_hub_attribute_info power_state;
> > > >   	struct hid_sensor_hub_attribute_info sensitivity;
> > > > +	struct hid_sensor_hub_attribute_info report_latency;
> > > >   	struct work_struct work;
> > > >   };
> > > >   
> > > > @@ -276,5 +278,8 @@ s32 hid_sensor_read_poll_value(struct
> > > > hid_sensor_common *st);
> > > >   
> > > >   int64_t hid_sensor_convert_timestamp(struct hid_sensor_common
> > > > *st,
> > > >   				     int64_t raw_value);
> > > > +bool hid_sensor_batch_mode_supported(struct hid_sensor_common
> > > > *st);
> > > > +int hid_sensor_set_report_latency(struct hid_sensor_common
> > > > *st,
> > > > int latency);
> > > > +int hid_sensor_get_report_latency(struct hid_sensor_common
> > > > *st);
> > > >   
> > > >   #endif
> > > > diff --git a/include/linux/hid-sensor-ids.h
> > > > b/include/linux/hid-
> > > > sensor-ids.h
> > > > index 30c7dc4..d7caf8c 100644
> > > > --- a/include/linux/hid-sensor-ids.h
> > > > +++ b/include/linux/hid-sensor-ids.h
> > > > @@ -142,6 +142,9 @@
> > > >   #define HID_USAGE_SENSOR_PROP_REPORT_STATE			
> > > > 0x200316
> > > >   #define HID_USAGE_SENSOR_PROY_POWER_STATE			
> > > > 0
> > > > x200319
> > > >   
> > > > +/* Batch mode selectors */
> > > > +#define HID_USAGE_SENSOR_PROP_REPORT_LATENCY			
> > > > 0x20031B
> > > > +
> > > >   /* Per data field properties */
> > > >   #define HID_USAGE_SENSOR_DATA_MOD_NONE			
> > > > 	
> > > > 	0x00
> > > >   #define HID_USAGE_SENSOR_DATA_MOD_CHANGE_SENSITIVITY_ABS	
> > > > 	
> > > > 0x1000
> > > > 
> > > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-
> > iio" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
>