[alsa-devel] [PATCH 1/3] new ad1836 codec driver based on asoc

[Barry Song]

On Wed, Aug 12, 2009 at 9:46 PM, Mark
Brown<broonie at opensource.wolfsonmicro.com> 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 in
arch/blackfin/mach... are using names like "xxx-spi" for almost all
spi 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 :-)