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.
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
