Re: [PATCH v3 2/3] iio: adc: Add ad7124 support

From: Popa, Stefan Serban
Date: Thu Nov 08 2018 - 09:29:08 EST

Next message: Sebastian Andrzej Siewior: "Re: [PATCH v7 0/7] clocksource: rework Atmel TCB timer driver"
Previous message: Christian Brauner: "Re: [PATCH V3] binder: ipc namespace support for android binder"
Next in thread: Rob Herring: "Re: [PATCH v3 2/3] iio: adc: Add ad7124 support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Sb, 2018-11-03 at 12:16 +0000, Jonathan Cameron wrote:
> On Mon, 29 Oct 2018 18:38:31 +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.
> >
> > Datasheets:
> > Link: http://www.analog.com/media/en/technical-documentation/data-sheet
> > s/AD7124-4.pdf
> > Link: http://www.analog.com/media/en/technical-documentation/data-sheet
> > s/ad7124-8.pdf
> >
> > Signed-off-by: Stefan Popa <stefan.popa@xxxxxxxxxx>
> Hi Stefan,
>
> The discussion around the DT binding has gotten me looking at bit
> more closely at that for this version.
>
> Some most comments in that section.ÂÂAlso a really minor ordering issue
> in
> remove which I'd just have fixed if we weren't going around again for
> the binding.
>
> Main binding thing is I don't think the odr value belongs in DT.
> Gain is more marginal (unless the part can actually be damaged by
> a wrong value - which I hope it can't!).ÂÂI'm not that fussed
> as there are definitely reasons to specify a default scale to
> cover the reasonable range on a pin.
>
> Thanks,
>
> Jonathan

Hi Jonathan,

Thank you for the review! So, how should I proceed?

First, we need an adc.txt file where "bipolar" and something like "diff-
channels" should be documented. Should the file be placed under
Documentation/devicetree/bindings/iio/adc?

Regarding the "odr-hz" property, it totally makes sense to remove it from
the DT. How about the "gain"? Should we leave it in the DT and also add the
possibility to be configured from user space?

Regards,
-Stefan
> >
> > ---
> > Changes in v2:
> > - Added this commit.
> > 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().
> >
> > ÂMAINTAINERSÂÂÂÂÂÂÂÂÂÂÂÂÂÂ|ÂÂÂ7 +
> > Âdrivers/iio/adc/KconfigÂÂ|ÂÂ11 +
> > Âdrivers/iio/adc/Makefile |ÂÂÂ1 +
> > Âdrivers/iio/adc/ad7124.c | 648
> > +++++++++++++++++++++++++++++++++++++++++++++++
> > Â4 files changed, 667 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..0660135
> > --- /dev/null
> > +++ b/drivers/iio/adc/ad7124.c
> > @@ -0,0 +1,648 @@
> > +// 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_CT
> > RL_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_CHANNE
> > L_AINP_MSK, x)
> > +#define AD7124_CHANNEL_AINM_MSK GENMASK(4, 0)
> > +#define AD7124_CHANNEL_AINM(x) FIELD_PREP(AD7124_CHANNE
> > L_AINM_MSK, x)
> > +
> > +/* AD7124_CONFIG_X */
> > +#define AD7124_CONFIG_BIPOLAR_MSK BIT(11)
> > +#define AD7124_CONFIG_BIPOLAR(x) FIELD_PREP(AD7124_CONFIG_BIPOL
> > AR_MSK, x)
> > +#define AD7124_CONFIG_REF_SEL_MSK GENMASK(4, 3)
> > +#define AD7124_CONFIG_REF_SEL(x) FIELD_PREP(AD7124_CONFIG_REF_S
> > EL_MSK, x)
> > +#define AD7124_CONFIG_PGA_MSK GENMASK(2, 0)
> > +#define AD7124_CONFIG_PGA(x) FIELD_PREP(AD7124_CONFIG_P
> > GA_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),
> > + .scan_type = {
> > + .sign = 'u',
> > + .realbits = 24,
> > + .storagebits = 32,
> > + },
> > +};
> > +
> > +static struct ad7124_chip_info ad7124_chip_info_tbl[] = {
> > + [ID_AD7124_4] = {
> > + .num_inputs = 8,
> > + },
> > + [ID_AD7124_8] = {
> > + .num_inputs = 16,
> > + },
> > +};
> > +
> > +static int ad7124_find_closest_match(const int *array,
> > + ÂÂÂÂÂunsigned int size, int val)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < size; i++) {
> > + if (val <= array[i])
> > + return i;
> > + }
> > +
> > + return size - 1;
> > +}
> > +
> > +static int ad7124_spi_write_mask(struct ad7124_state *st,
> > + Âunsigned int addr,
> > + Âunsigned long mask,
> > + Âunsigned int val,
> > + Âunsigned int bytes)
> > +{
> > + unsigned int readval;
> > + int ret;
> > +
> > + ret = ad_sd_read_reg(&st->sd, addr, bytes, &readval);
> > + if (ret < 0)
> > + return ret;
> > +
> > + readval &= ~mask;
> > + readval |= val;
> > +
> > + return ad_sd_write_reg(&st->sd, addr, bytes, readval);
> > +}
> > +
> > +static int ad7124_set_mode(struct ad_sigma_delta *sd,
> > + ÂÂÂenum ad_sigma_delta_mode mode)
> > +{
> > + struct ad7124_state *st = container_of(sd, struct
> > ad7124_state, sd);
> > +
> > + st->adc_control &= ~AD7124_ADC_CTRL_MODE_MSK;
> > + st->adc_control |= AD7124_ADC_CTRL_MODE(mode);
> > +
> > + return ad_sd_write_reg(&st->sd, AD7124_ADC_CONTROL, 2, st-
> > >adc_control);
> > +}
> > +
> > +static int ad7124_set_channel(struct ad_sigma_delta *sd, unsigned int
> > channel)
> > +{
> > + struct ad7124_state *st = container_of(sd, struct
> > ad7124_state, sd);
> > + unsigned int val;
> > +
> > + val = st->channel_config[channel].ain | AD7124_CHANNEL_EN(1) |
> > + ÂÂÂÂÂÂAD7124_CHANNEL_SETUP(channel);
> > +
> > + return ad_sd_write_reg(&st->sd, AD7124_CHANNEL(channel), 2,
> > val);
> > +}
> > +
> > +static const struct ad_sigma_delta_info ad7124_sigma_delta_info = {
> > + .set_channel = ad7124_set_channel,
> > + .set_mode = ad7124_set_mode,
> > + .has_registers = true,
> > + .addr_shift = 0,
> > + .read_mask = BIT(6),
> > + .data_reg = AD7124_DATA,
> > +};
> > +
> > +static int ad7124_set_channel_odr(struct ad7124_state *st,
> > + ÂÂunsigned int channel,
> > + ÂÂunsigned int odr)
> > +{
> > + unsigned int fclk, odr_sel_bits;
> > + int ret;
> > +
> > + fclk = clk_get_rate(st->mclk);
> > + /*
> > + Â* FS[10:0] = fCLK / (fADC x 32) where:
> > + Â* fADC is the output data rate
> > + Â* fCLK is the master clock frequency
> > + Â* FS[10:0] are the bits in the filter register
> > + Â* FS[10:0] can have a value from 1 to 2047
> > + Â*/
> > + odr_sel_bits = DIV_ROUND_CLOSEST(fclk, odr * 32);
> > + if (odr_sel_bits < 1)
> > + odr_sel_bits = 1;
> > + else if (odr_sel_bits > 2047)
> > + odr_sel_bits = 2047;
> > +
> > + ret = ad7124_spi_write_mask(st, AD7124_FILTER(channel),
> > + ÂÂÂÂAD7124_FILTER_FS_MSK,
> > + ÂÂÂÂAD7124_FILTER_FS(odr_sel_bits),
> > 3);
> > + if (ret < 0)
> > + return ret;
> > + /* fADC = fCLK / (FS[10:0] x 32) */
> > + st->channel_config[channel].odr =
> > + DIV_ROUND_CLOSEST(fclk, odr_sel_bits * 32);
> > +
> > + return 0;
> > +}
> > +
> > +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];
> > + 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]);
> > + } else {
> > + *val = 0;
> > + }
> > +
> > + return IIO_VAL_INT;
> > + case IIO_CHAN_INFO_SAMP_FREQ:
> > + *val = st->channel_config[chan->address].odr;
> > +
> > + return IIO_VAL_INT;
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static int ad7124_write_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);
> > +
> > + switch (info) {
> > + case IIO_CHAN_INFO_SAMP_FREQ:
> > + if (val2 != 0)
> > + return -EINVAL;
> > +
> > + return ad7124_set_channel_odr(st, chan->address, val);
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static const struct iio_info ad7124_info = {
> > + .read_raw = ad7124_read_raw,
> > + .write_raw = ad7124_write_raw,
> > +};
> > +
> > +static int ad7124_soft_reset(struct ad7124_state *st)
> > +{
> > + unsigned int readval, timeout;
> > + int ret;
> > +
> > + ret = ad_sd_reset(&st->sd, 64);
> > + if (ret < 0)
> > + return ret;
> > +
> > + timeout = 100;
> > + do {
> > + ret = ad_sd_read_reg(&st->sd, AD7124_STATUS, 1,
> > &readval);
> > + if (ret < 0)
> > + return ret;
> > +
> > + if (!(readval & AD7124_STATUS_POR_FLAG_MSK))
> > + return 0;
> > +
> > + /* The AD7124 requires typically 2ms to power up and
> > settle */
> > + usleep_range(100, 2000);
> > + } while (--timeout);
> > +
> > + dev_err(&st->sd.spi->dev, "Soft reset failed\n");
> > +
> > + return -EIO;
> > +}
> > +
> > +static int ad7124_init_channel_vref(struct ad7124_state *st,
> > + ÂÂÂÂunsigned int channel_number)
> > +{
> > + unsigned int refsel = st-
> > >channel_config[channel_number].refsel;
> > +
> > + switch (refsel) {
> > + case AD7124_REFIN1:
> > + case AD7124_REFIN2:
> > + case AD7124_AVDD_REF:
> > + if (IS_ERR(st->vref[refsel])) {
> > + dev_err(&st->sd.spi->dev,
> > + "Error, trying to use external voltage
> > reference without a %s regulator.",
> > + ad7124_ref_names[refsel]);
> > + return PTR_ERR(st->vref[refsel]);
> > + }
> > + st->channel_config[channel_number].vref_mv =
> > + regulator_get_voltage(st->vref[refsel]);
> > + /* Conversion from uV to mV */
> > + st->channel_config[channel_number].vref_mv /= 1000;
> > + break;
> > + case AD7124_INT_REF:
> > + st->channel_config[channel_number].vref_mv = 2500;
> > + break;
> > + default:
> > + dev_err(&st->sd.spi->dev, "Invalid reference %d\n",
> > refsel);
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int ad7124_of_parse_channel_config(struct iio_dev *indio_dev,
> > + ÂÂstruct device_node *np)
> > +{
> > + struct ad7124_state *st = iio_priv(indio_dev);
> > + struct device_node *child;
> > + struct iio_chan_spec *chan;
> > + unsigned int ain[2], channel = 0, tmp;
> > + int ret, res;
> > +
> > + st->num_channels = of_get_available_child_count(np);
> > + if (!st->num_channels) {
> > + dev_err(indio_dev->dev.parent, "no channel
> > children\n");
> > + return -ENODEV;
> > + }
> > +
> > + chan = devm_kcalloc(indio_dev->dev.parent, st->num_channels,
> > + ÂÂÂÂsizeof(*chan), GFP_KERNEL);
> > + if (!chan)
> > + return -ENOMEM;
> > +
> > + indio_dev->channels = chan;
> > + indio_dev->num_channels = st->num_channels;
> > +
> > + for_each_available_child_of_node(np, child) {
> > + ret = of_property_read_u32(child, "reg", &channel);
> > + if (ret)
> > + goto err;
> > +
> > + ret = of_property_read_u32_array(child, "adi,diff-
> > channels",
> > + Âain, 2);
> This actually feels like something we could standardize as well as
> bipolar.
> In the oldest drivers we actually let userspace configure all of this,
> but
> I can understand that only some combinations make sense on a given board
> so it arguably makes sense to only enable those, particularly when there
> is a reference select that has to be paired with channel choice.
>
> >
> > + if (ret)
> > + goto err;
> > +
> > + if (ain[0] >= st->chip_info->num_inputs ||
> > + ÂÂÂÂain[1] >= st->chip_info->num_inputs) {
> > + dev_err(indio_dev->dev.parent,
> > + "Input pin number out of range.\n");
> > + ret = -EINVAL;
> > + goto err;
> > + }
> > + st->channel_config[channel].ain =
> > AD7124_CHANNEL_AINP(ain[0]) |
> > + ÂÂAD7124_CHANNEL_AINM(
> > ain[1]);
> > + st->channel_config[channel].bipolar =
> > + of_property_read_bool(child, "adi,bipolar");
> > +
> > + ret = of_property_read_u32(child, "adi,reference-
> > select", &tmp);
> > + if (ret)
> > + st->channel_config[channel].refsel =
> > AD7124_INT_REF;
> > + else
> > + st->channel_config[channel].refsel = tmp;
> > +
> > + ret = of_property_read_u32(child, "adi,gain", &tmp);
> > + if (ret) {
> > + st->channel_config[channel].pga_bits = 0;
> > + } else {
> > + res = ad7124_find_closest_match(ad7124_gain,
> > + ARRAY_SIZE(ad7124_gain
> > ), tmp);
> > + st->channel_config[channel].pga_bits = res;
> Hmm. The old question of what to put in DT as it reflects wiring and
> what to leave to userspace. Gain is tricky as only some values make sense
> for a given system, but there can be more than one that does...
> This is probably reasonable as it can be considered as setting the
> default
> that makes sense for what is wired.ÂÂPotentially user space could
> override
> it later if it wanted to.
>
> >
> > + }
> > +
> > + ret = of_property_read_u32(child, "adi,odr-hz", &tmp);
> Why is this in DT. This one feels like a userspace choice to me. It's
> only tangentially connected to how things are connected on the board.
> You also support control from userspace.ÂÂI would pick a sensible
> general default and then drop this from the DT binding. It's optional
> anyway.
>
> >
> > + if (ret)
> > + /*
> > + Â* 9 SPS is the minimum output data rate
> > supported
> > + Â* regardless of the selected power mode.
> > + Â*/
> > + st->channel_config[channel].odr = 9;
> > + else
> > + st->channel_config[channel].odr = tmp;
> > +
> > + *chan = ad7124_channel_template;
> > + chan->address = channel;
> > + chan->scan_index = channel;
> > + chan->channel = ain[0];
> > + chan->channel2 = ain[1];
> > +
> > + chan++;
> > + }
> > +
> > + return 0;
> > +err:
> > + of_node_put(child);
> > +
> > + return ret;
> > +}
> > +
> > +static int ad7124_setup(struct ad7124_state *st)
> > +{
> > + unsigned int val, fclk, power_mode;
> > + int i, ret;
> > +
> > + fclk = clk_get_rate(st->mclk);
> > + if (!fclk)
> > + return -EINVAL;
> > +
> > + /* The power mode changes the master clock frequency */
> > + power_mode =
> > ad7124_find_closest_match(ad7124_master_clk_freq_hz,
> > + ARRAY_SIZE(ad7124_master_clk_f
> > req_hz),
> > + fclk);
> > + if (fclk != ad7124_master_clk_freq_hz[power_mode]) {
> > + ret = clk_set_rate(st->mclk, fclk);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + /* Set the power mode */
> > + st->adc_control &= ~AD7124_ADC_CTRL_PWR_MSK;
> > + st->adc_control |= AD7124_ADC_CTRL_PWR(power_mode);
> > + ret = ad_sd_write_reg(&st->sd, AD7124_ADC_CONTROL, 2, st-
> > >adc_control);
> > + if (ret < 0)
> > + return ret;
> > +
> > + for (i = 0; i < st->num_channels; i++) {
> > + val = st->channel_config[i].ain |
> > AD7124_CHANNEL_SETUP(i);
> > + ret = ad_sd_write_reg(&st->sd, AD7124_CHANNEL(i), 2,
> > val);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = ad7124_init_channel_vref(st, i);
> > + if (ret < 0)
> > + return ret;
> > +
> > + val = AD7124_CONFIG_BIPOLAR(st-
> > >channel_config[i].bipolar) |
> > + ÂÂÂÂÂÂAD7124_CONFIG_REF_SEL(st-
> > >channel_config[i].refsel) |
> > + ÂÂÂÂÂÂAD7124_CONFIG_PGA(st-
> > >channel_config[i].pga_bits);
> > + ret = ad_sd_write_reg(&st->sd, AD7124_CONFIG(i), 2,
> > val);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = ad7124_set_channel_odr(st, i, st-
> > >channel_config[i].odr);
> > + if (ret < 0)
> > + return ret;
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static int ad7124_probe(struct spi_device *spi)
> > +{
> > + const struct spi_device_id *id;
> > + struct ad7124_state *st;
> > + struct iio_dev *indio_dev;
> > + int i, ret;
> > +
> > + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> > + if (!indio_dev)
> > + return -ENOMEM;
> > +
> > + st = iio_priv(indio_dev);
> > +
> > + id = spi_get_device_id(spi);
> > + st->chip_info = &ad7124_chip_info_tbl[id->driver_data];
> > +
> > + ad_sd_init(&st->sd, indio_dev, spi, &ad7124_sigma_delta_info);
> > +
> > + spi_set_drvdata(spi, indio_dev);
> > +
> > + indio_dev->dev.parent = &spi->dev;
> > + indio_dev->name = spi_get_device_id(spi)->name;
> > + indio_dev->modes = INDIO_DIRECT_MODE;
> > + indio_dev->info = &ad7124_info;
> > +
> > + ret = ad7124_of_parse_channel_config(indio_dev, spi-
> > >dev.of_node);
> > + if (ret < 0)
> > + return ret;
> > +
> > + for (i = 0; i < ARRAY_SIZE(st->vref); i++) {
> > + if (i != AD7124_INT_REF) {
> > + st->vref[i] =
> > devm_regulator_get_optional(&spi->dev,
> > + ad7124_ref_nam
> > es[i]);
> > + if (PTR_ERR(st->vref[i]) == -ENODEV)
> > + continue;
> > + else if (IS_ERR(st->vref[i]))
> > + return PTR_ERR(st->vref[i]);
> > +
> > + ret = regulator_enable(st->vref[i]);
> > + if (ret)
> > + return ret;
> > + }
> > + }
> > +
> > + st->mclk = devm_clk_get(&spi->dev, "mclk");
> > + if (IS_ERR(st->mclk)) {
> > + ret = PTR_ERR(st->mclk);
> > + goto error_regulator_disable;
> > + }
> > +
> > + ret = clk_prepare_enable(st->mclk);
> > + if (ret < 0)
> > + goto error_regulator_disable;
> > +
> > + ret = ad7124_soft_reset(st);
> > + if (ret < 0)
> > + goto error_clk_disable_unprepare;
> > +
> > + ret = ad7124_setup(st);
> > + if (ret < 0)
> > + goto error_clk_disable_unprepare;
> > +
> > + ret = ad_sd_setup_buffer_and_trigger(indio_dev);
> > + if (ret < 0)
> > + goto error_clk_disable_unprepare;
> > +
> > + ret = iio_device_register(indio_dev);
> > + if (ret < 0) {
> > + dev_err(&spi->dev, "Failed to register iio device\n");
> > + goto error_remove_trigger;
> > + }
> > +
> > + return 0;
> > +
> > +error_remove_trigger:
> > + ad_sd_cleanup_buffer_and_trigger(indio_dev);
> > +error_clk_disable_unprepare:
> > + clk_disable_unprepare(st->mclk);
> > +error_regulator_disable:
> > + for (i = ARRAY_SIZE(st->vref) - 1; i >= 0; i--) {
> > + if (!IS_ERR_OR_NULL(st->vref[i]))
> > + regulator_disable(st->vref[i]);
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static int ad7124_remove(struct spi_device *spi)
> > +{
> > + struct iio_dev *indio_dev = spi_get_drvdata(spi);
> > + struct ad7124_state *st = iio_priv(indio_dev);
> > + int i;
> > +
> > + iio_device_unregister(indio_dev);
> > + clk_disable_unprepare(st->mclk);
> > + ad_sd_cleanup_buffer_and_trigger(indio_dev);
> The ordering here should match that in the error path above.
> (so the two things here should be reversed).
> It's in the category of making things obviously safe rather than an
> actual issue.
> I like to be able to check the ordering only once rather than twice
> when reviewing so will always confirm they match.
>
> >
> > +
> > + for (i = ARRAY_SIZE(st->vref) - 1; i >= 0; i--) {
> > + if (!IS_ERR_OR_NULL(st->vref[i]))
> > + regulator_disable(st->vref[i]);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static const struct spi_device_id ad7124_id_table[] = {
> > + { "ad7124-4", ID_AD7124_4 },
> > + { "ad7124-8", ID_AD7124_8 },
> > + {}
> > +};
> > +MODULE_DEVICE_TABLE(spi, ad7124_id_table);
> > +
> > +static const struct of_device_id ad7124_of_match[] = {
> > + { .compatible = "adi,ad7124-4" },
> > + { .compatible = "adi,ad7124-8" },
> > + { },
> > +};
> > +MODULE_DEVICE_TABLE(of, ad7124_of_match);
> > +
> > +static struct spi_driver ad71124_driver = {
> > + .driver = {
> > + .name = "ad7124",
> > + .of_match_table = ad7124_of_match,
> > + },
> > + .probe = ad7124_probe,
> > + .remove = ad7124_remove,
> > + .id_table = ad7124_id_table,
> > +};
> > +module_spi_driver(ad71124_driver);
> > +
> > +MODULE_AUTHOR("Stefan Popa <stefan.popa@xxxxxxxxxx>");
> > +MODULE_DESCRIPTION("Analog Devices AD7124 SPI driver");
> > +MODULE_LICENSE("GPL");
>

Next message: Sebastian Andrzej Siewior: "Re: [PATCH v7 0/7] clocksource: rework Atmel TCB timer driver"
Previous message: Christian Brauner: "Re: [PATCH V3] binder: ipc namespace support for android binder"
Next in thread: Rob Herring: "Re: [PATCH v3 2/3] iio: adc: Add ad7124 support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]