From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755714AbYIRK3v (ORCPT ); Thu, 18 Sep 2008 06:29:51 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1752503AbYIRK3n (ORCPT ); Thu, 18 Sep 2008 06:29:43 -0400 Received: from opensource.wolfsonmicro.com ([80.75.67.52]:42478 "EHLO opensource2.wolfsonmicro.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751626AbYIRK3n (ORCPT ); Thu, 18 Sep 2008 06:29:43 -0400 Date: Thu, 18 Sep 2008 11:29:40 +0100 From: Mark Brown To: Andrew Morton Cc: Liam Girdwood , Samuel Ortiz , linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/2] mfd: Core support for the WM8400 AudioPlus HiFi CODEC and PMU Message-ID: <20080918102940.GA29719@rakim.wolfsonmicro.main> References: <20080916094201.GA15271@rakim.wolfsonmicro.main> <1221558194-15505-1-git-send-email-broonie@opensource.wolfsonmicro.com> <20080917192836.ef3545d2.akpm@linux-foundation.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20080917192836.ef3545d2.akpm@linux-foundation.org> X-Cookie: We are what we are. User-Agent: Mutt/1.5.18 (2008-05-17) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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.