Re: [PATCH v4 15/15] iio: adc: ad7091r: Allow users to configure device events

[David Lechner]

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
>