Messages in this thread | | | Date | Thu, 10 Oct 2013 13:20:48 +0100 | From | Mark Brown <> | Subject | Re: [PATCH V1] new API regmap_multi_write() |
| |
On Thu, Oct 10, 2013 at 11:50:23AM +0100, Anthony Olech wrote:
> +/* > + * regmap_multi_write(): Write multiple non sequential registers to the device > + * > + * @map: Register map to write to > + * @reg: Array of registers to be written, all on the same page
Why all on the same page? That doesn't seem helpful for things trying to build on top of this. We currently manage to split even block writes up over page boundaries.
> + * @val: Block of data to be written, in native register size for device > + * @reg_count: Number of registers to write
I'm not seeing anything here which says how the registers and values are related to each other. I assume that the idea is that the same number of registers are provided as values...
This really doesn't feel like an idiomatic abstraction - it's a bit cumbersome to have the pair of arrays and try to line them up especially in native register format, normally we do this with an array reg_default. This would also mean that generic code like patches and cache syncing could pick up on the same functionality.
> + /* > + * Some devices do not support multi write, for > + * them we have a series of single write operations. > + */ > + if (map->use_single_rw) {
single_rw is somewhat relevant but this needs a separate flag since...
> + } else { > + ret = _regmap_raw_multi_write(map, > + reg_count, > + reg, > + wval); > + }
...this will try to use the new functionality even if the device doesn't support it. Indeed what this looks like is support for devices that can only do single register writes but in the I2C case allow it to be done without releasing the bus (it looks a lot like someone optimised things to look like a bunch of SPI register writes, with SPI bouncing /CS is much quicker than starting a new I2C transfer is).
I can see it being nice to have something like this but it needs more thought on the API and implementation. I'd suggest splitting the API addition from the underlying implementation to make things easier to review (the API should work for any user even if it just ends up as a series of separate register writes). Something like DAPM in ASoC could use it for example, as well as the patch and cache sync code. [unhandled content-type:application/pgp-signature] | |