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

[Naveen Kaje]

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
>