Date: Wed, 26 Jun 2024 13:04:32 +0200
From: Nuno Sá <noname.nuno@...il.com>
To: Marcelo Schmitt <marcelo.schmitt@...log.com>, broonie@...nel.org, 
 lars@...afoo.de, Michael.Hennerich@...log.com, jic23@...nel.org, 
 robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
  nuno.sa@...log.com, dlechner@...libre.com, corbet@....net, 
 marcelo.schmitt1@...il.com
Cc: linux-iio@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-spi@...r.kernel.org, linux-doc@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 6/7] iio: adc: Add support for AD4000

On Tue, 2024-06-25 at 18:55 -0300, Marcelo Schmitt wrote:
> Add support for AD4000 series of low noise, low power, high speed,
> successive approximation register (SAR) ADCs.
> 
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@...log.com>
> ---
>  MAINTAINERS              |   1 +
>  drivers/iio/adc/Kconfig  |  12 +
>  drivers/iio/adc/Makefile |   1 +
>  drivers/iio/adc/ad4000.c | 711 +++++++++++++++++++++++++++++++++++++++
>  4 files changed, 725 insertions(+)
>  create mode 100644 drivers/iio/adc/ad4000.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 9aa6531f7cf2..f4ffedada8ea 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1205,6 +1205,7 @@ L:	linux-iio@...r.kernel.org
>  S:	Supported
>  W:	https://ez.analog.com/linux-software-drivers
>  F:	Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
> +F:	drivers/iio/adc/ad4000.c
>  
>  ANALOG DEVICES INC AD4130 DRIVER
>  M:	Cosmin Tanislav <cosmin.tanislav@...log.com>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index b8184706c7d1..5bbe843916a3 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -21,6 +21,18 @@ config AD_SIGMA_DELTA
>  	select IIO_BUFFER
>  	select IIO_TRIGGERED_BUFFER
>  
> +config AD4000
> +	tristate "Analog Devices AD4000 ADC Driver"
> +	depends on SPI
> +	select IIO_BUFFER
> +	select IIO_TRIGGERED_BUFFER
> +	help
> +	  Say yes here to build support for Analog Devices AD4000 high speed
> +	  SPI analog to digital converters (ADC).
> +
> +	  To compile this driver as a module, choose M here: the module will
> be
> +	  called ad4000.
> +
>  config AD4130
>  	tristate "Analog Device AD4130 ADC Driver"
>  	depends on SPI
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 51298c52b223..f4361df40cca 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -6,6 +6,7 @@
>  # When adding new entries keep the list in alphabetical order
>  obj-$(CONFIG_AB8500_GPADC) += ab8500-gpadc.o
>  obj-$(CONFIG_AD_SIGMA_DELTA) += ad_sigma_delta.o
> +obj-$(CONFIG_AD4000) += ad4000.o
>  obj-$(CONFIG_AD4130) += ad4130.o
>  obj-$(CONFIG_AD7091R) += ad7091r-base.o
>  obj-$(CONFIG_AD7091R5) += ad7091r5.o
> diff --git a/drivers/iio/adc/ad4000.c b/drivers/iio/adc/ad4000.c
> new file mode 100644
> index 000000000000..0b6293db68dc
> --- /dev/null
> +++ b/drivers/iio/adc/ad4000.c
> @@ -0,0 +1,711 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * AD4000 SPI ADC driver
> + *
> + * Copyright 2024 Analog Devices Inc.
> + */
> +#include <linux/bits.h>
> +#include <linux/bitfield.h>
> +#include <linux/byteorder/generic.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/math.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +#include <linux/units.h>
> +#include <linux/util_macros.h>
> +#include <linux/iio/iio.h>
> +
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/trigger_consumer.h>
> +
> 

...

> +
> +static int ad4000_read_reg(struct ad4000_state *st, unsigned int *val)
> +{
> +	struct spi_transfer t = {
> +		.tx_buf = st->tx_buf,
> +		.rx_buf = st->rx_buf,
> +		.len = 2,
> +	};
> +	int ret;
> +
> +	st->tx_buf[0] = AD4000_READ_COMMAND;
> +	ret = spi_sync_transfer(st->spi, &t, 1);
> +	if (ret < 0)
> +		return ret;
> +
> +	*val = st->tx_buf[1];

I'm puzzled... tx_buf?

> +	return ret;
> +}
> +
> +/*
> + * This executes a data sample transfer for when the device connections are
> + * in "3-wire" mode, selected when the adi,sdi-pin device tree property is
> + * absent or set to "high". In this connection mode, the ADC SDI pin is
> + * connected to MOSI or to VIO and ADC CNV pin is connected either to a SPI
> + * controller CS or to a GPIO.
> + * AD4000 series of devices initiate conversions on the rising edge of CNV
> pin.
> + *
> + * If the CNV pin is connected to an SPI controller CS line (which is by
> default
> + * active low), the ADC readings would have a latency (delay) of one read.
> + * Moreover, since we also do ADC sampling for filling the buffer on
> triggered
> + * buffer mode, the timestamps of buffer readings would be disarranged.
> + * To prevent the read latency and reduce the time discrepancy between the
> + * sample read request and the time of actual sampling by the ADC, do a
> + * preparatory transfer to pulse the CS/CNV line.
> + */
> +static int ad4000_prepare_3wire_mode_message(struct ad4000_state *st,
> +					     const struct iio_chan_spec
> *chan)
> +{
> +	unsigned int cnv_pulse_time = st->turbo_mode ? AD4000_TQUIET1_NS
> +						     : AD4000_TCONV_NS;
> +	struct spi_transfer *xfers = st->xfers;
> +
> +	xfers[0].cs_change = 1;
> +	xfers[0].cs_change_delay.value = cnv_pulse_time;
> +	xfers[0].cs_change_delay.unit = SPI_DELAY_UNIT_NSECS;
> +
> +	xfers[1].rx_buf = &st->scan.data;
> +	xfers[1].len = BITS_TO_BYTES(chan->scan_type.storagebits);
> +	xfers[1].delay.value = AD4000_TQUIET2_NS;
> +	xfers[1].delay.unit = SPI_DELAY_UNIT_NSECS;
> +
> +	spi_message_init_with_transfers(&st->msg, st->xfers, 2);
> +
> +	return devm_spi_optimize_message(st->spi, &st->msg);
> +}
> +
> +/*
> + * This executes a data sample transfer for when the device connections are
> + * in "4-wire" mode, selected when the adi,sdi-pin device tree property is
> + * set to "cs". In this connection mode, the controller CS pin is connected
> to
> + * ADC SDI pin and a GPIO is connected to ADC CNV pin.
> + * The GPIO connected to ADC CNV pin is set outside of the SPI transfer.
> + */
> +static int ad4000_prepare_4wire_mode_message(struct ad4000_state *st,
> +					     const struct iio_chan_spec
> *chan)
> +{
> +	unsigned int cnv_to_sdi_time = st->turbo_mode ? AD4000_TQUIET1_NS
> +						      : AD4000_TCONV_NS;
> +	struct spi_transfer *xfers = st->xfers;
> +
> +	/*
> +	 * Dummy transfer to cause enough delay between CNV going high and
> SDI
> +	 * going low.
> +	 */
> +	xfers[0].cs_off = 1;
> +	xfers[0].delay.value = cnv_to_sdi_time;
> +	xfers[0].delay.unit = SPI_DELAY_UNIT_NSECS;
> +
> +	xfers[1].rx_buf = &st->scan.data;
> +	xfers[1].len = BITS_TO_BYTES(chan->scan_type.storagebits);
> +
> +	spi_message_init_with_transfers(&st->msg, st->xfers, 2);
> +
> +	return devm_spi_optimize_message(st->spi, &st->msg);
> +}

nit: you could reduce the scope of the above prepare functions...

