* [PATCH v2 2/2] i2c: qup: support SMBus block read [not found] ` <1463006736-24652-2-git-send-email-nkaje@codeaurora.org> @ 2016-05-18 7:06 ` Sricharan 2016-05-19 20:14 ` Naveen Kaje 0 siblings, 1 reply; 6+ messages in thread From: Sricharan @ 2016-05-18 7:06 UTC (permalink / raw) To: linux-arm-kernel Hi, <snip..> > +static bool qup_i2c_check_msg_len(struct i2c_msg *msg) { > + return ((msg->flags & I2C_M_RD) && (msg->flags & > I2C_M_RECV_LEN)); } > + > +static int qup_i2c_set_tags_smb(u16 addr, u8 *tags, struct qup_i2c_dev > *qup, > + struct i2c_msg *msg) > +{ > + int len = 0; > + int data_len = qup_i2c_get_data_len(qup); > + > + if (msg->len > 1) { > + tags[len++] = QUP_TAG_V2_DATARD_STOP; > + tags[len++] = data_len - 1; > + } else { So we hit this else part for msg->len = 1 and this case blk.pos = 0 will always be true right ? > + 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; > + } > + > + tags[len++] = QUP_TAG_V2_DATARD; > + /* 0 implies 256 bytes */ > + if (data_len == QUP_READ_LIMIT) > + tags[len++] = 0; > + else > + tags[len++] = data_len; > + } Even data_len will always be '1' right ? > + return len; > +} > + > static int qup_i2c_set_tags(u8 *tags, struct qup_i2c_dev *qup, > struct i2c_msg *msg, int is_dma) { @@ -526,6 > +559,10 @@ static int qup_i2c_set_tags(u8 *tags, struct qup_i2c_dev *qup, > > int last = (qup->blk.pos == (qup->blk.count - 1)) && (qup->is_last); > > + /* Handle tags for SMBus block read */ > + if (qup_i2c_check_msg_len(msg)) > + return qup_i2c_set_tags_smb(addr, tags, qup, msg); > + > if (qup->blk.pos == 0) { > tags[len++] = QUP_TAG_V2_START; > tags[len++] = addr & 0xff; > @@ -1065,9 +1102,17 @@ static int qup_i2c_read_fifo_v2(struct > qup_i2c_dev *qup, > struct i2c_msg *msg) > { > u32 val; > - int idx, pos = 0, ret = 0, total; > + int idx, pos = 0, ret = 0, total, msg_offset = 0; > > + /* > + * If the message length is already read in > + * the first byte of the buffer, account for > + * that by setting the offset > + */ > + if (qup_i2c_check_msg_len(msg) && (msg->len > 1)) > + msg_offset = 1; > total = qup_i2c_get_data_len(qup); > + total -= msg_offset; > > /* 2 extra bytes for read tags */ > while (pos < (total + 2)) { > @@ -1087,8 +1132,8 @@ static int qup_i2c_read_fifo_v2(struct > qup_i2c_dev *qup, > > if (pos >= (total + 2)) > goto out; > - > - msg->buf[qup->pos++] = val & 0xff; > + msg->buf[qup->pos+msg_offset] = val & 0xff; A space around '+' ? > + qup->pos++; > } > } > > @@ -1128,6 +1173,22 @@ static int qup_i2c_read_one_v2(struct > qup_i2c_dev *qup, struct i2c_msg *msg) > goto err; > > qup->blk.pos++; > + > + /* Handle SMBus block read length */ > + if (qup_i2c_check_msg_len(msg) && (msg->len == 1)) { > + if (msg->buf[0] > I2C_SMBUS_BLOCK_MAX) { > + ret = -EPROTO; > + goto err; > + } > + msg->len += msg->buf[0]; > + qup->pos = 0; > + qup_i2c_set_read_mode_v2(qup, msg->len); > + qup_i2c_issue_xfer_v2(qup, msg); > + ret = qup_i2c_wait_for_complete(qup, msg); > + if (ret) > + goto err; Is the issue_xfer_v2 needed inside this here ? > + qup_i2c_set_blk_data(qup, msg); > + } > } while (qup->blk.pos < qup->blk.count); Regards, Sricharan ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 2/2] i2c: qup: support SMBus block read 2016-05-18 7:06 ` [PATCH v2 2/2] i2c: qup: support SMBus block read Sricharan @ 2016-05-19 20:14 ` Naveen Kaje 2016-05-19 20:21 ` Timur Tabi 2016-05-20 8:31 ` Sricharan 0 siblings, 2 replies; 6+ messages in thread From: Naveen Kaje @ 2016-05-19 20:14 UTC (permalink / raw) To: linux-arm-kernel Hi Sricharan, On 5/18/2016 1:06 AM, Sricharan wrote: > Hi, > > <snip..> > >> +static bool qup_i2c_check_msg_len(struct i2c_msg *msg) { >> + return ((msg->flags & I2C_M_RD) && (msg->flags & >> I2C_M_RECV_LEN)); } >> + >> +static int qup_i2c_set_tags_smb(u16 addr, u8 *tags, struct qup_i2c_dev >> *qup, >> + struct i2c_msg *msg) >> +{ >> + int len = 0; >> + int data_len = qup_i2c_get_data_len(qup); >> + >> + if (msg->len > 1) { >> + tags[len++] = QUP_TAG_V2_DATARD_STOP; >> + tags[len++] = data_len - 1; >> + } else { > So we hit this else part for msg->len = 1 and this case blk.pos = 0 > will always be true right ? Yes, will fix that in V3. > >> + 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; >> + } >> + >> + tags[len++] = QUP_TAG_V2_DATARD; >> + /* 0 implies 256 bytes */ >> + if (data_len == QUP_READ_LIMIT) >> + tags[len++] = 0; >> + else >> + tags[len++] = data_len; >> + } > Even data_len will always be '1' right ? Yes, but here preferably we use a variable than a number without a context. >> + return len; >> +} >> + >> static int qup_i2c_set_tags(u8 *tags, struct qup_i2c_dev *qup, >> struct i2c_msg *msg, int is_dma) { @@ -526,6 >> +559,10 @@ static int qup_i2c_set_tags(u8 *tags, struct qup_i2c_dev *qup, >> >> int last = (qup->blk.pos == (qup->blk.count - 1)) && (qup->is_last); >> >> + /* Handle tags for SMBus block read */ >> + if (qup_i2c_check_msg_len(msg)) >> + return qup_i2c_set_tags_smb(addr, tags, qup, msg); >> + >> if (qup->blk.pos == 0) { >> tags[len++] = QUP_TAG_V2_START; >> tags[len++] = addr & 0xff; >> @@ -1065,9 +1102,17 @@ static int qup_i2c_read_fifo_v2(struct >> qup_i2c_dev *qup, >> struct i2c_msg *msg) >> { >> u32 val; >> - int idx, pos = 0, ret = 0, total; >> + int idx, pos = 0, ret = 0, total, msg_offset = 0; >> >> + /* >> + * If the message length is already read in >> + * the first byte of the buffer, account for >> + * that by setting the offset >> + */ >> + if (qup_i2c_check_msg_len(msg) && (msg->len > 1)) >> + msg_offset = 1; >> total = qup_i2c_get_data_len(qup); >> + total -= msg_offset; >> >> /* 2 extra bytes for read tags */ >> while (pos < (total + 2)) { >> @@ -1087,8 +1132,8 @@ static int qup_i2c_read_fifo_v2(struct >> qup_i2c_dev *qup, >> >> if (pos >= (total + 2)) >> goto out; >> - >> - msg->buf[qup->pos++] = val & 0xff; >> + msg->buf[qup->pos+msg_offset] = val & 0xff; > A space around '+' ? Thanks, will fix that. > >> + qup->pos++; >> } >> } >> >> @@ -1128,6 +1173,22 @@ static int qup_i2c_read_one_v2(struct >> qup_i2c_dev *qup, struct i2c_msg *msg) >> goto err; >> >> qup->blk.pos++; >> + >> + /* Handle SMBus block read length */ >> + if (qup_i2c_check_msg_len(msg) && (msg->len == 1)) { >> + if (msg->buf[0] > I2C_SMBUS_BLOCK_MAX) { >> + ret = -EPROTO; >> + goto err; >> + } >> + msg->len += msg->buf[0]; >> + qup->pos = 0; >> + qup_i2c_set_read_mode_v2(qup, msg->len); >> + qup_i2c_issue_xfer_v2(qup, msg); >> + ret = qup_i2c_wait_for_complete(qup, msg); >> + if (ret) >> + goto err; > Is the issue_xfer_v2 needed inside this here ? No, qup_i2c_issue_xfer_v2 is needed so that we read rest of the data that is indicated by the length we read earlier. > >> + qup_i2c_set_blk_data(qup, msg); >> + } >> } while (qup->blk.pos < qup->blk.count); > Regards, > Sricharan Thanks, Naveen > ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 2/2] i2c: qup: support SMBus block read 2016-05-19 20:14 ` Naveen Kaje @ 2016-05-19 20:21 ` Timur Tabi 2016-05-19 21:16 ` Naveen Kaje 2016-05-20 8:31 ` Sricharan 1 sibling, 1 reply; 6+ messages in thread From: Timur Tabi @ 2016-05-19 20:21 UTC (permalink / raw) To: linux-arm-kernel Naveen Kaje wrote: >>> >>> + tags[len++] = QUP_TAG_V2_DATARD; >>> + /* 0 implies 256 bytes */ >>> + if (data_len == QUP_READ_LIMIT) >>> + tags[len++] = 0; >>> + else >>> + tags[len++] = data_len; >>> + } >> Even data_len will always be '1' right ? > Yes, but here preferably we use a variable than a number without a context. Actually, I would say the opposite. I would rather see a constant with a comment explaining it, than a variable that we know will always contain only one number. -- Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation collaborative project. ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 2/2] i2c: qup: support SMBus block read 2016-05-19 20:21 ` Timur Tabi @ 2016-05-19 21:16 ` Naveen Kaje 0 siblings, 0 replies; 6+ messages in thread From: Naveen Kaje @ 2016-05-19 21:16 UTC (permalink / raw) To: linux-arm-kernel Hi Timur, Sricharan, On 5/19/2016 2:21 PM, Timur Tabi wrote: > Naveen Kaje wrote: >>>> >>>> + tags[len++] = QUP_TAG_V2_DATARD; >>>> + /* 0 implies 256 bytes */ >>>> + if (data_len == QUP_READ_LIMIT) >>>> + tags[len++] = 0; >>>> + else >>>> + tags[len++] = data_len; >>>> + } >>> Even data_len will always be '1' right ? >> Yes, but here preferably we use a variable than a number without a >> context. > > Actually, I would say the opposite. I would rather see a constant > with a comment explaining it, than a variable that we know will always > contain only one number. > Ok, got it. Will be fixed in V3. Thanks, Naveen ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 2/2] i2c: qup: support SMBus block read 2016-05-19 20:14 ` Naveen Kaje 2016-05-19 20:21 ` Timur Tabi @ 2016-05-20 8:31 ` Sricharan 2016-05-23 17:45 ` Christ, Austin 1 sibling, 1 reply; 6+ messages in thread From: Sricharan @ 2016-05-20 8:31 UTC (permalink / raw) To: linux-arm-kernel Hi, <snip..> >>> @@ -1128,6 +1173,22 @@ static int qup_i2c_read_one_v2(struct >>> qup_i2c_dev *qup, struct i2c_msg *msg) >>> goto err; >>> >>> qup->blk.pos++; >>> + >>> + /* Handle SMBus block read length */ >>> + if (qup_i2c_check_msg_len(msg) && (msg->len == 1)) { >>> + if (msg->buf[0] > I2C_SMBUS_BLOCK_MAX) { >>> + ret = -EPROTO; >>> + goto err; >>> + } >>> + msg->len += msg->buf[0]; >>> + qup->pos = 0; >>> + qup_i2c_set_read_mode_v2(qup, msg->len); >>> + qup_i2c_issue_xfer_v2(qup, msg); >>> + ret = qup_i2c_wait_for_complete(qup, msg); >>> + if (ret) >>> + goto err; >> Is the issue_xfer_v2 needed inside this here ? >No, qup_i2c_issue_xfer_v2 is needed so that we read rest of the data >that is indicated by the length we read earlier. So qup_i2c_issue_xfer_v2 writes the tags and there is one already in the loop above this check. So if you just do qup_i2c_set_read_mode_v2 and qup_i2c_set_blk_data inside this check, will not be enough ? Regards, Sricharan ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 2/2] i2c: qup: support SMBus block read 2016-05-20 8:31 ` Sricharan @ 2016-05-23 17:45 ` Christ, Austin 0 siblings, 0 replies; 6+ messages in thread From: Christ, Austin @ 2016-05-23 17:45 UTC (permalink / raw) To: linux-arm-kernel On 5/20/2016 2:31 AM, Sricharan wrote: > Hi, > > <snip..> >>>> @@ -1128,6 +1173,22 @@ static int qup_i2c_read_one_v2(struct >>>> qup_i2c_dev *qup, struct i2c_msg *msg) >>>> goto err; >>>> >>>> qup->blk.pos++; >>>> + >>>> + /* Handle SMBus block read length */ >>>> + if (qup_i2c_check_msg_len(msg) && (msg->len == 1)) { >>>> + if (msg->buf[0] > I2C_SMBUS_BLOCK_MAX) { >>>> + ret = -EPROTO; >>>> + goto err; >>>> + } >>>> + msg->len += msg->buf[0]; >>>> + qup->pos = 0; >>>> + qup_i2c_set_read_mode_v2(qup, msg->len); >>>> + qup_i2c_issue_xfer_v2(qup, msg); >>>> + ret = qup_i2c_wait_for_complete(qup, msg); >>>> + if (ret) >>>> + goto err; >>> Is the issue_xfer_v2 needed inside this here ? >> No, qup_i2c_issue_xfer_v2 is needed so that we read rest of the data >> that is indicated by the length we read earlier. > So qup_i2c_issue_xfer_v2 writes the tags and there is one already in the loop above this > check. So if you just do qup_i2c_set_read_mode_v2 and qup_i2c_set_blk_data inside this > check, will not be enough ? > > Regards, > Sricharan > In testing, removing the call to qup_i2c_issue_xfer_v2() within the conditional statement for SMBus length causes failures. That transfer request is needed to read that block of data. Thanks, Austin ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-05-23 17:45 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1463006736-24652-1-git-send-email-nkaje@codeaurora.org>
[not found] ` <1463006736-24652-2-git-send-email-nkaje@codeaurora.org>
2016-05-18 7:06 ` [PATCH v2 2/2] i2c: qup: support SMBus block read Sricharan
2016-05-19 20:14 ` Naveen Kaje
2016-05-19 20:21 ` Timur Tabi
2016-05-19 21:16 ` Naveen Kaje
2016-05-20 8:31 ` Sricharan
2016-05-23 17:45 ` Christ, Austin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).