AD7949 Fixes

Replace raw configuration register values by using FIELD_PREP and defines to improve readability.

drivers/iio/adc/ad7949.c +2 -0

+/* SEQ: channel sequencer. Allows for scanning channels */
+#define AD7949_CFG_BIT_SEQ		GENMASK(2, 1)

Quoted 2 lines · Liam Beguin

Replace raw configuration register values by using FIELD_PREP and defines to improve readability.

[]

drivers/iio/adc/ad7949.c

Quoted 1 line

drivers/iio/adc/ad7949.c +0 -0

[]
+#define AD7949_CFG_BIT_INCC GENMASK(12, 10)

I think the naming is a bit confusing as it appears as if these bitfield ranges are single bits.

drivers/iio/adc/ad7949.c line 38

Quoted 3 lines

drivers/iio/adc/ad7949.c +2 -0

+/* REF: reference/buffer selection */
+#define AD7949_CFG_BIT_REF		GENMASK(5, 3)

[]

Quoted 3 lines

drivers/iio/adc/ad7949.c +2 -0

+/* SEQ: channel sequencer. Allows for scanning channels */
+#define AD7949_CFG_BIT_SEQ		GENMASK(2, 1)

Quoted 8 lines · Joe Perches

Quoted 2 lines · Liam Beguin

Replace raw configuration register values by using FIELD_PREP and defines to improve readability.

[]

drivers/iio/adc/ad7949.c

Quoted 1 line

drivers/iio/adc/ad7949.c +0 -0

[]
+#define AD7949_CFG_BIT_INCC GENMASK(12, 10)

Hi Joe,

Quoted 2 lines

I think the naming is a bit confusing as it appears as if these bitfield ranges are single bits.

That makes sense.
Would AD7949_CFG_BITS_* be good enough?

From: Liam Beguin <[email protected]>

Add support for selecting a custom reference voltage from the devicetree. If an external source is used, a vref regulator should be defined in the devicetree.

Signed-off-by Liam Beguin <[email protected]>

---
 drivers/iio/adc/ad7949.c | 140 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 122 insertions(+), 18 deletions(-)

drivers/iio/adc/ad7949.c +99 -16

index 3f94ae639a44..14a7c79d637e 100644
@@ -35,7 +35,11 @@
 
 /* REF: reference/buffer selection */
 #define AD7949_CFG_BIT_REF		GENMASK(5, 3)
-#define AD7949_CFG_VAL_REF_EXT_BUF		7
+#define AD7949_CFG_VAL_REF_EXT_TEMP_BUF		3
+#define AD7949_CFG_VAL_REF_EXT_TEMP		2
+#define AD7949_CFG_VAL_REF_INT_4096		1
+#define AD7949_CFG_VAL_REF_INT_2500		0
+#define AD7949_CFG_VAL_REF_EXTERNAL		BIT(1)
 
 /* SEQ: channel sequencer. Allows for scanning channels */
 #define AD7949_CFG_BIT_SEQ		GENMASK(2, 1)

@@ -60,6 +64,14 @@ static const struct ad7949_adc_spec ad7949_adc_spec[] = {
 	[ID_AD7689] = { .num_channels = 8, .resolution = 16 },
 };
 
+/**
+ * struct ad7949_channel - ADC Channel parameters
+ * @refsel: reference selection
+ */
+struct ad7949_channel {
+	u32 refsel;
+};
+
 /**
  * struct ad7949_adc_chip - AD ADC chip
  * @lock: protects write sequences

@@ -78,6 +90,7 @@ struct ad7949_adc_chip {
 	struct regulator *vref;
 	struct iio_dev *indio_dev;
 	struct spi_device *spi;
+	struct ad7949_channel *channels;
 	u8 resolution;
 	u8 bits_per_word;
 	u16 cfg;

@@ -136,6 +149,7 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
 	int ret;
 	int i;
 	struct spi_message msg;
+	struct ad7949_channel *ad7949_chan = &ad7949_adc->channels[channel];
 	struct spi_transfer tx[] = {
 		{
 			.len = 2,

@@ -156,8 +170,9 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
 	 */
 	for (i = 0; i < 2; i++) {
 		ret = ad7949_spi_write_cfg(ad7949_adc,
-					   FIELD_PREP(AD7949_CFG_BIT_INX, channel),
-					   AD7949_CFG_BIT_INX);
+					   FIELD_PREP(AD7949_CFG_BIT_INX, channel) |
+					   FIELD_PREP(AD7949_CFG_BIT_REF, ad7949_chan->refsel),
+					   AD7949_CFG_BIT_INX | AD7949_CFG_BIT_REF);
 		if (ret)
 			return ret;
 		if (channel == ad7949_adc->current_channel)

@@ -225,6 +240,7 @@ static int ad7949_spi_read_raw(struct iio_dev *indio_dev,
 			   int *val, int *val2, long mask)
 {
 	struct ad7949_adc_chip *ad7949_adc = iio_priv(indio_dev);
+	struct ad7949_channel *ad7949_chan = &ad7949_adc->channels[chan->channel];
 	int ret;
 
 	if (!val)

@@ -242,12 +258,26 @@ static int ad7949_spi_read_raw(struct iio_dev *indio_dev,
 		return IIO_VAL_INT;
 
 	case IIO_CHAN_INFO_SCALE:
-		ret = regulator_get_voltage(ad7949_adc->vref);
-		if (ret < 0)
-			return ret;
+		switch (ad7949_chan->refsel) {
+		case AD7949_CFG_VAL_REF_INT_2500:
+			*val = 2500;
+			break;
+		case AD7949_CFG_VAL_REF_INT_4096:
+			*val = 4096;
+			break;
+		case AD7949_CFG_VAL_REF_EXT_TEMP:
+		case AD7949_CFG_VAL_REF_EXT_TEMP_BUF:
+			ret = regulator_get_voltage(ad7949_adc->vref);
+			if (ret < 0)
+				return ret;
+
+			/* convert value back to mV */
+			*val = ret / 1000;
+			break;
+		}
 
-		*val = ret / 5000;
-		return IIO_VAL_INT;
+		*val2 = (1 << ad7949_adc->resolution) - 1;
+		return IIO_VAL_FRACTIONAL;
 	}
 
 	return -EINVAL;

@@ -286,7 +316,7 @@ static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
 		FIELD_PREP(AD7949_CFG_BIT_INCC, AD7949_CFG_VAL_INCC_UNIPOLAR_GND) |
 		FIELD_PREP(AD7949_CFG_BIT_INX, ad7949_adc->current_channel) |
 		FIELD_PREP(AD7949_CFG_BIT_BW_FULL, 1) |
-		FIELD_PREP(AD7949_CFG_BIT_REF, AD7949_CFG_VAL_REF_EXT_BUF) |
+		FIELD_PREP(AD7949_CFG_BIT_REF, ad7949_adc->channels[0].refsel) |
 		FIELD_PREP(AD7949_CFG_BIT_SEQ, 0x0) |
 		FIELD_PREP(AD7949_CFG_BIT_RBN, 1);
 

@@ -302,14 +332,24 @@ static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
 	return ret;
 }
 
+static void ad7949_disable_reg(void *reg)
+{
+	regulator_disable(reg);
+}
+
 static int ad7949_spi_probe(struct spi_device *spi)
 {
 	u32 spi_ctrl_mask = spi->controller->bits_per_word_mask;
 	struct device *dev = &spi->dev;
 	const struct ad7949_adc_spec *spec;
 	struct ad7949_adc_chip *ad7949_adc;
+	struct ad7949_channel *ad7949_chan;
+	struct fwnode_handle *child;
 	struct iio_dev *indio_dev;
+	int mode;
+	u32 tmp;
 	int ret;
+	int i;
 
 	indio_dev = devm_iio_device_alloc(dev, sizeof(*ad7949_adc));
 	if (!indio_dev) {

@@ -343,16 +383,82 @@ static int ad7949_spi_probe(struct spi_device *spi)
 		return -EINVAL;
 	}
 
-	ad7949_adc->vref = devm_regulator_get(dev, "vref");
+	/* Setup external voltage ref, buffered? */
+	ad7949_adc->vref = devm_regulator_get_optional(dev, "vrefin");
 	if (IS_ERR(ad7949_adc->vref)) {
-		dev_err(dev, "fail to request regulator\n");
-		return PTR_ERR(ad7949_adc->vref);
+		ret = PTR_ERR(ad7949_adc->vref);
+		if (ret != -ENODEV)
+			return ret;
+		/* unbuffered? */
+		ad7949_adc->vref = devm_regulator_get_optional(dev, "vref");
+		if (IS_ERR(ad7949_adc->vref)) {
+			ret = PTR_ERR(ad7949_adc->vref);
+			if (ret != -ENODEV)
+				return ret;
+			/* Internal then */
+			mode = AD7949_CFG_VAL_REF_INT_4096;
+		} else {
+			mode = AD7949_CFG_VAL_REF_EXT_TEMP;
+		}
+	} else {
+		mode = AD7949_CFG_VAL_REF_EXT_TEMP_BUF;
 	}
 
-	ret = regulator_enable(ad7949_adc->vref);
-	if (ret < 0) {
-		dev_err(dev, "fail to enable regulator\n");
-		return ret;
+	if (mode & AD7949_CFG_VAL_REF_EXTERNAL) {
+		ret = regulator_enable(ad7949_adc->vref);
+		if (ret < 0) {
+			dev_err(dev, "fail to enable regulator\n");
+			return ret;
+		}
+
+		ret = devm_add_action_or_reset(dev, ad7949_disable_reg,
+					       ad7949_adc->vref);
+		if (ret)
+			return ret;
+	}
+
+	ad7949_adc->channels = devm_kcalloc(dev, spec->num_channels,
+					    sizeof(*ad7949_adc->channels),
+					    GFP_KERNEL);
+	if (!ad7949_adc->channels) {
+		dev_err(dev, "unable to allocate ADC channels\n");
+		return -ENOMEM;
+	}
+
+	/* Initialize all channel structures */
+	for (i = 0; i < spec->num_channels; i++)
+		ad7949_adc->channels[i].refsel = mode;
+
+	/* Read channel specific information form the devicetree */
+	device_for_each_child_node(dev, child) {
+		ret = fwnode_property_read_u32(child, "reg", &i);
+		if (ret) {
+			dev_err(dev, "missing reg property in %pfw\n", child);
+			fwnode_handle_put(child);
+			return ret;

drivers/iio/adc/ad7949.c +13 -2

+
+		switch (tmp) {
+		case 2500000:
+			ad7949_chan->refsel = AD7949_CFG_VAL_REF_INT_2500;
+			break;
+		case 4096000:
+			ad7949_chan->refsel = AD7949_CFG_VAL_REF_INT_4096;
+			break;
+		default:
+			dev_err(dev, "unsupported internal voltage reference\n");
+			fwnode_handle_put(child);
+			return -EINVAL;
+		}
 	}
 
 	mutex_init(&ad7949_adc->lock);

@@ -373,7 +479,6 @@ static int ad7949_spi_probe(struct spi_device *spi)
 
 err:
 	mutex_destroy(&ad7949_adc->lock);
-	regulator_disable(ad7949_adc->vref);
 
 	return ret;
 }

@@ -385,7 +490,6 @@ static int ad7949_spi_remove(struct spi_device *spi)
 
 	iio_device_unregister(indio_dev);
 	mutex_destroy(&ad7949_adc->lock);
-	regulator_disable(ad7949_adc->vref);
 
 	return 0;
 }

Quoted 7 lines · On Sat, 7 Aug 2021 21:56:57 -0400 Liam Beguin <[email protected]> wrote:

From: Liam Beguin <[email protected]>

Add support for selecting a custom reference voltage from the devicetree. If an external source is used, a vref regulator should be defined in the devicetree.

Signed-off-by Liam Beguin <[email protected]>

Hi Liam, I'm not sure the parsing of the child nodes will work if using an external reference. See below.

Jonathan

Quoted 204 lines

---
 drivers/iio/adc/ad7949.c | 140 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 122 insertions(+), 18 deletions(-)

drivers/iio/adc/ad7949.c +101 -16

index 3f94ae639a44..14a7c79d637e 100644
@@ -35,7 +35,11 @@
 
 /* REF: reference/buffer selection */
 #define AD7949_CFG_BIT_REF		GENMASK(5, 3)
-#define AD7949_CFG_VAL_REF_EXT_BUF		7
+#define AD7949_CFG_VAL_REF_EXT_TEMP_BUF		3
+#define AD7949_CFG_VAL_REF_EXT_TEMP		2
+#define AD7949_CFG_VAL_REF_INT_4096		1
+#define AD7949_CFG_VAL_REF_INT_2500		0
+#define AD7949_CFG_VAL_REF_EXTERNAL		BIT(1)
 
 /* SEQ: channel sequencer. Allows for scanning channels */
 #define AD7949_CFG_BIT_SEQ		GENMASK(2, 1)

@@ -60,6 +64,14 @@ static const struct ad7949_adc_spec ad7949_adc_spec[] = {
 	[ID_AD7689] = { .num_channels = 8, .resolution = 16 },
 };
 
+/**
+ * struct ad7949_channel - ADC Channel parameters
+ * @refsel: reference selection
+ */
+struct ad7949_channel {
+	u32 refsel;
+};
+
 /**
  * struct ad7949_adc_chip - AD ADC chip
  * @lock: protects write sequences

@@ -78,6 +90,7 @@ struct ad7949_adc_chip {
 	struct regulator *vref;
 	struct iio_dev *indio_dev;
 	struct spi_device *spi;
+	struct ad7949_channel *channels;
 	u8 resolution;
 	u8 bits_per_word;
 	u16 cfg;

@@ -136,6 +149,7 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
 	int ret;
 	int i;
 	struct spi_message msg;
+	struct ad7949_channel *ad7949_chan = &ad7949_adc->channels[channel];
 	struct spi_transfer tx[] = {
 		{
 			.len = 2,

@@ -156,8 +170,9 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
 	 */
 	for (i = 0; i < 2; i++) {
 		ret = ad7949_spi_write_cfg(ad7949_adc,
-					   FIELD_PREP(AD7949_CFG_BIT_INX, channel),
-					   AD7949_CFG_BIT_INX);
+					   FIELD_PREP(AD7949_CFG_BIT_INX, channel) |
+					   FIELD_PREP(AD7949_CFG_BIT_REF, ad7949_chan->refsel),
+					   AD7949_CFG_BIT_INX | AD7949_CFG_BIT_REF);
 		if (ret)
 			return ret;
 		if (channel == ad7949_adc->current_channel)

@@ -225,6 +240,7 @@ static int ad7949_spi_read_raw(struct iio_dev *indio_dev,
 			   int *val, int *val2, long mask)
 {
 	struct ad7949_adc_chip *ad7949_adc = iio_priv(indio_dev);
+	struct ad7949_channel *ad7949_chan = &ad7949_adc->channels[chan->channel];
 	int ret;
 
 	if (!val)

@@ -242,12 +258,26 @@ static int ad7949_spi_read_raw(struct iio_dev *indio_dev,
 		return IIO_VAL_INT;
 
 	case IIO_CHAN_INFO_SCALE:
-		ret = regulator_get_voltage(ad7949_adc->vref);
-		if (ret < 0)
-			return ret;
+		switch (ad7949_chan->refsel) {
+		case AD7949_CFG_VAL_REF_INT_2500:
+			*val = 2500;
+			break;
+		case AD7949_CFG_VAL_REF_INT_4096:
+			*val = 4096;
+			break;
+		case AD7949_CFG_VAL_REF_EXT_TEMP:
+		case AD7949_CFG_VAL_REF_EXT_TEMP_BUF:
+			ret = regulator_get_voltage(ad7949_adc->vref);
+			if (ret < 0)
+				return ret;
+
+			/* convert value back to mV */
+			*val = ret / 1000;
+			break;
+		}
 
-		*val = ret / 5000;
-		return IIO_VAL_INT;
+		*val2 = (1 << ad7949_adc->resolution) - 1;
+		return IIO_VAL_FRACTIONAL;
 	}
 
 	return -EINVAL;

@@ -286,7 +316,7 @@ static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
 		FIELD_PREP(AD7949_CFG_BIT_INCC, AD7949_CFG_VAL_INCC_UNIPOLAR_GND) |
 		FIELD_PREP(AD7949_CFG_BIT_INX, ad7949_adc->current_channel) |
 		FIELD_PREP(AD7949_CFG_BIT_BW_FULL, 1) |
-		FIELD_PREP(AD7949_CFG_BIT_REF, AD7949_CFG_VAL_REF_EXT_BUF) |
+		FIELD_PREP(AD7949_CFG_BIT_REF, ad7949_adc->channels[0].refsel) |
 		FIELD_PREP(AD7949_CFG_BIT_SEQ, 0x0) |
 		FIELD_PREP(AD7949_CFG_BIT_RBN, 1);
 

@@ -302,14 +332,24 @@ static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
 	return ret;
 }
 
+static void ad7949_disable_reg(void *reg)
+{
+	regulator_disable(reg);
+}
+
 static int ad7949_spi_probe(struct spi_device *spi)
 {
 	u32 spi_ctrl_mask = spi->controller->bits_per_word_mask;
 	struct device *dev = &spi->dev;
 	const struct ad7949_adc_spec *spec;
 	struct ad7949_adc_chip *ad7949_adc;
+	struct ad7949_channel *ad7949_chan;
+	struct fwnode_handle *child;
 	struct iio_dev *indio_dev;
+	int mode;
+	u32 tmp;
 	int ret;
+	int i;
 
 	indio_dev = devm_iio_device_alloc(dev, sizeof(*ad7949_adc));
 	if (!indio_dev) {

@@ -343,16 +383,82 @@ static int ad7949_spi_probe(struct spi_device *spi)
 		return -EINVAL;
 	}
 
-	ad7949_adc->vref = devm_regulator_get(dev, "vref");
+	/* Setup external voltage ref, buffered? */
+	ad7949_adc->vref = devm_regulator_get_optional(dev, "vrefin");
 	if (IS_ERR(ad7949_adc->vref)) {
-		dev_err(dev, "fail to request regulator\n");
-		return PTR_ERR(ad7949_adc->vref);
+		ret = PTR_ERR(ad7949_adc->vref);
+		if (ret != -ENODEV)
+			return ret;
+		/* unbuffered? */
+		ad7949_adc->vref = devm_regulator_get_optional(dev, "vref");
+		if (IS_ERR(ad7949_adc->vref)) {
+			ret = PTR_ERR(ad7949_adc->vref);
+			if (ret != -ENODEV)
+				return ret;
+			/* Internal then */
+			mode = AD7949_CFG_VAL_REF_INT_4096;
+		} else {
+			mode = AD7949_CFG_VAL_REF_EXT_TEMP;
+		}
+	} else {
+		mode = AD7949_CFG_VAL_REF_EXT_TEMP_BUF;
 	}
 
-	ret = regulator_enable(ad7949_adc->vref);
-	if (ret < 0) {
-		dev_err(dev, "fail to enable regulator\n");
-		return ret;
+	if (mode & AD7949_CFG_VAL_REF_EXTERNAL) {
+		ret = regulator_enable(ad7949_adc->vref);
+		if (ret < 0) {
+			dev_err(dev, "fail to enable regulator\n");
+			return ret;
+		}
+
+		ret = devm_add_action_or_reset(dev, ad7949_disable_reg,
+					       ad7949_adc->vref);
+		if (ret)
+			return ret;
+	}
+
+	ad7949_adc->channels = devm_kcalloc(dev, spec->num_channels,
+					    sizeof(*ad7949_adc->channels),
+					    GFP_KERNEL);
+	if (!ad7949_adc->channels) {
+		dev_err(dev, "unable to allocate ADC channels\n");
+		return -ENOMEM;
+	}
+
+	/* Initialize all channel structures */
+	for (i = 0; i < spec->num_channels; i++)
+		ad7949_adc->channels[i].refsel = mode;
+
+	/* Read channel specific information form the devicetree */
+	device_for_each_child_node(dev, child) {
+		ret = fwnode_property_read_u32(child, "reg", &i);
+		if (ret) {
+			dev_err(dev, "missing reg property in %pfw\n", child);
+			fwnode_handle_put(child);
+			return ret;
+		}
+

drivers/iio/adc/ad7949.c static int ad7949_spi_probe(struct spi_device *spi) · line 448

Quoted 8 lines

drivers/iio/adc/ad7949.c +8 -0

+		ad7949_chan = &ad7949_adc->channels[i];
+
+		ret = fwnode_property_read_u32(child, "adi,internal-ref-microvolt", &tmp);
+		if (ret < 0 && ret != -EINVAL) {
+			dev_err(dev, "invalid internal reference in %pfw\n", child);
+			fwnode_handle_put(child);
+			return ret;
+		}

So if we are using external voltage, then we'd not expect this to exist. ret != -EINVAL should deal with that. However, we then hit the following switch
and temp isn't set to an appropriate value so we get the error.

Am I missing something?

Quoted 33 lines

drivers/iio/adc/ad7949.c +13 -2

+
+		switch (tmp) {
+		case 2500000:
+			ad7949_chan->refsel = AD7949_CFG_VAL_REF_INT_2500;
+			break;
+		case 4096000:
+			ad7949_chan->refsel = AD7949_CFG_VAL_REF_INT_4096;
+			break;
+		default:
+			dev_err(dev, "unsupported internal voltage reference\n");
+			fwnode_handle_put(child);
+			return -EINVAL;
+		}
 	}
 
 	mutex_init(&ad7949_adc->lock);

@@ -373,7 +479,6 @@ static int ad7949_spi_probe(struct spi_device *spi)
 
 err:
 	mutex_destroy(&ad7949_adc->lock);
-	regulator_disable(ad7949_adc->vref);
 
 	return ret;
 }

@@ -385,7 +490,6 @@ static int ad7949_spi_remove(struct spi_device *spi)
 
 	iio_device_unregister(indio_dev);
 	mutex_destroy(&ad7949_adc->lock);
-	regulator_disable(ad7949_adc->vref);
 
 	return 0;
 }

+

Quoted 82 lines

Quoted 82 lines

static int ad7949_spi_probe(struct spi_device *spi) {

drivers/iio/adc/ad7949.c +57 -7

 	u32 spi_ctrl_mask = spi->controller->bits_per_word_mask;
 	struct device *dev = &spi->dev;
 	const struct ad7949_adc_spec *spec;
 	struct ad7949_adc_chip *ad7949_adc;
+	struct ad7949_channel *ad7949_chan;
+	struct fwnode_handle *child;
 	struct iio_dev *indio_dev;
+	int mode;
+	u32 tmp;
 	int ret;
+	int i;
 
 	indio_dev = devm_iio_device_alloc(dev, sizeof(*ad7949_adc));
 	if (!indio_dev) {

@@ -343,16 +383,82 @@ static int ad7949_spi_probe(struct spi_device *spi)
 		return -EINVAL;
 	}
 
-	ad7949_adc->vref = devm_regulator_get(dev, "vref");
+	/* Setup external voltage ref, buffered? */
+	ad7949_adc->vref = devm_regulator_get_optional(dev, "vrefin");
 	if (IS_ERR(ad7949_adc->vref)) {
-		dev_err(dev, "fail to request regulator\n");
-		return PTR_ERR(ad7949_adc->vref);
+		ret = PTR_ERR(ad7949_adc->vref);
+		if (ret != -ENODEV)
+			return ret;
+		/* unbuffered? */
+		ad7949_adc->vref = devm_regulator_get_optional(dev, "vref");
+		if (IS_ERR(ad7949_adc->vref)) {
+			ret = PTR_ERR(ad7949_adc->vref);
+			if (ret != -ENODEV)
+				return ret;
+			/* Internal then */
+			mode = AD7949_CFG_VAL_REF_INT_4096;
+		} else {
+			mode = AD7949_CFG_VAL_REF_EXT_TEMP;
+		}
+	} else {
+		mode = AD7949_CFG_VAL_REF_EXT_TEMP_BUF;
 	}
 
-	ret = regulator_enable(ad7949_adc->vref);
-	if (ret < 0) {
-		dev_err(dev, "fail to enable regulator\n");
-		return ret;
+	if (mode & AD7949_CFG_VAL_REF_EXTERNAL) {
+		ret = regulator_enable(ad7949_adc->vref);
+		if (ret < 0) {
+			dev_err(dev, "fail to enable regulator\n");
+			return ret;
+		}
+
+		ret = devm_add_action_or_reset(dev, ad7949_disable_reg,
+					       ad7949_adc->vref);
+		if (ret)
+			return ret;
+	}
+
+	ad7949_adc->channels = devm_kcalloc(dev, spec->num_channels,
+					    sizeof(*ad7949_adc->channels),
+					    GFP_KERNEL);
+	if (!ad7949_adc->channels) {
+		dev_err(dev, "unable to allocate ADC channels\n");
+		return -ENOMEM;
+	}
+
+	/* Initialize all channel structures */
+	for (i = 0; i < spec->num_channels; i++)
+		ad7949_adc->channels[i].refsel = mode;
+
+	/* Read channel specific information form the devicetree */
+	device_for_each_child_node(dev, child) {
+		ret = fwnode_property_read_u32(child, "reg", &i);
+		if (ret) {
+			dev_err(dev, "missing reg property in %pfw\n", child);
+			fwnode_handle_put(child);
+			return ret;
+		}
+

Quoted 8 lines

drivers/iio/adc/ad7949.c static int ad7949_spi_probe(struct spi_device *spi) · line 448

Quoted 8 lines

drivers/iio/adc/ad7949.c +8 -0

+		ad7949_chan = &ad7949_adc->channels[i];
+
+		ret = fwnode_property_read_u32(child, "adi,internal-ref-microvolt", &tmp);
+		if (ret < 0 && ret != -EINVAL) {
+			dev_err(dev, "invalid internal reference in %pfw\n", child);
+			fwnode_handle_put(child);
+			return ret;
+		}  

Hi Jonathan,

Quoted 7 lines

So if we are using external voltage, then we'd not expect this to exist. ret != -EINVAL should deal with that. However, we then hit the following switch
and temp isn't set to an appropriate value so we get the error.

Am I missing something?

You're right that using an external reference, will cause this to fail. My apologies, I've done a poor job of testing the last two revisions of this patch. I'll do better.

Since a regulator is also required when we're using an external source, I'll add a check for that. Something like this:

	ret = fwnode_property_read_u32(child, "adi,internal-ref-microvolt", &tmp);
	if (ret == -EINVAL && mode & AD7949_CFG_VAL_REF_EXTERNAL) {
		continue;
	} else if (ret < 0) {
		dev_err(dev, "invalid voltage reference in %pfw\n", child);
		fwnode_handle_put(child);
		return ret;
	}

Given that we can now have a different scale for each channel based on the vref source, should BIT(IIO_CHAN_INFO_SCALE) be moved to .info_mask_separate in the channel definition?

Thanks,
Liam

Quoted 33 lines

Quoted 33 lines

drivers/iio/adc/ad7949.c +13 -2

+
+		switch (tmp) {
+		case 2500000:
+			ad7949_chan->refsel = AD7949_CFG_VAL_REF_INT_2500;
+			break;
+		case 4096000:
+			ad7949_chan->refsel = AD7949_CFG_VAL_REF_INT_4096;
+			break;
+		default:
+			dev_err(dev, "unsupported internal voltage reference\n");
+			fwnode_handle_put(child);
+			return -EINVAL;
+		}
 	}
 
 	mutex_init(&ad7949_adc->lock);

@@ -373,7 +479,6 @@ static int ad7949_spi_probe(struct spi_device *spi)
 
 err:
 	mutex_destroy(&ad7949_adc->lock);
-	regulator_disable(ad7949_adc->vref);
 
 	return ret;
 }

@@ -385,7 +490,6 @@ static int ad7949_spi_remove(struct spi_device *spi)
 
 	iio_device_unregister(indio_dev);
 	mutex_destroy(&ad7949_adc->lock);
-	regulator_disable(ad7949_adc->vref);
 
 	return 0;
 }  

...

Quoted 109 lines

+

Quoted 82 lines

Quoted 82 lines

static int ad7949_spi_probe(struct spi_device *spi) {

drivers/iio/adc/ad7949.c +57 -7

 	u32 spi_ctrl_mask = spi->controller->bits_per_word_mask;
 	struct device *dev = &spi->dev;
 	const struct ad7949_adc_spec *spec;
 	struct ad7949_adc_chip *ad7949_adc;
+	struct ad7949_channel *ad7949_chan;
+	struct fwnode_handle *child;
 	struct iio_dev *indio_dev;
+	int mode;
+	u32 tmp;
 	int ret;
+	int i;
 
 	indio_dev = devm_iio_device_alloc(dev, sizeof(*ad7949_adc));
 	if (!indio_dev) {

@@ -343,16 +383,82 @@ static int ad7949_spi_probe(struct spi_device *spi)
 		return -EINVAL;
 	}
 
-	ad7949_adc->vref = devm_regulator_get(dev, "vref");
+	/* Setup external voltage ref, buffered? */
+	ad7949_adc->vref = devm_regulator_get_optional(dev, "vrefin");
 	if (IS_ERR(ad7949_adc->vref)) {
-		dev_err(dev, "fail to request regulator\n");
-		return PTR_ERR(ad7949_adc->vref);
+		ret = PTR_ERR(ad7949_adc->vref);
+		if (ret != -ENODEV)
+			return ret;
+		/* unbuffered? */
+		ad7949_adc->vref = devm_regulator_get_optional(dev, "vref");
+		if (IS_ERR(ad7949_adc->vref)) {
+			ret = PTR_ERR(ad7949_adc->vref);
+			if (ret != -ENODEV)
+				return ret;
+			/* Internal then */
+			mode = AD7949_CFG_VAL_REF_INT_4096;
+		} else {
+			mode = AD7949_CFG_VAL_REF_EXT_TEMP;
+		}
+	} else {
+		mode = AD7949_CFG_VAL_REF_EXT_TEMP_BUF;
 	}
 
-	ret = regulator_enable(ad7949_adc->vref);
-	if (ret < 0) {
-		dev_err(dev, "fail to enable regulator\n");
-		return ret;
+	if (mode & AD7949_CFG_VAL_REF_EXTERNAL) {
+		ret = regulator_enable(ad7949_adc->vref);
+		if (ret < 0) {
+			dev_err(dev, "fail to enable regulator\n");
+			return ret;
+		}
+
+		ret = devm_add_action_or_reset(dev, ad7949_disable_reg,
+					       ad7949_adc->vref);
+		if (ret)
+			return ret;
+	}
+
+	ad7949_adc->channels = devm_kcalloc(dev, spec->num_channels,
+					    sizeof(*ad7949_adc->channels),
+					    GFP_KERNEL);
+	if (!ad7949_adc->channels) {
+		dev_err(dev, "unable to allocate ADC channels\n");
+		return -ENOMEM;
+	}
+
+	/* Initialize all channel structures */
+	for (i = 0; i < spec->num_channels; i++)
+		ad7949_adc->channels[i].refsel = mode;
+
+	/* Read channel specific information form the devicetree */
+	device_for_each_child_node(dev, child) {
+		ret = fwnode_property_read_u32(child, "reg", &i);
+		if (ret) {
+			dev_err(dev, "missing reg property in %pfw\n", child);
+			fwnode_handle_put(child);
+			return ret;
+		}
+

Quoted 8 lines

drivers/iio/adc/ad7949.c static int ad7949_spi_probe(struct spi_device *spi) · line 448

Quoted 8 lines

drivers/iio/adc/ad7949.c +8 -0

+		ad7949_chan = &ad7949_adc->channels[i];
+
+		ret = fwnode_property_read_u32(child, "adi,internal-ref-microvolt", &tmp);
+		if (ret < 0 && ret != -EINVAL) {
+			dev_err(dev, "invalid internal reference in %pfw\n", child);
+			fwnode_handle_put(child);
+			return ret;
+		}  

Hi Jonathan,

Quoted 7 lines

So if we are using external voltage, then we'd not expect this to exist. ret != -EINVAL should deal with that. However, we then hit the following switch
and temp isn't set to an appropriate value so we get the error.

Am I missing something?

You're right that using an external reference, will cause this to fail. My apologies, I've done a poor job of testing the last two revisions of this patch. I'll do better.

Since a regulator is also required when we're using an external source, I'll add a check for that. Something like this:

Quoted 8 lines

	ret = fwnode_property_read_u32(child, "adi,internal-ref-microvolt", &tmp);
	if (ret == -EINVAL && mode & AD7949_CFG_VAL_REF_EXTERNAL) {
		continue;
	} else if (ret < 0) {
		dev_err(dev, "invalid voltage reference in %pfw\n", child);
		fwnode_handle_put(child);
		return ret;
	}

Makes sense.

Quoted 5 lines

Given that we can now have a different scale for each channel based on the vref source, should BIT(IIO_CHAN_INFO_SCALE) be moved to .info_mask_separate in the channel definition?

Yes, good point.

Hopefully no one will notice the ABI change *crosses fingers*.

Jonathan

Quoted 38 lines

Thanks,
Liam

Quoted 33 lines

Quoted 33 lines

drivers/iio/adc/ad7949.c +13 -2

+
+		switch (tmp) {
+		case 2500000:
+			ad7949_chan->refsel = AD7949_CFG_VAL_REF_INT_2500;
+			break;
+		case 4096000:
+			ad7949_chan->refsel = AD7949_CFG_VAL_REF_INT_4096;
+			break;
+		default:
+			dev_err(dev, "unsupported internal voltage reference\n");
+			fwnode_handle_put(child);
+			return -EINVAL;
+		}
 	}
 
 	mutex_init(&ad7949_adc->lock);

@@ -373,7 +479,6 @@ static int ad7949_spi_probe(struct spi_device *spi)
 
 err:
 	mutex_destroy(&ad7949_adc->lock);
-	regulator_disable(ad7949_adc->vref);
 
 	return ret;
 }

@@ -385,7 +490,6 @@ static int ad7949_spi_remove(struct spi_device *spi)
 
 	iio_device_unregister(indio_dev);
 	mutex_destroy(&ad7949_adc->lock);
-	regulator_disable(ad7949_adc->vref);
 
 	return 0;
 }  

On Mon Aug 9, 2021 at 3:42 PM EDT, Jonathan Cameron wrote:

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

+ /* Read channel specific information form the devicetree */

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

drivers/iio/adc/ad7949.c +11 -0

+       device_for_each_child_node(dev, child) {
+               ret = fwnode_property_read_u32(child, "reg", &i);
+               if (ret) {
+                       dev_err(dev, "missing reg property in %pfw\n", child);
+                       fwnode_handle_put(child);
+                       return ret;
+               }
+
+               ad7949_chan = &ad7949_adc->channels[i];
+
+               ret = fwnode_property_read_u32(child, "adi,internal-ref-microvolt", &tmp);

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

drivers/iio/adc/ad7949.c static int ad7949_spi_probe(struct spi_device *spi) · line 448

Quoted 5 lines

drivers/iio/adc/ad7949.c +5 -0

+               if (ret < 0 && ret != -EINVAL) {
+                       dev_err(dev, "invalid internal reference in %pfw\n", child);
+                       fwnode_handle_put(child);
+                       return ret;
+               }

After getting access to another setup to run more tests, I noticed that setting the reference per channel isn't really feasible.

Quoted 1 line · Jonathan Cameron

On Sat, 7 Aug 2021 21:56:57 -0400

        ret = fwnode_property_read_u32(child, "adi,internal-ref-microvolt", &tmp);
        if (ret == -EINVAL && mode & AD7949_CFG_VAL_REF_EXTERNAL) {
                continue;

        } else if (ret < 0) {

                dev_err(dev, "invalid voltage reference in %pfw\n", child);
                fwnode_handle_put(child);
                return ret;
        }

Quoted 7 lines · Liam Beguin

Quoted 1 line · Jonathan Cameron

On Sat, 7 Aug 2021 21:56:57 -0400

        ret = fwnode_property_read_u32(child, "adi,internal-ref-microvolt", &tmp);
        if (ret == -EINVAL && mode & AD7949_CFG_VAL_REF_EXTERNAL) {
                continue;

        } else if (ret < 0) {

Side note, redundant 'else'

Quoted 5 lines

                dev_err(dev, "invalid voltage reference in %pfw\n", child);
                fwnode_handle_put(child);
                return ret;
        }

--
With Best Regards,
Andy Shevchenko

Quoted 7 lines · Andy Shevchenko

Quoted 5 lines · Liam Beguin

Quoted 1 line · Jonathan Cameron

On Sat, 7 Aug 2021 21:56:57 -0400

        ret = fwnode_property_read_u32(child, "adi,internal-ref-microvolt", &tmp);
        if (ret == -EINVAL && mode & AD7949_CFG_VAL_REF_EXTERNAL) {
                continue;

Quoted 3 lines

Quoted 1 line

        } else if (ret < 0) {

Side note, redundant 'else'

Are you asking to add an 'else' statement?

because, unless I'm mistaken, in this case ret can have other negative values that we want to catch with this 'else if'.

Quoted 4 lines

Quoted 4 lines

                dev_err(dev, "invalid voltage reference in %pfw\n", child);
                fwnode_handle_put(child);
                return ret;
        }

Quoted 12 lines · Liam Beguin

Quoted 10 lines · Andy Shevchenko

Quoted 7 lines · Liam Beguin

Quoted 1 line · Jonathan Cameron

On Sat, 7 Aug 2021 21:56:57 -0400

        ret = fwnode_property_read_u32(child, "adi,internal-ref-microvolt", &tmp);
        if (ret == -EINVAL && mode & AD7949_CFG_VAL_REF_EXTERNAL) {
                continue;

        } else if (ret < 0) {

Side note, redundant 'else'

Quoted 4 lines

Are you asking to add an 'else' statement?

because, unless I'm mistaken, in this case ret can have other negative values that we want to catch with this 'else if'.

You lost me, I have no idea what "to add" and "other" mean here. No, I asked to remove it. It's redundant.

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

                dev_err(dev, "invalid voltage reference in %pfw\n", child);
                fwnode_handle_put(child);
                return ret;
        }

--
With Best Regards,
Andy Shevchenko