Re: FW: [RFC] EP93xx SPI driver

Re: FW: [RFC] EP93xx SPI driver

[hartleys]

On Wednesday, December 10, 2008 1:06 PM, Ryan Mallon wrote:
>> Following is a spi driver for the ep93xx based on a driver from Ryan 
>> Mallon at Bluewatersys.
>> 
>> This driver has been cleaned up and modified to handle multiple 
>> devices on the spi bus.  I have been using/testing it with the m25p80

>> and mmc_spi drivers with good success.
>
> Cool.
>
>> I would appreciate any comments/suggestions needed to get this driver

>> ready for mainline.
>> 
>> Sorry that this is not a git diff.  I haven't figured out how to add
>> a new file yet.
>
> It works the same as for existing files: just do git add/git commit
> on the file. Alternatively, you can generate a unified diff for a
> new file with:
>
>  diff -uNr /dev/null foo.c

I'll try the git add/git commit and re-post the diff.

>> The first block of code below is file
>> arch/arm/mach-ep93xx/include/mach/spi.h.
>> The second one is file drivers/spi/spi_ep93xx.c.
>> 
>> Thanks for any help and/or review comments.
>> 
>> Signed-off-by: H Hartley Sweeten <hsweeten-3FF4nKcrg1dE2c76skzGb0EOCMrvLtNR@public.gmane.org>
>> 
>> ---
>> 
>> 
>> #ifndef __ASM_ARCH_SPI_H
>> #define __ASM_ARCH_SPI_H
>> 
>> /* device.platform_data for SSP controller devices */ struct 
>> ep93xx_spi_master {
>> 	u16	num_chipselects;
>> };
>
> I think you have got line wrapping happening again. struct should
> definitely not be on the same line as the comment :-).

Yah, noticed that when the post came thru.  I still haven't figured out
why it happens only some of the time...

>> /* spi_board_info.controller_data for SPI slave devices,
>>  * copied to spi_device.platform_data
>>  */
>> struct ep93xx_spi_chip {
>> 	void	(*cs_control)(int level);
>> 	void	(*cleanup)(void);
>> };
>> 
>> #endif /* __ASM_ARCH_SPI_H */
>> 
>> 
>> 
>> /*
>>  * Cirrus EP93xx SPI Driver
>>  *
>>  * Copyright (c) 2007 Ryan Mallon <ryan-7Wk5F4Od5/oYd5yxfr4S2w@public.gmane.org>
>>  * Copyright (c) 2008 H Hartley Sweeten
<hsweeten-3FF4nKcrg1dE2c76skzGb0EOCMrvLtNR@public.gmane.org>
>>  *
>>  * Based on the ep93xx spi bitbang driver by Chase Douglas
>>  * and linux/drivers/spi/spi_txx9.c
>>  *
>>  * Licensed under the GPL-2 or later.
>>  */
>> 
>> #include <linux/platform_device.h>
>> #include <linux/interrupt.h>
>> #include <linux/clk.h>
>> #include <linux/delay.h>
>> #include <linux/err.h>
>> #include <linux/spi/spi.h>
>> 
>> #include <mach/hardware.h>
>> #include <mach/spi.h>
>> 
>> /*
>>  * Synchronous Serial Port Register offsets  */
>> #define SSPCR0			0x00
>> #define SSPCR1			0x04
>> #define SSPDR			0x08
>> #define SSPSR			0x0c
>> #define SSPCPSR			0x10
>> #define SSPIIR			0x14
>> #define SSPICR			SSPIIR
>> 
>> /* Control Register 0 bit defines */
>> #define SSPCR0_SCR_MASK		(0x0000ff00)
>> #define SSPCR0_SCR_SHIFT	(8)
>> #define SSPCR0_SPH		(1<<7)
>> #define SSPCR0_SPO		(1<<6)
>> #define SSPCR0_FRF_MASK		(0x00000030)
>> #define SSPCR0_FRF_SHIFT	(4)
>> #define SSPCR0_FRF_MOTOROLA	(0<<4)
>> #define SSPCR0_FRF_TI		(1<<4)
>> #define SSPCR0_FRF_NI		(2<<4)
>> #define SSPCR0_DSS_MASK		(0x0000000f)
>> #define SSPCR0_DSS_SHIFT	(0)
>> #define SSPCR0_DSS_4BIT		(3<<0)
>> #define SSPCR0_DSS_5BIT		(4<<0)
>> #define SSPCR0_DSS_6BIT		(5<<0)
>> #define SSPCR0_DSS_7BIT		(6<<0)
>> #define SSPCR0_DSS_8BIT		(7<<0)
>> #define SSPCR0_DSS_9BIT		(8<<0)
>> #define SSPCR0_DSS_10BIT	(9<<0)
>> #define SSPCR0_DSS_11BIT	(10<<0)
>> #define SSPCR0_DSS_12BIT	(11<<0)
>> #define SSPCR0_DSS_13BIT	(12<<0)
>> #define SSPCR0_DSS_14BIT	(13<<0)
>> #define SSPCR0_DSS_15BIT	(14<<0)
>> #define SSPCR0_DSS_16BIT	(15<<0)
>> 
>> /* Control Register 1 bit defines */
>> #define SSPCR1_SOD		(1<<6)
>> #define SSPCR1_MS		(1<<5)
>> #define SSPCR1_SSE		(1<<4)
>> #define SSPCR1_LBM		(1<<3)
>> #define SSPCR1_RORIE		(1<<2)
>> #define SSPCR1_TIE		(1<<1)
>> #define SSPCR1_RIE		(1<<0)
>> 
>> /* Receive (read) / Transmit (write) FIFO Register bit defines */
>> #define SSPDR_MASK		(0x0000ffff)
>> #define SSPDR_SHIFT		(0)
>> 
>> #define FIFO_SIZE		8
>> 
>> /* Status Register bit defines */
>> #define SSPSR_BSY		(1<<4)
>> #define SSPSR_RFF		(1<<3)
>> #define SSPSR_RNE		(1<<2)
>> #define SSPSR_TNF		(1<<1)
>> #define SSPSR_TFE		(1<<0)
>> 
>> /* Clock Prescale Register bit defines */
>> #define SSPCPSR_MASK		(0x000000ff)
>> #define SSPCPSR_SHIFT		(0)
>> 
>> /* Interrupt Identification (read) / Interrupt Clear (write) register

>> bit defines */
>> #define SSPIIR_RORIS		(1<<2)
>> #define SSPIIR_TIS		(1<<1)
>> #define SSPIIR_RIS		(1<<0)
>> 
>> #define spi_readl(off)		__raw_readl(info->mmio_base +
(off))
>> #define spi_writel(v, off)	__raw_writel((v), info->mmio_base +
>> (off))
>
> These are a bit dodgy, since they refer to the info variable, which
isn't
> a global. If the calling function (or a new function) were to change
the
> name of the variable these macros will break. You should either pass
info
> to the macro, or move the code directly inside of spi_write, etc. They
could
> also be changed to static inlines: easier to read and you get type
safety.

I will change them to static inlines.  I think saw that macro usage in
some other driver I was looking at and figured it was a "normal" usage.

>>
>> struct ep93xx_chip_data {
>> 	u32	max_speed_hz;
>> 	u8	bits_per_word;
>> 	int	cs_active_high;
>> 	void	(*cs_control)(int level);
>> 	void	(*cleanup)(void);
>> };
>> 
>> struct ep93xxspi {
>
> ep93xx_spi? Easier to read.

Agree.  But it pushes some of the lines past 78 chars.

>> 	struct spi_master		*master;
>> 	struct device			*dev;
>> 	struct ep93xx_spi_master	*pdata;
>> 	struct clk			*sspclk;
>> 	struct clk			*sspclkout;
>
> Same here. ssp_clk and ssp_clk_out are a little easier to read.

Not a problem.  Will change before I post the driver again.

>> 	void __iomem			*mmio_base;
>> 
>> 	struct ep93xx_chip_data		*chip;
>> 
>> 	struct list_head		msg_queue;
>> 	struct workqueue_struct 	*workq;
>> 	struct work_struct		work;
>> 	wait_queue_head_t		waitq;
>> 
>> 	spinlock_t			lock;
>> 
>> 	int				tx_irq;
>> 	int				rx_irq;
>> };
>> 
>> /*
>>  * Read a value from the Receive FIFO
>>  */
>> static inline unsigned int ep93xx_spi_read(struct ep93xxspi *info) {
>> 	return spi_readl(SSPDR) & SSPDR_MASK; }
>> 
>> /*
>>  * Write a value to the Transmit FIFO
>>  */
>> static inline void ep93xx_spi_write(struct ep93xxspi *info,
>> 		unsigned int val)
>> {
>> 	spi_writel(val & SSPDR_MASK, SSPDR);
>> }
>> 
>> /*
>>  * Check if the Transmit FIFO is full
>>  */
>> static inline int ep93xx_spi_tx_full(struct ep93xxspi *info) {
>> 	return (spi_readl(SSPSR) & SSPSR_TNF) ? 0 : 1; }
>
> Line wrapping? { and } should be on their own lines.
>
>> 
>> /*
>>  * Check if the Receive FIFO is empty
>>  */
>> static inline int ep93xx_spi_rx_empty(struct ep93xxspi *info) {
>> 	return (spi_readl(SSPSR) & SSPSR_RNE) ? 0 : 1; }
>
> Same here.

All the line wrapping issues are fine in the actual code.

>> /*
>>  * Get the next transfer from the list, lock is already held  */ 
>> static inline struct spi_transfer *ep93xx_spi_next_transfer(
>> 		struct spi_transfer *transfer, struct spi_message *msg)
{
>> 	if (transfer->transfer_list.next == &msg->transfers)
>> 		return NULL;
>> 
>> 	return list_entry(transfer->transfer_list.next,
>> 			  struct spi_transfer, transfer_list); }
>> 
>> /*
>>  * Process all the transfers in a spi message  */ static void 
>> ep93xx_spi_handle_message(struct ep93xxspi *info,
>> 		struct spi_message *msg)
>> {
>> 	struct spi_transfer *rx, *tx;
>> 	int rx_left, tx_left, sent = 0, recv = 0;
>> 	const u8 *tx_buf;
>> 	u8 *rx_buf;
>> 	unsigned long flags;
>> 
>> 	spin_lock_irqsave(&info->lock, flags);
>> 	rx = tx = list_entry(msg->transfers.next, struct spi_transfer, 
>> transfer_list);
>> 	spin_unlock_irqrestore(&info->lock, flags);
>> 
>> 	tx_left = tx->len;
>> 	rx_left = rx->len;
>> 	tx_buf = tx->tx_buf;
>> 	rx_buf = rx->rx_buf;
>> 
>> 	msg->actual_length = 0;
>> 
>> 	spi_writel(SSPCR1_SSE, SSPCR1);
>> 
>> 	/* Change speed for the transfer */
>> 	if (tx->speed_hz) {
>> 		dev_dbg(info->dev, "transfer->speed_hz = %u\n",
>> tx->speed_hz);
>> 		clk_set_rate(info->sspclkout, tx->speed_hz);
>> 	} else {
>> 		clk_set_rate(info->sspclkout, info->chip->max_speed_hz);
>> 	}
>
> Should the speed control go here? It seems odd to control the speed on
> a per message basis. Is it possible to move it out to spi_work or
> somewhere else so that it doesn't have to get set on every message?

Not really sure.  I think I had to do that to get the mmc_spi driver to
work correctly.  It actually changes the speed between some of the
messages.

>> 
>> 	/* FIXME: Change bits per word for the transfer? */
>> 	if (tx->bits_per_word && tx->bits_per_word !=
>> info->chip->bits_per_word) {
>> 		dev_dbg(info->dev,
>> 			"transfer->bits_per_word = %d\n",
>> tx->bits_per_word);
>> 	}
>> 
>> 	spi_writel(0x0, SSPCR1);
>> 	spi_writel(SSPCR1_SSE, SSPCR1);
>> 
>> 	/* Enable the frame latch */
>> 	if (info->chip->cs_control)
>> 		info->chip->cs_control(info->chip->cs_active_high);
>> 
>> 	while (tx || rx) {
>> 
>> 		/* Send */
>> 		while (tx && !ep93xx_spi_tx_full(info)) {
>> 			/* Don't send more bytes than we have read */
>> 			if (sent - recv >= FIFO_SIZE)
>> 				break;
>> 
>> 			if (tx_left) {
>> 				if (tx_buf) {
>> 					ep93xx_spi_write(info, *tx_buf);
>> 					tx_buf++;
>> 				} else {
>> 					ep93xx_spi_write(info, 0);
>> 				}
>> 				tx_left--;
>> 				sent++;
>> 			}
>> 
>> 			if (!tx_left) {
>> 				/* Next transfer */
>> 				spin_lock_irqsave(&info->lock, flags);
>> 				tx = ep93xx_spi_next_transfer(tx, msg);
>> 				spin_unlock_irqrestore(&info->lock,
>> flags);
>> 
>> 				if (tx) {
>> 					tx_left = tx->len;
>> 					tx_buf = tx->tx_buf;
>> 					if (tx->speed_hz) {
>> 						/* FIXME: Change speed?
>> */
>> 						dev_dbg(info->dev,
>> 	
>> "transfer->speed_hz = %u\n",
>> 							tx->speed_hz);
>> 					}
>> 				}
>> 			}
>> 		}
>> 
>> 		/* Receive */
>> 		while (rx && !ep93xx_spi_rx_empty(info)) {
>> 			/* Don't empty the rx fifo to early */
>
> Nitpick: 'too' early ;-).

Never claimed to be an English major... actually that comment spelling
was in your original code ;-)

>> 			if (tx && (sent - recv <= FIFO_SIZE / 2))
>> 				break;
>> 
>> 			if (rx_left) {
>> 				u8 data;
>> 
>> 				data = ep93xx_spi_read(info);
>> 				if (rx_buf) {
>> 					*rx_buf = data;
>> 					rx_buf++;
>> 				}
>> 				rx_left--;
>> 				recv++;
>> 			}
>> 
>> 			if (!rx_left) {
>> 				/* Next transfer */
>> 				spin_lock_irqsave(&info->lock, flags);
>> 				rx = ep93xx_spi_next_transfer(rx, msg);
>> 				spin_unlock_irqrestore(&info->lock,
>> flags);
>> 
>> 				if (rx) {
>> 					rx_left = rx->len;
>> 					rx_buf = rx->rx_buf;
>> 					if (rx->speed_hz) {
>> 						/* FIXME: Change speed?
>> */
>
> Does this actually need fixing, or just something you are unsure
about?
> In the latter case, I would put "FIXME: Do we need to change speed
here?"
> just to make it clear that it is not broken.

Again, not sure.  It does appear that the speed can change between
transfers but I'm not sure about between the individual messages in a
given transfer.  I was hoping someone with more knowledge would make a
comment.  If you notice, I don't actually change the speed, only a debug
message is printed.

>> 						dev_dbg(info->dev,
>> 	
>> "transfer->speed_hz = %u\n",
>> 							rx->speed_hz);
>> 					}
>> 				}
>> 			}
>> 		}
>> 	}
>> 
>> 	dev_dbg(info->dev, "sent = %d, received = %d\n", sent, recv);
>> 
>> 	/* Wait for the Transmit FIFO to drain */
>> 	while (!(spi_readl(SSPSR) & SSPSR_TFE))
>> 		;
>> 
>> 	/* FIXME: Do I need to empty the receive FIFO? */
>> 
>> 	/* Disable the frame latch */
>> 	if (info->chip->cs_control)
>> 		info->chip->cs_control(!info->chip->cs_active_high);
>> 
>> 	msg->actual_length = sent;
>> 	msg->complete(msg->context);
>> }
>> 
>> /*
>>  * Work queue handler to process all queued spi messages from
>>  * ep93xx_spi_transfer()
>>  */
>> static void ep93xx_spi_work(struct work_struct *work) {
>> 	struct ep93xxspi *info = container_of(work, struct ep93xxspi,
work);
>> 	unsigned long flags;
>> 
>> 	spin_lock_irqsave(&info->lock, flags);
>> 	while (!list_empty(&info->msg_queue)) {
>> 		struct spi_message *msg;
>> 
>> 		msg = container_of(info->msg_queue.next,
>> 				struct spi_message, queue);
>> 		list_del(&msg->queue);
>> 		spin_unlock_irqrestore(&info->lock, flags);
>> 
>> 		info->chip = spi_get_ctldata(msg->spi);
>> 
>> 		ep93xx_spi_handle_message(info, msg);
>> 
>> 		spin_lock_irqsave(&info->lock, flags);
>> 	}
>> 	spin_unlock_irqrestore(&info->lock, flags); }
>> 
>> /*
>>  * spi_master->transfer() - adds a message to the controller's 
>> transfer queue.
>>  */
>> static int ep93xx_spi_transfer(struct spi_device *spi, struct 
>> spi_message *msg) {
>> 	struct ep93xxspi *info = spi_master_get_devdata(spi->master);
>> 	unsigned long flags;
>> 
>> 	spin_lock_irqsave(&info->lock, flags);
>> 
>> 	msg->actual_length = 0;
>
> How come this is done? I can't remember.

Not sure.  That is from your original code.

>> 	list_add_tail(&msg->queue, &info->msg_queue);
>> 	queue_work(info->workq, &info->work);
>> 
>> 	spin_unlock_irqrestore(&info->lock, flags);
>> 
>> 	return 0;
>> }
>> 
>> /* the spi->mode bits understood by this driver */
>> #define MODEBITS	(SPI_CPHA | SPI_CPOL | SPI_CS_HIGH)
>> 
>> /*
>>  * spi_master->setup() - updates the device mode and clocking records

>> used
>>  *                       by a device's SPI controller; protocol code
may
>>  *                       call this.
>>  *
>>  * This must fail if an unrecognized or unsupported mode is
requested.
>>  * It's always safe to call this unless transfers are pending on the
>>  * device whose settings are being modified.
>>  */
>> static int ep93xx_spi_setup(struct spi_device *spi) {
>> 	struct ep93xxspi *info = spi_master_get_devdata(spi->master);
>> 	struct ep93xx_spi_chip *chip_info = NULL;
>
> I don't think you need to initialise chip_info, since you assign it
> before checking if it is NULL later on.

For the multiple slave stuff I used the spi_bfin5xx.c driver as a
reference.  That's what it does.  I agree it's probably unnecessary.

>> 	struct ep93xx_chip_data *chip;
>> 	unsigned int val;
>> 
>> 	if (!spi->bits_per_word)
>> 		spi->bits_per_word = 8;
>> 	if (spi->bits_per_word < 4 || spi->bits_per_word > 16) {
>> 		dev_err(&spi->dev,
>> 			"failed setup: unsupported bits/word %d\n",
>> 			spi->bits_per_word);
>> 		return -EINVAL;
>> 	}
>> 
>> 	if (spi->mode & ~MODEBITS) {
>> 		dev_err(&spi->dev,
>> 			"failed setup: unsupported mode bits %x\n",
>> 			spi->mode & ~MODEBITS);
>> 		return -EINVAL;
>> 	}
>> 
>> 	/*
>> 	 * Only alloc chip data on first setup
>> 	 */
>> 	chip = spi_get_ctldata(spi);
>> 	if (!chip) {
>
> If this only needs to be done once, can it be moved to the probe/init
> function instead?

It happens once for each slave, not once for the driver.  After this
driver is probed each of the slaves start showing up and the setup
callback gets called.  But, the callback can get called multiple times
for a given slave device.

>> 		chip = kzalloc(sizeof(struct ep93xx_chip_data),
GFP_KERNEL);
>> 		if (!chip) {
>> 			dev_err(&spi->dev,
>> 				"failed setup: cannot allocate chip
data");
>> 			return -ENOMEM;
>> 		}
>> 
>> 		chip_info = spi->controller_data;
>> 		if (!chip_info) {
>> 			dev_err(&spi->dev,
>> 				"failed setup: %s is missing
>> controller_data\n",
>> 				spi->modalias);
>> 			kfree(chip);
>> 			return -ENODEV;
>> 		}
>
> Same here. Can we check spi->controller_data just once in the
> probe/init code?

The spi->controller_data is unique to each slave device.  It has nothing
to do with this driver.

>> 
>> 		if (chip_info->cs_control)
>> 			chip->cs_control = chip_info->cs_control;
>> 		if (chip_info->cleanup)
>> 			chip->cleanup = chip_info->cleanup;
>> 
>> 		spi_set_ctldata(spi, chip);
>> 
>> 		dev_info(&spi->dev, "initial setup for %s\n",
>> spi->modalias);
>> 	}
>> 
>> 	chip->max_speed_hz = spi->max_speed_hz;
>> 	chip->bits_per_word = spi->bits_per_word;
>> 	chip->cs_active_high = !!(spi->mode & SPI_CS_HIGH);
>> 
>> 	clk_set_rate(info->sspclkout, spi->max_speed_hz);
>> 
>> 	val = spi_readl(SSPCR0);
>> 	val &= SSPCR0_SCR_MASK;
>> 	val |= SSPCR0_FRF_MOTOROLA;
>> 	val |= SSPCR0_SPH | SSPCR0_SPO;
>> //	val |= ((spi->mode & SPI_CPHA) ? SSPCR0_SPH : 0);
>> //	val |= ((spi->mode & SPI_CPOL) ? SSPCR0_SPO : 0);
>
> Fix or remove.

Still looking into this.  As far as I can tell the code should work.
But when I tried it initially it broke the mmc_spi driver.

>> 	val |= (spi->bits_per_word - 1);
>> 	spi_writel(val, SSPCR0);
>> 
>> 	spi_writel(SSPCR1_SSE, SSPCR1);
>> 	spi_writel(0x0, SSPCR1);
>> 	spi_writel(SSPCR1_SSE, SSPCR1);
>> 
>> 	return 0;
>> }
>> 
>> /*
>>  * spi_master->cleanup() - frees controller-specific state  */ static

>> void ep93xx_spi_cleanup(struct spi_device *spi) {
>> 	struct ep93xx_chip_data *chip = spi_get_ctldata(spi);
>> 
>> 	if (chip->cleanup)
>> 		chip->cleanup();
>> 
>> 	kfree(chip);
>> }
>> 
>> static int ep93xx_spi_probe(struct platform_device *pdev) {
>> 	struct ep93xxspi *info = NULL;
>> 	struct spi_master *master;
>> 	struct resource *res;
>> 	int err = 0;
>> 
>> 	master = spi_alloc_master(&pdev->dev, sizeof(struct ep93xxspi));
>> 	if (!master) {
>> 		dev_err(&pdev->dev, "failed to allocate spi_master\n");
>> 		return -ENOMEM;
>> 	}
>> 
>> 	info = spi_master_get_devdata(master);
>> 	memset(info, 0, sizeof(struct ep93xxspi));
>> 	info->master = spi_master_get(master);
>>

My own question here...

Does spi_alloc_master() automatically zero the devdata region?
 
>> 	info->pdata = pdev->dev.platform_data;
>> 	if (!info->pdata) {
>> 		dev_err(&pdev->dev, "no platform data defined\n");
>> 		err = -EINVAL;
>> 		goto failed_free;
>> 	}
>> 
>> 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> 	if (res == NULL) {
>> 		dev_err(&pdev->dev, "failed to get I/O memory\n");
>> 		err = -ENXIO;
>> 		goto failed_free;
>> 	}
>> 
>> 	res = request_mem_region(res->start, resource_size(res),
>> pdev->name);
>> 	if (res == NULL) {
>> 		dev_err(&pdev->dev, "failed to request I/O memory\n");
>> 		err = -EBUSY;
>> 		goto failed_free;
>> 	}
>> 
>> 	info->mmio_base = ioremap(res->start, resource_size(res));
>> 	if (info->mmio_base == NULL) {
>> 		dev_err(&pdev->dev, "failed to remap I/O memory\n");
>> 		err = -ENXIO;
>> 		goto failed_free_mem;
>> 	}
>> 
>> 	info->sspclk = clk_get(&pdev->dev, "ssp_clk");
>> 	if (IS_ERR(info->sspclk)) {
>> 		dev_err(&pdev->dev, "failed to get ssp input clock\n");
>> 		err = PTR_ERR(info->sspclk);
>> 		goto failed_free_io;
>> 	}
>> 
>> 	info->sspclkout = clk_get(&pdev->dev, "ssp_clkout");
>> 	if (IS_ERR(info->sspclkout)) {
>> 		dev_err(&pdev->dev, "failed to get ssp output clock\n");
>> 		err = PTR_ERR(info->sspclkout);
>> 		goto failed_put_sspclk;
>> 	}
>> 

Again my own comment...

I will be updating the clk_get calls above once Russell King's clkdev
API hits mainline.

>> 	info->dev = &pdev->dev;
>> 	platform_set_drvdata(pdev, info);
>> 
>> 	spin_lock_init(&info->lock);
>> 
>> 	INIT_LIST_HEAD(&info->msg_queue);
>> 	INIT_WORK(&info->work, ep93xx_spi_work);
>> 
>> 	init_waitqueue_head(&info->waitq);
>> 
>> 	info->workq =
>> create_singlethread_workqueue(master->dev.parent->bus_id);
>> 	if (!info->workq) {
>> 		dev_err(&pdev->dev, "Cannot create work queue\n");
>> 		goto failed_put_sspclkout;
>> 	}
>> 
>> 	master->bus_num		= pdev->id;
>> 	master->num_chipselect	= info->pdata->num_chipselects;
>> 	master->setup		= ep93xx_spi_setup;
>> 	master->transfer	= ep93xx_spi_transfer;
>> 	master->cleanup		= ep93xx_spi_cleanup;
>> 
>> 	clk_enable(info->sspclk);
>> 	clk_enable(info->sspclkout);
>> 
>> 	err = spi_register_master(master);
>> 	if (err)
>> 		goto failed_disable_clks;
>> 
>> 	dev_info(&pdev->dev, "registered in bit-bang mode\n");
>> 
>> 	return 0;
>> 
>> failed_disable_clks:
>> 	clk_disable(info->sspclkout);
>> 	clk_disable(info->sspclk);
>> failed_put_sspclkout:
>> 	platform_set_drvdata(pdev, NULL);
>> 	clk_put(info->sspclkout);
>> failed_put_sspclk:
>> 	clk_put(info->sspclk);
>> failed_free_io:
>> 	iounmap(info->mmio_base);
>> failed_free_mem:
>> 	release_mem_region(res->start, resource_size(res));
>> failed_free:
>> 	spi_master_put(info->master);
>> 	return err;
>> }
>> 
>> static int ep93xx_spi_remove(struct platform_device *pdev) {
>> 	struct ep93xxspi *info = platform_get_drvdata(pdev);
>> 	struct resource *res;
>> 
>> 	platform_set_drvdata(pdev, NULL);
>> 	spi_unregister_master(info->master);
>> 
>> 	spi_writel(0x0, SSPCR1);
>> 
>> 	clk_disable(info->sspclkout);
>> 	clk_disable(info->sspclk);
>> 
>> 	clk_put(info->sspclkout);
>> 	clk_put(info->sspclk);
>> 
>> 	spi_master_put(info->master);
>> 
>> 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> 	release_mem_region(res->start, resource_size(res));
>> 
>> 	return 0;
>> }
>> 
>> 
>> #ifdef CONFIG_PM
>> static int ep93xx_spi_suspend(struct platform_device *pdev, 
>> pm_message_t
>> msg)
>> {
>> 	return 0;
>> }
>> 
>> static int ep93xx_spi_resume(struct platform_device *pdev) {
>> 	return 0;
>> }
>> #else
>> #define ep93xx_spi_suspend	NULL
>> #define ep93xx_spi_resume	NULL
>> #endif /* CONFIG_PM */
>
> If nothing needs to be done for suspend/resume then I think it is safe
> to just leave the functions as NULL pointers (ie don't define them
below).
> Are you able to test suspend/resume at all? I think at the very least
> should should stop/start the clocks. You may need to be careful with
the
> state of the queue (can you suspend in the middle of a transaction?).
If
> you can't test, I would just remove the code for now.

My usage for the driver does not require suspend/resume.  Your original
driver had this CONFIG_PM stuff in it and I just left it.  I do agree
that the clks should be stopped but I'm not sure how to handle the
queue.

Also, I'm not really sure how to test the power management stuff since I
haven't required it.

>> static struct platform_driver ep93xx_spidrv = {
>> 	.driver		= {
>> 		.name	= "ep93xx-spi",
>> 		.owner	= THIS_MODULE,
>> 	},
>> 	.suspend	= ep93xx_spi_suspend,
>> 	.resume		= ep93xx_spi_resume,
>> 	.remove		= ep93xx_spi_remove,
>> };
>> 
>> static int __init ep93xx_spi_init(void) {
>>         return platform_driver_probe(&ep93xx_spidrv, 
>> ep93xx_spi_probe); }
>> 
>> static void __exit ep93xx_spi_exit(void) {
>>         platform_driver_unregister(&ep93xx_spidrv);
>> }
>> 
>> module_init(ep93xx_spi_init);
>> module_exit(ep93xx_spi_exit);
>> 
>> MODULE_DESCRIPTION("EP93XX SPI Driver"); MODULE_AUTHOR("Ryan Mallon 
>> <ryan-7Wk5F4Od5/oYd5yxfr4S2w@public.gmane.org>"); MODULE_LICENSE("GPL");
>
> You can comma separate the names in MODULE_AUTHOR if you want to add
yourself there too.

Cool. I'll worry about that later.

Thanks for the review.


------------------------------------------------------------------------------
SF.Net email is Sponsored by MIX09, March 18-20, 2009 in Las Vegas, Nevada.
The future of the web can't happen without you.  Join us at MIX09 to help
pave the way to the Next Web now. Learn more and register at
http://ad.doubleclick.net/clk;208669438;13503038;i?http://2009.visitmix.com/