| Message ID | 1328873572-12603-1-git-send-email-ldewangan@nvidia.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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().
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); -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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);
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@nvidia.com> --- drivers/base/regmap/regmap.c | 25 ++++++++++++++++++++----- 1 files changed, 20 insertions(+), 5 deletions(-)