| 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: <20251018172625.42f13f4a@jic23-huawei>
Date: Sat, 18 Oct 2025 17:26:25 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Jorge Marques <jorge.marques@...log.com>
Cc: 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 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>
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?
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.
> + 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", " ");
> +}
> 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.
> + }
> +
> 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;
> +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?
>
> 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.
>
> if (readval)
> ret = regmap_read(st->regmap, reg, readval);
Powered by blists - more mailing lists