linux-kernel - Re: [PATCH 5/7] iio: adc: ad4062: Add IIO Trigger support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <egfjokiqoo6dxh6m3dhjtj2jtpbnw6p7pxxllth2unycl5fdf4@lirpbye74rtg>
Date: Sun, 23 Nov 2025 20:48:27 +0100
From: Jorge Marques <gastmaier@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Jorge Marques <jorge.marques@...log.com>, 
	Lars-Peter Clausen <lars@...afoo.de>, Michael Hennerich <Michael.Hennerich@...log.com>, 
	David Lechner <dlechner@...libre.com>, Nuno Sá <nuno.sa@...log.com>, 
	Andy Shevchenko <andy@...nel.org>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Jonathan Corbet <corbet@....net>, linux-iio@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org
Subject: Re: [PATCH 5/7] iio: adc: ad4062: Add IIO Trigger support

On Sat, Oct 18, 2025 at 05:14:25PM +0100, Jonathan Cameron wrote:
> On Mon, 13 Oct 2025 09:28:03 +0200
> Jorge Marques <jorge.marques@...log.com> wrote:
> 
Hi Jonathan,

> > 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.
> > 
> > Signed-off-by: Jorge Marques <jorge.marques@...log.com>
> 
> A few things inline.
> Thanks,
> 
> > diff --git a/drivers/iio/adc/ad4062.c b/drivers/iio/adc/ad4062.c
> > index e55a69c62694a71a4e29f29b9a2bfeec3b16c990..40b7c10b8ce7145b010bb11e8e4698baacb6b3d3 100644
> > --- a/drivers/iio/adc/ad4062.c
> > +++ b/drivers/iio/adc/ad4062.c
> 
> > +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);
> > +	u8 addr = AD4062_REG_CONV_TRIGGER;
> > +	int ret;
> > +
> > +	/* Read current and trigger next conversion */
> > +	struct i3c_priv_xfer t[2] = {
> > +		{
> > +			.data.in = &st->raw,
> 
> If it is safe to use addr on the stack, do we need to have
> a dma safe buffer for raw?  I'm not sure for i3c!
> 
All buffer should be dma aligned, I will use the heap.
I will use a separate buffer that is written once (CONV_READ or
CONV_TRIGGER, depending if gp1 is routed or using the IBI fallback).
> > +			.len = 4,
> > +			.rnw = true,
> > +		},
> > +		{
> > +			.data.out = &addr,
> > +			.len = 1,
> > +			.rnw = false,
> > +		}
> > +	};
> > +
> > +	/* Separated transfers to not infeer repeated-start */
> > +	ret = i3c_device_do_priv_xfers(st->i3cdev, &t[0], 1);
> > +	if (ret)
> > +		goto out;
> > +	ret = i3c_device_do_priv_xfers(st->i3cdev, &t[1], 1);
> 
> Add a comment on this. I assume it's setting things up for the next
> scan?
> 
yes, the next scan is triggered after the reading of the current scan.
There are 2 registers that can be used for scans, CONV_READ and
CONV_TRIGGER:
* CONV_READ: Stop bit of the previous read (without write address),
  triggers the next scan. Allows roughly twice the sample rate, since
  does not requires writing the address every transfer.
* CONV_TRIGGER: The conversion is triggered by writing the address, so
  every new conversion requires writing the address again. Only this
  registers will issue an IBI data ready.

That means, if GPIO is not routed as the IRQ, in the fallback using IBI,
CONV_TRIGGER needs to be used. v2 will also use schedule_work to avoid
the IRQF_ONESHOT being triggered (next conversion data ready), before
the current irq_handler returns.
> > +	if (ret)
> > +		goto out;
> > +
> > +	iio_push_to_buffers_with_timestamp(indio_dev, &st->raw,
> > +					   pf->timestamp);
> > +
> > +out:
> > +	iio_trigger_notify_done(indio_dev->trig);
> > +
> > +	return IRQ_HANDLED;
> >  }
> 
> > +
> > +static int ad4062_triggered_buffer_predisable(struct iio_dev *indio_dev)
> > +{
> > +	struct ad4062_state *st = iio_priv(indio_dev);
> > +
> > +	pm_runtime_mark_last_busy(&st->i3cdev->dev);
> 
> Take a look at the changes across the tree recently.
> Now pm_runtime_put_autosuspend() calls pm_runtime_mark_last_busy()
> internally to avoid the need for this pair.
> 
Ack.
> > +	pm_runtime_put_autosuspend(&st->i3cdev->dev);
> > +	return 0;
> > +}
> 
> 
Best regards,
Jorge