On Wed, Aug 12, 2009 at 9:46 PM, MarkBrown<broonie@xxxxxxxxxxxxxxxxxxxxxxxxxxx> wrote:> On Wed, Aug 12, 2009 at 12:32:52PM +0800, Barry Song wrote:>> There has been an ad1836 driver in sound/blackfin based on traditional alsa.>> The new driver is based on asoc. The architecture of ad1836 codec driver>> is very much like ad1938.>> This is mostly good, a few relatively minor issues though.>>> + SOC_DOUBLE_R("DAC3 Volume", AD1836_DAC_L3_VOL,>> + AD1836_DAC_R3_VOL, 0, 0x3FF, 0),>> You've got an extra space in the volume control names which I'd expect> to confuse user space.Fixed.>>> + { "DAC1OUT", "DAC1 Switch", "DAC" },>> + { "DAC2OUT", "DAC2 Switch", "DAC" },>> I'm surprised this works since the switches weren't declared as DAPM> controls but if the core supports it that's OK.>>> +static int ad1836_set_tdm_slot(struct snd_soc_dai *dai,>> + unsigned int mask, int slots)>> +{>> This needs updating for the new set_tdm_slot() API but given that> there's only one possible configuration I'd suggest just removing it -> the function does nothing other than check its arguments.>Fixed>> + reg = codec->read(codec, AD1836_DAC_CTRL1);>> + reg = (reg & (~AD1836_DAC_WORD_LEN_MASK)) | word_len;>> + codec->write(codec, AD1836_DAC_CTRL1, reg);>> Not essential but snd_soc_update_bits() does the read/modify/write cycle> for you.>Fixed>> +static struct spi_driver ad1836_spi_driver = {>> + .driver = {>> + .name = "ad1836-spi",>> Just "ad1836" - the fact that it's controlled via SPI is clear from the> fact that this is a struct spi_driver.>I agree the spi structure implies the meaning of spi. But files inarch/blackfin/mach... are using names like "xxx-spi" for almost allspi devices. How about keeping the coherence?
>> + ret = snd_soc_register_codec(codec);>> + if (ret != 0) {>> + dev_err(codec->dev, "Failed to register codec: %d\n", ret);>> + return ret;>> + }>> +>> + ret = snd_soc_register_dai(&ad1836_dai);>> + if (ret != 0) {>> + dev_err(codec->dev, "Failed to register DAI: %d\n", ret);>> + snd_soc_unregister_codec(codec);>> + return ret;>> + }>> Should also be freeing the private data structure on error.>Fixed>> +struct snd_soc_codec_device soc_codec_dev_ad1836 = {>> + .probe = ad1836_probe,>> + .remove = ad1836_remove,>> + /* The power management of ad1836 is very simple. There are>> + * only adc&dac 2 components to control. Dapm handles them.>> + */>> + .suspend = NULL,>> + .resume = NULL,>> The power control will be handled by DAPM but your driver still needs to> restore things like the volume settings - when the driver is powered off> the register settings will be forgotten.>My test shows registers setting doesn't lose after dac/adc shutdown :-)_______________________________________________Alsa-devel mailing listAlsa-devel@xxxxxxxxxxxxxxxxxxxx://mailman.alsa-project.org/mailman/listinfo/alsa-devel
