On 21/02/2023 15:49, Julien Panis wrote: > > > On 2/21/23 12:17, Krzysztof Kozlowski wrote: >> 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... > > I will reorder it alphabetically in v2. > >> >>>>> + >>>>> + 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. > > I will remove 'ti,use-crc;' property from the DT. This will be only in > the driver. > Do you also consider that a property such as 'ti,is-secondary-pmic;' > would not be acceptable either ? From driver point of view, this > primary/secondary role on SPMI bus is a 'built-in' property of the > PMIC (CRC must be enabled only via primary PMIC but using the > primary PMIC does not imply that CRC is necessarily used). Depends, I am not sure. Are the PMICs in some kind of hierarchical topology? Like one is parent of another? If not (so both are parallel/equal children of SPMI bus), then some property to indicate which one is the main PMIC makes sense. (...) >>> 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. > > OK. Regarding buck34, it might be unnecessary and could finally be > removed in v2. If we keep it, my understanding of your suggestion is: > allOf: > - if: > required: > - buck12 > then: > properties: > buck123: false > buck1234: false > - if: > required: > - buck123 > then: > properties: > buck34: false > - if: > required: > - buck1234 > then: > properties: > buck34: false Yes, something like this. Best regards, Krzysztof
