[PATCH v2 0/2] Add max14001 support

[PATCH v2 0/2] Add max14001 support

[Kim Seer Paller]

It took more time than I expected, but I have followed up on this and now
I'm presenting the patch for your review.

Modified the code based on the implementation of this application note
as a reference:
https://www.analog.com/media/en/technical-documentation/user-guides/guide-to-programming-the-max14001max14002-isolated-adcs--maxim-integrated.pdf
For now, I'm figuring out how to implement daisy chain for future support.

Changes in v2:

 max14001:
  - Replaced verification register definitions with macro.
  - Removed local definition of IIO_DMA_MINALIGN.
  - Changed driver to big endian format and reverse data prior to outputting
  it to the device as per the app note.
  - Utilized default register values instead of zero to clear memory validation
  fault.
  - Converted driver to use custom regmap for register access.
  - Modified scaled value calculation and changed to IIO_VAL_FRACTIONAL_LOG2.
  - Removed offset attribute.
  - Added comments for clarity in write transactions.
  - Misc changes(blank lines...).

 Bindings:
  - Removed unneccessary device tree propertied and updated bindings.

The code was tested using max14001pmb and max14001evsys boards with
raspberry pi 4.

Kim Seer Paller (2):
  dt-bindings:iio:adc: add max14001
  iio: adc: max14001: New driver

 .../bindings/iio/adc/adi,max14001.yaml        |  55 +++
 MAINTAINERS                                   |   8 +
 drivers/iio/adc/Kconfig                       |  10 +
 drivers/iio/adc/Makefile                      |   1 +
 drivers/iio/adc/max14001.c                    | 333 ++++++++++++++++++
 5 files changed, 407 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,max14001.yaml
 create mode 100644 drivers/iio/adc/max14001.c


base-commit: 44149752e9987a9eac5ad78e6d3a20934b5e018d
-- 
2.34.1

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

[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>
---
 .../bindings/iio/adc/adi,max14001.yaml        | 55 +++++++++++++++++++
 MAINTAINERS                                   |  7 +++
 2 files changed, 62 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 000000000..1b17f5dc0
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/adi,max14001.yaml
@@ -0,0 +1,55 @@
+# 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.
+
+allOf:
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+required:
+  - compatible
+  - reg
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        status = "okay";
+
+        adc@0 {
+            compatible = "adi,max14001";
+            reg = <0>;
+            spi-max-frequency = <5000000>;
+            vref-supply = <&vref_reg>;
+        };
+    };
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index 0e64787aa..766847ad2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12573,6 +12573,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/dac/adi,max14001.yaml
+
 MAXBOTIX ULTRASONIC RANGER IIO DRIVER
 M:	Andreas Klinger <ak@it-klinger.de>
 L:	linux-iio@vger.kernel.org
-- 
2.34.1

[PATCH v2 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>
---
 MAINTAINERS                |   1 +
 drivers/iio/adc/Kconfig    |  10 ++
 drivers/iio/adc/Makefile   |   1 +
 drivers/iio/adc/max14001.c | 333 +++++++++++++++++++++++++++++++++++++
 4 files changed, 345 insertions(+)
 create mode 100644 drivers/iio/adc/max14001.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 766847ad2..16b74c072 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12579,6 +12579,7 @@ L:	linux-iio@vger.kernel.org
 S:	Supported
 W:	https://ez.analog.com/linux-software-drivers
 F:	Documentation/devicetree/bindings/iio/dac/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 45af2302b..2e5137471 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 36c181773..016064727 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 000000000..7c5272756
--- /dev/null
+++ b/drivers/iio/adc/max14001.c
@@ -0,0 +1,333 @@
+// 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/regmap.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 agains multiple concurrent accesses */
+	struct mutex		lock;
+	struct regmap		*regmap;
+	int			vref_mv;
+	/*
+	 * DMA (thus cache coherency maintenance) requires the
+	 * transfer buffers to live in their own cache lines.
+	 */
+	__be16			spi_tx_buffer ____cacheline_aligned;
+	__be16			spi_rx_buffer;
+};
+
+static int max14001_read(void *context, unsigned int reg_addr,
+					unsigned int *data)
+{
+	struct max14001_state *st = context;
+	u16 tx = 0;
+	int ret;
+
+	struct spi_transfer xfers[] = {
+		{
+			.tx_buf = &st->spi_tx_buffer,
+			.len = 2,
+			.cs_change = 1,
+		}, {
+			.rx_buf = &st->spi_rx_buffer,
+			.len = 2,
+		},
+	};
+
+	tx = FIELD_PREP(MAX14001_ADDR_MASK, reg_addr);
+	st->spi_tx_buffer = bitrev16(cpu_to_be16(tx));
+
+	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;
+	u16 tx = 0;
+
+	tx = FIELD_PREP(MAX14001_ADDR_MASK, reg_addr);
+	tx |= FIELD_PREP(MAX14001_SET_WRITE_BIT, 1);
+	tx |= FIELD_PREP(MAX14001_DATA_MASK, data);
+
+	st->spi_tx_buffer = bitrev16(cpu_to_be16(tx));
+
+	return spi_write(st->spi, &st->spi_tx_buffer, 2);
+}
+
+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;
+
+	/* Enable SPI Registers Write */
+	ret = max14001_write(st, MAX14001_WEN, MAX14001_WRITE_WEN);
+	if (ret)
+		return ret;
+
+	ret = regmap_update_bits(st->regmap, reg_addr, mask, val);
+	if (ret)
+		return ret;
+
+	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 regmap_config max14001_regmap_config = {
+	.reg_read = max14001_read,
+	.reg_write = max14001_write,
+};
+
+static const struct iio_info max14001_info = {
+	.read_raw = max14001_read_raw,
+};
+
+static const struct iio_chan_spec max14001_channels[] = {
+	{
+		.type = IIO_VOLTAGE,
+		.channel = 0,
+		.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;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	st = iio_priv(indio_dev);
+	st->spi = spi;
+
+	st->regmap = devm_regmap_init(&spi->dev, NULL, st,
+				      &max14001_regmap_config);
+
+	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(&spi->dev, "vref");
+	if (IS_ERR(vref)) {
+		if (PTR_ERR(vref) != -ENODEV)
+			return dev_err_probe(&spi->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(&spi->dev, ret,
+					"Failed to enable vref regulators\n");
+
+		ret = devm_add_action_or_reset(&spi->dev,
+					       max14001_regulator_disable,
+					       vref);
+		if (ret)
+			return ret;
+
+		/* enable external voltage reference */
+		ret = max14001_reg_update(st, MAX14001_CFG,
+					  MAX14001_CFG_EXRF, 1);
+
+		ret = regulator_get_voltage(vref);
+		if (ret < 0)
+			return dev_err_probe(&spi->dev, ret,
+					     "Failed to get vref\n");
+
+		st->vref_mv = ret / 1000;
+	}
+
+	mutex_init(&st->lock);
+
+	return devm_iio_device_register(&spi->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 v2 1/2] dt-bindings:iio:adc: add max14001

[Krzysztof Kozlowski]

On 05/06/2023 15:07, Kim Seer Paller wrote:
> The MAX14001 is configurable, isolated 10-bit ADCs for multi-range
> binary inputs.
> 
> Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>

Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC.  It might happen, that command when run on an older
kernel, gives you outdated entries.  Therefore please be sure you base
your patches on recent Linux kernel.

You missed at least DT list (maybe more), so this won't be tested.
Please resend and include all necessary entries.

Subject - ignored comments.

This is a friendly reminder during the review process.

It seems my previous comments were not fully addressed. Maybe my
feedback got lost between the quotes, maybe you just forgot to apply it.
Please go back to the previous discussion and either implement all
requested changes or keep discussing them.

Thank you.

> ---
>  .../bindings/iio/adc/adi,max14001.yaml        | 55 +++++++++++++++++++
>  MAINTAINERS                                   |  7 +++
>  2 files changed, 62 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 000000000..1b17f5dc0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,max14001.yaml
> @@ -0,0 +1,55 @@
> +# 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.
> +
> +allOf:
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#

Place it like other bindings, so after required or before properties.

Anyway, what happened with all the properties you had here and should be
switched to generic ones?

> +
> +required:
> +  - compatible
> +  - reg
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    spi {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        status = "okay";

Really... You did not respond to my feedback, so sending uncorrected
version feels like being ignored. :(

Best regards,
Krzysztof

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

[Jonathan Cameron]

On Mon, 5 Jun 2023 21:07:55 +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>
> ---
...

Hi Kim,

A few comments inline.

> diff --git a/drivers/iio/adc/max14001.c b/drivers/iio/adc/max14001.c
> new file mode 100644
> index 000000000..7c5272756
> --- /dev/null
> +++ b/drivers/iio/adc/max14001.c
> @@ -0,0 +1,333 @@
> +// 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/regmap.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 agains multiple concurrent accesses */

To what?  Here I suspect it's RMW sequence on device and perhaps
more importantly the buffers below.

> +	struct mutex		lock;
> +	struct regmap		*regmap;
> +	int			vref_mv;
> +	/*
> +	 * DMA (thus cache coherency maintenance) requires the
> +	 * transfer buffers to live in their own cache lines.

You are looking at an old kernel I guess - we fixed all of these - and
introduced IIO_DMA_MINALIGN for __aligned(IIO_DMA_MINALIGN) to make
it easier to fix any such problems in future.

Upshot is that ___cacheline_aligned aligns to the l1 cacheline length.
Some fun systems (such as the big servers I use in my dayjob) have higher
cacheline sizes for their larger / further from CPU caches.
One group of SoCs out there is known to both do non coherent DMA and have
a larger line size for the bit relevant to that than ___cacheline_aligned
gives you. So on that rare platform this is currently broken.
 
> +	 */
> +	__be16			spi_tx_buffer ____cacheline_aligned;
> +	__be16			spi_rx_buffer;
> +};
> +
> +static int max14001_read(void *context, unsigned int reg_addr,
> +					unsigned int *data)
> +{
> +	struct max14001_state *st = context;
> +	u16 tx = 0;
> +	int ret;
> +
> +	struct spi_transfer xfers[] = {
> +		{
> +			.tx_buf = &st->spi_tx_buffer,
> +			.len = 2,
> +			.cs_change = 1,
> +		}, {
> +			.rx_buf = &st->spi_rx_buffer,
> +			.len = 2,
> +		},
> +	};
> +
> +	tx = FIELD_PREP(MAX14001_ADDR_MASK, reg_addr);
> +	st->spi_tx_buffer = bitrev16(cpu_to_be16(tx));
> +
> +	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;
> +	u16 tx = 0;
> +
> +	tx = FIELD_PREP(MAX14001_ADDR_MASK, reg_addr);
> +	tx |= FIELD_PREP(MAX14001_SET_WRITE_BIT, 1);
> +	tx |= FIELD_PREP(MAX14001_DATA_MASK, data);
> +
> +	st->spi_tx_buffer = bitrev16(cpu_to_be16(tx));
> +
> +	return spi_write(st->spi, &st->spi_tx_buffer, 2);
> +}
> +
> +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);

Even though this is a bit unusual, I'd still expect this to use
the regmap_read / regmap_write interfaces not directly use the callbacks.

> +}
> +
> +static int max14001_reg_update(struct max14001_state *st,
> +				unsigned int reg_addr,
> +				unsigned int mask,
> +				unsigned int val)
> +{
> +	int ret;
> +
> +	/* Enable SPI Registers Write */
> +	ret = max14001_write(st, MAX14001_WEN, MAX14001_WRITE_WEN);

Mixing regmap and non regmap rather defeats the point of
having a standard interface.  Use regmap_read and regmap_write
throughout or not at all.

> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(st->regmap, reg_addr, mask, val);
> +	if (ret)
> +		return ret;
> +
> +	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 regmap_config max14001_regmap_config = {
> +	.reg_read = max14001_read,
> +	.reg_write = max14001_write,

I'd keep this up by the callbacks, so all the regmap setup stuff
is in one place.

> +};
> +
> +static const struct iio_info max14001_info = {
> +	.read_raw = max14001_read_raw,
> +};
> +

...

> +static int max14001_probe(struct spi_device *spi)
> +{

...

> +
> +	vref = devm_regulator_get_optional(&spi->dev, "vref");
> +	if (IS_ERR(vref)) {
> +		if (PTR_ERR(vref) != -ENODEV)
> +			return dev_err_probe(&spi->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(&spi->dev, ret,
> +					"Failed to enable vref regulators\n");
> +
> +		ret = devm_add_action_or_reset(&spi->dev,
> +					       max14001_regulator_disable,
> +					       vref);
> +		if (ret)
> +			return ret;
> +
> +		/* enable external voltage reference */

use external voltage reference?

It's enabled by the regulator_enable() above, not this line.

> +		ret = max14001_reg_update(st, MAX14001_CFG,
> +					  MAX14001_CFG_EXRF, 1);
> +
> +		ret = regulator_get_voltage(vref);
> +		if (ret < 0)
> +			return dev_err_probe(&spi->dev, ret,
> +					     "Failed to get vref\n");
> +
> +		st->vref_mv = ret / 1000;
> +	}
> +
> +	mutex_init(&st->lock);
> +
> +	return devm_iio_device_register(&spi->dev, indio_dev);
> +}

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

[andy.shevchenko]

Mon, Jun 05, 2023 at 09:07:55PM +0800, Kim Seer Paller kirjoitti:
> The MAX14001 is configurable, isolated 10-bit ADCs for multi-range
> binary inputs.

First question, why don't you use regmap SPI?

...

> +static int max14001_read(void *context, unsigned int reg_addr,
> +					unsigned int *data)

Strange indentation.

> +{
> +	struct max14001_state *st = context;
> +	u16 tx = 0;

Redundant assignment.

> +	int ret;
> +
> +	struct spi_transfer xfers[] = {
> +		{
> +			.tx_buf = &st->spi_tx_buffer,
> +			.len = 2,
> +			.cs_change = 1,
> +		}, {
> +			.rx_buf = &st->spi_rx_buffer,
> +			.len = 2,
> +		},
> +	};
> +
> +	tx = FIELD_PREP(MAX14001_ADDR_MASK, reg_addr);

> +	st->spi_tx_buffer = bitrev16(cpu_to_be16(tx));

This is strange. Why this monsteur construct with bitrev16() is used?

According to the datasheet, the bit stream is LE16, where 10 LSB is data,
5 MSB is address and bit 11 is R/W.

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

Ditto.

> +	return 0;
> +}

> +static int max14001_write(void *context, unsigned int reg_addr,
> +					unsigned int data)
> +{
> +	struct max14001_state *st = context;
> +	u16 tx = 0;

Redundant assignment.

> +	tx = FIELD_PREP(MAX14001_ADDR_MASK, reg_addr);
> +	tx |= FIELD_PREP(MAX14001_SET_WRITE_BIT, 1);
> +	tx |= FIELD_PREP(MAX14001_DATA_MASK, data);
> +
> +	st->spi_tx_buffer = bitrev16(cpu_to_be16(tx));
> +
> +	return spi_write(st->spi, &st->spi_tx_buffer, 2);

sizeof() ?

> +}

...

> +			return dev_err_probe(&spi->dev, PTR_ERR(vref),
> +					     "Failed to get vref regulator");

With

	struct device *dev = &spi->dev;

this and other calls in this function can be made neater.

-- 
With Best Regards,
Andy Shevchenko

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

[Paller, Kim Seer]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Monday, June 5, 2023 9:30 PM
> To: Paller, Kim Seer <KimSeer.Paller@analog.com>; jic23@kernel.org;
> lars@metafoo.de
> Cc: broonie@kernel.org; lgirdwood@gmail.com; linux-iio@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v2 1/2] dt-bindings:iio:adc: add max14001
> 
> [External]
> 
> On 05/06/2023 15:07, Kim Seer Paller wrote:
> > The MAX14001 is configurable, isolated 10-bit ADCs for multi-range
> > binary inputs.
> >
> > Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
> 
> Please use scripts/get_maintainers.pl to get a list of necessary people and
> lists to CC.  It might happen, that command when run on an older kernel,
> gives you outdated entries.  Therefore please be sure you base your patches
> on recent Linux kernel.
> 
> You missed at least DT list (maybe more), so this won't be tested.
> Please resend and include all necessary entries.
> 
> Subject - ignored comments.
> 
> This is a friendly reminder during the review process.
> 
> It seems my previous comments were not fully addressed. Maybe my
> feedback got lost between the quotes, maybe you just forgot to apply it.
> Please go back to the previous discussion and either implement all requested
> changes or keep discussing them.

Thank you for your input. I appreciate your feedback, and I apologize for not 
addressing all your previous comments. It seems I may have missed them.

> 
> Thank you.
> 
> > ---
> >  .../bindings/iio/adc/adi,max14001.yaml        | 55 +++++++++++++++++++
> >  MAINTAINERS                                   |  7 +++
> >  2 files changed, 62 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 000000000..1b17f5dc0
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/adi,max14001.yaml
> > @@ -0,0 +1,55 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) # Copyright 2023
> > +Analog Devices Inc.
> > +%YAML 1.2
> > +---
> > +$id:
> > +https://urldefense.com/v3/__http://devicetree.org/schemas/iio/adc/adi
> >
> +,max14001.yaml*__;Iw!!A3Ni8CS0y2Y!4kyf116cWnmdDQYfS_6HwdLqnsCd3
> mGIDAG
> > +uHyornecB2wjo6Yv3S6YD88DRCVplVQhyOVYNvhfSdA-
> gyquDGZpeBZP25Wg$
> > +$schema:
> > +https://urldefense.com/v3/__http://devicetree.org/meta-
> schemas/core.y
> >
> +aml*__;Iw!!A3Ni8CS0y2Y!4kyf116cWnmdDQYfS_6HwdLqnsCd3mGIDAGuH
> yornecB2w
> > +jo6Yv3S6YD88DRCVplVQhyOVYNvhfSdA-gyquDGZpe_rIl2_c$
> > +
> > +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/M
> > +AX14001-MAX14002.pdf
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - adi,max14001
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  spi-max-frequency:
> > +    maximum: 5000000
> > +
> > +  vref-supply:
> > +    description: Voltage reference to establish input scaling.
> > +
> > +allOf:
> > +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> 
> Place it like other bindings, so after required or before properties.
> 
> Anyway, what happened with all the properties you had here and should be
> switched to generic ones?

I have decided to remove the properties and utilize the default register values 
during initialization to clear memory validation faults, which I believe is a 
better approach. I am not yet familiar with how to implement some of the 
properties to switch to the userspace ABI, but for now, is it okay to exclude it 
and plan to implement it for future support? 

> 
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +    spi {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +        status = "okay";
> 
> Really... You did not respond to my feedback, so sending uncorrected version
> feels like being ignored. :(

I sincerely apologize, it was an oversight on my part and I didn't mean to 
ignore it, and I understand if it caused any problems. Moving forward, I will ensure 
to thoroughly review and address all feedback provided.

> 
> Best regards,
> Krzysztof

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

[Paller, Kim Seer]

> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Tuesday, June 6, 2023 3:24 AM
> To: Paller, Kim Seer <KimSeer.Paller@analog.com>
> Cc: lars@metafoo.de; krzysztof.kozlowski@linaro.org; broonie@kernel.org;
> lgirdwood@gmail.com; linux-iio@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH v2 2/2] iio: adc: max14001: New driver
> 
> [External]
> 
> On Mon, 5 Jun 2023 21:07:55 +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>
> > ---
> ...
> 
> Hi Kim,
> 
> A few comments inline.
> 
> > diff --git a/drivers/iio/adc/max14001.c b/drivers/iio/adc/max14001.c
> > new file mode 100644 index 000000000..7c5272756
> > --- /dev/null
> > +++ b/drivers/iio/adc/max14001.c
> > @@ -0,0 +1,333 @@
> > +// 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/regmap.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 agains multiple concurrent accesses */
> 
> To what?  Here I suspect it's RMW sequence on device and perhaps more
> importantly the buffers below.
> 
> > +	struct mutex		lock;
> > +	struct regmap		*regmap;
> > +	int			vref_mv;
> > +	/*
> > +	 * DMA (thus cache coherency maintenance) requires the
> > +	 * transfer buffers to live in their own cache lines.
> 
> You are looking at an old kernel I guess - we fixed all of these - and
> introduced IIO_DMA_MINALIGN for __aligned(IIO_DMA_MINALIGN) to
> make it easier to fix any such problems in future.
> 
> Upshot is that ___cacheline_aligned aligns to the l1 cacheline length.
> Some fun systems (such as the big servers I use in my dayjob) have higher
> cacheline sizes for their larger / further from CPU caches.
> One group of SoCs out there is known to both do non coherent DMA and
> have a larger line size for the bit relevant to that than ___cacheline_aligned
> gives you. So on that rare platform this is currently broken.

It's good to know. Given this information, is there anything specific that I 
need to change in the code or implementation related to 
the ___cacheline_aligned part?
> 
> > +	 */
> > +	__be16			spi_tx_buffer ____cacheline_aligned;
> > +	__be16			spi_rx_buffer;
> > +};
> > +
> > +static int max14001_read(void *context, unsigned int reg_addr,
> > +					unsigned int *data)
> > +{
> > +	struct max14001_state *st = context;
> > +	u16 tx = 0;
> > +	int ret;
> > +
> > +	struct spi_transfer xfers[] = {
> > +		{
> > +			.tx_buf = &st->spi_tx_buffer,
> > +			.len = 2,
> > +			.cs_change = 1,
> > +		}, {
> > +			.rx_buf = &st->spi_rx_buffer,
> > +			.len = 2,
> > +		},
> > +	};
> > +
> > +	tx = FIELD_PREP(MAX14001_ADDR_MASK, reg_addr);
> > +	st->spi_tx_buffer = bitrev16(cpu_to_be16(tx));
> > +
> > +	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;
> > +	u16 tx = 0;
> > +
> > +	tx = FIELD_PREP(MAX14001_ADDR_MASK, reg_addr);
> > +	tx |= FIELD_PREP(MAX14001_SET_WRITE_BIT, 1);
> > +	tx |= FIELD_PREP(MAX14001_DATA_MASK, data);
> > +
> > +	st->spi_tx_buffer = bitrev16(cpu_to_be16(tx));
> > +
> > +	return spi_write(st->spi, &st->spi_tx_buffer, 2); }
> > +
> > +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);
> 
> Even though this is a bit unusual, I'd still expect this to use the regmap_read /
> regmap_write interfaces not directly use the callbacks.
> 
> > +}
> > +
> > +static int max14001_reg_update(struct max14001_state *st,
> > +				unsigned int reg_addr,
> > +				unsigned int mask,
> > +				unsigned int val)
> > +{
> > +	int ret;
> > +
> > +	/* Enable SPI Registers Write */
> > +	ret = max14001_write(st, MAX14001_WEN,
> MAX14001_WRITE_WEN);
> 
> Mixing regmap and non regmap rather defeats the point of having a standard
> interface.  Use regmap_read and regmap_write throughout or not at all.

I found it difficult to implement the regmap interface due to the timing diagram 
requirements. The chip select needs to be changed between transfers, which, 
as far as I know, does not work with regmap. Perhaps, I will consider sticking 
to the non-regmap approach.

> 
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_update_bits(st->regmap, reg_addr, mask, val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	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 regmap_config max14001_regmap_config = {
> > +	.reg_read = max14001_read,
> > +	.reg_write = max14001_write,
> 
> I'd keep this up by the callbacks, so all the regmap setup stuff is in one place.
> 
> > +};
> > +
> > +static const struct iio_info max14001_info = {
> > +	.read_raw = max14001_read_raw,
> > +};
> > +
> 
> ...
> 
> > +static int max14001_probe(struct spi_device *spi) {
> 
> ...
> 
> > +
> > +	vref = devm_regulator_get_optional(&spi->dev, "vref");
> > +	if (IS_ERR(vref)) {
> > +		if (PTR_ERR(vref) != -ENODEV)
> > +			return dev_err_probe(&spi->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(&spi->dev, ret,
> > +					"Failed to enable vref regulators\n");
> > +
> > +		ret = devm_add_action_or_reset(&spi->dev,
> > +					       max14001_regulator_disable,
> > +					       vref);
> > +		if (ret)
> > +			return ret;
> > +
> > +		/* enable external voltage reference */
> 
> use external voltage reference?
> 
> It's enabled by the regulator_enable() above, not this line.

What I meant was to enable the CFG register to utilize the external voltage 
source within the ADC. I've missed to specify the comment on this one.

> 
> > +		ret = max14001_reg_update(st, MAX14001_CFG,
> > +					  MAX14001_CFG_EXRF, 1);
> > +
> > +		ret = regulator_get_voltage(vref);
> > +		if (ret < 0)
> > +			return dev_err_probe(&spi->dev, ret,
> > +					     "Failed to get vref\n");
> > +
> > +		st->vref_mv = ret / 1000;
> > +	}
> > +
> > +	mutex_init(&st->lock);
> > +
> > +	return devm_iio_device_register(&spi->dev, indio_dev); }

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

[Paller, Kim Seer]

> -----Original Message-----
> From: andy.shevchenko@gmail.com <andy.shevchenko@gmail.com>
> Sent: Tuesday, June 6, 2023 8:49 AM
> To: Paller, Kim Seer <KimSeer.Paller@analog.com>
> Cc: jic23@kernel.org; lars@metafoo.de; krzysztof.kozlowski@linaro.org;
> broonie@kernel.org; lgirdwood@gmail.com; linux-iio@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH v2 2/2] iio: adc: max14001: New driver
> 
> [External]
> 
> Mon, Jun 05, 2023 at 09:07:55PM +0800, Kim Seer Paller kirjoitti:
> > The MAX14001 is configurable, isolated 10-bit ADCs for multi-range
> > binary inputs.
> 
> First question, why don't you use regmap SPI?

I found it difficult to implement the regmap interface due to the timing diagram 
requirements. The chip select needs to be changed between transfers, which, as 
far as I know, does not work with regmap. Additionally, the regmap_config 
parameters, specifically reg_bits and val_bits, cannot be set to specific values. 
This limitation makes it challenging to accommodate specific configurations 
such as using 5 bits for register address and 10 bits for data.

> 
> ...
> 
> > +static int max14001_read(void *context, unsigned int reg_addr,
> > +					unsigned int *data)
> 
> Strange indentation.
> 
> > +{
> > +	struct max14001_state *st = context;
> > +	u16 tx = 0;
> 
> Redundant assignment.
> 
> > +	int ret;
> > +
> > +	struct spi_transfer xfers[] = {
> > +		{
> > +			.tx_buf = &st->spi_tx_buffer,
> > +			.len = 2,
> > +			.cs_change = 1,
> > +		}, {
> > +			.rx_buf = &st->spi_rx_buffer,
> > +			.len = 2,
> > +		},
> > +	};
> > +
> > +	tx = FIELD_PREP(MAX14001_ADDR_MASK, reg_addr);
> 
> > +	st->spi_tx_buffer = bitrev16(cpu_to_be16(tx));
> 
> This is strange. Why this monsteur construct with bitrev16() is used?
> 
> According to the datasheet, the bit stream is LE16, where 10 LSB is data,
> 5 MSB is address and bit 11 is R/W.

I discovered that following the instructions in the application note made 
the implementation much easier. 
https://www.analog.com/media/en/technical-documentation/user-guides/guide-to-programming-the-max14001max14002-isolated-adcs--maxim-integrated.pdf

To ensure compatibility with the device, I changed the format to big 
endian and reversed the data before sending it to the device. 
This is why I used the "bitrev" function. I'm happy to report that 
this approach worked perfectly.

> 
> > +	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;
> 
> Ditto.
> 
> > +	return 0;
> > +}
> 
> > +static int max14001_write(void *context, unsigned int reg_addr,
> > +					unsigned int data)
> > +{
> > +	struct max14001_state *st = context;
> > +	u16 tx = 0;
> 
> Redundant assignment.
> 
> > +	tx = FIELD_PREP(MAX14001_ADDR_MASK, reg_addr);
> > +	tx |= FIELD_PREP(MAX14001_SET_WRITE_BIT, 1);
> > +	tx |= FIELD_PREP(MAX14001_DATA_MASK, data);
> > +
> > +	st->spi_tx_buffer = bitrev16(cpu_to_be16(tx));
> > +
> > +	return spi_write(st->spi, &st->spi_tx_buffer, 2);
> 
> sizeof() ?
> 
> > +}
> 
> ...
> 
> > +			return dev_err_probe(&spi->dev, PTR_ERR(vref),
> > +					     "Failed to get vref regulator");
> 
> With
> 
> 	struct device *dev = &spi->dev;
> 
> this and other calls in this function can be made neater.
> 
> --
> With Best Regards,
> Andy Shevchenko
> 

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

[Jonathan Cameron]

> >   
> > > +	struct mutex		lock;
> > > +	struct regmap		*regmap;
> > > +	int			vref_mv;
> > > +	/*
> > > +	 * DMA (thus cache coherency maintenance) requires the
> > > +	 * transfer buffers to live in their own cache lines.  
> > 
> > You are looking at an old kernel I guess - we fixed all of these - and
> > introduced IIO_DMA_MINALIGN for __aligned(IIO_DMA_MINALIGN) to
> > make it easier to fix any such problems in future.
> > 
> > Upshot is that ___cacheline_aligned aligns to the l1 cacheline length.
> > Some fun systems (such as the big servers I use in my dayjob) have higher
> > cacheline sizes for their larger / further from CPU caches.
> > One group of SoCs out there is known to both do non coherent DMA and
> > have a larger line size for the bit relevant to that than ___cacheline_aligned
> > gives you. So on that rare platform this is currently broken.  
> 
> It's good to know. Given this information, is there anything specific that I 
> need to change in the code or implementation related to 
> the ___cacheline_aligned part?

Replace it with __aligned(IIO_DMA_MINALIGN) as has hopefully now been done
in all upstream drivers.

> >   

> > > +}
> > > +
> > > +static int max14001_reg_update(struct max14001_state *st,
> > > +				unsigned int reg_addr,
> > > +				unsigned int mask,
> > > +				unsigned int val)
> > > +{
> > > +	int ret;
> > > +
> > > +	/* Enable SPI Registers Write */
> > > +	ret = max14001_write(st, MAX14001_WEN,  
> > MAX14001_WRITE_WEN);
> > 
> > Mixing regmap and non regmap rather defeats the point of having a standard
> > interface.  Use regmap_read and regmap_write throughout or not at all.  
> 
> I found it difficult to implement the regmap interface due to the timing diagram 
> requirements. The chip select needs to be changed between transfers, which, 
> as far as I know, does not work with regmap. Perhaps, I will consider sticking 
> to the non-regmap approach.

That may be sensible if there are odd requirements or just call regmap_write()
which will call your max14001_write() anyway and opencode the timing
requirements etc by multiple remap calls.  Obviously benefits of regmap reduced
though so may not be worth bothering unless it is worth using the caching or similar.

Jonathan

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

[Paller, Kim Seer]

> -----Original Message-----
> From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> Sent: Tuesday, June 6, 2023 6:36 PM
> To: Paller, Kim Seer <KimSeer.Paller@analog.com>
> Cc: Jonathan Cameron <jic23@kernel.org>; lars@metafoo.de;
> krzysztof.kozlowski@linaro.org; broonie@kernel.org; lgirdwood@gmail.com;
> linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v2 2/2] iio: adc: max14001: New driver
> 
> [External]
> 
> 
> 
> > >
> > > > +	struct mutex		lock;
> > > > +	struct regmap		*regmap;
> > > > +	int			vref_mv;
> > > > +	/*
> > > > +	 * DMA (thus cache coherency maintenance) requires the
> > > > +	 * transfer buffers to live in their own cache lines.
> > >
> > > You are looking at an old kernel I guess - we fixed all of these - and
> > > introduced IIO_DMA_MINALIGN for __aligned(IIO_DMA_MINALIGN) to
> > > make it easier to fix any such problems in future.
> > >
> > > Upshot is that ___cacheline_aligned aligns to the l1 cacheline length.
> > > Some fun systems (such as the big servers I use in my dayjob) have higher
> > > cacheline sizes for their larger / further from CPU caches.
> > > One group of SoCs out there is known to both do non coherent DMA and
> > > have a larger line size for the bit relevant to that than ___cacheline_aligned
> > > gives you. So on that rare platform this is currently broken.
> >
> > It's good to know. Given this information, is there anything specific that I
> > need to change in the code or implementation related to
> > the ___cacheline_aligned part?
> 
> Replace it with __aligned(IIO_DMA_MINALIGN) as has hopefully now been
> done
> in all upstream drivers.

When I attempted to implement this change, I encountered a checkpatch warning 
in the latest kernel version. The warning indicated that externs should be avoided 
in .c files and emphasized the need for an identifier name for the function 
definition argument 'IIO_DMA_MINALIGN'. I attempted to define a macro with an 
appropriate identifier name, but I still received the same checkpatch warning. 
It's possible that I may have overlooked something in my approach. I would 
appreciate your thoughts and insights on this matter. Thanks.

> 
> > >
> 
> > > > +}
> > > > +
> > > > +static int max14001_reg_update(struct max14001_state *st,
> > > > +				unsigned int reg_addr,
> > > > +				unsigned int mask,
> > > > +				unsigned int val)
> > > > +{
> > > > +	int ret;
> > > > +
> > > > +	/* Enable SPI Registers Write */
> > > > +	ret = max14001_write(st, MAX14001_WEN,
> > > MAX14001_WRITE_WEN);
> > >
> > > Mixing regmap and non regmap rather defeats the point of having a
> standard
> > > interface.  Use regmap_read and regmap_write throughout or not at all.
> >
> > I found it difficult to implement the regmap interface due to the timing
> diagram
> > requirements. The chip select needs to be changed between transfers, which,
> > as far as I know, does not work with regmap. Perhaps, I will consider sticking
> > to the non-regmap approach.
> 
> That may be sensible if there are odd requirements or just call regmap_write()
> which will call your max14001_write() anyway and opencode the timing
> requirements etc by multiple remap calls.  Obviously benefits of regmap
> reduced
> though so may not be worth bothering unless it is worth using the caching or
> similar.
> 
> Jonathan
> 
> 

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

[Jonathan Cameron]

On Wed, 7 Jun 2023 11:17:33 +0000
"Paller, Kim Seer" <KimSeer.Paller@analog.com> wrote:

> > -----Original Message-----
> > From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> > Sent: Tuesday, June 6, 2023 6:36 PM
> > To: Paller, Kim Seer <KimSeer.Paller@analog.com>
> > Cc: Jonathan Cameron <jic23@kernel.org>; lars@metafoo.de;
> > krzysztof.kozlowski@linaro.org; broonie@kernel.org; lgirdwood@gmail.com;
> > linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH v2 2/2] iio: adc: max14001: New driver
> > 
> > [External]
> > 
> > 
> >   
> > > >  
> > > > > +	struct mutex		lock;
> > > > > +	struct regmap		*regmap;
> > > > > +	int			vref_mv;
> > > > > +	/*
> > > > > +	 * DMA (thus cache coherency maintenance) requires the
> > > > > +	 * transfer buffers to live in their own cache lines.  
> > > >
> > > > You are looking at an old kernel I guess - we fixed all of these - and
> > > > introduced IIO_DMA_MINALIGN for __aligned(IIO_DMA_MINALIGN) to
> > > > make it easier to fix any such problems in future.
> > > >
> > > > Upshot is that ___cacheline_aligned aligns to the l1 cacheline length.
> > > > Some fun systems (such as the big servers I use in my dayjob) have higher
> > > > cacheline sizes for their larger / further from CPU caches.
> > > > One group of SoCs out there is known to both do non coherent DMA and
> > > > have a larger line size for the bit relevant to that than ___cacheline_aligned
> > > > gives you. So on that rare platform this is currently broken.  
> > >
> > > It's good to know. Given this information, is there anything specific that I
> > > need to change in the code or implementation related to
> > > the ___cacheline_aligned part?  
> > 
> > Replace it with __aligned(IIO_DMA_MINALIGN) as has hopefully now been
> > done
> > in all upstream drivers.  
> 
> When I attempted to implement this change, I encountered a checkpatch warning 
> in the latest kernel version. The warning indicated that externs should be avoided 
> in .c files and emphasized the need for an identifier name for the function 
> definition argument 'IIO_DMA_MINALIGN'. I attempted to define a macro with an 
> appropriate identifier name, but I still received the same checkpatch warning. 
> It's possible that I may have overlooked something in my approach. I would 
> appreciate your thoughts and insights on this matter. Thanks.

Ignore checkpatch for this one. Check patch is miss handling of
__aligned.

At a guess - needs an entry in this list:
https://elixir.bootlin.com/linux/latest/source/scripts/checkpatch.pl#L513

Though I'm never great at understanding the perl so may have that wrong.

Jonathan