Re: [PATCH v6] i2c: qcom-geni: Add support for GPI DMA

[Vinod Koul]

On 18-02-22, 11:07, Bjorn Andersson wrote:
> On Fri 18 Feb 10:55 PST 2022, Vinod Koul wrote:
> 
> > QUP Serial engines supports data transfers thru FIFO mode, SE DMA mode
> > and lastly GPI DMA mode. Former two are already supported and this adds
> > supports for the last mode.
> > 
> > In GPI DMA mode, the firmware is issued commands by driver to perform
> > DMA and setup the serial port.
> > 
> > Signed-off-by: Vinod Koul <vkoul@xxxxxxxxxx>
> > ---
> > 
> > Changes since v5:
> >  - Fix comments by Bjorn, Dmitry and Jordan
> 
> Thanks, but it's useful to know what those things where. Please list the
> actual changes.

My bad or rather lazy to post late in night, here it is for record:
 - Fix the order of remove
 - make sure completion is called when status is !NOERROR
 - dont reset dma channel on exit
 - Make sure to return num on scucess for i2c xfer for both fifo and gpi
   mode

> 
> > 
> > Changes since v4:
> >  - Fix buildbot warn
> >  - Fix flase warn reported by Alexey
> >  - Fix feedback from Bjorn and cleanup the probe code and add more details
> >    in changelog
> > 
> > Changes since v3:
> >  - remove separate tx and rx function for gsi dma and make a common one
> >  - remove global structs and use local variables instead
> > 
> > 
> >  drivers/i2c/busses/i2c-qcom-geni.c | 303 ++++++++++++++++++++++++++---
> >  1 file changed, 275 insertions(+), 28 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> > index 6d635a7c104c..e03544ba827c 100644
> > --- a/drivers/i2c/busses/i2c-qcom-geni.c
> > +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> > @@ -3,7 +3,9 @@
> >  
> >  #include <linux/acpi.h>
> >  #include <linux/clk.h>
> > +#include <linux/dmaengine.h>
> >  #include <linux/dma-mapping.h>
> > +#include <linux/dma/qcom-gpi-dma.h>
> >  #include <linux/err.h>
> >  #include <linux/i2c.h>
> >  #include <linux/interrupt.h>
> > @@ -48,6 +50,9 @@
> >  #define LOW_COUNTER_SHFT	10
> >  #define CYCLE_COUNTER_MSK	GENMASK(9, 0)
> >  
> > +#define I2C_PACK_TX		BIT(0)
> > +#define I2C_PACK_RX		BIT(1)
> > +
> >  enum geni_i2c_err_code {
> >  	GP_IRQ0,
> >  	NACK,
> > @@ -89,6 +94,9 @@ struct geni_i2c_dev {
> >  	void *dma_buf;
> >  	size_t xfer_len;
> >  	dma_addr_t dma_addr;
> > +	struct dma_chan *tx_c;
> > +	struct dma_chan *rx_c;
> > +	bool gpi_mode;
> >  };
> >  
> >  struct geni_i2c_err_log {
> > @@ -456,12 +464,202 @@ static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
> >  	return gi2c->err;
> >  }
> >  
> > +static void i2c_gpi_cb_result(void *cb, const struct dmaengine_result *result)
> > +{
> > +	struct geni_i2c_dev *gi2c = cb;
> > +
> > +	if (result->result != DMA_TRANS_NOERROR) {
> > +		dev_err(gi2c->se.dev, "DMA txn failed:%d\n", result->result);
> > +		goto err_trans;
> 
> Maybe I'm just missing it, but aren't you completing done now to avoid
> the timeout, which causes the xfer to return success?
> 
> Note that, as I mentioned in previous review, we're under the adaptor
> lock, so I don't think you need any additional locking to pass a value
> back to xfer().

Yes we should pass the err value, will add that up in gi2c to pass
around...

> 
> > +	}
> > +
> > +	if (result->residue)
> 
> You can avoid the goto, and make it clear from the overall structure that
> only one (or none) of these paths will be taken.
> 
> 	if (NOERROR) {
> 
> 	} else if (result->residue) {
> 
> 	}

Sure

-- 
~Vinod