From: ChiYuan Huang <[email protected]>
Add Richtek RT5120 PMIC devicetree document.
Signed-off-by: ChiYuan Huang <[email protected]>
---
.../devicetree/bindings/mfd/richtek,rt5120.yaml | 180 +++++++++++++++++++++
1 file changed, 180 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mfd/richtek,rt5120.yaml
diff --git a/Documentation/devicetree/bindings/mfd/richtek,rt5120.yaml b/Documentation/devicetree/bindings/mfd/richtek,rt5120.yaml
new file mode 100644
index 00000000..376bf73
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/richtek,rt5120.yaml
@@ -0,0 +1,180 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/richtek,rt5120.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Richtek RT5120 PMIC
+
+maintainers:
+ - ChiYuan Huang <[email protected]>
+
+description: |
+ The RT5120 provides four high-efficiency buck converters and one LDO voltage
+ regulator. The device is targeted at providingthe processor voltage, memory,
+ I/O, and peripheral rails in home entertainment devices. The I2C interface is
+ used for dynamic voltage scaling of the processor voltage, power rails on/off
+ sequence control, operation mode selection.
+
+properties:
+ compatible:
+ enum:
+ - richtek,rt5120
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ interrupt-controller: true
+
+ "#interrupt-cells":
+ const: 1
+
+ wakeup-source: true
+
+ richtek,enable-undervolt-hiccup:
+ type: boolean
+ description: |
+ If used, under voltage protection trigger hiccup behavior, else latchup as
+ default
+
+ richtek,enable-overvolt-hiccup:
+ type: boolean
+ description:
+ Like as 'enable-uv-hiccup', it configures over voltage protection to
+ hiccup, else latchup as default
+
+ vin1-supply:
+ description: phandle for buck1 input power source
+
+ vin2-supply:
+ description: phandle for buck2 input power source
+
+ vin3-supply:
+ description: phandle for buck3 input power source
+
+ vin4-supply:
+ description: phandle for buck4 input power source
+
+ vinldo-supply:
+ description: phandle for ldo input power source
+
+ regulators:
+ type: object
+
+ patternProperties:
+ "^buck[1-4]$":
+ type: object
+ $ref: /schemas/regulator/regulator.yaml#
+
+ properties:
+ regulator-allowed-modes:
+ description: |
+ Used to specify the allowed buck converter operating mode
+ mode mapping:
+ 0: auto mode
+ 1: force pwm mode
+ items:
+ enum: [0, 1]
+
+ unevaluatedProperties: false
+
+ "^(ldo|exten)$":
+ type: object
+ $ref: /schemas/regulator/regulator.yaml#
+
+ additionalProperties: false
+
+ powerkey:
+ type: object
+ description:
+ The power key driver may be optional. If not used, change node status to
+ 'disabled'
+
+ properties:
+ compatible:
+ enum:
+ - richtek,rt5120-pwrkey
+
+ required:
+ - compatible
+
+ additionalProperties: false
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - '#interrupt-cells'
+ - interrupt-controller
+ - regulators
+ - powerkey
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ pmic@62 {
+ compatible = "richtek,rt5120";
+ reg = <0x62>;
+ interrupts-extended = <&gpio_intc 32 IRQ_TYPE_LEVEL_LOW>;
+ interrupt-controller;
+ #interrupt-cells = <1>;
+ wakeup-source;
+
+ regulators {
+ buck1 {
+ regulator-name = "rt5120-buck1";
+ regulator-min-microvolt = <600000>;
+ regulator-max-microvolt = <1393750>;
+ regulator-allowed-modes = <0 1>;
+ regulator-boot-on;
+ };
+ buck2 {
+ regulator-name = "rt5120-buck2";
+ regulator-min-microvolt = <1100000>;
+ regulator-max-microvolt = <1100000>;
+ regulator-allowed-modes = <0 1>;
+ regulator-always-on;
+ };
+ buck3 {
+ regulator-name = "rt5120-buck3";
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ regulator-allowed-modes = <0 1>;
+ regulator-always-on;
+ };
+ buck4 {
+ regulator-name = "rt5120-buck4";
+ regulator-min-microvolt = <3300000>;
+ regulator-max-microvolt = <3300000>;
+ regulator-allowed-modes = <0 1>;
+ regulator-always-on;
+ };
+ ldo {
+ regulator-name = "rt5120-ldo";
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ regulator-always-on;
+ };
+ exten {
+ regulator-name = "rt5120-exten";
+ regulator-min-microvolt = <3000000>;
+ regulator-max-microvolt = <3000000>;
+ regulator-always-on;
+ };
+ };
+ powerkey {
+ status = "okay";
+ compatible = "richtek,rt5120-pwrkey";
+ };
+ };
+ };
--
2.7.4
On 07/06/2022 07:52, cy_huang wrote:
> From: ChiYuan Huang <[email protected]>
>
> Add Richtek RT5120 PMIC devicetree document.
>
> Signed-off-by: ChiYuan Huang <[email protected]>
> ---
> .../devicetree/bindings/mfd/richtek,rt5120.yaml | 180 +++++++++++++++++++++
> 1 file changed, 180 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mfd/richtek,rt5120.yaml
>
> diff --git a/Documentation/devicetree/bindings/mfd/richtek,rt5120.yaml b/Documentation/devicetree/bindings/mfd/richtek,rt5120.yaml
> new file mode 100644
> index 00000000..376bf73
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/richtek,rt5120.yaml
> @@ -0,0 +1,180 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mfd/richtek,rt5120.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Richtek RT5120 PMIC
> +
> +maintainers:
> + - ChiYuan Huang <[email protected]>
> +
> +description: |
> + The RT5120 provides four high-efficiency buck converters and one LDO voltage
> + regulator. The device is targeted at providingthe processor voltage, memory,
> + I/O, and peripheral rails in home entertainment devices. The I2C interface is
> + used for dynamic voltage scaling of the processor voltage, power rails on/off
> + sequence control, operation mode selection.
> +
> +properties:
> + compatible:
> + enum:
> + - richtek,rt5120
> +
> + reg:
> + maxItems: 1
> +
> + interrupts:
> + maxItems: 1
> +
> + interrupt-controller: true
> +
> + "#interrupt-cells":
> + const: 1
> +
> + wakeup-source: true
> +
> + richtek,enable-undervolt-hiccup:
> + type: boolean
> + description: |
> + If used, under voltage protection trigger hiccup behavior, else latchup as
> + default
> +
> + richtek,enable-overvolt-hiccup:
> + type: boolean
> + description:
> + Like as 'enable-uv-hiccup', it configures over voltage protection to
> + hiccup, else latchup as default
> +
> + vin1-supply:
> + description: phandle for buck1 input power source
> +
> + vin2-supply:
> + description: phandle for buck2 input power source
> +
> + vin3-supply:
> + description: phandle for buck3 input power source
> +
> + vin4-supply:
> + description: phandle for buck4 input power source
> +
> + vinldo-supply:
> + description: phandle for ldo input power source
> +
> + regulators:
> + type: object
> +
> + patternProperties:
> + "^buck[1-4]$":
> + type: object
> + $ref: /schemas/regulator/regulator.yaml#
> +
> + properties:
> + regulator-allowed-modes:
> + description: |
> + Used to specify the allowed buck converter operating mode
> + mode mapping:
> + 0: auto mode
> + 1: force pwm mode
> + items:
> + enum: [0, 1]
> +
> + unevaluatedProperties: false
Better to put it after '$ref' for readability.
> +
> + "^(ldo|exten)$":
> + type: object
> + $ref: /schemas/regulator/regulator.yaml#
You need here unevaluatedProperties:false as well (for the ldo/exten
properties)
> +
> + additionalProperties: false
> +
> + powerkey:
> + type: object
> + description:
> + The power key driver may be optional. If not used, change node status to
> + 'disabled'
This description is not helpful, does not describe the hardware. Please
describe hardware, not Devicetree usage.
> +
> + properties:
> + compatible:
> + enum:
> + - richtek,rt5120-pwrkey
> +
> + required:
> + - compatible
> +
> + additionalProperties: false
> +
> +required:
> + - compatible
> + - reg
> + - interrupts
> + - '#interrupt-cells'
> + - interrupt-controller
> + - regulators
> + - powerkey
You wrote powerkey is optional... so the node should not be required, right?
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/interrupt-controller/irq.h>
> +
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + pmic@62 {
> + compatible = "richtek,rt5120";
> + reg = <0x62>;
> + interrupts-extended = <&gpio_intc 32 IRQ_TYPE_LEVEL_LOW>;
> + interrupt-controller;
> + #interrupt-cells = <1>;
> + wakeup-source;
> +
> + regulators {
> + buck1 {
> + regulator-name = "rt5120-buck1";
> + regulator-min-microvolt = <600000>;
> + regulator-max-microvolt = <1393750>;
> + regulator-allowed-modes = <0 1>;
> + regulator-boot-on;
> + };
> + buck2 {
> + regulator-name = "rt5120-buck2";
> + regulator-min-microvolt = <1100000>;
> + regulator-max-microvolt = <1100000>;
> + regulator-allowed-modes = <0 1>;
> + regulator-always-on;
> + };
> + buck3 {
> + regulator-name = "rt5120-buck3";
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <1800000>;
> + regulator-allowed-modes = <0 1>;
> + regulator-always-on;
> + };
> + buck4 {
> + regulator-name = "rt5120-buck4";
> + regulator-min-microvolt = <3300000>;
> + regulator-max-microvolt = <3300000>;
> + regulator-allowed-modes = <0 1>;
> + regulator-always-on;
> + };
> + ldo {
> + regulator-name = "rt5120-ldo";
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <1800000>;
> + regulator-always-on;
> + };
> + exten {
> + regulator-name = "rt5120-exten";
> + regulator-min-microvolt = <3000000>;
> + regulator-max-microvolt = <3000000>;
> + regulator-always-on;
> + };
> + };
> + powerkey {
> + status = "okay";
Messed up indentation. No need for status in examples.
> + compatible = "richtek,rt5120-pwrkey";
> + };
> + };
> + };
Best regards,
Krzysztof
On Tue, Jun 07, 2022 at 01:52:38PM +0800, cy_huang wrote:
> + richtek,enable-undervolt-hiccup:
> + type: boolean
> + description: |
> + If used, under voltage protection trigger hiccup behavior, else latchup as
> + default
> +
> + richtek,enable-overvolt-hiccup:
> + type: boolean
> + description:
> + Like as 'enable-uv-hiccup', it configures over voltage protection to
> + hiccup, else latchup as default
Why would we not want to have these enabled? Unless there's some cost
to having them on it seems safer and more helpful to look for problems
with regulation all the time, it might help debug a system.
Krzysztof Kozlowski <[email protected]> 於 2022年6月7日 週二 下午7:52寫道：
>
> On 07/06/2022 07:52, cy_huang wrote:
> > From: ChiYuan Huang <[email protected]>
> >
> > Add Richtek RT5120 PMIC devicetree document.
> >
> > Signed-off-by: ChiYuan Huang <[email protected]>
> > ---
> > .../devicetree/bindings/mfd/richtek,rt5120.yaml | 180 +++++++++++++++++++++
> > 1 file changed, 180 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/mfd/richtek,rt5120.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/mfd/richtek,rt5120.yaml b/Documentation/devicetree/bindings/mfd/richtek,rt5120.yaml
> > new file mode 100644
> > index 00000000..376bf73
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mfd/richtek,rt5120.yaml
> > @@ -0,0 +1,180 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/mfd/richtek,rt5120.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Richtek RT5120 PMIC
> > +
> > +maintainers:
> > + - ChiYuan Huang <[email protected]>
> > +
> > +description: |
> > + The RT5120 provides four high-efficiency buck converters and one LDO voltage
> > + regulator. The device is targeted at providingthe processor voltage, memory,
> > + I/O, and peripheral rails in home entertainment devices. The I2C interface is
> > + used for dynamic voltage scaling of the processor voltage, power rails on/off
> > + sequence control, operation mode selection.
> > +
> > +properties:
> > + compatible:
> > + enum:
> > + - richtek,rt5120
> > +
> > + reg:
> > + maxItems: 1
> > +
> > + interrupts:
> > + maxItems: 1
> > +
> > + interrupt-controller: true
> > +
> > + "#interrupt-cells":
> > + const: 1
> > +
> > + wakeup-source: true
> > +
> > + richtek,enable-undervolt-hiccup:
> > + type: boolean
> > + description: |
> > + If used, under voltage protection trigger hiccup behavior, else latchup as
> > + default
> > +
> > + richtek,enable-overvolt-hiccup:
> > + type: boolean
> > + description:
> > + Like as 'enable-uv-hiccup', it configures over voltage protection to
> > + hiccup, else latchup as default
> > +
> > + vin1-supply:
> > + description: phandle for buck1 input power source
> > +
> > + vin2-supply:
> > + description: phandle for buck2 input power source
> > +
> > + vin3-supply:
> > + description: phandle for buck3 input power source
> > +
> > + vin4-supply:
> > + description: phandle for buck4 input power source
> > +
> > + vinldo-supply:
> > + description: phandle for ldo input power source
> > +
> > + regulators:
> > + type: object
> > +
> > + patternProperties:
> > + "^buck[1-4]$":
> > + type: object
> > + $ref: /schemas/regulator/regulator.yaml#
> > +
> > + properties:
> > + regulator-allowed-modes:
> > + description: |
> > + Used to specify the allowed buck converter operating mode
> > + mode mapping:
> > + 0: auto mode
> > + 1: force pwm mode
> > + items:
> > + enum: [0, 1]
> > +
> > + unevaluatedProperties: false
>
> Better to put it after '$ref' for readability.
OK, Fix in next
>
> > +
> > + "^(ldo|exten)$":
> > + type: object
> > + $ref: /schemas/regulator/regulator.yaml#
>
> You need here unevaluatedProperties:false as well (for the ldo/exten
> properties)
Fix in next.
>
> > +
> > + additionalProperties: false
> > +
> > + powerkey:
> > + type: object
> > + description:
> > + The power key driver may be optional. If not used, change node status to
> > + 'disabled'
>
> This description is not helpful, does not describe the hardware. Please
> describe hardware, not Devicetree usage.
That's because it's a PMIC. Power key is also connected to it.
For power key press, all power rails will start to power up.
But in the application, there may be other PMIC that's also connected
to power key.
That's why this power key driver may need to be optional.
One system only need one driver to report the power key status.
Currently in some linux OS, it uses the auto module loading mechanism.
All kernel module files may be all the same, but it uses the
devicetree to decide how many devices
need to be declared. Since RT5120 power key device may be optional,
following by mfd_add_device, if of_node is
found, and status is "disabled", the sub device would be skipped.
Actually, I'm also confused about it. There may be three ways to implement it
1. not to build this kernel module -> seems to violate my above application
2. Use one boolean property to decide power key cell need to be used or not??
3. like as now, use the node status to decide it.
Is there the better way to do it?
>
> > +
> > + properties:
> > + compatible:
> > + enum:
> > + - richtek,rt5120-pwrkey
> > +
> > + required:
> > + - compatible
> > +
> > + additionalProperties: false
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - interrupts
> > + - '#interrupt-cells'
> > + - interrupt-controller
> > + - regulators
> > + - powerkey
>
> You wrote powerkey is optional... so the node should not be required, right?
Yes, required. Please refer to the above explanation.
>
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > + #include <dt-bindings/interrupt-controller/irq.h>
> > +
> > + i2c {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + pmic@62 {
> > + compatible = "richtek,rt5120";
> > + reg = <0x62>;
> > + interrupts-extended = <&gpio_intc 32 IRQ_TYPE_LEVEL_LOW>;
> > + interrupt-controller;
> > + #interrupt-cells = <1>;
> > + wakeup-source;
> > +
> > + regulators {
> > + buck1 {
> > + regulator-name = "rt5120-buck1";
> > + regulator-min-microvolt = <600000>;
> > + regulator-max-microvolt = <1393750>;
> > + regulator-allowed-modes = <0 1>;
> > + regulator-boot-on;
> > + };
> > + buck2 {
> > + regulator-name = "rt5120-buck2";
> > + regulator-min-microvolt = <1100000>;
> > + regulator-max-microvolt = <1100000>;
> > + regulator-allowed-modes = <0 1>;
> > + regulator-always-on;
> > + };
> > + buck3 {
> > + regulator-name = "rt5120-buck3";
> > + regulator-min-microvolt = <1800000>;
> > + regulator-max-microvolt = <1800000>;
> > + regulator-allowed-modes = <0 1>;
> > + regulator-always-on;
> > + };
> > + buck4 {
> > + regulator-name = "rt5120-buck4";
> > + regulator-min-microvolt = <3300000>;
> > + regulator-max-microvolt = <3300000>;
> > + regulator-allowed-modes = <0 1>;
> > + regulator-always-on;
> > + };
> > + ldo {
> > + regulator-name = "rt5120-ldo";
> > + regulator-min-microvolt = <1800000>;
> > + regulator-max-microvolt = <1800000>;
> > + regulator-always-on;
> > + };
> > + exten {
> > + regulator-name = "rt5120-exten";
> > + regulator-min-microvolt = <3000000>;
> > + regulator-max-microvolt = <3000000>;
> > + regulator-always-on;
> > + };
> > + };
> > + powerkey {
> > + status = "okay";
>
> Messed up indentation. No need for status in examples.
Fix in next.
>
> > + compatible = "richtek,rt5120-pwrkey";
> > + };
> > + };
> > + };
>
>
> Best regards,
> Krzysztof
Mark Brown <[email protected]> 於 2022年6月8日 週三 上午3:02寫道：
>
> On Tue, Jun 07, 2022 at 01:52:38PM +0800, cy_huang wrote:
>
> > + richtek,enable-undervolt-hiccup:
> > + type: boolean
> > + description: |
> > + If used, under voltage protection trigger hiccup behavior, else latchup as
> > + default
> > +
> > + richtek,enable-overvolt-hiccup:
> > + type: boolean
> > + description:
> > + Like as 'enable-uv-hiccup', it configures over voltage protection to
> > + hiccup, else latchup as default
>
> Why would we not want to have these enabled? Unless there's some cost
> to having them on it seems safer and more helpful to look for problems
> with regulation all the time, it might help debug a system.
The default latch mode would be safer during power rails boot up.
If during power rails boot up, any oc/ov protection triggered, latch
mode will keep power rails off.
Only toggle the external pin to re-enable can trigger next power up.
Like as your said, hiccup can help the debug for this kind of HW issue.
So after all power rails and system booted up to make the user choose
the hiccup option.
That's why the property here to enable hiccup mode.
Krzysztof Kozlowski <[email protected]> 於 2022年6月8日 週三 下午3:02寫道：
>
> On 08/06/2022 04:52, ChiYuan Huang wrote:
> > Krzysztof Kozlowski <[email protected]> 於 2022年6月7日 週二 下午7:52寫道：
> >>
> >> On 07/06/2022 07:52, cy_huang wrote:
> >>> From: ChiYuan Huang <[email protected]>
> >>>
> >>> Add Richtek RT5120 PMIC devicetree document.
> >>>
> >>> Signed-off-by: ChiYuan Huang <[email protected]>
> >>> ---
> >>> .../devicetree/bindings/mfd/richtek,rt5120.yaml | 180 +++++++++++++++++++++
> >>> 1 file changed, 180 insertions(+)
> >>> create mode 100644 Documentation/devicetree/bindings/mfd/richtek,rt5120.yaml
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/mfd/richtek,rt5120.yaml b/Documentation/devicetree/bindings/mfd/richtek,rt5120.yaml
> >>> new file mode 100644
> >>> index 00000000..376bf73
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/mfd/richtek,rt5120.yaml
> >>> @@ -0,0 +1,180 @@
> >>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> >>> +%YAML 1.2
> >>> +---
> >>> +$id: http://devicetree.org/schemas/mfd/richtek,rt5120.yaml#
> >>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>> +
> >>> +title: Richtek RT5120 PMIC
> >>> +
> >>> +maintainers:
> >>> + - ChiYuan Huang <[email protected]>
> >>> +
> >>> +description: |
> >>> + The RT5120 provides four high-efficiency buck converters and one LDO voltage
> >>> + regulator. The device is targeted at providingthe processor voltage, memory,
> >>> + I/O, and peripheral rails in home entertainment devices. The I2C interface is
> >>> + used for dynamic voltage scaling of the processor voltage, power rails on/off
> >>> + sequence control, operation mode selection.
> >>> +
> >>> +properties:
> >>> + compatible:
> >>> + enum:
> >>> + - richtek,rt5120
> >>> +
> >>> + reg:
> >>> + maxItems: 1
> >>> +
> >>> + interrupts:
> >>> + maxItems: 1
>
> Your powerkey driver takes two interrupts. You should describe them in
> the powerkey.
>
> >>> +
> >>> + interrupt-controller: true
> >>> +
> >>> + "#interrupt-cells":
> >>> + const: 1
> >>> +
> >>> + wakeup-source: true
> >>> +
> >>> + richtek,enable-undervolt-hiccup:
> >>> + type: boolean
> >>> + description: |
> >>> + If used, under voltage protection trigger hiccup behavior, else latchup as
> >>> + default
> >>> +
> >>> + richtek,enable-overvolt-hiccup:
> >>> + type: boolean
> >>> + description:
> >>> + Like as 'enable-uv-hiccup', it configures over voltage protection to
> >>> + hiccup, else latchup as default
> >>> +
> >>> + vin1-supply:
> >>> + description: phandle for buck1 input power source
> >>> +
> >>> + vin2-supply:
> >>> + description: phandle for buck2 input power source
> >>> +
> >>> + vin3-supply:
> >>> + description: phandle for buck3 input power source
> >>> +
> >>> + vin4-supply:
> >>> + description: phandle for buck4 input power source
> >>> +
> >>> + vinldo-supply:
> >>> + description: phandle for ldo input power source
> >>> +
> >>> + regulators:
> >>> + type: object
> >>> +
> >>> + patternProperties:
> >>> + "^buck[1-4]$":
> >>> + type: object
> >>> + $ref: /schemas/regulator/regulator.yaml#
> >>> +
> >>> + properties:
> >>> + regulator-allowed-modes:
> >>> + description: |
> >>> + Used to specify the allowed buck converter operating mode
> >>> + mode mapping:
> >>> + 0: auto mode
> >>> + 1: force pwm mode
> >>> + items:
> >>> + enum: [0, 1]
> >>> +
> >>> + unevaluatedProperties: false
> >>
> >> Better to put it after '$ref' for readability.
> > OK, Fix in next
> >>
> >>> +
> >>> + "^(ldo|exten)$":
> >>> + type: object
> >>> + $ref: /schemas/regulator/regulator.yaml#
> >>
> >> You need here unevaluatedProperties:false as well (for the ldo/exten
> >> properties)
> > Fix in next.
> >>
> >>> +
> >>> + additionalProperties: false
> >>> +
> >>> + powerkey:
> >>> + type: object
> >>> + description:
> >>> + The power key driver may be optional. If not used, change node status to
> >>> + 'disabled'
> >>
> >> This description is not helpful, does not describe the hardware. Please
> >> describe hardware, not Devicetree usage.
> > That's because it's a PMIC. Power key is also connected to it.
> > For power key press, all power rails will start to power up.
> > But in the application, there may be other PMIC that's also connected
> > to power key.
> > That's why this power key driver may need to be optional.
> > One system only need one driver to report the power key status.
> >
> > Currently in some linux OS, it uses the auto module loading mechanism.
> > All kernel module files may be all the same, but it uses the
> > devicetree to decide how many devices
> > need to be declared. Since RT5120 power key device may be optional,
> > following by mfd_add_device, if of_node is
> > found, and status is "disabled", the sub device would be skipped.
> >
> > Actually, I'm also confused about it. There may be three ways to implement it
> > 1. not to build this kernel module -> seems to violate my above application
> > 2. Use one boolean property to decide power key cell need to be used or not??
> > 3. like as now, use the node status to decide it.
> >
> > Is there the better way to do it?
>
> The status does not determine whether device in the bindings is optional
> or not. Rather it's presence. In the term of bindings the "optional"
> means that something might not be there physically. E.g. clock line
> connected or not. System implementation - MFD, power off handling - is
> here (almost) irrelevant.
>
> In your case, the power key feature seems to be there always, so the
> "powerkey" node should be required and not disabled. Don't mention in
> description of hardware anything about disabling it or not.
>
> In your application, I would say it is interesting design that someone
> connects one power up line to two different PMICs in a conflicting way.
> This sounds like total mistake from hardware point of view.
>
> Anyway it is not the job for this patch to solve such conflicts.
>
Thanks, I think your point is 'optional' keyword.
If there's only redundant description line, I may decide to remove it.
The property name already show its usage.
> Best regards,
> Krzysztof
On 08/06/2022 04:52, ChiYuan Huang wrote:
> Krzysztof Kozlowski <[email protected]> 於 2022年6月7日 週二 下午7:52寫道：
>>
>> On 07/06/2022 07:52, cy_huang wrote:
>>> From: ChiYuan Huang <[email protected]>
>>>
>>> Add Richtek RT5120 PMIC devicetree document.
>>>
>>> Signed-off-by: ChiYuan Huang <[email protected]>
>>> ---
>>> .../devicetree/bindings/mfd/richtek,rt5120.yaml | 180 +++++++++++++++++++++
>>> 1 file changed, 180 insertions(+)
>>> create mode 100644 Documentation/devicetree/bindings/mfd/richtek,rt5120.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/mfd/richtek,rt5120.yaml b/Documentation/devicetree/bindings/mfd/richtek,rt5120.yaml
>>> new file mode 100644
>>> index 00000000..376bf73
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/mfd/richtek,rt5120.yaml
>>> @@ -0,0 +1,180 @@
>>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/mfd/richtek,rt5120.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Richtek RT5120 PMIC
>>> +
>>> +maintainers:
>>> + - ChiYuan Huang <[email protected]>
>>> +
>>> +description: |
>>> + The RT5120 provides four high-efficiency buck converters and one LDO voltage
>>> + regulator. The device is targeted at providingthe processor voltage, memory,
>>> + I/O, and peripheral rails in home entertainment devices. The I2C interface is
>>> + used for dynamic voltage scaling of the processor voltage, power rails on/off
>>> + sequence control, operation mode selection.
>>> +
>>> +properties:
>>> + compatible:
>>> + enum:
>>> + - richtek,rt5120
>>> +
>>> + reg:
>>> + maxItems: 1
>>> +
>>> + interrupts:
>>> + maxItems: 1
Your powerkey driver takes two interrupts. You should describe them in
the powerkey.
>>> +
>>> + interrupt-controller: true
>>> +
>>> + "#interrupt-cells":
>>> + const: 1
>>> +
>>> + wakeup-source: true
>>> +
>>> + richtek,enable-undervolt-hiccup:
>>> + type: boolean
>>> + description: |
>>> + If used, under voltage protection trigger hiccup behavior, else latchup as
>>> + default
>>> +
>>> + richtek,enable-overvolt-hiccup:
>>> + type: boolean
>>> + description:
>>> + Like as 'enable-uv-hiccup', it configures over voltage protection to
>>> + hiccup, else latchup as default
>>> +
>>> + vin1-supply:
>>> + description: phandle for buck1 input power source
>>> +
>>> + vin2-supply:
>>> + description: phandle for buck2 input power source
>>> +
>>> + vin3-supply:
>>> + description: phandle for buck3 input power source
>>> +
>>> + vin4-supply:
>>> + description: phandle for buck4 input power source
>>> +
>>> + vinldo-supply:
>>> + description: phandle for ldo input power source
>>> +
>>> + regulators:
>>> + type: object
>>> +
>>> + patternProperties:
>>> + "^buck[1-4]$":
>>> + type: object
>>> + $ref: /schemas/regulator/regulator.yaml#
>>> +
>>> + properties:
>>> + regulator-allowed-modes:
>>> + description: |
>>> + Used to specify the allowed buck converter operating mode
>>> + mode mapping:
>>> + 0: auto mode
>>> + 1: force pwm mode
>>> + items:
>>> + enum: [0, 1]
>>> +
>>> + unevaluatedProperties: false
>>
>> Better to put it after '$ref' for readability.
> OK, Fix in next
>>
>>> +
>>> + "^(ldo|exten)$":
>>> + type: object
>>> + $ref: /schemas/regulator/regulator.yaml#
>>
>> You need here unevaluatedProperties:false as well (for the ldo/exten
>> properties)
> Fix in next.
>>
>>> +
>>> + additionalProperties: false
>>> +
>>> + powerkey:
>>> + type: object
>>> + description:
>>> + The power key driver may be optional. If not used, change node status to
>>> + 'disabled'
>>
>> This description is not helpful, does not describe the hardware. Please
>> describe hardware, not Devicetree usage.
> That's because it's a PMIC. Power key is also connected to it.
> For power key press, all power rails will start to power up.
> But in the application, there may be other PMIC that's also connected
> to power key.
> That's why this power key driver may need to be optional.
> One system only need one driver to report the power key status.
>
> Currently in some linux OS, it uses the auto module loading mechanism.
> All kernel module files may be all the same, but it uses the
> devicetree to decide how many devices
> need to be declared. Since RT5120 power key device may be optional,
> following by mfd_add_device, if of_node is
> found, and status is "disabled", the sub device would be skipped.
>
> Actually, I'm also confused about it. There may be three ways to implement it
> 1. not to build this kernel module -> seems to violate my above application
> 2. Use one boolean property to decide power key cell need to be used or not??
> 3. like as now, use the node status to decide it.
>
> Is there the better way to do it?
The status does not determine whether device in the bindings is optional
or not. Rather it's presence. In the term of bindings the "optional"
means that something might not be there physically. E.g. clock line
connected or not. System implementation - MFD, power off handling - is
here (almost) irrelevant.
In your case, the power key feature seems to be there always, so the
"powerkey" node should be required and not disabled. Don't mention in
description of hardware anything about disabling it or not.
In your application, I would say it is interesting design that someone
connects one power up line to two different PMICs in a conflicting way.
This sounds like total mistake from hardware point of view.
Anyway it is not the job for this patch to solve such conflicts.
Best regards,
Krzysztof
On 08/06/2022 09:25, ChiYuan Huang wrote:
> Krzysztof Kozlowski <[email protected]> 於 2022年6月8日 週三 下午3:02寫道：
>>
>> On 08/06/2022 04:52, ChiYuan Huang wrote:
>>> Krzysztof Kozlowski <[email protected]> 於 2022年6月7日 週二 下午7:52寫道：
>>>>
>>>> On 07/06/2022 07:52, cy_huang wrote:
>>>>> From: ChiYuan Huang <[email protected]>
>>>>>
>>>>> Add Richtek RT5120 PMIC devicetree document.
>>>>>
>>>>> Signed-off-by: ChiYuan Huang <[email protected]>
>>>>> ---
>>>>> .../devicetree/bindings/mfd/richtek,rt5120.yaml | 180 +++++++++++++++++++++
>>>>> 1 file changed, 180 insertions(+)
>>>>> create mode 100644 Documentation/devicetree/bindings/mfd/richtek,rt5120.yaml
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/mfd/richtek,rt5120.yaml b/Documentation/devicetree/bindings/mfd/richtek,rt5120.yaml
>>>>> new file mode 100644
>>>>> index 00000000..376bf73
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/mfd/richtek,rt5120.yaml
>>>>> @@ -0,0 +1,180 @@
>>>>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>>>>> +%YAML 1.2
>>>>> +---
>>>>> +$id: http://devicetree.org/schemas/mfd/richtek,rt5120.yaml#
>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>> +
>>>>> +title: Richtek RT5120 PMIC
>>>>> +
>>>>> +maintainers:
>>>>> + - ChiYuan Huang <[email protected]>
>>>>> +
>>>>> +description: |
>>>>> + The RT5120 provides four high-efficiency buck converters and one LDO voltage
>>>>> + regulator. The device is targeted at providingthe processor voltage, memory,
>>>>> + I/O, and peripheral rails in home entertainment devices. The I2C interface is
>>>>> + used for dynamic voltage scaling of the processor voltage, power rails on/off
>>>>> + sequence control, operation mode selection.
>>>>> +
>>>>> +properties:
>>>>> + compatible:
>>>>> + enum:
>>>>> + - richtek,rt5120
>>>>> +
>>>>> + reg:
>>>>> + maxItems: 1
>>>>> +
>>>>> + interrupts:
>>>>> + maxItems: 1
>>
>> Your powerkey driver takes two interrupts. You should describe them in
>> the powerkey.
>>
>>>>> +
>>>>> + interrupt-controller: true
>>>>> +
>>>>> + "#interrupt-cells":
>>>>> + const: 1
>>>>> +
>>>>> + wakeup-source: true
>>>>> +
>>>>> + richtek,enable-undervolt-hiccup:
>>>>> + type: boolean
>>>>> + description: |
>>>>> + If used, under voltage protection trigger hiccup behavior, else latchup as
>>>>> + default
>>>>> +
>>>>> + richtek,enable-overvolt-hiccup:
>>>>> + type: boolean
>>>>> + description:
>>>>> + Like as 'enable-uv-hiccup', it configures over voltage protection to
>>>>> + hiccup, else latchup as default
>>>>> +
>>>>> + vin1-supply:
>>>>> + description: phandle for buck1 input power source
>>>>> +
>>>>> + vin2-supply:
>>>>> + description: phandle for buck2 input power source
>>>>> +
>>>>> + vin3-supply:
>>>>> + description: phandle for buck3 input power source
>>>>> +
>>>>> + vin4-supply:
>>>>> + description: phandle for buck4 input power source
>>>>> +
>>>>> + vinldo-supply:
>>>>> + description: phandle for ldo input power source
>>>>> +
>>>>> + regulators:
>>>>> + type: object
>>>>> +
>>>>> + patternProperties:
>>>>> + "^buck[1-4]$":
>>>>> + type: object
>>>>> + $ref: /schemas/regulator/regulator.yaml#
>>>>> +
>>>>> + properties:
>>>>> + regulator-allowed-modes:
>>>>> + description: |
>>>>> + Used to specify the allowed buck converter operating mode
>>>>> + mode mapping:
>>>>> + 0: auto mode
>>>>> + 1: force pwm mode
>>>>> + items:
>>>>> + enum: [0, 1]
>>>>> +
>>>>> + unevaluatedProperties: false
>>>>
>>>> Better to put it after '$ref' for readability.
>>> OK, Fix in next
>>>>
>>>>> +
>>>>> + "^(ldo|exten)$":
>>>>> + type: object
>>>>> + $ref: /schemas/regulator/regulator.yaml#
>>>>
>>>> You need here unevaluatedProperties:false as well (for the ldo/exten
>>>> properties)
>>> Fix in next.
>>>>
>>>>> +
>>>>> + additionalProperties: false
>>>>> +
>>>>> + powerkey:
>>>>> + type: object
>>>>> + description:
>>>>> + The power key driver may be optional. If not used, change node status to
>>>>> + 'disabled'
>>>>
>>>> This description is not helpful, does not describe the hardware. Please
>>>> describe hardware, not Devicetree usage.
>>> That's because it's a PMIC. Power key is also connected to it.
>>> For power key press, all power rails will start to power up.
>>> But in the application, there may be other PMIC that's also connected
>>> to power key.
>>> That's why this power key driver may need to be optional.
>>> One system only need one driver to report the power key status.
>>>
>>> Currently in some linux OS, it uses the auto module loading mechanism.
>>> All kernel module files may be all the same, but it uses the
>>> devicetree to decide how many devices
>>> need to be declared. Since RT5120 power key device may be optional,
>>> following by mfd_add_device, if of_node is
>>> found, and status is "disabled", the sub device would be skipped.
>>>
>>> Actually, I'm also confused about it. There may be three ways to implement it
>>> 1. not to build this kernel module -> seems to violate my above application
>>> 2. Use one boolean property to decide power key cell need to be used or not??
>>> 3. like as now, use the node status to decide it.
>>>
>>> Is there the better way to do it?
>>
>> The status does not determine whether device in the bindings is optional
>> or not. Rather it's presence. In the term of bindings the "optional"
>> means that something might not be there physically. E.g. clock line
>> connected or not. System implementation - MFD, power off handling - is
>> here (almost) irrelevant.
>>
>> In your case, the power key feature seems to be there always, so the
>> "powerkey" node should be required and not disabled. Don't mention in
>> description of hardware anything about disabling it or not.
>>
>> In your application, I would say it is interesting design that someone
>> connects one power up line to two different PMICs in a conflicting way.
>> This sounds like total mistake from hardware point of view.
>>
>> Anyway it is not the job for this patch to solve such conflicts.
>>
> Thanks, I think your point is 'optional' keyword.
> If there's only redundant description line, I may decide to remove it.
> The property name already show its usage.
I repeated my point twice - your description is not relevant to the
bindings:
"This description is not helpful, does not describe the hardware. Please
describe hardware, not Devicetree usage."
"System implementation - MFD, power off handling - is
here (almost) irrelevant."
"Don't mention in
description of hardware anything about disabling it or not."
Please describe briefly the hardware behind this property, not status or
other Devicetree usage.
Best regards,
Krzysztof