Hi,
thanks for your submission.
> +static void sprd_i2c_dump_reg(struct sprd_i2c *i2c_dev)
> +{
> + dev_err(&i2c_dev->adap.dev, ": ======dump i2c-%d reg=======\n",
> + i2c_dev->adap.nr);
> + dev_err(&i2c_dev->adap.dev, ": I2C_CTRL:0x%x\n",
> + readl(i2c_dev->base + I2C_CTL));
> + dev_err(&i2c_dev->adap.dev, ": I2C_ADDR_CFG:0x%x\n",
> + readl(i2c_dev->base + I2C_ADDR_CFG));
> + dev_err(&i2c_dev->adap.dev, ": I2C_COUNT:0x%x\n",
> + readl(i2c_dev->base + I2C_COUNT));
> + dev_err(&i2c_dev->adap.dev, ": I2C_RX:0x%x\n",
> + readl(i2c_dev->base + I2C_RX));
> + dev_err(&i2c_dev->adap.dev, ": I2C_STATUS:0x%x\n",
> + readl(i2c_dev->base + I2C_STATUS));
> + dev_err(&i2c_dev->adap.dev, ": ADDR_DVD0:0x%x\n",
> + readl(i2c_dev->base + ADDR_DVD0));
> + dev_err(&i2c_dev->adap.dev, ": ADDR_DVD1:0x%x\n",
> + readl(i2c_dev->base + ADDR_DVD1));
> + dev_err(&i2c_dev->adap.dev, ": ADDR_STA0_DVD:0x%x\n",
> + readl(i2c_dev->base + ADDR_STA0_DVD));
> + dev_err(&i2c_dev->adap.dev, ": ADDR_RST:0x%x\n",
> + readl(i2c_dev->base + ADDR_RST));
I really thing register dumps should be dev_dbg().
> +}
> +
> +static void sprd_i2c_set_count(struct sprd_i2c *i2c_dev, u32 count)
> +{
> + writel(count, i2c_dev->base + I2C_COUNT);
> +}
> +
> +static void sprd_i2c_send_stop(struct sprd_i2c *i2c_dev, int stop)
> +{
> + unsigned int tmp = readl(i2c_dev->base + I2C_CTL);
u32? Here and in many other places?
...
> +static irqreturn_t sprd_i2c_isr_thread(int irq, void *dev_id)
> +{
> + struct sprd_i2c *i2c_dev = dev_id;
> + struct i2c_msg *msg = i2c_dev->msg;
> + int ack = readl(i2c_dev->base + I2C_STATUS) & I2C_RX_ACK;
> + u32 i2c_count = readl(i2c_dev->base + I2C_COUNT);
> + u32 i2c_tran;
> +
> + if (msg->flags & I2C_M_RD)
> + i2c_tran = i2c_dev->count >= I2C_FIFO_FULL_THLD;
> + else
> + i2c_tran = i2c_count;
> +
> + /*
> + * If we got one ACK from slave when writing data, and we did not
Here you say: "If we get ack..."
> + * finish this transmission (i2c_tran is not zero), then we should
> + * continue to write data.
> + *
> + * For reading data, ack is always 0, if i2c_tran is not 0 which
> + * means we still need to contine to read data from slave.
> + */
> + if (i2c_tran && !ack) {
... but the code gives the assumption you did NOT get an ack. So, either
rename the variable to 'ack_err' or keep it 'ack' and invert the logic
when initializing the variable.
> + sprd_i2c_data_transfer(i2c_dev);
> + return IRQ_HANDLED;
> + }
> +
> + i2c_dev->err = 0;
> +
> + /*
> + * If we did not get one ACK from slave when writing data, we should
> + * dump all registers to check I2C status.
Why? I would say no. NACK from a slave can always happen, e.g. when an
EEPROM is busy erasing a page.
> + */
> + if (ack) {
> + i2c_dev->err = -EIO;
> + sprd_i2c_dump_reg(i2c_dev);
> + } else if (msg->flags & I2C_M_RD && i2c_dev->count) {
> + sprd_i2c_read_bytes(i2c_dev, i2c_dev->buf, i2c_dev->count);
> + }
> +
> + /* Transmission is done and clear ack and start operation */
> + sprd_i2c_clear_ack(i2c_dev);
> + sprd_i2c_clear_start(i2c_dev);
> + complete(&i2c_dev->complete);
> +
> + return IRQ_HANDLED;
> +}
...
> +
> + pm_runtime_set_autosuspend_delay(i2c_dev->dev, SPRD_I2C_PM_TIMEOUT);
> + pm_runtime_use_autosuspend(i2c_dev->dev);
> + pm_runtime_set_active(i2c_dev->dev);
> + pm_runtime_enable(i2c_dev->dev);
> +
> + ret = pm_runtime_get_sync(i2c_dev->dev);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "i2c%d pm runtime resume failed!\n",
> + pdev->id);
Error message has wrong text.
> + goto err_rpm_put;
> + }
> +
> +static int sprd_i2c_init(void)
> +{
> + return platform_driver_register(&sprd_i2c_driver);
> +}
> +arch_initcall_sync(sprd_i2c_init);
arch_initcall? and no exit() function? Why is it that way and/or why
can't you use platform_module_driver()?
Rest looks good. I like the comments you added to the code.
Regards,
Wolfram
Attachment:
signature.asc
Description: PGP signature
