diff mbox

[6/6] DaVinci: Adding SPI driver for DM3xx/DM6467/DA8xx

Message ID 200907140954.22401.david-b@pacbell.net (mailing list archive)
State Superseded
Headers show

Commit Message

David Brownell July 14, 2009, 4:54 p.m. UTC 
On Monday 13 July 2009, s-paulraj@ti.com wrote:
> The patch adds support for SPI driver for DaVinci
> DM355/DM365/DM6467 and DA8xx

Comments-in-the-form-of-a-patch below ... I figured this
could expedite things.  :)

- Dave

Comments

s-paulraj@ti.com July 14, 2009, 5:43 p.m. UTC | #1 
David,

Thanks for the patch. I'll trying to send updates ASAP.

I have just a few questions though.

IS it OK if I continue to use DAVINCI_ in my #defines.

I think we should because these are used only in DaVinci.
I had a look at the mmc driver for DaVinci and it is full of such #define with DAVINCI_

And a few more below

> -----Original Message-----
> From: David Brownell [mailto:david-b@pacbell.net]
> Sent: Tuesday, July 14, 2009 12:54 PM
> To: davinci-linux-open-source@linux.davincidsp.com
> Cc: Paulraj, Sandeep
> Subject: Re: [PATCH 6/6] DaVinci: Adding SPI driver for DM3xx/DM6467/DA8xx
> 
> On Monday 13 July 2009, s-paulraj@ti.com wrote:
> > The patch adds support for SPI driver for DaVinci
> > DM355/DM365/DM6467 and DA8xx
> 
> Comments-in-the-form-of-a-patch below ... I figured this
> could expedite things.  :)
> 
> - Dave
> 
> ===============================
> Various bits of cleanup for the davinci_spi driver:
> 
>  - Kconfig should never have "default y"
>  - Reorder #includes ... standard stuff should go first
>  - Remove pointless inlines
>  - Remove needless (and sometimes unused) #defines
>  - Declare the special SPI_* modes which are supported!
>  - Whitespace fixes
>  - Update busy-wait loops
>  - Streamline op_mode setup
>  - ... more
> 
> NOT ADDRESSED:
> 
>  - The printk(KERN_ERR ...) messages that should be dev_dbg(...)
[Sandeep] I'll address
>  - The use of "-1" error codes instead of negative errno
[Sandeep] What would be the most appropriate error value.

What if I use -EADV that is for "advertise error".

If it is not appropriate then can you suggest a more appropriate return value

>  - Timing out busy-wait loops
[Sandeep] OK
>  - Adding a comment explaining that either built-in chipselects
>    *or* GPIO versions may be used.  (If GPIO, can up to 4 slaves
>    be supported?)
[Sandeep] The most ideal place for this explanation would be in a wiki page at wiki.davincidsp.com.

But I will still add an explanation for this in dm355.c and dm365.c and not in the driver.

> 
> ---
>  drivers/spi/Kconfig       |    1
>  drivers/spi/davinci_spi.c |  142 ++++++++++++++++------------------------
> ----
>  drivers/spi/davinci_spi.h |   42 +------------
>  3 files changed, 59 insertions(+), 126 deletions(-)
> 
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -81,7 +81,6 @@ config SPI_DAVINCI
>  	tristate "SPI controller driver for DaVinci SoC"
>  	depends on SPI_MASTER && ARCH_DAVINCI
>  	select SPI_BITBANG
> -	default y
>  	help
>  	  SPI master controller for DaVinci SPI modules.
> 
> --- a/drivers/spi/davinci_spi.c
> +++ b/drivers/spi/davinci_spi.c
> @@ -17,11 +17,10 @@
>   * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307
> USA
>   */
> 
> -#include "davinci_spi.h"
> +#include <linux/types.h>
> +#include <linux/io.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/io.h>
> -#include <mach/mux.h>
> -#include <mach/gpio.h>
>  #include <linux/interrupt.h>
>  #include <linux/module.h>
>  #include <linux/device.h>
> @@ -29,9 +28,17 @@
>  #include <linux/platform_device.h>
>  #include <linux/err.h>
>  #include <linux/clk.h>
> +#include <linux/gpio.h>
> 
> -static inline void davinci_spi_rx_buf_u8(u32 data,
> -					 struct davinci_spi *davinci_spi)
> +#include <linux/spi/spi.h>
> +#include <linux/spi/spi_bitbang.h>
> +
> +#include <mach/spi.h>
> +
> +#include "davinci_spi.h"
> +
> +
> +static void davinci_spi_rx_buf_u8(u32 data, struct davinci_spi
> *davinci_spi)
>  {
>  	u8 *rx = davinci_spi->rx;
> 
> @@ -39,8 +46,7 @@ static inline void davinci_spi_rx_buf_u8
>  	davinci_spi->rx = rx;
>  }
> 
> -static inline void davinci_spi_rx_buf_u16(u32 data,
> -					  struct davinci_spi *davinci_spi)
> +static void davinci_spi_rx_buf_u16(u32 data, struct davinci_spi
> *davinci_spi)
>  {
>  	u16 *rx = davinci_spi->rx;
> 
> @@ -48,7 +54,7 @@ static inline void davinci_spi_rx_buf_u1
>  	davinci_spi->rx = rx;
>  }
> 
> -static inline u32 davinci_spi_tx_buf_u8(struct davinci_spi *davinci_spi)
> +static u32 davinci_spi_tx_buf_u8(struct davinci_spi *davinci_spi)
>  {
>  	u32 data;
>  	const u8 *tx = davinci_spi->tx;
> @@ -58,7 +64,7 @@ static inline u32 davinci_spi_tx_buf_u8(
>  	return data;
>  }
> 
> -static inline u32 davinci_spi_tx_buf_u16(struct davinci_spi *davinci_spi)
> +static u32 davinci_spi_tx_buf_u16(struct davinci_spi *davinci_spi)
>  {
>  	u32 data;
>  	const u16 *tx = davinci_spi->tx;
> @@ -71,6 +77,7 @@ static inline u32 davinci_spi_tx_buf_u16
>  static inline void set_bits(void __iomem *addr, u32 bits)
>  {
>  	u32 v = ioread32(addr);
> +
>  	v |= bits;
>  	iowrite32(v, addr);
>  }
> @@ -78,6 +85,7 @@ static inline void set_bits(void __iomem
>  static inline void clear_bits(void __iomem *addr, u32 bits)
>  {
>  	u32 v = ioread32(addr);
> +
>  	v &= ~bits;
>  	iowrite32(v, addr);
>  }
> @@ -114,7 +122,7 @@ static void davinci_spi_chipselect(struc
>  		if (pdata->chip_sel != NULL) {
>  			for (i = 0; i < pdata->num_chipselect; i++) {
>  				if (pdata->chip_sel[i] != DAVINCI_SPI_INTERN_CS)
> -					__gpio_set_value(pdata->chip_sel[i], 1);
> +					gpio_set_value(pdata->chip_sel[i], 1);
>  			}
>  		}
> 
> @@ -123,10 +131,9 @@ static void davinci_spi_chipselect(struc
>  		data1_reg_val |= CS_DEFAULT << DAVINCI_SPIDAT1_CSNR_SHIFT;
>  		iowrite32(data1_reg_val, davinci_spi->base + SPIDAT1);
> 
> -		while (1)
> -			if (ioread32(davinci_spi->base + SPIBUF)
> -					& DAVINCI_SPIBUF_RXEMPTY_MASK)
> -				break;
> +		while ((ioread32(davinci_spi->base + SPIBUF)
> +					& DAVINCI_SPIBUF_RXEMPTY_MASK) == 0)
> +			cpu_relax();
>  	}
>  }
> 
> @@ -309,67 +316,21 @@ static int davinci_spi_bufs_prep(struct
>  		iowrite32(DAVINCI_SPI_INTLVL_0, davinci_spi->base + SPILVL);
> 
>  	/*
> -	 * The driver uses new flags SPI_NO_CS and SPI_RAEDY
> -	 * These can be found in /include/linux/spi/spi.h
> -	 * Standard SPI mode is a 4 pin variant
> -	 * 3 pin SPI is actually the 4 pin variant with no CS(SPI_NO_CS)
> +	 * Standard SPI mode uses 4 pins, with chipselect
> +	 * 3 pin SPI is a 4 pin variant without CS (SPI_NO_CS)
> +	 *	(distinct from SPI_3WIRE, with just one data wire;
> +	 *	or similar variants without MOSI or without MISO)
>  	 * 4 pin with enable is (SPI_READY | SPI_NO_CS)
> -	 * 5 pin SPI variant is SPI_READY
> -	 * Operation mode(op_mode) depends on these flags
> -	 * op_mode 0 = standard 4 pin mode
> -	 * op_mode 1 = 3 pin mode
> -	 * op_mode 2 = 5 pin mode
> -	 * op_mode 3 = 4 pin mode with enable
> +	 * 5 pin SPI variant is standard SPI plus SPI_READY
>  	 */
> 
> -	op_mode = ((spi->mode & (SPI_NO_CS | SPI_READY)) >> 6);
> -
> -	switch (op_mode) {
> -	case 0:
> -		op_mode = (DAVINCI_SPIPC0_DIFUN_DI <<
> -				DAVINCI_SPIPC0_DIFUN_SHIFT)
> -			| (DAVINCI_SPIPC0_DOFUN_DO <<
> -				DAVINCI_SPIPC0_DOFUN_SHIFT)
> -			| (DAVINCI_SPIPC0_CLKFUN_CLK <<
> -				DAVINCI_SPIPC0_CLKFUN_SHIFT)
> -			| (1 << spi->chip_select);
> -		break;
> -
> -	case 1:
> -		op_mode = (DAVINCI_SPIPC0_DIFUN_DI <<
> -				DAVINCI_SPIPC0_DIFUN_SHIFT)
> -			| (DAVINCI_SPIPC0_DOFUN_DO <<
> -				DAVINCI_SPIPC0_DOFUN_SHIFT)
> -			| (DAVINCI_SPIPC0_CLKFUN_CLK <<
> -				DAVINCI_SPIPC0_CLKFUN_SHIFT);
> -		break;
> -
> -	case 2:
> -		op_mode = (DAVINCI_SPIPC0_DIFUN_DI <<
> -				DAVINCI_SPIPC0_DIFUN_SHIFT)
> -			| (DAVINCI_SPIPC0_DOFUN_DO <<
> -				DAVINCI_SPIPC0_DOFUN_SHIFT)
> -			| (DAVINCI_SPIPC0_CLKFUN_CLK <
> -				DAVINCI_SPIPC0_CLKFUN_SHIFT)
> -			| (DAVINCI_SPIPC0_SPIENA <<
> -				DAVINCI_SPIPC0_SPIENA_SHIFT)
> -			| (1 << spi->chip_select);
> -		break;
> -
> -	case 3:
> -		op_mode = (DAVINCI_SPIPC0_DIFUN_DI <<
> -				DAVINCI_SPIPC0_DIFUN_SHIFT)
> -			| (DAVINCI_SPIPC0_DOFUN_DO <<
> -				DAVINCI_SPIPC0_DOFUN_SHIFT)
> -			| (DAVINCI_SPIPC0_CLKFUN_CLK <<
> -				DAVINCI_SPIPC0_CLKFUN_SHIFT)
> -			| (DAVINCI_SPIPC0_SPIENA <<
> -				DAVINCI_SPIPC0_SPIENA_SHIFT);
> -		break;
> -
> -	default:
> -		return -1;
> -	}
> +	op_mode = DAVINCI_SPIPC0_DIFUN_MASK
> +		| DAVINCI_SPIPC0_DOFUN_MASK
> +		| DAVINCI_SPIPC0_CLKFUN_MASK;
> +	if (!(spi->mode & SPI_NO_CS))
> +		op_mode |= 1 << spi->chip_select;
> +	if (spi->mode & SPI_READY)
> +		op_mode |= DAVINCI_SPIPC0_SPIENA_MASK;
> 
>  	iowrite32(op_mode, davinci_spi->base + SPIPC0);
> 
> @@ -475,16 +436,15 @@ static int davinci_spi_bufs_pio(struct s
>  	/* checking for GPIO */
>  	if ((pdata->chip_sel != NULL) &&
>  	    (pdata->chip_sel[spi->chip_select] != DAVINCI_SPI_INTERN_CS))
> -		__gpio_set_value(pdata->chip_sel[spi->chip_select], 0);
> +		gpio_set_value(pdata->chip_sel[spi->chip_select], 0);
>  	 else
>  		clear_bits(davinci_spi->base + SPIDEF, ~tmp);
> 
>  	data1_reg_val |= tmp << DAVINCI_SPIDAT1_CSNR_SHIFT;
> 
> -	while (1)
> -		if (ioread32(davinci_spi->base + SPIBUF)
> -				& DAVINCI_SPIBUF_RXEMPTY_MASK)
> -			break;
> +	while ((ioread32(davinci_spi->base + SPIBUF)
> +				& DAVINCI_SPIBUF_RXEMPTY_MASK) == 0)
> +		cpu_relax();
> 
>  	/* Determine the command to execute READ or WRITE */
>  	if (t->tx_buf) {
> @@ -505,7 +465,8 @@ static int davinci_spi_bufs_pio(struct s
>  			}
>  			while (ioread32(davinci_spi->base + SPIBUF)
>  					& DAVINCI_SPIBUF_RXEMPTY_MASK)
> -				udelay(1);
> +				cpu_relax();
> +
>  			/* getting the returned byte */
>  			if (t->rx_buf) {
>  				buf_val = ioread32(davinci_spi->base + SPIBUF);
> @@ -519,13 +480,13 @@ static int davinci_spi_bufs_pio(struct s
>  			while (1) {
>  				/* keeps the serial clock going */
>  				if ((ioread32(davinci_spi->base + SPIBUF)
> -					& DAVINCI_SPIBUF_TXFULL_MASK) == 0)
> +						& DAVINCI_SPIBUF_TXFULL_MASK) == 0)
>  					iowrite32(data1_reg_val,
>  						davinci_spi->base + SPIDAT1);
> 
> -				while ((ioread32(davinci_spi->base + SPIBUF)) &
> -					(DAVINCI_SPIBUF_RXEMPTY_MASK))
> -						;
> +				while (ioread32(davinci_spi->base + SPIBUF) &
> +						DAVINCI_SPIBUF_RXEMPTY_MASK)
> +					cpu_relax();
> 
>  				flg_val = ioread32(davinci_spi->base + SPIFLG);
>  				buf_val = ioread32(davinci_spi->base + SPIBUF);
> @@ -549,8 +510,8 @@ static int davinci_spi_bufs_pio(struct s
>  						davinci_spi->base + SPIDAT1);
> 
>  				while (ioread32(davinci_spi->base + SPIINT) &
> -					(DAVINCI_SPIINT_RX_INTR))
> -					;
> +						DAVINCI_SPIINT_RX_INTR)
> +					cpu_relax();
>  			}
>  			iowrite32((data1_reg_val & 0x0ffcffff),
>  					davinci_spi->base + SPIDAT1);
> @@ -659,7 +620,7 @@ static int davinci_spi_probe(struct plat
>  	davinci_spi->region_size = resource_size(r);
>  	davinci_spi->pdata = pdata;
> 
> -	/* initialize gpio used as chip select */
> +	/* initialize any gpio lines used as chip select */
>  	if (pdata->chip_sel != NULL) {
>  		for (i = 0; i < pdata->num_chipselect; i++) {
>  			if (pdata->chip_sel[i] != DAVINCI_SPI_INTERN_CS) {
> @@ -690,7 +651,7 @@ static int davinci_spi_probe(struct plat
>  	}
> 
>  	ret = request_irq(davinci_spi->irq, davinci_spi_irq, IRQF_DISABLED,
> -			  pdev->name, davinci_spi);
> +			  dev_name(&pdev->dev), davinci_spi);
>  	if (ret != 0) {
>  		ret = -EAGAIN;
>  		goto unmap_io;
> @@ -716,8 +677,12 @@ static int davinci_spi_probe(struct plat
> 
>  	davinci_spi->bitbang.chipselect = davinci_spi_chipselect;
>  	davinci_spi->bitbang.setup_transfer = davinci_spi_setup_transfer;
> -
>  	davinci_spi->bitbang.txrx_bufs = davinci_spi_bufs_pio;
> +
> +	davinci_spi->bitbang.flags = SPI_LSB_FIRST | SPI_LOOP;
> +	if (davinci_spi->version == DAVINCI_SPI_VERSION_2)
> +		davinci_spi->bitbang.flags |= SPI_NO_CS | SPI_READY;
> +
>  	davinci_spi->version = pdata->version;
>  	davinci_spi->get_rx = davinci_spi_rx_buf_u8;
>  	davinci_spi->get_tx = davinci_spi_tx_buf_u8;
> @@ -733,8 +698,7 @@ static int davinci_spi_probe(struct plat
>  	if (ret != 0)
>  		goto free_clk;
> 
> -	printk(KERN_NOTICE "Davinci SPI Controller driver at "
> -		"0x%p (irq = %d) \n",
> +	dev_info(&pdev->dev,"Controller at 0x%p (irq = %d) \n",
>  		davinci_spi->base, davinci_spi->irq);
> 
>  	return ret;
> --- a/drivers/spi/davinci_spi.h
> +++ b/drivers/spi/davinci_spi.h
> @@ -19,13 +19,6 @@
>  #ifndef __DAVINCI_SPI_H
>  #define __DAVINCI_SPI_H
> 
> -#include <linux/types.h>
> -#include <linux/io.h>
> -#include <linux/clk.h>
> -#include <linux/spi/spi.h>
> -#include <linux/spi/spi_bitbang.h>
> -#include <mach/spi.h>
> -
>  #define DAVINCI_SPI_MAX_CHIPSELECT	2
> 
>  #define CS_DEFAULT	0xFF
> @@ -53,33 +46,12 @@
>  #define DAVINCI_SPIGCR1_SPIENA_MASK	0x01000000u
> 
>  /* SPIPC0 */
> -#define DAVINCI_SPIPC0_DIFUN_MASK	BIT(11)
> -#define DAVINCI_SPIPC0_DIFUN_SHIFT	11
> -
> -/* DIFUN */
> -#define DAVINCI_SPIPC0_DIFUN_DI		BIT(0)
> -
> -/* DOFUN */
> -#define DAVINCI_SPIPC0_DOFUN_MASK	BIT(10)
> -#define DAVINCI_SPIPC0_DOFUN_SHIFT	10
> -#define DAVINCI_SPIPC0_DOFUN_DO		BIT(0)
> -
> -/* CLKFUN */
> -#define DAVINCI_SPIPC0_CLKFUN_MASK	BIT(9)
> -#define DAVINCI_SPIPC0_CLKFUN_SHIFT	9
> -#define DAVINCI_SPIPC0_CLKFUN_CLK	BIT(0)
> -
> -/* EN1FUN */
> +#define DAVINCI_SPIPC0_DIFUN_MASK	BIT(11)		/* MISO */
> +#define DAVINCI_SPIPC0_DOFUN_MASK	BIT(10)		/* MOSI */
> +#define DAVINCI_SPIPC0_CLKFUN_MASK	BIT(9)		/* CLK */
> +#define DAVINCI_SPIPC0_SPIENA_MASK	BIT(8)		/* nREADY */
>  #define DAVINCI_SPIPC0_EN1FUN_MASK	BIT(1)
> -#define DAVINCI_SPIPC0_EN1FUN_EN1	BIT(0)
> -
> -/* EN0FUN Tokens */
>  #define DAVINCI_SPIPC0_EN0FUN_MASK	BIT(0)
> -#define DAVINCI_SPIPC0_EN0FUN_SHIFT	0
> -#define DAVINCI_SPIPC0_EN0FUN_EN0	BIT(0)
> -
> -#define DAVINCI_SPIPC0_SPIENA		BIT(0)
> -#define DAVINCI_SPIPC0_SPIENA_SHIFT	8
> 
>  #define DAVINCI_SPIINT_MASKALL		0x0101035F
>  #define DAVINCI_SPI_INTLVL_1		0x000001FFu
> @@ -127,8 +99,6 @@
>  #define DAVINCI_SPIINT_DMA_REQ_EN	BIT(16)
>  #define DAVINCI_SPIINT_ENABLE_HIGHZ	BIT(24)
> 
> -#define DAVINCI_BYTELENGTH 8u
> -
>  #define DAVINCI_SPI_T2CDELAY_SHIFT	16
>  #define DAVINCI_SPI_C2TDELAY_SHIFT	24
> 
> @@ -171,7 +141,7 @@ struct davinci_spi_slave {
>  struct davinci_spi {
>  	/* bitbang has to be first */
>  	struct spi_bitbang	bitbang;
> -	struct clk 		*clk;
> +	struct clk		*clk;
> 
>  	u8			version;
>  	resource_size_t		pbase;
> @@ -183,7 +153,7 @@ struct davinci_spi {
>  	const void		*tx;
>  	void			*rx;
>  	int			count;
> -	struct 			davinci_spi_platform_data *pdata;
> +	struct			davinci_spi_platform_data *pdata;
> 
>  	void			(*get_rx)(u32 rx_data, struct davinci_spi *);
>  	u32			(*get_tx)(struct davinci_spi *);
> 

[Sandeep] 
Thanks,
Sandeep
David Brownell July 14, 2009, 11:40 p.m. UTC | #2 
On Tuesday 14 July 2009, Paulraj, Sandeep wrote:
> David,
> 
> Thanks for the patch. I'll trying to send updates ASAP.
> 
> I have just a few questions though.
> 
> IS it OK if I continue to use DAVINCI_ in my #defines.

It's OK.  You're maintaining it, not me.  :)


> I think we should because these are used only in DaVinci.
> I had a look at the mmc driver for DaVinci and it is full of such #define with DAVINCI_

I wouldn't have used them there either.  Those prefixes
are superfluous; using them adds nothing.

 
> And a few more below
> 
> > -----Original Message-----
> > From: David Brownell [mailto:david-b@pacbell.net]
> > Sent: Tuesday, July 14, 2009 12:54 PM
> > To: davinci-linux-open-source@linux.davincidsp.com
> > Cc: Paulraj, Sandeep
> > Subject: Re: [PATCH 6/6] DaVinci: Adding SPI driver for DM3xx/DM6467/DA8xx
> > 
> > On Monday 13 July 2009, s-paulraj@ti.com wrote:
> > > The patch adds support for SPI driver for DaVinci
> > > DM355/DM365/DM6467 and DA8xx
> > 
> > Comments-in-the-form-of-a-patch below ... I figured this
> > could expedite things.  :)
> > 
> > - Dave
> > 
> > ===============================
> > Various bits of cleanup for the davinci_spi driver:
> > 
> >  - Kconfig should never have "default y"
> >  - Reorder #includes ... standard stuff should go first
> >  - Remove pointless inlines
> >  - Remove needless (and sometimes unused) #defines
> >  - Declare the special SPI_* modes which are supported!
> >  - Whitespace fixes
> >  - Update busy-wait loops
> >  - Streamline op_mode setup
> >  - ... more
> > 
> > NOT ADDRESSED:
> > 
> >  - The printk(KERN_ERR ...) messages that should be dev_dbg(...)
> [Sandeep] I'll address
> >  - The use of "-1" error codes instead of negative errno
> [Sandeep] What would be the most appropriate error value.
> 
> What if I use -EADV that is for "advertise error".
> 
> If it is not appropriate then can you suggest a more appropriate return value

Look at each error and return an appropriate error.
Like -ETIMEDOUT for a timeout ... this driver doesn't
advertise anything so "-EADV" will be meaningless.


> >  - Timing out busy-wait loops
> [Sandeep] OK
> >  - Adding a comment explaining that either built-in chipselects
> >    *or* GPIO versions may be used.  (If GPIO, can up to 4 slaves
> >    be supported?)
> [Sandeep] The most ideal place for this explanation would be in a
> wiki page at wiki.davincidsp.com. 

No; add at least a mention of the issue here.  HOWTO, yeah that
could go into a wiki.

 
> But I will still add an explanation for this in dm355.c and dm365.c
> and not in the driver. 

Why put it in two places, when one (driver) suffices?  :)
Kevin Hilman July 15, 2009, 6:12 p.m. UTC | #3 
David Brownell <david-b@pacbell.net> writes:

> On Tuesday 14 July 2009, Paulraj, Sandeep wrote:
>> David,
>> 
>> Thanks for the patch. I'll trying to send updates ASAP.
>> 
>> I have just a few questions though.
>> 
>> IS it OK if I continue to use DAVINCI_ in my #defines.
>
> It's OK.  You're maintaining it, not me.  :)

I'd prefer to see them disappear.  These defines aren't going to
be used/included outside of davinci specific code, so they are
surpurflous.

>> I think we should because these are used only in DaVinci.
>> I had a look at the mmc driver for DaVinci and it is full of such #define with DAVINCI_
>
> I wouldn't have used them there either.  Those prefixes
> are superfluous; using them adds nothing.

Agreed.

Kevin
s-paulraj@ti.com July 15, 2009, 6:21 p.m. UTC | #4 
> -----Original Message-----
> From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
> Sent: Wednesday, July 15, 2009 2:13 PM
> To: David Brownell
> Cc: Paulraj, Sandeep; davinci-linux-open-source@linux.davincidsp.com
> Subject: Re: [PATCH 6/6] DaVinci: Adding SPI driver for DM3xx/DM6467/DA8xx
> 
> David Brownell <david-b@pacbell.net> writes:
> 
> > On Tuesday 14 July 2009, Paulraj, Sandeep wrote:
> >> David,
> >>
> >> Thanks for the patch. I'll trying to send updates ASAP.
> >>
> >> I have just a few questions though.
> >>
> >> IS it OK if I continue to use DAVINCI_ in my #defines.
> >
> > It's OK.  You're maintaining it, not me.  :)
> 
> I'd prefer to see them disappear.  These defines aren't going to
> be used/included outside of davinci specific code, so they are
> surpurflous.
[Sandeep] OK
> 
> >> I think we should because these are used only in DaVinci.
> >> I had a look at the mmc driver for DaVinci and it is full of such
> #define with DAVINCI_
> >
> > I wouldn't have used them there either.  Those prefixes
> > are superfluous; using them adds nothing.
> 
> Agreed.
[Sandeep] OK, I'll get rid of them. Can I still keep a separate "davinci_spi.h" header file in /drivers/spi?
> 
> Kevin
>
David Brownell July 15, 2009, 6:24 p.m. UTC | #5 
On Wednesday 15 July 2009, Paulraj, Sandeep wrote:
> [Sandeep] OK, I'll get rid of them. Can I still keep a
> separate "davinci_spi.h" header file in /drivers/spi? 

You already got an OK for that.  Both styles are in use.
diff mbox

Patch

===============================
Various bits of cleanup for the davinci_spi driver:

 - Kconfig should never have "default y"
 - Reorder #includes ... standard stuff should go first
 - Remove pointless inlines
 - Remove needless (and sometimes unused) #defines
 - Declare the special SPI_* modes which are supported!
 - Whitespace fixes
 - Update busy-wait loops
 - Streamline op_mode setup
 - ... more

NOT ADDRESSED:

 - The printk(KERN_ERR ...) messages that should be dev_dbg(...)
 - The use of "-1" error codes instead of negative errno
 - Timing out busy-wait loops
 - Adding a comment explaining that either built-in chipselects
   *or* GPIO versions may be used.  (If GPIO, can up to 4 slaves
   be supported?)

---
 drivers/spi/Kconfig       |    1 
 drivers/spi/davinci_spi.c |  142 ++++++++++++++++----------------------------
 drivers/spi/davinci_spi.h |   42 +------------
 3 files changed, 59 insertions(+), 126 deletions(-)

--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -81,7 +81,6 @@  config SPI_DAVINCI
 	tristate "SPI controller driver for DaVinci SoC"
 	depends on SPI_MASTER && ARCH_DAVINCI
 	select SPI_BITBANG
-	default y
 	help
 	  SPI master controller for DaVinci SPI modules.
 
--- a/drivers/spi/davinci_spi.c
+++ b/drivers/spi/davinci_spi.c
@@ -17,11 +17,10 @@ 
  * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
  */
 
-#include "davinci_spi.h"
+#include <linux/types.h>
+#include <linux/io.h>
 #include <linux/dma-mapping.h>
 #include <linux/io.h>
-#include <mach/mux.h>
-#include <mach/gpio.h>
 #include <linux/interrupt.h>
 #include <linux/module.h>
 #include <linux/device.h>
@@ -29,9 +28,17 @@ 
 #include <linux/platform_device.h>
 #include <linux/err.h>
 #include <linux/clk.h>
+#include <linux/gpio.h>
 
-static inline void davinci_spi_rx_buf_u8(u32 data,
-					 struct davinci_spi *davinci_spi)
+#include <linux/spi/spi.h>
+#include <linux/spi/spi_bitbang.h>
+
+#include <mach/spi.h>
+
+#include "davinci_spi.h"
+
+
+static void davinci_spi_rx_buf_u8(u32 data, struct davinci_spi *davinci_spi)
 {
 	u8 *rx = davinci_spi->rx;
 
@@ -39,8 +46,7 @@  static inline void davinci_spi_rx_buf_u8
 	davinci_spi->rx = rx;
 }
 
-static inline void davinci_spi_rx_buf_u16(u32 data,
-					  struct davinci_spi *davinci_spi)
+static void davinci_spi_rx_buf_u16(u32 data, struct davinci_spi *davinci_spi)
 {
 	u16 *rx = davinci_spi->rx;
 
@@ -48,7 +54,7 @@  static inline void davinci_spi_rx_buf_u1
 	davinci_spi->rx = rx;
 }
 
-static inline u32 davinci_spi_tx_buf_u8(struct davinci_spi *davinci_spi)
+static u32 davinci_spi_tx_buf_u8(struct davinci_spi *davinci_spi)
 {
 	u32 data;
 	const u8 *tx = davinci_spi->tx;
@@ -58,7 +64,7 @@  static inline u32 davinci_spi_tx_buf_u8(
 	return data;
 }
 
-static inline u32 davinci_spi_tx_buf_u16(struct davinci_spi *davinci_spi)
+static u32 davinci_spi_tx_buf_u16(struct davinci_spi *davinci_spi)
 {
 	u32 data;
 	const u16 *tx = davinci_spi->tx;
@@ -71,6 +77,7 @@  static inline u32 davinci_spi_tx_buf_u16
 static inline void set_bits(void __iomem *addr, u32 bits)
 {
 	u32 v = ioread32(addr);
+
 	v |= bits;
 	iowrite32(v, addr);
 }
@@ -78,6 +85,7 @@  static inline void set_bits(void __iomem
 static inline void clear_bits(void __iomem *addr, u32 bits)
 {
 	u32 v = ioread32(addr);
+
 	v &= ~bits;
 	iowrite32(v, addr);
 }
@@ -114,7 +122,7 @@  static void davinci_spi_chipselect(struc
 		if (pdata->chip_sel != NULL) {
 			for (i = 0; i < pdata->num_chipselect; i++) {
 				if (pdata->chip_sel[i] != DAVINCI_SPI_INTERN_CS)
-					__gpio_set_value(pdata->chip_sel[i], 1);
+					gpio_set_value(pdata->chip_sel[i], 1);
 			}
 		}
 
@@ -123,10 +131,9 @@  static void davinci_spi_chipselect(struc
 		data1_reg_val |= CS_DEFAULT << DAVINCI_SPIDAT1_CSNR_SHIFT;
 		iowrite32(data1_reg_val, davinci_spi->base + SPIDAT1);
 
-		while (1)
-			if (ioread32(davinci_spi->base + SPIBUF)
-					& DAVINCI_SPIBUF_RXEMPTY_MASK)
-				break;
+		while ((ioread32(davinci_spi->base + SPIBUF)
+					& DAVINCI_SPIBUF_RXEMPTY_MASK) == 0)
+			cpu_relax();
 	}
 }
 
@@ -309,67 +316,21 @@  static int davinci_spi_bufs_prep(struct 
 		iowrite32(DAVINCI_SPI_INTLVL_0, davinci_spi->base + SPILVL);
 
 	/*
-	 * The driver uses new flags SPI_NO_CS and SPI_RAEDY
-	 * These can be found in /include/linux/spi/spi.h
-	 * Standard SPI mode is a 4 pin variant
-	 * 3 pin SPI is actually the 4 pin variant with no CS(SPI_NO_CS)
+	 * Standard SPI mode uses 4 pins, with chipselect
+	 * 3 pin SPI is a 4 pin variant without CS (SPI_NO_CS)
+	 *	(distinct from SPI_3WIRE, with just one data wire;
+	 *	or similar variants without MOSI or without MISO)
 	 * 4 pin with enable is (SPI_READY | SPI_NO_CS)
-	 * 5 pin SPI variant is SPI_READY
-	 * Operation mode(op_mode) depends on these flags
-	 * op_mode 0 = standard 4 pin mode
-	 * op_mode 1 = 3 pin mode
-	 * op_mode 2 = 5 pin mode
-	 * op_mode 3 = 4 pin mode with enable
+	 * 5 pin SPI variant is standard SPI plus SPI_READY
 	 */
 
-	op_mode = ((spi->mode & (SPI_NO_CS | SPI_READY)) >> 6);
-
-	switch (op_mode) {
-	case 0:
-		op_mode = (DAVINCI_SPIPC0_DIFUN_DI <<
-				DAVINCI_SPIPC0_DIFUN_SHIFT)
-			| (DAVINCI_SPIPC0_DOFUN_DO <<
-				DAVINCI_SPIPC0_DOFUN_SHIFT)
-			| (DAVINCI_SPIPC0_CLKFUN_CLK <<
-				DAVINCI_SPIPC0_CLKFUN_SHIFT)
-			| (1 << spi->chip_select);
-		break;
-
-	case 1:
-		op_mode = (DAVINCI_SPIPC0_DIFUN_DI <<
-				DAVINCI_SPIPC0_DIFUN_SHIFT)
-			| (DAVINCI_SPIPC0_DOFUN_DO <<
-				DAVINCI_SPIPC0_DOFUN_SHIFT)
-			| (DAVINCI_SPIPC0_CLKFUN_CLK <<
-				DAVINCI_SPIPC0_CLKFUN_SHIFT);
-		break;
-
-	case 2:
-		op_mode = (DAVINCI_SPIPC0_DIFUN_DI <<
-				DAVINCI_SPIPC0_DIFUN_SHIFT)
-			| (DAVINCI_SPIPC0_DOFUN_DO <<
-				DAVINCI_SPIPC0_DOFUN_SHIFT)
-			| (DAVINCI_SPIPC0_CLKFUN_CLK <
-				DAVINCI_SPIPC0_CLKFUN_SHIFT)
-			| (DAVINCI_SPIPC0_SPIENA <<
-				DAVINCI_SPIPC0_SPIENA_SHIFT)
-			| (1 << spi->chip_select);
-		break;
-
-	case 3:
-		op_mode = (DAVINCI_SPIPC0_DIFUN_DI <<
-				DAVINCI_SPIPC0_DIFUN_SHIFT)
-			| (DAVINCI_SPIPC0_DOFUN_DO <<
-				DAVINCI_SPIPC0_DOFUN_SHIFT)
-			| (DAVINCI_SPIPC0_CLKFUN_CLK <<
-				DAVINCI_SPIPC0_CLKFUN_SHIFT)
-			| (DAVINCI_SPIPC0_SPIENA <<
-				DAVINCI_SPIPC0_SPIENA_SHIFT);
-		break;
-
-	default:
-		return -1;
-	}
+	op_mode = DAVINCI_SPIPC0_DIFUN_MASK
+		| DAVINCI_SPIPC0_DOFUN_MASK
+		| DAVINCI_SPIPC0_CLKFUN_MASK;
+	if (!(spi->mode & SPI_NO_CS))
+		op_mode |= 1 << spi->chip_select;
+	if (spi->mode & SPI_READY)
+		op_mode |= DAVINCI_SPIPC0_SPIENA_MASK;
 
 	iowrite32(op_mode, davinci_spi->base + SPIPC0);
 
@@ -475,16 +436,15 @@  static int davinci_spi_bufs_pio(struct s
 	/* checking for GPIO */
 	if ((pdata->chip_sel != NULL) &&
 	    (pdata->chip_sel[spi->chip_select] != DAVINCI_SPI_INTERN_CS))
-		__gpio_set_value(pdata->chip_sel[spi->chip_select], 0);
+		gpio_set_value(pdata->chip_sel[spi->chip_select], 0);
 	 else
 		clear_bits(davinci_spi->base + SPIDEF, ~tmp);
 
 	data1_reg_val |= tmp << DAVINCI_SPIDAT1_CSNR_SHIFT;
 
-	while (1)
-		if (ioread32(davinci_spi->base + SPIBUF)
-				& DAVINCI_SPIBUF_RXEMPTY_MASK)
-			break;
+	while ((ioread32(davinci_spi->base + SPIBUF)
+				& DAVINCI_SPIBUF_RXEMPTY_MASK) == 0)
+		cpu_relax();
 
 	/* Determine the command to execute READ or WRITE */
 	if (t->tx_buf) {
@@ -505,7 +465,8 @@  static int davinci_spi_bufs_pio(struct s
 			}
 			while (ioread32(davinci_spi->base + SPIBUF)
 					& DAVINCI_SPIBUF_RXEMPTY_MASK)
-				udelay(1);
+				cpu_relax();
+
 			/* getting the returned byte */
 			if (t->rx_buf) {
 				buf_val = ioread32(davinci_spi->base + SPIBUF);
@@ -519,13 +480,13 @@  static int davinci_spi_bufs_pio(struct s
 			while (1) {
 				/* keeps the serial clock going */
 				if ((ioread32(davinci_spi->base + SPIBUF)
-					& DAVINCI_SPIBUF_TXFULL_MASK) == 0)
+						& DAVINCI_SPIBUF_TXFULL_MASK) == 0)
 					iowrite32(data1_reg_val,
 						davinci_spi->base + SPIDAT1);
 
-				while ((ioread32(davinci_spi->base + SPIBUF)) &
-					(DAVINCI_SPIBUF_RXEMPTY_MASK))
-						;
+				while (ioread32(davinci_spi->base + SPIBUF) &
+						DAVINCI_SPIBUF_RXEMPTY_MASK)
+					cpu_relax();
 
 				flg_val = ioread32(davinci_spi->base + SPIFLG);
 				buf_val = ioread32(davinci_spi->base + SPIBUF);
@@ -549,8 +510,8 @@  static int davinci_spi_bufs_pio(struct s
 						davinci_spi->base + SPIDAT1);
 
 				while (ioread32(davinci_spi->base + SPIINT) &
-					(DAVINCI_SPIINT_RX_INTR))
-					;
+						DAVINCI_SPIINT_RX_INTR)
+					cpu_relax();
 			}
 			iowrite32((data1_reg_val & 0x0ffcffff),
 					davinci_spi->base + SPIDAT1);
@@ -659,7 +620,7 @@  static int davinci_spi_probe(struct plat
 	davinci_spi->region_size = resource_size(r);
 	davinci_spi->pdata = pdata;
 
-	/* initialize gpio used as chip select */
+	/* initialize any gpio lines used as chip select */
 	if (pdata->chip_sel != NULL) {
 		for (i = 0; i < pdata->num_chipselect; i++) {
 			if (pdata->chip_sel[i] != DAVINCI_SPI_INTERN_CS) {
@@ -690,7 +651,7 @@  static int davinci_spi_probe(struct plat
 	}
 
 	ret = request_irq(davinci_spi->irq, davinci_spi_irq, IRQF_DISABLED,
-			  pdev->name, davinci_spi);
+			  dev_name(&pdev->dev), davinci_spi);
 	if (ret != 0) {
 		ret = -EAGAIN;
 		goto unmap_io;
@@ -716,8 +677,12 @@  static int davinci_spi_probe(struct plat
 
 	davinci_spi->bitbang.chipselect = davinci_spi_chipselect;
 	davinci_spi->bitbang.setup_transfer = davinci_spi_setup_transfer;
-
 	davinci_spi->bitbang.txrx_bufs = davinci_spi_bufs_pio;
+
+	davinci_spi->bitbang.flags = SPI_LSB_FIRST | SPI_LOOP;
+	if (davinci_spi->version == DAVINCI_SPI_VERSION_2)
+		davinci_spi->bitbang.flags |= SPI_NO_CS | SPI_READY;
+
 	davinci_spi->version = pdata->version;
 	davinci_spi->get_rx = davinci_spi_rx_buf_u8;
 	davinci_spi->get_tx = davinci_spi_tx_buf_u8;
@@ -733,8 +698,7 @@  static int davinci_spi_probe(struct plat
 	if (ret != 0)
 		goto free_clk;
 
-	printk(KERN_NOTICE "Davinci SPI Controller driver at "
-		"0x%p (irq = %d) \n",
+	dev_info(&pdev->dev,"Controller at 0x%p (irq = %d) \n",
 		davinci_spi->base, davinci_spi->irq);
 
 	return ret;
--- a/drivers/spi/davinci_spi.h
+++ b/drivers/spi/davinci_spi.h
@@ -19,13 +19,6 @@ 
 #ifndef __DAVINCI_SPI_H
 #define __DAVINCI_SPI_H
 
-#include <linux/types.h>
-#include <linux/io.h>
-#include <linux/clk.h>
-#include <linux/spi/spi.h>
-#include <linux/spi/spi_bitbang.h>
-#include <mach/spi.h>
-
 #define DAVINCI_SPI_MAX_CHIPSELECT	2
 
 #define CS_DEFAULT	0xFF
@@ -53,33 +46,12 @@ 
 #define DAVINCI_SPIGCR1_SPIENA_MASK	0x01000000u
 
 /* SPIPC0 */
-#define DAVINCI_SPIPC0_DIFUN_MASK	BIT(11)
-#define DAVINCI_SPIPC0_DIFUN_SHIFT	11
-
-/* DIFUN */
-#define DAVINCI_SPIPC0_DIFUN_DI		BIT(0)
-
-/* DOFUN */
-#define DAVINCI_SPIPC0_DOFUN_MASK	BIT(10)
-#define DAVINCI_SPIPC0_DOFUN_SHIFT	10
-#define DAVINCI_SPIPC0_DOFUN_DO		BIT(0)
-
-/* CLKFUN */
-#define DAVINCI_SPIPC0_CLKFUN_MASK	BIT(9)
-#define DAVINCI_SPIPC0_CLKFUN_SHIFT	9
-#define DAVINCI_SPIPC0_CLKFUN_CLK	BIT(0)
-
-/* EN1FUN */
+#define DAVINCI_SPIPC0_DIFUN_MASK	BIT(11)		/* MISO */
+#define DAVINCI_SPIPC0_DOFUN_MASK	BIT(10)		/* MOSI */
+#define DAVINCI_SPIPC0_CLKFUN_MASK	BIT(9)		/* CLK */
+#define DAVINCI_SPIPC0_SPIENA_MASK	BIT(8)		/* nREADY */
 #define DAVINCI_SPIPC0_EN1FUN_MASK	BIT(1)
-#define DAVINCI_SPIPC0_EN1FUN_EN1	BIT(0)
-
-/* EN0FUN Tokens */
 #define DAVINCI_SPIPC0_EN0FUN_MASK	BIT(0)
-#define DAVINCI_SPIPC0_EN0FUN_SHIFT	0
-#define DAVINCI_SPIPC0_EN0FUN_EN0	BIT(0)
-
-#define DAVINCI_SPIPC0_SPIENA		BIT(0)
-#define DAVINCI_SPIPC0_SPIENA_SHIFT	8
 
 #define DAVINCI_SPIINT_MASKALL		0x0101035F
 #define DAVINCI_SPI_INTLVL_1		0x000001FFu
@@ -127,8 +99,6 @@ 
 #define DAVINCI_SPIINT_DMA_REQ_EN	BIT(16)
 #define DAVINCI_SPIINT_ENABLE_HIGHZ	BIT(24)
 
-#define DAVINCI_BYTELENGTH 8u
-
 #define DAVINCI_SPI_T2CDELAY_SHIFT	16
 #define DAVINCI_SPI_C2TDELAY_SHIFT	24
 
@@ -171,7 +141,7 @@  struct davinci_spi_slave {
 struct davinci_spi {
 	/* bitbang has to be first */
 	struct spi_bitbang	bitbang;
-	struct clk 		*clk;
+	struct clk		*clk;
 
 	u8			version;
 	resource_size_t		pbase;
@@ -183,7 +153,7 @@  struct davinci_spi {
 	const void		*tx;
 	void			*rx;
 	int			count;
-	struct 			davinci_spi_platform_data *pdata;
+	struct			davinci_spi_platform_data *pdata;
 
 	void			(*get_rx)(u32 rx_data, struct davinci_spi *);
 	u32			(*get_tx)(struct davinci_spi *);