[PATCH v4 0/5] i2c:ocores: improvements

[PATCH v4 0/5] i2c:ocores: improvements

[Federico Vaga]

This patch set provides improvements to the i2c-ocore driver.

[V3 -> V4]
- add reviews-by/tested-by
- add comment to justify the formula in
    udelay((8 * 1000) / i2c->bus_clock_khz);

[V2 -> V3]
- fix particular error condition on platform_get_irq(). Copied from
  https://patchwork.ozlabs.org/patch/1038409/

[V1 -> V2]
- replaced usleep_range() with udelay() so that the polling version can be
  used in atomic context.
- added dedicated patch for minor style issues
- fixed delay computation
- use spin_lock_irqsave(), instead of spin_trylock_irqsave(). IACK is always
  necessary and a trylock would generate an extra interrupt for nothing
- make the driver ready for an eventual master_xfer_irqless()

[PATCH v4 1/5] i2c:ocores: stop transfer on timeout

[Federico Vaga]

Detecting a timeout is ok, but we also need to assert a STOP command on
the bus in order to prevent it from generating interrupts when there are
no on going transfers.

Example: very long transmission.

1. ocores_xfer: START a transfer
2. ocores_isr : handle byte by byte the transfer
3. ocores_xfer: goes in timeout [[bugfix here]]
4. ocores_xfer: return to I2C subsystem and to the I2C driver
5. I2C driver : it may clean up the i2c_msg memory
6. ocores_isr : receives another interrupt (pending bytes to be
                transferred) but the i2c_msg memory is invalid now

So, since the transfer was too long, we have to detect the timeout and
STOP the transfer.

Another point is that we have a critical region here. When handling the
timeout condition we may have a running IRQ handler. For this reason I
introduce a spinlock.

In order to make easier to understan locking I have:
- added a new function to handle timeout
- modified the current ocores_process() function in order to be protected
  by the new spinlock
Like this it is obvious at first sight that this locking serializes
the execution of ocores_process() and ocores_process_timeout()

Signed-off-by: Federico Vaga <federico.vaga@cern.ch>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>

---
 drivers/i2c/busses/i2c-ocores.c | 54 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 45 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
index 87f9caa..aa85202 100644
--- a/drivers/i2c/busses/i2c-ocores.c
+++ b/drivers/i2c/busses/i2c-ocores.c
@@ -25,7 +25,12 @@
 #include <linux/slab.h>
 #include <linux/io.h>
 #include <linux/log2.h>
+#include <linux/spinlock.h>
 
+/**
+ * @process_lock: protect I2C transfer process.
+ *     ocores_process() and ocores_process_timeout() can't run in parallel.
+ */
 struct ocores_i2c {
 	void __iomem *base;
 	u32 reg_shift;
@@ -36,6 +41,7 @@ struct ocores_i2c {
 	int pos;
 	int nmsgs;
 	int state; /* see STATE_ */
+	spinlock_t process_lock;
 	struct clk *clk;
 	int ip_clock_khz;
 	int bus_clock_khz;
@@ -141,19 +147,26 @@ static void ocores_process(struct ocores_i2c *i2c)
 {
 	struct i2c_msg *msg = i2c->msg;
 	u8 stat = oc_getreg(i2c, OCI2C_STATUS);
+	unsigned long flags;
+
+	/*
+	 * If we spin here is because we are in timeout, so we are going
+	 * to be in STATE_ERROR. See ocores_process_timeout()
+	 */
+	spin_lock_irqsave(&i2c->process_lock, flags);
 
 	if ((i2c->state == STATE_DONE) || (i2c->state == STATE_ERROR)) {
 		/* stop has been sent */
 		oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_IACK);
 		wake_up(&i2c->wait);
-		return;
+		goto out;
 	}
 
 	/* error? */
 	if (stat & OCI2C_STAT_ARBLOST) {
 		i2c->state = STATE_ERROR;
 		oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_STOP);
-		return;
+		goto out;
 	}
 
 	if ((i2c->state == STATE_START) || (i2c->state == STATE_WRITE)) {
@@ -163,7 +176,7 @@ static void ocores_process(struct ocores_i2c *i2c)
 		if (stat & OCI2C_STAT_NACK) {
 			i2c->state = STATE_ERROR;
 			oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_STOP);
-			return;
+			goto out;
 		}
 	} else
 		msg->buf[i2c->pos++] = oc_getreg(i2c, OCI2C_DATA);
@@ -184,14 +197,14 @@ static void ocores_process(struct ocores_i2c *i2c)
 
 				oc_setreg(i2c, OCI2C_DATA, addr);
 				oc_setreg(i2c, OCI2C_CMD,  OCI2C_CMD_START);
-				return;
+				goto out;
 			} else
 				i2c->state = (msg->flags & I2C_M_RD)
 					? STATE_READ : STATE_WRITE;
 		} else {
 			i2c->state = STATE_DONE;
 			oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_STOP);
-			return;
+			goto out;
 		}
 	}
 
@@ -202,6 +215,9 @@ static void ocores_process(struct ocores_i2c *i2c)
 		oc_setreg(i2c, OCI2C_DATA, msg->buf[i2c->pos++]);
 		oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_WRITE);
 	}
+
+out:
+	spin_unlock_irqrestore(&i2c->process_lock, flags);
 }
 
 static irqreturn_t ocores_isr(int irq, void *dev_id)
@@ -213,9 +229,24 @@ static irqreturn_t ocores_isr(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+/**
+ * Process timeout event
+ * @i2c: ocores I2C device instance
+ */
+static void ocores_process_timeout(struct ocores_i2c *i2c)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&i2c->process_lock, flags);
+	i2c->state = STATE_ERROR;
+	oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_STOP);
+	spin_unlock_irqrestore(&i2c->process_lock, flags);
+}
+
 static int ocores_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 {
 	struct ocores_i2c *i2c = i2c_get_adapdata(adap);
+	int ret;
 
 	i2c->msg = msgs;
 	i2c->pos = 0;
@@ -225,11 +256,14 @@ static int ocores_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 	oc_setreg(i2c, OCI2C_DATA, i2c_8bit_addr_from_msg(i2c->msg));
 	oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_START);
 
-	if (wait_event_timeout(i2c->wait, (i2c->state == STATE_ERROR) ||
-			       (i2c->state == STATE_DONE), HZ))
-		return (i2c->state == STATE_DONE) ? num : -EIO;
-	else
+	ret = wait_event_timeout(i2c->wait, (i2c->state == STATE_ERROR) ||
+				 (i2c->state == STATE_DONE), HZ);
+	if (ret == 0) {
+		ocores_process_timeout(i2c);
 		return -ETIMEDOUT;
+	}
+
+	return (i2c->state == STATE_DONE) ? num : -EIO;
 }
 
 static int ocores_init(struct device *dev, struct ocores_i2c *i2c)
@@ -422,6 +456,8 @@ static int ocores_i2c_probe(struct platform_device *pdev)
 	if (!i2c)
 		return -ENOMEM;
 
+	spin_lock_init(&i2c->process_lock);
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	i2c->base = devm_ioremap_resource(&pdev->dev, res);
 	if (IS_ERR(i2c->base))
-- 
2.15.0

[PATCH v4 2/5] i2c:ocores: do not handle IRQ if IF is not set

[Federico Vaga]

If the Interrupt Flag (IF) is not set, we should not handle the IRQ:
- the line can be shared with other devices
- it can be a spurious interrupt

To avoid reading twice the status register, the ocores_process() function
expects it to be read by the caller.

Signed-off-by: Federico Vaga <federico.vaga@cern.ch>
Acked-by: Peter Korsgaard <peter@korsgaard.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>

---
 drivers/i2c/busses/i2c-ocores.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
index aa85202..fcc2558 100644
--- a/drivers/i2c/busses/i2c-ocores.c
+++ b/drivers/i2c/busses/i2c-ocores.c
@@ -143,10 +143,9 @@ static inline u8 oc_getreg(struct ocores_i2c *i2c, int reg)
 	return i2c->getreg(i2c, reg);
 }
 
-static void ocores_process(struct ocores_i2c *i2c)
+static void ocores_process(struct ocores_i2c *i2c, u8 stat)
 {
 	struct i2c_msg *msg = i2c->msg;
-	u8 stat = oc_getreg(i2c, OCI2C_STATUS);
 	unsigned long flags;
 
 	/*
@@ -223,8 +222,12 @@ out:
 static irqreturn_t ocores_isr(int irq, void *dev_id)
 {
 	struct ocores_i2c *i2c = dev_id;
+	u8 stat = oc_getreg(i2c, OCI2C_STATUS);
+
+	if (!(stat & OCI2C_STAT_IF))
+		return IRQ_NONE;
 
-	ocores_process(i2c);
+	ocores_process(i2c, stat);
 
 	return IRQ_HANDLED;
 }
-- 
2.15.0

[PATCH v4 3/5] i2c:ocores: add polling interface

[Federico Vaga]

This driver assumes that an interrupt line is always available for
the I2C master. This is not always the case and this patch adds support
for a polling version.

Report from Andrew Lunn:

  I did some timing tests for this. On my box, we request a udelay of
  80uS. The kernel actually delays for about 79uS. We then spin in
  ocores_wait() for an additional 10-11uS, which is 3 to 4 iterations.

  There are actually 9 bits on the wire, not 8, since there is an
  ACK/NACK bit after the actual data transfer. So i changed the delay to
  (9 * 1000) / i2c->bus_clock_khz. That resulted in ocores_wait() mostly
  not looping at all. But for reading an 4K AT24 EEPROM, it increased
  the read time by 10ms, from 424ms to 434ms. So we should probably keep
  with 8.

Signed-off-by: Federico Vaga <federico.vaga@cern.ch>
Tested-by: Andrew Lunn <andrew@lunn.ch>

---
 drivers/i2c/busses/i2c-ocores.c | 180 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 160 insertions(+), 20 deletions(-)

diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
index fcc2558..d42a324 100644
--- a/drivers/i2c/busses/i2c-ocores.c
+++ b/drivers/i2c/busses/i2c-ocores.c
@@ -13,6 +13,7 @@
  */
 
 #include <linux/clk.h>
+#include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
@@ -26,6 +27,9 @@
 #include <linux/io.h>
 #include <linux/log2.h>
 #include <linux/spinlock.h>
+#include <linux/jiffies.h>
+
+#define OCORES_FLAG_POLL BIT(0)
 
 /**
  * @process_lock: protect I2C transfer process.
@@ -35,6 +39,7 @@ struct ocores_i2c {
 	void __iomem *base;
 	u32 reg_shift;
 	u32 reg_io_width;
+	unsigned long flags;
 	wait_queue_head_t wait;
 	struct i2c_adapter adap;
 	struct i2c_msg *msg;
@@ -246,10 +251,117 @@ static void ocores_process_timeout(struct ocores_i2c *i2c)
 	spin_unlock_irqrestore(&i2c->process_lock, flags);
 }
 
-static int ocores_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
+/**
+ * Wait until something change in a given register
+ * @i2c: ocores I2C device instance
+ * @reg: register to query
+ * @mask: bitmask to apply on register value
+ * @val: expected result
+ * @timeout: timeout in jiffies
+ *
+ * Timeout is necessary to avoid to stay here forever when the chip
+ * does not answer correctly.
+ *
+ * Return: 0 on success, -ETIMEDOUT on timeout
+ */
+static int ocores_wait(struct ocores_i2c *i2c,
+		       int reg, u8 mask, u8 val,
+		       const unsigned long timeout)
+{
+	unsigned long j;
+
+	j = jiffies + timeout;
+	while (1) {
+		u8 status = oc_getreg(i2c, reg);
+
+		if ((status & mask) == val)
+			break;
+
+		if (time_after(jiffies, j))
+			return -ETIMEDOUT;
+	}
+	return 0;
+}
+
+/**
+ * Wait until is possible to process some data
+ * @i2c: ocores I2C device instance
+ *
+ * Used when the device is in polling mode (interrupts disabled).
+ *
+ * Return: 0 on success, -ETIMEDOUT on timeout
+ */
+static int ocores_poll_wait(struct ocores_i2c *i2c)
+{
+	u8 mask;
+	int err;
+
+	if (i2c->state == STATE_DONE || i2c->state == STATE_ERROR) {
+		/* transfer is over */
+		mask = OCI2C_STAT_BUSY;
+	} else {
+		/* on going transfer */
+		mask = OCI2C_STAT_TIP;
+		/*
+		 * We wait for the data to be transfered (8bit),
+		 * then we start polling on the ACK/NACK bit
+		 */
+		udelay((8 * 1000) / i2c->bus_clock_khz);
+	}
+
+	/*
+	 * once we are here we expect to get the expected result immediately
+	 * so if after 1ms we timeout then something is broken.
+	 */
+	err = ocores_wait(i2c, OCI2C_STATUS, mask, 0, msecs_to_jiffies(1));
+	if (err)
+		dev_warn(i2c->adap.dev.parent,
+			 "%s: STATUS timeout, bit 0x%x did not clear in 1ms\n",
+			 __func__, mask);
+	return err;
+}
+
+
+/**
+ * It handles an IRQ-less transfer
+ * @i2c: ocores I2C device instance
+ *
+ * Even if IRQ are disabled, the I2C OpenCore IP behavior is exactly the same
+ * (only that IRQ are not produced). This means that we can re-use entirely
+ * ocores_isr(), we just add our polling code around it.
+ *
+ * It can run in atomic context
+ */
+static void ocores_process_polling(struct ocores_i2c *i2c)
+{
+	while (1) {
+		irqreturn_t ret;
+		int err;
+
+		err = ocores_poll_wait(i2c);
+		if (err) {
+			i2c->state = STATE_ERROR;
+			break; /* timeout */
+		}
+
+		ret = ocores_isr(-1, i2c);
+		if (ret == IRQ_NONE)
+			break; /* all messages have been transfered */
+	}
+}
+
+static int ocores_xfer_core(struct ocores_i2c *i2c,
+			    struct i2c_msg *msgs, int num,
+			    bool polling)
 {
-	struct ocores_i2c *i2c = i2c_get_adapdata(adap);
 	int ret;
+	u8 ctrl;
+
+	ctrl = oc_getreg(i2c, OCI2C_CONTROL);
+	if (polling)
+		oc_setreg(i2c, OCI2C_CONTROL, ctrl & ~OCI2C_CTRL_IEN);
+	else
+		oc_setreg(i2c, OCI2C_CONTROL, ctrl | OCI2C_CTRL_IEN);
 
 	i2c->msg = msgs;
 	i2c->pos = 0;
@@ -259,16 +371,37 @@ static int ocores_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 	oc_setreg(i2c, OCI2C_DATA, i2c_8bit_addr_from_msg(i2c->msg));
 	oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_START);
 
-	ret = wait_event_timeout(i2c->wait, (i2c->state == STATE_ERROR) ||
-				 (i2c->state == STATE_DONE), HZ);
-	if (ret == 0) {
-		ocores_process_timeout(i2c);
-		return -ETIMEDOUT;
+	if (polling) {
+		ocores_process_polling(i2c);
+	} else {
+		ret = wait_event_timeout(i2c->wait,
+					 (i2c->state == STATE_ERROR) ||
+					 (i2c->state == STATE_DONE), HZ);
+		if (ret == 0) {
+			ocores_process_timeout(i2c);
+			return -ETIMEDOUT;
+		}
 	}
 
 	return (i2c->state == STATE_DONE) ? num : -EIO;
 }
 
+static int ocores_xfer_polling(struct i2c_adapter *adap,
+			       struct i2c_msg *msgs, int num)
+{
+	return ocores_xfer_core(i2c_get_adapdata(adap), msgs, num, true);
+}
+
+static int ocores_xfer(struct i2c_adapter *adap,
+		       struct i2c_msg *msgs, int num)
+{
+	struct ocores_i2c *i2c = i2c_get_adapdata(adap);
+
+	if (i2c->flags & OCORES_FLAG_POLL)
+		return ocores_xfer_polling(adap, msgs, num);
+	return ocores_xfer_core(i2c, msgs, num, false);
+}
+
 static int ocores_init(struct device *dev, struct ocores_i2c *i2c)
 {
 	int prescale;
@@ -294,7 +427,7 @@ static int ocores_init(struct device *dev, struct ocores_i2c *i2c)
 
 	/* Init the device */
 	oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_IACK);
-	oc_setreg(i2c, OCI2C_CONTROL, ctrl | OCI2C_CTRL_IEN | OCI2C_CTRL_EN);
+	oc_setreg(i2c, OCI2C_CONTROL, ctrl | OCI2C_CTRL_EN);
 
 	return 0;
 }
@@ -451,10 +584,6 @@ static int ocores_i2c_probe(struct platform_device *pdev)
 	int ret;
 	int i;
 
-	irq = platform_get_irq(pdev, 0);
-	if (irq < 0)
-		return irq;
-
 	i2c = devm_kzalloc(&pdev->dev, sizeof(*i2c), GFP_KERNEL);
 	if (!i2c)
 		return -ENOMEM;
@@ -509,18 +638,29 @@ static int ocores_i2c_probe(struct platform_device *pdev)
 		}
 	}
 
+	init_waitqueue_head(&i2c->wait);
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq == -ENXIO) {
+		i2c->flags |= OCORES_FLAG_POLL;
+	} else {
+		if (irq < 0)
+			return irq;
+	}
+
+	if (!(i2c->flags & OCORES_FLAG_POLL)) {
+		ret = devm_request_irq(&pdev->dev, irq, ocores_isr, 0,
+				       pdev->name, i2c);
+		if (ret) {
+			dev_err(&pdev->dev, "Cannot claim IRQ\n");
+			goto err_clk;
+		}
+	}
+
 	ret = ocores_init(&pdev->dev, i2c);
 	if (ret)
 		goto err_clk;
 
-	init_waitqueue_head(&i2c->wait);
-	ret = devm_request_irq(&pdev->dev, irq, ocores_isr, 0,
-			       pdev->name, i2c);
-	if (ret) {
-		dev_err(&pdev->dev, "Cannot claim IRQ\n");
-		goto err_clk;
-	}
-
 	/* hook up driver to tree */
 	platform_set_drvdata(pdev, i2c);
 	i2c->adap = ocores_adapter;
-- 
2.15.0

[PATCH v4 4/5] i2c:ocores: add SPDX tag

[Federico Vaga]

It adds the SPDX tag and it removes the old text about the GPLv2.

Signed-off-by: Federico Vaga <federico.vaga@cern.ch>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>

---
 drivers/i2c/busses/i2c-ocores.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
index bbe3e96..e87fce0 100644
--- a/drivers/i2c/busses/i2c-ocores.c
+++ b/drivers/i2c/busses/i2c-ocores.c
@@ -1,3 +1,4 @@
+// SPDX-License-Identifier: GPL-2.0
 /*
  * i2c-ocores.c: I2C bus driver for OpenCores I2C controller
  * (https://opencores.org/project/i2c/overview)
@@ -6,10 +7,6 @@
  *
  * Support for the GRLIB port of the controller by
  * Andreas Larsson <andreas@gaisler.com>
- *
- * This file is licensed under the terms of the GNU General Public License
- * version 2.  This program is licensed "as is" without any warranty of any
- * kind, whether express or implied.
  */
 
 #include <linux/clk.h>
-- 
2.15.0

[PATCH v4 5/5] i2c:ocores: checkpatch fixes

[Federico Vaga]

Miscellaneous style fixes from checkpatch

Signed-off-by: Federico Vaga <federico.vaga@cern.ch>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>

---
 drivers/i2c/busses/i2c-ocores.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
index 5b80190..ba35d2a 100644
--- a/drivers/i2c/busses/i2c-ocores.c
+++ b/drivers/i2c/busses/i2c-ocores.c
@@ -182,8 +182,9 @@ static void ocores_process(struct ocores_i2c *i2c, u8 stat)
 			oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_STOP);
 			goto out;
 		}
-	} else
+	} else {
 		msg->buf[i2c->pos++] = oc_getreg(i2c, OCI2C_DATA);
+	}
 
 	/* end of msg? */
 	if (i2c->pos == msg->len) {
@@ -202,9 +203,9 @@ static void ocores_process(struct ocores_i2c *i2c, u8 stat)
 				oc_setreg(i2c, OCI2C_DATA, addr);
 				oc_setreg(i2c, OCI2C_CMD,  OCI2C_CMD_START);
 				goto out;
-			} else
-				i2c->state = (msg->flags & I2C_M_RD)
-					? STATE_READ : STATE_WRITE;
+			}
+			i2c->state = (msg->flags & I2C_M_RD)
+				? STATE_READ : STATE_WRITE;
 		} else {
 			i2c->state = STATE_DONE;
 			oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_STOP);
@@ -405,7 +406,8 @@ static int ocores_init(struct device *dev, struct ocores_i2c *i2c)
 	u8 ctrl = oc_getreg(i2c, OCI2C_CONTROL);
 
 	/* make sure the device is disabled */
-	oc_setreg(i2c, OCI2C_CONTROL, ctrl & ~(OCI2C_CTRL_EN|OCI2C_CTRL_IEN));
+	ctrl &= ~(OCI2C_CTRL_EN | OCI2C_CTRL_IEN);
+	oc_setreg(i2c, OCI2C_CONTROL, ctrl);
 
 	prescale = (i2c->ip_clock_khz / (5 * i2c->bus_clock_khz)) - 1;
 	prescale = clamp(prescale, 0, 0xffff);
@@ -462,11 +464,13 @@ MODULE_DEVICE_TABLE(of, ocores_i2c_match);
 #ifdef CONFIG_OF
 /* Read and write functions for the GRLIB port of the controller. Registers are
  * 32-bit big endian and the PRELOW and PREHIGH registers are merged into one
- * register. The subsequent registers has their offset decreased accordingly. */
+ * register. The subsequent registers has their offset decreased accordingly.
+ */
 static u8 oc_getreg_grlib(struct ocores_i2c *i2c, int reg)
 {
 	u32 rd;
 	int rreg = reg;
+
 	if (reg != OCI2C_PRELOW)
 		rreg--;
 	rd = ioread32be(i2c->base + (rreg << i2c->reg_shift));
@@ -480,6 +484,7 @@ static void oc_setreg_grlib(struct ocores_i2c *i2c, int reg, u8 value)
 {
 	u32 curr, wr;
 	int rreg = reg;
+
 	if (reg != OCI2C_PRELOW)
 		rreg--;
 	if (reg == OCI2C_PRELOW || reg == OCI2C_PREHIGH) {
@@ -568,7 +573,7 @@ static int ocores_i2c_of_probe(struct platform_device *pdev,
 	return 0;
 }
 #else
-#define ocores_i2c_of_probe(pdev,i2c) -ENODEV
+#define ocores_i2c_of_probe(pdev, i2c) -ENODEV
 #endif
 
 static int ocores_i2c_probe(struct platform_device *pdev)
-- 
2.15.0

Re: [PATCH v4 5/5] i2c:ocores: checkpatch fixes

[Peter Rosin]

On 2019-02-11 09:31, Federico Vaga wrote:
> Miscellaneous style fixes from checkpatch
> 
> Signed-off-by: Federico Vaga <federico.vaga@cern.ch>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> 
> ---
>  drivers/i2c/busses/i2c-ocores.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
> index 5b80190..ba35d2a 100644
> --- a/drivers/i2c/busses/i2c-ocores.c
> +++ b/drivers/i2c/busses/i2c-ocores.c
> @@ -182,8 +182,9 @@ static void ocores_process(struct ocores_i2c *i2c, u8 stat)
>  			oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_STOP);
>  			goto out;
>  		}
> -	} else
> +	} else {
>  		msg->buf[i2c->pos++] = oc_getreg(i2c, OCI2C_DATA);
> +	}
>  
>  	/* end of msg? */
>  	if (i2c->pos == msg->len) {
> @@ -202,9 +203,9 @@ static void ocores_process(struct ocores_i2c *i2c, u8 stat)
>  				oc_setreg(i2c, OCI2C_DATA, addr);
>  				oc_setreg(i2c, OCI2C_CMD,  OCI2C_CMD_START);
>  				goto out;
> -			} else
> -				i2c->state = (msg->flags & I2C_M_RD)
> -					? STATE_READ : STATE_WRITE;
> +			}
> +			i2c->state = (msg->flags & I2C_M_RD)
> +				? STATE_READ : STATE_WRITE;
>  		} else {
>  			i2c->state = STATE_DONE;
>  			oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_STOP);
> @@ -405,7 +406,8 @@ static int ocores_init(struct device *dev, struct ocores_i2c *i2c)
>  	u8 ctrl = oc_getreg(i2c, OCI2C_CONTROL);
>  
>  	/* make sure the device is disabled */
> -	oc_setreg(i2c, OCI2C_CONTROL, ctrl & ~(OCI2C_CTRL_EN|OCI2C_CTRL_IEN));
> +	ctrl &= ~(OCI2C_CTRL_EN | OCI2C_CTRL_IEN);
> +	oc_setreg(i2c, OCI2C_CONTROL, ctrl);
>  
>  	prescale = (i2c->ip_clock_khz / (5 * i2c->bus_clock_khz)) - 1;
>  	prescale = clamp(prescale, 0, 0xffff);
> @@ -462,11 +464,13 @@ MODULE_DEVICE_TABLE(of, ocores_i2c_match);
>  #ifdef CONFIG_OF
>  /* Read and write functions for the GRLIB port of the controller. Registers are
>   * 32-bit big endian and the PRELOW and PREHIGH registers are merged into one
> - * register. The subsequent registers has their offset decreased accordingly. */
> + * register. The subsequent registers has their offset decreased accordingly.
> + */

The preferred multi-line comment styles is:

/*
 * Read...
 * ...accordingly.
 */

So, please fix all issues when you are changing it to conform.

Cheers,
Peter

>  static u8 oc_getreg_grlib(struct ocores_i2c *i2c, int reg)
>  {
>  	u32 rd;
>  	int rreg = reg;
> +
>  	if (reg != OCI2C_PRELOW)
>  		rreg--;
>  	rd = ioread32be(i2c->base + (rreg << i2c->reg_shift));
> @@ -480,6 +484,7 @@ static void oc_setreg_grlib(struct ocores_i2c *i2c, int reg, u8 value)
>  {
>  	u32 curr, wr;
>  	int rreg = reg;
> +
>  	if (reg != OCI2C_PRELOW)
>  		rreg--;
>  	if (reg == OCI2C_PRELOW || reg == OCI2C_PREHIGH) {
> @@ -568,7 +573,7 @@ static int ocores_i2c_of_probe(struct platform_device *pdev,
>  	return 0;
>  }
>  #else
> -#define ocores_i2c_of_probe(pdev,i2c) -ENODEV
> +#define ocores_i2c_of_probe(pdev, i2c) -ENODEV
>  #endif
>  
>  static int ocores_i2c_probe(struct platform_device *pdev)
> 

Re: [PATCH v4 1/5] i2c:ocores: stop transfer on timeout

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1444 bytes --]

On Mon, Feb 11, 2019 at 09:31:18AM +0100, Federico Vaga wrote:
> Detecting a timeout is ok, but we also need to assert a STOP command on
> the bus in order to prevent it from generating interrupts when there are
> no on going transfers.
> 
> Example: very long transmission.
> 
> 1. ocores_xfer: START a transfer
> 2. ocores_isr : handle byte by byte the transfer
> 3. ocores_xfer: goes in timeout [[bugfix here]]
> 4. ocores_xfer: return to I2C subsystem and to the I2C driver
> 5. I2C driver : it may clean up the i2c_msg memory
> 6. ocores_isr : receives another interrupt (pending bytes to be
>                 transferred) but the i2c_msg memory is invalid now
> 
> So, since the transfer was too long, we have to detect the timeout and
> STOP the transfer.
> 
> Another point is that we have a critical region here. When handling the
> timeout condition we may have a running IRQ handler. For this reason I
> introduce a spinlock.
> 
> In order to make easier to understan locking I have:
> - added a new function to handle timeout
> - modified the current ocores_process() function in order to be protected
>   by the new spinlock
> Like this it is obvious at first sight that this locking serializes
> the execution of ocores_process() and ocores_process_timeout()
> 
> Signed-off-by: Federico Vaga <federico.vaga@cern.ch>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> 

Applied to for-next, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 2/5] i2c:ocores: do not handle IRQ if IF is not set

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 534 bytes --]

On Mon, Feb 11, 2019 at 09:31:19AM +0100, Federico Vaga wrote:
> If the Interrupt Flag (IF) is not set, we should not handle the IRQ:
> - the line can be shared with other devices
> - it can be a spurious interrupt
> 
> To avoid reading twice the status register, the ocores_process() function
> expects it to be read by the caller.
> 
> Signed-off-by: Federico Vaga <federico.vaga@cern.ch>
> Acked-by: Peter Korsgaard <peter@korsgaard.com>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> 

Applied to for-next, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 3/5] i2c:ocores: add polling interface

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1501 bytes --]

On Mon, Feb 11, 2019 at 09:31:20AM +0100, Federico Vaga wrote:
> This driver assumes that an interrupt line is always available for
> the I2C master. This is not always the case and this patch adds support
> for a polling version.
> 
> Report from Andrew Lunn:
> 
>   I did some timing tests for this. On my box, we request a udelay of
>   80uS. The kernel actually delays for about 79uS. We then spin in
>   ocores_wait() for an additional 10-11uS, which is 3 to 4 iterations.
> 
>   There are actually 9 bits on the wire, not 8, since there is an
>   ACK/NACK bit after the actual data transfer. So i changed the delay to
>   (9 * 1000) / i2c->bus_clock_khz. That resulted in ocores_wait() mostly
>   not looping at all. But for reading an 4K AT24 EEPROM, it increased
>   the read time by 10ms, from 424ms to 434ms. So we should probably keep
>   with 8.
> 
> Signed-off-by: Federico Vaga <federico.vaga@cern.ch>
> Tested-by: Andrew Lunn <andrew@lunn.ch>
> 

Fixed these checkpatch warnings:

WARNING: 'transfered' may be misspelled - perhaps 'transferred'?
#111: FILE: drivers/i2c/busses/i2c-ocores.c:306:
+		 * We wait for the data to be transfered (8bit),

CHECK: Please don't use multiple blank lines
#129: FILE: drivers/i2c/busses/i2c-ocores.c:324:
+
+

WARNING: 'transfered' may be misspelled - perhaps 'transferred'?
#154: FILE: drivers/i2c/busses/i2c-ocores.c:349:
+			break; /* all messages have been transfered */

and applied to for-next, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 4/5] i2c:ocores: add SPDX tag

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 332 bytes --]

On Mon, Feb 11, 2019 at 09:31:21AM +0100, Federico Vaga wrote:
> It adds the SPDX tag and it removes the old text about the GPLv2.
> 
> Signed-off-by: Federico Vaga <federico.vaga@cern.ch>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>

Applied SPDX to the platform_data include as well and...

Applied to for-next, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 5/5] i2c:ocores: checkpatch fixes

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 613 bytes --]


> >  /* Read and write functions for the GRLIB port of the controller. Registers are
> >   * 32-bit big endian and the PRELOW and PREHIGH registers are merged into one
> > - * register. The subsequent registers has their offset decreased accordingly. */
> > + * register. The subsequent registers has their offset decreased accordingly.
> > + */
> 
> The preferred multi-line comment styles is:
> 
> /*
>  * Read...
>  * ...accordingly.
>  */
> 
> So, please fix all issues when you are changing it to conform.

Yes. It is enough to send this updated patch alone; I applied the rest
already.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 5/5] i2c:ocores: checkpatch fixes

[Peter Rosin]

On 2019-02-11 11:16, Peter Rosin wrote:
> On 2019-02-11 09:31, Federico Vaga wrote:
>> Miscellaneous style fixes from checkpatch
>>
>> Signed-off-by: Federico Vaga <federico.vaga@cern.ch>
>> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
>>
>> ---
>>  drivers/i2c/busses/i2c-ocores.c | 19 ++++++++++++-------
>>  1 file changed, 12 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
>> index 5b80190..ba35d2a 100644
>> --- a/drivers/i2c/busses/i2c-ocores.c
>> +++ b/drivers/i2c/busses/i2c-ocores.c
>> @@ -182,8 +182,9 @@ static void ocores_process(struct ocores_i2c *i2c, u8 stat)
>>  			oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_STOP);
>>  			goto out;
>>  		}
>> -	} else
>> +	} else {
>>  		msg->buf[i2c->pos++] = oc_getreg(i2c, OCI2C_DATA);
>> +	}
>>  
>>  	/* end of msg? */
>>  	if (i2c->pos == msg->len) {
>> @@ -202,9 +203,9 @@ static void ocores_process(struct ocores_i2c *i2c, u8 stat)
>>  				oc_setreg(i2c, OCI2C_DATA, addr);
>>  				oc_setreg(i2c, OCI2C_CMD,  OCI2C_CMD_START);
>>  				goto out;
>> -			} else
>> -				i2c->state = (msg->flags & I2C_M_RD)
>> -					? STATE_READ : STATE_WRITE;
>> +			}
>> +			i2c->state = (msg->flags & I2C_M_RD)
>> +				? STATE_READ : STATE_WRITE;
>>  		} else {
>>  			i2c->state = STATE_DONE;
>>  			oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_STOP);
>> @@ -405,7 +406,8 @@ static int ocores_init(struct device *dev, struct ocores_i2c *i2c)
>>  	u8 ctrl = oc_getreg(i2c, OCI2C_CONTROL);
>>  
>>  	/* make sure the device is disabled */
>> -	oc_setreg(i2c, OCI2C_CONTROL, ctrl & ~(OCI2C_CTRL_EN|OCI2C_CTRL_IEN));
>> +	ctrl &= ~(OCI2C_CTRL_EN | OCI2C_CTRL_IEN);
>> +	oc_setreg(i2c, OCI2C_CONTROL, ctrl);
>>  
>>  	prescale = (i2c->ip_clock_khz / (5 * i2c->bus_clock_khz)) - 1;
>>  	prescale = clamp(prescale, 0, 0xffff);
>> @@ -462,11 +464,13 @@ MODULE_DEVICE_TABLE(of, ocores_i2c_match);
>>  #ifdef CONFIG_OF
>>  /* Read and write functions for the GRLIB port of the controller. Registers are
>>   * 32-bit big endian and the PRELOW and PREHIGH registers are merged into one
>> - * register. The subsequent registers has their offset decreased accordingly. */
>> + * register. The subsequent registers has their offset decreased accordingly.

has -> have

offset -> offsets  ??? I think?

>> + */
> 
> The preferred multi-line comment styles is:

And finally, this typo is my own...

styles -> style

Cheers and sorry for replying to self,
Peter

> 
> /*
>  * Read...
>  * ...accordingly.
>  */
> 
> So, please fix all issues when you are changing it to conform.
> 
> Cheers,
> Peter
> 
>>  static u8 oc_getreg_grlib(struct ocores_i2c *i2c, int reg)
>>  {
>>  	u32 rd;
>>  	int rreg = reg;
>> +
>>  	if (reg != OCI2C_PRELOW)
>>  		rreg--;
>>  	rd = ioread32be(i2c->base + (rreg << i2c->reg_shift));
>> @@ -480,6 +484,7 @@ static void oc_setreg_grlib(struct ocores_i2c *i2c, int reg, u8 value)
>>  {
>>  	u32 curr, wr;
>>  	int rreg = reg;
>> +
>>  	if (reg != OCI2C_PRELOW)
>>  		rreg--;
>>  	if (reg == OCI2C_PRELOW || reg == OCI2C_PREHIGH) {
>> @@ -568,7 +573,7 @@ static int ocores_i2c_of_probe(struct platform_device *pdev,
>>  	return 0;
>>  }
>>  #else
>> -#define ocores_i2c_of_probe(pdev,i2c) -ENODEV
>> +#define ocores_i2c_of_probe(pdev, i2c) -ENODEV
>>  #endif
>>  
>>  static int ocores_i2c_probe(struct platform_device *pdev)
>>
> 

Re: [PATCH v4 3/5] i2c:ocores: add polling interface

[Peter Rosin]

On 2019-02-11 09:31, Federico Vaga wrote:
> This driver assumes that an interrupt line is always available for
> the I2C master. This is not always the case and this patch adds support
> for a polling version.
> 
> Report from Andrew Lunn:
> 
>   I did some timing tests for this. On my box, we request a udelay of
>   80uS. The kernel actually delays for about 79uS. We then spin in
>   ocores_wait() for an additional 10-11uS, which is 3 to 4 iterations.
> 
>   There are actually 9 bits on the wire, not 8, since there is an
>   ACK/NACK bit after the actual data transfer. So i changed the delay to
>   (9 * 1000) / i2c->bus_clock_khz. That resulted in ocores_wait() mostly
>   not looping at all. But for reading an 4K AT24 EEPROM, it increased
>   the read time by 10ms, from 424ms to 434ms. So we should probably keep
>   with 8.
> 
> Signed-off-by: Federico Vaga <federico.vaga@cern.ch>
> Tested-by: Andrew Lunn <andrew@lunn.ch>
> 
> ---
>  drivers/i2c/busses/i2c-ocores.c | 180 +++++++++++++++++++++++++++++++++++-----
>  1 file changed, 160 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
> index fcc2558..d42a324 100644
> --- a/drivers/i2c/busses/i2c-ocores.c
> +++ b/drivers/i2c/busses/i2c-ocores.c
> @@ -13,6 +13,7 @@
>   */
>  
>  #include <linux/clk.h>
> +#include <linux/delay.h>
>  #include <linux/err.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> @@ -26,6 +27,9 @@
>  #include <linux/io.h>
>  #include <linux/log2.h>
>  #include <linux/spinlock.h>
> +#include <linux/jiffies.h>
> +
> +#define OCORES_FLAG_POLL BIT(0)
>  
>  /**
>   * @process_lock: protect I2C transfer process.
> @@ -35,6 +39,7 @@ struct ocores_i2c {
>  	void __iomem *base;
>  	u32 reg_shift;
>  	u32 reg_io_width;
> +	unsigned long flags;
>  	wait_queue_head_t wait;
>  	struct i2c_adapter adap;
>  	struct i2c_msg *msg;
> @@ -246,10 +251,117 @@ static void ocores_process_timeout(struct ocores_i2c *i2c)
>  	spin_unlock_irqrestore(&i2c->process_lock, flags);
>  }
>  
> -static int ocores_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
> +/**
> + * Wait until something change in a given register
> + * @i2c: ocores I2C device instance
> + * @reg: register to query
> + * @mask: bitmask to apply on register value
> + * @val: expected result
> + * @timeout: timeout in jiffies
> + *
> + * Timeout is necessary to avoid to stay here forever when the chip
> + * does not answer correctly.
> + *
> + * Return: 0 on success, -ETIMEDOUT on timeout
> + */
> +static int ocores_wait(struct ocores_i2c *i2c,
> +		       int reg, u8 mask, u8 val,
> +		       const unsigned long timeout)
> +{
> +	unsigned long j;
> +
> +	j = jiffies + timeout;
> +	while (1) {
> +		u8 status = oc_getreg(i2c, reg);
> +
> +		if ((status & mask) == val)
> +			break;
> +
> +		if (time_after(jiffies, j))
> +			return -ETIMEDOUT;
> +	}
> +	return 0;
> +}
> +
> +/**
> + * Wait until is possible to process some data
> + * @i2c: ocores I2C device instance
> + *
> + * Used when the device is in polling mode (interrupts disabled).
> + *
> + * Return: 0 on success, -ETIMEDOUT on timeout
> + */
> +static int ocores_poll_wait(struct ocores_i2c *i2c)
> +{
> +	u8 mask;
> +	int err;
> +
> +	if (i2c->state == STATE_DONE || i2c->state == STATE_ERROR) {
> +		/* transfer is over */
> +		mask = OCI2C_STAT_BUSY;
> +	} else {
> +		/* on going transfer */
> +		mask = OCI2C_STAT_TIP;
> +		/*
> +		 * We wait for the data to be transfered (8bit),
> +		 * then we start polling on the ACK/NACK bit
> +		 */
> +		udelay((8 * 1000) / i2c->bus_clock_khz);
> +	}
> +
> +	/*
> +	 * once we are here we expect to get the expected result immediately
> +	 * so if after 1ms we timeout then something is broken.
> +	 */
> +	err = ocores_wait(i2c, OCI2C_STATUS, mask, 0, msecs_to_jiffies(1));
> +	if (err)
> +		dev_warn(i2c->adap.dev.parent,
> +			 "%s: STATUS timeout, bit 0x%x did not clear in 1ms\n",
> +			 __func__, mask);
> +	return err;
> +}
> +
> +
> +/**
> + * It handles an IRQ-less transfer
> + * @i2c: ocores I2C device instance
> + *
> + * Even if IRQ are disabled, the I2C OpenCore IP behavior is exactly the same
> + * (only that IRQ are not produced). This means that we can re-use entirely
> + * ocores_isr(), we just add our polling code around it.
> + *
> + * It can run in atomic context
> + */
> +static void ocores_process_polling(struct ocores_i2c *i2c)
> +{
> +	while (1) {
> +		irqreturn_t ret;
> +		int err;
> +
> +		err = ocores_poll_wait(i2c);
> +		if (err) {
> +			i2c->state = STATE_ERROR;
> +			break; /* timeout */
> +		}
> +
> +		ret = ocores_isr(-1, i2c);
> +		if (ret == IRQ_NONE)
> +			break; /* all messages have been transfered */
> +	}
> +}
> +
> +static int ocores_xfer_core(struct ocores_i2c *i2c,
> +			    struct i2c_msg *msgs, int num,
> +			    bool polling)
>  {
> -	struct ocores_i2c *i2c = i2c_get_adapdata(adap);
>  	int ret;
> +	u8 ctrl;
> +
> +	ctrl = oc_getreg(i2c, OCI2C_CONTROL);
> +	if (polling)
> +		oc_setreg(i2c, OCI2C_CONTROL, ctrl & ~OCI2C_CTRL_IEN);
> +	else
> +		oc_setreg(i2c, OCI2C_CONTROL, ctrl | OCI2C_CTRL_IEN);
>  
>  	i2c->msg = msgs;
>  	i2c->pos = 0;
> @@ -259,16 +371,37 @@ static int ocores_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
>  	oc_setreg(i2c, OCI2C_DATA, i2c_8bit_addr_from_msg(i2c->msg));
>  	oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_START);
>  
> -	ret = wait_event_timeout(i2c->wait, (i2c->state == STATE_ERROR) ||
> -				 (i2c->state == STATE_DONE), HZ);
> -	if (ret == 0) {
> -		ocores_process_timeout(i2c);
> -		return -ETIMEDOUT;
> +	if (polling) {
> +		ocores_process_polling(i2c);
> +	} else {
> +		ret = wait_event_timeout(i2c->wait,
> +					 (i2c->state == STATE_ERROR) ||
> +					 (i2c->state == STATE_DONE), HZ);
> +		if (ret == 0) {
> +			ocores_process_timeout(i2c);
> +			return -ETIMEDOUT;
> +		}
>  	}
>  
>  	return (i2c->state == STATE_DONE) ? num : -EIO;
>  }
>  
> +static int ocores_xfer_polling(struct i2c_adapter *adap,
> +			       struct i2c_msg *msgs, int num)
> +{
> +	return ocores_xfer_core(i2c_get_adapdata(adap), msgs, num, true);
> +}
> +
> +static int ocores_xfer(struct i2c_adapter *adap,
> +		       struct i2c_msg *msgs, int num)
> +{
> +	struct ocores_i2c *i2c = i2c_get_adapdata(adap);
> +
> +	if (i2c->flags & OCORES_FLAG_POLL)
> +		return ocores_xfer_polling(adap, msgs, num);
> +	return ocores_xfer_core(i2c, msgs, num, false);
> +}
> +
>  static int ocores_init(struct device *dev, struct ocores_i2c *i2c)
>  {
>  	int prescale;
> @@ -294,7 +427,7 @@ static int ocores_init(struct device *dev, struct ocores_i2c *i2c)
>  
>  	/* Init the device */
>  	oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_IACK);
> -	oc_setreg(i2c, OCI2C_CONTROL, ctrl | OCI2C_CTRL_IEN | OCI2C_CTRL_EN);
> +	oc_setreg(i2c, OCI2C_CONTROL, ctrl | OCI2C_CTRL_EN);

You fix this up in patch 5 (in what is perhaps carelessly marketed as
fixes for minor checkpatch issues), but for this patch you are actually
no longer making sure that you clear the OCI2C_CTRL_IEN bit as the code
used to before this patch. I think you intended to preserve that
behavior, no?

Cheers,
Peter

>  
>  	return 0;
>  }
> @@ -451,10 +584,6 @@ static int ocores_i2c_probe(struct platform_device *pdev)
>  	int ret;
>  	int i;
>  
> -	irq = platform_get_irq(pdev, 0);
> -	if (irq < 0)
> -		return irq;
> -
>  	i2c = devm_kzalloc(&pdev->dev, sizeof(*i2c), GFP_KERNEL);
>  	if (!i2c)
>  		return -ENOMEM;
> @@ -509,18 +638,29 @@ static int ocores_i2c_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> +	init_waitqueue_head(&i2c->wait);
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq == -ENXIO) {
> +		i2c->flags |= OCORES_FLAG_POLL;
> +	} else {
> +		if (irq < 0)
> +			return irq;
> +	}
> +
> +	if (!(i2c->flags & OCORES_FLAG_POLL)) {
> +		ret = devm_request_irq(&pdev->dev, irq, ocores_isr, 0,
> +				       pdev->name, i2c);
> +		if (ret) {
> +			dev_err(&pdev->dev, "Cannot claim IRQ\n");
> +			goto err_clk;
> +		}
> +	}
> +
>  	ret = ocores_init(&pdev->dev, i2c);
>  	if (ret)
>  		goto err_clk;
>  
> -	init_waitqueue_head(&i2c->wait);
> -	ret = devm_request_irq(&pdev->dev, irq, ocores_isr, 0,
> -			       pdev->name, i2c);
> -	if (ret) {
> -		dev_err(&pdev->dev, "Cannot claim IRQ\n");
> -		goto err_clk;
> -	}
> -
>  	/* hook up driver to tree */
>  	platform_set_drvdata(pdev, i2c);
>  	i2c->adap = ocores_adapter;
> 

Re: [PATCH v4 1/5] i2c:ocores: stop transfer on timeout

[Peter Rosin]

On 2019-02-11 09:31, Federico Vaga wrote:
> Detecting a timeout is ok, but we also need to assert a STOP command on
> the bus in order to prevent it from generating interrupts when there are
> no on going transfers.
> 
> Example: very long transmission.
> 
> 1. ocores_xfer: START a transfer
> 2. ocores_isr : handle byte by byte the transfer
> 3. ocores_xfer: goes in timeout [[bugfix here]]
> 4. ocores_xfer: return to I2C subsystem and to the I2C driver
> 5. I2C driver : it may clean up the i2c_msg memory
> 6. ocores_isr : receives another interrupt (pending bytes to be
>                 transferred) but the i2c_msg memory is invalid now
> 
> So, since the transfer was too long, we have to detect the timeout and
> STOP the transfer.
> 
> Another point is that we have a critical region here. When handling the
> timeout condition we may have a running IRQ handler. For this reason I
> introduce a spinlock.
> 
> In order to make easier to understan locking I have:
> - added a new function to handle timeout
> - modified the current ocores_process() function in order to be protected
>   by the new spinlock
> Like this it is obvious at first sight that this locking serializes
> the execution of ocores_process() and ocores_process_timeout()
> 

*snip*

> @@ -184,14 +197,14 @@ static void ocores_process(struct ocores_i2c *i2c)
>  
>  				oc_setreg(i2c, OCI2C_DATA, addr);
>  				oc_setreg(i2c, OCI2C_CMD,  OCI2C_CMD_START);

Didn't checkpatch complain about the double space? Fixing it fits in
patch 5...

Cheers,
Peter

Re: [PATCH v4 5/5] i2c:ocores: checkpatch fixes

[Peter Rosin]

On 2019-02-11 09:31, Federico Vaga wrote:
> Miscellaneous style fixes from checkpatch
> 
> Signed-off-by: Federico Vaga <federico.vaga@cern.ch>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> 
> ---
>  drivers/i2c/busses/i2c-ocores.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
> index 5b80190..ba35d2a 100644
> --- a/drivers/i2c/busses/i2c-ocores.c
> +++ b/drivers/i2c/busses/i2c-ocores.c
> @@ -182,8 +182,9 @@ static void ocores_process(struct ocores_i2c *i2c, u8 stat)
>  			oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_STOP);
>  			goto out;
>  		}
> -	} else
> +	} else {
>  		msg->buf[i2c->pos++] = oc_getreg(i2c, OCI2C_DATA);
> +	}
>  
>  	/* end of msg? */
>  	if (i2c->pos == msg->len) {
> @@ -202,9 +203,9 @@ static void ocores_process(struct ocores_i2c *i2c, u8 stat)
>  				oc_setreg(i2c, OCI2C_DATA, addr);
>  				oc_setreg(i2c, OCI2C_CMD,  OCI2C_CMD_START);
>  				goto out;
> -			} else
> -				i2c->state = (msg->flags & I2C_M_RD)
> -					? STATE_READ : STATE_WRITE;
> +			}
> +			i2c->state = (msg->flags & I2C_M_RD)
> +				? STATE_READ : STATE_WRITE;
>  		} else {
>  			i2c->state = STATE_DONE;
>  			oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_STOP);
> @@ -405,7 +406,8 @@ static int ocores_init(struct device *dev, struct ocores_i2c *i2c)
>  	u8 ctrl = oc_getreg(i2c, OCI2C_CONTROL);
>  
>  	/* make sure the device is disabled */
> -	oc_setreg(i2c, OCI2C_CONTROL, ctrl & ~(OCI2C_CTRL_EN|OCI2C_CTRL_IEN));
> +	ctrl &= ~(OCI2C_CTRL_EN | OCI2C_CTRL_IEN);
> +	oc_setreg(i2c, OCI2C_CONTROL, ctrl);

