Re: [PATCH v3 1/2] dt-bindings: iio: adc: adding support for PAC193X

[Conor Dooley]

On Thu, Nov 16, 2023 at 04:01:43PM +0100, Krzysztof Kozlowski wrote:
> On 15/11/2023 14:44, marius.cristea@xxxxxxxxxxxxx wrote:
> > From: Marius Cristea <marius.cristea@xxxxxxxxxxxxx>
> > 
> > This is the device tree schema for iio driver for
> > Microchip PAC193X series of Power Monitors with Accumulator.
> > 
> > Signed-off-by: Marius Cristea <marius.cristea@xxxxxxxxxxxxx>
> > ---
> >  .../bindings/iio/adc/microchip,pac1934.yaml   | 137 ++++++++++++++++++
> >  1 file changed, 137 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/adc/microchip,pac1934.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/adc/microchip,pac1934.yaml b/Documentation/devicetree/bindings/iio/adc/microchip,pac1934.yaml
> > new file mode 100644
> > index 000000000000..2609cb19c377
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/microchip,pac1934.yaml
> > @@ -0,0 +1,137 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iio/adc/microchip,pac1934.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Microchip PAC1934 Power Monitors with Accumulator
> > +
> > +maintainers:
> > +  - Marius Cristea <marius.cristea@xxxxxxxxxxxxx>
> > +
> > +description: |
> > +  This device is part of the Microchip family of Power Monitors with Accumulator.
> > +  The datasheet for PAC1931, PAC1932, PAC1933 and PAC1934 can be found here:
> > +    https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ProductDocuments/DataSheets/PAC1931-Family-Data-Sheet-DS20005850E.pdf
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - microchip,pac1931
> > +      - microchip,pac1932
> > +      - microchip,pac1933
> > +      - microchip,pac1934
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  "#address-cells":
> > +    const: 1
> > +
> > +  "#size-cells":
> > +    const: 0
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  microchip,slow-io:
> > +    type: boolean
> > +    description: |
> > +      A GPIO used to trigger a change is sampling rate (lowering the chip power consumption).
> 
> Use Linux coding style wrapping (as described in Linux Coding style). I
> am not going to tell you numbers because I want you to read the document
> first.
> 
> This is boolean, not GPIO. I don't understand. "A GPIO", so any GPIO or
> some specific? How is this property related to GPIO?
> 
> 
> > +      If configured in SLOW mode, if this pin is forced high, sampling rate is forced to eight
> 
> This pin? This is boolean, not a GPIO. GPIOs are phandles.

I said it on the previous version, but this really seems like it should
be something like "slow-io-gpios". I know Jonathan expressed some
concerns about having to deal with it on the operating system side (as
the pin is either an input & used for this slow-io control, or an output
and used as an interrupt) but that is, in my opinion, a problem for the
operating system & the binding should describe how the hardware works,
even if that is not convenient. With this sort of property, a GPIO hog
would be required to be set up (and the driver for that gpio controller
bound etc before the pac driver loads) for correction functionality if
this property was in the non-default state.

> > +      samples/second. When it is forced low, the sampling rate is 1024 samples/second unless
> > +      a different sample rate has been programmed.
> > +
> > +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.
> > +
> > +    required:
> > +      - reg
> > +      - shunt-resistor-micro-ohms
> > +
> > +    unevaluatedProperties: false
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - "#address-cells"
> > +  - "#size-cells"
> > +
> > +allOf:
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: interrupts
> 
> 
> I don't understand what do you want to say here. I am also 100% sure you
> did not test it on a real case (maybe example passes but nothing more).

As far as I understand, the same pin on the device is used for both an
output or an input depending on the configuration. As an input, it is
the "slow-io" control, and as an output it is an interrupt.
I think Marius is trying to convey that either this pin can be in
exclusively one state or another.

_However_ I am not sure that that is really the right thing to do - they
might well be mutually exclusive modes, but I think the decision can be
made at runtime, rather than at devicetree creation time. Say for
example the GPIO controller this is connected to is capable of acting as
an interrupt controller. Unless I am misunderstanding the runtime
configurability of this hardware, I think it is possible to actually
provide a "slow-io-gpios" and an interrupt property & let the operating
system decide at runtime which mode it wants to work in.

I'm off travelling at the moment Marius, but I should be back in work on
Monday if you want to have a chat about it & explain a bit more to me?

Cheers,
Conor.

> 
> > +    then:
> > +      properties:
> > +        microchip,slow-io: false
> > +    else:
> > +      if:
> > +        properties:
> > +          compatible:
> > +            contains:
> > +              const: microchip,slow-io
> > +      then:
> > +        properties:
> > +          interrupts: false
> 
> Best regards,
> Krzysztof
>

Attachment: signature.asc
Description: PGP signature