Hello Mark
On 3/20/2012 8:55 PM, Mark Brown wrote:
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.
Ok
+config SND_SOC_STA529
- tristate
- depends on I2C
Agreed
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...
Oops
+static const u8 sta529_reg[STA529_CACHEREGNUM] = {
Use regmap for register I/O.
OK
+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).
Agreed,
+static const char *pwm_mode_text[] = { "binary", "headphone", "ternary",
- "phase-shift"};
+static const char *interface_mode_text[] = { "slave", "master"};
ALSA controls always use capitalisation.
you mean to say static const char *interface_mode_text[] = { "SLAVE", "MASTER"}
- 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.
OK I will take mask for this.
- 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.
In default , it is returning an error.
- default:
printk(KERN_WARNING, "bad rate\n");return -EINVAL;- }
dev_warn() - always use the dev_ prints where possible.
Agreed,
- sta529_set_bias_level(codec, SND_SOC_BIAS_PREPARE);
- mdelay(10);
Absolutely no - why are you doing this?
In probe I am putting codec in standby mode. so to come out of this sta529_set_bias_level(codec, SND_SOC_BIAS_PREPARE) is called. Since there are some transition time between STANDBY and ON/PREAPRE, so a delay is introduced in the code.
+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...
Oops, it should be snd_soc_update_bits(codec, STA529_FFXCFG0, 0x80, mute_reg);
- 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.
ok
- sta529 = kzalloc(sizeof(struct sta529), GFP_KERNEL);
- if (sta529 == NULL)
return -ENOMEM;devm_kzalloc().
OK
Best Regards Rajeev
+static int __init sta529_modinit(void)
module_i2c_driver().