New API regmap_multi_write() to support a single I2C transfer consisting
of writes to multiple non-sequential registers for those I2C clients that
implement the alternative non-standard MULTIWRITE block write mode.
Signed-off-by: Anthony Olech <[email protected]>
Signed-off-by: David Dajun Chen <[email protected]>
---
This patch is relative to linux-mainline repository tag v3.12-rc4
The Dialog DA9052 family of multifunction power management devices implement
an alternative non-standard I2C MULTIWRITE block write mode that appears on
the I2C bus looking like a normal block write A1-D1-D2-D3-..-Dn, but in fact
the I2C client decodes the bytes as A1-D1-A2-D2-A3-D3-..-An-Dn, where both
the data and addresses are 8 bits wide.
The reason for this unusual mode is to ensure that the set of registers are
recieved atomically, and is crutial in a multi-I2C-master system where the
application processor (a modem chip for example) competes for the I2C bus.
This patch only adds functionality, it does not alter any existing regmap API.
The new API works in the degenerative case of a single register write as well.
drivers/base/regmap/regmap.c | 144 ++++++++++++++++++++++++++++++++++++++++++
include/linux/regmap.h | 2 +
2 files changed, 146 insertions(+)
diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 7d689a1..d47b00d 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -1439,6 +1439,150 @@ out:
}
EXPORT_SYMBOL_GPL(regmap_bulk_write);
+int _regmap_raw_multi_write(struct regmap *map, size_t reg_count,
+ unsigned int *reg, const void *val)
+{
+ u8 *u8;
+ void *buf;
+ int ret;
+ size_t len;
+ int i;
+ size_t unit_len;
+ int val_bytes = map->format.val_bytes;
+
+ WARN_ON(!map->bus);
+
+ /* Check for unwritable registers before we start */
+ if (map->writeable_reg)
+ for (i = 0; i < reg_count; i++)
+ if (!map->writeable_reg(map->dev,
+ reg[i]))
+ return -EINVAL;
+
+ if (!map->cache_bypass && map->format.parse_val) {
+ unsigned int ival;
+ for (i = 0; i < reg_count; i++) {
+ ival = map->format.parse_val(val + (i * val_bytes));
+ ret = regcache_write(map, reg[i], ival);
+ if (ret) {
+ dev_err(map->dev,
+ "Error in caching of register: %x ret: %d\n",
+ reg[i], ret);
+ return ret;
+ }
+ }
+ if (map->cache_only) {
+ map->cache_dirty = true;
+ return 0;
+ }
+ }
+
+ trace_regmap_hw_write_start(map->dev, reg[0], reg_count);
+
+ /* Because a multi-write has an array of registers and an array
+ * of values a gather_write will not work and in the simple case
+ * of a single register write the client driver should not be
+ * using this API anyway. So we have to linearise by hand.
+ */
+ unit_len = map->format.reg_bytes + map->format.pad_bytes + val_bytes;
+
+ len = reg_count * unit_len;
+ buf = kzalloc(len, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+ u8 = buf;
+
+ for (i = 0; i < reg_count; i++) {
+
+ map->format.format_reg(u8, reg[i], map->reg_shift);
+ *u8 |= map->write_flag_mask;
+ u8 += map->format.reg_bytes + map->format.pad_bytes;
+
+ memcpy(u8, val + (i * val_bytes), val_bytes);
+ u8 += val_bytes;
+ }
+
+ ret = map->bus->write(map->bus_context, buf, len);
+
+ kfree(buf);
+
+ trace_regmap_hw_write_done(map->dev, reg[0], reg_count);
+
+ return ret;
+}
+
+/*
+ * 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
+ * @val: Block of data to be written, in native register size for device
+ * @reg_count: Number of registers to write
+ *
+ * This function is intended to be used for writing a large block of data
+ * atomically to the device in single transfer for those I2C client devices
+ * that implement this alternative block write mode.
+ *
+ * A value of zero will be returned on success, a negative errno will
+ * be returned in error cases.
+ */
+int regmap_multi_write(struct regmap *map, unsigned int *reg, const void *val,
+ size_t reg_count)
+{
+ int ret = 0, i;
+ size_t val_bytes = map->format.val_bytes;
+ void *wval;
+
+ if (!map->bus)
+ return -EINVAL;
+ if (!map->format.parse_inplace)
+ return -EINVAL;
+
+ map->lock(map->lock_arg);
+
+ /* No formatting is require if val_byte is 1 */
+ if (val_bytes == 1) {
+ wval = (void *)val;
+ } else {
+ wval = kmemdup(val, reg_count * val_bytes, GFP_KERNEL);
+ if (!wval) {
+ ret = -ENOMEM;
+ dev_err(map->dev, "Error in memory allocation\n");
+ goto out;
+ }
+ for (i = 0; i < reg_count * val_bytes; i += val_bytes)
+ map->format.parse_inplace(wval + i);
+ }
+ /*
+ * Some devices do not support multi write, for
+ * them we have a series of single write operations.
+ */
+ if (map->use_single_rw) {
+ for (i = 0; i < reg_count; i++) {
+ ret = _regmap_raw_write(map,
+ reg[i],
+ val + (i * val_bytes),
+ val_bytes,
+ false);
+ if (ret != 0)
+ return ret;
+ }
+ } else {
+ ret = _regmap_raw_multi_write(map,
+ reg_count,
+ reg,
+ wval);
+ }
+
+ if (val_bytes != 1)
+ kfree(wval);
+
+out:
+ map->unlock(map->lock_arg);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(regmap_multi_write);
+
/**
* regmap_raw_write_async(): Write raw values to one or more registers
* asynchronously
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index a10380b..f9c16db 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -378,6 +378,8 @@ 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_multi_write(struct regmap *map, unsigned int *reg, const void *val,
+ size_t reg_count);
int regmap_raw_write_async(struct regmap *map, unsigned int reg,
const void *val, size_t val_len);
int regmap_read(struct regmap *map, unsigned int reg, unsigned int *val);
--
end-of-patch for new API regmap_multi_write() V1
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.
> -----Original Message-----
> From: Mark Brown [mailto:[email protected]]
> Sent: 10 October 2013 13:21
> To: Opensource [Anthony Olech]
> Cc: Greg Kroah-Hartman; LKML; David Dajun Chen
> 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.
As far as I could see, the splitting of a block write that spans a page boundary
requires a read-modify-write of a "page" register. That therefore breaks the
primary raison d'etre for the new API, namely that it is atomic on the I2C bus
with respect to multiple I2C bus masters.
Cutting the transfer at a page boundary and inserting a write to a "page"
register would work very well for most of our PM MFDs because there is
no requirement to do a read modify write. Indeed I had thought that it should
be the responsibility of the device driver to insert any necessary "page" register
writes into a transfer that spans page boundaries. The driver knows where the
boundaries are so it should be easy.
>
> > + * @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...
Yes - 2 arrays with the same dimension.
> 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.
You seem to be suggesting that the API could be used by drivers of devices that
do not support in hardware the MULTIWRITE capability. If that is the case then
driver need an config "multi_write_supported" field for initialization.
> > + /*
> > + * 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.
Thanks Mark for your thoughtful comments.
What should the next step in progressing this be?
Tony Olech - Dialog Semiconductor
On Thu, Oct 10, 2013 at 12:45:03PM +0000, Opensource [Anthony Olech] wrote:
> > 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.
> As far as I could see, the splitting of a block write that spans a page boundary
> requires a read-modify-write of a "page" register. That therefore breaks the
> primary raison d'etre for the new API, namely that it is atomic on the I2C bus
> with respect to multiple I2C bus masters.
I would expect this to be handled by inserting a page update into the
sequence (it could presumably go into the block write unless the
hardware were being a bit perverse) or by just splitting the transfers
at each page change.
> Cutting the transfer at a page boundary and inserting a write to a "page"
> register would work very well for most of our PM MFDs because there is
> no requirement to do a read modify write. Indeed I had thought that it should
> be the responsibility of the device driver to insert any necessary "page" register
> writes into a transfer that spans page boundaries. The driver knows where the
> boundaries are so it should be easy.
This only works if it is chip specific code that is doing the update.
If there is generic code doing an update (either core Linux framework
code or something for an IP that appears on multiple chips) then it's
not going to know that without jumping through hoops which are going to
apply to all users that trigger this so may as well just be handled in
the core.
Of course a driver is free to not issue updates which cross page
boundaries (or to group the writes so that they get split up into the
minimum set of page boundary changes) but it doesn't seem helpful to
require that the caller understands any paging the device has.
> > 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.
> You seem to be suggesting that the API could be used by drivers of devices that
> do not support in hardware the MULTIWRITE capability. If that is the case then
Yes, and for example all SPI devices have essentially this functionality
as standard since it's possible to issue an uninterrupted batch of
transfers to a device with no special hardware support.
> driver need an config "multi_write_supported" field for initialization.
It's I2C specific too.
> What should the next step in progressing this be?
Like I say I'd suggest getting an API that allows drivers to send a
batch of writes to the framework done first (which should be fairly
simple - a first pass should probably be something like
int regmap_multi_reg_write(struct regmap *map,
struct reg_default *regs,
int num_regs)
{
for (i = 0; i < num_regs; i++)
regmap_reg_write(map, regs[i].reg, regs[i].val);
}
with error handling and stuff) and then loop round on how to implement
the actual functionality to get I2C and SPI to do the right thing on the
wire if the device supports it. It's slightly different for each,
obviously you only care about I2C which is fine - I'm not saying you'd
need to actually do SPI yourself.
For I2C type stuff a flag to say if the device can do this and then
something to render the data appropriately and send it ought to do the
trick, it should be fairly straightforward. I'm more concerned about
the external API than the implementation, it's much easier to refactor
the implementation if there's a problem than it is to change the
external API.