Re: [PATCH 01/19] ASoC: upd9976: Add Renesas uPD9976 codec driver

[Lu Guanqun <guanqun.lu@intel.com>]

On Wed, May 04, 2011 at 10:46:00PM +0800, Dimitris Papastamos wrote:
> On Wed, May 04, 2011 at 09:44:58PM +0800, Lu Guanqun wrote:
> > +	if (ret)
> > +		pr_err("upd9976 read of 0x%x failed, error: %d\n", reg, ret);
> > +	return value;
> > +}
> 
> dev_err() would be more preferable.

OK.

> > +	switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
> > +	case SND_SOC_DAIFMT_CBM_CFM:
> > +	case SND_SOC_DAIFMT_CBM_CFS:
> > +		mode |= BIT(7) | BIT(3);
> > +		break;
> > +	}
> > +
> > +	return snd_soc_update_bits(codec, UPD9976_AUDIOPORT1, mask, mode);
> > +}
> 
> I am not sure whether the BIT() macro is more confusing than helpful.

I was trying to be helpful. :) but if that confuses you.,I'm OK to use hex
number.

> > +	switch (params_rate(params)) {
> > +	case 8000:
> > +		tmp = 0x00;
> > +		break;
> > +	case 11025:
> > +		tmp = 0x01;
> > +		break;
> > +	case 12000:
> > +		tmp = 0x02;
> > +		break;
> > +	case 16000:
> > +		tmp = 0x03;
> > +		break;
> > +	case 22050:
> > +		tmp = 0x04;
> > +		break;
> > +	case 24000:
> > +		tmp = 0x05;
> > +		break;
> > +	case 32000:
> > +		tmp = 0x07;
> > +		break;
> > +	case 44100:
> > +		tmp = 0x08;
> > +		break;
> > +	case 48000:
> > +		tmp = 0x09;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> 
> Looks fine, I'd rather use an array though.

OK. Array makes it less code.

> 
> > +static int upd9976_set_bias_level(struct snd_soc_codec *codec,
> > +				  enum snd_soc_bias_level level)
> > +{
> > +	switch (level) {
> > +	case SND_SOC_BIAS_ON:
> > +		break;
> > +
> > +	case SND_SOC_BIAS_PREPARE:
> > +		if (codec->dapm.bias_level == SND_SOC_BIAS_STANDBY) {
> > +			snd_soc_update_bits(codec, UPD9976_VAUDIOCNT,
> > +					    0x27, 0x27);
> > +			snd_soc_update_bits(codec, UPD9976_VREFPLL,
> > +					    0x35, 0x35);
> > +		}
> > +		break;
> > +
> > +	case SND_SOC_BIAS_STANDBY:
> > +		snd_soc_write(codec, UPD9976_VAUDIOCNT, 0x25);
> > +		snd_soc_write(codec, UPD9976_VREFPLL, 0x10);
> > +		break;
> > +
> > +	case SND_SOC_BIAS_OFF:
> > +		snd_soc_write(codec, UPD9976_VREFPLL, 0);
> > +		snd_soc_write(codec, UPD9976_VAUDIOCNT, 0x24);
> > +		break;
> > +	}
> 
> Why not snd_soc_update_bits()?  These should normally be DAPM widgets.

There's no DAPM widgets bound to these two registers. So I'm afraid it's
OK to use snd_soc_write when it's been powered off totally.

> 
> > +static int upd9976_codec_probe(struct snd_soc_codec *codec)
> > +{
> > +	upd9976_set_bias_level(codec, SND_SOC_BIAS_OFF);
> > +
> > +	return 0;
> > +}
> 
> Why SND_SOC_BIAS_OFF and not SND_SOC_BIAS_STANDBY?

Try to use as little power as possible.

> 
> > +static int upd9976_codec_remove(struct snd_soc_codec *codec)
> > +{
> > +	return 0;
> > +}
> 
> You can call upd9976_set_bias_level(codec, SND_SOC_BIAS_OFF) here.

Thanks for reminding this.

-- 
guanqun