RE: [PATCH v1 3/4] iio: adc: ad7949: add support for internal vref

From: Liam Beguin
Date: Fri Jul 09 2021 - 10:49:18 EST

Next message: Johannes Weiner: "Re: [PATCH 3/3] mm, memcg: inline swap-related functions to improve disabled memcg config"
Previous message: IMF: "NOTICE FROM OUR OFFICE."
In reply to: Sa, Nuno: "RE: [PATCH v1 3/4] iio: adc: ad7949: add support for internal vref"
Next in thread: Liam Beguin: "[PATCH v1 2/4] iio: adc: ad7949: fix spi messages on non 14-bit controllers"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Fri Jul 9, 2021 at 4:23 AM EDT, Sa, Nuno wrote:
> > From: Liam Beguin <liambeguin@xxxxxxxxx>
> > Sent: Friday, July 9, 2021 1:56 AM
> > To: liambeguin@xxxxxxxxx; lars@xxxxxxxxxx; Hennerich, Michael
> > <Michael.Hennerich@xxxxxxxxxx>; jic23@xxxxxxxxxx; charles-
> > antoine.couret@xxxxxxxxxxxxx
> > Cc: linux-kernel@xxxxxxxxxxxxxxx; linux-iio@xxxxxxxxxxxxxxx;
> > devicetree@xxxxxxxxxxxxxxx; robh+dt@xxxxxxxxxx
> > Subject: [PATCH v1 3/4] iio: adc: ad7949: add support for internal vref
> >
> > [External]
> >
> > From: Liam Beguin <lvb@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > ---
> > 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

Next message: Johannes Weiner: "Re: [PATCH 3/3] mm, memcg: inline swap-related functions to improve disabled memcg config"
Previous message: IMF: "NOTICE FROM OUR OFFICE."
In reply to: Sa, Nuno: "RE: [PATCH v1 3/4] iio: adc: ad7949: add support for internal vref"
Next in thread: Liam Beguin: "[PATCH v1 2/4] iio: adc: ad7949: fix spi messages on non 14-bit controllers"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]