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

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

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

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

Hey,

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

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

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

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

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

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

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

Thanks,
Conor.

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

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

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

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

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

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

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

Jonathan

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