| Message ID | 20231115134453.6656-2-marius.cristea@microchip.com |
|---|---|
| State | Accepted |
| Commit | a8ce0b4e5653c3aafd602be1e3aaf5d08cb00923 |
| Headers | show |
| Series | adding support for Microchip PAC193X Power Monitor | expand |
On 16/11/2023 19:21, Conor Dooley wrote: >>> +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? Sure, but which compatible contains "interrupts"? Best regards, Krzysztof
On Thu, Nov 16, 2023 at 09:00:50PM +0100, Krzysztof Kozlowski wrote: > On 16/11/2023 19:21, Conor Dooley wrote: > > >>> +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? > > Sure, but which compatible contains "interrupts"? Yeah, I did notice that - I figured you understood that that was meant to not be a check on compatibles, but rather on regular old properties & the rationale for the mutual exclusion was what you were missing.
On Sun, 26 Nov 2023 11:24:56 +0000 Conor Dooley <conor@kernel.org> wrote: > On Sat, Nov 25, 2023 at 07:47:54PM +0000, Jonathan Cameron wrote: > > On Thu, 16 Nov 2023 18:21:33 +0000 > > Conor Dooley <conor@kernel.org> wrote: > > > On Thu, Nov 16, 2023 at 04:01:43PM +0100, Krzysztof Kozlowski wrote: > > > > On 15/11/2023 14:44, marius.cristea@microchip.com wrote: > > > > > From: Marius Cristea <marius.cristea@microchip.com> > > > > > > +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'll admit I've long forgotten what was going on here, but based just on > > this bit of text I agree. There is nothing 'stopping' us having a pin > > uses as either / or / both interrupt and gpio. > > > > It'll be a bit messy to support in the driver as IIRC there are some sanity > > checks that limit combinations on IRQs and output GPIOS. Can't remember > > how bad the dance to navigate it safely is. > > > > First version I'd just say pick one option if both are provided and > > don't support configuring it at runtime. > > Just to be clear, you are suggesting having the > "microchip,slow-io-gpios" and "interrupts" properties in the binding, > but the driver will just (for example) put that pin into alert mode > always & leave it there? Yes. > If that is what you are suggesting, that seems pragmatic to me. If a use case to do something else comes along later, then we can be smarter, but I'd like to keep it simple initially at least. Jonathan > > Cheers, > Conor. >
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@microchip.com> + +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). + 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. + +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 + then: + properties: + microchip,slow-io: false + else: + if: + properties: + compatible: + contains: + const: microchip,slow-io + then: + properties: + interrupts: false + +additionalProperties: false + +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + pac193x: power-monitor@10 { + compatible = "microchip,pac1934"; + reg = <0x10>; + + #address-cells = <1>; + #size-cells = <0>; + + channel@1 { + reg = <0x1>; + shunt-resistor-micro-ohms = <24900000>; + label = "CPU"; + }; + + channel@2 { + reg = <0x2>; + shunt-resistor-micro-ohms = <49900000>; + label = "GPU"; + }; + + channel@3 { + reg = <0x3>; + shunt-resistor-micro-ohms = <75000000>; + label = "MEM"; + bipolar; + }; + + channel@4 { + reg = <0x4>; + shunt-resistor-micro-ohms = <100000000>; + label = "NET"; + bipolar; + }; + }; + }; + +...