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

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

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