[PATCH v3 0/2] Add support for MAX31331 RTC

[PATCH v3 0/2] Add support for MAX31331 RTC

[PavithraUdayakumar-adi via B4 Relay]

This patch series introduces support for the Maxim MAX31331 RTC.
It includes:

1. Device Tree bindings documentation for the MAX31331 chip.
2. The driver implementation for the MAX31331 RTC

Signed-off-by: PavithraUdayakumar-adi <pavithra.u@analog.com>

Changes in v3:
- Address review comments
- Rebase on v6.13-rc6
- Link to v2: https://lore.kernel.org/all/20250103-add_support_max31331_fix-v1-0-8ff3c7a81734@analog.com/

---
PavithraUdayakumar-adi (2):
      dt-bindings: rtc: max31335: Add max31331 support
      rtc: max31335: Add driver support for max31331

 .../devicetree/bindings/rtc/adi,max31335.yaml      |  22 ++-
 drivers/rtc/rtc-max31335.c                         | 163 +++++++++++++++------
 2 files changed, 138 insertions(+), 47 deletions(-)
---
base-commit: eea6e4b4dfb8859446177c32961c96726d0117be
change-id: 20250109-add_support_max31331_fix_3-c64269a291e0

Best regards,
-- 
PavithraUdayakumar-adi <pavithra.u@analog.com>

[PATCH v3 1/2] dt-bindings: rtc: max31335: Add max31331 support

[PavithraUdayakumar-adi via B4 Relay]

From: PavithraUdayakumar-adi <pavithra.u@analog.com>

MAX31331 is an ultra-low-power, I2C Real-Time Clock RTC with flexible
crystal support. While, MAX31335 offers higher precision, MEMS resonator,
and integrated temperature sensor. MAX31331 uses I2C address as 0x68
where as max31335 uses 0x69.

Changes: Added example for max31331 and modified the register address
for max31335.

Signed-off-by: PavithraUdayakumar-adi <pavithra.u@analog.com>
---
 .../devicetree/bindings/rtc/adi,max31335.yaml      | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/rtc/adi,max31335.yaml b/Documentation/devicetree/bindings/rtc/adi,max31335.yaml
index 0125cf6727cc3d9eb3e0253299904ee363ec40ca..f249313bc485d7a6154ce684726d6a950405ef0e 100644
--- a/Documentation/devicetree/bindings/rtc/adi,max31335.yaml
+++ b/Documentation/devicetree/bindings/rtc/adi,max31335.yaml
@@ -18,10 +18,13 @@ allOf:
 
 properties:
   compatible:
-    const: adi,max31335
+    enum:
+      - adi,max31331
+      - adi,max31335
 
   reg:
-    maxItems: 1
+    items:
+      - enum: [0x68, 0x69]
 
   interrupts:
     maxItems: 1
@@ -57,9 +60,9 @@ examples:
         #address-cells = <1>;
         #size-cells = <0>;
 
-        rtc@68 {
+        rtc@69 {
             compatible = "adi,max31335";
-            reg = <0x68>;
+            reg = <0x69>;
             pinctrl-0 = <&rtc_nint_pins>;
             interrupts-extended = <&gpio1 16 IRQ_TYPE_LEVEL_HIGH>;
             aux-voltage-chargeable = <1>;
@@ -67,4 +70,15 @@ examples:
             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";
+        };
+    };
 ...

-- 
2.25.1

[PATCH v3 2/2] rtc: max31335: Add driver support for max31331

[PavithraUdayakumar-adi via B4 Relay]

From: PavithraUdayakumar-adi <pavithra.u@analog.com>

MAX31331 is an ultra-low-power, I2C Real-Time Clock RTC.

Signed-off-by: PavithraUdayakumar-adi <pavithra.u@analog.com>
---
 drivers/rtc/rtc-max31335.c | 163 +++++++++++++++++++++++++++++++++------------
 1 file changed, 120 insertions(+), 43 deletions(-)

diff --git a/drivers/rtc/rtc-max31335.c b/drivers/rtc/rtc-max31335.c
index 3fbcf5f6b92ffd4581e9c4dbc87ec848867522dc..9d8f220fba2b21fc6393d67bff47f99b0227c707 100644
--- a/drivers/rtc/rtc-max31335.c
+++ b/drivers/rtc/rtc-max31335.c
@@ -184,31 +184,88 @@
 #define MAX31335_RAM_SIZE			32
 #define MAX31335_TIME_SIZE			0x07
 
+/* MAX31331 Register Map */
+#define MAX31331_RTC_CONFIG2			0x04
+
 #define clk_hw_to_max31335(_hw) container_of(_hw, struct max31335_data, clkout)
 
+/* Supported Maxim RTC */
+enum max_rtc_ids {
+	ID_MAX31331,
+	ID_MAX31335,
+	MAX_RTC_ID_NR
+};
+
+struct chip_desc {
+	u8 sec_reg;
+	u8 alarm1_sec_reg;
+
+	u8 int_en_reg;
+	u8 int_status_reg;
+
+	u8 ram_reg;
+	u8 ram_size;
+
+	u8 temp_reg;
+
+	u8 trickle_reg;
+
+	u8 clkout_reg;
+};
+
 struct max31335_data {
+	enum max_rtc_ids id;
 	struct regmap *regmap;
 	struct rtc_device *rtc;
 	struct clk_hw clkout;
+	struct clk *clkin;
+	const struct chip_desc *chip;
+	int irq;
 };
 
 static const int max31335_clkout_freq[] = { 1, 64, 1024, 32768 };
 
+static const struct chip_desc chip[MAX_RTC_ID_NR] = {
+	[ID_MAX31331] = {
+		.int_en_reg = 0x01,
+		.int_status_reg = 0x00,
+		.sec_reg = 0x08,
+		.alarm1_sec_reg = 0x0F,
+		.ram_reg = 0x20,
+		.ram_size = 32,
+		.trickle_reg = 0x1B,
+		.clkout_reg = 0x04,
+	},
+	[ID_MAX31335] = {
+		.int_en_reg = 0x01,
+		.int_status_reg = 0x00,
+		.sec_reg = 0x0A,
+		.alarm1_sec_reg = 0x11,
+		.ram_reg = 0x40,
+		.ram_size = 32,
+		.temp_reg = 0x35,
+		.trickle_reg = 0x1D,
+		.clkout_reg = 0x06,
+	},
+};
+
 static const u16 max31335_trickle_resistors[] = {3000, 6000, 11000};
 
 static bool max31335_volatile_reg(struct device *dev, unsigned int reg)
 {
+	struct max31335_data *max31335 = dev_get_drvdata(dev);
+	const struct chip_desc *chip = max31335->chip;
+
 	/* time keeping registers */
-	if (reg >= MAX31335_SECONDS &&
-	    reg < MAX31335_SECONDS + MAX31335_TIME_SIZE)
+	if (reg >= chip->sec_reg && reg < chip->sec_reg + MAX31335_TIME_SIZE)
 		return true;
 
 	/* interrupt status register */
-	if (reg == MAX31335_STATUS1)
+	if (reg == chip->int_status_reg)
 		return true;
 
-	/* temperature registers */
-	if (reg == MAX31335_TEMP_DATA_MSB || reg == MAX31335_TEMP_DATA_LSB)
+	/* temperature registers if valid */
+	if (chip->temp_reg && (reg == chip->temp_reg || reg == chip->temp_reg + 1))
 		return true;
 
 	return false;
@@ -227,7 +284,7 @@ static int max31335_read_time(struct device *dev, struct rtc_time *tm)
 	u8 date[7];
 	int ret;
 
-	ret = regmap_bulk_read(max31335->regmap, MAX31335_SECONDS, date,
+	ret = regmap_bulk_read(max31335->regmap, max31335->chip->sec_reg, date,
 			       sizeof(date));
 	if (ret)
 		return ret;
@@ -262,7 +319,7 @@ static int max31335_set_time(struct device *dev, struct rtc_time *tm)
 	if (tm->tm_year >= 200)
 		date[5] |= FIELD_PREP(MAX31335_MONTH_CENTURY, 1);
 
-	return regmap_bulk_write(max31335->regmap, MAX31335_SECONDS, date,
+	return regmap_bulk_write(max31335->regmap, max31335->chip->sec_reg, date,
 				 sizeof(date));
 }
 
@@ -273,7 +330,7 @@ static int max31335_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 	struct rtc_time time;
 	u8 regs[6];
 
-	ret = regmap_bulk_read(max31335->regmap, MAX31335_ALM1_SEC, regs,
+	ret = regmap_bulk_read(max31335->regmap, max31335->chip->alarm1_sec_reg, regs,
 			       sizeof(regs));
 	if (ret)
 		return ret;
@@ -292,11 +349,11 @@ static int max31335_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 	if (time.tm_year >= 200)
 		alrm->time.tm_year += 100;
 
-	ret = regmap_read(max31335->regmap, MAX31335_INT_EN1, &ctrl);
+	ret = regmap_read(max31335->regmap, max31335->chip->int_en_reg, &ctrl);
 	if (ret)
 		return ret;
 
-	ret = regmap_read(max31335->regmap, MAX31335_STATUS1, &status);
+	ret = regmap_read(max31335->regmap, max31335->chip->int_status_reg, &status);
 	if (ret)
 		return ret;
 
@@ -320,18 +377,18 @@ static int max31335_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 	regs[4] = bin2bcd(alrm->time.tm_mon + 1);
 	regs[5] = bin2bcd(alrm->time.tm_year % 100);
 
-	ret = regmap_bulk_write(max31335->regmap, MAX31335_ALM1_SEC,
+	ret = regmap_bulk_write(max31335->regmap, max31335->chip->alarm1_sec_reg,
 				regs, sizeof(regs));
 	if (ret)
 		return ret;
 
 	reg = FIELD_PREP(MAX31335_INT_EN1_A1IE, alrm->enabled);
-	ret = regmap_update_bits(max31335->regmap, MAX31335_INT_EN1,
+	ret = regmap_update_bits(max31335->regmap, max31335->chip->int_en_reg,
 				 MAX31335_INT_EN1_A1IE, reg);
 	if (ret)
 		return ret;
 
-	ret = regmap_update_bits(max31335->regmap, MAX31335_STATUS1,
+	ret = regmap_update_bits(max31335->regmap, max31335->chip->int_status_reg,
 				 MAX31335_STATUS1_A1F, 0);
 
 	return 0;
@@ -341,23 +398,33 @@ static int max31335_alarm_irq_enable(struct device *dev, unsigned int enabled)
 {
 	struct max31335_data *max31335 = dev_get_drvdata(dev);
 
-	return regmap_update_bits(max31335->regmap, MAX31335_INT_EN1,
+	return regmap_update_bits(max31335->regmap, max31335->chip->int_en_reg,
 				  MAX31335_INT_EN1_A1IE, enabled);
 }
 
 static irqreturn_t max31335_handle_irq(int irq, void *dev_id)
 {
 	struct max31335_data *max31335 = dev_id;
-	bool status;
-	int ret;
+	struct mutex *lock = &max31335->rtc->ops_lock;
+	int ret, status;
+
+	mutex_lock(lock);
 
-	ret = regmap_update_bits_check(max31335->regmap, MAX31335_STATUS1,
-				       MAX31335_STATUS1_A1F, 0, &status);
+	ret = regmap_read(max31335->regmap, max31335->chip->int_status_reg, &status);
 	if (ret)
-		return IRQ_HANDLED;
+		goto exit;
+
+	if (FIELD_GET(MAX31335_STATUS1_A1F, status)) {
+		ret = regmap_update_bits(max31335->regmap, max31335->chip->int_status_reg,
+					 MAX31335_STATUS1_A1F, 0);
+		if (ret)
+			goto exit;
 
-	if (status)
 		rtc_update_irq(max31335->rtc, 1, RTC_AF | RTC_IRQF);
+	}
+
+exit:
+	mutex_unlock(lock);
 
 	return IRQ_HANDLED;
 }
@@ -404,7 +471,7 @@ static int max31335_trickle_charger_setup(struct device *dev,
 
 	i = i + trickle_cfg;
 
-	return regmap_write(max31335->regmap, MAX31335_TRICKLE_REG,
+	return regmap_write(max31335->regmap, max31335->chip->trickle_reg,
 			    FIELD_PREP(MAX31335_TRICKLE_REG_TRICKLE, i) |
 			    FIELD_PREP(MAX31335_TRICKLE_REG_EN_TRICKLE,
 				       chargeable));
@@ -418,7 +485,7 @@ static unsigned long max31335_clkout_recalc_rate(struct clk_hw *hw,
 	unsigned int reg;
 	int ret;
 
-	ret = regmap_read(max31335->regmap, MAX31335_RTC_CONFIG2, &reg);
+	ret = regmap_read(max31335->regmap, max31335->chip->clkout_reg, &reg);
 	if (ret)
 		return 0;
 
@@ -449,23 +516,23 @@ static int max31335_clkout_set_rate(struct clk_hw *hw, unsigned long rate,
 			     ARRAY_SIZE(max31335_clkout_freq));
 	freq_mask = __roundup_pow_of_two(ARRAY_SIZE(max31335_clkout_freq)) - 1;
 
-	return regmap_update_bits(max31335->regmap, MAX31335_RTC_CONFIG2,
-				  freq_mask, index);
+	return regmap_update_bits(max31335->regmap, max31335->chip->clkout_reg,
+				 freq_mask, index);
 }
 
 static int max31335_clkout_enable(struct clk_hw *hw)
 {
 	struct max31335_data *max31335 = clk_hw_to_max31335(hw);
 
-	return regmap_set_bits(max31335->regmap, MAX31335_RTC_CONFIG2,
-			       MAX31335_RTC_CONFIG2_ENCLKO);
+	return regmap_set_bits(max31335->regmap, max31335->chip->clkout_reg,
+			      MAX31335_RTC_CONFIG2_ENCLKO);
 }
 
 static void max31335_clkout_disable(struct clk_hw *hw)
 {
 	struct max31335_data *max31335 = clk_hw_to_max31335(hw);
 
-	regmap_clear_bits(max31335->regmap, MAX31335_RTC_CONFIG2,
+	regmap_clear_bits(max31335->regmap, max31335->chip->clkout_reg,
 			  MAX31335_RTC_CONFIG2_ENCLKO);
 }
 
@@ -475,7 +542,7 @@ static int max31335_clkout_is_enabled(struct clk_hw *hw)
 	unsigned int reg;
 	int ret;
 
-	ret = regmap_read(max31335->regmap, MAX31335_RTC_CONFIG2, &reg);
+	ret = regmap_read(max31335->regmap, max31335->chip->clkout_reg, &reg);
 	if (ret)
 		return ret;
 
@@ -500,7 +567,7 @@ static int max31335_nvmem_reg_read(void *priv, unsigned int offset,
 				   void *val, size_t bytes)
 {
 	struct max31335_data *max31335 = priv;
-	unsigned int reg = MAX31335_TS0_SEC_1_128 + offset;
+	unsigned int reg = max31335->chip->ram_reg + offset;
 
 	return regmap_bulk_read(max31335->regmap, reg, val, bytes);
 }
@@ -509,7 +576,7 @@ static int max31335_nvmem_reg_write(void *priv, unsigned int offset,
 				    void *val, size_t bytes)
 {
 	struct max31335_data *max31335 = priv;
-	unsigned int reg = MAX31335_TS0_SEC_1_128 + offset;
+	unsigned int reg = max31335->chip->ram_reg + offset;
 
 	return regmap_bulk_write(max31335->regmap, reg, val, bytes);
 }
@@ -533,7 +600,7 @@ static int max31335_read_temp(struct device *dev, enum hwmon_sensor_types type,
 	if (type != hwmon_temp || attr != hwmon_temp_input)
 		return -EOPNOTSUPP;
 
-	ret = regmap_bulk_read(max31335->regmap, MAX31335_TEMP_DATA_MSB,
+	ret = regmap_bulk_read(max31335->regmap, max31335->chip->temp_reg,
 			       reg, 2);
 	if (ret)
 		return ret;
@@ -577,8 +644,8 @@ static int max31335_clkout_register(struct device *dev)
 	int ret;
 
 	if (!device_property_present(dev, "#clock-cells"))
-		return regmap_clear_bits(max31335->regmap, MAX31335_RTC_CONFIG2,
-					 MAX31335_RTC_CONFIG2_ENCLKO);
+		return regmap_clear_bits(max31335->regmap, max31335->chip->clkout_reg,
+				  MAX31335_RTC_CONFIG2_ENCLKO);
 
 	max31335->clkout.init = &max31335_clk_init;
 
@@ -605,6 +672,7 @@ static int max31335_probe(struct i2c_client *client)
 #if IS_REACHABLE(HWMON)
 	struct device *hwmon;
 #endif
+	const struct chip_desc *match;
 	int ret;
 
 	max31335 = devm_kzalloc(&client->dev, sizeof(*max31335), GFP_KERNEL);
@@ -616,7 +684,11 @@ static int max31335_probe(struct i2c_client *client)
 		return PTR_ERR(max31335->regmap);
 
 	i2c_set_clientdata(client, max31335);
-
+	match = i2c_get_match_data(client);
+	if (!match)
+		return -ENODEV;
+	max31335->chip = match;
+	max31335->id = max31335->chip - chip;
 	max31335->rtc = devm_rtc_allocate_device(&client->dev);
 	if (IS_ERR(max31335->rtc))
 		return PTR_ERR(max31335->rtc);
@@ -639,6 +711,8 @@ static int max31335_probe(struct i2c_client *client)
 			dev_warn(&client->dev,
 				 "unable to request IRQ, alarm max31335 disabled\n");
 			client->irq = 0;
+		} else {
+			max31335->irq = client->irq;
 		}
 	}
 
@@ -652,13 +726,13 @@ static int max31335_probe(struct i2c_client *client)
 				     "cannot register rtc nvmem\n");
 
 #if IS_REACHABLE(HWMON)
-	hwmon = devm_hwmon_device_register_with_info(&client->dev, client->name,
-						     max31335,
-						     &max31335_chip_info,
-						     NULL);
-	if (IS_ERR(hwmon))
-		return dev_err_probe(&client->dev, PTR_ERR(hwmon),
-				     "cannot register hwmon device\n");
+	if (max31335->chip->temp_reg) {
+		hwmon = devm_hwmon_device_register_with_info(&client->dev, client->name, max31335,
+							     &max31335_chip_info, NULL);
+		if (IS_ERR(hwmon))
+			return dev_err_probe(&client->dev, PTR_ERR(hwmon),
+					     "cannot register hwmon device\n");
+	}
 #endif
 
 	ret = max31335_trickle_charger_setup(&client->dev, max31335);
@@ -669,14 +743,16 @@ static int max31335_probe(struct i2c_client *client)
 }
 
 static const struct i2c_device_id max31335_id[] = {
-	{ "max31335" },
+	{ "max31331", (kernel_ulong_t)&chip[ID_MAX31331] },
+	{ "max31335", (kernel_ulong_t)&chip[ID_MAX31335] },
 	{ }
 };
 
 MODULE_DEVICE_TABLE(i2c, max31335_id);
 
 static const struct of_device_id max31335_of_match[] = {
-	{ .compatible = "adi,max31335" },
+	{ .compatible = "adi,max31331", .data = &chip[ID_MAX31331] },
+	{ .compatible = "adi,max31335", .data = &chip[ID_MAX31335] },
 	{ }
 };
 
@@ -693,5 +769,6 @@ static struct i2c_driver max31335_driver = {
 module_i2c_driver(max31335_driver);
 
 MODULE_AUTHOR("Antoniu Miclaus <antoniu.miclaus@analog.com>");
+MODULE_AUTHOR("Saket Kumar Purwar <Saket.Kumarpurwar@analog.com>");
 MODULE_DESCRIPTION("MAX31335 RTC driver");
 MODULE_LICENSE("GPL");

-- 
2.25.1

Re: [PATCH v3 0/2] Add support for MAX31331 RTC

[Krzysztof Kozlowski]

On Thu, Jan 09, 2025 at 03:59:56PM +0530, PavithraUdayakumar-adi wrote:
> This patch series introduces support for the Maxim MAX31331 RTC.
> It includes:
> 
> 1. Device Tree bindings documentation for the MAX31331 chip.
> 2. The driver implementation for the MAX31331 RTC
> 
> Signed-off-by: PavithraUdayakumar-adi <pavithra.u@analog.com>
> 
> Changes in v3:
> - Address review comments

Which ones? What exactly changed? This has to be detailed.

Best regards,
Krzysztof

Re: [PATCH v3 1/2] dt-bindings: rtc: max31335: Add max31331 support

[Krzysztof Kozlowski]

On Thu, Jan 09, 2025 at 03:59:57PM +0530, PavithraUdayakumar-adi wrote:
> MAX31331 is an ultra-low-power, I2C Real-Time Clock RTC with flexible
> crystal support. While, MAX31335 offers higher precision, MEMS resonator,
> and integrated temperature sensor. MAX31331 uses I2C address as 0x68
> where as max31335 uses 0x69.
> 
> Changes: Added example for max31331 and modified the register address
> for max31335.

1. Why?
2. What does it mean "changes"? You did much more so I really do not
understand this paragraph.

> 
> Signed-off-by: PavithraUdayakumar-adi <pavithra.u@analog.com>
> ---
>  .../devicetree/bindings/rtc/adi,max31335.yaml      | 22 ++++++++++++++++++----
>  1 file changed, 18 insertions(+), 4 deletions(-)
> 

What changed here exactly?

> diff --git a/Documentation/devicetree/bindings/rtc/adi,max31335.yaml b/Documentation/devicetree/bindings/rtc/adi,max31335.yaml
> index 0125cf6727cc3d9eb3e0253299904ee363ec40ca..f249313bc485d7a6154ce684726d6a950405ef0e 100644
> --- a/Documentation/devicetree/bindings/rtc/adi,max31335.yaml
> +++ b/Documentation/devicetree/bindings/rtc/adi,max31335.yaml
> @@ -18,10 +18,13 @@ allOf:
>  
>  properties:
>    compatible:
> -    const: adi,max31335
> +    enum:
> +      - adi,max31331
> +      - adi,max31335
>  
>    reg:
> -    maxItems: 1
> +    items:
> +      - enum: [0x68, 0x69]
>  
>    interrupts:
>      maxItems: 1
> @@ -57,9 +60,9 @@ examples:
>          #address-cells = <1>;
>          #size-cells = <0>;
>  
> -        rtc@68 {
> +        rtc@69 {
>              compatible = "adi,max31335";
> -            reg = <0x68>;
> +            reg = <0x69>;

Why? I already asked about this - the same question "Why"


>              pinctrl-0 = <&rtc_nint_pins>;
>              interrupts-extended = <&gpio1 16 IRQ_TYPE_LEVEL_HIGH>;
>              aux-voltage-chargeable = <1>;
> @@ -67,4 +70,15 @@ examples:
>              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";

Drop this example, not necessary.

> +        };
> +    };
>  ...
> 
> -- 
> 2.25.1
> 

RE: [PATCH v3 1/2] dt-bindings: rtc: max31335: Add max31331 support

[U, Pavithra]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@kernel.org>
> Sent: Friday, January 10, 2025 2:05 PM
> To: U, Pavithra <Pavithra.U@analog.com>
> Cc: Miclaus, Antoniu <Antoniu.Miclaus@analog.com>; Alexandre Belloni
> <alexandre.belloni@bootlin.com>; Rob Herring <robh@kernel.org>; Krzysztof
> Kozlowski <krzk+dt@kernel.org>; Conor Dooley <conor+dt@kernel.org>; Jean
> Delvare <jdelvare@suse.com>; Guenter Roeck <linux@roeck-us.net>;
> Christophe JAILLET <christophe.jaillet@wanadoo.fr>; linux-rtc@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> hwmon@vger.kernel.org
> Subject: Re: [PATCH v3 1/2] dt-bindings: rtc: max31335: Add max31331 support
> 
> [External]
> 
> On Thu, Jan 09, 2025 at 03:59:57PM +0530, PavithraUdayakumar-adi wrote:
> > MAX31331 is an ultra-low-power, I2C Real-Time Clock RTC with flexible
> > crystal support. While, MAX31335 offers higher precision, MEMS
> > resonator, and integrated temperature sensor. MAX31331 uses I2C
> > address as 0x68 where as max31335 uses 0x69.
> >
> > Changes: Added example for max31331 and modified the register address
> > for max31335.
> 
> 1. Why?
> 2. What does it mean "changes"? You did much more so I really do not
> understand this paragraph.
 
- Added DT compatible string for MAX31331. MAX31331 is compatible with MAX31335 without any additional features.
- Updated I2C address for MAX31335 RTC to 0x69. (I will be reverting this change and sending as fix separately.)
- Included the address 0x69 in property reg for MAX31335. (will remove this change and include in fix)

> 
> >
> > Signed-off-by: PavithraUdayakumar-adi <pavithra.u@analog.com>
> > ---
> >  .../devicetree/bindings/rtc/adi,max31335.yaml      | 22
> ++++++++++++++++++----
> >  1 file changed, 18 insertions(+), 4 deletions(-)
> >
> 
> What changed here exactly?
> 
> > diff --git a/Documentation/devicetree/bindings/rtc/adi,max31335.yaml
> > b/Documentation/devicetree/bindings/rtc/adi,max31335.yaml
> > index
> >
> 0125cf6727cc3d9eb3e0253299904ee363ec40ca..f249313bc485d7a6154ce6847
> 26d
> > 6a950405ef0e 100644
> > --- a/Documentation/devicetree/bindings/rtc/adi,max31335.yaml
> > +++ b/Documentation/devicetree/bindings/rtc/adi,max31335.yaml
> > @@ -18,10 +18,13 @@ allOf:
> >
> >  properties:
> >    compatible:
> > -    const: adi,max31335
> > +    enum:
> > +      - adi,max31331
> > +      - adi,max31335
> >
> >    reg:
> > -    maxItems: 1
> > +    items:
> > +      - enum: [0x68, 0x69]
> >
> >    interrupts:
> >      maxItems: 1
> > @@ -57,9 +60,9 @@ examples:
> >          #address-cells = <1>;
> >          #size-cells = <0>;
> >
> > -        rtc@68 {
> > +        rtc@69 {
> >              compatible = "adi,max31335";
> > -            reg = <0x68>;
> > +            reg = <0x69>;
> 
> Why? I already asked about this - the same question "Why"

While testing, it was identified that the i2c address for max31335 is 0x69. Sorry, I will revert and send the fix in a separate patch.

> 
> 
> >              pinctrl-0 = <&rtc_nint_pins>;
> >              interrupts-extended = <&gpio1 16 IRQ_TYPE_LEVEL_HIGH>;
> >              aux-voltage-chargeable = <1>; @@ -67,4 +70,15 @@
> > examples:
> >              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";
> 
> Drop this example, not necessary.

Ok, I will remove and send in next patch.
> 
> > +        };
> > +    };
> >  ...
> >
> > --
> > 2.25.1
> >

Re: [PATCH v3 1/2] dt-bindings: rtc: max31335: Add max31331 support

[Krzysztof Kozlowski]

On 15/01/2025 11:21, U, Pavithra wrote:
>>>
>>>    interrupts:
>>>      maxItems: 1
>>> @@ -57,9 +60,9 @@ examples:
>>>          #address-cells = <1>;
>>>          #size-cells = <0>;
>>>
>>> -        rtc@68 {
>>> +        rtc@69 {
>>>              compatible = "adi,max31335";
>>> -            reg = <0x68>;
>>> +            reg = <0x69>;
>>
>> Why? I already asked about this - the same question "Why"
> 
> While testing, it was identified that the i2c address for max31335 is 0x69. Sorry, I will revert and send the fix in a separate patch.

Yes, please.



Best regards,
Krzysztof

Re: [PATCH v3 2/2] rtc: max31335: Add driver support for max31331

[Nuno Sá]

On Thu, 2025-01-09 at 15:59 +0530, PavithraUdayakumar-adi via B4 Relay wrote:
> From: PavithraUdayakumar-adi <pavithra.u@analog.com>
> 
> MAX31331 is an ultra-low-power, I2C Real-Time Clock RTC.
> 
> Signed-off-by: PavithraUdayakumar-adi <pavithra.u@analog.com>
> ---
>  drivers/rtc/rtc-max31335.c | 163 +++++++++++++++++++++++++++++++++-----------
> -
>  1 file changed, 120 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-max31335.c b/drivers/rtc/rtc-max31335.c
> index
> 3fbcf5f6b92ffd4581e9c4dbc87ec848867522dc..9d8f220fba2b21fc6393d67bff47f99b0227
> c707 100644
> --- a/drivers/rtc/rtc-max31335.c
> +++ b/drivers/rtc/rtc-max31335.c
> @@ -184,31 +184,88 @@
>  #define MAX31335_RAM_SIZE			32
>  #define MAX31335_TIME_SIZE			0x07
>  
> +/* MAX31331 Register Map */
> +#define MAX31331_RTC_CONFIG2			0x04
> +
>  #define clk_hw_to_max31335(_hw) container_of(_hw, struct max31335_data,
> clkout)
>  
> +/* Supported Maxim RTC */
> +enum max_rtc_ids {
> +	ID_MAX31331,
> +	ID_MAX31335,
> +	MAX_RTC_ID_NR
> +};
> +
> +struct chip_desc {
> +	u8 sec_reg;
> +	u8 alarm1_sec_reg;
> +
> +	u8 int_en_reg;
> +	u8 int_status_reg;
> +
> +	u8 ram_reg;
> +	u8 ram_size;
> +
> +	u8 temp_reg;
> +
> +	u8 trickle_reg;
> +
> +	u8 clkout_reg;
> +};
> +
>  struct max31335_data {
> +	enum max_rtc_ids id;
>  	struct regmap *regmap;
>  	struct rtc_device *rtc;
>  	struct clk_hw clkout;
> +	struct clk *clkin;
> +	const struct chip_desc *chip;
> +	int irq;
>  };
>  
>  static const int max31335_clkout_freq[] = { 1, 64, 1024, 32768 };
>  
> +static const struct chip_desc chip[MAX_RTC_ID_NR] = {
> +	[ID_MAX31331] = {
> +		.int_en_reg = 0x01,
> +		.int_status_reg = 0x00,
> +		.sec_reg = 0x08,
> +		.alarm1_sec_reg = 0x0F,
> +		.ram_reg = 0x20,
> +		.ram_size = 32,
> +		.trickle_reg = 0x1B,
> +		.clkout_reg = 0x04,
> +	},
> +	[ID_MAX31335] = {
> +		.int_en_reg = 0x01,
> +		.int_status_reg = 0x00,
> +		.sec_reg = 0x0A,
> +		.alarm1_sec_reg = 0x11,
> +		.ram_reg = 0x40,
> +		.ram_size = 32,
> +		.temp_reg = 0x35,
> +		.trickle_reg = 0x1D,
> +		.clkout_reg = 0x06,
> +	},
> +};
> +
>  static const u16 max31335_trickle_resistors[] = {3000, 6000, 11000};
>  
>  static bool max31335_volatile_reg(struct device *dev, unsigned int reg)
>  {
> +	struct max31335_data *max31335 = dev_get_drvdata(dev);
> +	const struct chip_desc *chip = max31335->chip;
> +
>  	/* time keeping registers */
> -	if (reg >= MAX31335_SECONDS &&
> -	    reg < MAX31335_SECONDS + MAX31335_TIME_SIZE)
> +	if (reg >= chip->sec_reg && reg < chip->sec_reg + MAX31335_TIME_SIZE)
>  		return true;
>  
>  	/* interrupt status register */
> -	if (reg == MAX31335_STATUS1)
> +	if (reg == chip->int_status_reg)
>  		return true;
>  
> -	/* temperature registers */
> -	if (reg == MAX31335_TEMP_DATA_MSB || reg == MAX31335_TEMP_DATA_LSB)
> +	/* temperature registers if valid */
> +	if (chip->temp_reg && (reg == chip->temp_reg || reg == chip->temp_reg
> + 1))
>  		return true;
>  
>  	return false;
> @@ -227,7 +284,7 @@ static int max31335_read_time(struct device *dev, struct
> rtc_time *tm)
>  	u8 date[7];
>  	int ret;
>  
> -	ret = regmap_bulk_read(max31335->regmap, MAX31335_SECONDS, date,
> +	ret = regmap_bulk_read(max31335->regmap, max31335->chip->sec_reg,
> date,
>  			       sizeof(date));
>  	if (ret)
>  		return ret;
> @@ -262,7 +319,7 @@ static int max31335_set_time(struct device *dev, struct
> rtc_time *tm)
>  	if (tm->tm_year >= 200)
>  		date[5] |= FIELD_PREP(MAX31335_MONTH_CENTURY, 1);
>  
> -	return regmap_bulk_write(max31335->regmap, MAX31335_SECONDS, date,
> +	return regmap_bulk_write(max31335->regmap, max31335->chip->sec_reg,
> date,
>  				 sizeof(date));
>  }
>  
> @@ -273,7 +330,7 @@ static int max31335_read_alarm(struct device *dev, struct
> rtc_wkalrm *alrm)
>  	struct rtc_time time;
>  	u8 regs[6];
>  
> -	ret = regmap_bulk_read(max31335->regmap, MAX31335_ALM1_SEC, regs,
> +	ret = regmap_bulk_read(max31335->regmap, max31335->chip-
> >alarm1_sec_reg, regs,
>  			       sizeof(regs));
>  	if (ret)
>  		return ret;
> @@ -292,11 +349,11 @@ static int max31335_read_alarm(struct device *dev,
> struct rtc_wkalrm *alrm)
>  	if (time.tm_year >= 200)
>  		alrm->time.tm_year += 100;
>  
> -	ret = regmap_read(max31335->regmap, MAX31335_INT_EN1, &ctrl);
> +	ret = regmap_read(max31335->regmap, max31335->chip->int_en_reg,
> &ctrl);
>  	if (ret)
>  		return ret;
>  
> -	ret = regmap_read(max31335->regmap, MAX31335_STATUS1, &status);
> +	ret = regmap_read(max31335->regmap, max31335->chip->int_status_reg,
> &status);
>  	if (ret)
>  		return ret;
>  
> @@ -320,18 +377,18 @@ static int max31335_set_alarm(struct device *dev, struct
> rtc_wkalrm *alrm)
>  	regs[4] = bin2bcd(alrm->time.tm_mon + 1);
>  	regs[5] = bin2bcd(alrm->time.tm_year % 100);
>  
> -	ret = regmap_bulk_write(max31335->regmap, MAX31335_ALM1_SEC,
> +	ret = regmap_bulk_write(max31335->regmap, max31335->chip-
> >alarm1_sec_reg,
>  				regs, sizeof(regs));
>  	if (ret)
>  		return ret;
>  
>  	reg = FIELD_PREP(MAX31335_INT_EN1_A1IE, alrm->enabled);
> -	ret = regmap_update_bits(max31335->regmap, MAX31335_INT_EN1,
> +	ret = regmap_update_bits(max31335->regmap, max31335->chip-
> >int_en_reg,
>  				 MAX31335_INT_EN1_A1IE, reg);
>  	if (ret)
>  		return ret;
>  
> -	ret = regmap_update_bits(max31335->regmap, MAX31335_STATUS1,
> +	ret = regmap_update_bits(max31335->regmap, max31335->chip-
> >int_status_reg,
>  				 MAX31335_STATUS1_A1F, 0);
>  
>  	return 0;
> @@ -341,23 +398,33 @@ static int max31335_alarm_irq_enable(struct device *dev,
> unsigned int enabled)
>  {
>  	struct max31335_data *max31335 = dev_get_drvdata(dev);
>  
> -	return regmap_update_bits(max31335->regmap, MAX31335_INT_EN1,
> +	return regmap_update_bits(max31335->regmap, max31335->chip-
> >int_en_reg,
>  				  MAX31335_INT_EN1_A1IE, enabled);
>  }
>  
>  static irqreturn_t max31335_handle_irq(int irq, void *dev_id)
>  {
>  	struct max31335_data *max31335 = dev_id;
> -	bool status;
> -	int ret;
> +	struct mutex *lock = &max31335->rtc->ops_lock;
> +	int ret, status;
> +
> +	mutex_lock(lock);
>  
> -	ret = regmap_update_bits_check(max31335->regmap, MAX31335_STATUS1,
> -				       MAX31335_STATUS1_A1F, 0, &status);
> +	ret = regmap_read(max31335->regmap, max31335->chip->int_status_reg,
> &status);
>  	if (ret)
> -		return IRQ_HANDLED;
> +		goto exit;
> +
> +	if (FIELD_GET(MAX31335_STATUS1_A1F, status)) {
> +		ret = regmap_update_bits(max31335->regmap, max31335->chip-
> >int_status_reg,
> +					 MAX31335_STATUS1_A1F, 0);
> +		if (ret)
> +			goto exit;
>  
> -	if (status)
>  		rtc_update_irq(max31335->rtc, 1, RTC_AF | RTC_IRQF);
> +	}
> +
> +exit:
> +	mutex_unlock(lock);
>  
>  	return IRQ_HANDLED;
>  }
> @@ -404,7 +471,7 @@ static int max31335_trickle_charger_setup(struct device
> *dev,
>  
>  	i = i + trickle_cfg;
>  
> -	return regmap_write(max31335->regmap, MAX31335_TRICKLE_REG,
> +	return regmap_write(max31335->regmap, max31335->chip->trickle_reg,
>  			    FIELD_PREP(MAX31335_TRICKLE_REG_TRICKLE, i) |
>  			    FIELD_PREP(MAX31335_TRICKLE_REG_EN_TRICKLE,
>  				       chargeable));
> @@ -418,7 +485,7 @@ static unsigned long max31335_clkout_recalc_rate(struct
> clk_hw *hw,
>  	unsigned int reg;
>  	int ret;
>  
> -	ret = regmap_read(max31335->regmap, MAX31335_RTC_CONFIG2, &reg);
> +	ret = regmap_read(max31335->regmap, max31335->chip->clkout_reg,
> &reg);
>  	if (ret)
>  		return 0;
>  
> @@ -449,23 +516,23 @@ static int max31335_clkout_set_rate(struct clk_hw *hw,
> unsigned long rate,
>  			     ARRAY_SIZE(max31335_clkout_freq));
>  	freq_mask = __roundup_pow_of_two(ARRAY_SIZE(max31335_clkout_freq)) -
> 1;
>  
> -	return regmap_update_bits(max31335->regmap, MAX31335_RTC_CONFIG2,
> -				  freq_mask, index);
> +	return regmap_update_bits(max31335->regmap, max31335->chip-
> >clkout_reg,
> +				 freq_mask, index);
>  }
>  
>  static int max31335_clkout_enable(struct clk_hw *hw)
>  {
>  	struct max31335_data *max31335 = clk_hw_to_max31335(hw);
>  
> -	return regmap_set_bits(max31335->regmap, MAX31335_RTC_CONFIG2,
> -			       MAX31335_RTC_CONFIG2_ENCLKO);
> +	return regmap_set_bits(max31335->regmap, max31335->chip->clkout_reg,
> +			      MAX31335_RTC_CONFIG2_ENCLKO);
>  }
>  
>  static void max31335_clkout_disable(struct clk_hw *hw)
>  {
>  	struct max31335_data *max31335 = clk_hw_to_max31335(hw);
>  
> -	regmap_clear_bits(max31335->regmap, MAX31335_RTC_CONFIG2,
> +	regmap_clear_bits(max31335->regmap, max31335->chip->clkout_reg,
>  			  MAX31335_RTC_CONFIG2_ENCLKO);
>  }
>  
> @@ -475,7 +542,7 @@ static int max31335_clkout_is_enabled(struct clk_hw *hw)
>  	unsigned int reg;
>  	int ret;
>  
> -	ret = regmap_read(max31335->regmap, MAX31335_RTC_CONFIG2, &reg);
> +	ret = regmap_read(max31335->regmap, max31335->chip->clkout_reg,
> &reg);
>  	if (ret)
>  		return ret;
>  
> @@ -500,7 +567,7 @@ static int max31335_nvmem_reg_read(void *priv, unsigned
> int offset,
>  				   void *val, size_t bytes)
>  {
>  	struct max31335_data *max31335 = priv;
> -	unsigned int reg = MAX31335_TS0_SEC_1_128 + offset;
> +	unsigned int reg = max31335->chip->ram_reg + offset;
>  
>  	return regmap_bulk_read(max31335->regmap, reg, val, bytes);
>  }
> @@ -509,7 +576,7 @@ static int max31335_nvmem_reg_write(void *priv, unsigned
> int offset,
>  				    void *val, size_t bytes)
>  {
>  	struct max31335_data *max31335 = priv;
> -	unsigned int reg = MAX31335_TS0_SEC_1_128 + offset;
> +	unsigned int reg = max31335->chip->ram_reg + offset;
>  
>  	return regmap_bulk_write(max31335->regmap, reg, val, bytes);
>  }
> @@ -533,7 +600,7 @@ static int max31335_read_temp(struct device *dev, enum
> hwmon_sensor_types type,
>  	if (type != hwmon_temp || attr != hwmon_temp_input)
>  		return -EOPNOTSUPP;
>  
> -	ret = regmap_bulk_read(max31335->regmap, MAX31335_TEMP_DATA_MSB,
> +	ret = regmap_bulk_read(max31335->regmap, max31335->chip->temp_reg,
>  			       reg, 2);
>  	if (ret)
>  		return ret;
> @@ -577,8 +644,8 @@ static int max31335_clkout_register(struct device *dev)
>  	int ret;
>  
>  	if (!device_property_present(dev, "#clock-cells"))
> -		return regmap_clear_bits(max31335->regmap,
> MAX31335_RTC_CONFIG2,
> -					 MAX31335_RTC_CONFIG2_ENCLKO);
> +		return regmap_clear_bits(max31335->regmap, max31335->chip-
> >clkout_reg,
> +				  MAX31335_RTC_CONFIG2_ENCLKO);
>  
>  	max31335->clkout.init = &max31335_clk_init;
>  
> @@ -605,6 +672,7 @@ static int max31335_probe(struct i2c_client *client)
>  #if IS_REACHABLE(HWMON)
>  	struct device *hwmon;
>  #endif
> +	const struct chip_desc *match;
>  	int ret;
>  
>  	max31335 = devm_kzalloc(&client->dev, sizeof(*max31335), GFP_KERNEL);
> @@ -616,7 +684,11 @@ static int max31335_probe(struct i2c_client *client)
>  		return PTR_ERR(max31335->regmap);
>  
>  	i2c_set_clientdata(client, max31335);
> -
> +	match = i2c_get_match_data(client);
> +	if (!match)
> +		return -ENODEV;
> +	max31335->chip = match;
> +	max31335->id = max31335->chip - chip;

I kind of already expressed this internally... I can't agree with the above. Why
not making 'id' a member of 'struct chip_desc'? The above is very useless IMHO.

- Nuno Sá