[PATCH v2 0/2] Add maxim max34408/34409 ADC driver and yaml

[PATCH v2 0/2] Add maxim max34408/34409 ADC driver and yaml

[Ivan Mikhaylov]

Add Maxim max34408/34409 ADC driver and yaml for it. Until now it
supports only current monitioring function without overcurrent
threshold/delay, shutdown delay configuration, alert interrupt.

Changes from v1:
   - minor changes from Rob's comments for yaml
   - add ena, shtdn and make 4 inputs for R sense from Jonathan's comments for yaml
   - add _REG suffix and make prefix for bitmasks and statuses
   - add SCALE/OFFSET instead of AVG/PROCESSED from Jonathan and
     Lars-Peter comments
   - add chip data from Jonathan and Lars-Peter comments
   - minor changes from Lars-Peter and Jonathan comments for driver
  
Ivan Mikhaylov (2):
  dt-bindings: adc: provide max34408/9 device tree binding document
  iio: adc: Add driver support for MAX34408/9

 .../bindings/iio/adc/maxim,max34408.yaml      | 101 ++++++
 drivers/iio/adc/Kconfig                       |  11 +
 drivers/iio/adc/Makefile                      |   1 +
 drivers/iio/adc/max34408.c                    | 334 ++++++++++++++++++
 4 files changed, 447 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/maxim,max34408.yaml
 create mode 100644 drivers/iio/adc/max34408.c

-- 
2.42.0

[PATCH v2 1/2] dt-bindings: adc: provide max34408/9 device tree binding document

[Ivan Mikhaylov]

The hardware binding for i2c current monitoring device with overcurrent
control.

Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
---
 .../bindings/iio/adc/maxim,max34408.yaml      | 101 ++++++++++++++++++
 1 file changed, 101 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/maxim,max34408.yaml

diff --git a/Documentation/devicetree/bindings/iio/adc/maxim,max34408.yaml b/Documentation/devicetree/bindings/iio/adc/maxim,max34408.yaml
new file mode 100644
index 000000000000..cdf89fa4c80e
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/maxim,max34408.yaml
@@ -0,0 +1,101 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/maxim,max34408.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Two- and four-channel current monitors with overcurrent control
+
+maintainers:
+  - Ivan Mikhaylov <fr0st61te@gmail.com>
+
+description: |
+  The MAX34408/MAX34409 are two- and four-channel current monitors that are
+  configured and monitored with a standard I2C/SMBus serial interface. Each
+  unidirectional current sensor offers precision high-side operation with a
+  low full-scale sense voltage. The devices automatically sequence through
+  two or four channels and collect the current-sense samples and average them
+  to reduce the effect of impulse noise. The raw ADC samples are compared to
+  user-programmable digital thresholds to indicate overcurrent conditions.
+  Overcurrent conditions trigger a hardware output to provide an immediate
+  indication to shut down any necessary external circuitry.
+
+  Specifications about the devices can be found at:
+  https://www.analog.com/media/en/technical-documentation/data-sheets/MAX34408-MAX34409.pdf
+
+properties:
+  compatible:
+    enum:
+      - maxim,max34408
+      - maxim,max34409
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  maxim,input1-rsense-val-micro-ohms:
+    description:
+      Adjust the Rsense value to monitor higher or lower current levels for
+      input 1.
+    enum: [250, 500, 1000, 5000, 10000, 50000, 100000, 200000, 500000]
+    default: 1000
+
+  maxim,input2-rsense-val-micro-ohms:
+    description:
+      Adjust the Rsense value to monitor higher or lower current levels for
+      input 2.
+    enum: [250, 500, 1000, 5000, 10000, 50000, 100000, 200000, 500000]
+    default: 1000
+
+  maxim,input3-rsense-val-micro-ohms:
+    description:
+      Adjust the Rsense value to monitor higher or lower current levels for
+      input 3.
+    enum: [250, 500, 1000, 5000, 10000, 50000, 100000, 200000, 500000]
+    default: 1000
+
+  maxim,input4-rsense-val-micro-ohms:
+    description:
+      Adjust the Rsense value to monitor higher or lower current levels for
+      input 4.
+    enum: [250, 500, 1000, 5000, 10000, 50000, 100000, 200000, 500000]
+    default: 1000
+
+  maxim,shtdn:
+    description:
+      Shutdown Output. Open-drain output. This output transitions to high impedance
+      when any of the digital comparator thresholds are exceeded as long as the ENA
+      pin is high.
+    type: boolean
+
+  maxim,ena:
+    description:
+      SHTDN Enable Input. CMOS digital input. Connect to GND to clear the latch and
+      unconditionally deassert (force low) the SHTDN output and reset the shutdown
+      delay. Connect to VDD to enable normal latch operation of the SHTDN output.
+    type: boolean
+
+  supply-vdd: true
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        adc@1e {
+              compatible = "maxim,max34409";
+              reg = <0x1e>;
+              maxim,input1-rsense-val-micro-ohms = <5000>;
+              maxim,input2-rsense-val-micro-ohms = <10000>;
+        };
+    };
-- 
2.42.0

[PATCH v2 2/2] iio: adc: Add driver support for MAX34408/9

[Ivan Mikhaylov]

The MAX34408/MAX34409 are two- and four-channel current monitors that are
configured and monitored with a standard I2C/SMBus serial interface. Each
unidirectional current sensor offers precision high-side operation with a
low full-scale sense voltage. The devices automatically sequence through
two or four channels and collect the current-sense samples and average them
to reduce the effect of impulse noise. The raw ADC samples are compared to
user-programmable digital thresholds to indicate overcurrent conditions.
Overcurrent conditions trigger a hardware output to provide an immediate
indication to shut down any necessary external circuitry.

Add as ADC driver which only supports current monitoring for now.

Link: https://www.analog.com/media/en/technical-documentation/data-sheets/MAX34408-MAX34409.pdf

Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
---
 drivers/iio/adc/Kconfig    |  11 ++
 drivers/iio/adc/Makefile   |   1 +
 drivers/iio/adc/max34408.c | 334 +++++++++++++++++++++++++++++++++++++
 3 files changed, 346 insertions(+)
 create mode 100644 drivers/iio/adc/max34408.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 517b3db114b8..c215a2861350 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -735,6 +735,17 @@ config MAX1363
 	  To compile this driver as a module, choose M here: the module will be
 	  called max1363.
 
+config MAX34408
+	tristate "Maxim max34408/max344089 ADC driver"
+	depends on I2C
+	help
+	  Say yes here to build support for Maxim max34408/max34409 current sense
+	  monitor with 8-bits ADC interface with overcurrent delay/threshold and
+	  shutdown delay.
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called max34408.
+
 config MAX77541_ADC
 	tristate "Analog Devices MAX77541 ADC driver"
 	depends on MFD_MAX77541
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 2facf979327d..46dceab85e9a 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -67,6 +67,7 @@ obj-$(CONFIG_MAX11205) += max11205.o
 obj-$(CONFIG_MAX11410) += max11410.o
 obj-$(CONFIG_MAX1241) += max1241.o
 obj-$(CONFIG_MAX1363) += max1363.o
+obj-$(CONFIG_MAX34408) += max34408.o
 obj-$(CONFIG_MAX77541_ADC) += max77541-adc.o
 obj-$(CONFIG_MAX9611) += max9611.o
 obj-$(CONFIG_MCP320X) += mcp320x.o
diff --git a/drivers/iio/adc/max34408.c b/drivers/iio/adc/max34408.c
new file mode 100644
index 000000000000..f576c65e3900
--- /dev/null
+++ b/drivers/iio/adc/max34408.c
@@ -0,0 +1,334 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * IIO driver for Maxim MAX34409/34408 ADC, 4-Channels/2-Channels, 8bits, I2C
+ *
+ * Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/MAX34408-MAX34409.pdf
+ *
+ * TODO: ALERT interrupt, Overcurrent delay, Shutdown delay
+ */
+
+#include <linux/bitfield.h>
+#include <linux/init.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of_device.h>
+#include <linux/regmap.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/types.h>
+
+#define MAX34408_STATUS_REG		0x0
+#define MAX34408_CONTROL_REG		0x1
+#define MAX34408_OCDELAY_REG		0x2
+#define MAX34408_SDDELAY_REG		0x3
+
+#define MAX34408_ADC1_REG		0x4
+#define MAX34408_ADC2_REG		0x5
+/* ADC3 & ADC4 always returns 0x0 on 34408 */
+#define MAX34409_ADC3_REG		0x6
+#define MAX34409_ADC4_REG		0x7
+
+#define MAX34408_OCT1_REG		0x8
+#define MAX34408_OCT2_REG		0x9
+#define MAX34408_OCT3_REG		0xA
+#define MAX34408_OCT4_REG		0xB
+
+#define MAX34408_DID_REG		0xC
+#define MAX34408_DCYY_REG		0xD
+#define MAX34408_DCWW_REG		0xE
+
+/* Bit masks for status register */
+#define MAX34408_STATUS_OC_MSK		GENMASK(1, 0)
+#define MAX34409_STATUS_OC_MSK		GENMASK(3, 0)
+#define MAX34408_STATUS_SHTDN		BIT(4)
+#define MAX34408_STATUS_ENA		BIT(5)
+
+/* Bit masks for control register */
+#define MAX34408_CONTROL_AVG0		BIT(0)
+#define MAX34408_CONTROL_AVG1		BIT(1)
+#define MAX34408_CONTROL_AVG2		BIT(2)
+#define MAX34408_CONTROL_ALERT		BIT(3)
+
+/* Bit masks for over current delay */
+#define MAX34408_OCDELAY_OCD_MSK	GENMASK(6, 0)
+#define MAX34408_OCDELAY_RESET		BIT(7)
+
+/* Bit masks for shutdown delay */
+#define MAX34408_SDDELAY_SHD_MSK	GENMASK(6, 0)
+#define MAX34408_SDDELAY_RESET		BIT(7)
+
+#define MAX34408_DEFAULT_RSENSE		1000
+
+/**
+ * struct max34408_data - max34408/max34409 specific data.
+ * @regmap:	device register map.
+ * @dev:	max34408 device.
+ * @lock:	lock for protecting access to device hardware registers, mostly
+ *		for read modify write cycles for control registers.
+ * @rsense:	Rsense value in uOhm.
+ */
+struct max34408_data {
+	struct regmap *regmap;
+	struct device *dev;
+	struct mutex lock;
+	u32 input1_rsense;
+	u32 input2_rsense;
+	u32 input3_rsense;
+	u32 input4_rsense;
+};
+
+static const struct regmap_config max34408_regmap_config = {
+	.reg_bits	= 8,
+	.val_bits	= 8,
+	.max_register	= MAX34408_DCWW_REG,
+};
+
+struct max34408_adc_model_data {
+	const char *model_name;
+	const struct iio_chan_spec *channels;
+	const int num_channels;
+};
+
+#define MAX34008_CHANNEL(_index)					\
+	{								\
+		.type = IIO_CURRENT,					\
+		.info_mask_separate	= BIT(IIO_CHAN_INFO_RAW) |	\
+					  BIT(IIO_CHAN_INFO_SCALE) |	\
+					  BIT(IIO_CHAN_INFO_OFFSET),	\
+		.channel = (_index),					\
+		.indexed = 1,						\
+	}
+
+static const struct iio_chan_spec max34408_channels[] = {
+	MAX34008_CHANNEL(0),
+	MAX34008_CHANNEL(1),
+};
+
+static const struct iio_chan_spec max34409_channels[] = {
+	MAX34008_CHANNEL(0),
+	MAX34008_CHANNEL(1),
+	MAX34008_CHANNEL(2),
+	MAX34008_CHANNEL(3),
+};
+
+static int max34408_read_adc(struct max34408_data *max34408, int channel,
+			     int *val)
+{
+	u32 adc_reg;
+
+	switch (channel) {
+	case 0:
+		adc_reg = MAX34408_ADC1_REG;
+		break;
+	case 1:
+		adc_reg = MAX34408_ADC2_REG;
+		break;
+	case 2:
+		adc_reg = MAX34409_ADC3_REG;
+		break;
+	case 3:
+		adc_reg = MAX34409_ADC4_REG;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return regmap_read(max34408->regmap, adc_reg, val);
+}
+
+static int max34408_read_adc_avg(struct max34408_data *max34408, int channel, int *val)
+{
+	unsigned int ctrl;
+	int rc;
+	u8 tmp;
+
+	guard(mutex)(&max34408->lock);
+	rc = regmap_read(max34408->regmap, MAX34408_CONTROL_REG, (u32 *)&ctrl);
+	if (rc)
+		return rc;
+
+	/* set averaging (0b100) default values*/
+	tmp = ctrl;
+	ctrl |= MAX34408_CONTROL_AVG2;
+	ctrl &= ~MAX34408_CONTROL_AVG1;
+	ctrl &= ~MAX34408_CONTROL_AVG0;
+	rc = regmap_write(max34408->regmap, MAX34408_CONTROL_REG, ctrl);
+	if (rc) {
+		dev_err(max34408->dev,
+			"Error (%d) writing control register\n", rc);
+		return rc;
+	}
+
+	rc = max34408_read_adc(max34408, channel, val);
+	if (rc)
+		return rc;
+
+	/* back to old values */
+	rc = regmap_write(max34408->regmap, MAX34408_CONTROL_REG, tmp);
+	if (rc)
+		dev_err(max34408->dev,
+			"Error (%d) writing control register\n", rc);
+
+	return rc;
+}
+
+static int max34408_read_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     int *val, int *val2, long mask)
+{
+	struct max34408_data *max34408 = iio_priv(indio_dev);
+	int input_rsense;
+	int rc;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		rc = max34408_read_adc_avg(max34408, chan->channel,
+					   val);
+		if (rc)
+			return rc;
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		/*
+		 * calcluate current for 8bit ADC with Rsense
+		 * value.
+		 * 10 mV * 1000 / Rsense uOhm = max current
+		 * (max current * adc val * 1000) / (2^8 - 1) mA
+		 */
+		switch (chan->channel) {
+		case 0:
+			input_rsense = max34408->input1_rsense;
+			break;
+		case 1:
+			input_rsense = max34408->input2_rsense;
+			break;
+		case 2:
+			input_rsense = max34408->input3_rsense;
+			break;
+		case 3:
+			input_rsense = max34408->input4_rsense;
+			break;
+		default:
+			return -EINVAL;
+		}
+		*val = 10000 / input_rsense;
+		*val2 = 8;
+		return IIO_VAL_FRACTIONAL_LOG2;
+	case IIO_CHAN_INFO_OFFSET:
+		*val = 0;
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_info max34408_info = {
+	.read_raw	= max34408_read_raw,
+};
+
+static const struct max34408_adc_model_data max34408_model_data = {
+	.model_name = "max34408",
+	.channels = max34408_channels,
+	.num_channels = 2,
+};
+
+static const struct max34408_adc_model_data max34409_model_data = {
+	.model_name = "max34409",
+	.channels = max34409_channels,
+	.num_channels = 4,
+};
+
+static const struct of_device_id max34408_of_match[] = {
+	{
+		.compatible = "maxim,max34408",
+		.data = &max34408_model_data,
+	},
+	{
+		.compatible = "maxim,max34409",
+		.data = &max34409_model_data,
+	},
+	{}
+};
+MODULE_DEVICE_TABLE(of, max34408_of_match);
+
+static int max34408_probe(struct i2c_client *client)
+{
+	const struct max34408_adc_model_data *model_data;
+	const struct of_device_id *match;
+	struct max34408_data *max34408;
+	struct iio_dev *indio_dev;
+	struct regmap *regmap;
+	int rc;
+
+	match = of_match_device(max34408_of_match, &client->dev);
+	if (!match)
+		return -EINVAL;
+	model_data = match->data;
+
+	regmap = devm_regmap_init_i2c(client, &max34408_regmap_config);
+	if (IS_ERR(regmap)) {
+		dev_err_probe(&client->dev, PTR_ERR(regmap),
+			      "regmap_init failed\n");
+		return PTR_ERR(regmap);
+	}
+
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*max34408));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	max34408 = iio_priv(indio_dev);
+	max34408->regmap = regmap;
+	max34408->dev = &client->dev;
+	mutex_init(&max34408->lock);
+
+	rc = device_property_read_u32(&client->dev,
+				      "maxim,input1-rsense-val-micro-ohms",
+				      &max34408->input1_rsense);
+	if (rc)
+		max34408->input1_rsense = MAX34408_DEFAULT_RSENSE;
+
+	rc = device_property_read_u32(&client->dev,
+				      "maxim,input2-rsense-val-micro-ohms",
+				      &max34408->input2_rsense);
+	if (rc)
+		max34408->input2_rsense = MAX34408_DEFAULT_RSENSE;
+
+	rc = device_property_read_u32(&client->dev,
+				      "maxim,input3-rsense-val-micro-ohms",
+				      &max34408->input3_rsense);
+	if (rc)
+		max34408->input3_rsense = MAX34408_DEFAULT_RSENSE;
+
+	rc = device_property_read_u32(&client->dev,
+				      "maxim,input4-rsense-val-micro-ohms",
+				      &max34408->input4_rsense);
+	if (rc)
+		max34408->input4_rsense = MAX34408_DEFAULT_RSENSE;
+
+	/* disable ALERT and averaging */
+	rc = regmap_write(max34408->regmap, MAX34408_CONTROL_REG, 0x0);
+	if (rc)
+		return rc;
+
+	indio_dev->channels = model_data->channels;
+	indio_dev->num_channels = model_data->num_channels;
+	indio_dev->name = model_data->model_name;
+
+	indio_dev->info = &max34408_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	return devm_iio_device_register(&client->dev, indio_dev);
+}
+
+static struct i2c_driver max34408_driver = {
+	.driver = {
+		.name   = "max34408",
+		.of_match_table = max34408_of_match,
+	},
+	.probe = max34408_probe,
+};
+module_i2c_driver(max34408_driver);
+
+MODULE_AUTHOR("Ivan Mikhaylov <fr0st61te@gmail.com>");
+MODULE_DESCRIPTION("Maxim MAX34408/34409 ADC driver");
+MODULE_LICENSE("GPL");
-- 
2.42.0

Re: [PATCH v2 1/2] dt-bindings: adc: provide max34408/9 device tree binding document

[Rob Herring]

On Fri, 29 Sep 2023 23:08:43 +0300, Ivan Mikhaylov wrote:
> The hardware binding for i2c current monitoring device with overcurrent
> control.
> 
> Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
> ---
>  .../bindings/iio/adc/maxim,max34408.yaml      | 101 ++++++++++++++++++
>  1 file changed, 101 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/maxim,max34408.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/adc/maxim,max34408.yaml: supply-vdd: missing type definition

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230929200844.23316-2-fr0st61te@gmail.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH v2 2/2] iio: adc: Add driver support for MAX34408/9

[kernel test robot]

Hi Ivan,

kernel test robot noticed the following build warnings:

[auto build test WARNING on jic23-iio/togreg]
[also build test WARNING on robh/for-next linus/master v6.6-rc3 next-20230929]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Ivan-Mikhaylov/dt-bindings-adc-provide-max34408-9-device-tree-binding-document/20230930-043842
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link:    https://lore.kernel.org/r/20230929200844.23316-3-fr0st61te%40gmail.com
patch subject: [PATCH v2 2/2] iio: adc: Add driver support for MAX34408/9
config: sparc-allyesconfig (https://download.01.org/0day-ci/archive/20230930/202309300847.Vb7WCpaN-lkp@intel.com/config)
compiler: sparc64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230930/202309300847.Vb7WCpaN-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309300847.Vb7WCpaN-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/iio/adc/max34408.c:79: warning: Function parameter or member 'input1_rsense' not described in 'max34408_data'
>> drivers/iio/adc/max34408.c:79: warning: Function parameter or member 'input2_rsense' not described in 'max34408_data'
>> drivers/iio/adc/max34408.c:79: warning: Function parameter or member 'input3_rsense' not described in 'max34408_data'
>> drivers/iio/adc/max34408.c:79: warning: Function parameter or member 'input4_rsense' not described in 'max34408_data'


vim +79 drivers/iio/adc/max34408.c

    62	
    63	/**
    64	 * struct max34408_data - max34408/max34409 specific data.
    65	 * @regmap:	device register map.
    66	 * @dev:	max34408 device.
    67	 * @lock:	lock for protecting access to device hardware registers, mostly
    68	 *		for read modify write cycles for control registers.
    69	 * @rsense:	Rsense value in uOhm.
    70	 */
    71	struct max34408_data {
    72		struct regmap *regmap;
    73		struct device *dev;
    74		struct mutex lock;
    75		u32 input1_rsense;
    76		u32 input2_rsense;
    77		u32 input3_rsense;
    78		u32 input4_rsense;
  > 79	};
    80	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 1/2] dt-bindings: adc: provide max34408/9 device tree binding document

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 4895 bytes --]

Hey,

On Fri, Sep 29, 2023 at 11:08:43PM +0300, Ivan Mikhaylov wrote:
> The hardware binding for i2c current monitoring device with overcurrent
> control.
> 
> Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
> ---
>  .../bindings/iio/adc/maxim,max34408.yaml      | 101 ++++++++++++++++++
>  1 file changed, 101 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/maxim,max34408.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/maxim,max34408.yaml b/Documentation/devicetree/bindings/iio/adc/maxim,max34408.yaml
> new file mode 100644
> index 000000000000..cdf89fa4c80e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/maxim,max34408.yaml
> @@ -0,0 +1,101 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/maxim,max34408.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Two- and four-channel current monitors with overcurrent control
> +
> +maintainers:
> +  - Ivan Mikhaylov <fr0st61te@gmail.com>
> +
> +description: |
> +  The MAX34408/MAX34409 are two- and four-channel current monitors that are
> +  configured and monitored with a standard I2C/SMBus serial interface. Each
> +  unidirectional current sensor offers precision high-side operation with a
> +  low full-scale sense voltage. The devices automatically sequence through
> +  two or four channels and collect the current-sense samples and average them
> +  to reduce the effect of impulse noise. The raw ADC samples are compared to
> +  user-programmable digital thresholds to indicate overcurrent conditions.
> +  Overcurrent conditions trigger a hardware output to provide an immediate
> +  indication to shut down any necessary external circuitry.
> +
> +  Specifications about the devices can be found at:
> +  https://www.analog.com/media/en/technical-documentation/data-sheets/MAX34408-MAX34409.pdf
> +
> +properties:
> +  compatible:
> +    enum:
> +      - maxim,max34408
> +      - maxim,max34409
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  maxim,input1-rsense-val-micro-ohms:
> +    description:
> +      Adjust the Rsense value to monitor higher or lower current levels for
> +      input 1.
> +    enum: [250, 500, 1000, 5000, 10000, 50000, 100000, 200000, 500000]
> +    default: 1000
> +
> +  maxim,input2-rsense-val-micro-ohms:
> +    description:
> +      Adjust the Rsense value to monitor higher or lower current levels for
> +      input 2.
> +    enum: [250, 500, 1000, 5000, 10000, 50000, 100000, 200000, 500000]
> +    default: 1000
> +
> +  maxim,input3-rsense-val-micro-ohms:
> +    description:
> +      Adjust the Rsense value to monitor higher or lower current levels for
> +      input 3.
> +    enum: [250, 500, 1000, 5000, 10000, 50000, 100000, 200000, 500000]
> +    default: 1000
> +
> +  maxim,input4-rsense-val-micro-ohms:
> +    description:
> +      Adjust the Rsense value to monitor higher or lower current levels for
> +      input 4.
> +    enum: [250, 500, 1000, 5000, 10000, 50000, 100000, 200000, 500000]
> +    default: 1000

Having 4 almost identical properties makes it seem like this should have
some channel nodes, each containing an rsense-micro-ohms type property.

> +
> +  maxim,shtdn:
> +    description:
> +      Shutdown Output. Open-drain output. This output transitions to high impedance
> +      when any of the digital comparator thresholds are exceeded as long as the ENA
> +      pin is high.
> +    type: boolean

I don't understand what this property is used for. The description here,
and below for "ena", read like they are the descriptions in the
datasheet for the pin, rather than how to use the property.

The drivers don't appear to contain users either - what is the point of
these properties?

> +
> +  maxim,ena:
> +    description:
> +      SHTDN Enable Input. CMOS digital input. Connect to GND to clear the latch and
> +      unconditionally deassert (force low) the SHTDN output and reset the shutdown
> +      delay. Connect to VDD to enable normal latch operation of the SHTDN output.
> +    type: boolean
> +
> +  supply-vdd: true

As pointed out by the bot, this is not correct. You need to use a
-supply affix, not a supply-prefix.

Thanks,
Conor.

> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        adc@1e {
> +              compatible = "maxim,max34409";
> +              reg = <0x1e>;
> +              maxim,input1-rsense-val-micro-ohms = <5000>;
> +              maxim,input2-rsense-val-micro-ohms = <10000>;
> +        };
> +    };
> -- 
> 2.42.0
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 1/2] dt-bindings: adc: provide max34408/9 device tree binding document

[Jonathan Cameron]

On Sat, 30 Sep 2023 10:37:09 +0100
Conor Dooley <conor@kernel.org> wrote:

> Hey,
> 
> On Fri, Sep 29, 2023 at 11:08:43PM +0300, Ivan Mikhaylov wrote:
> > The hardware binding for i2c current monitoring device with overcurrent
> > control.
> > 
> > Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
> > ---
> >  .../bindings/iio/adc/maxim,max34408.yaml      | 101 ++++++++++++++++++
> >  1 file changed, 101 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/adc/maxim,max34408.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/adc/maxim,max34408.yaml b/Documentation/devicetree/bindings/iio/adc/maxim,max34408.yaml
> > new file mode 100644
> > index 000000000000..cdf89fa4c80e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/maxim,max34408.yaml
> > @@ -0,0 +1,101 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iio/adc/maxim,max34408.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Two- and four-channel current monitors with overcurrent control
> > +
> > +maintainers:
> > +  - Ivan Mikhaylov <fr0st61te@gmail.com>
> > +
> > +description: |
> > +  The MAX34408/MAX34409 are two- and four-channel current monitors that are
> > +  configured and monitored with a standard I2C/SMBus serial interface. Each
> > +  unidirectional current sensor offers precision high-side operation with a
> > +  low full-scale sense voltage. The devices automatically sequence through
> > +  two or four channels and collect the current-sense samples and average them
> > +  to reduce the effect of impulse noise. The raw ADC samples are compared to
> > +  user-programmable digital thresholds to indicate overcurrent conditions.
> > +  Overcurrent conditions trigger a hardware output to provide an immediate
> > +  indication to shut down any necessary external circuitry.
> > +
> > +  Specifications about the devices can be found at:
> > +  https://www.analog.com/media/en/technical-documentation/data-sheets/MAX34408-MAX34409.pdf
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - maxim,max34408
> > +      - maxim,max34409
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  maxim,input1-rsense-val-micro-ohms:
> > +    description:
> > +      Adjust the Rsense value to monitor higher or lower current levels for
> > +      input 1.
> > +    enum: [250, 500, 1000, 5000, 10000, 50000, 100000, 200000, 500000]
> > +    default: 1000
> > +
> > +  maxim,input2-rsense-val-micro-ohms:
> > +    description:
> > +      Adjust the Rsense value to monitor higher or lower current levels for
> > +      input 2.
> > +    enum: [250, 500, 1000, 5000, 10000, 50000, 100000, 200000, 500000]
> > +    default: 1000
> > +
> > +  maxim,input3-rsense-val-micro-ohms:
> > +    description:
> > +      Adjust the Rsense value to monitor higher or lower current levels for
> > +      input 3.
> > +    enum: [250, 500, 1000, 5000, 10000, 50000, 100000, 200000, 500000]
> > +    default: 1000
> > +
> > +  maxim,input4-rsense-val-micro-ohms:
> > +    description:
> > +      Adjust the Rsense value to monitor higher or lower current levels for
> > +      input 4.
> > +    enum: [250, 500, 1000, 5000, 10000, 50000, 100000, 200000, 500000]
> > +    default: 1000  
> 
> Having 4 almost identical properties makes it seem like this should have
> some channel nodes, each containing an rsense-micro-ohms type property.
Agreed.  That is most flexible route if there is any chance of ending up
with more channel specific stuff in future.

There should also be some magic in here to rule out the input3 and input4
for the devices with only two channels. (lots of examples in tree)

Otherwise, in theory this could be an array I guess, but I'd also prefer
channel nodes.

> 
> > +
> > +  maxim,shtdn:
> > +    description:
> > +      Shutdown Output. Open-drain output. This output transitions to high impedance
> > +      when any of the digital comparator thresholds are exceeded as long as the ENA
> > +      pin is high.
> > +    type: boolean  
> 
> I don't understand what this property is used for. The description here,
> and below for "ena", read like they are the descriptions in the
> datasheet for the pin, rather than how to use the property.
> 
> The drivers don't appear to contain users either - what is the point of
> these properties?
> 
> > +
> > +  maxim,ena:
> > +    description:
> > +      SHTDN Enable Input. CMOS digital input. Connect to GND to clear the latch and
> > +      unconditionally deassert (force low) the SHTDN output and reset the shutdown
> > +      delay. Connect to VDD to enable normal latch operation of the SHTDN output.
> > +    type: boolean
> > +
> > +  supply-vdd: true  
> 
> As pointed out by the bot, this is not correct. You need to use a
> -supply affix, not a supply-prefix.
My error in earlier review (not enough coffee that day I guess :)

Anyhow it does show that the tests weren't run which isn't a good thing to see.

Jonathan

> 
> Thanks,
> Conor.
> 
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    i2c {
> > +
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        adc@1e {
> > +              compatible = "maxim,max34409";
> > +              reg = <0x1e>;
> > +              maxim,input1-rsense-val-micro-ohms = <5000>;
> > +              maxim,input2-rsense-val-micro-ohms = <10000>;
> > +        };
> > +    };
> > -- 
> > 2.42.0
> >  

Re: [PATCH v2 2/2] iio: adc: Add driver support for MAX34408/9

[Jonathan Cameron]

On Fri, 29 Sep 2023 23:08:44 +0300
Ivan Mikhaylov <fr0st61te@gmail.com> wrote:

> The MAX34408/MAX34409 are two- and four-channel current monitors that are
> configured and monitored with a standard I2C/SMBus serial interface. Each
> unidirectional current sensor offers precision high-side operation with a
> low full-scale sense voltage. The devices automatically sequence through
> two or four channels and collect the current-sense samples and average them
> to reduce the effect of impulse noise. The raw ADC samples are compared to
> user-programmable digital thresholds to indicate overcurrent conditions.
> Overcurrent conditions trigger a hardware output to provide an immediate
> indication to shut down any necessary external circuitry.
> 
> Add as ADC driver which only supports current monitoring for now.
> 
> Link: https://www.analog.com/media/en/technical-documentation/data-sheets/MAX34408-MAX34409.pdf
> 
> Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>

Hi Ivan,

Various small comments inline. Mostly simplifications to the code.

Thanks,

Jonathan

