[alsa-devel] [PATCH] ASoC: si476x: Remove custom register I/O implementation

[Andrey Smirnov]

On Thu, Sep 26, 2013 at 12:43 PM, Mark Brown <broonie at 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 at 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.