Re: [PATCH 2/2] iio: adc: ad7944: add driver for AD7944/AD7985/AD7986

["Nuno Sá" <noname.nuno@gmail.com>]

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@gmail.com> 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@baylibre.com>
> > > ---
> > >  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@baylibre.com>
> > >  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@gmail.com>
> > > 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á
>