[alsa-devel] [PATCH] ASoC: nau8824: new codec driver

[Mark Brown]

On Thu, Jan 26, 2017 at 11:48:09AM +0800, John Hsu wrote:

This looks mostly good, a few fairly small things below:

> +       SOC_SINGLE_TLV("Speaker Right Volume from DACR",
> +               NAU8824_REG_CLASSD_GAIN_1, 8, 0x1f, 0, spk_vol_tlv),

Names for volume controls need to end with Volume for userspace to
understand them properly, give these the same names that the DAPM mixer
controls have but end with Volume instead of Switch and then userspace
will be able to combine them when displaying.

> +       case SND_SOC_DAPM_POST_PMU:
> +               /* Prevent startup click by letting charge pump to ramp up */
> +               usleep_range(10000, 11000);

I believe the current preference is to do this as just msleep(10).

> +static int nau8824_codec_probe(struct snd_soc_codec *codec)
> +{
> +       struct nau8824 *nau8824 = snd_soc_codec_get_drvdata(codec);
> +       struct snd_soc_dapm_context *dapm = snd_soc_codec_get_dapm(codec);
> +
> +       nau8824->dapm = dapm;
> +       snd_soc_dapm_sync(nau8824->dapm);

This sync shouldn't do anything, just remove it.

> +static int __maybe_unused nau8824_suspend(struct snd_soc_codec *codec)
> +{
> +       struct nau8824 *nau8824 = snd_soc_codec_get_drvdata(codec);
> +
> +       if (nau8824->irq) {
> +               disable_irq(nau8824->irq);
> +               snd_soc_codec_force_bias_level(codec, SND_SOC_BIAS_OFF);
> +       }

Why are we disabling the IRQ here?  

> +       if (nau8824->irq) {
> +               nau8824_sema_acquire(nau8824, 0);
> +               enable_irq(nau8824->irq);
> +       }

We didn't drop this sempahore in the suspend path?  This stuff could at
least use better comments.

> +       /* Boost FEPGA 20dB */
> +       regmap_update_bits(regmap, NAU8824_REG_FEPGA_II,
> +               NAU8824_FEPGA_GAINL_MASK | NAU8824_FEPGA_GAINR_MASK,
> +               0xa | (0xa << NAU8824_FEPGA_GAINR_SFT));

Possibly something that should be controllable from userspace?

> +       nau8824_reset_chip(nau8824->regmap);
> +       ret = regmap_read(nau8824->regmap, NAU8824_REG_I2C_DEVICE_ID, &value);
> +       if (ret < 0) {
> +               dev_err(dev, "Failed to read device id from the NAU8824: %d\n",
> +                       ret);
> +               return ret;
> +       }

Perhaps check the device ID before resetting the chip (and verify that
the device ID is what's expected)?  That way if the system is
misconfigured then the check will be less disruptive.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20170201/ee3daaf9/attachment.sig>