2023-06-14 01:12:05

by Paller, Kim Seer

[permalink] [raw]
Subject: [PATCH v6 1/2] dt-bindings: iio: adc: add max14001

The MAX14001 is a configurable, isolated 10-bit ADC for multi-range
binary inputs.

Signed-off-by: Kim Seer Paller <[email protected]>
---
V5 -> V6: Removed tags.
V3 -> V5: Added spaces between prefixes in subject. Fixed MAINTAINERS reference.

.../bindings/iio/adc/adi,max14001.yaml | 54 +++++++++++++++++++
MAINTAINERS | 7 +++
2 files changed, 61 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..9d03c611fca3
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/adi,max14001.yaml
@@ -0,0 +1,54 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# Copyright 2023 Analog Devices Inc.
+%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 ADC
+
+maintainers:
+ - Kim Seer Paller <[email protected]>
+
+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
+
+properties:
+ compatible:
+ enum:
+ - adi,max14001
+
+ reg:
+ maxItems: 1
+
+ spi-max-frequency:
+ maximum: 5000000
+
+ vref-supply:
+ description: Voltage reference to establish input scaling.
+
+required:
+ - compatible
+ - reg
+
+allOf:
+ - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ spi {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ adc@0 {
+ compatible = "adi,max14001";
+ reg = <0>;
+ spi-max-frequency = <5000000>;
+ vref-supply = <&vref_reg>;
+ };
+ };
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index f794002a192e..dcea2c31f920 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12670,6 +12670,13 @@ S: Maintained
F: Documentation/devicetree/bindings/sound/max9860.txt
F: sound/soc/codecs/max9860.*

+MAX14001 IIO ADC DRIVER
+M: Kim Seer Paller <[email protected]>
+L: [email protected]
+S: Supported
+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 <[email protected]>
L: [email protected]

base-commit: 6f449d52b90fdd927fcf9df0388701de6d5381c6
--
2.34.1



2023-06-14 01:31:12

by Paller, Kim Seer

[permalink] [raw]
Subject: [PATCH v6 2/2] iio: adc: max14001: New driver

The MAX14001 is configurable, isolated 10-bit ADCs for multi-range
binary inputs.

Signed-off-by: Kim Seer Paller <[email protected]>
---
V5 -> V6: Replaced fixed length assignment with dynamic size
assignment in xfers struct initialization. Removed .channel
assignment in max14001_channels definition.
V4 -> V5: No changes.
V3 -> V4: Removed regmap setup, structures, and include.

MAINTAINERS | 1 +
drivers/iio/adc/Kconfig | 10 ++
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/max14001.c | 327 +++++++++++++++++++++++++++++++++++++
4 files changed, 339 insertions(+)
create mode 100644 drivers/iio/adc/max14001.c

