Re: [PATCH v3 02/10] iio: adc: Add basic support for AD4170

["Nuno Sá" <noname.nuno@gmail.com>]

On Tue, 2025-05-13 at 09:34 -0300, Marcelo Schmitt wrote:
> From: Ana-Maria Cusco <ana-maria.cusco@analog.com>
> 
> The AD4170 is a multichannel, low noise, 24-bit precision sigma-delta
> analog to digital converter. The AD4170 design offers a flexible data
> aquisition solution with crosspoint multiplexed analog inputs, configurable
> ADC voltage reference inputs, ultra-low noise integrated PGA, digital
> filtering, wide range of configurable output data rates, internal
> oscillator and temperature sensor, four GPIOs, and integrated features for
> interfacing with load cell weigh scales, RTD, and thermocouple sensors.
> 
> Add basic support for the AD4170 ADC with the following features:
> - Single-shot read.
> - Analog front end PGA configuration.
> - Differential and pseudo-differential input configuration.
> 
> Signed-off-by: Ana-Maria Cusco <ana-maria.cusco@analog.com>
> Co-developed-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
> ---

Looks very good. Just some small notes...

> Change log v2 -> v3
> - Updated Copyright year.
> - Separated handling of channel setup cases for better understanding of code
> flow.
> - Now comparing setups field by field instead of using memcmp().
> - Disable channel on ad4170_read_sample() error path.
> - Reinit completion before entering single conversion mode.
> - Organized ad4170_sinc3_filt_fs_tbl.
> - Used clamp to simplify configuration value checking.
> - Returned earlier whenever possible.
> - Used HZ_PER_KHZ/MHZ.
> - Declared internal voltage reference constant AD4170_INT_REF_2_5V
> - Many other minor code style and readability improvements.
> 
>  MAINTAINERS              |    1 +
>  drivers/iio/adc/Kconfig  |   12 +
>  drivers/iio/adc/Makefile |    1 +
>  drivers/iio/adc/ad4170.c | 1553 ++++++++++++++++++++++++++++++++++++++
>  4 files changed, 1567 insertions(+)
>  create mode 100644 drivers/iio/adc/ad4170.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 0a8f2c7a139c..541e37ed304e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1352,6 +1352,7 @@ L:	linux-iio@vger.kernel.org
>  S:	Supported
>  W:	https://ez.analog.com/linux-software-drivers
>  F:	Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml
> +F:	drivers/iio/adc/ad4170.c
>  
>  ANALOG DEVICES INC AD4695 DRIVER
>  M:	Michael Hennerich <michael.hennerich@analog.com>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 0fe6601e59ed..594b9f55ec0a 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -70,6 +70,18 @@ config AD4130
>  	  To compile this driver as a module, choose M here: the module will
> be
>  	  called ad4130.
>  
> +
> +config AD4170
> +	tristate "Analog Device AD4170 ADC Driver"
> +	depends on SPI
> +	select REGMAP_SPI
> +	help
> +	  Say yes here to build support for Analog Devices AD4170 SPI analog
> +	  to digital converters (ADC).
> +
> +	  To compile this driver as a module, choose M here: the module will
> be
> +	  called ad4170.
> +
>  config AD4695
>  	tristate "Analog Device AD4695 ADC Driver"
>  	depends on SPI
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 07d4b832c42e..d3a1376d1f96 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -11,6 +11,7 @@ obj-$(CONFIG_AD_SIGMA_DELTA) += ad_sigma_delta.o
>  obj-$(CONFIG_AD4000) += ad4000.o
>  obj-$(CONFIG_AD4030) += ad4030.o
>  obj-$(CONFIG_AD4130) += ad4130.o
> +obj-$(CONFIG_AD4170) += ad4170.o
>  obj-$(CONFIG_AD4695) += ad4695.o
>  obj-$(CONFIG_AD4851) += ad4851.o
>  obj-$(CONFIG_AD7091R) += ad7091r-base.o
> diff --git a/drivers/iio/adc/ad4170.c b/drivers/iio/adc/ad4170.c
> new file mode 100644
> index 000000000000..bf19b31095ee
> --- /dev/null
> +++ b/drivers/iio/adc/ad4170.c
> @@ -0,0 +1,1553 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2025 Analog Devices, Inc.
> + * Author: Ana-Maria Cusco <ana-maria.cusco@analog.com>
> + * Author: Marcelo Schmitt <marcelo.schmitt@analog.com>
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bitmap.h>
> +#include <linux/bitops.h>
> +#include <linux/bits.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/iio/iio.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/math64.h>
> +#include <linux/module.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +#include <linux/unaligned.h>
> +#include <linux/units.h>
> +#include <linux/util_macros.h>
> 

...

> +
> +static int ad4170_debugfs_reg_access(struct iio_dev *indio_dev,
> +				     unsigned int reg, unsigned int writeval,
> +				     unsigned int *readval)
> +{
> +	struct ad4170_state *st = iio_priv(indio_dev);
> +
> +	if (readval)
> +		return regmap_read(st->regmap, reg, readval);
> +	else
> +		return regmap_write(st->regmap, reg, writeval);

redundant else

...

> 
> +
> +/*
> + * Sets the ADC operating mode. Supported modes are
> + * - Single conversion mode
> + * - Idle mode
> + */
> +static int ad4170_set_mode(struct ad4170_state *st, unsigned int mode)
> +{
> +	return regmap_update_bits(st->regmap, AD4170_ADC_CTRL_REG,
> +				  AD4170_ADC_CTRL_MODE_MSK,
> +				  FIELD_PREP(AD4170_ADC_CTRL_MODE_MSK,
> mode));
> +}

I'm usually not a fan of these wrappers.

...

> 
> +
> +static int ad4170_parse_firmware(struct iio_dev *indio_dev)
> +{
> +	struct ad4170_state *st = iio_priv(indio_dev);
> +	struct device *dev = &st->spi->dev;
> +	int reg_data, ret;
> +	unsigned int i;
> +
> +	st->mclk_hz = AD4170_INT_CLOCK_16MHZ;
> +
> +	for (i = 0; i < AD4170_NUM_ANALOG_PINS; i++)
> +		st->pins_fn[i] = AD4170_PIN_UNASIGNED;

Isn't the above the default already?


- Nuno Sá