Re: [PATCH] ASoC: codecs: Add DA732x codec driver

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

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

On Tue, May 22, 2012 at 12:57:35PM +0000, Software Maintainer wrote:

> +static void da732x_set_charge_pump(struct snd_soc_codec *codec, int state)
> +{
> +	switch (state) {
> +	case DA732X_ENABLE_CP:

You only ever enable this and it seems like it should be a widget - why
is it not a widget?

> +static const char *da732x_eq_text[] = {
> +	"Disable", "Enable",
> +};

This should be a switch not an enum.

> +static int da732x_hpf_set(struct snd_kcontrol *kcontrol,
> +			  struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);
> +	struct soc_enum *enum_ctrl = (struct soc_enum *)kcontrol->private_value;
> +	unsigned int reg = enum_ctrl->reg;
> +	unsigned int sel = ucontrol->value.integer.value[0];
> +	unsigned int bits;
> +
> +	switch (sel) {
> +	case DA732X_HPF_DISABLED:
> +		bits = DA732X_HPF_DIS;
> +		break;
> +	case DA732X_HPF_VOICE:
> +		bits = DA732X_HPF_VOICE_EN;
> +		break;
> +	case DA732X_HPF_MUSIC:
> +		bits = DA732X_HPF_MUSIC_EN;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	snd_soc_update_bits(codec, reg, DA732X_HPF_MASK, bits);

Why is this not done using a normal enum?

> +	if ((val & DA732X_HPF_MUSIC_EN) == DA732X_HPF_MUSIC_EN)
> +		ucontrol->value.integer.value[0] = DA732X_HPF_MUSIC;
> +	else if ((val & DA732X_HPF_VOICE_EN) == DA732X_HPF_VOICE_EN)
> +		ucontrol->value.integer.value[0] = DA732X_HPF_VOICE;
> +	else
> +		ucontrol->value.integer.value[0] = DA732X_HPF_DISABLED;

Similarly here, and this looks like a switch statement.

> +static int da732x_info_volsw(struct snd_kcontrol *kcontrol,
> +	struct snd_ctl_elem_info *uinfo)
> +{
> +	struct soc_mixer_control *mc =
> +		(struct soc_mixer_control *)kcontrol->private_value;
> +	int platform_max;
> +	int min = mc->min;
> +
> +	if (!mc->platform_max)
> +		mc->platform_max = mc->max;
> +	platform_max = mc->platform_max;
> +
> +	uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER;
> +	uinfo->count = 1;
> +	uinfo->value.integer.min = 0;
> +	uinfo->value.integer.max = platform_max - min;

This should be generic code, there's nothing device specific about this
control type and it'd be useful for other devices.

> +	/* MICs */
> +	SOC_SINGLE("MIC1 Mute Switch", DA732X_REG_MIC1, DA732X_MIC_MUTE_SHIFT,
> +		   DA732X_SWITCH_MAX, DA732X_INVERT),

Just MIC1 Switch.

> +	/* ADCs */
> +	SOC_SINGLE_TLV("ADC1 L Capture", DA732X_REG_ADC1_SEL,
> +		       DA732X_ADCL_VOL_SHIFT, DA732X_ADC_VOL_VAL_MAX,
> +		       DA732X_INVERT, adc_pga_tlv),

All these should be Volumes, and really they should be stereo controls.

> +	/* DACs */
> +	SOC_DOUBLE_R_TLV("Digital Playback Volume DAC12", DA732X_REG_DAC1_L_VOL,
> +			 DA732X_REG_DAC1_R_VOL, DA732X_DAC_VOL_SHIFT,
> +			 DA732X_DAC_VOL_VAL_MAX, DA732X_INVERT, dac_pga_tlv),

Volume needs to be the last thing in the name.

> +static int da732x_adc_event(struct snd_soc_dapm_widget *w,
> +			    struct snd_kcontrol *kcontrol, int event)
> +{
> +	struct snd_soc_codec *codec = w->codec;
> +
> +	switch (event) {
> +	case SND_SOC_DAPM_POST_PMU:
> +		snd_soc_update_bits(codec, w->reg, 1 << w->shift,
> +				    1 << w->shift);
> +		snd_soc_update_bits(codec, w->reg,
> +				    DA732X_ADC_ACT_MASK(w->shift),
> +				    DA732X_ADC_ACT_EN(w->shift));
> +		snd_soc_update_bits(codec, w->reg + DA732X_ADC_REG_SHIFT,
> +				    DA732X_ADC_MASK(w->shift),
> +				    DA732X_ADC_EN(w->shift));
> +		break;

This looks like you perhaps need some supply widgets.  Similarly for
most of your events.

> +	/* Micbias */
> +	SND_SOC_DAPM_MICBIAS("MICBIAS1", DA732X_REG_MICBIAS1,
> +			     DA732X_MICBIAS_EN_SHIFT, 0),
> +	SND_SOC_DAPM_MICBIAS("MICBIAS2", DA732X_REG_MICBIAS2,
> +			     DA732X_MICBIAS_EN_SHIFT, 0),

Don't use MICBIAS widgets for new code, use supply widgets.


> +	{"MICBIAS1", "NULL", "MIC1"},
> +	{"MICBIAS2", "NULL", "MIC2"},

The MICBIASes should be connected on the board.

> +	switch (dai->id) {
> +	case DA732X_DAI_ID1:
> +		reg_aif = DA732X_REG_AIFA1;
> +		break;

Use dai->base.

> +	aif = snd_soc_read(codec, reg_aif);
> +	switch (params_format(params)) {
> +	case SNDRV_PCM_FORMAT_S16_LE:
> +		aif |= DA732X_AIF_WORD_16;
> +		break;
> +	case SNDRV_PCM_FORMAT_S20_3LE:
> +		aif |= DA732X_AIF_WORD_20;
> +		break;
> +	case SNDRV_PCM_FORMAT_S24_LE:
> +		aif |= DA732X_AIF_WORD_24;
> +		break;
> +	case SNDRV_PCM_FORMAT_S32_LE:
> +		aif |= DA732X_AIF_WORD_32;
> +		break;

Use snd_soc_update_bits, this will fix the issue where you don't
currently clear the register.  You're also missing error handling in the
default: case.


> +	switch (params_rate(params)) {
> +	case SR_8000:

Don't make up private defines for generic things like this.  Though
really a define like this is a bit silly...

> +static int da732x_set_dai_pll(struct snd_soc_codec *codec, int pll_id,
> +			      int source, unsigned int freq_in,
> +			      unsigned int freq_out)
> +{
> +	struct da732x_priv *da732x = snd_soc_codec_get_drvdata(codec);
> +	int fref, indiv;
> +	u8 div_lo, div_mid, div_hi;
> +	u64 frac_div;
> +
> +	if (da732x->pll_en)
> +		return 0;

This should either stop and restart the PLL or return an error if it's
already enabled.

> +	if (source == DA732X_SRCCLK_MCLK) {
> +		snd_soc_write(codec, DA732X_REG_PLL_CTRL, DA732X_PLL_BYPASS);
> +		return 0;
> +	}

This doesn't validate against the sysclk rate...

> +	indiv = da732x_get_input_div(codec, da732x->sysclk);
> +	if (indiv < 0)
> +		return -EINVAL;

Pass back the error code.

> +	snd_soc_update_bits(codec, DA732X_REG_PLL_CTRL, DA732X_PLL_EN,
> +			    DA732X_PLL_EN);
> +
> +	da732x->pll_en = true;

This means there's no way to stop the PLL.

> +	if (mute) {
> +		snd_soc_update_bits(codec, DA732X_REG_DAC1_SOFTMUTE,
> +				    DA732X_SOFTMUTE_MASK,
> +				    1 << DA732X_SOFTMUTE_SHIFT);
> +		snd_soc_update_bits(codec, DA732X_REG_DAC2_SOFTMUTE,
> +				    DA732X_SOFTMUTE_MASK,
> +				    1 << DA732X_SOFTMUTE_SHIFT);
> +		snd_soc_update_bits(codec, DA732X_REG_DAC3_SOFTMUTE,
> +				    DA732X_SOFTMUTE_MASK,
> +				    1 << DA732X_SOFTMUTE_SHIFT);

This appears to mute all the DACs but you have two audio interfaces and
also had some other code which also managed the mutes...  If there's no
mutes on the actual audio interfaces just don't implement this.

> +#define DA732X_RATES (SNDRV_PCM_RATE_8000 | SNDRV_PCM_RATE_11025 | \
> +		      SNDRV_PCM_RATE_16000 | SNDRV_PCM_RATE_22050 | \
> +		      SNDRV_PCM_RATE_44100 | SNDRV_PCM_RATE_48000)

SNDRV_PCM_RATE_8000_48000

> +	/* Init Codec */
> +	snd_soc_write(codec, DA732X_REG_REF1, DA732X_VMID_FASTCHG);
> +	snd_soc_write(codec, DA732X_REG_BIAS_EN, DA732X_BIAS_EN);
> +
> +	mdelay(DA732X_STARTUP_DELAY);

Almost all of this sequence looks like it should be in set_bias_level(),
it'll be needed every time the chip is powered up and sequencing looks
important.

> +	/* Enable Charge Pump */
> +	da732x_set_charge_pump(codec, DA732X_ENABLE_CP);

Shouldn't this be a supply, or again in set_bias_level()?

> +	/* Initialize all clocks */
> +	snd_soc_write(codec, DA732X_REG_CLK_EN1,
> +		      DA732X_SYS3_CLK_EN | DA732X_PC_CLK_EN);
> +	snd_soc_write(codec, DA732X_REG_CLK_EN3,
> +		      DA732X_ADCA_BB_CLK_EN | DA732X_ADCC_BB_CLK_EN);
> +	snd_soc_write(codec, DA732X_REG_CLK_EN4,
> +		      DA732X_DACA_BB_CLK_EN | DA732X_DACC_BB_CLK_EN);
> +	snd_soc_write(codec, DA732X_REG_CLK_EN5, DA732X_DACE_BB_CLK_EN);

Should be dynamically managed.

> +	da732x->regmap = regmap_init_i2c(i2c, &da732x_regmap);

devm_regmap_init_i2c().

> +static const struct i2c_device_id da732x_i2c_id[] = {
> +	{ "da732x-i2c", 0},

Remove the i2c, devices connected by I2C are generally i2c devices...

> +		.name	= "da732x-i2c",

Similarly here.

> +static int __init da732x_init(void)
> +{
> +	int ret = 0;
> +
> +	ret = i2c_add_driver(&da732x_i2c_driver);

module_i2c_driver().

> +/* DAPM helper macros */
> +#define	DA732X_ADC_REG_SHIFT		4
> +#define	DA732X_ADC_ACT_MASK(v)		(1 << ((v) - 2))
> +#define	DA732X_ADC_ACT_EN(v)		(1 << ((v) - 2))
> +#define	DA732X_ADC_ACT_DIS(v)		(0 << ((v) - 2))
> +#define	DA732X_ADC_EN_SHIFT(v)		(((v) - 2) ? 7 : 3)
> +#define	DA732X_ADC_MASK(v)		(1 << DA732X_ADC_EN_SHIFT(v))
> +#define	DA732X_ADC_EN(v)		(1 << DA732X_ADC_EN_SHIFT(v))
> +#define	DA732X_ADC_DIS(v)		(0 << DA732X_ADC_EN_SHIFT(v))
> +#define	DA732X_DAC_EN_MUTED_MASK(v)	((1 << (v)) | (1 << ((v) - 1)))
> +#define	DA732X_DAC_EN_MUTED(v)		DA732X_DAC_EN_MUTED_MASK(v)
> +#define DA732X_DAC_MUTE_MASK(v)		(1 << ((v) - 1))
> +#define DA732X_DAC_EN_MASK(v)		(1 << (v))
> +#define	DA732X_DAC_DIS(v)		(0 << (v))
> +#define	DA732X_DAC_UNMUTE(v)		(0 << ((v) - 1))
> +#define	DA732X_DAC_MUTE(v)		(1 << ((v) - 1))

Really not sure how helpful I found any of these reading the code.

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

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