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

[Andrey Smirnov]

On Thu, Sep 26, 2013 at 11:34 AM, Mark Brown <broonie at kernel.org> wrote:
> From: Mark Brown <broonie at 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 at 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
>