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
