Message-ID: <1541687328.2091.2.camel@analog.com>
Date:   Thu, 8 Nov 2018 14:28:50 +0000
From:   "Popa, Stefan Serban" <StefanSerban.Popa@...log.com>
To:     "jic23@...nel.org" <jic23@...nel.org>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>
CC:     "knaack.h@....de" <knaack.h@....de>,
        "lars@...afoo.de" <lars@...afoo.de>,
        "pmeerw@...erw.net" <pmeerw@...erw.net>,
        "Hennerich, Michael" <Michael.Hennerich@...log.com>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 2/3] iio: adc: Add ad7124 support

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@...log.com> 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@...log.com>
> 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@...log.com>
> > +L:	linux-iio@...r.kernel.org
> > +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@...co.com>
> >  L:	linux-media@...r.kernel.org
> > 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@...log.com>");
> > +MODULE_DESCRIPTION("Analog Devices AD7124 SPI driver");
> > +MODULE_LICENSE("GPL");
> 

Powered by blists - more mailing lists