Re: [PATCH v4 1/2] dt-bindings: rtc: Add pcf85053 support

[Conor Dooley <conor@kernel.org>]

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

On Tue, Sep 23, 2025 at 05:04:40PM +0530, Lakshay Piplani wrote:
> Add device tree bindings for NXP PCF85053 RTC chip.
> 
> Signed-off-by: Pankit Garg <pankit.garg@nxp.com>
> Signed-off-by: Lakshay Piplani <lakshay.piplani@nxp.com>
> ---
> V3 -> V4: Add dedicated nxp,pcf85053.yaml.
>           Remove entry from trivial-rtc.yaml.
> V2 -> V3: Moved MAINTAINERS file changes to the driver patch
> V1 -> V2: Handled dt-bindings by trivial-rtc.yaml
> 
>  .../devicetree/bindings/rtc/nxp,pcf85053.yaml | 128 ++++++++++++++++++
>  1 file changed, 128 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/rtc/nxp,pcf85053.yaml
> 
> diff --git a/Documentation/devicetree/bindings/rtc/nxp,pcf85053.yaml b/Documentation/devicetree/bindings/rtc/nxp,pcf85053.yaml
> new file mode 100644
> index 000000000000..6b1c97358486
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/rtc/nxp,pcf85053.yaml
> @@ -0,0 +1,128 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright 2025 NXP
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/rtc/nxp,pcf85053.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NXP PCF85053 Real Time Clock
> +
> +maintainers:
> +  - Pankit Garg <pankit.garg@nxp.com>
> +  - Lakshay Piplani <lakshay.piplani@nxp.com>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - nxp,pcf85053
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  nxp,interface:
> +    $ref: /schemas/types.yaml#/definitions/string
> +    enum: [ primary, secondary ]
> +    description: |
> +      Identifies this host's logical role in a multi-host topology for the
> +      PCF85053 RTC. The device exposes a "TWO" ownership bit in the CTRL
> +      register that gates which host may write time/alarm registers.
> +        - "primary": Designated host that *may* claim write ownership (set
> +          CTRL.TWO=1) **if** write-access is explicitly requested.
> +        - "secondary": Peer host that writes only when CTRL.TWO=0 (default).
> +
> +  nxp,write-access:
> +    type: boolean
> +    description: |
> +      Request the driver to claim write ownership at probe time by setting
> +      CTRL.TWO=1. This property is only valid when nxp,interface="primary".
> +      The driver will not modify any other CTRL bits (HF/DM/etc.) and will not
> +      clear any status/interrupt flags at probe.
> +
> +required:
> +  - compatible
> +  - reg
> +  - nxp,interface
> +
> +additionalProperties: false
> +
> +# Schema constraints matching driver:
> +# 1) If nxp,write-access is present, nxp,interface must be "primary".
> +#    Rationale: only the primary may claim ownership; driver will set TWO=1.
> +# 2) If nxp,interface is "secondary", nxp,write-access must not be present.
> +#    Rationale: secondary never claims ownership and cannot write CTRL/ST/alarm.
> +#
> +# Practical effect:
> +# - Primary without 'nxp,write-access'; primary is read only; secondary may
> +#   write time registers.
> +# - Primary with 'nxp,write-access'; primary owns writes, secondary is read only.
> +allOf:
> +  - $ref: rtc.yaml#
> +  - oneOf:
> +      # Case 1: primary with write-access
> +      - required: [ "nxp,write-access" ]
> +        properties:
> +          nxp,interface:
> +            const: primary
> +
> +      # Case 2: primary without write-access
> +      - properties:
> +          nxp,interface:
> +            const: primary
> +        not:
> +          required: [ "nxp,write-access" ]

Aren't case 1 and case 2 here redundant? All you need to do is block
interface == secondary when nxp,write-access is present, which your case
3 should be able to be modified to do via

if:
  properties:
    nxp,interface:
      const: secondary
then:
  properties:
   nxp,write-access: false

I think your description for nxp,write-access gets the point across
about when it can be used, and the additional commentary is not really
helpful.

> +
> +      # Case 3: secondary (must not have write-access)
> +      - properties:
> +          nxp,interface:
> +            const: secondary
> +        not:
> +          required: [ "nxp,write-access" ]
> +
> +examples:
> +  # Single host example.
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      rtc@6f {
> +        compatible = "nxp,pcf85053";
> +        reg = <0x6f>;
> +        nxp,interface = "primary";
> +        nxp,write-access;
> +        interrupt-parent = <&gpio2>;
> +        interrupts = <3 IRQ_TYPE_EDGE_FALLING>;
> +      };
> +    };
> +
> +  # Dual-host example: one primary that claims writes; one secondary that never claims writes.
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    i2c0 {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      rtc@6f {
> +        compatible = "nxp,pcf85053";
> +        reg = <0x6f>;
> +        nxp,interface = "primary";
> +        nxp,write-access;
> +        interrupt-parent = <&gpio2>;
> +        interrupts = <3 IRQ_TYPE_EDGE_FALLING>;
> +      };
> +    };
> +
> +    i2c1 {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      rtc@6f {
> +        compatible = "nxp,pcf85053";
> +        reg = <0x6f>;
> +        nxp,interface = "secondary";

Maybe a silly question, but if you have a system that wants to have two
pairs of RTCs, how would you determine which primary a secondary belongs
to? I notice you have no link between these devices in dt so I am
curious. Would it be better to eschew nxp,interface and have a phandle
from the secondary to the primary?

I don't know anything about your use case or features, so maybe knowing
the relationship just is not relevant at all, or it can be determined at
runtime.

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]