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

[Stephen Boyd]

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@xxxxxxxxxxxxxx>
> ---
>  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