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@xxxxxxxxxx>
>>
>> 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@xxxxxxxxxx>
>> Signed-off-by: Zeynep Arslanbenzer <Zeynep.Arslanbenzer@xxxxxxxxxx>
>> Signed-off-by: Chris Packham <chris.packham@xxxxxxxxxxxxxxxxxxx>
>> ---
>> .../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.
