Re: [PATCH 1/2] mfd: Core support for the WM8400 AudioPlus HiFi CODEC and PMU

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

On Wed, Sep 17, 2008 at 07:28:36PM -0700, Andrew Morton wrote:

> Various ankle-biting comments, just to show I read it:

Thanks.  Liam, I've just pushed out a version of the series fixing the
issues below.

> > +static struct
> > +{

> hm, checkpatch chews a couple minutes CPU time then misses this error.

Yeah, it seems to be doing something wildly inefficient.

> static struct {
> 
> please.

Done - spot the autogenerated code. :/

> } reg_data[] = {

> would be conventional, too.

Done also.

> > +	wm8400_read(wm8400, reg, 1, &val);
> > +
> > +	mutex_unlock(&wm8400->io_lock);
> > +
> > +	return val;
> > +}
> > +EXPORT_SYMBOL_GPL(wm8400_reg_read);

> Is it just me, or do all the useless newlines there look silly?  sigh.

I doubt it but once you get on to multi-line operations in the middle of
the lock (which some of the other functions do) I find it much clearer
so went for consistency.  I can change it if you like, though?

> > +#if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE)

> Is CONFIG_I2C=n worth supporting?  Is anyone likely to test and
> maintain that combination?

Right now it's silly to build the driver without I2C but once I push in
the SPI support for the device it's likely that some systems will want
to use the driver with I2C built out for the space saving.  I do intend
to support users doing that.

As far as test coverage goes it shouldn't make much difference if I2C is
built or not when the device is accessed via SPI - the I2C support will
be built but not run.

> > +struct wm8400 {

...

> > +	u16 reg_cache[WM8400_REGISTER_COUNT];

> Should this be __be16?

No, the cache is stored in CPU format.  In normal operation reads are
more common than writes.

> This patch adds an amazing 1733 #defines, of which only a few
> percent are used.  Oh well.

Yeah, we have a program which generates these #defines from data
extracted from the chip definition.  If it helps rather more (but most
likely not half) of them will be used once the remaining drivers for the
chip are pushed.  The current driver only covers a small proportion of
the functionality of the device.