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

[PATCH v5 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

Changes from v2:
   - add channels into hardware description into yaml
   - add rsense property per channel
   - rename pins for shtdn and ena pins
   - make one array for input_rsense values

Changes from v3:
   - change *_34408_OCT3 and 4 to *_34409_OCT3 and 4
   - change of_property_read_u32 to fwnode family calls
   - add i2c dev table
   - change of_match_device to i2c_of_match_device
   - change match->data to i2c_get_match_data 

Changes from v4:
   - minor changes in yaml

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      | 141 +++++++++
 drivers/iio/adc/Kconfig                       |  11 +
 drivers/iio/adc/Makefile                      |   1 +
 drivers/iio/adc/max34408.c                    | 278 ++++++++++++++++++
 4 files changed, 431 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/maxim,max34408.yaml
 create mode 100644 drivers/iio/adc/max34408.c

-- 
2.42.0

[PATCH v5 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      | 141 ++++++++++++++++++
 1 file changed, 141 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..9749f1fd1802
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/maxim,max34408.yaml
@@ -0,0 +1,141 @@
+# 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
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  powerdown-gpios:
+    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.
+    maxItems: 1
+
+  shtdn-enable-gpios:
+    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.
+    maxItems: 1
+
+  vdd-supply: true
+
+patternProperties:
+  "^channel@[0-3]$":
+    $ref: adc.yaml
+    type: object
+    description:
+      Represents the internal channels of the ADC.
+
+    properties:
+      reg:
+        items:
+          minimum: 0
+          maximum: 3
+
+      maxim,rsense-val-micro-ohms:
+        description:
+          Adjust the Rsense value to monitor higher or lower current levels for
+          input.
+        enum: [250, 500, 1000, 5000, 10000, 50000, 100000, 200000, 500000]
+        default: 1000
+
+    required:
+      - reg
+      - maxim,rsense-val-micro-ohms
+
+    unevaluatedProperties: false
+
+required:
+  - compatible
+  - reg
+
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: maxim,max34408
+    then:
+      patternProperties:
+        "^channel@[2-3]$": false
+        "^channel@[0-1]$":
+          properties:
+            reg:
+              minimum: 0
+              maximum: 1
+    else:
+      patternProperties:
+        "^channel@[0-3]$":
+          properties:
+            reg:
+              minimum: 0
+              maximum: 3
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        adc@1e {
+              compatible = "maxim,max34409";
+              reg = <0x1e>;
+              powerdown-gpios = <&gpio0 1 GPIO_ACTIVE_LOW>;
+              shtdn-enable-gpios = <&gpio0 2 GPIO_ACTIVE_HIGH>;
+
+              #address-cells = <1>;
+              #size-cells = <0>;
+
+              channel@0 {
+                  reg = <0x0>;
+                  maxim,rsense-val-micro-ohms = <5000>;
+              };
+
+              channel@1 {
+                  reg = <0x1>;
+                  maxim,rsense-val-micro-ohms = <10000>;
+             };
+        };
+    };
-- 
2.42.0

[PATCH v5 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 | 278 +++++++++++++++++++++++++++++++++++++
 3 files changed, 290 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..85cd7b1ec186
--- /dev/null
+++ b/drivers/iio/adc/max34408.c
@@ -0,0 +1,278 @@
+// 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/mod_devicetable.h>
+#include <linux/property.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 MAX34409_OCT3_REG		0xA
+#define MAX34409_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)
+
+#define MAX34408_DEFAULT_AVG		0x4
+
+/* 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.
+ * @input_rsense:	Rsense values in uOhm, will be overwritten by
+ *			values from channel nodes.
+ */
+struct max34408_data {
+	struct regmap *regmap;
+	struct device *dev;
+	struct mutex lock;
+	u32 input_rsense[4];
+};
+
+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, _address)			\
+	{							\
+		.type = IIO_CURRENT,				\
+		.info_mask_separate	= BIT(IIO_CHAN_INFO_RAW) | \
+					  BIT(IIO_CHAN_INFO_SCALE) | \
+					  BIT(IIO_CHAN_INFO_OFFSET), \
+		.channel = (_index),				\
+		.address = (_address),				\
+		.indexed = 1,					\
+	}
+
+static const struct iio_chan_spec max34408_channels[] = {
+	MAX34008_CHANNEL(0, MAX34408_ADC1_REG),
+	MAX34008_CHANNEL(1, MAX34408_ADC2_REG),
+};
+
+static const struct iio_chan_spec max34409_channels[] = {
+	MAX34008_CHANNEL(0, MAX34408_ADC1_REG),
+	MAX34008_CHANNEL(1, MAX34408_ADC2_REG),
+	MAX34008_CHANNEL(2, MAX34409_ADC3_REG),
+	MAX34008_CHANNEL(3, MAX34409_ADC4_REG),
+};
+
+static int max34408_read_adc_avg(struct max34408_data *max34408,
+				 const struct iio_chan_spec *chan, int *val)
+{
+	unsigned int ctrl;
+	int rc;
+
+	guard(mutex)(&max34408->lock);
+	rc = regmap_read(max34408->regmap, MAX34408_CONTROL_REG, (u32 *)&ctrl);
+	if (rc)
+		return rc;
+
+	/* set averaging (0b100) default values*/
+	rc = regmap_write(max34408->regmap, MAX34408_CONTROL_REG,
+			  MAX34408_DEFAULT_AVG);
+	if (rc) {
+		dev_err(max34408->dev,
+			"Error (%d) writing control register\n", rc);
+		return rc;
+	}
+
+	rc = regmap_read(max34408->regmap, chan->address, val);
+	if (rc)
+		return rc;
+
+	/* back to old values */
+	rc = regmap_write(max34408->regmap, MAX34408_CONTROL_REG, ctrl);
+	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 rc;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		rc = max34408_read_adc_avg(max34408, chan, 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
+		 */
+		*val = 10000 / max34408->input_rsense[chan->channel];
+		*val2 = 8;
+		return IIO_VAL_FRACTIONAL_LOG2;
+	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;
+	struct device *dev = &client->dev;
+	const struct of_device_id *match;
+	struct max34408_data *max34408;
+	struct fwnode_handle *node;
+	struct iio_dev *indio_dev;
+	struct regmap *regmap;
+	int rc, i;
+
+	match = i2c_of_match_device(max34408_of_match, client);
+	if (!match)
+		return -EINVAL;
+	model_data = i2c_get_match_data(client);
+
+	regmap = devm_regmap_init_i2c(client, &max34408_regmap_config);
+	if (IS_ERR(regmap)) {
+		dev_err_probe(dev, PTR_ERR(regmap),
+			      "regmap_init failed\n");
+		return PTR_ERR(regmap);
+	}
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*max34408));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	max34408 = iio_priv(indio_dev);
+	max34408->regmap = regmap;
+	max34408->dev = dev;
+	mutex_init(&max34408->lock);
+
+	device_for_each_child_node(dev, node) {
+		fwnode_property_read_u32(node, "maxim,rsense-val-micro-ohms",
+					 &max34408->input_rsense[i]);
+		i++;
+	}
+
+	/* 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(dev, indio_dev);
+}
+
+static const struct i2c_device_id max34408_id[] = {
+	{ "max34408", (kernel_ulong_t)&max34408_model_data },
+	{ "max34409", (kernel_ulong_t)&max34409_model_data },
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, max34408_id);
+
+static struct i2c_driver max34408_driver = {
+	.driver = {
+		.name   = "max34408",
+		.of_match_table = max34408_of_match,
+	},
+	.probe = max34408_probe,
+	.id_table = max34408_id,
+};
+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 v5 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-rc5 next-20231009]
[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/20231008-074933
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link:    https://lore.kernel.org/r/20231007234838.8748-3-fr0st61te%40gmail.com
patch subject: [PATCH v5 2/2] iio: adc: Add driver support for MAX34408/9
config: powerpc-allmodconfig (https://download.01.org/0day-ci/archive/20231009/202310091704.gr3fx4Vo-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231009/202310091704.gr3fx4Vo-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/202310091704.gr3fx4Vo-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/iio/adc/max34408.c:240:31: warning: variable 'i' is uninitialized when used here [-Wuninitialized]
     240 |                                          &max34408->input_rsense[i]);
         |                                                                  ^
   drivers/iio/adc/max34408.c:215:11: note: initialize the variable 'i' to silence this warning
     215 |         int rc, i;
         |                  ^
         |                   = 0
   1 warning generated.


vim +/i +240 drivers/iio/adc/max34408.c

   205	
   206	static int max34408_probe(struct i2c_client *client)
   207	{
   208		const struct max34408_adc_model_data *model_data;
   209		struct device *dev = &client->dev;
   210		const struct of_device_id *match;
   211		struct max34408_data *max34408;
   212		struct fwnode_handle *node;
   213		struct iio_dev *indio_dev;
   214		struct regmap *regmap;
   215		int rc, i;
   216	
   217		match = i2c_of_match_device(max34408_of_match, client);
   218		if (!match)
   219			return -EINVAL;
   220		model_data = i2c_get_match_data(client);
   221	
   222		regmap = devm_regmap_init_i2c(client, &max34408_regmap_config);
   223		if (IS_ERR(regmap)) {
   224			dev_err_probe(dev, PTR_ERR(regmap),
   225				      "regmap_init failed\n");
   226			return PTR_ERR(regmap);
   227		}
   228	
   229		indio_dev = devm_iio_device_alloc(dev, sizeof(*max34408));
   230		if (!indio_dev)
   231			return -ENOMEM;
   232	
   233		max34408 = iio_priv(indio_dev);
   234		max34408->regmap = regmap;
   235		max34408->dev = dev;
   236		mutex_init(&max34408->lock);
   237	
   238		device_for_each_child_node(dev, node) {
   239			fwnode_property_read_u32(node, "maxim,rsense-val-micro-ohms",
 > 240						 &max34408->input_rsense[i]);
   241			i++;
   242		}
   243	
   244		/* disable ALERT and averaging */
   245		rc = regmap_write(max34408->regmap, MAX34408_CONTROL_REG, 0x0);
   246		if (rc)
   247			return rc;
   248	
   249		indio_dev->channels = model_data->channels;
   250		indio_dev->num_channels = model_data->num_channels;
   251		indio_dev->name = model_data->model_name;
   252	
   253		indio_dev->info = &max34408_info;
   254		indio_dev->modes = INDIO_DIRECT_MODE;
   255	
   256		return devm_iio_device_register(dev, indio_dev);
   257	}
   258	

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

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

