On Fri, 16 Jan 2015, Mark Brown wrote:
On Fri, Jan 16, 2015 at 06:36:14PM +0100, Paul Osmialowski wrote:
On Fri, 16 Jan 2015, Mark Brown wrote:
I don't know what this means, sorry. I'm also very worried about the
fact that this is being discussed purely in terms of I2C - why would
this not affect other buses?
I tried to open some gate for further extension to any bus that is used for
regmap communications. Currently it goes down to regmap-i2c.c since I
enhanced i2c API for this. Anyone who feels it is useful or saves oneself
from locking troubles can voluntarily adapt other regmap-i2c.* places (as
needed?).
My whole point is that I proposed a way to solve nasty deadlock which is
better to fix than just leave as it is. I got a feeling that situation I
adressed here may occur others too, so I proposed this extension that allows
future adaptations. I don't expect it to be accepted easily (i.e. I'm new
here and have mixed feelins about proposing changes that go so far),
therefore I prepared other solution for this particular deadlock that occurs
on this particular device.
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.
Maybe adding a pair of callbacks (map->reg_write_sync_prepared(),
map->reg_read_sync_prepared()) would make situation clearer.
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.
+ void (*reg_unprepare_sync_io)(void *context);
The first question here is why this only affects synchronous I/O or
alternatively why these operations have _sync in the name if they aren't
for synchronous I/O.
IMHO this whole idea is against asynchronous I/O.
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().
+ if (bus) {
+ map->reg_prepare_sync_io = regmap_bus_prepare_sync_io;
+ map->reg_unprepare_sync_io = regmap_bus_unprepare_sync_io;
+ }
Why are we using these indirections instead of assigning the operation
directly? They...
I followed the pattern used throughout this file.
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
