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

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 5 Dec 2025 16:12:08 +0100
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>
> ---
Various comments inline.

Thanks,

Jonathan

> +static ssize_t sampling_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 ad4062_state *st = iio_priv(indio_dev);
> +	int val, ret;
> +
> +	if (!iio_device_claim_direct(indio_dev))
> +		return -EBUSY;
> +	if (st->wait_event) {
> +		ret = -EBUSY;
> +		goto out_release;
> +	}
> +
> +	ret = kstrtoint(buf, 10, &val);
> +	if (ret < 0)

if (ret)
 would make the following bit about ret == 0 if this isn't true
more obvious.

> +		goto out_release;
> +
> +	st->events_frequency = find_closest_descending(val, ad4062_conversion_freqs,
> +						       ARRAY_SIZE(ad4062_conversion_freqs));
> +	ret = 0;
If you get here it's zero anyway.
> +
> +out_release:
> +	iio_device_release_direct(indio_dev);
> +	return ret ?: len;
> +}

>  static irqreturn_t ad4062_irq_handler_drdy(int irq, void *private)
>  {
>  	struct iio_dev *indio_dev = private;
> @@ -432,6 +549,14 @@ static void ad4062_ibi_handler(struct i3c_device *i3cdev,
>  {
>  	struct ad4062_state *st = i3cdev_get_drvdata(i3cdev);
>  
> +	if (st->wait_event) {
> +		iio_push_event(st->indio_dev,
> +			       IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE, 0,
> +						    IIO_EV_TYPE_THRESH,
> +						    IIO_EV_DIR_EITHER),
> +			       iio_get_time_ns(st->indio_dev));
> +		return;
> +	}
>  	if (iio_buffer_enabled(st->indio_dev))
>  		iio_trigger_poll_nested(st->trigger);
>  	else
> @@ -523,6 +648,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 == -EPROBE_DEFER) {
> +		return ret;
> +	} else if (ret < 0) {

no need for else.

> +		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 {
> +		ret = devm_request_threaded_irq(dev, ret, NULL,
> +						ad4062_irq_handler_thresh,
> +						IRQF_ONESHOT, indio_dev->name,
> +						indio_dev);
> +		if (ret)
> +			return ret;
> +	}

> +static int ad4062_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 ad4062_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	if (!iio_device_claim_direct(indio_dev))
> +		return -EBUSY;

Similar to below. Consider factoring out this stuff or I guess wait
for an ACQUIRE() based standard solution.

> +	if (st->wait_event) {
> +		ret = -EBUSY;
> +		goto out_release;
> +	}
> +
> +	switch (info) {
> +	case IIO_EV_INFO_VALUE:
> +		ret = __ad4062_read_event_info_value(st, dir, val);
> +		break;
> +	case IIO_EV_INFO_HYSTERESIS:
> +		ret = __ad4062_read_event_info_hysteresis(st, dir, val);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +out_release:
> +	iio_device_release_direct(indio_dev);
> +	return ret ?: IIO_VAL_INT;
> +}
> +
> +static int __ad4062_write_event_info_value(struct ad4062_state *st,
> +					   enum iio_event_direction dir, int val)
> +{
> +	u8 reg;
> +
> +	if (val != sign_extend32(val, AD4062_LIMIT_BITS - 1))
> +		return -EINVAL;
> +	if (dir == IIO_EV_DIR_RISING)
> +		reg = AD4062_REG_MAX_LIMIT;
> +	else
> +		reg = AD4062_REG_MIN_LIMIT;
> +	put_unaligned_be16(val, st->buf.bytes);

I'll stop comment on this now an assume you'll find all these places
where we know it is aligned and so don't need to use these less efficient
functions.

> +
> +	return regmap_bulk_write(st->regmap, reg, &st->buf.be16,
> +				 sizeof(st->buf.be16));
> +}

> +
> +static int ad4062_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 ad4062_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	if (!iio_device_claim_direct(indio_dev))
> +		return -EBUSY;

Whilst I do plan to take a look at whether we can do an ACQUIRE pattern
like the runtime pm ones, for now (unless you fancy taking that on)
I'd be tempted to factor out this stuff under the direct mode claim into
a helper that can then do direct returns. That should end up easier to ready
that this.

> +	if (st->wait_event) {
> +		ret = -EBUSY;
> +		goto out_release;
> +	}
> +
> +	switch (type) {
> +	case IIO_EV_TYPE_THRESH:
> +		switch (info) {
> +		case IIO_EV_INFO_VALUE:
> +			ret = __ad4062_write_event_info_value(st, dir, val);
> +			break;
> +		case IIO_EV_INFO_HYSTERESIS:
> +			ret = __ad4062_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;
>  }