On Wed, Jun 03, 2009 at 08:34:36PM +0800, Eric Miao wrote:
This looks good overall - some comments below, mostly at the style
level.
> index 0000000..159a883
> --- /dev/null
> +++ b/sound/soc/codecs/da9034-audio.c
> @@ -0,0 +1,696 @@
> +/*
> + * linux/sound/soc/codecs/micco.c
Make your mind up :)
> +#include <asm/mach-types.h>
This shouldn't be needed and doesn't appear to be used so should be
removed.
> +/* NOTE: the ES1 revision of the chip has the following two issues
> + * regarding audio:
> + * 1. The MONO and BEAR are incorrectly exchanged
> + * 2. The Stereo DAC has to be enabled for Voice Codec
> + */
> +#define DA9034_ERRATA_ES1 1
This should be moved into platform data in order to allow it to be
selected based on board type - if this has been fixed in subsequent
silicon the workaround won't be appropriate for a lot of systems.
Probably a module option would also be good in case a board may have
either revision.
> +static const struct snd_kcontrol_new da9034_mixer_mono_out_controls[] = {
> + SOC_DAPM_SINGLE("AUX1", DA9034_MUX_MONO, 0, 1, 0),
> + SOC_DAPM_SINGLE("AUX2", DA9034_MUX_MONO, 1, 1, 0),
These should all be in the form "Foo Switch" - ths applies to most of
the controls. Ideally the relevant PGAs should be named in the form
"Foo Volume" so that ALSA can present them to the user as a single
volume/PGA control.
> +static const struct soc_enum da9034_mux_enum[] = {
> + SOC_VALUE_ENUM_SINGLE(DA9034_TX_PGA_MUX, 0, 0xff, 7,
> + mux_tx_pga_texts, mux_tx_pga_values),
> + SOC_ENUM_SINGLE(DA9034_MIC_PGA, 4, 2, mux_mic_texts),
> +};
Please move these out of arrays since...
> + /* DACs and ADCs */
> +#ifdef DA9034_ERRATA_ES1
> + SND_SOC_DAPM_DAC("DAC1", "HiFi Playback CH1", -1, 0, 0),
> + SND_SOC_DAPM_DAC("DAC2", "HiFi Playback CH2", -1, 0, 0),
This should use SND_SOC_NOPM for the invalid register.
> + SND_SOC_DAPM_PGA("LINE_OUT PGA", DA9034_AUDIO_LINE_AMP, 4, 0,
> + &pga_ctrls[7], 1),
...these numerical references into the arrays from further down the
driver get confusing. I know many of the older drivers do this but
I'm trying to avoid adding any new code using this scheme due to the
legibility and maintainability issues that result.
> +#ifdef DA9034_ERRATA_ES1
> + { "BEAR PGA", NULL, "MONO Mixer" },
> +#endif
The description of the erratum at the top said this was exchanged with
something else, not absent? I'd expect an #else if so...
> +#define DA9034_HIFI_RATES (SNDRV_PCM_RATE_8000 | SNDRV_PCM_RATE_11025 |\
> + SNDRV_PCM_RATE_16000 | SNDRV_PCM_RATE_22050 |\
> + SNDRV_PCM_RATE_32000 | SNDRV_PCM_RATE_44100 |\
> + SNDRV_PCM_RATE_48000)
This could be SNDRV_PCM_RATE_8000_48000 unless I misread.
> +struct snd_soc_dai da9034_dais[] = {
> + [0] = {
> + .name = "I2S HiFi",
> + .playback = {
> + .stream_name = "HiFi Playback",
> + .channels_min = 2,
> + .channels_max = 2,
> + .rates = DA9034_HIFI_RATES,
> + .formats = DA9034_PCM_FORMATS,
> + },
> + .ops = &da9034_dai_ops_hifi,
> + },
Your hw_params function above sets the rate unconditionally without
seperate configuration for playback and capture. This suggests that you
should be defining symmetric_rates here in order to stop applications
trying to reconfigure the rates with a record while a playback is active
(or vice versa).
> + [1] = {
> + .name = "PCM Voice",
> + .playback = {
> + .stream_name = "Voice Playback",
> + .channels_min = 1,
> + .channels_max = 1,
> + .rates = DA9034_VOICE_RATES,
> + .formats = DA9034_PCM_FORMATS,
> + },
> + .capture = {
> + .stream_name = "Voice Capture",
> + .channels_min = 1,
> + .channels_max = 1,
> + .rates = DA9034_VOICE_RATES,
> + .formats = DA9034_PCM_FORMATS,
> + },
> + .ops = &da9034_dai_ops_voice,
> + },
Same comment looks like it applies to the voice DAI.
> +#ifdef CONFIG_DEBUG_FS
Please remove all this code - ASoC already has register dump support via
both debugfs and sysfs. It currently uses register numbers rather than
names but I wouldn't be averse to patches adding name support.
> +static struct snd_soc_codec *da9034_codec;
> +
> +static int da9034_soc_probe(struct platform_device *pdev)
> +{
> + struct snd_soc_device *socdev = platform_get_drvdata(pdev);
> + int ret = 0;
> +
> + BUG_ON(!da9034_codec);
Seems harsh but OK.
> + dev_info(&pdev->dev, "da9034 (aka micco) audio codec registered\n");
dev_dbg() if you're going to have this. Might be better to use
codec->dev rather than the platform device since other things will use
that and it's a bit more friendly.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
