Skip to contentSkip to conversation

rtc: ds1307: fix ds1339 wakealarm support

PATCH In review Soeren Moch 2 participants 5 messages Oct 22, 2018

Subsystems (2)
Stable branch Real time clock (RTC)
lore
Share & tools
lore

Soeren Moch

author
#0 lore raw

PATCH 1/1 rtc: ds1307: fix ds1339 wakealarm support

Commit 51ed73eb998a1c79a2b0e9bed68f75a8a2c93b9b ("rtc: ds1340: Add support for trickle charger.") breaks ds1339 wakealarm support by limiting accessible registers. Fix this.

Fixes 51ed73eb998a ("rtc: ds1340: Add support for trickle charger.")
Cc [email protected]
Signed-off-by Soeren Moch <[email protected]>

--
Cc: Andrea Greco <[email protected]>
Cc: Alessandro Zummo <[email protected]>
Cc: Alexandre Belloni <[email protected]>
Cc: [email protected]
Cc: [email protected]

---
 drivers/rtc/rtc-ds1307.c | 1 -
 1 file changed, 1 deletion(-)
drivers/rtc/rtc-ds1307.c +0 -1
index 4b2b4627daeb..71396b62dc52 100644
@@ -1384,7 +1384,6 @@ static void ds1307_clks_register(struct ds1307 *ds1307)
 static const struct regmap_config regmap_config = {
 	.reg_bits = 8,
 	.val_bits = 8,
-	.max_register = 0x9,
 };
 
 static int ds1307_probe(struct i2c_client *client,
Signature · 1 line
2.17.1
maintainer
#1 lore raw

Hello,

Quoted 22 lines · Soeren Moch

Commit 51ed73eb998a1c79a2b0e9bed68f75a8a2c93b9b ("rtc: ds1340: Add support for trickle charger.") breaks ds1339 wakealarm support by limiting accessible registers. Fix this.

Fixes 51ed73eb998a ("rtc: ds1340: Add support for trickle charger.")
Cc [email protected]
Signed-off-by Soeren Moch <[email protected]>

--
Cc: Andrea Greco <[email protected]>
Cc: Alessandro Zummo <[email protected]>
Cc: Alexandre Belloni <[email protected]>
Cc: [email protected]
Cc: [email protected]

---
 drivers/rtc/rtc-ds1307.c | 1 -
 1 file changed, 1 deletion(-)
drivers/rtc/rtc-ds1307.c +0 -0
index 4b2b4627daeb..71396b62dc52 100644

drivers/rtc/rtc-ds1307.c static void ds1307_clks_register(struct ds1307 *ds1307) · line 1387

Quoted 4 lines
drivers/rtc/rtc-ds1307.c +0 -1
@@ -1384,7 +1384,6 @@ static void ds1307_clks_register(struct ds1307 *ds1307)
 static const struct regmap_config regmap_config = {
 	.reg_bits = 8,
 	.val_bits = 8,
-	.max_register = 0x9,

I would prefer having a different regmap_config for chips that have more registers. I guess you could put a pointer to a regmap_config in chip_desc.

Signature · 3 lines
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
author
#2 lore raw

Hi,

Am 22.10.2018 um 15:57 schrieb Alexandre Belloni:

Quoted 20 lines

Hello,

Quoted 17 lines · Soeren Moch

Commit 51ed73eb998a1c79a2b0e9bed68f75a8a2c93b9b ("rtc: ds1340: Add support for trickle charger.") breaks ds1339 wakealarm support by limiting accessible registers. Fix this.

Fixes 51ed73eb998a ("rtc: ds1340: Add support for trickle charger.")
Cc [email protected]
Signed-off-by Soeren Moch <[email protected]>

--
Cc: Andrea Greco <[email protected]>
Cc: Alessandro Zummo <[email protected]>
Cc: Alexandre Belloni <[email protected]>
Cc: [email protected]
Cc: [email protected]

---
 drivers/rtc/rtc-ds1307.c | 1 -
 1 file changed, 1 deletion(-)
Quoted 12 lines

drivers/rtc/rtc-ds1307.c static void ds1307_clks_register(struct ds1307 *ds1307) · line 1387

Quoted 9 lines
drivers/rtc/rtc-ds1307.c +0 -1
index 4b2b4627daeb..71396b62dc52 100644
@@ -1384,7 +1384,6 @@ static void ds1307_clks_register(struct ds1307 *ds1307)
 static const struct regmap_config regmap_config = {
 	.reg_bits = 8,
 	.val_bits = 8,
-	.max_register = 0x9,

I would prefer having a different regmap_config for chips that have more registers. I guess you could put a pointer to a regmap_config in chip_desc.

What looks strange to me, the original patch (according to its description) enabled trickle charger functionality for a single chip. This is something totally different than limiting the register space for all chips that are supported by this driver. So this seems to be an accident to me. And this causes an regression for ds1339, which was supported for years.

Are there any advantages for limiting the address space of i2c devices? If we really need this, can we apply this partial revert as regression fix (also for stable) and implement the address space limitation separately?

Thanks,
Soeren

maintainer
#3 lore raw
Quoted 35 lines · Soeren Moch

Hi,

Am 22.10.2018 um 15:57 schrieb Alexandre Belloni:

Quoted 20 lines

Hello,

Quoted 17 lines · Soeren Moch

Commit 51ed73eb998a1c79a2b0e9bed68f75a8a2c93b9b ("rtc: ds1340: Add support for trickle charger.") breaks ds1339 wakealarm support by limiting accessible registers. Fix this.

Fixes 51ed73eb998a ("rtc: ds1340: Add support for trickle charger.")
Cc [email protected]
Signed-off-by Soeren Moch <[email protected]>

--
Cc: Andrea Greco <[email protected]>
Cc: Alessandro Zummo <[email protected]>
Cc: Alexandre Belloni <[email protected]>
Cc: [email protected]
Cc: [email protected]

---
 drivers/rtc/rtc-ds1307.c | 1 -
 1 file changed, 1 deletion(-)
Quoted 12 lines

drivers/rtc/rtc-ds1307.c static void ds1307_clks_register(struct ds1307 *ds1307) · line 1387

Quoted 9 lines
drivers/rtc/rtc-ds1307.c +0 -1
index 4b2b4627daeb..71396b62dc52 100644
@@ -1384,7 +1384,6 @@ static void ds1307_clks_register(struct ds1307 *ds1307)
 static const struct regmap_config regmap_config = {
 	.reg_bits = 8,
 	.val_bits = 8,
-	.max_register = 0x9,

I would prefer having a different regmap_config for chips that have more registers. I guess you could put a pointer to a regmap_config in chip_desc.

Quoted 8 lines

What looks strange to me, the original patch (according to its description) enabled trickle charger functionality for a single chip. This is something totally different than limiting the register space for all chips that are supported by this driver. So this seems to be an accident to me. And this causes an regression for ds1339, which was supported for years.

Are there any advantages for limiting the address space of i2c devices?

I would think it was done as a facility to debug the feature because when max_register is set, regmap will allow you to dump the register space.

Quoted 3 lines

If we really need this, can we apply this partial revert as regression fix (also for stable) and implement the address space limitation separately?

Fine, I'm applying this patch right now.

Signature · 3 lines
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
author
#4 lore raw

Am 22.10.2018 um 17:52 schrieb Alexandre Belloni:

Quoted 44 lines · Soeren Moch
Quoted 35 lines

Hi,

Am 22.10.2018 um 15:57 schrieb Alexandre Belloni:

Quoted 20 lines

Hello,

Quoted 17 lines · Soeren Moch

Commit 51ed73eb998a1c79a2b0e9bed68f75a8a2c93b9b ("rtc: ds1340: Add support for trickle charger.") breaks ds1339 wakealarm support by limiting accessible registers. Fix this.

Fixes 51ed73eb998a ("rtc: ds1340: Add support for trickle charger.")
Cc [email protected]
Signed-off-by Soeren Moch <[email protected]>

--
Cc: Andrea Greco <[email protected]>
Cc: Alessandro Zummo <[email protected]>
Cc: Alexandre Belloni <[email protected]>
Cc: [email protected]
Cc: [email protected]

---
 drivers/rtc/rtc-ds1307.c | 1 -
 1 file changed, 1 deletion(-)
Quoted 12 lines

drivers/rtc/rtc-ds1307.c static void ds1307_clks_register(struct ds1307 *ds1307) · line 1387

Quoted 9 lines
drivers/rtc/rtc-ds1307.c +0 -1
index 4b2b4627daeb..71396b62dc52 100644
@@ -1384,7 +1384,6 @@ static void ds1307_clks_register(struct ds1307 *ds1307)
 static const struct regmap_config regmap_config = {
 	.reg_bits = 8,
 	.val_bits = 8,
-	.max_register = 0x9,

I would prefer having a different regmap_config for chips that have more registers. I guess you could put a pointer to a regmap_config in chip_desc.

Quoted 8 lines

What looks strange to me, the original patch (according to its description) enabled trickle charger functionality for a single chip. This is something totally different than limiting the register space for all chips that are supported by this driver. So this seems to be an accident to me. And this causes an regression for ds1339, which was supported for years.

Are there any advantages for limiting the address space of i2c devices?

Quoted 9 lines

I would think it was done as a facility to debug the feature because when max_register is set, regmap will allow you to dump the register space.

Quoted 3 lines

If we really need this, can we apply this partial revert as regression fix (also for stable) and implement the address space limitation separately?

Fine, I'm applying this patch right now.

OK, thank you,
Soeren