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

[PATCH v8 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>
---
V7 -> V8: Added "Reviewed-by" tag.
V6 -> V7: No changes.
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 <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 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 <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: 6f449d52b90fdd927fcf9df0388701de6d5381c6
-- 
2.34.1

[PATCH v8 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>
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202306211545.7b6CdqsL-lkp@intel.com/
---
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 | 340 +++++++++++++++++++++++++++++++++++++
 4 files changed, 352 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:	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 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..a21ebcde71fa
--- /dev/null
+++ b/drivers/iio/adc/max14001.c
@@ -0,0 +1,340 @@
+// 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),
+		},
+	};
+
+	/*
+	 * Convert transmit buffer to big-endian format and reverse transmit
+	 * buffer to align with the LSB-first input on SDI port.
+	 */
+	st->spi_tx_buffer = cpu_to_be16(bitrev16(FIELD_PREP(MAX14001_ADDR_MASK,
+								reg_addr)));
+
+	ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
+	if (ret)
+		return ret;
+
+	/*
+	 * Align received data from the receive buffer, reversing and reordering
+	 * it to match the expected MSB-first format.
+	 */
+	*data = (__force u16)(be16_to_cpu(bitrev16(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;
+
+	/*
+	 * Convert transmit buffer to big-endian format and reverse transmit
+	 * buffer to align with the LSB-first input on SDI port.
+	 */
+	st->spi_tx_buffer = (__force u16)(cpu_to_be16(bitrev16(
+				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;
+
+		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);
+
+	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 v8 2/2] iio: adc: max14001: New driver

[Jonathan Cameron]

On Thu, 22 Jun 2023 22:32:27 +0800
Kim Seer Paller <kimseer.paller@analog.com> wrote:

> The MAX14001 is configurable, isolated 10-bit ADCs for multi-range
> binary inputs.
> 
> Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202306211545.7b6CdqsL-lkp@intel.com/

Hi, 

Two outstanding comments that I think I raised in earlier reviews..

Jonathan

> diff --git a/drivers/iio/adc/max14001.c b/drivers/iio/adc/max14001.c
> new file mode 100644
> index 000000000000..a21ebcde71fa
> --- /dev/null
> +++ b/drivers/iio/adc/max14001.c

...

> +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),
> +		},
> +	};
> +
> +	/*
> +	 * Convert transmit buffer to big-endian format and reverse transmit
> +	 * buffer to align with the LSB-first input on SDI port.
> +	 */
> +	st->spi_tx_buffer = cpu_to_be16(bitrev16(FIELD_PREP(MAX14001_ADDR_MASK,
> +								reg_addr)));
> +
> +	ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Align received data from the receive buffer, reversing and reordering
> +	 * it to match the expected MSB-first format.
> +	 */
> +	*data = (__force u16)(be16_to_cpu(bitrev16(st->spi_rx_buffer))) &
> +							MAX14001_DATA_MASK;
> +
These sequences still confuse me a lot because I'd expect the values in tx
to have the opposite operations applied to those for rx and that's not the
case.

Let's take a le system. 
tx = cpu_to_be16(bitrev16(x))
   = cpu_to_be16((__bitrev8(x & 0xff) << 8) | __bitrev8(x >> 8));
   = __bitrev8(x & 0xff) | (__bitrev8(x >> 8) << 8)
or swap all the bits in each byte, but don't swap the bytes.

rx = cpu_to_be16(bitrev16(x))
   = be16_to_cpu(((__bitrev8(x & 0xff) << 8) | __bitrev8(x >> 8)_
   = __bitrev8(x & 0xff) | __bitrev(x >> 8)
   
also swap all the bits in each byte, but don't swap the bytes.

So it is the reverse because the bytes swaps unwind themselves somewhat.
For a be system cpu_to_be16 etc are noop.
tx = (__bitrev8(x & 0xff) << 8) | __bitrev8(x >> 8)
rx = (__bitrev8(x & 0xff) << 8) | __bitrev8(x >> 8)

So in this case swap all 16 bits.

Now, given I'd expected them to be reversed for the tx vs rx case.
E.g.
tx = cpu_to_be16(bitrev16(x))
As above.
For rx, le host
rx = bitrev16(be16_to_cpu(x))
   = __bitrev8((x >> 8) & 0xff) << 8) |  __bitrev8((((x & 0xff) << 8) >> 8)
same as above (if you swap the two terms I think.

For be the be16_to_cpu is a noop again, so it's just bitrev16(x) as expected.

Hence if I've understood this correctly you could reverse the terms so that
it was 'obvious' you were doing the opposite for the tx term vs the rx one
without making the slightest bit of difference....
   
hmm. Might be worth doing simply to avoid questions.


> +	return 0;
> +}
> +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);

This is still a problem if the compiler happens to fail to figure out
that mask is a compile time constant.  Given it only ever takes one value
I'd suggest either calling the FIELD_PREP at the caller, or just 
pushing all this code inline so that you can put the definition
inline.

> +
> +	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);
> +}

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

