Re: [RESEND PATCH v4 2/2] i2c: Add Spreadtrum I2C controller driver

[Wolfram Sang]

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