* [PATCH 1/4] dt-binding: mfd: Add Richtek RT5120 PMIC support
2022-06-07 5:52 [PATCH 0/4] Add Richtek RT5120 PMIC support cy_huang
@ 2022-06-07 5:52 ` cy_huang
2022-06-07 11:52 ` Krzysztof Kozlowski
2022-06-07 19:02 ` Mark Brown
2022-06-07 5:52 ` [PATCH 2/4] mfd: rt5120: Add Richtek " cy_huang
` (2 subsequent siblings)
3 siblings, 2 replies; 21+ messages in thread
From: cy_huang @ 2022-06-07 5:52 UTC (permalink / raw)
To: robh+dt, krzysztof.kozlowski+dt, lee.jones, broonie, dmitry.torokhov
Cc: lgirdwood, cy_huang, devicetree, linux-kernel, linux-input
From: ChiYuan Huang <cy_huang@richtek.com>
Add Richtek RT5120 PMIC devicetree document.
Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
---
.../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 <cy_huang@richtek.com>
+
+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
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 1/4] dt-binding: mfd: Add Richtek RT5120 PMIC support
2022-06-07 5:52 ` [PATCH 1/4] dt-binding: mfd: " cy_huang
@ 2022-06-07 11:52 ` Krzysztof Kozlowski
2022-06-08 2:52 ` ChiYuan Huang
2022-06-07 19:02 ` Mark Brown
1 sibling, 1 reply; 21+ messages in thread
From: Krzysztof Kozlowski @ 2022-06-07 11:52 UTC (permalink / raw)
To: cy_huang, robh+dt, krzysztof.kozlowski+dt, lee.jones, broonie,
dmitry.torokhov
Cc: lgirdwood, cy_huang, devicetree, linux-kernel, linux-input
On 07/06/2022 07:52, cy_huang wrote:
> From: ChiYuan Huang <cy_huang@richtek.com>
>
> Add Richtek RT5120 PMIC devicetree document.
>
> Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> ---
> .../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 <cy_huang@richtek.com>
> +
> +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
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 1/4] dt-binding: mfd: Add Richtek RT5120 PMIC support
2022-06-07 11:52 ` Krzysztof Kozlowski
@ 2022-06-08 2:52 ` ChiYuan Huang
2022-06-08 7:01 ` Krzysztof Kozlowski
0 siblings, 1 reply; 21+ messages in thread
From: ChiYuan Huang @ 2022-06-08 2:52 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Rob Herring, Krzysztof Kozlowski, Lee Jones, Mark Brown,
dmitry.torokhov, Liam Girdwood, cy_huang,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, lkml,
linux-input
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 於 2022年6月7日 週二 下午7:52寫道：
>
> On 07/06/2022 07:52, cy_huang wrote:
> > From: ChiYuan Huang <cy_huang@richtek.com>
> >
> > Add Richtek RT5120 PMIC devicetree document.
> >
> > Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> > ---
> > .../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 <cy_huang@richtek.com>
> > +
> > +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
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 1/4] dt-binding: mfd: Add Richtek RT5120 PMIC support
2022-06-08 2:52 ` ChiYuan Huang
@ 2022-06-08 7:01 ` Krzysztof Kozlowski
2022-06-08 7:25 ` ChiYuan Huang
0 siblings, 1 reply; 21+ messages in thread
From: Krzysztof Kozlowski @ 2022-06-08 7:01 UTC (permalink / raw)
To: ChiYuan Huang
Cc: Rob Herring, Krzysztof Kozlowski, Lee Jones, Mark Brown,
dmitry.torokhov, Liam Girdwood, cy_huang,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, lkml,
linux-input
On 08/06/2022 04:52, ChiYuan Huang wrote:
> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 於 2022年6月7日 週二 下午7:52寫道：
>>
>> On 07/06/2022 07:52, cy_huang wrote:
>>> From: ChiYuan Huang <cy_huang@richtek.com>
>>>
>>> Add Richtek RT5120 PMIC devicetree document.
>>>
>>> Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
>>> ---
>>> .../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 <cy_huang@richtek.com>
>>> +
>>> +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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/4] dt-binding: mfd: Add Richtek RT5120 PMIC support
2022-06-08 7:01 ` Krzysztof Kozlowski
@ 2022-06-08 7:25 ` ChiYuan Huang
2022-06-08 7:44 ` Krzysztof Kozlowski
0 siblings, 1 reply; 21+ messages in thread
From: ChiYuan Huang @ 2022-06-08 7:25 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Rob Herring, Krzysztof Kozlowski, Lee Jones, Mark Brown,
dmitry.torokhov, Liam Girdwood, cy_huang,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, lkml,
linux-input
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 於 2022年6月8日 週三 下午3:02寫道：
>
> On 08/06/2022 04:52, ChiYuan Huang wrote:
> > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 於 2022年6月7日 週二 下午7:52寫道：
> >>
> >> On 07/06/2022 07:52, cy_huang wrote:
> >>> From: ChiYuan Huang <cy_huang@richtek.com>
> >>>
> >>> Add Richtek RT5120 PMIC devicetree document.
> >>>
> >>> Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> >>> ---
> >>> .../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 <cy_huang@richtek.com>
> >>> +
> >>> +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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/4] dt-binding: mfd: Add Richtek RT5120 PMIC support
2022-06-08 7:25 ` ChiYuan Huang
@ 2022-06-08 7:44 ` Krzysztof Kozlowski
0 siblings, 0 replies; 21+ messages in thread
From: Krzysztof Kozlowski @ 2022-06-08 7:44 UTC (permalink / raw)
To: ChiYuan Huang
Cc: Rob Herring, Krzysztof Kozlowski, Lee Jones, Mark Brown,
dmitry.torokhov, Liam Girdwood, cy_huang,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, lkml,
linux-input
On 08/06/2022 09:25, ChiYuan Huang wrote:
> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 於 2022年6月8日 週三 下午3:02寫道：
>>
>> On 08/06/2022 04:52, ChiYuan Huang wrote:
>>> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 於 2022年6月7日 週二 下午7:52寫道：
>>>>
>>>> On 07/06/2022 07:52, cy_huang wrote:
>>>>> From: ChiYuan Huang <cy_huang@richtek.com>
>>>>>
>>>>> Add Richtek RT5120 PMIC devicetree document.
>>>>>
>>>>> Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
>>>>> ---
>>>>> .../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 <cy_huang@richtek.com>
>>>>> +
>>>>> +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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/4] dt-binding: mfd: Add Richtek RT5120 PMIC support
2022-06-07 5:52 ` [PATCH 1/4] dt-binding: mfd: " cy_huang
2022-06-07 11:52 ` Krzysztof Kozlowski
@ 2022-06-07 19:02 ` Mark Brown
2022-06-08 3:04 ` ChiYuan Huang
1 sibling, 1 reply; 21+ messages in thread
From: Mark Brown @ 2022-06-07 19:02 UTC (permalink / raw)
To: cy_huang
Cc: robh+dt, krzysztof.kozlowski+dt, lee.jones, dmitry.torokhov,
lgirdwood, cy_huang, devicetree, linux-kernel, linux-input
[-- Attachment #1: Type: text/plain, Size: 645 bytes --]
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.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/4] dt-binding: mfd: Add Richtek RT5120 PMIC support
2022-06-07 19:02 ` Mark Brown
@ 2022-06-08 3:04 ` ChiYuan Huang
0 siblings, 0 replies; 21+ messages in thread
From: ChiYuan Huang @ 2022-06-08 3:04 UTC (permalink / raw)
To: Mark Brown
Cc: Rob Herring, Krzysztof Kozlowski, Lee Jones, dmitry.torokhov,
Liam Girdwood, cy_huang,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, lkml,
linux-input
Mark Brown <broonie@kernel.org> 於 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.
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 2/4] mfd: rt5120: Add Richtek PMIC support
2022-06-07 5:52 [PATCH 0/4] Add Richtek RT5120 PMIC support cy_huang
2022-06-07 5:52 ` [PATCH 1/4] dt-binding: mfd: " cy_huang
@ 2022-06-07 5:52 ` cy_huang
2022-06-07 5:52 ` [PATCH 3/4] regulator: rt5120: Add PMIC regulator support cy_huang
2022-06-07 5:52 ` [PATCH 4/4] input: misc: rt5120: Add power key support cy_huang
3 siblings, 0 replies; 21+ messages in thread
From: cy_huang @ 2022-06-07 5:52 UTC (permalink / raw)
To: robh+dt, krzysztof.kozlowski+dt, lee.jones, broonie, dmitry.torokhov
Cc: lgirdwood, cy_huang, devicetree, linux-kernel, linux-input
From: ChiYuan Huang <cy_huang@richtek.com>
Add Richtek RT5120 PMIC I2C driver.
Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
---
drivers/mfd/Kconfig | 12 +++++
drivers/mfd/Makefile | 1 +
drivers/mfd/rt5120.c | 125 +++++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 138 insertions(+)
create mode 100644 drivers/mfd/rt5120.c
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 3b59456..866619c 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1127,6 +1127,18 @@ config MFD_RT5033
sub-devices like charger, fuel gauge, flash LED, current source,
LDO and Buck.
+config MFD_RT5120
+ tristate "Richtek RT5120 Power Management IC"
+ depends on I2C
+ select MFD_CORE
+ select REGMAP_I2C
+ select REGMAP_IRQ
+ help
+ The enables support for Richtek RT5120 PMIC. It includes four high
+ efficiency buck converters and one LDO voltage regulator. The device
+ is targeted at providing the CPU voltage, memory, I/O and peripheral
+ power rails in home entertainment devices.
+
config MFD_RC5T583
bool "Ricoh RC5T583 Power Management system device"
depends on I2C=y
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 858cacf..27e8add 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -234,6 +234,7 @@ obj-$(CONFIG_MFD_HI655X_PMIC) += hi655x-pmic.o
obj-$(CONFIG_MFD_DLN2) += dln2.o
obj-$(CONFIG_MFD_RT4831) += rt4831.o
obj-$(CONFIG_MFD_RT5033) += rt5033.o
+obj-$(CONFIG_MFD_RT5120) += rt5120.o
obj-$(CONFIG_MFD_SKY81452) += sky81452.o
intel-soc-pmic-objs := intel_soc_pmic_core.o intel_soc_pmic_crc.o
diff --git a/drivers/mfd/rt5120.c b/drivers/mfd/rt5120.c
new file mode 100644
index 00000000..e7c5f3c
--- /dev/null
+++ b/drivers/mfd/rt5120.c
@@ -0,0 +1,125 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/mfd/core.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/regmap.h>
+
+#define RT5120_REG_INTENABLE 0x1D
+#define RT5120_REG_INTSTAT 0x1E
+#define RT5120_REG_FZCMODE 0x44
+
+#define RT5120_INT_HOTDIE 0
+#define RT5120_INT_PWRKEY_REL 5
+#define RT5120_INT_PWRKEY_PRESS 6
+
+static const struct regmap_range rt5120_rd_yes_ranges[] = {
+ regmap_reg_range(0x03, 0x13),
+ regmap_reg_range(0x1c, 0x20),
+ regmap_reg_range(0x44, 0x44)
+};
+
+static const struct regmap_range rt5120_wr_yes_ranges[] = {
+ regmap_reg_range(0x06, 0x13),
+ regmap_reg_range(0x1c, 0x20),
+ regmap_reg_range(0x44, 0x44)
+};
+
+static const struct regmap_access_table rt5120_rd_table = {
+ .yes_ranges = rt5120_rd_yes_ranges,
+ .n_yes_ranges = ARRAY_SIZE(rt5120_rd_yes_ranges),
+};
+
+static const struct regmap_access_table rt5120_wr_table = {
+ .yes_ranges = rt5120_wr_yes_ranges,
+ .n_yes_ranges = ARRAY_SIZE(rt5120_wr_yes_ranges),
+};
+
+static const struct regmap_config rt5120_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+ .max_register = RT5120_REG_FZCMODE,
+
+ .wr_table = &rt5120_wr_table,
+ .rd_table = &rt5120_rd_table,
+};
+
+static const struct regmap_irq rt5120_irqs[] = {
+ REGMAP_IRQ_REG_LINE(RT5120_INT_HOTDIE, 8),
+ REGMAP_IRQ_REG_LINE(RT5120_INT_PWRKEY_REL, 8),
+ REGMAP_IRQ_REG_LINE(RT5120_INT_PWRKEY_PRESS, 8)
+};
+
+static const struct regmap_irq_chip rt5120_irq_chip = {
+ .name = "rt5120-pmic",
+ .status_base = RT5120_REG_INTSTAT,
+ .mask_base = RT5120_REG_INTENABLE,
+ .ack_base = RT5120_REG_INTSTAT,
+ .mask_invert = true,
+ .use_ack = true,
+ .num_regs = 1,
+ .irqs = rt5120_irqs,
+ .num_irqs = ARRAY_SIZE(rt5120_irqs),
+};
+
+static const struct resource rt5120_regulator_resources[] = {
+ DEFINE_RES_IRQ(RT5120_INT_HOTDIE)
+};
+
+static const struct resource rt5120_pwrkey_resources[] = {
+ DEFINE_RES_IRQ_NAMED(RT5120_INT_PWRKEY_PRESS, "pwrkey-press"),
+ DEFINE_RES_IRQ_NAMED(RT5120_INT_PWRKEY_REL, "pwrkey-release")
+};
+
+static const struct mfd_cell rt5120_devs[] = {
+ MFD_CELL_RES("rt5120-regulator", rt5120_regulator_resources),
+ MFD_CELL_OF("rt5120-pwrkey", rt5120_pwrkey_resources, NULL, 0, 0,
+ "richtek,rt5120-pwrkey")
+};
+
+static int rt5120_probe(struct i2c_client *i2c)
+{
+ struct regmap *regmap;
+ struct regmap_irq_chip_data *irq_data;
+ int ret;
+
+ regmap = devm_regmap_init_i2c(i2c, &rt5120_regmap_config);
+ if (IS_ERR(regmap)) {
+ ret = PTR_ERR(regmap);
+ dev_err(&i2c->dev, "Failed to init regmap (%d)\n", ret);
+ return ret;
+ }
+
+ ret = devm_regmap_add_irq_chip(&i2c->dev, regmap, i2c->irq,
+ IRQF_ONESHOT, 0, &rt5120_irq_chip,
+ &irq_data);
+ if (ret) {
+ dev_err(&i2c->dev, "Failed to add irq chip (%d)\n", ret);
+ return ret;
+ }
+
+ return devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO, rt5120_devs,
+ ARRAY_SIZE(rt5120_devs), NULL, 0,
+ regmap_irq_get_domain(irq_data));
+}
+
+static const struct of_device_id rt5120_device_match_table[] = {
+ { .compatible = "richtek,rt5120", },
+ {}
+};
+MODULE_DEVICE_TABLE(of, rt5120_device_match_table);
+
+static struct i2c_driver rt5120_driver = {
+ .driver = {
+ .name = "rt5120",
+ .of_match_table = rt5120_device_match_table,
+ },
+ .probe_new = rt5120_probe,
+};
+module_i2c_driver(rt5120_driver);
+
+MODULE_AUTHOR("ChiYuan Huang <cy_huang@richtek.com>");
+MODULE_DESCRIPTION("Richtek RT5120 I2C driver");
+MODULE_LICENSE("GPL v2");
--
2.7.4
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH 3/4] regulator: rt5120: Add PMIC regulator support
2022-06-07 5:52 [PATCH 0/4] Add Richtek RT5120 PMIC support cy_huang
2022-06-07 5:52 ` [PATCH 1/4] dt-binding: mfd: " cy_huang
2022-06-07 5:52 ` [PATCH 2/4] mfd: rt5120: Add Richtek " cy_huang
@ 2022-06-07 5:52 ` cy_huang
2022-06-07 19:00 ` Mark Brown
2022-06-07 5:52 ` [PATCH 4/4] input: misc: rt5120: Add power key support cy_huang
3 siblings, 1 reply; 21+ messages in thread
From: cy_huang @ 2022-06-07 5:52 UTC (permalink / raw)
To: robh+dt, krzysztof.kozlowski+dt, lee.jones, broonie, dmitry.torokhov
Cc: lgirdwood, cy_huang, devicetree, linux-kernel, linux-input
From: ChiYuan Huang <cy_huang@richtek.com>
Add RT5120 PMIC regulator support.
It integrates 4 buck convertes, 1 LDO voltage regulator, 1 external
enable signal to control the external power source.
Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
---
drivers/regulator/Kconfig | 10 +
drivers/regulator/Makefile | 1 +
drivers/regulator/rt5120-regulator.c | 417 +++++++++++++++++++++++++++++++++++
3 files changed, 428 insertions(+)
create mode 100644 drivers/regulator/rt5120-regulator.c
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 5ef2306..76039d5 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -1047,6 +1047,16 @@ config REGULATOR_RT5033
RT5033 PMIC. The device supports multiple regulators like
current source, LDO and Buck.
+config REGULATOR_RT5120
+ tristate "Richtek RT5120 PMIC Regulators"
+ depends on MFD_RT5120
+ help
+ This adds support for voltage regulator in Richtek RT5120 PMIC.
+ It integrates 4 channels buck controller, 1 channel LDO, 1 EXTEN
+ to control external power source. Only BUCK1 is adjustable from
+ 600mV to 1395mV, per step 6.250mV. The others are all fixed voltage
+ by external hardware circuit.
+
config REGULATOR_RT5190A
tristate "Richtek RT5190A PMIC"
depends on I2C
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 1b64ad5..32fce42 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -126,6 +126,7 @@ obj-$(CONFIG_REGULATOR_ROHM) += rohm-regulator.o
obj-$(CONFIG_REGULATOR_RT4801) += rt4801-regulator.o
obj-$(CONFIG_REGULATOR_RT4831) += rt4831-regulator.o
obj-$(CONFIG_REGULATOR_RT5033) += rt5033-regulator.o
+obj-$(CONFIG_REGULATOR_RT5120) += rt5120-regulator.o
obj-$(CONFIG_REGULATOR_RT5190A) += rt5190a-regulator.o
obj-$(CONFIG_REGULATOR_RT6160) += rt6160-regulator.o
obj-$(CONFIG_REGULATOR_RT6245) += rt6245-regulator.o
diff --git a/drivers/regulator/rt5120-regulator.c b/drivers/regulator/rt5120-regulator.c
new file mode 100644
index 00000000..c786564
--- /dev/null
+++ b/drivers/regulator/rt5120-regulator.c
@@ -0,0 +1,417 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#include <linux/bits.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+#include <linux/regulator/of_regulator.h>
+
+#define RT5120_REG_PGSTAT 0x03
+#define RT5120_REG_CH1VID 0x06
+#define RT5120_REG_CH1SLPVID 0x07
+#define RT5120_REG_ENABLE 0x08
+#define RT5120_REG_MODECTL 0x09
+#define RT5120_REG_UVOVPROT 0x0A
+#define RT5120_REG_SLPCTL 0x0C
+#define RT5120_REG_INTSTAT 0x1E
+#define RT5120_REG_DISCHG 0x1F
+
+#define RT5120_OUTPG_MASK(rid) BIT(rid + 1)
+#define RT5120_OUTUV_MASK(rid) BIT(rid + 9)
+#define RT5120_OUTOV_MASK(rid) BIT(rid + 16)
+#define RT5120_CH1VID_MASK GENMASK(6, 0)
+#define RT5120_RIDEN_MASK(rid) BIT(rid + 1)
+#define RT5120_RADEN_MASK(rid) BIT(rid)
+#define RT5120_FPWM_MASK(rid) BIT(rid + 1)
+#define RT5120_UVHICCUP_MASK BIT(1)
+#define RT5120_OVHICCUP_MASK BIT(0)
+#define RT5120_HOTDIE_MASK BIT(1)
+
+#define RT5120_BUCK1_MINUV 600000
+#define RT5120_BUCK1_MAXUV 1393750
+#define RT5120_BUCK1_STEPUV 6250
+#define RT5120_BUCK1_NUM_VOLT 0x80
+
+#define RT5120_AUTO_MODE 0
+#define RT5120_FPWM_MODE 1
+
+enum {
+ RT5120_REGULATOR_BUCK1 = 0,
+ RT5120_REGULATOR_BUCK2,
+ RT5120_REGULATOR_BUCK3,
+ RT5120_REGULATOR_BUCK4,
+ RT5120_REGULATOR_LDO,
+ RT5120_REGULATOR_EXTEN,
+ RT5120_MAX_REGULATOR
+};
+
+struct rt5120_priv {
+ struct device *dev;
+ struct regmap *regmap;
+ struct regulator_desc rdesc[RT5120_MAX_REGULATOR];
+};
+
+static int rt5120_buck_set_mode(struct regulator_dev *rdev, unsigned int mode)
+{
+ struct regmap *regmap = rdev_get_regmap(rdev);
+ int rid = rdev_get_id(rdev);
+ unsigned int mask = RT5120_FPWM_MASK(rid), val;
+
+ switch (mode) {
+ case REGULATOR_MODE_NORMAL:
+ val = 0;
+ break;
+ case REGULATOR_MODE_FAST:
+ val = RT5120_FPWM_MASK(rid);
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return regmap_update_bits(regmap, RT5120_REG_MODECTL, mask, val);
+}
+
+static unsigned int rt5120_buck_get_mode(struct regulator_dev *rdev)
+{
+ struct regmap *regmap = rdev_get_regmap(rdev);
+ int ret, rid = rdev_get_id(rdev);
+ unsigned int val;
+
+ ret = regmap_read(regmap, RT5120_REG_MODECTL, &val);
+ if (ret)
+ return REGULATOR_MODE_INVALID;
+
+ if (val & RT5120_FPWM_MASK(rid))
+ return REGULATOR_MODE_FAST;
+
+ return REGULATOR_MODE_NORMAL;
+}
+
+static int rt5120_regulator_get_error_flags(struct regulator_dev *rdev,
+ unsigned int *flags)
+{
+ struct regmap *regmap = rdev_get_regmap(rdev);
+ unsigned int stat, hd_stat, cur_flags = 0;
+ int rid = rdev_get_id(rdev), ret;
+
+ /*
+ * reg 0x03/0x04/0x05 to indicate PG/UV/OV
+ * use block read to descrease I/O xfer time
+ */
+ ret = regmap_raw_read(regmap, RT5120_REG_PGSTAT, &stat, 3);
+ if (ret)
+ return ret;
+
+ ret = regmap_read(regmap, RT5120_REG_INTSTAT, &hd_stat);
+ if (ret)
+ return ret;
+
+ if (!(stat & RT5120_OUTPG_MASK(rid))) {
+ if (stat & RT5120_OUTUV_MASK(rid))
+ cur_flags |= REGULATOR_ERROR_UNDER_VOLTAGE;
+
+ if (stat & RT5120_OUTOV_MASK(rid))
+ cur_flags |= REGULATOR_ERROR_REGULATION_OUT;
+ }
+
+ if (hd_stat & RT5120_HOTDIE_MASK)
+ cur_flags |= REGULATOR_ERROR_OVER_TEMP;
+
+ *flags = cur_flags;
+ return 0;
+}
+
+static int rt5120_buck1_set_suspend_voltage(struct regulator_dev *rdev, int uV)
+{
+ struct regmap *regmap = rdev_get_regmap(rdev);
+ int sel;
+
+ if (uV < RT5120_BUCK1_MINUV || uV > RT5120_BUCK1_MAXUV)
+ return -EINVAL;
+
+ sel = (uV - RT5120_BUCK1_MINUV) / RT5120_BUCK1_STEPUV;
+ return regmap_write(regmap, RT5120_REG_CH1SLPVID, sel);
+}
+
+static int rt5120_regulator_set_suspend_enable(struct regulator_dev *rdev)
+{
+ struct regmap *regmap = rdev_get_regmap(rdev);
+ int rid = rdev_get_id(rdev);
+ unsigned int mask = RT5120_RIDEN_MASK(rid);
+
+ return regmap_update_bits(regmap, RT5120_REG_SLPCTL, mask, mask);
+}
+
+static int rt5120_regulator_set_suspend_disable(struct regulator_dev *rdev)
+{
+ struct regmap *regmap = rdev_get_regmap(rdev);
+ int rid = rdev_get_id(rdev);
+ unsigned int mask = RT5120_RIDEN_MASK(rid);
+
+ return regmap_update_bits(regmap, RT5120_REG_SLPCTL, mask, 0);
+}
+
+static const struct regulator_ops rt5120_buck1_ops = {
+ .enable = regulator_enable_regmap,
+ .disable = regulator_disable_regmap,
+ .is_enabled = regulator_is_enabled_regmap,
+ .list_voltage = regulator_list_voltage_linear,
+ .set_voltage_sel = regulator_set_voltage_sel_regmap,
+ .get_voltage_sel = regulator_get_voltage_sel_regmap,
+ .set_active_discharge = regulator_set_active_discharge_regmap,
+ .set_mode = rt5120_buck_set_mode,
+ .get_mode = rt5120_buck_get_mode,
+ .get_error_flags = rt5120_regulator_get_error_flags,
+ .set_suspend_voltage = rt5120_buck1_set_suspend_voltage,
+ .set_suspend_enable = rt5120_regulator_set_suspend_enable,
+ .set_suspend_disable = rt5120_regulator_set_suspend_disable,
+};
+
+static const struct regulator_ops rt5120_buck234_ops = {
+ .enable = regulator_enable_regmap,
+ .disable = regulator_disable_regmap,
+ .is_enabled = regulator_is_enabled_regmap,
+ .set_active_discharge = regulator_set_active_discharge_regmap,
+ .set_mode = rt5120_buck_set_mode,
+ .get_mode = rt5120_buck_get_mode,
+ .get_error_flags = rt5120_regulator_get_error_flags,
+ .set_suspend_enable = rt5120_regulator_set_suspend_enable,
+ .set_suspend_disable = rt5120_regulator_set_suspend_disable,
+};
+
+static const struct regulator_ops rt5120_ldo_ops = {
+ .enable = regulator_enable_regmap,
+ .disable = regulator_disable_regmap,
+ .is_enabled = regulator_is_enabled_regmap,
+ .set_active_discharge = regulator_set_active_discharge_regmap,
+ .get_error_flags = rt5120_regulator_get_error_flags,
+ .set_suspend_enable = rt5120_regulator_set_suspend_enable,
+ .set_suspend_disable = rt5120_regulator_set_suspend_disable,
+};
+
+static const struct regulator_ops rt5120_exten_ops = {
+ .enable = regulator_enable_regmap,
+ .disable = regulator_disable_regmap,
+ .is_enabled = regulator_is_enabled_regmap,
+ .set_suspend_enable = rt5120_regulator_set_suspend_enable,
+ .set_suspend_disable = rt5120_regulator_set_suspend_disable,
+};
+
+static unsigned int rt5120_buck_of_map_mode(unsigned int mode)
+{
+ switch (mode) {
+ case RT5120_AUTO_MODE:
+ return REGULATOR_MODE_NORMAL;
+ case RT5120_FPWM_MODE:
+ return REGULATOR_MODE_FAST;
+ default:
+ return REGULATOR_MODE_INVALID;
+ }
+}
+
+static void rt5120_fillin_regulator_desc(struct regulator_desc *desc, int rid)
+{
+ static const char * const name[] = { "buck1", "buck2", "buck3", "buck4",
+ "ldo", "exten" };
+ static const char * const sname[] = { "vin1", "vin2", "vin3", "vin4",
+ "vinldo", NULL };
+
+ desc->name = name[rid];
+ desc->supply_name = sname[rid];
+ desc->owner = THIS_MODULE;
+ desc->type = REGULATOR_VOLTAGE;
+ desc->id = rid;
+ desc->enable_reg = RT5120_REG_ENABLE;
+ desc->enable_mask = RT5120_RIDEN_MASK(rid);
+ desc->active_discharge_reg = RT5120_REG_DISCHG;
+ desc->active_discharge_mask = RT5120_RADEN_MASK(rid);
+ desc->active_discharge_on = RT5120_RADEN_MASK(rid);
+
+ switch (rid) {
+ case RT5120_REGULATOR_BUCK1:
+ desc->of_map_mode = rt5120_buck_of_map_mode;
+ desc->min_uV = RT5120_BUCK1_MINUV;
+ desc->uV_step = RT5120_BUCK1_STEPUV;
+ desc->n_voltages = RT5120_BUCK1_NUM_VOLT;
+ desc->vsel_reg = RT5120_REG_CH1VID,
+ desc->vsel_mask = RT5120_CH1VID_MASK,
+ desc->ops = &rt5120_buck1_ops;
+ break;
+ case RT5120_REGULATOR_BUCK2 ... RT5120_REGULATOR_BUCK4:
+ desc->of_map_mode = rt5120_buck_of_map_mode;
+ desc->n_voltages = 1;
+ desc->ops = &rt5120_buck234_ops;
+ break;
+ case RT5120_REGULATOR_LDO:
+ desc->n_voltages = 1;
+ desc->ops = &rt5120_ldo_ops;
+ break;
+ default:
+ desc->n_voltages = 1;
+ desc->ops = &rt5120_exten_ops;
+ break;
+ }
+}
+
+static int rt5120_of_parse_cb(struct rt5120_priv *priv, int rid,
+ struct of_regulator_match *match)
+{
+ struct regulator_desc *desc = priv->rdesc + rid;
+ struct regulator_init_data *init_data = match->init_data;
+
+ if (!init_data || rid == RT5120_REGULATOR_BUCK1)
+ return 0;
+
+ if (init_data->constraints.min_uV != init_data->constraints.max_uV) {
+ dev_err(priv->dev, "Variable voltage for fixed regulator\n");
+ return -EINVAL;
+ }
+
+ desc->fixed_uV = init_data->constraints.min_uV;
+ init_data->constraints.apply_uV = 0;
+ return 0;
+}
+
+static struct of_regulator_match rt5120_regu_match[RT5120_MAX_REGULATOR] = {
+ [RT5120_REGULATOR_BUCK1] = { .name = "buck1", },
+ [RT5120_REGULATOR_BUCK2] = { .name = "buck2", },
+ [RT5120_REGULATOR_BUCK3] = { .name = "buck3", },
+ [RT5120_REGULATOR_BUCK4] = { .name = "buck4", },
+ [RT5120_REGULATOR_LDO] = { .name = "ldo", },
+ [RT5120_REGULATOR_EXTEN] = { .name = "exten", }
+};
+
+static int rt5120_parse_regulator_dt_data(struct rt5120_priv *priv)
+{
+ struct device *dev = priv->dev->parent;
+ struct device_node *reg_node;
+ int i, ret;
+
+ for (i = 0; i < RT5120_MAX_REGULATOR; i++) {
+ rt5120_fillin_regulator_desc(priv->rdesc + i, i);
+
+ rt5120_regu_match[i].desc = priv->rdesc + i;
+ }
+
+ reg_node = of_get_child_by_name(dev->of_node, "regulators");
+ if (!reg_node) {
+ dev_err(priv->dev, "Couldn't find 'regulators' node\n");
+ return -ENODEV;
+ }
+
+ ret = of_regulator_match(priv->dev, reg_node, rt5120_regu_match,
+ ARRAY_SIZE(rt5120_regu_match));
+
+ of_node_put(reg_node);
+
+ if (ret < 0) {
+ dev_err(priv->dev,
+ "Error parsing regulator init data (%d)\n", ret);
+ return ret;
+ }
+
+ for (i = 0; i < RT5120_MAX_REGULATOR; i++) {
+ ret = rt5120_of_parse_cb(priv, i, rt5120_regu_match + i);
+ if (ret) {
+ dev_err(priv->dev, "Failed in [%d] of_passe_cb\n", i);
+ return ret;
+ }
+ }
+
+ return 0;
+}
+
+static int rt5120_device_property_init(struct rt5120_priv *priv)
+{
+ struct device *dev = priv->dev->parent;
+ bool prot_enable;
+ unsigned int prot_enable_val = 0;
+
+ /* Assign UV/OV HW protection behavior */
+ prot_enable = device_property_read_bool(dev,
+ "richtek,enable-undervolt-hiccup");
+ if (prot_enable)
+ prot_enable_val |= RT5120_UVHICCUP_MASK;
+
+ prot_enable = device_property_read_bool(dev,
+ "richtek,enable-overvolt-hiccup");
+ if (prot_enable)
+ prot_enable_val |= RT5120_OVHICCUP_MASK;
+
+ return regmap_update_bits(priv->regmap, RT5120_REG_UVOVPROT,
+ RT5120_UVHICCUP_MASK | RT5120_OVHICCUP_MASK,
+ prot_enable_val);
+}
+
+static int rt5120_regulator_probe(struct platform_device *pdev)
+{
+ struct rt5120_priv *priv;
+ struct regulator_dev *rdev;
+ struct regulator_config config = {};
+ int i, ret;
+
+ priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->dev = &pdev->dev;
+
+ priv->regmap = dev_get_regmap(pdev->dev.parent, NULL);
+ if (!priv->regmap) {
+ dev_err(&pdev->dev, "Failed to init regmap\n");
+ return -ENODEV;
+ }
+
+ ret = rt5120_device_property_init(priv);
+ if (ret) {
+ dev_err(&pdev->dev, "Failed to do property init\n");
+ return ret;
+ }
+
+ ret = rt5120_parse_regulator_dt_data(priv);
+ if (ret) {
+ dev_err(&pdev->dev, "Failed to parse dt data\n");
+ return ret;
+ }
+
+ config.dev = &pdev->dev;
+ config.regmap = priv->regmap;
+
+ for (i = 0; i < RT5120_MAX_REGULATOR; i++) {
+ config.of_node = rt5120_regu_match[i].of_node;
+ config.init_data = rt5120_regu_match[i].init_data;
+
+ rdev = devm_regulator_register(&pdev->dev, priv->rdesc + i,
+ &config);
+ if (IS_ERR(rdev)) {
+ dev_err(&pdev->dev,
+ "Failed to register regulator [%d]\n", i);
+ return PTR_ERR(rdev);
+ }
+ }
+
+ return 0;
+}
+
+static const struct platform_device_id rt5120_regulator_dev_table[] = {
+ { "rt5120-regulator", 0 },
+ {}
+};
+MODULE_DEVICE_TABLE(platform, rt5120_regulator_dev_table);
+
+static struct platform_driver rt5120_regulator_driver = {
+ .driver = {
+ .name = "rt5120-regulator",
+ },
+ .id_table = rt5120_regulator_dev_table,
+ .probe = rt5120_regulator_probe,
+};
+module_platform_driver(rt5120_regulator_driver);
+
+MODULE_AUTHOR("ChiYuan Huang <cy_huang@richtek.com>");
+MODULE_DESCRIPTION("Richtek RT5120 regulator driver");
+MODULE_LICENSE("GPL v2");
--
2.7.4
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 3/4] regulator: rt5120: Add PMIC regulator support
2022-06-07 5:52 ` [PATCH 3/4] regulator: rt5120: Add PMIC regulator support cy_huang
@ 2022-06-07 19:00 ` Mark Brown
2022-06-08 3:15 ` ChiYuan Huang
0 siblings, 1 reply; 21+ messages in thread
From: Mark Brown @ 2022-06-07 19:00 UTC (permalink / raw)
To: cy_huang
Cc: robh+dt, krzysztof.kozlowski+dt, lee.jones, dmitry.torokhov,
lgirdwood, cy_huang, devicetree, linux-kernel, linux-input
[-- Attachment #1: Type: text/plain, Size: 3396 bytes --]
On Tue, Jun 07, 2022 at 01:52:40PM +0800, cy_huang wrote:
This looks mostly good, a few things though:
> +static void rt5120_fillin_regulator_desc(struct regulator_desc *desc, int rid)
> +{
> + static const char * const name[] = { "buck1", "buck2", "buck3", "buck4",
> + "ldo", "exten" };
> + static const char * const sname[] = { "vin1", "vin2", "vin3", "vin4",
> + "vinldo", NULL };
It would be easier and clearer to just make this a static table like
other drivers do, there's no need to generate anything dynamically as
far as I can see.
> +static int rt5120_of_parse_cb(struct rt5120_priv *priv, int rid,
> + struct of_regulator_match *match)
> +{
> + struct regulator_desc *desc = priv->rdesc + rid;
> + struct regulator_init_data *init_data = match->init_data;
> +
> + if (!init_data || rid == RT5120_REGULATOR_BUCK1)
> + return 0;
> +
> + if (init_data->constraints.min_uV != init_data->constraints.max_uV) {
> + dev_err(priv->dev, "Variable voltage for fixed regulator\n");
> + return -EINVAL;
> + }
> +
> + desc->fixed_uV = init_data->constraints.min_uV;
> + init_data->constraints.apply_uV = 0;
Drivers should never override constraints passed in by machine drivers,
if there's validation needed let the core do it. The same probably
applies to providing a voltage range for a fixed regulator though that's
not modifying everything so not such a problem.
> +static int rt5120_parse_regulator_dt_data(struct rt5120_priv *priv)
> +{
> + struct device *dev = priv->dev->parent;
> + struct device_node *reg_node;
> + int i, ret;
> +
> + for (i = 0; i < RT5120_MAX_REGULATOR; i++) {
> + rt5120_fillin_regulator_desc(priv->rdesc + i, i);
> +
> + rt5120_regu_match[i].desc = priv->rdesc + i;
> + }
Like I said above just make the list of regulators static data and loop
through registering them.
> +
> + reg_node = of_get_child_by_name(dev->of_node, "regulators");
> + if (!reg_node) {
> + dev_err(priv->dev, "Couldn't find 'regulators' node\n");
> + return -ENODEV;
> + }
> +
> + ret = of_regulator_match(priv->dev, reg_node, rt5120_regu_match,
> + ARRAY_SIZE(rt5120_regu_match));
> +
> + of_node_put(reg_node);
> +
> + if (ret < 0) {
> + dev_err(priv->dev,
> + "Error parsing regulator init data (%d)\n", ret);
> + return ret;
> + }
> +
> + for (i = 0; i < RT5120_MAX_REGULATOR; i++) {
> + ret = rt5120_of_parse_cb(priv, i, rt5120_regu_match + i);
> + if (ret) {
> + dev_err(priv->dev, "Failed in [%d] of_passe_cb\n", i);
> + return ret;
> + }
> + }
This is all open coding stuff that's in the core - just provde an
of_parse_cb() operation and let the core take care of calling it.
> +static int rt5120_device_property_init(struct rt5120_priv *priv)
> +{
> + struct device *dev = priv->dev->parent;
> + bool prot_enable;
> + unsigned int prot_enable_val = 0;
> +
> + /* Assign UV/OV HW protection behavior */
> + prot_enable = device_property_read_bool(dev,
> + "richtek,enable-undervolt-hiccup");
> + if (prot_enable)
> + prot_enable_val |= RT5120_UVHICCUP_MASK;
Use the DT APIs to parse DT - since ACPI has a very strong idea of how
power management works which is fundamentally incompatible with with the
DT model we should be writing code in a way that minimises the risk that
we'll end up trying to parse DT properties out of ACPI systems and
creating confusion as DT and ACPI software tries to run on the same
system.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 3/4] regulator: rt5120: Add PMIC regulator support
2022-06-07 19:00 ` Mark Brown
@ 2022-06-08 3:15 ` ChiYuan Huang
2022-06-08 10:12 ` Mark Brown
0 siblings, 1 reply; 21+ messages in thread
From: ChiYuan Huang @ 2022-06-08 3:15 UTC (permalink / raw)
To: Mark Brown
Cc: Rob Herring, Krzysztof Kozlowski, Lee Jones, dmitry.torokhov,
Liam Girdwood, cy_huang,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, lkml,
linux-input
Mark Brown <broonie@kernel.org> 於 2022年6月8日 週三 上午3:00寫道：
>
> On Tue, Jun 07, 2022 at 01:52:40PM +0800, cy_huang wrote:
>
> This looks mostly good, a few things though:
>
> > +static void rt5120_fillin_regulator_desc(struct regulator_desc *desc, int rid)
> > +{
> > + static const char * const name[] = { "buck1", "buck2", "buck3", "buck4",
> > + "ldo", "exten" };
> > + static const char * const sname[] = { "vin1", "vin2", "vin3", "vin4",
> > + "vinldo", NULL };
>
> It would be easier and clearer to just make this a static table like
> other drivers do, there's no need to generate anything dynamically as
> far as I can see.
My excuse. let me explain it.
buck1 voltage range from 600mV to 1393.75mV.
buck2~4/ldo/exten is the fixed regulator.
buck3 and buck4 is fixed by the IC efuse default.
buck2 and ldo is fixed by the external resistor chosen.
exten is designed to connected to the external power.
That's why I cannot directly declared it as the static regulator_desc.
>
> > +static int rt5120_of_parse_cb(struct rt5120_priv *priv, int rid,
> > + struct of_regulator_match *match)
> > +{
> > + struct regulator_desc *desc = priv->rdesc + rid;
> > + struct regulator_init_data *init_data = match->init_data;
> > +
> > + if (!init_data || rid == RT5120_REGULATOR_BUCK1)
> > + return 0;
> > +
> > + if (init_data->constraints.min_uV != init_data->constraints.max_uV) {
> > + dev_err(priv->dev, "Variable voltage for fixed regulator\n");
> > + return -EINVAL;
> > + }
> > +
> > + desc->fixed_uV = init_data->constraints.min_uV;
> > + init_data->constraints.apply_uV = 0;
>
> Drivers should never override constraints passed in by machine drivers,
> if there's validation needed let the core do it. The same probably
> applies to providing a voltage range for a fixed regulator though that's
> not modifying everything so not such a problem.
Please check the above explanation about each power rails.
>
> > +static int rt5120_parse_regulator_dt_data(struct rt5120_priv *priv)
> > +{
> > + struct device *dev = priv->dev->parent;
> > + struct device_node *reg_node;
> > + int i, ret;
> > +
> > + for (i = 0; i < RT5120_MAX_REGULATOR; i++) {
> > + rt5120_fillin_regulator_desc(priv->rdesc + i, i);
> > +
> > + rt5120_regu_match[i].desc = priv->rdesc + i;
> > + }
>
> Like I said above just make the list of regulators static data and loop
> through registering them.
Ditto
>
> > +
> > + reg_node = of_get_child_by_name(dev->of_node, "regulators");
> > + if (!reg_node) {
> > + dev_err(priv->dev, "Couldn't find 'regulators' node\n");
> > + return -ENODEV;
> > + }
> > +
> > + ret = of_regulator_match(priv->dev, reg_node, rt5120_regu_match,
> > + ARRAY_SIZE(rt5120_regu_match));
> > +
> > + of_node_put(reg_node);
> > +
> > + if (ret < 0) {
> > + dev_err(priv->dev,
> > + "Error parsing regulator init data (%d)\n", ret);
> > + return ret;
> > + }
> > +
> > + for (i = 0; i < RT5120_MAX_REGULATOR; i++) {
> > + ret = rt5120_of_parse_cb(priv, i, rt5120_regu_match + i);
> > + if (ret) {
> > + dev_err(priv->dev, "Failed in [%d] of_passe_cb\n", i);
> > + return ret;
> > + }
> > + }
>
> This is all open coding stuff that's in the core - just provde an
> of_parse_cb() operation and let the core take care of calling it.
Ditto
>
> > +static int rt5120_device_property_init(struct rt5120_priv *priv)
> > +{
> > + struct device *dev = priv->dev->parent;
> > + bool prot_enable;
> > + unsigned int prot_enable_val = 0;
> > +
> > + /* Assign UV/OV HW protection behavior */
> > + prot_enable = device_property_read_bool(dev,
> > + "richtek,enable-undervolt-hiccup");
> > + if (prot_enable)
> > + prot_enable_val |= RT5120_UVHICCUP_MASK;
>
> Use the DT APIs to parse DT - since ACPI has a very strong idea of how
> power management works which is fundamentally incompatible with with the
> DT model we should be writing code in a way that minimises the risk that
> we'll end up trying to parse DT properties out of ACPI systems and
> creating confusion as DT and ACPI software tries to run on the same
> system.
Ok, I'll replace it by DT API 'of_property_read_bool'.
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 3/4] regulator: rt5120: Add PMIC regulator support
2022-06-08 3:15 ` ChiYuan Huang
@ 2022-06-08 10:12 ` Mark Brown
2022-06-09 6:35 ` ChiYuan Huang
0 siblings, 1 reply; 21+ messages in thread
From: Mark Brown @ 2022-06-08 10:12 UTC (permalink / raw)
To: ChiYuan Huang
Cc: Rob Herring, Krzysztof Kozlowski, Lee Jones, dmitry.torokhov,
Liam Girdwood, cy_huang,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, lkml,
linux-input
[-- Attachment #1: Type: text/plain, Size: 2526 bytes --]
On Wed, Jun 08, 2022 at 11:15:56AM +0800, ChiYuan Huang wrote:
> Mark Brown <broonie@kernel.org> 於 2022年6月8日 週三 上午3:00寫道：
> > On Tue, Jun 07, 2022 at 01:52:40PM +0800, cy_huang wrote:
> > > + static const char * const name[] = { "buck1", "buck2", "buck3", "buck4",
> > > + "ldo", "exten" };
> > > + static const char * const sname[] = { "vin1", "vin2", "vin3", "vin4",
> > > + "vinldo", NULL };
> > It would be easier and clearer to just make this a static table like
> > other drivers do, there's no need to generate anything dynamically as
> > far as I can see.
> My excuse. let me explain it.
> buck1 voltage range from 600mV to 1393.75mV.
> buck2~4/ldo/exten is the fixed regulator.
> buck3 and buck4 is fixed by the IC efuse default.
> buck2 and ldo is fixed by the external resistor chosen.
> exten is designed to connected to the external power.
> That's why I cannot directly declared it as the static regulator_desc.
So buck 2-4 need some dynamic handling then but the rest can be static -
that would be a lot clearer. You could also have a template for the
ones with some dynamic values and just override the few fields that need
it.
> > > + if (init_data->constraints.min_uV != init_data->constraints.max_uV) {
> > > + dev_err(priv->dev, "Variable voltage for fixed regulator\n");
> > > + return -EINVAL;
> > > + }
> > > +
> > > + desc->fixed_uV = init_data->constraints.min_uV;
> > > + init_data->constraints.apply_uV = 0;
> > Drivers should never override constraints passed in by machine drivers,
> > if there's validation needed let the core do it. The same probably
> > applies to providing a voltage range for a fixed regulator though that's
> > not modifying everything so not such a problem.
> Please check the above explanation about each power rails.
I'm not sure what you're referencing here?
> > > + for (i = 0; i < RT5120_MAX_REGULATOR; i++) {
> > > + ret = rt5120_of_parse_cb(priv, i, rt5120_regu_match + i);
> > > + if (ret) {
> > > + dev_err(priv->dev, "Failed in [%d] of_passe_cb\n", i);
> > > + return ret;
> > > + }
> > > + }
> >
> > This is all open coding stuff that's in the core - just provde an
> > of_parse_cb() operation and let the core take care of calling it.
> Ditto
Or here.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 3/4] regulator: rt5120: Add PMIC regulator support
2022-06-08 10:12 ` Mark Brown
@ 2022-06-09 6:35 ` ChiYuan Huang
2022-06-10 11:03 ` Mark Brown
0 siblings, 1 reply; 21+ messages in thread
From: ChiYuan Huang @ 2022-06-09 6:35 UTC (permalink / raw)
To: Mark Brown
Cc: Rob Herring, Krzysztof Kozlowski, Lee Jones, dmitry.torokhov,
Liam Girdwood, cy_huang,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, lkml,
linux-input
Mark Brown <broonie@kernel.org> 於 2022年6月8日 週三 下午6:12寫道：
>
> On Wed, Jun 08, 2022 at 11:15:56AM +0800, ChiYuan Huang wrote:
> > Mark Brown <broonie@kernel.org> 於 2022年6月8日 週三 上午3:00寫道：
> > > On Tue, Jun 07, 2022 at 01:52:40PM +0800, cy_huang wrote:
>
> > > > + static const char * const name[] = { "buck1", "buck2", "buck3", "buck4",
> > > > + "ldo", "exten" };
> > > > + static const char * const sname[] = { "vin1", "vin2", "vin3", "vin4",
> > > > + "vinldo", NULL };
>
> > > It would be easier and clearer to just make this a static table like
> > > other drivers do, there's no need to generate anything dynamically as
> > > far as I can see.
>
> > My excuse. let me explain it.
> > buck1 voltage range from 600mV to 1393.75mV.
> > buck2~4/ldo/exten is the fixed regulator.
> > buck3 and buck4 is fixed by the IC efuse default.
> > buck2 and ldo is fixed by the external resistor chosen.
> > exten is designed to connected to the external power.
>
> > That's why I cannot directly declared it as the static regulator_desc.
>
> So buck 2-4 need some dynamic handling then but the rest can be static -
> that would be a lot clearer. You could also have a template for the
> ones with some dynamic values and just override the few fields that need
> it.
>
Not just buck2/3, buck2/3/4/ldo/exten all need the dynamic handling.
> > > > + if (init_data->constraints.min_uV != init_data->constraints.max_uV) {
> > > > + dev_err(priv->dev, "Variable voltage for fixed regulator\n");
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > + desc->fixed_uV = init_data->constraints.min_uV;
> > > > + init_data->constraints.apply_uV = 0;
>
> > > Drivers should never override constraints passed in by machine drivers,
> > > if there's validation needed let the core do it. The same probably
> > > applies to providing a voltage range for a fixed regulator though that's
> > > not modifying everything so not such a problem.
>
> > Please check the above explanation about each power rails.
>
> I'm not sure what you're referencing here?
>
Sorry. Let me explain it.
You mean 'of_parse_cb' must not override constraint.
But if the regulator is fixed and dynamic, after
'of_get_regulation_constraint', apply_uV will be true.
The is referring to 'fixed.c'
> > > > + for (i = 0; i < RT5120_MAX_REGULATOR; i++) {
> > > > + ret = rt5120_of_parse_cb(priv, i, rt5120_regu_match + i);
> > > > + if (ret) {
> > > > + dev_err(priv->dev, "Failed in [%d] of_passe_cb\n", i);
> > > > + return ret;
> > > > + }
> > > > + }
> > >
> > > This is all open coding stuff that's in the core - just provde an
> > > of_parse_cb() operation and let the core take care of calling it.
>
> > Ditto
>
> Or here.
If I put 'of_parce_cb' to make core handling it, the input parameter
'init_data' is declared as const.
I cannot override the 'apply_uV'.
Right?
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 3/4] regulator: rt5120: Add PMIC regulator support
2022-06-09 6:35 ` ChiYuan Huang
@ 2022-06-10 11:03 ` Mark Brown
2022-06-13 1:49 ` ChiYuan Huang
0 siblings, 1 reply; 21+ messages in thread
From: Mark Brown @ 2022-06-10 11:03 UTC (permalink / raw)
To: ChiYuan Huang
Cc: Rob Herring, Krzysztof Kozlowski, Lee Jones, dmitry.torokhov,
Liam Girdwood, cy_huang,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, lkml,
linux-input
[-- Attachment #1: Type: text/plain, Size: 2325 bytes --]
On Thu, Jun 09, 2022 at 02:35:07PM +0800, ChiYuan Huang wrote:
> Mark Brown <broonie@kernel.org> 於 2022年6月8日 週三 下午6:12寫道：
> > On Wed, Jun 08, 2022 at 11:15:56AM +0800, ChiYuan Huang wrote:
> > > My excuse. let me explain it.
> > > buck1 voltage range from 600mV to 1393.75mV.
> > > buck2~4/ldo/exten is the fixed regulator.
> > > buck3 and buck4 is fixed by the IC efuse default.
> > > buck2 and ldo is fixed by the external resistor chosen.
> > > exten is designed to connected to the external power.
> > > That's why I cannot directly declared it as the static regulator_desc.
> > So buck 2-4 need some dynamic handling then but the rest can be static -
> > that would be a lot clearer. You could also have a template for the
> > ones with some dynamic values and just override the few fields that need
> > it.
> Not just buck2/3, buck2/3/4/ldo/exten all need the dynamic handling.
Why do the others need it?
> > > > Drivers should never override constraints passed in by machine drivers,
> > > > if there's validation needed let the core do it. The same probably
> > > > applies to providing a voltage range for a fixed regulator though that's
> > > > not modifying everything so not such a problem.
> > > Please check the above explanation about each power rails.
> > I'm not sure what you're referencing here?
> Sorry. Let me explain it.
> You mean 'of_parse_cb' must not override constraint.
> But if the regulator is fixed and dynamic, after
> 'of_get_regulation_constraint', apply_uV will be true.
> The is referring to 'fixed.c'
fixed.c is a special case due to legacy issues and being generic, for
normal fixed voltage regulators in a device where we know what they're
fixed to they can just have their voltage hard coded in the driver. If
there's issues with the machine providing invalid or nonsensical
constraints the driver should just let the core deal with them.
> > > > This is all open coding stuff that's in the core - just provde an
> > > > of_parse_cb() operation and let the core take care of calling it.
> > > Ditto
> > Or here.
> If I put 'of_parce_cb' to make core handling it, the input parameter
> 'init_data' is declared as const.
> I cannot override the 'apply_uV'.
> Right?
Yes, that's by design.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/4] regulator: rt5120: Add PMIC regulator support
2022-06-10 11:03 ` Mark Brown
@ 2022-06-13 1:49 ` ChiYuan Huang
2022-06-13 11:10 ` Mark Brown
0 siblings, 1 reply; 21+ messages in thread
From: ChiYuan Huang @ 2022-06-13 1:49 UTC (permalink / raw)
To: Mark Brown
Cc: Rob Herring, Krzysztof Kozlowski, Lee Jones, dmitry.torokhov,
Liam Girdwood, cy_huang,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, lkml,
linux-input
Mark Brown <broonie@kernel.org> 於 2022年6月10日 週五 下午7:03寫道：
>
> On Thu, Jun 09, 2022 at 02:35:07PM +0800, ChiYuan Huang wrote:
> > Mark Brown <broonie@kernel.org> 於 2022年6月8日 週三 下午6:12寫道：
> > > On Wed, Jun 08, 2022 at 11:15:56AM +0800, ChiYuan Huang wrote:
>
> > > > My excuse. let me explain it.
> > > > buck1 voltage range from 600mV to 1393.75mV.
> > > > buck2~4/ldo/exten is the fixed regulator.
> > > > buck3 and buck4 is fixed by the IC efuse default.
> > > > buck2 and ldo is fixed by the external resistor chosen.
> > > > exten is designed to connected to the external power.
>
> > > > That's why I cannot directly declared it as the static regulator_desc.
>
> > > So buck 2-4 need some dynamic handling then but the rest can be static -
> > > that would be a lot clearer. You could also have a template for the
> > > ones with some dynamic values and just override the few fields that need
> > > it.
>
> > Not just buck2/3, buck2/3/4/ldo/exten all need the dynamic handling.
>
> Why do the others need it?
>
Sometimes, for this kind of general purpose PMIC, it need to provide
the flexibility.
Cause buck2 and ldo can already be fixed by the external resistor,
buck3 and buck4 seems to be fixed by IC default.
So there may be the same part number and use the postfix to be
different like as 5120'A'/5120'B', etc...
And use it to define the voltage for the different IC default for
buck3 and buck4, and exten behavior.
That's due to the different application use the different power on
sequence and default voltages.l
> > > > > Drivers should never override constraints passed in by machine drivers,
> > > > > if there's validation needed let the core do it. The same probably
> > > > > applies to providing a voltage range for a fixed regulator though that's
> > > > > not modifying everything so not such a problem.
>
> > > > Please check the above explanation about each power rails.
>
> > > I'm not sure what you're referencing here?
>
> > Sorry. Let me explain it.
>
> > You mean 'of_parse_cb' must not override constraint.
> > But if the regulator is fixed and dynamic, after
> > 'of_get_regulation_constraint', apply_uV will be true.
> > The is referring to 'fixed.c'
>
> fixed.c is a special case due to legacy issues and being generic, for
> normal fixed voltage regulators in a device where we know what they're
> fixed to they can just have their voltage hard coded in the driver. If
> there's issues with the machine providing invalid or nonsensical
> constraints the driver should just let the core deal with them.
>
> > > > > This is all open coding stuff that's in the core - just provde an
> > > > > of_parse_cb() operation and let the core take care of calling it.
>
> > > > Ditto
>
> > > Or here.
>
> > If I put 'of_parce_cb' to make core handling it, the input parameter
> > 'init_data' is declared as const.
> > I cannot override the 'apply_uV'.
> > Right?
>
> Yes, that's by design.
I have traced the code for 'of_get_regulator_init_data' and
'set_machine_constraints' in regulator register.
If I cannot overwrite apply_uV variable, it will cause the
regulator_register return -EINVAL.
Is the below flow that you suggested?
1. of_parse_cb to check min_uV and max_uV, and fill in the fixed_uV in
regulator_desc
2. provide the duummy set/get voltage to make set_machine_constraints
not return '-EINVAL'.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/4] regulator: rt5120: Add PMIC regulator support
2022-06-13 1:49 ` ChiYuan Huang
@ 2022-06-13 11:10 ` Mark Brown
0 siblings, 0 replies; 21+ messages in thread
From: Mark Brown @ 2022-06-13 11:10 UTC (permalink / raw)
To: ChiYuan Huang
Cc: Rob Herring, Krzysztof Kozlowski, Lee Jones, dmitry.torokhov,
Liam Girdwood, cy_huang,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, lkml,
linux-input
[-- Attachment #1: Type: text/plain, Size: 2174 bytes --]
On Mon, Jun 13, 2022 at 09:49:31AM +0800, ChiYuan Huang wrote:
> Mark Brown <broonie@kernel.org> 於 2022年6月10日 週五 下午7:03寫道：
> > > Not just buck2/3, buck2/3/4/ldo/exten all need the dynamic handling.
> > Why do the others need it?
> Sometimes, for this kind of general purpose PMIC, it need to provide
> the flexibility.
> Cause buck2 and ldo can already be fixed by the external resistor,
> buck3 and buck4 seems to be fixed by IC default.
> So there may be the same part number and use the postfix to be
> different like as 5120'A'/5120'B', etc...
> And use it to define the voltage for the different IC default for
> buck3 and buck4, and exten behavior.
> That's due to the different application use the different power on
> sequence and default voltages.l
Variants should have separate compatibles, and if the code is doing
something fixed then it shouldn't make any difference if that fixed
thing is written in code or as a data table.
> > > If I put 'of_parce_cb' to make core handling it, the input parameter
> > > 'init_data' is declared as const.
> > > I cannot override the 'apply_uV'.
> > > Right?
> > Yes, that's by design.
> I have traced the code for 'of_get_regulator_init_data' and
> 'set_machine_constraints' in regulator register.
> If I cannot overwrite apply_uV variable, it will cause the
> regulator_register return -EINVAL.
We have a very large number of fixed voltage regulators in use on
various systems which manage perfectly fine without this. If the
constraints are trying to set something invalid then they're what
should be fixed.
> Is the below flow that you suggested?
> 1. of_parse_cb to check min_uV and max_uV, and fill in the fixed_uV in
> regulator_desc
No, the device should just know what voltage the fixed voltage
regulators in the device have.
> 2. provide the duummy set/get voltage to make set_machine_constraints
> not return '-EINVAL'.
No, people just shouldn't be trying to set the voltage for fixed voltage
regulators. If the regulators are configurable in hardware then provide
DT properties for whatever is configured (eg, resistors).
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 4/4] input: misc: rt5120: Add power key support
2022-06-07 5:52 [PATCH 0/4] Add Richtek RT5120 PMIC support cy_huang
` (2 preceding siblings ...)
2022-06-07 5:52 ` [PATCH 3/4] regulator: rt5120: Add PMIC regulator support cy_huang
@ 2022-06-07 5:52 ` cy_huang
2022-06-08 7:03 ` Krzysztof Kozlowski
3 siblings, 1 reply; 21+ messages in thread
From: cy_huang @ 2022-06-07 5:52 UTC (permalink / raw)
To: robh+dt, krzysztof.kozlowski+dt, lee.jones, broonie, dmitry.torokhov
Cc: lgirdwood, cy_huang, devicetree, linux-kernel, linux-input
From: ChiYuan Huang <cy_huang@richtek.com>
Add RT5120 PMIC power key support.
Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
---
drivers/input/misc/Kconfig | 9 +++
drivers/input/misc/Makefile | 1 +
drivers/input/misc/rt5120-pwrkey.c | 115 +++++++++++++++++++++++++++++++++++++
3 files changed, 125 insertions(+)
create mode 100644 drivers/input/misc/rt5120-pwrkey.c
diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
index dd5227c..9c0d814 100644
--- a/drivers/input/misc/Kconfig
+++ b/drivers/input/misc/Kconfig
@@ -881,6 +881,15 @@ config INPUT_SC27XX_VIBRA
To compile this driver as a module, choose M here. The module will
be called sc27xx_vibra.
+config INPUT_RT5120_PWRKEY
+ tristate "RT5120 PMIC power key support"
+ depends on MFD_RT5120
+ help
+ This enables support for RT5120 PMIC power key driver.
+
+ To compile this driver as a module, choose M here. the module will
+ be called rt5120-pwerkey.
+
config INPUT_STPMIC1_ONKEY
tristate "STPMIC1 PMIC Onkey support"
depends on MFD_STPMIC1
diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
index b92c53a..164ea20 100644
--- a/drivers/input/misc/Makefile
+++ b/drivers/input/misc/Makefile
@@ -68,6 +68,7 @@ obj-$(CONFIG_INPUT_RAVE_SP_PWRBUTTON) += rave-sp-pwrbutton.o
obj-$(CONFIG_INPUT_RB532_BUTTON) += rb532_button.o
obj-$(CONFIG_INPUT_REGULATOR_HAPTIC) += regulator-haptic.o
obj-$(CONFIG_INPUT_RETU_PWRBUTTON) += retu-pwrbutton.o
+obj-$(CONFIG_INPUT_RT5120_PWRKEY) += rt5120-pwrkey.o
obj-$(CONFIG_INPUT_AXP20X_PEK) += axp20x-pek.o
obj-$(CONFIG_INPUT_GPIO_ROTARY_ENCODER) += rotary_encoder.o
obj-$(CONFIG_INPUT_RK805_PWRKEY) += rk805-pwrkey.o
diff --git a/drivers/input/misc/rt5120-pwrkey.c b/drivers/input/misc/rt5120-pwrkey.c
new file mode 100644
index 00000000..42bd2f3
--- /dev/null
+++ b/drivers/input/misc/rt5120-pwrkey.c
@@ -0,0 +1,115 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#include <linux/bits.h>
+#include <linux/input.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#define RT5120_REG_INTSTAT 0x1E
+#define RT5120_PWRKEYSTAT_MASK BIT(7)
+
+struct rt5120_priv {
+ struct regmap *regmap;
+ struct input_dev *input;
+ int press_irq;
+ int release_irq;
+};
+
+static irqreturn_t rt5120_pwrkey_handler(int irq, void *devid)
+{
+ struct rt5120_priv *priv = devid;
+ unsigned int stat;
+ bool is_pressed;
+ int ret;
+
+ ret = regmap_read(priv->regmap, RT5120_REG_INTSTAT, &stat);
+ if (ret)
+ return IRQ_NONE;
+
+ is_pressed = !(stat & RT5120_PWRKEYSTAT_MASK);
+
+ if ((is_pressed && irq == priv->press_irq) ||
+ (!is_pressed && irq == priv->release_irq)) {
+ input_report_key(priv->input, KEY_POWER, is_pressed);
+ input_sync(priv->input);
+ }
+
+ return IRQ_HANDLED;
+}
+
+static int rt5120_pwrkey_probe(struct platform_device *pdev)
+{
+ struct rt5120_priv *priv;
+ int ret;
+
+ priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->regmap = dev_get_regmap(pdev->dev.parent, NULL);
+ if (!priv->regmap) {
+ dev_err(&pdev->dev, "Failed to init regmap\n");
+ return -ENODEV;
+ }
+
+ priv->press_irq = platform_get_irq_byname(pdev, "pwrkey-press");
+ if (priv->press_irq < 0)
+ return priv->press_irq;
+
+ priv->release_irq = platform_get_irq_byname(pdev, "pwrkey-release");
+ if (priv->release_irq < 0)
+ return priv->release_irq;
+
+ /* Make input device be device resource managed */
+ priv->input = devm_input_allocate_device(&pdev->dev);
+ if (!priv->input) {
+ dev_err(&pdev->dev, "Failed to allocate input device\n");
+ return -ENOMEM;
+ }
+
+ priv->input->name = "rt5120_pwrkey";
+ priv->input->phys = "rt5120_pwrkey/input0";
+ priv->input->id.bustype = BUS_I2C;
+ input_set_capability(priv->input, EV_KEY, KEY_POWER);
+
+ ret = input_register_device(priv->input);
+ if (ret) {
+ dev_err(&pdev->dev, "Failed to register input device\n");
+ return ret;
+ }
+
+ ret = devm_request_threaded_irq(&pdev->dev, priv->press_irq, NULL,
+ rt5120_pwrkey_handler, 0,
+ "pwrkey-press", priv);
+ if (ret) {
+ dev_err(&pdev->dev, "Failed to register pwrkey press irq\n");
+ return ret;
+ }
+
+ return devm_request_threaded_irq(&pdev->dev, priv->release_irq, NULL,
+ rt5120_pwrkey_handler, 0,
+ "pwrkey-release", priv);
+}
+
+static const struct of_device_id r5120_pwrkey_match_table[] = {
+ { .compatible = "richtek,rt5120-pwrkey", },
+ {}
+};
+MODULE_DEVICE_TABLE(of, r5120_pwrkey_match_table);
+
+static struct platform_driver rt5120_pwrkey_driver = {
+ .driver = {
+ .name = "rt5120-pwrkey",
+ .of_match_table = r5120_pwrkey_match_table,
+ },
+ .probe = rt5120_pwrkey_probe,
+};
+module_platform_driver(rt5120_pwrkey_driver);
+
+MODULE_AUTHOR("ChiYuan Huang <cy_huang@richtek.com>");
+MODULE_DESCRIPTION("Richtek RT5120 power key driver");
+MODULE_LICENSE("GPL v2");
--
2.7.4
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 4/4] input: misc: rt5120: Add power key support
2022-06-07 5:52 ` [PATCH 4/4] input: misc: rt5120: Add power key support cy_huang
@ 2022-06-08 7:03 ` Krzysztof Kozlowski
2022-06-08 7:05 ` Krzysztof Kozlowski
0 siblings, 1 reply; 21+ messages in thread
From: Krzysztof Kozlowski @ 2022-06-08 7:03 UTC (permalink / raw)
To: cy_huang, robh+dt, krzysztof.kozlowski+dt, lee.jones, broonie,
dmitry.torokhov
Cc: lgirdwood, cy_huang, devicetree, linux-kernel, linux-input
On 07/06/2022 07:52, cy_huang wrote:
> From: ChiYuan Huang <cy_huang@richtek.com>
>
> Add RT5120 PMIC power key support.
>
> Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> ---
> drivers/input/misc/Kconfig | 9 +++
> drivers/input/misc/Makefile | 1 +
> drivers/input/misc/rt5120-pwrkey.c | 115 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 125 insertions(+)
> create mode 100644 drivers/input/misc/rt5120-pwrkey.c
>
> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
> index dd5227c..9c0d814 100644
> --- a/drivers/input/misc/Kconfig
> +++ b/drivers/input/misc/Kconfig
> @@ -881,6 +881,15 @@ config INPUT_SC27XX_VIBRA
> To compile this driver as a module, choose M here. The module will
> be called sc27xx_vibra.
>
> +config INPUT_RT5120_PWRKEY
> + tristate "RT5120 PMIC power key support"
> + depends on MFD_RT5120
> + help
> + This enables support for RT5120 PMIC power key driver.
> +
> + To compile this driver as a module, choose M here. the module will
> + be called rt5120-pwerkey.
> +
> config INPUT_STPMIC1_ONKEY
> tristate "STPMIC1 PMIC Onkey support"
> depends on MFD_STPMIC1
> diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
> index b92c53a..164ea20 100644
> --- a/drivers/input/misc/Makefile
> +++ b/drivers/input/misc/Makefile
> @@ -68,6 +68,7 @@ obj-$(CONFIG_INPUT_RAVE_SP_PWRBUTTON) += rave-sp-pwrbutton.o
> obj-$(CONFIG_INPUT_RB532_BUTTON) += rb532_button.o
> obj-$(CONFIG_INPUT_REGULATOR_HAPTIC) += regulator-haptic.o
> obj-$(CONFIG_INPUT_RETU_PWRBUTTON) += retu-pwrbutton.o
> +obj-$(CONFIG_INPUT_RT5120_PWRKEY) += rt5120-pwrkey.o
> obj-$(CONFIG_INPUT_AXP20X_PEK) += axp20x-pek.o
> obj-$(CONFIG_INPUT_GPIO_ROTARY_ENCODER) += rotary_encoder.o
> obj-$(CONFIG_INPUT_RK805_PWRKEY) += rk805-pwrkey.o
> diff --git a/drivers/input/misc/rt5120-pwrkey.c b/drivers/input/misc/rt5120-pwrkey.c
> new file mode 100644
> index 00000000..42bd2f3
> --- /dev/null
> +++ b/drivers/input/misc/rt5120-pwrkey.c
> @@ -0,0 +1,115 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +#include <linux/bits.h>
> +#include <linux/input.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#define RT5120_REG_INTSTAT 0x1E
> +#define RT5120_PWRKEYSTAT_MASK BIT(7)
> +
> +struct rt5120_priv {
> + struct regmap *regmap;
> + struct input_dev *input;
> + int press_irq;
> + int release_irq;
> +};
> +
> +static irqreturn_t rt5120_pwrkey_handler(int irq, void *devid)
> +{
> + struct rt5120_priv *priv = devid;
> + unsigned int stat;
> + bool is_pressed;
> + int ret;
> +
> + ret = regmap_read(priv->regmap, RT5120_REG_INTSTAT, &stat);
> + if (ret)
> + return IRQ_NONE;
> +
> + is_pressed = !(stat & RT5120_PWRKEYSTAT_MASK);
> +
> + if ((is_pressed && irq == priv->press_irq) ||
> + (!is_pressed && irq == priv->release_irq)) {
> + input_report_key(priv->input, KEY_POWER, is_pressed);
> + input_sync(priv->input);
> + }
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int rt5120_pwrkey_probe(struct platform_device *pdev)
> +{
> + struct rt5120_priv *priv;
> + int ret;
> +
> + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> + if (!priv->regmap) {
> + dev_err(&pdev->dev, "Failed to init regmap\n");
> + return -ENODEV;
> + }
> +
> + priv->press_irq = platform_get_irq_byname(pdev, "pwrkey-press");
> + if (priv->press_irq < 0)
> + return priv->press_irq;
> +
> + priv->release_irq = platform_get_irq_byname(pdev, "pwrkey-release");
> + if (priv->release_irq < 0)
> + return priv->release_irq;
Not described in the bindings. All properties need to be documented.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 4/4] input: misc: rt5120: Add power key support
2022-06-08 7:03 ` Krzysztof Kozlowski
@ 2022-06-08 7:05 ` Krzysztof Kozlowski
0 siblings, 0 replies; 21+ messages in thread
From: Krzysztof Kozlowski @ 2022-06-08 7:05 UTC (permalink / raw)
To: cy_huang, robh+dt, krzysztof.kozlowski+dt, lee.jones, broonie,
dmitry.torokhov
Cc: lgirdwood, cy_huang, devicetree, linux-kernel, linux-input
On 08/06/2022 09:03, Krzysztof Kozlowski wrote:
> On 07/06/2022 07:52, cy_huang wrote:
>> From: ChiYuan Huang <cy_huang@richtek.com>
>>
>> Add RT5120 PMIC power key support.
>>
>> Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
>> ---
>> drivers/input/misc/Kconfig | 9 +++
>> drivers/input/misc/Makefile | 1 +
>> drivers/input/misc/rt5120-pwrkey.c | 115 +++++++++++++++++++++++++++++++++++++
>> 3 files changed, 125 insertions(+)
>> create mode 100644 drivers/input/misc/rt5120-pwrkey.c
>>
>> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
>> index dd5227c..9c0d814 100644
>> --- a/drivers/input/misc/Kconfig
>> +++ b/drivers/input/misc/Kconfig
>> @@ -881,6 +881,15 @@ config INPUT_SC27XX_VIBRA
>> To compile this driver as a module, choose M here. The module will
>> be called sc27xx_vibra.
>>
>> +config INPUT_RT5120_PWRKEY
>> + tristate "RT5120 PMIC power key support"
>> + depends on MFD_RT5120
>> + help
>> + This enables support for RT5120 PMIC power key driver.
>> +
>> + To compile this driver as a module, choose M here. the module will
>> + be called rt5120-pwerkey.
>> +
>> config INPUT_STPMIC1_ONKEY
>> tristate "STPMIC1 PMIC Onkey support"
>> depends on MFD_STPMIC1
>> diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
>> index b92c53a..164ea20 100644
>> --- a/drivers/input/misc/Makefile
>> +++ b/drivers/input/misc/Makefile
>> @@ -68,6 +68,7 @@ obj-$(CONFIG_INPUT_RAVE_SP_PWRBUTTON) += rave-sp-pwrbutton.o
>> obj-$(CONFIG_INPUT_RB532_BUTTON) += rb532_button.o
>> obj-$(CONFIG_INPUT_REGULATOR_HAPTIC) += regulator-haptic.o
>> obj-$(CONFIG_INPUT_RETU_PWRBUTTON) += retu-pwrbutton.o
>> +obj-$(CONFIG_INPUT_RT5120_PWRKEY) += rt5120-pwrkey.o
>> obj-$(CONFIG_INPUT_AXP20X_PEK) += axp20x-pek.o
>> obj-$(CONFIG_INPUT_GPIO_ROTARY_ENCODER) += rotary_encoder.o
>> obj-$(CONFIG_INPUT_RK805_PWRKEY) += rk805-pwrkey.o
>> diff --git a/drivers/input/misc/rt5120-pwrkey.c b/drivers/input/misc/rt5120-pwrkey.c
>> new file mode 100644
>> index 00000000..42bd2f3
>> --- /dev/null
>> +++ b/drivers/input/misc/rt5120-pwrkey.c
>> @@ -0,0 +1,115 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +
>> +#include <linux/bits.h>
>> +#include <linux/input.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +
>> +#define RT5120_REG_INTSTAT 0x1E
>> +#define RT5120_PWRKEYSTAT_MASK BIT(7)
>> +
>> +struct rt5120_priv {
>> + struct regmap *regmap;
>> + struct input_dev *input;
>> + int press_irq;
>> + int release_irq;
>> +};
>> +
>> +static irqreturn_t rt5120_pwrkey_handler(int irq, void *devid)
>> +{
>> + struct rt5120_priv *priv = devid;
>> + unsigned int stat;
>> + bool is_pressed;
>> + int ret;
>> +
>> + ret = regmap_read(priv->regmap, RT5120_REG_INTSTAT, &stat);
>> + if (ret)
>> + return IRQ_NONE;
>> +
>> + is_pressed = !(stat & RT5120_PWRKEYSTAT_MASK);
>> +
>> + if ((is_pressed && irq == priv->press_irq) ||
>> + (!is_pressed && irq == priv->release_irq)) {
>> + input_report_key(priv->input, KEY_POWER, is_pressed);
>> + input_sync(priv->input);
>> + }
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static int rt5120_pwrkey_probe(struct platform_device *pdev)
>> +{
>> + struct rt5120_priv *priv;
>> + int ret;
>> +
>> + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
>> + if (!priv)
>> + return -ENOMEM;
>> +
>> + priv->regmap = dev_get_regmap(pdev->dev.parent, NULL);
>> + if (!priv->regmap) {
>> + dev_err(&pdev->dev, "Failed to init regmap\n");
>> + return -ENODEV;
>> + }
>> +
>> + priv->press_irq = platform_get_irq_byname(pdev, "pwrkey-press");
>> + if (priv->press_irq < 0)
>> + return priv->press_irq;
>> +
>> + priv->release_irq = platform_get_irq_byname(pdev, "pwrkey-release");
>> + if (priv->release_irq < 0)
>> + return priv->release_irq;
>
> Not described in the bindings. All properties need to be documented.
Hm, it seems it is coming from the parent device, dynamically, so looks ok.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 21+ messages in thread