[PATCH 0/4] Add nxp bbnsm module support

[PATCH 0/4] Add nxp bbnsm module support

[Jacky Bai]

NXP BBNSM (Battery-Backed Non-Secure Module) serves as non-volatile
logic and storage for the system. it provides some similar functions
like RTC and ON/OFF support as previous SNVS module found on legacy
i.MX SoCs. The BBNSM is replacement of previous SNVS module, and more
likely it will be used on all the future i.MX SoC or other SoCs from
NXP.

This patchset add the basic support for BBNSM that found on i.MX93.

Jacky Bai (4):
  dt-bindings: mfd: nxp,bbnsm: Add binding for nxp bbnsm
  input: bbnsm_pwrkey: Add bbnsm power key support
  rtc: bbnsm: Add the bbnsm rtc support
  arm64: dts: imx93: Add the bbnsm dts node

 .../devicetree/bindings/mfd/nxp,bbnsm.yaml    | 103 ++++++++
 arch/arm64/boot/dts/freescale/imx93.dtsi      |  18 ++
 drivers/input/keyboard/Kconfig                |  11 +
 drivers/input/keyboard/Makefile               |   1 +
 drivers/input/keyboard/bbnsm_pwrkey.c         | 196 +++++++++++++++
 drivers/rtc/Kconfig                           |  12 +
 drivers/rtc/Makefile                          |   1 +
 drivers/rtc/rtc-bbnsm.c                       | 223 ++++++++++++++++++
 8 files changed, 565 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/nxp,bbnsm.yaml
 create mode 100644 drivers/input/keyboard/bbnsm_pwrkey.c
 create mode 100644 drivers/rtc/rtc-bbnsm.c

-- 
2.37.1

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

[Jacky Bai]

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 ++++++++++++++++++
 1 file changed, 103 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/nxp,bbnsm.yaml

diff --git a/Documentation/devicetree/bindings/mfd/nxp,bbnsm.yaml b/Documentation/devicetree/bindings/mfd/nxp,bbnsm.yaml
new file mode 100644
index 000000000000..b3f22b0daea6
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/nxp,bbnsm.yaml
@@ -0,0 +1,103 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/nxp,bbnsm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+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
+
+      regmap:
+        maxItems: 1
+
+      interrupts:
+        maxItems: 1
+
+    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";
+        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>;
+       };
+    };
-- 
2.37.1

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

[Krzysztof Kozlowski]

On 21/11/2022 07:51, Jacky Bai wrote:
> 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 ++++++++++++++++++
>  1 file changed, 103 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/nxp,bbnsm.yaml
> 
> diff --git a/Documentation/devicetree/bindings/mfd/nxp,bbnsm.yaml b/Documentation/devicetree/bindings/mfd/nxp,bbnsm.yaml
> new file mode 100644
> index 000000000000..b3f22b0daea6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/nxp,bbnsm.yaml
> @@ -0,0 +1,103 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mfd/nxp,bbnsm.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +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.

> +
> +      regmap:

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

> +        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.

> +
> +      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.

> +
> +    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.

> +        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

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

[Alexandre Belloni]

On 21/11/2022 10:09:40+0100, Krzysztof Kozlowski wrote:
> On 21/11/2022 07:51, Jacky Bai wrote:
> > 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 ++++++++++++++++++
> >  1 file changed, 103 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/mfd/nxp,bbnsm.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/mfd/nxp,bbnsm.yaml b/Documentation/devicetree/bindings/mfd/nxp,bbnsm.yaml
> > new file mode 100644
> > index 000000000000..b3f22b0daea6
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mfd/nxp,bbnsm.yaml
> > @@ -0,0 +1,103 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/mfd/nxp,bbnsm.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +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

> > +
> > +      regmap:
> 
> Use vendor prefix, descriptive name and description. Where is the type
> of 'regmap' defined?
> 
> > +        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.
> 
> > +
> > +      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.
> 
> > +
> > +    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.
> 
> > +        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://bootlin.com

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

[Jacky Bai]

[-- Attachment #1: Type: text/plain, Size: 4512 bytes --]

> 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:
> > On 21/11/2022 07:51, Jacky Bai wrote:
> > > 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
> ++++++++++++++++++
> > >  1 file changed, 103 insertions(+)
> > >  create mode 100644
> > > Documentation/devicetree/bindings/mfd/nxp,bbnsm.yaml

...

> > > +
> > > +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?

BR
> 
> > > +
> > > +      regmap:
> >
> > Use vendor prefix, descriptive name and description. Where is the type
> > of 'regmap' defined?
> >
> > > +        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.
> >
> > > +
> > > +      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.
> >
> > > +
> > > +    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.
> >
> > > +        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&amp;data=05%7C01%7Cping.bai%40nxp.com%7Cd188bbb7b6ec40
> 5c481f08dacba2af8b%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%
> 7C638046196834682924%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjA
> wMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7
> C%7C&amp;sdata=L%2FTZNaG01NTrKvbKwz9%2FNFEFQ6JqdnsOIzUydww1D
> ZU%3D&amp;reserved=0

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 9646 bytes --]

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

[Alexandre Belloni]

On 21/11/2022 10:33:15+0000, Jacky Bai wrote:
> > 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:
> > > On 21/11/2022 07:51, Jacky Bai wrote:
> > > > 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
> > ++++++++++++++++++
> > > >  1 file changed, 103 insertions(+)
> > > >  create mode 100644
> > > > Documentation/devicetree/bindings/mfd/nxp,bbnsm.yaml
> 
> ...
> 
> > > > +
> > > > +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
> > 
> > > > +
> > > > +      regmap:
> > >
> > > Use vendor prefix, descriptive name and description. Where is the type
> > > of 'regmap' defined?
> > >
> > > > +        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.
> > >
> > > > +
> > > > +      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.
> > >
> > > > +
> > > > +    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.
> > >
> > > > +        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&amp;data=05%7C01%7Cping.bai%40nxp.com%7Cd188bbb7b6ec40
> > 5c481f08dacba2af8b%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%
> > 7C638046196834682924%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjA
> > wMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7
> > C%7C&amp;sdata=L%2FTZNaG01NTrKvbKwz9%2FNFEFQ6JqdnsOIzUydww1D
> > ZU%3D&amp;reserved=0



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

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

[Jacky Bai]

> Subject: 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:
> > > 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:
> > > > On 21/11/2022 07:51, Jacky Bai wrote:
> > > > > 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
> > > ++++++++++++++++++
> > > > >  1 file changed, 103 insertions(+)  create mode 100644
> > > > > Documentation/devicetree/bindings/mfd/nxp,bbnsm.yaml
> >
> > ...
> >
> > > > > +
> > > > > +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?

Sorry, I think I misunderstand your previous comment. The 'start-year' is used to expand the rtc range,
I will add this property in V2. Thx.

BR
> 
> > BR
> > >
> > > > > +
> > > > > +      regmap:
> > > >
> > > > Use vendor prefix, descriptive name and description. Where is the
> > > > type of 'regmap' defined?
> > > >
> > > > > +        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.
> > > >
> > > > > +
> > > > > +      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.
> > > >
> > > > > +
> > > > > +    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.
> > > >
> > > > > +        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
> > > >
> > >
> > > --

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

[Alexandre Belloni]

On 21/11/2022 13:45:27+0000, Jacky Bai wrote:
> > > > > 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?
> 
> Sorry, I think I misunderstand your previous comment. The 'start-year' is used to expand the rtc range,
> I will add this property in V2. Thx.
> 

I think my question is still valid, if the counter is reset to 0 on POR,
what is the use case?


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

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

[Jacky Bai]

Hi Alexandre,

> Subject: Re: [PATCH 1/4] dt-bindings: mfd: nxp,bbnsm: Add binding for nxp
> bbnsm
> 
> On 21/11/2022 13:45:27+0000, Jacky Bai wrote:
> > > > > > 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?
> >
> > Sorry, I think I misunderstand your previous comment. The 'start-year'
> > is used to expand the rtc range, I will add this property in V2. Thx.
> >
> 
> I think my question is still valid, if the counter is reset to 0 on POR, what is the
> use case?
> 

The RTC can keep running without losing the state if the power rail to it is not cut off.
It has a battery backed supply even if the main power rail is off when device shutdown.
I thought the 'start-year' is to indicate the initial value of the RTC count after first power up.

BR
> 
> --

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

[Jacky Bai]

[-- Attachment #1: Type: text/plain, Size: 3196 bytes --]

> Subject: Re: [PATCH 1/4] dt-bindings: mfd: nxp,bbnsm: Add binding for nxp
> bbnsm
> 
> On 21/11/2022 07:51, Jacky Bai wrote:
> > Add binding for NXP BBNSM(Battery-Backed Non-Secure Module).
> >
> > Signed-off-by: Jacky Bai <ping.bai@nxp.com>
> > ---

...

> > +
> > +    properties:
> > +      compatible:
> > +        const: nxp,bbnsm-rtc
> 
> 
> Missing ref to rtc.yaml.

Ok will include in v2.
> 
> > +
> > +      regmap:
> 
> Use vendor prefix, descriptive name and description. Where is the type of
> 'regmap' defined?

Type is missed. Will add a description and type define if necessary.

> 
> > +        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.

For this BBNSM module, basically, it provides two sperate & different
function: RTC and ON/OFF button control. But
no separate register offset range for each of these functions. For example,
the interrupt enable control,
Interrupt status and basic function control are mixed in the same registers'
different bit.

Any good suggestion on how to handle such case? ^_^

> 
> > +
> > +      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.
> 

It is depends on how SoC integrates this BBNSM module. From the BBNSM side,
it has separate IRQ lines for RTC function and ON/OFF function. Different
IRQ lines
can be used for RTC and ON/OFF button. But in current i.MX93 SoC, those
interrupts
are ORed together at SoC level. That's why same interrupt in the example.

> > +
> > +    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.
> 

Ok, will fix it.

Thx

BR

> > +        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


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 9646 bytes --]

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

[Lee Jones]

My email client takes around 20s to open your emails.

Please fix that.

    [-- Begin signature information --]
    Problem signature from: CN=nxa19010,OU=Developers,OU=Managed Users,OU=CN,OU=NXP,DC=wbi,DC=nxp,DC=com
                   aka: <ping.bai@nxp.com>
               created: Mon 21 Nov 2022 10:26:32 GMT
               expires: Mon 08 Apr 2024 10:15:04 BST
    [-- End signature information --]

-- 
Lee Jones [李琼斯]

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

[Krzysztof Kozlowski]

On 21/11/2022 11:26, Jacky Bai wrote:
>> Subject: Re: [PATCH 1/4] dt-bindings: mfd: nxp,bbnsm: Add binding for nxp
>> bbnsm
>>
>> On 21/11/2022 07:51, Jacky Bai wrote:
>>> Add binding for NXP BBNSM(Battery-Backed Non-Secure Module).
>>>
>>> Signed-off-by: Jacky Bai <ping.bai@nxp.com>
>>> ---
> 
> ...
> 
>>> +
>>> +    properties:
>>> +      compatible:
>>> +        const: nxp,bbnsm-rtc
>>
>>
>> Missing ref to rtc.yaml.
> 
> Ok will include in v2.
>>
>>> +
>>> +      regmap:
>>
>> Use vendor prefix, descriptive name and description. Where is the type of
>> 'regmap' defined?
> 
> Type is missed. Will add a description and type define if necessary.
> 
>>
>>> +        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.
> 
> For this BBNSM module, basically, it provides two sperate & different
> function: RTC and ON/OFF button control. But
> no separate register offset range for each of these functions. For example,
> the interrupt enable control,
> Interrupt status and basic function control are mixed in the same registers'
> different bit.
> 
> Any good suggestion on how to handle such case? ^_^

The solution for more complex common parts, dedicated device driver (MFD
driver) with its own binding. However I understand why it would be
overshoot here.

> 
>>
>>> +
>>> +      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.
>>
> 
> It is depends on how SoC integrates this BBNSM module. From the BBNSM side,
> it has separate IRQ lines for RTC function and ON/OFF function. Different
> IRQ lines
> can be used for RTC and ON/OFF button. But in current i.MX93 SoC, those
> interrupts
> are ORed together at SoC level. That's why same interrupt in the example.

It's fine then.

Best regards,
Krzysztof

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

[Jacky Bai]

> Subject: Re: [PATCH 1/4] dt-bindings: mfd: nxp,bbnsm: Add binding for nxp
> bbnsm
> 
> On 21/11/2022 11:26, Jacky Bai wrote:
> >> Subject: Re: [PATCH 1/4] dt-bindings: mfd: nxp,bbnsm: Add binding for
> >> nxp bbnsm
> >>
> >> On 21/11/2022 07:51, Jacky Bai wrote:
> >>> Add binding for NXP BBNSM(Battery-Backed Non-Secure Module).
> >>>
> >>> Signed-off-by: Jacky Bai <ping.bai@nxp.com>
> >>> ---
> >
> > ...
> >
> >>> +
> >>> +    properties:
> >>> +      compatible:
> >>> +        const: nxp,bbnsm-rtc
> >>
> >>
> >> Missing ref to rtc.yaml.
> >
> > Ok will include in v2.
> >>
> >>> +
> >>> +      regmap:
> >>
> >> Use vendor prefix, descriptive name and description. Where is the
> >> type of 'regmap' defined?
> >
> > Type is missed. Will add a description and type define if necessary.
> >
> >>
> >>> +        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.
> >
> > For this BBNSM module, basically, it provides two sperate & different
> > function: RTC and ON/OFF button control. But no separate register
> > offset range for each of these functions. For example, the interrupt
> > enable control, Interrupt status and basic function control are mixed
> > in the same registers'
> > different bit.
> >
> > Any good suggestion on how to handle such case? ^_^
> 
> The solution for more complex common parts, dedicated device driver (MFD
> driver) with its own binding. However I understand why it would be overshoot
> here.
>

Is it ok to keep current implementation rather than reimplement a new dedicate MFD wrapper driver?

BR
> >
> >>
> >>> +
> >>> +      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.
> >>
> >
> > It is depends on how SoC integrates this BBNSM module. From the BBNSM
> > side, it has separate IRQ lines for RTC function and ON/OFF function.
> > Different IRQ lines can be used for RTC and ON/OFF button. But in
> > current i.MX93 SoC, those interrupts are ORed together at SoC level.
> > That's why same interrupt in the example.
> 
> It's fine then.
> 
> Best regards,
> Krzysztof

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

[Krzysztof Kozlowski]

On 23/11/2022 08:43, Jacky Bai wrote:
>> Subject: Re: [PATCH 1/4] dt-bindings: mfd: nxp,bbnsm: Add binding for nxp
>> bbnsm
>>
>> On 21/11/2022 11:26, Jacky Bai wrote:
>>>> Subject: Re: [PATCH 1/4] dt-bindings: mfd: nxp,bbnsm: Add binding for
>>>> nxp bbnsm
>>>>
>>>> On 21/11/2022 07:51, Jacky Bai wrote:
>>>>> Add binding for NXP BBNSM(Battery-Backed Non-Secure Module).
>>>>>
>>>>> Signed-off-by: Jacky Bai <ping.bai@nxp.com>
>>>>> ---
>>>
>>> ...
>>>
>>>>> +
>>>>> +    properties:
>>>>> +      compatible:
>>>>> +        const: nxp,bbnsm-rtc
>>>>
>>>>
>>>> Missing ref to rtc.yaml.
>>>
>>> Ok will include in v2.
>>>>
>>>>> +
>>>>> +      regmap:
>>>>
>>>> Use vendor prefix, descriptive name and description. Where is the
>>>> type of 'regmap' defined?
>>>
>>> Type is missed. Will add a description and type define if necessary.
>>>
>>>>
>>>>> +        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.
>>>
>>> For this BBNSM module, basically, it provides two sperate & different
>>> function: RTC and ON/OFF button control. But no separate register
>>> offset range for each of these functions. For example, the interrupt
>>> enable control, Interrupt status and basic function control are mixed
>>> in the same registers'
>>> different bit.
>>>
>>> Any good suggestion on how to handle such case? ^_^
>>
>> The solution for more complex common parts, dedicated device driver (MFD
>> driver) with its own binding. However I understand why it would be overshoot
>> here.
>>
> 
> Is it ok to keep current implementation rather than reimplement a new dedicate MFD wrapper driver?

Yes

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

Also few nits:

On 21/11/2022 07:51, Jacky Bai wrote:
> Add binding for NXP BBNSM(Battery-Backed Non-Secure Module).

Subject: drop second, redundant "bindings".

> 
> Signed-off-by: Jacky Bai <ping.bai@nxp.com>
> ---
>  .../devicetree/bindings/mfd/nxp,bbnsm.yaml    | 103 ++++++++++++++++++
>  1 file changed, 103 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/nxp,bbnsm.yaml
> 
> diff --git a/Documentation/devicetree/bindings/mfd/nxp,bbnsm.yaml b/Documentation/devicetree/bindings/mfd/nxp,bbnsm.yaml
> new file mode 100644
> index 000000000000..b3f22b0daea6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/nxp,bbnsm.yaml
> @@ -0,0 +1,103 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mfd/nxp,bbnsm.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NXP Battery-Backed Non-Secure Module bindings

Drop "bindings"

> +
> +maintainers:
> +  - Jacky Bai <ping.bai@nxp.com>

Best regards,
Krzysztof

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

[Jacky Bai]

[-- Attachment #1: Type: text/plain, Size: 535 bytes --]

> Subject: Re: [PATCH 1/4] dt-bindings: mfd: nxp,bbnsm: Add binding for nxp
> bbnsm
> 
> Also few nits:
> 
> On 21/11/2022 07:51, Jacky Bai wrote:
> > Add binding for NXP BBNSM(Battery-Backed Non-Secure Module).
> 
> Subject: drop second, redundant "bindings".
> 

Do you mean the redundant 'bindings' in below title line?

> > +
> > +title: NXP Battery-Backed Non-Secure Module bindings
> 
> Drop "bindings"
> 

Ok will drop it in v2.

BR

> > +
> > +maintainers:
> > +  - Jacky Bai <ping.bai@nxp.com>
> 
> Best regards,
> Krzysztof


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 9646 bytes --]

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

[Krzysztof Kozlowski]

On 21/11/2022 11:30, Jacky Bai wrote:
>> Subject: Re: [PATCH 1/4] dt-bindings: mfd: nxp,bbnsm: Add binding for nxp
>> bbnsm
>>
>> Also few nits:
>>
>> On 21/11/2022 07:51, Jacky Bai wrote:
>>> Add binding for NXP BBNSM(Battery-Backed Non-Secure Module).
>>
>> Subject: drop second, redundant "bindings".
>>
> 
> Do you mean the redundant 'bindings' in below title line?

No, I meant subject.


Best regards,
Krzysztof

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

[Rob Herring]

On Mon, 21 Nov 2022 14:51:41 +0800, Jacky Bai wrote:
> 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 ++++++++++++++++++
>  1 file changed, 103 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/nxp,bbnsm.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Error: Documentation/devicetree/bindings/mfd/nxp,bbnsm.example.dts:28.27-28 syntax error
FATAL ERROR: Unable to parse input tree
make[1]: *** [scripts/Makefile.lib:406: Documentation/devicetree/bindings/mfd/nxp,bbnsm.example.dtb] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1492: dt_binding_check] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20221121065144.3667658-2-ping.bai@nxp.com

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command.

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

[Jacky Bai]

Hi Rob,

> Subject: Re: [PATCH 1/4] dt-bindings: mfd: nxp,bbnsm: Add binding for nxp
> bbnsm
> 
> 
> On Mon, 21 Nov 2022 14:51:41 +0800, Jacky Bai wrote:
> > 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
> ++++++++++++++++++
> >  1 file changed, 103 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/mfd/nxp,bbnsm.yaml
> >
> 
> My bot found errors running 'make DT_CHECKER_FLAGS=-m
> dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
> 
> yamllint warnings/errors:
> 
> dtschema/dtc warnings/errors:
> Error:
> Documentation/devicetree/bindings/mfd/nxp,bbnsm.example.dts:28.27-28
> syntax error FATAL ERROR: Unable to parse input tree
> make[1]: *** [scripts/Makefile.lib:406:
> Documentation/devicetree/bindings/mfd/nxp,bbnsm.example.dtb] Error 1
> make[1]: *** Waiting for unfinished jobs....
> make: *** [Makefile:1492: dt_binding_check] Error 2
> 

This error should be related to the 'interrupts = <GIC_SPI 73 IRQ_TYPE_LEVEL_HIGH>;'
Do we need to change it a magic number define?

BR
Jacky Bai
> doc reference errors (make refcheckdocs):
> 
...
> 
> This check can fail if there are any dependencies. The base for a patch series
> is generally the most recent rc1.
> 
> If you already ran 'make dt_binding_check' and didn't see the above error(s),
> then make sure 'yamllint' is installed and dt-schema is up to
> date:
> 
> pip3 install dtschema --upgrade
> 
> Please check and re-submit after running the above command.

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

[Krzysztof Kozlowski]

On 23/11/2022 08:54, Jacky Bai wrote:
> Hi Rob,
> 
>> Subject: Re: [PATCH 1/4] dt-bindings: mfd: nxp,bbnsm: Add binding for nxp
>> bbnsm
>>
>>
>> On Mon, 21 Nov 2022 14:51:41 +0800, Jacky Bai wrote:
>>> 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
>> ++++++++++++++++++
>>>  1 file changed, 103 insertions(+)
>>>  create mode 100644
>>> Documentation/devicetree/bindings/mfd/nxp,bbnsm.yaml
>>>
>>
>> My bot found errors running 'make DT_CHECKER_FLAGS=-m
>> dt_binding_check'
>> on your patch (DT_CHECKER_FLAGS is new in v5.13):
>>
>> yamllint warnings/errors:
>>
>> dtschema/dtc warnings/errors:
>> Error:
>> Documentation/devicetree/bindings/mfd/nxp,bbnsm.example.dts:28.27-28
>> syntax error FATAL ERROR: Unable to parse input tree
>> make[1]: *** [scripts/Makefile.lib:406:
>> Documentation/devicetree/bindings/mfd/nxp,bbnsm.example.dtb] Error 1
>> make[1]: *** Waiting for unfinished jobs....
>> make: *** [Makefile:1492: dt_binding_check] Error 2
>>
> 
> This error should be related to the 'interrupts = <GIC_SPI 73 IRQ_TYPE_LEVEL_HIGH>;'
> Do we need to change it a magic number define?

You should include a proper header. Look at other bindings.

Best regards,
Krzysztof

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

[Jacky Bai]

> Subject: Re: [PATCH 1/4] dt-bindings: mfd: nxp,bbnsm: Add binding for nxp
> bbnsm
> 
> On 23/11/2022 08:54, Jacky Bai wrote:
> > Hi Rob,
> >
> >> Subject: Re: [PATCH 1/4] dt-bindings: mfd: nxp,bbnsm: Add binding for
> >> nxp bbnsm
> >>
> >>
> >> On Mon, 21 Nov 2022 14:51:41 +0800, Jacky Bai wrote:
> >>> 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
> >> ++++++++++++++++++
> >>>  1 file changed, 103 insertions(+)
> >>>  create mode 100644
> >>> Documentation/devicetree/bindings/mfd/nxp,bbnsm.yaml
> >>>
> >>
> >> My bot found errors running 'make DT_CHECKER_FLAGS=-m
> >> dt_binding_check'
> >> on your patch (DT_CHECKER_FLAGS is new in v5.13):
> >>
> >> yamllint warnings/errors:
> >>
> >> dtschema/dtc warnings/errors:
> >> Error:
> >>
> Documentation/devicetree/bindings/mfd/nxp,bbnsm.example.dts:28.27-28
> >> syntax error FATAL ERROR: Unable to parse input tree
> >> make[1]: *** [scripts/Makefile.lib:406:
> >> Documentation/devicetree/bindings/mfd/nxp,bbnsm.example.dtb] Error 1
> >> make[1]: *** Waiting for unfinished jobs....
> >> make: *** [Makefile:1492: dt_binding_check] Error 2
> >>
> >
> > This error should be related to the 'interrupts = <GIC_SPI 73
> IRQ_TYPE_LEVEL_HIGH>;'
> > Do we need to change it a magic number define?
> 
> You should include a proper header. Look at other bindings.
> 

Great thx. I missed the header file including . Will fix in V2. ^_^

BR
> Best regards,
> Krzysztof

[PATCH 2/4] input: bbnsm_pwrkey: Add bbnsm power key support

[Jacky Bai]

The ON/OFF logic inside the BBNSM allows for connecting directly
into a PMIC or other voltage regulator device. The module has an
button input signal and a wakeup request input signal. It also
has two interrupts (set_pwr_off_irq and set_pwr_on_irq) and an
active-low PMIC enable (pmic_en_b) output.

Add the power key support for the ON/OFF button function found in
BBNSM module.

Signed-off-by: Jacky Bai <ping.bai@nxp.com>
Reviewed-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/input/keyboard/Kconfig        |  11 ++
 drivers/input/keyboard/Makefile       |   1 +
 drivers/input/keyboard/bbnsm_pwrkey.c | 196 ++++++++++++++++++++++++++
 3 files changed, 208 insertions(+)
 create mode 100644 drivers/input/keyboard/bbnsm_pwrkey.c

diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
index 00292118b79b..8efcd95492b3 100644
--- a/drivers/input/keyboard/Kconfig
+++ b/drivers/input/keyboard/Kconfig
@@ -456,6 +456,17 @@ config KEYBOARD_SNVS_PWRKEY
 	  To compile this driver as a module, choose M here; the
 	  module will be called snvs_pwrkey.
 
+config KEYBOARD_BBNSM_PWRKEY
+	tristate "NXP BBNSM Power Key Driver"
+	depends on ARCH_MXC || COMPILE_TEST
+	depends on OF
+	help
+	  This is the bbnsm powerkey driver for the NXP i.MX application
+	  processors.
+
+	  To compile this driver as a module, choose M here; the
+	  module will be called bbnsm_pwrkey.
+
 config KEYBOARD_IMX
 	tristate "IMX keypad support"
 	depends on ARCH_MXC || COMPILE_TEST
diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
index 5f67196bb2c1..0bc101e004ae 100644
--- a/drivers/input/keyboard/Makefile
+++ b/drivers/input/keyboard/Makefile
@@ -62,6 +62,7 @@ obj-$(CONFIG_KEYBOARD_QT2160)		+= qt2160.o
 obj-$(CONFIG_KEYBOARD_SAMSUNG)		+= samsung-keypad.o
 obj-$(CONFIG_KEYBOARD_SH_KEYSC)		+= sh_keysc.o
 obj-$(CONFIG_KEYBOARD_SNVS_PWRKEY)	+= snvs_pwrkey.o
+obj-$(CONFIG_KEYBOARD_BBNSM_PWRKEY)	+= bbnsm_pwrkey.o
 obj-$(CONFIG_KEYBOARD_SPEAR)		+= spear-keyboard.o
 obj-$(CONFIG_KEYBOARD_STMPE)		+= stmpe-keypad.o
 obj-$(CONFIG_KEYBOARD_STOWAWAY)		+= stowaway.o
diff --git a/drivers/input/keyboard/bbnsm_pwrkey.c b/drivers/input/keyboard/bbnsm_pwrkey.c
new file mode 100644
index 000000000000..288ee6844000
--- /dev/null
+++ b/drivers/input/keyboard/bbnsm_pwrkey.c
@@ -0,0 +1,196 @@
+// SPDX-License-Identifier: GPL-2.0+
+//
+// Copyright 2022 NXP.
+
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/input.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/jiffies.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/pm_wakeirq.h>
+#include <linux/mfd/syscon.h>
+#include <linux/regmap.h>
+
+#define BBNSM_CTRL		0x8
+#define BBNSM_INT_EN		0x10
+#define BBNSM_EVENTS		0x14
+#define BBNSM_PAD_CTRL		0x24
+
+#define BBNSM_BTN_PRESSED	BIT(7)
+#define BBNSM_PWR_ON		BIT(6)
+#define BBNSM_BTN_OFF		BIT(5)
+#define BBNSM_EMG_OFF		BIT(4)
+#define BBNSM_PWRKEY_EVENTS	(BBNSM_PWR_ON | BBNSM_BTN_OFF | BBNSM_EMG_OFF)
+#define BBNSM_DP_EN		BIT(24)
+
+#define DEBOUNCE_TIME		30
+#define REPEAT_INTERVAL		60
+
+struct bbnsm_pwrkey {
+	struct regmap *regmap;
+	int irq;
+	int keycode;
+	int keystate;  /* 1:pressed */
+	struct timer_list check_timer;
+	struct input_dev *input;
+};
+
+static void bbnsm_pwrkey_check_for_events(struct timer_list *t)
+{
+	struct bbnsm_pwrkey *bbnsm = from_timer(bbnsm, t, check_timer);
+	struct input_dev *input = bbnsm->input;
+	u32 state;
+
+	regmap_read(bbnsm->regmap, BBNSM_EVENTS, &state);
+
+	state = state & BBNSM_BTN_PRESSED ? 1 : 0;
+
+	/* only report new event if status changed */
+	if (state ^ bbnsm->keystate) {
+		bbnsm->keystate = state;
+		input_event(input, EV_KEY, bbnsm->keycode, state);
+		input_sync(input);
+		pm_relax(bbnsm->input->dev.parent);
+	}
+
+	/* repeat check if pressed long */
+	if (state) {
+		mod_timer(&bbnsm->check_timer,
+			  jiffies + msecs_to_jiffies(REPEAT_INTERVAL));
+	}
+}
+
+static irqreturn_t bbnsm_pwrkey_interrupt(int irq, void *dev_id)
+{
+	struct platform_device *pdev = dev_id;
+	struct bbnsm_pwrkey *bbnsm = platform_get_drvdata(pdev);
+	struct input_dev *input = bbnsm->input;
+	u32 event;
+
+	regmap_read(bbnsm->regmap, BBNSM_EVENTS, &event);
+	if (event & BBNSM_BTN_OFF)
+		mod_timer(&bbnsm->check_timer, jiffies + msecs_to_jiffies(DEBOUNCE_TIME));
+	else
+		return IRQ_NONE;
+
+	pm_wakeup_event(input->dev.parent, 0);
+
+	/* clear PWR OFF */
+	regmap_write(bbnsm->regmap, BBNSM_EVENTS, BBNSM_BTN_OFF);
+
+	return IRQ_HANDLED;
+}
+
+static void bbnsm_pwrkey_act(void *pdata)
+{
+	struct bbnsm_pwrkey *bbnsm = pdata;
+
+	del_timer_sync(&bbnsm->check_timer);
+}
+
+static int bbnsm_pwrkey_probe(struct platform_device *pdev)
+{
+	struct bbnsm_pwrkey *bbnsm;
+	struct input_dev *input;
+	struct device_node *np = pdev->dev.of_node;
+	int error;
+
+	bbnsm = devm_kzalloc(&pdev->dev, sizeof(*bbnsm), GFP_KERNEL);
+	if (!bbnsm)
+		return -ENOMEM;
+
+	bbnsm->regmap = syscon_regmap_lookup_by_phandle(pdev->dev.of_node, "regmap");
+	if (IS_ERR(bbnsm->regmap)) {
+		dev_err(&pdev->dev, "bbnsm pwerkey get regmap failed\n");
+		return PTR_ERR(bbnsm->regmap);
+	}
+
+	if (of_property_read_u32(np, "linux,code", &bbnsm->keycode)) {
+		bbnsm->keycode = KEY_POWER;
+		dev_warn(&pdev->dev, "KEY_POWER without setting in dts\n");
+	}
+
+	bbnsm->irq = platform_get_irq(pdev, 0);
+	if (bbnsm->irq < 0)
+		return -EINVAL;
+
+	/* config the BBNSM power related register */
+	regmap_update_bits(bbnsm->regmap, BBNSM_CTRL, BBNSM_DP_EN, BBNSM_DP_EN);
+
+	/* clear the unexpected interrupt before driver ready */
+	regmap_write_bits(bbnsm->regmap, BBNSM_EVENTS, BBNSM_PWRKEY_EVENTS, BBNSM_PWRKEY_EVENTS);
+
+	timer_setup(&bbnsm->check_timer, bbnsm_pwrkey_check_for_events, 0);
+
+	input = devm_input_allocate_device(&pdev->dev);
+	if (!input) {
+		dev_err(&pdev->dev, "failed to allocate the input device\n");
+		error = -ENOMEM;
+		goto error_probe;
+	}
+
+	input->name = pdev->name;
+	input->phys = "bbnsm-pwrkey/input0";
+	input->id.bustype = BUS_HOST;
+
+	input_set_capability(input, EV_KEY, bbnsm->keycode);
+
+	/* input customer action to cancel release timer */
+	error = devm_add_action(&pdev->dev, bbnsm_pwrkey_act, bbnsm);
+	if (error) {
+		dev_err(&pdev->dev, "failed to register remove action\n");
+		goto error_probe;
+	}
+
+	bbnsm->input = input;
+	platform_set_drvdata(pdev, bbnsm);
+
+	error = devm_request_irq(&pdev->dev, bbnsm->irq, bbnsm_pwrkey_interrupt,
+			       IRQF_SHARED, pdev->name, pdev);
+	if (error) {
+		dev_err(&pdev->dev, "interrupt not available.\n");
+		goto error_probe;
+	}
+
+	error = input_register_device(input);
+	if (error < 0) {
+		dev_err(&pdev->dev, "failed to register input device\n");
+		goto error_probe;
+	}
+
+	device_init_wakeup(&pdev->dev, true);
+	error = dev_pm_set_wake_irq(&pdev->dev, bbnsm->irq);
+	if (error)
+		dev_err(&pdev->dev, "irq wake enable failed.\n");
+
+	return 0;
+
+error_probe:
+	return error;
+}
+
+static const struct of_device_id bbnsm_pwrkey_ids[] = {
+	{ .compatible = "nxp,bbnsm-pwrkey" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, bbnsm_pwrkey_ids);
+
+static struct platform_driver bbnsm_pwrkey_driver = {
+	.driver = {
+		.name = "bbnsm_pwrkey",
+		.of_match_table = bbnsm_pwrkey_ids,
+	},
+	.probe = bbnsm_pwrkey_probe,
+};
+module_platform_driver(bbnsm_pwrkey_driver);
+
+MODULE_AUTHOR("Jacky Bai <ping.bai@nxp.com>");
+MODULE_DESCRIPTION("NXP bbnsm power key Driver");
+MODULE_LICENSE("GPL");
-- 
2.37.1

Re: [PATCH 2/4] input: bbnsm_pwrkey: Add bbnsm power key support

[Dmitry Torokhov]

Hi Jacky,

On Mon, Nov 21, 2022 at 02:51:42PM +0800, Jacky Bai wrote:
> The ON/OFF logic inside the BBNSM allows for connecting directly
> into a PMIC or other voltage regulator device. The module has an
> button input signal and a wakeup request input signal. It also
> has two interrupts (set_pwr_off_irq and set_pwr_on_irq) and an
> active-low PMIC enable (pmic_en_b) output.
> 
> Add the power key support for the ON/OFF button function found in
> BBNSM module.
> 
> Signed-off-by: Jacky Bai <ping.bai@nxp.com>
> Reviewed-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/input/keyboard/Kconfig        |  11 ++
>  drivers/input/keyboard/Makefile       |   1 +
>  drivers/input/keyboard/bbnsm_pwrkey.c | 196 ++++++++++++++++++++++++++
>  3 files changed, 208 insertions(+)
>  create mode 100644 drivers/input/keyboard/bbnsm_pwrkey.c
> 
> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
> index 00292118b79b..8efcd95492b3 100644
> --- a/drivers/input/keyboard/Kconfig
> +++ b/drivers/input/keyboard/Kconfig
> @@ -456,6 +456,17 @@ config KEYBOARD_SNVS_PWRKEY
>  	  To compile this driver as a module, choose M here; the
>  	  module will be called snvs_pwrkey.
>  
> +config KEYBOARD_BBNSM_PWRKEY
> +	tristate "NXP BBNSM Power Key Driver"
> +	depends on ARCH_MXC || COMPILE_TEST
> +	depends on OF
> +	help
> +	  This is the bbnsm powerkey driver for the NXP i.MX application
> +	  processors.
> +
> +	  To compile this driver as a module, choose M here; the
> +	  module will be called bbnsm_pwrkey.
> +
>  config KEYBOARD_IMX
>  	tristate "IMX keypad support"
>  	depends on ARCH_MXC || COMPILE_TEST
> diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
> index 5f67196bb2c1..0bc101e004ae 100644
> --- a/drivers/input/keyboard/Makefile
> +++ b/drivers/input/keyboard/Makefile
> @@ -62,6 +62,7 @@ obj-$(CONFIG_KEYBOARD_QT2160)		+= qt2160.o
>  obj-$(CONFIG_KEYBOARD_SAMSUNG)		+= samsung-keypad.o
>  obj-$(CONFIG_KEYBOARD_SH_KEYSC)		+= sh_keysc.o
>  obj-$(CONFIG_KEYBOARD_SNVS_PWRKEY)	+= snvs_pwrkey.o
> +obj-$(CONFIG_KEYBOARD_BBNSM_PWRKEY)	+= bbnsm_pwrkey.o
>  obj-$(CONFIG_KEYBOARD_SPEAR)		+= spear-keyboard.o
>  obj-$(CONFIG_KEYBOARD_STMPE)		+= stmpe-keypad.o
>  obj-$(CONFIG_KEYBOARD_STOWAWAY)		+= stowaway.o
> diff --git a/drivers/input/keyboard/bbnsm_pwrkey.c b/drivers/input/keyboard/bbnsm_pwrkey.c
> new file mode 100644
> index 000000000000..288ee6844000
> --- /dev/null
> +++ b/drivers/input/keyboard/bbnsm_pwrkey.c
> @@ -0,0 +1,196 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +//
> +// Copyright 2022 NXP.
> +
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/input.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/jiffies.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_wakeirq.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/regmap.h>
> +
> +#define BBNSM_CTRL		0x8
> +#define BBNSM_INT_EN		0x10
> +#define BBNSM_EVENTS		0x14
> +#define BBNSM_PAD_CTRL		0x24
> +
> +#define BBNSM_BTN_PRESSED	BIT(7)
> +#define BBNSM_PWR_ON		BIT(6)
> +#define BBNSM_BTN_OFF		BIT(5)
> +#define BBNSM_EMG_OFF		BIT(4)
> +#define BBNSM_PWRKEY_EVENTS	(BBNSM_PWR_ON | BBNSM_BTN_OFF | BBNSM_EMG_OFF)
> +#define BBNSM_DP_EN		BIT(24)
> +
> +#define DEBOUNCE_TIME		30
> +#define REPEAT_INTERVAL		60
> +
> +struct bbnsm_pwrkey {
> +	struct regmap *regmap;
> +	int irq;
> +	int keycode;
> +	int keystate;  /* 1:pressed */
> +	struct timer_list check_timer;
> +	struct input_dev *input;
> +};
> +
> +static void bbnsm_pwrkey_check_for_events(struct timer_list *t)
> +{
> +	struct bbnsm_pwrkey *bbnsm = from_timer(bbnsm, t, check_timer);
> +	struct input_dev *input = bbnsm->input;
> +	u32 state;
> +
> +	regmap_read(bbnsm->regmap, BBNSM_EVENTS, &state);

Can this fail?

> +
> +	state = state & BBNSM_BTN_PRESSED ? 1 : 0;
> +
> +	/* only report new event if status changed */
> +	if (state ^ bbnsm->keystate) {
> +		bbnsm->keystate = state;
> +		input_event(input, EV_KEY, bbnsm->keycode, state);
> +		input_sync(input);
> +		pm_relax(bbnsm->input->dev.parent);
> +	}
> +
> +	/* repeat check if pressed long */
> +	if (state) {
> +		mod_timer(&bbnsm->check_timer,
> +			  jiffies + msecs_to_jiffies(REPEAT_INTERVAL));
> +	}

So interrupt is only generated once when key is pressed, but not on
release?

> +}
> +
> +static irqreturn_t bbnsm_pwrkey_interrupt(int irq, void *dev_id)
> +{
> +	struct platform_device *pdev = dev_id;
> +	struct bbnsm_pwrkey *bbnsm = platform_get_drvdata(pdev);
> +	struct input_dev *input = bbnsm->input;
> +	u32 event;
> +
> +	regmap_read(bbnsm->regmap, BBNSM_EVENTS, &event);
> +	if (event & BBNSM_BTN_OFF)
> +		mod_timer(&bbnsm->check_timer, jiffies + msecs_to_jiffies(DEBOUNCE_TIME));
> +	else
> +		return IRQ_NONE;
> +
> +	pm_wakeup_event(input->dev.parent, 0);
> +
> +	/* clear PWR OFF */
> +	regmap_write(bbnsm->regmap, BBNSM_EVENTS, BBNSM_BTN_OFF);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static void bbnsm_pwrkey_act(void *pdata)
> +{
> +	struct bbnsm_pwrkey *bbnsm = pdata;
> +
> +	del_timer_sync(&bbnsm->check_timer);
> +}
> +
> +static int bbnsm_pwrkey_probe(struct platform_device *pdev)
> +{
> +	struct bbnsm_pwrkey *bbnsm;
> +	struct input_dev *input;
> +	struct device_node *np = pdev->dev.of_node;
> +	int error;
> +
> +	bbnsm = devm_kzalloc(&pdev->dev, sizeof(*bbnsm), GFP_KERNEL);
> +	if (!bbnsm)
> +		return -ENOMEM;
> +
> +	bbnsm->regmap = syscon_regmap_lookup_by_phandle(pdev->dev.of_node, "regmap");
> +	if (IS_ERR(bbnsm->regmap)) {
> +		dev_err(&pdev->dev, "bbnsm pwerkey get regmap failed\n");
> +		return PTR_ERR(bbnsm->regmap);
> +	}
> +
> +	if (of_property_read_u32(np, "linux,code", &bbnsm->keycode)) {

Please use device_property_read_u32() here.

> +		bbnsm->keycode = KEY_POWER;
> +		dev_warn(&pdev->dev, "KEY_POWER without setting in dts\n");
> +	}
> +
> +	bbnsm->irq = platform_get_irq(pdev, 0);
> +	if (bbnsm->irq < 0)
> +		return -EINVAL;
> +
> +	/* config the BBNSM power related register */
> +	regmap_update_bits(bbnsm->regmap, BBNSM_CTRL, BBNSM_DP_EN, BBNSM_DP_EN);
> +
> +	/* clear the unexpected interrupt before driver ready */
> +	regmap_write_bits(bbnsm->regmap, BBNSM_EVENTS, BBNSM_PWRKEY_EVENTS, BBNSM_PWRKEY_EVENTS);
> +
> +	timer_setup(&bbnsm->check_timer, bbnsm_pwrkey_check_for_events, 0);
> +
> +	input = devm_input_allocate_device(&pdev->dev);
> +	if (!input) {
> +		dev_err(&pdev->dev, "failed to allocate the input device\n");
> +		error = -ENOMEM;
> +		goto error_probe;

Please return directly here and below, since there is not explicit
cleanup.

> +	}
> +
> +	input->name = pdev->name;
> +	input->phys = "bbnsm-pwrkey/input0";
> +	input->id.bustype = BUS_HOST;
> +
> +	input_set_capability(input, EV_KEY, bbnsm->keycode);
> +
> +	/* input customer action to cancel release timer */
> +	error = devm_add_action(&pdev->dev, bbnsm_pwrkey_act, bbnsm);
> +	if (error) {
> +		dev_err(&pdev->dev, "failed to register remove action\n");
> +		goto error_probe;
> +	}
> +
> +	bbnsm->input = input;
> +	platform_set_drvdata(pdev, bbnsm);
> +
> +	error = devm_request_irq(&pdev->dev, bbnsm->irq, bbnsm_pwrkey_interrupt,
> +			       IRQF_SHARED, pdev->name, pdev);
> +	if (error) {
> +		dev_err(&pdev->dev, "interrupt not available.\n");
> +		goto error_probe;
> +	}
> +
> +	error = input_register_device(input);
> +	if (error < 0) {
> +		dev_err(&pdev->dev, "failed to register input device\n");
> +		goto error_probe;
> +	}
> +
> +	device_init_wakeup(&pdev->dev, true);
> +	error = dev_pm_set_wake_irq(&pdev->dev, bbnsm->irq);
> +	if (error)
> +		dev_err(&pdev->dev, "irq wake enable failed.\n");
> +
> +	return 0;
> +
> +error_probe:
> +	return error;
> +}
> +
> +static const struct of_device_id bbnsm_pwrkey_ids[] = {
> +	{ .compatible = "nxp,bbnsm-pwrkey" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, bbnsm_pwrkey_ids);
> +
> +static struct platform_driver bbnsm_pwrkey_driver = {
> +	.driver = {
> +		.name = "bbnsm_pwrkey",
> +		.of_match_table = bbnsm_pwrkey_ids,
> +	},
> +	.probe = bbnsm_pwrkey_probe,
> +};
> +module_platform_driver(bbnsm_pwrkey_driver);
> +
> +MODULE_AUTHOR("Jacky Bai <ping.bai@nxp.com>");
> +MODULE_DESCRIPTION("NXP bbnsm power key Driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.37.1
> 

Thanks.

-- 
Dmitry

RE: [PATCH 2/4] input: bbnsm_pwrkey: Add bbnsm power key support

[Jacky Bai]

> Subject: Re: [PATCH 2/4] input: bbnsm_pwrkey: Add bbnsm power key
> support
> 
> Hi Jacky,
> 
> On Mon, Nov 21, 2022 at 02:51:42PM +0800, Jacky Bai wrote:
> > The ON/OFF logic inside the BBNSM allows for connecting directly into
> > a PMIC or other voltage regulator device. The module has an button
> > input signal and a wakeup request input signal. It also has two
> > interrupts (set_pwr_off_irq and set_pwr_on_irq) and an active-low PMIC
> > enable (pmic_en_b) output.
> >
> > Add the power key support for the ON/OFF button function found in
> > BBNSM module.
> >
> > Signed-off-by: Jacky Bai <ping.bai@nxp.com>
> > Reviewed-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  drivers/input/keyboard/Kconfig        |  11 ++
> >  drivers/input/keyboard/Makefile       |   1 +
> >  drivers/input/keyboard/bbnsm_pwrkey.c | 196
> > ++++++++++++++++++++++++++
> >  3 files changed, 208 insertions(+)
> >  create mode 100644 drivers/input/keyboard/bbnsm_pwrkey.c
> >
> > diff --git a/drivers/input/keyboard/Kconfig
> > b/drivers/input/keyboard/Kconfig index 00292118b79b..8efcd95492b3
> > 100644
> > --- a/drivers/input/keyboard/Kconfig
> > +++ b/drivers/input/keyboard/Kconfig
> > @@ -456,6 +456,17 @@ config KEYBOARD_SNVS_PWRKEY
> >  	  To compile this driver as a module, choose M here; the
> >  	  module will be called snvs_pwrkey.
> >
> > +config KEYBOARD_BBNSM_PWRKEY
> > +	tristate "NXP BBNSM Power Key Driver"
> > +	depends on ARCH_MXC || COMPILE_TEST
> > +	depends on OF
> > +	help
> > +	  This is the bbnsm powerkey driver for the NXP i.MX application
> > +	  processors.
> > +
> > +	  To compile this driver as a module, choose M here; the
> > +	  module will be called bbnsm_pwrkey.
> > +
> >  config KEYBOARD_IMX
> >  	tristate "IMX keypad support"
> >  	depends on ARCH_MXC || COMPILE_TEST
> > diff --git a/drivers/input/keyboard/Makefile
> > b/drivers/input/keyboard/Makefile index 5f67196bb2c1..0bc101e004ae
> > 100644
> > --- a/drivers/input/keyboard/Makefile
> > +++ b/drivers/input/keyboard/Makefile
> > @@ -62,6 +62,7 @@ obj-$(CONFIG_KEYBOARD_QT2160)		+=
> qt2160.o
> >  obj-$(CONFIG_KEYBOARD_SAMSUNG)		+= samsung-keypad.o
> >  obj-$(CONFIG_KEYBOARD_SH_KEYSC)		+= sh_keysc.o
> >  obj-$(CONFIG_KEYBOARD_SNVS_PWRKEY)	+= snvs_pwrkey.o
> > +obj-$(CONFIG_KEYBOARD_BBNSM_PWRKEY)	+= bbnsm_pwrkey.o
> >  obj-$(CONFIG_KEYBOARD_SPEAR)		+= spear-keyboard.o
> >  obj-$(CONFIG_KEYBOARD_STMPE)		+= stmpe-keypad.o
> >  obj-$(CONFIG_KEYBOARD_STOWAWAY)		+= stowaway.o
> > diff --git a/drivers/input/keyboard/bbnsm_pwrkey.c
> > b/drivers/input/keyboard/bbnsm_pwrkey.c
> > new file mode 100644
> > index 000000000000..288ee6844000
> > --- /dev/null
> > +++ b/drivers/input/keyboard/bbnsm_pwrkey.c

...

> > +
> > +static void bbnsm_pwrkey_check_for_events(struct timer_list *t) {
> > +	struct bbnsm_pwrkey *bbnsm = from_timer(bbnsm, t, check_timer);
> > +	struct input_dev *input = bbnsm->input;
> > +	u32 state;
> > +
> > +	regmap_read(bbnsm->regmap, BBNSM_EVENTS, &state);
> 
> Can this fail?

Should no chance to fail. Any more tips?

> 
> > +
> > +	state = state & BBNSM_BTN_PRESSED ? 1 : 0;
> > +
> > +	/* only report new event if status changed */
> > +	if (state ^ bbnsm->keystate) {
> > +		bbnsm->keystate = state;
> > +		input_event(input, EV_KEY, bbnsm->keycode, state);
> > +		input_sync(input);
> > +		pm_relax(bbnsm->input->dev.parent);
> > +	}
> > +
> > +	/* repeat check if pressed long */
> > +	if (state) {
> > +		mod_timer(&bbnsm->check_timer,
> > +			  jiffies + msecs_to_jiffies(REPEAT_INTERVAL));
> > +	}
> 
> So interrupt is only generated once when key is pressed, but not on release?
> 

Yes, at lease from my test, this interrupt can only be triggered when pressed.

> > +}
> > +
> > +static irqreturn_t bbnsm_pwrkey_interrupt(int irq, void *dev_id) {
> > +	struct platform_device *pdev = dev_id;
> > +	struct bbnsm_pwrkey *bbnsm = platform_get_drvdata(pdev);
> > +	struct input_dev *input = bbnsm->input;
> > +	u32 event;
> > +
> > +	regmap_read(bbnsm->regmap, BBNSM_EVENTS, &event);
> > +	if (event & BBNSM_BTN_OFF)
> > +		mod_timer(&bbnsm->check_timer, jiffies +
> msecs_to_jiffies(DEBOUNCE_TIME));
> > +	else
> > +		return IRQ_NONE;
> > +
> > +	pm_wakeup_event(input->dev.parent, 0);
> > +
> > +	/* clear PWR OFF */
> > +	regmap_write(bbnsm->regmap, BBNSM_EVENTS, BBNSM_BTN_OFF);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static void bbnsm_pwrkey_act(void *pdata) {
> > +	struct bbnsm_pwrkey *bbnsm = pdata;
> > +
> > +	del_timer_sync(&bbnsm->check_timer);
> > +}
> > +
> > +static int bbnsm_pwrkey_probe(struct platform_device *pdev) {
> > +	struct bbnsm_pwrkey *bbnsm;
> > +	struct input_dev *input;
> > +	struct device_node *np = pdev->dev.of_node;
> > +	int error;
> > +
> > +	bbnsm = devm_kzalloc(&pdev->dev, sizeof(*bbnsm), GFP_KERNEL);
> > +	if (!bbnsm)
> > +		return -ENOMEM;
> > +
> > +	bbnsm->regmap =
> syscon_regmap_lookup_by_phandle(pdev->dev.of_node, "regmap");
> > +	if (IS_ERR(bbnsm->regmap)) {
> > +		dev_err(&pdev->dev, "bbnsm pwerkey get regmap failed\n");
> > +		return PTR_ERR(bbnsm->regmap);
> > +	}
> > +
> > +	if (of_property_read_u32(np, "linux,code", &bbnsm->keycode)) {
> 
> Please use device_property_read_u32() here.

Ok, will fix in V2.

> 
> > +		bbnsm->keycode = KEY_POWER;
> > +		dev_warn(&pdev->dev, "KEY_POWER without setting in dts\n");
> > +	}
> > +
> > +	bbnsm->irq = platform_get_irq(pdev, 0);
> > +	if (bbnsm->irq < 0)
> > +		return -EINVAL;
> > +
> > +	/* config the BBNSM power related register */
> > +	regmap_update_bits(bbnsm->regmap, BBNSM_CTRL, BBNSM_DP_EN,
> > +BBNSM_DP_EN);
> > +
> > +	/* clear the unexpected interrupt before driver ready */
> > +	regmap_write_bits(bbnsm->regmap, BBNSM_EVENTS,
> BBNSM_PWRKEY_EVENTS,
> > +BBNSM_PWRKEY_EVENTS);
> > +
> > +	timer_setup(&bbnsm->check_timer, bbnsm_pwrkey_check_for_events,
> 0);
> > +
> > +	input = devm_input_allocate_device(&pdev->dev);
> > +	if (!input) {
> > +		dev_err(&pdev->dev, "failed to allocate the input device\n");
> > +		error = -ENOMEM;
> > +		goto error_probe;
> 
> Please return directly here and below, since there is not explicit cleanup.
> 

Thx, will fix in V2.

BR

> > +	}
> > +
> > +	input->name = pdev->name;
> > +	input->phys = "bbnsm-pwrkey/input0";
> > +	input->id.bustype = BUS_HOST;
> > +
> > +	input_set_capability(input, EV_KEY, bbnsm->keycode);
> > +
> > +	/* input customer action to cancel release timer */
> > +	error = devm_add_action(&pdev->dev, bbnsm_pwrkey_act, bbnsm);
> > +	if (error) {
> > +		dev_err(&pdev->dev, "failed to register remove action\n");
> > +		goto error_probe;
> > +	}
> > +
> > +	bbnsm->input = input;
> > +	platform_set_drvdata(pdev, bbnsm);
> > +
> > +	error = devm_request_irq(&pdev->dev, bbnsm->irq,
> bbnsm_pwrkey_interrupt,
> > +			       IRQF_SHARED, pdev->name, pdev);
> > +	if (error) {
> > +		dev_err(&pdev->dev, "interrupt not available.\n");
> > +		goto error_probe;
> > +	}
> > +
> > +	error = input_register_device(input);
> > +	if (error < 0) {
> > +		dev_err(&pdev->dev, "failed to register input device\n");
> > +		goto error_probe;
> > +	}
> > +
> > +	device_init_wakeup(&pdev->dev, true);
> > +	error = dev_pm_set_wake_irq(&pdev->dev, bbnsm->irq);
> > +	if (error)
> > +		dev_err(&pdev->dev, "irq wake enable failed.\n");
> > +
> > +	return 0;
> > +
> > +error_probe:
> > +	return error;
> > +}
> > +
> > +static const struct of_device_id bbnsm_pwrkey_ids[] = {
> > +	{ .compatible = "nxp,bbnsm-pwrkey" },
> > +	{ /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, bbnsm_pwrkey_ids);
> > +
> > +static struct platform_driver bbnsm_pwrkey_driver = {
> > +	.driver = {
> > +		.name = "bbnsm_pwrkey",
> > +		.of_match_table = bbnsm_pwrkey_ids,
> > +	},
> > +	.probe = bbnsm_pwrkey_probe,
> > +};
> > +module_platform_driver(bbnsm_pwrkey_driver);
> > +
> > +MODULE_AUTHOR("Jacky Bai <ping.bai@nxp.com>");
> > +MODULE_DESCRIPTION("NXP bbnsm power key Driver");
> > +MODULE_LICENSE("GPL");
> > --
> > 2.37.1
> >
> 
> Thanks.
> 
> --
> Dmitry

[PATCH 3/4] rtc: bbnsm: Add the bbnsm rtc support

[Jacky Bai]

The BBNSM module includes a real time counter with alarm.
Add a RTC driver for this function.

Signed-off-by: Jacky Bai <ping.bai@nxp.com>
Reviewed-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/rtc/Kconfig     |  12 +++
 drivers/rtc/Makefile    |   1 +
 drivers/rtc/rtc-bbnsm.c | 223 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 236 insertions(+)
 create mode 100644 drivers/rtc/rtc-bbnsm.c

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index ab9a1f814119..0c8534a49c78 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -1786,6 +1786,18 @@ config RTC_DRV_SNVS
 	   This driver can also be built as a module, if so, the module
 	   will be called "rtc-snvs".
 
+config RTC_DRV_BBNSM
+	tristate "NXP BBNSM RTC support"
+	select REGMAP_MMIO
+	depends on ARCH_MXC || COMPILE_TEST
+	depends on HAS_IOMEM
+	depends on OF
+	help
+	   If you say yes here you get support for the NXP BBNSM RTC module.
+
+	   This driver can also be built as a module, if so, the module
+	   will be called "rtc-bbnsm".
+
 config RTC_DRV_IMX_SC
 	depends on IMX_SCU
 	depends on HAVE_ARM_SMCCC
diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index d3c042dcbc73..43bd29b2f42f 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -33,6 +33,7 @@ obj-$(CONFIG_RTC_DRV_ASPEED)	+= rtc-aspeed.o
 obj-$(CONFIG_RTC_DRV_AT91RM9200)+= rtc-at91rm9200.o
 obj-$(CONFIG_RTC_DRV_AT91SAM9)	+= rtc-at91sam9.o
 obj-$(CONFIG_RTC_DRV_AU1XXX)	+= rtc-au1xxx.o
+obj-$(CONFIG_RTC_DRV_BBNSM)	+= rtc-bbnsm.o
 obj-$(CONFIG_RTC_DRV_BD70528)	+= rtc-bd70528.o
 obj-$(CONFIG_RTC_DRV_BQ32K)	+= rtc-bq32k.o
 obj-$(CONFIG_RTC_DRV_BQ4802)	+= rtc-bq4802.o
diff --git a/drivers/rtc/rtc-bbnsm.c b/drivers/rtc/rtc-bbnsm.c
new file mode 100644
index 000000000000..4157b238ed9a
--- /dev/null
+++ b/drivers/rtc/rtc-bbnsm.c
@@ -0,0 +1,223 @@
+// SPDX-License-Identifier: GPL-2.0+
+//
+// Copyright 2022 NXP.
+
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pm_wakeirq.h>
+#include <linux/rtc.h>
+#include <linux/mfd/syscon.h>
+#include <linux/regmap.h>
+
+#define BBNSM_CTRL	0x8
+#define BBNSM_INT_EN	0x10
+#define BBNSM_EVENTS	0x14
+#define BBNSM_RTC_LS	0x40
+#define BBNSM_RTC_MS	0x44
+#define BBNSM_TA	0x50
+
+#define RTC_EN		0x2
+#define RTC_EN_MSK	0x3
+#define TA_EN		(0x2 << 2)
+#define TA_DIS		(0x1 << 2)
+#define TA_EN_MSK	(0x3 << 2)
+#define RTC_INT_EN	0x2
+#define TA_INT_EN	(0x2 << 2)
+
+#define BBNSM_EVENT_TA	TA_EN
+
+#define CNTR_TO_SECS_SH	15
+
+struct bbnsm_rtc {
+	struct rtc_device *rtc;
+	struct regmap *regmap;
+	int irq;
+	struct clk *clk;
+};
+
+static u32 bbnsm_read_counter(struct bbnsm_rtc *bbnsm)
+{
+	u32 rtc_msb, rtc_lsb;
+	unsigned int timeout = 100;
+	u32 time;
+	u32 tmp = 0;
+
+	do {
+		time = tmp;
+		/* read the msb */
+		regmap_read(bbnsm->regmap, BBNSM_RTC_MS, &rtc_msb);
+		/* read the lsb */
+		regmap_read(bbnsm->regmap, BBNSM_RTC_LS, &rtc_lsb);
+		/* convert to seconds */
+		tmp = (rtc_msb << 17) | (rtc_lsb >> 15);
+	} while (tmp != time && --timeout);
+
+	return time;
+}
+
+static int bbnsm_rtc_read_time(struct device *dev, struct rtc_time *tm)
+{
+	struct bbnsm_rtc *bbnsm = dev_get_drvdata(dev);
+	unsigned long time;
+
+	time = bbnsm_read_counter(bbnsm);
+	rtc_time64_to_tm(time, tm);
+
+	return 0;
+}
+
+static int bbnsm_rtc_set_time(struct device *dev, struct rtc_time *tm)
+{
+	struct bbnsm_rtc *bbnsm = dev_get_drvdata(dev);
+	unsigned long time = rtc_tm_to_time64(tm);
+
+	/* disable the RTC first */
+	regmap_update_bits(bbnsm->regmap, BBNSM_CTRL, RTC_EN_MSK, 0);
+
+	/* write the 32bit sec time to 47 bit timer counter, leaving 15 LSBs blank */
+	regmap_write(bbnsm->regmap, BBNSM_RTC_LS, time << CNTR_TO_SECS_SH);
+	regmap_write(bbnsm->regmap, BBNSM_RTC_MS, time >> (32 - CNTR_TO_SECS_SH));
+
+	/* Enable the RTC again */
+	regmap_update_bits(bbnsm->regmap, BBNSM_CTRL, RTC_EN_MSK, RTC_EN);
+
+	return 0;
+}
+
+static int bbnsm_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
+{
+	struct bbnsm_rtc *bbnsm = dev_get_drvdata(dev);
+	u32 bbnsm_events, bbnsm_ta;
+
+	regmap_read(bbnsm->regmap, BBNSM_TA, &bbnsm_ta);
+	rtc_time64_to_tm(bbnsm_ta, &alrm->time);
+
+	regmap_read(bbnsm->regmap, BBNSM_EVENTS, &bbnsm_events);
+	alrm->pending = (bbnsm_events & BBNSM_EVENT_TA) ? 1 : 0;
+
+	return 0;
+}
+
+static int bbnsm_rtc_alarm_irq_enable(struct device *dev, unsigned int enable)
+{
+	struct bbnsm_rtc *bbnsm = dev_get_drvdata(dev);
+
+	/* enable the alarm event */
+	regmap_update_bits(bbnsm->regmap, BBNSM_CTRL, TA_EN_MSK, enable ? TA_EN : TA_DIS);
+	/* enable the alarm interrupt */
+	regmap_update_bits(bbnsm->regmap, BBNSM_INT_EN, TA_EN_MSK, enable ? TA_EN : TA_DIS);
+
+	return 0;
+}
+
+static int bbnsm_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
+{
+	struct bbnsm_rtc *bbnsm = dev_get_drvdata(dev);
+	unsigned long time = rtc_tm_to_time64(&alrm->time);
+
+	/* disable the alarm */
+	regmap_update_bits(bbnsm->regmap, BBNSM_CTRL, TA_EN, TA_EN);
+
+	/* write the seconds to TA */
+	regmap_write(bbnsm->regmap, BBNSM_TA, time);
+
+	return bbnsm_rtc_alarm_irq_enable(dev, alrm->enabled);
+}
+
+static const struct rtc_class_ops bbnsm_rtc_ops = {
+	.read_time = bbnsm_rtc_read_time,
+	.set_time = bbnsm_rtc_set_time,
+	.read_alarm = bbnsm_rtc_read_alarm,
+	.set_alarm = bbnsm_rtc_set_alarm,
+	.alarm_irq_enable = bbnsm_rtc_alarm_irq_enable,
+};
+
+static irqreturn_t bbnsm_rtc_irq_handler(int irq, void *dev_id)
+{
+	struct device *dev = dev_id;
+	struct bbnsm_rtc  *bbnsm = dev_get_drvdata(dev);
+	u32 val;
+	u32 event = 0;
+
+	regmap_read(bbnsm->regmap, BBNSM_EVENTS, &val);
+	if (val & BBNSM_EVENT_TA) {
+		event |= (RTC_AF | RTC_IRQF);
+		bbnsm_rtc_alarm_irq_enable(dev, false);
+		/* clear the alarm event */
+		regmap_write_bits(bbnsm->regmap, BBNSM_EVENTS, TA_EN_MSK, BBNSM_EVENT_TA);
+		rtc_update_irq(bbnsm->rtc, 1, event);
+	}
+
+	return event ? IRQ_HANDLED : IRQ_NONE;
+}
+
+static int bbnsm_rtc_probe(struct platform_device *pdev)
+{
+	struct bbnsm_rtc *bbnsm;
+	int ret;
+
+	bbnsm = devm_kzalloc(&pdev->dev, sizeof(*bbnsm), GFP_KERNEL);
+	if (!bbnsm)
+		return -ENOMEM;
+
+	bbnsm->rtc = devm_rtc_allocate_device(&pdev->dev);
+
+	bbnsm->regmap = syscon_regmap_lookup_by_phandle(pdev->dev.of_node, "regmap");
+	if (IS_ERR(bbnsm->regmap)) {
+		dev_err(&pdev->dev, "bbnsm get regmap failed\n");
+		return PTR_ERR(bbnsm->regmap);
+	}
+
+	bbnsm->irq = platform_get_irq(pdev, 0);
+	if (bbnsm->irq < 0)
+		return bbnsm->irq;
+
+	platform_set_drvdata(pdev, bbnsm);
+
+	/* clear all the pending events */
+	regmap_write(bbnsm->regmap, BBNSM_EVENTS, 0x7A);
+
+	/* Enable the Real-Time counter */
+	regmap_update_bits(bbnsm->regmap, BBNSM_CTRL, RTC_EN_MSK, RTC_EN);
+
+	device_init_wakeup(&pdev->dev, true);
+	ret = dev_pm_set_wake_irq(&pdev->dev, bbnsm->irq);
+	if (ret)
+		dev_err(&pdev->dev, "Failed to enable the irq wakeup\n");
+
+	ret = devm_request_irq(&pdev->dev, bbnsm->irq, bbnsm_rtc_irq_handler,
+			IRQF_SHARED, "rtc alarm", &pdev->dev);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to request irq %d: %d\n",
+			bbnsm->irq, ret);
+		return ret;
+	}
+
+	bbnsm->rtc->ops = &bbnsm_rtc_ops;
+	bbnsm->rtc->range_max = U32_MAX;
+
+	return devm_rtc_register_device(bbnsm->rtc);
+}
+
+static const struct of_device_id bbnsm_dt_ids[] = {
+	{ .compatible = "nxp,bbnsm-rtc", },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, bbnsm_dt_ids);
+
+static struct platform_driver bbnsm_rtc_driver = {
+	.driver = {
+		.name = "bbnsm_rtc",
+		.of_match_table = bbnsm_dt_ids,
+	},
+	.probe = bbnsm_rtc_probe,
+};
+module_platform_driver(bbnsm_rtc_driver);
+
+MODULE_AUTHOR("Jacky Bai <ping.bai@nxp.com>");
+MODULE_DESCRIPTION("NXP BBNSM RTC Driver");
+MODULE_LICENSE("GPL");
-- 
2.37.1

Re: [PATCH 3/4] rtc: bbnsm: Add the bbnsm rtc support

[Alexandre Belloni]

On 21/11/2022 14:51:43+0800, Jacky Bai wrote:
> The BBNSM module includes a real time counter with alarm.
> Add a RTC driver for this function.
> 
> Signed-off-by: Jacky Bai <ping.bai@nxp.com>
> Reviewed-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/rtc/Kconfig     |  12 +++
>  drivers/rtc/Makefile    |   1 +
>  drivers/rtc/rtc-bbnsm.c | 223 ++++++++++++++++++++++++++++++++++++++++

I'd prefer that filename to include imx or mxc if this is only available
on those SoCs.

> diff --git a/drivers/rtc/rtc-bbnsm.c b/drivers/rtc/rtc-bbnsm.c
> new file mode 100644
> index 000000000000..4157b238ed9a
> --- /dev/null
> +++ b/drivers/rtc/rtc-bbnsm.c
> @@ -0,0 +1,223 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +//
> +// Copyright 2022 NXP.
> +
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_wakeirq.h>
> +#include <linux/rtc.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/regmap.h>

This list should be sorted alphabetically


> +
> +#define BBNSM_CTRL	0x8
> +#define BBNSM_INT_EN	0x10
> +#define BBNSM_EVENTS	0x14
> +#define BBNSM_RTC_LS	0x40
> +#define BBNSM_RTC_MS	0x44
> +#define BBNSM_TA	0x50
> +
> +#define RTC_EN		0x2
> +#define RTC_EN_MSK	0x3
> +#define TA_EN		(0x2 << 2)
> +#define TA_DIS		(0x1 << 2)
> +#define TA_EN_MSK	(0x3 << 2)
> +#define RTC_INT_EN	0x2
> +#define TA_INT_EN	(0x2 << 2)
> +
> +#define BBNSM_EVENT_TA	TA_EN
> +

I'm not clear why this define is needed

> +static irqreturn_t bbnsm_rtc_irq_handler(int irq, void *dev_id)
> +{
> +	struct device *dev = dev_id;
> +	struct bbnsm_rtc  *bbnsm = dev_get_drvdata(dev);
> +	u32 val;
> +	u32 event = 0;

You can rework this function to remove this variable because it is
either 0 or RTC_AF | RTC_IRQF

> +
> +	regmap_read(bbnsm->regmap, BBNSM_EVENTS, &val);
> +	if (val & BBNSM_EVENT_TA) {
> +		event |= (RTC_AF | RTC_IRQF);
> +		bbnsm_rtc_alarm_irq_enable(dev, false);
> +		/* clear the alarm event */
> +		regmap_write_bits(bbnsm->regmap, BBNSM_EVENTS, TA_EN_MSK, BBNSM_EVENT_TA);
> +		rtc_update_irq(bbnsm->rtc, 1, event);
> +	}
> +
> +	return event ? IRQ_HANDLED : IRQ_NONE;
> +}
> +
> +static int bbnsm_rtc_probe(struct platform_device *pdev)
> +{
> +	struct bbnsm_rtc *bbnsm;
> +	int ret;
> +
> +	bbnsm = devm_kzalloc(&pdev->dev, sizeof(*bbnsm), GFP_KERNEL);
> +	if (!bbnsm)
> +		return -ENOMEM;
> +
> +	bbnsm->rtc = devm_rtc_allocate_device(&pdev->dev);
> +
> +	bbnsm->regmap = syscon_regmap_lookup_by_phandle(pdev->dev.of_node, "regmap");
> +	if (IS_ERR(bbnsm->regmap)) {
> +		dev_err(&pdev->dev, "bbnsm get regmap failed\n");
> +		return PTR_ERR(bbnsm->regmap);
> +	}
> +
> +	bbnsm->irq = platform_get_irq(pdev, 0);
> +	if (bbnsm->irq < 0)
> +		return bbnsm->irq;
> +
> +	platform_set_drvdata(pdev, bbnsm);
> +
> +	/* clear all the pending events */
> +	regmap_write(bbnsm->regmap, BBNSM_EVENTS, 0x7A);
> +
> +	/* Enable the Real-Time counter */
> +	regmap_update_bits(bbnsm->regmap, BBNSM_CTRL, RTC_EN_MSK, RTC_EN);
> +

Please don't do that, this removes an important piece of information.
Instead, let .set_time enable it and check it in .read_time as if this
is not set, you now you are out of PoR and the time is invalid

> +	device_init_wakeup(&pdev->dev, true);
> +	ret = dev_pm_set_wake_irq(&pdev->dev, bbnsm->irq);
> +	if (ret)
> +		dev_err(&pdev->dev, "Failed to enable the irq wakeup\n");

dev_err but the function is not failing. Maybe just dev_warn? Also, is
this message really necessary?

> +
> +	ret = devm_request_irq(&pdev->dev, bbnsm->irq, bbnsm_rtc_irq_handler,
> +			IRQF_SHARED, "rtc alarm", &pdev->dev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to request irq %d: %d\n",
> +			bbnsm->irq, ret);
> +		return ret;
> +	}
> +
> +	bbnsm->rtc->ops = &bbnsm_rtc_ops;
> +	bbnsm->rtc->range_max = U32_MAX;
> +
> +	return devm_rtc_register_device(bbnsm->rtc);
> +}
> +
> +static const struct of_device_id bbnsm_dt_ids[] = {
> +	{ .compatible = "nxp,bbnsm-rtc", },
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, bbnsm_dt_ids);
> +
> +static struct platform_driver bbnsm_rtc_driver = {
> +	.driver = {
> +		.name = "bbnsm_rtc",
> +		.of_match_table = bbnsm_dt_ids,
> +	},
> +	.probe = bbnsm_rtc_probe,
> +};
> +module_platform_driver(bbnsm_rtc_driver);
> +
> +MODULE_AUTHOR("Jacky Bai <ping.bai@nxp.com>");
> +MODULE_DESCRIPTION("NXP BBNSM RTC Driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.37.1
> 

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

RE: [PATCH 3/4] rtc: bbnsm: Add the bbnsm rtc support

[Jacky Bai]

> Subject: Re: [PATCH 3/4] rtc: bbnsm: Add the bbnsm rtc support
> 
> On 21/11/2022 14:51:43+0800, Jacky Bai wrote:
> > The BBNSM module includes a real time counter with alarm.
> > Add a RTC driver for this function.
> >
> > Signed-off-by: Jacky Bai <ping.bai@nxp.com>
> > Reviewed-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  drivers/rtc/Kconfig     |  12 +++
> >  drivers/rtc/Makefile    |   1 +
> >  drivers/rtc/rtc-bbnsm.c | 223
> > ++++++++++++++++++++++++++++++++++++++++
> 
> I'd prefer that filename to include imx or mxc if this is only available on those
> SoCs.
>

For now, it is used on i.MX SoC, not sure if it will be used on NXP other SoC. I will
update the file name to include 'imx' in v2.

> > diff --git a/drivers/rtc/rtc-bbnsm.c b/drivers/rtc/rtc-bbnsm.c new
> > file mode 100644 index 000000000000..4157b238ed9a
> > --- /dev/null
> > +++ b/drivers/rtc/rtc-bbnsm.c
> > @@ -0,0 +1,223 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +//
> > +// Copyright 2022 NXP.
> > +
> > +#include <linux/init.h>
> > +#include <linux/io.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm_wakeirq.h>
> > +#include <linux/rtc.h>
> > +#include <linux/mfd/syscon.h>
> > +#include <linux/regmap.h>
> 
> This list should be sorted alphabetically
> 

Thx, will fix it in V2.

> 
> > +
> > +#define BBNSM_CTRL	0x8
> > +#define BBNSM_INT_EN	0x10
> > +#define BBNSM_EVENTS	0x14
> > +#define BBNSM_RTC_LS	0x40
> > +#define BBNSM_RTC_MS	0x44
> > +#define BBNSM_TA	0x50
> > +
> > +#define RTC_EN		0x2
> > +#define RTC_EN_MSK	0x3
> > +#define TA_EN		(0x2 << 2)
> > +#define TA_DIS		(0x1 << 2)
> > +#define TA_EN_MSK	(0x3 << 2)
> > +#define RTC_INT_EN	0x2
> > +#define TA_INT_EN	(0x2 << 2)
> > +
> > +#define BBNSM_EVENT_TA	TA_EN
> > +
> 
> I'm not clear why this define is needed

This define is for RTC alarm event status check, as it use the same bitfield offset as TA_EN,
I just did the above define. It seems introduce some unnecessary confusion. 

> 
> > +static irqreturn_t bbnsm_rtc_irq_handler(int irq, void *dev_id) {
> > +	struct device *dev = dev_id;
> > +	struct bbnsm_rtc  *bbnsm = dev_get_drvdata(dev);
> > +	u32 val;
> > +	u32 event = 0;
> 
> You can rework this function to remove this variable because it is either 0 or
> RTC_AF | RTC_IRQF
> 

Ok, will refine in V2.

> > +
> > +	regmap_read(bbnsm->regmap, BBNSM_EVENTS, &val);
> > +	if (val & BBNSM_EVENT_TA) {
> > +		event |= (RTC_AF | RTC_IRQF);
> > +		bbnsm_rtc_alarm_irq_enable(dev, false);
> > +		/* clear the alarm event */
> > +		regmap_write_bits(bbnsm->regmap, BBNSM_EVENTS, TA_EN_MSK,
> BBNSM_EVENT_TA);
> > +		rtc_update_irq(bbnsm->rtc, 1, event);
> > +	}
> > +
> > +	return event ? IRQ_HANDLED : IRQ_NONE; }
> > +
> > +static int bbnsm_rtc_probe(struct platform_device *pdev) {
> > +	struct bbnsm_rtc *bbnsm;
> > +	int ret;
> > +
> > +	bbnsm = devm_kzalloc(&pdev->dev, sizeof(*bbnsm), GFP_KERNEL);
> > +	if (!bbnsm)
> > +		return -ENOMEM;
> > +
> > +	bbnsm->rtc = devm_rtc_allocate_device(&pdev->dev);
> > +
> > +	bbnsm->regmap =
> syscon_regmap_lookup_by_phandle(pdev->dev.of_node, "regmap");
> > +	if (IS_ERR(bbnsm->regmap)) {
> > +		dev_err(&pdev->dev, "bbnsm get regmap failed\n");
> > +		return PTR_ERR(bbnsm->regmap);
> > +	}
> > +
> > +	bbnsm->irq = platform_get_irq(pdev, 0);
> > +	if (bbnsm->irq < 0)
> > +		return bbnsm->irq;
> > +
> > +	platform_set_drvdata(pdev, bbnsm);
> > +
> > +	/* clear all the pending events */
> > +	regmap_write(bbnsm->regmap, BBNSM_EVENTS, 0x7A);
> > +
> > +	/* Enable the Real-Time counter */
> > +	regmap_update_bits(bbnsm->regmap, BBNSM_CTRL, RTC_EN_MSK,
> RTC_EN);
> > +
> 
> Please don't do that, this removes an important piece of information.
> Instead, let .set_time enable it and check it in .read_time as if this is not set,
> you now you are out of PoR and the time is invalid
> 

Will remove it. set_time has the enable operation. I will add enable check in read_time callback.

> > +	device_init_wakeup(&pdev->dev, true);
> > +	ret = dev_pm_set_wake_irq(&pdev->dev, bbnsm->irq);
> > +	if (ret)
> > +		dev_err(&pdev->dev, "Failed to enable the irq wakeup\n");
> 
> dev_err but the function is not failing. Maybe just dev_warn? Also, is this
> message really necessary?
> 

Not too necessary, I can drop the above log print.

BR

> > +
> > +	ret = devm_request_irq(&pdev->dev, bbnsm->irq,
> bbnsm_rtc_irq_handler,
> > +			IRQF_SHARED, "rtc alarm", &pdev->dev);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "failed to request irq %d: %d\n",
> > +			bbnsm->irq, ret);
> > +		return ret;
> > +	}
> > +
> > +	bbnsm->rtc->ops = &bbnsm_rtc_ops;
> > +	bbnsm->rtc->range_max = U32_MAX;
> > +
> > +	return devm_rtc_register_device(bbnsm->rtc);
> > +}
> > +
> > +static const struct of_device_id bbnsm_dt_ids[] = {
> > +	{ .compatible = "nxp,bbnsm-rtc", },
> > +	{ /* sentinel */ },
> > +};
> > +MODULE_DEVICE_TABLE(of, bbnsm_dt_ids);
> > +
> > +static struct platform_driver bbnsm_rtc_driver = {
> > +	.driver = {
> > +		.name = "bbnsm_rtc",
> > +		.of_match_table = bbnsm_dt_ids,
> > +	},
> > +	.probe = bbnsm_rtc_probe,
> > +};
> > +module_platform_driver(bbnsm_rtc_driver);
> > +
> > +MODULE_AUTHOR("Jacky Bai <ping.bai@nxp.com>");
> > +MODULE_DESCRIPTION("NXP BBNSM RTC Driver");
> MODULE_LICENSE("GPL");
> > --
> > 2.37.1
> >
> 
> --

[PATCH 4/4] arm64: dts: imx93: Add the bbnsm dts node

[Jacky Bai]

Add the bbnsm node for RTC & ON/OFF button support

Signed-off-by: Jacky Bai <ping.bai@nxp.com>
---
 arch/arm64/boot/dts/freescale/imx93.dtsi | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/imx93.dtsi b/arch/arm64/boot/dts/freescale/imx93.dtsi
index 5d79663b3b84..ffc4f46c4820 100644
--- a/arch/arm64/boot/dts/freescale/imx93.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx93.dtsi
@@ -229,6 +229,24 @@ iomuxc: pinctrl@443c0000 {
 				status = "okay";
 			};
 
+			bbnsm: bbnsm@44440000 {
+				compatible = "nxp,bbnsm", "syscon", "simple-mfd";
+				reg = <0x44440000 0x10000>;
+
+				bbnsm_rtc: rtc {
+					compatible = "nxp,bbnsm-rtc";
+					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>;
+				};
+			};
+
 			clk: clock-controller@44450000 {
 				compatible = "fsl,imx93-ccm";
 				reg = <0x44450000 0x10000>;
-- 
2.37.1