| Message ID | 20220222214331.1557723-1-broonie@kernel.org |
|---|---|
| State | Accepted |
| Commit | 4c678b7a6442c1d2d137639b2b6730f0587a956d |
| Headers | show |
| Series | power: supply: Use an rbtree rather than flat register cache | expand |
Hi, On Tue, Feb 22, 2022 at 09:43:31PM +0000, Mark Brown wrote: > 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). > > Signed-off-by: Mark Brown <broonie@kernel.org> > --- Thanks, queued. -- Sebastian > drivers/power/supply/smb347-charger.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/power/supply/smb347-charger.c b/drivers/power/supply/smb347-charger.c > index d56e469043bb..1511f71f937c 100644 > --- a/drivers/power/supply/smb347-charger.c > +++ b/drivers/power/supply/smb347-charger.c > @@ -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 = { > -- > 2.30.2 >
23.02.2022 00:43, Mark Brown пишет: > 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). > > Signed-off-by: Mark Brown <broonie@kernel.org> > --- > drivers/power/supply/smb347-charger.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/power/supply/smb347-charger.c b/drivers/power/supply/smb347-charger.c > index d56e469043bb..1511f71f937c 100644 > --- a/drivers/power/supply/smb347-charger.c > +++ b/drivers/power/supply/smb347-charger.c > @@ -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 пишет: > 23.02.2022 00:43, Mark Brown пишет: >> 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). >> >> Signed-off-by: Mark Brown <broonie@kernel.org> >> --- >> drivers/power/supply/smb347-charger.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/drivers/power/supply/smb347-charger.c b/drivers/power/supply/smb347-charger.c >> index d56e469043bb..1511f71f937c 100644 >> --- a/drivers/power/supply/smb347-charger.c >> +++ b/drivers/power/supply/smb347-charger.c >> @@ -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. 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.
On Thu, Feb 24, 2022 at 02:07:58PM +0300, Dmitry Osipenko wrote: > 23.02.2022 00:43, Mark Brown пишет: > > - .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. 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.
On Thu, Feb 24, 2022 at 02:35:42PM +0300, Dmitry Osipenko wrote: > 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 пишет: > On Thu, Feb 24, 2022 at 02:35:42PM +0300, Dmitry Osipenko wrote: > >> 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.
On 2/23/22 00:43, Mark Brown wrote: > 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). > > Signed-off-by: Mark Brown <broonie@kernel.org> > --- > drivers/power/supply/smb347-charger.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/power/supply/smb347-charger.c b/drivers/power/supply/smb347-charger.c > index d56e469043bb..1511f71f937c 100644 > --- a/drivers/power/supply/smb347-charger.c > +++ b/drivers/power/supply/smb347-charger.c > @@ -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 = { Tested-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
diff --git a/drivers/power/supply/smb347-charger.c b/drivers/power/supply/smb347-charger.c index d56e469043bb..1511f71f937c 100644 --- a/drivers/power/supply/smb347-charger.c +++ b/drivers/power/supply/smb347-charger.c @@ -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 = {
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). Signed-off-by: Mark Brown <broonie@kernel.org> --- drivers/power/supply/smb347-charger.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)