diff mbox series

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

Message ID 20240227010312.3305966-3-chris.packham@alliedtelesis.co.nz
State Changes Requested
Headers show
Series drivers: rtc: add max313xx series rtc driver | expand

Commit Message

Chris Packham Feb. 27, 2024, 1:03 a.m. UTC 
From: Ibrahim Tilki <Ibrahim.Tilki@analog.com>

Add devicetree binding documentation for Analog Devices MAX313XX RTCs.
This combines the new models with the existing max31335 binding.

Signed-off-by: Ibrahim Tilki <Ibrahim.Tilki@analog.com>
Signed-off-by: Zeynep Arslanbenzer <Zeynep.Arslanbenzer@analog.com>
Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
 .../devicetree/bindings/rtc/adi,max31335.yaml | 88 ++++++++++++++++---
 1 file changed, 78 insertions(+), 10 deletions(-)

Comments

Conor Dooley Feb. 28, 2024, 11:58 a.m. UTC | #1 
On Tue, Feb 27, 2024 at 02:03:10PM +1300, Chris Packham wrote:

>    interrupts:
> +    description:
> +      Alarm1 interrupt line of the RTC. Some of the RTCs have two interrupt
> +      lines and alarm1 interrupt muxing depends on the clockin/clockout
> +      configuration.
>      maxItems: 1

The maxItems: 1 looks odd here when you state "some of the RTCs have two
interrupt lines", which makes it sound as if there are actually two
interrupts that should be exposed here. If those two interrupts get
muxed to the same pin for output I'd suggest that you clarify that here.

Otherwise, this looks good to me - although I do wonder if the
authorship on the commit (attributed to the analog guys) is still
accurate.

Thanks,
Conor.
Chris Packham Feb. 28, 2024, 8:21 p.m. UTC | #2 
On 29/02/24 00:58, Conor Dooley wrote:
> On Tue, Feb 27, 2024 at 02:03:10PM +1300, Chris Packham wrote:
>
>>     interrupts:
>> +    description:
>> +      Alarm1 interrupt line of the RTC. Some of the RTCs have two interrupt
>> +      lines and alarm1 interrupt muxing depends on the clockin/clockout
>> +      configuration.
>>       maxItems: 1
> The maxItems: 1 looks odd here when you state "some of the RTCs have two
> interrupt lines", which makes it sound as if there are actually two
> interrupts that should be exposed here. If those two interrupts get
> muxed to the same pin for output I'd suggest that you clarify that here.

This may end up changing if I can come up with something that Alexandre 
is happy with. Basically (some of) the chips have a configurable pin 
that can either be dedicated to the ALARM1 output (annoyingly labelled 
as INTB) or to a clock output. There is an INTA line that has other 
interrupts and if the clock output option is used then it also has 
ALARM1. The driver doesn't currently do anything with the other 
interrupt sources so as written this needs to correspond to whichever 
interrupt output is asserted for ALARM1.

> Otherwise, this looks good to me - although I do wonder if the
> authorship on the commit (attributed to the analog guys) is still
> accurate.

I think the binding is still pretty close to the last version sent out 
by the analog folks. The driver at this point is probably unrecognizable 
even though I've only really renamed stuff and moved functions around.

I was kind of hoping my prodding would be met with a "oh we've already 
done this in our SDK, here's the latest version" but that hasn't 
happened. I'm fairly close to dropping anything in this series that 
isn't related to the MAX31334 as that is the only hardware I can 
actually test.

>
> Thanks,
> Conor.
Conor Dooley Feb. 29, 2024, 6:07 p.m. UTC | #3 
On Wed, Feb 28, 2024 at 08:21:35PM +0000, Chris Packham wrote:
> 
> On 29/02/24 00:58, Conor Dooley wrote:
> > On Tue, Feb 27, 2024 at 02:03:10PM +1300, Chris Packham wrote:
> >
> >>     interrupts:
> >> +    description:
> >> +      Alarm1 interrupt line of the RTC. Some of the RTCs have two interrupt
> >> +      lines and alarm1 interrupt muxing depends on the clockin/clockout
> >> +      configuration.
> >>       maxItems: 1
> > The maxItems: 1 looks odd here when you state "some of the RTCs have two
> > interrupt lines", which makes it sound as if there are actually two
> > interrupts that should be exposed here. If those two interrupts get
> > muxed to the same pin for output I'd suggest that you clarify that here.
> 
> This may end up changing if I can come up with something that Alexandre 
> is happy with. Basically (some of) the chips have a configurable pin 
> that can either be dedicated to the ALARM1 output (annoyingly labelled 
> as INTB) or to a clock output. There is an INTA line that has other 
> interrupts and if the clock output option is used then it also has 
> ALARM1. The driver doesn't currently do anything with the other 
> interrupt sources so as written this needs to correspond to whichever 
> interrupt output is asserted for ALARM1.

So you're saying that depending on whether or not the clock output is
used, there could be two interrupts?

Without looking further, it sounds like you should be setting maxItems
to 1 if #clock-cells is present and to 2 if it is not. Then if there are
two interrupts provided, the driver is free to configure whatever way it
wants. If there aren't, send everything to INTA.

Am I missing something?

Cheers,
Conor.
Chris Packham Feb. 29, 2024, 8:11 p.m. UTC | #4 
On 1/03/24 07:07, Conor Dooley wrote:
> On Wed, Feb 28, 2024 at 08:21:35PM +0000, Chris Packham wrote:
>> On 29/02/24 00:58, Conor Dooley wrote:
>>> On Tue, Feb 27, 2024 at 02:03:10PM +1300, Chris Packham wrote:
>>>
>>>>      interrupts:
>>>> +    description:
>>>> +      Alarm1 interrupt line of the RTC. Some of the RTCs have two interrupt
>>>> +      lines and alarm1 interrupt muxing depends on the clockin/clockout
>>>> +      configuration.
>>>>        maxItems: 1
>>> The maxItems: 1 looks odd here when you state "some of the RTCs have two
>>> interrupt lines", which makes it sound as if there are actually two
>>> interrupts that should be exposed here. If those two interrupts get
>>> muxed to the same pin for output I'd suggest that you clarify that here.
>> This may end up changing if I can come up with something that Alexandre
>> is happy with. Basically (some of) the chips have a configurable pin
>> that can either be dedicated to the ALARM1 output (annoyingly labelled
>> as INTB) or to a clock output. There is an INTA line that has other
>> interrupts and if the clock output option is used then it also has
>> ALARM1. The driver doesn't currently do anything with the other
>> interrupt sources so as written this needs to correspond to whichever
>> interrupt output is asserted for ALARM1.
> So you're saying that depending on whether or not the clock output is
> used, there could be two interrupts?
Correct.
> Without looking further, it sounds like you should be setting maxItems
> to 1 if #clock-cells is present and to 2 if it is not.
My idea was an explicit property about the function of the INTB/CLKOUT 
pin. The current code does use #clock-cells as a proxy for this (and 
Alexandre has some concerns with how this is handled).
>   Then if there are
> two interrupts provided, the driver is free to configure whatever way it
> wants. If there aren't, send everything to INTA.
>
> Am I missing something?

Right now the only interrupt that the RTC cares about is for ALARM1 
(which moves between INTA and INTB depending on the clock config). There 
are other hardware events and an ALARM2 that can generate an interrupt 
but these are ignored. I don't think the rtc framework supports more 
than one alarm.

Binding wise I think this should take 1 or 2 interrupts. For simplicity 
the first interrupt should always correspond to ALARM1 (which could be 
INTB or INTA depending on the hardware design). The 2nd interrupt (if 
supplied) would be for the other events (which we don't currently do 
anything with).
Alexandre Belloni Feb. 29, 2024, 8:29 p.m. UTC | #5 
On 29/02/2024 20:11:16+0000, Chris Packham wrote:
> 
> On 1/03/24 07:07, Conor Dooley wrote:
> > On Wed, Feb 28, 2024 at 08:21:35PM +0000, Chris Packham wrote:
> >> On 29/02/24 00:58, Conor Dooley wrote:
> >>> On Tue, Feb 27, 2024 at 02:03:10PM +1300, Chris Packham wrote:
> >>>
> >>>>      interrupts:
> >>>> +    description:
> >>>> +      Alarm1 interrupt line of the RTC. Some of the RTCs have two interrupt
> >>>> +      lines and alarm1 interrupt muxing depends on the clockin/clockout
> >>>> +      configuration.
> >>>>        maxItems: 1
> >>> The maxItems: 1 looks odd here when you state "some of the RTCs have two
> >>> interrupt lines", which makes it sound as if there are actually two
> >>> interrupts that should be exposed here. If those two interrupts get
> >>> muxed to the same pin for output I'd suggest that you clarify that here.
> >> This may end up changing if I can come up with something that Alexandre
> >> is happy with. Basically (some of) the chips have a configurable pin
> >> that can either be dedicated to the ALARM1 output (annoyingly labelled
> >> as INTB) or to a clock output. There is an INTA line that has other
> >> interrupts and if the clock output option is used then it also has
> >> ALARM1. The driver doesn't currently do anything with the other
> >> interrupt sources so as written this needs to correspond to whichever
> >> interrupt output is asserted for ALARM1.
> > So you're saying that depending on whether or not the clock output is
> > used, there could be two interrupts?
> Correct.
> > Without looking further, it sounds like you should be setting maxItems
> > to 1 if #clock-cells is present and to 2 if it is not.
> My idea was an explicit property about the function of the INTB/CLKOUT 
> pin. The current code does use #clock-cells as a proxy for this (and 
> Alexandre has some concerns with how this is handled).

#clock-cells must not be used for pinmuxing, I can't see how anyone
would allow this.

> >   Then if there are
> > two interrupts provided, the driver is free to configure whatever way it
> > wants. If there aren't, send everything to INTA.
> >
> > Am I missing something?
> 
> Right now the only interrupt that the RTC cares about is for ALARM1 
> (which moves between INTA and INTB depending on the clock config). There 
> are other hardware events and an ALARM2 that can generate an interrupt 
> but these are ignored. I don't think the rtc framework supports more 
> than one alarm.
> 
> Binding wise I think this should take 1 or 2 interrupts. For simplicity 
> the first interrupt should always correspond to ALARM1 (which could be 
> INTB or INTA depending on the hardware design). The 2nd interrupt (if 
> supplied) would be for the other events (which we don't currently do 
> anything with).

Not using an interrupt simply means the CPU doesn't care about it but
there are other components that may care for example a PMIC. If you
reason about what linux and the CPU it is running on can do, you will
cripple functionality and we will have to break the devicetree binding
later on to restore it.
We need to be able to express all the possible pin configurations with
the binding. I'm not sure why everyone is so resistant to use pinmuxing
to mux pins....
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/rtc/adi,max31335.yaml b/Documentation/devicetree/bindings/rtc/adi,max31335.yaml
index 0125cf6727cc..ad8d6fec9a2b 100644
--- a/Documentation/devicetree/bindings/rtc/adi,max31335.yaml
+++ b/Documentation/devicetree/bindings/rtc/adi,max31335.yaml
@@ -1,36 +1,54 @@ 
 # SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+# Copyright 2022 Analog Devices Inc.
 %YAML 1.2
 ---
 $id: http://devicetree.org/schemas/rtc/adi,max31335.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#
 
-title: Analog Devices MAX31335 RTC
+title: Analog Devices MAX313XX series I2C RTCs
 
 maintainers:
   - Antoniu Miclaus <antoniu.miclaus@analog.com>
+  - Chris Packham <chris.packham@alliedtelesis.co.nz>
 
-description:
-  Analog Devices MAX31335 I2C RTC ±2ppm Automotive Real-Time Clock with
-  Integrated MEMS Resonator.
-
-allOf:
-  - $ref: rtc.yaml#
+description: Analog Devices MAX313XX series I2C RTCs.
 
 properties:
   compatible:
-    const: adi,max31335
+    enum:
+      - adi,max31328
+      - adi,max31329
+      - adi,max31331
+      - adi,max31334
+      - adi,max31335
+      - adi,max31341
+      - adi,max31342
+      - adi,max31343
 
   reg:
-    maxItems: 1
+    description: I2C address of the RTC
+    items:
+      - enum: [0x68, 0x69]
 
   interrupts:
+    description:
+      Alarm1 interrupt line of the RTC. Some of the RTCs have two interrupt
+      lines and alarm1 interrupt muxing depends on the clockin/clockout
+      configuration.
     maxItems: 1
 
   "#clock-cells":
     description:
-      RTC can be used as a clock source through its clock output pin.
+      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
+
   adi,tc-diode:
     description:
       Select the diode configuration for the trickle charger.
@@ -48,6 +66,56 @@  required:
   - compatible
   - reg
 
+allOf:
+  - $ref: rtc.yaml#
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - adi,max31328
+              - adi,max31342
+
+    then:
+      properties:
+        aux-voltage-chargeable: false
+        trickle-resistor-ohms: false
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - adi,max31328
+              - adi,max31331
+              - adi,max31334
+              - adi,max31335
+              - 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
+
 unevaluatedProperties: false
 
 examples: