Re: [PATCH v4 3/4] iio: adc: Add ad7124 support

From: Jonathan Cameron
Date: Sun Nov 11 2018 - 07:14:08 EST

Next message: Jernej Skrabec: "[PATCH] arm64: dts: allwinner: a64: Add mali node"
Previous message: David Sterba: "[GIT PULL] Btrfs fixes for 4.20-rc1"
In reply to: Stefan Popa: "[PATCH v4 3/4] iio: adc: Add ad7124 support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Fri, 9 Nov 2018 17:41:46 +0200
Stefan Popa <stefan.popa@xxxxxxxxxx> wrote:

> The ad7124-4 and ad7124-8 are a family of 4 and 8 channel sigma-delta ADCs
> with 24-bit precision and reference.
>
> Three power modes are available which in turn affect the output data rate:
> * Full power: 9.38 SPS to 19,200 SPS
> * Mid power: 2.34 SPS to 4800 SPS
> * Low power: 1.17 SPS to 2400 SPS
>
> The ad7124-4 can be configured to have four differential inputs, while
> ad7124-8 can have 8. Moreover, ad7124 also supports per channel
> configuration. Each configuration consists of gain, reference source,
> output data rate and bipolar/unipolar configuration.

One question around the offset.
Voltage = (raw value + offset) * scale.
So I think the offset should simply be half the raw value range, not dependent
on the current gain?

Perhaps I'm missing something.

The other thing that I think needs to be dropped is the use of hardware gain.
It was never intended for this use case and means we effectively have two
interfaces for the same thing. Scale and hardwaregain.

Otherwise, looking very nice.

Jonathan

>
> Datasheets:
> Link: http://www.analog.com/media/en/technical-documentation/data-sheets/AD7124-4.pdf
> Link: http://www.analog.com/media/en/technical-documentation/data-sheets/ad7124-8.pdf
>
> Signed-off-by: Stefan Popa <stefan.popa@xxxxxxxxxx>
> ---
> Changes in v2:
> - Nothing changed.
> Changes in v3:
> - Removed channel, address, scan_index and shift fields from
> ad7124_channel_template.
> - Added a sanity check for val2 in ad7124_write_raw().
> - Used the "reg" property to get the channel address and "adi,diff-channels"
> for the differential pins. The "adi,channel-number" property was removed.
> - When calling regulator_get_optional, the probe is given up in case of error,
> but continues in case of -ENODEV.
> - clk_disable_unprepare() is called before ad_sd_cleanup_buffer_and_trigger
> in ad7124_remove().
> Changes in v4:
> - Added the .shift and .endianness fields as part of the ad7124_channel_template.
> - Made the gain configurable from the user space.
> - Removed the odr_hz and gain properties from the DT.
> - Used the bipolar and diff-channels properties defined in the new adc.txt doc.
> - Misc style fixes.
>
> MAINTAINERS | 7 +
> drivers/iio/adc/Kconfig | 11 +
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/ad7124.c | 676 +++++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 695 insertions(+)
> create mode 100644 drivers/iio/adc/ad7124.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f642044..3a1bfcb 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -839,6 +839,13 @@ S: Supported
> F: drivers/iio/dac/ad5758.c
> F: Documentation/devicetree/bindings/iio/dac/ad5758.txt
>
> +ANALOG DEVICES INC AD7124 DRIVER
> +M: Stefan Popa <stefan.popa@xxxxxxxxxx>
> +L: linux-iio@xxxxxxxxxxxxxxx
> +W: http://ez.analog.com/community/linux-device-drivers
> +S: Supported
> +F: drivers/iio/adc/ad7124.c
> +
> ANALOG DEVICES INC AD9389B DRIVER
> M: Hans Verkuil <hans.verkuil@xxxxxxxxx>
> L: linux-media@xxxxxxxxxxxxxxx
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index a52fea8..148a10f 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -10,6 +10,17 @@ config AD_SIGMA_DELTA
> select IIO_BUFFER
> select IIO_TRIGGERED_BUFFER
>
> +config AD7124
> + tristate "Analog Devices AD7124 and similar sigma-delta ADCs driver"
> + depends on SPI_MASTER
> + select AD_SIGMA_DELTA
> + help
> + Say yes here to build support for Analog Devices AD7124-4 and AD7124-8
> + SPI analog to digital converters (ADC).
> +
> + To compile this driver as a module, choose M here: the module will be
> + called ad7124.
> +
> config AD7266
> tristate "Analog Devices AD7265/AD7266 ADC driver"
> depends on SPI_MASTER
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index a6e6a0b..76168b2 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -5,6 +5,7 @@
>
> # When adding new entries keep the list in alphabetical order
> obj-$(CONFIG_AD_SIGMA_DELTA) += ad_sigma_delta.o
> +obj-$(CONFIG_AD7124) += ad7124.o
> obj-$(CONFIG_AD7266) += ad7266.o
> obj-$(CONFIG_AD7291) += ad7291.o
> obj-$(CONFIG_AD7298) += ad7298.o
> diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
> new file mode 100644
> index 0000000..64d2aa7
> --- /dev/null
> +++ b/drivers/iio/adc/ad7124.c
> @@ -0,0 +1,676 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * AD7124 SPI ADC driver
> + *
> + * Copyright 2018 Analog Devices Inc.
> + */
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/adc/ad_sigma_delta.h>
> +#include <linux/iio/sysfs.h>
> +
> +/* AD7124 registers */
> +#define AD7124_COMMS 0x00
> +#define AD7124_STATUS 0x00
> +#define AD7124_ADC_CONTROL 0x01
> +#define AD7124_DATA 0x02
> +#define AD7124_IO_CONTROL_1 0x03
> +#define AD7124_IO_CONTROL_2 0x04
> +#define AD7124_ID 0x05
> +#define AD7124_ERROR 0x06
> +#define AD7124_ERROR_EN 0x07
> +#define AD7124_MCLK_COUNT 0x08
> +#define AD7124_CHANNEL(x) (0x09 + (x))
> +#define AD7124_CONFIG(x) (0x19 + (x))
> +#define AD7124_FILTER(x) (0x21 + (x))
> +#define AD7124_OFFSET(x) (0x29 + (x))
> +#define AD7124_GAIN(x) (0x31 + (x))
> +
> +/* AD7124_STATUS */
> +#define AD7124_STATUS_POR_FLAG_MSK BIT(4)
> +
> +/* AD7124_ADC_CONTROL */
> +#define AD7124_ADC_CTRL_PWR_MSK GENMASK(7, 6)
> +#define AD7124_ADC_CTRL_PWR(x) FIELD_PREP(AD7124_ADC_CTRL_PWR_MSK, x)
> +#define AD7124_ADC_CTRL_MODE_MSK GENMASK(5, 2)
> +#define AD7124_ADC_CTRL_MODE(x) FIELD_PREP(AD7124_ADC_CTRL_MODE_MSK, x)
> +
> +/* AD7124_CHANNEL_X */
> +#define AD7124_CHANNEL_EN_MSK BIT(15)
> +#define AD7124_CHANNEL_EN(x) FIELD_PREP(AD7124_CHANNEL_EN_MSK, x)
> +#define AD7124_CHANNEL_SETUP_MSK GENMASK(14, 12)
> +#define AD7124_CHANNEL_SETUP(x) FIELD_PREP(AD7124_CHANNEL_SETUP_MSK, x)
> +#define AD7124_CHANNEL_AINP_MSK GENMASK(9, 5)
> +#define AD7124_CHANNEL_AINP(x) FIELD_PREP(AD7124_CHANNEL_AINP_MSK, x)
> +#define AD7124_CHANNEL_AINM_MSK GENMASK(4, 0)
> +#define AD7124_CHANNEL_AINM(x) FIELD_PREP(AD7124_CHANNEL_AINM_MSK, x)
> +
> +/* AD7124_CONFIG_X */
> +#define AD7124_CONFIG_BIPOLAR_MSK BIT(11)
> +#define AD7124_CONFIG_BIPOLAR(x) FIELD_PREP(AD7124_CONFIG_BIPOLAR_MSK, x)
> +#define AD7124_CONFIG_REF_SEL_MSK GENMASK(4, 3)
> +#define AD7124_CONFIG_REF_SEL(x) FIELD_PREP(AD7124_CONFIG_REF_SEL_MSK, x)
> +#define AD7124_CONFIG_PGA_MSK GENMASK(2, 0)
> +#define AD7124_CONFIG_PGA(x) FIELD_PREP(AD7124_CONFIG_PGA_MSK, x)
> +
> +/* AD7124_FILTER_X */
> +#define AD7124_FILTER_FS_MSK GENMASK(10, 0)
> +#define AD7124_FILTER_FS(x) FIELD_PREP(AD7124_FILTER_FS_MSK, x)
> +
> +enum ad7124_ids {
> + ID_AD7124_4,
> + ID_AD7124_8,
> +};
> +
> +enum ad7124_ref_sel {
> + AD7124_REFIN1,
> + AD7124_REFIN2,
> + AD7124_INT_REF,
> + AD7124_AVDD_REF,
> +};
> +
> +enum ad7124_power_mode {
> + AD7124_LOW_POWER,
> + AD7124_MID_POWER,
> + AD7124_FULL_POWER,
> +};
> +
> +static const unsigned int ad7124_gain[8] = {
> + 1, 2, 4, 8, 16, 32, 64, 128
> +};
> +
> +static const int ad7124_master_clk_freq_hz[3] = {
> + [AD7124_LOW_POWER] = 76800,
> + [AD7124_MID_POWER] = 153600,
> + [AD7124_FULL_POWER] = 614400,
> +};
> +
> +static const char * const ad7124_ref_names[] = {
> + [AD7124_REFIN1] = "refin1",
> + [AD7124_REFIN2] = "refin2",
> + [AD7124_INT_REF] = "int",
> + [AD7124_AVDD_REF] = "avdd",
> +};
> +
> +struct ad7124_chip_info {
> + unsigned int num_inputs;
> +};
> +
> +struct ad7124_channel_config {
> + enum ad7124_ref_sel refsel;
> + bool bipolar;
> + unsigned int ain;
> + unsigned int vref_mv;
> + unsigned int pga_bits;
> + unsigned int odr;
> +};
> +
> +struct ad7124_state {
> + const struct ad7124_chip_info *chip_info;
> + struct ad_sigma_delta sd;
> + struct ad7124_channel_config channel_config[4];
> + struct regulator *vref[4];
> + struct clk *mclk;
> + unsigned int adc_control;
> + unsigned int num_channels;
> +};
> +
> +static const struct iio_chan_spec ad7124_channel_template = {
> + .type = IIO_VOLTAGE,
> + .indexed = 1,
> + .differential = 1,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_SCALE) |
> + BIT(IIO_CHAN_INFO_OFFSET) |
> + BIT(IIO_CHAN_INFO_SAMP_FREQ) |
> + BIT(IIO_CHAN_INFO_HARDWAREGAIN),
> + .scan_type = {
> + .sign = 'u',
> + .realbits = 24,
> + .storagebits = 32,
> + .shift = 8,
> + .endianness = IIO_BE,
> + },
> +};

