[alsa-devel] [PATCH v2] ASoC: Add support for cs42l73 codec

[Austin, Brian]

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 at 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);
>> +		else
>> +
>> +		    if (format == SND_SOC_DAIFMT_DSP_B
>> +				&& inv == SND_SOC_DAIFMT_IB_NF)
>> +			spc |= (xPCM_MODE1 << 4);
>> +		else
>> +
>> +		    if (format == SND_SOC_DAIFMT_DSP_A
>> +				&& inv == SND_SOC_DAIFMT_IB_IF)
>> +			spc |= (xPCM_MODE1 << 4);
>> +		else
>> +			return -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.
> 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 3917 bytes
Desc: not available
Url : http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20111004/d1d0f124/attachment.p7s