[2/2] dt-bindings: rtc: add bindings for max313xx RTCs

  Devicetree binding documentation for Analog Devices MAX313XX RTCs

Signed-off-by: Ibrahim Tilki <Ibrahim.Tilki@analog.com>
Signed-off-by: Zeynep Arslanbenzer <Zeynep.Arslanbenzer@analog.com>
---
 .../devicetree/bindings/rtc/adi,max313xx.yaml | 163 ++++++++++++++++++
 1 file changed, 163 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rtc/adi,max313xx.yaml
  

On 19/10/2022 09:39, Ibrahim Tilki wrote:
> Devicetree binding documentation for Analog Devices MAX313XX RTCs
> 
> Signed-off-by: Ibrahim Tilki <Ibrahim.Tilki@analog.com>
> Signed-off-by: Zeynep Arslanbenzer <Zeynep.Arslanbenzer@analog.com>
> ---
>  .../devicetree/bindings/rtc/adi,max313xx.yaml | 163 ++++++++++++++++++
>  1 file changed, 163 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/rtc/adi,max313xx.yaml
> 

Thank you for your patch. There is something to discuss/improve.

> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        rtc@68 {
> +            reg = <0x68>;
> +            compatible = "adi,max31329";
> +            clocks = <&clkin>;
> +            interrupt-parent = <&gpio>;
> +            interrupts = <26 IRQ_TYPE_EDGE_FALLING>;
> +            interrupt-names = "INTB";
> +        };
> +    };
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        rtc@68 {
> +            reg = <0x68>;
> +            compatible = "adi,max31331";
> +            #clock-cells = <0>;
> +            interrupt-parent = <&gpio>;
> +            interrupts = <25 IRQ_TYPE_EDGE_FALLING>, <26 IRQ_TYPE_EDGE_FALLING>;
> +            interrupt-names = "INTA", "INTB";
> +        };
> +    };
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        rtc@69 {
> +            reg = <0x69>;
> +            compatible = "adi,max31341";
> +            #clock-cells = <0>;
> +            clocks = <&clkin>;


Drop the last example - it's basically the same as first, without
interrupts.

With that:

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof
  

On 19/10/2022 16:39:10+0300, Ibrahim Tilki wrote:
> Devicetree binding documentation for Analog Devices MAX313XX RTCs
> 
> Signed-off-by: Ibrahim Tilki <Ibrahim.Tilki@analog.com>
> Signed-off-by: Zeynep Arslanbenzer <Zeynep.Arslanbenzer@analog.com>
> ---
>  .../devicetree/bindings/rtc/adi,max313xx.yaml | 163 ++++++++++++++++++
>  1 file changed, 163 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/rtc/adi,max313xx.yaml
> 
> diff --git a/Documentation/devicetree/bindings/rtc/adi,max313xx.yaml b/Documentation/devicetree/bindings/rtc/adi,max313xx.yaml
> new file mode 100644
> index 000000000..1aa491799
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/rtc/adi,max313xx.yaml
> @@ -0,0 +1,163 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright 2022 Analog Devices Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/rtc/adi,max313xx.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices MAX313XX series I2C RTC driver
> +
> +maintainers:
> +  - Ibrahim Tilki <Ibrahim.Tilki@analog.com>
> +  - Zeynep Arslanbenzer <Zeynep.Arslanbenzer@analog.com>
> +
> +description: Bindings for the Analog Devices MAX313XX series RTCs.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,max31328
> +      - adi,max31329
> +      - adi,max31331
> +      - adi,max31334
> +      - adi,max31341
> +      - adi,max31342
> +      - adi,max31343
> +
> +  reg:
> +    description: I2C address of the RTC
> +    items:
> +      - enum: [0x68, 0x69]
> +
> +  interrupts:
> +    minItems: 1
> +    maxItems: 2
> +
> +  interrupt-names:
> +    description: |
> +      Name of the interrupt pin of the RTC used for IRQ. Not required for
> +      RTCs that only have single interrupt pin available. Some of the RTCs
> +      share interrupt pins with clock input/output pins.
> +    minItems: 1
> +    items:
> +      - enum: [INTA, INTB]
> +      - enum: [INTA, INTB]
> +

I don't think this is right, what this is doing is essentially pinmuxing
interrupts versus clocks. What happens if you want INTB but this goes
directly to a PMIC instead of the SoC?
It is not something you can express with your current bindings.


> +  "#clock-cells":
> +    description: |
> +      RTC can be used as a clock source through its clock output pin when
> +      supplied.
> +    const: 0
> +
> +  clocks:
> +    description: |
> +      RTC uses this clock for clock input when supplied. Clock has to provide
> +      one of these four frequencies: 1Hz, 50Hz, 60Hz or 32.768kHz.
> +    maxItems: 1
> +
> +  trickle-diode-disable: true
> +
> +  trickle-resistor-ohms:
> +    description: Enables trickle charger with specified resistor value.
> +    enum: [3000, 6000, 11000]
> +
> +  wakeup-source: true
> +
> +additionalProperties: false
> +
> +allOf:
> +  - $ref: rtc.yaml#
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - adi,max31328
> +              - adi,max31342
> +
> +    then:
> +      properties:
> +        trickle-diode-disable: false
> +        trickle-resistor-ohms: false
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - adi,max31328
> +              - adi,max31331
> +              - adi,max31334
> +              - adi,max31343
> +
> +    then:
> +      properties:
> +        clocks: false
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - adi,max31341
> +              - adi,max31342
> +
> +    then:
> +      properties:
> +        reg:
> +          items:
> +            - const: 0x69
> +
> +    else:
> +      properties:
> +        reg:
> +          items:
> +            - const: 0x68
> +
> +required:
> +  - compatible
> +  - reg
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        rtc@68 {
> +            reg = <0x68>;
> +            compatible = "adi,max31329";
> +            clocks = <&clkin>;
> +            interrupt-parent = <&gpio>;
> +            interrupts = <26 IRQ_TYPE_EDGE_FALLING>;
> +            interrupt-names = "INTB";
> +        };
> +    };
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        rtc@68 {
> +            reg = <0x68>;
> +            compatible = "adi,max31331";
> +            #clock-cells = <0>;
> +            interrupt-parent = <&gpio>;
> +            interrupts = <25 IRQ_TYPE_EDGE_FALLING>, <26 IRQ_TYPE_EDGE_FALLING>;
> +            interrupt-names = "INTA", "INTB";
> +        };
> +    };
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        rtc@69 {
> +            reg = <0x69>;
> +            compatible = "adi,max31341";
> +            #clock-cells = <0>;
> +            clocks = <&clkin>;
> +        };
> +    };
> -- 
> 2.25.1
>
  

> On 19/10/2022 16:39:10+0300, Ibrahim Tilki wrote:
> > Devicetree binding documentation for Analog Devices MAX313XX RTCs
> > 
> > Signed-off-by: Ibrahim Tilki <Ibrahim.Tilki@analog.com>
> > Signed-off-by: Zeynep Arslanbenzer <Zeynep.Arslanbenzer@analog.com>
> > ---
> > .../devicetree/bindings/rtc/adi,max313xx.yaml | 163 ++++++++++++++++++
> > 1 file changed, 163 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/rtc/adi,max313xx.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/rtc/adi,max313xx.yaml \
> > b/Documentation/devicetree/bindings/rtc/adi,max313xx.yaml new file mode 100644
> > index 000000000..1aa491799
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/rtc/adi,max313xx.yaml
> > @@ -0,0 +1,163 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +# Copyright 2022 Analog Devices Inc.
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/rtc/adi,max313xx.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Analog Devices MAX313XX series I2C RTC driver
> > +
> > +maintainers:
> > +  - Ibrahim Tilki <Ibrahim.Tilki@analog.com>
> > +  - Zeynep Arslanbenzer <Zeynep.Arslanbenzer@analog.com>
> > +
> > +description: Bindings for the Analog Devices MAX313XX series RTCs.
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - adi,max31328
> > +      - adi,max31329
> > +      - adi,max31331
> > +      - adi,max31334
> > +      - adi,max31341
> > +      - adi,max31342
> > +      - adi,max31343
> > +
> > +  reg:
> > +    description: I2C address of the RTC
> > +    items:
> > +      - enum: [0x68, 0x69]
> > +
> > +  interrupts:
> > +    minItems: 1
> > +    maxItems: 2
> > +
> > +  interrupt-names:
> > +    description: |
> > +      Name of the interrupt pin of the RTC used for IRQ. Not required for
> > +      RTCs that only have single interrupt pin available. Some of the RTCs
> > +      share interrupt pins with clock input/output pins.
> > +    minItems: 1
> > +    items:
> > +      - enum: [INTA, INTB]
> > +      - enum: [INTA, INTB]
> > +
> 
> I don't think this is right, what this is doing is essentially pinmuxing
> interrupts versus clocks. What happens if you want INTB but this goes
> directly to a PMIC instead of the SoC?
> It is not something you can express with your current bindings.
> 

Why would a user want INTB when it is not connected to SoC?
User can specify none, either one or both of the interrupt pins.
I don't see what the problem here is.

Ibrahim

> 
> > +  "#clock-cells":
> > +    description: |
> > +      RTC can be used as a clock source through its clock output pin when
> > +      supplied.
> > +    const: 0
> > +
> > +  clocks:
> > +    description: |
> > +      RTC uses this clock for clock input when supplied. Clock has to provide
> > +      one of these four frequencies: 1Hz, 50Hz, 60Hz or 32.768kHz.
> > +    maxItems: 1
> > +
> > +  trickle-diode-disable: true
> > +
> > +  trickle-resistor-ohms:
> > +    description: Enables trickle charger with specified resistor value.
> > +    enum: [3000, 6000, 11000]
> > +
> > +  wakeup-source: true
> > +
> > +additionalProperties: false
> > +
> > +allOf:
> > +  - $ref: rtc.yaml#
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            enum:
> > +              - adi,max31328
> > +              - adi,max31342
> > +
> > +    then:
> > +      properties:
> > +        trickle-diode-disable: false
> > +        trickle-resistor-ohms: false
> > +
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            enum:
> > +              - adi,max31328
> > +              - adi,max31331
> > +              - adi,max31334
> > +              - adi,max31343
> > +
> > +    then:
> > +      properties:
> > +        clocks: false
> > +
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            enum:
> > +              - adi,max31341
> > +              - adi,max31342
> > +
> > +    then:
> > +      properties:
> > +        reg:
> > +          items:
> > +            - const: 0x69
> > +
> > +    else:
> > +      properties:
> > +        reg:
> > +          items:
> > +            - const: 0x68
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +    i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        rtc@68 {
> > +            reg = <0x68>;
> > +            compatible = "adi,max31329";
> > +            clocks = <&clkin>;
> > +            interrupt-parent = <&gpio>;
> > +            interrupts = <26 IRQ_TYPE_EDGE_FALLING>;
> > +            interrupt-names = "INTB";
> > +        };
> > +    };
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +    i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        rtc@68 {
> > +            reg = <0x68>;
> > +            compatible = "adi,max31331";
> > +            #clock-cells = <0>;
> > +            interrupt-parent = <&gpio>;
> > +            interrupts = <25 IRQ_TYPE_EDGE_FALLING>, <26 IRQ_TYPE_EDGE_FALLING>;
> > +            interrupt-names = "INTA", "INTB";
> > +        };
> > +    };
> > +  - |
> > +    i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        rtc@69 {
> > +            reg = <0x69>;
> > +            compatible = "adi,max31341";
> > +            #clock-cells = <0>;
> > +            clocks = <&clkin>;
> > +        };
> > +    };
> > -- 
> > 2.25.1
> > 
> 
> -- 
> Alexandre Belloni, co-owner and COO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
  

