Re: [PATCH 1/2] i2c: i2c-qcom-geni: Add tx_dma, rx_dma and xfer_len to geni_i2c_dev struct

From: rojay@codeaurora.org
To: Stephen Boyd <swboyd@chromium.org>
Cc: wsa@kernel.org, dianders@chromium.org,
	saiprakash.ranjan@codeaurora.org, gregkh@linuxfoundation.org,
	mka@chromium.org, akashast@codeaurora.org,
	msavaliy@qti.qualcomm.com, skakit@codeaurora.org,
	rnayak@codeaurora.org, agross@kernel.org,
	bjorn.andersson@linaro.org, linux-arm-msm@vger.kernel.org,
	linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org,
	sumit.semwal@linaro.org, linux-media@vger.kernel.org,
	dri-devel@lists.freedesktop.org, linaro-mm-sig@lists.linaro.org
Subject: Re: [PATCH 1/2] i2c: i2c-qcom-geni: Add tx_dma, rx_dma and xfer_len to geni_i2c_dev struct
Date: Thu, 20 Aug 2020 15:59:54 +0530	[thread overview]
Message-ID: <872641764c4a03b92c8f2dafe6f2764a@codeaurora.org> (raw)
In-Reply-To: <159780835380.334488.10270114810481187992@swboyd.mtv.corp.google.com>

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