Re: [PATCH v4 2/2] I2C: mediatek: Add driver for MediaTek I2C controller

[Uwe Kleine-König]

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 <xudong.chen@xxxxxxxxxxxx>");

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/