From: David Lechner <dlechner@baylibre.com>
To: "Nuno Sá" <noname.nuno@gmail.com>
Cc: linux-iio@vger.kernel.org, devicetree@vger.kernel.org,
"Rob Herring" <robh+dt@kernel.org>,
"Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>,
"Conor Dooley" <conor+dt@kernel.org>,
"Jonathan Cameron" <jic23@kernel.org>,
"Michael Hennerich" <michael.hennerich@analog.com>,
"Nuno Sá" <nuno.sa@analog.com>,
"Alexandru Ardelean" <alexandru.ardelean@analog.com>,
"Liam Girdwood" <lgirdwood@gmail.com>,
"Mark Brown" <broonie@kernel.org>,
linux-kernel@vger.kernel.org,
"Stefan Popa" <stefan.popa@analog.com>
Subject: Re: [PATCH 2/2] iio: adc: ad7380: new driver for AD7380 ADCs
Date: Tue, 12 Dec 2023 16:42:33 +0100 [thread overview]
Message-ID: <CAMknhBHLAhTtvyPN=J5oYRdsPgX_2a7g9-Q+UB_mAs9=gV=sTg@mail.gmail.com> (raw)
In-Reply-To: <c21634252fec82dadd27b1bff69b24d3384acf00.camel@gmail.com>
On Tue, Dec 12, 2023 at 4:16 PM Nuno Sá <noname.nuno@gmail.com> 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@analog.com>
> > Signed-off-by: Stefan Popa <stefan.popa@analog.com>
> > Signed-off-by: David Lechner <dlechner@baylibre.com>
> > ---
> > 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@analog.com>
> > 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.)
>
> > + 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.
>
> > + 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_release_regulator,
> > + 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");
> > +
>
> Could we instead have a custom regmap_bus instead of bypass regmap with
> reg_read() and reg_write()?
> > +
> > + indio_dev->channels = st->chip_info->channels;
> > + indio_dev->num_channels = st->chip_info->num_channels;
> > + indio_dev->dev.parent = &spi->dev;
>
> no need to assign parent (the core does it for you)...
>
> - Nuno Sá
>
next prev parent reply other threads:[~2023-12-12 15:42 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-12-08 15:51 [PATCH 0/2] iio: adc: add new ad7380 driver David Lechner
2023-12-08 15:51 ` [PATCH 1/2] dt-bindings: iio: adc: Add binding for AD7380 ADCs David Lechner
2023-12-09 7:57 ` Krzysztof Kozlowski
2023-12-10 13:49 ` Jonathan Cameron
2023-12-11 9:13 ` David Lechner
2023-12-08 15:51 ` [PATCH 2/2] iio: adc: ad7380: new driver " David Lechner
2023-12-08 21:03 ` David Lechner
2023-12-10 14:02 ` Jonathan Cameron
2023-12-12 15:19 ` Nuno Sá
2023-12-12 15:42 ` David Lechner [this message]
2023-12-13 7:13 ` Nuno Sá
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAMknhBHLAhTtvyPN=J5oYRdsPgX_2a7g9-Q+UB_mAs9=gV=sTg@mail.gmail.com' \
--to=dlechner@baylibre.com \
--cc=alexandru.ardelean@analog.com \
--cc=broonie@kernel.org \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=jic23@kernel.org \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=lgirdwood@gmail.com \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=michael.hennerich@analog.com \
--cc=noname.nuno@gmail.com \
--cc=nuno.sa@analog.com \
--cc=robh+dt@kernel.org \
--cc=stefan.popa@analog.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.