Message-ID: <f02cac02f9404bf6dcc5a8274b51d836960871ee.camel@gmail.com>
Date: Wed, 26 Jun 2024 13:47:23 +0200
From: Nuno Sá <noname.nuno@...il.com>
To: David Lechner <dlechner@...libre.com>, Jonathan Cameron
 <jic23@...nel.org>,  Rob Herring <robh@...nel.org>, Krzysztof Kozlowski
 <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>
Cc: Michael Hennerich <michael.hennerich@...log.com>, Nuno
 Sá
	 <nuno.sa@...log.com>, Jonathan Corbet <corbet@....net>, 
	linux-iio@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org, Ramona Gradinariu
	 <ramona.gradinariu@...log.com>
Subject: Re: [PATCH v3 2/3] iio: adc: ad4695: Add driver for AD4695 and
 similar ADCs

Hi David,

minor stuff from me..

On Mon, 2024-06-24 at 17:01 -0500, David Lechner wrote:
> This is a new driver for Analog Devices Inc. AD4695 and similar ADCs.
> The initial driver supports initializing the chip including configuring
> all possible LDO and reference voltage sources as well as any possible
> voltage input channel wiring configuration.
> 
> Only the 4-wire SPI wiring mode where the CNV pin is tied to the CS pin
> is supported at this time. And reading sample data from the ADC can only
> be done in direct mode for now.
> 
> Co-developed-by: Ramona Gradinariu <ramona.gradinariu@...log.com>
> Signed-off-by: Ramona Gradinariu <ramona.gradinariu@...log.com>
> Signed-off-by: David Lechner <dlechner@...libre.com>
> ---
> 
> v3 changes:
> * always include all channels, not just ones customized in DT
> * rework channel fwnode parsing for DT changes
> 
> v2 changes:
> * rework register definition macros
> * remove code structure comments at top level
> * remove simple wrapper functions around regmap functions
> * rework channel fwnode parsing for DT changes
> * fix missing return value check
> ---
>  MAINTAINERS              |   1 +
>  drivers/iio/adc/Kconfig  |  11 +
>  drivers/iio/adc/Makefile |   1 +
>  drivers/iio/adc/ad4695.c | 730
> +++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 743 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 025ee138a713..c52c4b0e69a8 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1216,6 +1216,7 @@ L:	linux-iio@...r.kernel.org
>  S:	Supported
>  W:	https://ez.analog.com/linux-software-drivers
>  F:	Documentation/devicetree/bindings/iio/adc/adi,ad4695.yaml
> +F:	drivers/iio/adc/ad4695.c
>  F:	include/dt-bindings/iio/adi,ad4695.h
>  
>  ANALOG DEVICES INC AD7091R DRIVER
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index b8184706c7d1..eed5547a5078 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -36,6 +36,17 @@ config AD4130
>  	  To compile this driver as a module, choose M here: the module will
> be
>  	  called ad4130.
>  
> +config AD4695
> +	tristate "Analog Device AD4695 ADC Driver"
> +	depends on SPI
> +	select REGMAP_SPI
> +	help
> +	  Say yes here to build support for Analog Devices AD4695 and similar
> +	  analog to digital converters (ADC).
> +
> +	  To compile this driver as a module, choose M here: the module will
> be
> +	  called ad4695.
> +
>  config AD7091R
>  	tristate
>  
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 51298c52b223..c180c85b6327 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -7,6 +7,7 @@
>  obj-$(CONFIG_AB8500_GPADC) += ab8500-gpadc.o
>  obj-$(CONFIG_AD_SIGMA_DELTA) += ad_sigma_delta.o
>  obj-$(CONFIG_AD4130) += ad4130.o
> +obj-$(CONFIG_AD4695) += ad4695.o
>  obj-$(CONFIG_AD7091R) += ad7091r-base.o
>  obj-$(CONFIG_AD7091R5) += ad7091r5.o
>  obj-$(CONFIG_AD7091R8) += ad7091r8.o
> diff --git a/drivers/iio/adc/ad4695.c b/drivers/iio/adc/ad4695.c
> new file mode 100644
> index 000000000000..72ab5b997f6e
> --- /dev/null
> +++ b/drivers/iio/adc/ad4695.c
> @@ -0,0 +1,730 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * SPI ADC driver for Analog Devices Inc. AD4695 and similar chips
> + *
> + * https://www.analog.com/en/products/ad4695.html
> + * https://www.analog.com/en/products/ad4696.html
> + * https://www.analog.com/en/products/ad4697.html
> + * https://www.analog.com/en/products/ad4698.html
> + *
> + * Copyright 2024 Analog Devices Inc.
> + * Copyright 2024 BayLibre, SAS
> + */
> +
> +#include <dt-bindings/iio/adi,ad4695.h>
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/compiler.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +#include <linux/units.h>
> +
> 

...

> +
> +static int ad4695_write_chn_cfg(struct ad4695_state *st,
> +				struct ad4695_channel_config *cfg)
> +{
> +	u32 mask = 0, val = 0;
> +
> +	mask |= AD4695_REG_CONFIG_IN_MODE;
> +	val |= FIELD_PREP(AD4695_REG_CONFIG_IN_MODE, cfg->bipolar ? 1 : 0);
> +

nit: don't need to OR the first assignments and so initializing the variables.

> +	mask |= AD4695_REG_CONFIG_IN_PAIR;
> +	val |= FIELD_PREP(AD4695_REG_CONFIG_IN_PAIR, cfg->pin_pairing);
> +
> +	mask |= AD4695_REG_CONFIG_IN_AINHIGHZ_EN;
> +	val |= FIELD_PREP(AD4695_REG_CONFIG_IN_AINHIGHZ_EN, cfg->highz_en ? 1
> : 0);
> +
> +	return regmap_update_bits(st->regmap, AD4695_REG_CONFIG_IN(cfg-
> >channel),
> +				  mask, val);
> +}
> +
> +/**
> + * ad4695_read_one_sample - Read a single sample using single-cycle mode
> + * @st: The AD4695 state
> + * @address: The address of the channel to read
> + *
> + * Upon return, the sample will be stored in the raw_data field of @st.
> + *
> + * Context: can sleep, must be called with iio_device_claim_direct held
> + * Return: 0 on success, a negative error code on failure
> + */
> +static int ad4695_read_one_sample(struct ad4695_state *st, unsigned int
> address)
> +{
> +	struct spi_transfer xfer[2] = { };
> +	int ret;
> +
> +	ret = ad4695_set_single_cycle_mode(st, address);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Setting the first channel to the temperature channel isn't
> supported
> +	 * in single-cycle mode, so we have to do an extra xfer to read the
> +	 * temperature.
> +	 */
> +	if (address == AD4695_CMD_TEMP_CHAN) {
> +		/* We aren't reading, so we can make this a short xfer. */
> +		st->cnv_cmd2 = AD4695_CMD_TEMP_CHAN << 3;
> +		xfer[0].bits_per_word = 8;

nit: isn't this the default?

> +		xfer[0].tx_buf = &st->cnv_cmd2;
> +		xfer[0].len = 1;
> +		xfer[0].cs_change = 1;
> +		xfer[0].cs_change_delay.value = AD4695_T_CONVERT_NS;
> +		xfer[0].cs_change_delay.unit = SPI_DELAY_UNIT_NSECS;
> +
> +		/* Then read the result and exit conversion mode. */
> +		st->cnv_cmd = AD4695_CMD_EXIT_CNV_MODE << 11;
> +		xfer[1].bits_per_word = 16;
> +		xfer[1].tx_buf = &st->cnv_cmd;
> +		xfer[1].rx_buf = &st->raw_data;
> +		xfer[1].len = 2;
> +
> +		return spi_sync_transfer(st->spi, xfer, 2);
> +	}
> +
> +	/*
> +	 * The conversion has already been done and we just have to read the
> +	 * result and exit conversion mode.
> +	 */
> +	st->cnv_cmd = AD4695_CMD_EXIT_CNV_MODE << 11;
> +	xfer[0].bits_per_word = 16;
> +	xfer[0].tx_buf = &st->cnv_cmd;
> +	xfer[0].rx_buf = &st->raw_data;
> +	xfer[0].len = 2;
> +
> +	return spi_sync_transfer(st->spi, xfer, 1);
> +}
> +
> +static int ad4695_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan,
> +			   int *val, int *val2, long mask)
> +{
> +	struct ad4695_state *st = iio_priv(indio_dev);
> +	struct ad4695_channel_config *cfg = &st->channels_cfg[chan-
> >scan_index];
> +	u8 realbits = chan->scan_type.realbits;
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> +			ret = ad4695_read_one_sample(st, chan->address);
> +			if (ret)
> +				return ret;
> +
> +			if (chan->scan_type.sign == 's')
> +				*val = sign_extend32(st->raw_data, realbits -
> 1);
> +			else
> +				*val = st->raw_data;
> +
> +			return IIO_VAL_INT;
> +		}
> +		unreachable();
> +	case IIO_CHAN_INFO_SCALE:
> +		switch (chan->type) {
> +		case IIO_VOLTAGE:
> +			*val = st->vref_mv;
> +			*val2 = chan->scan_type.realbits;
> +			return IIO_VAL_FRACTIONAL_LOG2;
> +		case IIO_TEMP:
> +			/* T_scale (°C) = raw * V_REF (mV) / (-1.8 mV/°C *
> 2^16) */
> +			*val = st->vref_mv * -556;
> +			*val2 = 16;
> +			return IIO_VAL_FRACTIONAL_LOG2;
> +		default:
> +			return -EINVAL;
> +		}
> +	case IIO_CHAN_INFO_OFFSET:
> +		switch (chan->type) {
> +		case IIO_VOLTAGE:
> +			if (cfg->pin_pairing == AD4695_IN_PAIR_COM)
> +				*val = st->com_mv * (1 << realbits) / st-
> >vref_mv;
> +			else if (cfg->pin_pairing == AD4695_IN_PAIR_EVEN_ODD)
> +				*val = cfg->common_mode_mv * (1 << realbits)
> / st->vref_mv;
> +			else
> +				*val = 0;
> +
> +			return IIO_VAL_INT;
> +		case IIO_TEMP:
> +			/* T_offset (°C) = -725 mV / (-1.8 mV/°C) */
> +			/* T_offset (raw) = T_offset (°C) * (-1.8 mV/°C) *
> 2^16 / V_REF (mV) */
> +			*val = -47513600;
> +			*val2 = st->vref_mv;
> +			return IIO_VAL_FRACTIONAL;
> +		default:
> +			return -EINVAL;
> +		}
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int ad4695_debugfs_reg_access(struct iio_dev *indio_dev, unsigned int
> reg,
> +				     unsigned int writeval, unsigned int
> *readval)
> +{
> +	struct ad4695_state *st = iio_priv(indio_dev);
> +
> +	iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> +		if (readval)
> +			return regmap_read(st->regmap, reg, readval);
> +
> +		return regmap_write(st->regmap, reg, writeval);
> +	}
> +
> +	unreachable();
> +}
> +
> +static const struct iio_info ad4695_info = {
> +	.read_raw = &ad4695_read_raw,
> +	.debugfs_reg_access = &ad4695_debugfs_reg_access,
> +};
> +
> +static int ad4695_parse_channel_cfg(struct iio_dev *indio_dev)
> +{
> +	struct device *dev = indio_dev->dev.parent;
> +	struct ad4695_state *st = iio_priv(indio_dev);

Why not passing in struct ad4695_state directly?

...

> 
> +
> +	/* Needed for debugfs since it only access registers 1 byte at a
> time. */
> +	ret = regmap_set_bits(st->regmap, AD4695_REG_SPI_CONFIG_C,
> +			      AD4695_REG_SPI_CONFIG_C_MB_STRICT);
> +	if (ret)
> +		return ret;
> +

Question... do we gain something but not doing the above? Because debugfs is
optional and always doing it even when it's not present looks unnecessary. 

- Nuno Sá

Powered by blists - more mailing lists