[PATCH v9 1/2] dt-bindings: iio: adc: add max14001

[PATCH v9 1/2] dt-bindings: iio: adc: add max14001

[Kim Seer Paller]

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

Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 .../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 <kimseer.paller@analog.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
+
+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 e7d4ae01cdcc..0253058c345b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12791,6 +12791,13 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/sound/max9860.txt
 F:	sound/soc/codecs/max9860.*
 
+MAX14001 IIO ADC DRIVER
+M:	Kim Seer Paller <kimseer.paller@analog.com>
+L:	linux-iio@vger.kernel.org
+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 <ak@it-klinger.de>
 L:	linux-iio@vger.kernel.org

base-commit: 62fd3fb591d9faf06e74454f4d3dce0711c59a49
-- 
2.34.1

[PATCH v9 2/2] iio: adc: max14001: New driver

[Kim Seer Paller]

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

Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
---
V8 -> V9: Changed SPI buffer data types to __le16, removed force cast 
and modified spi buffers bit sequence. Removed max14001_reg_update 
function and directly handled its functionality within the caller. 
Removed unused headers.
V7 -> V8: Added a bitwise OR operation to combine the bitfield element 
with the reg_data value.
V6 -> V7: Swapped ADC external vref source and regulator_get_voltage 
calls. Added forced cast and comment to addressed endian warning.
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 | 325 +++++++++++++++++++++++++++++++++++++
 4 files changed, 337 insertions(+)
 create mode 100644 drivers/iio/adc/max14001.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 0253058c345b..bd4bbe4f1749 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12797,6 +12797,7 @@ L:	linux-iio@vger.kernel.org
 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 <ak@it-klinger.de>
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index dc14bde31ac1..bdd5033b0733 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 eb6e891790fb..5a825cf9066f 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_MAX77541_ADC) += max77541-adc.o
diff --git a/drivers/iio/adc/max14001.c b/drivers/iio/adc/max14001.c
new file mode 100644
index 000000000000..09686a9faa2e
--- /dev/null
+++ b/drivers/iio/adc/max14001.c
@@ -0,0 +1,325 @@
+// 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/regulator/consumer.h>
+#include <linux/spi/spi.h>
+#include <linux/types.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.
+	 */
+	__le16			spi_tx_buffer __aligned(IIO_DMA_MINALIGN);
+	__le16			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),
+		},
+	};
+
+	/*
+	 * Prepare SPI transmit buffer 16 bit-value to big-endian format and
+	 * reverses bit order to align with the LSB-first input on SDI port.
+	 */
+	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;
+
+	/*
+	 * Convert received 16-bit value from big-endian to little-endian format
+	 * and reverses bit order.
+	 */
+	*data = bitrev16(be16_to_cpu(st->spi_rx_buffer));
+
+	return 0;
+}
+
+static int max14001_write(void *context, unsigned int reg_addr, unsigned int data)
+{
+	struct max14001_state *st = context;
+
+	/*
+	 * Prepare SPI transmit buffer 16 bit-value to big-endian format and
+	 * reverses bit order to align with the LSB-first input on SDI port.
+	 */
+	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_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;
+	unsigned int reg_data;
+	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;
+
+		ret = regulator_get_voltage(vref);
+		if (ret < 0)
+			return dev_err_probe(dev, ret,
+					     "Failed to get vref\n");
+
+		st->vref_mv = ret / 1000;
+
+		/*
+		 * Enable SPI registers write and select external voltage
+		 * reference source for the ADC.
+		 */
+		ret = max14001_write(st, MAX14001_WEN, MAX14001_WRITE_WEN);
+		if (ret)
+			return ret;
+
+		ret = max14001_read(st, MAX14001_CFG, &reg_data);
+		if (ret)
+			return ret;
+
+		reg_data |= FIELD_PREP(MAX14001_CFG_EXRF, 1);
+
+		ret = max14001_write(st, MAX14001_CFG, reg_data);
+		if (ret)
+			return ret;
+
+		/* Write Verification Register */
+		ret = max14001_write_verification_reg(st, MAX14001_CFG);
+		if (ret)
+			return ret;
+
+		/* Disable SPI Registers Write */
+		ret = max14001_write(st, MAX14001_WEN, 0);
+		if (ret)
+			return ret;
+	}
+
+	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

