Re: [PATCH] Topcliff: Update PCH_SPI driver to 2.6.35

[Grant Likely]

2010/8/10 Masayuki Ohtak <masa-korg@xxxxxxxxxxxxxxx>:
> SPI driver of Topcliff PCH
>
> Topcliff PCH is the platform controller hub that is going to be used in
> Intel's upcoming general embedded platform. All IO peripherals in
> Topcliff PCH are actually devices sitting on AMBA bus.
> Topcliff PCH has SPI I/F. Using this I/F, it is able to access system
> devices connected to SPI.
>
> Signed-off-by: Masayuki Ohtake <masa-korg@xxxxxxxxxxxxxxx>

Hi Masayuki.  Thanks for the patch.  The driver is mostly structured
well and looks fairly good.  However, there are quite a few stylistic
issues that should be easy to clean up, and a few technical concerns.
Comments below.

BTW, please make sure patches get sent out as plain-text, not html formatted.

> ---
>  drivers/spi/Kconfig                    |   26 +
>  drivers/spi/Makefile                   |    4 +
>  drivers/spi/pch_spi.h                  |  177 +++
>  drivers/spi/pch_spi_main.c             | 1823
> ++++++++++++++++++++++++++++++++
>  drivers/spi/pch_spi_platform_devices.c |   56 +
>  5 files changed, 2086 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/spi/pch_spi.h
>  create mode 100644 drivers/spi/pch_spi_main.c
>  create mode 100644 drivers/spi/pch_spi_platform_devices.c
>
> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index f55eb01..b6ae72a 100644
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -53,6 +53,32 @@ if SPI_MASTER
>
>  comment "SPI Master Controller Drivers"
>
> +config PCH_SPI_PLATFORM_DEVICE
> +	boolean
> +	default y if PCH_SPI
> +	help
> +	  This driver is for PCH SPI of Topcliff which is an IOH for x86
> +	  embedded processor.

What is IOH and abbreviation for?

> +	  This registers SPI devices for a given board.
> +
> +config PCH_SPI_PLATFORM_DEVICE_COUNT
> +	int "PCH SPI Bus count"
> +	range 1 2
> +	depends on PCH_SPI_PLATFORM_DEVICE
> +	help
> +	  This driver is for PCH SPI of Topcliff which is an IOH for x86
> +	  embedded processor.
> +	  The number of SPI buses/channels supported by the PCH SPI controller.
> +	  PCH SPI of Topcliff supports only one channel.

Being required to specific this at kernel config time isn't
multiplatform friendly.  Can the driver detect the number of busses at
runtime.

> +
> +config PCH_SPI
> +	tristate "PCH SPI Controller"
> +	depends on PCI
> +	help
> +	  This driver is for PCH SPI of Topcliff which is an IOH for x86
> +	  embedded processor.
> +	  This driver can access PCH SPI bus device.

Put config PCH_SPI above PCH_SPI_PLATFORM_DEVICE and make
PCH_SPI_PLATFORM_DEVICE depend on PCH_SPI.  (In fact, since
PCH_SPI_PLATFORM_DEVICE is always yes when PCH_SPI is selected, then
you probably don't need two configuration signals).

> +
>  config SPI_ATMEL
>  	tristate "Atmel SPI Controller"
>  	depends on (ARCH_AT91 || AVR32)
> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> index f3d2810..aecc873 100644
> --- a/drivers/spi/Makefile
> +++ b/drivers/spi/Makefile
> @@ -59,3 +59,7 @@ obj-$(CONFIG_SPI_TLE62X0)	+= tle62x0.o
>
>  # SPI slave drivers (protocol for that link)
>  # 	... add above this line ...
> +obj-$(CONFIG_PCH_SPI_PLATFORM_DEVICE) += pch_spi_platform_devices.o
> +obj-$(CONFIG_PCH_SPI) += pch_spi.o
> +pch_spi-objs := pch_spi_main.o

No need to use pch_spi-objs when there is only one .o file.  Just name
the .c file what you want the resulting kernel module to be named.
Also, please rename the modules to "spi_pch_platform_device.o" and
"spi_pch.o" (I'm enforcing all new spi drivers to start with the
prefix "spi_").

Finally, do pch_spi_platform_devices.o and pch_spi_main.o really need
to be in separate modules?

> +
> diff --git a/drivers/spi/pch_spi.h b/drivers/spi/pch_spi.h
> new file mode 100644
> index 0000000..b40377a
> --- /dev/null
> +++ b/drivers/spi/pch_spi.h
> @@ -0,0 +1,177 @@
> +/*
> + * Copyright (C) 2010 OKI SEMICONDUCTOR Co., LTD.
> + *

It is helpful to have a one line description of what this file is for
in at the top of the header comment block.

[...]
> +#define PCH_SET_BITMSK(var, bitmask)	((var) |= (bitmask))
> +#define PCH_CLR_BITMSK(var, bitmask)	((var) &= (~(bitmask)))

Because the macro has side-effects, I'd prefer to see static inlines
used here with an all lowercase name.

> +
> +/**
> + * struct pch_spi_data - Holds the SPI channel specific details
> + * @io_remap_addr:		The remapped PCI base address
> + * @p_master:			Pointer to the SPI master structure
> + * @work:			Reference to work queue handler
> + * @p_wk_q:			Workqueue for carrying out execution of the
> + *				requests
> + * @wait:			Wait queue for waking up upon receiving an
> + *				interrupt.
> + * @b_transfer_complete:		Status of SPI Transfer
> + * @bcurrent_msg_processing:	Status flag for message processing
> + * @lock:			Lock for protecting this structure
> + * @queue:			SPI Message queue
> + * @status:			Status of the SPI driver
> + * @bpw_len:			Length of data to be transferred in bits per
> + *				word
> + * @b_transfer_active:		Flag showing active transfer
> + * @tx_index:			Transmit data count; for bookkeeping during
> + *				transfer
> + * @rx_index:			Receive data count; for bookkeeping during
> + *				transfer
> + * @tx_buff:			Buffer for data to be transmitted
> + * @rx_index:			Buffer for Received data
> + * @n_curnt_chip:		The chip number that this SPI driver currently
> + *				operates on
> + * @p_current_chip:		Reference to the current chip that this SPI
> + *				driver currently operates on
> + * @p_current_msg:		The current message that this SPI driver is
> + *				handling
> + * @p_cur_trans:		The current transfer that this SPI driver is
> + *				handling
> + * @p_board_dat:		Reference to the SPI device data structure
> + */
> +struct pch_spi_data {
> +	void __iomem *io_remap_addr;
> +	struct spi_master *p_master;
> +	struct work_struct work;
> +	struct workqueue_struct *p_wk_q;
> +	wait_queue_head_t wait;
> +	u8 b_transfer_complete;
> +	u8 bcurrent_msg_processing;
> +	spinlock_t lock;
> +	struct list_head queue;
> +	u8 status;
> +	u32 bpw_len;
> +	s8 b_transfer_active;
> +	u32 tx_index;
> +	u32 rx_index;
> +	u16 *pkt_tx_buff;
> +	u16 *pkt_rx_buff;
> +	u8 n_curnt_chip;
> +	struct spi_device *p_current_chip;
> +	struct spi_message *p_current_msg;
> +	struct spi_transfer *p_cur_trans;
> +	struct pch_spi_board_data *p_board_dat;
> +};
> +
> +/**
> + * struct pch_spi_board_data - Holds the SPI device specific details
> + * @pdev:		Pointer to the PCI device
> + * @irq_reg_sts:	Status of IRQ registration
> + * @pci_req_sts:	Status of pci_request_regions
> + * @suspend_sts:	Status of suspend
> + * @p_data:		Pointer to SPI channel data structure
> + */
> +struct pch_spi_board_data {
> +	struct pci_dev *pdev;
> +	u8 irq_reg_sts;
> +	u8 pci_req_sts;
> +	u8 suspend_sts;
> +	struct pch_spi_data *p_data[PCH_MAX_DEV];
> +};
> +#endif
> diff --git a/drivers/spi/pch_spi_main.c b/drivers/spi/pch_spi_main.c
> new file mode 100644
> index 0000000..da87d92
> --- /dev/null
> +++ b/drivers/spi/pch_spi_main.c
> @@ -0,0 +1,1823 @@
> +/*
> + * Copyright (C) 2010 OKI SEMICONDUCTOR Co., LTD.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307,
> USA.
> + */
> +
> +#include <linux/pci.h>
> +#include <linux/wait.h>
> +#include <linux/spi/spi.h>
> +#include <linux/interrupt.h>
> +#include <linux/sched.h>
> +#include <linux/spi/spidev.h>
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/mutex.h>
> +#include "pch_spi.h"
> +
> +static struct pci_device_id pch_spi_pcidev_id[] = {
> +	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_GE_SPI)},
> +	{0,}
> +};
> +
> +static void (*pch_spi_gcbptr) (struct pch_spi_data *);
> +static DEFINE_MUTEX(pch_spi_mutex);

Are these statics really necessary?  I think the callback can be
removed (see my comment below).  The mutex should probably be a member
of the pch_spi_data structure.  Getting rid of the static symbols will
make it easier if the driver ever needs to support multiple instances
of the device.

> +/**
> + * ch_spi_disable_interrupts() - Disables specified interrupts
> + * @master:	Pointer to struct spi_master.
> + * @interrupt:	Interrups to be enabled.
> + */
> +static void pch_spi_disable_interrupts(struct spi_master *master, u8
> interrupt)
> +{
> +	u32 reg_val_spcr;
> +
> +	reg_val_spcr = pch_spi_readreg(master, PCH_SPCR);
> +
> +	dev_dbg(&master->dev, "%s SPCR content =%x\n",
> +		   __func__, reg_val_spcr);
> +
> +	switch (interrupt & 0x1f) {

Is it possible for more than one of these interrupt bits to be set at
the same time?  If so, then this switch() statement won't always work.

> +	case PCH_RFI:
> +		/*clear RFIE bit in SPCR */
> +		dev_dbg(&master->dev,
> +		  "%s clearing RFI in pch_spi_disable_interrupts\n", __func__);
> +		PCH_CLR_BITMSK(reg_val_spcr, SPCR_RFIE_BIT);
> +		break;
> +
> +	case PCH_TFI:
> +		/*clear TFIE bit in SPCR */
> +		dev_dbg(&master->dev,
> +		  "%s clearing TFI in pch_spi_disable_interrupts\n", __func__);
> +		PCH_CLR_BITMSK(reg_val_spcr, SPCR_TFIE_BIT);
> +		break;
> +
> +	case PCH_FI:
> +		/*clear FIE bit in SPCR */
> +		dev_dbg(&master->dev,
> +		  "%s clearing FI in pch_spi_disable_interrupts\n", __func__);
> +		PCH_CLR_BITMSK(reg_val_spcr, SPCR_FIE_BIT);
> +		break;
> +
> +	case PCH_ORI:
> +		/*clear ORIE bit in SPCR */
> +		dev_dbg(&master->dev,
> +		  "%s clearing ORI in pch_spi_disable_interrupts\n", __func__);
> +		PCH_CLR_BITMSK(reg_val_spcr, SPCR_ORIE_BIT);
> +		break;
> +
> +	case PCH_MDFI:
> +		/*clear MODFIE bit in SPCR */
> +		dev_dbg(&master->dev,
> +		  "%s clearing MDFI in pch_spi_disable_interrupts\n", __func__);
> +		PCH_CLR_BITMSK(reg_val_spcr, SPCR_MDFIE_BIT);
> +		break;
> +
> +	default:
> +		dev_info(&master->dev,
> +		 "%s Unknown interrupt(=0x%02x)\n", __func__, interrupt & 0x1f);
> +	}
> +
> +	pch_spi_writereg(master, PCH_SPCR, reg_val_spcr);
> +
> +	dev_dbg(&master->dev, "%s SPCR after disabling interrupts =%x\n",
> +							__func__, reg_val_spcr);
> +}
> +
> +/**
> + * pch_spi_handler() - Interrupt handler
> + * @irq:	The interrupt number.
> + * @dev_id:	Pointer to struct pch_spi_board_data.
> + */
> +static irqreturn_t pch_spi_handler(int irq, void *dev_id)
> +{
> +	/*channel & read/write indices */
> +	int dev, read_cnt;
> +
> +	/*SPSR content */
> +	u32 reg_spsr_val, reg_spcr_val;
> +
> +	/*book keeping variables */
> +	u32 n_read, tx_index, rx_index, bpw_len;
> +
> +	/*to hold channel data */
> +	struct pch_spi_data *p_data;
> +
> +	/*buffer to store rx/tx data */
> +	u16 *pkt_rx_buffer, *pkt_tx_buff;
> +
> +	/*register addresses */
> +	void __iomem *spsr;
> +	void __iomem *spdrr;
> +	void __iomem *spdwr;
> +
> +	/*remapped pci base address */
> +	void __iomem *io_remap_addr;
> +
> +	irqreturn_t tRetVal = IRQ_NONE;

nit: use lowercase for variable names.

> +
> +	struct pch_spi_board_data *p_board_dat =
> +					(struct pch_spi_board_data *)dev_id;
> +
> +	if (p_board_dat->suspend_sts == true) {
> +		dev_dbg(&p_board_dat->pdev->dev,
> +				"%s returning due to suspend\n", __func__);
> +	} else {
> +		for (dev = 0; dev < PCH_MAX_DEV; dev++) {
> +			p_data = p_board_dat->p_data[dev];
> +			io_remap_addr = p_data->io_remap_addr;
> +			spsr = io_remap_addr + PCH_SPSR;
> +
> +			reg_spsr_val = ioread32(spsr);
> +
> +			/*Check if the interrupt is for SPI device */
> +
> +			if (reg_spsr_val & (SPSR_FI_BIT | SPSR_RFI_BIT)) {
> +				dev_dbg(&p_board_dat->pdev->dev,
> +				  "SPSR in %s=%x\n", __func__, reg_spsr_val);
> +				/*clear interrupt */
> +				iowrite32(reg_spsr_val, spsr);
> +
> +				if (p_data->b_transfer_active == true) {
> +					rx_index = p_data->rx_index;
> +					tx_index = p_data->tx_index;
> +					bpw_len = p_data->bpw_len;
> +					pkt_rx_buffer = p_data->pkt_rx_buff;
> +					pkt_tx_buff = p_data->pkt_tx_buff;
> +
> +					spdrr = io_remap_addr + PCH_SPDRR;
> +					spdwr = io_remap_addr + PCH_SPDWR;
> +
> +					n_read = PCH_READABLE(reg_spsr_val);
> +
> +					for (read_cnt = 0; (read_cnt < n_read);
> +					     read_cnt++) {
> +						/*read data */
> +						pkt_rx_buffer[rx_index++] =
> +								ioread32(spdrr);
> +						/*write data */
> +
> +						if (tx_index < bpw_len) {
> +							iowrite32
> +							    (pkt_tx_buff
> +							     [tx_index++],
> +							     spdwr);
> +						}
> +					}
> +
> +					/*disable RFI if not needed */
> +					if ((bpw_len - rx_index) <=
> +					    PCH_MAX_FIFO_DEPTH) {
> +						dev_dbg(&p_board_dat->pdev->dev,
> +						  "%s disabling RFI as data "
> +						  "remaining=%d\n", __func__,
> +						  (bpw_len - rx_index));
> +
> +						reg_spcr_val =
> +						  ioread32(io_remap_addr +
> +								PCH_SPCR);
> +
> +						/*disable RFI */
> +						PCH_CLR_BITMSK(reg_spcr_val,
> +								SPCR_RFIE_BIT);
> +
> +						/*reset rx threshold */
> +						reg_spcr_val &=
> +							 MASK_RFIC_SPCR_BITS;
> +						reg_spcr_val |=
> +							(PCH_RX_THOLD_MAX <<
> +							SPCR_RFIC_FIELD);
> +
> +						iowrite32(PCH_CLR_BITMSK
> +							(reg_spcr_val,
> +								SPCR_RFIE_BIT),
> +							(io_remap_addr +
> +								PCH_SPCR));
> +					}
> +
> +					/*update counts */
> +					p_data->tx_index = tx_index;
> +
> +					p_data->rx_index = rx_index;
> +
> +					dev_dbg(&p_board_dat->pdev->dev,
> +						"%s rx_index=%d tx_index=%d "
> +						"nWritable=%d n_read=%d\n",
> +						__func__, rx_index, tx_index,
> +						(16 -
> +						(PCH_WRITABLE(reg_spsr_val))),
> +						n_read);
> +
> +				}
> +
> +				/*if transfer complete interrupt */
> +				if (reg_spsr_val & SPSR_FI_BIT) {
> +					dev_dbg(&p_board_dat->pdev->dev,
> +							"%s FI bit in SPSR"
> +							" set\n", __func__);
> +
> +					/*disable FI & RFI interrupts */
> +					pch_spi_disable_interrupts(p_data->
> +						p_master, PCH_FI | PCH_RFI);
> +
> +					/*transfer is completed;inform
> +						 pch_spi_process_messages */
> +
> +					if (pch_spi_gcbptr != NULL) {
> +						dev_dbg(&p_board_dat->pdev->dev,
> +						  "%s invoking callback\n",
> +						  __func__);
> +						(*pch_spi_gcbptr) (p_data);
> +					}
> +				}
> +
> +				tRetVal = IRQ_HANDLED;
> +			}
> +		}
> +	}

Wow, the nesting on this function is very deep, and hard to follow.
Candidate for refactoring?

> +
> +	dev_dbg(&p_board_dat->pdev->dev, "%s EXIT return value=%d\n",
> +							__func__, tRetVal);
> +
> +	return tRetVal;
> +}
> +
> +/**
> + * pch_spi_entcb() - Registers the callback function
> + * @pch_spi_cb:	Callback function pointer.
> + */
> +static void pch_spi_entcb(void (*pch_spi_cb) (struct pch_spi_data *))
> +{
> +	if (pch_spi_cb != NULL)
> +		pch_spi_gcbptr = pch_spi_cb;
> +}

This trivial function is used exactly once in the probe routine.  Just
set the variable in-place.  In fact, this doesn't need to be a
callback at all because it is *always* set to pch_spi_callback.
Removing it will make the driver simpler.

> +/**
> + * pch_spi_setup_transfe() - Configures the PCH SPI hardware for transfer
> + * @spi:	Pointer to struct spi_device.
> + */
> +static void pch_spi_setup_transfer(struct spi_device *spi)
> +{
> +	u32 reg_spcr_val;
> +
> +	dev_dbg(&spi->dev, "%s SPBRR content =%x setting baud rate=%d\n",
> +	  __func__, pch_spi_readreg(spi->master, PCH_SPBRR), spi->max_speed_hz);
> +
> +	pch_spi_set_baud_rate(spi->master, spi->max_speed_hz);
> +
> +	/*set bits per word */
> +	dev_dbg(&spi->dev, "%s :setting bits_per_word=%d\n",
> +						__func__, spi->bits_per_word);
> +	pch_spi_set_bits_per_word(spi->master, spi->bits_per_word);
> +
> +	dev_dbg(&spi->dev,
> +		"%s SPBRR content after setting baud rate & bits per word=%x\n",
> +		__func__, pch_spi_readreg(spi->master, PCH_SPBRR));
> +
> +	reg_spcr_val = pch_spi_readreg(spi->master, PCH_SPCR);
> +	dev_dbg(&spi->dev, "%s SPCR content = %x\n", __func__, reg_spcr_val);
> +
> +	/*set bit justification */
> +
> +	if ((spi->mode & SPI_LSB_FIRST) != 0) {
> +		/*LSB first */
> +		PCH_CLR_BITMSK(reg_spcr_val, SPCR_LSBF_BIT);
> +		dev_dbg(&spi->dev, "%s :setting LSBF bit to 0\n", __func__);
> +	} else {
> +		/*MSB first */
> +		PCH_SET_BITMSK(reg_spcr_val, SPCR_LSBF_BIT);
> +		dev_dbg(&spi->dev, "%s :setting LSBF bit to 1\n", __func__);
> +	}

There are a lot of these if/else blocks which call
PCH_{SET,CLR}_BITMSK().  The code could be simplified with a helper
function that implements the two paths.  Something like
pch_spi_clrsetbits() perhaps.


> +static int pch_spi_transfer(struct spi_device *pspi, struct spi_message
> *pmsg)
> +{
> +
> +	struct spi_transfer *p_transfer;
> +	struct pch_spi_data *p_data = spi_master_get_devdata(pspi->master);
> +	int retval;
> +	int ret;
> +
> +	ret = mutex_lock_interruptible(&pch_spi_mutex);
> +	if (ret) {
> +		retval = -ERESTARTSYS;
> +		goto err_return;
> +	}
> +
> +	/*validate spi message and baud rate */
> +	if (unlikely((list_empty(&pmsg->transfers) == 1) ||
> +		     ((pspi->max_speed_hz) == 0))) {
> +		if (list_empty(&pmsg->transfers) == 1)
> +			dev_err(&pspi->dev, "%s list empty\n", __func__);
> +
> +		if ((pspi->max_speed_hz) == 0) {
> +			dev_err(&pspi->dev, "%s pch_spi_tranfer maxspeed=%d\n",
> +					__func__, (pspi->max_speed_hz));
> +		}
> +		dev_err(&pspi->dev, "%s returning EINVAL\n", __func__);
> +
> +		retval = -EINVAL;
> +		goto err_return_mutex;
> +	}
> +
> +	dev_dbg(&pspi->dev,
> +	    "%s Transfer List not empty. Transfer Speed is set.\n", __func__);
> +
> +	/*validate Tx/Rx buffers and Transfer length */
> +	list_for_each_entry(p_transfer, &pmsg->transfers,
> +						 transfer_list) {
> +		if ((((p_transfer->tx_buf) == NULL)
> +		     && ((p_transfer->rx_buf) == NULL))
> +		    || (p_transfer->len == 0)) {
> +			if (((p_transfer->tx_buf) == NULL)
> +			    && ((p_transfer->rx_buf) == NULL)) {
> +				dev_err(&pspi->dev,
> +					"%s Tx and Rx buffer NULL\n", __func__);
> +			}
> +
> +			if (p_transfer->len == 0) {
> +				dev_err(&pspi->dev, "%s Transfer"
> +						" length invalid\n", __func__);
> +			}
> +
> +			dev_err(&pspi->dev, "%s returning EINVAL\n", __func__);
> +
> +			retval = -EINVAL;
> +			goto err_return_mutex;
> +		}
> +
> +		dev_dbg(&pspi->dev,
> +		    "%s Tx/Rx buffer valid. Transfer length valid\n", __func__);
> +
> +		/*if baud rate hs been specified validate the same */
> +		if (p_transfer->speed_hz) {
> +			if ((p_transfer->speed_hz) >
> +			    PCH_MAX_BAUDRATE) {
> +				retval = -EINVAL;
> +				dev_err(&pspi->dev,
> +				  "%s Invalid Baud rate\n", __func__);
> +				goto err_return_mutex;
> +			}

If the requested speed is too fast, then the maximum bus speed should
be used instead.

> +	/*copy Tx Data */
> +	if ((p_data->p_cur_trans->tx_buf) != NULL) {
> +		for (j = 0; j < (p_data->bpw_len); j++) {
> +			if (PCH_8_BPW == *bpw) {
> +				p_data->pkt_tx_buff[j] =
> +				    (((u8 *) (p_data->p_cur_trans->
> +					      tx_buf))[j]);
> +				dev_dbg(&p_data->p_master->dev, "%s=%x\n",
> +					__func__, (p_data->pkt_tx_buff[j]));
> +			} else {
> +				p_data->pkt_tx_buff[j] =
> +				    ((u16 *) (p_data->p_cur_trans->
> +					      tx_buf))[j];
> +				dev_dbg(&p_data->p_master->dev,
> +					"%s xmt data = %x\n", __func__,
> +					(p_data->pkt_tx_buff[j]));
> +			}
> +		}

Optimization note; by coding it this way, the (PCH_8_BPW == *bpw) test
is being performed on every iteration through the loop.  If you had
separate iterators for 8 and 16 bit transfer, the code will be faster.

> +	}
> +
> +	/*if len greater than PCH_MAX_FIFO_DEPTH,
> +				write 16,else len bytes */

Nit: It would be easier to read if the comment blocks were tidied up.
There should be a space between /* and the first word, and some
comments, like this one, will fit on a single line.

> +	if ((p_data->bpw_len) > PCH_MAX_FIFO_DEPTH)
> +		n_writes = PCH_MAX_FIFO_DEPTH;
> +	else
> +		n_writes = (p_data->bpw_len);
> +
> +	dev_dbg(&p_data->p_master->dev,
> +	  "\n%s:Pulling down SSN low - writing 0x2 to SSNXCR\n", __func__);

When splitting lines in the middle of an argument list, please indent
up to the same level as the first argument.

> +static void pch_spi_copy_rx_data(struct pch_spi_data *p_data, int bpw)
> +{
> +	int j;
> +	/*copy Rx Data */
> +	if ((p_data->p_cur_trans->rx_buf) != NULL) {

if (p_data->pcur_trans->rx_buf)
	return;

That reduces the indentation on the rest of the function by one level
and makes it easier to read and understand.

> +		for (j = 0; j < (p_data->bpw_len); j++) {
> +			if (PCH_8_BPW == bpw) {
> +				((u8 *) (p_data->p_cur_trans->rx_buf))[j] =
> +				    (u8) ((p_data->pkt_rx_buff[j]) & 0xFF);
> +
> +				dev_dbg(&p_data->p_master->dev,
> +					"rcv data in %s =%x\n", __func__,
> +					(p_data->pkt_rx_buff[j]));
> +
> +			} else {
> +				((u16 *) (p_data->p_cur_trans->rx_buf))[j] =
> +				    (u16) (p_data->pkt_rx_buff[j]);
> +				dev_dbg(&p_data->p_master->dev,
> +					"rcv data in %s = %x\n", __func__,
> +					(p_data->pkt_rx_buff[j]));
> +			}
> +		}

Same comment on optimization.

> +	}
> +}
> +
> +
> +static void pch_spi_process_messages(struct work_struct *pwork)
> +{
> +	struct spi_message *pmsg;
> +	int bpw;
> +
> +	struct pch_spi_data *p_data =
> +				container_of(pwork, struct pch_spi_data, work);
> +	dev_dbg(&p_data->p_master->dev,
> +					"%s p_data initialized\n", __func__);
> +
> +	spin_lock(&p_data->lock);
> +
> +	/*check if suspend has been initiated;if yes flush queue */
> +	if ((p_data->p_board_dat->suspend_sts)
> +	    || (p_data->status == STATUS_EXITING)) {
> +		dev_dbg(&p_data->p_master->dev,
> +		    "%s suspend/remove initiated,flushing queue\n", __func__);
> +		list_for_each_entry(pmsg, p_data->queue.next, queue) {
> +			pmsg->status = -EIO;
> +
> +			if (pmsg->complete != 0)
> +				pmsg->complete(pmsg->context);

The complete callback cannot be called while still holding the spinlock.

> +
> +			/*delete from queue */
> +			list_del_init(&pmsg->queue);
> +		}
> +
> +		spin_unlock(&p_data->lock);

Put a return statement here.  It will eliminate the else { }
indentation below; again improving readability.

> +	} else {
> +		p_data->bcurrent_msg_processing = true;
> +		dev_dbg(&p_data->p_master->dev,
> +		    "%s Set p_data->bcurrent_msg_processing= true\n",
> +		    __func__);
> +
> +		/*Get the message from the queue and delete it from there. */
> +		p_data->p_current_msg =
> +		    list_entry(p_data->queue.next, struct spi_message,
> +			       queue);
> +		dev_dbg(&p_data->p_master->dev,
> +				"%s :Got new message from queue\n", __func__);
> +		list_del_init(&p_data->p_current_msg->queue);
> +
> +		p_data->p_current_msg->status = 0;
> +
> +		dev_dbg(&p_data->p_master->dev,
> +				"%s :Invoking pch_spi_select_chip\n", __func__);
> +		pch_spi_select_chip(p_data, p_data->p_current_msg->spi);
> +
> +		spin_unlock(&p_data->lock);
> +
> +		do {
> +			/*If we are already processing a message get the next
> +			transfer structure from the message otherwise retrieve
> +			the 1st transfer request from the message. */
> +			spin_lock(&p_data->lock);
> +
> +			if (p_data->p_cur_trans == NULL) {
> +				p_data->p_cur_trans =
> +				    list_entry(p_data->p_current_msg->transfers.
> +					       next, struct spi_transfer,
> +					       transfer_list);
> +				dev_dbg(&p_data->p_master->dev,
> +					"%s :Getting 1st transfer message\n",
> +					__func__);
> +			} else {
> +				p_data->p_cur_trans =
> +				    list_entry(p_data->p_cur_trans->
> +					       transfer_list.next,
> +					       struct spi_transfer,
> +					       transfer_list);
> +				dev_dbg(&p_data->p_master->dev,
> +					"%s :Getting next transfer message\n",
> +					__func__);
> +			}
> +
> +			spin_unlock(&p_data->lock);
> +
> +			pch_spi_set_tx(p_data, &bpw, &pmsg);
> +
> +			/*Control interrupt*/
> +			pch_spi_set_ir(p_data);
> +
> +			/*Disable SPI transfer */
> +			dev_dbg(&p_data->p_master->dev,
> +				"%s:invoking pch_spi_set_enable to "
> +				"disable spi transfer\n", __func__);
> +			pch_spi_set_enable((p_data->p_current_chip), false);
> +
> +			/*clear FIFO */
> +			dev_dbg(&p_data->p_master->dev,
> +			    "%s:invoking pch_spi_clear_fifo to clear fifo\n",
> +			    __func__);
> +			pch_spi_clear_fifo(p_data->p_master);
> +
> +			/*copy Rx Data */
> +			pch_spi_copy_rx_data(p_data, bpw);
> +
> +			/*free memory */
> +			kfree(p_data->pkt_rx_buff);
> +			if (p_data->pkt_rx_buff)
> +				p_data->pkt_rx_buff = NULL;
> +
> +			kfree(p_data->pkt_tx_buff);
> +			if (p_data->pkt_tx_buff)
> +				p_data->pkt_tx_buff = NULL;
> +
> +			/*increment message count */
> +			p_data->p_current_msg->actual_length +=
> +			    p_data->p_cur_trans->len;
> +
> +			dev_dbg(&p_data->p_master->dev,
> +				"%s:p_data->p_current_msg->actual_length=%d\n",
> +				__func__, p_data->p_current_msg->actual_length);
> +
> +			/*check for delay */
> +			if (p_data->p_cur_trans->delay_usecs) {
> +				dev_dbg
> +				    (&p_data->p_master->dev, "%s:"
> +					"delay in usec=%d\n", __func__,
> +				     p_data->p_cur_trans->delay_usecs);
> +				udelay(p_data->p_cur_trans->delay_usecs);
> +			}
> +
> +			spin_lock(&p_data->lock);
> +
> +			/*No more transfer in this message. */
> +			if ((p_data->p_cur_trans->transfer_list.next) ==
> +			    &(p_data->p_current_msg->transfers)) {
> +				pch_spi_nomore_transfer(p_data, pmsg);
> +			}
> +
> +			spin_unlock(&p_data->lock);
> +
> +		} while ((p_data->p_cur_trans) != NULL);
> +	}
> +}
> +
> +static int pch_spi_probe(struct pci_dev *pdev, const struct pci_device_id
> *id)
> +{
> +
> +	struct spi_master *p_master[PCH_MAX_DEV];
> +
> +	struct pch_spi_board_data *p_board_dat;
> +	int retval, i, j, k, l, m;

nit: for loop index variables should be reused.  Don't use a new index
for every single for() loop.

> +	int spi_alloc_num, spi_master_num;
> +
> +	dev_dbg(&pdev->dev, "%s ENTRY\n", __func__);
> +	/* initialize the call back function */
> +	pch_spi_entcb(pch_spi_callback);
> +	dev_dbg(&pdev->dev, "%s invoked pch_spi_entcb\n", __func__);
> +
> +	/* allocate memory for private data */
> +	p_board_dat = kmalloc(sizeof(struct pch_spi_board_data), GFP_KERNEL);

kzalloc()

> +
> +	if (p_board_dat == NULL) {
> +		dev_err(&pdev->dev,
> +		    " %s memory allocation for private data failed\n",
> +		    __func__);
> +		retval = -ENOMEM;
> +		goto err_kmalloc;
> +	}
> +
> +	dev_dbg(&pdev->dev,
> +		"%s memory allocation for private data success\n", __func__);
> +
> +	/* enable PCI device */
> +	retval = pci_enable_device(pdev);
> +	if (retval != 0) {
> +		dev_err(&pdev->dev, "%s pci_enable_device FAILED\n", __func__);
> +
> +		goto err_pci_en_device;
> +	}
> +
> +	dev_dbg(&pdev->dev, "%s pci_enable_device returned=%d\n",
> +							__func__, retval);
> +
> +	p_board_dat->pdev = pdev;
> +
> +	/* alllocate memory for SPI master */
> +	for (i = 0, spi_alloc_num = 0; i < PCH_MAX_DEV; i++, spi_alloc_num++) {
> +		p_master[i] = spi_alloc_master(&pdev->dev,
> +				     sizeof(struct pch_spi_data));
> +
> +		if (p_master[i] == NULL) {
> +			retval = -ENOMEM;
> +			dev_err(&pdev->dev, "%s [ch=%d]Fail.\n", __func__, i);
> +			goto err_spi_alloc_master;
> +		}
> +	}
> +
> +	dev_dbg(&pdev->dev,
> +			"%s spi_alloc_master returned non NULL\n", __func__);
> +
> +	/* initialize members of SPI master */
> +	for (j = 0, spi_master_num = 0; j < PCH_MAX_DEV;
> +							j++, spi_master_num++) {

spi_master_num isn't used except for the error path, and it always has
the same value as 'j'.  Get it out of the loop initializer!  :-)  It
can be set in the error path.

> +		p_master[j]->bus_num = 0;

You probably want -1 here so that the spi layer dynamically assigns an
spi bus number.

> +		p_master[j]->num_chipselect = PCH_MAX_CS;
> +		p_master[j]->setup = pch_spi_setup;
> +		dev_dbg(&pdev->dev,
> +			"%s setup member of SPI master initialized\n",
> +			__func__);
> +		p_master[j]->transfer = pch_spi_transfer;
> +		dev_dbg(&pdev->dev,
> +		    "%s transfer member of SPI master initialized\n", __func__);
> +		p_master[j]->cleanup = pch_spi_cleanup;
> +		dev_dbg(&pdev->dev,
> +		    "%s cleanup member of SPI master initialized\n", __func__);
> +
> +		p_board_dat->p_data[j] =
> +				spi_master_get_devdata(p_master[j]);

Unnecessary line break.  There are a lot of these in this driver.

> +
> +		p_board_dat->p_data[j]->p_master = p_master[j];
> +		p_board_dat->p_data[j]->n_curnt_chip = 255;
> +		p_board_dat->p_data[j]->p_current_chip = NULL;
> +		p_board_dat->p_data[j]->b_transfer_complete = false;
> +		p_board_dat->p_data[j]->pkt_tx_buff = NULL;
> +		p_board_dat->p_data[j]->pkt_rx_buff = NULL;
> +		p_board_dat->p_data[j]->tx_index = 0;
> +		p_board_dat->p_data[j]->rx_index = 0;
> +		p_board_dat->p_data[j]->b_transfer_active = false;

The above 7 lines can be dropped when the code is changed to use
kzalloc().  kzalloc() clears the allocated memory.

> +		p_board_dat->p_data[j]->p_board_dat = p_board_dat;
> +		p_board_dat->suspend_sts = false;
> +
> +		/* Register the controller with the SPI core. */
> +		retval = spi_register_master(p_master[j]);
> +		if (retval != 0) {
> +			dev_err(&pdev->dev,
> +			   "%s spi_register_master FAILED\n", __func__);
> +			goto err_spi_reg_master;
> +		}
> +
> +		dev_dbg(&pdev->dev, "%s spi_register_master returned=%d\n",
> +							__func__, retval);
> +	}
> +
> +	/* allocate resources for PCH SPI */
> +	retval = pch_spi_get_resources(p_board_dat);
> +	if (retval != 0) {
> +		dev_err(&pdev->dev, "%s fail(retval=%d)\n",
> +							__func__, retval);
> +		goto err_spi_get_resources;
> +	}
> +
> +	dev_dbg(&pdev->dev, "%s pch_spi_get_resources returned=%d\n",
> +							__func__, retval);
> +
> +	/* save private data in dev */
> +	pci_set_drvdata(pdev, (void *)p_board_dat);

Remove the (void *) cast.  It should not be necessary.

> +	dev_dbg(&pdev->dev, "%s invoked pci_set_drvdata\n", __func__);
> +
> +	/* set master mode */
> +	for (k = 0; k < PCH_MAX_DEV; k++) {
> +		pch_spi_set_master_mode(p_master[k]);
> +		dev_dbg(&pdev->dev,
> +			"%s invoked pch_spi_set_master_mode\n", __func__);
> +	}
> +
> +	return 0;
> +
> +err_spi_get_resources:
> +	for (l = 0; l <= spi_master_num; l++)
> +		spi_unregister_master(p_master[l]);
> +err_spi_reg_master:
> +err_spi_alloc_master:
> +	for (m = 0; m <= spi_alloc_num; m++)
> +		spi_master_put(p_master[m]);
> +	pci_disable_device(pdev);
> +err_pci_en_device:
> +	kfree(p_board_dat);
> +err_kmalloc:
> +	return retval;
> +}
> +
> +static void pch_spi_remove(struct pci_dev *pdev)
> +{
> +	int i;
> +
> +	struct pch_spi_board_data *p_board_dat = pci_get_drvdata(pdev);
> +
> +	dev_dbg(&pdev->dev, "%s ENTRY\n", __func__);
> +
> +	if (p_board_dat != NULL) {

if (!p_board_dat) {
	dev_err(...);
	return;
}

> +		dev_dbg(&pdev->dev, "%s invoked pci_get_drvdata\n", __func__);
> +
> +		/* check for any pending messages */
> +		if ((-EBUSY) == pch_spi_check_request_pending(p_board_dat)) {
> +			dev_dbg(&pdev->dev,
> +			    "%s pch_spi_check_request_pending returned EBUSY\n",
> +			    __func__);
> +			/* no need to take any particular action;
> +				proceed with remove even
> +			   though queue is not empty */
> +		}
> +
> +		dev_dbg(&pdev->dev,
> +			"%s pch_spi_check_request_pending invoked\n", __func__);
> +
> +		/* Free resources allocated for PCH SPI */
> +		pch_spi_free_resources(p_board_dat);
> +		dev_dbg(&pdev->dev,
> +			"%s invoked pch_spi_free_resources\n", __func__);
> +
> +		/* Unregister SPI master */
> +		for (i = 0; i < PCH_MAX_DEV; i++) {
> +			spi_unregister_master(p_board_dat->p_data[i]->
> +					      p_master);
> +			dev_dbg(&pdev->dev,
> +				"%s invoked spi_unregister_master\n", __func__);
> +		}
> +
> +		/* free memory for private data */
> +		kfree(p_board_dat);
> +
> +		pci_set_drvdata(pdev, NULL);
> +
> +		dev_dbg(&pdev->dev,
> +				"%s memory for private data freed\n", __func__);
> +
> +		/* disable PCI device */
> +		pci_disable_device(pdev);
> +
> +		dev_dbg(&pdev->dev,
> +				"%s invoked pci_disable_device\n", __func__);
> +
> +	} else {
> +		dev_err(&pdev->dev,
> +			"%s pci_get_drvdata returned NULL\n", __func__);
> +	}
> +}
> +
> +#ifdef CONFIG_PM
> +static int pch_spi_suspend(struct pci_dev *pdev, pm_message_t state)
> +{
> +	int i;
> +	u8 count;
> +	s32 retval;
> +
> +	struct pch_spi_board_data *p_board_dat = pci_get_drvdata(pdev);
> +
> +	dev_dbg(&pdev->dev, "%s ENTRY\n", __func__);
> +
> +	if (p_board_dat == NULL) {
> +		dev_err(&pdev->dev,
> +			"%s pci_get_drvdata returned NULL\n", __func__);
> +		retval = -EFAULT;

Ditto here; return -EFAULT;

> +	} else {
> +		retval = 0;
> +		dev_dbg(&pdev->dev,
> +			"%s pci_get_drvdata invoked successfully\n", __func__);
> +		p_board_dat->suspend_sts = true;
> +		dev_dbg(&pdev->dev,
> +			"%s p_board_dat->bSuspending set to true\n", __func__);
> +
> +		/* check if the current message is processed:
> +		   Only after thats done the transfer will be suspended */
> +		for (i = 0; i < PCH_MAX_DEV; i++) {
> +			count = 255;
> +
> +			while ((--count) > 0) {
> +				if (p_board_dat->p_data[i]->
> +				    bcurrent_msg_processing == false) {
> +					dev_dbg(&pdev->dev, "%s p_board_dat->"
> +						"p_data->bCurrent_"
> +						"msg_processing = false\n",
> +						__func__);
> +					break;
> +				} else {
> +					dev_dbg(&pdev->dev, "%s p_board_dat->"
> +						"p_data->bCurrent_msg_"
> +						"processing = true\n",
> +						__func__);
> +				}
> +				msleep(PCH_SLEEP_TIME);
> +			}
> +		}
> +
> +		/* Free IRQ */
> +		if (p_board_dat->irq_reg_sts == true) {
> +			/* disable all interrupts */
> +			for (i = 0; i < PCH_MAX_DEV; i++) {
> +				pch_spi_disable_interrupts(p_board_dat->
> +							   p_data[i]->
> +							   p_master,
> +							   PCH_ALL);
> +				pch_spi_reset(p_board_dat->p_data[i]->
> +					      p_master);
> +				dev_dbg(&pdev->dev,
> +					"%s pch_spi_disable_interrupts invoked"
> +					"successfully\n", __func__);
> +			}
> +
> +			free_irq(p_board_dat->pdev->irq, (void *)p_board_dat);
> +
> +			p_board_dat->irq_reg_sts = false;
> +			dev_dbg(&pdev->dev,
> +				"%s free_irq invoked successfully."
> +				"p_data->irq_reg_sts = false\n",
> +				__func__);
> +		}
> +
> +		/* save config space */
> +		retval = pci_save_state(pdev);
> +
> +		if (retval == 0) {
> +			dev_dbg(&pdev->dev, "%s pci_save_state returned=%d\n",
> +							__func__, retval);
> +			/* disable PM notifications */
> +			pci_enable_wake(pdev, PCI_D3hot, 0);
> +			dev_dbg(&pdev->dev,
> +				"%s pci_enable_wake invoked successfully\n",
> +				__func__);
> +			/* disable PCI device */
> +			pci_disable_device(pdev);
> +			dev_dbg(&pdev->dev,
> +				"%s pci_disable_device invoked successfully\n",
> +				__func__);
> +			/* move device to D3hot  state */
> +			pci_set_power_state(pdev, PCI_D3hot);
> +			dev_dbg(&pdev->dev,
> +				"%s pci_set_power_state invoked successfully\n",
> +				__func__);
> +		} else {
> +			dev_err(&pdev->dev,
> +					"%s pci_save_state failed\n", __func__);
> +		}
> +	}
> +
> +	dev_dbg(&pdev->dev, "%s return=%d\n", __func__, retval);
> +
> +	return retval;
> +}
> +
> +static int pch_spi_resume(struct pci_dev *pdev)
> +{
> +	int i;
> +	s32 retval;
> +
> +	struct pch_spi_board_data *p_board_dat = pci_get_drvdata(pdev);
> +	dev_dbg(&pdev->dev, "%s ENTRY\n", __func__);
> +
> +	if (p_board_dat == NULL) {
> +		dev_err(&pdev->dev,
> +			"%s pci_get_drvdata returned NULL\n", __func__);
> +		retval = -EFAULT;
> +	} else {
> +		retval = 0;
> +		/* move device to DO power state */
> +		pci_set_power_state(pdev, PCI_D0);
> +		dev_dbg(&pdev->dev,
> +		    "%s pci_set_power_state invoked successfully\n", __func__);
> +
> +		/* restore state */
> +		pci_restore_state(pdev);
> +		dev_dbg(&pdev->dev,
> +		    "%s pci_restore_state invoked successfully\n", __func__);
> +
> +		retval = pci_enable_device(pdev);
> +		if (retval < 0) {
> +			dev_err(&pdev->dev,
> +				"%s pci_enable_device failed\n", __func__);
> +		} else {
> +			dev_dbg(&pdev->dev,
> +					"%s pci_enable_device returned=%d\n",
> +					__func__, retval);
> +
> +			/* disable PM notifications */
> +			pci_enable_wake(pdev, PCI_D3hot, 0);
> +			dev_dbg(&pdev->dev,
> +			 "%s pci_enable_wake invoked successfully\n", __func__);
> +
> +			/* register IRQ handler */
> +			if ((p_board_dat->irq_reg_sts) != true) {
> +				/* register IRQ */
> +				retval = request_irq(p_board_dat->pdev->irq,
> +						 pch_spi_handler, IRQF_SHARED,
> +						 DRIVER_NAME,
> +						 p_board_dat);
> +				if (retval < 0) {
> +					dev_err(&pdev->dev,
> +					   "%s request_irq failed\n", __func__);
> +				} else {
> +					dev_dbg(&pdev->dev, "%s request_irq"
> +					   " returned=%d\n", __func__, retval);
> +					p_board_dat->irq_reg_sts = true;
> +
> +					/* reset PCH SPI h/w */
> +					for (i = 0; i < PCH_MAX_DEV; i++) {
> +						pch_spi_reset(p_board_dat->
> +							      p_data[i]->
> +							      p_master);
> +						dev_dbg(&pdev->dev,
> +						  "pdev->dev, %s pch_spi_reset "
> +						  "invoked successfully\n",
> +						  __func__);
> +						pch_spi_set_master_mode
> +						    (p_board_dat->p_data[i]->
> +						     p_master);
> +						dev_dbg(&pdev->dev,
> +						  "%s pch_spi_set_master_mode "
> +						  "invoked successfully\n",
> +						  __func__);
> +					}
> +
> +					/* set suspend status to false */
> +					p_board_dat->suspend_sts = false;
> +
> +					dev_dbg(&pdev->dev, "%s set p_board_dat"
> +					   "->bSuspending = false\n", __func__);
> +				}
> +			}
> +		}
> +	}

Another very deeply indented function.

> +
> +	dev_dbg(&pdev->dev, "%s returning=%d\n", __func__, retval);
> +
> +	return retval;
> +}
> +#else
> +#define pch_spi_suspend NULL
> +#define pch_spi_resume NULL
> +
> +#endif
> +
> +static struct pci_driver pch_spi_pcidev = {
> +	.name = "pch_spi",
> +	.id_table = pch_spi_pcidev_id,
> +	.probe = pch_spi_probe,
> +	.remove = pch_spi_remove,
> +	.suspend = pch_spi_suspend,
> +	.resume = pch_spi_resume,
> +};
> +
> +static int __init pch_spi_init(void)
> +{
> +	return pci_register_driver(&pch_spi_pcidev);
> +}
> +
> +
> +static void __exit pch_spi_exit(void)
> +{
> +	pci_unregister_driver(&pch_spi_pcidev);
> +}
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("PCH SPI PCI Driver");
> +module_init(pch_spi_init);
> +module_exit(pch_spi_exit);
> +
> diff --git a/drivers/spi/pch_spi_platform_devices.c
> b/drivers/spi/pch_spi_platform_devices.c
> new file mode 100644
> index 0000000..a0d6488
> --- /dev/null
> +++ b/drivers/spi/pch_spi_platform_devices.c
> @@ -0,0 +1,56 @@
> +/*
> + * Copyright (C) 2010 OKI SEMICONDUCTOR Co., LTD.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307,
> USA.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/spi/spidev.h>
> +#include <linux/device.h>
> +#include <linux/spi/spi.h>
> +
> +static struct spi_board_info pch_spi_slaves[] = {
> +	{
> +	 .modalias = "spidev",
> +	 .max_speed_hz = 1000000,
> +	 .bus_num = 0,
> +	 .chip_select = 0,
> +	 .platform_data = NULL,
> +	 .mode = SPI_MODE_0,
> +	 },
> +#if (CONFIG_PCH_SPI_PLATFORM_DEVICE_COUNT - 1)
> +	{
> +	 .modalias = "spidev",
> +	 .max_speed_hz = 1000000,
> +	 .bus_num = 1,
> +	 .chip_select = 1,
> +	 .platform_data = NULL,
> +	 .mode = SPI_MODE_0,
> +	 },
> +#endif
> +};

This file is misnamed; these aren't platform_devices, they are spi_devices.

> +
> +static __init int Load(void)
> +{
> +	int iRetVal = -1;

int rc = -1;

> +
> +	if (!spi_register_board_info
> +	    (pch_spi_slaves, ARRAY_SIZE(pch_spi_slaves)))
> +		iRetVal = 0;
> +
> +	return iRetVal;
> +}
> +
> +module_init(Load);

How about when the module is removed?

This particular module (not the rest of the driver though) is really
rather a hack.  It seems like you need a method for creating spidev
devices from userspace at runtime, or having a method for specifying
spi device from the kernel command line.  Using a flattened device
tree fragment would work as well.

g.


-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/