On Oct 2, 2011, at 1:40 PM, Mark Brown wrote:
On Fri, Sep 30, 2011 at 11:41:52AM -0500, Brian Austin wrote:
This patch adds support for the Cirrus Logic CS42L73 low power stereo codec.
A few comments here beyond those others made (though I may be replicating some of what they said) but overall this is shaping up well - hopefully next version we can get this merged.
Folks: when replying to patches (or mails in general) you should delete unneeded context. This makes it much easier to find the text you wrote, all the stuff about cutting text improving bandwidth usage applies just as much to the humans reading your mail as to the computers moving it.
This patch has cleared checkpatch.pl with no warnings or errors. Code changes requested were implemented. ASoC API changes requested were implemented.
Stuff like this should go after the ---.
- cs42l73.c -- CS42L73 ALSA Soc Audio driver
- Copyright 2011 Cirrus Logic, Inc.
- Authors: Georgi Vlaev, Nucleus Systems Ltd, office@nucleusys.com
Who's not CCed on the patch, and who hasn't signed it off?
Wrong email in the file.
- SOC_DOUBLE("Input Path Digital Switch", CS42L73_ADCIPC, 0, 4, 1, 1),
s/Input Path/Capture/
got it
- SOC_DOUBLE("Invert ADC Signal Polarity Switch", CS42L73_ADCIPC, 1, 5, 1,
0),ADC Signal Polarity Switch.
- SOC_DOUBLE("ADC Boost Switch", CS42L73_ADCIPC, 2, 6, 1, 0),
This should be a Volume control with TLV information assuming it does what it sounds like and makes the signal louder.
This is just 0dB or 25dB so now it's just an enum if that's OK.
- SOC_SINGLE("Charge Pump Frequency Volume", CS42L73_CPFCHC, 4, 15, 0),
No Volume (clearly this isn't a volume control), this should probably be an enum listing the rates.
Now enumerated
- SOC_SINGLE("HL Limiter Attack Rate Volume", CS42L73_LIMARATEHL, 0, 0x3F,
0),- SOC_SINGLE("HL Limiter Release Rate Volume", CS42L73_LIMRRATEHL, 0,
0x3F, 0),Similarly here, you probably want an enumaration showing the rates.
This is a range of values that are used to set the rate by setting the step period size. It's values are 0 - 3F. I'm hoping to leave it as an SOC_SINGLE control if possible
- SOC_DOUBLE_R_TLV("XSP-IP Attenuation Volume",
CS42L73_XSPAIPAA, CS42L73_XSPBIPBA, 0, 0x3F, 1,attn_tlv),Attenuation Volume makes no sense - they're approximate synonyms. Just Volume.
- if (clk_id != CS42L73_CLKID_MCLK1 && clk_id != CS42L73_CLKID_MCLK2) {
dev_err(codec->dev, "Invalid clk_id %u\n", clk_id);return -EINVAL;- }
A switch statement would be more idomatic.
- if (spc & xSPDIF_PCM) {
spc &= (31 << 3); /* Clear PCM mode, set MSB->LSB */if (format == SND_SOC_DAIFMT_DSP_B&& inv == SND_SOC_DAIFMT_IB_IF)spc |= (xPCM_MODE0 << 4);elseif (format == SND_SOC_DAIFMT_DSP_B&& inv == SND_SOC_DAIFMT_IB_NF)spc |= (xPCM_MODE1 << 4);elseif (format == SND_SOC_DAIFMT_DSP_A&& inv == SND_SOC_DAIFMT_IB_IF)spc |= (xPCM_MODE1 << 4);elsereturn -EINVAL;I can't help but feel this would be clearer with a switch statement.
I'm trying to make this look better...
+static int cs42l73_set_tristate(struct snd_soc_dai *dai, int tristate) +{
- struct snd_soc_codec *codec = dai->codec;
- int id = dai->id;
- return snd_soc_update_bits(codec, CS42L73_SPC(id), 0x7F, tristate << 7);
This is going to fail if someone passes a non-boolean value in - better make sure that tristate is actually 1 or 0.
.ops = &cs42l73_ops,.symmetric_rates = 1,},- {
Please use the kernel coding style for indentation.
+/* CS42L73_PWRCTL1 */ +#define PDN_ADCB (1 << 7) +#define PDN_DMICB (1 << 6) +#define PDN_ADCA (1 << 5) +#define PDN_DMICA (1 << 4) +#define PDN_LDO (1 << 2) +#define DISCHG_FILT (1 << 1) +#define PDN (1 << 0)
All the identifiers in the header need to be namespaced.