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@xxxxxx>
> > ---
[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
http://www.open-technology.de/
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
