On Mon, Aug 15, 2016 at 05:02:23PM +0800, John Hsu wrote:
This looks very good overall, a few fairly small things but nothing too major.
- val = (u16 *)ucontrol->value.bytes.data;
- reg = NAU8810_REG_EQ1;
- for (i = 0; i < params->max / sizeof(u16); i++) {
regmap_read(nau8810->regmap, reg + i, ®_val);reg_val = cpu_to_be16(reg_val);memcpy(val + i, ®_val, sizeof(reg_val));- }
This looks like it's trying to do regmap_raw_read()? Raw I/O bypasses all the endianness conversions.
+static int nau8810_set_clkdiv(struct snd_soc_dai *dai, int div_id, int div) +{
- struct snd_soc_codec *codec = dai->codec;
- struct nau8810 *nau8810 = snd_soc_codec_get_drvdata(codec);
- struct regmap *regmap = nau8810->regmap;
- struct nau8810_pll *pll = &nau8810->pll;
- switch (nau8810->div_id) {
- case NAU8810_MCLK_DIV_PLL:
/* master clock from PLL and enable PLL */
I'd really not expect new drivers to need to have a set_clkdiv() operation. Most things should just be worked out by the driver, that means less duplicated code in machine drivers and that things like simple-card have more chance of working for a device. This one I'm not really sure what the divider is so it's hard to make specific recommendations.
- case NAU8810_MCLK_DIV_MCLK:
/* Defer the master clock prescaler configuration to DAI* hardware parameter if master clock from MCLK because* it needs runtime fs information to get the proper div.*/break;
This is obviously not good since it means that we just ignore anything that's set - if the caller is trying to set a divider we shouldn't just be silently discarding what they set. It looks like this can just be removed since the driver is able to calcuate
- case NAU8810_BCLK_DIV:
regmap_update_bits(regmap, NAU8810_REG_CLOCK,NAU8810_BCLKSEL_MASK, (div << NAU8810_BCLKSEL_SFT));break;
If this is really needed by anything the set_bclk_ratio() call is more appropriate.
+static int nau8810_probe(struct snd_soc_codec *codec) +{
- return 0;
+}
Remove empty functions, if they can reasonably be empty then the framework will handle them being missing.
+static struct snd_soc_codec_driver soc_codec_dev_nau8810 = {
- .controls = nau8810_snd_controls,
- .num_controls = ARRAY_SIZE(nau8810_snd_controls),
- .dapm_widgets = nau8810_dapm_widgets,
- .num_dapm_widgets = ARRAY_SIZE(nau8810_dapm_widgets),
- .dapm_routes = nau8810_dapm_routes,
- .num_dapm_routes = ARRAY_SIZE(nau8810_dapm_routes),
Move this data into the component driver please.