On 12/18/24 8:37 AM, Marcelo Schmitt wrote:
> From: Ana-Maria Cusco <ana-maria.cusco@xxxxxxxxxx>
>
> From: Ana-Maria Cusco <ana-maria.cusco@xxxxxxxxxx>
>
> Add support for the AD4170 ADC with the following features:
> - Single-shot read (read_raw), scale, sampling freq
> - Multi channel buffer support
> - Buffered capture in triggered mode
> - Gain and offset calibration
> - chop_iexc and chop_adc device configuration
> - Powerdown switch configuration
>
> Co-developed-by: Dragos Bogdan <dragos.bogdan@xxxxxxxxxx>
> Signed-off-by: Dragos Bogdan <dragos.bogdan@xxxxxxxxxx>
> Signed-off-by: Ana-Maria Cusco <ana-maria.cusco@xxxxxxxxxx>
> Co-developed-by: Marcelo Schmitt <marcelo.schmitt@xxxxxxxxxx>
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@xxxxxxxxxx>
> ---
> drivers/iio/adc/Kconfig | 16 +
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/ad4170.c | 2049 ++++++++++++++++++++++++++++++++++++++
I know it is a pain to split this up into multiple patches, but this is really
too huge to try to review all at once! 500 lines changed or less per patch is
much more manageable.
Just commenting on a few things that jupmed out at me for now...
> drivers/iio/adc/ad4170.h | 316 ++++++
> 4 files changed, 2382 insertions(+)
> create mode 100644 drivers/iio/adc/ad4170.c
> create mode 100644 drivers/iio/adc/ad4170.h
>
> +
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/gpio/driver.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/units.h>
> +#include <linux/util_macros.h>
> +
> +#include <asm/div64.h>
Usually don't use asm but rather linux/math64.h.
> +#include <linux/unaligned.h>
> +
> +struct ad4170_state {
> + struct regmap *regmap;
> + struct spi_device *spi;
> + struct regulator_bulk_data supplies[7];
> + struct mutex lock; /* Protect filter, PGA, GPIO, chan read, chan config */
> + struct iio_chan_spec chans[AD4170_MAX_CHANNELS];
> + struct ad4170_chan_info chans_info[AD4170_MAX_CHANNELS];
> + struct ad4170_setup setups[AD4170_MAX_SETUPS];
> + u32 mclk_hz;
> + enum ad4170_pin_function pins_fn[AD4170_NUM_ANALOG_PINS];
> + enum ad4170_gpio_function gpio_fn[AD4170_NUM_GPIO_PINS];
> + unsigned int clock_ctrl;
> + struct clk *ext_clk;
> + struct clk_hw int_clk_hw;
> + int sps_tbl[ARRAY_SIZE(ad4170_filt_names)][ARRAY_SIZE(ad4170_filt_fs_tbl)][2];
> + struct completion completion;
> + struct iio_trigger *trig;
> + u32 data[AD4170_MAX_CHANNELS];
> +
> + struct spi_transfer xfer;
> + struct spi_message msg;
> + /*
> + * DMA (thus cache coherency maintenance) requires the transfer buffers
> + * to live in their own cache lines.
> + */
> + u8 reg_write_tx_buf[6];
> + __be32 reg_read_rx_buf __aligned(IIO_DMA_MINALIGN);
Looks like __align() needs to be moved up one line.
> + __be16 reg_read_tx_buf;
> +};
> +
> +static int ad4170_get_input_range(struct ad4170_state *st,
> + struct iio_chan_spec const *chan,
> + unsigned int ref_sel)
> +{
> + struct ad4170_chan_info *chan_info = &st->chans_info[chan->address];
> + struct ad4170_setup *setup = &st->setups[chan_info->setup_num];
> + bool bipolar = FIELD_GET(AD4170_AFE_BIPOLAR_MSK, setup->afe);
> + int refp, refn, ain_voltage, ret;
> +
> + switch (ref_sel) {
> + case AD4170_AFE_REFIN_REFIN1:
> + refp = regulator_get_voltage(st->supplies[AD4170_REFIN1P_SUP].consumer);
> + refn = regulator_get_voltage(st->supplies[AD4170_REFIN1N_SUP].consumer);
> + break;
> + case AD4170_AFE_REFIN_REFIN2:
> + refp = regulator_get_voltage(st->supplies[AD4170_REFIN2P_SUP].consumer);
> + refn = regulator_get_voltage(st->supplies[AD4170_REFIN2N_SUP].consumer);
> + break;
> + case AD4170_AFE_REFIN_AVDD:
> + refp = regulator_get_voltage(st->supplies[AD4170_AVDD_SUP].consumer);
> + ret = regulator_get_voltage(st->supplies[AD4170_AVSS_SUP].consumer);
> + /*
> + * TODO AVSS is actually optional.
> + * Should we handle -EPROBE_DEFER here?
Since this is calling regulator_get_voltage(), we won't get EPROBE_DEFER here.
> + */
> +
> +static int ad4170_clock_select(struct iio_dev *indio_dev)
> +{
> + struct ad4170_state *st = iio_priv(indio_dev);
> + struct device *dev = &st->spi->dev;
> + int ret;
> +
> + st->mclk_hz = AD4170_INT_FREQ_16MHZ;
> + ret = device_property_match_property_string(dev, "clock-names",
> + ad4170_clk_sel,
> + ARRAY_SIZE(ad4170_clk_sel));
> + if (ret < 0) {
> + /* Use internal clock reference */
> + st->clock_ctrl |= FIELD_PREP(AD4170_CLOCK_CTRL_CLOCKSEL_MSK,
> + AD4170_INTERNAL_OSC_OUTPUT);
> + return ad4170_register_clk_provider(indio_dev);
> + }
> +
> + /* Use external clock reference */
> + st->ext_clk = devm_clk_get(dev, ad4170_clk_sel[ret]);
devm_clk_get_enabled() ?
> + if (IS_ERR(st->ext_clk)) {
> + if (PTR_ERR(st->ext_clk) == -EPROBE_DEFER)
> + return -EPROBE_DEFER;
dev_err_probe() already does this check.
> +
> + return dev_err_probe(dev, PTR_ERR(st->ext_clk),
> + "Failed to get external clock\n");
> + }
> + st->clock_ctrl |= FIELD_PREP(AD4170_CLOCK_CTRL_CLOCKSEL_MSK,
> + AD4170_EXTERNAL_OSC + ret);
> +
> + ret = clk_prepare_enable(st->ext_clk);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "Failed to enable external clock\n");
> +
> + ret = devm_add_action_or_reset(dev, ad4170_clk_disable_unprepare,
> + st->ext_clk);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "Failed to add clock unwind action\n");
> +
> + st->mclk_hz = clk_get_rate(st->ext_clk);
> + if (st->mclk_hz < AD4170_EXT_FREQ_MHZ_MIN ||
> + st->mclk_hz > AD4170_EXT_FREQ_MHZ_MAX) {
> + return dev_err_probe(dev, -EINVAL,
> + "Invalid external clock frequency %u\n",
> + st->mclk_hz);
> + }
> + return 0;
> +}
> +
> +/* ADC Register Lengths */
> +static const unsigned int ad4170_reg_size[] = {
> + [AD4170_INTERFACE_CONFIG_A_REG] = 1,
> + [AD4170_STATUS_REG] = 2,
> + [AD4170_DATA_24b_REG] = 3,
> + [AD4170_PIN_MUXING_REG] = 2,
> + [AD4170_CLOCK_CTRL_REG] = 2,
> + [AD4170_POWER_DOWN_SW_REG] = 2,
> + [AD4170_ADC_CTRL_REG] = 2,
> + [AD4170_CHANNEL_EN_REG] = 2,
> + /*
> + * CHANNEL_SETUP and CHANNEL_MAP register are all 2 byte size each and
> + * their addresses are interleaved such that we have CHANNEL_SETUP0
> + * address followed by CHANNEL_MAP0 address, followed by CHANNEL_SETUP1,
> + * and so on until CHANNEL_MAP15.
> + * Thus, initialize the register size for them only once.
> + */
> + [AD4170_CHAN_SETUP_REG(0) ... AD4170_CHAN_MAP_REG(AD4170_MAX_CHANNELS - 1)] = 2,
> + /*
> + * MISC, AFE, FILTER, FILTER_FS, OFFSET, and GAIN register addresses are
> + * also interleaved but MISC, AFE, FILTER, FILTER_FS, OFFSET are 16-bit
> + * while OFFSET, GAIN are 24-bit registers so we can't init them all to
> + * the same size.
> + */
> + /* Init MISC register size */
> + [AD4170_MISC_REG(0)] = 2,
> + [AD4170_MISC_REG(1)] = 2,
> + [AD4170_MISC_REG(2)] = 2,
> + [AD4170_MISC_REG(3)] = 2,
> + [AD4170_MISC_REG(4)] = 2,
> + [AD4170_MISC_REG(5)] = 2,
> + [AD4170_MISC_REG(6)] = 2,
> + [AD4170_MISC_REG(7)] = 2,
> + /* Init AFE register size */
> + [AD4170_AFE_REG(0)] = 2,
> + [AD4170_AFE_REG(1)] = 2,
> + [AD4170_AFE_REG(2)] = 2,
> + [AD4170_AFE_REG(3)] = 2,
> + [AD4170_AFE_REG(4)] = 2,
> + [AD4170_AFE_REG(5)] = 2,
> + [AD4170_AFE_REG(6)] = 2,
> + [AD4170_AFE_REG(7)] = 2,
> + /* Init FILTER register size */
> + [AD4170_FILTER_REG(0)] = 2,
> + [AD4170_FILTER_REG(1)] = 2,
> + [AD4170_FILTER_REG(2)] = 2,
> + [AD4170_FILTER_REG(3)] = 2,
> + [AD4170_FILTER_REG(4)] = 2,
> + [AD4170_FILTER_REG(5)] = 2,
> + [AD4170_FILTER_REG(6)] = 2,
> + [AD4170_FILTER_REG(7)] = 2,
> + /* Init FILTER_FS register size */
> + [AD4170_FILTER_FS_REG(0)] = 2,
> + [AD4170_FILTER_FS_REG(1)] = 2,
> + [AD4170_FILTER_FS_REG(2)] = 2,
> + [AD4170_FILTER_FS_REG(3)] = 2,
> + [AD4170_FILTER_FS_REG(4)] = 2,
> + [AD4170_FILTER_FS_REG(5)] = 2,
> + [AD4170_FILTER_FS_REG(6)] = 2,
> + [AD4170_FILTER_FS_REG(7)] = 2,
> + /* Init OFFSET register size */
> + [AD4170_OFFSET_REG(0)] = 3,
> + [AD4170_OFFSET_REG(1)] = 3,
> + [AD4170_OFFSET_REG(2)] = 3,
> + [AD4170_OFFSET_REG(3)] = 3,
> + [AD4170_OFFSET_REG(4)] = 3,
> + [AD4170_OFFSET_REG(5)] = 3,
> + [AD4170_OFFSET_REG(6)] = 3,
> + [AD4170_OFFSET_REG(7)] = 3,
> + /* Init GAIN register size */
> + [AD4170_GAIN_REG(0)] = 3,
> + [AD4170_GAIN_REG(1)] = 3,
> + [AD4170_GAIN_REG(2)] = 3,
> + [AD4170_GAIN_REG(3)] = 3,
> + [AD4170_GAIN_REG(4)] = 3,
> + [AD4170_GAIN_REG(5)] = 3,
> + [AD4170_GAIN_REG(6)] = 3,
> + [AD4170_GAIN_REG(7)] = 3,
> + [AD4170_V_BIAS_REG] = 2,
> + [AD4170_CURRENT_SRC_REG(0) ... AD4170_CURRENT_SRC_REG(AD4170_NUM_CURRENT_SRC - 1)] = 2,
Another way to do this is to make 3 regmaps, one for each word size.
It's a bit more work to set up in the first place, but then makes
using it easier. (see ad4695 for an example)
> +};
