Messages in this thread |  | | Date | Mon, 24 Nov 2025 12:33:12 +0200 | | From | Andy Shevchenko <> | | 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
|  |