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().