Re: [PATCH V2 2/9] sound:asoc: Add support for STA529 Audio Codec

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

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

On Thu, Jun 21, 2012 at 03:54:50PM +0530, Rajeev Kumar wrote:
> This patch adds the support for STA529 audio codec.

As I said last time always use subject lines corresponding to the style
of the subsystem, don't make up your own style.

This is mostly OK, a few things below but nothing terribly major.

> +static int sta529_mute(struct snd_soc_dai *dai, int mute)
> +{
> +	u8 val;
> +
> +	if (mute)
> +		val |= CODEC_MUTE_VAL;

You need to initialise val otherwise it's going to have an undefined
value when you're not muting.

> +	codec->control_data = sta529->regmap;
> +	ret = snd_soc_codec_set_cache_io(codec, 8, 8, SND_SOC_I2C);

You've got a regmap, you should be using SND_SOC_REGMAP as the control
mode.

> +	snd_soc_add_codec_controls(codec, sta529_snd_controls,
> +			ARRAY_SIZE(sta529_snd_controls));
> +
> +	snd_soc_add_codec_controls(codec, sta529_new_snd_controls,
> +			ARRAY_SIZE(sta529_new_snd_controls));

Set these in the CODEC driver structure and merge the two arrays, no
point in having code for this.

> +static int sta529_resume(struct snd_soc_codec *codec)
> +{
> +	snd_soc_cache_sync(codec);

You're not using the ASoC cache code.

> +	sta529_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
> +	sta529_set_bias_level(codec, codec->dapm.suspend_bias_level);

The second of these isn't needed, the core will ensure the device is in
standby before suspend.

> +	sta529->regmap = regmap_init_i2c(i2c, &sta529_regmap);
> +	if (IS_ERR(sta529->regmap)) {

devm_regmap_init_i2c() then you don't need any cleanup code for it.

> +static int __init sta529_modinit(void)
> +{
> +	int 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 --]