| Message ID | 20200814095540.32115-2-rojay@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Implement Shutdown callback for i2c | expand |
Quoting Roja Rani Yarubandi (2020-08-14 02:55:39) > Adding tx_dma, rx_dma and xfer length in geni_i2c_dev struct to > store DMA mapping data to enhance its scope. For example during > shutdown callback to unmap DMA mapping, these new struct members > can be used as part of geni_se_tx_dma_unprep and > geni_se_rx_dma_unprep calls. Please read about how to write commit text from kernel docs[1]. Hint, use imperative mood. > > Signed-off-by: Roja Rani Yarubandi <rojay@codeaurora.org> > --- > drivers/i2c/busses/i2c-qcom-geni.c | 23 +++++++++++++---------- > 1 file changed, 13 insertions(+), 10 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c > index 7f130829bf01..53ca41f76080 100644 > --- a/drivers/i2c/busses/i2c-qcom-geni.c > +++ b/drivers/i2c/busses/i2c-qcom-geni.c > @@ -86,6 +86,9 @@ struct geni_i2c_dev { > u32 clk_freq_out; > const struct geni_i2c_clk_fld *clk_fld; > int suspended; > + dma_addr_t tx_dma; > + dma_addr_t rx_dma; > + u32 xfer_len; Why not size_t? > }; > > struct geni_i2c_err_log { > @@ -352,12 +355,11 @@ static void geni_i2c_tx_fsm_rst(struct geni_i2c_dev *gi2c) > static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg, > u32 m_param) > { > - dma_addr_t rx_dma; > unsigned long time_left; > void *dma_buf = NULL; > struct geni_se *se = &gi2c->se; > - size_t len = msg->len; > > + gi2c->xfer_len = msg->len; I'd prefer to keep the local variable and then have len = gi2c->xfer_len = msg->len; > if (!of_machine_is_compatible("lenovo,yoga-c630")) > dma_buf = i2c_get_dma_safe_msg_buf(msg, 32); > > @@ -366,9 +368,10 @@ static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg, > else > geni_se_select_mode(se, GENI_SE_FIFO); > > - writel_relaxed(len, se->base + SE_I2C_RX_TRANS_LEN); > + writel_relaxed(gi2c->xfer_len, se->base + SE_I2C_RX_TRANS_LEN); So that all this doesn't have to change. > > - if (dma_buf && geni_se_rx_dma_prep(se, dma_buf, len, &rx_dma)) { > + if (dma_buf && geni_se_rx_dma_prep(se, dma_buf, gi2c->xfer_len, > + &gi2c->rx_dma)) { > geni_se_select_mode(se, GENI_SE_FIFO); > i2c_put_dma_safe_msg_buf(dma_buf, msg, false); > dma_buf = NULL; > @@ -384,7 +387,7 @@ static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg, > if (dma_buf) { > if (gi2c->err) > geni_i2c_rx_fsm_rst(gi2c); > - geni_se_rx_dma_unprep(se, rx_dma, len); > + geni_se_rx_dma_unprep(se, gi2c->rx_dma, gi2c->xfer_len); > i2c_put_dma_safe_msg_buf(dma_buf, msg, !gi2c->err); > } > > @@ -394,12 +397,11 @@ static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg, > static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg, > u32 m_param) > { > - dma_addr_t tx_dma; > unsigned long time_left; > void *dma_buf = NULL; > struct geni_se *se = &gi2c->se; > - size_t len = msg->len; > > + gi2c->xfer_len = msg->len; Same comment. > if (!of_machine_is_compatible("lenovo,yoga-c630")) > dma_buf = i2c_get_dma_safe_msg_buf(msg, 32); > [1] https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes
Hi Stephen, Thanks for reviewing the patches. On 2020-08-19 09:09, Stephen Boyd wrote: > Quoting Roja Rani Yarubandi (2020-08-14 02:55:39) >> Adding tx_dma, rx_dma and xfer length in geni_i2c_dev struct to >> store DMA mapping data to enhance its scope. For example during >> shutdown callback to unmap DMA mapping, these new struct members >> can be used as part of geni_se_tx_dma_unprep and >> geni_se_rx_dma_unprep calls. > > Please read about how to write commit text from kernel docs[1]. Hint, > use imperative mood. > Ok, will update the commit text. >> >> Signed-off-by: Roja Rani Yarubandi <rojay@codeaurora.org> >> --- >> drivers/i2c/busses/i2c-qcom-geni.c | 23 +++++++++++++---------- >> 1 file changed, 13 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c >> b/drivers/i2c/busses/i2c-qcom-geni.c >> index 7f130829bf01..53ca41f76080 100644 >> --- a/drivers/i2c/busses/i2c-qcom-geni.c >> +++ b/drivers/i2c/busses/i2c-qcom-geni.c >> @@ -86,6 +86,9 @@ struct geni_i2c_dev { >> u32 clk_freq_out; >> const struct geni_i2c_clk_fld *clk_fld; >> int suspended; >> + dma_addr_t tx_dma; >> + dma_addr_t rx_dma; >> + u32 xfer_len; > > Why not size_t? > Will change it to size_t. >> }; >> >> struct geni_i2c_err_log { >> @@ -352,12 +355,11 @@ static void geni_i2c_tx_fsm_rst(struct >> geni_i2c_dev *gi2c) >> static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct >> i2c_msg *msg, >> u32 m_param) >> { >> - dma_addr_t rx_dma; >> unsigned long time_left; >> void *dma_buf = NULL; >> struct geni_se *se = &gi2c->se; >> - size_t len = msg->len; >> >> + gi2c->xfer_len = msg->len; > > I'd prefer to keep the local variable and then have > > len = gi2c->xfer_len = msg->len; > Ok, will keep the local variable. >> if (!of_machine_is_compatible("lenovo,yoga-c630")) >> dma_buf = i2c_get_dma_safe_msg_buf(msg, 32); >> >> @@ -366,9 +368,10 @@ static int geni_i2c_rx_one_msg(struct >> geni_i2c_dev *gi2c, struct i2c_msg *msg, >> else >> geni_se_select_mode(se, GENI_SE_FIFO); >> >> - writel_relaxed(len, se->base + SE_I2C_RX_TRANS_LEN); >> + writel_relaxed(gi2c->xfer_len, se->base + >> SE_I2C_RX_TRANS_LEN); > > So that all this doesn't have to change. > >> >> - if (dma_buf && geni_se_rx_dma_prep(se, dma_buf, len, &rx_dma)) >> { >> + if (dma_buf && geni_se_rx_dma_prep(se, dma_buf, >> gi2c->xfer_len, >> + &gi2c->rx_dma)) { >> geni_se_select_mode(se, GENI_SE_FIFO); >> i2c_put_dma_safe_msg_buf(dma_buf, msg, false); >> dma_buf = NULL; >> @@ -384,7 +387,7 @@ static int geni_i2c_rx_one_msg(struct geni_i2c_dev >> *gi2c, struct i2c_msg *msg, >> if (dma_buf) { >> if (gi2c->err) >> geni_i2c_rx_fsm_rst(gi2c); >> - geni_se_rx_dma_unprep(se, rx_dma, len); >> + geni_se_rx_dma_unprep(se, gi2c->rx_dma, >> gi2c->xfer_len); >> i2c_put_dma_safe_msg_buf(dma_buf, msg, !gi2c->err); >> } >> >> @@ -394,12 +397,11 @@ static int geni_i2c_rx_one_msg(struct >> geni_i2c_dev *gi2c, struct i2c_msg *msg, >> static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct >> i2c_msg *msg, >> u32 m_param) >> { >> - dma_addr_t tx_dma; >> unsigned long time_left; >> void *dma_buf = NULL; >> struct geni_se *se = &gi2c->se; >> - size_t len = msg->len; >> >> + gi2c->xfer_len = msg->len; > > Same comment. > Ok. >> if (!of_machine_is_compatible("lenovo,yoga-c630")) >> dma_buf = i2c_get_dma_safe_msg_buf(msg, 32); >> > > [1] > https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes Thanks, Roja
diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c index 7f130829bf01..53ca41f76080 100644 --- a/drivers/i2c/busses/i2c-qcom-geni.c +++ b/drivers/i2c/busses/i2c-qcom-geni.c @@ -86,6 +86,9 @@ struct geni_i2c_dev { u32 clk_freq_out; const struct geni_i2c_clk_fld *clk_fld; int suspended; + dma_addr_t tx_dma; + dma_addr_t rx_dma; + u32 xfer_len; }; struct geni_i2c_err_log { @@ -352,12 +355,11 @@ static void geni_i2c_tx_fsm_rst(struct geni_i2c_dev *gi2c) static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg, u32 m_param) { - dma_addr_t rx_dma; unsigned long time_left; void *dma_buf = NULL; struct geni_se *se = &gi2c->se; - size_t len = msg->len; + gi2c->xfer_len = msg->len; if (!of_machine_is_compatible("lenovo,yoga-c630")) dma_buf = i2c_get_dma_safe_msg_buf(msg, 32); @@ -366,9 +368,10 @@ static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg, else geni_se_select_mode(se, GENI_SE_FIFO); - writel_relaxed(len, se->base + SE_I2C_RX_TRANS_LEN); + writel_relaxed(gi2c->xfer_len, se->base + SE_I2C_RX_TRANS_LEN); - if (dma_buf && geni_se_rx_dma_prep(se, dma_buf, len, &rx_dma)) { + if (dma_buf && geni_se_rx_dma_prep(se, dma_buf, gi2c->xfer_len, + &gi2c->rx_dma)) { geni_se_select_mode(se, GENI_SE_FIFO); i2c_put_dma_safe_msg_buf(dma_buf, msg, false); dma_buf = NULL; @@ -384,7 +387,7 @@ static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg, if (dma_buf) { if (gi2c->err) geni_i2c_rx_fsm_rst(gi2c); - geni_se_rx_dma_unprep(se, rx_dma, len); + geni_se_rx_dma_unprep(se, gi2c->rx_dma, gi2c->xfer_len); i2c_put_dma_safe_msg_buf(dma_buf, msg, !gi2c->err); } @@ -394,12 +397,11 @@ static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg, static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg, u32 m_param) { - dma_addr_t tx_dma; unsigned long time_left; void *dma_buf = NULL; struct geni_se *se = &gi2c->se; - size_t len = msg->len; + gi2c->xfer_len = msg->len; if (!of_machine_is_compatible("lenovo,yoga-c630")) dma_buf = i2c_get_dma_safe_msg_buf(msg, 32); @@ -408,9 +410,10 @@ static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg, else geni_se_select_mode(se, GENI_SE_FIFO); - writel_relaxed(len, se->base + SE_I2C_TX_TRANS_LEN); + writel_relaxed(gi2c->xfer_len, se->base + SE_I2C_TX_TRANS_LEN); - if (dma_buf && geni_se_tx_dma_prep(se, dma_buf, len, &tx_dma)) { + if (dma_buf && geni_se_tx_dma_prep(se, dma_buf, gi2c->xfer_len, + &gi2c->tx_dma)) { geni_se_select_mode(se, GENI_SE_FIFO); i2c_put_dma_safe_msg_buf(dma_buf, msg, false); dma_buf = NULL; @@ -429,7 +432,7 @@ static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg, if (dma_buf) { if (gi2c->err) geni_i2c_tx_fsm_rst(gi2c); - geni_se_tx_dma_unprep(se, tx_dma, len); + geni_se_tx_dma_unprep(se, gi2c->tx_dma, gi2c->xfer_len); i2c_put_dma_safe_msg_buf(dma_buf, msg, !gi2c->err); }
Adding tx_dma, rx_dma and xfer length in geni_i2c_dev struct to store DMA mapping data to enhance its scope. For example during shutdown callback to unmap DMA mapping, these new struct members can be used as part of geni_se_tx_dma_unprep and geni_se_rx_dma_unprep calls. Signed-off-by: Roja Rani Yarubandi <rojay@codeaurora.org> --- drivers/i2c/busses/i2c-qcom-geni.c | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-)