The pattern ~(OCI2C_CTRL_EN|OCI2C_CTRL_IEN) without spaces around the | exists
in a couple of other places in the driver (at least the version I'm looking at).
You could fix all instances while at it.

Cheers,
Peter

>  
>  	prescale = (i2c->ip_clock_khz / (5 * i2c->bus_clock_khz)) - 1;
>  	prescale = clamp(prescale, 0, 0xffff);
> @@ -462,11 +464,13 @@ MODULE_DEVICE_TABLE(of, ocores_i2c_match);
>  #ifdef CONFIG_OF
>  /* Read and write functions for the GRLIB port of the controller. Registers are
>   * 32-bit big endian and the PRELOW and PREHIGH registers are merged into one
> - * register. The subsequent registers has their offset decreased accordingly. */
> + * register. The subsequent registers has their offset decreased accordingly.
> + */
>  static u8 oc_getreg_grlib(struct ocores_i2c *i2c, int reg)
>  {
>  	u32 rd;
>  	int rreg = reg;
> +
>  	if (reg != OCI2C_PRELOW)
>  		rreg--;
>  	rd = ioread32be(i2c->base + (rreg << i2c->reg_shift));
> @@ -480,6 +484,7 @@ static void oc_setreg_grlib(struct ocores_i2c *i2c, int reg, u8 value)
>  {
>  	u32 curr, wr;
>  	int rreg = reg;
> +
>  	if (reg != OCI2C_PRELOW)
>  		rreg--;
>  	if (reg == OCI2C_PRELOW || reg == OCI2C_PREHIGH) {
> @@ -568,7 +573,7 @@ static int ocores_i2c_of_probe(struct platform_device *pdev,
>  	return 0;
>  }
>  #else
> -#define ocores_i2c_of_probe(pdev,i2c) -ENODEV
> +#define ocores_i2c_of_probe(pdev, i2c) -ENODEV
>  #endif
>  
>  static int ocores_i2c_probe(struct platform_device *pdev)
> 

Re: [PATCH v4 1/5] i2c:ocores: stop transfer on timeout

[Federico Vaga]

On Monday, February 11, 2019 11:44:46 AM CET Peter Rosin wrote:
> On 2019-02-11 09:31, Federico Vaga wrote:
> 
> > Detecting a timeout is ok, but we also need to assert a STOP command on
> > the bus in order to prevent it from generating interrupts when there are
> > no on going transfers.
> > 
> > Example: very long transmission.
> > 
> > 1. ocores_xfer: START a transfer
> > 2. ocores_isr : handle byte by byte the transfer
> > 3. ocores_xfer: goes in timeout [[bugfix here]]
> > 4. ocores_xfer: return to I2C subsystem and to the I2C driver
> > 5. I2C driver : it may clean up the i2c_msg memory
> > 6. ocores_isr : receives another interrupt (pending bytes to be
> > 
> >                 transferred) but the i2c_msg memory is invalid now
> > 
> > 
> > So, since the transfer was too long, we have to detect the timeout and
> > STOP the transfer.
> > 
> > Another point is that we have a critical region here. When handling the
> > timeout condition we may have a running IRQ handler. For this reason I
> > introduce a spinlock.
> > 
> > In order to make easier to understan locking I have:
> > - added a new function to handle timeout
> > - modified the current ocores_process() function in order to be protected
> > 
> >   by the new spinlock
> > 
> > Like this it is obvious at first sight that this locking serializes
> > the execution of ocores_process() and ocores_process_timeout()
> > 
> 
> 
> *snip*
> 
> 
> > @@ -184,14 +197,14 @@ static void ocores_process(struct ocores_i2c *i2c)
> > 
> >  
> >  
> >  				oc_setreg(i2c, OCI2C_DATA, addr);
> >  				oc_setreg(i2c, OCI2C_CMD,  OCI2C_CMD_START);
> 
> 
> Didn't checkpatch complain about the double space? Fixing it fits in
> patch 5...

Apparently not, I will add the fix the checkpatch PATCH

> Cheers,
> Peter

Re: [PATCH v4 3/5] i2c:ocores: add polling interface

[Federico Vaga]

On Monday, February 11, 2019 11:43:45 AM CET Peter Rosin wrote:
> On 2019-02-11 09:31, Federico Vaga wrote:
> 
> > This driver assumes that an interrupt line is always available for
> > the I2C master. This is not always the case and this patch adds support
> > for a polling version.
> > 
> > Report from Andrew Lunn:
> > 
> > 
> >   I did some timing tests for this. On my box, we request a udelay of
> >   80uS. The kernel actually delays for about 79uS. We then spin in
> >   ocores_wait() for an additional 10-11uS, which is 3 to 4 iterations.
> > 
> > 
> > 
> >   There are actually 9 bits on the wire, not 8, since there is an
> >   ACK/NACK bit after the actual data transfer. So i changed the delay to
> >   (9 * 1000) / i2c->bus_clock_khz. That resulted in ocores_wait() mostly
> >   not looping at all. But for reading an 4K AT24 EEPROM, it increased
> >   the read time by 10ms, from 424ms to 434ms. So we should probably keep
> >   with 8.
> > 
> > 
> > Signed-off-by: Federico Vaga <federico.vaga@cern.ch>
> > Tested-by: Andrew Lunn <andrew@lunn.ch>
> > 
> > ---
> > 
> >  drivers/i2c/busses/i2c-ocores.c | 180
> >  +++++++++++++++++++++++++++++++++++-----
 1 file changed, 160
> >  insertions(+), 20 deletions(-)
> > 
> > 
> > diff --git a/drivers/i2c/busses/i2c-ocores.c
> > b/drivers/i2c/busses/i2c-ocores.c
 index fcc2558..d42a324 100644
> > --- a/drivers/i2c/busses/i2c-ocores.c
> > +++ b/drivers/i2c/busses/i2c-ocores.c
> > @@ -13,6 +13,7 @@
> > 
> >   */
> >  
> >  
> >  #include <linux/clk.h>
> > 
> > +#include <linux/delay.h>
> > 
> >  #include <linux/err.h>
> >  #include <linux/kernel.h>
> >  #include <linux/module.h>
> > 
> > @@ -26,6 +27,9 @@
> > 
> >  #include <linux/io.h>
> >  #include <linux/log2.h>
> >  #include <linux/spinlock.h>
> > 
> > +#include <linux/jiffies.h>
> > +
> > +#define OCORES_FLAG_POLL BIT(0)
> > 
> >  
> >  /**
> >  
> >   * @process_lock: protect I2C transfer process.
> > 
> > @@ -35,6 +39,7 @@ struct ocores_i2c {
> > 
> >  	void __iomem *base;
> >  	u32 reg_shift;
> >  	u32 reg_io_width;
> > 
> > +	unsigned long flags;
> > 
> >  	wait_queue_head_t wait;
> >  	struct i2c_adapter adap;
> >  	struct i2c_msg *msg;
> > 
> > @@ -246,10 +251,117 @@ static void ocores_process_timeout(struct
> > ocores_i2c *i2c)
> 
> >  	spin_unlock_irqrestore(&i2c->process_lock, flags);
> >  
> >  }
> >  
> > 
> > -static int ocores_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> > int num)
 +/**
> > + * Wait until something change in a given register
> > + * @i2c: ocores I2C device instance
> > + * @reg: register to query
> > + * @mask: bitmask to apply on register value
> > + * @val: expected result
> > + * @timeout: timeout in jiffies
> > + *
> > + * Timeout is necessary to avoid to stay here forever when the chip
> > + * does not answer correctly.
> > + *
> > + * Return: 0 on success, -ETIMEDOUT on timeout
> > + */
> > +static int ocores_wait(struct ocores_i2c *i2c,
> > +		       int reg, u8 mask, u8 val,
> > +		       const unsigned long timeout)
> > +{
> > +	unsigned long j;
> > +
> > +	j = jiffies + timeout;
> > +	while (1) {
> > +		u8 status = oc_getreg(i2c, reg);
> > +
> > +		if ((status & mask) == val)
> > +			break;
> > +
> > +		if (time_after(jiffies, j))
> > +			return -ETIMEDOUT;
> > +	}
> > +	return 0;
> > +}
> > +
> > +/**
> > + * Wait until is possible to process some data
> > + * @i2c: ocores I2C device instance
> > + *
> > + * Used when the device is in polling mode (interrupts disabled).
> > + *
> > + * Return: 0 on success, -ETIMEDOUT on timeout
> > + */
> > +static int ocores_poll_wait(struct ocores_i2c *i2c)
> > +{
> > +	u8 mask;
> > +	int err;
> > +
> > +	if (i2c->state == STATE_DONE || i2c->state == STATE_ERROR) {
> > +		/* transfer is over */
> > +		mask = OCI2C_STAT_BUSY;
> > +	} else {
> > +		/* on going transfer */
> > +		mask = OCI2C_STAT_TIP;
> > +		/*
> > +		 * We wait for the data to be transfered (8bit),
> > +		 * then we start polling on the ACK/NACK bit
> > +		 */
> > +		udelay((8 * 1000) / i2c->bus_clock_khz);
> > +	}
> > +
> > +	/*
> > +	 * once we are here we expect to get the expected result immediately
> > +	 * so if after 1ms we timeout then something is broken.
> > +	 */
> > +	err = ocores_wait(i2c, OCI2C_STATUS, mask, 0, msecs_to_jiffies(1));
> > +	if (err)
> > +		dev_warn(i2c->adap.dev.parent,
> > +			 "%s: STATUS timeout, bit 0x%x did not clear in 1ms\n",
> > +			 __func__, mask);
> > +	return err;
> > +}
> > +
> > +
> > +/**
> > + * It handles an IRQ-less transfer
> > + * @i2c: ocores I2C device instance
> > + *
> > + * Even if IRQ are disabled, the I2C OpenCore IP behavior is exactly the
> > same
 + * (only that IRQ are not produced). This means that we can re-use
> > entirely + * ocores_isr(), we just add our polling code around it.
> > + *
> > + * It can run in atomic context
> > + */
> > +static void ocores_process_polling(struct ocores_i2c *i2c)
> > +{
> > +	while (1) {
> > +		irqreturn_t ret;
> > +		int err;
> > +
> > +		err = ocores_poll_wait(i2c);
> > +		if (err) {
> > +			i2c->state = STATE_ERROR;
> > +			break; /* timeout */
> > +		}
> > +
> > +		ret = ocores_isr(-1, i2c);
> > +		if (ret == IRQ_NONE)
> > +			break; /* all messages have been transfered */
> > +	}
> > +}
> > +
> > +static int ocores_xfer_core(struct ocores_i2c *i2c,
> > +			    struct i2c_msg *msgs, int num,
> > +			    bool polling)
> > 
> >  {
> > 
> > -	struct ocores_i2c *i2c = i2c_get_adapdata(adap);
> > 
> >  	int ret;
> > 
> > +	u8 ctrl;
> > +
> > +	ctrl = oc_getreg(i2c, OCI2C_CONTROL);
> > +	if (polling)
> > +		oc_setreg(i2c, OCI2C_CONTROL, ctrl & ~OCI2C_CTRL_IEN);
> > +	else
> > +		oc_setreg(i2c, OCI2C_CONTROL, ctrl | OCI2C_CTRL_IEN);
> > 
> >  
> >  
> >  	i2c->msg = msgs;
> >  	i2c->pos = 0;
> > 
> > @@ -259,16 +371,37 @@ static int ocores_xfer(struct i2c_adapter *adap,
> > struct i2c_msg *msgs, int num)
> 
> >  	oc_setreg(i2c, OCI2C_DATA, i2c_8bit_addr_from_msg(i2c->msg));
> >  	oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_START);
> >  
> >  
> > 
> > -	ret = wait_event_timeout(i2c->wait, (i2c->state == STATE_ERROR) ||
> > -				 (i2c->state == STATE_DONE), HZ);
> > -	if (ret == 0) {
> > -		ocores_process_timeout(i2c);
> > -		return -ETIMEDOUT;
> > +	if (polling) {
> > +		ocores_process_polling(i2c);
> > +	} else {
> > +		ret = wait_event_timeout(i2c->wait,
> > +					 (i2c->state == STATE_ERROR) ||
> > +					 (i2c->state == STATE_DONE), HZ);
> > +		if (ret == 0) {
> > +			ocores_process_timeout(i2c);
> > +			return -ETIMEDOUT;
> > +		}
> > 
> >  	}
> >  
> >  
> >  
> >  	return (i2c->state == STATE_DONE) ? num : -EIO;
> >  
> >  }
> >  
> > 
> > +static int ocores_xfer_polling(struct i2c_adapter *adap,
> > +			       struct i2c_msg *msgs, int num)
> > +{
> > +	return ocores_xfer_core(i2c_get_adapdata(adap), msgs, num, true);
> > +}
> > +
> > +static int ocores_xfer(struct i2c_adapter *adap,
> > +		       struct i2c_msg *msgs, int num)
> > +{
> > +	struct ocores_i2c *i2c = i2c_get_adapdata(adap);
> > +
> > +	if (i2c->flags & OCORES_FLAG_POLL)
> > +		return ocores_xfer_polling(adap, msgs, num);
> > +	return ocores_xfer_core(i2c, msgs, num, false);
> > +}
> > +
> > 
> >  static int ocores_init(struct device *dev, struct ocores_i2c *i2c)
> >  {
> >  
> >  	int prescale;
> > 
> > @@ -294,7 +427,7 @@ static int ocores_init(struct device *dev, struct
> > ocores_i2c *i2c)
> 
> >  
> >  
> >  	/* Init the device */
> >  	oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_IACK);
> > 
> > -	oc_setreg(i2c, OCI2C_CONTROL, ctrl | OCI2C_CTRL_IEN | OCI2C_CTRL_EN);
> > +	oc_setreg(i2c, OCI2C_CONTROL, ctrl | OCI2C_CTRL_EN);
> 
> 
> You fix this up in patch 5 (in what is perhaps carelessly marketed as
> fixes for minor checkpatch issues), but for this patch you are actually
> no longer making sure that you clear the OCI2C_CTRL_IEN bit as the code
> used to before this patch. I think you intended to preserve that
> behavior, no?

I think you got confused by the two patches because those lines look very 
similar.

In PATCH 5 what you see is the "style fix" to clear EN and IEN before clock 
configuration

in PATCH 3 (this one) what you see is when later (same function, after clock 
configuration) the device is re-enabled (EN), but without enabling the 
interrupt because on polling configuration we do not want to see interrupt 
flowing.

So, the behavior is preserved for what concern clearing the IEN bit before 
clock configuration.

am I wrong?

> 
> Cheers,
> Peter
> 
> 
> >  
> >  
> >  	return 0;
> >  
> >  }
> > 
> > @@ -451,10 +584,6 @@ static int ocores_i2c_probe(struct platform_device
> > *pdev)
> 
> >  	int ret;
> >  	int i;
> >  
> >  
> > 
> > -	irq = platform_get_irq(pdev, 0);
> > -	if (irq < 0)
> > -		return irq;
> > -
> > 
> >  	i2c = devm_kzalloc(&pdev->dev, sizeof(*i2c), GFP_KERNEL);
> >  	if (!i2c)
> >  	
> >  		return -ENOMEM;
> > 
> > @@ -509,18 +638,29 @@ static int ocores_i2c_probe(struct platform_device
> > *pdev)
> 
> >  		}
> >  	
> >  	}
> >  
> >  
> > 
> > +	init_waitqueue_head(&i2c->wait);
> > +
> > +	irq = platform_get_irq(pdev, 0);
> > +	if (irq == -ENXIO) {
> > +		i2c->flags |= OCORES_FLAG_POLL;
> > +	} else {
> > +		if (irq < 0)
> > +			return irq;
> > +	}
> > +
> > +	if (!(i2c->flags & OCORES_FLAG_POLL)) {
> > +		ret = devm_request_irq(&pdev->dev, irq, ocores_isr, 0,
> > +				       pdev->name, i2c);
> > +		if (ret) {
> > +			dev_err(&pdev->dev, "Cannot claim IRQ\n");
> > +			goto err_clk;
> > +		}
> > +	}
> > +
> > 
> >  	ret = ocores_init(&pdev->dev, i2c);
> >  	if (ret)
> >  	
> >  		goto err_clk;
> >  
> >  
> > 
> > -	init_waitqueue_head(&i2c->wait);
> > -	ret = devm_request_irq(&pdev->dev, irq, ocores_isr, 0,
> > -			       pdev->name, i2c);
> > -	if (ret) {
> > -		dev_err(&pdev->dev, "Cannot claim IRQ\n");
> > -		goto err_clk;
> > -	}
> > -
> > 
> >  	/* hook up driver to tree */
> >  	platform_set_drvdata(pdev, i2c);
> >  	i2c->adap = ocores_adapter;
> > 
> > 
> 
> 

Re: [PATCH v4 3/5] i2c:ocores: add polling interface

[Peter Rosin]

>>> @@ -294,7 +427,7 @@ static int ocores_init(struct device *dev, struct
>>> ocores_i2c *i2c)
>>
>>>  
>>>  
>>>  	/* Init the device */
>>>  	oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_IACK);
>>>
>>> -	oc_setreg(i2c, OCI2C_CONTROL, ctrl | OCI2C_CTRL_IEN | OCI2C_CTRL_EN);
>>> +	oc_setreg(i2c, OCI2C_CONTROL, ctrl | OCI2C_CTRL_EN);
>>
>>
>> You fix this up in patch 5 (in what is perhaps carelessly marketed as
>> fixes for minor checkpatch issues), but for this patch you are actually
>> no longer making sure that you clear the OCI2C_CTRL_IEN bit as the code
>> used to before this patch. I think you intended to preserve that
>> behavior, no?
> 
> I think you got confused by the two patches because those lines look very 
> similar.
> 
> In PATCH 5 what you see is the "style fix" to clear EN and IEN before clock 
> configuration
> 
> in PATCH 3 (this one) what you see is when later (same function, after clock 
> configuration) the device is re-enabled (EN), but without enabling the 
> interrupt because on polling configuration we do not want to see interrupt 
> flowing.
> 
> So, the behavior is preserved for what concern clearing the IEN bit before 
> clock configuration.
> 
> am I wrong?

No, I don't think I'm confused and I think you are wrong. Current code does
this:

	u8 ctrl = oc_getreg(i2c, OCI2C_CONTROL);

	/* make sure the device is disabled */
	oc_setreg(i2c, OCI2C_CONTROL, ctrl & ~(OCI2C_CTRL_IEN|OCI2C_CTRL_EN));
	...
	oc_setreg(i2c, OCI2C_CONTROL, ctrl | OCI2C_CTRL_IEN | OCI2C_CTRL_EN));

Here, the final oc_setreg always sets OCI2C_CTRL_IEN.


Patch 3 changes it to:

	u8 ctrl = oc_getreg(i2c, OCI2C_CONTROL);

	/* make sure the device is disabled */
	oc_setreg(i2c, OCI2C_CONTROL, ctrl & ~(OCI2C_CTRL_IEN|OCI2C_CTRL_EN));
	...
	oc_setreg(i2c, OCI2C_CONTROL, ctrl | OCI2C_CTRL_EN));

Here, the final oc_setreg restores OCI2C_CTRL_IEN to whatever it was
at function entry.


And patch 5 changes it again to:

	u8 ctrl = oc_getreg(i2c, OCI2C_CONTROL);

	/* make sure the device is disabled */
	ctrl &= ~(OCI2C_CTRL_IEN | OCI2C_CTRL_EN);
	oc_setreg(i2c, OCI2C_CONTROL, ctrl);
	...
	oc_setreg(i2c, OCI2C_CONTROL, ctrl | OCI2C_CTRL_EN));

