On Sat, Dec 16, 2023 at 11:52 AM Marcelo Schmitt <marcelo.schmitt@xxxxxxxxxx> wrote: > > Implement event configuration callbacks allowing users to read/write > event thresholds and enable/disable event generation. > > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@xxxxxxxxxx> > --- > This is from a review suggestion David made on v3 [1]. > > Is this the case for a Suggested-by tag? > > [1]: https://lore.kernel.org/linux-iio/CAMknhBFPbAqp4-AQdmbp+VRW-Ksk1PxaLCG+3n=Zk4gyStqhgw@xxxxxxxxxxxxxx/#t > > drivers/iio/adc/ad7091r-base.c | 117 +++++++++++++++++++++++++++++++-- > 1 file changed, 113 insertions(+), 4 deletions(-) > > diff --git a/drivers/iio/adc/ad7091r-base.c b/drivers/iio/adc/ad7091r-base.c > index 57355ca157a1..64e8baeff258 100644 > --- a/drivers/iio/adc/ad7091r-base.c > +++ b/drivers/iio/adc/ad7091r-base.c > @@ -20,19 +20,18 @@ const struct iio_event_spec ad7091r_events[] = { > { > .type = IIO_EV_TYPE_THRESH, > .dir = IIO_EV_DIR_RISING, > - .mask_separate = BIT(IIO_EV_INFO_VALUE) | > - BIT(IIO_EV_INFO_ENABLE), > + .mask_separate = BIT(IIO_EV_INFO_VALUE), > }, > { > .type = IIO_EV_TYPE_THRESH, > .dir = IIO_EV_DIR_FALLING, > - .mask_separate = BIT(IIO_EV_INFO_VALUE) | > - BIT(IIO_EV_INFO_ENABLE), > + .mask_separate = BIT(IIO_EV_INFO_VALUE), > }, > { > .type = IIO_EV_TYPE_THRESH, > .dir = IIO_EV_DIR_EITHER, > .mask_separate = BIT(IIO_EV_INFO_HYSTERESIS), > + .mask_shared_by_all = BIT(IIO_EV_INFO_ENABLE), > }, > }; > EXPORT_SYMBOL_NS_GPL(ad7091r_events, IIO_AD7091R); > @@ -128,8 +127,118 @@ static int ad7091r_read_raw(struct iio_dev *iio_dev, > return ret; > } > > +static int ad7091r_read_event_config(struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan, > + enum iio_event_type type, > + enum iio_event_direction dir) > +{ > + struct ad7091r_state *st = iio_priv(indio_dev); > + unsigned int alert; > + int ret; > + > + ret = regmap_read(st->map, AD7091R_REG_CONF, &alert); > + if (ret) > + return ret; > + > + return !!(FIELD_GET(AD7091R_REG_CONF_ALERT_EN, alert)); > +} According to the datasheet, bit 4 of the config register is for selecting the function of the ALERT/BUSY/GPO0 pin as either ALERT/BUSY or GPIO, so this sounds like a pinmux function rather than an event enable function. context: `#define AD7091R_REG_CONF_ALERT_EN BIT(4)` in a previous patch > + > +static int ad7091r_write_event_config(struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan, > + enum iio_event_type type, > + enum iio_event_direction dir, int state) > +{ > + struct ad7091r_state *st = iio_priv(indio_dev); > + > + /* > + * Whenever alerts are enabled, every ADC conversion will generate > + * an alert if the conversion result for a particular channel falls > + * bellow the respective low limit register or above the respective > + * high limit register. > + * We can enable or disable all alerts by writing to the config reg. > + */ > + return regmap_update_bits(st->map, AD7091R_REG_CONF, > + AD7091R_REG_CONF_ALERT_EN, state << 4); > +} > + > +static int ad7091r_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 ad7091r_state *st = iio_priv(indio_dev); > + int ret; > + > + switch (info) { > + case IIO_EV_INFO_VALUE: > + switch (dir) { > + case IIO_EV_DIR_RISING: > + ret = regmap_read(st->map, > + AD7091R_REG_CH_HIGH_LIMIT(chan->channel), > + val); > + if (ret) > + return ret; > + return IIO_VAL_INT; > + case IIO_EV_DIR_FALLING: > + ret = regmap_read(st->map, > + AD7091R_REG_CH_LOW_LIMIT(chan->channel), > + val); > + if (ret) > + return ret; > + return IIO_VAL_INT; > + default: > + return -EINVAL; > + } > + case IIO_EV_INFO_HYSTERESIS: > + ret = regmap_read(st->map, > + AD7091R_REG_CH_HYSTERESIS(chan->channel), > + val); > + if (ret) > + return ret; > + return IIO_VAL_INT; > + default: > + return -EINVAL; > + } > +} > + > +static int ad7091r_write_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 ad7091r_state *st = iio_priv(indio_dev); > + > + switch (info) { > + case IIO_EV_INFO_VALUE: > + switch (dir) { > + case IIO_EV_DIR_RISING: > + return regmap_write(st->map, > + AD7091R_REG_CH_HIGH_LIMIT(chan->channel), > + val); > + case IIO_EV_DIR_FALLING: > + return regmap_write(st->map, > + AD7091R_REG_CH_LOW_LIMIT(chan->channel), > + val); These registers are limited to 12-bit values. Do we need to check val first and return -EINVAL if out of range? > + default: > + return -EINVAL; > + } > + case IIO_EV_INFO_HYSTERESIS: > + return regmap_write(st->map, > + AD7091R_REG_CH_HYSTERESIS(chan->channel), > + val); > + default: > + return -EINVAL; > + } > +} > + > static const struct iio_info ad7091r_info = { > .read_raw = ad7091r_read_raw, > + .read_event_config = &ad7091r_read_event_config, > + .write_event_config = &ad7091r_write_event_config, > + .read_event_value = &ad7091r_read_event_value, > + .write_event_value = &ad7091r_write_event_value, > }; > > static irqreturn_t ad7091r_event_handler(int irq, void *private) > -- > 2.42.0 >
