On 17/02/2023 13:10, Julien Panis wrote: > > On 2/17/23 10:06, Krzysztof Kozlowski wrote: >> On 16/02/2023 12:44, Julien Panis wrote: >>> TPS6594 is a Power Management IC which provides regulators and others >> Subject: drop second/last, redundant "DT bindings for". The >> "dt-bindings" prefix is already stating that these are bindings. >> >> >>> features like GPIOs, RTC, watchdog, ESMs (Error Signal Monitor), and >>> PFSM (Pre-configurable Finite State Machine) managing the state of the >>> device. >>> TPS6594 is the super-set device while TPS6593 and LP8764X are derivatives. >>> >>> Signed-off-by: Julien Panis <jpanis@xxxxxxxxxxxx> >>> --- >>> .../devicetree/bindings/mfd/ti,tps6594.yaml | 164 ++++++++++++++++++ >>> 1 file changed, 164 insertions(+) >>> create mode 100644 Documentation/devicetree/bindings/mfd/ti,tps6594.yaml >>> >>> diff --git a/Documentation/devicetree/bindings/mfd/ti,tps6594.yaml b/Documentation/devicetree/bindings/mfd/ti,tps6594.yaml >>> new file mode 100644 >>> index 000000000000..37968d6c0420 >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/mfd/ti,tps6594.yaml >>> @@ -0,0 +1,164 @@ >>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause >>> +%YAML 1.2 >>> +--- >>> +$id: http://devicetree.org/schemas/mfd/ti,tps6594.yaml# >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>> + >>> +title: TI TPS6594 Power Management Integrated Circuit >>> + >>> +maintainers: >>> + - Julien Panis <jpanis@xxxxxxxxxxxx> >>> + >>> +description: | >>> + TPS6594 is a Power Management IC which provides regulators and others >>> + features like GPIOs, RTC, watchdog, ESMs (Error Signal Monitor), and >>> + PFSM (Pre-configurable Finite State Machine) managing the state of the device. >>> + TPS6594 is the super-set device while TPS6593 and LP8764X are derivatives. >>> + >>> +properties: >>> + compatible: >>> + enum: >>> + - ti,tps6594 >>> + - ti,tps6593 >>> + - ti,lp8764x >> Any particular choice of ordering (different than alphabetical)? > > Thank you for the review. > > I chose this ordering because it emphasizes the fact that tps6593 and > lp8764x > are derivatives of tps6594 : tps6593 is nearly the same (a minor feature > is not > supported), and lp8764x has less resources (less bucks/LDO, and no RTC). > > Besides, a multi-PMIC synchronization scheme is implemented in the PMIC > device > to synchronize the power state changes with other PMIC devices. This is done > through a SPMI bus : the master PMIC is the controller device on the > SPMI bus, > and the slave PMICs are the target devices on the SPMI bus. For the 5 boards > we work on (for which device trees will be sent in another patch series): > - tps6594 is used on 3 boards and is always master (multi-PMIC config) > - tps6593 is used on 1 board and is master (single-PMIC config) > - lp8764x is used on 2 boards and is always slave (multi-PMIC config) > There might not be situations in which lp8764x would be master and tps6594 > or tps6593 would be slave. > > That's why I preferred this ordering. > > Do you think that alphabetical order would be better ? It's simpler (requires no knowledge about chips) and reduces the future conflicts. It's fine to keep it also ordered like you said, although I wonder how other people adding new compatibles here will figure it out... > >> >>> + >>> + reg: >>> + description: I2C slave address or SPI chip select number. >>> + maxItems: 1 >>> + >>> + ti,use-crc: >>> + type: boolean >>> + description: If true, use CRC for I2C and SPI interface protocols. >> Hm, why different boards would like to enable or disable it? Why this >> suits DT? > > You're right. Reading your comment, it appears to me that CRC feature is > not fully > related to HW description and should not be set in DT. > > CRC is not 'fully' related to HW, but... > For CRC feature as well, PMICs are synchronized (for boards with > multi-PMIC config). > To use CRC mode, this feature must be requested explicitly on the master > PMIC > through I2C or SPI driver, then it is enabled for the slave PMICs > through SPMI bus: that > sync is performed 'automatically', without intervention from the I2C or > SPI driver to > enable CRC on slave PMICs. > As a consequence, CRC feature is enabled for all PMICs at I2C/SPI driver > probe, > or it is let disabled for all PMICs. But it can't be enabled for one > PMIC and disabled > for another one. > > This will probably rediscussed for I2C/SPI drivers, but do you think > that a 'use_crc' > driver parameter would be an acceptable solution ? If so, the master > PMIC would have > to be identified, so that the driver can explicitly enable CRC mode for > this one if > 'use_crc' is true. With this solution, some 'ti,is-master;' bool > property would be necessary. It looks the property should be only in the drivers, not in the DT. > >> >>> + >>> + system-power-controller: true >>> + >>> + interrupts: >>> + maxItems: 1 >>> + >>> + ti,multi-phase-id: >>> + description: | >>> + Describes buck multi-phase configuration, if any. For instance, XY id means >>> + that outputs of buck converters X and Y are combined in multi-phase mode. >>> + $ref: /schemas/types.yaml#/definitions/uint32 >>> + enum: [12, 34, 123, 1234] >>> + >>> +patternProperties: >>> + "^buck([1-5]|12|34|123|1234)-supply$": >>> + description: Input supply phandle for each buck. >>> + >>> + "^ldo[1-4]-supply$": >>> + description: Input supply phandle for each ldo. >>> + >>> + regulators: >> This should go to properties, not patternProperties. >> >>> + type: object >>> + description: List of regulators provided by this controller. >>> + >>> + patternProperties: >>> + "^buck([1-5]|12|34|123|1234)$": >>> + type: object >>> + $ref: /schemas/regulator/regulator.yaml# >>> + >>> + unevaluatedProperties: false >>> + >>> + "^ldo[1-4]$": >>> + type: object >>> + $ref: /schemas/regulator/regulator.yaml# >>> + >>> + unevaluatedProperties: false >>> + >> You could add here - on this level - of indentation allOf:if for >> excluding setups >> >> if: >> required: >> - buck12 >> then: >> properties: >> buck123: false >> buck1234: false >> >> Or, if you want to require regulator then: >> oneOf: >> - required: >> - buck12 >> - required: >> - buck123 >> - required: >> - buck1234 >> >> and anyway exclude buck34 with two above. > > I am not sure that we have the same understanding of the multi-phase setup. > Maybe the description I wrote is not clear enough (?) Or I just don't > understand > what you mean exactly. > > How would you combine outputs of bucks 3 and 4 ? No one discusses here changing this... > We use 'buck34' property to mean that: > - buck1 output is mono-phase, > - buck2 output is mono-phase, > - buck3 and buck4 outputs are combined (i.e. multi-phases). > This weird configuration is supported by these PMICs. > > Using a PMIC without using the provided regulators does not seem very > interesting > indeed. > But strictly speaking, these regulators are not required. One could use > some others > resources provided by the PMIC (the Error Signal Monitor device for > instance). Then the first method. > Besides, multi-phase mode depends on the chosen design and is not > required for > all situations. Sorry, I don't think it is related to the topic I proposed. Best regards, Krzysztof
