staging:iio:cdc:ad7150: cleanup / fixup / graduate

From: Jonathan Cameron <[email protected]>

Note this doesn't support everything the chip can do as we ignore window mode for now (in window / out of window).

* Given the chip doesn't have any way of disabling the threshold pins, use disable_irq() etc to mask them except when we actually want them enabled (previously events were always enabled). Note there are race conditions, but using the current state from the status register and disabling interrupts across changes in type of event should mean those races result in interrupts, but no events to userspace.

* Correctly reflect that there is one threshold line per channel.

* Only take notice of rising edge. If anyone wants the other edge then they should set the other threshold (they are available for rising and falling directions). This isn't perfect but it makes it a lot simpler.

* If insufficient interrupts are specified in firnware, don't support events.

* Adaptive events use the same pos/neg values of thrMD as non adaptive ones.

Tested against qemu based emulation.

Signed-off-by Jonathan Cameron <[email protected]>

Link https://lore.kernel.org/r/[email protected] open here

---
 drivers/staging/iio/cdc/ad7150.c | 258 ++++++++++++++++++-------------
 1 file changed, 153 insertions(+), 105 deletions(-)

drivers/staging/iio/cdc/ad7150.c +47 -48

index f8183c540d5c..24be97456c03 100644
@@ -11,6 +11,7 @@
 #include <linux/kernel.h>
 #include <linux/slab.h>
 #include <linux/i2c.h>
+#include <linux/irq.h>
 #include <linux/module.h>
 
 #include <linux/iio/iio.h>

