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

Message ID 20240202025241.834283-3-chris.packham@alliedtelesis.co.nz
State New
Headers
Series None |

Commit Message

Chris Packham Feb. 2, 2024, 2:52 a.m. UTC
  From: Ibrahim Tilki <Ibrahim.Tilki@analog.com>

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>
Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
 .../devicetree/bindings/rtc/adi,max313xx.yaml | 145 ++++++++++++++++++
 1 file changed, 145 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rtc/adi,max313xx.yaml
  

Comments

Krzysztof Kozlowski Feb. 2, 2024, 8:28 a.m. UTC | #1
On 02/02/2024 03:52, Chris Packham wrote:
> From: Ibrahim Tilki <Ibrahim.Tilki@analog.com>
> 
> 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>
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---

..

> +  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.
> +      schottky - Schottky diode in series.
> +      standard+schottky - standard diode + Schottky diode in series.
> +    enum: [schottky, standard+schottky]
> +
> +  trickle-resistor-ohms:
> +    description: Selected resistor for trickle charger.
> +    enum: [3000, 6000, 11000]

Please remind us and document in commit msg, why this cannot be part of
max31335 binding? Looks exactly the same.

> +
> +required:
> +  - compatible
> +  - reg



Best regards,
Krzysztof
  
Chris Packham Feb. 6, 2024, 8:21 p.m. UTC | #2
(resend as plain text, sorry for the noise)


On 2/02/24 21:28, Krzysztof Kozlowski wrote:
> On 02/02/2024 03:52, Chris Packham wrote:
>> From: Ibrahim Tilki<Ibrahim.Tilki@analog.com>
>>
>> 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>
>> Signed-off-by: Chris Packham<chris.packham@alliedtelesis.co.nz>
>> ---
> ...
>
>> +  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.
>> +      schottky - Schottky diode in series.
>> +      standard+schottky - standard diode + Schottky diode in series.
>> +    enum: [schottky, standard+schottky]
>> +
>> +  trickle-resistor-ohms:
>> +    description: Selected resistor for trickle charger.
>> +    enum: [3000, 6000, 11000]
> Please remind us and document in commit msg, why this cannot be part of
> max31335 binding? Looks exactly the same.

That is an incredibly good point. The max31335 binding covers one 
specific chip. This binding covers more and with that there are a few 
more properties that the max31335 on it's own doesn't have (e.g. the 
clock consumer, the ability to have different i2c addresses). Binding 
wise I could probably roll all of the max31335 into this max313xx binding.

Driver wise things are a bit trickier. I've only got access to one of 
the variants so I am hoping to leverage the work Ibrahim had already 
done. I could attempt to incorporate max31335 support into the max313xx 
driver but I wouldn't really be able to test it properly and there is a 
reasonably high chance of regressing something.

>> +
>> +required:
>> +  - compatible
>> +  - reg
> Best regards,
> Krzysztof
>
  
Alexandre Belloni Feb. 6, 2024, 9:12 p.m. UTC | #3
On 06/02/2024 20:19:20+0000, Chris Packham wrote:
> That is an incredibly good point. The max31335 binding covers one specific chip. This binding covers more and with that there are a few more properties that the max31335 on it's own doesn't have (e.g. the clock consumer, the ability to have different i2c addresses). Binding wise I could probably roll all of the max31335 into this max313xx binding.
> 
> Driver wise things are a bit trickier. I've only got access to one of
> the variants so I am hoping to leverage the work Ibrahim had already
> done. I could attempt to incorporate max31335 support into the
> max313xx driver but I wouldn't really be able to test it properly and
> there is a reasonably high chance of regressing something.

But I won't take a separate driver. Everything would be better if Analog
was sharing the datasheets...
  
Chris Packham Feb. 6, 2024, 9:41 p.m. UTC | #4
On 7/02/24 10:12, Alexandre Belloni wrote:
> On 06/02/2024 20:19:20+0000, Chris Packham wrote:
>> That is an incredibly good point. The max31335 binding covers one specific chip. This binding covers more and with that there are a few more properties that the max31335 on it's own doesn't have (e.g. the clock consumer, the ability to have different i2c addresses). Binding wise I could probably roll all of the max31335 into this max313xx binding.
>>
>> Driver wise things are a bit trickier. I've only got access to one of
>> the variants so I am hoping to leverage the work Ibrahim had already
>> done. I could attempt to incorporate max31335 support into the
>> max313xx driver but I wouldn't really be able to test it properly and
>> there is a reasonably high chance of regressing something.
> But I won't take a separate driver. Everything would be better if Analog
> was sharing the datasheets...

The datasheets are pretty accessible so I'd give Analog a pass on that 
(they're certainly better than some vendors). I'll include some links on 
the next update.

I'll attempt to roll the max31335 into the max313xx driver.
  
Alexandre Belloni Feb. 6, 2024, 10:16 p.m. UTC | #5
On 06/02/2024 21:41:10+0000, Chris Packham wrote:
> 
> On 7/02/24 10:12, Alexandre Belloni wrote:
> > On 06/02/2024 20:19:20+0000, Chris Packham wrote:
> >> That is an incredibly good point. The max31335 binding covers one specific chip. This binding covers more and with that there are a few more properties that the max31335 on it's own doesn't have (e.g. the clock consumer, the ability to have different i2c addresses). Binding wise I could probably roll all of the max31335 into this max313xx binding.
> >>
> >> Driver wise things are a bit trickier. I've only got access to one of
> >> the variants so I am hoping to leverage the work Ibrahim had already
> >> done. I could attempt to incorporate max31335 support into the
> >> max313xx driver but I wouldn't really be able to test it properly and
> >> there is a reasonably high chance of regressing something.
> > But I won't take a separate driver. Everything would be better if Analog
> > was sharing the datasheets...
> 
> The datasheets are pretty accessible so I'd give Analog a pass on that 
> (they're certainly better than some vendors). I'll include some links on 
> the next update.
> 

The max31335 is not available

> I'll attempt to roll the max31335 into the max313xx driver.

I guess this would be the opposite. Renaming a driver breaks existing
kernel configurations.
  
Antoniu Miclaus Feb. 7, 2024, 9:54 a.m. UTC | #6
> On 06/02/2024 21:41:10+0000, Chris Packham wrote:
> >
> > On 7/02/24 10:12, Alexandre Belloni wrote:
> > > On 06/02/2024 20:19:20+0000, Chris Packham wrote:
> > >> That is an incredibly good point. The max31335 binding covers one
> specific chip. This binding covers more and with that there are a few more
> properties that the max31335 on it's own doesn't have (e.g. the clock
> consumer, the ability to have different i2c addresses). Binding wise I could
> probably roll all of the max31335 into this max313xx binding.
> > >>
> > >> Driver wise things are a bit trickier. I've only got access to one of
> > >> the variants so I am hoping to leverage the work Ibrahim had already
> > >> done. I could attempt to incorporate max31335 support into the
> > >> max313xx driver but I wouldn't really be able to test it properly and
> > >> there is a reasonably high chance of regressing something.
> > > But I won't take a separate driver. Everything would be better if Analog
> > > was sharing the datasheets...
> >
> > The datasheets are pretty accessible so I'd give Analog a pass on that
> > (they're certainly better than some vendors). I'll include some links on
> > the next update.
> >
> 
> The max31335 is not available
>
 
Indeed, the max31355 datasheet is not available yet. This is also stated in the
cover letter of the patch series for max31335.

It was an urgent request from a customer to have it mainline as soon as possible.

If there are questions about the part functionality, I can definitely help.

> > I'll attempt to roll the max31335 into the max313xx driver.
> 
> I guess this would be the opposite. Renaming a driver breaks existing
> kernel configurations.
> 
> --
> Alexandre Belloni, co-owner and COO, Bootlin
> Embedded Linux and Kernel engineering
> https://urldefense.com/v3/__https://bootlin.com__;!!A3Ni8CS0y2Y!7A0VM
> Nj1SLsNWZ8SnPrFwG6KX2TqjeLQi38EYIV164_5MWlQ7yCFvp8yOjaswIRNLC
> 0EK-_bhHEfy6xAXEJimVEW8BZXdn_r$
  
Guenter Roeck Feb. 7, 2024, 3 p.m. UTC | #7
On 2/7/24 01:54, Miclaus, Antoniu wrote:
[ ... ]
>>
>> The max31335 is not available
>>
>   
> Indeed, the max31355 datasheet is not available yet. This is also stated in the
> cover letter of the patch series for max31335.
> 
> It was an urgent request from a customer to have it mainline as soon as possible.
> 
> If there are questions about the part functionality, I can definitely help.
> 

It seems to be quite common for automotive chips, though, that they are held
tightly under wrap, making it all but impossible to properly review their drivers.
I have observed several times now that information not available to reviewers
resulted in bad or buggy drivers. This isn't about willingness to help, it is
about the ability to understand chip capabilities and requirements.

Guenter
  

Patch

diff --git a/Documentation/devicetree/bindings/rtc/adi,max313xx.yaml b/Documentation/devicetree/bindings/rtc/adi,max313xx.yaml
new file mode 100644
index 000000000000..ccfb0cbfb045
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/adi,max313xx.yaml
@@ -0,0 +1,145 @@ 
+# 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 RTCs
+
+maintainers:
+  - Chris Packham <chris.packham@alliedtelesis.co.nz>
+
+description: Analog Devices MAX313XX series I2C 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:
+    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 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.
+      schottky - Schottky diode in series.
+      standard+schottky - standard diode + Schottky diode in series.
+    enum: [schottky, standard+schottky]
+
+  trickle-resistor-ohms:
+    description: Selected resistor for trickle charger.
+    enum: [3000, 6000, 11000]
+
+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,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:
+  - |
+    #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>;
+            aux-voltage-chargeable = <1>;
+            trickle-resistor-ohms = <6000>;
+            adi,tc-diode = "schottky";
+        };
+    };
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        rtc@68 {
+            reg = <0x68>;
+            compatible = "adi,max31331";
+            #clock-cells = <0>;
+        };
+    };