On Tue, 2023-12-12 at 16:42 +0100, David Lechner wrote: > On Tue, Dec 12, 2023 at 4:16 PM Nuno Sá <noname.nuno@xxxxxxxxx> wrote: > > > > Hi David, > > > > minor stuff from me... > > > > On Fri, 2023-12-08 at 09:51 -0600, David Lechner wrote: > > > This adds a new driver for the AD7380 family ADCs. > > > > > > The driver currently implements basic support for the AD7380, AD7381, > > > AD7383, and AD7384 2-channel differential ADCs. Support for additional > > > single-ended and 4-channel chips that use the same register map as well > > > as additional features of the chip will be added in future patches. > > > > > > Co-developed-by: Stefan Popa <stefan.popa@xxxxxxxxxx> > > > Signed-off-by: Stefan Popa <stefan.popa@xxxxxxxxxx> > > > Signed-off-by: David Lechner <dlechner@xxxxxxxxxxxx> > > > --- > > > MAINTAINERS | 1 + > > > drivers/iio/adc/Kconfig | 16 ++ > > > drivers/iio/adc/Makefile | 1 + > > > drivers/iio/adc/ad7380.c | 467 > > > +++++++++++++++++++++++++++++++++++++++++++++++ > > > 4 files changed, 485 insertions(+) > > > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > > index e2a998be5879..5a54620a31b8 100644 > > > --- a/MAINTAINERS > > > +++ b/MAINTAINERS > > > @@ -438,6 +438,7 @@ S: Supported > > > W: > > > https://wiki.analog.com/resources/tools-software/linux-drivers/iio-adc/ad738x > > > W: https://ez.analog.com/linux-software-drivers > > > F: Documentation/devicetree/bindings/iio/adc/adi,ad7380.yaml > > > +F: drivers/iio/adc/ad7380.c > > > > > > AD7877 TOUCHSCREEN DRIVER > > > M: Michael Hennerich <michael.hennerich@xxxxxxxxxx> > > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig > > > index 35f9867da12c..cbfd626712e3 100644 > > > --- a/drivers/iio/adc/Kconfig > > > +++ b/drivers/iio/adc/Kconfig > > > @@ -122,6 +122,22 @@ config AD7298 > > > To compile this driver as a module, choose M here: the > > > module will be called ad7298. > > > > > > +config AD7380 > > > + tristate "Analog Devices AD7380 ADC driver" > > > + depends on SPI_MASTER > > > + select IIO_BUFFER > > > + select IIO_TRIGGER > > > + select IIO_TRIGGERED_BUFFER > > > + help > > > + AD7380 is a family of simultaneous sampling ADCs that share the > > > same > > > + SPI register map and have similar pinouts. > > > + > > > + Say yes here to build support for Analog Devices AD7380 ADC and > > > + similar chips. > > > + > > > + To compile this driver as a module, choose M here: the module will > > > be > > > + called ad7380. > > > + > > > config AD7476 > > > tristate "Analog Devices AD7476 1-channel ADCs driver and other > > > similar devices from AD and TI" > > > depends on SPI > > > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile > > > index bee11d442af4..e046d8004f41 100644 > > > --- a/drivers/iio/adc/Makefile > > > +++ b/drivers/iio/adc/Makefile > > > @@ -16,6 +16,7 @@ obj-$(CONFIG_AD7291) += ad7291.o > > > obj-$(CONFIG_AD7292) += ad7292.o > > > obj-$(CONFIG_AD7298) += ad7298.o > > > obj-$(CONFIG_AD7923) += ad7923.o > > > +obj-$(CONFIG_AD7476) += ad7380.o > > > obj-$(CONFIG_AD7476) += ad7476.o > > > obj-$(CONFIG_AD7606_IFACE_PARALLEL) += ad7606_par.o > > > obj-$(CONFIG_AD7606_IFACE_SPI) += ad7606_spi.o > > > diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c > > > new file mode 100644 > > > index 000000000000..6a5ec59bd1fd > > > --- /dev/null > > > +++ b/drivers/iio/adc/ad7380.c > > > @@ -0,0 +1,467 @@ > > > +// SPDX-License-Identifier: GPL-2.0-only > > > +/* > > > + * Analog Devices AD738x Simultaneous Sampling SAR ADCs > > > + * > > > + * Copyright 2017 Analog Devices Inc. > > > + * Copyright 2023 BayLibre, SAS > > > + */ > > > + > > > +#include <linux/bitfield.h> > > > +#include <linux/bitops.h> > > > +#include <linux/cleanup.h> > > > +#include <linux/device.h> > > > +#include <linux/err.h> > > > +#include <linux/kernel.h> > > > +#include <linux/module.h> > > > +#include <linux/regmap.h> > > > +#include <linux/regulator/consumer.h> > > > +#include <linux/slab.h> > > > +#include <linux/spi/spi.h> > > > +#include <linux/sysfs.h> > > > + > > > +#include <linux/iio/buffer.h> > > > +#include <linux/iio/iio.h> > > > +#include <linux/iio/sysfs.h> > > > +#include <linux/iio/trigger_consumer.h> > > > +#include <linux/iio/triggered_buffer.h> > > > + > > > > > > > ... > > > > > + > > > +static int ad7380_debugfs_reg_access(struct iio_dev *indio_dev, u32 reg, > > > + u32 writeval, u32 *readval) > > > +{ > > > + struct ad7380_state *st = iio_priv(indio_dev); > > > + int ret; > > > + > > > + ret = iio_device_claim_direct_mode(indio_dev); > > > + if (ret) > > > + return ret; > > > + > > > > potential controversial take: do we really need locking in here? regmap already > > has it's own lock (I think you're not disabling it) and if someone plays with > > registers it shouldn't while buffering, well... This is a debug interface so I > > would probably not worry much. One could anyways for write stuff by going > > directly to regmap :) > > > > That said, fine to be as-is from my side (just wanted to take it out of my > > system :))... > > I put it in because we often run `iio_info` which reads register 0 via > debugfs. So even though we "shouldn't" be using debug while buffer is > enabled, it is easy to do unintentionally. (This was particularly > problem while working on offload support.) > fair enough... > > > > > + if (readval) > > > + ret = regmap_read(st->regmap, reg, readval); > > > + else > > > + ret = regmap_write(st->regmap, reg, writeval); > > > + > > > + iio_device_release_direct_mode(indio_dev); > > > + > > > + return ret; > > > +} > > > + > > > +static irqreturn_t ad7380_trigger_handler(int irq, void *p) > > > +{ > > > + struct iio_poll_func *pf = p; > > > + struct iio_dev *indio_dev = pf->indio_dev; > > > + struct ad7380_state *st = iio_priv(indio_dev); > > > + struct spi_transfer xfer = { > > > + .bits_per_word = st->chip_info- > > > > channels[0].scan_type.realbits, > > > + .len = 4, > > > + .rx_buf = &st->scan_data, > > > + }; > > > + int ret; > > > + > > > + ret = spi_sync_transfer(st->spi, &xfer, 1); > > > + > > > + if (ret == 0) > > > + iio_push_to_buffers_with_timestamp(indio_dev, &st->scan_data, > > > + pf->timestamp); > > > + > > > + iio_trigger_notify_done(indio_dev->trig); > > > + > > > + return IRQ_HANDLED; > > > +} > > > + > > > +static int ad7380_read_direct(struct ad7380_state *st, > > > + struct iio_chan_spec const *chan, int *val) > > > +{ > > > + struct spi_transfer xfers[] = { > > > + /* toggle CS (no data xfer) to trigger a conversion */ > > > + { > > > + .speed_hz = AD7380_REG_WR_SPEED_HZ, > > > + .bits_per_word = chan->scan_type.realbits, > > > + .delay = { > > > + .value = 190, /* t[CONVERT] */ > > > + .unit = SPI_DELAY_UNIT_NSECS, > > > + }, > > > + .cs_change = 1, > > > + .cs_change_delay = { > > > + .value = 10, /* t[CSH] */ > > > + .unit = SPI_DELAY_UNIT_NSECS, > > > + }, > > > + }, > > > + /* then read both channels */ > > > + { > > > + .speed_hz = AD7380_REG_WR_SPEED_HZ, > > > + .bits_per_word = chan->scan_type.realbits, > > > + .rx_buf = &st->rx[0], > > > + .len = 4, > > > + }, > > > + }; > > > + int ret; > > > + > > > + ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers)); > > > + > > > + if (ret < 0) > > > + return ret; > > > + > > > + *val = sign_extend32(st->rx[chan->scan_index], > > > + chan->scan_type.realbits - 1); > > > + > > > + return IIO_VAL_INT; > > > +} > > > + > > > +static int ad7380_read_raw(struct iio_dev *indio_dev, > > > + struct iio_chan_spec const *chan, > > > + int *val, int *val2, long info) > > > +{ > > > + struct ad7380_state *st = 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; > > > + > > > + ret = ad7380_read_direct(st, chan, val); > > > + iio_device_release_direct_mode(indio_dev); > > > + > > > + return ret; > > > + case IIO_CHAN_INFO_SCALE: > > > + if (st->vref) { > > > + ret = regulator_get_voltage(st->vref); > > > > nit: I wonder how likely it is for vref to change at runtime... > > > > > + if (ret < 0) > > > + return ret; > > > + > > > + *val = ret / 1000; > > > + } else { > > > + *val = AD7380_INTERNAL_REF_MV; > > > + } > > > + > > > + *val2 = chan->scan_type.realbits; > > > + > > > + return IIO_VAL_FRACTIONAL_LOG2; > > > + } > > > + > > > + return -EINVAL; > > > +} > > > + > > > +static const struct iio_info ad7380_info = { > > > + .read_raw = &ad7380_read_raw, > > > + .debugfs_reg_access = &ad7380_debugfs_reg_access, > > > +}; > > > + > > > +static int ad7380_init(struct ad7380_state *st) > > > +{ > > > + int ret; > > > + > > > + /* perform hard reset */ > > > + ret = regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG2, > > > + AD7380_CONFIG2_RESET, > > > + FIELD_PREP(AD7380_CONFIG2_RESET, > > > + AD7380_CONFIG2_RESET_HARD)); > > > + if (ret < 0) > > > + return ret; > > > + > > > + > > > + /* select internal or external reference voltage */ > > > + ret = regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG1, > > > + AD7380_CONFIG1_REFSEL, > > > + FIELD_PREP(AD7380_CONFIG1_REFSEL, !!st- > > > > vref)); > > > + if (ret < 0) > > > + return ret; > > > + > > > + /* SPI 1-wire mode */ > > > + return regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG2, > > > + AD7380_CONFIG2_SDO, > > > + FIELD_PREP(AD7380_CONFIG2_SDO, 1)); > > > +} > > > + > > > +static void ad7380_release_regulator(void *p) > > > +{ > > > + struct regulator *reg = p; > > > + > > > + regulator_disable(reg); > > > +} > > > + > > > +static int ad7380_probe(struct spi_device *spi) > > > +{ > > > + struct iio_dev *indio_dev; > > > + struct ad7380_state *st; > > > + const char *str_val; > > > + int ret; > > > + > > > + ret = device_property_read_string(&spi->dev, "adi,sdo-mode", > > > &str_val); > > > + if (ret < 0) > > > + return dev_err_probe(&spi->dev, ret, > > > + "Failed to read adi,sdo-mode > > > property\n"); > > > + > > > + if (strcmp(str_val, "1-wire")) > > > + return dev_err_probe(&spi->dev, -EINVAL, > > > + "Only 1-wire SDO is supported\n"); > > > + > > > + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st)); > > > + if (!indio_dev) > > > + return -ENOMEM; > > > + > > > + st = iio_priv(indio_dev); > > > + st->spi = spi; > > > + st->chip_info = spi_get_device_match_data(spi); > > > + > > > > if (!st->chip_info) ? > > It looks like quite a few drivers don't check this, but I guess safer > to check anyway. > Don't make it right :). I mean, FWIW, I'm not quite sure if you can really get NULL in here but since it's a possibility (from the API point of view), better to do it properly. - Nuno Sá
