The AD4052 family supports autonomous monitoring readings for threshold
crossings. Add support for catching the GPIO interrupt and expose as an IIO
event. The device allows to set either, rising and falling directions. Only
either threshold crossing is implemented.

Signed-off-by: Jorge Marques <jorge.marques@analog.com>
---
 drivers/iio/adc/ad4052.c | 369 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 369 insertions(+)

diff --git a/drivers/iio/adc/ad4052.c b/drivers/iio/adc/ad4052.c
index 7d32dc4701ddb0204b5505a650ce7caafc2cb5ed..ff52ff002bfe0ee413ae352b0c1854798b8e89f8 100644
--- a/drivers/iio/adc/ad4052.c
+++ b/drivers/iio/adc/ad4052.c
@@ -13,6 +13,7 @@
 #include <linux/gpio/consumer.h>
 #include <linux/iio/buffer.h>
 #include <linux/iio/buffer-dmaengine.h>
+#include <linux/iio/events.h>
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
 #include <linux/interrupt.h>
@@ -69,6 +70,8 @@
 #define AD4052_REG_MON_VAL				0x2F
 #define AD4052_REG_FUSE_CRC				0x40
 #define AD4052_REG_DEVICE_STATUS			0x41
+#define     AD4052_REG_DEVICE_STATUS_MIN_FLAG		BIT(2)
+#define     AD4052_REG_DEVICE_STATUS_MAX_FLAG		BIT(3)
 #define     AD4052_REG_DEVICE_STATUS_DEVICE_RDY		BIT(7)
 #define     AD4052_REG_DEVICE_STATUS_DEVICE_RESET	BIT(6)
 #define AD4052_REG_MIN_SAMPLE				0x45
@@ -173,6 +176,8 @@ struct ad4052_state {
 	struct completion completion;
 	struct regmap *regmap;
 	u16 oversampling_frequency;
+	u16 events_frequency;
+	bool wait_event;
 	int gp1_irq;
 	int vio_uv;
 	int vref_uv;
@@ -259,6 +264,26 @@ static const struct regmap_access_table ad4052_regmap_wr_table = {
 	.n_yes_ranges = ARRAY_SIZE(ad4052_regmap_wr_ranges),
 };
 
+static const struct iio_event_spec ad4052_events[] = {
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_EITHER,
+		.mask_shared_by_all = BIT(IIO_EV_INFO_ENABLE),
+	},
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_RISING,
+		.mask_shared_by_all = BIT(IIO_EV_INFO_VALUE) |
+				      BIT(IIO_EV_INFO_HYSTERESIS),
+	},
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_FALLING,
+		.mask_shared_by_all = BIT(IIO_EV_INFO_VALUE) |
+				      BIT(IIO_EV_INFO_HYSTERESIS),
+	},
+};
+
 static const char *const ad4052_conversion_freqs[] = {
 	"2000000", "1000000", "300000", "100000",	/*  0 -  3 */
 	"33300", "10000", "3000", "500",		/*  4 -  7 */
@@ -328,6 +353,8 @@ AD4052_EXT_INFO(AD4052_500KSPS);
 	.info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),	\
 	.indexed = 1,									\
 	.channel = 0,									\
+	.event_spec = ad4052_events,							\
+	.num_event_specs = ARRAY_SIZE(ad4052_events),					\
 	.has_ext_scan_type = 1,								\
 	.ext_scan_type = ad4052_scan_type_##bits##_s,					\
 	.num_ext_scan_type = ARRAY_SIZE(ad4052_scan_type_##bits##_s),			\
@@ -344,6 +371,8 @@ AD4052_EXT_INFO(AD4052_500KSPS);
 	.info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),	\
 	.indexed = 1,									\
 	.channel = 0,									\
