Re: [PATCH v2] iio: adc: ad7606: remove frstdata check for serial mode

Guillaume Stols <gstols@xxxxxxxxxxxx> · Mon, 24 Jun 2024 15:33:01 +0200

On 6/23/24 17:54, Jonathan Cameron wrote:
On Tue, 18 Jun 2024 13:45:00 +0000
Guillaume Stols <gstols@xxxxxxxxxxxx> wrote:

Frstdata pin is set high during the first sample's transmission and then
set low.  This code chunk attempts to recover from an eventual glitch in
the clock by checking frstdata state after reading the first channel's
sample.  Currently, in serial mode, this check happens AFTER the 16th
pulse, and if frstdata is not set it resets the device and returns EINVAL.
According to the datasheet, "The FRSTDATA output returns to a logic low
following the 16th SCLK falling edge.", thus after the 16th pulse, the
check will always be true, and the driver will not work as expected.  Thus
it must be removed for serial mode.
when you say will not work as expected, is this is normal circumstances, or
when dealing with a clock glitch?  i.e. should this have a fixes tag and
got upstream asap or is it just cleaning up a corner case and can wait for
now?

One trivial comment inline.

Jonathan

It completely prevents the driver to work when adi, first-data 
(optional) is defined in the DT.

So I guess anyone having the driver working with the serial interface 
right now did not define frstdata in the DT.

However, for someone new that sets adi, first-data in the DT, it is not 
very straightforward to spot where the issue is.


Signed-off-by: Guillaume Stols <gstols@xxxxxxxxxxxx>
---
Changes in v2:
  - Remove frstdata check only for the serial interface as suggested by
    Michael Hennerich.
  - Link to v1: https://lore.kernel.org/r/20240417-cleanup-ad7606-v1-1-5c2a29662c0a@xxxxxxxxxxxx
---
  drivers/iio/adc/ad7606.c     | 28 ++-----------------------
  drivers/iio/adc/ad7606.h     |  2 ++
  drivers/iio/adc/ad7606_par.c | 49 +++++++++++++++++++++++++++++++++++++++++---
  3 files changed, 50 insertions(+), 29 deletions(-)

diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
index 3a417595294f..c321c6ef48df 100644
--- a/drivers/iio/adc/ad7606.c
+++ b/drivers/iio/adc/ad7606.c
@@ -49,7 +49,7 @@ static const unsigned int ad7616_oversampling_avail[8] = {
  	1, 2, 4, 8, 16, 32, 64, 128,
  };
  
