* [PATCH v3 0/3] Fix qcom geni i2c DMA handling @ 2018-09-24 23:52 Stephen Boyd 2018-09-24 23:52 ` [PATCH v3 2/3] i2c: i2c-qcom-geni: Simplify tx/rx functions Stephen Boyd 2018-09-24 23:52 ` [PATCH v3 3/3] i2c: i2c-qcom-geni: Simplify irq handler Stephen Boyd 0 siblings, 2 replies; 9+ messages in thread From: Stephen Boyd @ 2018-09-24 23:52 UTC (permalink / raw) To: Wolfram Sang Cc: linux-kernel, linux-i2c, linux-arm-msm, Karthikeyan Ramasubramanian, Sagar Dharia, Girish Mahadevan, Doug Anderson (Numbering is weird because I dropped patch 1 but left numbering the same) The qcom GENI I2C driver fails DMA sometimes when things from request firmware are passed in as the message buffer. This patch series fixes that problem in the first patch and the second patch cleans up the code a little to reduce lines and simplify lines. Cc: Karthikeyan Ramasubramanian <kramasub@codeaurora.org> Cc: Sagar Dharia <sdharia@codeaurora.org> Cc: Girish Mahadevan <girishm@codeaurora.org> Cc: Doug Anderson <dianders@chromium.org> Changes from v2: * Dropped first patch because it's applied * New patch 3 to simplify irq handler * Updated patch 2 to hoist out common code and remove 'mode' local variable Changes from v1: * Use i2c helpers to map buffers * New patch 2 to clean up seriously indented code Stephen Boyd (2): i2c: i2c-qcom-geni: Simplify tx/rx functions i2c: i2c-qcom-geni: Simplify irq handler drivers/i2c/busses/i2c-qcom-geni.c | 149 +++++++++++++---------------- 1 file changed, 65 insertions(+), 84 deletions(-) -- Sent by a computer through tubes ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3 2/3] i2c: i2c-qcom-geni: Simplify tx/rx functions 2018-09-24 23:52 [PATCH v3 0/3] Fix qcom geni i2c DMA handling Stephen Boyd @ 2018-09-24 23:52 ` Stephen Boyd 2018-09-25 21:49 ` Doug Anderson ` (2 more replies) 2018-09-24 23:52 ` [PATCH v3 3/3] i2c: i2c-qcom-geni: Simplify irq handler Stephen Boyd 1 sibling, 3 replies; 9+ messages in thread From: Stephen Boyd @ 2018-09-24 23:52 UTC (permalink / raw) To: Wolfram Sang Cc: linux-kernel, linux-i2c, linux-arm-msm, Karthikeyan Ramasubramanian, Sagar Dharia, Girish Mahadevan, Doug Anderson We never really look at the 'ret' local variable in these functions, so let's remove it to make way for shorter and simpler code. Furthermore, we can shorten some lines by adding two local variables for the SE and the message length so that everything fits in 80 columns and testing the 'dma_buf' local variable in lieu of the 'mode' local variable. And kernel style is to leave the return statement by itself, detached from the rest of the function. Cc: Karthikeyan Ramasubramanian <kramasub@codeaurora.org> Cc: Sagar Dharia <sdharia@codeaurora.org> Cc: Girish Mahadevan <girishm@codeaurora.org> Cc: Doug Anderson <dianders@chromium.org> Signed-off-by: Stephen Boyd <swboyd@chromium.org> --- drivers/i2c/busses/i2c-qcom-geni.c | 79 ++++++++++++++---------------- 1 file changed, 36 insertions(+), 43 deletions(-) diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c index 9f2eb02481d3..0b466835cf40 100644 --- a/drivers/i2c/busses/i2c-qcom-geni.c +++ b/drivers/i2c/busses/i2c-qcom-geni.c @@ -365,29 +365,24 @@ static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg, u32 m_param) { dma_addr_t rx_dma; - enum geni_se_xfer_mode mode; - unsigned long time_left = XFER_TIMEOUT; + unsigned long time_left; void *dma_buf; + struct geni_se *se = &gi2c->se; + size_t len = msg->len; - gi2c->cur = msg; - mode = GENI_SE_FIFO; dma_buf = i2c_get_dma_safe_msg_buf(msg, 32); if (dma_buf) - mode = GENI_SE_DMA; - - geni_se_select_mode(&gi2c->se, mode); - writel_relaxed(msg->len, gi2c->se.base + SE_I2C_RX_TRANS_LEN); - geni_se_setup_m_cmd(&gi2c->se, I2C_READ, m_param); - if (mode == GENI_SE_DMA) { - int ret; - - ret = geni_se_rx_dma_prep(&gi2c->se, dma_buf, msg->len, - &rx_dma); - if (ret) { - mode = GENI_SE_FIFO; - geni_se_select_mode(&gi2c->se, mode); - i2c_put_dma_safe_msg_buf(dma_buf, msg, false); - } + geni_se_select_mode(se, GENI_SE_DMA); + else + geni_se_select_mode(se, GENI_SE_FIFO); + + writel_relaxed(len, se->base + SE_I2C_RX_TRANS_LEN); + geni_se_setup_m_cmd(se, I2C_READ, m_param); + + if (dma_buf && geni_se_rx_dma_prep(se, dma_buf, len, &rx_dma)) { + geni_se_select_mode(se, GENI_SE_FIFO); + i2c_put_dma_safe_msg_buf(dma_buf, msg, false); + dma_buf = NULL; } time_left = wait_for_completion_timeout(&gi2c->done, XFER_TIMEOUT); @@ -395,12 +390,13 @@ static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg, geni_i2c_abort_xfer(gi2c); gi2c->cur_rd = 0; - if (mode == GENI_SE_DMA) { + if (dma_buf) { if (gi2c->err) geni_i2c_rx_fsm_rst(gi2c); - geni_se_rx_dma_unprep(&gi2c->se, rx_dma, msg->len); + geni_se_rx_dma_unprep(se, rx_dma, len); i2c_put_dma_safe_msg_buf(dma_buf, msg, !gi2c->err); } + return gi2c->err; } @@ -408,45 +404,41 @@ static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg, u32 m_param) { dma_addr_t tx_dma; - enum geni_se_xfer_mode mode; unsigned long time_left; void *dma_buf; + struct geni_se *se = &gi2c->se; + size_t len = msg->len; - gi2c->cur = msg; - mode = GENI_SE_FIFO; dma_buf = i2c_get_dma_safe_msg_buf(msg, 32); if (dma_buf) - mode = GENI_SE_DMA; - - geni_se_select_mode(&gi2c->se, mode); - writel_relaxed(msg->len, gi2c->se.base + SE_I2C_TX_TRANS_LEN); - geni_se_setup_m_cmd(&gi2c->se, I2C_WRITE, m_param); - if (mode == GENI_SE_DMA) { - int ret; - - ret = geni_se_tx_dma_prep(&gi2c->se, dma_buf, msg->len, - &tx_dma); - if (ret) { - mode = GENI_SE_FIFO; - geni_se_select_mode(&gi2c->se, mode); - i2c_put_dma_safe_msg_buf(dma_buf, msg, false); - } + geni_se_select_mode(se, GENI_SE_DMA); + else + geni_se_select_mode(se, GENI_SE_FIFO); + + writel_relaxed(len, se->base + SE_I2C_TX_TRANS_LEN); + geni_se_setup_m_cmd(se, I2C_WRITE, m_param); + + if (dma_buf && geni_se_tx_dma_prep(se, dma_buf, len, &tx_dma)) { + geni_se_select_mode(se, GENI_SE_FIFO); + i2c_put_dma_safe_msg_buf(dma_buf, msg, false); + dma_buf = NULL; } - if (mode == GENI_SE_FIFO) /* Get FIFO IRQ */ - writel_relaxed(1, gi2c->se.base + SE_GENI_TX_WATERMARK_REG); + if (!dma_buf) /* Get FIFO IRQ */ + writel_relaxed(1, se->base + SE_GENI_TX_WATERMARK_REG); time_left = wait_for_completion_timeout(&gi2c->done, XFER_TIMEOUT); if (!time_left) geni_i2c_abort_xfer(gi2c); gi2c->cur_wr = 0; - if (mode == GENI_SE_DMA) { + if (dma_buf) { if (gi2c->err) geni_i2c_tx_fsm_rst(gi2c); - geni_se_tx_dma_unprep(&gi2c->se, tx_dma, msg->len); + geni_se_tx_dma_unprep(se, tx_dma, len); i2c_put_dma_safe_msg_buf(dma_buf, msg, !gi2c->err); } + return gi2c->err; } @@ -474,6 +466,7 @@ static int geni_i2c_xfer(struct i2c_adapter *adap, m_param |= ((msgs[i].addr << SLV_ADDR_SHFT) & SLV_ADDR_MSK); + gi2c->cur = &msgs[i]; if (msgs[i].flags & I2C_M_RD) ret = geni_i2c_rx_one_msg(gi2c, &msgs[i], m_param); else -- Sent by a computer through tubes ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3 2/3] i2c: i2c-qcom-geni: Simplify tx/rx functions 2018-09-24 23:52 ` [PATCH v3 2/3] i2c: i2c-qcom-geni: Simplify tx/rx functions Stephen Boyd @ 2018-09-25 21:49 ` Doug Anderson 2018-10-04 7:28 ` alokc 2018-10-11 21:11 ` Wolfram Sang 2 siblings, 0 replies; 9+ messages in thread From: Doug Anderson @ 2018-09-25 21:49 UTC (permalink / raw) To: Stephen Boyd Cc: Wolfram Sang, LKML, linux-i2c, linux-arm-msm, Karthikeyan Ramasubramanian, Sagar Dharia, Girish Mahadevan Hi, On Mon, Sep 24, 2018 at 4:52 PM Stephen Boyd <swboyd@chromium.org> wrote: > > We never really look at the 'ret' local variable in these functions, so > let's remove it to make way for shorter and simpler code. Furthermore, > we can shorten some lines by adding two local variables for the SE and > the message length so that everything fits in 80 columns and testing the > 'dma_buf' local variable in lieu of the 'mode' local variable. And > kernel style is to leave the return statement by itself, detached from > the rest of the function. > > Cc: Karthikeyan Ramasubramanian <kramasub@codeaurora.org> > Cc: Sagar Dharia <sdharia@codeaurora.org> > Cc: Girish Mahadevan <girishm@codeaurora.org> > Cc: Doug Anderson <dianders@chromium.org> > Signed-off-by: Stephen Boyd <swboyd@chromium.org> > --- > drivers/i2c/busses/i2c-qcom-geni.c | 79 ++++++++++++++---------------- > 1 file changed, 36 insertions(+), 43 deletions(-) It's so polished I can see my reflection in it now. Looks like this snuck in a few other cleanups compared to v2 (move "gi2c->cur = ..." to be common between tx/rx and removed a pointless init of time_left). Seems good. In total 7 lines shorter and easier to read. Reviewed-by: Douglas Anderson <dianders@chromium.org> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 2/3] i2c: i2c-qcom-geni: Simplify tx/rx functions 2018-09-24 23:52 ` [PATCH v3 2/3] i2c: i2c-qcom-geni: Simplify tx/rx functions Stephen Boyd 2018-09-25 21:49 ` Doug Anderson @ 2018-10-04 7:28 ` alokc 2018-10-11 21:11 ` Wolfram Sang 2 siblings, 0 replies; 9+ messages in thread From: alokc @ 2018-10-04 7:28 UTC (permalink / raw) To: Stephen Boyd Cc: Wolfram Sang, linux-kernel, linux-i2c, linux-arm-msm, Karthikeyan Ramasubramanian, Sagar Dharia, Girish Mahadevan, Doug Anderson, linux-i2c-owner On 2018-09-25 05:22, Stephen Boyd wrote: > We never really look at the 'ret' local variable in these functions, so > let's remove it to make way for shorter and simpler code. Furthermore, > we can shorten some lines by adding two local variables for the SE and > the message length so that everything fits in 80 columns and testing > the > 'dma_buf' local variable in lieu of the 'mode' local variable. And > kernel style is to leave the return statement by itself, detached from > the rest of the function. > > Cc: Karthikeyan Ramasubramanian <kramasub@codeaurora.org> > Cc: Sagar Dharia <sdharia@codeaurora.org> > Cc: Girish Mahadevan <girishm@codeaurora.org> > Cc: Doug Anderson <dianders@chromium.org> > Signed-off-by: Stephen Boyd <swboyd@chromium.org> > --- changes looks more clean now. Reviewed-by: Alok Chauhan <alokc@codeaurora.org> > drivers/i2c/busses/i2c-qcom-geni.c | 79 ++++++++++++++---------------- > 1 file changed, 36 insertions(+), 43 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-qcom-geni.c > b/drivers/i2c/busses/i2c-qcom-geni.c > index 9f2eb02481d3..0b466835cf40 100644 > --- a/drivers/i2c/busses/i2c-qcom-geni.c > +++ b/drivers/i2c/busses/i2c-qcom-geni.c > @@ -365,29 +365,24 @@ static int geni_i2c_rx_one_msg(struct > geni_i2c_dev *gi2c, struct i2c_msg *msg, > u32 m_param) > { > dma_addr_t rx_dma; > - enum geni_se_xfer_mode mode; > - unsigned long time_left = XFER_TIMEOUT; > + unsigned long time_left; > void *dma_buf; > + struct geni_se *se = &gi2c->se; > + size_t len = msg->len; > > - gi2c->cur = msg; > - mode = GENI_SE_FIFO; > dma_buf = i2c_get_dma_safe_msg_buf(msg, 32); > if (dma_buf) > - mode = GENI_SE_DMA; > - > - geni_se_select_mode(&gi2c->se, mode); > - writel_relaxed(msg->len, gi2c->se.base + SE_I2C_RX_TRANS_LEN); > - geni_se_setup_m_cmd(&gi2c->se, I2C_READ, m_param); > - if (mode == GENI_SE_DMA) { > - int ret; > - > - ret = geni_se_rx_dma_prep(&gi2c->se, dma_buf, msg->len, > - &rx_dma); > - if (ret) { > - mode = GENI_SE_FIFO; > - geni_se_select_mode(&gi2c->se, mode); > - i2c_put_dma_safe_msg_buf(dma_buf, msg, false); > - } > + geni_se_select_mode(se, GENI_SE_DMA); > + else > + geni_se_select_mode(se, GENI_SE_FIFO); > + > + writel_relaxed(len, se->base + SE_I2C_RX_TRANS_LEN); > + geni_se_setup_m_cmd(se, I2C_READ, m_param); > + > + if (dma_buf && geni_se_rx_dma_prep(se, dma_buf, len, &rx_dma)) { > + geni_se_select_mode(se, GENI_SE_FIFO); > + i2c_put_dma_safe_msg_buf(dma_buf, msg, false); > + dma_buf = NULL; > } > > time_left = wait_for_completion_timeout(&gi2c->done, XFER_TIMEOUT); > @@ -395,12 +390,13 @@ static int geni_i2c_rx_one_msg(struct > geni_i2c_dev *gi2c, struct i2c_msg *msg, > geni_i2c_abort_xfer(gi2c); > > gi2c->cur_rd = 0; > - if (mode == GENI_SE_DMA) { > + if (dma_buf) { > if (gi2c->err) > geni_i2c_rx_fsm_rst(gi2c); > - geni_se_rx_dma_unprep(&gi2c->se, rx_dma, msg->len); > + geni_se_rx_dma_unprep(se, rx_dma, len); > i2c_put_dma_safe_msg_buf(dma_buf, msg, !gi2c->err); > } > + > return gi2c->err; > } > > @@ -408,45 +404,41 @@ static int geni_i2c_tx_one_msg(struct > geni_i2c_dev *gi2c, struct i2c_msg *msg, > u32 m_param) > { > dma_addr_t tx_dma; > - enum geni_se_xfer_mode mode; > unsigned long time_left; > void *dma_buf; > + struct geni_se *se = &gi2c->se; > + size_t len = msg->len; > > - gi2c->cur = msg; > - mode = GENI_SE_FIFO; > dma_buf = i2c_get_dma_safe_msg_buf(msg, 32); > if (dma_buf) > - mode = GENI_SE_DMA; > - > - geni_se_select_mode(&gi2c->se, mode); > - writel_relaxed(msg->len, gi2c->se.base + SE_I2C_TX_TRANS_LEN); > - geni_se_setup_m_cmd(&gi2c->se, I2C_WRITE, m_param); > - if (mode == GENI_SE_DMA) { > - int ret; > - > - ret = geni_se_tx_dma_prep(&gi2c->se, dma_buf, msg->len, > - &tx_dma); > - if (ret) { > - mode = GENI_SE_FIFO; > - geni_se_select_mode(&gi2c->se, mode); > - i2c_put_dma_safe_msg_buf(dma_buf, msg, false); > - } > + geni_se_select_mode(se, GENI_SE_DMA); > + else > + geni_se_select_mode(se, GENI_SE_FIFO); > + > + writel_relaxed(len, se->base + SE_I2C_TX_TRANS_LEN); > + geni_se_setup_m_cmd(se, I2C_WRITE, m_param); > + > + if (dma_buf && geni_se_tx_dma_prep(se, dma_buf, len, &tx_dma)) { > + geni_se_select_mode(se, GENI_SE_FIFO); > + i2c_put_dma_safe_msg_buf(dma_buf, msg, false); > + dma_buf = NULL; > } > > - if (mode == GENI_SE_FIFO) /* Get FIFO IRQ */ > - writel_relaxed(1, gi2c->se.base + SE_GENI_TX_WATERMARK_REG); > + if (!dma_buf) /* Get FIFO IRQ */ > + writel_relaxed(1, se->base + SE_GENI_TX_WATERMARK_REG); > > time_left = wait_for_completion_timeout(&gi2c->done, XFER_TIMEOUT); > if (!time_left) > geni_i2c_abort_xfer(gi2c); > > gi2c->cur_wr = 0; > - if (mode == GENI_SE_DMA) { > + if (dma_buf) { > if (gi2c->err) > geni_i2c_tx_fsm_rst(gi2c); > - geni_se_tx_dma_unprep(&gi2c->se, tx_dma, msg->len); > + geni_se_tx_dma_unprep(se, tx_dma, len); > i2c_put_dma_safe_msg_buf(dma_buf, msg, !gi2c->err); > } > + > return gi2c->err; > } > > @@ -474,6 +466,7 @@ static int geni_i2c_xfer(struct i2c_adapter *adap, > > m_param |= ((msgs[i].addr << SLV_ADDR_SHFT) & SLV_ADDR_MSK); > > + gi2c->cur = &msgs[i]; > if (msgs[i].flags & I2C_M_RD) > ret = geni_i2c_rx_one_msg(gi2c, &msgs[i], m_param); > else -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 2/3] i2c: i2c-qcom-geni: Simplify tx/rx functions 2018-09-24 23:52 ` [PATCH v3 2/3] i2c: i2c-qcom-geni: Simplify tx/rx functions Stephen Boyd 2018-09-25 21:49 ` Doug Anderson 2018-10-04 7:28 ` alokc @ 2018-10-11 21:11 ` Wolfram Sang 2 siblings, 0 replies; 9+ messages in thread From: Wolfram Sang @ 2018-10-11 21:11 UTC (permalink / raw) To: Stephen Boyd Cc: linux-kernel, linux-i2c, linux-arm-msm, Karthikeyan Ramasubramanian, Sagar Dharia, Girish Mahadevan, Doug Anderson [-- Attachment #1: Type: text/plain, Size: 827 bytes --] On Mon, Sep 24, 2018 at 04:52:34PM -0700, Stephen Boyd wrote: > We never really look at the 'ret' local variable in these functions, so > let's remove it to make way for shorter and simpler code. Furthermore, > we can shorten some lines by adding two local variables for the SE and > the message length so that everything fits in 80 columns and testing the > 'dma_buf' local variable in lieu of the 'mode' local variable. And > kernel style is to leave the return statement by itself, detached from > the rest of the function. > > Cc: Karthikeyan Ramasubramanian <kramasub@codeaurora.org> > Cc: Sagar Dharia <sdharia@codeaurora.org> > Cc: Girish Mahadevan <girishm@codeaurora.org> > Cc: Doug Anderson <dianders@chromium.org> > Signed-off-by: Stephen Boyd <swboyd@chromium.org> Applied to for-next, thanks! [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3 3/3] i2c: i2c-qcom-geni: Simplify irq handler 2018-09-24 23:52 [PATCH v3 0/3] Fix qcom geni i2c DMA handling Stephen Boyd 2018-09-24 23:52 ` [PATCH v3 2/3] i2c: i2c-qcom-geni: Simplify tx/rx functions Stephen Boyd @ 2018-09-24 23:52 ` Stephen Boyd 2018-09-25 21:49 ` Doug Anderson 2018-10-11 21:11 ` Wolfram Sang 1 sibling, 2 replies; 9+ messages in thread From: Stephen Boyd @ 2018-09-24 23:52 UTC (permalink / raw) To: Wolfram Sang Cc: linux-kernel, linux-i2c, linux-arm-msm, Karthikeyan Ramasubramanian, Sagar Dharia, Girish Mahadevan, Doug Anderson We don't need to use goto here, we can just collapse the if statement and goto chain into multiple branches and then combine some duplicate completion calls into one big if statement. Let's do it to clean up code some more. Cc: Karthikeyan Ramasubramanian <kramasub@codeaurora.org> Cc: Sagar Dharia <sdharia@codeaurora.org> Cc: Girish Mahadevan <girishm@codeaurora.org> Cc: Doug Anderson <dianders@chromium.org> Signed-off-by: Stephen Boyd <swboyd@chromium.org> --- drivers/i2c/busses/i2c-qcom-geni.c | 70 +++++++++++++----------------- 1 file changed, 29 insertions(+), 41 deletions(-) diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c index 0b466835cf40..527f55c8c4c7 100644 --- a/drivers/i2c/busses/i2c-qcom-geni.c +++ b/drivers/i2c/busses/i2c-qcom-geni.c @@ -201,21 +201,23 @@ static void geni_i2c_err(struct geni_i2c_dev *gi2c, int err) static irqreturn_t geni_i2c_irq(int irq, void *dev) { struct geni_i2c_dev *gi2c = dev; - int j; + void __iomem *base = gi2c->se.base; + int j, p; u32 m_stat; u32 rx_st; u32 dm_tx_st; u32 dm_rx_st; u32 dma; + u32 val; struct i2c_msg *cur; unsigned long flags; spin_lock_irqsave(&gi2c->lock, flags); - m_stat = readl_relaxed(gi2c->se.base + SE_GENI_M_IRQ_STATUS); - rx_st = readl_relaxed(gi2c->se.base + SE_GENI_RX_FIFO_STATUS); - dm_tx_st = readl_relaxed(gi2c->se.base + SE_DMA_TX_IRQ_STAT); - dm_rx_st = readl_relaxed(gi2c->se.base + SE_DMA_RX_IRQ_STAT); - dma = readl_relaxed(gi2c->se.base + SE_GENI_DMA_MODE_EN); + m_stat = readl_relaxed(base + SE_GENI_M_IRQ_STATUS); + rx_st = readl_relaxed(base + SE_GENI_RX_FIFO_STATUS); + dm_tx_st = readl_relaxed(base + SE_DMA_TX_IRQ_STAT); + dm_rx_st = readl_relaxed(base + SE_DMA_RX_IRQ_STAT); + dma = readl_relaxed(base + SE_GENI_DMA_MODE_EN); cur = gi2c->cur; if (!cur || @@ -238,26 +240,17 @@ static irqreturn_t geni_i2c_irq(int irq, void *dev) /* Disable the TX Watermark interrupt to stop TX */ if (!dma) - writel_relaxed(0, gi2c->se.base + - SE_GENI_TX_WATERMARK_REG); - goto irqret; - } - - if (dma) { + writel_relaxed(0, base + SE_GENI_TX_WATERMARK_REG); + } else if (dma) { dev_dbg(gi2c->se.dev, "i2c dma tx:0x%x, dma rx:0x%x\n", dm_tx_st, dm_rx_st); - goto irqret; - } - - if (cur->flags & I2C_M_RD && - m_stat & (M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN)) { + } else if (cur->flags & I2C_M_RD && + m_stat & (M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN)) { u32 rxcnt = rx_st & RX_FIFO_WC_MSK; for (j = 0; j < rxcnt; j++) { - u32 val; - int p = 0; - - val = readl_relaxed(gi2c->se.base + SE_GENI_RX_FIFOn); + p = 0; + val = readl_relaxed(base + SE_GENI_RX_FIFOn); while (gi2c->cur_rd < cur->len && p < sizeof(val)) { cur->buf[gi2c->cur_rd++] = val & 0xff; val >>= 8; @@ -270,44 +263,39 @@ static irqreturn_t geni_i2c_irq(int irq, void *dev) m_stat & M_TX_FIFO_WATERMARK_EN) { for (j = 0; j < gi2c->tx_wm; j++) { u32 temp; - u32 val = 0; - int p = 0; + val = 0; + p = 0; while (gi2c->cur_wr < cur->len && p < sizeof(val)) { temp = cur->buf[gi2c->cur_wr++]; val |= temp << (p * 8); p++; } - writel_relaxed(val, gi2c->se.base + SE_GENI_TX_FIFOn); + writel_relaxed(val, base + SE_GENI_TX_FIFOn); /* TX Complete, Disable the TX Watermark interrupt */ if (gi2c->cur_wr == cur->len) { - writel_relaxed(0, gi2c->se.base + - SE_GENI_TX_WATERMARK_REG); + writel_relaxed(0, base + SE_GENI_TX_WATERMARK_REG); break; } } } -irqret: + if (m_stat) - writel_relaxed(m_stat, gi2c->se.base + SE_GENI_M_IRQ_CLEAR); + writel_relaxed(m_stat, base + SE_GENI_M_IRQ_CLEAR); + + if (dma && dm_tx_st) + writel_relaxed(dm_tx_st, base + SE_DMA_TX_IRQ_CLR); + if (dma && dm_rx_st) + writel_relaxed(dm_rx_st, base + SE_DMA_RX_IRQ_CLR); - if (dma) { - if (dm_tx_st) - writel_relaxed(dm_tx_st, gi2c->se.base + - SE_DMA_TX_IRQ_CLR); - if (dm_rx_st) - writel_relaxed(dm_rx_st, gi2c->se.base + - SE_DMA_RX_IRQ_CLR); - } /* if this is err with done-bit not set, handle that through timeout. */ - if (m_stat & M_CMD_DONE_EN || m_stat & M_CMD_ABORT_EN) - complete(&gi2c->done); - else if (dm_tx_st & TX_DMA_DONE || dm_tx_st & TX_RESET_DONE) - complete(&gi2c->done); - else if (dm_rx_st & RX_DMA_DONE || dm_rx_st & RX_RESET_DONE) + if (m_stat & M_CMD_DONE_EN || m_stat & M_CMD_ABORT_EN || + dm_tx_st & TX_DMA_DONE || dm_tx_st & TX_RESET_DONE || + dm_rx_st & RX_DMA_DONE || dm_rx_st & RX_RESET_DONE) complete(&gi2c->done); spin_unlock_irqrestore(&gi2c->lock, flags); + return IRQ_HANDLED; } -- Sent by a computer through tubes ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3 3/3] i2c: i2c-qcom-geni: Simplify irq handler 2018-09-24 23:52 ` [PATCH v3 3/3] i2c: i2c-qcom-geni: Simplify irq handler Stephen Boyd @ 2018-09-25 21:49 ` Doug Anderson 2018-10-04 7:46 ` alokc 2018-10-11 21:11 ` Wolfram Sang 1 sibling, 1 reply; 9+ messages in thread From: Doug Anderson @ 2018-09-25 21:49 UTC (permalink / raw) To: Stephen Boyd Cc: Wolfram Sang, LKML, linux-i2c, linux-arm-msm, Karthikeyan Ramasubramanian, Sagar Dharia, Girish Mahadevan Hi, On Mon, Sep 24, 2018 at 4:52 PM Stephen Boyd <swboyd@chromium.org> wrote: > > We don't need to use goto here, we can just collapse the if statement > and goto chain into multiple branches and then combine some duplicate > completion calls into one big if statement. Let's do it to clean up code > some more. > > Cc: Karthikeyan Ramasubramanian <kramasub@codeaurora.org> > Cc: Sagar Dharia <sdharia@codeaurora.org> > Cc: Girish Mahadevan <girishm@codeaurora.org> > Cc: Doug Anderson <dianders@chromium.org> > Signed-off-by: Stephen Boyd <swboyd@chromium.org> > --- > drivers/i2c/busses/i2c-qcom-geni.c | 70 +++++++++++++----------------- > 1 file changed, 29 insertions(+), 41 deletions(-) It doesn't gleam as powerfully the cleanups in patch 2/3 but this does have a few nice readability improvements. Reviewed-by: Douglas Anderson <dianders@chromium.org> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 3/3] i2c: i2c-qcom-geni: Simplify irq handler 2018-09-25 21:49 ` Doug Anderson @ 2018-10-04 7:46 ` alokc 0 siblings, 0 replies; 9+ messages in thread From: alokc @ 2018-10-04 7:46 UTC (permalink / raw) To: Doug Anderson Cc: Stephen Boyd, Wolfram Sang, LKML, linux-i2c, linux-arm-msm, Karthikeyan Ramasubramanian, Sagar Dharia, Girish Mahadevan, linux-i2c-owner On 2018-09-26 03:19, Doug Anderson wrote: > Hi, > > On Mon, Sep 24, 2018 at 4:52 PM Stephen Boyd <swboyd@chromium.org> > wrote: >> >> We don't need to use goto here, we can just collapse the if statement >> and goto chain into multiple branches and then combine some duplicate >> completion calls into one big if statement. Let's do it to clean up >> code >> some more. >> >> Cc: Karthikeyan Ramasubramanian <kramasub@codeaurora.org> >> Cc: Sagar Dharia <sdharia@codeaurora.org> >> Cc: Girish Mahadevan <girishm@codeaurora.org> >> Cc: Doug Anderson <dianders@chromium.org> >> Signed-off-by: Stephen Boyd <swboyd@chromium.org> >> --- >> drivers/i2c/busses/i2c-qcom-geni.c | 70 >> +++++++++++++----------------- >> 1 file changed, 29 insertions(+), 41 deletions(-) > > It doesn't gleam as powerfully the cleanups in patch 2/3 but this does > have a few nice readability improvements. > > Reviewed-by: Douglas Anderson <dianders@chromium.org> agree. Reviewed-by: Alok Chauhan <alokc@codeaurora.org> -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 3/3] i2c: i2c-qcom-geni: Simplify irq handler 2018-09-24 23:52 ` [PATCH v3 3/3] i2c: i2c-qcom-geni: Simplify irq handler Stephen Boyd 2018-09-25 21:49 ` Doug Anderson @ 2018-10-11 21:11 ` Wolfram Sang 1 sibling, 0 replies; 9+ messages in thread From: Wolfram Sang @ 2018-10-11 21:11 UTC (permalink / raw) To: Stephen Boyd Cc: linux-kernel, linux-i2c, linux-arm-msm, Karthikeyan Ramasubramanian, Sagar Dharia, Girish Mahadevan, Doug Anderson [-- Attachment #1: Type: text/plain, Size: 590 bytes --] On Mon, Sep 24, 2018 at 04:52:35PM -0700, Stephen Boyd wrote: > We don't need to use goto here, we can just collapse the if statement > and goto chain into multiple branches and then combine some duplicate > completion calls into one big if statement. Let's do it to clean up code > some more. > > Cc: Karthikeyan Ramasubramanian <kramasub@codeaurora.org> > Cc: Sagar Dharia <sdharia@codeaurora.org> > Cc: Girish Mahadevan <girishm@codeaurora.org> > Cc: Doug Anderson <dianders@chromium.org> > Signed-off-by: Stephen Boyd <swboyd@chromium.org> Applied to for-next, thanks! [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-10-11 21:11 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-09-24 23:52 [PATCH v3 0/3] Fix qcom geni i2c DMA handling Stephen Boyd 2018-09-24 23:52 ` [PATCH v3 2/3] i2c: i2c-qcom-geni: Simplify tx/rx functions Stephen Boyd 2018-09-25 21:49 ` Doug Anderson 2018-10-04 7:28 ` alokc 2018-10-11 21:11 ` Wolfram Sang 2018-09-24 23:52 ` [PATCH v3 3/3] i2c: i2c-qcom-geni: Simplify irq handler Stephen Boyd 2018-09-25 21:49 ` Doug Anderson 2018-10-04 7:46 ` alokc 2018-10-11 21:11 ` Wolfram Sang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).