Re: [RFC 2/3] regmap: Use the enhancement of i2c API to address circular dependency problem

[Mark Brown]

On Mon, Jan 19, 2015 at 10:31:22AM +0100, Paul Osmialowski wrote:
> On Fri, 16 Jan 2015, Mark Brown wrote:

> >What I'm saying is that I want to understand this change from a point of
> >view that isn't tied to I2C - at the regmap level what is this doing,

> From the regmap point of view, it allows its functions to have a chance to
> prepare transfer medium for (synchronous) transfer (no matter what bus
> handles it) before it actually start to happen (then unprepare it when it's
> done) and crucially before any lock is obtained in functions like
> regmap_write(), regmap_read() or regmap_update_bits.

OK, so that's what should go in the changelog (along with an explanation
of why this preparation is required at all) - but I still don't see the
async bit of this I'm afraid.

> Maybe adding a pair of callbacks (map->reg_write_sync_prepared(),
> map->reg_read_sync_prepared()) would make situation clearer.

No, I don't think so - it'd just complicate the callers.

> >I2C is a bus that has some properties which you're saying needs some
> >changes, what are those properties and those changes?

> I'm not saying I2C as a bus requires changes. What I'm saying is that I2C
> API can be extended to allow more detailed control on what happens with the
> transfer.

My point here is that your explanation is in terms of I2C specifics and
not really at a generic regmap level.

> >Can you be more specific please?  If something needs preparing it seems
> >like it'd need preparing over an async transaction just as much as over
> >a synchronous one.

> Even with those preparation and unpreparation stages, this transfer is still
> synchronous. For example, it starts when regmap_read() starts and ends when
> regmap_read() ends. Nothing is queued or deferred. Namely, when
> max_gen_clk_unprepare() function calls regmap_update_bits() it expects that
> when regmap_update_bits() returned, no outstanding transfer are happening
> nor waiting to proceed. Everything must be completed before returning to
> max_gen_clk_unprepare().

That doesn't address my question - all you're saying is that in a
synchronous call path things are synchronous which is fine but obviously
regmap supports async I/O too.

> >Not in this pattern where the caller needs to check too.

> I don't persist on that. Apparently, you're the author of this file, though
> regmap_init() function was later expanded by other guys. They never assigned
> bus callback function pointers directly to map operation callbacks. It is
> possible to replace 'map->reg_prepare_sync_io = regmap_bus_prepare_sync_io'
> with 'map->reg_prepare_sync_io = map->bus->prepare_sync_io' - this will
> compile and this will work properly. But IMHO it wouldn't match with what
> the others did.

If you look at the other callbacks they're doing other things beyond
simply forwarding the functions on.  That's the problem here, the
functions just add a layer of indirection and nothing else.

Attachment: signature.asc
Description: Digital signature