-static int ad7606_reset(struct ad7606_state *st)
+int ad7606_reset(struct ad7606_state *st)
  {
  	if (st->gpio_reset) {
  		gpiod_set_value(st->gpio_reset, 1);
@@ -60,6 +60,7 @@ static int ad7606_reset(struct ad7606_state *st)
  
  	return -ENODEV;
  }
+EXPORT_SYMBOL_NS_GPL(ad7606_reset, IIO_AD7606);
  
  static int ad7606_reg_access(struct iio_dev *indio_dev,
  			     unsigned int reg,
@@ -88,31 +89,6 @@ static int ad7606_read_samples(struct ad7606_state *st)
  {
  	unsigned int num = st->chip_info->num_channels - 1;
  	u16 *data = st->data;
-	int ret;
-
-	/*
-	 * The frstdata signal is set to high while and after reading the sample
-	 * of the first channel and low for all other channels. This can be used
-	 * to check that the incoming data is correctly aligned. During normal
-	 * operation the data should never become unaligned, but some glitch or
-	 * electrostatic discharge might cause an extra read or clock cycle.
-	 * Monitoring the frstdata signal allows to recover from such failure
-	 * situations.
-	 */
-
-	if (st->gpio_frstdata) {
-		ret = st->bops->read_block(st->dev, 1, data);
-		if (ret)
-			return ret;
-
-		if (!gpiod_get_value(st->gpio_frstdata)) {
-			ad7606_reset(st);
-			return -EIO;
-		}
-
-		data++;
-		num--;
-	}
  
  	return st->bops->read_block(st->dev, num, data);
  }
diff --git a/drivers/iio/adc/ad7606.h b/drivers/iio/adc/ad7606.h
index 0c6a88cc4695..6649e84d25de 100644
--- a/drivers/iio/adc/ad7606.h
+++ b/drivers/iio/adc/ad7606.h
@@ -151,6 +151,8 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
  		 const char *name, unsigned int id,
  		 const struct ad7606_bus_ops *bops);
  
+int ad7606_reset(struct ad7606_state *st);
+
  enum ad7606_supported_device_ids {
  	ID_AD7605_4,
  	ID_AD7606_8,
diff --git a/drivers/iio/adc/ad7606_par.c b/drivers/iio/adc/ad7606_par.c
index d8408052262e..1f7050297b64 100644
--- a/drivers/iio/adc/ad7606_par.c
+++ b/drivers/iio/adc/ad7606_par.c
@@ -7,6 +7,7 @@
  
  #include <linux/mod_devicetable.h>
  #include <linux/module.h>
+#include <linux/gpio/consumer.h>
  #include <linux/platform_device.h>
  #include <linux/types.h>
  #include <linux/err.h>
@@ -21,8 +22,30 @@ static int ad7606_par16_read_block(struct device *dev,
  	struct iio_dev *indio_dev = dev_get_drvdata(dev);
  	struct ad7606_state *st = iio_priv(indio_dev);
  
-	insw((unsigned long)st->base_address, buf, count);
  
+	/*
+	 * On the parallel interface, the frstdata signal is set to high while
+	 * and after reading the sample of the first channel and low for all
+	 * other channels.  This can be used to check that the incoming data is
+	 * correctly aligned.  During normal operation the data should never
+	 * become unaligned, but some glitch or electrostatic discharge might
+	 * cause an extra read or clock cycle.  Monitoring the frstdata signal
+	 * allows to recover from such failure situations.
+	 */
+	int num = count;
+	u16 *_buf = buf;
+
+	if (st->gpio_frstdata) {
+		insw((unsigned long)st->base_address, _buf, 1);
+		if (!gpiod_get_value(st->gpio_frstdata)) {
+			ad7606_reset(st);
+			return -EIO;
+		}
+		_buf++;
+		num--;
+	}
+	insw((unsigned long)st->base_address, _buf, num)
+;
Seems this slipped onto the next line.
Make sure to run checkpatch which I would have thought would catch this.

  	return 0;
  }
  
@@ -35,8 +58,28 @@ static int ad7606_par8_read_block(struct device *dev,
  {
  	struct iio_dev *indio_dev = dev_get_drvdata(dev);
  	struct ad7606_state *st = iio_priv(indio_dev);
-
-	insb((unsigned long)st->base_address, buf, count * 2);
+	/*
+	 * On the parallel interface, the frstdata signal is set to high while
+	 * and after reading the sample of the first channel and low for all
+	 * other channels.  This can be used to check that the incoming data is
+	 * correctly aligned.  During normal operation the data should never
+	 * become unaligned, but some glitch or electrostatic discharge might
+	 * cause an extra read or clock cycle.  Monitoring the frstdata signal
+	 * allows to recover from such failure situations.
+	 */
+	int num = count;
+	u16 *_buf = buf;
+
+	if (st->gpio_frstdata) {
+		insb((unsigned long)st->base_address, _buf, 2);
+		if (!gpiod_get_value(st->gpio_frstdata)) {
+			ad7606_reset(st);
+			return -EIO;
+		}
+		_buf++;
+		num--;
+	}
+	insb((unsigned long)st->base_address, _buf, num * 2);
  
  	return 0;
  }

---
base-commit: 07d4d0bb4a8ddcc463ed599b22f510d5926c2495
change-id: 20240416-cleanup-ad7606-161e2ed9818b

Best regards,