On 21/10/2022 11:44:12+0000, Tilki, Ibrahim wrote:
> > > +  interrupt-names:
> > > +    description: |
> > > +      Name of the interrupt pin of the RTC used for IRQ. Not required for
> > > +      RTCs that only have single interrupt pin available. Some of the RTCs
> > > +      share interrupt pins with clock input/output pins.
> > > +    minItems: 1
> > > +    items:
> > > +      - enum: [INTA, INTB]
> > > +      - enum: [INTA, INTB]
> > > +
> > 
> > I don't think this is right, what this is doing is essentially pinmuxing
> > interrupts versus clocks. What happens if you want INTB but this goes
> > directly to a PMIC instead of the SoC?
> > It is not something you can express with your current bindings.
> > 
> 
> Why would a user want INTB when it is not connected to SoC?
> User can specify none, either one or both of the interrupt pins.
> I don't see what the problem here is.
> 

the interrupt pin may be connected to a PMIC that is able to start or
wake up the platform. In that case, the user would not have any
interrupt-names and your driver will fail to mux the interrupt on INTB.
Please fix.
  

> On 21/10/2022 11:44:12+0000, Tilki, Ibrahim wrote:
> > > > +  interrupt-names:
> > > > +    description: |
> > > > +      Name of the interrupt pin of the RTC used for IRQ. Not required for
> > > > +      RTCs that only have single interrupt pin available. Some of the RTCs
> > > > +      share interrupt pins with clock input/output pins.
> > > > +    minItems: 1
> > > > +    items:
> > > > +      - enum: [INTA, INTB]
> > > > +      - enum: [INTA, INTB]
> > > > +
> > > 
> > > I don't think this is right, what this is doing is essentially pinmuxing
> > > interrupts versus clocks. What happens if you want INTB but this goes
> > > directly to a PMIC instead of the SoC?
> > > It is not something you can express with your current bindings.
> > > 
> > 
> > Why would a user want INTB when it is not connected to SoC?
> > User can specify none, either one or both of the interrupt pins.
> > I don't see what the problem here is.
> > 
> 
> the interrupt pin may be connected to a PMIC that is able to start or
> wake up the platform. In that case, the user would not have any
> interrupt-names and your driver will fail to mux the interrupt on INTB.
> Please fix.

Interrupt muxing depends on the clock configuration, not the interrupt-names property.
Devices don't support muxing the alarm interrupt independently.

For example in the case of max31329, alarm interrupt is muxed into INTA by default.
Alarm interrupt is muxed into INTB pin if and only if clkin is enabled.

This means that if a user wants the alarm interrupt on INTB pin, they have to
provide a clock input through "clocks" property. If that is the case, they can provide
an interrupt for INTB pin, otherwise the alarm feature of the rtc gets disabled.

Side note: Some devices have 2 Alarms but Alarm2 does not have a register for
matching "seconds" so only Alarm1 is used by the driver.

Ibrahim
  

On 21/10/2022 09:05, Tilki, Ibrahim wrote:
>> On 21/10/2022 11:44:12+0000, Tilki, Ibrahim wrote:
>>>>> +  interrupt-names:
>>>>> +    description: |
>>>>> +      Name of the interrupt pin of the RTC used for IRQ. Not required for
>>>>> +      RTCs that only have single interrupt pin available. Some of the RTCs
>>>>> +      share interrupt pins with clock input/output pins.
>>>>> +    minItems: 1
>>>>> +    items:
>>>>> +      - enum: [INTA, INTB]
>>>>> +      - enum: [INTA, INTB]
>>>>> +
>>>>
>>>> I don't think this is right, what this is doing is essentially pinmuxing
>>>> interrupts versus clocks. What happens if you want INTB but this goes
>>>> directly to a PMIC instead of the SoC?
>>>> It is not something you can express with your current bindings.
>>>>
>>>
>>> Why would a user want INTB when it is not connected to SoC?
>>> User can specify none, either one or both of the interrupt pins.
>>> I don't see what the problem here is.
>>>
>>
>> the interrupt pin may be connected to a PMIC that is able to start or
>> wake up the platform. In that case, the user would not have any
>> interrupt-names and your driver will fail to mux the interrupt on INTB.
>> Please fix.
> 
> Interrupt muxing depends on the clock configuration, not the interrupt-names property.
> Devices don't support muxing the alarm interrupt independently.
> 
> For example in the case of max31329, alarm interrupt is muxed into INTA by default.
> Alarm interrupt is muxed into INTB pin if and only if clkin is enabled.\

Just to be sure: are you now describing hardware or Linux driver behavior?

> 
> This means that if a user wants the alarm interrupt on INTB pin, they have to
> provide a clock input through "clocks" property. If that is the case, they can provide
> an interrupt for INTB pin, otherwise the alarm feature of the rtc gets disabled.
> 
> Side note: Some devices have 2 Alarms but Alarm2 does not have a register for
> matching "seconds" so only Alarm1 is used by the driver.

Best regards,
Krzysztof
  

> On 21/10/2022 09:05, Tilki, Ibrahim wrote:
> >> On 21/10/2022 11:44:12+0000, Tilki, Ibrahim wrote:
> >>>>> +  interrupt-names:
> >>>>> +    description: |
> >>>>> +      Name of the interrupt pin of the RTC used for IRQ. Not required for
> >>>>> +      RTCs that only have single interrupt pin available. Some of the RTCs
> >>>>> +      share interrupt pins with clock input/output pins.
> >>>>> +    minItems: 1
> >>>>> +    items:
> >>>>> +      - enum: [INTA, INTB]
> >>>>> +      - enum: [INTA, INTB]
> >>>>> +
> >>>>
> >>>> I don't think this is right, what this is doing is essentially 
> >>>> pinmuxing interrupts versus clocks. What happens if you want INTB 
> >>>> but this goes directly to a PMIC instead of the SoC?
> >>>> It is not something you can express with your current bindings.
> >>>>
> >>>
> >>> Why would a user want INTB when it is not connected to SoC?
> >>> User can specify none, either one or both of the interrupt pins.
> >>> I don't see what the problem here is.
> >>>
> >>
> >> the interrupt pin may be connected to a PMIC that is able to start or 
> >> wake up the platform. In that case, the user would not have any 
> >> interrupt-names and your driver will fail to mux the interrupt on INTB.
> >> Please fix.
> > 
> > Interrupt muxing depends on the clock configuration, not the interrupt-names property.
> > Devices don't support muxing the alarm interrupt independently.
> > 
> > For example in the case of max31329, alarm interrupt is muxed into INTA by default.
> > Alarm interrupt is muxed into INTB pin if and only if clkin is 
> > enabled.\
> 
> Just to be sure: are you now describing hardware or Linux driver behavior?
> 

That is hardware behavior.

Below is Interrupt Modes Table from max31329 datasheet:
+--------+---------+------------------------------------------+-----------------------------------------+
| ENCLKO | ENCLKIN |               INTA/CLKIN                 |               INTB/CLKOUT               |
+--------+---------+------------------------------------------+-----------------------------------------+
|      0 |       0 | INTA: Alarm1, Timer, PFAIL, DIN          | INTB: Alarm2                            |
|      0 |       1 | CLKIN                                    | INTB: Alarm1, Alarm2, Timer, PFAIL, DIN |
|      1 |       0 | INTA: Alarm1, Alarm2, Timer, PFAIL, DIN  | CLKOUT                                  |
|      1 |       1 | CLKIN                                    | CLKOUT                                  |
+--------+---------+------------------------------------------+-----------------------------------------+

> > 
> > This means that if a user wants the alarm interrupt on INTB pin, they 
> > have to provide a clock input through "clocks" property. If that is 
> > the case, they can provide an interrupt for INTB pin, otherwise the alarm feature of the rtc gets disabled.
> > 
> > Side note: Some devices have 2 Alarms but Alarm2 does not have a 
> > register for matching "seconds" so only Alarm1 is used by the driver.


Regards,
Ibrahim