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

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

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.


(...)

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