[alsa-devel] [PATCH] ASoC: CS4271 codec support

Dear Dimitris,

On Mon, 2011-01-17 at 10:30 +0000, Dimitris Papastamos wrote:

On Mon, 2011-01-17 at 01:35 +0300, Alexander wrote:

quoted

+/* CS4271 default register settings, except auto-mute is off */
+static const u8 cs4271_dflt_reg[CS4271_NR_REGS] = {
+	0, 0, 0, CS4271_DACVOL_ATAPI_AL_BR, 0, 0, 0,
+	CS4271_MODE2_CPEN | CS4271_MODE2_PDN,
+};

I'd be better to leave these as defaults, and perform any initialization
in the the cs4271_probe() function.

It's space optimized without affecting readability. I can rename it to
cs4271_init_reg[].

quoted

+	/* Configure DAC */
+	val = cs4271_clk_tab[i].speed_mode;
+
+	if (cs4271->master)
+		val |= cs4271_clk_tab[i].mclk_master | CS4271_MODE1_MASTER;
+	else
+		val |= cs4271_clk_tab[i].mclk_slave;

This should ideally live in cs4271_set_dai_fmt().

It's space optimized for particular codec hardware without affecting
readability.

quoted

+	switch (cs4271->mode) {
+	case SND_SOC_DAIFMT_LEFT_J:
+		val |= CS4271_MODE1_DAC_DIF_LJ;
+		break;
+	case SND_SOC_DAIFMT_I2S:
+		val |= CS4271_MODE1_DAC_DIF_I2S;
+		break;
+	default:
+		dev_err(codec->dev, "Invalid DAI format\n");
+		return -EINVAL;
+	}

Same here.

Same here.

quoted

+/* CS4271 controls */
+static DECLARE_TLV_DB_SCALE(cs4271_dac_tlv, -12700, 100, 0);

Are you use this doesn't mute the DAC?  If so the the last parameter
should be 1.

No I do not use this, some people asked for this last time I've tried to
submit this code. And no, this doesn't mute the DAC.

quoted

+#ifdef CONFIG_PM
+static int cs4271_soc_suspend(struct snd_soc_codec *codec, pm_message_t mesg)
+{
+	/* Set power-down bit */
+	snd_soc_update_bits(codec, CS4271_MODE2, CS4271_MODE2_PDN,
+		CS4271_MODE2_PDN);
+
+	return 0;
+}
+#else
+#define cs4271_soc_suspend	NULL
+#endif /* CONFIG_PM */
+
+/* This function used also in codec probe function and not only for PM */
+static int cs4271_soc_resume(struct snd_soc_codec *codec)
+{
+	int ret;
+
+	/* Restore codec state */
+	ret = cs4271_write_cache(codec);
+	if (ret < 0)
+		return ret;
+
+	/* then disable the power-down bit */
+	snd_soc_update_bits(codec, CS4271_MODE2, CS4271_MODE2_PDN, 0);
+
+	return 0;
+}

Consider setting the set_bias_level callback and performing the required
work in there.  You can then trigger a change in the bias levels by
calling your cs4271_set_bias_level() function from within your suspend()
and resume() functions.

quoted

+	/*
+	 * In case of I2C, chip address specified in board data.
+	 * So cache IO operations use 8 bit codec register address.
+	 * In case of SPI, chip address and register address
+	 * passed together as 16 bit value.
+	 * Anyway, register address is masked with 0xFF inside
+	 * soc-cache code.
+	 */

Have you tested your driver using both SPI and I2C?

Yes.

quoted

+	ret = (cs4271->bus_type == SND_SOC_SPI) ? 16 : 8;

Please avoid using the ternary operator like this.

What's wrong with it?

quoted

+	ret = snd_soc_codec_set_cache_io(codec, ret, 8, cs4271->bus_type);
+	if (ret) {
+		dev_err(codec->dev, "Failed to set cache I/O: %d\n", ret);
+		return ret;
+	}
+
+	ret = cs4271_soc_resume(codec);
+	if (ret < 0)
+		return ret;

This should also changed to a call to
cs4271_set_bias_level(codec, SND_SOC_BIAS_STANDBY);

There is no need for DAPM on hardware I consider. I can even remove PM
stuff. Someone may implement it later, if CS4271 would appear on
something except Cirrus dev. boards.

Thanks,
Dimitris

Thanks,
	Alexander.