Re: [PATCH V1] new API regmap_multi_write()

[Mark Brown]

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.

Attachment: signature.asc
Description: Digital signature