[PATCH v2 0/5] iio: Add output buffer support

[PATCH v2 0/5] iio: Add output buffer support

[Alexandru Ardelean]

This patchset is based on the new multibuffer set.
It doesn't require the high-speed/mmap interface.
That will come later on a v2.

Changelog v1 -> v2:
* https://lore.kernel.org/linux-iio/20210212102021.47276-4-alexandru.ardelean@analog.com/T/#u
* removed DMA patches for now
* in patch 'iio: Add output buffer support'
  - added /sys/bus/iio/devices/iio:deviceX/bufferY/direction attribute
  - thinking about this, an update to the new buffer infrastructure is
    required when adding ADDAC/MxFE/transceivers; right now there is no
    problem, because we have only ADCs and DACs; but when we get
    transceivers, a bufferY/ directory needs to filter in/out
    scan_elements/ ; this only occured to me recently (it's one of those
    things that pops up later in mind)
* added 'iio: Documentation: update definitions for bufferY and scan_elements'
  - seems I forgot this on the original multibuffer patchset
* added 'iio: triggered-buffer: extend support to configure output buffers'
  - basically output triggered buffer support
* added 'iio: dac: ad5686: Add PWM as a trigger source'
  - this is a first user of this infrastructure

Alexandru Ardelean (2):
  iio: Documentation: update definitions for bufferY and scan_elements
  iio: triggered-buffer: extend support to configure output buffers

Lars-Peter Clausen (2):
  iio: Add output buffer support
  iio: kfifo-buffer: Add output buffer support

Mircea Caprioru (1):
  iio: dac: ad5686: Add PWM as a trigger source

 Documentation/ABI/testing/sysfs-bus-iio       |  92 +++++++++++
 drivers/iio/accel/adxl372.c                   |   1 +
 drivers/iio/accel/bmc150-accel-core.c         |   1 +
 drivers/iio/adc/at91-sama5d2_adc.c            |   4 +-
 .../buffer/industrialio-triggered-buffer.c    |   8 +-
 drivers/iio/buffer/kfifo_buf.c                |  50 ++++++
 .../cros_ec_sensors/cros_ec_sensors_core.c    |   1 +
 .../common/hid-sensors/hid-sensor-trigger.c   |   5 +-
 drivers/iio/dac/ad5686-spi.c                  |   2 +-
 drivers/iio/dac/ad5686.c                      | 146 +++++++++++++++++-
 drivers/iio/dac/ad5686.h                      |   7 +-
 drivers/iio/dac/ad5696-i2c.c                  |   2 +-
 drivers/iio/industrialio-buffer.c             | 128 ++++++++++++++-
 include/linux/iio/buffer.h                    |   7 +
 include/linux/iio/buffer_impl.h               |  11 ++
 include/linux/iio/triggered_buffer.h          |  11 +-
 16 files changed, 459 insertions(+), 17 deletions(-)

-- 
2.17.1

[PATCH v2 1/5] iio: Documentation: update definitions for bufferY and scan_elements

[Alexandru Ardelean]

Since the new change to the IIO buffer infrastructure, the buffer/ and
scan_elements/ directories have been merged into bufferY/ to have some
attributes available per-buffer.

This change updates the ABI docs to reflect this change.

The hwfifo attributes are not updated, as for now these should be used
via the legacy buffer/ directory until they are moved into core.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 Documentation/ABI/testing/sysfs-bus-iio | 85 +++++++++++++++++++++++++
 1 file changed, 85 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
index d957f5da5c04..f2a72d7fbacb 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -1118,12 +1118,16 @@ Description:
 
 What:		/sys/bus/iio/devices/iio:deviceX/buffer/length
 KernelVersion:	2.6.35
+What:		/sys/bus/iio/devices/iio:deviceX/bufferY/length
+KernelVersion:	5.11
 Contact:	linux-iio@vger.kernel.org
 Description:
 		Number of scans contained by the buffer.
 
 What:		/sys/bus/iio/devices/iio:deviceX/buffer/enable
 KernelVersion:	2.6.35
+What:		/sys/bus/iio/devices/iio:deviceX/bufferY/enable
+KernelVersion:	5.11
 Contact:	linux-iio@vger.kernel.org
 Description:
 		Actually start the buffer capture up.  Will start trigger
@@ -1131,11 +1135,16 @@ Description:
 
 What:		/sys/bus/iio/devices/iio:deviceX/scan_elements
 KernelVersion:	2.6.37
+What:		/sys/bus/iio/devices/iio:deviceX/buffeY
+KernelVersion:	5.11
 Contact:	linux-iio@vger.kernel.org
 Description:
 		Directory containing interfaces for elements that will be
 		captured for a single triggered sample set in the buffer.
 
+		Since kernel 5.11 the scan_elements attributes are merged into
+		the bufferY directory, to be configurable per buffer.
+
 What:		/sys/.../iio:deviceX/scan_elements/in_accel_x_en
 What:		/sys/.../iio:deviceX/scan_elements/in_accel_y_en
 What:		/sys/.../iio:deviceX/scan_elements/in_accel_z_en
@@ -1164,6 +1173,34 @@ What:		/sys/.../iio:deviceX/scan_elements/in_pressure_en
 What:		/sys/.../iio:deviceX/scan_elements/in_rot_quaternion_en
 What:		/sys/.../iio:deviceX/scan_elements/in_proximity_en
 KernelVersion:	2.6.37
+What:		/sys/.../iio:deviceX/bufferY/in_accel_x_en
+What:		/sys/.../iio:deviceX/bufferY/in_accel_y_en
+What:		/sys/.../iio:deviceX/bufferY/in_accel_z_en
+What:		/sys/.../iio:deviceX/bufferY/in_anglvel_x_en
+What:		/sys/.../iio:deviceX/bufferY/in_anglvel_y_en
+What:		/sys/.../iio:deviceX/bufferY/in_anglvel_z_en
+What:		/sys/.../iio:deviceX/bufferY/in_magn_x_en
+What:		/sys/.../iio:deviceX/bufferY/in_magn_y_en
+What:		/sys/.../iio:deviceX/bufferY/in_magn_z_en
+What:		/sys/.../iio:deviceX/bufferY/in_rot_from_north_magnetic_en
+What:		/sys/.../iio:deviceX/bufferY/in_rot_from_north_true_en
+What:		/sys/.../iio:deviceX/bufferY/in_rot_from_north_magnetic_tilt_comp_en
+What:		/sys/.../iio:deviceX/bufferY/in_rot_from_north_true_tilt_comp_en
+What:		/sys/.../iio:deviceX/bufferY/in_timestamp_en
+What:		/sys/.../iio:deviceX/bufferY/in_voltageY_supply_en
+What:		/sys/.../iio:deviceX/bufferY/in_voltageY_en
+What:		/sys/.../iio:deviceX/bufferY/in_voltageY-voltageZ_en
+What:		/sys/.../iio:deviceX/bufferY/in_voltageY_i_en
+What:		/sys/.../iio:deviceX/bufferY/in_voltageY_q_en
+What:		/sys/.../iio:deviceX/bufferY/in_voltage_i_en
+What:		/sys/.../iio:deviceX/bufferY/in_voltage_q_en
+What:		/sys/.../iio:deviceX/bufferY/in_incli_x_en
+What:		/sys/.../iio:deviceX/bufferY/in_incli_y_en
+What:		/sys/.../iio:deviceX/bufferY/in_pressureY_en
+What:		/sys/.../iio:deviceX/bufferY/in_pressure_en
+What:		/sys/.../iio:deviceX/bufferY/in_rot_quaternion_en
+What:		/sys/.../iio:deviceX/bufferY/in_proximity_en
+KernelVersion:	5.11
 Contact:	linux-iio@vger.kernel.org
 Description:
 		Scan element control for triggered data capture.
@@ -1185,6 +1222,23 @@ What:		/sys/.../iio:deviceX/scan_elements/in_pressure_type
 What:		/sys/.../iio:deviceX/scan_elements/in_rot_quaternion_type
 What:		/sys/.../iio:deviceX/scan_elements/in_proximity_type
 KernelVersion:	2.6.37
+What:		/sys/.../iio:deviceX/bufferY/in_accel_type
+What:		/sys/.../iio:deviceX/bufferY/in_anglvel_type
+What:		/sys/.../iio:deviceX/bufferY/in_magn_type
+What:		/sys/.../iio:deviceX/bufferY/in_incli_type
+What:		/sys/.../iio:deviceX/bufferY/in_voltageY_type
+What:		/sys/.../iio:deviceX/bufferY/in_voltage_type
+What:		/sys/.../iio:deviceX/bufferY/in_voltageY_supply_type
+What:		/sys/.../iio:deviceX/bufferY/in_voltageY_i_type
+What:		/sys/.../iio:deviceX/bufferY/in_voltageY_q_type
+What:		/sys/.../iio:deviceX/bufferY/in_voltage_i_type
+What:		/sys/.../iio:deviceX/bufferY/in_voltage_q_type
+What:		/sys/.../iio:deviceX/bufferY/in_timestamp_type
+What:		/sys/.../iio:deviceX/bufferY/in_pressureY_type
+What:		/sys/.../iio:deviceX/bufferY/in_pressure_type
+What:		/sys/.../iio:deviceX/bufferY/in_rot_quaternion_type
+What:		/sys/.../iio:deviceX/bufferY/in_proximity_type
+KernelVersion:	5.11
 Contact:	linux-iio@vger.kernel.org
 Description:
 		Description of the scan element data storage within the buffer
@@ -1241,6 +1295,33 @@ What:		/sys/.../iio:deviceX/scan_elements/in_pressure_index
 What:		/sys/.../iio:deviceX/scan_elements/in_rot_quaternion_index
 What:		/sys/.../iio:deviceX/scan_elements/in_proximity_index
 KernelVersion:	2.6.37
+What:		/sys/.../iio:deviceX/bufferY/in_voltageY_index
+What:		/sys/.../iio:deviceX/bufferY/in_voltageY_supply_index
+What:		/sys/.../iio:deviceX/bufferY/in_voltageY_i_index
+What:		/sys/.../iio:deviceX/bufferY/in_voltageY_q_index
+What:		/sys/.../iio:deviceX/bufferY/in_voltage_i_index
+What:		/sys/.../iio:deviceX/bufferY/in_voltage_q_index
+What:		/sys/.../iio:deviceX/bufferY/in_accel_x_index
+What:		/sys/.../iio:deviceX/bufferY/in_accel_y_index
+What:		/sys/.../iio:deviceX/bufferY/in_accel_z_index
+What:		/sys/.../iio:deviceX/bufferY/in_anglvel_x_index
+What:		/sys/.../iio:deviceX/bufferY/in_anglvel_y_index
+What:		/sys/.../iio:deviceX/bufferY/in_anglvel_z_index
+What:		/sys/.../iio:deviceX/bufferY/in_magn_x_index
+What:		/sys/.../iio:deviceX/bufferY/in_magn_y_index
+What:		/sys/.../iio:deviceX/bufferY/in_magn_z_index
+What:		/sys/.../iio:deviceX/bufferY/in_rot_from_north_magnetic_index
+What:		/sys/.../iio:deviceX/bufferY/in_rot_from_north_true_index
+What:		/sys/.../iio:deviceX/bufferY/in_rot_from_north_magnetic_tilt_comp_index
+What:		/sys/.../iio:deviceX/bufferY/in_rot_from_north_true_tilt_comp_index
+What:		/sys/.../iio:deviceX/bufferY/in_incli_x_index
+What:		/sys/.../iio:deviceX/bufferY/in_incli_y_index
+What:		/sys/.../iio:deviceX/bufferY/in_timestamp_index
+What:		/sys/.../iio:deviceX/bufferY/in_pressureY_index
+What:		/sys/.../iio:deviceX/bufferY/in_pressure_index
+What:		/sys/.../iio:deviceX/bufferY/in_rot_quaternion_index
+What:		/sys/.../iio:deviceX/bufferY/in_proximity_index
+KernelVersion:	5.11
 Contact:	linux-iio@vger.kernel.org
 Description:
 		A single positive integer specifying the position of this
@@ -1455,6 +1536,8 @@ Description:
 
 What:		/sys/bus/iio/devices/iio:deviceX/buffer/watermark
 KernelVersion:	4.2
+What:		/sys/bus/iio/devices/iio:deviceX/bufferY/watermark
+KernelVersion:	5.11
 Contact:	linux-iio@vger.kernel.org
 Description:
 		A single positive integer specifying the maximum number of scan
@@ -1473,6 +1556,8 @@ Description:
 
 What:		/sys/bus/iio/devices/iio:deviceX/buffer/data_available
 KernelVersion: 4.16
+What:		/sys/bus/iio/devices/iio:deviceX/bufferY/data_available
+KernelVersion:	5.11
 Contact:	linux-iio@vger.kernel.org
 Description:
 		A read-only value indicating the bytes of data available in the
-- 
2.17.1

Re: [PATCH v2 1/5] iio: Documentation: update definitions for bufferY and scan_elements

[Jonathan Cameron]

On Wed, 17 Feb 2021 10:34:34 +0200
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> Since the new change to the IIO buffer infrastructure, the buffer/ and
> scan_elements/ directories have been merged into bufferY/ to have some
> attributes available per-buffer.
> 
> This change updates the ABI docs to reflect this change.
> 
> The hwfifo attributes are not updated, as for now these should be used
> via the legacy buffer/ directory until they are moved into core.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

This doesn't really have anything to do with the rest of the set - so ideally
you'd have sent it separately.

One trivial typo below, that I can fix whilst applying.

