[PATCH v4 0/9] staging: iio: ad7780: move out of staging

[PATCH v4 0/9] staging: iio: ad7780: move out of staging

[Renato Lui Geh]

This series of patches contains the following:
 - Adds user input for the 'gain' and 'filter' GPIO pins for the ad778x
   family chips;
 - Filter reading for the ad778x;
 - Sets pattern macro values and mask for PATTERN status bits;
 - Adds ID values for the ad7170, ad7171, ad7780 and ad7781 for ID
   status bits checking;
 - Moves regulator initialization to after GPIO init to maintain
   consistency between probe and remove;
 - Copyright edits, adding SPDX identifier and new copyright holder;
 - Moves the ad7780 driver out of staging to the mainline;
 - Adds device tree binding for the ad7780 driver.

Renato Lui Geh (9):
  staging: iio: ad7780: add gain & filter gpio support
  staging: iio: ad7780: add filter reading to ad778x
  staging: iio: ad7780: set pattern values and masks directly
  staging:iio:ad7780: add chip ID values and mask
  staging: iio: ad7780: move regulator to after GPIO init
  staging: iio: ad7780: add SPDX identifier
  staging: iio: ad7780: add new copyright holder
  staging: iio: ad7780: moving ad7780 out of staging
  staging: iio: ad7780: add device tree binding

Changelog:
*v3
 - SPDX and regulator init as patches
 - Renamed filter to odr and ad778x_filter to ad778x_odr_avail
 - Removed unnecessary regulator disabling
 - Removed unnecessary AD_SD_CHANNEL macro
 - Changed unsigned int to unsigned long long to avoid overflow
*v4
 - Split gain & filter patch into two, with the new commit adding only
   filter reading
 - Changed pattern values to direct values, and added pattern mask
 - Added ID values and mask
 - Added new copyright holder
 - Added device tree binding to the ad7780 driver

 .../bindings/iio/adc/adi,ad7780.txt           |  48 +++
 drivers/iio/adc/Kconfig                       |  12 +
 drivers/iio/adc/Makefile                      |   1 +
 drivers/iio/adc/ad7780.c                      | 365 ++++++++++++++++++
 drivers/staging/iio/adc/Kconfig               |  13 -
 drivers/staging/iio/adc/Makefile              |   1 -
 drivers/staging/iio/adc/ad7780.c              | 277 -------------
 7 files changed, 426 insertions(+), 291 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad7780.txt
 create mode 100644 drivers/iio/adc/ad7780.c
 delete mode 100644 drivers/staging/iio/adc/ad7780.c

--
2.21.0

[PATCH v4 1/9] staging: iio: ad7780: add gain & filter gpio support

[Renato Lui Geh]

Previously, the AD7780 driver only supported gpio for the 'powerdown'
pin. This commit adds suppport for the 'gain' and 'filter' pin.

Signed-off-by: Renato Lui Geh <renatogeh@gmail.com>
Signed-off-by: Giuliano Belinassi <giuliano.belinassi@usp.br>
Co-developed-by: Giuliano Belinassi <giuliano.belinassi@usp.br>
---
Changes in v3:
 - Renamed ad7780_chip_info's filter to odr
 - Renamed ad778x_filter to ad778x_odr_avail
 - Changed vref variable from unsigned int to unsigned long long to avoid
   overflow
 - Removed unnecessary AD_SD_CHANNEL macro
Changes in v4:
 - Removed useless macro
 - Added default case for switch to suppress warning
 - Removed chunks belonging to filter reading, adding these as a
   patch for itself

 drivers/staging/iio/adc/ad7780.c | 90 +++++++++++++++++++++++++++++---
 1 file changed, 84 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/iio/adc/ad7780.c b/drivers/staging/iio/adc/ad7780.c
index c4a85789c2db..87fbcf510d45 100644
--- a/drivers/staging/iio/adc/ad7780.c
+++ b/drivers/staging/iio/adc/ad7780.c
@@ -39,6 +39,12 @@
 #define AD7170_PATTERN		(AD7780_PAT0 | AD7170_PAT2)
 #define AD7170_PATTERN_MASK	(AD7780_PAT0 | AD7780_PAT1 | AD7170_PAT2)
 
+#define AD7780_GAIN_MIDPOINT	64
+#define AD7780_FILTER_MIDPOINT	13350
+
+static const unsigned int ad778x_gain[2]      = { 1, 128 };
+static const unsigned int ad778x_odr_avail[2] = { 10000, 16700 };
+
 struct ad7780_chip_info {
 	struct iio_chan_spec	channel;
 	unsigned int		pattern_mask;
@@ -50,7 +56,10 @@ struct ad7780_state {
 	const struct ad7780_chip_info	*chip_info;
 	struct regulator		*reg;
 	struct gpio_desc		*powerdown_gpio;
-	unsigned int	gain;
+	struct gpio_desc		*gain_gpio;
+	struct gpio_desc		*filter_gpio;
+	unsigned int			gain;
+	unsigned int			int_vref_mv;
 
 	struct ad_sigma_delta sd;
 };
@@ -104,17 +113,65 @@ static int ad7780_read_raw(struct iio_dev *indio_dev,
 		voltage_uv = regulator_get_voltage(st->reg);
 		if (voltage_uv < 0)
 			return voltage_uv;
-		*val = (voltage_uv / 1000) * st->gain;
+		voltage_uv /= 1000;
+		*val = voltage_uv * st->gain;
 		*val2 = chan->scan_type.realbits - 1;
+		st->int_vref_mv = voltage_uv;
 		return IIO_VAL_FRACTIONAL_LOG2;
 	case IIO_CHAN_INFO_OFFSET:
 		*val = -(1 << (chan->scan_type.realbits - 1));
 		return IIO_VAL_INT;
+	default:
+	break;
 	}
 
 	return -EINVAL;
 }
 
+static int ad7780_write_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int val,
+			    int val2,
+			    long m)
+{
+	struct ad7780_state *st = iio_priv(indio_dev);
+	const struct ad7780_chip_info *chip_info = st->chip_info;
+	unsigned long long vref;
+	unsigned int full_scale, gain;
+
+	if (!chip_info->is_ad778x)
+		return 0;
+
+	switch (m) {
+	case IIO_CHAN_INFO_SCALE:
+		if (val != 0)
+			return -EINVAL;
+
+		vref = st->int_vref_mv * 1000000LL;
+		full_scale = 1 << (chip_info->channel.scan_type.realbits - 1);
+		gain = DIV_ROUND_CLOSEST(vref, full_scale);
+		gain = DIV_ROUND_CLOSEST(gain, val2);
+		st->gain = gain;
+		if (gain < AD7780_GAIN_MIDPOINT)
+			gain = 0;
+		else
+			gain = 1;
+		gpiod_set_value(st->gain_gpio, gain);
+		break;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		if (1000*val + val2/1000 < AD7780_FILTER_MIDPOINT)
+			val = 0;
+		else
+			val = 1;
+		gpiod_set_value(st->filter_gpio, val);
+		break;
+	default:
+	break;
+	}
+
+	return 0;
+}
+
 static int ad7780_postprocess_sample(struct ad_sigma_delta *sigma_delta,
 				     unsigned int raw_sample)
 {
@@ -126,10 +183,7 @@ static int ad7780_postprocess_sample(struct ad_sigma_delta *sigma_delta,
 		return -EIO;
 
 	if (chip_info->is_ad778x) {
-		if (raw_sample & AD7780_GAIN)
-			st->gain = 1;
-		else
-			st->gain = 128;
+		st->gain = ad778x_gain[raw_sample & AD7780_GAIN];
 	}
 
 	return 0;
@@ -173,6 +227,7 @@ static const struct ad7780_chip_info ad7780_chip_info_tbl[] = {
 
 static const struct iio_info ad7780_info = {
 	.read_raw = ad7780_read_raw,
+	.write_raw = ad7780_write_raw,
 };
 
 static int ad7780_probe(struct spi_device *spi)
@@ -222,6 +277,29 @@ static int ad7780_probe(struct spi_device *spi)
 		goto error_disable_reg;
 	}
 
+	if (st->chip_info->is_ad778x) {
+		st->gain_gpio = devm_gpiod_get_optional(&spi->dev,
+							"adi,gain",
+							GPIOD_OUT_HIGH);
+		if (IS_ERR(st->gain_gpio)) {
+			ret = PTR_ERR(st->gain_gpio);
+			dev_err(&spi->dev, "Failed to request gain GPIO: %d\n",
+				ret);
+			goto error_disable_reg;
+		}
+
+		st->filter_gpio = devm_gpiod_get_optional(&spi->dev,
+							  "adi,filter",
+							  GPIOD_OUT_HIGH);
+		if (IS_ERR(st->filter_gpio)) {
+			ret = PTR_ERR(st->filter_gpio);
+			dev_err(&spi->dev,
+				"Failed to request filter GPIO: %d\n",
+				ret);
+			goto error_disable_reg;
+		}
+	}
+
 	ret = ad_sd_setup_buffer_and_trigger(indio_dev);
 	if (ret)
 		goto error_disable_reg;
-- 
2.21.0

[PATCH v4 2/9] staging: iio: ad7780: add filter reading to ad778x

[Renato Lui Geh]

This patch adds the new feature of reading the filter odr value for
ad778x chips. This value is stored in the chip's state struct whenever a
read or write call is performed on the chip's driver.

This feature requires sharing SAMP_FREQ. Since the ad717x does not have
a filter option, the driver only shares the relevant info mask for the
ad778x family.

Signed-off-by: Renato Lui Geh <renatogeh@gmail.com>
---
Changes in v4:
 - As mentioned in the previous patch, this was originally as part of
   the 'add gain & filter gpio support' patch

 drivers/staging/iio/adc/ad7780.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/iio/adc/ad7780.c b/drivers/staging/iio/adc/ad7780.c
index 87fbcf510d45..7a68e90ddf14 100644
--- a/drivers/staging/iio/adc/ad7780.c
+++ b/drivers/staging/iio/adc/ad7780.c
@@ -59,6 +59,7 @@ struct ad7780_state {
 	struct gpio_desc		*gain_gpio;
 	struct gpio_desc		*filter_gpio;
 	unsigned int			gain;
+	unsigned int			odr;
 	unsigned int			int_vref_mv;
 
 	struct ad_sigma_delta sd;
@@ -121,6 +122,9 @@ static int ad7780_read_raw(struct iio_dev *indio_dev,
 	case IIO_CHAN_INFO_OFFSET:
 		*val = -(1 << (chan->scan_type.realbits - 1));
 		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*val = st->odr;
+		return IIO_VAL_INT;
 	default:
 	break;
 	}
@@ -163,6 +167,7 @@ static int ad7780_write_raw(struct iio_dev *indio_dev,
 			val = 0;
 		else
 			val = 1;
+		st->odr = ad778x_odr_avail[val];
 		gpiod_set_value(st->filter_gpio, val);
 		break;
 	default:
@@ -184,6 +189,7 @@ static int ad7780_postprocess_sample(struct ad_sigma_delta *sigma_delta,
 
 	if (chip_info->is_ad778x) {
 		st->gain = ad778x_gain[raw_sample & AD7780_GAIN];
+		st->odr = ad778x_odr_avail[raw_sample & AD7780_FILTER];
 	}
 
 	return 0;
@@ -196,17 +202,19 @@ static const struct ad_sigma_delta_info ad7780_sigma_delta_info = {
 };
 
 #define AD7780_CHANNEL(bits, wordsize) \
+	AD_SD_CHANNEL(1, 0, 0, bits, 32, wordsize - bits)
+#define AD7170_CHANNEL(bits, wordsize) \
 	AD_SD_CHANNEL_NO_SAMP_FREQ(1, 0, 0, bits, 32, wordsize - bits)
 
 static const struct ad7780_chip_info ad7780_chip_info_tbl[] = {
 	[ID_AD7170] = {
-		.channel = AD7780_CHANNEL(12, 24),
+		.channel = AD7170_CHANNEL(12, 24),
 		.pattern = AD7170_PATTERN,
 		.pattern_mask = AD7170_PATTERN_MASK,
 		.is_ad778x = false,
 	},
 	[ID_AD7171] = {
-		.channel = AD7780_CHANNEL(16, 24),
+		.channel = AD7170_CHANNEL(16, 24),
 		.pattern = AD7170_PATTERN,
 		.pattern_mask = AD7170_PATTERN_MASK,
 		.is_ad778x = false,
-- 
2.21.0

[PATCH v4 3/9] staging: iio: ad7780: set pattern values and masks directly

[Renato Lui Geh]

The AD7780 driver contains status pattern bits designed for checking
whether serial transfers have been correctly performed. Pattern macros
were previously generated through bit fields. This patch sets good
pattern values directly and masks through GENMASK.

Signed-off-by: Renato Lui Geh <renatogeh@gmail.com>
---
 drivers/staging/iio/adc/ad7780.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/iio/adc/ad7780.c b/drivers/staging/iio/adc/ad7780.c
index 7a68e90ddf14..56c49e28f432 100644
--- a/drivers/staging/iio/adc/ad7780.c
+++ b/drivers/staging/iio/adc/ad7780.c
@@ -17,6 +17,7 @@
 #include <linux/sched.h>
 #include <linux/gpio/consumer.h>
 #include <linux/module.h>
+#include <linux/bits.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
@@ -28,16 +29,13 @@
 #define AD7780_ID1		BIT(4)
 #define AD7780_ID0		BIT(3)
 #define AD7780_GAIN		BIT(2)
-#define AD7780_PAT1		BIT(1)
-#define AD7780_PAT0		BIT(0)
 
-#define AD7780_PATTERN		(AD7780_PAT0)
-#define AD7780_PATTERN_MASK	(AD7780_PAT0 | AD7780_PAT1)
 
-#define AD7170_PAT2		BIT(2)
+#define AD7780_PATTERN_GOOD	1
+#define AD7780_PATTERN_MASK	GENMASK(1, 0)
 
-#define AD7170_PATTERN		(AD7780_PAT0 | AD7170_PAT2)
-#define AD7170_PATTERN_MASK	(AD7780_PAT0 | AD7780_PAT1 | AD7170_PAT2)
+#define AD7170_PATTERN_GOOD	5
+#define AD7170_PATTERN_MASK	GENMASK(2, 0)
 
 #define AD7780_GAIN_MIDPOINT	64
 #define AD7780_FILTER_MIDPOINT	13350
@@ -209,25 +207,25 @@ static const struct ad_sigma_delta_info ad7780_sigma_delta_info = {
 static const struct ad7780_chip_info ad7780_chip_info_tbl[] = {
 	[ID_AD7170] = {
 		.channel = AD7170_CHANNEL(12, 24),
-		.pattern = AD7170_PATTERN,
+		.pattern = AD7170_PATTERN_GOOD,
 		.pattern_mask = AD7170_PATTERN_MASK,
 		.is_ad778x = false,
 	},
 	[ID_AD7171] = {
 		.channel = AD7170_CHANNEL(16, 24),
-		.pattern = AD7170_PATTERN,
+		.pattern = AD7170_PATTERN_GOOD,
 		.pattern_mask = AD7170_PATTERN_MASK,
 		.is_ad778x = false,
 	},
 	[ID_AD7780] = {
 		.channel = AD7780_CHANNEL(24, 32),
-		.pattern = AD7780_PATTERN,
+		.pattern = AD7780_PATTERN_GOOD,
 		.pattern_mask = AD7780_PATTERN_MASK,
 		.is_ad778x = true,
 	},
 	[ID_AD7781] = {
 		.channel = AD7780_CHANNEL(20, 32),
-		.pattern = AD7780_PATTERN,
+		.pattern = AD7780_PATTERN_GOOD,
 		.pattern_mask = AD7780_PATTERN_MASK,
 		.is_ad778x = true,
 	},
-- 
2.21.0

[PATCH v4 4/9] staging:iio:ad7780: add chip ID values and mask

[Renato Lui Geh]

The ad7780 supports both the ad778x and ad717x families. Each chip has
a corresponding ID. This patch provides a mask for extracting ID values
from the status bits and also macros for the correct values for the
ad7170, ad7171, ad7780 and ad7781.

Signed-off-by: Renato Lui Geh <renatogeh@gmail.com>
---
 drivers/staging/iio/adc/ad7780.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/iio/adc/ad7780.c b/drivers/staging/iio/adc/ad7780.c
index 56c49e28f432..ad7617a3a141 100644
--- a/drivers/staging/iio/adc/ad7780.c
+++ b/drivers/staging/iio/adc/ad7780.c
@@ -26,10 +26,14 @@
 #define AD7780_RDY		BIT(7)
 #define AD7780_FILTER		BIT(6)
 #define AD7780_ERR		BIT(5)
-#define AD7780_ID1		BIT(4)
-#define AD7780_ID0		BIT(3)
 #define AD7780_GAIN		BIT(2)
 
+#define AD7170_ID		0
+#define AD7171_ID		1
+#define AD7780_ID		1
+#define AD7781_ID		0
+
+#define AD7780_ID_MASK		(BIT(3) | BIT(4))
 
 #define AD7780_PATTERN_GOOD	1
 #define AD7780_PATTERN_MASK	GENMASK(1, 0)
-- 
2.21.0

[PATCH v4 5/9] staging: iio: ad7780: move regulator to after GPIO init

[Renato Lui Geh]

To maintain consistency between ad7780_probe and ad7780_remove orders,
regulator initialization has been moved to after GPIO initializations.

Signed-off-by: Renato Lui Geh <renatogeh@gmail.com>
---
 drivers/staging/iio/adc/ad7780.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/staging/iio/adc/ad7780.c b/drivers/staging/iio/adc/ad7780.c
index ad7617a3a141..12aef0f101bc 100644
--- a/drivers/staging/iio/adc/ad7780.c
+++ b/drivers/staging/iio/adc/ad7780.c
@@ -255,16 +255,6 @@ static int ad7780_probe(struct spi_device *spi)
 
 	ad_sd_init(&st->sd, indio_dev, spi, &ad7780_sigma_delta_info);
 
-	st->reg = devm_regulator_get(&spi->dev, "avdd");
-	if (IS_ERR(st->reg))
-		return PTR_ERR(st->reg);
-
-	ret = regulator_enable(st->reg);
-	if (ret) {
-		dev_err(&spi->dev, "Failed to enable specified AVdd supply\n");
-		return ret;
-	}
-
 	st->chip_info =
 		&ad7780_chip_info_tbl[spi_get_device_id(spi)->driver_data];
 
@@ -284,7 +274,7 @@ static int ad7780_probe(struct spi_device *spi)
 		ret = PTR_ERR(st->powerdown_gpio);
 		dev_err(&spi->dev, "Failed to request powerdown GPIO: %d\n",
 			ret);
-		goto error_disable_reg;
+		return ret;
 	}
 
 	if (st->chip_info->is_ad778x) {
@@ -295,7 +285,7 @@ static int ad7780_probe(struct spi_device *spi)
 			ret = PTR_ERR(st->gain_gpio);
 			dev_err(&spi->dev, "Failed to request gain GPIO: %d\n",
 				ret);
-			goto error_disable_reg;
+			return ret;
 		}
 
 		st->filter_gpio = devm_gpiod_get_optional(&spi->dev,
@@ -306,10 +296,20 @@ static int ad7780_probe(struct spi_device *spi)
 			dev_err(&spi->dev,
 				"Failed to request filter GPIO: %d\n",
 				ret);
-			goto error_disable_reg;
+			return ret;
 		}
 	}
 
+	st->reg = devm_regulator_get(&spi->dev, "avdd");
+	if (IS_ERR(st->reg))
+		return PTR_ERR(st->reg);
+
+	ret = regulator_enable(st->reg);
+	if (ret) {
+		dev_err(&spi->dev, "Failed to enable specified AVdd supply\n");
+		return ret;
+	}
+
 	ret = ad_sd_setup_buffer_and_trigger(indio_dev);
 	if (ret)
 		goto error_disable_reg;
-- 
2.21.0

[PATCH v4 6/9] staging: iio: ad7780: add SPDX identifier

[Renato Lui Geh]

Add SPDX identifier (GPL-2.0) to the AD7780 driver.

Signed-off-by: Renato Lui Geh <renatogeh@gmail.com>
---
 drivers/staging/iio/adc/ad7780.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/iio/adc/ad7780.c b/drivers/staging/iio/adc/ad7780.c
index 12aef0f101bc..b1d4909ba025 100644
--- a/drivers/staging/iio/adc/ad7780.c
+++ b/drivers/staging/iio/adc/ad7780.c
@@ -1,9 +1,8 @@
+// SPDX-License-Identifier: GPL-2.0
 /*
  * AD7170/AD7171 and AD7780/AD7781 SPI ADC driver
  *
  * Copyright 2011 Analog Devices Inc.
- *
- * Licensed under the GPL-2.
  */
 
 #include <linux/interrupt.h>
-- 
2.21.0

[PATCH v4 7/9] staging: iio: ad7780: add new copyright holder

[Renato Lui Geh]

This patch adds a new copyright holder to the ad7780 driver.

Signed-off-by: Renato Lui Geh <renatogeh@gmail.com>
---
 drivers/staging/iio/adc/ad7780.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/staging/iio/adc/ad7780.c b/drivers/staging/iio/adc/ad7780.c
index b1d4909ba025..aa1ddd0c45ee 100644
--- a/drivers/staging/iio/adc/ad7780.c
+++ b/drivers/staging/iio/adc/ad7780.c
@@ -3,6 +3,7 @@
  * AD7170/AD7171 and AD7780/AD7781 SPI ADC driver
  *
  * Copyright 2011 Analog Devices Inc.
+ * Copyright 2019 Renato Lui Geh
  */
 
 #include <linux/interrupt.h>
-- 
2.21.0

[PATCH v4 8/9] staging: iio: ad7780: moving ad7780 out of staging

[Renato Lui Geh]

Move ad7780 ADC driver out of staging and into the mainline.

The ad7780 is a sigma-delta analog to digital converter. This driver provides
reading voltage values and status bits from both the ad778x and ad717x series.
Its interface also allows writing on the FILTER and GAIN GPIO pins on the
ad778x.

Signed-off-by: Renato Lui Geh <renatogeh@gmail.com>
Signed-off-by: Giuliano Belinassi <giuliano.belinassi@usp.br>
Co-developed-by: Giuliano Belinassi <giuliano.belinassi@usp.br>
---
Changes in v3:
 - Changes unrelated to moving the driver to main tree were resent as
   individual patches
Changes in v4:
 - Removed line warning it was safe to build the ad7780 driver by
   default

About the above change in v4, I was unsure whether it was ok to add this
in this patch, as 'move out of staging' patches should be just that. But
this change is so minimal I'm willing to risk a v5. Bottom line: should
I add this change as another patch?

 drivers/iio/adc/Kconfig          |  12 +
 drivers/iio/adc/Makefile         |   1 +
 drivers/iio/adc/ad7780.c         | 365 +++++++++++++++++++++++++++++++
 drivers/staging/iio/adc/Kconfig  |  13 --
 drivers/staging/iio/adc/Makefile |   1 -
 drivers/staging/iio/adc/ad7780.c | 365 -------------------------------
 6 files changed, 378 insertions(+), 379 deletions(-)
 create mode 100644 drivers/iio/adc/ad7780.c
 delete mode 100644 drivers/staging/iio/adc/ad7780.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index f3cc7a31bce5..6b36a45bd09f 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -108,6 +108,18 @@ config AD7766
 	  To compile this driver as a module, choose M here: the module will be
 	  called ad7766.
 
+config AD7780
+	tristate "Analog Devices AD7780 and similar ADCs driver"
+	depends on SPI
+	depends on GPIOLIB || COMPILE_TEST
+	select AD_SIGMA_DELTA
+	help
+	  Say yes here to build support for Analog Devices AD7170, AD7171,
+	  AD7780 and AD7781 SPI analog to digital converters (ADC).
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called ad7780.
+
 config AD7791
 	tristate "Analog Devices AD7791 ADC driver"
 	depends on SPI
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index ea5031348052..b48852157115 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_AD7606_IFACE_PARALLEL) += ad7606_par.o
 obj-$(CONFIG_AD7606_IFACE_SPI) += ad7606_spi.o
 obj-$(CONFIG_AD7606) += ad7606.o
 obj-$(CONFIG_AD7766) += ad7766.o
+obj-$(CONFIG_AD7780) += ad7780.o
 obj-$(CONFIG_AD7791) += ad7791.o
 obj-$(CONFIG_AD7793) += ad7793.o
 obj-$(CONFIG_AD7887) += ad7887.o
diff --git a/drivers/iio/adc/ad7780.c b/drivers/iio/adc/ad7780.c
new file mode 100644
index 000000000000..aa1ddd0c45ee
--- /dev/null
+++ b/drivers/iio/adc/ad7780.c
@@ -0,0 +1,365 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * AD7170/AD7171 and AD7780/AD7781 SPI ADC driver
+ *
+ * Copyright 2011 Analog Devices Inc.
+ * Copyright 2019 Renato Lui Geh
+ */
+
+#include <linux/interrupt.h>
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/sysfs.h>
+#include <linux/spi/spi.h>
+#include <linux/regulator/consumer.h>
+#include <linux/err.h>
+#include <linux/sched.h>
+#include <linux/gpio/consumer.h>
+#include <linux/module.h>
+#include <linux/bits.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/adc/ad_sigma_delta.h>
+
+#define AD7780_RDY		BIT(7)
+#define AD7780_FILTER		BIT(6)
+#define AD7780_ERR		BIT(5)
+#define AD7780_GAIN		BIT(2)
+
+#define AD7170_ID		0
+#define AD7171_ID		1
+#define AD7780_ID		1
+#define AD7781_ID		0
+
+#define AD7780_ID_MASK		(BIT(3) | BIT(4))
+
+#define AD7780_PATTERN_GOOD	1
+#define AD7780_PATTERN_MASK	GENMASK(1, 0)
+
+#define AD7170_PATTERN_GOOD	5
+#define AD7170_PATTERN_MASK	GENMASK(2, 0)
+
+#define AD7780_GAIN_MIDPOINT	64
+#define AD7780_FILTER_MIDPOINT	13350
+
+static const unsigned int ad778x_gain[2]      = { 1, 128 };
+static const unsigned int ad778x_odr_avail[2] = { 10000, 16700 };
+
+struct ad7780_chip_info {
+	struct iio_chan_spec	channel;
+	unsigned int		pattern_mask;
+	unsigned int		pattern;
+	bool			is_ad778x;
+};
+
+struct ad7780_state {
+	const struct ad7780_chip_info	*chip_info;
+	struct regulator		*reg;
+	struct gpio_desc		*powerdown_gpio;
+	struct gpio_desc		*gain_gpio;
+	struct gpio_desc		*filter_gpio;
+	unsigned int			gain;
+	unsigned int			odr;
+	unsigned int			int_vref_mv;
+
+	struct ad_sigma_delta sd;
+};
+
+enum ad7780_supported_device_ids {
+	ID_AD7170,
+	ID_AD7171,
+	ID_AD7780,
+	ID_AD7781,
+};
+
+static struct ad7780_state *ad_sigma_delta_to_ad7780(struct ad_sigma_delta *sd)
+{
+	return container_of(sd, struct ad7780_state, sd);
+}
+
+static int ad7780_set_mode(struct ad_sigma_delta *sigma_delta,
+			   enum ad_sigma_delta_mode mode)
+{
+	struct ad7780_state *st = ad_sigma_delta_to_ad7780(sigma_delta);
+	unsigned int val;
+
+	switch (mode) {
+	case AD_SD_MODE_SINGLE:
+	case AD_SD_MODE_CONTINUOUS:
+		val = 1;
+		break;
+	default:
+		val = 0;
+		break;
+	}
+
+	gpiod_set_value(st->powerdown_gpio, val);
+
+	return 0;
+}
+
+static int ad7780_read_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan,
+			   int *val,
+			   int *val2,
+			   long m)
+{
+	struct ad7780_state *st = iio_priv(indio_dev);
+	int voltage_uv;
+
+	switch (m) {
+	case IIO_CHAN_INFO_RAW:
+		return ad_sigma_delta_single_conversion(indio_dev, chan, val);
+	case IIO_CHAN_INFO_SCALE:
+		voltage_uv = regulator_get_voltage(st->reg);
+		if (voltage_uv < 0)
+			return voltage_uv;
+		voltage_uv /= 1000;
+		*val = voltage_uv * st->gain;
+		*val2 = chan->scan_type.realbits - 1;
+		st->int_vref_mv = voltage_uv;
+		return IIO_VAL_FRACTIONAL_LOG2;
+	case IIO_CHAN_INFO_OFFSET:
+		*val = -(1 << (chan->scan_type.realbits - 1));
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*val = st->odr;
+		return IIO_VAL_INT;
+	default:
+	break;
+	}
+
+	return -EINVAL;
+}
+
+static int ad7780_write_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int val,
+			    int val2,
+			    long m)
+{
+	struct ad7780_state *st = iio_priv(indio_dev);
+	const struct ad7780_chip_info *chip_info = st->chip_info;
+	unsigned long long vref;
+	unsigned int full_scale, gain;
+
+	if (!chip_info->is_ad778x)
+		return 0;
+
+	switch (m) {
+	case IIO_CHAN_INFO_SCALE:
+		if (val != 0)
+			return -EINVAL;
+
+		vref = st->int_vref_mv * 1000000LL;
+		full_scale = 1 << (chip_info->channel.scan_type.realbits - 1);
+		gain = DIV_ROUND_CLOSEST(vref, full_scale);
+		gain = DIV_ROUND_CLOSEST(gain, val2);
+		st->gain = gain;
+		if (gain < AD7780_GAIN_MIDPOINT)
+			gain = 0;
+		else
+			gain = 1;
+		gpiod_set_value(st->gain_gpio, gain);
+		break;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		if (1000*val + val2/1000 < AD7780_FILTER_MIDPOINT)
+			val = 0;
+		else
+			val = 1;
+		st->odr = ad778x_odr_avail[val];
+		gpiod_set_value(st->filter_gpio, val);
+		break;
+	default:
+	break;
+	}
+
+	return 0;
+}
+
+static int ad7780_postprocess_sample(struct ad_sigma_delta *sigma_delta,
+				     unsigned int raw_sample)
+{
+	struct ad7780_state *st = ad_sigma_delta_to_ad7780(sigma_delta);
+	const struct ad7780_chip_info *chip_info = st->chip_info;
+
+	if ((raw_sample & AD7780_ERR) ||
+	    ((raw_sample & chip_info->pattern_mask) != chip_info->pattern))
+		return -EIO;
+
+	if (chip_info->is_ad778x) {
+		st->gain = ad778x_gain[raw_sample & AD7780_GAIN];
+		st->odr = ad778x_odr_avail[raw_sample & AD7780_FILTER];
+	}
+
+	return 0;
+}
+
+static const struct ad_sigma_delta_info ad7780_sigma_delta_info = {
+	.set_mode = ad7780_set_mode,
+	.postprocess_sample = ad7780_postprocess_sample,
+	.has_registers = false,
+};
+
+#define AD7780_CHANNEL(bits, wordsize) \
+	AD_SD_CHANNEL(1, 0, 0, bits, 32, wordsize - bits)
+#define AD7170_CHANNEL(bits, wordsize) \
+	AD_SD_CHANNEL_NO_SAMP_FREQ(1, 0, 0, bits, 32, wordsize - bits)
+
+static const struct ad7780_chip_info ad7780_chip_info_tbl[] = {
+	[ID_AD7170] = {
+		.channel = AD7170_CHANNEL(12, 24),
+		.pattern = AD7170_PATTERN_GOOD,
+		.pattern_mask = AD7170_PATTERN_MASK,
+		.is_ad778x = false,
+	},
+	[ID_AD7171] = {
+		.channel = AD7170_CHANNEL(16, 24),
+		.pattern = AD7170_PATTERN_GOOD,
+		.pattern_mask = AD7170_PATTERN_MASK,
+		.is_ad778x = false,
+	},
+	[ID_AD7780] = {
+		.channel = AD7780_CHANNEL(24, 32),
+		.pattern = AD7780_PATTERN_GOOD,
+		.pattern_mask = AD7780_PATTERN_MASK,
+		.is_ad778x = true,
+	},
+	[ID_AD7781] = {
+		.channel = AD7780_CHANNEL(20, 32),
+		.pattern = AD7780_PATTERN_GOOD,
+		.pattern_mask = AD7780_PATTERN_MASK,
+		.is_ad778x = true,
+	},
+};
+
+static const struct iio_info ad7780_info = {
+	.read_raw = ad7780_read_raw,
+	.write_raw = ad7780_write_raw,
+};
+
+static int ad7780_probe(struct spi_device *spi)
+{
+	struct ad7780_state *st;
+	struct iio_dev *indio_dev;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	st = iio_priv(indio_dev);
+	st->gain = 1;
+
+	ad_sd_init(&st->sd, indio_dev, spi, &ad7780_sigma_delta_info);
+
+	st->chip_info =
+		&ad7780_chip_info_tbl[spi_get_device_id(spi)->driver_data];
+
+	spi_set_drvdata(spi, indio_dev);
+
+	indio_dev->dev.parent = &spi->dev;
+	indio_dev->name = spi_get_device_id(spi)->name;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = &st->chip_info->channel;
+	indio_dev->num_channels = 1;
+	indio_dev->info = &ad7780_info;
+
+	st->powerdown_gpio = devm_gpiod_get_optional(&spi->dev,
+						     "powerdown",
+						     GPIOD_OUT_LOW);
+	if (IS_ERR(st->powerdown_gpio)) {
+		ret = PTR_ERR(st->powerdown_gpio);
+		dev_err(&spi->dev, "Failed to request powerdown GPIO: %d\n",
+			ret);
+		return ret;
+	}
+
+	if (st->chip_info->is_ad778x) {
+		st->gain_gpio = devm_gpiod_get_optional(&spi->dev,
+							"adi,gain",
+							GPIOD_OUT_HIGH);
+		if (IS_ERR(st->gain_gpio)) {
+			ret = PTR_ERR(st->gain_gpio);
+			dev_err(&spi->dev, "Failed to request gain GPIO: %d\n",
+				ret);
+			return ret;
+		}
+
+		st->filter_gpio = devm_gpiod_get_optional(&spi->dev,
+							  "adi,filter",
+							  GPIOD_OUT_HIGH);
+		if (IS_ERR(st->filter_gpio)) {
+			ret = PTR_ERR(st->filter_gpio);
+			dev_err(&spi->dev,
+				"Failed to request filter GPIO: %d\n",
+				ret);
+			return ret;
+		}
+	}
+
+	st->reg = devm_regulator_get(&spi->dev, "avdd");
+	if (IS_ERR(st->reg))
+		return PTR_ERR(st->reg);
+
+	ret = regulator_enable(st->reg);
+	if (ret) {
+		dev_err(&spi->dev, "Failed to enable specified AVdd supply\n");
+		return ret;
+	}
+
+	ret = ad_sd_setup_buffer_and_trigger(indio_dev);
+	if (ret)
+		goto error_disable_reg;
+
+	ret = iio_device_register(indio_dev);
+	if (ret)
+		goto error_cleanup_buffer_and_trigger;
+
+	return 0;
+
+error_cleanup_buffer_and_trigger:
+	ad_sd_cleanup_buffer_and_trigger(indio_dev);
+error_disable_reg:
+	regulator_disable(st->reg);
+
+	return ret;
+}
+
+static int ad7780_remove(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev = spi_get_drvdata(spi);
+	struct ad7780_state *st = iio_priv(indio_dev);
+
+	iio_device_unregister(indio_dev);
+	ad_sd_cleanup_buffer_and_trigger(indio_dev);
+
+	regulator_disable(st->reg);
+
+	return 0;
+}
+
+static const struct spi_device_id ad7780_id[] = {
+	{"ad7170", ID_AD7170},
+	{"ad7171", ID_AD7171},
+	{"ad7780", ID_AD7780},
+	{"ad7781", ID_AD7781},
+	{}
+};
+MODULE_DEVICE_TABLE(spi, ad7780_id);
+
+static struct spi_driver ad7780_driver = {
+	.driver = {
+		.name	= "ad7780",
+	},
+	.probe		= ad7780_probe,
+	.remove		= ad7780_remove,
+	.id_table	= ad7780_id,
+};
+module_spi_driver(ad7780_driver);
+
+MODULE_AUTHOR("Michael Hennerich <michael.hennerich@analog.com>");
+MODULE_DESCRIPTION("Analog Devices AD7780 and similar ADCs");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/staging/iio/adc/Kconfig b/drivers/staging/iio/adc/Kconfig
index 7a93d3a5c113..404a53c743a6 100644
--- a/drivers/staging/iio/adc/Kconfig
+++ b/drivers/staging/iio/adc/Kconfig
@@ -3,19 +3,6 @@
 #
 menu "Analog to digital converters"
 
-config AD7780
-	tristate "Analog Devices AD7780 and similar ADCs driver"
-	depends on SPI
-	depends on GPIOLIB || COMPILE_TEST
-	select AD_SIGMA_DELTA
-	help
-	  Say yes here to build support for Analog Devices AD7170, AD7171,
-	  AD7780 and AD7781 SPI analog to digital converters (ADC).
-	  If unsure, say N (but it's safe to say "Y").
-
-	  To compile this driver as a module, choose M here: the
-	  module will be called ad7780.
-
 config AD7816
 	tristate "Analog Devices AD7816/7/8 temperature sensor and ADC driver"
 	depends on SPI
diff --git a/drivers/staging/iio/adc/Makefile b/drivers/staging/iio/adc/Makefile
index 7a421088ff82..4b76769b32bc 100644
--- a/drivers/staging/iio/adc/Makefile
+++ b/drivers/staging/iio/adc/Makefile
@@ -3,7 +3,6 @@
 # Makefile for industrial I/O ADC drivers
 #
 
-obj-$(CONFIG_AD7780) += ad7780.o
 obj-$(CONFIG_AD7816) += ad7816.o
 obj-$(CONFIG_AD7192) += ad7192.o
 obj-$(CONFIG_AD7280) += ad7280a.o
diff --git a/drivers/staging/iio/adc/ad7780.c b/drivers/staging/iio/adc/ad7780.c
deleted file mode 100644
index aa1ddd0c45ee..000000000000
--- a/drivers/staging/iio/adc/ad7780.c
+++ /dev/null
@@ -1,365 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/*
- * AD7170/AD7171 and AD7780/AD7781 SPI ADC driver
- *
- * Copyright 2011 Analog Devices Inc.
- * Copyright 2019 Renato Lui Geh
- */
-
-#include <linux/interrupt.h>
-#include <linux/device.h>
-#include <linux/kernel.h>
-#include <linux/slab.h>
-#include <linux/sysfs.h>
-#include <linux/spi/spi.h>
-#include <linux/regulator/consumer.h>
-#include <linux/err.h>
-#include <linux/sched.h>
-#include <linux/gpio/consumer.h>
-#include <linux/module.h>
-#include <linux/bits.h>
-
-#include <linux/iio/iio.h>
-#include <linux/iio/sysfs.h>
-#include <linux/iio/adc/ad_sigma_delta.h>
-
-#define AD7780_RDY		BIT(7)
-#define AD7780_FILTER		BIT(6)
-#define AD7780_ERR		BIT(5)
-#define AD7780_GAIN		BIT(2)
-
-#define AD7170_ID		0
-#define AD7171_ID		1
-#define AD7780_ID		1
-#define AD7781_ID		0
-
-#define AD7780_ID_MASK		(BIT(3) | BIT(4))
-
-#define AD7780_PATTERN_GOOD	1
-#define AD7780_PATTERN_MASK	GENMASK(1, 0)
-
-#define AD7170_PATTERN_GOOD	5
-#define AD7170_PATTERN_MASK	GENMASK(2, 0)
-
-#define AD7780_GAIN_MIDPOINT	64
-#define AD7780_FILTER_MIDPOINT	13350
-
-static const unsigned int ad778x_gain[2]      = { 1, 128 };
-static const unsigned int ad778x_odr_avail[2] = { 10000, 16700 };
-
-struct ad7780_chip_info {
-	struct iio_chan_spec	channel;
-	unsigned int		pattern_mask;
-	unsigned int		pattern;
-	bool			is_ad778x;
-};
-
-struct ad7780_state {
-	const struct ad7780_chip_info	*chip_info;
-	struct regulator		*reg;
-	struct gpio_desc		*powerdown_gpio;
-	struct gpio_desc		*gain_gpio;
-	struct gpio_desc		*filter_gpio;
-	unsigned int			gain;
-	unsigned int			odr;
-	unsigned int			int_vref_mv;
-
-	struct ad_sigma_delta sd;
-};
-
-enum ad7780_supported_device_ids {
-	ID_AD7170,
-	ID_AD7171,
-	ID_AD7780,
-	ID_AD7781,
-};
-
-static struct ad7780_state *ad_sigma_delta_to_ad7780(struct ad_sigma_delta *sd)
-{
-	return container_of(sd, struct ad7780_state, sd);
-}
-
-static int ad7780_set_mode(struct ad_sigma_delta *sigma_delta,
-			   enum ad_sigma_delta_mode mode)
-{
-	struct ad7780_state *st = ad_sigma_delta_to_ad7780(sigma_delta);
-	unsigned int val;
-
-	switch (mode) {
-	case AD_SD_MODE_SINGLE:
-	case AD_SD_MODE_CONTINUOUS:
-		val = 1;
-		break;
-	default:
-		val = 0;
-		break;
-	}
-
-	gpiod_set_value(st->powerdown_gpio, val);
-
-	return 0;
-}
-
-static int ad7780_read_raw(struct iio_dev *indio_dev,
-			   struct iio_chan_spec const *chan,
-			   int *val,
-			   int *val2,
-			   long m)
-{
-	struct ad7780_state *st = iio_priv(indio_dev);
-	int voltage_uv;
-
-	switch (m) {
-	case IIO_CHAN_INFO_RAW:
-		return ad_sigma_delta_single_conversion(indio_dev, chan, val);
-	case IIO_CHAN_INFO_SCALE:
-		voltage_uv = regulator_get_voltage(st->reg);
-		if (voltage_uv < 0)
-			return voltage_uv;
-		voltage_uv /= 1000;
-		*val = voltage_uv * st->gain;
-		*val2 = chan->scan_type.realbits - 1;
-		st->int_vref_mv = voltage_uv;
-		return IIO_VAL_FRACTIONAL_LOG2;
-	case IIO_CHAN_INFO_OFFSET:
-		*val = -(1 << (chan->scan_type.realbits - 1));
-		return IIO_VAL_INT;
-	case IIO_CHAN_INFO_SAMP_FREQ:
-		*val = st->odr;
-		return IIO_VAL_INT;
-	default:
-	break;
-	}
-
-	return -EINVAL;
-}
-
-static int ad7780_write_raw(struct iio_dev *indio_dev,
-			    struct iio_chan_spec const *chan,
-			    int val,
-			    int val2,
-			    long m)
-{
-	struct ad7780_state *st = iio_priv(indio_dev);
-	const struct ad7780_chip_info *chip_info = st->chip_info;
-	unsigned long long vref;
-	unsigned int full_scale, gain;
-
-	if (!chip_info->is_ad778x)
-		return 0;
-
-	switch (m) {
-	case IIO_CHAN_INFO_SCALE:
-		if (val != 0)
-			return -EINVAL;
-
-		vref = st->int_vref_mv * 1000000LL;
-		full_scale = 1 << (chip_info->channel.scan_type.realbits - 1);
-		gain = DIV_ROUND_CLOSEST(vref, full_scale);
-		gain = DIV_ROUND_CLOSEST(gain, val2);
-		st->gain = gain;
-		if (gain < AD7780_GAIN_MIDPOINT)
-			gain = 0;
-		else
-			gain = 1;
-		gpiod_set_value(st->gain_gpio, gain);
-		break;
-	case IIO_CHAN_INFO_SAMP_FREQ:
-		if (1000*val + val2/1000 < AD7780_FILTER_MIDPOINT)
-			val = 0;
-		else
-			val = 1;
-		st->odr = ad778x_odr_avail[val];
-		gpiod_set_value(st->filter_gpio, val);
-		break;
-	default:
-	break;
-	}
-
-	return 0;
-}
-
-static int ad7780_postprocess_sample(struct ad_sigma_delta *sigma_delta,
-				     unsigned int raw_sample)
-{
-	struct ad7780_state *st = ad_sigma_delta_to_ad7780(sigma_delta);
-	const struct ad7780_chip_info *chip_info = st->chip_info;
-
-	if ((raw_sample & AD7780_ERR) ||
-	    ((raw_sample & chip_info->pattern_mask) != chip_info->pattern))
-		return -EIO;
-
-	if (chip_info->is_ad778x) {
-		st->gain = ad778x_gain[raw_sample & AD7780_GAIN];
-		st->odr = ad778x_odr_avail[raw_sample & AD7780_FILTER];
-	}
-
-	return 0;
-}
-
-static const struct ad_sigma_delta_info ad7780_sigma_delta_info = {
-	.set_mode = ad7780_set_mode,
-	.postprocess_sample = ad7780_postprocess_sample,
-	.has_registers = false,
-};
-
-#define AD7780_CHANNEL(bits, wordsize) \
-	AD_SD_CHANNEL(1, 0, 0, bits, 32, wordsize - bits)
-#define AD7170_CHANNEL(bits, wordsize) \
-	AD_SD_CHANNEL_NO_SAMP_FREQ(1, 0, 0, bits, 32, wordsize - bits)
-
-static const struct ad7780_chip_info ad7780_chip_info_tbl[] = {
-	[ID_AD7170] = {
-		.channel = AD7170_CHANNEL(12, 24),
-		.pattern = AD7170_PATTERN_GOOD,
-		.pattern_mask = AD7170_PATTERN_MASK,
-		.is_ad778x = false,
-	},
-	[ID_AD7171] = {
-		.channel = AD7170_CHANNEL(16, 24),
-		.pattern = AD7170_PATTERN_GOOD,
-		.pattern_mask = AD7170_PATTERN_MASK,
-		.is_ad778x = false,
-	},
-	[ID_AD7780] = {
-		.channel = AD7780_CHANNEL(24, 32),
-		.pattern = AD7780_PATTERN_GOOD,
-		.pattern_mask = AD7780_PATTERN_MASK,
-		.is_ad778x = true,
-	},
-	[ID_AD7781] = {
-		.channel = AD7780_CHANNEL(20, 32),
-		.pattern = AD7780_PATTERN_GOOD,
-		.pattern_mask = AD7780_PATTERN_MASK,
-		.is_ad778x = true,
-	},
-};
-
-static const struct iio_info ad7780_info = {
-	.read_raw = ad7780_read_raw,
-	.write_raw = ad7780_write_raw,
-};
-
-static int ad7780_probe(struct spi_device *spi)
-{
-	struct ad7780_state *st;
-	struct iio_dev *indio_dev;
-	int ret;
-
-	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
-	if (!indio_dev)
-		return -ENOMEM;
-
-	st = iio_priv(indio_dev);
-	st->gain = 1;
-
-	ad_sd_init(&st->sd, indio_dev, spi, &ad7780_sigma_delta_info);
-
-	st->chip_info =
-		&ad7780_chip_info_tbl[spi_get_device_id(spi)->driver_data];
-
-	spi_set_drvdata(spi, indio_dev);
-
-	indio_dev->dev.parent = &spi->dev;
-	indio_dev->name = spi_get_device_id(spi)->name;
-	indio_dev->modes = INDIO_DIRECT_MODE;
-	indio_dev->channels = &st->chip_info->channel;
-	indio_dev->num_channels = 1;
-	indio_dev->info = &ad7780_info;
-
-	st->powerdown_gpio = devm_gpiod_get_optional(&spi->dev,
-						     "powerdown",
-						     GPIOD_OUT_LOW);
-	if (IS_ERR(st->powerdown_gpio)) {
-		ret = PTR_ERR(st->powerdown_gpio);
-		dev_err(&spi->dev, "Failed to request powerdown GPIO: %d\n",
-			ret);
-		return ret;
-	}
-
-	if (st->chip_info->is_ad778x) {
-		st->gain_gpio = devm_gpiod_get_optional(&spi->dev,
-							"adi,gain",
-							GPIOD_OUT_HIGH);
-		if (IS_ERR(st->gain_gpio)) {
-			ret = PTR_ERR(st->gain_gpio);
-			dev_err(&spi->dev, "Failed to request gain GPIO: %d\n",
-				ret);
-			return ret;
-		}
-
-		st->filter_gpio = devm_gpiod_get_optional(&spi->dev,
-							  "adi,filter",
-							  GPIOD_OUT_HIGH);
-		if (IS_ERR(st->filter_gpio)) {
-			ret = PTR_ERR(st->filter_gpio);
-			dev_err(&spi->dev,
-				"Failed to request filter GPIO: %d\n",
-				ret);
-			return ret;
-		}
-	}
-
-	st->reg = devm_regulator_get(&spi->dev, "avdd");
-	if (IS_ERR(st->reg))
-		return PTR_ERR(st->reg);
-
-	ret = regulator_enable(st->reg);
-	if (ret) {
-		dev_err(&spi->dev, "Failed to enable specified AVdd supply\n");
-		return ret;
-	}
-
-	ret = ad_sd_setup_buffer_and_trigger(indio_dev);
-	if (ret)
-		goto error_disable_reg;
-
-	ret = iio_device_register(indio_dev);
-	if (ret)
-		goto error_cleanup_buffer_and_trigger;
-
-	return 0;
-
-error_cleanup_buffer_and_trigger:
-	ad_sd_cleanup_buffer_and_trigger(indio_dev);
-error_disable_reg:
-	regulator_disable(st->reg);
-
-	return ret;
-}
-
-static int ad7780_remove(struct spi_device *spi)
-{
-	struct iio_dev *indio_dev = spi_get_drvdata(spi);
-	struct ad7780_state *st = iio_priv(indio_dev);
-
-	iio_device_unregister(indio_dev);
-	ad_sd_cleanup_buffer_and_trigger(indio_dev);
-
-	regulator_disable(st->reg);
-
-	return 0;
-}
-
-static const struct spi_device_id ad7780_id[] = {
-	{"ad7170", ID_AD7170},
-	{"ad7171", ID_AD7171},
-	{"ad7780", ID_AD7780},
-	{"ad7781", ID_AD7781},
-	{}
-};
-MODULE_DEVICE_TABLE(spi, ad7780_id);
-
-static struct spi_driver ad7780_driver = {
-	.driver = {
-		.name	= "ad7780",
-	},
-	.probe		= ad7780_probe,
-	.remove		= ad7780_remove,
-	.id_table	= ad7780_id,
-};
-module_spi_driver(ad7780_driver);
-
-MODULE_AUTHOR("Michael Hennerich <michael.hennerich@analog.com>");
-MODULE_DESCRIPTION("Analog Devices AD7780 and similar ADCs");
-MODULE_LICENSE("GPL v2");
-- 
2.21.0

[PATCH v4 9/9] staging: iio: ad7780: add device tree binding

[Renato Lui Geh]

Adds a device tree binding for the ad7780 driver.

Signed-off-by: Renato Lui Geh <renatogeh@gmail.com>
---
 .../bindings/iio/adc/adi,ad7780.txt           | 48 +++++++++++++++++++
 1 file changed, 48 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad7780.txt

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7780.txt b/Documentation/devicetree/bindings/iio/adc/adi,ad7780.txt
new file mode 100644
index 000000000000..440e52555349
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7780.txt
@@ -0,0 +1,48 @@
+* Analog Devices AD7170/AD7171/AD7780/AD7781
+
+Data sheets:
+
+- AD7170:
+	* https://www.analog.com/media/en/technical-documentation/data-sheets/AD7170.pdf
+- AD7171:
+	* https://www.analog.com/media/en/technical-documentation/data-sheets/AD7171.pdf
+- AD7780:
+	* https://www.analog.com/media/en/technical-documentation/data-sheets/ad7780.pdf
+- AD7781:
+	* https://www.analog.com/media/en/technical-documentation/data-sheets/AD7781.pdf
+
+Required properties:
+
+- compatible: should be one of
+	* "adi,ad7170"
+	* "adi,ad7171"
+	* "adi,ad7780"
+	* "adi,ad7781"
+- reg: spi chip select number for the device
+- vref-supply: the regulator supply for the ADC reference voltage
+
+Optional properties:
+
+- powerdown-gpios:  must be the device tree identifier of the PDRST pin. If
+		    specified, it will be asserted during driver probe. As the
+		    line is active high, it should be marked GPIO_ACTIVE_HIGH.
+- adi,gain-gpios:   must be the device tree identifier of the GAIN pin. Only for
+		    the ad778x chips. If specified, it will be asserted during
+		    driver probe. As the line is active low, it should be marked
+		    GPIO_ACTIVE_LOW.
+- adi,filter-gpios: must be the device tree identifier of the FILTER pin. Only
+		    for the ad778x chips. If specified, it will be asserted
+		    during driver probe. As the line is active low, it should be
+		    marked GPIO_ACTIVE_LOW.
+
+Example:
+
+adc@0 {
+	compatible =  "adi,ad7780";
+	reg =	      <0>;
+	vref-supply = <&vdd_supply>
+
+	powerdown-gpios  = <&gpio 12 GPIO_ACTIVE_HIGH>;
+	adi,gain-gpios   = <&gpio  5 GPIO_ACTIVE_LOW>;
+	adi,filter-gpios = <&gpio 15 GPIO_ACTIVE_LOW>;
+};
-- 
2.21.0

Re: [PATCH v4 1/9] staging: iio: ad7780: add gain & filter gpio support

[Ardelean, Alexandru]

On Thu, 2019-02-28 at 11:23 -0300, Renato Lui Geh wrote:
> 
> 
> Previously, the AD7780 driver only supported gpio for the 'powerdown'
> pin. This commit adds suppport for the 'gain' and 'filter' pin.
> 
> Signed-off-by: Renato Lui Geh <renatogeh@gmail.com>
> Signed-off-by: Giuliano Belinassi <giuliano.belinassi@usp.br>
> Co-developed-by: Giuliano Belinassi <giuliano.belinassi@usp.br>
> ---
> Changes in v3:
>  - Renamed ad7780_chip_info's filter to odr
>  - Renamed ad778x_filter to ad778x_odr_avail
>  - Changed vref variable from unsigned int to unsigned long long to avoid
>    overflow
>  - Removed unnecessary AD_SD_CHANNEL macro
> Changes in v4:
>  - Removed useless macro
>  - Added default case for switch to suppress warning
>  - Removed chunks belonging to filter reading, adding these as a
>    patch for itself
> 
>  drivers/staging/iio/adc/ad7780.c | 90 +++++++++++++++++++++++++++++---
>  1 file changed, 84 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7780.c
> b/drivers/staging/iio/adc/ad7780.c
> index c4a85789c2db..87fbcf510d45 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -39,6 +39,12 @@
>  #define AD7170_PATTERN         (AD7780_PAT0 | AD7170_PAT2)
>  #define AD7170_PATTERN_MASK    (AD7780_PAT0 | AD7780_PAT1 | AD7170_PAT2)
> 
> +#define AD7780_GAIN_MIDPOINT   64
> +#define AD7780_FILTER_MIDPOINT 13350
> +
> +static const unsigned int ad778x_gain[2]      = { 1, 128 };
> +static const unsigned int ad778x_odr_avail[2] = { 10000, 16700 };

ad778x_odr_avail[2] is not used in this patch, so it should probably go
into the next one 
(i.e. staging: iio: ad7780: add filter reading to ad778x )

one good way of catching stuff like this is to do interactive rebase and
compile your driver on each patch to see if the compiler catches this;
i suspect the compiler would have thrown an error for this change


> 
>  struct ad7780_chip_info
>         struct iio_chan_spec    channel;
>         unsigned int            pattern_mask;
> @@ -50,7 +56,10 @@ struct ad7780_state {
>         const struct ad7780_chip_info   *chip_info;
>         struct regulator                *reg;
>         struct gpio_desc                *powerdown_gpio;
> -       unsigned int    gain;
> +       struct gpio_desc                *gain_gpio;
> +       struct gpio_desc                *filter_gpio;
> +       unsigned int                    gain;
> +       unsigned int                    int_vref_mv;
> 
>         struct ad_sigma_delta sd;
>  };
> @@ -104,17 +113,65 @@ static int ad7780_read_raw(struct iio_dev
> *indio_dev,
>                 voltage_uv = regulator_get_voltage(st->reg);
>                 if (voltage_uv < 0)
>                         return voltage_uv;
> -               *val = (voltage_uv / 1000) * st->gain;
> +               voltage_uv /= 1000;
> +               *val = voltage_uv * st->gain;
>                 *val2 = chan->scan_type.realbits - 1;
> +               st->int_vref_mv = voltage_uv;
>                 return IIO_VAL_FRACTIONAL_LOG2;
>         case IIO_CHAN_INFO_OFFSET:
>                 *val = -(1 << (chan->scan_type.realbits - 1));
>                 return IIO_VAL_INT;
> +       default:
> +       break;

The indentation of the break statement is inconsistent with other places.
Still, it does not add much value adding this change as-is, since it does
not change any behavior, and is not an element needed by this change (i.e.
adding gain & filter support via gpios)

>         }
> 
>         return -EINVAL;
>  }
> 
> +static int ad7780_write_raw(struct iio_dev *indio_dev,
> +                           struct iio_chan_spec const *chan,
> +                           int val,
> +                           int val2,
> +                           long m)
> +{
> +       struct ad7780_state *st = iio_priv(indio_dev);
> +       const struct ad7780_chip_info *chip_info = st->chip_info;
> +       unsigned long long vref;
> +       unsigned int full_scale, gain;
> +
> +       if (!chip_info->is_ad778x)
> +               return 0;
> +
> +       switch (m) {
> +       case IIO_CHAN_INFO_SCALE:
> +               if (val != 0)
> +                       return -EINVAL;
> +
> +               vref = st->int_vref_mv * 1000000LL;
> +               full_scale = 1 << (chip_info->channel.scan_type.realbits
> - 1);
> +               gain = DIV_ROUND_CLOSEST(vref, full_scale);
> +               gain = DIV_ROUND_CLOSEST(gain, val2);
> +               st->gain = gain;
> +               if (gain < AD7780_GAIN_MIDPOINT)
> +                       gain = 0;
> +               else
> +                       gain = 1;
> +               gpiod_set_value(st->gain_gpio, gain);
> +               break;
> +       case IIO_CHAN_INFO_SAMP_FREQ:
> +               if (1000*val + val2/1000 < AD7780_FILTER_MIDPOINT)
> +                       val = 0;
> +               else
> +                       val = 1;
> +               gpiod_set_value(st->filter_gpio, val);
> +               break;
> +       default:
> +       break;
> +       }
> +
> +       return 0;
> +}
> +
>  static int ad7780_postprocess_sample(struct ad_sigma_delta *sigma_delta,
>                                      unsigned int raw_sample)
>  {
> @@ -126,10 +183,7 @@ static int ad7780_postprocess_sample(struct
> ad_sigma_delta *sigma_delta,
>                 return -EIO;
> 
>         if (chip_info->is_ad778x) {
> -               if (raw_sample & AD7780_GAIN)
> -                       st->gain = 1;
> -               else
> -                       st->gain = 128;
> +               st->gain = ad778x_gain[raw_sample & AD7780_GAIN];

The new `ad778x_gain[]` array could have been it's own patch, but from my
side it's fine to leave it here.
I do like this change, but it's not a patch that semantically has to do
anything with adding gain & filter gpio support.


Let's see what Jonathan says.

>         }
> 
>         return 0;
> @@ -173,6 +227,7 @@ static const struct ad7780_chip_info
> ad7780_chip_info_tbl[] = {
> 
>  static const struct iio_info ad7780_info = {
>         .read_raw = ad7780_read_raw,
> +       .write_raw = ad7780_write_raw,
>  };
> 
>  static int ad7780_probe(struct spi_device *spi)
> @@ -222,6 +277,29 @@ static int ad7780_probe(struct spi_device *spi)
>                 goto error_disable_reg;
>         }
> 
> +       if (st->chip_info->is_ad778x) {
> +               st->gain_gpio = devm_gpiod_get_optional(&spi->dev,
> +                                                       "adi,gain",
> +                                                       GPIOD_OUT_HIGH);
> +               if (IS_ERR(st->gain_gpio)) {
> +                       ret = PTR_ERR(st->gain_gpio);
> +                       dev_err(&spi->dev, "Failed to request gain GPIO:
> %d\n",
> +                               ret);
> +                       goto error_disable_reg;
> +               }
> +
> +               st->filter_gpio = devm_gpiod_get_optional(&spi->dev,
> +                                                         "adi,filter",
> +                                                         GPIOD_OUT_HIGH)
> ;
> +               if (IS_ERR(st->filter_gpio)) {
> +                       ret = PTR_ERR(st->filter_gpio);
> +                       dev_err(&spi->dev,
> +                               "Failed to request filter GPIO: %d\n",
> +                               ret);
> +                       goto error_disable_reg;
> +               }
> +       }
> +

This is just a preference of mine [feel free to ignore for this patch].
But, for this block of code, I would have added a separate function [
called something like ad7780_init_gpios(struct device *dev, struct
ad7780_state *st) )

you could also move the powerdown gpio there; and do something like

static int ad7780_init_gpios(struct device *dev, struct ad7780_state *st)
{

      st->powerdown_gpio = devm_gpiod_get_optional(dev,
                                                     "powerdown",
                                                     GPIOD_OUT_LOW);
        if (IS_ERR(st->powerdown_gpio)) {
              ret = PTR_ERR(st->powerdown_gpio);
              dev_err(dev, "Failed to request powerdown GPIO: %d\n", ret);
              return ret;
        }

       /**
        * Note Alex: I'm a big fan of keeping things to a minimal 
        * indentation level [especially where things can be elegant], 
        * but that's a preference of mine
        */
       if (!st->chip_info->is_ad778x)
           return 0;


        st->gain_gpio = devm_gpiod_get_optional(dev,
                        "adi,gain",
                        GPIOD_OUT_HIGH);
        if (IS_ERR(st->gain_gpio)) {
                ret = PTR_ERR(st->gain_gpio);
                dev_err(dev, "Failed to request gain GPIO: %d\n", ret);
                return ret;
        }

        st->filter_gpio = devm_gpiod_get_optional(dev,
                        "adi,filter",
                        GPIOD_OUT_HIGH);

        if (IS_ERR(st->filter_gpio)) {
                ret = PTR_ERR(st->filter_gpio);
                dev_err(dev, "Failed to request filter GPIO: %d\n", ret);
                return ret;
        }

        return 0;
}



>         ret = ad_sd_setup_buffer_and_trigger(indio_dev);
>         if (ret)
>                 goto error_disable_reg;
> --
> 2.21.0
> 

Re: [PATCH v4 2/9] staging: iio: ad7780: add filter reading to ad778x

[Ardelean, Alexandru]

On Thu, 2019-02-28 at 11:24 -0300, Renato Lui Geh wrote:
> 
> 
> This patch adds the new feature of reading the filter odr value for
> ad778x chips. This value is stored in the chip's state struct whenever a
> read or write call is performed on the chip's driver.
> 
> This feature requires sharing SAMP_FREQ. Since the ad717x does not have
> a filter option, the driver only shares the relevant info mask for the
> ad778x family.
> 
> Signed-off-by: Renato Lui Geh <renatogeh@gmail.com>
> ---
> Changes in v4:
>  - As mentioned in the previous patch, this was originally as part of
>    the 'add gain & filter gpio support' patch
> 

This needs the  ad778x_odr_avail[2] array to be moved to this patch.
Otherwise from what I can tell, this looks good.

>  drivers/staging/iio/adc/ad7780.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7780.c
> b/drivers/staging/iio/adc/ad7780.c
> index 87fbcf510d45..7a68e90ddf14 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -59,6 +59,7 @@ struct ad7780_state {
>         struct gpio_desc                *gain_gpio;
>         struct gpio_desc                *filter_gpio;
>         unsigned int                    gain;
> +       unsigned int                    odr;
>         unsigned int                    int_vref_mv;
> 
>         struct ad_sigma_delta sd;
> @@ -121,6 +122,9 @@ static int ad7780_read_raw(struct iio_dev *indio_dev,
>         case IIO_CHAN_INFO_OFFSET:
>                 *val = -(1 << (chan->scan_type.realbits - 1));
>                 return IIO_VAL_INT;
> +       case IIO_CHAN_INFO_SAMP_FREQ:
> +               *val = st->odr;
> +               return IIO_VAL_INT;
>         default:
>         break;
>         }
> @@ -163,6 +167,7 @@ static int ad7780_write_raw(struct iio_dev
> *indio_dev,
>                         val = 0;
>                 else
>                         val = 1;
> +               st->odr = ad778x_odr_avail[val];
>                 gpiod_set_value(st->filter_gpio, val);
>                 break;
>         default:
> @@ -184,6 +189,7 @@ static int ad7780_postprocess_sample(struct
> ad_sigma_delta *sigma_delta,
> 
>         if (chip_info->is_ad778x) {
>                 st->gain = ad778x_gain[raw_sample & AD7780_GAIN];
> +               st->odr = ad778x_odr_avail[raw_sample & AD7780_FILTER];
>         }
> 
>         return 0;
> @@ -196,17 +202,19 @@ static const struct ad_sigma_delta_info
> ad7780_sigma_delta_info = {
>  };
> 
>  #define AD7780_CHANNEL(bits, wordsize) \
> +       AD_SD_CHANNEL(1, 0, 0, bits, 32, wordsize - bits)
> +#define AD7170_CHANNEL(bits, wordsize) \
>         AD_SD_CHANNEL_NO_SAMP_FREQ(1, 0, 0, bits, 32, wordsize - bits)
> 
>  static const struct ad7780_chip_info ad7780_chip_info_tbl[] = {
>         [ID_AD7170] = {
> -               .channel = AD7780_CHANNEL(12, 24),
> +               .channel = AD7170_CHANNEL(12, 24),
>                 .pattern = AD7170_PATTERN,
>                 .pattern_mask = AD7170_PATTERN_MASK,
>                 .is_ad778x = false,
>         },
>         [ID_AD7171] = {
> -               .channel = AD7780_CHANNEL(16, 24),
> +               .channel = AD7170_CHANNEL(16, 24),
>                 .pattern = AD7170_PATTERN,
>                 .pattern_mask = AD7170_PATTERN_MASK,
>                 .is_ad778x = false,
> --
> 2.21.0
> 

Re: [PATCH v4 3/9] staging: iio: ad7780: set pattern values and masks directly

[Ardelean, Alexandru]

On Thu, 2019-02-28 at 11:24 -0300, Renato Lui Geh wrote:
> 
> 
> The AD7780 driver contains status pattern bits designed for checking
> whether serial transfers have been correctly performed. Pattern macros
> were previously generated through bit fields. This patch sets good
> pattern values directly and masks through GENMASK.
> 
> Signed-off-by: Renato Lui Geh <renatogeh@gmail.com>
> ---
>  drivers/staging/iio/adc/ad7780.c | 20 +++++++++-----------
>  1 file changed, 9 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7780.c
> b/drivers/staging/iio/adc/ad7780.c
> index 7a68e90ddf14..56c49e28f432 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -17,6 +17,7 @@
>  #include <linux/sched.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/module.h>
> +#include <linux/bits.h>
> 
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> @@ -28,16 +29,13 @@
>  #define AD7780_ID1             BIT(4)
>  #define AD7780_ID0             BIT(3)
>  #define AD7780_GAIN            BIT(2)
> -#define AD7780_PAT1            BIT(1)
> -#define AD7780_PAT0            BIT(0)

I don't see a problem to leave the bitfields;  they can be read & matched
easier with the datasheet.

> 
> -#define AD7780_PATTERN         (AD7780_PAT0)
> -#define AD7780_PATTERN_MASK    (AD7780_PAT0 | AD7780_PAT1)
> 
> -#define AD7170_PAT2            BIT(2)

> +#define AD7780_PATTERN_GOOD    1

It was also nice before that the PAT0..PAT2 bitfields were used to define a
good pattern, since it's easier to match with the datasheet.


> +#define AD7780_PATTERN_MASK    GENMASK(1, 0)

I like the general usage of GENMASK, but I'm not sure in this case it's
worth doing. Maybe I missed a discussion somewhere, about doing this
change, but it is mostly a cosmetic without any functional change.


> 
> -#define AD7170_PATTERN         (AD7780_PAT0 | AD7170_PAT2)
> -#define AD7170_PATTERN_MASK    (AD7780_PAT0 | AD7780_PAT1 | AD7170_PAT2)
> +#define AD7170_PATTERN_GOOD    5
> +#define AD7170_PATTERN_MASK    GENMASK(2, 0)
> 
>  #define AD7780_GAIN_MIDPOINT   64
>  #define AD7780_FILTER_MIDPOINT 13350
> @@ -209,25 +207,25 @@ static const struct ad_sigma_delta_info
> ad7780_sigma_delta_info = {
>  static const struct ad7780_chip_info ad7780_chip_info_tbl[] = {
>         [ID_AD7170] = {
>                 .channel = AD7170_CHANNEL(12, 24),
> -               .pattern = AD7170_PATTERN,
> +               .pattern = AD7170_PATTERN_GOOD,
>                 .pattern_mask = AD7170_PATTERN_MASK,
>                 .is_ad778x = false,
>         },
>         [ID_AD7171] = {
>                 .channel = AD7170_CHANNEL(16, 24),
> -               .pattern = AD7170_PATTERN,
> +               .pattern = AD7170_PATTERN_GOOD,
>                 .pattern_mask = AD7170_PATTERN_MASK,
>                 .is_ad778x = false,
>         },
>         [ID_AD7780] = {
>                 .channel = AD7780_CHANNEL(24, 32),
> -               .pattern = AD7780_PATTERN,
> +               .pattern = AD7780_PATTERN_GOOD,
>                 .pattern_mask = AD7780_PATTERN_MASK,
>                 .is_ad778x = true,
>         },
>         [ID_AD7781] = {
>                 .channel = AD7780_CHANNEL(20, 32),
> -               .pattern = AD7780_PATTERN,
> +               .pattern = AD7780_PATTERN_GOOD,
>                 .pattern_mask = AD7780_PATTERN_MASK,
>                 .is_ad778x = true,
>         },
> --
> 2.21.0
> 

Re: [PATCH v4 4/9] staging:iio:ad7780: add chip ID values and mask

[Ardelean, Alexandru]

On Thu, 2019-02-28 at 11:24 -0300, Renato Lui Geh wrote:
> 
> 
> The ad7780 supports both the ad778x and ad717x families. Each chip has
> a corresponding ID. This patch provides a mask for extracting ID values
> from the status bits and also macros for the correct values for the
> ad7170, ad7171, ad7780 and ad7781.
> 
> Signed-off-by: Renato Lui Geh <renatogeh@gmail.com>
> ---
>  drivers/staging/iio/adc/ad7780.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7780.c
> b/drivers/staging/iio/adc/ad7780.c
> index 56c49e28f432..ad7617a3a141 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -26,10 +26,14 @@
>  #define AD7780_RDY             BIT(7)
>  #define AD7780_FILTER          BIT(6)
>  #define AD7780_ERR             BIT(5)
> -#define AD7780_ID1             BIT(4)
> -#define AD7780_ID0             BIT(3)
>  #define AD7780_GAIN            BIT(2)
> 
> +#define AD7170_ID              0
> +#define AD7171_ID              1
> +#define AD7780_ID              1
> +#define AD7781_ID              0
> +
> +#define AD7780_ID_MASK         (BIT(3) | BIT(4))

This also doesn't have any functionality change.
The AD7170_ID, AD7171_ID, AD7780_ID & AD7781_ID IDs are also unused (maybe
in a later patch they are ?).

I would also leave the AD7780_ID1 & AD7780_ID0 definitions in place, since
they're easier matched with the datasheet.

> 
>  #define AD7780_PATTERN_GOOD    1
>  #define AD7780_PATTERN_MASK    GENMASK(1, 0)
> --
> 2.21.0
> 

Re: [PATCH v4 5/9] staging: iio: ad7780: move regulator to after GPIO init

[Ardelean, Alexandru]

On Thu, 2019-02-28 at 11:25 -0300, Renato Lui Geh wrote:
> 
> 
> To maintain consistency between ad7780_probe and ad7780_remove orders,
> regulator initialization has been moved to after GPIO initializations.
> 
> Signed-off-by: Renato Lui Geh <renatogeh@gmail.com>
> ---
>  drivers/staging/iio/adc/ad7780.c | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7780.c
> b/drivers/staging/iio/adc/ad7780.c
> index ad7617a3a141..12aef0f101bc 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -255,16 +255,6 @@ static int ad7780_probe(struct spi_device *spi)
> 
>         ad_sd_init(&st->sd, indio_dev, spi, &ad7780_sigma_delta_info);
> 
> -       st->reg = devm_regulator_get(&spi->dev, "avdd");
> -       if (IS_ERR(st->reg))
> -               return PTR_ERR(st->reg);
> -
> -       ret = regulator_enable(st->reg);
> -       if (ret) {
> -               dev_err(&spi->dev, "Failed to enable specified AVdd
> supply\n");
> -               return ret;
> -       }
> -
>         st->chip_info =
>                 &ad7780_chip_info_tbl[spi_get_device_id(spi)-
> >driver_data];
> 
> @@ -284,7 +274,7 @@ static int ad7780_probe(struct spi_device *spi)
>                 ret = PTR_ERR(st->powerdown_gpio);
>                 dev_err(&spi->dev, "Failed to request powerdown GPIO:
> %d\n",
>                         ret);
> -               goto error_disable_reg;
> +               return ret;
>         }
> 
>         if (st->chip_info->is_ad778x) {
> @@ -295,7 +285,7 @@ static int ad7780_probe(struct spi_device *spi)
>                         ret = PTR_ERR(st->gain_gpio);
>                         dev_err(&spi->dev, "Failed to request gain GPIO:
> %d\n",
>                                 ret);
> -                       goto error_disable_reg;
> +                       return ret;
>                 }
> 
>                 st->filter_gpio = devm_gpiod_get_optional(&spi->dev,
> @@ -306,10 +296,20 @@ static int ad7780_probe(struct spi_device *spi)
>                         dev_err(&spi->dev,
>                                 "Failed to request filter GPIO: %d\n",
>                                 ret);
> -                       goto error_disable_reg;
> +                       return ret;
>                 }
>         }
> 
> +       st->reg = devm_regulator_get(&spi->dev, "avdd");
> +       if (IS_ERR(st->reg))
> +               return PTR_ERR(st->reg);
> +
> +       ret = regulator_enable(st->reg);
> +       if (ret) {
> +               dev_err(&spi->dev, "Failed to enable specified AVdd
> supply\n");
> +               return ret;
> +       }
> +

I'm probably missing something, but other than the fact that this moves the
regulator init after the GPIOs init, it doesn't change much.
The order of the probe & remove is more-or-less the same.
The GPIOs will be free'd via devm_ API/stuff.


>         ret = ad_sd_setup_buffer_and_trigger(indio_dev);
>         if (ret)
>                 goto error_disable_reg;
> --
> 2.21.0
> 

Re: [PATCH v4 1/9] staging: iio: ad7780: add gain & filter gpio support

[Dan Carpenter]

On Fri, Mar 01, 2019 at 06:56:14AM +0000, Ardelean, Alexandru wrote:
> On Thu, 2019-02-28 at 11:23 -0300, Renato Lui Geh wrote:
> > 
> > 
> > Previously, the AD7780 driver only supported gpio for the 'powerdown'
> > pin. This commit adds suppport for the 'gain' and 'filter' pin.
> > 
> > Signed-off-by: Renato Lui Geh <renatogeh@gmail.com>
> > Signed-off-by: Giuliano Belinassi <giuliano.belinassi@usp.br>
> > Co-developed-by: Giuliano Belinassi <giuliano.belinassi@usp.br>
> > ---
> > Changes in v3:
> >  - Renamed ad7780_chip_info's filter to odr
> >  - Renamed ad778x_filter to ad778x_odr_avail
> >  - Changed vref variable from unsigned int to unsigned long long to avoid
> >    overflow
> >  - Removed unnecessary AD_SD_CHANNEL macro
> > Changes in v4:
> >  - Removed useless macro
> >  - Added default case for switch to suppress warning
> >  - Removed chunks belonging to filter reading, adding these as a
> >    patch for itself
> > 
> >  drivers/staging/iio/adc/ad7780.c | 90 +++++++++++++++++++++++++++++---
> >  1 file changed, 84 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/staging/iio/adc/ad7780.c
> > b/drivers/staging/iio/adc/ad7780.c
> > index c4a85789c2db..87fbcf510d45 100644
> > --- a/drivers/staging/iio/adc/ad7780.c
> > +++ b/drivers/staging/iio/adc/ad7780.c
> > @@ -39,6 +39,12 @@
> >  #define AD7170_PATTERN         (AD7780_PAT0 | AD7170_PAT2)
> >  #define AD7170_PATTERN_MASK    (AD7780_PAT0 | AD7780_PAT1 | AD7170_PAT2)
> > 
> > +#define AD7780_GAIN_MIDPOINT   64
> > +#define AD7780_FILTER_MIDPOINT 13350
> > +
> > +static const unsigned int ad778x_gain[2]      = { 1, 128 };
> > +static const unsigned int ad778x_odr_avail[2] = { 10000, 16700 };
> 
> ad778x_odr_avail[2] is not used in this patch, so it should probably go
> into the next one 
> (i.e. staging: iio: ad7780: add filter reading to ad778x )
> 
> one good way of catching stuff like this is to do interactive rebase and
> compile your driver on each patch to see if the compiler catches this;
> i suspect the compiler would have thrown an error for this change
> 

GCC (v7.3) doesn't complain about unused variables if they're const.

regards,
dan carpenter

Re: [PATCH v4 0/9] staging: iio: ad7780: move out of staging

[Ardelean, Alexandru]

On Thu, 2019-02-28 at 11:23 -0300, Renato Lui Geh wrote:
> 

The patch-series is a bit big.
I guess that the intent is to move this out-of-staging, but various patches
are holding this in it's place.
For patch series above a certain size, you could get many re-spins
[V2,3,4... so on].

You could send some of the changes as individual patches, or group them in
series of 1,2 or 3 patches. That way, you "parallelize" patch sending, and
when you get reviews on each patch, you can re-spin them individually.
You'll find over time that certain patches get accepted on V1, others on V2
and some on V7 [ hopefully, there isn't any frustration at that point ].

Well, this is a technique I use to distribute some of my upstream-patch-
work, so that I can switch easier between internal-work & upstreaming-work.

Coming back to this patch-series.
My general input, is that the patches are fine over-all; some are just
cosmetics/noise/a-different-way-of-doing-things-for-this-driver, and those
usually can be left to preference [of the maintainer usually].

I do suggest to not hurry when re-spinning patches, and not change too much
the number of patches in a new series. That can complicate things
sometimes. But, if doing small patch-series or individual patches, you
won't have this problem too much.

Thanks
Alex

> 
> This series of patches contains the following:
>  - Adds user input for the 'gain' and 'filter' GPIO pins for the ad778x
>    family chips;
>  - Filter reading for the ad778x;
>  - Sets pattern macro values and mask for PATTERN status bits;
>  - Adds ID values for the ad7170, ad7171, ad7780 and ad7781 for ID
>    status bits checking;
>  - Moves regulator initialization to after GPIO init to maintain
>    consistency between probe and remove;
>  - Copyright edits, adding SPDX identifier and new copyright holder;
>  - Moves the ad7780 driver out of staging to the mainline;
>  - Adds device tree binding for the ad7780 driver.
> 
> Renato Lui Geh (9):
>   staging: iio: ad7780: add gain & filter gpio support
>   staging: iio: ad7780: add filter reading to ad778x
>   staging: iio: ad7780: set pattern values and masks directly
>   staging:iio:ad7780: add chip ID values and mask
>   staging: iio: ad7780: move regulator to after GPIO init
>   staging: iio: ad7780: add SPDX identifier
>   staging: iio: ad7780: add new copyright holder
>   staging: iio: ad7780: moving ad7780 out of staging
>   staging: iio: ad7780: add device tree binding
> 
> Changelog:
> *v3
>  - SPDX and regulator init as patches
>  - Renamed filter to odr and ad778x_filter to ad778x_odr_avail
>  - Removed unnecessary regulator disabling
>  - Removed unnecessary AD_SD_CHANNEL macro
>  - Changed unsigned int to unsigned long long to avoid overflow
> *v4
>  - Split gain & filter patch into two, with the new commit adding only
>    filter reading
>  - Changed pattern values to direct values, and added pattern mask
>  - Added ID values and mask
>  - Added new copyright holder
>  - Added device tree binding to the ad7780 driver
> 
>  .../bindings/iio/adc/adi,ad7780.txt           |  48 +++
>  drivers/iio/adc/Kconfig                       |  12 +
>  drivers/iio/adc/Makefile                      |   1 +
>  drivers/iio/adc/ad7780.c                      | 365 ++++++++++++++++++
>  drivers/staging/iio/adc/Kconfig               |  13 -
>  drivers/staging/iio/adc/Makefile              |   1 -
>  drivers/staging/iio/adc/ad7780.c              | 277 -------------
>  7 files changed, 426 insertions(+), 291 deletions(-)
>  create mode 100644
> Documentation/devicetree/bindings/iio/adc/adi,ad7780.txt
>  create mode 100644 drivers/iio/adc/ad7780.c
>  delete mode 100644 drivers/staging/iio/adc/ad7780.c
> 
> --
> 2.21.0
> 

Re: [PATCH v4 1/9] staging: iio: ad7780: add gain & filter gpio support

[Jonathan Cameron]

On Fri, 1 Mar 2019 06:56:14 +0000
"Ardelean, Alexandru" <Alex.Ardelean@analog.com> wrote:

> On Thu, 2019-02-28 at 11:23 -0300, Renato Lui Geh wrote:
> > 
> > 
> > Previously, the AD7780 driver only supported gpio for the 'powerdown'
> > pin. This commit adds suppport for the 'gain' and 'filter' pin.
> > 
> > Signed-off-by: Renato Lui Geh <renatogeh@gmail.com>
> > Signed-off-by: Giuliano Belinassi <giuliano.belinassi@usp.br>
> > Co-developed-by: Giuliano Belinassi <giuliano.belinassi@usp.br>

A few follow up comments (particularly as Alex asked questions ;)

> > ---
> > Changes in v3:
> >  - Renamed ad7780_chip_info's filter to odr
> >  - Renamed ad778x_filter to ad778x_odr_avail
> >  - Changed vref variable from unsigned int to unsigned long long to avoid
> >    overflow
> >  - Removed unnecessary AD_SD_CHANNEL macro
> > Changes in v4:
> >  - Removed useless macro
> >  - Added default case for switch to suppress warning
> >  - Removed chunks belonging to filter reading, adding these as a
> >    patch for itself
> > 
> >  drivers/staging/iio/adc/ad7780.c | 90 +++++++++++++++++++++++++++++---
> >  1 file changed, 84 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/staging/iio/adc/ad7780.c
> > b/drivers/staging/iio/adc/ad7780.c
> > index c4a85789c2db..87fbcf510d45 100644
> > --- a/drivers/staging/iio/adc/ad7780.c
> > +++ b/drivers/staging/iio/adc/ad7780.c
> > @@ -39,6 +39,12 @@
> >  #define AD7170_PATTERN         (AD7780_PAT0 | AD7170_PAT2)
> >  #define AD7170_PATTERN_MASK    (AD7780_PAT0 | AD7780_PAT1 | AD7170_PAT2)
> > 
> > +#define AD7780_GAIN_MIDPOINT   64
> > +#define AD7780_FILTER_MIDPOINT 13350
> > +
> > +static const unsigned int ad778x_gain[2]      = { 1, 128 };
> > +static const unsigned int ad778x_odr_avail[2] = { 10000, 16700 };  
> 
> ad778x_odr_avail[2] is not used in this patch, so it should probably go
> into the next one 
> (i.e. staging: iio: ad7780: add filter reading to ad778x )
> 
> one good way of catching stuff like this is to do interactive rebase and
> compile your driver on each patch to see if the compiler catches this;
> i suspect the compiler would have thrown an error for this change
> 
> 
> > 
> >  struct ad7780_chip_info
> >         struct iio_chan_spec    channel;
> >         unsigned int            pattern_mask;
> > @@ -50,7 +56,10 @@ struct ad7780_state {
> >         const struct ad7780_chip_info   *chip_info;
> >         struct regulator                *reg;
> >         struct gpio_desc                *powerdown_gpio;
> > -       unsigned int    gain;
> > +       struct gpio_desc                *gain_gpio;
> > +       struct gpio_desc                *filter_gpio;
> > +       unsigned int                    gain;
> > +       unsigned int                    int_vref_mv;
> > 
> >         struct ad_sigma_delta sd;
> >  };
> > @@ -104,17 +113,65 @@ static int ad7780_read_raw(struct iio_dev
> > *indio_dev,
> >                 voltage_uv = regulator_get_voltage(st->reg);
> >                 if (voltage_uv < 0)
> >                         return voltage_uv;
> > -               *val = (voltage_uv / 1000) * st->gain;
> > +               voltage_uv /= 1000;
> > +               *val = voltage_uv * st->gain;
> >                 *val2 = chan->scan_type.realbits - 1;
> > +               st->int_vref_mv = voltage_uv;
> >                 return IIO_VAL_FRACTIONAL_LOG2;
> >         case IIO_CHAN_INFO_OFFSET:
> >                 *val = -(1 << (chan->scan_type.realbits - 1));
> >                 return IIO_VAL_INT;
> > +       default:
> > +       break;  
> 
> The indentation of the break statement is inconsistent with other places.
> Still, it does not add much value adding this change as-is, since it does
> not change any behavior, and is not an element needed by this change (i.e.
> adding gain & filter support via gpios)

Agreed. This is a tidy up.  Good one, but put it in a series doing just
tidy ups.

> 
> >         }
> > 
> >         return -EINVAL;
> >  }
> > 
> > +static int ad7780_write_raw(struct iio_dev *indio_dev,
> > +                           struct iio_chan_spec const *chan,
> > +                           int val,
> > +                           int val2,
> > +                           long m)
> > +{
> > +       struct ad7780_state *st = iio_priv(indio_dev);
> > +       const struct ad7780_chip_info *chip_info = st->chip_info;
> > +       unsigned long long vref;
> > +       unsigned int full_scale, gain;
> > +
> > +       if (!chip_info->is_ad778x)
> > +               return 0;

Should return an error I think?  Any such write isn't valid.

> > +
> > +       switch (m) {
> > +       case IIO_CHAN_INFO_SCALE:
> > +               if (val != 0)
> > +                       return -EINVAL;
> > +
> > +               vref = st->int_vref_mv * 1000000LL;
> > +               full_scale = 1 << (chip_info->channel.scan_type.realbits
> > - 1);
> > +               gain = DIV_ROUND_CLOSEST(vref, full_scale);
> > +               gain = DIV_ROUND_CLOSEST(gain, val2);
> > +               st->gain = gain;
> > +               if (gain < AD7780_GAIN_MIDPOINT)
> > +                       gain = 0;
> > +               else
> > +                       gain = 1;
> > +               gpiod_set_value(st->gain_gpio, gain);
> > +               break;
> > +       case IIO_CHAN_INFO_SAMP_FREQ:
> > +               if (1000*val + val2/1000 < AD7780_FILTER_MIDPOINT)
> > +                       val = 0;
> > +               else
> > +                       val = 1;
> > +               gpiod_set_value(st->filter_gpio, val);
> > +               break;
> > +       default:
> > +       break;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> >  static int ad7780_postprocess_sample(struct ad_sigma_delta *sigma_delta,
> >                                      unsigned int raw_sample)
> >  {
> > @@ -126,10 +183,7 @@ static int ad7780_postprocess_sample(struct
> > ad_sigma_delta *sigma_delta,
> >                 return -EIO;
> > 
> >         if (chip_info->is_ad778x) {
> > -               if (raw_sample & AD7780_GAIN)
> > -                       st->gain = 1;
> > -               else
> > -                       st->gain = 128;
> > +               st->gain = ad778x_gain[raw_sample & AD7780_GAIN];  
> 
> The new `ad778x_gain[]` array could have been it's own patch, but from my
> side it's fine to leave it here.
> I do like this change, but it's not a patch that semantically has to do
> anything with adding gain & filter gpio support.
> 
> 
> Let's see what Jonathan says.

It's small enough I don't mind, but ideal would have been a precursor patch
doing that refactoring.

> 
> >         }
> > 
> >         return 0;
> > @@ -173,6 +227,7 @@ static const struct ad7780_chip_info
> > ad7780_chip_info_tbl[] = {
> > 
> >  static const struct iio_info ad7780_info = {
> >         .read_raw = ad7780_read_raw,
> > +       .write_raw = ad7780_write_raw,
> >  };
> > 
> >  static int ad7780_probe(struct spi_device *spi)
> > @@ -222,6 +277,29 @@ static int ad7780_probe(struct spi_device *spi)
> >                 goto error_disable_reg;
> >         }
> > 
> > +       if (st->chip_info->is_ad778x) {
> > +               st->gain_gpio = devm_gpiod_get_optional(&spi->dev,
> > +                                                       "adi,gain",
> > +                                                       GPIOD_OUT_HIGH);
> > +               if (IS_ERR(st->gain_gpio)) {
> > +                       ret = PTR_ERR(st->gain_gpio);
> > +                       dev_err(&spi->dev, "Failed to request gain GPIO:
> > %d\n",
> > +                               ret);
> > +                       goto error_disable_reg;
> > +               }
> > +
> > +               st->filter_gpio = devm_gpiod_get_optional(&spi->dev,
> > +                                                         "adi,filter",
> > +                                                         GPIOD_OUT_HIGH)
> > ;
> > +               if (IS_ERR(st->filter_gpio)) {
> > +                       ret = PTR_ERR(st->filter_gpio);
> > +                       dev_err(&spi->dev,
> > +                               "Failed to request filter GPIO: %d\n",
> > +                               ret);
> > +                       goto error_disable_reg;
> > +               }
> > +       }
> > +  
> 
> This is just a preference of mine [feel free to ignore for this patch].
> But, for this block of code, I would have added a separate function [
> called something like ad7780_init_gpios(struct device *dev, struct
> ad7780_state *st) )
> 
> you could also move the powerdown gpio there; and do something like
> 
> static int ad7780_init_gpios(struct device *dev, struct ad7780_state *st)
> {
> 
>       st->powerdown_gpio = devm_gpiod_get_optional(dev,
>                                                      "powerdown",
>                                                      GPIOD_OUT_LOW);
>         if (IS_ERR(st->powerdown_gpio)) {
>               ret = PTR_ERR(st->powerdown_gpio);
>               dev_err(dev, "Failed to request powerdown GPIO: %d\n", ret);
>               return ret;
>         }
> 
>        /**
>         * Note Alex: I'm a big fan of keeping things to a minimal 
>         * indentation level [especially where things can be elegant], 
>         * but that's a preference of mine
>         */

Me too ;)  Fast exit is always nice as well as saves anyone who cares
about this condition looking further.

>        if (!st->chip_info->is_ad778x)
>            return 0;
> 
> 
>         st->gain_gpio = devm_gpiod_get_optional(dev,
>                         "adi,gain",
>                         GPIOD_OUT_HIGH);
>         if (IS_ERR(st->gain_gpio)) {
>                 ret = PTR_ERR(st->gain_gpio);
>                 dev_err(dev, "Failed to request gain GPIO: %d\n", ret);
>                 return ret;
>         }
> 
>         st->filter_gpio = devm_gpiod_get_optional(dev,
>                         "adi,filter",
>                         GPIOD_OUT_HIGH);
> 
>         if (IS_ERR(st->filter_gpio)) {
>                 ret = PTR_ERR(st->filter_gpio);
>                 dev_err(dev, "Failed to request filter GPIO: %d\n", ret);
>                 return ret;
>         }
> 
>         return 0;
> }
> 
> 
> 
> >         ret = ad_sd_setup_buffer_and_trigger(indio_dev);
> >         if (ret)
> >                 goto error_disable_reg;
> > --
> > 2.21.0
> >   

Re: [PATCH v4 3/9] staging: iio: ad7780: set pattern values and masks directly

[Jonathan Cameron]

On Fri, 1 Mar 2019 07:17:04 +0000
"Ardelean, Alexandru" <Alex.Ardelean@analog.com> wrote:

> On Thu, 2019-02-28 at 11:24 -0300, Renato Lui Geh wrote:
> > 
> > 
> > The AD7780 driver contains status pattern bits designed for checking
> > whether serial transfers have been correctly performed. Pattern macros
> > were previously generated through bit fields. This patch sets good
> > pattern values directly and masks through GENMASK.
> > 
> > Signed-off-by: Renato Lui Geh <renatogeh@gmail.com>
> > ---
> >  drivers/staging/iio/adc/ad7780.c | 20 +++++++++-----------
> >  1 file changed, 9 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/staging/iio/adc/ad7780.c
> > b/drivers/staging/iio/adc/ad7780.c
> > index 7a68e90ddf14..56c49e28f432 100644
> > --- a/drivers/staging/iio/adc/ad7780.c
> > +++ b/drivers/staging/iio/adc/ad7780.c
> > @@ -17,6 +17,7 @@
> >  #include <linux/sched.h>
> >  #include <linux/gpio/consumer.h>
> >  #include <linux/module.h>
> > +#include <linux/bits.h>
> > 
> >  #include <linux/iio/iio.h>
> >  #include <linux/iio/sysfs.h>
> > @@ -28,16 +29,13 @@
> >  #define AD7780_ID1             BIT(4)
> >  #define AD7780_ID0             BIT(3)
> >  #define AD7780_GAIN            BIT(2)
> > -#define AD7780_PAT1            BIT(1)
> > -#define AD7780_PAT0            BIT(0)  
> 
> I don't see a problem to leave the bitfields;  they can be read & matched
> easier with the datasheet.
> 
> > 
> > -#define AD7780_PATTERN         (AD7780_PAT0)
> > -#define AD7780_PATTERN_MASK    (AD7780_PAT0 | AD7780_PAT1)
> > 
> > -#define AD7170_PAT2            BIT(2)  
> 
> > +#define AD7780_PATTERN_GOOD    1  
> 
> It was also nice before that the PAT0..PAT2 bitfields were used to define a
> good pattern, since it's easier to match with the datasheet.
This one was much suggestion.  Not particularly important one.

Personally if a datasheet is pointlessly confusing I tend to ignore it.
This is a two bit field as the bits don't have independent meaning!

I'm not strongly tied to it though and as it's an Analog driver and
you all do a good job maintaining the set I'll go with your preference!
I do prefer the naming of PATTERN_GOOD though if nothing else!
> 
> 
> > +#define AD7780_PATTERN_MASK    GENMASK(1, 0)  
> 
> I like the general usage of GENMASK, but I'm not sure in this case it's
> worth doing. Maybe I missed a discussion somewhere, about doing this
> change, but it is mostly a cosmetic without any functional change.
> 
> 
> > 
> > -#define AD7170_PATTERN         (AD7780_PAT0 | AD7170_PAT2)
> > -#define AD7170_PATTERN_MASK    (AD7780_PAT0 | AD7780_PAT1 | AD7170_PAT2)
> > +#define AD7170_PATTERN_GOOD    5
> > +#define AD7170_PATTERN_MASK    GENMASK(2, 0)
> > 
> >  #define AD7780_GAIN_MIDPOINT   64
> >  #define AD7780_FILTER_MIDPOINT 13350
> > @@ -209,25 +207,25 @@ static const struct ad_sigma_delta_info
> > ad7780_sigma_delta_info = {
> >  static const struct ad7780_chip_info ad7780_chip_info_tbl[] = {
> >         [ID_AD7170] = {
> >                 .channel = AD7170_CHANNEL(12, 24),
> > -               .pattern = AD7170_PATTERN,
> > +               .pattern = AD7170_PATTERN_GOOD,
> >                 .pattern_mask = AD7170_PATTERN_MASK,
> >                 .is_ad778x = false,
> >         },
> >         [ID_AD7171] = {
> >                 .channel = AD7170_CHANNEL(16, 24),
> > -               .pattern = AD7170_PATTERN,
> > +               .pattern = AD7170_PATTERN_GOOD,
> >                 .pattern_mask = AD7170_PATTERN_MASK,
> >                 .is_ad778x = false,
> >         },
> >         [ID_AD7780] = {
> >                 .channel = AD7780_CHANNEL(24, 32),
> > -               .pattern = AD7780_PATTERN,
> > +               .pattern = AD7780_PATTERN_GOOD,
> >                 .pattern_mask = AD7780_PATTERN_MASK,
> >                 .is_ad778x = true,
> >         },
> >         [ID_AD7781] = {
> >                 .channel = AD7780_CHANNEL(20, 32),
> > -               .pattern = AD7780_PATTERN,
> > +               .pattern = AD7780_PATTERN_GOOD,
> >                 .pattern_mask = AD7780_PATTERN_MASK,
> >                 .is_ad778x = true,
> >         },
> > --
> > 2.21.0
> >   

Re: [PATCH v4 3/9] staging: iio: ad7780: set pattern values and masks directly

[Jonathan Cameron]

On Sat, 2 Mar 2019 19:07:16 +0000
Jonathan Cameron <jic23@kernel.org> wrote:

> On Fri, 1 Mar 2019 07:17:04 +0000
> "Ardelean, Alexandru" <Alex.Ardelean@analog.com> wrote:
> 
> > On Thu, 2019-02-28 at 11:24 -0300, Renato Lui Geh wrote:  
> > > 
> > > 
> > > The AD7780 driver contains status pattern bits designed for checking
> > > whether serial transfers have been correctly performed. Pattern macros
> > > were previously generated through bit fields. This patch sets good
> > > pattern values directly and masks through GENMASK.
> > > 
> > > Signed-off-by: Renato Lui Geh <renatogeh@gmail.com>
> > > ---
> > >  drivers/staging/iio/adc/ad7780.c | 20 +++++++++-----------
> > >  1 file changed, 9 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/drivers/staging/iio/adc/ad7780.c
> > > b/drivers/staging/iio/adc/ad7780.c
> > > index 7a68e90ddf14..56c49e28f432 100644
> > > --- a/drivers/staging/iio/adc/ad7780.c
> > > +++ b/drivers/staging/iio/adc/ad7780.c
> > > @@ -17,6 +17,7 @@
> > >  #include <linux/sched.h>
> > >  #include <linux/gpio/consumer.h>
> > >  #include <linux/module.h>
> > > +#include <linux/bits.h>
> > > 
> > >  #include <linux/iio/iio.h>
> > >  #include <linux/iio/sysfs.h>
> > > @@ -28,16 +29,13 @@
> > >  #define AD7780_ID1             BIT(4)
> > >  #define AD7780_ID0             BIT(3)
> > >  #define AD7780_GAIN            BIT(2)
> > > -#define AD7780_PAT1            BIT(1)
> > > -#define AD7780_PAT0            BIT(0)    
> > 
> > I don't see a problem to leave the bitfields;  they can be read & matched
> > easier with the datasheet.
> >   
> > > 
> > > -#define AD7780_PATTERN         (AD7780_PAT0)
> > > -#define AD7780_PATTERN_MASK    (AD7780_PAT0 | AD7780_PAT1)
> > > 
> > > -#define AD7170_PAT2            BIT(2)    
> >   
> > > +#define AD7780_PATTERN_GOOD    1    
> > 
> > It was also nice before that the PAT0..PAT2 bitfields were used to define a
> > good pattern, since it's easier to match with the datasheet.  
> This one was much suggestion.  Not particularly important one.
Not enough sleep this week clearly :)  
This one was _my_ suggestion.

> 
> Personally if a datasheet is pointlessly confusing I tend to ignore it.
> This is a two bit field as the bits don't have independent meaning!
> 
> I'm not strongly tied to it though and as it's an Analog driver and
> you all do a good job maintaining the set I'll go with your preference!
> I do prefer the naming of PATTERN_GOOD though if nothing else!
> > 
> >   
> > > +#define AD7780_PATTERN_MASK    GENMASK(1, 0)    
> > 
> > I like the general usage of GENMASK, but I'm not sure in this case it's
> > worth doing. Maybe I missed a discussion somewhere, about doing this
> > change, but it is mostly a cosmetic without any functional change.
> > 
> >   
> > > 
> > > -#define AD7170_PATTERN         (AD7780_PAT0 | AD7170_PAT2)
> > > -#define AD7170_PATTERN_MASK    (AD7780_PAT0 | AD7780_PAT1 | AD7170_PAT2)
> > > +#define AD7170_PATTERN_GOOD    5
> > > +#define AD7170_PATTERN_MASK    GENMASK(2, 0)
> > > 
> > >  #define AD7780_GAIN_MIDPOINT   64
> > >  #define AD7780_FILTER_MIDPOINT 13350
> > > @@ -209,25 +207,25 @@ static const struct ad_sigma_delta_info
> > > ad7780_sigma_delta_info = {
> > >  static const struct ad7780_chip_info ad7780_chip_info_tbl[] = {
> > >         [ID_AD7170] = {
> > >                 .channel = AD7170_CHANNEL(12, 24),
> > > -               .pattern = AD7170_PATTERN,
> > > +               .pattern = AD7170_PATTERN_GOOD,
> > >                 .pattern_mask = AD7170_PATTERN_MASK,
> > >                 .is_ad778x = false,
> > >         },
> > >         [ID_AD7171] = {
> > >                 .channel = AD7170_CHANNEL(16, 24),
> > > -               .pattern = AD7170_PATTERN,
> > > +               .pattern = AD7170_PATTERN_GOOD,
> > >                 .pattern_mask = AD7170_PATTERN_MASK,
> > >                 .is_ad778x = false,
> > >         },
> > >         [ID_AD7780] = {
> > >                 .channel = AD7780_CHANNEL(24, 32),
> > > -               .pattern = AD7780_PATTERN,
> > > +               .pattern = AD7780_PATTERN_GOOD,
> > >                 .pattern_mask = AD7780_PATTERN_MASK,
> > >                 .is_ad778x = true,
> > >         },
> > >         [ID_AD7781] = {
> > >                 .channel = AD7780_CHANNEL(20, 32),
> > > -               .pattern = AD7780_PATTERN,
> > > +               .pattern = AD7780_PATTERN_GOOD,
> > >                 .pattern_mask = AD7780_PATTERN_MASK,
> > >                 .is_ad778x = true,
> > >         },
> > > --
> > > 2.21.0
> > >     
> 

Re: [PATCH v4 5/9] staging: iio: ad7780: move regulator to after GPIO init

[Jonathan Cameron]

On Fri, 1 Mar 2019 07:38:45 +0000
"Ardelean, Alexandru" <Alex.Ardelean@analog.com> wrote:

> On Thu, 2019-02-28 at 11:25 -0300, Renato Lui Geh wrote:
> > 
> > 
> > To maintain consistency between ad7780_probe and ad7780_remove orders,
> > regulator initialization has been moved to after GPIO initializations.
> > 
> > Signed-off-by: Renato Lui Geh <renatogeh@gmail.com>
> > ---
> >  drivers/staging/iio/adc/ad7780.c | 26 +++++++++++++-------------
> >  1 file changed, 13 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/staging/iio/adc/ad7780.c
> > b/drivers/staging/iio/adc/ad7780.c
> > index ad7617a3a141..12aef0f101bc 100644
> > --- a/drivers/staging/iio/adc/ad7780.c
> > +++ b/drivers/staging/iio/adc/ad7780.c
> > @@ -255,16 +255,6 @@ static int ad7780_probe(struct spi_device *spi)
> > 
> >         ad_sd_init(&st->sd, indio_dev, spi, &ad7780_sigma_delta_info);
> > 
> > -       st->reg = devm_regulator_get(&spi->dev, "avdd");
> > -       if (IS_ERR(st->reg))
> > -               return PTR_ERR(st->reg);
> > -
> > -       ret = regulator_enable(st->reg);
> > -       if (ret) {
> > -               dev_err(&spi->dev, "Failed to enable specified AVdd
> > supply\n");
> > -               return ret;
> > -       }
> > -
> >         st->chip_info =
> >                 &ad7780_chip_info_tbl[spi_get_device_id(spi)-  
> > >driver_data];  
> > 
> > @@ -284,7 +274,7 @@ static int ad7780_probe(struct spi_device *spi)
> >                 ret = PTR_ERR(st->powerdown_gpio);
> >                 dev_err(&spi->dev, "Failed to request powerdown GPIO:
> > %d\n",
> >                         ret);
> > -               goto error_disable_reg;
> > +               return ret;
> >         }
> > 
> >         if (st->chip_info->is_ad778x) {
> > @@ -295,7 +285,7 @@ static int ad7780_probe(struct spi_device *spi)
> >                         ret = PTR_ERR(st->gain_gpio);
> >                         dev_err(&spi->dev, "Failed to request gain GPIO:
> > %d\n",
> >                                 ret);
> > -                       goto error_disable_reg;
> > +                       return ret;
> >                 }
> > 
> >                 st->filter_gpio = devm_gpiod_get_optional(&spi->dev,
> > @@ -306,10 +296,20 @@ static int ad7780_probe(struct spi_device *spi)
> >                         dev_err(&spi->dev,
> >                                 "Failed to request filter GPIO: %d\n",
> >                                 ret);
> > -                       goto error_disable_reg;
> > +                       return ret;
> >                 }
> >         }
> > 
> > +       st->reg = devm_regulator_get(&spi->dev, "avdd");
> > +       if (IS_ERR(st->reg))
> > +               return PTR_ERR(st->reg);
> > +
> > +       ret = regulator_enable(st->reg);
> > +       if (ret) {
> > +               dev_err(&spi->dev, "Failed to enable specified AVdd
> > supply\n");
> > +               return ret;
> > +       }
> > +  
> 
> I'm probably missing something, but other than the fact that this moves the
> regulator init after the GPIOs init, it doesn't change much.
> The order of the probe & remove is more-or-less the same.
> The GPIOs will be free'd via devm_ API/stuff.

This is another one from me.  I'm a fanatic at times when it comes
to probe and remove orders being precise reverses.  It just makes
review easier.  Nice to not actually have to think.

So I agree there is no 'actual' effect but it is in my view
still worth doing.

Jonathan

> 
> 
> >         ret = ad_sd_setup_buffer_and_trigger(indio_dev);
> >         if (ret)
> >                 goto error_disable_reg;
> > --
> > 2.21.0
> >   

Re: [PATCH v4 9/9] staging: iio: ad7780: add device tree binding

[Jonathan Cameron]

On Thu, 28 Feb 2019 11:26:28 -0300
Renato Lui Geh <renatogeh@gmail.com> wrote:

> Adds a device tree binding for the ad7780 driver.
> 
> Signed-off-by: Renato Lui Geh <renatogeh@gmail.com>
Looks good to me, but we'll see if Rob picks up on things
I've missed.

Jonathan

> ---
>  .../bindings/iio/adc/adi,ad7780.txt           | 48 +++++++++++++++++++
>  1 file changed, 48 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad7780.txt
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7780.txt b/Documentation/devicetree/bindings/iio/adc/adi,ad7780.txt
> new file mode 100644
> index 000000000000..440e52555349
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7780.txt
> @@ -0,0 +1,48 @@
> +* Analog Devices AD7170/AD7171/AD7780/AD7781
> +
> +Data sheets:
> +
> +- AD7170:
> +	* https://www.analog.com/media/en/technical-documentation/data-sheets/AD7170.pdf
> +- AD7171:
> +	* https://www.analog.com/media/en/technical-documentation/data-sheets/AD7171.pdf
> +- AD7780:
> +	* https://www.analog.com/media/en/technical-documentation/data-sheets/ad7780.pdf
> +- AD7781:
> +	* https://www.analog.com/media/en/technical-documentation/data-sheets/AD7781.pdf
> +
> +Required properties:
> +
> +- compatible: should be one of
> +	* "adi,ad7170"
> +	* "adi,ad7171"
> +	* "adi,ad7780"
> +	* "adi,ad7781"
> +- reg: spi chip select number for the device
> +- vref-supply: the regulator supply for the ADC reference voltage
> +
> +Optional properties:
> +
> +- powerdown-gpios:  must be the device tree identifier of the PDRST pin. If
> +		    specified, it will be asserted during driver probe. As the
> +		    line is active high, it should be marked GPIO_ACTIVE_HIGH.
> +- adi,gain-gpios:   must be the device tree identifier of the GAIN pin. Only for
> +		    the ad778x chips. If specified, it will be asserted during
> +		    driver probe. As the line is active low, it should be marked
> +		    GPIO_ACTIVE_LOW.
> +- adi,filter-gpios: must be the device tree identifier of the FILTER pin. Only
> +		    for the ad778x chips. If specified, it will be asserted
> +		    during driver probe. As the line is active low, it should be
> +		    marked GPIO_ACTIVE_LOW.
> +
> +Example:
> +
> +adc@0 {
> +	compatible =  "adi,ad7780";
> +	reg =	      <0>;
> +	vref-supply = <&vdd_supply>
> +
> +	powerdown-gpios  = <&gpio 12 GPIO_ACTIVE_HIGH>;
> +	adi,gain-gpios   = <&gpio  5 GPIO_ACTIVE_LOW>;
> +	adi,filter-gpios = <&gpio 15 GPIO_ACTIVE_LOW>;
> +};

Re: [PATCH v4 8/9] staging: iio: ad7780: moving ad7780 out of staging

[Jonathan Cameron]

On Thu, 28 Feb 2019 11:26:08 -0300
Renato Lui Geh <renatogeh@gmail.com> wrote:

> Move ad7780 ADC driver out of staging and into the mainline.
> 
> The ad7780 is a sigma-delta analog to digital converter. This driver provides
> reading voltage values and status bits from both the ad778x and ad717x series.
> Its interface also allows writing on the FILTER and GAIN GPIO pins on the
> ad778x.
> 
> Signed-off-by: Renato Lui Geh <renatogeh@gmail.com>
> Signed-off-by: Giuliano Belinassi <giuliano.belinassi@usp.br>
> Co-developed-by: Giuliano Belinassi <giuliano.belinassi@usp.br>

I think this is fine up to the minor tweaks Alex raised in the
precursor patches.  Should be good to move in v5 :)

Jonathan

> ---
> Changes in v3:
>  - Changes unrelated to moving the driver to main tree were resent as
>    individual patches
> Changes in v4:
>  - Removed line warning it was safe to build the ad7780 driver by
>    default
> 
> About the above change in v4, I was unsure whether it was ok to add this
> in this patch, as 'move out of staging' patches should be just that. But
> this change is so minimal I'm willing to risk a v5. Bottom line: should
> I add this change as another patch?
> 
>  drivers/iio/adc/Kconfig          |  12 +
>  drivers/iio/adc/Makefile         |   1 +
>  drivers/iio/adc/ad7780.c         | 365 +++++++++++++++++++++++++++++++
>  drivers/staging/iio/adc/Kconfig  |  13 --
>  drivers/staging/iio/adc/Makefile |   1 -
>  drivers/staging/iio/adc/ad7780.c | 365 -------------------------------
>  6 files changed, 378 insertions(+), 379 deletions(-)
>  create mode 100644 drivers/iio/adc/ad7780.c
>  delete mode 100644 drivers/staging/iio/adc/ad7780.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index f3cc7a31bce5..6b36a45bd09f 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -108,6 +108,18 @@ config AD7766
>  	  To compile this driver as a module, choose M here: the module will be
>  	  called ad7766.
>  
> +config AD7780
> +	tristate "Analog Devices AD7780 and similar ADCs driver"
> +	depends on SPI
> +	depends on GPIOLIB || COMPILE_TEST
> +	select AD_SIGMA_DELTA
> +	help
> +	  Say yes here to build support for Analog Devices AD7170, AD7171,
> +	  AD7780 and AD7781 SPI analog to digital converters (ADC).
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called ad7780.
> +
>  config AD7791
>  	tristate "Analog Devices AD7791 ADC driver"
>  	depends on SPI
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index ea5031348052..b48852157115 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -15,6 +15,7 @@ obj-$(CONFIG_AD7606_IFACE_PARALLEL) += ad7606_par.o
>  obj-$(CONFIG_AD7606_IFACE_SPI) += ad7606_spi.o
>  obj-$(CONFIG_AD7606) += ad7606.o
>  obj-$(CONFIG_AD7766) += ad7766.o
> +obj-$(CONFIG_AD7780) += ad7780.o
>  obj-$(CONFIG_AD7791) += ad7791.o
>  obj-$(CONFIG_AD7793) += ad7793.o
>  obj-$(CONFIG_AD7887) += ad7887.o
> diff --git a/drivers/iio/adc/ad7780.c b/drivers/iio/adc/ad7780.c
> new file mode 100644
> index 000000000000..aa1ddd0c45ee
> --- /dev/null
> +++ b/drivers/iio/adc/ad7780.c
> @@ -0,0 +1,365 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * AD7170/AD7171 and AD7780/AD7781 SPI ADC driver
> + *
> + * Copyright 2011 Analog Devices Inc.
> + * Copyright 2019 Renato Lui Geh
> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/sysfs.h>
> +#include <linux/spi/spi.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/err.h>
> +#include <linux/sched.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/bits.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/adc/ad_sigma_delta.h>
> +
> +#define AD7780_RDY		BIT(7)
> +#define AD7780_FILTER		BIT(6)
> +#define AD7780_ERR		BIT(5)
> +#define AD7780_GAIN		BIT(2)
> +
> +#define AD7170_ID		0
> +#define AD7171_ID		1
> +#define AD7780_ID		1
> +#define AD7781_ID		0
> +
> +#define AD7780_ID_MASK		(BIT(3) | BIT(4))
> +
> +#define AD7780_PATTERN_GOOD	1
> +#define AD7780_PATTERN_MASK	GENMASK(1, 0)
> +
> +#define AD7170_PATTERN_GOOD	5
> +#define AD7170_PATTERN_MASK	GENMASK(2, 0)
> +
> +#define AD7780_GAIN_MIDPOINT	64
> +#define AD7780_FILTER_MIDPOINT	13350
> +
> +static const unsigned int ad778x_gain[2]      = { 1, 128 };
> +static const unsigned int ad778x_odr_avail[2] = { 10000, 16700 };
> +
> +struct ad7780_chip_info {
> +	struct iio_chan_spec	channel;
> +	unsigned int		pattern_mask;
> +	unsigned int		pattern;
> +	bool			is_ad778x;
> +};
> +
> +struct ad7780_state {
> +	const struct ad7780_chip_info	*chip_info;
> +	struct regulator		*reg;
> +	struct gpio_desc		*powerdown_gpio;
> +	struct gpio_desc		*gain_gpio;
> +	struct gpio_desc		*filter_gpio;
> +	unsigned int			gain;
> +	unsigned int			odr;
> +	unsigned int			int_vref_mv;
> +
> +	struct ad_sigma_delta sd;
> +};
> +
> +enum ad7780_supported_device_ids {
> +	ID_AD7170,
> +	ID_AD7171,
> +	ID_AD7780,
> +	ID_AD7781,
> +};
> +
> +static struct ad7780_state *ad_sigma_delta_to_ad7780(struct ad_sigma_delta *sd)
> +{
> +	return container_of(sd, struct ad7780_state, sd);
> +}
> +
> +static int ad7780_set_mode(struct ad_sigma_delta *sigma_delta,
> +			   enum ad_sigma_delta_mode mode)
> +{
> +	struct ad7780_state *st = ad_sigma_delta_to_ad7780(sigma_delta);
> +	unsigned int val;
> +
> +	switch (mode) {
> +	case AD_SD_MODE_SINGLE:
> +	case AD_SD_MODE_CONTINUOUS:
> +		val = 1;
> +		break;
> +	default:
> +		val = 0;
> +		break;
> +	}
> +
> +	gpiod_set_value(st->powerdown_gpio, val);
> +
> +	return 0;
> +}
> +
> +static int ad7780_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan,
> +			   int *val,
> +			   int *val2,
> +			   long m)
> +{
> +	struct ad7780_state *st = iio_priv(indio_dev);
> +	int voltage_uv;
> +
> +	switch (m) {
> +	case IIO_CHAN_INFO_RAW:
> +		return ad_sigma_delta_single_conversion(indio_dev, chan, val);
> +	case IIO_CHAN_INFO_SCALE:
> +		voltage_uv = regulator_get_voltage(st->reg);
> +		if (voltage_uv < 0)
> +			return voltage_uv;
> +		voltage_uv /= 1000;
> +		*val = voltage_uv * st->gain;
> +		*val2 = chan->scan_type.realbits - 1;
> +		st->int_vref_mv = voltage_uv;
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +	case IIO_CHAN_INFO_OFFSET:
> +		*val = -(1 << (chan->scan_type.realbits - 1));
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		*val = st->odr;
> +		return IIO_VAL_INT;
> +	default:
> +	break;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int ad7780_write_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int val,
> +			    int val2,
> +			    long m)
> +{
> +	struct ad7780_state *st = iio_priv(indio_dev);
> +	const struct ad7780_chip_info *chip_info = st->chip_info;
> +	unsigned long long vref;
> +	unsigned int full_scale, gain;
> +
> +	if (!chip_info->is_ad778x)
> +		return 0;
> +
> +	switch (m) {
> +	case IIO_CHAN_INFO_SCALE:
> +		if (val != 0)
> +			return -EINVAL;
> +
> +		vref = st->int_vref_mv * 1000000LL;
> +		full_scale = 1 << (chip_info->channel.scan_type.realbits - 1);
> +		gain = DIV_ROUND_CLOSEST(vref, full_scale);
> +		gain = DIV_ROUND_CLOSEST(gain, val2);
> +		st->gain = gain;
> +		if (gain < AD7780_GAIN_MIDPOINT)
> +			gain = 0;
> +		else
> +			gain = 1;
> +		gpiod_set_value(st->gain_gpio, gain);
> +		break;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		if (1000*val + val2/1000 < AD7780_FILTER_MIDPOINT)
> +			val = 0;
> +		else
> +			val = 1;
> +		st->odr = ad778x_odr_avail[val];
> +		gpiod_set_value(st->filter_gpio, val);
> +		break;
> +	default:
> +	break;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ad7780_postprocess_sample(struct ad_sigma_delta *sigma_delta,
> +				     unsigned int raw_sample)
> +{
> +	struct ad7780_state *st = ad_sigma_delta_to_ad7780(sigma_delta);
> +	const struct ad7780_chip_info *chip_info = st->chip_info;
> +
> +	if ((raw_sample & AD7780_ERR) ||
> +	    ((raw_sample & chip_info->pattern_mask) != chip_info->pattern))
> +		return -EIO;
> +
> +	if (chip_info->is_ad778x) {
> +		st->gain = ad778x_gain[raw_sample & AD7780_GAIN];
> +		st->odr = ad778x_odr_avail[raw_sample & AD7780_FILTER];
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct ad_sigma_delta_info ad7780_sigma_delta_info = {
> +	.set_mode = ad7780_set_mode,
> +	.postprocess_sample = ad7780_postprocess_sample,
> +	.has_registers = false,
> +};
> +
> +#define AD7780_CHANNEL(bits, wordsize) \
> +	AD_SD_CHANNEL(1, 0, 0, bits, 32, wordsize - bits)
> +#define AD7170_CHANNEL(bits, wordsize) \
> +	AD_SD_CHANNEL_NO_SAMP_FREQ(1, 0, 0, bits, 32, wordsize - bits)
> +
> +static const struct ad7780_chip_info ad7780_chip_info_tbl[] = {
> +	[ID_AD7170] = {
> +		.channel = AD7170_CHANNEL(12, 24),
> +		.pattern = AD7170_PATTERN_GOOD,
> +		.pattern_mask = AD7170_PATTERN_MASK,
> +		.is_ad778x = false,
> +	},
> +	[ID_AD7171] = {
> +		.channel = AD7170_CHANNEL(16, 24),
> +		.pattern = AD7170_PATTERN_GOOD,
> +		.pattern_mask = AD7170_PATTERN_MASK,
> +		.is_ad778x = false,
> +	},
> +	[ID_AD7780] = {
> +		.channel = AD7780_CHANNEL(24, 32),
> +		.pattern = AD7780_PATTERN_GOOD,
> +		.pattern_mask = AD7780_PATTERN_MASK,
> +		.is_ad778x = true,
> +	},
> +	[ID_AD7781] = {
> +		.channel = AD7780_CHANNEL(20, 32),
> +		.pattern = AD7780_PATTERN_GOOD,
> +		.pattern_mask = AD7780_PATTERN_MASK,
> +		.is_ad778x = true,
> +	},
> +};
> +
> +static const struct iio_info ad7780_info = {
> +	.read_raw = ad7780_read_raw,
> +	.write_raw = ad7780_write_raw,
> +};
> +
> +static int ad7780_probe(struct spi_device *spi)
> +{
> +	struct ad7780_state *st;
> +	struct iio_dev *indio_dev;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +	st->gain = 1;
> +
> +	ad_sd_init(&st->sd, indio_dev, spi, &ad7780_sigma_delta_info);
> +
> +	st->chip_info =
> +		&ad7780_chip_info_tbl[spi_get_device_id(spi)->driver_data];
> +
> +	spi_set_drvdata(spi, indio_dev);
> +
> +	indio_dev->dev.parent = &spi->dev;
> +	indio_dev->name = spi_get_device_id(spi)->name;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = &st->chip_info->channel;
> +	indio_dev->num_channels = 1;
> +	indio_dev->info = &ad7780_info;
> +
> +	st->powerdown_gpio = devm_gpiod_get_optional(&spi->dev,
> +						     "powerdown",
> +						     GPIOD_OUT_LOW);
> +	if (IS_ERR(st->powerdown_gpio)) {
> +		ret = PTR_ERR(st->powerdown_gpio);
> +		dev_err(&spi->dev, "Failed to request powerdown GPIO: %d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	if (st->chip_info->is_ad778x) {
> +		st->gain_gpio = devm_gpiod_get_optional(&spi->dev,
> +							"adi,gain",
> +							GPIOD_OUT_HIGH);
> +		if (IS_ERR(st->gain_gpio)) {
> +			ret = PTR_ERR(st->gain_gpio);
> +			dev_err(&spi->dev, "Failed to request gain GPIO: %d\n",
> +				ret);
> +			return ret;
> +		}
> +
> +		st->filter_gpio = devm_gpiod_get_optional(&spi->dev,
> +							  "adi,filter",
> +							  GPIOD_OUT_HIGH);
> +		if (IS_ERR(st->filter_gpio)) {
> +			ret = PTR_ERR(st->filter_gpio);
> +			dev_err(&spi->dev,
> +				"Failed to request filter GPIO: %d\n",
> +				ret);
> +			return ret;
> +		}
> +	}
> +
> +	st->reg = devm_regulator_get(&spi->dev, "avdd");
> +	if (IS_ERR(st->reg))
> +		return PTR_ERR(st->reg);
> +
> +	ret = regulator_enable(st->reg);
> +	if (ret) {
> +		dev_err(&spi->dev, "Failed to enable specified AVdd supply\n");
> +		return ret;
> +	}
> +
> +	ret = ad_sd_setup_buffer_and_trigger(indio_dev);
> +	if (ret)
> +		goto error_disable_reg;
> +
> +	ret = iio_device_register(indio_dev);
> +	if (ret)
> +		goto error_cleanup_buffer_and_trigger;
> +
> +	return 0;
> +
> +error_cleanup_buffer_and_trigger:
> +	ad_sd_cleanup_buffer_and_trigger(indio_dev);
> +error_disable_reg:
> +	regulator_disable(st->reg);
> +
> +	return ret;
> +}
> +
> +static int ad7780_remove(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev = spi_get_drvdata(spi);
> +	struct ad7780_state *st = iio_priv(indio_dev);
> +
> +	iio_device_unregister(indio_dev);
> +	ad_sd_cleanup_buffer_and_trigger(indio_dev);
> +
> +	regulator_disable(st->reg);
> +
> +	return 0;
> +}
> +
> +static const struct spi_device_id ad7780_id[] = {
> +	{"ad7170", ID_AD7170},
> +	{"ad7171", ID_AD7171},
> +	{"ad7780", ID_AD7780},
> +	{"ad7781", ID_AD7781},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(spi, ad7780_id);
> +
> +static struct spi_driver ad7780_driver = {
> +	.driver = {
> +		.name	= "ad7780",
> +	},
> +	.probe		= ad7780_probe,
> +	.remove		= ad7780_remove,
> +	.id_table	= ad7780_id,
> +};
> +module_spi_driver(ad7780_driver);
> +
> +MODULE_AUTHOR("Michael Hennerich <michael.hennerich@analog.com>");
> +MODULE_DESCRIPTION("Analog Devices AD7780 and similar ADCs");
> +MODULE_LICENSE("GPL v2");

Re: [PATCH v4 0/9] staging: iio: ad7780: move out of staging

[Renato Lui Geh]

Hi Alexandru,

Thanks for the review. Some questions inline.

Thanks,
Renato

On 03/01, Ardelean, Alexandru wrote:
>On Thu, 2019-02-28 at 11:23 -0300, Renato Lui Geh wrote:
>>
>
>The patch-series is a bit big.
>I guess that the intent is to move this out-of-staging, but various patches
>are holding this in it's place.
>For patch series above a certain size, you could get many re-spins
>[V2,3,4... so on].
>
>You could send some of the changes as individual patches, or group them in
>series of 1,2 or 3 patches. That way, you "parallelize" patch sending, and
>when you get reviews on each patch, you can re-spin them individually.
>You'll find over time that certain patches get accepted on V1, others on V2
>and some on V7 [ hopefully, there isn't any frustration at that point ].

On these subseries, should versioning follow this patchset (v5) or should
they start anew (v1), ignoring this series version?
>
>Well, this is a technique I use to distribute some of my upstream-patch-
>work, so that I can switch easier between internal-work & upstreaming-work.
>
>Coming back to this patch-series.
>My general input, is that the patches are fine over-all; some are just
>cosmetics/noise/a-different-way-of-doing-things-for-this-driver, and those
>usually can be left to preference [of the maintainer usually].
>
>I do suggest to not hurry when re-spinning patches, and not change too much
>the number of patches in a new series. That can complicate things
>sometimes. But, if doing small patch-series or individual patches, you
>won't have this problem too much.
>
>Thanks
>Alex
>
>>
>> This series of patches contains the following:
>>  - Adds user input for the 'gain' and 'filter' GPIO pins for the ad778x
>>    family chips;
>>  - Filter reading for the ad778x;
>>  - Sets pattern macro values and mask for PATTERN status bits;
>>  - Adds ID values for the ad7170, ad7171, ad7780 and ad7781 for ID
>>    status bits checking;
>>  - Moves regulator initialization to after GPIO init to maintain
>>    consistency between probe and remove;
>>  - Copyright edits, adding SPDX identifier and new copyright holder;
>>  - Moves the ad7780 driver out of staging to the mainline;
>>  - Adds device tree binding for the ad7780 driver.
>>
>> Renato Lui Geh (9):
>>   staging: iio: ad7780: add gain & filter gpio support
>>   staging: iio: ad7780: add filter reading to ad778x
>>   staging: iio: ad7780: set pattern values and masks directly
>>   staging:iio:ad7780: add chip ID values and mask
>>   staging: iio: ad7780: move regulator to after GPIO init
>>   staging: iio: ad7780: add SPDX identifier
>>   staging: iio: ad7780: add new copyright holder
>>   staging: iio: ad7780: moving ad7780 out of staging
>>   staging: iio: ad7780: add device tree binding
>>
>> Changelog:
>> *v3
>>  - SPDX and regulator init as patches
>>  - Renamed filter to odr and ad778x_filter to ad778x_odr_avail
>>  - Removed unnecessary regulator disabling
>>  - Removed unnecessary AD_SD_CHANNEL macro
>>  - Changed unsigned int to unsigned long long to avoid overflow
>> *v4
>>  - Split gain & filter patch into two, with the new commit adding only
>>    filter reading
>>  - Changed pattern values to direct values, and added pattern mask
>>  - Added ID values and mask
>>  - Added new copyright holder
>>  - Added device tree binding to the ad7780 driver
>>
>>  .../bindings/iio/adc/adi,ad7780.txt           |  48 +++
>>  drivers/iio/adc/Kconfig                       |  12 +
>>  drivers/iio/adc/Makefile                      |   1 +
>>  drivers/iio/adc/ad7780.c                      | 365 ++++++++++++++++++
>>  drivers/staging/iio/adc/Kconfig               |  13 -
>>  drivers/staging/iio/adc/Makefile              |   1 -
>>  drivers/staging/iio/adc/ad7780.c              | 277 -------------
>>  7 files changed, 426 insertions(+), 291 deletions(-)
>>  create mode 100644
>> Documentation/devicetree/bindings/iio/adc/adi,ad7780.txt
>>  create mode 100644 drivers/iio/adc/ad7780.c
>>  delete mode 100644 drivers/staging/iio/adc/ad7780.c
>>
>> --
>> 2.21.0
>>
>
>-- 
>You received this message because you are subscribed to the Google Groups "Kernel USP" group.
>To unsubscribe from this group and stop receiving emails from it, send an email to kernel-usp+unsubscribe@googlegroups.com.
>To post to this group, send email to kernel-usp@googlegroups.com.
>To view this discussion on the web visit https://groups.google.com/d/msgid/kernel-usp/72a54cd5f58aeb9507b95b7e33ca3d9a38c853e9.camel%40analog.com.
>For more options, visit https://groups.google.com/d/optout.

Re: [PATCH v4 4/9] staging:iio:ad7780: add chip ID values and mask

[Renato Lui Geh]

On 03/01, Ardelean, Alexandru wrote:
>On Thu, 2019-02-28 at 11:24 -0300, Renato Lui Geh wrote:
>>
>>
>> The ad7780 supports both the ad778x and ad717x families. Each chip has
>> a corresponding ID. This patch provides a mask for extracting ID values
>> from the status bits and also macros for the correct values for the
>> ad7170, ad7171, ad7780 and ad7781.
>>
>> Signed-off-by: Renato Lui Geh <renatogeh@gmail.com>
>> ---
>>  drivers/staging/iio/adc/ad7780.c | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/staging/iio/adc/ad7780.c
>> b/drivers/staging/iio/adc/ad7780.c
>> index 56c49e28f432..ad7617a3a141 100644
>> --- a/drivers/staging/iio/adc/ad7780.c
>> +++ b/drivers/staging/iio/adc/ad7780.c
>> @@ -26,10 +26,14 @@
>>  #define AD7780_RDY             BIT(7)
>>  #define AD7780_FILTER          BIT(6)
>>  #define AD7780_ERR             BIT(5)
>> -#define AD7780_ID1             BIT(4)
>> -#define AD7780_ID0             BIT(3)
>>  #define AD7780_GAIN            BIT(2)
>>
>> +#define AD7170_ID              0
>> +#define AD7171_ID              1
>> +#define AD7780_ID              1
>> +#define AD7781_ID              0
>> +
>> +#define AD7780_ID_MASK         (BIT(3) | BIT(4))
>
>This also doesn't have any functionality change.
>The AD7170_ID, AD7171_ID, AD7780_ID & AD7781_ID IDs are also unused (maybe
>in a later patch they are ?).

They aren't. I added them following a previous review suggestion. Should
I remove them?
>
>I would also leave the AD7780_ID1 & AD7780_ID0 definitions in place, since
>they're easier matched with the datasheet.
>
>>
>>  #define AD7780_PATTERN_GOOD    1
>>  #define AD7780_PATTERN_MASK    GENMASK(1, 0)
>> --
>> 2.21.0
>>

Re: [PATCH v4 0/9] staging: iio: ad7780: move out of staging

[Alexandru Ardelean]

On Sun, Mar 3, 2019 at 3:52 PM Renato Lui Geh <renatogeh@gmail.com> wrote:
>
> Hi Alexandru,
>
> Thanks for the review. Some questions inline.
>
> Thanks,
> Renato
>
> On 03/01, Ardelean, Alexandru wrote:
> >On Thu, 2019-02-28 at 11:23 -0300, Renato Lui Geh wrote:
> >>
> >
> >The patch-series is a bit big.
> >I guess that the intent is to move this out-of-staging, but various patches
> >are holding this in it's place.
> >For patch series above a certain size, you could get many re-spins
> >[V2,3,4... so on].
> >
> >You could send some of the changes as individual patches, or group them in
> >series of 1,2 or 3 patches. That way, you "parallelize" patch sending, and
> >when you get reviews on each patch, you can re-spin them individually.
> >You'll find over time that certain patches get accepted on V1, others on V2
> >and some on V7 [ hopefully, there isn't any frustration at that point ].
>
> On these subseries, should versioning follow this patchset (v5) or should
> they start anew (v1), ignoring this series version?

I guess, in this case it's fine to leave it as is [in this series].
The series has been reviewed now.

But [for me typically], I delay doing a review if a patch-series is
longer than 4-5 patches. And I think some reviewers may do the same.
So, if I want more people to review/look at my code, I try to make
things as easy to review, as possible.
And one way, is to definitely keep things decoupled.
If one patch can be independent of another [for the same driver/code],
I send them as separate patches.

This [of course], is a preference. Some reviewers don't mind longer
series [than 4-5 patches].

> >
> >Well, this is a technique I use to distribute some of my upstream-patch-
> >work, so that I can switch easier between internal-work & upstreaming-work.
> >
> >Coming back to this patch-series.
> >My general input, is that the patches are fine over-all; some are just
> >cosmetics/noise/a-different-way-of-doing-things-for-this-driver, and those
> >usually can be left to preference [of the maintainer usually].
> >
> >I do suggest to not hurry when re-spinning patches, and not change too much
> >the number of patches in a new series. That can complicate things
> >sometimes. But, if doing small patch-series or individual patches, you
> >won't have this problem too much.
> >
> >Thanks
> >Alex
> >
> >>
> >> This series of patches contains the following:
> >>  - Adds user input for the 'gain' and 'filter' GPIO pins for the ad778x
> >>    family chips;
> >>  - Filter reading for the ad778x;
> >>  - Sets pattern macro values and mask for PATTERN status bits;
> >>  - Adds ID values for the ad7170, ad7171, ad7780 and ad7781 for ID
> >>    status bits checking;
> >>  - Moves regulator initialization to after GPIO init to maintain
> >>    consistency between probe and remove;
> >>  - Copyright edits, adding SPDX identifier and new copyright holder;
> >>  - Moves the ad7780 driver out of staging to the mainline;
> >>  - Adds device tree binding for the ad7780 driver.
> >>
> >> Renato Lui Geh (9):
> >>   staging: iio: ad7780: add gain & filter gpio support
> >>   staging: iio: ad7780: add filter reading to ad778x
> >>   staging: iio: ad7780: set pattern values and masks directly
> >>   staging:iio:ad7780: add chip ID values and mask
> >>   staging: iio: ad7780: move regulator to after GPIO init
> >>   staging: iio: ad7780: add SPDX identifier
> >>   staging: iio: ad7780: add new copyright holder
> >>   staging: iio: ad7780: moving ad7780 out of staging
> >>   staging: iio: ad7780: add device tree binding
> >>
> >> Changelog:
> >> *v3
> >>  - SPDX and regulator init as patches
> >>  - Renamed filter to odr and ad778x_filter to ad778x_odr_avail
> >>  - Removed unnecessary regulator disabling
> >>  - Removed unnecessary AD_SD_CHANNEL macro
> >>  - Changed unsigned int to unsigned long long to avoid overflow
> >> *v4
> >>  - Split gain & filter patch into two, with the new commit adding only
> >>    filter reading
> >>  - Changed pattern values to direct values, and added pattern mask
> >>  - Added ID values and mask
> >>  - Added new copyright holder
> >>  - Added device tree binding to the ad7780 driver
> >>
> >>  .../bindings/iio/adc/adi,ad7780.txt           |  48 +++
> >>  drivers/iio/adc/Kconfig                       |  12 +
> >>  drivers/iio/adc/Makefile                      |   1 +
> >>  drivers/iio/adc/ad7780.c                      | 365 ++++++++++++++++++
> >>  drivers/staging/iio/adc/Kconfig               |  13 -
> >>  drivers/staging/iio/adc/Makefile              |   1 -
> >>  drivers/staging/iio/adc/ad7780.c              | 277 -------------
> >>  7 files changed, 426 insertions(+), 291 deletions(-)
> >>  create mode 100644
> >> Documentation/devicetree/bindings/iio/adc/adi,ad7780.txt
> >>  create mode 100644 drivers/iio/adc/ad7780.c
> >>  delete mode 100644 drivers/staging/iio/adc/ad7780.c
> >>
> >> --
> >> 2.21.0
> >>
> >
> >--
> >You received this message because you are subscribed to the Google Groups "Kernel USP" group.
> >To unsubscribe from this group and stop receiving emails from it, send an email to kernel-usp+unsubscribe@googlegroups.com.
> >To post to this group, send email to kernel-usp@googlegroups.com.
> >To view this discussion on the web visit https://groups.google.com/d/msgid/kernel-usp/72a54cd5f58aeb9507b95b7e33ca3d9a38c853e9.camel%40analog.com.
> >For more options, visit https://groups.google.com/d/optout.

Re: [PATCH v4 4/9] staging:iio:ad7780: add chip ID values and mask

[Jonathan Cameron]

On Sun, 3 Mar 2019 11:01:09 -0300
Renato Lui Geh <renatogeh@gmail.com> wrote:

> On 03/01, Ardelean, Alexandru wrote:
> >On Thu, 2019-02-28 at 11:24 -0300, Renato Lui Geh wrote:  
> >>
> >>
> >> The ad7780 supports both the ad778x and ad717x families. Each chip has
> >> a corresponding ID. This patch provides a mask for extracting ID values
> >> from the status bits and also macros for the correct values for the
> >> ad7170, ad7171, ad7780 and ad7781.
> >>
> >> Signed-off-by: Renato Lui Geh <renatogeh@gmail.com>
> >> ---
> >>  drivers/staging/iio/adc/ad7780.c | 8 ++++++--
> >>  1 file changed, 6 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/staging/iio/adc/ad7780.c
> >> b/drivers/staging/iio/adc/ad7780.c
> >> index 56c49e28f432..ad7617a3a141 100644
> >> --- a/drivers/staging/iio/adc/ad7780.c
> >> +++ b/drivers/staging/iio/adc/ad7780.c
> >> @@ -26,10 +26,14 @@
> >>  #define AD7780_RDY             BIT(7)
> >>  #define AD7780_FILTER          BIT(6)
> >>  #define AD7780_ERR             BIT(5)
> >> -#define AD7780_ID1             BIT(4)
> >> -#define AD7780_ID0             BIT(3)
> >>  #define AD7780_GAIN            BIT(2)
> >>
> >> +#define AD7170_ID              0
> >> +#define AD7171_ID              1
> >> +#define AD7780_ID              1
> >> +#define AD7781_ID              0
> >> +
> >> +#define AD7780_ID_MASK         (BIT(3) | BIT(4))  
> >
> >This also doesn't have any functionality change.
> >The AD7170_ID, AD7171_ID, AD7780_ID & AD7781_ID IDs are also unused (maybe
> >in a later patch they are ?).  
> 
> They aren't. I added them following a previous review suggestion. Should
> I remove them?

Can we check them?  It's always useful to confirm that the device is
the one you think it is.  Then we can either use what is there
with a suitable warning, or if that is tricky just fault out as the
dt is giving us the wrong part number.

J

> >
> >I would also leave the AD7780_ID1 & AD7780_ID0 definitions in place, since
> >they're easier matched with the datasheet.
> >  
> >>
> >>  #define AD7780_PATTERN_GOOD    1
> >>  #define AD7780_PATTERN_MASK    GENMASK(1, 0)
> >> --
> >> 2.21.0
> >>  

Re: [PATCH v4 3/9] staging: iio: ad7780: set pattern values and masks directly

[Ardelean, Alexandru]

On Sat, 2019-03-02 at 19:08 +0000, Jonathan Cameron wrote:
> [External]
> 
> 
> On Sat, 2 Mar 2019 19:07:16 +0000
> Jonathan Cameron <jic23@kernel.org> wrote:
> 
> > On Fri, 1 Mar 2019 07:17:04 +0000
> > "Ardelean, Alexandru" <Alex.Ardelean@analog.com> wrote:
> > 
> > > On Thu, 2019-02-28 at 11:24 -0300, Renato Lui Geh wrote:
> > > > 
> > > > 
> > > > The AD7780 driver contains status pattern bits designed for
> > > > checking
> > > > whether serial transfers have been correctly performed. Pattern
> > > > macros
> > > > were previously generated through bit fields. This patch sets good
> > > > pattern values directly and masks through GENMASK.
> > > > 
> > > > Signed-off-by: Renato Lui Geh <renatogeh@gmail.com>
> > > > ---
> > > >  drivers/staging/iio/adc/ad7780.c | 20 +++++++++-----------
> > > >  1 file changed, 9 insertions(+), 11 deletions(-)
> > > > 
> > > > diff --git a/drivers/staging/iio/adc/ad7780.c
> > > > b/drivers/staging/iio/adc/ad7780.c
> > > > index 7a68e90ddf14..56c49e28f432 100644
> > > > --- a/drivers/staging/iio/adc/ad7780.c
> > > > +++ b/drivers/staging/iio/adc/ad7780.c
> > > > @@ -17,6 +17,7 @@
> > > >  #include <linux/sched.h>
> > > >  #include <linux/gpio/consumer.h>
> > > >  #include <linux/module.h>
> > > > +#include <linux/bits.h>
> > > > 
> > > >  #include <linux/iio/iio.h>
> > > >  #include <linux/iio/sysfs.h>
> > > > @@ -28,16 +29,13 @@
> > > >  #define AD7780_ID1             BIT(4)
> > > >  #define AD7780_ID0             BIT(3)
> > > >  #define AD7780_GAIN            BIT(2)
> > > > -#define AD7780_PAT1            BIT(1)
> > > > -#define AD7780_PAT0            BIT(0)
> > > 
> > > I don't see a problem to leave the bitfields;  they can be read &
> > > matched
> > > easier with the datasheet.
> > > 
> > > > 
> > > > -#define AD7780_PATTERN         (AD7780_PAT0)
> > > > -#define AD7780_PATTERN_MASK    (AD7780_PAT0 | AD7780_PAT1)
> > > > 
> > > > -#define AD7170_PAT2            BIT(2)
> > > > +#define AD7780_PATTERN_GOOD    1
> > > 
> > > It was also nice before that the PAT0..PAT2 bitfields were used to
> > > define a
> > > good pattern, since it's easier to match with the datasheet.
> > 
> > This one was much suggestion.  Not particularly important one.
> 
> Not enough sleep this week clearly :)
> This one was _my_ suggestion.

Ok.
I assumed a bit I missed a discussion somewhere.

Let's have this as-is here.
I don't mind it much either.
My [personal] feeling is that [in the context of a move-out-of-staging-of-
this-driver] this patch is a bit of noise.

If the series were more tidy-up, then I probably would not have replied.

> 
> > 
> > Personally if a datasheet is pointlessly confusing I tend to ignore it.
> > This is a two bit field as the bits don't have independent meaning!
> > 
> > I'm not strongly tied to it though and as it's an Analog driver and
> > you all do a good job maintaining the set I'll go with your preference!
> > I do prefer the naming of PATTERN_GOOD though if nothing else!
> > > 
> > > 
> > > > +#define AD7780_PATTERN_MASK    GENMASK(1, 0)
> > > 
> > > I like the general usage of GENMASK, but I'm not sure in this case
> > > it's
> > > worth doing. Maybe I missed a discussion somewhere, about doing this
> > > change, but it is mostly a cosmetic without any functional change.
> > > 
> > > 
> > > > 
> > > > -#define AD7170_PATTERN         (AD7780_PAT0 | AD7170_PAT2)
> > > > -#define AD7170_PATTERN_MASK    (AD7780_PAT0 | AD7780_PAT1 |
> > > > AD7170_PAT2)
> > > > +#define AD7170_PATTERN_GOOD    5
> > > > +#define AD7170_PATTERN_MASK    GENMASK(2, 0)
> > > > 
> > > >  #define AD7780_GAIN_MIDPOINT   64
> > > >  #define AD7780_FILTER_MIDPOINT 13350
> > > > @@ -209,25 +207,25 @@ static const struct ad_sigma_delta_info
> > > > ad7780_sigma_delta_info = {
> > > >  static const struct ad7780_chip_info ad7780_chip_info_tbl[] = {
> > > >         [ID_AD7170] = {
> > > >                 .channel = AD7170_CHANNEL(12, 24),
> > > > -               .pattern = AD7170_PATTERN,
> > > > +               .pattern = AD7170_PATTERN_GOOD,
> > > >                 .pattern_mask = AD7170_PATTERN_MASK,
> > > >                 .is_ad778x = false,
> > > >         },
> > > >         [ID_AD7171] = {
> > > >                 .channel = AD7170_CHANNEL(16, 24),
> > > > -               .pattern = AD7170_PATTERN,
> > > > +               .pattern = AD7170_PATTERN_GOOD,
> > > >                 .pattern_mask = AD7170_PATTERN_MASK,
> > > >                 .is_ad778x = false,
> > > >         },
> > > >         [ID_AD7780] = {
> > > >                 .channel = AD7780_CHANNEL(24, 32),
> > > > -               .pattern = AD7780_PATTERN,
> > > > +               .pattern = AD7780_PATTERN_GOOD,
> > > >                 .pattern_mask = AD7780_PATTERN_MASK,
> > > >                 .is_ad778x = true,
> > > >         },
> > > >         [ID_AD7781] = {
> > > >                 .channel = AD7780_CHANNEL(20, 32),
> > > > -               .pattern = AD7780_PATTERN,
> > > > +               .pattern = AD7780_PATTERN_GOOD,
> > > >                 .pattern_mask = AD7780_PATTERN_MASK,
> > > >                 .is_ad778x = true,
> > > >         },
> > > > --
> > > > 2.21.0
> > > > 
> 
> 

Re: [PATCH v4 5/9] staging: iio: ad7780: move regulator to after GPIO init

[Ardelean, Alexandru]

On Sat, 2019-03-02 at 19:11 +0000, Jonathan Cameron wrote:
> [External]
> 
> 
> On Fri, 1 Mar 2019 07:38:45 +0000
> "Ardelean, Alexandru" <Alex.Ardelean@analog.com> wrote:
> 
> > On Thu, 2019-02-28 at 11:25 -0300, Renato Lui Geh wrote:
> > > 
> > > 
> > > To maintain consistency between ad7780_probe and ad7780_remove
> > > orders,
> > > regulator initialization has been moved to after GPIO
> > > initializations.
> > > 
> > > Signed-off-by: Renato Lui Geh <renatogeh@gmail.com>
> > > ---
> > >  drivers/staging/iio/adc/ad7780.c | 26 +++++++++++++-------------
> > >  1 file changed, 13 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/drivers/staging/iio/adc/ad7780.c
> > > b/drivers/staging/iio/adc/ad7780.c
> > > index ad7617a3a141..12aef0f101bc 100644
> > > --- a/drivers/staging/iio/adc/ad7780.c
> > > +++ b/drivers/staging/iio/adc/ad7780.c
> > > @@ -255,16 +255,6 @@ static int ad7780_probe(struct spi_device *spi)
> > > 
> > >         ad_sd_init(&st->sd, indio_dev, spi,
> > > &ad7780_sigma_delta_info);
> > > 
> > > -       st->reg = devm_regulator_get(&spi->dev, "avdd");
> > > -       if (IS_ERR(st->reg))
> > > -               return PTR_ERR(st->reg);
> > > -
> > > -       ret = regulator_enable(st->reg);
> > > -       if (ret) {
> > > -               dev_err(&spi->dev, "Failed to enable specified AVdd
> > > supply\n");
> > > -               return ret;
> > > -       }
> > > -
> > >         st->chip_info =
> > >                 &ad7780_chip_info_tbl[spi_get_device_id(spi)-
> > > > driver_data];
> > > 
> > > @@ -284,7 +274,7 @@ static int ad7780_probe(struct spi_device *spi)
> > >                 ret = PTR_ERR(st->powerdown_gpio);
> > >                 dev_err(&spi->dev, "Failed to request powerdown GPIO:
> > > %d\n",
> > >                         ret);
> > > -               goto error_disable_reg;
> > > +               return ret;
> > >         }
> > > 
> > >         if (st->chip_info->is_ad778x) {
> > > @@ -295,7 +285,7 @@ static int ad7780_probe(struct spi_device *spi)
> > >                         ret = PTR_ERR(st->gain_gpio);
> > >                         dev_err(&spi->dev, "Failed to request gain
> > > GPIO:
> > > %d\n",
> > >                                 ret);
> > > -                       goto error_disable_reg;
> > > +                       return ret;
> > >                 }
> > > 
> > >                 st->filter_gpio = devm_gpiod_get_optional(&spi->dev,
> > > @@ -306,10 +296,20 @@ static int ad7780_probe(struct spi_device *spi)
> > >                         dev_err(&spi->dev,
> > >                                 "Failed to request filter GPIO:
> > > %d\n",
> > >                                 ret);
> > > -                       goto error_disable_reg;
> > > +                       return ret;
> > >                 }
> > >         }
> > > 
> > > +       st->reg = devm_regulator_get(&spi->dev, "avdd");
> > > +       if (IS_ERR(st->reg))
> > > +               return PTR_ERR(st->reg);
> > > +
> > > +       ret = regulator_enable(st->reg);
> > > +       if (ret) {
> > > +               dev_err(&spi->dev, "Failed to enable specified AVdd
> > > supply\n");
> > > +               return ret;
> > > +       }
> > > +
> > 
> > I'm probably missing something, but other than the fact that this moves
> > the
> > regulator init after the GPIOs init, it doesn't change much.
> > The order of the probe & remove is more-or-less the same.
> > The GPIOs will be free'd via devm_ API/stuff.
> 
> This is another one from me.  I'm a fanatic at times when it comes
> to probe and remove orders being precise reverses.  It just makes
> review easier.  Nice to not actually have to think.
> 
> So I agree there is no 'actual' effect but it is in my view
> still worth doing.

Ack.
Let's leave it

> 
> Jonathan
> 
> > 
> > 
> > >         ret = ad_sd_setup_buffer_and_trigger(indio_dev);
> > >         if (ret)
> > >                 goto error_disable_reg;
> > > --
> > > 2.21.0
> > > 
> 
> 

Re: [PATCH v4 4/9] staging:iio:ad7780: add chip ID values and mask

[Ardelean, Alexandru]

On Sun, 2019-03-03 at 14:53 +0000, Jonathan Cameron wrote:
> [External]
> 
> 
> On Sun, 3 Mar 2019 11:01:09 -0300
> Renato Lui Geh <renatogeh@gmail.com> wrote:
> 
> > On 03/01, Ardelean, Alexandru wrote:
> > > On Thu, 2019-02-28 at 11:24 -0300, Renato Lui Geh wrote:
> > > > 
> > > > 
> > > > The ad7780 supports both the ad778x and ad717x families. Each chip
> > > > has
> > > > a corresponding ID. This patch provides a mask for extracting ID
> > > > values
> > > > from the status bits and also macros for the correct values for the
> > > > ad7170, ad7171, ad7780 and ad7781.
> > > > 
> > > > Signed-off-by: Renato Lui Geh <renatogeh@gmail.com>
> > > > ---
> > > >  drivers/staging/iio/adc/ad7780.c | 8 ++++++--
> > > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/staging/iio/adc/ad7780.c
> > > > b/drivers/staging/iio/adc/ad7780.c
> > > > index 56c49e28f432..ad7617a3a141 100644
> > > > --- a/drivers/staging/iio/adc/ad7780.c
> > > > +++ b/drivers/staging/iio/adc/ad7780.c
> > > > @@ -26,10 +26,14 @@
> > > >  #define AD7780_RDY             BIT(7)
> > > >  #define AD7780_FILTER          BIT(6)
> > > >  #define AD7780_ERR             BIT(5)
> > > > -#define AD7780_ID1             BIT(4)
> > > > -#define AD7780_ID0             BIT(3)
> > > >  #define AD7780_GAIN            BIT(2)
> > > > 
> > > > +#define AD7170_ID              0
> > > > +#define AD7171_ID              1
> > > > +#define AD7780_ID              1
> > > > +#define AD7781_ID              0
> > > > +
> > > > +#define AD7780_ID_MASK         (BIT(3) | BIT(4))
> > > 
> > > This also doesn't have any functionality change.
> > > The AD7170_ID, AD7171_ID, AD7780_ID & AD7781_ID IDs are also unused
> > > (maybe
> > > in a later patch they are ?).
> > 
> > They aren't. I added them following a previous review suggestion.
> > Should
> > I remove them?
> 
> Can we check them?  It's always useful to confirm that the device is
> the one you think it is.  Then we can either use what is there
> with a suitable warning, or if that is tricky just fault out as the
> dt is giving us the wrong part number.
> 
> J

I guess `dev_warn_ratelimited()` could be used to make sure syslog isn't
spammed-to-death when doing multiple conversions, and the ID isn't correct.
Since these IDs are read after you get a sample, I'm a bit fearful of log-
spam.

I wouldn't throw an error in the ad7780_postprocess_sample() for this, but
warning [with rate-limit] sounds reasonable.

> 
> > > 
> > > I would also leave the AD7780_ID1 & AD7780_ID0 definitions in place,
> > > since
> > > they're easier matched with the datasheet.
> > > 
> > > > 
> > > >  #define AD7780_PATTERN_GOOD    1
> > > >  #define AD7780_PATTERN_MASK    GENMASK(1, 0)
> > > > --
> > > > 2.21.0
> > > > 
> 
> 

Re: [PATCH v4 4/9] staging:iio:ad7780: add chip ID values and mask

[Renato Lui Geh]

On 03/04, Ardelean, Alexandru wrote:
>On Sun, 2019-03-03 at 14:53 +0000, Jonathan Cameron wrote:
>> [External]
>>
>>
>> On Sun, 3 Mar 2019 11:01:09 -0300
>> Renato Lui Geh <renatogeh@gmail.com> wrote:
>>
>> > On 03/01, Ardelean, Alexandru wrote:
>> > > On Thu, 2019-02-28 at 11:24 -0300, Renato Lui Geh wrote:
>> > > >
>> > > >
>> > > > The ad7780 supports both the ad778x and ad717x families. Each chip
>> > > > has
>> > > > a corresponding ID. This patch provides a mask for extracting ID
>> > > > values
>> > > > from the status bits and also macros for the correct values for the
>> > > > ad7170, ad7171, ad7780 and ad7781.
>> > > >
>> > > > Signed-off-by: Renato Lui Geh <renatogeh@gmail.com>
>> > > > ---
>> > > >  drivers/staging/iio/adc/ad7780.c | 8 ++++++--
>> > > >  1 file changed, 6 insertions(+), 2 deletions(-)
>> > > >
>> > > > diff --git a/drivers/staging/iio/adc/ad7780.c
>> > > > b/drivers/staging/iio/adc/ad7780.c
>> > > > index 56c49e28f432..ad7617a3a141 100644
>> > > > --- a/drivers/staging/iio/adc/ad7780.c
>> > > > +++ b/drivers/staging/iio/adc/ad7780.c
>> > > > @@ -26,10 +26,14 @@
>> > > >  #define AD7780_RDY             BIT(7)
>> > > >  #define AD7780_FILTER          BIT(6)
>> > > >  #define AD7780_ERR             BIT(5)
>> > > > -#define AD7780_ID1             BIT(4)
>> > > > -#define AD7780_ID0             BIT(3)
>> > > >  #define AD7780_GAIN            BIT(2)
>> > > >
>> > > > +#define AD7170_ID              0
>> > > > +#define AD7171_ID              1
>> > > > +#define AD7780_ID              1
>> > > > +#define AD7781_ID              0
>> > > > +
>> > > > +#define AD7780_ID_MASK         (BIT(3) | BIT(4))
>> > >
>> > > This also doesn't have any functionality change.
>> > > The AD7170_ID, AD7171_ID, AD7780_ID & AD7781_ID IDs are also unused
>> > > (maybe
>> > > in a later patch they are ?).
>> >
>> > They aren't. I added them following a previous review suggestion.
>> > Should
>> > I remove them?
>>
>> Can we check them?  It's always useful to confirm that the device is
>> the one you think it is.  Then we can either use what is there
>> with a suitable warning, or if that is tricky just fault out as the
>> dt is giving us the wrong part number.
>>
>> J
>
>I guess `dev_warn_ratelimited()` could be used to make sure syslog isn't
>spammed-to-death when doing multiple conversions, and the ID isn't correct.
>Since these IDs are read after you get a sample, I'm a bit fearful of log-
>spam.
>
>I wouldn't throw an error in the ad7780_postprocess_sample() for this, but
>warning [with rate-limit] sounds reasonable.

Looking at dev_warn_ratelimited's definition (and its use in other parts
of the kernel), I see that we'd need the device to be stored somewhere
(perhaps in ad7780_state?) in order for us to pass it as argument to
dev_warn from within postprocess_sample. Should this be stored in
ad7780_state?  Or can I get the spi_device some other way?
>
>>
>> > >
>> > > I would also leave the AD7780_ID1 & AD7780_ID0 definitions in place,
>> > > since
>> > > they're easier matched with the datasheet.
>> > >
>> > > >
>> > > >  #define AD7780_PATTERN_GOOD    1
>> > > >  #define AD7780_PATTERN_MASK    GENMASK(1, 0)
>> > > > --
>> > > > 2.21.0
>> > > >
>>
>>

Re: [PATCH v4 4/9] staging:iio:ad7780: add chip ID values and mask

[Jonathan Cameron]

On Fri, 8 Mar 2019 21:19:29 -0300
Renato Lui Geh <renatogeh@gmail.com> wrote:

> On 03/04, Ardelean, Alexandru wrote:
> >On Sun, 2019-03-03 at 14:53 +0000, Jonathan Cameron wrote:  
> >> [External]
> >>
> >>
> >> On Sun, 3 Mar 2019 11:01:09 -0300
> >> Renato Lui Geh <renatogeh@gmail.com> wrote:
> >>  
> >> > On 03/01, Ardelean, Alexandru wrote:  
> >> > > On Thu, 2019-02-28 at 11:24 -0300, Renato Lui Geh wrote:  
> >> > > >
> >> > > >
> >> > > > The ad7780 supports both the ad778x and ad717x families. Each chip
> >> > > > has
> >> > > > a corresponding ID. This patch provides a mask for extracting ID
> >> > > > values
> >> > > > from the status bits and also macros for the correct values for the
> >> > > > ad7170, ad7171, ad7780 and ad7781.
> >> > > >
> >> > > > Signed-off-by: Renato Lui Geh <renatogeh@gmail.com>
> >> > > > ---
> >> > > >  drivers/staging/iio/adc/ad7780.c | 8 ++++++--
> >> > > >  1 file changed, 6 insertions(+), 2 deletions(-)
> >> > > >
> >> > > > diff --git a/drivers/staging/iio/adc/ad7780.c
> >> > > > b/drivers/staging/iio/adc/ad7780.c
> >> > > > index 56c49e28f432..ad7617a3a141 100644
> >> > > > --- a/drivers/staging/iio/adc/ad7780.c
> >> > > > +++ b/drivers/staging/iio/adc/ad7780.c
> >> > > > @@ -26,10 +26,14 @@
> >> > > >  #define AD7780_RDY             BIT(7)
> >> > > >  #define AD7780_FILTER          BIT(6)
> >> > > >  #define AD7780_ERR             BIT(5)
> >> > > > -#define AD7780_ID1             BIT(4)
> >> > > > -#define AD7780_ID0             BIT(3)
> >> > > >  #define AD7780_GAIN            BIT(2)
> >> > > >
> >> > > > +#define AD7170_ID              0
> >> > > > +#define AD7171_ID              1
> >> > > > +#define AD7780_ID              1
> >> > > > +#define AD7781_ID              0
> >> > > > +
> >> > > > +#define AD7780_ID_MASK         (BIT(3) | BIT(4))  
> >> > >
> >> > > This also doesn't have any functionality change.
> >> > > The AD7170_ID, AD7171_ID, AD7780_ID & AD7781_ID IDs are also unused
> >> > > (maybe
> >> > > in a later patch they are ?).  
> >> >
> >> > They aren't. I added them following a previous review suggestion.
> >> > Should
> >> > I remove them?  
> >>
> >> Can we check them?  It's always useful to confirm that the device is
> >> the one you think it is.  Then we can either use what is there
> >> with a suitable warning, or if that is tricky just fault out as the
> >> dt is giving us the wrong part number.
> >>
> >> J  
> >
> >I guess `dev_warn_ratelimited()` could be used to make sure syslog isn't
> >spammed-to-death when doing multiple conversions, and the ID isn't correct.
> >Since these IDs are read after you get a sample, I'm a bit fearful of log-
> >spam.
> >
> >I wouldn't throw an error in the ad7780_postprocess_sample() for this, but
> >warning [with rate-limit] sounds reasonable.  
> 
> Looking at dev_warn_ratelimited's definition (and its use in other parts
> of the kernel), I see that we'd need the device to be stored somewhere
> (perhaps in ad7780_state?) in order for us to pass it as argument to
> dev_warn from within postprocess_sample. Should this be stored in
> ad7780_state?  Or can I get the spi_device some other way?
Ah. I was being lazy and hadn't looked at the datasheet.

If they are that nasty to get to, don't bother checking them.
Mostly we can assume people don't claim to have the wrong
compatible device.

Jonathan

> >  
> >>  
> >> > >
> >> > > I would also leave the AD7780_ID1 & AD7780_ID0 definitions in place,
> >> > > since
> >> > > they're easier matched with the datasheet.
> >> > >  
> >> > > >
> >> > > >  #define AD7780_PATTERN_GOOD    1
> >> > > >  #define AD7780_PATTERN_MASK    GENMASK(1, 0)
> >> > > > --
> >> > > > 2.21.0
> >> > > >  
> >>
> >>  

Re: [PATCH v4 9/9] staging: iio: ad7780: add device tree binding

[Rob Herring]

On Thu, 28 Feb 2019 11:26:28 -0300, Renato Lui Geh wrote:
> Adds a device tree binding for the ad7780 driver.
> 
> Signed-off-by: Renato Lui Geh <renatogeh@gmail.com>
> ---
>  .../bindings/iio/adc/adi,ad7780.txt           | 48 +++++++++++++++++++
>  1 file changed, 48 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad7780.txt
> 

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