by Randy Dunlap

[permalink] [raw]

Subject: Re: [PATCH 1/2] spi: add Intel Mid SSP driver

Hi,
Here are a few comments for your next version.

On 10/29/13 11:05, David Cohen wrote:
> From: Fei Yang <[email protected]>
>
> This patch adds driver for ssp spi interface on Intel Mid platform.
>
> Signed-off-by: David Cohen <[email protected]>
> Signed-off-by: Fei Yang <[email protected]>
> Reviewed-by: Ning Li <[email protected]>
> ---
> drivers/spi/Kconfig | 12 +
> drivers/spi/Makefile | 1 +
> drivers/spi/spi-intel-mid-ssp.c | 1506 +++++++++++++++++++++++++++++++++
> include/linux/spi/intel_mid_ssp_spi.h | 330 ++++++++
> 4 files changed, 1849 insertions(+)
> create mode 100644 drivers/spi/spi-intel-mid-ssp.c
> create mode 100644 include/linux/spi/intel_mid_ssp_spi.h
>
> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index b9c53cc..e211829 100644
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -211,6 +211,18 @@ config SPI_IMX
> This enables using the Freescale i.MX SPI controllers in master
> mode.
>
> +config SPI_INTEL_MID_SSP
> + tristate "SSP SPI controller driver for Intel MID platforms (EXPERIMENTAL)"
> + depends on X86_INTEL_MID && SPI_MASTER && INTEL_MID_DMAC
> + help
> + Select Y or M to build the unified SSP SPI slave controller driver
> + for the Intel MID platforms (Medfield, Clovertrail and Merrifield),
> + primarily used to implement a SPI host controller driver over a SSP

I would say:
an SPI an SSP

> + host controller.
> +
> + If you choose to build this driver as module it will be called
> + spi-intel-mid-ssp.ko
> +
> config SPI_LM70_LLP
> tristate "Parallel port adapter for LM70 eval board (DEVELOPMENT)"
> depends on PARPORT
> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> index ab8d864..60c8a1e 100644
> --- a/drivers/spi/Makefile
> +++ b/drivers/spi/Makefile
> @@ -38,6 +38,7 @@ obj-$(CONFIG_SPI_FSL_ESPI) += spi-fsl-espi.o
> obj-$(CONFIG_SPI_FSL_SPI) += spi-fsl-spi.o
> obj-$(CONFIG_SPI_GPIO) += spi-gpio.o
> obj-$(CONFIG_SPI_IMX) += spi-imx.o
> +obj-$(CONFIG_SPI_INTEL_MID_SSP) += spi-intel-mid-ssp.o
> obj-$(CONFIG_SPI_LM70_LLP) += spi-lm70llp.o
> obj-$(CONFIG_SPI_MPC512x_PSC) += spi-mpc512x-psc.o
> obj-$(CONFIG_SPI_MPC52xx_PSC) += spi-mpc52xx-psc.o
> diff --git a/drivers/spi/spi-intel-mid-ssp.c b/drivers/spi/spi-intel-mid-ssp.c
> new file mode 100644
> index 0000000..b3b9fe8
> --- /dev/null
> +++ b/drivers/spi/spi-intel-mid-ssp.c
> @@ -0,0 +1,1506 @@
> +/*
> + * spi-intel-mid-ssp.c
> + * This driver supports Bulverde SSP core used on Intel MID platforms
> + * It supports SSP of Moorestown & Medfield platforms and handles clock
> + * slave & master modes.
> + *
> + * Copyright (c) 2013, Intel Corporation.
> + * Contacts: Ning Li <[email protected]>
> + * David Cohen <[email protected]>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope 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.,
> + * 51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA.
> + *
> + */
> +
> +/*
> + * Note:
> + *
> + * Supports DMA and non-interrupt polled transfers.
> + *
> + */
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/highmem.h>
> +#include <linux/pci.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/intel_mid_dma.h>
> +#include <linux/pm_qos.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/completion.h>
> +#include <asm/intel-mid.h>
> +
> +#include <linux/spi/spi.h>
> +#include <linux/spi/intel_mid_ssp_spi.h>
> +
> +#define DRIVER_NAME "intel_mid_ssp_spi_unified"
> +
> +static int ssp_timing_wr;
> +
> +#ifdef DUMP_RX
> +static void dump_trailer(const struct device *dev, char *buf, int len, int sz)
> +{
> + int tlen1 = (len < sz ? len : sz);
> + int tlen2 = ((len - sz) > sz) ? sz : (len - sz);
> + unsigned char *p;
> + static char msg[MAX_SPI_TRANSFER_SIZE];
> +

#include <linux/printk.h>

and try to use print_hex_dump() for this instead of rolling your own...

> + memset(msg, '\0', sizeof(msg));
> + p = buf;
> + while (p < buf + tlen1)
> + sprintf(msg, "%s%02x", msg, (unsigned int)*p++);
> +
> + if (tlen2 > 0) {
> + sprintf(msg, "%s .....", msg);
> + p = (buf+len) - tlen2;
> + while (p < buf + len)
> + sprintf(msg, "%s%02x", msg, (unsigned int)*p++);
> + }
> +
> + dev_info(dev, "DUMP: %p[0:%d ... %d:%d]:%s", buf, tlen1 - 1,
> + len-tlen2, len - 1, msg);
> +}
> +#endif
> +
> +static void flush(struct ssp_drv_context *sspc)
> +{
> + void *reg = sspc->ioaddr;
> + u32 i = 0;
> +
> + /* If the transmit fifo is not empty, reset the interface. */
> + if (!is_tx_fifo_empty(sspc)) {
> + dev_err(&sspc->pdev->dev, "TX FIFO not empty. Reset of SPI IF");
> + disable_interface(sspc);
> + return;
> + }
> +
> + dev_dbg(&sspc->pdev->dev, " SSSR=%x\r\n", read_SSSR(reg));
> + while (!is_rx_fifo_empty(sspc) && (i < SPI_FIFO_SIZE + 1)) {
> + read_SSDR(reg);
> + i++;
> + }
> + WARN(i > 0, "%d words flush occured\n", i);

occurred

> +
> + return;
> +}
> +
> +
> +
> + /* set the dma done bit to 1 */

unneeded comment (obvious)

> + sspc->txdma_done = 1;
> + sspc->rxdma_done = 1;
> +
> + sspc->tx_param.drv_context = sspc;
> + sspc->tx_param.direction = TX_DIRECTION;
> + sspc->rx_param.drv_context = sspc;
> + sspc->rx_param.direction = RX_DIRECTION;
> +
> + sspc->dma_initialized = 1;
> + return;

> +/**
> + * map_dma_buffers() - Map DMA buffer before a transfer
> + * @sspc: Pointer to the private drivzer context

driver

> +static void poll_transfer_complete(struct ssp_drv_context *sspc)
> +{
> + struct spi_message *msg;
> +
> + /* Update total byte transfered return count actual bytes read */

transferred;

> + sspc->cur_msg->actual_length += sspc->len - (sspc->rx_end - sspc->rx);
> +
> + sspc->cur_msg->status = 0;
> + msg = sspc->cur_msg;
> + if (likely(msg->complete))
> + msg->complete(msg->context);
> + complete(&sspc->msg_done);
> +}
> +
> +/**
> + * ssp_int() - Interrupt handler
> + * @irq
> + * @dev_id

Please finish the kernel-doc notation (or drop it).

> + *
> + * The SSP interrupt is not used for transfer which are handled by
> + * DMA or polling: only under/over run are catched to detect
> + * broken transfers.
> + */
> +static irqreturn_t ssp_int(int irq, void *dev_id)
> +{
> + struct ssp_drv_context *sspc = dev_id;
> + void *reg = sspc->ioaddr;
> + struct device *dev = &sspc->pdev->dev;
> + u32 status = read_SSSR(reg);
> +
> + /* It should never be our interrupt since SSP will */
> + /* only trigs interrupt for under/over run. */

trigger

> + if (likely(!(status & sspc->mask_sr)))
> + return IRQ_NONE;
> +
> + if (status & SSSR_ROR || status & SSSR_TUR) {
> + dev_err(dev, "--- SPI ROR or TUR occurred : SSSR=%x\n", status);
> + WARN_ON(1);
> + if (status & SSSR_ROR)
> + dev_err(dev, "we have Overrun\n");
> + if (status & SSSR_TUR)
> + dev_err(dev, "we have Underrun\n");
> + }
> +
> + /* We can fall here when not using DMA mode */
> + if (!sspc->cur_msg) {
> + disable_interface(sspc);
> + disable_triggers(sspc);
> + }
> + /* clear status register */
> + write_SSSR(sspc->clear_sr, reg);
> + return IRQ_HANDLED;
> +}
> +

> diff --git a/include/linux/spi/intel_mid_ssp_spi.h b/include/linux/spi/intel_mid_ssp_spi.h
> new file mode 100644
> index 0000000..a3bb973
> --- /dev/null
> +++ b/include/linux/spi/intel_mid_ssp_spi.h
> @@ -0,0 +1,330 @@
> +/*
> + * Copyright (C) Intel 2013
> + *
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + *
> + * 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; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * 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
> + *
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + *
> + */
> +#ifndef INTEL_MID_SSP_SPI_H_
> +#define INTEL_MID_SSP_SPI_H_
> +
> +#include <linux/completion.h>
> +#include <linux/intel_mid_dma.h>
> +#include <linux/interrupt.h>
> +#include <linux/pm_qos.h>
> +#include <linux/spi/spi.h>
> +

> +#define SSCR0_DSS (0x0000000f) /* Data Size Select (mask) */
> +#define SSCR0_DataSize(x) ((x) - 1) /* Data Size Select [4..16] */
> +#define SSCR0_FRF (0x00000030) /* FRame Format (mask) */
> +#define SSCR0_Motorola (0x0 << 4) /* Motorola's SPI mode */

#include <linux/bitops.h>

> +#define SSCR0_ECS (1 << 6) /* External clock select */

#define SSCR0_ECS BIT(6) /* External clock select */

etc.

> +#define SSCR0_SSE (1 << 7) /* Synchronous Serial Port Enable */
> +
> +#define SSCR0_SCR (0x000fff00) /* Serial Clock Rate (mask) */
> +#define SSCR0_SerClkDiv(x) (((x) - 1) << 8) /* Divisor [1..4096] */
> +#define SSCR0_EDSS (1 << 20) /* Extended data size select */
> +#define SSCR0_NCS (1 << 21) /* Network clock select */
> +#define SSCR0_RIM (1 << 22) /* Receive FIFO overrrun int mask */
> +#define SSCR0_TUM (1 << 23) /* Transmit FIFO underrun int mask */
> +#define SSCR0_FRDC (0x07000000) /* Frame rate divider control (mask) */
> +#define SSCR0_SlotsPerFrm(x) (((x) - 1) << 24) /* Time slots per frame */
> +#define SSCR0_ADC (1 << 30) /* Audio clock select */
> +#define SSCR0_MOD (1 << 31) /* Mode (normal or network) */
> +
> +#define SSCR1_RIE (1 << 0) /* Receive FIFO Interrupt Enable */
> +#define SSCR1_TIE (1 << 1) /* Transmit FIFO Interrupt Enable */
> +#define SSCR1_LBM (1 << 2) /* Loop-Back Mode */
> +#define SSCR1_SPO (1 << 3) /* SSPSCLK polarity setting */
> +#define SSCR1_SPH (1 << 4) /* Motorola SPI SSPSCLK phase setting */
> +#define SSCR1_MWDS (1 << 5) /* Microwire Transmit Data Size */
> +#define SSCR1_TFT (0x000003c0) /* Transmit FIFO Threshold (mask) */
> +#define SSCR1_TxTresh(x) (((x) - 1) << 6) /* level [1..16] */
> +#define SSCR1_RFT (0x00003c00) /* Receive FIFO Threshold (mask) */
> +#define SSCR1_RxTresh(x) (((x) - 1) << 10) /* level [1..16] */
> +
> +#define SSSR_TNF (1 << 2) /* Tx FIFO Not Full */
> +#define SSSR_RNE (1 << 3) /* Rx FIFO Not Empty */
> +#define SSSR_BSY (1 << 4) /* SSP Busy */
> +#define SSSR_TFS (1 << 5) /* Tx FIFO Service Request */
> +#define SSSR_RFS (1 << 6) /* Rx FIFO Service Request */
> +#define SSSR_ROR (1 << 7) /* Rx FIFO Overrun */
> +#define SSSR_TFL_MASK (0x0f << 8) /* Tx FIFO level field mask */
> +
> +#define SSCR0_TIM (1 << 23) /* Transmit FIFO Under Run Int Mask */
> +#define SSCR0_RIM (1 << 22) /* Receive FIFO Over Run int Mask */
> +#define SSCR0_NCS (1 << 21) /* Network Clock Select */
> +#define SSCR0_EDSS (1 << 20) /* Extended Data Size Select */
> +
> +#define SSCR0_TISSP (1 << 4) /* TI Sync Serial Protocol */
> +#define SSCR0_PSP (3 << 4) /* PSP - Programmable Serial Protocol */
> +#define SSCR1_TTELP (1 << 31) /* TXD Tristate Enable Last Phase */
> +#define SSCR1_TTE (1 << 30) /* TXD Tristate Enable */
> +#define SSCR1_EBCEI (1 << 29) /* Enable Bit Count Error interrupt */
> +#define SSCR1_SCFR (1 << 28) /* Slave Clock free Running */
> +#define SSCR1_ECRA (1 << 27) /* Enable Clock Request A */
> +#define SSCR1_ECRB (1 << 26) /* Enable Clock request B */
> +#define SSCR1_SCLKDIR (1 << 25) /* Serial Bit Rate Clock Direction */
> +#define SSCR1_SFRMDIR (1 << 24) /* Frame Direction */
> +#define SSCR1_RWOT (1 << 23) /* Receive Without Transmit */
> +#define SSCR1_TRAIL (1 << 22) /* Trailing Byte */
> +#define SSCR1_TSRE (1 << 21) /* Transmit Service Request Enable */
> +#define SSCR1_RSRE (1 << 20) /* Receive Service Request Enable */
> +#define SSCR1_TINTE (1 << 19) /* Receiver Time-out Interrupt enable */
> +#define SSCR1_PINTE (1 << 18) /* Trailing Byte Interupt Enable */
> +#define SSCR1_STRF (1 << 15) /* Select FIFO or EFWR */
> +#define SSCR1_EFWR (1 << 14) /* Enable FIFO Write/Read */
> +#define SSCR1_IFS (1 << 16) /* Invert Frame Signal */
> +
> +#define SSSR_BCE (1 << 23) /* Bit Count Error */
> +#define SSSR_CSS (1 << 22) /* Clock Synchronisation Status */
> +#define SSSR_TUR (1 << 21) /* Transmit FIFO Under Run */
> +#define SSSR_EOC (1 << 20) /* End Of Chain */
> +#define SSSR_TINT (1 << 19) /* Receiver Time-out Interrupt */
> +#define SSSR_PINT (1 << 18) /* Peripheral Trailing Byte Interrupt */
> +
> +#define SSPSP_FSRT (1 << 25) /* Frame Sync Relative Timing */
> +#define SSPSP_DMYSTOP(x) ((x) << 23) /* Dummy Stop */
> +#define SSPSP_SFRMWDTH(x)((x) << 16) /* Serial Frame Width */
> +#define SSPSP_SFRMDLY(x) ((x) << 9) /* Serial Frame Delay */
> +#define SSPSP_DMYSTRT(x) ((x) << 7) /* Dummy Start */
> +#define SSPSP_STRTDLY(x) ((x) << 4) /* Start Delay */
> +#define SSPSP_ETDS (1 << 3) /* End of Transfer data State */
> +#define SSPSP_SFRMP (1 << 2) /* Serial Frame Polarity */
> +#define SSPSP_SCMODE(x) ((x) << 0) /* Serial Bit Rate Clock Mode */

--
~Randy

2013-10-29 18:30:52

On Tue, Oct 29, 2013 at 11:05:49AM -0700, David Cohen wrote:
> From: Fei Yang <[email protected]>
>
> This patch adds driver for ssp spi interface on Intel Mid platform.

