Re: [PATCHv3 1/4] mc13xxx-core: Consolidate common code to prepare for separate i2c and spi

From: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
To: "Uwe Kleine-König"
	<u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Cc: Marc Reilly <marc-DtE7ei5U7Kg0n/F98K4Iww@public.gmane.org>,
	spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org,
	w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
	linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Subject: Re: [PATCHv3 1/4] mc13xxx-core: Consolidate common code to prepare for separate i2c and spi
Date: Wed, 29 Dec 2010 00:40:33 -0700	[thread overview]
Message-ID: <20101229074033.GJ8172@angua.secretlab.ca> (raw)
In-Reply-To: <20101220083120.GP1940-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>

Hi Marc and Uwe,

On Mon, Dec 20, 2010 at 09:31:20AM +0100, Uwe Kleine-König wrote:
> On Mon, Dec 20, 2010 at 02:50:52PM +1100, Marc Reilly wrote:
> > This patch moves common mc13xxx definitions to the header file in
> > preparation for separate i2c and spi driver modules.
> > spi specific functions are also removed.
> > 
> > Changes to the mc13xxx struct are:
> > removing the redundant irq member,
> > adding driver read/write function ptrs,
> > adding ictype
> This list isn't complete, but see below.
> 
> I don't like that after this patch the driver isn't functional, because
> you removed the spi functionality which breaks bisection.

Ugh, yes.  Breaking bisection is absolutely a no-no.  I'll wait for
the next version of this series before I do a full review.

> 
> > Signed-off-by: Marc Reilly <marc-DtE7ei5U7Kg0n/F98K4Iww@public.gmane.org>
> > ---
> >  drivers/mfd/mc13xxx-core.c  |  207 +++++++-----------------------------------
> >  include/linux/mfd/mc13xxx.h |   77 +++++++++++-----
> >  2 files changed, 87 insertions(+), 197 deletions(-)
> > 
> > diff --git a/drivers/mfd/mc13xxx-core.c b/drivers/mfd/mc13xxx-core.c
> > index a2ac2ed..3367a40 100644
> > --- a/drivers/mfd/mc13xxx-core.c
> > +++ b/drivers/mfd/mc13xxx-core.c
> > @@ -9,24 +9,14 @@
> >   * the terms of the GNU General Public License version 2 as published by the
> >   * Free Software Foundation.
> >   */
> > -
> (hmm.  I think there is no style guide for that, but I know people who
> think that this nl should be there.  So IMHO don't touch this.)

A better comment is: don't make unrelated, non-functional whitespace
changes in patches for something else.  They add noise that make it
harder to review.

Cheers,
g.