> ---
>  Documentation/ABI/testing/sysfs-bus-iio | 85 +++++++++++++++++++++++++
>  1 file changed, 85 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> index d957f5da5c04..f2a72d7fbacb 100644
> --- a/Documentation/ABI/testing/sysfs-bus-iio
> +++ b/Documentation/ABI/testing/sysfs-bus-iio
> @@ -1118,12 +1118,16 @@ Description:
>  
>  What:		/sys/bus/iio/devices/iio:deviceX/buffer/length
>  KernelVersion:	2.6.35
> +What:		/sys/bus/iio/devices/iio:deviceX/bufferY/length
> +KernelVersion:	5.11
>  Contact:	linux-iio@vger.kernel.org
>  Description:
>  		Number of scans contained by the buffer.
>  
>  What:		/sys/bus/iio/devices/iio:deviceX/buffer/enable
>  KernelVersion:	2.6.35
> +What:		/sys/bus/iio/devices/iio:deviceX/bufferY/enable
> +KernelVersion:	5.11
>  Contact:	linux-iio@vger.kernel.org
>  Description:
>  		Actually start the buffer capture up.  Will start trigger
> @@ -1131,11 +1135,16 @@ Description:
>  
>  What:		/sys/bus/iio/devices/iio:deviceX/scan_elements
>  KernelVersion:	2.6.37
> +What:		/sys/bus/iio/devices/iio:deviceX/buffeY
bufferY

I'll fix whilst applying.

> +KernelVersion:	5.11
>  Contact:	linux-iio@vger.kernel.org
>  Description:
>  		Directory containing interfaces for elements that will be
>  		captured for a single triggered sample set in the buffer.
>  
> +		Since kernel 5.11 the scan_elements attributes are merged into
> +		the bufferY directory, to be configurable per buffer.
> +
>  What:		/sys/.../iio:deviceX/scan_elements/in_accel_x_en
>  What:		/sys/.../iio:deviceX/scan_elements/in_accel_y_en
>  What:		/sys/.../iio:deviceX/scan_elements/in_accel_z_en
> @@ -1164,6 +1173,34 @@ What:		/sys/.../iio:deviceX/scan_elements/in_pressure_en
>  What:		/sys/.../iio:deviceX/scan_elements/in_rot_quaternion_en
>  What:		/sys/.../iio:deviceX/scan_elements/in_proximity_en
>  KernelVersion:	2.6.37
> +What:		/sys/.../iio:deviceX/bufferY/in_accel_x_en
> +What:		/sys/.../iio:deviceX/bufferY/in_accel_y_en
> +What:		/sys/.../iio:deviceX/bufferY/in_accel_z_en
> +What:		/sys/.../iio:deviceX/bufferY/in_anglvel_x_en
> +What:		/sys/.../iio:deviceX/bufferY/in_anglvel_y_en
> +What:		/sys/.../iio:deviceX/bufferY/in_anglvel_z_en
> +What:		/sys/.../iio:deviceX/bufferY/in_magn_x_en
> +What:		/sys/.../iio:deviceX/bufferY/in_magn_y_en
> +What:		/sys/.../iio:deviceX/bufferY/in_magn_z_en
> +What:		/sys/.../iio:deviceX/bufferY/in_rot_from_north_magnetic_en
> +What:		/sys/.../iio:deviceX/bufferY/in_rot_from_north_true_en
> +What:		/sys/.../iio:deviceX/bufferY/in_rot_from_north_magnetic_tilt_comp_en
> +What:		/sys/.../iio:deviceX/bufferY/in_rot_from_north_true_tilt_comp_en
> +What:		/sys/.../iio:deviceX/bufferY/in_timestamp_en
> +What:		/sys/.../iio:deviceX/bufferY/in_voltageY_supply_en
> +What:		/sys/.../iio:deviceX/bufferY/in_voltageY_en
> +What:		/sys/.../iio:deviceX/bufferY/in_voltageY-voltageZ_en
> +What:		/sys/.../iio:deviceX/bufferY/in_voltageY_i_en
> +What:		/sys/.../iio:deviceX/bufferY/in_voltageY_q_en
> +What:		/sys/.../iio:deviceX/bufferY/in_voltage_i_en
> +What:		/sys/.../iio:deviceX/bufferY/in_voltage_q_en
> +What:		/sys/.../iio:deviceX/bufferY/in_incli_x_en
> +What:		/sys/.../iio:deviceX/bufferY/in_incli_y_en
> +What:		/sys/.../iio:deviceX/bufferY/in_pressureY_en
> +What:		/sys/.../iio:deviceX/bufferY/in_pressure_en
> +What:		/sys/.../iio:deviceX/bufferY/in_rot_quaternion_en
> +What:		/sys/.../iio:deviceX/bufferY/in_proximity_en
> +KernelVersion:	5.11
>  Contact:	linux-iio@vger.kernel.org
>  Description:
>  		Scan element control for triggered data capture.
> @@ -1185,6 +1222,23 @@ What:		/sys/.../iio:deviceX/scan_elements/in_pressure_type
>  What:		/sys/.../iio:deviceX/scan_elements/in_rot_quaternion_type
>  What:		/sys/.../iio:deviceX/scan_elements/in_proximity_type
>  KernelVersion:	2.6.37
> +What:		/sys/.../iio:deviceX/bufferY/in_accel_type
> +What:		/sys/.../iio:deviceX/bufferY/in_anglvel_type
> +What:		/sys/.../iio:deviceX/bufferY/in_magn_type
> +What:		/sys/.../iio:deviceX/bufferY/in_incli_type
> +What:		/sys/.../iio:deviceX/bufferY/in_voltageY_type
> +What:		/sys/.../iio:deviceX/bufferY/in_voltage_type
> +What:		/sys/.../iio:deviceX/bufferY/in_voltageY_supply_type
> +What:		/sys/.../iio:deviceX/bufferY/in_voltageY_i_type
> +What:		/sys/.../iio:deviceX/bufferY/in_voltageY_q_type
> +What:		/sys/.../iio:deviceX/bufferY/in_voltage_i_type
> +What:		/sys/.../iio:deviceX/bufferY/in_voltage_q_type
> +What:		/sys/.../iio:deviceX/bufferY/in_timestamp_type
> +What:		/sys/.../iio:deviceX/bufferY/in_pressureY_type
> +What:		/sys/.../iio:deviceX/bufferY/in_pressure_type
> +What:		/sys/.../iio:deviceX/bufferY/in_rot_quaternion_type
> +What:		/sys/.../iio:deviceX/bufferY/in_proximity_type
> +KernelVersion:	5.11
>  Contact:	linux-iio@vger.kernel.org
>  Description:
>  		Description of the scan element data storage within the buffer
> @@ -1241,6 +1295,33 @@ What:		/sys/.../iio:deviceX/scan_elements/in_pressure_index
>  What:		/sys/.../iio:deviceX/scan_elements/in_rot_quaternion_index
>  What:		/sys/.../iio:deviceX/scan_elements/in_proximity_index
>  KernelVersion:	2.6.37
> +What:		/sys/.../iio:deviceX/bufferY/in_voltageY_index
> +What:		/sys/.../iio:deviceX/bufferY/in_voltageY_supply_index
> +What:		/sys/.../iio:deviceX/bufferY/in_voltageY_i_index
> +What:		/sys/.../iio:deviceX/bufferY/in_voltageY_q_index
> +What:		/sys/.../iio:deviceX/bufferY/in_voltage_i_index
> +What:		/sys/.../iio:deviceX/bufferY/in_voltage_q_index
> +What:		/sys/.../iio:deviceX/bufferY/in_accel_x_index
> +What:		/sys/.../iio:deviceX/bufferY/in_accel_y_index
> +What:		/sys/.../iio:deviceX/bufferY/in_accel_z_index
> +What:		/sys/.../iio:deviceX/bufferY/in_anglvel_x_index
> +What:		/sys/.../iio:deviceX/bufferY/in_anglvel_y_index
> +What:		/sys/.../iio:deviceX/bufferY/in_anglvel_z_index
> +What:		/sys/.../iio:deviceX/bufferY/in_magn_x_index
> +What:		/sys/.../iio:deviceX/bufferY/in_magn_y_index
> +What:		/sys/.../iio:deviceX/bufferY/in_magn_z_index
> +What:		/sys/.../iio:deviceX/bufferY/in_rot_from_north_magnetic_index
> +What:		/sys/.../iio:deviceX/bufferY/in_rot_from_north_true_index
> +What:		/sys/.../iio:deviceX/bufferY/in_rot_from_north_magnetic_tilt_comp_index
> +What:		/sys/.../iio:deviceX/bufferY/in_rot_from_north_true_tilt_comp_index
> +What:		/sys/.../iio:deviceX/bufferY/in_incli_x_index
> +What:		/sys/.../iio:deviceX/bufferY/in_incli_y_index
> +What:		/sys/.../iio:deviceX/bufferY/in_timestamp_index
> +What:		/sys/.../iio:deviceX/bufferY/in_pressureY_index
> +What:		/sys/.../iio:deviceX/bufferY/in_pressure_index
> +What:		/sys/.../iio:deviceX/bufferY/in_rot_quaternion_index
> +What:		/sys/.../iio:deviceX/bufferY/in_proximity_index
> +KernelVersion:	5.11
>  Contact:	linux-iio@vger.kernel.org
>  Description:
>  		A single positive integer specifying the position of this
> @@ -1455,6 +1536,8 @@ Description:
>  
>  What:		/sys/bus/iio/devices/iio:deviceX/buffer/watermark
>  KernelVersion:	4.2
> +What:		/sys/bus/iio/devices/iio:deviceX/bufferY/watermark
> +KernelVersion:	5.11
>  Contact:	linux-iio@vger.kernel.org
>  Description:
>  		A single positive integer specifying the maximum number of scan
> @@ -1473,6 +1556,8 @@ Description:
>  
>  What:		/sys/bus/iio/devices/iio:deviceX/buffer/data_available
>  KernelVersion: 4.16
> +What:		/sys/bus/iio/devices/iio:deviceX/bufferY/data_available
> +KernelVersion:	5.11
>  Contact:	linux-iio@vger.kernel.org
>  Description:
>  		A read-only value indicating the bytes of data available in the

Re: [PATCH v2 1/5] iio: Documentation: update definitions for bufferY and scan_elements

[Jonathan Cameron]

On Thu, 18 Feb 2021 12:19:48 +0000
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Wed, 17 Feb 2021 10:34:34 +0200
> Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> 
> > Since the new change to the IIO buffer infrastructure, the buffer/ and
> > scan_elements/ directories have been merged into bufferY/ to have some
> > attributes available per-buffer.
> > 
> > This change updates the ABI docs to reflect this change.
> > 
> > The hwfifo attributes are not updated, as for now these should be used
> > via the legacy buffer/ directory until they are moved into core.
> > 
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>  
> 
> This doesn't really have anything to do with the rest of the set - so ideally
> you'd have sent it separately.
> 
> One trivial typo below, that I can fix whilst applying.
Applied with that fix.

Jonathan

> 
> > ---
> >  Documentation/ABI/testing/sysfs-bus-iio | 85 +++++++++++++++++++++++++
> >  1 file changed, 85 insertions(+)
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> > index d957f5da5c04..f2a72d7fbacb 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-iio
> > +++ b/Documentation/ABI/testing/sysfs-bus-iio
> > @@ -1118,12 +1118,16 @@ Description:
> >  
> >  What:		/sys/bus/iio/devices/iio:deviceX/buffer/length
> >  KernelVersion:	2.6.35
> > +What:		/sys/bus/iio/devices/iio:deviceX/bufferY/length
> > +KernelVersion:	5.11
> >  Contact:	linux-iio@vger.kernel.org
> >  Description:
> >  		Number of scans contained by the buffer.
> >  
> >  What:		/sys/bus/iio/devices/iio:deviceX/buffer/enable
> >  KernelVersion:	2.6.35
> > +What:		/sys/bus/iio/devices/iio:deviceX/bufferY/enable
> > +KernelVersion:	5.11
> >  Contact:	linux-iio@vger.kernel.org
> >  Description:
> >  		Actually start the buffer capture up.  Will start trigger
> > @@ -1131,11 +1135,16 @@ Description:
> >  
> >  What:		/sys/bus/iio/devices/iio:deviceX/scan_elements
> >  KernelVersion:	2.6.37
> > +What:		/sys/bus/iio/devices/iio:deviceX/buffeY  
> bufferY
> 
> I'll fix whilst applying.
> 
> > +KernelVersion:	5.11
> >  Contact:	linux-iio@vger.kernel.org
> >  Description:
> >  		Directory containing interfaces for elements that will be
> >  		captured for a single triggered sample set in the buffer.
> >  
> > +		Since kernel 5.11 the scan_elements attributes are merged into
> > +		the bufferY directory, to be configurable per buffer.
> > +
> >  What:		/sys/.../iio:deviceX/scan_elements/in_accel_x_en
> >  What:		/sys/.../iio:deviceX/scan_elements/in_accel_y_en
> >  What:		/sys/.../iio:deviceX/scan_elements/in_accel_z_en
> > @@ -1164,6 +1173,34 @@ What:		/sys/.../iio:deviceX/scan_elements/in_pressure_en
> >  What:		/sys/.../iio:deviceX/scan_elements/in_rot_quaternion_en
> >  What:		/sys/.../iio:deviceX/scan_elements/in_proximity_en
> >  KernelVersion:	2.6.37
> > +What:		/sys/.../iio:deviceX/bufferY/in_accel_x_en
> > +What:		/sys/.../iio:deviceX/bufferY/in_accel_y_en
> > +What:		/sys/.../iio:deviceX/bufferY/in_accel_z_en
> > +What:		/sys/.../iio:deviceX/bufferY/in_anglvel_x_en
> > +What:		/sys/.../iio:deviceX/bufferY/in_anglvel_y_en
> > +What:		/sys/.../iio:deviceX/bufferY/in_anglvel_z_en
> > +What:		/sys/.../iio:deviceX/bufferY/in_magn_x_en
> > +What:		/sys/.../iio:deviceX/bufferY/in_magn_y_en
> > +What:		/sys/.../iio:deviceX/bufferY/in_magn_z_en
> > +What:		/sys/.../iio:deviceX/bufferY/in_rot_from_north_magnetic_en
> > +What:		/sys/.../iio:deviceX/bufferY/in_rot_from_north_true_en
> > +What:		/sys/.../iio:deviceX/bufferY/in_rot_from_north_magnetic_tilt_comp_en
> > +What:		/sys/.../iio:deviceX/bufferY/in_rot_from_north_true_tilt_comp_en
> > +What:		/sys/.../iio:deviceX/bufferY/in_timestamp_en
> > +What:		/sys/.../iio:deviceX/bufferY/in_voltageY_supply_en
> > +What:		/sys/.../iio:deviceX/bufferY/in_voltageY_en
> > +What:		/sys/.../iio:deviceX/bufferY/in_voltageY-voltageZ_en
> > +What:		/sys/.../iio:deviceX/bufferY/in_voltageY_i_en
> > +What:		/sys/.../iio:deviceX/bufferY/in_voltageY_q_en
> > +What:		/sys/.../iio:deviceX/bufferY/in_voltage_i_en
> > +What:		/sys/.../iio:deviceX/bufferY/in_voltage_q_en
> > +What:		/sys/.../iio:deviceX/bufferY/in_incli_x_en
> > +What:		/sys/.../iio:deviceX/bufferY/in_incli_y_en
> > +What:		/sys/.../iio:deviceX/bufferY/in_pressureY_en
> > +What:		/sys/.../iio:deviceX/bufferY/in_pressure_en
> > +What:		/sys/.../iio:deviceX/bufferY/in_rot_quaternion_en
> > +What:		/sys/.../iio:deviceX/bufferY/in_proximity_en
> > +KernelVersion:	5.11
> >  Contact:	linux-iio@vger.kernel.org
> >  Description:
> >  		Scan element control for triggered data capture.
> > @@ -1185,6 +1222,23 @@ What:		/sys/.../iio:deviceX/scan_elements/in_pressure_type
> >  What:		/sys/.../iio:deviceX/scan_elements/in_rot_quaternion_type
> >  What:		/sys/.../iio:deviceX/scan_elements/in_proximity_type
> >  KernelVersion:	2.6.37
> > +What:		/sys/.../iio:deviceX/bufferY/in_accel_type
> > +What:		/sys/.../iio:deviceX/bufferY/in_anglvel_type
> > +What:		/sys/.../iio:deviceX/bufferY/in_magn_type
> > +What:		/sys/.../iio:deviceX/bufferY/in_incli_type
> > +What:		/sys/.../iio:deviceX/bufferY/in_voltageY_type
> > +What:		/sys/.../iio:deviceX/bufferY/in_voltage_type
> > +What:		/sys/.../iio:deviceX/bufferY/in_voltageY_supply_type
> > +What:		/sys/.../iio:deviceX/bufferY/in_voltageY_i_type
> > +What:		/sys/.../iio:deviceX/bufferY/in_voltageY_q_type
> > +What:		/sys/.../iio:deviceX/bufferY/in_voltage_i_type
> > +What:		/sys/.../iio:deviceX/bufferY/in_voltage_q_type
> > +What:		/sys/.../iio:deviceX/bufferY/in_timestamp_type
> > +What:		/sys/.../iio:deviceX/bufferY/in_pressureY_type
> > +What:		/sys/.../iio:deviceX/bufferY/in_pressure_type
> > +What:		/sys/.../iio:deviceX/bufferY/in_rot_quaternion_type
> > +What:		/sys/.../iio:deviceX/bufferY/in_proximity_type
> > +KernelVersion:	5.11
> >  Contact:	linux-iio@vger.kernel.org
> >  Description:
> >  		Description of the scan element data storage within the buffer
> > @@ -1241,6 +1295,33 @@ What:		/sys/.../iio:deviceX/scan_elements/in_pressure_index
> >  What:		/sys/.../iio:deviceX/scan_elements/in_rot_quaternion_index
> >  What:		/sys/.../iio:deviceX/scan_elements/in_proximity_index
> >  KernelVersion:	2.6.37
> > +What:		/sys/.../iio:deviceX/bufferY/in_voltageY_index
> > +What:		/sys/.../iio:deviceX/bufferY/in_voltageY_supply_index
> > +What:		/sys/.../iio:deviceX/bufferY/in_voltageY_i_index
> > +What:		/sys/.../iio:deviceX/bufferY/in_voltageY_q_index
> > +What:		/sys/.../iio:deviceX/bufferY/in_voltage_i_index
> > +What:		/sys/.../iio:deviceX/bufferY/in_voltage_q_index
> > +What:		/sys/.../iio:deviceX/bufferY/in_accel_x_index
> > +What:		/sys/.../iio:deviceX/bufferY/in_accel_y_index
> > +What:		/sys/.../iio:deviceX/bufferY/in_accel_z_index
> > +What:		/sys/.../iio:deviceX/bufferY/in_anglvel_x_index
> > +What:		/sys/.../iio:deviceX/bufferY/in_anglvel_y_index
> > +What:		/sys/.../iio:deviceX/bufferY/in_anglvel_z_index
> > +What:		/sys/.../iio:deviceX/bufferY/in_magn_x_index
> > +What:		/sys/.../iio:deviceX/bufferY/in_magn_y_index
> > +What:		/sys/.../iio:deviceX/bufferY/in_magn_z_index
> > +What:		/sys/.../iio:deviceX/bufferY/in_rot_from_north_magnetic_index
> > +What:		/sys/.../iio:deviceX/bufferY/in_rot_from_north_true_index
> > +What:		/sys/.../iio:deviceX/bufferY/in_rot_from_north_magnetic_tilt_comp_index
> > +What:		/sys/.../iio:deviceX/bufferY/in_rot_from_north_true_tilt_comp_index
> > +What:		/sys/.../iio:deviceX/bufferY/in_incli_x_index
> > +What:		/sys/.../iio:deviceX/bufferY/in_incli_y_index
> > +What:		/sys/.../iio:deviceX/bufferY/in_timestamp_index
> > +What:		/sys/.../iio:deviceX/bufferY/in_pressureY_index
> > +What:		/sys/.../iio:deviceX/bufferY/in_pressure_index
> > +What:		/sys/.../iio:deviceX/bufferY/in_rot_quaternion_index
> > +What:		/sys/.../iio:deviceX/bufferY/in_proximity_index
> > +KernelVersion:	5.11
> >  Contact:	linux-iio@vger.kernel.org
> >  Description:
> >  		A single positive integer specifying the position of this
> > @@ -1455,6 +1536,8 @@ Description:
> >  
> >  What:		/sys/bus/iio/devices/iio:deviceX/buffer/watermark
> >  KernelVersion:	4.2
> > +What:		/sys/bus/iio/devices/iio:deviceX/bufferY/watermark
> > +KernelVersion:	5.11
> >  Contact:	linux-iio@vger.kernel.org
> >  Description:
> >  		A single positive integer specifying the maximum number of scan
> > @@ -1473,6 +1556,8 @@ Description:
> >  
> >  What:		/sys/bus/iio/devices/iio:deviceX/buffer/data_available
> >  KernelVersion: 4.16
> > +What:		/sys/bus/iio/devices/iio:deviceX/bufferY/data_available
> > +KernelVersion:	5.11
> >  Contact:	linux-iio@vger.kernel.org
> >  Description:
> >  		A read-only value indicating the bytes of data available in the  
> 

Re: [PATCH v2 1/5] iio: Documentation: update definitions for bufferY and scan_elements

[Mauro Carvalho Chehab]

Em Wed, 17 Feb 2021 10:34:34 +0200
Alexandru Ardelean <alexandru.ardelean@analog.com> escreveu:

> Since the new change to the IIO buffer infrastructure, the buffer/ and
> scan_elements/ directories have been merged into bufferY/ to have some
> attributes available per-buffer.
> 
> This change updates the ABI docs to reflect this change.
> 
> The hwfifo attributes are not updated, as for now these should be used
> via the legacy buffer/ directory until they are moved into core.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
>  Documentation/ABI/testing/sysfs-bus-iio | 85 +++++++++++++++++++++++++
>  1 file changed, 85 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> index d957f5da5c04..f2a72d7fbacb 100644
> --- a/Documentation/ABI/testing/sysfs-bus-iio
> +++ b/Documentation/ABI/testing/sysfs-bus-iio
> @@ -1118,12 +1118,16 @@ Description:
>  
>  What:		/sys/bus/iio/devices/iio:deviceX/buffer/length
>  KernelVersion:	2.6.35
> +What:		/sys/bus/iio/devices/iio:deviceX/bufferY/length
> +KernelVersion:	5.11
>  Contact:	linux-iio@vger.kernel.org
>  Description:
>  		Number of scans contained by the buffer.

The ABI parser doesn't like things like this:

	$ ./scripts/get_abi.pl validate

	Warning: file Documentation/ABI/testing/sysfs-bus-iio#1167:
		What '/sys/bus/iio/devices/iio:deviceX/buffer/length' doesn't have a description

The main reason is that all properties, including KernelVersion, 
Contact and Description are associated to a group of properties.

To be frank, for me that don't work with IIO, the above ABI
description doesn't sound clear.

I mean, what's the difference between
	/sys/bus/iio/devices/iio:deviceX/buffer/length
and
	/sys/bus/iio/devices/iio:deviceX/bufferY/length?


If the intention is to tell that:
	/sys/bus/iio/devices/iio:deviceX/buffer/length
was obsoleted by:
	/sys/bus/iio/devices/iio:deviceX/bufferY/length

IMO, the right thing would be to move the deprecated definition to
	Documentation/ABI/obsolete/

If, on the other hand, both are completely identical and 
non-obsoleted, why to have both APIs? 

Or did you just missed adding a different description for the
new ABI symbols, but this was dropped due to some merge
conflict?

Thanks,
Mauro

Re: [PATCH v2 1/5] iio: Documentation: update definitions for bufferY and scan_elements

[Jonathan Cameron]

On Thu, 1 Apr 2021 15:44:08 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:

> Em Wed, 17 Feb 2021 10:34:34 +0200
> Alexandru Ardelean <alexandru.ardelean@analog.com> escreveu:
> 
> > Since the new change to the IIO buffer infrastructure, the buffer/ and
> > scan_elements/ directories have been merged into bufferY/ to have some
> > attributes available per-buffer.
> > 
> > This change updates the ABI docs to reflect this change.
> > 
> > The hwfifo attributes are not updated, as for now these should be used
> > via the legacy buffer/ directory until they are moved into core.
> > 
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > ---
> >  Documentation/ABI/testing/sysfs-bus-iio | 85 +++++++++++++++++++++++++
> >  1 file changed, 85 insertions(+)
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> > index d957f5da5c04..f2a72d7fbacb 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-iio
> > +++ b/Documentation/ABI/testing/sysfs-bus-iio
> > @@ -1118,12 +1118,16 @@ Description:
> >  
> >  What:		/sys/bus/iio/devices/iio:deviceX/buffer/length
> >  KernelVersion:	2.6.35
> > +What:		/sys/bus/iio/devices/iio:deviceX/bufferY/length
> > +KernelVersion:	5.11
> >  Contact:	linux-iio@vger.kernel.org
> >  Description:
> >  		Number of scans contained by the buffer.  
> 
> The ABI parser doesn't like things like this:
> 
> 	$ ./scripts/get_abi.pl validate
> 
> 	Warning: file Documentation/ABI/testing/sysfs-bus-iio#1167:
> 		What '/sys/bus/iio/devices/iio:deviceX/buffer/length' doesn't have a description
> 
> The main reason is that all properties, including KernelVersion, 
> Contact and Description are associated to a group of properties.
> 
> To be frank, for me that don't work with IIO, the above ABI
> description doesn't sound clear.
> 
> I mean, what's the difference between
> 	/sys/bus/iio/devices/iio:deviceX/buffer/length
> and
> 	/sys/bus/iio/devices/iio:deviceX/bufferY/length?
> 
> 
> If the intention is to tell that:
> 	/sys/bus/iio/devices/iio:deviceX/buffer/length
> was obsoleted by:
> 	/sys/bus/iio/devices/iio:deviceX/bufferY/length
> 
> IMO, the right thing would be to move the deprecated definition to
> 	Documentation/ABI/obsolete/
> 
> If, on the other hand, both are completely identical and 
> non-obsoleted, why to have both APIs? 
> 
> Or did you just missed adding a different description for the
> new ABI symbols, but this was dropped due to some merge
> conflict?

You are right that the version with out the index is obsolete
but it's only just become so and there are no realistic way we
are going to ever be able to remove it however much we encourage
userspace to use the new interface.  We are stuck with this bit
of backwards compatibility indefinitely.

I'm fine with moving it and related definitions over to
ABI/obsolete if that makes sense.

Jonathan



> 
> Thanks,
> Mauro

[PATCH v2 2/5] iio: Add output buffer support

[Alexandru Ardelean]

From: Lars-Peter Clausen <lars@metafoo.de>

Currently IIO only supports buffer mode for capture devices like ADCs. Add
support for buffered mode for output devices like DACs.

The output buffer implementation is analogous to the input buffer
implementation. Instead of using read() to get data from the buffer write()
is used to copy data into the buffer.

poll() with POLLOUT will wakeup if there is space available for more or
equal to the configured watermark of samples.

Drivers can remove data from a buffer using iio_buffer_remove_sample(), the
function can e.g. called from a trigger handler to write the data to
hardware.

A buffer can only be either a output buffer or an input, but not both. So,
for a device that has an ADC and DAC path, this will mean 2 IIO buffers
(one for each direction).

The direction of the buffer is decided by the new direction field of the
iio_buffer struct and should be set after allocating and before registering
it.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 Documentation/ABI/testing/sysfs-bus-iio |   7 ++
 drivers/iio/industrialio-buffer.c       | 128 +++++++++++++++++++++++-
 include/linux/iio/buffer.h              |   7 ++
 include/linux/iio/buffer_impl.h         |  11 ++
 4 files changed, 149 insertions(+), 4 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
index f2a72d7fbacb..a42b915bf585 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -1124,6 +1124,13 @@ Contact:	linux-iio@vger.kernel.org
 Description:
 		Number of scans contained by the buffer.
 
+What:		/sys/bus/iio/devices/iio:deviceX/bufferY/direction
+KernelVersion:	5.11
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Returns the direction of the data stream of the buffer.
+		The output is "in" or "out".
+
 What:		/sys/bus/iio/devices/iio:deviceX/buffer/enable
 KernelVersion:	2.6.35
 What:		/sys/bus/iio/devices/iio:deviceX/bufferY/enable
diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 5d641f8adfbd..b9970c68005d 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -162,6 +162,69 @@ static ssize_t iio_buffer_read(struct file *filp, char __user *buf,
 	return ret;
 }
 
+static size_t iio_buffer_space_available(struct iio_buffer *buf)
+{
+	if (buf->access->space_available)
+		return buf->access->space_available(buf);
+
+	return SIZE_MAX;
+}
+
+static ssize_t iio_buffer_write(struct file *filp, const char __user *buf,
+				size_t n, loff_t *f_ps)
+{
+	struct iio_dev_buffer_pair *ib = filp->private_data;
+	struct iio_buffer *rb = ib->buffer;
+	struct iio_dev *indio_dev = ib->indio_dev;
+	DEFINE_WAIT_FUNC(wait, woken_wake_function);
+	size_t datum_size;
+	size_t to_wait;
+	int ret;
+
+	if (!rb || !rb->access->write)
+		return -EINVAL;
+
+	datum_size = rb->bytes_per_datum;
+
+	/*
+	 * If datum_size is 0 there will never be anything to read from the
+	 * buffer, so signal end of file now.
+	 */
+	if (!datum_size)
+		return 0;
+
+	if (filp->f_flags & O_NONBLOCK)
+		to_wait = 0;
+	else
+		to_wait = min_t(size_t, n / datum_size, rb->watermark);
+
+	add_wait_queue(&rb->pollq, &wait);
+	do {
+		if (!indio_dev->info) {
+			ret = -ENODEV;
+			break;
+		}
+
+		if (iio_buffer_space_available(rb) < to_wait) {
+			if (signal_pending(current)) {
+				ret = -ERESTARTSYS;
+				break;
+			}
+
+			wait_woken(&wait, TASK_INTERRUPTIBLE,
+				   MAX_SCHEDULE_TIMEOUT);
+			continue;
+		}
+
+		ret = rb->access->write(rb, n, buf);
+		if (ret == 0 && (filp->f_flags & O_NONBLOCK))
+			ret = -EAGAIN;
+	} while (ret == 0);
+	remove_wait_queue(&rb->pollq, &wait);
+
+	return ret;
+}
+
 /**
  * iio_buffer_poll() - poll the buffer to find out if it has data
  * @filp:	File structure pointer for device access
@@ -182,8 +245,19 @@ static __poll_t iio_buffer_poll(struct file *filp,
 		return 0;
 
 	poll_wait(filp, &rb->pollq, wait);
-	if (iio_buffer_ready(indio_dev, rb, rb->watermark, 0))
-		return EPOLLIN | EPOLLRDNORM;
+
+	switch (rb->direction) {
+	case IIO_BUFFER_DIRECTION_IN:
+		if (iio_buffer_ready(indio_dev, rb, rb->watermark, 0))
+			return EPOLLIN | EPOLLRDNORM;
+		break;
+	case IIO_BUFFER_DIRECTION_OUT:
+		if (iio_buffer_space_available(rb) >= rb->watermark)
+			return EPOLLOUT | EPOLLWRNORM;
+		break;
+	}
+
+	/* need a way of knowing if there may be enough data... */
 	return 0;
 }
 
@@ -232,6 +306,16 @@ void iio_buffer_wakeup_poll(struct iio_dev *indio_dev)
 	}
 }
 
+int iio_buffer_remove_sample(struct iio_buffer *buffer, u8 *data)
+{
+	if (!buffer || !buffer->access)
+		return -EINVAL;
+	if (!buffer->access->write)
+		return -ENOSYS;
+	return buffer->access->remove_from(buffer, data);
+}
+EXPORT_SYMBOL_GPL(iio_buffer_remove_sample);
+
 void iio_buffer_init(struct iio_buffer *buffer)
 {
 	INIT_LIST_HEAD(&buffer->demux_list);
@@ -803,6 +887,8 @@ static int iio_verify_update(struct iio_dev *indio_dev,
 	}
 
 	if (insert_buffer) {
+		if (insert_buffer->direction == IIO_BUFFER_DIRECTION_OUT)
+			strict_scanmask = true;
 		bitmap_or(compound_mask, compound_mask,
 			  insert_buffer->scan_mask, indio_dev->masklength);
 		scan_timestamp |= insert_buffer->scan_timestamp;
@@ -945,6 +1031,8 @@ static int iio_update_demux(struct iio_dev *indio_dev)
 	int ret;
 
 	list_for_each_entry(buffer, &iio_dev_opaque->buffer_list, buffer_list) {
+		if (buffer->direction == IIO_BUFFER_DIRECTION_OUT)
+			continue;
 		ret = iio_buffer_update_demux(indio_dev, buffer);
 		if (ret < 0)
 			goto error_clear_mux_table;
@@ -1155,6 +1243,11 @@ int iio_update_buffers(struct iio_dev *indio_dev,
 	mutex_lock(&indio_dev->info_exist_lock);
 	mutex_lock(&indio_dev->mlock);
 
+	if (insert_buffer->direction == IIO_BUFFER_DIRECTION_OUT) {
+		ret = -EINVAL;
+		goto out_unlock;
+	}
+
 	if (insert_buffer && iio_buffer_is_active(insert_buffer))
 		insert_buffer = NULL;
 
@@ -1273,6 +1366,22 @@ static ssize_t iio_dma_show_data_available(struct device *dev,
 	return sprintf(buf, "%zu\n", iio_buffer_data_available(buffer));
 }
 
+static ssize_t direction_show(struct device *dev,
+			      struct device_attribute *attr,
+			      char *buf)
+{
+	struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer;
+
+	switch (buffer->direction) {
+	case IIO_BUFFER_DIRECTION_IN:
+		return sprintf(buf, "in\n");
+	case IIO_BUFFER_DIRECTION_OUT:
+		return sprintf(buf, "out\n");
+	default:
+		return -EINVAL;
+	}
+}
+
 static DEVICE_ATTR(length, S_IRUGO | S_IWUSR, iio_buffer_read_length,
 		   iio_buffer_write_length);
 static struct device_attribute dev_attr_length_ro = __ATTR(length,
@@ -1285,9 +1394,11 @@ static struct device_attribute dev_attr_watermark_ro = __ATTR(watermark,
 	S_IRUGO, iio_buffer_show_watermark, NULL);
 static DEVICE_ATTR(data_available, S_IRUGO,
 		iio_dma_show_data_available, NULL);
+static DEVICE_ATTR_RO(direction);
 
 static struct attribute *iio_buffer_attrs[] = {
 	&dev_attr_length.attr,
+	&dev_attr_direction.attr,
 	&dev_attr_enable.attr,
 	&dev_attr_watermark.attr,
 	&dev_attr_data_available.attr,
@@ -1401,6 +1512,7 @@ static const struct file_operations iio_buffer_chrdev_fileops = {
 	.owner = THIS_MODULE,
 	.llseek = noop_llseek,
 	.read = iio_buffer_read,
+	.write = iio_buffer_write,
 	.poll = iio_buffer_poll,
 	.unlocked_ioctl = iio_buffer_ioctl,
 	.compat_ioctl = compat_ptr_ioctl,
@@ -1920,8 +2032,16 @@ static int iio_buffer_mmap(struct file *filep, struct vm_area_struct *vma)
 	if (!(vma->vm_flags & VM_SHARED))
 		return -EINVAL;
 
-	if (!(vma->vm_flags & VM_READ))
-		return -EINVAL;
+	switch (buffer->direction) {
+	case IIO_BUFFER_DIRECTION_IN:
+		if (!(vma->vm_flags & VM_READ))
+			return -EINVAL;
+		break;
+	case IIO_BUFFER_DIRECTION_OUT:
+		if (!(vma->vm_flags & VM_WRITE))
+			return -EINVAL;
+		break;
+	}
 
 	return buffer->access->mmap(buffer, vma);
 }
diff --git a/include/linux/iio/buffer.h b/include/linux/iio/buffer.h
index b6928ac5c63d..e87b8773253d 100644
--- a/include/linux/iio/buffer.h
+++ b/include/linux/iio/buffer.h
@@ -11,8 +11,15 @@
 
 struct iio_buffer;
 
+enum iio_buffer_direction {
+	IIO_BUFFER_DIRECTION_IN,
+	IIO_BUFFER_DIRECTION_OUT,
+};
+
 int iio_push_to_buffers(struct iio_dev *indio_dev, const void *data);
 
+int iio_buffer_remove_sample(struct iio_buffer *buffer, u8 *data);
+
 /**
  * iio_push_to_buffers_with_timestamp() - push data and timestamp to buffers
  * @indio_dev:		iio_dev structure for device.
diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h
index 1d57dc7ccb4f..47bdbf4a4519 100644
--- a/include/linux/iio/buffer_impl.h
+++ b/include/linux/iio/buffer_impl.h
@@ -7,6 +7,7 @@
 #ifdef CONFIG_IIO_BUFFER
 
 #include <uapi/linux/iio/buffer.h>
+#include <linux/iio/buffer.h>
 
 struct iio_dev;
 struct iio_buffer;
@@ -23,6 +24,10 @@ struct iio_buffer;
  * @read:		try to get a specified number of bytes (must exist)
  * @data_available:	indicates how much data is available for reading from
  *			the buffer.
+ * @remove_from:	remove sample from buffer. Drivers should calls this to
+ *			remove a sample from a buffer.
+ * @write:		try to write a number of bytes
+ * @space_available:	returns the amount of bytes available in a buffer
  * @request_update:	if a parameter change has been marked, update underlying
  *			storage.
  * @set_bytes_per_datum:set number of bytes per datum
@@ -61,6 +66,9 @@ struct iio_buffer_access_funcs {
 	int (*store_to)(struct iio_buffer *buffer, const void *data);
 	int (*read)(struct iio_buffer *buffer, size_t n, char __user *buf);
 	size_t (*data_available)(struct iio_buffer *buffer);
+	int (*remove_from)(struct iio_buffer *buffer, void *data);
+	int (*write)(struct iio_buffer *buffer, size_t n, const char __user *buf);
+	size_t (*space_available)(struct iio_buffer *buffer);
 
 	int (*request_update)(struct iio_buffer *buffer);
 
@@ -103,6 +111,9 @@ struct iio_buffer {
 	/**  @bytes_per_datum: Size of individual datum including timestamp. */
 	size_t bytes_per_datum;
 
+	/* @direction: Direction of the data stream (in/out). */
+	enum iio_buffer_direction direction;
+
 	/**
 	 * @access: Buffer access functions associated with the
 	 * implementation.
-- 
2.17.1

Re: [PATCH v2 2/5] iio: Add output buffer support

[Jonathan Cameron]

On Wed, 17 Feb 2021 10:34:35 +0200
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> From: Lars-Peter Clausen <lars@metafoo.de>
> 
> Currently IIO only supports buffer mode for capture devices like ADCs. Add
> support for buffered mode for output devices like DACs.
> 
> The output buffer implementation is analogous to the input buffer
> implementation. Instead of using read() to get data from the buffer write()
> is used to copy data into the buffer.
> 
> poll() with POLLOUT will wakeup if there is space available for more or
> equal to the configured watermark of samples.
> 
> Drivers can remove data from a buffer using iio_buffer_remove_sample(), the
> function can e.g. called from a trigger handler to write the data to
> hardware.
> 
> A buffer can only be either a output buffer or an input, but not both. So,
> for a device that has an ADC and DAC path, this will mean 2 IIO buffers
> (one for each direction).
> 
> The direction of the buffer is decided by the new direction field of the
> iio_buffer struct and should be set after allocating and before registering
> it.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

Just one question on this, otherwise looks good to me.

Jonathan

> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index 5d641f8adfbd..b9970c68005d 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -162,6 +162,69 @@ static ssize_t iio_buffer_read(struct file *filp, char __user *buf,
>  	return ret;
>  }
>  
...

>  /**
>   * iio_buffer_poll() - poll the buffer to find out if it has data
>   * @filp:	File structure pointer for device access
> @@ -182,8 +245,19 @@ static __poll_t iio_buffer_poll(struct file *filp,
>  		return 0;
>  
>  	poll_wait(filp, &rb->pollq, wait);
> -	if (iio_buffer_ready(indio_dev, rb, rb->watermark, 0))
> -		return EPOLLIN | EPOLLRDNORM;
> +
> +	switch (rb->direction) {
> +	case IIO_BUFFER_DIRECTION_IN:
> +		if (iio_buffer_ready(indio_dev, rb, rb->watermark, 0))
> +			return EPOLLIN | EPOLLRDNORM;
> +		break;
> +	case IIO_BUFFER_DIRECTION_OUT:
> +		if (iio_buffer_space_available(rb) >= rb->watermark)
> +			return EPOLLOUT | EPOLLWRNORM;
> +		break;
> +	}
> +
> +	/* need a way of knowing if there may be enough data... */

Curious on what this comment is referring to?

>  	return 0;
>  }
>  
> @@ -232,6 +306,16 @@ void iio_buffer_wakeup_poll(struct iio_dev *indio_dev)
>  	}
>  }
...

Re: [PATCH v2 2/5] iio: Add output buffer support

[Alexandru Ardelean]

On Thu, Feb 18, 2021 at 5:30 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Wed, 17 Feb 2021 10:34:35 +0200
> Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
>
> > From: Lars-Peter Clausen <lars@metafoo.de>
> >
> > Currently IIO only supports buffer mode for capture devices like ADCs. Add
> > support for buffered mode for output devices like DACs.
> >
> > The output buffer implementation is analogous to the input buffer
> > implementation. Instead of using read() to get data from the buffer write()
> > is used to copy data into the buffer.
> >
> > poll() with POLLOUT will wakeup if there is space available for more or
> > equal to the configured watermark of samples.
> >
> > Drivers can remove data from a buffer using iio_buffer_remove_sample(), the
> > function can e.g. called from a trigger handler to write the data to
> > hardware.
> >
> > A buffer can only be either a output buffer or an input, but not both. So,
> > for a device that has an ADC and DAC path, this will mean 2 IIO buffers
> > (one for each direction).
> >
> > The direction of the buffer is decided by the new direction field of the
> > iio_buffer struct and should be set after allocating and before registering
> > it.
> >
> > Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
>
> Just one question on this, otherwise looks good to me.
>
> Jonathan
>
> > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> > index 5d641f8adfbd..b9970c68005d 100644
> > --- a/drivers/iio/industrialio-buffer.c
> > +++ b/drivers/iio/industrialio-buffer.c
> > @@ -162,6 +162,69 @@ static ssize_t iio_buffer_read(struct file *filp, char __user *buf,
> >       return ret;
> >  }
> >
> ...
>
> >  /**
> >   * iio_buffer_poll() - poll the buffer to find out if it has data
> >   * @filp:    File structure pointer for device access
> > @@ -182,8 +245,19 @@ static __poll_t iio_buffer_poll(struct file *filp,
> >               return 0;
> >
> >       poll_wait(filp, &rb->pollq, wait);
> > -     if (iio_buffer_ready(indio_dev, rb, rb->watermark, 0))
> > -             return EPOLLIN | EPOLLRDNORM;
> > +
> > +     switch (rb->direction) {
> > +     case IIO_BUFFER_DIRECTION_IN:
> > +             if (iio_buffer_ready(indio_dev, rb, rb->watermark, 0))
> > +                     return EPOLLIN | EPOLLRDNORM;
> > +             break;
> > +     case IIO_BUFFER_DIRECTION_OUT:
> > +             if (iio_buffer_space_available(rb) >= rb->watermark)
> > +                     return EPOLLOUT | EPOLLWRNORM;
> > +             break;
> > +     }
> > +
> > +     /* need a way of knowing if there may be enough data... */
>
> Curious on what this comment is referring to?

I'm also a bit curious about what this comment is referring to.
I can remove it.

Maybe Lars can give some insight.

>
> >       return 0;
> >  }
> >
> > @@ -232,6 +306,16 @@ void iio_buffer_wakeup_poll(struct iio_dev *indio_dev)
> >       }
> >  }
> ...

[PATCH v2 3/5] iio: kfifo-buffer: Add output buffer support

[Alexandru Ardelean]

From: Lars-Peter Clausen <lars@metafoo.de>

Add output buffer support to the kfifo buffer implementation.

The implementation is straight forward and mostly just wraps the kfifo
API to provide the required operations.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/buffer/kfifo_buf.c | 50 ++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/drivers/iio/buffer/kfifo_buf.c b/drivers/iio/buffer/kfifo_buf.c
index 34289ce12f20..e8a434f84778 100644
--- a/drivers/iio/buffer/kfifo_buf.c
+++ b/drivers/iio/buffer/kfifo_buf.c
@@ -138,10 +138,60 @@ static void iio_kfifo_buffer_release(struct iio_buffer *buffer)
 	kfree(kf);
 }
 
+static size_t iio_kfifo_buf_space_available(struct iio_buffer *r)
+{
+	struct iio_kfifo *kf = iio_to_kfifo(r);
+	size_t avail;
+
+	mutex_lock(&kf->user_lock);
+	avail = kfifo_avail(&kf->kf);
+	mutex_unlock(&kf->user_lock);
+
+	return avail;
+}
+
+static int iio_kfifo_remove_from(struct iio_buffer *r, void *data)
+{
+	int ret;
+	struct iio_kfifo *kf = iio_to_kfifo(r);
+
+	if (kfifo_size(&kf->kf) < r->bytes_per_datum)
+		return -EBUSY;
+
+	ret = kfifo_out(&kf->kf, data, r->bytes_per_datum);
+	if (ret != r->bytes_per_datum)
+		return -EBUSY;
+
+	wake_up_interruptible_poll(&r->pollq, POLLOUT | POLLWRNORM);
+
+	return 0;
+}
+
+static int iio_kfifo_write(struct iio_buffer *r, size_t n,
+	const char __user *buf)
+{
+	struct iio_kfifo *kf = iio_to_kfifo(r);
+	int ret, copied;
+
+	mutex_lock(&kf->user_lock);
+	if (!kfifo_initialized(&kf->kf) || n < kfifo_esize(&kf->kf))
+		ret = -EINVAL;
+	else
+		ret = kfifo_from_user(&kf->kf, buf, n, &copied);
+	mutex_unlock(&kf->user_lock);
+	if (ret)
+		return ret;
+
+	return copied;
+}
+
 static const struct iio_buffer_access_funcs kfifo_access_funcs = {
 	.store_to = &iio_store_to_kfifo,
 	.read = &iio_read_kfifo,
 	.data_available = iio_kfifo_buf_data_available,
+	.remove_from = &iio_kfifo_remove_from,
+	.write = &iio_kfifo_write,
+	.space_available = &iio_kfifo_buf_space_available,
 	.request_update = &iio_request_update_kfifo,
 	.set_bytes_per_datum = &iio_set_bytes_per_datum_kfifo,
 	.set_length = &iio_set_length_kfifo,
-- 
2.17.1

[PATCH v2 4/5] iio: triggered-buffer: extend support to configure output buffers

[Alexandru Ardelean]

Now that output (kfifo) buffers are supported, we need to extend the
{devm_}iio_triggered_buffer_setup_ext() parameter list to take a direction
parameter.

This allows us to attach an output triggered buffer to a DAC device.
Unfortunately it's a bit difficult to add another macro to avoid changing 5
drivers where {devm_}iio_triggered_buffer_setup_ext() is used.
Well, it's doable, but may not be worth the trouble vs just updating all
these 5 drivers.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/accel/adxl372.c                           |  1 +
 drivers/iio/accel/bmc150-accel-core.c                 |  1 +
 drivers/iio/adc/at91-sama5d2_adc.c                    |  4 ++--
 drivers/iio/buffer/industrialio-triggered-buffer.c    |  8 ++++++--
 .../iio/common/cros_ec_sensors/cros_ec_sensors_core.c |  1 +
 drivers/iio/common/hid-sensors/hid-sensor-trigger.c   |  5 +++--
 include/linux/iio/triggered_buffer.h                  | 11 +++++++++--
 7 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/accel/adxl372.c b/drivers/iio/accel/adxl372.c
index 8ba1453b8dbf..f2e077f72531 100644
--- a/drivers/iio/accel/adxl372.c
+++ b/drivers/iio/accel/adxl372.c
@@ -1214,6 +1214,7 @@ int adxl372_probe(struct device *dev, struct regmap *regmap,
 	ret = devm_iio_triggered_buffer_setup_ext(dev,
 						  indio_dev, NULL,
 						  adxl372_trigger_handler,
+						  IIO_BUFFER_DIRECTION_IN,
 						  &adxl372_buffer_ops,
 						  adxl372_fifo_attributes);
 	if (ret < 0)
diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
index b0dbd12cbf42..3e0305b0065b 100644
--- a/drivers/iio/accel/bmc150-accel-core.c
+++ b/drivers/iio/accel/bmc150-accel-core.c
@@ -1743,6 +1743,7 @@ int bmc150_accel_core_probe(struct device *dev, struct regmap *regmap, int irq,
 	ret = iio_triggered_buffer_setup_ext(indio_dev,
 					     &iio_pollfunc_store_time,
 					     bmc150_accel_trigger_handler,
+					     IIO_BUFFER_DIRECTION_IN,
 					     &bmc150_accel_buffer_ops,
 					     fifo_attrs);
 	if (ret < 0) {
diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index a7826f097b95..fc134f9c0200 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -1680,8 +1680,8 @@ static int at91_adc_buffer_and_trigger_init(struct device *dev,
 		fifo_attrs = NULL;
 
 	ret = devm_iio_triggered_buffer_setup_ext(&indio->dev, indio,
-		&iio_pollfunc_store_time,
-		&at91_adc_trigger_handler, &at91_buffer_setup_ops, fifo_attrs);
+		&iio_pollfunc_store_time, &at91_adc_trigger_handler,
+		IIO_BUFFER_DIRECTION_IN, &at91_buffer_setup_ops, fifo_attrs);
 	if (ret < 0) {
 		dev_err(dev, "couldn't initialize the buffer.\n");
 		return ret;
diff --git a/drivers/iio/buffer/industrialio-triggered-buffer.c b/drivers/iio/buffer/industrialio-triggered-buffer.c
index b2b1b7d27af4..f400e978cd1e 100644
--- a/drivers/iio/buffer/industrialio-triggered-buffer.c
+++ b/drivers/iio/buffer/industrialio-triggered-buffer.c
@@ -19,6 +19,7 @@
  * @indio_dev:		IIO device structure
  * @h:			Function which will be used as pollfunc top half
  * @thread:		Function which will be used as pollfunc bottom half
+ * @direction:		Direction of the data stream (in/out).
  * @setup_ops:		Buffer setup functions to use for this device.
  *			If NULL the default setup functions for triggered
  *			buffers will be used.
@@ -38,6 +39,7 @@
 int iio_triggered_buffer_setup_ext(struct iio_dev *indio_dev,
 	irqreturn_t (*h)(int irq, void *p),
 	irqreturn_t (*thread)(int irq, void *p),
+	enum iio_buffer_direction direction,
 	const struct iio_buffer_setup_ops *setup_ops,
 	const struct attribute **buffer_attrs)
 {
@@ -68,6 +70,7 @@ int iio_triggered_buffer_setup_ext(struct iio_dev *indio_dev,
 	/* Flag that polled ring buffering is possible */
 	indio_dev->modes |= INDIO_BUFFER_TRIGGERED;
 
+	buffer->direction = direction;
 	buffer->attrs = buffer_attrs;
 
 	ret = iio_device_attach_buffer(indio_dev, buffer);
@@ -105,6 +108,7 @@ int devm_iio_triggered_buffer_setup_ext(struct device *dev,
 					struct iio_dev *indio_dev,
 					irqreturn_t (*h)(int irq, void *p),
 					irqreturn_t (*thread)(int irq, void *p),
+					enum iio_buffer_direction direction,
 					const struct iio_buffer_setup_ops *ops,
 					const struct attribute **buffer_attrs)
 {
@@ -118,8 +122,8 @@ int devm_iio_triggered_buffer_setup_ext(struct device *dev,
 
 	*ptr = indio_dev;
 
-	ret = iio_triggered_buffer_setup_ext(indio_dev, h, thread, ops,
-					     buffer_attrs);
+	ret = iio_triggered_buffer_setup_ext(indio_dev, h, thread, direction,
+					     ops, buffer_attrs);
 	if (!ret)
 		devres_add(dev, ptr);
 	else
diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
index f8824afe595e..afe8917f8c49 100644
--- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
+++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
@@ -369,6 +369,7 @@ int cros_ec_sensors_core_init(struct platform_device *pdev,
 			 */
 			ret = devm_iio_triggered_buffer_setup_ext(
 					dev, indio_dev, NULL, trigger_capture,
+					IIO_BUFFER_DIRECTION_IN,
 					NULL, fifo_attrs);
 			if (ret)
 				return ret;
diff --git a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
index 064c32bec9c7..dfaadbcac08a 100644
--- a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
+++ b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
@@ -248,8 +248,9 @@ int hid_sensor_setup_trigger(struct iio_dev *indio_dev, const char *name,
 		fifo_attrs = NULL;
 
 	ret = iio_triggered_buffer_setup_ext(indio_dev,
-					     &iio_pollfunc_store_time,
-					     NULL, NULL, fifo_attrs);
+					     &iio_pollfunc_store_time, NULL,
+					     IIO_BUFFER_DIRECTION_IN,
+					     NULL, fifo_attrs);
 	if (ret) {
 		dev_err(&indio_dev->dev, "Triggered Buffer Setup Failed\n");
 		return ret;
diff --git a/include/linux/iio/triggered_buffer.h b/include/linux/iio/triggered_buffer.h
index 7f154d1f8739..7490b05fc5b2 100644
--- a/include/linux/iio/triggered_buffer.h
+++ b/include/linux/iio/triggered_buffer.h
@@ -2,6 +2,7 @@
 #ifndef _LINUX_IIO_TRIGGERED_BUFFER_H_
 #define _LINUX_IIO_TRIGGERED_BUFFER_H_
 
+#include <linux/iio/buffer.h>
 #include <linux/interrupt.h>
 
 struct attribute;
@@ -11,21 +12,27 @@ struct iio_buffer_setup_ops;
 int iio_triggered_buffer_setup_ext(struct iio_dev *indio_dev,
 	irqreturn_t (*h)(int irq, void *p),
 	irqreturn_t (*thread)(int irq, void *p),
+	enum iio_buffer_direction direction,
 	const struct iio_buffer_setup_ops *setup_ops,
 	const struct attribute **buffer_attrs);
 void iio_triggered_buffer_cleanup(struct iio_dev *indio_dev);
 
 #define iio_triggered_buffer_setup(indio_dev, h, thread, setup_ops)		\
-	iio_triggered_buffer_setup_ext((indio_dev), (h), (thread), (setup_ops), NULL)
+	iio_triggered_buffer_setup_ext((indio_dev), (h), (thread),		\
+					IIO_BUFFER_DIRECTION_IN, (setup_ops),	\
+					NULL)
 
 int devm_iio_triggered_buffer_setup_ext(struct device *dev,
 					struct iio_dev *indio_dev,
 					irqreturn_t (*h)(int irq, void *p),
 					irqreturn_t (*thread)(int irq, void *p),
+					enum iio_buffer_direction direction,
 					const struct iio_buffer_setup_ops *ops,
 					const struct attribute **buffer_attrs);
 
 #define devm_iio_triggered_buffer_setup(dev, indio_dev, h, thread, setup_ops)	\
-	devm_iio_triggered_buffer_setup_ext((dev), (indio_dev), (h), (thread), (setup_ops), NULL)
+	devm_iio_triggered_buffer_setup_ext((dev), (indio_dev), (h), (thread),	\
+					    IIO_BUFFER_DIRECTION_IN,		\
+					    (setup_ops), NULL)
 
 #endif
-- 
2.17.1

[PATCH v2 5/5] iio: dac: ad5686: Add PWM as a trigger source

[Alexandru Ardelean]

From: Mircea Caprioru <mircea.caprioru@analog.com>

A PWM signal will be used as a trigger source to have a deterministic
sampling frequency since this family of DAC has no hardware interrupt
source.

This feature is made optional however, as there are some board setups where
this isn't used.

Signed-off-by: Mircea Caprioru <mircea.caprioru@analog.com>
Signed-off-by: Mihail Chindris <Mihail.Chindris@analog.com>
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/dac/ad5686-spi.c |   2 +-
 drivers/iio/dac/ad5686.c     | 146 ++++++++++++++++++++++++++++++++++-
 drivers/iio/dac/ad5686.h     |   7 +-
 drivers/iio/dac/ad5696-i2c.c |   2 +-
 4 files changed, 152 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/dac/ad5686-spi.c b/drivers/iio/dac/ad5686-spi.c
index 0188ded5137c..07fadcf8e1e3 100644
--- a/drivers/iio/dac/ad5686-spi.c
+++ b/drivers/iio/dac/ad5686-spi.c
@@ -92,7 +92,7 @@ static int ad5686_spi_probe(struct spi_device *spi)
 	const struct spi_device_id *id = spi_get_device_id(spi);
 
 	return ad5686_probe(&spi->dev, id->driver_data, id->name,
-			    ad5686_spi_write, ad5686_spi_read);
+			    ad5686_spi_write, ad5686_spi_read, spi->irq);
 }
 
 static int ad5686_spi_remove(struct spi_device *spi)
diff --git a/drivers/iio/dac/ad5686.c b/drivers/iio/dac/ad5686.c
index 7d6792ac1020..9e48559ec566 100644
--- a/drivers/iio/dac/ad5686.c
+++ b/drivers/iio/dac/ad5686.c
@@ -16,6 +16,10 @@
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
 
 #include "ad5686.h"
 
@@ -123,6 +127,7 @@ static int ad5686_read_raw(struct iio_dev *indio_dev,
 			   long m)
 {
 	struct ad5686_state *st = iio_priv(indio_dev);
+	struct pwm_state state;
 	int ret;
 
 	switch (m) {
@@ -139,6 +144,10 @@ static int ad5686_read_raw(struct iio_dev *indio_dev,
 		*val = st->vref_mv;
 		*val2 = chan->scan_type.realbits;
 		return IIO_VAL_FRACTIONAL_LOG2;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		pwm_get_state(st->pwm, &state);
+		*val = DIV_ROUND_CLOSEST_ULL(1000000000ULL, state.period);
+		return IIO_VAL_INT;
 	}
 	return -EINVAL;
 }
@@ -150,6 +159,7 @@ static int ad5686_write_raw(struct iio_dev *indio_dev,
 			    long mask)
 {
 	struct ad5686_state *st = iio_priv(indio_dev);
+	struct pwm_state state;
 	int ret;
 
 	switch (mask) {
@@ -164,6 +174,14 @@ static int ad5686_write_raw(struct iio_dev *indio_dev,
 				val << chan->scan_type.shift);
 		mutex_unlock(&st->lock);
 		break;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		pwm_get_state(st->pwm, &state);
+
+		state.period = DIV_ROUND_CLOSEST_ULL(1000000000ULL, val);
+		pwm_set_relative_duty_cycle(&state, 50, 100);
+
+		ret = pwm_apply_state(st->pwm, &state);
+		break;
 	default:
 		ret = -EINVAL;
 	}
@@ -171,7 +189,37 @@ static int ad5686_write_raw(struct iio_dev *indio_dev,
 	return ret;
 }
 
+static int ad5686_trig_set_state(struct iio_trigger *trig,
+				 bool state)
+{
+	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
+	struct ad5686_state *st = iio_priv(indio_dev);
+	struct pwm_state pwm_st;
+
+	pwm_get_state(st->pwm, &pwm_st);
+	pwm_st.enabled = state;
+
+	return pwm_apply_state(st->pwm, &pwm_st);
+}
+
+static int ad5686_validate_trigger(struct iio_dev *indio_dev,
+				    struct iio_trigger *trig)
+{
+	struct ad5686_state *st = iio_priv(indio_dev);
+
+	if (st->trig != trig)
+		return -EINVAL;
+
+	return 0;
+}
+
+static const struct iio_trigger_ops ad5686_trigger_ops = {
+	.validate_device = &iio_trigger_validate_own_device,
+	.set_trigger_state = &ad5686_trig_set_state,
+};
+
 static const struct iio_info ad5686_info = {
+	.validate_trigger = &ad5686_validate_trigger,
 	.read_raw = ad5686_read_raw,
 	.write_raw = ad5686_write_raw,
 };
@@ -194,8 +242,10 @@ static const struct iio_chan_spec_ext_info ad5686_ext_info[] = {
 		.output = 1,					\
 		.channel = chan,				\
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),	\
-		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),\
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \
+					    BIT(IIO_CHAN_INFO_SAMP_FREQ),\
 		.address = addr,				\
+		.scan_index = chan,				\
 		.scan_type = {					\
 			.sign = 'u',				\
 			.realbits = (bits),			\
@@ -428,13 +478,57 @@ static const struct ad5686_chip_info ad5686_chip_info_tbl[] = {
 	},
 };
 
+static irqreturn_t ad5686_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	const struct iio_chan_spec *chan;
+	struct iio_buffer *buffer = indio_dev->buffer;
+	struct ad5686_state *st = iio_priv(indio_dev);
+	u8 sample[2];
+	unsigned int i;
+	u16 val;
+	int ret;
+
+	ret = iio_buffer_remove_sample(buffer, sample);
+	if (ret < 0)
+		goto out;
+
+	mutex_lock(&st->lock);
+	for_each_set_bit(i, indio_dev->active_scan_mask, indio_dev->masklength) {
+		val = (sample[1] << 8) + sample[0];
+
+		chan = iio_find_channel_from_si(indio_dev, i);
+		ret = st->write(st, AD5686_CMD_WRITE_INPUT_N_UPDATE_N,
+				chan->address, val << chan->scan_type.shift);
+	}
+	mutex_unlock(&st->lock);
+
+out:
+	iio_trigger_notify_done(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t ad5686_irq_handler(int irq, void *data)
+{
+	struct iio_dev *indio_dev = data;
+	struct ad5686_state *st = iio_priv(indio_dev);
+
+	if (iio_buffer_enabled(indio_dev))
+		iio_trigger_poll(st->trig);
+
+	return IRQ_HANDLED;
+}
+
 int ad5686_probe(struct device *dev,
 		 enum ad5686_supported_device_ids chip_type,
 		 const char *name, ad5686_write_func write,
-		 ad5686_read_func read)
+		 ad5686_read_func read, int irq)
 {
 	struct ad5686_state *st;
 	struct iio_dev *indio_dev;
+	struct pwm_state state;
 	unsigned int val, ref_bit_msk;
 	u8 cmd;
 	int ret, i, voltage_uv = 0;
@@ -450,6 +544,23 @@ int ad5686_probe(struct device *dev,
 	st->write = write;
 	st->read = read;
 
+	mutex_init(&st->lock);
+
+	st->trig = devm_iio_trigger_alloc(dev, "%s-dev%d", name, indio_dev->id);
+	if (st->trig == NULL)
+		ret = -ENOMEM;
+
+	st->trig->ops = &ad5686_trigger_ops;
+	st->trig->dev.parent = dev;
+	iio_trigger_set_drvdata(st->trig, indio_dev);
+
+	ret = devm_iio_trigger_register(dev, st->trig);
+	if (ret)
+		return ret;
+
+	/* select default trigger */
+	indio_dev->trig = iio_trigger_get(st->trig);
+
 	st->reg = devm_regulator_get_optional(dev, "vcc");
 	if (!IS_ERR(st->reg)) {
 		ret = regulator_enable(st->reg);
@@ -463,6 +574,30 @@ int ad5686_probe(struct device *dev,
 		voltage_uv = ret;
 	}
 
+	/* PWM configuration */
+	st->pwm = devm_pwm_get(dev, "pwm-trigger");
+	if (!IS_ERR(st->pwm)) {
+		/* Set a default pwm frequency of 1kHz and 50% duty cycle */
+		pwm_init_state(st->pwm, &state);
+		state.enabled = false;
+		state.period = 1000000;
+		pwm_set_relative_duty_cycle(&state, 50, 100);
+		ret = pwm_apply_state(st->pwm, &state);
+		if (ret < 0)
+			return ret;
+	}
+
+	/* Configure IRQ */
+	if (irq) {
+		ret = devm_request_threaded_irq(dev, irq, NULL, ad5686_irq_handler,
+						IRQF_TRIGGER_RISING | IRQF_ONESHOT,
+						"ad5686 irq", indio_dev);
+		if (ret)
+			return ret;
+
+		st->irq = irq;
+	}
+
 	st->chip_info = &ad5686_chip_info_tbl[chip_type];
 
 	if (voltage_uv)
@@ -513,6 +648,13 @@ int ad5686_probe(struct device *dev,
 	if (ret)
 		goto error_disable_reg;
 
+	ret = devm_iio_triggered_buffer_setup_ext(dev, indio_dev, NULL,
+						  &ad5686_trigger_handler,
+						  IIO_BUFFER_DIRECTION_OUT,
+						  NULL, NULL);
+	if (ret)
+		goto error_disable_reg;
+
 	ret = iio_device_register(indio_dev);
 	if (ret)
 		goto error_disable_reg;
diff --git a/drivers/iio/dac/ad5686.h b/drivers/iio/dac/ad5686.h
index d9c8ba413fe9..b287873553a1 100644
--- a/drivers/iio/dac/ad5686.h
+++ b/drivers/iio/dac/ad5686.h
@@ -12,6 +12,7 @@
 #include <linux/cache.h>
 #include <linux/mutex.h>
 #include <linux/kernel.h>
+#include <linux/pwm.h>
 
 #define AD5310_CMD(x)				((x) << 12)
 
@@ -112,6 +113,7 @@ struct ad5686_chip_info {
 /**
  * struct ad5446_state - driver instance specific data
  * @spi:		spi_device
+ * @pwm:		pwm used for buffer trigger
  * @chip_info:		chip model specific constants, available modes etc
  * @reg:		supply regulator
  * @vref_mv:		actual reference voltage used
@@ -124,6 +126,8 @@ struct ad5686_chip_info {
 
 struct ad5686_state {
 	struct device			*dev;
+	struct pwm_device		*pwm;
+	struct iio_trigger		*trig;
 	const struct ad5686_chip_info	*chip_info;
 	struct regulator		*reg;
 	unsigned short			vref_mv;
@@ -133,6 +137,7 @@ struct ad5686_state {
 	ad5686_read_func		read;
 	bool				use_internal_vref;
 	struct mutex			lock;
+	int				irq;
 
 	/*
 	 * DMA (thus cache coherency maintenance) requires the
@@ -150,7 +155,7 @@ struct ad5686_state {
 int ad5686_probe(struct device *dev,
 		 enum ad5686_supported_device_ids chip_type,
 		 const char *name, ad5686_write_func write,
-		 ad5686_read_func read);
+		 ad5686_read_func read, int irq);
 
 int ad5686_remove(struct device *dev);
 
diff --git a/drivers/iio/dac/ad5696-i2c.c b/drivers/iio/dac/ad5696-i2c.c
index a39eda7c02d2..f80acc0972ea 100644
--- a/drivers/iio/dac/ad5696-i2c.c
+++ b/drivers/iio/dac/ad5696-i2c.c
@@ -62,7 +62,7 @@ static int ad5686_i2c_probe(struct i2c_client *i2c,
 			    const struct i2c_device_id *id)
 {
 	return ad5686_probe(&i2c->dev, id->driver_data, id->name,
-			    ad5686_i2c_write, ad5686_i2c_read);
+			    ad5686_i2c_write, ad5686_i2c_read, i2c->irq);
 }
 
 static int ad5686_i2c_remove(struct i2c_client *i2c)
-- 
2.17.1

Re: [PATCH v2 5/5] iio: dac: ad5686: Add PWM as a trigger source

[Jonathan Cameron]

On Wed, 17 Feb 2021 10:34:38 +0200
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> From: Mircea Caprioru <mircea.caprioru@analog.com>
> 
> A PWM signal will be used as a trigger source to have a deterministic
> sampling frequency since this family of DAC has no hardware interrupt
> source.
> 
> This feature is made optional however, as there are some board setups where
> this isn't used.
> 

So this is taking a very generic setup, but then implementing it
as a bit of a hack within the driver.

It's effectively a PWM connected up to an instance
of iio/triggers/iio-trig-interrupt.c

Now, I've not looked at that trigger driver for a while, so you may well
need to figure out how to add a binding to instantiate it.
(looks like no one has used it since board file days, or via instantiation
from another driver).

It's a slightly odd corner case as what it reflects is that we have
an interrupt available that is intended to drive some sort of data
capture or output (it's a trigger signal) - but exactly what is done
is a runtime configurable.  In this particular case that interrupt
is hooked up to a PWM and we also want to represent that.

The fact it's being driven via a PWM is interesting but we should be
able to extend that trigger driver to optionally accept a pwm provider
and if it has one provide frequency control.

Binding might look something like the following..

interrupt-trigger {
   interrupts = <>;
   pwms = <&pwm 0 4000 PWM_POLARITY_INVERTED>;	
};

@Rob, what do you think of this odd beast?

So all in all, this generic facility needs a generic implementation, not
one buried in a driver.

Another open question here is whether you really can't just use an hrtimer
to get similar precision?  Way back at the dawn of time in IIO we had
code to use the RTC periodic ticks as a trigger with the theory that they
would give very precise and even timing.  In the end it turned out that
hrtimers worked just as well (and RTCs drivers emulated the periodic
ticks via hrtimers, dropping their use of the hardware periodic timers).

Jonathan



> Signed-off-by: Mircea Caprioru <mircea.caprioru@analog.com>
> Signed-off-by: Mihail Chindris <Mihail.Chindris@analog.com>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
>  drivers/iio/dac/ad5686-spi.c |   2 +-
>  drivers/iio/dac/ad5686.c     | 146 ++++++++++++++++++++++++++++++++++-
>  drivers/iio/dac/ad5686.h     |   7 +-
>  drivers/iio/dac/ad5696-i2c.c |   2 +-
>  4 files changed, 152 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/dac/ad5686-spi.c b/drivers/iio/dac/ad5686-spi.c
> index 0188ded5137c..07fadcf8e1e3 100644
> --- a/drivers/iio/dac/ad5686-spi.c
> +++ b/drivers/iio/dac/ad5686-spi.c
> @@ -92,7 +92,7 @@ static int ad5686_spi_probe(struct spi_device *spi)
>  	const struct spi_device_id *id = spi_get_device_id(spi);
>  
>  	return ad5686_probe(&spi->dev, id->driver_data, id->name,
> -			    ad5686_spi_write, ad5686_spi_read);
> +			    ad5686_spi_write, ad5686_spi_read, spi->irq);
>  }
>  
>  static int ad5686_spi_remove(struct spi_device *spi)
> diff --git a/drivers/iio/dac/ad5686.c b/drivers/iio/dac/ad5686.c
> index 7d6792ac1020..9e48559ec566 100644
> --- a/drivers/iio/dac/ad5686.c
> +++ b/drivers/iio/dac/ad5686.c
> @@ -16,6 +16,10 @@
>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
>  
>  #include "ad5686.h"
>  
> @@ -123,6 +127,7 @@ static int ad5686_read_raw(struct iio_dev *indio_dev,
>  			   long m)
>  {
>  	struct ad5686_state *st = iio_priv(indio_dev);
> +	struct pwm_state state;
>  	int ret;
>  
>  	switch (m) {
> @@ -139,6 +144,10 @@ static int ad5686_read_raw(struct iio_dev *indio_dev,
>  		*val = st->vref_mv;
>  		*val2 = chan->scan_type.realbits;
>  		return IIO_VAL_FRACTIONAL_LOG2;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		pwm_get_state(st->pwm, &state);
> +		*val = DIV_ROUND_CLOSEST_ULL(1000000000ULL, state.period);
> +		return IIO_VAL_INT;
>  	}
>  	return -EINVAL;
>  }
> @@ -150,6 +159,7 @@ static int ad5686_write_raw(struct iio_dev *indio_dev,
>  			    long mask)
>  {
>  	struct ad5686_state *st = iio_priv(indio_dev);
> +	struct pwm_state state;
>  	int ret;
>  
>  	switch (mask) {
> @@ -164,6 +174,14 @@ static int ad5686_write_raw(struct iio_dev *indio_dev,
>  				val << chan->scan_type.shift);
>  		mutex_unlock(&st->lock);
>  		break;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		pwm_get_state(st->pwm, &state);
> +
> +		state.period = DIV_ROUND_CLOSEST_ULL(1000000000ULL, val);
> +		pwm_set_relative_duty_cycle(&state, 50, 100);
> +
> +		ret = pwm_apply_state(st->pwm, &state);
> +		break;
>  	default:
>  		ret = -EINVAL;
>  	}
> @@ -171,7 +189,37 @@ static int ad5686_write_raw(struct iio_dev *indio_dev,
>  	return ret;
>  }
>  
> +static int ad5686_trig_set_state(struct iio_trigger *trig,
> +				 bool state)
> +{
> +	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> +	struct ad5686_state *st = iio_priv(indio_dev);
> +	struct pwm_state pwm_st;
> +
> +	pwm_get_state(st->pwm, &pwm_st);
> +	pwm_st.enabled = state;
> +
> +	return pwm_apply_state(st->pwm, &pwm_st);
> +}
> +
> +static int ad5686_validate_trigger(struct iio_dev *indio_dev,
> +				    struct iio_trigger *trig)
> +{
> +	struct ad5686_state *st = iio_priv(indio_dev);
> +
> +	if (st->trig != trig)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static const struct iio_trigger_ops ad5686_trigger_ops = {
> +	.validate_device = &iio_trigger_validate_own_device,
> +	.set_trigger_state = &ad5686_trig_set_state,
> +};
> +
>  static const struct iio_info ad5686_info = {
> +	.validate_trigger = &ad5686_validate_trigger,
>  	.read_raw = ad5686_read_raw,
>  	.write_raw = ad5686_write_raw,
>  };
> @@ -194,8 +242,10 @@ static const struct iio_chan_spec_ext_info ad5686_ext_info[] = {
>  		.output = 1,					\
>  		.channel = chan,				\
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),	\
> -		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),\
> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \
> +					    BIT(IIO_CHAN_INFO_SAMP_FREQ),\
>  		.address = addr,				\
> +		.scan_index = chan,				\
>  		.scan_type = {					\
>  			.sign = 'u',				\
>  			.realbits = (bits),			\
> @@ -428,13 +478,57 @@ static const struct ad5686_chip_info ad5686_chip_info_tbl[] = {
>  	},
>  };
>  
> +static irqreturn_t ad5686_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	const struct iio_chan_spec *chan;
> +	struct iio_buffer *buffer = indio_dev->buffer;
> +	struct ad5686_state *st = iio_priv(indio_dev);
> +	u8 sample[2];
> +	unsigned int i;
> +	u16 val;
> +	int ret;
> +
> +	ret = iio_buffer_remove_sample(buffer, sample);
> +	if (ret < 0)
> +		goto out;
> +
> +	mutex_lock(&st->lock);
> +	for_each_set_bit(i, indio_dev->active_scan_mask, indio_dev->masklength) {
> +		val = (sample[1] << 8) + sample[0];
> +
> +		chan = iio_find_channel_from_si(indio_dev, i);
> +		ret = st->write(st, AD5686_CMD_WRITE_INPUT_N_UPDATE_N,
> +				chan->address, val << chan->scan_type.shift);
> +	}
> +	mutex_unlock(&st->lock);
> +
> +out:
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t ad5686_irq_handler(int irq, void *data)
> +{
> +	struct iio_dev *indio_dev = data;
> +	struct ad5686_state *st = iio_priv(indio_dev);
> +
> +	if (iio_buffer_enabled(indio_dev))
> +		iio_trigger_poll(st->trig);
> +
> +	return IRQ_HANDLED;
> +}
> +
>  int ad5686_probe(struct device *dev,
>  		 enum ad5686_supported_device_ids chip_type,
>  		 const char *name, ad5686_write_func write,
> -		 ad5686_read_func read)
> +		 ad5686_read_func read, int irq)
>  {
>  	struct ad5686_state *st;
>  	struct iio_dev *indio_dev;
> +	struct pwm_state state;
>  	unsigned int val, ref_bit_msk;
>  	u8 cmd;
>  	int ret, i, voltage_uv = 0;
> @@ -450,6 +544,23 @@ int ad5686_probe(struct device *dev,
>  	st->write = write;
>  	st->read = read;
>  
> +	mutex_init(&st->lock);
> +
> +	st->trig = devm_iio_trigger_alloc(dev, "%s-dev%d", name, indio_dev->id);
> +	if (st->trig == NULL)
> +		ret = -ENOMEM;
> +
> +	st->trig->ops = &ad5686_trigger_ops;
> +	st->trig->dev.parent = dev;
> +	iio_trigger_set_drvdata(st->trig, indio_dev);
> +
> +	ret = devm_iio_trigger_register(dev, st->trig);
> +	if (ret)
> +		return ret;
> +
> +	/* select default trigger */
> +	indio_dev->trig = iio_trigger_get(st->trig);
> +
>  	st->reg = devm_regulator_get_optional(dev, "vcc");
>  	if (!IS_ERR(st->reg)) {
>  		ret = regulator_enable(st->reg);
> @@ -463,6 +574,30 @@ int ad5686_probe(struct device *dev,
>  		voltage_uv = ret;
>  	}
>  
> +	/* PWM configuration */
> +	st->pwm = devm_pwm_get(dev, "pwm-trigger");
> +	if (!IS_ERR(st->pwm)) {
> +		/* Set a default pwm frequency of 1kHz and 50% duty cycle */
> +		pwm_init_state(st->pwm, &state);
> +		state.enabled = false;
> +		state.period = 1000000;
> +		pwm_set_relative_duty_cycle(&state, 50, 100);
> +		ret = pwm_apply_state(st->pwm, &state);
> +		if (ret < 0)
> +			return ret;
> +	}

Hmm. This shouldn't be part of the individual device driver.
It's just an irq trigger that happens to be driven from a pwm.


> +
> +	/* Configure IRQ */
> +	if (irq) {
> +		ret = devm_request_threaded_irq(dev, irq, NULL, ad5686_irq_handler,
> +						IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> +						"ad5686 irq", indio_dev);
> +		if (ret)
> +			return ret;
> +
> +		st->irq = irq;
> +	}
> +
>  	st->chip_info = &ad5686_chip_info_tbl[chip_type];
>  
>  	if (voltage_uv)
> @@ -513,6 +648,13 @@ int ad5686_probe(struct device *dev,
>  	if (ret)
>  		goto error_disable_reg;
>  
> +	ret = devm_iio_triggered_buffer_setup_ext(dev, indio_dev, NULL,
> +						  &ad5686_trigger_handler,
> +						  IIO_BUFFER_DIRECTION_OUT,
> +						  NULL, NULL);
> +	if (ret)
> +		goto error_disable_reg;
> +
>  	ret = iio_device_register(indio_dev);
>  	if (ret)
>  		goto error_disable_reg;

Re: [PATCH v2 5/5] iio: dac: ad5686: Add PWM as a trigger source

[Rob Herring]

On Thu, Feb 18, 2021 at 8:05 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Wed, 17 Feb 2021 10:34:38 +0200
> Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
>
> > From: Mircea Caprioru <mircea.caprioru@analog.com>
> >
> > A PWM signal will be used as a trigger source to have a deterministic
> > sampling frequency since this family of DAC has no hardware interrupt
> > source.
> >
> > This feature is made optional however, as there are some board setups where
> > this isn't used.
> >
>
> So this is taking a very generic setup, but then implementing it
> as a bit of a hack within the driver.
>
> It's effectively a PWM connected up to an instance
> of iio/triggers/iio-trig-interrupt.c
>
> Now, I've not looked at that trigger driver for a while, so you may well
> need to figure out how to add a binding to instantiate it.
> (looks like no one has used it since board file days, or via instantiation
> from another driver).
>
> It's a slightly odd corner case as what it reflects is that we have
> an interrupt available that is intended to drive some sort of data
> capture or output (it's a trigger signal) - but exactly what is done
> is a runtime configurable.  In this particular case that interrupt
> is hooked up to a PWM and we also want to represent that.
>
> The fact it's being driven via a PWM is interesting but we should be
> able to extend that trigger driver to optionally accept a pwm provider
> and if it has one provide frequency control.
>
> Binding might look something like the following..
>
> interrupt-trigger {
>    interrupts = <>;
>    pwms = <&pwm 0 4000 PWM_POLARITY_INVERTED>;
> };
>
> @Rob, what do you think of this odd beast?

So a PWM routed back to a GPIO interrupt? It needs a compatible, but
otherwise I wouldn't object to the binding if that's what the h/w
looks like. But from an OS perspective, I don't think you need it.

> So all in all, this generic facility needs a generic implementation, not
> one buried in a driver.
>
> Another open question here is whether you really can't just use an hrtimer
> to get similar precision?  Way back at the dawn of time in IIO we had
> code to use the RTC periodic ticks as a trigger with the theory that they
> would give very precise and even timing.  In the end it turned out that
> hrtimers worked just as well (and RTCs drivers emulated the periodic
> ticks via hrtimers, dropping their use of the hardware periodic timers).

+100

A hrtimer is likely going to be more precise. IIRC, timers are
serviced first. Either way, you're going to have some amount of
interrupt service latency, so any precision you think you are gaining
by 'doing it in h/w' isn't really there.

Rob

Re: [PATCH v2 5/5] iio: dac: ad5686: Add PWM as a trigger source

[Alexandru Ardelean]

On Thu, Feb 18, 2021 at 5:31 PM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Thu, Feb 18, 2021 at 8:05 AM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Wed, 17 Feb 2021 10:34:38 +0200
> > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> >
> > > From: Mircea Caprioru <mircea.caprioru@analog.com>
> > >
> > > A PWM signal will be used as a trigger source to have a deterministic
> > > sampling frequency since this family of DAC has no hardware interrupt
> > > source.
> > >
> > > This feature is made optional however, as there are some board setups where
> > > this isn't used.
> > >
> >
> > So this is taking a very generic setup, but then implementing it
> > as a bit of a hack within the driver.
> >
> > It's effectively a PWM connected up to an instance
> > of iio/triggers/iio-trig-interrupt.c
> >
> > Now, I've not looked at that trigger driver for a while, so you may well
> > need to figure out how to add a binding to instantiate it.
> > (looks like no one has used it since board file days, or via instantiation
> > from another driver).
> >
> > It's a slightly odd corner case as what it reflects is that we have
> > an interrupt available that is intended to drive some sort of data
> > capture or output (it's a trigger signal) - but exactly what is done
> > is a runtime configurable.  In this particular case that interrupt
> > is hooked up to a PWM and we also want to represent that.
> >
> > The fact it's being driven via a PWM is interesting but we should be
> > able to extend that trigger driver to optionally accept a pwm provider
> > and if it has one provide frequency control.
> >

So, the main intent here was to provide a user for this new output kfifo.
I don't think I have time to re-spin this into a proper solution.
Someone else may come about and do it.

I'll drop this from the series [for now].

> > Binding might look something like the following..
> >
> > interrupt-trigger {
> >    interrupts = <>;
> >    pwms = <&pwm 0 4000 PWM_POLARITY_INVERTED>;
> > };
> >
> > @Rob, what do you think of this odd beast?
>
> So a PWM routed back to a GPIO interrupt? It needs a compatible, but
> otherwise I wouldn't object to the binding if that's what the h/w
> looks like. But from an OS perspective, I don't think you need it.
>
> > So all in all, this generic facility needs a generic implementation, not
> > one buried in a driver.
> >
> > Another open question here is whether you really can't just use an hrtimer
> > to get similar precision?  Way back at the dawn of time in IIO we had
> > code to use the RTC periodic ticks as a trigger with the theory that they
> > would give very precise and even timing.  In the end it turned out that
> > hrtimers worked just as well (and RTCs drivers emulated the periodic
> > ticks via hrtimers, dropping their use of the hardware periodic timers).
>
> +100
>
> A hrtimer is likely going to be more precise. IIRC, timers are
> serviced first. Either way, you're going to have some amount of
> interrupt service latency, so any precision you think you are gaining
> by 'doing it in h/w' isn't really there.
>
> Rob

Re: [PATCH v2 5/5] iio: dac: ad5686: Add PWM as a trigger source

[Lars-Peter Clausen]

On 2/18/21 3:05 PM, Jonathan Cameron wrote:
> On Wed, 17 Feb 2021 10:34:38 +0200
> Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
>
>> From: Mircea Caprioru <mircea.caprioru@analog.com>
>>
>> A PWM signal will be used as a trigger source to have a deterministic
>> sampling frequency since this family of DAC has no hardware interrupt
>> source.
>>
>> This feature is made optional however, as there are some board setups where
>> this isn't used.
>>
> So this is taking a very generic setup, but then implementing it
> as a bit of a hack within the driver.
>
> It's effectively a PWM connected up to an instance
> of iio/triggers/iio-trig-interrupt.c
>
> Now, I've not looked at that trigger driver for a while, so you may well
> need to figure out how to add a binding to instantiate it.
> (looks like no one has used it since board file days, or via instantiation
> from another driver).
>
> It's a slightly odd corner case as what it reflects is that we have
> an interrupt available that is intended to drive some sort of data
> capture or output (it's a trigger signal) - but exactly what is done
> is a runtime configurable.  In this particular case that interrupt
> is hooked up to a PWM and we also want to represent that.
>
> The fact it's being driven via a PWM is interesting but we should be
> able to extend that trigger driver to optionally accept a pwm provider
> and if it has one provide frequency control.
>
> Binding might look something like the following..
>
> interrupt-trigger {
>     interrupts = <>;
>     pwms = <&pwm 0 4000 PWM_POLARITY_INVERTED>;	
> };
>
> @Rob, what do you think of this odd beast?
>
> So all in all, this generic facility needs a generic implementation, not
> one buried in a driver.
>
> Another open question here is whether you really can't just use an hrtimer
> to get similar precision?  Way back at the dawn of time in IIO we had
> code to use the RTC periodic ticks as a trigger with the theory that they
> would give very precise and even timing.  In the end it turned out that
> hrtimers worked just as well (and RTCs drivers emulated the periodic
> ticks via hrtimers, dropping their use of the hardware periodic timers).
>
The way this DAC works is that it has a "latch" pin and some shadow 
registers. The way this is supposed to be used is that you update the 
shadow registers and then when the there is a rising edge on the latch 
pin all the shadow register values are transferred to DAC output registers.

This means if you hook up a periodic signal like a PWM or clock to the 
latch pin you can generate very precise waveforms that have much lower 
jitter than when using a hrtimer since there is no variable interrupt 
latency for the update step itself. This is useful when generating 
periodic signals.

But you could for example also use a GPIO to update multiple discrete 
DACs at the same time.

This is not specific to this particular chip. There are quite a few ADI 
(and probably from other vendors) precision DACs that have this 
functionality. I agree that this should be a some sort of generic 
trigger helper module.

Now for the implementation since there is a direct connection between 
the PWM and the DAC I think it makes sense to describe this connection 
in the DT. After all if there is no connection this will not work.

As for the interrupt, most PWM controllers do have the ability to 
generate an IRQ by themselves once per period. There should be not need 
for a hardware loopback. Unfortunately the PWM framework does not have a 
mechanism yet to expose those IRQs and register a callback.

A similar feature btw exists for many of the ADCs and we did have this 
special Blackfin PWM trigger[1] back in the day to support this. The 
bfin PWM trigger driver essentially implements what I'm describing 
above, but without using the PWM framework.

- Lars

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/staging/iio/trigger/iio-trig-bfin-timer.c?h=v3.15

Re: [PATCH v2 5/5] iio: dac: ad5686: Add PWM as a trigger source

[Jonathan Cameron]

On Tue, 23 Feb 2021 17:37:40 +0100
Lars-Peter Clausen <lars@metafoo.de> wrote:

> On 2/18/21 3:05 PM, Jonathan Cameron wrote:
> > On Wed, 17 Feb 2021 10:34:38 +0200
> > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> >  
> >> From: Mircea Caprioru <mircea.caprioru@analog.com>
> >>
> >> A PWM signal will be used as a trigger source to have a deterministic
> >> sampling frequency since this family of DAC has no hardware interrupt
> >> source.
> >>
> >> This feature is made optional however, as there are some board setups where
> >> this isn't used.
> >>  
> > So this is taking a very generic setup, but then implementing it
> > as a bit of a hack within the driver.
> >
> > It's effectively a PWM connected up to an instance
> > of iio/triggers/iio-trig-interrupt.c
> >
> > Now, I've not looked at that trigger driver for a while, so you may well
> > need to figure out how to add a binding to instantiate it.
> > (looks like no one has used it since board file days, or via instantiation
> > from another driver).
> >
> > It's a slightly odd corner case as what it reflects is that we have
> > an interrupt available that is intended to drive some sort of data
> > capture or output (it's a trigger signal) - but exactly what is done
> > is a runtime configurable.  In this particular case that interrupt
> > is hooked up to a PWM and we also want to represent that.
> >
> > The fact it's being driven via a PWM is interesting but we should be
> > able to extend that trigger driver to optionally accept a pwm provider
> > and if it has one provide frequency control.
> >
> > Binding might look something like the following..
> >
> > interrupt-trigger {
> >     interrupts = <>;
> >     pwms = <&pwm 0 4000 PWM_POLARITY_INVERTED>;	
> > };
> >
> > @Rob, what do you think of this odd beast?
> >
> > So all in all, this generic facility needs a generic implementation, not
> > one buried in a driver.
> >
> > Another open question here is whether you really can't just use an hrtimer
> > to get similar precision?  Way back at the dawn of time in IIO we had
> > code to use the RTC periodic ticks as a trigger with the theory that they
> > would give very precise and even timing.  In the end it turned out that
> > hrtimers worked just as well (and RTCs drivers emulated the periodic
> > ticks via hrtimers, dropping their use of the hardware periodic timers).
> >  
> The way this DAC works is that it has a "latch" pin and some shadow 
> registers. The way this is supposed to be used is that you update the 
> shadow registers and then when the there is a rising edge on the latch 
> pin all the shadow register values are transferred to DAC output registers.
> 
> This means if you hook up a periodic signal like a PWM or clock to the 
> latch pin you can generate very precise waveforms that have much lower 
> jitter than when using a hrtimer since there is no variable interrupt 
> latency for the update step itself. This is useful when generating 
> periodic signals.
> 
> But you could for example also use a GPIO to update multiple discrete 
> DACs at the same time.
> 
> This is not specific to this particular chip. There are quite a few ADI 
> (and probably from other vendors) precision DACs that have this 
> functionality. I agree that this should be a some sort of generic 
> trigger helper module.
> 
> Now for the implementation since there is a direct connection between 
> the PWM and the DAC I think it makes sense to describe this connection 
> in the DT. After all if there is no connection this will not work.

Thanks for the detailed description. That makes a lot more sense. 

This is some sort of hybrid of the hardware internal triggers
we have for some SoC ADCs and wiring up a gpio pin to trigger the latch
signal.   PWM is one valid way of wiring it up (possibly most sensible
one), but not necessarily the only one.
I guess the one behind element is also a bit non intuitive (data is
put in place on previous interrupt / edge but latched on the next
one)

Hmm. If we makes sure the binding is cleanly defined, we could do
a driver specific implementation for now, with the option to figure
something else out later.

Exactly how to do this needs some thought...
+ lifting this description of hot it works into the patch description
would help :)

Jonathan

> 
> As for the interrupt, most PWM controllers do have the ability to 
> generate an IRQ by themselves once per period. There should be not need 
> for a hardware loopback. Unfortunately the PWM framework does not have a 
> mechanism yet to expose those IRQs and register a callback.
> 
> A similar feature btw exists for many of the ADCs and we did have this 
> special Blackfin PWM trigger[1] back in the day to support this. The 
> bfin PWM trigger driver essentially implements what I'm describing 
> above, but without using the PWM framework.



> 
> - Lars
> 
> [1] 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/staging/iio/trigger/iio-trig-bfin-timer.c?h=v3.15
> 

Re: [PATCH v2 0/5] iio: Add output buffer support

[Jonathan Cameron]

On Wed, 17 Feb 2021 10:34:33 +0200
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> This patchset is based on the new multibuffer set.
> It doesn't require the high-speed/mmap interface.
> That will come later on a v2.
> 
> Changelog v1 -> v2:
> * https://lore.kernel.org/linux-iio/20210212102021.47276-4-alexandru.ardelean@analog.com/T/#u
> * removed DMA patches for now
> * in patch 'iio: Add output buffer support'
>   - added /sys/bus/iio/devices/iio:deviceX/bufferY/direction attribute
>   - thinking about this, an update to the new buffer infrastructure is
>     required when adding ADDAC/MxFE/transceivers; right now there is no
>     problem, because we have only ADCs and DACs; but when we get
>     transceivers, a bufferY/ directory needs to filter in/out
>     scan_elements/ ; this only occured to me recently (it's one of those
>     things that pops up later in mind)

For that we can rely on review in the short term, but agreed a sanity
check that everything matches would make sense.

> * added 'iio: Documentation: update definitions for bufferY and scan_elements'
>   - seems I forgot this on the original multibuffer patchset

Likewise. :)  Who needs docs?

> * added 'iio: triggered-buffer: extend support to configure output buffers'
>   - basically output triggered buffer support
> * added 'iio: dac: ad5686: Add PWM as a trigger source'
>   - this is a first user of this infrastructure
> 
> Alexandru Ardelean (2):
>   iio: Documentation: update definitions for bufferY and scan_elements
>   iio: triggered-buffer: extend support to configure output buffers
> 
> Lars-Peter Clausen (2):
>   iio: Add output buffer support
>   iio: kfifo-buffer: Add output buffer support
> 
> Mircea Caprioru (1):
>   iio: dac: ad5686: Add PWM as a trigger source
> 
>  Documentation/ABI/testing/sysfs-bus-iio       |  92 +++++++++++
>  drivers/iio/accel/adxl372.c                   |   1 +
>  drivers/iio/accel/bmc150-accel-core.c         |   1 +
>  drivers/iio/adc/at91-sama5d2_adc.c            |   4 +-
>  .../buffer/industrialio-triggered-buffer.c    |   8 +-
>  drivers/iio/buffer/kfifo_buf.c                |  50 ++++++
>  .../cros_ec_sensors/cros_ec_sensors_core.c    |   1 +
>  .../common/hid-sensors/hid-sensor-trigger.c   |   5 +-
>  drivers/iio/dac/ad5686-spi.c                  |   2 +-
>  drivers/iio/dac/ad5686.c                      | 146 +++++++++++++++++-
>  drivers/iio/dac/ad5686.h                      |   7 +-
>  drivers/iio/dac/ad5696-i2c.c                  |   2 +-
>  drivers/iio/industrialio-buffer.c             | 128 ++++++++++++++-
>  include/linux/iio/buffer.h                    |   7 +
>  include/linux/iio/buffer_impl.h               |  11 ++
>  include/linux/iio/triggered_buffer.h          |  11 +-
>  16 files changed, 459 insertions(+), 17 deletions(-)
>