Message-ID: <4e18f195-fec2-4562-bbe5-69ff465e44d9@baylibre.com>
Date: Mon, 24 Mar 2025 14:09:34 -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>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Nuno Sa <nuno.sa@...log.com>,
 Olivier Moysan <olivier.moysan@...s.st.com>,
 Javier Carrasco <javier.carrasco.cruz@...il.com>,
 Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
 Guillaume Stols <gstols@...libre.com>, Trevor Gamblin
 <tgamblin@...libre.com>, Dumitru Ceclan <mitrutzceclan@...il.com>,
 Matteo Martelli <matteomartelli3@...il.com>,
 AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
 Alisa-Dariana Roman <alisadariana@...il.com>,
 Michael Walle <michael@...le.cc>, Herve Codina <herve.codina@...tlin.com>,
 Thomas Bonnefille <thomas.bonnefille@...tlin.com>,
 Dragos Bogdan <dragos.bogdan@...log.com>, linux-iio@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 5/5] iio: adc: ad7405: add ad7405 driver

On 3/24/25 4:08 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.
> 

Dragos is listed as the MODULE_AUTHOR, so would expect to see Co-developed-by:
and Signed-off-by: tags for him as well, assuming he wrote some of this code.

More info: https://docs.kernel.org/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by

> Signed-off-by: Pop Ioan Daniel <pop.ioan-daniel@...log.com>
> ---
>  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).
> +
> +	  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 ee19afba62b7..0c3c1c69b6b4 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -21,6 +21,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..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>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/backend.h>
> +#include <linux/util_macros.h>
> +#include <linux/regulator/consumer.h>

Sort the includes in alphabetical order. And prune headers that aren't used
like log2.h and of.h.

> +
> +#define AD7405_DEFAULT_DEC_RATE 1024
> +
> +const unsigned int ad7405_dec_rates[] = {
> +		4096, 2048, 1024, 512, 256, 128, 64, 32,
> +};
> +
> +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];

Currently, all chips only have one channel, so we can leave out num_channels
and not use an array for the single struct iio_chan_spec.

> +	const unsigned long *available_mask;
> +};
> +
> +struct ad7405_state {
> +	struct iio_backend *back;
> +	struct clk *axi_clk_gen;

Just call it clk. Also, if we don't need to access it outside of probe, we
don't need it in this struct.

> +	/* lock to protect multiple accesses to the device registers */
> +	struct mutex lock;
> +	struct regmap *regmap;

These are not used, so should be removed.

> +	struct iio_info iio_info;

Don't need to have a copy in this struct.

> +	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 to 80 chars.

> +}
> +
> +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];
> +
> +	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;
> +
> +	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:
> +

Need to return -EINVAL on val = 0 to avoid divide by zero crash.

> +			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;
> +	}
> +}

./scripts/checkpatch.pl should be catching issues with indentation style in the
functions above.

> +
> +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)		  \

chan, bits and sign are always the same, so we could omit these paramters.

> +	{ .type = IIO_VOLTAGE,					  \

We need info_mask_shared_by_type (or _separate) with IIO_CHAN_INFO_SCALE and
IIO_CHAN_INFO_OFFSET flags so that userspace knows how to convert raw data to
the standard unit of millivolts.

> +	  .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,					 \

Also needs .scan_index = _chan, .differential = 1, .channel2 = _chan + 1,

> +	  .scan_type = {				\
> +		.sign = _sign,				\
> +		.realbits = _bits,			\
> +		.storagebits = 16,			\
> +		.shift = 0,				\
> +	  },						\
> +	}
> +
> +static const unsigned long ad7405_channel_masks[] = {
> +		BIT(0),
> +		0,
> +};

This should not be need since there is only one channel.

> +
> +static const struct ad7405_chip_info ad7405_chip_info = {
> +		.name = "AD7405",
> +		.max_rate = 625000UL,
> +		.min_rate = 4883UL,

Doesn't the max rate depend on the clock frequency? So not sure how useful it
is to specify this.

min_rate is not used anywhere, so can be omitted.

> +		.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 const char * const ad7405_power_supplies[] = {
> +	"vdd1",	"vdd2",
> +};
> +
> +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;

This uses the same chip info for all chips and ignores the .data in the module
device table.

> +
> +	platform_set_drvdata(pdev, indio_dev);

There is no platform_get_drvdata(), so this is unnecessary.

> +
> +	st->axi_clk_gen = devm_clk_get(dev, NULL);

Can be simplified to devm_clk_get_enabled().

> +	if (IS_ERR(st->axi_clk_gen))
> +		return PTR_ERR(st->axi_clk_gen);
> +
> +	ret = clk_prepare_enable(st->axi_clk_gen);
> +	if (ret)
> +		return ret;

Otherwise we also need to add something to disable_unprepare the clock when
the driver is removed.

> +
> +	ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(ad7405_power_supplies),
> +					     ad7405_power_supplies);

I didn't see anything in the datasheet about power up sequencing, but typically
we would turn on power to the chip first before applying any other signals, like
the clock.

> +
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to get and enable supplies");
> +
> +	st->ref_frequency = clk_get_rate(st->axi_clk_gen);

Should check for return value of 0 and raise an error, otherwise we would get
divide by zero crash later.

> +
> +	ad7405_fill_samp_freq_table(st);
> +
> +	indio_dev->dev.parent = dev;
> +	indio_dev->name = pdev->dev.of_node->name;

I think usually this is chip_info->name rather than the DT node name.

> +	indio_dev->modes = INDIO_DIRECT_MODE;

IIO_CHAN_INFO_RAW isn't implemented, so INDIO_DIRECT_MODE should not be set.

> +
> +	indio_dev->channels = chip_info->channel;
> +	indio_dev->num_channels = chip_info->num_channels;
> +
> +	st->iio_info = ad7405_iio_info;
> +	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 */
> +	iio_backend_disable(st->back);
> +	iio_backend_enable(st->back);

Seems like this would be redunant and should be done implicitly by
devm_iio_backend_enable() (i.e. disable in adi_axi_adc_probe() so that 
devm_iio_backend_enable() brings it out of reset).

> +
> +	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);

Can just return directly here.

> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +/* 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 */ },
> +};
> +
> +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_DESCRIPTION("Analog Devices AD7405 driver");
> +MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS("IIO_BACKEND");

Powered by blists - more mailing lists