On Thu 24 Sep 2009 05:48, [email protected] pondered:
> --- trunk/sound/soc/blackfin/bf5xx-adau1371.c                         (rev 
0)
> +++ trunk/sound/soc/blackfin/bf5xx-adau1371.c 2009-09-24 09:48:30 UTC (rev 
7448)
[snip]
> +static int bf5xx_adau1371_hw_params(struct snd_pcm_substream *substream,
> +     struct snd_pcm_hw_params *params)
> +{
> +     struct snd_soc_pcm_runtime *rtd = substream->private_data;
> +     struct snd_soc_dai *codec_dai = rtd->dai->codec_dai;
> +     struct snd_soc_dai *cpu_dai = rtd->dai->cpu_dai;
> +     int ret = 0;
> +     /* Change input crystal source here */
> +     unsigned int clk = 12288000;
> +     /* unsigned int clk = 11289600; */

Doesn't this belong in the ASOC equivalent of the platform data? (or 
is ./soc/blackfin/ where that goes?)

If so - shouldn't there be some comment about "this is the crystal on the 
ADAU1371 demo board"?

[snip]
> --- trunk/sound/soc/codecs/adau1371.c                         (rev 0)
> +++ trunk/sound/soc/codecs/adau1371.c 2009-09-24 09:48:30 UTC (rev 7448)
> @@ -0,0 +1,1105 @@
[snip]
> +/*
> + * adau1371 register cache
> + * We can't read the adau1371 register space when we are
> + * using 2 wire for device control, so we cache them instead.
> + * There is no point in caching the reset register
> + */
> +static const u8 adau1371_reg[ADAU1371_CACHEREGNUM] = {
> +     0x00, 0x1f, 0x1f, 0x1f, 0x1f, 0x1f, 0x1f, 0x1f, /* 0x00-0x07 */
> +     0x1f, 0x1f, 0x1f, 0x1f, 0x1f, 0x1f, 0x1f, 0x00, /* 0x08-0x0f */
> +     0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* 0x10-0x17 */
> +     0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* 0x18-0x1f */
> +     0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* 0x20-0x27 */
> +     0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* 0x28-0x2f */
> +     0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* 0x30-0x37 */
> +     0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* 0x38-0x3f */
> +     0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* 0x40-0x47 */
> +     0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* 0x48-0x4f */
> +     0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* 0x50-0x57 */
> +     0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* 0x58-0x5f */
> +     0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* 0x60-0x67 */
> +     0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* 0x68-0x6f */
> +     0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* 0x70-0x77 */
> +     0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* 0x78-0x7f */
> +     0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* 0x80-0x87 */
> +     0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* 0x88-0x8f */
> +     0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* 0x90-0x97 */
> +     0x20, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* 0x98-0x9f */
> +     0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* 0xa0-0xa7 */
> +     0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* 0xa8-0xaf */
> +     0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* 0xb0-0xb7 */
> +     0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* 0xb8-0xbf */
> +     0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* 0xc0-0xc7 */
> +     0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* 0xc8-0xcf */
> +     0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* 0xd0-0xd7 */
> +     0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* 0xd8-0xdf */
> +     0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* 0xe0-0xe7 */
> +     0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* 0xe8-0xef */
> +     0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* 0xf0-0xf7 */
> +     0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00  /* 0xf8-0xff */
> +};

these don't seem to be reset on the reset function - should they be?

> +struct _pll_settings {
> +     u32 mclk;
> +     u32 rate;
> +     u16 n;  /* N and M registers are inverted on REVB*/

I was told - although we only have rev B today - there is no plan to release 
rev B to production, and that we should only plan on supporting (long term) 
rev C....

So, I guess this needs to stay for now, but can be dropped before sending 
upstream.

> +     u16 m;
> +     u8 input_div:2;
> +     u8 integer:4;
> +     u8 type:1;
> +};
> +
> +/* PLL settings coefficients, add more here... */
> +static const struct _pll_settings pll_settings[] = {
> +     /* 96k */
> +     /* {12288000, 96000, 0, 0, 0x0, 0x8, 0x0}, */
> +     /* 88.2k */
> +     /* {12288000, 88200, 20, 7, 0x0, 0x7, 0x1}, */
> +     /* 48k */
> +     {12288000, 48000, 0, 0, 0x0, 0x4, 0x0},
> +     /* 44.1k */
> +     {12288000, 44100,  40, 27, 0x0, 0x3, 0x1},
> +     {11289600, 44100, 0, 0, 0x0, 0x4, 0x0},
> +     /* 22.050k */
> +     {12288000, 22050,  40, 27, 0x1, 0x3, 0x1},
> +     {11289600, 22050, 0, 0, 0x0, 0x2, 0x0},
> +     /* 8k */
> +     /* {12288000, 8000, 3, 2, 0x3, 0x2, 0x1}, */
> +
> +};

