[alsa-devel] [PATCH v2] ASoC: Add support for cs42l73 codec

[Lars-Peter Clausen]

On 09/30/2011 06:41 PM, Brian Austin wrote:
> This patch adds support for the Cirrus Logic CS42L73
> low power stereo codec.
> 
> This patch has cleared checkpatch.pl with no warnings or errors.
> Code changes requested were implemented.
> ASoC API changes requested were implemented.
> 
> Signed-off-by: Brian Austin <brian.austin at cirrus.com>
> ---
> [...]
> +struct  cs42l73_private {

extra space

> [...]
> +
> +static const struct snd_soc_dapm_route cs42l73_audio_map[] = {
> +	{"HPOUTA", NULL, "HP Amp Left"},
> +	{"HPOUTB", NULL, "HP Amp Right"},
> +	{"LINEOUTA", NULL, "LO Amp Left"},
> +	{"LINEOUTB", NULL, "LO Amp Right"},
> +	{"SPKOUT", NULL, "SPK Amp"},
> +	{"EAROUT", NULL, "EAR Amp"},
> +	{"SPKLINEOUT", NULL, "SPKLO Amp"},
> +
> +	{"HP Amp Left", "DAC", "DAC Left"},
> +	{"HP Amp Right", "DAC", "DAC Right"},
> +	{"LO Amp Left", "DAC", "DAC Left"},
> +	{"LO Amp Right", "DAC", "DAC Right"},
> +	{"SPK Amp", "DAC", "DAC Left"},
> +	{"SPKLO Amp", "DAC", "DAC Right"},
> +	{"EAR Amp", "DAC", "DAC Right"},

I suppose this won't work as expected. The control element of the path will
only have an effect if the sink is a mixer or a mux.

> +
> +	{"PGA Mux Left", NULL, "LINEINA"},
> +	{"PGA Mux Right", NULL, "LINEINB"},
> +	{"PGA Mux Left", NULL, "MIC1"},
> +	{"PGA Mux Right", NULL, "MIC2"},

You probably want connect the inputs of the mux depending on its setting.

e.g. {"PGA Mux Left", "Line A", "LINEINA"},

[...]
> +
> +struct cs42l73_mclk_div cs42l73_mclk_coeffs[] = {

static

> [...]
> +
> +struct cs42l73_mclkx_div cs42l73_mclkx_coeffs[] = {

static

> [...]
> +int cs42l73_get_mclkx_coeff(int mclkx)

static

> [...]
> +int cs42l73_get_mclk_coeff(int mclk, int srate)

static


> +
> +static int cs42l73_set_mclk(struct snd_soc_dai *dai)
> +{
> +	struct snd_soc_codec *codec = dai->codec;
> +	struct cs42l73_private *priv = snd_soc_codec_get_drvdata(codec);
> +
> +	int mclkx_coeff;
> +	u32 mclk = 0;
> +	u8 dmmcc = 0;
> +
> +	/* MCLKX -> MCLK */
> +	mclkx_coeff = cs42l73_get_mclkx_coeff(priv->sysclk);
> +

here you use priv->sysclk ...

> +
> +static int cs42l73_set_sysclk(struct snd_soc_dai *dai,
> +			      int clk_id, unsigned int freq, int dir)
> +{
> +	struct snd_soc_codec *codec = dai->codec;
> +	struct cs42l73_private *priv = snd_soc_codec_get_drvdata(codec);
> +
> +	if (clk_id != CS42L73_CLKID_MCLK1 && clk_id != CS42L73_CLKID_MCLK2) {
> +		dev_err(codec->dev, "Invalid clk_id %u\n", clk_id);
> +		return -EINVAL;
> +	}
> +
> +	if ((cs42l73_get_mclkx_coeff(freq) < 0)) {
> +		dev_err(codec->dev, "Invalid sysclk %u\n", freq);
> +		return -EINVAL;
> +	}
> +
> +	if ((cs42l73_set_mclk(dai)) < 0) {
> +		dev_err(codec->dev, "Unable to set MCLK for dai %s\n",
> +			dai->name);
> +		return -EINVAL;
> +	}
> +
> +	priv->sysclk = freq;
> +	priv->mclksel = clk_id;
> +

... but here you only set it after having called cs42l73_set_mclk. Is this
correct? Maybe pass it in as a parameter to cs42l73_set_mclk

> +	return 0;
> +}
> +
> +static int cs42l73_set_dai_fmt(struct snd_soc_dai *codec_dai, unsigned int fmt)
> +{
> +	struct snd_soc_codec *codec = codec_dai->codec;
> +	struct cs42l73_private *priv = snd_soc_codec_get_drvdata(codec);
> +	int id = codec_dai->id;
> +	int inv, format;

unsigned int

> +	u8 spc, mmcc;
> +
>[...]
> +

> +static int cs42l73_set_bias_level(struct snd_soc_codec *codec,
> +				  enum snd_soc_bias_level level)
> +{
> +	int ret;
> +
> +	switch (level) {
> +	case SND_SOC_BIAS_ON:
> +		snd_soc_update_bits(codec, CS42L73_DMMCC, MCLKDIS, 0);
> +		snd_soc_update_bits(codec, CS42L73_PWRCTL1, PDN, 0);
> +		break;
> +
> +	case SND_SOC_BIAS_PREPARE:
> +		break;
> +
> +	case SND_SOC_BIAS_STANDBY:
> +		if (codec->dapm.bias_level == SND_SOC_BIAS_OFF) {
> +				ret = snd_soc_cache_sync(codec);

indention

> [...]
> +
> +static struct snd_soc_dai_ops cs42l73_ops = {

const

> +	.startup = cs42l73_pcm_startup,
> +	.hw_params = cs42l73_pcm_hw_params,
> +	.set_fmt = cs42l73_set_dai_fmt,
> +	.set_sysclk = cs42l73_set_sysclk,
> +	.set_tristate = cs42l73_set_tristate,
> +};
> +
> +struct snd_soc_dai_driver cs42l73_dai[] = {

static

> +	{
> +	 .name = "cs42l73-xsp",
> +	 .id = CS42L73_XSP,
> +	 .playback = {
> +		      .stream_name = "XSP Playback",
> +		      .channels_min = 1,
> +		      .channels_max = 2,
> +		      .rates = CS42L73_RATES,
> +		      .formats = CS42L73_FORMATS,},
> +
> +	 .capture = {
> +		     .stream_name = "XSP Capture",
> +		     .channels_min = 1,
> +		     .channels_max = 2,
> +		     .rates = CS42L73_RATES,
> +		     .formats = CS42L73_FORMATS,},
> +
> +	 .ops = &cs42l73_ops,
> +	 .symmetric_rates = 1,
> +	 },

Indention looks a bit odd here

> [...]
> +static int cs42l73_probe(struct snd_soc_codec *codec)
> +{
> +	int ret;
> +	unsigned int devid = 0;
> +	struct cs42l73_private *cs42l73 = snd_soc_codec_get_drvdata(codec);
> +
> +	codec->control_data = cs42l73->control_data;
> +	codec->hw_write = (hw_write_t)i2c_master_send;

You don't need to initialize control_data and hw_write.
snd_soc_codec_set_cache_io will do this for you. This also means that you can
remove the control_data field from your driver private data struct.

> +
> +	ret = snd_soc_codec_set_cache_io(codec, 8, 8, cs42l73->control_type);

If you CODEC only has a I2C interface you can use SND_SOC_I2C here directly
instead of cs42l73->control_type

> [...]
> +
> +	snd_soc_add_controls(codec, cs42l73_snd_controls,
> +			     ARRAY_SIZE(cs42l73_snd_controls));

Just put the controls in your codec driver struct instead of manually adding them.

> +
> +	return ret;
> +}
> +
> +static int cs42l73_remove(struct snd_soc_codec *codec)
> +{
> +	cs42l73_set_bias_level(codec, SND_SOC_BIAS_OFF);
> +	return 0;
> +}
> +
> +struct snd_soc_codec_driver soc_codec_dev_cs42l73 = {

static

> +	.probe = cs42l73_probe,
> +	.remove = cs42l73_remove,
> +	.suspend = cs42l73_suspend,
> +	.resume = cs42l73_resume,
> +	.set_bias_level = cs42l73_set_bias_level,
> +	.reg_cache_size = ARRAY_SIZE(cs42l73_reg),
> +	.reg_cache_default = cs42l73_reg,
> +	.reg_word_size = sizeof(u8),
> +	.dapm_widgets = cs42l73_dapm_widgets,
> +	.num_dapm_widgets = ARRAY_SIZE(cs42l73_dapm_widgets),
> +	.dapm_routes = cs42l73_audio_map,
> +	.num_dapm_routes = ARRAY_SIZE(cs42l73_audio_map),
> +};
> +