| Message ID | 20240219221827.3821415-3-chris.packham@alliedtelesis.co.nz |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | drivers: rtc: add max313xx series rtc driver | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 0 errors, 2 warnings, 167 lines checked |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
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. 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. Cheers, Conor.
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.
On Tue, Feb 20, 2024 at 08:11:13PM +0000, Chris Packham wrote: > On 21/02/24 08:21, Conor Dooley wrote: > > On Tue, Feb 20, 2024 at 11:18:24AM +1300, Chris Packham wrote: > > 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. Yeah, I think you could collapse it in, if you modify the compatible appropriately. Cheers, Conor.
diff --git a/Documentation/devicetree/bindings/rtc/adi,max31335.yaml b/Documentation/devicetree/bindings/rtc/adi,max31335.yaml deleted file mode 100644 index 0125cf6727cc..000000000000 --- a/Documentation/devicetree/bindings/rtc/adi,max31335.yaml +++ /dev/null @@ -1,70 +0,0 @@ -# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause -%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 - -maintainers: - - Antoniu Miclaus <antoniu.miclaus@analog.com> - -description: - Analog Devices MAX31335 I2C RTC ±2ppm Automotive Real-Time Clock with - Integrated MEMS Resonator. - -allOf: - - $ref: rtc.yaml# - -properties: - compatible: - const: adi,max31335 - - reg: - maxItems: 1 - - interrupts: - maxItems: 1 - - "#clock-cells": - description: - RTC can be used as a clock source through its clock output pin. - const: 0 - - 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. Should be specified if trickle - charger should be enabled. - enum: [3000, 6000, 11000] - -required: - - compatible - - reg - -unevaluatedProperties: false - -examples: - - | - #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"; - }; - }; -... diff --git a/Documentation/devicetree/bindings/rtc/adi,max313xx.yaml b/Documentation/devicetree/bindings/rtc/adi,max313xx.yaml new file mode 100644 index 000000000000..e56e5394aa86 --- /dev/null +++ b/Documentation/devicetree/bindings/rtc/adi,max313xx.yaml @@ -0,0 +1,167 @@ +# 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,max313xx.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Analog Devices MAX313XX series I2C RTCs + +maintainers: + - Antoniu Miclaus <antoniu.miclaus@analog.com> + - 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,max31335 + - 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. Should be specified if trickle + charger should be enabled. + 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,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: + - | + #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>; + }; + }; +...