Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753687AbbGTJoN (ORCPT ); Mon, 20 Jul 2015 05:44:13 -0400 Received: from ns.mm-sol.com ([37.157.136.199]:34535 "EHLO extserv.mm-sol.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751172AbbGTJoK (ORCPT ); Mon, 20 Jul 2015 05:44:10 -0400 Message-ID: <1437385443.6267.5.camel@mm-sol.com> Subject: Re: [PATCH V4 3/7] i2c: qup: Add V2 tags support From: "Ivan T. Ivanov" To: Sricharan R Cc: devicetree@vger.kernel.org, linux-arm-msm@vger.kernel.org, galak@codeaurora.org, linux-kernel@vger.kernel.org, linux-i2c@vger.kernel.org, agross@codeaurora.org, dmaengine@vger.kernel.org, linux-arm-kernel@lists.infradead.org Date: Mon, 20 Jul 2015 12:44:03 +0300 In-Reply-To: <1436412350-19519-4-git-send-email-sricharan@codeaurora.org> References: <1436412350-19519-1-git-send-email-sricharan@codeaurora.org> <1436412350-19519-4-git-send-email-sricharan@codeaurora.org> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.13.7-fta1.2~trusty Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4058 Lines: 130 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. You haven't tried to read more than 256 bytes with this patch, right? See qup_i2c_quirks ;-) > > > 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. > +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. > + 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; > +} > + > +static int qup_i2c_get_tags(u8 *tags, struct qup_i2c_dev *qup, > + struct i2c_msg *msg) > +{ This is more like "set_tags". > + 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, Ivan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/