Message-ID: <20250923-capitol-easter-d0154d967522@spud>
Date: Tue, 23 Sep 2025 19:57:34 +0100
From: Conor Dooley <conor@...nel.org>
To: Lakshay Piplani <lakshay.piplani@....com>
Cc: alexandre.belloni@...tlin.com, linux-rtc@...r.kernel.org,
	linux-kernel@...r.kernel.org, robh@...nel.org, krzk+dt@...nel.org,
	conor+dt@...nel.org, devicetree@...r.kernel.org,
	pankit.garg@....com, vikash.bansal@....com, priyanka.jain@....com,
	shashank.rebbapragada@....com
Subject: Re: [PATCH v4 1/2] dt-bindings: rtc: Add pcf85053 support

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@....com>
> Signed-off-by: Lakshay Piplani <lakshay.piplani@....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@....com>
> +  - Lakshay Piplani <lakshay.piplani@....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.

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists