Hi David, The driver It's in pretty good shape... Just some comments from me On Tue, 2024-02-06 at 11:26 -0600, David Lechner wrote: > This adds a driver for the Analog Devices Inc. AD7944, AD7985, and > AD7986 ADCs. These are a family of pin-compatible ADCs that can sample > at rates up to 2.5 MSPS. > > The initial driver adds support for sampling at lower rates using the > usual IIO triggered buffer and can handle all 3 possible reference > voltage configurations. > > Signed-off-by: David Lechner <dlechner@xxxxxxxxxxxx> > --- > MAINTAINERS | 1 + > drivers/iio/adc/Kconfig | 10 ++ > drivers/iio/adc/Makefile | 1 + > drivers/iio/adc/ad7944.c | 397 > +++++++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 409 insertions(+) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 4f1e658e1e0d..83d8367595f1 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -458,6 +458,7 @@ R: David Lechner <dlechner@xxxxxxxxxxxx> > S: Supported > W: https://ez.analog.com/linux-software-drivers > F: Documentation/devicetree/bindings/iio/adc/adi,ad7944.yaml > +F: drivers/iio/adc/ad7944.c > > ADAFRUIT MINI I2C GAMEPAD > M: Anshul Dalal <anshulusr@xxxxxxxxx> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig > index 59ae1d17b50d..93fbe6f8e306 100644 > --- a/drivers/iio/adc/Kconfig > +++ b/drivers/iio/adc/Kconfig > @@ -280,6 +280,16 @@ config AD7923 > To compile this driver as a module, choose M here: the > module will be called ad7923. > > +config AD7944 > + tristate "Analog Devices AD7944 and similar ADCs driver" > + depends on SPI > + help > + Say yes here to build support for Analog Devices > + AD7944, AD7985, AD7986 ADCs. > + > + To compile this driver as a module, choose M here: the > + module will be called ad7944 > + > config AD7949 > tristate "Analog Devices AD7949 and similar ADCs driver" > depends on SPI > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile > index 5a26ab6f1109..52d803b92cd7 100644 > --- a/drivers/iio/adc/Makefile > +++ b/drivers/iio/adc/Makefile > @@ -29,6 +29,7 @@ obj-$(CONFIG_AD7780) += ad7780.o > obj-$(CONFIG_AD7791) += ad7791.o > obj-$(CONFIG_AD7793) += ad7793.o > obj-$(CONFIG_AD7887) += ad7887.o > +obj-$(CONFIG_AD7944) += ad7944.o > obj-$(CONFIG_AD7949) += ad7949.o > obj-$(CONFIG_AD799X) += ad799x.o > obj-$(CONFIG_AD9467) += ad9467.o > diff --git a/drivers/iio/adc/ad7944.c b/drivers/iio/adc/ad7944.c > new file mode 100644 > index 000000000000..67b525fb8e59 > --- /dev/null > +++ b/drivers/iio/adc/ad7944.c > @@ -0,0 +1,397 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Analog Devices AD7944/85/86 PulSAR ADC family driver. > + * > + * Copyright 2024 Analog Devices, Inc. > + * Copyright 2024 Baylibre, SAS > + */ > + > +#include <linux/bitfield.h> > +#include <linux/bitops.h> > +#include <linux/delay.h> > +#include <linux/device.h> > +#include <linux/err.h> > +#include <linux/gpio/consumer.h> > +#include <linux/module.h> > +#include <linux/property.h> > +#include <linux/regulator/consumer.h> > +#include <linux/spi/spi.h> > + > +#include <linux/iio/iio.h> > +#include <linux/iio/sysfs.h> > +#include <linux/iio/trigger_consumer.h> > +#include <linux/iio/triggered_buffer.h> > + > +#define AD7944_INTERNAL_REF_MV 4096 > + > +struct ad7944_timing_spec { > + /* Normal mode minimum CNV pulse width in nanoseconds. */ > + unsigned int cnv_ns; > + /* TURBO mode minimum CNV pulse width in nanoseconds. */ > + unsigned int turbo_cnv_ns; > +}; > + > ... > +} > + > +static int ad7944_single_conversion(struct ad7944_adc *adc, > + const struct iio_chan_spec *chan, > + int *val) > +{ > + int ret; > + > + ret = ad7944_4_wire_mode_conversion(adc, chan); > + if (ret) > + return ret; > + > + if (chan->scan_type.storagebits > 16) > + *val = adc->sample.raw.u32; > + else > + *val = adc->sample.raw.u16; > + Will this work both in big vs little endian archs? I don't think so but maybe I'm missing something. At a first glance, it seems we get big endian from spi so shouldn't we have __be16 and __be32? > + if (chan->scan_type.sign == 's') > + *val = sign_extend32(*val, chan->scan_type.realbits - 1); > + > + return IIO_VAL_INT; > +} > + > +static int ad7944_read_raw(struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan, > + int *val, int *val2, long info) > +{ > + struct ad7944_adc *adc = iio_priv(indio_dev); > + int ret; > + > + switch (info) { > + case IIO_CHAN_INFO_RAW: > + ret = iio_device_claim_direct_mode(indio_dev); > + if (ret) > + return ret; > + I'm not totally sure but I think Jonathan already merged his series for the cleanup stuff for the claim direct mode. Maybe take a look and use it? Not a big win in here but I guess we could still reduce some LOC. > + ret = ad7944_single_conversion(adc, chan, val); > + iio_device_release_direct_mode(indio_dev); > + return ret; > + > + case IIO_CHAN_INFO_SCALE: > + switch (chan->type) { > + case IIO_VOLTAGE: > + *val = adc->ref_mv; > + *val2 = chan->scan_type.realbits; > + > + return IIO_VAL_FRACTIONAL_LOG2; > + default: > + return -EINVAL; > + } > + > + default: > + return -EINVAL; > + } > +} > + > +static const struct iio_info ad7944_iio_info = { > + .read_raw = &ad7944_read_raw, > +}; > + > +static irqreturn_t ad7944_trigger_handler(int irq, void *p) > +{ > + struct iio_poll_func *pf = p; > + struct iio_dev *indio_dev = pf->indio_dev; > + struct ad7944_adc *adc = iio_priv(indio_dev); > + int ret; > + > + ret = ad7944_4_wire_mode_conversion(adc, &indio_dev->channels[0]); > + if (ret) > + goto out; > + > + iio_push_to_buffers_with_timestamp(indio_dev, &adc->sample.raw, > + indio_dev->scan_timestamp); > + > +out: > + iio_trigger_notify_done(indio_dev->trig); > + > + return IRQ_HANDLED; > +} > + > +static const char * const ad7944_power_supplies[] = { > + "avdd", "dvdd", "bvdd", "vio" > +}; > + > +static void ad7944_ref_disable(void *ref) > +{ > + regulator_disable(ref); > +} > + > +static int ad7944_probe(struct spi_device *spi) > +{ > + const struct ad7944_chip_info *chip_info; > + struct iio_dev *indio_dev; > + struct ad7944_adc *adc; > + struct regulator *ref; > + const char *str_val; > + int ret; > + > + /* adi,spi-mode property defaults to "4-wire" if not present */ > + if (device_property_read_string(&spi->dev, "adi,spi-mode", &str_val) > < 0) > + str_val = "4-wire"; > + > + if (strcmp(str_val, "4-wire")) > + return dev_err_probe(&spi->dev, -EINVAL, > + "only \"4-wire\" mode is currently > supported\n"); Did you looked at spi core? I guess the chain mode is not available but IIRC spi already has spi-3wire. So maybe you could just have a boolean property for the chain mode and check both that and 3wire? > + > + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*adc)); > + if (!indio_dev) > + return -ENOMEM; > + > + adc = iio_priv(indio_dev); > + adc->spi = spi; > + > + chip_info = spi_get_device_match_data(spi); > + if (!chip_info) > + return dev_err_probe(&spi->dev, -EINVAL, "no chip info\n"); > + > + adc->t = chip_info->t; > + > + /* > + * Some chips use unusual word sizes, so check now instead of waiting > + * for the first xfer. > + */ > + if (!spi_is_bpw_supported(spi, chip_info- > >channels[0].scan_type.realbits)) > + return dev_err_probe(&spi->dev, -EINVAL, > + "SPI host does not support %d bits per > word\n", > + chip_info->channels[0].scan_type.realbits); > + > + ret = devm_regulator_bulk_get_enable(&spi->dev, > + > ARRAY_SIZE(ad7944_power_supplies), > + ad7944_power_supplies); > + if (ret) > + return dev_err_probe(&spi->dev, ret, > + "failed to get and enable supplies\n"); > + > + /* adi,reference property defaults to "internal" if not present */ > + if (device_property_read_string(&spi->dev, "adi,reference", &str_val) > < 0) > + str_val = "internal"; > + > + /* sort out what is being used for the reference voltage */ > + if (strcmp(str_val, "internal") == 0) { Maybe you can make the code neater with match_string() and some enum... - Nuno Sá