diff --git a/MAINTAINERS b/MAINTAINERS
index dcea2c31f920..b527cf471d7a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12676,6 +12676,7 @@ L: [email protected]
S: Supported
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 <[email protected]>
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index eb2b09ef5d5b..e599d166e98d 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -706,6 +706,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 ADC driver"
+ depends on SPI
+ help
+ Say yes here to build support for Analog Devices MAX14001 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 e07e4a3e6237..9f8964258f03 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -65,6 +65,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_MAX9611) += max9611.o
diff --git a/drivers/iio/adc/max14001.c b/drivers/iio/adc/max14001.c
new file mode 100644
index 000000000000..b4d10539398b
--- /dev/null
+++ b/drivers/iio/adc/max14001.c
@@ -0,0 +1,327 @@
+// SPDX-License-Identifier: (GPL-2.0-only OR BSD-3-Clause)
+/*
+ * Analog Devices MAX14001 ADC driver
+ *
+ * Copyright 2023 Analog Devices Inc.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bitrev.h>
+#include <linux/device.h>
+#include <linux/iio/iio.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/property.h>
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+
+#include <asm/unaligned.h>
+
+/* MAX14001 Registers Address */
+#define MAX14001_ADC 0x00
+#define MAX14001_FADC 0x01
+#define MAX14001_FLAGS 0x02
+#define MAX14001_FLTEN 0x03
+#define MAX14001_THL 0x04
+#define MAX14001_THU 0x05
+#define MAX14001_INRR 0x06
+#define MAX14001_INRT 0x07
+#define MAX14001_INRP 0x08
+#define MAX14001_CFG 0x09
+#define MAX14001_ENBL 0x0A
+#define MAX14001_ACT 0x0B
+#define MAX14001_WEN 0x0C
+
+#define MAX14001_VERIFICATION_REG(x) ((x) + 0x10)
+
+#define MAX14001_CFG_EXRF BIT(5)
+
+#define MAX14001_ADDR_MASK GENMASK(15, 11)
+#define MAX14001_DATA_MASK GENMASK(9, 0)
+#define MAX14001_FILTER_MASK GENMASK(3, 2)
+
+#define MAX14001_SET_WRITE_BIT BIT(10)
+#define MAX14001_WRITE_WEN 0x294
+
+struct max14001_state {
+ struct spi_device *spi;
+ /*
+ * lock protect against multiple concurrent accesses, RMW sequence, and
+ * SPI transfer
+ */
+ struct mutex lock;
+ int vref_mv;
+ /*
+ * 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(void *context, unsigned int reg_addr, unsigned int *data)
+{
+ struct max14001_state *st = context;
+ int ret;
+
+ 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),
+ },
+ };
+
+ st->spi_tx_buffer = bitrev16(cpu_to_be16(FIELD_PREP(MAX14001_ADDR_MASK,
+ reg_addr)));
+
+ ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
+ if (ret)
+ return ret;
+
+ *data = bitrev16(be16_to_cpu(st->spi_rx_buffer)) & MAX14001_DATA_MASK;
+
+ return 0;
+}
+
+static int max14001_write(void *context, unsigned int reg_addr, unsigned int data)
+{
+ struct max14001_state *st = context;
+
+ st->spi_tx_buffer = bitrev16(cpu_to_be16(
+ FIELD_PREP(MAX14001_ADDR_MASK, reg_addr) |
+ FIELD_PREP(MAX14001_SET_WRITE_BIT, 1) |
+ FIELD_PREP(MAX14001_DATA_MASK, data)));
+
+ return spi_write(st->spi, &st->spi_tx_buffer, sizeof(st->spi_tx_buffer));
+}
+
+static int max14001_write_verification_reg(struct max14001_state *st,
+ unsigned int reg_addr)
+{
+ unsigned int reg_data;
+ int ret;
+
+ ret = max14001_read(st, reg_addr, &reg_data);
+ if (ret)
+ return ret;
+
+ return max14001_write(st, MAX14001_VERIFICATION_REG(reg_addr), reg_data);
+}
+
+static int max14001_reg_update(struct max14001_state *st,
+ unsigned int reg_addr,
+ unsigned int mask,
+ unsigned int val)
+{
+ int ret;
+ unsigned int reg_data;
+
+ /* Enable SPI Registers Write */
+ ret = max14001_write(st, MAX14001_WEN, MAX14001_WRITE_WEN);
+ if (ret)
+ return ret;
+
+ ret = max14001_read(st, reg_addr, &reg_data);
+ if (ret)
+ return ret;
+
+ reg_data = FIELD_PREP(mask, val);
+
+ ret = max14001_write(st, reg_addr, reg_data);
+ if (ret)
+ return ret;
+
+ /* Write Verification Register */
+ ret = max14001_write_verification_reg(st, reg_addr);
+ if (ret)
+ return ret;
+
+ /* Disable SPI Registers Write */
+ return max14001_write(st, MAX14001_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);
+ unsigned int data;
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ mutex_lock(&st->lock);
+ ret = max14001_read(st, MAX14001_ADC, &data);
+ mutex_unlock(&st->lock);
+ if (ret < 0)
+ return ret;
+
+ *val = 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_channels[] = {
+ {
+ .type = IIO_VOLTAGE,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+ BIT(IIO_CHAN_INFO_SCALE),
+ }
+};
+
+static void max14001_regulator_disable(void *data)
+{
+ struct regulator *reg = data;
+
+ regulator_disable(reg);
+}
+
+static int max14001_init(struct max14001_state *st)
+{
+ int ret;
+
+ /* Enable SPI Registers Write */
+ ret = max14001_write(st, MAX14001_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.
+ */
+ ret = max14001_write_verification_reg(st, MAX14001_FLTEN);
+ if (ret)
+ return ret;
+
+ ret = max14001_write_verification_reg(st, MAX14001_THL);
+ if (ret)
+ return ret;
+
+ ret = max14001_write_verification_reg(st, MAX14001_THU);
+ if (ret)
+ return ret;
+
+ ret = max14001_write_verification_reg(st, MAX14001_INRR);
+ if (ret)
+ return ret;
+
+ ret = max14001_write_verification_reg(st, MAX14001_INRT);
+ if (ret)
+ return ret;
+
+ ret = max14001_write_verification_reg(st, MAX14001_INRP);
+ if (ret)
+ return ret;
+
+ ret = max14001_write_verification_reg(st, MAX14001_CFG);
+ if (ret)
+ return ret;
+
+ ret = max14001_write_verification_reg(st, MAX14001_ENBL);
+ if (ret)
+ return ret;
+
+ /* Disable SPI Registers Write */
+ return max14001_write(st, MAX14001_WEN, 0);
+}
+
+static int max14001_probe(struct spi_device *spi)
+{
+ struct iio_dev *indio_dev;
+ struct max14001_state *st;
+ struct regulator *vref;
+ struct device *dev = &spi->dev;
+ int ret;
+
+ indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
+ if (!indio_dev)
+ return -ENOMEM;
+
+ st = iio_priv(indio_dev);
+ st->spi = spi;
+
+ indio_dev->name = "max14001";
+ indio_dev->info = &max14001_info;
+ indio_dev->channels = max14001_channels;
+ indio_dev->num_channels = ARRAY_SIZE(max14001_channels);
+ indio_dev->modes = INDIO_DIRECT_MODE;
+
+ ret = max14001_init(st);
+ if (ret)
+ return ret;
+
+ vref = devm_regulator_get_optional(dev, "vref");
+ if (IS_ERR(vref)) {
+ if (PTR_ERR(vref) != -ENODEV)
+ return dev_err_probe(dev, PTR_ERR(vref),
+ "Failed to get vref regulator");
+
+ /* internal reference */
+ st->vref_mv = 1250;
+ } else {
+ ret = regulator_enable(vref);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "Failed to enable vref regulators\n");
+
+ ret = devm_add_action_or_reset(dev, max14001_regulator_disable,
+ vref);
+ if (ret)
+ return ret;
+
+ /* select external voltage reference source for the ADC */
+ ret = max14001_reg_update(st, MAX14001_CFG,
+ MAX14001_CFG_EXRF, 1);
+
+ ret = regulator_get_voltage(vref);
+ if (ret < 0)
+ return dev_err_probe(dev, ret,
+ "Failed to get vref\n");
+
+ st->vref_mv = ret / 1000;
+ }
+
+ mutex_init(&st->lock);
+
+ return devm_iio_device_register(dev, indio_dev);
+}
+
+static const struct of_device_id max14001_of_match[] = {
+ { .compatible = "adi,max14001" },
+ { }
+};
+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,
+};
+module_spi_driver(max14001_driver);
+
+MODULE_AUTHOR("Kim Seer Paller");
+MODULE_DESCRIPTION("MAX14001 ADC driver");
+MODULE_LICENSE("GPL");
--
2.34.1


2023-06-14 06:40:51

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v6 1/2] dt-bindings: iio: adc: add max14001

On 14/06/2023 02:48, Kim Seer Paller wrote:
> The MAX14001 is a configurable, isolated 10-bit ADC for multi-range
> binary inputs.
>
> Signed-off-by: Kim Seer Paller <[email protected]>
> ---
> V5 -> V6: Removed tags.

Not the review one tag! Drop the fake reports because there was not
report and nothing to close.

Reviewed-by: Krzysztof Kozlowski <[email protected]>

Best regards,
Krzysztof


2023-06-14 09:47:51

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH v6 2/2] iio: adc: max14001: New driver

On Wed, Jun 14, 2023 at 3:49 AM Kim Seer Paller
<[email protected]> wrote:
>
> The MAX14001 is configurable, isolated 10-bit ADCs for multi-range
> binary inputs.

Reviewed-by: Andy Shevchenko <[email protected]>
One question below.

> Signed-off-by: Kim Seer Paller <[email protected]>
> ---
> V5 -> V6: Replaced fixed length assignment with dynamic size
> assignment in xfers struct initialization. Removed .channel
> assignment in max14001_channels definition.
> V4 -> V5: No changes.
> V3 -> V4: Removed regmap setup, structures, and include.
>
> MAINTAINERS | 1 +
> drivers/iio/adc/Kconfig | 10 ++
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/max14001.c | 327 +++++++++++++++++++++++++++++++++++++
> 4 files changed, 339 insertions(+)
> create mode 100644 drivers/iio/adc/max14001.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index dcea2c31f920..b527cf471d7a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12676,6 +12676,7 @@ L: [email protected]
> S: Supported
> 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 <[email protected]>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index eb2b09ef5d5b..e599d166e98d 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -706,6 +706,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 ADC driver"
> + depends on SPI
> + help
> + Say yes here to build support for Analog Devices MAX14001 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 e07e4a3e6237..9f8964258f03 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -65,6 +65,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_MAX9611) += max9611.o
> diff --git a/drivers/iio/adc/max14001.c b/drivers/iio/adc/max14001.c
> new file mode 100644
> index 000000000000..b4d10539398b
> --- /dev/null
> +++ b/drivers/iio/adc/max14001.c
> @@ -0,0 +1,327 @@
> +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-3-Clause)
> +/*
> + * Analog Devices MAX14001 ADC driver
> + *
> + * Copyright 2023 Analog Devices Inc.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bitrev.h>
> +#include <linux/device.h>
> +#include <linux/iio/iio.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/property.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +
> +#include <asm/unaligned.h>
> +
> +/* MAX14001 Registers Address */
> +#define MAX14001_ADC 0x00
> +#define MAX14001_FADC 0x01
> +#define MAX14001_FLAGS 0x02
> +#define MAX14001_FLTEN 0x03
> +#define MAX14001_THL 0x04
> +#define MAX14001_THU 0x05
> +#define MAX14001_INRR 0x06
> +#define MAX14001_INRT 0x07
> +#define MAX14001_INRP 0x08
> +#define MAX14001_CFG 0x09
> +#define MAX14001_ENBL 0x0A
> +#define MAX14001_ACT 0x0B
> +#define MAX14001_WEN 0x0C
> +
> +#define MAX14001_VERIFICATION_REG(x) ((x) + 0x10)
> +
> +#define MAX14001_CFG_EXRF BIT(5)
> +
> +#define MAX14001_ADDR_MASK GENMASK(15, 11)
> +#define MAX14001_DATA_MASK GENMASK(9, 0)
> +#define MAX14001_FILTER_MASK GENMASK(3, 2)
> +
> +#define MAX14001_SET_WRITE_BIT BIT(10)
> +#define MAX14001_WRITE_WEN 0x294
> +
> +struct max14001_state {
> + struct spi_device *spi;
> + /*
> + * lock protect against multiple concurrent accesses, RMW sequence, and
> + * SPI transfer
> + */
> + struct mutex lock;
> + int vref_mv;
> + /*
> + * 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(void *context, unsigned int reg_addr, unsigned int *data)
> +{
> + struct max14001_state *st = context;
> + int ret;
> +
> + 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),
> + },
> + };
> +
> + st->spi_tx_buffer = bitrev16(cpu_to_be16(FIELD_PREP(MAX14001_ADDR_MASK,
> + reg_addr)));
> +
> + ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
> + if (ret)
> + return ret;
> +
> + *data = bitrev16(be16_to_cpu(st->spi_rx_buffer)) & MAX14001_DATA_MASK;
> +
> + return 0;
> +}
> +
> +static int max14001_write(void *context, unsigned int reg_addr, unsigned int data)
> +{
> + struct max14001_state *st = context;
> +
> + st->spi_tx_buffer = bitrev16(cpu_to_be16(
> + FIELD_PREP(MAX14001_ADDR_MASK, reg_addr) |
> + FIELD_PREP(MAX14001_SET_WRITE_BIT, 1) |
> + FIELD_PREP(MAX14001_DATA_MASK, data)));
> +
> + return spi_write(st->spi, &st->spi_tx_buffer, sizeof(st->spi_tx_buffer));
> +}
> +
> +static int max14001_write_verification_reg(struct max14001_state *st,
> + unsigned int reg_addr)
> +{
> + unsigned int reg_data;
> + int ret;
> +
> + ret = max14001_read(st, reg_addr, &reg_data);
> + if (ret)
> + return ret;
> +
> + return max14001_write(st, MAX14001_VERIFICATION_REG(reg_addr), reg_data);
> +}
> +
> +static int max14001_reg_update(struct max14001_state *st,
> + unsigned int reg_addr,
> + unsigned int mask,
> + unsigned int val)
> +{
> + int ret;
> + unsigned int reg_data;
> +
> + /* Enable SPI Registers Write */
> + ret = max14001_write(st, MAX14001_WEN, MAX14001_WRITE_WEN);
> + if (ret)
> + return ret;
> +
> + ret = max14001_read(st, reg_addr, &reg_data);
> + if (ret)
> + return ret;
> +
> + reg_data = FIELD_PREP(mask, val);
> +
> + ret = max14001_write(st, reg_addr, reg_data);
> + if (ret)
> + return ret;
> +
> + /* Write Verification Register */
> + ret = max14001_write_verification_reg(st, reg_addr);
> + if (ret)
> + return ret;
> +
> + /* Disable SPI Registers Write */
> + return max14001_write(st, MAX14001_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);
> + unsigned int data;
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + mutex_lock(&st->lock);
> + ret = max14001_read(st, MAX14001_ADC, &data);
> + mutex_unlock(&st->lock);
> + if (ret < 0)
> + return ret;
> +
> + *val = 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_channels[] = {
> + {
> + .type = IIO_VOLTAGE,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_SCALE),
> + }
> +};
> +
> +static void max14001_regulator_disable(void *data)
> +{
> + struct regulator *reg = data;
> +
> + regulator_disable(reg);
> +}
> +
> +static int max14001_init(struct max14001_state *st)
> +{
> + int ret;
> +
> + /* Enable SPI Registers Write */
> + ret = max14001_write(st, MAX14001_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.
> + */
> + ret = max14001_write_verification_reg(st, MAX14001_FLTEN);
> + if (ret)
> + return ret;
> +
> + ret = max14001_write_verification_reg(st, MAX14001_THL);
> + if (ret)
> + return ret;
> +
> + ret = max14001_write_verification_reg(st, MAX14001_THU);
> + if (ret)
> + return ret;
> +
> + ret = max14001_write_verification_reg(st, MAX14001_INRR);
> + if (ret)
> + return ret;
> +
> + ret = max14001_write_verification_reg(st, MAX14001_INRT);
> + if (ret)
> + return ret;
> +
> + ret = max14001_write_verification_reg(st, MAX14001_INRP);
> + if (ret)
> + return ret;
> +
> + ret = max14001_write_verification_reg(st, MAX14001_CFG);
> + if (ret)
> + return ret;
> +
> + ret = max14001_write_verification_reg(st, MAX14001_ENBL);
> + if (ret)
> + return ret;
> +
> + /* Disable SPI Registers Write */
> + return max14001_write(st, MAX14001_WEN, 0);
> +}
> +
> +static int max14001_probe(struct spi_device *spi)
> +{
> + struct iio_dev *indio_dev;
> + struct max14001_state *st;
> + struct regulator *vref;
> + struct device *dev = &spi->dev;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + st = iio_priv(indio_dev);
> + st->spi = spi;
> +
> + indio_dev->name = "max14001";
> + indio_dev->info = &max14001_info;
> + indio_dev->channels = max14001_channels;
> + indio_dev->num_channels = ARRAY_SIZE(max14001_channels);
> + indio_dev->modes = INDIO_DIRECT_MODE;
> +
> + ret = max14001_init(st);
> + if (ret)
> + return ret;
> +
> + vref = devm_regulator_get_optional(dev, "vref");
> + if (IS_ERR(vref)) {
> + if (PTR_ERR(vref) != -ENODEV)
> + return dev_err_probe(dev, PTR_ERR(vref),
> + "Failed to get vref regulator");
> +
> + /* internal reference */
> + st->vref_mv = 1250;
> + } else {
> + ret = regulator_enable(vref);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "Failed to enable vref regulators\n");
> +
> + ret = devm_add_action_or_reset(dev, max14001_regulator_disable,
> + vref);
> + if (ret)
> + return ret;

> + /* select external voltage reference source for the ADC */
> + ret = max14001_reg_update(st, MAX14001_CFG,
> + MAX14001_CFG_EXRF, 1);
> +
> + ret = regulator_get_voltage(vref);
> + if (ret < 0)
> + return dev_err_probe(dev, ret,
> + "Failed to get vref\n");

Is it important to choose the external reference source _before_
getting the voltage of the regulator? If not, I would swap these
calls, otherwise the comment is needed to explain why the sequence is
important in the way it's written.

> + st->vref_mv = ret / 1000;
> + }
> +
> + mutex_init(&st->lock);
> +
> + return devm_iio_device_register(dev, indio_dev);
> +}
> +
> +static const struct of_device_id max14001_of_match[] = {
> + { .compatible = "adi,max14001" },
> + { }
> +};
> +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,
> +};
> +module_spi_driver(max14001_driver);
> +
> +MODULE_AUTHOR("Kim Seer Paller");
> +MODULE_DESCRIPTION("MAX14001 ADC driver");
> +MODULE_LICENSE("GPL");
> --
> 2.34.1
>


--
With Best Regards,
Andy Shevchenko

2023-06-14 10:49:37

by Paller, Kim Seer

[permalink] [raw]
Subject: RE: [PATCH v6 2/2] iio: adc: max14001: New driver


> -----Original Message-----
> From: Andy Shevchenko <[email protected]>
> Sent: Wednesday, June 14, 2023 5:12 PM
> To: Paller, Kim Seer <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; Hennerich, Michael <[email protected]>;
> [email protected]; [email protected]; [email protected]; linux-
> [email protected]; [email protected]; [email protected]
> Subject: Re: [PATCH v6 2/2] iio: adc: max14001: New driver
>
> [External]
>
> On Wed, Jun 14, 2023 at 3:49 AM Kim Seer Paller
> <[email protected]> wrote:
> >
> > The MAX14001 is configurable, isolated 10-bit ADCs for multi-range
> > binary inputs.
>
> Reviewed-by: Andy Shevchenko <[email protected]> One question
> below.
>
> > Signed-off-by: Kim Seer Paller <[email protected]>
> > ---

...

> > + /* select external voltage reference source for the ADC */
> > + ret = max14001_reg_update(st, MAX14001_CFG,
> > + MAX14001_CFG_EXRF, 1);
> > +
> > + ret = regulator_get_voltage(vref);
> > + if (ret < 0)
> > + return dev_err_probe(dev, ret,
> > + "Failed to get vref\n");
>
> Is it important to choose the external reference source _before_ getting the
> voltage of the regulator? If not, I would swap these calls, otherwise the
> comment is needed to explain why the sequence is important in the way it's
> written.

It is not important. These calls can be swap without any issues.

Best regards,
Kim Seer Paller

2023-06-14 12:26:32

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH v6 2/2] iio: adc: max14001: New driver

On Wed, Jun 14, 2023 at 1:43 PM Paller, Kim Seer
<[email protected]> wrote:
> > From: Andy Shevchenko <[email protected]>
> > Sent: Wednesday, June 14, 2023 5:12 PM

...

> > > + /* select external voltage reference source for the ADC */
> > > + ret = max14001_reg_update(st, MAX14001_CFG,
> > > + MAX14001_CFG_EXRF, 1);
> > > +
> > > + ret = regulator_get_voltage(vref);
> > > + if (ret < 0)
> > > + return dev_err_probe(dev, ret,
> > > + "Failed to get vref\n");
> >
> > Is it important to choose the external reference source _before_ getting the
> > voltage of the regulator? If not, I would swap these calls, otherwise the
> > comment is needed to explain why the sequence is important in the way it's
> > written.
>
> It is not important. These calls can be swap without any issues.

If you send a new version, please swap, otherwise I hope Jonathan can
do it when applying.

--
With Best Regards,
Andy Shevchenko

2023-06-17 18:45:38

by Jonathan Cameron

[permalink] [raw]
Subject: Re: [PATCH v6 2/2] iio: adc: max14001: New driver

On Wed, 14 Jun 2023 14:53:12 +0300
Andy Shevchenko <[email protected]> wrote:

> On Wed, Jun 14, 2023 at 1:43 PM Paller, Kim Seer
> <[email protected]> wrote:
> > > From: Andy Shevchenko <[email protected]>
> > > Sent: Wednesday, June 14, 2023 5:12 PM
>
> ...
>
> > > > + /* select external voltage reference source for the ADC */
> > > > + ret = max14001_reg_update(st, MAX14001_CFG,
> > > > + MAX14001_CFG_EXRF, 1);
> > > > +
> > > > + ret = regulator_get_voltage(vref);
> > > > + if (ret < 0)
> > > > + return dev_err_probe(dev, ret,
> > > > + "Failed to get vref\n");
> > >
> > > Is it important to choose the external reference source _before_ getting the
> > > voltage of the regulator? If not, I would swap these calls, otherwise the
> > > comment is needed to explain why the sequence is important in the way it's
> > > written.
> >
> > It is not important. These calls can be swap without any issues.
>
> If you send a new version, please swap, otherwise I hope Jonathan can
> do it when applying.
>

I made these changes whilst applying...

--- a/drivers/iio/adc/max14001.c
+++ b/drivers/iio/adc/max14001.c
@@ -290,16 +290,19 @@ static int max14001_probe(struct spi_device *spi)
if (ret)
return ret;

- /* select external voltage reference source for the ADC */
- ret = max14001_reg_update(st, MAX14001_CFG,
- MAX14001_CFG_EXRF, 1);
-
ret = regulator_get_voltage(vref);
if (ret < 0)
return dev_err_probe(dev, ret,
"Failed to get vref\n");

st->vref_mv = ret / 1000;
+
+ /* select external voltage reference source for the ADC */
+ ret = max14001_reg_update(st, MAX14001_CFG,
+ MAX14001_CFG_EXRF, 1);
+
+ if (ret < 0)
+ return ret;
}

mutex_init(&st->lock);


Note that whilst I've applied this to the togreg branch of iio.git - it is just
a tiny bit too late to make the merge window that starts in about a week.
As such I'll only push this out as testing until I can rebase the tree
on rc1 once available.

Thanks,

Jonathan

2023-06-17 18:46:08

by Jonathan Cameron

[permalink] [raw]
Subject: Re: [PATCH v6 2/2] iio: adc: max14001: New driver

On Wed, 14 Jun 2023 08:48:57 +0800
Kim Seer Paller <[email protected]> wrote:

> The MAX14001 is configurable, isolated 10-bit ADCs for multi-range
> binary inputs.
>
> Signed-off-by: Kim Seer Paller <[email protected]>

Build tests showed up a set of warnings we need to deal with.

Jonathan


> +
> +static int max14001_read(void *context, unsigned int reg_addr, unsigned int *data)
> +{
> + struct max14001_state *st = context;
> + int ret;
> +
> + 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),
> + },
> + };
> +
> + st->spi_tx_buffer = bitrev16(cpu_to_be16(FIELD_PREP(MAX14001_ADDR_MASK,
> + reg_addr)));

This gives an endian warning with sparse.

drivers/iio/adc/max14001.c:81:29: warning: incorrect type in initializer (different base types)
drivers/iio/adc/max14001.c:81:29: expected unsigned short [usertype] __x
drivers/iio/adc/max14001.c:81:29: got restricted __be16 [usertype]
drivers/iio/adc/max14001.c:81:27: warning: incorrect type in assignment (different base types)
drivers/iio/adc/max14001.c:81:27: expected restricted __be16 [usertype] spi_tx_buffer
drivers/iio/adc/max14001.c:81:27: got int
drivers/iio/adc/max14001.c:97:29: warning: incorrect type in initializer (different base types)
drivers/iio/adc/max14001.c:97:29: expected unsigned short [usertype] __x
drivers/iio/adc/max14001.c:97:29: got restricted __be16 [usertype]
drivers/iio/adc/max14001.c:97:27: warning: incorrect type in assignment (different base types)
drivers/iio/adc/max14001.c:97:27: expected restricted __be16 [usertype] spi_tx_buffer
drivers/iio/adc/max14001.c:97:27: got int

Using a forced cast and adding a comment is probably fine to fix this... Bit reversing
a big endian value is not going to be common enough to warrant anything clever.

> +
> + ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
> + if (ret)
> + return ret;
> +
> + *data = bitrev16(be16_to_cpu(st->spi_rx_buffer)) & MAX14001_DATA_MASK;
But it got me looking. Seems a bit odd that this isn't a direct reverse of what
is done on the tx bfufer.
e.g.
be16_to_cpu(bitrev16(st->spi_rx_buffer))

I'm struggling to figure out enough of the datasheet to understand this.
Perhaps you could add some comments on the logic?


> +
> + return 0;
> +}
> +
> +static int max14001_write(void *context, unsigned int reg_addr, unsigned int data)
> +{
> + struct max14001_state *st = context;
> +
> + st->spi_tx_buffer = bitrev16(cpu_to_be16(
> + FIELD_PREP(MAX14001_ADDR_MASK, reg_addr) |
> + FIELD_PREP(MAX14001_SET_WRITE_BIT, 1) |
> + FIELD_PREP(MAX14001_DATA_MASK, data)));
> +
> + return spi_write(st->spi, &st->spi_tx_buffer, sizeof(st->spi_tx_buffer));
> +}

2023-06-17 18:54:02

by Jonathan Cameron

[permalink] [raw]
Subject: Re: [PATCH v6 2/2] iio: adc: max14001: New driver

On Sat, 17 Jun 2023 19:34:23 +0100
Jonathan Cameron <[email protected]> wrote:

> On Wed, 14 Jun 2023 14:53:12 +0300
> Andy Shevchenko <[email protected]> wrote:
>
> > On Wed, Jun 14, 2023 at 1:43 PM Paller, Kim Seer
> > <[email protected]> wrote:
> > > > From: Andy Shevchenko <[email protected]>
> > > > Sent: Wednesday, June 14, 2023 5:12 PM
> >
> > ...
> >
> > > > > + /* select external voltage reference source for the ADC */
> > > > > + ret = max14001_reg_update(st, MAX14001_CFG,
> > > > > + MAX14001_CFG_EXRF, 1);
> > > > > +
> > > > > + ret = regulator_get_voltage(vref);
> > > > > + if (ret < 0)
> > > > > + return dev_err_probe(dev, ret,
> > > > > + "Failed to get vref\n");
> > > >
> > > > Is it important to choose the external reference source _before_ getting the
> > > > voltage of the regulator? If not, I would swap these calls, otherwise the
> > > > comment is needed to explain why the sequence is important in the way it's
> > > > written.
> > >
> > > It is not important. These calls can be swap without any issues.
> >
> > If you send a new version, please swap, otherwise I hope Jonathan can
> > do it when applying.
> >
>
> I made these changes whilst applying...
>
> --- a/drivers/iio/adc/max14001.c
> +++ b/drivers/iio/adc/max14001.c
> @@ -290,16 +290,19 @@ static int max14001_probe(struct spi_device *spi)
> if (ret)
> return ret;
>
> - /* select external voltage reference source for the ADC */
> - ret = max14001_reg_update(st, MAX14001_CFG,
> - MAX14001_CFG_EXRF, 1);
> -
> ret = regulator_get_voltage(vref);
> if (ret < 0)
> return dev_err_probe(dev, ret,
> "Failed to get vref\n");
>
> st->vref_mv = ret / 1000;
> +
> + /* select external voltage reference source for the ADC */
> + ret = max14001_reg_update(st, MAX14001_CFG,
> + MAX14001_CFG_EXRF, 1);
> +
> + if (ret < 0)
> + return ret;
> }
>
> mutex_init(&st->lock);
>
>
> Note that whilst I've applied this to the togreg branch of iio.git - it is just
> a tiny bit too late to make the merge window that starts in about a week.
> As such I'll only push this out as testing until I can rebase the tree
> on rc1 once available.

Actually nope. Dropped it again because a question came up in my build tests.

I'll address that in a separate reply.

Jonathan

>
> Thanks,
>
> Jonathan


2023-06-17 19:01:43

by Jonathan Cameron

[permalink] [raw]
Subject: Re: [PATCH v6 2/2] iio: adc: max14001: New driver

On Wed, 14 Jun 2023 12:11:55 +0300
Andy Shevchenko <[email protected]> wrote:

> On Wed, Jun 14, 2023 at 3:49 AM Kim Seer Paller
> <[email protected]> wrote:
> >
> > The MAX14001 is configurable, isolated 10-bit ADCs for multi-range
> > binary inputs.
>
> Reviewed-by: Andy Shevchenko <[email protected]>
> One question below.
>
> > Signed-off-by: Kim Seer Paller <[email protected]>
> > ---
> > V5 -> V6: Replaced fixed length assignment with dynamic size
> > assignment in xfers struct initialization. Removed .channel
> > assignment in max14001_channels definition.
> > V4 -> V5: No changes.
> > V3 -> V4: Removed regmap setup, structures, and include.
> >
> > MAINTAINERS | 1 +
> > drivers/iio/adc/Kconfig | 10 ++
> > drivers/iio/adc/Makefile | 1 +
> > drivers/iio/adc/max14001.c | 327 +++++++++++++++++++++++++++++++++++++
> > 4 files changed, 339 insertions(+)
> > create mode 100644 drivers/iio/adc/max14001.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index dcea2c31f920..b527cf471d7a 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -12676,6 +12676,7 @@ L: [email protected]
> > S: Supported
> > 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 <[email protected]>
> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > index eb2b09ef5d5b..e599d166e98d 100644
> > --- a/drivers/iio/adc/Kconfig
> > +++ b/drivers/iio/adc/Kconfig
> > @@ -706,6 +706,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 ADC driver"
> > + depends on SPI
> > + help
> > + Say yes here to build support for Analog Devices MAX14001 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 e07e4a3e6237..9f8964258f03 100644
> > --- a/drivers/iio/adc/Makefile
> > +++ b/drivers/iio/adc/Makefile
> > @@ -65,6 +65,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_MAX9611) += max9611.o
> > diff --git a/drivers/iio/adc/max14001.c b/drivers/iio/adc/max14001.c
> > new file mode 100644
> > index 000000000000..b4d10539398b
> > --- /dev/null
> > +++ b/drivers/iio/adc/max14001.c
> > @@ -0,0 +1,327 @@
> > +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-3-Clause)
> > +/*
> > + * Analog Devices MAX14001 ADC driver
> > + *
> > + * Copyright 2023 Analog Devices Inc.
> > + */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/bitrev.h>
> > +#include <linux/device.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/property.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/spi/spi.h>
> > +#include <linux/slab.h>
> > +#include <linux/types.h>
> > +
> > +#include <asm/unaligned.h>
> > +
> > +/* MAX14001 Registers Address */
> > +#define MAX14001_ADC 0x00
> > +#define MAX14001_FADC 0x01
> > +#define MAX14001_FLAGS 0x02
> > +#define MAX14001_FLTEN 0x03
> > +#define MAX14001_THL 0x04
> > +#define MAX14001_THU 0x05
> > +#define MAX14001_INRR 0x06
> > +#define MAX14001_INRT 0x07
> > +#define MAX14001_INRP 0x08
> > +#define MAX14001_CFG 0x09
> > +#define MAX14001_ENBL 0x0A
> > +#define MAX14001_ACT 0x0B
> > +#define MAX14001_WEN 0x0C
> > +
> > +#define MAX14001_VERIFICATION_REG(x) ((x) + 0x10)
> > +
> > +#define MAX14001_CFG_EXRF BIT(5)
> > +
> > +#define MAX14001_ADDR_MASK GENMASK(15, 11)
> > +#define MAX14001_DATA_MASK GENMASK(9, 0)
> > +#define MAX14001_FILTER_MASK GENMASK(3, 2)
> > +
> > +#define MAX14001_SET_WRITE_BIT BIT(10)
> > +#define MAX14001_WRITE_WEN 0x294
> > +
> > +struct max14001_state {
> > + struct spi_device *spi;
> > + /*
> > + * lock protect against multiple concurrent accesses, RMW sequence, and
> > + * SPI transfer
> > + */
> > + struct mutex lock;
> > + int vref_mv;
> > + /*
> > + * 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(void *context, unsigned int reg_addr, unsigned int *data)
> > +{
> > + struct max14001_state *st = context;
> > + int ret;
> > +
> > + 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),
> > + },
> > + };
> > +
> > + st->spi_tx_buffer = bitrev16(cpu_to_be16(FIELD_PREP(MAX14001_ADDR_MASK,
> > + reg_addr)));
> > +
> > + ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
> > + if (ret)
> > + return ret;
> > +
> > + *data = bitrev16(be16_to_cpu(st->spi_rx_buffer)) & MAX14001_DATA_MASK;
> > +
> > + return 0;
> > +}
> > +
> > +static int max14001_write(void *context, unsigned int reg_addr, unsigned int data)
> > +{
> > + struct max14001_state *st = context;
> > +
> > + st->spi_tx_buffer = bitrev16(cpu_to_be16(
> > + FIELD_PREP(MAX14001_ADDR_MASK, reg_addr) |
> > + FIELD_PREP(MAX14001_SET_WRITE_BIT, 1) |
> > + FIELD_PREP(MAX14001_DATA_MASK, data)));
> > +
> > + return spi_write(st->spi, &st->spi_tx_buffer, sizeof(st->spi_tx_buffer));
> > +}
> > +
> > +static int max14001_write_verification_reg(struct max14001_state *st,
> > + unsigned int reg_addr)
> > +{
> > + unsigned int reg_data;
> > + int ret;
> > +
> > + ret = max14001_read(st, reg_addr, &reg_data);
> > + if (ret)
> > + return ret;
> > +
> > + return max14001_write(st, MAX14001_VERIFICATION_REG(reg_addr), reg_data);
> > +}
> > +
> > +static int max14001_reg_update(struct max14001_state *st,
> > + unsigned int reg_addr,
> > + unsigned int mask,
> > + unsigned int val)
> > +{
> > + int ret;
> > + unsigned int reg_data;
> > +
> > + /* Enable SPI Registers Write */
> > + ret = max14001_write(st, MAX14001_WEN, MAX14001_WRITE_WEN);
> > + if (ret)
> > + return ret;
> > +
> > + ret = max14001_read(st, reg_addr, &reg_data);
> > + if (ret)
> > + return ret;
> > +
> > + reg_data = FIELD_PREP(mask, val);
> > +
> > + ret = max14001_write(st, reg_addr, reg_data);
> > + if (ret)
> > + return ret;
> > +
> > + /* Write Verification Register */
> > + ret = max14001_write_verification_reg(st, reg_addr);
> > + if (ret)
> > + return ret;
> > +
> > + /* Disable SPI Registers Write */
> > + return max14001_write(st, MAX14001_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);
> > + unsigned int data;
> > + int ret;
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_RAW:
> > + mutex_lock(&st->lock);
> > + ret = max14001_read(st, MAX14001_ADC, &data);
> > + mutex_unlock(&st->lock);
> > + if (ret < 0)
> > + return ret;
> > +
> > + *val = 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_channels[] = {
> > + {
> > + .type = IIO_VOLTAGE,
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > + BIT(IIO_CHAN_INFO_SCALE),
> > + }
> > +};
> > +
> > +static void max14001_regulator_disable(void *data)
> > +{
> > + struct regulator *reg = data;
> > +
> > + regulator_disable(reg);
> > +}
> > +
> > +static int max14001_init(struct max14001_state *st)
> > +{
> > + int ret;
> > +
> > + /* Enable SPI Registers Write */
> > + ret = max14001_write(st, MAX14001_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.
> > + */
> > + ret = max14001_write_verification_reg(st, MAX14001_FLTEN);
> > + if (ret)
> > + return ret;
> > +
> > + ret = max14001_write_verification_reg(st, MAX14001_THL);
> > + if (ret)
> > + return ret;
> > +
> > + ret = max14001_write_verification_reg(st, MAX14001_THU);
> > + if (ret)
> > + return ret;
> > +
> > + ret = max14001_write_verification_reg(st, MAX14001_INRR);
> > + if (ret)
> > + return ret;
> > +
> > + ret = max14001_write_verification_reg(st, MAX14001_INRT);
> > + if (ret)
> > + return ret;
> > +
> > + ret = max14001_write_verification_reg(st, MAX14001_INRP);
> > + if (ret)
> > + return ret;
> > +
> > + ret = max14001_write_verification_reg(st, MAX14001_CFG);
> > + if (ret)
> > + return ret;
> > +
> > + ret = max14001_write_verification_reg(st, MAX14001_ENBL);
> > + if (ret)
> > + return ret;
> > +
> > + /* Disable SPI Registers Write */
> > + return max14001_write(st, MAX14001_WEN, 0);
> > +}
> > +
> > +static int max14001_probe(struct spi_device *spi)
> > +{
> > + struct iio_dev *indio_dev;
> > + struct max14001_state *st;
> > + struct regulator *vref;
> > + struct device *dev = &spi->dev;
> > + int ret;
> > +
> > + indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> > + if (!indio_dev)
> > + return -ENOMEM;
> > +
> > + st = iio_priv(indio_dev);
> > + st->spi = spi;
> > +
> > + indio_dev->name = "max14001";
> > + indio_dev->info = &max14001_info;
> > + indio_dev->channels = max14001_channels;
> > + indio_dev->num_channels = ARRAY_SIZE(max14001_channels);
> > + indio_dev->modes = INDIO_DIRECT_MODE;
> > +
> > + ret = max14001_init(st);
> > + if (ret)
> > + return ret;
> > +
> > + vref = devm_regulator_get_optional(dev, "vref");
> > + if (IS_ERR(vref)) {
> > + if (PTR_ERR(vref) != -ENODEV)
> > + return dev_err_probe(dev, PTR_ERR(vref),
> > + "Failed to get vref regulator");
> > +
> > + /* internal reference */
> > + st->vref_mv = 1250;
> > + } else {
> > + ret = regulator_enable(vref);
> > + if (ret)
> > + return dev_err_probe(dev, ret,
> > + "Failed to enable vref regulators\n");
> > +
> > + ret = devm_add_action_or_reset(dev, max14001_regulator_disable,
> > + vref);
> > + if (ret)
> > + return ret;
>
> > + /* select external voltage reference source for the ADC */
> > + ret = max14001_reg_update(st, MAX14001_CFG,
> > + MAX14001_CFG_EXRF, 1);
> > +

ret not checked.

> > + ret = regulator_get_voltage(vref);
> > + if (ret < 0)
> > + return dev_err_probe(dev, ret,
> > + "Failed to get vref\n");
>
> Is it important to choose the external reference source _before_
> getting the voltage of the regulator? If not, I would swap these
> calls, otherwise the comment is needed to explain why the sequence is
> important in the way it's written.
>
> > + st->vref_mv = ret / 1000;
> > + }
> > +
> > + mutex_init(&st->lock);
> > +
> > + return devm_iio_device_register(dev, indio_dev);
> > +}
> > +
> > +static const struct of_device_id max14001_of_match[] = {
> > + { .compatible = "adi,max14001" },
> > + { }
> > +};
> > +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,
> > +};
> > +module_spi_driver(max14001_driver);
> > +
> > +MODULE_AUTHOR("Kim Seer Paller");
> > +MODULE_DESCRIPTION("MAX14001 ADC driver");
> > +MODULE_LICENSE("GPL");
> > --
> > 2.34.1
> >
>
>