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

[Krzysztof Kozlowski]

On 28/03/2023 12:45, Julien Panis wrote:
> 
> 
> On 3/28/23 08:51, Krzysztof Kozlowski wrote:
>> On 27/03/2023 17:40, Julien Panis wrote:
>>> 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.
>>>
>>> Signed-off-by: Julien Panis <jpanis@xxxxxxxxxxxx>
>>> ---
>>>   .../devicetree/bindings/mfd/ti,tps6594.yaml   | 231 ++++++++++++++++++
>>>   1 file changed, 231 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..4498e6361b34
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/mfd/ti,tps6594.yaml
>>> @@ -0,0 +1,231 @@
>>> +# 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.
>> LP8764X? Compatible says LP8764.
>>
>>> +
>>> +properties:
>>> +  compatible:
>>> +    enum:
>>> +      - ti,lp8764
>> It's confusing. If x was wildcard, didn't you remove part of model name?
> 
> OK, I will remove 'X' from model name in v5.

There is no x in compatible. What is (are) the model name(s)?

> 
>>
>>
>>> +      - ti,tps6593
>>> +      - ti,tps6594

(...)

>>> +
>>> +  rtc:
>>> +    type: object
>>> +    description: RTC provided by this controller.
>>> +    $ref: /schemas/rtc/rtc.yaml#
>> I doubt that you can have here any RTC and any watchdog (below). This
>> should be specific binding instead. Or list of compatibles if you have 3
>> or more possible bindings.
>>
>> Additionally, judging by your DTS you do not have any resources in rtc
>> and watchdog, so these should not be nodes by themself in such case.
> 
> It seems that I can't figure out what you and Rob mean by saying that
> "binding must be complete" and that "RTC and watchdog may or may not
> need binding changes".
> What does "specific binding" mean ?

Specific means not loose, not generic, precise with some accurate
properties.

> Should we add some specific property
> for RTC/WDG provided by the PMIC ?

You know ask me to know what is in your device. I don't know. You should
know.

> Should we write another yaml for both
> of them ? 

Depends. Pretty often yes, but think what do you want to put there?

> Why shouldn't they use the generic rtc/watchdog yaml ? 

There are no properties in these nodes, so you do not need nodes. Or if
you have properties then you need specific binding, not generic one.

> I don't
> understand why they would need some "binding changes". Any example
> I could refer to ? (I might have not looked at the relevant ones for my case
> before sending this v4)

git grep $ref | grep rtc.yaml


Best regards,
Krzysztof