Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1402752 > unrolled thread

RE: [PATCH v2 2/2] i2c: qup: support SMBus block read

Started by"Sricharan" <sricharan@codeaurora.org>
First post2016-05-18 09:10 +0200
Last post2016-05-23 19:50 +0200
Articles 6 — 4 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  RE: [PATCH v2 2/2] i2c: qup: support SMBus block read "Sricharan" <sricharan@codeaurora.org> - 2016-05-18 09:10 +0200
    Re: [PATCH v2 2/2] i2c: qup: support SMBus block read Naveen Kaje <nkaje@codeaurora.org> - 2016-05-19 22:20 +0200
      Re: [PATCH v2 2/2] i2c: qup: support SMBus block read Timur Tabi <timur@codeaurora.org> - 2016-05-19 22:30 +0200
        Re: [PATCH v2 2/2] i2c: qup: support SMBus block read Naveen Kaje <nkaje@codeaurora.org> - 2016-05-19 23:20 +0200
      RE: [PATCH v2 2/2] i2c: qup: support SMBus block read "Sricharan" <sricharan@codeaurora.org> - 2016-05-20 10:40 +0200
        Re: [PATCH v2 2/2] i2c: qup: support SMBus block read "Christ, Austin" <austinwc@codeaurora.org> - 2016-05-23 19:50 +0200

#1402752 — RE: [PATCH v2 2/2] i2c: qup: support SMBus block read

From"Sricharan" <sricharan@codeaurora.org>
Date2016-05-18 09:10 +0200
SubjectRE: [PATCH v2 2/2] i2c: qup: support SMBus block read
Message-ID<rA3Md-7f3-1@gated-at.bofh.it>
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

[toc] | [next] | [standalone]

#1403951

FromNaveen Kaje <nkaje@codeaurora.org>
Date2016-05-19 22:20 +0200
Message-ID<rACAh-4uH-7@gated-at.bofh.it>
In reply to#1402752
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
>

[toc] | [prev] | [next] | [standalone]

#1403954

FromTimur Tabi <timur@codeaurora.org>
Date2016-05-19 22:30 +0200
Message-ID<rACJX-4xR-1@gated-at.bofh.it>
In reply to#1403951
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.

[toc] | [prev] | [next] | [standalone]

#1403968

FromNaveen Kaje <nkaje@codeaurora.org>
Date2016-05-19 23:20 +0200
Message-ID<rADwm-55w-17@gated-at.bofh.it>
In reply to#1403954
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

[toc] | [prev] | [next] | [standalone]

#1404246

From"Sricharan" <sricharan@codeaurora.org>
Date2016-05-20 10:40 +0200
Message-ID<rAO8q-3ca-13@gated-at.bofh.it>
In reply to#1403951
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

[toc] | [prev] | [next] | [standalone]

#1405537

From"Christ, Austin" <austinwc@codeaurora.org>
Date2016-05-23 19:50 +0200
Message-ID<rC29j-u0-5@gated-at.bofh.it>
In reply to#1404246

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web