| 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
| ||
|
Message-ID: <h6d6bjuhb4ovnz5jghj4h3vkcqzypzbdgi5ufdmbv24x34a3px@nawt5lt7dsux>
Date: Mon, 8 Dec 2025 22:17:20 +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>, Linus Walleij <linus.walleij@...aro.org>,
Bartosz Golaszewski <brgl@...ev.pl>, linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org, linux-gpio@...r.kernel.org,
"Rafael J. Wysocki" <rafael@...nel.org>
Subject: Re: [PATCH v3 5/9] iio: adc: ad4062: Add IIO Trigger support
On Sat, Dec 06, 2025 at 05:45:03PM +0000, Jonathan Cameron wrote:
> On Fri, 5 Dec 2025 16:12:06 +0100
> 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>
>
> +CC Rafael; I'd like input on the ACQUIRE + take extra reference pattern
> and whether Rafael thinks it is a good idea!
>
> > ---
> > drivers/iio/adc/Kconfig | 2 +
> > drivers/iio/adc/ad4062.c | 188 +++++++++++++++++++++++++++++++++++++++++++----
> > 2 files changed, 175 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > index e506dbe83f488..ddb7820f0bdcc 100644
> > --- a/drivers/iio/adc/Kconfig
> > +++ b/drivers/iio/adc/Kconfig
> > @@ -74,6 +74,8 @@ config AD4062
> > tristate "Analog Devices AD4062 Driver"
> > depends on I3C
> > select REGMAP_I3C
> > + select IIO_BUFFER
> > + select IIO_TRIGGERED_BUFFER
> > help
> > Say yes here to build support for Analog Devices AD4062 I3C analog
> > to digital converters (ADC).
> > diff --git a/drivers/iio/adc/ad4062.c b/drivers/iio/adc/ad4062.c
> > index 54f7f69e40879..080dc80fd1621 100644
> > --- a/drivers/iio/adc/ad4062.c
> > +++ b/drivers/iio/adc/ad4062.c
>
> > +static void ad4062_trigger_work(struct work_struct *work)
> > +{
> > + struct ad4062_state *st =
> > + container_of(work, struct ad4062_state, trig_conv);
> > + int ret;
> > +
> > + /*
> > + * Read current conversion, if at reg CONV_READ, stop bit triggers
> > + * next sample and does not need writing the address.
> > + */
> > + struct i3c_priv_xfer t[2] = {
> > + {
> > + .data.in = &st->buf.be32,
> > + .len = sizeof(st->buf.be32),
> > + .rnw = true,
> > + },
> > + {
> > + .data.out = &st->reg_addr_conv,
> > + .len = sizeof(st->reg_addr_conv),
> > + .rnw = false,
> > + },
> > + };
> > +
> > + ret = i3c_device_do_priv_xfers(st->i3cdev, &t[0], 1);
> > + if (ret)
> > + return;
> > +
> > + iio_push_to_buffers_with_timestamp(st->indio_dev, &st->buf.be32,
> > + iio_get_time_ns(st->indio_dev));
>
> Use push_to_buffers_with_ts() (this function is deprecated)
> which would have had the helpful result here of pointing out the buffer
> isn't big enough for the timestamp. So this will write the timestamp
> over later fields in the st structure.
>
> Given that this sometimes fits in a be16 I wonder if it is worth
> storing those in a be16 element of the kfifo. That will halve it's size
> if the timestamp isn't enabled which would be a nice thing to have.
> Storing in a be32 isn't an ABI issue, it's just a bit unusual
> so if I'm missing some reason it makes more sense then fair enough.
>
Per last e-mail, due to ad4062 burst avg, it will be kept as 32-bits.
const bool is_32b = st->chip->prod_id == 0x7C;
const size_t _sizeof = is_32b ? sizeof(st->buf.be32) : sizeof(st->buf.be16);
//...
iio_push_to_buffers_with_ts(st->indio_dev, &st->buf.be32, _sizeof,
iio_get_time_ns(st->indio_dev));
> > + if (st->gpo_irq[1])
> > + return;
> > +
> > + i3c_device_do_priv_xfers(st->i3cdev, &t[1], 1);
> > +}
>
> ...
>
> > + {
> > + .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,
> > + }
> > };
>
> > @@ -687,6 +782,55 @@ static int ad4062_write_raw(struct iio_dev *indio_dev,
> > return ret;
> > }
> >
> > +static int ad4062_triggered_buffer_postenable(struct iio_dev *indio_dev)
> > +{
> > + struct ad4062_state *st = iio_priv(indio_dev);
> > + int ret;
> > +
> > + ACQUIRE(pm_runtime_active_try_enabled, pm)(&st->i3cdev->dev);
> > + ret = ACQUIRE_ERR(pm_runtime_active_try_enabled, &pm);
>
> This may also be affected by Rafael's patch set to provide some helpers
> to make this more readable.
>
>
> > + if (ret)
> > + return ret;
> > +
> > + ret = ad4062_set_operation_mode(st, st->mode);
> > + if (ret)
> > + return ret;
> > +
> > + /* 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)
> > + return ret;
> > +
> > + pm_runtime_get_noresume(&st->i3cdev->dev);
> As per my late reply I'm not keen on the double increment as a complex way
> to steal the ACQUIRED() reference. Might be better to just factor the stuff
> where you currently have acquired a reference out into a helper and use
> the traditional runtime pm calls in this outer function.
>
I will use a helper pm_ad4062_monitor_mode_enable() and
pm_ad4062_triggered_buffer_postenable().
> > + return 0;
>
>
Best regards,
Jorge
Powered by blists - more mailing lists