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