+	.event_spec = ad4052_events,							\
+	.num_event_specs = ARRAY_SIZE(ad4052_events),					\
 	.has_ext_scan_type = 1,								\
 	.ext_scan_type = ad4052_scan_type_##bits##_s,					\
 	.num_ext_scan_type = ARRAY_SIZE(ad4052_scan_type_##bits##_s),			\
@@ -386,6 +415,74 @@ static const struct ad4052_chip_info ad4058_chip_info = {
 	.grade = AD4052_500KSPS,
 };
 
+static ssize_t ad4052_events_frequency_show(struct device *dev,
+					    struct device_attribute *attr,
+					    char *buf)
+{
+	struct ad4052_state *st = iio_priv(dev_to_iio_dev(dev));
+
+	return sysfs_emit(buf, "%s\n", ad4052_conversion_freqs[st->events_frequency]);
+}
+
+static ssize_t ad4052_events_frequency_store(struct device *dev,
+					     struct device_attribute *attr,
+					     const char *buf,
+					     size_t len)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct ad4052_state *st = iio_priv(indio_dev);
+	int ret;
+
+	if (!iio_device_claim_direct(indio_dev))
+		return -EBUSY;
+	if (st->wait_event) {
+		ret = -EBUSY;
+		goto out_release;
+	}
+
+	ret = __sysfs_match_string(AD4052_FS(st->chip->grade),
+				   AD4052_FS_LEN(st->chip->grade), buf);
+	if (ret < 0)
+		goto out_release;
+
+	st->events_frequency = ret;
+
+out_release:
+	iio_device_release_direct(indio_dev);
+	return ret ? ret : len;
+}
+
+static IIO_DEVICE_ATTR(sampling_frequency, 0644,
+		       ad4052_events_frequency_show,
+		       ad4052_events_frequency_store, 0);
+
+static ssize_t sampling_frequency_available_show(struct device *dev,
+						 struct device_attribute *attr,
+						 char *buf)
+{
+	struct ad4052_state *st = iio_priv(dev_to_iio_dev(dev));
+	int ret = 0;
+
+	for (u8 i = AD4052_FS_OFFSET(st->chip->grade);
+	     i < AD4052_FS_LEN(st->chip->grade); i++)
+		ret += sysfs_emit_at(buf, ret, "%s ", ad4052_conversion_freqs[i]);
+
+	ret += sysfs_emit_at(buf, ret, "\n");
+	return ret;
+}
+
+static IIO_DEVICE_ATTR_RO(sampling_frequency_available, 0);
+
+static struct attribute *ad4052_event_attributes[] = {
+	&iio_dev_attr_sampling_frequency.dev_attr.attr,
+	&iio_dev_attr_sampling_frequency_available.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group ad4052_event_attribute_group = {
+	.attrs = ad4052_event_attributes,
+};
+
 static int ad4052_update_xfer_raw(struct iio_dev *indio_dev,
 				   struct iio_chan_spec const *chan)
 {
@@ -602,6 +699,19 @@ static int ad4052_setup(struct iio_dev *indio_dev, struct iio_chan_spec const *c
 				  val);
 }
 
+static irqreturn_t ad4052_irq_handler_thresh(int irq, void *private)
+{
+	struct iio_dev *indio_dev = private;
+
+	iio_push_event(indio_dev,
+		       IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE, 0,
+					    IIO_EV_TYPE_THRESH,
+					    IIO_EV_DIR_EITHER),
+		       iio_get_time_ns(indio_dev));
+
+	return IRQ_HANDLED;
+}
+
 static irqreturn_t ad4052_irq_handler_drdy(int irq, void *private)
 {
 	struct ad4052_state *st = private;
@@ -616,6 +726,18 @@ static int ad4052_request_irq(struct iio_dev *indio_dev)
 	struct device *dev = &st->spi->dev;
 	int ret;
 
+	ret = fwnode_irq_get_byname(dev_fwnode(&st->spi->dev), "gp0");
+	if (ret > 0) {
+		ret = devm_request_threaded_irq(dev, ret, NULL,
+						ad4052_irq_handler_thresh,
+						IRQF_ONESHOT, indio_dev->name,
+						indio_dev);
+		if (ret)
+			return ret;
+	} else if (ret == -EPROBE_DEFER) {
+		return ret;
+	}
+
 	ret = fwnode_irq_get_byname(dev_fwnode(&st->spi->dev), "gp1");
 	if (ret > 0) {
 		ret = devm_request_threaded_irq(dev, ret, NULL,
@@ -822,6 +944,7 @@ static int ad4052_read_raw(struct iio_dev *indio_dev,
 			   struct iio_chan_spec const *chan, int *val,
 			   int *val2, long info)
 {
+	struct ad4052_state *st = iio_priv(indio_dev);
 	int ret;
 
 	if (info ==  IIO_CHAN_INFO_SAMP_FREQ)
@@ -831,8 +954,14 @@ static int ad4052_read_raw(struct iio_dev *indio_dev,
 
 	if (!iio_device_claim_direct(indio_dev))
 		return -EBUSY;
+	if (st->wait_event) {
+		ret = -EBUSY;
+		goto out_release;
+	}
 
 	ret = ad4052_read_raw_dispatch(indio_dev, chan, val, val2, info);
+
+out_release:
 	iio_device_release_direct(indio_dev);
 	return ret ? ret : IIO_VAL_INT;
 }
@@ -867,8 +996,231 @@ static int ad4052_write_raw(struct iio_dev *indio_dev,
 
 	if (!iio_device_claim_direct(indio_dev))
 		return -EBUSY;
+	if (st->wait_event) {
+		ret = -EBUSY;
+		goto out_release;
+	}
 
 	ret = ad4052_write_raw_dispatch(indio_dev, chan, val, val2, info);
+
+out_release:
+	iio_device_release_direct(indio_dev);
+	return ret;
+}
+
+static int ad4052_monitor_mode_enable(struct ad4052_state *st)
+{
+	int ret;
+
+	ret = pm_runtime_resume_and_get(&st->spi->dev);
+	if (ret)
+		return ret;
+
+	ret = ad4052_conversion_frequency_set(st, st->events_frequency);
+	if (ret)
+		goto out_error;
+
+	ret = ad4052_set_operation_mode(st, AD4052_MONITOR_MODE);
+	if (ret)
+		goto out_error;
+
+	return ret;
+out_error:
+	pm_runtime_mark_last_busy(&st->spi->dev);
+	pm_runtime_put_autosuspend(&st->spi->dev);
+	return ret;
+}
+
+static int ad4052_monitor_mode_disable(struct ad4052_state *st)
+{
+	int ret;
+
+	pm_runtime_mark_last_busy(&st->spi->dev);
+	pm_runtime_put_autosuspend(&st->spi->dev);
+
+	ret = ad4052_exit_command(st);
+	if (ret)
+		return ret;
+	return regmap_write(st->regmap, AD4052_REG_DEVICE_STATUS,
+			    AD4052_REG_DEVICE_STATUS_MAX_FLAG |
+			    AD4052_REG_DEVICE_STATUS_MIN_FLAG);
+}
+
+static int ad4052_read_event_config(struct iio_dev *indio_dev,
+				    const struct iio_chan_spec *chan,
+				    enum iio_event_type type,
+				    enum iio_event_direction dir)
+{
+	struct ad4052_state *st = iio_priv(indio_dev);
+
+	return st->wait_event;
+}
+
+static int ad4052_write_event_config(struct iio_dev *indio_dev,
+				     const struct iio_chan_spec *chan,
+				     enum iio_event_type type,
+				     enum iio_event_direction dir,
+				     bool state)
+{
+	struct ad4052_state *st = iio_priv(indio_dev);
+	int ret;
+
+	if (!iio_device_claim_direct(indio_dev))
+		return -EBUSY;
+	if (st->wait_event == state) {
+		ret = 0;
+		goto out_release;
+	}
+
+	if (state)
+		ret = ad4052_monitor_mode_enable(st);
+	else
+		ret = ad4052_monitor_mode_disable(st);
+
+	if (!ret)
+		st->wait_event = state;
+
+out_release:
+	iio_device_release_direct(indio_dev);
+	return ret;
+}
+
+static int __ad4052_read_event_info_value(struct ad4052_state *st,
+					   enum iio_event_direction dir, int *val)
+{
+	int ret;
+	u8 reg;
+
+	if (dir == IIO_EV_DIR_RISING)
+		reg = AD4052_REG_MAX_LIMIT;
+	else
+		reg = AD4052_REG_MIN_LIMIT;
+
+	ret = regmap_bulk_read(st->regmap, reg, &st->raw, 2);
+	if (ret)
+		return ret;
+
+	*val = sign_extend32(get_unaligned_be16(st->raw), 11);
+
+	return 0;
+}
+
+static int __ad4052_read_event_info_hysteresis(struct ad4052_state *st,
+						enum iio_event_direction dir, int *val)
+{
+	u8 reg;
+
+	if (dir == IIO_EV_DIR_RISING)
+		reg = AD4052_REG_MAX_HYST;
+	else
+		reg = AD4052_REG_MIN_HYST;
+	return regmap_read(st->regmap, reg, val);
+}
+
+static int ad4052_read_event_value(struct iio_dev *indio_dev,
+				   const struct iio_chan_spec *chan,
+				   enum iio_event_type type,
+				   enum iio_event_direction dir,
+				   enum iio_event_info info, int *val,
+				   int *val2)
+{
+	struct ad4052_state *st = iio_priv(indio_dev);
+	int ret;
+
+	if (!iio_device_claim_direct(indio_dev))
+		return -EBUSY;
+
+	if (st->wait_event) {
+		ret = -EBUSY;
+		goto out_release;
+	}
+
+	switch (info) {
+	case IIO_EV_INFO_VALUE:
+		ret = __ad4052_read_event_info_value(st, dir, val);
+		break;
+	case IIO_EV_INFO_HYSTERESIS:
+		ret = __ad4052_read_event_info_hysteresis(st, dir, val);
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+out_release:
+	iio_device_release_direct(indio_dev);
+	return ret ? ret : IIO_VAL_INT;
+}
+
+static int __ad4052_write_event_info_value(struct ad4052_state *st,
+					   enum iio_event_direction dir, int val)
+{
+	u8 reg;
+
+	if (val > 2047 || val < -2048)
+		return -EINVAL;
+	if (dir == IIO_EV_DIR_RISING)
+		reg = AD4052_REG_MAX_LIMIT;
+	else
+		reg = AD4052_REG_MIN_LIMIT;
+	put_unaligned_be16(val, &st->raw);
+
+	return regmap_bulk_write(st->regmap, reg, &st->raw, 2);
+}
+
+static int __ad4052_write_event_info_hysteresis(struct ad4052_state *st,
+						enum iio_event_direction dir, int val)
+{
+	u8 reg;
+
+	if (val >= BIT(7))
+		return -EINVAL;
+	if (dir == IIO_EV_DIR_RISING)
+		reg = AD4052_REG_MAX_HYST;
+	else
+		reg = AD4052_REG_MIN_HYST;
+
+	return regmap_write(st->regmap, reg, val);
+}
+
+static int ad4052_write_event_value(struct iio_dev *indio_dev,
+				    const struct iio_chan_spec *chan,
+				    enum iio_event_type type,
+				    enum iio_event_direction dir,
+				    enum iio_event_info info, int val,
+				    int val2)
+{
+	struct ad4052_state *st = iio_priv(indio_dev);
+	int ret;
+
+	if (!iio_device_claim_direct(indio_dev))
+		return -EBUSY;
+
+	if (st->wait_event) {
+		ret = -EBUSY;
+		goto out_release;
+	}
+
+	switch (type) {
+	case IIO_EV_TYPE_THRESH:
+		switch (info) {
+		case IIO_EV_INFO_VALUE:
+			ret = __ad4052_write_event_info_value(st, dir, val);
+			break;
+		case IIO_EV_INFO_HYSTERESIS:
+			ret = __ad4052_write_event_info_hysteresis(st, dir, val);
+			break;
+		default:
+			ret = -EINVAL;
+			break;
+		}
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+out_release:
 	iio_device_release_direct(indio_dev);
 	return ret;
 }
@@ -881,6 +1233,9 @@ static int ad4052_offload_buffer_postenable(struct iio_dev *indio_dev)
 	};
 	int ret;
 
+	if (st->wait_event)
+		return -EBUSY;
+
 	ret = pm_runtime_resume_and_get(&st->spi->dev);
 	if (ret)
 		return ret;
@@ -963,10 +1318,17 @@ static int ad4052_debugfs_reg_access(struct iio_dev *indio_dev, unsigned int reg
 	if (!iio_device_claim_direct(indio_dev))
 		return -EBUSY;
 
+	if (st->wait_event) {
+		ret = -EBUSY;
+		goto out_release;
+	}
+
 	if (readval)
 		ret = regmap_read(st->regmap, reg, readval);
 	else
 		ret = regmap_write(st->regmap, reg, writeval);
+
+out_release:
 	iio_device_release_direct(indio_dev);
 	return ret;
 }
@@ -985,6 +1347,11 @@ static const struct iio_info ad4052_info = {
 	.read_raw = ad4052_read_raw,
 	.write_raw = ad4052_write_raw,
 	.read_avail = ad4052_read_avail,
+	.read_event_config = &ad4052_read_event_config,
+	.write_event_config = &ad4052_write_event_config,
+	.read_event_value = &ad4052_read_event_value,
+	.write_event_value = &ad4052_write_event_value,
+	.event_attrs = &ad4052_event_attribute_group,
 	.get_current_scan_type = &ad4052_get_current_scan_type,
 	.debugfs_reg_access = &ad4052_debugfs_reg_access,
 };
@@ -1193,8 +1560,10 @@ static int ad4052_probe(struct spi_device *spi)
 				     "Failed to initialize regmap\n");
 
 	st->mode = AD4052_SAMPLE_MODE;
+	st->wait_event = false;
 	st->chip = chip;
 	st->oversampling_frequency = AD4052_FS_OFFSET(st->chip->grade);
+	st->events_frequency = AD4052_FS_OFFSET(st->chip->grade);
 
 	st->cnv_gp = devm_gpiod_get_optional(dev, "cnv", GPIOD_OUT_LOW);
 	if (IS_ERR(st->cnv_gp))

-- 
2.49.0

On Tue, 10 Jun 2025 09:34:41 +0200
Jorge Marques <jorge.marques@analog.com> wrote:

> The AD4052 family supports autonomous monitoring readings for threshold
> crossings. Add support for catching the GPIO interrupt and expose as an IIO
> event. The device allows to set either, rising and falling directions. Only
> either threshold crossing is implemented.
> 
> Signed-off-by: Jorge Marques <jorge.marques@analog.com>
Hi Jorge,

A few comments inline.

Jonathan

>
> +
> +static int ad4052_write_event_config(struct iio_dev *indio_dev,
> +				     const struct iio_chan_spec *chan,
> +				     enum iio_event_type type,
> +				     enum iio_event_direction dir,
> +				     bool state)
> +{
> +	struct ad4052_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	if (!iio_device_claim_direct(indio_dev))
> +		return -EBUSY;
> +	if (st->wait_event == state) {
> +		ret = 0;

Feels like a case where init ret at declaration would be reasonable.

> +		goto out_release;
> +	}
> +
> +	if (state)
> +		ret = ad4052_monitor_mode_enable(st);
> +	else
> +		ret = ad4052_monitor_mode_disable(st);
> +
> +	if (!ret)
> +		st->wait_event = state;
> +
> +out_release:
> +	iio_device_release_direct(indio_dev);
> +	return ret;
> +}

> +
> +static int ad4052_read_event_value(struct iio_dev *indio_dev,
> +				   const struct iio_chan_spec *chan,
> +				   enum iio_event_type type,
> +				   enum iio_event_direction dir,
> +				   enum iio_event_info info, int *val,
> +				   int *val2)
> +{
> +	struct ad4052_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	if (!iio_device_claim_direct(indio_dev))
> +		return -EBUSY;
> +
> +	if (st->wait_event) {
> +		ret = -EBUSY;
> +		goto out_release;

Not being able to read event parameters whilst monitoring them seems
very restrictive.  Can't we cache the values?  Either play games to ensure
we get them from the regmap cache or just cache these few values in st.

Checking what you are monitoring for feels like the sort of thing
userspace might well do.

Even blocking changing the monitoring parameters is unusually strict.
Why not just drop out of monitor mode, update them and go back in?


> +	}
> +
> +	switch (info) {
> +	case IIO_EV_INFO_VALUE:
> +		ret = __ad4052_read_event_info_value(st, dir, val);
> +		break;
> +	case IIO_EV_INFO_HYSTERESIS:
> +		ret = __ad4052_read_event_info_hysteresis(st, dir, val);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +out_release:
> +	iio_device_release_direct(indio_dev);
> +	return ret ? ret : IIO_VAL_INT;
> +}

On Sat, Jun 14, 2025 at 11:36:16AM +0100, Jonathan Cameron wrote:
> On Tue, 10 Jun 2025 09:34:41 +0200
> Jorge Marques <jorge.marques@analog.com> wrote:
> 
> > The AD4052 family supports autonomous monitoring readings for threshold
> > crossings. Add support for catching the GPIO interrupt and expose as an IIO
> > event. The device allows to set either, rising and falling directions. Only
> > either threshold crossing is implemented.
> > 
> > Signed-off-by: Jorge Marques <jorge.marques@analog.com>
Hi Jonathan,
> Hi Jorge,
> 
> A few comments inline.
> 
> Jonathan
> 
> >
> > +
> > +static int ad4052_write_event_config(struct iio_dev *indio_dev,
> > +				     const struct iio_chan_spec *chan,
> > +				     enum iio_event_type type,
> > +				     enum iio_event_direction dir,
> > +				     bool state)
> > +{
> > +	struct ad4052_state *st = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	if (!iio_device_claim_direct(indio_dev))
> > +		return -EBUSY;
> > +	if (st->wait_event == state) {
> > +		ret = 0;
> 
> Feels like a case where init ret at declaration would be reasonable.
> 
Ack.
> > +		goto out_release;
> > +	}
> > +
> > +	if (state)
> > +		ret = ad4052_monitor_mode_enable(st);
> > +	else
> > +		ret = ad4052_monitor_mode_disable(st);
> > +
> > +	if (!ret)
> > +		st->wait_event = state;
> > +
> > +out_release:
> > +	iio_device_release_direct(indio_dev);
> > +	return ret;
> > +}
> 
> > +
> > +static int ad4052_read_event_value(struct iio_dev *indio_dev,
> > +				   const struct iio_chan_spec *chan,
> > +				   enum iio_event_type type,
> > +				   enum iio_event_direction dir,
> > +				   enum iio_event_info info, int *val,
> > +				   int *val2)
> > +{
> > +	struct ad4052_state *st = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	if (!iio_device_claim_direct(indio_dev))
> > +		return -EBUSY;
> > +
> > +	if (st->wait_event) {
> > +		ret = -EBUSY;
> > +		goto out_release;
> 

Below are two distinct options with different implications.
> Not being able to read event parameters whilst monitoring them seems
> very restrictive.  Can't we cache the values?  Either play games to ensure
> we get them from the regmap cache or just cache these few values in st.
> 
> Checking what you are monitoring for feels like the sort of thing
> userspace might well do.

(1)
I agree, I can investigate regcache_cache_only and the other cache
options to achieve this. If I come to the conclusion it is not possible,
storing into st will achieve the same.

> 
> Even blocking changing the monitoring parameters is unusually strict.
> Why not just drop out of monitor mode, update them and go back in?
> 
(2)
The core point of the blocking behaviour is to not have hidden downtimes
in the monitoring for the user. An early driver used to do what you
describe and it was a design decision.

Since a custom regmap_bus was necessary to restrict the regmap access
speed (ADC access is faster), bringing back this by behavior embedding
it in the custom regmap now seems plausible, with proper explanation in
the rst page. This should fully dismiss the st->wait_event -> -EBUSY.

Considering (1) and (2), what is the preferred approach?

Regards,
Jorge
> > +	}
> > +
> > +	switch (info) {
> > +	case IIO_EV_INFO_VALUE:
> > +		ret = __ad4052_read_event_info_value(st, dir, val);
> > +		break;
> > +	case IIO_EV_INFO_HYSTERESIS:
> > +		ret = __ad4052_read_event_info_hysteresis(st, dir, val);
> > +		break;
> > +	default:
> > +		ret = -EINVAL;
> > +		break;
> > +	}
> > +
> > +out_release:
> > +	iio_device_release_direct(indio_dev);
> > +	return ret ? ret : IIO_VAL_INT;
> > +}

> > > +
> > > +static int ad4052_read_event_value(struct iio_dev *indio_dev,
> > > +				   const struct iio_chan_spec *chan,
> > > +				   enum iio_event_type type,
> > > +				   enum iio_event_direction dir,
> > > +				   enum iio_event_info info, int *val,
> > > +				   int *val2)
> > > +{
> > > +	struct ad4052_state *st = iio_priv(indio_dev);
> > > +	int ret;
> > > +
> > > +	if (!iio_device_claim_direct(indio_dev))
> > > +		return -EBUSY;
> > > +
> > > +	if (st->wait_event) {
> > > +		ret = -EBUSY;
> > > +		goto out_release;  
> >   
> 
> Below are two distinct options with different implications.
> > Not being able to read event parameters whilst monitoring them seems
> > very restrictive.  Can't we cache the values?  Either play games to ensure
> > we get them from the regmap cache or just cache these few values in st.
> > 
> > Checking what you are monitoring for feels like the sort of thing
> > userspace might well do.  
> 
> (1)
> I agree, I can investigate regcache_cache_only and the other cache
> options to achieve this. If I come to the conclusion it is not possible,
> storing into st will achieve the same.
> 
> > 
> > Even blocking changing the monitoring parameters is unusually strict.
> > Why not just drop out of monitor mode, update them and go back in?
> >   
> (2)
> The core point of the blocking behaviour is to not have hidden downtimes
> in the monitoring for the user. An early driver used to do what you
> describe and it was a design decision.
> 
> Since a custom regmap_bus was necessary to restrict the regmap access
> speed (ADC access is faster), bringing back this by behavior embedding
> it in the custom regmap now seems plausible, with proper explanation in
> the rst page. This should fully dismiss the st->wait_event -> -EBUSY.
> 
> Considering (1) and (2), what is the preferred approach?

Key here is that the user made the choice to change the parameters.
Most of the time they won't choose to do that, but if they do then
that's what they want to do. Why make them turn the monitoring off,
change value and turn it on again if we can support it reasonably
cleanly.  In many devices there is no interruption to monitoring so
we may well have userspace code written against assumption it
can just update this stuff without that dance.  So prefer (2)
but (1) is better than nothing if (2) proves too complex.

J
> 
> Regards,
> Jorge
> > > +	}
> > > +
> > > +	switch (info) {
> > > +	case IIO_EV_INFO_VALUE:
> > > +		ret = __ad4052_read_event_info_value(st, dir, val);
> > > +		break;
> > > +	case IIO_EV_INFO_HYSTERESIS:
> > > +		ret = __ad4052_read_event_info_hysteresis(st, dir, val);
> > > +		break;
> > > +	default:
> > > +		ret = -EINVAL;
> > > +		break;
> > > +	}
> > > +
> > > +out_release:
> > > +	iio_device_release_direct(indio_dev);
> > > +	return ret ? ret : IIO_VAL_INT;
> > > +}

On 6/10/25 2:34 AM, Jorge Marques wrote:
> The AD4052 family supports autonomous monitoring readings for threshold
> crossings. Add support for catching the GPIO interrupt and expose as an IIO
> event. The device allows to set either, rising and falling directions. Only
> either threshold crossing is implemented.
> 
> Signed-off-by: Jorge Marques <jorge.marques@analog.com>
> ---

...

> +
> +static ssize_t ad4052_events_frequency_store(struct device *dev,
> +					     struct device_attribute *attr,
> +					     const char *buf,
> +					     size_t len)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct ad4052_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	if (!iio_device_claim_direct(indio_dev))
> +		return -EBUSY;
> +	if (st->wait_event) {
> +		ret = -EBUSY;
> +		goto out_release;
> +	}

I'm wondering if we should instead have some kind of iio_device_claim_monitor_mode()
so that we don't have to implement this manually everywhere. If monitor mode was
claimed, then iio_device_claim_direct() and iio_device_claim_buffer_mode() would
both return -EBUSY. If buffer mode was claimed, iio_device_claim_monitor_mode()
would fail. If direct mode was claimed, iio_device_claim_monitor_mode() would wait.

> +
> +	ret = __sysfs_match_string(AD4052_FS(st->chip->grade),
> +				   AD4052_FS_LEN(st->chip->grade), buf);
> +	if (ret < 0)
> +		goto out_release;
> +
> +	st->events_frequency = ret;
> +
> +out_release:
> +	iio_device_release_direct(indio_dev);
> +	return ret ? ret : len;
> +}
> +
> +static IIO_DEVICE_ATTR(sampling_frequency, 0644,
> +		       ad4052_events_frequency_show,
> +		       ad4052_events_frequency_store, 0);
> +
> +static ssize_t sampling_frequency_available_show(struct device *dev,
> +						 struct device_attribute *attr,
> +						 char *buf)
> +{
> +	struct ad4052_state *st = iio_priv(dev_to_iio_dev(dev));
> +	int ret = 0;
> +
> +	for (u8 i = AD4052_FS_OFFSET(st->chip->grade);
> +	     i < AD4052_FS_LEN(st->chip->grade); i++)
> +		ret += sysfs_emit_at(buf, ret, "%s ", ad4052_conversion_freqs[i]);
> +
> +	ret += sysfs_emit_at(buf, ret, "\n");
> +	return ret;
> +}
> +
> +static IIO_DEVICE_ATTR_RO(sampling_frequency_available, 0);
> +
> +static struct attribute *ad4052_event_attributes[] = {
> +	&iio_dev_attr_sampling_frequency.dev_attr.attr,
> +	&iio_dev_attr_sampling_frequency_available.dev_attr.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group ad4052_event_attribute_group = {
> +	.attrs = ad4052_event_attributes,
> +};
> +
>  static int ad4052_update_xfer_raw(struct iio_dev *indio_dev,
>  				   struct iio_chan_spec const *chan)
>  {
> @@ -602,6 +699,19 @@ static int ad4052_setup(struct iio_dev *indio_dev, struct iio_chan_spec const *c
>  				  val);
>  }
>  
> +static irqreturn_t ad4052_irq_handler_thresh(int irq, void *private)
> +{
> +	struct iio_dev *indio_dev = private;
> +

Can we not read the status register here to find out what the exact
event was? I guess that would require taking it out of monitor mode.

> +	iio_push_event(indio_dev,
> +		       IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE, 0,
> +					    IIO_EV_TYPE_THRESH,
> +					    IIO_EV_DIR_EITHER),
> +		       iio_get_time_ns(indio_dev));
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static irqreturn_t ad4052_irq_handler_drdy(int irq, void *private)
>  {
>  	struct ad4052_state *st = private;
> @@ -616,6 +726,18 @@ static int ad4052_request_irq(struct iio_dev *indio_dev)
>  	struct device *dev = &st->spi->dev;
>  	int ret;
>  
> +	ret = fwnode_irq_get_byname(dev_fwnode(&st->spi->dev), "gp0");
> +	if (ret > 0) {
> +		ret = devm_request_threaded_irq(dev, ret, NULL,
> +						ad4052_irq_handler_thresh,
> +						IRQF_ONESHOT, indio_dev->name,
> +						indio_dev);
> +		if (ret)
> +			return ret;
> +	} else if (ret == -EPROBE_DEFER) {
> +		return ret;
> +	}

By swapping the order, we can avoid the else. Also, do we really want to
ignore all other errors? It seems like there would just be ENODEV or ENOENT
that means the interrupt is not there and we would want to pass on other
errors.

> +
>  	ret = fwnode_irq_get_byname(dev_fwnode(&st->spi->dev), "gp1");
>  	if (ret > 0) {
>  		ret = devm_request_threaded_irq(dev, ret, NULL,


...

> +
> +static int ad4052_monitor_mode_enable(struct ad4052_state *st)
> +{
> +	int ret;
> +
> +	ret = pm_runtime_resume_and_get(&st->spi->dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad4052_conversion_frequency_set(st, st->events_frequency);
> +	if (ret)
> +		goto out_error;
> +
> +	ret = ad4052_set_operation_mode(st, AD4052_MONITOR_MODE);
> +	if (ret)
> +		goto out_error;
> +
> +	return ret;
> +out_error:
> +	pm_runtime_mark_last_busy(&st->spi->dev);
> +	pm_runtime_put_autosuspend(&st->spi->dev);
> +	return ret;
> +}
> +
> +static int ad4052_monitor_mode_disable(struct ad4052_state *st)
> +{
> +	int ret;
> +
> +	pm_runtime_mark_last_busy(&st->spi->dev);
> +	pm_runtime_put_autosuspend(&st->spi->dev);
> +
> +	ret = ad4052_exit_command(st);
> +	if (ret)
> +		return ret;
> +	return regmap_write(st->regmap, AD4052_REG_DEVICE_STATUS,
> +			    AD4052_REG_DEVICE_STATUS_MAX_FLAG |
> +			    AD4052_REG_DEVICE_STATUS_MIN_FLAG);
> +}
> +

It seems like we need to make sure monitor mode is disabled when the
driver is removed. Otherwise we could end up with unbalanced calls to
the pm_runtime stuff and leave the chip running.


> +static int ad4052_read_event_value(struct iio_dev *indio_dev,
> +				   const struct iio_chan_spec *chan,
> +				   enum iio_event_type type,
> +				   enum iio_event_direction dir,
> +				   enum iio_event_info info, int *val,
> +				   int *val2)
> +{
> +	struct ad4052_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	if (!iio_device_claim_direct(indio_dev))
> +		return -EBUSY;
> +
> +	if (st->wait_event) {
> +		ret = -EBUSY;
> +		goto out_release;
> +	}
> +
> +	switch (info) {
> +	case IIO_EV_INFO_VALUE:
> +		ret = __ad4052_read_event_info_value(st, dir, val);
> +		break;
> +	case IIO_EV_INFO_HYSTERESIS:
> +		ret = __ad4052_read_event_info_hysteresis(st, dir, val);
> +		break;

These functions don't need __ prefix. There is no name clash.

> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +out_release:
> +	iio_device_release_direct(indio_dev);
> +	return ret ? ret : IIO_VAL_INT;
> +}
> +

Hi David,
On Thu, Jun 12, 2025 at 02:38:45PM -0500, David Lechner wrote:
> On 6/10/25 2:34 AM, Jorge Marques wrote:
> > The AD4052 family supports autonomous monitoring readings for threshold
> > crossings. Add support for catching the GPIO interrupt and expose as an IIO
> > event. The device allows to set either, rising and falling directions. Only
> > either threshold crossing is implemented.
> > 
> > Signed-off-by: Jorge Marques <jorge.marques@analog.com>
> > ---
> 
> ...
> 
> > +
> > +static ssize_t ad4052_events_frequency_store(struct device *dev,
> > +					     struct device_attribute *attr,
> > +					     const char *buf,
> > +					     size_t len)
> > +{
> > +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > +	struct ad4052_state *st = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	if (!iio_device_claim_direct(indio_dev))
> > +		return -EBUSY;
> > +	if (st->wait_event) {
> > +		ret = -EBUSY;
> > +		goto out_release;
> > +	}
> 
> I'm wondering if we should instead have some kind of iio_device_claim_monitor_mode()
> so that we don't have to implement this manually everywhere. If monitor mode was
> claimed, then iio_device_claim_direct() and iio_device_claim_buffer_mode() would
> both return -EBUSY. If buffer mode was claimed, iio_device_claim_monitor_mode()
> would fail. If direct mode was claimed, iio_device_claim_monitor_mode() would wait.
> 
I don't think this would scale with other vendors and devices, it is a
limitation of ADI:ADC:SPI requiring to enter configuration mode to read
registers. A deep dive into the other drivers that use IIO Events is
needed.
> > +
> > +	ret = __sysfs_match_string(AD4052_FS(st->chip->grade),
> > +				   AD4052_FS_LEN(st->chip->grade), buf);
> > +	if (ret < 0)
> > +		goto out_release;
> > +
> > +	st->events_frequency = ret;
> > +
> > +out_release:
> > +	iio_device_release_direct(indio_dev);
> > +	return ret ? ret : len;
> > +}
> > +
> > +static IIO_DEVICE_ATTR(sampling_frequency, 0644,
> > +		       ad4052_events_frequency_show,
> > +		       ad4052_events_frequency_store, 0);
> > +
> > +static ssize_t sampling_frequency_available_show(struct device *dev,
> > +						 struct device_attribute *attr,
> > +						 char *buf)
> > +{
> > +	struct ad4052_state *st = iio_priv(dev_to_iio_dev(dev));
> > +	int ret = 0;
> > +
> > +	for (u8 i = AD4052_FS_OFFSET(st->chip->grade);
> > +	     i < AD4052_FS_LEN(st->chip->grade); i++)
> > +		ret += sysfs_emit_at(buf, ret, "%s ", ad4052_conversion_freqs[i]);
> > +
> > +	ret += sysfs_emit_at(buf, ret, "\n");
> > +	return ret;
> > +}
> > +
> > +static IIO_DEVICE_ATTR_RO(sampling_frequency_available, 0);
> > +
> > +static struct attribute *ad4052_event_attributes[] = {
> > +	&iio_dev_attr_sampling_frequency.dev_attr.attr,
> > +	&iio_dev_attr_sampling_frequency_available.dev_attr.attr,
> > +	NULL
> > +};
> > +
> > +static const struct attribute_group ad4052_event_attribute_group = {
> > +	.attrs = ad4052_event_attributes,
> > +};
> > +
> >  static int ad4052_update_xfer_raw(struct iio_dev *indio_dev,
> >  				   struct iio_chan_spec const *chan)
> >  {
> > @@ -602,6 +699,19 @@ static int ad4052_setup(struct iio_dev *indio_dev, struct iio_chan_spec const *c
> >  				  val);
> >  }
> >  
> > +static irqreturn_t ad4052_irq_handler_thresh(int irq, void *private)
> > +{
> > +	struct iio_dev *indio_dev = private;
> > +
> 
> Can we not read the status register here to find out what the exact
> event was? I guess that would require taking it out of monitor mode.
> 
It requires entering configuration mode and results in a monitoring
downtime. Earlier versions of this driver would do that, but the
conclusion was that it was better to have the user disabling events and
reading registers, so he is explicitly aware of the monitoring downtime.
> > +	iio_push_event(indio_dev,
> > +		       IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE, 0,
> > +					    IIO_EV_TYPE_THRESH,
> > +					    IIO_EV_DIR_EITHER),
> > +		       iio_get_time_ns(indio_dev));
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> >  static irqreturn_t ad4052_irq_handler_drdy(int irq, void *private)
> >  {
> >  	struct ad4052_state *st = private;
> > @@ -616,6 +726,18 @@ static int ad4052_request_irq(struct iio_dev *indio_dev)
> >  	struct device *dev = &st->spi->dev;
> >  	int ret;
> >  
> > +	ret = fwnode_irq_get_byname(dev_fwnode(&st->spi->dev), "gp0");
> > +	if (ret > 0) {
> > +		ret = devm_request_threaded_irq(dev, ret, NULL,
> > +						ad4052_irq_handler_thresh,
> > +						IRQF_ONESHOT, indio_dev->name,
> > +						indio_dev);
> > +		if (ret)
> > +			return ret;
> > +	} else if (ret == -EPROBE_DEFER) {
> > +		return ret;
> > +	}
> 
> By swapping the order, we can avoid the else. Also, do we really want to
> ignore all other errors? It seems like there would just be ENODEV or ENOENT
> that means the interrupt is not there and we would want to pass on other
> errors.
> 
Ack on the swap order.

If not set on the devicetree, including improper devicetree cases, it
should continue without. If the driver that manages the irq is not
probed, defer probe.

I tested different devicetrees and got:

* any property is missing: -EINVAL
* wrong interrupt-names: -ENODATA
* inconsistent array length between properties: -EOVERFLOW

EPROTO and ENXIO errors are also expected according the method comment,
the latter seems to be when the system doesn't support dts at all? And
EPROTO just another user-set dts issue.
I'm okay with ignoring them silently, or logging if gp0/1 found or not,
but not micromanage every error.

> > +
> >  	ret = fwnode_irq_get_byname(dev_fwnode(&st->spi->dev), "gp1");
> >  	if (ret > 0) {
> >  		ret = devm_request_threaded_irq(dev, ret, NULL,
> 
> 
> ...
> 
> > +
> > +static int ad4052_monitor_mode_enable(struct ad4052_state *st)
> > +{
> > +	int ret;
> > +
> > +	ret = pm_runtime_resume_and_get(&st->spi->dev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = ad4052_conversion_frequency_set(st, st->events_frequency);
> > +	if (ret)
> > +		goto out_error;
> > +
> > +	ret = ad4052_set_operation_mode(st, AD4052_MONITOR_MODE);
> > +	if (ret)
> > +		goto out_error;
> > +
> > +	return ret;
> > +out_error:
> > +	pm_runtime_mark_last_busy(&st->spi->dev);
> > +	pm_runtime_put_autosuspend(&st->spi->dev);
> > +	return ret;
> > +}
> > +
> > +static int ad4052_monitor_mode_disable(struct ad4052_state *st)
> > +{
> > +	int ret;
> > +
> > +	pm_runtime_mark_last_busy(&st->spi->dev);
> > +	pm_runtime_put_autosuspend(&st->spi->dev);
> > +
> > +	ret = ad4052_exit_command(st);
> > +	if (ret)
> > +		return ret;
> > +	return regmap_write(st->regmap, AD4052_REG_DEVICE_STATUS,
> > +			    AD4052_REG_DEVICE_STATUS_MAX_FLAG |
> > +			    AD4052_REG_DEVICE_STATUS_MIN_FLAG);
> > +}
> > +
> 
> It seems like we need to make sure monitor mode is disabled when the
> driver is removed. Otherwise we could end up with unbalanced calls to
> the pm_runtime stuff and leave the chip running.
> 
> 
When monitor mode is enabled, pm is already disabled (won't enter low
power). I expect the pm to handle the clean-up properly since devm is
used.
The .remove() I suggest is reg access to:

* Put in configuration mode, if not.
* Put on low power mode, if not.

> > +static int ad4052_read_event_value(struct iio_dev *indio_dev,
> > +				   const struct iio_chan_spec *chan,
> > +				   enum iio_event_type type,
> > +				   enum iio_event_direction dir,
> > +				   enum iio_event_info info, int *val,
> > +				   int *val2)
> > +{
> > +	struct ad4052_state *st = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	if (!iio_device_claim_direct(indio_dev))
> > +		return -EBUSY;
> > +
> > +	if (st->wait_event) {
> > +		ret = -EBUSY;
> > +		goto out_release;
> > +	}
> > +
> > +	switch (info) {
> > +	case IIO_EV_INFO_VALUE:
> > +		ret = __ad4052_read_event_info_value(st, dir, val);
> > +		break;
> > +	case IIO_EV_INFO_HYSTERESIS:
> > +		ret = __ad4052_read_event_info_hysteresis(st, dir, val);
> > +		break;
> 
> These functions don't need __ prefix. There is no name clash.
> 
Ack.

Best regards,
Jorge
> > +	default:
> > +		ret = -EINVAL;
> > +		break;
> > +	}
> > +
> > +out_release:
> > +	iio_device_release_direct(indio_dev);
> > +	return ret ? ret : IIO_VAL_INT;
> > +}
> > +

On 6/13/25 5:02 AM, Jorge Marques wrote:
> Hi David,
> On Thu, Jun 12, 2025 at 02:38:45PM -0500, David Lechner wrote:
>> On 6/10/25 2:34 AM, Jorge Marques wrote:
>>> The AD4052 family supports autonomous monitoring readings for threshold
>>> crossings. Add support for catching the GPIO interrupt and expose as an IIO
>>> event. The device allows to set either, rising and falling directions. Only
>>> either threshold crossing is implemented.
>>>
>>> Signed-off-by: Jorge Marques <jorge.marques@analog.com>
>>> ---
>>
>> ...
>>
>>> +
>>> +static ssize_t ad4052_events_frequency_store(struct device *dev,
>>> +					     struct device_attribute *attr,
>>> +					     const char *buf,
>>> +					     size_t len)
>>> +{
>>> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>>> +	struct ad4052_state *st = iio_priv(indio_dev);
>>> +	int ret;
>>> +
>>> +	if (!iio_device_claim_direct(indio_dev))
>>> +		return -EBUSY;
>>> +	if (st->wait_event) {
>>> +		ret = -EBUSY;
>>> +		goto out_release;
>>> +	}
>>
>> I'm wondering if we should instead have some kind of iio_device_claim_monitor_mode()
>> so that we don't have to implement this manually everywhere. If monitor mode was
>> claimed, then iio_device_claim_direct() and iio_device_claim_buffer_mode() would
>> both return -EBUSY. If buffer mode was claimed, iio_device_claim_monitor_mode()
>> would fail. If direct mode was claimed, iio_device_claim_monitor_mode() would wait.
>>
> I don't think this would scale with other vendors and devices, it is a

Why not? I've seen lots of devices that have some sort of monitor mode
where they are internally continuously comparing measurements to something
and only signal an interrupt when some condition is met.

> limitation of ADI:ADC:SPI requiring to enter configuration mode to read

I don't see how it could be a limitiation exclusive to this combination of
vendor, sensor type and bus type.

> registers. A deep dive into the other drivers that use IIO Events is
> needed.
>>> +

...

>>> +
>>> +static int ad4052_monitor_mode_disable(struct ad4052_state *st)
>>> +{
>>> +	int ret;
>>> +
>>> +	pm_runtime_mark_last_busy(&st->spi->dev);
>>> +	pm_runtime_put_autosuspend(&st->spi->dev);
>>> +
>>> +	ret = ad4052_exit_command(st);
>>> +	if (ret)
>>> +		return ret;
>>> +	return regmap_write(st->regmap, AD4052_REG_DEVICE_STATUS,
>>> +			    AD4052_REG_DEVICE_STATUS_MAX_FLAG |
>>> +			    AD4052_REG_DEVICE_STATUS_MIN_FLAG);
>>> +}
>>> +
>>
>> It seems like we need to make sure monitor mode is disabled when the
>> driver is removed. Otherwise we could end up with unbalanced calls to
>> the pm_runtime stuff and leave the chip running.
>>
>>
> When monitor mode is enabled, pm is already disabled (won't enter low
> power). I expect the pm to handle the clean-up properly since devm is
> used.
> The .remove() I suggest is reg access to:
> 
> * Put in configuration mode, if not.
> * Put on low power mode, if not.
> 
I was just thinking something like:

	if (st->wait_event)
		ad4052_monitor_mode_disable(st);

Also might need to use devm_add_action_or_reset() instead of .remove
to get correct ordering.

On Fri, 13 Jun 2025 11:03:24 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 6/13/25 5:02 AM, Jorge Marques wrote:
> > Hi David,
> > On Thu, Jun 12, 2025 at 02:38:45PM -0500, David Lechner wrote:  
> >> On 6/10/25 2:34 AM, Jorge Marques wrote:  
> >>> The AD4052 family supports autonomous monitoring readings for threshold
> >>> crossings. Add support for catching the GPIO interrupt and expose as an IIO
> >>> event. The device allows to set either, rising and falling directions. Only
> >>> either threshold crossing is implemented.
> >>>
> >>> Signed-off-by: Jorge Marques <jorge.marques@analog.com>
> >>> ---  
> >>
> >> ...
> >>  
> >>> +
> >>> +static ssize_t ad4052_events_frequency_store(struct device *dev,
> >>> +					     struct device_attribute *attr,
> >>> +					     const char *buf,
> >>> +					     size_t len)
> >>> +{
> >>> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> >>> +	struct ad4052_state *st = iio_priv(indio_dev);
> >>> +	int ret;
> >>> +
> >>> +	if (!iio_device_claim_direct(indio_dev))
> >>> +		return -EBUSY;
> >>> +	if (st->wait_event) {
> >>> +		ret = -EBUSY;
> >>> +		goto out_release;
> >>> +	}  
> >>
> >> I'm wondering if we should instead have some kind of iio_device_claim_monitor_mode()
> >> so that we don't have to implement this manually everywhere. If monitor mode was
> >> claimed, then iio_device_claim_direct() and iio_device_claim_buffer_mode() would
> >> both return -EBUSY. If buffer mode was claimed, iio_device_claim_monitor_mode()
> >> would fail. If direct mode was claimed, iio_device_claim_monitor_mode() would wait.
> >>  
> > I don't think this would scale with other vendors and devices, it is a  
> 
> Why not? I've seen lots of devices that have some sort of monitor mode
> where they are internally continuously comparing measurements to something
> and only signal an interrupt when some condition is met.

There are lots that support such a monitor, but I think far fewer were direct
accesses don't work at the same time.  The max1363 comes to mind but in that
case it is possible to do both monitor and direct reads it is just that the
data format changes and I think we never bothered implementing the handling
for that combination.

I wouldn't mind such helpers if there are at least a couple of users.

> 
> > limitation of ADI:ADC:SPI requiring to enter configuration mode to read  
> 
> I don't see how it could be a limitiation exclusive to this combination of
> vendor, sensor type and bus type.
> 
> > registers. A deep dive into the other drivers that use IIO Events is
> > needed.  
> >>> +  
> 
> ...
> 
> >>> +
> >>> +static int ad4052_monitor_mode_disable(struct ad4052_state *st)
> >>> +{
> >>> +	int ret;
> >>> +
> >>> +	pm_runtime_mark_last_busy(&st->spi->dev);
> >>> +	pm_runtime_put_autosuspend(&st->spi->dev);
> >>> +
> >>> +	ret = ad4052_exit_command(st);
> >>> +	if (ret)
> >>> +		return ret;
> >>> +	return regmap_write(st->regmap, AD4052_REG_DEVICE_STATUS,
> >>> +			    AD4052_REG_DEVICE_STATUS_MAX_FLAG |
> >>> +			    AD4052_REG_DEVICE_STATUS_MIN_FLAG);
> >>> +}
> >>> +  
> >>
> >> It seems like we need to make sure monitor mode is disabled when the
> >> driver is removed. Otherwise we could end up with unbalanced calls to
> >> the pm_runtime stuff and leave the chip running.
> >>
> >>  
> > When monitor mode is enabled, pm is already disabled (won't enter low
> > power). I expect the pm to handle the clean-up properly since devm is
> > used.
> > The .remove() I suggest is reg access to:
> > 
> > * Put in configuration mode, if not.
> > * Put on low power mode, if not.
> >   
> I was just thinking something like:
> 
> 	if (st->wait_event)
> 		ad4052_monitor_mode_disable(st);
> 
> Also might need to use devm_add_action_or_reset() instead of .remove
> to get correct ordering.

On Sat, 14 Jun 2025 11:25:44 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Fri, 13 Jun 2025 11:03:24 -0500
> David Lechner <dlechner@baylibre.com> wrote:
> 
> > On 6/13/25 5:02 AM, Jorge Marques wrote:  
> > > Hi David,
> > > On Thu, Jun 12, 2025 at 02:38:45PM -0500, David Lechner wrote:    
> > >> On 6/10/25 2:34 AM, Jorge Marques wrote:    
> > >>> The AD4052 family supports autonomous monitoring readings for threshold
> > >>> crossings. Add support for catching the GPIO interrupt and expose as an IIO
> > >>> event. The device allows to set either, rising and falling directions. Only
> > >>> either threshold crossing is implemented.
> > >>>
> > >>> Signed-off-by: Jorge Marques <jorge.marques@analog.com>
> > >>> ---    
> > >>
> > >> ...
> > >>    
> > >>> +
> > >>> +static ssize_t ad4052_events_frequency_store(struct device *dev,
> > >>> +					     struct device_attribute *attr,
> > >>> +					     const char *buf,
> > >>> +					     size_t len)
> > >>> +{
> > >>> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > >>> +	struct ad4052_state *st = iio_priv(indio_dev);
> > >>> +	int ret;
> > >>> +
> > >>> +	if (!iio_device_claim_direct(indio_dev))
> > >>> +		return -EBUSY;
> > >>> +	if (st->wait_event) {
> > >>> +		ret = -EBUSY;
> > >>> +		goto out_release;
> > >>> +	}    
> > >>
> > >> I'm wondering if we should instead have some kind of iio_device_claim_monitor_mode()
> > >> so that we don't have to implement this manually everywhere. If monitor mode was
> > >> claimed, then iio_device_claim_direct() and iio_device_claim_buffer_mode() would
> > >> both return -EBUSY. If buffer mode was claimed, iio_device_claim_monitor_mode()
> > >> would fail. If direct mode was claimed, iio_device_claim_monitor_mode() would wait.
> > >>    
> > > I don't think this would scale with other vendors and devices, it is a    
> > 
> > Why not? I've seen lots of devices that have some sort of monitor mode
> > where they are internally continuously comparing measurements to something
> > and only signal an interrupt when some condition is met.  
> 
> There are lots that support such a monitor, but I think far fewer were direct
> accesses don't work at the same time.  The max1363 comes to mind but in that
> case it is possible to do both monitor and direct reads it is just that the
> data format changes and I think we never bothered implementing the handling
> for that combination.
> 
> I wouldn't mind such helpers if there are at least a couple of users.
> 
I got this wrong.  Key here is not direct access and monitor, but rather
monitor and buffering (which is the odd format case on the max1363 etc).

Anyhow, conclusion that helpers are fine is the same.

I would try to minimise what doesn't work when monitor mode is enabled though
(as commented in review of this patch).

We also have to cover the internal cases where buffer mode is claimed but
there isn't (IIRC) a call to that particular claim function as we don't
want to end up holding the lock - same will be true for monitor mode - there
is a difference between temporary fixing of state where locks are fine
and the mode running for a long period in which we don't hold the lock.

> >   
> > > limitation of ADI:ADC:SPI requiring to enter configuration mode to read    
> > 
> > I don't see how it could be a limitiation exclusive to this combination of
> > vendor, sensor type and bus type.
> >   
> > > registers. A deep dive into the other drivers that use IIO Events is
> > > needed.    
> > >>> +    
> > 
> > ...
> >   
> > >>> +
> > >>> +static int ad4052_monitor_mode_disable(struct ad4052_state *st)
> > >>> +{
> > >>> +	int ret;
> > >>> +
> > >>> +	pm_runtime_mark_last_busy(&st->spi->dev);
> > >>> +	pm_runtime_put_autosuspend(&st->spi->dev);
> > >>> +
> > >>> +	ret = ad4052_exit_command(st);
> > >>> +	if (ret)
> > >>> +		return ret;
> > >>> +	return regmap_write(st->regmap, AD4052_REG_DEVICE_STATUS,
> > >>> +			    AD4052_REG_DEVICE_STATUS_MAX_FLAG |
> > >>> +			    AD4052_REG_DEVICE_STATUS_MIN_FLAG);
> > >>> +}
> > >>> +    
> > >>
> > >> It seems like we need to make sure monitor mode is disabled when the
> > >> driver is removed. Otherwise we could end up with unbalanced calls to
> > >> the pm_runtime stuff and leave the chip running.
> > >>
> > >>    
> > > When monitor mode is enabled, pm is already disabled (won't enter low
> > > power). I expect the pm to handle the clean-up properly since devm is
> > > used.
> > > The .remove() I suggest is reg access to:
> > > 
> > > * Put in configuration mode, if not.
> > > * Put on low power mode, if not.
> > >     
> > I was just thinking something like:
> > 
> > 	if (st->wait_event)
> > 		ad4052_monitor_mode_disable(st);
> > 
> > Also might need to use devm_add_action_or_reset() instead of .remove
> > to get correct ordering.  
> 
>