| Message ID | 20231031153100.92939-1-antoniu.miclaus@analog.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3,1/2] dt-bindings: rtc: max31335: add max31335 bindings | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On 10/31/23 08:30, Antoniu Miclaus wrote: > RTC driver for MAX31335 ±2ppm Automotive Real-Time Clock with > Integrated MEMS Resonator. > > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com> > --- > changes in v3: > - drop MAX31335_STATUS1 register check inside probe function. > drivers/rtc/Kconfig | 11 + > drivers/rtc/Makefile | 1 + > drivers/rtc/rtc-max31335.c | 765 +++++++++++++++++++++++++++++++++++++ > 3 files changed, 777 insertions(+) > create mode 100644 drivers/rtc/rtc-max31335.c > > diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig > index d7502433c78a..11c7d7fe1e85 100644 > --- a/drivers/rtc/Kconfig > +++ b/drivers/rtc/Kconfig > @@ -373,6 +373,17 @@ config RTC_DRV_MAX8997 > This driver can also be built as a module. If so, the module > will be called rtc-max8997. > > +config RTC_DRV_MAX31335 > + tristate "Analog Devices MAX31335" > + depends on I2C > + select REGMAP_I2C > + help > + If you say yes here you get support for the Analog Devices > + MAX31335. > + > + This driver can also be built as a module. If so, the module > + will be called rtc-max31335. > + Just out of curiosity, is this an unreleased chip ? I only find MAX311331 and MAX31334 on the Analog website, but the register map for those is different, and they don't support reporting the chip temperature. [ ... ] > + > +static const struct hwmon_channel_info *max31335_info[] = { > + HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT), According to the register map above, the chip does support low and high temperature limits as well as over- and undertemperature alarms and interrupts. I would suggest to add support for all of those. You might also consider adding support for temperature alarm interrupts and report temperature alarm events by calling hwmon_notify_event() if a thermal event occurs. [ ... ] > + > + hwmon = devm_hwmon_device_register_with_info(&client->dev, client->name, > + max31335, > + &max31335_chip_info, > + NULL); There is no "depends on HWMON" in the Kconfig entry, meaning this will fail to compile if HWMON=n or if HWMON=m and RTC_DRV_MAX31335=y. Guenter
> On 10/31/23 08:30, Antoniu Miclaus wrote: > > RTC driver for MAX31335 ±2ppm Automotive Real-Time Clock with > > Integrated MEMS Resonator. > > > > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com> > > --- > > changes in v3: > > - drop MAX31335_STATUS1 register check inside probe function. > > drivers/rtc/Kconfig | 11 + > > drivers/rtc/Makefile | 1 + > > drivers/rtc/rtc-max31335.c | 765 > +++++++++++++++++++++++++++++++++++++ > > 3 files changed, 777 insertions(+) > > create mode 100644 drivers/rtc/rtc-max31335.c > > > > diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig > > index d7502433c78a..11c7d7fe1e85 100644 > > --- a/drivers/rtc/Kconfig > > +++ b/drivers/rtc/Kconfig > > @@ -373,6 +373,17 @@ config RTC_DRV_MAX8997 > > This driver can also be built as a module. If so, the module > > will be called rtc-max8997. > > > > +config RTC_DRV_MAX31335 > > + tristate "Analog Devices MAX31335" > > + depends on I2C > > + select REGMAP_I2C > > + help > > + If you say yes here you get support for the Analog Devices > > + MAX31335. > > + > > + This driver can also be built as a module. If so, the module > > + will be called rtc-max31335. > > + > > Just out of curiosity, is this an unreleased chip ? I only find > MAX311331 and MAX31334 on the Analog website, but the register > map for those is different, and they don't support reporting > the chip temperature. > > [ ... ] > > > + > > +static const struct hwmon_channel_info *max31335_info[] = { > > + HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT), > > According to the register map above, the chip does support > low and high temperature limits as well as over- and undertemperature > alarms and interrupts. I would suggest to add support for all of those. > You might also consider adding support for temperature alarm interrupts > and report temperature alarm events by calling hwmon_notify_event() > if a thermal event occurs. I've sent in the first version of this patch series a cover letter: "Although the datasheet is not public yet, the driver can be made public (on other linux custom trees it is already). The driver was tested with actual hardware and works. Even though the datasheet is not available, if there are any queries about the functionality of the part, these can be provided/inserted as code comments inside the driver." The reason why I am rushing this a bit is because the customer that uses the driver wants the driver released and mainline kernel compliant. This is an initial version of the driver covering the main use cases (which were requested, therefore actually used). Additional features can be added afterwards, if requested. > > > + > > + hwmon = devm_hwmon_device_register_with_info(&client->dev, > client->name, > > + max31335, > > + &max31335_chip_info, > > + NULL); > > There is no "depends on HWMON" in the Kconfig entry, meaning this will fail > to compile if HWMON=n or if HWMON=m and RTC_DRV_MAX31335=y. > Will do in v4. > Guenter
On 31/10/2023 16:30, Antoniu Miclaus wrote: > Document the Analog Devices MAX31335 device tree bindings. > > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com> > --- > changes in v3: > - `trickle-resistor-ohms` description specifies that the property is mandatory > if trickle charger should be enabled. > .../devicetree/bindings/rtc/adi,max31335.yaml | 64 +++++++++++++++++++ > 1 file changed, 64 insertions(+) > create mode 100644 Documentation/devicetree/bindings/rtc/adi,max31335.yaml > > diff --git a/Documentation/devicetree/bindings/rtc/adi,max31335.yaml b/Documentation/devicetree/bindings/rtc/adi,max31335.yaml > new file mode 100644 > index 000000000000..8da9cf2565be > --- /dev/null > +++ b/Documentation/devicetree/bindings/rtc/adi,max31335.yaml > @@ -0,0 +1,64 @@ > +# 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 > + > + trickle-resistor-ohms: > + description: > + Selected resistor for trickle charger. Should be specified if trickle > + charger should be enabled. > + enum: [3000, 6000, 11000] > + > + aux-voltage-chargeable: true Drop, it comes with rtc.yaml. With this property removed: Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
On 31/10/2023 16:28:07+0000, Miclaus, Antoniu wrote: > > According to the register map above, the chip does support > > low and high temperature limits as well as over- and undertemperature > > alarms and interrupts. I would suggest to add support for all of those. > > You might also consider adding support for temperature alarm interrupts > > and report temperature alarm events by calling hwmon_notify_event() > > if a thermal event occurs. > > I've sent in the first version of this patch series a cover letter: > > "Although the datasheet is not public yet, the driver can be made public (on > other linux custom trees it is already). > > The driver was tested with actual hardware and works. > Did you run rtctest? Please provide the output. > Even though the datasheet is not available, if there are any queries about > the functionality of the part, these can be provided/inserted as code comments > inside the driver." > > The reason why I am rushing this a bit is because the customer that uses the > driver wants the driver released and mainline kernel compliant. > > This is an initial version of the driver covering the main use cases (which were > requested, therefore actually used). > > Additional features can be added afterwards, if requested. > > > > > > + > > > + hwmon = devm_hwmon_device_register_with_info(&client->dev, > > client->name, > > > + max31335, > > > + &max31335_chip_info, > > > + NULL); > > > > There is no "depends on HWMON" in the Kconfig entry, meaning this will fail > > to compile if HWMON=n or if HWMON=m and RTC_DRV_MAX31335=y. > > > > Will do in v4. > > > Guenter
diff --git a/Documentation/devicetree/bindings/rtc/adi,max31335.yaml b/Documentation/devicetree/bindings/rtc/adi,max31335.yaml new file mode 100644 index 000000000000..8da9cf2565be --- /dev/null +++ b/Documentation/devicetree/bindings/rtc/adi,max31335.yaml @@ -0,0 +1,64 @@ +# 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 + + trickle-resistor-ohms: + description: + Selected resistor for trickle charger. Should be specified if trickle + charger should be enabled. + enum: [3000, 6000, 11000] + + aux-voltage-chargeable: true + +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>; + trickle-resistor-ohms = <6000>; + aux-voltage-chargeable =<1>; + }; + }; +...
Document the Analog Devices MAX31335 device tree bindings. Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com> --- changes in v3: - `trickle-resistor-ohms` description specifies that the property is mandatory if trickle charger should be enabled. .../devicetree/bindings/rtc/adi,max31335.yaml | 64 +++++++++++++++++++ 1 file changed, 64 insertions(+) create mode 100644 Documentation/devicetree/bindings/rtc/adi,max31335.yaml