| Message ID | 20240202025241.834283-3-chris.packham@alliedtelesis.co.nz |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | drivers: rtc: add max313xx series rtc driver | expand |
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
(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 >
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...
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.
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.
> 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$
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
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>; + }; + };