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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
