[PATCH] iio: chemical: atlas-ph-sensor: switch regmap cache

[PATCH] iio: chemical: atlas-ph-sensor: switch regmap cache

[Matt Ranostay]

switch from using REGCACHE_FLAT to REGCACHE_RBTREE so initial hw values
are read from device. This also allows some volatile ranges to be
dropped.

Signed-off-by: Matt Ranostay <mranostay@gmail.com>
---
 drivers/iio/chemical/atlas-ph-sensor.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/iio/chemical/atlas-ph-sensor.c b/drivers/iio/chemical/atlas-ph-sensor.c
index 06cd49c..71c8e02 100644
--- a/drivers/iio/chemical/atlas-ph-sensor.c
+++ b/drivers/iio/chemical/atlas-ph-sensor.c
@@ -65,8 +65,6 @@ struct atlas_data {
 
 static const struct regmap_range atlas_volatile_ranges[] = {
 	regmap_reg_range(ATLAS_REG_INT_CONTROL, ATLAS_REG_INT_CONTROL),
-	regmap_reg_range(ATLAS_REG_CALIB_STATUS, ATLAS_REG_CALIB_STATUS),
-	regmap_reg_range(ATLAS_REG_TEMP_DATA, ATLAS_REG_TEMP_DATA + 4),
 	regmap_reg_range(ATLAS_REG_PH_DATA, ATLAS_REG_PH_DATA + 4),
 };
 
@@ -83,7 +81,7 @@ static const struct regmap_config atlas_regmap_config = {
 
 	.volatile_table = &atlas_volatile_table,
 	.max_register = ATLAS_REG_PH_DATA + 4,
-	.cache_type = REGCACHE_FLAT,
+	.cache_type = REGCACHE_RBTREE,
 };
 
 static const struct iio_chan_spec atlas_channels[] = {
-- 
2.7.0

Re: [PATCH] iio: chemical: atlas-ph-sensor: switch regmap cache

[Jonathan Cameron]

On 14/02/16 01:20, Matt Ranostay wrote:
> switch from using REGCACHE_FLAT to REGCACHE_RBTREE so initial hw values
> are read from device. This also allows some volatile ranges to be
> dropped.
> 
> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
I'm lost here. Why should changing the storage of the register cache result
in different behaviour other than in efficiency of access to cached values?

Jonathan
> ---
>  drivers/iio/chemical/atlas-ph-sensor.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/chemical/atlas-ph-sensor.c b/drivers/iio/chemical/atlas-ph-sensor.c
> index 06cd49c..71c8e02 100644
> --- a/drivers/iio/chemical/atlas-ph-sensor.c
> +++ b/drivers/iio/chemical/atlas-ph-sensor.c
> @@ -65,8 +65,6 @@ struct atlas_data {
>  
>  static const struct regmap_range atlas_volatile_ranges[] = {
>  	regmap_reg_range(ATLAS_REG_INT_CONTROL, ATLAS_REG_INT_CONTROL),
> -	regmap_reg_range(ATLAS_REG_CALIB_STATUS, ATLAS_REG_CALIB_STATUS),
> -	regmap_reg_range(ATLAS_REG_TEMP_DATA, ATLAS_REG_TEMP_DATA + 4),
>  	regmap_reg_range(ATLAS_REG_PH_DATA, ATLAS_REG_PH_DATA + 4),
>  };
>  
> @@ -83,7 +81,7 @@ static const struct regmap_config atlas_regmap_config = {
>  
>  	.volatile_table = &atlas_volatile_table,
>  	.max_register = ATLAS_REG_PH_DATA + 4,
> -	.cache_type = REGCACHE_FLAT,
> +	.cache_type = REGCACHE_RBTREE,
>  };
>  
>  static const struct iio_chan_spec atlas_channels[] = {
> 

Re: [PATCH] iio: chemical: atlas-ph-sensor: switch regmap cache

[Lars-Peter Clausen]

On 02/14/2016 04:02 PM, Jonathan Cameron wrote:
> On 14/02/16 01:20, Matt Ranostay wrote:
>> switch from using REGCACHE_FLAT to REGCACHE_RBTREE so initial hw values
>> are read from device. This also allows some volatile ranges to be
>> dropped.
>>
>> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
> I'm lost here. Why should changing the storage of the register cache result
> in different behaviour other than in efficiency of access to cached values?

And if there is really a difference this should probably be addressed at the
regmap level.

- Lars

> 
> Jonathan
>> ---
>>  drivers/iio/chemical/atlas-ph-sensor.c | 4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/iio/chemical/atlas-ph-sensor.c b/drivers/iio/chemical/atlas-ph-sensor.c
>> index 06cd49c..71c8e02 100644
>> --- a/drivers/iio/chemical/atlas-ph-sensor.c
>> +++ b/drivers/iio/chemical/atlas-ph-sensor.c
>> @@ -65,8 +65,6 @@ struct atlas_data {
>>  
>>  static const struct regmap_range atlas_volatile_ranges[] = {
>>  	regmap_reg_range(ATLAS_REG_INT_CONTROL, ATLAS_REG_INT_CONTROL),
>> -	regmap_reg_range(ATLAS_REG_CALIB_STATUS, ATLAS_REG_CALIB_STATUS),
>> -	regmap_reg_range(ATLAS_REG_TEMP_DATA, ATLAS_REG_TEMP_DATA + 4),
>>  	regmap_reg_range(ATLAS_REG_PH_DATA, ATLAS_REG_PH_DATA + 4),
>>  };
>>  
>> @@ -83,7 +81,7 @@ static const struct regmap_config atlas_regmap_config = {
>>  
>>  	.volatile_table = &atlas_volatile_table,
>>  	.max_register = ATLAS_REG_PH_DATA + 4,
>> -	.cache_type = REGCACHE_FLAT,
>> +	.cache_type = REGCACHE_RBTREE,
>>  };
>>  
>>  static const struct iio_chan_spec atlas_channels[] = {
>>

Re: [PATCH] iio: chemical: atlas-ph-sensor: switch regmap cache

[Matt Ranostay]

Should have CC'ed Mark Brown on the original post.
But anyway after talking to him it seems that REGCACHE_FLAT is
"special" since it a flat tree it won't read it and cache the initial
value, so it then defaults to zero.

Mark, thoughts?

Thanks,

Matt

On Sun, Feb 14, 2016 at 9:40 AM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> On 02/14/2016 04:02 PM, Jonathan Cameron wrote:
>> On 14/02/16 01:20, Matt Ranostay wrote:
>>> switch from using REGCACHE_FLAT to REGCACHE_RBTREE so initial hw values
>>> are read from device. This also allows some volatile ranges to be
>>> dropped.
>>>
>>> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
>> I'm lost here. Why should changing the storage of the register cache result
>> in different behaviour other than in efficiency of access to cached values?
>
> And if there is really a difference this should probably be addressed at the
> regmap level.
>
> - Lars
>
>>
>> Jonathan
>>> ---
>>>  drivers/iio/chemical/atlas-ph-sensor.c | 4 +---
>>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/iio/chemical/atlas-ph-sensor.c b/drivers/iio/chemical/atlas-ph-sensor.c
>>> index 06cd49c..71c8e02 100644
>>> --- a/drivers/iio/chemical/atlas-ph-sensor.c
>>> +++ b/drivers/iio/chemical/atlas-ph-sensor.c
>>> @@ -65,8 +65,6 @@ struct atlas_data {
>>>
>>>  static const struct regmap_range atlas_volatile_ranges[] = {
>>>      regmap_reg_range(ATLAS_REG_INT_CONTROL, ATLAS_REG_INT_CONTROL),
>>> -    regmap_reg_range(ATLAS_REG_CALIB_STATUS, ATLAS_REG_CALIB_STATUS),
>>> -    regmap_reg_range(ATLAS_REG_TEMP_DATA, ATLAS_REG_TEMP_DATA + 4),
>>>      regmap_reg_range(ATLAS_REG_PH_DATA, ATLAS_REG_PH_DATA + 4),
>>>  };
>>>
>>> @@ -83,7 +81,7 @@ static const struct regmap_config atlas_regmap_config = {
>>>
>>>      .volatile_table = &atlas_volatile_table,
>>>      .max_register = ATLAS_REG_PH_DATA + 4,
>>> -    .cache_type = REGCACHE_FLAT,
>>> +    .cache_type = REGCACHE_RBTREE,
>>>  };
>>>
>>>  static const struct iio_chan_spec atlas_channels[] = {
>>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] iio: chemical: atlas-ph-sensor: switch regmap cache

[Mark Brown]

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

On Sun, Feb 14, 2016 at 06:40:15PM +0100, Lars-Peter Clausen wrote:
> On 02/14/2016 04:02 PM, Jonathan Cameron wrote:

> > I'm lost here. Why should changing the storage of the register cache result
> > in different behaviour other than in efficiency of access to cached values?

> And if there is really a difference this should probably be addressed at the
> regmap level.

You'd need to make regcache-flat keep track of which cache values are
valid either during init (in which case it'd have to read every other
register at that time) or at runtime (which would mean extra operations
in a fast path).  Flat really is special here, there's no reason to use
it over a rbtree other than being very sensitive about performance.

Please also be aware that if you CC me on an iio patch series there's a
good chance I'll just delete it without reading it, for some reason I
keep on getting copied on lots of them which have no relevance to me
that I'm able to identify.

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

Re: [PATCH] iio: chemical: atlas-ph-sensor: switch regmap cache

[Jonathan Cameron]

On 15/02/16 15:02, Mark Brown wrote:
> On Sun, Feb 14, 2016 at 06:40:15PM +0100, Lars-Peter Clausen wrote:
>> On 02/14/2016 04:02 PM, Jonathan Cameron wrote:
> 
>>> I'm lost here. Why should changing the storage of the register cache result
>>> in different behaviour other than in efficiency of access to cached values?
> 
>> And if there is really a difference this should probably be addressed at the
>> regmap level.
> 
> You'd need to make regcache-flat keep track of which cache values are
> valid either during init (in which case it'd have to read every other
> register at that time) or at runtime (which would mean extra operations
> in a fast path).  Flat really is special here, there's no reason to use
> it over a rbtree other than being very sensitive about performance.
> 
> Please also be aware that if you CC me on an iio patch series there's a
> good chance I'll just delete it without reading it, for some reason I
> keep on getting copied on lots of them which have no relevance to me
> that I'm able to identify.
> 
Hmm.  Not the best documented 'non intuitive case' ever, but fair enough.
Glad you did pick this one up Mark!

So Matt, the reason those two elements are dropped from being volatile
is that they never were, but because we were using a flat cache it wasn't
getting the original values?

If so fair enough I suppose, but please confirm that I've understood this
correctly + I'll probably expand the patch description somewhat to make it
clearer why the original was wrong.

Jonathan

Re: [PATCH] iio: chemical: atlas-ph-sensor: switch regmap cache

[Matt Ranostay]

On Wed, Feb 17, 2016 at 1:19 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 15/02/16 15:02, Mark Brown wrote:
>> On Sun, Feb 14, 2016 at 06:40:15PM +0100, Lars-Peter Clausen wrote:
>>> On 02/14/2016 04:02 PM, Jonathan Cameron wrote:
>>
>>>> I'm lost here. Why should changing the storage of the register cache result
>>>> in different behaviour other than in efficiency of access to cached values?
>>
>>> And if there is really a difference this should probably be addressed at the
>>> regmap level.
>>
>> You'd need to make regcache-flat keep track of which cache values are
>> valid either during init (in which case it'd have to read every other
>> register at that time) or at runtime (which would mean extra operations
>> in a fast path).  Flat really is special here, there's no reason to use
>> it over a rbtree other than being very sensitive about performance.
>>
>> Please also be aware that if you CC me on an iio patch series there's a
>> good chance I'll just delete it without reading it, for some reason I
>> keep on getting copied on lots of them which have no relevance to me
>> that I'm able to identify.
>>
> Hmm.  Not the best documented 'non intuitive case' ever, but fair enough.
> Glad you did pick this one up Mark!
>
> So Matt, the reason those two elements are dropped from being volatile
> is that they never were, but because we were using a flat cache it wasn't
> getting the original values?
>
> If so fair enough I suppose, but please confirm that I've understood this
> correctly + I'll probably expand the patch description somewhat to make it
> clearer why the original was wrong.

That is correct. Flat cache wasn't getting the on device value without
it being set as volatile.


>
> Jonathan

Re: [PATCH] iio: chemical: atlas-ph-sensor: switch regmap cache

[Jonathan Cameron]

On 17/02/16 21:44, Matt Ranostay wrote:
> On Wed, Feb 17, 2016 at 1:19 PM, Jonathan Cameron <jic23@kernel.org> wrote:
>> On 15/02/16 15:02, Mark Brown wrote:
>>> On Sun, Feb 14, 2016 at 06:40:15PM +0100, Lars-Peter Clausen wrote:
>>>> On 02/14/2016 04:02 PM, Jonathan Cameron wrote:
>>>
>>>>> I'm lost here. Why should changing the storage of the register cache result
>>>>> in different behaviour other than in efficiency of access to cached values?
>>>
>>>> And if there is really a difference this should probably be addressed at the
>>>> regmap level.
>>>
>>> You'd need to make regcache-flat keep track of which cache values are
>>> valid either during init (in which case it'd have to read every other
>>> register at that time) or at runtime (which would mean extra operations
>>> in a fast path).  Flat really is special here, there's no reason to use
>>> it over a rbtree other than being very sensitive about performance.
>>>
>>> Please also be aware that if you CC me on an iio patch series there's a
>>> good chance I'll just delete it without reading it, for some reason I
>>> keep on getting copied on lots of them which have no relevance to me
>>> that I'm able to identify.
>>>
>> Hmm.  Not the best documented 'non intuitive case' ever, but fair enough.
>> Glad you did pick this one up Mark!
>>
>> So Matt, the reason those two elements are dropped from being volatile
>> is that they never were, but because we were using a flat cache it wasn't
>> getting the original values?
>>
>> If so fair enough I suppose, but please confirm that I've understood this
>> correctly + I'll probably expand the patch description somewhat to make it
>> clearer why the original was wrong.
> 
> That is correct. Flat cache wasn't getting the on device value without
> it being set as volatile.
Applied to the togreg branch iio.git - initially pushed out as testing
for the autobuilders to play with it.

Thanks,
Jonathan
> 
> 
>>
>> Jonathan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>