Message-id: <alpine.DEB.2.10.1501191031080.13777@AMDC1262.digital.local>
Date:	Mon, 19 Jan 2015 10:31:22 +0100 (CET)
From:	Paul Osmialowski <p.osmialowsk@...sung.com>
To:	Mark Brown <broonie@...nel.org>
Cc:	Wolfram Sang <wsa@...-dreams.de>, Jonathan Corbet <corbet@....net>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Kukjin Kim <kgene@...nel.org>, linux-i2c@...r.kernel.org,
	linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-samsung-soc@...r.kernel.org, p.osmialowsk@...sung.com
Subject: Re: [RFC 2/3] regmap: Use the enhancement of i2c API to address
 circular dependency problem



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-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists