lkml.org 
[lkml]   [2012]   [Feb]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v6] sound/soc/codecs: add LAPIS Semiconductor ML26124
On Thu, Feb 23, 2012 at 02:46:49PM +0900, Tomoya MORINAGA wrote:

> + SOC_SINGLE_TLV("Playback Limitter Min Input Volume",
> + ML26124_PL_MAXMIN_GAIN, 0, 7, 0, mingain),

Limiter.

> + SOC_SINGLE("Zero Cross Comparator Switch", ML26124_PW_ZCCMP_PW_MNG, 1,
> + 1, 0),

Usually just "ZC Switch".

> + SOC_SINGLE("Digital Switch", ML26124_DVOL_CTL, 4, 1, 0),

This should probably be Digital Playback Switch so it lines up with the
volume control in the UIs (and the Volume controls should be Digital X
Volume).

> +static int ml26124_update_bits(struct snd_soc_codec *codec, unsigned short reg,
> + unsigned int mask, unsigned int value)

Why are you open coding this in your driver? There is no point in
replicating subsystem functionality.

> +if (!priv)
> + return -1;
> +

Coding style and return real error codes (though this looks like it
should never happen).

> +static int ml26124_set_dai_clkdiv(struct snd_soc_dai *codec_dai,
> + int div_id, int div)
> +{
> + struct snd_soc_codec *codec = codec_dai->codec;
> +
> + switch (div_id) {
> + case ML26124_MCLK:
> + ml26124_update_bits(codec, ML26124_CLK_CTL,
> + BIT(0) | BIT(1), div);
> + break;

Why can't the driver calculate this automatically given the MCLK?

> + case SND_SOC_BIAS_ON:
> + pr_debug("%s: level=ON priv=%p\n", __func__, priv);

There's already more than enough logging in the subsystem for this.

> + /* VMID ON */
> + ml26124_update_bits(codec, ML26124_PW_REF_PW_MNG,
> + ML26124_VMID, ML26124_VMID);
> + msleep(500);

This looks like it should be _STANDBY - usually VMID must be on to power
everything else.

> +static int ml26124_pcm_trigger(struct snd_pcm_substream *substream,
> + int cmd, struct snd_soc_dai *codec_dai)
> +{
> + struct snd_soc_codec *codec = codec_dai->codec;
> +
> + switch (cmd) {
> + case SNDRV_PCM_TRIGGER_STOP:
> + case SNDRV_PCM_TRIGGER_SUSPEND:
> + case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
> + ml26124_update_bits(codec, ML26124_REC_PLYBAK_RUN, 0x3,
> + 0);

I'm not sure you've tested this, you can't do I2C I/O from atomic
context and trigger is atomic. What does this control actually do?

> + return 0;
> + break;

Duplicate return and break here.

> +static int ml26124_resume(struct snd_soc_codec *codec)
> +{
> + struct ml26124_priv *priv = snd_soc_codec_get_drvdata(codec);
> +
> + regcache_sync(priv->regmap);
> + ml26124_set_bias_level(codec, SND_SOC_BIAS_STANDBY);

Setting the bias level will duplicate the sync.

> +static int ml26124_probe(struct snd_soc_codec *codec)
> +{
> + int ret;
> + struct ml26124_priv *priv = snd_soc_codec_get_drvdata(codec);
> + codec->control_data = priv->regmap;
> +
> + ret = snd_soc_codec_set_cache_io(codec, 8, 8, SND_SOC_I2C);
> + if (ret < 0) {
> + dev_err(codec->dev, "Failed to set cache I/O: %d\n", ret);
> + return ret;
> + }

You're mixing regmap API usage and ASoC level I2C, this should be
_REGMAP.

> +/* power down chip */
> +static struct snd_soc_codec_driver soc_codec_dev_ml26124 = {

This comment is misplaced.

> +static struct i2c_driver ml26124_i2c_driver = {
> + .driver = {
> + .name = "ml26124-codec",

No -codec, the device only does one thing.

> +static int __init ml26124_modinit(void)
> +{
> + int ret;
> +
> + ret = i2c_add_driver(&ml26124_i2c_driver);
> + if (ret != 0)

module_i2c_driver().
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2012-03-01 00:55    [from the cache]
©2003-2011 Jasper Spaans