| 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: <aSQ0aM2u49qzIZDm@smile.fi.intel.com>
Date: Mon, 24 Nov 2025 12:33:12 +0200
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: Jorge Marques <jorge.marques@...log.com>
Cc: Lars-Peter Clausen <lars@...afoo.de>,
Michael Hennerich <Michael.Hennerich@...log.com>,
Jonathan Cameron <jic23@...nel.org>,
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
Subject: Re: [PATCH v2 7/9] iio: adc: ad4062: Add IIO Events support
On Mon, Nov 24, 2025 at 10:18:06AM +0100, Jorge Marques 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.
...
> +static ssize_t ad4062_events_frequency_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct ad4062_state *st = iio_priv(indio_dev);
> + int val, ret;
> +
> + if (!iio_device_claim_direct(indio_dev))
> + return -EBUSY;
> + if (st->wait_event) {
> + ret = -EBUSY;
> + goto out_release;
> + }
> +
> + ret = kstrtoint(buf, 10, &val);
> + if (ret < 0)
> + goto out_release;
> +
> + st->events_frequency = find_closest_descending(val, ad4062_conversion_freqs,
> + ARRAY_SIZE(ad4062_conversion_freqs));
> + ret = 0;
> +
> +out_release:
> + iio_device_release_direct(indio_dev);
> + return ret ? ret : len;
return ret ?: len;
> +}
...
> +static IIO_DEVICE_ATTR(sampling_frequency, 0644, ad4062_events_frequency_show,
> + ad4062_events_frequency_store, 0);
IIO_DEVICE_ATTR_RW()
...
> {
> struct ad4062_state *st = i3cdev_get_drvdata(i3cdev);
>
> - if (iio_buffer_enabled(st->indio_dev))
> - iio_trigger_poll_nested(st->trigger);
> - else
> - complete(&st->completion);
> + if (st->wait_event) {
> + iio_push_event(st->indio_dev,
> + IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE, 0,
> + IIO_EV_TYPE_THRESH,
> + IIO_EV_DIR_EITHER),
> + iio_get_time_ns(st->indio_dev));
> + } else {
> + if (iio_buffer_enabled(st->indio_dev))
> + iio_trigger_poll_nested(st->trigger);
> + else
> + complete(&st->completion);
> + }
Less ping-pong:ish if you simply add a new code.
if (st->wait_event) {
iio_push_event(st->indio_dev,
IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE, 0,
IIO_EV_TYPE_THRESH,
IIO_EV_DIR_EITHER),
iio_get_time_ns(st->indio_dev));
return;
}
> }
...
> +static int ad4062_monitor_mode_enable(struct ad4062_state *st, bool enable)
> +{
> + int ret = 0;
Unneeded assignment.
> + if (!enable) {
> + pm_runtime_put_autosuspend(&st->i3cdev->dev);
> + return 0;
> + }
Just split to two functions and drop parameter 'enable',
> + ACQUIRE(pm_runtime_active_try_enabled, pm)(&st->i3cdev->dev);
> + ret = ACQUIRE_ERR(pm_runtime_active_try_enabled, &pm);
> + if (ret)
> + return ret;
> +
> + ret = ad4062_conversion_frequency_set(st, st->events_frequency);
> + if (ret)
> + return ret;
> +
> + ret = ad4062_set_operation_mode(st, AD4062_MONITOR_MODE);
> + if (ret)
> + return ret;
> +
> + pm_runtime_get_noresume(&st->i3cdev->dev);
> + return 0;
> +}
...
> +static int ad4062_write_event_config(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir,
> + bool state)
> +{
> + struct ad4062_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + if (!iio_device_claim_direct(indio_dev))
> + return -EBUSY;
> + if (st->wait_event == state) {
> + ret = 0;
> + goto out_release;
> + }
> +
> + ret = ad4062_monitor_mode_enable(st, state);
> + if (!ret)
> + st->wait_event = state;
Please use regular patter to check for errors first.
if (st->wait_event == state)
ret = 0;
else
ret = ad4062_monitor_mode_enable(st, state);
if (ret)
goto out_release;
st->wait_event = state;
Always think about readability first and then about size of the source code.
> +out_release:
> + iio_device_release_direct(indio_dev);
> + return ret;
> +}
...
> +static int ad4062_read_event_value(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir,
> + enum iio_event_info info, int *val,
> + int *val2)
> +{
> + struct ad4062_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + if (!iio_device_claim_direct(indio_dev))
> + return -EBUSY;
> + if (st->wait_event) {
> + ret = -EBUSY;
> + goto out_release;
> + }
> +
> + switch (info) {
> + case IIO_EV_INFO_VALUE:
> + ret = __ad4062_read_event_info_value(st, dir, val);
> + break;
> + case IIO_EV_INFO_HYSTERESIS:
> + ret = __ad4062_read_event_info_hysteresis(st, dir, val);
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> + }
> +
> +out_release:
> + iio_device_release_direct(indio_dev);
> + return ret ? ret : IIO_VAL_INT;
return ret ?: IIO_VAL_INT;
> +}
...
> +static int __ad4062_write_event_info_value(struct ad4062_state *st,
> + enum iio_event_direction dir, int val)
> +{
> + u8 reg;
> +
> + if (val > 2047 || val < -2048)
> + return -EINVAL;
There was already magic '11', perhaps define it and use there and here?
#define x11 11 // needs a good name
if (val > BIT(x11) || val < -BIT(x11))
> + if (dir == IIO_EV_DIR_RISING)
> + reg = AD4062_REG_MAX_LIMIT;
> + else
> + reg = AD4062_REG_MIN_LIMIT;
> + put_unaligned_be16(val, st->buf.bytes);
> +
> + return regmap_bulk_write(st->regmap, reg, &st->buf.be16,
> + sizeof(st->buf.be16));
> +}
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists