Message-ID: <ef807996-af5b-4b53-a81b-931ae49a6214@baylibre.com>
Date: Fri, 16 May 2025 10:08:03 -0500
From: David Lechner <dlechner@...libre.com>
To: Pop Ioan Daniel <pop.ioan-daniel@...log.com>,
 Lars-Peter Clausen <lars@...afoo.de>,
 Michael Hennerich <Michael.Hennerich@...log.com>,
 Jonathan Cameron <jic23@...nel.org>, Nuno Sá
 <nuno.sa@...log.com>, Andy Shevchenko <andy@...nel.org>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>,
 Sergiu Cuciurean <sergiu.cuciurean@...log.com>,
 Dragos Bogdan <dragos.bogdan@...log.com>,
 Antoniu Miclaus <antoniu.miclaus@...log.com>,
 Olivier Moysan <olivier.moysan@...s.st.com>,
 Javier Carrasco <javier.carrasco.cruz@...il.com>,
 Matti Vaittinen <mazziesaccount@...il.com>,
 Tobias Sperling <tobias.sperling@...ting.com>,
 Marcelo Schmitt <marcelo.schmitt@...log.com>,
 Alisa-Dariana Roman <alisadariana@...il.com>,
 Esteban Blanc <eblanc@...libre.com>, linux-iio@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 4/4] iio: adc: ad7405: add ad7405 driver

On 5/16/25 5:58 AM, Pop Ioan Daniel wrote:
> Add support for the AD7405/ADUM770x, a high performance isolated ADC,
> 1-channel, 16-bit with a second-order Σ-Δ modulator that converts an
> analog input signal into a high speed, single-bit data stream.
> 
> Signed-off-by: Pop Ioan Daniel <pop.ioan-daniel@...log.com>
> ---
> changes in v3:
>  - edit ad7405_chip_info struct instances
>  - remove lock
>  - add implementation for IIO_CHAN_INFO_SCALE
>  - use IIO_CHAN_INFO_OVERSAMPLING_RATIO for controlling the decimation rate
>  - use IIO_CHAN_INFO_SAMP_FREQ for read-only
>  - remove dem_clk_get_enabled() function
>  - remove chip_info variable from probe function
>  - fix indentation
>  - remove max_rate
>  - rename ad7405_set_sampling_rate in ad7405_det_dec_rate
> add adum7702 and adum7703 chip_info
>  drivers/iio/adc/Kconfig  |  10 ++
>  drivers/iio/adc/Makefile |   1 +
>  drivers/iio/adc/ad7405.c | 276 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 287 insertions(+)
>  create mode 100644 drivers/iio/adc/ad7405.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index ad06cf556785..6ed1042636d9 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -251,6 +251,16 @@ config AD7380
>  	  To compile this driver as a module, choose M here: the module will be
>  	  called ad7380.
>  
> +config AD7405
> +	tristate "Analog Device AD7405 ADC Driver"
> +	select IIO_BACKEND

It might make more sense to make this "depends" instead of "select".
Otherwise, this will show up for everyone, even if they can't actually
use it because they don't have a backend.

> +	help
> +	  Say yes here to build support for Analog Devices AD7405, ADUM7701,
> +	  ADUM7702, ADUM7703 analog to digital converters (ADC).
> +
> +	  To compile this driver as a module, choose M here: the module will be
> +	  called ad7405.
> +
>  config AD7476
>  	tristate "Analog Devices AD7476 1-channel ADCs driver and other similar devices from AD and TI"
>  	depends on SPI
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 07d4b832c42e..8115f30b7862 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -26,6 +26,7 @@ obj-$(CONFIG_AD7291) += ad7291.o
>  obj-$(CONFIG_AD7292) += ad7292.o
>  obj-$(CONFIG_AD7298) += ad7298.o
>  obj-$(CONFIG_AD7380) += ad7380.o
> +obj-$(CONFIG_AD7405) += ad7405.o
>  obj-$(CONFIG_AD7476) += ad7476.o
>  obj-$(CONFIG_AD7606_IFACE_PARALLEL) += ad7606_par.o
>  obj-$(CONFIG_AD7606_IFACE_SPI) += ad7606_spi.o
> diff --git a/drivers/iio/adc/ad7405.c b/drivers/iio/adc/ad7405.c
> new file mode 100644
> index 000000000000..1a96a283ab01
> --- /dev/null
> +++ b/drivers/iio/adc/ad7405.c
> @@ -0,0 +1,276 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Analog Devices AD7405 driver
> + *
> + * Copyright 2025 Analog Devices Inc.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/util_macros.h>
> +
> +#include <linux/iio/backend.h>
> +#include <linux/iio/iio.h>
> +
> +static const unsigned int ad7405_scale_table[][2] = {
> +	{640, 0},
> +};
> +
> +static const unsigned int adum7702_scale_table[][2] = {
> +	{128, 0},
> +};

Are there future plans for these scale tables? I don't see the point
since we only use the first number.

Also, it would make more sense to use the datasheet numbers of 320 and 64.
Otherwise, we need an explanation of where these values come from.

> +
> +static const unsigned int ad7405_dec_rates[] = {
> +	4096, 2048, 1024, 512, 256, 128, 64, 32,
> +};
> +
> +struct ad7405_chip_info {
> +	const char *name;
> +	struct iio_chan_spec channel;
> +	const unsigned int (*scale_table)[2];
> +};
> +
> +struct ad7405_state {
> +	struct iio_backend *back;
> +	const struct ad7405_chip_info *info;

> +	unsigned int sample_frequency_tbl[ARRAY_SIZE(ad7405_dec_rates)];

This is written but never read, so can be removed.

> +	unsigned int sample_frequency;

This is unused and can be removed.

> +	unsigned int ref_frequency;
> +	unsigned int dec_rate;
> +};
> +
> +static void ad7405_fill_samp_freq_table(struct ad7405_state *st)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(ad7405_dec_rates); i++)
> +		st->sample_frequency_tbl[i] =
> +			DIV_ROUND_CLOSEST_ULL(st->ref_frequency, ad7405_dec_rates[i]);
> +}
> +
> +static int ad7405_set_dec_rate(struct iio_dev *indio_dev,
> +			       const struct iio_chan_spec *chan,
> +			       unsigned int dec_rate)
> +{
> +	struct ad7405_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = iio_backend_oversampling_ratio_set(st->back, 0, dec_rate);

Should pass e.g. chan->scan_index here instead of 0. Otherwise chan param
is unused.

> +	if (ret)
> +		return ret;
> +
> +	st->dec_rate = dec_rate;
> +
> +	return 0;
> +}
> +
> +static int ad7405_get_scale(struct ad7405_state *st, int *val, int *val2)
> +{
> +	unsigned int tmp;
> +
> +	tmp = (st->info->scale_table[0][0] * 1000000ULL) >>
> +		    st->info->channel.scan_type.realbits;
> +	*val = tmp / 1000000;
> +	*val2 = tmp % 1000000;
> +
> +	return IIO_VAL_INT_PLUS_NANO;

As in the comment above about the scale_tables, the more typical way to do this
would be to store the full scale voltage in st->info->full_scale_mv (320 or 64),
then here:

	*val = st->info->full_scale_mv;
	*val2 = st->info->channel.scan_type.realbits - 1;

	return IIO_VAL_FRACTIONAL_LOG2;

> +}
> +
> +static int ad7405_read_raw(struct iio_dev *indio_dev,
> +			   const struct iio_chan_spec *chan, int *val,
> +			   int *val2, long info)
> +{
> +	struct ad7405_state *st = iio_priv(indio_dev);
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_SCALE:
> +		return ad7405_get_scale(st, val, val2);
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		*val = st->dec_rate;
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		*val = DIV_ROUND_CLOSEST_ULL(st->ref_frequency, st->dec_rate);
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int ad7405_write_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan, int val,
> +			    int val2, long info)
> +{
> +	switch (info) {
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		return ad7405_set_dec_rate(indio_dev, chan, val);
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		if (val < 1)
> +			return -EINVAL;
> +		return ad7405_set_dec_rate(indio_dev, chan, val);

I would be tempted to just leave the sampling_freqnecy read-only.
But if we want it to be writeable, we have to divide val by ref_freqnecy
to convert sampling freqnecy to decimation rate before calling
ad7405_set_dec_rate().

> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int ad7405_read_avail(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     const int **vals, int *type, int *length,
> +			     long info)
> +{
> +	switch (info) {
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		*vals = ad7405_dec_rates;
> +		*length = ARRAY_SIZE(ad7405_dec_rates);
> +		*type = IIO_VAL_INT;
> +		return IIO_AVAIL_LIST;

If we have a writeable sampling_frequency, the we should also have
case IIO_CHAN_INFO_SAMP_FREQ here. (But as above, I think not writeable
is fine, so this is OK as-is.)

> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct iio_info ad7405_iio_info = {
> +	.read_raw = &ad7405_read_raw,
> +	.write_raw = &ad7405_write_raw,
> +	.read_avail = &ad7405_read_avail,
> +};
> +
> +#define AD7405_IIO_CHANNEL {					\
> +	.type = IIO_VOLTAGE,					\
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
> +	.info_mask_shared_by_all = IIO_CHAN_INFO_SAMP_FREQ |	\
> +			BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),	\
> +	.info_mask_shared_by_all_available =			\
> +			BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),	\
> +	.indexed = 1,						\
> +	.channel = 0,						\
> +	.channel2 = 1,						\
> +	.differential = 1,					\
> +	.scan_index = 0,					\
> +	.scan_type = {						\
> +		.sign = 'u',					\

Since the raw value is unsigned but we can measure negative
differences, it means we need to implement IIO_CHAN_INFO_OFFSET
with a value of 2 << 15 so that usespace knows what value
is 0 volts.

> +		.realbits = 16,					\
> +		.storagebits = 16,				\
> +	},							\
> +}
> +
> +static const struct ad7405_chip_info ad7405_chip_info = {
> +	.name = "AD7405",
> +	.scale_table = ad7405_scale_table,
> +	.channel = AD7405_IIO_CHANNEL,
> +};
> +
> +static const struct ad7405_chip_info adum7701_chip_info = {
> +	.name = "ADUM7701",
> +	.scale_table = ad7405_scale_table,
> +	.channel = AD7405_IIO_CHANNEL,
> +};
> +
> +static const struct ad7405_chip_info adum7702_chip_info = {
> +	.name = "ADUM7702",
> +	.scale_table = adum7702_scale_table,
> +	.channel = AD7405_IIO_CHANNEL,
> +};
> +
> +static const struct ad7405_chip_info adum7703_chip_info = {
> +	.name = "ADUM7703",
> +	.scale_table = ad7405_scale_table,
> +	.channel = AD7405_IIO_CHANNEL,
> +};
> +
> +static const char * const ad7405_power_supplies[] = {
> +	"vdd1",	"vdd2",
> +};
> +
> +static int ad7405_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct iio_dev *indio_dev;
> +	struct ad7405_state *st;
> +	struct clk *clk;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +
> +	st->info = device_get_match_data(dev);
> +	if (!st->info)
> +		return dev_err_probe(dev, -EINVAL, "no chip info\n");
> +
> +	ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(ad7405_power_supplies),
> +					     ad7405_power_supplies);
> +

nit: no blank line here

> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to get and enable supplies");
> +
> +	clk = devm_clk_get_enabled(dev, NULL);
> +	if (IS_ERR(clk))
> +		return PTR_ERR(clk);
> +
> +	st->ref_frequency = clk_get_rate(clk);
> +	if (!st->ref_frequency)
> +		return -EINVAL;
> +
> +	ad7405_fill_samp_freq_table(st);
> +
> +	indio_dev->dev.parent = dev;
> +	indio_dev->name = st->info->name;
> +	indio_dev->channels = &st->info->channel;
> +	indio_dev->num_channels = 1;
> +	indio_dev->info = &ad7405_iio_info;
> +
> +	st->back = devm_iio_backend_get(dev, NULL);
> +	if (IS_ERR(st->back))
> +		return dev_err_probe(dev, PTR_ERR(st->back),
> +				     "failed to get IIO backend");
> +
> +	ret = iio_backend_chan_enable(st->back, 0);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_iio_backend_request_buffer(dev, st->back, indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_iio_backend_enable(dev, st->back);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad7405_set_dec_rate(indio_dev, &indio_dev->channels[0], 256);
> +	if (ret)
> +		return ret;
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}
> +
> +/* Match table for of_platform binding */
> +static const struct of_device_id ad7405_of_match[] = {
> +	{ .compatible = "adi,ad7405", .data = &ad7405_chip_info, },
> +	{ .compatible = "adi,adum7701", .data = &adum7701_chip_info, },
> +	{ .compatible = "adi,adum7702", .data = &adum7702_chip_info, },
> +	{ .compatible = "adi,adum7703", .data = &adum7703_chip_info, },
> +	{ },

no trailing comma please (iio subsystem style)

> +};
> +MODULE_DEVICE_TABLE(of, ad7405_of_match);
> +
> +static struct platform_driver ad7405_driver = {
> +	.driver = {
> +		.name = "ad7405",
> +		.owner = THIS_MODULE,
> +		.of_match_table = ad7405_of_match,
> +	},
> +	.probe = ad7405_probe,
> +};
> +module_platform_driver(ad7405_driver);
> +
> +MODULE_AUTHOR("Dragos Bogdan <dragos.bogdan@...log.com>");
> +MODULE_AUTHOR("Pop Ioan Daniel <pop.ioan-daniel@...log.com>");
> +MODULE_DESCRIPTION("Analog Devices AD7405 driver");
> +MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS("IIO_BACKEND");

Powered by blists - more mailing lists