[Paller, Kim Seer]

> -----Original Message-----
> From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> Sent: Sunday, July 2, 2023 6:04 PM
> To: Paller, Kim Seer <KimSeer.Paller@analog.com>
> Cc: jic23@kernel.org; lars@metafoo.de; lgirdwood@gmail.com;
> broonie@kernel.org; Hennerich, Michael <Michael.Hennerich@analog.com>;
> andy.shevchenko@gmail.com; robh@kernel.org;
> krzysztof.kozlowski@linaro.org; conor+dt@kernel.org; linux-
> iio@vger.kernel.org; linux-kernel@vger.kernel.org; devicetree@vger.kernel.org;
> kernel test robot <lkp@intel.com>
> Subject: Re: [PATCH v8 2/2] iio: adc: max14001: New driver
> 
> [External]
> 
> On Thu, 22 Jun 2023 22:32:27 +0800
> Kim Seer Paller <kimseer.paller@analog.com> wrote:
> 
> > The MAX14001 is configurable, isolated 10-bit ADCs for multi-range
> > binary inputs.
> >
> > Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
> > Reported-by: kernel test robot <lkp@intel.com>
> > Closes:
> > https://urldefense.com/v3/__https://lore.kernel.org/oe-kbuild-all/2023
> > 06211545.7b6CdqsL-
> lkp@intel.com/__;!!A3Ni8CS0y2Y!4npD8X6TpKmeLcUf8QqQW
> >
> yEFp_Z1ORKb2dZNpuqfj0ZK74NiCYKQLNWEfKzVmuKTHJO0RW8n01vdXURqBvc
> ueb3V1Sb
> > GQdI$
> 
> Hi,
> 
> Two outstanding comments that I think I raised in earlier reviews..
> 
> Jonathan
> 
> > diff --git a/drivers/iio/adc/max14001.c b/drivers/iio/adc/max14001.c
> > new file mode 100644 index 000000000000..a21ebcde71fa
> > --- /dev/null
> > +++ b/drivers/iio/adc/max14001.c
> 
> ...
> 
> > +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),
> > +		},
> > +	};
> > +
> > +	/*
> > +	 * Convert transmit buffer to big-endian format and reverse transmit
> > +	 * buffer to align with the LSB-first input on SDI port.
> > +	 */
> > +	st->spi_tx_buffer =
> cpu_to_be16(bitrev16(FIELD_PREP(MAX14001_ADDR_MASK,
> > +								reg_addr)));
> > +
> > +	ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
> > +	if (ret)
> > +		return ret;
> > +
> > +	/*
> > +	 * Align received data from the receive buffer, reversing and reordering
> > +	 * it to match the expected MSB-first format.
> > +	 */
> > +	*data = (__force u16)(be16_to_cpu(bitrev16(st->spi_rx_buffer))) &
> > +
> 	MAX14001_DATA_MASK;
> > +
> These sequences still confuse me a lot because I'd expect the values in tx
> to have the opposite operations applied to those for rx and that's not the
> case.
> 
> Let's take a le system.
> tx = cpu_to_be16(bitrev16(x))
>    = cpu_to_be16((__bitrev8(x & 0xff) << 8) | __bitrev8(x >> 8));
>    = __bitrev8(x & 0xff) | (__bitrev8(x >> 8) << 8)
> or swap all the bits in each byte, but don't swap the bytes.
> 
> rx = cpu_to_be16(bitrev16(x))
>    = be16_to_cpu(((__bitrev8(x & 0xff) << 8) | __bitrev8(x >> 8)_
>    = __bitrev8(x & 0xff) | __bitrev(x >> 8)
> 
> also swap all the bits in each byte, but don't swap the bytes.
> 
> So it is the reverse because the bytes swaps unwind themselves somewhat.
> For a be system cpu_to_be16 etc are noop.
> tx = (__bitrev8(x & 0xff) << 8) | __bitrev8(x >> 8)
> rx = (__bitrev8(x & 0xff) << 8) | __bitrev8(x >> 8)
> 
> So in this case swap all 16 bits.
> 
> Now, given I'd expected them to be reversed for the tx vs rx case.
> E.g.
> tx = cpu_to_be16(bitrev16(x))
> As above.
> For rx, le host
> rx = bitrev16(be16_to_cpu(x))
>    = __bitrev8((x >> 8) & 0xff) << 8) |  __bitrev8((((x & 0xff) << 8) >> 8)
> same as above (if you swap the two terms I think.
> 
> For be the be16_to_cpu is a noop again, so it's just bitrev16(x) as expected.
> 
> Hence if I've understood this correctly you could reverse the terms so that
> it was 'obvious' you were doing the opposite for the tx term vs the rx one
> without making the slightest bit of difference....
> 
> hmm. Might be worth doing simply to avoid questions.

Thank you for your feedback. I have tested the modifications based on your 
suggestions, taking the le system into account, and it appears that the code is 
functioning correctly. Before sending the new patch version, I would like to 
confirm if this aligns with your comments.

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 = cpu_to_be16(bitrev16(FIELD_PREP(MAX14001_ADDR_MASK, reg_addr)));

	ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
	if (ret)
		return ret;

	*data = cpu_to_be16(bitrev16(st->spi_rx_buffer));

	return 0;
}

static int max14001_write(void *context, unsigned int reg_addr, unsigned int data)
{
	struct max14001_state *st = context;

	st->spi_tx_buffer = cpu_to_be16(bitrev16(
				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));
}
 
> > +	return 0;
> > +}
> > +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);
> 
> This is still a problem if the compiler happens to fail to figure out
> that mask is a compile time constant.  Given it only ever takes one value
> I'd suggest either calling the FIELD_PREP at the caller, or just
> pushing all this code inline so that you can put the definition
> inline.

