Skip to contentSkip to conversation

staging: iio: ad7780: move out of staging

PATCH Superseded Renato Lui Geh 5 participants 15 messages Feb 5, 2019 – Feb 18, 2019

Subsystems (5)

v3 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Renato Lui Geh

author maintainer
#0 lore raw

v3 cover staging: iio: ad7780: move out of staging

This series of patches adds user input to ad7780 'gain' & 'filter' gpio pins, moves regulator initialization to after gpio initialization to maintain consistency between probe and remove, adds SPDX to the driver's license, and moves the ad7780 to the mainline.

Renato Lui Geh (4):
staging: iio: ad7780: add gain & filter gpio support
staging: iio: ad7780: move regulator to after GPIO init
staging: iio: ad7780: add SPDX identifier
staging: iio: ad7780: moving ad7780 out of staging

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

drivers/iio/adc/Kconfig          |  13 ++
drivers/iio/adc/Makefile         |   1 +
drivers/iio/adc/ad7780.c         | 359 +++++++++++++++++++++++++++++++
drivers/staging/iio/adc/Kconfig  |  13 --
drivers/staging/iio/adc/Makefile |   1 -
drivers/staging/iio/adc/ad7780.c | 277 ------------------------
6 files changed, 373 insertions(+), 291 deletions(-)

create mode 100644 drivers/iio/adc/ad7780.c delete mode 100644 drivers/staging/iio/adc/ad7780.c

Signature · 1 line
2.20.1
author maintainer
#1 lore raw

PATCH v3 1/4 staging: iio: ad7780: add gain & filter gpio support

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

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

drivers/staging/iio/adc/ad7780.c | 95 ++++++++++++++++++++++++++++++--

1 file changed, 89 insertions(+), 6 deletions(-)
drivers/staging/iio/adc/ad7780.c +0 -0
index c4a85789c2db..6e4357800d31 100644
@@ -39,6 +39,15 @@

#define AD7170_PATTERN (AD7780_PAT0 | AD7170_PAT2)
#define AD7170_PATTERN_MASK (AD7780_PAT0 | AD7780_PAT1 | AD7170_PAT2)

+#define AD7780_GAIN_GPIO 0
+#define AD7780_FILTER_GPIO 1
+
+#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 +59,11 @@ 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 odr;
+ unsigned int int_vref_mv;

	struct ad_sigma_delta sd;
};
@@ -104,17 +117,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;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*val = st->odr;
+		return IIO_VAL_INT;
	}

	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;
+ }
+
+ return 0;
+}
+
static int ad7780_postprocess_sample(struct ad_sigma_delta *sigma_delta,
unsigned int raw_sample)
{
@@ -126,10 +187,8 @@ 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];

+		st->odr = ad778x_odr_avail[raw_sample & AD7780_FILTER];
	}

	return 0;

@@ -173,6 +232,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 +282,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,
+ "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,
+ "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;
Signature · 1 line
2.20.1
#2 lore raw

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

Quoted 2 lines · Renato Lui Geh

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

comments below

Quoted 52 lines

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

drivers/staging/iio/adc/ad7780.c | 95 ++++++++++++++++++++++++++++++--

1 file changed, 89 insertions(+), 6 deletions(-)
drivers/staging/iio/adc/ad7780.c +14 -1
index c4a85789c2db..6e4357800d31 100644
@@ -39,6 +39,15 @@
 #define AD7170_PATTERN		(AD7780_PAT0 | AD7170_PAT2)
 #define AD7170_PATTERN_MASK	(AD7780_PAT0 | AD7780_PAT1 | AD7170_PAT2)
 
+#define AD7780_GAIN_GPIO	0
+#define AD7780_FILTER_GPIO	1
+
+#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 +59,11 @@ 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			odr;
+	unsigned int			int_vref_mv;
 
 	struct ad_sigma_delta sd;
 };
@@ -104,17 +117,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;

drivers/staging/iio/adc/ad7780.c static int ad7780_read_raw(struct iio_dev *indio_dev, · line 128

Quoted 10 lines
drivers/staging/iio/adc/ad7780.c +4 -1
-		*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;
+	case IIO_CHAN_INFO_SAMP_FREQ:

was this missing before?
this is not covered by the patch description and seems unrelated needs to be added to the channel spec?

Quoted 27 lines
drivers/staging/iio/adc/ad7780.c +22 -0
+		*val = st->odr;
+		return IIO_VAL_INT;
 	}
 
 	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;

drivers/staging/iio/adc/ad7780.c static int ad7780_read_raw(struct iio_dev *indio_dev, · line 165

Quoted 10 lines
drivers/staging/iio/adc/ad7780.c +10 -0
+		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;

indentation with the other, previous statements

drivers/staging/iio/adc/ad7780.c static int ad7780_read_raw(struct iio_dev *indio_dev, · line 167

Quoted 2 lines
drivers/staging/iio/adc/ad7780.c +2 -0
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		if (1000*val + val2/1000 < AD7780_FILTER_MIDPOINT)

space around operators missing

Quoted 69 lines
drivers/staging/iio/adc/ad7780.c +13 -4
+			val = 0;
+		else
+			val = 1;
+		st->odr = ad778x_odr_avail[val];
+		gpiod_set_value(st->filter_gpio, val);
+	break;
+	}
+
+	return 0;
+}
+
 static int ad7780_postprocess_sample(struct ad_sigma_delta *sigma_delta,
 				     unsigned int raw_sample)
 {
@@ -126,10 +187,8 @@ 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];
+		st->odr = ad778x_odr_avail[raw_sample & AD7780_FILTER];
 	}
 
	return 0;