[Rob Herring]

On Sun, Oct 08, 2023 at 02:48:37AM +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      | 141 ++++++++++++++++++
>  1 file changed, 141 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..9749f1fd1802
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/maxim,max34408.yaml
> @@ -0,0 +1,141 @@
> +# 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

Maxim MAX34408/MAX34409 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
> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 0
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  powerdown-gpios:
> +    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.
> +    maxItems: 1
> +
> +  shtdn-enable-gpios:
> +    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.
> +    maxItems: 1
> +
> +  vdd-supply: true
> +
> +patternProperties:
> +  "^channel@[0-3]$":
> +    $ref: adc.yaml
> +    type: object
> +    description:
> +      Represents the internal channels of the ADC.
> +
> +    properties:
> +      reg:
> +        items:
> +          minimum: 0
> +          maximum: 3

This allows any number of 'reg' entries. You need this instead:

items:
  - minimum: 0
    maximum: 3


> +
> +      maxim,rsense-val-micro-ohms:
> +        description:
> +          Adjust the Rsense value to monitor higher or lower current levels for
> +          input.
> +        enum: [250, 500, 1000, 5000, 10000, 50000, 100000, 200000, 500000]
> +        default: 1000
> +
> +    required:
> +      - reg
> +      - maxim,rsense-val-micro-ohms
> +
> +    unevaluatedProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: maxim,max34408
> +    then:
> +      patternProperties:
> +        "^channel@[2-3]$": false
> +        "^channel@[0-1]$":
> +          properties:
> +            reg:
> +              minimum: 0

0 is already the minimum

> +              maximum: 1
> +    else:
> +      patternProperties:
> +        "^channel@[0-3]$":
> +          properties:
> +            reg:
> +              minimum: 0

ditto

> +              maximum: 3
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        adc@1e {
> +              compatible = "maxim,max34409";
> +              reg = <0x1e>;
> +              powerdown-gpios = <&gpio0 1 GPIO_ACTIVE_LOW>;
> +              shtdn-enable-gpios = <&gpio0 2 GPIO_ACTIVE_HIGH>;
> +
> +              #address-cells = <1>;
> +              #size-cells = <0>;
> +
> +              channel@0 {
> +                  reg = <0x0>;
> +                  maxim,rsense-val-micro-ohms = <5000>;
> +              };
> +
> +              channel@1 {
> +                  reg = <0x1>;
> +                  maxim,rsense-val-micro-ohms = <10000>;
> +             };
> +        };
> +    };
> -- 
> 2.42.0
> 

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

[Jonathan Cameron]

On Sun,  8 Oct 2023 02:48:37 +0300
Ivan Mikhaylov <fr0st61te@gmail.com> 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      | 141 ++++++++++++++++++
>  1 file changed, 141 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..9749f1fd1802
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/maxim,max34408.yaml
> @@ -0,0 +1,141 @@
> +# 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
> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 0
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  powerdown-gpios:
> +    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.
> +    maxItems: 1
> +
> +  shtdn-enable-gpios:

I guess the review crossed with you sending v5.  There is some feedback on v4 you need
to address here.

> +    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.
> +    maxItems: 1
> +
> +  vdd-supply: true
> +
> +patternProperties:
> +  "^channel@[0-3]$":
> +    $ref: adc.yaml
> +    type: object
> +    description:
> +      Represents the internal channels of the ADC.
> +
> +    properties:
> +      reg:
> +        items:
> +          minimum: 0
> +          maximum: 3
> +
> +      maxim,rsense-val-micro-ohms:
> +        description:
> +          Adjust the Rsense value to monitor higher or lower current levels for
> +          input.
> +        enum: [250, 500, 1000, 5000, 10000, 50000, 100000, 200000, 500000]
> +        default: 1000
> +
> +    required:
> +      - reg
> +      - maxim,rsense-val-micro-ohms
> +
> +    unevaluatedProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: maxim,max34408
> +    then:
> +      patternProperties:
> +        "^channel@[2-3]$": false
> +        "^channel@[0-1]$":
> +          properties:
> +            reg:
> +              minimum: 0
> +              maximum: 1
> +    else:
> +      patternProperties:
> +        "^channel@[0-3]$":
> +          properties:
> +            reg:
> +              minimum: 0
> +              maximum: 3
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        adc@1e {
> +              compatible = "maxim,max34409";
> +              reg = <0x1e>;
> +              powerdown-gpios = <&gpio0 1 GPIO_ACTIVE_LOW>;
> +              shtdn-enable-gpios = <&gpio0 2 GPIO_ACTIVE_HIGH>;
> +
> +              #address-cells = <1>;
> +              #size-cells = <0>;
> +
> +              channel@0 {
> +                  reg = <0x0>;
> +                  maxim,rsense-val-micro-ohms = <5000>;
> +              };
> +
> +              channel@1 {
> +                  reg = <0x1>;
> +                  maxim,rsense-val-micro-ohms = <10000>;
> +             };
> +        };
> +    };

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

[Jonathan Cameron]

On Sun,  8 Oct 2023 02:48:38 +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
> 
Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/MAX34408-MAX34409.pdf
> Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>

A few other comments inline.

Jonathan


> diff --git a/drivers/iio/adc/max34408.c b/drivers/iio/adc/max34408.c
> new file mode 100644
> index 000000000000..85cd7b1ec186
> --- /dev/null
> +++ b/drivers/iio/adc/max34408.c
> @@ -0,0 +1,278 @@


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

Having dropped the unnecessary check in probe, move this to next to the
other tables.

> +
> +static int max34408_probe(struct i2c_client *client)
> +{
> +	const struct max34408_adc_model_data *model_data;
> +	struct device *dev = &client->dev;
> +	const struct of_device_id *match;
> +	struct max34408_data *max34408;
> +	struct fwnode_handle *node;
> +	struct iio_dev *indio_dev;
> +	struct regmap *regmap;
> +	int rc, i;
> +
> +	match = i2c_of_match_device(max34408_of_match, client);

Why check this?  This prevents any other firmware binding being used for no
obvious purpose.  Just check...

> +	if (!match)
> +		return -EINVAL;
> +	model_data = i2c_get_match_data(client);
.. 	if (!model_data)
		return -EINVAL;

instead.

> +
> +	regmap = devm_regmap_init_i2c(client, &max34408_regmap_config);
> +	if (IS_ERR(regmap)) {
> +		dev_err_probe(dev, PTR_ERR(regmap),
> +			      "regmap_init failed\n");
> +		return PTR_ERR(regmap);
> +	}
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*max34408));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	max34408 = iio_priv(indio_dev);
> +	max34408->regmap = regmap;
> +	max34408->dev = dev;
> +	mutex_init(&max34408->lock);
> +
> +	device_for_each_child_node(dev, node) {
> +		fwnode_property_read_u32(node, "maxim,rsense-val-micro-ohms",
> +					 &max34408->input_rsense[i]);
> +		i++;
As 0-day pointed out, i isn't initialized.

> +	}
> +
> +	/* 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(dev, indio_dev);
> +}

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

[Ivan Mikhaylov]

On Tue, 2023-10-10 at 15:40 +0100, Jonathan Cameron wrote:
> On Sun,  8 Oct 2023 02:48:37 +0300
> Ivan Mikhaylov <fr0st61te@gmail.com> 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      | 141
> > ++++++++++++++++++
> >  1 file changed, 141 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..9749f1fd1802
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/maxim,max34408.yaml
> > @@ -0,0 +1,141 @@
> > +# 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
> > +
> > +  "#address-cells":
> > +    const: 1
> > +
> > +  "#size-cells":
> > +    const: 0
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  powerdown-gpios:
> > +    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.
> > +    maxItems: 1
> > +
> > +  shtdn-enable-gpios:
> 
> I guess the review crossed with you sending v5.  There is some
> feedback on v4 you need
> to address here.

Jonathan, I thought I did, I've changed ena to powerdown-gpios from
Krzysztof's comments but about this one pin I'm still not sure, it
looks like *-enable-gpios (like in *-enable-gpios pins in
iio/frequency/adi,adf4377.yaml) pin or is it not? Or maybe any other
suggestions about naming of this one?

Thanks.

> 
> > +    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.
> > +    maxItems: 1
> > +
> > +  vdd-supply: true
> > +
> > +patternProperties:
> > +  "^channel@[0-3]$":
> > +    $ref: adc.yaml
> > +    type: object
> > +    description:
> > +      Represents the internal channels of the ADC.
> > +
> > +    properties:
> > +      reg:
> > +        items:
> > +          minimum: 0
> > +          maximum: 3
> > +
> > +      maxim,rsense-val-micro-ohms:
> > +        description:
> > +          Adjust the Rsense value to monitor higher or lower
> > current levels for
> > +          input.
> > +        enum: [250, 500, 1000, 5000, 10000, 50000, 100000, 200000,
> > 500000]
> > +        default: 1000
> > +
> > +    required:
> > +      - reg
> > +      - maxim,rsense-val-micro-ohms
> > +
> > +    unevaluatedProperties: false
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +allOf:
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: maxim,max34408
> > +    then:
> > +      patternProperties:
> > +        "^channel@[2-3]$": false
> > +        "^channel@[0-1]$":
> > +          properties:
> > +            reg:
> > +              minimum: 0
> > +              maximum: 1
> > +    else:
> > +      patternProperties:
> > +        "^channel@[0-3]$":
> > +          properties:
> > +            reg:
> > +              minimum: 0
> > +              maximum: 3
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/gpio/gpio.h>
> > +
> > +    i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        adc@1e {
> > +              compatible = "maxim,max34409";
> > +              reg = <0x1e>;
> > +              powerdown-gpios = <&gpio0 1 GPIO_ACTIVE_LOW>;
> > +              shtdn-enable-gpios = <&gpio0 2 GPIO_ACTIVE_HIGH>;
> > +
> > +              #address-cells = <1>;
> > +              #size-cells = <0>;
> > +
> > +              channel@0 {
> > +                  reg = <0x0>;
> > +                  maxim,rsense-val-micro-ohms = <5000>;
> > +              };
> > +
> > +              channel@1 {
> > +                  reg = <0x1>;
> > +                  maxim,rsense-val-micro-ohms = <10000>;
> > +             };
> > +        };
> > +    };
> 

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

[Jonathan Cameron]

On Tue, 10 Oct 2023 23:22:48 +0300
Ivan Mikhaylov <fr0st61te@gmail.com> wrote:

> On Tue, 2023-10-10 at 15:40 +0100, Jonathan Cameron wrote:
> > On Sun,  8 Oct 2023 02:48:37 +0300
> > Ivan Mikhaylov <fr0st61te@gmail.com> 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      | 141
> > > ++++++++++++++++++
> > >  1 file changed, 141 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..9749f1fd1802
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/iio/adc/maxim,max34408.yaml
> > > @@ -0,0 +1,141 @@
> > > +# 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
> > > +
> > > +  "#address-cells":
> > > +    const: 1
> > > +
> > > +  "#size-cells":
> > > +    const: 0
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  interrupts:
> > > +    maxItems: 1
> > > +
> > > +  powerdown-gpios:
> > > +    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.
> > > +    maxItems: 1
> > > +
> > > +  shtdn-enable-gpios:  
> > 
> > I guess the review crossed with you sending v5.  There is some
> > feedback on v4 you need
> > to address here.  
> 
> Jonathan, I thought I did, I've changed ena to powerdown-gpios from
> Krzysztof's comments but about this one pin I'm still not sure, it
> looks like *-enable-gpios (like in *-enable-gpios pins in
> iio/frequency/adi,adf4377.yaml) pin or is it not? Or maybe any other
> suggestions about naming of this one?
> 
> Thanks.

shutdown-gpios and make the sense (active high / low) such that setting
it results in teh device being shut down.
Or treat it as an enable and enable-gpios

Something that indicates both shutdown and enable is confusing ;)

Jonathan


> 
> >   
> > > +    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.
> > > +    maxItems: 1
> > > +
> > > +  vdd-supply: true
> > > +
> > > +patternProperties:
> > > +  "^channel@[0-3]$":
> > > +    $ref: adc.yaml
> > > +    type: object
> > > +    description:
> > > +      Represents the internal channels of the ADC.
> > > +
> > > +    properties:
> > > +      reg:
> > > +        items:
> > > +          minimum: 0
> > > +          maximum: 3
> > > +
> > > +      maxim,rsense-val-micro-ohms:
> > > +        description:
> > > +          Adjust the Rsense value to monitor higher or lower
> > > current levels for
> > > +          input.
> > > +        enum: [250, 500, 1000, 5000, 10000, 50000, 100000, 200000,
> > > 500000]
> > > +        default: 1000
> > > +
> > > +    required:
> > > +      - reg
> > > +      - maxim,rsense-val-micro-ohms
> > > +
> > > +    unevaluatedProperties: false
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +
> > > +allOf:
> > > +  - if:
> > > +      properties:
> > > +        compatible:
> > > +          contains:
> > > +            const: maxim,max34408
> > > +    then:
> > > +      patternProperties:
> > > +        "^channel@[2-3]$": false
> > > +        "^channel@[0-1]$":
> > > +          properties:
> > > +            reg:
> > > +              minimum: 0
> > > +              maximum: 1
> > > +    else:
> > > +      patternProperties:
> > > +        "^channel@[0-3]$":
> > > +          properties:
> > > +            reg:
> > > +              minimum: 0
> > > +              maximum: 3
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +    #include <dt-bindings/gpio/gpio.h>
> > > +
> > > +    i2c {
> > > +        #address-cells = <1>;
> > > +        #size-cells = <0>;
> > > +
> > > +        adc@1e {
> > > +              compatible = "maxim,max34409";
> > > +              reg = <0x1e>;
> > > +              powerdown-gpios = <&gpio0 1 GPIO_ACTIVE_LOW>;
> > > +              shtdn-enable-gpios = <&gpio0 2 GPIO_ACTIVE_HIGH>;
> > > +
> > > +              #address-cells = <1>;
> > > +              #size-cells = <0>;
> > > +
> > > +              channel@0 {
> > > +                  reg = <0x0>;
> > > +                  maxim,rsense-val-micro-ohms = <5000>;
> > > +              };
> > > +
> > > +              channel@1 {
> > > +                  reg = <0x1>;
> > > +                  maxim,rsense-val-micro-ohms = <10000>;
> > > +             };
> > > +        };
> > > +    };  
> >   
> 

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

[Ivan Mikhaylov]

On Thu, 2023-10-12 at 08:40 +0100, Jonathan Cameron wrote:
> On Tue, 10 Oct 2023 23:22:48 +0300
> Ivan Mikhaylov <fr0st61te@gmail.com> wrote:
> 
> > On Tue, 2023-10-10 at 15:40 +0100, Jonathan Cameron wrote:
> > > On Sun,  8 Oct 2023 02:48:37 +0300
> > > Ivan Mikhaylov <fr0st61te@gmail.com> 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      | 141
> > > > ++++++++++++++++++
> > > >  1 file changed, 141 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..9749f1fd1802
> > > > --- /dev/null
> > > > +++
> > > > b/Documentation/devicetree/bindings/iio/adc/maxim,max34408.yaml
> > > > @@ -0,0 +1,141 @@
> > > > +# 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
> > > > +
> > > > +  "#address-cells":
> > > > +    const: 1
> > > > +
> > > > +  "#size-cells":
> > > > +    const: 0
> > > > +
> > > > +  reg:
> > > > +    maxItems: 1
> > > > +
> > > > +  interrupts:
> > > > +    maxItems: 1
> > > > +
> > > > +  powerdown-gpios:
> > > > +    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.
> > > > +    maxItems: 1
> > > > +
> > > > +  shtdn-enable-gpios:  
> > > 
> > > I guess the review crossed with you sending v5.  There is some
> > > feedback on v4 you need
> > > to address here.  
> > 
> > Jonathan, I thought I did, I've changed ena to powerdown-gpios from
> > Krzysztof's comments but about this one pin I'm still not sure, it
> > looks like *-enable-gpios (like in *-enable-gpios pins in
> > iio/frequency/adi,adf4377.yaml) pin or is it not? Or maybe any
> > other
> > suggestions about naming of this one?
> > 
> > Thanks.
> 
> shutdown-gpios and make the sense (active high / low) such that
> setting
> it results in teh device being shut down.
> Or treat it as an enable and enable-gpios
> 
> Something that indicates both shutdown and enable is confusing ;)
> 
> Jonathan


Jonathan, then I make these changes:

powerdown-gpios: -> output-enable:
shtdn-enable-gpios: -> enable-gpios:

Is it ok?

Thanks.

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

[Jonathan Cameron]

On Thu, 12 Oct 2023 19:27:33 +0300
Ivan Mikhaylov <fr0st61te@gmail.com> wrote:

> On Thu, 2023-10-12 at 08:40 +0100, Jonathan Cameron wrote:
> > On Tue, 10 Oct 2023 23:22:48 +0300
> > Ivan Mikhaylov <fr0st61te@gmail.com> wrote:
> >   
> > > On Tue, 2023-10-10 at 15:40 +0100, Jonathan Cameron wrote:  
> > > > On Sun,  8 Oct 2023 02:48:37 +0300
> > > > Ivan Mikhaylov <fr0st61te@gmail.com> 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      | 141
> > > > > ++++++++++++++++++
> > > > >  1 file changed, 141 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..9749f1fd1802
> > > > > --- /dev/null
> > > > > +++
> > > > > b/Documentation/devicetree/bindings/iio/adc/maxim,max34408.yaml
> > > > > @@ -0,0 +1,141 @@
> > > > > +# 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
> > > > > +
> > > > > +  "#address-cells":
> > > > > +    const: 1
> > > > > +
> > > > > +  "#size-cells":
> > > > > +    const: 0
> > > > > +
> > > > > +  reg:
> > > > > +    maxItems: 1
> > > > > +
> > > > > +  interrupts:
> > > > > +    maxItems: 1
> > > > > +
> > > > > +  powerdown-gpios:
> > > > > +    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.
> > > > > +    maxItems: 1
> > > > > +
> > > > > +  shtdn-enable-gpios:    
> > > > 
> > > > I guess the review crossed with you sending v5.  There is some
> > > > feedback on v4 you need
> > > > to address here.    
> > > 
> > > Jonathan, I thought I did, I've changed ena to powerdown-gpios from
> > > Krzysztof's comments but about this one pin I'm still not sure, it
> > > looks like *-enable-gpios (like in *-enable-gpios pins in
> > > iio/frequency/adi,adf4377.yaml) pin or is it not? Or maybe any
> > > other
> > > suggestions about naming of this one?
> > > 
> > > Thanks.  
> > 
> > shutdown-gpios and make the sense (active high / low) such that
> > setting
> > it results in teh device being shut down.
> > Or treat it as an enable and enable-gpios
> > 
> > Something that indicates both shutdown and enable is confusing ;)
> > 
> > Jonathan  
> 
> 
> Jonathan, then I make these changes:
> 
> powerdown-gpios: -> output-enable:
Needs to retain the gpios bit as we want the standard gpio stuff to pick
them up. I'm not that keen on output-enable-gpios though.  The activity
here is very much 'shutdown because of error or not enabled' I think.
So perhaps we flip the sense and document that it needs to be active low?

> shtdn-enable-gpios: -> enable-gpios:
> 
> Is it ok?

Conor, Rob, Krzysztof - you probably have a better insight into this than
I do.

Thanks,

Jonathan

> 
> Thanks.
> 

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

[Krzysztof Kozlowski]

On 13/10/2023 10:19, Jonathan Cameron wrote:
> On Thu, 12 Oct 2023 19:27:33 +0300
> Ivan Mikhaylov <fr0st61te@gmail.com> wrote:
> 
>> On Thu, 2023-10-12 at 08:40 +0100, Jonathan Cameron wrote:
>>> On Tue, 10 Oct 2023 23:22:48 +0300
>>> Ivan Mikhaylov <fr0st61te@gmail.com> wrote:
>>>   
>>>> On Tue, 2023-10-10 at 15:40 +0100, Jonathan Cameron wrote:  
>>>>> On Sun,  8 Oct 2023 02:48:37 +0300
>>>>> Ivan Mikhaylov <fr0st61te@gmail.com> 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      | 141
>>>>>> ++++++++++++++++++
>>>>>>  1 file changed, 141 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..9749f1fd1802
>>>>>> --- /dev/null
>>>>>> +++
>>>>>> b/Documentation/devicetree/bindings/iio/adc/maxim,max34408.yaml
>>>>>> @@ -0,0 +1,141 @@
>>>>>> +# 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
>>>>>> +
>>>>>> +  "#address-cells":
>>>>>> +    const: 1
>>>>>> +
>>>>>> +  "#size-cells":
>>>>>> +    const: 0
>>>>>> +
>>>>>> +  reg:
>>>>>> +    maxItems: 1
>>>>>> +
>>>>>> +  interrupts:
>>>>>> +    maxItems: 1
>>>>>> +
>>>>>> +  powerdown-gpios:
>>>>>> +    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.
>>>>>> +    maxItems: 1
>>>>>> +
>>>>>> +  shtdn-enable-gpios:    
>>>>>
>>>>> I guess the review crossed with you sending v5.  There is some
>>>>> feedback on v4 you need
>>>>> to address here.    
>>>>
>>>> Jonathan, I thought I did, I've changed ena to powerdown-gpios from
>>>> Krzysztof's comments but about this one pin I'm still not sure, it
>>>> looks like *-enable-gpios (like in *-enable-gpios pins in
>>>> iio/frequency/adi,adf4377.yaml) pin or is it not? Or maybe any
>>>> other
>>>> suggestions about naming of this one?
>>>>
>>>> Thanks.  
>>>
>>> shutdown-gpios and make the sense (active high / low) such that
>>> setting
>>> it results in teh device being shut down.
>>> Or treat it as an enable and enable-gpios
>>>
>>> Something that indicates both shutdown and enable is confusing ;)
>>>
>>> Jonathan  
>>
>>
>> Jonathan, then I make these changes:
>>
>> powerdown-gpios: -> output-enable:
> Needs to retain the gpios bit as we want the standard gpio stuff to pick
> them up. I'm not that keen on output-enable-gpios though.  The activity
> here is very much 'shutdown because of error or not enabled' I think.
> So perhaps we flip the sense and document that it needs to be active low?
> 
>> shtdn-enable-gpios: -> enable-gpios:
>>
>> Is it ok?
> 
> Conor, Rob, Krzysztof - you probably have a better insight into this than
> I do.
> 

"enable-gpios" are for turning on a specific feature, not powering
on/off entire device. For example to enable regulator output.

"powerdown-gpios" are for turning device on/off.

I don't know what do you have in your device.

Best regards,
Krzysztof

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

[Jonathan Cameron]

On Fri, 13 Oct 2023 10:35:49 +0200
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 13/10/2023 10:19, Jonathan Cameron wrote:
> > On Thu, 12 Oct 2023 19:27:33 +0300
> > Ivan Mikhaylov <fr0st61te@gmail.com> wrote:
> >   
> >> On Thu, 2023-10-12 at 08:40 +0100, Jonathan Cameron wrote:  
> >>> On Tue, 10 Oct 2023 23:22:48 +0300
> >>> Ivan Mikhaylov <fr0st61te@gmail.com> wrote:
> >>>     
> >>>> On Tue, 2023-10-10 at 15:40 +0100, Jonathan Cameron wrote:    
> >>>>> On Sun,  8 Oct 2023 02:48:37 +0300
> >>>>> Ivan Mikhaylov <fr0st61te@gmail.com> 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      | 141
> >>>>>> ++++++++++++++++++
> >>>>>>  1 file changed, 141 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..9749f1fd1802
> >>>>>> --- /dev/null
> >>>>>> +++
> >>>>>> b/Documentation/devicetree/bindings/iio/adc/maxim,max34408.yaml
> >>>>>> @@ -0,0 +1,141 @@
> >>>>>> +# 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
> >>>>>> +
> >>>>>> +  "#address-cells":
> >>>>>> +    const: 1
> >>>>>> +
> >>>>>> +  "#size-cells":
> >>>>>> +    const: 0
> >>>>>> +
> >>>>>> +  reg:
> >>>>>> +    maxItems: 1
> >>>>>> +
> >>>>>> +  interrupts:
> >>>>>> +    maxItems: 1
> >>>>>> +
> >>>>>> +  powerdown-gpios:
> >>>>>> +    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.
> >>>>>> +    maxItems: 1
> >>>>>> +
> >>>>>> +  shtdn-enable-gpios:      
> >>>>>
> >>>>> I guess the review crossed with you sending v5.  There is some
> >>>>> feedback on v4 you need
> >>>>> to address here.      
> >>>>
> >>>> Jonathan, I thought I did, I've changed ena to powerdown-gpios from
> >>>> Krzysztof's comments but about this one pin I'm still not sure, it
> >>>> looks like *-enable-gpios (like in *-enable-gpios pins in
> >>>> iio/frequency/adi,adf4377.yaml) pin or is it not? Or maybe any
> >>>> other
> >>>> suggestions about naming of this one?
> >>>>
> >>>> Thanks.    
> >>>
> >>> shutdown-gpios and make the sense (active high / low) such that
> >>> setting
> >>> it results in teh device being shut down.
> >>> Or treat it as an enable and enable-gpios
> >>>
> >>> Something that indicates both shutdown and enable is confusing ;)
> >>>
> >>> Jonathan    
> >>
> >>
> >> Jonathan, then I make these changes:
> >>
> >> powerdown-gpios: -> output-enable:  
> > Needs to retain the gpios bit as we want the standard gpio stuff to pick
> > them up. I'm not that keen on output-enable-gpios though.  The activity
> > here is very much 'shutdown because of error or not enabled' I think.
> > So perhaps we flip the sense and document that it needs to be active low?
> >   
> >> shtdn-enable-gpios: -> enable-gpios:
> >>
> >> Is it ok?  
> > 
> > Conor, Rob, Krzysztof - you probably have a better insight into this than
> > I do.
> >   
> 
> "enable-gpios" are for turning on a specific feature, not powering
> on/off entire device. For example to enable regulator output.
> 
> "powerdown-gpios" are for turning device on/off.
> 
> I don't know what do you have in your device.
Ok. Sounds like that what is enable-gpios above should be shutdown-gpios.

The other case is a device output indicating whether the device is
shutdown.  That can happen because it was told to do so (via the other gpio),
or because it is in an error state. What's a good naming convention for that?

Thanks,

Jonathan

> 
> Best regards,
> Krzysztof
> 
> 

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

[Krzysztof Kozlowski]

On 13/10/2023 11:09, Jonathan Cameron wrote:
>>>>>>>> +  shtdn-enable-gpios:      
>>>>>>>
>>>>>>> I guess the review crossed with you sending v5.  There is some
>>>>>>> feedback on v4 you need
>>>>>>> to address here.      
>>>>>>
>>>>>> Jonathan, I thought I did, I've changed ena to powerdown-gpios from
>>>>>> Krzysztof's comments but about this one pin I'm still not sure, it
>>>>>> looks like *-enable-gpios (like in *-enable-gpios pins in
>>>>>> iio/frequency/adi,adf4377.yaml) pin or is it not? Or maybe any
>>>>>> other
>>>>>> suggestions about naming of this one?
>>>>>>
>>>>>> Thanks.    
>>>>>
>>>>> shutdown-gpios and make the sense (active high / low) such that
>>>>> setting
>>>>> it results in teh device being shut down.
>>>>> Or treat it as an enable and enable-gpios
>>>>>
>>>>> Something that indicates both shutdown and enable is confusing ;)
>>>>>
>>>>> Jonathan    
>>>>
>>>>
>>>> Jonathan, then I make these changes:
>>>>
>>>> powerdown-gpios: -> output-enable:  
>>> Needs to retain the gpios bit as we want the standard gpio stuff to pick
>>> them up. I'm not that keen on output-enable-gpios though.  The activity
>>> here is very much 'shutdown because of error or not enabled' I think.
>>> So perhaps we flip the sense and document that it needs to be active low?
>>>   
>>>> shtdn-enable-gpios: -> enable-gpios:
>>>>
>>>> Is it ok?  
>>>
>>> Conor, Rob, Krzysztof - you probably have a better insight into this than
>>> I do.
>>>   
>>
>> "enable-gpios" are for turning on a specific feature, not powering
>> on/off entire device. For example to enable regulator output.
>>
>> "powerdown-gpios" are for turning device on/off.
>>
>> I don't know what do you have in your device.
> Ok. Sounds like that what is enable-gpios above should be shutdown-gpios.

shutdown-gpios sounds exactly the same as powerdown-gpios and it is
already used in exactly same context.

> The other case is a device output indicating whether the device is
> shutdown.  That can happen because it was told to do so (via the other gpio),
> or because it is in an error state. What's a good naming convention for that?

There is no convention and I did not see such case so far.
powerdown-status-gpios? powerdown-state-gpios?



Best regards,
Krzysztof

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

[Jonathan Cameron]

On Fri, 13 Oct 2023 11:53:33 +0200
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 13/10/2023 11:09, Jonathan Cameron wrote:
> >>>>>>>> +  shtdn-enable-gpios:        
> >>>>>>>
> >>>>>>> I guess the review crossed with you sending v5.  There is some
> >>>>>>> feedback on v4 you need
> >>>>>>> to address here.        
> >>>>>>
> >>>>>> Jonathan, I thought I did, I've changed ena to powerdown-gpios from
> >>>>>> Krzysztof's comments but about this one pin I'm still not sure, it
> >>>>>> looks like *-enable-gpios (like in *-enable-gpios pins in
> >>>>>> iio/frequency/adi,adf4377.yaml) pin or is it not? Or maybe any
> >>>>>> other
> >>>>>> suggestions about naming of this one?
> >>>>>>
> >>>>>> Thanks.      
> >>>>>
> >>>>> shutdown-gpios and make the sense (active high / low) such that
> >>>>> setting
> >>>>> it results in teh device being shut down.
> >>>>> Or treat it as an enable and enable-gpios
> >>>>>
> >>>>> Something that indicates both shutdown and enable is confusing ;)
> >>>>>
> >>>>> Jonathan      
> >>>>
> >>>>
> >>>> Jonathan, then I make these changes:
> >>>>
> >>>> powerdown-gpios: -> output-enable:    
> >>> Needs to retain the gpios bit as we want the standard gpio stuff to pick
> >>> them up. I'm not that keen on output-enable-gpios though.  The activity
> >>> here is very much 'shutdown because of error or not enabled' I think.
> >>> So perhaps we flip the sense and document that it needs to be active low?
> >>>     
> >>>> shtdn-enable-gpios: -> enable-gpios:
> >>>>
> >>>> Is it ok?    
> >>>
> >>> Conor, Rob, Krzysztof - you probably have a better insight into this than
> >>> I do.
> >>>     
> >>
> >> "enable-gpios" are for turning on a specific feature, not powering
> >> on/off entire device. For example to enable regulator output.
> >>
> >> "powerdown-gpios" are for turning device on/off.
> >>
> >> I don't know what do you have in your device.  
> > Ok. Sounds like that what is enable-gpios above should be shutdown-gpios.  
> 
> shutdown-gpios sounds exactly the same as powerdown-gpios and it is
> already used in exactly same context.
Oops. Yup. powerdown-gpios seems appropriate.
> 
> > The other case is a device output indicating whether the device is
> > shutdown.  That can happen because it was told to do so (via the other gpio),
> > or because it is in an error state. What's a good naming convention for that?  
> 
> There is no convention and I did not see such case so far.
> powerdown-status-gpios? powerdown-state-gpios?
Either seems reasonable.

Thanks,

J
> 
> 
> 
> Best regards,
> Krzysztof
>