On Thu, Sep 26, 2019 at 09:17:06AM +0200, Nuno Sá wrote:
> --- /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.
Attachment:
signature.asc
Description: PGP signature
