Hello,

On Fri, Jan 16, 2015 at 06:33:38PM +0800, Eddie Huang wrote:

+config I2C_MT65XX
+	tristate "MediaTek I2C adapter"
+	depends on ARCH_MEDIATEK

depends on ARCH_MEDIATEK || COMPILE_TEST
default ARCH_MEDIATEK

would be nice to have to get better compile coverage.

+struct mtk_i2c {
+	struct i2c_adapter adap;	/* i2c host adapter */
+	struct device *dev;
+	wait_queue_head_t wait;		/* i2c transfer wait queue */
+	/* set in i2c probe */
+	void __iomem *base;		/* i2c base addr */
+	void __iomem *pdmabase;		/* dma base address*/
+	int irqnr;			/* i2c interrupt number */

irqs are unsigned quantities

+	struct i2c_dma_buf dma_buf;	/* memory alloc for DMA mode */
+	struct clk *clk_main;		/* main clock for i2c bus */
+	struct clk *clk_dma;		/* DMA clock for i2c via DMA */
+	struct clk *clk_pmic;		/* PMIC clock for i2c from PMIC */
+	bool have_pmic;			/* can use i2c pins from PMIC */
+	bool use_push_pull;		/* IO config push-pull mode */
+	u32 platform_compat;		/* platform compatible data */
+	/* set when doing the transfer */
+	u16 irq_stat;			/* interrupt status */
+	unsigned int speed_hz;		/* The speed in transfer */
+	bool trans_stop;		/* i2c transfer stop */
+	enum mtk_trans_op op;
+	u16 msg_len;
+	u8 *msg_buf;			/* pointer to msg data */
+	u16 msg_aux_len;		/* WRRD mode to set AUX_LEN register*/
+	u16 addr;	/* 7bit slave address, without read/write bit */

Wouldn't it be easier to maintain a pointer to the message to be
transferred?

+	u16 timing_reg;
+	u16 high_speed_reg;
+};
+
+static const struct of_device_id mtk_i2c_of_match[] = {
+	{ .compatible = "mediatek,mt6577-i2c", .data = (void *)COMPAT_MT6577 },
+	{ .compatible = "mediatek,mt6589-i2c", .data = (void *)COMPAT_MT6589 },
+	{},

There is usually no , after the sentinel entry.

+};
+MODULE_DEVICE_TABLE(of, mtk_i2c_match);
+
+static inline void i2c_writew(u16 value, struct mtk_i2c *i2c, u8 offset)
+{
+	writew(value, i2c->base + offset);
+}

hmm, these simple wrappers are fine in general for me because they might
ease debugging or changing the accessor-function. Still "i2c_writew"
sounds too generic. IMHO you should spend the few chars to make this
mtk_i2c_writew. And to match my taste completely, move the driver data
parameter to the front. (But there are too much different tastes out
there to really request a certain style here.) Same for the other
wrappers of course.

+	/* Prepare buffer data to start transfer */
+	if (i2c->op == I2C_MASTER_RD) {
+		i2c_writel_dma(I2C_DMA_INT_FLAG_NONE, i2c, OFFSET_INT_FLAG);
+		i2c_writel_dma(I2C_DMA_CON_RX, i2c, OFFSET_CON);
+		i2c_writel_dma((u32)i2c->msg_buf, i2c, OFFSET_RX_MEM_ADDR);
+		i2c_writel_dma(i2c->msg_len, i2c, OFFSET_RX_LEN);
+	} else if (i2c->op == I2C_MASTER_WR) {
+		i2c_writel_dma(I2C_DMA_INT_FLAG_NONE, i2c, OFFSET_INT_FLAG);
+		i2c_writel_dma(I2C_DMA_CON_TX, i2c, OFFSET_CON);
+		i2c_writel_dma((u32)i2c->msg_buf, i2c, OFFSET_TX_MEM_ADDR);
+		i2c_writel_dma(i2c->msg_len, i2c, OFFSET_TX_LEN);
+	} else {

		/* i2c->op == I2C_MASTER_WRRD */

+		i2c_writel_dma(I2C_DMA_CLR_FLAG, i2c, OFFSET_INT_FLAG);
+		i2c_writel_dma(I2C_DMA_CLR_FLAG, i2c, OFFSET_CON);
+		i2c_writel_dma((u32)i2c->msg_buf, i2c, OFFSET_TX_MEM_ADDR);
+		i2c_writel_dma((u32)i2c->dma_buf.paddr, i2c,
+			OFFSET_RX_MEM_ADDR);
+		i2c_writel_dma(i2c->msg_len, i2c, OFFSET_TX_LEN);
+		i2c_writel_dma(i2c->msg_aux_len, i2c, OFFSET_RX_LEN);
+	}
+
+	/* flush before sending start */
+	mb();
+	i2c_writel_dma(I2C_DMA_START_EN, i2c, OFFSET_EN);
+	i2c_writew(I2C_TRANSAC_START, i2c, OFFSET_START);
+
+	tmo = wait_event_timeout(i2c->wait, i2c->trans_stop, tmo);

