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

[Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>]

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@xxxxxxx>
> ---
>  .../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@xxxxxxx>
> +
> +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