[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <9c115086bd574b6c778a093143ebf54e14d7202b.camel@gmail.com>
Date: Thu, 10 Apr 2025 07:31:59 +0100
From: Nuno Sá <noname.nuno@...il.com>
To: Marcelo Schmitt <marcelo.schmitt@...log.com>, linux-iio@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Cc: Ana-Maria Cusco <ana-maria.cusco@...log.com>, jic23@...nel.org,
lars@...afoo.de, Michael.Hennerich@...log.com, dlechner@...libre.com,
nuno.sa@...log.com, andy@...nel.org, robh@...nel.org, krzk+dt@...nel.org,
conor+dt@...nel.org, marcelo.schmitt1@...il.com
Subject: Re: [PATCH v1 2/7] iio: adc: Add basic support for AD4170
Hi Marecelo,
First, superficial look...
On Wed, 2025-04-09 at 09:24 -0300, Marcelo Schmitt wrote:
> From: Ana-Maria Cusco <ana-maria.cusco@...log.com>
>
> Add support for the AD4170 ADC with the following features:
> - Single-shot read.
> - Analog front end PGA configuration.
> - Digital filter and sampling frequency configuration.
> - Calibration gain and offset configuration.
> - Differential and pseudo-differential input configuration.
>
> Signed-off-by: Ana-Maria Cusco <ana-maria.cusco@...log.com>
> Co-developed-by: Marcelo Schmitt <marcelo.schmitt@...log.com>
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@...log.com>
> ---
> MAINTAINERS | 1 +
> drivers/iio/adc/Kconfig | 16 +
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/ad4170.c | 1950 ++++++++++++++++++++++++++++++++++++++
> 4 files changed, 1968 insertions(+)
> create mode 100644 drivers/iio/adc/ad4170.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 991b6e2e373a..56cd87028dfd 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1343,6 +1343,7 @@ L: linux-iio@...r.kernel.org
> S: Supported
> W: https://ez.analog.com/linux-software-drivers
> F: Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml
> +F: drivers/iio/adc/ad4170.c
>
> ANALOG DEVICES INC AD4695 DRIVER
> M: Michael Hennerich <michael.hennerich@...log.com>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 636469392945..de7139fc2a1f 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -70,6 +70,22 @@ config AD4130
> To compile this driver as a module, choose M here: the module will be
> called ad4130.
>
> +
> +config AD4170
> + tristate "Analog Device AD4170 ADC Driver"
> + depends on SPI
> + depends on GPIOLIB
> + select IIO_BUFFER
> + select IIO_TRIGGERED_BUFFER
> + select REGMAP_SPI
> + depends on COMMON_CLK
> + help
> + Say yes here to build support for Analog Devices AD4170 SPI analog
> + to digital converters (ADC).
> +
> + To compile this driver as a module, choose M here: the module will be
> + called ad4170.
> +
> config AD4695
> tristate "Analog Device AD4695 ADC Driver"
> depends on SPI
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 07d4b832c42e..d3a1376d1f96 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -11,6 +11,7 @@ obj-$(CONFIG_AD_SIGMA_DELTA) += ad_sigma_delta.o
> obj-$(CONFIG_AD4000) += ad4000.o
> obj-$(CONFIG_AD4030) += ad4030.o
> obj-$(CONFIG_AD4130) += ad4130.o
> +obj-$(CONFIG_AD4170) += ad4170.o
> obj-$(CONFIG_AD4695) += ad4695.o
> obj-$(CONFIG_AD4851) += ad4851.o
> obj-$(CONFIG_AD7091R) += ad7091r-base.o
> diff --git a/drivers/iio/adc/ad4170.c b/drivers/iio/adc/ad4170.c
> new file mode 100644
> index 000000000000..0d24286ac2ab
> --- /dev/null
> +++ b/drivers/iio/adc/ad4170.c
> @@ -0,0 +1,1950 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2024 Analog Devices, Inc.
> + * Author: Ana-Maria Cusco <ana-maria.cusco@...log.com>
> + * Author: Marcelo Schmitt <marcelo.schmitt@...log.com>
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/kernel.h>
> +#include <linux/math64.h>
> +#include <linux/module.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +#include <linux/unaligned.h>
> +#include <linux/units.h>
> +#include <linux/util_macros.h>
> +
...
> +
> +static int ad4170_read_sample(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int *val)
> +{
> + struct ad4170_state *st = iio_priv(indio_dev);
> + int settling_time_ms, ret;
> +
> + guard(mutex)(&st->lock);
> + /*
> + * The ADC sequences through all enabled channels. That can lead to
> + * incorrect channel being sampled if a previous read would have left a
> + * different channel enabled. Thus, always enable and disable the
> + * channel on single-shot read.
> + */
> + ret = ad4170_set_channel_enable(st, chan->address, true);
> + if (ret)
> + return ret;
> +
> + reinit_completion(&st->completion);
I would do the above right before wait_for_completion_timeout()...
> +
> + ret = ad4170_set_mode(st, AD4170_ADC_CTRL_MODE_SINGLE);
> + if (ret)
> + goto err_disable;
>
...
> +
> +static int ad4170_set_pga(struct ad4170_state *st,
> + struct iio_chan_spec const *chan, int val, int val2)
> +{
> + struct ad4170_chan_info *chan_info = &st->chan_infos[chan->address];
> + struct ad4170_setup *setup = &chan_info->setup;
> + unsigned int old_pga = FIELD_GET(AD4170_AFE_PGA_GAIN_MSK, setup->afe);
> + unsigned int pga;
> + int ret = 0;
> +
> + for (pga = 0; pga < AD4170_NUM_PGA_OPTIONS; pga++) {
> + if (val == chan_info->scale_tbl[pga][0] &&
> + val2 == chan_info->scale_tbl[pga][1])
> + break;
> + }
> +
> + if (pga == AD4170_NUM_PGA_OPTIONS)
> + return -EINVAL;
> +
> + if (pga == old_pga)
> + return 0;
> +
> + setup->afe &= ~AD4170_AFE_PGA_GAIN_MSK;
> + setup->afe |= FIELD_PREP(AD4170_AFE_PGA_GAIN_MSK, pga);
>
ditto...
> +
> + guard(mutex)(&st->lock);
> + ret = ad4170_write_channel_setup(st, chan->address, false);
> + if (ret) {
> + setup->afe &= ~AD4170_AFE_PGA_GAIN_MSK;
> + setup->afe |= FIELD_PREP(AD4170_AFE_PGA_GAIN_MSK, old_pga);
> + }
> +
> + return ret;
> +}
> +
> +static int ad4170_set_channel_freq(struct ad4170_state *st,
> + struct iio_chan_spec const *chan, int val,
> + int val2)
> +{
> + struct ad4170_chan_info *chan_info = &st->chan_infos[chan->address];
> + struct ad4170_setup *setup = &chan_info->setup;
> + enum ad4170_filter_type f_type = __ad4170_get_filter_type(setup->filter);
> + int filt_fs_tbl_size, i, ret = 0;
> + unsigned int old_filter_fs;
> +
> + switch (f_type) {
> + case AD4170_SINC5_AVG:
> + fallthrough;
> + case AD4170_SINC3:
> + filt_fs_tbl_size = ARRAY_SIZE(ad4170_sinc3_filt_fs_tbl);
> + break;
> + case AD4170_SINC5:
> + filt_fs_tbl_size = ARRAY_SIZE(ad4170_sinc5_filt_fs_tbl);
> + break;
> + }
> +
> + for (i = 0; i < filt_fs_tbl_size; i++) {
> + if (st->sps_tbl[f_type][i][0] == val &&
> + st->sps_tbl[f_type][i][1] == val2)
> + break;
> + }
> + if (i >= filt_fs_tbl_size)
> + return -EINVAL;
> +
> + old_filter_fs = setup->filter_fs;
> + if (f_type == AD4170_SINC5)
> + setup->filter_fs = ad4170_sinc5_filt_fs_tbl[i];
> + else
> + setup->filter_fs = ad4170_sinc3_filt_fs_tbl[i];
> +
> + guard(mutex)(&st->lock);
Shouldn't the lock also protect the 'setup' struct?
> + ret = ad4170_write_channel_setup(st, chan->address, false);
> + if (ret)
> + setup->filter_fs = old_filter_fs;
> +
> + return ret;
> +}
> +
> +static int ad4170_set_calib_offset(struct ad4170_state *st,
> + struct iio_chan_spec const *chan, int val)
> +{
> + struct ad4170_chan_info *chan_info = &st->chan_infos[chan->address];
> + struct ad4170_setup *setup = &chan_info->setup;
> + u32 old_offset;
> + int ret;
> +
> + old_offset = setup->offset;
> + setup->offset = val;
> +
> + guard(mutex)(&st->lock);
> + ret = ad4170_write_channel_setup(st, chan->address, false);
> + if (ret)
> + setup->offset = old_offset;
> +
> + return ret;
> +}
> +
> +static int ad4170_set_calib_gain(struct ad4170_state *st,
> + struct iio_chan_spec const *chan, int val)
> +{
> + struct ad4170_chan_info *chan_info = &st->chan_infos[chan->address];
> + struct ad4170_setup *setup = &chan_info->setup;
> + u32 old_gain;
> + int ret;
> +
> + old_gain = setup->gain;
> + setup->gain = val;
> +
> + guard(mutex)(&st->lock);
> + ret = ad4170_write_channel_setup(st, chan->address, false);
> + if (ret)
> + setup->gain = old_gain;
> +
> + return ret;
> +}
> +
> +static int __ad4170_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int val,
> + int val2, long info)
> +{
> + struct ad4170_state *st = iio_priv(indio_dev);
> +
> + switch (info) {
> + case IIO_CHAN_INFO_SCALE:
> + return ad4170_set_pga(st, chan, val, val2);
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + return ad4170_set_channel_freq(st, chan, val, val2);
> + case IIO_CHAN_INFO_CALIBBIAS:
> + return ad4170_set_calib_offset(st, chan, val);
> + case IIO_CHAN_INFO_CALIBSCALE:
> + return ad4170_set_calib_gain(st, chan, val);
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int ad4170_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int val,
> + int val2, long info)
> +{
> + int ret;
> +
> + if (!iio_device_claim_direct(indio_dev))
> + return -EBUSY;
> +
> + ret = __ad4170_write_raw(indio_dev, chan, val, val2, info);
> + iio_device_release_direct(indio_dev);
> + return ret;
> +}
> +
> +static int ad4170_write_raw_get_fmt(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + long info)
> +{
> + switch (info) {
> + case IIO_CHAN_INFO_SCALE:
> + return IIO_VAL_INT_PLUS_NANO;
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + return IIO_VAL_INT_PLUS_MICRO;
> + case IIO_CHAN_INFO_CALIBBIAS:
> + case IIO_CHAN_INFO_CALIBSCALE:
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static const struct iio_info ad4170_info = {
> + .read_raw = ad4170_read_raw,
> + .read_avail = ad4170_read_avail,
> + .write_raw = ad4170_write_raw,
> + .write_raw_get_fmt = ad4170_write_raw_get_fmt,
> + .debugfs_reg_access = ad4170_debugfs_reg_access,
> +};
> +
> +static int ad4170_soft_reset(struct ad4170_state *st)
> +{
> + int ret;
> +
> + ret = regmap_write(st->regmap8, AD4170_CONFIG_A_REG,
> + AD4170_SW_RESET_MSK);
> + if (ret)
> + return ret;
> +
> + /* AD4170-4 requires 1 ms between reset and any register access. */
> + fsleep(MILLI);
> +
> + return 0;
> +}
> +
> +static int ad4170_parse_reference(struct ad4170_state *st,
> + struct fwnode_handle *child,
> + struct ad4170_setup *setup)
> +{
> + struct device *dev = &st->spi->dev;
> + int ret;
> + u8 aux;
> +
> + /* Positive reference buffer setup */
> + aux = AD4170_REF_BUF_PRE; /* Default to have precharge buffer enabled. */
> + ret = fwnode_property_read_u8(child, "adi,buffered-positive", &aux);
> + if (ret) {
Shouldn't this be if (!ret)?
> + if (aux < AD4170_REF_BUF_PRE || aux > AD4170_REF_BUF_BYPASS)
> + return dev_err_probe(dev, -EINVAL,
> + "Invalid adi,buffered-positive:
> %u\n",
> + aux);
> + }
> + setup->afe |= FIELD_PREP(AD4170_AFE_REF_BUF_P_MSK, aux);
> +
> + /* Negative reference buffer setup */
> + aux = AD4170_REF_BUF_PRE; /* Default to have precharge buffer enabled. */
> + ret = fwnode_property_read_u8(child, "adi,buffered-negative", &aux);
> + if (ret) {
ditto
> + if (aux < AD4170_REF_BUF_PRE || aux > AD4170_REF_BUF_BYPASS)
> + return dev_err_probe(dev, -EINVAL,
> + "Invalid adi,buffered-negative:
> %u\n",
> + aux);
> + }
> + setup->afe |= FIELD_PREP(AD4170_AFE_REF_BUF_M_MSK, aux);
> +
> + /* Voltage reference selection */
> + aux = AD4170_REF_REFOUT; /* Default reference selection. */
> + fwnode_property_read_u8(child, "adi,reference-select", &aux);
> + if (aux > AD4170_REF_AVDD)
> + return dev_err_probe(dev, -EINVAL,
> + "Invalid reference selected %u\n", aux);
> + setup->afe |= FIELD_PREP(AD4170_AFE_REF_SELECT_MSK, aux);
> +
> + return 0;
> +}
> +
> +static int ad4170_parse_adc_channel_type(struct device *dev,
> + struct fwnode_handle *child,
> + struct iio_chan_spec *chan)
> +{
> + u32 pins[2];
> + int ret;
> +
> + ret = fwnode_property_read_u32_array(child, "diff-channels", pins,
> + ARRAY_SIZE(pins));
> + if (!ret) {
> + chan->differential = true;
> + chan->channel = pins[0];
> + chan->channel2 = pins[1];
> + return 0;
> + }
> + ret = fwnode_property_read_u32(child, "single-channel", &pins[0]);
> + if (!ret) {
> + chan->differential = false;
> + chan->channel = pins[0];
> +
> + ret = fwnode_property_read_u32(child, "common-mode-channel",
> + &pins[1]);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "single-ended channels must define common-mode-
> channel\n");
> +
> + chan->channel2 = pins[1];
> + return 0;
> + }
Kind of a nitpick but for the above I would flip the logic. First check for errors in
the single-channel case and then you can have one less of level of indentation...
> + return dev_err_probe(dev, ret,
> + "Channel must define one of diff-channels or single-channel.\n");
> +}
> +
> +static int ad4170_parse_channel_node(struct iio_dev *indio_dev,
> + struct fwnode_handle *child,
> + unsigned int chan_num)
> +{
> + struct ad4170_state *st = iio_priv(indio_dev);
> + struct device *dev = &st->spi->dev;
> + struct ad4170_chan_info *chan_info;
> + struct ad4170_setup *setup;
> + struct iio_chan_spec *chan;
> + unsigned int ch_reg;
> + u8 ref_select;
> + bool bipolar;
> + int ret;
> +
> + ret = fwnode_property_read_u32(child, "reg", &ch_reg);
> + if (ret)
> + return ret;
> +
Could also deserve a log message?
> + if (ch_reg >= AD4170_MAX_CHANNELS)
> + return dev_err_probe(dev, -EINVAL,
> + "Channel idx greater than no of channels\n");
>
...
>
> +
> +static int ad4170_trigger_setup(struct iio_dev *indio_dev)
> +{
> + struct ad4170_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + st->trig = devm_iio_trigger_alloc(indio_dev->dev.parent, "%s-trig%d",
> + indio_dev->name,
> + iio_device_id(indio_dev));
> + if (!st->trig)
> + return -ENOMEM;
> +
> + st->trig->ops = &ad4170_trigger_ops;
> + st->trig->dev.parent = indio_dev->dev.parent;
> +
> + iio_trigger_set_drvdata(st->trig, indio_dev);
> + ret = devm_iio_trigger_register(indio_dev->dev.parent, st->trig);
> + if (ret)
> + return ret;
> +
> + indio_dev->trig = iio_trigger_get(st->trig);
> +
> + return request_irq(st->spi->irq, &ad4170_irq_handler, IRQF_ONESHOT,
> + indio_dev->name, indio_dev);
devm_request_irq()...
> +}
> +
> +static int ad4170_regulator_setup(struct ad4170_state *st)
> +{
> + struct device *dev = &st->spi->dev;
> + int ret;
> +
> + /* Required regulators */
> + ret = devm_regulator_get_enable_read_voltage(dev, "avdd");
> + if (ret < 0)
> + return dev_err_probe(dev, ret, "Failed to get AVDD voltage.\n");
> +
> + st->vrefs_uv[AD4170_AVDD_SUP] = ret;
> +
> + ret = devm_regulator_get_enable_read_voltage(dev, "iovdd");
> + if (ret < 0)
> + return dev_err_probe(dev, ret, "Failed to get IOVDD voltage.\n");
> +
> + st->vrefs_uv[AD4170_IOVDD_SUP] = ret;
> +
> + /* Optional regulators */
> + ret = devm_regulator_get_enable_read_voltage(dev, "avss");
> + if (ret < 0 && ret != -ENODEV)
> + return dev_err_probe(dev, ret, "Failed to get AVSS voltage.\n");
> +
> + /* Assume AVSS at GND (0V) if not provided */
> + st->vrefs_uv[AD4170_AVSS_SUP] = ret == -ENODEV ? 0 : -ret;
> +
> + ret = devm_regulator_get_enable_read_voltage(dev, "refin1p");
> + if (ret < 0 && ret != -ENODEV)
> + return dev_err_probe(dev, ret, "Failed to get REFIN+ voltage.\n");
> +
> + st->vrefs_uv[AD4170_REFIN1P_SUP] = ret;
> +
> + ret = devm_regulator_get_enable_read_voltage(dev, "refin1n");
> + if (ret < 0 && ret != -ENODEV)
> + return dev_err_probe(dev, ret, "Failed to get REFIN- voltage.\n");
> +
> + /* Negative supplies are assumed to provide negative voltage */
> + st->vrefs_uv[AD4170_REFIN1N_SUP] = ret == -ENODEV ? -ENODEV : -ret;
Maybe to early for me but the comment does not make it clear to me why the negation?
Won't the regulator return a negative voltage?
> +
> + ret = devm_regulator_get_enable_read_voltage(dev, "refin2p");
> + if (ret < 0 && ret != -ENODEV)
> + return dev_err_probe(dev, ret, "Failed to get REFIN2+
> voltage.\n");
> +
> + st->vrefs_uv[AD4170_REFIN2P_SUP] = ret;
> +
> + ret = devm_regulator_get_enable_read_voltage(dev, "refin2n");
> + if (ret < 0 && ret != -ENODEV)
> + return dev_err_probe(dev, ret, "Failed to get REFIN2-
> voltage.\n");
> +
> + /* Negative supplies are assumed to provide negative voltage */
> + st->vrefs_uv[AD4170_REFIN2N_SUP] = ret == -ENODEV ? -ENODEV : -ret;
> +
> + return 0;
> +}
> +
> +static int ad4170_probe(struct spi_device *spi)
> +{
> + struct device *dev = &spi->dev;
> + struct iio_dev *indio_dev;
> + struct ad4170_state *st;
> + const char *dev_name;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + st = iio_priv(indio_dev);
> + devm_mutex_init(dev, &st->lock);
check for errors...
> +
> + dev_name = spi_get_device_match_data(spi);
> + if (!dev_name)
> + return -EINVAL;
> +
> + indio_dev->name = dev_name;
> + indio_dev->info = &ad4170_info;
> +
> + st->spi = spi;
> + st->regmap8 = devm_regmap_init_spi(spi, &ad4170_regmap8_config);
> + if (IS_ERR(st->regmap8))
> + return dev_err_probe(dev, PTR_ERR(st->regmap8),
> + "Failed to initialize regmap8\n");
> +
> + st->regmap16 = devm_regmap_init_spi(spi, &ad4170_regmap16_config);
> + if (IS_ERR(st->regmap16))
> + return dev_err_probe(dev, PTR_ERR(st->regmap16),
> + "Failed to initialize regmap16\n");
> +
> + st->regmap24 = devm_regmap_init_spi(spi, &ad4170_regmap24_config);
> + if (IS_ERR(st->regmap24))
> + return dev_err_probe(dev, PTR_ERR(st->regmap24),
> + "Failed to initialize regmap24\n");
> +
Hmm, interesting idea... but I would expect an explanation on why can't we have bulk
reads for the 16 and 24 bit cases? Without it, I have to ask why not?
> + ret = ad4170_regulator_setup(st);
> + if (ret)
> + return ret;
> +
> + ret = ad4170_soft_reset(st);
> + if (ret)
> + return ret;
> +
> + ret = ad4170_parse_firmware(indio_dev);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to parse firmware\n");
> +
> + ret = ad4170_initial_config(indio_dev);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to setup device\n");
> +
> + init_completion(&st->completion);
> +
> + if (spi->irq) {
> + ret = ad4170_trigger_setup(indio_dev);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to setup
> trigger\n");
Typically it's better to log the errors inside ad4170_trigger_setup() unless you use
it outside probe.
- Nuno Sá
>
> +MODULE_AUTHOR("Marcelo Schmitt <marcelo.schmitt@...log.com>");
> +MODULE_DESCRIPTION("Analog Devices AD4170 SPI driver");
> +MODULE_LICENSE("GPL");
Powered by blists - more mailing lists