On Mon, Nov 24, 2025 at 11:36:12AM +0200, Andy Shevchenko wrote:
> On Mon, Nov 24, 2025 at 10:18:04AM +0100, Jorge Marques wrote:
Hi Andy,
> > 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.
> 
> ...
> 
Ack.
> > +	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.
> 
Ack.
> > +	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.
>  */
> 
Ack.
> > +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.
> 
Oh this one slipped through my fingers, should be using ACQUIRE as well, thanks
> > +}
> 
> ...
> 
> >  	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?
> 
Oh yeah, missed this helper

  	ret = devm_work_autocancel(dev, &st->trig_conv, ad4062_trigger_work);
  	if (ret)
  		return ret;

the path was missing clean-up, and did cause issue if there was work
being done on detach

  ERROR: iio:device0: Unable to get next block: Connection timed out (110)
  8<--- cut here ---
  Unable to handle kernel paging request at virtual address bf00715c when execute
  [bf00715c] *pgd=0b43f811, *pte=00000000, *ppte=00000000
  Internal error: Oops: 80000007 [#1] PREEMPT SMP ARM
  Modules linked in: ad4062 regmap_i3c nvmem_axi_sysid [last unloaded: adi_i3c_master]
  CPU: 0 UID: 0 PID: 8033 Comm: kworker/0:6 Not tainted 6.12.0+ #4
  Hardware name: Xilinx Zynq Platform
  Workqueue: events ad4062_trigger_work [ad4062]
  PC is at 0xbf00715c
  LR is at wait_for_completion_timeout+0xf0/0x118
  pc : [<bf00715c>]    lr : [<c07ec920>]    psr: 60000013
  sp : e0911ec0  ip : 00000000  fp : dfb960e0
  r10: 00000011  r9 : 00000001  r8 : c4b89d40
  r7 : c49e4500  r6 : c49da040  r5 : 00000001  r4 : e0911ef4
  r3 : cb455500  r2 : 00000000  r1 : 00000000  r0 : 00000000
  Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none

thanks,

Best regards,
Jorge
> >  	return devm_iio_device_register(dev, indio_dev);
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>