Message-ID: <48ce035746cae8eab049ff178b6de194ead56960.camel@gmail.com>
Date: Fri, 02 May 2025 16:37:02 +0100
From: Nuno Sá <noname.nuno@...il.com>
To: Antoniu Miclaus <antoniu.miclaus@...log.com>, jic23@...nel.org, 
	robh@...nel.org, conor+dt@...nel.org, linux-iio@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 09/10] iio: adc: ad4080: add driver support

On Fri, 2025-05-02 at 11:59 +0300, Antoniu Miclaus wrote:
> Add support for AD4080 high-speed, low noise, low distortion,
> 20-bit, Easy Drive, successive approximation register (SAR)
> analog-to-digital converter (ADC).
> 
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@...log.com>
> ---
> changes in v4:
>  - drop _MSK postfix for bit definitions
>  - add space before }
>  - drop writing reserved bit.
>  - use regmap_set_bits/regmap_clear_bits instead of regmap_write
>  - use dev_dbg() where applies
>  - use dev_err() where applies
>  - use FIELD_PREP when _MSK defines are implied.
>  - drop explicit statement `.shift = 0`
>  - get clk_rate during probe.
>  - handle IIO_CHAN_INFO_SCALE and IIO_CHAN_INFO_SAMP_FREQ in default.
>  - use the new iio_backend_interface_data_align function
>  - drop redundant else.
>  - drop redundat mutex guards.
>  - rearrange includes.
>  - rename sinc5+pf1
>  - use regmap_get_device
>  - drop outermost ()
>  - check mode for < 2
>  - drop st->dec_rate and use functions inline.
>  - drop ad4080_channels[] and use ad4080_channel instead
>  - drop redundat if statement for num_lanes.
>  - use - instead of _ for device properties.
>  - drop reduandant assignement
>  - drop indio_dev->modes = INDIO_DIRECT_MODE;
>  - rename filter_disabled to filter_none.
>  MAINTAINERS              |   8 +
>  drivers/iio/adc/Kconfig  |  14 +
>  drivers/iio/adc/Makefile |   1 +
>  drivers/iio/adc/ad4080.c | 570 +++++++++++++++++++++++++++++++++++++++
>  4 files changed, 593 insertions(+)
>  create mode 100644 drivers/iio/adc/ad4080.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index bd04375ab4a2..0038f7a078ae 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1317,6 +1317,14 @@
> F:	Documentation/devicetree/bindings/iio/adc/adi,ad4030.yaml
>  F:	Documentation/iio/ad4030.rst
>  F:	drivers/iio/adc/ad4030.c
>  
> +ANALOG DEVICES INC AD4080 DRIVER
> +M:	Antoniu Miclaus <antoniu.miclaus@...log.com>
> +L:	linux-iio@...r.kernel.org
> +S:	Supported
> +W:	https://ez.analog.com/linux-software-drivers
> +F:	Documentation/devicetree/bindings/iio/adc/adi,ad4080.yaml
> +F:	drivers/iio/adc/ad4080.c
> +
>  ANALOG DEVICES INC AD4130 DRIVER
>  M:	Cosmin Tanislav <cosmin.tanislav@...log.com>
>  L:	linux-iio@...r.kernel.org
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 27413516216c..17df328f5322 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -47,6 +47,20 @@ config AD4030
>  	  To compile this driver as a module, choose M here: the module will
> be
>  	  called ad4030.
>  
> +config AD4080
> +	tristate "Analog Devices AD4080 high speed ADC"
> +	depends on SPI
> +	select REGMAP_SPI
> +	select IIO_BACKEND
> +	help
> +	  Say yes here to build support for Analog Devices AD4080
> +	  high speed, low noise, low distortion, 20-bit, Easy Drive,
> +	  successive approximation register (SAR) analog-to-digital
> +	  converter (ADC). Supports iio_backended devices for AD4080.
> +
> +	  To compile this driver as a module, choose M here: the module will
> be
> +	  called ad4080.
> +
>  config AD4130
>  	tristate "Analog Device AD4130 ADC Driver"
>  	depends on SPI
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 9f26d5eca822..e6efed5b4e7a 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -8,6 +8,7 @@ obj-$(CONFIG_AB8500_GPADC) += ab8500-gpadc.o
>  obj-$(CONFIG_AD_SIGMA_DELTA) += ad_sigma_delta.o
>  obj-$(CONFIG_AD4000) += ad4000.o
>  obj-$(CONFIG_AD4030) += ad4030.o
> +obj-$(CONFIG_AD4080) += ad4080.o
>  obj-$(CONFIG_AD4130) += ad4130.o
>  obj-$(CONFIG_AD4695) += ad4695.o
>  obj-$(CONFIG_AD4851) += ad4851.o
> diff --git a/drivers/iio/adc/ad4080.c b/drivers/iio/adc/ad4080.c
> new file mode 100644
> index 000000000000..9168dee9323e
> --- /dev/null
> +++ b/drivers/iio/adc/ad4080.c
> @@ -0,0 +1,570 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Analog Devices AD4080 SPI ADC driver
> + *
> + * Copyright 2025 Analog Devices Inc.
> + */
> +
> +#include <linux/array_size.h>
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/clk.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/iio/backend.h>
> +#include <linux/iio/iio.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +#include <linux/types.h>
> +#include <linux/unaligned.h>
> +#include <linux/units.h>
> +
> 

...

> +static int ad4080_lvds_sync_write(struct ad4080_state *st)
> +{
> +	struct device *dev = regmap_get_device(st->regmap);
> +	int ret;
> +
> +	ret = regmap_set_bits(st->regmap, AD4080_REG_ADC_DATA_INTF_CONFIG_A,
> +			      AD4080_ADC_DATA_INTF_CONFIG_A_INTF_CHK_EN);
> +	if (ret)
> +		return ret;
> +
> +	ret = iio_backend_interface_data_align(st->back, 100);
> +	if (ret == -ETIMEDOUT)
> +		dev_err(dev, "LVDS Sync Timeout.\n");
> +	if (ret)
> +		return ret;

Hmm I don't think the above two if() to be that helpful... Also,
ad4080_lvds_sync_write() seems to be only called from probe, so dev_err_probe()?

> +
> +	dev_dbg(dev, "Success: Pattern correct and Locked!\n");
> +	return regmap_clear_bits(st->regmap,
> AD4080_REG_ADC_DATA_INTF_CONFIG_A,
> +				 AD4080_ADC_DATA_INTF_CONFIG_A_INTF_CHK_EN);
> +}
> +
> +static ssize_t ad4080_get_filter_type(struct iio_dev *dev,
> +				      const struct iio_chan_spec *chan)
> +{
> +	struct ad4080_state *st = iio_priv(dev);
> +	unsigned int data;
> +	int ret;
> +
> +	ret = regmap_read(st->regmap, AD4080_REG_FILTER_CONFIG, &data);
> +	if (ret)
> +		return ret;
> +
> +	return FIELD_GET(AD4080_FILTER_CONFIG_FILTER_SEL_MSK, data);
> +}
> +
> +static int ad4080_set_filter_type(struct iio_dev *dev,
> +				  const struct iio_chan_spec *chan,
> +				  unsigned int mode)
> +{
> +	struct ad4080_state *st = iio_priv(dev);
> +	unsigned int dec_rate;
> +	int ret;
> +
> +	dec_rate = ad4080_get_dec_rate(dev, chan);
> +
> +	if (mode >= SINC_5 && dec_rate >= 512)
> +		return -EINVAL;
> +
> +	guard(mutex)(&st->lock);
> +	ret = iio_backend_filter_type_set(st->back, mode);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(st->regmap, AD4080_REG_FILTER_CONFIG,
> +				 AD4080_FILTER_CONFIG_FILTER_SEL_MSK,
> +				
> FIELD_PREP(AD4080_FILTER_CONFIG_FILTER_SEL_MSK,
> +					    mode));
> +	if (ret)
> +		return ret;
> +
> +	st->filter_type = mode;
> +

The above is not properly synchronized with ad4080_set_dec_rate().
ad4080_set_dec_rate() also needs to be protected by the same lock otherwise
dec_rate can be change under our feet when changing the filter. The same is true
for when updating the dec_rate.

I think that for ad4080_set_gec_rate() we do not really need to lock (i.e,
regmap should be fine)

> +	return 0;
> +}
> +
> +static int ad4080_read_avail(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     const int **vals, int *type, int *length,
> +			     long mask)
> +{
> +	switch (mask) {
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		*vals = ad4080_dec_rate_iio_enum;
> +		*length = ARRAY_SIZE(ad4080_dec_rate_iio_enum);
> +		*type = IIO_VAL_INT;
> +		return IIO_AVAIL_LIST;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct iio_info ad4080_iio_info = {
> +	.debugfs_reg_access = ad4080_reg_access,
> +	.read_raw = ad4080_read_raw,
> +	.write_raw = ad4080_write_raw,
> +	.read_avail = ad4080_read_avail,
> +};
> +
> +static const struct iio_enum ad4080_filter_type_enum = {
> +	.items = ad4080_filter_type_iio_enum,
> +	.num_items = ARRAY_SIZE(ad4080_filter_type_iio_enum),
> +	.set = ad4080_set_filter_type,
> +	.get = ad4080_get_filter_type,
> +};
> +
> +static struct iio_chan_spec_ext_info ad4080_ext_info[] = {
> +	IIO_ENUM("filter_type", IIO_SHARED_BY_ALL, &ad4080_filter_type_enum),
> +	IIO_ENUM_AVAILABLE("filter_type", IIO_SHARED_BY_ALL,
> +			   &ad4080_filter_type_enum),
> +	{ }
> +};
> +
> +static const struct iio_chan_spec ad4080_channel = {
> +	.type = IIO_VOLTAGE,
> +	.indexed = 1,
> +	.channel = 0,
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_SCALE),
> +	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
> +			BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
> +	.info_mask_shared_by_all_available =
> +			BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
> +	.ext_info = ad4080_ext_info,
> +	.scan_index = 0,
> +	.scan_type = {
> +		.sign = 's',
> +		.realbits = 20,
> +		.storagebits = 32,
> +	},
> +};
> +
> +static const struct ad4080_chip_info ad4080_chip_info = {
> +	.name = "AD4080",
> +	.product_id = AD4080_CHIP_ID,
> +	.scale_table = ad4080_scale_table,
> +	.num_scales = ARRAY_SIZE(ad4080_scale_table),
> +	.num_channels = 1,
> +	.channels = &ad4080_channel,
> +};
> +
> +static int ad4080_setup(struct iio_dev *indio_dev)
> +{
> +	struct ad4080_state *st = iio_priv(indio_dev);
> +	struct device *dev = regmap_get_device(st->regmap);
> +	unsigned int id;
> +	int ret;
> +
> +	ret = regmap_write(st->regmap, AD4080_REG_INTERFACE_CONFIG_A,
> +			   AD4080_INTERFACE_CONFIG_A_SW_RESET);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(st->regmap, AD4080_REG_INTERFACE_CONFIG_A,
> +			   AD4080_INTERFACE_CONFIG_A_SDO_ENABLE);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(st->regmap, AD4080_REG_CHIP_TYPE, &id);
> +	if (ret)
> +		return ret;
> +
> +	if (id != AD4080_CHIP_ID)
> +		dev_info(dev, "Unrecognized CHIP_ID 0x%X\n", id);
> +
> +	ret = regmap_set_bits(st->regmap, AD4080_REG_GPIO_CONFIG_A,
> +			      AD4080_GPIO_CONFIG_A_GPO_1_EN);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(st->regmap, AD4080_REG_GPIO_CONFIG_B,
> +			   FIELD_PREP(AD4080_GPIO_CONFIG_B_GPIO_1_SEL_MSK,
> 3));
> +	if (ret)
> +		return ret;
> +
> +	ret = iio_backend_num_lanes_set(st->back, st->num_lanes);
> +	if (ret)
> +		return ret;
> +
> +	if (!st->lvds_cnv_en)
> +		return 0;
> +
> +	ret = regmap_update_bits(st->regmap,
> +				 AD4080_REG_ADC_DATA_INTF_CONFIG_B,
> +				
> AD4080_ADC_DATA_INTF_CONFIG_B_LVDS_CNV_CLK_CNT_MSK,
> +				
> FIELD_PREP(AD4080_ADC_DATA_INTF_CONFIG_B_LVDS_CNV_CLK_CNT_MSK, 7));
> +	if (ret)
> +		return ret;
> +
> +	if (st->num_lanes != 1) {

I think st->num_lanes > 1 would make the intent more explicit...

> +		ret = regmap_set_bits(st->regmap,
> AD4080_REG_ADC_DATA_INTF_CONFIG_A,
> +				     
> AD4080_ADC_DATA_INTF_CONFIG_A_SPI_LVDS_LANES);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = regmap_set_bits(st->regmap,
> +			      AD4080_REG_ADC_DATA_INTF_CONFIG_B,
> +			      AD4080_ADC_DATA_INTF_CONFIG_B_LVDS_CNV_EN);
> +	if (ret)
> +		return ret;
> +
> +	return ad4080_lvds_sync_write(st);
> +}
> +
> +static void ad4080_properties_parse(struct ad4080_state *st)
> +{
> +	struct device *dev = regmap_get_device(st->regmap);
> +
> +	st->lvds_cnv_en = device_property_read_bool(dev, "adi,lvds-cnv-
> enable");
> +
> +	st->num_lanes = 1;
> +	device_property_read_u32(dev, "adi,num-lanes", &st->num_lanes);

we should validate the value given by DT...

- Nuno Sá

Powered by blists - more mailing lists