Re: [PATCH v2 5/9] iio: adc: ad4062: Add IIO Trigger support

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

On Mon, Nov 24, 2025 at 10:18:04AM +0100, Jorge Marques wrote:
> Adds support for IIO Trigger. Optionally, gp1 is assigned as Data Ready
> signal, if not present, fallback to an I3C IBI with the same role.
> The software trigger is allocated by the device, but must be attached by
> the user before enabling the buffer. The purpose is to not impede
> removing the driver due to the increased reference count when
> iio_trigger_set_immutable or iio_trigger_get is used.

We refer to the functions as func(). Mind the parentheses.

...

> +	struct ad4062_state *st = container_of(work, struct ad4062_state,
> +					       trig_conv);

I think the

	struct ad4062_state *st =
		container_of(work, struct ad4062_state, trig_conv);

reads better.

> +	int ret;

...

> +	/* Read current conversion, if at reg CONV_READ, stop bit triggers
> +	 * next sample and does not need writing the address.
> +	 */

/*
 * The multi-line comment style is as in
 * this example. Please, check and update.
 */

> +static irqreturn_t ad4062_poll_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct ad4062_state *st = iio_priv(indio_dev);
> +
> +	iio_trigger_notify_done(indio_dev->trig);
> +	schedule_work(&st->trig_conv);
> +
> +	return IRQ_HANDLED;
>  }

...

> +static int ad4062_triggered_buffer_postenable(struct iio_dev *indio_dev)
> +{
> +	struct ad4062_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = pm_runtime_resume_and_get(&st->i3cdev->dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad4062_set_operation_mode(st, st->mode);
> +	if (ret)
> +		goto out_mode_error;
> +
> +	/* CONV_READ requires read to trigger first sample. */
> +	struct i3c_priv_xfer t[2] = {
> +		{
> +			.data.out = &st->reg_addr_conv,
> +			.len = sizeof(st->reg_addr_conv),
> +			.rnw = false,
> +		},
> +		{
> +			.data.in = &st->buf.be32,
> +			.len = sizeof(st->buf.be32),
> +			.rnw = true,
> +		}
> +	};
> +
> +	ret = i3c_device_do_priv_xfers(st->i3cdev, t, st->gpo_irq[1] ? 2 : 1);
> +	if (ret)
> +		goto out_mode_error;
> +	return 0;
> +
> +out_mode_error:
> +	pm_runtime_put_autosuspend(&st->i3cdev->dev);
> +
> +	return ret;

I guess with ACQUIRE() this function will look better, because the explicit
reference count bumping (with an associated comment) is more descriptive on
what's going on here with PM. Same for other related functions.

> +}

...

>  	if (ret)
>  		return dev_err_probe(dev, ret, "Failed to request i3c ibi\n");
>  
> +	INIT_WORK(&st->trig_conv, ad4062_trigger_work);

This is mixture of devm_*() and non-devm_*() calls. How did you (stress) test
the removal and error paths here? Wouldn't devm-helpers.h APIs help here to
make / keep order correct?

>  	return devm_iio_device_register(dev, indio_dev);

-- 
With Best Regards,
Andy Shevchenko