| 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
| ||
|
Message-ID: <000301d0c384$7fe8d890$7fba89b0$@codeaurora.org>
Date: Tue, 21 Jul 2015 12:41:37 +0530
From: "Sricharan" <sricharan@...eaurora.org>
To: "'Ivan T. Ivanov'" <iivanov@...sol.com>
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 Ivan,
> -----Original Message-----
> From: Ivan T. Ivanov [mailto:iivanov@...sol.com]
> Sent: Monday, July 20, 2015 3:14 PM
> To: Sricharan R
> 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.
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.
>
> >
> >
> > 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>
>
> > +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.
> > + 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".
Ok, will change.
>
> > + 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,
Sricharan
--
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/
Powered by blists - more mailing lists