I would like to confirm including the 'static inline' keyword for the 
max14001_reg_update function.

> > +
> > +	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);
> > +}
> 

Best Regards,
Kim Seer Paller

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

[Jonathan Cameron]

On Tue, 4 Jul 2023 09:40:56 +0000
"Paller, Kim Seer" <KimSeer.Paller@analog.com> wrote:

> > -----Original Message-----
> > From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> > Sent: Sunday, July 2, 2023 6:04 PM
> > To: Paller, Kim Seer <KimSeer.Paller@analog.com>
> > Cc: jic23@kernel.org; lars@metafoo.de; lgirdwood@gmail.com;
> > broonie@kernel.org; Hennerich, Michael <Michael.Hennerich@analog.com>;
> > andy.shevchenko@gmail.com; robh@kernel.org;
> > krzysztof.kozlowski@linaro.org; conor+dt@kernel.org; linux-
> > iio@vger.kernel.org; linux-kernel@vger.kernel.org; devicetree@vger.kernel.org;
> > kernel test robot <lkp@intel.com>
> > Subject: Re: [PATCH v8 2/2] iio: adc: max14001: New driver
> > 
> > [External]
> > 
> > On Thu, 22 Jun 2023 22:32:27 +0800
> > Kim Seer Paller <kimseer.paller@analog.com> wrote:
> >   
> > > The MAX14001 is configurable, isolated 10-bit ADCs for multi-range
> > > binary inputs.
> > >
> > > Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
> > > Reported-by: kernel test robot <lkp@intel.com>
> > > Closes:
> > > https://urldefense.com/v3/__https://lore.kernel.org/oe-kbuild-all/2023
> > > 06211545.7b6CdqsL-  
> > lkp@intel.com/__;!!A3Ni8CS0y2Y!4npD8X6TpKmeLcUf8QqQW  
> > >  
> > yEFp_Z1ORKb2dZNpuqfj0ZK74NiCYKQLNWEfKzVmuKTHJO0RW8n01vdXURqBvc
> > ueb3V1Sb  
> > > GQdI$  
> > 
> > Hi,
> > 
> > Two outstanding comments that I think I raised in earlier reviews..
> > 
> > Jonathan
> >   
> > > diff --git a/drivers/iio/adc/max14001.c b/drivers/iio/adc/max14001.c
> > > new file mode 100644 index 000000000000..a21ebcde71fa
> > > --- /dev/null
> > > +++ b/drivers/iio/adc/max14001.c  
> > 
> > ...
> >   
> > > +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),
> > > +		},
> > > +	};
> > > +
> > > +	/*
> > > +	 * Convert transmit buffer to big-endian format and reverse transmit
> > > +	 * buffer to align with the LSB-first input on SDI port.
> > > +	 */
> > > +	st->spi_tx_buffer =  
> > cpu_to_be16(bitrev16(FIELD_PREP(MAX14001_ADDR_MASK,  
> > > +								reg_addr)));
> > > +
> > > +	ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	/*
> > > +	 * Align received data from the receive buffer, reversing and reordering
> > > +	 * it to match the expected MSB-first format.
> > > +	 */
> > > +	*data = (__force u16)(be16_to_cpu(bitrev16(st->spi_rx_buffer))) &
> > > +  
> > 	MAX14001_DATA_MASK;  
> > > +  
> > These sequences still confuse me a lot because I'd expect the values in tx
> > to have the opposite operations applied to those for rx and that's not the
> > case.
> > 
> > Let's take a le system.
> > tx = cpu_to_be16(bitrev16(x))
> >    = cpu_to_be16((__bitrev8(x & 0xff) << 8) | __bitrev8(x >> 8));
> >    = __bitrev8(x & 0xff) | (__bitrev8(x >> 8) << 8)
> > or swap all the bits in each byte, but don't swap the bytes.
> > 
> > rx = cpu_to_be16(bitrev16(x))
> >    = be16_to_cpu(((__bitrev8(x & 0xff) << 8) | __bitrev8(x >> 8)_
> >    = __bitrev8(x & 0xff) | __bitrev(x >> 8)
> > 
> > also swap all the bits in each byte, but don't swap the bytes.
> > 
> > So it is the reverse because the bytes swaps unwind themselves somewhat.
> > For a be system cpu_to_be16 etc are noop.
> > tx = (__bitrev8(x & 0xff) << 8) | __bitrev8(x >> 8)
> > rx = (__bitrev8(x & 0xff) << 8) | __bitrev8(x >> 8)
> > 
> > So in this case swap all 16 bits.
> > 
> > Now, given I'd expected them to be reversed for the tx vs rx case.
> > E.g.
> > tx = cpu_to_be16(bitrev16(x))
> > As above.
> > For rx, le host
> > rx = bitrev16(be16_to_cpu(x))
> >    = __bitrev8((x >> 8) & 0xff) << 8) |  __bitrev8((((x & 0xff) << 8) >> 8)
> > same as above (if you swap the two terms I think.
> > 
> > For be the be16_to_cpu is a noop again, so it's just bitrev16(x) as expected.
> > 
> > Hence if I've understood this correctly you could reverse the terms so that
> > it was 'obvious' you were doing the opposite for the tx term vs the rx one
> > without making the slightest bit of difference....
> > 
> > hmm. Might be worth doing simply to avoid questions.  
> 
> Thank you for your feedback. I have tested the modifications based on your 
> suggestions, taking the le system into account, and it appears that the code is 
> functioning correctly. Before sending the new patch version, I would like to 
> confirm if this aligns with your comments.
Yes. This looks good to me.

> 
> 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 = cpu_to_be16(bitrev16(FIELD_PREP(MAX14001_ADDR_MASK, reg_addr)));
> 
> 	ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
> 	if (ret)
> 		return ret;
> 
> 	*data = cpu_to_be16(bitrev16(st->spi_rx_buffer));
> 
> 	return 0;
> }
> 
> static int max14001_write(void *context, unsigned int reg_addr, unsigned int data)
> {
> 	struct max14001_state *st = context;
> 
> 	st->spi_tx_buffer = cpu_to_be16(bitrev16(
> 				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));
> }
>  
> > > +	return 0;
> > > +}
> > > +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);  
> > 
> > This is still a problem if the compiler happens to fail to figure out
> > that mask is a compile time constant.  Given it only ever takes one value
> > I'd suggest either calling the FIELD_PREP at the caller, or just
> > pushing all this code inline so that you can put the definition
> > inline.  
> 
> I would like to confirm including the 'static inline' keyword for the 
> max14001_reg_update function.

I think it's not sufficient as the compiler is allowed to ignore that.
There are ways to force it but they aren't pretty so don't use them!

Better to reorganize the code so that the value is clearly const
without the compiler having to be clever.

Jonathan

> 
> > > +
> > > +	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);
> > > +}  
> >   
> 
> Best Regards,
> Kim Seer Paller
> 
> 

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

[Andy Shevchenko]

On Wed, Jul 5, 2023 at 10:55 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
> > > From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> > > Sent: Sunday, July 2, 2023 6:04 PM
> > > On Thu, 22 Jun 2023 22:32:27 +0800
> > > Kim Seer Paller <kimseer.paller@analog.com> wrote:

...

> > > > + /*
> > > > +  * Convert transmit buffer to big-endian format and reverse transmit
> > > > +  * buffer to align with the LSB-first input on SDI port.
> > > > +  */
> > > > + st->spi_tx_buffer =
> > > cpu_to_be16(bitrev16(FIELD_PREP(MAX14001_ADDR_MASK,
> > > > +                                                         reg_addr)));
> > > > +
> > > > + ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
> > > > + if (ret)
> > > > +         return ret;
> > > > +
> > > > + /*
> > > > +  * Align received data from the receive buffer, reversing and reordering
> > > > +  * it to match the expected MSB-first format.
> > > > +  */
> > > > + *data = (__force u16)(be16_to_cpu(bitrev16(st->spi_rx_buffer))) &
> > > > +
> > >     MAX14001_DATA_MASK;
> > > > +
> > > These sequences still confuse me a lot because I'd expect the values in tx
> > > to have the opposite operations applied to those for rx and that's not the
> > > case.
> > >
> > > Let's take a le system.
> > > tx = cpu_to_be16(bitrev16(x))
> > >    = cpu_to_be16((__bitrev8(x & 0xff) << 8) | __bitrev8(x >> 8));
> > >    = __bitrev8(x & 0xff) | (__bitrev8(x >> 8) << 8)
> > > or swap all the bits in each byte, but don't swap the bytes.
> > >
> > > rx = cpu_to_be16(bitrev16(x))
> > >    = be16_to_cpu(((__bitrev8(x & 0xff) << 8) | __bitrev8(x >> 8)_
> > >    = __bitrev8(x & 0xff) | __bitrev(x >> 8)
> > >
> > > also swap all the bits in each byte, but don't swap the bytes.
> > >
> > > So it is the reverse because the bytes swaps unwind themselves somewhat.
> > > For a be system cpu_to_be16 etc are noop.
> > > tx = (__bitrev8(x & 0xff) << 8) | __bitrev8(x >> 8)
> > > rx = (__bitrev8(x & 0xff) << 8) | __bitrev8(x >> 8)
> > >
> > > So in this case swap all 16 bits.
> > >
> > > Now, given I'd expected them to be reversed for the tx vs rx case.
> > > E.g.
> > > tx = cpu_to_be16(bitrev16(x))
> > > As above.
> > > For rx, le host
> > > rx = bitrev16(be16_to_cpu(x))
> > >    = __bitrev8((x >> 8) & 0xff) << 8) |  __bitrev8((((x & 0xff) << 8) >> 8)
> > > same as above (if you swap the two terms I think.
> > >
> > > For be the be16_to_cpu is a noop again, so it's just bitrev16(x) as expected.
> > >
> > > Hence if I've understood this correctly you could reverse the terms so that
> > > it was 'obvious' you were doing the opposite for the tx term vs the rx one
> > > without making the slightest bit of difference....
> > >
> > > hmm. Might be worth doing simply to avoid questions.
> >
> > Thank you for your feedback. I have tested the modifications based on your
> > suggestions, taking the le system into account, and it appears that the code is
> > functioning correctly. Before sending the new patch version, I would like to
> > confirm if this aligns with your comments.

> Yes. This looks good to me.

I think the implementation is still incorrect. See below.

> > 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 = cpu_to_be16(bitrev16(FIELD_PREP(MAX14001_ADDR_MASK, reg_addr)));

Here we got bits in CPU order, reversed them and converted to BE16.

> >       ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
> >       if (ret)
> >               return ret;

> >       *data = cpu_to_be16(bitrev16(st->spi_rx_buffer));

Here we take __be16 response, reverse them and convert to BE16?!
This is weird. You should have be16_to_cpu() somewhere, not the opposite.

> >       return 0;
> > }

Isn't, btw, the reinvented spi_...write_then_read() (or what is it
called?) call?

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

-- 
With Best Regards,
Andy Shevchenko

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

[Jonathan Cameron]

On Wed, 5 Jul 2023 11:53:17 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Wed, Jul 5, 2023 at 10:55 AM Jonathan Cameron
> <Jonathan.Cameron@huawei.com> wrote:
> > > > From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> > > > Sent: Sunday, July 2, 2023 6:04 PM
> > > > On Thu, 22 Jun 2023 22:32:27 +0800
> > > > Kim Seer Paller <kimseer.paller@analog.com> wrote:  
> 
> ...
> 
> > > > > + /*
> > > > > +  * Convert transmit buffer to big-endian format and reverse transmit
> > > > > +  * buffer to align with the LSB-first input on SDI port.
> > > > > +  */
> > > > > + st->spi_tx_buffer =  
> > > > cpu_to_be16(bitrev16(FIELD_PREP(MAX14001_ADDR_MASK,  
> > > > > +                                                         reg_addr)));
> > > > > +
> > > > > + ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
> > > > > + if (ret)
> > > > > +         return ret;
> > > > > +
> > > > > + /*
> > > > > +  * Align received data from the receive buffer, reversing and reordering
> > > > > +  * it to match the expected MSB-first format.
> > > > > +  */
> > > > > + *data = (__force u16)(be16_to_cpu(bitrev16(st->spi_rx_buffer))) &
> > > > > +  
> > > >     MAX14001_DATA_MASK;  
> > > > > +  
> > > > These sequences still confuse me a lot because I'd expect the values in tx
> > > > to have the opposite operations applied to those for rx and that's not the
> > > > case.
> > > >
> > > > Let's take a le system.
> > > > tx = cpu_to_be16(bitrev16(x))
> > > >    = cpu_to_be16((__bitrev8(x & 0xff) << 8) | __bitrev8(x >> 8));
> > > >    = __bitrev8(x & 0xff) | (__bitrev8(x >> 8) << 8)
> > > > or swap all the bits in each byte, but don't swap the bytes.
> > > >
> > > > rx = cpu_to_be16(bitrev16(x))
> > > >    = be16_to_cpu(((__bitrev8(x & 0xff) << 8) | __bitrev8(x >> 8)_
> > > >    = __bitrev8(x & 0xff) | __bitrev(x >> 8)
> > > >
> > > > also swap all the bits in each byte, but don't swap the bytes.
> > > >
> > > > So it is the reverse because the bytes swaps unwind themselves somewhat.
> > > > For a be system cpu_to_be16 etc are noop.
> > > > tx = (__bitrev8(x & 0xff) << 8) | __bitrev8(x >> 8)
> > > > rx = (__bitrev8(x & 0xff) << 8) | __bitrev8(x >> 8)
> > > >
> > > > So in this case swap all 16 bits.
> > > >
> > > > Now, given I'd expected them to be reversed for the tx vs rx case.
> > > > E.g.
> > > > tx = cpu_to_be16(bitrev16(x))
> > > > As above.
> > > > For rx, le host
> > > > rx = bitrev16(be16_to_cpu(x))
> > > >    = __bitrev8((x >> 8) & 0xff) << 8) |  __bitrev8((((x & 0xff) << 8) >> 8)
> > > > same as above (if you swap the two terms I think.
> > > >
> > > > For be the be16_to_cpu is a noop again, so it's just bitrev16(x) as expected.
> > > >
> > > > Hence if I've understood this correctly you could reverse the terms so that
> > > > it was 'obvious' you were doing the opposite for the tx term vs the rx one
> > > > without making the slightest bit of difference....
> > > >
> > > > hmm. Might be worth doing simply to avoid questions.  
> > >
> > > Thank you for your feedback. I have tested the modifications based on your
> > > suggestions, taking the le system into account, and it appears that the code is
> > > functioning correctly. Before sending the new patch version, I would like to
> > > confirm if this aligns with your comments.  
> 
> > Yes. This looks good to me.  
> 
> I think the implementation is still incorrect. See below.
> 
> > > 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 = cpu_to_be16(bitrev16(FIELD_PREP(MAX14001_ADDR_MASK, reg_addr)));  
> 
> Here we got bits in CPU order, reversed them and converted to BE16.
> 
> > >       ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
> > >       if (ret)
> > >               return ret;  
> 
> > >       *data = cpu_to_be16(bitrev16(st->spi_rx_buffer));  
> 
> Here we take __be16 response, reverse them and convert to BE16?!
> This is weird. You should have be16_to_cpu() somewhere, not the opposite.
Good point - though functionally they end up the same (and the bitrev
is making mess of type markings anyway). It is more logical
to ensure the direction is reversed as you suggest.

> 
> > >       return 0;
> > > }  
> 
> Isn't, btw, the reinvented spi_...write_then_read() (or what is it
> called?) call?

I think the cs_change = 1 prevents using that.

Jonathan


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

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

[Andy Shevchenko]

On Wed, Jul 5, 2023 at 12:28 PM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
> On Wed, 5 Jul 2023 11:53:17 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Wed, Jul 5, 2023 at 10:55 AM Jonathan Cameron
> > <Jonathan.Cameron@huawei.com> wrote:
> > > > > From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> > > > > Sent: Sunday, July 2, 2023 6:04 PM
> > > > > On Thu, 22 Jun 2023 22:32:27 +0800
> > > > > Kim Seer Paller <kimseer.paller@analog.com> wrote:

...

> > > > > > + /*
> > > > > > +  * Convert transmit buffer to big-endian format and reverse transmit
> > > > > > +  * buffer to align with the LSB-first input on SDI port.
> > > > > > +  */
> > > > > > + st->spi_tx_buffer =
> > > > > cpu_to_be16(bitrev16(FIELD_PREP(MAX14001_ADDR_MASK,
> > > > > > +                                                         reg_addr)));
> > > > > > +
> > > > > > + ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
> > > > > > + if (ret)
> > > > > > +         return ret;
> > > > > > +
> > > > > > + /*
> > > > > > +  * Align received data from the receive buffer, reversing and reordering
> > > > > > +  * it to match the expected MSB-first format.
> > > > > > +  */
> > > > > > + *data = (__force u16)(be16_to_cpu(bitrev16(st->spi_rx_buffer))) &
> > > > > > +
> > > > >     MAX14001_DATA_MASK;
> > > > > > +
> > > > > These sequences still confuse me a lot because I'd expect the values in tx
> > > > > to have the opposite operations applied to those for rx and that's not the
> > > > > case.
> > > > >
> > > > > Let's take a le system.
> > > > > tx = cpu_to_be16(bitrev16(x))
> > > > >    = cpu_to_be16((__bitrev8(x & 0xff) << 8) | __bitrev8(x >> 8));
> > > > >    = __bitrev8(x & 0xff) | (__bitrev8(x >> 8) << 8)
> > > > > or swap all the bits in each byte, but don't swap the bytes.
> > > > >
> > > > > rx = cpu_to_be16(bitrev16(x))
> > > > >    = be16_to_cpu(((__bitrev8(x & 0xff) << 8) | __bitrev8(x >> 8)_
> > > > >    = __bitrev8(x & 0xff) | __bitrev(x >> 8)
> > > > >
> > > > > also swap all the bits in each byte, but don't swap the bytes.
> > > > >
> > > > > So it is the reverse because the bytes swaps unwind themselves somewhat.
> > > > > For a be system cpu_to_be16 etc are noop.
> > > > > tx = (__bitrev8(x & 0xff) << 8) | __bitrev8(x >> 8)
> > > > > rx = (__bitrev8(x & 0xff) << 8) | __bitrev8(x >> 8)
> > > > >
> > > > > So in this case swap all 16 bits.
> > > > >
> > > > > Now, given I'd expected them to be reversed for the tx vs rx case.
> > > > > E.g.
> > > > > tx = cpu_to_be16(bitrev16(x))
> > > > > As above.
> > > > > For rx, le host
> > > > > rx = bitrev16(be16_to_cpu(x))
> > > > >    = __bitrev8((x >> 8) & 0xff) << 8) |  __bitrev8((((x & 0xff) << 8) >> 8)
> > > > > same as above (if you swap the two terms I think.
> > > > >
> > > > > For be the be16_to_cpu is a noop again, so it's just bitrev16(x) as expected.
> > > > >
> > > > > Hence if I've understood this correctly you could reverse the terms so that
> > > > > it was 'obvious' you were doing the opposite for the tx term vs the rx one
> > > > > without making the slightest bit of difference....
> > > > >
> > > > > hmm. Might be worth doing simply to avoid questions.
> > > >
> > > > Thank you for your feedback. I have tested the modifications based on your
> > > > suggestions, taking the le system into account, and it appears that the code is
> > > > functioning correctly. Before sending the new patch version, I would like to
> > > > confirm if this aligns with your comments.
> >
> > > Yes. This looks good to me.
> >
> > I think the implementation is still incorrect. See below.
> >
> > > > 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 = cpu_to_be16(bitrev16(FIELD_PREP(MAX14001_ADDR_MASK, reg_addr)));
> >
> > Here we got bits in CPU order, reversed them and converted to BE16.
> >
> > > >       ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
> > > >       if (ret)
> > > >               return ret;
> >
> > > >       *data = cpu_to_be16(bitrev16(st->spi_rx_buffer));
> >
> > Here we take __be16 response, reverse them and convert to BE16?!
> > This is weird. You should have be16_to_cpu() somewhere, not the opposite.
> Good point - though functionally they end up the same (and the bitrev
> is making mess of type markings anyway). It is more logical
> to ensure the direction is reversed as you suggest.

Also a question why we don't do that in reversed order.
Logically it sounds like bitrev16(be16_to_cpu()) should be.
Will it give the wrong results?

All in all this algo should be described in the comment in the code
(if not yet).

> > > >       return 0;
> > > > }

-- 
With Best Regards,
Andy Shevchenko

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

[Jonathan Cameron]

On Wed, 5 Jul 2023 12:36:40 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Wed, Jul 5, 2023 at 12:28 PM Jonathan Cameron
> <Jonathan.Cameron@huawei.com> wrote:
> > On Wed, 5 Jul 2023 11:53:17 +0300
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:  
> > > On Wed, Jul 5, 2023 at 10:55 AM Jonathan Cameron
> > > <Jonathan.Cameron@huawei.com> wrote:  
> > > > > > From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> > > > > > Sent: Sunday, July 2, 2023 6:04 PM
> > > > > > On Thu, 22 Jun 2023 22:32:27 +0800
> > > > > > Kim Seer Paller <kimseer.paller@analog.com> wrote:  
> 
> ...
> 
> > > > > > > + /*
> > > > > > > +  * Convert transmit buffer to big-endian format and reverse transmit
> > > > > > > +  * buffer to align with the LSB-first input on SDI port.
> > > > > > > +  */
> > > > > > > + st->spi_tx_buffer =  
> > > > > > cpu_to_be16(bitrev16(FIELD_PREP(MAX14001_ADDR_MASK,  
> > > > > > > +                                                         reg_addr)));
> > > > > > > +
> > > > > > > + ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
> > > > > > > + if (ret)
> > > > > > > +         return ret;
> > > > > > > +
> > > > > > > + /*
> > > > > > > +  * Align received data from the receive buffer, reversing and reordering
> > > > > > > +  * it to match the expected MSB-first format.
> > > > > > > +  */
> > > > > > > + *data = (__force u16)(be16_to_cpu(bitrev16(st->spi_rx_buffer))) &
> > > > > > > +  
> > > > > >     MAX14001_DATA_MASK;  
> > > > > > > +  
> > > > > > These sequences still confuse me a lot because I'd expect the values in tx
> > > > > > to have the opposite operations applied to those for rx and that's not the
> > > > > > case.
> > > > > >
> > > > > > Let's take a le system.
> > > > > > tx = cpu_to_be16(bitrev16(x))
> > > > > >    = cpu_to_be16((__bitrev8(x & 0xff) << 8) | __bitrev8(x >> 8));
> > > > > >    = __bitrev8(x & 0xff) | (__bitrev8(x >> 8) << 8)
> > > > > > or swap all the bits in each byte, but don't swap the bytes.
> > > > > >
> > > > > > rx = cpu_to_be16(bitrev16(x))
> > > > > >    = be16_to_cpu(((__bitrev8(x & 0xff) << 8) | __bitrev8(x >> 8)_
> > > > > >    = __bitrev8(x & 0xff) | __bitrev(x >> 8)
> > > > > >
> > > > > > also swap all the bits in each byte, but don't swap the bytes.
> > > > > >
> > > > > > So it is the reverse because the bytes swaps unwind themselves somewhat.
> > > > > > For a be system cpu_to_be16 etc are noop.
> > > > > > tx = (__bitrev8(x & 0xff) << 8) | __bitrev8(x >> 8)
> > > > > > rx = (__bitrev8(x & 0xff) << 8) | __bitrev8(x >> 8)
> > > > > >
> > > > > > So in this case swap all 16 bits.
> > > > > >
> > > > > > Now, given I'd expected them to be reversed for the tx vs rx case.
> > > > > > E.g.
> > > > > > tx = cpu_to_be16(bitrev16(x))
> > > > > > As above.
> > > > > > For rx, le host
> > > > > > rx = bitrev16(be16_to_cpu(x))
> > > > > >    = __bitrev8((x >> 8) & 0xff) << 8) |  __bitrev8((((x & 0xff) << 8) >> 8)
> > > > > > same as above (if you swap the two terms I think.
> > > > > >
> > > > > > For be the be16_to_cpu is a noop again, so it's just bitrev16(x) as expected.
> > > > > >
> > > > > > Hence if I've understood this correctly you could reverse the terms so that
> > > > > > it was 'obvious' you were doing the opposite for the tx term vs the rx one
> > > > > > without making the slightest bit of difference....
> > > > > >
> > > > > > hmm. Might be worth doing simply to avoid questions.  
> > > > >
> > > > > Thank you for your feedback. I have tested the modifications based on your
> > > > > suggestions, taking the le system into account, and it appears that the code is
> > > > > functioning correctly. Before sending the new patch version, I would like to
> > > > > confirm if this aligns with your comments.  
> > >  
> > > > Yes. This looks good to me.  
> > >
> > > I think the implementation is still incorrect. See below.
> > >  
> > > > > 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 = cpu_to_be16(bitrev16(FIELD_PREP(MAX14001_ADDR_MASK, reg_addr)));  
> > >
> > > Here we got bits in CPU order, reversed them and converted to BE16.
> > >  
> > > > >       ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
> > > > >       if (ret)
> > > > >               return ret;  
> > >  
> > > > >       *data = cpu_to_be16(bitrev16(st->spi_rx_buffer));  
> > >
> > > Here we take __be16 response, reverse them and convert to BE16?!
> > > This is weird. You should have be16_to_cpu() somewhere, not the opposite.  
> > Good point - though functionally they end up the same (and the bitrev
> > is making mess of type markings anyway). It is more logical
> > to ensure the direction is reversed as you suggest.  
> 
> Also a question why we don't do that in reversed order.
> Logically it sounds like bitrev16(be16_to_cpu()) should be.
> Will it give the wrong results?
Shouldn't make any difference as the two operations commute.
I'd missed this.  You are right that the other order makes more
sense.

Jonathan

> 
> All in all this algo should be described in the comment in the code
> (if not yet).
> 
> > > > >       return 0;
> > > > > }  
>