drivers/staging/iio/adc/ad7780.c +1 -0
@@ -173,6 +232,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)

drivers/staging/iio/adc/ad7780.c +23 -0
@@ -222,6 +282,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,
+							"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,
+							  "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;
Signature · 2 lines
Peter Meerwald-Stadler
Mobile: +43 664 24 44 418
author maintainer
#3 lore raw

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

Hi Peter,

Thank you for the review! Comments inline.

Renato

Quoted 65 lines · Peter Meerwald-Stadler
Quoted 2 lines · Renato Lui Geh

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

comments below

Quoted 58 lines

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

drivers/staging/iio/adc/ad7780.c | 95 ++++++++++++++++++++++++++++++--

1 file changed, 89 insertions(+), 6 deletions(-)
drivers/staging/iio/adc/ad7780.c +17 -2
index c4a85789c2db..6e4357800d31 100644
@@ -39,6 +39,15 @@
 #define AD7170_PATTERN		(AD7780_PAT0 | AD7170_PAT2)
 #define AD7170_PATTERN_MASK	(AD7780_PAT0 | AD7780_PAT1 | AD7170_PAT2)
 
+#define AD7780_GAIN_GPIO	0
+#define AD7780_FILTER_GPIO	1
+
+#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 +59,11 @@ 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			odr;
+	unsigned int			int_vref_mv;
 
 	struct ad_sigma_delta sd;
 };
@@ -104,17 +117,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;
Quoted 8 lines

drivers/staging/iio/adc/ad7780.c static int ad7780_read_raw(struct iio_dev *indio_dev, · line 128

Quoted 4 lines
drivers/staging/iio/adc/ad7780.c +1 -0
 	case IIO_CHAN_INFO_OFFSET:
 		*val = -(1 << (chan->scan_type.realbits - 1));
 		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SAMP_FREQ:

was this missing before?
this is not covered by the patch description and seems unrelated needs to be added to the channel spec?

Yes, this is a new feature. Should I have this chunk sent as a separate patch?

What is the channel spec? How would I add this to the channel spec?

Quoted 120 lines
Quoted 28 lines
drivers/staging/iio/adc/ad7780.c +22 -0
+		*val = st->odr;
+		return IIO_VAL_INT;
 	}
 
 	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;

drivers/staging/iio/adc/ad7780.c static int ad7780_read_raw(struct iio_dev *indio_dev, · line 165

Quoted 10 lines
drivers/staging/iio/adc/ad7780.c +10 -0
+		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;

indentation with the other, previous statements

drivers/staging/iio/adc/ad7780.c static int ad7780_read_raw(struct iio_dev *indio_dev, · line 167

Quoted 2 lines
drivers/staging/iio/adc/ad7780.c +2 -0
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		if (1000*val + val2/1000 < AD7780_FILTER_MIDPOINT)

space around operators missing

Quoted 69 lines
drivers/staging/iio/adc/ad7780.c +13 -4
+			val = 0;
+		else
+			val = 1;
+		st->odr = ad778x_odr_avail[val];
+		gpiod_set_value(st->filter_gpio, val);
+	break;
+	}
+
+	return 0;
+}
+
 static int ad7780_postprocess_sample(struct ad_sigma_delta *sigma_delta,
 				     unsigned int raw_sample)
 {
@@ -126,10 +187,8 @@ 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];
+		st->odr = ad778x_odr_avail[raw_sample & AD7780_FILTER];
 	}
 
	return 0;
drivers/staging/iio/adc/ad7780.c +1 -0
@@ -173,6 +232,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)

drivers/staging/iio/adc/ad7780.c +23 -0
@@ -222,6 +282,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,
+							"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,
+							  "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;
Signature · 2 lines
Peter Meerwald-Stadler
Mobile: +43 664 24 44 418
#4 lore raw

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

In reply to Peter Meerwald-Stadler

Peter,

this email was marked as spam for me (and probably others) because of
this DKIM signature:

   DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=pmeerw.net; s=mail;
       ....

where the problem is that when the message goes through the vger.kernel.org mailing list machinery, the header whitespace will be modified.

As a result, the DKIM signature no longer matched, and at least gmail will consider the email to be spam.

That's arguably a bug in the vger mail server setup, but equally arguably the "c=simple/simple" model of hashing in DKIM is just broken, and shouldn't be used. Whitespace in SMTP headers is simply not meaningful, and checking whitespace in them (like simple/simple does) is misguided.

If you have control over the DKIM setup of pmeerw.net, may I suggest changing the DKIM setup to use "c=relaxed/relaxed", which doesn't invalidate the signature just for whitespace changes in the headers, and which is the proper DKIM model to use. The simple canonicalization really is too simple-minded for email.

