Re: [PATCH V4 3/4] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP

[dkota@codeaurora.org]

[...]
>> +#define TIMESTAMP_BEFORE       BIT(1)
>> +#define FRAGMENTATION          BIT(2)
>> +#define TIMESTAMP_AFTER                BIT(3)
>> +#define POST_CMD_DELAY         BIT(4)
>> +
>> +static irqreturn_t geni_spi_isr(int irq, void *data);
> 
> Does this need to be forward declared?
Not required, will remove it.
> 
>> +
>> +struct spi_geni_master {
>> +       struct geni_se se;
>> +       struct device *dev;
>> +       u32 rx_fifo_depth;
> 
> Is this used?
It can be removed.
> 
>> +       u32 tx_fifo_depth;
>> +       u32 fifo_width_bits;
>> +       u32 tx_wm;
>> +       unsigned int cur_speed_hz;
> 
> unsigned long for Hz? The clk framework uses that type.

cur_speed_hz stores the speed value requested as part of transfer (not 
the resultant or rounded off frequency got from clk framework. It is u32 
type, i will change cur_speed_hz to u32 type instead of unsigned long.
Code snippet:
         mas->cur_speed_hz = xfer->speed_hz;

> 
>> +       unsigned int cur_bits_per_word;
>> +       unsigned int tx_rem_bytes;
>> +       unsigned int rx_rem_bytes;
>> +       struct spi_transfer *cur_xfer;
> 
> const?
Yes can be marked as const, will update it.
> 
>> +       struct completion xfer_done;
>> +       unsigned int oversampling;
>> +       spinlock_t lock;
>> +};
>> +
> [...]
>> +
>> +static int spi_geni_prepare_message(struct spi_master *spi,
>> +                                       struct spi_message *spi_msg)
>> +{
>> +       int ret;
>> +       struct spi_geni_master *mas = spi_master_get_devdata(spi);
>> +       struct geni_se *se = &mas->se;
>> +
>> +       geni_se_select_mode(se, GENI_SE_FIFO);
>> +       reinit_completion(&mas->xfer_done);
>> +       ret = setup_fifo_params(spi_msg->spi, spi);
>> +       if (ret)
>> +               dev_err(mas->dev, "Couldn't select mode %d", ret);
> 
> Missing newline on error print.
> 
Will add it.
>> +       return ret;
>> +}
>> +
> [...]
>> +
>> +static void setup_fifo_xfer(struct spi_transfer *xfer,
>> +                               struct spi_geni_master *mas,
>> +                               u16 mode, struct spi_master *spi)
>> +{
>> +       u32 m_cmd = 0, m_param = 0;
>> +       u32 spi_tx_cfg, trans_len;
>> +       struct geni_se *se = &mas->se;
>> +
>> +       spi_tx_cfg = readl(se->base + SE_SPI_TRANS_CFG);
>> +       if (xfer->bits_per_word != mas->cur_bits_per_word) {
>> +               spi_setup_word_len(mas, mode, xfer->bits_per_word);
>> +               mas->cur_bits_per_word = xfer->bits_per_word;
>> +       }
>> +
>> +       /* Speed and bits per word can be overridden per transfer */
>> +       if (xfer->speed_hz != mas->cur_speed_hz) {
>> +               int ret;
>> +               u32 clk_sel, m_clk_cfg;
>> +               unsigned int idx, div;
>> +
>> +               ret = get_spi_clk_cfg(xfer->speed_hz, mas, &idx, 
>> &div);
>> +               if (ret) {
>> +                       dev_err(mas->dev, "Err setting clks:%d\n", 
>> ret);
>> +                       return;
>> +               }
>> +               /*
>> +                * SPI core clock gets configured with the requested 
>> frequency
>> +                * or the frequency closer to the requested frequency.
>> +                * For that reason requested frequency is stored in 
>> the
>> +                * cur_speed_hz and referred in the consicutive 
>> transfer instead
> 
> s/consicutive/consecutive/
will correct it.
> 
>> +                * of calling clk_get_rate() API.
>> +                */
>> +               mas->cur_speed_hz = xfer->speed_hz;
>> +               clk_sel = idx & CLK_SEL_MSK;
>> +               m_clk_cfg = (div << CLK_DIV_SHFT) | SER_CLK_EN;
>> +               writel(clk_sel, se->base + SE_GENI_CLK_SEL);
>> +               writel(m_clk_cfg, se->base + GENI_SER_M_CLK_CFG);
>> +       }
>> +
>> +       mas->tx_rem_bytes = 0;
>> +       mas->rx_rem_bytes = 0;
>> +       if (xfer->tx_buf && xfer->rx_buf)
>> +               m_cmd = SPI_FULL_DUPLEX;
>> +       else if (xfer->tx_buf)
>> +               m_cmd = SPI_TX_ONLY;
>> +       else if (xfer->rx_buf)
>> +               m_cmd = SPI_RX_ONLY;
>> +
>> +       spi_tx_cfg &= ~CS_TOGGLE;
>> +       if (!(mas->cur_bits_per_word % MIN_WORD_LEN)) {
>> +               trans_len =
>> +                       (xfer->len * BITS_PER_BYTE /
>> +                                       mas->cur_bits_per_word) & 
>> TRANS_LEN_MSK;
>> +       } else {
>> +               unsigned int bytes_per_word =
>> +                       mas->cur_bits_per_word / BITS_PER_BYTE + 1;
>> +
>> +               trans_len = (xfer->len / bytes_per_word) & 
>> TRANS_LEN_MSK;
>> +       }
> 
> Rename 'trans_len' to 'len' and shorten some lines by taking out the
> mask to get:
> 
>        if (!(mas->cur_bits_per_word % MIN_WORD_LEN))
>                len = xfer->len * BITS_PER_BYTE / mas->cur_bits_per_word
>        else
>                len = xfer->len / (mas->cur_bits_per_word / 
> BITS_PER_BYTE + 1);
>        len &= TRANS_LEN_MSK;
> 
OK
>> +
>> +       /*
>> +        * If CS change flag is set, then toggle the CS line in 
>> between
>> +        * transfers and keep CS asserted after the last transfer.
>> +        * Else if keep CS flag asserted in between transfers and 
>> de-assert
>> +        * CS after the last message.
>> +        */
>> +       if (xfer->cs_change) {
>> +               if (list_is_last(&xfer->transfer_list,
>> +                               &spi->cur_msg->transfers))
>> +                       m_param = FRAGMENTATION;
>> +       } else {
>> +               if (!list_is_last(&xfer->transfer_list,
>> +                               &spi->cur_msg->transfers))
>> +                       m_param = FRAGMENTATION;
>> +       }
>> +
>> +       mas->cur_xfer = xfer;
>> +       if (m_cmd & SPI_TX_ONLY) {
>> +               mas->tx_rem_bytes = xfer->len;
>> +               writel(trans_len, se->base + SE_SPI_TX_TRANS_LEN);
>> +       }
>> +
>> +       if (m_cmd & SPI_RX_ONLY) {
>> +               writel(trans_len, se->base + SE_SPI_RX_TRANS_LEN);
>> +               mas->rx_rem_bytes = xfer->len;
>> +       }
>> +       writel(spi_tx_cfg, se->base + SE_SPI_TRANS_CFG);
>> +       geni_se_setup_m_cmd(se, m_cmd, m_param);
>> +
>> +       /*
>> +        * TX_WATERMARK_REG should be set after SPI configuration and
>> +        * setting up GENI SE engine, as driver starts data transfer
>> +        * for the watermark interrupt.
>> +        */
>> +       if (m_cmd & SPI_TX_ONLY)
>> +               writel(mas->tx_wm, se->base + 
>> SE_GENI_TX_WATERMARK_REG);
>> +}
>> +
>> +static void handle_fifo_timeout(struct spi_master *spi,
>> +                               struct spi_message *msg)
>> +{
>> +       struct spi_geni_master *mas = spi_master_get_devdata(spi);
>> +       unsigned long timeout, flags;
>> +       struct geni_se *se = &mas->se;
>> +
>> +       spin_lock_irqsave(&mas->lock, flags);
>> +       reinit_completion(&mas->xfer_done);
>> +       geni_se_cancel_m_cmd(se);
>> +       writel(0, se->base + SE_GENI_TX_WATERMARK_REG);
>> +       spin_unlock_irqrestore(&mas->lock, flags);
>> +       timeout = wait_for_completion_timeout(&mas->xfer_done, HZ);
> 
> Can you rename this time_left or completed? Then the if condition reads
> properly as "if not time left" or "if not completed". And then invert
> that logic so things aren't so indented?
> 
> 	time_left = wait_for_completion_timeout(..)
> 	if (time_left)
> 		return;
> 
> 	spin_lock_irqsave(&mas->lock, flags);
> 	reinit_completion(..)
> 	...
> 
Ok
>> +       if (!timeout) {
>> +               spin_lock_irqsave(&mas->lock, flags);
>> +               reinit_completion(&mas->xfer_done);
>> +               geni_se_abort_m_cmd(se);
>> +               spin_unlock_irqrestore(&mas->lock, flags);
>> +               timeout = wait_for_completion_timeout(&mas->xfer_done,
>> +                                                               HZ);
>> +               if (!timeout)
>> +                       dev_err(mas->dev,
>> +                               "Failed to cancel/abort m_cmd\n");
>> +       }
>> +}
>> +
>> +static int spi_geni_transfer_one(struct spi_master *spi,
>> +                               struct spi_device *slv,
>> +                               struct spi_transfer *xfer)
>> +{
>> +       struct spi_geni_master *mas = spi_master_get_devdata(spi);
>> +
>> +       setup_fifo_xfer(xfer, mas, slv->mode, spi);
>> +       return 1;
>> +}
>> +
>> +static unsigned int geni_byte_per_fifo_word(struct spi_geni_master 
>> *mas)
>> +{
>> +       /*
>> +        * Calculate how many bytes we'll put in each FIFO word.  If 
>> the
>> +        * transfer words don't pack cleanly into a FIFO word we'll 
>> just put
>> +        * one transfer word in each FIFO word.  If they do pack we'll 
>> pack 'em.
>> +        */
>> +       if (mas->fifo_width_bits % mas->cur_bits_per_word)
>> +               return 
>> roundup_pow_of_two(DIV_ROUND_UP(mas->cur_bits_per_word,
>> +                                                      
>> BITS_PER_BYTE));
>> +       else
>> +               return mas->fifo_width_bits / BITS_PER_BYTE;
> 
> Just do a return. No else please.
> 
Ok
>> +}
>> +
>> +static irqreturn_t geni_spi_handle_tx(struct spi_geni_master *mas)
>> +{
>> +       struct geni_se *se = &mas->se;
>> +       unsigned int max_bytes;
>> +       const u8 *tx_buf;
>> +       unsigned int bytes_per_fifo_word = 
>> geni_byte_per_fifo_word(mas);
>> +       unsigned int i = 0;
>> +
>> +       if (!mas->cur_xfer)
>> +               return IRQ_NONE;
>> +
>> +       max_bytes = (mas->tx_fifo_depth - mas->tx_wm) * 
>> bytes_per_fifo_word;
>> +       if (mas->tx_rem_bytes < max_bytes)
>> +               max_bytes = mas->tx_rem_bytes;
>> +
>> +       tx_buf = mas->cur_xfer->tx_buf + mas->cur_xfer->len - 
>> mas->tx_rem_bytes;
>> +       while (i < max_bytes) {
>> +               unsigned int j;
>> +               unsigned int bytes_to_write;
>> +               u32 fifo_word = 0;
>> +               u8 *fifo_byte = (u8 *)&fifo_word;
>> +
>> +               bytes_to_write = min(bytes_per_fifo_word, max_bytes - 
>> i);
>> +               for (j = 0; j < bytes_to_write; j++)
>> +                       fifo_byte[j] = tx_buf[i++];
>> +               iowrite32_rep(se->base + SE_GENI_TX_FIFOn, &fifo_word, 
>> 1);
>> +       }
>> +       mas->tx_rem_bytes -= max_bytes;
>> +       if (!mas->tx_rem_bytes)
>> +               writel(0, se->base + SE_GENI_TX_WATERMARK_REG);
>> +
>> +       return IRQ_HANDLED;
>> +}
>> +
>> +static irqreturn_t geni_spi_handle_rx(struct spi_geni_master *mas)
>> +{
>> +       struct geni_se *se = &mas->se;
>> +       u32 rx_fifo_status;
>> +       unsigned int rx_bytes;
>> +       u8 *rx_buf;
>> +       unsigned int bytes_per_fifo_word = 
>> geni_byte_per_fifo_word(mas);
>> +       unsigned int i = 0;
>> +
>> +       if (!mas->cur_xfer)
>> +               return IRQ_NONE;
>> +
>> +       rx_fifo_status = readl(se->base + SE_GENI_RX_FIFO_STATUS);
> 
> Rename to 'status'? rx_fifo is implicit in the register macro.
Declared as "rx_fifo_status" for code readability.

> 
>> +       rx_bytes = (rx_fifo_status & RX_FIFO_WC_MSK) * 
>> bytes_per_fifo_word;
>> +       if (rx_fifo_status & RX_LAST) {
>> +               unsigned int rx_last_byte_valid =
>> +                       (rx_fifo_status & RX_LAST_BYTE_VALID_MSK)
>> +                               >> RX_LAST_BYTE_VALID_SHFT;
> 
> Hoist this variable into function scope? So then the line is:
> 
> 		last_valid = status & RX_LAST_BYTE_VALID_MSK;
> 		last_valid >>= RX_LAST_BYTE_VALID_SHFT;
> 
Ok
>> +               if (rx_last_byte_valid && (rx_last_byte_valid < 4))
> 
> Drop useless parenthesis please.
> 
Ok
>> +                       rx_bytes -= bytes_per_fifo_word - 
>> rx_last_byte_valid;
>> +       }
>> +       if (mas->rx_rem_bytes < rx_bytes)
>> +               rx_bytes = mas->rx_rem_bytes;
>> +
>> +       rx_buf = mas->cur_xfer->rx_buf + mas->cur_xfer->len - 
>> mas->rx_rem_bytes;
>> +       while (i < rx_bytes) {
>> +               u32 fifo_word = 0;
>> +               u8 *fifo_byte = (u8 *)&fifo_word;
>> +               unsigned int bytes_to_read;
>> +               unsigned int j;
>> +
>> +               bytes_to_read = min(bytes_per_fifo_word, rx_bytes - 
>> i);
>> +               ioread32_rep(se->base + SE_GENI_RX_FIFOn, &fifo_word, 
>> 1);
>> +               for (j = 0; j < bytes_to_read; j++)
>> +                       rx_buf[i++] = fifo_byte[j];
>> +       }
>> +       mas->rx_rem_bytes -= rx_bytes;
>> +
>> +       return IRQ_HANDLED;
>> +}
>> +
>> +static irqreturn_t geni_spi_isr(int irq, void *data)
>> +{
>> +       struct spi_master *spi = data;
>> +       struct spi_geni_master *mas = spi_master_get_devdata(spi);
>> +       struct geni_se *se = &mas->se;
>> +       u32 m_irq;
>> +       unsigned long flags;
>> +       irqreturn_t ret = IRQ_HANDLED;
>> +
>> +       if (pm_runtime_status_suspended(mas->dev))
>> +               return IRQ_NONE;
>> +
>> +       spin_lock_irqsave(&mas->lock, flags);
>> +       m_irq = readl(se->base + SE_GENI_M_IRQ_STATUS);
>> +       if ((m_irq & M_RX_FIFO_WATERMARK_EN) || (m_irq & 
>> M_RX_FIFO_LAST_EN))
>> +               ret = geni_spi_handle_rx(mas);
>> +
>> +       if (m_irq & M_TX_FIFO_WATERMARK_EN)
>> +               ret = geni_spi_handle_tx(mas);
> 
> Is it possible for all three conditions above to happen in one
> interrupt? I ask because 'ret' is overwritten and so what may have been
> IRQ_HANDLED may become IRQ_NONE which will lead to confusion in the irq
> layer. Maybe the handle tx/rx functions can return a bool, that gets
> orred together each time so that we know if something handled an
> interrupt?
> 
Will check it again by running fullduplex transfer and update.
>> +
>> +       if (m_irq & M_CMD_DONE_EN) {
>> +               spi_finalize_current_transfer(spi);
>> +               /*
>> +                * If this happens, then a CMD_DONE came before all 
>> the Tx
>> +                * buffer bytes were sent out. This is unusual, log 
>> this
>> +                * condition and disable the WM interrupt to prevent 
>> the
>> +                * system from stalling due an interrupt storm.
>> +                * If this happens when all Rx bytes haven't been 
>> received, log
>> +                * the condition.
>> +                * The only known time this can happen is if 
>> bits_per_word != 8
>> +                * and some registers that expect xfer lengths in num 
>> spi_words
>> +                * weren't written correctly.
>> +                */
>> +               if (mas->tx_rem_bytes) {
>> +                       writel(0, se->base + 
>> SE_GENI_TX_WATERMARK_REG);
>> +                       dev_err(mas->dev, "Premature Done.tx_rem%d 
>> bpw%d\n",
> 
> Why isn't there a space after "Done."? And why is 'done' capitalized?
> Should 'tx_rem' be 'tx_rem='?
Yes, will update.
> 
>> +                               mas->tx_rem_bytes, 
>> mas->cur_bits_per_word);
>> +               }
>> +               if (mas->rx_rem_bytes)
>> +                       dev_err(mas->dev, "Premature Done.rx_rem%d 
>> bpw%d\n",
>> +                               mas->rx_rem_bytes, 
>> mas->cur_bits_per_word);
>> +       }
>> +
>> +       if ((m_irq & M_CMD_CANCEL_EN) || (m_irq & M_CMD_ABORT_EN))
>> +               complete(&mas->xfer_done);
>> +
>> +       writel(m_irq, se->base + SE_GENI_M_IRQ_CLEAR);
>> +       spin_unlock_irqrestore(&mas->lock, flags);
>> +       return ret;
>> +}
>> +
>> +static int spi_geni_probe(struct platform_device *pdev)
>> +{
>> +       int ret;
>> +       struct spi_master *spi;
>> +       struct spi_geni_master *mas;
>> +       struct resource *res;
>> +       struct geni_se *se;
>> +
>> +       spi = spi_alloc_master(&pdev->dev, sizeof(struct 
>> spi_geni_master));
> 
> sizeof(*mas) for code clarity?
> 
Ok
>> +       if (!spi)
>> +               return -ENOMEM;
>> +
>> +       platform_set_drvdata(pdev, spi);
>> +       mas = spi_master_get_devdata(spi);
>> +       mas->dev = &pdev->dev;
>> +       mas->se.dev = &pdev->dev;
>> +       mas->se.wrapper = dev_get_drvdata(pdev->dev.parent);
>> +       se = &mas->se;
>> +
>> +       spi->bus_num = -1;
>> +       spi->dev.of_node = pdev->dev.of_node;
>> +       mas->se.clk = devm_clk_get(&pdev->dev, "se");
>> +       if (IS_ERR(mas->se.clk)) {
>> +               ret = PTR_ERR(mas->se.clk);
>> +               dev_err(&pdev->dev, "Err getting SE Core clk %d\n", 
>> ret);
>> +               goto spi_geni_probe_err;
>> +       }
>> +
>> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +       se->base = devm_ioremap_resource(&pdev->dev, res);
>> +       if (IS_ERR(se->base)) {
>> +               ret = -ENOMEM;
> 
> 		ret = PTR_ERR(se->base);
> 
> so we don't lose the error value.
> 
Ok
>> +               goto spi_geni_probe_err;
>> +       }
>> +
>> +       ret = platform_get_irq(pdev, 0);
>> +       if (ret < 0) {
>> +               dev_err(&pdev->dev, "Err getting IRQ %d\n", ret);
>> +               goto spi_geni_probe_err;
>> +       }
>> +       ret = devm_request_irq(&pdev->dev, ret, geni_spi_isr,
>> +                               IRQF_TRIGGER_HIGH, "spi_geni", spi);
>> +       if (ret)
>> +               goto spi_geni_probe_err;
> 
> Can you request this irq as late as possible in the probe function? I
> worry there may be some pending irq line set and then this will cause 
> an
> interrupt storm with IRQ_NONE returned because the device is runtime
> suspended.
> 
Ok; will call it after spi_register_master()
>> +
>> +       spi->mode_bits = SPI_CPOL | SPI_CPHA | SPI_LOOP | SPI_CS_HIGH;
>> +       spi->bits_per_word_mask = SPI_BPW_RANGE_MASK(4, 32);
>> +       spi->num_chipselect = 4;
>> +       spi->max_speed_hz = 50000000;
>> +       spi->prepare_message = spi_geni_prepare_message;
>> +       spi->transfer_one = spi_geni_transfer_one;
>> +       spi->auto_runtime_pm = true;
>> +       spi->handle_err = handle_fifo_timeout;
>> +
>> +       init_completion(&mas->xfer_done);
>> +       spin_lock_init(&mas->lock);
>> +       pm_runtime_enable(&pdev->dev);
>> +
>> +       ret = spi_geni_init(mas);
>> +       if (ret)
>> +               goto spi_geni_probe_runtime_disable;
>> +
>> +       ret = spi_register_master(spi);
>> +       if (ret)
>> +               goto spi_geni_probe_runtime_disable;
>> +
>> +       return 0;
>> +spi_geni_probe_runtime_disable:
>> +       pm_runtime_disable(&pdev->dev);
>> +spi_geni_probe_err:
>> +       spi_master_put(spi);
>> +       return ret;
>> +}
>> +
>> +static int spi_geni_remove(struct platform_device *pdev)
>> +{
>> +       struct spi_master *spi = platform_get_drvdata(pdev);
>> +
>> +       /* Unregister _before_ disabling pm_runtime() so we stop 
>> transfers */
>> +       spi_unregister_master(spi);
>> +
>> +       pm_runtime_disable(&pdev->dev);
>> +       return 0;
>> +}
>> +
>> +static int __maybe_unused spi_geni_runtime_suspend(struct device 
>> *dev)
>> +{
>> +       struct spi_master *spi = dev_get_drvdata(dev);
>> +       struct spi_geni_master *mas = spi_master_get_devdata(spi);
>> +
>> +       return geni_se_resources_off(&mas->se);
>> +}
>> +
>> +static int __maybe_unused spi_geni_runtime_resume(struct device *dev)
>> +{
>> +       struct spi_master *spi = dev_get_drvdata(dev);
>> +       struct spi_geni_master *mas = spi_master_get_devdata(spi);
>> +
>> +       return geni_se_resources_on(&mas->se);
>> +}
>> +
>> +static int __maybe_unused spi_geni_suspend(struct device *dev)
>> +{
>> +       if (!pm_runtime_status_suspended(dev))
>> +               return -EBUSY;
>> +       return 0;
>> +}
>> +
>> +static const struct dev_pm_ops spi_geni_pm_ops = {
>> +       SET_RUNTIME_PM_OPS(spi_geni_runtime_suspend,
>> +                                       spi_geni_runtime_resume, NULL)
>> +       SET_SYSTEM_SLEEP_PM_OPS(spi_geni_suspend, NULL)
>> +};
>> +
>> +static const struct of_device_id spi_geni_dt_match[] = {
>> +       { .compatible = "qcom,geni-spi" },
>> +       {}
>> +};
> 
> Please add a MODULE_DEVICE_TABLE(of, spi_geni_dt_match) here.
> 
Ok
>> +
>> +static struct platform_driver spi_geni_driver = {
>> +       .probe  = spi_geni_probe,
>> +       .remove = spi_geni_remove,
>> +       .driver = {
>> +               .name = "geni_spi",
>> +               .pm = &spi_geni_pm_ops,
>> +               .of_match_table = spi_geni_dt_match,
>> +       },
>> +};
>> +module_platform_driver(spi_geni_driver);
>> +
>> +MODULE_DESCRIPTION("SPI driver for GENI based QUP cores");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/include/linux/spi/spi-geni-qcom.h 
>> b/include/linux/spi/spi-geni-qcom.h
>> new file mode 100644
>> index 0000000..dc95764
>> --- /dev/null
>> +++ b/include/linux/spi/spi-geni-qcom.h
>> @@ -0,0 +1,14 @@
>> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>> +/*
>> + * Copyright (c) 2017-2018, The Linux Foundation. All rights 
>> reserved.
>> + */
>> +
>> +#ifndef __SPI_GENI_QCOM_HEADER___
>> +#define __SPI_GENI_QCOM_HEADER___
>> +
>> +struct spi_geni_qcom_ctrl_data {
>> +       u32 spi_cs_clk_delay;
>> +       u32 spi_inter_words_delay;
> 
> It was decided we still needed this? I don't see it in v3 so please
> remove this file unless it's needed.
Its not required; I will remove it.

Along with these changes, i will submit the fixes of loopback issue and 
suspend/resume issue.

--Dilip