If the request completes just when wait_event_timeout returned 0 you
shouldn't throw it away.

+	if (tmo == 0) {
+		dev_dbg(i2c->dev, "addr: %x, transfer timeout\n", i2c->addr);
+		mtk_i2c_init_hw(i2c);
+		return -ETIMEDOUT;
+	}
[...]
+	if (msgs->addr == 0) {
+		dev_dbg(i2c->dev, " addr is invalid.\n");

Is this a hardware limitation?

I'd remove the leading space in the message. (Applies also to other
places.)

+		ret = -EINVAL;
+		goto err_exit;
+	}
+
+	if (msgs->buf == NULL) {
+		dev_dbg(i2c->dev, " data buffer is NULL.\n");
+		ret = -EINVAL;
+		goto err_exit;
+	}
+
+	i2c->addr = msgs->addr;
+	i2c->msg_len = msgs->len;
+	i2c->msg_buf = msgs->buf;
+
+	if (msgs->flags & I2C_M_RD)
+		i2c->op = I2C_MASTER_RD;
+	else
+		i2c->op = I2C_MASTER_WR;
+
+	/* combined two messages into one transaction */
+	if (num > 1) {
+		i2c->msg_aux_len = (msgs + 1)->len;
+		i2c->op = I2C_MASTER_WRRD;
+	}

This means "write then read", right? You should check here that the
first message is really a write and the 2nd a read then.
Can this happen at all with the quirks defined below (.max_num_msgs =
1)?

+	/*
+	 * always use DMA mode.

Out of interest: Did you benchmark DMA vs manual mode for a typical
(what ever that means) scenario?

+	 * 1st when write need copy the data of message to dma memory
+	 * 2nd when read need copy the DMA data to the message buffer.
+	 */
+	mtk_i2c_copy_to_dma(i2c, msgs);
+	i2c->msg_buf = (u8 *)i2c->dma_buf.paddr;
+	ret = mtk_i2c_do_transfer(i2c);
+	if (ret < 0)
+		goto err_exit;
+
+	if (i2c->op == I2C_MASTER_WRRD)
+		mtk_i2c_copy_from_dma(i2c, msgs + 1);
+	else
+		mtk_i2c_copy_from_dma(i2c, msgs);
+
+	/* the return value is number of executed messages */
+	ret = num;
+
+err_exit:
+	mtk_i2c_clock_disable(i2c);
+	return ret;
+}
+
+static irqreturn_t mtk_i2c_irq(int irqno, void *dev_id)
+{
+	struct mtk_i2c *i2c = dev_id;
+
+	/* Clear interrupt mask */
+	i2c_writew(~(I2C_HS_NACKERR | I2C_ACKERR | I2C_TRANSAC_COMP),
+		i2c, OFFSET_INTR_MASK);

What is the effect of this. Does it disable further irqs?

+	i2c->irq_stat = i2c_readw(i2c, OFFSET_INTR_STAT);

Maybe you need locking here to modify your driver data in the irq?

+	i2c_writew(I2C_HS_NACKERR | I2C_ACKERR | I2C_TRANSAC_COMP,
+		i2c, OFFSET_INTR_STAT);

This is the ack? Then this is racy; I guess you want

	i2c_writew(i2c->irq_stat & (I2C_HS_NACKERR | I2C_ACKERR ...),
		   i2c, OFFSET_INTR_STAT);

then.

+	i2c->trans_stop = true;
+	wake_up(&i2c->wait);
+
+	return IRQ_HANDLED;
+}
+
+static u32 mtk_i2c_functionality(struct i2c_adapter *adap)
+{
+	return I2C_FUNC_I2C | I2C_FUNC_10BIT_ADDR | I2C_FUNC_SMBUS_EMUL;

Does your hardware handle 10bit addresses that nice that there is
nothing visible in the driver apart from this functionality
announcement?

+}
+
+static const struct i2c_algorithm mtk_i2c_algorithm = {
+	.master_xfer = mtk_i2c_transfer,
+	.functionality = mtk_i2c_functionality,
+};
+
+static inline u32 mtk_get_device_prop(struct platform_device *pdev)
+{
+	const struct of_device_id *match;
+
+	match = of_match_node(mtk_i2c_of_match, pdev->dev.of_node);
+	return (u32)match->data;
+}
+
+static int mtk_i2c_parse_dt(struct device_node *np, struct mtk_i2c *i2c,
+	unsigned int *clk_src_div)
+{
+	i2c->speed_hz = I2C_DEFAUT_SPEED;
+	of_property_read_u32(np, "clock-frequency", &i2c->speed_hz);
+	of_property_read_u32(np, "clock-div", clk_src_div);

You should check the return value of of_property_read_u32.

[...]
+	ret = devm_request_irq(&pdev->dev, i2c->irqnr, mtk_i2c_irq,
+		IRQF_TRIGGER_NONE, I2C_DRV_NAME, i2c);
+	if (ret < 0) {
+		dev_err(&pdev->dev,
+			"Request I2C IRQ %d fail\n", i2c->irqnr);
+		return ret;
+	}

I think the devm_request_irq should go near the end of probing.
Otherwise the irq might fire before the used resources are ready.

+
+	i2c->adap.dev.of_node = pdev->dev.of_node;
+	i2c->dev = &i2c->adap.dev;
+	i2c->adap.dev.parent = &pdev->dev;
+	i2c->adap.owner = THIS_MODULE;
+	i2c->adap.algo = &mtk_i2c_algorithm;
+	i2c->adap.quirks = &mt6577_i2c_quirks;
+	i2c->adap.algo_data = NULL;

No need to initialize this to NULL as the struct was allocated using
kzalloc.

+	i2c->adap.timeout = 2 * HZ;
+	i2c->adap.retries = 1;
+
+	i2c->clk_main = devm_clk_get(&pdev->dev, "main");
+	if (IS_ERR(i2c->clk_main)) {
+		dev_err(&pdev->dev, "cannot get main clock\n");
+		return PTR_ERR(i2c->clk_main);
+	}
+
+	i2c->clk_dma = devm_clk_get(&pdev->dev, "dma");
+	if (IS_ERR(i2c->clk_dma)) {
+		dev_err(&pdev->dev, "cannot get dma clock\n");
+		return PTR_ERR(i2c->clk_dma);
+	}
+
+	if (i2c->have_pmic) {
+		i2c->clk_pmic = devm_clk_get(&pdev->dev, "pmic");
+		if (IS_ERR(i2c->clk_pmic)) {
+			dev_err(&pdev->dev, "cannot get pmic clock\n");
+			return PTR_ERR(i2c->clk_pmic);
+		}
+		clk_src_in_hz = clk_get_rate(i2c->clk_pmic) / clk_src_div;
+	} else {
+		clk_src_in_hz = clk_get_rate(i2c->clk_main) / clk_src_div;
+	}

This can be simplified, i.e. the common line can go after the if block
and then the else branch can be dropped.

+	dev_dbg(&pdev->dev, "clock source %p,clock src frequency %d\n",
+		i2c->clk_main, clk_src_in_hz);
+	strlcpy(i2c->adap.name, I2C_DRV_NAME, sizeof(i2c->adap.name));
+	init_waitqueue_head(&i2c->wait);
+
+	ret = i2c_set_speed(i2c, clk_src_in_hz);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to set the speed.\n");
+		return -EINVAL;
+	}
+
+	ret = mtk_i2c_clock_enable(i2c);
+	if (ret) {
+		dev_err(&pdev->dev, "clock enable failed!\n");
+		return ret;
+	}
+	mtk_i2c_init_hw(i2c);
+	mtk_i2c_clock_disable(i2c);
+
+	i2c->dma_buf.vaddr = dma_alloc_coherent(&pdev->dev,
+		PAGE_SIZE, &i2c->dma_buf.paddr, GFP_KERNEL);
+	if (i2c->dma_buf.vaddr == NULL) {
+		dev_err(&pdev->dev, "dma_alloc_coherent fail\n");
+		return -ENOMEM;
+	}
+
+	i2c_set_adapdata(&i2c->adap, i2c);
+	ret = i2c_add_adapter(&i2c->adap);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to add i2c bus to i2c core\n");
+		free_i2c_dma_bufs(i2c);
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, i2c);
+
+	return 0;
+}
+
+static int mtk_i2c_remove(struct platform_device *pdev)
+{
+	struct mtk_i2c *i2c = platform_get_drvdata(pdev);
+
+	i2c_del_adapter(&i2c->adap);
+	free_i2c_dma_bufs(i2c);
+	platform_set_drvdata(pdev, NULL);
+

Here you need to make sure that no irq is running when i2c_del_adapter
is called.

+	return 0;
+}
+
+static struct platform_driver mtk_i2c_driver = {
+	.probe = mtk_i2c_probe,
+	.remove = mtk_i2c_remove,
+	.driver = {
+		.name = I2C_DRV_NAME,
+		.owner = THIS_MODULE,

You don't need to set .owner any more. That's included in
module_platform_driver since some time.

+		.of_match_table = of_match_ptr(mtk_i2c_of_match),
+	},
+};
+
+module_platform_driver(mtk_i2c_driver);
+
+MODULE_LICENSE("GPL");

MODULE_LICENSE("GPL v2");

+MODULE_DESCRIPTION("MediaTek I2C Bus Driver");
+MODULE_AUTHOR("Xudong Chen [off-list ref]");

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |