[PATCH V1] regmap: Support for caching in reg_raw_write()

[PATCH V1] regmap: Support for caching in reg_raw_write()

[Laxman Dewangan]

Adding support for caching of data into the
non-volatile register from the call of reg_raw_write().

This will allow the larger block of data write into multiple
register without worrying whether register is cached or not
through reg_raw_write().

Signed-off-by: Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/base/regmap/regmap.c |   25 ++++++++++++++++++++-----
 1 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 80129c0..e5986ab 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -323,6 +323,25 @@ static int _regmap_raw_write(struct regmap *map, unsigned int reg,
 			if (!map->writeable_reg(map->dev, reg + i))
 				return -EINVAL;
 
+	if (!map->cache_bypass && map->format.parse_val) {
+		int val_bytes = map->format.val_bytes;
+		unsigned int ival;
+		for (i = 0; i < val_len / map->format.val_bytes; i++) {
+			memcpy(map->work_buf, val + (i * val_bytes), val_bytes);
+			ival = map->format.parse_val(map->work_buf);
+			ret = regcache_write(map, reg + i, ival);
+			if (ret) {
+				dev_warn(map->dev,
+					"Error in caching of register\n");
+				return ret;
+			}
+		}
+		if (map->cache_only) {
+			map->cache_dirty = true;
+			return 0;
+		}
+	}
+
 	map->format.format_reg(map->work_buf, reg);
 
 	u8[0] |= map->write_flag_mask;
@@ -373,7 +392,7 @@ int _regmap_write(struct regmap *map, unsigned int reg,
 	int ret;
 	BUG_ON(!map->format.format_write && !map->format.format_val);
 
-	if (!map->cache_bypass) {
+	if (!map->cache_bypass && map->format.format_write) {
 		ret = regcache_write(map, reg, val);
 		if (ret != 0)
 			return ret;
@@ -450,12 +469,8 @@ EXPORT_SYMBOL_GPL(regmap_write);
 int regmap_raw_write(struct regmap *map, unsigned int reg,
 		     const void *val, size_t val_len)
 {
-	size_t val_count = val_len / map->format.val_bytes;
 	int ret;
 
-	WARN_ON(!regmap_volatile_range(map, reg, val_count) &&
-		map->cache_type != REGCACHE_NONE);
-
 	mutex_lock(&map->lock);
 
 	ret = _regmap_raw_write(map, reg, val, val_len);
-- 
1.7.1.1

Re: [PATCH V1] regmap: Support for caching in reg_raw_write()

[Mark Brown]

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

On Fri, Feb 10, 2012 at 05:02:52PM +0530, Laxman Dewangan wrote:

This looks good, but could you make the few small improvements below
please?

> +	if (!map->cache_bypass && map->format.parse_val) {
> +		int val_bytes = map->format.val_bytes;
> +		unsigned int ival;
> +		for (i = 0; i < val_len / map->format.val_bytes; i++) {

A bit nit picky but since you've got a local val_bytes you may as well
use it.

> +			memcpy(map->work_buf, val + (i * val_bytes), val_bytes);
> +			ival = map->format.parse_val(map->work_buf);
> +			ret = regcache_write(map, reg + i, ival);
> +			if (ret) {
> +				dev_warn(map->dev,
> +					"Error in caching of register\n");

Say which register and the return value here - knowing the register
might be very helpful for debug.  This should probably also be at least
a dev_err().

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH V1] regmap: Support for caching in reg_raw_write()

[Lars-Peter Clausen]

On 02/10/2012 12:32 PM, Laxman Dewangan wrote:
> Adding support for caching of data into the
> non-volatile register from the call of reg_raw_write().
> 
> This will allow the larger block of data write into multiple
> register without worrying whether register is cached or not
> through reg_raw_write().
> 

In my opinion it makes more sense to introduce a regmap_bulk_write function
for this. regmap_raw_write was not really meant to write registers but
rather binary blobs, like firmware. Also this keeps things consistent with
the read part of the regmap API.

> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> ---
>  drivers/base/regmap/regmap.c |   25 ++++++++++++++++++++-----
>  1 files changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
> index 80129c0..e5986ab 100644
> --- a/drivers/base/regmap/regmap.c
> +++ b/drivers/base/regmap/regmap.c
> @@ -323,6 +323,25 @@ static int _regmap_raw_write(struct regmap *map, unsigned int reg,
>  			if (!map->writeable_reg(map->dev, reg + i))
>  				return -EINVAL;
>  
> +	if (!map->cache_bypass && map->format.parse_val) {
> +		int val_bytes = map->format.val_bytes;
> +		unsigned int ival;
> +		for (i = 0; i < val_len / map->format.val_bytes; i++) {
> +			memcpy(map->work_buf, val + (i * val_bytes), val_bytes);
> +			ival = map->format.parse_val(map->work_buf);
> +			ret = regcache_write(map, reg + i, ival);
> +			if (ret) {
> +				dev_warn(map->dev,
> +					"Error in caching of register\n");
> +				return ret;
> +			}
> +		}
> +		if (map->cache_only) {
> +			map->cache_dirty = true;
> +			return 0;
> +		}
> +	}
> +
>  	map->format.format_reg(map->work_buf, reg);
>  
>  	u8[0] |= map->write_flag_mask;
> @@ -373,7 +392,7 @@ int _regmap_write(struct regmap *map, unsigned int reg,
>  	int ret;
>  	BUG_ON(!map->format.format_write && !map->format.format_val);
>  
> -	if (!map->cache_bypass) {
> +	if (!map->cache_bypass && map->format.format_write) {
>  		ret = regcache_write(map, reg, val);
>  		if (ret != 0)
>  			return ret;
> @@ -450,12 +469,8 @@ EXPORT_SYMBOL_GPL(regmap_write);
>  int regmap_raw_write(struct regmap *map, unsigned int reg,
>  		     const void *val, size_t val_len)
>  {
> -	size_t val_count = val_len / map->format.val_bytes;
>  	int ret;
>  
> -	WARN_ON(!regmap_volatile_range(map, reg, val_count) &&
> -		map->cache_type != REGCACHE_NONE);
> -
>  	mutex_lock(&map->lock);
>  
>  	ret = _regmap_raw_write(map, reg, val, val_len);

Re: [PATCH V1] regmap: Support for caching in reg_raw_write()

[Laxman Dewangan]

On Friday 10 February 2012 05:31 PM, Lars-Peter Clausen wrote:
> On 02/10/2012 12:32 PM, Laxman Dewangan wrote:
>> Adding support for caching of data into the
>> non-volatile register from the call of reg_raw_write().
>>
>> This will allow the larger block of data write into multiple
>> register without worrying whether register is cached or not
>> through reg_raw_write().
>>
> In my opinion it makes more sense to introduce a regmap_bulk_write function
> for this.
Yes, I am going to push another change which will have the 
regmap_bulk_write() which will be in sync with read part.

Re: [PATCH V1] regmap: Support for caching in reg_raw_write()

[Mark Brown]

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

On Fri, Feb 10, 2012 at 01:01:26PM +0100, Lars-Peter Clausen wrote:

> In my opinion it makes more sense to introduce a regmap_bulk_write function
> for this. regmap_raw_write was not really meant to write registers but

This isn't exclusive to that, I think Laxman planned to send a patch
doing that on top of this one which would be what most users would end
up using.

> rather binary blobs, like firmware. Also this keeps things consistent with
> the read part of the regmap API.

See the previous discussion on this in the past day or so - bulk_write()
is more complicated to implement by itself since it's going to end up
boling down to a raw_write() internally anyway (as does reg_write()) and
it seems nicer to just do the right thing if people ask for it.  

We can always refactor later to improve performance if needed but this
is a simple starting point.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH V1] regmap: Support for caching in reg_raw_write()

[Lars-Peter Clausen]

On 02/10/2012 01:14 PM, Mark Brown wrote:
> On Fri, Feb 10, 2012 at 01:01:26PM +0100, Lars-Peter Clausen wrote:
> 
>> In my opinion it makes more sense to introduce a regmap_bulk_write function
>> for this. regmap_raw_write was not really meant to write registers but
> 
> This isn't exclusive to that, I think Laxman planned to send a patch
> doing that on top of this one which would be what most users would end
> up using.
> 
>> rather binary blobs, like firmware. Also this keeps things consistent with
>> the read part of the regmap API.
> 
> See the previous discussion on this in the past day or so - bulk_write()
> is more complicated to implement by itself since it's going to end up
> boling down to a raw_write() internally anyway (as does reg_write()) and
> it seems nicer to just do the right thing if people ask for it.  
>

regmap_write doesn't always go the raw_write path. Also with this approach
we end up formatting the value into the raw format, only to parse it again
in the next step.

Furthermore I don't think it makes sense to cache raw values as the cache
operates on a register level, not on a byte level.

Re: [PATCH V1] regmap: Support for caching in reg_raw_write()

[Mark Brown]

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

On Fri, Feb 10, 2012 at 01:33:16PM +0100, Lars-Peter Clausen wrote:

> regmap_write doesn't always go the raw_write path. Also with this approach

We can ignore the non-byte formats as a wart for this; they're cut out
of this area of the subsystem functionality and just get single register
write operatons with single register cache access.

> we end up formatting the value into the raw format, only to parse it again
> in the next step.

Yes, like I say there's room for optimisation.  For 8 bit values there
would be win from working out that we don't need to do anything at all, 
though I doubt it's ever going to be performance critical.

> Furthermore I don't think it makes sense to cache raw values as the cache
> operates on a register level, not on a byte level.

The entire API operates on registers really.  The only reason
raw_write() takes a length is because it's what most of the users are
actually likely to have to hand, trying to write to less than a register
isn't particularly sensible.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]