On Tue, 17 Apr 2012, Mark Brown wrote:
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.
Yeah, that makes sense :)
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.
Got it. Thanks
All this beep stuff is really supposed to go through the input subsystem, or at least have the hookup in there to trigger it.
Thanks for the input... I'd add that functionality
- 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.
Thanks, I'll add that
snd_soc_update_bits(codec, CS42L52_PWRCTL1,
CS42L52_PWRCTL1_PDN_CODEC |
CS42L52_PWRCTL1_PDN_CHRG, 0);
This looks odd, especially having it in _ON - perhaps a comment explaining why?
I'm just enabling the power for the codec. Are you refering to the Charge Pump enable?
Should be setting cache_only when entering _OFF I think?
Yep. thanks
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.
I missed this. Yes, that is much cleaner, thanks
devm_regmap_init_i2c(), saves a small amount of error handling/cleanup code.
Will change thanks
module_i2c_driver().
I should have done this. Thanks