Re: [PATCH v1 1/4] dt-bindings: mfd: Add DT bindings for TI TPS6594 PMIC

Krzysztof Kozlowski
2023-02-21T11:18:09Z

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...

>
>>
>>> +
>>> + 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