Linus

Quoted 7 lines · Peter Meerwald-Stadler
Quoted 2 lines · Renato Lui Geh

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

comments below

[...]

maintainer
#5 lore raw

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

In reply to Renato Lui Geh
Quoted 6 lines · On Tue, 5 Feb 2019 15:13:00 -0200 Renato Lui Geh <[email protected]> wrote:

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

Comments inline.

Quoted 16 lines

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

 drivers/staging/iio/adc/ad7780.c | 95 ++++++++++++++++++++++++++++++--
 1 file changed, 89 insertions(+), 6 deletions(-)
drivers/staging/iio/adc/ad7780.c +0 -0
index c4a85789c2db..6e4357800d31 100644

drivers/staging/iio/adc/ad7780.c line 43

Quoted 5 lines
drivers/staging/iio/adc/ad7780.c +2 -0
@@ -39,6 +39,15 @@
 #define AD7170_PATTERN		(AD7780_PAT0 | AD7170_PAT2)
 #define AD7170_PATTERN_MASK	(AD7780_PAT0 | AD7780_PAT1 | AD7170_PAT2)
 
+#define AD7780_GAIN_GPIO	0
+#define AD7780_FILTER_GPIO	1

What are these for?

Quoted 112 lines
drivers/staging/iio/adc/ad7780.c +64 -6
+
+#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 +59,11 @@ 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			odr;
+	unsigned int			int_vref_mv;
 
 	struct ad_sigma_delta sd;
 };
@@ -104,17 +117,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;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*val = st->odr;
+		return IIO_VAL_INT;
 	}
 
 	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;
+	}
+
+	return 0;
+}
+
 static int ad7780_postprocess_sample(struct ad_sigma_delta *sigma_delta,
 				     unsigned int raw_sample)
 {
@@ -126,10 +187,8 @@ 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];
+		st->odr = ad778x_odr_avail[raw_sample & AD7780_FILTER];
 	}
 
 	return 0;
@@ -173,6 +232,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)

drivers/staging/iio/adc/ad7780.c static int ad7780_probe(struct spi_device *spi) · line 287

Quoted 6 lines
drivers/staging/iio/adc/ad7780.c +3 -0
@@ -222,6 +282,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,
+							"gain",

These are not particularly standard names (basically not "reset"), so they should be vendor prefixed, so that people know to go look at the device specific binding.

Quoted 23 lines
drivers/staging/iio/adc/ad7780.c +20 -0
+							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,
+							  "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;
author maintainer
#6 lore raw

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

Hi Jonathan,

Thanks for the review. Comments inline.

Renato

Quoted 30 lines · Jonathan Cameron
Quoted 6 lines · On Tue, 5 Feb 2019 15:13:00 -0200 Renato Lui Geh <[email protected]> wrote:

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

Comments inline.

Quoted 19 lines

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

 drivers/staging/iio/adc/ad7780.c | 95 ++++++++++++++++++++++++++++++--
 1 file changed, 89 insertions(+), 6 deletions(-)
drivers/staging/iio/adc/ad7780.c +0 -0
index c4a85789c2db..6e4357800d31 100644
@@ -39,6 +39,15 @@
 #define AD7170_PATTERN		(AD7780_PAT0 | AD7170_PAT2)
 #define AD7170_PATTERN_MASK	(AD7780_PAT0 | AD7780_PAT1 | AD7170_PAT2)
 
Quoted 3 lines

drivers/staging/iio/adc/ad7780.c line 43

Quoted 2 lines
drivers/staging/iio/adc/ad7780.c +2 -0
+#define AD7780_GAIN_GPIO	0
+#define AD7780_FILTER_GPIO	1

What are these for?

Sorry about that. That's leftover from a previous attempt.

Quoted 116 lines
Quoted 116 lines
drivers/staging/iio/adc/ad7780.c +64 -6
+
+#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 +59,11 @@ 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			odr;
+	unsigned int			int_vref_mv;
 
 	struct ad_sigma_delta sd;
 };
@@ -104,17 +117,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;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*val = st->odr;
+		return IIO_VAL_INT;
 	}
 
 	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;
+	}
+
+	return 0;
+}
+
 static int ad7780_postprocess_sample(struct ad_sigma_delta *sigma_delta,
 				     unsigned int raw_sample)
 {
@@ -126,10 +187,8 @@ 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];
+		st->odr = ad778x_odr_avail[raw_sample & AD7780_FILTER];
 	}
 
 	return 0;
@@ -173,6 +232,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 +282,29 @@ static int ad7780_probe(struct spi_device *spi)
 		goto error_disable_reg;
 	}
 
Quoted 7 lines

drivers/staging/iio/adc/ad7780.c static int ad7780_probe(struct spi_device *spi) · line 287

