From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Morton Subject: Re: [patch 2.6.31-rc7 2/3] spi: McSPI saves CHCONFx too Date: Wed, 2 Sep 2009 09:53:47 -0700 Message-ID: <20090902095347.a6d82e95.akpm@linux-foundation.org> References: <200908272021.12070.david-b@pacbell.net> <20090901135726.96173f7e.akpm@linux-foundation.org> <200909020853.17212.david-b@pacbell.net> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: khilman-1D3HCaltpLuhEniVeURVKkEOCMrvLtNR@public.gmane.org, spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org, tero.kristo-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org To: David Brownell Return-path: In-Reply-To: <200909020853.17212.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: spi-devel-general-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org List-Id: linux-spi.vger.kernel.org On Wed, 2 Sep 2009 08:53:16 -0700 David Brownell wrote: > On Tuesday 01 September 2009, Andrew Morton wrote: > > On Thu, 27 Aug 2009 20:21:11 -0700 > > David Brownell wrote: > > > > > From: Tero Kristo > > > > > > Previous restore was lazy and only restored CHxCONF when it was needed by a > > > specific chip select. This could cause occasional errors on an SPI bus where > > > multiple chip selects are in use. > > > > > > Signed-off-by: Tero Kristo > > > Signed-off-by: Kevin Hilman > > > Signed-off-by: David Brownell > > > --- > > > drivers/spi/omap2_mcspi.c | 16 ++++++++++++++++ > > > 1 file changed, 16 insertions(+) > > > > > > --- a/drivers/spi/omap2_mcspi.c > > > +++ b/drivers/spi/omap2_mcspi.c > > > @@ -134,6 +134,7 @@ struct omap2_mcspi_cs { > > > void __iomem *base; > > > unsigned long phys; > > > int word_len; > > > + struct list_head node; > > > /* Context save and restore shadow register */ > > > u32 chconf0; > > > }; > > > @@ -145,6 +146,7 @@ struct omap2_mcspi_regs { > > > u32 sysconfig; > > > u32 modulctrl; > > > u32 wakeupenable; > > > + struct list_head cs; > > > }; > > > > Which locking protects these new lists? > > One's the head, one's the instance ... the list is used in > activation and deactivation paths, so the same locks which > already protect those paths. > Which locks are those? omap2_mcspi_work() calls omap2_mcspi_enable_clocks() under mcspi->lock omap2_mcspi_setup() calls omap2_mcspi_enable_clocks() not under mcspi->lock The locking here is insufficiently clear and possibly buggy. Please revisit. If it's missing, add it. If it's unneeded, document it. If it's actually present and correct, document it. ------------------------------------------------------------------------------ Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day trial. Simplify your report design, integration and deployment - and focus on what you do best, core application coding. Discover what's new with Crystal Reports now. http://p.sf.net/sfu/bobj-july