The MAX14001/MAX14002 is configurable, isolated 10-bit ADCs for multi-range
binary inputs. In addition to ADC readings, the MAX14001/MAX14002 offers
more features, like a binary comparator, a filtered reading that can
provide the average of the last 2, 4, or 8 ADC readings, and an inrush
comparator that triggers the inrush current. There is also a fault feature
that can diagnose seven possible fault conditions. And an option to select
an external or internal ADC voltage reference.
MAX14001/MAX14002 features implemented so far:
- Raw ADC reading.
- Filtered ADC average reading with the default configuration.
- MV fault disable.
- Selection of external or internal ADC voltage reference, depending on
whether it is declared in the device tree.
Co-developed-by: Kim Seer Paller <kimseer.paller@analog.com>
Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
Signed-off-by: Marilene Andrade Garcia <marilene.agarcia@gmail.com>
---
I have addressed almost all of the comments, thank you very much for the
review. I would like to highlight some of them:
Changes since v10:
- Dropped the kernel.h include
- Add the cleanup.h, mutex.h, regmap.h and units.h includes
- Renamed the reg_addr variable name to reg
- Renamed the reg_data variable name to val
- Added the regmap implementation
- Used scoped_guard()
- Refactored the get refin voltage code
- Replace max14001_chip_model with data structures separated
- Added debugfs_reg_access
Change I was not able to do:
- I could not remove bitrev16 because I am using an SPI controller that
does not support SPI_LSB_FIRST. So I suggest keeping bitrev16 and not using
the spi-lsb-first devicetree property for now, since this driver currently
works for both types of controllers: those that support it and those that
do not. I left a TODO comment to address this issue as soon as the SPI
kernel code starts handling the bit-reverse operation for controllers that
do not have this support. Once I finish my work on this driver, if the SPI
code still does not include this handling, I can submit patches to add it.
MAINTAINERS | 1 +
drivers/iio/adc/Kconfig | 10 ++
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/max14001.c | 356 +++++++++++++++++++++++++++++++++++++
4 files changed, 368 insertions(+)
create mode 100644 drivers/iio/adc/max14001.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 0bae420caa63..a9cf93ba8b21 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14998,6 +14998,7 @@ L: linux-iio@vger.kernel.org
S: Maintained
W: https://ez.analog.com/linux-software-drivers
F: Documentation/devicetree/bindings/iio/adc/adi,max14001.yaml
+F: drivers/iio/adc/max14001.c
MAXBOTIX ULTRASONIC RANGER IIO DRIVER
M: Andreas Klinger <ak@it-klinger.de>
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 237fa2061329..a1f2afce60ad 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -991,6 +991,16 @@ config MAX11410
To compile this driver as a module, choose M here: the module will be
called max11410.
+config MAX14001
+ tristate "Analog Devices MAX14001/MAX14002 ADC driver"
+ depends on SPI
+ help
+ Say yes here to build support for Analog Devices MAX14001/MAX14002
+ Configurable, Isolated 10-bit ADCs for Multi-Range Binary Inputs.
+
+ To compile this driver as a module, choose M here: the module will be
+ called max14001.
+
config MAX1241
tristate "Maxim max1241 ADC driver"
depends on SPI_MASTER
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 611c16430621..9c4ceb527db7 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -87,6 +87,7 @@ obj-$(CONFIG_MAX11100) += max11100.o
obj-$(CONFIG_MAX1118) += max1118.o
obj-$(CONFIG_MAX11205) += max11205.o
obj-$(CONFIG_MAX11410) += max11410.o
+obj-$(CONFIG_MAX14001) += max14001.o
obj-$(CONFIG_MAX1241) += max1241.o
obj-$(CONFIG_MAX1363) += max1363.o
obj-$(CONFIG_MAX34408) += max34408.o
diff --git a/drivers/iio/adc/max14001.c b/drivers/iio/adc/max14001.c
new file mode 100644
index 000000000000..2ade57970064
--- /dev/null
+++ b/drivers/iio/adc/max14001.c
@@ -0,0 +1,356 @@
+// SPDX-License-Identifier: (GPL-2.0-only OR BSD-3-Clause)
+/*
+ * Analog Devices MAX14001/MAX14002 ADC driver
+ *
+ * Copyright (C) 2023-2025 Analog Devices Inc.
+ * Copyright (C) 2023 Kim Seer Paller <kimseer.paller@analog.com>
+ * Copyright (c) 2025 Marilene Andrade Garcia <marilene.agarcia@gmail.com>
+ *
+ * Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/MAX14001-MAX14002.pdf
+ */
+
+/*
+ * TODO:
+ * Replace bitrev16 with SPI_LSB_FIRST once the SPI kernel code supports handling
+ * SPI controllers that lack LSB-first support.
+ */
+
+#include <linux/array_size.h>
+#include <linux/bitfield.h>
+#include <linux/bitrev.h>
+#include <linux/bits.h>
+#include <linux/byteorder/generic.h>
+#include <linux/cleanup.h>
+#include <linux/device.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
+#include <linux/types.h>
+#include <linux/units.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/types.h>
+
+/* MAX14001 Registers Address */
+#define MAX14001_REG_ADC 0x00
+#define MAX14001_REG_FADC 0x01
+#define MAX14001_REG_FLAGS 0x02
+#define MAX14001_REG_FLTEN 0x03
+#define MAX14001_REG_THL 0x04
+#define MAX14001_REG_THU 0x05
+#define MAX14001_REG_INRR 0x06
+#define MAX14001_REG_INRT 0x07
+#define MAX14001_REG_INRP 0x08
+#define MAX14001_REG_CFG 0x09
+#define MAX14001_REG_ENBL 0x0A
+#define MAX14001_REG_ACT 0x0B
+#define MAX14001_REG_WEN 0x0C
+
+#define MAX14001_REG_VERIFICATION(x) ((x) + 0x10)
+
+#define MAX14001_REG_CFG_BIT_EXRF BIT(5)
+
+#define MAX14001_REG_WEN_VALUE_WRITE 0x294
+
+#define MAX14001_MASK_ADDR GENMASK(15, 11)
+#define MAX14001_MASK_WR BIT(10)
+#define MAX14001_MASK_DATA GENMASK(9, 0)
+
+struct max14001_state {
+ const struct max14001_chip_info *chip_info;
+ struct spi_device *spi;
+ struct regmap *regmap;
+ int vref_mV;
+ /*
+ * lock protect against multiple concurrent accesses, RMW sequence,
+ * and SPI transfer.
+ */
+ struct mutex lock;
+ /*
+ * The following buffers will be bit-reversed during device
+ * communication, because the device transmits and receives data
+ * LSB-first.
+ * DMA (thus cache coherency maintenance) requires the transfer
+ * buffers to live in their own cache lines.
+ */
+ __be16 spi_tx_buffer __aligned(IIO_DMA_MINALIGN);
+ __be16 spi_rx_buffer;
+};
+
+struct max14001_chip_info {
+ const char *name;
+};
+
+static struct max14001_chip_info max14001_chip_info = {
+ .name = "max14001",
+};
+
+static struct max14001_chip_info max14002_chip_info = {
+ .name = "max14002",
+};
+
+static int max14001_read(void *context, unsigned int reg, unsigned int *val)
+{
+ struct max14001_state *st = context;
+ struct spi_transfer xfers[] = {
+ {
+ .tx_buf = &st->spi_tx_buffer,
+ .len = sizeof(st->spi_tx_buffer),
+ .cs_change = 1,
+ }, {
+ .rx_buf = &st->spi_rx_buffer,
+ .len = sizeof(st->spi_rx_buffer),
+ },
+ };
+ int ret;
+
+ /*
+ * Prepare SPI transmit buffer 16 bit-value big-endian format and
+ * reverses bit order to align with the LSB-first input on SDI port
+ * in order to meet the device communication requirements.
+ */
+ st->spi_tx_buffer = cpu_to_be16(bitrev16(FIELD_PREP(MAX14001_MASK_ADDR, reg)));
+
+ ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
+ if (ret)
+ return ret;
+
+ /*
+ * Convert received 16-bit value from big-endian to cpu-endian format
+ * and reverses bit order.
+ */
+ *val = FIELD_GET(MAX14001_MASK_DATA, bitrev16(be16_to_cpu(st->spi_rx_buffer)));
+
+ return 0;
+}
+
+static int max14001_write(struct max14001_state *st, unsigned int reg, unsigned int val)
+{
+ /*
+ * Prepare SPI transmit buffer 16 bit-value big-endian format and
+ * reverses bit order to align with the LSB-first input on SDI port
+ * in order to meet the device communication requirements.
+ */
+ st->spi_tx_buffer = cpu_to_be16(bitrev16(FIELD_PREP(MAX14001_MASK_ADDR, reg) |
+ FIELD_PREP(MAX14001_MASK_WR, 1) |
+ FIELD_PREP(MAX14001_MASK_DATA, val)));
+
+ return spi_write(st->spi, &st->spi_tx_buffer, sizeof(st->spi_tx_buffer));
+}
+
+static int max14001_write_single_reg(void *context, unsigned int reg, unsigned int val)
+{
+ struct max14001_state *st = context;
+ int ret;
+
+ /* Enable writing to the SPI register */
+ ret = max14001_write(st, MAX14001_REG_WEN, MAX14001_REG_WEN_VALUE_WRITE);
+ if (ret)
+ return ret;
+
+ /* Writing data into SPI register */
+ ret = max14001_write(st, reg, val);
+ if (ret)
+ return ret;
+
+ /* Disable writing to the SPI register */
+ return max14001_write(st, MAX14001_REG_WEN, 0);
+}
+
+static int max14001_write_verification_reg(struct max14001_state *st, unsigned int reg)
+{
+ unsigned int val;
+ int ret;
+
+ ret = regmap_read(st->regmap, reg, &val);
+ if (ret)
+ return ret;
+
+ return max14001_write(st, MAX14001_REG_VERIFICATION(reg), val);
+}
+
+static int max14001_disable_mv_fault(struct max14001_state *st)
+{
+ unsigned int reg;
+ int ret;
+
+ /* Enable writing to the SPI registers */
+ ret = max14001_write(st, MAX14001_REG_WEN, MAX14001_REG_WEN_VALUE_WRITE);
+ if (ret)
+ return ret;
+
+ /*
+ * Reads all registers and writes the values to their appropriate
+ * verification registers to clear the Memory Validation fault.
+ */
+ for (reg = MAX14001_REG_FLTEN; reg <= MAX14001_REG_ENBL; reg++) {
+ ret = max14001_write_verification_reg(st, reg);
+ if (ret)
+ return ret;
+ }
+
+ /* Disable writing to the SPI registers */
+ return max14001_write(st, MAX14001_REG_WEN, 0);
+}
+
+static int max14001_debugfs_reg_access(struct iio_dev *indio_dev,
+ unsigned int reg, unsigned int writeval,
+ unsigned int *readval)
+{
+ struct max14001_state *st = iio_priv(indio_dev);
+
+ if (readval)
+ return regmap_read(st->regmap, reg, readval);
+
+ return regmap_write(st->regmap, reg, writeval);
+}
+
+static int max14001_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2, long mask)
+{
+ struct max14001_state *st = iio_priv(indio_dev);
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ scoped_guard(mutex, &st->lock)
+ ret = regmap_read(st->regmap, MAX14001_REG_ADC, val);
+ if (ret)
+ return ret;
+
+ return IIO_VAL_INT;
+ case IIO_CHAN_INFO_AVERAGE_RAW:
+ scoped_guard(mutex, &st->lock)
+ ret = regmap_read(st->regmap, MAX14001_REG_FADC, val);
+ if (ret)
+ return ret;
+
+ return IIO_VAL_INT;
+ case IIO_CHAN_INFO_SCALE:
+ *val = st->vref_mV;
+ *val2 = 10;
+
+ return IIO_VAL_FRACTIONAL_LOG2;
+ default:
+ return -EINVAL;
+ }
+}
+
+static const struct regmap_config max14001_regmap_config = {
+ .reg_read = max14001_read,
+ .reg_write = max14001_write_single_reg,
+};
+
+static const struct iio_info max14001_info = {
+ .read_raw = max14001_read_raw,
+ .debugfs_reg_access = max14001_debugfs_reg_access,
+};
+
+static const struct iio_chan_spec max14001_channel[] = {
+ {
+ .type = IIO_VOLTAGE,
+ .indexed = 1,
+ .channel = 0,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+ BIT(IIO_CHAN_INFO_AVERAGE_RAW) |
+ BIT(IIO_CHAN_INFO_SCALE),
+ },
+};
+
+static int max14001_probe(struct spi_device *spi)
+{
+ struct device *dev = &spi->dev;
+ struct iio_dev *indio_dev;
+ struct max14001_state *st;
+ int ret, ext_vrefin = 0;
+
+ indio_dev = devm_iio_device_alloc(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(dev, -ENODEV, "Failed to get match data\n");
+
+ indio_dev->name = st->chip_info->name;
+ indio_dev->info = &max14001_info;
+ indio_dev->channels = max14001_channel;
+ indio_dev->num_channels = ARRAY_SIZE(max14001_channel);
+ indio_dev->modes = INDIO_DIRECT_MODE;
+
+ st->regmap = devm_regmap_init(dev, NULL, st, &max14001_regmap_config);
+ if (IS_ERR(st->regmap))
+ return dev_err_probe(dev, PTR_ERR(st->regmap), "Failed to initialize regmap\n");
+
+ ret = devm_mutex_init(dev, &st->lock);
+ if (ret)
+ return ret;
+
+ ret = devm_regulator_get_enable(dev, "vdd");
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to enable Vdd supply\n");
+
+ ret = devm_regulator_get_enable(dev, "vddl");
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to enable Vddl supply\n");
+
+ ret = devm_regulator_get_enable_read_voltage(dev, "refin");
+ if (ret < 0 && ret != -ENODEV)
+ return dev_err_probe(dev, ret, "Failed to get REFIN voltage\n");
+
+ if (ret < 0)
+ ret = 1250000;
+ else
+ ext_vrefin = 1;
+ st->vref_mV = ret / (MICRO / MILLI);
+
+ if (ext_vrefin) {
+ /*
+ * Configure the MAX14001/MAX14002 to use an external voltage reference source
+ * by setting the bit 5 of the configuration register
+ */
+ ret = regmap_update_bits(st->regmap, MAX14001_REG_CFG, MAX14001_REG_CFG_BIT_EXRF, MAX14001_REG_CFG_BIT_EXRF);
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to set External REFIN in Configuration Register\n");
+ }
+
+ ret = max14001_disable_mv_fault(st);
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to disable MV Fault\n");
+
+ return devm_iio_device_register(dev, indio_dev);
+}
+
+static const struct spi_device_id max14001_id_table[] = {
+ { "max14001", (kernel_ulong_t)&max14001_chip_info },
+ { "max14002", (kernel_ulong_t)&max14002_chip_info },
+ { }
+};
+
+static const struct of_device_id max14001_of_match[] = {
+ { .compatible = "adi,max14001", .data = &max14001_chip_info },
+ { .compatible = "adi,max14002", .data = &max14002_chip_info },
+ { }
+};
+MODULE_DEVICE_TABLE(of, max14001_of_match);
+
+static struct spi_driver max14001_driver = {
+ .driver = {
+ .name = "max14001",
+ .of_match_table = max14001_of_match,
+ },
+ .probe = max14001_probe,
+ .id_table = max14001_id_table,
+};
+module_spi_driver(max14001_driver);
+
+MODULE_AUTHOR("Kim Seer Paller <kimseer.paller@analog.com>");
+MODULE_AUTHOR("Marilene Andrade Garcia <marilene.agarcia@gmail.com>");
+MODULE_DESCRIPTION("Analog Devices MAX14001/MAX14002 ADCs driver");
+MODULE_LICENSE("GPL");
--
2.34.1On 9/15/25 5:16 PM, Marilene Andrade Garcia wrote:
> The MAX14001/MAX14002 is configurable, isolated 10-bit ADCs for multi-range
> binary inputs. In addition to ADC readings, the MAX14001/MAX14002 offers
> more features, like a binary comparator, a filtered reading that can
> provide the average of the last 2, 4, or 8 ADC readings, and an inrush
> comparator that triggers the inrush current. There is also a fault feature
> that can diagnose seven possible fault conditions. And an option to select
> an external or internal ADC voltage reference.
>
> MAX14001/MAX14002 features implemented so far:
> - Raw ADC reading.
> - Filtered ADC average reading with the default configuration.
> - MV fault disable.
> - Selection of external or internal ADC voltage reference, depending on
> whether it is declared in the device tree.
>
> Co-developed-by: Kim Seer Paller <kimseer.paller@analog.com>
> Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
> Signed-off-by: Marilene Andrade Garcia <marilene.agarcia@gmail.com>
> ---
>
> I have addressed almost all of the comments, thank you very much for the
> review. I would like to highlight some of them:
>
> Changes since v10:
> - Dropped the kernel.h include
> - Add the cleanup.h, mutex.h, regmap.h and units.h includes
> - Renamed the reg_addr variable name to reg
> - Renamed the reg_data variable name to val
> - Added the regmap implementation
> - Used scoped_guard()
> - Refactored the get refin voltage code
> - Replace max14001_chip_model with data structures separated
> - Added debugfs_reg_access
>
> Change I was not able to do:
> - I could not remove bitrev16 because I am using an SPI controller that
> does not support SPI_LSB_FIRST. So I suggest keeping bitrev16 and not using
> the spi-lsb-first devicetree property for now, since this driver currently
> works for both types of controllers: those that support it and those that
> do not. I left a TODO comment to address this issue as soon as the SPI
> kernel code starts handling the bit-reverse operation for controllers that
> do not have this support. Once I finish my work on this driver, if the SPI
> code still does not include this handling, I can submit patches to add it.
I looked more at what it would take to implement this in the SPI core code
and found that it would actually be quite difficult to do in a generic way
because there are so many edge/corner/n-dim cases. We can't change tx_buf
data in-place because it might be const data that is in some memory area
that can't be modified. And things would get complicated if different
transfers pointed to the same buffer memory addresses anyway. So we would
basically have to allocate new memory for all buffers, copy all tx data to
that new memory, reverse all of the tx bits, and update all the pointers in
the transfer structs. Then when the message was finished, we would have to
reverse all of the rx bits, copy all of the rx buffers back to the original
buffers and put all the buffer pointers back the way they were. But this
could write over some of the original tx data if tx_buf and rx_buf point to
the same original buffer, which would break things if a peripheral driver
expected the tx data to persist. And we can't do this during the SPI optimize
step because that currently allows the tx_buf data values to be modified after
optimization.
So perhaps it is best to just handle it in the peripheral driver. It will
be much more efficent that way anyway.
However, we still do want to handle SPI_LSB_FIRST now so that people with
hardware support can be more efficient and we don't want things to break
if someone puts spi-lsb-first in the devicetree.
I made some comments below on how we could handle this.
>
> MAINTAINERS | 1 +
> drivers/iio/adc/Kconfig | 10 ++
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/max14001.c | 356 +++++++++++++++++++++++++++++++++++++
> 4 files changed, 368 insertions(+)
> create mode 100644 drivers/iio/adc/max14001.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 0bae420caa63..a9cf93ba8b21 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -14998,6 +14998,7 @@ L: linux-iio@vger.kernel.org
> S: Maintained
> W: https://ez.analog.com/linux-software-drivers
> F: Documentation/devicetree/bindings/iio/adc/adi,max14001.yaml
> +F: drivers/iio/adc/max14001.c
>
> MAXBOTIX ULTRASONIC RANGER IIO DRIVER
> M: Andreas Klinger <ak@it-klinger.de>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 237fa2061329..a1f2afce60ad 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -991,6 +991,16 @@ config MAX11410
> To compile this driver as a module, choose M here: the module will be
> called max11410.
>
> +config MAX14001
> + tristate "Analog Devices MAX14001/MAX14002 ADC driver"
> + depends on SPI
> + help
> + Say yes here to build support for Analog Devices MAX14001/MAX14002
> + Configurable, Isolated 10-bit ADCs for Multi-Range Binary Inputs.
> +
> + To compile this driver as a module, choose M here: the module will be
> + called max14001.
> +
Shouldn't MAX14... be after MAX12... for alphabetical order?
> config MAX1241
> tristate "Maxim max1241 ADC driver"
> depends on SPI_MASTER
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 611c16430621..9c4ceb527db7 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -87,6 +87,7 @@ obj-$(CONFIG_MAX11100) += max11100.o
> obj-$(CONFIG_MAX1118) += max1118.o
> obj-$(CONFIG_MAX11205) += max11205.o
> obj-$(CONFIG_MAX11410) += max11410.o
> +obj-$(CONFIG_MAX14001) += max14001.o
Same here.
> obj-$(CONFIG_MAX1241) += max1241.o
> obj-$(CONFIG_MAX1363) += max1363.o
> obj-$(CONFIG_MAX34408) += max34408.o
> diff --git a/drivers/iio/adc/max14001.c b/drivers/iio/adc/max14001.c
> new file mode 100644
> index 000000000000..2ade57970064
> --- /dev/null
> +++ b/drivers/iio/adc/max14001.c
> @@ -0,0 +1,356 @@
> +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-3-Clause)
> +/*
> + * Analog Devices MAX14001/MAX14002 ADC driver
> + *
> + * Copyright (C) 2023-2025 Analog Devices Inc.
> + * Copyright (C) 2023 Kim Seer Paller <kimseer.paller@analog.com>
> + * Copyright (c) 2025 Marilene Andrade Garcia <marilene.agarcia@gmail.com>
> + *
> + * Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/MAX14001-MAX14002.pdf
> + */
> +
> +/*
> + * TODO:
> + * Replace bitrev16 with SPI_LSB_FIRST once the SPI kernel code supports handling
> + * SPI controllers that lack LSB-first support.
> + */
> +
> +#include <linux/array_size.h>
> +#include <linux/bitfield.h>
> +#include <linux/bitrev.h>
> +#include <linux/bits.h>
> +#include <linux/byteorder/generic.h>
> +#include <linux/cleanup.h>
> +#include <linux/device.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +#include <linux/types.h>
> +#include <linux/units.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/types.h>
> +
> +/* MAX14001 Registers Address */
> +#define MAX14001_REG_ADC 0x00
> +#define MAX14001_REG_FADC 0x01
> +#define MAX14001_REG_FLAGS 0x02
> +#define MAX14001_REG_FLTEN 0x03
> +#define MAX14001_REG_THL 0x04
> +#define MAX14001_REG_THU 0x05
> +#define MAX14001_REG_INRR 0x06
> +#define MAX14001_REG_INRT 0x07
> +#define MAX14001_REG_INRP 0x08
> +#define MAX14001_REG_CFG 0x09
> +#define MAX14001_REG_ENBL 0x0A
> +#define MAX14001_REG_ACT 0x0B
> +#define MAX14001_REG_WEN 0x0C
> +
> +#define MAX14001_REG_VERIFICATION(x) ((x) + 0x10)
> +
> +#define MAX14001_REG_CFG_BIT_EXRF BIT(5)
> +
> +#define MAX14001_REG_WEN_VALUE_WRITE 0x294
> +
> +#define MAX14001_MASK_ADDR GENMASK(15, 11)
> +#define MAX14001_MASK_WR BIT(10)
> +#define MAX14001_MASK_DATA GENMASK(9, 0)
> +
> +struct max14001_state {
> + const struct max14001_chip_info *chip_info;
> + struct spi_device *spi;
> + struct regmap *regmap;
> + int vref_mV;
> + /*
> + * lock protect against multiple concurrent accesses, RMW sequence,
> + * and SPI transfer.
regmap has it's own locking. Currently, this is only protecteing a couple
of reads. So eaither we don't need this because the lock in regmap is good
enough or we have missed some places where locking would be needed.
In other words, we probably don't need this lock until later, so could
drop it for now.
> + */
> + struct mutex lock;
bool spi_hw_has_lsb_first;
> + /*
> + * The following buffers will be bit-reversed during device
> + * communication, because the device transmits and receives data
> + * LSB-first.
> + * DMA (thus cache coherency maintenance) requires the transfer
> + * buffers to live in their own cache lines.
> + */
> + __be16 spi_tx_buffer __aligned(IIO_DMA_MINALIGN);
> + __be16 spi_rx_buffer;
These would no longer be strictly big-endian, so we could
just make them u16 with a note in the comments.
> +};
> +
> +struct max14001_chip_info {
> + const char *name;
> +};
> +
> +static struct max14001_chip_info max14001_chip_info = {
> + .name = "max14001",
> +};
> +
> +static struct max14001_chip_info max14002_chip_info = {
> + .name = "max14002",
> +};
> +
> +static int max14001_read(void *context, unsigned int reg, unsigned int *val)
> +{
> + struct max14001_state *st = context;
> + struct spi_transfer xfers[] = {
> + {
> + .tx_buf = &st->spi_tx_buffer,
> + .len = sizeof(st->spi_tx_buffer),
> + .cs_change = 1,
> + }, {
> + .rx_buf = &st->spi_rx_buffer,
> + .len = sizeof(st->spi_rx_buffer),
> + },
> + };
> + int ret;
u16 addr, data;
> +
> + /*
> + * Prepare SPI transmit buffer 16 bit-value big-endian format and
> + * reverses bit order to align with the LSB-first input on SDI port
> + * in order to meet the device communication requirements.
> + */
> + st->spi_tx_buffer = cpu_to_be16(bitrev16(FIELD_PREP(MAX14001_MASK_ADDR, reg)));
addr = FIELD_PREP(MAX14001_MASK_ADDR, reg);
if (st->spi_hw_has_lsb_first)
st->spi_tx_buffer = cpu_to_le16(addr);
else
st->spi_tx_buffer = cpu_to_be16(bitrev16(addr));
> +
> + ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
> + if (ret)
> + return ret;
> +
> + /*
> + * Convert received 16-bit value from big-endian to cpu-endian format
> + * and reverses bit order.
> + */
> + *val = FIELD_GET(MAX14001_MASK_DATA, bitrev16(be16_to_cpu(st->spi_rx_buffer)));
if (st->spi_hw_has_lsb_first)
data = le16_to_cpu(st->spi_rx_buffer);
else
data = bitrev16(be16_to_cpu(st->spi_rx_buffer));
val = FIELD_GET(MAX14001_MASK_DATA, data);
> +
> + return 0;
> +}
> +
> +static int max14001_write(struct max14001_state *st, unsigned int reg, unsigned int val)
> +{
u16 addr;
> + /*
> + * Prepare SPI transmit buffer 16 bit-value big-endian format and
> + * reverses bit order to align with the LSB-first input on SDI port
> + * in order to meet the device communication requirements.
> + */
> + st->spi_tx_buffer = cpu_to_be16(bitrev16(FIELD_PREP(MAX14001_MASK_ADDR, reg) |
> + FIELD_PREP(MAX14001_MASK_WR, 1) |
> + FIELD_PREP(MAX14001_MASK_DATA, val)));
addr = FIELD_PREP(MAX14001_MASK_ADDR, reg) |
FIELD_PREP(MAX14001_MASK_WR, 1) |
FIELD_PREP(MAX14001_MASK_DATA, val);
if (st->spi_hw_has_lsb_first)
st->spi_tx_buffer = cpu_to_le16(addr);
else
st->spi_tx_buffer = cpu_to_be16(bitrev16(addr));
> +
> + return spi_write(st->spi, &st->spi_tx_buffer, sizeof(st->spi_tx_buffer));
> +}
> +
> +static int max14001_write_single_reg(void *context, unsigned int reg, unsigned int val)
> +{
> + struct max14001_state *st = context;
> + int ret;
> +
> + /* Enable writing to the SPI register */
> + ret = max14001_write(st, MAX14001_REG_WEN, MAX14001_REG_WEN_VALUE_WRITE);
> + if (ret)
> + return ret;
> +
> + /* Writing data into SPI register */
> + ret = max14001_write(st, reg, val);
> + if (ret)
> + return ret;
> +
> + /* Disable writing to the SPI register */
> + return max14001_write(st, MAX14001_REG_WEN, 0);
> +}
> +
> +static int max14001_write_verification_reg(struct max14001_state *st, unsigned int reg)
> +{
> + unsigned int val;
> + int ret;
> +
> + ret = regmap_read(st->regmap, reg, &val);
> + if (ret)
> + return ret;
> +
> + return max14001_write(st, MAX14001_REG_VERIFICATION(reg), val);
> +}
> +
> +static int max14001_disable_mv_fault(struct max14001_state *st)
> +{
> + unsigned int reg;
> + int ret;
> +
> + /* Enable writing to the SPI registers */
> + ret = max14001_write(st, MAX14001_REG_WEN, MAX14001_REG_WEN_VALUE_WRITE);
> + if (ret)
> + return ret;
> +
> + /*
> + * Reads all registers and writes the values to their appropriate
> + * verification registers to clear the Memory Validation fault.
> + */
> + for (reg = MAX14001_REG_FLTEN; reg <= MAX14001_REG_ENBL; reg++) {
> + ret = max14001_write_verification_reg(st, reg);
> + if (ret)
> + return ret;
> + }
> +
> + /* Disable writing to the SPI registers */
> + return max14001_write(st, MAX14001_REG_WEN, 0);
> +}
> +
> +static int max14001_debugfs_reg_access(struct iio_dev *indio_dev,
> + unsigned int reg, unsigned int writeval,
> + unsigned int *readval)
> +{
> + struct max14001_state *st = iio_priv(indio_dev);
> +
> + if (readval)
> + return regmap_read(st->regmap, reg, readval);
> +
> + return regmap_write(st->regmap, reg, writeval);
> +}
> +
> +static int max14001_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct max14001_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + scoped_guard(mutex, &st->lock)
> + ret = regmap_read(st->regmap, MAX14001_REG_ADC, val);
> + if (ret)
> + return ret;
> +
> + return IIO_VAL_INT;
The scoped_guard() looks a bit odd with the extra indent. I would write
it like this instead:
case IIO_CHAN_INFO_RAW: {
guard(mutex)(&st->lock);
ret = regmap_read(st->regmap, MAX14001_REG_ADC, val);
if (ret)
return ret;
return IIO_VAL_INT;
}
> + case IIO_CHAN_INFO_AVERAGE_RAW:
> + scoped_guard(mutex, &st->lock)
> + ret = regmap_read(st->regmap, MAX14001_REG_FADC, val);
> + if (ret)
> + return ret;
> +
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_SCALE:
> + *val = st->vref_mV;
> + *val2 = 10;
> +
> + return IIO_VAL_FRACTIONAL_LOG2;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static const struct regmap_config max14001_regmap_config = {
> + .reg_read = max14001_read,
> + .reg_write = max14001_write_single_reg,
.max_register = ?;
Setting max_register is useful.
> +};
> +
> +static const struct iio_info max14001_info = {
> + .read_raw = max14001_read_raw,
> + .debugfs_reg_access = max14001_debugfs_reg_access,
> +};
> +
> +static const struct iio_chan_spec max14001_channel[] = {
> + {
> + .type = IIO_VOLTAGE,
> + .indexed = 1,
> + .channel = 0,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_AVERAGE_RAW) |
I think there was some discussion about this last time [1] that it
would actually make sense to have the average as a separate channel.
There are a few reasons for this. If we add buffered read support
later, we could only read the average in a buffered read if it is
a separate channel. And the average channel should have an extra
control that is either the -3 dB low pass filter point (using existing
ABI) or the averaging window size (potentially new ABI).
If we don't care about ever reading the filtered and not filtered
value at the same time, we could just have a single channel and only
ever read from the FADC register and ignore the ADC register.
In either case, we would not need IIO_CHAN_INFO_AVERAGE_RAW.
[1]: https://lore.kernel.org/linux-iio/20250903171105.00003dcd@huawei.com/
> + BIT(IIO_CHAN_INFO_SCALE),
> + },
> +};
> +
> +static int max14001_probe(struct spi_device *spi)
> +{
> + struct device *dev = &spi->dev;
> + struct iio_dev *indio_dev;
> + struct max14001_state *st;
> + int ret, ext_vrefin = 0;
> +
> + indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + st = iio_priv(indio_dev);
> + st->spi = spi;
st->spi_hw_has_lsb_first = spi->mode & SPI_LSB_FIRST;
> + st->chip_info = spi_get_device_match_data(spi);
> + if (!st->chip_info)
> + return dev_err_probe(dev, -ENODEV, "Failed to get match data\n");
> +
> + indio_dev->name = st->chip_info->name;
> + indio_dev->info = &max14001_info;
> + indio_dev->channels = max14001_channel;
> + indio_dev->num_channels = ARRAY_SIZE(max14001_channel);
> + indio_dev->modes = INDIO_DIRECT_MODE;
> +
> + st->regmap = devm_regmap_init(dev, NULL, st, &max14001_regmap_config);
> + if (IS_ERR(st->regmap))
> + return dev_err_probe(dev, PTR_ERR(st->regmap), "Failed to initialize regmap\n");
> +
> + ret = devm_mutex_init(dev, &st->lock);
> + if (ret)
> + return ret;
> +
> + ret = devm_regulator_get_enable(dev, "vdd");
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to enable Vdd supply\n");
> +
> + ret = devm_regulator_get_enable(dev, "vddl");
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to enable Vddl supply\n");
> +
> + ret = devm_regulator_get_enable_read_voltage(dev, "refin");
> + if (ret < 0 && ret != -ENODEV)
> + return dev_err_probe(dev, ret, "Failed to get REFIN voltage\n");
> +
> + if (ret < 0)
> + ret = 1250000;
> + else
> + ext_vrefin = 1;
> + st->vref_mV = ret / (MICRO / MILLI);
Just a style choice here, but in other drivers with similar handling
we wrote it like this to avoid the extra if statement:
if (ret < 0 && ret != -ENODEV)
return dev_err_probe(dev, ret, "Failed to get REFIN voltage\n");
ext_vrefin = ret != -ENODEV;
st->vref_mV = ext_vrefin ? ret / 1000 : 1250;
Keeping (MICRO / MILLI) instead of 1000 is fine too. There are varying opinions
on this.
Or we could drop ext_vrefin and have:
if (ret < 0 && ret != -ENODEV)
return dev_err_probe(dev, ret, "Failed to get REFIN voltage\n");
if (ret != -ENODEV) {
st->vref_mV = ret / 1000;
/* regmap set bits goes here. */
...
} else {
st->vref_mV = 1250;
}
> +
> + if (ext_vrefin) {
> + /*
> + * Configure the MAX14001/MAX14002 to use an external voltage reference source
> + * by setting the bit 5 of the configuration register
> + */
> + ret = regmap_update_bits(st->regmap, MAX14001_REG_CFG, MAX14001_REG_CFG_BIT_EXRF, MAX14001_REG_CFG_BIT_EXRF);
Can be simplifed with regmap_set_bits().
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to set External REFIN in Configuration Register\n");
> + }
These lines are getting very long. We try to wrap to 80 characters
as much as we can in the IIO subsystem.
> +
> + ret = max14001_disable_mv_fault(st);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to disable MV Fault\n");
> +
> + return devm_iio_device_register(dev, indio_dev);
> +}
> +
> +static const struct spi_device_id max14001_id_table[] = {
> + { "max14001", (kernel_ulong_t)&max14001_chip_info },
> + { "max14002", (kernel_ulong_t)&max14002_chip_info },
> + { }
> +};
> +
> +static const struct of_device_id max14001_of_match[] = {
> + { .compatible = "adi,max14001", .data = &max14001_chip_info },
> + { .compatible = "adi,max14002", .data = &max14002_chip_info },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, max14001_of_match);
> +
> +static struct spi_driver max14001_driver = {
> + .driver = {
> + .name = "max14001",
> + .of_match_table = max14001_of_match,
> + },
> + .probe = max14001_probe,
> + .id_table = max14001_id_table,
> +};
> +module_spi_driver(max14001_driver);
> +
> +MODULE_AUTHOR("Kim Seer Paller <kimseer.paller@analog.com>");
> +MODULE_AUTHOR("Marilene Andrade Garcia <marilene.agarcia@gmail.com>");
> +MODULE_DESCRIPTION("Analog Devices MAX14001/MAX14002 ADCs driver");
> +MODULE_LICENSE("GPL");
On 16/09/2025 15:04, David Lechner wrote:
> On 9/15/25 5:16 PM, Marilene Andrade Garcia wrote:
>> The MAX14001/MAX14002 is configurable, isolated 10-bit ADCs for multi-range
>> binary inputs. In addition to ADC readings, the MAX14001/MAX14002 offers
>> more features, like a binary comparator, a filtered reading that can
>> provide the average of the last 2, 4, or 8 ADC readings, and an inrush
>> comparator that triggers the inrush current. There is also a fault feature
>> that can diagnose seven possible fault conditions. And an option to select
>> an external or internal ADC voltage reference.
>>
>> MAX14001/MAX14002 features implemented so far:
>> - Raw ADC reading.
>> - Filtered ADC average reading with the default configuration.
>> - MV fault disable.
>> - Selection of external or internal ADC voltage reference, depending on
>> whether it is declared in the device tree.
>>
>> Co-developed-by: Kim Seer Paller <kimseer.paller@analog.com>
>> Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
>> Signed-off-by: Marilene Andrade Garcia <marilene.agarcia@gmail.com>
>> ---
>>
>> I have addressed almost all of the comments, thank you very much for the
>> review. I would like to highlight some of them:
>>
>> Changes since v10:
>> - Dropped the kernel.h include
>> - Add the cleanup.h, mutex.h, regmap.h and units.h includes
>> - Renamed the reg_addr variable name to reg
>> - Renamed the reg_data variable name to val
>> - Added the regmap implementation
>> - Used scoped_guard()
>> - Refactored the get refin voltage code
>> - Replace max14001_chip_model with data structures separated
>> - Added debugfs_reg_access
>>
>> Change I was not able to do:
>> - I could not remove bitrev16 because I am using an SPI controller that
>> does not support SPI_LSB_FIRST. So I suggest keeping bitrev16 and not using
>> the spi-lsb-first devicetree property for now, since this driver currently
>> works for both types of controllers: those that support it and those that
>> do not. I left a TODO comment to address this issue as soon as the SPI
>> kernel code starts handling the bit-reverse operation for controllers that
>> do not have this support. Once I finish my work on this driver, if the SPI
>> code still does not include this handling, I can submit patches to add it.
>
> I looked more at what it would take to implement this in the SPI core code
> and found that it would actually be quite difficult to do in a generic way
> because there are so many edge/corner/n-dim cases. We can't change tx_buf
> data in-place because it might be const data that is in some memory area
> that can't be modified. And things would get complicated if different
> transfers pointed to the same buffer memory addresses anyway. So we would
> basically have to allocate new memory for all buffers, copy all tx data to
> that new memory, reverse all of the tx bits, and update all the pointers in
> the transfer structs. Then when the message was finished, we would have to
> reverse all of the rx bits, copy all of the rx buffers back to the original
> buffers and put all the buffer pointers back the way they were. But this
> could write over some of the original tx data if tx_buf and rx_buf point to
> the same original buffer, which would break things if a peripheral driver
> expected the tx data to persist. And we can't do this during the SPI optimize
> step because that currently allows the tx_buf data values to be modified after
> optimization.
>
> So perhaps it is best to just handle it in the peripheral driver. It will
> be much more efficent that way anyway.
>
> However, we still do want to handle SPI_LSB_FIRST now so that people with
> hardware support can be more efficient and we don't want things to break
> if someone puts spi-lsb-first in the devicetree.
>
> I made some comments below on how we could handle this.
>
>>
>> MAINTAINERS | 1 +
>> drivers/iio/adc/Kconfig | 10 ++
>> drivers/iio/adc/Makefile | 1 +
>> drivers/iio/adc/max14001.c | 356 +++++++++++++++++++++++++++++++++++++
>> 4 files changed, 368 insertions(+)
>> create mode 100644 drivers/iio/adc/max14001.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 0bae420caa63..a9cf93ba8b21 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -14998,6 +14998,7 @@ L: linux-iio@vger.kernel.org
>> S: Maintained
>> W: https://ez.analog.com/linux-software-drivers
>> F: Documentation/devicetree/bindings/iio/adc/adi,max14001.yaml
>> +F: drivers/iio/adc/max14001.c
>>
>> MAXBOTIX ULTRASONIC RANGER IIO DRIVER
>> M: Andreas Klinger <ak@it-klinger.de>
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index 237fa2061329..a1f2afce60ad 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -991,6 +991,16 @@ config MAX11410
>> To compile this driver as a module, choose M here: the module will be
>> called max11410.
>>
>> +config MAX14001
>> + tristate "Analog Devices MAX14001/MAX14002 ADC driver"
>> + depends on SPI
>> + help
>> + Say yes here to build support for Analog Devices MAX14001/MAX14002
>> + Configurable, Isolated 10-bit ADCs for Multi-Range Binary Inputs.
>> +
>> + To compile this driver as a module, choose M here: the module will be
>> + called max14001.
>> +
>
> Shouldn't MAX14... be after MAX12... for alphabetical order?
>
>> config MAX1241
>> tristate "Maxim max1241 ADC driver"
>> depends on SPI_MASTER
>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>> index 611c16430621..9c4ceb527db7 100644
>> --- a/drivers/iio/adc/Makefile
>> +++ b/drivers/iio/adc/Makefile
>> @@ -87,6 +87,7 @@ obj-$(CONFIG_MAX11100) += max11100.o
>> obj-$(CONFIG_MAX1118) += max1118.o
>> obj-$(CONFIG_MAX11205) += max11205.o
>> obj-$(CONFIG_MAX11410) += max11410.o
>> +obj-$(CONFIG_MAX14001) += max14001.o
>
> Same here.
>
>> obj-$(CONFIG_MAX1241) += max1241.o
>> obj-$(CONFIG_MAX1363) += max1363.o
>> obj-$(CONFIG_MAX34408) += max34408.o
>> diff --git a/drivers/iio/adc/max14001.c b/drivers/iio/adc/max14001.c
>> new file mode 100644
>> index 000000000000..2ade57970064
>> --- /dev/null
>> +++ b/drivers/iio/adc/max14001.c
>> @@ -0,0 +1,356 @@
>> +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-3-Clause)
>> +/*
>> + * Analog Devices MAX14001/MAX14002 ADC driver
>> + *
>> + * Copyright (C) 2023-2025 Analog Devices Inc.
>> + * Copyright (C) 2023 Kim Seer Paller <kimseer.paller@analog.com>
>> + * Copyright (c) 2025 Marilene Andrade Garcia <marilene.agarcia@gmail.com>
>> + *
>> + * Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/MAX14001-MAX14002.pdf
>> + */
>> +
>> +/*
>> + * TODO:
>> + * Replace bitrev16 with SPI_LSB_FIRST once the SPI kernel code supports handling
>> + * SPI controllers that lack LSB-first support.
>> + */
>> +
>> +#include <linux/array_size.h>
>> +#include <linux/bitfield.h>
>> +#include <linux/bitrev.h>
>> +#include <linux/bits.h>
>> +#include <linux/byteorder/generic.h>
>> +#include <linux/cleanup.h>
>> +#include <linux/device.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/module.h>
>> +#include <linux/mutex.h>
>> +#include <linux/regmap.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/spi/spi.h>
>> +#include <linux/types.h>
>> +#include <linux/units.h>
>> +
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/types.h>
>> +
>> +/* MAX14001 Registers Address */
>> +#define MAX14001_REG_ADC 0x00
>> +#define MAX14001_REG_FADC 0x01
>> +#define MAX14001_REG_FLAGS 0x02
>> +#define MAX14001_REG_FLTEN 0x03
>> +#define MAX14001_REG_THL 0x04
>> +#define MAX14001_REG_THU 0x05
>> +#define MAX14001_REG_INRR 0x06
>> +#define MAX14001_REG_INRT 0x07
>> +#define MAX14001_REG_INRP 0x08
>> +#define MAX14001_REG_CFG 0x09
>> +#define MAX14001_REG_ENBL 0x0A
>> +#define MAX14001_REG_ACT 0x0B
>> +#define MAX14001_REG_WEN 0x0C
>> +
>> +#define MAX14001_REG_VERIFICATION(x) ((x) + 0x10)
>> +
>> +#define MAX14001_REG_CFG_BIT_EXRF BIT(5)
>> +
>> +#define MAX14001_REG_WEN_VALUE_WRITE 0x294
>> +
>> +#define MAX14001_MASK_ADDR GENMASK(15, 11)
>> +#define MAX14001_MASK_WR BIT(10)
>> +#define MAX14001_MASK_DATA GENMASK(9, 0)
>> +
>> +struct max14001_state {
>> + const struct max14001_chip_info *chip_info;
>> + struct spi_device *spi;
>> + struct regmap *regmap;
>> + int vref_mV;
>> + /*
>> + * lock protect against multiple concurrent accesses, RMW sequence,
>> + * and SPI transfer.
>
> regmap has it's own locking. Currently, this is only protecteing a couple
> of reads. So eaither we don't need this because the lock in regmap is good
> enough or we have missed some places where locking would be needed.
>
> In other words, we probably don't need this lock until later, so could
> drop it for now.
>
>> + */
>> + struct mutex lock;
>
> bool spi_hw_has_lsb_first;
>
>> + /*
>> + * The following buffers will be bit-reversed during device
>> + * communication, because the device transmits and receives data
>> + * LSB-first.
>> + * DMA (thus cache coherency maintenance) requires the transfer
>> + * buffers to live in their own cache lines.
>> + */
>> + __be16 spi_tx_buffer __aligned(IIO_DMA_MINALIGN);
>> + __be16 spi_rx_buffer;
>
> These would no longer be strictly big-endian, so we could
> just make them u16 with a note in the comments.
Hello David, I have some doubts that I would like to clarify before
sending v12. Since I am not able to test the case using SPI_LSB_FIRST, I
noticed that you suggest saving the data as __le in this case. Just out
of curiosity, if I use SPI_LSB_FIRST, would saving the data as __be lead
to errors?
>
>> +};
>> +
>> +struct max14001_chip_info {
>> + const char *name;
>> +};
>> +
>> +static struct max14001_chip_info max14001_chip_info = {
>> + .name = "max14001",
>> +};
>> +
>> +static struct max14001_chip_info max14002_chip_info = {
>> + .name = "max14002",
>> +};
>> +
>> +static int max14001_read(void *context, unsigned int reg, unsigned int *val)
>> +{
>> + struct max14001_state *st = context;
>> + struct spi_transfer xfers[] = {
>> + {
>> + .tx_buf = &st->spi_tx_buffer,
>> + .len = sizeof(st->spi_tx_buffer),
>> + .cs_change = 1,
>> + }, {
>> + .rx_buf = &st->spi_rx_buffer,
>> + .len = sizeof(st->spi_rx_buffer),
>> + },
>> + };
>> + int ret;
>
> u16 addr, data;
>
>> +
>> + /*
>> + * Prepare SPI transmit buffer 16 bit-value big-endian format and
>> + * reverses bit order to align with the LSB-first input on SDI port
>> + * in order to meet the device communication requirements.
>> + */
>> + st->spi_tx_buffer = cpu_to_be16(bitrev16(FIELD_PREP(MAX14001_MASK_ADDR, reg)));
>
> addr = FIELD_PREP(MAX14001_MASK_ADDR, reg);
>
> if (st->spi_hw_has_lsb_first)
> st->spi_tx_buffer = cpu_to_le16(addr);
> else
> st->spi_tx_buffer = cpu_to_be16(bitrev16(addr));
> > +
>> + ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
>> + if (ret)
>> + return ret;
>> +
>> + /*
>> + * Convert received 16-bit value from big-endian to cpu-endian format
>> + * and reverses bit order.
>> + */
>> + *val = FIELD_GET(MAX14001_MASK_DATA, bitrev16(be16_to_cpu(st->spi_rx_buffer)));
>
> if (st->spi_hw_has_lsb_first)
> data = le16_to_cpu(st->spi_rx_buffer);
> else
> data = bitrev16(be16_to_cpu(st->spi_rx_buffer));
>
> val = FIELD_GET(MAX14001_MASK_DATA, data);
>
>> +
>> + return 0;
>> +}
>> +
>> +static int max14001_write(struct max14001_state *st, unsigned int reg, unsigned int val)
>> +{
>
> u16 addr;
>
>> + /*
>> + * Prepare SPI transmit buffer 16 bit-value big-endian format and
>> + * reverses bit order to align with the LSB-first input on SDI port
>> + * in order to meet the device communication requirements.
>> + */
>> + st->spi_tx_buffer = cpu_to_be16(bitrev16(FIELD_PREP(MAX14001_MASK_ADDR, reg) |
>> + FIELD_PREP(MAX14001_MASK_WR, 1) |
>> + FIELD_PREP(MAX14001_MASK_DATA, val)));
>
>
> addr = FIELD_PREP(MAX14001_MASK_ADDR, reg) |
> FIELD_PREP(MAX14001_MASK_WR, 1) |
> FIELD_PREP(MAX14001_MASK_DATA, val);
>
> if (st->spi_hw_has_lsb_first)
> st->spi_tx_buffer = cpu_to_le16(addr);
> else
> st->spi_tx_buffer = cpu_to_be16(bitrev16(addr));
>
>> +
>> + return spi_write(st->spi, &st->spi_tx_buffer, sizeof(st->spi_tx_buffer));
>> +}
>> +
>> +static int max14001_write_single_reg(void *context, unsigned int reg, unsigned int val)
>> +{
>> + struct max14001_state *st = context;
>> + int ret;
>> +
>> + /* Enable writing to the SPI register */
>> + ret = max14001_write(st, MAX14001_REG_WEN, MAX14001_REG_WEN_VALUE_WRITE);
>> + if (ret)
>> + return ret;
>> +
>> + /* Writing data into SPI register */
>> + ret = max14001_write(st, reg, val);
>> + if (ret)
>> + return ret;
>> +
>> + /* Disable writing to the SPI register */
>> + return max14001_write(st, MAX14001_REG_WEN, 0);
>> +}
>> +
>> +static int max14001_write_verification_reg(struct max14001_state *st, unsigned int reg)
>> +{
>> + unsigned int val;
>> + int ret;
>> +
>> + ret = regmap_read(st->regmap, reg, &val);
>> + if (ret)
>> + return ret;
>> +
>> + return max14001_write(st, MAX14001_REG_VERIFICATION(reg), val);
>> +}
>> +
>> +static int max14001_disable_mv_fault(struct max14001_state *st)
>> +{
>> + unsigned int reg;
>> + int ret;
>> +
>> + /* Enable writing to the SPI registers */
>> + ret = max14001_write(st, MAX14001_REG_WEN, MAX14001_REG_WEN_VALUE_WRITE);
>> + if (ret)
>> + return ret;
>> +
>> + /*
>> + * Reads all registers and writes the values to their appropriate
>> + * verification registers to clear the Memory Validation fault.
>> + */
>> + for (reg = MAX14001_REG_FLTEN; reg <= MAX14001_REG_ENBL; reg++) {
>> + ret = max14001_write_verification_reg(st, reg);
>> + if (ret)
>> + return ret;
>> + }
>> +
>> + /* Disable writing to the SPI registers */
>> + return max14001_write(st, MAX14001_REG_WEN, 0);
>> +}
>> +
>> +static int max14001_debugfs_reg_access(struct iio_dev *indio_dev,
>> + unsigned int reg, unsigned int writeval,
>> + unsigned int *readval)
>> +{
>> + struct max14001_state *st = iio_priv(indio_dev);
>> +
>> + if (readval)
>> + return regmap_read(st->regmap, reg, readval);
>> +
>> + return regmap_write(st->regmap, reg, writeval);
>> +}
>> +
>> +static int max14001_read_raw(struct iio_dev *indio_dev,
>> + struct iio_chan_spec const *chan,
>> + int *val, int *val2, long mask)
>> +{
>> + struct max14001_state *st = iio_priv(indio_dev);
>> + int ret;
>> +
>> + switch (mask) {
>> + case IIO_CHAN_INFO_RAW:
>> + scoped_guard(mutex, &st->lock)
>> + ret = regmap_read(st->regmap, MAX14001_REG_ADC, val);
>> + if (ret)
>> + return ret;
>> +
>> + return IIO_VAL_INT;
>
> The scoped_guard() looks a bit odd with the extra indent. I would write
> it like this instead:
>
>
>
> case IIO_CHAN_INFO_RAW: {
> guard(mutex)(&st->lock);
>
> ret = regmap_read(st->regmap, MAX14001_REG_ADC, val);
> if (ret)
> return ret;
>
> return IIO_VAL_INT;
> }
>
Ok, thank you. But since I removed the mutex, as it was mentioned in the
first comments, I should not use the guard, right? At least not for now.
>> + case IIO_CHAN_INFO_AVERAGE_RAW:
>> + scoped_guard(mutex, &st->lock)
>> + ret = regmap_read(st->regmap, MAX14001_REG_FADC, val);
>> + if (ret)
>> + return ret;
>> +
>> + return IIO_VAL_INT;
>> + case IIO_CHAN_INFO_SCALE:
>> + *val = st->vref_mV;
>> + *val2 = 10;
>> +
>> + return IIO_VAL_FRACTIONAL_LOG2;
>> + default:
>> + return -EINVAL;
>> + }
>> +}
>> +
>> +static const struct regmap_config max14001_regmap_config = {
>> + .reg_read = max14001_read,
>> + .reg_write = max14001_write_single_reg,
>
> .max_register = ?;
>
> Setting max_register is useful.
> > +};
>> +
>> +static const struct iio_info max14001_info = {
>> + .read_raw = max14001_read_raw,
>> + .debugfs_reg_access = max14001_debugfs_reg_access,
>> +};
>> +
>> +static const struct iio_chan_spec max14001_channel[] = {
>> + {
>> + .type = IIO_VOLTAGE,
>> + .indexed = 1,
>> + .channel = 0,
>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>> + BIT(IIO_CHAN_INFO_AVERAGE_RAW) |
>
> I think there was some discussion about this last time [1] that it
> would actually make sense to have the average as a separate channel.
>
> There are a few reasons for this. If we add buffered read support
> later, we could only read the average in a buffered read if it is
> a separate channel. And the average channel should have an extra
> control that is either the -3 dB low pass filter point (using existing
> ABI) or the averaging window size (potentially new ABI).
>
> If we don't care about ever reading the filtered and not filtered
> value at the same time, we could just have a single channel and only
> ever read from the FADC register and ignore the ADC register.
>
> In either case, we would not need IIO_CHAN_INFO_AVERAGE_RAW.
>
> [1]: https://lore.kernel.org/linux-iio/20250903171105.00003dcd@huawei.com/
>
>> + BIT(IIO_CHAN_INFO_SCALE),
>> + },
>> +};
>> +
>> +static int max14001_probe(struct spi_device *spi)
>> +{
>> + struct device *dev = &spi->dev;
>> + struct iio_dev *indio_dev;
>> + struct max14001_state *st;
>> + int ret, ext_vrefin = 0;
>> +
>> + indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
>> + if (!indio_dev)
>> + return -ENOMEM;
>> +
>> + st = iio_priv(indio_dev);
>> + st->spi = spi;
>
> st->spi_hw_has_lsb_first = spi->mode & SPI_LSB_FIRST;
>
>> + st->chip_info = spi_get_device_match_data(spi);
>> + if (!st->chip_info)
>> + return dev_err_probe(dev, -ENODEV, "Failed to get match data\n");
>> +
>> + indio_dev->name = st->chip_info->name;
>> + indio_dev->info = &max14001_info;
>> + indio_dev->channels = max14001_channel;
>> + indio_dev->num_channels = ARRAY_SIZE(max14001_channel);
>> + indio_dev->modes = INDIO_DIRECT_MODE;
>> +
>> + st->regmap = devm_regmap_init(dev, NULL, st, &max14001_regmap_config);
>> + if (IS_ERR(st->regmap))
>> + return dev_err_probe(dev, PTR_ERR(st->regmap), "Failed to initialize regmap\n");
>> +
>> + ret = devm_mutex_init(dev, &st->lock);
>> + if (ret)
>> + return ret;
>> +
>> + ret = devm_regulator_get_enable(dev, "vdd");
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to enable Vdd supply\n");
>> +
>> + ret = devm_regulator_get_enable(dev, "vddl");
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to enable Vddl supply\n");
>> +
>> + ret = devm_regulator_get_enable_read_voltage(dev, "refin");
>> + if (ret < 0 && ret != -ENODEV)
>> + return dev_err_probe(dev, ret, "Failed to get REFIN voltage\n");
>> +
>> + if (ret < 0)
>> + ret = 1250000;
>> + else
>> + ext_vrefin = 1;
>> + st->vref_mV = ret / (MICRO / MILLI);
>
> Just a style choice here, but in other drivers with similar handling
> we wrote it like this to avoid the extra if statement:
>
> if (ret < 0 && ret != -ENODEV)
> return dev_err_probe(dev, ret, "Failed to get REFIN voltage\n");
>
> ext_vrefin = ret != -ENODEV;
> st->vref_mV = ext_vrefin ? ret / 1000 : 1250;
>
> Keeping (MICRO / MILLI) instead of 1000 is fine too. There are varying opinions
> on this.
>
> Or we could drop ext_vrefin and have:
>
> if (ret < 0 && ret != -ENODEV)
> return dev_err_probe(dev, ret, "Failed to get REFIN voltage\n");
>
> if (ret != -ENODEV) {
> st->vref_mV = ret / 1000;
>
> /* regmap set bits goes here. */
> ...
> } else {
> st->vref_mV = 1250;
> }
>
>> +
>> + if (ext_vrefin) {
>> + /*
>> + * Configure the MAX14001/MAX14002 to use an external voltage reference source
>> + * by setting the bit 5 of the configuration register
>> + */
>> + ret = regmap_update_bits(st->regmap, MAX14001_REG_CFG, MAX14001_REG_CFG_BIT_EXRF, MAX14001_REG_CFG_BIT_EXRF);
>
> Can be simplifed with regmap_set_bits().
>
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to set External REFIN in Configuration Register\n");
>> + }
>
> These lines are getting very long. We try to wrap to 80 characters
> as much as we can in the IIO subsystem.
>
>> +
>> + ret = max14001_disable_mv_fault(st);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to disable MV Fault\n");
>> +
>> + return devm_iio_device_register(dev, indio_dev);
>> +}
>> +
>> +static const struct spi_device_id max14001_id_table[] = {
>> + { "max14001", (kernel_ulong_t)&max14001_chip_info },
>> + { "max14002", (kernel_ulong_t)&max14002_chip_info },
>> + { }
>> +};
>> +
>> +static const struct of_device_id max14001_of_match[] = {
>> + { .compatible = "adi,max14001", .data = &max14001_chip_info },
>> + { .compatible = "adi,max14002", .data = &max14002_chip_info },
>> + { }
>> +};
>> +MODULE_DEVICE_TABLE(of, max14001_of_match);
>> +
>> +static struct spi_driver max14001_driver = {
>> + .driver = {
>> + .name = "max14001",
>> + .of_match_table = max14001_of_match,
>> + },
>> + .probe = max14001_probe,
>> + .id_table = max14001_id_table,
>> +};
>> +module_spi_driver(max14001_driver);
>> +
>> +MODULE_AUTHOR("Kim Seer Paller <kimseer.paller@analog.com>");
>> +MODULE_AUTHOR("Marilene Andrade Garcia <marilene.agarcia@gmail.com>");
>> +MODULE_DESCRIPTION("Analog Devices MAX14001/MAX14002 ADCs driver");
>> +MODULE_LICENSE("GPL");
>
Best Regards,
Marilene
On 9/22/25 7:56 PM, Marilene Andrade Garcia wrote:
> On 16/09/2025 15:04, David Lechner wrote:
>> On 9/15/25 5:16 PM, Marilene Andrade Garcia wrote:
...
In general, please trim out extra stuff like I've done here when you
reply. It makes it easier to find the important parts. I hope I didn't
miss any of your questions.
>>> + /*
>>> + * The following buffers will be bit-reversed during device
>>> + * communication, because the device transmits and receives data
>>> + * LSB-first.
>>> + * DMA (thus cache coherency maintenance) requires the transfer
>>> + * buffers to live in their own cache lines.
>>> + */
>>> + __be16 spi_tx_buffer __aligned(IIO_DMA_MINALIGN);
>>> + __be16 spi_rx_buffer;
>>
>> These would no longer be strictly big-endian, so we could
>> just make them u16 with a note in the comments.
>
> Hello David, I have some doubts that I would like to clarify before sending v12. Since I am not able to test the case using SPI_LSB_FIRST, I noticed that you suggest saving the data as __le in this case. Just out of curiosity, if I use SPI_LSB_FIRST, would saving the data as __be lead to errors?
My thinking is that since we are sending things out 1 byte at a time, in order
for the least significant bit of 16 bits to be sent first, the least significant
byte has to be sent first. So will little-endian, the byte containing the least
significant bit of the 16 bits will be first in memory.
__be is just a naming convention and doesn't actually cause any bytes to
be swapped in memory. It just lets readers of the code know that the
value stored there has to be handled carefully because it may not be
cpu-endian. It would be confusing to readers to store a little-endian
value in a __be16 variable, but technically, no, it would not cause any
errors.
This is why I suggested to make it u16. It is still wrong but it is
equally wrong in both cases. If you still want to use __be16 though,
you could make a union instead.
union {
__be16 be;
__le16 le;
} spi_tx_buffer;
union {
__be16 be;
__le16 le;
} spi_rx_buffer;
>>
>> The scoped_guard() looks a bit odd with the extra indent. I would write
>> it like this instead:
>>
>>
>>
>> case IIO_CHAN_INFO_RAW: {
>> guard(mutex)(&st->lock);
>>
>> ret = regmap_read(st->regmap, MAX14001_REG_ADC, val);
>> if (ret)
>> return ret;
>>
>> return IIO_VAL_INT;
>> }
>>
>
> Ok, thank you. But since I removed the mutex, as it was mentioned in the first comments, I should not use the guard, right? At least not for now.
>
Correct. The regmap_read() has something similar internally already.
On 23/09/2025 11:27, David Lechner wrote:
> On 9/22/25 7:56 PM, Marilene Andrade Garcia wrote:
>> On 16/09/2025 15:04, David Lechner wrote:
>>> On 9/15/25 5:16 PM, Marilene Andrade Garcia wrote:
>
> ...
>
>
> In general, please trim out extra stuff like I've done here when you
> reply. It makes it easier to find the important parts. I hope I didn't
> miss any of your questions.
>
>>>> + /*
>>>> + * The following buffers will be bit-reversed during device
>>>> + * communication, because the device transmits and receives data
>>>> + * LSB-first.
>>>> + * DMA (thus cache coherency maintenance) requires the transfer
>>>> + * buffers to live in their own cache lines.
>>>> + */
>>>> + __be16 spi_tx_buffer __aligned(IIO_DMA_MINALIGN);
>>>> + __be16 spi_rx_buffer;
>>>
>>> These would no longer be strictly big-endian, so we could
>>> just make them u16 with a note in the comments.
>>
>> Hello David, I have some doubts that I would like to clarify before sending v12. Since I am not able to test the case using SPI_LSB_FIRST, I noticed that you suggest saving the data as __le in this case. Just out of curiosity, if I use SPI_LSB_FIRST, would saving the data as __be lead to errors?
>
> My thinking is that since we are sending things out 1 byte at a time, in order
> for the least significant bit of 16 bits to be sent first, the least significant
> byte has to be sent first. So will little-endian, the byte containing the least
> significant bit of the 16 bits will be first in memory.
>
> __be is just a naming convention and doesn't actually cause any bytes to
> be swapped in memory. It just lets readers of the code know that the
> value stored there has to be handled carefully because it may not be
> cpu-endian. It would be confusing to readers to store a little-endian
> value in a __be16 variable, but technically, no, it would not cause any
> errors.
>
> This is why I suggested to make it u16. It is still wrong but it is
> equally wrong in both cases. If you still want to use __be16 though,
> you could make a union instead.
>
> union {
> __be16 be;
> __le16 le;
> } spi_tx_buffer;
> union {
> __be16 be;
> __le16 le;
> } spi_rx_buffer;
>
>>>
>>> The scoped_guard() looks a bit odd with the extra indent. I would write
>>> it like this instead:
>>>
>>>
>>>
>>> case IIO_CHAN_INFO_RAW: {
>>> guard(mutex)(&st->lock);
>>>
>>> ret = regmap_read(st->regmap, MAX14001_REG_ADC, val);
>>> if (ret)
>>> return ret;
>>>
>>> return IIO_VAL_INT;
>>> }
>>>
>>
>> Ok, thank you. But since I removed the mutex, as it was mentioned in the first comments, I should not use the guard, right? At least not for now.
>>
>
> Correct. The regmap_read() has something similar internally already.
>
Ok, Thank you for the answers.
Best Regards,
Marilene
On Tue, Sep 16, 2025 at 01:04:41PM -0500, David Lechner wrote:
> On 9/15/25 5:16 PM, Marilene Andrade Garcia wrote:
...
> > Change I was not able to do:
> > - I could not remove bitrev16 because I am using an SPI controller that
> > does not support SPI_LSB_FIRST. So I suggest keeping bitrev16 and not using
> > the spi-lsb-first devicetree property for now, since this driver currently
> > works for both types of controllers: those that support it and those that
> > do not. I left a TODO comment to address this issue as soon as the SPI
> > kernel code starts handling the bit-reverse operation for controllers that
> > do not have this support. Once I finish my work on this driver, if the SPI
> > code still does not include this handling, I can submit patches to add it.
>
> I looked more at what it would take to implement this in the SPI core code
> and found that it would actually be quite difficult to do in a generic way
> because there are so many edge/corner/n-dim cases. We can't change tx_buf
> data in-place because it might be const data that is in some memory area
> that can't be modified. And things would get complicated if different
> transfers pointed to the same buffer memory addresses anyway. So we would
> basically have to allocate new memory for all buffers, copy all tx data to
> that new memory, reverse all of the tx bits, and update all the pointers in
> the transfer structs. Then when the message was finished, we would have to
> reverse all of the rx bits, copy all of the rx buffers back to the original
> buffers and put all the buffer pointers back the way they were. But this
> could write over some of the original tx data if tx_buf and rx_buf point to
> the same original buffer, which would break things if a peripheral driver
> expected the tx data to persist.
And what's the problem here? We do the same with bounce-buffers in case
of DMA/IOMMU (okay, without actual data modification, but it's possible
on-the-fly).
> And we can't do this during the SPI optimize
> step because that currently allows the tx_buf data values to be modified after
> optimization.
This I don't know, so perhaps it's indeed a showstopper.
> So perhaps it is best to just handle it in the peripheral driver. It will
> be much more efficent that way anyway.
>
> However, we still do want to handle SPI_LSB_FIRST now so that people with
> hardware support can be more efficient and we don't want things to break
> if someone puts spi-lsb-first in the devicetree.
...
> > + if (ret < 0)
> > + ret = 1250000;
> > + else
> > + ext_vrefin = 1;
> > + st->vref_mV = ret / (MICRO / MILLI);
>
> Just a style choice here, but in other drivers with similar handling
> we wrote it like this to avoid the extra if statement:
I didn't get this. You move from clear if to not-so-clear ternary. How is
the proposed code better?
> if (ret < 0 && ret != -ENODEV)
> return dev_err_probe(dev, ret, "Failed to get REFIN voltage\n");
>
> ext_vrefin = ret != -ENODEV;
> st->vref_mV = ext_vrefin ? ret / 1000 : 1250;
>
> Keeping (MICRO / MILLI) instead of 1000 is fine too. There are varying opinions
> on this.
> Or we could drop ext_vrefin and have:
It goes back and force. Can we keep the code as it's in this version?
> if (ret < 0 && ret != -ENODEV)
> return dev_err_probe(dev, ret, "Failed to get REFIN voltage\n");
>
> if (ret != -ENODEV) {
> st->vref_mV = ret / 1000;
>
> /* regmap set bits goes here. */
> ...
> } else {
> st->vref_mV = 1250;
> }
...
> > + return dev_err_probe(dev, ret, "Failed to set External REFIN in Configuration Register\n");
> These lines are getting very long. We try to wrap to 80 characters
> as much as we can in the IIO subsystem.
Side note: checkpatch.pl almost never complained (okay, something like 15y+
ago) on long string literals at the end of statements. For the code lines
I fully support the wrapping.
--
With Best Regards,
Andy Shevchenko
On 9/17/25 3:10 AM, Andy Shevchenko wrote:
> On Tue, Sep 16, 2025 at 01:04:41PM -0500, David Lechner wrote:
>> On 9/15/25 5:16 PM, Marilene Andrade Garcia wrote:
>
...
>>> + if (ret < 0)
>>> + ret = 1250000;
>>> + else
>>> + ext_vrefin = 1;
>>> + st->vref_mV = ret / (MICRO / MILLI);
>>
>> Just a style choice here, but in other drivers with similar handling
>> we wrote it like this to avoid the extra if statement:
>
> I didn't get this. You move from clear if to not-so-clear ternary. How is
> the proposed code better?
I can't say one is better than the other. What I suggested is just
how we've done it other similar other drivers.
>
>> if (ret < 0 && ret != -ENODEV)
>> return dev_err_probe(dev, ret, "Failed to get REFIN voltage\n");
>>
>> ext_vrefin = ret != -ENODEV;
>> st->vref_mV = ext_vrefin ? ret / 1000 : 1250;
>>
>> Keeping (MICRO / MILLI) instead of 1000 is fine too. There are varying opinions
>> on this.
>
>> Or we could drop ext_vrefin and have:
>
> It goes back and force. Can we keep the code as it's in this version?
Sure. Existing code is good enough for me. (And in that case, I
agree that renaming to `use_ext_vrefin` is an improvement.)
>
>> if (ret < 0 && ret != -ENODEV)
>> return dev_err_probe(dev, ret, "Failed to get REFIN voltage\n");
>>
>> if (ret != -ENODEV) {
>> st->vref_mV = ret / 1000;
>>
>> /* regmap set bits goes here. */
>> ...
>> } else {
>> st->vref_mV = 1250;
>> }
>
On Wed, Sep 17, 2025 at 11:10:42AM +0300, Andy Shevchenko wrote: > On Tue, Sep 16, 2025 at 01:04:41PM -0500, David Lechner wrote: > > On 9/15/25 5:16 PM, Marilene Andrade Garcia wrote: ... > > > Change I was not able to do: > > > - I could not remove bitrev16 because I am using an SPI controller that > > > does not support SPI_LSB_FIRST. So I suggest keeping bitrev16 and not using > > > the spi-lsb-first devicetree property for now, since this driver currently > > > works for both types of controllers: those that support it and those that > > > do not. I left a TODO comment to address this issue as soon as the SPI > > > kernel code starts handling the bit-reverse operation for controllers that > > > do not have this support. Once I finish my work on this driver, if the SPI > > > code still does not include this handling, I can submit patches to add it. > > > > I looked more at what it would take to implement this in the SPI core code > > and found that it would actually be quite difficult to do in a generic way > > because there are so many edge/corner/n-dim cases. We can't change tx_buf > > data in-place because it might be const data that is in some memory area > > that can't be modified. And things would get complicated if different > > transfers pointed to the same buffer memory addresses anyway. So we would > > basically have to allocate new memory for all buffers, copy all tx data to > > that new memory, reverse all of the tx bits, and update all the pointers in > > the transfer structs. Then when the message was finished, we would have to > > reverse all of the rx bits, copy all of the rx buffers back to the original > > buffers and put all the buffer pointers back the way they were. But this > > could write over some of the original tx data if tx_buf and rx_buf point to > > the same original buffer, which would break things if a peripheral driver > > expected the tx data to persist. > > And what's the problem here? We do the same with bounce-buffers in case > of DMA/IOMMU (okay, without actual data modification, but it's possible > on-the-fly). Actually, can this be done on a regmap level instead? We have a lot of custom regmap IO accessors, bulk accessor that does something to a data can be also implemented. > > And we can't do this during the SPI optimize > > step because that currently allows the tx_buf data values to be modified after > > optimization. > > This I don't know, so perhaps it's indeed a showstopper. > > > So perhaps it is best to just handle it in the peripheral driver. It will > > be much more efficent that way anyway. > > > > However, we still do want to handle SPI_LSB_FIRST now so that people with > > hardware support can be more efficient and we don't want things to break > > if someone puts spi-lsb-first in the devicetree. -- With Best Regards, Andy Shevchenko
On 9/17/25 3:12 AM, Andy Shevchenko wrote: > On Wed, Sep 17, 2025 at 11:10:42AM +0300, Andy Shevchenko wrote: >> On Tue, Sep 16, 2025 at 01:04:41PM -0500, David Lechner wrote: >>> On 9/15/25 5:16 PM, Marilene Andrade Garcia wrote: > > ... > >>>> Change I was not able to do: >>>> - I could not remove bitrev16 because I am using an SPI controller that >>>> does not support SPI_LSB_FIRST. So I suggest keeping bitrev16 and not using >>>> the spi-lsb-first devicetree property for now, since this driver currently >>>> works for both types of controllers: those that support it and those that >>>> do not. I left a TODO comment to address this issue as soon as the SPI >>>> kernel code starts handling the bit-reverse operation for controllers that >>>> do not have this support. Once I finish my work on this driver, if the SPI >>>> code still does not include this handling, I can submit patches to add it. >>> >>> I looked more at what it would take to implement this in the SPI core code >>> and found that it would actually be quite difficult to do in a generic way >>> because there are so many edge/corner/n-dim cases. We can't change tx_buf >>> data in-place because it might be const data that is in some memory area >>> that can't be modified. And things would get complicated if different >>> transfers pointed to the same buffer memory addresses anyway. So we would >>> basically have to allocate new memory for all buffers, copy all tx data to >>> that new memory, reverse all of the tx bits, and update all the pointers in >>> the transfer structs. Then when the message was finished, we would have to >>> reverse all of the rx bits, copy all of the rx buffers back to the original >>> buffers and put all the buffer pointers back the way they were. But this >>> could write over some of the original tx data if tx_buf and rx_buf point to >>> the same original buffer, which would break things if a peripheral driver >>> expected the tx data to persist. >> >> And what's the problem here? We do the same with bounce-buffers in case >> of DMA/IOMMU (okay, without actual data modification, but it's possible >> on-the-fly). OK, maybe not as much problem as I thought. Just rather inefficient. I might have another look. We could perhaps allocate the buffers during the spi_optimize phase and only swap bits on each transfer to make it as efficient as possible. > > Actually, can this be done on a regmap level instead? We have a lot of custom > regmap IO accessors, bulk accessor that does something to a data can be also > implemented. > Currently, if you have a peripheral that has the SPI_LSB_FIRST flag connected to a controller that does not have that flag, the SPI core code will refuse to make a SPI device for the peripheral. So to make anything work at all, the core SPI code is going to have to be made aware one way or another. >>> And we can't do this during the SPI optimize >>> step because that currently allows the tx_buf data values to be modified after >>> optimization. >> >> This I don't know, so perhaps it's indeed a showstopper. >> >>> So perhaps it is best to just handle it in the peripheral driver. It will >>> be much more efficent that way anyway. >>> >>> However, we still do want to handle SPI_LSB_FIRST now so that people with >>> hardware support can be more efficient and we don't want things to break >>> if someone puts spi-lsb-first in the devicetree. > >
On 9/16/25 1:04 PM, David Lechner wrote:
> On 9/15/25 5:16 PM, Marilene Andrade Garcia wrote:
...
>> +static const struct iio_chan_spec max14001_channel[] = {
>> + {
>> + .type = IIO_VOLTAGE,
>> + .indexed = 1,
>> + .channel = 0,
>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>> + BIT(IIO_CHAN_INFO_AVERAGE_RAW) |
>
> I think there was some discussion about this last time [1] that it
> would actually make sense to have the average as a separate channel.
>
> There are a few reasons for this. If we add buffered read support
> later, we could only read the average in a buffered read if it is
> a separate channel. And the average channel should have an extra
> control that is either the -3 dB low pass filter point (using existing
> ABI) or the averaging window size (potentially new ABI).
>
> If we don't care about ever reading the filtered and not filtered
> value at the same time, we could just have a single channel and only
> ever read from the FADC register and ignore the ADC register.
>
> In either case, we would not need IIO_CHAN_INFO_AVERAGE_RAW.
>
> [1]: https://lore.kernel.org/linux-iio/20250903171105.00003dcd@huawei.com/
>
I thought about this some more while I was eating lunch and
I think I like the second option the best. We keep things simple
with just one channel for now. And if anyone ever does need to
read both filtered and unfiltered at the same time, we can add
it, but if that never happens, then we won't have wasted time
implementing it.
On Tue, Sep 16, 2025 at 1:16 AM Marilene Andrade Garcia
<marilene.agarcia@gmail.com> wrote:
>
> The MAX14001/MAX14002 is configurable, isolated 10-bit ADCs for multi-range
> binary inputs. In addition to ADC readings, the MAX14001/MAX14002 offers
> more features, like a binary comparator, a filtered reading that can
> provide the average of the last 2, 4, or 8 ADC readings, and an inrush
> comparator that triggers the inrush current. There is also a fault feature
> that can diagnose seven possible fault conditions. And an option to select
> an external or internal ADC voltage reference.
>
> MAX14001/MAX14002 features implemented so far:
> - Raw ADC reading.
> - Filtered ADC average reading with the default configuration.
> - MV fault disable.
> - Selection of external or internal ADC voltage reference, depending on
> whether it is declared in the device tree.
This version looks almost good to me, a few nit-picks below.
...
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -87,6 +87,7 @@ obj-$(CONFIG_MAX11100) += max11100.o
> obj-$(CONFIG_MAX1118) += max1118.o
> obj-$(CONFIG_MAX11205) += max11205.o
> obj-$(CONFIG_MAX11410) += max11410.o
> +obj-$(CONFIG_MAX14001) += max14001.o
> obj-$(CONFIG_MAX1241) += max1241.o
> obj-$(CONFIG_MAX1363) += max1363.o
> obj-$(CONFIG_MAX34408) += max34408.o
Please, keep it ordered.
...
> +#include <linux/array_size.h>
> +#include <linux/bitfield.h>
> +#include <linux/bitrev.h>
> +#include <linux/bits.h>
> +#include <linux/byteorder/generic.h>
This is wrong, should be asm/byteorder.h going after linux/* but
before linux/iio/* ones...
> +#include <linux/cleanup.h>
> +#include <linux/device.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +#include <linux/types.h>
> +#include <linux/units.h>
...here
asm/byteorder.h
> +#include <linux/iio/iio.h>
> +#include <linux/iio/types.h>
...
> +struct max14001_state {
> + const struct max14001_chip_info *chip_info;
> + struct spi_device *spi;
> + struct regmap *regmap;
> + int vref_mV;
> + /*
> + * lock protect against multiple concurrent accesses, RMW sequence,
Lock to protect...
> + * and SPI transfer.
> + */
> + struct mutex lock;
> + /*
> + * The following buffers will be bit-reversed during device
> + * communication, because the device transmits and receives data
> + * LSB-first.
> + * DMA (thus cache coherency maintenance) requires the transfer
> + * buffers to live in their own cache lines.
> + */
> + __be16 spi_tx_buffer __aligned(IIO_DMA_MINALIGN);
> + __be16 spi_rx_buffer;
> +};
...
> +static struct max14001_chip_info max14001_chip_info = {
> + .name = "max14001",
> +};
> +
> +static struct max14001_chip_info max14002_chip_info = {
> + .name = "max14002",
> +};
These can be moved closer to their first user (ID table?).
...
> +static int max14001_write(struct max14001_state *st, unsigned int reg, unsigned int val)
> +{
> + /*
> + * Prepare SPI transmit buffer 16 bit-value big-endian format and
> + * reverses bit order to align with the LSB-first input on SDI port
reverse
> + * in order to meet the device communication requirements.
> + */
> + st->spi_tx_buffer = cpu_to_be16(bitrev16(FIELD_PREP(MAX14001_MASK_ADDR, reg) |
> + FIELD_PREP(MAX14001_MASK_WR, 1) |
> + FIELD_PREP(MAX14001_MASK_DATA, val)));
> +
> + return spi_write(st->spi, &st->spi_tx_buffer, sizeof(st->spi_tx_buffer));
> +}
...
> +static int max14001_probe(struct spi_device *spi)
> +{
> + struct device *dev = &spi->dev;
> + struct iio_dev *indio_dev;
> + struct max14001_state *st;
> + int ret, ext_vrefin = 0;
bool use_ext_vrefin = false;
> + indio_dev = devm_iio_device_alloc(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(dev, -ENODEV, "Failed to get match data\n");
Remove this check, it will be almost always a dead code. The developer
implementing a new device support won't be able to test the driver
anyway with properly given chip_info.
> + indio_dev->name = st->chip_info->name;
> + indio_dev->info = &max14001_info;
> + indio_dev->channels = max14001_channel;
> + indio_dev->num_channels = ARRAY_SIZE(max14001_channel);
> + indio_dev->modes = INDIO_DIRECT_MODE;
> +
> + st->regmap = devm_regmap_init(dev, NULL, st, &max14001_regmap_config);
> + if (IS_ERR(st->regmap))
> + return dev_err_probe(dev, PTR_ERR(st->regmap), "Failed to initialize regmap\n");
> +
> + ret = devm_mutex_init(dev, &st->lock);
> + if (ret)
> + return ret;
> +
> + ret = devm_regulator_get_enable(dev, "vdd");
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to enable Vdd supply\n");
> +
> + ret = devm_regulator_get_enable(dev, "vddl");
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to enable Vddl supply\n");
> +
> + ret = devm_regulator_get_enable_read_voltage(dev, "refin");
> + if (ret < 0 && ret != -ENODEV)
> + return dev_err_probe(dev, ret, "Failed to get REFIN voltage\n");
> +
> + if (ret < 0)
if (ret == -ENODEV) ?
It would be interesting out of curiosity to see bloat-o-meter output
for the original check and my proposal. I would choose one which gives
less code, but in case of equality, I would rather go with a more
explicit (my proposal) check.
> + ret = 1250000;
> + else
> + ext_vrefin = 1;
use_ext_vrefin = true;
> + st->vref_mV = ret / (MICRO / MILLI);
> +
> + if (ext_vrefin) {
if (use_ext_vrefin) {
> + /*
> + * Configure the MAX14001/MAX14002 to use an external voltage reference source
> + * by setting the bit 5 of the configuration register
Missing period.
> + */
> + ret = regmap_update_bits(st->regmap, MAX14001_REG_CFG, MAX14001_REG_CFG_BIT_EXRF, MAX14001_REG_CFG_BIT_EXRF);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to set External REFIN in Configuration Register\n");
> + }
> +
> + ret = max14001_disable_mv_fault(st);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to disable MV Fault\n");
> +
> + return devm_iio_device_register(dev, indio_dev);
> +}
--
With Best Regards,
Andy Shevchenko
© 2016 - 2025 Red Hat, Inc.