| Message ID | 20231213-ad7380-mainline-v2-0-cd32150d84a3@baylibre.com |
|---|---|
| Headers | show |
| Series | iio: adc: add new ad7380 driver | expand |
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@analog.com> > Signed-off-by: Stefan Popa <stefan.popa@analog.com> > Signed-off-by: David Lechner <dlechner@baylibre.com> > --- > > 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@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..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@analog.com> - Nuno Sá
On Wed, Dec 13, 2023 at 1:18 PM Nuno Sá <noname.nuno@gmail.com> 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@analog.com> > > Signed-off-by: Stefan Popa <stefan.popa@analog.com> > > Signed-off-by: David Lechner <dlechner@baylibre.com> > > --- > > > > 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@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..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@analog.com> > > > - 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. :-( If we need a v3, I will look into regmap_bus but at leas superficially, I don't see much difference for this part, i.e not really any case where bulk ops make sense and since it uses SPI bus underneath, endianness isn't an issue.
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@gmail.com> 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@analog.com> > > > Signed-off-by: Stefan Popa <stefan.popa@analog.com> > > > Signed-off-by: David Lechner <dlechner@baylibre.com> > > > --- > > > > > > 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@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..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@analog.com> > > > > > > - 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á
On Wed, 13 Dec 2023 05:21:20 -0600 David Lechner <dlechner@baylibre.com> 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> Just one additional comment. I 'might' sort both this an Nuno's comment if Mark is fine with the SPI and no on else has review comments. Feel free to send a v3 though if you like ;) > +/* fully differential */ > +DEFINE_AD7380_DIFFERENTIAL_2_CHANNEL(ad7380_channels, 16); > +DEFINE_AD7380_DIFFERENTIAL_2_CHANNEL(ad7381_channels, 14); > +/* pseudo differential */ > +DEFINE_AD7380_DIFFERENTIAL_2_CHANNEL(ad7383_channels, 16); > +DEFINE_AD7380_DIFFERENTIAL_2_CHANNEL(ad7384_channels, 14); > + > +/* Since this is simultaneous sampling, we don't allow individual channels. */ > +static const unsigned long ad7380_2_channel_scan_masks[] = { > + GENMASK(2, 0), /* both ADC channels and soft timestamp */ > + GENMASK(1, 0), /* both ADC channels, no timestamp */ https://elixir.bootlin.com/linux/v6.7-rc5/source/include/linux/iio/iio.h#L567 See the comment (added recently!) Also, if I remember how this works correctly there is no need to include the timestamp in the mask. We do special handling for it to avoid having to double the number of provided masks. The details being that it uses iio_scan_el_ts_store rather than iio_scan_el_Store. So as you have it I think you'll always end up with the first entry and that will include a bonus bit that isn't a problem as it will match anyway. So just have the second entry and 0. Jonathan > + 0 > +};
On Thu, Dec 14, 2023 at 11:14 AM Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote: > > On Wed, 13 Dec 2023 05:21:20 -0600 > David Lechner <dlechner@baylibre.com> 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> > > Just one additional comment. I 'might' sort both this an Nuno's comment > if Mark is fine with the SPI and no on else has review comments. > Feel free to send a v3 though if you like ;) > > > > +/* fully differential */ > > +DEFINE_AD7380_DIFFERENTIAL_2_CHANNEL(ad7380_channels, 16); > > +DEFINE_AD7380_DIFFERENTIAL_2_CHANNEL(ad7381_channels, 14); > > +/* pseudo differential */ > > +DEFINE_AD7380_DIFFERENTIAL_2_CHANNEL(ad7383_channels, 16); > > +DEFINE_AD7380_DIFFERENTIAL_2_CHANNEL(ad7384_channels, 14); > > + > > +/* Since this is simultaneous sampling, we don't allow individual channels. */ > > +static const unsigned long ad7380_2_channel_scan_masks[] = { > > + GENMASK(2, 0), /* both ADC channels and soft timestamp */ > > + GENMASK(1, 0), /* both ADC channels, no timestamp */ > > https://elixir.bootlin.com/linux/v6.7-rc5/source/include/linux/iio/iio.h#L567 > See the comment (added recently!) I did see this comment but this is already sorted in order of preference, so I'm not sure why you are calling it out. Just FYI, I guess? > > Also, if I remember how this works correctly there is no need to include > the timestamp in the mask. We do special handling for it to avoid having to double > the number of provided masks. The details being that it uses > iio_scan_el_ts_store rather than iio_scan_el_Store. Indeed. I've been working ahead on adding more features and noticed this. So we will need to find a way to say that we the timestamp should not be allowed under certain conditions. But that will be a discussion for a later series. > > So as you have it I think you'll always end up with the first entry > and that will include a bonus bit that isn't a problem as it will match > anyway. > > So just have the second entry and 0. > > Jonathan > > > + 0 > > +};
On Thu, 14 Dec 2023 11:33:51 +0100 David Lechner <dlechner@baylibre.com> wrote: > On Thu, Dec 14, 2023 at 11:14 AM Jonathan Cameron > <Jonathan.Cameron@huawei.com> wrote: > > > > On Wed, 13 Dec 2023 05:21:20 -0600 > > David Lechner <dlechner@baylibre.com> 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> > > > > Just one additional comment. I 'might' sort both this an Nuno's comment > > if Mark is fine with the SPI and no on else has review comments. > > Feel free to send a v3 though if you like ;) > > > > > > > +/* fully differential */ > > > +DEFINE_AD7380_DIFFERENTIAL_2_CHANNEL(ad7380_channels, 16); > > > +DEFINE_AD7380_DIFFERENTIAL_2_CHANNEL(ad7381_channels, 14); > > > +/* pseudo differential */ > > > +DEFINE_AD7380_DIFFERENTIAL_2_CHANNEL(ad7383_channels, 16); > > > +DEFINE_AD7380_DIFFERENTIAL_2_CHANNEL(ad7384_channels, 14); > > > + > > > +/* Since this is simultaneous sampling, we don't allow individual channels. */ > > > +static const unsigned long ad7380_2_channel_scan_masks[] = { > > > + GENMASK(2, 0), /* both ADC channels and soft timestamp */ > > > + GENMASK(1, 0), /* both ADC channels, no timestamp */ > > > > https://elixir.bootlin.com/linux/v6.7-rc5/source/include/linux/iio/iio.h#L567 > > See the comment (added recently!) > > I did see this comment but this is already sorted in order of > preference, so I'm not sure why you are calling it out. Just FYI, I > guess? No. Order of preference would be turn on the minimal if that is enough. First item is the highest preference (if the requested channels are a subset of that we don't look any further). Here that means we always stop on the first entry and never look at the second. > > > > > Also, if I remember how this works correctly there is no need to include > > the timestamp in the mask. We do special handling for it to avoid having to double > > the number of provided masks. The details being that it uses > > iio_scan_el_ts_store rather than iio_scan_el_Store. > > Indeed. I've been working ahead on adding more features and noticed > this. So we will need to find a way to say that we the timestamp > should not be allowed under certain conditions. But that will be a > discussion for a later series. Interesting - you have cases where it's not valid at all? It sometimes becomes inaccurate because we are interpolating across data from a fifo, but I've not seen a case where we can't provide anything useful. Ah well - as you say I'll wait for that later series! Jonathan > > > > > So as you have it I think you'll always end up with the first entry > > and that will include a bonus bit that isn't a problem as it will match > > anyway. > > > > So just have the second entry and 0. > > > > Jonathan > > > > > + 0 > > > +};
On Thu, Dec 14, 2023 at 1:36 PM Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote: > > On Thu, 14 Dec 2023 11:33:51 +0100 > David Lechner <dlechner@baylibre.com> wrote: > > > On Thu, Dec 14, 2023 at 11:14 AM Jonathan Cameron > > <Jonathan.Cameron@huawei.com> wrote: > > > > > > On Wed, 13 Dec 2023 05:21:20 -0600 > > > David Lechner <dlechner@baylibre.com> 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> > > > > > > Just one additional comment. I 'might' sort both this an Nuno's comment > > > if Mark is fine with the SPI and no on else has review comments. > > > Feel free to send a v3 though if you like ;) > > > > > > > > > > +/* fully differential */ > > > > +DEFINE_AD7380_DIFFERENTIAL_2_CHANNEL(ad7380_channels, 16); > > > > +DEFINE_AD7380_DIFFERENTIAL_2_CHANNEL(ad7381_channels, 14); > > > > +/* pseudo differential */ > > > > +DEFINE_AD7380_DIFFERENTIAL_2_CHANNEL(ad7383_channels, 16); > > > > +DEFINE_AD7380_DIFFERENTIAL_2_CHANNEL(ad7384_channels, 14); > > > > + > > > > +/* Since this is simultaneous sampling, we don't allow individual channels. */ > > > > +static const unsigned long ad7380_2_channel_scan_masks[] = { > > > > + GENMASK(2, 0), /* both ADC channels and soft timestamp */ > > > > + GENMASK(1, 0), /* both ADC channels, no timestamp */ > > > > > > https://elixir.bootlin.com/linux/v6.7-rc5/source/include/linux/iio/iio.h#L567 > > > See the comment (added recently!) > > > > I did see this comment but this is already sorted in order of > > preference, so I'm not sure why you are calling it out. Just FYI, I > > guess? > > No. Order of preference would be turn on the minimal if that is enough. > First item is the highest preference (if the requested channels are a subset of > that we don't look any further). Here that means we always stop on the first > entry and never look at the second. OK, I understand what you are getting at now. I thought the preference could be my personal preference rather than the minimal case. :-) But as you pointed out, the timestamp is handled separately, so it doesn't make a difference here. The main point was to ensure that both channels are always enabled since the ADC is doing simultaneous sampling and always reading two channels at the same time. > > > > > > > > > Also, if I remember how this works correctly there is no need to include > > > the timestamp in the mask. We do special handling for it to avoid having to double > > > the number of provided masks. The details being that it uses > > > iio_scan_el_ts_store rather than iio_scan_el_Store. > > > > Indeed. I've been working ahead on adding more features and noticed > > this. So we will need to find a way to say that we the timestamp > > should not be allowed under certain conditions. But that will be a > > discussion for a later series. > > Interesting - you have cases where it's not valid at all? > It sometimes becomes inaccurate because we are interpolating across > data from a fifo, but I've not seen a case where we can't provide anything > useful. Ah well - as you say I'll wait for that later series! > > Jonathan > > > > > > > > > So as you have it I think you'll always end up with the first entry > > > and that will include a bonus bit that isn't a problem as it will match > > > anyway. > > > > > > So just have the second entry and 0. > > > > > > Jonathan > > > > > > > + 0 > > > > +}; >
This series is adding a new driver for the Analog Devices Inc. AD7380, AD7381, AD7383, and AD7384 ADCs. These chips are part of a family of simultaneous sampling SAR ADCs. One quirk of these chips is that since they are simultaneous sampling, they have multiple SPI data output lines that allow transferring two data words (one for each input channel) at the same time. So a new generic devicetree binding is added to describe this sort of SPI bus configuration. To keep things simple, the initial driver implementation only supports the 2-channel differential chips listed above. There are also 4-channel differential chips and 4-channel single-ended chips in the family that can be added later. Furthermore, the driver is just implementing basic support for capturing data. Additional features like interrupts, CRC, etc. can be added later. Also, FYI, this driver will also be used as the base for an upcoming series adding AXI SPI Engine offload support for these chips along with [1]. This work is being done by BayLibre and on behalf of Analog Devices Inc. hence the maintainers are @analog.com. [1]: https://lore.kernel.org/linux-spi/20231204-axi-spi-engine-series-2-v1-0-063672323fce@baylibre.com/ --- Changes in v2: - dt-bindings: - Added new patch with generic spi-rx-bus-channels property - Added maxItems to reg property - Replaced adi,sdo-mode property with spi-rx-bus-channels - Made spi-rx-bus-channels property optional with default value of 1 (this made the if: check more complex) - Changed example to use gpio for interrupt - driver: - 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 - Added available_scan_masks - Added check for missing driver match data - Link to v1: https://lore.kernel.org/r/20231208-ad7380-mainline-v1-0-2b33fe2f44ae@baylibre.com --- David Lechner (3): dt-bindings: spi: add spi-rx-bus-channels peripheral property dt-bindings: iio: adc: Add binding for AD7380 ADCs iio: adc: ad7380: new driver for AD7380 ADCs .../devicetree/bindings/iio/adc/adi,ad7380.yaml | 107 +++++ .../bindings/spi/spi-peripheral-props.yaml | 12 + MAINTAINERS | 10 + drivers/iio/adc/Kconfig | 16 + drivers/iio/adc/Makefile | 1 + drivers/iio/adc/ad7380.c | 464 +++++++++++++++++++++ 6 files changed, 610 insertions(+) --- base-commit: 18f78b5e609b19b56237f0dae47068d44b8b0ecd change-id: 20231208-ad7380-mainline-e6c4fa7dbedd