Re: [PATCH 1/2] dt-bindings: iio: adc: add binding for pac1921

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3708 bytes --]

Hey,

On Wed, Jul 03, 2024 at 03:34:35PM +0200, Matteo Martelli wrote:
> Add binging for Microchip PAC1921 Power/Current monitor
> 
> Signed-off-by: Matteo Martelli <matteomartelli3@gmail.com>
> ---
>  .../bindings/iio/adc/microchip,pac1921.yaml        | 79 ++++++++++++++++++++++
>  1 file changed, 79 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/microchip,pac1921.yaml b/Documentation/devicetree/bindings/iio/adc/microchip,pac1921.yaml
> new file mode 100644
> index 000000000000..ec08228dcc16
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/microchip,pac1921.yaml
> @@ -0,0 +1,79 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/microchip,pac1921.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Microchip PAC1921 High-Side Power/Current Monitor with Anaog Output
> +
> +maintainers:
> +  - Matteo Martelli <matteomartelli3@gmail.com>
> +
> +description: |
> +  The PAC1921 is a power/current monitoring device with an analog output
> +  and I2C/SMBus interface.
> +
> +  Datasheet can be found here:
> +  https://ww1.microchip.com/downloads/en/DeviceDoc/PAC1921-Data-Sheet-DS20005293E.pdf
> +
> +properties:
> +  compatible:
> +    const: microchip,pac1921
> +
> +  reg:
> +    maxItems: 1
> +
> +  "#io-channel-cells":
> +    const: 1
> +
> +  shunt-resistor-micro-ohms:
> +    description:
> +      Value in micro Ohms of the shunt resistor connected between
> +      the SENSE+ and SENSE- inputs, across which the current is measured.
> +      Value is needed to compute the scaling of the measured current.
> +
> +  label:
> +    description: Unique name to identify which device this is.
> +
> +  microchip,dv-gain:
> +    description:
> +      Digital multiplier to control the effective bus voltage gain. The gain
> +      value of 1 is the setting for the full-scale range and it can be increased
> +      when the system is designed for a lower VBUS range.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [1, 2, 4, 8, 16, 32]
> +    default: 1
> +
> +  microchip,di-gain:

Why is this gain a fixed property in the devicetree, rather than
something the user can control? Feels like it should be user
controllable.

> +    description:
> +      Digital multiplier to control the effective current gain. The gain
> +      value of 1 is the setting for the full-scale range and it can be
> +      increased when the system is designed for a lower VSENSE range.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [1, 2, 4, 8, 16, 32, 64, 128]
> +    default: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - shunt-resistor-micro-ohms

You're missing a vdd-supply btw and the !read/int pin isn't described
here either. I think the latter needs a property to control it (probably
a GPIO since it is intended for host control) and a default value for if
the GPIO isn't provided?

> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        adc@4c {
> +            compatible = "microchip,pac1921";
> +            #io-channel-cells = <1>;
> +            label = "vbat";
> +            reg = <0x4c>;

Order here should be compatible, reg, generic properties and then finally
vendor ones.

Thanks for your patch,
Conor.

> +            shunt-resistor-micro-ohms = <10000>;
> +            microchip,dv-gain = <4>;
> +            microchip,di-gain = <32>;
> +        };
> +    };
> +...
> 
> -- 
> 2.45.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]