Hi Wolfram,

On Tue, Jul 31, 2018 at 10:09:32PM +0200, Wolfram Sang wrote:

Hi Manivannan,

On Thu, Jul 26, 2018 at 09:16:02PM +0530, Manivannan Sadhasivam wrote:

quoted

Add Actions Semiconductor Owl family S900 I2C driver.

Signed-off-by: Manivannan Sadhasivam <redacted>
Acked-by: Peter Rosin <redacted>

Looks mostly good already. Thanks Peter for the initial review!

Thanks to Andy also :)

quoted

+static irqreturn_t owl_i2c_interrupt(int irq, void *_dev)
+{
+	struct owl_i2c_dev *i2c_dev = _dev;
+	struct i2c_msg *msg = i2c_dev->msg;
+	unsigned long flags;
+	unsigned int stat, fifostat;
+
+	spin_lock_irqsave(&i2c_dev->lock, flags);
+
+	fifostat = readl(i2c_dev->base + OWL_I2C_REG_FIFOSTAT);
+	if (fifostat & OWL_I2C_FIFOSTAT_RNB) {
+		dev_dbg(&i2c_dev->adap.dev, "received NACK from device\n");
+		goto stop;
+	}
+
+	stat = readl(i2c_dev->base + OWL_I2C_REG_STAT);
+	if (stat & OWL_I2C_STAT_BEB) {
+		dev_dbg(&i2c_dev->adap.dev, "bus error\n");
+		goto stop;
+	}

I wonder if you can't pass back the different errors to the upper
layers? Like -ENXIO for NACK and -EIO for bus error? We have a
convention for that and it seems your HW can support it. The different
error codes would then maybe also make the debug outputs obsolete.

Sure, will catch the errors using an i2c_dev member and pass it to upper
layers in owl_i2c_master_xfer.

quoted

+	/*
+	 * Here, -ENXIO will be returned if interrupt occurred but no
+	 * read or write happened. Else if msg_ptr equals to message length,
+	 * message count will be returned.
+	 */
+	ret = i2c_dev->msg_ptr == msg->len ? num : -ENXIO;

I'd think this kinda unusual construct could go then as well by just
returning the error code derived from the interrupt handler above.

Makes sense! It will become:

ret = i2c_dev->err < 0 ? i2c_dev->err : num;

Thanks,
Mani

quoted

+static const struct i2c_adapter_quirks owl_i2c_quirks = {
+	.flags		= I2C_AQ_COMB | I2C_AQ_COMB_WRITE_FIRST,
+	.max_read_len   = 240,
+	.max_write_len  = 240,
+	.max_comb_1st_msg_len = 6,
+	.max_comb_2nd_msg_len = 240,
+};

Yay! Good use of the i2c_adapter_quirks struct :)

Regards,

   Wolfram