[PATCH v6 00/10] iio: adc: ad7124: Various fixes

[PATCH v6 00/10] iio: adc: ad7124: Various fixes

[Uwe Kleine-König]

Hello,

here comes v6 of this series. Compared to v5
(https://lore.kernel.org/linux-iio/20241203110019.1520071-12-u.kleine-koenig@baylibre.com)
the following things changed:

 - Rebased to v6.13-rc1 + 64612ec9b909. (No changes needed here.)

 - Add Ack from Conor to patch #3

 - Fixed how R̅D̅Y̅ is written. This was wrong before because the overline
   char must be added after the character that should get the overline,
   not before. I got that wrong because of
   https://bugs.debian.org/1089108. I would expect that now this is
   properly shown in most browsers, MUAs and editors.

   I guess Andy still doesn't agree to write it that way. Jonathan,
   would you decide here please? If you agree with Andy I suggest to
   replace it by #RDY. Andy suggested #RDY_N which I think is too far
   away from the original name. If you (also) like R̅D̅Y̅, just keep it as
   is.

 - Fix error handling in patch #8
   I just pasted "return ret" to all callers of
   ad_sigma_delta_clear_pending_event() before. Now the error handling
   matches the actual needs of the context.

 - s/irq controller's capabilities/irq controller capabilities/
   as suggested by Andy for patch #8

Best regards
Uwe

Uwe Kleine-König (10):
  iio: adc: ad7124: Don't create more channels than the driver can handle
  iio: adc: ad7124: Refuse invalid input specifiers
  dt-bindings: iio: adc: adi,ad7{124,173,192,780}: Allow specifications of a gpio for irq line
  iio: adc: ad_sigma_delta: Add support for reading irq status using a GPIO
  iio: adc: ad_sigma_delta: Handle CS assertion as intended in ad_sd_read_reg_raw()
  iio: adc: ad_sigma_delta: Fix a race condition
  iio: adc: ad_sigma_delta: Store information about reset sequence length
  iio: adc: ad_sigma_delta: Check for previous ready signals
  iio: adc: ad7124: Add error reporting during probe
  iio: adc: ad7124: Implement temperature measurement

 .../bindings/iio/adc/adi,ad7124.yaml          |  13 ++
 .../bindings/iio/adc/adi,ad7173.yaml          |  12 +
 .../bindings/iio/adc/adi,ad7192.yaml          |  15 ++
 .../bindings/iio/adc/adi,ad7780.yaml          |  11 +
 drivers/iio/adc/ad7124.c                      | 217 +++++++++++++-----
 drivers/iio/adc/ad7173.c                      |   1 +
 drivers/iio/adc/ad7192.c                      |   4 +-
 drivers/iio/adc/ad7791.c                      |   1 +
 drivers/iio/adc/ad7793.c                      |   3 +-
 drivers/iio/adc/ad_sigma_delta.c              | 194 +++++++++++++---
 include/linux/iio/adc/ad_sigma_delta.h        |   8 +-
 11 files changed, 390 insertions(+), 89 deletions(-)

base-commit: 40384c840ea1944d7c5a392e8975ed088ecf0b37
prerequisite-patch-id: 617af17fc377a984762c61893b9f2a92ae62213a
-- 
2.45.2

[PATCH v6 01/10] iio: adc: ad7124: Don't create more channels than the driver can handle

[Uwe Kleine-König]

The ad7124-4 and ad7124-8 both support 16 channel registers and assigns
each channel defined in dt statically such a register. While the driver
could be a bit more clever about this, it currently isn't and specifying
more than 16 channels yields broken behaviour. So just refuse to bind in
this situation.

Fixes: b3af341bbd96 ("iio: adc: Add ad7124 support")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/iio/adc/ad7124.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
index 8d94bc2b1cac..5352b26bb391 100644
--- a/drivers/iio/adc/ad7124.c
+++ b/drivers/iio/adc/ad7124.c
@@ -821,6 +821,16 @@ static int ad7124_parse_channel_config(struct iio_dev *indio_dev,
 	if (!st->num_channels)
 		return dev_err_probe(dev, -ENODEV, "no channel children\n");
 
+	/*
+	 * The driver assigns each logical channel defined in the device tree
+	 * statically one channel register. So only accept 16 such logical
+	 * channels to not treat CONFIG_0 (i.e. the register following
+	 * CHANNEL_15) as an additional channel register. The driver could be
+	 * improved to lift this limitation.
+	 */
+	if (st->num_channels > AD7124_MAX_CHANNELS)
+		return dev_err_probe(dev, -EINVAL, "Too many channels defined\n");
+
 	chan = devm_kcalloc(indio_dev->dev.parent, st->num_channels,
 			    sizeof(*chan), GFP_KERNEL);
 	if (!chan)
-- 
2.45.2

[PATCH v6 02/10] iio: adc: ad7124: Refuse invalid input specifiers

[Uwe Kleine-König]

The ad7124-4 has 8 analog inputs; the input select values 8 to 15 are
reserved and not to be used. These are fine for ad7124-8. For both
ad7124-4 and ad7124-8 values bigger than 15 are internal channels that
might appear as inputs in the channels specified in the device
description according to the description of commit f1794fd7bdf7 ("iio:
adc: ad7124: Remove input number limitation"), values bigger than 31
don't fit into the respective register bit field and the driver masked
them to smaller values.

Check for these invalid input specifiers and fail to probe if one is
found.

Fixes: f1794fd7bdf7 ("iio: adc: ad7124: Remove input number limitation")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/iio/adc/ad7124.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
index 5352b26bb391..1f3342373f1c 100644
--- a/drivers/iio/adc/ad7124.c
+++ b/drivers/iio/adc/ad7124.c
@@ -807,6 +807,19 @@ static int ad7124_check_chip_id(struct ad7124_state *st)
 	return 0;
 }
 
+/*
+ * Input specifiers 8 - 15 are explicitly reserved for ad7124-4
+ * while they are fine for ad7124-8. Values above 31 don't fit
+ * into the register field and so are invalid for sure.
+ */
+static bool ad7124_valid_input_select(unsigned int ain, const struct ad7124_chip_info *info)
+{
+	if (ain >= info->num_inputs && ain < 16)
+		return false;
+
+	return ain <= FIELD_MAX(AD7124_CHANNEL_AINM_MSK);
+}
+
 static int ad7124_parse_channel_config(struct iio_dev *indio_dev,
 				       struct device *dev)
 {
@@ -859,6 +872,11 @@ static int ad7124_parse_channel_config(struct iio_dev *indio_dev,
 		if (ret)
 			return ret;
 
+		if (!ad7124_valid_input_select(ain[0], st->chip_info) ||
+		    !ad7124_valid_input_select(ain[1], st->chip_info))
+			return dev_err_probe(dev, -EINVAL,
+					     "diff-channels property of %pfwP contains invalid data\n", child);
+
 		st->channels[channel].nr = channel;
 		st->channels[channel].ain = AD7124_CHANNEL_AINP(ain[0]) |
 						  AD7124_CHANNEL_AINM(ain[1]);
-- 
2.45.2

[PATCH v6 03/10] dt-bindings: iio: adc: adi,ad7{124,173,192,780}: Allow specifications of a gpio for irq line

[Uwe Kleine-König]

For the AD7124 chip and some of its cousins the logical irq line (R̅D̅Y̅)
is physically on the same pin as the spi MISO output (DOUT) and so
reading a register might trigger an interrupt. For correct operation
it's critical that the actual state of the pin can be read to judge if
an interrupt event is a real one or just a spurious one triggered by
toggling the line in its MISO mode.

Allow specification of an "rdy-gpios" property that references a GPIO
that can be used for that purpose. While this is typically the same GPIO
also used (implicitly) as interrupt source, it is still supposed that
the interrupt is specified as before and usual.

Acked-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 .../devicetree/bindings/iio/adc/adi,ad7124.yaml   | 13 +++++++++++++
 .../devicetree/bindings/iio/adc/adi,ad7173.yaml   | 12 ++++++++++++
 .../devicetree/bindings/iio/adc/adi,ad7192.yaml   | 15 +++++++++++++++
 .../devicetree/bindings/iio/adc/adi,ad7780.yaml   | 11 +++++++++++
 4 files changed, 51 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7124.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7124.yaml
index 35ed04350e28..7146a654ae38 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,ad7124.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7124.yaml
@@ -37,6 +37,17 @@ properties:
     description: IRQ line for the ADC
     maxItems: 1
 
+  rdy-gpios:
+    description:
+      GPIO reading the R̅D̅Y̅ line. Having such a GPIO is technically optional but
+      highly recommended because DOUT/R̅D̅Y̅ toggles during SPI transfers (in its
+      DOUT aka MISO role) and so usually triggers a spurious interrupt. The
+      distinction between such a spurious event and a real one can only be done
+      by reading such a GPIO. (There is a register telling the same
+      information, but accessing that one needs a SPI transfer which then
+      triggers another interrupt event.)
+    maxItems: 1
+
   '#address-cells':
     const: 1
 
@@ -111,6 +122,7 @@ unevaluatedProperties: false
 
 examples:
   - |
+    #include <dt-bindings/gpio/gpio.h>
     spi {
       #address-cells = <1>;
       #size-cells = <0>;
@@ -121,6 +133,7 @@ examples:
         spi-max-frequency = <5000000>;
         interrupts = <25 2>;
         interrupt-parent = <&gpio>;
+        rdy-gpios = <&gpio 25 GPIO_ACTIVE_LOW>;
         refin1-supply = <&adc_vref>;
         clocks = <&ad7124_mclk>;
         clock-names = "mclk";
diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
index ad15cf9bc2ff..21ee319d4675 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
@@ -135,6 +135,17 @@ properties:
   '#clock-cells':
     const: 0
 
+  rdy-gpios:
+    description:
+      GPIO reading the R̅D̅Y̅ line. Having such a GPIO is technically optional but
+      highly recommended because DOUT/R̅D̅Y̅ toggles during SPI transfers (in its
+      DOUT aka MISO role) and so usually triggers a spurious interrupt. The
+      distinction between such a spurious event and a real one can only be done
+      by reading such a GPIO. (There is a register telling the same
+      information, but accessing that one needs a SPI transfer which then
+      triggers another interrupt event.)
+    maxItems: 1
+
 patternProperties:
   "^channel@[0-9a-f]$":
     type: object
@@ -443,6 +454,7 @@ examples:
         interrupts = <25 IRQ_TYPE_EDGE_FALLING>;
         interrupt-names = "rdy";
         interrupt-parent = <&gpio>;
+        rdy-gpios = <&gpio 25 GPIO_ACTIVE_LOW>;
         spi-max-frequency = <5000000>;
         gpio-controller;
         #gpio-cells = <2>;
diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
index 66dd1c549bd3..0bd2c6906c83 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
@@ -106,6 +106,17 @@ properties:
     description: see Documentation/devicetree/bindings/iio/adc/adc.yaml
     type: boolean
 
+  rdy-gpios:
+    description:
+      GPIO reading the R̅D̅Y̅ line. Having such a GPIO is technically optional but
+      highly recommended because DOUT/R̅D̅Y̅ toggles during SPI transfers (in its
+      DOUT aka MISO role) and so usually triggers a spurious interrupt. The
+      distinction between such a spurious event and a real one can only be done
+      by reading such a GPIO. (There is a register telling the same
+      information, but accessing that one needs a SPI transfer which then
+      triggers another interrupt event.)
+    maxItems: 1
+
 patternProperties:
   "^channel@[0-9a-f]+$":
     type: object
@@ -181,6 +192,7 @@ unevaluatedProperties: false
 
 examples:
   - |
+    #include <dt-bindings/gpio/gpio.h>
     spi {
         #address-cells = <1>;
         #size-cells = <0>;
@@ -195,6 +207,7 @@ examples:
             clock-names = "mclk";
             interrupts = <25 0x2>;
             interrupt-parent = <&gpio>;
+            rdy-gpios = <&gpio 25 GPIO_ACTIVE_LOW>;
             aincom-supply = <&aincom>;
             dvdd-supply = <&dvdd>;
             avdd-supply = <&avdd>;
@@ -207,6 +220,7 @@ examples:
         };
     };
   - |
+    #include <dt-bindings/gpio/gpio.h>
     spi {
         #address-cells = <1>;
         #size-cells = <0>;
@@ -224,6 +238,7 @@ examples:
             #clock-cells = <0>;
             interrupts = <25 0x2>;
             interrupt-parent = <&gpio>;
+            rdy-gpios = <&gpio 25 GPIO_ACTIVE_LOW>;
             aincom-supply = <&aincom>;
             dvdd-supply = <&dvdd>;
             avdd-supply = <&avdd>;
diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7780.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7780.yaml
index be2616ff9af6..5c8df45bfab0 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,ad7780.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7780.yaml
@@ -63,6 +63,17 @@ properties:
       marked GPIO_ACTIVE_LOW.
     maxItems: 1
 
+  rdy-gpios:
+    description:
+      GPIO reading the R̅D̅Y̅ line. Having such a GPIO is technically optional but
+      highly recommended because DOUT/R̅D̅Y̅ toggles during SPI transfers (in its
+      DOUT aka MISO role) and so usually triggers a spurious interrupt. The
+      distinction between such a spurious event and a real one can only be done
+      by reading such a GPIO. (There is a register telling the same
+      information, but accessing that one needs a SPI transfer which then
+      triggers another interrupt event.)
+    maxItems: 1
+
 required:
   - compatible
   - reg
-- 
2.45.2

[PATCH v6 04/10] iio: adc: ad_sigma_delta: Add support for reading irq status using a GPIO

[Uwe Kleine-König]

Some of the ADCs by Analog signal their irq condition on the MISO line.
So typically that line is connected to an SPI controller and a GPIO. The
GPIO is used as input and the respective interrupt is enabled when the
last SPI transfer is completed.

Depending on the GPIO controller the toggling MISO line might make the
interrupt pending even while it's masked. In that case the irq handler
is called immediately after irq_enable() and so before the device
actually pulls that line low which results in non-sense values being
reported to the upper layers.

The only way to find out if the line was actually pulled low is to read
the GPIO. (There is a flag in AD7124's status register that also signals
if an interrupt was asserted, but reading that register toggles the MISO
line and so might trigger another spurious interrupt.)

Add the possibility to specify an interrupt GPIO in the machine
description in addition to the plain interrupt. This GPIO is used then
to check if the irq line is actually active in the irq handler.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/iio/adc/ad_sigma_delta.c       | 38 ++++++++++++++++++++++----
 include/linux/iio/adc/ad_sigma_delta.h |  2 ++
 2 files changed, 35 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
index 2f3b61765055..f25850f4965a 100644
--- a/drivers/iio/adc/ad_sigma_delta.c
+++ b/drivers/iio/adc/ad_sigma_delta.c
@@ -539,12 +539,29 @@ static irqreturn_t ad_sd_data_rdy_trig_poll(int irq, void *private)
 {
 	struct ad_sigma_delta *sigma_delta = private;
 
-	complete(&sigma_delta->completion);
-	disable_irq_nosync(irq);
-	sigma_delta->irq_dis = true;
-	iio_trigger_poll(sigma_delta->trig);
+	/*
+	 * AD7124 and a few others use the same physical line for interrupt
+	 * reporting (R̅D̅Y̅) and MISO.
+	 * As MISO toggles when reading a register, this likely results in a
+	 * pending interrupt. This has two consequences: a) The irq might
+	 * trigger immediately after it's enabled even though the conversion
+	 * isn't done yet; and b) checking the STATUS register's R̅D̅Y̅ flag is
+	 * off-limits as reading that would trigger another irq event.
+	 *
+	 * So read the MOSI line as GPIO (if available) and only trigger the irq
+	 * if the line is active. Without such a GPIO assume this is a valid
+	 * interrupt.
+	 */
+	if (!sigma_delta->rdy_gpiod || gpiod_get_value(sigma_delta->rdy_gpiod)) {
+		complete(&sigma_delta->completion);
+		disable_irq_nosync(irq);
+		sigma_delta->irq_dis = true;
+		iio_trigger_poll(sigma_delta->trig);
 
-	return IRQ_HANDLED;
+		return IRQ_HANDLED;
+	}
+
+	return IRQ_NONE;
 }
 
 /**
@@ -679,6 +696,17 @@ int ad_sd_init(struct ad_sigma_delta *sigma_delta, struct iio_dev *indio_dev,
 	else
 		sigma_delta->irq_line = spi->irq;
 
+	sigma_delta->rdy_gpiod = devm_gpiod_get_optional(&spi->dev, "rdy", GPIOD_IN);
+	if (IS_ERR(sigma_delta->rdy_gpiod))
+		return dev_err_probe(&spi->dev, PTR_ERR(sigma_delta->rdy_gpiod),
+				     "Failed to find rdy gpio\n");
+
+	if (sigma_delta->rdy_gpiod && !sigma_delta->irq_line) {
+		sigma_delta->irq_line = gpiod_to_irq(sigma_delta->rdy_gpiod);
+		if (sigma_delta->irq_line < 0)
+			return sigma_delta->irq_line;
+	}
+
 	iio_device_set_drvdata(indio_dev, sigma_delta);
 
 	return 0;
diff --git a/include/linux/iio/adc/ad_sigma_delta.h b/include/linux/iio/adc/ad_sigma_delta.h
index f8c1d2505940..126b187d70e9 100644
--- a/include/linux/iio/adc/ad_sigma_delta.h
+++ b/include/linux/iio/adc/ad_sigma_delta.h
@@ -29,6 +29,7 @@ struct ad_sd_calib_data {
 
 struct ad_sigma_delta;
 struct device;
+struct gpio_desc;
 struct iio_dev;
 
 /**
@@ -96,6 +97,7 @@ struct ad_sigma_delta {
 	unsigned int		active_slots;
 	unsigned int		current_slot;
 	unsigned int		num_slots;
+	struct gpio_desc	*rdy_gpiod;
 	int		irq_line;
 	bool			status_appended;
 	/* map slots to channels in order to know what to expect from devices */
-- 
2.45.2

[PATCH v6 05/10] iio: adc: ad_sigma_delta: Handle CS assertion as intended in ad_sd_read_reg_raw()

[Uwe Kleine-König]

When struct ad_sigma_delta::keep_cs_asserted was introduced only
register writing was adapted to honor this new flag. Also respect it
when reading a register.

Fixes: df1d80aee963 ("iio: ad_sigma_delta: Properly handle SPI bus locking vs CS assertion")
Reviewed-by: Trevor Gamblin <tgamblin@baylibre.com>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/iio/adc/ad_sigma_delta.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
index f25850f4965a..ff20fa61c293 100644
--- a/drivers/iio/adc/ad_sigma_delta.c
+++ b/drivers/iio/adc/ad_sigma_delta.c
@@ -109,7 +109,7 @@ static int ad_sd_read_reg_raw(struct ad_sigma_delta *sigma_delta,
 		}, {
 			.rx_buf = val,
 			.len = size,
-			.cs_change = sigma_delta->bus_locked,
+			.cs_change = sigma_delta->keep_cs_asserted,
 		},
 	};
 	struct spi_message m;
-- 
2.45.2

[PATCH v6 06/10] iio: adc: ad_sigma_delta: Fix a race condition

[Uwe Kleine-König]

The ad_sigma_delta driver helper uses irq_disable_nosync(). With that
one it is possible that the irq handler still runs after the
irq_disable_nosync() function call returns. Also to properly synchronize
irq disabling in the different threads proper locking is needed and
because it's unclear if the irq handler's irq_disable_nosync() call
comes first or the one in the enabler's error path, all code locations
that disable the irq must check for .irq_dis first to ensure there is
exactly one disable call per enable call.

So add a spinlock to the struct ad_sigma_delta and use it to synchronize
irq enabling and disabling. Also only act in the irq handler if the irq
is still enabled.

Fixes: af3008485ea0 ("iio:adc: Add common code for ADI Sigma Delta devices")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/iio/adc/ad_sigma_delta.c       | 56 ++++++++++++++++----------
 include/linux/iio/adc/ad_sigma_delta.h |  1 +
 2 files changed, 36 insertions(+), 21 deletions(-)

diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
index ff20fa61c293..a4c31baa9c9e 100644
--- a/drivers/iio/adc/ad_sigma_delta.c
+++ b/drivers/iio/adc/ad_sigma_delta.c
@@ -202,6 +202,27 @@ int ad_sd_reset(struct ad_sigma_delta *sigma_delta,
 }
 EXPORT_SYMBOL_NS_GPL(ad_sd_reset, IIO_AD_SIGMA_DELTA);
 
+static bool ad_sd_disable_irq(struct ad_sigma_delta *sigma_delta)
+{
+	guard(spinlock_irqsave)(&sigma_delta->irq_lock);
+
+	/* It's already off, return false to indicate nothing was changed */
+	if (sigma_delta->irq_dis)
+		return false;
+
+	sigma_delta->irq_dis = true;
+	disable_irq_nosync(sigma_delta->irq_line);
+	return true;
+}
+
+static void ad_sd_enable_irq(struct ad_sigma_delta *sigma_delta)
+{
+	guard(spinlock_irqsave)(&sigma_delta->irq_lock);
+
+	sigma_delta->irq_dis = false;
+	enable_irq(sigma_delta->irq_line);
+}
+
 int ad_sd_calibrate(struct ad_sigma_delta *sigma_delta,
 	unsigned int mode, unsigned int channel)
 {
@@ -221,12 +242,10 @@ int ad_sd_calibrate(struct ad_sigma_delta *sigma_delta,
 	if (ret < 0)
 		goto out;
 
-	sigma_delta->irq_dis = false;
-	enable_irq(sigma_delta->irq_line);
+	ad_sd_enable_irq(sigma_delta);
 	time_left = wait_for_completion_timeout(&sigma_delta->completion, 2 * HZ);
 	if (time_left == 0) {
-		sigma_delta->irq_dis = true;
-		disable_irq_nosync(sigma_delta->irq_line);
+		ad_sd_disable_irq(sigma_delta);
 		ret = -EIO;
 	} else {
 		ret = 0;
@@ -294,8 +313,7 @@ int ad_sigma_delta_single_conversion(struct iio_dev *indio_dev,
 
 	ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_SINGLE);
 
-	sigma_delta->irq_dis = false;
-	enable_irq(sigma_delta->irq_line);
+	ad_sd_enable_irq(sigma_delta);
 	ret = wait_for_completion_interruptible_timeout(
 			&sigma_delta->completion, HZ);
 
@@ -314,10 +332,7 @@ int ad_sigma_delta_single_conversion(struct iio_dev *indio_dev,
 		&raw_sample);
 
 out:
-	if (!sigma_delta->irq_dis) {
-		disable_irq_nosync(sigma_delta->irq_line);
-		sigma_delta->irq_dis = true;
-	}
+	ad_sd_disable_irq(sigma_delta);
 
 	sigma_delta->keep_cs_asserted = false;
 	ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_IDLE);
@@ -396,8 +411,7 @@ static int ad_sd_buffer_postenable(struct iio_dev *indio_dev)
 	if (ret)
 		goto err_unlock;
 
-	sigma_delta->irq_dis = false;
-	enable_irq(sigma_delta->irq_line);
+	ad_sd_enable_irq(sigma_delta);
 
 	return 0;
 
@@ -414,10 +428,7 @@ static int ad_sd_buffer_postdisable(struct iio_dev *indio_dev)
 	reinit_completion(&sigma_delta->completion);
 	wait_for_completion_timeout(&sigma_delta->completion, HZ);
 
-	if (!sigma_delta->irq_dis) {
-		disable_irq_nosync(sigma_delta->irq_line);
-		sigma_delta->irq_dis = true;
-	}
+	ad_sd_disable_irq(sigma_delta);
 
 	sigma_delta->keep_cs_asserted = false;
 	ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_IDLE);
@@ -516,8 +527,7 @@ static irqreturn_t ad_sd_trigger_handler(int irq, void *p)
 
 irq_handled:
 	iio_trigger_notify_done(indio_dev->trig);
-	sigma_delta->irq_dis = false;
-	enable_irq(sigma_delta->irq_line);
+	ad_sd_enable_irq(sigma_delta);
 
 	return IRQ_HANDLED;
 }
@@ -551,11 +561,13 @@ static irqreturn_t ad_sd_data_rdy_trig_poll(int irq, void *private)
 	 * So read the MOSI line as GPIO (if available) and only trigger the irq
 	 * if the line is active. Without such a GPIO assume this is a valid
 	 * interrupt.
+	 *
+	 * Also as disable_irq_nosync() is used to disable the irq, only act if
+	 * the irq wasn't disabled before.
 	 */
-	if (!sigma_delta->rdy_gpiod || gpiod_get_value(sigma_delta->rdy_gpiod)) {
+	if ((!sigma_delta->rdy_gpiod || gpiod_get_value(sigma_delta->rdy_gpiod)) &&
+	    ad_sd_disable_irq(sigma_delta)) {
 		complete(&sigma_delta->completion);
-		disable_irq_nosync(irq);
-		sigma_delta->irq_dis = true;
 		iio_trigger_poll(sigma_delta->trig);
 
 		return IRQ_HANDLED;
@@ -691,6 +703,8 @@ int ad_sd_init(struct ad_sigma_delta *sigma_delta, struct iio_dev *indio_dev,
 		}
 	}
 
+	spin_lock_init(&sigma_delta->irq_lock);
+
 	if (info->irq_line)
 		sigma_delta->irq_line = info->irq_line;
 	else
diff --git a/include/linux/iio/adc/ad_sigma_delta.h b/include/linux/iio/adc/ad_sigma_delta.h
index 126b187d70e9..f86eca6126b4 100644
--- a/include/linux/iio/adc/ad_sigma_delta.h
+++ b/include/linux/iio/adc/ad_sigma_delta.h
@@ -86,6 +86,7 @@ struct ad_sigma_delta {
 
 /* private: */
 	struct completion	completion;
+	spinlock_t		irq_lock; /* protects .irq_dis and irq en/disable state */
 	bool			irq_dis;
 
 	bool			bus_locked;
-- 
2.45.2

Re: [PATCH v6 06/10] iio: adc: ad_sigma_delta: Fix a race condition

[Jonathan Cameron]

On Fri,  6 Dec 2024 18:28:38 +0100
Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:

> The ad_sigma_delta driver helper uses irq_disable_nosync(). With that
> one it is possible that the irq handler still runs after the
> irq_disable_nosync() function call returns. Also to properly synchronize
> irq disabling in the different threads proper locking is needed and
> because it's unclear if the irq handler's irq_disable_nosync() call
> comes first or the one in the enabler's error path, all code locations
> that disable the irq must check for .irq_dis first to ensure there is
> exactly one disable call per enable call.
> 
> So add a spinlock to the struct ad_sigma_delta and use it to synchronize
> irq enabling and disabling. Also only act in the irq handler if the irq
> is still enabled.
> 
> Fixes: af3008485ea0 ("iio:adc: Add common code for ADI Sigma Delta devices")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>

From sparse.

drivers/iio/adc/ad_sigma_delta.c:205:13: warning: context imbalance in 'ad_sd_disable_irq' - wrong count at exit
drivers/iio/adc/ad_sigma_delta.c:218:13: warning: context imbalance in 'ad_sd_enable_irq' - wrong count at exit

I saw your discussion with Linus on this...

https://lore.kernel.org/all/CAHk-=wiVDZejo_1BhOaR33qb=pny7sWnYtP4JUbRTXkXCkW6jA@mail.gmail.com/

So I guess we just treat that as a false positive and move on.

> ---
>  drivers/iio/adc/ad_sigma_delta.c       | 56 ++++++++++++++++----------
>  include/linux/iio/adc/ad_sigma_delta.h |  1 +
>  2 files changed, 36 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
> index ff20fa61c293..a4c31baa9c9e 100644
> --- a/drivers/iio/adc/ad_sigma_delta.c
> +++ b/drivers/iio/adc/ad_sigma_delta.c
> @@ -202,6 +202,27 @@ int ad_sd_reset(struct ad_sigma_delta *sigma_delta,
>  }
>  EXPORT_SYMBOL_NS_GPL(ad_sd_reset, IIO_AD_SIGMA_DELTA);
>  
> +static bool ad_sd_disable_irq(struct ad_sigma_delta *sigma_delta)
> +{
> +	guard(spinlock_irqsave)(&sigma_delta->irq_lock);
> +
> +	/* It's already off, return false to indicate nothing was changed */
> +	if (sigma_delta->irq_dis)
> +		return false;
> +
> +	sigma_delta->irq_dis = true;
> +	disable_irq_nosync(sigma_delta->irq_line);
> +	return true;
> +}
> +
> +static void ad_sd_enable_irq(struct ad_sigma_delta *sigma_delta)
> +{
> +	guard(spinlock_irqsave)(&sigma_delta->irq_lock);
> +
> +	sigma_delta->irq_dis = false;
> +	enable_irq(sigma_delta->irq_line);
> +}
> +
>  int ad_sd_calibrate(struct ad_sigma_delta *sigma_delta,
>  	unsigned int mode, unsigned int channel)
>  {
> @@ -221,12 +242,10 @@ int ad_sd_calibrate(struct ad_sigma_delta *sigma_delta,
>  	if (ret < 0)
>  		goto out;
>  
> -	sigma_delta->irq_dis = false;
> -	enable_irq(sigma_delta->irq_line);
> +	ad_sd_enable_irq(sigma_delta);
>  	time_left = wait_for_completion_timeout(&sigma_delta->completion, 2 * HZ);
>  	if (time_left == 0) {
> -		sigma_delta->irq_dis = true;
> -		disable_irq_nosync(sigma_delta->irq_line);
> +		ad_sd_disable_irq(sigma_delta);
>  		ret = -EIO;
>  	} else {
>  		ret = 0;
> @@ -294,8 +313,7 @@ int ad_sigma_delta_single_conversion(struct iio_dev *indio_dev,
>  
>  	ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_SINGLE);
>  
> -	sigma_delta->irq_dis = false;
> -	enable_irq(sigma_delta->irq_line);
> +	ad_sd_enable_irq(sigma_delta);
>  	ret = wait_for_completion_interruptible_timeout(
>  			&sigma_delta->completion, HZ);
>  
> @@ -314,10 +332,7 @@ int ad_sigma_delta_single_conversion(struct iio_dev *indio_dev,
>  		&raw_sample);
>  
>  out:
> -	if (!sigma_delta->irq_dis) {
> -		disable_irq_nosync(sigma_delta->irq_line);
> -		sigma_delta->irq_dis = true;
> -	}
> +	ad_sd_disable_irq(sigma_delta);
>  
>  	sigma_delta->keep_cs_asserted = false;
>  	ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_IDLE);
> @@ -396,8 +411,7 @@ static int ad_sd_buffer_postenable(struct iio_dev *indio_dev)
>  	if (ret)
>  		goto err_unlock;
>  
> -	sigma_delta->irq_dis = false;
> -	enable_irq(sigma_delta->irq_line);
> +	ad_sd_enable_irq(sigma_delta);
>  
>  	return 0;
>  
> @@ -414,10 +428,7 @@ static int ad_sd_buffer_postdisable(struct iio_dev *indio_dev)
>  	reinit_completion(&sigma_delta->completion);
>  	wait_for_completion_timeout(&sigma_delta->completion, HZ);
>  
> -	if (!sigma_delta->irq_dis) {
> -		disable_irq_nosync(sigma_delta->irq_line);
> -		sigma_delta->irq_dis = true;
> -	}
> +	ad_sd_disable_irq(sigma_delta);
>  
>  	sigma_delta->keep_cs_asserted = false;
>  	ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_IDLE);
> @@ -516,8 +527,7 @@ static irqreturn_t ad_sd_trigger_handler(int irq, void *p)
>  
>  irq_handled:
>  	iio_trigger_notify_done(indio_dev->trig);
> -	sigma_delta->irq_dis = false;
> -	enable_irq(sigma_delta->irq_line);
> +	ad_sd_enable_irq(sigma_delta);
>  
>  	return IRQ_HANDLED;
>  }
> @@ -551,11 +561,13 @@ static irqreturn_t ad_sd_data_rdy_trig_poll(int irq, void *private)
>  	 * So read the MOSI line as GPIO (if available) and only trigger the irq
>  	 * if the line is active. Without such a GPIO assume this is a valid
>  	 * interrupt.
> +	 *
> +	 * Also as disable_irq_nosync() is used to disable the irq, only act if
> +	 * the irq wasn't disabled before.
>  	 */
> -	if (!sigma_delta->rdy_gpiod || gpiod_get_value(sigma_delta->rdy_gpiod)) {
> +	if ((!sigma_delta->rdy_gpiod || gpiod_get_value(sigma_delta->rdy_gpiod)) &&
> +	    ad_sd_disable_irq(sigma_delta)) {
>  		complete(&sigma_delta->completion);
> -		disable_irq_nosync(irq);
> -		sigma_delta->irq_dis = true;
>  		iio_trigger_poll(sigma_delta->trig);
>  
>  		return IRQ_HANDLED;
> @@ -691,6 +703,8 @@ int ad_sd_init(struct ad_sigma_delta *sigma_delta, struct iio_dev *indio_dev,
>  		}
>  	}
>  
> +	spin_lock_init(&sigma_delta->irq_lock);
> +
>  	if (info->irq_line)
>  		sigma_delta->irq_line = info->irq_line;
>  	else
> diff --git a/include/linux/iio/adc/ad_sigma_delta.h b/include/linux/iio/adc/ad_sigma_delta.h
> index 126b187d70e9..f86eca6126b4 100644
> --- a/include/linux/iio/adc/ad_sigma_delta.h
> +++ b/include/linux/iio/adc/ad_sigma_delta.h
> @@ -86,6 +86,7 @@ struct ad_sigma_delta {
>  
>  /* private: */
>  	struct completion	completion;
> +	spinlock_t		irq_lock; /* protects .irq_dis and irq en/disable state */
>  	bool			irq_dis;
>  
>  	bool			bus_locked;

Re: [PATCH v6 06/10] iio: adc: ad_sigma_delta: Fix a race condition

[Andy Shevchenko]

On Sun, Dec 8, 2024 at 2:42 PM Jonathan Cameron <jic23@kernel.org> wrote:
> On Fri,  6 Dec 2024 18:28:38 +0100
> Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:

...

> From sparse.
>
> drivers/iio/adc/ad_sigma_delta.c:205:13: warning: context imbalance in 'ad_sd_disable_irq' - wrong count at exit
> drivers/iio/adc/ad_sigma_delta.c:218:13: warning: context imbalance in 'ad_sd_enable_irq' - wrong count at exit
>
> I saw your discussion with Linus on this...
>
> https://lore.kernel.org/all/CAHk-=wiVDZejo_1BhOaR33qb=pny7sWnYtP4JUbRTXkXCkW6jA@mail.gmail.com/
>
> So I guess we just treat that as a false positive and move on.

I'm wondering if sparse annotation __acquire and __release may help here...

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 06/10] iio: adc: ad_sigma_delta: Fix a race condition

[Jonathan Cameron]

On Sun, 8 Dec 2024 15:05:38 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Sun, Dec 8, 2024 at 2:42 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > On Fri,  6 Dec 2024 18:28:38 +0100
> > Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:  
> 
> ...
> 
> > From sparse.
> >
> > drivers/iio/adc/ad_sigma_delta.c:205:13: warning: context imbalance in 'ad_sd_disable_irq' - wrong count at exit
> > drivers/iio/adc/ad_sigma_delta.c:218:13: warning: context imbalance in 'ad_sd_enable_irq' - wrong count at exit
> >
> > I saw your discussion with Linus on this...
> >
> > https://lore.kernel.org/all/CAHk-=wiVDZejo_1BhOaR33qb=pny7sWnYtP4JUbRTXkXCkW6jA@mail.gmail.com/
> >
> > So I guess we just treat that as a false positive and move on.  
> 
> I'm wondering if sparse annotation __acquire and __release may help here...

The complaint is (I think) about guard(spinlock_irqsave)
so I'm not immediately sure how.

Re: [PATCH v6 06/10] iio: adc: ad_sigma_delta: Fix a race condition

[Andy Shevchenko]

On Sun, Dec 8, 2024 at 8:23 PM Jonathan Cameron <jic23@kernel.org> wrote:
> On Sun, 8 Dec 2024 15:05:38 +0200
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Sun, Dec 8, 2024 at 2:42 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > > On Fri,  6 Dec 2024 18:28:38 +0100

...

> > > From sparse.
> > >
> > > drivers/iio/adc/ad_sigma_delta.c:205:13: warning: context imbalance in 'ad_sd_disable_irq' - wrong count at exit
> > > drivers/iio/adc/ad_sigma_delta.c:218:13: warning: context imbalance in 'ad_sd_enable_irq' - wrong count at exit
> > >
> > > I saw your discussion with Linus on this...
> > >
> > > https://lore.kernel.org/all/CAHk-=wiVDZejo_1BhOaR33qb=pny7sWnYtP4JUbRTXkXCkW6jA@mail.gmail.com/
> > >
> > > So I guess we just treat that as a false positive and move on.
> >
> > I'm wondering if sparse annotation __acquire and __release may help here...
>
> The complaint is (I think) about guard(spinlock_irqsave)
> so I'm not immediately sure how.

In cases like

 if (x)
   lock();
 ...do smth...
if (x)
  unlock()

Those annotations give sparse hints that locking is balanced. That is
why I was thinking it may help guard as well.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 06/10] iio: adc: ad_sigma_delta: Fix a race condition

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 3163 bytes --]

Hello Jonathan,

On Sun, Dec 08, 2024 at 12:42:05PM +0000, Jonathan Cameron wrote:
> On Fri,  6 Dec 2024 18:28:38 +0100
> Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:
> 
> > The ad_sigma_delta driver helper uses irq_disable_nosync(). With that
> > one it is possible that the irq handler still runs after the
> > irq_disable_nosync() function call returns. Also to properly synchronize
> > irq disabling in the different threads proper locking is needed and
> > because it's unclear if the irq handler's irq_disable_nosync() call
> > comes first or the one in the enabler's error path, all code locations
> > that disable the irq must check for .irq_dis first to ensure there is
> > exactly one disable call per enable call.
> > 
> > So add a spinlock to the struct ad_sigma_delta and use it to synchronize
> > irq enabling and disabling. Also only act in the irq handler if the irq
> > is still enabled.
> > 
> > Fixes: af3008485ea0 ("iio:adc: Add common code for ADI Sigma Delta devices")
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> 
> From sparse.
> 
> drivers/iio/adc/ad_sigma_delta.c:205:13: warning: context imbalance in 'ad_sd_disable_irq' - wrong count at exit
> drivers/iio/adc/ad_sigma_delta.c:218:13: warning: context imbalance in 'ad_sd_enable_irq' - wrong count at exit
> 
> I saw your discussion with Linus on this...
> 
> https://lore.kernel.org/all/CAHk-=wiVDZejo_1BhOaR33qb=pny7sWnYtP4JUbRTXkXCkW6jA@mail.gmail.com/
> 
> So I guess we just treat that as a false positive and move on.

Yes, my discussion was about a different driver, but it's the same here.
sparse is happy when applying the following patch:

diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
index d32102b25530..dea4816793fa 100644
--- a/drivers/iio/adc/ad_sigma_delta.c
+++ b/drivers/iio/adc/ad_sigma_delta.c
@@ -206,23 +206,33 @@ EXPORT_SYMBOL_NS_GPL(ad_sd_reset, IIO_AD_SIGMA_DELTA);
 
 static bool ad_sd_disable_irq(struct ad_sigma_delta *sigma_delta)
 {
-	guard(spinlock_irqsave)(&sigma_delta->irq_lock);
+	unsigned long flags;
+
+	spin_lock_irqsave(&sigma_delta->irq_lock, flags);
 
 	/* It's already off, return false to indicate nothing was changed */
-	if (sigma_delta->irq_dis)
+	if (sigma_delta->irq_dis) {
+		spin_unlock_irqrestore(&sigma_delta->irq_lock, flags);
 		return false;
+	}
 
 	sigma_delta->irq_dis = true;
 	disable_irq_nosync(sigma_delta->irq_line);
+	spin_unlock_irqrestore(&sigma_delta->irq_lock, flags);
+
 	return true;
 }
 
 static void ad_sd_enable_irq(struct ad_sigma_delta *sigma_delta)
 {
-	guard(spinlock_irqsave)(&sigma_delta->irq_lock);
+	unsigned long flags;
+
+	spin_lock_irqsave(&sigma_delta->irq_lock, flags);
 
 	sigma_delta->irq_dis = false;
 	enable_irq(sigma_delta->irq_line);
+
+	spin_unlock_irqrestore(&sigma_delta->irq_lock, flags);
 }
 
 #define AD_SD_CLEAR_DATA_BUFLEN 9

which results in equivalent code. Also TTBOMK this can only be fixed by
teaching sparse about the cleanup stuff and an annotation doesn't help.

So yes, please move on (or fix sparse :-)

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v6 07/10] iio: adc: ad_sigma_delta: Store information about reset sequence length

[Uwe Kleine-König]

The various chips can be reset using a sequence of SPI transfers with
MOSI = 1. The length of such a sequence varies from chip to chip. Store
that length in struct ad_sigma_delta_info and replace the respective
parameter to ad_sd_reset() with it.

Note the ad7192 used to pass 48 as length but the documentation
specifies 40 as the required length. Assuming the latter is right.
(Using a too long sequence doesn't hurt apart from using a longer spi
transfer than necessary, so this is no relevant fix.)

The motivation for storing this information is that this is useful to
clear a pending R̅D̅Y̅ signal in the next change.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/iio/adc/ad7124.c               | 3 ++-
 drivers/iio/adc/ad7173.c               | 1 +
 drivers/iio/adc/ad7192.c               | 4 +++-
 drivers/iio/adc/ad7791.c               | 1 +
 drivers/iio/adc/ad7793.c               | 3 ++-
 drivers/iio/adc/ad_sigma_delta.c       | 5 ++---
 include/linux/iio/adc/ad_sigma_delta.h | 5 +++--
 7 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
index 1f3342373f1c..b17c3dbeaeba 100644
--- a/drivers/iio/adc/ad7124.c
+++ b/drivers/iio/adc/ad7124.c
@@ -571,6 +571,7 @@ static const struct ad_sigma_delta_info ad7124_sigma_delta_info = {
 	.data_reg = AD7124_DATA,
 	.num_slots = 8,
 	.irq_flags = IRQF_TRIGGER_FALLING,
+	.num_resetclks = 64,
 };
 
 static int ad7124_read_raw(struct iio_dev *indio_dev,
@@ -756,7 +757,7 @@ static int ad7124_soft_reset(struct ad7124_state *st)
 	unsigned int readval, timeout;
 	int ret;
 
-	ret = ad_sd_reset(&st->sd, 64);
+	ret = ad_sd_reset(&st->sd);
 	if (ret < 0)
 		return ret;
 
diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
index a0fca16c3be0..ca46bb20288d 100644
--- a/drivers/iio/adc/ad7173.c
+++ b/drivers/iio/adc/ad7173.c
@@ -764,6 +764,7 @@ static struct ad_sigma_delta_info ad7173_sigma_delta_info = {
 	.read_mask = BIT(6),
 	.status_ch_mask = GENMASK(3, 0),
 	.data_reg = AD7173_REG_DATA,
+	.num_resetclks = 64,
 };
 
 static int ad7173_setup(struct iio_dev *indio_dev)
diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
index 955e9eff0099..f744441bd13f 100644
--- a/drivers/iio/adc/ad7192.c
+++ b/drivers/iio/adc/ad7192.c
@@ -361,6 +361,7 @@ static const struct ad_sigma_delta_info ad7192_sigma_delta_info = {
 	.status_ch_mask = GENMASK(3, 0),
 	.num_slots = 4,
 	.irq_flags = IRQF_TRIGGER_FALLING,
+	.num_resetclks = 40,
 };
 
 static const struct ad_sigma_delta_info ad7194_sigma_delta_info = {
@@ -373,6 +374,7 @@ static const struct ad_sigma_delta_info ad7194_sigma_delta_info = {
 	.read_mask = BIT(6),
 	.status_ch_mask = GENMASK(3, 0),
 	.irq_flags = IRQF_TRIGGER_FALLING,
+	.num_resetclks = 40,
 };
 
 static const struct ad_sd_calib_data ad7192_calib_arr[8] = {
@@ -565,7 +567,7 @@ static int ad7192_setup(struct iio_dev *indio_dev, struct device *dev)
 	int i, ret, id;
 
 	/* reset the serial interface */
-	ret = ad_sd_reset(&st->sd, 48);
+	ret = ad_sd_reset(&st->sd);
 	if (ret < 0)
 		return ret;
 	usleep_range(500, 1000); /* Wait for at least 500us */
diff --git a/drivers/iio/adc/ad7791.c b/drivers/iio/adc/ad7791.c
index 5d2ad3dd6caa..897e628de611 100644
--- a/drivers/iio/adc/ad7791.c
+++ b/drivers/iio/adc/ad7791.c
@@ -254,6 +254,7 @@ static const struct ad_sigma_delta_info ad7791_sigma_delta_info = {
 	.addr_shift = 4,
 	.read_mask = BIT(3),
 	.irq_flags = IRQF_TRIGGER_FALLING,
+	.num_resetclks = 32,
 };
 
 static int ad7791_read_raw(struct iio_dev *indio_dev,
diff --git a/drivers/iio/adc/ad7793.c b/drivers/iio/adc/ad7793.c
index b86e89370e0d..5f15fd7aab74 100644
--- a/drivers/iio/adc/ad7793.c
+++ b/drivers/iio/adc/ad7793.c
@@ -206,6 +206,7 @@ static const struct ad_sigma_delta_info ad7793_sigma_delta_info = {
 	.addr_shift = 3,
 	.read_mask = BIT(6),
 	.irq_flags = IRQF_TRIGGER_FALLING,
+	.num_resetclks = 32,
 };
 
 static const struct ad_sd_calib_data ad7793_calib_arr[6] = {
@@ -265,7 +266,7 @@ static int ad7793_setup(struct iio_dev *indio_dev,
 		return ret;
 
 	/* reset the serial interface */
-	ret = ad_sd_reset(&st->sd, 32);
+	ret = ad_sd_reset(&st->sd);
 	if (ret < 0)
 		goto out;
 	usleep_range(500, 2000); /* Wait for at least 500us */
diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
index a4c31baa9c9e..101cf30f4458 100644
--- a/drivers/iio/adc/ad_sigma_delta.c
+++ b/drivers/iio/adc/ad_sigma_delta.c
@@ -178,13 +178,12 @@ EXPORT_SYMBOL_NS_GPL(ad_sd_read_reg, IIO_AD_SIGMA_DELTA);
  * ad_sd_reset() - Reset the serial interface
  *
  * @sigma_delta: The sigma delta device
- * @reset_length: Number of SCLKs with DIN = 1
  *
  * Returns 0 on success, an error code otherwise.
  **/
-int ad_sd_reset(struct ad_sigma_delta *sigma_delta,
-	unsigned int reset_length)
+int ad_sd_reset(struct ad_sigma_delta *sigma_delta)
 {
+	unsigned int reset_length = sigma_delta->info->num_resetclks;
 	uint8_t *buf;
 	unsigned int size;
 	int ret;
diff --git a/include/linux/iio/adc/ad_sigma_delta.h b/include/linux/iio/adc/ad_sigma_delta.h
index f86eca6126b4..eef87d04eb6b 100644
--- a/include/linux/iio/adc/ad_sigma_delta.h
+++ b/include/linux/iio/adc/ad_sigma_delta.h
@@ -54,6 +54,7 @@ struct iio_dev;
  * @irq_flags: flags for the interrupt used by the triggered buffer
  * @num_slots: Number of sequencer slots
  * @irq_line: IRQ for reading conversions. If 0, spi->irq will be used
+ * @num_resetclks: Number of SPI clk cycles with MOSI=1 to reset the chip.
  */
 struct ad_sigma_delta_info {
 	int (*set_channel)(struct ad_sigma_delta *, unsigned int channel);
@@ -70,6 +71,7 @@ struct ad_sigma_delta_info {
 	unsigned long irq_flags;
 	unsigned int num_slots;
 	int irq_line;
+	unsigned int num_resetclks;
 };
 
 /**
@@ -181,8 +183,7 @@ int ad_sd_write_reg(struct ad_sigma_delta *sigma_delta, unsigned int reg,
 int ad_sd_read_reg(struct ad_sigma_delta *sigma_delta, unsigned int reg,
 	unsigned int size, unsigned int *val);
 
-int ad_sd_reset(struct ad_sigma_delta *sigma_delta,
-	unsigned int reset_length);
+int ad_sd_reset(struct ad_sigma_delta *sigma_delta);
 
 int ad_sigma_delta_single_conversion(struct iio_dev *indio_dev,
 	const struct iio_chan_spec *chan, int *val);
-- 
2.45.2

[PATCH v6 08/10] iio: adc: ad_sigma_delta: Check for previous ready signals

[Uwe Kleine-König]

It can happen if a previous conversion was aborted the ADC pulls down
the R̅D̅Y̅ line but the event wasn't handled before. In that case enabling
the irq might immediately fire (depending on the irq controller
capabilities) and even with a rdy-gpio isn't identified as an unrelated
one.

To cure that problem check for a pending event before the measurement is
started and clear it if needed.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/iio/adc/ad_sigma_delta.c | 99 +++++++++++++++++++++++++++++++-
 1 file changed, 98 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
index 101cf30f4458..d32102b25530 100644
--- a/drivers/iio/adc/ad_sigma_delta.c
+++ b/drivers/iio/adc/ad_sigma_delta.c
@@ -29,8 +29,11 @@
 #define AD_SD_COMM_CHAN_MASK	0x3
 
 #define AD_SD_REG_COMM		0x00
+#define AD_SD_REG_STATUS	0x00
 #define AD_SD_REG_DATA		0x03
 
+#define AD_SD_REG_STATUS_RDY	0x80
+
 /**
  * ad_sd_set_comm() - Set communications register
  *
@@ -222,6 +225,86 @@ static void ad_sd_enable_irq(struct ad_sigma_delta *sigma_delta)
 	enable_irq(sigma_delta->irq_line);
 }
 
+#define AD_SD_CLEAR_DATA_BUFLEN 9
+
+/* Called with `sigma_delta->bus_locked == true` only. */
+static int ad_sigma_delta_clear_pending_event(struct ad_sigma_delta *sigma_delta)
+{
+	bool pending_event;
+	unsigned int data_read_len = BITS_TO_BYTES(sigma_delta->info->num_resetclks);
+	u8 *data;
+	struct spi_transfer t[] = {
+		{
+			.len = 1,
+		}, {
+			.len = data_read_len,
+		}
+	};
+	struct spi_message m;
+	int ret;
+
+	/*
+	 * Read R̅D̅Y̅ pin (if possible) or status register to check if there is an
+	 * old event.
+	 */
+	if (sigma_delta->rdy_gpiod) {
+		pending_event = gpiod_get_value(sigma_delta->rdy_gpiod);
+	} else {
+		unsigned status_reg;
+
+		ret = ad_sd_read_reg(sigma_delta, AD_SD_REG_STATUS, 1, &status_reg);
+		if (ret)
+			return ret;
+
+		pending_event = !(status_reg & AD_SD_REG_STATUS_RDY);
+	}
+
+	if (!pending_event)
+		return 0;
+
+	/*
+	 * In general the size of the data register is unknown. It varies from
+	 * device to device, might be one byte longer if CONTROL.DATA_STATUS is
+	 * set and even varies on some devices depending on which input is
+	 * selected. So send one byte to start reading the data register and
+	 * then just clock for some bytes with DIN (aka MOSI) high to not
+	 * confuse the register access state machine after the data register was
+	 * completely read. Note however that the sequence length must be
+	 * shorter than the reset procedure.
+	 */
+
+	data = kzalloc(data_read_len + 1, GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	spi_message_init(&m);
+	if (sigma_delta->info->has_registers) {
+		unsigned int data_reg = sigma_delta->info->data_reg ?: AD_SD_REG_DATA;
+
+		data[0] = data_reg << sigma_delta->info->addr_shift;
+		data[0] |= sigma_delta->info->read_mask;
+		data[0] |= sigma_delta->comm;
+		t[0].tx_buf = data;
+		spi_message_add_tail(&t[0], &m);
+	}
+
+	/*
+	 * The first transferred byte is part of the real data register,
+	 * so this doesn't need to be 0xff. In the remaining
+	 * `data_read_len - 1` bytes are less than $num_resetclks ones.
+	 */
+	t[1].tx_buf = data + 1;
+	data[1] = 0x00;
+	memset(data + 2, 0xff, data_read_len - 1);
+	spi_message_add_tail(&t[1], &m);
+
+	ret = spi_sync_locked(sigma_delta->spi, &m);
+
+	kfree(data);
+
+	return ret;
+}
+
 int ad_sd_calibrate(struct ad_sigma_delta *sigma_delta,
 	unsigned int mode, unsigned int channel)
 {
@@ -237,6 +320,10 @@ int ad_sd_calibrate(struct ad_sigma_delta *sigma_delta,
 	sigma_delta->keep_cs_asserted = true;
 	reinit_completion(&sigma_delta->completion);
 
+	ret = ad_sigma_delta_clear_pending_event(sigma_delta);
+	if (ret)
+		goto out;
+
 	ret = ad_sigma_delta_set_mode(sigma_delta, mode);
 	if (ret < 0)
 		goto out;
@@ -310,6 +397,10 @@ int ad_sigma_delta_single_conversion(struct iio_dev *indio_dev,
 	sigma_delta->keep_cs_asserted = true;
 	reinit_completion(&sigma_delta->completion);
 
+	ret = ad_sigma_delta_clear_pending_event(sigma_delta);
+	if (ret)
+		goto out_unlock;
+
 	ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_SINGLE);
 
 	ad_sd_enable_irq(sigma_delta);
@@ -333,9 +424,11 @@ int ad_sigma_delta_single_conversion(struct iio_dev *indio_dev,
 out:
 	ad_sd_disable_irq(sigma_delta);
 
-	sigma_delta->keep_cs_asserted = false;
 	ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_IDLE);
 	ad_sigma_delta_disable_one(sigma_delta, chan->address);
+
+out_unlock:
+	sigma_delta->keep_cs_asserted = false;
 	sigma_delta->bus_locked = false;
 	spi_bus_unlock(sigma_delta->spi->controller);
 	iio_device_release_direct_mode(indio_dev);
@@ -406,6 +499,10 @@ static int ad_sd_buffer_postenable(struct iio_dev *indio_dev)
 	sigma_delta->bus_locked = true;
 	sigma_delta->keep_cs_asserted = true;
 
+	ret = ad_sigma_delta_clear_pending_event(sigma_delta);
+	if (ret)
+		goto err_unlock;
+
 	ret = ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_CONTINUOUS);
 	if (ret)
 		goto err_unlock;
-- 
2.45.2

Re: [PATCH v6 08/10] iio: adc: ad_sigma_delta: Check for previous ready signals

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1765 bytes --]

Hello Jonathan,

On Fri, Dec 06, 2024 at 06:28:40PM +0100, Uwe Kleine-König wrote:
> diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
> index 101cf30f4458..d32102b25530 100644
> --- a/drivers/iio/adc/ad_sigma_delta.c
> +++ b/drivers/iio/adc/ad_sigma_delta.c
> [...]
> @@ -222,6 +225,86 @@ static void ad_sd_enable_irq(struct ad_sigma_delta *sigma_delta)
>  	enable_irq(sigma_delta->irq_line);
>  }
>  
> +#define AD_SD_CLEAR_DATA_BUFLEN 9
> +
> +/* Called with `sigma_delta->bus_locked == true` only. */
> +static int ad_sigma_delta_clear_pending_event(struct ad_sigma_delta *sigma_delta)
> +{
> +	bool pending_event;
> +	unsigned int data_read_len = BITS_TO_BYTES(sigma_delta->info->num_resetclks);
> +	u8 *data;
> +	struct spi_transfer t[] = {
> +		{
> +			.len = 1,
> +		}, {
> +			.len = data_read_len,
> +		}
> +	};
> +	struct spi_message m;
> +	int ret;
> +
> +	/*
> +	 * Read R̅D̅Y̅ pin (if possible) or status register to check if there is an
> +	 * old event.
> +	 */
> +	if (sigma_delta->rdy_gpiod) {
> +		pending_event = gpiod_get_value(sigma_delta->rdy_gpiod);
> +	} else {
> +		unsigned status_reg;

While backporting this patch to a vendor tree I noticed checkpatch criticising:

	WARNING: Prefer 'unsigned int' to bare use of 'unsigned'

for the above line. Would you fixup an "int" into that line please, or
is that already too late?

Something like:

	git checkout togreg
	sed -i 's/unsigned status_reg;/unsigned int status_reg;/' drivers/iio/adc/ad_sigma_delta.c
	git commit --fixup=132d44dc6966 drivers/iio/adc/ad_sigma_delta.c
	git rebase -r --autosquash 132d44dc6966^

If it's already to late, I can prepare a proper patch for that.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v6 09/10] iio: adc: ad7124: Add error reporting during probe

[Uwe Kleine-König]

A driver that silently fails to probe is annoying and hard to debug. So
add messages in the error paths of the probe function.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/iio/adc/ad7124.c | 76 +++++++++++++++++++++-------------------
 1 file changed, 40 insertions(+), 36 deletions(-)

diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
index b17c3dbeaeba..9405cb579324 100644
--- a/drivers/iio/adc/ad7124.c
+++ b/drivers/iio/adc/ad7124.c
@@ -360,20 +360,21 @@ static int ad7124_find_free_config_slot(struct ad7124_state *st)
 	return free_cfg_slot;
 }
 
+/* Only called during probe, so dev_err_probe() can be used */
 static int ad7124_init_config_vref(struct ad7124_state *st, struct ad7124_channel_config *cfg)
 {
+	struct device *dev = &st->sd.spi->dev;
 	unsigned int refsel = cfg->refsel;
 
 	switch (refsel) {
 	case AD7124_REFIN1:
 	case AD7124_REFIN2:
 	case AD7124_AVDD_REF:
-		if (IS_ERR(st->vref[refsel])) {
-			dev_err(&st->sd.spi->dev,
-				"Error, trying to use external voltage reference without a %s regulator.\n",
-				ad7124_ref_names[refsel]);
-			return PTR_ERR(st->vref[refsel]);
-		}
+		if (IS_ERR(st->vref[refsel]))
+			return dev_err_probe(dev, PTR_ERR(st->vref[refsel]),
+					     "Error, trying to use external voltage reference without a %s regulator.\n",
+					     ad7124_ref_names[refsel]);
+
 		cfg->vref_mv = regulator_get_voltage(st->vref[refsel]);
 		/* Conversion from uV to mV */
 		cfg->vref_mv /= 1000;
@@ -384,8 +385,7 @@ static int ad7124_init_config_vref(struct ad7124_state *st, struct ad7124_channe
 		st->adc_control |= AD7124_ADC_CTRL_REF_EN(1);
 		return 0;
 	default:
-		dev_err(&st->sd.spi->dev, "Invalid reference %d\n", refsel);
-		return -EINVAL;
+		return dev_err_probe(dev, -EINVAL, "Invalid reference %d\n", refsel);
 	}
 }
 
@@ -752,8 +752,10 @@ static const struct iio_info ad7124_info = {
 	.attrs = &ad7124_attrs_group,
 };
 
+/* Only called during probe, so dev_err_probe() can be used */
 static int ad7124_soft_reset(struct ad7124_state *st)
 {
+	struct device *dev = &st->sd.spi->dev;
 	unsigned int readval, timeout;
 	int ret;
 
@@ -766,7 +768,7 @@ static int ad7124_soft_reset(struct ad7124_state *st)
 	do {
 		ret = ad_sd_read_reg(&st->sd, AD7124_STATUS, 1, &readval);
 		if (ret < 0)
-			return ret;
+			return dev_err_probe(dev, ret, "Error reading status register\n");
 
 		if (!(readval & AD7124_STATUS_POR_FLAG_MSK))
 			return 0;
@@ -775,35 +777,30 @@ static int ad7124_soft_reset(struct ad7124_state *st)
 		usleep_range(100, 2000);
 	} while (--timeout);
 
-	dev_err(&st->sd.spi->dev, "Soft reset failed\n");
-
-	return -EIO;
+	return dev_err_probe(dev, -EIO, "Soft reset failed\n");
 }
 
 static int ad7124_check_chip_id(struct ad7124_state *st)
 {
+	struct device *dev = &st->sd.spi->dev;
 	unsigned int readval, chip_id, silicon_rev;
 	int ret;
 
 	ret = ad_sd_read_reg(&st->sd, AD7124_ID, 1, &readval);
 	if (ret < 0)
-		return ret;
+		return dev_err_probe(dev, ret, "Failure to read ID register\n");
 
 	chip_id = AD7124_DEVICE_ID_GET(readval);
 	silicon_rev = AD7124_SILICON_REV_GET(readval);
 
-	if (chip_id != st->chip_info->chip_id) {
-		dev_err(&st->sd.spi->dev,
-			"Chip ID mismatch: expected %u, got %u\n",
-			st->chip_info->chip_id, chip_id);
-		return -ENODEV;
-	}
+	if (chip_id != st->chip_info->chip_id)
+		return dev_err_probe(dev, -ENODEV,
+				     "Chip ID mismatch: expected %u, got %u\n",
+				     st->chip_info->chip_id, chip_id);
 
-	if (silicon_rev == 0) {
-		dev_err(&st->sd.spi->dev,
-			"Silicon revision empty. Chip may not be present\n");
-		return -ENODEV;
-	}
+	if (silicon_rev == 0)
+		return dev_err_probe(dev, -ENODEV,
+				     "Silicon revision empty. Chip may not be present\n");
 
 	return 0;
 }
@@ -862,16 +859,18 @@ static int ad7124_parse_channel_config(struct iio_dev *indio_dev,
 	device_for_each_child_node_scoped(dev, child) {
 		ret = fwnode_property_read_u32(child, "reg", &channel);
 		if (ret)
-			return ret;
+			return dev_err_probe(dev, ret,
+					     "Failed to parse reg property of %pfwP\n", child);
 
 		if (channel >= indio_dev->num_channels)
 			return dev_err_probe(dev, -EINVAL,
-				"Channel index >= number of channels\n");
+					     "Channel index >= number of channels in %pfwP\n", child);
 
 		ret = fwnode_property_read_u32_array(child, "diff-channels",
 						     ain, 2);
 		if (ret)
-			return ret;
+			return dev_err_probe(dev, ret,
+					     "Failed to parse diff-channels property of %pfwP\n", child);
 
 		if (!ad7124_valid_input_select(ain[0], st->chip_info) ||
 		    !ad7124_valid_input_select(ain[1], st->chip_info))
@@ -908,12 +907,13 @@ static int ad7124_parse_channel_config(struct iio_dev *indio_dev,
 
 static int ad7124_setup(struct ad7124_state *st)
 {
+	struct device *dev = &st->sd.spi->dev;
 	unsigned int fclk, power_mode;
 	int i, ret;
 
 	fclk = clk_get_rate(st->mclk);
 	if (!fclk)
-		return -EINVAL;
+		return dev_err_probe(dev, -EINVAL, "Failed to get mclk rate\n");
 
 	/* The power mode changes the master clock frequency */
 	power_mode = ad7124_find_closest_match(ad7124_master_clk_freq_hz,
@@ -922,7 +922,7 @@ static int ad7124_setup(struct ad7124_state *st)
 	if (fclk != ad7124_master_clk_freq_hz[power_mode]) {
 		ret = clk_set_rate(st->mclk, fclk);
 		if (ret)
-			return ret;
+			return dev_err_probe(dev, ret, "Failed to set mclk rate\n");
 	}
 
 	/* Set the power mode */
@@ -953,7 +953,7 @@ static int ad7124_setup(struct ad7124_state *st)
 
 	ret = ad_sd_write_reg(&st->sd, AD7124_ADC_CONTROL, 2, st->adc_control);
 	if (ret < 0)
-		return ret;
+		return dev_err_probe(dev, ret, "Failed to setup CONTROL register\n");
 
 	return ret;
 }
@@ -966,13 +966,14 @@ static void ad7124_reg_disable(void *r)
 static int ad7124_probe(struct spi_device *spi)
 {
 	const struct ad7124_chip_info *info;
+	struct device *dev = &spi->dev;
 	struct ad7124_state *st;
 	struct iio_dev *indio_dev;
 	int i, ret;
 
 	info = spi_get_device_match_data(spi);
 	if (!info)
-		return -ENODEV;
+		return dev_err_probe(dev, -ENODEV, "Failed to get match data\n");
 
 	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
 	if (!indio_dev)
@@ -1007,17 +1008,17 @@ static int ad7124_probe(struct spi_device *spi)
 
 		ret = regulator_enable(st->vref[i]);
 		if (ret)
-			return ret;
+			return dev_err_probe(dev, ret, "Failed to enable regulator #%d\n", i);
 
 		ret = devm_add_action_or_reset(&spi->dev, ad7124_reg_disable,
 					       st->vref[i]);
 		if (ret)
-			return ret;
+			return dev_err_probe(dev, ret, "Failed to register disable handler for regulator #%d\n", i);
 	}
 
 	st->mclk = devm_clk_get_enabled(&spi->dev, "mclk");
 	if (IS_ERR(st->mclk))
-		return PTR_ERR(st->mclk);
+		return dev_err_probe(dev, PTR_ERR(st->mclk), "Failed to get mclk\n");
 
 	ret = ad7124_soft_reset(st);
 	if (ret < 0)
@@ -1033,10 +1034,13 @@ static int ad7124_probe(struct spi_device *spi)
 
 	ret = devm_ad_sd_setup_buffer_and_trigger(&spi->dev, indio_dev);
 	if (ret < 0)
-		return ret;
+		return dev_err_probe(dev, ret, "Failed to setup triggers\n");
 
-	return devm_iio_device_register(&spi->dev, indio_dev);
+	ret = devm_iio_device_register(&spi->dev, indio_dev);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Failed to register iio device\n");
 
+	return 0;
 }
 
 static const struct of_device_id ad7124_of_match[] = {
-- 
2.45.2

[PATCH v6 10/10] iio: adc: ad7124: Implement temperature measurement

[Uwe Kleine-König]

If the maximal count of channels the driver supports isn't fully
utilized, add an attribute providing the internal temperature.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/iio/adc/ad7124.c | 112 +++++++++++++++++++++++++++++++--------
 1 file changed, 91 insertions(+), 21 deletions(-)

diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
index 9405cb579324..d858bffd2628 100644
--- a/drivers/iio/adc/ad7124.c
+++ b/drivers/iio/adc/ad7124.c
@@ -95,6 +95,10 @@
 #define AD7124_MAX_CONFIGS	8
 #define AD7124_MAX_CHANNELS	16
 
+/* AD7124 input sources */
+#define AD7124_INPUT_TEMPSENSOR	16
+#define AD7124_INPUT_AVSS	17
+
 enum ad7124_ids {
 	ID_AD7124_4,
 	ID_AD7124_8,
@@ -589,26 +593,59 @@ static int ad7124_read_raw(struct iio_dev *indio_dev,
 
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SCALE:
-		mutex_lock(&st->cfgs_lock);
+		switch (chan->type) {
+		case IIO_VOLTAGE:
+			mutex_lock(&st->cfgs_lock);
 
-		idx = st->channels[chan->address].cfg.pga_bits;
-		*val = st->channels[chan->address].cfg.vref_mv;
-		if (st->channels[chan->address].cfg.bipolar)
-			*val2 = chan->scan_type.realbits - 1 + idx;
-		else
-			*val2 = chan->scan_type.realbits + idx;
+			idx = st->channels[chan->address].cfg.pga_bits;
+			*val = st->channels[chan->address].cfg.vref_mv;
+			if (st->channels[chan->address].cfg.bipolar)
+				*val2 = chan->scan_type.realbits - 1 + idx;
+			else
+				*val2 = chan->scan_type.realbits + idx;
+
+			mutex_unlock(&st->cfgs_lock);
+			return IIO_VAL_FRACTIONAL_LOG2;
+
+		case IIO_TEMP:
+			/*
+			 * According to the data sheet
+			 *   Temperature (°C)
+			 * = ((Conversion − 0x800000)/13584) − 272.5
+			 * = (Conversion − 0x800000 - 13584 * 272.5) / 13584
+			 * = (Conversion − 12090248) / 13584
+			 * So scale with 1000/13584 to yield °mC. Reduce by 8 to
+			 * 125/1698.
+			 */
+			*val = 125;
+			*val2 = 1698;
+			return IIO_VAL_FRACTIONAL;
+
+		default:
+			return -EINVAL;
+		}
 
-		mutex_unlock(&st->cfgs_lock);
-		return IIO_VAL_FRACTIONAL_LOG2;
 	case IIO_CHAN_INFO_OFFSET:
-		mutex_lock(&st->cfgs_lock);
-		if (st->channels[chan->address].cfg.bipolar)
-			*val = -(1 << (chan->scan_type.realbits - 1));
-		else
-			*val = 0;
+		switch (chan->type) {
+		case IIO_VOLTAGE:
+			mutex_lock(&st->cfgs_lock);
+			if (st->channels[chan->address].cfg.bipolar)
+				*val = -(1 << (chan->scan_type.realbits - 1));
+			else
+				*val = 0;
+
+			mutex_unlock(&st->cfgs_lock);
+			return IIO_VAL_INT;
+
+		case IIO_TEMP:
+			/* see calculation above */
+			*val = -12090248;
+			return IIO_VAL_INT;
+
+		default:
+			return -EINVAL;
+		}
 
-		mutex_unlock(&st->cfgs_lock);
-		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		mutex_lock(&st->cfgs_lock);
 		*val = st->channels[chan->address].cfg.odr;
@@ -826,11 +863,10 @@ static int ad7124_parse_channel_config(struct iio_dev *indio_dev,
 	struct ad7124_channel *channels;
 	struct iio_chan_spec *chan;
 	unsigned int ain[2], channel = 0, tmp;
+	unsigned int num_channels;
 	int ret;
 
-	st->num_channels = device_get_child_node_count(dev);
-	if (!st->num_channels)
-		return dev_err_probe(dev, -ENODEV, "no channel children\n");
+	num_channels = device_get_child_node_count(dev);
 
 	/*
 	 * The driver assigns each logical channel defined in the device tree
@@ -839,9 +875,12 @@ static int ad7124_parse_channel_config(struct iio_dev *indio_dev,
 	 * CHANNEL_15) as an additional channel register. The driver could be
 	 * improved to lift this limitation.
 	 */
-	if (st->num_channels > AD7124_MAX_CHANNELS)
+	if (num_channels > AD7124_MAX_CHANNELS)
 		return dev_err_probe(dev, -EINVAL, "Too many channels defined\n");
 
+	/* Add one for temperature */
+	st->num_channels = min(num_channels + 1, AD7124_MAX_CHANNELS);
+
 	chan = devm_kcalloc(indio_dev->dev.parent, st->num_channels,
 			    sizeof(*chan), GFP_KERNEL);
 	if (!chan)
@@ -862,7 +901,7 @@ static int ad7124_parse_channel_config(struct iio_dev *indio_dev,
 			return dev_err_probe(dev, ret,
 					     "Failed to parse reg property of %pfwP\n", child);
 
-		if (channel >= indio_dev->num_channels)
+		if (channel >= num_channels)
 			return dev_err_probe(dev, -EINVAL,
 					     "Channel index >= number of channels in %pfwP\n", child);
 
@@ -902,6 +941,37 @@ static int ad7124_parse_channel_config(struct iio_dev *indio_dev,
 		chan[channel].channel2 = ain[1];
 	}
 
+	if (num_channels < AD7124_MAX_CHANNELS) {
+		st->channels[num_channels] = (struct ad7124_channel) {
+			.nr = num_channels,
+			.ain = AD7124_CHANNEL_AINP(AD7124_INPUT_TEMPSENSOR) |
+				AD7124_CHANNEL_AINM(AD7124_INPUT_AVSS),
+			.cfg = {
+				.bipolar = true,
+			},
+		};
+
+		chan[num_channels] = (struct iio_chan_spec) {
+			.type = IIO_TEMP,
+			.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_OFFSET) |
+				BIT(IIO_CHAN_INFO_SAMP_FREQ),
+			.scan_type = {
+				/*
+				 * You might find it strange that a bipolar
+				 * measurement yields an unsigned value, but
+				 * this matches the device's manual.
+				 */
+				.sign = 'u',
+				.realbits = 24,
+				.storagebits = 32,
+				.endianness = IIO_BE,
+			},
+			.address = num_channels,
+			.scan_index = num_channels,
+		};
+	};
+
 	return 0;
 }
 
-- 
2.45.2

Re: [PATCH v6 10/10] iio: adc: ad7124: Implement temperature measurement

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1522 bytes --]

Hello Jonathan,

On Fri, Dec 06, 2024 at 06:28:42PM +0100, Uwe Kleine-König wrote:
> diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
> index 9405cb579324..d858bffd2628 100644
> --- a/drivers/iio/adc/ad7124.c
> +++ b/drivers/iio/adc/ad7124.c
> [...]
> @@ -902,6 +941,37 @@ static int ad7124_parse_channel_config(struct iio_dev *indio_dev,
>  		chan[channel].channel2 = ain[1];
>  	}
>  
> +	if (num_channels < AD7124_MAX_CHANNELS) {
> +		st->channels[num_channels] = (struct ad7124_channel) {
> +			.nr = num_channels,
> +			.ain = AD7124_CHANNEL_AINP(AD7124_INPUT_TEMPSENSOR) |
> +				AD7124_CHANNEL_AINM(AD7124_INPUT_AVSS),
> +			.cfg = {
> +				.bipolar = true,
> +			},
> +		};
> +
> +		chan[num_channels] = (struct iio_chan_spec) {
> +			.type = IIO_TEMP,
> +			.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_OFFSET) |
> +				BIT(IIO_CHAN_INFO_SAMP_FREQ),
> +			.scan_type = {
> +				/*
> +				 * You might find it strange that a bipolar
> +				 * measurement yields an unsigned value, but
> +				 * this matches the device's manual.
> +				 */
> +				.sign = 'u',
> +				.realbits = 24,
> +				.storagebits = 32,
> +				.endianness = IIO_BE,
> +			},
> +			.address = num_channels,
> +			.scan_index = num_channels,
> +		};
> +	};

The kernel build bot wailed about the ; here. Should I send a proper
patch, or can you still just rewrite your tree to drop it?

Best regards and thanks and sorry,
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v6 10/10] iio: adc: ad7124: Implement temperature measurement

[Jonathan Cameron]

On Mon, 9 Dec 2024 10:28:17 +0100
Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:

> Hello Jonathan,
> 
> On Fri, Dec 06, 2024 at 06:28:42PM +0100, Uwe Kleine-König wrote:
> > diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
> > index 9405cb579324..d858bffd2628 100644
> > --- a/drivers/iio/adc/ad7124.c
> > +++ b/drivers/iio/adc/ad7124.c
> > [...]
> > @@ -902,6 +941,37 @@ static int ad7124_parse_channel_config(struct iio_dev *indio_dev,
> >  		chan[channel].channel2 = ain[1];
> >  	}
> >  
> > +	if (num_channels < AD7124_MAX_CHANNELS) {
> > +		st->channels[num_channels] = (struct ad7124_channel) {
> > +			.nr = num_channels,
> > +			.ain = AD7124_CHANNEL_AINP(AD7124_INPUT_TEMPSENSOR) |
> > +				AD7124_CHANNEL_AINM(AD7124_INPUT_AVSS),
> > +			.cfg = {
> > +				.bipolar = true,
> > +			},
> > +		};
> > +
> > +		chan[num_channels] = (struct iio_chan_spec) {
> > +			.type = IIO_TEMP,
> > +			.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > +				BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_OFFSET) |
> > +				BIT(IIO_CHAN_INFO_SAMP_FREQ),
> > +			.scan_type = {
> > +				/*
> > +				 * You might find it strange that a bipolar
> > +				 * measurement yields an unsigned value, but
> > +				 * this matches the device's manual.
> > +				 */
> > +				.sign = 'u',
> > +				.realbits = 24,
> > +				.storagebits = 32,
> > +				.endianness = IIO_BE,
> > +			},
> > +			.address = num_channels,
> > +			.scan_index = num_channels,
> > +		};
> > +	};  
> 
> The kernel build bot wailed about the ; here. Should I send a proper
> patch, or can you still just rewrite your tree to drop it?

I tweaked it in the tree

Thanks,

Jonathan

> 
> Best regards and thanks and sorry,
> Uwe

Re: [PATCH v6 00/10] iio: adc: ad7124: Various fixes

[Andy Shevchenko]

On Fri, Dec 6, 2024 at 7:29 PM Uwe Kleine-König
<u.kleine-koenig@baylibre.com> wrote:
>
> Hello,
>
> here comes v6 of this series. Compared to v5
> (https://lore.kernel.org/linux-iio/20241203110019.1520071-12-u.kleine-koenig@baylibre.com)
> the following things changed:
>
>  - Rebased to v6.13-rc1 + 64612ec9b909. (No changes needed here.)
>
>  - Add Ack from Conor to patch #3
>
>  - Fixed how R̅D̅Y̅ is written. This was wrong before because the overline
>    char must be added after the character that should get the overline,
>    not before. I got that wrong because of
>    https://bugs.debian.org/1089108. I would expect that now this is
>    properly shown in most browsers, MUAs and editors.
>
>    I guess Andy still doesn't agree to write it that way.

Yeah, I prefer a solid overline which Unicode simply incapable of, but
HTML does.
In any case the representation in this version is much better than in
the previous version.
I leave this up to Jonathan, but as I said an electrical engineer in
me is not satisfied.

> Jonathan,
>    would you decide here please? If you agree with Andy I suggest to
>    replace it by #RDY. Andy suggested #RDY_N which I think is too far
>    away from the original name. If you (also) like R̅D̅Y̅, just keep it as
>    is.
>
>  - Fix error handling in patch #8
>    I just pasted "return ret" to all callers of
>    ad_sigma_delta_clear_pending_event() before. Now the error handling
>    matches the actual needs of the context.
>
>  - s/irq controller's capabilities/irq controller capabilities/
>    as suggested by Andy for patch #8

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 00/10] iio: adc: ad7124: Various fixes

[Jonathan Cameron]

On Sat, 7 Dec 2024 01:12:56 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Fri, Dec 6, 2024 at 7:29 PM Uwe Kleine-König
> <u.kleine-koenig@baylibre.com> wrote:
> >
> > Hello,
> >
> > here comes v6 of this series. Compared to v5
> > (https://lore.kernel.org/linux-iio/20241203110019.1520071-12-u.kleine-koenig@baylibre.com)
> > the following things changed:
> >
> >  - Rebased to v6.13-rc1 + 64612ec9b909. (No changes needed here.)
> >
> >  - Add Ack from Conor to patch #3
> >
> >  - Fixed how R̅D̅Y̅ is written. This was wrong before because the overline
> >    char must be added after the character that should get the overline,
> >    not before. I got that wrong because of
> >    https://bugs.debian.org/1089108. I would expect that now this is
> >    properly shown in most browsers, MUAs and editors.
> >
> >    I guess Andy still doesn't agree to write it that way.  
> 
> Yeah, I prefer a solid overline which Unicode simply incapable of, but
> HTML does.
> In any case the representation in this version is much better than in
> the previous version.
> I leave this up to Jonathan, but as I said an electrical engineer in
> me is not satisfied.

Fully agree it's unsatisfying, but I think it is clear enough what
is intended to go ahead as is.

Jonathan

> 
> > Jonathan,
> >    would you decide here please? If you agree with Andy I suggest to
> >    replace it by #RDY. Andy suggested #RDY_N which I think is too far
> >    away from the original name. If you (also) like R̅D̅Y̅, just keep it as
> >    is.
> >
> >  - Fix error handling in patch #8
> >    I just pasted "return ret" to all callers of
> >    ad_sigma_delta_clear_pending_event() before. Now the error handling
> >    matches the actual needs of the context.
> >
> >  - s/irq controller's capabilities/irq controller capabilities/
> >    as suggested by Andy for patch #8  
> 

Re: [PATCH v6 00/10] iio: adc: ad7124: Various fixes

[Jonathan Cameron]

On Fri,  6 Dec 2024 18:28:32 +0100
Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:

> Hello,
> 
> here comes v6 of this series. Compared to v5
> (https://lore.kernel.org/linux-iio/20241203110019.1520071-12-u.kleine-koenig@baylibre.com)
> the following things changed:
> 
>  - Rebased to v6.13-rc1 + 64612ec9b909. (No changes needed here.)
> 
>  - Add Ack from Conor to patch #3
> 
>  - Fixed how R̅D̅Y̅ is written. This was wrong before because the overline
>    char must be added after the character that should get the overline,
>    not before. I got that wrong because of
>    https://bugs.debian.org/1089108. I would expect that now this is
>    properly shown in most browsers, MUAs and editors.
> 
>    I guess Andy still doesn't agree to write it that way. Jonathan,
>    would you decide here please? If you agree with Andy I suggest to
>    replace it by #RDY. Andy suggested #RDY_N which I think is too far
>    away from the original name. If you (also) like R̅D̅Y̅, just keep it as
>    is.
> 
>  - Fix error handling in patch #8
>    I just pasted "return ret" to all callers of
>    ad_sigma_delta_clear_pending_event() before. Now the error handling
>    matches the actual needs of the context.
> 
>  - s/irq controller's capabilities/irq controller capabilities/
>    as suggested by Andy for patch #8
> 
> Best regards
> Uwe

Hi Uwe

Given the mix of fixes and other material (kind of fixes, but also kind
of new functionality), I've queued this for the next merge window in my
togreg branch.  If you think there are particular patches that need to
go sooner then I can handle them in a split fashion, but that does add
risk that the whole lot might no land depending on timings (particularly
given it's coming into vacation season).

Initially pushed out as testing - I assume we'll see that sparse warning.

Thanks,

Jonathan

> 
> Uwe Kleine-König (10):
>   iio: adc: ad7124: Don't create more channels than the driver can handle
>   iio: adc: ad7124: Refuse invalid input specifiers
>   dt-bindings: iio: adc: adi,ad7{124,173,192,780}: Allow specifications of a gpio for irq line
>   iio: adc: ad_sigma_delta: Add support for reading irq status using a GPIO
>   iio: adc: ad_sigma_delta: Handle CS assertion as intended in ad_sd_read_reg_raw()
>   iio: adc: ad_sigma_delta: Fix a race condition
>   iio: adc: ad_sigma_delta: Store information about reset sequence length
>   iio: adc: ad_sigma_delta: Check for previous ready signals
>   iio: adc: ad7124: Add error reporting during probe
>   iio: adc: ad7124: Implement temperature measurement
> 
>  .../bindings/iio/adc/adi,ad7124.yaml          |  13 ++
>  .../bindings/iio/adc/adi,ad7173.yaml          |  12 +
>  .../bindings/iio/adc/adi,ad7192.yaml          |  15 ++
>  .../bindings/iio/adc/adi,ad7780.yaml          |  11 +
>  drivers/iio/adc/ad7124.c                      | 217 +++++++++++++-----
>  drivers/iio/adc/ad7173.c                      |   1 +
>  drivers/iio/adc/ad7192.c                      |   4 +-
>  drivers/iio/adc/ad7791.c                      |   1 +
>  drivers/iio/adc/ad7793.c                      |   3 +-
>  drivers/iio/adc/ad_sigma_delta.c              | 194 +++++++++++++---
>  include/linux/iio/adc/ad_sigma_delta.h        |   8 +-
>  11 files changed, 390 insertions(+), 89 deletions(-)
> 
> base-commit: 40384c840ea1944d7c5a392e8975ed088ecf0b37
> prerequisite-patch-id: 617af17fc377a984762c61893b9f2a92ae62213a

Re: [PATCH v6 00/10] iio: adc: ad7124: Various fixes

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1128 bytes --]

Hello Jonathan,

On Sun, Dec 08, 2024 at 12:44:27PM +0000, Jonathan Cameron wrote:
> Given the mix of fixes and other material (kind of fixes, but also kind
> of new functionality), I've queued this for the next merge window in my
> togreg branch.  If you think there are particular patches that need to
> go sooner then I can handle them in a split fashion, but that does add
> risk that the whole lot might no land depending on timings (particularly
> given it's coming into vacation season).

So you tend to not backport the rdy-gpios patches (i.e.

	dt-bindings: iio: adc: adi,ad7{124,173,192,780}: Allow specifications of a gpio for irq line
	iio: adc: ad_sigma_delta: Add support for reading irq status using a GPIO

)? I personally would want to have these backported, too, but I can
understand that you might decide that in a different way.

Cherry picking

	iio: adc: ad_sigma_delta: Fix a race condition
	iio: adc: ad_sigma_delta: Check for previous ready signals

isn't trivial without the rdy-gpios, but they could be reworked. Tell me
if you want a helping hand (or an eye judging your backport).

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v6 00/10] iio: adc: ad7124: Various fixes

[Jonathan Cameron]

On Mon, 9 Dec 2024 10:47:29 +0100
Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:

> Hello Jonathan,
> 
> On Sun, Dec 08, 2024 at 12:44:27PM +0000, Jonathan Cameron wrote:
> > Given the mix of fixes and other material (kind of fixes, but also kind
> > of new functionality), I've queued this for the next merge window in my
> > togreg branch.  If you think there are particular patches that need to
> > go sooner then I can handle them in a split fashion, but that does add
> > risk that the whole lot might no land depending on timings (particularly
> > given it's coming into vacation season).  
> 
> So you tend to not backport the rdy-gpios patches (i.e.
> 
> 	dt-bindings: iio: adc: adi,ad7{124,173,192,780}: Allow specifications of a gpio for irq line
> 	iio: adc: ad_sigma_delta: Add support for reading irq status using a GPIO
> 
> )? I personally would want to have these backported, too, but I can
> understand that you might decide that in a different way.

Yeah. If it were tiny amount of code I might have gone the other way, but
this just got a bit too complex. 

> 
> Cherry picking
> 
> 	iio: adc: ad_sigma_delta: Fix a race condition
> 	iio: adc: ad_sigma_delta: Check for previous ready signals
> 
> isn't trivial without the rdy-gpios, but they could be reworked. Tell me
> if you want a helping hand (or an eye judging your backport).
> 
A backport won't go anywhere until these are upstream.  At that point if you
want them, feel free to suggest backporting these and provide the code ;)

Thanks,

Jonathan

> Best regards
> Uwe

Re: [PATCH v6 00/10] iio: adc: ad7124: Various fixes

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2296 bytes --]

On Wed, Dec 11, 2024 at 07:24:59PM +0000, Jonathan Cameron wrote:
> On Mon, 9 Dec 2024 10:47:29 +0100
> Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:
> 
> > Hello Jonathan,
> > 
> > On Sun, Dec 08, 2024 at 12:44:27PM +0000, Jonathan Cameron wrote:
> > > Given the mix of fixes and other material (kind of fixes, but also kind
> > > of new functionality), I've queued this for the next merge window in my
> > > togreg branch.  If you think there are particular patches that need to
> > > go sooner then I can handle them in a split fashion, but that does add
> > > risk that the whole lot might no land depending on timings (particularly
> > > given it's coming into vacation season).  
> > 
> > So you tend to not backport the rdy-gpios patches (i.e.
> > 
> > 	dt-bindings: iio: adc: adi,ad7{124,173,192,780}: Allow specifications of a gpio for irq line
> > 	iio: adc: ad_sigma_delta: Add support for reading irq status using a GPIO
> > 
> > )? I personally would want to have these backported, too, but I can
> > understand that you might decide that in a different way.
> 
> Yeah. If it were tiny amount of code I might have gone the other way, but
> this just got a bit too complex. 

I think it is easy to see that the changes for the rdy-gpios support has
zero impact if the device has no rdy-gpio. Then
devm_gpiod_get_optional() returns NULL and !sigma_delta->rdy_gpiod is
true and so nothing changes.

Of course it's subjective if you agree this to be easy to see, and also
if that matters for the backport.
 
> > Cherry picking
> > 
> > 	iio: adc: ad_sigma_delta: Fix a race condition
> > 	iio: adc: ad_sigma_delta: Check for previous ready signals
> > 
> > isn't trivial without the rdy-gpios, but they could be reworked. Tell me
> > if you want a helping hand (or an eye judging your backport).
>
> A backport won't go anywhere until these are upstream.  At that point if you
> want them, feel free to suggest backporting these and provide the code ;)

I've not given up hope that you agree to backport also the rdy-gpio
change yet. I won't invest the work without knowing it's used in the
end. So I'll wait until the changes are upstream and you made up your
mind. Then if the need arises, I will help.

Best regards
Uwe



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v6 00/10] iio: adc: ad7124: Various fixes

[Andy Shevchenko]

On Thu, Dec 12, 2024 at 1:28 PM Uwe Kleine-König
<u.kleine-koenig@baylibre.com> wrote:
> On Wed, Dec 11, 2024 at 07:24:59PM +0000, Jonathan Cameron wrote:
> > On Mon, 9 Dec 2024 10:47:29 +0100
> > Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:

...

> > A backport won't go anywhere until these are upstream.  At that point if you
> > want them, feel free to suggest backporting these and provide the code ;)
>
> I've not given up hope that you agree to backport also the rdy-gpio
> change yet. I won't invest the work without knowing it's used in the
> end. So I'll wait until the changes are upstream and you made up your
> mind. Then if the need arises, I will help.

AFAIK the process, any contributor may suggest a bacport of a few
patches if the justification is good enough, I don't understand how
Jonathan's view can be an impediment here (he may be a help, of
course).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 00/10] iio: adc: ad7124: Various fixes

[Jonathan Cameron]

On Thu, 12 Dec 2024 13:44:40 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Thu, Dec 12, 2024 at 1:28 PM Uwe Kleine-König
> <u.kleine-koenig@baylibre.com> wrote:
> > On Wed, Dec 11, 2024 at 07:24:59PM +0000, Jonathan Cameron wrote:  
> > > On Mon, 9 Dec 2024 10:47:29 +0100
> > > Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:  
> 
> ...
> 
> > > A backport won't go anywhere until these are upstream.  At that point if you
> > > want them, feel free to suggest backporting these and provide the code ;)  
> >
> > I've not given up hope that you agree to backport also the rdy-gpio
> > change yet. I won't invest the work without knowing it's used in the
> > end. So I'll wait until the changes are upstream and you made up your
> > mind. Then if the need arises, I will help.  
> 
> AFAIK the process, any contributor may suggest a bacport of a few
> patches if the justification is good enough, I don't understand how
> Jonathan's view can be an impediment here (he may be a help, of
> course).
> 

I could in theory object (as could any reviewer!) but on this one I
won't because I think it is justified, just not something to rush
into without some soaking time in upstream.

Thanks,

Jonathan