lkml.org 
[lkml]   [2015]   [Apr]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] ASoC: Add support for NAU8825 codec to ASoC
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@nuvoton.com>
> ---
> 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 */
>



\
 
 \ /
  Last update: 2015-04-10 11:21    [W:0.342 / U:0.008 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site