Hi David, minor stuff from me.. On Mon, 2024-06-24 at 17:01 -0500, David Lechner wrote: > This is a new driver for Analog Devices Inc. AD4695 and similar ADCs. > The initial driver supports initializing the chip including configuring > all possible LDO and reference voltage sources as well as any possible > voltage input channel wiring configuration. > > Only the 4-wire SPI wiring mode where the CNV pin is tied to the CS pin > is supported at this time. And reading sample data from the ADC can only > be done in direct mode for now. > > Co-developed-by: Ramona Gradinariu <ramona.gradinariu@xxxxxxxxxx> > Signed-off-by: Ramona Gradinariu <ramona.gradinariu@xxxxxxxxxx> > Signed-off-by: David Lechner <dlechner@xxxxxxxxxxxx> > --- > > v3 changes: > * always include all channels, not just ones customized in DT > * rework channel fwnode parsing for DT changes > > v2 changes: > * rework register definition macros > * remove code structure comments at top level > * remove simple wrapper functions around regmap functions > * rework channel fwnode parsing for DT changes > * fix missing return value check > --- > MAINTAINERS | 1 + > drivers/iio/adc/Kconfig | 11 + > drivers/iio/adc/Makefile | 1 + > drivers/iio/adc/ad4695.c | 730 > +++++++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 743 insertions(+) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 025ee138a713..c52c4b0e69a8 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -1216,6 +1216,7 @@ L: linux-iio@xxxxxxxxxxxxxxx > S: Supported > W: https://ez.analog.com/linux-software-drivers > F: Documentation/devicetree/bindings/iio/adc/adi,ad4695.yaml > +F: drivers/iio/adc/ad4695.c > F: include/dt-bindings/iio/adi,ad4695.h > > ANALOG DEVICES INC AD7091R DRIVER > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig > index b8184706c7d1..eed5547a5078 100644 > --- a/drivers/iio/adc/Kconfig > +++ b/drivers/iio/adc/Kconfig > @@ -36,6 +36,17 @@ config AD4130 > To compile this driver as a module, choose M here: the module will > be > called ad4130. > > +config AD4695 > + tristate "Analog Device AD4695 ADC Driver" > + depends on SPI > + select REGMAP_SPI > + help > + Say yes here to build support for Analog Devices AD4695 and similar > + analog to digital converters (ADC). > + > + To compile this driver as a module, choose M here: the module will > be > + called ad4695. > + > config AD7091R > tristate > > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile > index 51298c52b223..c180c85b6327 100644 > --- a/drivers/iio/adc/Makefile > +++ b/drivers/iio/adc/Makefile > @@ -7,6 +7,7 @@ > obj-$(CONFIG_AB8500_GPADC) += ab8500-gpadc.o > obj-$(CONFIG_AD_SIGMA_DELTA) += ad_sigma_delta.o > obj-$(CONFIG_AD4130) += ad4130.o > +obj-$(CONFIG_AD4695) += ad4695.o > obj-$(CONFIG_AD7091R) += ad7091r-base.o > obj-$(CONFIG_AD7091R5) += ad7091r5.o > obj-$(CONFIG_AD7091R8) += ad7091r8.o > diff --git a/drivers/iio/adc/ad4695.c b/drivers/iio/adc/ad4695.c > new file mode 100644 > index 000000000000..72ab5b997f6e > --- /dev/null > +++ b/drivers/iio/adc/ad4695.c > @@ -0,0 +1,730 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * SPI ADC driver for Analog Devices Inc. AD4695 and similar chips > + * > + * https://www.analog.com/en/products/ad4695.html > + * https://www.analog.com/en/products/ad4696.html > + * https://www.analog.com/en/products/ad4697.html > + * https://www.analog.com/en/products/ad4698.html > + * > + * Copyright 2024 Analog Devices Inc. > + * Copyright 2024 BayLibre, SAS > + */ > + > +#include <dt-bindings/iio/adi,ad4695.h> > +#include <linux/bitfield.h> > +#include <linux/bits.h> > +#include <linux/compiler.h> > +#include <linux/delay.h> > +#include <linux/device.h> > +#include <linux/err.h> > +#include <linux/gpio/consumer.h> > +#include <linux/iio/iio.h> > +#include <linux/property.h> > +#include <linux/regmap.h> > +#include <linux/regulator/consumer.h> > +#include <linux/spi/spi.h> > +#include <linux/units.h> > + > ... > + > +static int ad4695_write_chn_cfg(struct ad4695_state *st, > + struct ad4695_channel_config *cfg) > +{ > + u32 mask = 0, val = 0; > + > + mask |= AD4695_REG_CONFIG_IN_MODE; > + val |= FIELD_PREP(AD4695_REG_CONFIG_IN_MODE, cfg->bipolar ? 1 : 0); > + nit: don't need to OR the first assignments and so initializing the variables. > + mask |= AD4695_REG_CONFIG_IN_PAIR; > + val |= FIELD_PREP(AD4695_REG_CONFIG_IN_PAIR, cfg->pin_pairing); > + > + mask |= AD4695_REG_CONFIG_IN_AINHIGHZ_EN; > + val |= FIELD_PREP(AD4695_REG_CONFIG_IN_AINHIGHZ_EN, cfg->highz_en ? 1 > : 0); > + > + return regmap_update_bits(st->regmap, AD4695_REG_CONFIG_IN(cfg- > >channel), > + mask, val); > +} > + > +/** > + * ad4695_read_one_sample - Read a single sample using single-cycle mode > + * @st: The AD4695 state > + * @address: The address of the channel to read > + * > + * Upon return, the sample will be stored in the raw_data field of @st. > + * > + * Context: can sleep, must be called with iio_device_claim_direct held > + * Return: 0 on success, a negative error code on failure > + */ > +static int ad4695_read_one_sample(struct ad4695_state *st, unsigned int > address) > +{ > + struct spi_transfer xfer[2] = { }; > + int ret; > + > + ret = ad4695_set_single_cycle_mode(st, address); > + if (ret) > + return ret; > + > + /* > + * Setting the first channel to the temperature channel isn't > supported > + * in single-cycle mode, so we have to do an extra xfer to read the > + * temperature. > + */ > + if (address == AD4695_CMD_TEMP_CHAN) { > + /* We aren't reading, so we can make this a short xfer. */ > + st->cnv_cmd2 = AD4695_CMD_TEMP_CHAN << 3; > + xfer[0].bits_per_word = 8; nit: isn't this the default? > + xfer[0].tx_buf = &st->cnv_cmd2; > + xfer[0].len = 1; > + xfer[0].cs_change = 1; > + xfer[0].cs_change_delay.value = AD4695_T_CONVERT_NS; > + xfer[0].cs_change_delay.unit = SPI_DELAY_UNIT_NSECS; > + > + /* Then read the result and exit conversion mode. */ > + st->cnv_cmd = AD4695_CMD_EXIT_CNV_MODE << 11; > + xfer[1].bits_per_word = 16; > + xfer[1].tx_buf = &st->cnv_cmd; > + xfer[1].rx_buf = &st->raw_data; > + xfer[1].len = 2; > + > + return spi_sync_transfer(st->spi, xfer, 2); > + } > + > + /* > + * The conversion has already been done and we just have to read the > + * result and exit conversion mode. > + */ > + st->cnv_cmd = AD4695_CMD_EXIT_CNV_MODE << 11; > + xfer[0].bits_per_word = 16; > + xfer[0].tx_buf = &st->cnv_cmd; > + xfer[0].rx_buf = &st->raw_data; > + xfer[0].len = 2; > + > + return spi_sync_transfer(st->spi, xfer, 1); > +} > + > +static int ad4695_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int *val, int *val2, long mask) > +{ > + struct ad4695_state *st = iio_priv(indio_dev); > + struct ad4695_channel_config *cfg = &st->channels_cfg[chan- > >scan_index]; > + u8 realbits = chan->scan_type.realbits; > + int ret; > + > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + iio_device_claim_direct_scoped(return -EBUSY, indio_dev) { > + ret = ad4695_read_one_sample(st, chan->address); > + if (ret) > + return ret; > + > + if (chan->scan_type.sign == 's') > + *val = sign_extend32(st->raw_data, realbits - > 1); > + else > + *val = st->raw_data; > + > + return IIO_VAL_INT; > + } > + unreachable(); > + case IIO_CHAN_INFO_SCALE: > + switch (chan->type) { > + case IIO_VOLTAGE: > + *val = st->vref_mv; > + *val2 = chan->scan_type.realbits; > + return IIO_VAL_FRACTIONAL_LOG2; > + case IIO_TEMP: > + /* T_scale (°C) = raw * V_REF (mV) / (-1.8 mV/°C * > 2^16) */ > + *val = st->vref_mv * -556; > + *val2 = 16; > + return IIO_VAL_FRACTIONAL_LOG2; > + default: > + return -EINVAL; > + } > + case IIO_CHAN_INFO_OFFSET: > + switch (chan->type) { > + case IIO_VOLTAGE: > + if (cfg->pin_pairing == AD4695_IN_PAIR_COM) > + *val = st->com_mv * (1 << realbits) / st- > >vref_mv; > + else if (cfg->pin_pairing == AD4695_IN_PAIR_EVEN_ODD) > + *val = cfg->common_mode_mv * (1 << realbits) > / st->vref_mv; > + else > + *val = 0; > + > + return IIO_VAL_INT; > + case IIO_TEMP: > + /* T_offset (°C) = -725 mV / (-1.8 mV/°C) */ > + /* T_offset (raw) = T_offset (°C) * (-1.8 mV/°C) * > 2^16 / V_REF (mV) */ > + *val = -47513600; > + *val2 = st->vref_mv; > + return IIO_VAL_FRACTIONAL; > + default: > + return -EINVAL; > + } > + default: > + return -EINVAL; > + } > +} > + > +static int ad4695_debugfs_reg_access(struct iio_dev *indio_dev, unsigned int > reg, > + unsigned int writeval, unsigned int > *readval) > +{ > + struct ad4695_state *st = iio_priv(indio_dev); > + > + iio_device_claim_direct_scoped(return -EBUSY, indio_dev) { > + if (readval) > + return regmap_read(st->regmap, reg, readval); > + > + return regmap_write(st->regmap, reg, writeval); > + } > + > + unreachable(); > +} > + > +static const struct iio_info ad4695_info = { > + .read_raw = &ad4695_read_raw, > + .debugfs_reg_access = &ad4695_debugfs_reg_access, > +}; > + > +static int ad4695_parse_channel_cfg(struct iio_dev *indio_dev) > +{ > + struct device *dev = indio_dev->dev.parent; > + struct ad4695_state *st = iio_priv(indio_dev); Why not passing in struct ad4695_state directly? ... > > + > + /* Needed for debugfs since it only access registers 1 byte at a > time. */ > + ret = regmap_set_bits(st->regmap, AD4695_REG_SPI_CONFIG_C, > + AD4695_REG_SPI_CONFIG_C_MB_STRICT); > + if (ret) > + return ret; > + Question... do we gain something but not doing the above? Because debugfs is optional and always doing it even when it's not present looks unnecessary. - Nuno Sá
