Re: [PATCH 7/7] iio: adc: ad4062: Add IIO Events support

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 13 Oct 2025 09:28:05 +0200
Jorge Marques <jorge.marques@analog.com> wrote:

> Adds support for IIO Events. Optionally, gp0 is assigned as Threshold
> Either signal, if not present, fallback to an I3C IBI with the same
> role.
> 
> Signed-off-by: Jorge Marques <jorge.marques@analog.com>
The one bit of this that I'm not sure on is the apparent dropping out
of monitor mode on most userspace interactions that cause register accesses.
That seems like a fairly unintuitive ABI. It might be better to block the access
until the events are turned off. Perhaps I missed something?

Thanks,

Jonathan

> ---
>  drivers/iio/adc/ad4062.c | 351 ++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 347 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad4062.c b/drivers/iio/adc/ad4062.c
> index 40b7c10b8ce7145b010bb11e8e4698baacb6b3d3..b5b12f81c71b52f244600ed23dad11253290b868 100644
> --- a/drivers/iio/adc/ad4062.c
> +++ b/drivers/iio/adc/ad4062.c
> @@ -13,6 +13,7 @@

> +/**
> + * A register access will cause the device to drop from monitor mode
> + * into configuration mode, update the state to reflect that.
> + */
> +static void ad4062_exit_monitor_mode(struct ad4062_state *st)
> +{
> +	if (st->wait_event) {
> +		pm_runtime_mark_last_busy(&st->i3cdev->dev);

> +		pm_runtime_put_autosuspend(&st->i3cdev->dev);
As elsewhere, no longer need to have the mark_last_busy() call here.

> +		st->wait_event = 0;
> +	}
> +}

> +static ssize_t sampling_frequency_available_show(struct device *dev,
> +						 struct device_attribute *attr,
> +						 char *buf)
> +{
> +	struct ad4062_state *st = iio_priv(dev_to_iio_dev(dev));
> +	int ret = 0;
> +
> +	for (u8 i = AD4062_FS_OFFSET(st->chip->grade);
> +	     i < AD4062_FS_LEN(st->chip->grade); i++)
> +		ret += sysfs_emit_at(buf, ret, "%s ", ad4062_conversion_freqs[i]);
> +
> +	ret += sysfs_emit_at(buf, ret, "\n");
> +	return ret;

Has slightly ugly format of " \n" at end rather than "\n"
There are various ways to handle this perhaps easiest is something like
	for (u8 i = AD4062_FS_OFFSET(st->chip->grade);
	     i < AD4062_FS_LEN(st->chip->grade); i++)
		ret += sysfs_emit_at(buf, ret, "%s%c", ad4062_conversion_freqs[i],
				      i != (AD4062_FS_LEN(st->chip_grade) - 1) ? "\n", " ");


> +}

>  static irqreturn_t ad4062_poll_handler(int irq, void *p)
> @@ -523,6 +645,24 @@ static int ad4062_request_irq(struct iio_dev *indio_dev)
>  	struct device *dev = &st->i3cdev->dev;
>  	int ret;
>  
> +	ret = fwnode_irq_get_byname(dev_fwnode(&st->i3cdev->dev), "gp0");
> +	if (ret >= 0) {
> +		ret = devm_request_threaded_irq(dev, ret, NULL,
> +						ad4062_irq_handler_thresh,
> +						IRQF_ONESHOT, indio_dev->name,
> +						indio_dev);
> +		if (ret)
> +			return ret;
> +	} else if (ret != -EPROBE_DEFER) {
> +		ret = regmap_update_bits(st->regmap, AD4062_REG_ADC_IBI_EN,
> +					 AD4062_REG_ADC_IBI_EN_MAX | AD4062_REG_ADC_IBI_EN_MIN,
> +					 AD4062_REG_ADC_IBI_EN_MAX | AD4062_REG_ADC_IBI_EN_MIN);
> +		if (ret)
> +			return ret;
> +	} else {
> +		return ret;

As before. I'd prefer error cases handled first. The earlier code suggestion
doesn't quite work but something along those lines should be doable.

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

> @@ -779,6 +923,196 @@ static int ad4062_write_raw(struct iio_dev *indio_dev,
>  	return ret;
>  }
>  
> +static int ad4062_monitor_mode_enable(struct ad4062_state *st, bool enable)
> +{
> +	int ret = 0;
> +
> +	if (!enable)
> +		goto out_suspend;
> +
> +	ret = pm_runtime_resume_and_get(&st->i3cdev->dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad4062_conversion_frequency_set(st, st->events_frequency);
> +	if (ret)
> +		goto out_suspend;
> +
> +	ret = ad4062_set_operation_mode(st, AD4062_MONITOR_MODE);
> +	if (ret)
> +		goto out_suspend;
> +
> +	return ret;
return 0;

> +out_suspend:
> +	pm_runtime_put_autosuspend(&st->i3cdev->dev);
> +	return ret;
> +}

>  static int ad4062_triggered_buffer_postenable(struct iio_dev *indio_dev)
>  {
>  	struct ad4062_state *st = iio_priv(indio_dev);
> @@ -788,6 +1122,7 @@ static int ad4062_triggered_buffer_postenable(struct iio_dev *indio_dev)
>  	ret = pm_runtime_resume_and_get(&st->i3cdev->dev);
>  	if (ret)
>  		return ret;
> +	ad4062_exit_monitor_mode(st);
Hmm. So you always exist monitor mode if we enable the buffer. I assume that doesn't
change detection of events because the buffered mode also allows that?

Do we not need something to turn monitor mode on again once we disable buffered capture?
>  
>  	ret = ad4062_set_operation_mode(st, st->mode);
>  	if (ret)
> @@ -833,6 +1168,7 @@ static int ad4062_debugfs_reg_access(struct iio_dev *indio_dev, unsigned int reg
>  
>  	if (!iio_device_claim_direct(indio_dev))
>  		return -EBUSY;
> +	ad4062_exit_monitor_mode(st);

This probably needs a comment. Not obvious to me how you end up in with it enabled
again after the debugfs read / write finishes.

>  
>  	if (readval)
>  		ret = regmap_read(st->regmap, reg, readval);