linux-kernel - Re: [PATCH] i2c: mediatek: Add to support continuous mode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Date:   Tue, 30 Jun 2020 15:33:39 +0800
From:   Yingjoe Chen <yingjoe.chen@...iatek.com>
To:     Qiangming Xia <qiangming.xia@...iatek.com>
CC:     <wsa@...-dreams.de>,
        Wolfram Sang <wsa+renesas@...g-engineering.com>,
        "Qii Wang" <Qii.Wang@...iatek.com>, <linux-i2c@...r.kernel.org>,
        <devicetree@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>,
        <linux-mediatek@...ts.infradead.org>, <srv_heupstream@...iatek.com>
Subject: Re: [PATCH] i2c: mediatek: Add to support continuous mode

Hi Qiangming,

When you send new version, you should

- Add version number in subject, so we know this is a new one.
- List what's changed in this patch after ---,  so reviewer knows where
we should check. 


On Tue, 2020-06-23 at 15:42 +0800, Qiangming Xia wrote:
> From: "qiangming.xia" <qiangming.xia@...iatek.com>
> 
>     Mediatek i2c controller support for continuous mode,
> it allow to transfer once multiple writing messages of equal
> length.
>     A i2c slave sometimes need write a serial of non-continuous
> offset range in chip,e.g. camera sensor imx586,imx576. It need
> transfer 294 writing messages when initiate setting. It can use
> this mode to improve performance.
> 
> Change-Id: If473d96d2b76e9d51f20741a9380db4fcad15dbd

Remove this.


> Signed-off-by: Qiangming Xia <qiangming.xia@...iatek.com>
> ---
>  drivers/i2c/busses/i2c-mt65xx.c | 66 +++++++++++++++++++++++++++++++++
>  1 file changed, 66 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-mt65xx.c b/drivers/i2c/busses/i2c-mt65xx.c
> index deef69e56906..108bca1a4042 100644
> --- a/drivers/i2c/busses/i2c-mt65xx.c
> +++ b/drivers/i2c/busses/i2c-mt65xx.c
> @@ -97,6 +97,7 @@ enum mtk_trans_op {
>  	I2C_MASTER_WR = 1,
>  	I2C_MASTER_RD,
>  	I2C_MASTER_WRRD,
> +	I2C_MASTER_CONTINUOUS_WR,
>  };
>  
>  enum I2C_REGS_OFFSET {
> @@ -846,6 +847,9 @@ static int mtk_i2c_do_transfer(struct mtk_i2c *i2c, struct i2c_msg *msgs,
>  					    OFFSET_TRANSFER_LEN);
>  		}
>  		mtk_i2c_writew(i2c, I2C_WRRD_TRANAC_VALUE, OFFSET_TRANSAC_LEN);
> +	} else if (i2c->op == I2C_MASTER_CONTINUOUS_WR) {
> +		mtk_i2c_writew(i2c, msgs->len / num, OFFSET_TRANSFER_LEN);
> +		mtk_i2c_writew(i2c, num, OFFSET_TRANSAC_LEN);
>  	} else {
>  		mtk_i2c_writew(i2c, msgs->len, OFFSET_TRANSFER_LEN);
>  		mtk_i2c_writew(i2c, num, OFFSET_TRANSAC_LEN);
> @@ -896,6 +900,23 @@ static int mtk_i2c_do_transfer(struct mtk_i2c *i2c, struct i2c_msg *msgs,
>  			writel(reg_4g_mode, i2c->pdmabase + OFFSET_TX_4G_MODE);
>  		}
>  
> +		writel((u32)wpaddr, i2c->pdmabase + OFFSET_TX_MEM_ADDR);
> +		writel(msgs->len, i2c->pdmabase + OFFSET_TX_LEN);
> +	} else if (i2c->op == I2C_MASTER_CONTINUOUS_WR) {
> +		writel(I2C_DMA_INT_FLAG_NONE, i2c->pdmabase + OFFSET_INT_FLAG);
> +		writel(I2C_DMA_CON_TX, i2c->pdmabase + OFFSET_CON);
> +		wpaddr = dma_map_single(i2c->dev, msgs->buf,
> +					msgs->len, DMA_TO_DEVICE);
> +		if (dma_mapping_error(i2c->dev, wpaddr)) {
> +			kfree(msgs->buf);
> +			return -ENOMEM;
> +		}
> +
> +		if (i2c->dev_comp->support_33bits) {
> +			reg_4g_mode = mtk_i2c_set_4g_mode(wpaddr);
> +			writel(reg_4g_mode, i2c->pdmabase + OFFSET_TX_4G_MODE);
> +		}
> +
>  		writel((u32)wpaddr, i2c->pdmabase + OFFSET_TX_MEM_ADDR);
>  		writel(msgs->len, i2c->pdmabase + OFFSET_TX_LEN);
>  	} else {
> @@ -979,6 +1000,11 @@ static int mtk_i2c_do_transfer(struct mtk_i2c *i2c, struct i2c_msg *msgs,
>  				 msgs->len, DMA_FROM_DEVICE);
>  
>  		i2c_put_dma_safe_msg_buf(dma_rd_buf, msgs, true);
> +	} else if (i2c->op == I2C_MASTER_CONTINUOUS_WR) {
> +		dma_unmap_single(i2c->dev, wpaddr,
> +				 msgs->len, DMA_TO_DEVICE);
> +
> +		kfree(msgs->buf);
>  	} else {
>  		dma_unmap_single(i2c->dev, wpaddr, msgs->len,
>  				 DMA_TO_DEVICE);
> @@ -1009,6 +1035,9 @@ static int mtk_i2c_transfer(struct i2c_adapter *adap,
>  {
>  	int ret;
>  	int left_num = num;
> +	int i, j;
> +	u8 *dma_multi_wr_buf;
> +	struct i2c_msg multi_msg[1];
>  	struct mtk_i2c *i2c = i2c_get_adapdata(adap);
>  
>  	ret = mtk_i2c_clock_enable(i2c);
> @@ -1025,6 +1054,43 @@ static int mtk_i2c_transfer(struct i2c_adapter *adap,
>  		}
>  	}
>  
> +	if (num > 1 && !(msgs[0].flags & I2C_M_RD)) {
> +		for (i = 0; i < num - 1; i++) {
> +			if (!(msgs[i+1].flags & I2C_M_RD) &&
> +				(msgs[i].addr == msgs[i+1].addr)
> +					&& (msgs[i].len == msgs[i+1].len)) {
> +				continue;

Don't need () for addr/len check.
When wrap, please put operator at end of previous line.
The 2nd line of if is at the same indent level with continue, that
doesn't look good. Let's use 4 spaces as indent for addr and len check, 
so this should be:

	if (num > 1 && !(msgs[0].flags & I2C_M_RD)) {
		for (i = 0; i < num - 1; i++) {
			if (!(msgs[i+1].flags &	I2C_M_RD) && 
			    msgs[i].addr == msgs[i+1].addr &&
			    msgs[i].len == msgs[i+1].len) {


Joe.C