Re: [PATCH v9 2/2] iio: adc: max14001: New driver

[Andy Shevchenko]

On Mon, Jul 10, 2023 at 7:27 AM Kim Seer Paller
<kimseer.paller@analog.com> wrote:
>
> The MAX14001 is configurable, isolated 10-bit ADCs for multi-range
> binary inputs.

...

> V8 -> V9: Changed SPI buffer data types to __le16,

Why?

...

> +       __le16                  spi_tx_buffer __aligned(IIO_DMA_MINALIGN);
> +       __le16                  spi_rx_buffer;

...

> +       /*
> +        * Prepare SPI transmit buffer 16 bit-value to big-endian format and
> +        * reverses bit order to align with the LSB-first input on SDI port.

reverse

> +        */
> +       st->spi_tx_buffer = bitrev16(cpu_to_be16(FIELD_PREP(MAX14001_ADDR_MASK,
> +                                    reg_addr)));

...

> +       /*
> +        * Convert received 16-bit value from big-endian to little-endian format
> +        * and reverses bit order.

reverse

> +        */
> +       *data = bitrev16(be16_to_cpu(st->spi_rx_buffer));

...

> +       /*
> +        * Prepare SPI transmit buffer 16 bit-value to big-endian format and
> +        * reverses bit order to align with the LSB-first input on SDI port.

reverse

> +        */
> +       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)));

Obviously it's incorrect now even more than before.
The types are defined as __le, while ops are against __be.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v9 2/2] iio: adc: max14001: New driver

[Andy Shevchenko]

On Mon, Jul 10, 2023 at 10:36 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Mon, Jul 10, 2023 at 7:27 AM Kim Seer Paller
> <kimseer.paller@analog.com> wrote:

...

> > V8 -> V9: Changed SPI buffer data types to __le16,
>
> Why?
>
> ...
>
> > +       __le16                  spi_tx_buffer __aligned(IIO_DMA_MINALIGN);
> > +       __le16                  spi_rx_buffer;
>
> ...
>
> > +       /*
> > +        * Prepare SPI transmit buffer 16 bit-value to big-endian format and
> > +        * reverses bit order to align with the LSB-first input on SDI port.
>
> reverse
>
> > +        */
> > +       st->spi_tx_buffer = bitrev16(cpu_to_be16(FIELD_PREP(MAX14001_ADDR_MASK,
> > +                                    reg_addr)));
>
> ...
>
> > +       /*
> > +        * Convert received 16-bit value from big-endian to little-endian format
> > +        * and reverses bit order.
>
> reverse
>
> > +        */
> > +       *data = bitrev16(be16_to_cpu(st->spi_rx_buffer));

On top of that, this left unfixed.

...

> > +       /*
> > +        * Prepare SPI transmit buffer 16 bit-value to big-endian format and
> > +        * reverses bit order to align with the LSB-first input on SDI port.
>
> reverse
>
> > +        */
> > +       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)));
>
> Obviously it's incorrect now even more than before.
> The types are defined as __le, while ops are against __be.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v9 2/2] iio: adc: max14001: New driver

[Andy Shevchenko]

Return to the public space of the discussion.

On Mon, Jul 10, 2023 at 11:17 AM Paller, Kim Seer
<KimSeer.Paller@analog.com> wrote:
> > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Sent: Monday, July 10, 2023 3:37 PM
> > On Mon, Jul 10, 2023 at 7:27 AM Kim Seer Paller <kimseer.paller@analog.com>
> > wrote:

...

> > > V8 -> V9: Changed SPI buffer data types to __le16,
> >
> > Why?
>
> Based on the previous comments, I have taken the __le16 data type
> into account. The device seems to function the same as the __be data type.
> I have not yet sure but technically speaking, do I have to retain the data
> types as __be16 based on the overall operation?