Don't these need to be calculated - as they depend on the crystal input?


[snip]
> +static int adau1371_init(struct snd_soc_device *socdev)
> +{
> +     struct snd_soc_codec *codec = socdev->card->codec;
> +     int reg = 0, ret = 0;
> +     struct adau1371_priv *adau1371 = codec->private_data;
> +     codec->name = "adau1371";
> +     codec->owner = THIS_MODULE;
> +     codec->read = adau1371_read_reg_cache;
> +     codec->write = adau1371_write;
> +     codec->set_bias_level = adau1371_set_bias_level;
> +     codec->dai = &adau1371_dai;
> +     codec->num_dai = 1;
> +     codec->reg_cache_size = sizeof(adau1371_reg);
> +     codec->reg_cache = kmemdup(adau1371_reg, sizeof(adau1371_reg),
> +                                     GFP_KERNEL);
> +     if (codec->reg_cache == NULL)
> +             return -ENOMEM;
> +
> +     adau1371_reset(codec);
> +     /* By default line out and input A are enabled*/
> +     adau1371->out_chan_mask = PB_LINE;
> +     adau1371->in_chan_mask = CAP_INPA;
> +     /* register pcms */
> +     ret = snd_soc_new_pcms(socdev, SNDRV_DEFAULT_IDX1, SNDRV_DEFAULT_STR1);
> +     if (ret < 0) {
> +             pr_err("adau1371: failed to create pcms\n");
> +             goto pcm_err;
> +     }
> +     /* Playback mix settings, line out switched to DACs*/
> +     adau1371_write(codec, ADAU1371_LLINEMIX, LDAC_SIGNAL_ENA);
> +     adau1371_write(codec, ADAU1371_RLINEMIX, RDAC_SIGNAL_ENA);
> +     /* Line out volume gain:0 db by default */

0db by default, or should things be muted to start? (to reduce click/pop)

> +     adau1371_write(codec, ADAU1371_LLINEVOL, 0x1f);
> +     adau1371_write(codec, ADAU1371_RLINEVOL, 0x1f);
> +     adau1371_write(codec, ADAU1371_DAIAPBLVOL, 0x80);
> +     adau1371_write(codec, ADAU1371_DAIAPBRVOL, 0x80);
> +
> +     /* Capture mix settings, AIN1 switched to ADCs */
> +     adau1371_write(codec, ADAU1371_LADCMIX, AIN1_SIGNAL_ENA);
> +     adau1371_write(codec, ADAU1371_RADCMIX, AIN1_SIGNAL_ENA);
> +     /* Input volume gain:0 db by default */
> +     adau1371_write(codec, ADAU1371_INPUTMODE, 0x00);
> +     adau1371_write(codec, ADAU1371_INALVOL, 0x1f);
> +     adau1371_write(codec, ADAU1371_INARVOL, 0x1f);
> +     adau1371_write(codec, ADAU1371_DAIRECLVOL, 0x80);
> +     adau1371_write(codec, ADAU1371_DAIRECRVOL, 0x80);
> +     /* Shoul be set on REVB to enable ADCs*/
> +     adau1371_write(codec, ADAU1371_CODECCTL, 0x10);
> +     /* 64 bits per frame*/
> +     adau1371_write(codec, ADAU1371_BCLKDIV, BCLKDIV_BPFA64);
> +     /* Use PLL, the clock divisor is 4 */
> +     adau1371_write(codec, ADAU1371_CLKSDIV, (3 << CLKSDIV_CLKDIV_SHIFT));
> +     /* PWR on input port A,right and left ADCs.*/
> +     reg = PWRCTLA_INAPD | PWRCTLA_MICBPD | PWRCTLA_LADCPD | PWRCTLA_RADCPD;
> +     adau1371_write(codec, ADAU1371_PWRCTLA, reg);
> +     /* PWR on line out,right and left DACs and whole chip.*/
> +     reg = PWRCTLB_RLNPD | PWRCTLB_LLNPD | PWRCTLB_RDACPD |\
> +             PWRCTLB_LDACPD | PWRCTLB_PWDB;
> +     adau1371_write(codec, ADAU1371_PWRCTLB, reg);

I'm just a little concerned that we are setting board specific info in the 
codec driver.
_______________________________________________
Linux-kernel-commits mailing list
[email protected]
https://blackfin.uclinux.org/mailman/listinfo/linux-kernel-commits

Reply via email to