Quoted 3 lines
drivers/staging/iio/adc/ad7780.c +3 -0
+	if (st->chip_info->is_ad778x) {
+		st->gain_gpio = devm_gpiod_get_optional(&spi->dev,
+							"gain",

These are not particularly standard names (basically not "reset"), so they should be vendor prefixed, so that people know to go look at the device specific binding.

I see. Should they be something like "adi,gain" and "adi,filter"? Am I correct to assume that I'll have to somehow mention these in the dt-binding?

Quoted 25 lines
Quoted 24 lines
drivers/staging/iio/adc/ad7780.c +20 -0
+							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,
+							  "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;
#7 lore raw

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

Quoted 166 lines · On Thu, 14 Feb 2019 18:31:12 -0200 Renato Lui Geh <[email protected]> wrote:

Hi Jonathan,

Thanks for the review. Comments inline.

Renato

Quoted 30 lines · Jonathan Cameron
Quoted 6 lines · On Tue, 5 Feb 2019 15:13:00 -0200 Renato Lui Geh <[email protected]> wrote:

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

Comments inline.

Quoted 19 lines

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

 drivers/staging/iio/adc/ad7780.c | 95 ++++++++++++++++++++++++++++++--
 1 file changed, 89 insertions(+), 6 deletions(-)
drivers/staging/iio/adc/ad7780.c +0 -0
index c4a85789c2db..6e4357800d31 100644
@@ -39,6 +39,15 @@
 #define AD7170_PATTERN		(AD7780_PAT0 | AD7170_PAT2)
 #define AD7170_PATTERN_MASK	(AD7780_PAT0 | AD7780_PAT1 | AD7170_PAT2)
 
Quoted 3 lines

drivers/staging/iio/adc/ad7780.c line 43

Quoted 2 lines
drivers/staging/iio/adc/ad7780.c +2 -0
+#define AD7780_GAIN_GPIO	0
+#define AD7780_FILTER_GPIO	1  

What are these for?

Sorry about that. That's leftover from a previous attempt.

Quoted 116 lines
Quoted 116 lines
drivers/staging/iio/adc/ad7780.c +64 -6
+
+#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 +59,11 @@ 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			odr;
+	unsigned int			int_vref_mv;
 
 	struct ad_sigma_delta sd;
 };
@@ -104,17 +117,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;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*val = st->odr;
+		return IIO_VAL_INT;
 	}
 
 	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;
+	}
+
+	return 0;
+}
+
 static int ad7780_postprocess_sample(struct ad_sigma_delta *sigma_delta,
 				     unsigned int raw_sample)
 {
@@ -126,10 +187,8 @@ 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];
+		st->odr = ad778x_odr_avail[raw_sample & AD7780_FILTER];
 	}
 
 	return 0;
@@ -173,6 +232,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 +282,29 @@ static int ad7780_probe(struct spi_device *spi)
 		goto error_disable_reg;
 	}
 
Quoted 7 lines

drivers/staging/iio/adc/ad7780.c static int ad7780_probe(struct spi_device *spi) · line 287

Quoted 3 lines
drivers/staging/iio/adc/ad7780.c +3 -0
+	if (st->chip_info->is_ad778x) {
+		st->gain_gpio = devm_gpiod_get_optional(&spi->dev,
+							"gain",  

These are not particularly standard names (basically not "reset"), so they should be vendor prefixed, so that people know to go look at the device specific binding.

Quoted 3 lines

I see. Should they be something like "adi,gain" and "adi,filter"? Am I correct to assume that I'll have to somehow mention these in the dt-binding?

yes and yes - name is just adi,gain-gpios rather than gain-gpios.

Take a look at the other drivers doing the same thing. We used to be more lax on this so there are drivers without the prefixes, but can't fix them now.

Jonathan

Quoted 25 lines
Quoted 25 lines
Quoted 24 lines
drivers/staging/iio/adc/ad7780.c +20 -0
+							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,
+							  "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;  
author maintainer
#8 lore raw

PATCH v3 2/4 staging: iio: ad7780: move regulator to after GPIO init

In reply to Renato Lui Geh

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

---
drivers/staging/iio/adc/ad7780.c | 26 +++++++++++++-------------

1 file changed, 13 insertions(+), 13 deletions(-)
drivers/staging/iio/adc/ad7780.c +0 -0
index 6e4357800d31..7804cd2b273e 100644
@@ -250,16 +250,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];

@@ -279,7 +269,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) {
@@ -290,7 +280,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,
@@ -301,10 +291,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;
Signature · 1 line
2.20.1
maintainer
#9 lore raw

Re: [PATCH v3 2/4] staging: iio: ad7780: move regulator to after GPIO init

Quoted 4 lines · On Tue, 5 Feb 2019 15:13:21 -0200 Renato Lui Geh <[email protected]> wrote:

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

This looks fine, will pick up with the earlier patches when ready.

Thanks,

Jonathan

Quoted 65 lines
---
 drivers/staging/iio/adc/ad7780.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)
drivers/staging/iio/adc/ad7780.c +13 -13
index 6e4357800d31..7804cd2b273e 100644
@@ -250,16 +250,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];
 
@@ -279,7 +269,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) {
@@ -290,7 +280,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,
@@ -301,10 +291,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;
author maintainer
#10 lore raw

PATCH v3 3/4 staging: iio: ad7780: add SPDX identifier

In reply to Renato Lui Geh

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

---
drivers/staging/iio/adc/ad7780.c | 3 +--

1 file changed, 1 insertion(+), 2 deletions(-)
drivers/staging/iio/adc/ad7780.c +1 -0
index 7804cd2b273e..163e3c983598 100644
@@ -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>

Signature · 1 line
2.20.1
author maintainer
#11 lore raw

PATCH v3 4/4 staging: iio: ad7780: moving ad7780 out of staging

In reply to 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.

---
Changes in v3:
- Changes unrelated to moving the driver to main tree were resent as individual patches

drivers/iio/adc/Kconfig          |  13 ++
drivers/iio/adc/Makefile         |   1 +
drivers/iio/adc/ad7780.c         | 359 +++++++++++++++++++++++++++++++
drivers/staging/iio/adc/Kconfig  |  13 --
drivers/staging/iio/adc/Makefile |   1 -
drivers/staging/iio/adc/ad7780.c | 359 -------------------------------
6 files changed, 373 insertions(+), 373 deletions(-)

create mode 100644 drivers/iio/adc/ad7780.c delete mode 100644 drivers/staging/iio/adc/ad7780.c

drivers/iio/adc/Kconfig +0 -0
index f3cc7a31bce5..2cdee166d0e9 100644
@@ -108,6 +108,19 @@ 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).
+	  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 AD7791
tristate "Analog Devices AD7791 ADC driver" depends on SPI

drivers/iio/adc/Makefile +0 -0
index ea5031348052..b48852157115 100644
@@ -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

drivers/iio/adc/ad7780.c new file +359 -0

359 lines not on this page · raw message

drivers/staging/iio/adc/Kconfig +0 -0
index 7a93d3a5c113..404a53c743a6 100644
@@ -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

drivers/staging/iio/adc/Makefile +0 -0
index 7a421088ff82..4b76769b32bc 100644
@@ -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

drivers/staging/iio/adc/ad7780.c deleted +0 -359

359 lines not on this page · raw message

Signature · 1 line
2.20.1
maintainer
#12 lore raw

Re: [PATCH v3 4/4] staging: iio: ad7780: moving ad7780 out of staging

Quoted 10 lines · On Tue, 5 Feb 2019 15:14:03 -0200 Renato Lui Geh <[email protected]> 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.

This needs a device tree binding doc which should be reviewed before we move the driver out of staging. Make sure to cc the dt-binding maintainers and list. Doesn't really matter if that patch is before or after this one in the series but needs to be in the same series.

There are a few more minor tidy ups that would be nice to have inline given you are doing a v4. Stuff like this could have been cleaned up after moving out of staging (nothing wrong with improving non staging drivers after all) but always better to do it whilst we remember!

Quoted 22 lines

---
Changes in v3:
- Changes unrelated to moving the driver to main tree were resent as individual patches

 drivers/iio/adc/Kconfig          |  13 ++
 drivers/iio/adc/Makefile         |   1 +
 drivers/iio/adc/ad7780.c         | 359 +++++++++++++++++++++++++++++++
 drivers/staging/iio/adc/Kconfig  |  13 --
 drivers/staging/iio/adc/Makefile |   1 -
 drivers/staging/iio/adc/ad7780.c | 359 -------------------------------
 6 files changed, 373 insertions(+), 373 deletions(-)
 create mode 100644 drivers/iio/adc/ad7780.c
 delete mode 100644 drivers/staging/iio/adc/ad7780.c
drivers/iio/adc/Kconfig +0 -0
index f3cc7a31bce5..2cdee166d0e9 100644
@@ -108,6 +108,19 @@ config AD7766
 	  To compile this driver as a module, choose M here: the module will be
 	  called ad7766.

drivers/iio/adc/Kconfig config AD7766 · line 119

Quoted 10 lines
drivers/iio/adc/Kconfig +9 -0
 
+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").

I wouldn't bother with this statement, doesn't add any real info!

Quoted 25 lines
drivers/iio/adc/Kconfig +4 -0
+
+	  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
drivers/iio/adc/Makefile +1 -0
index ea5031348052..b48852157115 100644
@@ -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
drivers/iio/adc/ad7780.c new file +0 -0
new file mode 100644
index 000000000000..163e3c983598

drivers/iio/adc/ad7780.c line 5

Quoted 5 lines
drivers/iio/adc/ad7780.c new file +5 -0
@@ -0,0 +1,359 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * AD7170/AD7171 and AD7780/AD7781 SPI ADC driver
+ *
+ * Copyright 2011 Analog Devices Inc.

I think you have done more than enough to this driver to add an additional copyright line if you want to!

Quoted 16 lines
drivers/iio/adc/ad7780.c new file +16 -0
+ */
+
+#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/iio/iio.h>
+#include <linux/iio/sysfs.h>

drivers/iio/adc/ad7780.c line 31

Quoted 10 lines
drivers/iio/adc/ad7780.c new file +10 -0
+#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_ID1		BIT(4)
+#define AD7780_ID0		BIT(3)
+#define AD7780_GAIN		BIT(2)
+#define AD7780_PAT1		BIT(1)
+#define AD7780_PAT0		BIT(0)