@@ -60,8 +61,6 @@ enum {
 /**
  * struct ad7150_chip_info - instance specific chip data
  * @client: i2c client for this device
- * @current_event: device always has one type of event enabled.
- *     This element stores the event code of the current one.
  * @threshold: thresholds for simple capacitance value events
  * @thresh_sensitivity: threshold for simple capacitance offset
  *     from 'average' value.

@@ -70,21 +69,23 @@ enum {
  *     value jumps to current value.  Note made up of two fields,
  *      3:0 are for timeout receding - applies if below lower threshold
  *      7:4 are for timeout approaching - applies if above upper threshold
- * @old_state: store state from previous event, allowing confirmation
- *     of new condition.
- * @conversion_mode: the current conversion mode.
  * @state_lock: ensure consistent state of this structure wrt the
  *     hardware.
+ * @interrupts: one or two interrupt numbers depending on device type.
+ * @int_enabled: is a given interrupt currently enabled.
+ * @type: threshold type
+ * @dir: threshold direction
  */
 struct ad7150_chip_info {
        struct i2c_client *client;
-       u64 current_event;
        u16 threshold[2][2];
        u8 thresh_sensitivity[2][2];
        u8 thresh_timeout[2][2];
-       int old_state;
-       char *conversion_mode;
        struct mutex state_lock;
+       int interrupts[2];
+       bool int_enabled[2];
+       enum iio_event_type type;
+       enum iio_event_direction dir;
 };
 
 /*

@@ -158,8 +159,8 @@ static int ad7150_read_event_config(struct iio_dev *indio_dev,
        switch (type) {
        case IIO_EV_TYPE_THRESH_ADAPTIVE:
                if (dir == IIO_EV_DIR_RISING)
-                       return !thrfixed && (threshtype == 0x3);
-               return !thrfixed && (threshtype == 0x2);
+                       return !thrfixed && (threshtype == 0x1);
+               return !thrfixed && (threshtype == 0x0);
        case IIO_EV_TYPE_THRESH:
                if (dir == IIO_EV_DIR_RISING)
                        return thrfixed && (threshtype == 0x1);

@@ -179,11 +180,9 @@ static int ad7150_write_event_params(struct iio_dev *indio_dev,
 {
        struct ad7150_chip_info *chip = iio_priv(indio_dev);
        int rising = (dir == IIO_EV_DIR_RISING);
-       u64 event_code;
 
-       event_code = IIO_UNMOD_EVENT_CODE(IIO_CAPACITANCE, chan, type, dir);
-
-       if (event_code != chip->current_event)
+       /* Only update value live, if parameter is in use */
+       if ((type != chip->type) || (dir != chip->dir))
                return 0;
 
        switch (type) {

@@ -231,52 +230,91 @@ static int ad7150_write_event_config(struct iio_dev *indio_dev,
        int ret;
        struct ad7150_chip_info *chip = iio_priv(indio_dev);
        int rising = (dir == IIO_EV_DIR_RISING);
-       u64 event_code;
-
-       /* Something must always be turned on */
-       if (!state)
-               return -EINVAL;
 
-       event_code = IIO_UNMOD_EVENT_CODE(chan->type, chan->channel, type, dir);
-       if (event_code == chip->current_event)
+       /*
+        * There is only a single shared control and no on chip
+        * interrupt disables for the two interrupt lines.
+        * So, enabling will switch the events configured to enable
+        * whatever was most recently requested and if necessary enable_irq()
+        * the interrupt and any disable will disable_irq() for that
+        * channels interrupt.
+        */
+       if (!state) {
+               if ((chip->int_enabled[chan->channel]) &&
+                   (type == chip->type) && (dir == chip->dir)) {
+                       disable_irq(chip->interrupts[chan->channel]);
+                       chip->int_enabled[chan->channel] = false;
+               }
                return 0;
+       }
+
        mutex_lock(&chip->state_lock);
-       ret = i2c_smbus_read_byte_data(chip->client, AD7150_CFG);
-       if (ret < 0)
-               goto error_ret;
+       if ((type != chip->type) || (dir != chip->dir)) {
 
-       cfg = ret & ~((0x03 << 5) | BIT(7));
+               /*
+                * Need to temporarily disable both interrupts if
+                * enabled - this is to avoid races around changing
+                * config and thresholds.
+                * Note enable/disable_irq() are reference counted so
+                * no need to check if already enabled.
+                */
+               disable_irq(chip->interrupts[0]);
+               disable_irq(chip->interrupts[1]);
 
-       switch (type) {
-       case IIO_EV_TYPE_THRESH_ADAPTIVE:
-               adaptive = 1;
-               if (rising)
-                       thresh_type = 0x3;
-               else
-                       thresh_type = 0x2;
-               break;
-       case IIO_EV_TYPE_THRESH:
-               adaptive = 0;
-               if (rising)
-                       thresh_type = 0x1;
-               else
-                       thresh_type = 0x0;
-               break;
-       default:
-               ret = -EINVAL;
-               goto error_ret;
-       }
+               ret = i2c_smbus_read_byte_data(chip->client, AD7150_CFG);
+               if (ret < 0)
+                       goto error_ret;
 
-       cfg |= (!adaptive << 7) | (thresh_type << 5);
+               cfg = ret & ~((0x03 << 5) | BIT(7));
 
-       ret = i2c_smbus_write_byte_data(chip->client, AD7150_CFG, cfg);
-       if (ret < 0)
-               goto error_ret;
+               switch (type) {
+               case IIO_EV_TYPE_THRESH_ADAPTIVE:
+                       adaptive = 1;
+                       if (rising)

drivers/staging/iio/cdc/ad7150.c +17 -1

+                       break;
+               default:
+                       ret = -EINVAL;
+                       goto error_ret;
+               }
 
-       chip->current_event = event_code;
+               cfg |= (!adaptive << 7) | (thresh_type << 5);
+
+               ret = i2c_smbus_write_byte_data(chip->client, AD7150_CFG, cfg);
+               if (ret < 0)
+                       goto error_ret;
+
+               /*
+                * There is a potential race condition here, but not easy
+                * to close given we can't disable the interrupt at the
+                * chip side of things. Rely on the status bit.
+                */
+               chip->type = type;

drivers/staging/iio/cdc/ad7150.c +32 -44

+       }
+       if (!chip->int_enabled[chan->channel]) {
+               enable_irq(chip->interrupts[chan->channel]);
+               chip->int_enabled[chan->channel] = true;
+       }
 
-       /* update control attributes */
-       ret = ad7150_write_event_params(indio_dev, chan->channel, type, dir);
 error_ret:
        mutex_unlock(&chip->state_lock);
 

@@ -434,59 +472,44 @@ static const struct iio_chan_spec ad7151_channels_no_irq[] = {
        AD7150_CAPACITANCE_CHAN_NO_IRQ(0),
 };
 
-static irqreturn_t ad7150_event_handler(int irq, void *private)
+static irqreturn_t __ad7150_event_handler(void *private, u8 status_mask,
+                                         int channel)
 {
        struct iio_dev *indio_dev = private;
        struct ad7150_chip_info *chip = iio_priv(indio_dev);
-       u8 int_status;
        s64 timestamp = iio_get_time_ns(indio_dev);
-       int ret;
+       int int_status;
 
-       ret = i2c_smbus_read_byte_data(chip->client, AD7150_STATUS);
-       if (ret < 0)
+       int_status = i2c_smbus_read_byte_data(chip->client, AD7150_STATUS);
+       if (int_status < 0)
                return IRQ_HANDLED;
 
-       int_status = ret;
-
-       if ((int_status & AD7150_STATUS_OUT1) &&
-           !(chip->old_state & AD7150_STATUS_OUT1))
-               iio_push_event(indio_dev,
-                              IIO_UNMOD_EVENT_CODE(IIO_CAPACITANCE,
-                                                   0,
-                                                   IIO_EV_TYPE_THRESH,
-                                                   IIO_EV_DIR_RISING),
-                               timestamp);
-       else if ((!(int_status & AD7150_STATUS_OUT1)) &&
-                (chip->old_state & AD7150_STATUS_OUT1))
-               iio_push_event(indio_dev,
-                              IIO_UNMOD_EVENT_CODE(IIO_CAPACITANCE,
-                                                   0,
-                                                   IIO_EV_TYPE_THRESH,
-                                                   IIO_EV_DIR_FALLING),
-                              timestamp);
-
-       if ((int_status & AD7150_STATUS_OUT2) &&
-           !(chip->old_state & AD7150_STATUS_OUT2))
-               iio_push_event(indio_dev,
-                              IIO_UNMOD_EVENT_CODE(IIO_CAPACITANCE,
-                                                   1,
-                                                   IIO_EV_TYPE_THRESH,
-                                                   IIO_EV_DIR_RISING),
-                              timestamp);
-       else if ((!(int_status & AD7150_STATUS_OUT2)) &&
-                (chip->old_state & AD7150_STATUS_OUT2))
-               iio_push_event(indio_dev,
-                              IIO_UNMOD_EVENT_CODE(IIO_CAPACITANCE,
-                                                   1,
-                                                   IIO_EV_TYPE_THRESH,
-                                                   IIO_EV_DIR_FALLING),
-                              timestamp);
-       /* store the status to avoid repushing same events */
-       chip->old_state = int_status;
+       /*
+        * There are race conditions around enabling and disabling that
+        * could easily land us here with a spurious interrupt.
+        * Just eat it if so.
+        */
+       if (!(int_status & status_mask))
+               return IRQ_HANDLED;
+
+       iio_push_event(indio_dev,
+                      IIO_UNMOD_EVENT_CODE(IIO_CAPACITANCE, channel,
+                                           chip->type, chip->dir),
+                      timestamp);
 
        return IRQ_HANDLED;
 }
 
+static irqreturn_t ad7150_event_handler_ch1(int irq, void *private)
+{
+       return __ad7150_event_handler(private, AD7150_STATUS_OUT1, 0);
+}
+
+static irqreturn_t ad7150_event_handler_ch2(int irq, void *private)
+{
+       return __ad7150_event_handler(private, AD7150_STATUS_OUT2, 1);
+}
+
 static IIO_CONST_ATTR(in_capacitance_thresh_adaptive_timeout_available,
                      "[0 0.01 0.15]");
 

drivers/staging/iio/cdc/ad7150.c +33 -12

+               chip->interrupts[1] = fwnode_irq_get(dev_fwnode(&client->dev), 1);
+               if (chip->interrupts[1] < 0)
+                       return chip->interrupts[1];
+       }
+       if (chip->interrupts[0] &&
+           (id->driver_data == AD7151 || chip->interrupts[1])) {
+               irq_set_status_flags(chip->interrupts[0], IRQ_NOAUTOEN);
+               ret = devm_request_threaded_irq(&client->dev,
+                                               chip->interrupts[0],
+                                               NULL,
+                                               &ad7150_event_handler_ch1,
+                                               IRQF_TRIGGER_RISING |
+                                               IRQF_ONESHOT,
+                                               "ad7150_irq1",
+                                               indio_dev);
+               if (ret)
+                       return ret;
+
                indio_dev->info = &ad7150_info;
                switch (id->driver_data) {
                case AD7150:
                        indio_dev->channels = ad7150_channels;
                        indio_dev->num_channels = ARRAY_SIZE(ad7150_channels);
+                       irq_set_status_flags(chip->interrupts[1], IRQ_NOAUTOEN);
+                       ret = devm_request_threaded_irq(&client->dev,
+                                                       chip->interrupts[1],
+                                                       NULL,
+                                                       &ad7150_event_handler_ch2,
+                                                       IRQF_TRIGGER_RISING |
+                                                       IRQF_ONESHOT,
+                                                       "ad7150_irq2",
+                                                       indio_dev);
+                       if (ret)
+                               return ret;
                        break;
                case AD7151:
                        indio_dev->channels = ad7151_channels;

@@ -548,25 +603,18 @@ static int ad7150_probe(struct i2c_client *client,
                        return -EINVAL;
                }
 
-               ret = devm_request_threaded_irq(&client->dev, client->irq,
-                                               NULL,
-                                               &ad7150_event_handler,
-                                               IRQF_TRIGGER_RISING |
-                                               IRQF_ONESHOT,
-                                               "ad7150_irq1",
-                                               indio_dev);
-               if (ret)
-                       return ret;
        } else {
                indio_dev->info = &ad7150_info_no_irq;
                switch (id->driver_data) {
                case AD7150:
                        indio_dev->channels = ad7150_channels_no_irq;
-                       indio_dev->num_channels = ARRAY_SIZE(ad7150_channels_no_irq);
+                       indio_dev->num_channels =
+                               ARRAY_SIZE(ad7150_channels_no_irq);
                        break;
                case AD7151:
                        indio_dev->channels = ad7151_channels_no_irq;
-                       indio_dev->num_channels = ARRAY_SIZE(ad7151_channels_no_irq);
+                       indio_dev->num_channels =
+                               ARRAY_SIZE(ad7151_channels_no_irq);
                        break;
                default:
                        return -EINVAL;
--

2.30.2

Quoted 27 lines · Jonathan Cameron

From: Jonathan Cameron <[email protected]>

Note this doesn't support everything the chip can do as we ignore window mode for now (in window / out of window).

* Given the chip doesn't have any way of disabling the threshold pins, use disable_irq() etc to mask them except when we actually want them enabled (previously events were always enabled). Note there are race conditions, but using the current state from the status register and disabling interrupts across changes in type of event should mean those races result in interrupts, but no events to userspace.

* Correctly reflect that there is one threshold line per channel.

* Only take notice of rising edge. If anyone wants the other edge then they should set the other threshold (they are available for rising and falling directions). This isn't perfect but it makes it a lot simpler.

* If insufficient interrupts are specified in firnware, don't support events.

* Adaptive events use the same pos/neg values of thrMD as non adaptive ones.

Quoted 2 lines

Tested against qemu based emulation.

Overall looks ok.
A few minor issues with the device-matching and OF. And maybe with the interrupt handling.

Quoted 169 lines

Signed-off-by Jonathan Cameron <[email protected]>

Link https://lore.kernel.org/r/[email protected] open here

---
 drivers/staging/iio/cdc/ad7150.c | 258 ++++++++++++++++++-------------
 1 file changed, 153 insertions(+), 105 deletions(-)

drivers/staging/iio/cdc/ad7150.c +53 -48

index f8183c540d5c..24be97456c03 100644
@@ -11,6 +11,7 @@
 #include <linux/kernel.h>
 #include <linux/slab.h>
 #include <linux/i2c.h>
+#include <linux/irq.h>
 #include <linux/module.h>
 
 #include <linux/iio/iio.h>

@@ -60,8 +61,6 @@ enum {
 /**
  * struct ad7150_chip_info - instance specific chip data
  * @client: i2c client for this device
- * @current_event: device always has one type of event enabled.
- *     This element stores the event code of the current one.
  * @threshold: thresholds for simple capacitance value events
  * @thresh_sensitivity: threshold for simple capacitance offset
  *     from 'average' value.

@@ -70,21 +69,23 @@ enum {
  *     value jumps to current value.  Note made up of two fields,
  *      3:0 are for timeout receding - applies if below lower threshold
  *      7:4 are for timeout approaching - applies if above upper threshold
- * @old_state: store state from previous event, allowing confirmation
- *     of new condition.
- * @conversion_mode: the current conversion mode.
  * @state_lock: ensure consistent state of this structure wrt the
  *     hardware.
+ * @interrupts: one or two interrupt numbers depending on device type.
+ * @int_enabled: is a given interrupt currently enabled.
+ * @type: threshold type
+ * @dir: threshold direction
  */
 struct ad7150_chip_info {
        struct i2c_client *client;
-       u64 current_event;
        u16 threshold[2][2];
        u8 thresh_sensitivity[2][2];
        u8 thresh_timeout[2][2];
-       int old_state;
-       char *conversion_mode;
        struct mutex state_lock;
+       int interrupts[2];
+       bool int_enabled[2];
+       enum iio_event_type type;
+       enum iio_event_direction dir;
 };
 
 /*

@@ -158,8 +159,8 @@ static int ad7150_read_event_config(struct iio_dev *indio_dev,
        switch (type) {
        case IIO_EV_TYPE_THRESH_ADAPTIVE:
                if (dir == IIO_EV_DIR_RISING)
-                       return !thrfixed && (threshtype == 0x3);
-               return !thrfixed && (threshtype == 0x2);
+                       return !thrfixed && (threshtype == 0x1);
+               return !thrfixed && (threshtype == 0x0);
        case IIO_EV_TYPE_THRESH:
                if (dir == IIO_EV_DIR_RISING)
                        return thrfixed && (threshtype == 0x1);

@@ -179,11 +180,9 @@ static int ad7150_write_event_params(struct iio_dev *indio_dev,
 {
        struct ad7150_chip_info *chip = iio_priv(indio_dev);
        int rising = (dir == IIO_EV_DIR_RISING);
-       u64 event_code;
 
-       event_code = IIO_UNMOD_EVENT_CODE(IIO_CAPACITANCE, chan, type, dir);
-
-       if (event_code != chip->current_event)
+       /* Only update value live, if parameter is in use */
+       if ((type != chip->type) || (dir != chip->dir))
                return 0;
 
        switch (type) {

@@ -231,52 +230,91 @@ static int ad7150_write_event_config(struct iio_dev *indio_dev,
        int ret;
        struct ad7150_chip_info *chip = iio_priv(indio_dev);
        int rising = (dir == IIO_EV_DIR_RISING);
-       u64 event_code;
-
-       /* Something must always be turned on */
-       if (!state)
-               return -EINVAL;
 
-       event_code = IIO_UNMOD_EVENT_CODE(chan->type, chan->channel, type, dir);
-       if (event_code == chip->current_event)
+       /*
+        * There is only a single shared control and no on chip
+        * interrupt disables for the two interrupt lines.
+        * So, enabling will switch the events configured to enable
+        * whatever was most recently requested and if necessary enable_irq()
+        * the interrupt and any disable will disable_irq() for that
+        * channels interrupt.
+        */
+       if (!state) {
+               if ((chip->int_enabled[chan->channel]) &&
+                   (type == chip->type) && (dir == chip->dir)) {
+                       disable_irq(chip->interrupts[chan->channel]);
+                       chip->int_enabled[chan->channel] = false;
+               }
                return 0;
+       }
+
        mutex_lock(&chip->state_lock);
-       ret = i2c_smbus_read_byte_data(chip->client, AD7150_CFG);
-       if (ret < 0)
-               goto error_ret;
+       if ((type != chip->type) || (dir != chip->dir)) {
 
-       cfg = ret & ~((0x03 << 5) | BIT(7));
+               /*
+                * Need to temporarily disable both interrupts if
+                * enabled - this is to avoid races around changing
+                * config and thresholds.
+                * Note enable/disable_irq() are reference counted so
+                * no need to check if already enabled.
+                */
+               disable_irq(chip->interrupts[0]);
+               disable_irq(chip->interrupts[1]);
 
-       switch (type) {
-       case IIO_EV_TYPE_THRESH_ADAPTIVE:
-               adaptive = 1;
-               if (rising)
-                       thresh_type = 0x3;
-               else
-                       thresh_type = 0x2;
-               break;
-       case IIO_EV_TYPE_THRESH:
-               adaptive = 0;
-               if (rising)
-                       thresh_type = 0x1;
-               else
-                       thresh_type = 0x0;
-               break;
-       default:
-               ret = -EINVAL;
-               goto error_ret;
-       }
+               ret = i2c_smbus_read_byte_data(chip->client, AD7150_CFG);
+               if (ret < 0)
+                       goto error_ret;
 
-       cfg |= (!adaptive << 7) | (thresh_type << 5);
+               cfg = ret & ~((0x03 << 5) | BIT(7));
 
-       ret = i2c_smbus_write_byte_data(chip->client, AD7150_CFG, cfg);
-       if (ret < 0)
-               goto error_ret;
+               switch (type) {
+               case IIO_EV_TYPE_THRESH_ADAPTIVE:
+                       adaptive = 1;
+                       if (rising)
+                               thresh_type = 0x1;
+                       else
+                               thresh_type = 0x0;
+                       break;
+               case IIO_EV_TYPE_THRESH:
+                       adaptive = 0;

drivers/staging/iio/cdc/ad7150.c static int ad7150_write_event_config(struct iio_dev *indio_dev, · line 276

Quoted 4 lines

drivers/staging/iio/cdc/ad7150.c +4 -0

+                       if (rising)
+                               thresh_type = 0x1;
+                       else
+                               thresh_type = 0x0;  

this can be ignored; it's a minor nit;
the if (rising) {} else {} block looks duplicate;
it could be moved after the switch() block;

Quoted 20 lines

drivers/staging/iio/cdc/ad7150.c +17 -1

+                       break;
+               default:
+                       ret = -EINVAL;
+                       goto error_ret;
+               }
 
-       chip->current_event = event_code;
+               cfg |= (!adaptive << 7) | (thresh_type << 5);
+
+               ret = i2c_smbus_write_byte_data(chip->client, AD7150_CFG, cfg);
+               if (ret < 0)
+                       goto error_ret;
+
+               /*
+                * There is a potential race condition here, but not easy
+                * to close given we can't disable the interrupt at the
+                * chip side of things. Rely on the status bit.
+                */
+               chip->type = type;

drivers/staging/iio/cdc/ad7150.c static int ad7150_write_event_config(struct iio_dev *indio_dev, · line 311

Quoted 10 lines

drivers/staging/iio/cdc/ad7150.c +10 -0

+               chip->dir = dir;
+
+               /* update control attributes */
+               ret = ad7150_write_event_params(indio_dev, chan->channel, type,
+                                               dir);
+               if (ret)
+                       goto error_ret;
+               /* reenable any irq's we disabled whilst changing mode */
+               enable_irq(chip->interrupts[0]);
+               enable_irq(chip->interrupts[1]);  

i'm wondering if we need to set 'chip->int_enabled[ for 0 & 1 ] = true;'
tbh i am not sure if there is a chance of mismatch between 'int_enabled' and the actual enable_irq()

maybe it might make sense to add an ad7150_{enable/disable}_irq() that checks the per-channel 'int_enabled' field;

Quoted 104 lines

drivers/staging/iio/cdc/ad7150.c +32 -44

+       }
+       if (!chip->int_enabled[chan->channel]) {
+               enable_irq(chip->interrupts[chan->channel]);
+               chip->int_enabled[chan->channel] = true;
+       }
 
-       /* update control attributes */
-       ret = ad7150_write_event_params(indio_dev, chan->channel, type, dir);
 error_ret:
        mutex_unlock(&chip->state_lock);
 

@@ -434,59 +472,44 @@ static const struct iio_chan_spec ad7151_channels_no_irq[] = {
        AD7150_CAPACITANCE_CHAN_NO_IRQ(0),
 };
 
-static irqreturn_t ad7150_event_handler(int irq, void *private)
+static irqreturn_t __ad7150_event_handler(void *private, u8 status_mask,
+                                         int channel)
 {
        struct iio_dev *indio_dev = private;
        struct ad7150_chip_info *chip = iio_priv(indio_dev);
-       u8 int_status;
        s64 timestamp = iio_get_time_ns(indio_dev);
-       int ret;
+       int int_status;
 
-       ret = i2c_smbus_read_byte_data(chip->client, AD7150_STATUS);
-       if (ret < 0)
+       int_status = i2c_smbus_read_byte_data(chip->client, AD7150_STATUS);
+       if (int_status < 0)
                return IRQ_HANDLED;
 
-       int_status = ret;
-
-       if ((int_status & AD7150_STATUS_OUT1) &&
-           !(chip->old_state & AD7150_STATUS_OUT1))
-               iio_push_event(indio_dev,
-                              IIO_UNMOD_EVENT_CODE(IIO_CAPACITANCE,
-                                                   0,
-                                                   IIO_EV_TYPE_THRESH,
-                                                   IIO_EV_DIR_RISING),
-                               timestamp);
-       else if ((!(int_status & AD7150_STATUS_OUT1)) &&
-                (chip->old_state & AD7150_STATUS_OUT1))
-               iio_push_event(indio_dev,
-                              IIO_UNMOD_EVENT_CODE(IIO_CAPACITANCE,
-                                                   0,
-                                                   IIO_EV_TYPE_THRESH,
-                                                   IIO_EV_DIR_FALLING),
-                              timestamp);
-
-       if ((int_status & AD7150_STATUS_OUT2) &&
-           !(chip->old_state & AD7150_STATUS_OUT2))
-               iio_push_event(indio_dev,
-                              IIO_UNMOD_EVENT_CODE(IIO_CAPACITANCE,
-                                                   1,
-                                                   IIO_EV_TYPE_THRESH,
-                                                   IIO_EV_DIR_RISING),
-                              timestamp);
-       else if ((!(int_status & AD7150_STATUS_OUT2)) &&
-                (chip->old_state & AD7150_STATUS_OUT2))
-               iio_push_event(indio_dev,
-                              IIO_UNMOD_EVENT_CODE(IIO_CAPACITANCE,
-                                                   1,
-                                                   IIO_EV_TYPE_THRESH,
-                                                   IIO_EV_DIR_FALLING),
-                              timestamp);
-       /* store the status to avoid repushing same events */
-       chip->old_state = int_status;
+       /*
+        * There are race conditions around enabling and disabling that
+        * could easily land us here with a spurious interrupt.
+        * Just eat it if so.
+        */
+       if (!(int_status & status_mask))
+               return IRQ_HANDLED;
+
+       iio_push_event(indio_dev,
+                      IIO_UNMOD_EVENT_CODE(IIO_CAPACITANCE, channel,
+                                           chip->type, chip->dir),
+                      timestamp);
 
        return IRQ_HANDLED;
 }
 
+static irqreturn_t ad7150_event_handler_ch1(int irq, void *private)
+{
+       return __ad7150_event_handler(private, AD7150_STATUS_OUT1, 0);
+}
+
+static irqreturn_t ad7150_event_handler_ch2(int irq, void *private)
+{
+       return __ad7150_event_handler(private, AD7150_STATUS_OUT2, 1);
+}
+
 static IIO_CONST_ATTR(in_capacitance_thresh_adaptive_timeout_available,
                      "[0 0.01 0.15]");
 

@@ -533,12 +556,44 @@ static int ad7150_probe(struct i2c_client *client,
 
        indio_dev->modes = INDIO_DIRECT_MODE;
 

drivers/staging/iio/cdc/ad7150.c static int ad7150_probe(struct i2c_client *client, · line 562

Quoted 5 lines

drivers/staging/iio/cdc/ad7150.c +4 -1

-       if (client->irq) {
+       chip->interrupts[0] = fwnode_irq_get(dev_fwnode(&client->dev), 0);
+       if (chip->interrupts[0] < 0)
+               return chip->interrupts[0];
+       if (id->driver_data == AD7150) {  

checking 'id->driver_data' will be flaky after the of_match_table is added;

either we defer the of_match_table patch, or we update it somehow; maybe a chip_info struct is an idea;
otherwise we need to do void pointer to int conversions [which look nasty];

i'm also seeing that there is a 'type' field inside of_device_id that is underused, but looks useful for cases like this;
i'm also a bit paranoid to use it; because it may not be compatible with acpi_device_id

Quoted 71 lines

drivers/staging/iio/cdc/ad7150.c +33 -12

+               chip->interrupts[1] = fwnode_irq_get(dev_fwnode(&client->dev), 1);
+               if (chip->interrupts[1] < 0)
+                       return chip->interrupts[1];
+       }
+       if (chip->interrupts[0] &&
+           (id->driver_data == AD7151 || chip->interrupts[1])) {
+               irq_set_status_flags(chip->interrupts[0], IRQ_NOAUTOEN);
+               ret = devm_request_threaded_irq(&client->dev,
+                                               chip->interrupts[0],
+                                               NULL,
+                                               &ad7150_event_handler_ch1,
+                                               IRQF_TRIGGER_RISING |
+                                               IRQF_ONESHOT,
+                                               "ad7150_irq1",
+                                               indio_dev);
+               if (ret)
+                       return ret;
+
                indio_dev->info = &ad7150_info;
                switch (id->driver_data) {
                case AD7150:
                        indio_dev->channels = ad7150_channels;
                        indio_dev->num_channels = ARRAY_SIZE(ad7150_channels);
+                       irq_set_status_flags(chip->interrupts[1], IRQ_NOAUTOEN);
+                       ret = devm_request_threaded_irq(&client->dev,
+                                                       chip->interrupts[1],
+                                                       NULL,
+                                                       &ad7150_event_handler_ch2,
+                                                       IRQF_TRIGGER_RISING |
+                                                       IRQF_ONESHOT,
+                                                       "ad7150_irq2",
+                                                       indio_dev);
+                       if (ret)
+                               return ret;
                        break;
                case AD7151:
                        indio_dev->channels = ad7151_channels;

@@ -548,25 +603,18 @@ static int ad7150_probe(struct i2c_client *client,
                        return -EINVAL;
                }
 
-               ret = devm_request_threaded_irq(&client->dev, client->irq,
-                                               NULL,
-                                               &ad7150_event_handler,
-                                               IRQF_TRIGGER_RISING |
-                                               IRQF_ONESHOT,
-                                               "ad7150_irq1",
-                                               indio_dev);
-               if (ret)
-                       return ret;
        } else {
                indio_dev->info = &ad7150_info_no_irq;
                switch (id->driver_data) {
                case AD7150:
                        indio_dev->channels = ad7150_channels_no_irq;
-                       indio_dev->num_channels = ARRAY_SIZE(ad7150_channels_no_irq);
+                       indio_dev->num_channels =
+                               ARRAY_SIZE(ad7150_channels_no_irq);
                        break;
                case AD7151:
                        indio_dev->channels = ad7151_channels_no_irq;
-                       indio_dev->num_channels = ARRAY_SIZE(ad7151_channels_no_irq);
+                       indio_dev->num_channels =
+                               ARRAY_SIZE(ad7151_channels_no_irq);
                        break;
                default:
                        return -EINVAL;
--

2.30.2

Quoted 28 lines · On Wed, 31 Mar 2021 10:29:51 +0300 Alexandru Ardelean <[email protected]>…

Quoted 27 lines · Jonathan Cameron

From: Jonathan Cameron <[email protected]>

Note this doesn't support everything the chip can do as we ignore window mode for now (in window / out of window).

* Given the chip doesn't have any way of disabling the threshold pins, use disable_irq() etc to mask them except when we actually want them enabled (previously events were always enabled). Note there are race conditions, but using the current state from the status register and disabling interrupts across changes in type of event should mean those races result in interrupts, but no events to userspace.

* Correctly reflect that there is one threshold line per channel.

* Only take notice of rising edge. If anyone wants the other edge then they should set the other threshold (they are available for rising and falling directions). This isn't perfect but it makes it a lot simpler.

* If insufficient interrupts are specified in firnware, don't support events.

* Adaptive events use the same pos/neg values of thrMD as non adaptive ones.

Quoted 6 lines

Quoted 2 lines

Tested against qemu based emulation.

Overall looks ok.
A few minor issues with the device-matching and OF. And maybe with the interrupt handling.

Hopefully answers below address those two.

Quoted 171 lines

Quoted 171 lines

Signed-off-by Jonathan Cameron <[email protected]>

Link https://lore.kernel.org/r/[email protected] open here

---
 drivers/staging/iio/cdc/ad7150.c | 258 ++++++++++++++++++-------------
 1 file changed, 153 insertions(+), 105 deletions(-)

drivers/staging/iio/cdc/ad7150.c +53 -48

index f8183c540d5c..24be97456c03 100644
@@ -11,6 +11,7 @@
 #include <linux/kernel.h>
 #include <linux/slab.h>
 #include <linux/i2c.h>
+#include <linux/irq.h>
 #include <linux/module.h>
 
 #include <linux/iio/iio.h>

@@ -60,8 +61,6 @@ enum {
 /**
  * struct ad7150_chip_info - instance specific chip data
  * @client: i2c client for this device
- * @current_event: device always has one type of event enabled.
- *     This element stores the event code of the current one.
  * @threshold: thresholds for simple capacitance value events
  * @thresh_sensitivity: threshold for simple capacitance offset
  *     from 'average' value.

@@ -70,21 +69,23 @@ enum {
  *     value jumps to current value.  Note made up of two fields,
  *      3:0 are for timeout receding - applies if below lower threshold
  *      7:4 are for timeout approaching - applies if above upper threshold
- * @old_state: store state from previous event, allowing confirmation
- *     of new condition.
- * @conversion_mode: the current conversion mode.
  * @state_lock: ensure consistent state of this structure wrt the
  *     hardware.
+ * @interrupts: one or two interrupt numbers depending on device type.
+ * @int_enabled: is a given interrupt currently enabled.
+ * @type: threshold type
+ * @dir: threshold direction
  */
 struct ad7150_chip_info {
        struct i2c_client *client;
-       u64 current_event;
        u16 threshold[2][2];
        u8 thresh_sensitivity[2][2];
        u8 thresh_timeout[2][2];
-       int old_state;
-       char *conversion_mode;
        struct mutex state_lock;
+       int interrupts[2];
+       bool int_enabled[2];
+       enum iio_event_type type;
+       enum iio_event_direction dir;
 };
 
 /*

@@ -158,8 +159,8 @@ static int ad7150_read_event_config(struct iio_dev *indio_dev,
        switch (type) {
        case IIO_EV_TYPE_THRESH_ADAPTIVE:
                if (dir == IIO_EV_DIR_RISING)
-                       return !thrfixed && (threshtype == 0x3);
-               return !thrfixed && (threshtype == 0x2);
+                       return !thrfixed && (threshtype == 0x1);
+               return !thrfixed && (threshtype == 0x0);
        case IIO_EV_TYPE_THRESH:
                if (dir == IIO_EV_DIR_RISING)
                        return thrfixed && (threshtype == 0x1);

@@ -179,11 +180,9 @@ static int ad7150_write_event_params(struct iio_dev *indio_dev,
 {
        struct ad7150_chip_info *chip = iio_priv(indio_dev);
        int rising = (dir == IIO_EV_DIR_RISING);
-       u64 event_code;
 
-       event_code = IIO_UNMOD_EVENT_CODE(IIO_CAPACITANCE, chan, type, dir);
-
-       if (event_code != chip->current_event)
+       /* Only update value live, if parameter is in use */
+       if ((type != chip->type) || (dir != chip->dir))
                return 0;
 
        switch (type) {

@@ -231,52 +230,91 @@ static int ad7150_write_event_config(struct iio_dev *indio_dev,
        int ret;
        struct ad7150_chip_info *chip = iio_priv(indio_dev);
        int rising = (dir == IIO_EV_DIR_RISING);
-       u64 event_code;
-
-       /* Something must always be turned on */
-       if (!state)
-               return -EINVAL;
 
-       event_code = IIO_UNMOD_EVENT_CODE(chan->type, chan->channel, type, dir);
-       if (event_code == chip->current_event)
+       /*
+        * There is only a single shared control and no on chip
+        * interrupt disables for the two interrupt lines.
+        * So, enabling will switch the events configured to enable
+        * whatever was most recently requested and if necessary enable_irq()
+        * the interrupt and any disable will disable_irq() for that
+        * channels interrupt.
+        */
+       if (!state) {
+               if ((chip->int_enabled[chan->channel]) &&
+                   (type == chip->type) && (dir == chip->dir)) {
+                       disable_irq(chip->interrupts[chan->channel]);
+                       chip->int_enabled[chan->channel] = false;
+               }
                return 0;
+       }
+
        mutex_lock(&chip->state_lock);
-       ret = i2c_smbus_read_byte_data(chip->client, AD7150_CFG);
-       if (ret < 0)
-               goto error_ret;
+       if ((type != chip->type) || (dir != chip->dir)) {
 
-       cfg = ret & ~((0x03 << 5) | BIT(7));
+               /*
+                * Need to temporarily disable both interrupts if
+                * enabled - this is to avoid races around changing
+                * config and thresholds.
+                * Note enable/disable_irq() are reference counted so
+                * no need to check if already enabled.
+                */
+               disable_irq(chip->interrupts[0]);
+               disable_irq(chip->interrupts[1]);
 
-       switch (type) {
-       case IIO_EV_TYPE_THRESH_ADAPTIVE:
-               adaptive = 1;
-               if (rising)
-                       thresh_type = 0x3;
-               else
-                       thresh_type = 0x2;
-               break;
-       case IIO_EV_TYPE_THRESH:
-               adaptive = 0;
-               if (rising)
-                       thresh_type = 0x1;
-               else
-                       thresh_type = 0x0;
-               break;
-       default:
-               ret = -EINVAL;
-               goto error_ret;
-       }
+               ret = i2c_smbus_read_byte_data(chip->client, AD7150_CFG);
+               if (ret < 0)
+                       goto error_ret;
 
-       cfg |= (!adaptive << 7) | (thresh_type << 5);
+               cfg = ret & ~((0x03 << 5) | BIT(7));
 
-       ret = i2c_smbus_write_byte_data(chip->client, AD7150_CFG, cfg);
-       if (ret < 0)
-               goto error_ret;
+               switch (type) {
+               case IIO_EV_TYPE_THRESH_ADAPTIVE:
+                       adaptive = 1;
+                       if (rising)
+                               thresh_type = 0x1;
+                       else
+                               thresh_type = 0x0;
+                       break;
+               case IIO_EV_TYPE_THRESH:
+                       adaptive = 0;

Quoted 8 lines

drivers/staging/iio/cdc/ad7150.c static int ad7150_write_event_config(struct iio_dev *indio_dev, · line 276

Quoted 4 lines

drivers/staging/iio/cdc/ad7150.c +4 -0

+                       if (rising)
+                               thresh_type = 0x1;
+                       else
+                               thresh_type = 0x0;

this can be ignored; it's a minor nit;
the if (rising) {} else {} block looks duplicate;
it could be moved after the switch() block;

If you are happy with rest I'll just roll that in whilst applying.

Quoted 35 lines

Quoted 20 lines

drivers/staging/iio/cdc/ad7150.c +17 -1

+                       break;
+               default:
+                       ret = -EINVAL;
+                       goto error_ret;
+               }
 
-       chip->current_event = event_code;
+               cfg |= (!adaptive << 7) | (thresh_type << 5);
+
+               ret = i2c_smbus_write_byte_data(chip->client, AD7150_CFG, cfg);
+               if (ret < 0)
+                       goto error_ret;
+
+               /*
+                * There is a potential race condition here, but not easy
+                * to close given we can't disable the interrupt at the
+                * chip side of things. Rely on the status bit.
+                */
+               chip->type = type;

drivers/staging/iio/cdc/ad7150.c static int ad7150_write_event_config(struct iio_dev *indio_dev, · line 311

Quoted 10 lines

drivers/staging/iio/cdc/ad7150.c +10 -0

+               chip->dir = dir;
+
+               /* update control attributes */
+               ret = ad7150_write_event_params(indio_dev, chan->channel, type,
+                                               dir);
+               if (ret)
+                       goto error_ret;
+               /* reenable any irq's we disabled whilst changing mode */
+               enable_irq(chip->interrupts[0]);
+               enable_irq(chip->interrupts[1]);

i'm wondering if we need to set 'chip->int_enabled[ for 0 & 1 ] = true;'
tbh i am not sure if there is a chance of mismatch between 'int_enabled' and the actual enable_irq()

Quoted 2 lines

maybe it might make sense to add an ad7150_{enable/disable}_irq() that checks the per-channel 'int_enabled' field;

Ah. I'm playing a bit of a game here. (there is a comment at the irq_disable() calls that was meant to indicate what was going on).

There is a difference between when chip->int_enabled[] is changed in conjunction with enable_irq()/disable_irq() and when enable_irq()/disable_irq() are change on their own.

The intent is that int_enabled[] reflects non-transient state of whether the interrupt is enabled - I don't want to update it during transient states.

We are fine to do the disable_irq() calls on irqs that aren't enabled because disable_irq() calls can be safely nested. Only when you have more enable_irq() calls (including startup if it's auto enabled) than you have disable_irq() calls will the interrupt be enabled.

https://elixir.bootlin.com/linux/latest/source/kernel/irq/manage.c#L771
https://elixir.bootlin.com/linux/latest/source/include/linux/irqdesc.h#L28
The depth field in irq_desc is used for this.

Quoted 104 lines

Quoted 104 lines

drivers/staging/iio/cdc/ad7150.c +32 -44

+       }
+       if (!chip->int_enabled[chan->channel]) {
+               enable_irq(chip->interrupts[chan->channel]);
+               chip->int_enabled[chan->channel] = true;
+       }
 
-       /* update control attributes */
-       ret = ad7150_write_event_params(indio_dev, chan->channel, type, dir);
 error_ret:
        mutex_unlock(&chip->state_lock);
 

@@ -434,59 +472,44 @@ static const struct iio_chan_spec ad7151_channels_no_irq[] = {
        AD7150_CAPACITANCE_CHAN_NO_IRQ(0),
 };
 
-static irqreturn_t ad7150_event_handler(int irq, void *private)
+static irqreturn_t __ad7150_event_handler(void *private, u8 status_mask,
+                                         int channel)
 {
        struct iio_dev *indio_dev = private;
        struct ad7150_chip_info *chip = iio_priv(indio_dev);
-       u8 int_status;
        s64 timestamp = iio_get_time_ns(indio_dev);
-       int ret;
+       int int_status;
 
-       ret = i2c_smbus_read_byte_data(chip->client, AD7150_STATUS);
-       if (ret < 0)
+       int_status = i2c_smbus_read_byte_data(chip->client, AD7150_STATUS);
+       if (int_status < 0)
                return IRQ_HANDLED;
 
-       int_status = ret;
-
-       if ((int_status & AD7150_STATUS_OUT1) &&
-           !(chip->old_state & AD7150_STATUS_OUT1))
-               iio_push_event(indio_dev,
-                              IIO_UNMOD_EVENT_CODE(IIO_CAPACITANCE,
-                                                   0,
-                                                   IIO_EV_TYPE_THRESH,
-                                                   IIO_EV_DIR_RISING),
-                               timestamp);
-       else if ((!(int_status & AD7150_STATUS_OUT1)) &&
-                (chip->old_state & AD7150_STATUS_OUT1))
-               iio_push_event(indio_dev,
-                              IIO_UNMOD_EVENT_CODE(IIO_CAPACITANCE,
-                                                   0,
-                                                   IIO_EV_TYPE_THRESH,
-                                                   IIO_EV_DIR_FALLING),
-                              timestamp);
-
-       if ((int_status & AD7150_STATUS_OUT2) &&
-           !(chip->old_state & AD7150_STATUS_OUT2))
-               iio_push_event(indio_dev,
-                              IIO_UNMOD_EVENT_CODE(IIO_CAPACITANCE,
-                                                   1,
-                                                   IIO_EV_TYPE_THRESH,
-                                                   IIO_EV_DIR_RISING),
-                              timestamp);
-       else if ((!(int_status & AD7150_STATUS_OUT2)) &&
-                (chip->old_state & AD7150_STATUS_OUT2))
-               iio_push_event(indio_dev,
-                              IIO_UNMOD_EVENT_CODE(IIO_CAPACITANCE,
-                                                   1,
-                                                   IIO_EV_TYPE_THRESH,
-                                                   IIO_EV_DIR_FALLING),
-                              timestamp);
-       /* store the status to avoid repushing same events */
-       chip->old_state = int_status;
+       /*
+        * There are race conditions around enabling and disabling that
+        * could easily land us here with a spurious interrupt.
+        * Just eat it if so.
+        */
+       if (!(int_status & status_mask))
+               return IRQ_HANDLED;
+
+       iio_push_event(indio_dev,
+                      IIO_UNMOD_EVENT_CODE(IIO_CAPACITANCE, channel,
+                                           chip->type, chip->dir),
+                      timestamp);
 
        return IRQ_HANDLED;
 }
 
+static irqreturn_t ad7150_event_handler_ch1(int irq, void *private)
+{
+       return __ad7150_event_handler(private, AD7150_STATUS_OUT1, 0);
+}
+
+static irqreturn_t ad7150_event_handler_ch2(int irq, void *private)
+{
+       return __ad7150_event_handler(private, AD7150_STATUS_OUT2, 1);
+}
+
 static IIO_CONST_ATTR(in_capacitance_thresh_adaptive_timeout_available,
                      "[0 0.01 0.15]");
 

@@ -533,12 +556,44 @@ static int ad7150_probe(struct i2c_client *client,
 
        indio_dev->modes = INDIO_DIRECT_MODE;
 

Quoted 7 lines

drivers/staging/iio/cdc/ad7150.c static int ad7150_probe(struct i2c_client *client, · line 562

Quoted 5 lines

drivers/staging/iio/cdc/ad7150.c +4 -1

-       if (client->irq) {
+       chip->interrupts[0] = fwnode_irq_get(dev_fwnode(&client->dev), 0);
+       if (chip->interrupts[0] < 0)
+               return chip->interrupts[0];
+       if (id->driver_data == AD7150) {

checking 'id->driver_data' will be flaky after the of_match_table is added;

It's not, though the path is a bit obscure and only works if you don't use probe_new() - if using probe_new() you to call i2c_match_id() directly.

I remember raising this in a review I did a few years back and chasing it down. I just sanity checked and it still works ;)

So, even if we do an of_based registration the following happens

of_i2c_register_device()
-> of_i2c_get_board_info() // gets the of_node pointer
-> i2c_new_client_device(adap, &info);
.. some time later the i2c_bus_type.probe() gets called for the devices we added
and

-> i2c_device_probe()
-> driver->probe(client, i2c_match_id(driver->id_table, client))

    -> i2c_match_id() //matches against id table and hence gets us the driver_data.

Quoted 4 lines

either we defer the of_match_table patch, or we update it somehow; maybe a chip_info struct is an idea;
otherwise we need to do void pointer to int conversions [which look nasty];

Quoted 4 lines

i'm also seeing that there is a 'type' field inside of_device_id that is underused, but looks useful for cases like this;
i'm also a bit paranoid to use it; because it may not be compatible with acpi_device_id

We can refactor this at some point, but I don't think it is necessary to move the driver out of staging as a lot of drivers are doing things the same way we do it here.

Quoted 72 lines

Quoted 72 lines

drivers/staging/iio/cdc/ad7150.c +33 -12

+               chip->interrupts[1] = fwnode_irq_get(dev_fwnode(&client->dev), 1);
+               if (chip->interrupts[1] < 0)
+                       return chip->interrupts[1];
+       }
+       if (chip->interrupts[0] &&
+           (id->driver_data == AD7151 || chip->interrupts[1])) {
+               irq_set_status_flags(chip->interrupts[0], IRQ_NOAUTOEN);
+               ret = devm_request_threaded_irq(&client->dev,
+                                               chip->interrupts[0],
+                                               NULL,
+                                               &ad7150_event_handler_ch1,
+                                               IRQF_TRIGGER_RISING |
+                                               IRQF_ONESHOT,
+                                               "ad7150_irq1",
+                                               indio_dev);
+               if (ret)
+                       return ret;
+
                indio_dev->info = &ad7150_info;
                switch (id->driver_data) {
                case AD7150:
                        indio_dev->channels = ad7150_channels;
                        indio_dev->num_channels = ARRAY_SIZE(ad7150_channels);
+                       irq_set_status_flags(chip->interrupts[1], IRQ_NOAUTOEN);
+                       ret = devm_request_threaded_irq(&client->dev,
+                                                       chip->interrupts[1],
+                                                       NULL,
+                                                       &ad7150_event_handler_ch2,
+                                                       IRQF_TRIGGER_RISING |
+                                                       IRQF_ONESHOT,
+                                                       "ad7150_irq2",
+                                                       indio_dev);
+                       if (ret)
+                               return ret;
                        break;
                case AD7151:
                        indio_dev->channels = ad7151_channels;

@@ -548,25 +603,18 @@ static int ad7150_probe(struct i2c_client *client,
                        return -EINVAL;
                }
 
-               ret = devm_request_threaded_irq(&client->dev, client->irq,
-                                               NULL,
-                                               &ad7150_event_handler,
-                                               IRQF_TRIGGER_RISING |
-                                               IRQF_ONESHOT,
-                                               "ad7150_irq1",
-                                               indio_dev);
-               if (ret)
-                       return ret;
        } else {
                indio_dev->info = &ad7150_info_no_irq;
                switch (id->driver_data) {
                case AD7150:
                        indio_dev->channels = ad7150_channels_no_irq;
-                       indio_dev->num_channels = ARRAY_SIZE(ad7150_channels_no_irq);
+                       indio_dev->num_channels =
+                               ARRAY_SIZE(ad7150_channels_no_irq);
                        break;
                case AD7151:
                        indio_dev->channels = ad7151_channels_no_irq;
-                       indio_dev->num_channels = ARRAY_SIZE(ad7151_channels_no_irq);
+                       indio_dev->num_channels =
+                               ARRAY_SIZE(ad7151_channels_no_irq);
                        break;
                default:
                        return -EINVAL;
--

2.30.2

From: Jonathan Cameron <[email protected]>

Rather than using the fallback path in the i2c subsystem and hoping for no clashes across vendors, lets put in an explicit table for matching.

Signed-off-by Jonathan Cameron <[email protected]>

Link https://lore.kernel.org/r/[email protected] open here

---
 drivers/staging/iio/cdc/ad7150.c | 8 ++++++++
 1 file changed, 8 insertions(+)

drivers/staging/iio/cdc/ad7150.c +8 -1

index 30abfacda074..0afad49bcddc 100644
@@ -12,6 +12,7 @@
 #include <linux/i2c.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/mod_devicetable.h>
 #include <linux/regulator/consumer.h>
 #include <linux/slab.h>
 

@@ -655,9 +656,16 @@ static const struct i2c_device_id ad7150_id[] = {
 
 MODULE_DEVICE_TABLE(i2c, ad7150_id);
 
+static const struct of_device_id ad7150_of_match[] = {
+       { "adi,ad7150" },
+       { "adi,ad7151" },
+       { "adi,ad7156" },
+       {}
+};
 static struct i2c_driver ad7150_driver = {
        .driver = {
                .name = "ad7150",
+               .of_match_table = ad7150_of_match,
        },
        .probe = ad7150_probe,
        .id_table = ad7150_id,
--

2.30.2

From: Jonathan Cameron <[email protected]>

Main additions are around thresh_adaptive. This has been supported by the core of IIO for a long time, but no driver that uses it has previously graduated from staging, hence we are missing Docs.

Otherwise, just new entries in existing lists.

Signed-off-by Jonathan Cameron <[email protected]>

Link https://lore.kernel.org/r/[email protected] open here

---
 Documentation/ABI/testing/sysfs-bus-iio | 33 +++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

Documentation/ABI/testing/sysfs-bus-iio +33 -1

index affd4ce871d7..32d26c118141 100644
@@ -371,6 +371,7 @@ What:               /sys/bus/iio/devices/iio:deviceX/in_humidityrelative_offset
 What:          /sys/bus/iio/devices/iio:deviceX/in_magn_offset
 What:          /sys/bus/iio/devices/iio:deviceX/in_rot_offset
 What:          /sys/bus/iio/devices/iio:deviceX/in_angl_offset
+What:          /sys/bus/iio/devices/iio:deviceX/in_capacitanceX_offset
 KernelVersion: 2.6.35
 Contact:       [email protected]
 Description:

@@ -702,6 +703,8 @@ What:               /sys/.../iio:deviceX/events/in_voltageY_thresh_falling_en
 What:          /sys/.../iio:deviceX/events/in_voltageY_thresh_either_en
 What:          /sys/.../iio:deviceX/events/in_tempY_thresh_rising_en
 What:          /sys/.../iio:deviceX/events/in_tempY_thresh_falling_en
+What:          /sys/.../iio:deviceX/events/in_capacitanceY_thresh_rising_en
+What:          /sys/.../iio:deviceX/events/in_capacitanceY_thresh_falling_en
 KernelVersion: 2.6.37
 Contact:       [email protected]
 Description:

@@ -779,6 +782,32 @@ Description:
                a given event type is enabled a future point (and not those for
                whatever event was previously enabled).
 
+What:          /sys/.../events/in_capacitanceY_adaptive_thresh_rising_en
+What:          /sys/.../events/in_capacitanceY_adaptive_thresh_falling_en
+KernelVersion: 5.11
+Contact:       [email protected]
+Descrption:
+               Adaptive thresholds are similar to normal fixed thresholds
+               but the value is expressed as an offset from a value which
+               provides a low frequency approximation of the channel itself.
+               Thus these detect if a rapid change occurs in the specified
+               direction which crosses tracking value + offset.
+               Tracking value calculation is devices specific.
+
+What:          /sys/.../in_capacitanceY_adaptive_thresh_rising_timeout
+What:          /sys/.../in_capacitanceY_adaptive_thresh_falling_timeout
+KernelVersion: 5.11
+Contact:       [email protected]
+Descrption:
+               When adaptive thresholds are used, the tracking signal
+               may adjust too slowly to step changes in the raw signal.
+               *_timeout (in seconds) specifies a time for which the
+               difference between the slow tracking signal and the raw
+               signal is allowed to remain out-of-range before a reset
+               event occurs in which the tracking signal is made equal
+               to the raw signal, allowing slow tracking to resume and the
+               adaptive threshold event detection to function as expected.
+
 What:          /sys/.../events/in_accel_thresh_rising_value
 What:          /sys/.../events/in_accel_thresh_falling_value
 What:          /sys/.../events/in_accel_x_raw_thresh_rising_value

@@ -819,6 +848,10 @@ What:              /sys/.../events/in_proximity0_thresh_falling_value
 What:          /sys/.../events/in_proximity0_thresh_rising_value
 What:          /sys/.../events/in_illuminance_thresh_rising_value
 What:          /sys/.../events/in_illuminance_thresh_falling_value
+What:          /sys/.../events/in_capacitanceY_thresh_rising_value
+What:          /sys/.../events/in_capacitanceY_thresh_falling_value
+What:          /sys/.../events/in_capacitanceY_thresh_adaptive_rising_value
+What:          /sys/.../events/in_capacitanceY_thresh_falling_rising_value
 KernelVersion: 2.6.37
 Contact:       [email protected]
 Description:
--

2.30.2

From: Jonathan Cameron <[email protected]>

Binding covering the ad7150, ad7151 and ad7156 capacitance to digital convertors. The only difference between these is how many channels they have (1 or 2)

Whilst it is clearly necessary to provide power to the part, we don't need to know the voltage or anything so if it is always on, there should be no need to have it specified in the binding.

Signed-off-by Jonathan Cameron <[email protected]>

Cc [email protected]

Cc [email protected]

Link https://lore.kernel.org/r/[email protected] open here

---
 .../bindings/iio/cdc/adi,ad7150.yaml          | 69 +++++++++++++++++++
 1 file changed, 69 insertions(+)