Is there not any possibility of code sharing with the PXA SSP IP? It
seems odd that Intel would make a second IP of the same name that's
totally incompatible... not looked at the register interfaces in detail
but the register names certainly look familiar.

> + tristate "SSP SPI controller driver for Intel MID platforms (EXPERIMENTAL)"
> + depends on X86_INTEL_MID && SPI_MASTER && INTEL_MID_DMAC
> + help

Are these all build time dependencies? There's certainly no need to
depend on SPI_MASTER, all drivers are inside an if SPI_MASTER block.

> +static int ssp_timing_wr;

Why is this a static global?

> +#ifdef DUMP_RX
> +static void dump_trailer(const struct device *dev, char *buf, int len, int sz)
> +{

Please add this support as a standard feature of the SPI core if it's
useful, there's nothing specific to this driver in it. Using trace
would probably be better than dumping to the console.

> + static char msg[MAX_SPI_TRANSFER_SIZE];

Where did that limit come from?

> +static inline u32 is_tx_fifo_empty(struct ssp_drv_context *sspc)
> +{
> + u32 sssr;
> + sssr = read_SSSR(sspc->ioaddr);
> + if ((sssr & SSSR_TFL_MASK) || (sssr & SSSR_TNF) == 0)

This looks odd, why not sssr & (SSSR_TFL_MASK | SSSR_TNF)?

> +static void flush(struct ssp_drv_context *sspc)
> +{
> + void *reg = sspc->ioaddr;
> + u32 i = 0;
> +
> + /* If the transmit fifo is not empty, reset the interface. */
> + if (!is_tx_fifo_empty(sspc)) {
> + dev_err(&sspc->pdev->dev, "TX FIFO not empty. Reset of SPI IF");
> + disable_interface(sspc);
> + return;
> + }

This isn't a flush then?

> + dev_dbg(&sspc->pdev->dev, " SSSR=%x\r\n", read_SSSR(reg));

No extra space at the start of the message and why is there a \r in
there? Throughout the driver your log messages have a range of odd
formatting quirks that aren't consistent and don't look like normal
Linux log messages either.

Please also check the severity of your messages, many of them seem too
loud.

> + while (!is_rx_fifo_empty(sspc) && (i < SPI_FIFO_SIZE + 1)) {
> + read_SSDR(reg);
> + i++;
> + }

What happens if the FIFO doesn't drain?

> + WARN(i > 0, "%d words flush occured\n", i);

This seems *very* loud for a flush operation...

> + if (sspc->quirks & QUIRKS_SRAM_ADDITIONAL_CPY) {
> + sspc->virt_addr_sram_rx = ioremap_nocache(SRAM_BASE_ADDR,
> + 2 * MAX_SPI_TRANSFER_SIZE);

This doesn't look terribly clever, it's remapping a hard coded address
rather than getting the address enumerated from a device enumeration
interface.

> + /* get Data Read/Write address */
> + ssdr_addr = (dma_addr_t)(sspc->paddr + 0x10);

I'm not entirely sure what this does but it looks dodgy... what's the
cast doing for a start?

> + /* In Rx direction, TRAIL Bytes are handled by memcpy */

Please don't randomly CAPITALISE words.

> + rxdesc = rxchan->device->device_prep_dma_memcpy
> + (rxchan, /* DMA Channel */
> + sspc->rx_dma, /* DAR */
> + ssdr_addr, /* SAR */
> + sspc->len_dma_rx, /* Data Length */
> + flag); /* Flag */

What's going on here? Why is there a DMAed memcpy()? This doesn't
reflect normal DMA usage in SPI drivers at all, I'd expect to see
dmaengine_prep_ being used.

> +static void int_transfer_complete(struct ssp_drv_context *sspc)
> +{
> + void *reg = sspc->ioaddr;
> + struct spi_message *msg;
> + struct device *dev = &sspc->pdev->dev;
> +
> + if (unlikely(sspc->quirks & QUIRKS_USE_PM_QOS))
> + pm_qos_update_request(&sspc->pm_qos_req,
> + PM_QOS_DEFAULT_VALUE);

Why is this a quirk and not abstracted away by the PM QoS API on
platforms that don't need it? I'd also expect these updates to be done
in runtime PM callbacks so that if two transfers follow one another we
don't bounce the Qos up and down.

> + dev_dbg(dev, "End of transfer. SSSR:%08X\n", read_SSSR(reg));
> + msg = sspc->cur_msg;
> + if (likely(msg->complete))
> + msg->complete(msg->context);
> + complete(&sspc->msg_done);

Remove all these likely()s, they're making the code less clear and seem
vanishingly unlikely to have any practical impact on performance.

> +static void int_transfer_complete_work(struct work_struct *work)
> +{
> + struct ssp_drv_context *sspc = container_of(work,
> + struct ssp_drv_context, complete_work);
> +
> + int_transfer_complete(sspc);
> +}

This wrapper function doesn't seem terribly useful... why not just
inline it into the internal function?

> + if (status & SSSR_ROR || status & SSSR_TUR) {
> + dev_err(dev, "--- SPI ROR or TUR occurred : SSSR=%x\n", status);
> + WARN_ON(1);
> + if (status & SSSR_ROR)
> + dev_err(dev, "we have Overrun\n");
> + if (status & SSSR_TUR)
> + dev_err(dev, "we have Underrun\n");
> + }
> +
> + /* We can fall here when not using DMA mode */
> + if (!sspc->cur_msg) {
> + disable_interface(sspc);
> + disable_triggers(sspc);
> + }
> + /* clear status register */
> + write_SSSR(sspc->clear_sr, reg);

More comments explaining what's going on here would be good...

> +static void poll_transfer(unsigned long data)
> +{
> + struct ssp_drv_context *sspc = (void *)data;
> +
> + if (sspc->tx) {
> + while (sspc->tx != sspc->tx_end) {
> + if (ssp_timing_wr) {
> + int timeout = 100;
> + /* It is used as debug UART on Tangier. Since
> + baud rate = 115200, it needs at least 312us
> + for one word transferring. Becuase of silicon
> + issue, it MUST check SFIFOL here instead of
> + TNF. It is the workaround for A0 stepping*/
> + while (--timeout &&
> + ((read_SFIFOL(sspc->ioaddr)) & 0xFFFF))
> + udelay(10);
> + }

This doesn't look like it shouldn't be here at all or should be made
more generic but I can't really tell what it's supposed to do...

> +static void start_bitbanging(struct ssp_drv_context *sspc)
> +{

The SPI core has bitbanging helpers which you don't seem to be using.
However...

> + /* Bit bang the clock until CSS clears */
> + while ((sssr & 0x400000) && (count < MAX_BITBANGING_LOOP)) {
> + write_I2CDATA(0x2, i2c_reg);
> + udelay(I2C_ACCESS_USDELAY);

...this code is talking about I2C?

> + udelay(I2C_ACCESS_USDELAY);
> + write_I2CCTRL(0x01070034, i2c_reg);

This appears to be randomly bashing on an absolute register address.

> +/**
> + * transfer() - Start a SPI transfer
> + * @spi: Pointer to the spi_device struct
> + * @msg: Pointer to the spi_message struct
> + */
> +static int transfer(struct spi_device *spi, struct spi_message *msg)

Use of plain transfer() has been deprecated since v3.4, you should at
least be using transfer_one_message(). I've stopped reviewing at this
point since a lot of the rest of the code is replicating stuff that's in
the SPI core and will go away when you update to use the current APIs.

Attachments:

(No filename) (6.69 kB)
signature.asc (836.00 B)
Digital signature Download all attachments

2013-10-29 18:58:25

On 10/31/2013 04:12 PM, Joe Perches wrote:
> Find a few more cases where parentheses are used around
> the value of a return statement.
>
> This now uses the "$balanced_parens" test and also makes
> the test depend on perl v5.10 and higher.
>
> This now finds return with parenthesis uses the old code
> did not find like:
>
> ERROR: return is not a function, parentheses are not required
> #211: FILE: arch/m68k/include/asm/sun3xflop.h:211:
> + return ((error == 0) ? 0 : -1);
>
> Signed-off-by: Joe Perches <[email protected]>

It catches the previous false-negative I had.

Acked-by: David Cohen <[email protected]>