...
> +static int ad7124_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long info)
> +{
> + struct ad7124_state *st = iio_priv(indio_dev);
> + int idx, ret;
> +
> + switch (info) {
> + case IIO_CHAN_INFO_RAW:
> + ret = ad_sigma_delta_single_conversion(indio_dev, chan, val);
> + if (ret < 0)
> + return ret;
> +
> + /* After the conversion is performed, disable the channel */
> + ret = ad_sd_write_reg(&st->sd,
> + AD7124_CHANNEL(chan->address), 2,
> + st->channel_config[chan->address].ain |
> + AD7124_CHANNEL_EN(0));
> + if (ret < 0)
> + return ret;
> +
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_SCALE:
> + idx = st->channel_config[chan->address].pga_bits;
> + *val = st->channel_config[chan->address].vref_mv /
> + ad7124_gain[idx];

Given the gains are all a power of 2. You 'could' apply them to
*val2 instead? Might give better precision. I haven't checked!

> + if (st->channel_config[chan->address].bipolar)
> + *val2 = chan->scan_type.realbits - 1;
> + else
> + *val2 = chan->scan_type.realbits;
> +
> + return IIO_VAL_FRACTIONAL_LOG2;
> + case IIO_CHAN_INFO_OFFSET:
> + if (st->channel_config[chan->address].bipolar) {
> + /* Code = 2^(n â 1) Ã ((Ain Ã Gain / Vref) + 1) */
> + idx = st->channel_config[chan->address].pga_bits;
> + *val = -(st->channel_config[chan->address].vref_mv /
> + ad7124_gain[idx]);
This one surprised me. Offset is applied before gain and I think this part
is symmetric when bipolar, so I think should just be based on the raw adc
counts as half the full range?

> + } else {
> + *val = 0;
> + }
> +
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + *val = st->channel_config[chan->address].odr;
> +
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_HARDWAREGAIN:
> + idx = st->channel_config[chan->address].pga_bits;
> + *val = ad7124_gain[idx];
Hmm. Normally hardwaregain is reserved for those cases where scale
doesn't work. Scale is definitely the preferred option and it'll
take a pretty persuasive argument to convince me otherwise.

Normally hardware gain is when it is is actually effecting the
measurement (so things like light intensity for time of flight
sensors).

> +
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> +}
> +

...

Next message: Jernej Skrabec: "[PATCH] arm64: dts: allwinner: a64: Add mali node"
Previous message: David Sterba: "[GIT PULL] Btrfs fixes for 4.20-rc1"
In reply to: Stefan Popa: "[PATCH v4 3/4] iio: adc: Add ad7124 support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]