Re: [PATCH 1/4] dt-bindings: mfd: nxp,bbnsm: Add binding for nxp bbnsm

On 21/11/2022 10:33:15+0000, Jacky Bai wrote:

quoted

Subject: Re: [PATCH 1/4] dt-bindings: mfd: nxp,bbnsm: Add binding for nxp
bbnsm

On 21/11/2022 10:09:40+0100, Krzysztof Kozlowski wrote:

quoted

On 21/11/2022 07:51, Jacky Bai wrote:

quoted

Add binding for NXP BBNSM(Battery-Backed Non-Secure Module).

Signed-off-by: Jacky Bai <ping.bai@nxp.com>
---
 .../devicetree/bindings/mfd/nxp,bbnsm.yaml    | 103

++++++++++++++++++

quoted

quoted

 1 file changed, 103 insertions(+)
 create mode 100644
Documentation/devicetree/bindings/mfd/nxp,bbnsm.yaml

...

quoted

quoted

quoted

+
+title: NXP Battery-Backed Non-Secure Module bindings
+
+maintainers:
+  - Jacky Bai <ping.bai@nxp.com>
+
+description: |
+  NXP BBNSM serves as non-volatile logic and storage for the system.
+  it Intergrates RTC & ON/OFF control.
+  The RTC can retain its state and continues counting even when the
+  main chip is power down. A time alarm is generated once the most
+  significant 32 bits of the real-time counter match the value in
+the
+  Time Alarm register.
+  The ON/OFF logic inside the BBNSM allows for connecting directly
+to
+  a PMIC or other voltage regulator device. both smart PMIC mode
+and
+  Dumb PMIC mode supported.
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - nxp,bbnsm
+      - const: syscon
+      - const: simple-mfd
+
+  reg:
+    maxItems: 1
+
+  rtc:
+    type: object
+
+    properties:
+      compatible:
+        const: nxp,bbnsm-rtc

Missing ref to rtc.yaml.

This is also missing start-year

The RTC counter will be reset to 0 after PoR reset, do we still need to add
this property?

Is this really an RTC then?

BR

quoted

quoted

quoted

+
+      regmap:

Use vendor prefix, descriptive name and description. Where is the type
of 'regmap' defined?

quoted

+        maxItems: 1

I don't think this is correct. Rob explained the simple-mfd means
children do not depend on anything from the parent, but taking a
regmap from its parent contradicts it.

quoted

+
+      interrupts:
+        maxItems: 1

You have same interrupt and same address space used by two devices.

Both arguments (depending on parent regmap, sharing interrupt)
suggests that this is one device block, so describing it with
simple-mfd is quite unflexible.

quoted

+
+    required:
+      - compatible
+      - regmap
+      - interrupts
+
+    additionalProperties: false
+
+  pwrkey:
+    type: object
+    $ref: /schemas/input/input.yaml#
+
+    properties:
+      compatible:
+        const: nxp,bbnsm-pwrkey
+
+      regmap:
+        maxItems: 1
+
+      interrupts:
+        maxItems: 1
+
+      linux,code: true
+
+    required:
+      - compatible
+      - regmap
+      - interrupts
+
+    additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - rtc
+  - pwrkey
+
+additionalProperties: false
+
+examples:
+  - |
+    bbnsm: bbnsm@44440000 {
+      compatible = "nxp,bbnsm", "syscon", "simple-mfd";
+      reg = <0x44440000 0x10000>;
+
+      bbnsm_rtc: rtc {
+        compatible = "nxp,bbnsm-rtc";

Use 4 spaces for example indentation.

quoted

+        regmap = <&bbnsm>;
+        interrupts = <GIC_SPI 73 IRQ_TYPE_LEVEL_HIGH>;
+      };
+
+      bbnsm_pwrkey: pwrkey {
+         compatible = "nxp,bbnsm-pwrkey";
+         regmap = <&bbnsm>;
+         interrupts = <GIC_SPI 73 IRQ_TYPE_LEVEL_HIGH>;
+         linux,code = <KEY_POWER>;
+       };
+    };

Best regards,
Krzysztof

--
Alexandre Belloni, co-owner and COO, Bootlin Embedded Linux and Kernel
engineering
https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbootlin
.com%2F&data=05%7C01%7Cping.bai%40nxp.com%7Cd188bbb7b6ec40
5c481f08dacba2af8b%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%
7C638046196834682924%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjA
wMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7
C%7C&sdata=L%2FTZNaG01NTrKvbKwz9%2FNFEFQ6JqdnsOIzUydww1D
ZU%3D&reserved=0

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com