From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Subject: Re: [PATCH v2 5/7] i2c: i2c-qcom-geni: Add bus driver for the Qualcomm GENI I2C controller References: <1515805547-22816-1-git-send-email-kramasub@codeaurora.org> <1515805547-22816-6-git-send-email-kramasub@codeaurora.org> <20180118052353.GA12583@builder> From: Karthik Ramasubramanian Message-ID: <4ff97d07-c28b-0bbb-5891-e696bce6a3c9@codeaurora.org> Date: Tue, 27 Feb 2018 06:16:06 -0700 MIME-Version: 1.0 In-Reply-To: <20180118052353.GA12583@builder> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit To: Bjorn Andersson Cc: corbet@lwn.net, andy.gross@linaro.org, david.brown@linaro.org, robh+dt@kernel.org, mark.rutland@arm.com, wsa@the-dreams.de, gregkh@linuxfoundation.org, linux-doc@vger.kernel.org, linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org, linux-i2c@vger.kernel.org, linux-serial@vger.kernel.org, jslaby@suse.com, Sagar Dharia , Girish Mahadevan List-ID: 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 >> Signed-off-by: Sagar Dharia >> Signed-off-by: Girish Mahadevan >> --- >> 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 >> +#include > > Unused? Ok, I will remove unused header files. > >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#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@vger.kernel.org >> 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