Hi Ivan,

-----Original Message-----
From: Ivan T. Ivanov [mailto:iivanov at mm-sol.com]
Sent: Monday, July 20, 2015 3:14 PM
To: Sricharan R
Cc: devicetree at vger.kernel.org; linux-arm-msm at vger.kernel.org;
galak at codeaurora.org; linux-kernel at vger.kernel.org; linux-
i2c at vger.kernel.org; agross at codeaurora.org; dmaengine at vger.kernel.org;
linux-arm-kernel at lists.infradead.org
Subject: Re: [PATCH V4 3/7] i2c: qup: Add V2 tags support


Hi Sricharan,

On Thu, 2015-07-09 at 08:55 +0530, Sricharan R wrote:

quoted

QUP from version 2.1.1 onwards, supports a new format of i2c command
tags. Tag codes instructs the controller to perform a operation like
read/write. This new tagging version supports bam dma and transfers of
more than 256 bytes without 'stop'
in between. Adding the support for the same.

IIRC, more than 256 bytes in message is supported only in BAM(DMA) mode,
if this is true, please be more explicit in commit message.

   More than 256 byte read transfers are possible in V2 mode and not possible only
   in V1 mode.  So qup_i2c_quirks should be populated only for V1 mode and V2 mode
   during probe.

You haven't tried to read more than 256 bytes with this patch, right? See
qup_i2c_quirks ;-)

     Ya I have tested for 256 bytes transfer in my previous series, not on
     this one. My bad. Should have error'ed out with qup_i2c_quirks in place.
     Anyways will not populate qup_i2c_quirks for v2 mode.

quoted

 struct qup_i2c_dev {
        struct device*dev;
        void __iomem*base;

@@ -117,6 +138,7 @@ struct qup_i2c_dev {
        int     in_blk_sz;

        unsigned longone_byte_t;
+       struct qup_i2c_blockblk;

        struct i2c_msg*msg;
        /* Current posion in user message buffer */ @@ -126,6 +148,14

@@ struct qup_i2c_dev {
        /* QUP core errors */
        u32     qup_err;

+       int     use_v2_tags;
+
+       int (*qup_i2c_write_one)(struct qup_i2c_dev *qup,
+                                       struct i2c_msg *msg);
+
+       int (*qup_i2c_read_one)(struct qup_i2c_dev *qup,
+                               struct i2c_msg *msg);
+

Do we really need additional level of indirection?

We have separate struct i2c_algorithm, then we have common
qup_i2c_read/write methods and then we have different read/write sub
functions. I don't think 3-4 lines code reuse deserve increased complexity.

   Infact I was thinking this way as well. But anyways wanted to get reviewed
   and see which was better. Will change this.

<snip>

quoted

+static void qup_i2c_get_blk_data(struct qup_i2c_dev *qup,
+                                       struct i2c_msg *msg) {

This is more like "set_blk_metadata". Second argument could fit line above.

 Ok. Will change name and indentation.

quoted

+       memset(&qup->blk, 0, sizeof(qup->blk));
+
+       if (!qup->use_v2_tags) {
+               if (!(msg->flags & I2C_M_RD))
+                       qup->blk.tx_tag_len = 1;
+               return;
+       }
+
+       qup->blk.data_len = msg->len;
+       qup->blk.count = (msg->len + QUP_READ_LIMIT - 1) /
+ QUP_READ_LIMIT;
+
+       /* 4 bytes for first block and 2 writes for rest */
+       qup->blk.tx_tag_len = 4 + (qup->blk.count - 1) * 2;
+
+       /* There are 2 tag bytes that are read in to fifo for every block */
+       if (msg->flags & I2C_M_RD)
+               qup->blk.rx_tag_len = qup->blk.count * 2; }
+

<snip>

quoted

+static int qup_i2c_get_tags(u8 *tags, struct qup_i2c_dev *qup,
+                                                       struct i2c_msg
+*msg) {

This is more like "set_tags".

 Ok, will change.

quoted

+       u16 addr = (msg->addr << 1) | ((msg->flags & I2C_M_RD) ==

I2C_M_RD);

quoted

+       int len = 0;
+       int data_len;
+
+       if (qup->blk.pos == 0) {
+               tags[len++] = QUP_TAG_V2_START;
+               tags[len++] = addr & 0xff;
+
+               if (msg->flags & I2C_M_TEN)
+                       tags[len++] = addr >> 8;
+       }
+
+       /* Send _STOP commands for the last block */
+       if (qup->blk.pos == (qup->blk.count - 1)) {
+               if (msg->flags & I2C_M_RD)
+                       tags[len++] = QUP_TAG_V2_DATARD_STOP;
+               else
+                       tags[len++] = QUP_TAG_V2_DATAWR_STOP;
+       } else {
+               if (msg->flags & I2C_M_RD)
+                       tags[len++] = QUP_TAG_V2_DATARD;
+               else
+                       tags[len++] = QUP_TAG_V2_DATAWR;
+       }
+
+       data_len = qup_i2c_get_data_len(qup);
+
+       /* 0 implies 256 bytes */
+       if (data_len == QUP_READ_LIMIT)
+               tags[len++] = 0;
+       else
+               tags[len++] = data_len;
+
+       return len;
+}
+

Regards,
  Sricharan