March 2012 - Alsa-devel - mailman.alsa-project.org

Re: [alsa-devel] [PATCH v6] sound/soc/codecs: add LAPIS Semiconductor ML26124
by Mark Brown 21 Mar '12

21 Mar '12

On Tue, Mar 06, 2012 at 07:49:16PM +0900, Tomoya MORINAGA wrote: > On Tue, Mar 06, 2012　19:00 Mark Brown <broonie(a)opensource.wolfsonmicro.com>: > > This is not what the above prototype says, the above prototype has only > > one register in it. You need to make this *much* clearer for the > > registers it's affecting. > Sorry, I couldn't understand your saying / concern. > Do you mean I shouldn't use ml26124_update_bits() but snd_soc_update_bits() ? Ideally you should be using the framework features. If there is some reason why you can't you need to make it much clearer what the reason is so people reading the code can understand.

3 15

Re: [alsa-devel] [PATCH v5] sound/soc/lapis: add machine driver for ML7213 Carrier Board
by Mark Brown 21 Mar '12

21 Mar '12

On Tue, Mar 06, 2012 at 02:49:24PM +0900, Tomoya MORINAGA wrote: > 2012年3月2日22:05 Mark Brown <broonie(a)opensource.wolfsonmicro.com>: > > You're also missing a blank line between this and the previous stanza. > Sorry, I can't understand your saying. Your file should look like other similar files in the kernel. > > Use snd_soc_register_card() and a real device. > OK. > As a matter of interest, why do you recommend me to use > snd_soc_register_card() ? It's been the standard since 2.6.38.

2 11

[alsa-devel] [PATCH 1/3] ASoC: tegra: fix comment indentation in ALC5632 machine
by Stephen Warren 21 Mar '12

21 Mar '12

Fix comment indentation to clear checkpatch errors in a later patch. Signed-off-by: Stephen Warren <swarren(a)wwwdotorg.org> --- This series is for 3.5, and based on a merge of next-20120320 plus ASoC's for-3.5 branch, plus a couple minor patches to get recent next- compiling for Tegra, that don't affect the context of this series. Applying all the patches to the ASoC tree should be fine; I don't anticpate any conflicts with the likely Tegra tree for 3.5. sound/soc/tegra/tegra_alc5632.c | 24 ++++++++++++------------ 1 files changed, 12 insertions(+), 12 deletions(-) diff --git a/sound/soc/tegra/tegra_alc5632.c b/sound/soc/tegra/tegra_alc5632.c index e45ccd8..03ae095 100644 --- a/sound/soc/tegra/tegra_alc5632.c +++ b/sound/soc/tegra/tegra_alc5632.c @@ -1,16 +1,16 @@ /* -* tegra_alc5632.c -- Toshiba AC100(PAZ00) machine ASoC driver -* -* Copyright (C) 2011 The AC100 Kernel Team <ac100(a)lists.lauchpad.net> -* -* Authors: Leon Romanovsky <leon(a)leon.nu> -* Andrey Danin <danindrey(a)mail.ru> -* Marc Dietrich <marvin24(a)gmx.de> -* -* This program is free software; you can redistribute it and/or modify -* it under the terms of the GNU General Public License version 2 as -* published by the Free Software Foundation. -*/ + * tegra_alc5632.c -- Toshiba AC100(PAZ00) machine ASoC driver + * + * Copyright (C) 2011 The AC100 Kernel Team <ac100(a)lists.lauchpad.net> + * + * Authors: Leon Romanovsky <leon(a)leon.nu> + * Andrey Danin <danindrey(a)mail.ru> + * Marc Dietrich <marvin24(a)gmx.de> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ #include <asm/mach-types.h> -- 1.7.0.4

2 3

[alsa-devel] HDA intel, wired headset w/mic, jack mic detect
by Kevin Hilman 20 Mar '12

20 Mar '12

Hello, I'm trying to get a wired headset with a mic working on MacBookAir running v3.0 (3.0.0-13-generic from Ubuntu to be precise.) It has Intel HDA audio[1] and everything works fine, including the built-in mic. The only think I can't seem to get working is a headset w/mic. I know the HW for dynamic detect works because the mic is detected dynamically under OSX. Is there some setting I need to tweak in order to enable the automatic mic detection? Thanks for any pointers, Kevin [1] $ cat /proc/asound/cards 0 [PCH ]: HDA-Intel - HDA Intel PCH HDA Intel PCH at 0xa0600000 irq 46

6 25

[alsa-devel] [PATCH 1/3] ASoC: wm_hubs: Don't actively manage LINEOUT_VMID_BUF
by Mark Brown 19 Mar '12

19 Mar '12

It can just be enabled all the time with no impact. Signed-off-by: Mark Brown <broonie(a)opensource.wolfsonmicro.com> --- sound/soc/codecs/wm_hubs.c | 16 +++++----------- 1 files changed, 5 insertions(+), 11 deletions(-) diff --git a/sound/soc/codecs/wm_hubs.c b/sound/soc/codecs/wm_hubs.c index c08d1c2..f13f288 100644 --- a/sound/soc/codecs/wm_hubs.c +++ b/sound/soc/codecs/wm_hubs.c @@ -643,8 +643,6 @@ SND_SOC_DAPM_INPUT("IN2RP:VXRP"), SND_SOC_DAPM_SUPPLY("MICBIAS2", WM8993_POWER_MANAGEMENT_1, 5, 0, NULL, 0), SND_SOC_DAPM_SUPPLY("MICBIAS1", WM8993_POWER_MANAGEMENT_1, 4, 0, NULL, 0), -SND_SOC_DAPM_SUPPLY("LINEOUT_VMID_BUF", WM8993_ANTIPOP1, 7, 0, NULL, 0), - SND_SOC_DAPM_MIXER("IN1L PGA", WM8993_POWER_MANAGEMENT_2, 6, 0, in1l_pga, ARRAY_SIZE(in1l_pga)), SND_SOC_DAPM_MIXER("IN1R PGA", WM8993_POWER_MANAGEMENT_2, 4, 0, @@ -869,11 +867,9 @@ static const struct snd_soc_dapm_route lineout1_diff_routes[] = { }; static const struct snd_soc_dapm_route lineout1_se_routes[] = { - { "LINEOUT1N Mixer", NULL, "LINEOUT_VMID_BUF" }, { "LINEOUT1N Mixer", "Left Output Switch", "Left Output PGA" }, { "LINEOUT1N Mixer", "Right Output Switch", "Right Output PGA" }, - { "LINEOUT1P Mixer", NULL, "LINEOUT_VMID_BUF" }, { "LINEOUT1P Mixer", "Left Output Switch", "Left Output PGA" }, { "LINEOUT1N Driver", NULL, "LINEOUT1N Mixer" }, @@ -890,11 +886,9 @@ static const struct snd_soc_dapm_route lineout2_diff_routes[] = { }; static const struct snd_soc_dapm_route lineout2_se_routes[] = { - { "LINEOUT2N Mixer", NULL, "LINEOUT_VMID_BUF" }, { "LINEOUT2N Mixer", "Left Output Switch", "Left Output PGA" }, { "LINEOUT2N Mixer", "Right Output Switch", "Right Output PGA" }, - { "LINEOUT2P Mixer", NULL, "LINEOUT_VMID_BUF" }, { "LINEOUT2P Mixer", "Right Output Switch", "Right Output PGA" }, { "LINEOUT2N Driver", NULL, "LINEOUT2N Mixer" }, @@ -996,6 +990,11 @@ int wm_hubs_handle_analogue_pdata(struct snd_soc_codec *codec, WM8993_LINEOUT2_MODE, WM8993_LINEOUT2_MODE); + if (!lineout1_diff && !lineout2_diff) + snd_soc_update_bits(codec, WM8993_ANTIPOP1, + WM8993_LINEOUT_VMID_BUF_ENA, + WM8993_LINEOUT_VMID_BUF_ENA); + if (lineout1fb) snd_soc_update_bits(codec, WM8993_ADDITIONAL_CONTROL, WM8993_LINEOUT1_FB, WM8993_LINEOUT1_FB); @@ -1068,11 +1067,6 @@ void wm_hubs_set_bias_level(struct snd_soc_codec *codec, WM8993_LINEOUT2P_ENA, val); - if (!hubs->lineout1n_ena && !hubs->lineout1p_ena && - !hubs->lineout2n_ena && !hubs->lineout2p_ena) - snd_soc_update_bits(codec, WM8993_ANTIPOP1, - WM8993_LINEOUT_VMID_BUF_ENA, 0); - /* Remove the input clamps */ snd_soc_update_bits(codec, WM8993_INPUTS_CLAMP_REG, WM8993_INPUTS_CLAMP, 0); -- 1.7.9.1

1 2

[alsa-devel] [PATCH] ASoC: cs4270: Check that we can enable regulators on resume
by Mark Brown 19 Mar '12

19 Mar '12

It's possible that the regulator enable will fail and if it does we may as well just give up with trying to bring the rest of the device up and report the original error. Signed-off-by: Mark Brown <broonie(a)opensource.wolfsonmicro.com> --- sound/soc/codecs/cs4270.c | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-) diff --git a/sound/soc/codecs/cs4270.c b/sound/soc/codecs/cs4270.c index 1d672f5..15df950 100644 --- a/sound/soc/codecs/cs4270.c +++ b/sound/soc/codecs/cs4270.c @@ -602,8 +602,10 @@ static int cs4270_soc_resume(struct snd_soc_codec *codec) struct cs4270_private *cs4270 = snd_soc_codec_get_drvdata(codec); int reg; - regulator_bulk_enable(ARRAY_SIZE(cs4270->supplies), - cs4270->supplies); + ret = regulator_bulk_enable(ARRAY_SIZE(cs4270->supplies), + cs4270->supplies); + if (ret != 0) + return ret; /* In case the device was put to hard reset during sleep, we need to * wait 500ns here before any I2C communication. */ -- 1.7.9.1

2 4

[alsa-devel] [PATCH] ASoC: dapm: Remove bodges for no-widget CODECs
by Mark Brown 19 Mar '12

19 Mar '12

Now that we're creating widgets for all DAIs there should be no more need for the bodges we've been carrying for non-DAPM CODEC drivers so remove them. Signed-off-by: Mark Brown <broonie(a)opensource.wolfsonmicro.com> --- I don't actually have any no widget CODECs so can't really test - Timur, would it be possible for you to give it a spin? sound/soc/soc-dapm.c | 36 ++++-------------------------------- 1 files changed, 4 insertions(+), 32 deletions(-) diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c index 8ce1e9c..6e5869d 100644 --- a/sound/soc/soc-dapm.c +++ b/sound/soc/soc-dapm.c @@ -1441,12 +1441,10 @@ static int dapm_power_widgets(struct snd_soc_dapm_context *dapm, int event) trace_snd_soc_dapm_start(card); list_for_each_entry(d, &card->dapm_list, list) { - if (d->n_widgets || d->codec == NULL) { - if (d->idle_bias_off) - d->target_bias_level = SND_SOC_BIAS_OFF; - else - d->target_bias_level = SND_SOC_BIAS_STANDBY; - } + if (d->idle_bias_off) + d->target_bias_level = SND_SOC_BIAS_OFF; + else + d->target_bias_level = SND_SOC_BIAS_STANDBY; } dapm_reset(card); @@ -1491,32 +1489,6 @@ static int dapm_power_widgets(struct snd_soc_dapm_context *dapm, int event) } - /* If there are no DAPM widgets then try to figure out power from the - * event type. - */ - if (!dapm->n_widgets) { - switch (event) { - case SND_SOC_DAPM_STREAM_START: - case SND_SOC_DAPM_STREAM_RESUME: - dapm->target_bias_level = SND_SOC_BIAS_ON; - break; - case SND_SOC_DAPM_STREAM_STOP: - if (dapm->codec && dapm->codec->active) - dapm->target_bias_level = SND_SOC_BIAS_ON; - else - dapm->target_bias_level = SND_SOC_BIAS_STANDBY; - break; - case SND_SOC_DAPM_STREAM_SUSPEND: - dapm->target_bias_level = SND_SOC_BIAS_STANDBY; - break; - case SND_SOC_DAPM_STREAM_NOP: - dapm->target_bias_level = dapm->bias_level; - break; - default: - break; - } - } - /* Force all contexts in the card to the same bias state if * they're not ground referenced. */ -- 1.7.9.1

4 4

[alsa-devel] ASoC:Question rate constraint between the dais
by Ramesh Babu 19 Mar '12

19 Mar '12

Hi I would like to understand how to handle the different sampling rates between dais. I need to make sure that all simultaneous playback and capture (across the all dai_links) needs to be of same frequency family. If the sampling frequency of a new request is different from ongoing playback/capture, then it needs to be rejected. How do I handle it in machine driver? Thanks Ramesh

6 8

[alsa-devel] [PATCH] ASoC: omap-mcbsp: Use the common interrupt line if supported by the SoC
by Peter Ujfalusi 19 Mar '12

19 Mar '12

With the common irq the driver only needs to use one interrupt line, and it provides better debugging possibilites compared to the legacy TX/RX interrupt lines. Signed-off-by: Peter Ujfalusi <peter.ujfalusi(a)ti.com> --- Hello, with chages going via linux-omap: http://marc.info/?l=linux-omap&m=133217004811408&w=2 http://marc.info/?l=linux-omap&m=133217006911415&w=2 the McBSP driver will use the common interrupt line where it is supported. Regards, Peter sound/soc/omap/mcbsp.c | 113 +++++++++++++++++++++++++++++++++++++---------- sound/soc/omap/mcbsp.h | 7 +++- 2 files changed, 95 insertions(+), 25 deletions(-) diff --git a/sound/soc/omap/mcbsp.c b/sound/soc/omap/mcbsp.c index ec6832c..34835e8 100644 --- a/sound/soc/omap/mcbsp.c +++ b/sound/soc/omap/mcbsp.c @@ -109,6 +109,47 @@ static void omap_mcbsp_dump_reg(struct omap_mcbsp *mcbsp) dev_dbg(mcbsp->dev, "***********************\n"); } +static irqreturn_t omap_mcbsp_irq_handler(int irq, void *dev_id) +{ + struct omap_mcbsp *mcbsp = dev_id; + u16 irqst; + + irqst = MCBSP_READ(mcbsp, IRQST); + dev_dbg(mcbsp->dev, "IRQ callback : 0x%x\n", irqst); + + if (irqst & RSYNCERREN) + dev_err(mcbsp->dev, "RX Frame Sync Error!\n"); + if (irqst & RFSREN) + dev_dbg(mcbsp->dev, "RX Frame Sync\n"); + if (irqst & REOFEN) + dev_dbg(mcbsp->dev, "RX End Of Frame\n"); + if (irqst & RRDYEN) + dev_dbg(mcbsp->dev, "RX Buffer Threshold Reached\n"); + if (irqst & RUNDFLEN) + dev_err(mcbsp->dev, "RX Buffer Underflow!\n"); + if (irqst & ROVFLEN) + dev_err(mcbsp->dev, "RX Buffer Overflow!\n"); + + if (irqst & XSYNCERREN) + dev_err(mcbsp->dev, "TX Frame Sync Error!\n"); + if (irqst & XFSXEN) + dev_dbg(mcbsp->dev, "TX Frame Sync\n"); + if (irqst & XEOFEN) + dev_dbg(mcbsp->dev, "TX End Of Frame\n"); + if (irqst & XRDYEN) + dev_dbg(mcbsp->dev, "TX Buffer threshold Reached\n"); + if (irqst & XUNDFLEN) + dev_err(mcbsp->dev, "TX Buffer Underflow!\n"); + if (irqst & XOVFLEN) + dev_err(mcbsp->dev, "TX Buffer Overflow!\n"); + if (irqst & XEMPTYEOFEN) + dev_dbg(mcbsp->dev, "TX Buffer empty at end of frame\n"); + + MCBSP_WRITE(mcbsp, IRQST, irqst); + + return IRQ_HANDLED; +} + static irqreturn_t omap_mcbsp_tx_irq_handler(int irq, void *dev_id) { struct omap_mcbsp *mcbsp_tx = dev_id; @@ -176,6 +217,10 @@ void omap_mcbsp_config(struct omap_mcbsp *mcbsp, /* Enable wakeup behavior */ if (mcbsp->pdata->has_wakeup) MCBSP_WRITE(mcbsp, WAKEUPEN, XRDYEN | RRDYEN); + + /* Enable TX/RX sync error interrupts by default */ + if (mcbsp->irq) + MCBSP_WRITE(mcbsp, IRQEN, RSYNCERREN | XSYNCERREN); } /** @@ -489,23 +534,25 @@ int omap_mcbsp_request(struct omap_mcbsp *mcbsp) MCBSP_WRITE(mcbsp, SPCR1, 0); MCBSP_WRITE(mcbsp, SPCR2, 0); - err = request_irq(mcbsp->tx_irq, omap_mcbsp_tx_irq_handler, - 0, "McBSP", (void *)mcbsp); - if (err != 0) { - dev_err(mcbsp->dev, "Unable to request TX IRQ %d " - "for McBSP%d\n", mcbsp->tx_irq, - mcbsp->id); - goto err_clk_disable; - } + if (mcbsp->irq) { + err = request_irq(mcbsp->irq, omap_mcbsp_irq_handler, 0, + "McBSP", (void *)mcbsp); + if (err != 0) { + dev_err(mcbsp->dev, "Unable to request IRQ\n"); + goto err_clk_disable; + } + } else { + err = request_irq(mcbsp->tx_irq, omap_mcbsp_tx_irq_handler, 0, + "McBSP TX", (void *)mcbsp); + if (err != 0) { + dev_err(mcbsp->dev, "Unable to request TX IRQ\n"); + goto err_clk_disable; + } - if (mcbsp->rx_irq) { - err = request_irq(mcbsp->rx_irq, - omap_mcbsp_rx_irq_handler, - 0, "McBSP", (void *)mcbsp); + err = request_irq(mcbsp->rx_irq, omap_mcbsp_rx_irq_handler, 0, + "McBSP RX", (void *)mcbsp); if (err != 0) { - dev_err(mcbsp->dev, "Unable to request RX IRQ %d " - "for McBSP%d\n", mcbsp->rx_irq, - mcbsp->id); + dev_err(mcbsp->dev, "Unable to request RX IRQ\n"); goto err_free_irq; } } @@ -542,9 +589,16 @@ void omap_mcbsp_free(struct omap_mcbsp *mcbsp) if (mcbsp->pdata->has_wakeup) MCBSP_WRITE(mcbsp, WAKEUPEN, 0); - if (mcbsp->rx_irq) + /* Disable interrupt requests */ + if (mcbsp->irq) + MCBSP_WRITE(mcbsp, IRQEN, 0); + + if (mcbsp->irq) { + free_irq(mcbsp->irq, (void *)mcbsp); + } else { free_irq(mcbsp->rx_irq, (void *)mcbsp); - free_irq(mcbsp->tx_irq, (void *)mcbsp); + free_irq(mcbsp->tx_irq, (void *)mcbsp); + } reg_cache = mcbsp->reg_cache; @@ -949,13 +1003,24 @@ int __devinit omap_mcbsp_init(struct platform_device *pdev) else mcbsp->phys_dma_base = res->start; - mcbsp->tx_irq = platform_get_irq_byname(pdev, "tx"); - mcbsp->rx_irq = platform_get_irq_byname(pdev, "rx"); - - /* From OMAP4 there will be a single irq line */ - if (mcbsp->tx_irq == -ENXIO) { - mcbsp->tx_irq = platform_get_irq(pdev, 0); - mcbsp->rx_irq = 0; + /* + * OMAP1, 2 uses two interrupt lines: TX, RX + * OMAP2430, OMAP3 SoC have combined IRQ line as well. + * OMAP4 and newer SoC only have the combined IRQ line. + * Use the combined IRQ if available since it gives better debugging + * possibilities. + */ + mcbsp->irq = platform_get_irq_byname(pdev, "common"); + if (mcbsp->irq == -ENXIO) { + mcbsp->tx_irq = platform_get_irq_byname(pdev, "tx"); + + if (mcbsp->tx_irq == -ENXIO) { + mcbsp->irq = platform_get_irq(pdev, 0); + mcbsp->tx_irq = 0; + } else { + mcbsp->rx_irq = platform_get_irq_byname(pdev, "rx"); + mcbsp->irq = 0; + } } res = platform_get_resource_byname(pdev, IORESOURCE_DMA, "rx"); diff --git a/sound/soc/omap/mcbsp.h b/sound/soc/omap/mcbsp.h index 179a822..262a615 100644 --- a/sound/soc/omap/mcbsp.h +++ b/sound/soc/omap/mcbsp.h @@ -218,15 +218,19 @@ enum { #define MCBSP_DMA_MODE_ELEMENT 0 #define MCBSP_DMA_MODE_THRESHOLD 1 -/********************** McBSP WAKEUPEN bit definitions *********************/ +/********************** McBSP WAKEUPEN/IRQST/IRQEN bit definitions *********/ #define RSYNCERREN BIT(0) #define RFSREN BIT(1) #define REOFEN BIT(2) #define RRDYEN BIT(3) +#define RUNDFLEN BIT(4) +#define ROVFLEN BIT(5) #define XSYNCERREN BIT(7) #define XFSXEN BIT(8) #define XEOFEN BIT(9) #define XRDYEN BIT(10) +#define XUNDFLEN BIT(11) +#define XOVFLEN BIT(12) #define XEMPTYEOFEN BIT(14) /* Clock signal muxing options */ @@ -294,6 +298,7 @@ struct omap_mcbsp { int configured; u8 free; + int irq; int rx_irq; int tx_irq; -- 1.7.8.5

2 1

Re: [alsa-devel] [PATCH 13/16] ASoC: codecs: Add AB8500 codec-driver
by Mark Brown 19 Mar '12

19 Mar '12

On Tue, Mar 13, 2012 at 04:11:40PM +0100, Ola Lilja wrote: > +++ b/sound/soc/codecs/Kconfig > @@ -12,6 +12,7 @@ config SND_SOC_ALL_CODECS > tristate "Build all ASoC CODEC drivers" > select SND_SOC_88PM860X if MFD_88PM860X > select SND_SOC_L3 > + select SND_SOC_AB8500 > select SND_SOC_AC97_CODEC if SND_SOC_AC97_BUS > select SND_SOC_AD1836 if SPI_MASTER > select SND_SOC_AD193X if SND_SOC_I2C_AND_SPI This driver really has no dependencies? That seems *extremely* surprising - have you tried building on x86? > + > +ifdef CONFIG_SND_SOC_UX500_DEBUG > +CFLAGS_ab8500_audio.o := -DDEBUG > +endif Once again if you're adding random custom stuff like this for your driver you shouldn't be. > diff --git a/sound/soc/codecs/ab8500_audio.c b/sound/soc/codecs/ab8500_audio.c Everything else uses -codec. > +/* AB8500 register cache */ > +static const u8 ab8500_reg_cache[AB8500_CACHEREGNUM]; > +static enum sid_state sid_status = SID_UNCONFIGURED; No static data or custom cache implementations. > +/* Read a register from the audio-bank of AB8500 */ > +static unsigned int ab8500_codec_read_reg(struct snd_soc_codec *codec, unsigned int reg) > +{ > + int status; > + unsigned int value = 0; > + > + if (reg < VIRT_REG_BASE) { Why are you doing this virtual register stuff? It's making the code a lot more complex and doesn't look at all driver specific. > + } else { > + dev_dbg(codec->dev, "%s: Read 0x%02x from register 0x%02x:0x%02x\n", > + __func__, value8, (u8)AB8500_AUDIO, (u8)reg); There's already *plenty* of trace for register I/O in the framework. > +static const char * const enum_ena_dis[] = {"Enabled", "Disabled"}; > +static const char * const enum_dis_ena[] = {"Disabled", "Enabled"}; Why are the controls using these enums and not Switch controls? UIs know how to display switches. > +/* Earpiece - Mute */ > +static const struct snd_kcontrol_new dapm_ear_mute[] = { > + SOC_DAPM_SINGLE("Playback Switch", REG_MUTECONF, REG_MUTECONF_MUTEAR, 1, 1), > +}; This looks like it's just a mute rather than a mixer input enable (there's only one control...) so should be a regular sound control; unless there's a very good reason mutes other than mixer inputs normally don't affect the audio path as you might get pops and you probably don't want to do things like stop clocking just because the output is silenced. Similar thing applies in several other places. > +/* Earpiece source selector */ > +static const char * const enum_ear_lineout_source[] = {"Headset Left", "IHF Left"}; > +static SOC_ENUM_SINGLE_DECL(dapm_enum_ear_lineout_source, REG_DMICFILTCONF, > + REG_DMICFILTCONF_DA3TOEAR, enum_ear_lineout_source); > +static const struct snd_kcontrol_new dapm_ear_lineout_source = > + SOC_DAPM_ENUM("Earpiece or LineOut Mono Source", dapm_enum_ear_lineout_source); This control can probably have a much better name... > +/* IHF - Enable/Disable */ > +static const struct soc_enum enum_ihf_left = SOC_ENUM_SINGLE(0, 0, 2, enum_dis_ena); 80 columns please. > +/* from -31 to 31 dB in 1 dB steps (mute instead of -32 dB) */ > +static DECLARE_TLV_DB_SCALE(adx_dig_gain_tlv, -3200, 100, 1); I'm really not a big fan of this sort of comment, adds greatly to the verbosity but just duplicates the code. > +static const char * const enum_earselcm[] = {"0.95V", "1.10V", "1.27V", "1.58V"}; > +static SOC_ENUM_SINGLE_DECL(soc_enum_earselcm, > + REG_ANACONF1, REG_ANACONF1_EARSELCM, enum_earselcm); Should this really be configured by the application layer and not platform data? > +/* Digital interface - DA from slot mapping */ > +static const char * const enum_da_from_slot_map[] = {"SLOT0", > + "SLOT1", Indentation. Also are you sure that this isn't DAPM stuff? > + SOC_ENUM("Earpiece DAC Low Power Playback Switch", > + soc_enum_eardaclowpow), These aren't switches, they're enums. Switches are strictly boolean things with values 0 and 1. > + snd_soc_update_bits(codec, > + REG_DIGIFCONF3, > + BMASK(REG_DIGIFCONF3_IF1MASTER), > + BMASK(REG_DIGIFCONF3_IF1MASTER)); You're unconditionally setting these bits? > + snd_soc_update_bits(codec, REG_DIGIFCONF4, clr_mask, set_mask); Either clr_mask and set_mask are misnamed or this doesn't do what you think it does. The bits specified by the third argument are set to the values given in the fourth argument. It looks like the code thinks that the bits in the third argument will be cleared and the bits in the fourth argument will be set. > +static int ab8500_codec_set_word_length_if1(struct snd_soc_codec *codec, unsigned int wl) > +{ Just inline stuff like this. > +static int ab8500_codec_set_bit_delay_if1(struct snd_soc_codec *codec, unsigned int delay) This too. > +/* Configures audio macrocell into the AB8500 Chip */ > +static int ab8500_codec_configure_audio_macrocell(struct snd_soc_codec *codec) > +{ > + u8 value8; > + unsigned int value; > + int status; > + > + status = ab8500_sysctrl_write(AB8500_STW4500CTRL3, > + AB8500_STW4500CTRL3_CLK32KOUT2DIS | AB8500_STW4500CTRL3_RESETAUDN, > + AB8500_STW4500CTRL3_RESETAUDN); > + if (status < 0) > + return status; You're just unconditionally setting these values - shouldn't they be platform data? > +static int ab8500_codec_add_widgets(struct snd_soc_codec *codec) > +{ > + int status; > + > + status = snd_soc_dapm_new_controls(&codec->dapm, ab8500_dapm_widgets, > + ARRAY_SIZE(ab8500_dapm_widgets)); Just point at the tables from the snd_soc_codec and remove all this code. > +static int ab8500_codec_pcm_hw_params(struct snd_pcm_substream *substream, > + struct snd_pcm_hw_params *hw_params, struct snd_soc_dai *dai) > +{ > + dev_dbg(dai->codec->dev, "%s Enter.\n", __func__); > + > + return 0; > +} Remove all empty functions. > + > +struct snd_soc_dai_driver ab8500_codec_dai[] = { > + { > + .name = "ab8500-codec-dai.0", > + .id = 0, > + .playback = { > + .stream_name = "ab8500_0p", You should hook up the links to the widgets using DAPM routes, currently you're using the implicitly created routes from the stream name which is still supported but isn't best practice. > +static int ab8500_codec_remove(struct snd_soc_codec *codec) > +{ > + dev_dbg(codec->dev, "%s Enter.\n", __func__); > + > + snd_soc_dapm_free(&codec->dapm); Why are you doing this? > + /* Create register cache */ > + ab8500_codec_driver.reg_cache_size = AB8500_LAST_REG - AB8500_FIRST_REG; > + ab8500_codec_driver.reg_cache_default = > + kzalloc(ab8500_codec_driver.reg_cache_size * sizeof(u8), GFP_KERNEL); The driver struct should be constant, the stuff you're initialising it with here is compile time constant - why are you doing this dynamically at runtime/ > + > + /* Create driver private-data struct */ > + drvdata = kzalloc(sizeof(struct ab8500_codec_drvdata), GFP_KERNEL); Anything you do alloc should be devm_kzalloc(). > + struct ab8500_codec_drvdata *drvdata = dev_get_drvdata(&pdev->dev); > + > + dev_info(&pdev->dev, "%s Enter.\n", __func__); > + > + kfree(ab8500_codec_driver.reg_cache_default); > + > + kfree(drvdata->virt_reg_cache); > + kfree(drvdata); > + > + snd_soc_unregister_codec(&pdev->dev); This would avoid issues like this where you free the data the device uses before you free the device. > +static int __devinit ab8500_codec_platform_driver_init(void) module_platform_driver. > +/* Extended interface for codec-driver */ > + > +int ab8500_audio_power_control(struct snd_soc_codec *codec, bool power_on); > +int ab8500_audio_set_word_length(struct snd_soc_dai *dai, unsigned int wl); > +int ab8500_audio_set_bit_delay(struct snd_soc_dai *dai, unsigned int delay); > +int ab8500_audio_setup_if1(struct snd_soc_codec *codec, > + unsigned int fmt, > + unsigned int wl, > + unsigned int delay); > +void ab8500_audio_anc_configure(struct snd_soc_codec *codec, > + bool apply_fir, bool apply_iir); No, you shouldn't be exporting this stuff - it all looks like basic framework stuff. > +/* Virtual register base > + * This address should be located above any physical register used by ASoC. > + * It is used to map virtual registers needed by the codec-driver. > + */ > +#define VIRT_REG_BASE 0x3000 Why are you doing this virtual register stuff? > +#define REG_DAPATHCONF_ENDACHSL 5 All these constants need to be driver local or namespaced (preferrably namespaced whatever).

4 11