power: supply: Use an rbtree rather than flat register cache
PATCH 1/1 power: supply: Use an rbtree rather than flat register cache
The smb347 has a very sparse register map (the maximum register is 0x3f but less than 10% of the possible registers appear to be defined) and doesn't have any hardware defaults specified so the sparser data structure of an rbtree is a better fit for it's needs than a flat cache. Since it uses I2C for the control interface there is no performance concern with the slightly more involved code so let's convert it.
This will mean we avoid any issues created by assuming that any previously unaccessed registers hold a value that doesn't match what's in the hardware (eg, an _update_bits() suppressing a write).
--- drivers/power/supply/smb347-charger.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/power/supply/smb347-charger.c +1 -2
@@ -1488,8 +1488,7 @@ static const struct regmap_config smb347_regmap = { .max_register = SMB347_MAX_REGISTER, .volatile_reg = smb347_volatile_reg, .readable_reg = smb347_readable_reg, - .cache_type = REGCACHE_FLAT, - .num_reg_defaults_raw = SMB347_MAX_REGISTER, + .cache_type = REGCACHE_RBTREE, };static const struct regulator_ops smb347_usb_vbus_regulator_ops = {
Signature · 1 line
2.30.2
Hi,
Quoted 7 lines · Mark Brown
The smb347 has a very sparse register map (the maximum register is
0x3fbut less than 10% of the possible registers appear to be defined) and doesn't have any hardware defaults specified so the sparser data structure of an rbtree is a better fit for it's needs than a flat cache. Since it uses I2C for the control interface there is no performance concern with the slightly more involved code so let's convert it.
Quoted 6 lines
This will mean we avoid any issues created by assuming that any previously unaccessed registers hold a value that doesn't match what's in the hardware (eg, an
_update_bits()suppressing a write).Signed-off-by Mark Brown <[email protected]>---
Thanks, queued.
-- Sebastian
Quoted 20 lines
drivers/power/supply/smb347-charger.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)drivers/power/supply/smb347-charger.c +1
-2@@ -1488,8 +1488,7 @@ static const struct regmap_config smb347_regmap = { .max_register = SMB347_MAX_REGISTER, .volatile_reg = smb347_volatile_reg, .readable_reg = smb347_readable_reg, - .cache_type = REGCACHE_FLAT, - .num_reg_defaults_raw = SMB347_MAX_REGISTER, + .cache_type = REGCACHE_RBTREE, };static const struct regulator_ops smb347_usb_vbus_regulator_ops = {Signature · 1 line
2.30.2
23.02.2022 00:43, Mark Brown пишет:
Quoted 21 lines
The smb347 has a very sparse register map (the maximum register is
0x3fbut less than 10% of the possible registers appear to be defined) and doesn't have any hardware defaults specified so the sparser data structure of an rbtree is a better fit for it's needs than a flat cache. Since it uses I2C for the control interface there is no performance concern with the slightly more involved code so let's convert it.This will mean we avoid any issues created by assuming that any previously unaccessed registers hold a value that doesn't match what's in the hardware (eg, an
_update_bits()suppressing a write).Signed-off-by Mark Brown <[email protected]>--- drivers/power/supply/smb347-charger.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)drivers/power/supply/smb347-charger.c +0
-0
drivers/power/supply/smb347-charger.c static const struct regmap_config smb347_regmap = { · line 1491
Quoted 6 lines
drivers/power/supply/smb347-charger.c +1
-2@@ -1488,8 +1488,7 @@ static const struct regmap_config smb347_regmap = { .max_register = SMB347_MAX_REGISTER, .volatile_reg = smb347_volatile_reg, .readable_reg = smb347_readable_reg, - .cache_type = REGCACHE_FLAT, - .num_reg_defaults_raw = SMB347_MAX_REGISTER, + .cache_type = REGCACHE_RBTREE,
Why you removed the num_reg_defaults_raw? It was needed in order to populate the default values on the regcache init, is it somehow different for the REGCACHE_RBTREE? Otherwise this patch is incorrect.
24.02.2022 14:07, Dmitry Osipenko пишет:
Quoted 17 lines
23.02.2022 00:43, Mark Brown пишет:
Quoted 16 lines
The smb347 has a very sparse register map (the maximum register is
0x3fbut less than 10% of the possible registers appear to be defined) and doesn't have any hardware defaults specified so the sparser data structure of an rbtree is a better fit for it's needs than a flat cache. Since it uses I2C for the control interface there is no performance concern with the slightly more involved code so let's convert it.This will mean we avoid any issues created by assuming that any previously unaccessed registers hold a value that doesn't match what's in the hardware (eg, an
_update_bits()suppressing a write).Signed-off-by Mark Brown <[email protected]>--- drivers/power/supply/smb347-charger.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
Quoted 15 lines
drivers/power/supply/smb347-charger.c static const struct regmap_config smb347_regmap = { · line 1491
Quoted 11 lines
drivers/power/supply/smb347-charger.c +1
-2@@ -1488,8 +1488,7 @@ static const struct regmap_config smb347_regmap = { .max_register = SMB347_MAX_REGISTER, .volatile_reg = smb347_volatile_reg, .readable_reg = smb347_readable_reg, - .cache_type = REGCACHE_FLAT, - .num_reg_defaults_raw = SMB347_MAX_REGISTER, + .cache_type = REGCACHE_RBTREE,Why you removed the
num_reg_defaults_raw? It was needed in order to populate the default values on the regcache init, is it somehow different for theREGCACHE_RBTREE? Otherwise this patch is incorrect.
To make it more clear.. there was no problem with "suppressing a write", the problem was that regmap reads are done from the cache, and if cache isn't per-initialized, then you read zeros from the cache until write is made.
Quoted 4 lines · Dmitry Osipenko
To make it more clear.. there was no problem with "suppressing a write", the problem was that regmap reads are done from the cache, and if cache isn't per-initialized, then you read zeros from the cache until write is made.
With update_bits() that can result in an attempt to set a bit to zero not actually taking effect if it believes that the bit was already set to zero and there has been no change in the value of the register.
24.02.2022 15:08, Mark Brown пишет:
Quoted 10 lines · Dmitry Osipenko
Quoted 5 lines
To make it more clear.. there was no problem with "suppressing a write", the problem was that regmap reads are done from the cache, and if cache isn't per-initialized, then you read zeros from the cache until write is made.
With
update_bits()that can result in an attempt to set a bit to zero not actually taking effect if it believes that the bit was already set to zero and there has been no change in the value of the register.
Alright, I tested this patch and see that there is no need to pre-init the RBTREE cache.
I'm a bit doubtful whether this change will save us much memory since there are 64 regs total. Nevertheless, the driver will continue to work as before, so I'm now okay with the change.
Quoted 8 lines · Dmitry Osipenko
23.02.2022 00:43, Mark Brown пишет:
drivers/power/supply/smb347-charger.c static const struct regmap_config smb347_regmap = { · line 1491
Quoted 2 lines
drivers/power/supply/smb347-charger.c +1
-1- .num_reg_defaults_raw = SMB347_MAX_REGISTER, + .cache_type = REGCACHE_RBTREE,Why you removed the
num_reg_defaults_raw? It was needed in order to populate the default values on the regcache init, is it somehow different for theREGCACHE_RBTREE? Otherwise this patch is incorrect.
rbtree caches will dynamically grow at runtime to accomodate any new registers they find, it's not like flat caches where they are intended for use in atomic context.
Quoted 21 lines · Mark Brown
The smb347 has a very sparse register map (the maximum register is
0x3fbut less than 10% of the possible registers appear to be defined) and doesn't have any hardware defaults specified so the sparser data structure of an rbtree is a better fit for it's needs than a flat cache. Since it uses I2C for the control interface there is no performance concern with the slightly more involved code so let's convert it.This will mean we avoid any issues created by assuming that any previously unaccessed registers hold a value that doesn't match what's in the hardware (eg, an
_update_bits()suppressing a write).Signed-off-by Mark Brown <[email protected]>--- drivers/power/supply/smb347-charger.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)drivers/power/supply/smb347-charger.c +0
-0
drivers/power/supply/smb347-charger.c static const struct regmap_config smb347_regmap = { · line 1494
Quoted 9 lines
drivers/power/supply/smb347-charger.c +1
-2@@ -1488,8 +1488,7 @@ static const struct regmap_config smb347_regmap = { .max_register = SMB347_MAX_REGISTER, .volatile_reg = smb347_volatile_reg, .readable_reg = smb347_readable_reg, - .cache_type = REGCACHE_FLAT, - .num_reg_defaults_raw = SMB347_MAX_REGISTER, + .cache_type = REGCACHE_RBTREE, };static const struct regulator_ops smb347_usb_vbus_regulator_ops = {