Messages in this thread |  | | Date | Fri, 10 Feb 2012 14:43:43 +0530 | From | Laxman Dewangan <> | Subject | Re: [PATCH V1] regmap: add bulk_write() for non-volatile register set |
| |
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.
|  |