lkml.org 
[lkml]   [2020]   [Aug]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/2] i2c: i2c-qcom-geni: Add tx_dma, rx_dma and xfer_len to geni_i2c_dev struct
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

\
 
 \ /
  Last update: 2020-08-20 12:31    [W:0.987 / U:0.024 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site