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@xxxxxxxxx> wrote:
> 
> > The i2c driver with Rsense option for current monitoring.
> > 
> > Signed-off-by: Ivan Mikhaylov <fr0st61te@xxxxxxxxx>
> 
> 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@xxxxxxxxx>
> > +
> > +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.