Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754575AbbDJIiY (ORCPT ); Fri, 10 Apr 2015 04:38:24 -0400 Received: from smtp-out-032.synserver.de ([212.40.185.32]:1058 "EHLO smtp-out-032.synserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753241AbbDJIiR (ORCPT ); Fri, 10 Apr 2015 04:38:17 -0400 X-SynServer-TrustedSrc: 1 X-SynServer-AuthUser: lars@metafoo.de X-SynServer-PPID: 25189 Message-ID: <55278BEF.5070406@metafoo.de> Date: Fri, 10 Apr 2015 10:38:07 +0200 From: Lars-Peter Clausen User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Icedove/31.6.0 MIME-Version: 1.0 To: Chih-Chiang Chang , Mark Brown CC: "tiwai@suse.de" , "alsa-devel@alsa-project.org" , lgirdwood@gmail.com, "linux-kernel@vger.kernel.org" Subject: Re: [PATCH] ASoC: Add support for NAU8825 codec to ASoC References: <55277A06.3060206@nuvoton.com> In-Reply-To: <55277A06.3060206@nuvoton.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 9774 Lines: 324 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 > --- > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/