[RFC 0/8] iio: add support for hardware fifo

[RFC 0/8] iio: add support for hardware fifo

[Octavian Purdila]

Hi everybody,

I hope this RFC is a good starting point to discuss support for
hardware fifo in IIO. The main reason to support it is to reduce the
power consumtion, by allowing the CPU to enter deep sleep states for
longer periods of time.

Don't get discourage by the large number of patches most of them are
refactors in the bmc150 driver, to make it easier to add support for
the hardware fifo (basically to make adding interrupts and
events/triggers easier).

For discussing the hardware fifo stuff, only the first and last
patches are important: the first adds new IIO attributes so that we
can expose the hardware fifo and the last implements hadware fifo for
IIO (as an example of how would a device use the exposed attributes).

Note that the attributes can be exposed on a per device or per channel
basis, since it seems both types of hardware fifos exists: those that
store all data in a single fifo (temperature, accelerometer,
magnetometer, etc.) and those that have separate fifos for
accelerometer, gyroscope, etc. Thankfully, at the driver level we just
need to use the appropriate sharing level to support one mode or the
other.

Also note that this patch is orthogonal to the software watermark /
batching patch send on the list a while back.

Octavian Purdila (8):
  iio: add support for hardware fifo
  iio: bmc150: refactor slope duration and threshold update
  iio: bmc150: refactor interrupt enabling
  iio: bmc150: exit early if event / trigger state is not changed
  iio: bmc150: introduce bmc150_accel_interrupt
  iio: bmc150: introduce bmc150_accel_trigger
  iio: bmc150: introduce bmc150_accel_event
  iio: bmc150: add support for hardware fifo

 Documentation/ABI/testing/sysfs-bus-iio |  51 ++
 drivers/iio/accel/bmc150-accel.c        | 976 ++++++++++++++++++++++----------
 drivers/iio/industrialio-core.c         |   2 +
 drivers/iio/industrialio-event.c        |   2 +
 include/linux/iio/iio.h                 |  17 +
 include/linux/iio/types.h               |   2 +
 6 files changed, 739 insertions(+), 311 deletions(-)

-- 
1.9.1

[RFC 1/8] iio: add support for hardware fifo

[Octavian Purdila]

Some devices have hardware buffers that can store a number of samples
for later consumption. Hardware usually provides interrupts to notify
the processor when the fifo is full or when it has reached a certain
threshold.

Some devices also offer flexibility in what they do when the hardware
fifo is full: either discard future samples, or overwrite older
samples with newer ones.

The hardware fifo is useful for reducing the number of interrupts to
the host processor and thus it helps decreasing the power consumption.

This patch adds four new IIO attributes to expose the hardware fifo
and it's configuration to userspace:

 * _fifo_length - the number of samples that the hardware can store;
   it is usefull to expose it to userspace, so that applications can
   increase the device buffer size appropriately

 * _fifo_mode - the mode the hardware fifo operates

 * _fifo_mode_available - the available hardware fifo modes

 * _fifo_flush - this attribute can be used by userspace to trigger a
   fifo flush operation, where all data stored in the fifo will be
   made available to the device buffer

Since the last two attributes work with strings, they are using the
iio_chan_spec_ext_info infrastructure. To simplify the setup for a
hardware fifo the IIO_FIFO_EXT_INFO marcro can be used to initialize
them.

This patch also add two new events: IIO_EV_TYPE_FIFO_FULL and
IIO_EV_TYPE_FIFO_WATERMAK. If enabled they must be issued by the
device when the fifo level reaches full or, respectively, the
watermark level. The watermark level can be set by configuring the
event value.

Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
---
 Documentation/ABI/testing/sysfs-bus-iio | 51 +++++++++++++++++++++++++++++++++
 drivers/iio/industrialio-core.c         |  2 ++
 drivers/iio/industrialio-event.c        |  2 ++
 include/linux/iio/iio.h                 | 17 +++++++++++
 include/linux/iio/types.h               |  2 ++
 5 files changed, 74 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
index ea7e7ab..f326de1 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -1050,3 +1050,54 @@ Description:
 		after application of scale and offset. If no offset or scale is
 		present, output should be considered as processed with the
 		unit in milliamps.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_fifo_length
+What:		/sys/bus/iio/devices/iio:deviceX/in_accel_fifo_length
+What:		/sys/bus/iio/devices/iio:deviceX/in_magn_fifo_length
+What:		/sys/bus/iio/devices/iio:deviceX/in_anglvel_fifo_length
+KernelVersion:	3.19
+Contact:	linux-iio@vger.kernel.org
+Description:
+		The maximum number of samples that the hardware fifo can
+		store.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_fifo_mode
+What:		/sys/bus/iio/devices/iio:deviceX/in_accel_fifo_mode
+What:		/sys/bus/iio/devices/iio:deviceX/in_magn_fifo_mode
+What:		/sys/bus/iio/devices/iio:deviceX/in_anglvel_fifo_mode
+KernelVersion:	3.19
+Contact:	linux-iio@vger.kernel.org
+Description:
+		The mode in which the hardware fifo currently operates. The
+		supported values are "disabled", "fifo" and "stream".
+		In disabled mode the hardware fifo is not operational.
+		In fifo mode the samples are stored until the fifo is full at
+		which point all subsequent samples are discarded.
+		In stream mode, when the fifo is full new samples start to
+		overrride old samples.
+		If the hardware supports it, IIO_EVENT_TYPE_FIFO_FULL and
+		IIO_EVENT_TYPE_FIFO_WATERMARK events will be generated when they
+		are enabled by the application and the fifo is full and,
+		respectively, when the fifo level reaches the watermark
+		level.
+		The watermark level can be set by the application by configuring
+		the IIO_EVENT_TYPE_FIFO_WATERMARK value (IIO_EV_INFO_VALUE).
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_fifo_mode_available
+What:		/sys/bus/iio/devices/iio:deviceX/in_accel_fifo_mode_available
+What:		/sys/bus/iio/devices/iio:deviceX/in_magn_fifo_mode_available
+What:		/sys/bus/iio/devices/iio:deviceX/in_anglvel_fifo_mode_available
+KernelVersion:	3.19
+Contact:	linux-iio@vger.kernel.org
+Description:
+		The modes that the hardware fifo supports.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_fifo_flush
+What:		/sys/bus/iio/devices/iio:deviceX/in_accel_fifo_flush
+What:		/sys/bus/iio/devices/iio:deviceX/in_magn_fifo_flush
+What:		/sys/bus/iio/devices/iio:deviceX/in_anglvel_fifo_flush
+KernelVersion:	3.19
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Writting to this entry will cause the device to flush the data
+		from the hardware fifo into the device buffer.
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index af3e76d..17e84c3 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -113,6 +113,8 @@ static const char * const iio_chan_info_postfix[] = {
 	[IIO_CHAN_INFO_HARDWAREGAIN] = "hardwaregain",
 	[IIO_CHAN_INFO_HYSTERESIS] = "hysteresis",
 	[IIO_CHAN_INFO_INT_TIME] = "integration_time",
+	[IIO_CHAN_INFO_FIFO_LENGTH] = "fifo_length",
+	[IIO_CHAN_INFO_FIFO_FLUSH] = "fifo_flush",
 };
 
 /**
diff --git a/drivers/iio/industrialio-event.c b/drivers/iio/industrialio-event.c
index 0c1e37e..3d73fd0 100644
--- a/drivers/iio/industrialio-event.c
+++ b/drivers/iio/industrialio-event.c
@@ -197,6 +197,8 @@ static const char * const iio_ev_type_text[] = {
 	[IIO_EV_TYPE_ROC] = "roc",
 	[IIO_EV_TYPE_THRESH_ADAPTIVE] = "thresh_adaptive",
 	[IIO_EV_TYPE_MAG_ADAPTIVE] = "mag_adaptive",
+	[IIO_EV_TYPE_FIFO_FULL] = "fifo_full",
+	[IIO_EV_TYPE_FIFO_WATERMARK] = "fifo_watermak",
 };
 
 static const char * const iio_ev_dir_text[] = {
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index 3642ce7..ad100a1 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -38,6 +38,8 @@ enum iio_chan_info_enum {
 	IIO_CHAN_INFO_HARDWAREGAIN,
 	IIO_CHAN_INFO_HYSTERESIS,
 	IIO_CHAN_INFO_INT_TIME,
+	IIO_CHAN_INFO_FIFO_LENGTH,
+	IIO_CHAN_INFO_FIFO_FLUSH,
 };
 
 enum iio_shared_by {
@@ -629,4 +631,19 @@ int iio_str_to_fixpoint(const char *str, int fract_mult, int *integer,
  */
 #define IIO_G_TO_M_S_2(g) ((g) * 980665ULL / 100000ULL)
 
+/**
+ * IIO_FIFO_EXT_INFO() - define a hardware fifo associated with a channel/device
+ *
+ * @_fifo_mode_enum - a pointer to a struct iio_enum that defines the available
+ * modes for this hardware fifo (see Documentation/ABI/testing/sysfs-bus-iio for
+ * supported modes)
+ * @_shared - type of sharing; can be IIO_SEPARATE if we have fifo(s) per
+ * modifier (e.g. x, y, z), IIO_SHARED_BY_TYPE if we have fifo(s) per channel or
+ * IIO_SHARED_BY_ALL if we have a single fifo for all channels
+ *
+ */
+#define IIO_FIFO_EXT_INFO(_fifo_mode_enum, _shared)			\
+	IIO_ENUM("fifo_mode", _shared, _fifo_mode_enum),		\
+	IIO_ENUM_AVAILABLE("fifo_mode", _fifo_mode_enum)
+
 #endif /* _INDUSTRIAL_IO_H_ */
diff --git a/include/linux/iio/types.h b/include/linux/iio/types.h
index 4a2af8a..8ee5a21 100644
--- a/include/linux/iio/types.h
+++ b/include/linux/iio/types.h
@@ -68,6 +68,8 @@ enum iio_event_type {
 	IIO_EV_TYPE_ROC,
 	IIO_EV_TYPE_THRESH_ADAPTIVE,
 	IIO_EV_TYPE_MAG_ADAPTIVE,
+	IIO_EV_TYPE_FIFO_FULL,
+	IIO_EV_TYPE_FIFO_WATERMARK,
 };
 
 enum iio_event_info {
-- 
1.9.1

Re: [RFC 1/8] iio: add support for hardware fifo

[jic23]

Octavian Purdila writes: 

> Some devices have hardware buffers that can store a number of samples
> for later consumption. Hardware usually provides interrupts to notify
> the processor when the fifo is full or when it has reached a certain
> threshold. 
> 
> Some devices also offer flexibility in what they do when the hardware
> fifo is full: either discard future samples, or overwrite older
> samples with newer ones. 
> 
> The hardware fifo is useful for reducing the number of interrupts to
> the host processor and thus it helps decreasing the power consumption. 
> 
> This patch adds four new IIO attributes to expose the hardware fifo
> and it's configuration to userspace: 
> 
>  * _fifo_length - the number of samples that the hardware can store;
>    it is usefull to expose it to userspace, so that applications can
>    increase the device buffer size appropriately

> 
>  * _fifo_mode - the mode the hardware fifo operates 
> 
>  * _fifo_mode_available - the available hardware fifo modes 
> 
>  * _fifo_flush - this attribute can be used by userspace to trigger a
>    fifo flush operation, where all data stored in the fifo will be
>    made available to the device buffer 
> 
> Since the last two attributes work with strings, they are using the
> iio_chan_spec_ext_info infrastructure. To simplify the setup for a
> hardware fifo the IIO_FIFO_EXT_INFO marcro can be used to initialize
> them. 
> 
> This patch also add two new events: IIO_EV_TYPE_FIFO_FULL and
> IIO_EV_TYPE_FIFO_WATERMAK. If enabled they must be issued by the
> device when the fifo level reaches full or, respectively, the
> watermark level. The watermark level can be set by configuring the
> event value. 
> 
> Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
> ---
>  Documentation/ABI/testing/sysfs-bus-iio | 51 +++++++++++++++++++++++++++++++++
>  drivers/iio/industrialio-core.c         |  2 ++
>  drivers/iio/industrialio-event.c        |  2 ++
>  include/linux/iio/iio.h                 | 17 +++++++++++
>  include/linux/iio/types.h               |  2 ++
>  5 files changed, 74 insertions(+) 
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> index ea7e7ab..f326de1 100644
> --- a/Documentation/ABI/testing/sysfs-bus-iio
> +++ b/Documentation/ABI/testing/sysfs-bus-iio
> @@ -1050,3 +1050,54 @@ Description:
>  		after application of scale and offset. If no offset or scale is
>  		present, output should be considered as processed with the
>  		unit in milliamps.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/in_fifo_length
> +What:		/sys/bus/iio/devices/iio:deviceX/in_accel_fifo_length
> +What:		/sys/bus/iio/devices/iio:deviceX/in_magn_fifo_length
> +What:		/sys/bus/iio/devices/iio:deviceX/in_anglvel_fifo_length
> +KernelVersion:	3.19
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		The maximum number of samples that the hardware fifo can
> +		store.
This provides information useful in debugging, but userspace should not
care other than in how this constrains the watershed point. 


> +
> +What:		/sys/bus/iio/devices/iio:deviceX/in_fifo_mode
> +What:		/sys/bus/iio/devices/iio:deviceX/in_accel_fifo_mode
> +What:		/sys/bus/iio/devices/iio:deviceX/in_magn_fifo_mode
> +What:		/sys/bus/iio/devices/iio:deviceX/in_anglvel_fifo_mode
> +KernelVersion:	3.19
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		The mode in which the hardware fifo currently operates. The
> +		supported values are "disabled", "fifo" and "stream".
> +		In disabled mode the hardware fifo is not operational.
> +		In fifo mode the samples are stored until the fifo is full at
> +		which point all subsequent samples are discarded.
> +		In stream mode, when the fifo is full new samples start to
> +		overrride old samples.
In stream mode we end up with a nasty hybrid of a front end ring buffer
pushing into a fifo.  I can see you want to support the hardware fully
but for now am unconvinced that it ever makes sense to use stream mode. 

If user space wants the latest data it is because it is time critical and
hence it won't want to have a fifo in the way.  If it wants to not drop
data then the fifo should be fine.  Ultimately if we are hitting the
limit where this choice matters then we have a nasty problem... 

> +		If the hardware supports it, IIO_EVENT_TYPE_FIFO_FULL and
> +		IIO_EVENT_TYPE_FIFO_WATERMARK events will be generated when they
> +		are enabled by the application and the fifo is full and,
> +		respectively, when the fifo level reaches the watermark
> +		level.
In a sense fifo full is an error rather than a useful event.  Now error
reporting is an area we don't have well covered.  Suggestions welcome! 

> +		The watermark level can be set by the application by configuring
> +		the IIO_EVENT_TYPE_FIFO_WATERMARK value (IIO_EV_INFO_VALUE).
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/in_fifo_mode_available
> +What:		/sys/bus/iio/devices/iio:deviceX/in_accel_fifo_mode_available
> +What:		/sys/bus/iio/devices/iio:deviceX/in_magn_fifo_mode_available
> +What:		/sys/bus/iio/devices/iio:deviceX/in_anglvel_fifo_mode_available
> +KernelVersion:	3.19
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		The modes that the hardware fifo supports.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/in_fifo_flush
> +What:		/sys/bus/iio/devices/iio:deviceX/in_accel_fifo_flush
> +What:		/sys/bus/iio/devices/iio:deviceX/in_magn_fifo_flush
> +What:		/sys/bus/iio/devices/iio:deviceX/in_anglvel_fifo_flush
> +KernelVersion:	3.19
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Writting to this entry will cause the device to flush the data
> +		from the hardware fifo into the device buffer.
This is an interesting concept.  Why would userspace want to have control
of the rough time of the buffer read out?  If it is using the watershed
event, then why not make it push the data to the software buffer when that
occurs?  If not and we have no events on the buffer then I think we'd
want to push this polling into the kernel as it would depend on the
(hopefully known) sampling frequency and the size of the hardware buffer... 


 

> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index af3e76d..17e84c3 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -113,6 +113,8 @@ static const char * const iio_chan_info_postfix[] = {
>  	[IIO_CHAN_INFO_HARDWAREGAIN] = "hardwaregain",
>  	[IIO_CHAN_INFO_HYSTERESIS] = "hysteresis",
>  	[IIO_CHAN_INFO_INT_TIME] = "integration_time",
> +	[IIO_CHAN_INFO_FIFO_LENGTH] = "fifo_length",
> +	[IIO_CHAN_INFO_FIFO_FLUSH] = "fifo_flush",
Where is the watermark level set?
>  };
>  
>  /**
> diff --git a/drivers/iio/industrialio-event.c b/drivers/iio/industrialio-event.c
> index 0c1e37e..3d73fd0 100644
> --- a/drivers/iio/industrialio-event.c
> +++ b/drivers/iio/industrialio-event.c
> @@ -197,6 +197,8 @@ static const char * const iio_ev_type_text[] = {
>  	[IIO_EV_TYPE_ROC] = "roc",
>  	[IIO_EV_TYPE_THRESH_ADAPTIVE] = "thresh_adaptive",
>  	[IIO_EV_TYPE_MAG_ADAPTIVE] = "mag_adaptive",
> +	[IIO_EV_TYPE_FIFO_FULL] = "fifo_full",
> +	[IIO_EV_TYPE_FIFO_WATERMARK] = "fifo_watermak",
>  };
>  
>  static const char * const iio_ev_dir_text[] = {
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index 3642ce7..ad100a1 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -38,6 +38,8 @@ enum iio_chan_info_enum {
>  	IIO_CHAN_INFO_HARDWAREGAIN,
>  	IIO_CHAN_INFO_HYSTERESIS,
>  	IIO_CHAN_INFO_INT_TIME,
> +	IIO_CHAN_INFO_FIFO_LENGTH,
> +	IIO_CHAN_INFO_FIFO_FLUSH,
>  };
>  
>  enum iio_shared_by {
> @@ -629,4 +631,19 @@ int iio_str_to_fixpoint(const char *str, int fract_mult, int *integer,
>   */
>  #define IIO_G_TO_M_S_2(g) ((g) * 980665ULL / 100000ULL)
>  
> +/**
> + * IIO_FIFO_EXT_INFO() - define a hardware fifo associated with a channel/device
> + *
> + * @_fifo_mode_enum - a pointer to a struct iio_enum that defines the available
> + * modes for this hardware fifo (see Documentation/ABI/testing/sysfs-bus-iio for
> + * supported modes)
> + * @_shared - type of sharing; can be IIO_SEPARATE if we have fifo(s) per
> + * modifier (e.g. x, y, z), IIO_SHARED_BY_TYPE if we have fifo(s) per channel or
> + * IIO_SHARED_BY_ALL if we have a single fifo for all channels
> + *
> + */
> +#define IIO_FIFO_EXT_INFO(_fifo_mode_enum, _shared)			\
> +	IIO_ENUM("fifo_mode", _shared, _fifo_mode_enum),		\
> +	IIO_ENUM_AVAILABLE("fifo_mode", _fifo_mode_enum)
> +
>  #endif /* _INDUSTRIAL_IO_H_ */
> diff --git a/include/linux/iio/types.h b/include/linux/iio/types.h
> index 4a2af8a..8ee5a21 100644
> --- a/include/linux/iio/types.h
> +++ b/include/linux/iio/types.h
> @@ -68,6 +68,8 @@ enum iio_event_type {
>  	IIO_EV_TYPE_ROC,
>  	IIO_EV_TYPE_THRESH_ADAPTIVE,
>  	IIO_EV_TYPE_MAG_ADAPTIVE,
> +	IIO_EV_TYPE_FIFO_FULL,
> +	IIO_EV_TYPE_FIFO_WATERMARK,
>  };
>  
>  enum iio_event_info {
> -- 
> 1.9.1 
> 
> --
> 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: [RFC 1/8] iio: add support for hardware fifo

[Octavian Purdila]

>> +
>> +What:          /sys/bus/iio/devices/iio:deviceX/in_fifo_length
>> +What:          /sys/bus/iio/devices/iio:deviceX/in_accel_fifo_length
>> +What:          /sys/bus/iio/devices/iio:deviceX/in_magn_fifo_length
>> +What:          /sys/bus/iio/devices/iio:deviceX/in_anglvel_fifo_length
>> +KernelVersion: 3.19
>> +Contact:       linux-iio@vger.kernel.org
>> +Description:
>> +               The maximum number of samples that the hardware fifo can
>> +               store.
>
> This provides information useful in debugging, but userspace should not
> care other than in how this constrains the watershed point.
>

This is useful for the application so that it can increase the device
buffer size in order to be able to use the hardware fifo, since the
data will be flushed from the hardware fifo to the device buffer. I
should have probably explain that.

>> +
>> +What:          /sys/bus/iio/devices/iio:deviceX/in_fifo_mode
>> +What:          /sys/bus/iio/devices/iio:deviceX/in_accel_fifo_mode
>> +What:          /sys/bus/iio/devices/iio:deviceX/in_magn_fifo_mode
>> +What:          /sys/bus/iio/devices/iio:deviceX/in_anglvel_fifo_mode
>> +KernelVersion: 3.19
>> +Contact:       linux-iio@vger.kernel.org
>> +Description:
>> +               The mode in which the hardware fifo currently operates.
>> The
>> +               supported values are "disabled", "fifo" and "stream".
>> +               In disabled mode the hardware fifo is not operational.
>> +               In fifo mode the samples are stored until the fifo is full
>> at
>> +               which point all subsequent samples are discarded.
>> +               In stream mode, when the fifo is full new samples start to
>> +               overrride old samples.
>
> In stream mode we end up with a nasty hybrid of a front end ring buffer
> pushing into a fifo.  I can see you want to support the hardware fully
> but for now am unconvinced that it ever makes sense to use stream mode.
> If user space wants the latest data it is because it is time critical and
> hence it won't want to have a fifo in the way.  If it wants to not drop
> data then the fifo should be fine.  Ultimately if we are hitting the
> limit where this choice matters then we have a nasty problem...

The stream mode can help when we want to collect data while the CPU is
a sleep. See this Android reference:

http://source.android.com/devices/sensors/batching.html#behavior_in_suspend_mode

>>
>> +               If the hardware supports it, IIO_EVENT_TYPE_FIFO_FULL and
>> +               IIO_EVENT_TYPE_FIFO_WATERMARK events will be generated
>> when they
>> +               are enabled by the application and the fifo is full and,
>> +               respectively, when the fifo level reaches the watermark
>> +               level.
>
> In a sense fifo full is an error rather than a useful event.  Now error
> reporting is an area we don't have well covered.  Suggestions welcome!

Yes, it is an error. Still, I think it is an useful event. For
example, userspace can take note of this and reduce the watermark
level.

>>
>> +               The watermark level can be set by the application by
>> configuring
>> +               the IIO_EVENT_TYPE_FIFO_WATERMARK value
>> (IIO_EV_INFO_VALUE).
>> +
>> +What:          /sys/bus/iio/devices/iio:deviceX/in_fifo_mode_available
>> +What:
>> /sys/bus/iio/devices/iio:deviceX/in_accel_fifo_mode_available
>> +What:
>> /sys/bus/iio/devices/iio:deviceX/in_magn_fifo_mode_available
>> +What:
>> /sys/bus/iio/devices/iio:deviceX/in_anglvel_fifo_mode_available
>> +KernelVersion: 3.19
>> +Contact:       linux-iio@vger.kernel.org
>> +Description:
>> +               The modes that the hardware fifo supports.
>> +
>> +What:          /sys/bus/iio/devices/iio:deviceX/in_fifo_flush
>> +What:          /sys/bus/iio/devices/iio:deviceX/in_accel_fifo_flush
>> +What:          /sys/bus/iio/devices/iio:deviceX/in_magn_fifo_flush
>> +What:          /sys/bus/iio/devices/iio:deviceX/in_anglvel_fifo_flush
>> +KernelVersion: 3.19
>> +Contact:       linux-iio@vger.kernel.org
>> +Description:
>> +               Writting to this entry will cause the device to flush the
>> data
>> +               from the hardware fifo into the device buffer.
>
> This is an interesting concept.  Why would userspace want to have control
> of the rough time of the buffer read out?  If it is using the watershed
> event, then why not make it push the data to the software buffer when that
> occurs?  If not and we have no events on the buffer then I think we'd
> want to push this polling into the kernel as it would depend on the
> (hopefully known) sampling frequency and the size of the hardware buffer...
>

Yes, for most cases the watershed interrupt should be the one that
drives flushing the hardware fifo.

Having this knob helps the application balance power and latency, as
I've explain in the other email.

>
>
>> diff --git a/drivers/iio/industrialio-core.c
>> b/drivers/iio/industrialio-core.c
>> index af3e76d..17e84c3 100644
>> --- a/drivers/iio/industrialio-core.c
>> +++ b/drivers/iio/industrialio-core.c
>> @@ -113,6 +113,8 @@ static const char * const iio_chan_info_postfix[] = {
>>         [IIO_CHAN_INFO_HARDWAREGAIN] = "hardwaregain",
>>         [IIO_CHAN_INFO_HYSTERESIS] = "hysteresis",
>>         [IIO_CHAN_INFO_INT_TIME] = "integration_time",
>> +       [IIO_CHAN_INFO_FIFO_LENGTH] = "fifo_length",
>> +       [IIO_CHAN_INFO_FIFO_FLUSH] = "fifo_flush",
>
> Where is the watermark level set?
>>

I have defined watermark as an event, so the watermark value is set in
in write_event_value (just as for e.g. ROC).

[RFC 2/8] iio: bmc150: refactor slope duration and threshold update

[Octavian Purdila]

Move the slope duration and threshold update in separate functions
to reduce code duplicate between chip init and motion interrupt setup.

The patch also moves the update from the motion interrupt setup
function to the write event function so that we can later refactor the
interrupt code.

Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
---
 drivers/iio/accel/bmc150-accel.c | 109 ++++++++++++++++++++++-----------------
 1 file changed, 62 insertions(+), 47 deletions(-)

diff --git a/drivers/iio/accel/bmc150-accel.c b/drivers/iio/accel/bmc150-accel.c
index 22c096c..5bfb09d 100644
--- a/drivers/iio/accel/bmc150-accel.c
+++ b/drivers/iio/accel/bmc150-accel.c
@@ -266,6 +266,51 @@ static int bmc150_accel_set_bw(struct bmc150_accel_data *data, int val,
 	return -EINVAL;
 }
 
+static int bmc150_accel_update_slope_threshold(struct bmc150_accel_data *data,
+					       int val)
+{
+	int ret = 0;
+
+	val &= 0xFF;
+
+	ret = i2c_smbus_write_byte_data(data->client, BMC150_ACCEL_REG_INT_6,
+					val);
+	if (ret < 0) {
+		dev_err(&data->client->dev, "Error writing reg_int_6\n");
+		return ret;
+	}
+	data->slope_thres = val;
+
+	dev_dbg(&data->client->dev, "%s: %x\n", __func__, val);
+
+	return ret;
+}
+
+static int bmc150_accel_update_slope_duration(struct bmc150_accel_data *data,
+					      int val)
+{
+	int ret;
+
+	val &= BMC150_ACCEL_SLOPE_DUR_MASK;
+
+	ret = i2c_smbus_read_byte_data(data->client, BMC150_ACCEL_REG_INT_5);
+	if (ret < 0) {
+		dev_err(&data->client->dev, "Error reading reg_int_5\n");
+		return ret;
+	}
+	ret = i2c_smbus_write_byte_data(data->client, BMC150_ACCEL_REG_INT_5,
+					val | ret);
+	if (ret < 0) {
+		dev_err(&data->client->dev, "Error write reg_int_5\n");
+		return ret;
+	}
+	data->slope_dur = val;
+
+	dev_dbg(&data->client->dev, "%s: %x\n", __func__, val);
+
+	return ret;
+}
+
 static int bmc150_accel_chip_init(struct bmc150_accel_data *data)
 {
 	int ret;
@@ -304,32 +349,16 @@ static int bmc150_accel_chip_init(struct bmc150_accel_data *data)
 
 	data->range = BMC150_ACCEL_DEF_RANGE_4G;
 
-	/* Set default slope duration */
-	ret = i2c_smbus_read_byte_data(data->client, BMC150_ACCEL_REG_INT_5);
-	if (ret < 0) {
-		dev_err(&data->client->dev, "Error reading reg_int_5\n");
-		return ret;
-	}
-	data->slope_dur |= BMC150_ACCEL_DEF_SLOPE_DURATION;
-	ret = i2c_smbus_write_byte_data(data->client,
-					BMC150_ACCEL_REG_INT_5,
-					data->slope_dur);
-	if (ret < 0) {
-		dev_err(&data->client->dev, "Error writing reg_int_5\n");
+	/* Set default slope duration and thresholds */
+	ret = bmc150_accel_update_slope_duration(data,
+					 BMC150_ACCEL_DEF_SLOPE_DURATION);
+	if (ret < 0)
 		return ret;
-	}
-	dev_dbg(&data->client->dev, "slope_dur %x\n", data->slope_dur);
 
-	/* Set default slope thresholds */
-	ret = i2c_smbus_write_byte_data(data->client,
-					BMC150_ACCEL_REG_INT_6,
-					BMC150_ACCEL_DEF_SLOPE_THRESHOLD);
-	if (ret < 0) {
-		dev_err(&data->client->dev, "Error writing reg_int_6\n");
+	ret = bmc150_accel_update_slope_threshold(data,
+					  BMC150_ACCEL_DEF_SLOPE_THRESHOLD);
+	if (ret < 0)
 		return ret;
-	}
-	data->slope_thres = BMC150_ACCEL_DEF_SLOPE_THRESHOLD;
-	dev_dbg(&data->client->dev, "slope_thres %x\n", data->slope_thres);
 
 	/* Set default as latched interrupts */
 	ret = i2c_smbus_write_byte_data(data->client,
@@ -372,24 +401,6 @@ static int bmc150_accel_setup_any_motion_interrupt(
 	}
 
 	if (status) {
-		/* Set slope duration (no of samples) */
-		ret = i2c_smbus_write_byte_data(data->client,
-						BMC150_ACCEL_REG_INT_5,
-						data->slope_dur);
-		if (ret < 0) {
-			dev_err(&data->client->dev, "Error write reg_int_5\n");
-			return ret;
-		}
-
-		/* Set slope thresholds */
-		ret = i2c_smbus_write_byte_data(data->client,
-						BMC150_ACCEL_REG_INT_6,
-						data->slope_thres);
-		if (ret < 0) {
-			dev_err(&data->client->dev, "Error write reg_int_6\n");
-			return ret;
-		}
-
 		/*
 		 * New data interrupt is always non-latched,
 		 * which will have higher priority, so no need
@@ -726,7 +737,7 @@ static int bmc150_accel_read_event(struct iio_dev *indio_dev,
 		*val = data->slope_thres;
 		break;
 	case IIO_EV_INFO_PERIOD:
-		*val = data->slope_dur & BMC150_ACCEL_SLOPE_DUR_MASK;
+		*val = data->slope_dur;
 		break;
 	default:
 		return -EINVAL;
@@ -743,23 +754,27 @@ static int bmc150_accel_write_event(struct iio_dev *indio_dev,
 				    int val, int val2)
 {
 	struct bmc150_accel_data *data = iio_priv(indio_dev);
+	int ret;
 
 	if (data->ev_enable_state)
 		return -EBUSY;
 
 	switch (info) {
 	case IIO_EV_INFO_VALUE:
-		data->slope_thres = val;
+		mutex_lock(&data->mutex);
+		ret = bmc150_accel_update_slope_threshold(data, val);
+		mutex_unlock(&data->mutex);
 		break;
 	case IIO_EV_INFO_PERIOD:
-		data->slope_dur &= ~BMC150_ACCEL_SLOPE_DUR_MASK;
-		data->slope_dur |= val & BMC150_ACCEL_SLOPE_DUR_MASK;
+		mutex_lock(&data->mutex);
+		ret = bmc150_accel_update_slope_duration(data, val);
+		mutex_unlock(&data->mutex);
 		break;
 	default:
-		return -EINVAL;
+		ret = -EINVAL;
 	}
 
-	return 0;
+	return ret;
 }
 
 static int bmc150_accel_read_event_config(struct iio_dev *indio_dev,
-- 
1.9.1

Re: [RFC 2/8] iio: bmc150: refactor slope duration and threshold update

[Hartmut Knaack]

Octavian Purdila schrieb am 17.11.2014 18:56:
> Move the slope duration and threshold update in separate functions
> to reduce code duplicate between chip init and motion interrupt setup.
> 
> The patch also moves the update from the motion interrupt setup
> function to the write event function so that we can later refactor the
> interrupt code.
> 
> Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
> ---
>  drivers/iio/accel/bmc150-accel.c | 109 ++++++++++++++++++++++-----------------
>  1 file changed, 62 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/iio/accel/bmc150-accel.c b/drivers/iio/accel/bmc150-accel.c
> index 22c096c..5bfb09d 100644
> --- a/drivers/iio/accel/bmc150-accel.c
> +++ b/drivers/iio/accel/bmc150-accel.c
> @@ -266,6 +266,51 @@ static int bmc150_accel_set_bw(struct bmc150_accel_data *data, int val,
>  	return -EINVAL;
>  }
>  
> +static int bmc150_accel_update_slope_threshold(struct bmc150_accel_data *data,
> +					       int val)
> +{
> +	int ret = 0;
> +
> +	val &= 0xFF;
> +
> +	ret = i2c_smbus_write_byte_data(data->client, BMC150_ACCEL_REG_INT_6,
> +					val);
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "Error writing reg_int_6\n");
> +		return ret;
> +	}
> +	data->slope_thres = val;
> +
> +	dev_dbg(&data->client->dev, "%s: %x\n", __func__, val);
> +
> +	return ret;
> +}
> +
> +static int bmc150_accel_update_slope_duration(struct bmc150_accel_data *data,
> +					      int val)
> +{
> +	int ret;
> +
> +	val &= BMC150_ACCEL_SLOPE_DUR_MASK;
> +
> +	ret = i2c_smbus_read_byte_data(data->client, BMC150_ACCEL_REG_INT_5);
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "Error reading reg_int_5\n");
> +		return ret;
> +	}
> +	ret = i2c_smbus_write_byte_data(data->client, BMC150_ACCEL_REG_INT_5,
> +					val | ret);
You will have more joy if you clear the appropriate bits in ret before setting new ones.
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "Error write reg_int_5\n");
> +		return ret;
> +	}
> +	data->slope_dur = val;
> +
> +	dev_dbg(&data->client->dev, "%s: %x\n", __func__, val);
> +
> +	return ret;
> +}
> +
>  static int bmc150_accel_chip_init(struct bmc150_accel_data *data)
>  {
>  	int ret;
> @@ -304,32 +349,16 @@ static int bmc150_accel_chip_init(struct bmc150_accel_data *data)
>  
>  	data->range = BMC150_ACCEL_DEF_RANGE_4G;
>  
> -	/* Set default slope duration */
> -	ret = i2c_smbus_read_byte_data(data->client, BMC150_ACCEL_REG_INT_5);
> -	if (ret < 0) {
> -		dev_err(&data->client->dev, "Error reading reg_int_5\n");
> -		return ret;
> -	}
> -	data->slope_dur |= BMC150_ACCEL_DEF_SLOPE_DURATION;
> -	ret = i2c_smbus_write_byte_data(data->client,
> -					BMC150_ACCEL_REG_INT_5,
> -					data->slope_dur);
> -	if (ret < 0) {
> -		dev_err(&data->client->dev, "Error writing reg_int_5\n");
> +	/* Set default slope duration and thresholds */
> +	ret = bmc150_accel_update_slope_duration(data,
> +					 BMC150_ACCEL_DEF_SLOPE_DURATION);
> +	if (ret < 0)
>  		return ret;
> -	}
> -	dev_dbg(&data->client->dev, "slope_dur %x\n", data->slope_dur);
>  
> -	/* Set default slope thresholds */
> -	ret = i2c_smbus_write_byte_data(data->client,
> -					BMC150_ACCEL_REG_INT_6,
> -					BMC150_ACCEL_DEF_SLOPE_THRESHOLD);
> -	if (ret < 0) {
> -		dev_err(&data->client->dev, "Error writing reg_int_6\n");
> +	ret = bmc150_accel_update_slope_threshold(data,
> +					  BMC150_ACCEL_DEF_SLOPE_THRESHOLD);
> +	if (ret < 0)
>  		return ret;
> -	}
> -	data->slope_thres = BMC150_ACCEL_DEF_SLOPE_THRESHOLD;
> -	dev_dbg(&data->client->dev, "slope_thres %x\n", data->slope_thres);
>  
>  	/* Set default as latched interrupts */
>  	ret = i2c_smbus_write_byte_data(data->client,
> @@ -372,24 +401,6 @@ static int bmc150_accel_setup_any_motion_interrupt(
>  	}
>  
>  	if (status) {
> -		/* Set slope duration (no of samples) */
> -		ret = i2c_smbus_write_byte_data(data->client,
> -						BMC150_ACCEL_REG_INT_5,
> -						data->slope_dur);
> -		if (ret < 0) {
> -			dev_err(&data->client->dev, "Error write reg_int_5\n");
> -			return ret;
> -		}
> -
> -		/* Set slope thresholds */
> -		ret = i2c_smbus_write_byte_data(data->client,
> -						BMC150_ACCEL_REG_INT_6,
> -						data->slope_thres);
> -		if (ret < 0) {
> -			dev_err(&data->client->dev, "Error write reg_int_6\n");
> -			return ret;
> -		}
> -
>  		/*
>  		 * New data interrupt is always non-latched,
>  		 * which will have higher priority, so no need
> @@ -726,7 +737,7 @@ static int bmc150_accel_read_event(struct iio_dev *indio_dev,
>  		*val = data->slope_thres;
>  		break;
>  	case IIO_EV_INFO_PERIOD:
> -		*val = data->slope_dur & BMC150_ACCEL_SLOPE_DUR_MASK;
> +		*val = data->slope_dur;
>  		break;
>  	default:
>  		return -EINVAL;
> @@ -743,23 +754,27 @@ static int bmc150_accel_write_event(struct iio_dev *indio_dev,
>  				    int val, int val2)
>  {
>  	struct bmc150_accel_data *data = iio_priv(indio_dev);
> +	int ret;
>  
>  	if (data->ev_enable_state)
>  		return -EBUSY;
>  
>  	switch (info) {
>  	case IIO_EV_INFO_VALUE:
> -		data->slope_thres = val;
> +		mutex_lock(&data->mutex);
> +		ret = bmc150_accel_update_slope_threshold(data, val);
> +		mutex_unlock(&data->mutex);
>  		break;
>  	case IIO_EV_INFO_PERIOD:
> -		data->slope_dur &= ~BMC150_ACCEL_SLOPE_DUR_MASK;
> -		data->slope_dur |= val & BMC150_ACCEL_SLOPE_DUR_MASK;
> +		mutex_lock(&data->mutex);
> +		ret = bmc150_accel_update_slope_duration(data, val);
> +		mutex_unlock(&data->mutex);
>  		break;
>  	default:
> -		return -EINVAL;
> +		ret = -EINVAL;
>  	}
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static int bmc150_accel_read_event_config(struct iio_dev *indio_dev,
> 

Re: [RFC 2/8] iio: bmc150: refactor slope duration and threshold update

[Octavian Purdila]

On Sun, Nov 23, 2014 at 11:58 PM, Hartmut Knaack <knaack.h@gmx.de> wrote:
>
> Octavian Purdila schrieb am 17.11.2014 18:56:
> > Move the slope duration and threshold update in separate functions
> > to reduce code duplicate between chip init and motion interrupt setup.
> >
> > The patch also moves the update from the motion interrupt setup
> > function to the write event function so that we can later refactor the
> > interrupt code.
> >
> > Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
> > ---
> >  drivers/iio/accel/bmc150-accel.c | 109 ++++++++++++++++++++++-----------------
> >  1 file changed, 62 insertions(+), 47 deletions(-)
> >
> > diff --git a/drivers/iio/accel/bmc150-accel.c b/drivers/iio/accel/bmc150-accel.c
> > index 22c096c..5bfb09d 100644
> > --- a/drivers/iio/accel/bmc150-accel.c
> > +++ b/drivers/iio/accel/bmc150-accel.c
> > @@ -266,6 +266,51 @@ static int bmc150_accel_set_bw(struct bmc150_accel_data *data, int val,
> >       return -EINVAL;
> >  }
> >
> > +static int bmc150_accel_update_slope_threshold(struct bmc150_accel_data *data,
> > +                                            int val)
> > +{
> > +     int ret = 0;
> > +
> > +     val &= 0xFF;
> > +
> > +     ret = i2c_smbus_write_byte_data(data->client, BMC150_ACCEL_REG_INT_6,
> > +                                     val);
> > +     if (ret < 0) {
> > +             dev_err(&data->client->dev, "Error writing reg_int_6\n");
> > +             return ret;
> > +     }
> > +     data->slope_thres = val;
> > +
> > +     dev_dbg(&data->client->dev, "%s: %x\n", __func__, val);
> > +
> > +     return ret;
> > +}
> > +
> > +static int bmc150_accel_update_slope_duration(struct bmc150_accel_data *data,
> > +                                           int val)
> > +{
> > +     int ret;
> > +
> > +     val &= BMC150_ACCEL_SLOPE_DUR_MASK;
> > +
> > +     ret = i2c_smbus_read_byte_data(data->client, BMC150_ACCEL_REG_INT_5);
> > +     if (ret < 0) {
> > +             dev_err(&data->client->dev, "Error reading reg_int_5\n");
> > +             return ret;
> > +     }
> > +     ret = i2c_smbus_write_byte_data(data->client, BMC150_ACCEL_REG_INT_5,
> > +                                     val | ret);
> You will have more joy if you clear the appropriate bits in ret before setting new ones.

Yes, Daniel caught as well. I will fix it in v2.

[RFC 3/8] iio: bmc150: refactor interrupt enabling

[Octavian Purdila]

This patch combines the any motion and new data interrupts function
into a single, generic, interrupt enable function. On top of this, we
can later refactor triggers to make it easier to add new triggers.

Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
---
 drivers/iio/accel/bmc150-accel.c | 271 +++++++++++++++++----------------------
 1 file changed, 115 insertions(+), 156 deletions(-)

diff --git a/drivers/iio/accel/bmc150-accel.c b/drivers/iio/accel/bmc150-accel.c
index 5bfb09d..ee4afc4 100644
--- a/drivers/iio/accel/bmc150-accel.c
+++ b/drivers/iio/accel/bmc150-accel.c
@@ -144,6 +144,13 @@ struct bmc150_accel_chip_info {
 	const struct bmc150_scale_info scale_table[4];
 };
 
+struct bmc150_accel_interrupt_info {
+	u8 map_reg;
+	u8 map_bitmask;
+	u8 en_reg;
+	u8 en_bitmask;
+};
+
 struct bmc150_accel_data {
 	struct i2c_client *client;
 	struct iio_trigger *dready_trig;
@@ -374,137 +381,6 @@ static int bmc150_accel_chip_init(struct bmc150_accel_data *data)
 	return 0;
 }
 
-static int bmc150_accel_setup_any_motion_interrupt(
-					struct bmc150_accel_data *data,
-					bool status)
-{
-	int ret;
-
-	/* Enable/Disable INT1 mapping */
-	ret = i2c_smbus_read_byte_data(data->client,
-				       BMC150_ACCEL_REG_INT_MAP_0);
-	if (ret < 0) {
-		dev_err(&data->client->dev, "Error reading reg_int_map_0\n");
-		return ret;
-	}
-	if (status)
-		ret |= BMC150_ACCEL_INT_MAP_0_BIT_SLOPE;
-	else
-		ret &= ~BMC150_ACCEL_INT_MAP_0_BIT_SLOPE;
-
-	ret = i2c_smbus_write_byte_data(data->client,
-					BMC150_ACCEL_REG_INT_MAP_0,
-					ret);
-	if (ret < 0) {
-		dev_err(&data->client->dev, "Error writing reg_int_map_0\n");
-		return ret;
-	}
-
-	if (status) {
-		/*
-		 * New data interrupt is always non-latched,
-		 * which will have higher priority, so no need
-		 * to set latched mode, we will be flooded anyway with INTR
-		 */
-		if (!data->dready_trigger_on) {
-			ret = i2c_smbus_write_byte_data(data->client,
-					BMC150_ACCEL_REG_INT_RST_LATCH,
-					BMC150_ACCEL_INT_MODE_LATCH_INT |
-					BMC150_ACCEL_INT_MODE_LATCH_RESET);
-			if (ret < 0) {
-				dev_err(&data->client->dev,
-					"Error writing reg_int_rst_latch\n");
-				return ret;
-			}
-		}
-
-		ret = i2c_smbus_write_byte_data(data->client,
-						BMC150_ACCEL_REG_INT_EN_0,
-						BMC150_ACCEL_INT_EN_BIT_SLP_X |
-						BMC150_ACCEL_INT_EN_BIT_SLP_Y |
-						BMC150_ACCEL_INT_EN_BIT_SLP_Z);
-	} else
-		ret = i2c_smbus_write_byte_data(data->client,
-						BMC150_ACCEL_REG_INT_EN_0,
-						0);
-
-	if (ret < 0) {
-		dev_err(&data->client->dev, "Error writing reg_int_en_0\n");
-		return ret;
-	}
-
-	return 0;
-}
-
-static int bmc150_accel_setup_new_data_interrupt(struct bmc150_accel_data *data,
-					   bool status)
-{
-	int ret;
-
-	/* Enable/Disable INT1 mapping */
-	ret = i2c_smbus_read_byte_data(data->client,
-				       BMC150_ACCEL_REG_INT_MAP_1);
-	if (ret < 0) {
-		dev_err(&data->client->dev, "Error reading reg_int_map_1\n");
-		return ret;
-	}
-	if (status)
-		ret |= BMC150_ACCEL_INT_MAP_1_BIT_DATA;
-	else
-		ret &= ~BMC150_ACCEL_INT_MAP_1_BIT_DATA;
-
-	ret = i2c_smbus_write_byte_data(data->client,
-					BMC150_ACCEL_REG_INT_MAP_1,
-					ret);
-	if (ret < 0) {
-		dev_err(&data->client->dev, "Error writing reg_int_map_1\n");
-		return ret;
-	}
-
-	if (status) {
-		/*
-		 * Set non latched mode interrupt and clear any latched
-		 * interrupt
-		 */
-		ret = i2c_smbus_write_byte_data(data->client,
-					BMC150_ACCEL_REG_INT_RST_LATCH,
-					BMC150_ACCEL_INT_MODE_NON_LATCH_INT |
-					BMC150_ACCEL_INT_MODE_LATCH_RESET);
-		if (ret < 0) {
-			dev_err(&data->client->dev,
-				"Error writing reg_int_rst_latch\n");
-			return ret;
-		}
-
-		ret = i2c_smbus_write_byte_data(data->client,
-					BMC150_ACCEL_REG_INT_EN_1,
-					BMC150_ACCEL_INT_EN_BIT_DATA_EN);
-
-	} else {
-		/* Restore default interrupt mode */
-		ret = i2c_smbus_write_byte_data(data->client,
-					BMC150_ACCEL_REG_INT_RST_LATCH,
-					BMC150_ACCEL_INT_MODE_LATCH_INT |
-					BMC150_ACCEL_INT_MODE_LATCH_RESET);
-		if (ret < 0) {
-			dev_err(&data->client->dev,
-				"Error writing reg_int_rst_latch\n");
-			return ret;
-		}
-
-		ret = i2c_smbus_write_byte_data(data->client,
-						BMC150_ACCEL_REG_INT_EN_1,
-						0);
-	}
-
-	if (ret < 0) {
-		dev_err(&data->client->dev, "Error writing reg_int_en_1\n");
-		return ret;
-	}
-
-	return 0;
-}
-
 static int bmc150_accel_get_bw(struct bmc150_accel_data *data, int *val,
 			       int *val2)
 {
@@ -559,6 +435,99 @@ static int bmc150_accel_set_power_state(struct bmc150_accel_data *data, bool on)
 }
 #endif
 
+static struct bmc150_accel_interrupt_info bmc150_accel_interrupts[] = {
+	{ /* data ready interrupt */
+		.map_reg = BMC150_ACCEL_REG_INT_MAP_1,
+		.map_bitmask = BMC150_ACCEL_INT_MAP_1_BIT_DATA,
+		.en_reg = BMC150_ACCEL_REG_INT_EN_1,
+		.en_bitmask = BMC150_ACCEL_INT_EN_BIT_DATA_EN,
+	},
+	{  /* motion interrupt */
+		.map_reg = BMC150_ACCEL_REG_INT_MAP_0,
+		.map_bitmask = BMC150_ACCEL_INT_MAP_0_BIT_SLOPE,
+		.en_reg = BMC150_ACCEL_REG_INT_EN_0,
+		.en_bitmask =  BMC150_ACCEL_INT_EN_BIT_SLP_X |
+			BMC150_ACCEL_INT_EN_BIT_SLP_Y |
+			BMC150_ACCEL_INT_EN_BIT_SLP_Z
+	},
+};
+
+static int bmc150_accel_set_interrupt(struct bmc150_accel_data *data,
+				      struct bmc150_accel_interrupt_info *info,
+				      bool state)
+{
+	int ret;
+
+	/*
+	 * We will expect the enable and disable to do operation in
+	 * in reverse order. This will happen here anyway as our
+	 * resume operation uses sync mode runtime pm calls, the
+	 * suspend operation will be delayed by autosuspend delay
+	 * So the disable operation will still happen in reverse of
+	 * enable operation. When runtime pm is disabled the mode
+	 * is always on so sequence doesn't matter
+	 */
+	ret = bmc150_accel_set_power_state(data, state);
+	if (ret < 0)
+		return ret;
+
+
+	/* map the interrupt to the appropriate pins */
+	if (info->map_reg) {
+		ret = i2c_smbus_read_byte_data(data->client, info->map_reg);
+		if (ret < 0) {
+			dev_err(&data->client->dev, "Error reading reg_int_map\n");
+			return ret;
+		}
+		if (state)
+			ret |= info->map_bitmask;
+		else
+			ret &= ~info->map_bitmask;
+
+		ret = i2c_smbus_write_byte_data(data->client, info->map_reg,
+						ret);
+		if (ret < 0) {
+			dev_err(&data->client->dev, "Error writing reg_int_map\n");
+			return ret;
+		}
+	}
+
+	/* enable/disable the interrupt */
+	ret = i2c_smbus_read_byte_data(data->client, info->en_reg);
+	if (ret < 0) {
+		dev_err(&data->client->dev, "Error reading reg_int_en\n");
+		return ret;
+	}
+
+	if (state)
+		ret |= info->en_bitmask;
+	else
+		ret &= ~info->en_bitmask;
+
+	ret = i2c_smbus_write_byte_data(data->client, info->en_reg, ret);
+	if (ret < 0) {
+		dev_err(&data->client->dev, "Error writing reg_int_en\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int bmc150_accel_setup_any_motion_interrupt(
+					struct bmc150_accel_data *data,
+					bool status)
+{
+	return bmc150_accel_set_interrupt(data, &bmc150_accel_interrupts[1],
+					  status);
+}
+
+static int bmc150_accel_setup_new_data_interrupt(struct bmc150_accel_data *data,
+					   bool status)
+{
+	return bmc150_accel_set_interrupt(data, &bmc150_accel_interrupts[0],
+					  status);
+}
+
 static int bmc150_accel_set_scale(struct bmc150_accel_data *data, int val)
 {
 	int ret, i;
@@ -808,22 +777,6 @@ static int bmc150_accel_write_event_config(struct iio_dev *indio_dev,
 		return 0;
 	}
 
-	/*
-	 * We will expect the enable and disable to do operation in
-	 * in reverse order. This will happen here anyway as our
-	 * resume operation uses sync mode runtime pm calls, the
-	 * suspend operation will be delayed by autosuspend delay
-	 * So the disable operation will still happen in reverse of
-	 * enable operation. When runtime pm is disabled the mode
-	 * is always on so sequence doesn't matter
-	 */
-
-	ret = bmc150_accel_set_power_state(data, state);
-	if (ret < 0) {
-		mutex_unlock(&data->mutex);
-		return ret;
-	}
-
 	ret =  bmc150_accel_setup_any_motion_interrupt(data, state);
 	if (ret < 0) {
 		mutex_unlock(&data->mutex);
@@ -1055,15 +1008,6 @@ static int bmc150_accel_data_rdy_trigger_set_state(struct iio_trigger *trig,
 		return 0;
 	}
 
-	/*
-	 * Refer to comment in bmc150_accel_write_event_config for
-	 * enable/disable operation order
-	 */
-	ret = bmc150_accel_set_power_state(data, state);
-	if (ret < 0) {
-		mutex_unlock(&data->mutex);
-		return ret;
-	}
 	if (data->motion_trig == trig)
 		ret =  bmc150_accel_setup_any_motion_interrupt(data, state);
 	else
@@ -1240,6 +1184,21 @@ static int bmc150_accel_probe(struct i2c_client *client,
 		if (ret)
 			return ret;
 
+		/*
+		 * Set latched mode interrupt. While certain interrupts are
+		 * non-latched regardless of this settings (e.g. new data) we
+		 * want to use latch mode when we can to prevent interrupt
+		 * flooding.
+		 */
+		ret = i2c_smbus_write_byte_data(data->client,
+						BMC150_ACCEL_REG_INT_RST_LATCH,
+					     BMC150_ACCEL_INT_MODE_LATCH_RESET);
+		if (ret < 0) {
+			dev_err(&data->client->dev, "Error writing reg_int_rst_latch\n");
+			return ret;
+		}
+
+
 		data->dready_trig = devm_iio_trigger_alloc(&client->dev,
 							   "%s-dev%d",
 							   indio_dev->name,
-- 
1.9.1

Re: [RFC 3/8] iio: bmc150: refactor interrupt enabling

[Hartmut Knaack]

Octavian Purdila schrieb am 17.11.2014 18:56:
> This patch combines the any motion and new data interrupts function
> into a single, generic, interrupt enable function. On top of this, we
> can later refactor triggers to make it easier to add new triggers.
> 
> Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
> ---
>  drivers/iio/accel/bmc150-accel.c | 271 +++++++++++++++++----------------------
>  1 file changed, 115 insertions(+), 156 deletions(-)
> 
> diff --git a/drivers/iio/accel/bmc150-accel.c b/drivers/iio/accel/bmc150-accel.c
> index 5bfb09d..ee4afc4 100644
> --- a/drivers/iio/accel/bmc150-accel.c
> +++ b/drivers/iio/accel/bmc150-accel.c
> @@ -144,6 +144,13 @@ struct bmc150_accel_chip_info {
>  	const struct bmc150_scale_info scale_table[4];
>  };
>  
> +struct bmc150_accel_interrupt_info {
> +	u8 map_reg;
> +	u8 map_bitmask;
> +	u8 en_reg;
> +	u8 en_bitmask;
> +};
> +
>  struct bmc150_accel_data {
>  	struct i2c_client *client;
>  	struct iio_trigger *dready_trig;
> @@ -374,137 +381,6 @@ static int bmc150_accel_chip_init(struct bmc150_accel_data *data)
>  	return 0;
>  }
>  
> -static int bmc150_accel_setup_any_motion_interrupt(
> -					struct bmc150_accel_data *data,
> -					bool status)
> -{
> -	int ret;
> -
> -	/* Enable/Disable INT1 mapping */
> -	ret = i2c_smbus_read_byte_data(data->client,
> -				       BMC150_ACCEL_REG_INT_MAP_0);
> -	if (ret < 0) {
> -		dev_err(&data->client->dev, "Error reading reg_int_map_0\n");
> -		return ret;
> -	}
> -	if (status)
> -		ret |= BMC150_ACCEL_INT_MAP_0_BIT_SLOPE;
> -	else
> -		ret &= ~BMC150_ACCEL_INT_MAP_0_BIT_SLOPE;
> -
> -	ret = i2c_smbus_write_byte_data(data->client,
> -					BMC150_ACCEL_REG_INT_MAP_0,
> -					ret);
> -	if (ret < 0) {
> -		dev_err(&data->client->dev, "Error writing reg_int_map_0\n");
> -		return ret;
> -	}
> -
> -	if (status) {
> -		/*
> -		 * New data interrupt is always non-latched,
> -		 * which will have higher priority, so no need
> -		 * to set latched mode, we will be flooded anyway with INTR
> -		 */
> -		if (!data->dready_trigger_on) {
> -			ret = i2c_smbus_write_byte_data(data->client,
> -					BMC150_ACCEL_REG_INT_RST_LATCH,
> -					BMC150_ACCEL_INT_MODE_LATCH_INT |
> -					BMC150_ACCEL_INT_MODE_LATCH_RESET);
> -			if (ret < 0) {
> -				dev_err(&data->client->dev,
> -					"Error writing reg_int_rst_latch\n");
> -				return ret;
> -			}
> -		}
> -
> -		ret = i2c_smbus_write_byte_data(data->client,
> -						BMC150_ACCEL_REG_INT_EN_0,
> -						BMC150_ACCEL_INT_EN_BIT_SLP_X |
> -						BMC150_ACCEL_INT_EN_BIT_SLP_Y |
> -						BMC150_ACCEL_INT_EN_BIT_SLP_Z);
> -	} else
> -		ret = i2c_smbus_write_byte_data(data->client,
> -						BMC150_ACCEL_REG_INT_EN_0,
> -						0);
> -
> -	if (ret < 0) {
> -		dev_err(&data->client->dev, "Error writing reg_int_en_0\n");
> -		return ret;
> -	}
> -
> -	return 0;
> -}
> -
> -static int bmc150_accel_setup_new_data_interrupt(struct bmc150_accel_data *data,
> -					   bool status)
> -{
> -	int ret;
> -
> -	/* Enable/Disable INT1 mapping */
> -	ret = i2c_smbus_read_byte_data(data->client,
> -				       BMC150_ACCEL_REG_INT_MAP_1);
> -	if (ret < 0) {
> -		dev_err(&data->client->dev, "Error reading reg_int_map_1\n");
> -		return ret;
> -	}
> -	if (status)
> -		ret |= BMC150_ACCEL_INT_MAP_1_BIT_DATA;
> -	else
> -		ret &= ~BMC150_ACCEL_INT_MAP_1_BIT_DATA;
> -
> -	ret = i2c_smbus_write_byte_data(data->client,
> -					BMC150_ACCEL_REG_INT_MAP_1,
> -					ret);
> -	if (ret < 0) {
> -		dev_err(&data->client->dev, "Error writing reg_int_map_1\n");
> -		return ret;
> -	}
> -
> -	if (status) {
> -		/*
> -		 * Set non latched mode interrupt and clear any latched
> -		 * interrupt
> -		 */
> -		ret = i2c_smbus_write_byte_data(data->client,
> -					BMC150_ACCEL_REG_INT_RST_LATCH,
> -					BMC150_ACCEL_INT_MODE_NON_LATCH_INT |
> -					BMC150_ACCEL_INT_MODE_LATCH_RESET);
> -		if (ret < 0) {
> -			dev_err(&data->client->dev,
> -				"Error writing reg_int_rst_latch\n");
> -			return ret;
> -		}
> -
> -		ret = i2c_smbus_write_byte_data(data->client,
> -					BMC150_ACCEL_REG_INT_EN_1,
> -					BMC150_ACCEL_INT_EN_BIT_DATA_EN);
> -
> -	} else {
> -		/* Restore default interrupt mode */
> -		ret = i2c_smbus_write_byte_data(data->client,
> -					BMC150_ACCEL_REG_INT_RST_LATCH,
> -					BMC150_ACCEL_INT_MODE_LATCH_INT |
> -					BMC150_ACCEL_INT_MODE_LATCH_RESET);
> -		if (ret < 0) {
> -			dev_err(&data->client->dev,
> -				"Error writing reg_int_rst_latch\n");
> -			return ret;
> -		}
> -
> -		ret = i2c_smbus_write_byte_data(data->client,
> -						BMC150_ACCEL_REG_INT_EN_1,
> -						0);
> -	}
> -
> -	if (ret < 0) {
> -		dev_err(&data->client->dev, "Error writing reg_int_en_1\n");
> -		return ret;
> -	}
> -
> -	return 0;
> -}
> -
>  static int bmc150_accel_get_bw(struct bmc150_accel_data *data, int *val,
>  			       int *val2)
>  {
> @@ -559,6 +435,99 @@ static int bmc150_accel_set_power_state(struct bmc150_accel_data *data, bool on)
>  }
>  #endif
>  
> +static struct bmc150_accel_interrupt_info bmc150_accel_interrupts[] = {
> +	{ /* data ready interrupt */
> +		.map_reg = BMC150_ACCEL_REG_INT_MAP_1,
> +		.map_bitmask = BMC150_ACCEL_INT_MAP_1_BIT_DATA,
> +		.en_reg = BMC150_ACCEL_REG_INT_EN_1,
> +		.en_bitmask = BMC150_ACCEL_INT_EN_BIT_DATA_EN,
> +	},
> +	{  /* motion interrupt */
> +		.map_reg = BMC150_ACCEL_REG_INT_MAP_0,
> +		.map_bitmask = BMC150_ACCEL_INT_MAP_0_BIT_SLOPE,
> +		.en_reg = BMC150_ACCEL_REG_INT_EN_0,
> +		.en_bitmask =  BMC150_ACCEL_INT_EN_BIT_SLP_X |
> +			BMC150_ACCEL_INT_EN_BIT_SLP_Y |
> +			BMC150_ACCEL_INT_EN_BIT_SLP_Z
> +	},
> +};
> +
> +static int bmc150_accel_set_interrupt(struct bmc150_accel_data *data,
> +				      struct bmc150_accel_interrupt_info *info,
> +				      bool state)
> +{
> +	int ret;
> +
> +	/*
> +	 * We will expect the enable and disable to do operation in
> +	 * in reverse order. This will happen here anyway as our
> +	 * resume operation uses sync mode runtime pm calls, the
> +	 * suspend operation will be delayed by autosuspend delay
> +	 * So the disable operation will still happen in reverse of
> +	 * enable operation. When runtime pm is disabled the mode
> +	 * is always on so sequence doesn't matter
> +	 */
> +	ret = bmc150_accel_set_power_state(data, state);
> +	if (ret < 0)
> +		return ret;
> +
> +
> +	/* map the interrupt to the appropriate pins */
> +	if (info->map_reg) {
So, info->map_reg might be set, but info->en_reg will always be set? 
> +		ret = i2c_smbus_read_byte_data(data->client, info->map_reg);
> +		if (ret < 0) {
> +			dev_err(&data->client->dev, "Error reading reg_int_map\n");
> +			return ret;
> +		}
> +		if (state)
> +			ret |= info->map_bitmask;
> +		else
> +			ret &= ~info->map_bitmask;
> +
> +		ret = i2c_smbus_write_byte_data(data->client, info->map_reg,
> +						ret);
> +		if (ret < 0) {
> +			dev_err(&data->client->dev, "Error writing reg_int_map\n");
> +			return ret;
> +		}
> +	}
> +
> +	/* enable/disable the interrupt */
> +	ret = i2c_smbus_read_byte_data(data->client, info->en_reg);
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "Error reading reg_int_en\n");
> +		return ret;
> +	}
> +
> +	if (state)
> +		ret |= info->en_bitmask;
> +	else
> +		ret &= ~info->en_bitmask;
> +
> +	ret = i2c_smbus_write_byte_data(data->client, info->en_reg, ret);
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "Error writing reg_int_en\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int bmc150_accel_setup_any_motion_interrupt(
> +					struct bmc150_accel_data *data,
> +					bool status)
> +{
> +	return bmc150_accel_set_interrupt(data, &bmc150_accel_interrupts[1],
> +					  status);
> +}
> +
> +static int bmc150_accel_setup_new_data_interrupt(struct bmc150_accel_data *data,
> +					   bool status)
> +{
> +	return bmc150_accel_set_interrupt(data, &bmc150_accel_interrupts[0],
> +					  status);
> +}
> +
I don't think these wrappers really provide a benefit.
>  static int bmc150_accel_set_scale(struct bmc150_accel_data *data, int val)
>  {
>  	int ret, i;
> @@ -808,22 +777,6 @@ static int bmc150_accel_write_event_config(struct iio_dev *indio_dev,
>  		return 0;
>  	}
>  
> -	/*
> -	 * We will expect the enable and disable to do operation in
> -	 * in reverse order. This will happen here anyway as our
> -	 * resume operation uses sync mode runtime pm calls, the
> -	 * suspend operation will be delayed by autosuspend delay
> -	 * So the disable operation will still happen in reverse of
> -	 * enable operation. When runtime pm is disabled the mode
> -	 * is always on so sequence doesn't matter
> -	 */
> -
> -	ret = bmc150_accel_set_power_state(data, state);
> -	if (ret < 0) {
> -		mutex_unlock(&data->mutex);
> -		return ret;
> -	}
> -
>  	ret =  bmc150_accel_setup_any_motion_interrupt(data, state);
>  	if (ret < 0) {
>  		mutex_unlock(&data->mutex);
> @@ -1055,15 +1008,6 @@ static int bmc150_accel_data_rdy_trigger_set_state(struct iio_trigger *trig,
>  		return 0;
>  	}
>  
> -	/*
> -	 * Refer to comment in bmc150_accel_write_event_config for
> -	 * enable/disable operation order
> -	 */
> -	ret = bmc150_accel_set_power_state(data, state);
> -	if (ret < 0) {
> -		mutex_unlock(&data->mutex);
> -		return ret;
> -	}
>  	if (data->motion_trig == trig)
>  		ret =  bmc150_accel_setup_any_motion_interrupt(data, state);
>  	else
> @@ -1240,6 +1184,21 @@ static int bmc150_accel_probe(struct i2c_client *client,
>  		if (ret)
>  			return ret;
>  
> +		/*
> +		 * Set latched mode interrupt. While certain interrupts are
> +		 * non-latched regardless of this settings (e.g. new data) we
> +		 * want to use latch mode when we can to prevent interrupt
> +		 * flooding.
> +		 */
> +		ret = i2c_smbus_write_byte_data(data->client,
> +						BMC150_ACCEL_REG_INT_RST_LATCH,
> +					     BMC150_ACCEL_INT_MODE_LATCH_RESET);
> +		if (ret < 0) {
> +			dev_err(&data->client->dev, "Error writing reg_int_rst_latch\n");
> +			return ret;
> +		}
> +
> +
>  		data->dready_trig = devm_iio_trigger_alloc(&client->dev,
>  							   "%s-dev%d",
>  							   indio_dev->name,
> 

Re: [RFC 3/8] iio: bmc150: refactor interrupt enabling

[Octavian Purdila]

On Mon, Nov 24, 2014 at 12:02 AM, Hartmut Knaack <knaack.h@gmx.de> wrote:

>> +static int bmc150_accel_set_interrupt(struct bmc150_accel_data *data,
>> +                                   struct bmc150_accel_interrupt_info *info,
>> +                                   bool state)
>> +{
>> +     int ret;
>> +
>> +     /*
>> +      * We will expect the enable and disable to do operation in
>> +      * in reverse order. This will happen here anyway as our
>> +      * resume operation uses sync mode runtime pm calls, the
>> +      * suspend operation will be delayed by autosuspend delay
>> +      * So the disable operation will still happen in reverse of
>> +      * enable operation. When runtime pm is disabled the mode
>> +      * is always on so sequence doesn't matter
>> +      */
>> +     ret = bmc150_accel_set_power_state(data, state);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +
>> +     /* map the interrupt to the appropriate pins */
>> +     if (info->map_reg) {
> So, info->map_reg might be set, but info->en_reg will always be set?

Good catch, originally I thought map_reg may not be needed, but it
looks like it is not needed after all. I will remove the if in the
next version.

>> +
>> +static int bmc150_accel_setup_any_motion_interrupt(
>> +                                     struct bmc150_accel_data *data,
>> +                                     bool status)
>> +{
>> +     return bmc150_accel_set_interrupt(data, &bmc150_accel_interrupts[1],
>> +                                       status);
>> +}
>> +
>> +static int bmc150_accel_setup_new_data_interrupt(struct bmc150_accel_data *data,
>> +                                        bool status)
>> +{
>> +     return bmc150_accel_set_interrupt(data, &bmc150_accel_interrupts[0],
>> +                                       status);
>> +}
>> +
> I don't think these wrappers really provide a benefit.

They are removed in one of the later patches. I wanted to keep this
patch small, hence I kept the two original functions in this patch.

[RFC 4/8] iio: bmc150: exit early if event / trigger state is not changed

[Octavian Purdila]

Previous of this patch the check was only done if we enabled the event
and it was already enabled. We can do the same if the event is
disabled and we want to disable it.

The patch also adds the same check on the trigger code.

Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
---
 drivers/iio/accel/bmc150-accel.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/accel/bmc150-accel.c b/drivers/iio/accel/bmc150-accel.c
index ee4afc4..30d7bcd 100644
--- a/drivers/iio/accel/bmc150-accel.c
+++ b/drivers/iio/accel/bmc150-accel.c
@@ -766,7 +766,7 @@ static int bmc150_accel_write_event_config(struct iio_dev *indio_dev,
 	struct bmc150_accel_data *data = iio_priv(indio_dev);
 	int ret;
 
-	if (state && data->ev_enable_state)
+	if (state == data->ev_enable_state)
 		return 0;
 
 	mutex_lock(&data->mutex);
@@ -1002,6 +1002,18 @@ static int bmc150_accel_data_rdy_trigger_set_state(struct iio_trigger *trig,
 
 	mutex_lock(&data->mutex);
 
+	if (data->motion_trig == trig) {
+		if (data->motion_trigger_on == state) {
+			mutex_unlock(&data->mutex);
+			return 0;
+		}
+	} else {
+		if (data->dready_trigger_on == state) {
+			mutex_unlock(&data->mutex);
+			return 0;
+		}
+	}
+
 	if (!state && data->ev_enable_state && data->motion_trigger_on) {
 		data->motion_trigger_on = false;
 		mutex_unlock(&data->mutex);
-- 
1.9.1

[RFC 5/8] iio: bmc150: introduce bmc150_accel_interrupt

[Octavian Purdila]

Since both interrupts and events can share an interrupt, add a data
structure that tracks the users of an interrupt so that it enables or
disables it only for the first users and respectively last user.

This will allows us to easily add more events or triggers.

The patch also adds an interrupt enabled counter, so that we can
easily know if we need to put the device in normal mode when the
resume callback is issued.

Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
---
 drivers/iio/accel/bmc150-accel.c | 87 +++++++++++++++++++++-------------------
 1 file changed, 45 insertions(+), 42 deletions(-)

diff --git a/drivers/iio/accel/bmc150-accel.c b/drivers/iio/accel/bmc150-accel.c
index 30d7bcd..5a430dc 100644
--- a/drivers/iio/accel/bmc150-accel.c
+++ b/drivers/iio/accel/bmc150-accel.c
@@ -151,10 +151,19 @@ struct bmc150_accel_interrupt_info {
 	u8 en_bitmask;
 };
 
+struct bmc150_accel_interrupt {
+	struct bmc150_accel_interrupt_info *info;
+	atomic_t users;
+};
+
+#define BMC150_ACCEL_INTERRUPTS		2
+
 struct bmc150_accel_data {
 	struct i2c_client *client;
+	struct bmc150_accel_interrupt interrupts[BMC150_ACCEL_INTERRUPTS];
 	struct iio_trigger *dready_trig;
 	struct iio_trigger *motion_trig;
+	atomic_t active_intr;
 	struct mutex mutex;
 	s16 buffer[8];
 	u8 bw_bits;
@@ -435,7 +444,8 @@ static int bmc150_accel_set_power_state(struct bmc150_accel_data *data, bool on)
 }
 #endif
 
-static struct bmc150_accel_interrupt_info bmc150_accel_interrupts[] = {
+static struct bmc150_accel_interrupt_info
+bmc150_accel_interrupts[BMC150_ACCEL_INTERRUPTS] = {
 	{ /* data ready interrupt */
 		.map_reg = BMC150_ACCEL_REG_INT_MAP_1,
 		.map_bitmask = BMC150_ACCEL_INT_MAP_1_BIT_DATA,
@@ -452,12 +462,30 @@ static struct bmc150_accel_interrupt_info bmc150_accel_interrupts[] = {
 	},
 };
 
+static void bmc150_accel_interrupts_setup(struct iio_dev *indio_dev,
+					 struct bmc150_accel_data *data)
+{
+	int i;
+
+	for (i = 0; i < BMC150_ACCEL_INTERRUPTS; i++)
+		data->interrupts[i].info = &bmc150_accel_interrupts[i];
+}
+
 static int bmc150_accel_set_interrupt(struct bmc150_accel_data *data,
-				      struct bmc150_accel_interrupt_info *info,
+				      struct bmc150_accel_interrupt *intr,
 				      bool state)
 {
+	struct bmc150_accel_interrupt_info *info = intr->info;
 	int ret;
 
+	if (state) {
+		if (atomic_inc_and_test(&intr->users) > 1)
+			return 0;
+	} else {
+		if (atomic_dec_and_test(&intr->users) > 0)
+			return 0;
+	}
+
 	/*
 	 * We will expect the enable and disable to do operation in
 	 * in reverse order. This will happen here anyway as our
@@ -510,22 +538,12 @@ static int bmc150_accel_set_interrupt(struct bmc150_accel_data *data,
 		return ret;
 	}
 
-	return 0;
-}
-
-static int bmc150_accel_setup_any_motion_interrupt(
-					struct bmc150_accel_data *data,
-					bool status)
-{
-	return bmc150_accel_set_interrupt(data, &bmc150_accel_interrupts[1],
-					  status);
-}
+	if (state)
+		atomic_inc(&data->active_intr);
+	else
+		atomic_dec(&data->active_intr);
 
-static int bmc150_accel_setup_new_data_interrupt(struct bmc150_accel_data *data,
-					   bool status)
-{
-	return bmc150_accel_set_interrupt(data, &bmc150_accel_interrupts[0],
-					  status);
+	return 0;
 }
 
 static int bmc150_accel_set_scale(struct bmc150_accel_data *data, int val)
@@ -770,23 +788,12 @@ static int bmc150_accel_write_event_config(struct iio_dev *indio_dev,
 		return 0;
 
 	mutex_lock(&data->mutex);
-
-	if (!state && data->motion_trigger_on) {
-		data->ev_enable_state = 0;
-		mutex_unlock(&data->mutex);
-		return 0;
-	}
-
-	ret =  bmc150_accel_setup_any_motion_interrupt(data, state);
-	if (ret < 0) {
-		mutex_unlock(&data->mutex);
-		return ret;
-	}
-
-	data->ev_enable_state = state;
+	ret = bmc150_accel_set_interrupt(data, &data->interrupts[1], state);
+	if (!ret)
+		data->ev_enable_state = state;
 	mutex_unlock(&data->mutex);
 
-	return 0;
+	return ret;
 }
 
 static int bmc150_accel_validate_trigger(struct iio_dev *indio_dev,
@@ -1014,16 +1021,12 @@ static int bmc150_accel_data_rdy_trigger_set_state(struct iio_trigger *trig,
 		}
 	}
 
-	if (!state && data->ev_enable_state && data->motion_trigger_on) {
-		data->motion_trigger_on = false;
-		mutex_unlock(&data->mutex);
-		return 0;
-	}
-
 	if (data->motion_trig == trig)
-		ret =  bmc150_accel_setup_any_motion_interrupt(data, state);
+		ret = bmc150_accel_set_interrupt(data, &data->interrupts[1],
+						 state);
 	else
-		ret = bmc150_accel_setup_new_data_interrupt(data, state);
+		ret = bmc150_accel_set_interrupt(data, &data->interrupts[0],
+						 state);
 	if (ret < 0) {
 		mutex_unlock(&data->mutex);
 		return ret;
@@ -1210,6 +1213,7 @@ static int bmc150_accel_probe(struct i2c_client *client,
 			return ret;
 		}
 
+		bmc150_accel_interrupts_setup(indio_dev, data);
 
 		data->dready_trig = devm_iio_trigger_alloc(&client->dev,
 							   "%s-dev%d",
@@ -1326,8 +1330,7 @@ static int bmc150_accel_resume(struct device *dev)
 	struct bmc150_accel_data *data = iio_priv(indio_dev);
 
 	mutex_lock(&data->mutex);
-	if (data->dready_trigger_on || data->motion_trigger_on ||
-							data->ev_enable_state)
+	if (atomic_read(&data->active_intr))
 		bmc150_accel_set_mode(data, BMC150_ACCEL_SLEEP_MODE_NORMAL, 0);
 	mutex_unlock(&data->mutex);
 
-- 
1.9.1

[RFC 6/8] iio: bmc150: introduce bmc150_accel_trigger

[Octavian Purdila]

Add a separate structure for triggers and add the infrastructure to
support an arbitrary number of triggers. Each trigger is associated
with an interrupt and has an enabled/disabled state.

Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
---
 drivers/iio/accel/bmc150-accel.c | 175 ++++++++++++++++++++++-----------------
 1 file changed, 101 insertions(+), 74 deletions(-)

diff --git a/drivers/iio/accel/bmc150-accel.c b/drivers/iio/accel/bmc150-accel.c
index 5a430dc..ed02ced 100644
--- a/drivers/iio/accel/bmc150-accel.c
+++ b/drivers/iio/accel/bmc150-accel.c
@@ -156,14 +156,21 @@ struct bmc150_accel_interrupt {
 	atomic_t users;
 };
 
+struct bmc150_accel_trigger {
+	struct bmc150_accel_interrupt *intr;
+	struct bmc150_accel_data *data;
+	struct iio_trigger *indio_trig;
+	bool enabled;
+};
+
 #define BMC150_ACCEL_INTERRUPTS		2
+#define BMC150_ACCEL_TRIGGERS		2
 
 struct bmc150_accel_data {
 	struct i2c_client *client;
 	struct bmc150_accel_interrupt interrupts[BMC150_ACCEL_INTERRUPTS];
-	struct iio_trigger *dready_trig;
-	struct iio_trigger *motion_trig;
 	atomic_t active_intr;
+	struct bmc150_accel_trigger triggers[BMC150_ACCEL_TRIGGERS];
 	struct mutex mutex;
 	s16 buffer[8];
 	u8 bw_bits;
@@ -171,8 +178,6 @@ struct bmc150_accel_data {
 	u32 slope_thres;
 	u32 range;
 	int ev_enable_state;
-	bool dready_trigger_on;
-	bool motion_trigger_on;
 	int64_t timestamp;
 	const struct bmc150_accel_chip_info *chip_info;
 };
@@ -800,11 +805,14 @@ static int bmc150_accel_validate_trigger(struct iio_dev *indio_dev,
 				   struct iio_trigger *trig)
 {
 	struct bmc150_accel_data *data = iio_priv(indio_dev);
+	int i;
 
-	if (data->dready_trig != trig && data->motion_trig != trig)
-		return -EINVAL;
+	for (i = 0; i < BMC150_ACCEL_TRIGGERS; i++) {
+		if (data->triggers[i].indio_trig == trig)
+			return 0;
+	}
 
-	return 0;
+	return -EINVAL;
 }
 
 static IIO_CONST_ATTR_SAMP_FREQ_AVAIL(
@@ -976,12 +984,12 @@ err_read:
 
 static int bmc150_accel_trig_try_reen(struct iio_trigger *trig)
 {
-	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
-	struct bmc150_accel_data *data = iio_priv(indio_dev);
+	struct bmc150_accel_trigger *t = iio_trigger_get_drvdata(trig);
+	struct bmc150_accel_data *data = t->data;
 	int ret;
 
 	/* new data interrupts don't need ack */
-	if (data->dready_trigger_on)
+	if (t == &t->data->triggers[0])
 		return 0;
 
 	mutex_lock(&data->mutex);
@@ -1003,38 +1011,24 @@ static int bmc150_accel_trig_try_reen(struct iio_trigger *trig)
 static int bmc150_accel_data_rdy_trigger_set_state(struct iio_trigger *trig,
 						   bool state)
 {
-	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
-	struct bmc150_accel_data *data = iio_priv(indio_dev);
+	struct bmc150_accel_trigger *t = iio_trigger_get_drvdata(trig);
+	struct bmc150_accel_data *data = t->data;
 	int ret;
 
 	mutex_lock(&data->mutex);
 
-	if (data->motion_trig == trig) {
-		if (data->motion_trigger_on == state) {
-			mutex_unlock(&data->mutex);
-			return 0;
-		}
-	} else {
-		if (data->dready_trigger_on == state) {
-			mutex_unlock(&data->mutex);
-			return 0;
-		}
+	if (t->enabled == state) {
+		mutex_unlock(&data->mutex);
+		return 0;
 	}
 
-	if (data->motion_trig == trig)
-		ret = bmc150_accel_set_interrupt(data, &data->interrupts[1],
-						 state);
-	else
-		ret = bmc150_accel_set_interrupt(data, &data->interrupts[0],
-						 state);
+	ret = bmc150_accel_set_interrupt(data, t->intr, state);
 	if (ret < 0) {
 		mutex_unlock(&data->mutex);
 		return ret;
 	}
-	if (data->motion_trig == trig)
-		data->motion_trigger_on = state;
-	else
-		data->dready_trigger_on = state;
+
+	t->enabled = state;
 
 	mutex_unlock(&data->mutex);
 
@@ -1074,7 +1068,7 @@ static irqreturn_t bmc150_accel_event_handler(int irq, void *private)
 							IIO_EV_DIR_EITHER),
 							data->timestamp);
 ack_intr_status:
-	if (!data->dready_trigger_on)
+	if (!data->triggers[0].enabled)
 		ret = i2c_smbus_write_byte_data(data->client,
 					BMC150_ACCEL_REG_INT_RST_LATCH,
 					BMC150_ACCEL_INT_MODE_LATCH_INT |
@@ -1087,13 +1081,16 @@ static irqreturn_t bmc150_accel_data_rdy_trig_poll(int irq, void *private)
 {
 	struct iio_dev *indio_dev = private;
 	struct bmc150_accel_data *data = iio_priv(indio_dev);
+	int i;
 
 	data->timestamp = iio_get_time_ns();
 
-	if (data->dready_trigger_on)
-		iio_trigger_poll(data->dready_trig);
-	else if (data->motion_trigger_on)
-		iio_trigger_poll(data->motion_trig);
+	for (i = 0; i < BMC150_ACCEL_TRIGGERS; i++) {
+		if (data->triggers[i].enabled) {
+			iio_trigger_poll(data->triggers[i].indio_trig);
+			break;
+		}
+	}
 
 	if (data->ev_enable_state)
 		return IRQ_WAKE_THREAD;
@@ -1145,6 +1142,69 @@ static int bmc150_accel_gpio_probe(struct i2c_client *client,
 	return ret;
 }
 
+static struct {
+	int intr;
+	const char *name;
+} bmc150_accel_triggers[BMC150_ACCEL_TRIGGERS] = {
+	{
+		.intr = 0,
+		.name = "%s-dev%d",
+	},
+	{
+		.intr = 1,
+		.name = "%s-any-motion-dev%d",
+	},
+};
+
+static void bmc150_accel_unregister_triggers(struct bmc150_accel_data *data,
+					     int from)
+{
+	int i;
+
+	for (i = from; i >= 0; i++) {
+		if (data->triggers[i].indio_trig) {
+			iio_trigger_unregister(data->triggers[i].indio_trig);
+			data->triggers[i].indio_trig = NULL;
+		}
+		i--;
+	}
+}
+
+static int bmc150_accel_triggers_setup(struct iio_dev *indio_dev,
+				       struct bmc150_accel_data *data)
+{
+	int i, ret;
+
+	for (i = 0; i < BMC150_ACCEL_TRIGGERS; i++) {
+		struct bmc150_accel_trigger *t = &data->triggers[i];
+		const char *name = bmc150_accel_triggers[i].name;
+		int intr = bmc150_accel_triggers[i].intr;
+
+		t->indio_trig = devm_iio_trigger_alloc(&data->client->dev, name,
+						       indio_dev->name,
+						       indio_dev->id);
+		if (!t->indio_trig) {
+			ret = -ENOMEM;
+			break;
+		}
+
+		t->indio_trig->dev.parent = &data->client->dev;
+		t->indio_trig->ops = &bmc150_accel_trigger_ops;
+		t->intr = &data->interrupts[intr];
+		t->data = data;
+		iio_trigger_set_drvdata(t->indio_trig, t);
+
+		ret = iio_trigger_register(t->indio_trig);
+		if (ret)
+			break;
+	}
+
+	if (ret)
+		bmc150_accel_unregister_triggers(data, i);
+
+	return ret;
+}
+
 static int bmc150_accel_probe(struct i2c_client *client,
 			      const struct i2c_device_id *id)
 {
@@ -1215,36 +1275,10 @@ static int bmc150_accel_probe(struct i2c_client *client,
 
 		bmc150_accel_interrupts_setup(indio_dev, data);
 
-		data->dready_trig = devm_iio_trigger_alloc(&client->dev,
-							   "%s-dev%d",
-							   indio_dev->name,
-							   indio_dev->id);
-		if (!data->dready_trig)
-			return -ENOMEM;
-
-		data->motion_trig = devm_iio_trigger_alloc(&client->dev,
-							  "%s-any-motion-dev%d",
-							  indio_dev->name,
-							  indio_dev->id);
-		if (!data->motion_trig)
-			return -ENOMEM;
-
-		data->dready_trig->dev.parent = &client->dev;
-		data->dready_trig->ops = &bmc150_accel_trigger_ops;
-		iio_trigger_set_drvdata(data->dready_trig, indio_dev);
-		ret = iio_trigger_register(data->dready_trig);
+		ret = bmc150_accel_triggers_setup(indio_dev, data);
 		if (ret)
 			return ret;
 
-		data->motion_trig->dev.parent = &client->dev;
-		data->motion_trig->ops = &bmc150_accel_trigger_ops;
-		iio_trigger_set_drvdata(data->motion_trig, indio_dev);
-		ret = iio_trigger_register(data->motion_trig);
-		if (ret) {
-			data->motion_trig = NULL;
-			goto err_trigger_unregister;
-		}
-
 		ret = iio_triggered_buffer_setup(indio_dev,
 						 &iio_pollfunc_store_time,
 						 bmc150_accel_trigger_handler,
@@ -1276,13 +1310,10 @@ static int bmc150_accel_probe(struct i2c_client *client,
 err_iio_unregister:
 	iio_device_unregister(indio_dev);
 err_buffer_cleanup:
-	if (data->dready_trig)
+	if (indio_dev->pollfunc)
 		iio_triggered_buffer_cleanup(indio_dev);
 err_trigger_unregister:
-	if (data->dready_trig)
-		iio_trigger_unregister(data->dready_trig);
-	if (data->motion_trig)
-		iio_trigger_unregister(data->motion_trig);
+	bmc150_accel_unregister_triggers(data, BMC150_ACCEL_TRIGGERS - 1);
 
 	return ret;
 }
@@ -1298,11 +1329,7 @@ static int bmc150_accel_remove(struct i2c_client *client)
 
 	iio_device_unregister(indio_dev);
 
-	if (data->dready_trig) {
-		iio_triggered_buffer_cleanup(indio_dev);
-		iio_trigger_unregister(data->dready_trig);
-		iio_trigger_unregister(data->motion_trig);
-	}
+	bmc150_accel_unregister_triggers(data, BMC150_ACCEL_TRIGGERS - 1);
 
 	mutex_lock(&data->mutex);
 	bmc150_accel_set_mode(data, BMC150_ACCEL_SLEEP_MODE_DEEP_SUSPEND, 0);
-- 
1.9.1

Re: [RFC 6/8] iio: bmc150: introduce bmc150_accel_trigger

[Hartmut Knaack]

Octavian Purdila schrieb am 17.11.2014 18:56:
> Add a separate structure for triggers and add the infrastructure to
> support an arbitrary number of triggers. Each trigger is associated
> with an interrupt and has an enabled/disabled state.
> 
> Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
> ---
>  drivers/iio/accel/bmc150-accel.c | 175 ++++++++++++++++++++++-----------------
>  1 file changed, 101 insertions(+), 74 deletions(-)
> 
> diff --git a/drivers/iio/accel/bmc150-accel.c b/drivers/iio/accel/bmc150-accel.c
> index 5a430dc..ed02ced 100644
> --- a/drivers/iio/accel/bmc150-accel.c
> +++ b/drivers/iio/accel/bmc150-accel.c
> @@ -156,14 +156,21 @@ struct bmc150_accel_interrupt {
>  	atomic_t users;
>  };
>  
> +struct bmc150_accel_trigger {
> +	struct bmc150_accel_interrupt *intr;
> +	struct bmc150_accel_data *data;
> +	struct iio_trigger *indio_trig;
> +	bool enabled;
> +};
> +
>  #define BMC150_ACCEL_INTERRUPTS		2
> +#define BMC150_ACCEL_TRIGGERS		2
>  
>  struct bmc150_accel_data {
>  	struct i2c_client *client;
>  	struct bmc150_accel_interrupt interrupts[BMC150_ACCEL_INTERRUPTS];
> -	struct iio_trigger *dready_trig;
> -	struct iio_trigger *motion_trig;
>  	atomic_t active_intr;
> +	struct bmc150_accel_trigger triggers[BMC150_ACCEL_TRIGGERS];
>  	struct mutex mutex;
>  	s16 buffer[8];
>  	u8 bw_bits;
> @@ -171,8 +178,6 @@ struct bmc150_accel_data {
>  	u32 slope_thres;
>  	u32 range;
>  	int ev_enable_state;
> -	bool dready_trigger_on;
> -	bool motion_trigger_on;
>  	int64_t timestamp;
>  	const struct bmc150_accel_chip_info *chip_info;
>  };
> @@ -800,11 +805,14 @@ static int bmc150_accel_validate_trigger(struct iio_dev *indio_dev,
>  				   struct iio_trigger *trig)
>  {
>  	struct bmc150_accel_data *data = iio_priv(indio_dev);
> +	int i;
>  
> -	if (data->dready_trig != trig && data->motion_trig != trig)
> -		return -EINVAL;
> +	for (i = 0; i < BMC150_ACCEL_TRIGGERS; i++) {
> +		if (data->triggers[i].indio_trig == trig)
> +			return 0;
> +	}
>  
> -	return 0;
> +	return -EINVAL;
>  }
>  
>  static IIO_CONST_ATTR_SAMP_FREQ_AVAIL(
> @@ -976,12 +984,12 @@ err_read:
>  
>  static int bmc150_accel_trig_try_reen(struct iio_trigger *trig)
>  {
> -	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> -	struct bmc150_accel_data *data = iio_priv(indio_dev);
> +	struct bmc150_accel_trigger *t = iio_trigger_get_drvdata(trig);
> +	struct bmc150_accel_data *data = t->data;
>  	int ret;
>  
>  	/* new data interrupts don't need ack */
> -	if (data->dready_trigger_on)
> +	if (t == &t->data->triggers[0])
>  		return 0;
>  
>  	mutex_lock(&data->mutex);
> @@ -1003,38 +1011,24 @@ static int bmc150_accel_trig_try_reen(struct iio_trigger *trig)
>  static int bmc150_accel_data_rdy_trigger_set_state(struct iio_trigger *trig,
>  						   bool state)
>  {
> -	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> -	struct bmc150_accel_data *data = iio_priv(indio_dev);
> +	struct bmc150_accel_trigger *t = iio_trigger_get_drvdata(trig);
> +	struct bmc150_accel_data *data = t->data;
>  	int ret;
>  
>  	mutex_lock(&data->mutex);
>  
> -	if (data->motion_trig == trig) {
> -		if (data->motion_trigger_on == state) {
> -			mutex_unlock(&data->mutex);
> -			return 0;
> -		}
> -	} else {
> -		if (data->dready_trigger_on == state) {
> -			mutex_unlock(&data->mutex);
> -			return 0;
> -		}
> +	if (t->enabled == state) {
> +		mutex_unlock(&data->mutex);
> +		return 0;
>  	}
>  
> -	if (data->motion_trig == trig)
> -		ret = bmc150_accel_set_interrupt(data, &data->interrupts[1],
> -						 state);
> -	else
> -		ret = bmc150_accel_set_interrupt(data, &data->interrupts[0],
> -						 state);
> +	ret = bmc150_accel_set_interrupt(data, t->intr, state);
>  	if (ret < 0) {
>  		mutex_unlock(&data->mutex);
>  		return ret;
>  	}
> -	if (data->motion_trig == trig)
> -		data->motion_trigger_on = state;
> -	else
> -		data->dready_trigger_on = state;
> +
> +	t->enabled = state;
>  
>  	mutex_unlock(&data->mutex);
>  
> @@ -1074,7 +1068,7 @@ static irqreturn_t bmc150_accel_event_handler(int irq, void *private)
>  							IIO_EV_DIR_EITHER),
>  							data->timestamp);
>  ack_intr_status:
> -	if (!data->dready_trigger_on)
> +	if (!data->triggers[0].enabled)
>  		ret = i2c_smbus_write_byte_data(data->client,
>  					BMC150_ACCEL_REG_INT_RST_LATCH,
>  					BMC150_ACCEL_INT_MODE_LATCH_INT |
> @@ -1087,13 +1081,16 @@ static irqreturn_t bmc150_accel_data_rdy_trig_poll(int irq, void *private)
>  {
>  	struct iio_dev *indio_dev = private;
>  	struct bmc150_accel_data *data = iio_priv(indio_dev);
> +	int i;
>  
>  	data->timestamp = iio_get_time_ns();
>  
> -	if (data->dready_trigger_on)
> -		iio_trigger_poll(data->dready_trig);
> -	else if (data->motion_trigger_on)
> -		iio_trigger_poll(data->motion_trig);
> +	for (i = 0; i < BMC150_ACCEL_TRIGGERS; i++) {
> +		if (data->triggers[i].enabled) {
> +			iio_trigger_poll(data->triggers[i].indio_trig);
> +			break;
> +		}
> +	}
>  
>  	if (data->ev_enable_state)
>  		return IRQ_WAKE_THREAD;
> @@ -1145,6 +1142,69 @@ static int bmc150_accel_gpio_probe(struct i2c_client *client,
>  	return ret;
>  }
>  
> +static struct {
> +	int intr;
> +	const char *name;
> +} bmc150_accel_triggers[BMC150_ACCEL_TRIGGERS] = {
> +	{
> +		.intr = 0,
> +		.name = "%s-dev%d",
> +	},
> +	{
> +		.intr = 1,
> +		.name = "%s-any-motion-dev%d",
> +	},
> +};
> +
> +static void bmc150_accel_unregister_triggers(struct bmc150_accel_data *data,
> +					     int from)
> +{
> +	int i;
> +
> +	for (i = from; i >= 0; i++) {
> +		if (data->triggers[i].indio_trig) {
> +			iio_trigger_unregister(data->triggers[i].indio_trig);
> +			data->triggers[i].indio_trig = NULL;
Better use devm_iio_trigger_free()?
> +		}
> +		i--;
> +	}
i++ and i-- in the same loop looks like an infinite loop to me. Not to mention what happens on the second attempt to unregister the same trigger.
> +}
> +
> +static int bmc150_accel_triggers_setup(struct iio_dev *indio_dev,
> +				       struct bmc150_accel_data *data)
> +{
> +	int i, ret;
> +
> +	for (i = 0; i < BMC150_ACCEL_TRIGGERS; i++) {
> +		struct bmc150_accel_trigger *t = &data->triggers[i];
> +		const char *name = bmc150_accel_triggers[i].name;
> +		int intr = bmc150_accel_triggers[i].intr;
> +
> +		t->indio_trig = devm_iio_trigger_alloc(&data->client->dev, name,
> +						       indio_dev->name,
> +						       indio_dev->id);
> +		if (!t->indio_trig) {
> +			ret = -ENOMEM;
> +			break;
> +		}
> +
> +		t->indio_trig->dev.parent = &data->client->dev;
> +		t->indio_trig->ops = &bmc150_accel_trigger_ops;
> +		t->intr = &data->interrupts[intr];
> +		t->data = data;
> +		iio_trigger_set_drvdata(t->indio_trig, t);
> +
> +		ret = iio_trigger_register(t->indio_trig);
> +		if (ret)
> +			break;
> +	}
> +
> +	if (ret)
> +		bmc150_accel_unregister_triggers(data, i);
I think this should be called with i - 1.
> +
> +	return ret;
> +}
> +
>  static int bmc150_accel_probe(struct i2c_client *client,
>  			      const struct i2c_device_id *id)
>  {
> @@ -1215,36 +1275,10 @@ static int bmc150_accel_probe(struct i2c_client *client,
>  
>  		bmc150_accel_interrupts_setup(indio_dev, data);
>  
> -		data->dready_trig = devm_iio_trigger_alloc(&client->dev,
> -							   "%s-dev%d",
> -							   indio_dev->name,
> -							   indio_dev->id);
> -		if (!data->dready_trig)
> -			return -ENOMEM;
> -
> -		data->motion_trig = devm_iio_trigger_alloc(&client->dev,
> -							  "%s-any-motion-dev%d",
> -							  indio_dev->name,
> -							  indio_dev->id);
> -		if (!data->motion_trig)
> -			return -ENOMEM;
> -
> -		data->dready_trig->dev.parent = &client->dev;
> -		data->dready_trig->ops = &bmc150_accel_trigger_ops;
> -		iio_trigger_set_drvdata(data->dready_trig, indio_dev);
> -		ret = iio_trigger_register(data->dready_trig);
> +		ret = bmc150_accel_triggers_setup(indio_dev, data);
>  		if (ret)
>  			return ret;
>  
> -		data->motion_trig->dev.parent = &client->dev;
> -		data->motion_trig->ops = &bmc150_accel_trigger_ops;
> -		iio_trigger_set_drvdata(data->motion_trig, indio_dev);
> -		ret = iio_trigger_register(data->motion_trig);
> -		if (ret) {
> -			data->motion_trig = NULL;
> -			goto err_trigger_unregister;
> -		}
> -
>  		ret = iio_triggered_buffer_setup(indio_dev,
>  						 &iio_pollfunc_store_time,
>  						 bmc150_accel_trigger_handler,
> @@ -1276,13 +1310,10 @@ static int bmc150_accel_probe(struct i2c_client *client,
>  err_iio_unregister:
>  	iio_device_unregister(indio_dev);
>  err_buffer_cleanup:
> -	if (data->dready_trig)
> +	if (indio_dev->pollfunc)
>  		iio_triggered_buffer_cleanup(indio_dev);
>  err_trigger_unregister:
> -	if (data->dready_trig)
> -		iio_trigger_unregister(data->dready_trig);
> -	if (data->motion_trig)
> -		iio_trigger_unregister(data->motion_trig);
> +	bmc150_accel_unregister_triggers(data, BMC150_ACCEL_TRIGGERS - 1);
>  
>  	return ret;
>  }
> @@ -1298,11 +1329,7 @@ static int bmc150_accel_remove(struct i2c_client *client)
>  
>  	iio_device_unregister(indio_dev);
>  
> -	if (data->dready_trig) {
> -		iio_triggered_buffer_cleanup(indio_dev);
> -		iio_trigger_unregister(data->dready_trig);
> -		iio_trigger_unregister(data->motion_trig);
> -	}
> +	bmc150_accel_unregister_triggers(data, BMC150_ACCEL_TRIGGERS - 1);
>  
>  	mutex_lock(&data->mutex);
>  	bmc150_accel_set_mode(data, BMC150_ACCEL_SLEEP_MODE_DEEP_SUSPEND, 0);
> 

Re: [RFC 6/8] iio: bmc150: introduce bmc150_accel_trigger

[Octavian Purdila]

On Mon, Nov 24, 2014 at 1:06 AM, Hartmut Knaack <knaack.h@gmx.de> wrote:
> > +
> > +static void bmc150_accel_unregister_triggers(struct bmc150_accel_data *data,
> > +                                          int from)
> > +{
> > +     int i;
> > +
> > +     for (i = from; i >= 0; i++) {
> > +             if (data->triggers[i].indio_trig) {
> > +                     iio_trigger_unregister(data->triggers[i].indio_trig);
> > +                     data->triggers[i].indio_trig = NULL;
> Better use devm_iio_trigger_free()?

Wouldn't that be called anyway a little bit later?

> > +             }
> > +             i--;
> > +     }
> i++ and i-- in the same loop looks like an infinite loop to me. Not to mention what happens on the second attempt to unregister the same trigger.

Oops.

> > +}
> > +
> > +static int bmc150_accel_triggers_setup(struct iio_dev *indio_dev,
> > +                                    struct bmc150_accel_data *data)
> > +{
> > +     int i, ret;
> > +
> > +     for (i = 0; i < BMC150_ACCEL_TRIGGERS; i++) {
> > +             struct bmc150_accel_trigger *t = &data->triggers[i];
> > +             const char *name = bmc150_accel_triggers[i].name;
> > +             int intr = bmc150_accel_triggers[i].intr;
> > +
> > +             t->indio_trig = devm_iio_trigger_alloc(&data->client->dev, name,
> > +                                                    indio_dev->name,
> > +                                                    indio_dev->id);
> > +             if (!t->indio_trig) {
> > +                     ret = -ENOMEM;
> > +                     break;
> > +             }
> > +
> > +             t->indio_trig->dev.parent = &data->client->dev;
> > +             t->indio_trig->ops = &bmc150_accel_trigger_ops;
> > +             t->intr = &data->interrupts[intr];
> > +             t->data = data;
> > +             iio_trigger_set_drvdata(t->indio_trig, t);
> > +
> > +             ret = iio_trigger_register(t->indio_trig);
> > +             if (ret)
> > +                     break;
> > +     }
> > +
> > +     if (ret)
> > +             bmc150_accel_unregister_triggers(data, i);
> I think this should be called with i - 1.

You're right.

Thanks for the reviews Hartmut, I will send a new version after we
decide on the hardware buffer approach.

Re: [RFC 6/8] iio: bmc150: introduce bmc150_accel_trigger

[Hartmut Knaack]

Octavian Purdila schrieb am 24.11.2014 11:42:
> On Mon, Nov 24, 2014 at 1:06 AM, Hartmut Knaack <knaack.h@gmx.de> wrote:
>>> +
>>> +static void bmc150_accel_unregister_triggers(struct bmc150_accel_data *data,
>>> +                                          int from)
>>> +{
>>> +     int i;
>>> +
>>> +     for (i = from; i >= 0; i++) {
>>> +             if (data->triggers[i].indio_trig) {
>>> +                     iio_trigger_unregister(data->triggers[i].indio_trig);
>>> +                     data->triggers[i].indio_trig = NULL;
>> Better use devm_iio_trigger_free()?
> 
> Wouldn't that be called anyway a little bit later?
I thought it would be cleaner/safer to release it using the opposite of devm_iio_trigger_alloc(), instead of changing its reference to NULL.
> 
>>> +             }
>>> +             i--;
>>> +     }
>> i++ and i-- in the same loop looks like an infinite loop to me. Not to mention what happens on the second attempt to unregister the same trigger.
> 
> Oops.
> 
>>> +}
>>> +
>>> +static int bmc150_accel_triggers_setup(struct iio_dev *indio_dev,
>>> +                                    struct bmc150_accel_data *data)
>>> +{
>>> +     int i, ret;
>>> +
>>> +     for (i = 0; i < BMC150_ACCEL_TRIGGERS; i++) {
>>> +             struct bmc150_accel_trigger *t = &data->triggers[i];
>>> +             const char *name = bmc150_accel_triggers[i].name;
>>> +             int intr = bmc150_accel_triggers[i].intr;
>>> +
>>> +             t->indio_trig = devm_iio_trigger_alloc(&data->client->dev, name,
>>> +                                                    indio_dev->name,
>>> +                                                    indio_dev->id);
>>> +             if (!t->indio_trig) {
>>> +                     ret = -ENOMEM;
>>> +                     break;
>>> +             }
>>> +
>>> +             t->indio_trig->dev.parent = &data->client->dev;
>>> +             t->indio_trig->ops = &bmc150_accel_trigger_ops;
>>> +             t->intr = &data->interrupts[intr];
>>> +             t->data = data;
>>> +             iio_trigger_set_drvdata(t->indio_trig, t);
>>> +
>>> +             ret = iio_trigger_register(t->indio_trig);
>>> +             if (ret)
>>> +                     break;
>>> +     }
>>> +
>>> +     if (ret)
>>> +             bmc150_accel_unregister_triggers(data, i);
>> I think this should be called with i - 1.
> 
> You're right.
> 
> Thanks for the reviews Hartmut, I will send a new version after we
> decide on the hardware buffer approach.
> --
> 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: [RFC 6/8] iio: bmc150: introduce bmc150_accel_trigger

[Octavian Purdila]

On Mon, Nov 24, 2014 at 10:26 PM, Hartmut Knaack <knaack.h@gmx.de> wrote:
> Octavian Purdila schrieb am 24.11.2014 11:42:
>> On Mon, Nov 24, 2014 at 1:06 AM, Hartmut Knaack <knaack.h@gmx.de> wrote:
>>>> +
>>>> +static void bmc150_accel_unregister_triggers(struct bmc150_accel_data *data,
>>>> +                                          int from)
>>>> +{
>>>> +     int i;
>>>> +
>>>> +     for (i = from; i >= 0; i++) {
>>>> +             if (data->triggers[i].indio_trig) {
>>>> +                     iio_trigger_unregister(data->triggers[i].indio_trig);
>>>> +                     data->triggers[i].indio_trig = NULL;
>>> Better use devm_iio_trigger_free()?
>>
>> Wouldn't that be called anyway a little bit later?
>
> I thought it would be cleaner/safer to release it using the opposite of devm_iio_trigger_alloc(), instead of changing its reference to NULL.

I thought bmc150_accel_unregister_triggers could be called twice, but
reviewing the code again I see that can't happen. I guess I can remove
the NULL assignment altogether.

[RFC 7/8] iio: bmc150: introduce bmc150_accel_event

[Octavian Purdila]

Add a separate structure for events and add the infrastructure to
support an arbitrary number of events. Each event is associated
with an interrupt and has an enabled/disabled state.

Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
---
 drivers/iio/accel/bmc150-accel.c | 177 ++++++++++++++++++++++++++++++---------
 1 file changed, 139 insertions(+), 38 deletions(-)

diff --git a/drivers/iio/accel/bmc150-accel.c b/drivers/iio/accel/bmc150-accel.c
index ed02ced..72d2dca 100644
--- a/drivers/iio/accel/bmc150-accel.c
+++ b/drivers/iio/accel/bmc150-accel.c
@@ -163,21 +163,37 @@ struct bmc150_accel_trigger {
 	bool enabled;
 };
 
+struct bmc150_accel_event {
+	struct bmc150_accel_interrupt *intr;
+	struct bmc150_accel_data *data;
+	enum iio_event_type type;
+	bool enabled;
+	int (*read)(struct bmc150_accel_event *event, enum iio_event_info info,
+		    int *val, int *val2);
+	int (*write)(struct bmc150_accel_event *event, enum iio_event_info info,
+		     int val, int val2);
+	union {
+		struct {
+			u32 duration;
+			u32 threshold;
+		} slope;
+	};
+};
+
 #define BMC150_ACCEL_INTERRUPTS		2
 #define BMC150_ACCEL_TRIGGERS		2
+#define BMC150_ACCEL_EVENTS		1
 
 struct bmc150_accel_data {
 	struct i2c_client *client;
 	struct bmc150_accel_interrupt interrupts[BMC150_ACCEL_INTERRUPTS];
 	atomic_t active_intr;
 	struct bmc150_accel_trigger triggers[BMC150_ACCEL_TRIGGERS];
+	struct bmc150_accel_event events[BMC150_ACCEL_EVENTS];
 	struct mutex mutex;
 	s16 buffer[8];
 	u8 bw_bits;
-	u32 slope_dur;
-	u32 slope_thres;
 	u32 range;
-	int ev_enable_state;
 	int64_t timestamp;
 	const struct bmc150_accel_chip_info *chip_info;
 };
@@ -287,7 +303,8 @@ static int bmc150_accel_set_bw(struct bmc150_accel_data *data, int val,
 	return -EINVAL;
 }
 
-static int bmc150_accel_update_slope_threshold(struct bmc150_accel_data *data,
+static int bmc150_accel_update_slope_threshold(struct bmc150_accel_event *event,
+					       struct bmc150_accel_data *data,
 					       int val)
 {
 	int ret = 0;
@@ -300,14 +317,15 @@ static int bmc150_accel_update_slope_threshold(struct bmc150_accel_data *data,
 		dev_err(&data->client->dev, "Error writing reg_int_6\n");
 		return ret;
 	}
-	data->slope_thres = val;
+	event->slope.threshold = val;
 
 	dev_dbg(&data->client->dev, "%s: %x\n", __func__, val);
 
 	return ret;
 }
 
-static int bmc150_accel_update_slope_duration(struct bmc150_accel_data *data,
+static int bmc150_accel_update_slope_duration(struct bmc150_accel_event *event,
+					      struct bmc150_accel_data *data,
 					      int val)
 {
 	int ret;
@@ -325,7 +343,7 @@ static int bmc150_accel_update_slope_duration(struct bmc150_accel_data *data,
 		dev_err(&data->client->dev, "Error write reg_int_5\n");
 		return ret;
 	}
-	data->slope_dur = val;
+	event->slope.duration = val;
 
 	dev_dbg(&data->client->dev, "%s: %x\n", __func__, val);
 
@@ -371,12 +389,12 @@ static int bmc150_accel_chip_init(struct bmc150_accel_data *data)
 	data->range = BMC150_ACCEL_DEF_RANGE_4G;
 
 	/* Set default slope duration and thresholds */
-	ret = bmc150_accel_update_slope_duration(data,
+	ret = bmc150_accel_update_slope_duration(&data->events[0], data,
 					 BMC150_ACCEL_DEF_SLOPE_DURATION);
 	if (ret < 0)
 		return ret;
 
-	ret = bmc150_accel_update_slope_threshold(data,
+	ret = bmc150_accel_update_slope_threshold(&data->events[0], data,
 					  BMC150_ACCEL_DEF_SLOPE_THRESHOLD);
 	if (ret < 0)
 		return ret;
@@ -714,22 +732,30 @@ static int bmc150_accel_write_raw(struct iio_dev *indio_dev,
 	return ret;
 }
 
-static int bmc150_accel_read_event(struct iio_dev *indio_dev,
-				   const struct iio_chan_spec *chan,
-				   enum iio_event_type type,
-				   enum iio_event_direction dir,
-				   enum iio_event_info info,
-				   int *val, int *val2)
+static struct bmc150_accel_event*
+bmc150_accel_get_event(struct iio_dev *indio_dev, enum iio_event_type type)
 {
 	struct bmc150_accel_data *data = iio_priv(indio_dev);
+	int i;
+
+	for (i = 0; i < BMC150_ACCEL_EVENTS; i++)
+		if (data->events[i].type == type)
+			return &data->events[i];
+
+	return NULL;
+}
 
+static int bmc150_accel_event_roc_read(struct bmc150_accel_event *event,
+				       enum iio_event_info info,
+				       int *val, int *val2)
+{
 	*val2 = 0;
 	switch (info) {
 	case IIO_EV_INFO_VALUE:
-		*val = data->slope_thres;
+		*val = event->slope.threshold;
 		break;
 	case IIO_EV_INFO_PERIOD:
-		*val = data->slope_dur;
+		*val = event->slope.duration;
 		break;
 	default:
 		return -EINVAL;
@@ -738,28 +764,40 @@ static int bmc150_accel_read_event(struct iio_dev *indio_dev,
 	return IIO_VAL_INT;
 }
 
-static int bmc150_accel_write_event(struct iio_dev *indio_dev,
-				    const struct iio_chan_spec *chan,
-				    enum iio_event_type type,
-				    enum iio_event_direction dir,
-				    enum iio_event_info info,
-				    int val, int val2)
+static int bmc150_accel_read_event(struct iio_dev *indio_dev,
+				   const struct iio_chan_spec *chan,
+				   enum iio_event_type type,
+				   enum iio_event_direction dir,
+				   enum iio_event_info info,
+				   int *val, int *val2)
 {
-	struct bmc150_accel_data *data = iio_priv(indio_dev);
-	int ret;
+	struct bmc150_accel_event *event;
 
-	if (data->ev_enable_state)
-		return -EBUSY;
+	event = bmc150_accel_get_event(indio_dev, type);
+	if (!event || !event->read)
+		return -EINVAL;
+
+	return event->read(event, info, val, val2);
+}
+
+static int bmc150_accel_event_roc_write(struct bmc150_accel_event *event,
+					enum iio_event_info info,
+					int val, int val2)
+{
+	struct bmc150_accel_data *data = event->data;
+	int ret;
 
 	switch (info) {
 	case IIO_EV_INFO_VALUE:
 		mutex_lock(&data->mutex);
-		ret = bmc150_accel_update_slope_threshold(data, val);
+		ret = bmc150_accel_update_slope_threshold(event, event->data,
+							  val);
 		mutex_unlock(&data->mutex);
 		break;
 	case IIO_EV_INFO_PERIOD:
 		mutex_lock(&data->mutex);
-		ret = bmc150_accel_update_slope_duration(data, val);
+		ret = bmc150_accel_update_slope_duration(event, event->data,
+							 val);
 		mutex_unlock(&data->mutex);
 		break;
 	default:
@@ -769,15 +807,37 @@ static int bmc150_accel_write_event(struct iio_dev *indio_dev,
 	return ret;
 }
 
+static int bmc150_accel_write_event(struct iio_dev *indio_dev,
+				    const struct iio_chan_spec *chan,
+				    enum iio_event_type type,
+				    enum iio_event_direction dir,
+				    enum iio_event_info info,
+				    int val, int val2)
+{
+	struct bmc150_accel_event *event;
+
+	event = bmc150_accel_get_event(indio_dev, type);
+	if (!event || !event->write)
+		return -EINVAL;
+
+	if (event->enabled)
+		return -EBUSY;
+
+	return event->write(event, info, val, val2);
+}
+
 static int bmc150_accel_read_event_config(struct iio_dev *indio_dev,
 					  const struct iio_chan_spec *chan,
 					  enum iio_event_type type,
 					  enum iio_event_direction dir)
 {
+	struct bmc150_accel_event *event;
 
-	struct bmc150_accel_data *data = iio_priv(indio_dev);
+	event = bmc150_accel_get_event(indio_dev, type);
+	if (!event)
+		return -EINVAL;
 
-	return data->ev_enable_state;
+	return event->enabled;
 }
 
 static int bmc150_accel_write_event_config(struct iio_dev *indio_dev,
@@ -787,15 +847,20 @@ static int bmc150_accel_write_event_config(struct iio_dev *indio_dev,
 					   int state)
 {
 	struct bmc150_accel_data *data = iio_priv(indio_dev);
+	struct bmc150_accel_event *event;
 	int ret;
 
-	if (state == data->ev_enable_state)
+	event = bmc150_accel_get_event(indio_dev, type);
+	if (!event)
+		return -EINVAL;
+
+	if (state == event->enabled)
 		return 0;
 
 	mutex_lock(&data->mutex);
-	ret = bmc150_accel_set_interrupt(data, &data->interrupts[1], state);
+	ret = bmc150_accel_set_interrupt(data, event->intr, state);
 	if (!ret)
-		data->ev_enable_state = state;
+		event->enabled = state;
 	mutex_unlock(&data->mutex);
 
 	return ret;
@@ -827,12 +892,14 @@ static const struct attribute_group bmc150_accel_attrs_group = {
 	.attrs = bmc150_accel_attributes,
 };
 
-static const struct iio_event_spec bmc150_accel_event = {
+static const struct iio_event_spec bmc150_accel_events[BMC150_ACCEL_EVENTS] = {
+	{
 		.type = IIO_EV_TYPE_ROC,
 		.dir = IIO_EV_DIR_RISING | IIO_EV_DIR_FALLING,
 		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
 				 BIT(IIO_EV_INFO_ENABLE) |
 				 BIT(IIO_EV_INFO_PERIOD)
+	},
 };
 
 #define BMC150_ACCEL_CHANNEL(_axis, bits) {				\
@@ -849,8 +916,8 @@ static const struct iio_event_spec bmc150_accel_event = {
 		.storagebits = 16,					\
 		.shift = 16 - (bits),					\
 	},								\
-	.event_spec = &bmc150_accel_event,				\
-	.num_event_specs = 1						\
+	.event_spec = bmc150_accel_events,				\
+	.num_event_specs = ARRAY_SIZE(bmc150_accel_events)		\
 }
 
 #define BMC150_ACCEL_CHANNELS(bits) {					\
@@ -1092,7 +1159,7 @@ static irqreturn_t bmc150_accel_data_rdy_trig_poll(int irq, void *private)
 		}
 	}
 
-	if (data->ev_enable_state)
+	if (data->events[0].enabled)
 		return IRQ_WAKE_THREAD;
 	else
 		return IRQ_HANDLED;
@@ -1205,6 +1272,38 @@ static int bmc150_accel_triggers_setup(struct iio_dev *indio_dev,
 	return ret;
 }
 
+static struct {
+	int intr;
+	enum iio_event_type type;
+	int (*read)(struct bmc150_accel_event *event, enum iio_event_info info,
+		    int *val, int *val2);
+	int (*write)(struct bmc150_accel_event *event, enum iio_event_info info,
+		     int val, int val2);
+} bmc150_accel_events_info[BMC150_ACCEL_EVENTS] = {
+	{
+		.intr = 1,
+		.type = IIO_EV_TYPE_ROC,
+		.read = bmc150_accel_event_roc_read,
+		.write = bmc150_accel_event_roc_write,
+	},
+};
+
+static void bmc150_accel_events_setup(struct iio_dev *indio_dev,
+				      struct bmc150_accel_data *data)
+{
+	int i;
+
+	for (i = 0; i < BMC150_ACCEL_EVENTS; i++) {
+		int intr = bmc150_accel_events_info[i].intr;
+
+		data->events[i].intr = &data->interrupts[intr];
+		data->events[i].data = data;
+		data->events[i].type = bmc150_accel_events_info[i].type;
+		data->events[i].read = bmc150_accel_events_info[i].read;
+		data->events[i].write = bmc150_accel_events_info[i].write;
+	}
+}
+
 static int bmc150_accel_probe(struct i2c_client *client,
 			      const struct i2c_device_id *id)
 {
@@ -1279,6 +1378,8 @@ static int bmc150_accel_probe(struct i2c_client *client,
 		if (ret)
 			return ret;
 
+		bmc150_accel_events_setup(indio_dev, data);
+
 		ret = iio_triggered_buffer_setup(indio_dev,
 						 &iio_pollfunc_store_time,
 						 bmc150_accel_trigger_handler,
-- 
1.9.1

[RFC 8/8] iio: bmc150: add support for hardware fifo

[Octavian Purdila]

This patch adds support for hardware fifo. Fifo and stream mode are
supported as well as fifo-full and fifo-watermark events.

Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
---
 drivers/iio/accel/bmc150-accel.c | 249 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 243 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/accel/bmc150-accel.c b/drivers/iio/accel/bmc150-accel.c
index 72d2dca..404cfec 100644
--- a/drivers/iio/accel/bmc150-accel.c
+++ b/drivers/iio/accel/bmc150-accel.c
@@ -67,7 +67,9 @@
 #define BMC150_ACCEL_INT_MAP_0_BIT_SLOPE	BIT(2)
 
 #define BMC150_ACCEL_REG_INT_MAP_1		0x1A
-#define BMC150_ACCEL_INT_MAP_1_BIT_DATA	BIT(0)
+#define BMC150_ACCEL_INT_MAP_1_BIT_DATA		BIT(0)
+#define BMC150_ACCEL_INT_MAP_1_BIT_FWM		BIT(1)
+#define BMC150_ACCEL_INT_MAP_1_BIT_FFULL	BIT(2)
 
 #define BMC150_ACCEL_REG_INT_RST_LATCH		0x21
 #define BMC150_ACCEL_INT_MODE_LATCH_RESET	0x80
@@ -80,7 +82,9 @@
 #define BMC150_ACCEL_INT_EN_BIT_SLP_Z		BIT(2)
 
 #define BMC150_ACCEL_REG_INT_EN_1		0x17
-#define BMC150_ACCEL_INT_EN_BIT_DATA_EN	BIT(4)
+#define BMC150_ACCEL_INT_EN_BIT_DATA_EN		BIT(4)
+#define BMC150_ACCEL_INT_EN_BIT_FFULL_EN	BIT(5)
+#define BMC150_ACCEL_INT_EN_BIT_FWM_EN		BIT(6)
 
 #define BMC150_ACCEL_REG_INT_OUT_CTRL		0x20
 #define BMC150_ACCEL_INT_OUT_CTRL_INT1_LVL	BIT(0)
@@ -119,6 +123,13 @@
 #define BMC150_ACCEL_AXIS_TO_REG(axis)	(BMC150_ACCEL_REG_XOUT_L + (axis * 2))
 #define BMC150_AUTO_SUSPEND_DELAY_MS		2000
 
+#define BMC150_ACCEL_REG_FIFO_STATUS		0x0E
+#define BMC150_ACCEL_REG_FIFO_CONFIG0		0x30
+#define BMC150_ACCEL_REG_FIFO_CONFIG1		0x3E
+#define BMC150_ACCEL_REG_FIFO_DATA		0x3F
+#define BMC150_ACCEL_REG_FIFO_
+#define BMC150_ACCEL_FIFO_LENGTH		32
+
 enum bmc150_accel_axis {
 	AXIS_X,
 	AXIS_Y,
@@ -177,12 +188,13 @@ struct bmc150_accel_event {
 			u32 duration;
 			u32 threshold;
 		} slope;
+		u8 watermark;
 	};
 };
 
-#define BMC150_ACCEL_INTERRUPTS		2
+#define BMC150_ACCEL_INTERRUPTS		4
 #define BMC150_ACCEL_TRIGGERS		2
-#define BMC150_ACCEL_EVENTS		1
+#define BMC150_ACCEL_EVENTS		3
 
 struct bmc150_accel_data {
 	struct i2c_client *client;
@@ -191,6 +203,7 @@ struct bmc150_accel_data {
 	struct bmc150_accel_trigger triggers[BMC150_ACCEL_TRIGGERS];
 	struct bmc150_accel_event events[BMC150_ACCEL_EVENTS];
 	struct mutex mutex;
+	int fifo_mode;
 	s16 buffer[8];
 	u8 bw_bits;
 	u32 range;
@@ -483,6 +496,18 @@ bmc150_accel_interrupts[BMC150_ACCEL_INTERRUPTS] = {
 			BMC150_ACCEL_INT_EN_BIT_SLP_Y |
 			BMC150_ACCEL_INT_EN_BIT_SLP_Z
 	},
+	{ /* fifo full interrupt */
+		.map_reg = BMC150_ACCEL_REG_INT_MAP_1,
+		.map_bitmask = BMC150_ACCEL_INT_MAP_1_BIT_FFULL,
+		.en_reg = BMC150_ACCEL_REG_INT_EN_1,
+		.en_bitmask = BMC150_ACCEL_INT_EN_BIT_FFULL_EN,
+	},
+	{ /* fifo watermark interrupt */
+		.map_reg = BMC150_ACCEL_REG_INT_MAP_1,
+		.map_bitmask = BMC150_ACCEL_INT_MAP_1_BIT_FWM,
+		.en_reg = BMC150_ACCEL_REG_INT_EN_1,
+		.en_bitmask = BMC150_ACCEL_INT_EN_BIT_FWM_EN,
+	},
 };
 
 static void bmc150_accel_interrupts_setup(struct iio_dev *indio_dev,
@@ -699,11 +724,85 @@ static int bmc150_accel_read_raw(struct iio_dev *indio_dev,
 		ret = bmc150_accel_get_bw(data, val, val2);
 		mutex_unlock(&data->mutex);
 		return ret;
+	case IIO_CHAN_INFO_FIFO_LENGTH:
+		*val2 = 0;
+		*val = BMC150_ACCEL_FIFO_LENGTH;
+		return IIO_VAL_INT;
 	default:
 		return -EINVAL;
 	}
 }
 
+static int bmc150_accel_fifo_flush(struct iio_dev *indio_dev)
+{
+	struct bmc150_accel_data *data = iio_priv(indio_dev);
+	int ret, i;
+	u8 count;
+	u16 buffer[BMC150_ACCEL_FIFO_LENGTH * 3];
+	u8 reg_fifo_data = BMC150_ACCEL_REG_FIFO_DATA;
+	struct i2c_msg msg[2];
+	int64_t tstamp;
+	int sample_freq = 0, sec, ms;
+
+	ret = bmc150_accel_get_bw(data, &sec, &ms);
+	if (ret == 0)
+		sample_freq = sec * 1000000000 + ms * 1000;
+
+	ret = i2c_smbus_read_byte_data(data->client,
+				       BMC150_ACCEL_REG_FIFO_STATUS);
+	if (ret < 0) {
+		dev_err(&indio_dev->dev, "Error reading reg_fifo_status\n");
+		return ret;
+	}
+
+	count = ret & 0x7F;
+
+	i = i2c_smbus_read_byte_data(data->client,
+				     BMC150_ACCEL_REG_FIFO_CONFIG1);
+	if (!count)
+		return 0;
+
+	msg[0].addr = data->client->addr;
+	msg[0].flags = 0;
+	msg[0].buf = &reg_fifo_data;
+	msg[0].len = sizeof(reg_fifo_data);
+
+	msg[1].addr = data->client->addr;
+	msg[1].flags = I2C_M_RD;
+	msg[1].buf = (u8 *)buffer;
+	msg[1].len = count * 3 * 2;
+
+	ret = i2c_transfer(data->client->adapter, msg, 2);
+	if (ret != 2) {
+		dev_err(&indio_dev->dev, "Error reading reg_fifo_data\n");
+		return ret;
+	}
+
+	if (!data->timestamp)
+		data->timestamp = iio_get_time_ns();
+
+	tstamp = data->timestamp - count * sample_freq;
+
+	for (i = 0; i < count; i++) {
+		u16 sample[8];
+		int j, bit;
+
+		j = 0;
+		for_each_set_bit(bit, indio_dev->buffer->scan_mask,
+				 indio_dev->masklength) {
+			memcpy(&sample[j++], &buffer[i * 3 + bit], 2);
+		}
+
+		iio_push_to_buffers_with_timestamp(indio_dev, sample, tstamp);
+
+		tstamp += sample_freq;
+	}
+
+	data->timestamp = 0;
+
+	return 0;
+}
+
 static int bmc150_accel_write_raw(struct iio_dev *indio_dev,
 				  struct iio_chan_spec const *chan,
 				  int val, int val2, long mask)
@@ -725,6 +824,8 @@ static int bmc150_accel_write_raw(struct iio_dev *indio_dev,
 		ret = bmc150_accel_set_scale(data, val2);
 		mutex_unlock(&data->mutex);
 		return ret;
+	case IIO_CHAN_INFO_FIFO_FLUSH:
+		return bmc150_accel_fifo_flush(indio_dev);
 	default:
 		ret = -EINVAL;
 	}
@@ -764,6 +865,22 @@ static int bmc150_accel_event_roc_read(struct bmc150_accel_event *event,
 	return IIO_VAL_INT;
 }
 
+static int bmc150_accel_event_watermark_read(struct bmc150_accel_event *event,
+					     enum iio_event_info info,
+					     int *val, int *val2)
+{
+	*val2 = 0;
+	switch (info) {
+	case IIO_EV_INFO_VALUE:
+		*val = event->watermark;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return IIO_VAL_INT;
+}
+
 static int bmc150_accel_read_event(struct iio_dev *indio_dev,
 				   const struct iio_chan_spec *chan,
 				   enum iio_event_type type,
@@ -807,6 +924,38 @@ static int bmc150_accel_event_roc_write(struct bmc150_accel_event *event,
 	return ret;
 }
 
+static int bmc150_accel_event_watermark_write(struct bmc150_accel_event *event,
+					      enum iio_event_info info,
+					      int val, int val2)
+{
+	struct bmc150_accel_data *data = event->data;
+	int ret;
+
+
+	switch (info) {
+	case IIO_EV_INFO_VALUE:
+		if (val > BMC150_ACCEL_FIFO_LENGTH) {
+			ret = -EINVAL;
+			break;
+		}
+		mutex_lock(&data->mutex);
+		ret = i2c_smbus_write_byte_data(data->client,
+						BMC150_ACCEL_REG_FIFO_CONFIG0,
+						val);
+		if (ret < 0) {
+			dev_err(&data->client->dev, "Error writing reg_fifo_config0\n");
+			break;
+		}
+		event->watermark = val;
+		mutex_unlock(&data->mutex);
+		break;
+	default:
+		ret = -EINVAL;
+	}
+
+	return ret;
+}
+
 static int bmc150_accel_write_event(struct iio_dev *indio_dev,
 				    const struct iio_chan_spec *chan,
 				    enum iio_event_type type,
@@ -900,6 +1049,76 @@ static const struct iio_event_spec bmc150_accel_events[BMC150_ACCEL_EVENTS] = {
 				 BIT(IIO_EV_INFO_ENABLE) |
 				 BIT(IIO_EV_INFO_PERIOD)
 	},
+	{
+		.type = IIO_EV_TYPE_FIFO_FULL,
+		.dir = IIO_EV_DIR_RISING,
+		.mask_separate = BIT(IIO_EV_INFO_ENABLE)
+	},
+	{
+		.type = IIO_EV_TYPE_FIFO_WATERMARK,
+		.dir = IIO_EV_DIR_RISING,
+		.mask_separate = BIT(IIO_EV_INFO_ENABLE) |
+				 BIT(IIO_EV_INFO_VALUE)
+	},
+};
+
+
+static const char * const bmc150_accel_fifo_modes[] = {
+	"disabled",
+	"fifo",
+	"stream"
+};
+
+static int bmc150_accel_fifo_mode_set(struct iio_dev *indio_dev,
+				      const struct iio_chan_spec *chan,
+				      unsigned int mode)
+{
+	struct bmc150_accel_data *data = iio_priv(indio_dev);
+	int ret;
+	u8 val;
+
+	switch (mode) {
+	case 0:
+		val = 0;
+		break;
+	case 1:
+		val = 0x40;
+		break;
+	case 2:
+		val = 0x80;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	ret = i2c_smbus_write_byte_data(data->client,
+					BMC150_ACCEL_REG_FIFO_CONFIG1, val);
+	if (ret < 0)
+		dev_err(&indio_dev->dev, "Error writing reg_fifo_config1\n");
+
+	data->fifo_mode  = mode;
+
+	return ret;
+}
+
+static int bmc150_accel_fifo_mode_get(struct iio_dev *indio_dev,
+				      const struct iio_chan_spec *chan)
+{
+	struct bmc150_accel_data *data = iio_priv(indio_dev);
+
+	return data->fifo_mode;
+}
+
+static const struct iio_enum bmc150_accel_fifo_mode_enum = {
+	.items = bmc150_accel_fifo_modes,
+	.num_items = ARRAY_SIZE(bmc150_accel_fifo_modes),
+	.set = bmc150_accel_fifo_mode_set,
+	.get = bmc150_accel_fifo_mode_get,
+};
+
+static const struct iio_chan_spec_ext_info bmc150_accel_ext_info[] = {
+	IIO_FIFO_EXT_INFO(&bmc150_accel_fifo_mode_enum, IIO_SHARED_BY_TYPE),
+	{},
 };
 
 #define BMC150_ACCEL_CHANNEL(_axis, bits) {				\
@@ -908,7 +1127,9 @@ static const struct iio_event_spec bmc150_accel_events[BMC150_ACCEL_EVENTS] = {
 	.channel2 = IIO_MOD_##_axis,					\
 	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\
 	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |		\
-				BIT(IIO_CHAN_INFO_SAMP_FREQ),		\
+				BIT(IIO_CHAN_INFO_SAMP_FREQ) |		\
+				BIT(IIO_CHAN_INFO_FIFO_LENGTH) |	\
+				BIT(IIO_CHAN_INFO_FIFO_FLUSH),		\
 	.scan_index = AXIS_##_axis,					\
 	.scan_type = {							\
 		.sign = 's',						\
@@ -917,7 +1138,8 @@ static const struct iio_event_spec bmc150_accel_events[BMC150_ACCEL_EVENTS] = {
 		.shift = 16 - (bits),					\
 	},								\
 	.event_spec = bmc150_accel_events,				\
-	.num_event_specs = ARRAY_SIZE(bmc150_accel_events)		\
+	.num_event_specs = ARRAY_SIZE(bmc150_accel_events),		\
+	.ext_info = bmc150_accel_ext_info,				\
 }
 
 #define BMC150_ACCEL_CHANNELS(bits) {					\
@@ -1286,6 +1508,17 @@ static struct {
 		.read = bmc150_accel_event_roc_read,
 		.write = bmc150_accel_event_roc_write,
 	},
+	{
+		.intr = 2,
+		.type = IIO_EV_TYPE_FIFO_FULL,
+	},
+	{
+		.intr = 3,
+		.type = IIO_EV_TYPE_FIFO_WATERMARK,
+		.read = bmc150_accel_event_watermark_read,
+		.write = bmc150_accel_event_watermark_write,
+	},
+
 };
 
 static void bmc150_accel_events_setup(struct iio_dev *indio_dev,
@@ -1460,6 +1693,7 @@ static int bmc150_accel_resume(struct device *dev)
 	mutex_lock(&data->mutex);
 	if (atomic_read(&data->active_intr))
 		bmc150_accel_set_mode(data, BMC150_ACCEL_SLEEP_MODE_NORMAL, 0);
+	bmc150_accel_fifo_mode_set(indio_dev, NULL, data->fifo_mode);
 	mutex_unlock(&data->mutex);
 
 	return 0;
@@ -1489,6 +1723,9 @@ static int bmc150_accel_runtime_resume(struct device *dev)
 	ret = bmc150_accel_set_mode(data, BMC150_ACCEL_SLEEP_MODE_NORMAL, 0);
 	if (ret < 0)
 		return ret;
+	ret = bmc150_accel_fifo_mode_set(indio_dev, NULL, data->fifo_mode);
+	if (ret < 0)
+		return ret;
 
 	sleep_val = bmc150_accel_get_startup_times(data);
 	if (sleep_val < 20)
-- 
1.9.1

Re: [RFC 8/8] iio: bmc150: add support for hardware fifo

[jic23]

Octavian Purdila writes: 

> This patch adds support for hardware fifo. Fifo and stream mode are
> supported as well as fifo-full and fifo-watermark events. 
> 
> Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
A few minor bits inline.  The implementation seems fine, I am just
unconvinced that the user interface is the best way to go.
> ---
>  drivers/iio/accel/bmc150-accel.c | 249 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 243 insertions(+), 6 deletions(-) 
> 
> diff --git a/drivers/iio/accel/bmc150-accel.c b/drivers/iio/accel/bmc150-accel.c
> index 72d2dca..404cfec 100644
> --- a/drivers/iio/accel/bmc150-accel.c
> +++ b/drivers/iio/accel/bmc150-accel.c
> @@ -67,7 +67,9 @@
>  #define BMC150_ACCEL_INT_MAP_0_BIT_SLOPE	BIT(2)
>  
>  #define BMC150_ACCEL_REG_INT_MAP_1		0x1A
> -#define BMC150_ACCEL_INT_MAP_1_BIT_DATA	BIT(0)
> +#define BMC150_ACCEL_INT_MAP_1_BIT_DATA		BIT(0)
> +#define BMC150_ACCEL_INT_MAP_1_BIT_FWM		BIT(1)
> +#define BMC150_ACCEL_INT_MAP_1_BIT_FFULL	BIT(2)
>  
>  #define BMC150_ACCEL_REG_INT_RST_LATCH		0x21
>  #define BMC150_ACCEL_INT_MODE_LATCH_RESET	0x80
> @@ -80,7 +82,9 @@
>  #define BMC150_ACCEL_INT_EN_BIT_SLP_Z		BIT(2)
>  
>  #define BMC150_ACCEL_REG_INT_EN_1		0x17
> -#define BMC150_ACCEL_INT_EN_BIT_DATA_EN	BIT(4)
> +#define BMC150_ACCEL_INT_EN_BIT_DATA_EN		BIT(4)
> +#define BMC150_ACCEL_INT_EN_BIT_FFULL_EN	BIT(5)
> +#define BMC150_ACCEL_INT_EN_BIT_FWM_EN		BIT(6)
>  
>  #define BMC150_ACCEL_REG_INT_OUT_CTRL		0x20
>  #define BMC150_ACCEL_INT_OUT_CTRL_INT1_LVL	BIT(0)
> @@ -119,6 +123,13 @@
>  #define BMC150_ACCEL_AXIS_TO_REG(axis)	(BMC150_ACCEL_REG_XOUT_L + (axis * 2))
>  #define BMC150_AUTO_SUSPEND_DELAY_MS		2000
>  
> +#define BMC150_ACCEL_REG_FIFO_STATUS		0x0E
> +#define BMC150_ACCEL_REG_FIFO_CONFIG0		0x30
> +#define BMC150_ACCEL_REG_FIFO_CONFIG1		0x3E
> +#define BMC150_ACCEL_REG_FIFO_DATA		0x3F
> +#define BMC150_ACCEL_REG_FIFO_
> +#define BMC150_ACCEL_FIFO_LENGTH		32
> +
>  enum bmc150_accel_axis {
>  	AXIS_X,
>  	AXIS_Y,
> @@ -177,12 +188,13 @@ struct bmc150_accel_event {
>  			u32 duration;
>  			u32 threshold;
>  		} slope;
> +		u8 watermark;
>  	};
>  };
>  
> -#define BMC150_ACCEL_INTERRUPTS		2
> +#define BMC150_ACCEL_INTERRUPTS		4
>  #define BMC150_ACCEL_TRIGGERS		2
> -#define BMC150_ACCEL_EVENTS		1
> +#define BMC150_ACCEL_EVENTS		3
>  
>  struct bmc150_accel_data {
>  	struct i2c_client *client;
> @@ -191,6 +203,7 @@ struct bmc150_accel_data {
>  	struct bmc150_accel_trigger triggers[BMC150_ACCEL_TRIGGERS];
>  	struct bmc150_accel_event events[BMC150_ACCEL_EVENTS];
>  	struct mutex mutex;
> +	int fifo_mode;
>  	s16 buffer[8];
>  	u8 bw_bits;
>  	u32 range;
> @@ -483,6 +496,18 @@ bmc150_accel_interrupts[BMC150_ACCEL_INTERRUPTS] = {
>  			BMC150_ACCEL_INT_EN_BIT_SLP_Y |
>  			BMC150_ACCEL_INT_EN_BIT_SLP_Z
>  	},
> +	{ /* fifo full interrupt */
> +		.map_reg = BMC150_ACCEL_REG_INT_MAP_1,
> +		.map_bitmask = BMC150_ACCEL_INT_MAP_1_BIT_FFULL,
> +		.en_reg = BMC150_ACCEL_REG_INT_EN_1,
> +		.en_bitmask = BMC150_ACCEL_INT_EN_BIT_FFULL_EN,
> +	},
> +	{ /* fifo watermark interrupt */
> +		.map_reg = BMC150_ACCEL_REG_INT_MAP_1,
> +		.map_bitmask = BMC150_ACCEL_INT_MAP_1_BIT_FWM,
> +		.en_reg = BMC150_ACCEL_REG_INT_EN_1,
> +		.en_bitmask = BMC150_ACCEL_INT_EN_BIT_FWM_EN,
> +	},
>  };
>  
>  static void bmc150_accel_interrupts_setup(struct iio_dev *indio_dev,
> @@ -699,11 +724,85 @@ static int bmc150_accel_read_raw(struct iio_dev *indio_dev,
>  		ret = bmc150_accel_get_bw(data, val, val2);
>  		mutex_unlock(&data->mutex);
>  		return ret;
> +	case IIO_CHAN_INFO_FIFO_LENGTH:
> +		*val2 = 0;
> +		*val = BMC150_ACCEL_FIFO_LENGTH;
> +		return IIO_VAL_INT;
>  	default:
>  		return -EINVAL;
>  	}
>  }
>  
> +static int bmc150_accel_fifo_flush(struct iio_dev *indio_dev)
> +{
> +	struct bmc150_accel_data *data = iio_priv(indio_dev);
> +	int ret, i;
> +	u8 count;
> +	u16 buffer[BMC150_ACCEL_FIFO_LENGTH * 3];
> +	u8 reg_fifo_data = BMC150_ACCEL_REG_FIFO_DATA;
> +	struct i2c_msg msg[2];
> +	int64_t tstamp;
> +	int sample_freq = 0, sec, ms;
> +
> +	ret = bmc150_accel_get_bw(data, &sec, &ms);
> +	if (ret == 0)
> +		sample_freq = sec * 1000000000 + ms * 1000;
> +
> +	ret = i2c_smbus_read_byte_data(data->client,
> +				       BMC150_ACCEL_REG_FIFO_STATUS);
> +	if (ret < 0) {
> +		dev_err(&indio_dev->dev, "Error reading reg_fifo_status\n");
> +		return ret;
> +	}
> +
> +	count = ret & 0x7F;
> +
> +	i = i2c_smbus_read_byte_data(data->client,
> +				     BMC150_ACCEL_REG_FIFO_CONFIG1);
> +	if (!count)
> +		return 0;
> +
> +	msg[0].addr = data->client->addr;
> +	msg[0].flags = 0;
> +	msg[0].buf = &reg_fifo_data;
> +	msg[0].len = sizeof(reg_fifo_data);
> +
> +	msg[1].addr = data->client->addr;
> +	msg[1].flags = I2C_M_RD;
> +	msg[1].buf = (u8 *)buffer;
> +	msg[1].len = count * 3 * 2;
> +
> +	ret = i2c_transfer(data->client->adapter, msg, 2);
> +	if (ret != 2) {
> +		dev_err(&indio_dev->dev, "Error reading reg_fifo_data\n");
> +		return ret;
> +	}
> +
> +	if (!data->timestamp)
> +		data->timestamp = iio_get_time_ns();
> +
> +	tstamp = data->timestamp - count * sample_freq;

Hmm... These are likely to be less than accurate...
> +
> +	for (i = 0; i < count; i++) {
> +		u16 sample[8];
> +		int j, bit;
> +
> +		j = 0;
> +		for_each_set_bit(bit, indio_dev->buffer->scan_mask,
> +				 indio_dev->masklength) {
> +			memcpy(&sample[j++], &buffer[i * 3 + bit], 2);
> +		}
> +
> +		iio_push_to_buffers_with_timestamp(indio_dev, sample, tstamp);
> +
> +		tstamp += sample_freq;
> +	}
> +
> +	data->timestamp = 0;
> +
> +	return 0;
> +}
> +
>  static int bmc150_accel_write_raw(struct iio_dev *indio_dev,
>  				  struct iio_chan_spec const *chan,
>  				  int val, int val2, long mask)
> @@ -725,6 +824,8 @@ static int bmc150_accel_write_raw(struct iio_dev *indio_dev,
>  		ret = bmc150_accel_set_scale(data, val2);
>  		mutex_unlock(&data->mutex);
>  		return ret;
> +	case IIO_CHAN_INFO_FIFO_FLUSH:
> +		return bmc150_accel_fifo_flush(indio_dev);
>  	default:
>  		ret = -EINVAL;
>  	}
> @@ -764,6 +865,22 @@ static int bmc150_accel_event_roc_read(struct bmc150_accel_event *event,
>  	return IIO_VAL_INT;
>  }
>  
> +static int bmc150_accel_event_watermark_read(struct bmc150_accel_event *event,
> +					     enum iio_event_info info,
> +					     int *val, int *val2)
> +{
> +	*val2 = 0;
> +	switch (info) {
> +	case IIO_EV_INFO_VALUE:
> +		*val = event->watermark;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return IIO_VAL_INT;
> +}
> +
>  static int bmc150_accel_read_event(struct iio_dev *indio_dev,
>  				   const struct iio_chan_spec *chan,
>  				   enum iio_event_type type,
> @@ -807,6 +924,38 @@ static int bmc150_accel_event_roc_write(struct bmc150_accel_event *event,
>  	return ret;
>  }
>  
> +static int bmc150_accel_event_watermark_write(struct bmc150_accel_event *event,
> +					      enum iio_event_info info,
> +					      int val, int val2)
> +{
> +	struct bmc150_accel_data *data = event->data;
> +	int ret;
> +
> +
> +	switch (info) {
> +	case IIO_EV_INFO_VALUE:
> +		if (val > BMC150_ACCEL_FIFO_LENGTH) {
> +			ret = -EINVAL;
> +			break;
> +		}
> +		mutex_lock(&data->mutex);
> +		ret = i2c_smbus_write_byte_data(data->client,
> +						BMC150_ACCEL_REG_FIFO_CONFIG0,
> +						val);
> +		if (ret < 0) {
> +			dev_err(&data->client->dev, "Error writing reg_fifo_config0\n");
> +			break;
> +		}
> +		event->watermark = val;
> +		mutex_unlock(&data->mutex);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +	}
> +
> +	return ret;
> +}
> +
>  static int bmc150_accel_write_event(struct iio_dev *indio_dev,
>  				    const struct iio_chan_spec *chan,
>  				    enum iio_event_type type,
> @@ -900,6 +1049,76 @@ static const struct iio_event_spec bmc150_accel_events[BMC150_ACCEL_EVENTS] = {
>  				 BIT(IIO_EV_INFO_ENABLE) |
>  				 BIT(IIO_EV_INFO_PERIOD)
>  	},
> +	{
> +		.type = IIO_EV_TYPE_FIFO_FULL,
> +		.dir = IIO_EV_DIR_RISING,
> +		.mask_separate = BIT(IIO_EV_INFO_ENABLE)
> +	},
> +	{
> +		.type = IIO_EV_TYPE_FIFO_WATERMARK,
> +		.dir = IIO_EV_DIR_RISING,
> +		.mask_separate = BIT(IIO_EV_INFO_ENABLE) |
> +				 BIT(IIO_EV_INFO_VALUE)
> +	},
> +};
> +
> +
> +static const char * const bmc150_accel_fifo_modes[] = {
> +	"disabled",
> +	"fifo",
> +	"stream"
> +};
> +
> +static int bmc150_accel_fifo_mode_set(struct iio_dev *indio_dev,
> +				      const struct iio_chan_spec *chan,
> +				      unsigned int mode)
> +{
> +	struct bmc150_accel_data *data = iio_priv(indio_dev);
> +	int ret;
> +	u8 val;
> +
> +	switch (mode) {
> +	case 0:
Some defines and a local enum should get rid of the magic numbers here.
> +		val = 0;
> +		break;
> +	case 1:
> +		val = 0x40;
> +		break;
> +	case 2:
> +		val = 0x80;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	ret = i2c_smbus_write_byte_data(data->client,
> +					BMC150_ACCEL_REG_FIFO_CONFIG1, val);
> +	if (ret < 0)
> +		dev_err(&indio_dev->dev, "Error writing reg_fifo_config1\n");
> +
> +	data->fifo_mode  = mode;
> +
> +	return ret;
> +}
> +
> +static int bmc150_accel_fifo_mode_get(struct iio_dev *indio_dev,
> +				      const struct iio_chan_spec *chan)
> +{
> +	struct bmc150_accel_data *data = iio_priv(indio_dev);
> +
> +	return data->fifo_mode;
> +}
> +
> +static const struct iio_enum bmc150_accel_fifo_mode_enum = {
> +	.items = bmc150_accel_fifo_modes,
> +	.num_items = ARRAY_SIZE(bmc150_accel_fifo_modes),
> +	.set = bmc150_accel_fifo_mode_set,
> +	.get = bmc150_accel_fifo_mode_get,
> +};
> +
> +static const struct iio_chan_spec_ext_info bmc150_accel_ext_info[] = {
> +	IIO_FIFO_EXT_INFO(&bmc150_accel_fifo_mode_enum, IIO_SHARED_BY_TYPE),
> +	{},
>  };
>  
>  #define BMC150_ACCEL_CHANNEL(_axis, bits) {				\
> @@ -908,7 +1127,9 @@ static const struct iio_event_spec bmc150_accel_events[BMC150_ACCEL_EVENTS] = {
>  	.channel2 = IIO_MOD_##_axis,					\
>  	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\
>  	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |		\
> -				BIT(IIO_CHAN_INFO_SAMP_FREQ),		\
> +				BIT(IIO_CHAN_INFO_SAMP_FREQ) |		\
> +				BIT(IIO_CHAN_INFO_FIFO_LENGTH) |	\
> +				BIT(IIO_CHAN_INFO_FIFO_FLUSH),		\
>  	.scan_index = AXIS_##_axis,					\
>  	.scan_type = {							\
>  		.sign = 's',						\
> @@ -917,7 +1138,8 @@ static const struct iio_event_spec bmc150_accel_events[BMC150_ACCEL_EVENTS] = {
>  		.shift = 16 - (bits),					\
>  	},								\
>  	.event_spec = bmc150_accel_events,				\
> -	.num_event_specs = ARRAY_SIZE(bmc150_accel_events)		\
> +	.num_event_specs = ARRAY_SIZE(bmc150_accel_events),		\
> +	.ext_info = bmc150_accel_ext_info,				\
>  }
>  
>  #define BMC150_ACCEL_CHANNELS(bits) {					\
> @@ -1286,6 +1508,17 @@ static struct {
>  		.read = bmc150_accel_event_roc_read,
>  		.write = bmc150_accel_event_roc_write,
>  	},
> +	{
> +		.intr = 2,
> +		.type = IIO_EV_TYPE_FIFO_FULL,
> +	},
> +	{
> +		.intr = 3,
> +		.type = IIO_EV_TYPE_FIFO_WATERMARK,
> +		.read = bmc150_accel_event_watermark_read,
> +		.write = bmc150_accel_event_watermark_write,
> +	},
> +
>  };
>  
>  static void bmc150_accel_events_setup(struct iio_dev *indio_dev,
> @@ -1460,6 +1693,7 @@ static int bmc150_accel_resume(struct device *dev)
>  	mutex_lock(&data->mutex);
>  	if (atomic_read(&data->active_intr))
>  		bmc150_accel_set_mode(data, BMC150_ACCEL_SLEEP_MODE_NORMAL, 0);
> +	bmc150_accel_fifo_mode_set(indio_dev, NULL, data->fifo_mode);
>  	mutex_unlock(&data->mutex);
>  
>  	return 0;
> @@ -1489,6 +1723,9 @@ static int bmc150_accel_runtime_resume(struct device *dev)
>  	ret = bmc150_accel_set_mode(data, BMC150_ACCEL_SLEEP_MODE_NORMAL, 0);
>  	if (ret < 0)
>  		return ret;
> +	ret = bmc150_accel_fifo_mode_set(indio_dev, NULL, data->fifo_mode);
> +	if (ret < 0)
> +		return ret;
>  
>  	sleep_val = bmc150_accel_get_startup_times(data);
>  	if (sleep_val < 20)
> -- 
> 1.9.1 
> 
> --
> 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: [RFC 8/8] iio: bmc150: add support for hardware fifo

[Octavian Purdila]

On Tue, Nov 18, 2014 at 3:49 PM,  <jic23@kernel.org> wrote:
> Octavian Purdila writes:
>>
>> This patch adds support for hardware fifo. Fifo and stream mode are
>> supported as well as fifo-full and fifo-watermark events.
>> Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
>
> A few minor bits inline.  The implementation seems fine, I am just
> unconvinced that the user interface is the best way to go.
>

Of course, I didn't expect to get the right interface :) This was
suppose to be just code support for discussing it (the interface).

>> +
>> +       if (!data->timestamp)
>> +               data->timestamp = iio_get_time_ns();
>> +
>> +       tstamp = data->timestamp - count * sample_freq;
>
>
> Hmm... These are likely to be less than accurate...
>

Yes, especially for fifo mode (for stream should be close).
Unfortunately the hardware doesn't store any sort of timestamp in the
buffer, so any ideas of how can we improve this?

>> +static int bmc150_accel_fifo_mode_set(struct iio_dev *indio_dev,
>> +                                     const struct iio_chan_spec *chan,
>> +                                     unsigned int mode)
>> +{
>> +       struct bmc150_accel_data *data = iio_priv(indio_dev);
>> +       int ret;
>> +       u8 val;
>> +
>> +       switch (mode) {
>> +       case 0:
>
> Some defines and a local enum should get rid of the magic numbers here.

You're right, I'll update that.

Thanks for taking the time to look at these patches Jonathan, I know
that you are very busy.

Re: [RFC 8/8] iio: bmc150: add support for hardware fifo

[Hartmut Knaack]

Octavian Purdila schrieb am 17.11.2014 18:56:
> This patch adds support for hardware fifo. Fifo and stream mode are
> supported as well as fifo-full and fifo-watermark events.
> 
> Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
> ---
>  drivers/iio/accel/bmc150-accel.c | 249 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 243 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/accel/bmc150-accel.c b/drivers/iio/accel/bmc150-accel.c
> index 72d2dca..404cfec 100644
> --- a/drivers/iio/accel/bmc150-accel.c
> +++ b/drivers/iio/accel/bmc150-accel.c
> @@ -67,7 +67,9 @@
>  #define BMC150_ACCEL_INT_MAP_0_BIT_SLOPE	BIT(2)
>  
>  #define BMC150_ACCEL_REG_INT_MAP_1		0x1A
> -#define BMC150_ACCEL_INT_MAP_1_BIT_DATA	BIT(0)
> +#define BMC150_ACCEL_INT_MAP_1_BIT_DATA		BIT(0)
> +#define BMC150_ACCEL_INT_MAP_1_BIT_FWM		BIT(1)
> +#define BMC150_ACCEL_INT_MAP_1_BIT_FFULL	BIT(2)
>  
>  #define BMC150_ACCEL_REG_INT_RST_LATCH		0x21
>  #define BMC150_ACCEL_INT_MODE_LATCH_RESET	0x80
> @@ -80,7 +82,9 @@
>  #define BMC150_ACCEL_INT_EN_BIT_SLP_Z		BIT(2)
>  
>  #define BMC150_ACCEL_REG_INT_EN_1		0x17
> -#define BMC150_ACCEL_INT_EN_BIT_DATA_EN	BIT(4)
> +#define BMC150_ACCEL_INT_EN_BIT_DATA_EN		BIT(4)
> +#define BMC150_ACCEL_INT_EN_BIT_FFULL_EN	BIT(5)
> +#define BMC150_ACCEL_INT_EN_BIT_FWM_EN		BIT(6)
>  
>  #define BMC150_ACCEL_REG_INT_OUT_CTRL		0x20
>  #define BMC150_ACCEL_INT_OUT_CTRL_INT1_LVL	BIT(0)
> @@ -119,6 +123,13 @@
>  #define BMC150_ACCEL_AXIS_TO_REG(axis)	(BMC150_ACCEL_REG_XOUT_L + (axis * 2))
>  #define BMC150_AUTO_SUSPEND_DELAY_MS		2000
>  
> +#define BMC150_ACCEL_REG_FIFO_STATUS		0x0E
> +#define BMC150_ACCEL_REG_FIFO_CONFIG0		0x30
> +#define BMC150_ACCEL_REG_FIFO_CONFIG1		0x3E
> +#define BMC150_ACCEL_REG_FIFO_DATA		0x3F
> +#define BMC150_ACCEL_REG_FIFO_
Something got lost here?
> +#define BMC150_ACCEL_FIFO_LENGTH		32
> +
>  enum bmc150_accel_axis {
>  	AXIS_X,
>  	AXIS_Y,
> @@ -177,12 +188,13 @@ struct bmc150_accel_event {
>  			u32 duration;
>  			u32 threshold;
>  		} slope;
> +		u8 watermark;
>  	};
>  };
>  
> -#define BMC150_ACCEL_INTERRUPTS		2
> +#define BMC150_ACCEL_INTERRUPTS		4
>  #define BMC150_ACCEL_TRIGGERS		2
> -#define BMC150_ACCEL_EVENTS		1
> +#define BMC150_ACCEL_EVENTS		3
>  
>  struct bmc150_accel_data {
>  	struct i2c_client *client;
> @@ -191,6 +203,7 @@ struct bmc150_accel_data {
>  	struct bmc150_accel_trigger triggers[BMC150_ACCEL_TRIGGERS];
>  	struct bmc150_accel_event events[BMC150_ACCEL_EVENTS];
>  	struct mutex mutex;
> +	int fifo_mode;
>  	s16 buffer[8];
>  	u8 bw_bits;
>  	u32 range;
> @@ -483,6 +496,18 @@ bmc150_accel_interrupts[BMC150_ACCEL_INTERRUPTS] = {
>  			BMC150_ACCEL_INT_EN_BIT_SLP_Y |
>  			BMC150_ACCEL_INT_EN_BIT_SLP_Z
>  	},
> +	{ /* fifo full interrupt */
> +		.map_reg = BMC150_ACCEL_REG_INT_MAP_1,
> +		.map_bitmask = BMC150_ACCEL_INT_MAP_1_BIT_FFULL,
> +		.en_reg = BMC150_ACCEL_REG_INT_EN_1,
> +		.en_bitmask = BMC150_ACCEL_INT_EN_BIT_FFULL_EN,
> +	},
> +	{ /* fifo watermark interrupt */
> +		.map_reg = BMC150_ACCEL_REG_INT_MAP_1,
> +		.map_bitmask = BMC150_ACCEL_INT_MAP_1_BIT_FWM,
> +		.en_reg = BMC150_ACCEL_REG_INT_EN_1,
> +		.en_bitmask = BMC150_ACCEL_INT_EN_BIT_FWM_EN,
> +	},
>  };
>  
>  static void bmc150_accel_interrupts_setup(struct iio_dev *indio_dev,
> @@ -699,11 +724,85 @@ static int bmc150_accel_read_raw(struct iio_dev *indio_dev,
>  		ret = bmc150_accel_get_bw(data, val, val2);
>  		mutex_unlock(&data->mutex);
>  		return ret;
> +	case IIO_CHAN_INFO_FIFO_LENGTH:
> +		*val2 = 0;
> +		*val = BMC150_ACCEL_FIFO_LENGTH;
> +		return IIO_VAL_INT;
>  	default:
>  		return -EINVAL;
>  	}
>  }
>  
> +static int bmc150_accel_fifo_flush(struct iio_dev *indio_dev)
> +{
> +	struct bmc150_accel_data *data = iio_priv(indio_dev);
> +	int ret, i;
> +	u8 count;
> +	u16 buffer[BMC150_ACCEL_FIFO_LENGTH * 3];
> +	u8 reg_fifo_data = BMC150_ACCEL_REG_FIFO_DATA;
> +	struct i2c_msg msg[2];
> +	int64_t tstamp;
> +	int sample_freq = 0, sec, ms;
> +
> +	ret = bmc150_accel_get_bw(data, &sec, &ms);
> +	if (ret == 0)
ret can be IIO_VAL_INT_PLUS_MICRO or -EINVAL, but not 0.
> +		sample_freq = sec * 1000000000 + ms * 1000;
> +
> +	ret = i2c_smbus_read_byte_data(data->client,
> +				       BMC150_ACCEL_REG_FIFO_STATUS);
> +	if (ret < 0) {
> +		dev_err(&indio_dev->dev, "Error reading reg_fifo_status\n");
> +		return ret;
> +	}
> +
> +	count = ret & 0x7F;
> +
> +	i = i2c_smbus_read_byte_data(data->client,
> +				     BMC150_ACCEL_REG_FIFO_CONFIG1);
This read doesn't seem to serve a purpose
> +	if (!count)
> +		return 0;
> +
> +	msg[0].addr = data->client->addr;
> +	msg[0].flags = 0;
> +	msg[0].buf = &reg_fifo_data;
> +	msg[0].len = sizeof(reg_fifo_data);
> +
> +	msg[1].addr = data->client->addr;
> +	msg[1].flags = I2C_M_RD;
> +	msg[1].buf = (u8 *)buffer;
> +	msg[1].len = count * 3 * 2;
> +
> +	ret = i2c_transfer(data->client->adapter, msg, 2);
> +	if (ret != 2) {
> +		dev_err(&indio_dev->dev, "Error reading reg_fifo_data\n");
> +		return ret;
> +	}
> +
> +	if (!data->timestamp)
> +		data->timestamp = iio_get_time_ns();
> +
> +	tstamp = data->timestamp - count * sample_freq;
> +
> +	for (i = 0; i < count; i++) {
> +		u16 sample[8];
> +		int j, bit;
> +
> +		j = 0;
> +		for_each_set_bit(bit, indio_dev->buffer->scan_mask,
> +				 indio_dev->masklength) {
> +			memcpy(&sample[j++], &buffer[i * 3 + bit], 2);
> +		}
> +
> +		iio_push_to_buffers_with_timestamp(indio_dev, sample, tstamp);
> +
> +		tstamp += sample_freq;
> +	}
> +
> +	data->timestamp = 0;
> +
> +	return 0;
> +}
> +
>  static int bmc150_accel_write_raw(struct iio_dev *indio_dev,
>  				  struct iio_chan_spec const *chan,
>  				  int val, int val2, long mask)
> @@ -725,6 +824,8 @@ static int bmc150_accel_write_raw(struct iio_dev *indio_dev,
>  		ret = bmc150_accel_set_scale(data, val2);
>  		mutex_unlock(&data->mutex);
>  		return ret;
> +	case IIO_CHAN_INFO_FIFO_FLUSH:
> +		return bmc150_accel_fifo_flush(indio_dev);
You may want to trigger this for specific values written to that file?
>  	default:
>  		ret = -EINVAL;
>  	}
> @@ -764,6 +865,22 @@ static int bmc150_accel_event_roc_read(struct bmc150_accel_event *event,
>  	return IIO_VAL_INT;
>  }
>  
> +static int bmc150_accel_event_watermark_read(struct bmc150_accel_event *event,
> +					     enum iio_event_info info,
> +					     int *val, int *val2)
> +{
> +	*val2 = 0;
> +	switch (info) {
> +	case IIO_EV_INFO_VALUE:
> +		*val = event->watermark;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return IIO_VAL_INT;
> +}
> +
>  static int bmc150_accel_read_event(struct iio_dev *indio_dev,
>  				   const struct iio_chan_spec *chan,
>  				   enum iio_event_type type,
> @@ -807,6 +924,38 @@ static int bmc150_accel_event_roc_write(struct bmc150_accel_event *event,
>  	return ret;
>  }
>  
> +static int bmc150_accel_event_watermark_write(struct bmc150_accel_event *event,
> +					      enum iio_event_info info,
> +					      int val, int val2)
> +{
> +	struct bmc150_accel_data *data = event->data;
> +	int ret;
> +
> +
> +	switch (info) {
> +	case IIO_EV_INFO_VALUE:
> +		if (val > BMC150_ACCEL_FIFO_LENGTH) {
> +			ret = -EINVAL;
> +			break;
> +		}
> +		mutex_lock(&data->mutex);
> +		ret = i2c_smbus_write_byte_data(data->client,
> +						BMC150_ACCEL_REG_FIFO_CONFIG0,
> +						val);
> +		if (ret < 0) {
> +			dev_err(&data->client->dev, "Error writing reg_fifo_config0\n");
> +			break;
> +		}
> +		event->watermark = val;
> +		mutex_unlock(&data->mutex);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +	}
> +
> +	return ret;
> +}
> +
>  static int bmc150_accel_write_event(struct iio_dev *indio_dev,
>  				    const struct iio_chan_spec *chan,
>  				    enum iio_event_type type,
> @@ -900,6 +1049,76 @@ static const struct iio_event_spec bmc150_accel_events[BMC150_ACCEL_EVENTS] = {
>  				 BIT(IIO_EV_INFO_ENABLE) |
>  				 BIT(IIO_EV_INFO_PERIOD)
>  	},
> +	{
> +		.type = IIO_EV_TYPE_FIFO_FULL,
> +		.dir = IIO_EV_DIR_RISING,
> +		.mask_separate = BIT(IIO_EV_INFO_ENABLE)
> +	},
> +	{
> +		.type = IIO_EV_TYPE_FIFO_WATERMARK,
> +		.dir = IIO_EV_DIR_RISING,
> +		.mask_separate = BIT(IIO_EV_INFO_ENABLE) |
> +				 BIT(IIO_EV_INFO_VALUE)
> +	},
> +};
> +
> +
> +static const char * const bmc150_accel_fifo_modes[] = {
> +	"disabled",
> +	"fifo",
> +	"stream"
> +};
> +
> +static int bmc150_accel_fifo_mode_set(struct iio_dev *indio_dev,
> +				      const struct iio_chan_spec *chan,
> +				      unsigned int mode)
> +{
> +	struct bmc150_accel_data *data = iio_priv(indio_dev);
> +	int ret;
> +	u8 val;
> +
> +	switch (mode) {
> +	case 0:
> +		val = 0;
> +		break;
> +	case 1:
> +		val = 0x40;
> +		break;
> +	case 2:
> +		val = 0x80;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	ret = i2c_smbus_write_byte_data(data->client,
> +					BMC150_ACCEL_REG_FIFO_CONFIG1, val);
> +	if (ret < 0)
> +		dev_err(&indio_dev->dev, "Error writing reg_fifo_config1\n");
> +
> +	data->fifo_mode  = mode;
> +
> +	return ret;
> +}
> +
> +static int bmc150_accel_fifo_mode_get(struct iio_dev *indio_dev,
> +				      const struct iio_chan_spec *chan)
> +{
> +	struct bmc150_accel_data *data = iio_priv(indio_dev);
> +
> +	return data->fifo_mode;
> +}
> +
> +static const struct iio_enum bmc150_accel_fifo_mode_enum = {
> +	.items = bmc150_accel_fifo_modes,
> +	.num_items = ARRAY_SIZE(bmc150_accel_fifo_modes),
> +	.set = bmc150_accel_fifo_mode_set,
> +	.get = bmc150_accel_fifo_mode_get,
> +};
> +
> +static const struct iio_chan_spec_ext_info bmc150_accel_ext_info[] = {
> +	IIO_FIFO_EXT_INFO(&bmc150_accel_fifo_mode_enum, IIO_SHARED_BY_TYPE),
> +	{},
>  };
>  
>  #define BMC150_ACCEL_CHANNEL(_axis, bits) {				\
> @@ -908,7 +1127,9 @@ static const struct iio_event_spec bmc150_accel_events[BMC150_ACCEL_EVENTS] = {
>  	.channel2 = IIO_MOD_##_axis,					\
>  	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\
>  	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |		\
> -				BIT(IIO_CHAN_INFO_SAMP_FREQ),		\
> +				BIT(IIO_CHAN_INFO_SAMP_FREQ) |		\
> +				BIT(IIO_CHAN_INFO_FIFO_LENGTH) |	\
> +				BIT(IIO_CHAN_INFO_FIFO_FLUSH),		\
>  	.scan_index = AXIS_##_axis,					\
>  	.scan_type = {							\
>  		.sign = 's',						\
> @@ -917,7 +1138,8 @@ static const struct iio_event_spec bmc150_accel_events[BMC150_ACCEL_EVENTS] = {
>  		.shift = 16 - (bits),					\
>  	},								\
>  	.event_spec = bmc150_accel_events,				\
> -	.num_event_specs = ARRAY_SIZE(bmc150_accel_events)		\
> +	.num_event_specs = ARRAY_SIZE(bmc150_accel_events),		\
> +	.ext_info = bmc150_accel_ext_info,				\
>  }
>  
>  #define BMC150_ACCEL_CHANNELS(bits) {					\
> @@ -1286,6 +1508,17 @@ static struct {
>  		.read = bmc150_accel_event_roc_read,
>  		.write = bmc150_accel_event_roc_write,
>  	},
> +	{
> +		.intr = 2,
> +		.type = IIO_EV_TYPE_FIFO_FULL,
> +	},
> +	{
> +		.intr = 3,
> +		.type = IIO_EV_TYPE_FIFO_WATERMARK,
> +		.read = bmc150_accel_event_watermark_read,
> +		.write = bmc150_accel_event_watermark_write,
> +	},
> +
>  };
>  
>  static void bmc150_accel_events_setup(struct iio_dev *indio_dev,
> @@ -1460,6 +1693,7 @@ static int bmc150_accel_resume(struct device *dev)
>  	mutex_lock(&data->mutex);
>  	if (atomic_read(&data->active_intr))
>  		bmc150_accel_set_mode(data, BMC150_ACCEL_SLEEP_MODE_NORMAL, 0);
> +	bmc150_accel_fifo_mode_set(indio_dev, NULL, data->fifo_mode);
>  	mutex_unlock(&data->mutex);
>  
>  	return 0;
> @@ -1489,6 +1723,9 @@ static int bmc150_accel_runtime_resume(struct device *dev)
>  	ret = bmc150_accel_set_mode(data, BMC150_ACCEL_SLEEP_MODE_NORMAL, 0);
>  	if (ret < 0)
>  		return ret;
> +	ret = bmc150_accel_fifo_mode_set(indio_dev, NULL, data->fifo_mode);
> +	if (ret < 0)
> +		return ret;
>  
>  	sleep_val = bmc150_accel_get_startup_times(data);
>  	if (sleep_val < 20)
> 

Re: [RFC 0/8] iio: add support for hardware fifo

[jic23]

Octavian Purdila writes: 

> Hi everybody,
Hi Octavian.
> 
> I hope this RFC is a good starting point to discuss support for
> hardware fifo in IIO. The main reason to support it is to reduce the
> power consumtion, by allowing the CPU to enter deep sleep states for
> longer periods of time.
A worthwhile aim indeed.
> 
> Don't get discourage by the large number of patches most of them are
> refactors in the bmc150 driver, to make it easier to add support for
> the hardware fifo (basically to make adding interrupts and
> events/triggers easier). 
> 
> For discussing the hardware fifo stuff, only the first and last
> patches are important: the first adds new IIO attributes so that we
> can expose the hardware fifo and the last implements hadware fifo for
> IIO (as an example of how would a device use the exposed attributes). 
> 
> Note that the attributes can be exposed on a per device or per channel
> basis, since it seems both types of hardware fifos exists: those that
> store all data in a single fifo (temperature, accelerometer,
> magnetometer, etc.) and those that have separate fifos for
> accelerometer, gyroscope, etc. Thankfully, at the driver level we just
> need to use the appropriate sharing level to support one mode or the
> other.
If this is the case, then the buffered outputs will have to be separate
so we know what is coming.  E.g. it will have to be effectively treated
as separate iio_buffers.  Now we have devices that already do (see some
of the more interesting SOC ADCs) 

Is this actually the case for the bmc150? If you could point at
devices where it is then it would be great (as mentioned above one
or two SOC ADCs have a bonkers level of flexibility).
My reading of the datasheet is that the data is interleaved in the
buffer for any channels enable. 

> 
> Also note that this patch is orthogonal to the software watermark /
> batching patch send on the list a while back.

I'm not so sure this is orthogonal at all.   That proposal was actually
about hardware support.  I pushed back that I wanted any new interface
to work whether or not there was hardware support.  In a sense that is
a more complex problem - hence the discussion became a little bit focused
on that case. 

The intent there was to hide the implementation details of exactly this
sort of hardware/software buffer interaction. 

Perhaps some history is in order. 

We had exactly what you propose here a long time back with the sca3000
accelerometer (which is why there is still core support for hardware
buffers).  The interface was precisely the watershed type you have
here. 

A review by Arnd Bergman pointed out that this was seriously clunky. 

It puts data related to the in-band data flow into our out-of-band channel.
His suggestion was to allow for watershed interrupts using one of the
more interesting POLL types leaving the main poll for the data flow.
Arnd also suggested the bits about using anonymous chardevs for the event
reporting etc.  The unusual form of poll bit never got implemented,
but in the interests of moving forward with the common case and because
they were on there way out anyway the watershed interrupts were dropped. 

The more recent discussion came to the conclusion that there was no need
to use the weird forms of poll.  We could simply have an attribute to
control when poll was fired.  The only bit that caused friction was whether
there should be a timeout or not. 

Now, when then ti_am35xx driver came along it became clear that it wasn't
actually terribly useful exposing the hardware buffers directly to
user space.  The buffers tend not to be terribly long (in your bma160) I
see they are only 32 elements.  As such the conclusion was that it makes
more sense to use the buffers as temporary storage to smooth out the
data flow.  Thus that driver fills a software buffer from a hardware
buffer.  This seems the method that is most likely to work long term.
I note this is effectively what you have here, though I'm not sure of
the advantage of the explicit software flush over just reading the data
whenever the interrupt fires. 

Hence we started with something that at least superficially (I haven't
had a chance to go through the implementation in detail yet)
looks similar to what you have but ended up changing the method of
signalling to and from userspace. 

Hardware buffer -> Software buffer -> user space. 

Userspace watershed control -> Software buffer watershed control
 -> Hardware buffer watershed control. 

If userspace sets the watershed to say 16 then, as well as setting
that level in the software buffer it should be passed on to the
device driver allowing the watershed there to be set appropriately.
Now things get interesting if userspace sets the watershed to a value
that makes no sense for the hardware (say 17 on a device that does
power of 2 values only) as then it will have to fall back to
grabbing every one (Watershed of 1).  Perhaps we can provide 'hints'
for this? 

Anyhow, having given this general comment, I'll give some feedback on
the implementation details. 

Jonathan 

> 
> Octavian Purdila (8):
>   iio: add support for hardware fifo
>   iio: bmc150: refactor slope duration and threshold update
>   iio: bmc150: refactor interrupt enabling
>   iio: bmc150: exit early if event / trigger state is not changed
>   iio: bmc150: introduce bmc150_accel_interrupt
>   iio: bmc150: introduce bmc150_accel_trigger
>   iio: bmc150: introduce bmc150_accel_event
>   iio: bmc150: add support for hardware fifo 
> 
>  Documentation/ABI/testing/sysfs-bus-iio |  51 ++
>  drivers/iio/accel/bmc150-accel.c        | 976 ++++++++++++++++++++++----------
>  drivers/iio/industrialio-core.c         |   2 +
>  drivers/iio/industrialio-event.c        |   2 +
>  include/linux/iio/iio.h                 |  17 +
>  include/linux/iio/types.h               |   2 +
>  6 files changed, 739 insertions(+), 311 deletions(-) 
> 
> -- 
> 1.9.1 
> 
> --
> 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: [RFC 0/8] iio: add support for hardware fifo

[Octavian Purdila]

On Tue, Nov 18, 2014 at 3:24 PM, <jic23@kernel.org> wrote:
>
> Octavian Purdila writes:
>>
>> Hi everybody,
>
> Hi Octavian.
>>

Hi Jonathan,

Thanks for taking the time to look at this and for the detailed
feedback. I am new to IIO, so I may not understand your comments
fully, hope you will have patience with me :)

>>
>> I hope this RFC is a good starting point to discuss support for
>> hardware fifo in IIO. The main reason to support it is to reduce the
>> power consumtion, by allowing the CPU to enter deep sleep states for
>> longer periods of time.
>
> A worthwhile aim indeed.

BTW, the larger context into which we are looking at this, is the new
batching Android API:

http://source.android.com/devices/sensors/batching.html

>>
>>
>> Don't get discourage by the large number of patches most of them are
>> refactors in the bmc150 driver, to make it easier to add support for
>> the hardware fifo (basically to make adding interrupts and
>> events/triggers easier).
>> For discussing the hardware fifo stuff, only the first and last
>> patches are important: the first adds new IIO attributes so that we
>> can expose the hardware fifo and the last implements hadware fifo for
>> IIO (as an example of how would a device use the exposed attributes).
>> Note that the attributes can be exposed on a per device or per channel
>> basis, since it seems both types of hardware fifos exists: those that
>> store all data in a single fifo (temperature, accelerometer,
>> magnetometer, etc.) and those that have separate fifos for
>> accelerometer, gyroscope, etc. Thankfully, at the driver level we just
>> need to use the appropriate sharing level to support one mode or the
>> other.
>
> If this is the case, then the buffered outputs will have to be separate
> so we know what is coming.  E.g. it will have to be effectively treated
> as separate iio_buffers.  Now we have devices that already do (see some
> of the more interesting SOC ADCs)
> Is this actually the case for the bmc150? If you could point at
> devices where it is then it would be great (as mentioned above one
> or two SOC ADCs have a bonkers level of flexibility).
> My reading of the datasheet is that the data is interleaved in the
> buffer for any channels enable.
>

While working on this I did though about exposing the hardware fifo by
implementing a custom iio bufer. However, the iio buffers are much
more flexible that most hadware fifos. In bmc150' case, the fifo case
store x,y,z data or x or y or z data. It can not store x,y or x,z,
etc. And if I understand correctly, the user can enable x,z collection
for the device buffer. Also I have noticed that there can be multiple
buffers attached to a device, for what it seems to be demuxing
buffers?

That is why I thought of keeping the hardware fifo decoupled from the
buffers and add a flush operation that will read the data from the
fifo and push it to the device buffer(s). This way we don't affect the
demuxing buffers logic.

>>
>> Also note that this patch is orthogonal to the software watermark /
>> batching patch send on the list a while back.
>
>
> I'm not so sure this is orthogonal at all.   That proposal was actually
> about hardware support.  I pushed back that I wanted any new interface
> to work whether or not there was hardware support.  In a sense that is
> a more complex problem - hence the discussion became a little bit focused
> on that case.
> The intent there was to hide the implementation details of exactly this
> sort of hardware/software buffer interaction.

The reason I say they are orthogonal (IMHO) is that the software
watermark patch will only help reducing the number of kernel/user
context switches by allowing more data to gather before waking up
userspace. However, we will still have interrupts waking up the
processor to store the samples in the kernel buffer. With a hardware
fifo we can reduce that number of interrupts.

> Perhaps some history is in order.
> We had exactly what you propose here a long time back with the sca3000
> accelerometer (which is why there is still core support for hardware
> buffers).  The interface was precisely the watershed type you have
> here.
> A review by Arnd Bergman pointed out that this was seriously clunky.
> It puts data related to the in-band data flow into our out-of-band channel.
> His suggestion was to allow for watershed interrupts using one of the
> more interesting POLL types leaving the main poll for the data flow.
> Arnd also suggested the bits about using anonymous chardevs for the event
> reporting etc.  The unusual form of poll bit never got implemented,
> but in the interests of moving forward with the common case and because
> they were on there way out anyway the watershed interrupts were dropped.

You lost me here :) but I will look for that discussion and try to
understand Arnd's review.

> The more recent discussion came to the conclusion that there was no need
> to use the weird forms of poll.  We could simply have an attribute to
> control when poll was fired.  The only bit that caused friction was whether
> there should be a timeout or not.
> Now, when then ti_am35xx driver came along it became clear that it wasn't
> actually terribly useful exposing the hardware buffers directly to
> user space.  The buffers tend not to be terribly long (in your bma160) I
> see they are only 32 elements.

As I mentioned before, the main gain of using hardware fifo is to
reduce the number of interrupts. Right now, if we have a rate of 25HZ
we will get 25 interrupts per second. With hardware fifos we can
reduce that significanlty, and even small buffers help.


>  As such the conclusion was that it makes
> more sense to use the buffers as temporary storage to smooth out the
> data flow.  Thus that driver fills a software buffer from a hardware
> buffer.  This seems the method that is most likely to work long term.
> I note this is effectively what you have here, though I'm not sure of
> the advantage of the explicit software flush over just reading the data
> whenever the interrupt fires.

The explicit software flush allows userspace to balance power and
latency. Userspace can use a timeout based poll to guarantee a maximum
latency.  If we have enough data then we get an interrupt, we flush
the fifo and read the data. If not, we get a timeout, we flush the
fifo and read the data. Perhaps the explicit flush is not optimal and
we can find out other ways, but in the end we should have a way to
allow userspace to balance power and latency.

> Hence we started with something that at least superficially (I haven't
> had a chance to go through the implementation in detail yet)
> looks similar to what you have but ended up changing the method of
> signalling to and from userspace.
> Hardware buffer -> Software buffer -> user space.
> Userspace watershed control -> Software buffer watershed control
> -> Hardware buffer watershed control.
> If userspace sets the watershed to say 16 then, as well as setting
> that level in the software buffer it should be passed on to the
> device driver allowing the watershed there to be set appropriately.
> Now things get interesting if userspace sets the watershed to a value
> that makes no sense for the hardware (say 17 on a device that does
> power of 2 values only) as then it will have to fall back to
> grabbing every one (Watershed of 1).  Perhaps we can provide 'hints'
> for this?

As far as I understood, the proposed watermark implementation only
affects the device buffer and as I mentioned above that will not help
with reducing the interrupt rate.

Re: [RFC 0/8] iio: add support for hardware fifo

[Lars-Peter Clausen]

On 11/18/2014 04:03 PM, Octavian Purdila wrote:
> On Tue, Nov 18, 2014 at 3:24 PM, <jic23@kernel.org> wrote:
[...]
>> Hence we started with something that at least superficially (I haven't
>> had a chance to go through the implementation in detail yet)
>> looks similar to what you have but ended up changing the method of
>> signalling to and from userspace.
>> Hardware buffer -> Software buffer -> user space.
>> Userspace watershed control -> Software buffer watershed control
>> -> Hardware buffer watershed control.
>> If userspace sets the watershed to say 16 then, as well as setting
>> that level in the software buffer it should be passed on to the
>> device driver allowing the watershed there to be set appropriately.
>> Now things get interesting if userspace sets the watershed to a value
>> that makes no sense for the hardware (say 17 on a device that does
>> power of 2 values only) as then it will have to fall back to
>> grabbing every one (Watershed of 1).  Perhaps we can provide 'hints'
>> for this?
>
> As far as I understood, the proposed watermark implementation only
> affects the device buffer and as I mentioned above that will not help
> with reducing the interrupt rate.

By setting the watershed level the userspace application tells you that it 
is OK with getting data with a higher latency than one sample. This allows 
the driver to configure the FIFO level and hence reduce the interrupt rate.

- Lars

Re: [RFC 0/8] iio: add support for hardware fifo

[Octavian Purdila]

On Tue, Nov 18, 2014 at 6:44 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> On 11/18/2014 04:03 PM, Octavian Purdila wrote:
>>
>> On Tue, Nov 18, 2014 at 3:24 PM, <jic23@kernel.org> wrote:
>
> [...]
>>>
>>> Hence we started with something that at least superficially (I haven't
>>> had a chance to go through the implementation in detail yet)
>>> looks similar to what you have but ended up changing the method of
>>> signalling to and from userspace.
>>> Hardware buffer -> Software buffer -> user space.
>>> Userspace watershed control -> Software buffer watershed control
>>> -> Hardware buffer watershed control.
>>> If userspace sets the watershed to say 16 then, as well as setting
>>> that level in the software buffer it should be passed on to the
>>> device driver allowing the watershed there to be set appropriately.
>>> Now things get interesting if userspace sets the watershed to a value
>>> that makes no sense for the hardware (say 17 on a device that does
>>> power of 2 values only) as then it will have to fall back to
>>> grabbing every one (Watershed of 1).  Perhaps we can provide 'hints'
>>> for this?
>>
>>
>> As far as I understood, the proposed watermark implementation only
>> affects the device buffer and as I mentioned above that will not help
>> with reducing the interrupt rate.
>
>
> By setting the watershed level the userspace application tells you that it
> is OK with getting data with a higher latency than one sample. This allows
> the driver to configure the FIFO level and hence reduce the interrupt rate.
>

Hi Lars,

The implementation (as proposed in the patch by Josselin and Yannick)
does not inform the driver of changes to watermark, that is only
visible to core iio / buffer logic.

Also, the watermark alone is not enough to properly support hardware
fifos: the fifo can operate in multiple modes, we need to read data
from the hardware fifo even when the watermark interrupt is not issued
(the flush operation in the current patch set).

Re: [RFC 0/8] iio: add support for hardware fifo

[Lars-Peter Clausen]

On 11/18/2014 06:04 PM, Octavian Purdila wrote:
> On Tue, Nov 18, 2014 at 6:44 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>> On 11/18/2014 04:03 PM, Octavian Purdila wrote:
>>>
>>> On Tue, Nov 18, 2014 at 3:24 PM, <jic23@kernel.org> wrote:
>>
>> [...]
>>>>
>>>> Hence we started with something that at least superficially (I haven't
>>>> had a chance to go through the implementation in detail yet)
>>>> looks similar to what you have but ended up changing the method of
>>>> signalling to and from userspace.
>>>> Hardware buffer -> Software buffer -> user space.
>>>> Userspace watershed control -> Software buffer watershed control
>>>> -> Hardware buffer watershed control.
>>>> If userspace sets the watershed to say 16 then, as well as setting
>>>> that level in the software buffer it should be passed on to the
>>>> device driver allowing the watershed there to be set appropriately.
>>>> Now things get interesting if userspace sets the watershed to a value
>>>> that makes no sense for the hardware (say 17 on a device that does
>>>> power of 2 values only) as then it will have to fall back to
>>>> grabbing every one (Watershed of 1).  Perhaps we can provide 'hints'
>>>> for this?
>>>
>>>
>>> As far as I understood, the proposed watermark implementation only
>>> affects the device buffer and as I mentioned above that will not help
>>> with reducing the interrupt rate.
>>
>>
>> By setting the watershed level the userspace application tells you that it
>> is OK with getting data with a higher latency than one sample. This allows
>> the driver to configure the FIFO level and hence reduce the interrupt rate.
>>
>
> Hi Lars,
>
> The implementation (as proposed in the patch by Josselin and Yannick)
> does not inform the driver of changes to watermark, that is only
> visible to core iio / buffer logic.

That should be trivial to add though.

>
> Also, the watermark alone is not enough to properly support hardware
> fifos: the fifo can operate in multiple modes, we need to read data
> from the hardware fifo even when the watermark interrupt is not issued
> (the flush operation in the current patch set).

What kind of modes are there?

- Lars

Re: [RFC 0/8] iio: add support for hardware fifo

[Octavian Purdila]

On Tue, Nov 18, 2014 at 7:23 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:

>>>> As far as I understood, the proposed watermark implementation only
>>>> affects the device buffer and as I mentioned above that will not help
>>>> with reducing the interrupt rate.
>>>
>>>
>>>
>>> By setting the watershed level the userspace application tells you that
>>> it
>>> is OK with getting data with a higher latency than one sample. This
>>> allows
>>> the driver to configure the FIFO level and hence reduce the interrupt
>>> rate.
>>>
>>
>> Hi Lars,
>>
>> The implementation (as proposed in the patch by Josselin and Yannick)
>> does not inform the driver of changes to watermark, that is only
>> visible to core iio / buffer logic.
>
>
> That should be trivial to add though.
>

True. I've actually started by implementing hardware fifo support as a
new type of iio buffer, but I got scared by the buffer demux stuff. I
can take another stab at it, if that sounds better?

>>
>> Also, the watermark alone is not enough to properly support hardware
>> fifos: the fifo can operate in multiple modes, we need to read data
>> from the hardware fifo even when the watermark interrupt is not issued
>> (the flush operation in the current patch set).
>
>
> What kind of modes are there?
>

Stream - new values overwrrite old values, Fifo - drop new values.

Re: [RFC 0/8] iio: add support for hardware fifo

[Lars-Peter Clausen]

On 11/18/2014 08:35 PM, Octavian Purdila wrote:
> On Tue, Nov 18, 2014 at 7:23 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>
>>>>> As far as I understood, the proposed watermark implementation only
>>>>> affects the device buffer and as I mentioned above that will not help
>>>>> with reducing the interrupt rate.
>>>>
>>>>
>>>>
>>>> By setting the watershed level the userspace application tells you that
>>>> it
>>>> is OK with getting data with a higher latency than one sample. This
>>>> allows
>>>> the driver to configure the FIFO level and hence reduce the interrupt
>>>> rate.
>>>>
>>>
>>> Hi Lars,
>>>
>>> The implementation (as proposed in the patch by Josselin and Yannick)
>>> does not inform the driver of changes to watermark, that is only
>>> visible to core iio / buffer logic.
>>
>>
>> That should be trivial to add though.
>>
>
> True. I've actually started by implementing hardware fifo support as a
> new type of iio buffer, but I got scared by the buffer demux stuff. I
> can take another stab at it, if that sounds better?
>
>>>
>>> Also, the watermark alone is not enough to properly support hardware
>>> fifos: the fifo can operate in multiple modes, we need to read data
>>> from the hardware fifo even when the watermark interrupt is not issued
>>> (the flush operation in the current patch set).
>>
>>
>> What kind of modes are there?
>>
>
> Stream - new values overwrrite old values, Fifo - drop new values.

So basically ring buffer) style (old samples are dropped and FIFO style (new 
samples are dropped). This mode should probably be aligned with the mode 
that the sw buffer is working. E.g. you wouldn't want the software buffer to 
work in FIFO mode and the hardware buffer in ring buffer mode. That would 
lead to weird effects.

We talked about adding support for ring buffer mode to the software buffer a 
while ago, but so far nobody really needed it so it hasn't been implemented yet.

- Lars

Re: [RFC 0/8] iio: add support for hardware fifo

[Octavian Purdila]

On Wed, Nov 19, 2014 at 1:48 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> On 11/18/2014 08:35 PM, Octavian Purdila wrote:
>>
>> On Tue, Nov 18, 2014 at 7:23 PM, Lars-Peter Clausen <lars@metafoo.de>
>> wrote:
>>
>>>>>> As far as I understood, the proposed watermark implementation only
>>>>>> affects the device buffer and as I mentioned above that will not help
>>>>>> with reducing the interrupt rate.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> By setting the watershed level the userspace application tells you that
>>>>> it
>>>>> is OK with getting data with a higher latency than one sample. This
>>>>> allows
>>>>> the driver to configure the FIFO level and hence reduce the interrupt
>>>>> rate.
>>>>>
>>>>
>>>> Hi Lars,
>>>>
>>>> The implementation (as proposed in the patch by Josselin and Yannick)
>>>> does not inform the driver of changes to watermark, that is only
>>>> visible to core iio / buffer logic.
>>>
>>>
>>>
>>> That should be trivial to add though.
>>>
>>
>> True. I've actually started by implementing hardware fifo support as a
>> new type of iio buffer, but I got scared by the buffer demux stuff. I
>> can take another stab at it, if that sounds better?
>>
>>>>
>>>> Also, the watermark alone is not enough to properly support hardware
>>>> fifos: the fifo can operate in multiple modes, we need to read data
>>>> from the hardware fifo even when the watermark interrupt is not issued
>>>> (the flush operation in the current patch set).
>>>
>>>
>>>
>>> What kind of modes are there?
>>>
>>
>> Stream - new values overwrrite old values, Fifo - drop new values.
>
>
> So basically ring buffer) style (old samples are dropped and FIFO style (new
> samples are dropped). This mode should probably be aligned with the mode
> that the sw buffer is working. E.g. you wouldn't want the software buffer to
> work in FIFO mode and the hardware buffer in ring buffer mode. That would
> lead to weird effects.
>

Correct.

> We talked about adding support for ring buffer mode to the software buffer a
> while ago, but so far nobody really needed it so it hasn't been implemented
> yet.
>

This is something that would need ring buffer mode:

https://source.android.com/devices/sensors/batching.html#behavior_in_suspend_mode


Leaving the buffer mode aside, there is another piece that we need for
hardware fifo. We need the ability to flush the fifo data earlier then
the watermark trigger and / or fifo full trigger so that we can
balance power vs latency. It is probably equivalent to the timeout
parameter in the watermark patch.

Re: [RFC 0/8] iio: add support for hardware fifo

[Jonathan Cameron]

On 19/11/14 12:33, Octavian Purdila wrote:
> On Wed, Nov 19, 2014 at 1:48 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>> On 11/18/2014 08:35 PM, Octavian Purdila wrote:
>>>
>>> On Tue, Nov 18, 2014 at 7:23 PM, Lars-Peter Clausen <lars@metafoo.de>
>>> wrote:
>>>
>>>>>>> As far as I understood, the proposed watermark implementation only
>>>>>>> affects the device buffer and as I mentioned above that will not help
>>>>>>> with reducing the interrupt rate.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> By setting the watershed level the userspace application tells you that
>>>>>> it
>>>>>> is OK with getting data with a higher latency than one sample. This
>>>>>> allows
>>>>>> the driver to configure the FIFO level and hence reduce the interrupt
>>>>>> rate.
>>>>>>
>>>>>
>>>>> Hi Lars,
>>>>>
>>>>> The implementation (as proposed in the patch by Josselin and Yannick)
>>>>> does not inform the driver of changes to watermark, that is only
>>>>> visible to core iio / buffer logic.
>>>>
>>>>
>>>>
>>>> That should be trivial to add though.
>>>>
>>>
>>> True. I've actually started by implementing hardware fifo support as a
>>> new type of iio buffer, but I got scared by the buffer demux stuff. I
>>> can take another stab at it, if that sounds better?
>>>
>>>>>
>>>>> Also, the watermark alone is not enough to properly support hardware
>>>>> fifos: the fifo can operate in multiple modes, we need to read data
>>>>> from the hardware fifo even when the watermark interrupt is not issued
>>>>> (the flush operation in the current patch set).
>>>>
>>>>
>>>>
>>>> What kind of modes are there?
>>>>
>>>
>>> Stream - new values overwrrite old values, Fifo - drop new values.
>>
>>
>> So basically ring buffer) style (old samples are dropped and FIFO style (new
>> samples are dropped). This mode should probably be aligned with the mode
>> that the sw buffer is working. E.g. you wouldn't want the software buffer to
>> work in FIFO mode and the hardware buffer in ring buffer mode. That would
>> lead to weird effects.
>>
> 
> Correct.
> 
>> We talked about adding support for ring buffer mode to the software buffer a
>> while ago, but so far nobody really needed it so it hasn't been implemented
>> yet.
>>
> 
> This is something that would need ring buffer mode:
> 
> https://source.android.com/devices/sensors/batching.html#behavior_in_suspend_mode
> 
Hmm.. This gets ugly if we have a hardware ring buffer feeding a software ring buffer.
For remotely consistent data I guess we'd have to dump the software buffer whenever
we grab data from the hardware buffer that has overflowed.  
> 
> Leaving the buffer mode aside, there is another piece that we need for
> hardware fifo. We need the ability to flush the fifo data earlier then
> the watermark trigger and / or fifo full trigger so that we can
> balance power vs latency. It is probably equivalent to the timeout
> parameter in the watermark patch.
Not really as the timeout could be trivially implemented in userspace and
didn't cause a flush.  

The suggestion I made earlier of flushing the buffer if an attempt to read
more from the software buffer than it contains is made.  This wouldn't
happen in our 'normal' e.g. current usage.

> 

Re: [RFC 0/8] iio: add support for hardware fifo

[Octavian Purdila]

On Tue, Nov 18, 2014 at 9:35 PM, Octavian Purdila
<octavian.purdila@intel.com> wrote:
> On Tue, Nov 18, 2014 at 7:23 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>
>>>>> As far as I understood, the proposed watermark implementation only
>>>>> affects the device buffer and as I mentioned above that will not help
>>>>> with reducing the interrupt rate.
>>>>
>>>>
>>>>
>>>> By setting the watershed level the userspace application tells you that
>>>> it
>>>> is OK with getting data with a higher latency than one sample. This
>>>> allows
>>>> the driver to configure the FIFO level and hence reduce the interrupt
>>>> rate.
>>>>
>>>
>>> Hi Lars,
>>>
>>> The implementation (as proposed in the patch by Josselin and Yannick)
>>> does not inform the driver of changes to watermark, that is only
>>> visible to core iio / buffer logic.
>>
>>
>> That should be trivial to add though.
>>
>
> True. I've actually started by implementing hardware fifo support as a
> new type of iio buffer, but I got scared by the buffer demux stuff. I
> can take another stab at it, if that sounds better?
>

OK, I remembered why I bailed on that approach: it would break the
callback buffer. It looks like the buffer cb infrastructure relies on
a push model and for a hardware fifo implemented as a iio buffer we
would need a pull model. While there is one driver that takes this
approach (sca3000_ring.c) it is in staging and the hardware buffer
part seems to be marked as RFC.

BTW, I didn't find any users for the buffer cb infrastructure, what is
it used for?

Re: [RFC 0/8] iio: add support for hardware fifo

[Octavian Purdila]

On Wed, Nov 19, 2014 at 3:32 PM, Octavian Purdila
<octavian.purdila@intel.com> wrote:
> On Tue, Nov 18, 2014 at 9:35 PM, Octavian Purdila
> <octavian.purdila@intel.com> wrote:
>> On Tue, Nov 18, 2014 at 7:23 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>>
>>>>>> As far as I understood, the proposed watermark implementation only
>>>>>> affects the device buffer and as I mentioned above that will not help
>>>>>> with reducing the interrupt rate.
>>>>>
>>>>> By setting the watershed level the userspace application tells you that
>>>>> it
>>>>> is OK with getting data with a higher latency than one sample. This
>>>>> allows
>>>>> the driver to configure the FIFO level and hence reduce the interrupt
>>>>> rate.
>>>>
>>>> Hi Lars,
>>>>
>>>> The implementation (as proposed in the patch by Josselin and Yannick)
>>>> does not inform the driver of changes to watermark, that is only
>>>> visible to core iio / buffer logic.
>>>
>>> That should be trivial to add though.
>>
>> True. I've actually started by implementing hardware fifo support as a
>> new type of iio buffer, but I got scared by the buffer demux stuff. I
>> can take another stab at it, if that sounds better?
>>
> OK, I remembered why I bailed on that approach: it would break the
> callback buffer. It looks like the buffer cb infrastructure relies on
> a push model and for a hardware fifo implemented as a iio buffer we
> would need a pull model. While there is one driver that takes this
> approach (sca3000_ring.c) it is in staging and the hardware buffer
> part seems to be marked as RFC.
>

Hi again,

I want to revive this thread to help us start moving in the right
direction. Here is a summary of things discuss so far:

1. The watermark patch from Josselin and Yannick does not allow
reducing the interrupt rate because watermark is not propagated to the
driver level. It lacks setting the fifo mode (which is important for
Android use-cases). Also, we either need the timeout parameter or an
explicit flush trigger.

2. There are two approaches to implement hardware buffering:

a) The one exemplified in the current patch set, where we have
hardware buffers and based on interrupt or software trigger we push
data to device buffers. I'm going to call this the push model.

b) Implementing the hardware buffer as an iio buffer. That basically
means having the driver implement the read_first_n to read data
directly from the hardware buffer. I am going to call this the pull
model.

Although the pull model seems more natural it has some disadvantages:
it breaks the callback buffers (which do not seem to be used though),
it breaks in the case where we have a single hardware buffer but we
server multiple iio devices (sensor hub).

The push model has the disadvantage that we are using double buffering
and that we need to match the software and hardware fifo policies.

So, to move forward, I would like to build consensus on what is the
preferred model: push or pull?

Thanks,
Tavi

Re: [RFC 0/8] iio: add support for hardware fifo

[Lars-Peter Clausen]

On 11/26/2014 02:06 PM, Octavian Purdila wrote:
> On Wed, Nov 19, 2014 at 3:32 PM, Octavian Purdila
> <octavian.purdila@intel.com> wrote:
>> On Tue, Nov 18, 2014 at 9:35 PM, Octavian Purdila
>> <octavian.purdila@intel.com> wrote:
>>> On Tue, Nov 18, 2014 at 7:23 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>>>
>>>>>>> As far as I understood, the proposed watermark implementation only
>>>>>>> affects the device buffer and as I mentioned above that will not help
>>>>>>> with reducing the interrupt rate.
>>>>>>
>>>>>> By setting the watershed level the userspace application tells you that
>>>>>> it
>>>>>> is OK with getting data with a higher latency than one sample. This
>>>>>> allows
>>>>>> the driver to configure the FIFO level and hence reduce the interrupt
>>>>>> rate.
>>>>>
>>>>> Hi Lars,
>>>>>
>>>>> The implementation (as proposed in the patch by Josselin and Yannick)
>>>>> does not inform the driver of changes to watermark, that is only
>>>>> visible to core iio / buffer logic.
>>>>
>>>> That should be trivial to add though.
>>>
>>> True. I've actually started by implementing hardware fifo support as a
>>> new type of iio buffer, but I got scared by the buffer demux stuff. I
>>> can take another stab at it, if that sounds better?
>>>
>> OK, I remembered why I bailed on that approach: it would break the
>> callback buffer. It looks like the buffer cb infrastructure relies on
>> a push model and for a hardware fifo implemented as a iio buffer we
>> would need a pull model. While there is one driver that takes this
>> approach (sca3000_ring.c) it is in staging and the hardware buffer
>> part seems to be marked as RFC.
>>
>
> Hi again,
>
> I want to revive this thread to help us start moving in the right
> direction. Here is a summary of things discuss so far:
>
> 1. The watermark patch from Josselin and Yannick does not allow
> reducing the interrupt rate because watermark is not propagated to the
> driver level. It lacks setting the fifo mode (which is important for
> Android use-cases). Also, we either need the timeout parameter or an
> explicit flush trigger.
>
> 2. There are two approaches to implement hardware buffering:
>
> a) The one exemplified in the current patch set, where we have
> hardware buffers and based on interrupt or software trigger we push
> data to device buffers. I'm going to call this the push model.
>
> b) Implementing the hardware buffer as an iio buffer. That basically
> means having the driver implement the read_first_n to read data
> directly from the hardware buffer. I am going to call this the pull
> model.
>
> Although the pull model seems more natural it has some disadvantages:
> it breaks the callback buffers (which do not seem to be used though),
> it breaks in the case where we have a single hardware buffer but we
> server multiple iio devices (sensor hub).
>
> The push model has the disadvantage that we are using double buffering
> and that we need to match the software and hardware fifo policies.
>
> So, to move forward, I would like to build consensus on what is the
> preferred model: push or pull?

The two models have completely different semantics and which is the 
preferred probably depends on the use case and not necessarily on the device 
itself. Right now for IIO the buffered access uses a push approach and the 
sysfs access uses a pull approach. Although when using sysfs there is no 
buffering and is supposed to always fetch the latest conversion (Which is 
what the BCM150 seems to call bypass mode).

There might be a need to add support for switching between the two different 
modes depending on the userspace use-case. But to be honest I can't see how 
a pure pull based approach would be that use full.

Maybe hybrid is the way to go with the kernel driver moving samples from the 
hardware buffer to the software buffer if the hardware buffer gets to full. 
But at the same time allow userspace to read directly from the hardware 
buffer if there are no more samples in the software buffer.

- Lars

Re: [RFC 0/8] iio: add support for hardware fifo

[Octavian Purdila]

On Mon, Dec 1, 2014 at 11:19 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> On 11/26/2014 02:06 PM, Octavian Purdila wrote:
>>
>> On Wed, Nov 19, 2014 at 3:32 PM, Octavian Purdila
>> <octavian.purdila@intel.com> wrote:
>>>
>>> On Tue, Nov 18, 2014 at 9:35 PM, Octavian Purdila
>>> <octavian.purdila@intel.com> wrote:
>>>>
>>>> On Tue, Nov 18, 2014 at 7:23 PM, Lars-Peter Clausen <lars@metafoo.de>
>>>> wrote:
>>>>
>>>>>>>> As far as I understood, the proposed watermark implementation only
>>>>>>>> affects the device buffer and as I mentioned above that will not
>>>>>>>> help
>>>>>>>> with reducing the interrupt rate.
>>>>>>>
>>>>>>>
>>>>>>> By setting the watershed level the userspace application tells you
>>>>>>> that
>>>>>>> it
>>>>>>> is OK with getting data with a higher latency than one sample. This
>>>>>>> allows
>>>>>>> the driver to configure the FIFO level and hence reduce the interrupt
>>>>>>> rate.
>>>>>>
>>>>>>
>>>>>> Hi Lars,
>>>>>>
>>>>>> The implementation (as proposed in the patch by Josselin and Yannick)
>>>>>> does not inform the driver of changes to watermark, that is only
>>>>>> visible to core iio / buffer logic.
>>>>>
>>>>>
>>>>> That should be trivial to add though.
>>>>
>>>>
>>>> True. I've actually started by implementing hardware fifo support as a
>>>> new type of iio buffer, but I got scared by the buffer demux stuff. I
>>>> can take another stab at it, if that sounds better?
>>>>
>>> OK, I remembered why I bailed on that approach: it would break the
>>> callback buffer. It looks like the buffer cb infrastructure relies on
>>> a push model and for a hardware fifo implemented as a iio buffer we
>>> would need a pull model. While there is one driver that takes this
>>> approach (sca3000_ring.c) it is in staging and the hardware buffer
>>> part seems to be marked as RFC.
>>>
>>
>> Hi again,
>>
>> I want to revive this thread to help us start moving in the right
>> direction. Here is a summary of things discuss so far:
>>
>> 1. The watermark patch from Josselin and Yannick does not allow
>> reducing the interrupt rate because watermark is not propagated to the
>> driver level. It lacks setting the fifo mode (which is important for
>> Android use-cases). Also, we either need the timeout parameter or an
>> explicit flush trigger.
>>
>> 2. There are two approaches to implement hardware buffering:
>>
>> a) The one exemplified in the current patch set, where we have
>> hardware buffers and based on interrupt or software trigger we push
>> data to device buffers. I'm going to call this the push model.
>>
>> b) Implementing the hardware buffer as an iio buffer. That basically
>> means having the driver implement the read_first_n to read data
>> directly from the hardware buffer. I am going to call this the pull
>> model.
>>
>> Although the pull model seems more natural it has some disadvantages:
>> it breaks the callback buffers (which do not seem to be used though),
>> it breaks in the case where we have a single hardware buffer but we
>> server multiple iio devices (sensor hub).
>>
>> The push model has the disadvantage that we are using double buffering
>> and that we need to match the software and hardware fifo policies.
>>
>> So, to move forward, I would like to build consensus on what is the
>> preferred model: push or pull?
>
>
> The two models have completely different semantics and which is the
> preferred probably depends on the use case and not necessarily on the device
> itself.

I agree.

> Right now for IIO the buffered access uses a push approach and the
> sysfs access uses a pull approach. Although when using sysfs there is no
> buffering and is supposed to always fetch the latest conversion (Which is
> what the BCM150 seems to call bypass mode).
>
> There might be a need to add support for switching between the two different
> modes depending on the userspace use-case. But to be honest I can't see how
> a pure pull based approach would be that use full.
>

Yes, a pull based approach will either force userspace to poll - which
defeats the purpose of hardware buffering, or will increase the
latency as userspace will have to wait for a fifo full/watermark event
then read.

> Maybe hybrid is the way to go with the kernel driver moving samples from the
> hardware buffer to the software buffer if the hardware buffer gets to full.
> But at the same time allow userspace to read directly from the hardware
> buffer if there are no more samples in the software buffer.
>

I tried something similar in one of the earlier approaches: force a
fifo flush in iio_buffer_read_first_n_outer(). This will allow us to
get rid of the timeout parameter or the sysfs fifo flush entry as it
allows userspace to generate a fifo flush just by reading. Userspace
can wait for the fifo watermark/full and if that timeouts it can force
a read. I think this approach also cuts down some overhead.

I can rework this patchset as above if it sounds reasonable to you.

Re: [RFC 0/8] iio: add support for hardware fifo

[Jonathan Cameron]

On 02/12/14 09:13, Octavian Purdila wrote:
> On Mon, Dec 1, 2014 at 11:19 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>> On 11/26/2014 02:06 PM, Octavian Purdila wrote:
>>>
>>> On Wed, Nov 19, 2014 at 3:32 PM, Octavian Purdila
>>> <octavian.purdila@intel.com> wrote:
>>>>
>>>> On Tue, Nov 18, 2014 at 9:35 PM, Octavian Purdila
>>>> <octavian.purdila@intel.com> wrote:
>>>>>
>>>>> On Tue, Nov 18, 2014 at 7:23 PM, Lars-Peter Clausen <lars@metafoo.de>
>>>>> wrote:
>>>>>
>>>>>>>>> As far as I understood, the proposed watermark implementation only
>>>>>>>>> affects the device buffer and as I mentioned above that will not
>>>>>>>>> help
>>>>>>>>> with reducing the interrupt rate.
>>>>>>>>
>>>>>>>>
>>>>>>>> By setting the watershed level the userspace application tells you
>>>>>>>> that
>>>>>>>> it
>>>>>>>> is OK with getting data with a higher latency than one sample. This
>>>>>>>> allows
>>>>>>>> the driver to configure the FIFO level and hence reduce the interrupt
>>>>>>>> rate.
>>>>>>>
>>>>>>>
>>>>>>> Hi Lars,
>>>>>>>
>>>>>>> The implementation (as proposed in the patch by Josselin and Yannick)
>>>>>>> does not inform the driver of changes to watermark, that is only
>>>>>>> visible to core iio / buffer logic.
>>>>>>
>>>>>>
>>>>>> That should be trivial to add though.
>>>>>
>>>>>
>>>>> True. I've actually started by implementing hardware fifo support as a
>>>>> new type of iio buffer, but I got scared by the buffer demux stuff. I
>>>>> can take another stab at it, if that sounds better?
>>>>>
>>>> OK, I remembered why I bailed on that approach: it would break the
>>>> callback buffer. It looks like the buffer cb infrastructure relies on
>>>> a push model and for a hardware fifo implemented as a iio buffer we
>>>> would need a pull model. While there is one driver that takes this
>>>> approach (sca3000_ring.c) it is in staging and the hardware buffer
>>>> part seems to be marked as RFC.
>>>>
>>>
>>> Hi again,
>>>
>>> I want to revive this thread to help us start moving in the right
>>> direction. Here is a summary of things discuss so far:
>>>
>>> 1. The watermark patch from Josselin and Yannick does not allow
>>> reducing the interrupt rate because watermark is not propagated to the
>>> driver level. It lacks setting the fifo mode (which is important for
>>> Android use-cases). Also, we either need the timeout parameter or an
>>> explicit flush trigger.
>>>
>>> 2. There are two approaches to implement hardware buffering:
>>>
>>> a) The one exemplified in the current patch set, where we have
>>> hardware buffers and based on interrupt or software trigger we push
>>> data to device buffers. I'm going to call this the push model.
>>>
>>> b) Implementing the hardware buffer as an iio buffer. That basically
>>> means having the driver implement the read_first_n to read data
>>> directly from the hardware buffer. I am going to call this the pull
>>> model.
>>>
>>> Although the pull model seems more natural it has some disadvantages:
>>> it breaks the callback buffers (which do not seem to be used though),
>>> it breaks in the case where we have a single hardware buffer but we
>>> server multiple iio devices (sensor hub).
>>>
>>> The push model has the disadvantage that we are using double buffering
>>> and that we need to match the software and hardware fifo policies.
>>>
>>> So, to move forward, I would like to build consensus on what is the
>>> preferred model: push or pull?
>>
>>
>> The two models have completely different semantics and which is the
>> preferred probably depends on the use case and not necessarily on the device
>> itself.
> 
> I agree.
> 
>> Right now for IIO the buffered access uses a push approach and the
>> sysfs access uses a pull approach. Although when using sysfs there is no
>> buffering and is supposed to always fetch the latest conversion (Which is
>> what the BCM150 seems to call bypass mode).
>>
>> There might be a need to add support for switching between the two different
>> modes depending on the userspace use-case. But to be honest I can't see how
>> a pure pull based approach would be that use full.
>>
> 
> Yes, a pull based approach will either force userspace to poll - which
> defeats the purpose of hardware buffering, or will increase the
> latency as userspace will have to wait for a fifo full/watermark event
> then read.
> 
>> Maybe hybrid is the way to go with the kernel driver moving samples from the
>> hardware buffer to the software buffer if the hardware buffer gets to full.
>> But at the same time allow userspace to read directly from the hardware
>> buffer if there are no more samples in the software buffer.
>>
> 
> I tried something similar in one of the earlier approaches: force a
> fifo flush in iio_buffer_read_first_n_outer(). This will allow us to
> get rid of the timeout parameter or the sysfs fifo flush entry as it
> allows userspace to generate a fifo flush just by reading. Userspace
> can wait for the fifo watermark/full and if that timeouts it can force
> a read. I think this approach also cuts down some overhead.
> 
> I can rework this patchset as above if it sounds reasonable to you.
I should have read the whole thread before replying earlier (was too big :)
Anyhow, this is pretty much what I was suggesting earlier as well.
Obviously only force the flush if there isn't already enough data in the
front end buffer.

This should work fine even if we have multiple buffers (e.g. buffer_cb)
as if any of them force a read, the others will just get data earlier
than otherwise.  Note with the watershed stuff, note that the the one
relevant to the hardware will be that of whichever buffer has requested
the shortest.  We after all have to meet the strictest demand on latency.

Anyhow this sounds like the right direction to take to me.

Sorry for the slow reply!

Jonathan
> --
> 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: [RFC 0/8] iio: add support for hardware fifo

[Jonathan Cameron]

On 19/11/14 13:32, Octavian Purdila wrote:
> On Tue, Nov 18, 2014 at 9:35 PM, Octavian Purdila
> <octavian.purdila@intel.com> wrote:
>> On Tue, Nov 18, 2014 at 7:23 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>>
>>>>>> As far as I understood, the proposed watermark implementation only
>>>>>> affects the device buffer and as I mentioned above that will not help
>>>>>> with reducing the interrupt rate.
>>>>>
>>>>>
>>>>>
>>>>> By setting the watershed level the userspace application tells you that
>>>>> it
>>>>> is OK with getting data with a higher latency than one sample. This
>>>>> allows
>>>>> the driver to configure the FIFO level and hence reduce the interrupt
>>>>> rate.
>>>>>
>>>>
>>>> Hi Lars,
>>>>
>>>> The implementation (as proposed in the patch by Josselin and Yannick)
>>>> does not inform the driver of changes to watermark, that is only
>>>> visible to core iio / buffer logic.
>>>
>>>
>>> That should be trivial to add though.
>>>
>>
>> True. I've actually started by implementing hardware fifo support as a
>> new type of iio buffer, but I got scared by the buffer demux stuff. I
>> can take another stab at it, if that sounds better?
>>
> 
> OK, I remembered why I bailed on that approach: it would break the
> callback buffer. It looks like the buffer cb infrastructure relies on
> a push model and for a hardware fifo implemented as a iio buffer we
> would need a pull model. While there is one driver that takes this
> approach (sca3000_ring.c) it is in staging and the hardware buffer
> part seems to be marked as RFC.
Yup.  The sca3000 indeed breaks the cb buffer stuff.  The approach
of bouncing through a software buffer is definitely now the preferred
option.  The SCA3000 driver is old, very old...  Needs a rewrite
(which is why it is still in staging).  
> 
> BTW, I didn't find any users for the buffer cb infrastructure, what is
> it used for?
Ah, this was meant to be used for things like the iio input bridge.
That is to pass 'push' data on to other kernel drivers.  I never finished
up the iio input bridge (was nearly there) due to other stuff getting
in the way and no one else has taken up my suggestions to take it on...
It was always something that was demanded by users, but never enough
that anyone actually took it over.

Ultimately the idea was also to be able to cleanly separate the IIO userspace
interface from the back end. This was just one step along the way.
The point where we can have an iio driver that is a consumer of an iio driver
and provides all the same functionality is the point where we can cleanly
separate these parts.   If we were starting again I think we'd aim for
this separation from the start.  Ah well.

Note that for the full set of in kernel user interfaces we also
need to do the events interface at some point.

Re: [RFC 0/8] iio: add support for hardware fifo

[Jonathan Cameron]

On 18/11/14 15:03, Octavian Purdila wrote:
> On Tue, Nov 18, 2014 at 3:24 PM, <jic23@kernel.org> wrote:
>>
>> Octavian Purdila writes:
>>>
>>> Hi everybody,
>>
>> Hi Octavian.
>>>
> 
> Hi Jonathan,
> 
> Thanks for taking the time to look at this and for the detailed
> feedback. I am new to IIO, so I may not understand your comments
> fully, hope you will have patience with me :)
> 
>>>
>>> I hope this RFC is a good starting point to discuss support for
>>> hardware fifo in IIO. The main reason to support it is to reduce the
>>> power consumtion, by allowing the CPU to enter deep sleep states for
>>> longer periods of time.
>>
>> A worthwhile aim indeed.
> 
> BTW, the larger context into which we are looking at this, is the new
> batching Android API:
> 
> http://source.android.com/devices/sensors/batching.html
> 
>>>
>>>
>>> Don't get discourage by the large number of patches most of them are
>>> refactors in the bmc150 driver, to make it easier to add support for
>>> the hardware fifo (basically to make adding interrupts and
>>> events/triggers easier).
>>> For discussing the hardware fifo stuff, only the first and last
>>> patches are important: the first adds new IIO attributes so that we
>>> can expose the hardware fifo and the last implements hadware fifo for
>>> IIO (as an example of how would a device use the exposed attributes).
>>> Note that the attributes can be exposed on a per device or per channel
>>> basis, since it seems both types of hardware fifos exists: those that
>>> store all data in a single fifo (temperature, accelerometer,
>>> magnetometer, etc.) and those that have separate fifos for
>>> accelerometer, gyroscope, etc. Thankfully, at the driver level we just
>>> need to use the appropriate sharing level to support one mode or the
>>> other.
>>
>> If this is the case, then the buffered outputs will have to be separate
>> so we know what is coming.  E.g. it will have to be effectively treated
>> as separate iio_buffers.  Now we have devices that already do (see some
>> of the more interesting SOC ADCs)
>> Is this actually the case for the bmc150? If you could point at
>> devices where it is then it would be great (as mentioned above one
>> or two SOC ADCs have a bonkers level of flexibility).
>> My reading of the datasheet is that the data is interleaved in the
>> buffer for any channels enable.
>>
> 
> While working on this I did though about exposing the hardware fifo by
> implementing a custom iio bufer. However, the iio buffers are much
> more flexible that most hadware fifos. In bmc150' case, the fifo case
> store x,y,z data or x or y or z data. It can not store x,y or x,z,
> etc. And if I understand correctly, the user can enable x,z collection
> for the device buffer. Also I have noticed that there can be multiple
> buffers attached to a device, for what it seems to be demuxing
> buffers?
The demuxing is also used to allow handling the x,y cases etc you have
above.  Available_scan_masks sets which things the hardware supports, then
the demux will rip out the elements actually requested by userspace.

The other use is the inkernel 'push' interface.  The original target
was an iio to input bridge, but I got side tracked and no one else
has yet picked up that driver (google will find it for you) and pushed
it through the last few things needed to make it a nice input driver.
The biggest nasty corner is no one likes the iio_hwmon dt bindings and
at the moment the iio_input ones would be similar.  Other options
exist for setting up these links (configfs perhaps?) but nothing much
has happened on that front.  I have some configfs patches for other
userspace setup tasks - particularly creating sysfs based triggers etc
but those aren't ready to go yet either!

> 
> That is why I thought of keeping the hardware fifo decoupled from the
> buffers and add a flush operation that will read the data from the
> fifo and push it to the device buffer(s). This way we don't affect the
> demuxing buffers logic.
Yes. I think we'll need to do something like that - just with a different
userspace interface to trigger this attempt to fetch extra data.
> 
>>>
>>> Also note that this patch is orthogonal to the software watermark /
>>> batching patch send on the list a while back.
>>
>>
>> I'm not so sure this is orthogonal at all.   That proposal was actually
>> about hardware support.  I pushed back that I wanted any new interface
>> to work whether or not there was hardware support.  In a sense that is
>> a more complex problem - hence the discussion became a little bit focused
>> on that case.
>> The intent there was to hide the implementation details of exactly this
>> sort of hardware/software buffer interaction.
> 
> The reason I say they are orthogonal (IMHO) is that the software
> watermark patch will only help reducing the number of kernel/user
> context switches by allowing more data to gather before waking up
> userspace. However, we will still have interrupts waking up the
> processor to store the samples in the kernel buffer. With a hardware
> fifo we can reduce that number of interrupts.
Sure - but there is nothing stopping us having the same interface for
both cases.
> 
>> Perhaps some history is in order.
>> We had exactly what you propose here a long time back with the sca3000
>> accelerometer (which is why there is still core support for hardware
>> buffers).  The interface was precisely the watershed type you have
>> here.
>> A review by Arnd Bergman pointed out that this was seriously clunky.
>> It puts data related to the in-band data flow into our out-of-band channel.
>> His suggestion was to allow for watershed interrupts using one of the
>> more interesting POLL types leaving the main poll for the data flow.
>> Arnd also suggested the bits about using anonymous chardevs for the event
>> reporting etc.  The unusual form of poll bit never got implemented,
>> but in the interests of moving forward with the common case and because
>> they were on there way out anyway the watershed interrupts were dropped.
> 
> You lost me here :) but I will look for that discussion and try to
> understand Arnd's review.
In short we did the way you suggest (more or less) before and decided it
was a bad route long run ;)
> 
>> The more recent discussion came to the conclusion that there was no need
>> to use the weird forms of poll.  We could simply have an attribute to
>> control when poll was fired.  The only bit that caused friction was whether
>> there should be a timeout or not.
>> Now, when then ti_am35xx driver came along it became clear that it wasn't
>> actually terribly useful exposing the hardware buffers directly to
>> user space.  The buffers tend not to be terribly long (in your bma160) I
>> see they are only 32 elements.
> 
> As I mentioned before, the main gain of using hardware fifo is to
> reduce the number of interrupts. Right now, if we have a rate of 25HZ
> we will get 25 interrupts per second. With hardware fifos we can
> reduce that significanlty, and even small buffers help.
Of course, that is why the hardware support is again becoming more common
(there was a phase a few years ago when this was happening, but it seems
it then went away again, probably because android etc didn't care)
> 
> 
>>  As such the conclusion was that it makes
>> more sense to use the buffers as temporary storage to smooth out the
>> data flow.  Thus that driver fills a software buffer from a hardware
>> buffer.  This seems the method that is most likely to work long term.
>> I note this is effectively what you have here, though I'm not sure of
>> the advantage of the explicit software flush over just reading the data
>> whenever the interrupt fires.
> 
> The explicit software flush allows userspace to balance power and
> latency. Userspace can use a timeout based poll to guarantee a maximum
> latency.  If we have enough data then we get an interrupt, we flush
> the fifo and read the data. If not, we get a timeout, we flush the
> fifo and read the data. Perhaps the explicit flush is not optimal and
> we can find out other ways, but in the end we should have a way to
> allow userspace to balance power and latency.
I wonder if we can have a callback in the driver that will be called to
see if more data is available if a 'too large' read is attempted?
Thus to do a full pull, one would simply read the size of the software
buffer + that of the hardware buffer (the maximum data that might be
available somewhere) Or perhaps twice the software buffer length...

Under normal circumstances userspace would never read more than is in
the software buffer so our current case would not change.  However
if needed this would trigger a hardware flush.
> 
>> Hence we started with something that at least superficially (I haven't
>> had a chance to go through the implementation in detail yet)
>> looks similar to what you have but ended up changing the method of
>> signalling to and from userspace.
>> Hardware buffer -> Software buffer -> user space.
>> Userspace watershed control -> Software buffer watershed control
>> -> Hardware buffer watershed control.
>> If userspace sets the watershed to say 16 then, as well as setting
>> that level in the software buffer it should be passed on to the
>> device driver allowing the watershed there to be set appropriately.
>> Now things get interesting if userspace sets the watershed to a value
>> that makes no sense for the hardware (say 17 on a device that does
>> power of 2 values only) as then it will have to fall back to
>> grabbing every one (Watershed of 1).  Perhaps we can provide 'hints'
>> for this?
> 
> As far as I understood, the proposed watermark implementation only
> affects the device buffer and as I mentioned above that will not help
> with reducing the interrupt rate.
> 
Plan was always to do more with it.  Like many nice bits and pieces
it fell by the wayside when the changes requested were very minor...

Ah well,

Jonathan

Re: [RFC 0/8] iio: add support for hardware fifo

[Pandruvada, Srinivas]

T24gVHVlLCAyMDE0LTExLTE4IGF0IDEzOjI0ICswMDAwLCBqaWMyM0BrZXJuZWwub3JnIHdyb3Rl
Og0KPiBPY3RhdmlhbiBQdXJkaWxhIHdyaXRlczogDQo+IA0KPiA+IEhpIGV2ZXJ5Ym9keSwNCj4g
SGkgT2N0YXZpYW4uDQo+ID4gDQo+ID4gSSBob3BlIHRoaXMgUkZDIGlzIGEgZ29vZCBzdGFydGlu
ZyBwb2ludCB0byBkaXNjdXNzIHN1cHBvcnQgZm9yDQo+ID4gaGFyZHdhcmUgZmlmbyBpbiBJSU8u
IFRoZSBtYWluIHJlYXNvbiB0byBzdXBwb3J0IGl0IGlzIHRvIHJlZHVjZSB0aGUNCj4gPiBwb3dl
ciBjb25zdW10aW9uLCBieSBhbGxvd2luZyB0aGUgQ1BVIHRvIGVudGVyIGRlZXAgc2xlZXAgc3Rh
dGVzIGZvcg0KPiA+IGxvbmdlciBwZXJpb2RzIG9mIHRpbWUuDQo+IEEgd29ydGh3aGlsZSBhaW0g
aW5kZWVkLg0KPiA+IA0KPiA+IERvbid0IGdldCBkaXNjb3VyYWdlIGJ5IHRoZSBsYXJnZSBudW1i
ZXIgb2YgcGF0Y2hlcyBtb3N0IG9mIHRoZW0gYXJlDQo+ID4gcmVmYWN0b3JzIGluIHRoZSBibWMx
NTAgZHJpdmVyLCB0byBtYWtlIGl0IGVhc2llciB0byBhZGQgc3VwcG9ydCBmb3INCj4gPiB0aGUg
aGFyZHdhcmUgZmlmbyAoYmFzaWNhbGx5IHRvIG1ha2UgYWRkaW5nIGludGVycnVwdHMgYW5kDQo+
ID4gZXZlbnRzL3RyaWdnZXJzIGVhc2llcikuIA0KPiA+IA0KPiA+IEZvciBkaXNjdXNzaW5nIHRo
ZSBoYXJkd2FyZSBmaWZvIHN0dWZmLCBvbmx5IHRoZSBmaXJzdCBhbmQgbGFzdA0KPiA+IHBhdGNo
ZXMgYXJlIGltcG9ydGFudDogdGhlIGZpcnN0IGFkZHMgbmV3IElJTyBhdHRyaWJ1dGVzIHNvIHRo
YXQgd2UNCj4gPiBjYW4gZXhwb3NlIHRoZSBoYXJkd2FyZSBmaWZvIGFuZCB0aGUgbGFzdCBpbXBs
ZW1lbnRzIGhhZHdhcmUgZmlmbyBmb3INCj4gPiBJSU8gKGFzIGFuIGV4YW1wbGUgb2YgaG93IHdv
dWxkIGEgZGV2aWNlIHVzZSB0aGUgZXhwb3NlZCBhdHRyaWJ1dGVzKS4gDQo+ID4gDQo+ID4gTm90
ZSB0aGF0IHRoZSBhdHRyaWJ1dGVzIGNhbiBiZSBleHBvc2VkIG9uIGEgcGVyIGRldmljZSBvciBw
ZXIgY2hhbm5lbA0KPiA+IGJhc2lzLCBzaW5jZSBpdCBzZWVtcyBib3RoIHR5cGVzIG9mIGhhcmR3
YXJlIGZpZm9zIGV4aXN0czogdGhvc2UgdGhhdA0KPiA+IHN0b3JlIGFsbCBkYXRhIGluIGEgc2lu
Z2xlIGZpZm8gKHRlbXBlcmF0dXJlLCBhY2NlbGVyb21ldGVyLA0KPiA+IG1hZ25ldG9tZXRlciwg
ZXRjLikgYW5kIHRob3NlIHRoYXQgaGF2ZSBzZXBhcmF0ZSBmaWZvcyBmb3INCj4gPiBhY2NlbGVy
b21ldGVyLCBneXJvc2NvcGUsIGV0Yy4gVGhhbmtmdWxseSwgYXQgdGhlIGRyaXZlciBsZXZlbCB3
ZSBqdXN0DQo+ID4gbmVlZCB0byB1c2UgdGhlIGFwcHJvcHJpYXRlIHNoYXJpbmcgbGV2ZWwgdG8g
c3VwcG9ydCBvbmUgbW9kZSBvciB0aGUNCj4gPiBvdGhlci4NCj4gSWYgdGhpcyBpcyB0aGUgY2Fz
ZSwgdGhlbiB0aGUgYnVmZmVyZWQgb3V0cHV0cyB3aWxsIGhhdmUgdG8gYmUgc2VwYXJhdGUNCj4g
c28gd2Uga25vdyB3aGF0IGlzIGNvbWluZy4gIEUuZy4gaXQgd2lsbCBoYXZlIHRvIGJlIGVmZmVj
dGl2ZWx5IHRyZWF0ZWQNCj4gYXMgc2VwYXJhdGUgaWlvX2J1ZmZlcnMuICBOb3cgd2UgaGF2ZSBk
ZXZpY2VzIHRoYXQgYWxyZWFkeSBkbyAoc2VlIHNvbWUNCj4gb2YgdGhlIG1vcmUgaW50ZXJlc3Rp
bmcgU09DIEFEQ3MpIA0KPiANCj4gSXMgdGhpcyBhY3R1YWxseSB0aGUgY2FzZSBmb3IgdGhlIGJt
YzE1MD8gSWYgeW91IGNvdWxkIHBvaW50IGF0DQo+IGRldmljZXMgd2hlcmUgaXQgaXMgdGhlbiBp
dCB3b3VsZCBiZSBncmVhdCAoYXMgbWVudGlvbmVkIGFib3ZlIG9uZQ0KPiBvciB0d28gU09DIEFE
Q3MgaGF2ZSBhIGJvbmtlcnMgbGV2ZWwgb2YgZmxleGliaWxpdHkpLg0KPiBNeSByZWFkaW5nIG9m
IHRoZSBkYXRhc2hlZXQgaXMgdGhhdCB0aGUgZGF0YSBpcyBpbnRlcmxlYXZlZCBpbiB0aGUNCj4g
YnVmZmVyIGZvciBhbnkgY2hhbm5lbHMgZW5hYmxlLiANCj4gDQo+ID4gDQo+ID4gQWxzbyBub3Rl
IHRoYXQgdGhpcyBwYXRjaCBpcyBvcnRob2dvbmFsIHRvIHRoZSBzb2Z0d2FyZSB3YXRlcm1hcmsg
Lw0KPiA+IGJhdGNoaW5nIHBhdGNoIHNlbmQgb24gdGhlIGxpc3QgYSB3aGlsZSBiYWNrLg0KPiAN
CkkgdGhpbmsgbGFzdCB3YXRlcnNoZWQgcGF0Y2ggd2FzIHJldmlld2VkIGFuZCBhZ3JlZWQgdXBv
biwgYnV0IGxvb2tzDQpsaWtlIG5ldmVyIG1lcmdlZC4gSSB0aGluayB0aGUgYXV0aG9yIGRyb3Bw
ZWQgdGhlIGFwcHJvYWNoLiBJIHRoaW5rIGFzIGENCnN0YXJ0IHdlIHNob3VsZCBtZXJnZSB0aGlz
IHBhdGNoLiBJbiBhZGRpdGlvbiB0aG9zZSB3YXRlcnNoZWRzIHNob3VsZCBiZQ0KZXhwb3NlZCB0
byBJSU8gZHJpdmVycywgc28gdGhhdCBpbmRpdmlkdWFsIGRyaXZlcnMgY2FuIGVzdGltYXRlIEZJ
Rk8NCnNpemUgaW4gY29tYmluYXRpb24gd2l0aCBzYW1wbGluZyBmcmVxdWVuY3kuIA0KPiBJJ20g
bm90IHNvIHN1cmUgdGhpcyBpcyBvcnRob2dvbmFsIGF0IGFsbC4gICBUaGF0IHByb3Bvc2FsIHdh
cyBhY3R1YWxseQ0KPiBhYm91dCBoYXJkd2FyZSBzdXBwb3J0LiAgSSBwdXNoZWQgYmFjayB0aGF0
IEkgd2FudGVkIGFueSBuZXcgaW50ZXJmYWNlDQo+IHRvIHdvcmsgd2hldGhlciBvciBub3QgdGhl
cmUgd2FzIGhhcmR3YXJlIHN1cHBvcnQuICBJbiBhIHNlbnNlIHRoYXQgaXMNCj4gYSBtb3JlIGNv
bXBsZXggcHJvYmxlbSAtIGhlbmNlIHRoZSBkaXNjdXNzaW9uIGJlY2FtZSBhIGxpdHRsZSBiaXQg
Zm9jdXNlZA0KPiBvbiB0aGF0IGNhc2UuIA0KPiANCj4gVGhlIGludGVudCB0aGVyZSB3YXMgdG8g
aGlkZSB0aGUgaW1wbGVtZW50YXRpb24gZGV0YWlscyBvZiBleGFjdGx5IHRoaXMNCj4gc29ydCBv
ZiBoYXJkd2FyZS9zb2Z0d2FyZSBidWZmZXIgaW50ZXJhY3Rpb24uIA0KPiANCj4gUGVyaGFwcyBz
b21lIGhpc3RvcnkgaXMgaW4gb3JkZXIuIA0KPiANCj4gV2UgaGFkIGV4YWN0bHkgd2hhdCB5b3Ug
cHJvcG9zZSBoZXJlIGEgbG9uZyB0aW1lIGJhY2sgd2l0aCB0aGUgc2NhMzAwMA0KPiBhY2NlbGVy
b21ldGVyICh3aGljaCBpcyB3aHkgdGhlcmUgaXMgc3RpbGwgY29yZSBzdXBwb3J0IGZvciBoYXJk
d2FyZQ0KPiBidWZmZXJzKS4gIFRoZSBpbnRlcmZhY2Ugd2FzIHByZWNpc2VseSB0aGUgd2F0ZXJz
aGVkIHR5cGUgeW91IGhhdmUNCj4gaGVyZS4gDQo+IA0KPiBBIHJldmlldyBieSBBcm5kIEJlcmdt
YW4gcG9pbnRlZCBvdXQgdGhhdCB0aGlzIHdhcyBzZXJpb3VzbHkgY2x1bmt5LiANCj4gDQo+IEl0
IHB1dHMgZGF0YSByZWxhdGVkIHRvIHRoZSBpbi1iYW5kIGRhdGEgZmxvdyBpbnRvIG91ciBvdXQt
b2YtYmFuZCBjaGFubmVsLg0KPiBIaXMgc3VnZ2VzdGlvbiB3YXMgdG8gYWxsb3cgZm9yIHdhdGVy
c2hlZCBpbnRlcnJ1cHRzIHVzaW5nIG9uZSBvZiB0aGUNCj4gbW9yZSBpbnRlcmVzdGluZyBQT0xM
IHR5cGVzIGxlYXZpbmcgdGhlIG1haW4gcG9sbCBmb3IgdGhlIGRhdGEgZmxvdy4NCj4gQXJuZCBh
bHNvIHN1Z2dlc3RlZCB0aGUgYml0cyBhYm91dCB1c2luZyBhbm9ueW1vdXMgY2hhcmRldnMgZm9y
IHRoZSBldmVudA0KPiByZXBvcnRpbmcgZXRjLiAgVGhlIHVudXN1YWwgZm9ybSBvZiBwb2xsIGJp
dCBuZXZlciBnb3QgaW1wbGVtZW50ZWQsDQo+IGJ1dCBpbiB0aGUgaW50ZXJlc3RzIG9mIG1vdmlu
ZyBmb3J3YXJkIHdpdGggdGhlIGNvbW1vbiBjYXNlIGFuZCBiZWNhdXNlDQo+IHRoZXkgd2VyZSBv
biB0aGVyZSB3YXkgb3V0IGFueXdheSB0aGUgd2F0ZXJzaGVkIGludGVycnVwdHMgd2VyZSBkcm9w
cGVkLiANCj4gDQo+IFRoZSBtb3JlIHJlY2VudCBkaXNjdXNzaW9uIGNhbWUgdG8gdGhlIGNvbmNs
dXNpb24gdGhhdCB0aGVyZSB3YXMgbm8gbmVlZA0KPiB0byB1c2UgdGhlIHdlaXJkIGZvcm1zIG9m
IHBvbGwuICBXZSBjb3VsZCBzaW1wbHkgaGF2ZSBhbiBhdHRyaWJ1dGUgdG8NCj4gY29udHJvbCB3
aGVuIHBvbGwgd2FzIGZpcmVkLiAgVGhlIG9ubHkgYml0IHRoYXQgY2F1c2VkIGZyaWN0aW9uIHdh
cyB3aGV0aGVyDQo+IHRoZXJlIHNob3VsZCBiZSBhIHRpbWVvdXQgb3Igbm90LiANCj4gDQo+IE5v
dywgd2hlbiB0aGVuIHRpX2FtMzV4eCBkcml2ZXIgY2FtZSBhbG9uZyBpdCBiZWNhbWUgY2xlYXIg
dGhhdCBpdCB3YXNuJ3QNCj4gYWN0dWFsbHkgdGVycmlibHkgdXNlZnVsIGV4cG9zaW5nIHRoZSBo
YXJkd2FyZSBidWZmZXJzIGRpcmVjdGx5IHRvDQo+IHVzZXIgc3BhY2UuICBUaGUgYnVmZmVycyB0
ZW5kIG5vdCB0byBiZSB0ZXJyaWJseSBsb25nIChpbiB5b3VyIGJtYTE2MCkgSQ0KPiBzZWUgdGhl
eSBhcmUgb25seSAzMiBlbGVtZW50cy4gIEFzIHN1Y2ggdGhlIGNvbmNsdXNpb24gd2FzIHRoYXQg
aXQgbWFrZXMNCj4gbW9yZSBzZW5zZSB0byB1c2UgdGhlIGJ1ZmZlcnMgYXMgdGVtcG9yYXJ5IHN0
b3JhZ2UgdG8gc21vb3RoIG91dCB0aGUNCj4gZGF0YSBmbG93LiAgVGh1cyB0aGF0IGRyaXZlciBm
aWxscyBhIHNvZnR3YXJlIGJ1ZmZlciBmcm9tIGEgaGFyZHdhcmUNCj4gYnVmZmVyLiAgVGhpcyBz
ZWVtcyB0aGUgbWV0aG9kIHRoYXQgaXMgbW9zdCBsaWtlbHkgdG8gd29yayBsb25nIHRlcm0uDQo+
IEkgbm90ZSB0aGlzIGlzIGVmZmVjdGl2ZWx5IHdoYXQgeW91IGhhdmUgaGVyZSwgdGhvdWdoIEkn
bSBub3Qgc3VyZSBvZg0KPiB0aGUgYWR2YW50YWdlIG9mIHRoZSBleHBsaWNpdCBzb2Z0d2FyZSBm
bHVzaCBvdmVyIGp1c3QgcmVhZGluZyB0aGUgZGF0YQ0KPiB3aGVuZXZlciB0aGUgaW50ZXJydXB0
IGZpcmVzLiANCj4gDQo+IEhlbmNlIHdlIHN0YXJ0ZWQgd2l0aCBzb21ldGhpbmcgdGhhdCBhdCBs
ZWFzdCBzdXBlcmZpY2lhbGx5IChJIGhhdmVuJ3QNCj4gaGFkIGEgY2hhbmNlIHRvIGdvIHRocm91
Z2ggdGhlIGltcGxlbWVudGF0aW9uIGluIGRldGFpbCB5ZXQpDQo+IGxvb2tzIHNpbWlsYXIgdG8g
d2hhdCB5b3UgaGF2ZSBidXQgZW5kZWQgdXAgY2hhbmdpbmcgdGhlIG1ldGhvZCBvZg0KPiBzaWdu
YWxsaW5nIHRvIGFuZCBmcm9tIHVzZXJzcGFjZS4gDQo+IA0KPiBIYXJkd2FyZSBidWZmZXIgLT4g
U29mdHdhcmUgYnVmZmVyIC0+IHVzZXIgc3BhY2UuIA0KPiANCj4gVXNlcnNwYWNlIHdhdGVyc2hl
ZCBjb250cm9sIC0+IFNvZnR3YXJlIGJ1ZmZlciB3YXRlcnNoZWQgY29udHJvbA0KPiAgLT4gSGFy
ZHdhcmUgYnVmZmVyIHdhdGVyc2hlZCBjb250cm9sLiANCj4gDQo+IElmIHVzZXJzcGFjZSBzZXRz
IHRoZSB3YXRlcnNoZWQgdG8gc2F5IDE2IHRoZW4sIGFzIHdlbGwgYXMgc2V0dGluZw0KPiB0aGF0
IGxldmVsIGluIHRoZSBzb2Z0d2FyZSBidWZmZXIgaXQgc2hvdWxkIGJlIHBhc3NlZCBvbiB0byB0
aGUNCj4gZGV2aWNlIGRyaXZlciBhbGxvd2luZyB0aGUgd2F0ZXJzaGVkIHRoZXJlIHRvIGJlIHNl
dCBhcHByb3ByaWF0ZWx5Lg0KPiBOb3cgdGhpbmdzIGdldCBpbnRlcmVzdGluZyBpZiB1c2Vyc3Bh
Y2Ugc2V0cyB0aGUgd2F0ZXJzaGVkIHRvIGEgdmFsdWUNCj4gdGhhdCBtYWtlcyBubyBzZW5zZSBm
b3IgdGhlIGhhcmR3YXJlIChzYXkgMTcgb24gYSBkZXZpY2UgdGhhdCBkb2VzDQo+IHBvd2VyIG9m
IDIgdmFsdWVzIG9ubHkpIGFzIHRoZW4gaXQgd2lsbCBoYXZlIHRvIGZhbGwgYmFjayB0bw0KPiBn
cmFiYmluZyBldmVyeSBvbmUgKFdhdGVyc2hlZCBvZiAxKS4gIFBlcmhhcHMgd2UgY2FuIHByb3Zp
ZGUgJ2hpbnRzJw0KPiBmb3IgdGhpcz8gDQo+IA0KPiBBbnlob3csIGhhdmluZyBnaXZlbiB0aGlz
IGdlbmVyYWwgY29tbWVudCwgSSdsbCBnaXZlIHNvbWUgZmVlZGJhY2sgb24NCj4gdGhlIGltcGxl
bWVudGF0aW9uIGRldGFpbHMuIA0KPiANCj4gSm9uYXRoYW4gDQo+IA0KPiA+IA0KPiA+IE9jdGF2
aWFuIFB1cmRpbGEgKDgpOg0KPiA+ICAgaWlvOiBhZGQgc3VwcG9ydCBmb3IgaGFyZHdhcmUgZmlm
bw0KPiA+ICAgaWlvOiBibWMxNTA6IHJlZmFjdG9yIHNsb3BlIGR1cmF0aW9uIGFuZCB0aHJlc2hv
bGQgdXBkYXRlDQo+ID4gICBpaW86IGJtYzE1MDogcmVmYWN0b3IgaW50ZXJydXB0IGVuYWJsaW5n
DQo+ID4gICBpaW86IGJtYzE1MDogZXhpdCBlYXJseSBpZiBldmVudCAvIHRyaWdnZXIgc3RhdGUg
aXMgbm90IGNoYW5nZWQNCj4gPiAgIGlpbzogYm1jMTUwOiBpbnRyb2R1Y2UgYm1jMTUwX2FjY2Vs
X2ludGVycnVwdA0KPiA+ICAgaWlvOiBibWMxNTA6IGludHJvZHVjZSBibWMxNTBfYWNjZWxfdHJp
Z2dlcg0KPiA+ICAgaWlvOiBibWMxNTA6IGludHJvZHVjZSBibWMxNTBfYWNjZWxfZXZlbnQNCj4g
PiAgIGlpbzogYm1jMTUwOiBhZGQgc3VwcG9ydCBmb3IgaGFyZHdhcmUgZmlmbyANCj4gPiANCj4g
PiAgRG9jdW1lbnRhdGlvbi9BQkkvdGVzdGluZy9zeXNmcy1idXMtaWlvIHwgIDUxICsrDQo+ID4g
IGRyaXZlcnMvaWlvL2FjY2VsL2JtYzE1MC1hY2NlbC5jICAgICAgICB8IDk3NiArKysrKysrKysr
KysrKysrKysrKysrLS0tLS0tLS0tLQ0KPiA+ICBkcml2ZXJzL2lpby9pbmR1c3RyaWFsaW8tY29y
ZS5jICAgICAgICAgfCAgIDIgKw0KPiA+ICBkcml2ZXJzL2lpby9pbmR1c3RyaWFsaW8tZXZlbnQu
YyAgICAgICAgfCAgIDIgKw0KPiA+ICBpbmNsdWRlL2xpbnV4L2lpby9paW8uaCAgICAgICAgICAg
ICAgICAgfCAgMTcgKw0KPiA+ICBpbmNsdWRlL2xpbnV4L2lpby90eXBlcy5oICAgICAgICAgICAg
ICAgfCAgIDIgKw0KPiA+ICA2IGZpbGVzIGNoYW5nZWQsIDczOSBpbnNlcnRpb25zKCspLCAzMTEg
ZGVsZXRpb25zKC0pIA0KPiA+IA0KPiA+IC0tIA0KPiA+IDEuOS4xIA0KPiA+IA0KPiA+IC0tDQo+
ID4gVG8gdW5zdWJzY3JpYmUgZnJvbSB0aGlzIGxpc3Q6IHNlbmQgdGhlIGxpbmUgInVuc3Vic2Ny
aWJlIGxpbnV4LWlpbyIgaW4NCj4gPiB0aGUgYm9keSBvZiBhIG1lc3NhZ2UgdG8gbWFqb3Jkb21v
QHZnZXIua2VybmVsLm9yZw0KPiA+IE1vcmUgbWFqb3Jkb21vIGluZm8gYXQgIGh0dHA6Ly92Z2Vy
Lmtlcm5lbC5vcmcvbWFqb3Jkb21vLWluZm8uaHRtbA0KDQo=

Re: [RFC 0/8] iio: add support for hardware fifo

[Lars-Peter Clausen]

On 11/18/2014 02:24 PM, jic23@kernel.org wrote:
> Octavian Purdila writes:
>> Hi everybody,
> Hi Octavian.
>>
>> I hope this RFC is a good starting point to discuss support for
>> hardware fifo in IIO. The main reason to support it is to reduce the
>> power consumtion, by allowing the CPU to enter deep sleep states for
>> longer periods of time.
> A worthwhile aim indeed.
>>
>> Don't get discourage by the large number of patches most of them are
>> refactors in the bmc150 driver, to make it easier to add support for
>> the hardware fifo (basically to make adding interrupts and
>> events/triggers easier).
>> For discussing the hardware fifo stuff, only the first and last
>> patches are important: the first adds new IIO attributes so that we
>> can expose the hardware fifo and the last implements hadware fifo for
>> IIO (as an example of how would a device use the exposed attributes).
>> Note that the attributes can be exposed on a per device or per channel
>> basis, since it seems both types of hardware fifos exists: those that
>> store all data in a single fifo (temperature, accelerometer,
>> magnetometer, etc.) and those that have separate fifos for
>> accelerometer, gyroscope, etc. Thankfully, at the driver level we just
>> need to use the appropriate sharing level to support one mode or the
>> other.
> If this is the case, then the buffered outputs will have to be separate
> so we know what is coming.  E.g. it will have to be effectively treated
> as separate iio_buffers.  Now we have devices that already do (see some
> of the more interesting SOC ADCs)
> Is this actually the case for the bmc150? If you could point at
> devices where it is then it would be great (as mentioned above one
> or two SOC ADCs have a bonkers level of flexibility).
> My reading of the datasheet is that the data is interleaved in the
> buffer for any channels enable.
>>
>> Also note that this patch is orthogonal to the software watermark /
>> batching patch send on the list a while back.
>
> I'm not so sure this is orthogonal at all.   That proposal was actually
> about hardware support.  I pushed back that I wanted any new interface
> to work whether or not there was hardware support.  In a sense that is
> a more complex problem - hence the discussion became a little bit focused
> on that case.
> The intent there was to hide the implementation details of exactly this
> sort of hardware/software buffer interaction.
> Perhaps some history is in order.
> We had exactly what you propose here a long time back with the sca3000
> accelerometer (which is why there is still core support for hardware
> buffers).  The interface was precisely the watershed type you have
> here.
> A review by Arnd Bergman pointed out that this was seriously clunky.
> It puts data related to the in-band data flow into our out-of-band channel.
> His suggestion was to allow for watershed interrupts using one of the
> more interesting POLL types leaving the main poll for the data flow.
> Arnd also suggested the bits about using anonymous chardevs for the event
> reporting etc.  The unusual form of poll bit never got implemented,
> but in the interests of moving forward with the common case and because
> they were on there way out anyway the watershed interrupts were dropped.
> The more recent discussion came to the conclusion that there was no need
> to use the weird forms of poll.  We could simply have an attribute to
> control when poll was fired.  The only bit that caused friction was whether
> there should be a timeout or not.
> Now, when then ti_am35xx driver came along it became clear that it wasn't
> actually terribly useful exposing the hardware buffers directly to
> user space.  The buffers tend not to be terribly long (in your bma160) I
> see they are only 32 elements.  As such the conclusion was that it makes
> more sense to use the buffers as temporary storage to smooth out the
> data flow.  Thus that driver fills a software buffer from a hardware
> buffer.  This seems the method that is most likely to work long term.
> I note this is effectively what you have here, though I'm not sure of
> the advantage of the explicit software flush over just reading the data
> whenever the interrupt fires.
> Hence we started with something that at least superficially (I haven't
> had a chance to go through the implementation in detail yet)
> looks similar to what you have but ended up changing the method of
> signalling to and from userspace.
> Hardware buffer -> Software buffer -> user space.
> Userspace watershed control -> Software buffer watershed control
> -> Hardware buffer watershed control.
> If userspace sets the watershed to say 16 then, as well as setting
> that level in the software buffer it should be passed on to the
> device driver allowing the watershed there to be set appropriately.
> Now things get interesting if userspace sets the watershed to a value
> that makes no sense for the hardware (say 17 on a device that does
> power of 2 values only) as then it will have to fall back to
> grabbing every one (Watershed of 1).  Perhaps we can provide 'hints'
> for this?

Fully agreed, I do have a accelerometer in the pipeline which has a built-in 
FIFO and the plan was to do exactly what you just described. Pick up the 
watershed patches and then configure the FIFO threshold level according to 
the configured level.

In my opinion the internal FIFO in a device should be as transparent to 
userspace as possible. E.g. instead of having to separately flush the FIFO 
the FIFO should be flushed when the buffer is enabled, etc.

- Lars