[alsa-devel] [PATCH] ASoC: Add max98088 CODEC driver

[Peter Hsiang]

On Wed, Sep 29, 2010, Mark Brown wrote:
> On Tue, Sep 28, 2010 at 07:34:34PM -0700, Peter Hsiang wrote:
> 
> > +#define EQ_CFG_MAX 32
> 
> There doesn't seem to be any need to hard code a limit here?

Are you requiring the implementation be exactly the same way you did?

> 
> > +/*
> > + * For kernels compiled without unsigned long long int division
> > + */
> > +unsigned long long int ulldiv(unsigned long long int dividend,
> > +                             unsigned long long int divisor)
> 
> This is causing me some concern.  Even if it is required this does not
> look like something that should be part of a specific driver - what
> happens if some other driver needs the same thing?

You are more than welcome to use this in your driver as well :)
Just use it.  Sounds like you are suggesting to have it located in a
central location outside of the codec driver?
Would you like that in soc-core.c?  Let me know.

> 
> > +/*
> > + * The INx1 and INx2 PGAs share a power control signal.
> > + * This function OR's the two power events to keep an unpowered INx
> > + * from turning off it's counterpart.
> > + * The control names are used to identify the PGA.
> > + */
> 
> This...
> 
> > +       if (strncmp(w->name, "INA1", 4) == 0) {
> > +               pga = INA1_PGA_BIT;
> > +               mask = INA1_PGA_BIT | INA2_PGA_BIT;
> 
> ...doesn't seem to correspond to this, at least with the normal way mask
> is used.  Apart from anything else it looks like you have individual
> mask bits for each of the INx1 and INx2 PGAs (since you can define mask
> bits for each of them).  It would be much clearer if the code made it
> obvious that these aren't register bits but rather that you're storing
> the data in a register-like bitfield in a variable.  I can't help but
> think that a reference count would be much clearer.

There are more than one way to solve a problem, and they all can be
correct.
Would additional comments to clarify that this is not register bits
satisfy your requirement?

> 
> > +       if (event == SND_SOC_DAPM_POST_PMU) {
> > +               /* ON */
> > +               *state |= pga;
> > +
> > +               /* Turn on, avoiding unnecessary writes */
> > +               val = snd_soc_read(codec, w->reg);
> > +               if (!(val & (1 << w->shift))) {
> > +                       val |= (1 << w->shift);
> > +                       snd_soc_write(codec, w->reg, val);
> > +               }
> 
> snd_soc_update_bits() will suppress unwanted register writes.