Re: [PATCH V4 3/7] i2c: qup: Add V2 tags support

["Ivan T. Ivanov" <iivanov@mm-sol.com>]

Hi Sricharan,

On Thu, 2015-07-09 at 08:55 +0530, Sricharan R wrote:
> 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.

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

> 
> 
>  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.

<snip>

> +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.

> +       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>

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

This is more like "set_tags".

> +       u16 addr = (msg->addr << 1) | ((msg->flags & I2C_M_RD) == I2C_M_RD);
> +       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,
Ivan