[PATCH RFC 0/6] iio: core: Introduce cleanup.h support for mode locks

[PATCH RFC 0/6] iio: core: Introduce cleanup.h support for mode locks

[Kurt Borja]

Hi,

In a recent driver review discussion [1], Andy Shevchenko suggested we
add cleanup.h support for the lock API:

	iio_device_claim_{direct,buffer_mode}().

Which would allow some nice code simplification* in many places. Some
examples are given as patches, but the last two are the biggest
differences.

Although I was never entirely sure if Andy meant cleanup classes for
locks or for iio_trigger_notify_done(), I still think this is a great
addition to the API :).

Thanks for taking a look!

* It's important to mention that David Lechner expressed some concerns
  about this [2], hence why this is an RFC series.

[1] https://lore.kernel.org/linux-iio/aSsBdJZDWcadxEHC@smile.fi.intel.com/
[2] https://lore.kernel.org/linux-iio/248b009e-0401-4531-b9f0-56771e16bdef@baylibre.com/

Signed-off-by: Kurt Borja <kuurtb@gmail.com>
---
Kurt Borja (6):
      iio: core: Match iio_device_claim_*() return semantics
      iio: core: Match iio_device_claim_*() naming
      iio: core: Add cleanup.h support for iio_device_claim_*()
      iio: light: vcnl4000: Use cleanup.h for IIO locks
      iio: health: max30102: Use cleanup.h for IIO locks
      iio: light: opt4060: Use cleanup.h for IIO locks

 drivers/iio/adc/ade9000.c                          |  4 +-
 .../common/cros_ec_sensors/cros_ec_sensors_core.c  |  7 +--
 drivers/iio/health/max30100.c                      |  4 +-
 drivers/iio/health/max30102.c                      | 24 +++-------
 drivers/iio/industrialio-core.c                    | 34 +++++++++-----
 drivers/iio/light/opt4060.c                        | 52 +++++++---------------
 drivers/iio/light/vcnl4000.c                       | 24 ++++------
 include/linux/iio/iio.h                            | 24 +++++++++-
 8 files changed, 83 insertions(+), 90 deletions(-)
---
base-commit: f9e05791642810a0cf6237d39fafd6fec5e0b4bb
change-id: 20251130-lock-impr-6f22748c15e8

-- 
 ~ Kurt

[PATCH RFC 1/6] iio: core: Match iio_device_claim_*() return semantics

[Kurt Borja]

In order to improve API consistency with conditional locks, use
true/false return semantics in iio_device_claim_buffer_mode().

This also matches iio_device_claim_direct() return semantics.

Signed-off-by: Kurt Borja <kuurtb@gmail.com>
---
 drivers/iio/adc/ade9000.c                                 | 2 +-
 drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c | 5 +----
 drivers/iio/health/max30100.c                             | 2 +-
 drivers/iio/health/max30102.c                             | 2 +-
 drivers/iio/industrialio-core.c                           | 8 ++++----
 drivers/iio/light/opt4060.c                               | 2 +-
 include/linux/iio/iio.h                                   | 2 +-
 7 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/drivers/iio/adc/ade9000.c b/drivers/iio/adc/ade9000.c
index 2de8a718d62a..b345c4d1ef24 100644
--- a/drivers/iio/adc/ade9000.c
+++ b/drivers/iio/adc/ade9000.c
@@ -964,7 +964,7 @@ static irqreturn_t ade9000_dready_thread(int irq, void *data)
 	struct iio_dev *indio_dev = data;
 
 	/* Handle data ready interrupt from C4/EVENT/DREADY pin */
-	if (!iio_device_claim_buffer_mode(indio_dev)) {
+	if (iio_device_claim_buffer_mode(indio_dev)) {
 		ade9000_iio_push_buffer(indio_dev);
 		iio_device_release_buffer_mode(indio_dev);
 	}
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 9ac80e4b7d75..8ed4b2e410c8 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
@@ -188,11 +188,8 @@ int cros_ec_sensors_push_data(struct iio_dev *indio_dev,
 	/*
 	 * Ignore samples if the buffer is not set: it is needed if the ODR is
 	 * set but the buffer is not enabled yet.
-	 *
-	 * Note: iio_device_claim_buffer_mode() returns -EBUSY if the buffer
-	 * is not enabled.
 	 */
-	if (iio_device_claim_buffer_mode(indio_dev) < 0)
+	if (!iio_device_claim_buffer_mode(indio_dev))
 		return 0;
 
 	out = (s16 *)st->samples;
diff --git a/drivers/iio/health/max30100.c b/drivers/iio/health/max30100.c
index 3d441013893c..3f3680c4b42f 100644
--- a/drivers/iio/health/max30100.c
+++ b/drivers/iio/health/max30100.c
@@ -417,7 +417,7 @@ static int max30100_read_raw(struct iio_dev *indio_dev,
 		 * Temperature reading can only be acquired while engine
 		 * is running
 		 */
-		if (iio_device_claim_buffer_mode(indio_dev)) {
+		if (!iio_device_claim_buffer_mode(indio_dev)) {
 			/*
 			 * Replacing -EBUSY or other error code
 			 * returned by iio_device_claim_buffer_mode()
diff --git a/drivers/iio/health/max30102.c b/drivers/iio/health/max30102.c
index a48c0881a4c7..288c2f37a4a2 100644
--- a/drivers/iio/health/max30102.c
+++ b/drivers/iio/health/max30102.c
@@ -476,7 +476,7 @@ static int max30102_read_raw(struct iio_dev *indio_dev,
 		 * shutdown; leave shutdown briefly when buffer not running
 		 */
 any_mode_retry:
-		if (iio_device_claim_buffer_mode(indio_dev)) {
+		if (!iio_device_claim_buffer_mode(indio_dev)) {
 			/*
 			 * This one is a *bit* hacky. If we cannot claim buffer
 			 * mode, then try direct mode so that we make sure
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index f69deefcfb6f..a10590ac4e17 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -2224,19 +2224,19 @@ EXPORT_SYMBOL_GPL(__iio_device_release_direct);
  *
  * Use with iio_device_release_buffer_mode().
  *
- * Returns: 0 on success, -EBUSY on failure.
+ * Returns: true on success, false on failure.
  */
-int iio_device_claim_buffer_mode(struct iio_dev *indio_dev)
+bool iio_device_claim_buffer_mode(struct iio_dev *indio_dev)
 {
 	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
 
 	mutex_lock(&iio_dev_opaque->mlock);
 
 	if (iio_buffer_enabled(indio_dev))
-		return 0;
+		return true;
 
 	mutex_unlock(&iio_dev_opaque->mlock);
-	return -EBUSY;
+	return false;
 }
 EXPORT_SYMBOL_GPL(iio_device_claim_buffer_mode);
 
diff --git a/drivers/iio/light/opt4060.c b/drivers/iio/light/opt4060.c
index 981c704e7df5..8cb3fa38077e 100644
--- a/drivers/iio/light/opt4060.c
+++ b/drivers/iio/light/opt4060.c
@@ -304,7 +304,7 @@ static int opt4060_set_driver_state(struct iio_dev *indio_dev,
 	struct opt4060_chip *chip = iio_priv(indio_dev);
 	int ret = 0;
 any_mode_retry:
-	if (iio_device_claim_buffer_mode(indio_dev)) {
+	if (!iio_device_claim_buffer_mode(indio_dev)) {
 		/*
 		 * This one is a *bit* hacky. If we cannot claim buffer mode,
 		 * then try direct mode so that we make sure things cannot
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index 872ebdf0dd77..bf7b7337ff1b 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -687,7 +687,7 @@ static inline void iio_device_release_direct(struct iio_dev *indio_dev)
 	__release(indio_dev);
 }
 
-int iio_device_claim_buffer_mode(struct iio_dev *indio_dev);
+bool iio_device_claim_buffer_mode(struct iio_dev *indio_dev);
 void iio_device_release_buffer_mode(struct iio_dev *indio_dev);
 
 extern const struct bus_type iio_bus_type;

-- 
2.52.0

[PATCH RFC 2/6] iio: core: Match iio_device_claim_*() naming

[Kurt Borja]

Rename iio_device_claim_buffer_mode() -> iio_device_claim_buffer() to
match iio_device_claim_direct().

Signed-off-by: Kurt Borja <kuurtb@gmail.com>
---
 drivers/iio/adc/ade9000.c                                |  4 ++--
 .../iio/common/cros_ec_sensors/cros_ec_sensors_core.c    |  4 ++--
 drivers/iio/health/max30100.c                            |  4 ++--
 drivers/iio/health/max30102.c                            |  4 ++--
 drivers/iio/industrialio-core.c                          | 16 ++++++++--------
 drivers/iio/light/opt4060.c                              |  4 ++--
 include/linux/iio/iio.h                                  |  4 ++--
 7 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/drivers/iio/adc/ade9000.c b/drivers/iio/adc/ade9000.c
index b345c4d1ef24..b0bc75999f40 100644
--- a/drivers/iio/adc/ade9000.c
+++ b/drivers/iio/adc/ade9000.c
@@ -964,9 +964,9 @@ static irqreturn_t ade9000_dready_thread(int irq, void *data)
 	struct iio_dev *indio_dev = data;
 
 	/* Handle data ready interrupt from C4/EVENT/DREADY pin */
-	if (iio_device_claim_buffer_mode(indio_dev)) {
+	if (iio_device_claim_buffer(indio_dev)) {
 		ade9000_iio_push_buffer(indio_dev);
-		iio_device_release_buffer_mode(indio_dev);
+		iio_device_release_buffer(indio_dev);
 	}
 
 	return IRQ_HANDLED;
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 8ed4b2e410c8..b1c50139ea92 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
@@ -189,7 +189,7 @@ int cros_ec_sensors_push_data(struct iio_dev *indio_dev,
 	 * Ignore samples if the buffer is not set: it is needed if the ODR is
 	 * set but the buffer is not enabled yet.
 	 */
-	if (!iio_device_claim_buffer_mode(indio_dev))
+	if (!iio_device_claim_buffer(indio_dev))
 		return 0;
 
 	out = (s16 *)st->samples;
@@ -206,7 +206,7 @@ int cros_ec_sensors_push_data(struct iio_dev *indio_dev,
 	iio_push_to_buffers_with_timestamp(indio_dev, st->samples,
 					   timestamp + delta);
 
-	iio_device_release_buffer_mode(indio_dev);
+	iio_device_release_buffer(indio_dev);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(cros_ec_sensors_push_data);
diff --git a/drivers/iio/health/max30100.c b/drivers/iio/health/max30100.c
index 3f3680c4b42f..6aefe9c4a8f6 100644
--- a/drivers/iio/health/max30100.c
+++ b/drivers/iio/health/max30100.c
@@ -417,7 +417,7 @@ static int max30100_read_raw(struct iio_dev *indio_dev,
 		 * Temperature reading can only be acquired while engine
 		 * is running
 		 */
-		if (!iio_device_claim_buffer_mode(indio_dev)) {
+		if (!iio_device_claim_buffer(indio_dev)) {
 			/*
 			 * Replacing -EBUSY or other error code
 			 * returned by iio_device_claim_buffer_mode()
@@ -430,7 +430,7 @@ static int max30100_read_raw(struct iio_dev *indio_dev,
 			if (!ret)
 				ret = IIO_VAL_INT;
 
-			iio_device_release_buffer_mode(indio_dev);
+			iio_device_release_buffer(indio_dev);
 		}
 		break;
 	case IIO_CHAN_INFO_SCALE:
diff --git a/drivers/iio/health/max30102.c b/drivers/iio/health/max30102.c
index 288c2f37a4a2..678720102f2b 100644
--- a/drivers/iio/health/max30102.c
+++ b/drivers/iio/health/max30102.c
@@ -476,7 +476,7 @@ static int max30102_read_raw(struct iio_dev *indio_dev,
 		 * shutdown; leave shutdown briefly when buffer not running
 		 */
 any_mode_retry:
-		if (!iio_device_claim_buffer_mode(indio_dev)) {
+		if (!iio_device_claim_buffer(indio_dev)) {
 			/*
 			 * This one is a *bit* hacky. If we cannot claim buffer
 			 * mode, then try direct mode so that we make sure
@@ -490,7 +490,7 @@ static int max30102_read_raw(struct iio_dev *indio_dev,
 			iio_device_release_direct(indio_dev);
 		} else {
 			ret = max30102_get_temp(data, val, false);
-			iio_device_release_buffer_mode(indio_dev);
+			iio_device_release_buffer(indio_dev);
 		}
 		if (ret)
 			return ret;
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index a10590ac4e17..adf0142d0300 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -2216,17 +2216,17 @@ void __iio_device_release_direct(struct iio_dev *indio_dev)
 EXPORT_SYMBOL_GPL(__iio_device_release_direct);
 
 /**
- * iio_device_claim_buffer_mode - Keep device in buffer mode
+ * iio_device_claim_buffer - Keep device in buffer mode
  * @indio_dev:	the iio_dev associated with the device
  *
  * If the device is in buffer mode it is guaranteed to stay
  * that way until iio_device_release_buffer_mode() is called.
  *
- * Use with iio_device_release_buffer_mode().
+ * Use with iio_device_release_buffer().
  *
  * Returns: true on success, false on failure.
  */
-bool iio_device_claim_buffer_mode(struct iio_dev *indio_dev)
+bool iio_device_claim_buffer(struct iio_dev *indio_dev)
 {
 	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
 
@@ -2238,22 +2238,22 @@ bool iio_device_claim_buffer_mode(struct iio_dev *indio_dev)
 	mutex_unlock(&iio_dev_opaque->mlock);
 	return false;
 }
-EXPORT_SYMBOL_GPL(iio_device_claim_buffer_mode);
+EXPORT_SYMBOL_GPL(iio_device_claim_buffer);
 
 /**
- * iio_device_release_buffer_mode - releases claim on buffer mode
+ * iio_device_release_buffer - releases claim on buffer mode
  * @indio_dev:	the iio_dev associated with the device
  *
  * Release the claim. Device is no longer guaranteed to stay
  * in buffer mode.
  *
- * Use with iio_device_claim_buffer_mode().
+ * Use with iio_device_claim_buffer().
  */
-void iio_device_release_buffer_mode(struct iio_dev *indio_dev)
+void iio_device_release_buffer(struct iio_dev *indio_dev)
 {
 	mutex_unlock(&to_iio_dev_opaque(indio_dev)->mlock);
 }
-EXPORT_SYMBOL_GPL(iio_device_release_buffer_mode);
+EXPORT_SYMBOL_GPL(iio_device_release_buffer);
 
 /**
  * iio_device_get_current_mode() - helper function providing read-only access to
diff --git a/drivers/iio/light/opt4060.c b/drivers/iio/light/opt4060.c
index 8cb3fa38077e..500899d7bd62 100644
--- a/drivers/iio/light/opt4060.c
+++ b/drivers/iio/light/opt4060.c
@@ -304,7 +304,7 @@ static int opt4060_set_driver_state(struct iio_dev *indio_dev,
 	struct opt4060_chip *chip = iio_priv(indio_dev);
 	int ret = 0;
 any_mode_retry:
-	if (!iio_device_claim_buffer_mode(indio_dev)) {
+	if (!iio_device_claim_buffer(indio_dev)) {
 		/*
 		 * This one is a *bit* hacky. If we cannot claim buffer mode,
 		 * then try direct mode so that we make sure things cannot
@@ -334,7 +334,7 @@ static int opt4060_set_driver_state(struct iio_dev *indio_dev,
 		else
 			ret = opt4060_set_state_common(chip, continuous_sampling,
 						       continuous_irq);
-		iio_device_release_buffer_mode(indio_dev);
+		iio_device_release_buffer(indio_dev);
 	}
 	return ret;
 }
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index bf7b7337ff1b..27da9af67c47 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -687,8 +687,8 @@ static inline void iio_device_release_direct(struct iio_dev *indio_dev)
 	__release(indio_dev);
 }
 
-bool iio_device_claim_buffer_mode(struct iio_dev *indio_dev);
-void iio_device_release_buffer_mode(struct iio_dev *indio_dev);
+bool iio_device_claim_buffer(struct iio_dev *indio_dev);
+void iio_device_release_buffer(struct iio_dev *indio_dev);
 
 extern const struct bus_type iio_bus_type;
 

-- 
2.52.0

[PATCH RFC 3/6] iio: core: Add cleanup.h support for iio_device_claim_*()

[Kurt Borja]

Add guard() and ACQUIRE() support for iio_device_claim_*() lock.

This involves exporting iio_device_{claim, release}() wrappers to define
a general GUARD class, and then defining the _direct and _buffer
conditional ones.

Suggested-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Signed-off-by: Kurt Borja <kuurtb@gmail.com>
---
 drivers/iio/industrialio-core.c | 12 ++++++++++++
 include/linux/iio/iio.h         | 20 ++++++++++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index adf0142d0300..da090c993fe8 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -2171,6 +2171,18 @@ int __devm_iio_device_register(struct device *dev, struct iio_dev *indio_dev,
 }
 EXPORT_SYMBOL_GPL(__devm_iio_device_register);
 
+void __iio_device_claim(struct iio_dev *indio_dev)
+{
+	mutex_lock(&to_iio_dev_opaque(indio_dev)->mlock);
+}
+EXPORT_SYMBOL_GPL(__iio_device_claim);
+
+void __iio_device_release(struct iio_dev *indio_dev)
+{
+	mutex_unlock(&to_iio_dev_opaque(indio_dev)->mlock);
+}
+EXPORT_SYMBOL_GPL(__iio_device_release);
+
 /**
  * __iio_device_claim_direct - Keep device in direct mode
  * @indio_dev:	the iio_dev associated with the device
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index 27da9af67c47..472b13ec28d3 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -10,6 +10,7 @@
 #include <linux/align.h>
 #include <linux/device.h>
 #include <linux/cdev.h>
+#include <linux/cleanup.h>
 #include <linux/compiler_types.h>
 #include <linux/minmax.h>
 #include <linux/slab.h>
@@ -661,9 +662,23 @@ void iio_device_unregister(struct iio_dev *indio_dev);
 int __devm_iio_device_register(struct device *dev, struct iio_dev *indio_dev,
 			       struct module *this_mod);
 int iio_push_event(struct iio_dev *indio_dev, u64 ev_code, s64 timestamp);
+void __iio_device_claim(struct iio_dev *indio_dev);
+void __iio_device_release(struct iio_dev *indio_dev);
 bool __iio_device_claim_direct(struct iio_dev *indio_dev);
 void __iio_device_release_direct(struct iio_dev *indio_dev);
 
+static inline void iio_device_claim(struct iio_dev *indio_dev)
+	__acquires(indio_dev)
+{
+	__iio_device_claim(indio_dev);
+}
+
+static inline void iio_device_release(struct iio_dev *indio_dev)
+	__releases(indio_dev)
+{
+	__iio_device_release(indio_dev);
+}
+
 /*
  * Helper functions that allow claim and release of direct mode
  * in a fashion that doesn't generate many false positives from sparse.
@@ -690,6 +705,11 @@ static inline void iio_device_release_direct(struct iio_dev *indio_dev)
 bool iio_device_claim_buffer(struct iio_dev *indio_dev);
 void iio_device_release_buffer(struct iio_dev *indio_dev);
 
+DEFINE_GUARD(iio_device_claim, struct iio_dev *, iio_device_claim(_T),
+	     iio_device_release(_T));
+DEFINE_GUARD_COND(iio_device_claim, _buffer, iio_device_claim_buffer(_T));
+DEFINE_GUARD_COND(iio_device_claim, _direct, iio_device_claim_direct(_T));
+
 extern const struct bus_type iio_bus_type;
 
 /**

-- 
2.52.0

[PATCH RFC 4/6] iio: light: vcnl4000: Use cleanup.h for IIO locks

[Kurt Borja]

Use ACQUIRE() for iio_device_claim_direct().

Signed-off-by: Kurt Borja <kuurtb@gmail.com>
---
 drivers/iio/light/vcnl4000.c | 24 ++++++++----------------
 1 file changed, 8 insertions(+), 16 deletions(-)

diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
index 4dbb2294a843..55e5060ce337 100644
--- a/drivers/iio/light/vcnl4000.c
+++ b/drivers/iio/light/vcnl4000.c
@@ -25,6 +25,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/interrupt.h>
 #include <linux/units.h>
+#include <linux/cleanup.h>
 
 #include <linux/iio/buffer.h>
 #include <linux/iio/events.h>
@@ -1148,36 +1149,27 @@ static int vcnl4010_write_raw(struct iio_dev *indio_dev,
 			      struct iio_chan_spec const *chan,
 			      int val, int val2, long mask)
 {
-	int ret;
 	struct vcnl4000_data *data = iio_priv(indio_dev);
 
-	if (!iio_device_claim_direct(indio_dev))
+	ACQUIRE(iio_device_claim_direct, busy)(indio_dev);
+	if (ACQUIRE_ERR(iio_device_claim_direct, &busy))
 		return -EBUSY;
 
 	/* Protect against event capture. */
-	if (vcnl4010_is_in_periodic_mode(data)) {
-		ret = -EBUSY;
-		goto end;
-	}
+	if (vcnl4010_is_in_periodic_mode(data))
+		return -EBUSY;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		switch (chan->type) {
 		case IIO_PROXIMITY:
-			ret = vcnl4010_write_proxy_samp_freq(data, val, val2);
-			goto end;
+			return vcnl4010_write_proxy_samp_freq(data, val, val2);
 		default:
-			ret = -EINVAL;
-			goto end;
+			return -EINVAL;
 		}
 	default:
-		ret = -EINVAL;
-		goto end;
+		return -EINVAL;
 	}
-
-end:
-	iio_device_release_direct(indio_dev);
-	return ret;
 }
 
 static int vcnl4010_read_event(struct iio_dev *indio_dev,

-- 
2.52.0

[PATCH RFC 5/6] iio: health: max30102: Use cleanup.h for IIO locks

[Kurt Borja]

Simplify and drop "hacky" busy-waiting code in max30102_read_raw() by
using scoped_guard().

Signed-off-by: Kurt Borja <kuurtb@gmail.com>
---
 drivers/iio/health/max30102.c | 24 +++++++-----------------
 1 file changed, 7 insertions(+), 17 deletions(-)

diff --git a/drivers/iio/health/max30102.c b/drivers/iio/health/max30102.c
index 678720102f2b..c642842cb5fb 100644
--- a/drivers/iio/health/max30102.c
+++ b/drivers/iio/health/max30102.c
@@ -24,6 +24,7 @@
 #include <linux/iio/iio.h>
 #include <linux/iio/buffer.h>
 #include <linux/iio/kfifo_buf.h>
+#include <linux/cleanup.h>
 
 #define MAX30102_DRV_NAME	"max30102"
 #define MAX30102_PART_NUMBER	0x15
@@ -468,6 +469,7 @@ static int max30102_read_raw(struct iio_dev *indio_dev,
 {
 	struct max30102_data *data = iio_priv(indio_dev);
 	int ret = -EINVAL;
+	bool direct_en;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
@@ -475,25 +477,13 @@ static int max30102_read_raw(struct iio_dev *indio_dev,
 		 * Temperature reading can only be acquired when not in
 		 * shutdown; leave shutdown briefly when buffer not running
 		 */
-any_mode_retry:
-		if (!iio_device_claim_buffer(indio_dev)) {
-			/*
-			 * This one is a *bit* hacky. If we cannot claim buffer
-			 * mode, then try direct mode so that we make sure
-			 * things cannot concurrently change. And we just keep
-			 * trying until we get one of the modes...
-			 */
-			if (!iio_device_claim_direct(indio_dev))
-				goto any_mode_retry;
+		scoped_guard(iio_device_claim, indio_dev) {
+			direct_en = !iio_buffer_enabled(indio_dev);
 
-			ret = max30102_get_temp(data, val, true);
-			iio_device_release_direct(indio_dev);
-		} else {
-			ret = max30102_get_temp(data, val, false);
-			iio_device_release_buffer(indio_dev);
+			ret = max30102_get_temp(data, val, direct_en);
+			if (ret)
+				return ret;
 		}
-		if (ret)
-			return ret;
 
 		ret = IIO_VAL_INT;
 		break;

-- 
2.52.0

[PATCH RFC 6/6] iio: light: opt4060: Use cleanup.h for IIO locks

[Kurt Borja]

Simplify and drop "hacky" busy-waiting code in
opt4060_set_driver_state() by using guard().

Signed-off-by: Kurt Borja <kuurtb@gmail.com>
---
 drivers/iio/light/opt4060.c | 52 +++++++++++++++------------------------------
 1 file changed, 17 insertions(+), 35 deletions(-)

diff --git a/drivers/iio/light/opt4060.c b/drivers/iio/light/opt4060.c
index 500899d7bd62..903963606143 100644
--- a/drivers/iio/light/opt4060.c
+++ b/drivers/iio/light/opt4060.c
@@ -22,6 +22,7 @@
 #include <linux/iio/trigger.h>
 #include <linux/iio/trigger_consumer.h>
 #include <linux/iio/triggered_buffer.h>
+#include <linux/cleanup.h>
 
 /* OPT4060 register set */
 #define OPT4060_RED_MSB				0x00
@@ -302,41 +303,22 @@ static int opt4060_set_driver_state(struct iio_dev *indio_dev,
 				    bool continuous_irq)
 {
 	struct opt4060_chip *chip = iio_priv(indio_dev);
-	int ret = 0;
-any_mode_retry:
-	if (!iio_device_claim_buffer(indio_dev)) {
-		/*
-		 * This one is a *bit* hacky. If we cannot claim buffer mode,
-		 * then try direct mode so that we make sure things cannot
-		 * concurrently change. And we just keep trying until we get one
-		 * of the modes...
-		 */
-		if (!iio_device_claim_direct(indio_dev))
-			goto any_mode_retry;
-		/*
-		 * This path means that we managed to claim direct mode. In
-		 * this case the buffer isn't enabled and it's okay to leave
-		 * continuous mode for sampling and/or irq.
-		 */
-		ret = opt4060_set_state_common(chip, continuous_sampling,
-					       continuous_irq);
-		iio_device_release_direct(indio_dev);
-		return ret;
-	} else {
-		/*
-		 * This path means that we managed to claim buffer mode. In
-		 * this case the buffer is enabled and irq and sampling must go
-		 * to or remain continuous, but only if the trigger is from this
-		 * device.
-		 */
-		if (!iio_trigger_validate_own_device(indio_dev->trig, indio_dev))
-			ret = opt4060_set_state_common(chip, true, true);
-		else
-			ret = opt4060_set_state_common(chip, continuous_sampling,
-						       continuous_irq);
-		iio_device_release_buffer(indio_dev);
-	}
-	return ret;
+
+	guard(iio_device_claim)(indio_dev);
+
+	/*
+	 * If we manage to claim buffer mode and we are using our own trigger,
+	 * IRQ and sampling must go to or remain continuous.
+	 */
+	if (iio_buffer_enabled(indio_dev) && iio_trigger_using_own(indio_dev))
+		return opt4060_set_state_common(chip, true, true);
+
+	/*
+	 * This path means that we managed to claim direct mode. In this case
+	 * the buffer isn't enabled and it's okay to leave continuous mode for
+	 * sampling and/or irq.
+	 */
+	return opt4060_set_state_common(chip, continuous_sampling, continuous_irq);
 }
 
 /*

-- 
2.52.0

Re: [PATCH RFC 2/6] iio: core: Match iio_device_claim_*() naming

[David Lechner]

On 12/3/25 1:18 PM, Kurt Borja wrote:
> Rename iio_device_claim_buffer_mode() -> iio_device_claim_buffer() to
> match iio_device_claim_direct().
> 
> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
> ---
If we decide to do this, I would squash this with the previous patch
to make a clean break of it. Although it is helpful to have "mode"
in the name if we can keep that without breaking things.

Re: [PATCH RFC 3/6] iio: core: Add cleanup.h support for iio_device_claim_*()

[David Lechner]

On 12/3/25 1:18 PM, Kurt Borja wrote:
> Add guard() and ACQUIRE() support for iio_device_claim_*() lock.
> 
> This involves exporting iio_device_{claim, release}() wrappers to define
> a general GUARD class, and then defining the _direct and _buffer
> conditional ones.

Commit messages should say why we need this.

Also, this seems like two separate things. Adding a new claim/release pair
and adding the conditional guard support to the existing ones. So perhaps
better as two separate patches.

> 
> Suggested-by: Andy Shevchenko <andriy.shevchenko@intel.com>
> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
> ---
>  drivers/iio/industrialio-core.c | 12 ++++++++++++
>  include/linux/iio/iio.h         | 20 ++++++++++++++++++++
>  2 files changed, 32 insertions(+)
> 
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index adf0142d0300..da090c993fe8 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -2171,6 +2171,18 @@ int __devm_iio_device_register(struct device *dev, struct iio_dev *indio_dev,
>  }
>  EXPORT_SYMBOL_GPL(__devm_iio_device_register);
>  
> +void __iio_device_claim(struct iio_dev *indio_dev)
> +{
> +	mutex_lock(&to_iio_dev_opaque(indio_dev)->mlock);
> +}
> +EXPORT_SYMBOL_GPL(__iio_device_claim);
> +
> +void __iio_device_release(struct iio_dev *indio_dev)
> +{
> +	mutex_unlock(&to_iio_dev_opaque(indio_dev)->mlock);
> +}
> +EXPORT_SYMBOL_GPL(__iio_device_release);
> +
>  /**
>   * __iio_device_claim_direct - Keep device in direct mode
>   * @indio_dev:	the iio_dev associated with the device
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index 27da9af67c47..472b13ec28d3 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -10,6 +10,7 @@
>  #include <linux/align.h>
>  #include <linux/device.h>
>  #include <linux/cdev.h>
> +#include <linux/cleanup.h>
>  #include <linux/compiler_types.h>
>  #include <linux/minmax.h>
>  #include <linux/slab.h>
> @@ -661,9 +662,23 @@ void iio_device_unregister(struct iio_dev *indio_dev);
>  int __devm_iio_device_register(struct device *dev, struct iio_dev *indio_dev,
>  			       struct module *this_mod);
>  int iio_push_event(struct iio_dev *indio_dev, u64 ev_code, s64 timestamp);
> +void __iio_device_claim(struct iio_dev *indio_dev);
> +void __iio_device_release(struct iio_dev *indio_dev);
>  bool __iio_device_claim_direct(struct iio_dev *indio_dev);
>  void __iio_device_release_direct(struct iio_dev *indio_dev);
>  
> +static inline void iio_device_claim(struct iio_dev *indio_dev)
> +	__acquires(indio_dev)
> +{
> +	__iio_device_claim(indio_dev);
> +}
> +
> +static inline void iio_device_release(struct iio_dev *indio_dev)
> +	__releases(indio_dev)
> +{
> +	__iio_device_release(indio_dev);
> +}

It was unfortunate that we had to drop "mode" from iio_device_claim_direct_mode()
during the recent API change, but at least it is fairly obvious that "direct"
is a mode. Here, dropping "mode" from the name hurts the understanding. These
could also use some documentation comments to explain what these are for and
when it is appropriate to use them. I had to really dig around the code to
come to the understanding that these mean "don't allow switching modes until
we release the claim".

I would call it something like iio_device_{claim,release}_current_mode().


> +
>  /*
>   * Helper functions that allow claim and release of direct mode
>   * in a fashion that doesn't generate many false positives from sparse.
> @@ -690,6 +705,11 @@ static inline void iio_device_release_direct(struct iio_dev *indio_dev)
>  bool iio_device_claim_buffer(struct iio_dev *indio_dev);
>  void iio_device_release_buffer(struct iio_dev *indio_dev);
>  
> +DEFINE_GUARD(iio_device_claim, struct iio_dev *, iio_device_claim(_T),
> +	     iio_device_release(_T));
> +DEFINE_GUARD_COND(iio_device_claim, _buffer, iio_device_claim_buffer(_T));
> +DEFINE_GUARD_COND(iio_device_claim, _direct, iio_device_claim_direct(_T));
> +
>  extern const struct bus_type iio_bus_type;
>  
>  /**
> 

Re: [PATCH RFC 5/6] iio: health: max30102: Use cleanup.h for IIO locks

[David Lechner]

On 12/3/25 1:18 PM, Kurt Borja wrote:
> Simplify and drop "hacky" busy-waiting code in max30102_read_raw() by
> using scoped_guard().
> 
> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
> ---
>  drivers/iio/health/max30102.c | 24 +++++++-----------------
>  1 file changed, 7 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/iio/health/max30102.c b/drivers/iio/health/max30102.c
> index 678720102f2b..c642842cb5fb 100644
> --- a/drivers/iio/health/max30102.c
> +++ b/drivers/iio/health/max30102.c
> @@ -24,6 +24,7 @@
>  #include <linux/iio/iio.h>
>  #include <linux/iio/buffer.h>
>  #include <linux/iio/kfifo_buf.h>
> +#include <linux/cleanup.h>
>  
>  #define MAX30102_DRV_NAME	"max30102"
>  #define MAX30102_PART_NUMBER	0x15
> @@ -468,6 +469,7 @@ static int max30102_read_raw(struct iio_dev *indio_dev,
>  {
>  	struct max30102_data *data = iio_priv(indio_dev);
>  	int ret = -EINVAL;
> +	bool direct_en;
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
> @@ -475,25 +477,13 @@ static int max30102_read_raw(struct iio_dev *indio_dev,
>  		 * Temperature reading can only be acquired when not in
>  		 * shutdown; leave shutdown briefly when buffer not running
>  		 */
> -any_mode_retry:
> -		if (!iio_device_claim_buffer(indio_dev)) {
> -			/*
> -			 * This one is a *bit* hacky. If we cannot claim buffer
> -			 * mode, then try direct mode so that we make sure
> -			 * things cannot concurrently change. And we just keep
> -			 * trying until we get one of the modes...
> -			 */
> -			if (!iio_device_claim_direct(indio_dev))
> -				goto any_mode_retry;
> +		scoped_guard(iio_device_claim, indio_dev) {

scoped_guard() is sketchy in switch statements because there is
a hidden for loop. If someone came along later and put a break;
inside of the scope, it would break out of the hidden for loop
rather than the apparent switch case!

Besides that, it adds extra indent that we could avoid.

> +			direct_en = !iio_buffer_enabled(indio_dev);
>  
> -			ret = max30102_get_temp(data, val, true);
> -			iio_device_release_direct(indio_dev);
> -		} else {
> -			ret = max30102_get_temp(data, val, false);
> -			iio_device_release_buffer(indio_dev);
> +			ret = max30102_get_temp(data, val, direct_en);
> +			if (ret)
> +				return ret;
>  		}
> -		if (ret)
> -			return ret;
>  
>  		ret = IIO_VAL_INT;
>  		break;
> 

I would write the whole function like this:

static int max30102_read_raw(struct iio_dev *indio_dev,
			     struct iio_chan_spec const *chan,
			     int *val, int *val2, long mask)
{
	struct max30102_data *data = iio_priv(indio_dev);
	int ret;

	switch (mask) {
	case IIO_CHAN_INFO_RAW: {
		/*
		 * Temperature reading can only be acquired when not in
		 * shutdown; leave shutdown briefly when buffer not running
		 */
		guard(iio_device_claim)(indio_dev);
		ret = max30102_get_temp(data, val, !iio_buffer_enabled(indio_dev));
		if (ret)
			return ret;

		return IIO_VAL_INT;
	}
	case IIO_CHAN_INFO_SCALE:
		*val = 1000;  /* 62.5 */
		*val2 = 16;
		return IIO_VAL_FRACTIONAL;
	default:
		return -EINVAL;
	}
}

Could also simplify things further by moving the call to iio_buffer_enabled()
into max30102_get_temp().

Re: [PATCH RFC 4/6] iio: light: vcnl4000: Use cleanup.h for IIO locks

[David Lechner]

On 12/3/25 1:18 PM, Kurt Borja wrote:
> Use ACQUIRE() for iio_device_claim_direct().
> 
> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
> ---
>  drivers/iio/light/vcnl4000.c | 24 ++++++++----------------
>  1 file changed, 8 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
> index 4dbb2294a843..55e5060ce337 100644
> --- a/drivers/iio/light/vcnl4000.c
> +++ b/drivers/iio/light/vcnl4000.c
> @@ -25,6 +25,7 @@
>  #include <linux/pm_runtime.h>
>  #include <linux/interrupt.h>
>  #include <linux/units.h>
> +#include <linux/cleanup.h>
>  
>  #include <linux/iio/buffer.h>
>  #include <linux/iio/events.h>
> @@ -1148,36 +1149,27 @@ static int vcnl4010_write_raw(struct iio_dev *indio_dev,
>  			      struct iio_chan_spec const *chan,
>  			      int val, int val2, long mask)
>  {
> -	int ret;
>  	struct vcnl4000_data *data = iio_priv(indio_dev);
>  
> -	if (!iio_device_claim_direct(indio_dev))
> +	ACQUIRE(iio_device_claim_direct, busy)(indio_dev);
> +	if (ACQUIRE_ERR(iio_device_claim_direct, &busy))

Calling the cleanup variable `busy` seems odd to me since if
we successfully make the claim, busy will be non-null and
therefore `if (busy)` would evaluate as true even though
it wasn't "busy". I would call it `claim` to match
iio_device_claim_direct().

Re: [PATCH RFC 3/6] iio: core: Add cleanup.h support for iio_device_claim_*()

[David Lechner]

On 12/3/25 3:50 PM, David Lechner wrote:
> On 12/3/25 1:18 PM, Kurt Borja wrote:
>> Add guard() and ACQUIRE() support for iio_device_claim_*() lock.
>>
>> This involves exporting iio_device_{claim, release}() wrappers to define
>> a general GUARD class, and then defining the _direct and _buffer
>> conditional ones.
> 
> Commit messages should say why we need this.
> 
> Also, this seems like two separate things. Adding a new claim/release pair
> and adding the conditional guard support to the existing ones. So perhaps
> better as two separate patches.
> 
>>
>> Suggested-by: Andy Shevchenko <andriy.shevchenko@intel.com>
>> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
>> ---
>>  drivers/iio/industrialio-core.c | 12 ++++++++++++
>>  include/linux/iio/iio.h         | 20 ++++++++++++++++++++
>>  2 files changed, 32 insertions(+)
>>
>> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
>> index adf0142d0300..da090c993fe8 100644
>> --- a/drivers/iio/industrialio-core.c
>> +++ b/drivers/iio/industrialio-core.c
>> @@ -2171,6 +2171,18 @@ int __devm_iio_device_register(struct device *dev, struct iio_dev *indio_dev,
>>  }
>>  EXPORT_SYMBOL_GPL(__devm_iio_device_register);
>>  
>> +void __iio_device_claim(struct iio_dev *indio_dev)
>> +{
>> +	mutex_lock(&to_iio_dev_opaque(indio_dev)->mlock);
>> +}
>> +EXPORT_SYMBOL_GPL(__iio_device_claim);
>> +
>> +void __iio_device_release(struct iio_dev *indio_dev)
>> +{
>> +	mutex_unlock(&to_iio_dev_opaque(indio_dev)->mlock);
>> +}
>> +EXPORT_SYMBOL_GPL(__iio_device_release);
>> +
>>  /**
>>   * __iio_device_claim_direct - Keep device in direct mode
>>   * @indio_dev:	the iio_dev associated with the device
>> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
>> index 27da9af67c47..472b13ec28d3 100644
>> --- a/include/linux/iio/iio.h
>> +++ b/include/linux/iio/iio.h
>> @@ -10,6 +10,7 @@
>>  #include <linux/align.h>
>>  #include <linux/device.h>
>>  #include <linux/cdev.h>
>> +#include <linux/cleanup.h>
>>  #include <linux/compiler_types.h>
>>  #include <linux/minmax.h>
>>  #include <linux/slab.h>
>> @@ -661,9 +662,23 @@ void iio_device_unregister(struct iio_dev *indio_dev);
>>  int __devm_iio_device_register(struct device *dev, struct iio_dev *indio_dev,
>>  			       struct module *this_mod);
>>  int iio_push_event(struct iio_dev *indio_dev, u64 ev_code, s64 timestamp);
>> +void __iio_device_claim(struct iio_dev *indio_dev);
>> +void __iio_device_release(struct iio_dev *indio_dev);
>>  bool __iio_device_claim_direct(struct iio_dev *indio_dev);
>>  void __iio_device_release_direct(struct iio_dev *indio_dev);
>>  
>> +static inline void iio_device_claim(struct iio_dev *indio_dev)
>> +	__acquires(indio_dev)
>> +{
>> +	__iio_device_claim(indio_dev);
>> +}
>> +
>> +static inline void iio_device_release(struct iio_dev *indio_dev)
>> +	__releases(indio_dev)
>> +{
>> +	__iio_device_release(indio_dev);
>> +}
> 
> It was unfortunate that we had to drop "mode" from iio_device_claim_direct_mode()
> during the recent API change, but at least it is fairly obvious that "direct"
> is a mode. Here, dropping "mode" from the name hurts the understanding. These
> could also use some documentation comments to explain what these are for and
> when it is appropriate to use them. I had to really dig around the code to
> come to the understanding that these mean "don't allow switching modes until
> we release the claim".
> 
> I would call it something like iio_device_{claim,release}_current_mode().
> 
> 
>> +
>>  /*
>>   * Helper functions that allow claim and release of direct mode
>>   * in a fashion that doesn't generate many false positives from sparse.
>> @@ -690,6 +705,11 @@ static inline void iio_device_release_direct(struct iio_dev *indio_dev)
>>  bool iio_device_claim_buffer(struct iio_dev *indio_dev);
>>  void iio_device_release_buffer(struct iio_dev *indio_dev);
>>  
>> +DEFINE_GUARD(iio_device_claim, struct iio_dev *, iio_device_claim(_T),
>> +	     iio_device_release(_T));
>> +DEFINE_GUARD_COND(iio_device_claim, _buffer, iio_device_claim_buffer(_T));
>> +DEFINE_GUARD_COND(iio_device_claim, _direct, iio_device_claim_direct(_T));
>> +

When I made the comments about keeping "mode" in the name, I forgot
that DEFINE_GUARD_COND() only extends a DEFINE_GUARD(). So I understand
if we need to make names that fit a certain pattern rather than what
I suggested.

Still would be nice to have:

iio_device_claim_mode()
iio_device_claim_mode_direct()
iio_device_claim_mode_buffer()
iio_device_release_mode()

Just really annoying to rename iio_device_{claim,release}_direct()
everywhere since we just did that. We could keep both names around
for a while though to avoid the churn.

It also means that we should remove __iio_device_release_direct() and
iio_device_release_buffer_mode() to make it clear that there is only
a single "release" function used by all variants of "claim".

>>  extern const struct bus_type iio_bus_type;
>>  
>>  /**
>>
> 

Re: [PATCH RFC 6/6] iio: light: opt4060: Use cleanup.h for IIO locks

[David Lechner]

On 12/3/25 1:18 PM, Kurt Borja wrote:
> Simplify and drop "hacky" busy-waiting code in
> opt4060_set_driver_state() by using guard().
> 
> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
> ---
>  drivers/iio/light/opt4060.c | 52 +++++++++++++++------------------------------
>  1 file changed, 17 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/iio/light/opt4060.c b/drivers/iio/light/opt4060.c
> index 500899d7bd62..903963606143 100644
> --- a/drivers/iio/light/opt4060.c
> +++ b/drivers/iio/light/opt4060.c
> @@ -22,6 +22,7 @@
>  #include <linux/iio/trigger.h>
>  #include <linux/iio/trigger_consumer.h>
>  #include <linux/iio/triggered_buffer.h>
> +#include <linux/cleanup.h>
>  
>  /* OPT4060 register set */
>  #define OPT4060_RED_MSB				0x00
> @@ -302,41 +303,22 @@ static int opt4060_set_driver_state(struct iio_dev *indio_dev,
>  				    bool continuous_irq)
>  {
>  	struct opt4060_chip *chip = iio_priv(indio_dev);
> -	int ret = 0;
> -any_mode_retry:
> -	if (!iio_device_claim_buffer(indio_dev)) {
> -		/*
> -		 * This one is a *bit* hacky. If we cannot claim buffer mode,
> -		 * then try direct mode so that we make sure things cannot
> -		 * concurrently change. And we just keep trying until we get one
> -		 * of the modes...
> -		 */
> -		if (!iio_device_claim_direct(indio_dev))
> -			goto any_mode_retry;
> -		/*
> -		 * This path means that we managed to claim direct mode. In
> -		 * this case the buffer isn't enabled and it's okay to leave
> -		 * continuous mode for sampling and/or irq.
> -		 */
> -		ret = opt4060_set_state_common(chip, continuous_sampling,
> -					       continuous_irq);
> -		iio_device_release_direct(indio_dev);
> -		return ret;
> -	} else {
> -		/*
> -		 * This path means that we managed to claim buffer mode. In
> -		 * this case the buffer is enabled and irq and sampling must go
> -		 * to or remain continuous, but only if the trigger is from this
> -		 * device.
> -		 */
> -		if (!iio_trigger_validate_own_device(indio_dev->trig, indio_dev))
> -			ret = opt4060_set_state_common(chip, true, true);
> -		else
> -			ret = opt4060_set_state_common(chip, continuous_sampling,
> -						       continuous_irq);
> -		iio_device_release_buffer(indio_dev);
> -	}
> -	return ret;
> +
> +	guard(iio_device_claim)(indio_dev);
> +
> +	/*
> +	 * If we manage to claim buffer mode and we are using our own trigger,
> +	 * IRQ and sampling must go to or remain continuous.
> +	 */
> +	if (iio_buffer_enabled(indio_dev) && iio_trigger_using_own(indio_dev))

The commit message does not explain why this was changed from
iio_trigger_validate_own_device() to iio_trigger_using_own(). I'm guessing
that is why we also dropped the `!`?

> +		return opt4060_set_state_common(chip, true, true);
> +
> +	/*
> +	 * This path means that we managed to claim direct mode. In this case
> +	 * the buffer isn't enabled and it's okay to leave continuous mode for
> +	 * sampling and/or irq.
> +	 */
> +	return opt4060_set_state_common(chip, continuous_sampling, continuous_irq);
>  }
>  
>  /*
> 

Re: [PATCH RFC 1/6] iio: core: Match iio_device_claim_*() return semantics

[Nuno Sá]

On Wed, 2025-12-03 at 14:18 -0500, Kurt Borja wrote:
> In order to improve API consistency with conditional locks, use
> true/false return semantics in iio_device_claim_buffer_mode().
> 
> This also matches iio_device_claim_direct() return semantics.
> 
> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
> ---

Even if the rest gets a NACK, I think at least this patch makes sense. In fact I
would even extend it so that we have the same inline API with proper annotations:

https://elixir.bootlin.com/linux/v6.18/source/include/linux/iio/iio.h#L679

So it really has the same semantics as iio_device_claim_direct()

- Nuno Sá

>  drivers/iio/adc/ade9000.c                                 | 2 +-
>  drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c | 5 +----
>  drivers/iio/health/max30100.c                             | 2 +-
>  drivers/iio/health/max30102.c                             | 2 +-
>  drivers/iio/industrialio-core.c                           | 8 ++++----
>  drivers/iio/light/opt4060.c                               | 2 +-
>  include/linux/iio/iio.h                                   | 2 +-
>  7 files changed, 10 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/iio/adc/ade9000.c b/drivers/iio/adc/ade9000.c
> index 2de8a718d62a..b345c4d1ef24 100644
> --- a/drivers/iio/adc/ade9000.c
> +++ b/drivers/iio/adc/ade9000.c
> @@ -964,7 +964,7 @@ static irqreturn_t ade9000_dready_thread(int irq, void *data)
>  	struct iio_dev *indio_dev = data;
>  
>  	/* Handle data ready interrupt from C4/EVENT/DREADY pin */
> -	if (!iio_device_claim_buffer_mode(indio_dev)) {
> +	if (iio_device_claim_buffer_mode(indio_dev)) {
>  		ade9000_iio_push_buffer(indio_dev);
>  		iio_device_release_buffer_mode(indio_dev);
>  	}
> 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 9ac80e4b7d75..8ed4b2e410c8 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
> @@ -188,11 +188,8 @@ int cros_ec_sensors_push_data(struct iio_dev *indio_dev,
>  	/*
>  	 * Ignore samples if the buffer is not set: it is needed if the ODR is
>  	 * set but the buffer is not enabled yet.
> -	 *
> -	 * Note: iio_device_claim_buffer_mode() returns -EBUSY if the buffer
> -	 * is not enabled.
>  	 */
> -	if (iio_device_claim_buffer_mode(indio_dev) < 0)
> +	if (!iio_device_claim_buffer_mode(indio_dev))
>  		return 0;
>  
>  	out = (s16 *)st->samples;
> diff --git a/drivers/iio/health/max30100.c b/drivers/iio/health/max30100.c
> index 3d441013893c..3f3680c4b42f 100644
> --- a/drivers/iio/health/max30100.c
> +++ b/drivers/iio/health/max30100.c
> @@ -417,7 +417,7 @@ static int max30100_read_raw(struct iio_dev *indio_dev,
>  		 * Temperature reading can only be acquired while engine
>  		 * is running
>  		 */
> -		if (iio_device_claim_buffer_mode(indio_dev)) {
> +		if (!iio_device_claim_buffer_mode(indio_dev)) {
>  			/*
>  			 * Replacing -EBUSY or other error code
>  			 * returned by iio_device_claim_buffer_mode()
> diff --git a/drivers/iio/health/max30102.c b/drivers/iio/health/max30102.c
> index a48c0881a4c7..288c2f37a4a2 100644
> --- a/drivers/iio/health/max30102.c
> +++ b/drivers/iio/health/max30102.c
> @@ -476,7 +476,7 @@ static int max30102_read_raw(struct iio_dev *indio_dev,
>  		 * shutdown; leave shutdown briefly when buffer not running
>  		 */
>  any_mode_retry:
> -		if (iio_device_claim_buffer_mode(indio_dev)) {
> +		if (!iio_device_claim_buffer_mode(indio_dev)) {
>  			/*
>  			 * This one is a *bit* hacky. If we cannot claim buffer
>  			 * mode, then try direct mode so that we make sure
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index f69deefcfb6f..a10590ac4e17 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -2224,19 +2224,19 @@ EXPORT_SYMBOL_GPL(__iio_device_release_direct);
>   *
>   * Use with iio_device_release_buffer_mode().
>   *
> - * Returns: 0 on success, -EBUSY on failure.
> + * Returns: true on success, false on failure.
>   */
> -int iio_device_claim_buffer_mode(struct iio_dev *indio_dev)
> +bool iio_device_claim_buffer_mode(struct iio_dev *indio_dev)
>  {
>  	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
>  
>  	mutex_lock(&iio_dev_opaque->mlock);
>  
>  	if (iio_buffer_enabled(indio_dev))
> -		return 0;
> +		return true;
>  
>  	mutex_unlock(&iio_dev_opaque->mlock);
> -	return -EBUSY;
> +	return false;
>  }
>  EXPORT_SYMBOL_GPL(iio_device_claim_buffer_mode);
>  
> diff --git a/drivers/iio/light/opt4060.c b/drivers/iio/light/opt4060.c
> index 981c704e7df5..8cb3fa38077e 100644
> --- a/drivers/iio/light/opt4060.c
> +++ b/drivers/iio/light/opt4060.c
> @@ -304,7 +304,7 @@ static int opt4060_set_driver_state(struct iio_dev *indio_dev,
>  	struct opt4060_chip *chip = iio_priv(indio_dev);
>  	int ret = 0;
>  any_mode_retry:
> -	if (iio_device_claim_buffer_mode(indio_dev)) {
> +	if (!iio_device_claim_buffer_mode(indio_dev)) {
>  		/*
>  		 * This one is a *bit* hacky. If we cannot claim buffer mode,
>  		 * then try direct mode so that we make sure things cannot
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index 872ebdf0dd77..bf7b7337ff1b 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -687,7 +687,7 @@ static inline void iio_device_release_direct(struct iio_dev *indio_dev)
>  	__release(indio_dev);
>  }
>  
> -int iio_device_claim_buffer_mode(struct iio_dev *indio_dev);
> +bool iio_device_claim_buffer_mode(struct iio_dev *indio_dev);
>  void iio_device_release_buffer_mode(struct iio_dev *indio_dev);
>  
>  extern const struct bus_type iio_bus_type;

Re: [PATCH RFC 0/6] iio: core: Introduce cleanup.h support for mode locks

[Nuno Sá]

On Wed, 2025-12-03 at 14:18 -0500, Kurt Borja wrote:
> Hi,
> 
> In a recent driver review discussion [1], Andy Shevchenko suggested we
> add cleanup.h support for the lock API:
> 
> 	iio_device_claim_{direct,buffer_mode}().

We already went this patch and then reverted it. I guess before we did not had
ACQUIRE() and ACQUIRE_ERR() but I'm not sure that makes it much better. Looking at the
last two patches on how we are handling the buffer mode stuff, I'm really not convinced...

Also, I have doubts sparse can keep up with the __cleanup stuff so I'm not sure the
annotations much make sense if we go down this path. Unless we want to use both 
approaches which is also questionable.

- Nuno Sá

Re: [PATCH RFC 6/6] iio: light: opt4060: Use cleanup.h for IIO locks

[Nuno Sá]

On Wed, 2025-12-03 at 14:18 -0500, Kurt Borja wrote:
> Simplify and drop "hacky" busy-waiting code in
> opt4060_set_driver_state() by using guard().
> 
> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
> ---
>  drivers/iio/light/opt4060.c | 52 +++++++++++++++------------------------------
>  1 file changed, 17 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/iio/light/opt4060.c b/drivers/iio/light/opt4060.c
> index 500899d7bd62..903963606143 100644
> --- a/drivers/iio/light/opt4060.c
> +++ b/drivers/iio/light/opt4060.c
> @@ -22,6 +22,7 @@
>  #include <linux/iio/trigger.h>
>  #include <linux/iio/trigger_consumer.h>
>  #include <linux/iio/triggered_buffer.h>
> +#include <linux/cleanup.h>
>  
>  /* OPT4060 register set */
>  #define OPT4060_RED_MSB				0x00
> @@ -302,41 +303,22 @@ static int opt4060_set_driver_state(struct iio_dev *indio_dev,
>  				    bool continuous_irq)
>  {
>  	struct opt4060_chip *chip = iio_priv(indio_dev);
> -	int ret = 0;
> -any_mode_retry:
> -	if (!iio_device_claim_buffer(indio_dev)) {
> -		/*
> -		 * This one is a *bit* hacky. If we cannot claim buffer mode,
> -		 * then try direct mode so that we make sure things cannot
> -		 * concurrently change. And we just keep trying until we get one
> -		 * of the modes...
> -		 */
> -		if (!iio_device_claim_direct(indio_dev))
> -			goto any_mode_retry;
> -		/*
> -		 * This path means that we managed to claim direct mode. In
> -		 * this case the buffer isn't enabled and it's okay to leave
> -		 * continuous mode for sampling and/or irq.
> -		 */
> -		ret = opt4060_set_state_common(chip, continuous_sampling,
> -					       continuous_irq);
> -		iio_device_release_direct(indio_dev);
> -		return ret;
> -	} else {
> -		/*
> -		 * This path means that we managed to claim buffer mode. In
> -		 * this case the buffer is enabled and irq and sampling must go
> -		 * to or remain continuous, but only if the trigger is from this
> -		 * device.
> -		 */
> -		if (!iio_trigger_validate_own_device(indio_dev->trig, indio_dev))
> -			ret = opt4060_set_state_common(chip, true, true);
> -		else
> -			ret = opt4060_set_state_common(chip, continuous_sampling,
> -						       continuous_irq);
> -		iio_device_release_buffer(indio_dev);
> -	}
> -	return ret;
> +
> +	guard(iio_device_claim)(indio_dev);
> +
> +	/*
> +	 * If we manage to claim buffer mode and we are using our own trigger,
> +	 * IRQ and sampling must go to or remain continuous.
> +	 */
> +	if (iio_buffer_enabled(indio_dev) && iio_trigger_using_own(indio_dev))
> +		return opt4060_set_state_common(chip, true, true);
> +

I think that if we are open coding lock(mlock) plus iio_buffer_enabled, then
iio_device_claim kind of loses it's real meaning. To me

guard(iio_device_claim)(indio_dev);

it's basically iio_dev_lock

- Nuno Sá

> +	/*
> +	 * This path means that we managed to claim direct mode. In this case
> +	 * the buffer isn't enabled and it's okay to leave continuous mode for
> +	 * sampling and/or irq.
> +	 */
> +	return opt4060_set_state_common(chip, continuous_sampling, continuous_irq);
>  }
>  
>  /*

Re: [PATCH RFC 1/6] iio: core: Match iio_device_claim_*() return semantics

[Andy Shevchenko]

On Thu, Dec 4, 2025 at 4:22 PM Nuno Sá <noname.nuno@gmail.com> wrote:
> On Wed, 2025-12-03 at 14:18 -0500, Kurt Borja wrote:
> > In order to improve API consistency with conditional locks, use
> > true/false return semantics in iio_device_claim_buffer_mode().
> >
> > This also matches iio_device_claim_direct() return semantics.

> Even if the rest gets a NACK, I think at least this patch makes sense. In fact I
> would even extend it so that we have the same inline API with proper annotations:
>
> https://elixir.bootlin.com/linux/v6.18/source/include/linux/iio/iio.h#L679
>
> So it really has the same semantics as iio_device_claim_direct()

I remember I looked into this when Jonathan provided an API, but I
have no memory of why we have the -EBUSY which is not used in the
callers.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH RFC 0/6] iio: core: Introduce cleanup.h support for mode locks

[Andy Shevchenko]

On Thu, Dec 4, 2025 at 4:35 PM Nuno Sá <noname.nuno@gmail.com> wrote:
> On Wed, 2025-12-03 at 14:18 -0500, Kurt Borja wrote:
> >
> > In a recent driver review discussion [1], Andy Shevchenko suggested we
> > add cleanup.h support for the lock API:
> >
> >       iio_device_claim_{direct,buffer_mode}().
>
> We already went this patch and then reverted it. I guess before we did not had
> ACQUIRE() and ACQUIRE_ERR() but I'm not sure that makes it much better. Looking at the
> last two patches on how we are handling the buffer mode stuff, I'm really not convinced...
>
> Also, I have doubts sparse can keep up with the __cleanup stuff so I'm not sure the
> annotations much make sense if we go down this path. Unless we want to use both
> approaches which is also questionable.

This, indeed, needs a (broader) discussion and I appreciate that Kurt
sent this RFC. Jonathan, what's your thoughts?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH RFC 5/6] iio: health: max30102: Use cleanup.h for IIO locks

[Kurt Borja]

On Wed Dec 3, 2025 at 4:52 PM -05, David Lechner wrote:
> On 12/3/25 1:18 PM, Kurt Borja wrote:
>> Simplify and drop "hacky" busy-waiting code in max30102_read_raw() by
>> using scoped_guard().
>> 
>> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
>> ---
>>  drivers/iio/health/max30102.c | 24 +++++++-----------------
>>  1 file changed, 7 insertions(+), 17 deletions(-)
>> 
>> diff --git a/drivers/iio/health/max30102.c b/drivers/iio/health/max30102.c
>> index 678720102f2b..c642842cb5fb 100644
>> --- a/drivers/iio/health/max30102.c
>> +++ b/drivers/iio/health/max30102.c
>> @@ -24,6 +24,7 @@
>>  #include <linux/iio/iio.h>
>>  #include <linux/iio/buffer.h>
>>  #include <linux/iio/kfifo_buf.h>
>> +#include <linux/cleanup.h>
>>  
>>  #define MAX30102_DRV_NAME	"max30102"
>>  #define MAX30102_PART_NUMBER	0x15
>> @@ -468,6 +469,7 @@ static int max30102_read_raw(struct iio_dev *indio_dev,
>>  {
>>  	struct max30102_data *data = iio_priv(indio_dev);
>>  	int ret = -EINVAL;
>> +	bool direct_en;
>>  
>>  	switch (mask) {
>>  	case IIO_CHAN_INFO_RAW:
>> @@ -475,25 +477,13 @@ static int max30102_read_raw(struct iio_dev *indio_dev,
>>  		 * Temperature reading can only be acquired when not in
>>  		 * shutdown; leave shutdown briefly when buffer not running
>>  		 */
>> -any_mode_retry:
>> -		if (!iio_device_claim_buffer(indio_dev)) {
>> -			/*
>> -			 * This one is a *bit* hacky. If we cannot claim buffer
>> -			 * mode, then try direct mode so that we make sure
>> -			 * things cannot concurrently change. And we just keep
>> -			 * trying until we get one of the modes...
>> -			 */
>> -			if (!iio_device_claim_direct(indio_dev))
>> -				goto any_mode_retry;
>> +		scoped_guard(iio_device_claim, indio_dev) {
>
> scoped_guard() is sketchy in switch statements because there is
> a hidden for loop. If someone came along later and put a break;
> inside of the scope, it would break out of the hidden for loop
> rather than the apparent switch case!
>
> Besides that, it adds extra indent that we could avoid.
>
>> +			direct_en = !iio_buffer_enabled(indio_dev);
>>  
>> -			ret = max30102_get_temp(data, val, true);
>> -			iio_device_release_direct(indio_dev);
>> -		} else {
>> -			ret = max30102_get_temp(data, val, false);
>> -			iio_device_release_buffer(indio_dev);
>> +			ret = max30102_get_temp(data, val, direct_en);
>> +			if (ret)
>> +				return ret;
>>  		}
>> -		if (ret)
>> -			return ret;
>>  
>>  		ret = IIO_VAL_INT;
>>  		break;
>> 
>
> I would write the whole function like this:
>
> static int max30102_read_raw(struct iio_dev *indio_dev,
> 			     struct iio_chan_spec const *chan,
> 			     int *val, int *val2, long mask)
> {
> 	struct max30102_data *data = iio_priv(indio_dev);
> 	int ret;
>
> 	switch (mask) {
> 	case IIO_CHAN_INFO_RAW: {
> 		/*
> 		 * Temperature reading can only be acquired when not in
> 		 * shutdown; leave shutdown briefly when buffer not running
> 		 */
> 		guard(iio_device_claim)(indio_dev);

AFAIK you can't guard() inside switch-case blocks. I don't know the
exact reason, but it has to be scoped_guard().

> 		ret = max30102_get_temp(data, val, !iio_buffer_enabled(indio_dev));
> 		if (ret)
> 			return ret;
>
> 		return IIO_VAL_INT;
> 	}
> 	case IIO_CHAN_INFO_SCALE:
> 		*val = 1000;  /* 62.5 */
> 		*val2 = 16;
> 		return IIO_VAL_FRACTIONAL;
> 	default:
> 		return -EINVAL;
> 	}
> }
>
> Could also simplify things further by moving the call to iio_buffer_enabled()
> into max30102_get_temp().

I'll do it like this if this survives v2.


-- 
 ~ Kurt

Re: [PATCH RFC 3/6] iio: core: Add cleanup.h support for iio_device_claim_*()

[Kurt Borja]

On Wed Dec 3, 2025 at 5:34 PM -05, David Lechner wrote:
> On 12/3/25 3:50 PM, David Lechner wrote:
>> On 12/3/25 1:18 PM, Kurt Borja wrote:
>>> Add guard() and ACQUIRE() support for iio_device_claim_*() lock.
>>>
>>> This involves exporting iio_device_{claim, release}() wrappers to define
>>> a general GUARD class, and then defining the _direct and _buffer
>>> conditional ones.
>> 
>> Commit messages should say why we need this.
>> 
>> Also, this seems like two separate things. Adding a new claim/release pair
>> and adding the conditional guard support to the existing ones. So perhaps
>> better as two separate patches.
>> 
>>>
>>> Suggested-by: Andy Shevchenko <andriy.shevchenko@intel.com>
>>> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
>>> ---
>>>  drivers/iio/industrialio-core.c | 12 ++++++++++++
>>>  include/linux/iio/iio.h         | 20 ++++++++++++++++++++
>>>  2 files changed, 32 insertions(+)
>>>
>>> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
>>> index adf0142d0300..da090c993fe8 100644
>>> --- a/drivers/iio/industrialio-core.c
>>> +++ b/drivers/iio/industrialio-core.c
>>> @@ -2171,6 +2171,18 @@ int __devm_iio_device_register(struct device *dev, struct iio_dev *indio_dev,
>>>  }
>>>  EXPORT_SYMBOL_GPL(__devm_iio_device_register);
>>>  
>>> +void __iio_device_claim(struct iio_dev *indio_dev)
>>> +{
>>> +	mutex_lock(&to_iio_dev_opaque(indio_dev)->mlock);
>>> +}
>>> +EXPORT_SYMBOL_GPL(__iio_device_claim);
>>> +
>>> +void __iio_device_release(struct iio_dev *indio_dev)
>>> +{
>>> +	mutex_unlock(&to_iio_dev_opaque(indio_dev)->mlock);
>>> +}
>>> +EXPORT_SYMBOL_GPL(__iio_device_release);
>>> +
>>>  /**
>>>   * __iio_device_claim_direct - Keep device in direct mode
>>>   * @indio_dev:	the iio_dev associated with the device
>>> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
>>> index 27da9af67c47..472b13ec28d3 100644
>>> --- a/include/linux/iio/iio.h
>>> +++ b/include/linux/iio/iio.h
>>> @@ -10,6 +10,7 @@
>>>  #include <linux/align.h>
>>>  #include <linux/device.h>
>>>  #include <linux/cdev.h>
>>> +#include <linux/cleanup.h>
>>>  #include <linux/compiler_types.h>
>>>  #include <linux/minmax.h>
>>>  #include <linux/slab.h>
>>> @@ -661,9 +662,23 @@ void iio_device_unregister(struct iio_dev *indio_dev);
>>>  int __devm_iio_device_register(struct device *dev, struct iio_dev *indio_dev,
>>>  			       struct module *this_mod);
>>>  int iio_push_event(struct iio_dev *indio_dev, u64 ev_code, s64 timestamp);
>>> +void __iio_device_claim(struct iio_dev *indio_dev);
>>> +void __iio_device_release(struct iio_dev *indio_dev);
>>>  bool __iio_device_claim_direct(struct iio_dev *indio_dev);
>>>  void __iio_device_release_direct(struct iio_dev *indio_dev);
>>>  
>>> +static inline void iio_device_claim(struct iio_dev *indio_dev)
>>> +	__acquires(indio_dev)
>>> +{
>>> +	__iio_device_claim(indio_dev);
>>> +}
>>> +
>>> +static inline void iio_device_release(struct iio_dev *indio_dev)
>>> +	__releases(indio_dev)
>>> +{
>>> +	__iio_device_release(indio_dev);
>>> +}
>> 
>> It was unfortunate that we had to drop "mode" from iio_device_claim_direct_mode()
>> during the recent API change, but at least it is fairly obvious that "direct"
>> is a mode. Here, dropping "mode" from the name hurts the understanding. These
>> could also use some documentation comments to explain what these are for and
>> when it is appropriate to use them. I had to really dig around the code to
>> come to the understanding that these mean "don't allow switching modes until
>> we release the claim".

I agree.

>> 
>> I would call it something like iio_device_{claim,release}_current_mode().
>> 
>> 
>>> +
>>>  /*
>>>   * Helper functions that allow claim and release of direct mode
>>>   * in a fashion that doesn't generate many false positives from sparse.
>>> @@ -690,6 +705,11 @@ static inline void iio_device_release_direct(struct iio_dev *indio_dev)
>>>  bool iio_device_claim_buffer(struct iio_dev *indio_dev);
>>>  void iio_device_release_buffer(struct iio_dev *indio_dev);
>>>  
>>> +DEFINE_GUARD(iio_device_claim, struct iio_dev *, iio_device_claim(_T),
>>> +	     iio_device_release(_T));
>>> +DEFINE_GUARD_COND(iio_device_claim, _buffer, iio_device_claim_buffer(_T));
>>> +DEFINE_GUARD_COND(iio_device_claim, _direct, iio_device_claim_direct(_T));
>>> +
>
> When I made the comments about keeping "mode" in the name, I forgot
> that DEFINE_GUARD_COND() only extends a DEFINE_GUARD(). So I understand
> if we need to make names that fit a certain pattern rather than what
> I suggested.
>
> Still would be nice to have:
>
> iio_device_claim_mode()
> iio_device_claim_mode_direct()
> iio_device_claim_mode_buffer()
> iio_device_release_mode()
>
> Just really annoying to rename iio_device_{claim,release}_direct()
> everywhere since we just did that. We could keep both names around
> for a while though to avoid the churn.

If we rename iio_device_claim_direct() (which is huge), maybe we can
pick shorter names and more descriptive names while at it? I was
thinking something like:

	iio_mode_lock()
	iio_mode_lock_direct()
	iio_mode_lock_buffer()
	iio_mode_unlock()

Shorter names will also keep lines short when using guards.

>
> It also means that we should remove __iio_device_release_direct() and
> iio_device_release_buffer_mode() to make it clear that there is only
> a single "release" function used by all variants of "claim".

I agree.

-- 
 ~ Kurt

Re: [PATCH RFC 6/6] iio: light: opt4060: Use cleanup.h for IIO locks

[Kurt Borja]

On Wed Dec 3, 2025 at 5:40 PM -05, David Lechner wrote:
> On 12/3/25 1:18 PM, Kurt Borja wrote:
>> Simplify and drop "hacky" busy-waiting code in
>> opt4060_set_driver_state() by using guard().
>> 
>> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
>> ---
>>  drivers/iio/light/opt4060.c | 52 +++++++++++++++------------------------------
>>  1 file changed, 17 insertions(+), 35 deletions(-)
>> 
>> diff --git a/drivers/iio/light/opt4060.c b/drivers/iio/light/opt4060.c
>> index 500899d7bd62..903963606143 100644
>> --- a/drivers/iio/light/opt4060.c
>> +++ b/drivers/iio/light/opt4060.c
>> @@ -22,6 +22,7 @@
>>  #include <linux/iio/trigger.h>
>>  #include <linux/iio/trigger_consumer.h>
>>  #include <linux/iio/triggered_buffer.h>
>> +#include <linux/cleanup.h>
>>  
>>  /* OPT4060 register set */
>>  #define OPT4060_RED_MSB				0x00
>> @@ -302,41 +303,22 @@ static int opt4060_set_driver_state(struct iio_dev *indio_dev,
>>  				    bool continuous_irq)
>>  {
>>  	struct opt4060_chip *chip = iio_priv(indio_dev);
>> -	int ret = 0;
>> -any_mode_retry:
>> -	if (!iio_device_claim_buffer(indio_dev)) {
>> -		/*
>> -		 * This one is a *bit* hacky. If we cannot claim buffer mode,
>> -		 * then try direct mode so that we make sure things cannot
>> -		 * concurrently change. And we just keep trying until we get one
>> -		 * of the modes...
>> -		 */
>> -		if (!iio_device_claim_direct(indio_dev))
>> -			goto any_mode_retry;
>> -		/*
>> -		 * This path means that we managed to claim direct mode. In
>> -		 * this case the buffer isn't enabled and it's okay to leave
>> -		 * continuous mode for sampling and/or irq.
>> -		 */
>> -		ret = opt4060_set_state_common(chip, continuous_sampling,
>> -					       continuous_irq);
>> -		iio_device_release_direct(indio_dev);
>> -		return ret;
>> -	} else {
>> -		/*
>> -		 * This path means that we managed to claim buffer mode. In
>> -		 * this case the buffer is enabled and irq and sampling must go
>> -		 * to or remain continuous, but only if the trigger is from this
>> -		 * device.
>> -		 */
>> -		if (!iio_trigger_validate_own_device(indio_dev->trig, indio_dev))
>> -			ret = opt4060_set_state_common(chip, true, true);
>> -		else
>> -			ret = opt4060_set_state_common(chip, continuous_sampling,
>> -						       continuous_irq);
>> -		iio_device_release_buffer(indio_dev);
>> -	}
>> -	return ret;
>> +
>> +	guard(iio_device_claim)(indio_dev);
>> +
>> +	/*
>> +	 * If we manage to claim buffer mode and we are using our own trigger,
>> +	 * IRQ and sampling must go to or remain continuous.
>> +	 */
>> +	if (iio_buffer_enabled(indio_dev) && iio_trigger_using_own(indio_dev))
>
> The commit message does not explain why this was changed from
> iio_trigger_validate_own_device() to iio_trigger_using_own(). I'm guessing
> that is why we also dropped the `!`?

Hmm I just assumed this functions were equivalent, but looking at the
code is not directly obvious.

I will look into it and mention this in the commit message.


-- 
 ~ Kurt

Re: [PATCH RFC 1/6] iio: core: Match iio_device_claim_*() return semantics

[Kurt Borja]

On Thu Dec 4, 2025 at 9:23 AM -05, Nuno Sá wrote:
> On Wed, 2025-12-03 at 14:18 -0500, Kurt Borja wrote:
>> In order to improve API consistency with conditional locks, use
>> true/false return semantics in iio_device_claim_buffer_mode().
>> 
>> This also matches iio_device_claim_direct() return semantics.
>> 
>> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
>> ---
>
> Even if the rest gets a NACK, I think at least this patch makes sense. In fact I
> would even extend it so that we have the same inline API with proper annotations:
>
> https://elixir.bootlin.com/linux/v6.18/source/include/linux/iio/iio.h#L679
>
> So it really has the same semantics as iio_device_claim_direct()

I agree. I will include that here and just call it match semantics.

>
> - Nuno Sá


-- 
 ~ Kurt

Re: [PATCH RFC 6/6] iio: light: opt4060: Use cleanup.h for IIO locks

[Kurt Borja]

On Thu Dec 4, 2025 at 9:42 AM -05, Nuno Sá wrote:
> On Wed, 2025-12-03 at 14:18 -0500, Kurt Borja wrote:
>> Simplify and drop "hacky" busy-waiting code in
>> opt4060_set_driver_state() by using guard().
>> 
>> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
>> ---
>>  drivers/iio/light/opt4060.c | 52 +++++++++++++++------------------------------
>>  1 file changed, 17 insertions(+), 35 deletions(-)
>> 
>> diff --git a/drivers/iio/light/opt4060.c b/drivers/iio/light/opt4060.c
>> index 500899d7bd62..903963606143 100644
>> --- a/drivers/iio/light/opt4060.c
>> +++ b/drivers/iio/light/opt4060.c
>> @@ -22,6 +22,7 @@
>>  #include <linux/iio/trigger.h>
>>  #include <linux/iio/trigger_consumer.h>
>>  #include <linux/iio/triggered_buffer.h>
>> +#include <linux/cleanup.h>
>>  
>>  /* OPT4060 register set */
>>  #define OPT4060_RED_MSB				0x00
>> @@ -302,41 +303,22 @@ static int opt4060_set_driver_state(struct iio_dev *indio_dev,
>>  				    bool continuous_irq)
>>  {
>>  	struct opt4060_chip *chip = iio_priv(indio_dev);
>> -	int ret = 0;
>> -any_mode_retry:
>> -	if (!iio_device_claim_buffer(indio_dev)) {
>> -		/*
>> -		 * This one is a *bit* hacky. If we cannot claim buffer mode,
>> -		 * then try direct mode so that we make sure things cannot
>> -		 * concurrently change. And we just keep trying until we get one
>> -		 * of the modes...
>> -		 */
>> -		if (!iio_device_claim_direct(indio_dev))
>> -			goto any_mode_retry;
>> -		/*
>> -		 * This path means that we managed to claim direct mode. In
>> -		 * this case the buffer isn't enabled and it's okay to leave
>> -		 * continuous mode for sampling and/or irq.
>> -		 */
>> -		ret = opt4060_set_state_common(chip, continuous_sampling,
>> -					       continuous_irq);
>> -		iio_device_release_direct(indio_dev);
>> -		return ret;
>> -	} else {
>> -		/*
>> -		 * This path means that we managed to claim buffer mode. In
>> -		 * this case the buffer is enabled and irq and sampling must go
>> -		 * to or remain continuous, but only if the trigger is from this
>> -		 * device.
>> -		 */
>> -		if (!iio_trigger_validate_own_device(indio_dev->trig, indio_dev))
>> -			ret = opt4060_set_state_common(chip, true, true);
>> -		else
>> -			ret = opt4060_set_state_common(chip, continuous_sampling,
>> -						       continuous_irq);
>> -		iio_device_release_buffer(indio_dev);
>> -	}
>> -	return ret;
>> +
>> +	guard(iio_device_claim)(indio_dev);
>> +
>> +	/*
>> +	 * If we manage to claim buffer mode and we are using our own trigger,
>> +	 * IRQ and sampling must go to or remain continuous.
>> +	 */
>> +	if (iio_buffer_enabled(indio_dev) && iio_trigger_using_own(indio_dev))
>> +		return opt4060_set_state_common(chip, true, true);
>> +
>
> I think that if we are open coding lock(mlock) plus iio_buffer_enabled, then
> iio_device_claim kind of loses it's real meaning. To me
>
> guard(iio_device_claim)(indio_dev);
>
> it's basically iio_dev_lock

It is! That's why I think if we go down this route we should rename the
API as proposed in [1].

[1] https://lore.kernel.org/linux-iio/DEPLQT84HBAO.2GAY5BHP05HNL@gmail.com/


-- 
 ~ Kurt

Re: [PATCH RFC 0/6] iio: core: Introduce cleanup.h support for mode locks

[Kurt Borja]

On Thu Dec 4, 2025 at 9:36 AM -05, Nuno Sá wrote:
> On Wed, 2025-12-03 at 14:18 -0500, Kurt Borja wrote:
>> Hi,
>> 
>> In a recent driver review discussion [1], Andy Shevchenko suggested we
>> add cleanup.h support for the lock API:
>> 
>> 	iio_device_claim_{direct,buffer_mode}().
>
> We already went this patch and then reverted it. I guess before we did not had
> ACQUIRE() and ACQUIRE_ERR() but I'm not sure that makes it much better. Looking at the
> last two patches on how we are handling the buffer mode stuff, I'm really not convinced...
>
> Also, I have doubts sparse can keep up with the __cleanup stuff so I'm not sure the
> annotations much make sense if we go down this path. Unless we want to use both 
> approaches which is also questionable.

I think if we add iio_device_claim() or whatever the final name may be,
we can annotate that instead with acquires(&mlock) and maybe it could
work?

I will test that.

>
> - Nuno Sá


-- 
 ~ Kurt

Re: [PATCH RFC 5/6] iio: health: max30102: Use cleanup.h for IIO locks

[David Lechner]

On 12/4/25 11:07 AM, Kurt Borja wrote:
> On Wed Dec 3, 2025 at 4:52 PM -05, David Lechner wrote:
>> On 12/3/25 1:18 PM, Kurt Borja wrote:

...

>> I would write the whole function like this:
>>
>> static int max30102_read_raw(struct iio_dev *indio_dev,
>> 			     struct iio_chan_spec const *chan,
>> 			     int *val, int *val2, long mask)
>> {
>> 	struct max30102_data *data = iio_priv(indio_dev);
>> 	int ret;
>>
>> 	switch (mask) {
>> 	case IIO_CHAN_INFO_RAW: {
>> 		/*
>> 		 * Temperature reading can only be acquired when not in
>> 		 * shutdown; leave shutdown briefly when buffer not running
>> 		 */
>> 		guard(iio_device_claim)(indio_dev);
> 
> AFAIK you can't guard() inside switch-case blocks. I don't know the
> exact reason, but it has to be scoped_guard().

You can. You just need the braces like I showed in my suggestion.

The reason is that guard() is declaring local variables and some
compilers like LLVM don't like declaring local variables in a
switch case. By adding the { } scope, the variables are limited
to that scope and the issue goes away.

Re: [PATCH RFC 2/6] iio: core: Match iio_device_claim_*() naming

[Kurt Borja]

On Wed Dec 3, 2025 at 4:50 PM -05, David Lechner wrote:
> On 12/3/25 1:18 PM, Kurt Borja wrote:
>> Rename iio_device_claim_buffer_mode() -> iio_device_claim_buffer() to
>> match iio_device_claim_direct().
>> 
>> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
>> ---
> If we decide to do this, I would squash this with the previous patch
> to make a clean break of it. Although it is helpful to have "mode"
> in the name if we can keep that without breaking things.

Agree, but If rename iio_device_claim_direct() that would be too big and
I think it should be separate patches.


-- 
 ~ Kurt

Re: [PATCH RFC 3/6] iio: core: Add cleanup.h support for iio_device_claim_*()

[Andy Shevchenko]

On Thu, Dec 4, 2025 at 7:18 PM Kurt Borja <kuurtb@gmail.com> wrote:
> On Wed Dec 3, 2025 at 5:34 PM -05, David Lechner wrote:
> > On 12/3/25 3:50 PM, David Lechner wrote:
> >> On 12/3/25 1:18 PM, Kurt Borja wrote:

...

> > When I made the comments about keeping "mode" in the name, I forgot
> > that DEFINE_GUARD_COND() only extends a DEFINE_GUARD(). So I understand
> > if we need to make names that fit a certain pattern rather than what
> > I suggested.
> >
> > Still would be nice to have:
> >
> > iio_device_claim_mode()
> > iio_device_claim_mode_direct()
> > iio_device_claim_mode_buffer()
> > iio_device_release_mode()
> >
> > Just really annoying to rename iio_device_{claim,release}_direct()
> > everywhere since we just did that. We could keep both names around
> > for a while though to avoid the churn.
>
> If we rename iio_device_claim_direct() (which is huge), maybe we can
> pick shorter names and more descriptive names while at it? I was
> thinking something like:
>
>         iio_mode_lock()
>         iio_mode_lock_direct()
>         iio_mode_lock_buffer()
>         iio_mode_unlock()

The device context is important, so at least iio_dev_mode_lock() (and so on).

> Shorter names will also keep lines short when using guards.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH RFC 5/6] iio: health: max30102: Use cleanup.h for IIO locks

[Kurt Borja]

On Thu Dec 4, 2025 at 12:35 PM -05, David Lechner wrote:
> On 12/4/25 11:07 AM, Kurt Borja wrote:
>> On Wed Dec 3, 2025 at 4:52 PM -05, David Lechner wrote:
>>> On 12/3/25 1:18 PM, Kurt Borja wrote:
>
> ...
>
>>> I would write the whole function like this:
>>>
>>> static int max30102_read_raw(struct iio_dev *indio_dev,
>>> 			     struct iio_chan_spec const *chan,
>>> 			     int *val, int *val2, long mask)
>>> {
>>> 	struct max30102_data *data = iio_priv(indio_dev);
>>> 	int ret;
>>>
>>> 	switch (mask) {
>>> 	case IIO_CHAN_INFO_RAW: {
>>> 		/*
>>> 		 * Temperature reading can only be acquired when not in
>>> 		 * shutdown; leave shutdown briefly when buffer not running
>>> 		 */
>>> 		guard(iio_device_claim)(indio_dev);
>> 
>> AFAIK you can't guard() inside switch-case blocks. I don't know the
>> exact reason, but it has to be scoped_guard().
>
> You can. You just need the braces like I showed in my suggestion.
>
> The reason is that guard() is declaring local variables and some
> compilers like LLVM don't like declaring local variables in a
> switch case. By adding the { } scope, the variables are limited
> to that scope and the issue goes away.

Ah - You're right. I dind't see the braces.

Now it is overly complicated. In this case I see why guard() isn't
really an improvement.


-- 
 ~ Kurt

Re: [PATCH RFC 1/6] iio: core: Match iio_device_claim_*() return semantics

[Jonathan Cameron]

On Thu, 04 Dec 2025 14:23:19 +0000
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Wed, 2025-12-03 at 14:18 -0500, Kurt Borja wrote:
> > In order to improve API consistency with conditional locks, use
> > true/false return semantics in iio_device_claim_buffer_mode().
> > 
> > This also matches iio_device_claim_direct() return semantics.
> > 
> > Signed-off-by: Kurt Borja <kuurtb@gmail.com>
> > ---  
> 
> Even if the rest gets a NACK, I think at least this patch makes sense. In fact I
> would even extend it so that we have the same inline API with proper annotations:
> 
> https://elixir.bootlin.com/linux/v6.18/source/include/linux/iio/iio.h#L679
> 
> So it really has the same semantics as iio_device_claim_direct()

Yeah. This was on my mental todo list. So great to see Kurt
dealing with it! :)  These calls are much rarer than the direct ones
so I wasn't so fussed about getting the sparse coverage. Makes
sense to do it just wasn't high priority.

As Nuno, suggested though I would like the sparse support for these.

Jonathan

> 
> - Nuno Sá
> 
> >  drivers/iio/adc/ade9000.c                                 | 2 +-
> >  drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c | 5 +----
> >  drivers/iio/health/max30100.c                             | 2 +-
> >  drivers/iio/health/max30102.c                             | 2 +-
> >  drivers/iio/industrialio-core.c                           | 8 ++++----
> >  drivers/iio/light/opt4060.c                               | 2 +-
> >  include/linux/iio/iio.h                                   | 2 +-
> >  7 files changed, 10 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/ade9000.c b/drivers/iio/adc/ade9000.c
> > index 2de8a718d62a..b345c4d1ef24 100644
> > --- a/drivers/iio/adc/ade9000.c
> > +++ b/drivers/iio/adc/ade9000.c
> > @@ -964,7 +964,7 @@ static irqreturn_t ade9000_dready_thread(int irq, void *data)
> >  	struct iio_dev *indio_dev = data;
> >  
> >  	/* Handle data ready interrupt from C4/EVENT/DREADY pin */
> > -	if (!iio_device_claim_buffer_mode(indio_dev)) {
> > +	if (iio_device_claim_buffer_mode(indio_dev)) {
> >  		ade9000_iio_push_buffer(indio_dev);
> >  		iio_device_release_buffer_mode(indio_dev);
> >  	}
> > 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 9ac80e4b7d75..8ed4b2e410c8 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
> > @@ -188,11 +188,8 @@ int cros_ec_sensors_push_data(struct iio_dev *indio_dev,
> >  	/*
> >  	 * Ignore samples if the buffer is not set: it is needed if the ODR is
> >  	 * set but the buffer is not enabled yet.
> > -	 *
> > -	 * Note: iio_device_claim_buffer_mode() returns -EBUSY if the buffer
> > -	 * is not enabled.
> >  	 */
> > -	if (iio_device_claim_buffer_mode(indio_dev) < 0)
> > +	if (!iio_device_claim_buffer_mode(indio_dev))
> >  		return 0;
> >  
> >  	out = (s16 *)st->samples;
> > diff --git a/drivers/iio/health/max30100.c b/drivers/iio/health/max30100.c
> > index 3d441013893c..3f3680c4b42f 100644
> > --- a/drivers/iio/health/max30100.c
> > +++ b/drivers/iio/health/max30100.c
> > @@ -417,7 +417,7 @@ static int max30100_read_raw(struct iio_dev *indio_dev,
> >  		 * Temperature reading can only be acquired while engine
> >  		 * is running
> >  		 */
> > -		if (iio_device_claim_buffer_mode(indio_dev)) {
> > +		if (!iio_device_claim_buffer_mode(indio_dev)) {
> >  			/*
> >  			 * Replacing -EBUSY or other error code
> >  			 * returned by iio_device_claim_buffer_mode()
> > diff --git a/drivers/iio/health/max30102.c b/drivers/iio/health/max30102.c
> > index a48c0881a4c7..288c2f37a4a2 100644
> > --- a/drivers/iio/health/max30102.c
> > +++ b/drivers/iio/health/max30102.c
> > @@ -476,7 +476,7 @@ static int max30102_read_raw(struct iio_dev *indio_dev,
> >  		 * shutdown; leave shutdown briefly when buffer not running
> >  		 */
> >  any_mode_retry:
> > -		if (iio_device_claim_buffer_mode(indio_dev)) {
> > +		if (!iio_device_claim_buffer_mode(indio_dev)) {
> >  			/*
> >  			 * This one is a *bit* hacky. If we cannot claim buffer
> >  			 * mode, then try direct mode so that we make sure
> > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> > index f69deefcfb6f..a10590ac4e17 100644
> > --- a/drivers/iio/industrialio-core.c
> > +++ b/drivers/iio/industrialio-core.c
> > @@ -2224,19 +2224,19 @@ EXPORT_SYMBOL_GPL(__iio_device_release_direct);
> >   *
> >   * Use with iio_device_release_buffer_mode().
> >   *
> > - * Returns: 0 on success, -EBUSY on failure.
> > + * Returns: true on success, false on failure.
> >   */
> > -int iio_device_claim_buffer_mode(struct iio_dev *indio_dev)
> > +bool iio_device_claim_buffer_mode(struct iio_dev *indio_dev)
> >  {
> >  	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> >  
> >  	mutex_lock(&iio_dev_opaque->mlock);
> >  
> >  	if (iio_buffer_enabled(indio_dev))
> > -		return 0;
> > +		return true;
> >  
> >  	mutex_unlock(&iio_dev_opaque->mlock);
> > -	return -EBUSY;
> > +	return false;
> >  }
> >  EXPORT_SYMBOL_GPL(iio_device_claim_buffer_mode);
> >  
> > diff --git a/drivers/iio/light/opt4060.c b/drivers/iio/light/opt4060.c
> > index 981c704e7df5..8cb3fa38077e 100644
> > --- a/drivers/iio/light/opt4060.c
> > +++ b/drivers/iio/light/opt4060.c
> > @@ -304,7 +304,7 @@ static int opt4060_set_driver_state(struct iio_dev *indio_dev,
> >  	struct opt4060_chip *chip = iio_priv(indio_dev);
> >  	int ret = 0;
> >  any_mode_retry:
> > -	if (iio_device_claim_buffer_mode(indio_dev)) {
> > +	if (!iio_device_claim_buffer_mode(indio_dev)) {
> >  		/*
> >  		 * This one is a *bit* hacky. If we cannot claim buffer mode,
> >  		 * then try direct mode so that we make sure things cannot
> > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> > index 872ebdf0dd77..bf7b7337ff1b 100644
> > --- a/include/linux/iio/iio.h
> > +++ b/include/linux/iio/iio.h
> > @@ -687,7 +687,7 @@ static inline void iio_device_release_direct(struct iio_dev *indio_dev)
> >  	__release(indio_dev);
> >  }
> >  
> > -int iio_device_claim_buffer_mode(struct iio_dev *indio_dev);
> > +bool iio_device_claim_buffer_mode(struct iio_dev *indio_dev);
> >  void iio_device_release_buffer_mode(struct iio_dev *indio_dev);
> >  
> >  extern const struct bus_type iio_bus_type;  

Re: [PATCH RFC 1/6] iio: core: Match iio_device_claim_*() return semantics

[Jonathan Cameron]

On Thu, 4 Dec 2025 17:05:29 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Thu, Dec 4, 2025 at 4:22 PM Nuno Sá <noname.nuno@gmail.com> wrote:
> > On Wed, 2025-12-03 at 14:18 -0500, Kurt Borja wrote:  
> > > In order to improve API consistency with conditional locks, use
> > > true/false return semantics in iio_device_claim_buffer_mode().
> > >
> > > This also matches iio_device_claim_direct() return semantics.  
> 
> > Even if the rest gets a NACK, I think at least this patch makes sense. In fact I
> > would even extend it so that we have the same inline API with proper annotations:
> >
> > https://elixir.bootlin.com/linux/v6.18/source/include/linux/iio/iio.h#L679
> >
> > So it really has the same semantics as iio_device_claim_direct()  
> 
> I remember I looked into this when Jonathan provided an API, but I
> have no memory of why we have the -EBUSY which is not used in the
> callers.

Random historical choice.   I think at the time I vaguely thought
we might have other return values, but they never surfaces so these
might as well have always return booleans.

Jonathan

Re: [PATCH RFC 2/6] iio: core: Match iio_device_claim_*() naming

[Jonathan Cameron]

On Thu, 04 Dec 2025 12:35:38 -0500
"Kurt Borja" <kuurtb@gmail.com> wrote:

> On Wed Dec 3, 2025 at 4:50 PM -05, David Lechner wrote:
> > On 12/3/25 1:18 PM, Kurt Borja wrote:  
> >> Rename iio_device_claim_buffer_mode() -> iio_device_claim_buffer() to
> >> match iio_device_claim_direct().
> >> 
> >> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
> >> ---  
> > If we decide to do this, I would squash this with the previous patch
> > to make a clean break of it. Although it is helpful to have "mode"
> > in the name if we can keep that without breaking things.  
> 
> Agree, but If rename iio_device_claim_direct() that would be too big and
> I think it should be separate patches.
> 
For the iio_device_claim() there were far to many drivers to do
the conversions in a single patch hence I needed a different name
and took the view the _mode() wasn't that important.

We already had precedence in the _scoped() variant that I was ripping
out (the revert David refers to in the cover letter discussion).

As you say it is probably not worth the effort of putting the _mode()
prefix back and things are inconsistent.

So I think this is on balance the most practical way to get to a
consistent ABI again.  However as suggested, if we agree to go this
way squash with previous patch.

Jonathan

Re: [PATCH RFC 5/6] iio: health: max30102: Use cleanup.h for IIO locks

[Jonathan Cameron]

On Thu, 04 Dec 2025 12:47:08 -0500
"Kurt Borja" <kuurtb@gmail.com> wrote:

> On Thu Dec 4, 2025 at 12:35 PM -05, David Lechner wrote:
> > On 12/4/25 11:07 AM, Kurt Borja wrote:  
> >> On Wed Dec 3, 2025 at 4:52 PM -05, David Lechner wrote:  
> >>> On 12/3/25 1:18 PM, Kurt Borja wrote:  
> >
> > ...
> >  
> >>> I would write the whole function like this:
> >>>
> >>> static int max30102_read_raw(struct iio_dev *indio_dev,
> >>> 			     struct iio_chan_spec const *chan,
> >>> 			     int *val, int *val2, long mask)
> >>> {
> >>> 	struct max30102_data *data = iio_priv(indio_dev);
> >>> 	int ret;
> >>>
> >>> 	switch (mask) {
> >>> 	case IIO_CHAN_INFO_RAW: {
> >>> 		/*
> >>> 		 * Temperature reading can only be acquired when not in
> >>> 		 * shutdown; leave shutdown briefly when buffer not running
> >>> 		 */
> >>> 		guard(iio_device_claim)(indio_dev);  
> >> 
> >> AFAIK you can't guard() inside switch-case blocks. I don't know the
> >> exact reason, but it has to be scoped_guard().  
> >
> > You can. You just need the braces like I showed in my suggestion.
> >
> > The reason is that guard() is declaring local variables and some
> > compilers like LLVM don't like declaring local variables in a
> > switch case. By adding the { } scope, the variables are limited
> > to that scope and the issue goes away.  
> 
> Ah - You're right. I dind't see the braces.
> 
> Now it is overly complicated. In this case I see why guard() isn't
> really an improvement.

This bit of guard() usage is fairly well known and I think people are
getting familiar with it.

So I'd prefer the form David suggested. It is nice to get rid
of the mode claiming dance in here.

Jonathan

> 
> 

Re: [PATCH RFC 3/6] iio: core: Add cleanup.h support for iio_device_claim_*()

[Jonathan Cameron]

On Wed, 03 Dec 2025 14:18:17 -0500
Kurt Borja <kuurtb@gmail.com> wrote:

> Add guard() and ACQUIRE() support for iio_device_claim_*() lock.
> 
> This involves exporting iio_device_{claim, release}() wrappers to define
> a general GUARD class, and then defining the _direct and _buffer
> conditional ones.
> 
> Suggested-by: Andy Shevchenko <andriy.shevchenko@intel.com>
> Signed-off-by: Kurt Borja <kuurtb@gmail.com>

I'd like some documentation (alongside the DEFINE_GUARD() stuff)
- particularly warn people that using the non conditional variant
is very much the unusual one.

Perhaps add examples of usage for the other two and ignore that one
on basis alarm bells will ring whenever we see it in code.

Jonathan

> ---
>  drivers/iio/industrialio-core.c | 12 ++++++++++++
>  include/linux/iio/iio.h         | 20 ++++++++++++++++++++
>  2 files changed, 32 insertions(+)
> 
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index adf0142d0300..da090c993fe8 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -2171,6 +2171,18 @@ int __devm_iio_device_register(struct device *dev, struct iio_dev *indio_dev,
>  }
>  EXPORT_SYMBOL_GPL(__devm_iio_device_register);
>  
> +void __iio_device_claim(struct iio_dev *indio_dev)
> +{
> +	mutex_lock(&to_iio_dev_opaque(indio_dev)->mlock);
> +}
> +EXPORT_SYMBOL_GPL(__iio_device_claim);
> +
> +void __iio_device_release(struct iio_dev *indio_dev)
> +{
> +	mutex_unlock(&to_iio_dev_opaque(indio_dev)->mlock);
> +}
> +EXPORT_SYMBOL_GPL(__iio_device_release);
> +
>  /**
>   * __iio_device_claim_direct - Keep device in direct mode
>   * @indio_dev:	the iio_dev associated with the device
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index 27da9af67c47..472b13ec28d3 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -10,6 +10,7 @@
>  #include <linux/align.h>
>  #include <linux/device.h>
>  #include <linux/cdev.h>
> +#include <linux/cleanup.h>
>  #include <linux/compiler_types.h>
>  #include <linux/minmax.h>
>  #include <linux/slab.h>
> @@ -661,9 +662,23 @@ void iio_device_unregister(struct iio_dev *indio_dev);
>  int __devm_iio_device_register(struct device *dev, struct iio_dev *indio_dev,
>  			       struct module *this_mod);
>  int iio_push_event(struct iio_dev *indio_dev, u64 ev_code, s64 timestamp);
> +void __iio_device_claim(struct iio_dev *indio_dev);
> +void __iio_device_release(struct iio_dev *indio_dev);
>  bool __iio_device_claim_direct(struct iio_dev *indio_dev);
>  void __iio_device_release_direct(struct iio_dev *indio_dev);
>  
> +static inline void iio_device_claim(struct iio_dev *indio_dev)
> +	__acquires(indio_dev)
> +{
> +	__iio_device_claim(indio_dev);
> +}
> +
> +static inline void iio_device_release(struct iio_dev *indio_dev)
> +	__releases(indio_dev)
> +{
> +	__iio_device_release(indio_dev);
> +}
> +
>  /*
>   * Helper functions that allow claim and release of direct mode
>   * in a fashion that doesn't generate many false positives from sparse.
> @@ -690,6 +705,11 @@ static inline void iio_device_release_direct(struct iio_dev *indio_dev)
>  bool iio_device_claim_buffer(struct iio_dev *indio_dev);
>  void iio_device_release_buffer(struct iio_dev *indio_dev);
>  
> +DEFINE_GUARD(iio_device_claim, struct iio_dev *, iio_device_claim(_T),
> +	     iio_device_release(_T));
> +DEFINE_GUARD_COND(iio_device_claim, _buffer, iio_device_claim_buffer(_T));
> +DEFINE_GUARD_COND(iio_device_claim, _direct, iio_device_claim_direct(_T));
> +
>  extern const struct bus_type iio_bus_type;
>  
>  /**
> 

Re: [PATCH RFC 3/6] iio: core: Add cleanup.h support for iio_device_claim_*()

[Jonathan Cameron]

On Thu, 4 Dec 2025 19:36:28 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Thu, Dec 4, 2025 at 7:18 PM Kurt Borja <kuurtb@gmail.com> wrote:
> > On Wed Dec 3, 2025 at 5:34 PM -05, David Lechner wrote:  
> > > On 12/3/25 3:50 PM, David Lechner wrote:  
> > >> On 12/3/25 1:18 PM, Kurt Borja wrote:  
> 
> ...
> 
> > > When I made the comments about keeping "mode" in the name, I forgot
> > > that DEFINE_GUARD_COND() only extends a DEFINE_GUARD(). So I understand
> > > if we need to make names that fit a certain pattern rather than what
> > > I suggested.
> > >
> > > Still would be nice to have:
> > >
> > > iio_device_claim_mode()
> > > iio_device_claim_mode_direct()
> > > iio_device_claim_mode_buffer()
> > > iio_device_release_mode()
> > >
> > > Just really annoying to rename iio_device_{claim,release}_direct()
> > > everywhere since we just did that. We could keep both names around
> > > for a while though to avoid the churn.  

Definitely. Possibly indefinitely. I don't want a rename just to make
this facility easier to use as people won't see what is under the
ACQUIRE() anyway if we end up doing something like Rafael has done for
runtime PM where you don't call ACQUIRE() directly but use a runtime pm
specific macro (not sure if that will make this cycle or not, was
still being discussed when I went on holiday).

https://lore.kernel.org/all/3400866.aeNJFYEL58@rafael.j.wysocki/


> >
> > If we rename iio_device_claim_direct() (which is huge), maybe we can
> > pick shorter names and more descriptive names while at it? I was
> > thinking something like:
> >
> >         iio_mode_lock()
> >         iio_mode_lock_direct()
> >         iio_mode_lock_buffer()
> >         iio_mode_unlock()  
> 
> The device context is important, so at least iio_dev_mode_lock() (and so on).

If we are bringing lock into the name do we need to make it explicit which can fail?
Given you can't use them in the wrong place, maybe not.

iio_mode_lock_try_direct() or maybe iio_mode_lock_direct_try()?

This was less relevant when they all could fail.  Maybe we don't need to
bother given how rarely used the unconditional ones are.

I did like the claiming of mode terminology because it made it a little
more clear that we were taking a lock that was there for a purpose rather than
a normal lock.  Also the fact it's a lock is an implementation detail I'd
rather not back into the ABI.

Maybe it's worth something inspired by Rafael's patch linked above?

#define IIO_DEV_ACQUIRE_DIRECT_MODE(_dev, _var) \
	ACQUIRE(iio_device_claim_direct, _var)(_dev);
#define IIO_DEV_ACQUIRE_BUFFER_MODE(_dev, _var) \
	ACQUIRE(iio_device_claim_buffer, _var)(_dev);

For the two more complex ones and fine using guard() for the rare
any mode variant.

Then we can have whatever naming we like for the helpers under
the hood as no one will ever use them directly.

Hohum. Hardest problems in computer science etc, coherency and naming. :)


> 
> > Shorter names will also keep lines short when using guards.  
> 
> 

Re: [PATCH RFC 0/6] iio: core: Introduce cleanup.h support for mode locks

[Jonathan Cameron]

On Thu, 4 Dec 2025 17:07:28 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Thu, Dec 4, 2025 at 4:35 PM Nuno Sá <noname.nuno@gmail.com> wrote:
> > On Wed, 2025-12-03 at 14:18 -0500, Kurt Borja wrote:  
> > >
> > > In a recent driver review discussion [1], Andy Shevchenko suggested we
> > > add cleanup.h support for the lock API:
> > >
> > >       iio_device_claim_{direct,buffer_mode}().  
> >
> > We already went this patch and then reverted it. I guess before we did not had
> > ACQUIRE() and ACQUIRE_ERR() but I'm not sure that makes it much better. Looking at the
> > last two patches on how we are handling the buffer mode stuff, I'm really not convinced...
> >
> > Also, I have doubts sparse can keep up with the __cleanup stuff so I'm not sure the
> > annotations much make sense if we go down this path. Unless we want to use both
> > approaches which is also questionable.  
> 
> This, indeed, needs a (broader) discussion and I appreciate that Kurt
> sent this RFC. Jonathan, what's your thoughts?

I was pretty heavily involved in discussions around ACQUIRE() and it's use
in CXL and runtime PM (though that's still evolving with Rafael trying
to improve the syntax a little).  As you might guess I did have this use
in mind during those discussions.

As far as I know by avoiding the for loop complexity of the previous
try we made and looking (under the hood) like guard() it should be much
easier and safer to use.  Looking at this was on my list, so I'm very happy
to see this series from Kurt exploring how it would be done.

Sparse wise there is no support for now for any of the cleanup.h magic
other than ignoring it.  That doesn't bother me that much though as these
macros create more or less hidden local variables that are hard to mess
with in incorrect ways.

So in general I'm very much in favour of this for same reasons I jumped
in last time (which turned out to be premature!)

This will be particularly useful in avoiding the need for helper functions
in otherwise simple code flows.

Jonathan

Re: [PATCH RFC 3/6] iio: core: Add cleanup.h support for iio_device_claim_*()

[Andy Shevchenko]

On Sat, Dec 06, 2025 at 06:43:32PM +0000, Jonathan Cameron wrote:
> On Thu, 4 Dec 2025 19:36:28 +0200
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Thu, Dec 4, 2025 at 7:18 PM Kurt Borja <kuurtb@gmail.com> wrote:
> > > On Wed Dec 3, 2025 at 5:34 PM -05, David Lechner wrote:  
> > > > On 12/3/25 3:50 PM, David Lechner wrote:  
> > > >> On 12/3/25 1:18 PM, Kurt Borja wrote:  

...

> > > If we rename iio_device_claim_direct() (which is huge), maybe we can
> > > pick shorter names and more descriptive names while at it? I was
> > > thinking something like:
> > >
> > >         iio_mode_lock()
> > >         iio_mode_lock_direct()
> > >         iio_mode_lock_buffer()
> > >         iio_mode_unlock()  
> > 
> > The device context is important, so at least iio_dev_mode_lock() (and so on).
> 
> If we are bringing lock into the name do we need to make it explicit which can fail?
> Given you can't use them in the wrong place, maybe not.
> 
> iio_mode_lock_try_direct() or maybe iio_mode_lock_direct_try()?

For locking the pattern is to have *_trylock()

> This was less relevant when they all could fail.  Maybe we don't need to
> bother given how rarely used the unconditional ones are.
> 
> I did like the claiming of mode terminology because it made it a little
> more clear that we were taking a lock that was there for a purpose rather than
> a normal lock.  Also the fact it's a lock is an implementation detail I'd
> rather not back into the ABI.
> 
> Maybe it's worth something inspired by Rafael's patch linked above?
> 
> #define IIO_DEV_ACQUIRE_DIRECT_MODE(_dev, _var) \
> 	ACQUIRE(iio_device_claim_direct, _var)(_dev);
> #define IIO_DEV_ACQUIRE_BUFFER_MODE(_dev, _var) \
> 	ACQUIRE(iio_device_claim_buffer, _var)(_dev);
> 
> For the two more complex ones and fine using guard() for the rare
> any mode variant.
> 
> Then we can have whatever naming we like for the helpers under
> the hood as no one will ever use them directly.
> 
> Hohum. Hardest problems in computer science etc, coherency and naming. :)
> 
> > > Shorter names will also keep lines short when using guards.  

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH RFC 1/6] iio: core: Match iio_device_claim_*() return semantics

[Kurt Borja]

On Sat Dec 6, 2025 at 1:05 PM -05, Jonathan Cameron wrote:
> On Thu, 04 Dec 2025 14:23:19 +0000
> Nuno Sá <noname.nuno@gmail.com> wrote:
>
>> On Wed, 2025-12-03 at 14:18 -0500, Kurt Borja wrote:
>> > In order to improve API consistency with conditional locks, use
>> > true/false return semantics in iio_device_claim_buffer_mode().
>> > 
>> > This also matches iio_device_claim_direct() return semantics.
>> > 
>> > Signed-off-by: Kurt Borja <kuurtb@gmail.com>
>> > ---  
>> 
>> Even if the rest gets a NACK, I think at least this patch makes sense. In fact I
>> would even extend it so that we have the same inline API with proper annotations:
>> 
>> https://elixir.bootlin.com/linux/v6.18/source/include/linux/iio/iio.h#L679
>> 
>> So it really has the same semantics as iio_device_claim_direct()
>
> Yeah. This was on my mental todo list. So great to see Kurt
> dealing with it! :)  These calls are much rarer than the direct ones
> so I wasn't so fussed about getting the sparse coverage. Makes
> sense to do it just wasn't high priority.
>
> As Nuno, suggested though I would like the sparse support for these.
>
> Jonathan

Hi Jonathan,

Great! I'll squash all "semantics" patches and add sparse support.


-- 
 ~ Kurt

Re: [PATCH RFC 5/6] iio: health: max30102: Use cleanup.h for IIO locks

[Kurt Borja]

On Sat Dec 6, 2025 at 1:17 PM -05, Jonathan Cameron wrote:
> On Thu, 04 Dec 2025 12:47:08 -0500
> "Kurt Borja" <kuurtb@gmail.com> wrote:
>
>> On Thu Dec 4, 2025 at 12:35 PM -05, David Lechner wrote:
>> > On 12/4/25 11:07 AM, Kurt Borja wrote:  
>> >> On Wed Dec 3, 2025 at 4:52 PM -05, David Lechner wrote:  
>> >>> On 12/3/25 1:18 PM, Kurt Borja wrote:  
>> >
>> > ...
>> >  
>> >>> I would write the whole function like this:
>> >>>
>> >>> static int max30102_read_raw(struct iio_dev *indio_dev,
>> >>> 			     struct iio_chan_spec const *chan,
>> >>> 			     int *val, int *val2, long mask)
>> >>> {
>> >>> 	struct max30102_data *data = iio_priv(indio_dev);
>> >>> 	int ret;
>> >>>
>> >>> 	switch (mask) {
>> >>> 	case IIO_CHAN_INFO_RAW: {
>> >>> 		/*
>> >>> 		 * Temperature reading can only be acquired when not in
>> >>> 		 * shutdown; leave shutdown briefly when buffer not running
>> >>> 		 */
>> >>> 		guard(iio_device_claim)(indio_dev);  
>> >> 
>> >> AFAIK you can't guard() inside switch-case blocks. I don't know the
>> >> exact reason, but it has to be scoped_guard().  
>> >
>> > You can. You just need the braces like I showed in my suggestion.
>> >
>> > The reason is that guard() is declaring local variables and some
>> > compilers like LLVM don't like declaring local variables in a
>> > switch case. By adding the { } scope, the variables are limited
>> > to that scope and the issue goes away.  
>> 
>> Ah - You're right. I dind't see the braces.
>> 
>> Now it is overly complicated. In this case I see why guard() isn't
>> really an improvement.
>
> This bit of guard() usage is fairly well known and I think people are
> getting familiar with it.
>
> So I'd prefer the form David suggested. It is nice to get rid
> of the mode claiming dance in here.

Ok, that's good to hear. I'll take David's suggestions for the next
version.

>
> Jonathan


-- 
 ~ Kurt

Re: [PATCH RFC 3/6] iio: core: Add cleanup.h support for iio_device_claim_*()

[Kurt Borja]

On Sat Dec 6, 2025 at 1:20 PM -05, Jonathan Cameron wrote:
> On Wed, 03 Dec 2025 14:18:17 -0500
> Kurt Borja <kuurtb@gmail.com> wrote:
>
>> Add guard() and ACQUIRE() support for iio_device_claim_*() lock.
>> 
>> This involves exporting iio_device_{claim, release}() wrappers to define
>> a general GUARD class, and then defining the _direct and _buffer
>> conditional ones.
>> 
>> Suggested-by: Andy Shevchenko <andriy.shevchenko@intel.com>
>> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
>
> I'd like some documentation (alongside the DEFINE_GUARD() stuff)
> - particularly warn people that using the non conditional variant
> is very much the unusual one.
>
> Perhaps add examples of usage for the other two and ignore that one
> on basis alarm bells will ring whenever we see it in code.

Sure! I'll add it.

>
> Jonathan

-- 
 ~ Kurt

Re: [PATCH RFC 3/6] iio: core: Add cleanup.h support for iio_device_claim_*()

[Kurt Borja]

On Sat Dec 6, 2025 at 1:43 PM -05, Jonathan Cameron wrote:
> On Thu, 4 Dec 2025 19:36:28 +0200
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>
>> On Thu, Dec 4, 2025 at 7:18 PM Kurt Borja <kuurtb@gmail.com> wrote:
>> > On Wed Dec 3, 2025 at 5:34 PM -05, David Lechner wrote:  
>> > > On 12/3/25 3:50 PM, David Lechner wrote:  
>> > >> On 12/3/25 1:18 PM, Kurt Borja wrote:  
>> 
>> ...
>> 
>> > > When I made the comments about keeping "mode" in the name, I forgot
>> > > that DEFINE_GUARD_COND() only extends a DEFINE_GUARD(). So I understand
>> > > if we need to make names that fit a certain pattern rather than what
>> > > I suggested.
>> > >
>> > > Still would be nice to have:
>> > >
>> > > iio_device_claim_mode()
>> > > iio_device_claim_mode_direct()
>> > > iio_device_claim_mode_buffer()
>> > > iio_device_release_mode()
>> > >
>> > > Just really annoying to rename iio_device_{claim,release}_direct()
>> > > everywhere since we just did that. We could keep both names around
>> > > for a while though to avoid the churn.  
>
> Definitely. Possibly indefinitely. I don't want a rename just to make
> this facility easier to use as people won't see what is under the
> ACQUIRE() anyway if we end up doing something like Rafael has done for
> runtime PM where you don't call ACQUIRE() directly but use a runtime pm
> specific macro (not sure if that will make this cycle or not, was
> still being discussed when I went on holiday).
>
> https://lore.kernel.org/all/3400866.aeNJFYEL58@rafael.j.wysocki/

That looks nice.

>
>
>> >
>> > If we rename iio_device_claim_direct() (which is huge), maybe we can
>> > pick shorter names and more descriptive names while at it? I was
>> > thinking something like:
>> >
>> >         iio_mode_lock()
>> >         iio_mode_lock_direct()
>> >         iio_mode_lock_buffer()
>> >         iio_mode_unlock()  
>> 
>> The device context is important, so at least iio_dev_mode_lock() (and so on).
>
> If we are bringing lock into the name do we need to make it explicit which can fail?
> Given you can't use them in the wrong place, maybe not.
>
> iio_mode_lock_try_direct() or maybe iio_mode_lock_direct_try()?

As Andy mentioned, maybe iio_mode_trylock_{direct,buffer}()?

>
> This was less relevant when they all could fail.  Maybe we don't need to
> bother given how rarely used the unconditional ones are.
>
> I did like the claiming of mode terminology because it made it a little
> more clear that we were taking a lock that was there for a purpose rather than
> a normal lock.  Also the fact it's a lock is an implementation detail I'd
> rather not back into the ABI.

Even if it's an implementation detail, from what I've seen, a lot of
drivers might depend on this being a lock.

I my other series (ti-ads1x18), I dropped my private lock in favor of
iio_device_claim_direct() per David's suggestion too.

>
> Maybe it's worth something inspired by Rafael's patch linked above?
>
> #define IIO_DEV_ACQUIRE_DIRECT_MODE(_dev, _var) \
> 	ACQUIRE(iio_device_claim_direct, _var)(_dev);
> #define IIO_DEV_ACQUIRE_BUFFER_MODE(_dev, _var) \
> 	ACQUIRE(iio_device_claim_buffer, _var)(_dev);

I like this a lot, I'll add it here.

>
> For the two more complex ones and fine using guard() for the rare
> any mode variant.
>
> Then we can have whatever naming we like for the helpers under
> the hood as no one will ever use them directly.

Yes, I do think a rename would be nice, but maybe we can leave that for
a (near) future patch.

On that note, should I really rename iio_device_claim_buffer_mode()?
With this macros I don't think is necessary anymore.

>
> Hohum. Hardest problems in computer science etc, coherency and naming. :)

Indeed :)


-- 
 ~ Kurt

Re: [PATCH RFC 0/6] iio: core: Introduce cleanup.h support for mode locks

[Kurt Borja]

On Sat Dec 6, 2025 at 1:46 PM -05, Jonathan Cameron wrote:
> On Thu, 4 Dec 2025 17:07:28 +0200
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>
>> On Thu, Dec 4, 2025 at 4:35 PM Nuno Sá <noname.nuno@gmail.com> wrote:
>> > On Wed, 2025-12-03 at 14:18 -0500, Kurt Borja wrote:  
>> > >
>> > > In a recent driver review discussion [1], Andy Shevchenko suggested we
>> > > add cleanup.h support for the lock API:
>> > >
>> > >       iio_device_claim_{direct,buffer_mode}().  
>> >
>> > We already went this patch and then reverted it. I guess before we did not had
>> > ACQUIRE() and ACQUIRE_ERR() but I'm not sure that makes it much better. Looking at the
>> > last two patches on how we are handling the buffer mode stuff, I'm really not convinced...
>> >
>> > Also, I have doubts sparse can keep up with the __cleanup stuff so I'm not sure the
>> > annotations much make sense if we go down this path. Unless we want to use both
>> > approaches which is also questionable.  
>> 
>> This, indeed, needs a (broader) discussion and I appreciate that Kurt
>> sent this RFC. Jonathan, what's your thoughts?
>
> I was pretty heavily involved in discussions around ACQUIRE() and it's use
> in CXL and runtime PM (though that's still evolving with Rafael trying
> to improve the syntax a little).  As you might guess I did have this use
> in mind during those discussions.
>
> As far as I know by avoiding the for loop complexity of the previous
> try we made and looking (under the hood) like guard() it should be much
> easier and safer to use.  Looking at this was on my list, so I'm very happy
> to see this series from Kurt exploring how it would be done.
>
> Sparse wise there is no support for now for any of the cleanup.h magic
> other than ignoring it.  That doesn't bother me that much though as these
> macros create more or less hidden local variables that are hard to mess
> with in incorrect ways.
>
> So in general I'm very much in favour of this for same reasons I jumped
> in last time (which turned out to be premature!)
>
> This will be particularly useful in avoiding the need for helper functions
> in otherwise simple code flows.

Good to hear!

Next version, after we agree on the naming approach, I'll drop the RFC
and thake all suggestions for the next version.

Thank you all for your suggestions and comments :)

>
> Jonathan


-- 
 ~ Kurt

Re: [PATCH RFC 0/6] iio: core: Introduce cleanup.h support for mode locks

[Nuno Sá]

On Sat, 2025-12-06 at 18:46 +0000, Jonathan Cameron wrote:
> On Thu, 4 Dec 2025 17:07:28 +0200
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> 
> > On Thu, Dec 4, 2025 at 4:35 PM Nuno Sá <noname.nuno@gmail.com> wrote:
> > > On Wed, 2025-12-03 at 14:18 -0500, Kurt Borja wrote:  
> > > > 
> > > > In a recent driver review discussion [1], Andy Shevchenko suggested we
> > > > add cleanup.h support for the lock API:
> > > > 
> > > >       iio_device_claim_{direct,buffer_mode}().  
> > > 
> > > We already went this patch and then reverted it. I guess before we did not had
> > > ACQUIRE() and ACQUIRE_ERR() but I'm not sure that makes it much better. Looking at the
> > > last two patches on how we are handling the buffer mode stuff, I'm really not convinced...
> > > 
> > > Also, I have doubts sparse can keep up with the __cleanup stuff so I'm not sure the
> > > annotations much make sense if we go down this path. Unless we want to use both
> > > approaches which is also questionable.  
> > 
> > This, indeed, needs a (broader) discussion and I appreciate that Kurt
> > sent this RFC. Jonathan, what's your thoughts?
> 
> I was pretty heavily involved in discussions around ACQUIRE() and it's use
> in CXL and runtime PM (though that's still evolving with Rafael trying
> to improve the syntax a little).  As you might guess I did have this use
> in mind during those discussions.
> 
> As far as I know by avoiding the for loop complexity of the previous
> try we made and looking (under the hood) like guard() it should be much
> easier and safer to use.  Looking at this was on my list, so I'm very happy
> to see this series from Kurt exploring how it would be done.
> 
> Sparse wise there is no support for now for any of the cleanup.h magic
> other than ignoring it.  That doesn't bother me that much though as these
> macros create more or less hidden local variables that are hard to mess
> with in incorrect ways.
> 
> So in general I'm very much in favour of this for same reasons I jumped
> in last time (which turned out to be premature!)
> 
> This will be particularly useful in avoiding the need for helper functions
> in otherwise simple code flows.
> 

Ok, it seems we are going down the path to introduce this again. I do agree the new ACQUIRE()
macros make things better (btw, I would be in favor of something similar to pm runtime). Though
I'm still a bit worried about the device lock helper (the iio_device_claim one). We went through
some significant work in order to make mlock private (given historical abuse of it) and this
is basically making it public again. So I would like to either think a bit harder to see if we
can avoid it or just keep the code in patches 5 and 6 as is (even though the dance in there is
really not pretty).

At the very least I would like to see a big, fat comment stating that lock is not to be randomly
used by drivers to protect their own internal data structures and state.

- Nuno Sá

Re: [PATCH RFC 0/6] iio: core: Introduce cleanup.h support for mode locks

[David Lechner]

On 12/9/25 4:34 AM, Nuno Sá wrote:
> On Sat, 2025-12-06 at 18:46 +0000, Jonathan Cameron wrote:
>> On Thu, 4 Dec 2025 17:07:28 +0200
>> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>>
>>> On Thu, Dec 4, 2025 at 4:35 PM Nuno Sá <noname.nuno@gmail.com> wrote:
>>>> On Wed, 2025-12-03 at 14:18 -0500, Kurt Borja wrote:  
>>>>>
>>>>> In a recent driver review discussion [1], Andy Shevchenko suggested we
>>>>> add cleanup.h support for the lock API:
>>>>>
>>>>>       iio_device_claim_{direct,buffer_mode}().  
>>>>
>>>> We already went this patch and then reverted it. I guess before we did not had
>>>> ACQUIRE() and ACQUIRE_ERR() but I'm not sure that makes it much better. Looking at the
>>>> last two patches on how we are handling the buffer mode stuff, I'm really not convinced...
>>>>
>>>> Also, I have doubts sparse can keep up with the __cleanup stuff so I'm not sure the
>>>> annotations much make sense if we go down this path. Unless we want to use both
>>>> approaches which is also questionable.  
>>>
>>> This, indeed, needs a (broader) discussion and I appreciate that Kurt
>>> sent this RFC. Jonathan, what's your thoughts?
>>
>> I was pretty heavily involved in discussions around ACQUIRE() and it's use
>> in CXL and runtime PM (though that's still evolving with Rafael trying
>> to improve the syntax a little).  As you might guess I did have this use
>> in mind during those discussions.
>>
>> As far as I know by avoiding the for loop complexity of the previous
>> try we made and looking (under the hood) like guard() it should be much
>> easier and safer to use.  Looking at this was on my list, so I'm very happy
>> to see this series from Kurt exploring how it would be done.
>>
>> Sparse wise there is no support for now for any of the cleanup.h magic
>> other than ignoring it.  That doesn't bother me that much though as these
>> macros create more or less hidden local variables that are hard to mess
>> with in incorrect ways.
>>
>> So in general I'm very much in favour of this for same reasons I jumped
>> in last time (which turned out to be premature!)
>>
>> This will be particularly useful in avoiding the need for helper functions
>> in otherwise simple code flows.
>>
> 
> Ok, it seems we are going down the path to introduce this again. I do agree the new ACQUIRE()
> macros make things better (btw, I would be in favor of something similar to pm runtime). Though
> I'm still a bit worried about the device lock helper (the iio_device_claim one). We went through
> some significant work in order to make mlock private (given historical abuse of it) and this
> is basically making it public again. So I would like to either think a bit harder to see if we
> can avoid it or just keep the code in patches 5 and 6 as is (even though the dance in there is
> really not pretty).
> 
> At the very least I would like to see a big, fat comment stating that lock is not to be randomly
> used by drivers to protect their own internal data structures and state.
> 
> - Nuno Sá

Due to the way that conditional guards only extend regular guards, I don't
think there is a way to not expose the basic mlock wrapper. So "don't use this
unless you really know what you are doing" docs seem like the best option.

Re: [PATCH RFC 0/6] iio: core: Introduce cleanup.h support for mode locks

[Nuno Sá]

On Tue, 2025-12-09 at 11:05 -0600, David Lechner wrote:
> On 12/9/25 4:34 AM, Nuno Sá wrote:
> > On Sat, 2025-12-06 at 18:46 +0000, Jonathan Cameron wrote:
> > > On Thu, 4 Dec 2025 17:07:28 +0200
> > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > 
> > > > On Thu, Dec 4, 2025 at 4:35 PM Nuno Sá <noname.nuno@gmail.com> wrote:
> > > > > On Wed, 2025-12-03 at 14:18 -0500, Kurt Borja wrote:  
> > > > > > 
> > > > > > In a recent driver review discussion [1], Andy Shevchenko suggested we
> > > > > > add cleanup.h support for the lock API:
> > > > > > 
> > > > > >       iio_device_claim_{direct,buffer_mode}().  
> > > > > 
> > > > > We already went this patch and then reverted it. I guess before we did not had
> > > > > ACQUIRE() and ACQUIRE_ERR() but I'm not sure that makes it much better. Looking at the
> > > > > last two patches on how we are handling the buffer mode stuff, I'm really not convinced...
> > > > > 
> > > > > Also, I have doubts sparse can keep up with the __cleanup stuff so I'm not sure the
> > > > > annotations much make sense if we go down this path. Unless we want to use both
> > > > > approaches which is also questionable.  
> > > > 
> > > > This, indeed, needs a (broader) discussion and I appreciate that Kurt
> > > > sent this RFC. Jonathan, what's your thoughts?
> > > 
> > > I was pretty heavily involved in discussions around ACQUIRE() and it's use
> > > in CXL and runtime PM (though that's still evolving with Rafael trying
> > > to improve the syntax a little).  As you might guess I did have this use
> > > in mind during those discussions.
> > > 
> > > As far as I know by avoiding the for loop complexity of the previous
> > > try we made and looking (under the hood) like guard() it should be much
> > > easier and safer to use.  Looking at this was on my list, so I'm very happy
> > > to see this series from Kurt exploring how it would be done.
> > > 
> > > Sparse wise there is no support for now for any of the cleanup.h magic
> > > other than ignoring it.  That doesn't bother me that much though as these
> > > macros create more or less hidden local variables that are hard to mess
> > > with in incorrect ways.
> > > 
> > > So in general I'm very much in favour of this for same reasons I jumped
> > > in last time (which turned out to be premature!)
> > > 
> > > This will be particularly useful in avoiding the need for helper functions
> > > in otherwise simple code flows.
> > > 
> > 
> > Ok, it seems we are going down the path to introduce this again. I do agree the new ACQUIRE()
> > macros make things better (btw, I would be in favor of something similar to pm runtime). Though
> > I'm still a bit worried about the device lock helper (the iio_device_claim one). We went through
> > some significant work in order to make mlock private (given historical abuse of it) and this
> > is basically making it public again. So I would like to either think a bit harder to see if we
> > can avoid it or just keep the code in patches 5 and 6 as is (even though the dance in there is
> > really not pretty).
> > 
> > At the very least I would like to see a big, fat comment stating that lock is not to be randomly
> > used by drivers to protect their own internal data structures and state.
> > 
> > - Nuno Sá
> 
> Due to the way that conditional guards only extend regular guards, I don't
> think there is a way to not expose the basic mlock wrapper. So "don't use this
> unless you really know what you are doing" docs seem like the best option.

Right! I figured my first option would be very unlikely... But for the comment I hope we can
elaborate a bit more. Like "don't use this lock to protect your own driver state/data ... you might
need this together iio_buffer_enabled() and if for some reason you cannot use the claim helpers).

- Nuno Sá

Re: [PATCH RFC 0/6] iio: core: Introduce cleanup.h support for mode locks

[Jonathan Cameron]

On Wed, 10 Dec 2025 09:17:17 +0000
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Tue, 2025-12-09 at 11:05 -0600, David Lechner wrote:
> > On 12/9/25 4:34 AM, Nuno Sá wrote:  
> > > On Sat, 2025-12-06 at 18:46 +0000, Jonathan Cameron wrote:  
> > > > On Thu, 4 Dec 2025 17:07:28 +0200
> > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > >   
> > > > > On Thu, Dec 4, 2025 at 4:35 PM Nuno Sá <noname.nuno@gmail.com> wrote:  
> > > > > > On Wed, 2025-12-03 at 14:18 -0500, Kurt Borja wrote:    
> > > > > > > 
> > > > > > > In a recent driver review discussion [1], Andy Shevchenko suggested we
> > > > > > > add cleanup.h support for the lock API:
> > > > > > > 
> > > > > > >       iio_device_claim_{direct,buffer_mode}().    
> > > > > > 
> > > > > > We already went this patch and then reverted it. I guess before we did not had
> > > > > > ACQUIRE() and ACQUIRE_ERR() but I'm not sure that makes it much better. Looking at the
> > > > > > last two patches on how we are handling the buffer mode stuff, I'm really not convinced...
> > > > > > 
> > > > > > Also, I have doubts sparse can keep up with the __cleanup stuff so I'm not sure the
> > > > > > annotations much make sense if we go down this path. Unless we want to use both
> > > > > > approaches which is also questionable.    
> > > > > 
> > > > > This, indeed, needs a (broader) discussion and I appreciate that Kurt
> > > > > sent this RFC. Jonathan, what's your thoughts?  
> > > > 
> > > > I was pretty heavily involved in discussions around ACQUIRE() and it's use
> > > > in CXL and runtime PM (though that's still evolving with Rafael trying
> > > > to improve the syntax a little).  As you might guess I did have this use
> > > > in mind during those discussions.
> > > > 
> > > > As far as I know by avoiding the for loop complexity of the previous
> > > > try we made and looking (under the hood) like guard() it should be much
> > > > easier and safer to use.  Looking at this was on my list, so I'm very happy
> > > > to see this series from Kurt exploring how it would be done.
> > > > 
> > > > Sparse wise there is no support for now for any of the cleanup.h magic
> > > > other than ignoring it.  That doesn't bother me that much though as these
> > > > macros create more or less hidden local variables that are hard to mess
> > > > with in incorrect ways.
> > > > 
> > > > So in general I'm very much in favour of this for same reasons I jumped
> > > > in last time (which turned out to be premature!)
> > > > 
> > > > This will be particularly useful in avoiding the need for helper functions
> > > > in otherwise simple code flows.
> > > >   
> > > 
> > > Ok, it seems we are going down the path to introduce this again. I do agree the new ACQUIRE()
> > > macros make things better (btw, I would be in favor of something similar to pm runtime). Though
> > > I'm still a bit worried about the device lock helper (the iio_device_claim one). We went through
> > > some significant work in order to make mlock private (given historical abuse of it) and this
> > > is basically making it public again. So I would like to either think a bit harder to see if we
> > > can avoid it or just keep the code in patches 5 and 6 as is (even though the dance in there is
> > > really not pretty).
> > > 
> > > At the very least I would like to see a big, fat comment stating that lock is not to be randomly
> > > used by drivers to protect their own internal data structures and state.
> > > 
> > > - Nuno Sá  
> > 
> > Due to the way that conditional guards only extend regular guards, I don't
> > think there is a way to not expose the basic mlock wrapper. So "don't use this
> > unless you really know what you are doing" docs seem like the best option.  
> 
> Right! I figured my first option would be very unlikely... But for the comment I hope we can
> elaborate a bit more. Like "don't use this lock to protect your own driver state/data ... you might
> need this together iio_buffer_enabled() and if for some reason you cannot use the claim helpers).
I think this problem is possible to work around.

We use wrapper macros rather than ever using the ACQUIRE macro directly
(and obscure function names to make it obvious they aren't normal :)

The underlying lock guards might be there but they'll smell so bad that anyone
using them should have at least checked why and hence seen the 'do not use this'
documentation.

Jonathan

> 
> - Nuno Sá
> 
>