Message-ID: <20240720180959.23540945@jic23-huawei>
Date: Sat, 20 Jul 2024 18:09:59 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: <marius.cristea@...rochip.com>
Cc: <lars@...afoo.de>, <robh+dt@...nel.org>,
 <krzysztof.kozlowski+dt@...aro.org>, <conor+dt@...nel.org>,
 <linux-iio@...r.kernel.org>, <devicetree@...r.kernel.org>,
 <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v1 1/2] dt-bindings: iio: adc: adding support for
 PAC194X

On Fri, 19 Jul 2024 20:38:54 +0300
<marius.cristea@...rochip.com> wrote:

> From: Marius Cristea <marius.cristea@...rochip.com>
> 
> This is the device tree schema for iio driver for
> Microchip PAC194X and PAC195X series of Power Monitors with Accumulator.
Hi Marius

Good to add a tiny bit either here or in below description for why
there are so many part numbers.

Looks like mixture of number of channels and high vs low side monitors.
And the big number is about voltage range?

Other comments inline.

Thanks,
Jonathan

> 
> Signed-off-by: Marius Cristea <marius.cristea@...rochip.com>
> ---
>  .../bindings/iio/adc/microchip,pac1944.yaml   | 168 ++++++++++++++++++
>  1 file changed, 168 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/microchip,pac1944.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/microchip,pac1944.yaml b/Documentation/devicetree/bindings/iio/adc/microchip,pac1944.yaml
> new file mode 100644
> index 000000000000..e997a4d536e9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/microchip,pac1944.yaml
> @@ -0,0 +1,168 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/microchip,pac1944.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Microchip PAC1944 and PAC1954 Power Monitors with Accumulator
> +
> +maintainers:
> +  - Marius Cristea <marius.cristea@...rochip.com>
> +
> +description: |
> +  This device is part of the Microchip family of Power Monitors with Accumulator.
> +  The datasheet for PAC1941_1, PAC1941_1, PAC1942_1, PAC1942_2, PAC1943_1 and PAC1944_1 can be found here:
> +    https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/DataSheets/PAC194X-Family-Data-Sheet-DS20006543.pdf
> +  The datasheet for PAC1951_1, PAC1951_1, PAC1952_1, PAC1952_2, PAC1953_1 and PAC1954_1 can be found here:
> +    https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/DataSheets/PAC195X-Family-Data-Sheet-DS20006539.pdf
> +
> +properties:
> +  compatible:
> +    enum:
> +      - microchip,pac1941_1
> +      - microchip,pac1941_2
> +      - microchip,pac1942_1
> +      - microchip,pac1942_2
> +      - microchip,pac1943_1
> +      - microchip,pac1944_1
> +      - microchip,pac1951_1
> +      - microchip,pac1951_2
> +      - microchip,pac1952_1
> +      - microchip,pac1952_2
> +      - microchip,pac1953_1
> +      - microchip,pac1954_1
> +
> +  reg:
> +    maxItems: 1
> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 0
> +
> +  interrupts:
> +    maxItems: 2
Need names as annoyingly people often wire just the second one
(I've never understood why!)

> +
> +  slow-io-gpios:
> +    description:
> +      A GPIO used to trigger a change is sampling rate (lowering the chip power
> +      consumption). If configured in SLOW mode, if this pin is forced high,
> +      sampling rate is forced to eight samples/second. When it is forced low,
> +      the sampling rate is 1024 samples/second unless a different sample rate
> +      has been programmed.
> +
> +  microchip,gpio:
> +    type: boolean
> +    description:
> +      In default mode, this pin is a GPIO input pin. It can be
> +      configured to be an output pin, as well as the ALERT2
> +      function. This pin is an open-drain configuration and
> +      needs a pull-up resistor to VDD.

This one is a bit odd.
Can we do that configuration based on whether anyone requests it
or if it is wired as alert2?

So I don't think we should need this binding, but we may need
the stuff to provide a gpio.


Also, oddly short wrap.



> +
> +patternProperties:
> +  "^channel@[1-4]+$":
> +    type: object
> +    $ref: adc.yaml
> +    description:
> +      Represents the external channels which are connected to the ADC.
> +
> +    properties:
> +      reg:
> +        items:
> +          minimum: 1
> +          maximum: 4
> +
> +      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.
> +
> +      microchip,vbus-mode:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        description:
> +          The VBUS could be configured into the following full scale range (FSR)
Key with these range things is to make it clear why they are a wiring thing
rather than something userspace should control.  Perhaps you can add
some detail on that here?

> +            <0>  -  VBUS has unipolar +32V to 0V FSR (default)
> +            <1>  -  VBUS has bipolar +32V to -32V FSR
> +            <2>  -  VBUS has bipolar +16V to -16V FSR
> +        maximum: 2
> +
> +      microchip,vsense-mode:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        description:
> +          The VSENSE could be configured into the following full scale range (FSR)
> +            <0>  -  VSENSE has unipolar +100 mV to 0V FSR (default)
> +            <1>  -  VSENSE has bipolar +100 mV to -100 mV FSR
> +            <2>  -  VSENSE has bipolar +50 mV to -50 mV FSR
> +        maximum: 2
> +
> +      microchip,accumulation-mode:
> +        $ref: /schemas/types.yaml#/definitions/uint32
Why is this a hardware thing? Is it specific to particular wiring?
Not sure this one belong in DT.

> +        description:
> +          The Hardware Accumulator could be configured to accumulate
> +          VPOWER, VSENSE or VBUS
> +            <0>  -  Accumulator accumulates VPOWER (default)
> +            <1>  -  Accumulator accumulates VSENSE
Add some info on what this is for (charge measurement)
Thankfully there is a nice section on why you'd want to do this in the datasheet
as only the power option made sense to me.
There is a nice statement that you might do this as an extreme form
of oversampling as well.

> +            <2>  -  Accumulator accumulates VBUS


> +        maximum: 2
> +
> +    required:
> +      - reg
> +      - shunt-resistor-micro-ohms
> +
> +    unevaluatedProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +  - "#address-cells"
> +  - "#size-cells"
Basic power supplies should always be here as chips tend not to run without
power.  Note that doesn't mean a dts needs to actually list them if they are always
on as the regulator framework will provide stubs etc.

Jonathan

Powered by blists - more mailing lists