diff mbox series

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

Message ID 24a9f1bb721e66df65e36797b0c3fd2ca1f95227.1702746240.git.marcelo.schmitt1@gmail.com
State New
Headers show
Series Add support for AD7091R-2/-4/-8 | expand

Commit Message

Marcelo Schmitt Dec. 16, 2023, 5:51 p.m. UTC 
Implement event configuration callbacks allowing users to read/write
event thresholds and enable/disable event generation.

Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
---
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@mail.gmail.com/#t

 drivers/iio/adc/ad7091r-base.c | 117 +++++++++++++++++++++++++++++++--
 1 file changed, 113 insertions(+), 4 deletions(-)

Comments

Jonathan Cameron Dec. 17, 2023, 3:54 p.m. UTC | #1 
On Sat, 16 Dec 2023 14:51:50 -0300
Marcelo Schmitt <marcelo.schmitt@analog.com> wrote:

> Implement event configuration callbacks allowing users to read/write
> event thresholds and enable/disable event generation.
> 
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
> ---
> 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@mail.gmail.com/#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),

This is an ABI change.  So would need a really strong reason to make it...
mind you - it seems like this has been broken until now anyway so this change
may be fine.


>  	},
>  	{
>  		.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),
It's relatively unusual that you can't separate the two event directions with careful
control of the thresholds.  So I think you can implement the existing ABI by
just setting the thresholds to the either 0 or 2^12 - 1 as appropriate.
The docs seem to say it must exceed the value, or fall below the min so these values
should ensure it can't do either.

You can then enable the event generate if one of them is set.
David Lechner Dec. 17, 2023, 11:58 p.m. UTC | #2 
On Sat, Dec 16, 2023 at 11:52 AM Marcelo Schmitt
<marcelo.schmitt@analog.com> wrote:
>
> Implement event configuration callbacks allowing users to read/write
> event thresholds and enable/disable event generation.
>
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
> ---
> 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@mail.gmail.com/#t
>

I suppose it fits the definition of Suggested-by well enough and would
be appreciated. Even more so on [PATCH v4 02/15] "iio: adc: ad7091r:
Pass iio_dev to event handler".
David Lechner Dec. 18, 2023, 12:30 a.m. UTC | #3 
On Sat, Dec 16, 2023 at 11:52 AM Marcelo Schmitt
<marcelo.schmitt@analog.com> wrote:
>
> Implement event configuration callbacks allowing users to read/write
> event thresholds and enable/disable event generation.
>
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
> ---
> 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@mail.gmail.com/#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
>
diff mbox series

Patch

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));
+}
+
+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);
+		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)