Hi Uwe,
On Sun, 2015-01-18 at 11:18 +0100, Uwe Kleine-König wrote:
> 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.
OK
>
> > +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
OK
>
> > + 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?
I think use mtk_i2c pointer is more flexible than maintain a pointer to
message.
>
> > + 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.
OK
>
> > +};
> > +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.
Sure, I will add mtk_ prefix.
>
> > + /* 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.
OK, add check transfer complete in tmo == 0 case.
> > + 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?
No. addr 0 should be reserved for special purpose. No client should use
addr 0. I think driver should not block transfer addr 0, I will remove
this.
>
> I'd remove the leading space in the message. (Applies also to other
> places.)
Of course
>
> > + 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)?
Yes, mean write then read. Indeed, add check is better.
If msg number is 1, means normal write or read, not "write then read".
> > + /*
> > + * always use DMA mode.
> Out of interest: Did you benchmark DMA vs manual mode for a typical
> (what ever that means) scenario?
I think performance don't go to far, but always use DMA make driver
simplified.
>
> > + * 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?
Yes. interrupt enable in mtk_i2c_do_transfer, disable in mtk_i2c_irq.
>
> > + i2c->irq_stat = i2c_readw(i2c, OFFSET_INTR_STAT);
> Maybe you need locking here to modify your driver data in the irq?
OK
>
> > + 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
Clear interrupt
>
> i2c_writew(i2c->irq_stat & (I2C_HS_NACKERR | I2C_ACKERR ...),
> i2c, OFFSET_INTR_STAT);
>
Yes. you are right. This can avoid unnecessary write 1 bit.
> 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?
Our hardware has this capability, but not implement in this driver.
Remove I2C_FUNC_10BIT_ADDR in next round.
>
> > +}
> > +
> > +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.
OK
>
> > [...]
> > + 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.
OK
>
> > +
> > + 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.
OK
>
> > + 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.
One is clk_pmic, the other is clk_main, one way to reduce else branch is
use variable to store clk and set to i2c->clk_main by default. Then call
clk_get_rate at last. I don't know whether this is a better way.
>
> > + 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.
OK, add check here
>
> > + 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.
OK
>
> > + .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
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
