| Message ID | 1328789531-10067-1-git-send-email-ldewangan@nvidia.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Thu, Feb 09, 2012 at 05:42:11PM +0530, Laxman Dewangan wrote: > + if (vol || map->cache_type == REGCACHE_NONE) { > + ret = _regmap_raw_write(map, reg, val, val_bytes * val_count); You still need to do the byte swap here. > + } else { > + for (i = 0; i < val_count; i++) { > + memcpy(map->work_buf, val + (i * val_bytes), val_bytes); > + ival = map->format.parse_val(map->work_buf); They're currently symmetric but really this should use format_val().
On Thursday 09 February 2012 05:47 PM, Mark Brown wrote: > * PGP Signed by an unknown key > > On Thu, Feb 09, 2012 at 05:42:11PM +0530, Laxman Dewangan wrote: > >> + if (vol || map->cache_type == REGCACHE_NONE) { >> + ret = _regmap_raw_write(map, reg, val, val_bytes * val_count); > You still need to do the byte swap here. I saw the regmap_raw_write() and it is using the same way without byte swapping. Want to use the same function as it is.. I am not sure why do we require byte-swapping in this case. Required things will be done by _regmap_raw_write only. This api just break the transfer in register-wise if any of the register is cached.. >> + } else { >> + for (i = 0; i< val_count; i++) { >> + memcpy(map->work_buf, val + (i * val_bytes), val_bytes); >> + ival = map->format.parse_val(map->work_buf); > They're currently symmetric but really this should use format_val(). > The format_val is require integer argument and issue is that I dont have this otherwise I need not to parse, can use directly. Am I missing something here? > * Unknown Key > * 0x6E30FDDD -- 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 Thu, Feb 09, 2012 at 06:15:18PM +0530, Laxman Dewangan wrote: > On Thursday 09 February 2012 05:47 PM, Mark Brown wrote: > >On Thu, Feb 09, 2012 at 05:42:11PM +0530, Laxman Dewangan wrote: > >>+ if (vol || map->cache_type == REGCACHE_NONE) { > >>+ ret = _regmap_raw_write(map, reg, val, val_bytes * val_count); > >You still need to do the byte swap here. > I saw the regmap_raw_write() and it is using the same way without > byte swapping. That's the whole point with the difference between the bulk and raw APIs though, the raw API skips the byte swapping and the bulk does it. > Want to use the same function as it is.. I am not sure why do we > require byte-swapping in this case. Required things will be done by > _regmap_raw_write only. The byte swapping is important for any device which has more than 8 bit register values, it's only not an issue for you because you have 8 bit registers. > This api just break the transfer in register-wise if any of the > register is cached.. Well, there's no fundamental reason why we can't support cache on raw operations too. It's not implemented because there's no need for it with any current users rather than because it's impossible. > > >>+ } else { > >>+ for (i = 0; i< val_count; i++) { > >>+ memcpy(map->work_buf, val + (i * val_bytes), val_bytes); > >>+ ival = map->format.parse_val(map->work_buf); > >They're currently symmetric but really this should use format_val(). > > > The format_val is require integer argument and issue is that I dont > have this otherwise I need not to parse, can use directly. > > Am I missing something here? > > > >* Unknown Key > >* 0x6E30FDDD >
On Thursday 09 February 2012 06:25 PM, Mark Brown wrote: > * PGP Signed by an unknown key > > On Thu, Feb 09, 2012 at 06:15:18PM +0530, Laxman Dewangan wrote: >> On Thursday 09 February 2012 05:47 PM, Mark Brown wrote: >>> On Thu, Feb 09, 2012 at 05:42:11PM +0530, Laxman Dewangan wrote: >>>> + if (vol || map->cache_type == REGCACHE_NONE) { >>>> + ret = _regmap_raw_write(map, reg, val, val_bytes * val_count); >>> You still need to do the byte swap here. >> I saw the regmap_raw_write() and it is using the same way without >> byte swapping. > That's the whole point with the difference between the bulk and raw APIs > though, the raw API skips the byte swapping and the bulk does it. > Ok, re-going through the apis again about the byte swapping, what I understand is that raw_read or raw_write always happen in bing-endian regardless of cpu. And bulk read/write supported the data which is in the cpu-endianess. So we need to convert the data from cpu_to_bexx if we want to call raw_write from bulk_write. Similarly we need to convert the data from bexx_to_cpu when we read from raw-read which is called from bulk_read(). If this is case then if we want the data in integer type from bulk_write data pointer then just memcpy will be fine like unsigned int ival; memcpy(&ival, bulk_val_ptr, val_bytes); and for calling raw_write, it need to call format_val() so this will do byte swapping. This require to duplicate the data pointer to new pointer and then do manipulation. Once we do this then we will be able to call raw_write() with the new duplicated pointer. if the register writes are broken in multiple transaction then only get the integer_val using memcpy and call regmap_write(), need not to format it. This may be require mem alloc/free on every call. It can be avoided by allocating memory for size (val_bytes + max_register) in advance during init.. Is it correct? >> Want to use the same function as it is.. I am not sure why do we >> require byte-swapping in this case. Required things will be done by >> _regmap_raw_write only. > The byte swapping is important for any device which has more than 8 bit > register values, it's only not an issue for you because you have 8 bit > registers. > >> This api just break the transfer in register-wise if any of the >> register is cached.. > Well, there's no fundamental reason why we can't support cache on raw > operations too. It's not implemented because there's no need for it > with any current users rather than because it's impossible. > Now if we want to support the caching from raw-write then we need to either do caching first or device write first. I am seeing one issue with this approach: Whichever is first, if we do caching (which is in loop) and if it fails in between inside loop then we may not able to revert it or it will be complicate to implement the reversal of old values. Also if it is stored in cache first and later if write fails then also it will be difficult to revert it. If above error is not the issue then possibly following is the solution: - remove the warnings from raw-write... - Let allow the reg_write as what it is already there. - Then parse input val pointer and put in cache register by register 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 != 0) dev_warn("Unable to cache register %d\n", reg +i); } It seems modifying raw_write() looks simple if we are able to ignore the caching error.. Let me know your opinion? Thanks, Laxman >>>> + } else { >>>> + for (i = 0; i< val_count; i++) { >>>> + memcpy(map->work_buf, val + (i * val_bytes), val_bytes); >>>> + ival = map->format.parse_val(map->work_buf); >>> They're currently symmetric but really this should use format_val(). >>> >> The format_val is require integer argument and issue is that I dont >> have this otherwise I need not to parse, can use directly. >> >> Am I missing something here? >> >> >>> * Unknown Key >>> * 0x6E30FDDD > * Unknown Key > * 0x6E30FDDD -- 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 Thu, Feb 09, 2012 at 10:44:15PM +0530, Laxman Dewangan wrote: > On Thursday 09 February 2012 06:25 PM, Mark Brown wrote: > >That's the whole point with the difference between the bulk and raw APIs > >though, the raw API skips the byte swapping and the bulk does it. > Ok, re-going through the apis again about the byte swapping, what I > understand is that raw_read or raw_write always happen in > bing-endian regardless of cpu. And bulk read/write supported the > data which is in the cpu-endianess. Well, technically raw operations always work with device native data - it's just that nobody actually makes devices that are little endian so we've no software support for that. But pretty much, yes. > So we need to convert the data from cpu_to_bexx if we want to call > raw_write from bulk_write. Similarly we need to convert the data > from bexx_to_cpu when we read from raw-read which is called from > bulk_read(). Yes. > If this is case then if we want the data in integer type from > bulk_write data pointer then just memcpy will be fine like > unsigned int ival; > memcpy(&ival, bulk_val_ptr, val_bytes); > and for calling raw_write, it need to call format_val() so this will > do byte swapping. This require to duplicate the data pointer to new > pointer and then do manipulation. Once we do this then we will be > able to call raw_write() with the new duplicated pointer. Indeed, taking a copy of the data and modifying it will do the trick. > This may be require mem alloc/free on every call. It can be avoided > by allocating memory for size (val_bytes + max_register) in advance > during init.. > Is it correct? val_bytes * max_register, and obviously the worst case on that is rather large. > >Well, there's no fundamental reason why we can't support cache on raw > >operations too. It's not implemented because there's no need for it > >with any current users rather than because it's impossible. > Now if we want to support the caching from raw-write then we need to > either do caching first or device write first. Yes. > I am seeing one issue with this approach: > Whichever is first, if we do caching (which is in loop) and if it > fails in between inside loop then we may not able to revert it > or it will be complicate to implement the reversal of old values. > Also if it is stored in cache first and later if write fails then > also it will be difficult to revert it. I'm not overly worried about failed writes, they should essentially never happen and if they do happen we can always resync with the device by either reading the registers or discarding the cache (assuming we didn't completely loose track of the device). Doing something really expensive isn't too bad for rare events, and practically speaking if we fail to write once we'll never succeed. Besides, when we do get an error we have no way of telling what exactly the hardware did - even if we see that it got an error on the nth byte we don't know if it might've done something with that before it complained or if there was damage to some of the earlier data too. Upper layers are going to have to implement recovery mechanisms if they want them. > - remove the warnings from raw-write... > - Let allow the reg_write as what it is already there. > - Then parse input val pointer and put in cache register by register > 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 != 0) > dev_warn("Unable to cache register %d\n", reg +i); > } Hrm, we also need to handle cache bypass and cache only in here - and for consistency with vanilla write we need to cache before write. Indeed, we'll need to push all the cache handling down into _regmap_raw_write() from regmap_reg_write() as that's where writes from regmap_reg_write() end up. > It seems modifying raw_write() looks simple if we are able to ignore > the caching error.. > Let me know your opinion? We can't ignore errors completely but we don't need to go to great lengths to handle it due to the issues discussed above.
On Thursday 09 February 2012 11:42 PM, Mark Brown wrote: > * PGP Signed by an unknown key > > On Thu, Feb 09, 2012 at 10:44:15PM +0530, Laxman Dewangan wrote: > >> If this is case then if we want the data in integer type from >> bulk_write data pointer then just memcpy will be fine like >> unsigned int ival; >> memcpy(&ival, bulk_val_ptr, val_bytes); >> and for calling raw_write, it need to call format_val() so this will >> do byte swapping. This require to duplicate the data pointer to new >> pointer and then do manipulation. Once we do this then we will be >> able to call raw_write() with the new duplicated pointer. > Indeed, taking a copy of the data and modifying it will do the trick. > So I am going to allocate buffer for some size, initially min(val_bytes * max_register, 128) bytes, and in bulk_write(), if require buffer is more than 128 then re-alloc buffer which is now (req_size + 128). And then copy the data into this buffer, modify it and send to device. >> This may be require mem alloc/free on every call. It can be avoided >> by allocating memory for size (val_bytes + max_register) in advance >> during init.. >> Is it correct? > val_bytes * max_register, and obviously the worst case on that is rather > large. > We can allocate dynamically now based on requirements, start with min(val_bytes * max_register, 128).. I still think bulk_write will be helpful as it deals with cpu-endianness and it avoid reformatting of data. Case if register is 16 bit wide then u16 regvals[10]; setting regvals with the desired one is easy as regvals[0] = xxxx regvals[1] = yyyy and then just call bulk_write(,,regvals,..) The data will be stored in cpu endiness and will go to device in big endiness. This will also make the sync with bulk_read. But this should be in different patch. >>> Well, there's no fundamental reason why we can't support cache on raw >>> operations too. It's not implemented because there's no need for it >>> with any current users rather than because it's impossible. >> Now if we want to support the caching from raw-write then we need to >> either do caching first or device write first. > Yes. > >> I am seeing one issue with this approach: >> Whichever is first, if we do caching (which is in loop) and if it >> fails in between inside loop then we may not able to revert it >> or it will be complicate to implement the reversal of old values. >> Also if it is stored in cache first and later if write fails then >> also it will be difficult to revert it. > I'm not overly worried about failed writes, they should essentially > never happen and if they do happen we can always resync with the device > by either reading the registers or discarding the cache (assuming we > didn't completely loose track of the device). Doing something really > expensive isn't too bad for rare events, and practically speaking if we > fail to write once we'll never succeed. > > Besides, when we do get an error we have no way of telling what exactly > the hardware did - even if we see that it got an error on the nth byte > we don't know if it might've done something with that before it > complained or if there was damage to some of the earlier data too. > Upper layers are going to have to implement recovery mechanisms if they > want them. > Just for now, lets return error in this case so that client will take care of this. >> - remove the warnings from raw-write... >> - Let allow the reg_write as what it is already there. >> - Then parse input val pointer and put in cache register by register >> 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 != 0) >> dev_warn("Unable to cache register %d\n", reg +i); >> } > Hrm, we also need to handle cache bypass and cache only in here - and > for consistency with vanilla write we need to cache before write. > Indeed, we'll need to push all the cache handling down into > _regmap_raw_write() from regmap_reg_write() as that's where writes from > regmap_reg_write() end up. > regmap_reg_write() supports format_write() case which does not ends with the _regmap_raw_write() and so need to keep caching here without too much changes. However the caching on this function will be done only if there is format_write() otherwise not and hence it will be done in _regmap_raw_write(). I will send the patch for this. -- 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 02:43:43PM +0530, Laxman Dewangan wrote: Please remember to delete unnneded context from your mails, it makes it much easier to find what you're saying. > So I am going to allocate buffer for some size, initially > min(val_bytes * max_register, 128) bytes, and in bulk_write(), if > require buffer is more than 128 then re-alloc buffer which is now > (req_size + 128). > And then copy the data into this buffer, modify it and send to device. What I'd suggest doing as a first pass is just allocating the buffer each time, anything else is a performance optimisation we can worry about incrementally. Remember, this only has to be faster than I2C most of the time and I2C is really slow. For your case with 8 byte values there's an even better optimisation which is to notice that we don't need to swap at all and just use the original buffer directly.
diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c index 80129c0..28dbe96 100644 --- a/drivers/base/regmap/regmap.c +++ b/drivers/base/regmap/regmap.c @@ -466,6 +466,59 @@ int regmap_raw_write(struct regmap *map, unsigned int reg, } EXPORT_SYMBOL_GPL(regmap_raw_write); +/* + * regmap_bulk_write(): Write multiple registers from the device + * + * @map: Register map to write to + * @reg: First register to be write from + * @val: Block of data to be written, laid out for direct transmission to + * the device + * @val_count: Number of registers to write + * + * This function is intended to be used for writing a large block of + * data to be device either in single transfer or multiple transfer. + * If all registers which need to be written are volatile then all + * data will be send in single transfer. No data formatting is done in + * this case. + * If any of the register is non-volatile and caching is enabled then + * data will be written to device in single register wise and hence + * complete transfer is done in multiple small transfer. In this case, + * the register data will be formatted to device register value format. + * + * A value of zero will be returned on success, a negative errno will + * be returned in error cases. + */ +int regmap_bulk_write(struct regmap *map, unsigned int reg, const void *val, + size_t val_count) +{ + int ret = 0, i; + unsigned int ival; + size_t val_bytes = map->format.val_bytes; + size_t reg_bytes = map->format.reg_bytes; + bool vol = regmap_volatile_range(map, reg, val_count); + + if (!map->format.parse_val) + return -EINVAL; + + mutex_lock(&map->lock); + + if (vol || map->cache_type == REGCACHE_NONE) { + ret = _regmap_raw_write(map, reg, val, val_bytes * val_count); + } else { + for (i = 0; i < val_count; i++) { + memcpy(map->work_buf, val + (i * val_bytes), val_bytes); + ival = map->format.parse_val(map->work_buf); + ret = _regmap_write(map, reg + (i * reg_bytes), ival); + if (ret) + break; + } + } + + mutex_unlock(&map->lock); + return ret; +} +EXPORT_SYMBOL_GPL(regmap_bulk_write); + static int _regmap_raw_read(struct regmap *map, unsigned int reg, void *val, unsigned int val_len) { diff --git a/include/linux/regmap.h b/include/linux/regmap.h index a6ed6e6..0925e24 100644 --- a/include/linux/regmap.h +++ b/include/linux/regmap.h @@ -135,6 +135,8 @@ int regmap_reinit_cache(struct regmap *map, int regmap_write(struct regmap *map, unsigned int reg, unsigned int val); int regmap_raw_write(struct regmap *map, unsigned int reg, const void *val, size_t val_len); +int regmap_bulk_write(struct regmap *map, unsigned int reg, const void *val, + size_t val_count); int regmap_read(struct regmap *map, unsigned int reg, unsigned int *val); int regmap_raw_read(struct regmap *map, unsigned int reg, void *val, size_t val_len);
Adding bulk write which is used for writing a large block of data to the device. If all registers which need to be written are volatile then all data will be send in single transfer. If any of the register is non-volatile and caching is enabled then data will be written to device in single register wise and hence complete transfer is done in multiple small transfer. Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com> --- drivers/base/regmap/regmap.c | 53 ++++++++++++++++++++++++++++++++++++++++++ include/linux/regmap.h | 2 + 2 files changed, 55 insertions(+), 0 deletions(-)