| 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: <xxwczo3pno3dkyn3jmtj7hwupeo3ywp27bwbtohexfgpdoo56j@apapxa2pymyw>
Date: Sun, 23 Nov 2025 20:48:47 +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 7/7] iio: adc: ad4062: Add IIO Events support
On Sat, Oct 18, 2025 at 05:26:25PM +0100, Jonathan Cameron wrote:
> On Mon, 13 Oct 2025 09:28:05 +0200
> Jorge Marques <jorge.marques@...log.com> 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.
> >
> > Signed-off-by: Jorge Marques <jorge.marques@...log.com>
Hi Jonathan,
> The one bit of this that I'm not sure on is the apparent dropping out
> of monitor mode on most userspace interactions that cause register accesses.
> That seems like a fairly unintuitive ABI. It might be better to block the access
> until the events are turned off. Perhaps I missed something?
>
I will change the behaviour to block access until monitor mode is disabled:
if (!iio_device_claim_direct(indio_dev))
return -EBUSY;
if (st->wait_event) {
ret = -EBUSY;
goto out_release;
}
> Thanks,
>
> Jonathan
>
> > ---
> > drivers/iio/adc/ad4062.c | 351 ++++++++++++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 347 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/iio/adc/ad4062.c b/drivers/iio/adc/ad4062.c
> > index 40b7c10b8ce7145b010bb11e8e4698baacb6b3d3..b5b12f81c71b52f244600ed23dad11253290b868 100644
> > --- a/drivers/iio/adc/ad4062.c
> > +++ b/drivers/iio/adc/ad4062.c
> > @@ -13,6 +13,7 @@
>
> > +/**
> > + * A register access will cause the device to drop from monitor mode
> > + * into configuration mode, update the state to reflect that.
> > + */
> > +static void ad4062_exit_monitor_mode(struct ad4062_state *st)
> > +{
> > + if (st->wait_event) {
> > + pm_runtime_mark_last_busy(&st->i3cdev->dev);
>
> > + pm_runtime_put_autosuspend(&st->i3cdev->dev);
> As elsewhere, no longer need to have the mark_last_busy() call here.
>
Ack.
> > + st->wait_event = 0;
> > + }
> > +}
>
> > +static ssize_t sampling_frequency_available_show(struct device *dev,
> > + struct device_attribute *attr,
> > + char *buf)
> > +{
> > + struct ad4062_state *st = iio_priv(dev_to_iio_dev(dev));
> > + int ret = 0;
> > +
> > + for (u8 i = AD4062_FS_OFFSET(st->chip->grade);
> > + i < AD4062_FS_LEN(st->chip->grade); i++)
> > + ret += sysfs_emit_at(buf, ret, "%s ", ad4062_conversion_freqs[i]);
> > +
> > + ret += sysfs_emit_at(buf, ret, "\n");
> > + return ret;
>
> Has slightly ugly format of " \n" at end rather than "\n"
> There are various ways to handle this perhaps easiest is something like
> for (u8 i = AD4062_FS_OFFSET(st->chip->grade);
> i < AD4062_FS_LEN(st->chip->grade); i++)
> ret += sysfs_emit_at(buf, ret, "%s%c", ad4062_conversion_freqs[i],
> i != (AD4062_FS_LEN(st->chip_grade) - 1) ? "\n", " ");
Thanks for the suggestion, I will use.
>
>
> > +}
>
> > static irqreturn_t ad4062_poll_handler(int irq, void *p)
> > @@ -523,6 +645,24 @@ static int ad4062_request_irq(struct iio_dev *indio_dev)
> > struct device *dev = &st->i3cdev->dev;
> > int ret;
> >
> > + ret = fwnode_irq_get_byname(dev_fwnode(&st->i3cdev->dev), "gp0");
> > + if (ret >= 0) {
> > + ret = devm_request_threaded_irq(dev, ret, NULL,
> > + ad4062_irq_handler_thresh,
> > + IRQF_ONESHOT, indio_dev->name,
> > + indio_dev);
> > + if (ret)
> > + return ret;
> > + } else if (ret != -EPROBE_DEFER) {
> > + ret = regmap_update_bits(st->regmap, AD4062_REG_ADC_IBI_EN,
> > + AD4062_REG_ADC_IBI_EN_MAX | AD4062_REG_ADC_IBI_EN_MIN,
> > + AD4062_REG_ADC_IBI_EN_MAX | AD4062_REG_ADC_IBI_EN_MIN);
> > + if (ret)
> > + return ret;
> > + } else {
> > + return ret;
>
> As before. I'd prefer error cases handled first. The earlier code suggestion
> doesn't quite work but something along those lines should be doable.
>
Ack.
> > + }
> > +
> > ret = fwnode_irq_get_byname(dev_fwnode(&st->i3cdev->dev), "gp1");
> > if (ret >= 0) {
> > ret = devm_request_threaded_irq(dev, ret, NULL,
>
> > @@ -779,6 +923,196 @@ static int ad4062_write_raw(struct iio_dev *indio_dev,
> > return ret;
> > }
> >
> > +static int ad4062_monitor_mode_enable(struct ad4062_state *st, bool enable)
> > +{
> > + int ret = 0;
> > +
> > + if (!enable)
> > + goto out_suspend;
> > +
> > + ret = pm_runtime_resume_and_get(&st->i3cdev->dev);
> > + if (ret)
> > + return ret;
> > +
> > + ret = ad4062_conversion_frequency_set(st, st->events_frequency);
> > + if (ret)
> > + goto out_suspend;
> > +
> > + ret = ad4062_set_operation_mode(st, AD4062_MONITOR_MODE);
> > + if (ret)
> > + goto out_suspend;
> > +
> > + return ret;
> return 0;
>
Ack.
> > +out_suspend:
> > + pm_runtime_put_autosuspend(&st->i3cdev->dev);
> > + return ret;
> > +}
>
> > static int ad4062_triggered_buffer_postenable(struct iio_dev *indio_dev)
> > {
> > struct ad4062_state *st = iio_priv(indio_dev);
> > @@ -788,6 +1122,7 @@ static int ad4062_triggered_buffer_postenable(struct iio_dev *indio_dev)
> > ret = pm_runtime_resume_and_get(&st->i3cdev->dev);
> > if (ret)
> > return ret;
> > + ad4062_exit_monitor_mode(st);
> Hmm. So you always exist monitor mode if we enable the buffer. I assume that doesn't
> change detection of events because the buffered mode also allows that?
>
> Do we not need something to turn monitor mode on again once we disable buffered capture?
No, the buffer mode does not work alongside monitor mode, I will add the
lock
if (st->wait_event)
return -EBUSY;
> >
> > ret = ad4062_set_operation_mode(st, st->mode);
> > if (ret)
> > @@ -833,6 +1168,7 @@ static int ad4062_debugfs_reg_access(struct iio_dev *indio_dev, unsigned int reg
> >
> > if (!iio_device_claim_direct(indio_dev))
> > return -EBUSY;
> > + ad4062_exit_monitor_mode(st);
>
> This probably needs a comment. Not obvious to me how you end up in with it enabled
> again after the debugfs read / write finishes.
>
You don't. Once a register access is done, it will drop of monitor mode
or sample/buffer mode. But since it is a debug interface, that may be
alright?
> >
> > if (readval)
> > ret = regmap_read(st->regmap, reg, readval);
Best regards,
Jorge
Powered by blists - more mailing lists