On Thu, Sep 26, 2013 at 12:43 PM, Mark Brown broonie@kernel.org wrote:
On Thu, Sep 26, 2013 at 12:33:33PM -0700, Andrey Smirnov wrote:
On Thu, Sep 26, 2013 at 11:34 AM, Mark Brown broonie@kernel.org wrote:
A better and more idiomatic approach would be to have the MFD manage the cache, making the device cache only when it powers things down. This also allows ASoC to use the standard regmap helpers for the device which helps
Correct me if my understanding is wrong but using standard helper function would mean using hw_write and hw_read from soc-io.c. Looking at the source code of those functions I don't believe the are performing any sort of locking(once again I may be wrong) and rely on regmap_* implementation to do it internally. With si476x driver it is not the case and it is important that codec write/read functions perform si476x_core_lock(core) before they call regmap functions. If that is not done it is possible to wreak havoc on I2C bus by trying to access the registers of the codec and doing some V4L2 operations on the corresponding radio device.
Although I completely agree that cache managing should be moved to MFD.
What exactly is the locking you're talking about here?
I am talking about locking specific for si476x MFD device that is hadled by si476x_core_lock / si476x_core_unlock. The code before the patch has both externally exposed functions via V4L2 driver and SoC driver obtain said lock for the duration of their interaction with the device. The patch appears to remove lock acquisition for SoC functions.
Both regmap and I/O have locking at those levels, if you try to do a bus interaction using them it shouldn't conflict with any other register write. If it's something higher level then shouldn't the locking be higher level too?
The way the code is written it looks like the lock is being held to ensure there's no race due to a power state change.
That lock is being held to prevent device access contention between all of the code that can be triggered to be executed via V4L2 ioctl API and SoC API. As an example, I can open the radio device and start tuning it using corresponding ioctl, meanwhile, parallel to that process my other code is trying to change the sample rate of the codec. To prevent those two from clashes both of them try to acquire "core lock" with si476x_core_lock before the start doing anything with chip/MFD.