-
Notifications
You must be signed in to change notification settings - Fork 833
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RTC: MAX31331: Support and documentation for RTC MAX31331 #2593
base: main
Are you sure you want to change the base?
Conversation
8d299ca
to
ad28b9f
Compare
ad28b9f
to
37e49c7
Compare
MAX31331 is an ultra-low-power, I2C Real-Time Clock RTC Signed-off-by: Swaroop Kukkillaya <[email protected]>
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 <[email protected]>
37e49c7
to
9030670
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
||
reg: | ||
maxItems: 1 | ||
description: I2C address of the RTC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need for description
|
||
interrupts: | ||
description: | | ||
Alarm1 interrupt line of the RTC. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
properties: | ||
reg: | ||
items: | ||
- const: 0x68 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
@@ -59,7 +82,7 @@ examples: | |||
|
|||
rtc@68 { | |||
compatible = "adi,max31335"; | |||
reg = <0x68>; | |||
reg = <0x69>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't change this. Why going in favor of the new reg? If you really want, add another example for the new device.
ID_MAX31331, | ||
ID_MAX31335, | ||
MAX_RTC_ID_NR | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make the ids part of chip_desc
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 */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would drop the comment
if (max31335->id == ID_MAX31331) | ||
ret = regmap_read(max31335->regmap, MAX31331_RTC_CONFIG2, ®); | ||
else | ||
ret = regmap_read(max31335->regmap, MAX31335_RTC_CONFIG2, ®); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
if (!device_property_present(dev, "#clock-cells")) { | ||
if (max31335->id == ID_MAX31331) | ||
return 0; | ||
else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
int ret; | ||
|
||
max31335 = devm_kzalloc(&client->dev, sizeof(*max31335), GFP_KERNEL); | ||
if (!max31335) | ||
return -ENOMEM; | ||
|
||
dev_set_drvdata(&client->dev, max31335); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? Note the call to i2c_set_clientdata(client, max31335);
else | ||
return -ENODEV; | ||
|
||
max31335->id = max31335->chip - chip; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use i2c_get_match_data()
and check for NULL (return error in that case)
PR Description
PR Type
PR Checklist