[PATCH v3 00/13] Miscellaneous IIO core enhancements

[PATCH v3 00/13] Miscellaneous IIO core enhancements

[Miquel Raynal]

Hello,

As part of a previous discussion with Jonathan (I know, it was three
months ago, sorry for the delay :-$), it appeared necessary to do a
little bit of cleanup in the IIO core and at least provide a little bit
more documentation for developers diving into the IIO subsystem for the
first time. My first move was to write a blog post trying to explain
(from my point of view but with the review of Jonathan) what is
necessary to understand when writing and IIO kernel driver:
https://bootlin.com/blog/the-backbone-of-a-linux-industrial-i-o-driver/

Here is now the second halve of this work, with few things that I tried
to address.

* The currentmode indio_dev entry was misused by several device drivers,
  I tried to understand which ones were wrong (there are two still
  accessing this variable left, I decided not to touch them because I
  think it is legitimate) and I tried to find an alternative.

* Then, I tried to make new accesses to this variable harder in order to
  avoid future mistakes, by creating a helper to read it from outside the
  core and moving it to the opaque structure.

* I continued with the IIO modes which are completely
  undocumented. Following a few exchanges with Jonathan, I tried to take
  most of this discussion and put it into a kernel doc header.

* When doing the above task, I realized that there was a particular case
  handled for kfifo buffers which I tried to simplify, but if this is a
  mistake, I'll just drop the patch.

Hope this will help!

Cheers,
Miquèl

Changes in v3:
* Added a patch to cleanup the _write_raw() callbacks in the st_sensor
  drivers. Returned as early as possible.
* Updated the description of INDIO_DIRECT_MODE and INDIO_BUFFER_SOFTWARE
  as discussed.
* Added the R-by from Fabrice.
* Extended the coverage of the odr lock in order to avoid relying on the
  iio_device_claim_direct_mode() helper implementation for that.

Changes in v2:
* Added Alexandru Reviewed-by when relevant.
* Indicated that "modes" is a bitmask as suggested.
* Added a mention in the "modes" kdoc about the function that could
  alter its content in the core.
* Added more driver maintainers in Cc.
* Avoided deleting the INDIO_BUFFER_TRIGGERED mode from kfifos.
* Dropped entirely the currentmode setter helper.
* Avoided using the currentmode getter from places where we have access
  to the opaque structure (ie. from the core itself).
* Added a patch to move the iio_buffer_enabled() definition in the core
  as it cannot be inlined anymore.
* Dropped completely the use of the mlock lock form the st-sensor
  drivers by inserting an additional local lock when this was
  needed. Hopefully I didn't fail that, this will need testing.
* Reworded the modes kdoc as advised by Jonathan.

Miquel Raynal (13):
  iio: core: Enhance the kernel doc of modes and currentmodes iio_dev
    entries
  iio: magnetometer: rm3100: Stop abusing the ->currentmode
  iio: adc: stm32-dfsdm: Avoid dereferencing ->currentmode
  iio: st_sensors: Return as early as possible from the _write_raw()
    callbacks
  iio: st_sensors: Drop the protection on _avail functions
  iio: st_sensors: Add a local lock for protecting odr
  iio: st_sensors: Stop abusing mlock to ensure internal coherency
  iio: st_sensors: Use iio_device_claim/release_direct_mode() when
    relevant
  iio: Un-inline iio_buffer_enabled()
  iio: core: Hide read accesses to iio_dev->currentmode
  iio: core: Move the currentmode entry to the opaque structure
  iio: core: Simplify the registration of kfifo buffers
  iio: core: Clarify the modes

 drivers/iio/accel/bmc150-accel-core.c         |  4 +-
 drivers/iio/accel/fxls8962af-core.c           |  1 -
 drivers/iio/accel/sca3000.c                   |  1 -
 drivers/iio/accel/ssp_accel_sensor.c          |  1 -
 drivers/iio/accel/st_accel_core.c             | 13 +---
 drivers/iio/adc/at91-sama5d2_adc.c            |  4 +-
 drivers/iio/adc/ina2xx-adc.c                  |  1 -
 drivers/iio/adc/stm32-dfsdm-adc.c             |  5 +-
 drivers/iio/adc/ti_am335x_adc.c               |  4 +-
 drivers/iio/buffer/kfifo_buf.c                | 10 +--
 .../cros_ec_sensors/cros_ec_sensors_core.c    |  3 +-
 drivers/iio/common/scmi_sensors/scmi_iio.c    |  1 -
 .../iio/common/st_sensors/st_sensors_core.c   | 50 +++++++------
 drivers/iio/gyro/ssp_gyro_sensor.c            |  1 -
 drivers/iio/gyro/st_gyro_core.c               | 15 ++--
 drivers/iio/health/max30100.c                 |  1 -
 drivers/iio/health/max30102.c                 |  1 -
 .../iio/imu/inv_icm42600/inv_icm42600_accel.c |  1 -
 .../iio/imu/inv_icm42600/inv_icm42600_gyro.c  |  1 -
 .../iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c    |  1 -
 drivers/iio/industrialio-buffer.c             | 12 ++--
 drivers/iio/industrialio-core.c               | 27 +++++++
 drivers/iio/industrialio-trigger.c            |  2 +-
 drivers/iio/light/apds9960.c                  |  1 -
 drivers/iio/magnetometer/rm3100-core.c        | 15 +---
 drivers/iio/magnetometer/st_magn_core.c       | 15 ++--
 drivers/iio/pressure/st_pressure_core.c       |  8 +--
 .../staging/iio/impedance-analyzer/ad5933.c   |  1 -
 include/linux/iio/common/st_sensors.h         |  3 +
 include/linux/iio/iio-opaque.h                |  4 ++
 include/linux/iio/iio.h                       | 70 +++++++++++++++----
 include/linux/iio/kfifo_buf.h                 |  5 +-
 32 files changed, 151 insertions(+), 131 deletions(-)

-- 
2.27.0

[PATCH v3 01/13] iio: core: Enhance the kernel doc of modes and currentmodes iio_dev entries

[Miquel Raynal]

