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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 20 Jul 2015 12:44:03 +0300
From:	"Ivan T. Ivanov" <iivanov@...sol.com>
To:	Sricharan R <sricharan@...eaurora.org>
Cc:	devicetree@...r.kernel.org, linux-arm-msm@...r.kernel.org,
	galak@...eaurora.org, linux-kernel@...r.kernel.org,
	linux-i2c@...r.kernel.org, agross@...eaurora.org,
	dmaengine@...r.kernel.org, linux-arm-kernel@...ts.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:
> 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/