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

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

[Ivan Mikhaylov]

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

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

-- 
2.42.0

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

[Ivan Mikhaylov]

The i2c driver with Rsense option for current monitoring.

Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
---
 .../bindings/iio/adc/maxim,max34408.yaml      | 63 +++++++++++++++++++
 1 file changed, 63 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..ae7c6ddb13d8
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/maxim,max34408.yaml
@@ -0,0 +1,63 @@
+# 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,rsense-val-micro-ohms:
+    description:
+      Adjust the Rsense value to monitor higher or lower current levels.
+    enum: [250, 500, 1000, 5000, 10000, 50000, 100000, 200000, 500000]
+    default: 1000
+
+required:
+  - compatible
+  - reg
+  - maxim,rsense-val-micro-ohms
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        adc@1e {
+              compatible = "maxim,max34409";
+              reg = <0x1e>;
+              maxim,rsense-val-micro-ohms = <1000>;
+        };
+    };
-- 
2.42.0

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

[Rob Herring]

On Mon, Sep 18, 2023 at 12:11:42AM +0300, Ivan Mikhaylov wrote:
> The i2c driver with Rsense option for current monitoring.

driver? This is a binding for a hardware device.

> 
> Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
> ---
>  .../bindings/iio/adc/maxim,max34408.yaml      | 63 +++++++++++++++++++
>  1 file changed, 63 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..ae7c6ddb13d8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/maxim,max34408.yaml
> @@ -0,0 +1,63 @@
> +# 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.

Drop the period.

With those fixes:

Reviewed-by: Rob Herring <robh@kernel.org>

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

[Jonathan Cameron]

On Mon, 18 Sep 2023 00:11:42 +0300
Ivan Mikhaylov <fr0st61te@gmail.com> wrote:

> The i2c driver with Rsense option for current monitoring.
> 
> Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>

Hi Ivan,

Welcome to IIO!

Looks good, but there are a few things I'd add to make this describe the device
a little more fully and flexibly.  Ideally we want a binding to fully describe
a device, even if the particular driver for Linux doesn't use all the features.
Some are easy though such as enabling regulators (that are probably turned on
already on your board)

Thanks,

Jonathan

> ---
>  .../bindings/iio/adc/maxim,max34408.yaml      | 63 +++++++++++++++++++
>  1 file changed, 63 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..ae7c6ddb13d8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/maxim,max34408.yaml
> @@ -0,0 +1,63 @@
> +# 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,rsense-val-micro-ohms:
From the datasheet you link, it looks like this could be different for
the inputs?

> +    description:
> +      Adjust the Rsense value to monitor higher or lower current levels.
> +    enum: [250, 500, 1000, 5000, 10000, 50000, 100000, 200000, 500000]

These come from Table 18 which is example values I think?  Not sure there
is anything limiting us to those particular values given the equation given
just above that table should apply more generally.

> +    default: 1000

Please add regulator definitions.

supply-vdd: true
and add it to the required properties. It might be provided by a stub regulator
but we still list that as required.

Also good to add bindings for the other control pins that might be wired to be
in the binding from the start - no need for the driver to use them though.
Looks like we have SHTDN and ENA here that could be wired to GPIOs on the host.

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

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

[Ivan Mikhaylov]

On Sun, 2023-09-24 at 13:53 +0100, Jonathan Cameron wrote:
> On Mon, 18 Sep 2023 00:11:42 +0300
> Ivan Mikhaylov <fr0st61te@gmail.com> wrote:
> 
> > The i2c driver with Rsense option for current monitoring.
> > 
> > Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
> 
> Hi Ivan,
> 
> Welcome to IIO!
> 
> Looks good, but there are a few things I'd add to make this describe
> the device
> a little more fully and flexibly.  Ideally we want a binding to fully
> describe
> a device, even if the particular driver for Linux doesn't use all the
> features.
> Some are easy though such as enabling regulators (that are probably
> turned on
> already on your board)
> 
> Thanks,
> 
> Jonathan
> 
> > ---
> >  .../bindings/iio/adc/maxim,max34408.yaml      | 63
> > +++++++++++++++++++
> >  1 file changed, 63 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..ae7c6ddb13d8
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/maxim,max34408.yaml
> > @@ -0,0 +1,63 @@
> > +# 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,rsense-val-micro-ohms:
> From the datasheet you link, it looks like this could be different
> for
> the inputs?

Hi Jonathan, "maxim,input1-rsense-val-micro-ohms", "maxim,input2-
rsense-val-micro-ohms" and etc would be better?

> 
> > +    description:
> > +      Adjust the Rsense value to monitor higher or lower current
> > levels.
> > +    enum: [250, 500, 1000, 5000, 10000, 50000, 100000, 200000,
> > 500000]
> 
> These come from Table 18 which is example values I think?  Not sure
> there
> is anything limiting us to those particular values given the equation
> given
> just above that table should apply more generally.
> 
> > +    default: 1000
> 
> Please add regulator definitions.
> 
> supply-vdd: true
> and add it to the required properties. It might be provided by a stub
> regulator
> but we still list that as required.
> 
> Also good to add bindings for the other control pins that might be
> wired to be
> in the binding from the start - no need for the driver to use them
> though.
> Looks like we have SHTDN and ENA here that could be wired to GPIOs on
> the host.
> 
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - maxim,rsense-val-micro-ohms
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    i2c {
> > +
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        adc@1e {
> > +              compatible = "maxim,max34409";
> > +              reg = <0x1e>;
> > +              maxim,rsense-val-micro-ohms = <1000>;
> > +        };
> > +    };
> 

Rob, Jonathan, thanks for review, will do the changes which you asked.

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

[Jonathan Cameron]

On Mon, 25 Sep 2023 19:48:39 +0300
Ivan Mikhaylov <fr0st61te@gmail.com> wrote:

> On Sun, 2023-09-24 at 13:53 +0100, Jonathan Cameron wrote:
> > On Mon, 18 Sep 2023 00:11:42 +0300
> > Ivan Mikhaylov <fr0st61te@gmail.com> wrote:
> >   
> > > The i2c driver with Rsense option for current monitoring.
> > > 
> > > Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>  
> > 
> > Hi Ivan,
> > 
> > Welcome to IIO!
> > 
> > Looks good, but there are a few things I'd add to make this describe
> > the device
> > a little more fully and flexibly.  Ideally we want a binding to fully
> > describe
> > a device, even if the particular driver for Linux doesn't use all the
> > features.
> > Some are easy though such as enabling regulators (that are probably
> > turned on
> > already on your board)
> > 
> > Thanks,
> > 
> > Jonathan
> >   
> > > ---
> > >  .../bindings/iio/adc/maxim,max34408.yaml      | 63
> > > +++++++++++++++++++
> > >  1 file changed, 63 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..ae7c6ddb13d8
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/iio/adc/maxim,max34408.yaml
> > > @@ -0,0 +1,63 @@
> > > +# 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,rsense-val-micro-ohms:  
> > From the datasheet you link, it looks like this could be different
> > for
> > the inputs?  
> 
> Hi Jonathan, "maxim,input1-rsense-val-micro-ohms", "maxim,input2-
> rsense-val-micro-ohms" and etc would be better?

Sorry, missed this during the week (too many emails at work!)

Anyhow, I'd have suggested an array, but a better suggestion was
made anyway in reply to your v2.

[PATCH 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..538b086ed593 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -756,6 +756,17 @@ config MAX9611
 	  To compile this driver as a module, choose M here: the module will be
 	  called max9611.
 
+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 MCP320X
 	tristate "Microchip Technology MCP3x01/02/04/08 and MCP3550/1/3"
 	depends on SPI
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 2facf979327d..8fee08546bcc 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -69,6 +69,7 @@ obj-$(CONFIG_MAX1241) += max1241.o
 obj-$(CONFIG_MAX1363) += max1363.o
 obj-$(CONFIG_MAX77541_ADC) += max77541-adc.o
 obj-$(CONFIG_MAX9611) += max9611.o
+obj-$(CONFIG_MAX34408) += max34408.o
 obj-$(CONFIG_MCP320X) += mcp320x.o
 obj-$(CONFIG_MCP3422) += mcp3422.o
 obj-$(CONFIG_MCP3911) += mcp3911.o
diff --git a/drivers/iio/adc/max34408.c b/drivers/iio/adc/max34408.c
new file mode 100644
index 000000000000..96c1de59edb5
--- /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/regmap.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/types.h>
+
+#define MAX34408_STATUS		0x0
+#define MAX34408_CONTROL	0x1
+#define MAX34408_OCDELAY	0x2
+#define MAX34408_SDDELAY	0x3
+
+#define MAX34408_ADC1		0x4
+#define MAX34408_ADC2		0x5
+/* ADC3 & ADC4 always returns 0x0 on 34408 */
+#define MAX34408_ADC3		0x6
+#define MAX34408_ADC4		0x7
+
+#define MAX34408_OCT1		0x8
+#define MAX34408_OCT2		0x9
+#define MAX34408_OCT3		0xA
+#define MAX34408_OCT4		0xB
+
+#define MAX34408_DID		0xC
+#define MAX34408_DCYY		0xD
+#define MAX34408_DCWW		0xE
+
+#define MAX34408_CHANNELS	2
+#define MAX34409_CHANNELS	4
+
+/* Bit masks for status register */
+#define STATUS_OC1		BIT(0)
+#define STATUS_OC2		BIT(1)
+/* OC3 & OC4 only for max34409 */
+#define STATUS_OC3		BIT(2)
+#define STATUS_OC4		BIT(3)
+#define STATUS_SHTDN		BIT(4)
+#define STATUS_ENA		BIT(5)
+
+/* Bit masks for control register */
+#define CONTROL_AVG0		BIT(0)
+#define CONTROL_AVG1		BIT(1)
+#define CONTROL_AVG2		BIT(2)
+#define CONTROL_ALERT		BIT(3)
+
+/* Bit masks for over current delay */
+#define OCDELAY_OCD0		BIT(0)
+#define OCDELAY_OCD1		BIT(1)
+#define OCDELAY_OCD2		BIT(2)
+#define OCDELAY_OCD3		BIT(3)
+#define OCDELAY_OCD4		BIT(4)
+#define OCDELAY_OCD5		BIT(5)
+#define OCDELAY_OCD6		BIT(6)
+#define OCDELAY_RESET		BIT(7)
+
+/* Bit masks for shutdown delay */
+#define SDDELAY_SHD0		BIT(0)
+#define SDDELAY_SHD1		BIT(1)
+#define SDDELAY_SHD2		BIT(2)
+#define SDDELAY_SHD3		BIT(3)
+#define SDDELAY_SHD4		BIT(4)
+#define SDDELAY_SHD5		BIT(5)
+#define SDDELAY_SHD6		BIT(6)
+#define SDDELAY_RESET		BIT(7)
+
+/**
+ * struct max34408_data - max34408/max34409 specific data.
+ * @regmap:	device register map.
+ * @dev:	max34408 device.
+ * @lock:	lock for protecting access to device hardware registers.
+ * @rsense:	Rsense value in uOhm.
+ */
+struct max34408_data {
+	struct regmap *regmap;
+	struct device *dev;
+	struct mutex lock;
+	u32 rsense;
+};
+
+static const struct regmap_config max34408_regmap_config = {
+	.reg_bits	= 8,
+	.val_bits	= 8,
+	.max_register	= MAX34408_DCWW,
+};
+
+static const struct iio_chan_spec max34408_channels[] = {
+	{
+		.type			= IIO_CURRENT,
+		.info_mask_separate	= BIT(IIO_CHAN_INFO_RAW) |
+					  BIT(IIO_CHAN_INFO_PROCESSED) |
+					  BIT(IIO_CHAN_INFO_AVERAGE_RAW),
+		.channel		= 0,
+		.indexed = 1,
+	},
+	{
+		.type			= IIO_CURRENT,
+		.info_mask_separate	= BIT(IIO_CHAN_INFO_RAW) |
+					  BIT(IIO_CHAN_INFO_PROCESSED) |
+					  BIT(IIO_CHAN_INFO_AVERAGE_RAW),
+		.channel		= 1,
+		.indexed = 1,
+	},
+};
+
+static const struct iio_chan_spec max34409_channels[] = {
+	{
+		.type			= IIO_CURRENT,
+		.info_mask_separate	= BIT(IIO_CHAN_INFO_RAW) |
+					  BIT(IIO_CHAN_INFO_PROCESSED) |
+					  BIT(IIO_CHAN_INFO_AVERAGE_RAW),
+		.channel		= 0,
+		.indexed = 1,
+	},
+	{
+		.type			= IIO_CURRENT,
+		.info_mask_separate	= BIT(IIO_CHAN_INFO_RAW) |
+					  BIT(IIO_CHAN_INFO_PROCESSED) |
+					  BIT(IIO_CHAN_INFO_AVERAGE_RAW),
+		.channel		= 1,
+		.indexed = 1,
+	},
+	{
+		.type			= IIO_CURRENT,
+		.info_mask_separate	= BIT(IIO_CHAN_INFO_RAW) |
+					  BIT(IIO_CHAN_INFO_PROCESSED) |
+					  BIT(IIO_CHAN_INFO_AVERAGE_RAW),
+		.channel		= 2,
+		.indexed = 1,
+	},
+	{
+		.type			= IIO_CURRENT,
+		.info_mask_separate	= BIT(IIO_CHAN_INFO_RAW) |
+					  BIT(IIO_CHAN_INFO_PROCESSED) |
+					  BIT(IIO_CHAN_INFO_AVERAGE_RAW),
+		.channel		= 3,
+		.indexed = 1,
+	},
+};
+
+static int max34408_read_adc(struct max34408_data *max34408, int channel,
+			     int *val)
+{
+	int rc;
+	u32 adc_reg;
+
+	switch (channel) {
+	case 0:
+		adc_reg = MAX34408_ADC1;
+		break;
+	case 1:
+		adc_reg = MAX34408_ADC2;
+		break;
+	case 2:
+		adc_reg = MAX34408_ADC3;
+		break;
+	case 3:
+		adc_reg = MAX34408_ADC4;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	rc = regmap_read(max34408->regmap, adc_reg, val);
+	if (rc)
+		return rc;
+
+	return 0;
+}
+
+static int max34408_read_adc_avg(struct max34408_data *max34408, int channel, int *val)
+{
+	unsigned long ctrl;
+	int rc;
+	u8 tmp;
+
+	mutex_lock(&max34408->lock);
+	rc = regmap_read(max34408->regmap, MAX34408_CONTROL, (u32 *)&ctrl);
+	if (rc)
+		goto err_unlock;
+
+	/* set averaging (0b100) default values*/
+	tmp = ctrl;
+	set_bit(CONTROL_AVG2, &ctrl);
+	clear_bit(CONTROL_AVG1, &ctrl);
+	clear_bit(CONTROL_AVG0, &ctrl);
+	rc = regmap_write(max34408->regmap, MAX34408_CONTROL, ctrl);
+	if (rc) {
+		dev_err(max34408->dev,
+			"Error (%d) writing control register\n", rc);
+		goto err_unlock;
+	}
+
+	rc = max34408_read_adc(max34408, channel, val);
+	if (rc)
+		goto err_unlock;
+
+	/* back to old values */
+	rc = regmap_write(max34408->regmap, MAX34408_CONTROL, tmp);
+	if (rc)
+		dev_err(max34408->dev,
+			"Error (%d) writing control register\n", rc);
+
+err_unlock:
+	mutex_unlock(&max34408->lock);
+
+	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_PROCESSED:
+	case IIO_CHAN_INFO_AVERAGE_RAW:
+		rc = max34408_read_adc_avg(max34408, chan->channel,
+					   val);
+		if (rc)
+			return rc;
+
+		if (mask == IIO_CHAN_INFO_PROCESSED) {
+			/*
+			 * 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 = DIV_ROUND_CLOSEST((10000 / max34408->rsense) *
+						 *val * 1000, 256);
+		}
+
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_RAW:
+		rc = max34408_read_adc(max34408, chan->channel, val);
+		if (rc)
+			return rc;
+		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)
+{
+	struct device_node *np = client->dev.of_node;
+	struct max34408_data *max34408;
+	struct iio_dev *indio_dev;
+	struct regmap *regmap;
+	int rc;
+
+	regmap = devm_regmap_init_i2c(client, &max34408_regmap_config);
+	if (IS_ERR(regmap)) {
+		dev_err(&client->dev, "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);
+	i2c_set_clientdata(client, indio_dev);
+	max34408->regmap = regmap;
+	max34408->dev = &client->dev;
+	mutex_init(&max34408->lock);
+	rc = device_property_read_u32(&client->dev,
+				      "maxim,rsense-val-micro-ohms",
+				      &max34408->rsense);
+	if (rc)
+		return -EINVAL;
+
+	/* disable ALERT and averaging */
+	rc = regmap_write(max34408->regmap, MAX34408_CONTROL, 0x0);
+	if (rc)
+		return rc;
+
+	if (of_device_is_compatible(np, "maxim,max34408")) {
+		indio_dev->channels = max34408_channels;
+		indio_dev->num_channels = ARRAY_SIZE(max34408_channels);
+		indio_dev->name = "max34408";
+	} else if (of_device_is_compatible(np, "maxim,max34409")) {
+		indio_dev->channels = max34409_channels;
+		indio_dev->num_channels = ARRAY_SIZE(max34409_channels);
+		indio_dev->name = "max34409";
+	}
+	indio_dev->info = &max34408_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->dev.of_node = np;
+
+	return devm_iio_device_register(&client->dev, indio_dev);
+}
+
+static const struct of_device_id max34408_of_match[] = {
+	{
+		.compatible = "maxim,max34408",
+	},
+	{
+		.compatible = "maxim,max34409",
+	},
+	{}
+};
+MODULE_DEVICE_TABLE(of, max34408_of_match);
+
+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 2/2] iio: adc: Add driver support for MAX34408/9

[Lars-Peter Clausen]

Hi,

Looks very good. Few small comments.

On 9/17/23 14:11, Ivan Mikhaylov wrote:
> [...]
> +static int max34408_read_adc_avg(struct max34408_data *max34408, int channel, int *val)
> +{
> +	unsigned long ctrl;
Should be unsigned int so its compatible with the regmap API and you do 
not have to cast. Otherwise you'll run into trouble where long is 64 bit.
> +	int rc;
> +	u8 tmp;
> +
> +	mutex_lock(&max34408->lock);
> +	rc = regmap_read(max34408->regmap, MAX34408_CONTROL, (u32 *)&ctrl);
> +	if (rc)
> +		goto err_unlock;
> +
> +	/* set averaging (0b100) default values*/
> +	tmp = ctrl;
> +	set_bit(CONTROL_AVG2, &ctrl);
> +	clear_bit(CONTROL_AVG1, &ctrl);
> +	clear_bit(CONTROL_AVG0, &ctrl);

While using set and clear_bit is not wrong these are the atomic 
versions. There is __{set,clear}_bit as the non atomic version. But in 
my opinion is fine to just use |= and &= here.


> +	rc = regmap_write(max34408->regmap, MAX34408_CONTROL, ctrl);
> +	if (rc) {
> +		dev_err(max34408->dev,
> +			"Error (%d) writing control register\n", rc);
> +		goto err_unlock;
> +	}
> +
> +	rc = max34408_read_adc(max34408, channel, val);
> +	if (rc)
> +		goto err_unlock;
> +
> +	/* back to old values */
> +	rc = regmap_write(max34408->regmap, MAX34408_CONTROL, tmp);
> +	if (rc)
> +		dev_err(max34408->dev,
> +			"Error (%d) writing control register\n", rc);
> +
> +err_unlock:
> +	mutex_unlock(&max34408->lock);
> +
> +	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_PROCESSED:
> +	case IIO_CHAN_INFO_AVERAGE_RAW:
> +		rc = max34408_read_adc_avg(max34408, chan->channel,
> +					   val);
> +		if (rc)
> +			return rc;
> +
> +		if (mask == IIO_CHAN_INFO_PROCESSED) {


Usually we only have either raw + offset and scale or processed. In this 
case I'd go with raw + scale and offset since it is a linear 
transformation. processed is usually only used when the transformation 
is non linear.

> +			/*
> +			 * 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 = DIV_ROUND_CLOSEST((10000 / max34408->rsense) *
> +						 *val * 1000, 256);
> +		}
> +
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_RAW:
> +		rc = max34408_read_adc(max34408, chan->channel, val);
> +		if (rc)
> +			return rc;
> +		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)
> +{
> +	struct device_node *np = client->dev.of_node;
> +	struct max34408_data *max34408;
> +	struct iio_dev *indio_dev;
> +	struct regmap *regmap;
> +	int rc;
> +
> +	regmap = devm_regmap_init_i2c(client, &max34408_regmap_config);
> +	if (IS_ERR(regmap)) {
> +		dev_err(&client->dev, "regmap_init failed\n");
There is a the dev_err_probe function, which has the advantage of having 
a unified formatting for the error message style.
> +		return PTR_ERR(regmap);
> +	}
> +
> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*max34408));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	max34408 = iio_priv(indio_dev);
> +	i2c_set_clientdata(client, indio_dev);
> +	max34408->regmap = regmap;
> +	max34408->dev = &client->dev;
> +	mutex_init(&max34408->lock);
> +	rc = device_property_read_u32(&client->dev,
> +				      "maxim,rsense-val-micro-ohms",
> +				      &max34408->rsense);
> +	if (rc)
> +		return -EINVAL;
> +
> +	/* disable ALERT and averaging */
> +	rc = regmap_write(max34408->regmap, MAX34408_CONTROL, 0x0);
> +	if (rc)
> +		return rc;
> +
> +	if (of_device_is_compatible(np, "maxim,max34408")) {
Instead of using of_device_is_compatible it is usually preferred to 
assign some sort of chip_data struct to the data field of the of the 
compatible array. This makes it both easier to add new chips and also 
other ways to instantiate the device besides devicetree.
> +		indio_dev->channels = max34408_channels;
> +		indio_dev->num_channels = ARRAY_SIZE(max34408_channels);
> +		indio_dev->name = "max34408";
> +	} else if (of_device_is_compatible(np, "maxim,max34409")) {
> +		indio_dev->channels = max34409_channels;
> +		indio_dev->num_channels = ARRAY_SIZE(max34409_channels);
> +		indio_dev->name = "max34409";
> +	}
> +	indio_dev->info = &max34408_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->dev.of_node = np;
The assignment of of_node should not be needed, the IIO core will take 
care of this.
> +
> +	return devm_iio_device_register(&client->dev, indio_dev);
> +}
> +

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

[Jonathan Cameron]

On Mon, 18 Sep 2023 00:11:43 +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.

Hi Ivan. 

A few additional comments from me to add to what Lars said.
Generally looking nice.

Jonathan

> 
> Link: https://www.analog.com/media/en/technical-documentation/data-sheets/MAX34408-MAX34409.pdf
> 
Link is part of the tags block, so no blank line here (tools can then handle it as
a tag rather than freeform text.

> 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..538b086ed593 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -756,6 +756,17 @@ config MAX9611
>  	  To compile this driver as a module, choose M here: the module will be
>  	  called max9611.
>  
> +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 MCP320X
>  	tristate "Microchip Technology MCP3x01/02/04/08 and MCP3550/1/3"
>  	depends on SPI
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 2facf979327d..8fee08546bcc 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -69,6 +69,7 @@ obj-$(CONFIG_MAX1241) += max1241.o
>  obj-$(CONFIG_MAX1363) += max1363.o
>  obj-$(CONFIG_MAX77541_ADC) += max77541-adc.o
>  obj-$(CONFIG_MAX9611) += max9611.o
> +obj-$(CONFIG_MAX34408) += max34408.o

Hmm. For consistency should be after max1363 I think. We've
gone with alphanumeric order so far rather than taking digits into account
so we should keep to that.

That also means moving it up in the Kconfig file.

I've not checked we've been consistent beyond the names I can see here though.

>  obj-$(CONFIG_MCP320X) += mcp320x.o
>  obj-$(CONFIG_MCP3422) += mcp3422.o
>  obj-$(CONFIG_MCP3911) += mcp3911.o
> diff --git a/drivers/iio/adc/max34408.c b/drivers/iio/adc/max34408.c
> new file mode 100644
> index 000000000000..96c1de59edb5
> --- /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/regmap.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/types.h>
> +
> +#define MAX34408_STATUS		0x0
Useful to give clear naming separation between regisers
and fields.   I'd stick _REG on the end.

> +#define MAX34408_CONTROL	0x1
> +#define MAX34408_OCDELAY	0x2
> +#define MAX34408_SDDELAY	0x3
> +
> +#define MAX34408_ADC1		0x4
> +#define MAX34408_ADC2		0x5
> +/* ADC3 & ADC4 always returns 0x0 on 34408 */

Perhaps name them MAX34409_ADC3 to make that clearer at
the point of use.

> +#define MAX34408_ADC3		0x6
> +#define MAX34408_ADC4		0x7
> +
> +#define MAX34408_OCT1		0x8
> +#define MAX34408_OCT2		0x9
> +#define MAX34408_OCT3		0xA
> +#define MAX34408_OCT4		0xB
> +
> +#define MAX34408_DID		0xC
> +#define MAX34408_DCYY		0xD
> +#define MAX34408_DCWW		0xE
> +
> +#define MAX34408_CHANNELS	2
> +#define MAX34409_CHANNELS	4
> +
> +/* Bit masks for status register */
> +#define STATUS_OC1		BIT(0
To avoid potential naming clashes with generic headers
all defines should be prefixed with MA34008_STATUS_OC1

(note this is why I suggest a REG postfix for the register
addreses.

> +#define STATUS_OC2		BIT(1)
> +/* OC3 & OC4 only for max34409 */
Define a mask for each.
#define MAX34008_STATUS_OC_MSK GENMASK(1, 0)
#define MAX34009_STATUS_OC_MSK GENMASK(3, 0)
and the comment isn't needed.

> +#define STATUS_OC3		BIT(2)
> +#define STATUS_OC4		BIT(3)
> +#define STATUS_SHTDN		BIT(4)
> +#define STATUS_ENA		BIT(5)
> +
> +/* Bit masks for control register */
> +#define CONTROL_AVG0		BIT(0)
> +#define CONTROL_AVG1		BIT(1)
> +#define CONTROL_AVG2		BIT(2)
> +#define CONTROL_ALERT		BIT(3)
> +
> +/* Bit masks for over current delay */
> +#define OCDELAY_OCD0		BIT(0)
> +#define OCDELAY_OCD1		BIT(1)
> +#define OCDELAY_OCD2		BIT(2)
> +#define OCDELAY_OCD3		BIT(3)
> +#define OCDELAY_OCD4		BIT(4)
> +#define OCDELAY_OCD5		BIT(5)
> +#define OCDELAY_OCD6		BIT(6)
Looks like a 7 bit field - clear in table 9 where
values are given as 00h to 7fh

#define OCDELAY_OCD_MASK	GENMASK(6, 0

Same for other similar multi bit fields.

> +#define OCDELAY_RESET		BIT(7)
> +
> +/* Bit masks for shutdown delay */
> +#define SDDELAY_SHD0		BIT(0)
> +#define SDDELAY_SHD1		BIT(1)
> +#define SDDELAY_SHD2		BIT(2)
> +#define SDDELAY_SHD3		BIT(3)
> +#define SDDELAY_SHD4		BIT(4)
> +#define SDDELAY_SHD5		BIT(5)
> +#define SDDELAY_SHD6		BIT(6)
> +#define SDDELAY_RESET		BIT(7)
> +
> +/**
> + * struct max34408_data - max34408/max34409 specific data.
> + * @regmap:	device register map.
> + * @dev:	max34408 device.
> + * @lock:	lock for protecting access to device hardware registers.
The locks in the underlying bus driver will do that for us.
Perhaps this is to allow read modify write cycles?
If so, give that extra detail here.

> + * @rsense:	Rsense value in uOhm.
> + */
> +struct max34408_data {
> +	struct regmap *regmap;
> +	struct device *dev;
> +	struct mutex lock;
> +	u32 rsense;
> +};
> +
> +static const struct regmap_config max34408_regmap_config = {
> +	.reg_bits	= 8,
> +	.val_bits	= 8,
> +	.max_register	= MAX34408_DCWW,
> +};
> +
> +static const struct iio_chan_spec max34408_channels[] = {
> +	{
> +		.type			= IIO_CURRENT,
> +		.info_mask_separate	= BIT(IIO_CHAN_INFO_RAW) |
> +					  BIT(IIO_CHAN_INFO_PROCESSED) |
> +					  BIT(IIO_CHAN_INFO_AVERAGE_RAW),
> +		.channel		= 0,
> +		.indexed = 1,
> +	},
> +	{
> +		.type			= IIO_CURRENT,
> +		.info_mask_separate	= BIT(IIO_CHAN_INFO_RAW) |
> +					  BIT(IIO_CHAN_INFO_PROCESSED) |
> +					  BIT(IIO_CHAN_INFO_AVERAGE_RAW),
> +		.channel		= 1,
> +		.indexed = 1,
> +	},
> +};
> +
> +static const struct iio_chan_spec max34409_channels[] = {
> +	{
> +		.type			= IIO_CURRENT,
> +		.info_mask_separate	= BIT(IIO_CHAN_INFO_RAW) |
> +					  BIT(IIO_CHAN_INFO_PROCESSED) |
> +					  BIT(IIO_CHAN_INFO_AVERAGE_RAW),
> +		.channel		= 0,
> +		.indexed = 1,

I think a macro would help here as there is only one parameter.
#define MAX34008_CHANNEL(_index) 		\
	{ 					\
		.type = IIO_CURRENT,		\
		....
		.channel = (_index),		\
		.indexed = 1,
	}



> +	},
> +	{
> +		.type			= IIO_CURRENT,
> +		.info_mask_separate	= BIT(IIO_CHAN_INFO_RAW) |
> +					  BIT(IIO_CHAN_INFO_PROCESSED) |
> +					  BIT(IIO_CHAN_INFO_AVERAGE_RAW),
> +		.channel		= 1,
> +		.indexed = 1,
> +	},
> +	{
> +		.type			= IIO_CURRENT,
> +		.info_mask_separate	= BIT(IIO_CHAN_INFO_RAW) |
> +					  BIT(IIO_CHAN_INFO_PROCESSED) |
> +					  BIT(IIO_CHAN_INFO_AVERAGE_RAW),
> +		.channel		= 2,
> +		.indexed = 1,
> +	},
> +	{
> +		.type			= IIO_CURRENT,
> +		.info_mask_separate	= BIT(IIO_CHAN_INFO_RAW) |
> +					  BIT(IIO_CHAN_INFO_PROCESSED) |
> +					  BIT(IIO_CHAN_INFO_AVERAGE_RAW),
> +		.channel		= 3,
> +		.indexed = 1,
> +	},
> +};
> +
> +static int max34408_read_adc(struct max34408_data *max34408, int channel,
> +			     int *val)
> +{
> +	int rc;
> +	u32 adc_reg;
> +
> +	switch (channel) {
> +	case 0:
> +		adc_reg = MAX34408_ADC1;
> +		break;
> +	case 1:
> +		adc_reg = MAX34408_ADC2;
> +		break;
> +	case 2:
> +		adc_reg = MAX34408_ADC3;
> +		break;
> +	case 3:
> +		adc_reg = MAX34408_ADC4;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	rc = regmap_read(max34408->regmap, adc_reg, val);
return regmap_read();

Is same and saves us a couple of lines.

> +	if (rc)
> +		return rc;
> +
> +	return 0;
> +}
> +
> +static int max34408_read_adc_avg(struct max34408_data *max34408, int channel, int *val)
> +{
> +	unsigned long ctrl;
> +	int rc;
> +	u8 tmp;
> +
> +	mutex_lock(&max34408->lock);

Consider the new guard() functionality for this lock.

	guard(mutex)(&max34408->lock); 

That should ensure the mutex is release whatever path you take out of the
function thus allowing you to drop the goto and explicit unlock.

It's new this cycle, so don't think we have any users in IIO yet, but
I'll be encouraging it's use for cases like this where it provides
nice simplifications to the code flow by allowing directly returning
instead of dancing through manual cleanup.


> +	rc = regmap_read(max34408->regmap, MAX34408_CONTROL, (u32 *)&ctrl);
> +	if (rc)
> +		goto err_unlock;
> +
> +	/* set averaging (0b100) default values*/
> +	tmp = ctrl;
> +	set_bit(CONTROL_AVG2, &ctrl);
> +	clear_bit(CONTROL_AVG1, &ctrl);
> +	clear_bit(CONTROL_AVG0, &ctrl);
> +	rc = regmap_write(max34408->regmap, MAX34408_CONTROL, ctrl);
> +	if (rc) {
> +		dev_err(max34408->dev,
> +			"Error (%d) writing control register\n", rc);
> +		goto err_unlock;
> +	}
> +
> +	rc = max34408_read_adc(max34408, channel, val);
> +	if (rc)
> +		goto err_unlock;
> +
> +	/* back to old values */
> +	rc = regmap_write(max34408->regmap, MAX34408_CONTROL, tmp);
> +	if (rc)
> +		dev_err(max34408->dev,
> +			"Error (%d) writing control register\n", rc);
> +
> +err_unlock:
> +	mutex_unlock(&max34408->lock);
> +
> +	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_PROCESSED:
> +	case IIO_CHAN_INFO_AVERAGE_RAW:
> +		rc = max34408_read_adc_avg(max34408, chan->channel,
> +					   val);
> +		if (rc)
> +			return rc;
> +
> +		if (mask == IIO_CHAN_INFO_PROCESSED) {
> +			/*
> +			 * 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 = DIV_ROUND_CLOSEST((10000 / max34408->rsense) *
> +						 *val * 1000, 256);

What Lars said about not supporting processed when we can expose the channel
nicely as raw and present the scale an offset as needed to let userspace
do this maths (it has floating point so will probably do it better than
we can easily do in kernel).

> +		}
> +
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_RAW:
> +		rc = max34408_read_adc(max34408, chan->channel, val);
> +		if (rc)
> +			return rc;
> +		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)
> +{
> +	struct device_node *np = client->dev.of_node;
> +	struct max34408_data *max34408;
> +	struct iio_dev *indio_dev;
> +	struct regmap *regmap;
> +	int rc;
> +
> +	regmap = devm_regmap_init_i2c(client, &max34408_regmap_config);
> +	if (IS_ERR(regmap)) {
> +		dev_err(&client->dev, "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);
> +	i2c_set_clientdata(client, indio_dev);

I don't see this being used. If it is not don't set it.

> +	max34408->regmap = regmap;
> +	max34408->dev = &client->dev;
> +	mutex_init(&max34408->lock);
> +	rc = device_property_read_u32(&client->dev,
> +				      "maxim,rsense-val-micro-ohms",
> +				      &max34408->rsense);

As I mentioned in the review of the binding, I can't see why these
will necessarily be the same for all channels.

> +	if (rc)
> +		return -EINVAL;
> +
> +	/* disable ALERT and averaging */
> +	rc = regmap_write(max34408->regmap, MAX34408_CONTROL, 0x0);
> +	if (rc)
> +		return rc;
> +
> +	if (of_device_is_compatible(np, "maxim,max34408")) {
> +		indio_dev->channels = max34408_channels;
> +		indio_dev->num_channels = ARRAY_SIZE(max34408_channels);
> +		indio_dev->name = "max34408";
> +	} else if (of_device_is_compatible(np, "maxim,max34409")) {
> +		indio_dev->channels = max34409_channels;
> +		indio_dev->num_channels = ARRAY_SIZE(max34409_channels);
> +		indio_dev->name = "max34409";

Lars mentioned this already. Much prefer chip specific data to be
data, not code.  So this wants to go in a chip type specific structure that
is static const.  In general that scales much better as we get more variants
supported by a driver over time + as Lars mentioned works with other firmware
types. Not sure it applies elsewhere (I'm reviewing driver in reverse) but
in general use the generic firmware accessors in linux/property.h not the
of specific ones.  We are slowly moving almost all IIO drivers over to that
generic firmware approach, but it's take a while so you'll still find plenty
of examples that are using of specific calls.

> +	}
> +	indio_dev->info = &max34408_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->dev.of_node = np;

That should be set by the IIO core (though it does it in a more general fashion than this).

https://elixir.bootlin.com/linux/latest/source/drivers/iio/industrialio-core.c#L1905
> +
> +	return devm_iio_device_register(&client->dev, indio_dev);
> +}