> +
> +static int ad4000_convert_and_acquire(struct ad4000_state *st)
> +{
> +	int ret;
> +
> +	/*
> +	 * In 4-wire mode, the CNV line is held high for the entire
> conversion
> +	 * and acquisition process. In other modes, the CNV GPIO is optional
> +	 * and, if provided, replaces controller CS. If CNV GPIO is not
> defined
> +	 * gpiod_set_value_cansleep() has no effect.
> +	 */
> +	gpiod_set_value_cansleep(st->cnv_gpio, 1);
> +	ret = spi_sync(st->spi, &st->msg);
> +	gpiod_set_value_cansleep(st->cnv_gpio, 0);
> +
> +	return ret;
> +}
> +
> +static int ad4000_single_conversion(struct iio_dev *indio_dev,
> +				    const struct iio_chan_spec *chan, int
> *val)
> +{
> +	struct ad4000_state *st = iio_priv(indio_dev);
> +	u32 sample;
> +	int ret;
> +
> +	ret = ad4000_convert_and_acquire(st);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (chan->scan_type.storagebits > 16)
> +		sample = be32_to_cpu(st->scan.data.sample_buf32);

Just a minor note regarding your comment in the cover. FWIW, I prefer you leave
it like this. Yes, with 24 bits you save some space but then you need an
unaligned access... To me that space savings are really a micro optimization so
I would definitely go with the simpler form.

> +	else
> +		sample = be16_to_cpu(st->scan.data.sample_buf16);
> +
> +	sample >>= chan->scan_type.shift;
> +
> +	if (chan->scan_type.sign == 's')
> +		*val = sign_extend32(sample, chan->scan_type.realbits - 1);
> +
> +	return IIO_VAL_INT;
> +}
> +
> +static int ad4000_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan, int *val,
> +			   int *val2, long info)
> +{
> +	struct ad4000_state *st = iio_priv(indio_dev);
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_RAW:
> +		iio_device_claim_direct_scoped(return -EBUSY, indio_dev)
> +			return ad4000_single_conversion(indio_dev, chan,
> val);
> +		unreachable();
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = st->scale_tbl[st->span_comp][0];
> +		*val2 = st->scale_tbl[st->span_comp][1];
> +		return IIO_VAL_INT_PLUS_NANO;
> +	case IIO_CHAN_INFO_OFFSET:
> +		*val = 0;
> +		if (st->span_comp)
> +			*val = mult_frac(st->vref_mv, 1, 10);
> +
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int ad4000_read_avail(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     const int **vals, int *type, int *length,
> +			     long info)
> +{
> +	struct ad4000_state *st = iio_priv(indio_dev);
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_SCALE:
> +		*vals = (int *)st->scale_tbl;
> +		*length = AD4000_SCALE_OPTIONS * 2;
> +		*type = IIO_VAL_INT_PLUS_NANO;
> +		return IIO_AVAIL_LIST;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int ad4000_write_raw_get_fmt(struct iio_dev *indio_dev,
> +				    struct iio_chan_spec const *chan, long
> mask)
> +{
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:
> +		return IIO_VAL_INT_PLUS_NANO;
> +	default:
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	}
> +}
> +
> +static int ad4000_write_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan, int val, int
> val2,
> +			    long mask)
> +{
> +	struct ad4000_state *st = iio_priv(indio_dev);
> +	unsigned int reg_val;
> +	bool span_comp_en;
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:
> +		ret = iio_device_claim_direct_mode(indio_dev);

iio_device_claim_direct_scoped()?

> +		if (ret < 0)
> +			return ret;
> +
> +		mutex_lock(&st->lock);

guard()?

> +		ret = ad4000_read_reg(st, &reg_val);
> +		if (ret < 0)
> +			goto err_unlock;
> +
> +		span_comp_en = val2 == st->scale_tbl[1][1];
> +		reg_val &= ~AD4000_CFG_SPAN_COMP;
> +		reg_val |= FIELD_PREP(AD4000_CFG_SPAN_COMP, span_comp_en);
> +
> +		ret = ad4000_write_reg(st, reg_val);
> +		if (ret < 0)
> +			goto err_unlock;
> +
> +		st->span_comp = span_comp_en;
> +err_unlock:
> +		iio_device_release_direct_mode(indio_dev);
> +		mutex_unlock(&st->lock);
> +		return ret;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static irqreturn_t ad4000_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct ad4000_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = ad4000_convert_and_acquire(st);
> +	if (ret < 0)
> +		goto err_out;
> +
> +	iio_push_to_buffers_with_timestamp(indio_dev, &st->scan, pf-
> >timestamp);
> +
> +err_out:
> +	iio_trigger_notify_done(indio_dev->trig);
> +	return IRQ_HANDLED;
> +}
> +
> +static const struct iio_info ad4000_reg_access_info = {
> +	.read_raw = &ad4000_read_raw,
> +	.read_avail = &ad4000_read_avail,
> +	.write_raw = &ad4000_write_raw,
> +	.write_raw_get_fmt = &ad4000_write_raw_get_fmt,
> +};
> +
> +static const struct iio_info ad4000_info = {
> +	.read_raw = &ad4000_read_raw,
> +};
> +
> +static int ad4000_config(struct ad4000_state *st)
> +{
> +	unsigned int reg_val = AD4000_CONFIG_REG_DEFAULT;
> +
> +	if (device_property_present(&st->spi->dev, "adi,high-z-input"))
> +		reg_val |= FIELD_PREP(AD4000_CFG_HIGHZ, 1);
> +
> +	return ad4000_write_reg(st, reg_val);
> +}
> +
> +static int ad4000_probe(struct spi_device *spi)
> +{
> +	const struct ad4000_chip_info *chip;
> +	struct device *dev = &spi->dev;
> +	struct iio_dev *indio_dev;
> +	struct ad4000_state *st;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	chip = spi_get_device_match_data(spi);
> +	if (!chip)
> +		return -EINVAL;
> +
> +	st = iio_priv(indio_dev);
> +	st->spi = spi;
> +
> +	ret = devm_regulator_get_enable(dev, "vdd");
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to enable VDD
> supply\n");
> +
> +	ret = devm_regulator_get_enable(dev, "vio");
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to enable VIO
> supply\n");

devm_regulator_bulk_get_enable()? Do we have any ordering constrains?

> +
> +	ret = devm_regulator_get_enable_read_voltage(dev, "ref");
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to get ref regulator
> reference\n");
> +	st->vref_mv = ret / 1000;
> +
> +	st->cnv_gpio = devm_gpiod_get_optional(dev, "cnv", GPIOD_OUT_HIGH);
> +	if (IS_ERR(st->cnv_gpio))
> +		return dev_err_probe(dev, PTR_ERR(st->cnv_gpio),
> +				     "Failed to get CNV GPIO");
> +
> +	ret = device_property_match_property_string(dev, "adi,sdi-pin",
> +						    ad4000_sdi_pin,
> +						   
> ARRAY_SIZE(ad4000_sdi_pin));
> +	if (ret < 0 && ret != -EINVAL)
> +		return dev_err_probe(dev, ret,
> +				     "getting adi,sdi-pin property
> failed\n");
> +
> +	/* Default to usual SPI connections if pin properties are not present
> */
> +	st->sdi_pin = ret == -EINVAL ? AD4000_SDI_MOSI : ret;
> +	switch (st->sdi_pin) {
> +	case AD4000_SDI_MOSI:
> +		indio_dev->info = &ad4000_reg_access_info;
> +		indio_dev->channels = &chip->reg_access_chan_spec;
> +
> +		/*
> +		 * In "3-wire mode", the ADC SDI line must be kept high when
> +		 * data is not being clocked out of the controller.
> +		 * Request the SPI controller to make MOSI idle high.
> +		 */
> +		spi->mode |= SPI_MOSI_IDLE_HIGH;
> +		ret = spi_setup(spi);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = ad4000_prepare_3wire_mode_message(st, indio_dev-
> >channels);
> +		if (ret)
> +			return ret;
> +
> +		ret = ad4000_config(st);
> +		if (ret < 0)
> +			dev_warn(dev, "Failed to config device\n");
> +

Should this be a warning? Very suspicious :)

> +		break;
> +	case AD4000_SDI_VIO:
> +		indio_dev->info = &ad4000_info;
> +		indio_dev->channels = &chip->chan_spec;
> +		ret = ad4000_prepare_3wire_mode_message(st, indio_dev-
> >channels);
> +		if (ret)
> +			return ret;
> +
> +		break;
> +	case AD4000_SDI_CS:
> +		indio_dev->info = &ad4000_info;
> +		indio_dev->channels = &chip->chan_spec;
> +		ret = ad4000_prepare_4wire_mode_message(st, indio_dev-
> >channels);
> +		if (ret)
> +			return ret;
> +
> +		break;
> +	default:
> +		return dev_err_probe(dev, -EINVAL, "Unrecognized connection
> mode\n");
> +	}
> +
> +	indio_dev->name = chip->dev_name;
> +	indio_dev->num_channels = 1;
> +
> +	devm_mutex_init(dev, &st->lock);
> +
> +	st->gain_milli = 1000;
> +	if (chip->has_hardware_gain) {
> +		if (device_property_present(dev, "adi,gain-milli")) {
> +			ret = device_property_read_u16(dev, "adi,gain-milli",
> +						       &st->gain_milli);
> +			if (ret)
> +				return dev_err_probe(dev, ret,
> +						     "Failed to read gain
> property\n");
> +		}
> 

the above looks odd. Why not?

ret = device_property_read_u16(dev, "adi,gain-milli", &st->gain_milli);
if (!ret) {
	...
}

Note that you're also allowing any value for gain_milli when we just allow some
of them (according to the bindings). Hence you should make sure we get supported
values from FW.

- Nuno Sá

Powered by blists - more mailing lists