Re: [PATCH 5/5] iio: adc: ad7405: add ad7405 driver

[Jonathan Cameron]

On Mon, 24 Mar 2025 11:08:00 +0200
Pop Ioan Daniel <pop.ioan-daniel@xxxxxxxxxx> 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@xxxxxxxxxx>
Hi Pop,

I'll probably overlap with existing reviews but maybe there will
be other stuff.

Dragos is listed as the maintainer. If that's accurate should have a
Co-developed by and sign off.

> ---
>  drivers/iio/adc/Kconfig  |  10 ++
>  drivers/iio/adc/Makefile |   1 +
>  drivers/iio/adc/ad7405.c | 301 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 312 insertions(+)
>  create mode 100644 drivers/iio/adc/ad7405.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index f64b5faeb257..321a1ee7304f 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -203,6 +203,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
> +	help
> +	  Say yes here to build support for Analog Devices AD7405, ADUM7701,
> +	  ADUM7702, ADUM7703 analog to digital converters (ADC).

Maybe mention the bus?

> +
> +	  To compile this driver as a module, choose M here: the module will be
> +	  called ad7405.
> +

> diff --git a/drivers/iio/adc/ad7405.c b/drivers/iio/adc/ad7405.c
> new file mode 100644
> index 000000000000..40fe072369d5
> --- /dev/null
> +++ b/drivers/iio/adc/ad7405.c
> @@ -0,0 +1,301 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Analog Devices AD7405 driver
> + *
> + * Copyright 2025 Analog Devices Inc.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/log2.h>
> +#include <linux/clk.h>
> +#include <linux/platform_device.h>
> +#include <linux/of.h>

Use property.h if you need to access firmware properties.
For the table,  use
mod_devicetable.h not of.h.


> +#include <linux/iio/iio.h>
> +#include <linux/iio/backend.h>
> +#include <linux/util_macros.h>
> +#include <linux/regulator/consumer.h>
> +
> +#define AD7405_DEFAULT_DEC_RATE 1024
> +
> +const unsigned int ad7405_dec_rates[] = {
> +		4096, 2048, 1024, 512, 256, 128, 64, 32,

1 tab only is enough here.

> +};
> +
> +struct ad7405_chip_info {
> +	const char *name;
> +	unsigned int num_channels;
> +	unsigned int max_rate;
> +	unsigned int min_rate;
> +	struct iio_chan_spec channel[3];
Why 3? 

> +	const unsigned long *available_mask;
As below - this makes little sense. Drop it.

> +};
> +
> +struct ad7405_state {
> +	struct iio_backend *back;
> +	struct clk *axi_clk_gen;
> +	/* lock to protect multiple accesses to the device registers */

Why  I assume this is about read modify update sequences? 
If so say something more about that.  For now I can't see any
happening. All we have is an update of the backend sampling frequency.

> +	struct mutex lock;
> +	struct regmap *regmap;
Note used. Drop it. 
> +	struct iio_info iio_info;
As noted below, not obvious what this is for.

> +	const struct ad7405_chip_info *info;
> +	unsigned int sample_frequency_tbl[ARRAY_SIZE(ad7405_dec_rates)];
> +	unsigned int sample_frequency;
> +	unsigned int ref_frequency;
> +};
> +
> +static void ad7405_fill_samp_freq_table(struct ad7405_state *st)
> +{
> +	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]);
Wrap this line.
		st->sample_frequency_tbl[i] =
			DIV_ROUND_CLOSEST_ULL(st->ref_frequency, ad7405_dec_rates[i]);

> +}
> +
> +static int ad7405_set_sampling_rate(struct iio_dev *indio_dev,
> +				    const struct iio_chan_spec *chan,
> +				    unsigned int samp_rate)
> +{
> +	struct ad7405_state *st = iio_priv(indio_dev);
> +	unsigned int dec_rate, idx;
> +	int ret;
> +
> +	dec_rate = DIV_ROUND_CLOSEST_ULL(st->ref_frequency, samp_rate);
> +
> +	idx = find_closest_descending(dec_rate, ad7405_dec_rates,
> +				      ARRAY_SIZE(ad7405_dec_rates));
> +
> +	    dec_rate = ad7405_dec_rates[idx];

Odd indent.

> +
> +	ret = iio_backend_set_dec_rate(st->back, dec_rate);
> +	if (ret)
> +		return ret;
> +
> +	st->sample_frequency = DIV_ROUND_CLOSEST_ULL(st->ref_frequency, dec_rate);
> +
> +	return 0;
> +}
> +
> +static int ad7405_update_scan_mode(struct iio_dev *indio_dev,
> +				   const unsigned long *scan_mask)
> +{
> +	struct ad7405_state *st = iio_priv(indio_dev);
> +	unsigned int c;
> +	int ret;
> +

This is odd given you only have one channel.
Either do the enable in probe() or in postenable callback and
disable in predisable callback.


> +	for (c = 0; c < indio_dev->num_channels; c++) {
> +		if (test_bit(c, scan_mask))
> +			ret = iio_backend_chan_enable(st->back, c);
> +		else
> +			ret = iio_backend_chan_disable(st->back, c);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +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_SAMP_FREQ:
> +			*val = st->sample_frequency;
> +
> +			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_SAMP_FREQ:
> +
> +			return ad7405_set_sampling_rate(indio_dev, chan, val);
> +
> +	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)
> +{
> +	struct ad7405_state *st = iio_priv(indio_dev);
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +			*vals = st->sample_frequency_tbl;
> +			*length = ARRAY_SIZE(st->sample_frequency_tbl);
> +			*type = IIO_VAL_INT;
> +			return IIO_AVAIL_LIST;
> +	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,
> +	.update_scan_mode = ad7405_update_scan_mode,
> +};
> +
> +#define AD7405_IIO_CHANNEL(_chan, _bits, _sign)		  \
> +	{ .type = IIO_VOLTAGE,					  \
	{
		.type = IIO_VOLTAGE,					\
etc.

That is keep to normal formatting.  If you go over 80 chars for readability
reasons that is fine.
> +	  .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> +	  .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> +	  .indexed = 1,						 \
> +	  .channel = _chan,					 \

For now channel always 0 so just set to 0.
Bring in the parameter only when you need it.

> +	  .scan_type = {				\
> +		.sign = _sign,				\
> +		.realbits = _bits,			\
> +		.storagebits = 16,			\
> +		.shift = 0,				\

Never any need to set shift to 0. It's the obvious default and the c spec
ensures it is set to 0 if you leave it unspecified.

> +	  },						\
> +	}
> +
> +static const unsigned long ad7405_channel_masks[] = {
> +		BIT(0),

If there is only one channel this serves no useful purpose.
Drop it.

> +		0,
> +};
> +
> +static const struct ad7405_chip_info ad7405_chip_info = {
> +		.name = "AD7405",
> +		.max_rate = 625000UL,
> +		.min_rate = 4883UL,
> +		.num_channels = 1,
> +		.channel = {
> +			AD7405_IIO_CHANNEL(0, 16, 'u'),
> +		},
> +		.available_mask = ad7405_channel_masks,
> +};
> +
> +static const struct ad7405_chip_info adum7701_chip_info = {
> +		.name = "ADUM7701",
> +		.max_rate = 656250UL,
> +		.min_rate = 5127UL,
> +		.num_channels = 1,
> +		.channel = {
> +			AD7405_IIO_CHANNEL(0, 16, 'u'),
> +		},
> +		.available_mask = ad7405_channel_masks,
> +};

> +static int ad7405_probe(struct platform_device *pdev)
> +{
> +	const struct ad7405_chip_info *chip_info;
> +	struct device *dev = &pdev->dev;
> +	struct iio_dev *indio_dev;
> +	struct ad7405_state *st;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +
> +	ret = devm_mutex_init(dev, &st->lock);
> +	if (ret)
> +		return ret;
> +
> +	chip_info = &ad7405_chip_info;
> +
> +	platform_set_drvdata(pdev, indio_dev);
Is this used anywhere? If not drop it.

> +
> +	st->axi_clk_gen = devm_clk_get(dev, NULL);
> +	if (IS_ERR(st->axi_clk_gen))
> +		return PTR_ERR(st->axi_clk_gen);
> +
> +	ret = clk_prepare_enable(st->axi_clk_gen);

why not dev_clk_get_enabled()?

> +	if (ret)
> +		return ret;
> +
> +	ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(ad7405_power_supplies),
> +					     ad7405_power_supplies);
> +
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to get and enable supplies");
> +
> +	st->ref_frequency = clk_get_rate(st->axi_clk_gen);
> +
> +	ad7405_fill_samp_freq_table(st);
> +
> +	indio_dev->dev.parent = dev;

Set by the IIO core for you, so no need to do it explicitly unless you want
to modify it for a non obvious parent.


> +	indio_dev->name = pdev->dev.of_node->name;

First of all, don't use of_node directly. Always use the property.h accessors.
Secondly don't get the name from there - just hard code it so we can immediate
see what it is here. Given you are supporting multiple parts, and already
have it in the chip specific structure, just get it from there.


> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	indio_dev->channels = chip_info->channel;
> +	indio_dev->num_channels = chip_info->num_channels;
> +
> +	st->iio_info = ad7405_iio_info;
Why is the extra copy needed?  
> +	indio_dev->info = &st->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 = 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;
> +
> +	/* Reset all HDL Cores */

Needs more info.  Why is that needed for this specific part but not
for others?  I'd also expect the final one to be the devm case
if this sequence is needed.


> +	iio_backend_disable(st->back);
> +	iio_backend_enable(st->back);
> +
> +	ret = ad7405_set_sampling_rate(indio_dev, &indio_dev->channels[0],
> +				       chip_info->max_rate);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_iio_device_register(dev, indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	return 0;

	return devm_iio...

> +}
> +
> +/* 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 = &adum7701_chip_info, },
> +	{ .compatible = "adi,adum7703", .data = &adum7701_chip_info, },
> +	{ /* end of list */ },
> +};

Similar to below. Common to not have a blank line here given
close association of macro and table.

> +
> +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,
> +};
> +
Common practice to not have a blank line here as the association
between the following macro and the platform_driver structure is
very close.
> +module_platform_driver(ad7405_driver);
> +
> +MODULE_AUTHOR("Dragos Bogdan <dragos.bogdan@xxxxxxxxxx>");

As above. If this is correct then from + sign-offs above may be inapproprite.

> +MODULE_DESCRIPTION("Analog Devices AD7405 driver");
> +MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS("IIO_BACKEND");