Re: [alsa-devel] [PATCH 1/2] ASOC: Add ADAU7118 8 Channel PDM-to-I2S/TDM Converter driver

From: Mark Brown <broonie@kernel.org>
To: "Nuno Sá" <nuno.sa@analog.com>
Cc: Mark Rutland <mark.rutland@arm.com>,
	devicetree@vger.kernel.org, alsa-devel@alsa-project.org,
	Lars-Peter Clausen <lars@metafoo.de>,
	Takashi Iwai <tiwai@suse.com>,
	Liam Girdwood <lgirdwood@gmail.com>,
	Rob Herring <robh+dt@kernel.org>
Subject: Re: [alsa-devel] [PATCH 1/2] ASOC: Add ADAU7118 8 Channel PDM-to-I2S/TDM Converter driver
Date: Thu, 26 Sep 2019 11:43:18 -0700	[thread overview]
Message-ID: <20190926184318.GF2036@sirena.org.uk> (raw)
In-Reply-To: <20190926071707.17557-1-nuno.sa@analog.com>

[-- Attachment #1.1: Type: text/plain, Size: 2347 bytes --]

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 #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel