* [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).