| Message ID | 20210708235618.1541335-4-liambeguin@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Jonathan Cameron |
| Headers | show |
| Series | AD7949 Fixes | expand |
> From: Liam Beguin <liambeguin@gmail.com> > Sent: Friday, July 9, 2021 1:56 AM > To: liambeguin@gmail.com; lars@metafoo.de; Hennerich, Michael > <Michael.Hennerich@analog.com>; jic23@kernel.org; charles- > antoine.couret@essensium.com > Cc: linux-kernel@vger.kernel.org; linux-iio@vger.kernel.org; > devicetree@vger.kernel.org; robh+dt@kernel.org > Subject: [PATCH v1 3/4] iio: adc: ad7949: add support for internal vref > > [External] > > From: Liam Beguin <lvb@xiphos.com> > > 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 <lvb@xiphos.com> > --- > drivers/iio/adc/ad7949.c | 84 > +++++++++++++++++++++++++++++++++------- > 1 file changed, 69 insertions(+), 15 deletions(-) > > diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c > index bbc6b56330a3..3c1293922d2e 100644 > --- a/drivers/iio/adc/ad7949.c > +++ b/drivers/iio/adc/ad7949.c > @@ -31,6 +31,7 @@ > #define AD7949_CFG_VAL_BW_FULL 1 > #define AD7949_CFG_VAL_BW_QUARTER 0 > #define AD7949_CFG_BIT_REF GENMASK(5, 3) > +#define AD7949_CFG_VAL_REF_EXTERNAL BIT(1) > #define AD7949_CFG_BIT_SEQ GENMASK(2, 1) > #define AD7949_CFG_BIT_RBN BIT(0) > > @@ -40,6 +41,33 @@ enum { > ID_AD7689, > }; > > +/** > + * enum ad7949_ref - Reference selection > + * > + * AD7949_REF_INT_2500: Internal reference and temperature > sensor enabled. > + * Vref=2.5V, buffered output > + * AD7949_REF_INT_4096: Internal reference and temperature > sensor enabled. > + * Vref=4.096V, buffered output > + * AD7949_REF_EXT_TEMP: Use external reference, temperature > sensor enabled. > + * Internal buffer disabled > + * AD7949_REF_EXT_TEMP_BUF: Use external reference, internal > buffer and > + * temperature sensor enabled. > + * AD7949_REF_RSRV_4: Do not use > + * AD7949_REF_RSRV_5: Do not use > + * AD7949_REF_EXT: Use external reference, internal buffer and > + * temperature sensor disabled. > + * AD7949_REF_EXT_BUF: Use external reference, internal buffer > enabled. > + * Internal reference and temperature sensor disabled. > + */ > +enum ad7949_ref { > + AD7949_REF_INT_2500 = 0, > + AD7949_REF_INT_4096, > + AD7949_REF_EXT_TEMP, > + AD7949_REF_EXT_TEMP_BUF, > + AD7949_REF_EXT = 6, > + AD7949_REF_EXT_BUF, > +}; > + > struct ad7949_adc_spec { > u8 num_channels; > u8 resolution; > @@ -55,6 +83,7 @@ static const struct ad7949_adc_spec > ad7949_adc_spec[] = { > * struct ad7949_adc_chip - AD ADC chip > * @lock: protects write sequences > * @vref: regulator generating Vref > + * @refsel: reference selection > * @indio_dev: reference to iio structure > * @spi: reference to spi structure > * @resolution: resolution of the chip > @@ -66,6 +95,7 @@ static const struct ad7949_adc_spec > ad7949_adc_spec[] = { > struct ad7949_adc_chip { > struct mutex lock; > struct regulator *vref; > + enum ad7949_ref refsel; > struct iio_dev *indio_dev; > struct spi_device *spi; > u8 resolution; > @@ -241,12 +271,28 @@ 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_REF_INT_2500: > + *val = 2500; > + break; > + case AD7949_REF_INT_4096: > + *val = 4096; > + break; > + case AD7949_REF_EXT_TEMP: > + case AD7949_REF_EXT_TEMP_BUF: > + case AD7949_REF_EXT: > + case AD7949_REF_EXT_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; > @@ -285,7 +331,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, > AD7949_CFG_VAL_BW_FULL) | > - FIELD_PREP(AD7949_CFG_BIT_REF, > AD7949_REF_EXT_BUF) | > + FIELD_PREP(AD7949_CFG_BIT_REF, ad7949_adc- > >refsel) | > FIELD_PREP(AD7949_CFG_BIT_SEQ, 0x0) | > FIELD_PREP(AD7949_CFG_BIT_RBN, 1); > > @@ -304,6 +350,7 @@ static int ad7949_spi_init(struct > ad7949_adc_chip *ad7949_adc) > static int ad7949_spi_probe(struct spi_device *spi) > { > struct device *dev = &spi->dev; > + struct device_node *np = dev->of_node; > const struct ad7949_adc_spec *spec; > struct ad7949_adc_chip *ad7949_adc; > struct iio_dev *indio_dev; > @@ -315,6 +362,7 @@ static int ad7949_spi_probe(struct spi_device > *spi) > return -ENOMEM; > } > > + indio_dev->dev.of_node = np; Why doing this? The IIO core will do it for us at register time. > indio_dev->info = &ad7949_spi_info; > indio_dev->name = spi_get_device_id(spi)->name; > indio_dev->modes = INDIO_DIRECT_MODE; > @@ -330,16 +378,22 @@ static int ad7949_spi_probe(struct spi_device > *spi) > ad7949_adc->resolution = spec->resolution; > ad7949_set_bits_per_word(ad7949_adc); > > - ad7949_adc->vref = devm_regulator_get(dev, "vref"); > - if (IS_ERR(ad7949_adc->vref)) { > - dev_err(dev, "fail to request regulator\n"); > - return PTR_ERR(ad7949_adc->vref); > - } > + /* Set default devicetree parameters */ > + ad7949_adc->refsel = AD7949_REF_EXT_BUF; > + of_property_read_u32(np, "adi,reference-select", > &ad7949_adc->refsel); In case the property is given, we should make sure we get a valid value and error out if not... - Nuno Sá > - 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) { > + ad7949_adc->vref = devm_regulator_get(dev, "vref"); > + if (IS_ERR(ad7949_adc->vref)) { > + dev_err(dev, "fail to request regulator\n"); > + return PTR_ERR(ad7949_adc->vref); > + } > + > + ret = regulator_enable(ad7949_adc->vref); > + if (ret < 0) { > + dev_err(dev, "fail to enable regulator\n"); > + return ret; > + } > } > > mutex_init(&ad7949_adc->lock); > -- > 2.30.1.489.g328c10930387
On Fri Jul 9, 2021 at 4:23 AM EDT, Sa, Nuno wrote: > > From: Liam Beguin <liambeguin@gmail.com> > > Sent: Friday, July 9, 2021 1:56 AM > > To: liambeguin@gmail.com; lars@metafoo.de; Hennerich, Michael > > <Michael.Hennerich@analog.com>; jic23@kernel.org; charles- > > antoine.couret@essensium.com > > Cc: linux-kernel@vger.kernel.org; linux-iio@vger.kernel.org; > > devicetree@vger.kernel.org; robh+dt@kernel.org > > Subject: [PATCH v1 3/4] iio: adc: ad7949: add support for internal vref > > > > [External] > > > > From: Liam Beguin <lvb@xiphos.com> > > > > 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 <lvb@xiphos.com> > > --- > > drivers/iio/adc/ad7949.c | 84 > > +++++++++++++++++++++++++++++++++------- > > 1 file changed, 69 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c > > index bbc6b56330a3..3c1293922d2e 100644 > > --- a/drivers/iio/adc/ad7949.c > > +++ b/drivers/iio/adc/ad7949.c > > @@ -31,6 +31,7 @@ > > #define AD7949_CFG_VAL_BW_FULL 1 > > #define AD7949_CFG_VAL_BW_QUARTER 0 > > #define AD7949_CFG_BIT_REF GENMASK(5, 3) > > +#define AD7949_CFG_VAL_REF_EXTERNAL BIT(1) > > #define AD7949_CFG_BIT_SEQ GENMASK(2, 1) > > #define AD7949_CFG_BIT_RBN BIT(0) > > > > @@ -40,6 +41,33 @@ enum { > > ID_AD7689, > > }; > > > > +/** > > + * enum ad7949_ref - Reference selection > > + * > > + * AD7949_REF_INT_2500: Internal reference and temperature > > sensor enabled. > > + * Vref=2.5V, buffered output > > + * AD7949_REF_INT_4096: Internal reference and temperature > > sensor enabled. > > + * Vref=4.096V, buffered output > > + * AD7949_REF_EXT_TEMP: Use external reference, temperature > > sensor enabled. > > + * Internal buffer disabled > > + * AD7949_REF_EXT_TEMP_BUF: Use external reference, internal > > buffer and > > + * temperature sensor enabled. > > + * AD7949_REF_RSRV_4: Do not use > > + * AD7949_REF_RSRV_5: Do not use > > + * AD7949_REF_EXT: Use external reference, internal buffer and > > + * temperature sensor disabled. > > + * AD7949_REF_EXT_BUF: Use external reference, internal buffer > > enabled. > > + * Internal reference and temperature sensor disabled. > > + */ > > +enum ad7949_ref { > > + AD7949_REF_INT_2500 = 0, > > + AD7949_REF_INT_4096, > > + AD7949_REF_EXT_TEMP, > > + AD7949_REF_EXT_TEMP_BUF, > > + AD7949_REF_EXT = 6, > > + AD7949_REF_EXT_BUF, > > +}; > > + > > struct ad7949_adc_spec { > > u8 num_channels; > > u8 resolution; > > @@ -55,6 +83,7 @@ static const struct ad7949_adc_spec > > ad7949_adc_spec[] = { > > * struct ad7949_adc_chip - AD ADC chip > > * @lock: protects write sequences > > * @vref: regulator generating Vref > > + * @refsel: reference selection > > * @indio_dev: reference to iio structure > > * @spi: reference to spi structure > > * @resolution: resolution of the chip > > @@ -66,6 +95,7 @@ static const struct ad7949_adc_spec > > ad7949_adc_spec[] = { > > struct ad7949_adc_chip { > > struct mutex lock; > > struct regulator *vref; > > + enum ad7949_ref refsel; > > struct iio_dev *indio_dev; > > struct spi_device *spi; > > u8 resolution; > > @@ -241,12 +271,28 @@ 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_REF_INT_2500: > > + *val = 2500; > > + break; > > + case AD7949_REF_INT_4096: > > + *val = 4096; > > + break; > > + case AD7949_REF_EXT_TEMP: > > + case AD7949_REF_EXT_TEMP_BUF: > > + case AD7949_REF_EXT: > > + case AD7949_REF_EXT_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; > > @@ -285,7 +331,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, > > AD7949_CFG_VAL_BW_FULL) | > > - FIELD_PREP(AD7949_CFG_BIT_REF, > > AD7949_REF_EXT_BUF) | > > + FIELD_PREP(AD7949_CFG_BIT_REF, ad7949_adc- > > >refsel) | > > FIELD_PREP(AD7949_CFG_BIT_SEQ, 0x0) | > > FIELD_PREP(AD7949_CFG_BIT_RBN, 1); > > > > @@ -304,6 +350,7 @@ static int ad7949_spi_init(struct > > ad7949_adc_chip *ad7949_adc) > > static int ad7949_spi_probe(struct spi_device *spi) > > { > > struct device *dev = &spi->dev; > > + struct device_node *np = dev->of_node; > > const struct ad7949_adc_spec *spec; > > struct ad7949_adc_chip *ad7949_adc; > > struct iio_dev *indio_dev; > > @@ -315,6 +362,7 @@ static int ad7949_spi_probe(struct spi_device > > *spi) > > return -ENOMEM; > > } > > > > + indio_dev->dev.of_node = np; > > Why doing this? The IIO core will do it for us at register time. Sorry about that, will drop. > > > indio_dev->info = &ad7949_spi_info; > > indio_dev->name = spi_get_device_id(spi)->name; > > indio_dev->modes = INDIO_DIRECT_MODE; > > @@ -330,16 +378,22 @@ static int ad7949_spi_probe(struct spi_device > > *spi) > > ad7949_adc->resolution = spec->resolution; > > ad7949_set_bits_per_word(ad7949_adc); > > > > - ad7949_adc->vref = devm_regulator_get(dev, "vref"); > > - if (IS_ERR(ad7949_adc->vref)) { > > - dev_err(dev, "fail to request regulator\n"); > > - return PTR_ERR(ad7949_adc->vref); > > - } > > + /* Set default devicetree parameters */ > > + ad7949_adc->refsel = AD7949_REF_EXT_BUF; > > + of_property_read_u32(np, "adi,reference-select", > > &ad7949_adc->refsel); > > In case the property is given, we should make sure we get a valid > value and error out if not... > I thought it would be handled later on when processing the sample, but it's not really and it's better to fail early. I'll add that to v2. Thanks, Liam > - Nuno Sá > > - 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) { > > + ad7949_adc->vref = devm_regulator_get(dev, "vref"); > > + if (IS_ERR(ad7949_adc->vref)) { > > + dev_err(dev, "fail to request regulator\n"); > > + return PTR_ERR(ad7949_adc->vref); > > + } > > + > > + ret = regulator_enable(ad7949_adc->vref); > > + if (ret < 0) { > > + dev_err(dev, "fail to enable regulator\n"); > > + return ret; > > + } > > } > > > > mutex_init(&ad7949_adc->lock); > > -- > > 2.30.1.489.g328c10930387
diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c index bbc6b56330a3..3c1293922d2e 100644 --- a/drivers/iio/adc/ad7949.c +++ b/drivers/iio/adc/ad7949.c @@ -31,6 +31,7 @@ #define AD7949_CFG_VAL_BW_FULL 1 #define AD7949_CFG_VAL_BW_QUARTER 0 #define AD7949_CFG_BIT_REF GENMASK(5, 3) +#define AD7949_CFG_VAL_REF_EXTERNAL BIT(1) #define AD7949_CFG_BIT_SEQ GENMASK(2, 1) #define AD7949_CFG_BIT_RBN BIT(0) @@ -40,6 +41,33 @@ enum { ID_AD7689, }; +/** + * enum ad7949_ref - Reference selection + * + * AD7949_REF_INT_2500: Internal reference and temperature sensor enabled. + * Vref=2.5V, buffered output + * AD7949_REF_INT_4096: Internal reference and temperature sensor enabled. + * Vref=4.096V, buffered output + * AD7949_REF_EXT_TEMP: Use external reference, temperature sensor enabled. + * Internal buffer disabled + * AD7949_REF_EXT_TEMP_BUF: Use external reference, internal buffer and + * temperature sensor enabled. + * AD7949_REF_RSRV_4: Do not use + * AD7949_REF_RSRV_5: Do not use + * AD7949_REF_EXT: Use external reference, internal buffer and + * temperature sensor disabled. + * AD7949_REF_EXT_BUF: Use external reference, internal buffer enabled. + * Internal reference and temperature sensor disabled. + */ +enum ad7949_ref { + AD7949_REF_INT_2500 = 0, + AD7949_REF_INT_4096, + AD7949_REF_EXT_TEMP, + AD7949_REF_EXT_TEMP_BUF, + AD7949_REF_EXT = 6, + AD7949_REF_EXT_BUF, +}; + struct ad7949_adc_spec { u8 num_channels; u8 resolution; @@ -55,6 +83,7 @@ static const struct ad7949_adc_spec ad7949_adc_spec[] = { * struct ad7949_adc_chip - AD ADC chip * @lock: protects write sequences * @vref: regulator generating Vref + * @refsel: reference selection * @indio_dev: reference to iio structure * @spi: reference to spi structure * @resolution: resolution of the chip @@ -66,6 +95,7 @@ static const struct ad7949_adc_spec ad7949_adc_spec[] = { struct ad7949_adc_chip { struct mutex lock; struct regulator *vref; + enum ad7949_ref refsel; struct iio_dev *indio_dev; struct spi_device *spi; u8 resolution; @@ -241,12 +271,28 @@ 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_REF_INT_2500: + *val = 2500; + break; + case AD7949_REF_INT_4096: + *val = 4096; + break; + case AD7949_REF_EXT_TEMP: + case AD7949_REF_EXT_TEMP_BUF: + case AD7949_REF_EXT: + case AD7949_REF_EXT_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; @@ -285,7 +331,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, AD7949_CFG_VAL_BW_FULL) | - FIELD_PREP(AD7949_CFG_BIT_REF, AD7949_REF_EXT_BUF) | + FIELD_PREP(AD7949_CFG_BIT_REF, ad7949_adc->refsel) | FIELD_PREP(AD7949_CFG_BIT_SEQ, 0x0) | FIELD_PREP(AD7949_CFG_BIT_RBN, 1); @@ -304,6 +350,7 @@ static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc) static int ad7949_spi_probe(struct spi_device *spi) { struct device *dev = &spi->dev; + struct device_node *np = dev->of_node; const struct ad7949_adc_spec *spec; struct ad7949_adc_chip *ad7949_adc; struct iio_dev *indio_dev; @@ -315,6 +362,7 @@ static int ad7949_spi_probe(struct spi_device *spi) return -ENOMEM; } + indio_dev->dev.of_node = np; indio_dev->info = &ad7949_spi_info; indio_dev->name = spi_get_device_id(spi)->name; indio_dev->modes = INDIO_DIRECT_MODE; @@ -330,16 +378,22 @@ static int ad7949_spi_probe(struct spi_device *spi) ad7949_adc->resolution = spec->resolution; ad7949_set_bits_per_word(ad7949_adc); - ad7949_adc->vref = devm_regulator_get(dev, "vref"); - if (IS_ERR(ad7949_adc->vref)) { - dev_err(dev, "fail to request regulator\n"); - return PTR_ERR(ad7949_adc->vref); - } + /* Set default devicetree parameters */ + ad7949_adc->refsel = AD7949_REF_EXT_BUF; + of_property_read_u32(np, "adi,reference-select", &ad7949_adc->refsel); - 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) { + ad7949_adc->vref = devm_regulator_get(dev, "vref"); + if (IS_ERR(ad7949_adc->vref)) { + dev_err(dev, "fail to request regulator\n"); + return PTR_ERR(ad7949_adc->vref); + } + + ret = regulator_enable(ad7949_adc->vref); + if (ret < 0) { + dev_err(dev, "fail to enable regulator\n"); + return ret; + } } mutex_init(&ad7949_adc->lock);