Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4;
Message-ID: <f8735a6d81d485eb8e822e4fd55a00269ae688c2.camel@gmail.com>
Subject: Re: [PATCH 1/2] dt-bindings: adc: provide max34408/9 device tree
 binding document
From:   Ivan Mikhaylov <fr0st61te@gmail.com>
To:     Jonathan Cameron <jic23@kernel.org>
Cc:     Lars-Peter Clausen <lars@metafoo.de>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>, linux-iio@vger.kernel.org,
        linux-kernel@vger.kernel.org, devicetree@vger.kernel.org
Date:   Mon, 25 Sep 2023 19:48:39 +0300
In-Reply-To: <20230924135359.6404a867@jic23-huawei>
References: <20230917211143.7094-1-fr0st61te@gmail.com>
         <20230917211143.7094-2-fr0st61te@gmail.com>
         <20230924135359.6404a867@jic23-huawei>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.48.4 
MIME-Version: 1.0
Precedence: bulk

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:
>=20
> > The i2c driver with Rsense option for current monitoring.
> >=20
> > Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
>=20
> Hi Ivan,
>=20
> Welcome to IIO!
>=20
> Looks good, but there are a few things I'd add to make this describe
> the device
> a little more fully and flexibly.=C2=A0 Ideally we want a binding to full=
y
> 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)
>=20
> Thanks,
>=20
> Jonathan
>=20
> > ---
> > =C2=A0.../bindings/iio/adc/maxim,max34408.yaml=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0 | 63
> > +++++++++++++++++++
> > =C2=A01 file changed, 63 insertions(+)
> > =C2=A0create mode 100644
> > Documentation/devicetree/bindings/iio/adc/maxim,max34408.yaml
> >=20
> > 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:
> > +=C2=A0 - Ivan Mikhaylov <fr0st61te@gmail.com>
> > +
> > +description: |
> > +=C2=A0 The MAX34408/MAX34409 are two- and four-channel current monitor=
s
> > that are
> > +=C2=A0 configured and monitored with a standard I2C/SMBus serial
> > interface. Each
> > +=C2=A0 unidirectional current sensor offers precision high-side
> > operation with a
> > +=C2=A0 low full-scale sense voltage. The devices automatically sequenc=
e
> > through
> > +=C2=A0 two or four channels and collect the current-sense samples and
> > average them
> > +=C2=A0 to reduce the effect of impulse noise. The raw ADC samples are
> > compared to
> > +=C2=A0 user-programmable digital thresholds to indicate overcurrent
> > conditions.
> > +=C2=A0 Overcurrent conditions trigger a hardware output to provide an
> > immediate
> > +=C2=A0 indication to shut down any necessary external circuitry.
> > +
> > +=C2=A0 Specifications about the devices can be found at:
> > +=C2=A0
> > https://www.analog.com/media/en/technical-documentation/data-sheets/MAX=
34408-MAX34409.pdf
> > +
> > +properties:
> > +=C2=A0 compatible:
> > +=C2=A0=C2=A0=C2=A0 enum:
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 - maxim,max34408
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 - maxim,max34409
> > +
> > +=C2=A0 reg:
> > +=C2=A0=C2=A0=C2=A0 maxItems: 1
> > +
> > +=C2=A0 interrupts:
> > +=C2=A0=C2=A0=C2=A0 maxItems: 1
> > +
> > +=C2=A0 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?

>=20
> > +=C2=A0=C2=A0=C2=A0 description:
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Adjust the Rsense value to monitor high=
er or lower current
> > levels.
> > +=C2=A0=C2=A0=C2=A0 enum: [250, 500, 1000, 5000, 10000, 50000, 100000, =
200000,
> > 500000]
>=20
> These come from Table 18 which is example values I think?=C2=A0 Not sure
> there
> is anything limiting us to those particular values given the equation
> given
> just above that table should apply more generally.
>=20
> > +=C2=A0=C2=A0=C2=A0 default: 1000
>=20
> Please add regulator definitions.
>=20
> 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.
>=20
> 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.
>=20
> > +
> > +required:
> > +=C2=A0 - compatible
> > +=C2=A0 - reg
> > +=C2=A0 - maxim,rsense-val-micro-ohms
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +=C2=A0 - |
> > +=C2=A0=C2=A0=C2=A0 i2c {
> > +
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 #address-cells =3D <1>;
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 #size-cells =3D <0>;
> > +
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 adc@1e {
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0 compatible =3D "maxim,max34409";
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0 reg =3D <0x1e>;
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0 maxim,rsense-val-micro-ohms =3D <1000>;
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 };
> > +=C2=A0=C2=A0=C2=A0 };
>=20

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