Re: [PATCH] ASoC: Add support for NAU8825 codec to ASoC
From: Lars-Peter Clausen
Date: Fri Apr 10 2015 - 04:38:28 EST
On 04/10/2015 09:21 AM, Chih-Chiang Chang wrote:
The NAU88L25 is an ultra-low power high performance audio codec designed
for smartphone, tablet PC, and other portable devices by Nuvoton, now
add linux driver support for it.
Signed-off-by: Chih-Chiang Chang <ccchang12@xxxxxxxxxxx>
---
include/sound/nau8825_plat.h | 22 ++
sound/soc/codecs/Kconfig | 5 +
sound/soc/codecs/Makefile | 2 +
sound/soc/codecs/nau8825.c | 593 +++++++++++++++++++++++++++++++++++++++++++
sound/soc/codecs/nau8825.h | 150 +++++++++++
5 files changed, 772 insertions(+)
create mode 100644 include/sound/nau8825_plat.h
create mode 100644 sound/soc/codecs/nau8825.c
create mode 100644 sound/soc/codecs/nau8825.h
diff --git a/include/sound/nau8825_plat.h b/include/sound/nau8825_plat.h
new file mode 100644
index 0000000..87afcd3
--- /dev/null
+++ b/include/sound/nau8825_plat.h
the preferred place for platform_data files is include/linux/platform_data/,
but the driver doesn't seem to use the platform data anyway, so maybe drop it?
[...]
diff --git a/sound/soc/codecs/nau8825.c b/sound/soc/codecs/nau8825.c
new file mode 100644
index 0000000..a8c8f59
--- /dev/null
+++ b/sound/soc/codecs/nau8825.c
@@ -0,0 +1,593 @@
+/*
+ * linux/sound/soc/codecs/nau8825.c
No need to include the file path in the header of the file, this will just
become outdated if the file is ever moved.
[...]
+static int nau8825_hw_params(struct snd_pcm_substream *substream,
+ struct snd_pcm_hw_params *params,
+ struct snd_soc_dai *dai);
+static int nau8825_set_dai_fmt(struct snd_soc_dai *codec_dai,
+ unsigned int fmt);
+static int nau8825_set_bias_level(struct snd_soc_codec *codec,
+ enum snd_soc_bias_level level);
+static int nau8825_dai_set_sysclk(struct snd_soc_dai *dai, int clk_id,
+ unsigned int freq, int dir);
These forward declarations don't seem to be necessary.
[...]
+static const struct snd_kcontrol_new nau8825_snd_controls[] = {
+
+ SOC_SINGLE("HP Class OP", NAU8825_CLASS_G_CTRL, NAU8825_CLASS_G_SHIFT,
+ 1, 0),
What is "Class OP"?
+ SOC_SINGLE("DAC Right", NAU8825_DAC_CTRL, NAU8825_DAC_R_SFT, 1, 0),
+ SOC_SINGLE("DAC Left", NAU8825_DAC_CTRL, NAU8825_DAC_L_SFT, 1, 0),
The same bits are controlled by the DAC DAPM widgets, there shouldn't be any
controls for them.
+ SOC_SINGLE("DAC Right Clock", NAU8825_DAC_CTRL, NAU8825_DAC_CLK_R_SFT,
+ 1, 0),
+ SOC_SINGLE("DAC Left Clock", NAU8825_DAC_CTRL, NAU8825_DAC_CLK_L_SFT,
+ 1, 0),
The clock controls should probably not be user controllable but rather be
DAPM supply widgets.
+};
+
+static const struct snd_kcontrol_new nau8825_hpo_mix[] = {
+ SOC_DAPM_SINGLE("HP L Switch", NAU8825_HSVOL_CTRL,
+ NAU8825_L_MUTE_SFT, 1, 1),
+ SOC_DAPM_SINGLE("HP R Switch", NAU8825_HSVOL_CTRL,
+ NAU8825_R_MUTE_SFT, 1, 1),
+};
+
+static const struct snd_soc_dapm_widget nau8825_dapm_widgets[] = {
+
+ SND_SOC_DAPM_INPUT("Mic Jack"),
Input and output widgets should have the same name as the matching pin of
the CODEC.
+ SND_SOC_DAPM_MICBIAS("MIC BIAS", NAU8825_BOOST, NAU8825_G_BIAS_SFT, 0),
New drivers shouldn't use DAPM_MICBIAS widgets as they are known to be
broken. Use a DAPM_SUPPLY widget instead.
+ SND_SOC_DAPM_SUPPLY("micbias", SND_SOC_NOPM, 0, 0,
+ NULL, SND_SOC_DAPM_PRE_PMU
+ | SND_SOC_DAPM_POST_PMD),
+ SND_SOC_DAPM_SUPPLY("vmid", SND_SOC_NOPM, 0, 0, NULL,
+ SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMD),
Both the vmid and micbias supply seem to be unused and don't do anything either.
+ /* ADCs */
+ SND_SOC_DAPM_ADC("ADC L", NULL, SND_SOC_NOPM, 0, 0),
+ SND_SOC_DAPM_ADC("ADC R", NULL, SND_SOC_NOPM, 0, 0),
+ /* ADC IF1 */
+ SND_SOC_DAPM_PGA("IF1 ADC", SND_SOC_NOPM, 0, 0, NULL, 0),
This one seems to be unused, and doesn't do anything either.
+ /* Audio Interface */
+ SND_SOC_DAPM_AIF_IN("AIF1RX", "AIF1 Playback", 0, SND_SOC_NOPM, 0, 0),
+ SND_SOC_DAPM_AIF_OUT("AIF1TX", "AIF1 Capture", 0, SND_SOC_NOPM, 0, 0),
+ /* DACs */
+ SND_SOC_DAPM_DAC_E("DAC L1", NULL, NAU8825_DAC_CTRL,
+ NAU8825_DAC_L_SFT, 0, NULL,
+ SND_SOC_DAPM_PRE_PMD),
Just use DAC instead of DAC_E if you don't have to implement a callback
+ SND_SOC_DAPM_DAC_E("DAC R1", NULL, NAU8825_DAC_CTRL,
+ NAU8825_DAC_R_SFT, 0, NULL,
+ SND_SOC_DAPM_PRE_PMD),
Same here.
+ /* SPO/HPO/LOUT/Mono Mixer */
+ SND_SOC_DAPM_MIXER("HPO MIX", SND_SOC_NOPM, 0, 0, nau8825_hpo_mix,
+ ARRAY_SIZE(nau8825_hpo_mix)),
+ SND_SOC_DAPM_PGA_S("HP amp", 1, NAU8825_CLASS_G_CTRL,
+ NAU8825_CLASS_G_SHIFT, 0, NULL, 0),
No need for _S if there is no sub-sequencing involved.
+ /* Output Lines */
+ SND_SOC_DAPM_OUTPUT("HPOL"),
+ SND_SOC_DAPM_OUTPUT("HPOR"),
+};
+
[...
+static int nau8825_hw_params(struct snd_pcm_substream *substream,
+ struct snd_pcm_hw_params *params,
+ struct snd_soc_dai *tmp)
+{
+ struct snd_soc_pcm_runtime *rtd = substream->private_data;
+ struct snd_soc_codec *codec = rtd->codec;
rtd->codec does not necessarily point to your CODEC device, use dai->codec
instead. As rule of thumb you should never have to look at the
snd_soc_pcm_runtime in a CODEC driver.
[...]
+
+static void config_fll_clk_12m(struct snd_soc_codec *codec)
+{
+ snd_soc_update_bits(codec, NAU8825_CLK_DIVIDER, 0x000F, 0x0003);
+ snd_soc_update_bits(codec, NAU8825_FL_1, 0x007F, 0x0001);
+ snd_soc_write(codec, NAU8825_FL_2, 0xC49B);
+ snd_soc_update_bits(codec, NAU8825_FL_3, 0x03FF, 0x0020);
+ snd_soc_update_bits(codec, NAU8825_FL_4, 0x0C00, 0x0800);
+ snd_soc_update_bits(codec, NAU8825_FL_5, 0x2000, 0x0000);
+ snd_soc_update_bits(codec, NAU8825_FL_6, 0x4000, 0x4000);
So what do these magic values do?
+}
+
+void set_sys_clk(struct snd_soc_codec *codec, int sys_clk)
static
+{
+ pr_debug("%s :: sys_clk=%x\n", __func__, sys_clk);
[...
+static int nau8825_codec_suspend(struct snd_soc_codec *codec)
+{
+ nau8825_set_bias_level(codec, SND_SOC_BIAS_OFF);
+ return 0;
+}
set the suspend_bias_off flag in your codec driver to let the core take care
of this.
+
+static int nau8825_resume(struct snd_soc_codec *codec)
+{
+ nau8825_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
+ return 0;
+}
This is not necessary the core already takes care of it.
+
[...]
+static int nau8825_codec_probe(struct snd_soc_codec *codec)
+{
+ int i;
+ struct nau8825_priv *nau8825;
+
+ nau8825 = snd_soc_codec_get_drvdata(codec);
+ nau8825->codec = codec;
+ /*writing initial register values to the codec*/
+ for (i = 0; i < ARRAY_SIZE(nau8825_reg); i++)
+ snd_soc_write(codec, nau8825_reg[i].reg, nau8825_reg[i].def);
If that is really necessary use regmap_sync() and preferably in the i2c
probe function. But I'd expect that a soft reset should put the device in
the default register configuration?
+ return 0;
+}
+
[...]
+static struct snd_soc_codec_driver soc_codec_driver_nau8825 = {
const
+ .probe = nau8825_codec_probe,
+ .remove = nau8825_codec_remove,
+ .suspend = nau8825_codec_suspend,
+ .resume = nau8825_resume,
+ .set_bias_level = nau8825_set_bias_level,
+ .controls = nau8825_snd_controls,
+ .num_controls = ARRAY_SIZE(nau8825_snd_controls),
+ .dapm_widgets = nau8825_dapm_widgets,
+ .num_dapm_widgets = ARRAY_SIZE(nau8825_dapm_widgets),
+ .dapm_routes = nau8825_dapm_routes,
+ .num_dapm_routes = ARRAY_SIZE(nau8825_dapm_routes),
+};
+
+static struct snd_soc_dai_ops nau8825_dai_ops = {
const
+ .hw_params = nau8825_hw_params,
+ .set_sysclk = nau8825_dai_set_sysclk,
+ .set_fmt = nau8825_set_dai_fmt,
+ .digital_mute = nau8825_dac_mute,
+};
+
+static struct snd_soc_dai_driver nau8825_dai_driver[] = {
+ {
+ .name = "nau8825-aif1",
+ .playback = {
+ .stream_name = "AIF1 Playback",
+ .channels_min = 1,
+ .channels_max = 2,
+ .rates = NAU8825_RATES,
+ .formats = NAU8825_FORMATS,
+ },
+ .capture = {
+ .stream_name = "AIF1 Capture",
+ .channels_min = 1,
+ .channels_max = 2,
+ .rates = NAU8825_RATES,
+ .formats = NAU8825_FORMATS,
+ },
+ .ops = &nau8825_dai_ops,
+ }
+};
+
+
+static int nau8825_i2c_probe(struct i2c_client *i2c,
+ const struct i2c_device_id *i2c_id)
+{
+ struct nau8825_platform_data *pdata = dev_get_platdata(&i2c->dev);
+ struct nau8825_priv *nau8825;
+ int ret;
+
+ nau8825 = devm_kzalloc(&i2c->dev, sizeof(struct nau8825_priv),
preferred form for this is sizeof(*nau8825)
[...]
+MODULE_DESCRIPTION("ASoC NAU8825 codec driver");
+MODULE_AUTHOR("Nuvoton");
+MODULE_LICENSE("GPL v2");
+
+
No need for the extra new lines at the end
diff --git a/sound/soc/codecs/nau8825.h b/sound/soc/codecs/nau8825.h
new file mode 100644
index 0000000..b16d4d1
--- /dev/null
+++ b/sound/soc/codecs/nau8825.h
@@ -0,0 +1,150 @@
+/*
[...]
+
+struct nau8825_priv {
+ struct snd_soc_codec *codec;
+ struct nau8825_platform_data pdata;
+ struct regmap *regmap;
+ struct i2c_client *i2c;
+ struct snd_soc_jack *hp_jack;
+ struct snd_soc_jack *mic_jack;
+ struct delayed_work delayed_work;
+
+ struct workqueue_struct *workqueue;
+ struct mutex mutex;
+ unsigned int irq;
+ bool jd_status;
+ bool bp_status;
+ int jack_type;
+};
It looks like pretty much all of the fields in the struct are not used by
the driver.
+#endif /* _NAU8825_H */
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/