On Tue, Sep 30, 2008 at 3:49 PM, Mark Brown <broonie@xxxxxxxxxxxxx> wrote:
> On Tue, Sep 30, 2008 at 03:29:32PM +0530, Arun KS wrote:
>
> Thanks! It looks like you've addressed pretty much all the comments
> from the first round of reviews but a few additional (fairly minor
> comments):
>
>> +static const char *tlv320aic23_rec_src[] = { "Line", "Mic" };
>> +static const char *tlv320aic23_sidetone_src[] =
>> + { "-6db", "-9db", "-12db", "-18db", "0db" };
>> +
>> +static const struct soc_enum tlv320aic23_enum[] = {
>> + SOC_ENUM_SINGLE(TLV320AIC23_ANALOG_AUDIO_CONTROL, 2, 2,
>> + tlv320aic23_rec_src),
>> + SOC_ENUM_SINGLE(TLV320AIC23_ANALOG_AUDIO_CONTROL, 6, 5,
>> + tlv320aic23_sidetone_src),
>> +};
>> +
>
> sidetone_src really ought to be a SOC_SINGLE_TLV rather than an enum
> from the looks of it - it's not selecting between sources for the
> sidetone, it's determining the level of the signal so a gain control
> with TLV information would present better in UIs.
Thanks for the comments. Will do that.
>
>> +static const struct snd_kcontrol_new tlv320aic23_rec_src_mux_controls =
>> +SOC_DAPM_ENUM("Route", tlv320aic23_enum[0]);
>
>> +static const struct snd_kcontrol_new tlv320aic23_sidetone_src_controls =
>> +SOC_DAPM_ENUM("Route", tlv320aic23_enum[1]);
>
> I know some of the older codec drivers do it but there's no need to have
> the enums be an array and it doesn't help clarity. Equally well, it's
> not a blocker for merge.
>
>> +static const struct snd_kcontrol_new tlv320aic23_snd_controls[] = {
>> + SOC_DOUBLE_R("PCM Playback Volume", TLV320AIC23_LEFT_CHANNEL_VOLUME,
>> + TLV320AIC23_RIGHT_CHANNEL_VOLUME, 0, 0x7f, 0),
>
> TLV information might be nice here too (but it's not required). I
> suspect "Digital Playback Volume" would be a better name, but I'm not
> 100% sure on that one.
>
>> + SOC_DOUBLE_R("Line Input Mute", TLV320AIC23_LEFT_LINE_VOLUME,
>> + TLV320AIC23_RIGHT_LINE_VOLUME, 7, 0x01, 0),
>
> This should be "Line Input Switch" for user interfaces to pick it up
> prop
>
>> +static const struct snd_soc_dapm_route intercon[] = {
>> +
>> + {"LHPOUT", NULL, "DAC"},
>> + {"RHPOUT", NULL, "DAC"},
>> +
>> + {"LOUT", NULL, "DAC"},
>> + {"ROUT", NULL, "DAC"},
>> +
>> + {"Capture Source", "Line", "LLINEIN"},
>> + {"Capture Source", "Line", "RLINEIN"},
>> +
>> + {"Capture Source", "Mic", "MICIN"},
>> +
>> + {"PGA Mixer", "Line Switch", "Capture Source"},
>> + {"PGA Mixer", "Mic Switch", "Capture Source"},
>> +
>> + {"ADC", NULL, "PGA Mixer"},
>> +};
>
> There are no routes here for your bypass path(s) - this will mean that
> DAPM won't power them on unless there's an active playback and record.
> At present that's fine for digital only bypass paths but if there are
> analogue ones they should be visible here.
I made the bypass and sidetone as controls. The user can switchon them
through amixer controls.
sh-3.00# amixer controls
numid=2,iface=MIXER,name='PCM Playback Switch'
numid=1,iface=MIXER,name='PCM Playback Volume'
numid=3,iface=MIXER,name='Line Input Mute'
numid=4,iface=MIXER,name='Line Input Volume'
numid=9,iface=MIXER,name='Line Bypass'
numid=6,iface=MIXER,name='Mic Booster Switch'
numid=5,iface=MIXER,name='Mic Switch'
numid=10,iface=MIXER,name='Capture Source'
numid=11,iface=MIXER,name='PGA Mixer Line Switch'
numid=12,iface=MIXER,name='PGA Mixer Mic Switch'
numid=8,iface=MIXER,name='Sidetone Gain'
numid=7,iface=MIXER,name='Sidetone Switch'
sh-3.00# amixer cset numid=9 1
numid=9,iface=MIXER,name='Line Bypass'
; type=BOOLEAN,access=rw------,values=1
: values=on
sh-3.00# amixer cset numid=11 1
numid=11,iface=MIXER,name='PGA Mixer Line Switch'
; type=BOOLEAN,access=rw------,values=1
: values=on
Its working.
>
>> +static int tlv320aic23_mute(struct snd_soc_dai *dai, int mute)
>> +{
>> + struct snd_soc_codec *codec = dai->codec;
>> + u16 reg;
>> +
>> + reg =
>> + tlv320aic23_read_reg_cache(codec,
>> + TLV320AIC23_DIGITAL_AUDIO_CONTROL) &
>> + ~DACM_MUTE;
>
> Have you resolved the word wrapping issues with your mailer? I've not
> tried applying the patch but things like this make me think that it has
> been mangled.
>
>> +static int tlv320aic23_set_dai_sysclk(struct snd_soc_dai *codec_dai,
>> + int clk_id, unsigned int freq, int dir)
>> +{
>> + struct snd_soc_codec *codec = codec_dai->codec;
>> + struct tlv320aic23_priv *tlv320aic23 = codec->private_data;
>> +
>> + tlv320aic23->sysclk = freq;
>> + return 0;
>> +}
>
> You don't actually use sysclk for anything so you may as well drop this
> function and the local variable.
I'll do it.
>
>> + tlv320aic23->master = 1;
>> + iface_reg |= MS_MASTER;
>> + break;
>> + case SND_SOC_DAIFMT_CBS_CFS:
>> + tlv320aic23->master = 0;
>> + break;
>
> You don't seem to use master anywhere either so it could also be
> dropped?
Ok i'll drop.
>
>> +
>> +/* Left (right) line input volume control register */
>> +#define LRS_ENABLED 0x0100
>> +#define LIM_MUTED 0x0080
>> +#define LIV_DEFAULT 0x0017
>> +#define LIV_MAX 0x001f
>> +#define LIV_MIN 0x0000
>
> All the definitions in the header should be namespaced - there's things
> like:
>
>> +/* Power control down register */
>> +#define DEVICE_POWER_OFF 0x0080
>> +#define CLK_OFF 0x0040
>> +#define OSC_OFF 0x0020
>> +#define OUT_OFF 0x0010
>> +#define DAC_OFF 0x0008
>> +#define ADC_OFF 0x0004
>> +#define MIC_OFF 0x0002
>> +#define LINE_OFF 0x0001
>
> which are very likely to collide with other chips.
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@xxxxxxxxxxxxxxxx
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
