* Re: [PATCH] Topcliff: Update PCH_SPI driver to 2.6.35
2010-08-14 6:49 ` [PATCH] Topcliff: Update PCH_SPI driver to 2.6.35 Grant Likely
@ 2010-08-17 7:34 ` Masayuki Ohtake
2010-08-19 7:31 ` Linus Walleij
2010-08-27 13:30 ` Masayuki Ohtake
2 siblings, 0 replies; 7+ messages in thread
From: Masayuki Ohtake @ 2010-08-17 7:34 UTC (permalink / raw)
To: Grant Likely
Cc: meego-dev, LKML, David Brownell, spi-devel-general, qi.wang,
yong.y.wang, andrew.chih.howe.khor, arjan, gregkh
Hi Grant,
I will modify your indications ASAP.
Thanks, Ohtake
----- 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.
I understand.
>
> > ---
> > 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?
Input/Output Hub.
>
> > + 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.
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] Topcliff: Update PCH_SPI driver to 2.6.35
2010-08-14 6:49 ` [PATCH] Topcliff: Update PCH_SPI driver to 2.6.35 Grant Likely
2010-08-17 7:34 ` Masayuki Ohtake
@ 2010-08-19 7:31 ` Linus Walleij
2010-08-20 2:20 ` Masayuki Ohtake
2010-08-27 13:30 ` Masayuki Ohtake
2 siblings, 1 reply; 7+ messages in thread
From: Linus Walleij @ 2010-08-19 7:31 UTC (permalink / raw)
To: Masayuki Ohtak
Cc: meego-dev, LKML, David Brownell, spi-devel-general, qi.wang,
yong.y.wang, andrew.chih.howe.khor, arjan, gregkh, Grant Likely
Hi Masayuki,
Can't find the orginal patch on LKML or SPI-dev so guess it's on meego-dev.
I have to quote Grants comments to comment...
2010/8/14 Grant Likely <grant.likely@secretlab.ca>:
> 2010/8/10 Masayuki Ohtak <masa-korg@dsn.okisemi.com>:
>>(...)
>> +struct pch_spi_data {
>> + void __iomem *io_remap_addr;
>> + struct spi_master *p_master;
p_* is for "pointer" is it not? Hungarian notation is not liked in the
kernel, see Documentation/CodingStyle.
>> + struct work_struct work;
>> + struct workqueue_struct *p_wk_q;
Dito.
>> + 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;
This is hungarian notation again, signifying that this s8 (signed!)
should actually be a bool.
Change to bool and drop the b_ prefix.
>> + 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;
The hungarian p_* prefix issue again.
>> +};
>> +
>> +/**
>> + * 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;
Hungarian p*?
>> + u8 irq_reg_sts;
>> + u8 pci_req_sts;
>> + u8 suspend_sts;
>> + struct pch_spi_data *p_data[PCH_MAX_DEV];
Hungarian p_*?
>> (...)
>> +/**
>> + * 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;
Hungarian p_*
>> +
>> + /*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;
Horrible hungarian construct with t* prefix for "type",
And CamlCase. What's wrong with plain "ret"?
> 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 */
>> +
>From this point...
>> + 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);
>> + }
>> + }
>> +
To this point you are implementing an arrow antipattern.
For an in-depth explanation see:
http://c2.com/cgi/wiki?ArrowAntiPattern
if (noerror)
... much code ...
else
errorpath
All these should be converted to the form:
if (error)
errorpath
return errorcode / goto cleanup
... rest of the code ...
This removes one indent and make the code much easier to
read.
This in combination with the fact that you're obviously running
checkpatch on top and forcing it down to 80 chars/line gives this
weird looking code.
>> +static int pch_spi_probe(struct pci_dev *pdev, const struct pci_device_id
>> *id)
>> +{
>> +
>> + struct spi_master *p_master[PCH_MAX_DEV];
Hungarian p_* and even in hungarian it is also wrong because
this is an array of pointers not a plain pointer.
>> +
>> + struct pch_spi_board_data *p_board_dat;
Hungarian p_*
>> (...)
>> +static int pch_spi_resume(struct pci_dev *pdev)
>> +{
>> + int i;
>> + s32 retval;
s32?? What's wrong with an int?
>> +
>> + struct pch_spi_board_data *p_board_dat = pci_get_drvdata(pdev);
>> + dev_dbg(&pdev->dev, "%s ENTRY\n", __func__);
Here begins another deep nested arrow antipattern...
>> + if (p_board_dat == NULL) {
>> + dev_err(&pdev->dev,
>> + "%s pci_get_drvdata returned NULL\n", __func__);
>> + retval = -EFAULT;
return -EFAULT and de-indent, etc. You know the drill.
>> + } 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__);
>> + }
>> + }
>> + }
>> + }
All the way to here.
>> (...)
>> --- /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)
What does this #if() actually do?
Since the range in Kconfig is set to 1..2 I *think* this can be
rewritten
#if (CONFIG_PCH_SPI_PLATFORM_DEVICE_COUNT == 2)
Which makes a lot more sense.
>> + {
>> + .modalias = "spidev",
>> + .max_speed_hz = 1000000,
>> + .bus_num = 1,
>> + .chip_select = 1,
>> + .platform_data = NULL,
>> + .mode = SPI_MODE_0,
>> + },
>> +#endif
>> +};
>> +
>> +static __init int Load(void)
Rename from "Load" to pch_init or *_init whatever like everyone
else.
>> +{
>> + int iRetVal = -1;
Hungarian notation.
>> +
>> + if (!spi_register_board_info
>> + (pch_spi_slaves, ARRAY_SIZE(pch_spi_slaves)))
>> + iRetVal = 0;
>> +
>> + return iRetVal;
>> +}
>> +
>> +module_init(Load);
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] Topcliff: Update PCH_SPI driver to 2.6.35
2010-08-19 7:31 ` Linus Walleij
@ 2010-08-20 2:20 ` Masayuki Ohtake
0 siblings, 0 replies; 7+ messages in thread
From: Masayuki Ohtake @ 2010-08-20 2:20 UTC (permalink / raw)
To: Linus Walleij
Cc: meego-dev, LKML, David Brownell, spi-devel-general, qi.wang,
yong.y.wang, andrew.chih.howe.khor, arjan, gregkh, Grant Likely,
Morinaga
Hi Linus Walleij,
We will update your comments ASAP.
Thanks, Ohtake(OKISEMI)
----- Original Message -----
From: "Linus Walleij" <linus.ml.walleij@gmail.com>
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>; "Grant Likely" <grant.likely@secretlab.ca>
Sent: Thursday, August 19, 2010 4:31 PM
Subject: Re: [PATCH] Topcliff: Update PCH_SPI driver to 2.6.35
Hi Masayuki,
Can't find the orginal patch on LKML or SPI-dev so guess it's on meego-dev.
I have to quote Grants comments to comment...
2010/8/14 Grant Likely <grant.likely@secretlab.ca>:
> 2010/8/10 Masayuki Ohtak <masa-korg@dsn.okisemi.com>:
>>(...)
>> +struct pch_spi_data {
>> + void __iomem *io_remap_addr;
>> + struct spi_master *p_master;
p_* is for "pointer" is it not? Hungarian notation is not liked in the
kernel, see Documentation/CodingStyle.
>> + struct work_struct work;
>> + struct workqueue_struct *p_wk_q;
Dito.
>> + 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;
This is hungarian notation again, signifying that this s8 (signed!)
should actually be a bool.
Change to bool and drop the b_ prefix.
>> + 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;
The hungarian p_* prefix issue again.
>> +};
>> +
>> +/**
>> + * 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;
Hungarian p*?
>> + u8 irq_reg_sts;
>> + u8 pci_req_sts;
>> + u8 suspend_sts;
>> + struct pch_spi_data *p_data[PCH_MAX_DEV];
Hungarian p_*?
>> (...)
>> +/**
>> + * 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;
Hungarian p_*
>> +
>> + /*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;
Horrible hungarian construct with t* prefix for "type",
And CamlCase. What's wrong with plain "ret"?
> 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 */
>> +
>From this point...
>> + 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);
>> + }
>> + }
>> +
To this point you are implementing an arrow antipattern.
For an in-depth explanation see:
http://c2.com/cgi/wiki?ArrowAntiPattern
if (noerror)
... much code ...
else
errorpath
All these should be converted to the form:
if (error)
errorpath
return errorcode / goto cleanup
... rest of the code ...
This removes one indent and make the code much easier to
read.
This in combination with the fact that you're obviously running
checkpatch on top and forcing it down to 80 chars/line gives this
weird looking code.
>> +static int pch_spi_probe(struct pci_dev *pdev, const struct pci_device_id
>> *id)
>> +{
>> +
>> + struct spi_master *p_master[PCH_MAX_DEV];
Hungarian p_* and even in hungarian it is also wrong because
this is an array of pointers not a plain pointer.
>> +
>> + struct pch_spi_board_data *p_board_dat;
Hungarian p_*
>> (...)
>> +static int pch_spi_resume(struct pci_dev *pdev)
>> +{
>> + int i;
>> + s32 retval;
s32?? What's wrong with an int?
>> +
>> + struct pch_spi_board_data *p_board_dat = pci_get_drvdata(pdev);
>> + dev_dbg(&pdev->dev, "%s ENTRY\n", __func__);
Here begins another deep nested arrow antipattern...
>> + if (p_board_dat == NULL) {
>> + dev_err(&pdev->dev,
>> + "%s pci_get_drvdata returned NULL\n", __func__);
>> + retval = -EFAULT;
return -EFAULT and de-indent, etc. You know the drill.
>> + } 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__);
>> + }
>> + }
>> + }
>> + }
All the way to here.
>> (...)
>> --- /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)
What does this #if() actually do?
Since the range in Kconfig is set to 1..2 I *think* this can be
rewritten
#if (CONFIG_PCH_SPI_PLATFORM_DEVICE_COUNT == 2)
Which makes a lot more sense.
>> + {
>> + .modalias = "spidev",
>> + .max_speed_hz = 1000000,
>> + .bus_num = 1,
>> + .chip_select = 1,
>> + .platform_data = NULL,
>> + .mode = SPI_MODE_0,
>> + },
>> +#endif
>> +};
>> +
>> +static __init int Load(void)
Rename from "Load" to pch_init or *_init whatever like everyone
else.
>> +{
>> + int iRetVal = -1;
Hungarian notation.
>> +
>> + if (!spi_register_board_info
>> + (pch_spi_slaves, ARRAY_SIZE(pch_spi_slaves)))
>> + iRetVal = 0;
>> +
>> + return iRetVal;
>> +}
>> +
>> +module_init(Load);
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Topcliff: Update PCH_SPI driver to 2.6.35
2010-08-14 6:49 ` [PATCH] Topcliff: Update PCH_SPI driver to 2.6.35 Grant Likely
2010-08-17 7:34 ` Masayuki Ohtake
2010-08-19 7:31 ` Linus Walleij
@ 2010-08-27 13:30 ` Masayuki Ohtake
2010-08-27 17:15 ` Grant Likely
2 siblings, 1 reply; 7+ messages in thread
From: Masayuki Ohtake @ 2010-08-27 13:30 UTC (permalink / raw)
To: Grant Likely
Cc: meego-dev, LKML, David Brownell, spi-devel-general, Wang, Qi,
Wang, Yong Y, andrew.chih.howe.khor, arjan, gregkh
----- 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)
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] Topcliff: Update PCH_SPI driver to 2.6.35
2010-08-27 13:30 ` Masayuki Ohtake
@ 2010-08-27 17:15 ` Grant Likely
2010-08-31 6:55 ` Masayuki Ohtake
0 siblings, 1 reply; 7+ messages in thread
From: Grant Likely @ 2010-08-27 17:15 UTC (permalink / raw)
To: Masayuki Ohtake
Cc: meego-dev, LKML, David Brownell, spi-devel-general, Wang, Qi,
Wang, Yong Y, andrew.chih.howe.khor, arjan, gregkh
Hi Ohtake-san,
Thanks for the reply. Some more comments below.
On Fri, Aug 27, 2010 at 7:30 AM, Masayuki Ohtake
<masa-korg@dsn.okisemi.com> wrote:
> ----- 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 ?
You know the hardware better than I. Is there a configuration or
identification register that will tell you how many spi bus instances
there are? Regardless, then number of instances the driver can
support should not be hard coded into the kernel config. If it is
hard coded, then the driver will not support running a single kernel
on both kinds of hardware.
>
>>
>> > +
>> > +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.
I'm not sure what you mean. It sounds like you're talking about the
case where both this driver and spidev are compiled into the kernel
(not modules). Am I correct?
What is the problem that you're seeing with the init order? What is
the problem when spidev gets initialized before the driver?
Regardless, init order should not be an issue with merging
pch_spi_platform_devices and pch_spi_main into a single module.
>
>>
>> > +
>> > 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.
which also loses any form of typechecking. Use a static inlines
please, even if it does mean 4 functions. Hmmm.... Actually, given
that this is just set/clear bit operation, I'd prefer to see the
macros removed entirely and the |= and &=~ written directly in the
code.
>
>>
>> > +
>> > +/**
>> > + * 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.
No. Since each spi bus instance is separate without any shared data
between them, the mutex should be per-instance. Move it into the
private data structure and initialize it in the probe routine with mutex_init().
g.
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] Topcliff: Update PCH_SPI driver to 2.6.35
2010-08-27 17:15 ` Grant Likely
@ 2010-08-31 6:55 ` Masayuki Ohtake
0 siblings, 0 replies; 7+ messages in thread
From: Masayuki Ohtake @ 2010-08-31 6:55 UTC (permalink / raw)
To: Grant Likely
Cc: meego-dev, LKML, David Brownell, spi-devel-general, Wang, Qi,
Wang, Yong Y, andrew.chih.howe.khor, arjan, gregkh, Morinaga
>
> ----- Original Message -----
> From: "Grant Likely" <grant.likely@secretlab.ca>
> To: "Masayuki Ohtake" <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>; "Wang, Qi" <qi.wang@intel.com>; "Wang, Yong Y" <yong.y.wang@intel.com>;
<andrew.chih.howe.khor@intel.com>; <arjan@linux.intel.com>; <gregkh@suse.de>
> Sent: Saturday, August 28, 2010 2:15 AM
> Subject: Re: [PATCH] Topcliff: Update PCH_SPI driver to 2.6.35
>
>
> Hi Ohtake-san,
>
> Thanks for the reply. Some more comments below.
>
> On Fri, Aug 27, 2010 at 7:30 AM, Masayuki Ohtake
> <masa-korg@dsn.okisemi.com> wrote:
> > ----- 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 ?
>
> You know the hardware better than I. Is there a configuration or
> identification register that will tell you how many spi bus instances
> there are? Regardless, then number of instances the driver can
> support should not be hard coded into the kernel config. If it is
> hard coded, then the driver will not support running a single kernel
> on both kinds of hardware.
Topcliff doesn't have register which shows the number of spi bus interfaces.
This multi-channel code is for other IOH device's.
Thus, I think there is not another way except for hard coding.
If you can't accept our multi-cannel code, we must delete these code.
>
> >
> >>
> >> > +
> >> > +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.
>
> I'm not sure what you mean. It sounds like you're talking about the
> case where both this driver and spidev are compiled into the kernel
> (not modules). Am I correct?
>
> What is the problem that you're seeing with the init order? What is
> the problem when spidev gets initialized before the driver?
>
> Regardless, init order should not be an issue with merging
> pch_spi_platform_devices and pch_spi_main into a single module.
Following your comments, we will merge to a single file.
>
> >
> >>
> >> > +
> >> > 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.
>
> which also loses any form of typechecking. Use a static inlines
> please, even if it does mean 4 functions. Hmmm.... Actually, given
> that this is just set/clear bit operation, I'd prefer to see the
> macros removed entirely and the |= and &=~ written directly in the
> code.
Following your comments, we will replace inline functions..
>
> >
> >>
> >> > +
> >> > +/**
> >> > + * 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.
>
> No. Since each spi bus instance is separate without any shared data
> between them, the mutex should be per-instance. Move it into the
> private data structure and initialize it in the probe routine with mutex_init().
>
I understand.
We will move mutex variable into private data structure.
Thanks, Ohtake(OKISEMI)
^ permalink raw reply [flat|nested] 7+ messages in thread