[RFC,v2,1/6] dt-bindings: mfd: add entry for Marvell 88PM886 PMIC
Commit Message
From: Karel Balej <balejk@matfyz.cz>
Marvell 88PM886 is a PMIC with several subdevices such as onkey,
regulators or battery and charger. It comes in at least two revisions,
A0 and A1 -- only A1 is described here at the moment.
Signed-off-by: Karel Balej <balejk@matfyz.cz>
---
Notes:
RFC v2:
- Address Rob's feedback:
- Drop mention of 88PM880.
- Make sure the file passes bindings check (add the necessary header
and fix `interrupt-cells`).
- Other small changes.
- Add regulators. Changes with respect to the regulator RFC series:
- Address Krzysztof's comments:
- Drop unused compatible.
- Fix sub-node pattern.
.../bindings/mfd/marvell,88pm88x.yaml | 74 +++++++++++++++++++
.../regulator/marvell,88pm88x-regulator.yaml | 28 +++++++
2 files changed, 102 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mfd/marvell,88pm88x.yaml
create mode 100644 Documentation/devicetree/bindings/regulator/marvell,88pm88x-regulator.yaml
Comments
On Sun, Feb 11, 2024 at 10:35:51AM +0100, Karel Balej wrote:
> From: Karel Balej <balejk@matfyz.cz>
>
> Marvell 88PM886 is a PMIC with several subdevices such as onkey,
> regulators or battery and charger. It comes in at least two revisions,
> A0 and A1 -- only A1 is described here at the moment.
>
> Signed-off-by: Karel Balej <balejk@matfyz.cz>
> ---
>
> Notes:
> RFC v2:
> - Address Rob's feedback:
> - Drop mention of 88PM880.
> - Make sure the file passes bindings check (add the necessary header
> and fix `interrupt-cells`).
> - Other small changes.
> - Add regulators. Changes with respect to the regulator RFC series:
> - Address Krzysztof's comments:
> - Drop unused compatible.
> - Fix sub-node pattern.
>
> .../bindings/mfd/marvell,88pm88x.yaml | 74 +++++++++++++++++++
Filename should match the compatible.
In general, drop the 'x' wildcard.
> .../regulator/marvell,88pm88x-regulator.yaml | 28 +++++++
> 2 files changed, 102 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mfd/marvell,88pm88x.yaml
> create mode 100644 Documentation/devicetree/bindings/regulator/marvell,88pm88x-regulator.yaml
>
> diff --git a/Documentation/devicetree/bindings/mfd/marvell,88pm88x.yaml b/Documentation/devicetree/bindings/mfd/marvell,88pm88x.yaml
> new file mode 100644
> index 000000000000..29ab979862d5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/marvell,88pm88x.yaml
> @@ -0,0 +1,74 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mfd/marvell,88pm88x.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Marvell 88PM88X PMIC core
> +
> +maintainers:
> + - Karel Balej <balejk@matfyz.cz>
> +
> +description:
> + Marvell 88PM886 is a PMIC providing several functions such as onkey,
> + regulators or battery and charger.
> +
> +properties:
> + compatible:
> + const: marvell,88pm886-a1
> +
> + reg:
> + maxItems: 1
> +
> + interrupt-controller: true
What is the device providing interrupts to (in DT)?
> +
> + interrupts:
> + maxItems: 1
> +
> + "#interrupt-cells":
> + const: 1
> +
> + regulators:
> + $ref: /schemas/regulator/marvell,88pm88x-regulator.yaml#
That's simple enough, I'd just move the regulator nodes into this doc.
> +
> +required:
> + - compatible
> + - reg
> + - interrupt-controller
> + - interrupts
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/interrupt-controller/irq.h>
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + pmic@30 {
> + compatible = "marvell,88pm886-a1";
> + reg = <0x30>;
> + interrupts = <0 4 IRQ_TYPE_LEVEL_HIGH>;
> + interrupt-parent = <&gic>;
> + interrupt-controller;
> + #interrupt-cells = <1>;
> +
> + regulators {
> + ldo2: ldo2 {
> + regulator-min-microvolt = <3100000>;
> + regulator-max-microvolt = <3300000>;
> + };
> +
> + ldo15: ldo15 {
> + regulator-min-microvolt = <3300000>;
> + regulator-max-microvolt = <3300000>;
> + };
> +
> + buck2: buck2 {
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <1800000>;
> + };
> + };
> + };
> + };
> +...
> diff --git a/Documentation/devicetree/bindings/regulator/marvell,88pm88x-regulator.yaml b/Documentation/devicetree/bindings/regulator/marvell,88pm88x-regulator.yaml
> new file mode 100644
> index 000000000000..1b4b5f1b4932
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/marvell,88pm88x-regulator.yaml
> @@ -0,0 +1,28 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/regulator/marvell,88pm88x-regulator.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Regulators of Marvell 88PM88X PMICs.
> +
> +maintainers:
> + - Karel Balej <balejk@matfyz.cz>
> +
> +description: |
> + This is a part of device tree bindings for Marvell 88PM88X MFD.
> +
> + The regulators node is represented as a sub-node of the PMIC node on the
> + device tree.
> +
> + See also Documentation/devicetree/bindings/mfd/marvell,88pm88x.yaml for
> + additional information and example.
> +
> +patternProperties:
> + "^(ldo(1[0-6]|[1-9])|buck[1-5])$":
> + type: object
> + $ref: /schemas/regulator/regulator.yaml#
> + description: LDO or buck regulator.
> + unevaluatedProperties: false
> +
> +additionalProperties: false
> --
> 2.43.0
>
Rob Herring, 2024-02-15T08:20:52-06:00:
> > .../bindings/mfd/marvell,88pm88x.yaml | 74 +++++++++++++++++++
>
> Filename should match the compatible.
>
> In general, drop the 'x' wildcard.
By "in general", do you mean for the drivers code also?
As I have mentioned in the commit message for the driver, the other
device is very similar and if the support for it was ever to be added
(which I personally currently have no interest in), I believe it would
make sense to extend this driver. Is it then still prefered to call it
all just 88pm886 now?
> > +properties:
> > + compatible:
> > + const: marvell,88pm886-a1
So the file should be called marvell,88pm886-a1.yaml, correct? Again, is
it prefered to call it like this even if the other revision could
eventually be added (again, I am not interested in that right now
personally)? I mean, if I was implementing support for both revisions
right now, it would make sense to name it just marvell,88pm886.yaml, no?
Thank you, kind regards,
K. B.
On 18/02/2024 16:10, Karel Balej wrote:
> Rob Herring, 2024-02-15T08:20:52-06:00:
>>> .../bindings/mfd/marvell,88pm88x.yaml | 74 +++++++++++++++++++
>>
>> Filename should match the compatible.
>>
>> In general, drop the 'x' wildcard.
>
> By "in general", do you mean for the drivers code also?
No, not driver. The rules for wildcard, that they are discouraged, are
DT binding rules.
>
> As I have mentioned in the commit message for the driver, the other
> device is very similar and if the support for it was ever to be added
> (which I personally currently have no interest in), I believe it would
> make sense to extend this driver. Is it then still prefered to call it
> all just 88pm886 now?
Extend the driver, it's unrelated. Binding still should be named like
compatible, because that extension might never happen.
>
>>> +properties:
>>> + compatible:
>>> + const: marvell,88pm886-a1
>
> So the file should be called marvell,88pm886-a1.yaml, correct? Again, is
> it prefered to call it like this even if the other revision could
> eventually be added (again, I am not interested in that right now
If you already add two devices, flexible name would be fine. But you do
not add it now and you might never add, so keep the filename=compatible.
It is fine if it has also other compatibles later. We already accepted
many bindings like that.
Best regards,
Krzysztof
new file mode 100644
@@ -0,0 +1,74 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/marvell,88pm88x.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Marvell 88PM88X PMIC core
+
+maintainers:
+ - Karel Balej <balejk@matfyz.cz>
+
+description:
+ Marvell 88PM886 is a PMIC providing several functions such as onkey,
+ regulators or battery and charger.
+
+properties:
+ compatible:
+ const: marvell,88pm886-a1
+
+ reg:
+ maxItems: 1
+
+ interrupt-controller: true
+
+ interrupts:
+ maxItems: 1
+
+ "#interrupt-cells":
+ const: 1
+
+ regulators:
+ $ref: /schemas/regulator/marvell,88pm88x-regulator.yaml#
+
+required:
+ - compatible
+ - reg
+ - interrupt-controller
+ - interrupts
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ pmic@30 {
+ compatible = "marvell,88pm886-a1";
+ reg = <0x30>;
+ interrupts = <0 4 IRQ_TYPE_LEVEL_HIGH>;
+ interrupt-parent = <&gic>;
+ interrupt-controller;
+ #interrupt-cells = <1>;
+
+ regulators {
+ ldo2: ldo2 {
+ regulator-min-microvolt = <3100000>;
+ regulator-max-microvolt = <3300000>;
+ };
+
+ ldo15: ldo15 {
+ regulator-min-microvolt = <3300000>;
+ regulator-max-microvolt = <3300000>;
+ };
+
+ buck2: buck2 {
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ };
+ };
+ };
+ };
+...
new file mode 100644
@@ -0,0 +1,28 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/regulator/marvell,88pm88x-regulator.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Regulators of Marvell 88PM88X PMICs.
+
+maintainers:
+ - Karel Balej <balejk@matfyz.cz>
+
+description: |
+ This is a part of device tree bindings for Marvell 88PM88X MFD.
+
+ The regulators node is represented as a sub-node of the PMIC node on the
+ device tree.
+
+ See also Documentation/devicetree/bindings/mfd/marvell,88pm88x.yaml for
+ additional information and example.
+
+patternProperties:
+ "^(ldo(1[0-6]|[1-9])|buck[1-5])$":
+ type: object
+ $ref: /schemas/regulator/regulator.yaml#
+ description: LDO or buck regulator.
+ unevaluatedProperties: false
+
+additionalProperties: false