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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Mon, Nov 24, 2025 at 10:18:06AM +0100, Jorge Marques 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.

...

> +static ssize_t ad4062_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 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)
> +		goto out_release;
> +
> +	st->events_frequency = find_closest_descending(val, ad4062_conversion_freqs,
> +						       ARRAY_SIZE(ad4062_conversion_freqs));
> +	ret = 0;
> +
> +out_release:
> +	iio_device_release_direct(indio_dev);
> +	return ret ? ret : len;

	return ret ?: len;

> +}

...

> +static IIO_DEVICE_ATTR(sampling_frequency, 0644, ad4062_events_frequency_show,
> +		       ad4062_events_frequency_store, 0);

IIO_DEVICE_ATTR_RW()

...

>  {
>  	struct ad4062_state *st = i3cdev_get_drvdata(i3cdev);
>  
> -	if (iio_buffer_enabled(st->indio_dev))
> -		iio_trigger_poll_nested(st->trigger);
> -	else
> -		complete(&st->completion);
> +	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));
> +	} else {
> +		if (iio_buffer_enabled(st->indio_dev))
> +			iio_trigger_poll_nested(st->trigger);
> +		else
> +			complete(&st->completion);
> +	}

Less ping-pong:ish if you simply add a new code.

	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;
	}

>  }

...

> +static int ad4062_monitor_mode_enable(struct ad4062_state *st, bool enable)
> +{
> +	int ret = 0;

Unneeded assignment.

> +	if (!enable) {
> +		pm_runtime_put_autosuspend(&st->i3cdev->dev);
> +		return 0;
> +	}

Just split to two functions and drop parameter 'enable',

> +	ACQUIRE(pm_runtime_active_try_enabled, pm)(&st->i3cdev->dev);
> +	ret = ACQUIRE_ERR(pm_runtime_active_try_enabled, &pm);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad4062_conversion_frequency_set(st, st->events_frequency);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad4062_set_operation_mode(st, AD4062_MONITOR_MODE);
> +	if (ret)
> +		return ret;
> +
> +	pm_runtime_get_noresume(&st->i3cdev->dev);
> +	return 0;
> +}

...

> +static int ad4062_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 ad4062_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;
> +	}
> +
> +	ret = ad4062_monitor_mode_enable(st, state);
> +	if (!ret)
> +		st->wait_event = state;

Please use regular patter to check for errors first.

	if (st->wait_event == state)
		ret = 0;
	else
		ret = ad4062_monitor_mode_enable(st, state);
	if (ret)
		goto out_release;

	st->wait_event = state;

Always think about readability first and then about size of the source code.

> +out_release:
> +	iio_device_release_direct(indio_dev);
> +	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;
> +	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 ? ret : IIO_VAL_INT;

	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 > 2047 || val < -2048)
> +		return -EINVAL;

There was already magic '11', perhaps define it and use there and here?

#define x11	11 // needs a good name

	if (val > BIT(x11) || val < -BIT(x11))

> +	if (dir == IIO_EV_DIR_RISING)
> +		reg = AD4062_REG_MAX_LIMIT;
> +	else
> +		reg = AD4062_REG_MIN_LIMIT;
> +	put_unaligned_be16(val, st->buf.bytes);
> +
> +	return regmap_bulk_write(st->regmap, reg, &st->buf.be16,
> +				 sizeof(st->buf.be16));
> +}

-- 
With Best Regards,
Andy Shevchenko