Re: [PATCH 1/8] sound:asoc: Add support for STA529 Audio Codec

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

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

On Tue, Mar 20, 2012 at 05:03:45PM +0530, Rajeev Kumar wrote:
> This patch adds the support for STA529 audio codec.
> Details of the audio codec can be seen here:
> http://www.st.com/internet/imag_video/product/159187.jsp

Please use subject lines appropriate for the subsystem.  If your commits
look visually different to all the other commits in the log that's not a
good sign.

> +config SND_SOC_STA529
> +	tristate
> +	depends on I2C
> +

Drop the dependency, none of the other CODECs have this for a reason...

> +#include <linux/platform_device.h>

If you need this something went wrong...

> +static const u8 sta529_reg[STA529_CACHEREGNUM] = {

Use regmap for register I/O.

> +struct sta529 {
> +	unsigned int sysclk;
> +	enum snd_soc_control_type control_type;
> +	void *control_data;

Your device only supports I2C, no need for the infrastructure for other
buses (though this will go away with regmap).

> +static const char *pwm_mode_text[] = { "binary", "headphone", "ternary",
> +	"phase-shift"};
> +static const char *interface_mode_text[] = { "slave", "master"};

ALSA controls always use capitalisation.

> +	case SND_SOC_BIAS_PREPARE:
> +		snd_soc_update_bits(codec, STA529_FFXCFG0, POWER_CNTLMSAK,
> +				POWER_UP);
> +		snd_soc_update_bits(codec, STA529_MISC, 1, 0x01);

One of these has magic numbers and the other doesn't, and the magic
numbers are a little confusing as the mask is written in decimal but the
binary in hex.

> +	switch (params_format(params)) {
> +	case SNDRV_PCM_FORMAT_S16_LE:
> +		pdata = 1;
> +		break;
> +	case SNDRV_PCM_FORMAT_S24_LE:
> +		pdata = 2;
> +		break;
> +	case SNDRV_PCM_FORMAT_S32_LE:
> +		pdata = 3;
> +		break;
> +	}

Return an error on unsupported sample sizes.

> +	default:
> +		printk(KERN_WARNING, "bad rate\n");
> +		return -EINVAL;
> +	}

dev_warn() - always use the dev_ prints where possible.

> +	sta529_set_bias_level(codec, SND_SOC_BIAS_PREPARE);
> +	mdelay(10);

Absolutely no - why are you doing this?

> +static int sta529_mute(struct snd_soc_dai *dai, int mute)
> +{
> +	struct snd_soc_codec *codec = dai->codec;
> +
> +	u8 mute_reg = snd_soc_read(codec, STA529_FFXCFG0) & ~CODEC_MUTE_VAL;
> +
> +	if (mute)
> +		mute_reg |= CODEC_MUTE_VAL;
> +
> +	snd_soc_update_bits(codec, STA529_FFXCFG0, 0x80, 00);

You're always setting the same value here...

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

Use the initialisers in the card struct.

> +	sta529 = kzalloc(sizeof(struct sta529), GFP_KERNEL);
> +	if (sta529 == NULL)
> +		return -ENOMEM;

devm_kzalloc().

> +static int __init sta529_modinit(void)

module_i2c_driver().

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

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