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

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

[Mark Brown]

From: Mark Brown <broonie@linaro.org>

The current si476x I/O implementation wraps the regmap for the core with
functions that make the register map cache only when the device is powered
down. This implementation appears to be incomplete since there is no code
to synchronise the cache so writes done while the core is powered down
will be ignored, the device will only be configured if it is powered.

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
remove the ASoC custom ones so do convert to do that.

Signed-off-by: Mark Brown <broonie@linaro.org>
---
 sound/soc/codecs/si476x.c | 46 +---------------------------------------------
 1 file changed, 1 insertion(+), 45 deletions(-)

diff --git a/sound/soc/codecs/si476x.c b/sound/soc/codecs/si476x.c
index 38f3b10..ec29d26 100644
--- a/sound/soc/codecs/si476x.c
+++ b/sound/soc/codecs/si476x.c
@@ -60,48 +60,6 @@ enum si476x_pcm_format {
 	SI476X_PCM_FORMAT_S24_LE	= 6,
 };
 
-static unsigned int si476x_codec_read(struct snd_soc_codec *codec,
-				      unsigned int reg)
-{
-	int err;
-	unsigned int val;
-	struct si476x_core *core = codec->control_data;
-
-	si476x_core_lock(core);
-	if (!si476x_core_is_powered_up(core))
-		regcache_cache_only(core->regmap, true);
-
-	err = regmap_read(core->regmap, reg, &val);
-
-	if (!si476x_core_is_powered_up(core))
-		regcache_cache_only(core->regmap, false);
-	si476x_core_unlock(core);
-
-	if (err < 0)
-		return err;
-
-	return val;
-}
-
-static int si476x_codec_write(struct snd_soc_codec *codec,
-			      unsigned int reg, unsigned int val)
-{
-	int err;
-	struct si476x_core *core = codec->control_data;
-
-	si476x_core_lock(core);
-	if (!si476x_core_is_powered_up(core))
-		regcache_cache_only(core->regmap, true);
-
-	err = regmap_write(core->regmap, reg, val);
-
-	if (!si476x_core_is_powered_up(core))
-		regcache_cache_only(core->regmap, false);
-	si476x_core_unlock(core);
-
-	return err;
-}
-
 static const struct snd_soc_dapm_widget si476x_dapm_widgets[] = {
 SND_SOC_DAPM_OUTPUT("LOUT"),
 SND_SOC_DAPM_OUTPUT("ROUT"),
@@ -239,7 +197,7 @@ static int si476x_codec_hw_params(struct snd_pcm_substream *substream,
 
 static int si476x_codec_probe(struct snd_soc_codec *codec)
 {
-	codec->control_data = i2c_mfd_cell_to_core(codec->dev);
+	codec->control_data = dev_get_regmap(codec->dev.parent);
 	return 0;
 }
 
@@ -268,8 +226,6 @@ static struct snd_soc_dai_driver si476x_dai = {
 
 static struct snd_soc_codec_driver soc_codec_dev_si476x = {
 	.probe  = si476x_codec_probe,
-	.read   = si476x_codec_read,
-	.write  = si476x_codec_write,
 	.dapm_widgets = si476x_dapm_widgets,
 	.num_dapm_widgets = ARRAY_SIZE(si476x_dapm_widgets),
 	.dapm_routes = si476x_dapm_routes,
-- 
1.8.4.rc3

Re: [PATCH] ASoC: si476x: Remove custom register I/O implementation

[Andrey Smirnov]

On Thu, Sep 26, 2013 at 11:34 AM, Mark Brown <broonie@kernel.org> wrote:
> From: Mark Brown <broonie@linaro.org>
>
> The current si476x I/O implementation wraps the regmap for the core with
> functions that make the register map cache only when the device is powered
> down. This implementation appears to be incomplete since there is no code
> to synchronise the cache so writes done while the core is powered down
> will be ignored, the device will only be configured if it is powered.

I wouldn't be surprised if I messed up that part of the code since the
HW that I used to have to test it didn't actually use the codec
driver(I initially developed the driver on a different HW that did use
the driver but after some time I lost my access to that particular HW)

>
> 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.


> remove the ASoC custom ones so do convert to do that.
>
> Signed-off-by: Mark Brown <broonie@linaro.org>
> ---
>  sound/soc/codecs/si476x.c | 46 +---------------------------------------------
>  1 file changed, 1 insertion(+), 45 deletions(-)
>
> diff --git a/sound/soc/codecs/si476x.c b/sound/soc/codecs/si476x.c
> index 38f3b10..ec29d26 100644
> --- a/sound/soc/codecs/si476x.c
> +++ b/sound/soc/codecs/si476x.c
> @@ -60,48 +60,6 @@ enum si476x_pcm_format {
>         SI476X_PCM_FORMAT_S24_LE        = 6,
>  };
>
> -static unsigned int si476x_codec_read(struct snd_soc_codec *codec,
> -                                     unsigned int reg)
> -{
> -       int err;
> -       unsigned int val;
> -       struct si476x_core *core = codec->control_data;
> -
> -       si476x_core_lock(core);
> -       if (!si476x_core_is_powered_up(core))
> -               regcache_cache_only(core->regmap, true);
> -
> -       err = regmap_read(core->regmap, reg, &val);
> -
> -       if (!si476x_core_is_powered_up(core))
> -               regcache_cache_only(core->regmap, false);
> -       si476x_core_unlock(core);
> -
> -       if (err < 0)
> -               return err;
> -
> -       return val;
> -}
> -
> -static int si476x_codec_write(struct snd_soc_codec *codec,
> -                             unsigned int reg, unsigned int val)
> -{
> -       int err;
> -       struct si476x_core *core = codec->control_data;
> -
> -       si476x_core_lock(core);
> -       if (!si476x_core_is_powered_up(core))
> -               regcache_cache_only(core->regmap, true);
> -
> -       err = regmap_write(core->regmap, reg, val);
> -
> -       if (!si476x_core_is_powered_up(core))
> -               regcache_cache_only(core->regmap, false);
> -       si476x_core_unlock(core);
> -
> -       return err;
> -}
> -
>  static const struct snd_soc_dapm_widget si476x_dapm_widgets[] = {
>  SND_SOC_DAPM_OUTPUT("LOUT"),
>  SND_SOC_DAPM_OUTPUT("ROUT"),
> @@ -239,7 +197,7 @@ static int si476x_codec_hw_params(struct snd_pcm_substream *substream,
>
>  static int si476x_codec_probe(struct snd_soc_codec *codec)
>  {
> -       codec->control_data = i2c_mfd_cell_to_core(codec->dev);
> +       codec->control_data = dev_get_regmap(codec->dev.parent);
>         return 0;
>  }
>
> @@ -268,8 +226,6 @@ static struct snd_soc_dai_driver si476x_dai = {
>
>  static struct snd_soc_codec_driver soc_codec_dev_si476x = {
>         .probe  = si476x_codec_probe,
> -       .read   = si476x_codec_read,
> -       .write  = si476x_codec_write,
>         .dapm_widgets = si476x_dapm_widgets,
>         .num_dapm_widgets = ARRAY_SIZE(si476x_dapm_widgets),
>         .dapm_routes = si476x_dapm_routes,
> --
> 1.8.4.rc3
>

Re: [PATCH] ASoC: si476x: Remove custom register I/O implementation

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1519 bytes --]

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?  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.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: si476x: Remove custom register I/O implementation

[Andrey Smirnov]

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.

Re: [PATCH] ASoC: si476x: Remove custom register I/O implementation

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1340 bytes --]

On Thu, Sep 26, 2013 at 12:58:27PM -0700, Andrey Smirnov wrote:
> On Thu, Sep 26, 2013 at 12:43 PM, Mark Brown <broonie@kernel.org> wrote:

> > 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.

But what exactly are these locks supposed to be protecting?

> > 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.

In what way might these things conflict?  What are the clashes you are
concerned about?

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: si476x: Remove custom register I/O implementation

[Andrey Smirnov]

On Thu, Sep 26, 2013 at 1:58 PM, Mark Brown <broonie@kernel.org> wrote:
> On Thu, Sep 26, 2013 at 12:58:27PM -0700, Andrey Smirnov wrote:
>> On Thu, Sep 26, 2013 at 12:43 PM, Mark Brown <broonie@kernel.org> wrote:
>
>> > 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.
>
> But what exactly are these locks supposed to be protecting?
>
>> > 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.
>
> In what way might these things conflict?  What are the clashes you are
> concerned about?

That lock serves the purpose of making sure that
si476x_core_send_command, calls to which all the rest of the functions
boil down to, is not executed in parallel to itself. So I am concerned
that with the lock acquisition removal it would be possible to make
that condition happen.

Re: [PATCH] ASoC: si476x: Remove custom register I/O implementation

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 726 bytes --]

On Thu, Sep 26, 2013 at 03:48:09PM -0700, Andrey Smirnov wrote:
> On Thu, Sep 26, 2013 at 1:58 PM, Mark Brown <broonie@kernel.org> wrote:

> > In what way might these things conflict?  What are the clashes you are
> > concerned about?

> That lock serves the purpose of making sure that
> si476x_core_send_command, calls to which all the rest of the functions
> boil down to, is not executed in parallel to itself. So I am concerned
> that with the lock acquisition removal it would be possible to make
> that condition happen.

OK, so if it's purely about send_command() then why is the locking not
being done in that function?  Surely the most obvious and robust place
to protect the function is within the function itself?

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: si476x: Remove custom register I/O implementation

[Andrey Smirnov]

>
> OK, so if it's purely about send_command() then why is the locking not
> being done in that function?  Surely the most obvious and robust place
> to protect the function is within the function itself?

Not having send_command acquire any locks allows me to have variable
levels of granularity of locking, for example in the worker thread
that fetches RDS data from the chip it allows me to get exclusive
access to the chip for the duration of the FIFO draining. Also any
band switch for that chip, like AM to FM, etc. require chip to be
power cycled which translates to multiple calls to send_command as
well, I think having those performed atomically simplifies the driver
as well.

Even if such fine granularity could be made to work, I think it is a
big change that would need to be tested on the real HW and
unfortunately I don't have any now and I don't believe there are or
has ever been any users of that driver besides me and the company I
used to work for(which no longer ships the device with that chip, so
we can exclude them). I am trying to get some sort of evaluation board
from SiLabs, but until and if they give it to me, any changes like
that can not be verified to work on a real HW.

Re: [PATCH] ASoC: si476x: Remove custom register I/O implementation

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 985 bytes --]

On Fri, Sep 27, 2013 at 08:53:58AM -0700, Andrey Smirnov wrote:

> > OK, so if it's purely about send_command() then why is the locking not
> > being done in that function?  Surely the most obvious and robust place
> > to protect the function is within the function itself?

> Not having send_command acquire any locks allows me to have variable
> levels of granularity of locking, for example in the worker thread
> that fetches RDS data from the chip it allows me to get exclusive
> access to the chip for the duration of the FIFO draining. Also any
> band switch for that chip, like AM to FM, etc. require chip to be
> power cycled which translates to multiple calls to send_command as
> well, I think having those performed atomically simplifies the driver
> as well.

Right, so it's not just protecting send_command() but is instead
protecting higher level things.  In that case shouldn't the locking be
performed further up in this driver too rather than on each register
write?

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: si476x: Remove custom register I/O implementation

[Andrey Smirnov]

On Fri, Sep 27, 2013 at 10:02 AM, Mark Brown <broonie@kernel.org> wrote:
> On Fri, Sep 27, 2013 at 08:53:58AM -0700, Andrey Smirnov wrote:
>
>> > OK, so if it's purely about send_command() then why is the locking not
>> > being done in that function?  Surely the most obvious and robust place
>> > to protect the function is within the function itself?
>
>> Not having send_command acquire any locks allows me to have variable
>> levels of granularity of locking, for example in the worker thread
>> that fetches RDS data from the chip it allows me to get exclusive
>> access to the chip for the duration of the FIFO draining. Also any
>> band switch for that chip, like AM to FM, etc. require chip to be
>> power cycled which translates to multiple calls to send_command as
>> well, I think having those performed atomically simplifies the driver
>> as well.
>
> Right, so it's not just protecting send_command() but is instead
> protecting higher level things.  In that case shouldn't the locking be
> performed further up in this driver too rather than on each register
> write?

That is the case for the V4L driver, in that code the locking is
acquired for logical operations, such "tune" or "power up" rather than
for a single register write, but for the codec driver, to the best of
my knowledge, read and write register are the highest level operations
that it provides. Or am I just unaware of some codec driver
functionality that driver can provide and SoC sound subsystem will use
that would allow the lock to be obtained at higher levels?

Re: [PATCH] ASoC: si476x: Remove custom register I/O implementation

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 998 bytes --]

On Fri, Sep 27, 2013 at 12:28:14PM -0700, Andrey Smirnov wrote:

> That is the case for the V4L driver, in that code the locking is
> acquired for logical operations, such "tune" or "power up" rather than
> for a single register write, but for the codec driver, to the best of
> my knowledge, read and write register are the highest level operations
> that it provides. Or am I just unaware of some codec driver
> functionality that driver can provide and SoC sound subsystem will use
> that would allow the lock to be obtained at higher levels?

Since the driver doesn't expose any controls to userspace the only
things that can cause register writes are the operations provided by the
CODEC driver, they could take the lock for their runtime (which is about
the level the v4l stuff is running at AIUI).

This still won't fix the issue with the device being powered off but I
guess that normal usage patterns will have powered the device up to do
tuning or something prior to audio being started.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: si476x: Remove custom register I/O implementation

[Andrey Smirnov]

>
> Since the driver doesn't expose any controls to userspace the only
> things that can cause register writes are the operations provided by the
> CODEC driver, they could take the lock for their runtime (which is about
> the level the v4l stuff is running at AIUI).

Do you mean moving it to si476x_codec_set_dai_fmt and
si476x_codec_hw_params? If those are the only two places where
register read and write are called by SoC sound subsystem then I think
that would work.

> This still won't fix the issue with the device being powered off but I
> guess that normal usage patterns will have powered the device up to do
> tuning or something prior to audio being started

You mean the issue cause by my forgetting to sync cache on power up?
That would be a subject for a separate patch.

Re: [PATCH] ASoC: si476x: Remove custom register I/O implementation

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 690 bytes --]

On Fri, Sep 27, 2013 at 09:37:01PM -0700, Andrey Smirnov wrote:

> > Since the driver doesn't expose any controls to userspace the only
> > things that can cause register writes are the operations provided by the
> > CODEC driver, they could take the lock for their runtime (which is about
> > the level the v4l stuff is running at AIUI).

> Do you mean moving it to si476x_codec_set_dai_fmt and
> si476x_codec_hw_params? If those are the only two places where
> register read and write are called by SoC sound subsystem then I think
> that would work.

Yes, exactly.  Unless you provide controls or DAPM nothing outside of
the callbacks in your driver is going to interact with registers.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]