> diff --git a/drivers/iio/adc/max34408.c b/drivers/iio/adc/max34408.c
> new file mode 100644
> index 000000000000..f576c65e3900
> --- /dev/null
> +++ b/drivers/iio/adc/max34408.c
> @@ -0,0 +1,334 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * IIO driver for Maxim MAX34409/34408 ADC, 4-Channels/2-Channels, 8bits, I2C
> + *
> + * Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/MAX34408-MAX34409.pdf
> + *
> + * TODO: ALERT interrupt, Overcurrent delay, Shutdown delay
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/init.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of_device.h>
> +#include <linux/regmap.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/types.h>
> +
> +#define MAX34408_STATUS_REG		0x0
> +#define MAX34408_CONTROL_REG		0x1
> +#define MAX34408_OCDELAY_REG		0x2
> +#define MAX34408_SDDELAY_REG		0x3
> +
> +#define MAX34408_ADC1_REG		0x4
> +#define MAX34408_ADC2_REG		0x5
> +/* ADC3 & ADC4 always returns 0x0 on 34408 */
> +#define MAX34409_ADC3_REG		0x6
> +#define MAX34409_ADC4_REG		0x7
> +
> +#define MAX34408_OCT1_REG		0x8
> +#define MAX34408_OCT2_REG		0x9
> +#define MAX34408_OCT3_REG		0xA
> +#define MAX34408_OCT4_REG		0xB
> +
> +#define MAX34408_DID_REG		0xC
> +#define MAX34408_DCYY_REG		0xD
> +#define MAX34408_DCWW_REG		0xE
> +
> +/* Bit masks for status register */
> +#define MAX34408_STATUS_OC_MSK		GENMASK(1, 0)
> +#define MAX34409_STATUS_OC_MSK		GENMASK(3, 0)
> +#define MAX34408_STATUS_SHTDN		BIT(4)
> +#define MAX34408_STATUS_ENA		BIT(5)
> +
> +/* Bit masks for control register */
> +#define MAX34408_CONTROL_AVG0		BIT(0)
> +#define MAX34408_CONTROL_AVG1		BIT(1)
> +#define MAX34408_CONTROL_AVG2		BIT(2)
> +#define MAX34408_CONTROL_ALERT		BIT(3)
> +
> +/* Bit masks for over current delay */
> +#define MAX34408_OCDELAY_OCD_MSK	GENMASK(6, 0)
> +#define MAX34408_OCDELAY_RESET		BIT(7)
> +
> +/* Bit masks for shutdown delay */
> +#define MAX34408_SDDELAY_SHD_MSK	GENMASK(6, 0)
> +#define MAX34408_SDDELAY_RESET		BIT(7)
> +
> +#define MAX34408_DEFAULT_RSENSE		1000
> +
> +/**
> + * struct max34408_data - max34408/max34409 specific data.
> + * @regmap:	device register map.
> + * @dev:	max34408 device.
> + * @lock:	lock for protecting access to device hardware registers, mostly
> + *		for read modify write cycles for control registers.
> + * @rsense:	Rsense value in uOhm.

Bot complained about this doc being out of date.
As mentioned below, an array seems more appropriate to me than 4
separate struct elements.

> + */
> +struct max34408_data {
> +	struct regmap *regmap;
> +	struct device *dev;
> +	struct mutex lock;
> +	u32 input1_rsense;
> +	u32 input2_rsense;
> +	u32 input3_rsense;
> +	u32 input4_rsense;
> +};
> +
> +static const struct regmap_config max34408_regmap_config = {
> +	.reg_bits	= 8,
> +	.val_bits	= 8,
> +	.max_register	= MAX34408_DCWW_REG,
> +};
> +
> +struct max34408_adc_model_data {
> +	const char *model_name;
> +	const struct iio_chan_spec *channels;
> +	const int num_channels;
> +};
> +
> +#define MAX34008_CHANNEL(_index)					\
> +	{								\
> +		.type = IIO_CURRENT,					\
> +		.info_mask_separate	= BIT(IIO_CHAN_INFO_RAW) |	\

I'd drop the tab and not force that indent.  A space is enough and consistent
with other elements.

> +					  BIT(IIO_CHAN_INFO_SCALE) |	\
> +					  BIT(IIO_CHAN_INFO_OFFSET),	\
> +		.channel = (_index),					\
> +		.indexed = 1,						\
> +	}
> +
> +static const struct iio_chan_spec max34408_channels[] = {
> +	MAX34008_CHANNEL(0),
> +	MAX34008_CHANNEL(1),
> +};
> +
> +static const struct iio_chan_spec max34409_channels[] = {
> +	MAX34008_CHANNEL(0),
> +	MAX34008_CHANNEL(1),
> +	MAX34008_CHANNEL(2),
> +	MAX34008_CHANNEL(3),
> +};
> +
> +static int max34408_read_adc(struct max34408_data *max34408, int channel,
> +			     int *val)
> +{
> +	u32 adc_reg;
> +
> +	switch (channel) {
> +	case 0:
> +		adc_reg = MAX34408_ADC1_REG;
Channels have an .address field for this purpose.
I would put this in there then this just becomes

	return regmap_read(max34408->regmap, chan->address, val);

with no need for the switch statement.


> +		break;
> +	case 1:
> +		adc_reg = MAX34408_ADC2_REG;
> +		break;
> +	case 2:
> +		adc_reg = MAX34409_ADC3_REG;
> +		break;
> +	case 3:
> +		adc_reg = MAX34409_ADC4_REG;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return regmap_read(max34408->regmap, adc_reg, val);
> +}
> +
> +static int max34408_read_adc_avg(struct max34408_data *max34408, int channel, int *val)
> +{
> +	unsigned int ctrl;
> +	int rc;
> +	u8 tmp;
> +
> +	guard(mutex)(&max34408->lock);
> +	rc = regmap_read(max34408->regmap, MAX34408_CONTROL_REG, (u32 *)&ctrl);
> +	if (rc)
> +		return rc;
> +
> +	/* set averaging (0b100) default values*/
> +	tmp = ctrl;
> +	ctrl |= MAX34408_CONTROL_AVG2;
> +	ctrl &= ~MAX34408_CONTROL_AVG1;
> +	ctrl &= ~MAX34408_CONTROL_AVG0;
It's a 3 bit field. Define a single mask for the field and then either
as set of defines for the values, or (if my maths is write)

#define	MAX34408_CONTROL_AVG_FIELD_TO_SAMPLES(x)	(1 << x)
#define MAX34408_CONTROL_AVG_SAMPLES_TO_FIELD(x)  ffs(x)

to go backwards and forwards from the encoded value which is just powers of 2

> +	rc = regmap_write(max34408->regmap, MAX34408_CONTROL_REG, ctrl);
> +	if (rc) {
> +		dev_err(max34408->dev,
> +			"Error (%d) writing control register\n", rc);
> +		return rc;
> +	}
> +
> +	rc = max34408_read_adc(max34408, channel, val);
> +	if (rc)
> +		return rc;
> +
> +	/* back to old values */
> +	rc = regmap_write(max34408->regmap, MAX34408_CONTROL_REG, tmp);
> +	if (rc)
> +		dev_err(max34408->dev,
> +			"Error (%d) writing control register\n", rc);
> +
> +	return rc;
> +}
> +
> +static int max34408_read_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int *val, int *val2, long mask)
> +{
> +	struct max34408_data *max34408 = iio_priv(indio_dev);
> +	int input_rsense;
> +	int rc;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		rc = max34408_read_adc_avg(max34408, chan->channel,
> +					   val);
Fits on one line under 80 chars.  Check for other cases and tidy
those up as well.

> +		if (rc)
> +			return rc;
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		/*
> +		 * calcluate current for 8bit ADC with Rsense
> +		 * value.
> +		 * 10 mV * 1000 / Rsense uOhm = max current
> +		 * (max current * adc val * 1000) / (2^8 - 1) mA
> +		 */
> +		switch (chan->channel) {
> +		case 0:
> +			input_rsense = max34408->input1_rsense;
Array?
Then this becomes
		input_rsense = max34408->input_rsense[chan->channel];

or skip the local variable and use it directly

		*val = 10000 / max34408->input_rsense[chan->chanel];
		*val2 = 8;
		return IIO_VAL_FRACTION_LOG2;

> +			break;
> +		case 1:
> +			input_rsense = max34408->input2_rsense;
> +			break;
> +		case 2:
> +			input_rsense = max34408->input3_rsense;
> +			break;
> +		case 3:
> +			input_rsense = max34408->input4_rsense;
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +		*val = 10000 / input_rsense;
> +		*val2 = 8;
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +	case IIO_CHAN_INFO_OFFSET:
> +		*val = 0;

When the offset is 0, there is no in exposing that interface.
0 is the default if the file isn't there in sysfs.

> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct iio_info max34408_info = {
> +	.read_raw	= max34408_read_raw,
> +};

...

> +static int max34408_probe(struct i2c_client *client)
> +{
> +	const struct max34408_adc_model_data *model_data;
> +	const struct of_device_id *match;
> +	struct max34408_data *max34408;
> +	struct iio_dev *indio_dev;
> +	struct regmap *regmap;
> +	int rc;
> +
> +	match = of_match_device(max34408_of_match, &client->dev);
> +	if (!match)
> +		return -EINVAL;
> +	model_data = match->data;
> +
> +	regmap = devm_regmap_init_i2c(client, &max34408_regmap_config);
> +	if (IS_ERR(regmap)) {
> +		dev_err_probe(&client->dev, PTR_ERR(regmap),
> +			      "regmap_init failed\n");
> +		return PTR_ERR(regmap);
> +	}
> +
> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*max34408));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	max34408 = iio_priv(indio_dev);
> +	max34408->regmap = regmap;
> +	max34408->dev = &client->dev;
> +	mutex_init(&max34408->lock);
> +
> +	rc = device_property_read_u32(&client->dev,
> +				      "maxim,input1-rsense-val-micro-ohms",
> +				      &max34408->input1_rsense);
> +	if (rc)
> +		max34408->input1_rsense = MAX34408_DEFAULT_RSENSE;
> +
> +	rc = device_property_read_u32(&client->dev,
> +				      "maxim,input2-rsense-val-micro-ohms",
> +				      &max34408->input2_rsense);
> +	if (rc)
> +		max34408->input2_rsense = MAX34408_DEFAULT_RSENSE;
> +
> +	rc = device_property_read_u32(&client->dev,
> +				      "maxim,input3-rsense-val-micro-ohms",
> +				      &max34408->input3_rsense);
> +	if (rc)
> +		max34408->input3_rsense = MAX34408_DEFAULT_RSENSE;
> +
> +	rc = device_property_read_u32(&client->dev,
> +				      "maxim,input4-rsense-val-micro-ohms",
> +				      &max34408->input4_rsense);
> +	if (rc)
> +		max34408->input4_rsense = MAX34408_DEFAULT_RSENSE;
> +
So, device_property_read_u32() is guaranteed to have no side effects on the
value being set if there is an error. Hence normal way of doing defaults
for optional parameters is the simpler.

	max34408->input4_rsense = MAX34408_DEFAULT_RENSE;
	device_property_read_u32(dev, "....", &max34408->input4_rsense);

So deliberately don't check the return value.

Not I would also suggest introducing a
struct device *dev = &client->dev; local variable as you use &client->dev
a lot.

Jonathan

Re: [PATCH v2 1/2] dt-bindings: adc: provide max34408/9 device tree binding document

[Ivan Mikhaylov]

On Sat, 2023-09-30 at 10:37 +0100, Conor Dooley wrote:
> Hey,
> 
> On Fri, Sep 29, 2023 at 11:08:43PM +0300, Ivan Mikhaylov wrote:
> > The hardware binding for i2c current monitoring device with
> > overcurrent
> > control.
> > 
> > Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
> > ---
> >  .../bindings/iio/adc/maxim,max34408.yaml      | 101
> > ++++++++++++++++++
> >  1 file changed, 101 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/iio/adc/maxim,max34408.yaml
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/iio/adc/maxim,max34408.yaml
> > b/Documentation/devicetree/bindings/iio/adc/maxim,max34408.yaml
> > new file mode 100644
> > index 000000000000..cdf89fa4c80e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/maxim,max34408.yaml
> > @@ -0,0 +1,101 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iio/adc/maxim,max34408.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Two- and four-channel current monitors with overcurrent
> > control
> > +
> > +maintainers:
> > +  - Ivan Mikhaylov <fr0st61te@gmail.com>
> > +
> > +description: |
> > +  The MAX34408/MAX34409 are two- and four-channel current monitors
> > that are
> > +  configured and monitored with a standard I2C/SMBus serial
> > interface. Each
> > +  unidirectional current sensor offers precision high-side
> > operation with a
> > +  low full-scale sense voltage. The devices automatically sequence
> > through
> > +  two or four channels and collect the current-sense samples and
> > average them
> > +  to reduce the effect of impulse noise. The raw ADC samples are
> > compared to
> > +  user-programmable digital thresholds to indicate overcurrent
> > conditions.
> > +  Overcurrent conditions trigger a hardware output to provide an
> > immediate
> > +  indication to shut down any necessary external circuitry.
> > +
> > +  Specifications about the devices can be found at:
> > + 
> > https://www.analog.com/media/en/technical-documentation/data-sheets/MAX34408-MAX34409.pdf
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - maxim,max34408
> > +      - maxim,max34409
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  maxim,input1-rsense-val-micro-ohms:
> > +    description:
> > +      Adjust the Rsense value to monitor higher or lower current
> > levels for
> > +      input 1.
> > +    enum: [250, 500, 1000, 5000, 10000, 50000, 100000, 200000,
> > 500000]
> > +    default: 1000
> > +
> > +  maxim,input2-rsense-val-micro-ohms:
> > +    description:
> > +      Adjust the Rsense value to monitor higher or lower current
> > levels for
> > +      input 2.
> > +    enum: [250, 500, 1000, 5000, 10000, 50000, 100000, 200000,
> > 500000]
> > +    default: 1000
> > +
> > +  maxim,input3-rsense-val-micro-ohms:
> > +    description:
> > +      Adjust the Rsense value to monitor higher or lower current
> > levels for
> > +      input 3.
> > +    enum: [250, 500, 1000, 5000, 10000, 50000, 100000, 200000,
> > 500000]
> > +    default: 1000
> > +
> > +  maxim,input4-rsense-val-micro-ohms:
> > +    description:
> > +      Adjust the Rsense value to monitor higher or lower current
> > levels for
> > +      input 4.
> > +    enum: [250, 500, 1000, 5000, 10000, 50000, 100000, 200000,
> > 500000]
> > +    default: 1000
> 
> Having 4 almost identical properties makes it seem like this should
> have
> some channel nodes, each containing an rsense-micro-ohms type
> property.

Conor, I'll look through.

> 
> > +
> > +  maxim,shtdn:
> > +    description:
> > +      Shutdown Output. Open-drain output. This output transitions
> > to high impedance
> > +      when any of the digital comparator thresholds are exceeded
> > as long as the ENA
> > +      pin is high.
> > +    type: boolean
> 
> I don't understand what this property is used for. The description
> here,
> and below for "ena", read like they are the descriptions in the
> datasheet for the pin, rather than how to use the property.
> 
> The drivers don't appear to contain users either - what is the point
> of
> these properties?

ena and shtdn physical pins of hardware, in the previous version
Jonathan asked about adding them into yaml even if it's not used in
code. should I do it in some other way?

> 
> > +
> > +  maxim,ena:
> > +    description:
> > +      SHTDN Enable Input. CMOS digital input. Connect to GND to
> > clear the latch and
> > +      unconditionally deassert (force low) the SHTDN output and
> > reset the shutdown
> > +      delay. Connect to VDD to enable normal latch operation of
> > the SHTDN output.
> > +    type: boolean
> > +
> > +  supply-vdd: true
> 
> As pointed out by the bot, this is not correct. You need to use a
> -supply affix, not a supply-prefix.

Oops.

Thanks.

Re: [PATCH v2 1/2] dt-bindings: adc: provide max34408/9 device tree binding document

[Jonathan Cameron]

On Sat, 30 Sep 2023 22:38:58 +0300
Ivan Mikhaylov <fr0st61te@gmail.com> wrote:

> On Sat, 2023-09-30 at 10:37 +0100, Conor Dooley wrote:
> > Hey,
> > 
> > On Fri, Sep 29, 2023 at 11:08:43PM +0300, Ivan Mikhaylov wrote:  
> > > The hardware binding for i2c current monitoring device with
> > > overcurrent
> > > control.
> > > 
> > > Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
> > > ---
> > >  .../bindings/iio/adc/maxim,max34408.yaml      | 101
> > > ++++++++++++++++++
> > >  1 file changed, 101 insertions(+)
> > >  create mode 100644
> > > Documentation/devicetree/bindings/iio/adc/maxim,max34408.yaml
> > > 
> > > diff --git
> > > a/Documentation/devicetree/bindings/iio/adc/maxim,max34408.yaml
> > > b/Documentation/devicetree/bindings/iio/adc/maxim,max34408.yaml
> > > new file mode 100644
> > > index 000000000000..cdf89fa4c80e
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/iio/adc/maxim,max34408.yaml
> > > @@ -0,0 +1,101 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/iio/adc/maxim,max34408.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Two- and four-channel current monitors with overcurrent
> > > control
> > > +
> > > +maintainers:
> > > +  - Ivan Mikhaylov <fr0st61te@gmail.com>
> > > +
> > > +description: |
> > > +  The MAX34408/MAX34409 are two- and four-channel current monitors
> > > that are
> > > +  configured and monitored with a standard I2C/SMBus serial
> > > interface. Each
> > > +  unidirectional current sensor offers precision high-side
> > > operation with a
> > > +  low full-scale sense voltage. The devices automatically sequence
> > > through
> > > +  two or four channels and collect the current-sense samples and
> > > average them
> > > +  to reduce the effect of impulse noise. The raw ADC samples are
> > > compared to
> > > +  user-programmable digital thresholds to indicate overcurrent
> > > conditions.
> > > +  Overcurrent conditions trigger a hardware output to provide an
> > > immediate
> > > +  indication to shut down any necessary external circuitry.
> > > +
> > > +  Specifications about the devices can be found at:
> > > + 
> > > https://www.analog.com/media/en/technical-documentation/data-sheets/MAX34408-MAX34409.pdf
> > > +
> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - maxim,max34408
> > > +      - maxim,max34409
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  interrupts:
> > > +    maxItems: 1
> > > +
> > > +  maxim,input1-rsense-val-micro-ohms:
> > > +    description:
> > > +      Adjust the Rsense value to monitor higher or lower current
> > > levels for
> > > +      input 1.
> > > +    enum: [250, 500, 1000, 5000, 10000, 50000, 100000, 200000,
> > > 500000]
> > > +    default: 1000
> > > +
> > > +  maxim,input2-rsense-val-micro-ohms:
> > > +    description:
> > > +      Adjust the Rsense value to monitor higher or lower current
> > > levels for
> > > +      input 2.
> > > +    enum: [250, 500, 1000, 5000, 10000, 50000, 100000, 200000,
> > > 500000]
> > > +    default: 1000
> > > +
> > > +  maxim,input3-rsense-val-micro-ohms:
> > > +    description:
> > > +      Adjust the Rsense value to monitor higher or lower current
> > > levels for
> > > +      input 3.
> > > +    enum: [250, 500, 1000, 5000, 10000, 50000, 100000, 200000,
> > > 500000]
> > > +    default: 1000
> > > +
> > > +  maxim,input4-rsense-val-micro-ohms:
> > > +    description:
> > > +      Adjust the Rsense value to monitor higher or lower current
> > > levels for
> > > +      input 4.
> > > +    enum: [250, 500, 1000, 5000, 10000, 50000, 100000, 200000,
> > > 500000]
> > > +    default: 1000  
> > 
> > Having 4 almost identical properties makes it seem like this should
> > have
> > some channel nodes, each containing an rsense-micro-ohms type
> > property.  
> 
> Conor, I'll look through.
> 
> >   
> > > +
> > > +  maxim,shtdn:
> > > +    description:
> > > +      Shutdown Output. Open-drain output. This output transitions
> > > to high impedance
> > > +      when any of the digital comparator thresholds are exceeded
> > > as long as the ENA
> > > +      pin is high.
> > > +    type: boolean  
> > 
> > I don't understand what this property is used for. The description
> > here,
> > and below for "ena", read like they are the descriptions in the
> > datasheet for the pin, rather than how to use the property.
> > 
> > The drivers don't appear to contain users either - what is the point
> > of
> > these properties?  
> 
> ena and shtdn physical pins of hardware, in the previous version
> Jonathan asked about adding them into yaml even if it's not used in
> code. should I do it in some other way?

Yes, use the gpio bindings.

enable-gpios, isshutdown-gpios 
perhaps - though try and find a similar example for the naming.

The shutdown one is a bit unusual in that it indicates the device
has shutdown for one of several possible reasons (IIRC)

Jonathan

> 
> >   
> > > +
> > > +  maxim,ena:
> > > +    description:
> > > +      SHTDN Enable Input. CMOS digital input. Connect to GND to
> > > clear the latch and
> > > +      unconditionally deassert (force low) the SHTDN output and
> > > reset the shutdown
> > > +      delay. Connect to VDD to enable normal latch operation of
> > > the SHTDN output.
> > > +    type: boolean
> > > +
> > > +  supply-vdd: true  
> > 
> > As pointed out by the bot, this is not correct. You need to use a
> > -supply affix, not a supply-prefix.  
> 
> Oops.
> 
> Thanks.
> 
>