[alsa-devel] [PATCH] ASoC: add RT5670 CODEC driver

[Mark Brown]

On Fri, Jun 06, 2014 at 12:15:38PM +0800, bardliao at realtek.com wrote:

> This patch adds a minimum support of Realtek ALC5670 codec.

This looks good overall, there are a few issues I've identified below
but they're relatively small.

> +	/* IN1/IN2 Control */
> +	SOC_SINGLE_TLV("IN1 Boost", RT5670_CJ_CTRL1,
> +		RT5670_BST_SFT1, 8, 0, bst_tlv),
> +	SOC_SINGLE_TLV("IN2 Boost", RT5670_IN2,
> +		RT5670_BST_SFT1, 8, 0, bst_tlv),

Boost Volume.

> +	/* ADC Boost Volume Control */
> +	SOC_DOUBLE_TLV("STO1 ADC Boost Gain", RT5670_ADC_BST_VOL1,
> +			RT5670_STO1_ADC_L_BST_SFT, RT5670_STO1_ADC_R_BST_SFT,
> +			3, 0, adc_bst_tlv),
> +	SOC_DOUBLE_TLV("STO2 ADC Boost Gain", RT5670_ADC_BST_VOL1,
> +			RT5670_STO2_ADC_L_BST_SFT, RT5670_STO2_ADC_R_BST_SFT,
> +			3, 0, adc_bst_tlv),

s/Gain/Volume/

> +static int check_dac_monor_asrc_source(struct snd_soc_dapm_widget *source,
> +			 struct snd_soc_dapm_widget *sink)
> +{
> +	struct snd_soc_codec *codec = source->codec;
> +
> +	return is_using_asrc(codec, RT5670_ASRC_2, 4);
> +}

I have to say I'm not convinced all these wrapper functions are really
adding much - just making is_using_asrc() take a widget and passing the
arguments directly seems as simple.

> +	/* ASRC */
> +	SND_SOC_DAPM_SUPPLY_S("I2S1 ASRC", 1, RT5670_ASRC_1,
> +			      11, 0, NULL, 0),
> +	SND_SOC_DAPM_SUPPLY_S("I2S2 ASRC", 1, RT5670_ASRC_1,
> +			      12, 0, NULL, 0),
> +	SND_SOC_DAPM_SUPPLY_S("DAC STO ASRC", 1, RT5670_ASRC_1,
> +			      10, 0, NULL, 0),
> +	SND_SOC_DAPM_SUPPLY_S("DAC MONO L ASRC", 1, RT5670_ASRC_1,
> +			      9, 0, NULL, 0),
> +	SND_SOC_DAPM_SUPPLY_S("DAC MONO R ASRC", 1, RT5670_ASRC_1,
> +			      8, 0, NULL, 0),
> +	SND_SOC_DAPM_SUPPLY_S("ADC STO1 ASRC", 1, RT5670_ASRC_1,
> +			      3, 0, NULL, 0),
> +	SND_SOC_DAPM_SUPPLY_S("ADC STO2 ASRC", 1, RT5670_ASRC_1,
> +			      2, 0, NULL, 0),
> +	SND_SOC_DAPM_SUPPLY_S("ADC MONO L ASRC", 1, RT5670_ASRC_1,
> +			      1, 0, NULL, 0),
> +	SND_SOC_DAPM_SUPPLY_S("ADC MONO R ASRC", 1, RT5670_ASRC_1,
> +			      0, 0, NULL, 0),

These are all _S but they seem not to have different sequence numbers -
do they really need to be _S.

> +	/* Input Side */
> +	/* micbias */
> +	SND_SOC_DAPM_MICBIAS("MICBIAS1", RT5670_PWR_ANLG2,
> +			     RT5670_PWR_MB1_BIT, 0),

Modern drivers should use supply widgets for MICBIAS.

> +static const struct snd_soc_dapm_route rt5670_dapm_routes[] = {
> +	{ "adc stereo1 filter", NULL, "ADC STO1 ASRC",
> +					check_adc_sto1_asrc_source },
> +	{ "adc stereo2 filter", NULL, "ADC STO2 ASRC",
> +					check_adc_sto2_asrc_source },
> +	{ "adc mono left filter", NULL, "ADC MONO L ASRC",
> +					check_adc_monol_asrc_source },
> +	{ "adc mono right filter", NULL, "ADC MONO R ASRC",
> +					check_adc_monor_asrc_source },
> +	{ "dac mono left filter", NULL, "DAC MONO L ASRC",
> +					check_dac_monol_asrc_source },
> +	{ "dac mono right filter", NULL, "DAC MONO R ASRC",
> +					check_dac_monor_asrc_source },
> +	{ "dac stereo1 filter", NULL, "DAC STO ASRC",
> +					check_dac_sto_asrc_source },
> +
> +	{"I2S1", NULL, "I2S1 ASRC"},

Coding style here is a bit inconsistent, and normally widget names are
capitalised.

> +	switch (dai->id) {
> +	case RT5670_AIF1:
> +		snd_soc_update_bits(codec, RT5670_I2S1_SDP,
> +			RT5670_I2S_MS_MASK | RT5670_I2S_BP_MASK |
> +			RT5670_I2S_DF_MASK, reg_val);
> +		break;
> +	case  RT5670_AIF2:

Extra space.

> +	switch (slots) {
> +	case 4:
> +		val |= (1 << 12);
> +		break;
> +	case 6:
> +		val |= (2 << 12);
> +		break;
> +	case 8:
> +		val |= (3 << 12);
> +		break;
> +	case 2:
> +	default:
> +		break;
> +	}

This seems wrong - the code will map all unknown slot counts to stereo
which probably won't do what was expected.

> +	switch (slot_width) {
> +	case 20:
> +		val |= (1 << 10);
> +		break;
> +	case 24:
> +		val |= (2 << 10);
> +		break;
> +	case 32:
> +		val |= (3 << 10);
> +		break;
> +	case 16:
> +	default:
> +		break;
> +	}

Similarly here for 16 bits.

> +static int rt5670_probe(struct snd_soc_codec *codec)
> +{
> +	struct rt5670_priv *rt5670 = snd_soc_codec_get_drvdata(codec);
> +
> +	rt5670->codec = codec;
> +
> +	snd_soc_update_bits(codec, RT5670_GEN_CTRL3, 0x4, 0x4);
> +	rt5670_set_bias_level(codec, SND_SOC_BIAS_OFF);
> +
> +	return 0;
> +}

I would expect this setup to be done in the bus level probe() rather
than at ASoC level.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20140627/e892121d/attachment.sig>