LKML Archive on
help / color / mirror / Atom feed
From: Mark Brown <>
To: Andrew Morton <>
Cc: Liam Girdwood <>,
	Samuel Ortiz <>,
Subject: Re: [PATCH 1/2] mfd: Core support for the WM8400 AudioPlus HiFi CODEC and PMU
Date: Thu, 18 Sep 2008 11:29:40 +0100	[thread overview]
Message-ID: <20080918102940.GA29719@rakim.wolfsonmicro.main> (raw)
In-Reply-To: <>

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.

  reply	other threads:[~2008-09-18 10:29 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-09-16  9:42 [PATCH 0/2] WM8400 support v2 Mark Brown
2008-09-16  9:43 ` [PATCH 1/2] mfd: Core support for the WM8400 AudioPlus HiFi CODEC and PMU Mark Brown
2008-09-16  9:43   ` [PATCH 2/2] regulator: Add WM8400 regulator support Mark Brown
2008-09-18  2:28   ` [PATCH 1/2] mfd: Core support for the WM8400 AudioPlus HiFi CODEC and PMU Andrew Morton
2008-09-18 10:29     ` Mark Brown [this message]
2008-09-22 18:33   ` Samuel Ortiz
2008-09-23  0:27     ` Liam Girdwood
  -- strict thread matches above, loose matches on Subject: below --
2008-09-10 18:28 [PATCH 0/2] WM8400 support Mark Brown
2008-09-10 18:28 ` [PATCH 1/2] mfd: Core support for the WM8400 AudioPlus HiFi CODEC and PMU Mark Brown
2008-09-10 18:47   ` Liam Girdwood
2008-09-10 19:13     ` Mark Brown
2008-09-10 19:38       ` Liam Girdwood
2008-09-11 10:02         ` Mark Brown
2008-09-12 10:54   ` Samuel Ortiz
2008-09-12 12:05     ` Mark Brown

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20080918102940.GA29719@rakim.wolfsonmicro.main \ \ \ \ \ \
    --subject='Re: [PATCH 1/2] mfd: Core support for the WM8400 AudioPlus HiFi CODEC and PMU' \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).