Messages in this thread |  | | Date | Wed, 26 Nov 2025 16:00:36 +0100 | | From | Jorge Marques <> | | 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
|  |