On Thu, Oct 29, 2009 at 10:13:22AM -0500, Bill Gatliff wrote:
>
> Signed-off-by: Bill Gatliff <bgat@xxxxxxxxxxxxxxx>
Something seems to have got mixed up with the submission here - the
changelog and patch appear to have been detached from each other and
there are quite a few other issues below. I suspect finger trouble
somewhere along the line. Please also remember to CC maintainers on
patches to help avoid them getting lost in the mailing list.
> +#define PREFIX "wm8731: "
> +
Use dev_printk() functions, don't introduce things like this.
> * There is no point in caching the reset register
> */
> static const u16 wm8731_reg[WM8731_CACHEREGNUM] = {
> - 0x0097, 0x0097, 0x0079, 0x0079,
> - 0x000a, 0x0008, 0x009f, 0x000a,
> - 0x0000, 0x0000
> + 0x0097, 0x0097, 0x0079, 0x0079,
> + 0x000a, 0x0008, 0x009f, 0x000a,
> + 0x0000, 0x0000
Almost all of the patch actually appears to consist of a large number of
unrelated formatting and stylistic changes like this which aren't
mentioned in your changelog and making it hard for me to review. Please
rework this as a series of patches, each making a single change.
> static const struct snd_soc_dapm_route intercon[] = {
> @@ -265,8 +271,6 @@ static int wm8731_hw_params(struct snd_pcm_substream *substream,
> u16 srate = (coeff_div[i].sr << 2) |
> (coeff_div[i].bosr << 1) | coeff_div[i].usb;
>
> - wm8731_write(codec, WM8731_SRATE, srate);
> -
> /* bit size */
> switch (params_format(params)) {
> case SNDRV_PCM_FORMAT_S16_LE:
This is a separate functional change - it needs to be in a patch by
itself.
> + if (dir == SND_SOC_CLOCK_OUT) {
> + dev_dbg(codec->dev, PREFIX "%s turning on CLKOUT\n", __func__);
> + reg = wm8731_read_reg_cache(codec, WM8731_PWR);
> + reg &= ~(1 << WM8731_PWR_CLKOUTPD);
> + wm8731_write(codec, WM8731_PWR, reg);
> + }
> + else {
} else { please - I'm surprised checkpatch dosn't warn.
> + dev_dbg(codec->dev, PREFIX "%s turning off CLKOUT\n", __func__);
> + reg = wm8731_read_reg_cache(codec, WM8731_PWR);
> + reg |= (1 << WM8731_PWR_CLKOUTPD);
> + wm8731_write(codec, WM8731_PWR, reg);
> + }
It'd be better define a new clock for the CLKOUT pin rather than munging
it in with the master clock. MCLK is always an input on the WM8731, the
CLKOUT output is a separate pin and so including it in MCLK is likely to
make things confusing and could well introduce errors.
> -#warning "TODO: figure out how to turn on/off CLKOUT properly"
> - wm8731_write(codec, WM8731_PWR, 0);
> - break;
> case SND_SOC_BIAS_PREPARE:
> -#if 1
> -#warning "TODO: figure out how to turn on/off CLKOUT properly"
> - /* probably has to do with SND_SOC_CLOCK_OUT
> - and whether we're master or slave ... */
> - wm8731_write(codec, WM8731_PWR, 0);
> -#endif
> - break;
This patch isn't against mainline...
> + /* turn off CLKOUT in STANDBY */
> + if (level == SND_SOC_BIAS_STANDBY)
> + reg |= (1 << WM8731_PWR_CLKOUTPD);
> +
> + wm8731_write(codec, WM8731_PWR, reg);
If you're putting CLKOUT control in the hands of machine drivers give
them complete control over it, mixing management between the machine
drivers and the CODEC driver is likely to cause confusion.
> -#define WM8731_RATES (SNDRV_PCM_RATE_8000 | SNDRV_PCM_RATE_11025 |\
> - SNDRV_PCM_RATE_16000 | SNDRV_PCM_RATE_22050 |\
> - SNDRV_PCM_RATE_32000 | SNDRV_PCM_RATE_44100 |\
> - SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_88200 |\
> - SNDRV_PCM_RATE_96000)
> +#define WM8731_RATES (SNDRV_PCM_RATE_8000 | SNDRV_PCM_RATE_11025 | \
> + SNDRV_PCM_RATE_16000 | SNDRV_PCM_RATE_22050 | \
> + SNDRV_PCM_RATE_32000 | SNDRV_PCM_RATE_44100 | \
> + SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_88200 | \
> + SNDRV_PCM_RATE_96000)
SDNDRV_PCM_RATE_8000_96000.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
