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.