Mark Brown broonie@opensource.wolfsonmicro.com writes:
Hi,
On Tue, Oct 12, 2010 at 11:44:54AM +0200, Arnaud Patard wrote:
This patch is adding support for alc562[123] codecs. It's based on the source code available in HP source code and other places.
Looks very good - a few minor things below but they should be fairly easy to fix up.
+static int amp_mixer_event(struct snd_soc_dapm_widget *w,
- struct snd_kcontrol *kcontrol, int event)
+{
- /* index 0x46: class-d internal register */
- snd_soc_write(w->codec, ALC5623_HID_CTRL_INDEX, 0x46);
This is a bit magic; some comments would help.
ok. I've added a comment. The aim is to power on/off the class-d amp parts and to do that one has to write to index-46h reg and to do that one has to write 0x46 to ALC5623_HID_CTRL_INDEX and then the value into ALC5623_HID_CTRL_DATA.
- if (pll_id == ALC5623_PLL_FR_MCLK) {
for (i = 0; i < ARRAY_SIZE(codec_master_pll_div); i++) {if (codec_master_pll_div[i].pll_in == freq_in&& codec_master_pll_div[i].pll_out == freq_out) {/* PLL source from MCLK */pll_div = codec_master_pll_div[i].regvalue;break;}}- } else {
for (i = 0; i < ARRAY_SIZE(codec_slave_pll_div); i++) {I'd expect a switch statement here.
ok.
- coeff = get_coeff(codec, rate);
- if (coeff >= 0) {
coeff = coeff_div[coeff].regvalue;dev_dbg(codec->dev, "%s: sysclk=%d,rate=%d,coeff=0x%04x\n",__func__, alc5623->sysclk, rate, coeff);snd_soc_write(codec, ALC5623_STEREO_AD_DA_CLK_CTRL, coeff);- }
Shouldn't we be returning an error if we can't get coefficients?
I was hoping that current value of the ALC5623_STEREO_AD_DA_CLK_CTRL would be ok but you're probably right. It should be safer to return an error.
- switch (level) {
- case SND_SOC_BIAS_ON:
enable_power_depop(codec);break;enable_power_depop() takes a rather long time - about 500ms - which is surprising for _ON. Are you sure it should be done here?
It was there in the original driver and when working on this driver, I didn't see any reason for moving it elsewhere. tbh, if I have to move it elsewhere, I don't know where I'll put it.
+static int alc5623_suspend(struct snd_soc_codec *codec, pm_message_t mesg) +{
- /* we only need to suspend if we are a valid card */
- if (!codec->card)
return 0;This isn't needed any more, this won't get called if there isn't a card.
ok. removed.
- /* charge alc5623 caps */
- if (codec->suspend_bias_level == SND_SOC_BIAS_ON) {
alc5623_set_bias_level(codec, SND_SOC_BIAS_STANDBY);codec->bias_level = SND_SOC_BIAS_ON;schedule_delayed_work(&codec->delayed_work,msecs_to_jiffies(caps_charge));- }
Just go directly to the bias level you want, don't use delayed work. ASoC will already do resume out of line with the main system so there's no need to do this and it will disrupt audio operation just after resume if you do it.
ok.
Arnaud