Re: [alsa-devel] [PATCH 1/4] ASoC: add a WM8978 codec driver

Guennadi Liakhovetski
2010-01-20T20:01:46Z

On Tue, 19 Jan 2010, Liam Girdwood wrote:

> Looks ok, some questions below.
>
> On Tue, 2010-01-19 at 09:08 +0100, Guennadi Liakhovetski wrote:
> > The WM8978 codec from Wolfson Microelectronics is very similar to wm8974, but
> > is stereo and also has some differences in pin configuration and internal
> > signal routing. This driver is based on wm8974 and takes the differences into
> > account.
> >
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > ---

[snip]

> > +/* OUT1 - HeadPhones */
> > +SOC_SINGLE("Left HeadPhone Playback ZC Switch",
> > + WM8978_LOUT1_HP_CONTROL, 7, 1, 0),
>
> HeadPhone is usually "Headphone"

Ok

> > +#define MIXER_ARRAY(n, r, s, i, m) SND_SOC_DAPM_MIXER(n, r, s, i, \
> > + m, ARRAY_SIZE(m))
>
> I'd be tempted to rename this and add to soc-dapm.h

Please, see my reply to Mark's review.

> > +static void pll_factors(struct wm8978_pll_div *pll_div, unsigned int target,
> > + unsigned int source)
> > +{
> > + u64 Kpart;
> > + unsigned int K, Ndiv, Nmod;
> > +
> > + Ndiv = target / source;
> > + if (Ndiv < 6) {
> > + source >>= 1;
> > + pll_div->div2 = 1;
> > + Ndiv = target / source;
> > + } else {
> > + pll_div->div2 = 0;
> > + }
> > +
>
> I would personally not put the extra { here

That's also what I did a couple of years ago, but this contradicts the
kernel CodingStyle, and, I think, checkpatch would complain too.

> > + snd_soc_write(codec, WM8978_AUDIO_INTERFACE, iface_ctl);
> > + snd_soc_write(codec, WM8978_ADDITIONAL_CONTROL, add_ctl);
> > +
> > + /* Mic bias */
> > + snd_soc_write(codec, WM8978_POWER_MANAGEMENT_1,
> > + (snd_soc_read(codec, 1) & ~4) | 0x10);
> > +
> > + /* Out-1 enabled, left/right input channel enabled */
> > + snd_soc_write(codec, WM8978_POWER_MANAGEMENT_2, 0x1bf);
> > +
> > + /* Out-2 disabled, right/left output channel enabled, dac enabled */
> > + snd_soc_write(codec, WM8978_POWER_MANAGEMENT_3, 0x10f);
>
> Power stuff should be in either dapm or bias functions.

Yes, still working on this.

> > +#define WM8978_LOUT2_SPK_CONTROL 0x36
> > +#define WM8978_ROUT2_SPK_CONTROL 0x37
> > +#define WM8978_OUT3_MIXER_CONTROL 0x38
> > +#define WM8978_OUT4_MIXER_CONTROL 0x39
> > +
> > +#define WM8978_CACHEREGNUM 58
> > +
>
> It would be nice to have the relevant bits defined here for set_fmt()
> etc instead of just the magic numbers used in the above codec driver.

As I explained privately, I agree, that using names instead of bits helps
- but (mostly) only where those bits are reused multiple times in the
code. If you only have to initialise a register once with some bitmask, I
think, code like

/* Enable input X, output Y, set default W polarity to Z */
__raw_writel(0x123, reg);

looks better than

__raw_writel(CHIP_INPUT_X_ENABLE | CHIP_OUTPUT_Y_ENABLE |
CHIP_SIGNAL_W_POLARITY_Z, reg);

so, unless there strong preferences in ALSA world, I'll try to combine
both. Let me know if this contradicts the common ALSA style.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer