Messages in this thread | | | Date | Tue, 21 Feb 2023 16:18:30 +0100 | Subject | Re: [PATCH v1 1/4] dt-bindings: mfd: Add DT bindings for TI TPS6594 PMIC | From | Julien Panis <> |
| |
On 2/21/23 16:01, Krzysztof Kozlowski wrote: > 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@baylibre.com> >>>>>> --- >>>>>> .../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@baylibre.com> >>>>>> + >>>>>> +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.
There is no hierarchical topology. So, I will consider identifying in DT which one is the main PMIC.
(...)
Julien
| |