2010/8/10 Masayuki Ohtak <masa-korg@dsn.okisemi.com>:
quoted text > 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@dsn.okisemi.com>
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.
quoted text > ---
> 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?
quoted text > + 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.
quoted text > +
> +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).
quoted text > +
> 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?
quoted text > +
> 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.
[...]
quoted text > +#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.
quoted text > +
> +/**
> + * 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.
quoted text > +/**
> + * 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.
quoted text > + 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.
quoted text > +
> + 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?
quoted text > +
> + 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.
quoted text > +/**
> + * 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.
quoted text > +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.
quoted text > + /*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.
quoted text > + }
> +
> + /*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.
quoted text > + 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.
quoted text > +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.
quoted text > + 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.
quoted text > + }
> +}
> +
> +
> +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.
quoted text > +
> + /*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.
quoted text > + } 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.
quoted text > + 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()
quoted text > +
> + 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.
quoted text > + p_master[j]->bus_num = 0;
You probably want -1 here so that the spi layer dynamically assigns an
spi bus number.
quoted text > + 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.
quoted text > +
> + 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.
quoted text > + 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.
quoted text > + 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;
}
quoted text > + 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;
quoted text > + } 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.
quoted text > +
> + 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.
quoted text > +
> +static __init int Load(void)
> +{
> + int iRetVal = -1;
int rc = -1;
quoted text > +
> + 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.
--
unsubscribe notice To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to
majordomo@vger.kernel.org
More majordomo info at
http://vger.kernel.org/majordomo-info.html
Please read the FAQ at
http://www.tux.org/lkml/ Messages in current thread:
Re: [PATCH] Topcliff: Update PCH_SPI driver to 2.6.35 , Grant Likely , (Fri Aug 13, 11:49 pm)
