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

[Jonathan Cameron <jic23@xxxxxxxxxx>]

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@xxxxxxxxxxxxxxx
> > > +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@xxxxxxxxxxxxxxx
> > > 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@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html