Re: [PATCH v3 2/2] spi: Introduce new driver for Qualcomm QuadSPI controller

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Quoting Ryan Case (2018-09-26 13:52:04)
> From: Girish Mahadevan <girishm@xxxxxxxxxxxxxx>
> 
> New driver for Qualcomm QuadSPI(QSPI) controller that is used to
> communicate with slaves such flash memory devices. The QSPI controller

s/such/such as/?

> can operate in 2 or 4 wire mode but only supports SPI Mode 0. The
> controller can also operate in Single or Dual data rate modes.
> 
> Signed-off-by: Girish Mahadevan <girishm@xxxxxxxxxxxxxx>
> Signed-off-by: Ryan Case <ryandcase@xxxxxxxxxxxx>
> ---
> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> index 876f8690fc47..f997c49255a6 100644
> --- a/drivers/spi/Makefile
> +++ b/drivers/spi/Makefile
> @@ -112,3 +112,4 @@ obj-$(CONFIG_SPI_ZYNQMP_GQSPI)              += spi-zynqmp-gqspi.o
>  # SPI slave protocol handlers
>  obj-$(CONFIG_SPI_SLAVE_TIME)           += spi-slave-time.o
>  obj-$(CONFIG_SPI_SLAVE_SYSTEM_CONTROL) += spi-slave-system-control.o
> +obj-$(CONFIG_SPI_QCOM_QSPI)            += spi-qcom-qspi.o

Move this to alphabetical in the SPI master controller list of this
file please.

> diff --git a/drivers/spi/spi-qcom-qspi.c b/drivers/spi/spi-qcom-qspi.c
> new file mode 100644
> index 000000000000..0ad2ef003068
> --- /dev/null
> +++ b/drivers/spi/spi-qcom-qspi.c
> @@ -0,0 +1,598 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2017-2018, The Linux foundation. All rights reserved.
> +
> +#include <linux/clk.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/spi/spi.h>
> +#include <linux/spi/spi-mem.h>
> +
> +#include <asm/unaligned.h>
> +
> +#define AHB_MIN_HZ             9600000UL

Is this used?

> +#define QSPI_NUM_CS            2
> +#define QSPI_BYTES_PER_WORD    4
> +#define MSTR_CONFIG            0x0000
> +#define AHB_MASTER_CFG         0x0004
> +#define MSTR_INT_EN            0x000C
> +#define MSTR_INT_STATUS                0x0010
> +#define PIO_XFER_CTRL          0x0014
> +#define PIO_XFER_CFG           0x0018
> +#define PIO_XFER_STATUS                0x001c
> +#define PIO_DATAOUT_1B         0x0020
> +#define PIO_DATAOUT_4B         0x0024
> +#define RD_FIFO_CFG            0x0028
> +#define RD_FIFO_STATUS         0x002c
> +#define RD_FIFO_RESET          0x0030
> +#define CUR_MEM_ADDR           0x0048
> +#define HW_VERSION             0x004c
> +#define RD_FIFO                        0x0050
> +#define SAMPLING_CLK_CFG       0x0090
> +#define SAMPLING_CLK_STATUS    0x0094
> +
> +/* Macros to help set/get fields in MSTR_CONFIG register */
> +#define        FULL_CYCLE_MODE         BIT(3)
> +#define        FB_CLK_EN               BIT(4)
> +#define        PIN_HOLDN               BIT(6)
> +#define        PIN_WPN                 BIT(7)
> +#define        DMA_ENABLE              BIT(8)
> +#define        BIG_ENDIAN_MODE         BIT(9)
> +#define        SPI_MODE_MSK            0xc00
> +#define        SPI_MODE_SHFT           10
> +#define        CHIP_SELECT_NUM         BIT(12)
> +#define        SBL_EN                  BIT(13)
> +#define        LPA_BASE_MSK            0x3c000
> +#define        LPA_BASE_SHFT           14
> +#define        TX_DATA_DELAY_MSK       0xc0000
> +#define        TX_DATA_DELAY_SHFT      18
> +#define        TX_CLK_DELAY_MSK        0x300000
> +#define        TX_CLK_DELAY_SHFT       20
> +#define        TX_CS_N_DELAY_MSK       0xc00000
> +#define        TX_CS_N_DELAY_SHFT      22
> +#define        TX_DATA_OE_DELAY_MSK    0x3000000
> +#define        TX_DATA_OE_DELAY_SHFT   24
> +
> +/* Macros to help set/get fields in AHB_MSTR_CFG register */
> +#define        HMEM_TYPE_START_MID_TRANS_MSK           0x7
> +#define        HMEM_TYPE_START_MID_TRANS_SHFT          0
> +#define        HMEM_TYPE_LAST_TRANS_MSK                0x38
> +#define        HMEM_TYPE_LAST_TRANS_SHFT               3
> +#define        USE_HMEMTYPE_LAST_ON_DESC_OR_CHAIN_MSK  0xc0
> +#define        USE_HMEMTYPE_LAST_ON_DESC_OR_CHAIN_SHFT 6
> +#define        HMEMTYPE_READ_TRANS_MSK                 0x700
> +#define        HMEMTYPE_READ_TRANS_SHFT                8
> +#define        HSHARED                                 BIT(11)
> +#define        HINNERSHARED                            BIT(12)
> +
> +/* Macros to help set/get fields in MSTR_INT_EN/MSTR_INT_STATUS registers */
> +#define        RESP_FIFO_UNDERRUN      BIT(0)
> +#define        RESP_FIFO_NOT_EMPTY     BIT(1)
> +#define        RESP_FIFO_RDY           BIT(2)
> +#define        HRESP_FROM_NOC_ERR      BIT(3)
> +#define        WR_FIFO_EMPTY           BIT(9)
> +#define        WR_FIFO_FULL            BIT(10)
> +#define        WR_FIFO_OVERRUN         BIT(11)
> +#define        TRANSACTION_DONE        BIT(16)
> +#define QSPI_ERR_IRQS          (RESP_FIFO_UNDERRUN | HRESP_FROM_NOC_ERR | \
> +                                WR_FIFO_OVERRUN)
> +#define        QSPI_ALL_IRQS           (QSPI_ERR_IRQS | RESP_FIFO_RDY | \
> +                                WR_FIFO_EMPTY | WR_FIFO_FULL | \
> +                                TRANSACTION_DONE)
> +
> +/* Macros to help set/get fields in RD_FIFO_CONFIG register */
> +#define        CONTINUOUS_MODE         BIT(0)
> +
> +/* Macros to help set/get fields in RD_FIFO_RESET register */
> +#define        RESET_FIFO              BIT(0)
> +
> +/* Macros to help set/get fields in PIO_TRANSFER_CONFIG register */
> +#define        TRANSFER_DIRECTION      BIT(0)
> +#define        MULTI_IO_MODE_MSK       0xe
> +#define        MULTI_IO_MODE_SHFT      1
> +#define        TRANSFER_FRAGMENT       BIT(8)
> +
> +/* Macros to help set/get fields in PIO_TRANSFER_CONTROL register */
> +#define        REQUEST_COUNT_MSK       0xffff
> +
> +/* Macros to help set/get fields in PIO_TRANSFER_STATUS register */
> +#define        WR_FIFO_BYTES_MSK       0xffff0000
> +#define        WR_FIFO_BYTES_SHFT      16
> +
> +/* Macros to help set/get fields in RD_FIFO_STATUS register */

Typically we put these definitions immediately after the register offset
that uses them so we don't need these comments to tell us what registers
they apply to.

> +#define        FIFO_EMPTY      BIT(11)
> +#define        WR_CNTS_MSK     0x7f0
> +#define        WR_CNTS_SHFT    4
> +#define        RDY_64BYTE      BIT(3)
> +#define        RDY_32BYTE      BIT(2)
> +#define        RDY_16BYTE      BIT(1)
> +#define        FIFO_RDY        BIT(0)
> +
> +/*
> + * The Mode transfer macros, the values are programmed to the HW registers
> + * when doing PIO mode of transfers.
> + */
> +#define        SDR_1BIT        1
> +#define        SDR_2BIT        2
> +#define        SDR_4BIT        3
> +#define        DDR_1BIT        5
> +#define        DDR_2BIT        6
> +#define        DDR_4BIT        7
> +
> +/* The Mode transfer macros when setting up DMA descriptors */
> +#define        DMA_DESC_SINGLE_SPI     1
> +#define        DMA_DESC_DUAL_SPI       2
> +#define        DMA_DESC_QUAD_SPI       3
> +
> +enum qspi_dir {
> +       QSPI_READ,
> +       QSPI_WRITE,
> +};
> +
> +struct qspi_xfer {
> +       union {
> +               const void *tx_buf;
> +               void *rx_buf;
> +       };
> +       unsigned int rem_bytes;
> +       int buswidth;

unsigned int?

> +       enum qspi_dir dir;
> +       bool is_last;
> +};
> +
> +enum qspi_clocks {
> +       QSPI_CLK_CORE,
> +       QSPI_CLK_IFACE,
> +       QSPI_NUM_CLKS
> +};
> +
> +struct qcom_qspi {
> +       void __iomem *base;
> +       struct device *dev;
> +       struct clk_bulk_data clks[QSPI_NUM_CLKS];
> +       struct qspi_xfer xfer;
> +       spinlock_t lock;

What is the lock protecting? Hardware interrupt state? Maybe add a small
comment to help the reader know what needs protecting.

> +};
> +
> +static int qspi_buswidth_to_iomode(struct qcom_qspi *ctrl, int buswidth)
> +{
> +       switch (buswidth) {
> +       case 1:
> +               return SDR_1BIT;
> +       case 2:
> +               return SDR_2BIT;
> +       case 4:
> +               return SDR_4BIT;
> +       default:
> +               dev_warn_once(ctrl->dev,
> +                               "Unexpected bus width: %d\n", buswidth);
> +               return SDR_1BIT;
> +       }
> +}
> +
> +static void qcom_qspi_pio_xfer_cfg(struct qcom_qspi *ctrl)
> +{
> +       u32 pio_xfer_cfg = 0;

Please remove useless initial assignment.

> +       struct qspi_xfer *xfer;

const?

> +
> +       xfer = &ctrl->xfer;
> +       pio_xfer_cfg = readl(ctrl->base + PIO_XFER_CFG);
> +       pio_xfer_cfg &= ~TRANSFER_DIRECTION;
> +       pio_xfer_cfg |= xfer->dir;
> +       if (xfer->is_last)
> +               pio_xfer_cfg &= ~TRANSFER_FRAGMENT;
> +       else
> +               pio_xfer_cfg |= TRANSFER_FRAGMENT;
> +       pio_xfer_cfg &= ~MULTI_IO_MODE_MSK;
> +       pio_xfer_cfg |= qspi_buswidth_to_iomode(ctrl, xfer->buswidth) <<
> +                       MULTI_IO_MODE_SHFT;

Style nitpick, this looks very odd split across two lines. Probably
better to make qspi_buswidth_to_iomode() return the shifted value
because this is the only call-site and then everything fits on one line.
Could also rename 'pio_xfer_cfg' to just 'cfg' and probably everything
would be short too.

> +
> +       writel(pio_xfer_cfg, ctrl->base + PIO_XFER_CFG);
> +}
> +
> +static void qcom_qspi_pio_xfer_ctrl(struct qcom_qspi *ctrl)
> +{
> +       u32 pio_xfer_ctrl;
> +
> +       pio_xfer_ctrl = readl(ctrl->base + PIO_XFER_CTRL);
> +       pio_xfer_ctrl &= ~REQUEST_COUNT_MSK;
> +       pio_xfer_ctrl |= ctrl->xfer.rem_bytes;
> +       writel(pio_xfer_ctrl, ctrl->base + PIO_XFER_CTRL);
> +}
> +
> +static void qcom_qspi_pio_xfer(struct qcom_qspi *ctrl)
> +{
> +       u32 ints;
> +
> +       qcom_qspi_pio_xfer_cfg(ctrl);
> +
> +       /* Ack any previous interrupts that might be hanging around */
> +       writel(QSPI_ALL_IRQS, ctrl->base + MSTR_INT_STATUS);
> +
> +       /* Setup new interrupts */
> +       if (ctrl->xfer.dir == QSPI_WRITE)
> +               ints = QSPI_ERR_IRQS | WR_FIFO_EMPTY;
> +       else
> +               ints = QSPI_ERR_IRQS | RESP_FIFO_RDY;
> +       writel(ints, ctrl->base + MSTR_INT_EN);
> +
> +       /* Kick off the transfer */
> +       qcom_qspi_pio_xfer_ctrl(ctrl);
> +}
> +
> +static void qcom_qspi_handle_err(struct spi_master *master,
> +                                struct spi_message *msg)
> +{
> +       struct qcom_qspi *ctrl = spi_master_get_devdata(master);
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&ctrl->lock, flags);
> +       writel(0, ctrl->base + MSTR_INT_EN);
> +       ctrl->xfer.rem_bytes = 0;
> +       spin_unlock_irqrestore(&ctrl->lock, flags);
> +}
> +
> +static int qcom_qspi_transfer_one(struct spi_master *master,
> +                                 struct spi_device *slv,
> +                                 struct spi_transfer *xfer)
> +{
> +       struct qcom_qspi *ctrl = spi_master_get_devdata(master);
> +       int ret;
> +       unsigned long speed_hz;
> +       unsigned long flags;
> +
> +       speed_hz = slv->max_speed_hz;
> +       if (xfer->speed_hz)
> +               speed_hz = xfer->speed_hz;
> +
> +       ret = clk_set_rate(ctrl->clks[QSPI_CLK_CORE].clk, speed_hz * 4);

Why 4? Is that related to the number of wires?

> +       if (ret) {
> +               dev_err(ctrl->dev, "Failed to set core clk %d\n", ret);
> +               return ret;
> +       }
> +
> +       spin_lock_irqsave(&ctrl->lock, flags);
> +
> +       /* We are half duplex, so either rx or tx will be set */
> +       if (xfer->rx_buf) {
> +               ctrl->xfer.dir = QSPI_READ;
> +               ctrl->xfer.buswidth = xfer->rx_nbits;
> +               ctrl->xfer.rx_buf = xfer->rx_buf;
> +       } else {
> +               ctrl->xfer.dir = QSPI_WRITE;
> +               ctrl->xfer.buswidth = xfer->tx_nbits;
> +               ctrl->xfer.tx_buf = xfer->tx_buf;
> +       }
> +       ctrl->xfer.is_last = list_is_last(&xfer->transfer_list,
> +                                         &master->cur_msg->transfers);
> +       ctrl->xfer.rem_bytes = xfer->len;
> +       qcom_qspi_pio_xfer(ctrl);
> +
> +       spin_unlock_irqrestore(&ctrl->lock, flags);
> +
> +       /* We'll call spi_finalize_current_transfer() when done */
> +       return 1;
> +}
> +
> +static int qcom_qspi_prepare_message(struct spi_master *master,
> +                                    struct spi_message *message)
> +{
> +       u32 mstr_cfg;
> +       struct qcom_qspi *ctrl;
> +       int tx_data_oe_delay = 1;
> +       int tx_data_delay = 1;
> +       unsigned long flags;
> +
> +       ctrl = spi_master_get_devdata(master);
> +       spin_lock_irqsave(&ctrl->lock, flags);
> +
> +       mstr_cfg = readl(ctrl->base + MSTR_CONFIG);
> +       mstr_cfg &= ~CHIP_SELECT_NUM;
> +       if (message->spi->chip_select)
> +               mstr_cfg |= CHIP_SELECT_NUM;
> +
> +       mstr_cfg = (mstr_cfg & ~SPI_MODE_MSK) |
> +                               (message->spi->mode << SPI_MODE_SHFT);
> +       mstr_cfg |= FB_CLK_EN | PIN_WPN | PIN_HOLDN | SBL_EN | FULL_CYCLE_MODE;
> +       mstr_cfg |= (mstr_cfg & ~TX_DATA_OE_DELAY_MSK) |
> +                               (tx_data_oe_delay << TX_DATA_OE_DELAY_SHFT);
> +       mstr_cfg |= (mstr_cfg & ~TX_DATA_DELAY_MSK) |
> +                               (tx_data_delay << TX_DATA_DELAY_SHFT);

Maybe just write them all on one line with mstr_cfg |=? Then it's not
indendented like that:

       mstr_cfg &= ~(SPI_MODE_MSK | TX_DATA_OE_DELAY_MSK | TX_DATA_DELAY_MSK);
       mstr_cfg |= message->spi->mode << SPI_MODE_SHFT;
       mstr_cfg |= FB_CLK_EN | PIN_WPN | PIN_HOLDN | SBL_EN | FULL_CYCLE_MODE;
       mstr_cfg |= tx_data_oe_delay << TX_DATA_OE_DELAY_SHFT;
       mstr_cfg |= tx_data_delay << TX_DATA_DELAY_SHFT;

> +       mstr_cfg &= ~DMA_ENABLE;
> +
> +       writel(mstr_cfg, ctrl->base + MSTR_CONFIG);
> +       spin_unlock_irqrestore(&ctrl->lock, flags);
> +
> +       return 0;
> +}
> +
> +static irqreturn_t pio_read(struct qcom_qspi *ctrl)
> +{
> +       u32 rd_fifo_status;
> +       u32 rd_fifo;
> +       unsigned int wr_cnts;
> +       unsigned int bytes_to_read;
> +       unsigned int words_to_read;
> +       u32 *word_buf;
> +       u8 *byte_buf;
> +       int i;
> +
> +       rd_fifo_status = readl(ctrl->base + RD_FIFO_STATUS);
> +
> +       if (!(rd_fifo_status & FIFO_RDY)) {
> +               dev_dbg(ctrl->dev, "Spurious IRQ %#x\n", rd_fifo_status);
> +               return IRQ_NONE;
> +       }
> +
> +       wr_cnts = (rd_fifo_status & WR_CNTS_MSK) >> WR_CNTS_SHFT;
> +
> +       if (wr_cnts > ctrl->xfer.rem_bytes)
> +               wr_cnts = ctrl->xfer.rem_bytes;

Could be

	wr_cnts = min(wr_cnts, ctrl->xfer.rem_bytes)

> +
> +       words_to_read = wr_cnts / QSPI_BYTES_PER_WORD;
> +       bytes_to_read = wr_cnts % QSPI_BYTES_PER_WORD;
> +
> +       if (words_to_read) {
> +               word_buf = ctrl->xfer.rx_buf;
> +               ctrl->xfer.rem_bytes -= words_to_read * QSPI_BYTES_PER_WORD;
> +               for (i = 0; i < words_to_read; i++) {
> +                       rd_fifo = readl(ctrl->base + RD_FIFO);

This will mess things up on big endian CPUs and make the CPU buffer byte
swapped. Use readsl() or ioread32_rep().

> +                       put_unaligned(rd_fifo, word_buf++);
> +               }
> +               ctrl->xfer.rx_buf = word_buf;
> +       }
> +
> +       if (bytes_to_read) {
> +               byte_buf = ctrl->xfer.rx_buf;

Does this need to move forward by words_to_read bytes so that the left
over bytes are tacked onto the end? Or this should be an else if
statement?

> +               rd_fifo = readl(ctrl->base + RD_FIFO);
> +               ctrl->xfer.rem_bytes -= bytes_to_read;
> +               for (i = 0; i < bytes_to_read; i++)
> +                       *byte_buf++ = rd_fifo >> (i * BITS_PER_BYTE);
> +               ctrl->xfer.rx_buf = byte_buf;
> +       }
> +
> +       return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t pio_write(struct qcom_qspi *ctrl)
> +{
> +       const void *xfer_buf = ctrl->xfer.tx_buf;
> +       const int *word_buf;
> +       const char *byte_buf;
> +       unsigned int wr_fifo_bytes;
> +       unsigned int wr_fifo_words;
> +       unsigned int wr_size;
> +       unsigned int rem_words;
> +
> +       wr_fifo_bytes = readl(ctrl->base + pio_xfer_status)
> +                               >> wr_fifo_bytes_shft;

Just write this as:

       wr_fifo_bytes = readl(ctrl->base + pio_xfer_status);
       wr_fifo_bytes >>= WR_FIFO_BYTES_SHFT;

and is that supposed to be uppercase but it's lower case? Or did I
somehow mess that up when replying?

> +
> +       if (ctrl->xfer.rem_bytes < QSPI_BYTES_PER_WORD) {
> +               /* Process the last 1-3 bytes */
> +               wr_size = min(wr_fifo_bytes, ctrl->xfer.rem_bytes);
> +               ctrl->xfer.rem_bytes -= wr_size;
> +
> +               byte_buf = xfer_buf;
> +               while (wr_size--)
> +                       writel(*byte_buf++,
> +                              ctrl->base + PIO_DATAOUT_1B);
> +               ctrl->xfer.tx_buf = byte_buf;
> +       } else {
> +               /*
> +                * Process all the whole words; to keep things simple we'll
> +                * just wait for the next interrupt to handle the last 1-3
> +                * bytes if we don't have an even number of words.
> +                */
> +               rem_words = ctrl->xfer.rem_bytes / QSPI_BYTES_PER_WORD;
> +               wr_fifo_words = wr_fifo_bytes / QSPI_BYTES_PER_WORD;
> +
> +               wr_size = min(rem_words, wr_fifo_words);
> +               ctrl->xfer.rem_bytes -= wr_size * QSPI_BYTES_PER_WORD;
> +
> +               word_buf = xfer_buf;
> +               while (wr_size--)
> +                       writel(get_unaligned(word_buf++),
> +                              ctrl->base + PIO_DATAOUT_4B);

Is this a FIFO? Should use writesl() or iowrite32_rep() then when
throwing words at a time into the FIFO so that it works on any CPU
endianess for a little endian device.

> +               ctrl->xfer.tx_buf = word_buf;
> +
> +       }
> +
> +       return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t qcom_qspi_irq(int irq, void *dev_id)
> +{
> +       u32 int_status;
> +       struct qcom_qspi *ctrl = dev_id;
> +       irqreturn_t ret;
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&ctrl->lock, flags);
> +
> +       int_status = readl(ctrl->base + MSTR_INT_STATUS);
> +       writel(int_status, ctrl->base + MSTR_INT_STATUS);
> +
> +       if (ctrl->xfer.dir == QSPI_WRITE) {
> +               if (int_status & WR_FIFO_EMPTY)
> +                       ret = pio_write(ctrl);
> +       } else {
> +               if (int_status & RESP_FIFO_RDY)
> +                       ret = pio_read(ctrl);

What if int_status & RESP_FIFO_RDY isn't set? Then 'ret' is never
assigned? Should have ret = IRQ_NONE up above I guess.

> +       }

And should the error interrupt bits be checked and printed if they
happen? We seem to unmask them, but then we don't really do anything
with them if they happen.

> +
> +       if (!ctrl->xfer.rem_bytes) {
> +               writel(0, ctrl->base + MSTR_INT_EN);
> +               spi_finalize_current_transfer(dev_get_drvdata(ctrl->dev));
> +       }
> +
> +       spin_unlock_irqrestore(&ctrl->lock, flags);
> +       return ret;
> +}
> +
> +static int qcom_qspi_probe(struct platform_device *pdev)
> +{
> +       int ret = 0;

Should be able to drop the assignment here. Hopefully compiler doesn't
complain.

> +       struct device *dev;
> +       struct resource *res;
> +       struct spi_master *master;
> +       struct qcom_qspi *ctrl;
> +
> +       dev = &pdev->dev;
> +
> +       master = spi_alloc_master(dev, sizeof(struct qcom_qspi));

sizeof(*ctrl) so we know what is being stored inside?

> +       if (!master) {
> +               dev_err(dev, "Failed to alloc spi master\n");

We don't need allocation error messages. Just return -ENOMEM here.

> +               return -ENOMEM;
> +       }
> +       platform_set_drvdata(pdev, master);
> +
> +       ctrl = spi_master_get_devdata(master);
> +
> +       spin_lock_init(&ctrl->lock);
> +       ctrl->dev = dev;
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       ctrl->base = devm_ioremap(dev, res->start, resource_size(res));
> +       if (IS_ERR(ctrl->base)) {
> +               ret = PTR_ERR(ctrl->base);
> +               dev_err(dev, "Failed to get base addr %d\n", ret);

Use devm_ioremap_resource()? And then drop this error print?

> +               goto exit_probe_master_put;
> +       }
> +
> +       ctrl->clks[QSPI_CLK_CORE].id = "core";
> +       ctrl->clks[QSPI_CLK_IFACE].id = "iface";
> +       ret = devm_clk_bulk_get(dev, QSPI_NUM_CLKS, ctrl->clks);
> +       if (ret)
> +               goto exit_probe_master_put;
> +
> +       ret = platform_get_irq(pdev, 0);
> +       if (ret < 0) {
> +               dev_err(dev, "Failed to get irq %d\n", ret);
> +               goto exit_probe_master_put;
> +       }
> +       ret = devm_request_irq(dev, ret, qcom_qspi_irq,
> +                       IRQF_TRIGGER_HIGH, dev_name(dev), ctrl);
> +       if (ret) {
> +               dev_err(dev, "Failed to request irq %d\n", ret);
> +               goto exit_probe_master_put;
> +       }
> +
> +       master->max_speed_hz = 300000000;
> +       master->num_chipselect = QSPI_NUM_CS;
> +       master->bus_num = pdev->id;

Can this come from DT aliases? I've never thought about qspi and
"regular" spi being in the same spi bus numbering system, but I suppose
that will happen now and we need to make sure that qspi numbers start
after the regular ones?

> +       master->dev.of_node = pdev->dev.of_node;
> +       master->mode_bits = SPI_MODE_0 |
> +                           SPI_TX_DUAL | SPI_RX_DUAL |
> +                           SPI_TX_QUAD | SPI_RX_QUAD;
> +       master->flags = SPI_MASTER_HALF_DUPLEX;
> +       master->prepare_message = qcom_qspi_prepare_message;
> +       master->transfer_one = qcom_qspi_transfer_one;
> +       master->handle_err = qcom_qspi_handle_err;
> +       master->auto_runtime_pm = true;
> +
> +       pm_runtime_enable(dev);
> +
> +       ret = spi_register_master(master);
> +       if (ret)
> +               goto exit_probe_runtime_disable;
> +
> +       return 0;

Or 

	if (!ret)
		return 0;
	
> +
> +exit_probe_runtime_disable:

And then drop this label.

> +       pm_runtime_disable(dev);
> +
> +exit_probe_master_put:
> +       spi_master_put(master);
> +
> +       return ret;
> +}




[Index of Archives]     [Linux Kernel]     [Linux ARM (vger)]     [Linux ARM MSM]     [Linux Omap]     [Linux Arm]     [Linux Tegra]     [Fedora ARM]     [Linux for Samsung SOC]     [eCos]     [Linux Fastboot]     [Gcc Help]     [Git]     [DCCP]     [IETF Announce]     [Security]     [Linux MIPS]     [Yosemite Campsites]

  Powered by Linux