lkml.org 
[lkml]   [2010]   [Aug]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    SubjectRe: [PATCH] Topcliff: Update PCH_SPI driver to 2.6.35
    Date
    ----- Original Message ----- 
    From: "Grant Likely" <grant.likely@secretlab.ca>
    To: "Masayuki Ohtak" <masa-korg@dsn.okisemi.com>
    Cc: <meego-dev@meego.com>; "LKML" <linux-kernel@vger.kernel.org>; "David Brownell" <dbrownell@users.sourceforge.net>;
    <spi-devel-general@lists.sourceforge.net>; <qi.wang@intel.com>; <yong.y.wang@intel.com>;
    <andrew.chih.howe.khor@intel.com>; <arjan@linux.intel.com>; <gregkh@suse.de>
    Sent: Saturday, August 14, 2010 3:49 PM
    Subject: Re: [PATCH] Topcliff: Update PCH_SPI driver to 2.6.35


    > 2010/8/10 Masayuki Ohtak <masa-korg@dsn.okisemi.com>:
    > > 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.
    >
    > > ---
    > > 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.

    How can the driver detect ?
    Could you show reference driver ?

    >
    > > +
    > > +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?

    "spi_pch_platform_device.o" must be installed than spidev.o.
    In case, spi_pch and spidev is integrated as module,
    it seems spidev is installed firstly.
    If you know the install order control, please how to control install ordering.

    >
    > > +
    > > 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.

    I understand your saying.
    But this macro uses for both setting value and returned value.
    If replacing inline function, we should make 4 functions(set/clear * void/u32)
    Thus, I want to use these macros.

    >
    > > +
    > > +/**
    > > + * 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.

    As to 'pch_spi_gcbptr', I agree with you.

    As to mutex, since many drivers accepted by upstream use 'static DEFINE_MUTEX',
    I think it is right to use it.

    >
    > > +/**
    > > + * 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.

    Yes, you are right.

    >
    > > + 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.
    I will remove.

    >
    > > +/**
    > > + * 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.
    I should have set bus number.
    Thus, I think
    p_master[j]->bus_num = i;

    >
    > > + 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?
    It seems I can't find API for spi_register_board_info.

    >
    > 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.
    >



    Thanks, Ohtake(OKISEMI)





    \
     
     \ /
      Last update: 2010-08-27 15:33    [W:0.186 / U:0.076 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site