RTC: MAX31331: Support and documentation for RTC MAX31331 #2593

SwaroopPKADI · 2024-09-06T06:45:33Z

PR Description

RTC: MAX31331: Support for MAX31331 and updated the documents.

PR Type

Bug fix (a change that fixes an issue)
New feature (a change that adds new functionality)
Breaking change (a change that affects other repos or cause CIs to fail)

PR Checklist

I have conducted a self-review of my own code changes
I have tested the changes on the relevant hardware
I have updated the documentation outside this repo accordingly (if there is the case)

nunojsa

Note that I'm expecting this to be first upstreamed before merging it in our tree...

Also, make sure you change your commit titles. Run:

git log --oneline Documentation/devicetree/bindings
git log --oneline drivers/rtc

and use the same style.

nunojsa · 2024-09-20T07:57:23Z

Documentation/devicetree/bindings/rtc/adi,max31335.yaml


  reg:
-    maxItems: 1
+    description: I2C address of the RTC


no need for description

nunojsa · 2024-09-20T07:57:28Z

Documentation/devicetree/bindings/rtc/adi,max31335.yaml


  interrupts:
+    description: |
+      Alarm1 interrupt line of the RTC.


nunojsa · 2024-09-20T08:01:10Z

Documentation/devicetree/bindings/rtc/adi,max31335.yaml

+      properties:
+        reg:
+          items:
+            - const: 0x68


I think, typically, this comes after required: but I see that you're already making use of allOf: here. Maybe it's acceptable to move it to the end. Not sure, up to you...

nunojsa · 2024-09-20T08:01:45Z

Documentation/devicetree/bindings/rtc/adi,max31335.yaml

@@ -59,7 +82,7 @@ examples:

        rtc@68 {
            compatible = "adi,max31335";
-            reg = <0x68>;
+            reg = <0x69>;


Don't change this. Why going in favor of the new reg? If you really want, add another example for the new device.

nunojsa · 2024-09-20T08:02:26Z

drivers/rtc/rtc-max31335.c

+	ID_MAX31331,
+	ID_MAX31335,
+	MAX_RTC_ID_NR
+};


Make the ids part of chip_desc

The id is a part of max31335_data. chip_desc is being used for the register address mapping between multiple RTCs.

Would making it a part of chip_desc be more useful than keeping it out of it?

Why doing the below?

max31335->id = max31335->chip - chip;

when the id is already a constant piece of information? Just make it part of that struct and be done with it :)

nunojsa · 2024-09-20T08:04:09Z

drivers/rtc/rtc-max31335.c

 			       sizeof(date));
 	if (ret)
 		return ret;

 	tm->tm_sec  = bcd2bin(date[0] & 0x7f);
 	tm->tm_min  = bcd2bin(date[1] & 0x7f);
+	/* Hour is always set to 24Hr format */


I would drop the comment

nunojsa · 2024-09-20T08:05:13Z

drivers/rtc/rtc-max31335.c

+	if (max31335->id == ID_MAX31331)
+		ret = regmap_read(max31335->regmap, MAX31331_RTC_CONFIG2, &reg);
+	else
+		ret = regmap_read(max31335->regmap, MAX31335_RTC_CONFIG2, &reg);


You can make the config register part of your chip_info struct and get rid of the if(). It even looks like you can actually remove the enum...

nunojsa · 2024-09-20T08:09:44Z

drivers/rtc/rtc-max31335.c

+	if (!device_property_present(dev, "#clock-cells")) {
+		if (max31335->id == ID_MAX31331)
+			return 0;
+		else


Redundant else... Why do we return 0 in the other case? Are the bits not part of the register? But I assume we still have an output clock to register?

This is the only case where the id seems to be needed. Not sure what those bits are but maybe you can also have a dedicated boolean in your chip_info to make the decision in here.

nunojsa · 2024-09-20T08:11:46Z

drivers/rtc/rtc-max31335.c

 	int ret;

 	max31335 = devm_kzalloc(&client->dev, sizeof(*max31335), GFP_KERNEL);
 	if (!max31335)
 		return -ENOMEM;

+	dev_set_drvdata(&client->dev, max31335);


Why? Note the call to i2c_set_clientdata(client, max31335);

nunojsa · 2024-09-20T08:12:47Z

drivers/rtc/rtc-max31335.c

+	else
+		return -ENODEV;
+
+	max31335->id = max31335->chip - chip;


Use i2c_get_match_data() and check for NULL (return error in that case)

Couldn't find a definition for i2c_get_match_data()

It might be it's yet not supported in our tree. But it is upstream and that what you should use when upstreaming this driver

MAX31331 is an ultra-low-power, I2C Real-Time Clock RTC Signed-off-by: Swaroop Kukkillaya <Swaroop.PrakashKumar@analog.com>

Add details to use max31331. The main difference between max31331 and max31335 is the I2C Slave Address. Max31331 uses 0x68 where as max31335 uses 0x69. Signed-off-by: Swaroop Kukkillaya <Swaroop.PrakashKumar@analog.com>

nunojsa

There are things that I still don't really agree but I would say to send it upstream and see what the maintainers have to say...

nunojsa · 2024-11-04T10:27:01Z

Documentation/devicetree/bindings/rtc/adi,max31335.yaml


  reg:
-    maxItems: 1
+    items:
+      - enum: [0x68, 0x69]


I thin you can just keep maxItems. Below you already make it clear the addresses.

I'll also take the opportunity to advise on the commit message. Maintainers are getting more strict about having fallbacks devices. In short fallback is a way for an older kernel (which does not contain the new device driver - in this case max31331) to still be able to use and probe the fallback device (in this case max31335). The reasoning is that we can't still operate the device but just missing some features.

That said, if they are not compatible at all (i.e - we can't fallback into the other device), we need to state why is that in the commit message.

nunojsa · 2024-11-04T10:28:35Z

drivers/rtc/rtc-max31335.c

+	else
+		return -ENODEV;
+
+	max31335->id = max31335->chip - chip;


This is still nok IMO. But if you really think it's ok like, I won't argue more :)

SwaroopPKADI requested review from nunojsa, mhennerich, dbogdan, btogorean, chegbeli, buha, amiclaus, rbolboac, danmois, CiprianRegus and machschmitt as code owners September 6, 2024 06:45

SwaroopPKADI force-pushed the rtc-max31331 branch from 8d299ca to ad28b9f Compare September 6, 2024 07:13

SwaroopPKADI marked this pull request as draft September 6, 2024 07:35

SwaroopPKADI force-pushed the rtc-max31331 branch 2 times, most recently from 37e49c7 to 9030670 Compare September 6, 2024 09:40

SwaroopPKADI marked this pull request as ready for review September 6, 2024 11:59

nunojsa reviewed Sep 20, 2024

View reviewed changes

SwaroopPKADI force-pushed the rtc-max31331 branch from 9030670 to e697b9b Compare October 17, 2024 06:22

SwaroopPKADI added 2 commits October 17, 2024 16:14

rtc: max31335: Add driver support for MAX31331

7d69eef

MAX31331 is an ultra-low-power, I2C Real-Time Clock RTC Signed-off-by: Swaroop Kukkillaya <Swaroop.PrakashKumar@analog.com>

dt-bindings: rtc: max31335: Add max31331 support

6f413ca

Add details to use max31331. The main difference between max31331 and max31335 is the I2C Slave Address. Max31331 uses 0x68 where as max31335 uses 0x69. Signed-off-by: Swaroop Kukkillaya <Swaroop.PrakashKumar@analog.com>

SwaroopPKADI force-pushed the rtc-max31331 branch from e697b9b to 6f413ca Compare October 17, 2024 10:46

nunojsa reviewed Nov 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RTC: MAX31331: Support and documentation for RTC MAX31331 #2593

RTC: MAX31331: Support and documentation for RTC MAX31331 #2593

SwaroopPKADI commented Sep 6, 2024

nunojsa left a comment

nunojsa Sep 20, 2024

nunojsa Sep 20, 2024

nunojsa Sep 20, 2024

nunojsa Sep 20, 2024

nunojsa Sep 20, 2024

SwaroopPKADI Oct 16, 2024

nunojsa Oct 21, 2024

nunojsa Sep 20, 2024

nunojsa Sep 20, 2024

nunojsa Sep 20, 2024

nunojsa Sep 20, 2024

nunojsa Sep 20, 2024

SwaroopPKADI Oct 17, 2024

nunojsa Oct 21, 2024

nunojsa left a comment

nunojsa Nov 4, 2024

nunojsa Nov 4, 2024

RTC: MAX31331: Support and documentation for RTC MAX31331 #2593

Are you sure you want to change the base?

RTC: MAX31331: Support and documentation for RTC MAX31331 #2593

Conversation

SwaroopPKADI commented Sep 6, 2024

PR Description

PR Type

PR Checklist

nunojsa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nunojsa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment