Re: [PATCH v6] sound/soc/codecs: add LAPIS Semiconductor ML26124

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

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

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

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

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