| Message ID | 20230327154101.211732-2-jpanis@baylibre.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | TI TPS6594 PMIC support (Core, ESM, PFSM) | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
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@baylibre.com> > --- > .../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@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. LP8764X? Compatible says LP8764. > + > +properties: > + compatible: > + enum: > + - ti,lp8764 It's confusing. If x was wildcard, didn't you remove part of model name? > + - ti,tps6593 > + - ti,tps6594 > + > + reg: > + description: I2C slave address or SPI chip select number. > + maxItems: 1 > + > + ti,primary-pmic: > + type: boolean > + description: | > + Identify the primary PMIC on SPMI bus. > + A multi-PMIC synchronization scheme is implemented in the PMIC device > + to synchronize the power state changes with other PMIC devices. This is > + accomplished through a SPMI bus: the primary PMIC is the controller > + device on the SPMI bus, and the secondary PMICs are the target devices > + on the SPMI bus. > + > + system-power-controller: true > + > + gpio-controller: true > + > + '#gpio-cells': > + const: 2 > + description: | > + The first cell is the pin number, the second cell is used to specify flags. > + See ../gpio/gpio.txt for more information. > + > + 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-array > + oneOf: > + - items: > + - const: 12 > + - items: > + - const: 34 > + - items: > + - const: 12 > + - const: 34 > + - items: > + - const: 123 > + - items: > + - const: 1234 > + > + regulators: > + type: object > + description: List of regulators provided by this controller. > + > + patternProperties: > + "^buck([1-5]|12|34|123|1234)$": Why do you need ti,multi-phase-id property at all? Having buck123 implies ti,multi-phase-id=123. > + type: object > + $ref: /schemas/regulator/regulator.yaml# > + > + unevaluatedProperties: false > + > + "^ldo[1-4]$": > + type: object > + $ref: /schemas/regulator/regulator.yaml# > + > + unevaluatedProperties: false > + > + allOf: > + - if: > + required: > + - buck12 > + then: > + properties: > + buck123: false > + buck1234: false > + - if: > + required: > + - buck123 > + then: > + properties: > + buck34: false > + - if: > + required: > + - buck1234 > + then: > + properties: > + buck34: false > + > + additionalProperties: false > + > + 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. > + > + watchdog: > + type: object > + description: Watchdog provided by this controller. > + $ref: /schemas/watchdog/watchdog.yaml# > + > +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. > + > +required: > + - compatible > + - reg > + - interrupts > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/interrupt-controller/arm-gic.h> > + i2c { > + #address-cells = <1>; > + #size-cells = <0>; > + > + tps6593: pmic@48 { > + compatible = "ti,tps6593"; > + reg = <0x48>; > + ti,primary-pmic; > + system-power-controller; > + > + gpio-controller; > + #gpio-cells = <2>; > + > + pinctrl-names = "default"; > + pinctrl-0 = <&pmic_irq_pins_default>; > + interrupt-parent = <&mcu_gpio0>; > + interrupts = <0 IRQ_TYPE_EDGE_FALLING>; > + > + ti,multi-phase-id = <123>; > + > + buck123-supply = <&vcc_3v3_sys>; > + buck4-supply = <&vcc_3v3_sys>; > + buck5-supply = <&vcc_3v3_sys>; > + ldo1-supply = <&vcc_3v3_sys>; > + ldo2-supply = <&vcc_3v3_sys>; > + ldo3-supply = <&buck5>; > + ldo4-supply = <&vcc_3v3_sys>; > + > + regulators { > + buck123: buck123 { > + regulator-name = "vcc_core"; > + regulator-min-microvolt = <750000>; > + regulator-max-microvolt = <850000>; > + regulator-boot-on; > + regulator-always-on; > + }; > + > + buck4: buck4 { > + regulator-name = "vcc_1v1"; > + regulator-min-microvolt = <1100000>; > + regulator-max-microvolt = <1100000>; > + regulator-boot-on; > + regulator-always-on; > + }; > + > + buck5: buck5 { > + regulator-name = "vcc_1v8_sys"; > + regulator-min-microvolt = <1800000>; > + regulator-max-microvolt = <1800000>; > + regulator-boot-on; > + regulator-always-on; > + }; > + > + ldo1: ldo1 { > + regulator-name = "vddshv5_sdio"; > + regulator-min-microvolt = <3300000>; > + regulator-max-microvolt = <3300000>; > + regulator-boot-on; > + regulator-always-on; > + }; > + > + ldo2: ldo2 { > + regulator-name = "vpp_1v8"; > + regulator-min-microvolt = <1800000>; > + regulator-max-microvolt = <1800000>; > + regulator-boot-on; > + regulator-always-on; > + }; > + > + ldo3: ldo3 { > + regulator-name = "vcc_0v85"; > + regulator-min-microvolt = <850000>; > + regulator-max-microvolt = <850000>; > + regulator-boot-on; > + regulator-always-on; > + }; > + > + ldo4: ldo4 { > + regulator-name = "vdda_1v8"; > + regulator-min-microvolt = <1800000>; > + regulator-max-microvolt = <1800000>; > + regulator-boot-on; > + regulator-always-on; > + }; > + }; > + > + rtc: rtc { > + wakeup-source; No. We do not create nodes for single property. > + }; > + > + watchdog: watchdog { > + timeout-sec = <10>; Same problem. Best regards, Krzysztof
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@baylibre.com> >> --- >> .../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@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. > 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. > > >> + - ti,tps6593 >> + - ti,tps6594 >> + >> + reg: >> + description: I2C slave address or SPI chip select number. >> + maxItems: 1 >> + >> + ti,primary-pmic: >> + type: boolean >> + description: | >> + Identify the primary PMIC on SPMI bus. >> + A multi-PMIC synchronization scheme is implemented in the PMIC device >> + to synchronize the power state changes with other PMIC devices. This is >> + accomplished through a SPMI bus: the primary PMIC is the controller >> + device on the SPMI bus, and the secondary PMICs are the target devices >> + on the SPMI bus. >> + >> + system-power-controller: true >> + >> + gpio-controller: true >> + >> + '#gpio-cells': >> + const: 2 >> + description: | >> + The first cell is the pin number, the second cell is used to specify flags. >> + See ../gpio/gpio.txt for more information. >> + >> + 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-array >> + oneOf: >> + - items: >> + - const: 12 >> + - items: >> + - const: 34 >> + - items: >> + - const: 12 >> + - const: 34 >> + - items: >> + - const: 123 >> + - items: >> + - const: 1234 >> + >> + regulators: >> + type: object >> + description: List of regulators provided by this controller. >> + >> + patternProperties: >> + "^buck([1-5]|12|34|123|1234)$": > Why do you need ti,multi-phase-id property at all? Having buck123 > implies ti,multi-phase-id=123. I will speak about that with Jerome Neanne (cc of this mail) who uses this multiphase property for the regulator driver. We will consider removing the property, which looks redundant indeed. >> + type: object >> + $ref: /schemas/regulator/regulator.yaml# >> + >> + unevaluatedProperties: false >> + >> + "^ldo[1-4]$": >> + type: object >> + $ref: /schemas/regulator/regulator.yaml# >> + >> + unevaluatedProperties: false >> + >> + allOf: >> + - if: >> + required: >> + - buck12 >> + then: >> + properties: >> + buck123: false >> + buck1234: false >> + - if: >> + required: >> + - buck123 >> + then: >> + properties: >> + buck34: false >> + - if: >> + required: >> + - buck1234 >> + then: >> + properties: >> + buck34: false >> + >> + additionalProperties: false >> + >> + 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 ? Should we add some specific property for RTC/WDG provided by the PMIC ? Should we write another yaml for both of them ? Why shouldn't they use the generic rtc/watchdog yaml ? 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) > >> + >> + watchdog: >> + type: object >> + description: Watchdog provided by this controller. >> + $ref: /schemas/watchdog/watchdog.yaml# >> + >> +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. >> + >> +required: >> + - compatible >> + - reg >> + - interrupts >> + >> +additionalProperties: false >> + >> +examples: >> + - | >> + #include <dt-bindings/interrupt-controller/arm-gic.h> >> + i2c { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + tps6593: pmic@48 { >> + compatible = "ti,tps6593"; >> + reg = <0x48>; >> + ti,primary-pmic; >> + system-power-controller; >> + >> + gpio-controller; >> + #gpio-cells = <2>; >> + >> + pinctrl-names = "default"; >> + pinctrl-0 = <&pmic_irq_pins_default>; >> + interrupt-parent = <&mcu_gpio0>; >> + interrupts = <0 IRQ_TYPE_EDGE_FALLING>; >> + >> + ti,multi-phase-id = <123>; >> + >> + buck123-supply = <&vcc_3v3_sys>; >> + buck4-supply = <&vcc_3v3_sys>; >> + buck5-supply = <&vcc_3v3_sys>; >> + ldo1-supply = <&vcc_3v3_sys>; >> + ldo2-supply = <&vcc_3v3_sys>; >> + ldo3-supply = <&buck5>; >> + ldo4-supply = <&vcc_3v3_sys>; >> + >> + regulators { >> + buck123: buck123 { >> + regulator-name = "vcc_core"; >> + regulator-min-microvolt = <750000>; >> + regulator-max-microvolt = <850000>; >> + regulator-boot-on; >> + regulator-always-on; >> + }; >> + >> + buck4: buck4 { >> + regulator-name = "vcc_1v1"; >> + regulator-min-microvolt = <1100000>; >> + regulator-max-microvolt = <1100000>; >> + regulator-boot-on; >> + regulator-always-on; >> + }; >> + >> + buck5: buck5 { >> + regulator-name = "vcc_1v8_sys"; >> + regulator-min-microvolt = <1800000>; >> + regulator-max-microvolt = <1800000>; >> + regulator-boot-on; >> + regulator-always-on; >> + }; >> + >> + ldo1: ldo1 { >> + regulator-name = "vddshv5_sdio"; >> + regulator-min-microvolt = <3300000>; >> + regulator-max-microvolt = <3300000>; >> + regulator-boot-on; >> + regulator-always-on; >> + }; >> + >> + ldo2: ldo2 { >> + regulator-name = "vpp_1v8"; >> + regulator-min-microvolt = <1800000>; >> + regulator-max-microvolt = <1800000>; >> + regulator-boot-on; >> + regulator-always-on; >> + }; >> + >> + ldo3: ldo3 { >> + regulator-name = "vcc_0v85"; >> + regulator-min-microvolt = <850000>; >> + regulator-max-microvolt = <850000>; >> + regulator-boot-on; >> + regulator-always-on; >> + }; >> + >> + ldo4: ldo4 { >> + regulator-name = "vdda_1v8"; >> + regulator-min-microvolt = <1800000>; >> + regulator-max-microvolt = <1800000>; >> + regulator-boot-on; >> + regulator-always-on; >> + }; >> + }; >> + >> + rtc: rtc { >> + wakeup-source; > No. We do not create nodes for single property. > > >> + }; >> + >> + watchdog: watchdog { >> + timeout-sec = <10>; > Same problem. > > > > Best regards, > Krzysztof >
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@baylibre.com> >>> --- >>> .../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@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. >> 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
On 3/28/23 13:21, Krzysztof Kozlowski wrote: > 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@baylibre.com> >>>> --- >>>> .../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@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. >>> 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)? Basically, the model names are: - lp8764-q1 ('x' came from an error in internal documentation) - tps6593-q1 - tps6594-q1 But...for these 3 pmics, there are 19 PN provided by TI. For instance: tps659413f, tps659411f, ... Each PN gives information about non-volatile memory settings and firmware. So, there can be several PN for a given model name. Which solution is recommended in such situation ? IMO, using the 3 'xxxxxx-q1' names for compatible strings makes sense (that's what already exists in linux for others TI pmics). But if you think that full PN names should be specified to differentiate internal FW/NVM configurations, I will do that and we will get 19 compatible strings. > >>> >>>> + - 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? There's nothing to put there actually. > >> 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. We do not need nodes indeed, since we do not have properties to put in them. > >> 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 >
On 28/03/2023 16:20, Julien Panis wrote: > > > On 3/28/23 13:21, Krzysztof Kozlowski wrote: >> 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@baylibre.com> >>>>> --- >>>>> .../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@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. >>>> 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)? > > Basically, the model names are: > - lp8764-q1 ('x' came from an error in internal documentation) Ah, ok, that explains. > - tps6593-q1 > - tps6594-q1 > > But...for these 3 pmics, there are 19 PN provided > by TI. For instance: tps659413f, tps659411f, ... > Each PN gives information about non-volatile memory > settings and firmware. So, there can be several PN > for a given model name. > > Which solution is recommended in such situation ? > > IMO, using the 3 'xxxxxx-q1' names for compatible > strings makes sense (that's what already exists in > linux for others TI pmics). > But if you think that full PN names should be specified > to differentiate internal FW/NVM configurations, I will do > that and we will get 19 compatible strings. I think it is fine as long as their programming model is exactly the same. > >> >>>> >>>>> + - 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? > > There's nothing to put there actually. > >> >>> 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. > > We do not need nodes indeed, since we do not have properties to put in them. OK. Then please remove the watchdog and rtc nodes. We asked about them to make binding complete, because often they need additional data (e.g. pins or interrupts). In this case the expectation will be to add rtc.yaml and watchdog.yaml refs to main device node (top level) if you ever need their properties (like start-year). Best regards, Krzysztof
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@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,lp8764 + - ti,tps6593 + - ti,tps6594 + + reg: + description: I2C slave address or SPI chip select number. + maxItems: 1 + + ti,primary-pmic: + type: boolean + description: | + Identify the primary PMIC on SPMI bus. + A multi-PMIC synchronization scheme is implemented in the PMIC device + to synchronize the power state changes with other PMIC devices. This is + accomplished through a SPMI bus: the primary PMIC is the controller + device on the SPMI bus, and the secondary PMICs are the target devices + on the SPMI bus. + + system-power-controller: true + + gpio-controller: true + + '#gpio-cells': + const: 2 + description: | + The first cell is the pin number, the second cell is used to specify flags. + See ../gpio/gpio.txt for more information. + + 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-array + oneOf: + - items: + - const: 12 + - items: + - const: 34 + - items: + - const: 12 + - const: 34 + - items: + - const: 123 + - items: + - const: 1234 + + regulators: + 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 + + allOf: + - if: + required: + - buck12 + then: + properties: + buck123: false + buck1234: false + - if: + required: + - buck123 + then: + properties: + buck34: false + - if: + required: + - buck1234 + then: + properties: + buck34: false + + additionalProperties: false + + rtc: + type: object + description: RTC provided by this controller. + $ref: /schemas/rtc/rtc.yaml# + + watchdog: + type: object + description: Watchdog provided by this controller. + $ref: /schemas/watchdog/watchdog.yaml# + +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. + +required: + - compatible + - reg + - interrupts + +additionalProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/arm-gic.h> + i2c { + #address-cells = <1>; + #size-cells = <0>; + + tps6593: pmic@48 { + compatible = "ti,tps6593"; + reg = <0x48>; + ti,primary-pmic; + system-power-controller; + + gpio-controller; + #gpio-cells = <2>; + + pinctrl-names = "default"; + pinctrl-0 = <&pmic_irq_pins_default>; + interrupt-parent = <&mcu_gpio0>; + interrupts = <0 IRQ_TYPE_EDGE_FALLING>; + + ti,multi-phase-id = <123>; + + buck123-supply = <&vcc_3v3_sys>; + buck4-supply = <&vcc_3v3_sys>; + buck5-supply = <&vcc_3v3_sys>; + ldo1-supply = <&vcc_3v3_sys>; + ldo2-supply = <&vcc_3v3_sys>; + ldo3-supply = <&buck5>; + ldo4-supply = <&vcc_3v3_sys>; + + regulators { + buck123: buck123 { + regulator-name = "vcc_core"; + regulator-min-microvolt = <750000>; + regulator-max-microvolt = <850000>; + regulator-boot-on; + regulator-always-on; + }; + + buck4: buck4 { + regulator-name = "vcc_1v1"; + regulator-min-microvolt = <1100000>; + regulator-max-microvolt = <1100000>; + regulator-boot-on; + regulator-always-on; + }; + + buck5: buck5 { + regulator-name = "vcc_1v8_sys"; + regulator-min-microvolt = <1800000>; + regulator-max-microvolt = <1800000>; + regulator-boot-on; + regulator-always-on; + }; + + ldo1: ldo1 { + regulator-name = "vddshv5_sdio"; + regulator-min-microvolt = <3300000>; + regulator-max-microvolt = <3300000>; + regulator-boot-on; + regulator-always-on; + }; + + ldo2: ldo2 { + regulator-name = "vpp_1v8"; + regulator-min-microvolt = <1800000>; + regulator-max-microvolt = <1800000>; + regulator-boot-on; + regulator-always-on; + }; + + ldo3: ldo3 { + regulator-name = "vcc_0v85"; + regulator-min-microvolt = <850000>; + regulator-max-microvolt = <850000>; + regulator-boot-on; + regulator-always-on; + }; + + ldo4: ldo4 { + regulator-name = "vdda_1v8"; + regulator-min-microvolt = <1800000>; + regulator-max-microvolt = <1800000>; + regulator-boot-on; + regulator-always-on; + }; + }; + + rtc: rtc { + wakeup-source; + }; + + watchdog: watchdog { + timeout-sec = <10>; + }; + }; + };
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@baylibre.com> --- .../devicetree/bindings/mfd/ti,tps6594.yaml | 231 ++++++++++++++++++ 1 file changed, 231 insertions(+) create mode 100644 Documentation/devicetree/bindings/mfd/ti,tps6594.yaml