From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mark Brown Subject: Re: [PATCH 06/10 V2] spi: Add SPI driver for mx233/mx28 Date: Wed, 1 Aug 2012 21:31:07 +0100 Message-ID: <20120801203106.GW4483@opensource.wolfsonmicro.com> References: <1343076052-27312-1-git-send-email-marex@denx.de> <1343076052-27312-7-git-send-email-marex@denx.de> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: Fabio Estevam , Shawn Guo , Attila Kinali , spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org, Chris Ball , Dong Aisheng , Linux ARM kernel To: Marek Vasut Return-path: Content-Disposition: inline In-Reply-To: <1343076052-27312-7-git-send-email-marex-ynQEQJNshbs@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 Mon, Jul 23, 2012 at 10:40:48PM +0200, Marek Vasut wrote: > This is slightly reworked version of the SPI driver. > Support for DT has been added and it's been converted > to queued API. Looks reasonable overall. > + bits_per_word = dev->bits_per_word; > + if (t && t->bits_per_word) > + bits_per_word = t->bits_per_word; > + > + if (bits_per_word != 8) { > + dev_err(&dev->dev, "%s, unsupported bits_per_word=%d\n", > + __func__, bits_per_word); > + return -EINVAL; > + } > + if (dev->max_speed_hz) > + hz = dev->max_speed_hz; > + if (t && t->speed_hz) > + hz = t->speed_hz; > + if (hz == 0) { > + dev_err(&dev->dev, "Cannot continue with zero clock\n"); > + return -EINVAL; > + } These two blocks use a different style (the first does the first assign unconditionally, the second uses initialisation with declaration). I prefer the first style but YMMV and it doesn't matter. For the missing clock rate might it make sense to just use whatever the clock happens to come out as? > +static void mxs_spi_cleanup(struct spi_device *dev) > +{ > + return; > +} Empty functions are generally a warning sign... why do we need this one? > +static int __devexit mxs_spi_remove(struct platform_device *pdev) > +{ > + clk_disable_unprepare(ssp->clk); It'd be nice to only keep the clocks enabled while doing transfers but again totally non-essential. ------------------------------------------------------------------------------ Live Security Virtual Conference Exclusive live event will cover all the ways today's security and threat landscape has changed and how IT managers can respond. Discussions will include endpoint security, mobile security and the latest in malware threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/