Let's provide more details about these two variables because their
understanding may not be straightforward for someone not used to the IIO
subsystem internal logic. The different modes will soon be also be more
documented for the same reason.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 include/linux/iio/iio.h | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index faf00f2c0be6..f191b80466cd 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -488,8 +488,15 @@ struct iio_buffer_setup_ops {
 
 /**
  * struct iio_dev - industrial I/O device
- * @modes:		[DRIVER] operating modes supported by device
- * @currentmode:	[INTERN] current operating mode
+ * @modes:		[DRIVER] bitmask listing all the operating modes
+ *			supported by the IIO device. This list should be
+ *			initialized before registering the IIO device. It can
+ *			also be filed up by the IIO core, as a result of
+ *			enabling particular features in the driver
+ *			(see iio_triggered_event_setup()).
+ * @currentmode:	[INTERN] operating mode currently in use, may be
+ *			eventually checked by device drivers but should be
+ *			considered read-only as this is a core internal bit
  * @dev:		[DRIVER] device structure, should be assigned a parent
  *			and owner
  * @buffer:		[DRIVER] any buffer present
-- 
2.27.0

[PATCH v3 02/13] iio: magnetometer: rm3100: Stop abusing the ->currentmode

[Miquel Raynal]

This is an internal variable for the core, here it is set to a "default"
value by the driver in order to later be able to perform checks against
it. None of this is needed because this check actually cares about the
buffers being enabled or not. So it is an unproper side-channel access
to the information "are the buffers enabled?", returned officially by
the iio_buffer_enabled() helper. Use this helper instead.

Cc: Song Qiang <songqiang1304521@gmail.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/iio/magnetometer/rm3100-core.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/drivers/iio/magnetometer/rm3100-core.c b/drivers/iio/magnetometer/rm3100-core.c
index 13914273c999..be0057f82218 100644
--- a/drivers/iio/magnetometer/rm3100-core.c
+++ b/drivers/iio/magnetometer/rm3100-core.c
@@ -141,18 +141,10 @@ static irqreturn_t rm3100_irq_handler(int irq, void *d)
 	struct iio_dev *indio_dev = d;
 	struct rm3100_data *data = iio_priv(indio_dev);
 
-	switch (indio_dev->currentmode) {
-	case INDIO_DIRECT_MODE:
+	if (!iio_buffer_enabled(indio_dev))
 		complete(&data->measuring_done);
-		break;
-	case INDIO_BUFFER_TRIGGERED:
+	else
 		iio_trigger_poll(data->drdy_trig);
-		break;
-	default:
-		dev_err(indio_dev->dev.parent,
-			"device mode out of control, current mode: %d",
-			indio_dev->currentmode);
-	}
 
 	return IRQ_WAKE_THREAD;
 }
@@ -377,7 +369,7 @@ static int rm3100_set_samp_freq(struct iio_dev *indio_dev, int val, int val2)
 			goto unlock_return;
 	}
 
-	if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
+	if (iio_buffer_enabled(indio_dev)) {
 		/* Writing TMRC registers requires CMM reset. */
 		ret = regmap_write(regmap, RM3100_REG_CMM, 0);
 		if (ret < 0)
@@ -553,7 +545,6 @@ int rm3100_common_probe(struct device *dev, struct regmap *regmap, int irq)
 	indio_dev->channels = rm3100_channels;
 	indio_dev->num_channels = ARRAY_SIZE(rm3100_channels);
 	indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_TRIGGERED;
-	indio_dev->currentmode = INDIO_DIRECT_MODE;
 
 	if (!irq)
 		data->use_interrupt = false;
-- 
2.27.0

[PATCH v3 03/13] iio: adc: stm32-dfsdm: Avoid dereferencing ->currentmode

[Miquel Raynal]

This is an internal variable of the core, let's use the
iio_buffer_enabled() helper which is exported for the following purpose:
telling if the current mode is a buffered mode, which is precisely what
this driver looks for.

Cc: Olivier Moysan <olivier.moysan@foss.st.com>
Cc: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
---
 drivers/iio/adc/stm32-dfsdm-adc.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/adc/stm32-dfsdm-adc.c b/drivers/iio/adc/stm32-dfsdm-adc.c
index 1cfefb3b5e56..a3b8827d3bbf 100644
--- a/drivers/iio/adc/stm32-dfsdm-adc.c
+++ b/drivers/iio/adc/stm32-dfsdm-adc.c
@@ -466,8 +466,7 @@ static int stm32_dfsdm_channels_configure(struct iio_dev *indio_dev,
 	 * In continuous mode, use fast mode configuration,
 	 * if it provides a better resolution.
 	 */
-	if (adc->nconv == 1 && !trig &&
-	    (indio_dev->currentmode & INDIO_BUFFER_SOFTWARE)) {
+	if (adc->nconv == 1 && !trig && iio_buffer_enabled(indio_dev)) {
 		if (fl->flo[1].res >= fl->flo[0].res) {
 			fl->fast = 1;
 			flo = &fl->flo[1];
@@ -562,7 +561,7 @@ static int stm32_dfsdm_filter_configure(struct iio_dev *indio_dev,
 		cr1 = DFSDM_CR1_RCH(chan->channel);
 
 		/* Continuous conversions triggered by SPI clk in buffer mode */
-		if (indio_dev->currentmode & INDIO_BUFFER_SOFTWARE)
+		if (iio_buffer_enabled(indio_dev))
 			cr1 |= DFSDM_CR1_RCONT(1);
 
 		cr1 |= DFSDM_CR1_RSYNC(fl->sync_mode);
-- 
2.27.0

[PATCH v3 04/13] iio: st_sensors: Return as early as possible from the _write_raw() callbacks

[Miquel Raynal]

As there is no cleanup to do, let's return as early as possible in the
various ST sensor drivers _write_raw() callback functions.

There is no functional change.

Suggested-by: Jonathan Cameron <jic23@kernel.org>
Cc: Denis Ciocca <denis.ciocca@st.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/iio/accel/st_accel_core.c       | 5 +----
 drivers/iio/gyro/st_gyro_core.c         | 7 ++-----
 drivers/iio/magnetometer/st_magn_core.c | 7 ++-----
 3 files changed, 5 insertions(+), 14 deletions(-)

diff --git a/drivers/iio/accel/st_accel_core.c b/drivers/iio/accel/st_accel_core.c
index 31ea19d0ba71..ae0e6414e8f4 100644
--- a/drivers/iio/accel/st_accel_core.c
+++ b/drivers/iio/accel/st_accel_core.c
@@ -1133,8 +1133,7 @@ static int st_accel_write_raw(struct iio_dev *indio_dev,
 		int gain;
 
 		gain = val * 1000000 + val2;
-		err = st_sensors_set_fullscale_by_gain(indio_dev, gain);
-		break;
+		return st_sensors_set_fullscale_by_gain(indio_dev, gain);
 	}
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		if (val2)
@@ -1146,8 +1145,6 @@ static int st_accel_write_raw(struct iio_dev *indio_dev,
 	default:
 		return -EINVAL;
 	}
-
-	return err;
 }
 
 static ST_SENSORS_DEV_ATTR_SAMP_FREQ_AVAIL();
diff --git a/drivers/iio/gyro/st_gyro_core.c b/drivers/iio/gyro/st_gyro_core.c
index 201050b76fe5..f574ee7aca95 100644
--- a/drivers/iio/gyro/st_gyro_core.c
+++ b/drivers/iio/gyro/st_gyro_core.c
@@ -410,8 +410,7 @@ static int st_gyro_write_raw(struct iio_dev *indio_dev,
 
 	switch (mask) {
 	case IIO_CHAN_INFO_SCALE:
-		err = st_sensors_set_fullscale_by_gain(indio_dev, val2);
-		break;
+		return st_sensors_set_fullscale_by_gain(indio_dev, val2);
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		if (val2)
 			return -EINVAL;
@@ -420,10 +419,8 @@ static int st_gyro_write_raw(struct iio_dev *indio_dev,
 		mutex_unlock(&indio_dev->mlock);
 		return err;
 	default:
-		err = -EINVAL;
+		return -EINVAL;
 	}
-
-	return err;
 }
 
 static ST_SENSORS_DEV_ATTR_SAMP_FREQ_AVAIL();
diff --git a/drivers/iio/magnetometer/st_magn_core.c b/drivers/iio/magnetometer/st_magn_core.c
index 0806a1e65ce4..34a0503bc571 100644
--- a/drivers/iio/magnetometer/st_magn_core.c
+++ b/drivers/iio/magnetometer/st_magn_core.c
@@ -544,8 +544,7 @@ static int st_magn_write_raw(struct iio_dev *indio_dev,
 
 	switch (mask) {
 	case IIO_CHAN_INFO_SCALE:
-		err = st_sensors_set_fullscale_by_gain(indio_dev, val2);
-		break;
+		return st_sensors_set_fullscale_by_gain(indio_dev, val2);
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		if (val2)
 			return -EINVAL;
@@ -554,10 +553,8 @@ static int st_magn_write_raw(struct iio_dev *indio_dev,
 		mutex_unlock(&indio_dev->mlock);
 		return err;
 	default:
-		err = -EINVAL;
+		return -EINVAL;
 	}
-
-	return err;
 }
 
 static ST_SENSORS_DEV_ATTR_SAMP_FREQ_AVAIL();
-- 
2.27.0

[PATCH v3 05/13] iio: st_sensors: Drop the protection on _avail functions

[Miquel Raynal]

The use of a lock there seems pointless. Besides preventing to read
these information from userspace while buffers are enabled (which is not
supposed to happen), it only protect read accesses over static const
values, which are never supposed to be written anyway.

Drop these lock calls.

Suggested-by: Jonathan Cameron <jic23@kernel.org>
Cc: Denis Ciocca <denis.ciocca@st.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/iio/common/st_sensors/st_sensors_core.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/iio/common/st_sensors/st_sensors_core.c b/drivers/iio/common/st_sensors/st_sensors_core.c
index eb452d0c423c..6c027150a5a4 100644
--- a/drivers/iio/common/st_sensors/st_sensors_core.c
+++ b/drivers/iio/common/st_sensors/st_sensors_core.c
@@ -641,7 +641,6 @@ ssize_t st_sensors_sysfs_sampling_frequency_avail(struct device *dev,
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct st_sensor_data *sdata = iio_priv(indio_dev);
 
-	mutex_lock(&indio_dev->mlock);
 	for (i = 0; i < ST_SENSORS_ODR_LIST_MAX; i++) {
 		if (sdata->sensor_settings->odr.odr_avl[i].hz == 0)
 			break;
@@ -649,7 +648,6 @@ ssize_t st_sensors_sysfs_sampling_frequency_avail(struct device *dev,
 		len += scnprintf(buf + len, PAGE_SIZE - len, "%d ",
 				sdata->sensor_settings->odr.odr_avl[i].hz);
 	}
-	mutex_unlock(&indio_dev->mlock);
 	buf[len - 1] = '\n';
 
 	return len;
@@ -663,7 +661,6 @@ ssize_t st_sensors_sysfs_scale_avail(struct device *dev,
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct st_sensor_data *sdata = iio_priv(indio_dev);
 
-	mutex_lock(&indio_dev->mlock);
 	for (i = 0; i < ST_SENSORS_FULLSCALE_AVL_MAX; i++) {
 		if (sdata->sensor_settings->fs.fs_avl[i].num == 0)
 			break;
@@ -673,7 +670,6 @@ ssize_t st_sensors_sysfs_scale_avail(struct device *dev,
 
 		len += scnprintf(buf + len, PAGE_SIZE - len, "%u.%06u ", q, r);
 	}
-	mutex_unlock(&indio_dev->mlock);
 	buf[len - 1] = '\n';
 
 	return len;
-- 
2.27.0

[PATCH v3 06/13] iio: st_sensors: Add a local lock for protecting odr

[Miquel Raynal]

Right now the (framework) mlock lock is (ab)used for multiple purposes:
1- protecting concurrent accesses over the odr local cache
2- avoid changing samplig frequency whilst buffer is running

Let's start by handling situation #1 with a local lock.

Suggested-by: Jonathan Cameron <jic23@kernel.org>
Cc: Denis Ciocca <denis.ciocca@st.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 .../iio/common/st_sensors/st_sensors_core.c   | 24 ++++++++++++++-----
 include/linux/iio/common/st_sensors.h         |  3 +++
 2 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/common/st_sensors/st_sensors_core.c b/drivers/iio/common/st_sensors/st_sensors_core.c
index 6c027150a5a4..665584e8bea4 100644
--- a/drivers/iio/common/st_sensors/st_sensors_core.c
+++ b/drivers/iio/common/st_sensors/st_sensors_core.c
@@ -71,16 +71,18 @@ static int st_sensors_match_odr(struct st_sensor_settings *sensor_settings,
 
 int st_sensors_set_odr(struct iio_dev *indio_dev, unsigned int odr)
 {
-	int err;
+	int err = 0;
 	struct st_sensor_odr_avl odr_out = {0, 0};
 	struct st_sensor_data *sdata = iio_priv(indio_dev);
 
+	mutex_lock(&sdata->odr_lock);
+
 	if (!sdata->sensor_settings->odr.mask)
-		return 0;
+		goto unlock_mutex;
 
 	err = st_sensors_match_odr(sdata->sensor_settings, odr, &odr_out);
 	if (err < 0)
-		goto st_sensors_match_odr_error;
+		goto unlock_mutex;
 
 	if ((sdata->sensor_settings->odr.addr ==
 					sdata->sensor_settings->pw.addr) &&
@@ -103,7 +105,9 @@ int st_sensors_set_odr(struct iio_dev *indio_dev, unsigned int odr)
 	if (err >= 0)
 		sdata->odr = odr_out.hz;
 
-st_sensors_match_odr_error:
+unlock_mutex:
+	mutex_unlock(&sdata->odr_lock);
+
 	return err;
 }
 EXPORT_SYMBOL(st_sensors_set_odr);
@@ -361,6 +365,8 @@ int st_sensors_init_sensor(struct iio_dev *indio_dev,
 	struct st_sensors_platform_data *of_pdata;
 	int err = 0;
 
+	mutex_init(&sdata->odr_lock);
+
 	/* If OF/DT pdata exists, it will take precedence of anything else */
 	of_pdata = st_sensors_dev_probe(indio_dev->dev.parent, pdata);
 	if (IS_ERR(of_pdata))
@@ -554,18 +560,24 @@ int st_sensors_read_info_raw(struct iio_dev *indio_dev,
 		err = -EBUSY;
 		goto out;
 	} else {
+		mutex_lock(&sdata->odr_lock);
 		err = st_sensors_set_enable(indio_dev, true);
-		if (err < 0)
+		if (err < 0) {
+			mutex_unlock(&sdata->odr_lock);
 			goto out;
+		}
 
 		msleep((sdata->sensor_settings->bootime * 1000) / sdata->odr);
 		err = st_sensors_read_axis_data(indio_dev, ch, val);
-		if (err < 0)
+		if (err < 0) {
+			mutex_unlock(&sdata->odr_lock);
 			goto out;
+		}
 
 		*val = *val >> ch->scan_type.shift;
 
 		err = st_sensors_set_enable(indio_dev, false);
+		mutex_unlock(&sdata->odr_lock);
 	}
 out:
 	mutex_unlock(&indio_dev->mlock);
diff --git a/include/linux/iio/common/st_sensors.h b/include/linux/iio/common/st_sensors.h
index 22f67845cdd3..db4a1b260348 100644
--- a/include/linux/iio/common/st_sensors.h
+++ b/include/linux/iio/common/st_sensors.h
@@ -237,6 +237,7 @@ struct st_sensor_settings {
  * @hw_irq_trigger: if we're using the hardware interrupt on the sensor.
  * @hw_timestamp: Latest timestamp from the interrupt handler, when in use.
  * @buffer_data: Data used by buffer part.
+ * @odr_lock: Local lock for preventing concurrent ODR accesses/changes
  */
 struct st_sensor_data {
 	struct iio_trigger *trig;
@@ -261,6 +262,8 @@ struct st_sensor_data {
 	s64 hw_timestamp;
 
 	char buffer_data[ST_SENSORS_MAX_BUFFER_SIZE] ____cacheline_aligned;
+
+	struct mutex odr_lock;
 };
 
 #ifdef CONFIG_IIO_BUFFER
-- 
2.27.0

[PATCH v3 07/13] iio: st_sensors: Stop abusing mlock to ensure internal coherency

[Miquel Raynal]

An odr_lock has been introduced to protect local accesses to the odr
internal cache and ensure the cached value always reflected the actual
value. Using the mlock() for this purpose is no longer needed, so let's
drop these extra mutex_lock/unlock() calls.

Suggested-by: Jonathan Cameron <jic23@kernel.org>
Cc: Denis Ciocca <denis.ciocca@st.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/iio/accel/st_accel_core.c       | 8 ++------
 drivers/iio/gyro/st_gyro_core.c         | 8 ++------
 drivers/iio/magnetometer/st_magn_core.c | 8 ++------
 drivers/iio/pressure/st_pressure_core.c | 8 ++------
 4 files changed, 8 insertions(+), 24 deletions(-)

diff --git a/drivers/iio/accel/st_accel_core.c b/drivers/iio/accel/st_accel_core.c
index ae0e6414e8f4..2ac13b3f2f58 100644
--- a/drivers/iio/accel/st_accel_core.c
+++ b/drivers/iio/accel/st_accel_core.c
@@ -1126,8 +1126,6 @@ static int st_accel_read_raw(struct iio_dev *indio_dev,
 static int st_accel_write_raw(struct iio_dev *indio_dev,
 		struct iio_chan_spec const *chan, int val, int val2, long mask)
 {
-	int err;
-
 	switch (mask) {
 	case IIO_CHAN_INFO_SCALE: {
 		int gain;
@@ -1138,10 +1136,8 @@ static int st_accel_write_raw(struct iio_dev *indio_dev,
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		if (val2)
 			return -EINVAL;
-		mutex_lock(&indio_dev->mlock);
-		err = st_sensors_set_odr(indio_dev, val);
-		mutex_unlock(&indio_dev->mlock);
-		return err;
+
+		return st_sensors_set_odr(indio_dev, val);
 	default:
 		return -EINVAL;
 	}
diff --git a/drivers/iio/gyro/st_gyro_core.c b/drivers/iio/gyro/st_gyro_core.c
index f574ee7aca95..626f1f45dbcc 100644
--- a/drivers/iio/gyro/st_gyro_core.c
+++ b/drivers/iio/gyro/st_gyro_core.c
@@ -406,18 +406,14 @@ static int st_gyro_read_raw(struct iio_dev *indio_dev,
 static int st_gyro_write_raw(struct iio_dev *indio_dev,
 		struct iio_chan_spec const *chan, int val, int val2, long mask)
 {
-	int err;
-
 	switch (mask) {
 	case IIO_CHAN_INFO_SCALE:
 		return st_sensors_set_fullscale_by_gain(indio_dev, val2);
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		if (val2)
 			return -EINVAL;
-		mutex_lock(&indio_dev->mlock);
-		err = st_sensors_set_odr(indio_dev, val);
-		mutex_unlock(&indio_dev->mlock);
-		return err;
+
+		return st_sensors_set_odr(indio_dev, val);
 	default:
 		return -EINVAL;
 	}
diff --git a/drivers/iio/magnetometer/st_magn_core.c b/drivers/iio/magnetometer/st_magn_core.c
index 34a0503bc571..e54fb7f09544 100644
--- a/drivers/iio/magnetometer/st_magn_core.c
+++ b/drivers/iio/magnetometer/st_magn_core.c
@@ -540,18 +540,14 @@ static int st_magn_read_raw(struct iio_dev *indio_dev,
 static int st_magn_write_raw(struct iio_dev *indio_dev,
 		struct iio_chan_spec const *chan, int val, int val2, long mask)
 {
-	int err;
-
 	switch (mask) {
 	case IIO_CHAN_INFO_SCALE:
 		return st_sensors_set_fullscale_by_gain(indio_dev, val2);
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		if (val2)
 			return -EINVAL;
-		mutex_lock(&indio_dev->mlock);
-		err = st_sensors_set_odr(indio_dev, val);
-		mutex_unlock(&indio_dev->mlock);
-		return err;
+
+		return st_sensors_set_odr(indio_dev, val);
 	default:
 		return -EINVAL;
 	}
diff --git a/drivers/iio/pressure/st_pressure_core.c b/drivers/iio/pressure/st_pressure_core.c
index 26a1ee43d56e..05a909eeaff0 100644
--- a/drivers/iio/pressure/st_pressure_core.c
+++ b/drivers/iio/pressure/st_pressure_core.c
@@ -560,16 +560,12 @@ static int st_press_write_raw(struct iio_dev *indio_dev,
 			      int val2,
 			      long mask)
 {
-	int err;
-
 	switch (mask) {
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		if (val2)
 			return -EINVAL;
-		mutex_lock(&indio_dev->mlock);
-		err = st_sensors_set_odr(indio_dev, val);
-		mutex_unlock(&indio_dev->mlock);
-		return err;
+
+		return st_sensors_set_odr(indio_dev, val);
 	default:
 		return -EINVAL;
 	}
-- 
2.27.0

[PATCH v3 08/13] iio: st_sensors: Use iio_device_claim/release_direct_mode() when relevant

[Miquel Raynal]

The st_sensors_core driver hardcodes the content of the
iio_device_claim_direct_mode() and iio_device_release_direct_mode()
helpers. Let's get rid of this handcrafted implementation and use the
proper core helpers instead. Additionally, this lowers the tab level
(which is always good) and prevents the use of the ->currentmode
variable which is not supposed to be used like this anyway.

Cc: Denis Ciocca <denis.ciocca@st.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 .../iio/common/st_sensors/st_sensors_core.c   | 38 +++++++++----------
 1 file changed, 17 insertions(+), 21 deletions(-)

diff --git a/drivers/iio/common/st_sensors/st_sensors_core.c b/drivers/iio/common/st_sensors/st_sensors_core.c
index 665584e8bea4..543a7d88e317 100644
--- a/drivers/iio/common/st_sensors/st_sensors_core.c
+++ b/drivers/iio/common/st_sensors/st_sensors_core.c
@@ -555,32 +555,28 @@ int st_sensors_read_info_raw(struct iio_dev *indio_dev,
 	int err;
 	struct st_sensor_data *sdata = iio_priv(indio_dev);
 
-	mutex_lock(&indio_dev->mlock);
-	if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
-		err = -EBUSY;
+	err = iio_device_claim_direct_mode(indio_dev);
+	if (err)
+		return err;
+
+	mutex_lock(&sdata->odr_lock);
+
+	err = st_sensors_set_enable(indio_dev, true);
+	if (err < 0)
+		goto out;
+
+	msleep((sdata->sensor_settings->bootime * 1000) / sdata->odr);
+	err = st_sensors_read_axis_data(indio_dev, ch, val);
+	if (err < 0)
 		goto out;
-	} else {
-		mutex_lock(&sdata->odr_lock);
-		err = st_sensors_set_enable(indio_dev, true);
-		if (err < 0) {
-			mutex_unlock(&sdata->odr_lock);
-			goto out;
-		}
 
-		msleep((sdata->sensor_settings->bootime * 1000) / sdata->odr);
-		err = st_sensors_read_axis_data(indio_dev, ch, val);
-		if (err < 0) {
-			mutex_unlock(&sdata->odr_lock);
-			goto out;
-		}
+	*val = *val >> ch->scan_type.shift;
 
-		*val = *val >> ch->scan_type.shift;
+	err = st_sensors_set_enable(indio_dev, false);
 
-		err = st_sensors_set_enable(indio_dev, false);
-		mutex_unlock(&sdata->odr_lock);
-	}
 out:
-	mutex_unlock(&indio_dev->mlock);
+	mutex_unlock(&sdata->odr_lock);
+	iio_device_release_direct_mode(indio_dev);
 
 	return err;
 }
-- 
2.27.0

[PATCH v3 09/13] iio: Un-inline iio_buffer_enabled()

[Miquel Raynal]

As we are going to hide the currentmode inside the opaque structure,
this helper would soon need to call a non-inline function which would
simply drop the benefit of having the helper defined inline in a header.

One alternative is to move this helper in the core as there is no more
interest in defining it inline in a header. We will pay the minor cost
either way.

Let's do like the iio_device_id() helper which also refers to the opaque
structure and gets defined in the core.

Suggested-by: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/iio/industrialio-core.c | 12 ++++++++++++
 include/linux/iio/iio.h         | 11 +----------
 2 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 409c278a4c2c..64381d0a3d01 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -184,6 +184,18 @@ int iio_device_id(struct iio_dev *indio_dev)
 }
 EXPORT_SYMBOL_GPL(iio_device_id);
 
+/**
+ * iio_buffer_enabled() - helper function to test if the buffer is enabled
+ * @indio_dev:		IIO device structure for device
+ */
+bool iio_buffer_enabled(struct iio_dev *indio_dev)
+{
+	return indio_dev->currentmode
+		& (INDIO_BUFFER_TRIGGERED | INDIO_BUFFER_HARDWARE |
+		   INDIO_BUFFER_SOFTWARE);
+}
+EXPORT_SYMBOL_GPL(iio_buffer_enabled);
+
 /**
  * iio_sysfs_match_string_with_gaps - matches given string in an array with gaps
  * @array: array of strings
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index f191b80466cd..faabb852128a 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -550,6 +550,7 @@ struct iio_dev {
 };
 
 int iio_device_id(struct iio_dev *indio_dev);
+bool iio_buffer_enabled(struct iio_dev *indio_dev);
 
 const struct iio_chan_spec
 *iio_find_channel_from_si(struct iio_dev *indio_dev, int si);
@@ -679,16 +680,6 @@ struct iio_dev *devm_iio_device_alloc(struct device *parent, int sizeof_priv);
 __printf(2, 3)
 struct iio_trigger *devm_iio_trigger_alloc(struct device *parent,
 					   const char *fmt, ...);
-/**
- * iio_buffer_enabled() - helper function to test if the buffer is enabled
- * @indio_dev:		IIO device structure for device
- **/
-static inline bool iio_buffer_enabled(struct iio_dev *indio_dev)
-{
-	return indio_dev->currentmode
-		& (INDIO_BUFFER_TRIGGERED | INDIO_BUFFER_HARDWARE |
-		   INDIO_BUFFER_SOFTWARE);
-}
 
 /**
  * iio_get_debugfs_dentry() - helper function to get the debugfs_dentry
-- 
2.27.0

[PATCH v3 10/13] iio: core: Hide read accesses to iio_dev->currentmode

[Miquel Raynal]

In order to later move this variable within the opaque structure, let's
create a helper for accessing it in read-only mode. This helper will be
exposed to device drivers and kept accessible for the few that could need
it. The write access to this variable however should be fully reserved to
the core so in a second step we will hide this variable into the opaque
structure.

Cc: Eugen Hristev <eugen.hristev@microchip.com>
Cc: Nicolas Ferre <nicolas.ferre@microchip.com>
Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
Cc: Ludovic Desroches <ludovic.desroches@microchip.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/iio/accel/bmc150-accel-core.c |  4 ++--
 drivers/iio/adc/at91-sama5d2_adc.c    |  4 ++--
 drivers/iio/industrialio-core.c       | 11 +++++++++++
 include/linux/iio/iio.h               |  1 +
 4 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
index e6081dd0a880..0ecded6d87df 100644
--- a/drivers/iio/accel/bmc150-accel-core.c
+++ b/drivers/iio/accel/bmc150-accel-core.c
@@ -1525,7 +1525,7 @@ static int bmc150_accel_buffer_postenable(struct iio_dev *indio_dev)
 	struct bmc150_accel_data *data = iio_priv(indio_dev);
 	int ret = 0;
 
-	if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED)
+	if (iio_device_get_current_mode(indio_dev) == INDIO_BUFFER_TRIGGERED)
 		return 0;
 
 	mutex_lock(&data->mutex);
@@ -1557,7 +1557,7 @@ static int bmc150_accel_buffer_predisable(struct iio_dev *indio_dev)
 {
 	struct bmc150_accel_data *data = iio_priv(indio_dev);
 
-	if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED)
+	if (iio_device_get_current_mode(indio_dev) == INDIO_BUFFER_TRIGGERED)
 		return 0;
 
 	mutex_lock(&data->mutex);
diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index 854b1f81d807..b764823ce57e 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -1117,7 +1117,7 @@ static int at91_adc_buffer_prepare(struct iio_dev *indio_dev)
 		return at91_adc_configure_touch(st, true);
 
 	/* if we are not in triggered mode, we cannot enable the buffer. */
-	if (!(indio_dev->currentmode & INDIO_ALL_TRIGGERED_MODES))
+	if (!(iio_device_get_current_mode(indio_dev) & INDIO_ALL_TRIGGERED_MODES))
 		return -EINVAL;
 
 	/* we continue with the triggered buffer */
@@ -1159,7 +1159,7 @@ static int at91_adc_buffer_postdisable(struct iio_dev *indio_dev)
 		return at91_adc_configure_touch(st, false);
 
 	/* if we are not in triggered mode, nothing to do here */
-	if (!(indio_dev->currentmode & INDIO_ALL_TRIGGERED_MODES))
+	if (!(iio_device_get_current_mode(indio_dev) & INDIO_ALL_TRIGGERED_MODES))
 		return -EINVAL;
 
 	/*
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 64381d0a3d01..f8fcfe12dfa2 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -2071,6 +2071,17 @@ void iio_device_release_direct_mode(struct iio_dev *indio_dev)
 }
 EXPORT_SYMBOL_GPL(iio_device_release_direct_mode);
 
+/**
+ * iio_device_get_current_mode() - helper function providing read-only access to
+ *				   the @currentmode variable
+ * @indio_dev:			   IIO device structure for device
+ */
+int iio_device_get_current_mode(struct iio_dev *indio_dev)
+{
+	return indio_dev->currentmode;
+}
+EXPORT_SYMBOL_GPL(iio_device_get_current_mode);
+
 subsys_initcall(iio_init);
 module_exit(iio_exit);
 
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index faabb852128a..31098ffa7dc9 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -550,6 +550,7 @@ struct iio_dev {
 };
 
 int iio_device_id(struct iio_dev *indio_dev);
+int iio_device_get_current_mode(struct iio_dev *indio_dev);
 bool iio_buffer_enabled(struct iio_dev *indio_dev);
 
 const struct iio_chan_spec
-- 
2.27.0

[PATCH v3 11/13] iio: core: Move the currentmode entry to the opaque structure

[Miquel Raynal]

This entry should, under no situation, be modified by device
drivers. Now that we have limited its read access to device drivers
really needing it and did so through a dedicated helper, we can
easily move this variable to the opaque structure in order to
prevent any further modification from non-authorized code (out of the
core, basically).

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Alexandru Ardelean <ardeleanalex@gmail.com>
---
 drivers/iio/industrialio-buffer.c  | 12 ++++++------
 drivers/iio/industrialio-core.c    | 10 +++++++---
 drivers/iio/industrialio-trigger.c |  2 +-
 include/linux/iio/iio-opaque.h     |  4 ++++
 include/linux/iio/iio.h            |  4 ----
 5 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 94eb9f6cf128..2a5a09693b08 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -1065,7 +1065,7 @@ static int iio_enable_buffers(struct iio_dev *indio_dev,
 	indio_dev->active_scan_mask = config->scan_mask;
 	indio_dev->scan_timestamp = config->scan_timestamp;
 	indio_dev->scan_bytes = config->scan_bytes;
-	indio_dev->currentmode = config->mode;
+	iio_dev_opaque->currentmode = config->mode;
 
 	iio_update_demux(indio_dev);
 
@@ -1101,7 +1101,7 @@ static int iio_enable_buffers(struct iio_dev *indio_dev,
 			goto err_disable_buffers;
 	}
 
-	if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
+	if (iio_dev_opaque->currentmode == INDIO_BUFFER_TRIGGERED) {
 		ret = iio_trigger_attach_poll_func(indio_dev->trig,
 						   indio_dev->pollfunc);
 		if (ret)
@@ -1120,7 +1120,7 @@ static int iio_enable_buffers(struct iio_dev *indio_dev,
 	return 0;
 
 err_detach_pollfunc:
-	if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
+	if (iio_dev_opaque->currentmode == INDIO_BUFFER_TRIGGERED) {
 		iio_trigger_detach_poll_func(indio_dev->trig,
 					     indio_dev->pollfunc);
 	}
@@ -1132,7 +1132,7 @@ static int iio_enable_buffers(struct iio_dev *indio_dev,
 	if (indio_dev->setup_ops->postdisable)
 		indio_dev->setup_ops->postdisable(indio_dev);
 err_undo_config:
-	indio_dev->currentmode = INDIO_DIRECT_MODE;
+	iio_dev_opaque->currentmode = INDIO_DIRECT_MODE;
 	indio_dev->active_scan_mask = NULL;
 
 	return ret;
@@ -1162,7 +1162,7 @@ static int iio_disable_buffers(struct iio_dev *indio_dev)
 			ret = ret2;
 	}
 
-	if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
+	if (iio_dev_opaque->currentmode == INDIO_BUFFER_TRIGGERED) {
 		iio_trigger_detach_poll_func(indio_dev->trig,
 					     indio_dev->pollfunc);
 	}
@@ -1181,7 +1181,7 @@ static int iio_disable_buffers(struct iio_dev *indio_dev)
 
 	iio_free_scan_mask(indio_dev, indio_dev->active_scan_mask);
 	indio_dev->active_scan_mask = NULL;
-	indio_dev->currentmode = INDIO_DIRECT_MODE;
+	iio_dev_opaque->currentmode = INDIO_DIRECT_MODE;
 
 	return ret;
 }
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index f8fcfe12dfa2..b757273e6652 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -190,7 +190,9 @@ EXPORT_SYMBOL_GPL(iio_device_id);
  */
 bool iio_buffer_enabled(struct iio_dev *indio_dev)
 {
-	return indio_dev->currentmode
+	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
+
+	return iio_dev_opaque->currentmode
 		& (INDIO_BUFFER_TRIGGERED | INDIO_BUFFER_HARDWARE |
 		   INDIO_BUFFER_SOFTWARE);
 }
@@ -2073,12 +2075,14 @@ EXPORT_SYMBOL_GPL(iio_device_release_direct_mode);
 
 /**
  * iio_device_get_current_mode() - helper function providing read-only access to
- *				   the @currentmode variable
+ *				   the opaque @currentmode variable
  * @indio_dev:			   IIO device structure for device
  */
 int iio_device_get_current_mode(struct iio_dev *indio_dev)
 {
-	return indio_dev->currentmode;
+	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
+
+	return iio_dev_opaque->currentmode;
 }
 EXPORT_SYMBOL_GPL(iio_device_get_current_mode);
 
diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
index f504ed351b3e..585b6cef8fcc 100644
--- a/drivers/iio/industrialio-trigger.c
+++ b/drivers/iio/industrialio-trigger.c
@@ -444,7 +444,7 @@ static ssize_t iio_trigger_write_current(struct device *dev,
 	int ret;
 
 	mutex_lock(&indio_dev->mlock);
-	if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
+	if (iio_dev_opaque->currentmode == INDIO_BUFFER_TRIGGERED) {
 		mutex_unlock(&indio_dev->mlock);
 		return -EBUSY;
 	}
diff --git a/include/linux/iio/iio-opaque.h b/include/linux/iio/iio-opaque.h
index 2be12b7b5dc5..6b3586b3f952 100644
--- a/include/linux/iio/iio-opaque.h
+++ b/include/linux/iio/iio-opaque.h
@@ -7,6 +7,9 @@
  * struct iio_dev_opaque - industrial I/O device opaque information
  * @indio_dev:			public industrial I/O device information
  * @id:			used to identify device internally
+ * @currentmode:		operating mode currently in use, may be eventually
+ *				checked by device drivers but should be considered
+ *				read-only as this is a core internal bit
  * @driver_module:		used to make it harder to undercut users
  * @info_exist_lock:		lock to prevent use during removal
  * @trig_readonly:		mark the current trigger immutable
@@ -36,6 +39,7 @@
  */
 struct iio_dev_opaque {
 	struct iio_dev			indio_dev;
+	int				currentmode;
 	int				id;
 	struct module			*driver_module;
 	struct mutex			info_exist_lock;
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index 31098ffa7dc9..85cb924debd9 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -494,9 +494,6 @@ struct iio_buffer_setup_ops {
  *			also be filed up by the IIO core, as a result of
  *			enabling particular features in the driver
  *			(see iio_triggered_event_setup()).
- * @currentmode:	[INTERN] operating mode currently in use, may be
- *			eventually checked by device drivers but should be
- *			considered read-only as this is a core internal bit
  * @dev:		[DRIVER] device structure, should be assigned a parent
  *			and owner
  * @buffer:		[DRIVER] any buffer present
@@ -523,7 +520,6 @@ struct iio_buffer_setup_ops {
  */
 struct iio_dev {
 	int				modes;
-	int				currentmode;
 	struct device			dev;
 
 	struct iio_buffer		*buffer;
-- 
2.27.0

[PATCH v3 12/13] iio: core: Simplify the registration of kfifo buffers

[Miquel Raynal]

Among all the users of the kfifo buffers, no one uses the
INDIO_BUFFER_HARDWARE mode. So let's take this as a general rule and
simplify a little bit the internals - overall the documentation - by
eliminating unused specific cases. Use the INDIO_BUFFER_SOFTWARE mode by
default with kfifo buffers, which will basically mimic what all the "non
direct" modes do.

Cc: Benson Leung <bleung@chromium.org>
Cc: Guenter Roeck <groeck@chromium.org>
Cc: Jyoti Bhayana <jbhayana@google.com>
Cc: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
Cc: Lorenzo Bianconi <lorenzo.bianconi83@gmail.com>
Cc: Michael Hennerich <Michael.Hennerich@analog.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/iio/accel/fxls8962af-core.c                    |  1 -
 drivers/iio/accel/sca3000.c                            |  1 -
 drivers/iio/accel/ssp_accel_sensor.c                   |  1 -
 drivers/iio/adc/ina2xx-adc.c                           |  1 -
 drivers/iio/adc/ti_am335x_adc.c                        |  4 +---
 drivers/iio/buffer/kfifo_buf.c                         | 10 +---------
 .../iio/common/cros_ec_sensors/cros_ec_sensors_core.c  |  3 +--
 drivers/iio/common/scmi_sensors/scmi_iio.c             |  1 -
 drivers/iio/gyro/ssp_gyro_sensor.c                     |  1 -
 drivers/iio/health/max30100.c                          |  1 -
 drivers/iio/health/max30102.c                          |  1 -
 drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c      |  1 -
 drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c       |  1 -
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c         |  1 -
 drivers/iio/light/apds9960.c                           |  1 -
 drivers/staging/iio/impedance-analyzer/ad5933.c        |  1 -
 include/linux/iio/kfifo_buf.h                          |  5 ++---
 17 files changed, 5 insertions(+), 30 deletions(-)

diff --git a/drivers/iio/accel/fxls8962af-core.c b/drivers/iio/accel/fxls8962af-core.c
index 32989d91b982..cf494937a346 100644
--- a/drivers/iio/accel/fxls8962af-core.c
+++ b/drivers/iio/accel/fxls8962af-core.c
@@ -1209,7 +1209,6 @@ int fxls8962af_core_probe(struct device *dev, struct regmap *regmap, int irq)
 			return ret;
 
 		ret = devm_iio_kfifo_buffer_setup(dev, indio_dev,
-						  INDIO_BUFFER_SOFTWARE,
 						  &fxls8962af_buffer_ops);
 		if (ret)
 			return ret;
diff --git a/drivers/iio/accel/sca3000.c b/drivers/iio/accel/sca3000.c
index 43ecacbdc95a..13b467c74636 100644
--- a/drivers/iio/accel/sca3000.c
+++ b/drivers/iio/accel/sca3000.c
@@ -1474,7 +1474,6 @@ static int sca3000_probe(struct spi_device *spi)
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
 	ret = devm_iio_kfifo_buffer_setup(&spi->dev, indio_dev,
-					  INDIO_BUFFER_SOFTWARE,
 					  &sca3000_ring_setup_ops);
 	if (ret)
 		return ret;
diff --git a/drivers/iio/accel/ssp_accel_sensor.c b/drivers/iio/accel/ssp_accel_sensor.c
index 04dcb2b657ee..d20f8d299c11 100644
--- a/drivers/iio/accel/ssp_accel_sensor.c
+++ b/drivers/iio/accel/ssp_accel_sensor.c
@@ -113,7 +113,6 @@ static int ssp_accel_probe(struct platform_device *pdev)
 	indio_dev->available_scan_masks = ssp_accel_scan_mask;
 
 	ret = devm_iio_kfifo_buffer_setup(&pdev->dev, indio_dev,
-					  INDIO_BUFFER_SOFTWARE,
 					  &ssp_accel_buffer_ops);
 	if (ret)
 		return ret;
diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index 4f9992a51e64..05fdfd06e3b0 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -1027,7 +1027,6 @@ static int ina2xx_probe(struct i2c_client *client,
 	indio_dev->name = id->name;
 
 	ret = devm_iio_kfifo_buffer_setup(&client->dev, indio_dev,
-					  INDIO_BUFFER_SOFTWARE,
 					  &ina2xx_setup_ops);
 	if (ret)
 		return ret;
diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index dbdc1ef48566..567d43a30955 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -376,9 +376,7 @@ static int tiadc_iio_buffered_hardware_setup(struct device *dev,
 {
 	int ret;
 
-	ret = devm_iio_kfifo_buffer_setup(dev, indio_dev,
-					  INDIO_BUFFER_SOFTWARE,
-					  setup_ops);
+	ret = devm_iio_kfifo_buffer_setup(dev, indio_dev, setup_ops);
 	if (ret)
 		return ret;
 
diff --git a/drivers/iio/buffer/kfifo_buf.c b/drivers/iio/buffer/kfifo_buf.c
index 416d35a61ae2..35d8b4077376 100644
--- a/drivers/iio/buffer/kfifo_buf.c
+++ b/drivers/iio/buffer/kfifo_buf.c
@@ -259,8 +259,6 @@ static struct iio_buffer *devm_iio_kfifo_allocate(struct device *dev)
  * devm_iio_kfifo_buffer_setup_ext - Allocate a kfifo buffer & attach it to an IIO device
  * @dev: Device object to which to attach the life-time of this kfifo buffer
  * @indio_dev: The device the buffer should be attached to
- * @mode_flags: The mode flags for this buffer (INDIO_BUFFER_SOFTWARE and/or
- *		INDIO_BUFFER_TRIGGERED).
  * @setup_ops: The setup_ops required to configure the HW part of the buffer (optional)
  * @buffer_attrs: Extra sysfs buffer attributes for this IIO buffer
  *
@@ -271,22 +269,16 @@ static struct iio_buffer *devm_iio_kfifo_allocate(struct device *dev)
  */
 int devm_iio_kfifo_buffer_setup_ext(struct device *dev,
 				    struct iio_dev *indio_dev,
-				    int mode_flags,
 				    const struct iio_buffer_setup_ops *setup_ops,
 				    const struct attribute **buffer_attrs)
 {
 	struct iio_buffer *buffer;
 
-	if (!mode_flags)
-		return -EINVAL;
-
 	buffer = devm_iio_kfifo_allocate(dev);
 	if (!buffer)
 		return -ENOMEM;
 
-	mode_flags &= kfifo_access_funcs.modes;
-
-	indio_dev->modes |= mode_flags;
+	indio_dev->modes |= INDIO_BUFFER_SOFTWARE;
 	indio_dev->setup_ops = setup_ops;
 
 	buffer->attrs = buffer_attrs;
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 b2725c6adc7f..a4cf1d9a8a49 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
@@ -333,8 +333,7 @@ int cros_ec_sensors_core_init(struct platform_device *pdev,
 			 * We can not use trigger here, as events are generated
 			 * as soon as sample_frequency is set.
 			 */
-			ret = devm_iio_kfifo_buffer_setup_ext(dev, indio_dev,
-							      INDIO_BUFFER_SOFTWARE, NULL,
+			ret = devm_iio_kfifo_buffer_setup_ext(dev, indio_dev, NULL,
 							      cros_ec_sensor_fifo_attributes);
 			if (ret)
 				return ret;
diff --git a/drivers/iio/common/scmi_sensors/scmi_iio.c b/drivers/iio/common/scmi_sensors/scmi_iio.c
index d538bf3ab1ef..793d628db55f 100644
--- a/drivers/iio/common/scmi_sensors/scmi_iio.c
+++ b/drivers/iio/common/scmi_sensors/scmi_iio.c
@@ -686,7 +686,6 @@ static int scmi_iio_dev_probe(struct scmi_device *sdev)
 
 		err = devm_iio_kfifo_buffer_setup(&scmi_iio_dev->dev,
 						  scmi_iio_dev,
-						  INDIO_BUFFER_SOFTWARE,
 						  &scmi_iio_buffer_ops);
 		if (err < 0) {
 			dev_err(dev,
diff --git a/drivers/iio/gyro/ssp_gyro_sensor.c b/drivers/iio/gyro/ssp_gyro_sensor.c
index 46ed12771d2f..eebc036717af 100644
--- a/drivers/iio/gyro/ssp_gyro_sensor.c
+++ b/drivers/iio/gyro/ssp_gyro_sensor.c
@@ -113,7 +113,6 @@ static int ssp_gyro_probe(struct platform_device *pdev)
 	indio_dev->available_scan_masks = ssp_gyro_scan_mask;
 
 	ret = devm_iio_kfifo_buffer_setup(&pdev->dev, indio_dev,
-					  INDIO_BUFFER_SOFTWARE,
 					  &ssp_gyro_buffer_ops);
 	if (ret)
 		return ret;
diff --git a/drivers/iio/health/max30100.c b/drivers/iio/health/max30100.c
index 36ba7611d9ce..ad5717965223 100644
--- a/drivers/iio/health/max30100.c
+++ b/drivers/iio/health/max30100.c
@@ -433,7 +433,6 @@ static int max30100_probe(struct i2c_client *client,
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
 	ret = devm_iio_kfifo_buffer_setup(&client->dev, indio_dev,
-					  INDIO_BUFFER_SOFTWARE,
 					  &max30100_buffer_setup_ops);
 	if (ret)
 		return ret;
diff --git a/drivers/iio/health/max30102.c b/drivers/iio/health/max30102.c
index 2292876c55e2..abbcef563807 100644
--- a/drivers/iio/health/max30102.c
+++ b/drivers/iio/health/max30102.c
@@ -542,7 +542,6 @@ static int max30102_probe(struct i2c_client *client,
 	}
 
 	ret = devm_iio_kfifo_buffer_setup(&client->dev, indio_dev,
-					  INDIO_BUFFER_SOFTWARE,
 					  &max30102_buffer_setup_ops);
 	if (ret)
 		return ret;
diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
index 383cc3250342..c3f433ad3af6 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
@@ -731,7 +731,6 @@ struct iio_dev *inv_icm42600_accel_init(struct inv_icm42600_state *st)
 	indio_dev->available_scan_masks = inv_icm42600_accel_scan_masks;
 
 	ret = devm_iio_kfifo_buffer_setup(dev, indio_dev,
-					  INDIO_BUFFER_SOFTWARE,
 					  &inv_icm42600_buffer_ops);
 	if (ret)
 		return ERR_PTR(ret);
diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
index cec1dd0e0464..9d94a8518e3c 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
@@ -743,7 +743,6 @@ struct iio_dev *inv_icm42600_gyro_init(struct inv_icm42600_state *st)
 	indio_dev->setup_ops = &inv_icm42600_buffer_ops;
 
 	ret = devm_iio_kfifo_buffer_setup(dev, indio_dev,
-					  INDIO_BUFFER_SOFTWARE,
 					  &inv_icm42600_buffer_ops);
 	if (ret)
 		return ERR_PTR(ret);
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
index 16730a780964..f80c62849d30 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
@@ -746,7 +746,6 @@ int st_lsm6dsx_fifo_setup(struct st_lsm6dsx_hw *hw)
 			continue;
 
 		ret = devm_iio_kfifo_buffer_setup(hw->dev, hw->iio_devs[i],
-						  INDIO_BUFFER_SOFTWARE,
 						  &st_lsm6dsx_buffer_ops);
 		if (ret)
 			return ret;
diff --git a/drivers/iio/light/apds9960.c b/drivers/iio/light/apds9960.c
index 4141c0fa7bc4..09b831f9f40b 100644
--- a/drivers/iio/light/apds9960.c
+++ b/drivers/iio/light/apds9960.c
@@ -1003,7 +1003,6 @@ static int apds9960_probe(struct i2c_client *client,
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
 	ret = devm_iio_kfifo_buffer_setup(&client->dev, indio_dev,
-					  INDIO_BUFFER_SOFTWARE,
 					  &apds9960_buffer_setup_ops);
 	if (ret)
 		return ret;
diff --git a/drivers/staging/iio/impedance-analyzer/ad5933.c b/drivers/staging/iio/impedance-analyzer/ad5933.c
index 793918e1c45f..f177b20f0f2d 100644
--- a/drivers/staging/iio/impedance-analyzer/ad5933.c
+++ b/drivers/staging/iio/impedance-analyzer/ad5933.c
@@ -749,7 +749,6 @@ static int ad5933_probe(struct i2c_client *client,
 	indio_dev->num_channels = ARRAY_SIZE(ad5933_channels);
 
 	ret = devm_iio_kfifo_buffer_setup(&client->dev, indio_dev,
-					  INDIO_BUFFER_SOFTWARE,
 					  &ad5933_ring_setup_ops);
 	if (ret)
 		return ret;
diff --git a/include/linux/iio/kfifo_buf.h b/include/linux/iio/kfifo_buf.h
index ccd2ceae7b25..8a83fb58232d 100644
--- a/include/linux/iio/kfifo_buf.h
+++ b/include/linux/iio/kfifo_buf.h
@@ -12,11 +12,10 @@ void iio_kfifo_free(struct iio_buffer *r);
 
 int devm_iio_kfifo_buffer_setup_ext(struct device *dev,
 				    struct iio_dev *indio_dev,
-				    int mode_flags,
 				    const struct iio_buffer_setup_ops *setup_ops,
 				    const struct attribute **buffer_attrs);
 
-#define devm_iio_kfifo_buffer_setup(dev, indio_dev, mode_flags, setup_ops)	\
-	devm_iio_kfifo_buffer_setup_ext((dev), (indio_dev), (mode_flags), (setup_ops), NULL)
+#define devm_iio_kfifo_buffer_setup(dev, indio_dev, setup_ops)	\
+	devm_iio_kfifo_buffer_setup_ext((dev), (indio_dev), (setup_ops), NULL)
 
 #endif
-- 
2.27.0

[PATCH v3 13/13] iio: core: Clarify the modes

[Miquel Raynal]

As part of a previous discussion with Jonathan Cameron [1], it appeared
necessary to clarify the meaning of each mode so that new developers
could understand better what they should use or not use and when.

The idea of renaming these modes as been let aside because naming is a
big deal and requires a lot of thinking. So for now let's focus on
correctly explaining what each mode implies.

[1] https://lore.kernel.org/linux-iio/20210930165510.2295e6c4@jic23-huawei/

Suggested-by: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 include/linux/iio/iio.h | 49 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 48 insertions(+), 1 deletion(-)

diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index 85cb924debd9..e383b0d96035 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -315,7 +315,54 @@ static inline bool iio_channel_has_available(const struct iio_chan_spec *chan,
 s64 iio_get_time_ns(const struct iio_dev *indio_dev);
 unsigned int iio_get_time_res(const struct iio_dev *indio_dev);
 
-/* Device operating modes */
+/**
+ * Device operating modes
+ * @INDIO_DIRECT_MODE: There is an access to either:
+ * a) The last single value available for devices that do not provide
+ *    on-demand reads.
+ * b) A new value after performing an on-demand read otherwise.
+ * On most devices, this is a single-shot read. On some devices with data
+ * streams without an 'on-demand' function, this might also be the 'last value'
+ * feature. Above all, this mode internally means that we are not in any of the
+ * other modes, and sysfs reads should work.
+ * Device drivers should inform the core if they support this mode.
+ * @INDIO_BUFFER_TRIGGERED: Common mode when dealing with kfifo buffers.
+ * It indicates that an explicit trigger is required. This requests the core to
+ * attach a poll function when enabling the buffer, which is indicated by the
+ * _TRIGGERED suffix.
+ * The core will ensure this mode is set when registering a triggered buffer
+ * with iio_triggered_buffer_setup().
+ * @INDIO_BUFFER_SOFTWARE: Another kfifo buffer mode, but not event triggered.
+ * No poll function can be attached because there is no triggered infrastructure
+ * we can use to cause capture. There is a kfifo that the driver will fill, but
+ * not "only one scan at a time". Typically, hardware will have a buffer that
+ * can hold multiple scans. Software may read one or more scans at a single time
+ * and push the available data to a Kfifo. This means the core will not attach
+ * any poll function when enabling the buffer.
+ * The core will ensure this mode is set when registering a simple kfifo buffer
+ * with devm_iio_kfifo_buffer_setup().
+ * @INDIO_BUFFER_HARDWARE: For specific hardware, if unsure do not use this mode.
+ * Same as above but this time the buffer is not a kfifo where we have direct
+ * access to the data. Instead, the consumer driver must access the data through
+ * non software visible channels (or DMA when there is no demux possible in
+ * software)
+ * The core will ensure this mode is set when registering a dmaengine buffer
+ * with devm_iio_dmaengine_buffer_setup().
+ * @INDIO_EVENT_TRIGGERED: Very unusual mode.
+ * Triggers usually refer to an external event which will start data capture.
+ * Here it is kind of the opposite as, a particular state of the data might
+ * produce an event which can be considered as an event. We don't necessarily
+ * have access to the data itself, but to the event produced. For example, this
+ * can be a threshold detector. The internal path of this mode is very close to
+ * the INDIO_BUFFER_TRIGGERED mode.
+ * The core will ensure this mode is set when registering a triggered event.
+ * @INDIO_HARDWARE_TRIGGERED: Very unusual mode.
+ * Here, triggers can result in data capture and can be routed to multiple
+ * hardware components, which make them close to regular triggers in the way
+ * they must be managed by the core, but without the entire interrupts/poll
+ * functions burden. Interrupts are irrelevant as the data flow is hardware
+ * mediated and distributed.
+ */
 #define INDIO_DIRECT_MODE		0x01
 #define INDIO_BUFFER_TRIGGERED		0x02
 #define INDIO_BUFFER_SOFTWARE		0x04
-- 
2.27.0

Re: [PATCH v3 13/13] iio: core: Clarify the modes

[Jonathan Cameron]

On Mon,  7 Feb 2022 15:38:40 +0100
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> As part of a previous discussion with Jonathan Cameron [1], it appeared
> necessary to clarify the meaning of each mode so that new developers
> could understand better what they should use or not use and when.
> 
> The idea of renaming these modes as been let aside because naming is a
> big deal and requires a lot of thinking. So for now let's focus on
> correctly explaining what each mode implies.
> 
> [1] https://lore.kernel.org/linux-iio/20210930165510.2295e6c4@jic23-huawei/
> 
> Suggested-by: Jonathan Cameron <jic23@kernel.org>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
One trivial thing inline as a result of edits in v3.

Otherwise, I want to let this series sit a little longer and ideally get
some eyes on the st_sensors patches.

Jonathan

> ---
>  include/linux/iio/iio.h | 49 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 48 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index 85cb924debd9..e383b0d96035 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -315,7 +315,54 @@ static inline bool iio_channel_has_available(const struct iio_chan_spec *chan,
>  s64 iio_get_time_ns(const struct iio_dev *indio_dev);
>  unsigned int iio_get_time_res(const struct iio_dev *indio_dev);
>  
> -/* Device operating modes */
> +/**
> + * Device operating modes
> + * @INDIO_DIRECT_MODE: There is an access to either:
> + * a) The last single value available for devices that do not provide
> + *    on-demand reads.
> + * b) A new value after performing an on-demand read otherwise.


> + * On most devices, this is a single-shot read. On some devices with data
> + * streams without an 'on-demand' function, this might also be the 'last value'
> + * feature.

This block duplicates what you now have as a/b above. I can drop it whilst
applying if nothing else comes up.

>  Above all, this mode internally means that we are not in any of the
> + * other modes, and sysfs reads should work.
> + * Device drivers should inform the core if they support this mode.
> + * @INDIO_BUFFER_TRIGGERED: Common mode when dealing with kfifo buffers.
> + * It indicates that an explicit trigger is required. This requests the core to
> + * attach a poll function when enabling the buffer, which is indicated by the
> + * _TRIGGERED suffix.
> + * The core will ensure this mode is set when registering a triggered buffer
> + * with iio_triggered_buffer_setup().
> + * @INDIO_BUFFER_SOFTWARE: Another kfifo buffer mode, but not event triggered.
> + * No poll function can be attached because there is no triggered infrastructure
> + * we can use to cause capture. There is a kfifo that the driver will fill, but
> + * not "only one scan at a time". Typically, hardware will have a buffer that
> + * can hold multiple scans. Software may read one or more scans at a single time
> + * and push the available data to a Kfifo. This means the core will not attach
> + * any poll function when enabling the buffer.
> + * The core will ensure this mode is set when registering a simple kfifo buffer
> + * with devm_iio_kfifo_buffer_setup().
> + * @INDIO_BUFFER_HARDWARE: For specific hardware, if unsure do not use this mode.
> + * Same as above but this time the buffer is not a kfifo where we have direct
> + * access to the data. Instead, the consumer driver must access the data through
> + * non software visible channels (or DMA when there is no demux possible in
> + * software)
> + * The core will ensure this mode is set when registering a dmaengine buffer
> + * with devm_iio_dmaengine_buffer_setup().
> + * @INDIO_EVENT_TRIGGERED: Very unusual mode.
> + * Triggers usually refer to an external event which will start data capture.
> + * Here it is kind of the opposite as, a particular state of the data might
> + * produce an event which can be considered as an event. We don't necessarily
> + * have access to the data itself, but to the event produced. For example, this
> + * can be a threshold detector. The internal path of this mode is very close to
> + * the INDIO_BUFFER_TRIGGERED mode.
> + * The core will ensure this mode is set when registering a triggered event.
> + * @INDIO_HARDWARE_TRIGGERED: Very unusual mode.
> + * Here, triggers can result in data capture and can be routed to multiple
> + * hardware components, which make them close to regular triggers in the way
> + * they must be managed by the core, but without the entire interrupts/poll
> + * functions burden. Interrupts are irrelevant as the data flow is hardware
> + * mediated and distributed.
> + */
>  #define INDIO_DIRECT_MODE		0x01
>  #define INDIO_BUFFER_TRIGGERED		0x02
>  #define INDIO_BUFFER_SOFTWARE		0x04

Re: [PATCH v3 13/13] iio: core: Clarify the modes

[Miquel Raynal]

Hi Jonathan,

jic23@kernel.org wrote on Sun, 13 Feb 2022 18:42:24 +0000:

> On Mon,  7 Feb 2022 15:38:40 +0100
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > As part of a previous discussion with Jonathan Cameron [1], it appeared
> > necessary to clarify the meaning of each mode so that new developers
> > could understand better what they should use or not use and when.
> > 
> > The idea of renaming these modes as been let aside because naming is a
> > big deal and requires a lot of thinking. So for now let's focus on
> > correctly explaining what each mode implies.
> > 
> > [1] https://lore.kernel.org/linux-iio/20210930165510.2295e6c4@jic23-huawei/
> > 
> > Suggested-by: Jonathan Cameron <jic23@kernel.org>
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>  
> One trivial thing inline as a result of edits in v3.
> 
> Otherwise, I want to let this series sit a little longer and ideally get
> some eyes on the st_sensors patches.

Sure.

> 
> Jonathan
> 
> > ---
> >  include/linux/iio/iio.h | 49 ++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 48 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> > index 85cb924debd9..e383b0d96035 100644
> > --- a/include/linux/iio/iio.h
> > +++ b/include/linux/iio/iio.h
> > @@ -315,7 +315,54 @@ static inline bool iio_channel_has_available(const struct iio_chan_spec *chan,
> >  s64 iio_get_time_ns(const struct iio_dev *indio_dev);
> >  unsigned int iio_get_time_res(const struct iio_dev *indio_dev);
> >  
> > -/* Device operating modes */
> > +/**
> > + * Device operating modes
> > + * @INDIO_DIRECT_MODE: There is an access to either:
> > + * a) The last single value available for devices that do not provide
> > + *    on-demand reads.
> > + * b) A new value after performing an on-demand read otherwise.  
> 
> 
> > + * On most devices, this is a single-shot read. On some devices with data
> > + * streams without an 'on-demand' function, this might also be the 'last value'
> > + * feature.  
> 
> This block duplicates what you now have as a/b above. I can drop it whilst
> applying if nothing else comes up.

We can get rid of it indeed. Let's see what ST people have in mind
regarding the st_sensors patches.

> >  Above all, this mode internally means that we are not in any of the
> > + * other modes, and sysfs reads should work.
> > + * Device drivers should inform the core if they support this mode.
> > + * @INDIO_BUFFER_TRIGGERED: Common mode when dealing with kfifo buffers.
> > + * It indicates that an explicit trigger is required. This requests the core to
> > + * attach a poll function when enabling the buffer, which is indicated by the
> > + * _TRIGGERED suffix.
> > + * The core will ensure this mode is set when registering a triggered buffer
> > + * with iio_triggered_buffer_setup().
> > + * @INDIO_BUFFER_SOFTWARE: Another kfifo buffer mode, but not event triggered.
> > + * No poll function can be attached because there is no triggered infrastructure
> > + * we can use to cause capture. There is a kfifo that the driver will fill, but
> > + * not "only one scan at a time". Typically, hardware will have a buffer that
> > + * can hold multiple scans. Software may read one or more scans at a single time
> > + * and push the available data to a Kfifo. This means the core will not attach
> > + * any poll function when enabling the buffer.
> > + * The core will ensure this mode is set when registering a simple kfifo buffer
> > + * with devm_iio_kfifo_buffer_setup().
> > + * @INDIO_BUFFER_HARDWARE: For specific hardware, if unsure do not use this mode.
> > + * Same as above but this time the buffer is not a kfifo where we have direct
> > + * access to the data. Instead, the consumer driver must access the data through
> > + * non software visible channels (or DMA when there is no demux possible in
> > + * software)
> > + * The core will ensure this mode is set when registering a dmaengine buffer
> > + * with devm_iio_dmaengine_buffer_setup().
> > + * @INDIO_EVENT_TRIGGERED: Very unusual mode.
> > + * Triggers usually refer to an external event which will start data capture.
> > + * Here it is kind of the opposite as, a particular state of the data might
> > + * produce an event which can be considered as an event. We don't necessarily
> > + * have access to the data itself, but to the event produced. For example, this
> > + * can be a threshold detector. The internal path of this mode is very close to
> > + * the INDIO_BUFFER_TRIGGERED mode.
> > + * The core will ensure this mode is set when registering a triggered event.
> > + * @INDIO_HARDWARE_TRIGGERED: Very unusual mode.
> > + * Here, triggers can result in data capture and can be routed to multiple
> > + * hardware components, which make them close to regular triggers in the way
> > + * they must be managed by the core, but without the entire interrupts/poll
> > + * functions burden. Interrupts are irrelevant as the data flow is hardware
> > + * mediated and distributed.
> > + */
> >  #define INDIO_DIRECT_MODE		0x01
> >  #define INDIO_BUFFER_TRIGGERED		0x02
> >  #define INDIO_BUFFER_SOFTWARE		0x04  
> 


Thanks,
Miquèl

Re: [PATCH v3 13/13] iio: core: Clarify the modes

[Jonathan Cameron]

On Mon, 14 Feb 2022 09:53:08 +0100
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hi Jonathan,
> 
> jic23@kernel.org wrote on Sun, 13 Feb 2022 18:42:24 +0000:
> 
> > On Mon,  7 Feb 2022 15:38:40 +0100
> > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >   
> > > As part of a previous discussion with Jonathan Cameron [1], it appeared
> > > necessary to clarify the meaning of each mode so that new developers
> > > could understand better what they should use or not use and when.
> > > 
> > > The idea of renaming these modes as been let aside because naming is a
> > > big deal and requires a lot of thinking. So for now let's focus on
> > > correctly explaining what each mode implies.
> > > 
> > > [1] https://lore.kernel.org/linux-iio/20210930165510.2295e6c4@jic23-huawei/
> > > 
> > > Suggested-by: Jonathan Cameron <jic23@kernel.org>
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>    
> > One trivial thing inline as a result of edits in v3.
> > 
> > Otherwise, I want to let this series sit a little longer and ideally get
> > some eyes on the st_sensors patches.  
> 
> Sure.

Denis, Linus, Lorenzo,

If any of you have time to take a look at patches 4-8 in this series or ideally
to run basic sanity tests with series in place that would be great.
https://patchwork.kernel.org/project/linux-iio/list/?series=611853

I don't have a convenient platform to test that driver on any more and the
changes are invasive enough to make me a little nervous about taking the
series without someone more familiar with that driver taking a look.

Thanks,

Jonathan

> 
> > 
> > Jonathan
> >   
> > > ---
> > >  include/linux/iio/iio.h | 49 ++++++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 48 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> > > index 85cb924debd9..e383b0d96035 100644
> > > --- a/include/linux/iio/iio.h
> > > +++ b/include/linux/iio/iio.h
> > > @@ -315,7 +315,54 @@ static inline bool iio_channel_has_available(const struct iio_chan_spec *chan,
> > >  s64 iio_get_time_ns(const struct iio_dev *indio_dev);
> > >  unsigned int iio_get_time_res(const struct iio_dev *indio_dev);
> > >  
> > > -/* Device operating modes */
> > > +/**
> > > + * Device operating modes
> > > + * @INDIO_DIRECT_MODE: There is an access to either:
> > > + * a) The last single value available for devices that do not provide
> > > + *    on-demand reads.
> > > + * b) A new value after performing an on-demand read otherwise.    
> > 
> >   
> > > + * On most devices, this is a single-shot read. On some devices with data
> > > + * streams without an 'on-demand' function, this might also be the 'last value'
> > > + * feature.    
> > 
> > This block duplicates what you now have as a/b above. I can drop it whilst
> > applying if nothing else comes up.  
> 
> We can get rid of it indeed. Let's see what ST people have in mind
> regarding the st_sensors patches.
> 
> > >  Above all, this mode internally means that we are not in any of the
> > > + * other modes, and sysfs reads should work.
> > > + * Device drivers should inform the core if they support this mode.
> > > + * @INDIO_BUFFER_TRIGGERED: Common mode when dealing with kfifo buffers.
> > > + * It indicates that an explicit trigger is required. This requests the core to
> > > + * attach a poll function when enabling the buffer, which is indicated by the
> > > + * _TRIGGERED suffix.
> > > + * The core will ensure this mode is set when registering a triggered buffer
> > > + * with iio_triggered_buffer_setup().
> > > + * @INDIO_BUFFER_SOFTWARE: Another kfifo buffer mode, but not event triggered.
> > > + * No poll function can be attached because there is no triggered infrastructure
> > > + * we can use to cause capture. There is a kfifo that the driver will fill, but
> > > + * not "only one scan at a time". Typically, hardware will have a buffer that
> > > + * can hold multiple scans. Software may read one or more scans at a single time
> > > + * and push the available data to a Kfifo. This means the core will not attach
> > > + * any poll function when enabling the buffer.
> > > + * The core will ensure this mode is set when registering a simple kfifo buffer
> > > + * with devm_iio_kfifo_buffer_setup().
> > > + * @INDIO_BUFFER_HARDWARE: For specific hardware, if unsure do not use this mode.
> > > + * Same as above but this time the buffer is not a kfifo where we have direct
> > > + * access to the data. Instead, the consumer driver must access the data through
> > > + * non software visible channels (or DMA when there is no demux possible in
> > > + * software)
> > > + * The core will ensure this mode is set when registering a dmaengine buffer
> > > + * with devm_iio_dmaengine_buffer_setup().
> > > + * @INDIO_EVENT_TRIGGERED: Very unusual mode.
> > > + * Triggers usually refer to an external event which will start data capture.
> > > + * Here it is kind of the opposite as, a particular state of the data might
> > > + * produce an event which can be considered as an event. We don't necessarily
> > > + * have access to the data itself, but to the event produced. For example, this
> > > + * can be a threshold detector. The internal path of this mode is very close to
> > > + * the INDIO_BUFFER_TRIGGERED mode.
> > > + * The core will ensure this mode is set when registering a triggered event.
> > > + * @INDIO_HARDWARE_TRIGGERED: Very unusual mode.
> > > + * Here, triggers can result in data capture and can be routed to multiple
> > > + * hardware components, which make them close to regular triggers in the way
> > > + * they must be managed by the core, but without the entire interrupts/poll
> > > + * functions burden. Interrupts are irrelevant as the data flow is hardware
> > > + * mediated and distributed.
> > > + */
> > >  #define INDIO_DIRECT_MODE		0x01
> > >  #define INDIO_BUFFER_TRIGGERED		0x02
> > >  #define INDIO_BUFFER_SOFTWARE		0x04    
> >   
> 
> 
> Thanks,
> Miquèl

Re: [PATCH v3 13/13] iio: core: Clarify the modes

[Miquel Raynal]

Hello,

+ Christophe

jic23@kernel.org wrote on Sun, 27 Feb 2022 13:35:49 +0000:

> On Mon, 14 Feb 2022 09:53:08 +0100
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > Hi Jonathan,
> > 
> > jic23@kernel.org wrote on Sun, 13 Feb 2022 18:42:24 +0000:
> >   
> > > On Mon,  7 Feb 2022 15:38:40 +0100
> > > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > >     
> > > > As part of a previous discussion with Jonathan Cameron [1], it appeared
> > > > necessary to clarify the meaning of each mode so that new developers
> > > > could understand better what they should use or not use and when.
> > > > 
> > > > The idea of renaming these modes as been let aside because naming is a
> > > > big deal and requires a lot of thinking. So for now let's focus on
> > > > correctly explaining what each mode implies.
> > > > 
> > > > [1] https://lore.kernel.org/linux-iio/20210930165510.2295e6c4@jic23-huawei/
> > > > 
> > > > Suggested-by: Jonathan Cameron <jic23@kernel.org>
> > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>      
> > > One trivial thing inline as a result of edits in v3.
> > > 
> > > Otherwise, I want to let this series sit a little longer and ideally get
> > > some eyes on the st_sensors patches.    
> > 
> > Sure.  
> 
> Denis, Linus, Lorenzo,
> 
> If any of you have time to take a look at patches 4-8 in this series or ideally
> to run basic sanity tests with series in place that would be great.
> https://patchwork.kernel.org/project/linux-iio/list/?series=611853
> 
> I don't have a convenient platform to test that driver on any more and the
> changes are invasive enough to make me a little nervous about taking the
> series without someone more familiar with that driver taking a look.

I'm adding Christophe from ST who might also help having these patches
tested or at least reviewed.

TLDR: as part of a wider cleanup, I ended up "playing" with locks in
the st_sensors drivers, so any testing or feedback is welcome!

Thanks,
Miquèl

Re: [PATCH v3 13/13] iio: core: Clarify the modes

[Miquel Raynal]

Hi Jonathan,

miquel.raynal@bootlin.com wrote on Tue, 15 Mar 2022 16:44:50 +0100:

> Hello,
> 
> + Christophe
> 
> jic23@kernel.org wrote on Sun, 27 Feb 2022 13:35:49 +0000:
> 
> > On Mon, 14 Feb 2022 09:53:08 +0100
> > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >   
> > > Hi Jonathan,
> > > 
> > > jic23@kernel.org wrote on Sun, 13 Feb 2022 18:42:24 +0000:
> > >     
> > > > On Mon,  7 Feb 2022 15:38:40 +0100
> > > > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > > >       
> > > > > As part of a previous discussion with Jonathan Cameron [1], it appeared
> > > > > necessary to clarify the meaning of each mode so that new developers
> > > > > could understand better what they should use or not use and when.
> > > > > 
> > > > > The idea of renaming these modes as been let aside because naming is a
> > > > > big deal and requires a lot of thinking. So for now let's focus on
> > > > > correctly explaining what each mode implies.
> > > > > 
> > > > > [1] https://lore.kernel.org/linux-iio/20210930165510.2295e6c4@jic23-huawei/
> > > > > 
> > > > > Suggested-by: Jonathan Cameron <jic23@kernel.org>
> > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>        
> > > > One trivial thing inline as a result of edits in v3.
> > > > 
> > > > Otherwise, I want to let this series sit a little longer and ideally get
> > > > some eyes on the st_sensors patches.      

Sometimes there is no other choice than applying patches to get
feedback :) Hopefully nothing unexpected will happen, but as this
series lived almost two month on the mailing list already, I would
propose to merge the series as it is. We can still drop/revert it as we
are rather soon in the new cycle. If rebasing is needed just let me
know.

Cheers,
Miquèl

> > > 
> > > Sure.    
> > 
> > Denis, Linus, Lorenzo,
> > 
> > If any of you have time to take a look at patches 4-8 in this series or ideally
> > to run basic sanity tests with series in place that would be great.
> > https://patchwork.kernel.org/project/linux-iio/list/?series=611853
> > 
> > I don't have a convenient platform to test that driver on any more and the
> > changes are invasive enough to make me a little nervous about taking the
> > series without someone more familiar with that driver taking a look.  
> 
> I'm adding Christophe from ST who might also help having these patches
> tested or at least reviewed.
> 
> TLDR: as part of a wider cleanup, I ended up "playing" with locks in
> the st_sensors drivers, so any testing or feedback is welcome!
> 
> Thanks,
> Miquèl

Re: [PATCH v3 13/13] iio: core: Clarify the modes

[Jonathan Cameron]

On Tue, 5 Apr 2022 10:02:00 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hi Jonathan,
> 
> miquel.raynal@bootlin.com wrote on Tue, 15 Mar 2022 16:44:50 +0100:
> 
> > Hello,
> > 
> > + Christophe
> > 
> > jic23@kernel.org wrote on Sun, 27 Feb 2022 13:35:49 +0000:
> >   
> > > On Mon, 14 Feb 2022 09:53:08 +0100
> > > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > >     
> > > > Hi Jonathan,
> > > > 
> > > > jic23@kernel.org wrote on Sun, 13 Feb 2022 18:42:24 +0000:
> > > >       
> > > > > On Mon,  7 Feb 2022 15:38:40 +0100
> > > > > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > > > >         
> > > > > > As part of a previous discussion with Jonathan Cameron [1], it appeared
> > > > > > necessary to clarify the meaning of each mode so that new developers
> > > > > > could understand better what they should use or not use and when.
> > > > > > 
> > > > > > The idea of renaming these modes as been let aside because naming is a
> > > > > > big deal and requires a lot of thinking. So for now let's focus on
> > > > > > correctly explaining what each mode implies.
> > > > > > 
> > > > > > [1] https://lore.kernel.org/linux-iio/20210930165510.2295e6c4@jic23-huawei/
> > > > > > 
> > > > > > Suggested-by: Jonathan Cameron <jic23@kernel.org>
> > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>          
> > > > > One trivial thing inline as a result of edits in v3.
> > > > > 
> > > > > Otherwise, I want to let this series sit a little longer and ideally get
> > > > > some eyes on the st_sensors patches.        
> 
> Sometimes there is no other choice than applying patches to get
> feedback :) Hopefully nothing unexpected will happen, but as this
> series lived almost two month on the mailing list already, I would
> propose to merge the series as it is. We can still drop/revert it as we
> are rather soon in the new cycle. If rebasing is needed just let me
> know.

Fully agree on pushing forwards with merging this.  I've done the rebase whilst
applying. Other than a bit of fuzz main change was that adxl367 made use
of a kfifo buffer in the meantime so needed updating as well to drop the
buffer type parameter. 

Take a quick look and see if I messed anything up.
Applied to the togreg branch of iio.git and pushed out as testing for 0-day
etc to take a poke at it before I make a mess of linux-next.

Thanks,

Jonathan

> 
> Cheers,
> Miquèl
> 
> > > > 
> > > > Sure.      
> > > 
> > > Denis, Linus, Lorenzo,
> > > 
> > > If any of you have time to take a look at patches 4-8 in this series or ideally
> > > to run basic sanity tests with series in place that would be great.
> > > https://patchwork.kernel.org/project/linux-iio/list/?series=611853
> > > 
> > > I don't have a convenient platform to test that driver on any more and the
> > > changes are invasive enough to make me a little nervous about taking the
> > > series without someone more familiar with that driver taking a look.    
> > 
> > I'm adding Christophe from ST who might also help having these patches
> > tested or at least reviewed.
> > 
> > TLDR: as part of a wider cleanup, I ended up "playing" with locks in
> > the st_sensors drivers, so any testing or feedback is welcome!
> > 
> > Thanks,
> > Miquèl  
> 
> 

Re: [PATCH v3 13/13] iio: core: Clarify the modes

[Miquel Raynal]

Hi Jonathan,

jic23@kernel.org wrote on Sun, 10 Apr 2022 16:27:41 +0100:

> On Tue, 5 Apr 2022 10:02:00 +0200
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > Hi Jonathan,
> > 
> > miquel.raynal@bootlin.com wrote on Tue, 15 Mar 2022 16:44:50 +0100:
> >   
> > > Hello,
> > > 
> > > + Christophe
> > > 
> > > jic23@kernel.org wrote on Sun, 27 Feb 2022 13:35:49 +0000:
> > >     
> > > > On Mon, 14 Feb 2022 09:53:08 +0100
> > > > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > > >       
> > > > > Hi Jonathan,
> > > > > 
> > > > > jic23@kernel.org wrote on Sun, 13 Feb 2022 18:42:24 +0000:
> > > > >         
> > > > > > On Mon,  7 Feb 2022 15:38:40 +0100
> > > > > > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > > > > >           
> > > > > > > As part of a previous discussion with Jonathan Cameron [1], it appeared
> > > > > > > necessary to clarify the meaning of each mode so that new developers
> > > > > > > could understand better what they should use or not use and when.
> > > > > > > 
> > > > > > > The idea of renaming these modes as been let aside because naming is a
> > > > > > > big deal and requires a lot of thinking. So for now let's focus on
> > > > > > > correctly explaining what each mode implies.
> > > > > > > 
> > > > > > > [1] https://lore.kernel.org/linux-iio/20210930165510.2295e6c4@jic23-huawei/
> > > > > > > 
> > > > > > > Suggested-by: Jonathan Cameron <jic23@kernel.org>
> > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>            
> > > > > > One trivial thing inline as a result of edits in v3.
> > > > > > 
> > > > > > Otherwise, I want to let this series sit a little longer and ideally get
> > > > > > some eyes on the st_sensors patches.          
> > 
> > Sometimes there is no other choice than applying patches to get
> > feedback :) Hopefully nothing unexpected will happen, but as this
> > series lived almost two month on the mailing list already, I would
> > propose to merge the series as it is. We can still drop/revert it as we
> > are rather soon in the new cycle. If rebasing is needed just let me
> > know.  
> 
> Fully agree on pushing forwards with merging this.  I've done the rebase whilst
> applying. Other than a bit of fuzz main change was that adxl367 made use
> of a kfifo buffer in the meantime so needed updating as well to drop the
> buffer type parameter. 

Great!

> Take a quick look and see if I messed anything up.
> Applied to the togreg branch of iio.git and pushed out as testing for 0-day
> etc to take a poke at it before I make a mess of linux-next.

Yep, found on the testing branch, everything LGTM but TBH I don't fully
remember everything as it was "some" time ago :)

Thanks,
Miquèl