On Wed, 2024-02-07 at 08:19 -0600, David Lechner wrote: > On Wed, Feb 7, 2024 at 4:07 AM Nuno Sá <noname.nuno@xxxxxxxxx> wrote: > > > > 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? > > Yes, in Linux SPI words are always CPU-endian. It is the drivers that > use 8-bit transfers to read 16 bits that need to handle big-endian > swapping. But here we are using 14/16/18 bit transfers. > Right... > > > > > + 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. > > Yes, if it is merged already, happy to make use of it here. > > > > > > + 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? > > I used the term "3-wire" because that is what the datasheet calls it, > but it is not the same as what the SPI core calls SPI_3WIRE. The > former is described in the DT bindings patch in this series and the > latter means that SDI and SDO are on the same pin, which is not the > case here. > Oh, I see... I could have looked at the bindings. - Nuno Sá >
