| 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: <zzrtxpcxzqcjxhxmp5miov4f3kx5i3fpzmrt55azvktkgowejm@n6ofgzoaxoxb>
Date: Wed, 26 Nov 2025 16:00:36 +0100
From: Jorge Marques <gastmaier@...il.com>
To: Andy Shevchenko <andriy.shevchenko@...el.com>
Cc: Jorge Marques <jorge.marques@...log.com>,
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 12:33:12PM +0200, Andy Shevchenko wrote:
> 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.
>
> ...
>
Hi Andy,
> > +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;
>
Ack
> > +}
>
> ...
>
> > +static IIO_DEVICE_ATTR(sampling_frequency, 0644, ad4062_events_frequency_show,
> > + ad4062_events_frequency_store, 0);
>
> IIO_DEVICE_ATTR_RW()
>
Sure
> ...
>
> > {
> > 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;
> }
>
> > }
>
Sure.
> ...
>
> > +static int ad4062_monitor_mode_enable(struct ad4062_state *st, bool enable)
> > +{
> > + int ret = 0;
>
> Unneeded assignment.
Ack.
> > + if (!enable) {
> > + pm_runtime_put_autosuspend(&st->i3cdev->dev);
> > + return 0;
> > + }
>
> Just split to two functions and drop parameter 'enable',
>
Sure.
> > + 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.
>
Sure.
> > +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;
>
> > +}
Ack.
>
> ...
>
> > +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))
>
Not magic number, but max and min signed 12-bit, maybe
if (val != sign_extend32(val, 11))
return -EINVAL;
to not look like magic numbers, or
if (val < (-BIT(11)) || val > BIT(11) - 1)
return -EINVAL;
For Hysteresis I will change from
if (val >= BIT(7))
to
if (val & ~GENMASK(6,0))
I believe iio only passes positive to the hysteresis, but is a little clearer.
> > + 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
>
>
Best regards,
Jorge
Powered by blists - more mailing lists