[alsa-devel] [PATCH] ASoC: Add support for TI LM49453 Audio codec
Mark Brown
broonie at opensource.wolfsonmicro.com
Tue Jan 31 21:12:07 CET 2012
On Tue, Jan 31, 2012 at 03:59:57AM -0800, Reddy, MR Swami wrote:
> ASoC: Add support for TI LM49453 Audio codec
This is a really good improvement on the previous version but there's
still several issues and I'm still having a hard time understanding how
you didn't see some of them in testing.
> +static struct reg_default lm49453_reg_defs[] = {
> + { 0, 0x00}, { 1, 0x00}, { 2, 0x00}, { 3, 0x00}, /*R3*/
The indentation here is *really* strange - you've no space before the }.
The comments are also pretty redundant given that the reg_default
already contains the register address.
Looking at the register defines in the header it seems like a lot of
these should be omitted - you've got registers up to 0x5 then you jump
up to page 1 starting at 0x80 so nothing between those two needs to be
in the defaults table. The reason we have the register address in there
is to support devices with sparse register maps like this.
> +static const struct snd_kcontrol_new lm49453_sidetone_mixer_controls[] = {
> +/* Sidetone supports mono only */
> +SOC_SINGLE_TLV("Sidetone ADCL Volume", LM49453_P0_STN_VOL_ADCL_REG,
> + 0, 0x3F, 0, digital_tlv),
> +SOC_SINGLE_TLV("Sidetone ADCR Volume", LM49453_P0_STN_VOL_ADCR_REG,
> + 0, 0x3F, 0, digital_tlv),
> +SOC_SINGLE_TLV("Sidetone DMIC1L Volume", LM49453_P0_STN_VOL_DMIC1L_REG,
> + 0, 0x3F, 0, digital_tlv),
> +SOC_SINGLE_TLV("Sidetone DMIC1R Volume", LM49453_P0_STN_VOL_DMIC1R_REG,
> + 0, 0x3F, 0, digital_tlv),
> +SOC_SINGLE_TLV("Sidetone DMIC2L Volume", LM49453_P0_STN_VOL_DMIC2L_REG,
> + 0, 0x3F, 0, digital_tlv),
> +SOC_SINGLE_TLV("Sidetone DMIC2R Volume", LM49453_P0_STN_VOL_DMIC2R_REG,
> + 0, 0x3F, 0, digital_tlv),
> +SOC_DAPM_SINGLE("Sidetone Enable Switch", LM49453_P0_DMIX_CLK_SEL_REG, 2, 1, 0),
Just Sidetone Switch, Switch implies enable/disable. You also shouldn't
have plain SOC_ controls mixed with the DAPM ones, the mixer should only
have DAPM controls.
> + SOC_DAPM_ENUM("Mic2Mode", lm49453_mic2mode_enum),
> + SOC_DAPM_ENUM("DMIC12 SRC", lm49453_dmic12_cfg_enum),
> + SOC_DAPM_ENUM("DMIC34 SRC", lm49453_dmic34_cfg_enum),
These shouldn't be in the regular ALSA controls, they should be attached
to a DAPM widget.
> + /* Capture path filter enable */
> + SOC_DAPM_SINGLE("DMIC1 HPFilter Switch",
> + LM49453_P0_ADC_FX_ENABLES_REG, 0, 1, 0),
This shouldn't be a DAPM control. I'm surprised this doesn't crash when
used...
> + SND_SOC_DAPM_PGA("Sidetone", SND_SOC_NOPM, 0, 0, NULL, 0),
Why have this separately to the mixer for the sidetone? It doesn't
actually do anything...
> + /* EP map */
> + { "EPOUT", "Earpiece Enable", "HPL DAC" },
Earpiece Enable should be a control of some kind for this to do anything
useful, and if it were a control it should be called Switch.
> + { "LSOUTL", "Speaker Left Enable", "LSL DAC"},
> + { "LSOUTR", "Speaker Left Enable", "LSR DAC"},
Similarly here and elsewhere.
> + /* Sidetone map */
> + { "Sidetone Mixer", "Sidetone Enable Switch", "ADC Left" },
> + { "Sidetone Mixer", "Sidetone Enable Switch", "ADC Right" },
> + { "Sidetone Mixer", "Sidetone Enable Switch", "DMIC1 Left" },
> + { "Sidetone Mixer", "Sidetone Enable Switch", "DMIC1 Right" },
> + { "Sidetone Mixer", "Sidetone Enable Switch", "DMIC2 Left" },
> + { "Sidetone Mixer", "Sidetone Enable Switch", "DMIC2 Right" },
This looks very wrong. Aside from the "Sidetone Enable Switch" naming
you've got *all* the inputs controlled by the same switch - is that
really the case? If it is you don't really have individual controls on
the inputs and you'd be better just putting a switch on the output of
the sidetone.
> + state->in = freq_in;
> + state->out = freq_out;
> + snd_soc_write(codec, LM49453_P0_FLL_REF_FREQL_REG, (state->in & 0xff));
> + snd_soc_write(codec, LM49453_P0_FLL_REF_FREQH_REG,
> + ((state->in >> 8) & 0xff));
So the PLL only supports inputs in the range 0-65535Hz? You probably
want to validate the input range...
> + snd_soc_write(codec, LM49453_P0_VCO_TARGETLL_REG,
> + state->out & 0xff);
> + snd_soc_write(codec, LM49453_P0_VCO_TARGETLH_REG,
> + (state->out >> 8) & 0xff);
> + snd_soc_write(codec, LM49453_P0_VCO_TARGETHL_REG,
> + (state->out >> 16) & 0xff);
> + snd_soc_write(codec, LM49453_P0_VCO_TARGETHH_REG,
> + (state->out >> 24) & 0xff);
Ditto here, though it's less urgent as the range is much greater, though
does the device really support up to 4.2GHz? That's *very* high for an
audio device.
> + dev_dbg(codec->dev, "\n in lm49453_hw_params\n");
I'm not a big fan of stuff like this, and the leading \n is certainly
out of style for the kernel.
> + switch (lm49453->sysclk) {
> + case 12288000:
> + case 26000000:
> + case 19000000:
> + pll_clk = ~BIT(4);
> + break;
> + case 48000:
> + case 32576:
> + pll_clk = BIT(4);
> + break;
This doesn't appear to be talking to the PLL configuration above which
is *very* weird.
> +static int lm49453_hp_mute(struct snd_soc_dai *dai, int mute)
> +{
> + if (mute)
> + snd_soc_update_bits(dai->codec, LM49453_P0_DAC_DSP_REG,
> + BIT(1)|BIT(0), BIT(1)|BIT(0));
> + return 0;
> +}
Once this has activated the mute the headphone will stay muted for ever
as it can only ever mute. That's probably not desirable.
> + case SND_SOC_BIAS_STANDBY:
> + if (codec->dapm.bias_level == SND_SOC_BIAS_OFF)
> + regcache_sync(lm49453->regmap);
> + break;
> +
> + case SND_SOC_BIAS_OFF:
> + reg = LM49453_PMC_SETUP_CHIP_EN;
> + snd_soc_update_bits(codec, LM49453_P0_PMC_SETUP_REG,
> + LM49453_PMC_SETUP_CHIP_EN, reg);
> + break;
This seems wrong, on STANDBY->OFF you disable the chip enable bit (which
is fine) but on OFF->STANDBY you only sync the cache (which is fine) and
don't reenable the chip (which seems like it's something you should do).
> + /* Get the codec into a known state */
> + reg = LM49453_RESET_REG_RST;
> + snd_soc_update_bits(codec, LM49453_P0_RESET_REG, BIT(0), reg);
> + if (ret != 0) {
> + dev_err(codec->dev, "Failed to reset codec: %d\n", ret);
> + return ret;
> + }
You should move this into the I2C probe, best to get control of the
device as soon as possible.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
Url : http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20120131/3a761001/attachment.sig
More information about the Alsa-devel
mailing list