Here, the final oc_setreg keeps OCI2C_CTRL_IEN cleared. I think you wanted
this deterministic behavior for patch 3 as well.

Cheers,
Peter

Re: [PATCH v4 3/5] i2c:ocores: add polling interface

[Federico Vaga]

On Monday, February 11, 2019 2:35:15 PM CET Peter Rosin wrote:
> >>> @@ -294,7 +427,7 @@ static int ocores_init(struct device *dev, struct
> >>> ocores_i2c *i2c)
> >>
> >>
> >>
> >>>  
> >>>  
> >>>  
> >>>  	/* Init the device */
> >>>  	oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_IACK);
> >>>
> >>>
> >>>
> >>> -	oc_setreg(i2c, OCI2C_CONTROL, ctrl | OCI2C_CTRL_IEN | OCI2C_CTRL_EN);
> >>> +	oc_setreg(i2c, OCI2C_CONTROL, ctrl | OCI2C_CTRL_EN);
> >>
> >>
> >>
> >>
> >> You fix this up in patch 5 (in what is perhaps carelessly marketed as
> >> fixes for minor checkpatch issues), but for this patch you are actually
> >> no longer making sure that you clear the OCI2C_CTRL_IEN bit as the code
> >> used to before this patch. I think you intended to preserve that
> >> behavior, no?
> > 
> > 
> > I think you got confused by the two patches because those lines look very
> > 
> > similar.
> > 
> > In PATCH 5 what you see is the "style fix" to clear EN and IEN before
> > clock 
 configuration
> > 
> > in PATCH 3 (this one) what you see is when later (same function, after
> > clock 
 configuration) the device is re-enabled (EN), but without
> > enabling the interrupt because on polling configuration we do not want to
> > see interrupt flowing.
> > 
> > So, the behavior is preserved for what concern clearing the IEN bit before
> > 
 clock configuration.
> > 
> > am I wrong?
> 
> 
> No, I don't think I'm confused and I think you are wrong. Current code does
> this:
> 
> 	u8 ctrl = oc_getreg(i2c, OCI2C_CONTROL);
> 
> 	/* make sure the device is disabled */
> 	oc_setreg(i2c, OCI2C_CONTROL, ctrl & ~(OCI2C_CTRL_IEN|OCI2C_CTRL_EN));
> 	...
> 	oc_setreg(i2c, OCI2C_CONTROL, ctrl | OCI2C_CTRL_IEN | OCI2C_CTRL_EN));
> 
> Here, the final oc_setreg always sets OCI2C_CTRL_IEN.
> 
> Patch 3 changes it to:
> 
> 	u8 ctrl = oc_getreg(i2c, OCI2C_CONTROL);
> 
> 	/* make sure the device is disabled */
> 	oc_setreg(i2c, OCI2C_CONTROL, ctrl & ~(OCI2C_CTRL_IEN|OCI2C_CTRL_EN));
> 	...
> 	oc_setreg(i2c, OCI2C_CONTROL, ctrl | OCI2C_CTRL_EN));
> 
> Here, the final oc_setreg restores OCI2C_CTRL_IEN to whatever it was
> at function entry.
> 
> 
> And patch 5 changes it again to:
> 
> 	u8 ctrl = oc_getreg(i2c, OCI2C_CONTROL);
> 
> 	/* make sure the device is disabled */
> 	ctrl &= ~(OCI2C_CTRL_IEN | OCI2C_CTRL_EN);
> 	oc_setreg(i2c, OCI2C_CONTROL, ctrl);
> 	...
> 	oc_setreg(i2c, OCI2C_CONTROL, ctrl | OCI2C_CTRL_EN));
> 
> Here, the final oc_setreg keeps OCI2C_CTRL_IEN cleared. I think you wanted
> this deterministic behavior for patch 3 as well.

Now I see what you mean and I agree. I will move the fix from PATCH 5 to PATCH 
3, so that bit IEN is clearly cleared.


thanks

> 
> Cheers,
> Peter

Re: [PATCH v4 3/5] i2c:ocores: add polling interface

[Federico Vaga]

On Monday, February 11, 2019 11:25:26 AM CET Wolfram Sang wrote:
> On Mon, Feb 11, 2019 at 09:31:20AM +0100, Federico Vaga wrote:
> > This driver assumes that an interrupt line is always available for
> > the I2C master. This is not always the case and this patch adds support
> > for a polling version.
> > 
> > Report from Andrew Lunn:
> >   I did some timing tests for this. On my box, we request a udelay of
> >   80uS. The kernel actually delays for about 79uS. We then spin in
> >   ocores_wait() for an additional 10-11uS, which is 3 to 4 iterations.
> >   
> >   There are actually 9 bits on the wire, not 8, since there is an
> >   ACK/NACK bit after the actual data transfer. So i changed the delay to
> >   (9 * 1000) / i2c->bus_clock_khz. That resulted in ocores_wait() mostly
> >   not looping at all. But for reading an 4K AT24 EEPROM, it increased
> >   the read time by 10ms, from 424ms to 434ms. So we should probably keep
> >   with 8.
> > 
> > Signed-off-by: Federico Vaga <federico.vaga@cern.ch>
> > Tested-by: Andrew Lunn <andrew@lunn.ch>
> 
> Fixed these checkpatch warnings:
> 
> WARNING: 'transfered' may be misspelled - perhaps 'transferred'?
> #111: FILE: drivers/i2c/busses/i2c-ocores.c:306:
> +		 * We wait for the data to be transfered (8bit),
> 
> CHECK: Please don't use multiple blank lines
> #129: FILE: drivers/i2c/busses/i2c-ocores.c:324:
> +
> +
> 
> WARNING: 'transfered' may be misspelled - perhaps 'transferred'?
> #154: FILE: drivers/i2c/busses/i2c-ocores.c:349:
> +			break; /* all messages have been transfered */
> 
> and applied to for-next, thanks!

I will resend this patch as v5 to add the fix suggested by Peter Rosin

Re: [PATCH v4 1/5] i2c:ocores: stop transfer on timeout

[Andrew Lunn]

> Applied to for-next, thanks!

Hi Wolfram

Could you drop these patches and wait for a new version?  I don't
think you have pushed it out yet? So it won't be a visible rebase.

Thanks
	Andrew

Re: [PATCH v4 1/5] i2c:ocores: stop transfer on timeout

[Federico Vaga]

On Monday, February 11, 2019 3:01:38 PM CET Andrew Lunn wrote:
> > Applied to for-next, thanks!
> 
> Hi Wolfram
> 
> Could you drop these patches and wait for a new version?  I don't
> think you have pushed it out yet? So it won't be a visible rebase.

I will wait to send v5: full patch set, or just PATCH 3 and 5 ?

> 
> Thanks
> 	Andrew

Re: [PATCH v4 1/5] i2c:ocores: stop transfer on timeout

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 250 bytes --]


> Could you drop these patches and wait for a new version?  I don't
> think you have pushed it out yet? So it won't be a visible rebase.

Yes, I can do that.

When resending, just make sure you include the fixes I mentioned when
applying.

Thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]