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

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

On Wed, Jan 20, 2010 at 08:50:59PM +0100, Guennadi Liakhovetski wrote:
> On Tue, 19 Jan 2010, Mark Brown wrote:

> > What do these comments refer to - do you mean to say that these are not
> > the actual chip register defaults?  The normal way to deal with those is

> No, I meant, that there's a "Value Update" bit in that register, usually 
> bit 8. So, that bit, of course, is not in default, but it was suggested to 
> me on IRC to set it in cache to force automatic value update.

Right, but it's better to set it in the cache by writing after the fact
rather than by changing the register defaults.  Like I say, that can
easily confuse things since sometimes people want to know the actual
default values.

> 
> > > +#define ARRAY_SINGLE(xreg, xshift, xtexts) SOC_ENUM_SINGLE(xreg, xshift, \
> > > +						ARRAY_SIZE(xtexts), xtexts)
> > 
> > This looks generic - please namespace it and add it to the header file,
> > other drivers can benefit from it.
> 
> SOC_ARRAY_SINGLE()?

How about SOC_ENUM_ARRAY() since it's really only useful for arrays?

> > > +#define MIXER_ARRAY(n, r, s, i, m) SND_SOC_DAPM_MIXER(n, r, s, i, \
> > > +							m, ARRAY_SIZE(m))

> > Again, this should be done somewhere generic.  Probably by going through
> > and changing the SND_SOC_DAPM_MIXER definition.

> Well, having discussed this privately and having looked at the heades, I'm 
> not sure, this is a good idea. First, there are

> SND_SOC_DAPM_PGA()
> SND_SOC_DAPM_MIXER()
> SND_SOC_DAPM_MIXER_NAMED_CTL()
> SND_SOC_DAPM_PGA_E()
> SND_SOC_DAPM_MIXER_E()
> SND_SOC_DAPM_MIXER_NAMED_CTL_E()

> - 6 macros that follow the pattern

> .kcontrols = wcontrols, .num_kcontrols = wncontrols,

That doesn't mean it's a good idea, though for PGAs note that the normal
case is that the array is omitted (and actually, thinking about this we
need an array omitted case for everything anyway).

> Changing only one of them would make the headers and the API look 
> inconsistent. Changing all of them... I really don't think we want to 
> endeavor this now, at least not me;) Besides, doing this locally is very 
> convenient, and is available to everyone - there's no space science 
> involved with this. One can be even as evil as doing

For maintainability of the subsystem it's really nice if a driver doing
bog standard things looks like it's doing bog standard things - neat
local tricks break the pattern recognition that helps a lot when looking
at multiple drivers.

> > > +	u16 power1 = snd_soc_read(codec, WM8978_POWER_MANAGEMENT_1) & ~3;
> > 
> > This bitmask maintains everything except the two LSB...
> > 
> > > +	switch (level) {
> > > +	case SND_SOC_BIAS_ON:
> > > +	case SND_SOC_BIAS_PREPARE:
> > > +		power1 |= 1;  /* VMID 75k */
> > > +		snd_soc_write(codec, WM8978_POWER_MANAGEMENT_1, power1);
> > > +		break;
> > > +	case SND_SOC_BIAS_STANDBY:
> > > +		power1 |= 0xC;
> > 
> > ...but this is also managing other bits.

> Right, bits 7-6 are either kept or set, nothing wrong with that.

That seems wrong - if they're being managed in the bias level
configuration they ought to be being turned off at some point.  If they
should be set all the time then set them during chip init.