| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 20 Apr 2015 16:57:55 +0200
From: Lars-Peter Clausen <lars@...afoo.de>
To: Chih-Chiang Chang <ccchang12@...oton.com>,
Mark Brown <broonie@...nel.org>
CC: tiwai@...e.de, alsa-devel@...a-project.org, lgirdwood@...il.com,
linux-kernel@...r.kernel.org
Subject: Re: [alsa-devel] [PATCH v2] ASoC: Add support for NAU8825 codec to
ASoC
On 04/16/2015 11:56 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@...oton.com>
Looks pretty good now.
> ---
> v2->v1:
> - fixes according to Lars-Peter Clausen's review comments
> - removes unused platform data file
> - corrects the naming of DAPM input widget
> - fixes some wrong coding of SOC widgets and other codes
> - adds definition and remark for config FLL clock
> - moves the code of reset hardware registers from codec_probe() to i2c_probe()
> - removes unused codes
>
[...]
> +static const struct snd_kcontrol_new nau8825_snd_controls[] = {
> +
Here and a few other places you leave the first line in the struct
declaration empty. No need for that.
> + SOC_SINGLE_TLV("MIC Volume", NAU8825_ADC_DGAIN_CTRL,
> + NAU8825_ADC_DGAIN_SFT,
> + NAU8825_ADC_VOL_RSCL_RANGE, 0, adc_vol_tlv),
> + SOC_DOUBLE_TLV("HP Volume", NAU8825_HSVOL_CTRL,
> + NAU8825_L_HSVOL_SFT, NAU8825_R_HSVOL_SFT,
> + NAU8825_VOL_RSCL_RANGE, 1, out_hp_vol_tlv),
> +};
[...]
> +
> +static void config_fll_clk_12m(struct snd_soc_codec *codec)
> +{
> + struct nau8825_priv *nau8825 = snd_soc_codec_get_drvdata(codec);
> +
> + regmap_update_bits(nau8825->regmap, NAU8825_CLK_DIVIDER,
> + NAU8825_CLK_MCLK_SRC_MASK, 0x0003);
> + regmap_update_bits(nau8825->regmap, NAU8825_FLL_1,
> + NAU8825_FLL_RATIO_MASK, 0x0001);
> + /* FLL 16-bit fractional input */
> + regmap_write(nau8825->regmap, NAU8825_FLL_2, 0xC49B);
> + /* FLL 10-bit integer input */
> + regmap_update_bits(nau8825->regmap, NAU8825_FLL_3,
> + NAU8825_FLL_INTEGER_MASK, 0x0020);
> + /* FLL pre-scaler */
> + regmap_update_bits(nau8825->regmap, NAU8825_FLL_4,
> + NAU8825_FLL_REF_DIV_MASK, 0x0800);
This seems to use some constant dividers and multipliers for the FLL. Does
this expect a specific reference clock from somewhere?
> + /* select divied VCO input */
> + regmap_update_bits(nau8825->regmap, NAU8825_FLL_5,
> + NAU8825_FLL_FILTER_SW_MASK, 0x0000);
> + /* FLL sigma delta modulator enable */
> + regmap_update_bits(nau8825->regmap, NAU8825_FLL_6,
> + NAU8825_SDM_EN_MASK, 0x4000);
> +}
> +
> +static void set_sys_clk(struct snd_soc_codec *codec, int sys_clk)
> +{
> + struct nau8825_priv *nau8825 = snd_soc_codec_get_drvdata(codec);
> +
> + pr_debug("%s :: sys_clk=%x\n", __func__, sys_clk);
> + switch (sys_clk) {
> + case NAU8825_INTERNALCLOCK:
> + regmap_update_bits(nau8825->regmap, NAU8825_CLK_DIVIDER,
> + NAU8825_SYSCLK_EN_MASK, NAU8825_SYSCLK_DIS);
> + regmap_update_bits(nau8825->regmap, NAU8825_FLL_6,
> + NAU8825_DCO_EN_MASK, NAU8825_DCO_EN);
> + regmap_update_bits(nau8825->regmap, NAU8825_CLK_DIVIDER,
> + NAU8825_SYSCLK_EN_MASK, NAU8825_SYSCLK_EN);
> + break;
> + case NAU8825_MCLK:
> + default:
> + regmap_update_bits(nau8825->regmap, NAU8825_CLK_DIVIDER,
> + NAU8825_SYSCLK_EN_MASK, NAU8825_SYSCLK_DIS);
> + regmap_update_bits(nau8825->regmap, NAU8825_FLL_6,
> + NAU8825_DCO_EN_MASK, NAU8825_DCO_DIS);
> + regmap_update_bits(nau8825->regmap, NAU8825_CLK_DIVIDER,
> + NAU8825_SYSCLK_EN_MASK, NAU8825_SYSCLK_EN);
> + break;
> + }
> +}
> +
> +static int nau8825_dai_set_sysclk(struct snd_soc_dai *dai,
> + int clk_id, unsigned int freq, int dir)
> +{
> + struct snd_soc_codec *codec = dai->codec;
> +
> + switch (clk_id) {
> + case NAU8825_MCLK:
> + config_fll_clk_12m(codec);
> + set_sys_clk(codec, clk_id);
Maybe just inline the contents of set_sys_clk here ...
> + break;
> + case NAU8825_INTERNALCLOCK:
> + set_sys_clk(codec, clk_id);
... and here
> + break;
> + default:
> + dev_err(codec->dev, "Wrong clock src\n");
> + return -EINVAL;
> + }
> + return 0;
> +}
[...]
> +static const struct reg_default nau8825_reg[] = {
> + /* enable clock source */
> + {0x0001, 0x07FF},
> + /* enable VMID and Bias */
> + {0x0076, 0x3140},
> + /* setup clock divider */
> + {0x0003, 0x0050},
> + /* jack detection configuration */
> + {0x000C, 0x0004},
> + {0x000D, 0x00E0},
> + {0x000F, 0x0801},
> + {0x0012, 0x0010},
> + /* keypad detection configuration */
> + {0x0013, 0x0280},
> + {0x0014, 0x7310},
> + {0x0015, 0x050E},
> + {0x0016, 0x1B2A},
> + /* audio format configuration */
> + {0x001A, 0x0800},
> + {0x001C, 0x000E},
> + {0x001D, 0x0010},
> + /* sampling rate control */
> + {0x002B, 0x0012},
> + /* DAC sampling rate control */
> + {0x002C, 0x0082},
> + /* ADC and DAC mixer control */
> + {0x0030, 0x00D2},
> + {0x0033, 0x00CF},
> + {0x0034, 0x02CF},
> + /* DAC class-G control */
> + {0x006A, 0x1003},
> + {0x0050, 0x2007},
> + /* ADC PGA control */
> + {0x0072, 0x0260},
> + /* DAC power down enabled */
> + {0x0080, 0x03A0},
> + /* enable DAC clock */
> + {0x0073, 0x336C},
> + /* enable MIC Bias */
> + {0x0074, 0x5502},
> + /* powerup output driver */
> + {0x007F, 0x473C},
> + {0x007F, 0x473F},
> + /* DAC power down disabled */
> + {0x0080, 0x00A0},
> + /* adjust MIC Bias */
> + {0x0066, 0x2060},
> + {0x0066, 0x2060},
> + {0x0066, 0x0060},
> +};
The register defaults should hold the content of the registers in the
power-on reset state of the chip. This is not meant to be used as a
initialization sequence.
> +
[...]
> +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,
> + },
The indent is a bit strange above.
> + .ops = &nau8825_dai_ops,
> + }
> +};
> +
> +
> +static int nau8825_i2c_probe(struct i2c_client *i2c,
> + const struct i2c_device_id *i2c_id)
> +{
> + struct nau8825_priv *nau8825;
> + int i, ret;
> +
> + nau8825 = devm_kzalloc(&i2c->dev, sizeof(*nau8825),
> + GFP_KERNEL);
> + if (nau8825 == NULL)
> + return -ENOMEM;
> + nau8825->i2c = i2c;
> + i2c_set_clientdata(i2c, nau8825);
> + nau8825->regmap = devm_regmap_init_i2c(i2c, &nau8825_regmap);
> + if (IS_ERR(nau8825->regmap)) {
> + ret = PTR_ERR(nau8825->regmap);
> + dev_err(&i2c->dev, "Failed to allocate regmap: %d\n", ret);
> + goto err_enable;
Since there is nothing to do at err_enable, just return ret directly and get
rid of the label.
> + }
> + /* software reset */
> + regmap_write(nau8825->regmap, NAU8825_RESET, 0x01);
> + regmap_write(nau8825->regmap, NAU8825_RESET, 0x02);
> + /*writing initial register values to the codec*/
> + for (i = 0; i < ARRAY_SIZE(nau8825_reg); i++)
> + regmap_write(nau8825->regmap, nau8825_reg[i].reg,
> + nau8825_reg[i].def);
> +
> + ret = snd_soc_register_codec(&i2c->dev, &soc_codec_driver_nau8825,
> + nau8825_dai_driver,
> + ARRAY_SIZE(nau8825_dai_driver));
> +err_enable:
> + return ret;
> +}
> +
[...]
> +#define NAU8825_RATES SNDRV_PCM_RATE_8000_192000
> +#define NAU8825_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S20_3LE \
> + | SNDRV_PCM_FMTBIT_S24_3LE | SNDRV_PCM_FMTBIT_S32_LE)
These defines should probably be moved to the C file close to their users.
> +
> +struct nau8825_priv {
> + struct snd_soc_codec *codec;
> + struct regmap *regmap;
> + struct i2c_client *i2c;
> + struct snd_soc_jack *jack;
> + struct delayed_work jack_detect_work;
All fields in here except for the regmap field are still unused.
> +};
> +#endif /* _NAU8825_H */
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists