| Message ID | 20250324090813.2775011-6-pop.ioan-daniel@analog.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add support for AD7405/ADUM770x | expand |
On Mon, Mar 24, 2025 at 11:08:00AM +0200, 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. Datasheet: tag? > Signed-off-by: Pop Ioan Daniel <pop.ioan-daniel@analog.com> ... The list of headers is semi-random and unordered. Please, follow IWYU principle and fix the ordering as well. > +#include <linux/module.h> > +#include <linux/log2.h> > +#include <linux/clk.h> > +#include <linux/platform_device.h> > +#include <linux/of.h> No of.h in the new code, please. > +#include <linux/iio/iio.h> > +#include <linux/iio/backend.h> Move this group... > +#include <linux/util_macros.h> > +#include <linux/regulator/consumer.h> > + ...somewhere here as other (recent) drivers do. ... > +#define AD7405_DEFAULT_DEC_RATE 1024 What is the units? ... > +const unsigned int ad7405_dec_rates[] = { > + 4096, 2048, 1024, 512, 256, 128, 64, 32, Too much TABbed. > +}; ... > +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]; > + const unsigned long *available_mask; `pahole` has been run and this is the best choice, right? > +}; > + > +struct ad7405_state { Ditto. > + struct iio_backend *back; > + struct clk *axi_clk_gen; > + /* lock to protect multiple accesses to the device registers */ > + struct mutex lock; > + struct regmap *regmap; > + struct iio_info iio_info; > + 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; Why signed. > + > + 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]); This is too long even for relaxed mode... > +} ... > +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]; Something happened there... > + 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; > +} ... Okay, I'll stop here, this driver is not ready for upstream. Please, consult with your colleagues and do round of internal review before sending a new version.
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@analog.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@analog.com>"); > +MODULE_DESCRIPTION("Analog Devices AD7405 driver"); > +MODULE_LICENSE("GPL"); > +MODULE_IMPORT_NS("IIO_BACKEND");
On Mon, 24 Mar 2025 11:08:00 +0200 Pop Ioan Daniel <pop.ioan-daniel@analog.com> 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@analog.com> 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@analog.com>"); 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");
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> + +#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]; + const unsigned long *available_mask; +}; + +struct ad7405_state { + struct iio_backend *back; + struct clk *axi_clk_gen; + /* lock to protect multiple accesses to the device registers */ + struct mutex lock; + struct regmap *regmap; + struct iio_info iio_info; + 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]); +} + +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: + + 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, \ + .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, \ + .scan_type = { \ + .sign = _sign, \ + .realbits = _bits, \ + .storagebits = 16, \ + .shift = 0, \ + }, \ + } + +static const unsigned long ad7405_channel_masks[] = { + BIT(0), + 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 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; + + platform_set_drvdata(pdev, indio_dev); + + 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); + 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; + indio_dev->name = pdev->dev.of_node->name; + 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; + 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); + + 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; +} + +/* 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@analog.com>"); +MODULE_DESCRIPTION("Analog Devices AD7405 driver"); +MODULE_LICENSE("GPL"); +MODULE_IMPORT_NS("IIO_BACKEND");
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@analog.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