Re: [PATCH 2/2] ASoC: Add ADAU1977 driver

[Mark Brown <broonie@kernel.org>]

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

On Mon, Feb 03, 2014 at 03:57:08PM +0100, Lars-Peter Clausen wrote:
> This patch adds support for the ADAU1977, ADAU1978 and ADAU1979 audio CODEC
> devices. They are a family of 4-channel differntial input audio ADC devices.
> They can be connected to either a SPI or I2C bus.

To avoid tedious iteration of the driver for the DT bindings it's
generally helpful to submit the DT bindings as a separate patch.

>  sound/soc/codecs/adau1977-i2c.c        |  58 ++
>  sound/soc/codecs/adau1977-spi.c        |  73 +++

This isn't the general style, if we want to do that we should be
converting all the other drivers.

> +config SND_SOC_ADAU1977
> +	tristate
> +
> +config SND_SOC_ADAU1977_I2C
> +	tristate
> +	select SND_SOC_ADAU1977
> +
> +config SND_SOC_ADAU1977_SPI
> +	tristate
> +	select SND_SOC_ADAU1977
> +

Please make these user visible if OF is in use.

> +static struct i2c_driver adau1977_i2c_driver = {
> +	.driver = {
> +		.name = "adau1977",
> +		.owner = THIS_MODULE,
> +	},
> +	.probe = adau1977_i2c_probe,
> +	.remove = adau1977_i2c_remove,
> +	.id_table = adau1977_i2c_ids,
> +};

You should have explicit OF IDs.

> +	if (rate >= 8000 && rate <= 12000)
> +		ctrl0 = ADAU1977_SAI_CTRL0_FS_8000_12000;
> +	else if (rate >= 16000 && rate <= 24000)
> +		ctrl0 = ADAU1977_SAI_CTRL0_FS_16000_24000;
> +	else if (rate >= 32000 && rate <= 48000)
> +		ctrl0 = ADAU1977_SAI_CTRL0_FS_32000_48000;
> +	else if (rate >= 64000 && rate <= 96000)
> +		ctrl0 = ADAU1977_SAI_CTRL0_FS_64000_96000;
> +	else if (rate >= 128000 && rate <= 192000)
> +		ctrl0 = ADAU1977_SAI_CTRL0_FS_128000_192000;
> +	else
> +		return -EINVAL;

There's a couple of these rate range if trees - how about a lookup in an
array?

> +	if (!enable) {
> +		ret = regmap_update_bits(adau1977->regmap, ADAU1977_REG_POWER,
> +			ADAU1977_POWER_PWUP, 0);
> +		regcache_mark_dirty(adau1977->regmap);
> +	} else {
> +		ret = regulator_enable(adau1977->avdd_reg);
> +		if (ret)
> +			return ret;
> +		if (adau1977->dvdd_reg) {
> +			ret = regulator_enable(adau1977->dvdd_reg);
> +			if (ret)
> +				goto err_disable_avdd;
> +		}
> +	}
> +
> +	if (adau1977->reset_gpio)
> +		gpiod_set_value(adau1977->reset_gpio, enable);

_cansleep().

I'm not sure all this enable/disable logic within the function is
helping clarity here.  There seems to be no meaningful sharing between
the two branches, separate functions for power up and power down is
probably going to be clearer.

> +static int adau1977_mute(struct snd_soc_dai *dai, int mute, int stream)
> +{
> +	struct adau1977 *adau1977 = snd_soc_codec_get_drvdata(dai->codec);
> +	unsigned int val;
> +
> +	if (stream == SNDRV_PCM_STREAM_PLAYBACK)
> +		return 0;

Why would this ever be called for an ADC?

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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