diff mbox series

[v6,3/5] iio: adc: ad7949: add vref selection support

Message ID 20210815213309.2847711-4-liambeguin@gmail.com (mailing list archive)
State Accepted
Headers show
Series AD7949 Fixes | expand

Commit Message

Liam Beguin Aug. 15, 2021, 9:33 p.m. UTC 
From: Liam Beguin <lvb@xiphos.com>

Add support for selecting the voltage reference from the devicetree.

This change is required to get valid readings with all three
vref hardware configurations supported by the ADC.

For instance if the ADC isn't provided with an external reference,
the sample request must specify an internal voltage reference to get a
valid reading.

Signed-off-by: Liam Beguin <lvb@xiphos.com>
---
 drivers/iio/adc/ad7949.c | 96 +++++++++++++++++++++++++++++++++-------
 1 file changed, 80 insertions(+), 16 deletions(-)

Comments

Andy Shevchenko Aug. 16, 2021, 8:04 a.m. UTC | #1 
On Mon, Aug 16, 2021 at 12:35 AM Liam Beguin <liambeguin@gmail.com> wrote:
>
> From: Liam Beguin <lvb@xiphos.com>
>
> Add support for selecting the voltage reference from the devicetree.
>
> This change is required to get valid readings with all three
> vref hardware configurations supported by the ADC.
>
> For instance if the ADC isn't provided with an external reference,
> the sample request must specify an internal voltage reference to get a
> valid reading.

...

> +       /* Setup internal voltage reference */
> +       tmp = 4096000;
> +       ret = device_property_read_u32(dev, "adi,internal-ref-microvolt", &tmp);

> +       if (ret < 0 && ret != -EINVAL) {

What does this check (second part) is supposed to mean?
The first part will make it mandatory, is it the goal?

> +               dev_err(dev, "invalid value for adi,internal-ref-microvolt\n");
> +               return ret;
> +       }
Liam Beguin Aug. 16, 2021, 12:39 p.m. UTC | #2 
On Mon Aug 16, 2021 at 4:04 AM EDT, Andy Shevchenko wrote:
> On Mon, Aug 16, 2021 at 12:35 AM Liam Beguin <liambeguin@gmail.com>
> wrote:
> >
> > From: Liam Beguin <lvb@xiphos.com>
> >
> > Add support for selecting the voltage reference from the devicetree.
> >
> > This change is required to get valid readings with all three
> > vref hardware configurations supported by the ADC.
> >
> > For instance if the ADC isn't provided with an external reference,
> > the sample request must specify an internal voltage reference to get a
> > valid reading.
>
> ...
>
> > +       /* Setup internal voltage reference */
> > +       tmp = 4096000;
> > +       ret = device_property_read_u32(dev, "adi,internal-ref-microvolt", &tmp);
>
> > +       if (ret < 0 && ret != -EINVAL) {

Hi Andy,

>
> What does this check (second part) is supposed to mean?
> The first part will make it mandatory, is it the goal?
>

device_property_read_u32() will return -EINVAL if the property isn't
found in the devicetree.

This checks for errors when the property is defined while keeping it
optional.

Liam

> > +               dev_err(dev, "invalid value for adi,internal-ref-microvolt\n");
> > +               return ret;
> > +       }
>
> --
> With Best Regards,
> Andy Shevchenko
Andy Shevchenko Aug. 16, 2021, 12:48 p.m. UTC | #3 
On Mon, Aug 16, 2021 at 3:39 PM Liam Beguin <liambeguin@gmail.com> wrote:
> On Mon Aug 16, 2021 at 4:04 AM EDT, Andy Shevchenko wrote:
> > On Mon, Aug 16, 2021 at 12:35 AM Liam Beguin <liambeguin@gmail.com>
> > wrote:

...

> > > +       tmp = 4096000;
> > > +       ret = device_property_read_u32(dev, "adi,internal-ref-microvolt", &tmp);
> >
> > > +       if (ret < 0 && ret != -EINVAL) {
>
> Hi Andy,
>
> >
> > What does this check (second part) is supposed to mean?
> > The first part will make it mandatory, is it the goal?
> >
>
> device_property_read_u32() will return -EINVAL if the property isn't
> found in the devicetree.
>
> This checks for errors when the property is defined while keeping it
> optional.

Don't assign and don't check the error code of the API. As simply as that.

> > > +               dev_err(dev, "invalid value for adi,internal-ref-microvolt\n");
> > > +               return ret;
> > > +       }
Liam Beguin Aug. 16, 2021, 1:07 p.m. UTC | #4 
On Mon Aug 16, 2021 at 8:48 AM EDT, Andy Shevchenko wrote:
> On Mon, Aug 16, 2021 at 3:39 PM Liam Beguin <liambeguin@gmail.com>
> wrote:
> > On Mon Aug 16, 2021 at 4:04 AM EDT, Andy Shevchenko wrote:
> > > On Mon, Aug 16, 2021 at 12:35 AM Liam Beguin <liambeguin@gmail.com>
> > > wrote:
>
> ...
>
> > > > +       tmp = 4096000;
> > > > +       ret = device_property_read_u32(dev, "adi,internal-ref-microvolt", &tmp);
> > >
> > > > +       if (ret < 0 && ret != -EINVAL) {
> >
> > Hi Andy,
> >
> > >
> > > What does this check (second part) is supposed to mean?
> > > The first part will make it mandatory, is it the goal?
> > >
> >
> > device_property_read_u32() will return -EINVAL if the property isn't
> > found in the devicetree.
> >
> > This checks for errors when the property is defined while keeping it
> > optional.
>
> Don't assign and don't check the error code of the API. As simply as
> that.

I'm not against getting rid of it, but I was asked to check for these
errors in earlier revisions of the patch.

Liam

>
> > > > +               dev_err(dev, "invalid value for adi,internal-ref-microvolt\n");
> > > > +               return ret;
> > > > +       }
>
>
> --
> With Best Regards,
> Andy Shevchenko
Andy Shevchenko Aug. 16, 2021, 1:12 p.m. UTC | #5 
On Mon, Aug 16, 2021 at 4:07 PM Liam Beguin <liambeguin@gmail.com> wrote:
> On Mon Aug 16, 2021 at 8:48 AM EDT, Andy Shevchenko wrote:
> > On Mon, Aug 16, 2021 at 3:39 PM Liam Beguin <liambeguin@gmail.com>
> > wrote:
> > > On Mon Aug 16, 2021 at 4:04 AM EDT, Andy Shevchenko wrote:
> > > > On Mon, Aug 16, 2021 at 12:35 AM Liam Beguin <liambeguin@gmail.com>
> > > > wrote:

...

> > > > > +       tmp = 4096000;
> > > > > +       ret = device_property_read_u32(dev, "adi,internal-ref-microvolt", &tmp);
> > > >
> > > > > +       if (ret < 0 && ret != -EINVAL) {
> > >
> > > Hi Andy,
> > >
> > > >
> > > > What does this check (second part) is supposed to mean?
> > > > The first part will make it mandatory, is it the goal?
> > > >
> > >
> > > device_property_read_u32() will return -EINVAL if the property isn't
> > > found in the devicetree.
> > >
> > > This checks for errors when the property is defined while keeping it
> > > optional.
> >
> > Don't assign and don't check the error code of the API. As simply as
> > that.
>
> I'm not against getting rid of it, but I was asked to check for these
> errors in earlier revisions of the patch.

Okay, I leave it to you, guys, to decide, just note that the usual
pattern for optional stuff
a) either check for (!ret);
b) or ignore the returned value completely.

> > > > > +               dev_err(dev, "invalid value for adi,internal-ref-microvolt\n");
> > > > > +               return ret;
> > > > > +       }
Jonathan Cameron Aug. 29, 2021, 2:35 p.m. UTC | #6 
On Mon, 16 Aug 2021 16:12:58 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Mon, Aug 16, 2021 at 4:07 PM Liam Beguin <liambeguin@gmail.com> wrote:
> > On Mon Aug 16, 2021 at 8:48 AM EDT, Andy Shevchenko wrote:  
> > > On Mon, Aug 16, 2021 at 3:39 PM Liam Beguin <liambeguin@gmail.com>
> > > wrote:  
> > > > On Mon Aug 16, 2021 at 4:04 AM EDT, Andy Shevchenko wrote:  
> > > > > On Mon, Aug 16, 2021 at 12:35 AM Liam Beguin <liambeguin@gmail.com>
> > > > > wrote:  
> 
> ...
> 
> > > > > > +       tmp = 4096000;
> > > > > > +       ret = device_property_read_u32(dev, "adi,internal-ref-microvolt", &tmp);  
> > > > >  
> > > > > > +       if (ret < 0 && ret != -EINVAL) {  
> > > >
> > > > Hi Andy,
> > > >  
> > > > >
> > > > > What does this check (second part) is supposed to mean?
> > > > > The first part will make it mandatory, is it the goal?
> > > > >  
> > > >
> > > > device_property_read_u32() will return -EINVAL if the property isn't
> > > > found in the devicetree.
> > > >
> > > > This checks for errors when the property is defined while keeping it
> > > > optional.  
> > >
> > > Don't assign and don't check the error code of the API. As simply as
> > > that.  
> >
> > I'm not against getting rid of it, but I was asked to check for these
> > errors in earlier revisions of the patch.  
> 
> Okay, I leave it to you, guys, to decide, just note that the usual
> pattern for optional stuff
> a) either check for (!ret);
> b) or ignore the returned value completely.

Hmm. My thinking (I suspect I asked for it to be checked, but can't remember :)
was that I'd really like to know if a device tree contains a property but that
property is invalid for some reason. The docs give a bunch of reasons beyond
the property not existing (which is unhelpfully described as just 'invalid parameters'). 

I guess that's a bit far fetched.  Let's drop the check as Andy suggests.

Dropped that check and applied to the togreg branch of iio.git, initially pushed out
as testing for 0-day to poke at it.  + we are about to enter merge window so I don't
want linux-next to pick it up just yet!

Jonathan

> 
> > > > > > +               dev_err(dev, "invalid value for adi,internal-ref-microvolt\n");
> > > > > > +               return ret;
> > > > > > +       }  
> 
>
Liam Beguin Aug. 29, 2021, 4:40 p.m. UTC | #7 
On Sun, Aug 29, 2021 at 03:35:39PM +0100, Jonathan Cameron wrote:
> On Mon, 16 Aug 2021 16:12:58 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> 
> > On Mon, Aug 16, 2021 at 4:07 PM Liam Beguin <liambeguin@gmail.com> wrote:
> > > On Mon Aug 16, 2021 at 8:48 AM EDT, Andy Shevchenko wrote:  
> > > > On Mon, Aug 16, 2021 at 3:39 PM Liam Beguin <liambeguin@gmail.com>
> > > > wrote:  
> > > > > On Mon Aug 16, 2021 at 4:04 AM EDT, Andy Shevchenko wrote:  
> > > > > > On Mon, Aug 16, 2021 at 12:35 AM Liam Beguin <liambeguin@gmail.com>
> > > > > > wrote:  
> > 
> > ...
> > 
> > > > > > > +       tmp = 4096000;
> > > > > > > +       ret = device_property_read_u32(dev, "adi,internal-ref-microvolt", &tmp);  
> > > > > >  
> > > > > > > +       if (ret < 0 && ret != -EINVAL) {  
> > > > >
> > > > > Hi Andy,
> > > > >  
> > > > > >
> > > > > > What does this check (second part) is supposed to mean?
> > > > > > The first part will make it mandatory, is it the goal?
> > > > > >  
> > > > >
> > > > > device_property_read_u32() will return -EINVAL if the property isn't
> > > > > found in the devicetree.
> > > > >
> > > > > This checks for errors when the property is defined while keeping it
> > > > > optional.  
> > > >
> > > > Don't assign and don't check the error code of the API. As simply as
> > > > that.  
> > >
> > > I'm not against getting rid of it, but I was asked to check for these
> > > errors in earlier revisions of the patch.  
> > 
> > Okay, I leave it to you, guys, to decide, just note that the usual
> > pattern for optional stuff
> > a) either check for (!ret);
> > b) or ignore the returned value completely.
> 

Hi Jonathan,

> Hmm. My thinking (I suspect I asked for it to be checked, but can't remember :)
> was that I'd really like to know if a device tree contains a property but that
> property is invalid for some reason. The docs give a bunch of reasons beyond
> the property not existing (which is unhelpfully described as just 'invalid parameters'). 
> 
> I guess that's a bit far fetched.  Let's drop the check as Andy suggests.
> 

Understood, Thanks for making the change.

Liam

> Dropped that check and applied to the togreg branch of iio.git, initially pushed out
> as testing for 0-day to poke at it.  + we are about to enter merge window so I don't
> want linux-next to pick it up just yet!
> 
> Jonathan
> 
> > 
> > > > > > > +               dev_err(dev, "invalid value for adi,internal-ref-microvolt\n");
> > > > > > > +               return ret;
> > > > > > > +       }  
> > 
> > 
>
diff mbox series

Patch

diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
index a263d0fcec75..5168d687687d 100644
--- a/drivers/iio/adc/ad7949.c
+++ b/drivers/iio/adc/ad7949.c
@@ -35,7 +35,11 @@ 
 
 /* REF: reference/buffer selection */
 #define AD7949_CFG_MASK_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_MASK_SEQ		GENMASK(2, 1)
@@ -66,6 +70,7 @@  static const struct ad7949_adc_spec ad7949_adc_spec[] = {
  * @vref: regulator generating Vref
  * @indio_dev: reference to iio structure
  * @spi: reference to spi structure
+ * @refsel: reference selection
  * @resolution: resolution of the chip
  * @cfg: copy of the configuration register
  * @current_channel: current channel in use
@@ -77,6 +82,7 @@  struct ad7949_adc_chip {
 	struct regulator *vref;
 	struct iio_dev *indio_dev;
 	struct spi_device *spi;
+	u32 refsel;
 	u8 resolution;
 	u16 cfg;
 	unsigned int current_channel;
@@ -221,12 +227,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_adc->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;
@@ -265,7 +285,7 @@  static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
 		FIELD_PREP(AD7949_CFG_MASK_INCC, AD7949_CFG_VAL_INCC_UNIPOLAR_GND) |
 		FIELD_PREP(AD7949_CFG_MASK_INX, ad7949_adc->current_channel) |
 		FIELD_PREP(AD7949_CFG_MASK_BW_FULL, 1) |
-		FIELD_PREP(AD7949_CFG_MASK_REF, AD7949_CFG_VAL_REF_EXT_BUF) |
+		FIELD_PREP(AD7949_CFG_MASK_REF, ad7949_adc->refsel) |
 		FIELD_PREP(AD7949_CFG_MASK_SEQ, 0x0) |
 		FIELD_PREP(AD7949_CFG_MASK_RBN, 1);
 
@@ -281,6 +301,11 @@  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;
@@ -288,6 +313,7 @@  static int ad7949_spi_probe(struct spi_device *spi)
 	const struct ad7949_adc_spec *spec;
 	struct ad7949_adc_chip *ad7949_adc;
 	struct iio_dev *indio_dev;
+	u32 tmp;
 	int ret;
 
 	indio_dev = devm_iio_device_alloc(dev, sizeof(*ad7949_adc));
@@ -322,16 +348,56 @@  static int ad7949_spi_probe(struct spi_device *spi)
 		return -EINVAL;
 	}
 
-	ad7949_adc->vref = devm_regulator_get(dev, "vref");
+	/* Setup internal voltage reference */
+	tmp = 4096000;
+	ret = device_property_read_u32(dev, "adi,internal-ref-microvolt", &tmp);
+	if (ret < 0 && ret != -EINVAL) {
+		dev_err(dev, "invalid value for adi,internal-ref-microvolt\n");
+		return ret;
+	}
+
+	switch (tmp) {
+	case 2500000:
+		ad7949_adc->refsel = AD7949_CFG_VAL_REF_INT_2500;
+		break;
+	case 4096000:
+		ad7949_adc->refsel = AD7949_CFG_VAL_REF_INT_4096;
+		break;
+	default:
+		dev_err(dev, "unsupported internal voltage reference\n");
+		return -EINVAL;
+	}
+
+	/* Setup external voltage reference, 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;
+		} else {
+			ad7949_adc->refsel = AD7949_CFG_VAL_REF_EXT_TEMP;
+		}
+	} else {
+		ad7949_adc->refsel = 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 (ad7949_adc->refsel & 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;
 	}
 
 	mutex_init(&ad7949_adc->lock);
@@ -352,7 +418,6 @@  static int ad7949_spi_probe(struct spi_device *spi)
 
 err:
 	mutex_destroy(&ad7949_adc->lock);
-	regulator_disable(ad7949_adc->vref);
 
 	return ret;
 }
@@ -364,7 +429,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;
 }