If the type is __be, the *be*() APIs should be used, otherwise __le and *le*().

...

> > Obviously it's incorrect now even more than before.
> > The types are defined as __le, while ops are against __be.
>
> Would it be right to implement this by reverting the types back to __be?
> What other considerations could there be?

First of all, you need to document what you are doing with these bit
twiddlers. Based on the clear understanding by everyone we can suggest
what data type(s) suits the best.

Hence instead of v10, reply with a draft of the comment in the code (I
have asked before) that explains these bit twiddlers.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v9 2/2] iio: adc: max14001: New driver

[Paller, Kim Seer]

> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: Monday, July 10, 2023 6:50 PM
> To: Paller, Kim Seer <KimSeer.Paller@analog.com>; Hennerich, Michael
> <Michael.Hennerich@analog.com>
> Cc: linux-iio <linux-iio@vger.kernel.org>; Jonathan Cameron
> <jic23@kernel.org>; Lars-Peter Clausen <lars@metafoo.de>
> Subject: Re: [PATCH v9 2/2] iio: adc: max14001: New driver
> 
> [External]
> 
> Return to the public space of the discussion.

Oh late to notice, I had mistakenly set my email client to "quick reply" 
instead of 'reply to all'.

> On Mon, Jul 10, 2023 at 11:17 AM Paller, Kim Seer
> <KimSeer.Paller@analog.com> wrote:
> > > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > Sent: Monday, July 10, 2023 3:37 PM
> > > On Mon, Jul 10, 2023 at 7:27 AM Kim Seer Paller
> <kimseer.paller@analog.com>
> > > wrote:
> 
> ...
> 
> > > > V8 -> V9: Changed SPI buffer data types to __le16,
> > >
> > > Why?
> >
> > Based on the previous comments, I have taken the __le16 data type
> > into account. The device seems to function the same as the __be data type.
> > I have not yet sure but technically speaking, do I have to retain the data
> > types as __be16 based on the overall operation?
> 
> If the type is __be, the *be*() APIs should be used, otherwise __le and *le*().

I became a little confused with the previous discussions, will make the 
necessary changes accordingly.

> > > Obviously it's incorrect now even more than before.
> > > The types are defined as __le, while ops are against __be.
> >
> > Would it be right to implement this by reverting the types back to __be?
> > What other considerations could there be?
> 
> First of all, you need to document what you are doing with these bit
> twiddlers. Based on the clear understanding by everyone we can suggest
> what data type(s) suits the best.
> 
> Hence instead of v10, reply with a draft of the comment in the code (I
> have asked before) that explains these bit twiddlers.

In patch v9, regarding with my bit arrangement comments, is it somewhat correct 
or do I need to totally replace it? 

I am not yet familiar with the terminologies, so I hope you can provide some 
suggestions and I'll definitely send the draft first.

Thanks,
Kim

Re: [PATCH v9 2/2] iio: adc: max14001: New driver

[Andy Shevchenko]

On Tue, Jul 11, 2023 at 9:55 AM Paller, Kim Seer
<KimSeer.Paller@analog.com> wrote:
> > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > On Mon, Jul 10, 2023 at 11:17 AM Paller, Kim Seer
> > <KimSeer.Paller@analog.com> wrote:

...

> > Hence instead of v10, reply with a draft of the comment in the code (I
> > have asked before) that explains these bit twiddlers.
>
> In patch v9, regarding with my bit arrangement comments, is it somewhat correct
> or do I need to totally replace it?
>
> I am not yet familiar with the terminologies, so I hope you can provide some
> suggestions and I'll definitely send the draft first.

I'm not sure I understand what comments you are referring to.
The v9 does not explain the algorithm clearly.

What you need is to cite or retell what the datasheet explains about
bit ordering along with the proposed algo (in AN as far as I
understood). Because I haven't got, why do you need to use be16 +
bitrev if your data is le16 (and that's my understanding of the
datasheet). Is it because of the answer from the device? I don't
remember if it keep the bit order the same (i.e. D0...D9) as on the
wire.

For the terminology, use what the datasheet and AN provide you. Also
good to put those URLs to the code and datasheet as Datasheet: tag in
the commit message.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v9 2/2] iio: adc: max14001: New driver

[Jonathan Cameron]

On Tue, 11 Jul 2023 12:08:04 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Tue, Jul 11, 2023 at 9:55 AM Paller, Kim Seer
> <KimSeer.Paller@analog.com> wrote:
> > > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > On Mon, Jul 10, 2023 at 11:17 AM Paller, Kim Seer
> > > <KimSeer.Paller@analog.com> wrote:  
> 
> ...
> 
> > > Hence instead of v10, reply with a draft of the comment in the code (I
> > > have asked before) that explains these bit twiddlers.  
> >
> > In patch v9, regarding with my bit arrangement comments, is it somewhat correct
> > or do I need to totally replace it?
> >
> > I am not yet familiar with the terminologies, so I hope you can provide some
> > suggestions and I'll definitely send the draft first.  
> 
> I'm not sure I understand what comments you are referring to.
> The v9 does not explain the algorithm clearly.
> 
> What you need is to cite or retell what the datasheet explains about
> bit ordering along with the proposed algo (in AN as far as I
> understood). Because I haven't got, why do you need to use be16 +
> bitrev if your data is le16 (and that's my understanding of the
> datasheet). Is it because of the answer from the device? I don't
> remember if it keep the bit order the same (i.e. D0...D9) as on the
> wire.
> 
> For the terminology, use what the datasheet and AN provide you. Also
> good to put those URLs to the code and datasheet as Datasheet: tag in
> the commit message.
> 

Absolutely agree.  The data format is weird enough we need the
info to be available for anyone who tries to work out what is going
on in the future.  This is a case where I'd rather see too much detail
in the comments than too little!

Jonathan

Re: [PATCH v9 2/2] iio: adc: max14001: New driver

[Jonathan Cameron]

On Sun, 16 Jul 2023 14:42:23 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Tue, 11 Jul 2023 12:08:04 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> 
> > On Tue, Jul 11, 2023 at 9:55 AM Paller, Kim Seer
> > <KimSeer.Paller@analog.com> wrote:  
> > > > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > > On Mon, Jul 10, 2023 at 11:17 AM Paller, Kim Seer
> > > > <KimSeer.Paller@analog.com> wrote:    
> > 
> > ...
> >   
> > > > Hence instead of v10, reply with a draft of the comment in the code (I
> > > > have asked before) that explains these bit twiddlers.    
> > >
> > > In patch v9, regarding with my bit arrangement comments, is it somewhat correct
> > > or do I need to totally replace it?
> > >
> > > I am not yet familiar with the terminologies, so I hope you can provide some
> > > suggestions and I'll definitely send the draft first.    
> > 
> > I'm not sure I understand what comments you are referring to.
> > The v9 does not explain the algorithm clearly.
> > 
> > What you need is to cite or retell what the datasheet explains about
> > bit ordering along with the proposed algo (in AN as far as I
> > understood). Because I haven't got, why do you need to use be16 +
> > bitrev if your data is le16 (and that's my understanding of the
> > datasheet). Is it because of the answer from the device? I don't
> > remember if it keep the bit order the same (i.e. D0...D9) as on the
> > wire.
> > 
> > For the terminology, use what the datasheet and AN provide you. Also
> > good to put those URLs to the code and datasheet as Datasheet: tag in
> > the commit message.
> >   
> 
> Absolutely agree.  The data format is weird enough we need the
> info to be available for anyone who tries to work out what is going
> on in the future.  This is a case where I'd rather see too much detail
> in the comments than too little!
> 
> Jonathan

Note that I had applied (and forgotten I'd done so) this driver.
Given the outstanding questions and a build issue with clang, I'm dropping
it for now.  Hopefully that doesn't cause anyone too many problems (those
based on my togreg branch which rarely rebases)

Jonathan