Topcliff: Update PCH_SPI driver to 2.6.35
PATCH 1/1 Topcliff: Update PCH_SPI driver to 2.6.35
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.
--- 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
drivers/spi/Kconfig +26 -0
@@ -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. + 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. + +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. + config SPI_ATMEL tristate "Atmel SPI Controller" depends on (ARCH_AT91 || AVR32)
drivers/spi/Makefile +4 -0
@@ -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 +
drivers/spi/pch_spi.h new file +177 -0
177 lines not on this page · raw message
drivers/spi/pch_spi_main.c new file +1823 -0
1,823 lines not on this page · raw message
drivers/spi/pch_spi_platform_devices.c new file +56 -0
56 lines not on this page · raw message
-- 1.6.0.6
------------------------------------------------------------------------------
Sell apps to millions through the Intel(R) Atom(Tm) Developer Program Be part of this innovative community and reach millions of netbook users worldwide. Take advantage of special opportunities to increase revenue and speed time-to-market. Join now, and jumpstart your future.
http://p.sf.net/sfu/intel-atom-d2d
2010/8/10 Masayuki Ohtak <[email protected]>:
Quoted 9 lines
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 <[email protected]>
Hi Masayuki. Thanks for the patch. The driver is mostly structured well and looks fairly good. However, there are quite a few stylistic issues that should be easy to clean up, and a few technical concerns. Comments below.
BTW, please make sure patches get sent out as plain-text, not html formatted.
Quoted 17 lines
--- 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.cdrivers/spi/Kconfig +0
-0
drivers/spi/Kconfig if SPI_MASTER · line 61
Quoted 9 lines
drivers/spi/Kconfig +6
-0@@ -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?
drivers/spi/Kconfig if SPI_MASTER · line 72
Quoted 11 lines
drivers/spi/Kconfig +11
-0+ 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.
drivers/spi/Kconfig if SPI_MASTER · line 80
Quoted 8 lines
drivers/spi/Kconfig +8
-0+ +config PCH_SPI + tristate "PCH SPI Controller" + depends on PCI + help + This driver is for PCH SPI of Topcliff which is an IOH for x86 + embedded processor. + This driver can access PCH SPI bus device.
Put config PCH_SPI above PCH_SPI_PLATFORM_DEVICE and make PCH_SPI_PLATFORM_DEVICE depend on PCH_SPI. (In fact, since PCH_SPI_PLATFORM_DEVICE is always yes when PCH_SPI is selected, then you probably don't need two configuration signals).
Quoted 9 lines
drivers/spi/Kconfig +1
-0+ config SPI_ATMEL tristate "Atmel SPI Controller" depends on (ARCH_AT91 || AVR32)drivers/spi/Makefile +0
-0
drivers/spi/Makefile obj-$(CONFIG_SPI_TLE62X0) += tle62x0.o · line 64
Quoted 6 lines
drivers/spi/Makefile +3
-0@@ -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?
drivers/spi/pch_spi.h line 3
Quoted 10 lines
drivers/spi/Makefile +1
-0+drivers/spi/pch_spi.h new file +3
-0@@ -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.
[...]
drivers/spi/pch_spi.h line 103
Quoted 2 lines
drivers/spi/pch_spi.h new file +2
-0+#define PCH_SET_BITMSK (var, bitmask) ((var) |= (bitmask)) +#define PCH_CLR_BITMSK (var, bitmask) ((var) &= (~(bitmask)))
Because the macro has side-effects, I'd prefer to see static inlines used here with an all lowercase name.
Quoted 106 lines
drivers/spi/pch_spi.h new file +74
-0+ +/** +* 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 drivers/spi/pch_spi_main.c new file +15
-0@@ -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.
+ */drivers/spi/pch_spi_main.c new file +9
-0+ +#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>
drivers/spi/pch_spi_main.c line 35
Quoted 10 lines
drivers/spi/pch_spi_main.c new file +10
-0+#include <linux/mutex.h> +#include "pch_spi.h" + +static struct pci_device_id pch_spi_pcidev_id[] = { + {PCI_DEVICE (PCI_VENDOR_ID_INTEL ,PCI_DEVICE_ID_GE_SPI )}, + {0 ,} +}; + +static void (*pch_spi_gcbptr) (struct pch_spi_data *); +static DEFINE_MUTEX (pch_spi_mutex);
Are these statics really necessary? I think the callback can be removed (see my comment below). The mutex should probably be a member of the pch_spi_data structure. Getting rid of the static symbols will make it easier if the driver ever needs to support multiple instances of the device.
Quoted 7 lines
drivers/spi/pch_spi_main.c new file +6
-0+/** +* 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, u8interrupt)
drivers/spi/pch_spi_main.c line 100
Quoted 9 lines
drivers/spi/pch_spi_main.c new file +9
-0+{ +u32 reg_val_spcr; + + reg_val_spcr =pch_spi_readreg (master,PCH_SPCR ); + +dev_dbg (&master->dev,"%s SPCR content =%x\n" , + __func__, reg_val_spcr); + +switch (interrupt &0x1f ) {
Is it possible for more than one of these interrupt bits to be set at the same time? If so, then this switch() statement won't always work.
Quoted 67 lines
drivers/spi/pch_spi_main.c new file +67
-0+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;
drivers/spi/pch_spi_main.c line 205
Quoted 10 lines
drivers/spi/pch_spi_main.c new file +10
-0+ +/*register addresses */ +void __iomem *spsr; +void __iomem *spdrr; +void __iomem *spdwr; + +/*remapped pci base address */ +void __iomem *io_remap_addr; + +irqreturn_t tRetVal =IRQ_NONE ;
nit: use lowercase for variable names.
Quoted 110 lines
drivers/spi/pch_spi_main.c new file +110
-0+ +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,
drivers/spi/pch_spi_main.c line 325
Quoted 10 lines
drivers/spi/pch_spi_main.c new file +10
-0+"%s invoking callback\n" , + __func__); + (*pch_spi_gcbptr) (p_data); + } + } + + tRetVal =IRQ_HANDLED ; + } + } + }
Wow, the nesting on this function is very deep, and hard to follow. Candidate for refactoring?
Quoted 6 lines
drivers/spi/pch_spi_main.c new file +6
-0+ +dev_dbg (&p_board_dat->pdev->dev,"%s EXIT return value=%d\n" , + __func__, tRetVal); + +return tRetVal; +}
drivers/spi/pch_spi_main.c line 341
Quoted 10 lines
drivers/spi/pch_spi_main.c new file +10
-0+ +/** +* pch_spi_entcb() - Registers the callback function +* @pch_spi_cb: Callback function pointer. +*/ +static void pch_spi_entcb (void (*pch_spi_cb) (struct pch_spi_data *)) +{ +if (pch_spi_cb !=NULL ) + pch_spi_gcbptr = pch_spi_cb; +}
This trivial function is used exactly once in the probe routine. Just set the variable in-place. In fact, this doesn't need to be a callback at all because it is *always* set to pch_spi_callback. Removing it will make the driver simpler.
Quoted 26 lines
drivers/spi/pch_spi_main.c new file +26
-0+/** +* 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 */
drivers/spi/pch_spi_main.c line 456
Quoted 10 lines
drivers/spi/pch_spi_main.c new file +10
-0+ +if ((spi->mode &SPI_LSB_FIRST ) !=0 ) { +/*LSB first */ +PCH_CLR_BITMSK (reg_spcr_val,SPCR_LSBF_BIT ); +dev_dbg (&spi->dev,"%s :setting LSBF bit to 0\n" , __func__); + }else { +/*MSB first */ +PCH_SET_BITMSK (reg_spcr_val,SPCR_LSBF_BIT ); +dev_dbg (&spi->dev,"%s :setting LSBF bit to 1\n" , __func__); + }
There are a lot of these if/else blocks which call PCH_{SET,CLR}_BITMSK(). The code could be simplified with a helper function that implements the two paths. Something like pch_spi_clrsetbits() perhaps.
Quoted 59 lines
+static int
pch_spi_transfer(struct spi_device *pspi, struct spi_message *pmsg)drivers/spi/pch_spi_main.c new file +57
-0+{ + +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__);
drivers/spi/pch_spi_main.c line 689
Quoted 10 lines
drivers/spi/pch_spi_main.c new file +10
-0+ +/*if baud rate hs been specified validate the same */ +if (p_transfer->speed_hz) { +if ((p_transfer->speed_hz) > +PCH_MAX_BAUDRATE ) { + retval = -EINVAL ; +dev_err (&pspi->dev, +"%s Invalid Baud rate\n" , __func__); + goto err_return_mutex; + }
If the requested speed is too fast, then the maximum bus speed should be used instead.
Quoted 8 lines
drivers/spi/pch_spi_main.c new file +8
-0+/*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",
drivers/spi/pch_spi_main.c line 881
Quoted 10 lines
drivers/spi/pch_spi_main.c new file +10
-0+ __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.
drivers/spi/pch_spi_main.c line 885
Quoted 4 lines
drivers/spi/pch_spi_main.c new file +4
-0+ } + +/*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.
drivers/spi/pch_spi_main.c line 892
Quoted 7 lines
drivers/spi/pch_spi_main.c new file +7
-0+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.
drivers/spi/pch_spi_main.c line 1029
Quoted 5 lines
drivers/spi/pch_spi_main.c new file +5
-0+static void pch_spi_copy_rx_data (struct pch_spi_data *p_data,int bpw) +{ +int j; +/*copy Rx Data */ +if ((p_data->p_cur_trans->rx_buf) !=NULL ) {
if (p_data->pcur_trans->rx_buf)
return;
That reduces the indentation on the rest of the function by one level and makes it easier to read and understand.
Quoted 7 lines
drivers/spi/pch_spi_main.c new file +7
-0+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__,
drivers/spi/pch_spi_main.c line 1046
Quoted 10 lines
drivers/spi/pch_spi_main.c new file +10
-0+ (p_data->pkt_rx_buff[j])); + + }else { + ((u16 *) (p_data->p_cur_trans->rx_buf))[j] = + (u16) (p_data->pkt_rx_buff[j]); +dev_dbg (&p_data->p_master->dev, +"rcv data in %s = %x\n" , __func__, + (p_data->pkt_rx_buff[j])); + } + }
Same comment on optimization.
Quoted 16 lines
drivers/spi/pch_spi_main.c new file +16
-0+ } +} + + +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); +
drivers/spi/pch_spi_main.c line 1072
Quoted 10 lines
drivers/spi/pch_spi_main.c new file +10
-0+/*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.
drivers/spi/pch_spi_main.c line 1078
Quoted 6 lines
drivers/spi/pch_spi_main.c new file +6
-0+ +/*delete from queue */ +list_del_init (&pmsg->queue); + } + +spin_unlock (&p_data->lock);
Put a return statement here. It will eliminate the else { } indentation below; again improving readability.
Quoted 111 lines
drivers/spi/pch_spi_main.c new file +110
-0+ }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)
drivers/spi/pch_spi_main.c line 1418
Quoted 6 lines
drivers/spi/pch_spi_main.c new file +6
-0+{ + +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.
drivers/spi/pch_spi_main.c line 1427
Quoted 9 lines
drivers/spi/pch_spi_main.c new file +9
-0+int spi_alloc_num, spi_master_num; + +dev_dbg (&pdev->dev,"%s ENTRY\n" , __func__); +/* initialize the call back function */ +pch_spi_entcb (pch_spi_callback); +dev_dbg (&pdev->dev,"%s invoked pch_spi_entcb\n" , __func__); + +/* allocate memory for private data */ + p_board_dat =kmalloc (sizeof (struct pch_spi_board_data ),GFP_KERNEL );
kzalloc()
Quoted 33 lines
drivers/spi/pch_spi_main.c new file +33
-0+ +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);
drivers/spi/pch_spi_main.c line 1470
Quoted 10 lines
drivers/spi/pch_spi_main.c new file +10
-0+ 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.
drivers/spi/pch_spi_main.c line 1471
Quoted 1 line
drivers/spi/pch_spi_main.c new file +1
-0+ p_master[j]->bus_num =0 ;
You probably want -1 here so that the spi layer dynamically assigns an spi bus number.
Quoted 4 lines
drivers/spi/pch_spi_main.c new file +4
-0+ 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",
drivers/spi/pch_spi_main.c line 1485
Quoted 10 lines
drivers/spi/pch_spi_main.c new file +10
-0+ __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.
drivers/spi/pch_spi_main.c line 1495
Quoted 10 lines
drivers/spi/pch_spi_main.c new file +10
-0+ + p_board_dat->p_data[j]->p_master = p_master[j]; + p_board_dat->p_data[j]->n_curnt_chip =255 ; + p_board_dat->p_data[j]->p_current_chip =NULL ; + p_board_dat->p_data[j]->b_transfer_complete = false; + p_board_dat->p_data[j]->pkt_tx_buff =NULL ; + p_board_dat->p_data[j]->pkt_rx_buff =NULL ; + p_board_dat->p_data[j]->tx_index =0 ; + p_board_dat->p_data[j]->rx_index =0 ; + p_board_dat->p_data[j]->b_transfer_active = false;
The above 7 lines can be dropped when the code is changed to use kzalloc(). kzalloc() clears the allocated memory.
Quoted 18 lines
drivers/spi/pch_spi_main.c new file +18
-0+ 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 ) {
drivers/spi/pch_spi_main.c line 1523
Quoted 10 lines
drivers/spi/pch_spi_main.c new file +10
-0+dev_err (&pdev->dev,"%s fail(retval=%d)\n" , + __func__, retval); + goto err_spi_get_resources; + } + +dev_dbg (&pdev->dev,"%s pch_spi_get_resources returned=%d\n" , + __func__, retval); + +/* save private data in dev */ +pci_set_drvdata (pdev, (void *)p_board_dat);
Remove the (void *) cast. It should not be necessary.
Quoted 24 lines
drivers/spi/pch_spi_main.c new file +24
-0+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; +}
drivers/spi/pch_spi_main.c line 1557
Quoted 10 lines
drivers/spi/pch_spi_main.c new file +10
-0+ +static void pch_spi_remove (struct pci_dev *pdev) +{ +int i; + +struct pch_spi_board_data *p_board_dat =pci_get_drvdata (pdev); + +dev_dbg (&pdev->dev,"%s ENTRY\n" , __func__); + +if (p_board_dat !=NULL ) {
if (!p_board_dat) {
dev_err(...);
return;
}Quoted 53 lines
drivers/spi/pch_spi_main.c new file +53
-0+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;
drivers/spi/pch_spi_main.c line 1620
Quoted 10 lines
drivers/spi/pch_spi_main.c new file +10
-0+s32 retval; + +struct pch_spi_board_data *p_board_dat =pci_get_drvdata (pdev); + +dev_dbg (&pdev->dev,"%s ENTRY\n" , __func__); + +if (p_board_dat ==NULL ) { +dev_err (&pdev->dev, +"%s pci_get_drvdata returned NULL\n" , __func__); + retval = -EFAULT ;
Ditto here; return -EFAULT;
Quoted 157 lines
drivers/spi/pch_spi_main.c new file +157
-0+ }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__); + }
drivers/spi/pch_spi_main.c line 1787
Quoted 10 lines
drivers/spi/pch_spi_main.c new file +10
-0+ +/* set suspend status to false */ + p_board_dat->suspend_sts = false; + +dev_dbg (&pdev->dev,"%s set p_board_dat" +"->bSuspending = false\n" , __func__); + } + } + } + }
Another very deeply indented function.
Quoted 76 lines
drivers/spi/pch_spi_main.c new file +36
-0+ +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); +drivers/spi/pch_spi_platform_devices.c new file +15
-0@@ -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.
+ */drivers/spi/pch_spi_platform_devices.c new file +16
-0+ +#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)
drivers/spi/pch_spi_platform_devices.c line 42
Quoted 10 lines
drivers/spi/pch_spi_platform_devices.c new file +10
-0+ { + .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.
drivers/spi/pch_spi_platform_devices.c line 46
Quoted 4 lines
drivers/spi/pch_spi_platform_devices.c new file +4
-0+ +static __init int Load (void ) +{ +int iRetVal =-1 ;
int rc = -1;
drivers/spi/pch_spi_platform_devices.c line 55
Quoted 9 lines
drivers/spi/pch_spi_platform_devices.c new file +9
-0+ +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.
Signature · 2 lines
Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd.
Hi Grant,
I will modify your indications ASAP.
Thanks, Ohtake
----- Original Message -----
From: "Grant Likely" <[email protected]>
To: "Masayuki Ohtak" <[email protected]>
Cc: <[email protected]>; "LKML" <[email protected]>; "David Brownell" <[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>
Sent: Saturday, August 14, 2010 3:49 PM
Subject: Re: [PATCH] Topcliff: Update PCH_SPI driver to 2.6.35
Quoted 9 lines
2010/8/10 Masayuki Ohtak <[email protected]>:
Quoted 8 lines
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.
Quoted 8 lines
Quoted 1 line
Signed-off-by Masayuki Ohtake <[email protected]>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.
Quoted 21 lines
Quoted 21 lines
--- 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.cdrivers/spi/Kconfig +0
-0@@ -53,6 +53,32 @@ if SPI_MASTER comment "SPI Master Controller Drivers"
Quoted 8 lines
drivers/spi/Kconfig if SPI_MASTER · line 61
Quoted 6 lines
drivers/spi/Kconfig +6
-0+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.
Quoted 1,321 lines
drivers/spi/Kconfig if SPI_MASTER · line 72
Quoted 12 lines
drivers/spi/Kconfig +11
-0+ 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.
drivers/spi/Kconfig if SPI_MASTER · line 80
Quoted 8 lines
drivers/spi/Kconfig +8
-0+ +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_SPIabovePCH_SPI_PLATFORM_DEVICEand makePCH_SPI_PLATFORM_DEVICEdepend onPCH_SPI. (In fact, sincePCH_SPI_PLATFORM_DEVICEis always yes whenPCH_SPIis selected, then you probably don't need two configuration signals).Quoted 9 lines
drivers/spi/Kconfig +1
-0+ config SPI_ATMEL tristate "Atmel SPI Controller" depends on (ARCH_AT91 || AVR32)drivers/spi/Makefile +0
-0drivers/spi/Makefile obj-$(CONFIG_SPI_TLE62X0) += tle62x0.o · line 64
Quoted 6 lines
drivers/spi/Makefile +3
-0@@ -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.oNo 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?
drivers/spi/pch_spi.h line 3
Quoted 10 lines
drivers/spi/Makefile +1
-0+drivers/spi/pch_spi.h new file +3
-0@@ -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.
[...]
drivers/spi/pch_spi.h line 103
Quoted 2 lines
drivers/spi/pch_spi.h new file +2
-0+#define PCH_SET_BITMSK (var, bitmask) ((var) |= (bitmask)) +#define PCH_CLR_BITMSK (var, bitmask) ((var) &= (~(bitmask)))Because the macro has side-effects, I'd prefer to see static inlines used here with an all lowercase name.
Quoted 106 lines
drivers/spi/pch_spi.h new file +74
-0+ +/** +* 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 drivers/spi/pch_spi_main.c new file +15
-0@@ -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.
+ */drivers/spi/pch_spi_main.c new file +9
-0+ +#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> drivers/spi/pch_spi_main.c line 35
Quoted 10 lines
drivers/spi/pch_spi_main.c new file +10
-0+#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_datastructure. Getting rid of the static symbols will make it easier if the driver ever needs to support multiple instances of the device.Quoted 7 lines
drivers/spi/pch_spi_main.c new file +6
-0+/** +* 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, u8interrupt)
drivers/spi/pch_spi_main.c line 100
Quoted 9 lines
drivers/spi/pch_spi_main.c new file +9
-0+{ +u32 reg_val_spcr; + + reg_val_spcr =pch_spi_readreg (master,PCH_SPCR ); + +dev_dbg (&master->dev,"%s SPCR content =%x\n" , + __func__, reg_val_spcr); + +switch (interrupt &0x1f ) {Is it possible for more than one of these interrupt bits to be set at the same time? If so, then this
switch()statement won't always work.Quoted 67 lines
+ case
PCH_RFI:
+ /*clear RFIE bit in SPCR */
+ dev_dbg(&master->dev,drivers/spi/pch_spi_main.c new file +64
-0+ "%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;drivers/spi/pch_spi_main.c line 205
Quoted 10 lines
drivers/spi/pch_spi_main.c new file +10
-0+ +/*register addresses */ +void __iomem *spsr; +void __iomem *spdrr; +void __iomem *spdwr; + +/*remapped pci base address */ +void __iomem *io_remap_addr; + +irqreturn_t tRetVal =IRQ_NONE ;nit: use lowercase for variable names.
Quoted 110 lines
drivers/spi/pch_spi_main.c new file +110
-0+ +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,drivers/spi/pch_spi_main.c line 325
Quoted 10 lines
drivers/spi/pch_spi_main.c new file +10
-0+"%s invoking callback\n" , + __func__); + (*pch_spi_gcbptr) (p_data); + } + } + + tRetVal =IRQ_HANDLED ; + } + } + }Wow, the nesting on this function is very deep, and hard to follow. Candidate for refactoring?
Quoted 6 lines
drivers/spi/pch_spi_main.c new file +6
-0+ +dev_dbg (&p_board_dat->pdev->dev,"%s EXIT return value=%d\n" , + __func__, tRetVal); + +return tRetVal; +}drivers/spi/pch_spi_main.c line 341
Quoted 10 lines
drivers/spi/pch_spi_main.c new file +10
-0+ +/** +* pch_spi_entcb() - Registers the callback function +* @pch_spi_cb: Callback function pointer. +*/ +static void pch_spi_entcb (void (*pch_spi_cb) (struct pch_spi_data *)) +{ +if (pch_spi_cb !=NULL ) + pch_spi_gcbptr = pch_spi_cb; +}This trivial function is used exactly once in the probe routine. Just set the variable in-place. In fact, this doesn't need to be a callback at all because it is *always* set to
pch_spi_callback. Removing it will make the driver simpler.Quoted 26 lines
drivers/spi/pch_spi_main.c new file +26
-0+/** +* 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 */ drivers/spi/pch_spi_main.c line 456
Quoted 10 lines
drivers/spi/pch_spi_main.c new file +10
-0+ +if ((spi->mode &SPI_LSB_FIRST ) !=0 ) { +/*LSB first */ +PCH_CLR_BITMSK (reg_spcr_val,SPCR_LSBF_BIT ); +dev_dbg (&spi->dev,"%s :setting LSBF bit to 0\n" , __func__); + }else { +/*MSB first */ +PCH_SET_BITMSK (reg_spcr_val,SPCR_LSBF_BIT ); +dev_dbg (&spi->dev,"%s :setting LSBF bit to 1\n" , __func__); + }There are a lot of these if/else blocks which call PCH_{SET,CLR}_BITMSK(). The code could be simplified with a helper function that implements the two paths. Something like
pch_spi_clrsetbits()perhaps.Quoted 59 lines
+static int
pch_spi_transfer(struct spi_device *pspi, struct spi_message *pmsg)drivers/spi/pch_spi_main.c new file +57
-0+{ + +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__);drivers/spi/pch_spi_main.c line 689
Quoted 10 lines
drivers/spi/pch_spi_main.c new file +10
-0+ +/*if baud rate hs been specified validate the same */ +if (p_transfer->speed_hz) { +if ((p_transfer->speed_hz) > +PCH_MAX_BAUDRATE ) { + retval = -EINVAL ; +dev_err (&pspi->dev, +"%s Invalid Baud rate\n" , __func__); + goto err_return_mutex; + }If the requested speed is too fast, then the maximum bus speed should be used instead.
Quoted 8 lines
+ /*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] =drivers/spi/pch_spi_main.c new file +3
-0+ (((u8 *) (p_data->p_cur_trans-> + tx_buf))[j]); + dev_dbg(&p_data->p_master->dev, "%s=%x\n",drivers/spi/pch_spi_main.c line 881
Quoted 10 lines
drivers/spi/pch_spi_main.c new file +10
-0+ __func__, (p_data->pkt_tx_buff[j])); + }else { + p_data->pkt_tx_buff[j] = + ((u16 *) (p_data->p_cur_trans-> + tx_buf))[j]; +dev_dbg (&p_data->p_master->dev, +"%s xmt data = %x\n" , __func__, + (p_data->pkt_tx_buff[j])); + } + }Optimization note; by coding it this way, the (
PCH_8_BPW== *bpw) test is being performed on every iteration through the loop. If you had separate iterators for 8 and 16 bit transfer, the code will be faster.Quoted 4 lines
+ }
+
+ /*if len greater thanPCH_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.
drivers/spi/pch_spi_main.c line 892
Quoted 7 lines
+ if ((
p_data->bpw_len) >PCH_MAX_FIFO_DEPTH)
+n_writes=PCH_MAX_FIFO_DEPTH;
+ else
+n_writes= (p_data->bpw_len);drivers/spi/pch_spi_main.c new file +3
-0+ +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.
drivers/spi/pch_spi_main.c line 1029
Quoted 5 lines
drivers/spi/pch_spi_main.c new file +5
-0+static void pch_spi_copy_rx_data (struct pch_spi_data *p_data,int bpw) +{ +int j; +/*copy Rx Data */ +if ((p_data->p_cur_trans->rx_buf) !=NULL ) {if (
p_data->pcur_trans->rx_buf)
return;That reduces the indentation on the rest of the function by one level and makes it easier to read and understand.
Quoted 7 lines
+ for (j = 0; j < (
p_data->bpw_len); j++) {
+ if (PCH_8_BPW== bpw) {
+ ((u8 *) (p_data->p_cur_trans->rx_buf))[j] =drivers/spi/pch_spi_main.c new file +4
-0+ (u8) ((p_data->pkt_rx_buff[j]) &0xFF ); + + dev_dbg(&p_data->p_master->dev, + "rcv data in %s =%x\n", __func__,drivers/spi/pch_spi_main.c line 1046
Quoted 10 lines
drivers/spi/pch_spi_main.c new file +10
-0+ (p_data->pkt_rx_buff[j])); + + }else { + ((u16 *) (p_data->p_cur_trans->rx_buf))[j] = + (u16) (p_data->pkt_rx_buff[j]); +dev_dbg (&p_data->p_master->dev, +"rcv data in %s = %x\n" , __func__, + (p_data->pkt_rx_buff[j])); + } + }Same comment on optimization.
Quoted 16 lines
+ }
drivers/spi/pch_spi_main.c new file +15
-0+} + + +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); +drivers/spi/pch_spi_main.c line 1072
Quoted 10 lines
drivers/spi/pch_spi_main.c new file +10
-0+/*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.
drivers/spi/pch_spi_main.c line 1078
Quoted 6 lines
drivers/spi/pch_spi_main.c new file +6
-0+ +/*delete from queue */ +list_del_init (&pmsg->queue); + } + +spin_unlock (&p_data->lock);Put a return statement here. It will eliminate the else { } indentation below; again improving readability.
Quoted 111 lines
+ } else {
+p_data->bcurrent_msg_processing= true;
+ dev_dbg(&p_data->p_master->dev,drivers/spi/pch_spi_main.c new file +107
-0+"%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)
drivers/spi/pch_spi_main.c line 1418
Quoted 6 lines
drivers/spi/pch_spi_main.c new file +6
-0+{ + +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.drivers/spi/pch_spi_main.c line 1427
Quoted 9 lines
+ int
spi_alloc_num,spi_master_num;drivers/spi/pch_spi_main.c new file +8
-0+ +dev_dbg (&pdev->dev,"%s ENTRY\n" , __func__); +/* initialize the call back function */ +pch_spi_entcb (pch_spi_callback); +dev_dbg (&pdev->dev,"%s invoked pch_spi_entcb\n" , __func__); + +/* allocate memory for private data */ + p_board_dat =kmalloc (sizeof (struct pch_spi_board_data ),GFP_KERNEL );
kzalloc()Quoted 33 lines
drivers/spi/pch_spi_main.c new file +33
-0+ +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);drivers/spi/pch_spi_main.c line 1470
Quoted 10 lines
drivers/spi/pch_spi_main.c new file +10
-0+ 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_numisn't used except for the error path, and it always has the same value as 'j'. Get it out of the loop initializer! :-) It can be set in the error path.Quoted 1 line
+ p_master[j]->bus_num = 0;
You probably want -1 here so that the spi layer dynamically assigns an spi bus number.
Quoted 7 lines
+ 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,drivers/spi/pch_spi_main.c line 1485
Quoted 7 lines
drivers/spi/pch_spi_main.c new file +7
-0+ "%s transfer member of SPI master initialized\n", __func__); + p_master[j]->cleanup = pch_spi_cleanup; +dev_dbg (&pdev->dev, +"%s cleanup member of SPI master initialized\n" , __func__); + + p_board_dat->p_data[j] = +spi_master_get_devdata (p_master[j]);Unnecessary line break. There are a lot of these in this driver.
Quoted 10 lines
+
+ p_board_dat->p_data[j]->p_master = p_master[j];
+ p_board_dat->p_data[j]->n_curnt_chip = 255;
+ p_board_dat->p_data[j]->p_current_chip = NULL;
+ p_board_dat->p_data[j]->b_transfer_complete = false;
+ p_board_dat->p_data[j]->pkt_tx_buff = NULL;
+ p_board_dat->p_data[j]->pkt_rx_buff = NULL;
+ p_board_dat->p_data[j]->tx_index = 0;
+ p_board_dat->p_data[j]->rx_index = 0;
+ p_board_dat->p_data[j]->b_transfer_active = false;The above 7 lines can be dropped when the code is changed to use
kzalloc().kzalloc()clears the allocated memory.Quoted 18 lines
+ p_board_dat->p_data[j]->p_board_dat =
p_board_dat;
+p_board_dat->suspend_sts= false;drivers/spi/pch_spi_main.c new file +16
-0+ +/* 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 ) {drivers/spi/pch_spi_main.c line 1523
Quoted 10 lines
drivers/spi/pch_spi_main.c new file +10
-0+dev_err (&pdev->dev,"%s fail(retval=%d)\n" , + __func__, retval); + goto err_spi_get_resources; + } + +dev_dbg (&pdev->dev,"%s pch_spi_get_resources returned=%d\n" , + __func__, retval); + +/* save private data in dev */ +pci_set_drvdata (pdev, (void *)p_board_dat);Remove the (void *) cast. It should not be necessary.
Quoted 24 lines
+
dev_dbg(&pdev->dev, "%s invoked pci_set_drvdata\n", __func__);drivers/spi/pch_spi_main.c new file +23
-0+ +/* 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; +}drivers/spi/pch_spi_main.c line 1557
Quoted 10 lines
drivers/spi/pch_spi_main.c new file +10
-0+ +static void pch_spi_remove (struct pci_dev *pdev) +{ +int i; + +struct pch_spi_board_data *p_board_dat =pci_get_drvdata (pdev); + +dev_dbg (&pdev->dev,"%s ENTRY\n" , __func__); + +if (p_board_dat !=NULL ) {if (!p_board_dat) {
dev_err(...);
return;
}Quoted 53 lines
+
dev_dbg(&pdev->dev, "%s invoked pci_get_drvdata\n", __func__);drivers/spi/pch_spi_main.c new file +52
-0+ +/* 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;drivers/spi/pch_spi_main.c line 1620
Quoted 10 lines
drivers/spi/pch_spi_main.c new file +10
-0+s32 retval; + +struct pch_spi_board_data *p_board_dat =pci_get_drvdata (pdev); + +dev_dbg (&pdev->dev,"%s ENTRY\n" , __func__); + +if (p_board_dat ==NULL ) { +dev_err (&pdev->dev, +"%s pci_get_drvdata returned NULL\n" , __func__); + retval = -EFAULT ;Ditto here; return
-EFAULT;Quoted 157 lines
+ } else {
+ retval = 0;
+ dev_dbg(&pdev->dev,
+ "%spci_get_drvdatainvoked successfully\n",__func__);
+p_board_dat->suspend_sts= true;
+ dev_dbg(&pdev->dev,
+ "%sp_board_dat->bSuspendingset to true\n",__func__);drivers/spi/pch_spi_main.c new file +150
-0+ +/* 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__); + }drivers/spi/pch_spi_main.c line 1787
Quoted 10 lines
drivers/spi/pch_spi_main.c new file +10
-0+ +/* set suspend status to false */ + p_board_dat->suspend_sts = false; + +dev_dbg (&pdev->dev,"%s set p_board_dat" +"->bSuspending = false\n" , __func__); + } + } + } + }Another very deeply indented function.
Quoted 76 lines
drivers/spi/pch_spi_main.c new file +36
-0+ +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); +drivers/spi/pch_spi_platform_devices.c new file +15
-0@@ -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.
+ */drivers/spi/pch_spi_platform_devices.c new file +16
-0+ +#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)drivers/spi/pch_spi_platform_devices.c line 42
Quoted 10 lines
drivers/spi/pch_spi_platform_devices.c new file +10
-0+ { + .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 arespi_devices.drivers/spi/pch_spi_platform_devices.c line 46
Quoted 4 lines
drivers/spi/pch_spi_platform_devices.c new file +4
-0+ +static __init int Load (void ) +{ +int iRetVal =-1 ;int rc = -1;
drivers/spi/pch_spi_platform_devices.c line 55
Quoted 9 lines
drivers/spi/pch_spi_platform_devices.c new file +9
-0+ +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.
Signature · 2 lines
Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd.
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 <[email protected]>:
Quoted 5 lines
2010/8/10 Masayuki Ohtak <[email protected]>:
Quoted 4 lines
(...)
+structpch_spi_data{
+ void__iomem*io_remap_addr;
+ structspi_master*p_master;
p_* is for "pointer" is it not? Hungarian notation is not liked in the kernel, see Documentation/CodingStyle.
Quoted 2 lines
Quoted 2 lines
+ struct
work_structwork;
+ structworkqueue_struct*p_wk_q;
Dito.
Quoted 8 lines
Quoted 8 lines
+
wait_queue_head_twait;
+ u8b_transfer_complete;
+ u8bcurrent_msg_processing;
+spinlock_tlock;
+ structlist_headqueue;
+ u8 status;
+ u32bpw_len;
+ s8b_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.
Quoted 9 lines
Quoted 9 lines
+ u32
tx_index;
+ u32rx_index;
+ u16*pkt_tx_buff;
+ u16*pkt_rx_buff;
+ u8n_curnt_chip;
+ structspi_device*p_current_chip;
+ structspi_message*p_current_msg;
+ structspi_transfer*p_cur_trans;
+ structpch_spi_board_data*p_board_dat;
The hungarian p_* prefix issue again.
Quoted 4 lines
Quoted 4 lines
drivers/spi/pch_spi.h new file +4
-0+}; + +/** +* struct pch_spi_board_data - Holds the SPI device specific details
Quoted 8 lines
Quoted 8 lines
+ * @pdev: Pointer to the PCI device
+ * @irq_reg_sts: Status of IRQ registration
+ * @pci_req_sts: Status ofpci_request_regions
+ * @suspend_sts: Status of suspend
+ * @p_data: Pointer to SPI channel data structure
+ */
+structpch_spi_board_data{
+ structpci_dev*pdev;
Hungarian p*?
Quoted 4 lines
Quoted 4 lines
+ u8
irq_reg_sts;
+ u8pci_req_sts;
+ u8suspend_sts;
+ structpch_spi_data*p_data[PCH_MAX_DEV];
Hungarian p_*?
Quoted 11 lines
Quoted 11 lines
(...)
drivers/spi/pch_spi_main.c new file +10
-0+/** +* 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; +
Quoted 8 lines
drivers/spi/pch_spi_main.c line 192
Quoted 8 lines
+ /*SPSR content */
+ u32reg_spsr_val,reg_spcr_val;drivers/spi/pch_spi_main.c new file +6
-0+ + /*book keeping variables */ + u32 n_read, tx_index, rx_index, bpw_len; + + /*to hold channel data */ + struct pch_spi_data *p_data;
Hungarian p_*
Quoted 5 lines
Quoted 5 lines
drivers/spi/pch_spi_main.c new file +5
-0+ + /*buffer to store rx/tx data */ + u16 *pkt_rx_buffer, *pkt_tx_buff; + + /*register addresses */
Quoted 8 lines
drivers/spi/pch_spi_main.c line 205
Quoted 8 lines
+ void
__iomem*spsr;
+ void__iomem*spdrr;
+ void__iomem*spdwr;drivers/spi/pch_spi_main.c new file +5
-0+ + /*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"?
Quoted 11 lines
nit: use lowercase for variable names.
Quoted 9 lines
drivers/spi/pch_spi_main.c new file +9
-0+ + 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++) {
Quoted 8 lines
drivers/spi/pch_spi_main.c line 222
Quoted 8 lines
+
p_data= p_board_dat->p_data[dev];
+io_remap_addr=p_data->io_remap_addr;
+ spsr =io_remap_addr+PCH_SPSR;drivers/spi/pch_spi_main.c new file +5
-0+ + reg_spsr_val = ioread32(spsr); + + /*Check if the interrupt is for SPI device */ +
From this point...
Quoted 91 lines
Quoted 91 lines
+ 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);drivers/spi/pch_spi_main.c new file +86
-0+ + 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 */ +
Quoted 8 lines
Quoted 8 lines
+ 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.
Quoted 5 lines
drivers/spi/pch_spi_main.c line 1415
Quoted 5 lines
+static int
pch_spi_probe(struct pci_dev *pdev, const struct pci_device_id *id)drivers/spi/pch_spi_main.c new file +3
-0+{ + + 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.
Quoted 2 lines
Quoted 2 lines
+
+ structpch_spi_board_data*p_board_dat;
Hungarian p_*
Quoted 5 lines
drivers/spi/pch_spi_main.c line 1711
Quoted 5 lines
(...)
drivers/spi/pch_spi_main.c new file +4
-0+static int pch_spi_resume (struct pci_dev *pdev) +{ + int i; + s32 retval;
s32?? What's wrong with an int?
Quoted 3 lines
Quoted 3 lines
+
+ structpch_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...
Quoted 4 lines
Quoted 4 lines
+ if (
p_board_dat== NULL) {
+ dev_err(&pdev->dev,
+ "%spci_get_drvdatareturned NULL\n",__func__);
+ retval =-EFAULT;
return -EFAULT and de-indent, etc. You know the drill.
Quoted 60 lines
Quoted 60 lines
+ } else {
+ retval = 0;
+ /* move device to DO power state */
+pci_set_power_state(pdev, PCI_D0);
+ dev_dbg(&pdev->dev,
+ "%spci_set_power_stateinvoked successfully\n",__func__);drivers/spi/pch_spi_main.c new file +54
-0+ + /* 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 */
Quoted 8 lines
Quoted 8 lines
+
p_board_dat->suspend_sts= false; +
+ dev_dbg(&pdev->dev, "%s setp_board_dat"
+ "->bSuspending = false\n",__func__);
+ }
+ }
+ }
+ }
All the way to here.
Quoted 29 lines
Quoted 29 lines
(...)
drivers/spi/pch_spi_platform_devices.c new file +15
-0@@ -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.
+ */drivers/spi/pch_spi_platform_devices.c new file +8
-0+ +#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[] = { + {
Quoted 8 lines
Quoted 8 lines
+ .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.
Quoted 4 lines
Quoted 4 lines
+ {
+ .modalias = "spidev",
+ .max_speed_hz = 1000000,
+ .bus_num = 1,
Quoted 8 lines
drivers/spi/pch_spi_platform_devices.c line 44
Quoted 8 lines
+ .chip_select = 1,
+ .platform_data = NULL,
+ .mode =SPI_MODE_0,
+ },drivers/spi/pch_spi_platform_devices.c new file +4
-0+#endif +}; + +static __init int Load (void )
Rename from "Load" to pch_init or *_init whatever like everyone else.
Quoted 2 lines
Quoted 2 lines
+{
+ int iRetVal = -1;
Hungarian notation.
Quoted 9 lines
drivers/spi/pch_spi_platform_devices.c line 55
Quoted 9 lines
drivers/spi/pch_spi_platform_devices.c new file +9
-0+ + if (!spi_register_board_info + (pch_spi_slaves, ARRAY_SIZE(pch_spi_slaves))) + iRetVal = 0; + + return iRetVal; +} + +module_init (Load);
Yours,
Linus Walleij
Hi Linus Walleij,
We will update your comments ASAP.
Thanks, Ohtake(OKISEMI)
----- Original Message -----
From: "Linus Walleij" <[email protected]>
To: "Masayuki Ohtak" <[email protected]>
Cc: <[email protected]>; "LKML" <[email protected]>; "David Brownell" <[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>; "Grant Likely" <[email protected]>
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 <[email protected]>:
Quoted 5 lines
2010/8/10 Masayuki Ohtak <[email protected]>:
Quoted 4 lines
(...)
+structpch_spi_data{
+ void__iomem*io_remap_addr;
+ structspi_master*p_master;
p_* is for "pointer" is it not? Hungarian notation is not liked in the kernel, see Documentation/CodingStyle.
Quoted 2 lines
Quoted 2 lines
+ struct
work_structwork;
+ structworkqueue_struct*p_wk_q;
Dito.
Quoted 8 lines
Quoted 8 lines
+
wait_queue_head_twait;
+ u8b_transfer_complete;
+ u8bcurrent_msg_processing;
+spinlock_tlock;
+ structlist_headqueue;
+ u8 status;
+ u32bpw_len;
+ s8b_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.
Quoted 9 lines
Quoted 9 lines
+ u32
tx_index;
+ u32rx_index;
+ u16*pkt_tx_buff;
+ u16*pkt_rx_buff;
+ u8n_curnt_chip;
+ structspi_device*p_current_chip;
+ structspi_message*p_current_msg;
+ structspi_transfer*p_cur_trans;
+ structpch_spi_board_data*p_board_dat;
The hungarian p_* prefix issue again.
Quoted 4 lines
Quoted 4 lines
drivers/spi/pch_spi.h new file +4
-0+}; + +/** +* struct pch_spi_board_data - Holds the SPI device specific details
Quoted 8 lines
Quoted 8 lines
+ * @pdev: Pointer to the PCI device
+ * @irq_reg_sts: Status of IRQ registration
+ * @pci_req_sts: Status ofpci_request_regions
+ * @suspend_sts: Status of suspend
+ * @p_data: Pointer to SPI channel data structure
+ */
+structpch_spi_board_data{
+ structpci_dev*pdev;
Hungarian p*?
Quoted 4 lines
Quoted 4 lines
+ u8
irq_reg_sts;
+ u8pci_req_sts;
+ u8suspend_sts;
+ structpch_spi_data*p_data[PCH_MAX_DEV];
Hungarian p_*?
Quoted 11 lines
Quoted 11 lines
(...)
drivers/spi/pch_spi_main.c new file +10
-0+/** +* 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; +
Quoted 8 lines
drivers/spi/pch_spi_main.c line 192
Quoted 8 lines
+ /*SPSR content */
+ u32reg_spsr_val,reg_spcr_val;drivers/spi/pch_spi_main.c new file +6
-0+ +/*book keeping variables */ +u32 n_read, tx_index, rx_index, bpw_len; + +/*to hold channel data */ +struct pch_spi_data *p_data;
Hungarian p_*
Quoted 5 lines
Quoted 5 lines
drivers/spi/pch_spi_main.c new file +5
-0+ +/*buffer to store rx/tx data */ +u16 *pkt_rx_buffer, *pkt_tx_buff; + +/*register addresses */
Quoted 8 lines
drivers/spi/pch_spi_main.c line 205
Quoted 8 lines
+ void
__iomem*spsr;
+ void__iomem*spdrr;
+ void__iomem*spdwr;drivers/spi/pch_spi_main.c new file +5
-0+ +/*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"?
Quoted 113 lines
nit: use lowercase for variable names.
Quoted 7 lines
drivers/spi/pch_spi_main.c new file +7
-0+ +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__);drivers/spi/pch_spi_main.c line 222
Quoted 10 lines
drivers/spi/pch_spi_main.c new file +10
-0+ }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...
Quoted 91 lines
+ 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);drivers/spi/pch_spi_main.c new file +86
-0+ +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 */ +
Quoted 8 lines
Quoted 8 lines
+ 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.
Quoted 5 lines
drivers/spi/pch_spi_main.c line 1415
Quoted 5 lines
+static int
pch_spi_probe(struct pci_dev *pdev, const struct pci_device_id *id)drivers/spi/pch_spi_main.c new file +3
-0+{ + +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.
Quoted 2 lines
Quoted 2 lines
+
+ structpch_spi_board_data*p_board_dat;
Hungarian p_*
Quoted 5 lines
drivers/spi/pch_spi_main.c line 1711
Quoted 5 lines
(...)
drivers/spi/pch_spi_main.c new file +4
-0+static int pch_spi_resume (struct pci_dev *pdev) +{ +int i; +s32 retval;
s32?? What's wrong with an int?
Quoted 3 lines
Quoted 3 lines
+
+ structpch_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...
Quoted 4 lines
Quoted 4 lines
+ if (
p_board_dat== NULL) {
+ dev_err(&pdev->dev,
+ "%spci_get_drvdatareturned NULL\n",__func__);
+ retval =-EFAULT;
return -EFAULT and de-indent, etc. You know the drill.
Quoted 60 lines
Quoted 60 lines
+ } else {
+ retval = 0;
+ /* move device to DO power state */
+pci_set_power_state(pdev, PCI_D0);
+ dev_dbg(&pdev->dev,
+ "%spci_set_power_stateinvoked successfully\n",__func__);drivers/spi/pch_spi_main.c new file +54
-0+ +/* 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 */
Quoted 8 lines
Quoted 8 lines
+
p_board_dat->suspend_sts= false;
+
+ dev_dbg(&pdev->dev, "%s setp_board_dat"
+ "->bSuspending = false\n",__func__);
+ }
+ }
+ }
+ }
All the way to here.
Quoted 29 lines
Quoted 29 lines
(...)
drivers/spi/pch_spi_platform_devices.c new file +15
-0@@ -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.
+ */drivers/spi/pch_spi_platform_devices.c new file +8
-0+ +#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[] = { + {
Quoted 8 lines
Quoted 8 lines
+ .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.
Quoted 4 lines
Quoted 4 lines
+ {
+ .modalias = "spidev",
+ .max_speed_hz = 1000000,
+ .bus_num = 1,
Quoted 8 lines
drivers/spi/pch_spi_platform_devices.c line 44
Quoted 8 lines
+ .chip_select = 1,
+ .platform_data = NULL,
+ .mode =SPI_MODE_0,
+ },drivers/spi/pch_spi_platform_devices.c new file +4
-0+#endif +}; + +static __init int Load (void )
Rename from "Load" to pch_init or *_init whatever like everyone else.
Quoted 2 lines
Quoted 2 lines
+{
+ int iRetVal = -1;
Hungarian notation.
Quoted 9 lines
drivers/spi/pch_spi_platform_devices.c line 55
Quoted 9 lines
drivers/spi/pch_spi_platform_devices.c new file +9
-0+ +if (!spi_register_board_info + (pch_spi_slaves,ARRAY_SIZE (pch_spi_slaves))) + iRetVal =0 ; + +return iRetVal; +} + +module_init (Load);
Yours,
Linus Walleij
----- Original Message -----
From: "Grant Likely" <[email protected]>
To: "Masayuki Ohtak" <[email protected]>
Cc: <[email protected]>; "LKML" <[email protected]>; "David Brownell" <[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>
Sent: Saturday, August 14, 2010 3:49 PM
Subject: Re: [PATCH] Topcliff: Update PCH_SPI driver to 2.6.35
Quoted 47 lines
2010/8/10 Masayuki Ohtak <[email protected]>:
Quoted 9 lines
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 <[email protected]>Hi Masayuki. Thanks for the patch. The driver is mostly structured well and looks fairly good. However, there are quite a few stylistic issues that should be easy to clean up, and a few technical concerns. Comments below.
BTW, please make sure patches get sent out as plain-text, not html formatted.
Quoted 17 lines
--- 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.cdrivers/spi/Kconfig +0
-0drivers/spi/Kconfig if SPI_MASTER · line 61
Quoted 9 lines
drivers/spi/Kconfig +6
-0@@ -53,6 +53,32 @@ if SPI_MASTER comment "SPI Master Controller Drivers" +config PCH_SPI_PLATFORM_DEVICE + boolean + default y if PCH_SPI + help + This driver is for PCH SPI of Topcliff which is an IOH for x86 + embedded processor.What is IOH and abbreviation for?
Quoted 15 lines
drivers/spi/Kconfig if SPI_MASTER · line 72
Quoted 11 lines
drivers/spi/Kconfig +11
-0+ 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 ?
Quoted 37 lines
drivers/spi/Kconfig if SPI_MASTER · line 80
Quoted 9 lines
drivers/spi/Kconfig +8
-0+ +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_SPIabovePCH_SPI_PLATFORM_DEVICEand makePCH_SPI_PLATFORM_DEVICEdepend onPCH_SPI. (In fact, sincePCH_SPI_PLATFORM_DEVICEis always yes whenPCH_SPIis selected, then you probably don't need two configuration signals).Quoted 9 lines
drivers/spi/Kconfig +1
-0+ config SPI_ATMEL tristate "Atmel SPI Controller" depends on (ARCH_AT91 || AVR32)drivers/spi/Makefile +0
-0drivers/spi/Makefile obj-$(CONFIG_SPI_TLE62X0) += tle62x0.o · line 64
Quoted 6 lines
drivers/spi/Makefile +3
-0@@ -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.oNo 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_").
Quoted 2 lines
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.
Quoted 15 lines
drivers/spi/pch_spi.h line 3
Quoted 11 lines
drivers/spi/Makefile +1
-0+drivers/spi/pch_spi.h new file +3
-0@@ -0,0 +1,177 @@ +/* +* Copyright (C) 2010 OKI SEMICONDUCTOR Co., LTD. +* It is helpful to have a one line description of what this file is for in at the top of the header comment block.
Quoted 6 lines
[...]
drivers/spi/pch_spi.h line 103
Quoted 2 lines
drivers/spi/pch_spi.h new file +2
-0+#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.
Quoted 115 lines
Quoted 115 lines
drivers/spi/pch_spi.h new file +74
-0+ +/** +* 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 drivers/spi/pch_spi_main.c new file +15
-0@@ -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.
+ */drivers/spi/pch_spi_main.c new file +17
-0+ +#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 ,} +}; +
Quoted 8 lines
drivers/spi/pch_spi_main.c line 35
Quoted 2 lines
drivers/spi/pch_spi_main.c new file +2
-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_datastructure. 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.
Quoted 12 lines
Quoted 12 lines
drivers/spi/pch_spi_main.c new file +6
-0+/** +* 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, u8interrupt)
drivers/spi/pch_spi_main.c new file +4
-0+{ +u32 reg_val_spcr; + + reg_val_spcr =pch_spi_readreg (master,PCH_SPCR );
Quoted 8 lines
drivers/spi/pch_spi_main.c line 100
Quoted 5 lines
drivers/spi/pch_spi_main.c new file +5
-0+ +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.
Quoted 218 lines
Quoted 68 lines
+ case
PCH_RFI:
+ /*clear RFIE bit in SPCR */
+ dev_dbg(&master->dev,drivers/spi/pch_spi_main.c new file +64
-0+ "%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;drivers/spi/pch_spi_main.c line 205
Quoted 10 lines
drivers/spi/pch_spi_main.c new file +10
-0+ +/*register addresses */ +void __iomem *spsr; +void __iomem *spdrr; +void __iomem *spdwr; + +/*remapped pci base address */ +void __iomem *io_remap_addr; + +irqreturn_t tRetVal =IRQ_NONE ;nit: use lowercase for variable names.
Quoted 110 lines
drivers/spi/pch_spi_main.c new file +110
-0+ +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,drivers/spi/pch_spi_main.c line 325
Quoted 10 lines
drivers/spi/pch_spi_main.c new file +10
-0+"%s invoking callback\n" , + __func__); + (*pch_spi_gcbptr) (p_data); + } + } + + tRetVal =IRQ_HANDLED ; + } + } + }Wow, the nesting on this function is very deep, and hard to follow. Candidate for refactoring?
Quoted 13 lines
drivers/spi/pch_spi_main.c new file +13
-0+ +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 *)) +{
Quoted 8 lines
Quoted 3 lines
+ 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.
Quoted 412 lines
Quoted 27 lines
drivers/spi/pch_spi_main.c new file +26
-0+/** +* 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 */ drivers/spi/pch_spi_main.c line 456
Quoted 10 lines
drivers/spi/pch_spi_main.c new file +10
-0+ +if ((spi->mode &SPI_LSB_FIRST ) !=0 ) { +/*LSB first */ +PCH_CLR_BITMSK (reg_spcr_val,SPCR_LSBF_BIT ); +dev_dbg (&spi->dev,"%s :setting LSBF bit to 0\n" , __func__); + }else { +/*MSB first */ +PCH_SET_BITMSK (reg_spcr_val,SPCR_LSBF_BIT ); +dev_dbg (&spi->dev,"%s :setting LSBF bit to 1\n" , __func__); + }There are a lot of these if/else blocks which call PCH_{SET,CLR}_BITMSK(). The code could be simplified with a helper function that implements the two paths. Something like
pch_spi_clrsetbits()perhaps.Quoted 59 lines
+static int
pch_spi_transfer(struct spi_device *pspi, struct spi_message *pmsg)drivers/spi/pch_spi_main.c new file +57
-0+{ + +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__);drivers/spi/pch_spi_main.c line 689
Quoted 10 lines
drivers/spi/pch_spi_main.c new file +10
-0+ +/*if baud rate hs been specified validate the same */ +if (p_transfer->speed_hz) { +if ((p_transfer->speed_hz) > +PCH_MAX_BAUDRATE ) { + retval = -EINVAL ; +dev_err (&pspi->dev, +"%s Invalid Baud rate\n" , __func__); + goto err_return_mutex; + }If the requested speed is too fast, then the maximum bus speed should be used instead.
Quoted 8 lines
+ /*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] =drivers/spi/pch_spi_main.c new file +3
-0+ (((u8 *) (p_data->p_cur_trans-> + tx_buf))[j]); + dev_dbg(&p_data->p_master->dev, "%s=%x\n",drivers/spi/pch_spi_main.c line 881
Quoted 10 lines
drivers/spi/pch_spi_main.c new file +10
-0+ __func__, (p_data->pkt_tx_buff[j])); + }else { + p_data->pkt_tx_buff[j] = + ((u16 *) (p_data->p_cur_trans-> + tx_buf))[j]; +dev_dbg (&p_data->p_master->dev, +"%s xmt data = %x\n" , __func__, + (p_data->pkt_tx_buff[j])); + } + }Optimization note; by coding it this way, the (
PCH_8_BPW== *bpw) test is being performed on every iteration through the loop. If you had separate iterators for 8 and 16 bit transfer, the code will be faster.Quoted 4 lines
+ }
+
+ /*if len greater thanPCH_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.
drivers/spi/pch_spi_main.c line 892
Quoted 7 lines
+ if ((
p_data->bpw_len) >PCH_MAX_FIFO_DEPTH)
+n_writes=PCH_MAX_FIFO_DEPTH;
+ else
+n_writes= (p_data->bpw_len);drivers/spi/pch_spi_main.c new file +3
-0+ +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.
drivers/spi/pch_spi_main.c line 1029
Quoted 5 lines
drivers/spi/pch_spi_main.c new file +5
-0+static void pch_spi_copy_rx_data (struct pch_spi_data *p_data,int bpw) +{ +int j; +/*copy Rx Data */ +if ((p_data->p_cur_trans->rx_buf) !=NULL ) {if (
p_data->pcur_trans->rx_buf)
return;That reduces the indentation on the rest of the function by one level and makes it easier to read and understand.
Quoted 7 lines
+ for (j = 0; j < (
p_data->bpw_len); j++) {
+ if (PCH_8_BPW== bpw) {
+ ((u8 *) (p_data->p_cur_trans->rx_buf))[j] =drivers/spi/pch_spi_main.c new file +4
-0+ (u8) ((p_data->pkt_rx_buff[j]) &0xFF ); + + dev_dbg(&p_data->p_master->dev, + "rcv data in %s =%x\n", __func__,drivers/spi/pch_spi_main.c line 1046
Quoted 10 lines
drivers/spi/pch_spi_main.c new file +10
-0+ (p_data->pkt_rx_buff[j])); + + }else { + ((u16 *) (p_data->p_cur_trans->rx_buf))[j] = + (u16) (p_data->pkt_rx_buff[j]); +dev_dbg (&p_data->p_master->dev, +"rcv data in %s = %x\n" , __func__, + (p_data->pkt_rx_buff[j])); + } + }Same comment on optimization.
Quoted 16 lines
+ }
drivers/spi/pch_spi_main.c new file +15
-0+} + + +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); +drivers/spi/pch_spi_main.c line 1072
Quoted 10 lines
drivers/spi/pch_spi_main.c new file +10
-0+/*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.
drivers/spi/pch_spi_main.c line 1078
Quoted 6 lines
drivers/spi/pch_spi_main.c new file +6
-0+ +/*delete from queue */ +list_del_init (&pmsg->queue); + } + +spin_unlock (&p_data->lock);Put a return statement here. It will eliminate the else { } indentation below; again improving readability.
Quoted 111 lines
+ } else {
+p_data->bcurrent_msg_processing= true;
+ dev_dbg(&p_data->p_master->dev,drivers/spi/pch_spi_main.c new file +107
-0+"%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)
drivers/spi/pch_spi_main.c line 1418
Quoted 6 lines
drivers/spi/pch_spi_main.c new file +6
-0+{ + +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.drivers/spi/pch_spi_main.c line 1427
Quoted 9 lines
+ int
spi_alloc_num,spi_master_num;drivers/spi/pch_spi_main.c new file +8
-0+ +dev_dbg (&pdev->dev,"%s ENTRY\n" , __func__); +/* initialize the call back function */ +pch_spi_entcb (pch_spi_callback); +dev_dbg (&pdev->dev,"%s invoked pch_spi_entcb\n" , __func__); + +/* allocate memory for private data */ + p_board_dat =kmalloc (sizeof (struct pch_spi_board_data ),GFP_KERNEL );
kzalloc()Quoted 33 lines
drivers/spi/pch_spi_main.c new file +33
-0+ +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);drivers/spi/pch_spi_main.c line 1470
Quoted 10 lines
drivers/spi/pch_spi_main.c new file +10
-0+ 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_numisn't used except for the error path, and it always has the same value as 'j'. Get it out of the loop initializer! :-) It can be set in the error path.
Quoted 4 lines
Quoted 1 line
+ 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;
Quoted 435 lines
Quoted 8 lines
+ 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,drivers/spi/pch_spi_main.c line 1485
Quoted 7 lines
drivers/spi/pch_spi_main.c new file +7
-0+ "%s transfer member of SPI master initialized\n", __func__); + p_master[j]->cleanup = pch_spi_cleanup; +dev_dbg (&pdev->dev, +"%s cleanup member of SPI master initialized\n" , __func__); + + p_board_dat->p_data[j] = +spi_master_get_devdata (p_master[j]);Unnecessary line break. There are a lot of these in this driver.
Quoted 10 lines
+
+ p_board_dat->p_data[j]->p_master = p_master[j];
+ p_board_dat->p_data[j]->n_curnt_chip = 255;
+ p_board_dat->p_data[j]->p_current_chip = NULL;
+ p_board_dat->p_data[j]->b_transfer_complete = false;
+ p_board_dat->p_data[j]->pkt_tx_buff = NULL;
+ p_board_dat->p_data[j]->pkt_rx_buff = NULL;
+ p_board_dat->p_data[j]->tx_index = 0;
+ p_board_dat->p_data[j]->rx_index = 0;
+ p_board_dat->p_data[j]->b_transfer_active = false;The above 7 lines can be dropped when the code is changed to use
kzalloc().kzalloc()clears the allocated memory.Quoted 18 lines
+ p_board_dat->p_data[j]->p_board_dat =
p_board_dat;
+p_board_dat->suspend_sts= false;drivers/spi/pch_spi_main.c new file +16
-0+ +/* 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 ) {drivers/spi/pch_spi_main.c line 1523
Quoted 10 lines
drivers/spi/pch_spi_main.c new file +10
-0+dev_err (&pdev->dev,"%s fail(retval=%d)\n" , + __func__, retval); + goto err_spi_get_resources; + } + +dev_dbg (&pdev->dev,"%s pch_spi_get_resources returned=%d\n" , + __func__, retval); + +/* save private data in dev */ +pci_set_drvdata (pdev, (void *)p_board_dat);Remove the (void *) cast. It should not be necessary.
Quoted 24 lines
+
dev_dbg(&pdev->dev, "%s invoked pci_set_drvdata\n", __func__);drivers/spi/pch_spi_main.c new file +23
-0+ +/* 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; +}drivers/spi/pch_spi_main.c line 1557
Quoted 10 lines
drivers/spi/pch_spi_main.c new file +10
-0+ +static void pch_spi_remove (struct pci_dev *pdev) +{ +int i; + +struct pch_spi_board_data *p_board_dat =pci_get_drvdata (pdev); + +dev_dbg (&pdev->dev,"%s ENTRY\n" , __func__); + +if (p_board_dat !=NULL ) {if (!p_board_dat) {
dev_err(...);
return;
}Quoted 53 lines
+
dev_dbg(&pdev->dev, "%s invoked pci_get_drvdata\n", __func__);drivers/spi/pch_spi_main.c new file +52
-0+ +/* 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;drivers/spi/pch_spi_main.c line 1620
Quoted 10 lines
drivers/spi/pch_spi_main.c new file +10
-0+s32 retval; + +struct pch_spi_board_data *p_board_dat =pci_get_drvdata (pdev); + +dev_dbg (&pdev->dev,"%s ENTRY\n" , __func__); + +if (p_board_dat ==NULL ) { +dev_err (&pdev->dev, +"%s pci_get_drvdata returned NULL\n" , __func__); + retval = -EFAULT ;Ditto here; return
-EFAULT;Quoted 157 lines
+ } else {
+ retval = 0;
+ dev_dbg(&pdev->dev,
+ "%spci_get_drvdatainvoked successfully\n",__func__);
+p_board_dat->suspend_sts= true;
+ dev_dbg(&pdev->dev,
+ "%sp_board_dat->bSuspendingset to true\n",__func__);drivers/spi/pch_spi_main.c new file +150
-0+ +/* 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__); + }drivers/spi/pch_spi_main.c line 1787
Quoted 10 lines
drivers/spi/pch_spi_main.c new file +10
-0+ +/* set suspend status to false */ + p_board_dat->suspend_sts = false; + +dev_dbg (&pdev->dev,"%s set p_board_dat" +"->bSuspending = false\n" , __func__); + } + } + } + }Another very deeply indented function.
Quoted 76 lines
drivers/spi/pch_spi_main.c new file +36
-0+ +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); +drivers/spi/pch_spi_platform_devices.c new file +15
-0@@ -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.
+ */drivers/spi/pch_spi_platform_devices.c new file +16
-0+ +#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)drivers/spi/pch_spi_platform_devices.c line 42
Quoted 10 lines
drivers/spi/pch_spi_platform_devices.c new file +10
-0+ { + .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 arespi_devices.drivers/spi/pch_spi_platform_devices.c line 46
Quoted 4 lines
drivers/spi/pch_spi_platform_devices.c new file +4
-0+ +static __init int Load (void ) +{ +int iRetVal =-1 ;int rc = -1;
Quoted 11 lines
drivers/spi/pch_spi_platform_devices.c line 55
Quoted 9 lines
drivers/spi/pch_spi_platform_devices.c new file +9
-0+ +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.
Quoted 7 lines
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)
Hi Ohtake-san,
Thanks for the reply. Some more comments below.
Quoted 73 lines · Masayuki Ohtake
----- Original Message -----
From: "Grant Likely" <[email protected]>
To: "Masayuki Ohtak" <[email protected]>
Cc: <[email protected]>; "LKML" <[email protected]>; "David Brownell" <[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>
Sent: Saturday, August 14, 2010 3:49 PM
Subject: Re: [PATCH] Topcliff: UpdatePCH_SPIdriver to 2.6.35Quoted 47 lines
2010/8/10 Masayuki Ohtak <[email protected]>:
Quoted 9 lines
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 <[email protected]>Hi Masayuki. Thanks for the patch. The driver is mostly structured well and looks fairly good. However, there are quite a few stylistic issues that should be easy to clean up, and a few technical concerns. Comments below.
BTW, please make sure patches get sent out as plain-text, not html formatted.
Quoted 20 lines
---
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 100644drivers/spi/pch_spi_main.ccreate mode 100644drivers/spi/pch_spi_platform_devices.cdrivers/spi/Kconfig +0
-0@@ -53,6 +53,32 @@ if SPI_MASTERcomment "SPI Master Controller Drivers"
Quoted 6 lines
+config
PCH_SPI_PLATFORM_DEVICE
+ boolean
+ default y ifPCH_SPI
+ help
+ This driver is for PCH SPI of Topcliff which is an IOH for x86
+ embedded processor.What is IOH and abbreviation for?
Quoted 15 lines
drivers/spi/Kconfig if SPI_MASTER · line 72
Quoted 11 lines
+ This registers SPI devices for a given board.
drivers/spi/Kconfig +10
-0+ +config PCH_SPI_PLATFORM_DEVICE_COUNT + int "PCH SPI Bus count" + range 1 2 + depends on PCH_SPI_PLATFORM_DEVICE + help + This driver is for PCH SPI of Topcliff which is an IOH for x86 + embedded processor. + The number of SPI buses/channels supported by the PCH SPI controller. + PCH SPI of Topcliff supports only one channel.Being required to specific this at kernel config time isn't multiplatform friendly. Can the driver detect the number of busses at runtime.
Quoted 2 lines
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.
Quoted 38 lines
Quoted 38 lines
drivers/spi/Kconfig if SPI_MASTER · line 80
Quoted 10 lines
drivers/spi/Kconfig +8
-0+ +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_SPIabovePCH_SPI_PLATFORM_DEVICEand makePCH_SPI_PLATFORM_DEVICEdepend onPCH_SPI. (In fact, sincePCH_SPI_PLATFORM_DEVICEis always yes whenPCH_SPIis selected, then you probably don't need two configuration signals).Quoted 9 lines
+
configSPI_ATMEL
tristate "Atmel SPI Controller"
depends on (ARCH_AT91|| AVR32)drivers/spi/Makefile +0
-0drivers/spi/Makefile obj-$(CONFIG_SPI_TLE62X0) += tle62x0.o · line 64
Quoted 6 lines
drivers/spi/Makefile +3
-0@@ -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.oNo 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_").
Quoted 7 lines
Quoted 2 lines
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.
Quoted 23 lines
Quoted 16 lines
drivers/spi/pch_spi.h line 3
Quoted 12 lines
drivers/spi/Makefile +1
-0+drivers/spi/pch_spi.h new file +3
-0@@ -0,0 +1,177 @@ +/* +* Copyright (C) 2010 OKI SEMICONDUCTOR Co., LTD. +* It is helpful to have a one line description of what this file is for in at the top of the header comment block.
Quoted 6 lines
[...]
drivers/spi/pch_spi.h line 103
Quoted 2 lines
drivers/spi/pch_spi.h new file +2
-0+#define PCH_SET_BITMSK (var, bitmask) ((var) |= (bitmask)) +#define PCH_CLR_BITMSK (var, bitmask) ((var) &= (~(bitmask)))Because the macro has side-effects, I'd prefer to see static inlines used here with an all lowercase name.
Quoted 4 lines
I understand your saying.
But this macro uses for both setting value and returned value.
If replacing inline function, we should make 4functions(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.
Quoted 125 lines
Quoted 116 lines
Quoted 116 lines
drivers/spi/pch_spi.h new file +74
-0+ +/** +* 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 drivers/spi/pch_spi_main.c new file +15
-0@@ -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.
+ */drivers/spi/pch_spi_main.c new file +17
-0+ +#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 ,} +}; +Quoted 8 lines
drivers/spi/pch_spi_main.c line 35
Quoted 2 lines
drivers/spi/pch_spi_main.c new file +2
-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_datastructure. Getting rid of the static symbols will make it easier if the driver ever needs to support multiple instances of the device.
Quoted 4 lines
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.
Quoted 5 lines
----- Original Message -----
From: "Grant Likely" <[email protected]>
To: "Masayuki Ohtake" <[email protected]>
Cc: <[email protected]>; "LKML" <[email protected]>; "David Brownell" <[email protected]>;
<[email protected]>; "Wang, Qi" <[email protected]>; "Wang, Yong Y" <[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>
Quoted 4 lines
Sent: Saturday, August 28, 2010 2:15 AM
Subject: Re: [PATCH] Topcliff: UpdatePCH_SPIdriver to 2.6.35
Quoted 10 lines
Hi Ohtake-san,
Thanks for the reply. Some more comments below.
Quoted 4 lines · Masayuki Ohtake
----- Original Message -----
From: "Grant Likely" <[email protected]>
To: "Masayuki Ohtak" <[email protected]>
Cc: <[email protected]>; "LKML" <[email protected]>; "David Brownell"
Quoted 72 lines
Quoted 69 lines
<[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>
Sent: Saturday, August 14, 2010 3:49 PM
Subject: Re: [PATCH] Topcliff: UpdatePCH_SPIdriver to 2.6.35Quoted 47 lines
2010/8/10 Masayuki Ohtak <[email protected]>:
Quoted 9 lines
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 <[email protected]>Hi Masayuki. Thanks for the patch. The driver is mostly structured well and looks fairly good. However, there are quite a few stylistic issues that should be easy to clean up, and a few technical concerns. Comments below.
BTW, please make sure patches get sent out as plain-text, not html formatted.
Quoted 20 lines
---
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 100644drivers/spi/pch_spi_main.ccreate mode 100644drivers/spi/pch_spi_platform_devices.cdrivers/spi/Kconfig +0
-0@@ -53,6 +53,32 @@ if SPI_MASTERcomment "SPI Master Controller Drivers"
Quoted 6 lines
+config
PCH_SPI_PLATFORM_DEVICE
+ boolean
+ default y ifPCH_SPI
+ help
+ This driver is for PCH SPI of Topcliff which is an IOH for x86
+ embedded processor.What is IOH and abbreviation for?
Quoted 15 lines
drivers/spi/Kconfig if SPI_MASTER · line 72
Quoted 11 lines
+ This registers SPI devices for a given board.
drivers/spi/Kconfig +10
-0+ +config PCH_SPI_PLATFORM_DEVICE_COUNT + int "PCH SPI Bus count" + range 1 2 + depends on PCH_SPI_PLATFORM_DEVICE + help + This driver is for PCH SPI of Topcliff which is an IOH for x86 + embedded processor. + The number of SPI buses/channels supported by the PCH SPI controller. + PCH SPI of Topcliff supports only one channel.Being required to specific this at kernel config time isn't multiplatform friendly. Can the driver detect the number of busses at runtime.
Quoted 2 lines
How can the driver detect ?
Could you show reference driver ?
Quoted 6 lines
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.
Quoted 51 lines
Quoted 39 lines
Quoted 39 lines
drivers/spi/Kconfig if SPI_MASTER · line 80
Quoted 11 lines
drivers/spi/Kconfig +8
-0+ +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_SPIabovePCH_SPI_PLATFORM_DEVICEand makePCH_SPI_PLATFORM_DEVICEdepend onPCH_SPI. (In fact, sincePCH_SPI_PLATFORM_DEVICEis always yes whenPCH_SPIis selected, then you probably don't need two configuration signals).Quoted 9 lines
+
configSPI_ATMEL
tristate "Atmel SPI Controller"
depends on (ARCH_AT91|| AVR32)drivers/spi/Makefile +0
-0drivers/spi/Makefile obj-$(CONFIG_SPI_TLE62X0) += tle62x0.o · line 64
Quoted 6 lines
drivers/spi/Makefile +3
-0@@ -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.oNo 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_").Quoted 7 lines
Quoted 2 lines
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_pchand 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?
Quoted 5 lines
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_devicesandpch_spi_maininto a single module.
Following your comments, we will merge to a single file.
Quoted 29 lines
Quoted 24 lines
Quoted 17 lines
drivers/spi/pch_spi.h line 3
Quoted 13 lines
drivers/spi/Makefile +1
-0+drivers/spi/pch_spi.h new file +3
-0@@ -0,0 +1,177 @@ +/* +* Copyright (C) 2010 OKI SEMICONDUCTOR Co., LTD. +* It is helpful to have a one line description of what this file is for in at the top of the header comment block.
Quoted 6 lines
[...]
drivers/spi/pch_spi.h line 103
Quoted 2 lines
drivers/spi/pch_spi.h new file +2
-0+#define PCH_SET_BITMSK (var, bitmask) ((var) |= (bitmask)) +#define PCH_CLR_BITMSK (var, bitmask) ((var) &= (~(bitmask)))Because the macro has side-effects, I'd prefer to see static inlines used here with an all lowercase name.
Quoted 4 lines
I understand your saying.
But this macro uses for both setting value and returned value.
If replacing inline function, we should make 4functions(set/clear * void/u32)Thus, I want to use these macros.
Quoted 5 lines
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..
Quoted 126 lines
Quoted 126 lines
Quoted 125 lines
Quoted 109 lines
drivers/spi/pch_spi.h new file +74
-0+ +/** +* 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 drivers/spi/pch_spi_main.c new file +15
-0@@ -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.
+ */drivers/spi/pch_spi_main.c new file +9
-0+ +#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> drivers/spi/pch_spi_main.c line 35
Quoted 10 lines
drivers/spi/pch_spi_main.c new file +10
-0+#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_datastructure. Getting rid of the static symbols will make it easier if the driver ever needs to support multiple instances of the device.
Quoted 9 lines
Quoted 4 lines
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)