Re: [PATCH V2 1/1] ASoC: ak5702: add support for ak5702 -- 4ch ADC

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

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

On Wed, Nov 28, 2012 at 04:54:49PM +0100, Paolo Doz wrote:

> +enum ak5702_clock_mode {
> +    AK5702_CLKPLL_SLAVE,
> +    AK5702_CLKPLL_SLAVE2,

The number one problem with this patch is that it's not following the
kernel coding style at all - please see CodingStyle, checkpatch should
also complain a lot about this.

> +#define AK5702_VERSION "0.5"
> +#define DRV_NAME "ak5702"

The kernel already has perfectly good version numbers, use them - people
aren't going to keep driver specific version numbers up to date anyway.

> +static const struct reg_default ak5702_reg_defaults[] = {
> +    {  0, 0x0000},    /* R0  - Power Management */

Looks a big odd - I'd expect spaces for both { and }.

> +static bool ak5702_writeable(struct device *dev, unsigned int reg)
> +{
> +    return reg < AK5702_MAX_REGISTER;

Should be <= or the #define is misnamed.

> +static const struct soc_enum ak5702_enum[] = {
> +    SOC_ENUM_SINGLE(AK5702_SIG1, 0, 2, ak5702_adca_left12_input),
> +    SOC_ENUM_SINGLE(AK5702_SIG1, 1, 2, ak5702_adca_right12_input),

Don't use a big array of enums, it's error prone and hard to read.  Use
individually named enums like other drivers.

> +    SOC_ENUM("ADCA Left1/2 Capture Source", ak5702_enum[0]),
> +    SOC_ENUM("ADCA Right1/2 Capture Source", ak5702_enum[1]),
> +    SOC_ENUM("ADCB Left1/2 Capture Source", ak5702_enum[2]),
> +    SOC_ENUM("ADCB Right1/2 Capture Source", ak5702_enum[3]),
> +    SOC_ENUM("ADCA Left5 Capture Source", ak5702_enum[4]),
> +    SOC_ENUM("ADCA Right5 Capture Source", ak5702_enum[5]),
> +    SOC_ENUM("ADCB Left5 Capture Source", ak5702_enum[6]),
> +    SOC_ENUM("ADCB Right5 Capture Source", ak5702_enum[7]),

These should all be DAPM controls.

> +    SOC_SINGLE_TLV("Mic A Boost Gain", AK5702_MICG1, 0, 3, 0, mic_tlv),
> +    SOC_SINGLE_TLV("Mic B Boost Gain", AK5702_MICG2, 0, 3, 0, mic_tlv),

Volume, not Gain.

> +        /* power on ADCA */
> +        value = AK5702_PM1_PMADAL |
> +            AK5702_PM1_PMADAR |
> +            AK5702_PM1_PMVCM;
> +        snd_soc_write(codec, AK5702_PM1, value);
> +        /* power up ADCB */
> +        value = AK5702_PM2_PMADBL | AK5702_PM2_PMADBR;
> +        snd_soc_write(codec, AK5702_PM2, value);

Just inline the value to write.

> +        if (params_format(params) == SNDRV_PCM_FORMAT_S16_LE)
> +            value = AK5702_FMT1_TDM128;
> +        else if (params_format(params) == SNDRV_PCM_FORMAT_S32_LE)
> +            value = AK5702_FMT1_TDM256;
> +        else
> +            return -EINVAL;

Use a switch statement for the check for params_format().

> +    case 48000:
> +        if (ak5702->clock_mode == AK5702_CLKPLL_SLAVE2)
> +            value = AK5702_FS1_BCLK_MODE2;
> +        else if (ak5702->clock_mode == AK5702_CLKEXT_SLAVE)
> +            value = AK5702_FS1_SLAVE_256FS; /* mode 3 256fs*/
> +        else
> +            return -EINVAL;

Similarly here.

> +static int ak5702_set_dai_pll(struct snd_soc_dai *codec_dai, int pll_id,
> +        int source, unsigned int freq_in, unsigned int freq_out)
> +{
> +    struct snd_soc_codec *codec = codec_dai->codec;
> +    u8 reg = 0;
> +    pr_debug("Entered %s", __func__);
> +    reg = snd_soc_read(codec, AK5702_PLL1);
> +    switch (pll_id) {
> +    case AK5702_PLL_POWERDOWN:
> +        reg &= (~AK5702_PLL1_PM_MASK);
> +        reg |= AK5702_PLL1_POWERDOWN;
> +        break;

pll_id should be which PLL is being changed, power down is done by
setting the PLL output to zero.

> +    case AK5702_PLL_MASTER:
> +        reg &= (~AK5702_PLL1_MODE_MASK);
> +        reg |= AK5702_PLL1_MASTER | AK5702_PLL1_POWERUP;
> +        break;
> +    case AK5702_PLL_SLAVE:

These probably want to be selected by source.

> +        reg &= (~AK5702_PLL1_MODE_MASK);
> +        reg |= AK5702_PLL1_SLAVE | AK5702_PLL1_POWERUP;

snd_soc_update_bits()

> +    default:
> +        return -ENODEV;

-EINVAL.

> +    if (div_id == AK5702_BCLK_CLKDIV) {

switch.

> +    dev_info(codec->dev, "AK5702 Audio Codec %s", AK5702_VERSION);

Remove this, it's just adding noise to the boot.

> +    codec->control_data = ak5702->regmap;

No need for this, the framework will use dev_get_regmap().

> +static const struct i2c_device_id ak5702_i2c_id[] = {
> +    { "ak5702-codec", 0 },

No -codec, this is a single function device and it appears to be an ADC
rather than a CODEC anyway.

> +    .driver = {
> +        .name = "ak5702-codec",

No -codec.

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

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