Hi Alexandre,
On 8. 06. 23 11:31, Alexandre Belloni wrote:
Hello,
On 08/06/2023 11:04:46+0200, Andrej Picej wrote:
+ ret = regmap_read(rv3028->regmap, RV3028_BACKUP, &val_old);
+ if (ret < 0)
+ return ret;
+
+ /* mask out only trickle charger bits */
+ val_old = val_old & (RV3028_BACKUP_TCE | RV3028_BACKUP_TCR_MASK);
+
/* setup trickle charger */
- if (!device_property_read_u32(&client->dev, "trickle-resistor-ohms",
- &ohms)) {
+ if (device_property_read_u32(&client->dev, "trickle-resistor-ohms", &ohms)) {
+ /* disable the trickle charger */
+ val = 0;
You can't do that, this will break existing users that may set the
trickle charger from their bootloader for example.
hmm...ok I understand that idea. I thought that might be a problem. I
guess keeping default as it is has a higher priority.
What do you say if setting this property to 0 (or maybe -1) disabled the
trickle charger?
So if users add:
trickle-resistor-ohms = <0>;
or
trickle-resistor-ohms = <(-1)>;
this would mean disable the trickle charger?
I know it is far from optimal, but this would solve both things:
* not braking existing implementation,
* users could disable the trickle charger.
What do you say.
Thanks for your review.
Best regards,
Andrej
+ } else {
int i;
for (i = 0; i < ARRAY_SIZE(rv3028_trickle_resistors); i++)
@@ -947,15 +957,21 @@ static int rv3028_probe(struct i2c_client *client)
break;
if (i < ARRAY_SIZE(rv3028_trickle_resistors)) {
- ret = rv3028_update_cfg(rv3028, RV3028_BACKUP, RV3028_BACKUP_TCE |
- RV3028_BACKUP_TCR_MASK, RV3028_BACKUP_TCE | i);
- if (ret)
- return ret;
+ /* enable the trickle charger and setup its resistor accordingly */
+ val = RV3028_BACKUP_TCE | i;
} else {
dev_warn(&client->dev, "invalid trickle resistor value\n");
}
}
+ /* only update EEPROM if changes are necessary */
+ if (val_old != val) {
+ ret = rv3028_update_cfg(rv3028, RV3028_BACKUP, RV3028_BACKUP_TCE |
+ RV3028_BACKUP_TCR_MASK, val);
+ if (ret)
+ return ret;
+ }
+
ret = rtc_add_group(rv3028->rtc, &rv3028_attr_group);
if (ret)
return ret;
--
2.25.1
