[alsa-devel] [PATCH] ASoC: Add support for CS42L52 Codec

[Mark Brown]

On Mon, Apr 16, 2012 at 03:58:58PM -0500, Brian Austin wrote:
> This patch adds support for Cirrus Logic CS42L52 Low Power Stereo Codec

Overall this looks good, a few issues below...

> +static const struct reg_default cs42l52_reg_defaults[] = {
> +	{ 1, 0xE0 },	/* r01 ChipID */

You shouldn't provide a default for this, the whole point with a
register like this is that it gets read from the device so you can
confirm you're talking to the right device.  As things stand your code
in probe will just check that the cache is initialised with the expected
default.

> +	{ 49, 0x00 },	/* r31 Speaker Status */

Going on the name this (and possibly some of the other registers) ought
to be marked as volatile and not have a default.

> +static const char * const mic_bias_level_text[] = {
> +	"0.5 +VA", "0.6 +VA", "0.7 +VA",
> +	"0.8 +VA", "0.83 +VA", "0.91 +VA"
> +};

Normally this is a platform data type thing.

> +static const char * const cs42l52_mic_text[] = { "Single", "Differential" };

This too - it's usually determined by the schematic, not something that
gets varied at runtime.

> +static const char * const beep_pitch_text[] = {
> +	"C4", "C5", "D5", "E5", "F5", "G5", "A5", "B5",
> +	"C6", "D6", "E6", "F6", "G6", "A6", "B6", "C7"
> +};
> +
> +static const struct soc_enum beep_pitch_enum =
> +	SOC_ENUM_SINGLE(CS42L52_BEEP_FREQ, 4,
> +		ARRAY_SIZE(beep_pitch_text), beep_pitch_text);
> +
> +static const char * const beep_ontime_text[] = {
> +	"86 ms", "430 ms", "780 ms", "1.20 s", "1.50 s",
> +	"1.80 s", "2.20 s", "2.50 s", "2.80 s", "3.20 s",
> +	"3.50 s", "3.80 s", "4.20 s", "4.50 s", "4.80 s", "5.20 s"
> +};
> +
> +static const struct soc_enum beep_ontime_enum =
> +	SOC_ENUM_SINGLE(CS42L52_BEEP_FREQ, 0,
> +		ARRAY_SIZE(beep_ontime_text), beep_ontime_text);
> +
> +static const char * const beep_offtime_text[] = {
> +	"1.23 s", "2.58 s", "3.90 s", "5.20 s",
> +	"6.60 s", "8.05 s", "9.35 s", "10.80 s"
> +};
> +
> +static const struct soc_enum beep_offtime_enum =
> +	SOC_ENUM_SINGLE(CS42L52_BEEP_VOL, 5,
> +		ARRAY_SIZE(beep_offtime_text), beep_offtime_text);
> +
> +static const char * const beep_config_text[] = {
> +	"Off", "Single", "Multiple", "Continuous"
> +};
> +
> +static const struct soc_enum beep_config_enum =
> +	SOC_ENUM_SINGLE(CS42L52_BEEP_TONE_CTL, 6,
> +		ARRAY_SIZE(beep_config_text), beep_config_text);
> +
> +static const char * const beep_bass_text[] = {
> +	"50 Hz", "100 Hz", "200 Hz", "250 Hz"
> +};

All this beep stuff is really supposed to go through the input
subsystem, or at least have the hookup in there to trigger it.

> +static const char * const charge_pump_freq_text[] = {
> +	"0", "1", "2", "3", "4",
> +	"5", "6", "7", "8", "9",
> +	"10", "11", "12", "13", "14", "15" };
> +
> +static const struct soc_enum charge_pump_enum =
> +	SOC_ENUM_SINGLE(CS42L52_CHARGE_PUMP, 4,
> +		ARRAY_SIZE(charge_pump_freq_text), charge_pump_freq_text);

Should this be runtime configurable?

> +static const unsigned int swap_values[] = { 0, 1, 3 };

> +static const struct soc_enum adca_swap_enum =
> +	SOC_VALUE_ENUM_SINGLE(CS42L52_ADC_PCM_MIXER, 2, 1,
> +			      ARRAY_SIZE(left_swap_text),
> +			      left_swap_text,
> +			      swap_values);

This L/R swap stuff should probably go into DAPM, it's not so useful
right now but we're hopefully going to be able to get the ability to
figure out which of the channels on the DAI are active and power manage
appropriately.

> +	SND_SOC_DAPM_AIF_OUT("AIFOUTL", "Capture",  0,
> +			SND_SOC_NOPM, 0, 0),
> +	SND_SOC_DAPM_AIF_OUT("AIFOUTR", "Capture",  0,
> +			SND_SOC_NOPM, 0, 0),

You should use DAPM routes to hook the AIF widgets to the DAIs -
they're new as of 3.4 but ideally we'll be transitioning everything over
to this.  Some of the Wolfson drivers like wm8996 have been converted so
provide examples.

> +	switch (level) {
> +	case SND_SOC_BIAS_ON:
> +		snd_soc_update_bits(codec, CS42L52_PWRCTL1,
> +				    CS42L52_PWRCTL1_PDN_CODEC |
> +				    CS42L52_PWRCTL1_PDN_CHRG, 0);
> +		break;

This looks odd, especially having it in _ON - perhaps a comment
explaining why?

> +	case SND_SOC_BIAS_STANDBY:
> +		if (codec->dapm.bias_level == SND_SOC_BIAS_OFF) {
> +			regcache_cache_only(cs42l52->regmap, false);
> +			regcache_sync(cs42l52->regmap);
> +		}
> +		snd_soc_write(codec, CS42L52_PWRCTL1, CS42L52_PWRCTL1_PDN_ALL);
> +		break;
> +	case SND_SOC_BIAS_OFF:
> +		snd_soc_write(codec, CS42L52_PWRCTL1, CS42L52_PWRCTL1_PDN_ALL);
> +		break;

Should be setting cache_only when entering _OFF I think?

> +/* page 37 from cs42l52 datasheet */
> +static void cs42l52_required_setup(struct snd_soc_codec *codec)
> +{
> +	cs42l52_set_bias_level(codec, SND_SOC_BIAS_ON);
> +
> +	snd_soc_write(codec, CS42L52_FIX_BITS_CTL, 0x99);
> +	snd_soc_write(codec, CS42L52_FIX_BITS1, 0xBA);
> +	snd_soc_write(codec, CS42L52_FIX_BITS2, 0x80);
> +
> +	snd_soc_update_bits(codec, CS42L52_TEM_CTL, CS42L52_TEM_CTL_SET, 1);
> +	snd_soc_update_bits(codec, CS42L52_TEM_CTL, CS42L52_TEM_CTL_SET, 0);
> +
> +	snd_soc_write(codec, CS42L52_FIX_BITS_CTL, 0x00);
> +
> +	cs42l52_set_bias_level(codec, SND_SOC_BIAS_OFF);
> +}

You should use a regmap patch for this.  It's intended for this sort of
magic register write sequence on startup and is also integrated with the
cache sync code so the fixes will be automatically applies whenver you
sync.  With the code as it stands if the CODEC is powered off over
suspend then after resume these updates won't be applied.

> +	cs42l52->regmap = regmap_init_i2c(i2c_client, &cs42l52_regmap);
> +	if (IS_ERR(cs42l52->regmap)) {
> +		ret = PTR_ERR(cs42l52->regmap);
> +		dev_err(&i2c_client->dev, "regmap_init() failed: %d\n", ret);
> +		goto err;
> +	}

devm_regmap_init_i2c(), saves a small amount of error handling/cleanup
code.

> +
> +static int __init cs42l52_modinit(void)
> +{
> +	int ret;
> +	ret = i2c_add_driver(&cs42l52_i2c_driver);

module_i2c_driver().
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
Url : http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20120417/d4af20af/attachment.sig