On Wed, 2023-12-13 at 14:47 +0100, David Lechner wrote: > On Wed, Dec 13, 2023 at 1:18 PM Nuno Sá <noname.nuno@xxxxxxxxx> wrote: > > > > On Wed, 2023-12-13 at 05:21 -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> > > > --- > > > > > > v2 changes: > > > - Fixed CONFIG_AD7380 in Makefile > > > - rx_buf = st->scan_data.raw instead of rx_buf = &st->scan_data > > > - Moved iio_push_to_buffers_with_timestamp() outside of if statement > > > - Removed extra blank lines > > > - Renamed regulator disable function > > > - Dropped checking of adi,sdo-mode property (regardless of the actual > > > wiring, we can always use 1-wire mode) > > > - Added available_scan_masks (we always sample two channels at the same time > > > so we need to let userspace know this) > > > - Added check for missing driver match data > > > > > > MAINTAINERS | 1 + > > > drivers/iio/adc/Kconfig | 16 ++ > > > drivers/iio/adc/Makefile | 1 + > > > drivers/iio/adc/ad7380.c | 464 +++++++++++++++++++++++++++++++++++++++++++++++ > > > 4 files changed, 482 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..9c921c497655 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_AD7380) += 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..b8025b636b67 > > > --- /dev/null > > > +++ b/drivers/iio/adc/ad7380.c > > > @@ -0,0 +1,464 @@ > > > +// 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_probe(struct spi_device *spi) > > > +{ > > > + struct iio_dev *indio_dev; > > > + struct ad7380_state *st; > > > + int ret; > > > + > > > + 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) > > > + return dev_err_probe(&spi->dev, -EINVAL, "missing match > > > data\n"); > > > + > > > + st->vref = devm_regulator_get_optional(&spi->dev, "refio"); > > > + if (IS_ERR(st->vref)) { > > > + /* > > > + * If there is no REFIO supply, then it means that we are using > > > + * the internal 2.5V reference. > > > + */ > > > + if (PTR_ERR(st->vref) == -ENODEV) > > > + st->vref = NULL; > > > + else > > > + return dev_err_probe(&spi->dev, PTR_ERR(st->vref), > > > + "Failed to get refio > > > regulator\n"); > > > + } > > > + > > > + if (st->vref) { > > > + ret = regulator_enable(st->vref); > > > + if (ret) > > > + return ret; > > > + > > > + ret = devm_add_action_or_reset(&spi->dev, > > > ad7380_regulator_disable, > > > + st->vref); > > > + if (ret) > > > + return ret; > > > + } > > > + > > > + st->regmap = devm_regmap_init(&spi->dev, NULL, st, > > > &ad7380_regmap_config); > > > + if (IS_ERR(st->regmap)) > > > + return dev_err_probe(&spi->dev, PTR_ERR(st->regmap), > > > + "failed to allocate register map\n"); > > > > Still not using a regmap_bus... You could at least argue in the last version why > > you're not doing it rather than ignoring the comment :). > > > > I'm asking for it because it already happened (in IIO) to me and I was asked for > > implementing the bus. You also gain things like regmap core handling endianism > > and > > formatting the work buffer for you (eg: regmap_bulk_read() could be more > > efficient), > > > > > + indio_dev->channels = st->chip_info->channels; > > > + indio_dev->num_channels = st->chip_info->num_channels; > > > + indio_dev->dev.parent = &spi->dev; > > > > still not addressed... > > > > With at least the above (for the regmap_bus I'll leave the ultimate decision to > > Jonathan - not a deal breaker for me): > > > > Reviewed-by: Nuno Sa <nuno.sa@xxxxxxxxxx> > > > > > > - Nuno Sá > > > > Sorry, I did not mean to ignore these. I just did a bad job of > double-checking that I addressed all comments before sending v2. :-( > no worries (done that and will do it again) :) > If we need a v3, I will look into regmap_bus but at leas Well, at least 'indio_dev->dev.parent = &spi->dev;' should be removed. But yeah, if the regmap_bus is not to be required then Jonathan might just do it while applying. - Nuno Sá
