On Tue, Mar 16, 2010 at 09:32:05AM +1300, Ryan Mallon wrote: > Mika Westerberg wrote: > > This patch adds an SPI master driver for the Cirrus EP93xx SPI controller > > found > > in EP93xx chips (EP9301, EP9302, EP9307, EP9312 and EP9315). > > > > Driver currently supports only interrupt driven mode but in future we may > > add > > polling mode support as well. > > > Thanks for this. Hartley and I have done some work on a polling mode > driver, but as he says, this looks more suitable for mainline inclusion. > Hopefully I can find some time to test this out. We have a board which > has several devices hooked onto the ep93xx spi bus, including an SD > card. In the meantime I have a few comments below.
Hi Ryan, Thanks for the comments. I'll address them in the next series. See below some comments for your comments. > > Signed-off-by: Mika Westerberg <mika.westerb...@iki.fi> > > --- > > arch/arm/mach-ep93xx/include/mach/ep93xx_spi.h | 34 + > > drivers/spi/Kconfig | 11 + > > drivers/spi/Makefile | 1 + > > drivers/spi/ep93xx_spi.c | 1020 > > ++++++++++++++++++++++++ > > 4 files changed, 1066 insertions(+), 0 deletions(-) > > create mode 100644 arch/arm/mach-ep93xx/include/mach/ep93xx_spi.h > > create mode 100644 drivers/spi/ep93xx_spi.c > > > > diff --git a/arch/arm/mach-ep93xx/include/mach/ep93xx_spi.h > > b/arch/arm/mach-ep93xx/include/mach/ep93xx_spi.h > > new file mode 100644 > > index 0000000..fe93d2e > > --- /dev/null > > +++ b/arch/arm/mach-ep93xx/include/mach/ep93xx_spi.h > > @@ -0,0 +1,34 @@ > > +#ifndef __ASM_MACH_EP93XX_SPI_H > > +#define __ASM_MACH_EP93XX_SPI_H > > + > > +/** > > + * struct ep93xx_spi_info - EP93xx specific SPI descriptor > > + * @num_chipselect: number of chip select GPIOs on this board > > + * @cs_control: chip select control function > > + * @data: pointer to data that is passed to @cs_control function. > > + * Can be %NULL if not needed. > > + * > > + * This structure is passed from board support files to EP93xx SPI > > controller > > + * driver. It provides callback hook to control chip select GPIOs that are > > + * allocated in board support files during board initialization. > > + */ > > +struct ep93xx_spi_info { > > + int num_chipselect; > > + /* > > + * cs_control() - control board chipselect GPIO lines > > + * @cs: chip select to control > > + * @value: value to set the chip select line to > > + * @data: optional data > > + * > > + * This function is used to control board specific GPIO lines that > > + * are allocated as SPI chipselects. @value is either %0 or %1. It > > + * is possibled to pass additional information using @data. > > + * > > + * This function is called from thread context and can sleep if > > + * necessary. > > + */ > > + void (*cs_control)(unsigned cs, unsigned value, void *data); > > + void *data; > > +}; > > + > > +#endif /* __ASM_MACH_EP93XX_SPI_H */ > > diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig > > index a191fa2..5852340 100644 > > --- a/drivers/spi/Kconfig > > +++ b/drivers/spi/Kconfig > > @@ -117,6 +117,17 @@ config SPI_DAVINCI > > help > > SPI master controller for DaVinci and DA8xx SPI modules. > > > > +config SPI_EP93XX > > + tristate "Cirrus Logic EP93xx SPI controller" > > + depends on ARCH_EP93XX > > + help > > + This enables using the Cirrus EP93xx SPI controller in master > > + mode. This driver supports EP9301, EP9302, EP9307, EP9312 and EP9315 > > + chips. > > + > > + To compile this driver as a module, choose M here. The module will be > > + called ep93xx_spi. > > + > > config SPI_GPIO > > tristate "GPIO-based bitbanging SPI Master" > > depends on GENERIC_GPIO > > diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile > > index d7d0f89..377f845 100644 > > --- a/drivers/spi/Makefile > > +++ b/drivers/spi/Makefile > > @@ -21,6 +21,7 @@ obj-$(CONFIG_SPI_DAVINCI) += davinci_spi.o > > obj-$(CONFIG_SPI_DESIGNWARE) += dw_spi.o > > obj-$(CONFIG_SPI_DW_PCI) += dw_spi_pci.o > > obj-$(CONFIG_SPI_DW_MMIO) += dw_spi_mmio.o > > +obj-$(CONFIG_SPI_EP93XX) += ep93xx_spi.o > > obj-$(CONFIG_SPI_GPIO) += spi_gpio.o > > obj-$(CONFIG_SPI_IMX) += spi_imx.o > > obj-$(CONFIG_SPI_LM70_LLP) += spi_lm70llp.o > > diff --git a/drivers/spi/ep93xx_spi.c b/drivers/spi/ep93xx_spi.c > > new file mode 100644 > > index 0000000..f084838 > > --- /dev/null > > +++ b/drivers/spi/ep93xx_spi.c > > @@ -0,0 +1,1020 @@ > > +/* > > + * Driver for Cirrus Logic EP93xx SPI controller. > > + * > > + * Copyright (c) 2010 Mika Westerberg > > + * > > + * For more information about the SPI controller see documentation on > > Cirrus > > + * Logic web site: > > + * http://www.cirrus.com/en/pubs/manual/EP93xx_Users_Guide_UM1.pdf > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License version 2 as > > + * published by the Free Software Foundation. > > + */ > > + > > +#include <linux/io.h> > > +#include <linux/clk.h> > > +#include <linux/err.h> > > +#include <linux/delay.h> > > +#include <linux/device.h> > > +#include <linux/bitops.h> > > +#include <linux/module.h> > > +#include <linux/interrupt.h> > > +#include <linux/platform_device.h> > > +#include <linux/workqueue.h> > > +#include <linux/sched.h> > > +#include <linux/spi/spi.h> > > + > > +#include <mach/ep93xx_spi.h> > > + > > +#define SSPCR0 0x0000 > > +#define SSPCR0_MODE_SHIFT 6 > > +#define SSPCR0_SCR_SHIFT 8 > > + > > +#define SSPCR1 0x0004 > > +#define SSPCR1_RIE BIT(0) > > +#define SSPCR1_TIE BIT(1) > > +#define SSPCR1_RORIE BIT(2) > > +#define SSPCR1_LBM BIT(3) > > +#define SSPCR1_SSE BIT(4) > > +#define SSPCR1_MS BIT(5) > > +#define SSPCR1_SOD BIT(6) > > + > > +#define SSPDR 0x0008 > > + > > +#define SSPSR 0x000c > > +#define SSPSR_TFE BIT(0) > > +#define SSPSR_TNF BIT(1) > > +#define SSPSR_RNE BIT(2) > > +#define SSPSR_RFF BIT(3) > > +#define SSPSR_BSY BIT(4) > > +#define SSPCPSR 0x0010 > > + > > +#define SSPIIR 0x0014 > > +#define SSPIIR_RIS BIT(0) > > +#define SSPIIR_TIS BIT(1) > > +#define SSPIIR_RORIS BIT(2) > > +#define SSPICR SSPIIR > > + > > +/* timeout in milliseconds */ > > +#define SPI_TIMEOUT 100 > > + > > +/** > > + * struct ep93xx_spi - EP93xx SPI controller structure > > + * @lock: spinlock that protects concurrent accesses to fields @running, > > + * @current_msg and @msg_queue > > + * @pdev: pointer to platform device > > + * @info: platform specific info > > + * @clk: clock for the controller > > + * @regs_base: pointer to ioremap()'d registers > > + * @irq: IRQ number used by the driver > > + * @min_rate: minimum clock rate (in Hz) supported by the controller > > + * @max_rate: maximum clock rate (in Hz) supported by the controller > > + * @running: is the queue running > > + * @msg_queue: queue for the messages > > + * @wq: workqueue used by the driver > > + * @msg_work: work that is queued for the driver > > + * @wait: wait here until given transfer is completed > > + * @current_msg: message that is currently processed (or %NULL if none) > > + * @spi_dev: pointer to device that was previously selected. > > + * @tx: current byte in transfer to transmit > > + * @rx: current byte in transfer to receive > > + * > > + * This structure holds EP93xx SPI controller specific information. When > > + * @running is %true, driver accepts transfer requests from protocol > > drivers. > > + * @current_msg is used to hold pointer to the message that is currently > > + * processed. If @current_msg is %NULL, it means that no processing is > > going > > + * on. > > + * > > + * Most of the fields are only written once and they can be accessed > > without > > + * taking the @lock. Fields that are accessed concurrently are: > > @current_msg, > > + * @running and @msg_queue. > > + */ > > +struct ep93xx_spi { > > + spinlock_t lock; > > + const struct platform_device *pdev; > > + const struct ep93xx_spi_info *info; > > + struct clk *clk; > > + void __iomem *regs_base; > > + int irq; > > + unsigned long min_rate; > > + unsigned long max_rate; > > + bool running; > > + struct workqueue_struct *wq; > > + struct work_struct msg_work; > > + struct completion wait; > > + struct list_head msg_queue; > > + struct spi_message *current_msg; > > + size_t tx; > > + size_t rx; > > +}; > > + > > +/** > > + * struct ep93xx_spi_chip - SPI device hardware settings > > + * @rate: max rate in hz this chip supports > > + * @div_cpsr: cpsr (pre-scaler) divider > > + * @div_scr: scr divider > > + * @dss: bits per word (4 - 16 bits) > > + * @mode: chip SPI mode (see also &struct spi_device) > > + * > > + * This structure is used to store hardware register specific settings for > > each > > + * SPI device. Settings are written to hardware by function > > + * ep93xx_spi_chip_setup(). > > + */ > > +struct ep93xx_spi_chip { > > + unsigned long rate; > > + u8 div_cpsr; > > + u8 div_scr; > > + u8 dss; > > + u8 mode; > > +}; > > + > > +/* converts bits per word to CR0.DSS value */ > > +#define bits_per_word_to_dss(bpw) ((bpw) - 1) > > + > > +static inline void > > +ep93xx_spi_write_u8(const struct ep93xx_spi *espi, u16 reg, u8 value) > > +{ > > + __raw_writeb(value, espi->regs_base + reg); > > +} > > + > > +static inline u8 > > +ep93xx_spi_read_u8(const struct ep93xx_spi *spi, u16 reg) > > +{ > > + return __raw_readb(spi->regs_base + reg); > > +} > > + > > +static inline void > > +ep93xx_spi_write_u16(const struct ep93xx_spi *espi, u16 reg, u16 value) > > +{ > > + __raw_writew(value, espi->regs_base + reg); > > +} > > + > > +static inline u16 > > +ep93xx_spi_read_u16(const struct ep93xx_spi *spi, u16 reg) > > +{ > > + return __raw_readw(spi->regs_base + reg); > > +} > > + > > +/** > > + * ep93xx_spi_enable() - enables the SPI controller and clock > > + * @espi: ep93xx SPI controller struct > > + * > > + * This function enables the SPI controller and its clock. Returns %0 in > > case > > + * of success and negative error in case if failure. > > + */ > > +static int ep93xx_spi_enable(const struct ep93xx_spi *espi) > > +{ > > + u16 regval; > > + int err; > > + > > + err = clk_enable(espi->clk); > > + if (err) > > + return err; > > + > > + regval = ep93xx_spi_read_u16(espi, SSPCR1); > > + regval |= SSPCR1_SSE; > > + ep93xx_spi_write_u16(espi, SSPCR1, regval); > > + > > + return 0; > > +} > > + > > +/** > > + * ep93xx_spi_disable() - disables the SPI controller and clock > > + * @espi: ep93xx SPI controller struct > > + * > > + * Function disables SPI controller and its clock. > > + */ > > +static void ep93xx_spi_disable(const struct ep93xx_spi *espi) > > +{ > > + u16 regval; > > + > > + regval = ep93xx_spi_read_u16(espi, SSPCR1); > > + regval &= ~SSPCR1_SSE; > > + ep93xx_spi_write_u16(espi, SSPCR1, regval); > > + > > + clk_disable(espi->clk); > > +} > > + > > +/** > > + * ep93xx_spi_enable_interrupts() - enables all SPI interrupts > > + * @espi: ep93xx SPI controller struct > > + * > > + * Enables all SPI interrupts: receive overrun (ROR), transmit and receive. > > + */ > > +static inline void > > +ep93xx_spi_enable_interrupts(const struct ep93xx_spi *espi) > > +{ > > + u8 regval; > > + > > + regval = ep93xx_spi_read_u8(espi, SSPCR1); > > + regval |= (SSPCR1_RORIE | SSPCR1_TIE | SSPCR1_RIE); > > + ep93xx_spi_write_u8(espi, SSPCR1, regval); > > +} > > + > > +/** > > + * ep93xx_spi_disable_interrupts() - disables all SPI interrupts > > + * @espi: ep93xx SPI controller struct > > + * > > + * Disables all SPI interrupts. > > + */ > > +static inline void > > +ep93xx_spi_disable_interrupts(const struct ep93xx_spi *espi) > > +{ > > + u8 regval; > > + > > + regval = ep93xx_spi_read_u8(espi, SSPCR1); > > + regval &= ~(SSPCR1_RORIE | SSPCR1_TIE | SSPCR1_RIE); > > + ep93xx_spi_write_u8(espi, SSPCR1, regval); > > +} > > + > > +/** > > + * ep93xx_spi_flush() - flushes SPI RX fifo > > + * @espi: ep93xx SPI controller struct > > + * > > + * Function goes through RX fifo, reading words as long as RX fifo is not > > empty. > > + * When this function is finished RX fifo should be empty. > > + */ > > +static void ep93xx_spi_flush(const struct ep93xx_spi *espi) > > +{ > > + while (ep93xx_spi_read_u16(espi, SSPSR) & SSPSR_RNE) > > + (void) ep93xx_spi_read_u16(espi, SSPDR); > > > Drop the (void) cast, its ugly and unnecessary. Also, should this sleep > while waiting for the condition to be true? It's probably good to have a > timeout here also to prevent hangs in the case where the spi subsystem > goes off into the weeds. I've normally used to have (void) casts in places where I don't use return value on purpose. But I'll remove that. I could add some timeout similar than in function below if that is ok (not sure whether sleeping is necessary). > > +} > > + > > +/** > > + * ep93xx_spi_wait_busy() - waits SPI controller while it is busy > > + * @espi: ep93xx SPI controller struct > > + * @msecs: timeout in milliseconds to wait > > + * > > + * Function waits while SPI controller is busy in transferring frames. > > Returns > > + * %0 when controller is not busy anymore and %-ETIMEDOUT on timeout. Wait > > is > > + * implemented by busylooping so this function can also be called in atomic > > + * context. > > + */ > > +static int ep93xx_spi_wait_busy(const struct ep93xx_spi *espi, > > + unsigned long msecs) > > +{ > > + unsigned long timeout = jiffies + msecs_to_jiffies(msecs); > > + > > + while (time_before(jiffies, timeout)) { > > + if ((ep93xx_spi_read_u16(espi, SSPSR) & SSPSR_BSY) == 0) > > + return 0; > > + cpu_relax(); > > + } > > + > > + return -ETIMEDOUT; > > +} > > + > > +/** > > + * ep93xx_spi_calc_divisors() - calculates SPI clock divisors > > + * @espi: ep93xx SPI controller struct > > + * @chip: divisors are calculated for this chip > > + * @rate: maximum rate (in Hz) that this chip supports > > + * > > + * Function calculates cpsr (clock pre-scaler) and scr divisors based on > > + * given @rate and places them to @chip->div_cpsr and @chip->div_scr. If, > > + * for some reason, divisors cannot be calculated nothing is stored and > > + * %-EINVAL is returned. > > + */ > > +static int ep93xx_spi_calc_divisors(const struct ep93xx_spi *espi, > > + struct ep93xx_spi_chip *chip, > > + unsigned long rate) > > +{ > > + int cpsr, scr; > > + > > + /* > > + * Make sure that max value is between values supported by the > > + * controller. Note that minimum value is already checked in > > + * ep93xx_spi_transfer(). > > + */ > > + rate = clamp(rate, espi->min_rate, espi->max_rate); > > + > > + /* > > + * Calculate divisors so that we can get speed according the > > + * following formula: > > + * rate = max_speed / (cpsr * (1 + scr)) > > + * where max_speed is same as SPI clock rate. > > + * > > + * cpsr must be even number and starts from 2, scr can be any number > > + * between 0 and 255. > > + */ > > + for (cpsr = 2; cpsr <= 254; cpsr += 2) { > > + for (scr = 0; scr <= 255; scr++) { > > + if ((espi->max_rate / (cpsr * (scr + 1))) < rate) { > > + chip->div_scr = (u8)scr; > > + chip->div_cpsr = (u8)cpsr; > > + return 0; > > + } > > + } > > + } > > + > > + return -EINVAL; > > +} > > + > > +/** > > + * ep93xx_spi_select_device() - select given SPI device > > + * @espi: ep93xx SPI controller struct > > + * @spi: SPI device to select > > + * > > + * Function asserts GPIO chipselect line as specified in @spi->chip_select > > in > > + * board specific manner (calls @info->cs_control()). > > + * > > + * Note that this function is called from a thread context and can sleep. > > + */ > > +static void ep93xx_spi_select_device(const struct ep93xx_spi *espi, > > + struct spi_device *spi) > > +{ > > + const struct ep93xx_spi_info *info = espi->info; > > + unsigned value = (spi->mode & SPI_CS_HIGH) ? 1 : 0; > > + > > + info->cs_control(spi->chip_select, value, info->data); > > +} > > + > > +/** > > + * ep93xx_spi_deselect_device() - deselects given SPI device > > + * @espi: ep93xx SPI controller struct > > + * @spi: SPI device to deselect > > + * > > + * Function deasserts GPIO chipselect line as specified in > > @spi->chip_select in > > + * board specific manner (calls @info->cs_control()). > > + * > > + * Note that this function is called from a thread context and can sleep. > > + */ > > +static void ep93xx_spi_deselect_device(const struct ep93xx_spi *espi, > > + struct spi_device *spi) > > +{ > > + const struct ep93xx_spi_info *info = espi->info; > > + unsigned value = (spi->mode & SPI_CS_HIGH) ? 0 : 1; > > + > > + info->cs_control(spi->chip_select, value, info->data); > > +} > > + > > +/** > > + * ep93xx_spi_setup() - setup an SPI device > > + * @spi: SPI device to setup > > + * > > + * This function sets up SPI device mode, speed etc. Can be called multiple > > + * times for a single device. Returns %0 in case of success, negative > > error in > > + * case of failure. When this function returns success, the device is > > + * deselected. > > + */ > > +static int ep93xx_spi_setup(struct spi_device *spi) > > +{ > > + struct ep93xx_spi *espi = spi_master_get_devdata(spi->master); > > + struct ep93xx_spi_chip *chip; > > + > > + if (spi->bits_per_word < 4 || spi->bits_per_word > 16) { > > + dev_err(&espi->pdev->dev, "invalid bits per word %d\n", > > + spi->bits_per_word); > > + return -EINVAL; > > + } > > + > > + chip = spi_get_ctldata(spi); > > + if (!chip) { > > + chip = kzalloc(sizeof(*chip), GFP_KERNEL); > > + if (!chip) > > + return -ENOMEM; > > + > > + spi_set_ctldata(spi, chip); > > + } > > + > > + if (spi->max_speed_hz != chip->rate) { > > + int err; > > + > > + err = ep93xx_spi_calc_divisors(espi, chip, spi->max_speed_hz); > > + if (err != 0) { > > + spi_set_ctldata(spi, NULL); > > + kfree(chip); > > + return err; > > + } > > + chip->rate = spi->max_speed_hz; > > + } > > + > > + chip->mode = spi->mode; > > + chip->dss = bits_per_word_to_dss(spi->bits_per_word); > > + > > + ep93xx_spi_deselect_device(espi, spi); > > + return 0; > > +} > > + > > +/** > > + * ep93xx_spi_transfer() - queue message to be transferred > > + * @spi: target SPI device > > + * @msg: message to be transferred > > + * > > + * This function is called by SPI device drivers when they are going to > > transfer > > + * a new message. It simply puts the message in the queue and schedules > > + * workqueue to perform the actual transfer later on. > > + * > > + * Returns %0 on success and negative error in case of failure. > > + */ > > +static int ep93xx_spi_transfer(struct spi_device *spi, struct spi_message > > *msg) > > +{ > > + struct ep93xx_spi *espi = spi_master_get_devdata(spi->master); > > + struct spi_transfer *transfer; > > + unsigned long flags; > > + > > + if (!msg || !msg->complete) > > + return -EINVAL; > > + > > + spin_lock_irqsave(&espi->lock, flags); > > + if (!espi->running) { > > + spin_unlock_irqrestore(&espi->lock, flags); > > + return -ESHUTDOWN; > > + } > > + spin_unlock_irqrestore(&espi->lock, flags); > > + > > + /* first validate each transfer */ > > + list_for_each_entry(transfer, &msg->transfers, transfer_list) { > > + if (transfer->bits_per_word) { > > + if (transfer->bits_per_word < 4 || > > + transfer->bits_per_word > 16) > > + return -EINVAL; > > + } > > + if (transfer->speed_hz && transfer->speed_hz < espi->min_rate) > > + return -EINVAL; > > + } > > + > > + /* > > + * Now that we own the message, let's initialize it so that it is > > + * suitable for us. We use @msg->status to signal whether there was > > + * error in transfer and @msg->state is used to hold pointer to the > > + * current transfer (or %NULL if no active current transfer). > > + */ > > + msg->state = NULL; > > + msg->status = 0; > > + msg->actual_length = 0; > > + > > + spin_lock_irqsave(&espi->lock, flags); > > + list_add_tail(&msg->queue, &espi->msg_queue); > > + (void) queue_work(espi->wq, &espi->msg_work); > > > Either drop the (void) cast or handle any error returns correctly. queue_work() doesn't return error, only != 0 when work was not already in the queue. Hence the cast (I explicitly state that I don't use that return value for anything). But I'll drop the (void) cast. > > + spin_unlock_irqrestore(&espi->lock, flags); > > + > > + return 0; > > +} > > + > > +/** > > + * ep93xx_spi_cleanup() - cleans up master controller specific state > > + * @spi: SPI device to cleanup > > + * > > + * This function releases master controller specific state for given @spi > > + * device. > > + */ > > +static void ep93xx_spi_cleanup(struct spi_device *spi) > > +{ > > + struct ep93xx_spi_chip *chip; > > + > > + chip = spi_get_ctldata(spi); > > + if (chip) { > > + spi_set_ctldata(spi, NULL); > > + kfree(chip); > > + } > > +} > > + > > +/** > > + * ep93xx_spi_chip_setup() - configures hardware according to given @chip > > + * @espi: ep93xx SPI controller struct > > + * @chip: chip specific settings > > + * > > + * This function sets up the actual hardware registers with settings given > > in > > + * @chip. Note that no validation is done so make sure that callers > > validate > > + * settings before calling this. > > + */ > > +static void ep93xx_spi_chip_setup(const struct ep93xx_spi *espi, > > + const struct ep93xx_spi_chip *chip) > > +{ > > + u16 cr0; > > + > > + cr0 = chip->div_scr << SSPCR0_SCR_SHIFT; > > + cr0 |= (chip->mode & (SPI_CPHA | SPI_CPOL)) << SSPCR0_MODE_SHIFT; > > + cr0 |= chip->dss; > > + > > + dev_dbg(&espi->pdev->dev, "setup: mode %d, cpsr %d, scr %d, dss %d\n", > > + chip->mode, chip->div_cpsr, chip->div_scr, chip->dss); > > + dev_dbg(&espi->pdev->dev, "setup: cr0 %#x", cr0); > > + > > + ep93xx_spi_write_u8(espi, SSPCPSR, chip->div_cpsr); > > + ep93xx_spi_write_u16(espi, SSPCR0, cr0); > > +} > > + > > +/** > > + * ep93xx_spi_work() - process SPI messages one at a time > > + * @work: work struct > > + * > > + * Workqueue worker function. This function is called when there are new > > + * SPI messages to be processed. Message is taken out from the queue and > > then > > + * processed one transfer at a time. Chipselect is asserted during whole > > message > > + * (unless @transfer->cs_change is set). > > + * > > + * After message is transferred, protocol driver is notified by calling > > + * @msg->complete(). In case of error, @msg->status is set to negative > > error > > + * number, otherwise it contains zero (and @msg->actual_length is updated). > > + */ > > +static void ep93xx_spi_work(struct work_struct *work) > > +{ > > + struct ep93xx_spi *espi = container_of(work, struct ep93xx_spi, > > + msg_work); > > + struct ep93xx_spi_chip *chip; > > + struct spi_transfer *transfer; > > + struct spi_device *spi; > > + struct spi_message *msg; > > + > > + spin_lock_irq(&espi->lock); > > + if (!espi->running || espi->current_msg || > > + list_empty(&espi->msg_queue)) { > > + spin_unlock_irq(&espi->lock); > > + return; > > + } > > + msg = list_first_entry(&espi->msg_queue, struct spi_message, queue); > > + list_del_init(&msg->queue); > > + espi->current_msg = msg; > > + spin_unlock_irq(&espi->lock); > > + > > + /* > > + * Enable SPI controller and clock and flush the RX FIFO. > > + */ > > + if (ep93xx_spi_enable(espi)) { > > + dev_err(&espi->pdev->dev, "failed to enable SPI controller\n"); > > + return; > > + } > > + ep93xx_spi_flush(espi); > > + > > + spi = msg->spi; > > + chip = spi_get_ctldata(spi); > > + > > + /* > > + * Update SPI controller registers according to @spi device and assert > > + * the chipselect. > > + */ > > + ep93xx_spi_chip_setup(espi, chip); > > + ep93xx_spi_select_device(espi, spi); > > + > > + dev_dbg(&espi->pdev->dev, "starting next message to device %s\n", > > + dev_name(&spi->dev)); > > + > > + list_for_each_entry(transfer, &msg->transfers, transfer_list) { > > + bool restore_chip = false; > > > Might be worth moving the inside of this loop to its own function, since > the indentation level gets a bit deep. Some of the line splitting below > gets a bit hard to read. I was thinking the same when I wrote this but I somehow forgot to change it. I'll create separate function for this. > > + > > + msg->state = transfer; > > + espi->rx = 0; > > + espi->tx = 0; > > + > > + /* > > + * Handle any transfer specific settings if needed. We use > > + * temporary chip settings here and restore original later when > > + * this transfer is finished. > > + */ > > + if (transfer->speed_hz || transfer->bits_per_word) { > > + struct ep93xx_spi_chip tmp_chip = *chip; > > + > > + if (transfer->speed_hz) { > > + int err; > > + > > + err = ep93xx_spi_calc_divisors(espi, &tmp_chip, > > + transfer->speed_hz); > > + if (err) { > > + dev_err(&espi->pdev->dev, > > + "failed to adjust speed\n"); > > + msg->status = err; > > + break; > > + } > > + dev_dbg(&espi->pdev->dev, > > + "setting transfer speed to %u Hz", > > + transfer->speed_hz); > > + } > > + if (transfer->bits_per_word) { > > + tmp_chip.dss = bits_per_word_to_dss( > > + transfer->bits_per_word); > > + dev_dbg(&espi->pdev->dev, > > + "setting bits per word to %d", > > + transfer->bits_per_word); > > + } > > + > > + restore_chip = true; > > + ep93xx_spi_chip_setup(espi, &tmp_chip); > > + } > > + > > + /* > > + * Now everything is set up for the current transfer. Enable > > + * interrupts and wait for the transfer to complete. > > + */ > > + ep93xx_spi_enable_interrupts(espi); > > + wait_for_completion(&espi->wait); > > + > > + /* > > + * In case of error during transmit, we bail out from processing > > + * the message. > > + */ > > + if (msg->status) > > + break; > > + > > + dev_dbg(&espi->pdev->dev, "transferred %d byte(s)", > > + transfer->len); > > + > > + /* > > + * After this transfer is finished, perform any possible > > + * post-transfer actions requested by the protocol driver. > > + */ > > + if (transfer->delay_usecs) { > > + dev_dbg(&espi->pdev->dev, "delaying %d usecs\n", > > + transfer->delay_usecs); > > + udelay(transfer->delay_usecs); > > + } > > + if (transfer->cs_change) { > > + /* > > + * In case protocol driver is asking us to drop the > > + * chipselect briefly, we let the scheduler to handle > > + * any "delay" here. > > + */ > > + if (!list_is_last(&transfer->transfer_list, > > + &msg->transfers)) { > > + dev_dbg(&espi->pdev->dev, > > + "dropping chipselect briefly\n"); > > + ep93xx_spi_deselect_device(espi, spi); > > + cond_resched(); > > + ep93xx_spi_select_device(espi, spi); > > + } > > + } > > + > > + if (restore_chip) { > > + restore_chip = false; > > + ep93xx_spi_chip_setup(espi, chip); > > + } > > + } > > + > > + /* > > + * Now the whole message is transferred (or failed for some reason). We > > + * deselect the device and disable the SPI controller. > > + */ > > + ep93xx_spi_deselect_device(espi, spi); > > + ep93xx_spi_disable(espi); > > + > > + /* > > + * Update current message and re-schedule ourselves if there are more > > + * messages in the queue. > > + */ > > + spin_lock_irq(&espi->lock); > > + espi->current_msg = NULL; > > + if (espi->running && !list_empty(&espi->msg_queue)) > > + (void) queue_work(espi->wq, &espi->msg_work); > > > Drop the (void) cast or handle the return code. Ok (again queue_work() doesn't return error so I'll just drop the (void) cast). > > > + spin_unlock_irq(&espi->lock); > > + > > + dev_dbg(&espi->pdev->dev, > > + "finished with the message: status %d, len %d\n", > > + msg->status, msg->actual_length); > > + > > + /* notify the protocol driver that we are done with this message */ > > + msg->complete(msg->context); > > +} > > + > > +#define GENERATE_WRITER(type) > > \ > > +static void type##_writer(struct ep93xx_spi *espi, \ > > + const struct spi_transfer *transfer) \ > > +{ \ > > + type tx_val = 0; \ > > + \ > > + if (transfer->tx_buf) \ > > + tx_val = ((type *)transfer->tx_buf)[espi->tx]; \ > > + ep93xx_spi_write_##type(espi, SSPDR, tx_val); \ > > + espi->tx += sizeof(type); \ > > +} > > > I'm not hugely keen on the generator macros. There are only four > functions, so its not too bad writing them all out. If you want to keep > the macros, you could just pass the type in as one of the arguments and > avoid the generator thing, ie: > > #define ep93xx_spi_write(espi, transfer, type) > \ > { \ > type tx_val; \ > \ > if (transfer->tx_buf) \ > tx_val = ((type *)transfer->tx_buf[espi->tx]; \ > ep93xx_spi_write_##type(espi, SSPDR, tx_val); \ > espi->tx += sizeof(type); \ > } \ Yes, your version looks more readable. I'll see what I can do with this. > > > + > > +#define GENERATE_READER(type) > > \ > > +static void type##_reader(struct ep93xx_spi *espi, \ > > + struct spi_transfer *transfer) \ > > +{ \ > > + type rx_val; \ > > + \ > > + rx_val = ep93xx_spi_read_##type(espi, SSPDR); \ > > + if (transfer->rx_buf) \ > > + ((type *)transfer->rx_buf)[espi->rx] = rx_val; \ > > + espi->rx += sizeof(type); \ > > +} > > + > > +GENERATE_WRITER(u8) > > +GENERATE_READER(u8) > > +GENERATE_WRITER(u16) > > +GENERATE_READER(u16) > > + > > +/** > > + * bits_per_word() - returns bits per word for current message > > + */ > > +static inline int bits_per_word(const struct ep93xx_spi *espi) > > +{ > > + struct spi_message *msg = espi->current_msg; > > + struct spi_transfer *transfer = msg->state; > > + > > + return transfer->bits_per_word ?: msg->spi->bits_per_word; > > +} > > + > > +static void do_write(struct ep93xx_spi *espi, > > + struct spi_transfer *transfer) > > +{ > > + if (bits_per_word(espi) > 8) > > + u16_writer(espi, transfer); > > + else > > + u8_writer(espi, transfer); > > +} > > + > > +static void do_read(struct ep93xx_spi *espi, > > + struct spi_transfer *transfer) > > +{ > > + if (bits_per_word(espi) > 8) > > + u16_reader(espi, transfer); > > + else > > + u8_reader(espi, transfer); > > +} > > > > > ep93xx_do_read and ep93xx_do_write? The names you have a bit generic > looking IMHO. Yeah, I'll change them as you suggested. > > > +/** > > + * ep93xx_spi_read_write() - perform next RX/TX transfer > > + * @espi: ep93xx SPI controller struct > > + * > > + * This function transfers next bytes (or words) to/from RX/TX FIFOs. If > > called > > + * several times, the whole transfer will be completed. Returns %0 when > > current > > + * transfer was not yet completed otherwise length of the transfer (>%0). > > In > > + * case of error negative errno is returned. > > + * > > + * Although this function is currently only used by interrupt handler it is > > + * possible that in future we support polling transfers as well: hence > > calling > > + * context can be thread or atomic. > > + */ > > +static int ep93xx_spi_read_write(struct ep93xx_spi *espi) > > +{ > > + struct spi_transfer *transfer; > > + struct spi_message *msg; > > + unsigned long flags; > > + unsigned len; > > + > > + spin_lock_irqsave(&espi->lock, flags); > > + msg = espi->current_msg; > > + spin_unlock_irqrestore(&espi->lock, flags); > > + > > + transfer = msg->state; > > + len = transfer->len; > > + > > + /* > > + * Write as long as TX FIFO is not full. After every write we check > > + * whether RX FIFO has any new data in it (and in that case we read what > > + * ever was in the RX FIFO). > > + */ > > + while ((ep93xx_spi_read_u8(espi, SSPSR) & SSPSR_TNF) && > > + espi->tx < len) { > > + do_write(espi, transfer); > > + > > + if (ep93xx_spi_wait_busy(espi, SPI_TIMEOUT)) { > > + msg->status = -ETIMEDOUT; > > + return msg->status; > > + } > > + > > + while ((ep93xx_spi_read_u8(espi, SSPSR) & SSPSR_RNE) && > > + espi->rx < len) { > > + do_read(espi, transfer); > > + } > > + } > > + > > + /* is transfer finished? */ > > + if (espi->tx == len && espi->rx == len) { > > + msg->actual_length += len; > > + return len; > > + } > > + > > + return 0; > > +} > > + > > +/** > > + * ep93xx_spi_interrupt() - SPI interrupt handler > > + * @irq: IRQ number (not used) > > + * @dev_id: pointer to EP93xx controller struct > > + * > > + * This function handles TX/RX/ROR interrupts that come from the SPI > > controller. > > + * Returns %IRQ_HANDLED when interrupt was handled and %IRQ_NONE in case > > the > > + * @irq was not handled. > > + */ > > +static irqreturn_t ep93xx_spi_interrupt(int irq, void *dev_id) > > +{ > > + struct ep93xx_spi *espi = dev_id; > > + u8 irq_status; > > + > > + (void)irq; > > > > > Does this generate a warning otherwise? If so you can use the > __always_unused or __maybe_unused attributes from include/linux/compiler.h It doesn't generate any warning. I just wanted explicitly to discard this parameter. I can drop this line. > > + irq_status = ep93xx_spi_read_u8(espi, SSPIIR); > > + > > + if (!(irq_status & (SSPIIR_RORIS | SSPIIR_TIS | SSPIIR_RIS))) > > + return IRQ_NONE; /* not for us */ > > + > > + /* > > + * If we got ROR (receive overrun) interrupt we know that something is > > + * wrong. Just abort the message. > > + */ > > + if (unlikely(irq_status & SSPIIR_RORIS)) { > > + dev_warn(&espi->pdev->dev, > > + "receive overrun, aborting the message\n"); > > + > > + spin_lock(&espi->lock); > > + espi->current_msg->status = -EIO; > > + spin_unlock(&espi->lock); > > + } else { > > + /* > > + * Interrupt is either RX (RIS) or TX (TIS). For both cases we > > + * simply execute next data transfer. > > + */ > > + if (!ep93xx_spi_read_write(espi)) { > > + /* > > + * In normal case, there still is some processing left > > + * for current transfer. Let's wait for the next > > + * interrupt then. > > + */ > > + return IRQ_HANDLED; > > + } > > + } > > + > > + /* > > + * Current transfer is finished, either with error or with success. In > > + * any case we disable interrupts and notify the worker to handle > > + * any post-processing of the message. > > + */ > > + ep93xx_spi_disable_interrupts(espi); > > + complete(&espi->wait); > > + return IRQ_HANDLED; > > +} > > + > > +static int __init ep93xx_spi_probe(struct platform_device *pdev) > > +{ > > + struct spi_master *master; > > + struct ep93xx_spi_info *info; > > + struct ep93xx_spi *espi; > > + struct resource *res; > > + int error; > > + > > + info = pdev->dev.platform_data; > > + > > + master = spi_alloc_master(&pdev->dev, sizeof(*espi)); > > + if (!master) { > > + dev_err(&pdev->dev, "failed to allocate spi master\n"); > > + return -ENOMEM; > > + } > > + > > + master->setup = ep93xx_spi_setup; > > + master->transfer = ep93xx_spi_transfer; > > + master->cleanup = ep93xx_spi_cleanup; > > + master->bus_num = 0; > > + master->num_chipselect = info->num_chipselect; > > + master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH; > > + > > + platform_set_drvdata(pdev, master); > > + > > + espi = spi_master_get_devdata(master); > > + > > + espi->clk = clk_get(NULL, "spi_clk"); > > > You should be able to get the clock based on the device rather than the > name. I think Hartley had some notes on this. OK. Will do. > > + if (IS_ERR(espi->clk)) { > > + dev_err(&pdev->dev, "unable to get spi clock\n"); > > + error = PTR_ERR(espi->clk); > > + goto fail_release_master; > > + } > > + > > + spin_lock_init(&espi->lock); > > + init_completion(&espi->wait); > > + > > + /* > > + * Calculate maximum and minimum supported clock rates > > + * for the controller. > > + */ > > + espi->max_rate = clk_get_rate(espi->clk) / 2; > > + espi->min_rate = clk_get_rate(espi->clk) / (254 * 255); > > + espi->info = info; > > + espi->pdev = pdev; > > + > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + if (!res) { > > + dev_err(&pdev->dev, "unable to get iomem resource\n"); > > + error = -ENODEV; > > + goto fail_put_clock; > > + } > > + > > + res = request_mem_region(res->start, resource_size(res), pdev->name); > > + if (!res) { > > + dev_err(&pdev->dev, "unable to request iomem resources\n"); > > + error = -EBUSY; > > + goto fail_put_clock; > > + } > > + > > + espi->regs_base = ioremap(res->start, resource_size(res)); > > + if (!espi->regs_base) { > > + dev_err(&pdev->dev, "failed to map resources\n"); > > + error = -ENODEV; > > + goto fail_free_mem; > > + } > > + > > + espi->irq = platform_get_irq(pdev, 0); > > + if (espi->irq < 0) { > > + error = -EBUSY; > > + dev_err(&pdev->dev, "failed to get irq resources\n"); > > + goto fail_unmap_regs; > > + } > > + > > + error = request_irq(espi->irq, ep93xx_spi_interrupt, > > + IRQF_SHARED, dev_name(&pdev->dev), espi); > > > How come you pass IRQF_SHARED? Hmm.. this is some remnant when I started writing this. No need to pass that. I'll change that. > > + if (error) { > > + dev_err(&pdev->dev, "failed to request rx irq\n"); > > + goto fail_unmap_regs; > > + } > > + > > + espi->wq = create_singlethread_workqueue("ep93xx_spid"); > > + if (!espi->wq) { > > + dev_err(&pdev->dev, "unable to create workqueue\n"); > > + goto fail_free_irq; > > + } > > > Do we need a dedicated workqueue, or can we use the system wide one? Hard to say. Other SPI master drivers have their own workqueue so I used that convention. Thanks, MW > > + INIT_WORK(&espi->msg_work, ep93xx_spi_work); > > + INIT_LIST_HEAD(&espi->msg_queue); > > + espi->running = true; > > + > > + error = spi_register_master(master); > > + if (error) > > + goto fail_free_queue; > > + > > + dev_info(&pdev->dev, "EP93xx SPI Controller at 0x%08lx irq %d\n", > > + (unsigned long)res->start, espi->irq); > > + > > + return 0; > > + > > +fail_free_queue: > > + destroy_workqueue(espi->wq); > > +fail_free_irq: > > + free_irq(espi->irq, espi); > > +fail_unmap_regs: > > + iounmap(espi->regs_base); > > +fail_free_mem: > > + release_mem_region(res->start, resource_size(res)); > > +fail_put_clock: > > + clk_put(espi->clk); > > +fail_release_master: > > + spi_master_put(master); > > + platform_set_drvdata(pdev, NULL); > > + > > + return error; > > +} > > + > > +static int __exit ep93xx_spi_remove(struct platform_device *pdev) > > +{ > > + struct spi_master *master = platform_get_drvdata(pdev); > > + struct ep93xx_spi *espi = spi_master_get_devdata(master); > > + struct resource *res; > > + > > + spin_lock_irq(&espi->lock); > > + espi->running = false; > > + spin_unlock_irq(&espi->lock); > > + > > + destroy_workqueue(espi->wq); > > + > > + /* > > + * Complete remaining messages with %-ESHUTDOWN status. > > + */ > > + spin_lock_irq(&espi->lock); > > + while (!list_empty(&espi->msg_queue)) { > > + struct spi_message *msg; > > + > > + msg = list_first_entry(&espi->msg_queue, > > + struct spi_message, queue); > > + list_del_init(&msg->queue); > > + msg->status = -ESHUTDOWN; > > + spin_unlock_irq(&espi->lock); > > + msg->complete(msg->context); > > + spin_lock_irq(&espi->lock); > > + } > > + spin_unlock_irq(&espi->lock); > > + > > + free_irq(espi->irq, espi); > > + iounmap(espi->regs_base); > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + release_mem_region(res->start, resource_size(res)); > > + clk_put(espi->clk); > > + platform_set_drvdata(pdev, NULL); > > + > > + spi_unregister_master(master); > > + return 0; > > +} > > + > > +static struct platform_driver ep93xx_spi_driver = { > > + .driver = { > > + .name = "ep93xx-spi", > > + .owner = THIS_MODULE, > > + }, > > + .remove = __exit_p(ep93xx_spi_remove), > > +}; > > + > > +static int __init ep93xx_spi_init(void) > > +{ > > + return platform_driver_probe(&ep93xx_spi_driver, ep93xx_spi_probe); > > +} > > +module_init(ep93xx_spi_init); > > + > > +static void __exit ep93xx_spi_exit(void) > > +{ > > + platform_driver_unregister(&ep93xx_spi_driver); > > +} > > +module_exit(ep93xx_spi_exit); > > + > > +MODULE_DESCRIPTION("EP93xx SPI Controller driver"); > > +MODULE_AUTHOR("Mika Westerberg <mika.westerb...@iki.fi>"); > > +MODULE_LICENSE("GPL"); > > +MODULE_ALIAS("platform:ep93xx-spi"); > > > > > -- > Bluewater Systems Ltd - ARM Technology Solution Centre > > Ryan Mallon 5 Amuri Park, 404 Barbadoes St > r...@bluewatersys.com PO Box 13 889, Christchurch 8013 > http://www.bluewatersys.com New Zealand > Phone: +64 3 3779127 Freecall: Australia 1800 148 751 > Fax: +64 3 3779135 USA 1800 261 2934 ------------------------------------------------------------------------------ Download Intel® Parallel Studio Eval Try the new software tools for yourself. Speed compiling, find bugs proactively, and fine-tune applications for parallel performance. See why Intel Parallel Studio got high marks during beta. http://p.sf.net/sfu/intel-sw-dev _______________________________________________ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general