Hi,
On 8/15/2016 10:06 PM, Mark Brown wrote:
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.
We also try regmap_raw_read() but it fails because the value width is 9 bits. Therefor, we make the functions by ourselves.
+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.
I agree to remove the function. The clock divider has done by PLL function if the clock source through PLL.
- 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
Yes, it's able to calculate and remove it.
- 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.
I can study it. The function is used seldom and maybe I won't provide the function.
+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.
OK, I'll do it.
+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.
I don't understand about it clearly. Is the snd_soc_codec_driver not component driver? Could you tell me more details? Thank you.