Re: [PATCH 1/2] ASOC: Add ADAU7118 8 Channel PDM-to-I2S/TDM Converter driver
From: Mark Brown <broonie@kernel.org>
Date: 2019-09-26 18:43:18
Also in:
alsa-devel
On Thu, Sep 26, 2019 at 09:17:06AM +0200, Nuno Sá wrote:
quoted hunk ↗ jump to hunk
--- /dev/null +++ b/sound/soc/codecs/adau7118-hw.c@@ -0,0 +1,43 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Analog Devices ADAU7118 8 channel PDM-to-I2S/TDM Converter Standalone Hw + * driver + * + * Copyright 2019 Analog Devices Inc. + */
Please make the entire comment a C++ style one in the .c files so things look more intentional.
+ switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
+ case SND_SOC_DAIFMT_I2S:
+ ret = snd_soc_component_update_bits(dai->component,
+ ADAU7118_REG_SPT_CTRL1,
+ ADAU7118_DATA_FMT_MASK,
+ ADAU7118_DATA_FMT(0));
+ break;
+ case SND_SOC_DAIFMT_LEFT_J:
+ ret = snd_soc_component_update_bits(dai->component,
+ ADAU7118_REG_SPT_CTRL1,
+ ADAU7118_DATA_FMT_MASK,
+ ADAU7118_DATA_FMT(1));
+ break;
+ case SND_SOC_DAIFMT_RIGHT_J:
+ st->right_j = true;
+ break;Don't we need to set any register values here?
+ + return ret < 0 ? ret : 0; +}
Please don't use the ternery operator like this, it just makes things harder to read - write normal if conditional statements.
+ case SND_SOC_BIAS_STANDBY:
+ if (snd_soc_component_get_bias_level(component) ==
+ SND_SOC_BIAS_OFF) {
+ if (!st->iovdd)
+ return 0;This is broken, the device will always require power so it should always control the regulators.
+static int adau7118_suspend(struct snd_soc_component *component)
+{
+ return snd_soc_component_force_bias_level(component, SND_SOC_BIAS_OFF);
+}
+
+static int adau7118_resume(struct snd_soc_component *component)
+{
+ return snd_soc_component_force_bias_level(component,
+ SND_SOC_BIAS_STANDBY);
+}Let DAPM do this for you, there's no substantial delays on power on so you're probably best just setting idle_bias_off.
+static int adau7118_regulator_setup(struct adau7118_data *st)
+{
+ int ret = 0;
+
+ st->iovdd = devm_regulator_get_optional(st->dev, "IOVDD");
+ if (!IS_ERR(st->iovdd)) {Unless the device can operate with supplies physically absent it should not be requesting regulators as optional, this breaks your error handling especially with probe deferral which is a fairly common case.