All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/4] ASoC: ak5702: add support for ak5702 -- 4ch ADC
@ 2012-10-29  8:43 Paolo Doz
  2012-10-29 12:41 ` Lars-Peter Clausen
  2012-10-29 16:04 ` Mark Brown
  0 siblings, 2 replies; 7+ messages in thread
From: Paolo Doz @ 2012-10-29  8:43 UTC (permalink / raw)
  To: broonie, lrg; +Cc: alsa-devel

This codec driver adds support for Asahi Kasei AK5702.

Signed-off-by: Paolo Doz <paolo.doz@gmail.com>
---
 sound/soc/codecs/ak5702.c |  728
+++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 728 insertions(+)
 create mode 100644 sound/soc/codecs/ak5702.c

diff --git a/sound/soc/codecs/ak5702.c b/sound/soc/codecs/ak5702.c
new file mode 100644
index 0000000..47c2e52
--- /dev/null
+++ b/sound/soc/codecs/ak5702.c
@@ -0,0 +1,728 @@
+/*
+ * ak5702.c  --  AK5702 ALSA SoC driver
+ *
+ * Author:      Paolo Doz <paolo.doz@gmail.com>
+ * Copyright:   (C) 2012 Soft In S.r.l.
+ *                  2009 Freescale Semiconductor, Inc. All rights reserved.
+ *
+ * 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.
+ *
+ * This code comes from the original Freescale Ak5702 Audio driver
+ */
+
+/* #define DEBUG 1 */
+
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/init.h>
+#include <linux/delay.h>
+#include <linux/pm.h>
+#include <linux/i2c.h>
+#include <linux/platform_device.h>
+#include <sound/core.h>
+#include <sound/pcm.h>
+#include <sound/pcm_params.h>
+#include <sound/soc.h>
+#include <sound/soc-dapm.h>
+#include <sound/initval.h>
+#include <linux/slab.h>
+#include <linux/debugfs.h>
+
+#include <sound/ak5702.h>
+#include "ak5702.h"
+
+#define AK5702_VERSION "0.2"
+#define DRV_NAME "ak5702"
+
+/* codec private data */
+struct ak5702_priv {
+    enum snd_soc_control_type control_type;
+    void *control_data;
+    unsigned int sysclk;
+    enum ak5702_clock_mode clock_mode;
+    struct dentry *debugfs;
+};
+
+/*
+ * ak5702 register cache
+ */
+static u16 ak5702_reg[AK5702_CACHEREGNUM] = {
+    0x00, 0x24, 0x00, 0x01, 0x23, 0x1f,
+    0x00, 0x01, 0x91, 0x00, 0xe1, 0x00,
+    0xa0, 0x00, 0x00, 0x00, 0x01, 0x20,
+    0x00, 0x00, 0x01, 0x91, 0x00, 0xe1,
+    0x00,
+};
+
+/*
+ * read ak5702 register cache
+ */
+static inline unsigned int ak5702_read_reg_cache(struct snd_soc_codec
*codec,
+                unsigned int reg)
+{
+    u16 *cache = codec->reg_cache;
+
+    if (reg >= AK5702_CACHEREGNUM)
+        return -1;
+    return cache[reg];
+}
+
+/*
+ * write ak5702 register cache
+ */
+static inline void ak5702_write_reg_cache(struct snd_soc_codec *codec,
+                u16 reg, unsigned int value)
+{
+    u16 *cache = codec->reg_cache;
+
+    if (reg >= AK5702_CACHEREGNUM)
+        return;
+    cache[reg] = value;
+    ak5702_reg[reg] = value;
+}
+
+/*
+ * write to the AK5702 register space
+ */
+static int ak5702_write(struct snd_soc_codec *codec, unsigned int reg,
+                unsigned int value)
+{
+    u8 data[2];
+    data[0] = reg & 0xff;
+    data[1] = value & 0xff;
+
+    if (codec->hw_write(codec->control_data, data, 2) == 2) {
+        pr_debug("I2c write success @ reg = %x, val = %x\n",
+             data[0], data[1]);
+        ak5702_write_reg_cache(codec, reg, value);
+        return 0;
+    } else {
+        pr_debug("I2c write error reg = %x, val = %x\n",
+            data[0], data[1]);
+        return -EIO;
+    }
+}
+
+#ifdef CONFIG_DEBUG_FS
+static int ak5702_show(struct seq_file *s, void *unused)
+{
+#define REG(r) { r, #r }
+    static const struct {
+        int offset;
+        const char *name;
+    } regs[] = {
+        REG(AK5702_PM1),
+        REG(AK5702_PLL1),
+        REG(AK5702_SIG1),
+        REG(AK5702_MICG1),
+        REG(AK5702_FMT1),
+        REG(AK5702_FS1),
+        REG(AK5702_CLK1),
+        REG(AK5702_VOLCTRL1),
+        REG(AK5702_LVOL1),
+        REG(AK5702_RVOL1),
+        REG(AK5702_TIMER1),
+        REG(AK5702_ALC11),
+        REG(AK5702_ALC12),
+        REG(AK5702_MODE11),
+        REG(AK5702_MODE12),
+        REG(AK5702_MODE13),
+
+        REG(AK5702_PM2),
+        REG(AK5702_PLL2),
+        REG(AK5702_SIG2),
+        REG(AK5702_MICG2),
+        REG(AK5702_FMT2),
+        REG(AK5702_FS2),
+        REG(AK5702_CLK2),
+        REG(AK5702_VOLCTRL2),
+        REG(AK5702_LVOL2),
+        REG(AK5702_RVOL2),
+        REG(AK5702_TIMER2),
+        REG(AK5702_ALC21),
+        REG(AK5702_ALC22),
+        REG(AK5702_MODE21),
+        REG(AK5702_MODE22),
+    };
+#undef REG
+
+    int i;
+    for (i = 0; i < ARRAY_SIZE(ak5702_reg); i++) {
+        seq_printf(s, "%s = 0x%02x\n", regs[i].name,
+                        ak5702_reg[regs[i].offset]);
+    }
+
+    return 0;
+}
+
+static int ak5702_debug_open(struct inode *inode, struct file *file)
+{
+    return single_open(file, ak5702_show, inode->i_private);
+}
+
+static const struct file_operations ak5702_debug_fops = {
+    .open    = ak5702_debug_open,
+    .read    = seq_read,
+    .llseek  = seq_lseek,
+    .release = single_release,
+};
+
+static void ak5702_debug_add(struct ak5702_priv *ak5702)
+{
+    char name[] = DRV_NAME ".0";
+
+    snprintf(name, sizeof(name), DRV_NAME);
+    ak5702->debugfs = debugfs_create_file(name, S_IRUGO,
+            snd_soc_debugfs_root, ak5702, &ak5702_debug_fops);
+}
+
+static void ak5702_debug_remove(struct ak5702_priv *ak5702)
+{
+    if (ak5702->debugfs)
+        debugfs_remove(ak5702->debugfs);
+}
+#else
+static inline void ak5702_debug_add(struct ak5702_priv *ak5702)
+{
+}
+
+static inline void ak5702_debug_remove(struct ak5702_priv *ak5702)
+{
+}
+#endif
+
+static const char *ak5702_mic_gain[] = { "0dB", "+15dB", "+30dB", "+36dB"
};
+static const char *ak5702_adc_input_type[] = {
+    "Single-ended", "Full-differential" };
+static const char *ak5702_adca_left_input[] = { "LIN1", "LIN2" };
+static const char *ak5702_adca_right_input[] = { "RIN1", "RIN2" };
+static const char *ak5702_adca_left5[] = { "LIN1-2", "LIN5" };
+static const char *ak5702_adca_right5[] = { "RIN1-2", "RIN5" };
+
+static const char *ak5702_adcb_left_input[] = { "LIN3", "LIN4" };
+static const char *ak5702_adcb_right_input[] = { "RIN3", "RIN4" };
+static const char *ak5702_adcb_left5[] = { "LIN3-4", "LIN5" };
+static const char *ak5702_adcb_right5[] = { "RIN3-4", "RIN5" };
+
+static const char *ak5702_input_vol_control[] = { "Independent",
"Dependent" };
+static const char *ak5702_mic_power[] = { "Off", "On" };
+
+static const struct soc_enum ak5702_enum[] = {
+    SOC_ENUM_SINGLE(AK5702_MICG1, 0, 4, ak5702_mic_gain),
+    SOC_ENUM_SINGLE(AK5702_MICG2, 0, 4, ak5702_mic_gain),
+
+    SOC_ENUM_SINGLE(AK5702_SIG1, 0, 2, ak5702_adca_left_input),
+    SOC_ENUM_SINGLE(AK5702_SIG1, 1, 2, ak5702_adca_right_input),
+    SOC_ENUM_SINGLE(AK5702_SIG2, 0, 2, ak5702_adcb_left_input),
+    SOC_ENUM_SINGLE(AK5702_SIG2, 1, 2, ak5702_adcb_right_input),
+
+    SOC_ENUM_SINGLE(AK5702_SIG1, 2, 2, ak5702_adc_input_type),
+    SOC_ENUM_SINGLE(AK5702_SIG1, 3, 2, ak5702_adc_input_type),
+    SOC_ENUM_SINGLE(AK5702_SIG2, 2, 2, ak5702_adc_input_type),
+    SOC_ENUM_SINGLE(AK5702_SIG2, 3, 2, ak5702_adc_input_type),
+
+    SOC_ENUM_SINGLE(AK5702_VOLCTRL1, 0, 2, ak5702_input_vol_control),
+    SOC_ENUM_SINGLE(AK5702_VOLCTRL2, 0, 2, ak5702_input_vol_control),
+
+    SOC_ENUM_SINGLE(AK5702_SIG1, 5, 2, ak5702_adca_left5),
+    SOC_ENUM_SINGLE(AK5702_SIG1, 6, 2, ak5702_adca_right5),
+    SOC_ENUM_SINGLE(AK5702_SIG2, 5, 2, ak5702_adcb_left5),
+    SOC_ENUM_SINGLE(AK5702_SIG2, 6, 2, ak5702_adcb_right5),
+
+    SOC_ENUM_SINGLE(AK5702_SIG1, 4, 2, ak5702_mic_power),
+    SOC_ENUM_SINGLE(AK5702_SIG2, 4, 2, ak5702_mic_power),
+};
+
+static const struct snd_kcontrol_new ak5702_snd_controls[] = {
+    SOC_SINGLE("ADCA Left Vol", AK5702_LVOL1, 0, 242, 0),
+    SOC_SINGLE("ADCA Right Vol", AK5702_RVOL1, 0, 242, 0),
+    SOC_SINGLE("ADCB Left Vol", AK5702_LVOL2, 0, 242, 0),
+    SOC_SINGLE("ADCB Right Vol", AK5702_RVOL2, 0, 242, 0),
+
+    SOC_ENUM("MIC-AmpA Gain", ak5702_enum[0]),
+    SOC_ENUM("MIC-AmpB Gain", ak5702_enum[1]),
+
+    SOC_ENUM("ADCA Left Source", ak5702_enum[2]),
+    SOC_ENUM("ADCA Right Source", ak5702_enum[3]),
+    SOC_ENUM("ADCB Left Source", ak5702_enum[4]),
+    SOC_ENUM("ADCB Right Source", ak5702_enum[5]),
+
+    SOC_ENUM("ADCA Left Type", ak5702_enum[6]),
+    SOC_ENUM("ADCA Right Type", ak5702_enum[7]),
+    SOC_ENUM("ADCB Left Type", ak5702_enum[8]),
+    SOC_ENUM("ADCB Right Type", ak5702_enum[9]),
+
+    SOC_ENUM("ADCA Input Vol mode", ak5702_enum[10]),
+    SOC_ENUM("ADCB Input Vol mode", ak5702_enum[11]),
+
+    SOC_ENUM("ADCA LIN5 Source", ak5702_enum[12]),
+    SOC_ENUM("ADCA RIN5 Source", ak5702_enum[13]),
+    SOC_ENUM("ADCB LIN5 Source", ak5702_enum[14]),
+    SOC_ENUM("ADCB RIN5 Source", ak5702_enum[15]),
+
+    SOC_ENUM("MIC-A Power", ak5702_enum[16]),
+    SOC_ENUM("MIC-B Power", ak5702_enum[17]),
+};
+
+/* ak5702 dapm widgets */
+static const struct snd_soc_dapm_widget ak5702_dapm_widgets[] = {
+    SND_SOC_DAPM_ADC("ADCA Left", "Capture", AK5702_PM1, 0, 0),
+    SND_SOC_DAPM_ADC("ADCA Right", "Capture", AK5702_PM1, 1, 0),
+    SND_SOC_DAPM_ADC("ADCB Left", "Capture", AK5702_PM2, 0, 0),
+    SND_SOC_DAPM_ADC("ADCB Right", "Capture", AK5702_PM2, 1, 0),
+
+    SND_SOC_DAPM_INPUT("ADCA Left Input"),
+    SND_SOC_DAPM_INPUT("ADCA Right Input"),
+    SND_SOC_DAPM_INPUT("ADCB Left Input"),
+    SND_SOC_DAPM_INPUT("ADCB Right Input"),
+};
+
+static const struct snd_soc_dapm_route audio_map[] = {
+    {"ADCA Left", NULL, "ADCA Left Input"},
+    {"ADCA Right", NULL, "ADCA Right Input"},
+    {"ADCB Left", NULL, "ADCB Left Input"},
+    {"ADCB Right", NULL, "ADCB Right Input"},
+};
+
+static int ak5702_dai_startup(struct snd_pcm_substream *substream,
+                struct snd_soc_dai *dai)
+{
+    pr_debug("Entered %s\n", __func__);
+    return 0;
+}
+
+static void ak5702_dai_shutdown(struct snd_pcm_substream *substream,
+                struct snd_soc_dai *dai)
+{
+    pr_debug("Entered %s\n", __func__);
+}
+
+static int ak5702_dai_hw_params(struct snd_pcm_substream *substream,
+                struct snd_pcm_hw_params *params,
+                struct snd_soc_dai *dai)
+{
+    u8 value;
+    struct snd_soc_codec *codec = dai->codec;
+    struct ak5702_priv *ak5702 = snd_soc_codec_get_drvdata(codec);
+
+    pr_debug("Entered %s\n", __func__);
+
+    switch (params_channels(params)) {
+    case 2:
+        value = ak5702_read_reg_cache(codec, AK5702_FMT1);
+        value &= (~AK5702_FMT1_TDM_MASK);
+        ak5702_write(codec, AK5702_FMT1, value);
+        break;
+    case 8:
+        value = ak5702_read_reg_cache(codec, AK5702_FMT1);
+        value &= (~AK5702_FMT1_TDM_MASK);
+        if (params_format(params) == SNDRV_PCM_FORMAT_S32_LE)
+            value |= AK5702_FMT1_TDM256;
+        else if (params_format(params) == SNDRV_PCM_FORMAT_S16_LE)
+            value |= AK5702_FMT1_TDM128;
+        ak5702_write(codec, AK5702_FMT1, value);
+        break;
+    default:
+        return -EINVAL;
+    }
+
+    /* read fs select register */
+    value = ak5702_read_reg_cache(codec, AK5702_FS1);
+    value &= (~(AK5702_FS1_FS_MASK|AK5702_FS1_BCKO_MASK));
+    switch (params_rate(params)) {
+    case 48000:
+        if (ak5702->clock_mode == PLL_slave_mode2)
+            value |= AK5702_FS1_BCLK_MODE2;
+        else if (ak5702->clock_mode == EXT_slave_mode)
+            value |= AK5702_FS1_SLAVE_256FS; /* mode 3 256fs*/
+        else
+            return -EINVAL;
+        break;
+    default:
+        return -EINVAL;
+    }
+    ak5702_write(codec, AK5702_FS1, value);
+
+    /* power on ADCA */
+    value = AK5702_PM1_PMADAL | AK5702_PM1_PMADAR | AK5702_PM1_PMVCM;
+    ak5702_write(codec, AK5702_PM1, value);
+
+    /* power on ADCB */
+    value = AK5702_PM2_PMADBL | AK5702_PM2_PMADBR;
+    ak5702_write(codec, AK5702_PM2, value);
+    return 0;
+}
+
+static int ak5702_dai_hw_free(struct snd_pcm_substream *substream,
+                struct snd_soc_dai *dai)
+{
+    struct snd_soc_codec *codec = dai->codec;
+    int value;
+
+    pr_debug("Entered %s\n", __func__);
+    /* power down ADCA */
+    value = AK5702_POWERDOWN | AK5702_PM1_PMVCM;
+    ak5702_write(codec, AK5702_PM1, value);
+
+    /* power down ADCB */
+    value = AK5702_POWERDOWN;
+    ak5702_write(codec, AK5702_PM2, value);
+    return 0;
+}
+
+static int ak5702_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 ak5702_priv *ak5702 = snd_soc_codec_get_drvdata(codec);
+    int value = 0;
+    switch (clk_id) {
+    case PLL_master_mode:
+    case EXT_master_mode:
+        if (dir != SND_SOC_CLOCK_OUT)
+            return -EINVAL;
+        break;
+    case PLL_slave_mode:
+    case PLL_slave_mode2:
+        if (dir != SND_SOC_CLOCK_IN)
+            return -EINVAL;
+        value = AK5702_PLL1_POWERUP |
+            AK5702_PLL1_SLAVE |
+            AK5702_PLL1_MODE2;
+        break;
+    case EXT_slave_mode:
+        if (dir != SND_SOC_CLOCK_IN)
+            return -EINVAL;
+        value = AK5702_PLL1_POWERDOWN | AK5702_PLL1_SLAVE;
+        break;
+    default:
+        return -EINVAL;
+    }
+    ak5702_write(codec, AK5702_PLL1, value);
+    ak5702->clock_mode = clk_id;
+
+    return 0;
+}
+
+static int ak5702_set_dai_fmt(struct snd_soc_dai *codec_dai, unsigned int
fmt)
+{
+    struct snd_soc_codec *codec = codec_dai->codec;
+    int value;
+    u8 fmt1 = 0;
+    pr_debug("Entered %s", __func__);
+
+    switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
+    case SND_SOC_DAIFMT_DSP_A:
+    case SND_SOC_DAIFMT_I2S:
+        fmt1 = AK5702_FMT1_I2S;
+        break;
+    case SND_SOC_DAIFMT_LEFT_J:
+        fmt1 = AK5702_FMT1_MSB;
+        break;
+    default:
+        return -EINVAL;
+    }
+    value = ak5702_read_reg_cache(codec, AK5702_FMT1);
+    value &= (~AK5702_FS1_FS_MASK);
+    value |= fmt1;
+    ak5702_write(codec, AK5702_FMT1, value);
+    ak5702_write(codec, AK5702_FMT2, AK5702_FMT2_STEREO);
+    return 0;
+}
+
+static int ak5702_set_dai_pll(struct snd_soc_dai *codec_dai, int pll_id,
+        int source, unsigned int freq_in, unsigned int freq_out)
+{
+    struct snd_soc_codec *codec = codec_dai->codec;
+    u8 reg = 0;
+    pr_debug("Entered %s", __func__);
+    reg = ak5702_read_reg_cache(codec, AK5702_PLL1);
+    switch (pll_id) {
+    case AK5702_PLL_POWERDOWN:
+        reg &= (~AK5702_PLL1_PM_MASK);
+        reg |= AK5702_PLL1_POWERDOWN;
+        break;
+    case AK5702_PLL_MASTER:
+        reg &= (~AK5702_PLL1_MODE_MASK);
+        reg |= AK5702_PLL1_MASTER;
+        reg |= AK5702_PLL1_POWERUP;
+        break;
+    case AK5702_PLL_SLAVE:
+        reg &= (~AK5702_PLL1_MODE_MASK);
+        reg |= AK5702_PLL1_SLAVE;
+        reg |= AK5702_PLL1_POWERUP;
+        break;
+    default:
+        return -ENODEV;
+    }
+
+    switch (freq_in) {
+    case 11289600:
+        reg &= (~AK5702_PLL1_PLL_MASK);
+        reg |= AK5702_PLL1_11289600;
+        break;
+    case 12000000:
+        reg &= (~AK5702_PLL1_PLL_MASK);
+        reg |= AK5702_PLL1_12000000;
+        break;
+    case 12288000:
+        reg &= (~AK5702_PLL1_PLL_MASK);
+        reg |= AK5702_PLL1_12288000;
+        break;
+    case 19200000:
+        reg &= (~AK5702_PLL1_PLL_MASK);
+        reg |= AK5702_PLL1_19200000;
+        break;
+    default:
+        return -ENODEV;
+    }
+
+    ak5702_write(codec, AK5702_PLL1, reg);
+    return 0;
+}
+
+static int ak5702_set_dai_clkdiv(struct snd_soc_dai *codec_dai,
+                    int div_id, int div)
+{
+    struct snd_soc_codec *codec = codec_dai->codec;
+    u8 reg = 0;
+    pr_debug("Entered %s", __func__);
+    if (div_id == AK5702_BCLK_CLKDIV) {
+        reg = ak5702_read_reg_cache(codec, AK5702_FS1);
+        switch (div) {
+        case AK5702_BCLK_DIV_32:
+            reg &= (~AK5702_FS1_BCKO_MASK);
+            reg |= AK5702_FS1_BCKO_32FS;
+            ak5702_write(codec, AK5702_FS1, reg);
+            break;
+        case AK5702_BCLK_DIV_64:
+            reg &= (~AK5702_FS1_BCKO_MASK);
+            reg |= AK5702_FS1_BCKO_64FS;
+            ak5702_write(codec, AK5702_FS1, reg);
+            break;
+        default:
+            return -EINVAL;
+        }
+    }
+    return 0;
+}
+
+#ifdef CONFIG_PM
+static int ak5702_sync(struct snd_soc_codec *codec)
+{
+    u16 *cache = codec->reg_cache;
+    int i, r = 0;
+
+    for (i = 0; i < AK5702_CACHEREGNUM; i++)
+        r |= ak5702_write(codec, i, cache[i]);
+
+    return r;
+}
+
+static int ak5702_suspend(struct snd_soc_codec *codec)
+{
+    return 0;
+}
+
+static int ak5702_resume(struct snd_soc_codec *codec)
+{
+    /* restore register status*/
+    return ak5702_sync(codec);
+}
+#endif
+
+/* define operations */
+struct snd_soc_dai_ops ak5702_ops = {
+    .set_sysclk    = ak5702_set_dai_sysclk,
+    .set_pll    = ak5702_set_dai_pll,
+    .set_clkdiv    = ak5702_set_dai_clkdiv,
+    .set_fmt    = ak5702_set_dai_fmt,
+    .startup    = ak5702_dai_startup,
+    .shutdown    = ak5702_dai_shutdown,
+    .hw_params    = ak5702_dai_hw_params,
+    .hw_free    = ak5702_dai_hw_free,
+};
+
+/* define name and capabilities */
+struct snd_soc_dai_driver ak5702_dai = {
+    .name = "ak5702-hifi",
+    .capture = {
+        .stream_name = "Capture",
+        .channels_min = 1,
+        .channels_max = 8,
+        .rates = SNDRV_PCM_RATE_8000_48000,
+        .formats = SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S32_LE,
+    },
+    .ops = &ak5702_ops,
+};
+
+static struct snd_soc_codec *ak5702_codec;
+
+static int ak5702_probe(struct snd_soc_codec *codec)
+{
+    struct ak5702_priv *ak5702 = snd_soc_codec_get_drvdata(codec);
+    int ret = 0;
+    u8 reg = 0;
+    dev_info(codec->dev, "AK5702 Audio Codec %s", AK5702_VERSION);
+
+    codec->hw_write    = (hw_write_t)i2c_master_send;
+    codec->control_data = ak5702->control_data;
+
+    /* set the clock type */
+    ak5702->clock_mode = EXT_slave_mode;
+
+    /* power off the device */
+    reg = AK5702_POWERDOWN;
+    ak5702_write(codec, AK5702_PM1, reg);
+
+    /* set default ADCA input LIN5-RIN5, power down ADCA mic power*/
+    reg = AK5702_SIG1_L_LIN5 | AK5702_SIG1_R_RIN5;
+    ak5702_write(codec, AK5702_SIG1, reg);
+
+    /* mic gain +0dB */
+    reg = AK5702_MICGAIN_0DB;
+    ak5702_write(codec, AK5702_MICG1, reg);
+
+    /* volume control dependent for ADCA */
+    reg = AK5702_VOLCTRL_DEP;
+    ak5702_write(codec, AK5702_VOLCTRL1, reg);
+
+    /* set default volume for ADCA */
+    reg = AK5702_VOL_INIT;
+    ak5702_write(codec, AK5702_LVOL1, reg);
+
+    /* power down ADCB */
+    reg = AK5702_POWERDOWN;
+    ak5702_write(codec, AK5702_PM2, reg);
+
+    /* set default ADCB input LIN3-RIN3, power off ADCB mic power*/
+    reg = AK5702_SIG2_L_LIN3 | AK5702_SIG2_R_RIN3;
+    ak5702_write(codec, AK5702_SIG2, reg);
+
+    /* mic gain +0dB */
+    reg = AK5702_MICGAIN_0DB;
+    ak5702_write(codec, AK5702_MICG2, reg);
+
+    /* volume control independent for ADCB */
+    reg = AK5702_VOLCTRL_INDEP;
+    ak5702_write(codec, AK5702_VOLCTRL2, reg);
+
+    /* set default volume for ADCB */
+    reg = AK5702_VOL_INIT;
+    ak5702_write(codec, AK5702_LVOL2, reg);
+    ak5702_write(codec, AK5702_RVOL2, reg);
+
+    snd_soc_add_codec_controls(codec, ak5702_snd_controls,
+               ARRAY_SIZE(ak5702_snd_controls));
+
+    /* add debugfs entry */
+    ak5702_debug_add(ak5702);
+
+    return ret;
+}
+
+static int ak5702_remove(struct snd_soc_codec *codec)
+{
+    struct ak5702_priv *ak5702 = snd_soc_codec_get_drvdata(codec);
+    u8 reg = 0;
+
+    /* power off the device */
+    reg = AK5702_POWERDOWN;
+    ak5702_write(codec, AK5702_PM1, reg);
+    ak5702_debug_remove(ak5702);
+
+    return 0;
+}
+
+#if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE)
+static __devinit int ak5702_i2c_probe(struct i2c_client *i2c,
+                const struct i2c_device_id *id)
+{
+    struct ak5702_priv *ak5702;
+    int ret;
+    pr_debug("Entered %s", __func__);
+    if (ak5702_codec) { /*maybe useless*/
+        dev_err(&i2c->dev,
+          "Multiple AK5702 devices not supported\n");
+        return -ENOMEM;
+    }
+
+    ak5702 = kzalloc(sizeof(struct ak5702_priv), GFP_KERNEL);
+    if (!ak5702)
+        return -ENOMEM;
+
+    i2c_set_clientdata(i2c, ak5702);
+    ak5702->control_data = i2c;
+    ak5702->control_type = SND_SOC_I2C;
+
+    ret = snd_soc_register_codec(&i2c->dev,
+                &soc_codec_dev_ak5702, &ak5702_dai, 1);
+    if (ret < 0) {
+        kfree(ak5702);
+        return ret;
+    }
+    return ret;
+}
+
+static __devexit int ak5702_i2c_remove(struct i2c_client *client)
+{
+    snd_soc_unregister_codec(&client->dev);
+    kfree(i2c_get_clientdata(client));
+    return 0;
+}
+
+static const struct i2c_device_id ak5702_i2c_id[] = {
+    {"ak5702-codec", 0},
+    {}
+};
+
+MODULE_DEVICE_TABLE(i2c, ak5702_i2c_id);
+
+static struct i2c_driver ak5702_i2c_driver = {
+    .driver = {
+        .name = "ak5702-codec",
+        .owner = THIS_MODULE,
+    },
+    .probe = ak5702_i2c_probe,
+    .remove = __devexit_p(ak5702_i2c_remove),
+    .id_table = ak5702_i2c_id,
+};
+#endif
+
+struct snd_soc_codec_driver soc_codec_dev_ak5702 = {
+    .probe            = ak5702_probe,
+    .remove            = ak5702_remove,
+#ifdef CONFIG_PM
+    .suspend        = ak5702_suspend,
+    .resume            = ak5702_resume,
+#endif
+    .read            = ak5702_read_reg_cache,
+    .write            = ak5702_write,
+    .reg_cache_size        = ARRAY_SIZE(ak5702_reg) ,
+    .reg_word_size        = sizeof(u16),
+    .reg_cache_default    = ak5702_reg,
+};
+
+static int __init ak5702_modinit(void)
+{
+    int ret = 0;
+#if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE)
+    ret = i2c_add_driver(&ak5702_i2c_driver);
+#endif
+    return ret;
+}
+module_init(ak5702_modinit);
+
+static void __exit ak5702_exit(void)
+{
+#if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE)
+    i2c_del_driver(&ak5702_i2c_driver);
+#endif
+}
+module_exit(ak5702_exit);
+
+MODULE_DESCRIPTION("Asahi Kasei AK5702 ALSA SoC driver");
+MODULE_AUTHOR("Paolo Doz <paolo.doz@gmail.com");
+MODULE_LICENSE("GPL");
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/4] ASoC: ak5702: add support for ak5702 -- 4ch ADC
  2012-10-29  8:43 [PATCH 1/4] ASoC: ak5702: add support for ak5702 -- 4ch ADC Paolo Doz
@ 2012-10-29 12:41 ` Lars-Peter Clausen
  2012-10-29 16:04 ` Mark Brown
  1 sibling, 0 replies; 7+ messages in thread
From: Lars-Peter Clausen @ 2012-10-29 12:41 UTC (permalink / raw)
  To: Paolo Doz; +Cc: alsa-devel, broonie, lrg

Hi,

On 10/29/2012 09:43 AM, Paolo Doz wrote:
> This codec driver adds support for Asahi Kasei AK5702.
> 
> Signed-off-by: Paolo Doz <paolo.doz@gmail.com>

Your mail client seems to have messed up the patches. Tabs replaces with
spaces, inserted a few extra newlines, etc.

Also, there is not really a need to have this split out over 4 commits. In
fact it makes things harder to review.


> ---
>  sound/soc/codecs/ak5702.c |  728
> +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 728 insertions(+)
>  create mode 100644 sound/soc/codecs/ak5702.c
> 
> diff --git a/sound/soc/codecs/ak5702.c b/sound/soc/codecs/ak5702.c
> new file mode 100644
> index 0000000..47c2e52
> --- /dev/null
> +++ b/sound/soc/codecs/ak5702.c
> @@ -0,0 +1,728 @@
> +/*
> + * ak5702.c  --  AK5702 ALSA SoC driver
> + *
> + * Author:      Paolo Doz <paolo.doz@gmail.com>
> + * Copyright:   (C) 2012 Soft In S.r.l.
> + *                  2009 Freescale Semiconductor, Inc. All rights reserved.
> + *
> + * 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.
> + *
> + * This code comes from the original Freescale Ak5702 Audio driver
> + */
> +
> +/* #define DEBUG 1 */
> +
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/init.h>
> +#include <linux/delay.h>
> +#include <linux/pm.h>
> +#include <linux/i2c.h>
> +#include <linux/platform_device.h>
> +#include <sound/core.h>
> +#include <sound/pcm.h>
> +#include <sound/pcm_params.h>
> +#include <sound/soc.h>
> +#include <sound/soc-dapm.h>
> +#include <sound/initval.h>
> +#include <linux/slab.h>
> +#include <linux/debugfs.h>
> +
> +#include <sound/ak5702.h>
> +#include "ak5702.h"
> +
> +#define AK5702_VERSION "0.2"
> +#define DRV_NAME "ak5702"
> +
> +/* codec private data */
> +struct ak5702_priv {
> +    enum snd_soc_control_type control_type;
> +    void *control_data;
> +    unsigned int sysclk;
> +    enum ak5702_clock_mode clock_mode;
> +    struct dentry *debugfs;
> +};
> +
> +/*
> + * ak5702 register cache
> + */
> +static u16 ak5702_reg[AK5702_CACHEREGNUM] = {
> +    0x00, 0x24, 0x00, 0x01, 0x23, 0x1f,
> +    0x00, 0x01, 0x91, 0x00, 0xe1, 0x00,
> +    0xa0, 0x00, 0x00, 0x00, 0x01, 0x20,
> +    0x00, 0x00, 0x01, 0x91, 0x00, 0xe1,
> +    0x00,
> +};
> +
> +/*
> + * read ak5702 register cache
> + */
> +static inline unsigned int ak5702_read_reg_cache(struct snd_soc_codec
> *codec,
> +                unsigned int reg)
> +{
> +    u16 *cache = codec->reg_cache;
> +
> +    if (reg >= AK5702_CACHEREGNUM)
> +        return -1;
> +    return cache[reg];
> +}
> +
> +/*
> + * write ak5702 register cache
> + */
> +static inline void ak5702_write_reg_cache(struct snd_soc_codec *codec,
> +                u16 reg, unsigned int value)
> +{
> +    u16 *cache = codec->reg_cache;
> +
> +    if (reg >= AK5702_CACHEREGNUM)
> +        return;
> +    cache[reg] = value;
> +    ak5702_reg[reg] = value;
> +}
> +
> +/*
> + * write to the AK5702 register space
> + */
> +static int ak5702_write(struct snd_soc_codec *codec, unsigned int reg,
> +                unsigned int value)
> +{
> +    u8 data[2];
> +    data[0] = reg & 0xff;
> +    data[1] = value & 0xff;
> +
> +    if (codec->hw_write(codec->control_data, data, 2) == 2) {
> +        pr_debug("I2c write success @ reg = %x, val = %x\n",
> +             data[0], data[1]);
> +        ak5702_write_reg_cache(codec, reg, value);
> +        return 0;
> +    } else {
> +        pr_debug("I2c write error reg = %x, val = %x\n",
> +            data[0], data[1]);
> +        return -EIO;
> +    }
> +}
> +
> +#ifdef CONFIG_DEBUG_FS
> +static int ak5702_show(struct seq_file *s, void *unused)
> +{
> +#define REG(r) { r, #r }
> +    static const struct {
> +        int offset;
> +        const char *name;
> +    } regs[] = {
> +        REG(AK5702_PM1),
> +        REG(AK5702_PLL1),
> +        REG(AK5702_SIG1),
> +        REG(AK5702_MICG1),
> +        REG(AK5702_FMT1),
> +        REG(AK5702_FS1),
> +        REG(AK5702_CLK1),
> +        REG(AK5702_VOLCTRL1),
> +        REG(AK5702_LVOL1),
> +        REG(AK5702_RVOL1),
> +        REG(AK5702_TIMER1),
> +        REG(AK5702_ALC11),
> +        REG(AK5702_ALC12),
> +        REG(AK5702_MODE11),
> +        REG(AK5702_MODE12),
> +        REG(AK5702_MODE13),
> +
> +        REG(AK5702_PM2),
> +        REG(AK5702_PLL2),
> +        REG(AK5702_SIG2),
> +        REG(AK5702_MICG2),
> +        REG(AK5702_FMT2),
> +        REG(AK5702_FS2),
> +        REG(AK5702_CLK2),
> +        REG(AK5702_VOLCTRL2),
> +        REG(AK5702_LVOL2),
> +        REG(AK5702_RVOL2),
> +        REG(AK5702_TIMER2),
> +        REG(AK5702_ALC21),
> +        REG(AK5702_ALC22),
> +        REG(AK5702_MODE21),
> +        REG(AK5702_MODE22),
> +    };
> +#undef REG
> +
> +    int i;
> +    for (i = 0; i < ARRAY_SIZE(ak5702_reg); i++) {
> +        seq_printf(s, "%s = 0x%02x\n", regs[i].name,
> +                        ak5702_reg[regs[i].offset]);
> +    }
> +
> +    return 0;
> +}
> +
> +static int ak5702_debug_open(struct inode *inode, struct file *file)
> +{
> +    return single_open(file, ak5702_show, inode->i_private);
> +}
> +
> +static const struct file_operations ak5702_debug_fops = {
> +    .open    = ak5702_debug_open,
> +    .read    = seq_read,
> +    .llseek  = seq_lseek,
> +    .release = single_release,
> +};
> +
> +static void ak5702_debug_add(struct ak5702_priv *ak5702)
> +{
> +    char name[] = DRV_NAME ".0";
> +
> +    snprintf(name, sizeof(name), DRV_NAME);
> +    ak5702->debugfs = debugfs_create_file(name, S_IRUGO,
> +            snd_soc_debugfs_root, ak5702, &ak5702_debug_fops);
> +}
> +
> +static void ak5702_debug_remove(struct ak5702_priv *ak5702)
> +{
> +    if (ak5702->debugfs)
> +        debugfs_remove(ak5702->debugfs);
> +}
> +#else
> +static inline void ak5702_debug_add(struct ak5702_priv *ak5702)
> +{
> +}
> +
> +static inline void ak5702_debug_remove(struct ak5702_priv *ak5702)
> +{
> +}
> +#endif


You should really use regmap for register IO and register debugfs.

> +
> +static const char *ak5702_mic_gain[] = { "0dB", "+15dB", "+30dB", "+
36dB"
> };

Uhm... no. Use a proper volume control with a TLV, instead of enum.

> +static const char *ak5702_adc_input_type[] = {
> +    "Single-ended", "Full-differential" };

Is this really something that should be runtime configurable. I'd image this
to depend on the board setup, so it should go into platform data

> +static const char *ak5702_adca_left_input[] = { "LIN1", "LIN2" };
> +static const char *ak5702_adca_right_input[] = { "RIN1", "RIN2" };
> +static const char *ak5702_adca_left5[] = { "LIN1-2", "LIN5" };
> +static const char *ak5702_adca_right5[] = { "RIN1-2", "RIN5" };
> +
> +static const char *ak5702_adcb_left_input[] = { "LIN3", "LIN4" };
> +static const char *ak5702_adcb_right_input[] = { "RIN3", "RIN4" };
> +static const char *ak5702_adcb_left5[] = { "LIN3-4", "LIN5" };
> +static const char *ak5702_adcb_right5[] = { "RIN3-4", "RIN5" };

These all look as if they should be implemented as DAPM Muxes as they
contain routing information.

> +
> +static const char *ak5702_input_vol_control[] = { "Independent",
> "Dependent" };

Not a 100% what this does, but should this really be runtime configurable?

> +static const char *ak5702_mic_power[] = { "Off", "On" };

Power management should be done by DAPM

> +
> +static const struct soc_enum ak5702_enum[] = {

No array here please, just makes things pretty hard to follow and easily
opens up a path for errors, when you access the array like ak5702_enum[x].


> +    SOC_ENUM_SINGLE(AK5702_MICG1, 0, 4, ak5702_mic_gain),
> +    SOC_ENUM_SINGLE(AK5702_MICG2, 0, 4, ak5702_mic_gain),
> +
> +    SOC_ENUM_SINGLE(AK5702_SIG1, 0, 2, ak5702_adca_left_input),
> +    SOC_ENUM_SINGLE(AK5702_SIG1, 1, 2, ak5702_adca_right_input),
> +    SOC_ENUM_SINGLE(AK5702_SIG2, 0, 2, ak5702_adcb_left_input),
> +    SOC_ENUM_SINGLE(AK5702_SIG2, 1, 2, ak5702_adcb_right_input),
> +
> +    SOC_ENUM_SINGLE(AK5702_SIG1, 2, 2, ak5702_adc_input_type),
> +    SOC_ENUM_SINGLE(AK5702_SIG1, 3, 2, ak5702_adc_input_type),
> +    SOC_ENUM_SINGLE(AK5702_SIG2, 2, 2, ak5702_adc_input_type),
> +    SOC_ENUM_SINGLE(AK5702_SIG2, 3, 2, ak5702_adc_input_type),
> +
> +    SOC_ENUM_SINGLE(AK5702_VOLCTRL1, 0, 2, ak5702_input_vol_control),
> +    SOC_ENUM_SINGLE(AK5702_VOLCTRL2, 0, 2, ak5702_input_vol_control),
> +
> +    SOC_ENUM_SINGLE(AK5702_SIG1, 5, 2, ak5702_adca_left5),
> +    SOC_ENUM_SINGLE(AK5702_SIG1, 6, 2, ak5702_adca_right5),
> +    SOC_ENUM_SINGLE(AK5702_SIG2, 5, 2, ak5702_adcb_left5),
> +    SOC_ENUM_SINGLE(AK5702_SIG2, 6, 2, ak5702_adcb_right5),
> +
> +    SOC_ENUM_SINGLE(AK5702_SIG1, 4, 2, ak5702_mic_power),
> +    SOC_ENUM_SINGLE(AK5702_SIG2, 4, 2, ak5702_mic_power),
> +};
> +
> +static const struct snd_kcontrol_new ak5702_snd_controls[] = {
> +    SOC_SINGLE("ADCA Left Vol", AK5702_LVOL1, 0, 242, 0),
> +    SOC_SINGLE("ADCA Right Vol", AK5702_RVOL1, 0, 242, 0),
> +    SOC_SINGLE("ADCB Left Vol", AK5702_LVOL2, 0, 242, 0),
> +    SOC_SINGLE("ADCB Right Vol", AK5702_RVOL2, 0, 242, 0),

If this is known please provide TLV dB informations for the controls.
Also please stick so standard ALSA control namings. Volume controls should
end with "Volume".

> +
> +    SOC_ENUM("MIC-AmpA Gain", ak5702_enum[0]),
> +    SOC_ENUM("MIC-AmpB Gain", ak5702_enum[1]),
> +
> +    SOC_ENUM("ADCA Left Source", ak5702_enum[2]),
> +    SOC_ENUM("ADCA Right Source", ak5702_enum[3]),
> +    SOC_ENUM("ADCB Left Source", ak5702_enum[4]),
> +    SOC_ENUM("ADCB Right Source", ak5702_enum[5]),
> +
> +    SOC_ENUM("ADCA Left Type", ak5702_enum[6]),
> +    SOC_ENUM("ADCA Right Type", ak5702_enum[7]),
> +    SOC_ENUM("ADCB Left Type", ak5702_enum[8]),
> +    SOC_ENUM("ADCB Right Type", ak5702_enum[9]),
> +
> +    SOC_ENUM("ADCA Input Vol mode", ak5702_enum[10]),
> +    SOC_ENUM("ADCB Input Vol mode", ak5702_enum[11]),
> +
> +    SOC_ENUM("ADCA LIN5 Source", ak5702_enum[12]),
> +    SOC_ENUM("ADCA RIN5 Source", ak5702_enum[13]),
> +    SOC_ENUM("ADCB LIN5 Source", ak5702_enum[14]),
> +    SOC_ENUM("ADCB RIN5 Source", ak5702_enum[15]),
> +
> +    SOC_ENUM("MIC-A Power", ak5702_enum[16]),
> +    SOC_ENUM("MIC-B Power", ak5702_enum[17]),
> +};
> +
> +/* ak5702 dapm widgets */
> +static const struct snd_soc_dapm_widget ak5702_dapm_widgets[] = {
> +    SND_SOC_DAPM_ADC("ADCA Left", "Capture", AK5702_PM1, 0, 0),
> +    SND_SOC_DAPM_ADC("ADCA Right", "Capture", AK5702_PM1, 1, 0),
> +    SND_SOC_DAPM_ADC("ADCB Left", "Capture", AK5702_PM2, 0, 0),
> +    SND_SOC_DAPM_ADC("ADCB Right", "Capture", AK5702_PM2, 1, 0),
> +
> +    SND_SOC_DAPM_INPUT("ADCA Left Input"),
> +    SND_SOC_DAPM_INPUT("ADCA Right Input"),
> +    SND_SOC_DAPM_INPUT("ADCB Left Input"),
> +    SND_SOC_DAPM_INPUT("ADCB Right Input"),
> +};
> +
> +static const struct snd_soc_dapm_route audio_map[] = {

I'd call this ak5702_dapm_routes

> +    {"ADCA Left", NULL, "ADCA Left Input"},
> +    {"ADCA Right", NULL, "ADCA Right Input"},
> +    {"ADCB Left", NULL, "ADCB Left Input"},
> +    {"ADCB Right", NULL, "ADCB Right Input"},
> +};
> +
> +static int ak5702_dai_startup(struct snd_pcm_substream *substream,
> +                struct snd_soc_dai *dai)
> +{
> +    pr_debug("Entered %s\n", __func__);
> +    return 0;
> +}
> +
> +static void ak5702_dai_shutdown(struct snd_pcm_substream *substream,
> +                struct snd_soc_dai *dai)
> +{
> +    pr_debug("Entered %s\n", __func__);
> +}

No need to implement these, if they do nothing.

[...]
> +static int ak5702_dai_hw_free(struct snd_pcm_substream *substream,
> +                struct snd_soc_dai *dai)
> +{
> +    struct snd_soc_codec *codec = dai->codec;
> +    int value;
> +
> +    pr_debug("Entered %s\n", __func__);
> +    /* power down ADCA */
> +    value = AK5702_POWERDOWN | AK5702_PM1_PMVCM;
> +    ak5702_write(codec, AK5702_PM1, value);
> +
> +    /* power down ADCB */
> +    value = AK5702_POWERDOWN;
> +    ak5702_write(codec, AK5702_PM2, value);

This looks all like power management, this should either be handeld by DAPM
controls directly or via the set_bias_level callback

> +    return 0;
> +}
> +
> +static int ak5702_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 ak5702_priv *ak5702 = snd_soc_codec_get_drvdata(codec);
> +    int value = 0;
> +    switch (clk_id) {
> +    case PLL_master_mode:
> +    case EXT_master_mode:

please all upper case for the defines. Also they need a driver specific
prefix. Also slave and master mode should be selected via the set_dai_fmt
callback.

> +        if (dir != SND_SOC_CLOCK_OUT)
> +            return -EINVAL;
> +        break;
> +    case PLL_slave_mode:
> +    case PLL_slave_mode2:
> +        if (dir != SND_SOC_CLOCK_IN)
> +            return -EINVAL;
> +        value = AK5702_PLL1_POWERUP |
> +            AK5702_PLL1_SLAVE |
> +            AK5702_PLL1_MODE2;
> +        break;
> +    case EXT_slave_mode:
> +        if (dir != SND_SOC_CLOCK_IN)
> +            return -EINVAL;
> +        value = AK5702_PLL1_POWERDOWN | AK5702_PLL1_SLAVE;
> +        break;
> +    default:
> +        return -EINVAL;
> +    }
> +    ak5702_write(codec, AK5702_PLL1, value);
> +    ak5702->clock_mode = clk_id;
> +
> +    return 0;
> +}
> +
[...]
> +}
> +
> +#ifdef CONFIG_PM
> +static int ak5702_sync(struct snd_soc_codec *codec)
> +{
> +    u16 *cache = codec->reg_cache;
> +    int i, r = 0;
> +
> +    for (i = 0; i < AK5702_CACHEREGNUM; i++)
> +        r |= ak5702_write(codec, i, cache[i]);
> +
> +    return r;
> +}
> +
> +static int ak5702_suspend(struct snd_soc_codec *codec)
> +{
> +    return 0;
> +}
> +
> +static int ak5702_resume(struct snd_soc_codec *codec)
> +{
> +    /* restore register status*/
> +    return ak5702_sync(codec);
> +}
> +#endif
> +
> +/* define operations */
> +struct snd_soc_dai_ops ak5702_ops = {

static const

> +    .set_sysclk    = ak5702_set_dai_sysclk,
> +    .set_pll    = ak5702_set_dai_pll,
> +    .set_clkdiv    = ak5702_set_dai_clkdiv,
> +    .set_fmt    = ak5702_set_dai_fmt,
> +    .startup    = ak5702_dai_startup,
> +    .shutdown    = ak5702_dai_shutdown,
> +    .hw_params    = ak5702_dai_hw_params,
> +    .hw_free    = ak5702_dai_hw_free,
> +};
> +
> +/* define name and capabilities */
> +struct snd_soc_dai_driver ak5702_dai = {

static

> +    .name = "ak5702-hifi",
> +    .capture = {
> +        .stream_name = "Capture",
> +        .channels_min = 1,
> +        .channels_max = 8,
> +        .rates = SNDRV_PCM_RATE_8000_48000,
> +        .formats = SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S32_LE,
> +    },
> +    .ops = &ak5702_ops,
> +};
> +
> +static struct snd_soc_codec *ak5702_codec;

Just move the definition of ak5702_codec here.

> +
> +static int ak5702_probe(struct snd_soc_codec *codec)
> +{
> +    struct ak5702_priv *ak5702 = snd_soc_codec_get_drvdata(codec);
> +    int ret = 0;
> +    u8 reg = 0;
> +    dev_info(codec->dev, "AK5702 Audio Codec %s", AK5702_VERSION);
> +
> +    codec->hw_write    = (hw_write_t)i2c_master_send;
> +    codec->control_data = ak5702->control_data;
> +
> +    /* set the clock type */
> +    ak5702->clock_mode = EXT_slave_mode;
> +
> +    /* power off the device */
> +    reg = AK5702_POWERDOWN;
> +    ak5702_write(codec, AK5702_PM1, reg);
> +
> +    /* set default ADCA input LIN5-RIN5, power down ADCA mic power*/
> +    reg = AK5702_SIG1_L_LIN5 | AK5702_SIG1_R_RIN5;
> +    ak5702_write(codec, AK5702_SIG1, reg);
> +
> +    /* mic gain +0dB */
> +    reg = AK5702_MICGAIN_0DB;
> +    ak5702_write(codec, AK5702_MICG1, reg);
> +
> +    /* volume control dependent for ADCA */
> +    reg = AK5702_VOLCTRL_DEP;
> +    ak5702_write(codec, AK5702_VOLCTRL1, reg);
> +
> +    /* set default volume for ADCA */
> +    reg = AK5702_VOL_INIT;
> +    ak5702_write(codec, AK5702_LVOL1, reg);
> +
> +    /* power down ADCB */
> +    reg = AK5702_POWERDOWN;
> +    ak5702_write(codec, AK5702_PM2, reg);
> +
> +    /* set default ADCB input LIN3-RIN3, power off ADCB mic power*/
> +    reg = AK5702_SIG2_L_LIN3 | AK5702_SIG2_R_RIN3;
> +    ak5702_write(codec, AK5702_SIG2, reg);
> +
> +    /* mic gain +0dB */
> +    reg = AK5702_MICGAIN_0DB;
> +    ak5702_write(codec, AK5702_MICG2, reg);
> +
> +    /* volume control independent for ADCB */
> +    reg = AK5702_VOLCTRL_INDEP;
> +    ak5702_write(codec, AK5702_VOLCTRL2, reg);
> +
> +    /* set default volume for ADCB */
> +    reg = AK5702_VOL_INIT;
> +    ak5702_write(codec, AK5702_LVOL2, reg);
> +    ak5702_write(codec, AK5702_RVOL2, reg);

There should be no need initialize all these registers, just leave them at
their reset default value and let userspace select whatever they need for
their mode of operation.

> +
> +    snd_soc_add_codec_controls(codec, ak5702_snd_controls,
> +               ARRAY_SIZE(ak5702_snd_controls));

Use the .controls/.num_controls field of the snd_soc_codec_driver struct to
get them registered instead of doing this here manually. Same goes for your
DAPM widgets and routes, which you don't seem to register at all at the moment.

> +
> +    /* add debugfs entry */
> +    ak5702_debug_add(ak5702);
> +
> +    return ret;
> +}
> +
> +static int ak5702_remove(struct snd_soc_codec *codec)
> +{
> +    struct ak5702_priv *ak5702 = snd_soc_codec_get_drvdata(codec);
> +    u8 reg = 0;
> +
> +    /* power off the device */
> +    reg = AK5702_POWERDOWN;
> +    ak5702_write(codec, AK5702_PM1, reg);
> +    ak5702_debug_remove(ak5702);
> +
> +    return 0;
> +}
> +
> +#if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE)
> +static __devinit int ak5702_i2c_probe(struct i2c_client *i2c,
> +                const struct i2c_device_id *id)
> +{
> +    struct ak5702_priv *ak5702;
> +    int ret;
> +    pr_debug("Entered %s", __func__);
> +    if (ak5702_codec) { /*maybe useless*/
> +        dev_err(&i2c->dev,
> +          "Multiple AK5702 devices not supported\n");
> +        return -ENOMEM;
> +    }
> +
> +    ak5702 = kzalloc(sizeof(struct ak5702_priv), GFP_KERNEL);

devm_kzalloc. Using devm_kzalloc means the memory will automatically be
freed when the device is removed, so you don't have to do this by hand.

> +    if (!ak5702)
> +        return -ENOMEM;
> +
> +    i2c_set_clientdata(i2c, ak5702);
> +    ak5702->control_data = i2c;
> +    ak5702->control_type = SND_SOC_I2C;
> +
> +    ret = snd_soc_register_codec(&i2c->dev,
> +                &soc_codec_dev_ak5702, &ak5702_dai, 1);
> +    if (ret < 0) {
> +        kfree(ak5702);
> +        return ret;
> +    }
> +    return ret;
> +}
> +
> +static __devexit int ak5702_i2c_remove(struct i2c_client *client)
> +{
> +    snd_soc_unregister_codec(&client->dev);
> +    kfree(i2c_get_clientdata(client));
> +    return 0;
> +}
> +
> +static const struct i2c_device_id ak5702_i2c_id[] = {
> +    {"ak5702-codec", 0},
> +    {}
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, ak5702_i2c_id);
> +
> +static struct i2c_driver ak5702_i2c_driver = {
> +    .driver = {
> +        .name = "ak5702-codec",
> +        .owner = THIS_MODULE,
> +    },
> +    .probe = ak5702_i2c_probe,
> +    .remove = __devexit_p(ak5702_i2c_remove),
> +    .id_table = ak5702_i2c_id,
> +};
> +#endif
> +
> +struct snd_soc_codec_driver soc_codec_dev_ak5702 = {

static

> +    .probe            = ak5702_probe,
> +    .remove            = ak5702_remove,
> +#ifdef CONFIG_PM
> +    .suspend        = ak5702_suspend,
> +    .resume            = ak5702_resume,
> +#endif
> +    .read            = ak5702_read_reg_cache,
> +    .write            = ak5702_write,
> +    .reg_cache_size        = ARRAY_SIZE(ak5702_reg) ,
> +    .reg_word_size        = sizeof(u16),
> +    .reg_cache_default    = ak5702_reg,
> +};
> +
> +static int __init ak5702_modinit(void)
> +{
> +    int ret = 0;
> +#if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE)

So if CONFIG_I2C is not selected the module does nothing?

> +    ret = i2c_add_driver(&ak5702_i2c_driver);
> +#endif
> +    return ret;
> +}
> +module_init(ak5702_modinit);
> +
> +static void __exit ak5702_exit(void)
> +{
> +#if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE)
> +    i2c_del_driver(&ak5702_i2c_driver);
> +#endif
> +}
> +module_exit(ak5702_exit);

The hole module init/exit functions can just be replaced with
module_i2c_driver(ak5702_i2c_driver);

> +
> +MODULE_DESCRIPTION("Asahi Kasei AK5702 ALSA SoC driver");
> +MODULE_AUTHOR("Paolo Doz <paolo.doz@gmail.com");
> +MODULE_LICENSE("GPL");

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/4] ASoC: ak5702: add support for ak5702 -- 4ch ADC
  2012-10-29  8:43 [PATCH 1/4] ASoC: ak5702: add support for ak5702 -- 4ch ADC Paolo Doz
  2012-10-29 12:41 ` Lars-Peter Clausen
@ 2012-10-29 16:04 ` Mark Brown
  2012-10-29 17:49   ` Paolo Doz
  1 sibling, 1 reply; 7+ messages in thread
From: Mark Brown @ 2012-10-29 16:04 UTC (permalink / raw)
  To: Paolo Doz; +Cc: alsa-devel, lrg


[-- Attachment #1.1: Type: text/plain, Size: 541 bytes --]

On Mon, Oct 29, 2012 at 09:43:57AM +0100, Paolo Doz wrote:
> This codec driver adds support for Asahi Kasei AK5702.
> 
> Signed-off-by: Paolo Doz <paolo.doz@gmail.com>

Lars-Peter already went through most of this so I'll not follow up in
detail.  One overall comment which Lars-Peter didn't make but which it's
worth bearing in mind was that your driver looks like it was written
several years ago, it'd probably help a lot if you were to look at more
modern drivers and make sure what you're doing resembles them
stylistically.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/4] ASoC: ak5702: add support for ak5702 -- 4ch ADC
  2012-10-29 16:04 ` Mark Brown
@ 2012-10-29 17:49   ` Paolo Doz
  2012-10-29 18:17     ` Mark Brown
  0 siblings, 1 reply; 7+ messages in thread
From: Paolo Doz @ 2012-10-29 17:49 UTC (permalink / raw)
  To: Mark Brown; +Cc: alsa-devel, lrg

Hi,
@Lars-Peter: thanks for the code review
@Mark: You're right. The original source code (that I found on the web)
was for kernel 2.6.35, and then I adapted it to work with kernel 3.2 series.
Can you suggest me a "state of the art" codec driver where can
I have a look at best practice?
In the end the codec should work with alsa infrastructure available in
kernel 3.2 so I can test it's correctness directly on my hw.

Thanks in advance,
Paolo

On Mon, Oct 29, 2012 at 5:04 PM, Mark Brown <
broonie@opensource.wolfsonmicro.com> wrote:

> On Mon, Oct 29, 2012 at 09:43:57AM +0100, Paolo Doz wrote:
> > This codec driver adds support for Asahi Kasei AK5702.
> >
> > Signed-off-by: Paolo Doz <paolo.doz@gmail.com>
>
> Lars-Peter already went through most of this so I'll not follow up in
> detail.  One overall comment which Lars-Peter didn't make but which it's
> worth bearing in mind was that your driver looks like it was written
> several years ago, it'd probably help a lot if you were to look at more
> modern drivers and make sure what you're doing resembles them
> stylistically.
>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/4] ASoC: ak5702: add support for ak5702 -- 4ch ADC
  2012-10-29 17:49   ` Paolo Doz
@ 2012-10-29 18:17     ` Mark Brown
  2012-11-26 16:14       ` Paolo Doz
  0 siblings, 1 reply; 7+ messages in thread
From: Mark Brown @ 2012-10-29 18:17 UTC (permalink / raw)
  To: Paolo Doz; +Cc: alsa-devel, lrg


[-- Attachment #1.1: Type: text/plain, Size: 720 bytes --]

On Mon, Oct 29, 2012 at 06:49:18PM +0100, Paolo Doz wrote:

Please don't top post.

> @Lars-Peter: thanks for the code review
> @Mark: You're right. The original source code (that I found on the web)
> was for kernel 2.6.35, and then I adapted it to work with kernel 3.2 series.
> Can you suggest me a "state of the art" codec driver where can
> I have a look at best practice?

Anything that's been getting work recently...  wm8731 is probably
similar to what you have there.

> In the end the codec should work with alsa infrastructure available in
> kernel 3.2 so I can test it's correctness directly on my hw.

That's a *very* old kernel now, any mainline submissions should be
against a current development kernel.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/4] ASoC: ak5702: add support for ak5702 -- 4ch ADC
  2012-10-29 18:17     ` Mark Brown
@ 2012-11-26 16:14       ` Paolo Doz
  2012-11-26 16:36         ` Mark Brown
  0 siblings, 1 reply; 7+ messages in thread
From: Paolo Doz @ 2012-11-26 16:14 UTC (permalink / raw)
  To: Mark Brown; +Cc: alsa-devel, lrg

On Mon, Oct 29, 2012 at 7:17 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
>
> On Mon, Oct 29, 2012 at 06:49:18PM +0100, Paolo Doz wrote:
>
> Please don't top post.
>
> > @Lars-Peter: thanks for the code review
> > @Mark: You're right. The original source code (that I found on the web)
> > was for kernel 2.6.35, and then I adapted it to work with kernel 3.2 series.
> > Can you suggest me a "state of the art" codec driver where can
> > I have a look at best practice?
>
> Anything that's been getting work recently...  wm8731 is probably
> similar to what you have there.
>
> > In the end the codec should work with alsa infrastructure available in
> > kernel 3.2 so I can test it's correctness directly on my hw.
>
> That's a *very* old kernel now, any mainline submissions should be
> against a current development kernel.


Hi,
I'm rewriting the ak5702 codec driver following you suggestions.
>From you space should be possible to select input source. The codec
has 2 ADC with 2 differents inputs and another one shared by both
(input5)

Inside the codec the input selection is made in the following way

8 bit register (ie ADC A)
bit 0 -> select left1 or left2
bit 1 -> select right1 or right2

bit2/3/4 other settings

bit5 -> if 1 select left5 otherwise select (left1 or left2) depending on bit0
bit6 -> if 1 select right5 otherwise select (right1 or rigth2) depending on bit1

Is there any SOC_ENUM that can be used to represent this situation? I
mean something like "Left1, Left2, Left5" ?

At the moment I have one SOC_ENUM to select "Left1, Left2" and another
for "Left1/2, Left5"

Thanks,
Paolo

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/4] ASoC: ak5702: add support for ak5702 -- 4ch ADC
  2012-11-26 16:14       ` Paolo Doz
@ 2012-11-26 16:36         ` Mark Brown
  0 siblings, 0 replies; 7+ messages in thread
From: Mark Brown @ 2012-11-26 16:36 UTC (permalink / raw)
  To: Paolo Doz; +Cc: alsa-devel, lrg


[-- Attachment #1.1: Type: text/plain, Size: 695 bytes --]

On Mon, Nov 26, 2012 at 05:14:58PM +0100, Paolo Doz wrote:

> 8 bit register (ie ADC A)
> bit 0 -> select left1 or left2
> bit 1 -> select right1 or right2

> bit2/3/4 other settings

> bit5 -> if 1 select left5 otherwise select (left1 or left2) depending on bit0
> bit6 -> if 1 select right5 otherwise select (right1 or rigth2) depending on bit1

> Is there any SOC_ENUM that can be used to represent this situation? I
> mean something like "Left1, Left2, Left5" ?

> At the moment I have one SOC_ENUM to select "Left1, Left2" and another
> for "Left1/2, Left5"

That's pretty special and creative, you'd have to open code it otherwise
the structure you've got sounds about as good as it gets.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]



^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2012-11-26 16:36 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-10-29  8:43 [PATCH 1/4] ASoC: ak5702: add support for ak5702 -- 4ch ADC Paolo Doz
2012-10-29 12:41 ` Lars-Peter Clausen
2012-10-29 16:04 ` Mark Brown
2012-10-29 17:49   ` Paolo Doz
2012-10-29 18:17     ` Mark Brown
2012-11-26 16:14       ` Paolo Doz
2012-11-26 16:36         ` Mark Brown

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.