* [PATCH v10 0/2] Add MAX14001/MAX14002 support
@ 2025-09-02 13:14 Marilene Andrade Garcia
2025-09-02 13:15 ` [PATCH v10 1/2] dt-bindings: iio: adc: add max14001 Marilene Andrade Garcia
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Marilene Andrade Garcia @ 2025-09-02 13:14 UTC (permalink / raw)
To: linux-iio, linux-kernel, devicetree
Cc: Marilene Andrade Garcia, Kim Seer Paller, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Lars-Peter Clausen,
Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Marcelo Schmitt, Marcelo Schmitt, Ceclan Dumitru, Jonathan Santos,
Dragos Bogdan
Hello everyone,
Thank you for your input on how to handle the situation with the driver code.
Kim, I also apologize for the unexpected situation involving your previous
code.
Based on the suggestions, I applied my v1 code changes to v9 of Kim’s code,
resulting in this v10 version that combines both.
Compared to v9, the updates are:
- Added support for max14002.
- Added a function to write a single register, since the write enable
register must be updated before writing to any others and updated again
afterward.
- Renamed the init function to better reflect its purpose, which is to
disable the memory verification fault. I also replaced the one-by-one
handling of registers verification values with a loop, since they are in
sequential ascending order.
- Replaced the old regulator APIs with the new ones.
- Updated the device tree documentation to align with the datasheet
nomenclature for voltage suppliers.
- Used IIO_CHAN_INFO_AVERAGE_RAW to return the filtered average of ADC
readings.
One of the reviews I received about my v1 version suggested using a custom
regmap. I attempted to implement that, but I feel that most of the default
regmap functions (e.g., regmap_update_bits) would need to be overridden
because of the unique way this device handles communication, such as
inverting bits before sending a message, updating the write enable register
before writing any other register, and updating it again afterward. However,
as I am still new to the IIO kernel code, I may be missing something. If you
could provide further explanation or an example, I would be grateful.
Regarding locking, Kim’s original code implemented it, and it remains in
the driver.
I still have a question about using _mean_raw (IIO_CHAN_INFO_AVERAGE_RAW)
to read the register containing the latest filtered average ADC readings.
Should I create a v11 version with a patch to include in_voltageY_mean_raw
in the file /linux/Documentation/ABI/testing/sysfs-bus-iio?
The idea is to use in_voltageY_mean_raw to return the filtered average and
also to set how many ADC readings (0, 2, 4, or 8) are included in the mean
calculation. Any feedback on using IIO_CHAN_INFO_AVERAGE_RAW this way would
be appreciated.
The v10 changes were tested on a Raspberry Pi 5 using a modified kernel
(rpi-6.12). The MAX14001PMB evaluation board, which contains two MAX14001
devices, was used for testing. One device measures current, and the other
measures voltage. The evaluation board introduces an offset to allow
measuring negative values. These board-specific characteristics were not
included in the driver code (neither the offset nor the current channel
capability), but they were considered in the calculation of the values read
by the devices. Should the code that applies these board configuration
parameters be added as an additional driver file inside the IIO subsystem,
or should it remain only in a user application?
I plan to continue sending patches to cover all the features of the device.
This includes adding interrupt handling for faults and for when the signal
exceeds the upper or lower threshold, implementing the inrush current
feature, and completing the filtered average reading functionality by
adding the ability to set the number of readings used in the mean
calculation.
And I would like to thank again my GSoC mentors Marcelo Schmitt, Ceclan
Dumitru, Jonathan Santos and Dragos Bogdan for their help with the code.
Thank you for your time,
Best regards,
Marilene Andrade Garcia.
Marilene Andrade Garcia (2):
dt-bindings: iio: adc: add max14001
iio: adc: max14001: New driver
.../bindings/iio/adc/adi,max14001.yaml | 79 ++++
MAINTAINERS | 9 +
drivers/iio/adc/Kconfig | 10 +
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/max14001.c | 355 ++++++++++++++++++
5 files changed, 454 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,max14001.yaml
create mode 100644 drivers/iio/adc/max14001.c
base-commit: d1487b0b78720b86ec2a2ac7acc683ec90627e5b
--
2.34.1
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v10 1/2] dt-bindings: iio: adc: add max14001
2025-09-02 13:14 [PATCH v10 0/2] Add MAX14001/MAX14002 support Marilene Andrade Garcia
@ 2025-09-02 13:15 ` Marilene Andrade Garcia
2025-09-02 14:29 ` David Lechner
2025-09-02 13:16 ` [PATCH v10 2/2] iio: adc: max14001: New driver Marilene Andrade Garcia
2025-09-02 16:10 ` [PATCH v10 0/2] Add MAX14001/MAX14002 support David Lechner
2 siblings, 1 reply; 13+ messages in thread
From: Marilene Andrade Garcia @ 2025-09-02 13:15 UTC (permalink / raw)
To: linux-iio, linux-kernel, devicetree
Cc: Marilene Andrade Garcia, Kim Seer Paller, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Lars-Peter Clausen,
Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Marcelo Schmitt, Marcelo Schmitt, Ceclan Dumitru, Jonathan Santos,
Dragos Bogdan
Add device-tree documentation for MAX14001/MAX14002 ADCs.
The MAX14001/MAX14002 are isolated, single-channel analog-to-digital
converters with programmable voltage comparators and inrush current
control optimized for configurable binary input applications.
Co-developed-by: Marilene Andrade Garcia <marilene.agarcia@gmail.com>
Signed-off-by: Marilene Andrade Garcia <marilene.agarcia@gmail.com>
Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
---
.../bindings/iio/adc/adi,max14001.yaml | 79 +++++++++++++++++++
MAINTAINERS | 8 ++
2 files changed, 87 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,max14001.yaml
diff --git a/Documentation/devicetree/bindings/iio/adc/adi,max14001.yaml b/Documentation/devicetree/bindings/iio/adc/adi,max14001.yaml
new file mode 100644
index 000000000000..ff9a41f04300
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/adi,max14001.yaml
@@ -0,0 +1,79 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# Copyright 2023-2025 Analog Devices Inc.
+# Copyright 2023 Kim Seer Paller
+# Copyright 2025 Marilene Andrade Garcia
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/adi,max14001.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices MAX14001-MAX14002 ADC
+
+maintainers:
+ - Kim Seer Paller <kimseer.paller@analog.com>
+ - Marilene Andrade Garcia <marilene.agarcia@gmail.com>
+
+description: |
+ Single channel 10 bit ADC with SPI interface.
+ Datasheet can be found here
+ https://www.analog.com/media/en/technical-documentation/data-sheets/MAX14001-MAX14002.pdf
+
+$ref: /schemas/spi/spi-peripheral-props.yaml#
+
+properties:
+ compatible:
+ enum:
+ - adi,max14001
+ - adi,max14002
+
+ reg:
+ maxItems: 1
+
+ spi-max-frequency:
+ maximum: 5000000
+
+ vdd-supply:
+ description:
+ Isolated DC-DC power supply input voltage.
+
+ vddl-supply:
+ description:
+ Logic power supply.
+
+ vrefin-supply:
+ description:
+ ADC voltage reference supply.
+
+ interrupts:
+ items:
+ - description: |
+ Interrupt for signaling when conversion results exceed the configured
+ upper threshold for ADC readings or fall below the lower threshold for
+ them. Interrupt source must be attached to COUT pin.
+ - description: |
+ Alert output that asserts low during a number of different error
+ conditions. The interrupt source must be attached to FAULT pin.
+
+required:
+ - compatible
+ - reg
+ - vdd-supply
+ - vddl-supply
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ spi {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ max14001: adc@0 {
+ compatible = "adi,max14001";
+ reg = <0>;
+ spi-max-frequency = <5000000>;
+ vdd-supply = <&vdd>;
+ vddl-supply = <&vddl>;
+ };
+ };
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index af1c8d2bfb3d..f145f0204407 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14969,6 +14969,14 @@ S: Maintained
F: Documentation/devicetree/bindings/sound/max9860.txt
F: sound/soc/codecs/max9860.*
+MAX14001/MAX14002 IIO ADC DRIVER
+M: Kim Seer Paller <kimseer.paller@analog.com>
+M: Marilene Andrade Garcia <marilene.agarcia@gmail.com>
+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
+
MAXBOTIX ULTRASONIC RANGER IIO DRIVER
M: Andreas Klinger <ak@it-klinger.de>
L: linux-iio@vger.kernel.org
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v10 2/2] iio: adc: max14001: New driver
2025-09-02 13:14 [PATCH v10 0/2] Add MAX14001/MAX14002 support Marilene Andrade Garcia
2025-09-02 13:15 ` [PATCH v10 1/2] dt-bindings: iio: adc: add max14001 Marilene Andrade Garcia
@ 2025-09-02 13:16 ` Marilene Andrade Garcia
2025-09-02 13:45 ` Andy Shevchenko
` (2 more replies)
2025-09-02 16:10 ` [PATCH v10 0/2] Add MAX14001/MAX14002 support David Lechner
2 siblings, 3 replies; 13+ messages in thread
From: Marilene Andrade Garcia @ 2025-09-02 13:16 UTC (permalink / raw)
To: linux-iio, linux-kernel, devicetree
Cc: Marilene Andrade Garcia, Kim Seer Paller, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Lars-Peter Clausen,
Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Marcelo Schmitt, Marcelo Schmitt, Ceclan Dumitru, Jonathan Santos,
Dragos Bogdan
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: Marilene Andrade Garcia <marilene.agarcia@gmail.com>
Signed-off-by: Marilene Andrade Garcia <marilene.agarcia@gmail.com>
Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
---
MAINTAINERS | 1 +
drivers/iio/adc/Kconfig | 10 ++
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/max14001.c | 355 +++++++++++++++++++++++++++++++++++++
4 files changed, 367 insertions(+)
create mode 100644 drivers/iio/adc/max14001.c
diff --git a/MAINTAINERS b/MAINTAINERS
index f145f0204407..b6457063da6c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14976,6 +14976,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 e3d3826c3357..11e911ceab4c 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -958,6 +958,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 89d72bf9ce70..569f2f5613d4 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -85,6 +85,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..6755df152976
--- /dev/null
+++ b/drivers/iio/adc/max14001.c
@@ -0,0 +1,355 @@
+// 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
+ */
+
+#include <linux/array_size.h>
+#include <linux/bitfield.h>
+#include <linux/bitrev.h>
+#include <linux/bits.h>
+#include <linux/byteorder/generic.h>
+#include <linux/device.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/types.h>
+#include <linux/kernel.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
+#include <linux/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_EXRF BIT(5)
+
+#define MAX14001_MASK_ADDR GENMASK(15, 11)
+#define MAX14001_MASK_DATA GENMASK(9, 0)
+
+#define MAX14001_SET_WRITE_BIT BIT(10)
+#define MAX14001_WRITE_WEN 0x294
+
+enum max14001_chip_model {
+ max14001,
+ max14002,
+};
+
+struct max14001_chip_info {
+ const char *name;
+};
+
+struct max14001_state {
+ const struct max14001_chip_info *chip_info;
+ struct spi_device *spi;
+ 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;
+};
+
+static struct max14001_chip_info max14001_chip_info_tbl[] = {
+ [max14001] = {
+ .name = "max14001",
+ },
+ [max14002] = {
+ .name = "max14002",
+ },
+};
+
+static int max14001_read(struct max14001_state *st, u16 reg_addr, u16 *reg_data)
+{
+ 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 = FIELD_PREP(MAX14001_MASK_ADDR, reg_addr);
+ st->spi_tx_buffer = bitrev16(cpu_to_be16(st->spi_tx_buffer));
+
+ 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.
+ */
+ st->spi_rx_buffer = bitrev16(be16_to_cpu(st->spi_rx_buffer));
+ *reg_data = FIELD_GET(MAX14001_MASK_DATA, st->spi_rx_buffer);
+
+ return 0;
+}
+
+static int max14001_write(struct max14001_state *st, u16 reg_addr, u16 reg_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 = FIELD_PREP(MAX14001_MASK_ADDR, reg_addr) |
+ FIELD_PREP(MAX14001_SET_WRITE_BIT, 1) |
+ FIELD_PREP(MAX14001_MASK_DATA, reg_data);
+ st->spi_tx_buffer = bitrev16(cpu_to_be16(st->spi_tx_buffer));
+
+ return spi_write(st->spi, &st->spi_tx_buffer, sizeof(st->spi_tx_buffer));
+}
+
+static int max14001_write_single_reg(struct max14001_state *st, u16 reg_addr,
+ u16 reg_data)
+{
+ int ret;
+
+ /* Enable register write */
+ ret = max14001_write(st, MAX14001_REG_WEN, MAX14001_WRITE_WEN);
+ if (ret)
+ return ret;
+
+ /* Write data into register */
+ ret = max14001_write(st, reg_addr, reg_data);
+ if (ret)
+ return ret;
+
+ /* Disable register write */
+ ret = max14001_write(st, MAX14001_REG_WEN, 0);
+ if (ret)
+ return ret;
+
+ return ret;
+}
+
+static int max14001_write_verification_reg(struct max14001_state *st,
+ u16 reg_addr)
+{
+ u16 reg_data;
+ int ret;
+
+ ret = max14001_read(st, reg_addr, ®_data);
+ if (ret)
+ return ret;
+
+ return max14001_write(st, MAX14001_REG_VERIFICATION(reg_addr), reg_data);
+}
+
+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);
+ u16 reg_data;
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ mutex_lock(&st->lock);
+ ret = max14001_read(st, MAX14001_REG_ADC, ®_data);
+ mutex_unlock(&st->lock);
+ if (ret)
+ return ret;
+
+ *val = reg_data;
+
+ return IIO_VAL_INT;
+ case IIO_CHAN_INFO_AVERAGE_RAW:
+ mutex_lock(&st->lock);
+ ret = max14001_read(st, MAX14001_REG_FADC, ®_data);
+ mutex_unlock(&st->lock);
+ if (ret)
+ return ret;
+
+ *val = reg_data;
+
+ 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 iio_info max14001_info = {
+ .read_raw = max14001_read_raw,
+};
+
+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_disable_mv_fault(struct max14001_state *st)
+{
+ u16 reg_addr;
+ int ret;
+
+ /* Enable SPI Registers Write */
+ ret = max14001_write(st, MAX14001_REG_WEN, MAX14001_WRITE_WEN);
+ if (ret)
+ return ret;
+
+ /*
+ * Reads all registers and writes the values back to their appropriate
+ * verification registers to clear the Memory Validation fault.
+ */
+ for (reg_addr = MAX14001_REG_FLTEN; reg_addr <= MAX14001_REG_ENBL; reg_addr++) {
+ ret = max14001_write_verification_reg(st, reg_addr);
+ if (ret)
+ return ret;
+ }
+
+ /* Disable SPI Registers Write */
+ return max14001_write(st, MAX14001_REG_WEN, 0);
+}
+
+static int max14001_probe(struct spi_device *spi)
+{
+ struct iio_dev *indio_dev;
+ struct max14001_state *st;
+ struct device *dev = &spi->dev;
+ int ret, ext_vrefin = 0;
+ u16 reg_data;
+
+ 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;
+
+ ret = devm_regulator_get_enable(dev, "vdd");
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "Failed to enable specified Vdd supply\n");
+
+ ret = devm_regulator_get_enable(dev, "vddl");
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "Failed to enable specified Vddl supply\n");
+
+ ret = devm_regulator_get_enable_read_voltage(dev, "vrefin");
+ if (ret < 0) {
+ st->vref_mv = 1250000 / 1000;
+ } else {
+ st->vref_mv = ret / 1000;
+ ext_vrefin = 1;
+ }
+
+ ret = devm_mutex_init(dev, &st->lock);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "Failed to init the mutex\n");
+
+ if (ext_vrefin) {
+ /*
+ * Configure the MAX14001/MAX14002 to use an external voltage
+ * reference source for the ADC.
+ */
+ ret = max14001_read(st, MAX14001_REG_CFG, ®_data);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "Failed to read Configuration Register\n");
+
+ reg_data |= FIELD_PREP(MAX14001_REG_CFG_EXRF, 1);
+ ret = max14001_write_single_reg(st, MAX14001_REG_CFG, reg_data);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "Failed to set 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_tbl[max14001] },
+ { "max14002", (kernel_ulong_t)&max14001_chip_info_tbl[max14002] },
+ { }
+};
+
+static const struct of_device_id max14001_of_match[] = {
+ { .compatible = "adi,max14001",
+ .data = &max14001_chip_info_tbl[max14001], },
+ { .compatible = "adi,max14002",
+ .data = &max14001_chip_info_tbl[max14002], },
+ { }
+};
+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.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v10 2/2] iio: adc: max14001: New driver
2025-09-02 13:16 ` [PATCH v10 2/2] iio: adc: max14001: New driver Marilene Andrade Garcia
@ 2025-09-02 13:45 ` Andy Shevchenko
2025-09-02 14:12 ` David Lechner
2025-09-02 15:44 ` David Lechner
2025-09-03 22:28 ` kernel test robot
2 siblings, 1 reply; 13+ messages in thread
From: Andy Shevchenko @ 2025-09-02 13:45 UTC (permalink / raw)
To: Marilene Andrade Garcia
Cc: linux-iio, linux-kernel, devicetree, Kim Seer Paller,
Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
Lars-Peter Clausen, Michael Hennerich, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Marcelo Schmitt,
Marcelo Schmitt, Ceclan Dumitru, Jonathan Santos, Dragos Bogdan
On Tue, Sep 02, 2025 at 10:16:12AM -0300, 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.
...
> +#include <linux/array_size.h>
> +#include <linux/bitfield.h>
> +#include <linux/bitrev.h>
> +#include <linux/bits.h>
> +#include <linux/byteorder/generic.h>
> +#include <linux/device.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/types.h>
Please, move this group...
> +#include <linux/kernel.h>
This header must not be used in a driver like this.
Please, replace it with the headers that you are actually used.
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +#include <linux/types.h>
...to be here (and add a blank line above).
...
> +#define MAX14001_REG_VERIFICATION(x) ((x) + 0x10)
> +
> +#define MAX14001_REG_CFG_EXRF BIT(5)
Is it REG? I'm lost in the naming(s) and value(s) in this driver.
...
> +#define MAX14001_WRITE_WEN 0x294
Also, what's this? Shouldn't it be a (WRITE_EN | 0x94) ?
...
> +enum max14001_chip_model {
> + max14001,
> + max14002,
> +};
No need, just make the data structures separate.
...
> +struct max14001_state {
> + const struct max14001_chip_info *chip_info;
> + struct spi_device *spi;
> + int vref_mv;
Can it be _mV? This will follow the real unit spelling.
(And yes, we have such suffixes in the variable names ion the kernel.)
> + /*
> + * lock protect against multiple concurrent accesses, RMW sequence,
> + * and SPI transfer.
> + */
> + struct mutex lock;
+ mutex.h
> + /*
> + * 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 int max14001_read(struct max14001_state *st, u16 reg_addr, u16 *reg_data)
> +{
> + 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 = FIELD_PREP(MAX14001_MASK_ADDR, reg_addr);
> + st->spi_tx_buffer = bitrev16(cpu_to_be16(st->spi_tx_buffer));
Use temporary variable. There are two issues with the above:
1) the usual pattern is to avoid putting data to the external buffers /
variables until it's ready (this will be more robust against potential
synchronisation issues);
2) it's simply hard to read and follow; also it's prone to mistakes if
something more comes in the future among these lines.
> + 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.
> + */
> + st->spi_rx_buffer = bitrev16(be16_to_cpu(st->spi_rx_buffer));
> + *reg_data = FIELD_GET(MAX14001_MASK_DATA, st->spi_rx_buffer);
> +
> + return 0;
> +}
...
> +static int max14001_write(struct max14001_state *st, u16 reg_addr, u16 reg_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 = FIELD_PREP(MAX14001_MASK_ADDR, reg_addr) |
> + FIELD_PREP(MAX14001_SET_WRITE_BIT, 1) |
> + FIELD_PREP(MAX14001_MASK_DATA, reg_data);
> + st->spi_tx_buffer = bitrev16(cpu_to_be16(st->spi_tx_buffer));
Ditto.
> + return spi_write(st->spi, &st->spi_tx_buffer, sizeof(st->spi_tx_buffer));
> +}
...
> +static int max14001_write_single_reg(struct max14001_state *st, u16 reg_addr,
> + u16 reg_data)
> +{
> + int ret;
> +
> + /* Enable register write */
> + ret = max14001_write(st, MAX14001_REG_WEN, MAX14001_WRITE_WEN);
> + if (ret)
> + return ret;
> +
> + /* Write data into register */
> + ret = max14001_write(st, reg_addr, reg_data);
> + if (ret)
> + return ret;
> + /* Disable register write */
> + ret = max14001_write(st, MAX14001_REG_WEN, 0);
> + if (ret)
> + return ret;
> +
> + return ret;
Are you expecting anything than 0 here? Why?
Also the whole block is simply
return max14001_write(st, MAX14001_REG_WEN, 0);
> +}
...
> +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);
> + u16 reg_data;
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
Use scoped_guard() from cleanup.h.
> + mutex_lock(&st->lock);
> + ret = max14001_read(st, MAX14001_REG_ADC, ®_data);
> + mutex_unlock(&st->lock);
> + if (ret)
> + return ret;
> +
> + *val = reg_data;
> +
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_AVERAGE_RAW:
> + mutex_lock(&st->lock);
> + ret = max14001_read(st, MAX14001_REG_FADC, ®_data);
> + mutex_unlock(&st->lock);
Ditto.
> + if (ret)
> + return ret;
> +
> + *val = reg_data;
> +
> + 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 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),
> + }
Leave a trailing comma. It's not a terminator entry.
> +};
...
> +static int max14001_disable_mv_fault(struct max14001_state *st)
> +{
> + u16 reg_addr;
It's enough to call it reg.
> + int ret;
> +
> + /* Enable SPI Registers Write */
> + ret = max14001_write(st, MAX14001_REG_WEN, MAX14001_WRITE_WEN);
> + if (ret)
> + return ret;
> +
> + /*
> + * Reads all registers and writes the values back to their appropriate
> + * verification registers to clear the Memory Validation fault.
> + */
> + for (reg_addr = MAX14001_REG_FLTEN; reg_addr <= MAX14001_REG_ENBL; reg_addr++) {
> + ret = max14001_write_verification_reg(st, reg_addr);
> + if (ret)
> + return ret;
> + }
> +
> + /* Disable SPI Registers Write */
> + return max14001_write(st, MAX14001_REG_WEN, 0);
> +}
...
> +static int max14001_probe(struct spi_device *spi)
> +{
> + struct iio_dev *indio_dev;
> + struct max14001_state *st;
> + struct device *dev = &spi->dev;
> + int ret, ext_vrefin = 0;
> + u16 reg_data;
Call it 'value' ('reg' part is redundant, and 'data' may be ambiguous in this function).
> + 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;
> +
> + ret = devm_regulator_get_enable(dev, "vdd");
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "Failed to enable specified Vdd supply\n");
> +
> + ret = devm_regulator_get_enable(dev, "vddl");
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "Failed to enable specified Vddl supply\n");
> +
> + ret = devm_regulator_get_enable_read_voltage(dev, "vrefin");
> + if (ret < 0) {
> + st->vref_mv = 1250000 / 1000;
(MICRO / MILLI)
> + } else {
> + st->vref_mv = ret / 1000;
Ditto.
> + ext_vrefin = 1;
> + }
And with deduplication refactored code:
ret = devm_regulator_get_enable_read_voltage(dev, "vrefin");
if (ret < 0)
ret = 1250000;
else
ext_vrefin = 1;
st->vref_mv = ret / (MICRO / MILLI);
> + ret = devm_mutex_init(dev, &st->lock);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "Failed to init the mutex\n");
One line and do not interleave vref related pieces. I.o.w. move this block
upper before this line "ret = devm_regulator_get_enable(dev, "vdd");".
> + if (ext_vrefin) {
> + /*
> + * Configure the MAX14001/MAX14002 to use an external voltage
> + * reference source for the ADC.
> + */
> + ret = max14001_read(st, MAX14001_REG_CFG, ®_data);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "Failed to read Configuration Register\n");
Indentation issue. Note, for the lines ending with string literals there is no
line limit for a long time (10+ years)
> + reg_data |= FIELD_PREP(MAX14001_REG_CFG_EXRF, 1);
FIELD_MODIFY() ?
> + ret = max14001_write_single_reg(st, MAX14001_REG_CFG, reg_data);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "Failed to set Configuration Register\n");
As per above.
> + }
> +
> + ret = max14001_disable_mv_fault(st);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "Failed to disable MV Fault\n");
One line.
> + return devm_iio_device_register(dev, indio_dev);
> +}
...
> +static const struct of_device_id max14001_of_match[] = {
> + { .compatible = "adi,max14001",
> + .data = &max14001_chip_info_tbl[max14001], },
> + { .compatible = "adi,max14002",
> + .data = &max14001_chip_info_tbl[max14002], },
> + { }
After splitting this hard-to-read style will be just as
{ .compatible = "adi,max14001", .data = &max14001_chip_info },
{ .compatible = "adi,max14002", .data = &max14002_chip_info },
> +};
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v10 2/2] iio: adc: max14001: New driver
2025-09-02 13:45 ` Andy Shevchenko
@ 2025-09-02 14:12 ` David Lechner
2025-09-02 14:23 ` Andy Shevchenko
0 siblings, 1 reply; 13+ messages in thread
From: David Lechner @ 2025-09-02 14:12 UTC (permalink / raw)
To: Andy Shevchenko, Marilene Andrade Garcia
Cc: linux-iio, linux-kernel, devicetree, Kim Seer Paller,
Jonathan Cameron, Nuno Sá, Andy Shevchenko,
Lars-Peter Clausen, Michael Hennerich, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Marcelo Schmitt,
Marcelo Schmitt, Ceclan Dumitru, Jonathan Santos, Dragos Bogdan
On 9/2/25 8:45 AM, Andy Shevchenko wrote:
...
>> + ret = devm_regulator_get_enable_read_voltage(dev, "vrefin");
>> + if (ret < 0) {
>> + st->vref_mv = 1250000 / 1000;
>
> (MICRO / MILLI)
>
>> + } else {
>> + st->vref_mv = ret / 1000;
>
> Ditto.
>
>> + ext_vrefin = 1;
>> + }
>
> And with deduplication refactored code:
>
> ret = devm_regulator_get_enable_read_voltage(dev, "vrefin");
if (ret < 0 && ret != -ENODEV)
return dev_err_probe(dev, ret, "Failed to get REFIN voltage\n");
Most errors should be propagated, so we should also add this.
Only -ENODEV means that the supply was omitted from the devicetree
and we should use the internal reference voltage.
> if (ret < 0)
> ret = 1250000;
> else
> ext_vrefin = 1;
> st->vref_mv = ret / (MICRO / MILLI);
>
>> + ret = devm_mutex_init(dev, &st->lock);
>> + if (ret)
>> + return dev_err_probe(dev, ret,
>> + "Failed to init the mutex\n");
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v10 2/2] iio: adc: max14001: New driver
2025-09-02 14:12 ` David Lechner
@ 2025-09-02 14:23 ` Andy Shevchenko
0 siblings, 0 replies; 13+ messages in thread
From: Andy Shevchenko @ 2025-09-02 14:23 UTC (permalink / raw)
To: David Lechner
Cc: Marilene Andrade Garcia, linux-iio, linux-kernel, devicetree,
Kim Seer Paller, Jonathan Cameron, Nuno Sá, Andy Shevchenko,
Lars-Peter Clausen, Michael Hennerich, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Marcelo Schmitt,
Marcelo Schmitt, Ceclan Dumitru, Jonathan Santos, Dragos Bogdan
On Tue, Sep 02, 2025 at 09:12:08AM -0500, David Lechner wrote:
> On 9/2/25 8:45 AM, Andy Shevchenko wrote:
...
> >> + ret = devm_regulator_get_enable_read_voltage(dev, "vrefin");
> >> + if (ret < 0) {
> >> + st->vref_mv = 1250000 / 1000;
> >
> > (MICRO / MILLI)
> >
> >> + } else {
> >> + st->vref_mv = ret / 1000;
> >
> > Ditto.
> >
> >> + ext_vrefin = 1;
> >> + }
> >
> > And with deduplication refactored code:
> >
> > ret = devm_regulator_get_enable_read_voltage(dev, "vrefin");
>
> if (ret < 0 && ret != -ENODEV)
> return dev_err_probe(dev, ret, "Failed to get REFIN voltage\n");
>
> Most errors should be propagated, so we should also add this.
> Only -ENODEV means that the supply was omitted from the devicetree
> and we should use the internal reference voltage.
Good point.
> > if (ret < 0)
> > ret = 1250000;
> > else
> > ext_vrefin = 1;
> > st->vref_mv = ret / (MICRO / MILLI);
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v10 1/2] dt-bindings: iio: adc: add max14001
2025-09-02 13:15 ` [PATCH v10 1/2] dt-bindings: iio: adc: add max14001 Marilene Andrade Garcia
@ 2025-09-02 14:29 ` David Lechner
2025-09-02 15:30 ` David Lechner
2025-09-02 19:42 ` Conor Dooley
0 siblings, 2 replies; 13+ messages in thread
From: David Lechner @ 2025-09-02 14:29 UTC (permalink / raw)
To: Marilene Andrade Garcia, linux-iio, linux-kernel, devicetree
Cc: Kim Seer Paller, Jonathan Cameron, Nuno Sá, Andy Shevchenko,
Lars-Peter Clausen, Michael Hennerich, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Marcelo Schmitt,
Marcelo Schmitt, Ceclan Dumitru, Jonathan Santos, Dragos Bogdan
On 9/2/25 8:15 AM, Marilene Andrade Garcia wrote:
> Add device-tree documentation for MAX14001/MAX14002 ADCs.
> The MAX14001/MAX14002 are isolated, single-channel analog-to-digital
> converters with programmable voltage comparators and inrush current
> control optimized for configurable binary input applications.
When there are multiple devices, DT maintainers like to know
what is the difference between the devices.
>
> Co-developed-by: Marilene Andrade Garcia <marilene.agarcia@gmail.com>
> Signed-off-by: Marilene Andrade Garcia <marilene.agarcia@gmail.com>
> Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
Sine the patch is From: M.A.G., according to [1], this should be:
Co-developed-by: K.S.P.
Signed-off-by: K.S.P.
Signed-off-by: M.A.G.
(hopefully obvious, but don't use the abbreviations - I just did
that for brevity)
[1]: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by
> ---
> .../bindings/iio/adc/adi,max14001.yaml | 79 +++++++++++++++++++
> MAINTAINERS | 8 ++
> 2 files changed, 87 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,max14001.yaml
>
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,max14001.yaml b/Documentation/devicetree/bindings/iio/adc/adi,max14001.yaml
> new file mode 100644
> index 000000000000..ff9a41f04300
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,max14001.yaml
> @@ -0,0 +1,79 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright 2023-2025 Analog Devices Inc.
> +# Copyright 2023 Kim Seer Paller
> +# Copyright 2025 Marilene Andrade Garcia
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/adi,max14001.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices MAX14001-MAX14002 ADC
> +
> +maintainers:
> + - Kim Seer Paller <kimseer.paller@analog.com>
> + - Marilene Andrade Garcia <marilene.agarcia@gmail.com>
> +
> +description: |
> + Single channel 10 bit ADC with SPI interface.
> + Datasheet can be found here
> + https://www.analog.com/media/en/technical-documentation/data-sheets/MAX14001-MAX14002.pdf
> +
> +$ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +properties:
> + compatible:
> + enum:
> + - adi,max14001
> + - adi,max14002
> +
> + reg:
> + maxItems: 1
> +
> + spi-max-frequency:
> + maximum: 5000000
> +
> + vdd-supply:
> + description:
> + Isolated DC-DC power supply input voltage.
> +
> + vddl-supply:
> + description:
> + Logic power supply.
> +
> + vrefin-supply:
The actual pin name is REFIN, so refin-supply would make more sense.
> + description:
> + ADC voltage reference supply.
> +
> + interrupts:
Likely needs `minItems: 1` in case only one interrupt is wired.
> + items:
> + - description: |
> + Interrupt for signaling when conversion results exceed the configured
> + upper threshold for ADC readings or fall below the lower threshold for
> + them. Interrupt source must be attached to COUT pin.
We could shorten these descriptions. The important part is which pin
it is connected to.
> + - description: |
> + Alert output that asserts low during a number of different error
> + conditions. The interrupt source must be attached to FAULT pin.
> +
And also `interrupt-names:` makes sense so we know which one is
is wired if only one is given.
> +required:
> + - compatible
> + - reg
> + - vdd-supply
> + - vddl-supply
> +
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v10 1/2] dt-bindings: iio: adc: add max14001
2025-09-02 14:29 ` David Lechner
@ 2025-09-02 15:30 ` David Lechner
2025-09-02 19:42 ` Conor Dooley
1 sibling, 0 replies; 13+ messages in thread
From: David Lechner @ 2025-09-02 15:30 UTC (permalink / raw)
To: Marilene Andrade Garcia, linux-iio, linux-kernel, devicetree
Cc: Kim Seer Paller, Jonathan Cameron, Nuno Sá, Andy Shevchenko,
Lars-Peter Clausen, Michael Hennerich, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Marcelo Schmitt,
Marcelo Schmitt, Ceclan Dumitru, Jonathan Santos, Dragos Bogdan
On 9/2/25 9:29 AM, David Lechner wrote:
> On 9/2/25 8:15 AM, Marilene Andrade Garcia wrote:
>> Add device-tree documentation for MAX14001/MAX14002 ADCs.
>> The MAX14001/MAX14002 are isolated, single-channel analog-to-digital
>> converters with programmable voltage comparators and inrush current
>> control optimized for configurable binary input applications.
>
> When there are multiple devices, DT maintainers like to know
> what is the difference between the devices.
>
>>
>> Co-developed-by: Marilene Andrade Garcia <marilene.agarcia@gmail.com>
>> Signed-off-by: Marilene Andrade Garcia <marilene.agarcia@gmail.com>
>> Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
>
> Sine the patch is From: M.A.G., according to [1], this should be:
>
> Co-developed-by: K.S.P.
> Signed-off-by: K.S.P.
> Signed-off-by: M.A.G.
>
> (hopefully obvious, but don't use the abbreviations - I just did
> that for brevity)
>
> [1]: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by
>
>> ---
>> .../bindings/iio/adc/adi,max14001.yaml | 79 +++++++++++++++++++
>> MAINTAINERS | 8 ++
>> 2 files changed, 87 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,max14001.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,max14001.yaml b/Documentation/devicetree/bindings/iio/adc/adi,max14001.yaml
>> new file mode 100644
>> index 000000000000..ff9a41f04300
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/adc/adi,max14001.yaml
>> @@ -0,0 +1,79 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +# Copyright 2023-2025 Analog Devices Inc.
>> +# Copyright 2023 Kim Seer Paller
>> +# Copyright 2025 Marilene Andrade Garcia
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/iio/adc/adi,max14001.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Analog Devices MAX14001-MAX14002 ADC
>> +
>> +maintainers:
>> + - Kim Seer Paller <kimseer.paller@analog.com>
>> + - Marilene Andrade Garcia <marilene.agarcia@gmail.com>
>> +
>> +description: |
>> + Single channel 10 bit ADC with SPI interface.
>> + Datasheet can be found here
>> + https://www.analog.com/media/en/technical-documentation/data-sheets/MAX14001-MAX14002.pdf
>> +
>> +$ref: /schemas/spi/spi-peripheral-props.yaml#
>> +
>> +properties:
>> + compatible:
>> + enum:
>> + - adi,max14001
>> + - adi,max14002
It looks like we could have a fallback here since it looks like
the only difference between the chips is:
The inrush trigger threshold, current magnitude, and
current duration are all programmable in the MAX14001
but are fixed in the MAX14002.
Which would look like this:
compatible:
oneOf:
- const: adi,max14002
- items:
- const: adi,max14001
- const: adi,max14002
And
compatible = "adi,max14001", "adi,max14002";
>> +
>> + reg:
>> + maxItems: 1
>> +
>> + spi-max-frequency:
>> + maximum: 5000000
After reading the driver, I see that we should also have:
spi-lsb-first: true
as a required property.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v10 2/2] iio: adc: max14001: New driver
2025-09-02 13:16 ` [PATCH v10 2/2] iio: adc: max14001: New driver Marilene Andrade Garcia
2025-09-02 13:45 ` Andy Shevchenko
@ 2025-09-02 15:44 ` David Lechner
2025-09-03 22:28 ` kernel test robot
2 siblings, 0 replies; 13+ messages in thread
From: David Lechner @ 2025-09-02 15:44 UTC (permalink / raw)
To: Marilene Andrade Garcia, linux-iio, linux-kernel, devicetree
Cc: Kim Seer Paller, Jonathan Cameron, Nuno Sá, Andy Shevchenko,
Lars-Peter Clausen, Michael Hennerich, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Marcelo Schmitt,
Marcelo Schmitt, Ceclan Dumitru, Jonathan Santos, Dragos Bogdan
On 9/2/25 8:16 AM, 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: Marilene Andrade Garcia <marilene.agarcia@gmail.com>
> Signed-off-by: Marilene Andrade Garcia <marilene.agarcia@gmail.com>
> Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
> ---
> MAINTAINERS | 1 +
> drivers/iio/adc/Kconfig | 10 ++
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/max14001.c | 355 +++++++++++++++++++++++++++++++++++++
> 4 files changed, 367 insertions(+)
> create mode 100644 drivers/iio/adc/max14001.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f145f0204407..b6457063da6c 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -14976,6 +14976,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 e3d3826c3357..11e911ceab4c 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -958,6 +958,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 89d72bf9ce70..569f2f5613d4 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -85,6 +85,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..6755df152976
> --- /dev/null
> +++ b/drivers/iio/adc/max14001.c
> @@ -0,0 +1,355 @@
> +// 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
> + */
> +
> +#include <linux/array_size.h>
> +#include <linux/bitfield.h>
> +#include <linux/bitrev.h>
> +#include <linux/bits.h>
> +#include <linux/byteorder/generic.h>
> +#include <linux/device.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/types.h>
> +#include <linux/kernel.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +#include <linux/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_EXRF BIT(5)
> +
> +#define MAX14001_MASK_ADDR GENMASK(15, 11)
> +#define MAX14001_MASK_DATA GENMASK(9, 0)
> +
> +#define MAX14001_SET_WRITE_BIT BIT(10)
> +#define MAX14001_WRITE_WEN 0x294
> +
> +enum max14001_chip_model {
> + max14001,
> + max14002,
> +};
> +
> +struct max14001_chip_info {
> + const char *name;
> +};
> +
> +struct max14001_state {
> + const struct max14001_chip_info *chip_info;
> + struct spi_device *spi;
> + 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.
Some SPI controllers may be able to handle LSB first without us having
to reverse things in the driver. Search the kernel for SPI_LSB_FIRST
to see what I mean.
By setting `spi-lsb-first;` in the devicetree, the core SPI code will
check if the controller supports it and let the hardare handle everything
so we don't need to reverse bits here in this driver.
If we need to make this work on a SPI controller that doesn't support
SPI_LSB_FIRST, then we should add something in the core SPI code that
does the reversing during __spi_optimize_message() so that every
peripheral driver doesn't have to do this. For example, search spi.c
for SPI_CS_WORD to see how it handles that flag for controllers that
don't support it.
> + * 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 int max14001_probe(struct spi_device *spi)
> +{
...
> + ret = devm_mutex_init(dev, &st->lock);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "Failed to init the mutex\n");
> +
The only possible error here is -ENOMEM, which we don't
print messages for. So just `return ret;` on this one.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v10 0/2] Add MAX14001/MAX14002 support
2025-09-02 13:14 [PATCH v10 0/2] Add MAX14001/MAX14002 support Marilene Andrade Garcia
2025-09-02 13:15 ` [PATCH v10 1/2] dt-bindings: iio: adc: add max14001 Marilene Andrade Garcia
2025-09-02 13:16 ` [PATCH v10 2/2] iio: adc: max14001: New driver Marilene Andrade Garcia
@ 2025-09-02 16:10 ` David Lechner
2025-09-03 16:11 ` Jonathan Cameron
2 siblings, 1 reply; 13+ messages in thread
From: David Lechner @ 2025-09-02 16:10 UTC (permalink / raw)
To: Marilene Andrade Garcia, linux-iio, linux-kernel, devicetree
Cc: Kim Seer Paller, Jonathan Cameron, Nuno Sá, Andy Shevchenko,
Lars-Peter Clausen, Michael Hennerich, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Marcelo Schmitt,
Marcelo Schmitt, Ceclan Dumitru, Jonathan Santos, Dragos Bogdan
On 9/2/25 8:14 AM, Marilene Andrade Garcia wrote:
> Hello everyone,
>
> Thank you for your input on how to handle the situation with the driver code.
> Kim, I also apologize for the unexpected situation involving your previous
> code.
>
> Based on the suggestions, I applied my v1 code changes to v9 of Kim’s code,
> resulting in this v10 version that combines both.
> Compared to v9, the updates are:
>
> - Added support for max14002.
> - Added a function to write a single register, since the write enable
> register must be updated before writing to any others and updated again
> afterward.
> - Renamed the init function to better reflect its purpose, which is to
> disable the memory verification fault. I also replaced the one-by-one
> handling of registers verification values with a loop, since they are in
> sequential ascending order.
> - Replaced the old regulator APIs with the new ones.
> - Updated the device tree documentation to align with the datasheet
> nomenclature for voltage suppliers.
> - Used IIO_CHAN_INFO_AVERAGE_RAW to return the filtered average of ADC
> readings.
>
> One of the reviews I received about my v1 version suggested using a custom
> regmap. I attempted to implement that, but I feel that most of the default
> regmap functions (e.g., regmap_update_bits) would need to be overridden
Usually, you only need to implement one read and one write function for
a custom regmap bus and the core code regmap code will use those for
all of it's function calls. Since you already have a read and write
function, it shouldn't be too hard to adapt them to the regmap bus
callback signatures.
> because of the unique way this device handles communication, such as
> inverting bits before sending a message, updating the write enable register
> before writing any other register, and updating it again afterward. However,
> as I am still new to the IIO kernel code, I may be missing something. If you
> could provide further explanation or an example, I would be grateful.
>
> Regarding locking, Kim’s original code implemented it, and it remains in
> the driver.
>
> I still have a question about using _mean_raw (IIO_CHAN_INFO_AVERAGE_RAW)
> to read the register containing the latest filtered average ADC readings.
> Should I create a v11 version with a patch to include in_voltageY_mean_raw
> in the file /linux/Documentation/ABI/testing/sysfs-bus-iio?
There is already "/sys/bus/iio/devices/iio:deviceX/in_Y_mean_raw" which
I think is intended to cover that.
> The idea is to use in_voltageY_mean_raw to return the filtered average and
> also to set how many ADC readings (0, 2, 4, or 8) are included in the mean
> calculation. Any feedback on using IIO_CHAN_INFO_AVERAGE_RAW this way would
> be appreciated.
>
> The v10 changes were tested on a Raspberry Pi 5 using a modified kernel
> (rpi-6.12). The MAX14001PMB evaluation board, which contains two MAX14001
> devices, was used for testing. One device measures current, and the other
> measures voltage. The evaluation board introduces an offset to allow
> measuring negative values. These board-specific characteristics were not
> included in the driver code (neither the offset nor the current channel
> capability), but they were considered in the calculation of the values read
> by the devices. Should the code that applies these board configuration
> parameters be added as an additional driver file inside the IIO subsystem,
> or should it remain only in a user application?
These features are provided by extra analog frontend (AFE) circuitry
so the are outside of the scope of this driver.
There is an iio/afe/iio-rescale.c driver that can be used to handle this
kind of circuitry. It has "current-sense-amplifier" and "current-sense-shunt".
I didn't look at the eval board schematic in detail to see which one is
the right one for this case. There isn't one for the voltage offset case
though. So if you have some extra time, you could consider adding that.
You will need to add #io-cells to the DT bindings for the MAX chips
so that we can connect it in the devcie tree to the frontend.
amplifier {
compatible = "current-sense-amplifier";
io-channels = <&eval_adc_1>;
sense-resistor-micro-ohms = <?>;
sense-gain-mult = <?>;
};
>
> I plan to continue sending patches to cover all the features of the device.
> This includes adding interrupt handling for faults and for when the signal
> exceeds the upper or lower threshold, implementing the inrush current
> feature, and completing the filtered average reading functionality by
> adding the ability to set the number of readings used in the mean
> calculation.
>
> And I would like to thank again my GSoC mentors Marcelo Schmitt, Ceclan
> Dumitru, Jonathan Santos and Dragos Bogdan for their help with the code.
>
> Thank you for your time,
> Best regards,
> Marilene Andrade Garcia.
>
> Marilene Andrade Garcia (2):
> dt-bindings: iio: adc: add max14001
> iio: adc: max14001: New driver
>
> .../bindings/iio/adc/adi,max14001.yaml | 79 ++++
> MAINTAINERS | 9 +
> drivers/iio/adc/Kconfig | 10 +
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/max14001.c | 355 ++++++++++++++++++
> 5 files changed, 454 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,max14001.yaml
> create mode 100644 drivers/iio/adc/max14001.c
>
>
> base-commit: d1487b0b78720b86ec2a2ac7acc683ec90627e5b
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v10 1/2] dt-bindings: iio: adc: add max14001
2025-09-02 14:29 ` David Lechner
2025-09-02 15:30 ` David Lechner
@ 2025-09-02 19:42 ` Conor Dooley
1 sibling, 0 replies; 13+ messages in thread
From: Conor Dooley @ 2025-09-02 19:42 UTC (permalink / raw)
To: David Lechner
Cc: Marilene Andrade Garcia, linux-iio, linux-kernel, devicetree,
Kim Seer Paller, Jonathan Cameron, Nuno Sá, Andy Shevchenko,
Lars-Peter Clausen, Michael Hennerich, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Marcelo Schmitt,
Marcelo Schmitt, Ceclan Dumitru, Jonathan Santos, Dragos Bogdan
[-- Attachment #1: Type: text/plain, Size: 4465 bytes --]
On Tue, Sep 02, 2025 at 09:29:04AM -0500, David Lechner wrote:
> On 9/2/25 8:15 AM, Marilene Andrade Garcia wrote:
> > Add device-tree documentation for MAX14001/MAX14002 ADCs.
> > The MAX14001/MAX14002 are isolated, single-channel analog-to-digital
> > converters with programmable voltage comparators and inrush current
> > control optimized for configurable binary input applications.
>
> When there are multiple devices, DT maintainers like to know
> what is the difference between the devices.
Looking at the driver, I don't really buy that there even is a
meaningful one, at least at first glance. What even is different between
them other than the name?
>
> >
> > Co-developed-by: Marilene Andrade Garcia <marilene.agarcia@gmail.com>
> > Signed-off-by: Marilene Andrade Garcia <marilene.agarcia@gmail.com>
> > Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
>
> Sine the patch is From: M.A.G., according to [1], this should be:
>
> Co-developed-by: K.S.P.
> Signed-off-by: K.S.P.
> Signed-off-by: M.A.G.
>
> (hopefully obvious, but don't use the abbreviations - I just did
> that for brevity)
Prob also worth putting a note under the --- line as to why the r-b from
Krzysztof got dropped that was on v9.
>
> [1]: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by
>
> > ---
> > .../bindings/iio/adc/adi,max14001.yaml | 79 +++++++++++++++++++
> > MAINTAINERS | 8 ++
> > 2 files changed, 87 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,max14001.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,max14001.yaml b/Documentation/devicetree/bindings/iio/adc/adi,max14001.yaml
> > new file mode 100644
> > index 000000000000..ff9a41f04300
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/adi,max14001.yaml
> > @@ -0,0 +1,79 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +# Copyright 2023-2025 Analog Devices Inc.
> > +# Copyright 2023 Kim Seer Paller
> > +# Copyright 2025 Marilene Andrade Garcia
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iio/adc/adi,max14001.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Analog Devices MAX14001-MAX14002 ADC
> > +
> > +maintainers:
> > + - Kim Seer Paller <kimseer.paller@analog.com>
> > + - Marilene Andrade Garcia <marilene.agarcia@gmail.com>
> > +
> > +description: |
> > + Single channel 10 bit ADC with SPI interface.
> > + Datasheet can be found here
> > + https://www.analog.com/media/en/technical-documentation/data-sheets/MAX14001-MAX14002.pdf
> > +
> > +$ref: /schemas/spi/spi-peripheral-props.yaml#
> > +
> > +properties:
> > + compatible:
> > + enum:
> > + - adi,max14001
> > + - adi,max14002
> > +
> > + reg:
> > + maxItems: 1
> > +
> > + spi-max-frequency:
> > + maximum: 5000000
> > +
> > + vdd-supply:
> > + description:
> > + Isolated DC-DC power supply input voltage.
> > +
> > + vddl-supply:
> > + description:
> > + Logic power supply.
> > +
> > + vrefin-supply:
>
> The actual pin name is REFIN, so refin-supply would make more sense.
>
> > + description:
> > + ADC voltage reference supply.
> > +
> > + interrupts:
>
> Likely needs `minItems: 1` in case only one interrupt is wired.
>
> > + items:
> > + - description: |
> > + Interrupt for signaling when conversion results exceed the configured
> > + upper threshold for ADC readings or fall below the lower threshold for
> > + them. Interrupt source must be attached to COUT pin.
>
> We could shorten these descriptions. The important part is which pin
> it is connected to.
>
> > + - description: |
> > + Alert output that asserts low during a number of different error
> > + conditions. The interrupt source must be attached to FAULT pin.
> > +
>
> And also `interrupt-names:` makes sense so we know which one is
> is wired if only one is given.
Additionally, v9 had no interrupts at all but only serviced once device.
Do both devices have the interrupts or should these only be permitted on
the 140002?
>
> > +required:
> > + - compatible
> > + - reg
> > + - vdd-supply
> > + - vddl-supply
> > +
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v10 0/2] Add MAX14001/MAX14002 support
2025-09-02 16:10 ` [PATCH v10 0/2] Add MAX14001/MAX14002 support David Lechner
@ 2025-09-03 16:11 ` Jonathan Cameron
0 siblings, 0 replies; 13+ messages in thread
From: Jonathan Cameron @ 2025-09-03 16:11 UTC (permalink / raw)
To: David Lechner
Cc: Marilene Andrade Garcia, linux-iio, linux-kernel, devicetree,
Kim Seer Paller, Jonathan Cameron, Nuno Sá, Andy Shevchenko,
Lars-Peter Clausen, Michael Hennerich, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Marcelo Schmitt,
Marcelo Schmitt, Ceclan Dumitru, Jonathan Santos, Dragos Bogdan
>
> > because of the unique way this device handles communication, such as
> > inverting bits before sending a message, updating the write enable register
> > before writing any other register, and updating it again afterward. However,
> > as I am still new to the IIO kernel code, I may be missing something. If you
> > could provide further explanation or an example, I would be grateful.
> >
> > Regarding locking, Kim’s original code implemented it, and it remains in
> > the driver.
> >
> > I still have a question about using _mean_raw (IIO_CHAN_INFO_AVERAGE_RAW)
> > to read the register containing the latest filtered average ADC readings.
> > Should I create a v11 version with a patch to include in_voltageY_mean_raw
> > in the file /linux/Documentation/ABI/testing/sysfs-bus-iio?
>
> There is already "/sys/bus/iio/devices/iio:deviceX/in_Y_mean_raw" which
> I think is intended to cover that.
>
> > The idea is to use in_voltageY_mean_raw to return the filtered average and
> > also to set how many ADC readings (0, 2, 4, or 8) are included in the mean
0 is an odd value, I assume 1 given average of 0 readings is effectively undefined.
> > calculation. Any feedback on using IIO_CHAN_INFO_AVERAGE_RAW this way would
> > be appreciated.
Sorry I missed this question in earlier versions. I'm terrible at reading
cover letters!
Definitely don't use the in_voltage_mean_raw value for the control of the averaging
width. That is too obscure and normal convention is read and write of
sysfs attributes should see effectively the same value (or not all
either read or write)
This is a little unusual as normally when we see this sort of thing it
easily maps to oversampling but in this case it's a moving window average
rather than a downsampling style.
So a few options come to mind.
1. Treat it as a filter on a channel. Describe with 3dB point and type as
box filter.
We probably want to describe it as an additional channel to do this or
we could have assumption that to read unfiltered, you set the
filter 3dB to inf (see other discussion ongoing about that).
2. Add another attribute to add richer info to the in_voltage_mean_raw
Something like in_voltage_mean_num.
Bit of a special case but we have had the mean_raw interface a long time
so we can't get rid of that and it seems illogical to force use of filter
ABI to control it.
> >
Other stuff from David followed but I have nothing to add to that.
Jonathan
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v10 2/2] iio: adc: max14001: New driver
2025-09-02 13:16 ` [PATCH v10 2/2] iio: adc: max14001: New driver Marilene Andrade Garcia
2025-09-02 13:45 ` Andy Shevchenko
2025-09-02 15:44 ` David Lechner
@ 2025-09-03 22:28 ` kernel test robot
2 siblings, 0 replies; 13+ messages in thread
From: kernel test robot @ 2025-09-03 22:28 UTC (permalink / raw)
To: Marilene Andrade Garcia, linux-iio, linux-kernel, devicetree
Cc: oe-kbuild-all, Marilene Andrade Garcia, Kim Seer Paller,
Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
Lars-Peter Clausen, Michael Hennerich, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Marcelo Schmitt,
Ceclan Dumitru, Jonathan Santos, Dragos Bogdan
Hi Marilene,
kernel test robot noticed the following build warnings:
[auto build test WARNING on d1487b0b78720b86ec2a2ac7acc683ec90627e5b]
url: https://github.com/intel-lab-lkp/linux/commits/Marilene-Andrade-Garcia/dt-bindings-iio-adc-add-max14001/20250902-212046
base: d1487b0b78720b86ec2a2ac7acc683ec90627e5b
patch link: https://lore.kernel.org/r/f3ea9c127b7836cc978def5d906740c6da1cfb1e.1756816682.git.marilene.agarcia%40gmail.com
patch subject: [PATCH v10 2/2] iio: adc: max14001: New driver
config: hexagon-randconfig-r113-20250904 (https://download.01.org/0day-ci/archive/20250904/202509040617.gcAKQNlG-lkp@intel.com/config)
compiler: clang version 22.0.0git (https://github.com/llvm/llvm-project 2e122990391b2ba062e6308a12cfedf7206270ba)
reproduce: (https://download.01.org/0day-ci/archive/20250904/202509040617.gcAKQNlG-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202509040617.gcAKQNlG-lkp@intel.com/
sparse warnings: (new ones prefixed by >>)
>> drivers/iio/adc/max14001.c:109:27: sparse: sparse: incorrect type in assignment (different base types) @@ expected restricted __be16 [usertype] spi_tx_buffer @@ got unsigned long @@
drivers/iio/adc/max14001.c:109:27: sparse: expected restricted __be16 [usertype] spi_tx_buffer
drivers/iio/adc/max14001.c:109:27: sparse: got unsigned long
>> drivers/iio/adc/max14001.c:110:29: sparse: sparse: incorrect type in argument 1 (different base types) @@ expected unsigned short [usertype] val @@ got restricted __be16 [usertype] spi_tx_buffer @@
drivers/iio/adc/max14001.c:110:29: sparse: expected unsigned short [usertype] val
drivers/iio/adc/max14001.c:110:29: sparse: got restricted __be16 [usertype] spi_tx_buffer
>> drivers/iio/adc/max14001.c:110:29: sparse: sparse: cast from restricted __be16
>> drivers/iio/adc/max14001.c:110:29: sparse: sparse: cast from restricted __be16
>> drivers/iio/adc/max14001.c:110:29: sparse: sparse: incorrect type in initializer (different base types) @@ expected unsigned short [usertype] __x @@ got restricted __be16 [usertype] @@
drivers/iio/adc/max14001.c:110:29: sparse: expected unsigned short [usertype] __x
drivers/iio/adc/max14001.c:110:29: sparse: got restricted __be16 [usertype]
>> drivers/iio/adc/max14001.c:110:27: sparse: sparse: incorrect type in assignment (different base types) @@ expected restricted __be16 [usertype] spi_tx_buffer @@ got int @@
drivers/iio/adc/max14001.c:110:27: sparse: expected restricted __be16 [usertype] spi_tx_buffer
drivers/iio/adc/max14001.c:110:27: sparse: got int
>> drivers/iio/adc/max14001.c:120:27: sparse: sparse: incorrect type in assignment (different base types) @@ expected restricted __be16 [usertype] spi_rx_buffer @@ got int @@
drivers/iio/adc/max14001.c:120:27: sparse: expected restricted __be16 [usertype] spi_rx_buffer
drivers/iio/adc/max14001.c:120:27: sparse: got int
>> drivers/iio/adc/max14001.c:121:21: sparse: sparse: cast to restricted __be16
>> drivers/iio/adc/max14001.c:121:21: sparse: sparse: restricted __be16 degrades to integer
>> drivers/iio/adc/max14001.c:121:21: sparse: sparse: restricted __be16 degrades to integer
drivers/iio/adc/max14001.c:133:27: sparse: sparse: incorrect type in assignment (different base types) @@ expected restricted __be16 [usertype] spi_tx_buffer @@ got unsigned long @@
drivers/iio/adc/max14001.c:133:27: sparse: expected restricted __be16 [usertype] spi_tx_buffer
drivers/iio/adc/max14001.c:133:27: sparse: got unsigned long
drivers/iio/adc/max14001.c:136:29: sparse: sparse: incorrect type in argument 1 (different base types) @@ expected unsigned short [usertype] val @@ got restricted __be16 [usertype] spi_tx_buffer @@
drivers/iio/adc/max14001.c:136:29: sparse: expected unsigned short [usertype] val
drivers/iio/adc/max14001.c:136:29: sparse: got restricted __be16 [usertype] spi_tx_buffer
drivers/iio/adc/max14001.c:136:29: sparse: sparse: cast from restricted __be16
drivers/iio/adc/max14001.c:136:29: sparse: sparse: cast from restricted __be16
drivers/iio/adc/max14001.c:136:29: sparse: sparse: incorrect type in initializer (different base types) @@ expected unsigned short [usertype] __x @@ got restricted __be16 [usertype] @@
drivers/iio/adc/max14001.c:136:29: sparse: expected unsigned short [usertype] __x
drivers/iio/adc/max14001.c:136:29: sparse: got restricted __be16 [usertype]
drivers/iio/adc/max14001.c:136:27: sparse: sparse: incorrect type in assignment (different base types) @@ expected restricted __be16 [usertype] spi_tx_buffer @@ got int @@
drivers/iio/adc/max14001.c:136:27: sparse: expected restricted __be16 [usertype] spi_tx_buffer
drivers/iio/adc/max14001.c:136:27: sparse: got int
vim +109 drivers/iio/adc/max14001.c
89
90 static int max14001_read(struct max14001_state *st, u16 reg_addr, u16 *reg_data)
91 {
92 struct spi_transfer xfers[] = {
93 {
94 .tx_buf = &st->spi_tx_buffer,
95 .len = sizeof(st->spi_tx_buffer),
96 .cs_change = 1,
97 }, {
98 .rx_buf = &st->spi_rx_buffer,
99 .len = sizeof(st->spi_rx_buffer),
100 },
101 };
102 int ret;
103
104 /*
105 * Prepare SPI transmit buffer 16 bit-value big-endian format and
106 * reverses bit order to align with the LSB-first input on SDI port
107 * in order to meet the device communication requirements.
108 */
> 109 st->spi_tx_buffer = FIELD_PREP(MAX14001_MASK_ADDR, reg_addr);
> 110 st->spi_tx_buffer = bitrev16(cpu_to_be16(st->spi_tx_buffer));
111
112 ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
113 if (ret)
114 return ret;
115
116 /*
117 * Convert received 16-bit value from big-endian to cpu-endian format
118 * and reverses bit order.
119 */
> 120 st->spi_rx_buffer = bitrev16(be16_to_cpu(st->spi_rx_buffer));
> 121 *reg_data = FIELD_GET(MAX14001_MASK_DATA, st->spi_rx_buffer);
122
123 return 0;
124 }
125
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2025-09-03 22:29 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-09-02 13:14 [PATCH v10 0/2] Add MAX14001/MAX14002 support Marilene Andrade Garcia
2025-09-02 13:15 ` [PATCH v10 1/2] dt-bindings: iio: adc: add max14001 Marilene Andrade Garcia
2025-09-02 14:29 ` David Lechner
2025-09-02 15:30 ` David Lechner
2025-09-02 19:42 ` Conor Dooley
2025-09-02 13:16 ` [PATCH v10 2/2] iio: adc: max14001: New driver Marilene Andrade Garcia
2025-09-02 13:45 ` Andy Shevchenko
2025-09-02 14:12 ` David Lechner
2025-09-02 14:23 ` Andy Shevchenko
2025-09-02 15:44 ` David Lechner
2025-09-03 22:28 ` kernel test robot
2025-09-02 16:10 ` [PATCH v10 0/2] Add MAX14001/MAX14002 support David Lechner
2025-09-03 16:11 ` Jonathan Cameron
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).