On 17/02/2023 13:10, Julien Panis wrote:
On 2/17/23 10:06, Krzysztof Kozlowski wrote:It's simpler (requires no knowledge about chips) and reduces the future
On 16/02/2023 12:44, Julien Panis wrote:Thank you for the review.
TPS6594 is a Power Management IC which provides regulators and othersSubject: 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), andAny particular choice of ordering (different than alphabetical)?
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
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 ?
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...
It looks the property should be only in the drivers, not in the DT.You're right. Reading your comment, it appears to me that CRC feature is+Hm, why different boards would like to enable or disable it? Why this
+ 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.
suits DT?
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.
No one discusses here changing this...I am not sure that we have the same understanding of the multi-phase setup.+This should go to properties, not patternProperties.
+ 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:
+ type: objectYou could add here - on this level - of indentation allOf:if for
+ 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
+
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.
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 ?
We use 'buck34' property to mean that:Then the first method.
- 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).
Besides, multi-phase mode depends on the chosen design and is notSorry, I don't think it is related to the topic I proposed.
required for
all situations.
Best regards,
Krzysztof