Re: [PATCH v7 2/2] dt-bindings: rtc: add max313xx RTCs

[Chris Packham <Chris.Packham@alliedtelesis.co.nz>]

On 21/02/24 08:21, Conor Dooley wrote:
> Hey Chris,
>
> On Tue, Feb 20, 2024 at 11:18:24AM +1300, Chris Packham wrote:
>> 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 |  70 --------
>>   .../devicetree/bindings/rtc/adi,max313xx.yaml | 167 ++++++++++++++++++
> There's no need to do this rename. Having the filename matching one of
> the compatibles is our preference.
OK wasn't sure. I'll keep the existing name.
> In addition, it makes it difficult to see what your actual additions are
> here. Fortunately, applying the patch locally allows me to use colour
> moved and all that jazz, so I can see that the underlying changes to the
> file actually look pretty good.
>
>> +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 {
>> +            compatible = "adi,max31335";
>> +            reg = <0x68>;
>> +            pinctrl-0 = <&rtc_nint_pins>;
>> +            interrupts-extended = <&gpio1 16 IRQ_TYPE_LEVEL_HIGH>;
>> +            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>;
>> +        };
>> +    };
> The one thing I do want the comment on is the number of examples.
> I don't really see what we gain from having 3 - I'd roll the clock
> provider example into with one of the other ones I think.
The 3 examples are an artifact of me combining the in-flight max313xx 
series with the one that landed. The clock output is only valid for some 
chips but that's probably just a matter of picking the right compatible.