[PATCH] power: supply: Use an rbtree rather than flat register cache

[PATCH] power: supply: Use an rbtree rather than flat register cache

[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,
 };
 
 static const struct regulator_ops smb347_usb_vbus_regulator_ops = {
-- 
2.30.2

Re: [PATCH] power: supply: Use an rbtree rather than flat register cache

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1571 bytes --]

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
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] power: supply: Use an rbtree rather than flat register cache

[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.

Re: [PATCH] power: supply: Use an rbtree rather than flat register cache

[Dmitry Osipenko]

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.

Re: [PATCH] power: supply: Use an rbtree rather than flat register cache

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 587 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] power: supply: Use an rbtree rather than flat register cache

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 507 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] power: supply: Use an rbtree rather than flat register cache

[Dmitry Osipenko]

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.

Re: [PATCH] power: supply: Use an rbtree rather than flat register cache

[Dmitry Osipenko]

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>