These two bits of pattern don't really add anything. I'd drop them in favour of something like

#define AD7780_PATTERN_GOOD 1
#define AD7780_PATTERN_MASK GENMASK(1, 0)

Same for ID for that matter. These aren't one bit fields, so we shouldn't ever present them as such (though the datasheet confusingly sort of does so!)

drivers/iio/adc/ad7780.c line 39

Quoted 8 lines
drivers/iio/adc/ad7780.c new file +8 -0
+
+#define AD7780_PATTERN		(AD7780_PAT0)
+#define AD7780_PATTERN_MASK	(AD7780_PAT0 | AD7780_PAT1)
+
+#define AD7170_PAT2		BIT(2)
+
+#define AD7170_PATTERN		(AD7780_PAT0 | AD7170_PAT2)
+#define AD7170_PATTERN_MASK	(AD7780_PAT0 | AD7780_PAT1 | AD7170_PAT2)

I'd use a value for the pattern directly and
GENMASK for the mask.

Quoted 123 lines
drivers/iio/adc/ad7780.c new file +123 -0
+
+#define AD7780_GAIN_GPIO	0
+#define AD7780_FILTER_GPIO	1
+
+#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;
+	}
+
+	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;

drivers/iio/adc/ad7780.c line 172

Quoted 10 lines
drivers/iio/adc/ad7780.c new file +10 -0
+		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;

We'll get a warning here due to the lack of a default handler. It's pointless except to suppress the warning, but best to add one.

Quoted 187 lines
drivers/iio/adc/ad7780.c new file +187 -0
+	}
+
+	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_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),
+		.pattern = AD7170_PATTERN,
+		.pattern_mask = AD7170_PATTERN_MASK,
+		.is_ad778x = false,
+	},
+	[ID_AD7171] = {
+		.channel = AD7780_CHANNEL(16, 24),
+		.pattern = AD7170_PATTERN,
+		.pattern_mask = AD7170_PATTERN_MASK,
+		.is_ad778x = false,
+	},
+	[ID_AD7780] = {
+		.channel = AD7780_CHANNEL(24, 32),
+		.pattern = AD7780_PATTERN,
+		.pattern_mask = AD7780_PATTERN_MASK,
+		.is_ad778x = true,
+	},
+	[ID_AD7781] = {
+		.channel = AD7780_CHANNEL(20, 32),
+		.pattern = AD7780_PATTERN,
+		.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,
+							"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,
+							  "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 <[email protected]>");
+MODULE_DESCRIPTION("Analog Devices AD7780 and similar ADCs");
+MODULE_LICENSE("GPL v2");
author maintainer
#13 lore raw

Re: [PATCH v3 4/4] staging: iio: ad7780: moving ad7780 out of staging

Quoted 10 lines · Jonathan Cameron
Quoted 7 lines · On Tue, 5 Feb 2019 15:14:03 -0200 Renato Lui Geh <[email protected]> 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.

Quoted 8 lines
Quoted 3 lines

This needs a device tree binding doc which should be reviewed before we move the driver out of staging. Make sure to cc the dt-binding maintainers and list. Doesn't really matter if that patch is before or after this one in the series but needs to be in the same series.

Ok! I see that some Analog dt-bindings are prefixed by adi and some are not. Should I follow any naming standard?

Quoted 66 lines

There are a few more minor tidy ups that would be nice to have inline given you are doing a v4. Stuff like this could have been cleaned up after moving out of staging (nothing wrong with improving non staging drivers after all) but always better to do it whilst we remember!

Quoted 22 lines

---
Changes in v3:
- Changes unrelated to moving the driver to main tree were resent as individual patches

 drivers/iio/adc/Kconfig          |  13 ++
 drivers/iio/adc/Makefile         |   1 +
 drivers/iio/adc/ad7780.c         | 359 +++++++++++++++++++++++++++++++
 drivers/staging/iio/adc/Kconfig  |  13 --
 drivers/staging/iio/adc/Makefile |   1 -
 drivers/staging/iio/adc/ad7780.c | 359 -------------------------------
 6 files changed, 373 insertions(+), 373 deletions(-)
 create mode 100644 drivers/iio/adc/ad7780.c
 delete mode 100644 drivers/staging/iio/adc/ad7780.c
drivers/iio/adc/Kconfig +0 -0
index f3cc7a31bce5..2cdee166d0e9 100644
@@ -108,6 +108,19 @@ config AD7766
 	  To compile this driver as a module, choose M here: the module will be
 	  called ad7766.

drivers/iio/adc/Kconfig config AD7766 · line 119

Quoted 10 lines
drivers/iio/adc/Kconfig +9 -0
 
+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").

I wouldn't bother with this statement, doesn't add any real info!

Quoted 25 lines
drivers/iio/adc/Kconfig +4 -0
+
+	  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
drivers/iio/adc/Makefile +1 -0
index ea5031348052..b48852157115 100644
@@ -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
drivers/iio/adc/ad7780.c new file +0 -0
new file mode 100644
index 000000000000..163e3c983598
@@ -0,0 +1,359 @@
Quoted 8 lines

drivers/iio/adc/ad7780.c line 5

Quoted 5 lines
drivers/iio/adc/ad7780.c new file +5 -0
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * AD7170/AD7171 and AD7780/AD7781 SPI ADC driver
+ *
+ * Copyright 2011 Analog Devices Inc.

I think you have done more than enough to this driver to add an additional copyright line if you want to!

Oh wow, that'd be awesome! Thanks! Should I just add my full name there?

Quoted 371 lines
Quoted 17 lines
drivers/iio/adc/ad7780.c new file +16 -0
+ */
+
+#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/iio/iio.h>
+#include <linux/iio/sysfs.h>

drivers/iio/adc/ad7780.c line 31

Quoted 10 lines
drivers/iio/adc/ad7780.c new file +10 -0
+#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_ID1		BIT(4)
+#define AD7780_ID0		BIT(3)
+#define AD7780_GAIN		BIT(2)
+#define AD7780_PAT1		BIT(1)
+#define AD7780_PAT0		BIT(0)

These two bits of pattern don't really add anything. I'd drop them in favour of something like

#define AD7780_PATTERN_GOOD 1
#define AD7780_PATTERN_MASK GENMASK(1, 0)

Same for ID for that matter. These aren't one bit fields, so we shouldn't ever present them as such (though the datasheet confusingly sort of does so!)

drivers/iio/adc/ad7780.c line 39

Quoted 8 lines
drivers/iio/adc/ad7780.c new file +8 -0
+
+#define AD7780_PATTERN		(AD7780_PAT0)
+#define AD7780_PATTERN_MASK	(AD7780_PAT0 | AD7780_PAT1)
+
+#define AD7170_PAT2		BIT(2)
+
+#define AD7170_PATTERN		(AD7780_PAT0 | AD7170_PAT2)
+#define AD7170_PATTERN_MASK	(AD7780_PAT0 | AD7780_PAT1 | AD7170_PAT2)

I'd use a value for the pattern directly and
GENMASK for the mask.

Quoted 123 lines
drivers/iio/adc/ad7780.c new file +123 -0
+
+#define AD7780_GAIN_GPIO	0
+#define AD7780_FILTER_GPIO	1
+
+#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;
+	}
+
+	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;

drivers/iio/adc/ad7780.c line 172

Quoted 10 lines
drivers/iio/adc/ad7780.c new file +10 -0
+		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;

We'll get a warning here due to the lack of a default handler. It's pointless except to suppress the warning, but best to add one.

Quoted 187 lines
drivers/iio/adc/ad7780.c new file +187 -0
+	}
+
+	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_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),
+		.pattern = AD7170_PATTERN,
+		.pattern_mask = AD7170_PATTERN_MASK,
+		.is_ad778x = false,
+	},
+	[ID_AD7171] = {
+		.channel = AD7780_CHANNEL(16, 24),
+		.pattern = AD7170_PATTERN,
+		.pattern_mask = AD7170_PATTERN_MASK,
+		.is_ad778x = false,
+	},
+	[ID_AD7780] = {
+		.channel = AD7780_CHANNEL(24, 32),
+		.pattern = AD7780_PATTERN,
+		.pattern_mask = AD7780_PATTERN_MASK,
+		.is_ad778x = true,
+	},
+	[ID_AD7781] = {
+		.channel = AD7780_CHANNEL(20, 32),
+		.pattern = AD7780_PATTERN,
+		.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,
+							"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,
+							  "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 <[email protected]>");
+MODULE_DESCRIPTION("Analog Devices AD7780 and similar ADCs");
+MODULE_LICENSE("GPL v2");
#14 lore raw

Re: [PATCH v3 4/4] staging: iio: ad7780: moving ad7780 out of staging

Quoted 15 lines · On Thu, 14 Feb 2019 18:48:15 -0200 Renato Lui Geh <[email protected]> wrote:
Quoted 14 lines · Jonathan Cameron
Quoted 10 lines · On Tue, 5 Feb 2019 15:14:03 -0200 Renato Lui Geh <[email protected]> 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.

Quoted 7 lines
Quoted 4 lines

This needs a device tree binding doc which should be reviewed before we move the driver out of staging. Make sure to cc the dt-binding maintainers and list. Doesn't really matter if that patch is before or after this one in the series but needs to be in the same series.

Ok! I see that some Analog dt-bindings are prefixed by adi and some are not. Should I follow any naming standard?

Yeah, comes of history and the fact that we can't 'fix' a binding that is in the wild if the driver is already out of staging. Even in the staging case we won't generally do it if we know of real devices out there using the defacto binding.

They should be prefixed if they aren't 'generic'. Exactly where that lines tends to be a little unclear unfortunately.

Quoted 75 lines
Quoted 66 lines

There are a few more minor tidy ups that would be nice to have inline given you are doing a v4. Stuff like this could have been cleaned up after moving out of staging (nothing wrong with improving non staging drivers after all) but always better to do it whilst we remember!

Quoted 22 lines

---
Changes in v3:
- Changes unrelated to moving the driver to main tree were resent as individual patches

 drivers/iio/adc/Kconfig          |  13 ++
 drivers/iio/adc/Makefile         |   1 +
 drivers/iio/adc/ad7780.c         | 359 +++++++++++++++++++++++++++++++
 drivers/staging/iio/adc/Kconfig  |  13 --
 drivers/staging/iio/adc/Makefile |   1 -
 drivers/staging/iio/adc/ad7780.c | 359 -------------------------------
 6 files changed, 373 insertions(+), 373 deletions(-)
 create mode 100644 drivers/iio/adc/ad7780.c
 delete mode 100644 drivers/staging/iio/adc/ad7780.c
drivers/iio/adc/Kconfig +0 -0
index f3cc7a31bce5..2cdee166d0e9 100644
@@ -108,6 +108,19 @@ config AD7766
 	  To compile this driver as a module, choose M here: the module will be
 	  called ad7766.

drivers/iio/adc/Kconfig config AD7766 · line 119

Quoted 10 lines
drivers/iio/adc/Kconfig +9 -0
 
+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").  

I wouldn't bother with this statement, doesn't add any real info!

Quoted 25 lines
drivers/iio/adc/Kconfig +4 -0
+
+	  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
drivers/iio/adc/Makefile +1 -0
index ea5031348052..b48852157115 100644
@@ -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
drivers/iio/adc/ad7780.c new file +0 -0
new file mode 100644
index 000000000000..163e3c983598
@@ -0,0 +1,359 @@
Quoted 8 lines

drivers/iio/adc/ad7780.c line 5

Quoted 5 lines
drivers/iio/adc/ad7780.c new file +5 -0
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * AD7170/AD7171 and AD7780/AD7781 SPI ADC driver
+ *
+ * Copyright 2011 Analog Devices Inc.  

I think you have done more than enough to this driver to add an additional copyright line if you want to!

Quoted 1 line

Oh wow, that'd be awesome! Thanks! Should I just add my full name there?

Yes.

Quoted 371 lines
Quoted 371 lines
Quoted 17 lines
drivers/iio/adc/ad7780.c new file +16 -0
+ */
+
+#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/iio/iio.h>
+#include <linux/iio/sysfs.h>

drivers/iio/adc/ad7780.c line 31

Quoted 10 lines
drivers/iio/adc/ad7780.c new file +10 -0
+#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_ID1		BIT(4)
+#define AD7780_ID0		BIT(3)
+#define AD7780_GAIN		BIT(2)
+#define AD7780_PAT1		BIT(1)
+#define AD7780_PAT0		BIT(0)  

These two bits of pattern don't really add anything. I'd drop them in favour of something like

#define AD7780_PATTERN_GOOD 1
#define AD7780_PATTERN_MASK GENMASK(1, 0)

Same for ID for that matter. These aren't one bit fields, so we shouldn't ever present them as such (though the datasheet confusingly sort of does so!)

drivers/iio/adc/ad7780.c line 39

Quoted 8 lines
drivers/iio/adc/ad7780.c new file +8 -0
+
+#define AD7780_PATTERN		(AD7780_PAT0)
+#define AD7780_PATTERN_MASK	(AD7780_PAT0 | AD7780_PAT1)
+
+#define AD7170_PAT2		BIT(2)
+
+#define AD7170_PATTERN		(AD7780_PAT0 | AD7170_PAT2)
+#define AD7170_PATTERN_MASK	(AD7780_PAT0 | AD7780_PAT1 | AD7170_PAT2)  

I'd use a value for the pattern directly and
GENMASK for the mask.

Quoted 123 lines
drivers/iio/adc/ad7780.c new file +123 -0
+
+#define AD7780_GAIN_GPIO	0
+#define AD7780_FILTER_GPIO	1
+
+#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;
+	}
+
+	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;

drivers/iio/adc/ad7780.c line 172

Quoted 10 lines
drivers/iio/adc/ad7780.c new file +10 -0
+		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;  

We'll get a warning here due to the lack of a default handler. It's pointless except to suppress the warning, but best to add one.

Quoted 187 lines
drivers/iio/adc/ad7780.c new file +187 -0
+	}
+
+	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_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),
+		.pattern = AD7170_PATTERN,
+		.pattern_mask = AD7170_PATTERN_MASK,
+		.is_ad778x = false,
+	},
+	[ID_AD7171] = {
+		.channel = AD7780_CHANNEL(16, 24),
+		.pattern = AD7170_PATTERN,
+		.pattern_mask = AD7170_PATTERN_MASK,
+		.is_ad778x = false,
+	},
+	[ID_AD7780] = {
+		.channel = AD7780_CHANNEL(24, 32),
+		.pattern = AD7780_PATTERN,
+		.pattern_mask = AD7780_PATTERN_MASK,
+		.is_ad778x = true,
+	},
+	[ID_AD7781] = {
+		.channel = AD7780_CHANNEL(20, 32),
+		.pattern = AD7780_PATTERN,
+		.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,
+							"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,
+							  "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 <[email protected]>");
+MODULE_DESCRIPTION("Analog Devices AD7780 and similar ADCs");
+MODULE_LICENSE("GPL v2");