[PATCH v2 00/24] staging:iio:cdc:ad7150: cleanup / fixup / graduate

[PATCH v2 00/24] staging:iio:cdc:ad7150: cleanup / fixup / graduate

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Hi All,

There is just enough in here to need a full posting as v2.
Note modifications are really small from v1. This v2 is more about seeking
reviews of a few patches that still need some eyes on them.

Thanks to Alex and Barry for reviews on v1.  There are a few patches
that still need someone to take a look though.

23 - DT bindings (I moved these later in the series to help them stand out)
     No changes in here beyond adding a clarification on the regulator to
     the patch description. @Rob, if you could sanity check this that would
     be great.

12 - The big irq rework patch. Alex wasn't quite happy to give a tag
     on that last time, but didn't mention anything specific. It's a bit
     fiddly so fair enough!

20 - of_match_table patch.  Simple patch, and I believe I addressed all
     questions in v1, but no tags given yet.

21 - ABI patch.  I don't think there is anything controversial in here
     but it's not gained any tags yet.
     
13 - Really minor tweak to avoid a build warning.  I've kept Alex's
     tag on that one.

v1 description:

This is an 'old' driver in IIO that has been in staging a while.
First submitted in October 2010.

I wanted to try and experiment and picked this driver to try it with.

The cleanup etc here was all tested against some basic emulation
added to QEMU rather than real hardware.  Once I've cleaned that up
a tiny bit I'll push it up to https://github.com/jic23/qemu
Note that for now I'm not proposing to upstream this to QEMU but
would be interested in hearing if people thing it is a good idea to
do so.

Whilst it's obviously hard to be absolutely sure that the emulation is
correct, the fact that the original driver worked as expected and the
cleaned up version still does is certainly encouraging.

Note however, that there were a few more significant changes in here than
basic cleanup.
1. Interrupts / events were handled in a rather esoteric fashion.
   (Always on, window modes represented as magnitudes).
   Note that for two channel devices there are separate lines. The original
   driver was not supporting this at all.
   They now look more like a standard IIO driver and reflect experience
   that we've gained over the years in dealing with devices where these
   aren't interrupt lines as such, but rather reporters of current status.
2. Timeouts were handled in a fashion that clearly wouldn't work.

Note that this moving out of staging makes a few bits of ABI 'official'
and so those are added to the main IIO ABI Docs.

Thanks in advance to anyone who has time to take a look.

Jonathan Cameron (24):
  staging:iio:cdc:ad7150: use swapped reads for i2c rather than open
    coding.
  staging:iio:cdc:ad7150: Remove magnitude adaptive events
  staging:iio:cdc:ad7150: Refactor event parameter update
  staging:iio:cdc:ad7150: Timeout register covers both directions so
    both need updating
  staging:iio:cdc:ad7150: Drop platform data support
  staging:iio:cdc:ad7150: Handle variation in chan_spec across device
    and irq present or not
  staging:iio:cdc:ad7150: Simplify event handling by only using rising
    direction.
  staging:iio:cdc:ad7150: Drop noisy print in probe
  staging:iio:cdc:ad7150: Add sampling_frequency support
  iio:event: Add timeout event info type
  staging:iio:cdc:ad7150: Change timeout units to seconds and use core
    support
  staging:iio:cdc:ad7150: Rework interrupt handling.
  staging:iio:cdc:ad7150: More consistent register and field naming
  staging:iio:cdc:ad7150: Reorganize headers.
  staging:iio:cdc:ad7150: Tidy up local variable positioning.
  staging:iio:cdc:ad7150: Drop unnecessary block comments.
  staging:iio:cdc:ad7150: Shift the _raw readings by 4 bits.
  staging:iio:cdc:ad7150: Add scale and offset to
    info_mask_shared_by_type
  staging:iio:cdc:ad7150: Really basic regulator support.
  staging:iio:cdc:ad7150: Add of_match_table
  iio:Documentation:ABI Add missing elements as used by the adi,ad7150
  staging:iio:cdc:ad7150: Add copyright notice given substantial
    changes.
  dt-bindings:iio:cdc:adi,ad7150 binding doc
  iio:cdc:ad7150: Move driver out of staging.

 Documentation/ABI/testing/sysfs-bus-iio       |  33 +
 .../bindings/iio/cdc/adi,ad7150.yaml          |  69 ++
 drivers/iio/Kconfig                           |   1 +
 drivers/iio/Makefile                          |   1 +
 drivers/iio/cdc/Kconfig                       |  17 +
 drivers/iio/cdc/Makefile                      |   6 +
 drivers/iio/cdc/ad7150.c                      | 678 ++++++++++++++++++
 drivers/iio/industrialio-event.c              |   1 +
 drivers/staging/iio/cdc/Kconfig               |  10 -
 drivers/staging/iio/cdc/Makefile              |   3 +-
 drivers/staging/iio/cdc/ad7150.c              | 655 -----------------
 include/linux/iio/types.h                     |   1 +
 12 files changed, 808 insertions(+), 667 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/cdc/adi,ad7150.yaml
 create mode 100644 drivers/iio/cdc/Kconfig
 create mode 100644 drivers/iio/cdc/Makefile
 create mode 100644 drivers/iio/cdc/ad7150.c
 delete mode 100644 drivers/staging/iio/cdc/ad7150.c

-- 
2.30.2

[PATCH v2 01/24] staging:iio:cdc:ad7150: use swapped reads for i2c rather than open coding.

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Reduces boilerplate and chances of getting the error handling wrong.
No functional change.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Link: https://lore.kernel.org/r/20210207154623.433442-2-jic23@kernel.org
---
 drivers/staging/iio/cdc/ad7150.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c
index 48132ab157ef..c3ed88c5e0a5 100644
--- a/drivers/staging/iio/cdc/ad7150.c
+++ b/drivers/staging/iio/cdc/ad7150.c
@@ -109,18 +109,20 @@ static int ad7150_read_raw(struct iio_dev *indio_dev,
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
-		ret = i2c_smbus_read_word_data(chip->client,
-					       ad7150_addresses[channel][0]);
+		ret = i2c_smbus_read_word_swapped(chip->client,
+						  ad7150_addresses[channel][0]);
 		if (ret < 0)
 			return ret;
-		*val = swab16(ret);
+		*val = ret;
+
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_AVERAGE_RAW:
-		ret = i2c_smbus_read_word_data(chip->client,
-					       ad7150_addresses[channel][1]);
+		ret = i2c_smbus_read_word_swapped(chip->client,
+						  ad7150_addresses[channel][1]);
 		if (ret < 0)
 			return ret;
-		*val = swab16(ret);
+		*val = ret;
+
 		return IIO_VAL_INT;
 	default:
 		return -EINVAL;
@@ -188,9 +190,9 @@ static int ad7150_write_event_params(struct iio_dev *indio_dev,
 		/* Note completely different from the adaptive versions */
 	case IIO_EV_TYPE_THRESH:
 		value = chip->threshold[rising][chan];
-		return i2c_smbus_write_word_data(chip->client,
-						 ad7150_addresses[chan][3],
-						 swab16(value));
+		return i2c_smbus_write_word_swapped(chip->client,
+						    ad7150_addresses[chan][3],
+						    value);
 	case IIO_EV_TYPE_MAG_ADAPTIVE:
 		sens = chip->mag_sensitivity[rising][chan];
 		timeout = chip->mag_timeout[rising][chan];
-- 
2.30.2

[PATCH v2 02/24] staging:iio:cdc:ad7150: Remove magnitude adaptive events

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

The devices support window detection, but that corresponds to
being outside of a range defined by a lower an uppper bound rather
than being related to magnitude as such.   Hence drop this interface
in the interests of making the driver ABI compliant.

We may bring back support for the window mode at somepoint in the future
but it will be in an ABI compliant fashion.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Link: https://lore.kernel.org/r/20210207154623.433442-3-jic23@kernel.org
---
 drivers/staging/iio/cdc/ad7150.c | 56 --------------------------------
 1 file changed, 56 deletions(-)

diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c
index c3ed88c5e0a5..4dac4aaec0cf 100644
--- a/drivers/staging/iio/cdc/ad7150.c
+++ b/drivers/staging/iio/cdc/ad7150.c
@@ -57,14 +57,9 @@
  * @threshold: thresholds for simple capacitance value events
  * @thresh_sensitivity: threshold for simple capacitance offset
  *	from 'average' value.
- * @mag_sensitity: threshold for magnitude of capacitance offset from
- *	from 'average' value.
  * @thresh_timeout: a timeout, in samples from the moment an
  *	adaptive threshold event occurs to when the average
  *	value jumps to current value.
- * @mag_timeout: a timeout, in sample from the moment an
- *	adaptive magnitude event occurs to when the average
- *	value jumps to the current value.
  * @old_state: store state from previous event, allowing confirmation
  *	of new condition.
  * @conversion_mode: the current conversion mode.
@@ -76,9 +71,7 @@ struct ad7150_chip_info {
 	u64 current_event;
 	u16 threshold[2][2];
 	u8 thresh_sensitivity[2][2];
-	u8 mag_sensitivity[2][2];
 	u8 thresh_timeout[2][2];
-	u8 mag_timeout[2][2];
 	int old_state;
 	char *conversion_mode;
 	struct mutex state_lock;
@@ -149,10 +142,6 @@ static int ad7150_read_event_config(struct iio_dev *indio_dev,
 	thrfixed = FIELD_GET(AD7150_CFG_FIX, ret);
 
 	switch (type) {
-	case IIO_EV_TYPE_MAG_ADAPTIVE:
-		if (dir == IIO_EV_DIR_RISING)
-			return !thrfixed && (threshtype == 0x1);
-		return !thrfixed && (threshtype == 0x0);
 	case IIO_EV_TYPE_THRESH_ADAPTIVE:
 		if (dir == IIO_EV_DIR_RISING)
 			return !thrfixed && (threshtype == 0x3);
@@ -193,10 +182,6 @@ static int ad7150_write_event_params(struct iio_dev *indio_dev,
 		return i2c_smbus_write_word_swapped(chip->client,
 						    ad7150_addresses[chan][3],
 						    value);
-	case IIO_EV_TYPE_MAG_ADAPTIVE:
-		sens = chip->mag_sensitivity[rising][chan];
-		timeout = chip->mag_timeout[rising][chan];
-		break;
 	case IIO_EV_TYPE_THRESH_ADAPTIVE:
 		sens = chip->thresh_sensitivity[rising][chan];
 		timeout = chip->thresh_timeout[rising][chan];
@@ -240,13 +225,6 @@ static int ad7150_write_event_config(struct iio_dev *indio_dev,
 	cfg = ret & ~((0x03 << 5) | BIT(7));
 
 	switch (type) {
-	case IIO_EV_TYPE_MAG_ADAPTIVE:
-		adaptive = 1;
-		if (rising)
-			thresh_type = 0x1;
-		else
-			thresh_type = 0x0;
-		break;
 	case IIO_EV_TYPE_THRESH_ADAPTIVE:
 		adaptive = 1;
 		if (rising)
@@ -294,9 +272,6 @@ static int ad7150_read_event_value(struct iio_dev *indio_dev,
 
 	/* Complex register sharing going on here */
 	switch (type) {
-	case IIO_EV_TYPE_MAG_ADAPTIVE:
-		*val = chip->mag_sensitivity[rising][chan->channel];
-		return IIO_VAL_INT;
 	case IIO_EV_TYPE_THRESH_ADAPTIVE:
 		*val = chip->thresh_sensitivity[rising][chan->channel];
 		return IIO_VAL_INT;
@@ -321,9 +296,6 @@ static int ad7150_write_event_value(struct iio_dev *indio_dev,
 
 	mutex_lock(&chip->state_lock);
 	switch (type) {
-	case IIO_EV_TYPE_MAG_ADAPTIVE:
-		chip->mag_sensitivity[rising][chan->channel] = val;
-		break;
 	case IIO_EV_TYPE_THRESH_ADAPTIVE:
 		chip->thresh_sensitivity[rising][chan->channel] = val;
 		break;
@@ -358,9 +330,6 @@ static ssize_t ad7150_show_timeout(struct device *dev,
 		      == IIO_EV_DIR_RISING) ? 1 : 0;
 
 	switch (IIO_EVENT_CODE_EXTRACT_TYPE(this_attr->address)) {
-	case IIO_EV_TYPE_MAG_ADAPTIVE:
-		value = chip->mag_timeout[rising][chan];
-		break;
 	case IIO_EV_TYPE_THRESH_ADAPTIVE:
 		value = chip->thresh_timeout[rising][chan];
 		break;
@@ -396,9 +365,6 @@ static ssize_t ad7150_store_timeout(struct device *dev,
 
 	mutex_lock(&chip->state_lock);
 	switch (type) {
-	case IIO_EV_TYPE_MAG_ADAPTIVE:
-		chip->mag_timeout[rising][chan] = data;
-		break;
 	case IIO_EV_TYPE_THRESH_ADAPTIVE:
 		chip->thresh_timeout[rising][chan] = data;
 		break;
@@ -426,10 +392,6 @@ static ssize_t ad7150_store_timeout(struct device *dev,
 				     chan,				\
 				     IIO_EV_TYPE_##ev_type,		\
 				     IIO_EV_DIR_##ev_dir))
-static AD7150_TIMEOUT(0, mag_adaptive, rising, MAG_ADAPTIVE, RISING);
-static AD7150_TIMEOUT(0, mag_adaptive, falling, MAG_ADAPTIVE, FALLING);
-static AD7150_TIMEOUT(1, mag_adaptive, rising, MAG_ADAPTIVE, RISING);
-static AD7150_TIMEOUT(1, mag_adaptive, falling, MAG_ADAPTIVE, FALLING);
 static AD7150_TIMEOUT(0, thresh_adaptive, rising, THRESH_ADAPTIVE, RISING);
 static AD7150_TIMEOUT(0, thresh_adaptive, falling, THRESH_ADAPTIVE, FALLING);
 static AD7150_TIMEOUT(1, thresh_adaptive, rising, THRESH_ADAPTIVE, RISING);
@@ -456,16 +418,6 @@ static const struct iio_event_spec ad7150_events[] = {
 		.dir = IIO_EV_DIR_FALLING,
 		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
 			BIT(IIO_EV_INFO_ENABLE),
-	}, {
-		.type = IIO_EV_TYPE_MAG_ADAPTIVE,
-		.dir = IIO_EV_DIR_RISING,
-		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
-			BIT(IIO_EV_INFO_ENABLE),
-	}, {
-		.type = IIO_EV_TYPE_MAG_ADAPTIVE,
-		.dir = IIO_EV_DIR_FALLING,
-		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
-			BIT(IIO_EV_INFO_ENABLE),
 	},
 };
 
@@ -539,14 +491,6 @@ static irqreturn_t ad7150_event_handler(int irq, void *private)
 
 /* Timeouts not currently handled by core */
 static struct attribute *ad7150_event_attributes[] = {
-	&iio_dev_attr_in_capacitance0_mag_adaptive_rising_timeout
-	.dev_attr.attr,
-	&iio_dev_attr_in_capacitance0_mag_adaptive_falling_timeout
-	.dev_attr.attr,
-	&iio_dev_attr_in_capacitance1_mag_adaptive_rising_timeout
-	.dev_attr.attr,
-	&iio_dev_attr_in_capacitance1_mag_adaptive_falling_timeout
-	.dev_attr.attr,
 	&iio_dev_attr_in_capacitance0_thresh_adaptive_rising_timeout
 	.dev_attr.attr,
 	&iio_dev_attr_in_capacitance0_thresh_adaptive_falling_timeout
-- 
2.30.2

[PATCH v2 03/24] staging:iio:cdc:ad7150: Refactor event parameter update

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Original code was ordered in a fairly unituitive fashion with
the non adaptive threshold handling returning from the switch
statement, whilst the adapative path did the actual writes outside
the switch.   Make it more readable by bringing everything within
the switch statement cases and reducing scope of local variables
as appropriate.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Link: https://lore.kernel.org/r/20210207154623.433442-4-jic23@kernel.org
---
 drivers/staging/iio/cdc/ad7150.c | 32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)

diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c
index 4dac4aaec0cf..d6a7bfd94f1c 100644
--- a/drivers/staging/iio/cdc/ad7150.c
+++ b/drivers/staging/iio/cdc/ad7150.c
@@ -163,9 +163,6 @@ static int ad7150_write_event_params(struct iio_dev *indio_dev,
 				     enum iio_event_type type,
 				     enum iio_event_direction dir)
 {
-	int ret;
-	u16 value;
-	u8 sens, timeout;
 	struct ad7150_chip_info *chip = iio_priv(indio_dev);
 	int rising = (dir == IIO_EV_DIR_RISING);
 	u64 event_code;
@@ -177,26 +174,31 @@ static int ad7150_write_event_params(struct iio_dev *indio_dev,
 
 	switch (type) {
 		/* Note completely different from the adaptive versions */
-	case IIO_EV_TYPE_THRESH:
-		value = chip->threshold[rising][chan];
+	case IIO_EV_TYPE_THRESH: {
+		u16 value = chip->threshold[rising][chan];
 		return i2c_smbus_write_word_swapped(chip->client,
 						    ad7150_addresses[chan][3],
 						    value);
-	case IIO_EV_TYPE_THRESH_ADAPTIVE:
+	}
+	case IIO_EV_TYPE_THRESH_ADAPTIVE: {
+		int ret;
+		u8 sens, timeout;
+
 		sens = chip->thresh_sensitivity[rising][chan];
+		ret = i2c_smbus_write_byte_data(chip->client,
+						ad7150_addresses[chan][4],
+						sens);
+		if (ret)
+			return ret;
+
 		timeout = chip->thresh_timeout[rising][chan];
-		break;
+		return i2c_smbus_write_byte_data(chip->client,
+						 ad7150_addresses[chan][5],
+						 timeout);
+	}
 	default:
 		return -EINVAL;
 	}
-	ret = i2c_smbus_write_byte_data(chip->client,
-					ad7150_addresses[chan][4],
-					sens);
-	if (ret)
-		return ret;
-	return i2c_smbus_write_byte_data(chip->client,
-					ad7150_addresses[chan][5],
-					timeout);
 }
 
 static int ad7150_write_event_config(struct iio_dev *indio_dev,
-- 
2.30.2

[PATCH v2 04/24] staging:iio:cdc:ad7150: Timeout register covers both directions so both need updating

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

The timeout is treated as one single value, but the datasheet describes
it as two 4 bit values, one for each direction of event.
As such change the driver to support the separate directions.
Also add limit checking to ensure it fits within the 4 bits.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Link: https://lore.kernel.org/r/20210207154623.433442-5-jic23@kernel.org
---
 drivers/staging/iio/cdc/ad7150.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c
index d6a7bfd94f1c..0dce1b8ce76d 100644
--- a/drivers/staging/iio/cdc/ad7150.c
+++ b/drivers/staging/iio/cdc/ad7150.c
@@ -49,6 +49,8 @@
 /* AD7150 masks */
 #define AD7150_THRESHTYPE_MSK			GENMASK(6, 5)
 
+#define AD7150_CH_TIMEOUT_RECEDING		GENMASK(3, 0)
+#define AD7150_CH_TIMEOUT_APPROACHING		GENMASK(7, 4)
 /**
  * struct ad7150_chip_info - instance specific chip data
  * @client: i2c client for this device
@@ -59,7 +61,9 @@
  *	from 'average' value.
  * @thresh_timeout: a timeout, in samples from the moment an
  *	adaptive threshold event occurs to when the average
- *	value jumps to current value.
+ *	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.
@@ -191,7 +195,14 @@ static int ad7150_write_event_params(struct iio_dev *indio_dev,
 		if (ret)
 			return ret;
 
-		timeout = chip->thresh_timeout[rising][chan];
+		/*
+		 * Single timeout register contains timeouts for both
+		 * directions.
+		 */
+		timeout = FIELD_PREP(AD7150_CH_TIMEOUT_APPROACHING,
+				     chip->thresh_timeout[1][chan]);
+		timeout |= FIELD_PREP(AD7150_CH_TIMEOUT_RECEDING,
+				      chip->thresh_timeout[0][chan]);
 		return i2c_smbus_write_byte_data(chip->client,
 						 ad7150_addresses[chan][5],
 						 timeout);
@@ -365,6 +376,9 @@ static ssize_t ad7150_store_timeout(struct device *dev,
 	if (ret < 0)
 		return ret;
 
+	if (data > GENMASK(3, 0))
+		return -EINVAL;
+
 	mutex_lock(&chip->state_lock);
 	switch (type) {
 	case IIO_EV_TYPE_THRESH_ADAPTIVE:
-- 
2.30.2

[PATCH v2 05/24] staging:iio:cdc:ad7150: Drop platform data support

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

There are no mainline board files using this driver so lets drop
the platform_data support in favour of devicetree and similar.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Link: https://lore.kernel.org/r/20210207154623.433442-6-jic23@kernel.org
---
 drivers/staging/iio/cdc/ad7150.c | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c
index 0dce1b8ce76d..7ad9105e6b46 100644
--- a/drivers/staging/iio/cdc/ad7150.c
+++ b/drivers/staging/iio/cdc/ad7150.c
@@ -570,20 +570,6 @@ static int ad7150_probe(struct i2c_client *client,
 			return ret;
 	}
 
-	if (client->dev.platform_data) {
-		ret = devm_request_threaded_irq(&client->dev, *(unsigned int *)
-						client->dev.platform_data,
-						NULL,
-						&ad7150_event_handler,
-						IRQF_TRIGGER_RISING |
-						IRQF_TRIGGER_FALLING |
-						IRQF_ONESHOT,
-						"ad7150_irq2",
-						indio_dev);
-		if (ret)
-			return ret;
-	}
-
 	ret = devm_iio_device_register(indio_dev->dev.parent, indio_dev);
 	if (ret)
 		return ret;
-- 
2.30.2

[PATCH v2 06/24] staging:iio:cdc:ad7150: Handle variation in chan_spec across device and irq present or not

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

The driver supports devices with different numbers of channels and
also can function without provision of an IRQ (with reduced features),
so this patch handles this cleanly by having multiple chan_spec
arrays and iio_info structures to pick between depending on what we
have.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Link: https://lore.kernel.org/r/20210207154623.433442-7-jic23@kernel.org
---
 drivers/staging/iio/cdc/ad7150.c | 71 ++++++++++++++++++++++++++++----
 1 file changed, 63 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c
index 7ad9105e6b46..539beed1a511 100644
--- a/drivers/staging/iio/cdc/ad7150.c
+++ b/drivers/staging/iio/cdc/ad7150.c
@@ -51,6 +51,12 @@
 
 #define AD7150_CH_TIMEOUT_RECEDING		GENMASK(3, 0)
 #define AD7150_CH_TIMEOUT_APPROACHING		GENMASK(7, 4)
+
+enum {
+	AD7150,
+	AD7151,
+};
+
 /**
  * struct ad7150_chip_info - instance specific chip data
  * @client: i2c client for this device
@@ -447,9 +453,30 @@ static const struct iio_event_spec ad7150_events[] = {
 		.num_event_specs = ARRAY_SIZE(ad7150_events),	\
 	}
 
+#define AD7150_CAPACITANCE_CHAN_NO_IRQ(_chan)	{		\
+		.type = IIO_CAPACITANCE,			\
+		.indexed = 1,					\
+		.channel = _chan,				\
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |	\
+		BIT(IIO_CHAN_INFO_AVERAGE_RAW),			\
+	}
+
 static const struct iio_chan_spec ad7150_channels[] = {
 	AD7150_CAPACITANCE_CHAN(0),
-	AD7150_CAPACITANCE_CHAN(1)
+	AD7150_CAPACITANCE_CHAN(1),
+};
+
+static const struct iio_chan_spec ad7150_channels_no_irq[] = {
+	AD7150_CAPACITANCE_CHAN_NO_IRQ(0),
+	AD7150_CAPACITANCE_CHAN_NO_IRQ(1),
+};
+
+static const struct iio_chan_spec ad7151_channels[] = {
+	AD7150_CAPACITANCE_CHAN(0),
+};
+
+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)
@@ -532,6 +559,10 @@ static const struct iio_info ad7150_info = {
 	.write_event_value = &ad7150_write_event_value,
 };
 
+static const struct iio_info ad7150_info_no_irq = {
+	.read_raw = &ad7150_read_raw,
+};
+
 static int ad7150_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
@@ -550,14 +581,24 @@ static int ad7150_probe(struct i2c_client *client,
 	chip->client = client;
 
 	indio_dev->name = id->name;
-	indio_dev->channels = ad7150_channels;
-	indio_dev->num_channels = ARRAY_SIZE(ad7150_channels);
-
-	indio_dev->info = &ad7150_info;
 
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
 	if (client->irq) {
+		indio_dev->info = &ad7150_info;
+		switch (id->driver_data) {
+		case AD7150:
+			indio_dev->channels = ad7150_channels;
+			indio_dev->num_channels = ARRAY_SIZE(ad7150_channels);
+			break;
+		case AD7151:
+			indio_dev->channels = ad7151_channels;
+			indio_dev->num_channels = ARRAY_SIZE(ad7151_channels);
+			break;
+		default:
+			return -EINVAL;
+		}
+
 		ret = devm_request_threaded_irq(&client->dev, client->irq,
 						NULL,
 						&ad7150_event_handler,
@@ -568,6 +609,20 @@ static int ad7150_probe(struct i2c_client *client,
 						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);
+			break;
+		case AD7151:
+			indio_dev->channels = ad7151_channels_no_irq;
+			indio_dev->num_channels = ARRAY_SIZE(ad7151_channels_no_irq);
+			break;
+		default:
+			return -EINVAL;
+		}
 	}
 
 	ret = devm_iio_device_register(indio_dev->dev.parent, indio_dev);
@@ -581,9 +636,9 @@ static int ad7150_probe(struct i2c_client *client,
 }
 
 static const struct i2c_device_id ad7150_id[] = {
-	{ "ad7150", 0 },
-	{ "ad7151", 0 },
-	{ "ad7156", 0 },
+	{ "ad7150", AD7150 },
+	{ "ad7151", AD7151 },
+	{ "ad7156", AD7150 },
 	{}
 };
 
-- 
2.30.2

[PATCH v2 07/24] staging:iio:cdc:ad7150: Simplify event handling by only using rising direction.

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

The event line is active high and not maskable within the device.
It indicates current state directly.

The device supports separate rising and falling thresholds so rather
than trying to using each bound to detect in both directions just use
IRQF_TRIGGER_RISING.  If a user wants to detect the value falling
back below the threshold, then set the falling threshold appropriately.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Link: https://lore.kernel.org/r/20210207154623.433442-8-jic23@kernel.org
---
 drivers/staging/iio/cdc/ad7150.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c
index 539beed1a511..34e6afe52f0e 100644
--- a/drivers/staging/iio/cdc/ad7150.c
+++ b/drivers/staging/iio/cdc/ad7150.c
@@ -603,7 +603,6 @@ static int ad7150_probe(struct i2c_client *client,
 						NULL,
 						&ad7150_event_handler,
 						IRQF_TRIGGER_RISING |
-						IRQF_TRIGGER_FALLING |
 						IRQF_ONESHOT,
 						"ad7150_irq1",
 						indio_dev);
-- 
2.30.2

[PATCH v2 08/24] staging:iio:cdc:ad7150: Drop noisy print in probe

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Also
* drop i2c_set_client_data() as now unused.
* white space cleanups

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Barry Song <song.bao.hua@hisilicon.com>
Reviewed-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Link: https://lore.kernel.org/r/20210207154623.433442-9-jic23@kernel.org
---
 drivers/staging/iio/cdc/ad7150.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c
index 34e6afe52f0e..8f8e472e3240 100644
--- a/drivers/staging/iio/cdc/ad7150.c
+++ b/drivers/staging/iio/cdc/ad7150.c
@@ -573,11 +573,9 @@ static int ad7150_probe(struct i2c_client *client,
 	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
 	if (!indio_dev)
 		return -ENOMEM;
+
 	chip = iio_priv(indio_dev);
 	mutex_init(&chip->state_lock);
-	/* this is only used for device removal purposes */
-	i2c_set_clientdata(client, indio_dev);
-
 	chip->client = client;
 
 	indio_dev->name = id->name;
@@ -624,14 +622,7 @@ static int ad7150_probe(struct i2c_client *client,
 		}
 	}
 
-	ret = devm_iio_device_register(indio_dev->dev.parent, indio_dev);
-	if (ret)
-		return ret;
-
-	dev_info(&client->dev, "%s capacitive sensor registered,irq: %d\n",
-		 id->name, client->irq);
-
-	return 0;
+	return devm_iio_device_register(indio_dev->dev.parent, indio_dev);
 }
 
 static const struct i2c_device_id ad7150_id[] = {
-- 
2.30.2

[PATCH v2 09/24] staging:iio:cdc:ad7150: Add sampling_frequency support

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Device uses a fixed sampling frequency. Let us expose it to userspace.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Link: https://lore.kernel.org/r/20210207154623.433442-10-jic23@kernel.org
---
 drivers/staging/iio/cdc/ad7150.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c
index 8f8e472e3240..54f31aadc92a 100644
--- a/drivers/staging/iio/cdc/ad7150.c
+++ b/drivers/staging/iio/cdc/ad7150.c
@@ -126,6 +126,10 @@ static int ad7150_read_raw(struct iio_dev *indio_dev,
 			return ret;
 		*val = ret;
 
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		/* Strangely same for both 1 and 2 chan parts */
+		*val = 100;
 		return IIO_VAL_INT;
 	default:
 		return -EINVAL;
@@ -449,6 +453,7 @@ static const struct iio_event_spec ad7150_events[] = {
 		.channel = _chan,				\
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |	\
 		BIT(IIO_CHAN_INFO_AVERAGE_RAW),			\
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),\
 		.event_spec = ad7150_events,			\
 		.num_event_specs = ARRAY_SIZE(ad7150_events),	\
 	}
@@ -459,6 +464,7 @@ static const struct iio_event_spec ad7150_events[] = {
 		.channel = _chan,				\
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |	\
 		BIT(IIO_CHAN_INFO_AVERAGE_RAW),			\
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),\
 	}
 
 static const struct iio_chan_spec ad7150_channels[] = {
-- 
2.30.2

[PATCH v2 10/24] iio:event: Add timeout event info type

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

For adaptive threshold events, the current value is compared with a
(typically) low pass filtered version of the same signal that slowly
tracks large scale changes.  However, sometimes a step change can
result in a large lag before the low pass filtered version begins
to track the signal again.  Timeouts can be used to made an
instantaneous 'correction'.  Documentation of this attribute
is added in a later patch.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Link: https://lore.kernel.org/r/20210207154623.433442-11-jic23@kernel.org
---
 drivers/iio/industrialio-event.c | 1 +
 include/linux/iio/types.h        | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/iio/industrialio-event.c b/drivers/iio/industrialio-event.c
index a30e289fc362..17420682e308 100644
--- a/drivers/iio/industrialio-event.c
+++ b/drivers/iio/industrialio-event.c
@@ -245,6 +245,7 @@ static const char * const iio_ev_info_text[] = {
 	[IIO_EV_INFO_PERIOD] = "period",
 	[IIO_EV_INFO_HIGH_PASS_FILTER_3DB] = "high_pass_filter_3db",
 	[IIO_EV_INFO_LOW_PASS_FILTER_3DB] = "low_pass_filter_3db",
+	[IIO_EV_INFO_TIMEOUT] = "timeout",
 };
 
 static enum iio_event_direction iio_ev_attr_dir(struct iio_dev_attr *attr)
diff --git a/include/linux/iio/types.h b/include/linux/iio/types.h
index 5aa7f66d4345..84b3f8175cc6 100644
--- a/include/linux/iio/types.h
+++ b/include/linux/iio/types.h
@@ -16,6 +16,7 @@ enum iio_event_info {
 	IIO_EV_INFO_PERIOD,
 	IIO_EV_INFO_HIGH_PASS_FILTER_3DB,
 	IIO_EV_INFO_LOW_PASS_FILTER_3DB,
+	IIO_EV_INFO_TIMEOUT,
 };
 
 #define IIO_VAL_INT 1
-- 
2.30.2

[PATCH v2 11/24] staging:iio:cdc:ad7150: Change timeout units to seconds and use core support

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Now we have core support for timeouts related to adaptive events, let us
use it.  Note the units of that attribute are seconds, so we also need
to scale the cycles value by the period of each sample.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Link: https://lore.kernel.org/r/20210207154623.433442-12-jic23@kernel.org
---
 drivers/staging/iio/cdc/ad7150.c | 159 ++++++++++---------------------
 1 file changed, 52 insertions(+), 107 deletions(-)

diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c
index 54f31aadc92a..f8183c540d5c 100644
--- a/drivers/staging/iio/cdc/ad7150.c
+++ b/drivers/staging/iio/cdc/ad7150.c
@@ -294,13 +294,22 @@ static int ad7150_read_event_value(struct iio_dev *indio_dev,
 	int rising = (dir == IIO_EV_DIR_RISING);
 
 	/* Complex register sharing going on here */
-	switch (type) {
-	case IIO_EV_TYPE_THRESH_ADAPTIVE:
-		*val = chip->thresh_sensitivity[rising][chan->channel];
-		return IIO_VAL_INT;
-	case IIO_EV_TYPE_THRESH:
-		*val = chip->threshold[rising][chan->channel];
-		return IIO_VAL_INT;
+	switch (info) {
+	case IIO_EV_INFO_VALUE:
+		switch (type) {
+		case IIO_EV_TYPE_THRESH_ADAPTIVE:
+			*val = chip->thresh_sensitivity[rising][chan->channel];
+			return IIO_VAL_INT;
+		case IIO_EV_TYPE_THRESH:
+			*val = chip->threshold[rising][chan->channel];
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+		}
+	case IIO_EV_INFO_TIMEOUT:
+		*val = 0;
+		*val2 = chip->thresh_timeout[rising][chan->channel] * 10000;
+		return IIO_VAL_INT_PLUS_MICRO;
 	default:
 		return -EINVAL;
 	}
@@ -318,13 +327,36 @@ static int ad7150_write_event_value(struct iio_dev *indio_dev,
 	int rising = (dir == IIO_EV_DIR_RISING);
 
 	mutex_lock(&chip->state_lock);
-	switch (type) {
-	case IIO_EV_TYPE_THRESH_ADAPTIVE:
-		chip->thresh_sensitivity[rising][chan->channel] = val;
+	switch (info) {
+	case IIO_EV_INFO_VALUE:
+		switch (type) {
+		case IIO_EV_TYPE_THRESH_ADAPTIVE:
+			chip->thresh_sensitivity[rising][chan->channel] = val;
+			break;
+		case IIO_EV_TYPE_THRESH:
+			chip->threshold[rising][chan->channel] = val;
+			break;
+		default:
+			ret = -EINVAL;
+			goto error_ret;
+		}
 		break;
-	case IIO_EV_TYPE_THRESH:
-		chip->threshold[rising][chan->channel] = val;
+	case IIO_EV_INFO_TIMEOUT: {
+		/*
+		 * Raw timeout is in cycles of 10 msecs as long as both
+		 * channels are enabled.
+		 * In terms of INT_PLUS_MICRO, that is in units of 10,000
+		 */
+		int timeout = val2 / 10000;
+
+		if (val != 0 || timeout < 0 || timeout > 15 || val2 % 10000) {
+			ret = -EINVAL;
+			goto error_ret;
+		}
+
+		chip->thresh_timeout[rising][chan->channel] = timeout;
 		break;
+	}
 	default:
 		ret = -EINVAL;
 		goto error_ret;
@@ -338,91 +370,6 @@ static int ad7150_write_event_value(struct iio_dev *indio_dev,
 	return ret;
 }
 
-static ssize_t ad7150_show_timeout(struct device *dev,
-				   struct device_attribute *attr,
-				   char *buf)
-{
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct ad7150_chip_info *chip = iio_priv(indio_dev);
-	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
-	u8 value;
-
-	/* use the event code for consistency reasons */
-	int chan = IIO_EVENT_CODE_EXTRACT_CHAN(this_attr->address);
-	int rising = (IIO_EVENT_CODE_EXTRACT_DIR(this_attr->address)
-		      == IIO_EV_DIR_RISING) ? 1 : 0;
-
-	switch (IIO_EVENT_CODE_EXTRACT_TYPE(this_attr->address)) {
-	case IIO_EV_TYPE_THRESH_ADAPTIVE:
-		value = chip->thresh_timeout[rising][chan];
-		break;
-	default:
-		return -EINVAL;
-	}
-
-	return sprintf(buf, "%d\n", value);
-}
-
-static ssize_t ad7150_store_timeout(struct device *dev,
-				    struct device_attribute *attr,
-				    const char *buf,
-				    size_t len)
-{
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct ad7150_chip_info *chip = iio_priv(indio_dev);
-	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
-	int chan = IIO_EVENT_CODE_EXTRACT_CHAN(this_attr->address);
-	enum iio_event_direction dir;
-	enum iio_event_type type;
-	int rising;
-	u8 data;
-	int ret;
-
-	type = IIO_EVENT_CODE_EXTRACT_TYPE(this_attr->address);
-	dir = IIO_EVENT_CODE_EXTRACT_DIR(this_attr->address);
-	rising = (dir == IIO_EV_DIR_RISING);
-
-	ret = kstrtou8(buf, 10, &data);
-	if (ret < 0)
-		return ret;
-
-	if (data > GENMASK(3, 0))
-		return -EINVAL;
-
-	mutex_lock(&chip->state_lock);
-	switch (type) {
-	case IIO_EV_TYPE_THRESH_ADAPTIVE:
-		chip->thresh_timeout[rising][chan] = data;
-		break;
-	default:
-		ret = -EINVAL;
-		goto error_ret;
-	}
-
-	ret = ad7150_write_event_params(indio_dev, chan, type, dir);
-error_ret:
-	mutex_unlock(&chip->state_lock);
-
-	if (ret < 0)
-		return ret;
-
-	return len;
-}
-
-#define AD7150_TIMEOUT(chan, type, dir, ev_type, ev_dir)		\
-	IIO_DEVICE_ATTR(in_capacitance##chan##_##type##_##dir##_timeout, \
-		0644,							\
-		&ad7150_show_timeout,					\
-		&ad7150_store_timeout,					\
-		IIO_UNMOD_EVENT_CODE(IIO_CAPACITANCE,			\
-				     chan,				\
-				     IIO_EV_TYPE_##ev_type,		\
-				     IIO_EV_DIR_##ev_dir))
-static AD7150_TIMEOUT(0, thresh_adaptive, rising, THRESH_ADAPTIVE, RISING);
-static AD7150_TIMEOUT(0, thresh_adaptive, falling, THRESH_ADAPTIVE, FALLING);
-static AD7150_TIMEOUT(1, thresh_adaptive, rising, THRESH_ADAPTIVE, RISING);
-static AD7150_TIMEOUT(1, thresh_adaptive, falling, THRESH_ADAPTIVE, FALLING);
-
 static const struct iio_event_spec ad7150_events[] = {
 	{
 		.type = IIO_EV_TYPE_THRESH,
@@ -438,12 +385,14 @@ static const struct iio_event_spec ad7150_events[] = {
 		.type = IIO_EV_TYPE_THRESH_ADAPTIVE,
 		.dir = IIO_EV_DIR_RISING,
 		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
-			BIT(IIO_EV_INFO_ENABLE),
+			BIT(IIO_EV_INFO_ENABLE) |
+			BIT(IIO_EV_INFO_TIMEOUT),
 	}, {
 		.type = IIO_EV_TYPE_THRESH_ADAPTIVE,
 		.dir = IIO_EV_DIR_FALLING,
 		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
-			BIT(IIO_EV_INFO_ENABLE),
+			BIT(IIO_EV_INFO_ENABLE) |
+			BIT(IIO_EV_INFO_TIMEOUT),
 	},
 };
 
@@ -538,15 +487,11 @@ static irqreturn_t ad7150_event_handler(int irq, void *private)
 	return IRQ_HANDLED;
 }
 
-/* Timeouts not currently handled by core */
+static IIO_CONST_ATTR(in_capacitance_thresh_adaptive_timeout_available,
+		      "[0 0.01 0.15]");
+
 static struct attribute *ad7150_event_attributes[] = {
-	&iio_dev_attr_in_capacitance0_thresh_adaptive_rising_timeout
-	.dev_attr.attr,
-	&iio_dev_attr_in_capacitance0_thresh_adaptive_falling_timeout
-	.dev_attr.attr,
-	&iio_dev_attr_in_capacitance1_thresh_adaptive_rising_timeout
-	.dev_attr.attr,
-	&iio_dev_attr_in_capacitance1_thresh_adaptive_falling_timeout
+	&iio_const_attr_in_capacitance_thresh_adaptive_timeout_available
 	.dev_attr.attr,
 	NULL,
 };
-- 
2.30.2

[PATCH v2 12/24] staging:iio:cdc:ad7150: Rework interrupt handling.

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

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 <Jonathan.Cameron@huawei.com>
Link: https://lore.kernel.org/r/20210207154623.433442-13-jic23@kernel.org
---
 drivers/staging/iio/cdc/ad7150.c | 258 ++++++++++++++++++-------------
 1 file changed, 153 insertions(+), 105 deletions(-)

diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c
index f8183c540d5c..24be97456c03 100644
--- a/drivers/staging/iio/cdc/ad7150.c
+++ b/drivers/staging/iio/cdc/ad7150.c
@@ -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;
+			if (rising)
+				thresh_type = 0x1;
+			else
+				thresh_type = 0x0;
+			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;
+		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]);
+	}
+	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;
 
-	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) {
+		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

[PATCH v2 13/24] staging:iio:cdc:ad7150: More consistent register and field naming

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Add _REG postfix to register addresses to avoid confusion with
fields.  Also add additional field defines and use throughout the
driver in place of magic numbers.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Link: https://lore.kernel.org/r/20210207154623.433442-14-jic23@kernel.org
---
 drivers/staging/iio/cdc/ad7150.c | 129 +++++++++++++++----------------
 1 file changed, 61 insertions(+), 68 deletions(-)

diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c
index 24be97456c03..99125abb889b 100644
--- a/drivers/staging/iio/cdc/ad7150.c
+++ b/drivers/staging/iio/cdc/ad7150.c
@@ -21,37 +21,38 @@
  * AD7150 registers definition
  */
 
-#define AD7150_STATUS              0
-#define AD7150_STATUS_OUT1         BIT(3)
-#define AD7150_STATUS_OUT2         BIT(5)
-#define AD7150_CH1_DATA_HIGH       1
-#define AD7150_CH2_DATA_HIGH       3
-#define AD7150_CH1_AVG_HIGH        5
-#define AD7150_CH2_AVG_HIGH        7
-#define AD7150_CH1_SENSITIVITY     9
-#define AD7150_CH1_THR_HOLD_H      9
-#define AD7150_CH1_TIMEOUT         10
-#define AD7150_CH1_SETUP           11
-#define AD7150_CH2_SENSITIVITY     12
-#define AD7150_CH2_THR_HOLD_H      12
-#define AD7150_CH2_TIMEOUT         13
-#define AD7150_CH2_SETUP           14
-#define AD7150_CFG                 15
-#define AD7150_CFG_FIX             BIT(7)
-#define AD7150_PD_TIMER            16
-#define AD7150_CH1_CAPDAC          17
-#define AD7150_CH2_CAPDAC          18
-#define AD7150_SN3                 19
-#define AD7150_SN2                 20
-#define AD7150_SN1                 21
-#define AD7150_SN0                 22
-#define AD7150_ID                  23
-
-/* AD7150 masks */
-#define AD7150_THRESHTYPE_MSK			GENMASK(6, 5)
-
-#define AD7150_CH_TIMEOUT_RECEDING		GENMASK(3, 0)
-#define AD7150_CH_TIMEOUT_APPROACHING		GENMASK(7, 4)
+#define AD7150_STATUS_REG		0
+#define   AD7150_STATUS_OUT1		BIT(3)
+#define   AD7150_STATUS_OUT2		BIT(5)
+#define AD7150_CH1_DATA_HIGH_REG	1
+#define AD7150_CH2_DATA_HIGH_REG	3
+#define AD7150_CH1_AVG_HIGH_REG		5
+#define AD7150_CH2_AVG_HIGH_REG		7
+#define AD7150_CH1_SENSITIVITY_REG	9
+#define AD7150_CH1_THR_HOLD_H_REG	9
+#define AD7150_CH1_TIMEOUT_REG		10
+#define   AD7150_CH_TIMEOUT_RECEDING	GENMASK(3, 0)
+#define   AD7150_CH_TIMEOUT_APPROACHING	GENMASK(7, 4)
+#define AD7150_CH1_SETUP_REG		11
+#define AD7150_CH2_SENSITIVITY_REG	12
+#define AD7150_CH2_THR_HOLD_H_REG	12
+#define AD7150_CH2_TIMEOUT_REG		13
+#define AD7150_CH2_SETUP_REG		14
+#define AD7150_CFG_REG			15
+#define   AD7150_CFG_FIX		BIT(7)
+#define   AD7150_CFG_THRESHTYPE_MSK	GENMASK(6, 5)
+#define   AD7150_CFG_TT_NEG		0x0
+#define   AD7150_CFG_TT_POS		0x1
+#define   AD7150_CFG_TT_IN_WINDOW	0x2
+#define   AD7150_CFG_TT_OUT_WINDOW	0x3
+#define AD7150_PD_TIMER_REG		16
+#define AD7150_CH1_CAPDAC_REG		17
+#define AD7150_CH2_CAPDAC_REG		18
+#define AD7150_SN3_REG			19
+#define AD7150_SN2_REG			20
+#define AD7150_SN1_REG			21
+#define AD7150_SN0_REG			22
+#define AD7150_ID_REG			23
 
 enum {
 	AD7150,
@@ -93,12 +94,12 @@ struct ad7150_chip_info {
  */
 
 static const u8 ad7150_addresses[][6] = {
-	{ AD7150_CH1_DATA_HIGH, AD7150_CH1_AVG_HIGH,
-	  AD7150_CH1_SETUP, AD7150_CH1_THR_HOLD_H,
-	  AD7150_CH1_SENSITIVITY, AD7150_CH1_TIMEOUT },
-	{ AD7150_CH2_DATA_HIGH, AD7150_CH2_AVG_HIGH,
-	  AD7150_CH2_SETUP, AD7150_CH2_THR_HOLD_H,
-	  AD7150_CH2_SENSITIVITY, AD7150_CH2_TIMEOUT },
+	{ AD7150_CH1_DATA_HIGH_REG, AD7150_CH1_AVG_HIGH_REG,
+	  AD7150_CH1_SETUP_REG, AD7150_CH1_THR_HOLD_H_REG,
+	  AD7150_CH1_SENSITIVITY_REG, AD7150_CH1_TIMEOUT_REG },
+	{ AD7150_CH2_DATA_HIGH_REG, AD7150_CH2_AVG_HIGH_REG,
+	  AD7150_CH2_SETUP_REG, AD7150_CH2_THR_HOLD_H_REG,
+	  AD7150_CH2_SENSITIVITY_REG, AD7150_CH2_TIMEOUT_REG },
 };
 
 static int ad7150_read_raw(struct iio_dev *indio_dev,
@@ -147,11 +148,11 @@ static int ad7150_read_event_config(struct iio_dev *indio_dev,
 	bool thrfixed;
 	struct ad7150_chip_info *chip = iio_priv(indio_dev);
 
-	ret = i2c_smbus_read_byte_data(chip->client, AD7150_CFG);
+	ret = i2c_smbus_read_byte_data(chip->client, AD7150_CFG_REG);
 	if (ret < 0)
 		return ret;
 
-	threshtype = FIELD_GET(AD7150_THRESHTYPE_MSK, ret);
+	threshtype = FIELD_GET(AD7150_CFG_THRESHTYPE_MSK, ret);
 
 	/*check if threshold mode is fixed or adaptive*/
 	thrfixed = FIELD_GET(AD7150_CFG_FIX, ret);
@@ -159,12 +160,12 @@ 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 == 0x1);
-		return !thrfixed && (threshtype == 0x0);
+			return !thrfixed && (threshtype == AD7150_CFG_TT_POS);
+		return !thrfixed && (threshtype == AD7150_CFG_TT_NEG);
 	case IIO_EV_TYPE_THRESH:
 		if (dir == IIO_EV_DIR_RISING)
-			return thrfixed && (threshtype == 0x1);
-		return thrfixed && (threshtype == 0x0);
+			return thrfixed && (threshtype == AD7150_CFG_TT_POS);
+		return thrfixed && (threshtype == AD7150_CFG_TT_NEG);
 	default:
 		break;
 	}
@@ -226,7 +227,7 @@ static int ad7150_write_event_config(struct iio_dev *indio_dev,
 				     enum iio_event_type type,
 				     enum iio_event_direction dir, int state)
 {
-	u8 thresh_type, cfg, adaptive;
+	u8 thresh_type, cfg, fixed;
 	int ret;
 	struct ad7150_chip_info *chip = iio_priv(indio_dev);
 	int rising = (dir == IIO_EV_DIR_RISING);
@@ -261,35 +262,27 @@ static int ad7150_write_event_config(struct iio_dev *indio_dev,
 		disable_irq(chip->interrupts[0]);
 		disable_irq(chip->interrupts[1]);
 
-		ret = i2c_smbus_read_byte_data(chip->client, AD7150_CFG);
+		ret = i2c_smbus_read_byte_data(chip->client, AD7150_CFG_REG);
 		if (ret < 0)
 			goto error_ret;
 
-		cfg = ret & ~((0x03 << 5) | BIT(7));
+		cfg = ret & ~(AD7150_CFG_THRESHTYPE_MSK | AD7150_CFG_FIX);
 
-		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;
-			if (rising)
-				thresh_type = 0x1;
-			else
-				thresh_type = 0x0;
-			break;
-		default:
-			ret = -EINVAL;
-			goto error_ret;
-		}
+		if (type == IIO_EV_TYPE_THRESH_ADAPTIVE)
+			fixed = 0;
+		else
+			fixed = 1;
+
+		if (rising)
+			thresh_type = AD7150_CFG_TT_POS;
+		else
+			thresh_type = AD7150_CFG_TT_NEG;
 
-		cfg |= (!adaptive << 7) | (thresh_type << 5);
+		cfg |= FIELD_PREP(AD7150_CFG_FIX, fixed) |
+			FIELD_PREP(AD7150_CFG_THRESHTYPE_MSK, thresh_type);
 
-		ret = i2c_smbus_write_byte_data(chip->client, AD7150_CFG, cfg);
+		ret = i2c_smbus_write_byte_data(chip->client, AD7150_CFG_REG,
+						cfg);
 		if (ret < 0)
 			goto error_ret;
 
@@ -480,7 +473,7 @@ static irqreturn_t __ad7150_event_handler(void *private, u8 status_mask,
 	s64 timestamp = iio_get_time_ns(indio_dev);
 	int int_status;
 
-	int_status = i2c_smbus_read_byte_data(chip->client, AD7150_STATUS);
+	int_status = i2c_smbus_read_byte_data(chip->client, AD7150_STATUS_REG);
 	if (int_status < 0)
 		return IRQ_HANDLED;
 
-- 
2.30.2

[PATCH v2 14/24] staging:iio:cdc:ad7150: Reorganize headers.

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Whilst not important, it's nice to have the general headers in
alphabetical order.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Barry Song <song.bao.hua@hisilicon.com>
Reviewed-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Link: https://lore.kernel.org/r/20210207154623.433442-15-jic23@kernel.org
---
 drivers/staging/iio/cdc/ad7150.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c
index 99125abb889b..8131fbb56d37 100644
--- a/drivers/staging/iio/cdc/ad7150.c
+++ b/drivers/staging/iio/cdc/ad7150.c
@@ -6,13 +6,13 @@
  */
 
 #include <linux/bitfield.h>
-#include <linux/interrupt.h>
 #include <linux/device.h>
-#include <linux/kernel.h>
-#include <linux/slab.h>
-#include <linux/i2c.h>
+#include <linux/interrupt.h>
 #include <linux/irq.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/slab.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
-- 
2.30.2

[PATCH v2 15/24] staging:iio:cdc:ad7150: Tidy up local variable positioning.

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Where there is no other basis on which to order declarations
let us prefer reverse xmas tree.  Also reduce scope where
sensible.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Barry Song <song.bao.hua@hisilicon.com>
Reviewed-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Link: https://lore.kernel.org/r/20210207154623.433442-16-jic23@kernel.org
---
 drivers/staging/iio/cdc/ad7150.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c
index 8131fbb56d37..1bfa71d7ca1d 100644
--- a/drivers/staging/iio/cdc/ad7150.c
+++ b/drivers/staging/iio/cdc/ad7150.c
@@ -108,9 +108,9 @@ static int ad7150_read_raw(struct iio_dev *indio_dev,
 			   int *val2,
 			   long mask)
 {
-	int ret;
 	struct ad7150_chip_info *chip = iio_priv(indio_dev);
 	int channel = chan->channel;
+	int ret;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
@@ -143,10 +143,10 @@ static int ad7150_read_event_config(struct iio_dev *indio_dev,
 				    enum iio_event_type type,
 				    enum iio_event_direction dir)
 {
-	int ret;
+	struct ad7150_chip_info *chip = iio_priv(indio_dev);
 	u8 threshtype;
 	bool thrfixed;
-	struct ad7150_chip_info *chip = iio_priv(indio_dev);
+	int ret;
 
 	ret = i2c_smbus_read_byte_data(chip->client, AD7150_CFG_REG);
 	if (ret < 0)
@@ -227,10 +227,8 @@ static int ad7150_write_event_config(struct iio_dev *indio_dev,
 				     enum iio_event_type type,
 				     enum iio_event_direction dir, int state)
 {
-	u8 thresh_type, cfg, fixed;
-	int ret;
 	struct ad7150_chip_info *chip = iio_priv(indio_dev);
-	int rising = (dir == IIO_EV_DIR_RISING);
+	int ret;
 
 	/*
 	 * There is only a single shared control and no on chip
@@ -251,6 +249,8 @@ static int ad7150_write_event_config(struct iio_dev *indio_dev,
 
 	mutex_lock(&chip->state_lock);
 	if ((type != chip->type) || (dir != chip->dir)) {
+		int rising = (dir == IIO_EV_DIR_RISING);
+		u8 thresh_type, cfg, fixed;
 
 		/*
 		 * Need to temporarily disable both interrupts if
@@ -533,9 +533,9 @@ static const struct iio_info ad7150_info_no_irq = {
 static int ad7150_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
-	int ret;
 	struct ad7150_chip_info *chip;
 	struct iio_dev *indio_dev;
+	int ret;
 
 	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
 	if (!indio_dev)
-- 
2.30.2

[PATCH v2 16/24] staging:iio:cdc:ad7150: Drop unnecessary block comments.

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

These have a habit of not getting updated with driver reorganizations
and don't add much info so drop them.

Also fix a minor comment syntax issue.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Barry Song <song.bao.hua@hisilicon.com>
Reviewed-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Link: https://lore.kernel.org/r/20210207154623.433442-17-jic23@kernel.org
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/staging/iio/cdc/ad7150.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c
index 1bfa71d7ca1d..67af39f80650 100644
--- a/drivers/staging/iio/cdc/ad7150.c
+++ b/drivers/staging/iio/cdc/ad7150.c
@@ -17,9 +17,6 @@
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
 #include <linux/iio/events.h>
-/*
- * AD7150 registers definition
- */
 
 #define AD7150_STATUS_REG		0
 #define   AD7150_STATUS_OUT1		BIT(3)
@@ -89,10 +86,6 @@ struct ad7150_chip_info {
 	enum iio_event_direction dir;
 };
 
-/*
- * sysfs nodes
- */
-
 static const u8 ad7150_addresses[][6] = {
 	{ AD7150_CH1_DATA_HIGH_REG, AD7150_CH1_AVG_HIGH_REG,
 	  AD7150_CH1_SETUP_REG, AD7150_CH1_THR_HOLD_H_REG,
@@ -172,8 +165,7 @@ static int ad7150_read_event_config(struct iio_dev *indio_dev,
 	return -EINVAL;
 }
 
-/* state_lock should be held to ensure consistent state*/
-
+/* state_lock should be held to ensure consistent state */
 static int ad7150_write_event_params(struct iio_dev *indio_dev,
 				     unsigned int chan,
 				     enum iio_event_type type,
-- 
2.30.2

[PATCH v2 17/24] staging:iio:cdc:ad7150: Shift the _raw readings by 4 bits.

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Every other register related to raw value on the datasheet is
described as correpsonding to the 12MSB of the actual
data registers + the bottom 4 bits are 0.  So lets treat this
as what it actually is, which is a 12 bit value.
Note that we will have to be a little careful to compensate for
the offset and scale values.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Link: https://lore.kernel.org/r/20210207154623.433442-18-jic23@kernel.org
---
 drivers/staging/iio/cdc/ad7150.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c
index 67af39f80650..1a041396ff9e 100644
--- a/drivers/staging/iio/cdc/ad7150.c
+++ b/drivers/staging/iio/cdc/ad7150.c
@@ -111,7 +111,7 @@ static int ad7150_read_raw(struct iio_dev *indio_dev,
 						  ad7150_addresses[channel][0]);
 		if (ret < 0)
 			return ret;
-		*val = ret;
+		*val = ret >> 4;
 
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_AVERAGE_RAW:
-- 
2.30.2

[PATCH v2 18/24] staging:iio:cdc:ad7150: Add scale and offset to info_mask_shared_by_type

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

The datasheet provides these two values on the assumption they are applied
to unshift raw value.  Hence shift both the offset and scale by 4
to compensate.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Link: https://lore.kernel.org/r/20210207154623.433442-19-jic23@kernel.org
---
 drivers/staging/iio/cdc/ad7150.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c
index 1a041396ff9e..39d70805d2a6 100644
--- a/drivers/staging/iio/cdc/ad7150.c
+++ b/drivers/staging/iio/cdc/ad7150.c
@@ -121,6 +121,18 @@ static int ad7150_read_raw(struct iio_dev *indio_dev,
 			return ret;
 		*val = ret;
 
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		/*
+		 * Base units for capacitance are nano farads and the value
+		 * calculated from the datasheet formula is in picofarad
+		 * so multiply by 1000
+		 */
+		*val = 1000;
+		*val2 = 40944 >> 4; /* To match shift in _RAW */
+		return IIO_VAL_FRACTIONAL;
+	case IIO_CHAN_INFO_OFFSET:
+		*val = -(12288 >> 4); /* To match shift in _RAW */
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		/* Strangely same for both 1 and 2 chan parts */
@@ -425,6 +437,8 @@ static const struct iio_event_spec ad7150_events[] = {
 		.channel = _chan,				\
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |	\
 		BIT(IIO_CHAN_INFO_AVERAGE_RAW),			\
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \
+			BIT(IIO_CHAN_INFO_OFFSET),		\
 		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),\
 		.event_spec = ad7150_events,			\
 		.num_event_specs = ARRAY_SIZE(ad7150_events),	\
@@ -436,6 +450,8 @@ static const struct iio_event_spec ad7150_events[] = {
 		.channel = _chan,				\
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |	\
 		BIT(IIO_CHAN_INFO_AVERAGE_RAW),			\
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \
+			BIT(IIO_CHAN_INFO_OFFSET),		\
 		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),\
 	}
 
-- 
2.30.2

[PATCH v2 19/24] staging:iio:cdc:ad7150: Really basic regulator support.

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Given DT docs will include regulators, lets just turn them on and
off with driver probe() and remove().

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Link: https://lore.kernel.org/r/20210207154623.433442-20-jic23@kernel.org
---
 drivers/staging/iio/cdc/ad7150.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c
index 39d70805d2a6..30abfacda074 100644
--- a/drivers/staging/iio/cdc/ad7150.c
+++ b/drivers/staging/iio/cdc/ad7150.c
@@ -12,6 +12,7 @@
 #include <linux/i2c.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/regulator/consumer.h>
 #include <linux/slab.h>
 
 #include <linux/iio/iio.h>
@@ -538,11 +539,19 @@ static const struct iio_info ad7150_info_no_irq = {
 	.read_raw = &ad7150_read_raw,
 };
 
+static void ad7150_reg_disable(void *data)
+{
+	struct regulator *reg = data;
+
+	regulator_disable(reg);
+}
+
 static int ad7150_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
 	struct ad7150_chip_info *chip;
 	struct iio_dev *indio_dev;
+	struct regulator *reg;
 	int ret;
 
 	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
@@ -557,6 +566,18 @@ static int ad7150_probe(struct i2c_client *client,
 
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
+	reg = devm_regulator_get(&client->dev, "vdd");
+	if (IS_ERR(reg))
+		return PTR_ERR(reg);
+
+	ret = regulator_enable(reg);
+	if (ret)
+		return ret;
+
+	ret = devm_add_action_or_reset(&client->dev, ad7150_reg_disable, reg);
+	if (ret)
+		return ret;
+
 	chip->interrupts[0] = fwnode_irq_get(dev_fwnode(&client->dev), 0);
 	if (chip->interrupts[0] < 0)
 		return chip->interrupts[0];
-- 
2.30.2

[PATCH v2 20/24] staging:iio:cdc:ad7150: Add of_match_table

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

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 <Jonathan.Cameron@huawei.com>
Link: https://lore.kernel.org/r/20210207154623.433442-21-jic23@kernel.org
---
 drivers/staging/iio/cdc/ad7150.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c
index 30abfacda074..0afad49bcddc 100644
--- a/drivers/staging/iio/cdc/ad7150.c
+++ b/drivers/staging/iio/cdc/ad7150.c
@@ -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

[PATCH v2 21/24] iio:Documentation:ABI Add missing elements as used by the adi,ad7150

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

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 <Jonathan.Cameron@huawei.com>
Link: https://lore.kernel.org/r/20210207154623.433442-23-jic23@kernel.org
---
 Documentation/ABI/testing/sysfs-bus-iio | 33 +++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
index affd4ce871d7..32d26c118141 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -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:	linux-iio@vger.kernel.org
 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:	linux-iio@vger.kernel.org
 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:	linux-iio@vger.kernel.org
+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:	linux-iio@vger.kernel.org
+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:	linux-iio@vger.kernel.org
 Description:
-- 
2.30.2

[PATCH v2 22/24] staging:iio:cdc:ad7150: Add copyright notice given substantial changes.

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

It seems to me that the changes made to get this ready to move out of
staging are substantial enough to warant a copyright notice addition.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Link: https://lore.kernel.org/r/20210207154623.433442-24-jic23@kernel.org
---
 drivers/staging/iio/cdc/ad7150.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c
index 0afad49bcddc..b6d2c02fa39d 100644
--- a/drivers/staging/iio/cdc/ad7150.c
+++ b/drivers/staging/iio/cdc/ad7150.c
@@ -3,6 +3,7 @@
  * AD7150 capacitive sensor driver supporting AD7150/1/6
  *
  * Copyright 2010-2011 Analog Devices Inc.
+ * Copyright 2021 Jonathan Cameron <Jonathan.Cameron@huawei.com>
  */
 
 #include <linux/bitfield.h>
-- 
2.30.2

[PATCH v2 23/24] dt-bindings:iio:cdc:adi,ad7150 binding doc

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

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 <Jonathan.Cameron@huawei.com>
Cc: Robh+dt@kernel.org
Cc: devicetree@vger.kernel.org
Link: https://lore.kernel.org/r/20210207154623.433442-22-jic23@kernel.org
---
 .../bindings/iio/cdc/adi,ad7150.yaml          | 69 +++++++++++++++++++
 1 file changed, 69 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/cdc/adi,ad7150.yaml b/Documentation/devicetree/bindings/iio/cdc/adi,ad7150.yaml
new file mode 100644
index 000000000000..2155d3f5666c
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/cdc/adi,ad7150.yaml
@@ -0,0 +1,69 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/cdc/adi,ad7150.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog device AD7150 and similar capacitance to digital convertors.
+
+maintainers:
+  - Jonathan Cameron <jic23@kernel.org>
+
+properties:
+  compatible:
+    enum:
+      - adi,ad7150
+      - adi,ad7151
+      - adi,ad7156
+
+  reg:
+    maxItems: 1
+
+  vdd-supply: true
+
+  interrupts: true
+
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - adi,ad7150
+              - adi,ad7156
+    then:
+      properties:
+        interrupts:
+          minItems: 2
+          maxItems: 2
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: adi,ad7151
+    then:
+      properties:
+        interrupts:
+          minItems: 1
+          maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        cdc@48 {
+            compatible = "adi,ad7150";
+            reg = <0x48>;
+            interrupts = <25 2>, <26 2>;
+            interrupt-parent = <&gpio>;
+        };
+    };
+...
-- 
2.30.2

[PATCH v2 24/24] iio:cdc:ad7150: Move driver out of staging.

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

This capacitance to digital converter (CDC) driver is compliant with
the IIO ABI.  Note, not all features supported (e.g. window event modes)
but the driver should be in a useful functional state.

The cleanup was done against QEMU emulation of the device rather than
actual hardware.   Whilst this was a bit of an experiment, it made it
easy to confirm that the driver remained in a consistent working state
through the various refactors.  If it worked in the first place, it
should still be working after this cleanup.

Given some IIO drivers require expensive hardware setups, (not particularly
true with this one) the use of QEMU may provide a viable way forward
for providing testing during code changes where previously we'd had
to rely on sharp eyes and crossed fingers.

Note, no explicit MAINTAINERS entry as it will be covered by the
generic catch-alls for ADI and IIO drivers which are sufficient.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Link: https://lore.kernel.org/r/20210207154623.433442-25-jic23@kernel.org
---
 drivers/iio/Kconfig              |   1 +
 drivers/iio/Makefile             |   1 +
 drivers/iio/cdc/Kconfig          |  17 +
 drivers/iio/cdc/Makefile         |   6 +
 drivers/iio/cdc/ad7150.c         | 678 +++++++++++++++++++++++++++++++
 drivers/staging/iio/cdc/Kconfig  |  10 -
 drivers/staging/iio/cdc/Makefile |   3 +-
 drivers/staging/iio/cdc/ad7150.c | 678 -------------------------------
 8 files changed, 704 insertions(+), 690 deletions(-)

diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
index b35e0c33b5e2..2334ad249b46 100644
--- a/drivers/iio/Kconfig
+++ b/drivers/iio/Kconfig
@@ -72,6 +72,7 @@ source "drivers/iio/accel/Kconfig"
 source "drivers/iio/adc/Kconfig"
 source "drivers/iio/afe/Kconfig"
 source "drivers/iio/amplifiers/Kconfig"
+source "drivers/iio/cdc/Kconfig"
 source "drivers/iio/chemical/Kconfig"
 source "drivers/iio/common/Kconfig"
 source "drivers/iio/dac/Kconfig"
diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
index 2561325aaa74..65e39bd4f934 100644
--- a/drivers/iio/Makefile
+++ b/drivers/iio/Makefile
@@ -18,6 +18,7 @@ obj-y += adc/
 obj-y += afe/
 obj-y += amplifiers/
 obj-y += buffer/
+obj-y += cdc/
 obj-y += chemical/
 obj-y += common/
 obj-y += dac/
diff --git a/drivers/iio/cdc/Kconfig b/drivers/iio/cdc/Kconfig
new file mode 100644
index 000000000000..5e3319a3ff48
--- /dev/null
+++ b/drivers/iio/cdc/Kconfig
@@ -0,0 +1,17 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# CDC drivers
+#
+menu "Capacitance to digital converters"
+
+config AD7150
+	tristate "Analog Devices ad7150/1/6 capacitive sensor driver"
+	depends on I2C
+	help
+	  Say yes here to build support for Analog Devices capacitive sensors.
+	  (ad7150, ad7151, ad7156) Provides direct access via sysfs.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called ad7150.
+
+endmenu
diff --git a/drivers/iio/cdc/Makefile b/drivers/iio/cdc/Makefile
new file mode 100644
index 000000000000..ee490637b032
--- /dev/null
+++ b/drivers/iio/cdc/Makefile
@@ -0,0 +1,6 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Makefile for industrial I/O capacitance to digital converter (CDC) drivers
+#
+
+obj-$(CONFIG_AD7150) += ad7150.o
diff --git a/drivers/iio/cdc/ad7150.c b/drivers/iio/cdc/ad7150.c
new file mode 100644
index 000000000000..b6d2c02fa39d
--- /dev/null
+++ b/drivers/iio/cdc/ad7150.c
@@ -0,0 +1,678 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * AD7150 capacitive sensor driver supporting AD7150/1/6
+ *
+ * Copyright 2010-2011 Analog Devices Inc.
+ * Copyright 2021 Jonathan Cameron <Jonathan.Cameron@huawei.com>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/device.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#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>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/events.h>
+
+#define AD7150_STATUS_REG		0
+#define   AD7150_STATUS_OUT1		BIT(3)
+#define   AD7150_STATUS_OUT2		BIT(5)
+#define AD7150_CH1_DATA_HIGH_REG	1
+#define AD7150_CH2_DATA_HIGH_REG	3
+#define AD7150_CH1_AVG_HIGH_REG		5
+#define AD7150_CH2_AVG_HIGH_REG		7
+#define AD7150_CH1_SENSITIVITY_REG	9
+#define AD7150_CH1_THR_HOLD_H_REG	9
+#define AD7150_CH1_TIMEOUT_REG		10
+#define   AD7150_CH_TIMEOUT_RECEDING	GENMASK(3, 0)
+#define   AD7150_CH_TIMEOUT_APPROACHING	GENMASK(7, 4)
+#define AD7150_CH1_SETUP_REG		11
+#define AD7150_CH2_SENSITIVITY_REG	12
+#define AD7150_CH2_THR_HOLD_H_REG	12
+#define AD7150_CH2_TIMEOUT_REG		13
+#define AD7150_CH2_SETUP_REG		14
+#define AD7150_CFG_REG			15
+#define   AD7150_CFG_FIX		BIT(7)
+#define   AD7150_CFG_THRESHTYPE_MSK	GENMASK(6, 5)
+#define   AD7150_CFG_TT_NEG		0x0
+#define   AD7150_CFG_TT_POS		0x1
+#define   AD7150_CFG_TT_IN_WINDOW	0x2
+#define   AD7150_CFG_TT_OUT_WINDOW	0x3
+#define AD7150_PD_TIMER_REG		16
+#define AD7150_CH1_CAPDAC_REG		17
+#define AD7150_CH2_CAPDAC_REG		18
+#define AD7150_SN3_REG			19
+#define AD7150_SN2_REG			20
+#define AD7150_SN1_REG			21
+#define AD7150_SN0_REG			22
+#define AD7150_ID_REG			23
+
+enum {
+	AD7150,
+	AD7151,
+};
+
+/**
+ * struct ad7150_chip_info - instance specific chip data
+ * @client: i2c client for this device
+ * @threshold: thresholds for simple capacitance value events
+ * @thresh_sensitivity: threshold for simple capacitance offset
+ *	from 'average' value.
+ * @thresh_timeout: a timeout, in samples from the moment an
+ *	adaptive threshold event occurs to when the average
+ *	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
+ * @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;
+	u16 threshold[2][2];
+	u8 thresh_sensitivity[2][2];
+	u8 thresh_timeout[2][2];
+	struct mutex state_lock;
+	int interrupts[2];
+	bool int_enabled[2];
+	enum iio_event_type type;
+	enum iio_event_direction dir;
+};
+
+static const u8 ad7150_addresses[][6] = {
+	{ AD7150_CH1_DATA_HIGH_REG, AD7150_CH1_AVG_HIGH_REG,
+	  AD7150_CH1_SETUP_REG, AD7150_CH1_THR_HOLD_H_REG,
+	  AD7150_CH1_SENSITIVITY_REG, AD7150_CH1_TIMEOUT_REG },
+	{ AD7150_CH2_DATA_HIGH_REG, AD7150_CH2_AVG_HIGH_REG,
+	  AD7150_CH2_SETUP_REG, AD7150_CH2_THR_HOLD_H_REG,
+	  AD7150_CH2_SENSITIVITY_REG, AD7150_CH2_TIMEOUT_REG },
+};
+
+static int ad7150_read_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan,
+			   int *val,
+			   int *val2,
+			   long mask)
+{
+	struct ad7150_chip_info *chip = iio_priv(indio_dev);
+	int channel = chan->channel;
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		ret = i2c_smbus_read_word_swapped(chip->client,
+						  ad7150_addresses[channel][0]);
+		if (ret < 0)
+			return ret;
+		*val = ret >> 4;
+
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_AVERAGE_RAW:
+		ret = i2c_smbus_read_word_swapped(chip->client,
+						  ad7150_addresses[channel][1]);
+		if (ret < 0)
+			return ret;
+		*val = ret;
+
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		/*
+		 * Base units for capacitance are nano farads and the value
+		 * calculated from the datasheet formula is in picofarad
+		 * so multiply by 1000
+		 */
+		*val = 1000;
+		*val2 = 40944 >> 4; /* To match shift in _RAW */
+		return IIO_VAL_FRACTIONAL;
+	case IIO_CHAN_INFO_OFFSET:
+		*val = -(12288 >> 4); /* To match shift in _RAW */
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		/* Strangely same for both 1 and 2 chan parts */
+		*val = 100;
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ad7150_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 ad7150_chip_info *chip = iio_priv(indio_dev);
+	u8 threshtype;
+	bool thrfixed;
+	int ret;
+
+	ret = i2c_smbus_read_byte_data(chip->client, AD7150_CFG_REG);
+	if (ret < 0)
+		return ret;
+
+	threshtype = FIELD_GET(AD7150_CFG_THRESHTYPE_MSK, ret);
+
+	/*check if threshold mode is fixed or adaptive*/
+	thrfixed = FIELD_GET(AD7150_CFG_FIX, ret);
+
+	switch (type) {
+	case IIO_EV_TYPE_THRESH_ADAPTIVE:
+		if (dir == IIO_EV_DIR_RISING)
+			return !thrfixed && (threshtype == AD7150_CFG_TT_POS);
+		return !thrfixed && (threshtype == AD7150_CFG_TT_NEG);
+	case IIO_EV_TYPE_THRESH:
+		if (dir == IIO_EV_DIR_RISING)
+			return thrfixed && (threshtype == AD7150_CFG_TT_POS);
+		return thrfixed && (threshtype == AD7150_CFG_TT_NEG);
+	default:
+		break;
+	}
+	return -EINVAL;
+}
+
+/* state_lock should be held to ensure consistent state */
+static int ad7150_write_event_params(struct iio_dev *indio_dev,
+				     unsigned int chan,
+				     enum iio_event_type type,
+				     enum iio_event_direction dir)
+{
+	struct ad7150_chip_info *chip = iio_priv(indio_dev);
+	int rising = (dir == IIO_EV_DIR_RISING);
+
+	/* Only update value live, if parameter is in use */
+	if ((type != chip->type) || (dir != chip->dir))
+		return 0;
+
+	switch (type) {
+		/* Note completely different from the adaptive versions */
+	case IIO_EV_TYPE_THRESH: {
+		u16 value = chip->threshold[rising][chan];
+		return i2c_smbus_write_word_swapped(chip->client,
+						    ad7150_addresses[chan][3],
+						    value);
+	}
+	case IIO_EV_TYPE_THRESH_ADAPTIVE: {
+		int ret;
+		u8 sens, timeout;
+
+		sens = chip->thresh_sensitivity[rising][chan];
+		ret = i2c_smbus_write_byte_data(chip->client,
+						ad7150_addresses[chan][4],
+						sens);
+		if (ret)
+			return ret;
+
+		/*
+		 * Single timeout register contains timeouts for both
+		 * directions.
+		 */
+		timeout = FIELD_PREP(AD7150_CH_TIMEOUT_APPROACHING,
+				     chip->thresh_timeout[1][chan]);
+		timeout |= FIELD_PREP(AD7150_CH_TIMEOUT_RECEDING,
+				      chip->thresh_timeout[0][chan]);
+		return i2c_smbus_write_byte_data(chip->client,
+						 ad7150_addresses[chan][5],
+						 timeout);
+	}
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ad7150_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 ad7150_chip_info *chip = iio_priv(indio_dev);
+	int ret;
+
+	/*
+	 * 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);
+	if ((type != chip->type) || (dir != chip->dir)) {
+		int rising = (dir == IIO_EV_DIR_RISING);
+		u8 thresh_type, cfg, fixed;
+
+		/*
+		 * 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]);
+
+		ret = i2c_smbus_read_byte_data(chip->client, AD7150_CFG_REG);
+		if (ret < 0)
+			goto error_ret;
+
+		cfg = ret & ~(AD7150_CFG_THRESHTYPE_MSK | AD7150_CFG_FIX);
+
+		if (type == IIO_EV_TYPE_THRESH_ADAPTIVE)
+			fixed = 0;
+		else
+			fixed = 1;
+
+		if (rising)
+			thresh_type = AD7150_CFG_TT_POS;
+		else
+			thresh_type = AD7150_CFG_TT_NEG;
+
+		cfg |= FIELD_PREP(AD7150_CFG_FIX, fixed) |
+			FIELD_PREP(AD7150_CFG_THRESHTYPE_MSK, thresh_type);
+
+		ret = i2c_smbus_write_byte_data(chip->client, AD7150_CFG_REG,
+						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;
+		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]);
+	}
+	if (!chip->int_enabled[chan->channel]) {
+		enable_irq(chip->interrupts[chan->channel]);
+		chip->int_enabled[chan->channel] = true;
+	}
+
+error_ret:
+	mutex_unlock(&chip->state_lock);
+
+	return ret;
+}
+
+static int ad7150_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 ad7150_chip_info *chip = iio_priv(indio_dev);
+	int rising = (dir == IIO_EV_DIR_RISING);
+
+	/* Complex register sharing going on here */
+	switch (info) {
+	case IIO_EV_INFO_VALUE:
+		switch (type) {
+		case IIO_EV_TYPE_THRESH_ADAPTIVE:
+			*val = chip->thresh_sensitivity[rising][chan->channel];
+			return IIO_VAL_INT;
+		case IIO_EV_TYPE_THRESH:
+			*val = chip->threshold[rising][chan->channel];
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+		}
+	case IIO_EV_INFO_TIMEOUT:
+		*val = 0;
+		*val2 = chip->thresh_timeout[rising][chan->channel] * 10000;
+		return IIO_VAL_INT_PLUS_MICRO;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ad7150_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)
+{
+	int ret;
+	struct ad7150_chip_info *chip = iio_priv(indio_dev);
+	int rising = (dir == IIO_EV_DIR_RISING);
+
+	mutex_lock(&chip->state_lock);
+	switch (info) {
+	case IIO_EV_INFO_VALUE:
+		switch (type) {
+		case IIO_EV_TYPE_THRESH_ADAPTIVE:
+			chip->thresh_sensitivity[rising][chan->channel] = val;
+			break;
+		case IIO_EV_TYPE_THRESH:
+			chip->threshold[rising][chan->channel] = val;
+			break;
+		default:
+			ret = -EINVAL;
+			goto error_ret;
+		}
+		break;
+	case IIO_EV_INFO_TIMEOUT: {
+		/*
+		 * Raw timeout is in cycles of 10 msecs as long as both
+		 * channels are enabled.
+		 * In terms of INT_PLUS_MICRO, that is in units of 10,000
+		 */
+		int timeout = val2 / 10000;
+
+		if (val != 0 || timeout < 0 || timeout > 15 || val2 % 10000) {
+			ret = -EINVAL;
+			goto error_ret;
+		}
+
+		chip->thresh_timeout[rising][chan->channel] = timeout;
+		break;
+	}
+	default:
+		ret = -EINVAL;
+		goto error_ret;
+	}
+
+	/* write back if active */
+	ret = ad7150_write_event_params(indio_dev, chan->channel, type, dir);
+
+error_ret:
+	mutex_unlock(&chip->state_lock);
+	return ret;
+}
+
+static const struct iio_event_spec ad7150_events[] = {
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_RISING,
+		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
+			BIT(IIO_EV_INFO_ENABLE),
+	}, {
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_FALLING,
+		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
+			BIT(IIO_EV_INFO_ENABLE),
+	}, {
+		.type = IIO_EV_TYPE_THRESH_ADAPTIVE,
+		.dir = IIO_EV_DIR_RISING,
+		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
+			BIT(IIO_EV_INFO_ENABLE) |
+			BIT(IIO_EV_INFO_TIMEOUT),
+	}, {
+		.type = IIO_EV_TYPE_THRESH_ADAPTIVE,
+		.dir = IIO_EV_DIR_FALLING,
+		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
+			BIT(IIO_EV_INFO_ENABLE) |
+			BIT(IIO_EV_INFO_TIMEOUT),
+	},
+};
+
+#define AD7150_CAPACITANCE_CHAN(_chan)	{			\
+		.type = IIO_CAPACITANCE,			\
+		.indexed = 1,					\
+		.channel = _chan,				\
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |	\
+		BIT(IIO_CHAN_INFO_AVERAGE_RAW),			\
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \
+			BIT(IIO_CHAN_INFO_OFFSET),		\
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),\
+		.event_spec = ad7150_events,			\
+		.num_event_specs = ARRAY_SIZE(ad7150_events),	\
+	}
+
+#define AD7150_CAPACITANCE_CHAN_NO_IRQ(_chan)	{		\
+		.type = IIO_CAPACITANCE,			\
+		.indexed = 1,					\
+		.channel = _chan,				\
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |	\
+		BIT(IIO_CHAN_INFO_AVERAGE_RAW),			\
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \
+			BIT(IIO_CHAN_INFO_OFFSET),		\
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),\
+	}
+
+static const struct iio_chan_spec ad7150_channels[] = {
+	AD7150_CAPACITANCE_CHAN(0),
+	AD7150_CAPACITANCE_CHAN(1),
+};
+
+static const struct iio_chan_spec ad7150_channels_no_irq[] = {
+	AD7150_CAPACITANCE_CHAN_NO_IRQ(0),
+	AD7150_CAPACITANCE_CHAN_NO_IRQ(1),
+};
+
+static const struct iio_chan_spec ad7151_channels[] = {
+	AD7150_CAPACITANCE_CHAN(0),
+};
+
+static const struct iio_chan_spec ad7151_channels_no_irq[] = {
+	AD7150_CAPACITANCE_CHAN_NO_IRQ(0),
+};
+
+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);
+	s64 timestamp = iio_get_time_ns(indio_dev);
+	int int_status;
+
+	int_status = i2c_smbus_read_byte_data(chip->client, AD7150_STATUS_REG);
+	if (int_status < 0)
+		return IRQ_HANDLED;
+
+	/*
+	 * 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]");
+
+static struct attribute *ad7150_event_attributes[] = {
+	&iio_const_attr_in_capacitance_thresh_adaptive_timeout_available
+	.dev_attr.attr,
+	NULL,
+};
+
+static const struct attribute_group ad7150_event_attribute_group = {
+	.attrs = ad7150_event_attributes,
+	.name = "events",
+};
+
+static const struct iio_info ad7150_info = {
+	.event_attrs = &ad7150_event_attribute_group,
+	.read_raw = &ad7150_read_raw,
+	.read_event_config = &ad7150_read_event_config,
+	.write_event_config = &ad7150_write_event_config,
+	.read_event_value = &ad7150_read_event_value,
+	.write_event_value = &ad7150_write_event_value,
+};
+
+static const struct iio_info ad7150_info_no_irq = {
+	.read_raw = &ad7150_read_raw,
+};
+
+static void ad7150_reg_disable(void *data)
+{
+	struct regulator *reg = data;
+
+	regulator_disable(reg);
+}
+
+static int ad7150_probe(struct i2c_client *client,
+			const struct i2c_device_id *id)
+{
+	struct ad7150_chip_info *chip;
+	struct iio_dev *indio_dev;
+	struct regulator *reg;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	chip = iio_priv(indio_dev);
+	mutex_init(&chip->state_lock);
+	chip->client = client;
+
+	indio_dev->name = id->name;
+
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	reg = devm_regulator_get(&client->dev, "vdd");
+	if (IS_ERR(reg))
+		return PTR_ERR(reg);
+
+	ret = regulator_enable(reg);
+	if (ret)
+		return ret;
+
+	ret = devm_add_action_or_reset(&client->dev, ad7150_reg_disable, reg);
+	if (ret)
+		return ret;
+
+	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) {
+		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;
+			indio_dev->num_channels = ARRAY_SIZE(ad7151_channels);
+			break;
+		default:
+			return -EINVAL;
+		}
+
+	} 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);
+			break;
+		case AD7151:
+			indio_dev->channels = ad7151_channels_no_irq;
+			indio_dev->num_channels =
+				ARRAY_SIZE(ad7151_channels_no_irq);
+			break;
+		default:
+			return -EINVAL;
+		}
+	}
+
+	return devm_iio_device_register(indio_dev->dev.parent, indio_dev);
+}
+
+static const struct i2c_device_id ad7150_id[] = {
+	{ "ad7150", AD7150 },
+	{ "ad7151", AD7151 },
+	{ "ad7156", AD7150 },
+	{}
+};
+
+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,
+};
+module_i2c_driver(ad7150_driver);
+
+MODULE_AUTHOR("Barry Song <21cnbao@gmail.com>");
+MODULE_DESCRIPTION("Analog Devices AD7150/1/6 capacitive sensor driver");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/staging/iio/cdc/Kconfig b/drivers/staging/iio/cdc/Kconfig
index e0a5ce66a984..a7386bbbcb79 100644
--- a/drivers/staging/iio/cdc/Kconfig
+++ b/drivers/staging/iio/cdc/Kconfig
@@ -4,16 +4,6 @@
 #
 menu "Capacitance to digital converters"
 
-config AD7150
-	tristate "Analog Devices ad7150/1/6 capacitive sensor driver"
-	depends on I2C
-	help
-	  Say yes here to build support for Analog Devices capacitive sensors.
-	  (ad7150, ad7151, ad7156) Provides direct access via sysfs.
-
-	  To compile this driver as a module, choose M here: the
-	  module will be called ad7150.
-
 config AD7746
 	tristate "Analog Devices AD7745, AD7746 AD7747 capacitive sensor driver"
 	depends on I2C
diff --git a/drivers/staging/iio/cdc/Makefile b/drivers/staging/iio/cdc/Makefile
index ab8222579e7e..afb7499a7090 100644
--- a/drivers/staging/iio/cdc/Makefile
+++ b/drivers/staging/iio/cdc/Makefile
@@ -1,7 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0
 #
-# Makefile for industrial I/O DAC drivers
+# Makefile for industrial I/O CDC drivers
 #
 
-obj-$(CONFIG_AD7150) += ad7150.o
 obj-$(CONFIG_AD7746) += ad7746.o
diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c
deleted file mode 100644
index b6d2c02fa39d..000000000000
--- a/drivers/staging/iio/cdc/ad7150.c
+++ /dev/null
@@ -1,678 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0+
-/*
- * AD7150 capacitive sensor driver supporting AD7150/1/6
- *
- * Copyright 2010-2011 Analog Devices Inc.
- * Copyright 2021 Jonathan Cameron <Jonathan.Cameron@huawei.com>
- */
-
-#include <linux/bitfield.h>
-#include <linux/device.h>
-#include <linux/interrupt.h>
-#include <linux/irq.h>
-#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>
-
-#include <linux/iio/iio.h>
-#include <linux/iio/sysfs.h>
-#include <linux/iio/events.h>
-
-#define AD7150_STATUS_REG		0
-#define   AD7150_STATUS_OUT1		BIT(3)
-#define   AD7150_STATUS_OUT2		BIT(5)
-#define AD7150_CH1_DATA_HIGH_REG	1
-#define AD7150_CH2_DATA_HIGH_REG	3
-#define AD7150_CH1_AVG_HIGH_REG		5
-#define AD7150_CH2_AVG_HIGH_REG		7
-#define AD7150_CH1_SENSITIVITY_REG	9
-#define AD7150_CH1_THR_HOLD_H_REG	9
-#define AD7150_CH1_TIMEOUT_REG		10
-#define   AD7150_CH_TIMEOUT_RECEDING	GENMASK(3, 0)
-#define   AD7150_CH_TIMEOUT_APPROACHING	GENMASK(7, 4)
-#define AD7150_CH1_SETUP_REG		11
-#define AD7150_CH2_SENSITIVITY_REG	12
-#define AD7150_CH2_THR_HOLD_H_REG	12
-#define AD7150_CH2_TIMEOUT_REG		13
-#define AD7150_CH2_SETUP_REG		14
-#define AD7150_CFG_REG			15
-#define   AD7150_CFG_FIX		BIT(7)
-#define   AD7150_CFG_THRESHTYPE_MSK	GENMASK(6, 5)
-#define   AD7150_CFG_TT_NEG		0x0
-#define   AD7150_CFG_TT_POS		0x1
-#define   AD7150_CFG_TT_IN_WINDOW	0x2
-#define   AD7150_CFG_TT_OUT_WINDOW	0x3
-#define AD7150_PD_TIMER_REG		16
-#define AD7150_CH1_CAPDAC_REG		17
-#define AD7150_CH2_CAPDAC_REG		18
-#define AD7150_SN3_REG			19
-#define AD7150_SN2_REG			20
-#define AD7150_SN1_REG			21
-#define AD7150_SN0_REG			22
-#define AD7150_ID_REG			23
-
-enum {
-	AD7150,
-	AD7151,
-};
-
-/**
- * struct ad7150_chip_info - instance specific chip data
- * @client: i2c client for this device
- * @threshold: thresholds for simple capacitance value events
- * @thresh_sensitivity: threshold for simple capacitance offset
- *	from 'average' value.
- * @thresh_timeout: a timeout, in samples from the moment an
- *	adaptive threshold event occurs to when the average
- *	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
- * @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;
-	u16 threshold[2][2];
-	u8 thresh_sensitivity[2][2];
-	u8 thresh_timeout[2][2];
-	struct mutex state_lock;
-	int interrupts[2];
-	bool int_enabled[2];
-	enum iio_event_type type;
-	enum iio_event_direction dir;
-};
-
-static const u8 ad7150_addresses[][6] = {
-	{ AD7150_CH1_DATA_HIGH_REG, AD7150_CH1_AVG_HIGH_REG,
-	  AD7150_CH1_SETUP_REG, AD7150_CH1_THR_HOLD_H_REG,
-	  AD7150_CH1_SENSITIVITY_REG, AD7150_CH1_TIMEOUT_REG },
-	{ AD7150_CH2_DATA_HIGH_REG, AD7150_CH2_AVG_HIGH_REG,
-	  AD7150_CH2_SETUP_REG, AD7150_CH2_THR_HOLD_H_REG,
-	  AD7150_CH2_SENSITIVITY_REG, AD7150_CH2_TIMEOUT_REG },
-};
-
-static int ad7150_read_raw(struct iio_dev *indio_dev,
-			   struct iio_chan_spec const *chan,
-			   int *val,
-			   int *val2,
-			   long mask)
-{
-	struct ad7150_chip_info *chip = iio_priv(indio_dev);
-	int channel = chan->channel;
-	int ret;
-
-	switch (mask) {
-	case IIO_CHAN_INFO_RAW:
-		ret = i2c_smbus_read_word_swapped(chip->client,
-						  ad7150_addresses[channel][0]);
-		if (ret < 0)
-			return ret;
-		*val = ret >> 4;
-
-		return IIO_VAL_INT;
-	case IIO_CHAN_INFO_AVERAGE_RAW:
-		ret = i2c_smbus_read_word_swapped(chip->client,
-						  ad7150_addresses[channel][1]);
-		if (ret < 0)
-			return ret;
-		*val = ret;
-
-		return IIO_VAL_INT;
-	case IIO_CHAN_INFO_SCALE:
-		/*
-		 * Base units for capacitance are nano farads and the value
-		 * calculated from the datasheet formula is in picofarad
-		 * so multiply by 1000
-		 */
-		*val = 1000;
-		*val2 = 40944 >> 4; /* To match shift in _RAW */
-		return IIO_VAL_FRACTIONAL;
-	case IIO_CHAN_INFO_OFFSET:
-		*val = -(12288 >> 4); /* To match shift in _RAW */
-		return IIO_VAL_INT;
-	case IIO_CHAN_INFO_SAMP_FREQ:
-		/* Strangely same for both 1 and 2 chan parts */
-		*val = 100;
-		return IIO_VAL_INT;
-	default:
-		return -EINVAL;
-	}
-}
-
-static int ad7150_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 ad7150_chip_info *chip = iio_priv(indio_dev);
-	u8 threshtype;
-	bool thrfixed;
-	int ret;
-
-	ret = i2c_smbus_read_byte_data(chip->client, AD7150_CFG_REG);
-	if (ret < 0)
-		return ret;
-
-	threshtype = FIELD_GET(AD7150_CFG_THRESHTYPE_MSK, ret);
-
-	/*check if threshold mode is fixed or adaptive*/
-	thrfixed = FIELD_GET(AD7150_CFG_FIX, ret);
-
-	switch (type) {
-	case IIO_EV_TYPE_THRESH_ADAPTIVE:
-		if (dir == IIO_EV_DIR_RISING)
-			return !thrfixed && (threshtype == AD7150_CFG_TT_POS);
-		return !thrfixed && (threshtype == AD7150_CFG_TT_NEG);
-	case IIO_EV_TYPE_THRESH:
-		if (dir == IIO_EV_DIR_RISING)
-			return thrfixed && (threshtype == AD7150_CFG_TT_POS);
-		return thrfixed && (threshtype == AD7150_CFG_TT_NEG);
-	default:
-		break;
-	}
-	return -EINVAL;
-}
-
-/* state_lock should be held to ensure consistent state */
-static int ad7150_write_event_params(struct iio_dev *indio_dev,
-				     unsigned int chan,
-				     enum iio_event_type type,
-				     enum iio_event_direction dir)
-{
-	struct ad7150_chip_info *chip = iio_priv(indio_dev);
-	int rising = (dir == IIO_EV_DIR_RISING);
-
-	/* Only update value live, if parameter is in use */
-	if ((type != chip->type) || (dir != chip->dir))
-		return 0;
-
-	switch (type) {
-		/* Note completely different from the adaptive versions */
-	case IIO_EV_TYPE_THRESH: {
-		u16 value = chip->threshold[rising][chan];
-		return i2c_smbus_write_word_swapped(chip->client,
-						    ad7150_addresses[chan][3],
-						    value);
-	}
-	case IIO_EV_TYPE_THRESH_ADAPTIVE: {
-		int ret;
-		u8 sens, timeout;
-
-		sens = chip->thresh_sensitivity[rising][chan];
-		ret = i2c_smbus_write_byte_data(chip->client,
-						ad7150_addresses[chan][4],
-						sens);
-		if (ret)
-			return ret;
-
-		/*
-		 * Single timeout register contains timeouts for both
-		 * directions.
-		 */
-		timeout = FIELD_PREP(AD7150_CH_TIMEOUT_APPROACHING,
-				     chip->thresh_timeout[1][chan]);
-		timeout |= FIELD_PREP(AD7150_CH_TIMEOUT_RECEDING,
-				      chip->thresh_timeout[0][chan]);
-		return i2c_smbus_write_byte_data(chip->client,
-						 ad7150_addresses[chan][5],
-						 timeout);
-	}
-	default:
-		return -EINVAL;
-	}
-}
-
-static int ad7150_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 ad7150_chip_info *chip = iio_priv(indio_dev);
-	int ret;
-
-	/*
-	 * 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);
-	if ((type != chip->type) || (dir != chip->dir)) {
-		int rising = (dir == IIO_EV_DIR_RISING);
-		u8 thresh_type, cfg, fixed;
-
-		/*
-		 * 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]);
-
-		ret = i2c_smbus_read_byte_data(chip->client, AD7150_CFG_REG);
-		if (ret < 0)
-			goto error_ret;
-
-		cfg = ret & ~(AD7150_CFG_THRESHTYPE_MSK | AD7150_CFG_FIX);
-
-		if (type == IIO_EV_TYPE_THRESH_ADAPTIVE)
-			fixed = 0;
-		else
-			fixed = 1;
-
-		if (rising)
-			thresh_type = AD7150_CFG_TT_POS;
-		else
-			thresh_type = AD7150_CFG_TT_NEG;
-
-		cfg |= FIELD_PREP(AD7150_CFG_FIX, fixed) |
-			FIELD_PREP(AD7150_CFG_THRESHTYPE_MSK, thresh_type);
-
-		ret = i2c_smbus_write_byte_data(chip->client, AD7150_CFG_REG,
-						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;
-		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]);
-	}
-	if (!chip->int_enabled[chan->channel]) {
-		enable_irq(chip->interrupts[chan->channel]);
-		chip->int_enabled[chan->channel] = true;
-	}
-
-error_ret:
-	mutex_unlock(&chip->state_lock);
-
-	return ret;
-}
-
-static int ad7150_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 ad7150_chip_info *chip = iio_priv(indio_dev);
-	int rising = (dir == IIO_EV_DIR_RISING);
-
-	/* Complex register sharing going on here */
-	switch (info) {
-	case IIO_EV_INFO_VALUE:
-		switch (type) {
-		case IIO_EV_TYPE_THRESH_ADAPTIVE:
-			*val = chip->thresh_sensitivity[rising][chan->channel];
-			return IIO_VAL_INT;
-		case IIO_EV_TYPE_THRESH:
-			*val = chip->threshold[rising][chan->channel];
-			return IIO_VAL_INT;
-		default:
-			return -EINVAL;
-		}
-	case IIO_EV_INFO_TIMEOUT:
-		*val = 0;
-		*val2 = chip->thresh_timeout[rising][chan->channel] * 10000;
-		return IIO_VAL_INT_PLUS_MICRO;
-	default:
-		return -EINVAL;
-	}
-}
-
-static int ad7150_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)
-{
-	int ret;
-	struct ad7150_chip_info *chip = iio_priv(indio_dev);
-	int rising = (dir == IIO_EV_DIR_RISING);
-
-	mutex_lock(&chip->state_lock);
-	switch (info) {
-	case IIO_EV_INFO_VALUE:
-		switch (type) {
-		case IIO_EV_TYPE_THRESH_ADAPTIVE:
-			chip->thresh_sensitivity[rising][chan->channel] = val;
-			break;
-		case IIO_EV_TYPE_THRESH:
-			chip->threshold[rising][chan->channel] = val;
-			break;
-		default:
-			ret = -EINVAL;
-			goto error_ret;
-		}
-		break;
-	case IIO_EV_INFO_TIMEOUT: {
-		/*
-		 * Raw timeout is in cycles of 10 msecs as long as both
-		 * channels are enabled.
-		 * In terms of INT_PLUS_MICRO, that is in units of 10,000
-		 */
-		int timeout = val2 / 10000;
-
-		if (val != 0 || timeout < 0 || timeout > 15 || val2 % 10000) {
-			ret = -EINVAL;
-			goto error_ret;
-		}
-
-		chip->thresh_timeout[rising][chan->channel] = timeout;
-		break;
-	}
-	default:
-		ret = -EINVAL;
-		goto error_ret;
-	}
-
-	/* write back if active */
-	ret = ad7150_write_event_params(indio_dev, chan->channel, type, dir);
-
-error_ret:
-	mutex_unlock(&chip->state_lock);
-	return ret;
-}
-
-static const struct iio_event_spec ad7150_events[] = {
-	{
-		.type = IIO_EV_TYPE_THRESH,
-		.dir = IIO_EV_DIR_RISING,
-		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
-			BIT(IIO_EV_INFO_ENABLE),
-	}, {
-		.type = IIO_EV_TYPE_THRESH,
-		.dir = IIO_EV_DIR_FALLING,
-		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
-			BIT(IIO_EV_INFO_ENABLE),
-	}, {
-		.type = IIO_EV_TYPE_THRESH_ADAPTIVE,
-		.dir = IIO_EV_DIR_RISING,
-		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
-			BIT(IIO_EV_INFO_ENABLE) |
-			BIT(IIO_EV_INFO_TIMEOUT),
-	}, {
-		.type = IIO_EV_TYPE_THRESH_ADAPTIVE,
-		.dir = IIO_EV_DIR_FALLING,
-		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
-			BIT(IIO_EV_INFO_ENABLE) |
-			BIT(IIO_EV_INFO_TIMEOUT),
-	},
-};
-
-#define AD7150_CAPACITANCE_CHAN(_chan)	{			\
-		.type = IIO_CAPACITANCE,			\
-		.indexed = 1,					\
-		.channel = _chan,				\
-		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |	\
-		BIT(IIO_CHAN_INFO_AVERAGE_RAW),			\
-		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \
-			BIT(IIO_CHAN_INFO_OFFSET),		\
-		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),\
-		.event_spec = ad7150_events,			\
-		.num_event_specs = ARRAY_SIZE(ad7150_events),	\
-	}
-
-#define AD7150_CAPACITANCE_CHAN_NO_IRQ(_chan)	{		\
-		.type = IIO_CAPACITANCE,			\
-		.indexed = 1,					\
-		.channel = _chan,				\
-		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |	\
-		BIT(IIO_CHAN_INFO_AVERAGE_RAW),			\
-		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \
-			BIT(IIO_CHAN_INFO_OFFSET),		\
-		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),\
-	}
-
-static const struct iio_chan_spec ad7150_channels[] = {
-	AD7150_CAPACITANCE_CHAN(0),
-	AD7150_CAPACITANCE_CHAN(1),
-};
-
-static const struct iio_chan_spec ad7150_channels_no_irq[] = {
-	AD7150_CAPACITANCE_CHAN_NO_IRQ(0),
-	AD7150_CAPACITANCE_CHAN_NO_IRQ(1),
-};
-
-static const struct iio_chan_spec ad7151_channels[] = {
-	AD7150_CAPACITANCE_CHAN(0),
-};
-
-static const struct iio_chan_spec ad7151_channels_no_irq[] = {
-	AD7150_CAPACITANCE_CHAN_NO_IRQ(0),
-};
-
-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);
-	s64 timestamp = iio_get_time_ns(indio_dev);
-	int int_status;
-
-	int_status = i2c_smbus_read_byte_data(chip->client, AD7150_STATUS_REG);
-	if (int_status < 0)
-		return IRQ_HANDLED;
-
-	/*
-	 * 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]");
-
-static struct attribute *ad7150_event_attributes[] = {
-	&iio_const_attr_in_capacitance_thresh_adaptive_timeout_available
-	.dev_attr.attr,
-	NULL,
-};
-
-static const struct attribute_group ad7150_event_attribute_group = {
-	.attrs = ad7150_event_attributes,
-	.name = "events",
-};
-
-static const struct iio_info ad7150_info = {
-	.event_attrs = &ad7150_event_attribute_group,
-	.read_raw = &ad7150_read_raw,
-	.read_event_config = &ad7150_read_event_config,
-	.write_event_config = &ad7150_write_event_config,
-	.read_event_value = &ad7150_read_event_value,
-	.write_event_value = &ad7150_write_event_value,
-};
-
-static const struct iio_info ad7150_info_no_irq = {
-	.read_raw = &ad7150_read_raw,
-};
-
-static void ad7150_reg_disable(void *data)
-{
-	struct regulator *reg = data;
-
-	regulator_disable(reg);
-}
-
-static int ad7150_probe(struct i2c_client *client,
-			const struct i2c_device_id *id)
-{
-	struct ad7150_chip_info *chip;
-	struct iio_dev *indio_dev;
-	struct regulator *reg;
-	int ret;
-
-	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
-	if (!indio_dev)
-		return -ENOMEM;
-
-	chip = iio_priv(indio_dev);
-	mutex_init(&chip->state_lock);
-	chip->client = client;
-
-	indio_dev->name = id->name;
-
-	indio_dev->modes = INDIO_DIRECT_MODE;
-
-	reg = devm_regulator_get(&client->dev, "vdd");
-	if (IS_ERR(reg))
-		return PTR_ERR(reg);
-
-	ret = regulator_enable(reg);
-	if (ret)
-		return ret;
-
-	ret = devm_add_action_or_reset(&client->dev, ad7150_reg_disable, reg);
-	if (ret)
-		return ret;
-
-	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) {
-		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;
-			indio_dev->num_channels = ARRAY_SIZE(ad7151_channels);
-			break;
-		default:
-			return -EINVAL;
-		}
-
-	} 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);
-			break;
-		case AD7151:
-			indio_dev->channels = ad7151_channels_no_irq;
-			indio_dev->num_channels =
-				ARRAY_SIZE(ad7151_channels_no_irq);
-			break;
-		default:
-			return -EINVAL;
-		}
-	}
-
-	return devm_iio_device_register(indio_dev->dev.parent, indio_dev);
-}
-
-static const struct i2c_device_id ad7150_id[] = {
-	{ "ad7150", AD7150 },
-	{ "ad7151", AD7151 },
-	{ "ad7156", AD7150 },
-	{}
-};
-
-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,
-};
-module_i2c_driver(ad7150_driver);
-
-MODULE_AUTHOR("Barry Song <21cnbao@gmail.com>");
-MODULE_DESCRIPTION("Analog Devices AD7150/1/6 capacitive sensor driver");
-MODULE_LICENSE("GPL v2");
-- 
2.30.2

Re: [PATCH v2 23/24] dt-bindings:iio:cdc:adi,ad7150 binding doc

[Rob Herring]

On Sun, 14 Mar 2021 18:15:10 +0000, Jonathan Cameron wrote:
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> 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 <Jonathan.Cameron@huawei.com>
> Cc: Robh+dt@kernel.org
> Cc: devicetree@vger.kernel.org
> Link: https://lore.kernel.org/r/20210207154623.433442-22-jic23@kernel.org
> ---
>  .../bindings/iio/cdc/adi,ad7150.yaml          | 69 +++++++++++++++++++
>  1 file changed, 69 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v2 00/24] staging:iio:cdc:ad7150: cleanup / fixup / graduate

[Jonathan Cameron]

Hi All,

Whilst I'll give up at some point and just apply this without additional
tags I really don't like doing that as I've made too many idiot mistakes
in the past. 

Some of these are fairly trivial so please can people take a look if
they have a chance. Rob did the DT one (thanks!) so we are down to...

> 
> 12 - The big irq rework patch. Alex wasn't quite happy to give a tag
>      on that last time, but didn't mention anything specific. It's a bit
>      fiddly so fair enough!

(it's not that bad!)

> 
> 20 - of_match_table patch.  Simple patch, and I believe I addressed all
>      questions in v1, but no tags given yet.
> 
> 21 - ABI patch.  I don't think there is anything controversial in here
>      but it's not gained any tags yet.
>      

Perhaps didn't help that I accidentally didn't cc Barry on v2.

Thanks,

Jonathan



> v1 description:
> 
> This is an 'old' driver in IIO that has been in staging a while.
> First submitted in October 2010.
> 
> I wanted to try and experiment and picked this driver to try it with.
> 
> The cleanup etc here was all tested against some basic emulation
> added to QEMU rather than real hardware.  Once I've cleaned that up
> a tiny bit I'll push it up to https://github.com/jic23/qemu
> Note that for now I'm not proposing to upstream this to QEMU but
> would be interested in hearing if people thing it is a good idea to
> do so.
> 
> Whilst it's obviously hard to be absolutely sure that the emulation is
> correct, the fact that the original driver worked as expected and the
> cleaned up version still does is certainly encouraging.
> 
> Note however, that there were a few more significant changes in here than
> basic cleanup.
> 1. Interrupts / events were handled in a rather esoteric fashion.
>    (Always on, window modes represented as magnitudes).
>    Note that for two channel devices there are separate lines. The original
>    driver was not supporting this at all.
>    They now look more like a standard IIO driver and reflect experience
>    that we've gained over the years in dealing with devices where these
>    aren't interrupt lines as such, but rather reporters of current status.
> 2. Timeouts were handled in a fashion that clearly wouldn't work.
> 
> Note that this moving out of staging makes a few bits of ABI 'official'
> and so those are added to the main IIO ABI Docs.
> 
> Thanks in advance to anyone who has time to take a look.
> 
> Jonathan Cameron (24):
>   staging:iio:cdc:ad7150: use swapped reads for i2c rather than open
>     coding.
>   staging:iio:cdc:ad7150: Remove magnitude adaptive events
>   staging:iio:cdc:ad7150: Refactor event parameter update
>   staging:iio:cdc:ad7150: Timeout register covers both directions so
>     both need updating
>   staging:iio:cdc:ad7150: Drop platform data support
>   staging:iio:cdc:ad7150: Handle variation in chan_spec across device
>     and irq present or not
>   staging:iio:cdc:ad7150: Simplify event handling by only using rising
>     direction.
>   staging:iio:cdc:ad7150: Drop noisy print in probe
>   staging:iio:cdc:ad7150: Add sampling_frequency support
>   iio:event: Add timeout event info type
>   staging:iio:cdc:ad7150: Change timeout units to seconds and use core
>     support
>   staging:iio:cdc:ad7150: Rework interrupt handling.
>   staging:iio:cdc:ad7150: More consistent register and field naming
>   staging:iio:cdc:ad7150: Reorganize headers.
>   staging:iio:cdc:ad7150: Tidy up local variable positioning.
>   staging:iio:cdc:ad7150: Drop unnecessary block comments.
>   staging:iio:cdc:ad7150: Shift the _raw readings by 4 bits.
>   staging:iio:cdc:ad7150: Add scale and offset to
>     info_mask_shared_by_type
>   staging:iio:cdc:ad7150: Really basic regulator support.
>   staging:iio:cdc:ad7150: Add of_match_table
>   iio:Documentation:ABI Add missing elements as used by the adi,ad7150
>   staging:iio:cdc:ad7150: Add copyright notice given substantial
>     changes.
>   dt-bindings:iio:cdc:adi,ad7150 binding doc
>   iio:cdc:ad7150: Move driver out of staging.
> 
>  Documentation/ABI/testing/sysfs-bus-iio       |  33 +
>  .../bindings/iio/cdc/adi,ad7150.yaml          |  69 ++
>  drivers/iio/Kconfig                           |   1 +
>  drivers/iio/Makefile                          |   1 +
>  drivers/iio/cdc/Kconfig                       |  17 +
>  drivers/iio/cdc/Makefile                      |   6 +
>  drivers/iio/cdc/ad7150.c                      | 678 ++++++++++++++++++
>  drivers/iio/industrialio-event.c              |   1 +
>  drivers/staging/iio/cdc/Kconfig               |  10 -
>  drivers/staging/iio/cdc/Makefile              |   3 +-
>  drivers/staging/iio/cdc/ad7150.c              | 655 -----------------
>  include/linux/iio/types.h                     |   1 +
>  12 files changed, 808 insertions(+), 667 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/iio/cdc/adi,ad7150.yaml
>  create mode 100644 drivers/iio/cdc/Kconfig
>  create mode 100644 drivers/iio/cdc/Makefile
>  create mode 100644 drivers/iio/cdc/ad7150.c
>  delete mode 100644 drivers/staging/iio/cdc/ad7150.c
> 

Re: [PATCH v2 00/24] staging:iio:cdc:ad7150: cleanup / fixup / graduate

[Jonathan Cameron]

On Mon, 29 Mar 2021 16:30:21 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> Hi All,
> 
> Whilst I'll give up at some point and just apply this without additional
> tags I really don't like doing that as I've made too many idiot mistakes
> in the past. 
> 
> Some of these are fairly trivial so please can people take a look if
> they have a chance. Rob did the DT one (thanks!) so we are down to...
> 
> > 
> > 12 - The big irq rework patch. Alex wasn't quite happy to give a tag
> >      on that last time, but didn't mention anything specific. It's a bit
> >      fiddly so fair enough!  
> 
> (it's not that bad!)
> 

20 had an ack that I think was accidentally sent off list and hence burried
in my junk. And then there were two...

> > 
> > 21 - ABI patch.  I don't think there is anything controversial in here
> >      but it's not gained any tags yet.
> >        
> 
> Perhaps didn't help that I accidentally didn't cc Barry on v2.
> 
> Thanks,
> 
> Jonathan
> 
> 
> 
> > v1 description:
> > 
> > This is an 'old' driver in IIO that has been in staging a while.
> > First submitted in October 2010.
> > 
> > I wanted to try and experiment and picked this driver to try it with.
> > 
> > The cleanup etc here was all tested against some basic emulation
> > added to QEMU rather than real hardware.  Once I've cleaned that up
> > a tiny bit I'll push it up to https://github.com/jic23/qemu
> > Note that for now I'm not proposing to upstream this to QEMU but
> > would be interested in hearing if people thing it is a good idea to
> > do so.
> > 
> > Whilst it's obviously hard to be absolutely sure that the emulation is
> > correct, the fact that the original driver worked as expected and the
> > cleaned up version still does is certainly encouraging.
> > 
> > Note however, that there were a few more significant changes in here than
> > basic cleanup.
> > 1. Interrupts / events were handled in a rather esoteric fashion.
> >    (Always on, window modes represented as magnitudes).
> >    Note that for two channel devices there are separate lines. The original
> >    driver was not supporting this at all.
> >    They now look more like a standard IIO driver and reflect experience
> >    that we've gained over the years in dealing with devices where these
> >    aren't interrupt lines as such, but rather reporters of current status.
> > 2. Timeouts were handled in a fashion that clearly wouldn't work.
> > 
> > Note that this moving out of staging makes a few bits of ABI 'official'
> > and so those are added to the main IIO ABI Docs.
> > 
> > Thanks in advance to anyone who has time to take a look.
> > 
> > Jonathan Cameron (24):
> >   staging:iio:cdc:ad7150: use swapped reads for i2c rather than open
> >     coding.
> >   staging:iio:cdc:ad7150: Remove magnitude adaptive events
> >   staging:iio:cdc:ad7150: Refactor event parameter update
> >   staging:iio:cdc:ad7150: Timeout register covers both directions so
> >     both need updating
> >   staging:iio:cdc:ad7150: Drop platform data support
> >   staging:iio:cdc:ad7150: Handle variation in chan_spec across device
> >     and irq present or not
> >   staging:iio:cdc:ad7150: Simplify event handling by only using rising
> >     direction.
> >   staging:iio:cdc:ad7150: Drop noisy print in probe
> >   staging:iio:cdc:ad7150: Add sampling_frequency support
> >   iio:event: Add timeout event info type
> >   staging:iio:cdc:ad7150: Change timeout units to seconds and use core
> >     support
> >   staging:iio:cdc:ad7150: Rework interrupt handling.
> >   staging:iio:cdc:ad7150: More consistent register and field naming
> >   staging:iio:cdc:ad7150: Reorganize headers.
> >   staging:iio:cdc:ad7150: Tidy up local variable positioning.
> >   staging:iio:cdc:ad7150: Drop unnecessary block comments.
> >   staging:iio:cdc:ad7150: Shift the _raw readings by 4 bits.
> >   staging:iio:cdc:ad7150: Add scale and offset to
> >     info_mask_shared_by_type
> >   staging:iio:cdc:ad7150: Really basic regulator support.
> >   staging:iio:cdc:ad7150: Add of_match_table
> >   iio:Documentation:ABI Add missing elements as used by the adi,ad7150
> >   staging:iio:cdc:ad7150: Add copyright notice given substantial
> >     changes.
> >   dt-bindings:iio:cdc:adi,ad7150 binding doc
> >   iio:cdc:ad7150: Move driver out of staging.
> > 
> >  Documentation/ABI/testing/sysfs-bus-iio       |  33 +
> >  .../bindings/iio/cdc/adi,ad7150.yaml          |  69 ++
> >  drivers/iio/Kconfig                           |   1 +
> >  drivers/iio/Makefile                          |   1 +
> >  drivers/iio/cdc/Kconfig                       |  17 +
> >  drivers/iio/cdc/Makefile                      |   6 +
> >  drivers/iio/cdc/ad7150.c                      | 678 ++++++++++++++++++
> >  drivers/iio/industrialio-event.c              |   1 +
> >  drivers/staging/iio/cdc/Kconfig               |  10 -
> >  drivers/staging/iio/cdc/Makefile              |   3 +-
> >  drivers/staging/iio/cdc/ad7150.c              | 655 -----------------
> >  include/linux/iio/types.h                     |   1 +
> >  12 files changed, 808 insertions(+), 667 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/iio/cdc/adi,ad7150.yaml
> >  create mode 100644 drivers/iio/cdc/Kconfig
> >  create mode 100644 drivers/iio/cdc/Makefile
> >  create mode 100644 drivers/iio/cdc/ad7150.c
> >  delete mode 100644 drivers/staging/iio/cdc/ad7150.c
> >   
> 

RE: [PATCH v2 00/24] staging:iio:cdc:ad7150: cleanup / fixup / graduate

[Song Bao Hua (Barry Song)]

> -----Original Message-----
> From: Jonathan Cameron [mailto:jic23@jic23.retrosnub.co.uk]
> Sent: Tuesday, March 30, 2021 4:36 AM
> To: linux-iio@vger.kernel.org; Song Bao Hua (Barry Song)
> <song.bao.hua@hisilicon.com>
> Cc: Lars-Peter Clausen <lars@metafoo.de>; Michael Hennerich
> <Michael.Hennerich@analog.com>; Alexandru Ardelean <aardelean@deviqon.com>;
> Robh+dt@kernel.org; Alexandru Ardelean <ardeleanalex@gmail.com>; Jonathan
> Cameron <jonathan.cameron@huawei.com>
> Subject: Re: [PATCH v2 00/24] staging:iio:cdc:ad7150: cleanup / fixup / graduate
> 
> On Mon, 29 Mar 2021 16:30:21 +0100
> Jonathan Cameron <jic23@kernel.org> wrote:
> 
> > Hi All,
> >
> > Whilst I'll give up at some point and just apply this without additional
> > tags I really don't like doing that as I've made too many idiot mistakes
> > in the past.
> >
> > Some of these are fairly trivial so please can people take a look if
> > they have a chance. Rob did the DT one (thanks!) so we are down to...
> >
> > >
> > > 12 - The big irq rework patch. Alex wasn't quite happy to give a tag
> > >      on that last time, but didn't mention anything specific. It's a bit
> > >      fiddly so fair enough!
> >
> > (it's not that bad!)
> >
> 
> 20 had an ack that I think was accidentally sent off list and hence burried
> in my junk. And then there were two...
> 
> > >
> > > 21 - ABI patch.  I don't think there is anything controversial in here
> > >      but it's not gained any tags yet.
> > >
> >
> > Perhaps didn't help that I accidentally didn't cc Barry on v2.
> >
> > Thanks,
> >
> > Jonathan
> >
> >
> >
> > > v1 description:
> > >
> > > This is an 'old' driver in IIO that has been in staging a while.
> > > First submitted in October 2010.
> > >
> > > I wanted to try and experiment and picked this driver to try it with.
> > >
> > > The cleanup etc here was all tested against some basic emulation
> > > added to QEMU rather than real hardware.  Once I've cleaned that up
> > > a tiny bit I'll push it up to https://github.com/jic23/qemu
> > > Note that for now I'm not proposing to upstream this to QEMU but
> > > would be interested in hearing if people thing it is a good idea to
> > > do so.
> > >
> > > Whilst it's obviously hard to be absolutely sure that the emulation is
> > > correct, the fact that the original driver worked as expected and the
> > > cleaned up version still does is certainly encouraging.
> > >
> > > Note however, that there were a few more significant changes in here than
> > > basic cleanup.
> > > 1. Interrupts / events were handled in a rather esoteric fashion.
> > >    (Always on, window modes represented as magnitudes).
> > >    Note that for two channel devices there are separate lines. The original
> > >    driver was not supporting this at all.
> > >    They now look more like a standard IIO driver and reflect experience
> > >    that we've gained over the years in dealing with devices where these
> > >    aren't interrupt lines as such, but rather reporters of current status.
> > > 2. Timeouts were handled in a fashion that clearly wouldn't work.
> > >
> > > Note that this moving out of staging makes a few bits of ABI 'official'
> > > and so those are added to the main IIO ABI Docs.
> > >
> > > Thanks in advance to anyone who has time to take a look.
> > >
> > > Jonathan Cameron (24):
> > >   staging:iio:cdc:ad7150: use swapped reads for i2c rather than open
> > >     coding.

Using i2c_smbus_read_word_swapper and i2c_smbus_write_word_swapped
looks good to me. The only thing is that your changelog didn't
mention you also used swapper write as you said "use swapped
reads". Otherwise,

Reviewed-by: Barry Song <song.bao.hua@hisilicon>

> > >   staging:iio:cdc:ad7150: Remove magnitude adaptive events
> > >   staging:iio:cdc:ad7150: Refactor event parameter update
> > >   staging:iio:cdc:ad7150: Timeout register covers both directions so
> > >     both need updating
> > >   staging:iio:cdc:ad7150: Drop platform data support
> > >   staging:iio:cdc:ad7150: Handle variation in chan_spec across device
> > >     and irq present or not
> > >   staging:iio:cdc:ad7150: Simplify event handling by only using rising
> > >     direction.
> > >   staging:iio:cdc:ad7150: Drop noisy print in probe
> > >   staging:iio:cdc:ad7150: Add sampling_frequency support
> > >   iio:event: Add timeout event info type
> > >   staging:iio:cdc:ad7150: Change timeout units to seconds and use core
> > >     support
> > >   staging:iio:cdc:ad7150: Rework interrupt handling.

+	/*
+	 * 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;
+

I am not sure what kind of race conditions we have since disable_irq()
will synchronize with the irq handler.

If we are in an interrupt, the user who calls disable_irq will wait
for the completion of irq handler.
If an interrupt comes in the gap of disable_irq and enable_irq, we should
have a valid int_status after we re-enable the interrupt?

Maybe it is because of the weird behavior of the hardware?

> > >   staging:iio:cdc:ad7150: More consistent register and field naming
> > >   staging:iio:cdc:ad7150: Reorganize headers.
> > >   staging:iio:cdc:ad7150: Tidy up local variable positioning.
> > >   staging:iio:cdc:ad7150: Drop unnecessary block comments.
> > >   staging:iio:cdc:ad7150: Shift the _raw readings by 4 bits.
> > >   staging:iio:cdc:ad7150: Add scale and offset to
> > >     info_mask_shared_by_type
> > >   staging:iio:cdc:ad7150: Really basic regulator support.
> > >   staging:iio:cdc:ad7150: Add of_match_table

Reviewed-by: Barry Song <song.bao.hua@hisilicon>

> > >   iio:Documentation:ABI Add missing elements as used by the adi,ad7150

+What:		/sys/.../events/in_capacitanceY_adaptive_thresh_rising_en
+What:		/sys/.../events/in_capacitanceY_adaptive_thresh_falling_en
+KernelVersion:	5.11

Is kernel 5.11 the right version? Guess not :-)

> > >   staging:iio:cdc:ad7150: Add copyright notice given substantial
> > >     changes.
> > >   dt-bindings:iio:cdc:adi,ad7150 binding doc

Reviewed-by: Barry Song <song.bao.hua@hisilicon>

+      properties:
+        compatible:
+          contains:
+            enum:
+              - adi,ad7150
+              - adi,ad7156
+    then:
+      properties:
+        interrupts:
+          minItems: 2
+          maxItems: 2
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: adi,ad7151
+    then:
+      properties:
+        interrupts:
+          minItems: 1
+          maxItems: 1

A further follow-up might be:
we can move to read two irq number for adi7150/7156
and one irq number for adi7151 in driver?

> > >   iio:cdc:ad7150: Move driver out of staging.
> > >
> > >  Documentation/ABI/testing/sysfs-bus-iio       |  33 +
> > >  .../bindings/iio/cdc/adi,ad7150.yaml          |  69 ++
> > >  drivers/iio/Kconfig                           |   1 +
> > >  drivers/iio/Makefile                          |   1 +
> > >  drivers/iio/cdc/Kconfig                       |  17 +
> > >  drivers/iio/cdc/Makefile                      |   6 +
> > >  drivers/iio/cdc/ad7150.c                      | 678 ++++++++++++++++++
> > >  drivers/iio/industrialio-event.c              |   1 +
> > >  drivers/staging/iio/cdc/Kconfig               |  10 -
> > >  drivers/staging/iio/cdc/Makefile              |   3 +-
> > >  drivers/staging/iio/cdc/ad7150.c              | 655 -----------------
> > >  include/linux/iio/types.h                     |   1 +
> > >  12 files changed, 808 insertions(+), 667 deletions(-)
> > >  create mode 100644
> Documentation/devicetree/bindings/iio/cdc/adi,ad7150.yaml
> > >  create mode 100644 drivers/iio/cdc/Kconfig
> > >  create mode 100644 drivers/iio/cdc/Makefile
> > >  create mode 100644 drivers/iio/cdc/ad7150.c
> > >  delete mode 100644 drivers/staging/iio/cdc/ad7150.c
> > >
> >


Thanks
Barry

Re: [PATCH v2 20/24] staging:iio:cdc:ad7150: Add of_match_table

[Alexandru Ardelean]

On Sun, Mar 14, 2021 at 8:18 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> 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.
>

I thought I replied on this.
Seems I didn't

Reviewed-by: Alexandru Ardelean <ardeleanalex@gmail.com>

> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Link: https://lore.kernel.org/r/20210207154623.433442-21-jic23@kernel.org
> ---
>  drivers/staging/iio/cdc/ad7150.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c
> index 30abfacda074..0afad49bcddc 100644
> --- a/drivers/staging/iio/cdc/ad7150.c
> +++ b/drivers/staging/iio/cdc/ad7150.c
> @@ -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
>

Re: [PATCH v2 21/24] iio:Documentation:ABI Add missing elements as used by the adi,ad7150

[Alexandru Ardelean]

On Sun, Mar 14, 2021 at 8:18 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> 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.
>

Reviewed-by: Alexandru Ardelean <ardeleanalex@gmail.com>

> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Link: https://lore.kernel.org/r/20210207154623.433442-23-jic23@kernel.org
> ---
>  Documentation/ABI/testing/sysfs-bus-iio | 33 +++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> index affd4ce871d7..32d26c118141 100644
> --- a/Documentation/ABI/testing/sysfs-bus-iio
> +++ b/Documentation/ABI/testing/sysfs-bus-iio
> @@ -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:       linux-iio@vger.kernel.org
>  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:       linux-iio@vger.kernel.org
>  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:       linux-iio@vger.kernel.org
> +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:       linux-iio@vger.kernel.org
> +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:       linux-iio@vger.kernel.org
>  Description:
> --
> 2.30.2
>

Re: [PATCH v2 12/24] staging:iio:cdc:ad7150: Rework interrupt handling.

[Alexandru Ardelean]

On Sun, Mar 14, 2021 at 8:17 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> 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.
>

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


> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Link: https://lore.kernel.org/r/20210207154623.433442-13-jic23@kernel.org
> ---
>  drivers/staging/iio/cdc/ad7150.c | 258 ++++++++++++++++++-------------
>  1 file changed, 153 insertions(+), 105 deletions(-)
>
> diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c
> index f8183c540d5c..24be97456c03 100644
> --- a/drivers/staging/iio/cdc/ad7150.c
> +++ b/drivers/staging/iio/cdc/ad7150.c
> @@ -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;
> +                       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;

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


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

> +               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
>

Re: [PATCH v2 12/24] staging:iio:cdc:ad7150: Rework interrupt handling.

[Jonathan Cameron]

On Wed, 31 Mar 2021 10:29:51 +0300
Alexandru Ardelean <ardeleanalex@gmail.com> wrote:

> On Sun, Mar 14, 2021 at 8:17 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >
> > 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.
> >  
> 
> 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.


> 
> 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Link: https://lore.kernel.org/r/20210207154623.433442-13-jic23@kernel.org
> > ---
> >  drivers/staging/iio/cdc/ad7150.c | 258 ++++++++++++++++++-------------
> >  1 file changed, 153 insertions(+), 105 deletions(-)
> >
> > diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c
> > index f8183c540d5c..24be97456c03 100644
> > --- a/drivers/staging/iio/cdc/ad7150.c
> > +++ b/drivers/staging/iio/cdc/ad7150.c
> > @@ -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;
> > +                       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.

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

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.

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



> 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

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.

> 
> > +               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
> >  

Re: [PATCH v2 00/24] staging:iio:cdc:ad7150: cleanup / fixup / graduate

[Jonathan Cameron]

On Tue, 30 Mar 2021 21:23:59 +0000
"Song Bao Hua (Barry Song)" <song.bao.hua@hisilicon.com> wrote:

> > -----Original Message-----
> > From: Jonathan Cameron [mailto:jic23@jic23.retrosnub.co.uk]
> > Sent: Tuesday, March 30, 2021 4:36 AM
> > To: linux-iio@vger.kernel.org; Song Bao Hua (Barry Song)
> > <song.bao.hua@hisilicon.com>
> > Cc: Lars-Peter Clausen <lars@metafoo.de>; Michael Hennerich
> > <Michael.Hennerich@analog.com>; Alexandru Ardelean <aardelean@deviqon.com>;
> > Robh+dt@kernel.org; Alexandru Ardelean <ardeleanalex@gmail.com>; Jonathan
> > Cameron <jonathan.cameron@huawei.com>
> > Subject: Re: [PATCH v2 00/24] staging:iio:cdc:ad7150: cleanup / fixup / graduate
> > 
> > On Mon, 29 Mar 2021 16:30:21 +0100
> > Jonathan Cameron <jic23@kernel.org> wrote:
> >   
> > > Hi All,
> > >
> > > Whilst I'll give up at some point and just apply this without additional
> > > tags I really don't like doing that as I've made too many idiot mistakes
> > > in the past.
> > >
> > > Some of these are fairly trivial so please can people take a look if
> > > they have a chance. Rob did the DT one (thanks!) so we are down to...
> > >  
> > > >
> > > > 12 - The big irq rework patch. Alex wasn't quite happy to give a tag
> > > >      on that last time, but didn't mention anything specific. It's a bit
> > > >      fiddly so fair enough!  
> > >
> > > (it's not that bad!)
> > >  
> > 
> > 20 had an ack that I think was accidentally sent off list and hence burried
> > in my junk. And then there were two...
> >   
> > > >
> > > > 21 - ABI patch.  I don't think there is anything controversial in here
> > > >      but it's not gained any tags yet.
> > > >  
> > >
> > > Perhaps didn't help that I accidentally didn't cc Barry on v2.
> > >
> > > Thanks,
> > >
> > > Jonathan
> > >
> > >
> > >  
> > > > v1 description:
> > > >
> > > > This is an 'old' driver in IIO that has been in staging a while.
> > > > First submitted in October 2010.
> > > >
> > > > I wanted to try and experiment and picked this driver to try it with.
> > > >
> > > > The cleanup etc here was all tested against some basic emulation
> > > > added to QEMU rather than real hardware.  Once I've cleaned that up
> > > > a tiny bit I'll push it up to https://github.com/jic23/qemu
> > > > Note that for now I'm not proposing to upstream this to QEMU but
> > > > would be interested in hearing if people thing it is a good idea to
> > > > do so.
> > > >
> > > > Whilst it's obviously hard to be absolutely sure that the emulation is
> > > > correct, the fact that the original driver worked as expected and the
> > > > cleaned up version still does is certainly encouraging.
> > > >
> > > > Note however, that there were a few more significant changes in here than
> > > > basic cleanup.
> > > > 1. Interrupts / events were handled in a rather esoteric fashion.
> > > >    (Always on, window modes represented as magnitudes).
> > > >    Note that for two channel devices there are separate lines. The original
> > > >    driver was not supporting this at all.
> > > >    They now look more like a standard IIO driver and reflect experience
> > > >    that we've gained over the years in dealing with devices where these
> > > >    aren't interrupt lines as such, but rather reporters of current status.
> > > > 2. Timeouts were handled in a fashion that clearly wouldn't work.
> > > >
> > > > Note that this moving out of staging makes a few bits of ABI 'official'
> > > > and so those are added to the main IIO ABI Docs.
> > > >
> > > > Thanks in advance to anyone who has time to take a look.
> > > >
> > > > Jonathan Cameron (24):
> > > >   staging:iio:cdc:ad7150: use swapped reads for i2c rather than open
> > > >     coding.  
> 
> Using i2c_smbus_read_word_swapper and i2c_smbus_write_word_swapped
> looks good to me. The only thing is that your changelog didn't
> mention you also used swapper write as you said "use swapped
> reads". Otherwise,
> 
> Reviewed-by: Barry Song <song.bao.hua@hisilicon>
> 
> > > >   staging:iio:cdc:ad7150: Remove magnitude adaptive events
> > > >   staging:iio:cdc:ad7150: Refactor event parameter update
> > > >   staging:iio:cdc:ad7150: Timeout register covers both directions so
> > > >     both need updating
> > > >   staging:iio:cdc:ad7150: Drop platform data support
> > > >   staging:iio:cdc:ad7150: Handle variation in chan_spec across device
> > > >     and irq present or not
> > > >   staging:iio:cdc:ad7150: Simplify event handling by only using rising
> > > >     direction.
> > > >   staging:iio:cdc:ad7150: Drop noisy print in probe
> > > >   staging:iio:cdc:ad7150: Add sampling_frequency support
> > > >   iio:event: Add timeout event info type
> > > >   staging:iio:cdc:ad7150: Change timeout units to seconds and use core
> > > >     support
> > > >   staging:iio:cdc:ad7150: Rework interrupt handling.  
> 
> +	/*
> +	 * 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;
> +
> 
> I am not sure what kind of race conditions we have since disable_irq()
> will synchronize with the irq handler.
> 
> If we are in an interrupt, the user who calls disable_irq will wait
> for the completion of irq handler.
> If an interrupt comes in the gap of disable_irq and enable_irq, we should
> have a valid int_status after we re-enable the interrupt?
> 
> Maybe it is because of the weird behavior of the hardware?

I think you are right and I was being overly paranoid on that one. I'll drop
the comment, though I'll leave the paranoid check.

> 
> > > >   staging:iio:cdc:ad7150: More consistent register and field naming
> > > >   staging:iio:cdc:ad7150: Reorganize headers.
> > > >   staging:iio:cdc:ad7150: Tidy up local variable positioning.
> > > >   staging:iio:cdc:ad7150: Drop unnecessary block comments.
> > > >   staging:iio:cdc:ad7150: Shift the _raw readings by 4 bits.
> > > >   staging:iio:cdc:ad7150: Add scale and offset to
> > > >     info_mask_shared_by_type
> > > >   staging:iio:cdc:ad7150: Really basic regulator support.
> > > >   staging:iio:cdc:ad7150: Add of_match_table  
> 
> Reviewed-by: Barry Song <song.bao.hua@hisilicon>
> 
> > > >   iio:Documentation:ABI Add missing elements as used by the adi,ad7150  
> 
> +What:		/sys/.../events/in_capacitanceY_adaptive_thresh_rising_en
> +What:		/sys/.../events/in_capacitanceY_adaptive_thresh_falling_en
> +KernelVersion:	5.11
> 
> Is kernel 5.11 the right version? Guess not :-)
Oops Can tell this took a while.  I'll tidy that up whilst applying.
> 
> > > >   staging:iio:cdc:ad7150: Add copyright notice given substantial
> > > >     changes.
> > > >   dt-bindings:iio:cdc:adi,ad7150 binding doc  
> 
> Reviewed-by: Barry Song <song.bao.hua@hisilicon>
> 
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - adi,ad7150
> +              - adi,ad7156
> +    then:
> +      properties:
> +        interrupts:
> +          minItems: 2
> +          maxItems: 2
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: adi,ad7151
> +    then:
> +      properties:
> +        interrupts:
> +          minItems: 1
> +          maxItems: 1
> 
> A further follow-up might be:
> we can move to read two irq number for adi7150/7156
> and one irq number for adi7151 in driver?

Unless I'm miss understanding you I think we already do.
There is a check against the driver_data being AD7150 which is set for
the two parts that have 2 interrupts.  If we don't get that we don't
query the second irq number.

> 
> > > >   iio:cdc:ad7150: Move driver out of staging.
> > > >
> > > >  Documentation/ABI/testing/sysfs-bus-iio       |  33 +
> > > >  .../bindings/iio/cdc/adi,ad7150.yaml          |  69 ++
> > > >  drivers/iio/Kconfig                           |   1 +
> > > >  drivers/iio/Makefile                          |   1 +
> > > >  drivers/iio/cdc/Kconfig                       |  17 +
> > > >  drivers/iio/cdc/Makefile                      |   6 +
> > > >  drivers/iio/cdc/ad7150.c                      | 678 ++++++++++++++++++
> > > >  drivers/iio/industrialio-event.c              |   1 +
> > > >  drivers/staging/iio/cdc/Kconfig               |  10 -
> > > >  drivers/staging/iio/cdc/Makefile              |   3 +-
> > > >  drivers/staging/iio/cdc/ad7150.c              | 655 -----------------
> > > >  include/linux/iio/types.h                     |   1 +
> > > >  12 files changed, 808 insertions(+), 667 deletions(-)
> > > >  create mode 100644  
> > Documentation/devicetree/bindings/iio/cdc/adi,ad7150.yaml  
> > > >  create mode 100644 drivers/iio/cdc/Kconfig
> > > >  create mode 100644 drivers/iio/cdc/Makefile
> > > >  create mode 100644 drivers/iio/cdc/ad7150.c
> > > >  delete mode 100644 drivers/staging/iio/cdc/ad7150.c
> > > >  
> > >  
> 
> 
> Thanks
> Barry
> 

Thanks!

Jonathan

Re: [PATCH v2 12/24] staging:iio:cdc:ad7150: Rework interrupt handling.

[Alexandru Ardelean]

On Wed, 31 Mar 2021 at 15:05, Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Wed, 31 Mar 2021 10:29:51 +0300
> Alexandru Ardelean <ardeleanalex@gmail.com> wrote:
>
> > On Sun, Mar 14, 2021 at 8:17 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > >
> > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > >
> > > 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.
> > >
> >
> > 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.

Looks good.
Thanks for the clarifications.
I'm still a bit paranoid about the int_enabled stuff, but I'll take
your word for it.

Reviewed-by: Alexandru Ardelean <ardeleanalex@gmail.com>


>
>
> >
> >
> > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > Link: https://lore.kernel.org/r/20210207154623.433442-13-jic23@kernel.org
> > > ---
> > >  drivers/staging/iio/cdc/ad7150.c | 258 ++++++++++++++++++-------------
> > >  1 file changed, 153 insertions(+), 105 deletions(-)
> > >
> > > diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c
> > > index f8183c540d5c..24be97456c03 100644
> > > --- a/drivers/staging/iio/cdc/ad7150.c
> > > +++ b/drivers/staging/iio/cdc/ad7150.c
> > > @@ -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;
> > > +                       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.
>
> >
> > > +                       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;
> > > +               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;
>
> 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.
>

I reserve the right remain paranoid about it :p

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

ack;
thanks for the info;

> 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.
>
>
>
> > 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
>
> 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.

ack


>
> >
> > > +               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
> > >
>

RE: [PATCH v2 00/24] staging:iio:cdc:ad7150: cleanup / fixup / graduate

[Song Bao Hua (Barry Song)]

> -----Original Message-----
> From: Jonathan Cameron [mailto:jic23@jic23.retrosnub.co.uk]
> Sent: Thursday, April 1, 2021 1:13 AM
> To: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>
> Cc: linux-iio@vger.kernel.org; Lars-Peter Clausen <lars@metafoo.de>; Michael
> Hennerich <Michael.Hennerich@analog.com>; Alexandru Ardelean
> <aardelean@deviqon.com>; Robh+dt@kernel.org; Alexandru Ardelean
> <ardeleanalex@gmail.com>; Jonathan Cameron <jonathan.cameron@huawei.com>
> Subject: Re: [PATCH v2 00/24] staging:iio:cdc:ad7150: cleanup / fixup / graduate
> 
> On Tue, 30 Mar 2021 21:23:59 +0000
> "Song Bao Hua (Barry Song)" <song.bao.hua@hisilicon.com> wrote:
> 
> > > -----Original Message-----
> > > From: Jonathan Cameron [mailto:jic23@jic23.retrosnub.co.uk]
> > > Sent: Tuesday, March 30, 2021 4:36 AM
> > > To: linux-iio@vger.kernel.org; Song Bao Hua (Barry Song)
> > > <song.bao.hua@hisilicon.com>
> > > Cc: Lars-Peter Clausen <lars@metafoo.de>; Michael Hennerich
> > > <Michael.Hennerich@analog.com>; Alexandru Ardelean
> <aardelean@deviqon.com>;
> > > Robh+dt@kernel.org; Alexandru Ardelean <ardeleanalex@gmail.com>; Jonathan
> > > Cameron <jonathan.cameron@huawei.com>
> > > Subject: Re: [PATCH v2 00/24] staging:iio:cdc:ad7150: cleanup / fixup /
> graduate
> > >
> > > On Mon, 29 Mar 2021 16:30:21 +0100
> > > Jonathan Cameron <jic23@kernel.org> wrote:
> > >
> > > > Hi All,
> > > >
> > > > Whilst I'll give up at some point and just apply this without additional
> > > > tags I really don't like doing that as I've made too many idiot mistakes
> > > > in the past.
> > > >
> > > > Some of these are fairly trivial so please can people take a look if
> > > > they have a chance. Rob did the DT one (thanks!) so we are down to...
> > > >
> > > > >
> > > > > 12 - The big irq rework patch. Alex wasn't quite happy to give a tag
> > > > >      on that last time, but didn't mention anything specific. It's a
> bit
> > > > >      fiddly so fair enough!
> > > >
> > > > (it's not that bad!)
> > > >
> > >
> > > 20 had an ack that I think was accidentally sent off list and hence burried
> > > in my junk. And then there were two...
> > >
> > > > >
> > > > > 21 - ABI patch.  I don't think there is anything controversial in here
> > > > >      but it's not gained any tags yet.
> > > > >
> > > >
> > > > Perhaps didn't help that I accidentally didn't cc Barry on v2.
> > > >
> > > > Thanks,
> > > >
> > > > Jonathan
> > > >
> > > >
> > > >
> > > > > v1 description:
> > > > >
> > > > > This is an 'old' driver in IIO that has been in staging a while.
> > > > > First submitted in October 2010.
> > > > >
> > > > > I wanted to try and experiment and picked this driver to try it with.
> > > > >
> > > > > The cleanup etc here was all tested against some basic emulation
> > > > > added to QEMU rather than real hardware.  Once I've cleaned that up
> > > > > a tiny bit I'll push it up to https://github.com/jic23/qemu
> > > > > Note that for now I'm not proposing to upstream this to QEMU but
> > > > > would be interested in hearing if people thing it is a good idea to
> > > > > do so.
> > > > >
> > > > > Whilst it's obviously hard to be absolutely sure that the emulation
> is
> > > > > correct, the fact that the original driver worked as expected and the
> > > > > cleaned up version still does is certainly encouraging.
> > > > >
> > > > > Note however, that there were a few more significant changes in here
> than
> > > > > basic cleanup.
> > > > > 1. Interrupts / events were handled in a rather esoteric fashion.
> > > > >    (Always on, window modes represented as magnitudes).
> > > > >    Note that for two channel devices there are separate lines. The original
> > > > >    driver was not supporting this at all.
> > > > >    They now look more like a standard IIO driver and reflect experience
> > > > >    that we've gained over the years in dealing with devices where these
> > > > >    aren't interrupt lines as such, but rather reporters of current status.
> > > > > 2. Timeouts were handled in a fashion that clearly wouldn't work.
> > > > >
> > > > > Note that this moving out of staging makes a few bits of ABI 'official'
> > > > > and so those are added to the main IIO ABI Docs.
> > > > >
> > > > > Thanks in advance to anyone who has time to take a look.
> > > > >
> > > > > Jonathan Cameron (24):
> > > > >   staging:iio:cdc:ad7150: use swapped reads for i2c rather than open
> > > > >     coding.
> >
> > Using i2c_smbus_read_word_swapper and i2c_smbus_write_word_swapped
> > looks good to me. The only thing is that your changelog didn't
> > mention you also used swapper write as you said "use swapped
> > reads". Otherwise,
> >
> > Reviewed-by: Barry Song <song.bao.hua@hisilicon>
> >
> > > > >   staging:iio:cdc:ad7150: Remove magnitude adaptive events
> > > > >   staging:iio:cdc:ad7150: Refactor event parameter update
> > > > >   staging:iio:cdc:ad7150: Timeout register covers both directions so
> > > > >     both need updating
> > > > >   staging:iio:cdc:ad7150: Drop platform data support
> > > > >   staging:iio:cdc:ad7150: Handle variation in chan_spec across device
> > > > >     and irq present or not
> > > > >   staging:iio:cdc:ad7150: Simplify event handling by only using rising
> > > > >     direction.
> > > > >   staging:iio:cdc:ad7150: Drop noisy print in probe
> > > > >   staging:iio:cdc:ad7150: Add sampling_frequency support
> > > > >   iio:event: Add timeout event info type
> > > > >   staging:iio:cdc:ad7150: Change timeout units to seconds and use core
> > > > >     support
> > > > >   staging:iio:cdc:ad7150: Rework interrupt handling.
> >
> > +	/*
> > +	 * 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;
> > +
> >
> > I am not sure what kind of race conditions we have since disable_irq()
> > will synchronize with the irq handler.
> >
> > If we are in an interrupt, the user who calls disable_irq will wait
> > for the completion of irq handler.
> > If an interrupt comes in the gap of disable_irq and enable_irq, we should
> > have a valid int_status after we re-enable the interrupt?
> >
> > Maybe it is because of the weird behavior of the hardware?
> 
> I think you are right and I was being overly paranoid on that one. I'll drop
> the comment, though I'll leave the paranoid check.
> 

Yes. Make sense.

> >
> > > > >   staging:iio:cdc:ad7150: More consistent register and field naming
> > > > >   staging:iio:cdc:ad7150: Reorganize headers.
> > > > >   staging:iio:cdc:ad7150: Tidy up local variable positioning.
> > > > >   staging:iio:cdc:ad7150: Drop unnecessary block comments.
> > > > >   staging:iio:cdc:ad7150: Shift the _raw readings by 4 bits.
> > > > >   staging:iio:cdc:ad7150: Add scale and offset to
> > > > >     info_mask_shared_by_type
> > > > >   staging:iio:cdc:ad7150: Really basic regulator support.
> > > > >   staging:iio:cdc:ad7150: Add of_match_table
> >
> > Reviewed-by: Barry Song <song.bao.hua@hisilicon>
> >
> > > > >   iio:Documentation:ABI Add missing elements as used by the adi,ad7150
> >
> > +What:		/sys/.../events/in_capacitanceY_adaptive_thresh_rising_en
> > +What:		/sys/.../events/in_capacitanceY_adaptive_thresh_falling_en
> > +KernelVersion:	5.11
> >
> > Is kernel 5.11 the right version? Guess not :-)
> Oops Can tell this took a while.  I'll tidy that up whilst applying.
> >
> > > > >   staging:iio:cdc:ad7150: Add copyright notice given substantial
> > > > >     changes.
> > > > >   dt-bindings:iio:cdc:adi,ad7150 binding doc
> >
> > Reviewed-by: Barry Song <song.bao.hua@hisilicon>
> >
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            enum:
> > +              - adi,ad7150
> > +              - adi,ad7156
> > +    then:
> > +      properties:
> > +        interrupts:
> > +          minItems: 2
> > +          maxItems: 2
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: adi,ad7151
> > +    then:
> > +      properties:
> > +        interrupts:
> > +          minItems: 1
> > +          maxItems: 1
> >
> > A further follow-up might be:
> > we can move to read two irq number for adi7150/7156
> > and one irq number for adi7151 in driver?
> 
> Unless I'm miss understanding you I think we already do.
> There is a check against the driver_data being AD7150 which is set for
> the two parts that have 2 interrupts.  If we don't get that we don't
> query the second irq number.

Sorry for missing that part.

> 
> >
> > > > >   iio:cdc:ad7150: Move driver out of staging.
> > > > >
> > > > >  Documentation/ABI/testing/sysfs-bus-iio       |  33 +
> > > > >  .../bindings/iio/cdc/adi,ad7150.yaml          |  69 ++
> > > > >  drivers/iio/Kconfig                           |   1 +
> > > > >  drivers/iio/Makefile                          |   1 +
> > > > >  drivers/iio/cdc/Kconfig                       |  17 +
> > > > >  drivers/iio/cdc/Makefile                      |   6 +
> > > > >  drivers/iio/cdc/ad7150.c                      | 678 ++++++++++++++++++
> > > > >  drivers/iio/industrialio-event.c              |   1 +
> > > > >  drivers/staging/iio/cdc/Kconfig               |  10 -
> > > > >  drivers/staging/iio/cdc/Makefile              |   3 +-
> > > > >  drivers/staging/iio/cdc/ad7150.c              | 655 -----------------
> > > > >  include/linux/iio/types.h                     |   1 +
> > > > >  12 files changed, 808 insertions(+), 667 deletions(-)
> > > > >  create mode 100644
> > > Documentation/devicetree/bindings/iio/cdc/adi,ad7150.yaml
> > > > >  create mode 100644 drivers/iio/cdc/Kconfig
> > > > >  create mode 100644 drivers/iio/cdc/Makefile
> > > > >  create mode 100644 drivers/iio/cdc/ad7150.c
> > > > >  delete mode 100644 drivers/staging/iio/cdc/ad7150.c
> > > > >
> > > >
> >
> >
Thanks
Barry

Re: [PATCH v2 00/24] staging:iio:cdc:ad7150: cleanup / fixup / graduate

[Jonathan Cameron]

On Thu, 1 Apr 2021 10:15:22 +0000
"Song Bao Hua (Barry Song)" <song.bao.hua@hisilicon.com> wrote:

Various tweaks as discussed below made and whole series applied to
the togreg branch of iio.git and pushed out as testing for hte
usual reasons.   Thanks to Alex and Barry for the quick follow up
on review responses!

With hindsight I should be more careful when using b4 to apply and
resend versions of my own patch series. Had to go through by hand
deleting the duplicate links :(

Jonathan


> > -----Original Message-----
> > From: Jonathan Cameron [mailto:jic23@jic23.retrosnub.co.uk]
> > Sent: Thursday, April 1, 2021 1:13 AM
> > To: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>
> > Cc: linux-iio@vger.kernel.org; Lars-Peter Clausen <lars@metafoo.de>; Michael
> > Hennerich <Michael.Hennerich@analog.com>; Alexandru Ardelean
> > <aardelean@deviqon.com>; Robh+dt@kernel.org; Alexandru Ardelean
> > <ardeleanalex@gmail.com>; Jonathan Cameron <jonathan.cameron@huawei.com>
> > Subject: Re: [PATCH v2 00/24] staging:iio:cdc:ad7150: cleanup / fixup / graduate
> > 
> > On Tue, 30 Mar 2021 21:23:59 +0000
> > "Song Bao Hua (Barry Song)" <song.bao.hua@hisilicon.com> wrote:
> >   
> > > > -----Original Message-----
> > > > From: Jonathan Cameron [mailto:jic23@jic23.retrosnub.co.uk]
> > > > Sent: Tuesday, March 30, 2021 4:36 AM
> > > > To: linux-iio@vger.kernel.org; Song Bao Hua (Barry Song)
> > > > <song.bao.hua@hisilicon.com>
> > > > Cc: Lars-Peter Clausen <lars@metafoo.de>; Michael Hennerich
> > > > <Michael.Hennerich@analog.com>; Alexandru Ardelean  
> > <aardelean@deviqon.com>;  
> > > > Robh+dt@kernel.org; Alexandru Ardelean <ardeleanalex@gmail.com>; Jonathan
> > > > Cameron <jonathan.cameron@huawei.com>
> > > > Subject: Re: [PATCH v2 00/24] staging:iio:cdc:ad7150: cleanup / fixup /  
> > graduate  
> > > >
> > > > On Mon, 29 Mar 2021 16:30:21 +0100
> > > > Jonathan Cameron <jic23@kernel.org> wrote:
> > > >  
> > > > > Hi All,
> > > > >
> > > > > Whilst I'll give up at some point and just apply this without additional
> > > > > tags I really don't like doing that as I've made too many idiot mistakes
> > > > > in the past.
> > > > >
> > > > > Some of these are fairly trivial so please can people take a look if
> > > > > they have a chance. Rob did the DT one (thanks!) so we are down to...
> > > > >  
> > > > > >
> > > > > > 12 - The big irq rework patch. Alex wasn't quite happy to give a tag
> > > > > >      on that last time, but didn't mention anything specific. It's a  
> > bit  
> > > > > >      fiddly so fair enough!  
> > > > >
> > > > > (it's not that bad!)
> > > > >  
> > > >
> > > > 20 had an ack that I think was accidentally sent off list and hence burried
> > > > in my junk. And then there were two...
> > > >  
> > > > > >
> > > > > > 21 - ABI patch.  I don't think there is anything controversial in here
> > > > > >      but it's not gained any tags yet.
> > > > > >  
> > > > >
> > > > > Perhaps didn't help that I accidentally didn't cc Barry on v2.
> > > > >
> > > > > Thanks,
> > > > >
> > > > > Jonathan
> > > > >
> > > > >
> > > > >  
> > > > > > v1 description:
> > > > > >
> > > > > > This is an 'old' driver in IIO that has been in staging a while.
> > > > > > First submitted in October 2010.
> > > > > >
> > > > > > I wanted to try and experiment and picked this driver to try it with.
> > > > > >
> > > > > > The cleanup etc here was all tested against some basic emulation
> > > > > > added to QEMU rather than real hardware.  Once I've cleaned that up
> > > > > > a tiny bit I'll push it up to https://github.com/jic23/qemu
> > > > > > Note that for now I'm not proposing to upstream this to QEMU but
> > > > > > would be interested in hearing if people thing it is a good idea to
> > > > > > do so.
> > > > > >
> > > > > > Whilst it's obviously hard to be absolutely sure that the emulation  
> > is  
> > > > > > correct, the fact that the original driver worked as expected and the
> > > > > > cleaned up version still does is certainly encouraging.
> > > > > >
> > > > > > Note however, that there were a few more significant changes in here  
> > than  
> > > > > > basic cleanup.
> > > > > > 1. Interrupts / events were handled in a rather esoteric fashion.
> > > > > >    (Always on, window modes represented as magnitudes).
> > > > > >    Note that for two channel devices there are separate lines. The original
> > > > > >    driver was not supporting this at all.
> > > > > >    They now look more like a standard IIO driver and reflect experience
> > > > > >    that we've gained over the years in dealing with devices where these
> > > > > >    aren't interrupt lines as such, but rather reporters of current status.
> > > > > > 2. Timeouts were handled in a fashion that clearly wouldn't work.
> > > > > >
> > > > > > Note that this moving out of staging makes a few bits of ABI 'official'
> > > > > > and so those are added to the main IIO ABI Docs.
> > > > > >
> > > > > > Thanks in advance to anyone who has time to take a look.
> > > > > >
> > > > > > Jonathan Cameron (24):
> > > > > >   staging:iio:cdc:ad7150: use swapped reads for i2c rather than open
> > > > > >     coding.  
> > >
> > > Using i2c_smbus_read_word_swapper and i2c_smbus_write_word_swapped
> > > looks good to me. The only thing is that your changelog didn't
> > > mention you also used swapper write as you said "use swapped
> > > reads". Otherwise,
> > >
> > > Reviewed-by: Barry Song <song.bao.hua@hisilicon>
> > >  
> > > > > >   staging:iio:cdc:ad7150: Remove magnitude adaptive events
> > > > > >   staging:iio:cdc:ad7150: Refactor event parameter update
> > > > > >   staging:iio:cdc:ad7150: Timeout register covers both directions so
> > > > > >     both need updating
> > > > > >   staging:iio:cdc:ad7150: Drop platform data support
> > > > > >   staging:iio:cdc:ad7150: Handle variation in chan_spec across device
> > > > > >     and irq present or not
> > > > > >   staging:iio:cdc:ad7150: Simplify event handling by only using rising
> > > > > >     direction.
> > > > > >   staging:iio:cdc:ad7150: Drop noisy print in probe
> > > > > >   staging:iio:cdc:ad7150: Add sampling_frequency support
> > > > > >   iio:event: Add timeout event info type
> > > > > >   staging:iio:cdc:ad7150: Change timeout units to seconds and use core
> > > > > >     support
> > > > > >   staging:iio:cdc:ad7150: Rework interrupt handling.  
> > >
> > > +	/*
> > > +	 * 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;
> > > +
> > >
> > > I am not sure what kind of race conditions we have since disable_irq()
> > > will synchronize with the irq handler.
> > >
> > > If we are in an interrupt, the user who calls disable_irq will wait
> > > for the completion of irq handler.
> > > If an interrupt comes in the gap of disable_irq and enable_irq, we should
> > > have a valid int_status after we re-enable the interrupt?
> > >
> > > Maybe it is because of the weird behavior of the hardware?  
> > 
> > I think you are right and I was being overly paranoid on that one. I'll drop
> > the comment, though I'll leave the paranoid check.
> >   
> 
> Yes. Make sense.
> 
> > >  
> > > > > >   staging:iio:cdc:ad7150: More consistent register and field naming
> > > > > >   staging:iio:cdc:ad7150: Reorganize headers.
> > > > > >   staging:iio:cdc:ad7150: Tidy up local variable positioning.
> > > > > >   staging:iio:cdc:ad7150: Drop unnecessary block comments.
> > > > > >   staging:iio:cdc:ad7150: Shift the _raw readings by 4 bits.
> > > > > >   staging:iio:cdc:ad7150: Add scale and offset to
> > > > > >     info_mask_shared_by_type
> > > > > >   staging:iio:cdc:ad7150: Really basic regulator support.
> > > > > >   staging:iio:cdc:ad7150: Add of_match_table  
> > >
> > > Reviewed-by: Barry Song <song.bao.hua@hisilicon>
> > >  
> > > > > >   iio:Documentation:ABI Add missing elements as used by the adi,ad7150  
> > >
> > > +What:		/sys/.../events/in_capacitanceY_adaptive_thresh_rising_en
> > > +What:		/sys/.../events/in_capacitanceY_adaptive_thresh_falling_en
> > > +KernelVersion:	5.11
> > >
> > > Is kernel 5.11 the right version? Guess not :-)  
> > Oops Can tell this took a while.  I'll tidy that up whilst applying.  
> > >  
> > > > > >   staging:iio:cdc:ad7150: Add copyright notice given substantial
> > > > > >     changes.
> > > > > >   dt-bindings:iio:cdc:adi,ad7150 binding doc  
> > >
> > > Reviewed-by: Barry Song <song.bao.hua@hisilicon>
> > >
> > > +      properties:
> > > +        compatible:
> > > +          contains:
> > > +            enum:
> > > +              - adi,ad7150
> > > +              - adi,ad7156
> > > +    then:
> > > +      properties:
> > > +        interrupts:
> > > +          minItems: 2
> > > +          maxItems: 2
> > > +  - if:
> > > +      properties:
> > > +        compatible:
> > > +          contains:
> > > +            const: adi,ad7151
> > > +    then:
> > > +      properties:
> > > +        interrupts:
> > > +          minItems: 1
> > > +          maxItems: 1
> > >
> > > A further follow-up might be:
> > > we can move to read two irq number for adi7150/7156
> > > and one irq number for adi7151 in driver?  
> > 
> > Unless I'm miss understanding you I think we already do.
> > There is a check against the driver_data being AD7150 which is set for
> > the two parts that have 2 interrupts.  If we don't get that we don't
> > query the second irq number.  
> 
> Sorry for missing that part.
> 
> >   
> > >  
> > > > > >   iio:cdc:ad7150: Move driver out of staging.
> > > > > >
> > > > > >  Documentation/ABI/testing/sysfs-bus-iio       |  33 +
> > > > > >  .../bindings/iio/cdc/adi,ad7150.yaml          |  69 ++
> > > > > >  drivers/iio/Kconfig                           |   1 +
> > > > > >  drivers/iio/Makefile                          |   1 +
> > > > > >  drivers/iio/cdc/Kconfig                       |  17 +
> > > > > >  drivers/iio/cdc/Makefile                      |   6 +
> > > > > >  drivers/iio/cdc/ad7150.c                      | 678 ++++++++++++++++++
> > > > > >  drivers/iio/industrialio-event.c              |   1 +
> > > > > >  drivers/staging/iio/cdc/Kconfig               |  10 -
> > > > > >  drivers/staging/iio/cdc/Makefile              |   3 +-
> > > > > >  drivers/staging/iio/cdc/ad7150.c              | 655 -----------------
> > > > > >  include/linux/iio/types.h                     |   1 +
> > > > > >  12 files changed, 808 insertions(+), 667 deletions(-)
> > > > > >  create mode 100644  
> > > > Documentation/devicetree/bindings/iio/cdc/adi,ad7150.yaml  
> > > > > >  create mode 100644 drivers/iio/cdc/Kconfig
> > > > > >  create mode 100644 drivers/iio/cdc/Makefile
> > > > > >  create mode 100644 drivers/iio/cdc/ad7150.c
> > > > > >  delete mode 100644 drivers/staging/iio/cdc/ad7150.c
> > > > > >  
> > > > >  
> > >
> > >  
> Thanks
> Barry
>