On 1/17/2018 10:23 PM, Bjorn Andersson wrote:
On Fri 12 Jan 17:05 PST 2018, Karthikeyan Ramasubramanian wrote:
This bus driver supports the GENI based i2c hardware controller in the
Qualcomm SOCs. The Qualcomm Generic Interface (GENI) is a programmable
module supporting a wide range of serial interfaces including I2C. The
driver supports FIFO mode and DMA mode of transfer and switches modes
dynamically depending on the size of the transfer.
Signed-off-by: Karthikeyan Ramasubramanian <kramasub@xxxxxxxxxxxxxx>
Signed-off-by: Sagar Dharia <sdharia@xxxxxxxxxxxxxx>
Signed-off-by: Girish Mahadevan <girishm@xxxxxxxxxxxxxx>
---
drivers/i2c/busses/Kconfig | 10 +
drivers/i2c/busses/Makefile | 1 +
drivers/i2c/busses/i2c-qcom-geni.c | 656 +++++++++++++++++++++++++++++++++++++
3 files changed, 667 insertions(+)
create mode 100644 drivers/i2c/busses/i2c-qcom-geni.c
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 009345d..caef309 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -838,6 +838,16 @@ config I2C_PXA_SLAVE
is necessary for systems where the PXA may be a target on the
I2C bus.
+config I2C_QCOM_GENI
+ tristate "Qualcomm Technologies Inc.'s GENI based I2C controller"
+ depends on ARCH_QCOM
Just depend on the GENI wrapper as well.
Ok.
+ help
+ If you say yes to this option, support will be included for the
+ built-in I2C interface on the Qualcomm Technologies Inc.'s SoCs.
+
+ This driver can also be built as a module. If so, the module
+ will be called i2c-qcom-geni.
+
config I2C_QUP
tristate "Qualcomm QUP based I2C controller"
depends on ARCH_QCOM
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 2ce8576..201fce1 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -84,6 +84,7 @@ obj-$(CONFIG_I2C_PNX) += i2c-pnx.o
obj-$(CONFIG_I2C_PUV3) += i2c-puv3.o
obj-$(CONFIG_I2C_PXA) += i2c-pxa.o
obj-$(CONFIG_I2C_PXA_PCI) += i2c-pxa-pci.o
+obj-$(CONFIG_I2C_QCOM_GENI) += i2c-qcom-geni.o
obj-$(CONFIG_I2C_QUP) += i2c-qup.o
obj-$(CONFIG_I2C_RIIC) += i2c-riic.o
obj-$(CONFIG_I2C_RK3X) += i2c-rk3x.o
diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
new file mode 100644
index 0000000..59ad4da
--- /dev/null
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -0,0 +1,656 @@
+/*
+ * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * 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.
+ *
+ */
Use SPDX license header.
Ok.
+
+#include <linux/clk.h>
+#include <linux/delay.h>
Unused?
Ok, I will remove unused header files.
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/dma-mapping.h>
+#include <linux/qcom-geni-se.h>
+
+#define SE_I2C_TX_TRANS_LEN (0x26C)
Drop the parenthesis, when not needed.
Ok.
+#define SE_I2C_RX_TRANS_LEN (0x270)
+#define SE_I2C_SCL_COUNTERS (0x278)
+#define SE_GENI_IOS (0x908)
+
+#define SE_I2C_ERR (M_CMD_OVERRUN_EN | M_ILLEGAL_CMD_EN | M_CMD_FAILURE_EN |\
+ M_GP_IRQ_1_EN | M_GP_IRQ_3_EN | M_GP_IRQ_4_EN)
+#define SE_I2C_ABORT (1U << 1)
+/* M_CMD OP codes for I2C */
+#define I2C_WRITE (0x1)
+#define I2C_READ (0x2)
+#define I2C_WRITE_READ (0x3)
+#define I2C_ADDR_ONLY (0x4)
+#define I2C_BUS_CLEAR (0x6)
+#define I2C_STOP_ON_BUS (0x7)
+/* M_CMD params for I2C */
+#define PRE_CMD_DELAY (BIT(0))
+#define TIMESTAMP_BEFORE (BIT(1))
+#define STOP_STRETCH (BIT(2))
+#define TIMESTAMP_AFTER (BIT(3))
+#define POST_COMMAND_DELAY (BIT(4))
+#define IGNORE_ADD_NACK (BIT(6))
+#define READ_FINISHED_WITH_ACK (BIT(7))
+#define BYPASS_ADDR_PHASE (BIT(8))
+#define SLV_ADDR_MSK (GENMASK(15, 9))
+#define SLV_ADDR_SHFT (9)
+
+#define I2C_CORE2X_VOTE (10000)
+#define GP_IRQ0 0
+#define GP_IRQ1 1
+#define GP_IRQ2 2
+#define GP_IRQ3 3
+#define GP_IRQ4 4
+#define GP_IRQ5 5
+#define GENI_OVERRUN 6
+#define GENI_ILLEGAL_CMD 7
+#define GENI_ABORT_DONE 8
+#define GENI_TIMEOUT 9
+
+#define I2C_NACK GP_IRQ1
+#define I2C_BUS_PROTO GP_IRQ3
+#define I2C_ARB_LOST GP_IRQ4
+#define DM_I2C_CB_ERR ((BIT(GP_IRQ1) | BIT(GP_IRQ3) | BIT(GP_IRQ4)) \
+ << 5)
+
+#define I2C_AUTO_SUSPEND_DELAY 250
+#define KHz(freq) (1000 * freq)
+
+struct geni_i2c_dev {
+ struct device *dev;
+ void __iomem *base;
+ unsigned int tx_wm;
+ int irq;
+ int err;
+ struct i2c_adapter adap;
+ struct completion xfer;
How about naming this "done" or something like that, gi2c->xfer doesn't
really give the sense of being a "we're done with the operation"-event.
Ok.
+ struct i2c_msg *cur;
+ struct geni_se_rsc i2c_rsc;
+ int cur_wr;
+ int cur_rd;
+ struct device *wrapper_dev;
This is already availabe in i2c_rsc, and in particular if you pass the
i2c_rsc down to the wrapper in the 2 cases you use the wrapper_dev you
don't need this duplication.
Ok.
+ u32 clk_freq_out;
+ int clk_fld_idx;
Keep track of the const struct geni_i2c_clk_fld * here instead.
Ok.
+};
+
+struct geni_i2c_err_log {
+ int err;
+ const char *msg;
+};
+
+static struct geni_i2c_err_log gi2c_log[] = {
+ [GP_IRQ0] = {-EINVAL, "Unknown I2C err GP_IRQ0"},
+ [I2C_NACK] = {-ENOTCONN,
+ "NACK: slv unresponsive, check its power/reset-ln"},
Break the 80-char rule, to improve readability.
Ok.
+ [GP_IRQ2] = {-EINVAL, "Unknown I2C err GP IRQ2"},
+ [I2C_BUS_PROTO] = {-EPROTO,
+ "Bus proto err, noisy/unepxected start/stop"},
+ [I2C_ARB_LOST] = {-EBUSY,
+ "Bus arbitration lost, clock line undriveable"},
+ [GP_IRQ5] = {-EINVAL, "Unknown I2C err GP IRQ5"},
+ [GENI_OVERRUN] = {-EIO, "Cmd overrun, check GENI cmd-state machine"},
+ [GENI_ILLEGAL_CMD] = {-EILSEQ,
+ "Illegal cmd, check GENI cmd-state machine"},
+ [GENI_ABORT_DONE] = {-ETIMEDOUT, "Abort after timeout successful"},
+ [GENI_TIMEOUT] = {-ETIMEDOUT, "I2C TXN timed out"},
+};
+
+struct geni_i2c_clk_fld {
+ u32 clk_freq_out;
+ u8 clk_div;
+ u8 t_high;
+ u8 t_low;
+ u8 t_cycle;
+};
+
+static struct geni_i2c_clk_fld geni_i2c_clk_map[] = {
const
Ok.
+ {KHz(100), 7, 10, 11, 26},
+ {KHz(400), 2, 5, 12, 24},
+ {KHz(1000), 1, 3, 9, 18},
+};
+
+static int geni_i2c_clk_map_idx(struct geni_i2c_dev *gi2c)
+{
+ int i;
+ int ret = 0;
+ bool clk_map_present = false;
+ struct geni_i2c_clk_fld *itr = geni_i2c_clk_map;
+
+ for (i = 0; i < ARRAY_SIZE(geni_i2c_clk_map); i++, itr++) {
+ if (itr->clk_freq_out == gi2c->clk_freq_out) {
+ clk_map_present = true;
+ break;
Make this:
gi2c->clk_fld = geni_i2c_clk_map + i;
return 0;
Ok.
+ }
+ }
+
...then you can drop ret and clk_map_present and just return -EINVAL
here.
Ok.
+ if (clk_map_present)
+ gi2c->clk_fld_idx = i;
+ else
+ ret = -EINVAL;
+
+ return ret;
+}
+
+static inline void qcom_geni_i2c_conf(struct geni_i2c_dev *gi2c, int dfs)
Drop the "inline", if it makes sense the compiler will inline it, if not
it knows better than we do.
dfs is always 0, so drop this parameter and hard code the value below.
Ok.
+{
+ struct geni_i2c_clk_fld *itr = geni_i2c_clk_map + gi2c->clk_fld_idx;
+
+ geni_write_reg(dfs, gi2c->base, SE_GENI_CLK_SEL);
+
+ geni_write_reg((itr->clk_div << 4) | 1, gi2c->base, GENI_SER_M_CLK_CFG);
+ geni_write_reg(((itr->t_high << 20) | (itr->t_low << 10) |
+ itr->t_cycle), gi2c->base, SE_I2C_SCL_COUNTERS);
+
+ /* Ensure Clk config completes before return */
That's not what "mb" does, it ensures that later memory operations
aren't reordered beyond the barrier.
Our intention also is for ordering purposes so that any later register
writes/reads does not get re-ordered until the writes to clock
configuration register is complete. I will update the comment.
+ mb();
+}
+
+static void geni_i2c_err(struct geni_i2c_dev *gi2c, int err)
Looking at the code in this function it's just a bunch of logic to print
various debug messages...except for the last line that uses the gi2c_log
lookup table to convert the error to a errno. Sneaky...and not very
nice.
I will update this function so that it is more obvious.
+{
+ u32 m_cmd = readl_relaxed(gi2c->base + SE_GENI_M_CMD0);
Unless you have a really good reason, just use readl/writel in favour of
their _relaxed versions.
The goal is to maintain the order of execution to the Serial Engine
register block. I believe _relaxed help enusre that order with a little
less overhead compared to non_relaxed function. Please correct me otherwise.
+ u32 m_stat = readl_relaxed(gi2c->base + SE_GENI_M_IRQ_STATUS);
+ u32 geni_s = readl_relaxed(gi2c->base + SE_GENI_STATUS);
+ u32 geni_ios = readl_relaxed(gi2c->base + SE_GENI_IOS);
+ u32 dma = readl_relaxed(gi2c->base + SE_GENI_DMA_MODE_EN);
+ u32 rx_st, tx_st;
+
+ if (gi2c->cur)
+ dev_dbg(gi2c->dev, "len:%d, slv-addr:0x%x, RD/WR:%d\n",
+ gi2c->cur->len, gi2c->cur->addr, gi2c->cur->flags);
+
+ if (err == I2C_NACK || err == GENI_ABORT_DONE) {
+ dev_dbg(gi2c->dev, "%s\n", gi2c_log[err].msg);
+ goto err_ret;
+ } else {
+ dev_err(gi2c->dev, "%s\n", gi2c_log[err].msg);
+ }
+
+ if (dma) {
+ rx_st = readl_relaxed(gi2c->base + SE_DMA_RX_IRQ_STAT);
+ tx_st = readl_relaxed(gi2c->base + SE_DMA_TX_IRQ_STAT);
+ } else {
+ rx_st = readl_relaxed(gi2c->base + SE_GENI_RX_FIFO_STATUS);
+ tx_st = readl_relaxed(gi2c->base + SE_GENI_TX_FIFO_STATUS);
+ }
+ dev_dbg(gi2c->dev, "DMA:%d tx_stat:0x%x, rx_stat:0x%x, irq-stat:0x%x\n",
+ dma, tx_st, rx_st, m_stat);
+ dev_dbg(gi2c->dev, "m_cmd:0x%x, geni_status:0x%x, geni_ios:0x%x\n",
+ m_cmd, geni_s, geni_ios);
+err_ret:
+ gi2c->err = gi2c_log[err].err;
+}
+
+static irqreturn_t geni_i2c_irq(int irq, void *dev)
+{
+ struct geni_i2c_dev *gi2c = dev;
+ int i, j;
+ u32 m_stat = readl_relaxed(gi2c->base + SE_GENI_M_IRQ_STATUS);
+ u32 rx_st = readl_relaxed(gi2c->base + SE_GENI_RX_FIFO_STATUS);
+ u32 dm_tx_st = readl_relaxed(gi2c->base + SE_DMA_TX_IRQ_STAT);
+ u32 dm_rx_st = readl_relaxed(gi2c->base + SE_DMA_RX_IRQ_STAT);
+ u32 dma = readl_relaxed(gi2c->base + SE_GENI_DMA_MODE_EN);
+ struct i2c_msg *cur = gi2c->cur;
+
+ if (!cur || (m_stat & M_CMD_FAILURE_EN) ||
+ (dm_rx_st & (DM_I2C_CB_ERR)) ||
+ (m_stat & M_CMD_ABORT_EN)) {
+
+ if (m_stat & M_GP_IRQ_1_EN)
+ geni_i2c_err(gi2c, I2C_NACK);
+ if (m_stat & M_GP_IRQ_3_EN)
+ geni_i2c_err(gi2c, I2C_BUS_PROTO);
+ if (m_stat & M_GP_IRQ_4_EN)
+ geni_i2c_err(gi2c, I2C_ARB_LOST);
+ if (m_stat & M_CMD_OVERRUN_EN)
+ geni_i2c_err(gi2c, GENI_OVERRUN);
+ if (m_stat & M_ILLEGAL_CMD_EN)
+ geni_i2c_err(gi2c, GENI_ILLEGAL_CMD);
+ if (m_stat & M_CMD_ABORT_EN)
+ geni_i2c_err(gi2c, GENI_ABORT_DONE);
+ if (m_stat & M_GP_IRQ_0_EN)
+ geni_i2c_err(gi2c, GP_IRQ0);
+
+ if (!dma)
+ writel_relaxed(0, (gi2c->base +
+ SE_GENI_TX_WATERMARK_REG));
Drop the extra parenthesis. And using writel() instead keeps you under
80 chars.
Ok, to dropping the parenthesis.
+ goto irqret;
+ }
+
+ if (dma) {
+ dev_dbg(gi2c->dev, "i2c dma tx:0x%x, dma rx:0x%x\n", dm_tx_st,
+ dm_rx_st);
+ goto irqret;
+ }
+
+ if (((m_stat & M_RX_FIFO_WATERMARK_EN) ||
+ (m_stat & M_RX_FIFO_LAST_EN)) && (cur->flags & I2C_M_RD)) {
Some of these parenthesis are unnecessary, but more importantly the way
you wrap this line is misleading; the wrapping indicates that the two
latter conditionals are related, but the parenthesis says the first two
are.
The most significant part of this conditional is "is this a read
operation", so put this first.
Ok.
+ u32 rxcnt = rx_st & RX_FIFO_WC_MSK;
+
+ for (j = 0; j < rxcnt; j++) {
+ u32 temp;
+ int p;
+
+ temp = readl_relaxed(gi2c->base + SE_GENI_RX_FIFOn);
+ for (i = gi2c->cur_rd, p = 0; (i < cur->len && p < 4);
+ i++, p++)
+ cur->buf[i] = (u8) ((temp >> (p * 8)) & 0xff);
+ gi2c->cur_rd = i;
The two-range for loop with a line wrap isn't very clean and the wrap
calls for a set of braces - which will look ugly.
How about something like this instead?
p = 0;
while (p < 4 && gi2c->cur_rd < cur->len) {
cur->buf[gi2c->cur_rd++] = temp & 0xff;
temp >>= 8;
p++;
}
Ok.
+ if (gi2c->cur_rd == cur->len) {
+ dev_dbg(gi2c->dev, "FIFO i:%d,read 0x%x\n",
+ i, temp);
Who's the consumer of this debug line?
I will make the debug message more comprehensible, if that is what you mean.
+ break;
+ }
+ }
+ } else if ((m_stat & M_TX_FIFO_WATERMARK_EN) &&
+ !(cur->flags & I2C_M_RD)) {
+ for (j = 0; j < gi2c->tx_wm; j++) {
+ u32 temp = 0;
+ int p;
+
+ for (i = gi2c->cur_wr, p = 0; (i < cur->len && p < 4);
+ i++, p++)
+ temp |= (((u32)(cur->buf[i]) << (p * 8)));
+ writel_relaxed(temp, gi2c->base + SE_GENI_TX_FIFOn);
Same concern as above.
Ok.
+ gi2c->cur_wr = i;
+ dev_dbg(gi2c->dev, "FIFO i:%d,wrote 0x%x\n", i, temp);
+ if (gi2c->cur_wr == cur->len) {
+ dev_dbg(gi2c->dev, "FIFO i2c bytes done writing\n");
+ writel_relaxed(0,
+ (gi2c->base + SE_GENI_TX_WATERMARK_REG));
The line break here is bad. checkpatch.pl --strict will help you find
these.
Ok/
Also using writel() and dropping the parenthesis keeps you below 80
chars.
Ok, to dropping the parenthesis.
+ break;
+ }
+ }
+ }
+irqret:
+ if (m_stat)
+ writel_relaxed(m_stat, gi2c->base + SE_GENI_M_IRQ_CLEAR);
Is it okay for this to be re-ordered wrt above writes?
It is okay. The interrupt bitmask can be cleared anytime while handling
the IRQ.
+
+ if (dma) {
+ if (dm_tx_st)
+ writel_relaxed(dm_tx_st, gi2c->base +
+ SE_DMA_TX_IRQ_CLR);
Use writel() and you don't have to wrap the line.
Please refer to my earlier response regarding using _relaxed() variants.
+ if (dm_rx_st)
+ writel_relaxed(dm_rx_st, gi2c->base +
+ SE_DMA_RX_IRQ_CLR);
+ /* Ensure all writes are done before returning from ISR. */
That's not what wmb does.
I will drop it.
+ wmb();
+ }
+ /* if this is err with done-bit not set, handle that thr' timeout. */
Is "thr'" should for "through"?
Yes. I will make it clear.
+ if (m_stat & M_CMD_DONE_EN)
+ complete(&gi2c->xfer);
+ else if ((dm_tx_st & TX_DMA_DONE) || (dm_rx_st & RX_DMA_DONE))
+ complete(&gi2c->xfer);
+
+ return IRQ_HANDLED;
+}
+
+static int geni_i2c_xfer(struct i2c_adapter *adap,
+ struct i2c_msg msgs[],
+ int num)
+{
+ struct geni_i2c_dev *gi2c = i2c_get_adapdata(adap);
+ int i, ret = 0, timeout = 0;
No need to initialize these, first use is an assignment.
Ok.
+
+ gi2c->err = 0;
+ gi2c->cur = &msgs[0];
You're assigning cur in each iteration of the loop below, why do you
need to do it here as well?
I will remove.
+ reinit_completion(&gi2c->xfer);
+ ret = pm_runtime_get_sync(gi2c->dev);
+ if (ret < 0) {
+ dev_err(gi2c->dev, "error turning SE resources:%d\n", ret);
+ pm_runtime_put_noidle(gi2c->dev);
+ /* Set device in suspended since resume failed */
+ pm_runtime_set_suspended(gi2c->dev);
+ return ret;
With the questionable assignment above you're leaving the function with
gi2c->cur pointing to an object that will soon be invalid.
I will fix the assignment.
+ }
+
+ qcom_geni_i2c_conf(gi2c, 0);
+ dev_dbg(gi2c->dev, "i2c xfer:num:%d, msgs:len:%d,flg:%d\n",
+ num, msgs[0].len, msgs[0].flags);
Use dynamic function tracing instead of debug prints for this.
Ok. I will remove the debug statements for now and use dynamic function
tracing in a different patch.
+ for (i = 0; i < num; i++) {
I suggest that you split out two functions here, one for rx-one-message
and one for tx-one-message. There are some duplication between the two,
but not too bad.
Ok.
+ int stretch = (i < (num - 1));
+ u32 m_param = 0;
+ u32 m_cmd = 0;
+ dma_addr_t tx_dma = 0;
+ dma_addr_t rx_dma = 0;
+ enum geni_se_xfer_mode mode = FIFO_MODE;
No need to initialize mode, as the first use is an assignment.
Ok.
+
+ m_param |= (stretch ? STOP_STRETCH : 0);
+ m_param |= ((msgs[i].addr & 0x7F) << SLV_ADDR_SHFT);
Drop some parenthesis.
Ok.
+
+ gi2c->cur = &msgs[i];
+ mode = msgs[i].len > 32 ? SE_DMA : FIFO_MODE;
+ ret = geni_se_select_mode(gi2c->base, mode);
+ if (ret) {
+ dev_err(gi2c->dev, "%s: Error mode init %d:%d:%d\n",
+ __func__, mode, i, msgs[i].len);
+ break;
+ }
+ if (msgs[i].flags & I2C_M_RD) {
+ dev_dbg(gi2c->dev,
+ "READ,n:%d,i:%d len:%d, stretch:%d\n",
+ num, i, msgs[i].len, stretch);
+ geni_write_reg(msgs[i].len,
+ gi2c->base, SE_I2C_RX_TRANS_LEN);
+ m_cmd = I2C_READ;
+ geni_se_setup_m_cmd(gi2c->base, m_cmd, m_param);
Drop m_cmd and just write I2C_READ in the function call.
Ok.
+ if (mode == SE_DMA) {
+ ret = geni_se_rx_dma_prep(gi2c->wrapper_dev,
+ gi2c->base, msgs[i].buf,
+ msgs[i].len, &rx_dma);
+ if (ret) {
+ mode = FIFO_MODE;
+ ret = geni_se_select_mode(gi2c->base,
+ mode);
+ }
+ }
+ } else {
+ dev_dbg(gi2c->dev,
+ "WRITE:n:%d,i:%d len:%d, stretch:%d, m_param:0x%x\n",
+ num, i, msgs[i].len, stretch, m_param);
+ geni_write_reg(msgs[i].len, gi2c->base,
+ SE_I2C_TX_TRANS_LEN);
+ m_cmd = I2C_WRITE;
+ geni_se_setup_m_cmd(gi2c->base, m_cmd, m_param);
+ if (mode == SE_DMA) {
+ ret = geni_se_tx_dma_prep(gi2c->wrapper_dev,
+ gi2c->base, msgs[i].buf,
+ msgs[i].len, &tx_dma);
+ if (ret) {
+ mode = FIFO_MODE;
+ ret = geni_se_select_mode(gi2c->base,
+ mode);
+ }
+ }
+ if (mode == FIFO_MODE) /* Get FIFO IRQ */
+ geni_write_reg(1, gi2c->base,
+ SE_GENI_TX_WATERMARK_REG);
+ }
+ /* Ensure FIFO write go through before waiting for Done evet */
That's not what mb does, drop it.
Ok.
+ mb();
+ timeout = wait_for_completion_timeout(&gi2c->xfer, HZ);
As written you should just use "ret", but the return value of
wait_for_completion_timeout() is unsigned long, so change the type of
timeout instead.
Ok.
+ if (!timeout) {
+ geni_i2c_err(gi2c, GENI_TIMEOUT);
+ gi2c->cur = NULL;
+ geni_se_abort_m_cmd(gi2c->base);
+ timeout = wait_for_completion_timeout(&gi2c->xfer, HZ);
What if it takes a fraction above HZ time to complete the previous
operation, then you might end up here with the completion completed, but
from the wrong operation.
I will update it to checking the specific "abort" flag using
readl_poll_timeout.
+ }
+ gi2c->cur_wr = 0;
+ gi2c->cur_rd = 0;
+ if (mode == SE_DMA) {
+ if (gi2c->err) {
+ if (msgs[i].flags != I2C_M_RD)
+ writel_relaxed(1, gi2c->base +
+ SE_DMA_TX_FSM_RST);
+ else
+ writel_relaxed(1, gi2c->base +
+ SE_DMA_RX_FSM_RST);
+ wait_for_completion_timeout(&gi2c->xfer, HZ);
+ }
+ geni_se_rx_dma_unprep(gi2c->wrapper_dev, rx_dma,
+ msgs[i].len);
Reduce the length of gi2c->wrapper_dev here by using a local variable,
so that you get below (or close to) 80 chars.
Also, in either rx or tx cases above I see only that you prep one of
thse, and then you're unprepping both.
I will re-organize the tx and rx into 2 separate functions. That way all
the comments will be taken care of.
+ geni_se_tx_dma_unprep(gi2c->wrapper_dev, tx_dma,
+ msgs[i].len);
+ }
+ ret = gi2c->err;
+ if (gi2c->err) {
+ dev_err(gi2c->dev, "i2c error :%d\n", gi2c->err);
+ break;
+ }
+ }
+ if (ret == 0)
+ ret = num;
+
+ pm_runtime_mark_last_busy(gi2c->dev);
+ pm_runtime_put_autosuspend(gi2c->dev);
+ gi2c->cur = NULL;
+ gi2c->err = 0;
+ dev_dbg(gi2c->dev, "i2c txn ret:%d\n", ret);
+ return ret;
+}
[..]
+static int geni_i2c_probe(struct platform_device *pdev)
+{
+ struct geni_i2c_dev *gi2c;
+ struct resource *res;
+ int ret;
+
+ gi2c = devm_kzalloc(&pdev->dev, sizeof(*gi2c), GFP_KERNEL);
+ if (!gi2c)
+ return -ENOMEM;
+
+ gi2c->dev = &pdev->dev;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res)
+ return -EINVAL;
Put this right before devm_ioremap_resource() and drop the error check.
Ok.
+
+ gi2c->wrapper_dev = pdev->dev.parent;
+ gi2c->i2c_rsc.wrapper_dev = pdev->dev.parent;
As suggested in the other patch, if you have an helper function in the
wrapper driver that initializes the i2c_rsc then this could fill out the
actual wrapper, instead of just the device.
Ok.
+ gi2c->i2c_rsc.se_clk = devm_clk_get(&pdev->dev, "se-clk");
+ if (IS_ERR(gi2c->i2c_rsc.se_clk)) {
+ ret = PTR_ERR(gi2c->i2c_rsc.se_clk);
+ dev_err(&pdev->dev, "Err getting SE Core clk %d\n", ret);
+ return ret;
+ }
+
+ gi2c->base = devm_ioremap_resource(gi2c->dev, res);
+ if (IS_ERR(gi2c->base))
+ return PTR_ERR(gi2c->base);
+
+ gi2c->i2c_rsc.geni_pinctrl = devm_pinctrl_get(&pdev->dev);
Drop all the pinctrl stuff. You might still want to call
pinctrl_pm_select_{idle,default,sleep}_state(), in the various stages of
PM state though.
Ok.
+ if (IS_ERR_OR_NULL(gi2c->i2c_rsc.geni_pinctrl)) {
+ dev_err(&pdev->dev, "No pinctrl config specified\n");
+ ret = PTR_ERR(gi2c->i2c_rsc.geni_pinctrl);
+ return ret;
+ }
[..]
+static int geni_i2c_runtime_resume(struct device *dev)
+{
+ int ret;
+ struct geni_i2c_dev *gi2c = dev_get_drvdata(dev);
+
+ ret = geni_se_resources_on(&gi2c->i2c_rsc);
+ if (ret)
+ return ret;
+
+ if (!unlikely(gi2c->tx_wm)) {
Drop the unlikely()
Ok.
+ int proto = geni_se_get_proto(gi2c->base);
+ int gi2c_tx_depth = geni_se_get_tx_fifo_depth(gi2c->base);
+
+ if (unlikely(proto != I2C)) {
Has this changes since probe? Can't we verify that the proto is correct
during probe and then just trust that the hardware doesn't change
function?
Yes we can do that. I will move the check to the probe.
+ dev_err(gi2c->dev, "Invalid proto %d\n", proto);
+ geni_se_resources_off(&gi2c->i2c_rsc);
+ return -ENXIO;
+ }
+
+ gi2c->tx_wm = gi2c_tx_depth - 1;
+ geni_se_init(gi2c->base, gi2c->tx_wm, gi2c_tx_depth);
+ geni_se_config_packing(gi2c->base, 8, 4, true);
+ dev_dbg(gi2c->dev, "i2c fifo/se-dma mode. fifo depth:%d\n",
+ gi2c_tx_depth);
+ }
+ enable_irq(gi2c->irq);
+
+ return 0;
+}
[..]
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:i2c_geni");
What will reference the kernel module by this alias?
I will remove it.
--
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
