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?