Re: [PATCH] ASoC TLV320AIC3X codec driver

["Vladimir A. Barinov" <vbarinov@ru.mvista.com>]

Hello Takashi,

Thank you for the review.

Takashi Iwai wrote:
> At Tue, 13 Nov 2007 22:41:25 +0300,
> Vladimir Barinov wrote:
>   
>> --- /dev/null
>> +++ linux-2.6.24.rc2.alsa/sound/soc/codecs/tlv320aic3x.c
>>     
> (snip)
>   
>> +
>> +struct snd_soc_codec_device soc_codec_dev_aic3x;
>>     
>
> Do you need this here?  It's defined in the later position.
>   
You are right. Thanks for pointing to this. That is an artefact of the 
driver that I based on.
>   
>> +/*
>> + * All input lines are connected when !0xf and disconnected with 0xf bit field,
>>     
>
> Please keep the line within 80 chars.  Try to run checkpatch.pl in
> linuxkernel/scripts for checking such minor coding-style issues.
>   
But this line is less then 80 chars :)

I've applied checkpatch.pl before sending the patch. The driver really 
has 3 lines with 80 chars warning,
but they are less then 82 char. It's in the codec interconnection map 
for DAPM. If that is a strong demand with
80 chars _warning_ then I'll try to rename the mixers/path/endpoint 
names to match, if not then the code will be
more understandable.
That lines are:
+        {"Right HPCOM Mux", "differential of HPROUT", "Right Line2 
Bypass Mixer"},
+        {"Right HPCOM Mux", "differential of HPLCOM", "Right Line2 
Bypass Mixer"},

If I'll split it with 2 lines each then will it be good to watch in 
accordance with DAPM usage strategy?
Destination Widget  <=== Path Name <=== Source Widget
>   
>> +static const char *aic3x_left_dac_mux[] = { "DAC_L1", "DAC_L3", "DAC_L2" };
>> +static const char *aic3x_right_dac_mux[] = { "DAC_R1", "DAC_R3", "DAC_R2" };
>> +static const char *aic3x_left_hpcom_mux[] =
>> +    { "differential of HPLOUT", "constant VCM", "single-ended" };
>> +static const char *aic3x_right_hpcom_mux[] =
>> +    { "differential of HPROUT", "constant VCM", "single-ended",
>> +      "differential of HPLCOM", "external feedback" };
>> +static const char *aic3x_linein_mode_mux[] = { "single-ended", "differential" };
>>     
>
> Ditto.
>   
Ditto :)
These lines are less then 80 chars.
>   
>> +#define AIC3X_RATES	SNDRV_PCM_RATE_8000_96000
>> +#define AIC3X_FORMATS	(SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S20_3LE | \
>> +			 SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S32_LE)
>>     
>
> Are you sure that it's FMTBIT_S24_LE?  It's not packed in 3 bytes but
> uses lower 3 bytes of 4 bytes frame.
>
>   
Aic3x supports 16/20/24/32 bits data word length and in accordance with 
aic33 documentation the
number of clocks per half-frame for each channel are equal to exact 
number of bits of the word.
Unfortunately, now I have no h/w to test 24bits mode.
>   
>> Index: linux-2.6.24.rc2.alsa/sound/soc/codecs/Kconfig
>> ===================================================================
>> --- linux-2.6.24.rc2.alsa.orig/sound/soc/codecs/Kconfig
>> +++ linux-2.6.24.rc2.alsa/sound/soc/codecs/Kconfig
>> @@ -37,3 +37,6 @@ config SND_SOC_CS4270_VD33_ERRATA
>>  	bool
>>  	depends on SND_SOC_CS4270
>>  
>> +config SND_SOC_TLV320AIC3X
>> +	tristate
>> +	depends on SND_SOC
>>     
>
> Currently, there is a known problem with dependency on I2C over soc
> codecs drivers.  If CONFIG_I2C=m and CONFIG_SND_SOC_*=y, you'll get
> unresolved symbols.
>
> I don't know how to resolve this issue right now.  But, the drivers
> that support *only* I2C right now can add "depends on I2C" to its
> Kconfig.  That doesn't work for CS4270 which may work without I2C as
> standalone mode, though.
>   
Ok, will add I2C dependency.

Regards,
Vladimir